| Message ID | 20210112132339.5621-4-ezequiel@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series | V4L2 Async notifier API cleanup | expand |
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/ > 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 <ezequiel@collabora.com> > --- > 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. > - 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. > > 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. > > 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
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: > > 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. > > 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 <ezequiel@collabora.com> > > --- > > 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. > > - 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. > > > > 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
Hi Laurent, 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: > > 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/ > > > 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 <ezequiel@collabora.com> > > --- > > 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. > After some thought, I'd say it would be better to leave ceu_init_async_subdevs() untouched, trying to be as uninvasive as possible. Thanks for the catch. Ezequiel
Hi Ezequiel, On Tue, Jan 12, 2021 at 10:23:29AM -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_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 <ezequiel@collabora.com> > --- > 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); > - if (!ceudev->subdevs) > - return -ENOMEM; > - > - ceudev->sd = NULL; > - ceudev->sd_index = 0; > - ceudev->num_sd = 0; > - As Laurent the array of subdevs still needs to be allocated, and the zeroing of the ceudev fields could be removed. If you don't want to mix too many things I can do it on top as.. > - return 0; > -} > - [snip] > } > > /* 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); This could also probably be moved before the endpoint parsing to avoid that if the async subdev registration fails. All on top though and not major. With the subdev allocation fixed Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org> Thanks j > 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); > } > -- > 2.29.2 >
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); - 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; 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; 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); }
The use of v4l2_async_notifier_add_subdev is discouraged. 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 <ezequiel@collabora.com> --- drivers/media/platform/renesas-ceu.c | 89 ++++++++++------------------ 1 file changed, 31 insertions(+), 58 deletions(-)