| Message ID | 20230330115853.1628216-1-sakari.ailus@linux.intel.com |
|---|---|
| Headers | show |
| Series | Separate links and async sub-devices | expand |
Hi Sakari, On Thu, Mar 30, 2023 at 02:58:36PM +0300, Sakari Ailus wrote: > When an async notifier is unregistered, the async sub-devices in the > notifier's done list will disappear with the notifier. However this is > currently also done to the sub-notifiers that remain registered. Their > sub-devices only need to be unbound while the async sub-devices themselves > need to be returned to the sub-notifier's waiting list. Do this now. > > Fixes: 2cab00bb076b ("media: v4l: async: Allow binding notifiers to sub-devices") > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Thanks for clarifying my question on the RFC version Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > --- > drivers/media/v4l2-core/v4l2-async.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index 2f1b718a9189..008a2a3e312e 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -414,7 +414,8 @@ static void v4l2_async_cleanup(struct v4l2_subdev *sd) > > /* Unbind all sub-devices in the notifier tree. */ > static void > -v4l2_async_nf_unbind_all_subdevs(struct v4l2_async_notifier *notifier) > +v4l2_async_nf_unbind_all_subdevs(struct v4l2_async_notifier *notifier, > + bool readd) > { > struct v4l2_subdev *sd, *tmp; > > @@ -423,9 +424,11 @@ v4l2_async_nf_unbind_all_subdevs(struct v4l2_async_notifier *notifier) > v4l2_async_find_subdev_notifier(sd); > > if (subdev_notifier) > - v4l2_async_nf_unbind_all_subdevs(subdev_notifier); > + v4l2_async_nf_unbind_all_subdevs(subdev_notifier, true); > > v4l2_async_nf_call_unbind(notifier, sd, sd->asd); > + if (readd) > + list_add_tail(&sd->asd->list, ¬ifier->waiting); > v4l2_async_cleanup(sd); > > list_move(&sd->async_list, &subdev_list); > @@ -557,7 +560,7 @@ static int __v4l2_async_nf_register(struct v4l2_async_notifier *notifier) > /* > * On failure, unbind all sub-devices registered through this notifier. > */ > - v4l2_async_nf_unbind_all_subdevs(notifier); > + v4l2_async_nf_unbind_all_subdevs(notifier, false); > > err_unlock: > mutex_unlock(&list_lock); > @@ -607,7 +610,7 @@ __v4l2_async_nf_unregister(struct v4l2_async_notifier *notifier) > if (!notifier || (!notifier->v4l2_dev && !notifier->sd)) > return; > > - v4l2_async_nf_unbind_all_subdevs(notifier); > + v4l2_async_nf_unbind_all_subdevs(notifier, false); > > notifier->sd = NULL; > notifier->v4l2_dev = NULL; > @@ -805,7 +808,7 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd) > */ > subdev_notifier = v4l2_async_find_subdev_notifier(sd); > if (subdev_notifier) > - v4l2_async_nf_unbind_all_subdevs(subdev_notifier); > + v4l2_async_nf_unbind_all_subdevs(subdev_notifier, false); > > if (sd->asd) > v4l2_async_nf_call_unbind(notifier, sd, sd->asd); > -- > 2.30.2 >
Hi Sakari On Thu, Mar 30, 2023 at 02:58:38PM +0300, Sakari Ailus wrote: > V4L2 async sub-device matching originally used the device nodes only. > Endpoint nodes were taken into use instead as using the device nodes was > problematic for it was in some cases ambiguous which link might have been > in question. > > There is however no need to use endpoint nodes on both sides, as the async > sub-device's fwnode can always be trivially obtained using > fwnode_graph_get_remote_endpoint() when needed while what counts is > whether or not the link is between two device nodes, i.e. the device nodes > match. > As you know I'm a bit debated. Strict endpoint-matching requires one subdev to be registed per each endpoint, and this is tedious for drivers that have to register a subdev for each of its endpoints Allowing a subdev to be matched multiple times on different endpoints gives a way for lazy drivers to take a shortcut and simplify their topologies to a single subdev, when they would actually need more. Also, knowing where this series is going, I wonder if this wouldn't be a good time to enforce all async subdevices to be registered with an endpoint. From a very quick look at the drivers we have in mainline only a few still use v4l2_async_nf_add_fwnode() and only 4 of them (camss, rcar_drif, am437x-vpfe, vpif_capture, xilinx_vipp) use as match.fwnode what the remote port parent. I wonder if this would be a good occasione to enforce with a WARN() if !fwnode_graph_is_endpoint(fwnode) in v4l2_async_nf_add_fwnode() (or possibily, even remove that function and port all drivers to use vl2_async_nf_add_fwnode_remote(). I can help, if the above makes sense to you as well. > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/i2c/adv748x/adv748x-csi2.c | 3 - > drivers/media/i2c/max9286.c | 14 +--- > drivers/media/i2c/rdacm20.c | 15 +---- > drivers/media/i2c/rdacm21.c | 15 +---- > drivers/media/i2c/tc358746.c | 3 - > drivers/media/v4l2-core/v4l2-async.c | 86 ++++++------------------ > 6 files changed, 24 insertions(+), 112 deletions(-) > > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c > index bd4f3fe0e309..3d830816243f 100644 > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c > @@ -300,9 +300,6 @@ int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx) > MEDIA_ENT_F_VID_IF_BRIDGE, > is_txa(tx) ? "txa" : "txb"); > > - /* Ensure that matching is based upon the endpoint fwnodes */ > - tx->sd.fwnode = of_fwnode_handle(state->endpoints[tx->port]); > - > /* Register internal ops for incremental subdev registration */ > tx->sd.internal_ops = &adv748x_csi2_internal_ops; > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > index 701038d6d19b..2d0f43e3fb9f 100644 > --- a/drivers/media/i2c/max9286.c > +++ b/drivers/media/i2c/max9286.c > @@ -1051,7 +1051,6 @@ static const struct v4l2_ctrl_ops max9286_ctrl_ops = { > static int max9286_v4l2_register(struct max9286_priv *priv) > { > struct device *dev = &priv->client->dev; > - struct fwnode_handle *ep; > int ret; > int i; > > @@ -1093,25 +1092,14 @@ static int max9286_v4l2_register(struct max9286_priv *priv) > if (ret) > goto err_async; > > - ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), MAX9286_SRC_PAD, > - 0, 0); > - if (!ep) { > - dev_err(dev, "Unable to retrieve endpoint on \"port@4\"\n"); > - ret = -ENOENT; > - goto err_async; > - } > - priv->sd.fwnode = ep; > - You should also remove the fwnode_handle_put() call from static void max9286_v4l2_unregister(struct max9286_priv *priv) { fwnode_handle_put(priv->sd.fwnode); v4l2_async_unregister_subdev(&priv->sd); max9286_v4l2_notifier_unregister(priv); } > ret = v4l2_async_register_subdev(&priv->sd); > if (ret < 0) { > dev_err(dev, "Unable to register subdevice\n"); > - goto err_put_node; > + goto err_async; > } > > return 0; > > -err_put_node: > - fwnode_handle_put(ep); > err_async: > v4l2_ctrl_handler_free(&priv->ctrls); > max9286_v4l2_notifier_unregister(priv); > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c > index a2263fa825b5..ea1111152285 100644 > --- a/drivers/media/i2c/rdacm20.c > +++ b/drivers/media/i2c/rdacm20.c > @@ -567,7 +567,6 @@ static int rdacm20_initialize(struct rdacm20_device *dev) > static int rdacm20_probe(struct i2c_client *client) > { > struct rdacm20_device *dev; > - struct fwnode_handle *ep; > int ret; > > dev = devm_kzalloc(&client->dev, sizeof(*dev), GFP_KERNEL); > @@ -616,24 +615,12 @@ static int rdacm20_probe(struct i2c_client *client) > if (ret < 0) > goto error_free_ctrls; > > - ep = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL); > - if (!ep) { > - dev_err(&client->dev, > - "Unable to get endpoint in node %pOF\n", > - client->dev.of_node); > - ret = -ENOENT; > - goto error_free_ctrls; > - } > - dev->sd.fwnode = ep; Same for this driver and for rdacm21 > - > ret = v4l2_async_register_subdev(&dev->sd); > if (ret) > - goto error_put_node; > + goto error_free_ctrls; > > return 0; > > -error_put_node: > - fwnode_handle_put(ep); > error_free_ctrls: > v4l2_ctrl_handler_free(&dev->ctrls); > error: > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c > index 9ccc56c30d3b..d67cfcb2e05a 100644 > --- a/drivers/media/i2c/rdacm21.c > +++ b/drivers/media/i2c/rdacm21.c > @@ -543,7 +543,6 @@ static int rdacm21_initialize(struct rdacm21_device *dev) > static int rdacm21_probe(struct i2c_client *client) > { > struct rdacm21_device *dev; > - struct fwnode_handle *ep; > int ret; > > dev = devm_kzalloc(&client->dev, sizeof(*dev), GFP_KERNEL); > @@ -588,24 +587,12 @@ static int rdacm21_probe(struct i2c_client *client) > if (ret < 0) > goto error_free_ctrls; > > - ep = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL); > - if (!ep) { > - dev_err(&client->dev, > - "Unable to get endpoint in node %pOF\n", > - client->dev.of_node); > - ret = -ENOENT; > - goto error_free_ctrls; > - } > - dev->sd.fwnode = ep; > - > ret = v4l2_async_register_subdev(&dev->sd); > if (ret) > - goto error_put_node; > + goto error_free_ctrls; > > return 0; > > -error_put_node: > - fwnode_handle_put(dev->sd.fwnode); > error_free_ctrls: > v4l2_ctrl_handler_free(&dev->ctrls); > error: > diff --git a/drivers/media/i2c/tc358746.c b/drivers/media/i2c/tc358746.c > index 4063754a6732..56f2b43d4edf 100644 > --- a/drivers/media/i2c/tc358746.c > +++ b/drivers/media/i2c/tc358746.c > @@ -1476,9 +1476,6 @@ static int tc358746_async_register(struct tc358746 *tc358746) > if (err) > goto err_cleanup; > > - tc358746->sd.fwnode = fwnode_graph_get_endpoint_by_id( > - dev_fwnode(tc358746->sd.dev), TC358746_SOURCE, 0, 0); > - > err = v4l2_async_register_subdev(&tc358746->sd); > if (err) > goto err_unregister; > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index 6dd426c2ca68..13fe0bdc70b6 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -86,84 +86,33 @@ match_fwnode_one(struct v4l2_async_notifier *notifier, > struct v4l2_subdev *sd, struct fwnode_handle *sd_fwnode, > struct v4l2_async_subdev *asd) > { > - struct fwnode_handle *other_fwnode; > - struct fwnode_handle *dev_fwnode; > - bool asd_fwnode_is_ep; > - bool sd_fwnode_is_ep; > - struct device *dev; > + struct fwnode_handle *asd_dev_fwnode; > + bool ret; > > dev_dbg(sd->dev, "async: fwnode match: need %pfw, trying %pfw\n", > sd_fwnode, asd->match.fwnode); > > - /* > - * Both the subdev and the async subdev can provide either an endpoint > - * fwnode or a device fwnode. Start with the simple case of direct > - * fwnode matching. > - */ > if (sd_fwnode == asd->match.fwnode) { > dev_dbg(sd->dev, "async: direct match found\n"); > return true; > } > > - /* > - * Otherwise, check if the sd fwnode and the asd fwnode refer to an > - * endpoint or a device. If they're of the same type, there's no match. > - * Technically speaking this checks if the nodes refer to a connected > - * endpoint, which is the simplest check that works for both OF and > - * ACPI. This won't make a difference, as drivers should not try to > - * match unconnected endpoints. > - */ > - sd_fwnode_is_ep = fwnode_graph_is_endpoint(sd_fwnode); > - asd_fwnode_is_ep = fwnode_graph_is_endpoint(asd->match.fwnode); > - > - if (sd_fwnode_is_ep == asd_fwnode_is_ep) { > - dev_dbg(sd->dev, "async: matching node types\n"); > + if (!fwnode_graph_is_endpoint(asd->match.fwnode)) { > + dev_dbg(sd->dev, > + "async: async subdev fwnode not endpoint, no match\n"); As per the previous patch I would just say "direct match failed"; > return false; > } > > - /* > - * The sd and asd fwnodes are of different types. Get the device fwnode > - * parent of the endpoint fwnode, and compare it with the other fwnode. > - */ > - if (sd_fwnode_is_ep) { > - dev_fwnode = fwnode_graph_get_port_parent(sd_fwnode); > - other_fwnode = asd->match.fwnode; > - } else { > - dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode); > - other_fwnode = sd_fwnode; > - } > - > - dev_dbg(sd->dev, "async: fwnode compat match, need %pfw, trying %pfw\n", > - dev_fwnode, other_fwnode); > + asd_dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode); > > - fwnode_handle_put(dev_fwnode); > + ret = sd_fwnode == asd_dev_fwnode; > > - if (dev_fwnode != other_fwnode) { > - dev_dbg(sd->dev, "async: compat match not found\n"); > - return false; > - } > + fwnode_handle_put(asd_dev_fwnode); > > - /* > - * We have a heterogeneous match. Retrieve the struct device of the side > - * that matched on a device fwnode to print its driver name. > - */ > - if (sd_fwnode_is_ep) > - dev = notifier->v4l2_dev ? notifier->v4l2_dev->dev > - : notifier->sd->dev; > - else > - dev = sd->dev; > - > - if (dev && dev->driver) { > - if (sd_fwnode_is_ep) > - dev_warn(dev, "Driver %s uses device fwnode, incorrect match may occur\n", > - dev->driver->name); > - dev_notice(dev, "Consider updating driver %s to match on endpoints\n", > - dev->driver->name); > - } > - > - dev_dbg(sd->dev, "async: compat match found\n"); > + dev_dbg(sd->dev, "async: device--endpoint match %sfound\n", double '--' However now that I think about it, a subdev will be matched agains a rather large number of async subdevs and most attempts will fail.. do we want a printout for each of them ? > + ret ? "" : "not "); > > - return true; > + return ret; > } > > static bool match_fwnode(struct v4l2_async_notifier *notifier, > @@ -804,12 +753,19 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd) > int ret; > > /* > - * No reference taken. The reference is held by the device > - * (struct v4l2_subdev.dev), and async sub-device does not > - * exist independently of the device at any point of time. > + * No reference taken. The reference is held by the device (struct > + * v4l2_subdev.dev), and async sub-device does not exist independently > + * of the device at any point of time. > + * > + * The async sub-device shall always be registered for its device node, > + * not the endpoint node. Issue a warning in that case. Once there is > + * certainty no driver no longer does this, remove the warning (and > + * compatibility code) below. > */ > if (!sd->fwnode && sd->dev) > sd->fwnode = dev_fwnode(sd->dev); > + else if (WARN_ON(fwnode_graph_is_endpoint(sd->fwnode))) > + sd->fwnode = fwnode_graph_get_port_parent(sd->fwnode); > > mutex_lock(&list_lock); > > -- > 2.30.2 >
Hi Sakari On Thu, Mar 30, 2023 at 02:58:40PM +0300, Sakari Ailus wrote: > There's a need to verify that a single async sub-device isn't being added > multiple times, this would be an error. This takes place at the time of > adding the async sub-device to the notifier's list as well as when the > notifier is added to the global notifier's list. > > Use the pointer to the sub-device for testing this instead of an index to > an array that is long gone. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/v4l2-core/v4l2-async.c | 18 ++++++++---------- > 1 file changed, 8 insertions(+), 10 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index bb78e3618ab5..fc9ae22e2b47 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -456,21 +456,19 @@ __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, > > /* > * Find out whether an async sub-device was set up already or > - * whether it exists in a given notifier before @this_index. > - * If @this_index < 0, search the notifier's entire @asd_list. > + * whether it exists in a given notifier. > */ > static bool > v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, > - struct v4l2_async_subdev *asd, int this_index) > + struct v4l2_async_subdev *asd, bool skip_self) is skip_self used ? > { > struct v4l2_async_subdev *asd_y; > - int j = 0; > > lockdep_assert_held(&list_lock); > > /* Check that an asd is not being added more than once. */ > list_for_each_entry(asd_y, ¬ifier->asd_list, asd_list) { > - if (this_index >= 0 && j++ >= this_index) > + if (asd == asd_y) > break; > if (asd_equal(asd, asd_y)) > return true; > @@ -486,7 +484,7 @@ v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, > > static int v4l2_async_nf_asd_valid(struct v4l2_async_notifier *notifier, > struct v4l2_async_subdev *asd, > - int this_index) > + bool skip_self) > { > struct device *dev = > notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL; > @@ -497,7 +495,7 @@ static int v4l2_async_nf_asd_valid(struct v4l2_async_notifier *notifier, > switch (asd->match.type) { > case V4L2_ASYNC_MATCH_I2C: > case V4L2_ASYNC_MATCH_FWNODE: > - if (v4l2_async_nf_has_async_subdev(notifier, asd, this_index)) { > + if (v4l2_async_nf_has_async_subdev(notifier, asd, skip_self)) { > dev_dbg(dev, "subdev descriptor already listed in this or other notifiers\n"); > return -EEXIST; > } > @@ -520,7 +518,7 @@ EXPORT_SYMBOL(v4l2_async_nf_init); > static int __v4l2_async_nf_register(struct v4l2_async_notifier *notifier) > { > struct v4l2_async_subdev *asd; > - int ret, i = 0; > + int ret; > > INIT_LIST_HEAD(¬ifier->waiting); > INIT_LIST_HEAD(¬ifier->done); > @@ -528,7 +526,7 @@ static int __v4l2_async_nf_register(struct v4l2_async_notifier *notifier) > mutex_lock(&list_lock); > > list_for_each_entry(asd, ¬ifier->asd_list, asd_list) { > - ret = v4l2_async_nf_asd_valid(notifier, asd, i++); > + ret = v4l2_async_nf_asd_valid(notifier, asd, true); > if (ret) > goto err_unlock; > > @@ -661,7 +659,7 @@ int __v4l2_async_nf_add_subdev(struct v4l2_async_notifier *notifier, > > mutex_lock(&list_lock); > > - ret = v4l2_async_nf_asd_valid(notifier, asd, -1); > + ret = v4l2_async_nf_asd_valid(notifier, asd, false); > if (ret) > goto unlock; > > -- > 2.30.2 >
Hi Sakari On Thu, Mar 30, 2023 at 02:58:42PM +0300, Sakari Ailus wrote: > The naming of list heads and list entries is confusing as they're named > similarly. Use _head for list head and _list for list entries. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 2 +- > .../platform/renesas/rcar-vin/rcar-core.c | 2 +- > .../platform/renesas/rzg2l-cru/rzg2l-core.c | 2 +- > drivers/media/platform/xilinx/xilinx-vipp.c | 10 +-- > drivers/media/v4l2-core/v4l2-async.c | 66 +++++++++---------- > .../staging/media/imx/imx-media-dev-common.c | 2 +- > drivers/staging/media/tegra-video/vi.c | 6 +- > include/media/v4l2-async.h | 21 +++--- > 8 files changed, 56 insertions(+), 55 deletions(-) > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c > index 3b76a9d0383a..8b37c2ec8643 100644 > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c > @@ -1419,7 +1419,7 @@ static int cio2_notifier_complete(struct v4l2_async_notifier *notifier) > unsigned int pad; > int ret; > > - list_for_each_entry(asd, &cio2->notifier.asd_list, asd_list) { > + list_for_each_entry(asd, &cio2->notifier.asd_head, asd_list) { > s_asd = to_sensor_asd(asd); > q = &cio2->queue[s_asd->csi2.port]; > > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c > index 5e53d6b7036c..1b530da1c341 100644 > --- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c > @@ -397,7 +397,7 @@ static int rvin_group_notifier_init(struct rvin_dev *vin, unsigned int port, > } > } > > - if (list_empty(&vin->group->notifier.asd_list)) > + if (list_empty(&vin->group->notifier.asd_head)) > return 0; > > vin->group->notifier.ops = &rvin_group_notify_ops; > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c > index 5939f5165a5e..bfef297f5ec5 100644 > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c > @@ -191,7 +191,7 @@ static int rzg2l_cru_mc_parse_of_graph(struct rzg2l_cru_dev *cru) > > cru->notifier.ops = &rzg2l_cru_async_ops; > > - if (list_empty(&cru->notifier.asd_list)) > + if (list_empty(&cru->notifier.asd_head)) > return 0; > > ret = v4l2_async_nf_register(&cru->v4l2_dev, &cru->notifier); > diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c b/drivers/media/platform/xilinx/xilinx-vipp.c > index 0a16c218a50a..80157b7a28ee 100644 > --- a/drivers/media/platform/xilinx/xilinx-vipp.c > +++ b/drivers/media/platform/xilinx/xilinx-vipp.c > @@ -56,7 +56,7 @@ xvip_graph_find_entity(struct xvip_composite_device *xdev, > struct xvip_graph_entity *entity; > struct v4l2_async_subdev *asd; > > - list_for_each_entry(asd, &xdev->notifier.asd_list, asd_list) { > + list_for_each_entry(asd, &xdev->notifier.asd_head, asd_list) { > entity = to_xvip_entity(asd); > if (entity->asd.match.fwnode == fwnode) > return entity; > @@ -291,7 +291,7 @@ static int xvip_graph_notify_complete(struct v4l2_async_notifier *notifier) > dev_dbg(xdev->dev, "notify complete, all subdevs registered\n"); > > /* Create links for every entity. */ > - list_for_each_entry(asd, &xdev->notifier.asd_list, asd_list) { > + list_for_each_entry(asd, &xdev->notifier.asd_head, asd_list) { > entity = to_xvip_entity(asd); > ret = xvip_graph_build_one(xdev, entity); > if (ret < 0) > @@ -322,7 +322,7 @@ static int xvip_graph_notify_bound(struct v4l2_async_notifier *notifier, > /* Locate the entity corresponding to the bound subdev and store the > * subdev pointer. > */ > - list_for_each_entry(asd, &xdev->notifier.asd_list, asd_list) { > + list_for_each_entry(asd, &xdev->notifier.asd_head, asd_list) { > entity = to_xvip_entity(asd); > > if (entity->asd.match.fwnode != subdev->fwnode) > @@ -415,7 +415,7 @@ static int xvip_graph_parse(struct xvip_composite_device *xdev) > if (ret < 0) > return 0; > > - list_for_each_entry(asd, &xdev->notifier.asd_list, asd_list) { > + list_for_each_entry(asd, &xdev->notifier.asd_head, asd_list) { > entity = to_xvip_entity(asd); > ret = xvip_graph_parse_one(xdev, entity->asd.match.fwnode); > if (ret < 0) { > @@ -523,7 +523,7 @@ static int xvip_graph_init(struct xvip_composite_device *xdev) > goto done; > } > > - if (list_empty(&xdev->notifier.asd_list)) { > + if (list_empty(&xdev->notifier.asd_head)) { > dev_err(xdev->dev, "no subdev found in graph\n"); > ret = -ENOENT; > goto done; > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index 224ebf50f2d0..fdc995dfc15c 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -133,8 +133,8 @@ static bool match_fwnode(struct v4l2_async_notifier *notifier, > return match_fwnode_one(notifier, sd, sd->fwnode->secondary, match); > } > > -static LIST_HEAD(subdev_list); > -static LIST_HEAD(notifier_list); > +static LIST_HEAD(subdev_head); > +static LIST_HEAD(notifier_head); > static DEFINE_MUTEX(list_lock); > > static struct v4l2_async_subdev * > @@ -145,7 +145,7 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier, > struct v4l2_subdev *sd, struct v4l2_async_match *match); > struct v4l2_async_subdev *asd; > > - list_for_each_entry(asd, ¬ifier->waiting, list) { > + list_for_each_entry(asd, ¬ifier->waiting_head, waiting_list) { > /* bus_type has been verified valid before */ > switch (asd->match.type) { > case V4L2_ASYNC_MATCH_I2C: > @@ -194,7 +194,7 @@ v4l2_async_find_subdev_notifier(struct v4l2_subdev *sd) > { > struct v4l2_async_notifier *n; > > - list_for_each_entry(n, ¬ifier_list, list) > + list_for_each_entry(n, ¬ifier_head, notifier_list) > if (n->sd == sd) > return n; > > @@ -219,12 +219,12 @@ v4l2_async_nf_can_complete(struct v4l2_async_notifier *notifier) > { > struct v4l2_subdev *sd; > > - if (!list_empty(¬ifier->waiting)) { > + if (!list_empty(¬ifier->waiting_head)) { > dev_dbg(notifier_dev(notifier), "async: waiting for subdevs\n"); > return false; > } > > - list_for_each_entry(sd, ¬ifier->done, async_list) { > + list_for_each_entry(sd, ¬ifier->done_head, async_list) { > struct v4l2_async_notifier *subdev_notifier = > v4l2_async_find_subdev_notifier(sd); > > @@ -249,7 +249,7 @@ v4l2_async_nf_try_complete(struct v4l2_async_notifier *notifier) > struct v4l2_async_notifier *__notifier = notifier; > > /* Quick check whether there are still more sub-devices here. */ > - if (!list_empty(¬ifier->waiting)) > + if (!list_empty(¬ifier->waiting_head)) > return 0; > > if (notifier->sd) > @@ -327,13 +327,12 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier, > return ret; > } > > - /* Remove from the waiting list */ > - list_del(&asd->list); > + list_del(&asd->waiting_list); > sd->asd = asd; > sd->notifier = notifier; > > /* Move from the global subdevice list to notifier's done */ > - list_move(&sd->async_list, ¬ifier->done); > + list_move(&sd->async_list, ¬ifier->done_head); > > /* > * See if the sub-device has a notifier. If not, return here. > @@ -369,7 +368,7 @@ v4l2_async_nf_try_all_subdevs(struct v4l2_async_notifier *notifier) > return 0; > > again: > - list_for_each_entry(sd, &subdev_list, async_list) { > + list_for_each_entry(sd, &subdev_head, async_list) { > struct v4l2_async_subdev *asd; > int ret; > > @@ -411,7 +410,7 @@ v4l2_async_nf_unbind_all_subdevs(struct v4l2_async_notifier *notifier, > { > struct v4l2_subdev *sd, *tmp; > > - list_for_each_entry_safe(sd, tmp, ¬ifier->done, async_list) { > + list_for_each_entry_safe(sd, tmp, ¬ifier->done_head, async_list) { > struct v4l2_async_notifier *subdev_notifier = > v4l2_async_find_subdev_notifier(sd); > > @@ -420,10 +419,11 @@ v4l2_async_nf_unbind_all_subdevs(struct v4l2_async_notifier *notifier, > > v4l2_async_nf_call_unbind(notifier, sd, sd->asd); > if (readd) > - list_add_tail(&sd->asd->list, ¬ifier->waiting); > + list_add_tail(&sd->asd->waiting_list, > + ¬ifier->waiting_head); > v4l2_async_cleanup(sd); > > - list_move(&sd->async_list, &subdev_list); > + list_move(&sd->async_list, &subdev_head); > } > > notifier->parent = NULL; > @@ -437,11 +437,11 @@ __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, > struct v4l2_async_subdev *asd; > struct v4l2_subdev *sd; > > - list_for_each_entry(asd, ¬ifier->waiting, list) > + list_for_each_entry(asd, ¬ifier->waiting_head, waiting_list) > if (asd_equal(&asd->match, match)) > return true; > > - list_for_each_entry(sd, ¬ifier->done, async_list) { > + list_for_each_entry(sd, ¬ifier->done_head, async_list) { > if (WARN_ON(!sd->asd)) > continue; > > @@ -465,7 +465,7 @@ v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, > lockdep_assert_held(&list_lock); > > /* Check that an asd is not being added more than once. */ > - list_for_each_entry(asd, ¬ifier->asd_list, asd_list) { > + list_for_each_entry(asd, ¬ifier->asd_head, asd_list) { > if (&asd->match == match) > break; > if (asd_equal(&asd->match, match)) > @@ -473,7 +473,7 @@ v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, > } > > /* Check that an asd does not exist in other notifiers. */ > - list_for_each_entry(notifier, ¬ifier_list, list) > + list_for_each_entry(notifier, ¬ifier_head, notifier_list) > if (__v4l2_async_nf_has_async_subdev(notifier, match)) > return true; > > @@ -510,7 +510,7 @@ static int v4l2_async_nf_asd_valid(struct v4l2_async_notifier *notifier, > > void v4l2_async_nf_init(struct v4l2_async_notifier *notifier) > { > - INIT_LIST_HEAD(¬ifier->asd_list); > + INIT_LIST_HEAD(¬ifier->asd_head); > } > EXPORT_SYMBOL(v4l2_async_nf_init); > > @@ -519,17 +519,17 @@ static int __v4l2_async_nf_register(struct v4l2_async_notifier *notifier) > struct v4l2_async_subdev *asd; > int ret; > > - INIT_LIST_HEAD(¬ifier->waiting); > - INIT_LIST_HEAD(¬ifier->done); > + INIT_LIST_HEAD(¬ifier->waiting_head); > + INIT_LIST_HEAD(¬ifier->done_head); > > mutex_lock(&list_lock); > > - list_for_each_entry(asd, ¬ifier->asd_list, asd_list) { > + list_for_each_entry(asd, ¬ifier->asd_head, asd_list) { > ret = v4l2_async_nf_asd_valid(notifier, &asd->match, true); > if (ret) > goto err_unlock; > > - list_add_tail(&asd->list, ¬ifier->waiting); > + list_add_tail(&asd->waiting_list, ¬ifier->waiting_head); > } > > ret = v4l2_async_nf_try_all_subdevs(notifier); > @@ -541,7 +541,7 @@ static int __v4l2_async_nf_register(struct v4l2_async_notifier *notifier) > goto err_unbind; > > /* Keep also completed notifiers on the list */ > - list_add(¬ifier->list, ¬ifier_list); > + list_add(¬ifier->notifier_list, ¬ifier_head); > > mutex_unlock(&list_lock); > > @@ -606,7 +606,7 @@ __v4l2_async_nf_unregister(struct v4l2_async_notifier *notifier) > notifier->sd = NULL; > notifier->v4l2_dev = NULL; > > - list_del(¬ifier->list); > + list_del(¬ifier->notifier_list); > } > > void v4l2_async_nf_unregister(struct v4l2_async_notifier *notifier) > @@ -623,10 +623,10 @@ static void __v4l2_async_nf_cleanup(struct v4l2_async_notifier *notifier) > { > struct v4l2_async_subdev *asd, *tmp; > > - if (!notifier || !notifier->asd_list.next) > + if (!notifier || !notifier->asd_head.next) > return; > > - list_for_each_entry_safe(asd, tmp, ¬ifier->asd_list, asd_list) { > + list_for_each_entry_safe(asd, tmp, ¬ifier->asd_head, asd_list) { > switch (asd->match.type) { > case V4L2_ASYNC_MATCH_FWNODE: > fwnode_handle_put(asd->match.fwnode); > @@ -662,7 +662,7 @@ int __v4l2_async_nf_add_subdev(struct v4l2_async_notifier *notifier, > if (ret) > goto unlock; > > - list_add_tail(&asd->asd_list, ¬ifier->asd_list); > + list_add_tail(&asd->asd_list, ¬ifier->asd_head); > > unlock: > mutex_unlock(&list_lock); > @@ -768,7 +768,7 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd) > > INIT_LIST_HEAD(&sd->async_list); > > - list_for_each_entry(notifier, ¬ifier_list, list) { > + list_for_each_entry(notifier, ¬ifier_head, notifier_list) { > struct v4l2_device *v4l2_dev = > v4l2_async_nf_find_v4l2_dev(notifier); > struct v4l2_async_subdev *asd; > @@ -792,7 +792,7 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd) > } > > /* None matched, wait for hot-plugging */ > - list_add(&sd->async_list, &subdev_list); > + list_add(&sd->async_list, &subdev_head); > > out_unlock: > mutex_unlock(&list_lock); > @@ -833,7 +833,7 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd) > if (sd->asd) { > struct v4l2_async_notifier *notifier = sd->notifier; > > - list_add(&sd->asd->list, ¬ifier->waiting); > + list_add(&sd->asd->waiting_list, ¬ifier->waiting_head); > > v4l2_async_nf_call_unbind(notifier, sd, sd->asd); > } > @@ -887,9 +887,9 @@ static int pending_subdevs_show(struct seq_file *s, void *data) > > mutex_lock(&list_lock); > > - list_for_each_entry(notif, ¬ifier_list, list) { > + list_for_each_entry(notif, ¬ifier_head, notifier_list) { > seq_printf(s, "%s:\n", v4l2_async_nf_name(notif)); > - list_for_each_entry(asd, ¬if->waiting, list) > + list_for_each_entry(asd, ¬if->waiting_head, waiting_list) > print_waiting_subdev(s, &asd->match); > } > > diff --git a/drivers/staging/media/imx/imx-media-dev-common.c b/drivers/staging/media/imx/imx-media-dev-common.c > index e6d6ed3b1161..eaa54848df6a 100644 > --- a/drivers/staging/media/imx/imx-media-dev-common.c > +++ b/drivers/staging/media/imx/imx-media-dev-common.c > @@ -398,7 +398,7 @@ int imx_media_dev_notifier_register(struct imx_media_dev *imxmd, > int ret; > > /* no subdevs? just bail */ > - if (list_empty(&imxmd->notifier.asd_list)) { > + if (list_empty(&imxmd->notifier.asd_head)) { > v4l2_err(&imxmd->v4l2_dev, "no subdevs\n"); > return -ENODEV; > } > diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c > index 11dd142c98c5..4818646fe637 100644 > --- a/drivers/staging/media/tegra-video/vi.c > +++ b/drivers/staging/media/tegra-video/vi.c > @@ -1563,7 +1563,7 @@ tegra_vi_graph_find_entity(struct tegra_vi_channel *chan, > struct tegra_vi_graph_entity *entity; > struct v4l2_async_subdev *asd; > > - list_for_each_entry(asd, &chan->notifier.asd_list, asd_list) { > + list_for_each_entry(asd, &chan->notifier.asd_head, asd_list) { > entity = to_tegra_vi_graph_entity(asd); > if (entity->asd.match.fwnode == fwnode) > return entity; > @@ -1707,7 +1707,7 @@ static int tegra_vi_graph_notify_complete(struct v4l2_async_notifier *notifier) > } > > /* create links between the entities */ > - list_for_each_entry(asd, &chan->notifier.asd_list, asd_list) { > + list_for_each_entry(asd, &chan->notifier.asd_head, asd_list) { > entity = to_tegra_vi_graph_entity(asd); > ret = tegra_vi_graph_build(chan, entity); > if (ret < 0) > @@ -1874,7 +1874,7 @@ static int tegra_vi_graph_init(struct tegra_vi *vi) > > ret = tegra_vi_graph_parse_one(chan, remote); > fwnode_handle_put(remote); > - if (ret < 0 || list_empty(&chan->notifier.asd_list)) > + if (ret < 0 || list_empty(&chan->notifier.asd_head)) > continue; > > chan->notifier.ops = &tegra_vi_async_ops; > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h > index 0c4cffd081c9..425280b4d387 100644 > --- a/include/media/v4l2-async.h > +++ b/include/media/v4l2-async.h > @@ -68,7 +68,7 @@ struct v4l2_async_match { > * @match: struct of match type and per-bus type matching data sets > * @asd_list: used to add struct v4l2_async_subdev objects to the > * master notifier @asd_list > - * @list: used to link struct v4l2_async_subdev objects, waiting to be > + * @waiting_list: used to link struct v4l2_async_subdev objects, waiting to be > * probed, to a notifier->waiting list notifier->waiting_head > * > * When this struct is used as a member in a driver specific struct, > @@ -77,9 +77,10 @@ struct v4l2_async_match { > */ > struct v4l2_async_subdev { > struct v4l2_async_match match; > + > /* v4l2-async core private: not to be used by drivers */ > - struct list_head list; > struct list_head asd_list; > + struct list_head waiting_list; > }; > > /** > @@ -108,20 +109,20 @@ struct v4l2_async_notifier_operations { > * @v4l2_dev: v4l2_device of the root notifier, NULL otherwise > * @sd: sub-device that registered the notifier, NULL otherwise > * @parent: parent notifier > - * @asd_list: master list of struct v4l2_async_subdev > - * @waiting: list of struct v4l2_async_subdev, waiting for their drivers > - * @done: list of struct v4l2_subdev, already probed > - * @list: member in a global list of notifiers > + * @asd_head: master list of struct v4l2_async_subdev > + * @waiting_list: list of struct v4l2_async_subdev, waiting for their drivers waiting_head > + * @done_head: list of struct v4l2_subdev, already probed > + * @notifier_list: member in a global list of notifiers > */ > struct v4l2_async_notifier { > const struct v4l2_async_notifier_operations *ops; > struct v4l2_device *v4l2_dev; > struct v4l2_subdev *sd; > struct v4l2_async_notifier *parent; > - struct list_head asd_list; > - struct list_head waiting; > - struct list_head done; > - struct list_head list; > + struct list_head asd_head; > + struct list_head waiting_head; > + struct list_head done_head; > + struct list_head notifier_list; > }; > > /** > -- > 2.30.2 >
Hi Jacopo, On Thu, Apr 13, 2023 at 06:58:56PM +0200, Jacopo Mondi wrote: > Hi Sakari > > On Thu, Mar 30, 2023 at 02:58:40PM +0300, Sakari Ailus wrote: > > There's a need to verify that a single async sub-device isn't being added > > multiple times, this would be an error. This takes place at the time of > > adding the async sub-device to the notifier's list as well as when the > > notifier is added to the global notifier's list. > > > > Use the pointer to the sub-device for testing this instead of an index to > > an array that is long gone. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/v4l2-core/v4l2-async.c | 18 ++++++++---------- > > 1 file changed, 8 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > index bb78e3618ab5..fc9ae22e2b47 100644 > > --- a/drivers/media/v4l2-core/v4l2-async.c > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > @@ -456,21 +456,19 @@ __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, > > > > /* > > * Find out whether an async sub-device was set up already or > > - * whether it exists in a given notifier before @this_index. > > - * If @this_index < 0, search the notifier's entire @asd_list. > > + * whether it exists in a given notifier. > > */ > > static bool > > v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, > > - struct v4l2_async_subdev *asd, int this_index) > > + struct v4l2_async_subdev *asd, bool skip_self) > > is skip_self used ? Yes, it should have been there. I'll add it for v2.
Hi Jacopo, On Fri, Apr 14, 2023 at 09:26:21AM +0200, Jacopo Mondi wrote: > > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h > > index 0c4cffd081c9..425280b4d387 100644 > > --- a/include/media/v4l2-async.h > > +++ b/include/media/v4l2-async.h > > @@ -68,7 +68,7 @@ struct v4l2_async_match { > > * @match: struct of match type and per-bus type matching data sets > > * @asd_list: used to add struct v4l2_async_subdev objects to the > > * master notifier @asd_list > > - * @list: used to link struct v4l2_async_subdev objects, waiting to be > > + * @waiting_list: used to link struct v4l2_async_subdev objects, waiting to be > > * probed, to a notifier->waiting list > > notifier->waiting_head Thanks, I'll fix these for v2.
Hi Sakari, On 2023-04-14 14:07:40 +0300, Sakari Ailus wrote: > Hi Jacopo, > > On Thu, Apr 13, 2023 at 06:50:04PM +0200, Jacopo Mondi wrote: > > Hi Sakari > > > > On Thu, Mar 30, 2023 at 02:58:38PM +0300, Sakari Ailus wrote: > > > V4L2 async sub-device matching originally used the device nodes only. > > > Endpoint nodes were taken into use instead as using the device nodes was > > > problematic for it was in some cases ambiguous which link might have been > > > in question. > > > > > > There is however no need to use endpoint nodes on both sides, as the async > > > sub-device's fwnode can always be trivially obtained using > > > fwnode_graph_get_remote_endpoint() when needed while what counts is > > > whether or not the link is between two device nodes, i.e. the device nodes > > > match. > > > > > > > As you know I'm a bit debated. > > > > Strict endpoint-matching requires one subdev to be registed per each > > endpoint, and this is tedious for drivers that have to register a > > subdev for each of its endpoints > > > > Allowing a subdev to be matched multiple times on different endpoints > > gives a way for lazy drivers to take a shortcut and simplify their > > topologies to a single subdev, when they would actually need more. > > I'd say this is really about interface design, not being "lazy". It depends > on the sub-device. Ideally the framework should be also as easy for drivers > drivers to use as possible. > > What is not supported, though, is multiple sub-devices with a single device > node. Do we need that? At least I don't think I came across a driver that > would. If I understand you correctly about multiple sub-device from a single device node, this exists today. The ADV748x driver have a single device node in DT and register multiple sub-devices, one for each source endpoint. The ADV748x have two CSI-2 transmitters, one 4-lane and one 1-lane as well as two different video capture "ports" one HDMI and one CVBS. Both capture ports can be active at the same time and routed internally inside the ADV748x to the two different CSI-2 transmitters. In order todo this the ADV748x register multiple subdevices and modifies the fwnode to be the endpoint instead of the device node. So the change in this patch for ADV748x driver would break that driver. > > Although it would be relatively easy add this in form of a list of > endpoints, if there's a need to. > > Also cc Niklas. > > > > > Also, knowing where this series is going, I wonder if this wouldn't be > > a good time to enforce all async subdevices to be registered with an > > endpoint. From a very quick look at the drivers we have in mainline > > only a few still use v4l2_async_nf_add_fwnode() and only 4 of them > > (camss, rcar_drif, am437x-vpfe, vpif_capture, xilinx_vipp) use as > > match.fwnode what the remote port parent. > > > > I wonder if this would be a good occasione to enforce with a WARN() if > > !fwnode_graph_is_endpoint(fwnode) in v4l2_async_nf_add_fwnode() (or > > possibily, even remove that function and port all drivers to use > > vl2_async_nf_add_fwnode_remote(). I can help, if the above makes sense > > to you as well. > > I prefer to keep things simple for drivers if possible, and this patch does > that by removing a noticeable number of lines from a few drivers. > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > --- > > > drivers/media/i2c/adv748x/adv748x-csi2.c | 3 - > > > drivers/media/i2c/max9286.c | 14 +--- > > > drivers/media/i2c/rdacm20.c | 15 +---- > > > drivers/media/i2c/rdacm21.c | 15 +---- > > > drivers/media/i2c/tc358746.c | 3 - > > > drivers/media/v4l2-core/v4l2-async.c | 86 ++++++------------------ > > > 6 files changed, 24 insertions(+), 112 deletions(-) > > > > > > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c > > > index bd4f3fe0e309..3d830816243f 100644 > > > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c > > > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c > > > @@ -300,9 +300,6 @@ int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx) > > > MEDIA_ENT_F_VID_IF_BRIDGE, > > > is_txa(tx) ? "txa" : "txb"); > > > > > > - /* Ensure that matching is based upon the endpoint fwnodes */ > > > - tx->sd.fwnode = of_fwnode_handle(state->endpoints[tx->port]); I agree it's not nice, but dropping this would break this driver :-( Nacked-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > - > > > /* Register internal ops for incremental subdev registration */ > > > tx->sd.internal_ops = &adv748x_csi2_internal_ops; > > > > > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > > > index 701038d6d19b..2d0f43e3fb9f 100644 > > > --- a/drivers/media/i2c/max9286.c > > > +++ b/drivers/media/i2c/max9286.c > > > @@ -1051,7 +1051,6 @@ static const struct v4l2_ctrl_ops max9286_ctrl_ops = { > > > static int max9286_v4l2_register(struct max9286_priv *priv) > > > { > > > struct device *dev = &priv->client->dev; > > > - struct fwnode_handle *ep; > > > int ret; > > > int i; > > > > > > @@ -1093,25 +1092,14 @@ static int max9286_v4l2_register(struct max9286_priv *priv) > > > if (ret) > > > goto err_async; > > > > > > - ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), MAX9286_SRC_PAD, > > > - 0, 0); > > > - if (!ep) { > > > - dev_err(dev, "Unable to retrieve endpoint on \"port@4\"\n"); > > > - ret = -ENOENT; > > > - goto err_async; > > > - } > > > - priv->sd.fwnode = ep; > > > - > > > > > > You should also remove the fwnode_handle_put() call from > > > > static void max9286_v4l2_unregister(struct max9286_priv *priv) > > { > > fwnode_handle_put(priv->sd.fwnode); > > v4l2_async_unregister_subdev(&priv->sd); > > max9286_v4l2_notifier_unregister(priv); > > } > > Thanks, I'll address these in v2. > > > > > > ret = v4l2_async_register_subdev(&priv->sd); > > > if (ret < 0) { > > > dev_err(dev, "Unable to register subdevice\n"); > > > - goto err_put_node; > > > + goto err_async; > > > } > > > > > > return 0; > > > > > > -err_put_node: > > > - fwnode_handle_put(ep); > > > err_async: > > > v4l2_ctrl_handler_free(&priv->ctrls); > > > max9286_v4l2_notifier_unregister(priv); > > > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c > > > index a2263fa825b5..ea1111152285 100644 > > > --- a/drivers/media/i2c/rdacm20.c > > > +++ b/drivers/media/i2c/rdacm20.c > > > @@ -567,7 +567,6 @@ static int rdacm20_initialize(struct rdacm20_device *dev) > > > static int rdacm20_probe(struct i2c_client *client) > > > { > > > struct rdacm20_device *dev; > > > - struct fwnode_handle *ep; > > > int ret; > > > > > > dev = devm_kzalloc(&client->dev, sizeof(*dev), GFP_KERNEL); > > > @@ -616,24 +615,12 @@ static int rdacm20_probe(struct i2c_client *client) > > > if (ret < 0) > > > goto error_free_ctrls; > > > > > > - ep = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL); > > > - if (!ep) { > > > - dev_err(&client->dev, > > > - "Unable to get endpoint in node %pOF\n", > > > - client->dev.of_node); > > > - ret = -ENOENT; > > > - goto error_free_ctrls; > > > - } > > > - dev->sd.fwnode = ep; > > > > Same for this driver and for rdacm21 > > > > > - > > > ret = v4l2_async_register_subdev(&dev->sd); > > > if (ret) > > > - goto error_put_node; > > > + goto error_free_ctrls; > > > > > > return 0; > > > > > > -error_put_node: > > > - fwnode_handle_put(ep); > > > error_free_ctrls: > > > v4l2_ctrl_handler_free(&dev->ctrls); > > > error: > > > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c > > > index 9ccc56c30d3b..d67cfcb2e05a 100644 > > > --- a/drivers/media/i2c/rdacm21.c > > > +++ b/drivers/media/i2c/rdacm21.c > > > @@ -543,7 +543,6 @@ static int rdacm21_initialize(struct rdacm21_device *dev) > > > static int rdacm21_probe(struct i2c_client *client) > > > { > > > struct rdacm21_device *dev; > > > - struct fwnode_handle *ep; > > > int ret; > > > > > > dev = devm_kzalloc(&client->dev, sizeof(*dev), GFP_KERNEL); > > > @@ -588,24 +587,12 @@ static int rdacm21_probe(struct i2c_client *client) > > > if (ret < 0) > > > goto error_free_ctrls; > > > > > > - ep = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL); > > > - if (!ep) { > > > - dev_err(&client->dev, > > > - "Unable to get endpoint in node %pOF\n", > > > - client->dev.of_node); > > > - ret = -ENOENT; > > > - goto error_free_ctrls; > > > - } > > > - dev->sd.fwnode = ep; > > > - > > > ret = v4l2_async_register_subdev(&dev->sd); > > > if (ret) > > > - goto error_put_node; > > > + goto error_free_ctrls; > > > > > > return 0; > > > > > > -error_put_node: > > > - fwnode_handle_put(dev->sd.fwnode); > > > error_free_ctrls: > > > v4l2_ctrl_handler_free(&dev->ctrls); > > > error: > > > diff --git a/drivers/media/i2c/tc358746.c b/drivers/media/i2c/tc358746.c > > > index 4063754a6732..56f2b43d4edf 100644 > > > --- a/drivers/media/i2c/tc358746.c > > > +++ b/drivers/media/i2c/tc358746.c > > > @@ -1476,9 +1476,6 @@ static int tc358746_async_register(struct tc358746 *tc358746) > > > if (err) > > > goto err_cleanup; > > > > > > - tc358746->sd.fwnode = fwnode_graph_get_endpoint_by_id( > > > - dev_fwnode(tc358746->sd.dev), TC358746_SOURCE, 0, 0); > > > - > > > err = v4l2_async_register_subdev(&tc358746->sd); > > > if (err) > > > goto err_unregister; > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > > index 6dd426c2ca68..13fe0bdc70b6 100644 > > > --- a/drivers/media/v4l2-core/v4l2-async.c > > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > > @@ -86,84 +86,33 @@ match_fwnode_one(struct v4l2_async_notifier *notifier, > > > struct v4l2_subdev *sd, struct fwnode_handle *sd_fwnode, > > > struct v4l2_async_subdev *asd) > > > { > > > - struct fwnode_handle *other_fwnode; > > > - struct fwnode_handle *dev_fwnode; > > > - bool asd_fwnode_is_ep; > > > - bool sd_fwnode_is_ep; > > > - struct device *dev; > > > + struct fwnode_handle *asd_dev_fwnode; > > > + bool ret; > > > > > > dev_dbg(sd->dev, "async: fwnode match: need %pfw, trying %pfw\n", > > > sd_fwnode, asd->match.fwnode); > > > > > > - /* > > > - * Both the subdev and the async subdev can provide either an endpoint > > > - * fwnode or a device fwnode. Start with the simple case of direct > > > - * fwnode matching. > > > - */ > > > if (sd_fwnode == asd->match.fwnode) { > > > dev_dbg(sd->dev, "async: direct match found\n"); > > > return true; > > > } > > > > > > - /* > > > - * Otherwise, check if the sd fwnode and the asd fwnode refer to an > > > - * endpoint or a device. If they're of the same type, there's no match. > > > - * Technically speaking this checks if the nodes refer to a connected > > > - * endpoint, which is the simplest check that works for both OF and > > > - * ACPI. This won't make a difference, as drivers should not try to > > > - * match unconnected endpoints. > > > - */ > > > - sd_fwnode_is_ep = fwnode_graph_is_endpoint(sd_fwnode); > > > - asd_fwnode_is_ep = fwnode_graph_is_endpoint(asd->match.fwnode); > > > - > > > - if (sd_fwnode_is_ep == asd_fwnode_is_ep) { > > > - dev_dbg(sd->dev, "async: matching node types\n"); > > > + if (!fwnode_graph_is_endpoint(asd->match.fwnode)) { > > > + dev_dbg(sd->dev, > > > + "async: async subdev fwnode not endpoint, no match\n"); > > > > As per the previous patch I would just say "direct match failed"; > > > > > return false; > > > } > > > > > > - /* > > > - * The sd and asd fwnodes are of different types. Get the device fwnode > > > - * parent of the endpoint fwnode, and compare it with the other fwnode. > > > - */ > > > - if (sd_fwnode_is_ep) { > > > - dev_fwnode = fwnode_graph_get_port_parent(sd_fwnode); > > > - other_fwnode = asd->match.fwnode; > > > - } else { > > > - dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode); > > > - other_fwnode = sd_fwnode; > > > - } > > > - > > > - dev_dbg(sd->dev, "async: fwnode compat match, need %pfw, trying %pfw\n", > > > - dev_fwnode, other_fwnode); > > > + asd_dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode); > > > > > > - fwnode_handle_put(dev_fwnode); > > > + ret = sd_fwnode == asd_dev_fwnode; > > > > > > - if (dev_fwnode != other_fwnode) { > > > - dev_dbg(sd->dev, "async: compat match not found\n"); > > > - return false; > > > - } > > > + fwnode_handle_put(asd_dev_fwnode); > > > > > > - /* > > > - * We have a heterogeneous match. Retrieve the struct device of the side > > > - * that matched on a device fwnode to print its driver name. > > > - */ > > > - if (sd_fwnode_is_ep) > > > - dev = notifier->v4l2_dev ? notifier->v4l2_dev->dev > > > - : notifier->sd->dev; > > > - else > > > - dev = sd->dev; > > > - > > > - if (dev && dev->driver) { > > > - if (sd_fwnode_is_ep) > > > - dev_warn(dev, "Driver %s uses device fwnode, incorrect match may occur\n", > > > - dev->driver->name); > > > - dev_notice(dev, "Consider updating driver %s to match on endpoints\n", > > > - dev->driver->name); > > > - } > > > - > > > - dev_dbg(sd->dev, "async: compat match found\n"); > > > + dev_dbg(sd->dev, "async: device--endpoint match %sfound\n", > > > > double '--' > > > > However now that I think about it, a subdev will be matched agains a > > rather large number of async subdevs and most attempts will fail.. do > > we want a printout for each of them ? > > Note that this will only happen if these debug prints are enabled. There > was recently a case where they would have been useful. > > > > > > > > + ret ? "" : "not "); > > > > > > - return true; > > > + return ret; > > > } > > > > > > static bool match_fwnode(struct v4l2_async_notifier *notifier, > > > @@ -804,12 +753,19 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd) > > > int ret; > > > > > > /* > > > - * No reference taken. The reference is held by the device > > > - * (struct v4l2_subdev.dev), and async sub-device does not > > > - * exist independently of the device at any point of time. > > > + * No reference taken. The reference is held by the device (struct > > > + * v4l2_subdev.dev), and async sub-device does not exist independently > > > + * of the device at any point of time. > > > + * > > > + * The async sub-device shall always be registered for its device node, > > > + * not the endpoint node. Issue a warning in that case. Once there is > > > + * certainty no driver no longer does this, remove the warning (and > > > + * compatibility code) below. > > > */ > > > if (!sd->fwnode && sd->dev) > > > sd->fwnode = dev_fwnode(sd->dev); > > > + else if (WARN_ON(fwnode_graph_is_endpoint(sd->fwnode))) > > > + sd->fwnode = fwnode_graph_get_port_parent(sd->fwnode); > > > > > > mutex_lock(&list_lock); > > > > > -- > Kind regards, > > Sakari Ailus
Hi Sakari, Thank you for the patch. On Thu, Mar 30, 2023 at 02:58:46PM +0300, Sakari Ailus wrote: > Register V4L2 device before the async notifier so the struct device will > be available for the notifier. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/platform/marvell/cafe-driver.c | 12 ++++++++++-- > drivers/media/platform/marvell/mcam-core.c | 6 ------ > 2 files changed, 10 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/platform/marvell/cafe-driver.c b/drivers/media/platform/marvell/cafe-driver.c > index dd1bba70bd79..4d8843623255 100644 > --- a/drivers/media/platform/marvell/cafe-driver.c > +++ b/drivers/media/platform/marvell/cafe-driver.c > @@ -536,6 +536,11 @@ static int cafe_pci_probe(struct pci_dev *pdev, > if (ret) > goto out_pdown; > > + ret = v4l2_device_register(mcam->dev, &mcam->v4l2_dev); > + if (ret) > + goto out_smbus_shutdown; > + > + > v4l2_async_nf_init(&mcam->notifier); > > asd = v4l2_async_nf_add_i2c(&mcam->notifier, > @@ -544,12 +549,12 @@ static int cafe_pci_probe(struct pci_dev *pdev, > struct v4l2_async_connection); > if (IS_ERR(asd)) { > ret = PTR_ERR(asd); > - goto out_smbus_shutdown; > + goto out_v4l2_device_unregister; > } > > ret = mccic_register(mcam); > if (ret) > - goto out_smbus_shutdown; > + goto out_v4l2_device_unregister; > > clkdev_create(mcam->mclk, "xclk", "%d-%04x", > i2c_adapter_id(cam->i2c_adapter), ov7670_info.addr); > @@ -565,6 +570,8 @@ static int cafe_pci_probe(struct pci_dev *pdev, > > out_mccic_shutdown: > mccic_shutdown(mcam); > +out_v4l2_device_unregister: > + v4l2_device_unregister(&mcam->v4l2_dev); > out_smbus_shutdown: > cafe_smbus_shutdown(cam); > out_pdown: > @@ -587,6 +594,7 @@ static int cafe_pci_probe(struct pci_dev *pdev, > static void cafe_shutdown(struct cafe_camera *cam) > { > mccic_shutdown(&cam->mcam); > + v4l2_device_unregister(&cam->mcam.v4l2_dev); > cafe_smbus_shutdown(cam); > free_irq(cam->pdev->irq, cam); > pci_iounmap(cam->pdev, cam->mcam.regs); > diff --git a/drivers/media/platform/marvell/mcam-core.c b/drivers/media/platform/marvell/mcam-core.c > index b74a362ec075..2ecdcbcb37fd 100644 > --- a/drivers/media/platform/marvell/mcam-core.c > +++ b/drivers/media/platform/marvell/mcam-core.c > @@ -1866,10 +1866,6 @@ int mccic_register(struct mcam_camera *cam) > /* > * Register with V4L > */ > - ret = v4l2_device_register(cam->dev, &cam->v4l2_dev); > - if (ret) > - goto out; > - > mutex_init(&cam->s_mutex); > cam->state = S_NOTREADY; > mcam_set_config_needed(cam, 1); > @@ -1915,7 +1911,6 @@ int mccic_register(struct mcam_camera *cam) > > out: > v4l2_async_nf_unregister(&cam->notifier); > - v4l2_device_unregister(&cam->v4l2_dev); > v4l2_async_nf_cleanup(&cam->notifier); Wouldn't the v4l2_async_nf_* calls be better placed in cafe-driver.c, given that v4l2_async_nf_init() is called there too ? Same below. > return ret; > } > @@ -1937,7 +1932,6 @@ void mccic_shutdown(struct mcam_camera *cam) > mcam_free_dma_bufs(cam); > v4l2_ctrl_handler_free(&cam->ctrl_handler); > v4l2_async_nf_unregister(&cam->notifier); > - v4l2_device_unregister(&cam->v4l2_dev); > v4l2_async_nf_cleanup(&cam->notifier); > } > EXPORT_SYMBOL_GPL(mccic_shutdown);
Hi Sakari, Thank you for the patch. On Thu, Mar 30, 2023 at 02:58:49PM +0300, Sakari Ailus wrote: > Initialise the V4L2 async notifier after registering the V4L2 device, just > before parsing DT for async sub-devices. This way struct device is > available to the notifier right from the beginning. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/platform/xilinx/xilinx-vipp.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c b/drivers/media/platform/xilinx/xilinx-vipp.c > index 70e7a1f6ed3b..83430633ed28 100644 > --- a/drivers/media/platform/xilinx/xilinx-vipp.c > +++ b/drivers/media/platform/xilinx/xilinx-vipp.c > @@ -516,6 +516,8 @@ static int xvip_graph_init(struct xvip_composite_device *xdev) > goto done; > } > > + v4l2_async_nf_init(&xdev->notifier); > + > /* Parse the graph to extract a list of subdevice DT nodes. */ > ret = xvip_graph_parse(xdev); > if (ret < 0) { > @@ -596,7 +598,6 @@ static int xvip_composite_probe(struct platform_device *pdev) > > xdev->dev = &pdev->dev; > INIT_LIST_HEAD(&xdev->dmas); > - v4l2_async_nf_init(&xdev->notifier); > > ret = xvip_composite_v4l2_init(xdev); > if (ret < 0)
Hi Sakari, Thank you for the patch. On Thu, Mar 30, 2023 at 02:58:42PM +0300, Sakari Ailus wrote: > The naming of list heads and list entries is confusing as they're named > similarly. Use _head for list head and _list for list entries. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 2 +- > .../platform/renesas/rcar-vin/rcar-core.c | 2 +- > .../platform/renesas/rzg2l-cru/rzg2l-core.c | 2 +- > drivers/media/platform/xilinx/xilinx-vipp.c | 10 +-- > drivers/media/v4l2-core/v4l2-async.c | 66 +++++++++---------- > .../staging/media/imx/imx-media-dev-common.c | 2 +- > drivers/staging/media/tegra-video/vi.c | 6 +- > include/media/v4l2-async.h | 21 +++--- > 8 files changed, 56 insertions(+), 55 deletions(-) [snip] > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h > index 0c4cffd081c9..425280b4d387 100644 > --- a/include/media/v4l2-async.h > +++ b/include/media/v4l2-async.h > @@ -68,7 +68,7 @@ struct v4l2_async_match { > * @match: struct of match type and per-bus type matching data sets > * @asd_list: used to add struct v4l2_async_subdev objects to the > * master notifier @asd_list It's now called @asd_head. > - * @list: used to link struct v4l2_async_subdev objects, waiting to be > + * @waiting_list: used to link struct v4l2_async_subdev objects, waiting to be > * probed, to a notifier->waiting list It's now called notifier->waiting_head. Please double-check comments and documentation to catch other occurrences. > * > * When this struct is used as a member in a driver specific struct, > @@ -77,9 +77,10 @@ struct v4l2_async_match { > */ > struct v4l2_async_subdev { > struct v4l2_async_match match; > + > /* v4l2-async core private: not to be used by drivers */ > - struct list_head list; > struct list_head asd_list; > + struct list_head waiting_list; > }; > > /** > @@ -108,20 +109,20 @@ struct v4l2_async_notifier_operations { > * @v4l2_dev: v4l2_device of the root notifier, NULL otherwise > * @sd: sub-device that registered the notifier, NULL otherwise > * @parent: parent notifier > - * @asd_list: master list of struct v4l2_async_subdev > - * @waiting: list of struct v4l2_async_subdev, waiting for their drivers > - * @done: list of struct v4l2_subdev, already probed > - * @list: member in a global list of notifiers > + * @asd_head: master list of struct v4l2_async_subdev > + * @waiting_list: list of struct v4l2_async_subdev, waiting for their drivers > + * @done_head: list of struct v4l2_subdev, already probed > + * @notifier_list: member in a global list of notifiers > */ > struct v4l2_async_notifier { > const struct v4l2_async_notifier_operations *ops; > struct v4l2_device *v4l2_dev; > struct v4l2_subdev *sd; > struct v4l2_async_notifier *parent; > - struct list_head asd_list; > - struct list_head waiting; > - struct list_head done; > - struct list_head list; > + struct list_head asd_head; > + struct list_head waiting_head; > + struct list_head done_head; > + struct list_head notifier_list; I find the _head suffix to still be confusing. How about the following ? struct { struct list_head all; struct list_head waiting; struct list_head done; } asds; > }; > > /**
Hi Jacopo, Thank you for the patch. On Fri, Apr 14, 2023 at 11:14:37AM +0200, Jacopo Mondi wrote: > The v4l2_async_nf_parse_fwnode_endpoints() function, part of > v4l2-fwnode.c, was an helper meant to register one async sub-dev s/an helper/a helper/ > for each fwnode endpoint of a device. > > The function is marked as deprecated in the documentation and is > actually not used anywhere anymore. Drop it and remove the helper > function v4l2_async_nf_fwnode_parse_endpoint() from v4l2-fwnode.c. > > This change allows to make the helper function > __v4l2_async_nf_add_connection() visibility private to v4l2-async.c so > that there is no risk drivers can mistakenly use it. > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > drivers/media/v4l2-core/v4l2-async.c | 1 - > drivers/media/v4l2-core/v4l2-fwnode.c | 97 --------------------------- > include/media/v4l2-async.h | 25 ------- > include/media/v4l2-fwnode.h | 65 ------------------ > 4 files changed, 188 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index c9874b3f411e..e4cd70da4814 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -782,7 +782,6 @@ int __v4l2_async_nf_add_connection(struct v4l2_async_notifier *notifier, > mutex_unlock(&list_lock); > return ret; > } > -EXPORT_SYMBOL_GPL(__v4l2_async_nf_add_connection); > > struct v4l2_async_connection * > __v4l2_async_nf_add_fwnode(struct v4l2_async_notifier *notifier, > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c > index 7f4fb6208b1f..a84af48ed4e3 100644 > --- a/drivers/media/v4l2-core/v4l2-fwnode.c > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c > @@ -798,103 +798,6 @@ int v4l2_fwnode_device_parse(struct device *dev, > } > EXPORT_SYMBOL_GPL(v4l2_fwnode_device_parse); > > -static int > -v4l2_async_nf_fwnode_parse_endpoint(struct device *dev, > - struct v4l2_async_notifier *notifier, > - struct fwnode_handle *endpoint, > - unsigned int asd_struct_size, > - parse_endpoint_func parse_endpoint) > -{ > - struct v4l2_fwnode_endpoint vep = { .bus_type = 0 }; > - struct v4l2_async_connection *asd; > - int ret; > - > - asd = kzalloc(asd_struct_size, GFP_KERNEL); > - if (!asd) > - return -ENOMEM; > - > - asd->match.type = V4L2_ASYNC_MATCH_FWNODE; > - asd->match.fwnode = > - fwnode_graph_get_remote_port_parent(endpoint); > - if (!asd->match.fwnode) { > - dev_dbg(dev, "no remote endpoint found\n"); > - ret = -ENOTCONN; > - goto out_err; > - } > - > - ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &vep); > - if (ret) { > - dev_warn(dev, "unable to parse V4L2 fwnode endpoint (%d)\n", > - ret); > - goto out_err; > - } > - > - ret = parse_endpoint ? parse_endpoint(dev, &vep, asd) : 0; > - if (ret == -ENOTCONN) > - dev_dbg(dev, "ignoring port@%u/endpoint@%u\n", vep.base.port, > - vep.base.id); > - else if (ret < 0) > - dev_warn(dev, > - "driver could not parse port@%u/endpoint@%u (%d)\n", > - vep.base.port, vep.base.id, ret); > - v4l2_fwnode_endpoint_free(&vep); > - if (ret < 0) > - goto out_err; > - > - ret = __v4l2_async_nf_add_connection(notifier, asd); > - if (ret < 0) { > - /* not an error if asd already exists */ > - if (ret == -EEXIST) > - ret = 0; > - goto out_err; > - } > - > - return 0; > - > -out_err: > - fwnode_handle_put(asd->match.fwnode); > - kfree(asd); > - > - return ret == -ENOTCONN ? 0 : ret; > -} > - > -int > -v4l2_async_nf_parse_fwnode_endpoints(struct device *dev, > - struct v4l2_async_notifier *notifier, > - size_t asd_struct_size, > - parse_endpoint_func parse_endpoint) > -{ > - struct fwnode_handle *fwnode; > - int ret = 0; > - > - if (WARN_ON(asd_struct_size < sizeof(struct v4l2_async_connection))) > - return -EINVAL; > - > - fwnode_graph_for_each_endpoint(dev_fwnode(dev), fwnode) { > - struct fwnode_handle *dev_fwnode; > - bool is_available; > - > - dev_fwnode = fwnode_graph_get_port_parent(fwnode); > - is_available = fwnode_device_is_available(dev_fwnode); > - fwnode_handle_put(dev_fwnode); > - if (!is_available) > - continue; > - > - > - ret = v4l2_async_nf_fwnode_parse_endpoint(dev, notifier, > - fwnode, > - asd_struct_size, > - parse_endpoint); > - if (ret < 0) > - break; > - } > - > - fwnode_handle_put(fwnode); > - > - return ret; > -} > -EXPORT_SYMBOL_GPL(v4l2_async_nf_parse_fwnode_endpoints); > - > /* > * v4l2_fwnode_reference_parse - parse references for async sub-devices > * @dev: the device node the properties of which are parsed for references > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h > index cf2082e17fc4..44080543e1b9 100644 > --- a/include/media/v4l2-async.h > +++ b/include/media/v4l2-async.h > @@ -167,8 +167,6 @@ void v4l2_async_debug_init(struct dentry *debugfs_dir); > * v4l2_async_nf_add_fwnode_remote(), > * v4l2_async_nf_add_fwnode(), > * v4l2_async_nf_add_i2c(), s/,/./ > - * __v4l2_async_nf_add_connection() or > - * v4l2_async_nf_parse_fwnode_endpoints(). > */ > void v4l2_async_nf_init(struct v4l2_device *v4l2_dev, > struct v4l2_async_notifier *notifier); > @@ -184,31 +182,10 @@ void v4l2_async_nf_init(struct v4l2_device *v4l2_dev, > * v4l2_async_nf_add_fwnode_remote(), > * v4l2_async_nf_add_fwnode(), > * v4l2_async_nf_add_i2c(), s/,/./ > - * __v4l2_async_nf_add_connection() or > - * v4l2_async_nf_parse_fwnode_endpoints(). > */ > void v4l2_async_subdev_nf_init(struct v4l2_subdev *sd, > struct v4l2_async_notifier *notifier); > > -/** > - * __v4l2_async_nf_add_connection() - Add an async subdev to the notifier's > - * master asc list. > - * > - * @notifier: pointer to &struct v4l2_async_notifier > - * @asc: pointer to &struct v4l2_async_connection > - * > - * \warning: Drivers should avoid using this function and instead use one of: > - * v4l2_async_nf_add_fwnode(), > - * v4l2_async_nf_add_fwnode_remote() or > - * v4l2_async_nf_add_i2c(). > - * > - * Call this function before registering a notifier to link the provided @asc to > - * the notifiers master @asc_list. The @asc must be allocated with k*alloc() as > - * it will be freed by the framework when the notifier is destroyed. > - */ You could move this documentation to the .c file (dropping the warning). There's little documentation of internal function for v4l2-async, which makes the code hard to understand. Let's not make it worse by dropping existing documentation :-) > -int __v4l2_async_nf_add_connection(struct v4l2_async_notifier *notifier, > - struct v4l2_async_connection *asc); > - > struct v4l2_async_connection * > __v4l2_async_nf_add_fwnode(struct v4l2_async_notifier *notifier, > struct fwnode_handle *fwnode, > @@ -306,8 +283,6 @@ void v4l2_async_nf_unregister(struct v4l2_async_notifier *notifier); > * v4l2_async_nf_add_fwnode_remote(), > * v4l2_async_nf_add_fwnode(), > * v4l2_async_nf_add_i2c(), s/,/./ > - * __v4l2_async_nf_add_connection() or > - * v4l2_async_nf_parse_fwnode_endpoints(). > * > * There is no harm from calling v4l2_async_nf_cleanup() in other > * cases as long as its memory has been zeroed after it has been > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h > index ebb83154abd5..f84fa73f041c 100644 > --- a/include/media/v4l2-fwnode.h > +++ b/include/media/v4l2-fwnode.h > @@ -393,71 +393,6 @@ int v4l2_fwnode_connector_add_link(struct fwnode_handle *fwnode, > int v4l2_fwnode_device_parse(struct device *dev, > struct v4l2_fwnode_device_properties *props); > > -/** > - * typedef parse_endpoint_func - Driver's callback function to be called on > - * each V4L2 fwnode endpoint. > - * > - * @dev: pointer to &struct device > - * @vep: pointer to &struct v4l2_fwnode_endpoint > - * @asd: pointer to &struct v4l2_async_connection > - * > - * Return: > - * * %0 on success > - * * %-ENOTCONN if the endpoint is to be skipped but this > - * should not be considered as an error > - * * %-EINVAL if the endpoint configuration is invalid > - */ > -typedef int (*parse_endpoint_func)(struct device *dev, > - struct v4l2_fwnode_endpoint *vep, > - struct v4l2_async_connection *asd); > - > -/** > - * v4l2_async_nf_parse_fwnode_endpoints - Parse V4L2 fwnode endpoints in a > - * device node > - * @dev: the device the endpoints of which are to be parsed > - * @notifier: notifier for @dev > - * @asd_struct_size: size of the driver's async sub-device struct, including > - * sizeof(struct v4l2_async_connection). The &struct > - * v4l2_async_connection shall be the first member of > - * the driver's async sub-device struct, i.e. both > - * begin at the same memory address. > - * @parse_endpoint: Driver's callback function called on each V4L2 fwnode > - * endpoint. Optional. > - * > - * DEPRECATED! This function is deprecated. Don't use it in new drivers. > - * Instead see an example in cio2_parse_firmware() function in > - * drivers/media/pci/intel/ipu3/ipu3-cio2.c . > - * > - * Parse the fwnode endpoints of the @dev device and populate the async sub- > - * devices list in the notifier. The @parse_endpoint callback function is > - * called for each endpoint with the corresponding async sub-device pointer to > - * let the caller initialize the driver-specific part of the async sub-device > - * structure. > - * > - * The notifier memory shall be zeroed before this function is called on the > - * notifier. > - * > - * This function may not be called on a registered notifier and may be called on > - * a notifier only once. > - * > - * The &struct v4l2_fwnode_endpoint passed to the callback function > - * @parse_endpoint is released once the function is finished. If there is a need > - * to retain that configuration, the user needs to allocate memory for it. > - * > - * Any notifier populated using this function must be released with a call to > - * v4l2_async_nf_cleanup() after it has been unregistered and the async > - * sub-devices are no longer in use, even if the function returned an error. > - * > - * Return: %0 on success, including when no async sub-devices are found > - * %-ENOMEM if memory allocation failed > - * %-EINVAL if graph or endpoint parsing failed > - * Other error codes as returned by @parse_endpoint > - */ > -int > -v4l2_async_nf_parse_fwnode_endpoints(struct device *dev, > - struct v4l2_async_notifier *notifier, > - size_t asd_struct_size, > - parse_endpoint_func parse_endpoint); > > /* Helper macros to access the connector links. */ >
Hi Sakari, Thank you for the patch. On Thu, Mar 30, 2023 at 02:58:40PM +0300, Sakari Ailus wrote: > There's a need to verify that a single async sub-device isn't being added > multiple times, this would be an error. This takes place at the time of > adding the async sub-device to the notifier's list as well as when the > notifier is added to the global notifier's list. > > Use the pointer to the sub-device for testing this instead of an index to > an array that is long gone. Reading the patch, I have no idea what the "long gone array" is. Could you please expand the commit message to make this easier to review ? v4l2-async is very difficult to follow in general, reviewing this series is painful :-S Let's try to improve it with better commit messages. > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/v4l2-core/v4l2-async.c | 18 ++++++++---------- > 1 file changed, 8 insertions(+), 10 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index bb78e3618ab5..fc9ae22e2b47 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -456,21 +456,19 @@ __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, > > /* > * Find out whether an async sub-device was set up already or > - * whether it exists in a given notifier before @this_index. > - * If @this_index < 0, search the notifier's entire @asd_list. > + * whether it exists in a given notifier. > */ > static bool > v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, > - struct v4l2_async_subdev *asd, int this_index) > + struct v4l2_async_subdev *asd, bool skip_self) > { > struct v4l2_async_subdev *asd_y; > - int j = 0; > > lockdep_assert_held(&list_lock); > > /* Check that an asd is not being added more than once. */ > list_for_each_entry(asd_y, ¬ifier->asd_list, asd_list) { > - if (this_index >= 0 && j++ >= this_index) > + if (asd == asd_y) > break; > if (asd_equal(asd, asd_y)) > return true; > @@ -486,7 +484,7 @@ v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, > > static int v4l2_async_nf_asd_valid(struct v4l2_async_notifier *notifier, > struct v4l2_async_subdev *asd, > - int this_index) > + bool skip_self) > { > struct device *dev = > notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL; > @@ -497,7 +495,7 @@ static int v4l2_async_nf_asd_valid(struct v4l2_async_notifier *notifier, > switch (asd->match.type) { > case V4L2_ASYNC_MATCH_I2C: > case V4L2_ASYNC_MATCH_FWNODE: > - if (v4l2_async_nf_has_async_subdev(notifier, asd, this_index)) { > + if (v4l2_async_nf_has_async_subdev(notifier, asd, skip_self)) { > dev_dbg(dev, "subdev descriptor already listed in this or other notifiers\n"); > return -EEXIST; > } > @@ -520,7 +518,7 @@ EXPORT_SYMBOL(v4l2_async_nf_init); > static int __v4l2_async_nf_register(struct v4l2_async_notifier *notifier) > { > struct v4l2_async_subdev *asd; > - int ret, i = 0; > + int ret; > > INIT_LIST_HEAD(¬ifier->waiting); > INIT_LIST_HEAD(¬ifier->done); > @@ -528,7 +526,7 @@ static int __v4l2_async_nf_register(struct v4l2_async_notifier *notifier) > mutex_lock(&list_lock); > > list_for_each_entry(asd, ¬ifier->asd_list, asd_list) { > - ret = v4l2_async_nf_asd_valid(notifier, asd, i++); > + ret = v4l2_async_nf_asd_valid(notifier, asd, true); > if (ret) > goto err_unlock; > > @@ -661,7 +659,7 @@ int __v4l2_async_nf_add_subdev(struct v4l2_async_notifier *notifier, > > mutex_lock(&list_lock); > > - ret = v4l2_async_nf_asd_valid(notifier, asd, -1); > + ret = v4l2_async_nf_asd_valid(notifier, asd, false); > if (ret) > goto unlock; >
Hi Sakari, Thank you for the patch. On Thu, Mar 30, 2023 at 02:58:36PM +0300, Sakari Ailus wrote: > When an async notifier is unregistered, the async sub-devices in the > notifier's done list will disappear with the notifier. However this is > currently also done to the sub-notifiers that remain registered. Their > sub-devices only need to be unbound while the async sub-devices themselves > need to be returned to the sub-notifier's waiting list. Do this now. > > Fixes: 2cab00bb076b ("media: v4l: async: Allow binding notifiers to sub-devices") > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/v4l2-core/v4l2-async.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index 2f1b718a9189..008a2a3e312e 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -414,7 +414,8 @@ static void v4l2_async_cleanup(struct v4l2_subdev *sd) > > /* Unbind all sub-devices in the notifier tree. */ > static void > -v4l2_async_nf_unbind_all_subdevs(struct v4l2_async_notifier *notifier) > +v4l2_async_nf_unbind_all_subdevs(struct v4l2_async_notifier *notifier, > + bool readd) I've read this as "read d" and was wondering what it meant. Maybe "re_add" would be a better variable name ? Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > { > struct v4l2_subdev *sd, *tmp; > > @@ -423,9 +424,11 @@ v4l2_async_nf_unbind_all_subdevs(struct v4l2_async_notifier *notifier) > v4l2_async_find_subdev_notifier(sd); > > if (subdev_notifier) > - v4l2_async_nf_unbind_all_subdevs(subdev_notifier); > + v4l2_async_nf_unbind_all_subdevs(subdev_notifier, true); > > v4l2_async_nf_call_unbind(notifier, sd, sd->asd); > + if (readd) > + list_add_tail(&sd->asd->list, ¬ifier->waiting); > v4l2_async_cleanup(sd); > > list_move(&sd->async_list, &subdev_list); > @@ -557,7 +560,7 @@ static int __v4l2_async_nf_register(struct v4l2_async_notifier *notifier) > /* > * On failure, unbind all sub-devices registered through this notifier. > */ > - v4l2_async_nf_unbind_all_subdevs(notifier); > + v4l2_async_nf_unbind_all_subdevs(notifier, false); > > err_unlock: > mutex_unlock(&list_lock); > @@ -607,7 +610,7 @@ __v4l2_async_nf_unregister(struct v4l2_async_notifier *notifier) > if (!notifier || (!notifier->v4l2_dev && !notifier->sd)) > return; > > - v4l2_async_nf_unbind_all_subdevs(notifier); > + v4l2_async_nf_unbind_all_subdevs(notifier, false); > > notifier->sd = NULL; > notifier->v4l2_dev = NULL; > @@ -805,7 +808,7 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd) > */ > subdev_notifier = v4l2_async_find_subdev_notifier(sd); > if (subdev_notifier) > - v4l2_async_nf_unbind_all_subdevs(subdev_notifier); > + v4l2_async_nf_unbind_all_subdevs(subdev_notifier, false); > > if (sd->asd) > v4l2_async_nf_call_unbind(notifier, sd, sd->asd);
Hi Sakari, On Mon, Apr 24, 2023 at 10:33:06PM +0300, Sakari Ailus wrote: > On Mon, Apr 24, 2023 at 09:20:22PM +0200, Niklas Söderlund wrote: > > On 2023-04-14 14:07:40 +0300, Sakari Ailus wrote: > > > On Thu, Apr 13, 2023 at 06:50:04PM +0200, Jacopo Mondi wrote: > > > > On Thu, Mar 30, 2023 at 02:58:38PM +0300, Sakari Ailus wrote: > > > > > V4L2 async sub-device matching originally used the device nodes only. > > > > > Endpoint nodes were taken into use instead as using the device nodes was > > > > > problematic for it was in some cases ambiguous which link might have been > > > > > in question. > > > > > > > > > > There is however no need to use endpoint nodes on both sides, as the async > > > > > sub-device's fwnode can always be trivially obtained using > > > > > fwnode_graph_get_remote_endpoint() when needed while what counts is > > > > > whether or not the link is between two device nodes, i.e. the device nodes > > > > > match. > > > > > > > > As you know I'm a bit debated. > > > > > > > > Strict endpoint-matching requires one subdev to be registed per each > > > > endpoint, and this is tedious for drivers that have to register a > > > > subdev for each of its endpoints > > > > > > > > Allowing a subdev to be matched multiple times on different endpoints > > > > gives a way for lazy drivers to take a shortcut and simplify their > > > > topologies to a single subdev, when they would actually need more. > > > > > > I'd say this is really about interface design, not being "lazy". It depends > > > on the sub-device. Ideally the framework should be also as easy for drivers > > > drivers to use as possible. > > > > > > What is not supported, though, is multiple sub-devices with a single device > > > node. Do we need that? At least I don't think I came across a driver that > > > would. > > > > If I understand you correctly about multiple sub-device from a single > > device node, this exists today. The ADV748x driver have a single device > > node in DT and register multiple sub-devices, one for each source > > endpoint. > > > > The ADV748x have two CSI-2 transmitters, one 4-lane and one 1-lane as > > well as two different video capture "ports" one HDMI and one CVBS. Both > > capture ports can be active at the same time and routed internally > > inside the ADV748x to the two different CSI-2 transmitters. > > > > In order todo this the ADV748x register multiple subdevices and modifies > > the fwnode to be the endpoint instead of the device node. So the change > > in this patch for ADV748x driver would break that driver. > > Ah, indeed. I guess I'll need to support that case as well then. It doesn't > seem to be troublesome to implement, but I'm tempted making it a special > case: every other driver would apparently be fine matching with device > fwnode whereas doing endpoint-to-endpoint matching adds complexity to the > drivers. This patch removes about 100 lines of rather ugly code largely > from v4l2-async. It's only 50 lines from v4l2-async, and I don't think the code is uglier than the rest of the file :-) In general, I prefer implementing tricky code in the framework and simplifying drivers. I think our goals align there, the framework should do the right thing by default for the majority of cases. However, as Niklas pointed out, endpoint matching is needed for drivers that register multiple subdevs with external connections (such as the adv742x), and that's exactly why endpoint matching was added in the first place. I think this needs to be kept. > There are other issues in the set with connection-subdev relations, I'll > post v2 to address those as well.
Hi Laurent, On Tue, Apr 25, 2023 at 04:28:57AM +0300, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Thu, Mar 30, 2023 at 02:58:36PM +0300, Sakari Ailus wrote: > > When an async notifier is unregistered, the async sub-devices in the > > notifier's done list will disappear with the notifier. However this is > > currently also done to the sub-notifiers that remain registered. Their > > sub-devices only need to be unbound while the async sub-devices themselves > > need to be returned to the sub-notifier's waiting list. Do this now. > > > > Fixes: 2cab00bb076b ("media: v4l: async: Allow binding notifiers to sub-devices") > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/v4l2-core/v4l2-async.c | 13 ++++++++----- > > 1 file changed, 8 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > index 2f1b718a9189..008a2a3e312e 100644 > > --- a/drivers/media/v4l2-core/v4l2-async.c > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > @@ -414,7 +414,8 @@ static void v4l2_async_cleanup(struct v4l2_subdev *sd) > > > > /* Unbind all sub-devices in the notifier tree. */ > > static void > > -v4l2_async_nf_unbind_all_subdevs(struct v4l2_async_notifier *notifier) > > +v4l2_async_nf_unbind_all_subdevs(struct v4l2_async_notifier *notifier, > > + bool readd) > > I've read this as "read d" and was wondering what it meant. Maybe > "re_add" would be a better variable name ? > > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> Thank you. The patch has been already merged and the argument will soon disappear with the async rework patchset.
Hi Laurent, On Tue, Apr 25, 2023 at 04:15:41AM +0300, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Thu, Mar 30, 2023 at 02:58:40PM +0300, Sakari Ailus wrote: > > There's a need to verify that a single async sub-device isn't being added > > multiple times, this would be an error. This takes place at the time of > > adding the async sub-device to the notifier's list as well as when the > > notifier is added to the global notifier's list. > > > > Use the pointer to the sub-device for testing this instead of an index to > > an array that is long gone. > > Reading the patch, I have no idea what the "long gone array" is. Could > you please expand the commit message to make this easier to review ? Yes... the async sub-devices were placed in an array earlier, that's what the index was referring to. Although this could be an entry in a linked list. Not how they are usually referred to though. This will go away permanently later on in the set. I'll add this to the commit message. > v4l2-async is very difficult to follow in general, reviewing this series > is painful :-S Let's try to improve it with better commit messages.
Hi Laurent, On Thu, Apr 27, 2023 at 08:36:21PM +0300, Laurent Pinchart wrote: > > > > - struct list_head asd_list; > > > > - struct list_head waiting; > > > > - struct list_head done; > > > > - struct list_head list; > > > > + struct list_head asd_head; > > > > + struct list_head waiting_head; > > > > + struct list_head done_head; > > > > + struct list_head notifier_list; > > > > > > I find the _head suffix to still be confusing. How about the following ? > > > > > > struct { > > > struct list_head all; > > > struct list_head waiting; > > > struct list_head done; > > > } asds; > > > > There are many list heads and entries in v4l2-async related structs and > > before this patch. _head is used for all list heads, _list for list > > entries. I prefer having _head as this way it is trivial to look for all > > instances of that list head, removing the _head part makes this much > > harder. > > > > How about using _entry for list entries instead? > > I like that. I would have used _entry for the list entries, and _list > for the list "heads". I don't like the _head suffix very much, as all of > them are struct list_head instances. I won't nack the series for this > though :-) I'm fine with "_list" suffix for list heads, I happened to choose "_head" when writing the set. It's trivial to change that.
Hi Laurent, On Tue, Apr 25, 2023 at 03:27:11AM +0300, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Thu, Mar 30, 2023 at 02:58:46PM +0300, Sakari Ailus wrote: > > Register V4L2 device before the async notifier so the struct device will > > be available for the notifier. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/platform/marvell/cafe-driver.c | 12 ++++++++++-- > > drivers/media/platform/marvell/mcam-core.c | 6 ------ > > 2 files changed, 10 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/media/platform/marvell/cafe-driver.c b/drivers/media/platform/marvell/cafe-driver.c > > index dd1bba70bd79..4d8843623255 100644 > > --- a/drivers/media/platform/marvell/cafe-driver.c > > +++ b/drivers/media/platform/marvell/cafe-driver.c > > @@ -536,6 +536,11 @@ static int cafe_pci_probe(struct pci_dev *pdev, > > if (ret) > > goto out_pdown; > > > > + ret = v4l2_device_register(mcam->dev, &mcam->v4l2_dev); > > + if (ret) > > + goto out_smbus_shutdown; > > + > > + > > v4l2_async_nf_init(&mcam->notifier); > > > > asd = v4l2_async_nf_add_i2c(&mcam->notifier, > > @@ -544,12 +549,12 @@ static int cafe_pci_probe(struct pci_dev *pdev, > > struct v4l2_async_connection); > > if (IS_ERR(asd)) { > > ret = PTR_ERR(asd); > > - goto out_smbus_shutdown; > > + goto out_v4l2_device_unregister; > > } > > > > ret = mccic_register(mcam); > > if (ret) > > - goto out_smbus_shutdown; > > + goto out_v4l2_device_unregister; > > > > clkdev_create(mcam->mclk, "xclk", "%d-%04x", > > i2c_adapter_id(cam->i2c_adapter), ov7670_info.addr); > > @@ -565,6 +570,8 @@ static int cafe_pci_probe(struct pci_dev *pdev, > > > > out_mccic_shutdown: > > mccic_shutdown(mcam); > > +out_v4l2_device_unregister: > > + v4l2_device_unregister(&mcam->v4l2_dev); > > out_smbus_shutdown: > > cafe_smbus_shutdown(cam); > > out_pdown: > > @@ -587,6 +594,7 @@ static int cafe_pci_probe(struct pci_dev *pdev, > > static void cafe_shutdown(struct cafe_camera *cam) > > { > > mccic_shutdown(&cam->mcam); > > + v4l2_device_unregister(&cam->mcam.v4l2_dev); > > cafe_smbus_shutdown(cam); > > free_irq(cam->pdev->irq, cam); > > pci_iounmap(cam->pdev, cam->mcam.regs); > > diff --git a/drivers/media/platform/marvell/mcam-core.c b/drivers/media/platform/marvell/mcam-core.c > > index b74a362ec075..2ecdcbcb37fd 100644 > > --- a/drivers/media/platform/marvell/mcam-core.c > > +++ b/drivers/media/platform/marvell/mcam-core.c > > @@ -1866,10 +1866,6 @@ int mccic_register(struct mcam_camera *cam) > > /* > > * Register with V4L > > */ > > - ret = v4l2_device_register(cam->dev, &cam->v4l2_dev); > > - if (ret) > > - goto out; > > - > > mutex_init(&cam->s_mutex); > > cam->state = S_NOTREADY; > > mcam_set_config_needed(cam, 1); > > @@ -1915,7 +1911,6 @@ int mccic_register(struct mcam_camera *cam) > > > > out: > > v4l2_async_nf_unregister(&cam->notifier); > > - v4l2_device_unregister(&cam->v4l2_dev); > > v4l2_async_nf_cleanup(&cam->notifier); > > Wouldn't the v4l2_async_nf_* calls be better placed in cafe-driver.c, > given that v4l2_async_nf_init() is called there too ? Same below. Probably yes, but I'd like to avoid making unnecessary changes to drivers I can't test. > > > return ret; > > } > > @@ -1937,7 +1932,6 @@ void mccic_shutdown(struct mcam_camera *cam) > > mcam_free_dma_bufs(cam); > > v4l2_ctrl_handler_free(&cam->ctrl_handler); > > v4l2_async_nf_unregister(&cam->notifier); > > - v4l2_device_unregister(&cam->v4l2_dev); > > v4l2_async_nf_cleanup(&cam->notifier); > > } > > EXPORT_SYMBOL_GPL(mccic_shutdown); >
Hi Laurent, On Tue, Apr 25, 2023 at 04:06:54AM +0300, Laurent Pinchart wrote: > > -/** > > - * __v4l2_async_nf_add_connection() - Add an async subdev to the notifier's > > - * master asc list. > > - * > > - * @notifier: pointer to &struct v4l2_async_notifier > > - * @asc: pointer to &struct v4l2_async_connection > > - * > > - * \warning: Drivers should avoid using this function and instead use one of: > > - * v4l2_async_nf_add_fwnode(), > > - * v4l2_async_nf_add_fwnode_remote() or > > - * v4l2_async_nf_add_i2c(). > > - * > > - * Call this function before registering a notifier to link the provided @asc to > > - * the notifiers master @asc_list. The @asc must be allocated with k*alloc() as > > - * it will be freed by the framework when the notifier is destroyed. > > - */ > > You could move this documentation to the .c file (dropping the warning). > There's little documentation of internal function for v4l2-async, which > makes the code hard to understand. Let's not make it worse by dropping > existing documentation :-) As this is no longer usable by drivers, I'm not sure how relevant keeping this around for just v4l2-async internal use is. For instance, did I read any of these comments when writing this patchset? I can say I did not read a single one of them. They are not specific enough for understanding the implementation anyway, this is written for driver authors in mind. Instead what can and probably should be added are object relations and why certain non-obvious things are done the way they are. I'll add this for the patches when the code as such seems fine. I'll address the rest of the comments in v2.
Hi all, This set adds support for multiple downstream links in an async sub-device, by separating the sub-device registration from the link creation. A new concept, V4L2 async connection is added. Generally async notifiers have a number of connections but at that level there is no knowledge of how many sub-devices they will connect to. The bound and unbound callbacks now work on connections. For the existing drivers there's only one connection so I do not expect regressions because of that. Async sub-device fwnode matching will now take place between the device (the dev field of struct v4l2_subdev) and a struct v4l2_async_connection (an endpoint for devices that have endpoints or the device for those that do not). This is because the graph data structure only describes point-to-point connections so therefore defining one end of the connection defines the entire connection. This set is unlikely to address all needs people have related to the async framework but I think that beyond what it does, it paves some way for addressing more of those additional needs. To be frank, I'd like to get rid of the entire V4L2 async framework, but it would require allowing much more dynamic driver initialisation, including sub-devices and device nodes popping up in the system in the order and extent there is successfully probed hardware. Until that, and this may well be the entire foreseeable future, we have at least some of this complexity. There's a bugfix, too, in the first patch. That should be merged separately. The rest depends on it so I'm sending it as part of the set. since RFC v1: - Address missing API usage changes in a lot of drivers. - Fix compilation problems in intermediate patches. - Move V4L2 device registration earlier or move notifier initialisation and fwnode endpoint parsing past the current V4L2 device registration (patches 11--16). Sakari Ailus (18): media: v4l: async: Return async sub-devices to subnotifier list media: v4l: async: Add some debug prints media: v4l: async: Simplify async sub-device fwnode matching media: v4l: async: Make V4L2 async match information a struct media: v4l: async: Clean testing for duplicated async subdevs media: v4l: async: Only pass match information for async subdev validation media: v4l: async: Clean up list heads and entries media: v4l: async: Rename v4l2_async_subdev as v4l2_async_connection media: v4l: async: Differentiate connecting and creating sub-devices media: pxa_camera: Register V4L2 device early, fix probe error handling media: marvell: cafe: Register V4L2 device earlier media: am437x-vpfe: Register V4L2 device early media: omap3isp: Initialise V4L2 async notifier later media: xilinx-vipp: Init async notifier after registering V4L2 device media: davinci: Init async notifier after registering V4L2 device media: qcom: Initialise V4L2 async notifier later media: v4l: async: Set v4l2_device in async notifier init Documentation: media: Document sub-device notifiers .../driver-api/media/v4l2-subdev.rst | 16 +- drivers/media/i2c/adv748x/adv748x-csi2.c | 3 - drivers/media/i2c/max9286.c | 27 +- drivers/media/i2c/rdacm20.c | 15 +- drivers/media/i2c/rdacm21.c | 15 +- drivers/media/i2c/st-mipid02.c | 12 +- drivers/media/i2c/tc358746.c | 13 +- drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 14 +- drivers/media/platform/atmel/atmel-isi.c | 12 +- drivers/media/platform/atmel/atmel-isi.h | 2 +- drivers/media/platform/cadence/cdns-csi2rx.c | 10 +- drivers/media/platform/intel/pxa_camera.c | 46 +- drivers/media/platform/marvell/cafe-driver.c | 19 +- drivers/media/platform/marvell/mcam-core.c | 12 +- drivers/media/platform/marvell/mmp-driver.c | 6 +- .../platform/microchip/microchip-csi2dc.c | 11 +- .../platform/microchip/microchip-isc-base.c | 4 +- .../media/platform/microchip/microchip-isc.h | 2 +- .../microchip/microchip-sama5d2-isc.c | 9 +- .../microchip/microchip-sama7g5-isc.c | 9 +- drivers/media/platform/nxp/imx-mipi-csis.c | 10 +- drivers/media/platform/nxp/imx7-media-csi.c | 10 +- drivers/media/platform/qcom/camss/camss.c | 26 +- drivers/media/platform/qcom/camss/camss.h | 2 +- drivers/media/platform/renesas/rcar-isp.c | 12 +- .../platform/renesas/rcar-vin/rcar-core.c | 26 +- .../platform/renesas/rcar-vin/rcar-csi2.c | 12 +- .../platform/renesas/rcar-vin/rcar-vin.h | 4 +- drivers/media/platform/renesas/rcar_drif.c | 12 +- drivers/media/platform/renesas/renesas-ceu.c | 10 +- .../platform/renesas/rzg2l-cru/rzg2l-core.c | 14 +- .../platform/renesas/rzg2l-cru/rzg2l-cru.h | 2 +- .../platform/renesas/rzg2l-cru/rzg2l-csi2.c | 12 +- .../platform/rockchip/rkisp1/rkisp1-dev.c | 12 +- .../platform/samsung/exynos4-is/media-dev.c | 11 +- .../platform/samsung/exynos4-is/media-dev.h | 2 +- drivers/media/platform/st/stm32/stm32-dcmi.c | 12 +- .../platform/sunxi/sun4i-csi/sun4i_csi.c | 10 +- .../sunxi/sun6i-csi/sun6i_csi_bridge.c | 8 +- .../sunxi/sun6i-csi/sun6i_csi_bridge.h | 2 +- .../sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c | 10 +- .../sun8i_a83t_mipi_csi2.c | 10 +- .../media/platform/ti/am437x/am437x-vpfe.c | 37 +- .../media/platform/ti/am437x/am437x-vpfe.h | 2 +- drivers/media/platform/ti/cal/cal.c | 10 +- .../media/platform/ti/davinci/vpif_capture.c | 33 +- drivers/media/platform/ti/omap3isp/isp.c | 17 +- drivers/media/platform/video-mux.c | 10 +- drivers/media/platform/xilinx/xilinx-vipp.c | 29 +- drivers/media/v4l2-core/v4l2-async.c | 601 +++++++++++------- drivers/media/v4l2-core/v4l2-fwnode.c | 20 +- .../media/deprecated/atmel/atmel-isc-base.c | 4 +- .../media/deprecated/atmel/atmel-isc.h | 2 +- .../deprecated/atmel/atmel-sama5d2-isc.c | 9 +- .../deprecated/atmel/atmel-sama7g5-isc.c | 4 +- drivers/staging/media/imx/imx-media-csi.c | 10 +- .../staging/media/imx/imx-media-dev-common.c | 8 +- drivers/staging/media/imx/imx-media-dev.c | 2 +- drivers/staging/media/imx/imx-media-of.c | 4 +- drivers/staging/media/imx/imx6-mipi-csi2.c | 12 +- drivers/staging/media/imx/imx8mq-mipi-csi2.c | 10 +- .../media/sunxi/sun6i-isp/sun6i_isp_proc.c | 6 +- .../media/sunxi/sun6i-isp/sun6i_isp_proc.h | 2 +- drivers/staging/media/tegra-video/vi.c | 18 +- drivers/staging/media/tegra-video/vi.h | 2 +- include/media/davinci/vpif_types.h | 2 +- include/media/v4l2-async.h | 181 +++--- include/media/v4l2-fwnode.h | 10 +- include/media/v4l2-subdev.h | 2 +- 69 files changed, 825 insertions(+), 708 deletions(-)