Hi Laurent,

Thanks a lot for the thorough review.

On Thu, 2021-01-14 at 04:06 +0200, Laurent Pinchart wrote:

Hi Ezequiel,

Thank you for the patch.

On Tue, Jan 12, 2021 at 10:23:29AM -0300, Ezequiel Garcia wrote:

quoted

The use of v4l2_async_notifier_add_subdev is discouraged.

It's not yet, that only happens at the end of the series :-)

s/is discouraged/will be discouraged/

Makes sense. Will change this on all the commits.

quoted

Drivers are instead encouraged to use a helper such as
v4l2_async_notifier_add_i2c_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_i2c_subdev
or v4l2_async_notifier_add_fwnode_remote_subdev, which handles
the needed setup, instead of open-coding it.

Using v4l2-async to allocate the driver-specific structs,
requires to change struct ceu_subdev so the embedded
struct v4l2_async_subdev is now the first element.

Signed-off-by: Ezequiel Garcia <redacted>
---
 drivers/media/platform/renesas-ceu.c | 89 ++++++++++------------------
 1 file changed, 31 insertions(+), 58 deletions(-)

diff --git a/drivers/media/platform/renesas-ceu.c b/drivers/media/platform/renesas-ceu.c
index 4a633ad0e8fa..18485812a21e 100644
--- a/drivers/media/platform/renesas-ceu.c
+++ b/drivers/media/platform/renesas-ceu.c

@@ -152,8 +152,8 @@ static inline struct ceu_buffer *vb2_to_ceu(struct vb2_v4l2_buffer *vbuf)

  * ceu_subdev - Wraps v4l2 sub-device and provides async subdevice.
  */
 struct ceu_subdev {
-       struct v4l2_subdev *v4l2_sd;
        struct v4l2_async_subdev asd;
+       struct v4l2_subdev *v4l2_sd;
 
        /* per-subdevice mbus configuration options */
        unsigned int mbus_flags;

@@ -174,7 +174,7 @@ struct ceu_device {

        struct v4l2_device      v4l2_dev;
 
        /* subdevices descriptors */
-       struct ceu_subdev       *subdevs;
+       struct ceu_subdev       **subdevs;
        /* the subdevice currently in use */
        struct ceu_subdev       *sd;
        unsigned int            sd_index;

@@ -1195,7 +1195,7 @@ static int ceu_enum_input(struct file *file, void *priv,

        if (inp->index >= ceudev->num_sd)
                return -EINVAL;
 
-       ceusd = &ceudev->subdevs[inp->index];
+       ceusd = ceudev->subdevs[inp->index];
 
        inp->type = V4L2_INPUT_TYPE_CAMERA;
        inp->std = 0;

@@ -1230,7 +1230,7 @@ static int ceu_s_input(struct file *file, void *priv, unsigned int i)

                return 0;
 
        ceu_sd_old = ceudev->sd;
-       ceudev->sd = &ceudev->subdevs[i];
+       ceudev->sd = ceudev->subdevs[i];
 
        /*
         * Make sure we can generate output image formats and apply

@@ -1423,7 +1423,7 @@ static int ceu_notify_complete(struct v4l2_async_notifier *notifier)

         * ceu formats.
         */
        if (!ceudev->sd) {
-               ceudev->sd = &ceudev->subdevs[0];
+               ceudev->sd = ceudev->subdevs[0];
                ceudev->sd_index = 0;
        }
 

@@ -1465,28 +1465,6 @@ static const struct v4l2_async_notifier_operations ceu_notify_ops = {

        .complete       = ceu_notify_complete,
 };
 
-/*
- * ceu_init_async_subdevs() - Initialize CEU subdevices and async_subdevs in
- *                           ceu device. Both DT and platform data parsing use
- *                           this routine.
- *
- * Returns 0 for success, -ENOMEM for failure.
- */
-static int ceu_init_async_subdevs(struct ceu_device *ceudev, unsigned int n_sd)
-{
-       /* Reserve memory for 'n_sd' ceu_subdev descriptors. */
-       ceudev->subdevs = devm_kcalloc(ceudev->dev, n_sd,
-                                      sizeof(*ceudev->subdevs), GFP_KERNEL);

I may be missing something, but it looks like the subdevs array isn't
allocated anymore. It turned to an array of pointers, but it still need
allocation.

Oops. Thanks for spotting this.

quoted

-       if (!ceudev->subdevs)
-               return -ENOMEM;
-
-       ceudev->sd = NULL;
-       ceudev->sd_index = 0;
-       ceudev->num_sd = 0;
-
-       return 0;
-}
-
 /*
  * ceu_parse_platform_data() - Initialize async_subdevices using platform
  *                            device provided data.

@@ -1495,6 +1473,7 @@ static int ceu_parse_platform_data(struct ceu_device *ceudev,

                                   const struct ceu_platform_data *pdata)
 {
        const struct ceu_async_subdev *async_sd;
+       struct v4l2_async_subdev *asd;
        struct ceu_subdev *ceu_sd;
        unsigned int i;
        int ret;

@@ -1502,29 +1481,26 @@ static int ceu_parse_platform_data(struct ceu_device *ceudev,

        if (pdata->num_subdevs == 0)
                return -ENODEV;
 
-       ret = ceu_init_async_subdevs(ceudev, pdata->num_subdevs);
-       if (ret)
-               return ret;
+       ceudev->sd = NULL;
+       ceudev->sd_index = 0;
+       ceudev->num_sd = 0;

ceudev is kzalloc()ed, so I think you could skip this.

Right.

quoted

 
        for (i = 0; i < pdata->num_subdevs; i++) {
 
                /* Setup the ceu subdevice and the async subdevice. */
                async_sd = &pdata->subdevs[i];
-               ceu_sd = &ceudev->subdevs[i];
-
-               INIT_LIST_HEAD(&ceu_sd->asd.list);
-
-               ceu_sd->mbus_flags      = async_sd->flags;
-               ceu_sd->asd.match_type  = V4L2_ASYNC_MATCH_I2C;
-               ceu_sd->asd.match.i2c.adapter_id = async_sd->i2c_adapter_id;
-               ceu_sd->asd.match.i2c.address = async_sd->i2c_address;
-
-               ret = v4l2_async_notifier_add_subdev(&ceudev->notifier,
-                                                    &ceu_sd->asd);
-               if (ret) {
+               asd = v4l2_async_notifier_add_i2c_subdev(&ceudev->notifier,
+                               async_sd->i2c_adapter_id,
+                               async_sd->i2c_address,
+                               sizeof(*ceu_sd));
+               if (IS_ERR(asd)) {
+                       ret = PTR_ERR(asd);
                        v4l2_async_notifier_cleanup(&ceudev->notifier);
                        return ret;
                }
+               ceu_sd = to_ceu_subdev(asd);
+               ceu_sd->mbus_flags = async_sd->flags;
+               ceudev->subdevs[i] = ceu_sd;
        }
 
        return pdata->num_subdevs;

@@ -1536,7 +1512,8 @@ static int ceu_parse_platform_data(struct ceu_device *ceudev,

 static int ceu_parse_dt(struct ceu_device *ceudev)
 {
        struct device_node *of = ceudev->dev->of_node;
-       struct device_node *ep, *remote;
+       struct device_node *ep;
+       struct v4l2_async_subdev *asd;
        struct ceu_subdev *ceu_sd;
        unsigned int i;
        int num_ep;

@@ -1546,9 +1523,9 @@ static int ceu_parse_dt(struct ceu_device *ceudev)

        if (!num_ep)
                return -ENODEV;
 
-       ret = ceu_init_async_subdevs(ceudev, num_ep);
-       if (ret)
-               return ret;
+       ceudev->sd = NULL;
+       ceudev->sd_index = 0;
+       ceudev->num_sd = 0;

Same here, I think this could be skipped.

Yup.

Thanks,
Ezequiel