Re: [PATCH 03/13] media: renesas-ceu: Use v4l2_async_notifier_add_{i2c,fwnode_remote}_subdev helpers
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Date: 2021-01-14 02:08:01
Hi Ezequiel, Thank you for the patch. On Tue, Jan 12, 2021 at 10:23:29AM -0300, Ezequiel Garcia wrote:
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/
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
- 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.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
for (i = 0; i < num_ep; i++) { struct v4l2_fwnode_endpoint fw_ep = {@@ -1578,20 +1555,16 @@ static int ceu_parse_dt(struct ceu_device *ceudev) } /* Setup the ceu subdevice and the async subdevice. */ - ceu_sd = &ceudev->subdevs[i]; - INIT_LIST_HEAD(&ceu_sd->asd.list); - - remote = of_graph_get_remote_port_parent(ep); - ceu_sd->mbus_flags = fw_ep.bus.parallel.flags; - ceu_sd->asd.match_type = V4L2_ASYNC_MATCH_FWNODE; - ceu_sd->asd.match.fwnode = of_fwnode_handle(remote); - - ret = v4l2_async_notifier_add_subdev(&ceudev->notifier, - &ceu_sd->asd); - if (ret) { - of_node_put(remote); + asd = v4l2_async_notifier_add_fwnode_remote_subdev( + &ceudev->notifier, of_fwnode_handle(ep), + sizeof(*ceu_sd)); + if (IS_ERR(asd)) { + ret = PTR_ERR(asd); goto error_cleanup; } + ceu_sd = to_ceu_subdev(asd); + ceu_sd->mbus_flags = fw_ep.bus.parallel.flags; + ceudev->subdevs[i] = ceu_sd; of_node_put(ep); }
-- Regards, Laurent Pinchart