| Message ID | 20210112132339.5621-5-ezequiel@collabora.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | V4L2 Async notifier API cleanup | expand |
Hi Ezequiel On Tue, Jan 12, 2021 at 10:23:30AM -0300, Ezequiel Garcia wrote: > The use of v4l2_async_notifier_add_subdev is discouraged. > Drivers are instead encouraged to use a helper such as > v4l2_async_notifier_add_fwnode_remote_subdev. > > This fixes a misuse of the API, as v4l2_async_notifier_add_subdev > should get a kmalloc'ed struct v4l2_async_subdev, > removing some boilerplate code while at it. > > Use the appropriate helper v4l2_async_notifier_add_fwnode_remote_subdev, > which handles the needed setup, instead of open-coding it. > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > drivers/media/platform/exynos4-is/media-dev.c | 25 +++++++++---------- > drivers/media/platform/exynos4-is/media-dev.h | 2 +- > 2 files changed, 13 insertions(+), 14 deletions(-) > > diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c > index e636c33e847b..196166a9a4e5 100644 > --- a/drivers/media/platform/exynos4-is/media-dev.c > +++ b/drivers/media/platform/exynos4-is/media-dev.c > @@ -401,6 +401,7 @@ static int fimc_md_parse_one_endpoint(struct fimc_md *fmd, > int index = fmd->num_sensors; > struct fimc_source_info *pd = &fmd->sensor[index].pdata; > struct device_node *rem, *np; > + struct v4l2_async_subdev *asd; > struct v4l2_fwnode_endpoint endpoint = { .bus_type = 0 }; > int ret; > > @@ -418,7 +419,6 @@ static int fimc_md_parse_one_endpoint(struct fimc_md *fmd, > pd->mux_id = (endpoint.base.port - 1) & 0x1; > > rem = of_graph_get_remote_port_parent(ep); > - of_node_put(ep); If you remove it from here, don't forget to put it in the here below error path > if (rem == NULL) { > v4l2_info(&fmd->v4l2_dev, "Remote device at %pOF not found\n", > ep); > @@ -450,6 +450,7 @@ static int fimc_md_parse_one_endpoint(struct fimc_md *fmd, > * checking parent's node name. > */ > np = of_get_parent(rem); > + of_node_put(rem); unrelated but good > > if (of_node_name_eq(np, "i2c-isp")) > pd->fimc_bus_type = FIMC_BUS_TYPE_ISP_WRITEBACK; > @@ -457,21 +458,18 @@ static int fimc_md_parse_one_endpoint(struct fimc_md *fmd, > pd->fimc_bus_type = pd->sensor_bus_type; > of_node_put(np); > > - if (WARN_ON(index >= ARRAY_SIZE(fmd->sensor))) { > - of_node_put(rem); I think if you need to keep 'ep' around until the call to v4l2_async_notifier_add_fwnode_remote_subdev() below, it should be put here as you remove the above of_node_put(ep). I wonder if registering the async subdev before parsing the endpoint would make things simpler. Not required for this patch though. > + if (WARN_ON(index >= ARRAY_SIZE(fmd->sensor))) > return -EINVAL; > - } > > - fmd->sensor[index].asd.match_type = V4L2_ASYNC_MATCH_FWNODE; > - fmd->sensor[index].asd.match.fwnode = of_fwnode_handle(rem); > + asd = v4l2_async_notifier_add_fwnode_remote_subdev( > + &fmd->subdev_notifier, of_fwnode_handle(ep), sizeof(*asd)); > > - ret = v4l2_async_notifier_add_subdev(&fmd->subdev_notifier, > - &fmd->sensor[index].asd); > - if (ret) { > - of_node_put(rem); > - return ret; > - } > + of_node_put(ep); > + > + if (IS_ERR(asd)) > + return PTR_ERR(asd); > > + fmd->sensor[index].asd = asd; > fmd->num_sensors++; > > return 0; > @@ -1381,7 +1379,8 @@ static int subdev_notifier_bound(struct v4l2_async_notifier *notifier, > > /* Find platform data for this sensor subdev */ > for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++) > - if (fmd->sensor[i].asd.match.fwnode == > + if (fmd->sensor[i].asd && Is this needed ? If the subdev has bound the async subdev has been allocated correctly, doesn't it ? With the ep ref counting clarified Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org> Thanks j > + fmd->sensor[i].asd->match.fwnode == > of_fwnode_handle(subdev->dev->of_node)) > si = &fmd->sensor[i]; > > diff --git a/drivers/media/platform/exynos4-is/media-dev.h b/drivers/media/platform/exynos4-is/media-dev.h > index 9447fafe23c6..a3876d668ea6 100644 > --- a/drivers/media/platform/exynos4-is/media-dev.h > +++ b/drivers/media/platform/exynos4-is/media-dev.h > @@ -83,7 +83,7 @@ struct fimc_camclk_info { > */ > struct fimc_sensor_info { > struct fimc_source_info pdata; > - struct v4l2_async_subdev asd; > + struct v4l2_async_subdev *asd; > struct v4l2_subdev *subdev; > struct fimc_dev *host; > }; > -- > 2.29.2 >
On Sat, 2021-01-16 at 17:07 +0100, Jacopo Mondi wrote: > Hi Ezequiel > > On Tue, Jan 12, 2021 at 10:23:30AM -0300, Ezequiel Garcia wrote: > > The use of v4l2_async_notifier_add_subdev is discouraged. > > Drivers are instead encouraged to use a helper such as > > v4l2_async_notifier_add_fwnode_remote_subdev. > > > > This fixes a misuse of the API, as v4l2_async_notifier_add_subdev > > should get a kmalloc'ed struct v4l2_async_subdev, > > removing some boilerplate code while at it. > > > > Use the appropriate helper v4l2_async_notifier_add_fwnode_remote_subdev, > > which handles the needed setup, instead of open-coding it. > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > --- > > drivers/media/platform/exynos4-is/media-dev.c | 25 +++++++++---------- > > drivers/media/platform/exynos4-is/media-dev.h | 2 +- > > 2 files changed, 13 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c > > index e636c33e847b..196166a9a4e5 100644 > > --- a/drivers/media/platform/exynos4-is/media-dev.c > > +++ b/drivers/media/platform/exynos4-is/media-dev.c > > @@ -401,6 +401,7 @@ static int fimc_md_parse_one_endpoint(struct fimc_md *fmd, > > int index = fmd->num_sensors; > > struct fimc_source_info *pd = &fmd->sensor[index].pdata; > > struct device_node *rem, *np; > > + struct v4l2_async_subdev *asd; > > struct v4l2_fwnode_endpoint endpoint = { .bus_type = 0 }; > > int ret; > > > > @@ -418,7 +419,6 @@ static int fimc_md_parse_one_endpoint(struct fimc_md *fmd, > > pd->mux_id = (endpoint.base.port - 1) & 0x1; > > > > rem = of_graph_get_remote_port_parent(ep); > > - of_node_put(ep); > > If you remove it from here, don't forget to put it in the here below > error path > Oops, think you are right. > > if (rem == NULL) { > > > v4l2_info(&fmd->v4l2_dev, "Remote device at %pOF not found\n", > > ep); > > @@ -450,6 +450,7 @@ static int fimc_md_parse_one_endpoint(struct fimc_md *fmd, > > * checking parent's node name. > > */ > > np = of_get_parent(rem); > > + of_node_put(rem); > > unrelated but good > > > > if (of_node_name_eq(np, "i2c-isp")) > > pd->fimc_bus_type = FIMC_BUS_TYPE_ISP_WRITEBACK; > > @@ -457,21 +458,18 @@ static int fimc_md_parse_one_endpoint(struct fimc_md *fmd, > > pd->fimc_bus_type = pd->sensor_bus_type; > > of_node_put(np); > > > > - if (WARN_ON(index >= ARRAY_SIZE(fmd->sensor))) { > > - of_node_put(rem); > > I think if you need to keep 'ep' around until the call to > v4l2_async_notifier_add_fwnode_remote_subdev() below, it should be put > here as you remove the above of_node_put(ep). > > I wonder if registering the async subdev before parsing the endpoint > would make things simpler. Not required for this patch though. > I have tried to make these conversions simple, and let the people with hardware do more interesting cleanups. > > + if (WARN_ON(index >= ARRAY_SIZE(fmd->sensor))) > > return -EINVAL; > > - } > > > > - fmd->sensor[index].asd.match_type = V4L2_ASYNC_MATCH_FWNODE; > > - fmd->sensor[index].asd.match.fwnode = of_fwnode_handle(rem); > > + asd = v4l2_async_notifier_add_fwnode_remote_subdev( > > + &fmd->subdev_notifier, of_fwnode_handle(ep), sizeof(*asd)); > > > > - ret = v4l2_async_notifier_add_subdev(&fmd->subdev_notifier, > > - &fmd->sensor[index].asd); > > - if (ret) { > > - of_node_put(rem); > > - return ret; > > - } > > + of_node_put(ep); > > + > > + if (IS_ERR(asd)) > > + return PTR_ERR(asd); > > > > + fmd->sensor[index].asd = asd; > > fmd->num_sensors++; > > > > return 0; > > @@ -1381,7 +1379,8 @@ static int subdev_notifier_bound(struct v4l2_async_notifier *notifier, > > > > /* Find platform data for this sensor subdev */ > > for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++) > > - if (fmd->sensor[i].asd.match.fwnode == > > + if (fmd->sensor[i].asd && > > Is this needed ? If the subdev has bound the async subdev has been > allocated correctly, doesn't it ? > The idea is to keep the code the same. You are probably right, and the above felt quite nasty, but then again, didn't want to go down the cleanup road. > With the ep ref counting clarified Sure. > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > Thanks a lot, Ezequiel
diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c index e636c33e847b..196166a9a4e5 100644 --- a/drivers/media/platform/exynos4-is/media-dev.c +++ b/drivers/media/platform/exynos4-is/media-dev.c @@ -401,6 +401,7 @@ static int fimc_md_parse_one_endpoint(struct fimc_md *fmd, int index = fmd->num_sensors; struct fimc_source_info *pd = &fmd->sensor[index].pdata; struct device_node *rem, *np; + struct v4l2_async_subdev *asd; struct v4l2_fwnode_endpoint endpoint = { .bus_type = 0 }; int ret; @@ -418,7 +419,6 @@ static int fimc_md_parse_one_endpoint(struct fimc_md *fmd, pd->mux_id = (endpoint.base.port - 1) & 0x1; rem = of_graph_get_remote_port_parent(ep); - of_node_put(ep); if (rem == NULL) { v4l2_info(&fmd->v4l2_dev, "Remote device at %pOF not found\n", ep); @@ -450,6 +450,7 @@ static int fimc_md_parse_one_endpoint(struct fimc_md *fmd, * checking parent's node name. */ np = of_get_parent(rem); + of_node_put(rem); if (of_node_name_eq(np, "i2c-isp")) pd->fimc_bus_type = FIMC_BUS_TYPE_ISP_WRITEBACK; @@ -457,21 +458,18 @@ static int fimc_md_parse_one_endpoint(struct fimc_md *fmd, pd->fimc_bus_type = pd->sensor_bus_type; of_node_put(np); - if (WARN_ON(index >= ARRAY_SIZE(fmd->sensor))) { - of_node_put(rem); + if (WARN_ON(index >= ARRAY_SIZE(fmd->sensor))) return -EINVAL; - } - fmd->sensor[index].asd.match_type = V4L2_ASYNC_MATCH_FWNODE; - fmd->sensor[index].asd.match.fwnode = of_fwnode_handle(rem); + asd = v4l2_async_notifier_add_fwnode_remote_subdev( + &fmd->subdev_notifier, of_fwnode_handle(ep), sizeof(*asd)); - ret = v4l2_async_notifier_add_subdev(&fmd->subdev_notifier, - &fmd->sensor[index].asd); - if (ret) { - of_node_put(rem); - return ret; - } + of_node_put(ep); + + if (IS_ERR(asd)) + return PTR_ERR(asd); + fmd->sensor[index].asd = asd; fmd->num_sensors++; return 0; @@ -1381,7 +1379,8 @@ static int subdev_notifier_bound(struct v4l2_async_notifier *notifier, /* Find platform data for this sensor subdev */ for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++) - if (fmd->sensor[i].asd.match.fwnode == + if (fmd->sensor[i].asd && + fmd->sensor[i].asd->match.fwnode == of_fwnode_handle(subdev->dev->of_node)) si = &fmd->sensor[i]; diff --git a/drivers/media/platform/exynos4-is/media-dev.h b/drivers/media/platform/exynos4-is/media-dev.h index 9447fafe23c6..a3876d668ea6 100644 --- a/drivers/media/platform/exynos4-is/media-dev.h +++ b/drivers/media/platform/exynos4-is/media-dev.h @@ -83,7 +83,7 @@ struct fimc_camclk_info { */ struct fimc_sensor_info { struct fimc_source_info pdata; - struct v4l2_async_subdev asd; + struct v4l2_async_subdev *asd; struct v4l2_subdev *subdev; struct fimc_dev *host; };
The use of v4l2_async_notifier_add_subdev is discouraged. Drivers are instead encouraged to use a helper such as v4l2_async_notifier_add_fwnode_remote_subdev. This fixes a misuse of the API, as v4l2_async_notifier_add_subdev should get a kmalloc'ed struct v4l2_async_subdev, removing some boilerplate code while at it. Use the appropriate helper v4l2_async_notifier_add_fwnode_remote_subdev, which handles the needed setup, instead of open-coding it. Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> --- drivers/media/platform/exynos4-is/media-dev.c | 25 +++++++++---------- drivers/media/platform/exynos4-is/media-dev.h | 2 +- 2 files changed, 13 insertions(+), 14 deletions(-)