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:

quoted

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 <redacted>
---
 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.

quoted

        if (rem == NULL) {

quoted

                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

quoted

        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.

quoted

+       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