| Message ID | 20210112132339.5621-7-ezequiel@collabora.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | V4L2 Async notifier API cleanup | expand |
Hi Ezequiel, On Tue, Jan 12, 2021 at 10:23:32AM -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. > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > drivers/media/platform/atmel/atmel-isc.h | 1 + > drivers/media/platform/atmel/atmel-isi.c | 46 ++++++------------- > .../media/platform/atmel/atmel-sama5d2-isc.c | 44 ++++++------------ > 3 files changed, 29 insertions(+), 62 deletions(-) > > } > [snip] > list_for_each_entry(subdev_entity, &isc->subdev_entities, list) { > + struct v4l2_async_subdev *asd; > + > v4l2_async_notifier_init(&subdev_entity->notifier); > > - ret = v4l2_async_notifier_add_subdev(&subdev_entity->notifier, > - subdev_entity->asd); > - if (ret) { > - fwnode_handle_put(subdev_entity->asd->match.fwnode); > - kfree(subdev_entity->asd); > + asd = v4l2_async_notifier_add_fwnode_remote_subdev( > + &subdev_entity->notifier, > + of_fwnode_handle(subdev_entity->epn), > + sizeof(*asd)); > + > + of_node_put(subdev_entity->epn); > + subdev_entity->epn = NULL; > + > + if (IS_ERR(asd)) { > + ret = PTR_ERR(asd); > goto cleanup_subdev; The isc_subdev_cleanup() this label jumps to does not put the other subdev_entity->epn The issue was there already if I'm not mistaken. If I'm not, I think it's worth recording it with a FIXME: comment while you're here Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org> Thanks j > } > > -- > 2.29.2 >
On Sat, 2021-01-16 at 18:21 +0100, Jacopo Mondi wrote: > Hi Ezequiel, > > On Tue, Jan 12, 2021 at 10:23:32AM -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. > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > --- > > drivers/media/platform/atmel/atmel-isc.h | 1 + > > drivers/media/platform/atmel/atmel-isi.c | 46 ++++++------------- > > .../media/platform/atmel/atmel-sama5d2-isc.c | 44 ++++++------------ > > 3 files changed, 29 insertions(+), 62 deletions(-) > > > > } > > > > [snip] > > > list_for_each_entry(subdev_entity, &isc->subdev_entities, list) { > > + struct v4l2_async_subdev *asd; > > + > > v4l2_async_notifier_init(&subdev_entity->notifier); > > > > - ret = v4l2_async_notifier_add_subdev(&subdev_entity->notifier, > > - subdev_entity->asd); > > - if (ret) { > > - fwnode_handle_put(subdev_entity->asd->match.fwnode); > > - kfree(subdev_entity->asd); > > + asd = v4l2_async_notifier_add_fwnode_remote_subdev( > > + &subdev_entity->notifier, > > + of_fwnode_handle(subdev_entity->epn), > > + sizeof(*asd)); > > + > > + of_node_put(subdev_entity->epn); > > + subdev_entity->epn = NULL; > > + > > + if (IS_ERR(asd)) { > > + ret = PTR_ERR(asd); > > goto cleanup_subdev; > > The isc_subdev_cleanup() this label jumps to does not put the other > subdev_entity->epn > > The issue was there already if I'm not mistaken. If I'm not, I think > it's worth recording it with a FIXME: comment while you're here > Although, as you've noticed I tend to break this rule myself, I'd rather avoid adding more changes to the patch. The OF/fwnode handling in this atmel driver might benefit from some improvements (and the same can be said of some other drivers) but I'd say let's stick to just improving the v4l2-async API. > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > Thanks a lot for the reviews! Ezequiel
diff --git a/drivers/media/platform/atmel/atmel-isc.h b/drivers/media/platform/atmel/atmel-isc.h index 24b784b893d6..fab8eca58d93 100644 --- a/drivers/media/platform/atmel/atmel-isc.h +++ b/drivers/media/platform/atmel/atmel-isc.h @@ -41,6 +41,7 @@ struct isc_buffer { struct isc_subdev_entity { struct v4l2_subdev *sd; struct v4l2_async_subdev *asd; + struct device_node *epn; struct v4l2_async_notifier notifier; u32 pfe_cfg0; diff --git a/drivers/media/platform/atmel/atmel-isi.c b/drivers/media/platform/atmel/atmel-isi.c index d74aa73f26be..c1a6dd7af002 100644 --- a/drivers/media/platform/atmel/atmel-isi.c +++ b/drivers/media/platform/atmel/atmel-isi.c @@ -70,7 +70,6 @@ struct frame_buffer { struct isi_graph_entity { struct device_node *node; - struct v4l2_async_subdev asd; struct v4l2_subdev *subdev; }; @@ -1136,45 +1135,26 @@ static const struct v4l2_async_notifier_operations isi_graph_notify_ops = { .complete = isi_graph_notify_complete, }; -static int isi_graph_parse(struct atmel_isi *isi, struct device_node *node) -{ - struct device_node *ep = NULL; - struct device_node *remote; - - ep = of_graph_get_next_endpoint(node, ep); - if (!ep) - return -EINVAL; - - remote = of_graph_get_remote_port_parent(ep); - of_node_put(ep); - if (!remote) - return -EINVAL; - - /* Remote node to connect */ - isi->entity.node = remote; - isi->entity.asd.match_type = V4L2_ASYNC_MATCH_FWNODE; - isi->entity.asd.match.fwnode = of_fwnode_handle(remote); - return 0; -} - static int isi_graph_init(struct atmel_isi *isi) { + struct v4l2_async_subdev *asd; + struct device_node *ep; int ret; - /* Parse the graph to extract a list of subdevice DT nodes. */ - ret = isi_graph_parse(isi, isi->dev->of_node); - if (ret < 0) { - dev_err(isi->dev, "Graph parsing failed\n"); - return ret; - } + ep = of_graph_get_next_endpoint(isi->dev->of_node, NULL); + if (!ep) + return -EINVAL; v4l2_async_notifier_init(&isi->notifier); - ret = v4l2_async_notifier_add_subdev(&isi->notifier, &isi->entity.asd); - if (ret) { - of_node_put(isi->entity.node); - return ret; - } + asd = v4l2_async_notifier_add_fwnode_remote_subdev( + &isi->notifier, + of_fwnode_handle(ep), + sizeof(*asd)); + of_node_put(ep); + + if (IS_ERR(asd)) + return PTR_ERR(asd); isi->notifier.ops = &isi_graph_notify_ops; diff --git a/drivers/media/platform/atmel/atmel-sama5d2-isc.c b/drivers/media/platform/atmel/atmel-sama5d2-isc.c index a3304f49e499..9ee2cd194f93 100644 --- a/drivers/media/platform/atmel/atmel-sama5d2-isc.c +++ b/drivers/media/platform/atmel/atmel-sama5d2-isc.c @@ -57,7 +57,7 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc) { struct device_node *np = dev->of_node; - struct device_node *epn = NULL, *rem; + struct device_node *epn = NULL; struct isc_subdev_entity *subdev_entity; unsigned int flags; int ret; @@ -71,17 +71,9 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc) if (!epn) return 0; - rem = of_graph_get_remote_port_parent(epn); - if (!rem) { - dev_notice(dev, "Remote device at %pOF not found\n", - epn); - continue; - } - ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(epn), &v4l2_epn); if (ret) { - of_node_put(rem); ret = -EINVAL; dev_err(dev, "Could not parse the endpoint\n"); break; @@ -90,21 +82,10 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc) subdev_entity = devm_kzalloc(dev, sizeof(*subdev_entity), GFP_KERNEL); if (!subdev_entity) { - of_node_put(rem); - ret = -ENOMEM; - break; - } - - /* asd will be freed by the subsystem once it's added to the - * notifier list - */ - subdev_entity->asd = kzalloc(sizeof(*subdev_entity->asd), - GFP_KERNEL); - if (!subdev_entity->asd) { - of_node_put(rem); ret = -ENOMEM; break; } + subdev_entity->epn = epn; flags = v4l2_epn.bus.parallel.flags; @@ -121,12 +102,10 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc) subdev_entity->pfe_cfg0 |= ISC_PFE_CFG0_CCIR_CRC | ISC_PFE_CFG0_CCIR656; - subdev_entity->asd->match_type = V4L2_ASYNC_MATCH_FWNODE; - subdev_entity->asd->match.fwnode = of_fwnode_handle(rem); list_add_tail(&subdev_entity->list, &isc->subdev_entities); } - of_node_put(epn); + return ret; } @@ -228,13 +207,20 @@ static int atmel_isc_probe(struct platform_device *pdev) } list_for_each_entry(subdev_entity, &isc->subdev_entities, list) { + struct v4l2_async_subdev *asd; + v4l2_async_notifier_init(&subdev_entity->notifier); - ret = v4l2_async_notifier_add_subdev(&subdev_entity->notifier, - subdev_entity->asd); - if (ret) { - fwnode_handle_put(subdev_entity->asd->match.fwnode); - kfree(subdev_entity->asd); + asd = v4l2_async_notifier_add_fwnode_remote_subdev( + &subdev_entity->notifier, + of_fwnode_handle(subdev_entity->epn), + sizeof(*asd)); + + of_node_put(subdev_entity->epn); + subdev_entity->epn = NULL; + + if (IS_ERR(asd)) { + ret = PTR_ERR(asd); goto cleanup_subdev; }
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. Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> --- drivers/media/platform/atmel/atmel-isc.h | 1 + drivers/media/platform/atmel/atmel-isi.c | 46 ++++++------------- .../media/platform/atmel/atmel-sama5d2-isc.c | 44 ++++++------------ 3 files changed, 29 insertions(+), 62 deletions(-)