[04/13] media: exynos4-is: Use v4l2_async_notifier_add_fwnode_remote_subdev
Commit Message
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(-)
Comments
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
@@ -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];
@@ -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;
};