| Message ID | 20230525091615.2324824-1-sakari.ailus@linux.intel.com |
|---|---|
| Headers | show |
| Series | Separate links and async sub-devices | expand |
Hi Sakari, Thank you for the patch. On Thu, May 25, 2023 at 12:15:52PM +0300, Sakari Ailus wrote: > Make V4L2 async match information a struct, making it easier to use it > elsewhere outside the scope of struct v4l2_async_subdev. > > Also remove an obsolete comment --- none of these fields are supposed to > be touched by drivers. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/v4l2-core/v4l2-async.c | 20 +++++++------- > include/media/v4l2-async.h | 41 ++++++++++++++++------------ > 2 files changed, 33 insertions(+), 28 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index 7c924faac4c10..7f56648e40c44 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -212,7 +212,7 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier, > > list_for_each_entry(asd, ¬ifier->waiting, list) { > /* bus_type has been verified valid before */ > - switch (asd->match_type) { > + switch (asd->match.type) { > case V4L2_ASYNC_MATCH_I2C: > match = match_i2c; > break; > @@ -237,10 +237,10 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier, > static bool asd_equal(struct v4l2_async_subdev *asd_x, > struct v4l2_async_subdev *asd_y) > { > - if (asd_x->match_type != asd_y->match_type) > + if (asd_x->match.type != asd_y->match.type) > return false; > > - switch (asd_x->match_type) { > + switch (asd_x->match.type) { > case V4L2_ASYNC_MATCH_I2C: > return asd_x->match.i2c.adapter_id == > asd_y->match.i2c.adapter_id && > @@ -552,7 +552,7 @@ static int v4l2_async_nf_asd_valid(struct v4l2_async_notifier *notifier, > { > struct device *dev = notifier_dev(notifier); > > - switch (asd->match_type) { > + switch (asd->match.type) { > case V4L2_ASYNC_MATCH_I2C: > case V4L2_ASYNC_MATCH_FWNODE: > if (v4l2_async_nf_has_async_subdev(notifier, asd, skip_self)) { > @@ -561,8 +561,8 @@ static int v4l2_async_nf_asd_valid(struct v4l2_async_notifier *notifier, > } > break; > default: > - dev_err(dev, "v4l2-async: Invalid match type %u on %p\n", > - asd->match_type, asd); > + dev_err(dev, "v4l2-asymc: Invalid match type %u on %p\n", Is this for asymmetrical notification ? With this fixed, Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > + asd->match.type, asd); > return -EINVAL; > } > > @@ -688,7 +688,7 @@ static void __v4l2_async_nf_cleanup(struct v4l2_async_notifier *notifier) > return; > > list_for_each_entry_safe(asd, tmp, ¬ifier->asd_list, asd_list) { > - switch (asd->match_type) { > + switch (asd->match.type) { > case V4L2_ASYNC_MATCH_FWNODE: > fwnode_handle_put(asd->match.fwnode); > break; > @@ -743,7 +743,7 @@ __v4l2_async_nf_add_fwnode(struct v4l2_async_notifier *notifier, > if (!asd) > return ERR_PTR(-ENOMEM); > > - asd->match_type = V4L2_ASYNC_MATCH_FWNODE; > + asd->match.type = V4L2_ASYNC_MATCH_FWNODE; > asd->match.fwnode = fwnode_handle_get(fwnode); > > ret = __v4l2_async_nf_add_subdev(notifier, asd); > @@ -790,7 +790,7 @@ __v4l2_async_nf_add_i2c(struct v4l2_async_notifier *notifier, int adapter_id, > if (!asd) > return ERR_PTR(-ENOMEM); > > - asd->match_type = V4L2_ASYNC_MATCH_I2C; > + asd->match.type = V4L2_ASYNC_MATCH_I2C; > asd->match.i2c.adapter_id = adapter_id; > asd->match.i2c.address = address; > > @@ -901,7 +901,7 @@ EXPORT_SYMBOL(v4l2_async_unregister_subdev); > static void print_waiting_subdev(struct seq_file *s, > struct v4l2_async_subdev *asd) > { > - switch (asd->match_type) { > + switch (asd->match.type) { > case V4L2_ASYNC_MATCH_I2C: > seq_printf(s, " [i2c] dev=%d-%04x\n", asd->match.i2c.adapter_id, > asd->match.i2c.address); > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h > index 2c9baa3c9266a..d347ef32f4ecb 100644 > --- a/include/media/v4l2-async.h > +++ b/include/media/v4l2-async.h > @@ -34,23 +34,37 @@ enum v4l2_async_match_type { > }; > > /** > - * struct v4l2_async_subdev - sub-device descriptor, as known to a bridge > + * struct v4l2_async_match_desc - async sub-device match information > * > - * @match_type: type of match that will be used > - * @match: union of per-bus type matching data sets > - * @match.fwnode: > - * pointer to &struct fwnode_handle to be matched. > + * @type: type of match that will be used > + * @fwnode: pointer to &struct fwnode_handle to be matched. > * Used if @match_type is %V4L2_ASYNC_MATCH_FWNODE. > - * @match.i2c: embedded struct with I2C parameters to be matched. > + * @i2c: embedded struct with I2C parameters to be matched. > * Both @match.i2c.adapter_id and @match.i2c.address > * should be matched. > * Used if @match_type is %V4L2_ASYNC_MATCH_I2C. > - * @match.i2c.adapter_id: > + * @i2c.adapter_id: > * I2C adapter ID to be matched. > * Used if @match_type is %V4L2_ASYNC_MATCH_I2C. > - * @match.i2c.address: > + * @i2c.address: > * I2C address to be matched. > * Used if @match_type is %V4L2_ASYNC_MATCH_I2C. > + */ > +struct v4l2_async_match_desc { > + enum v4l2_async_match_type type; > + union { > + struct fwnode_handle *fwnode; > + struct { > + int adapter_id; > + unsigned short address; > + } i2c; > + }; > +}; > + > +/** > + * struct v4l2_async_subdev - sub-device descriptor, as known to a bridge > + * > + * @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 > @@ -61,16 +75,7 @@ enum v4l2_async_match_type { > * v4l2_async_subdev as its first member. > */ > struct v4l2_async_subdev { > - enum v4l2_async_match_type match_type; > - union { > - struct fwnode_handle *fwnode; > - struct { > - int adapter_id; > - unsigned short address; > - } i2c; > - } match; > - > - /* v4l2-async core private: not to be used by drivers */ > + struct v4l2_async_match_desc match; > struct list_head list; > struct list_head asd_list; > };
Hi Sakari, Thank you for the patch. On Thu, May 25, 2023 at 12:15:54PM +0300, Sakari Ailus wrote: > Pass only information required for sub-device matching to functions > checking whether the async sub-device already exists. Do the same for > debug message printing. This makes further changes to other aspects of > async sub-devices easier. > > Accordingly, also perform further renames: > > asd_equal as v4l2_async_match_equal, > v4l2_async_nf_has_async_subdev as v4l2_async_nf_has_async_match, > __v4l2_async_nf_has_async_subdev as > v4l2_async_nf_has_async_subdev_entry and > v4l2_async_nf_asd_valid as v4l2_async_nf_match_valid. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/v4l2-core/v4l2-async.c | 109 ++++++++++++++------------- > 1 file changed, 56 insertions(+), 53 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index 93234c316aa6e..5eb9850f1c6c4 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -64,14 +64,15 @@ static void v4l2_async_nf_call_destroy(struct v4l2_async_notifier *n, > } > > static bool match_i2c(struct v4l2_async_notifier *notifier, > - struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > + struct v4l2_subdev *sd, > + struct v4l2_async_match_desc *match) > { > #if IS_ENABLED(CONFIG_I2C) > struct i2c_client *client = i2c_verify_client(sd->dev); > > return client && > - asd->match.i2c.adapter_id == client->adapter->nr && > - asd->match.i2c.address == client->addr; > + match->i2c.adapter_id == client->adapter->nr && > + match->i2c.address == client->addr; > #else > return false; > #endif > @@ -91,7 +92,7 @@ static struct device *notifier_dev(struct v4l2_async_notifier *notifier) > static bool > match_fwnode_one(struct v4l2_async_notifier *notifier, > struct v4l2_subdev *sd, struct fwnode_handle *sd_fwnode, > - struct v4l2_async_subdev *asd) > + struct v4l2_async_match_desc *match) > { > struct fwnode_handle *other_fwnode; > struct fwnode_handle *dev_fwnode; > @@ -101,14 +102,14 @@ match_fwnode_one(struct v4l2_async_notifier *notifier, > > dev_dbg(notifier_dev(notifier), > "v4l2-async: fwnode match: need %pfw, trying %pfw\n", > - sd_fwnode, asd->match.fwnode); > + sd_fwnode, 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) { > + if (sd_fwnode == match->fwnode) { > dev_dbg(notifier_dev(notifier), > "v4l2-async: direct match found\n"); > return true; > @@ -123,7 +124,7 @@ match_fwnode_one(struct v4l2_async_notifier *notifier, > * match unconnected endpoints. > */ > sd_fwnode_is_ep = fwnode_graph_is_endpoint(sd_fwnode); > - asd_fwnode_is_ep = fwnode_graph_is_endpoint(asd->match.fwnode); > + asd_fwnode_is_ep = fwnode_graph_is_endpoint(match->fwnode); > > if (sd_fwnode_is_ep == asd_fwnode_is_ep) { > dev_dbg(notifier_dev(notifier), > @@ -137,9 +138,9 @@ match_fwnode_one(struct v4l2_async_notifier *notifier, > */ > if (sd_fwnode_is_ep) { > dev_fwnode = fwnode_graph_get_port_parent(sd_fwnode); > - other_fwnode = asd->match.fwnode; > + other_fwnode = match->fwnode; > } else { > - dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode); > + dev_fwnode = fwnode_graph_get_port_parent(match->fwnode); > other_fwnode = sd_fwnode; > } > > @@ -179,13 +180,14 @@ match_fwnode_one(struct v4l2_async_notifier *notifier, > } > > static bool match_fwnode(struct v4l2_async_notifier *notifier, > - struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > + struct v4l2_subdev *sd, > + struct v4l2_async_match_desc *match) > { > dev_dbg(notifier_dev(notifier), > "v4l2-async: matching for notifier %pfw, sd fwnode %pfw\n", > dev_fwnode(notifier_dev(notifier)), sd->fwnode); > > - if (match_fwnode_one(notifier, sd, sd->fwnode, asd)) > + if (match_fwnode_one(notifier, sd, sd->fwnode, match)) > return true; > > /* Also check the secondary fwnode. */ > @@ -195,7 +197,7 @@ static bool match_fwnode(struct v4l2_async_notifier *notifier, > dev_dbg(notifier_dev(notifier), > "v4l2-async: trying secondary fwnode match\n"); > > - return match_fwnode_one(notifier, sd, sd->fwnode->secondary, asd); > + return match_fwnode_one(notifier, sd, sd->fwnode->secondary, match); > } > > static LIST_HEAD(subdev_list); > @@ -207,7 +209,8 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier, > struct v4l2_subdev *sd) > { > bool (*match)(struct v4l2_async_notifier *notifier, > - struct v4l2_subdev *sd, struct v4l2_async_subdev *asd); > + struct v4l2_subdev *sd, > + struct v4l2_async_match_desc *match); > struct v4l2_async_subdev *asd; > > list_for_each_entry(asd, ¬ifier->waiting, list) { > @@ -226,7 +229,7 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier, > } > > /* match cannot be NULL here */ > - if (match(notifier, sd, asd)) > + if (match(notifier, sd, &asd->match)) > return asd; > } > > @@ -234,20 +237,18 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier, > } > > /* Compare two async sub-device descriptors for equivalence */ s/sub-device/match/ ? Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > -static bool asd_equal(struct v4l2_async_subdev *asd_x, > - struct v4l2_async_subdev *asd_y) > +static bool v4l2_async_match_equal(struct v4l2_async_match_desc *match1, > + struct v4l2_async_match_desc *match2) > { > - if (asd_x->match.type != asd_y->match.type) > + if (match1->type != match2->type) > return false; > > - switch (asd_x->match.type) { > + switch (match1->type) { > case V4L2_ASYNC_MATCH_TYPE_I2C: > - return asd_x->match.i2c.adapter_id == > - asd_y->match.i2c.adapter_id && > - asd_x->match.i2c.address == > - asd_y->match.i2c.address; > + return match1->i2c.adapter_id == match2->i2c.adapter_id && > + match1->i2c.address == match2->i2c.address; > case V4L2_ASYNC_MATCH_TYPE_FWNODE: > - return asd_x->match.fwnode == asd_y->match.fwnode; > + return match1->fwnode == match2->fwnode; > default: > break; > } > @@ -497,21 +498,21 @@ v4l2_async_nf_unbind_all_subdevs(struct v4l2_async_notifier *notifier, > > /* See if an async sub-device can be found in a notifier's lists. */ > static bool > -__v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, > - struct v4l2_async_subdev *asd) > +v4l2_async_nf_has_async_match_entry(struct v4l2_async_notifier *notifier, > + struct v4l2_async_match_desc *match) > { > - struct v4l2_async_subdev *asd_y; > + struct v4l2_async_subdev *asd; > struct v4l2_subdev *sd; > > - list_for_each_entry(asd_y, ¬ifier->waiting, list) > - if (asd_equal(asd, asd_y)) > + list_for_each_entry(asd, ¬ifier->waiting, list) > + if (v4l2_async_match_equal(&asd->match, match)) > return true; > > list_for_each_entry(sd, ¬ifier->done, async_list) { > if (WARN_ON(!sd->asd)) > continue; > > - if (asd_equal(asd, sd->asd)) > + if (v4l2_async_match_equal(&sd->asd->match, match)) > return true; > } > > @@ -523,46 +524,48 @@ __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, > * 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, bool skip_self) > +v4l2_async_nf_has_async_match(struct v4l2_async_notifier *notifier, > + struct v4l2_async_match_desc *match, > + bool skip_self) > { > - struct v4l2_async_subdev *asd_y; > + struct v4l2_async_subdev *asd; > > 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 (skip_self && asd == asd_y) > + list_for_each_entry(asd, ¬ifier->asd_list, asd_list) { > + if (skip_self && &asd->match == match) > break; > - if (asd_equal(asd, asd_y)) > + if (v4l2_async_match_equal(&asd->match, match)) > return true; > } > > /* Check that an asd does not exist in other notifiers. */ > list_for_each_entry(notifier, ¬ifier_list, list) > - if (__v4l2_async_nf_has_async_subdev(notifier, asd)) > + if (v4l2_async_nf_has_async_match_entry(notifier, match)) > return true; > > return false; > } > > -static int v4l2_async_nf_asd_valid(struct v4l2_async_notifier *notifier, > - struct v4l2_async_subdev *asd, > - bool skip_self) > +static int v4l2_async_nf_match_valid(struct v4l2_async_notifier *notifier, > + struct v4l2_async_match_desc *match, > + bool skip_self) > { > struct device *dev = notifier_dev(notifier); > > - switch (asd->match.type) { > + switch (match->type) { > case V4L2_ASYNC_MATCH_TYPE_I2C: > case V4L2_ASYNC_MATCH_TYPE_FWNODE: > - if (v4l2_async_nf_has_async_subdev(notifier, asd, skip_self)) { > - dev_dbg(dev, "v4l2-async: subdev descriptor already listed in a notifier\n"); > + if (v4l2_async_nf_has_async_match(notifier, match, > + skip_self)) { > + dev_dbg(dev, "v4l2-async: match descriptor already listed in a notifier\n"); > return -EEXIST; > } > break; > default: > - dev_err(dev, "v4l2-asymc: Invalid match type %u on %p\n", > - asd->match.type, asd); > + dev_err(dev, "v4l2-async: Invalid match type %u on %p\n", > + match->type, match); > return -EINVAL; > } > > @@ -586,7 +589,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, true); > + ret = v4l2_async_nf_match_valid(notifier, &asd->match, true); > if (ret) > goto err_unlock; > > @@ -720,7 +723,7 @@ static int __v4l2_async_nf_add_subdev(struct v4l2_async_notifier *notifier, > > mutex_lock(&list_lock); > > - ret = v4l2_async_nf_asd_valid(notifier, asd, false); > + ret = v4l2_async_nf_match_valid(notifier, &asd->match, false); > if (ret) > goto unlock; > > @@ -898,16 +901,16 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd) > } > EXPORT_SYMBOL(v4l2_async_unregister_subdev); > > -static void print_waiting_subdev(struct seq_file *s, > - struct v4l2_async_subdev *asd) > +static void print_waiting_match(struct seq_file *s, > + struct v4l2_async_match_desc *match) > { > - switch (asd->match.type) { > + switch (match->type) { > case V4L2_ASYNC_MATCH_TYPE_I2C: > - seq_printf(s, " [i2c] dev=%d-%04x\n", asd->match.i2c.adapter_id, > - asd->match.i2c.address); > + seq_printf(s, " [i2c] dev=%d-%04x\n", match->i2c.adapter_id, > + match->i2c.address); > break; > case V4L2_ASYNC_MATCH_TYPE_FWNODE: { > - struct fwnode_handle *devnode, *fwnode = asd->match.fwnode; > + struct fwnode_handle *devnode, *fwnode = match->fwnode; > > devnode = fwnode_graph_is_endpoint(fwnode) ? > fwnode_graph_get_port_parent(fwnode) : > @@ -944,7 +947,7 @@ static int pending_subdevs_show(struct seq_file *s, void *data) > list_for_each_entry(notif, ¬ifier_list, list) { > seq_printf(s, "%s:\n", v4l2_async_nf_name(notif)); > list_for_each_entry(asd, ¬if->waiting, list) > - print_waiting_subdev(s, asd); > + print_waiting_match(s, &asd->match); > } > > mutex_unlock(&list_lock);
Hi Sakari, Thank you for the patch. On Thu, May 25, 2023 at 12:15:56PM +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. > > This will briefly break the adv748x driver but it will be fixed later in > the set, by patch "media: adv748x: Return to endpoint matching". I'm afraid I don't like this. This series is complex and has a high risk of causing tricky issues. I would like to be able to bisect the changes. > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/i2c/adv748x/adv748x-csi2.c | 5 +- > drivers/media/i2c/max9286.c | 14 +--- > drivers/media/i2c/rdacm20.c | 16 +--- > drivers/media/i2c/rdacm21.c | 15 +--- > drivers/media/i2c/tc358746.c | 5 -- > drivers/media/platform/nxp/imx-mipi-csis.c | 7 -- > drivers/media/v4l2-core/v4l2-async.c | 88 ++++++---------------- > 7 files changed, 26 insertions(+), 124 deletions(-) > > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c > index bd4f3fe0e3096..b6f93c1db3d2a 100644 > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c > @@ -296,13 +296,12 @@ int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx) > if (!is_tx_enabled(tx)) > return 0; > > + /* FIXME: Do endpoint matching again! */ > + > adv748x_subdev_init(&tx->sd, state, &adv748x_csi2_ops, > 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 13a986b885889..64f5c49cae776 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; > - > 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 a2263fa825b59..9d8a8f9efd835 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; > - > 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: > @@ -650,7 +637,6 @@ static void rdacm20_remove(struct i2c_client *client) > { > struct rdacm20_device *dev = i2c_to_rdacm20(client); > > - fwnode_handle_put(dev->sd.fwnode); > v4l2_async_unregister_subdev(&dev->sd); > v4l2_ctrl_handler_free(&dev->ctrls); > media_entity_cleanup(&dev->sd.entity); > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c > index 9ccc56c30d3b0..d67cfcb2e05a4 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 ec1a193ba161a..b37a9ff73f6ad 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; > @@ -1486,7 +1483,6 @@ static int tc358746_async_register(struct tc358746 *tc358746) > return 0; > > err_unregister: > - fwnode_handle_put(tc358746->sd.fwnode); > v4l2_async_nf_unregister(&tc358746->notifier); > err_cleanup: > v4l2_async_nf_cleanup(&tc358746->notifier); > @@ -1605,7 +1601,6 @@ static void tc358746_remove(struct i2c_client *client) > v4l2_fwnode_endpoint_free(&tc358746->csi_vep); > v4l2_async_nf_unregister(&tc358746->notifier); > v4l2_async_nf_cleanup(&tc358746->notifier); > - fwnode_handle_put(sd->fwnode); > v4l2_async_unregister_subdev(sd); > media_entity_cleanup(&sd->entity); > > diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c > index 05d52762e7926..d0dc30187775d 100644 > --- a/drivers/media/platform/nxp/imx-mipi-csis.c > +++ b/drivers/media/platform/nxp/imx-mipi-csis.c > @@ -1365,13 +1365,6 @@ static int mipi_csis_subdev_init(struct mipi_csis_device *csis) > > sd->dev = csis->dev; > > - sd->fwnode = fwnode_graph_get_endpoint_by_id(dev_fwnode(csis->dev), > - 1, 0, 0); > - if (!sd->fwnode) { > - dev_err(csis->dev, "Unable to retrieve endpoint for port@1\n"); > - return -ENOENT; > - } > - > csis->pads[CSIS_PAD_SINK].flags = MEDIA_PAD_FL_SINK > | MEDIA_PAD_FL_MUST_CONNECT; > csis->pads[CSIS_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index 06b1e1a1a5f87..335597889f365 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -94,89 +94,36 @@ match_fwnode_one(struct v4l2_async_notifier *notifier, > struct v4l2_subdev *sd, struct fwnode_handle *sd_fwnode, > struct v4l2_async_match_desc *match) > { > - 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(notifier_dev(notifier), > "v4l2-async: fwnode match: need %pfw, trying %pfw\n", > sd_fwnode, 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 == match->fwnode) { > dev_dbg(notifier_dev(notifier), > "v4l2-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(match->fwnode); > - > - if (sd_fwnode_is_ep == asd_fwnode_is_ep) { > + if (!fwnode_graph_is_endpoint(match->fwnode)) { > dev_dbg(notifier_dev(notifier), > "v4l2-async: direct match not found\n"); > 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 = match->fwnode; > - } else { > - dev_fwnode = fwnode_graph_get_port_parent(match->fwnode); > - other_fwnode = sd_fwnode; > - } > + asd_dev_fwnode = fwnode_graph_get_port_parent(match->fwnode); > > - dev_dbg(notifier_dev(notifier), > - "v4l2-async: fwnode compat match: need %pfw, trying %pfw\n", > - dev_fwnode, other_fwnode); > + ret = sd_fwnode == asd_dev_fwnode; > > - fwnode_handle_put(dev_fwnode); > + fwnode_handle_put(asd_dev_fwnode); > > - if (dev_fwnode != other_fwnode) { > - dev_dbg(notifier_dev(notifier), > - "v4l2-async: compat match not found\n"); > - return false; > - } > - > - /* > - * 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(notifier_dev(notifier), "v4l2-async: compat match found\n"); > + dev_dbg(notifier_dev(notifier), > + "v4l2-async: device--endpoint match %sfound\n", > + ret ? "" : "not "); > > - return true; > + return ret; > } > > static bool match_fwnode(struct v4l2_async_notifier *notifier, > @@ -814,12 +761,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. > */ > - if (!sd->fwnode && sd->dev) > + if (!sd->fwnode && sd->dev) { > sd->fwnode = dev_fwnode(sd->dev); > + } else if (fwnode_graph_is_endpoint(sd->fwnode)) { > + dev_warn(sd->dev, "sub-device fwnode is an endpoint!\n"); > + return -EINVAL; > + } > > mutex_lock(&list_lock); >
Hi Sakari, Thank you for the patch. On Thu, May 25, 2023 at 12:15:57PM +0300, Sakari Ailus wrote: > Rename v4l2_async_subdev as v4l2_async_connection, in order to > differentiate between the sub-devices and their connections: one > sub-device can have many connections but the V4L2 async framework has so > far allowed just a single one. Connections in this context will later > translate into either MC ancillary or data links. > > This patch prepares changing that relation by changing existing users of > v4l2_async_subdev to switch to v4l2_async_connection. Async sub-devices > themselves will not be needed anymore > > Additionally, __v4l2_async_nf_add_subdev() has been renamed as s/renamed as/renamed to/ > __v4l2_async_nf_add_connection(). I still don't like the name "connection" at all :-( It may seem fine as you've been working on this extensively, but for people less familiar with v4l2-async (myself included), I fear it will make the framework more difficult to understand. At the very least I'd like a detailed and clear glossary, to explain what a connection *is*. The v4l2-async documentation is fairly bad (and what is currently documented in v4l2-subdev.rst should likely be moved to v4l2-async.rst). > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > .../driver-api/media/v4l2-subdev.rst | 12 +- > drivers/media/i2c/max9286.c | 9 +- > drivers/media/i2c/st-mipid02.c | 8 +- > drivers/media/i2c/tc358746.c | 6 +- > drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 10 +- > drivers/media/platform/atmel/atmel-isi.c | 8 +- > drivers/media/platform/atmel/atmel-isi.h | 2 +- > drivers/media/platform/cadence/cdns-csi2rx.c | 6 +- > drivers/media/platform/intel/pxa_camera.c | 12 +- > drivers/media/platform/marvell/cafe-driver.c | 5 +- > drivers/media/platform/marvell/mcam-core.c | 4 +- > drivers/media/platform/marvell/mmp-driver.c | 4 +- > .../platform/microchip/microchip-csi2dc.c | 6 +- > .../platform/microchip/microchip-isc-base.c | 4 +- > .../media/platform/microchip/microchip-isc.h | 2 +- > .../microchip/microchip-sama5d2-isc.c | 4 +- > .../microchip/microchip-sama7g5-isc.c | 4 +- > drivers/media/platform/nxp/imx-mipi-csis.c | 6 +- > drivers/media/platform/nxp/imx7-media-csi.c | 6 +- > .../platform/nxp/imx8-isi/imx8-isi-core.c | 8 +- > drivers/media/platform/qcom/camss/camss.c | 2 +- > drivers/media/platform/qcom/camss/camss.h | 2 +- > drivers/media/platform/renesas/rcar-isp.c | 8 +- > .../platform/renesas/rcar-vin/rcar-core.c | 44 ++--- > .../platform/renesas/rcar-vin/rcar-csi2.c | 16 +- > .../platform/renesas/rcar-vin/rcar-vin.h | 8 +- > drivers/media/platform/renesas/rcar_drif.c | 8 +- > drivers/media/platform/renesas/renesas-ceu.c | 6 +- > .../platform/renesas/rzg2l-cru/rzg2l-core.c | 10 +- > .../platform/renesas/rzg2l-cru/rzg2l-cru.h | 2 +- > .../platform/renesas/rzg2l-cru/rzg2l-csi2.c | 8 +- > .../platform/rockchip/rkisp1/rkisp1-common.h | 2 +- > .../platform/rockchip/rkisp1/rkisp1-dev.c | 8 +- > .../platform/samsung/exynos4-is/media-dev.c | 6 +- > .../platform/samsung/exynos4-is/media-dev.h | 2 +- > drivers/media/platform/st/stm32/stm32-dcmi.c | 8 +- > .../platform/sunxi/sun4i-csi/sun4i_csi.c | 6 +- > .../sunxi/sun6i-csi/sun6i_csi_bridge.c | 2 +- > .../sunxi/sun6i-csi/sun6i_csi_bridge.h | 2 +- > .../sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c | 6 +- > .../sun8i_a83t_mipi_csi2.c | 6 +- > .../media/platform/ti/am437x/am437x-vpfe.c | 5 +- > .../media/platform/ti/am437x/am437x-vpfe.h | 2 +- > drivers/media/platform/ti/cal/cal.c | 6 +- > .../media/platform/ti/davinci/vpif_capture.c | 7 +- > drivers/media/platform/ti/omap3isp/isp.h | 2 +- > drivers/media/platform/video-mux.c | 6 +- > drivers/media/platform/xilinx/xilinx-vipp.c | 22 +-- > drivers/media/v4l2-core/v4l2-async.c | 159 +++++++++--------- > drivers/media/v4l2-core/v4l2-fwnode.c | 8 +- > .../media/deprecated/atmel/atmel-isc-base.c | 4 +- > .../media/deprecated/atmel/atmel-isc.h | 2 +- > .../deprecated/atmel/atmel-sama5d2-isc.c | 4 +- > .../deprecated/atmel/atmel-sama7g5-isc.c | 4 +- > drivers/staging/media/imx/imx-media-csi.c | 6 +- > .../staging/media/imx/imx-media-dev-common.c | 2 +- > 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 | 8 +- > drivers/staging/media/imx/imx8mq-mipi-csi2.c | 6 +- > .../media/sunxi/sun6i-isp/sun6i_isp_proc.c | 2 +- > .../media/sunxi/sun6i-isp/sun6i_isp_proc.h | 2 +- > drivers/staging/media/tegra-video/vi.c | 14 +- > drivers/staging/media/tegra-video/vi.h | 2 +- > include/media/davinci/vpif_types.h | 2 +- > include/media/v4l2-async.h | 66 ++++---- > include/media/v4l2-fwnode.h | 2 +- > include/media/v4l2-subdev.h | 5 +- > 68 files changed, 320 insertions(+), 322 deletions(-) > > diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst > index ce8e9d0a332bc..83d3d29608136 100644 > --- a/Documentation/driver-api/media/v4l2-subdev.rst > +++ b/Documentation/driver-api/media/v4l2-subdev.rst > @@ -214,14 +214,14 @@ notifier and further registers async sub-devices for lens and flash devices > found in firmware. The notifier for the sub-device is unregistered with the > async sub-device. > > -These functions allocate an async sub-device descriptor which is of type struct > -:c:type:`v4l2_async_subdev` embedded in a driver-specific struct. The &struct > -:c:type:`v4l2_async_subdev` shall be the first member of this struct: > +These functions allocate an async connection descriptor which is of type struct > +:c:type:`v4l2_async_connection` embedded in a driver-specific struct. The &struct > +:c:type:`v4l2_async_connection` shall be the first member of this struct: > > .. code-block:: c > > struct my_async_subdev { > - struct v4l2_async_subdev asd; > + struct v4l2_async_connection asd; s/asd/asc/ > ... > }; > > @@ -244,10 +244,10 @@ notifier callback is called. After all subdevices have been located the > system the .unbind() method is called. All three callbacks are optional. > > Drivers can store any type of custom data in their driver-specific > -:c:type:`v4l2_async_subdev` wrapper. If any of that data requires special > +:c:type:`v4l2_async_connection` wrapper. If any of that data requires special > handling when the structure is freed, drivers must implement the ``.destroy()`` > notifier callback. The framework will call it right before freeing the > -:c:type:`v4l2_async_subdev`. > +:c:type:`v4l2_async_connection`. > > Calling subdev operations > ~~~~~~~~~~~~~~~~~~~~~~~~~ > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > index 64f5c49cae776..44def5e3ba5d1 100644 > --- a/drivers/media/i2c/max9286.c > +++ b/drivers/media/i2c/max9286.c > @@ -161,11 +161,12 @@ struct max9286_source { > }; > > struct max9286_asd { This should be renamed to max9286_asc. I said in the review of a previous version that I was fine keeping the name as-is, as the v4l2_async_subdev structure was reintroduced later in the series, but that's not the case anymore. > - struct v4l2_async_subdev base; > + struct v4l2_async_connection base; > struct max9286_source *source; > }; > > -static inline struct max9286_asd *to_max9286_asd(struct v4l2_async_subdev *asd) > +static inline struct max9286_asd * > +to_max9286_asd(struct v4l2_async_connection *asd) s/asd/asc/g There's more below, in most drivers. > { > return container_of(asd, struct max9286_asd, base); > } > @@ -659,7 +660,7 @@ static int max9286_set_pixelrate(struct max9286_priv *priv) > > static int max9286_notify_bound(struct v4l2_async_notifier *notifier, > struct v4l2_subdev *subdev, > - struct v4l2_async_subdev *asd) > + struct v4l2_async_connection *asd) > { > struct max9286_priv *priv = sd_to_max9286(notifier->sd); > struct max9286_source *source = to_max9286_asd(asd)->source; > @@ -721,7 +722,7 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier, > > static void max9286_notify_unbind(struct v4l2_async_notifier *notifier, > struct v4l2_subdev *subdev, > - struct v4l2_async_subdev *asd) > + struct v4l2_async_connection *asd) > { > struct max9286_priv *priv = sd_to_max9286(notifier->sd); > struct max9286_source *source = to_max9286_asd(asd)->source; [snip] > diff --git a/drivers/media/platform/atmel/atmel-isi.h b/drivers/media/platform/atmel/atmel-isi.h > index 7ad3895a2c87e..58ce900ca4c90 100644 > --- a/drivers/media/platform/atmel/atmel-isi.h > +++ b/drivers/media/platform/atmel/atmel-isi.h > @@ -121,7 +121,7 @@ > #define ISI_DATAWIDTH_8 0x01 > #define ISI_DATAWIDTH_10 0x02 > > -struct v4l2_async_subdev; > +struct v4l2_async_connection; You can actually drop this, it's not used in this file. > > struct isi_platform_data { > u8 has_emb_sync; [snip] > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h > index cb206d3976ddf..c99d64e1cb01f 100644 > --- a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h > @@ -106,7 +106,7 @@ struct rvin_video_format { > > /** > * struct rvin_parallel_entity - Parallel video input endpoint descriptor > - * @asd: sub-device descriptor for async framework > + * @asc: sub-device descriptor for async framework The description of the field should also be updated. > * @subdev: subdevice matched using async framework > * @mbus_type: media bus type > * @bus: media bus parallel configuration > @@ -115,7 +115,7 @@ struct rvin_video_format { > * > */ > struct rvin_parallel_entity { > - struct v4l2_async_subdev *asd; > + struct v4l2_async_connection *asc; > struct v4l2_subdev *subdev; > > enum v4l2_mbus_type mbus_type; > @@ -275,7 +275,7 @@ struct rvin_dev { > * @notifier: group notifier for CSI-2 async subdevices > * @vin: VIN instances which are part of the group > * @link_setup: Callback to create all links for the media graph > - * @remotes: array of pairs of fwnode and subdev pointers > + * @remotes: array of pairs of async connection and subdev pointers > * to all remote subdevices. > */ > struct rvin_group { > @@ -291,7 +291,7 @@ struct rvin_group { > int (*link_setup)(struct rvin_dev *vin); > > struct { > - struct v4l2_async_subdev *asd; > + struct v4l2_async_connection *asc; > struct v4l2_subdev *subdev; > } remotes[RVIN_REMOTES_MAX]; > }; [snip] > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h > index 54f9f45ed3d8e..38d9d097fdb52 100644 > --- a/include/media/v4l2-async.h > +++ b/include/media/v4l2-async.h > @@ -34,7 +34,7 @@ enum v4l2_async_match_type { > }; > > /** > - * struct v4l2_async_match_desc - async sub-device match information > + * struct v4l2_async_match_desc - async connection match information > * > * @type: type of match that will be used > * @fwnode: pointer to &struct fwnode_handle to be matched. > @@ -62,21 +62,21 @@ struct v4l2_async_match_desc { > }; > > /** > - * struct v4l2_async_subdev - sub-device descriptor, as known to a bridge > + * struct v4l2_async_connection - connection descriptor, as known to a bridge > * > * @match: struct of match type and per-bus type matching data sets > - * @asd_entry: used to add struct v4l2_async_subdev objects to the > - * master notifier @asd_entry > - * @waiting_entry: used to link struct v4l2_async_subdev objects, waiting to be > - * probed, to a notifier->waiting_list list > + * @asc_entry: used to add struct v4l2_async_connection objects to the > + * master notifier @asc_list > + * @waiting_entry: used to link struct v4l2_async_connection objects, waiting to > + * be probed, to a notifier->waiting_list list > * > * When this struct is used as a member in a driver specific struct, > * the driver specific struct shall contain the &struct > - * v4l2_async_subdev as its first member. > + * v4l2_async_connection as its first member. > */ > -struct v4l2_async_subdev { > +struct v4l2_async_connection { > struct v4l2_async_match_desc match; > - struct list_head asd_entry; > + struct list_head asc_entry; > struct list_head waiting_entry; > }; > > @@ -86,17 +86,17 @@ struct v4l2_async_subdev { > * @complete: All subdevices have been probed successfully. The complete > * callback is only executed for the root notifier. > * @unbind: a subdevice is leaving > - * @destroy: the asd is about to be freed > + * @destroy: the asc is about to be freed > */ > struct v4l2_async_notifier_operations { > int (*bound)(struct v4l2_async_notifier *notifier, > struct v4l2_subdev *subdev, > - struct v4l2_async_subdev *asd); > + struct v4l2_async_connection *asc); > int (*complete)(struct v4l2_async_notifier *notifier); > void (*unbind)(struct v4l2_async_notifier *notifier, > struct v4l2_subdev *subdev, > - struct v4l2_async_subdev *asd); > - void (*destroy)(struct v4l2_async_subdev *asd); > + struct v4l2_async_connection *asc); > + void (*destroy)(struct v4l2_async_connection *asc); > }; > > /** > @@ -106,7 +106,7 @@ 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 > + * @asc_list: master list of struct v4l2_async_subdev v4l2_async_subdev is no more. > * @waiting_list: list of struct v4l2_async_subdev, waiting for their drivers Same here. > * @done_list: list of struct v4l2_subdev, already probed > * @notifier_entry: member in a global list of notifiers > @@ -116,7 +116,7 @@ struct v4l2_async_notifier { > struct v4l2_device *v4l2_dev; > struct v4l2_subdev *sd; > struct v4l2_async_notifier *parent; > - struct list_head asd_list; > + struct list_head asc_list; > struct list_head waiting_list; > struct list_head done_list; > struct list_head notifier_entry; > @@ -134,53 +134,53 @@ void v4l2_async_debug_init(struct dentry *debugfs_dir); > * > * @notifier: pointer to &struct v4l2_async_notifier > * > - * This function initializes the notifier @asd_entry. It must be called > + * This function initializes the notifier @asc_entry. It must be called This sounds like it should be asc_list. The issues was likely introduced in an earlier patch in the series. > * before adding a subdevice to a notifier, using one of: > * v4l2_async_nf_add_fwnode_remote(), v4l2_async_nf_add_fwnode() or > * v4l2_async_nf_add_i2c(). > */ > void v4l2_async_nf_init(struct v4l2_async_notifier *notifier); > > -struct v4l2_async_subdev * > +struct v4l2_async_connection * > __v4l2_async_nf_add_fwnode(struct v4l2_async_notifier *notifier, > struct fwnode_handle *fwnode, > - unsigned int asd_struct_size); > + unsigned int asc_struct_size); > /** > * v4l2_async_nf_add_fwnode - Allocate and add a fwnode async > - * subdev to the notifier's master asd_entry. > + * subdev to the notifier's master asc_entry. Same here. > * > * @notifier: pointer to &struct v4l2_async_notifier > * @fwnode: fwnode handle of the sub-device to be matched, pointer to > * &struct fwnode_handle > - * @type: Type of the driver's async sub-device struct. The &struct > - * v4l2_async_subdev shall be the first member of the driver's async > - * sub-device struct, i.e. both begin at the same memory address. > + * @type: Type of the driver's async sub-device or connection struct. The > + * &struct v4l2_async_connection shall be the first member of the > + * driver's async struct, i.e. both begin at the same memory address. > * > - * Allocate a fwnode-matched asd of size asd_struct_size, and add it to the > - * notifiers @asd_entry. The function also gets a reference of the fwnode which > + * Allocate a fwnode-matched asc of size asc_struct_size, and add it to the > + * notifiers @asc_entry. The function also gets a reference of the fwnode which Here too. > * is released later at notifier cleanup time. > */ > #define v4l2_async_nf_add_fwnode(notifier, fwnode, type) \ > ((type *)__v4l2_async_nf_add_fwnode(notifier, fwnode, sizeof(type))) > > -struct v4l2_async_subdev * > +struct v4l2_async_connection * > __v4l2_async_nf_add_fwnode_remote(struct v4l2_async_notifier *notif, > struct fwnode_handle *endpoint, > - unsigned int asd_struct_size); > + unsigned int asc_struct_size); > /** > * v4l2_async_nf_add_fwnode_remote - Allocate and add a fwnode > * remote async subdev to the > - * notifier's master asd_entry. > + * notifier's master asc_entry. And here. > * > * @notifier: pointer to &struct v4l2_async_notifier > * @ep: local endpoint pointing to the remote sub-device to be matched, > * pointer to &struct fwnode_handle > * @type: Type of the driver's async sub-device struct. The &struct > - * v4l2_async_subdev shall be the first member of the driver's async > + * 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. > * > * Gets the remote endpoint of a given local endpoint, set it up for fwnode > - * matching and adds the async sub-device to the notifier's @asd_entry. The > + * matching and adds the async connection to the notifier's @asc_entry. The Here too again. > * function also gets a reference of the fwnode which is released later at > * notifier cleanup time. > * > @@ -190,19 +190,19 @@ __v4l2_async_nf_add_fwnode_remote(struct v4l2_async_notifier *notif, > #define v4l2_async_nf_add_fwnode_remote(notifier, ep, type) \ > ((type *)__v4l2_async_nf_add_fwnode_remote(notifier, ep, sizeof(type))) > > -struct v4l2_async_subdev * > +struct v4l2_async_connection * > __v4l2_async_nf_add_i2c(struct v4l2_async_notifier *notifier, > int adapter_id, unsigned short address, > - unsigned int asd_struct_size); > + unsigned int asc_struct_size); > /** > * v4l2_async_nf_add_i2c - Allocate and add an i2c async > - * subdev to the notifier's master asd_entry. > + * subdev to the notifier's master asc_entry. And finally here. > * > * @notifier: pointer to &struct v4l2_async_notifier > * @adapter: I2C adapter ID to be matched > * @address: I2C address of sub-device to be matched > * @type: Type of the driver's async sub-device struct. The &struct > - * v4l2_async_subdev shall be the first member of the driver's async > + * 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. s/sub-device/connection/ ? Please double-check if other instances of subdev(ice) have been forgotten. > * > * Same as v4l2_async_nf_add_fwnode() but for I2C matched > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h > index 855dae84b751d..4e4a6cf83097a 100644 > --- a/include/media/v4l2-fwnode.h > +++ b/include/media/v4l2-fwnode.h > @@ -23,7 +23,7 @@ > > struct fwnode_handle; > struct v4l2_async_notifier; > -struct v4l2_async_subdev; > +struct v4l2_async_connection; This is a sign the forward-declaration isn't needed. > > /** > * struct v4l2_fwnode_endpoint - the endpoint data structure > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index 82e4cf3dd2e05..215fc8af87614 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -1022,8 +1022,7 @@ struct v4l2_subdev_platform_data { > * either dev->of_node->fwnode or dev->fwnode (whichever is non-NULL). > * @async_list: Links this subdev to a global subdev_entry or @notifier->done > * list. > - * @asd: Pointer to respective &struct v4l2_async_subdev. > - * @notifier: Pointer to the managing notifier. Did you drop this field by mistake ? > + * @asd: Pointer to respective &struct v4l2_async_connection. > * @subdev_notifier: A sub-device notifier implicitly registered for the sub- > * device using v4l2_async_register_subdev_sensor(). > * @pdata: common part of subdevice platform data > @@ -1065,7 +1064,7 @@ struct v4l2_subdev { > struct device *dev; > struct fwnode_handle *fwnode; > struct list_head async_list; > - struct v4l2_async_subdev *asd; > + struct v4l2_async_connection *asd; > struct v4l2_async_notifier *notifier; > struct v4l2_async_notifier *subdev_notifier; > struct v4l2_subdev_platform_data *pdata;
Hi Sakari, On Thu, May 25, 2023 at 12:15:59PM +0300, Sakari Ailus wrote: > The connections are checked for duplicates already when the notifier is > registered. This is effectively a sanity check for driver (and possibly > obscure firmware) bugs. Don't do this when adding the connection. Isn't it better to have this sanity check when the connection is added, instead of later when the notifier is registered ? The latter is more difficult to debug. If you want to avoid duplicate checks, could we drop the one at notifier registration time ? > Retain the int return type for now. It'll be needed very soon again. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/v4l2-core/v4l2-async.c | 23 +++++++---------------- > 1 file changed, 7 insertions(+), 16 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index f51f0c37210c9..5dfc6d5f6a7c3 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -475,8 +475,7 @@ v4l2_async_nf_has_async_match_entry(struct v4l2_async_notifier *notifier, > */ > static bool > v4l2_async_nf_has_async_match(struct v4l2_async_notifier *notifier, > - struct v4l2_async_match_desc *match, > - bool skip_self) > + struct v4l2_async_match_desc *match) > { > struct v4l2_async_connection *asc; > > @@ -484,7 +483,7 @@ v4l2_async_nf_has_async_match(struct v4l2_async_notifier *notifier, > > /* Check that an asd is not being added more than once. */ > list_for_each_entry(asc, ¬ifier->asc_list, asc_entry) { > - if (skip_self && &asc->match == match) > + if (&asc->match == match) > break; > if (v4l2_async_match_equal(&asc->match, match)) > return true; > @@ -499,16 +498,14 @@ v4l2_async_nf_has_async_match(struct v4l2_async_notifier *notifier, > } > > static int v4l2_async_nf_match_valid(struct v4l2_async_notifier *notifier, > - struct v4l2_async_match_desc *match, > - bool skip_self) > + struct v4l2_async_match_desc *match) > { > struct device *dev = notifier_dev(notifier); > > switch (match->type) { > case V4L2_ASYNC_MATCH_TYPE_I2C: > case V4L2_ASYNC_MATCH_TYPE_FWNODE: > - if (v4l2_async_nf_has_async_match(notifier, match, > - skip_self)) { > + if (v4l2_async_nf_has_async_match(notifier, match)) { > dev_dbg(dev, "v4l2-async: match descriptor already listed in a notifier\n"); > return -EEXIST; > } > @@ -539,7 +536,7 @@ static int __v4l2_async_nf_register(struct v4l2_async_notifier *notifier) > mutex_lock(&list_lock); > > list_for_each_entry(asc, ¬ifier->asc_list, asc_entry) { > - ret = v4l2_async_nf_match_valid(notifier, &asc->match, true); > + ret = v4l2_async_nf_match_valid(notifier, &asc->match); > if (ret) > goto err_unlock; > > @@ -668,19 +665,13 @@ EXPORT_SYMBOL_GPL(v4l2_async_nf_cleanup); > static int __v4l2_async_nf_add_connection(struct v4l2_async_notifier *notifier, > struct v4l2_async_connection *asc) > { > - int ret; > - > mutex_lock(&list_lock); > > - ret = v4l2_async_nf_match_valid(notifier, &asc->match, false); > - if (ret) > - goto unlock; > - > list_add_tail(&asc->asc_entry, ¬ifier->asc_list); > > -unlock: > mutex_unlock(&list_lock); > - return ret; > + > + return 0; > } > > struct v4l2_async_connection *
Hi Sakari, Thank you for the patch. On Thu, May 25, 2023 at 12:16:15PM +0300, Sakari Ailus wrote: > Document that sub-device notifiers are now registered using > v4l2_async_subdev_nf_init(). No documentation is changed as it seems that > sub-device notifiers were not documented apart from kernel-doc comments. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > Documentation/driver-api/media/v4l2-subdev.rst | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst > index 83d3d29608136..d62b341642c96 100644 > --- a/Documentation/driver-api/media/v4l2-subdev.rst > +++ b/Documentation/driver-api/media/v4l2-subdev.rst > @@ -193,9 +193,7 @@ picked up by bridge drivers. > Bridge drivers in turn have to register a notifier object. This is > performed using the :c:func:`v4l2_async_nf_register` call. To > unregister the notifier the driver has to call > -:c:func:`v4l2_async_nf_unregister`. The former of the two functions > -takes two arguments: a pointer to struct :c:type:`v4l2_device` and a > -pointer to struct :c:type:`v4l2_async_notifier`. > +:c:func:`v4l2_async_nf_unregister`. > > Before registering the notifier, bridge drivers must do two things: first, the > notifier must be initialized using the :c:func:`v4l2_async_nf_init`. > @@ -204,6 +202,12 @@ that the bridge device needs for its operation. Several functions are available > to add subdevice descriptors to a notifier, depending on the type of device and > the needs of the driver. > > +For a sub-device driver to register a notifier, the process is otherwise similar > +to that of a bridge driver, apart from that the notifier is initialised using > +:c:func:`v4l2_async_subdev_nf_init` instead. A sub-device notifier may complete > +only after the V4L2 device becomes available, i.e. there's a path via async > +sub-devices and notifiers to that root notifier. This is correct, but I doubt anyone who doesn't have an in-depth knowledge of the v4l2-async framework will be able to understand it. For instance, the concept of "root notifier" isn't explained anywhere. And the v4l2_async_subdev_nf_register() function isn't mentioned. The v4l2-async documentation needs a rewrite. > + > :c:func:`v4l2_async_nf_add_fwnode`, :c:func:`v4l2_async_nf_add_fwnode_remote` > :c:and func:`v4l2_async_nf_add_i2c` are for registering their async sub-devices > :c:with the notifier.
On 25.05.23 11:16, Sakari Ailus wrote: > Document that sub-device notifiers are now registered using > v4l2_async_subdev_nf_init(). No documentation is changed as it seems that > sub-device notifiers were not documented apart from kernel-doc comments. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > Documentation/driver-api/media/v4l2-subdev.rst | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst > index 83d3d29608136..d62b341642c96 100644 > --- a/Documentation/driver-api/media/v4l2-subdev.rst > +++ b/Documentation/driver-api/media/v4l2-subdev.rst > @@ -193,9 +193,7 @@ picked up by bridge drivers. > Bridge drivers in turn have to register a notifier object. This is > performed using the :c:func:`v4l2_async_nf_register` call. To > unregister the notifier the driver has to call > -:c:func:`v4l2_async_nf_unregister`. The former of the two functions > -takes two arguments: a pointer to struct :c:type:`v4l2_device` and a > -pointer to struct :c:type:`v4l2_async_notifier`. > +:c:func:`v4l2_async_nf_unregister`. > > Before registering the notifier, bridge drivers must do two things: first, the > notifier must be initialized using the :c:func:`v4l2_async_nf_init`. > @@ -204,6 +202,12 @@ that the bridge device needs for its operation. Several functions are available > to add subdevice descriptors to a notifier, depending on the type of device and > the needs of the driver. > > +For a sub-device driver to register a notifier, the process is otherwise similar > +to that of a bridge driver, apart from that the notifier is initialised using > +:c:func:`v4l2_async_subdev_nf_init` instead. A sub-device notifier may complete > +only after the V4L2 device becomes available, i.e. there's a path via async > +sub-devices and notifiers to that root notifier. > + > :c:func:`v4l2_async_nf_add_fwnode`, :c:func:`v4l2_async_nf_add_fwnode_remote` > :c:and func:`v4l2_async_nf_add_i2c` are for registering their async sub-devices > :c:with the notifier. Works on Apalis i.MX6Q with TC358743. Tested-by: Aishwarya Kothari <aishwarya.kothari@toradex.com> regards, Aishwarya
Hi Laurent, Thanks for the review. On Tue, May 30, 2023 at 05:23:23AM +0300, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Thu, May 25, 2023 at 12:15:47PM +0300, Sakari Ailus wrote: > > There's no need to check for a sub-device's notifier as we only register > > one notifier (and one V4L2 device). Remove this check and prepare for > > removing this field in struct v4l2_subdev. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/platform/ti/omap3isp/isp.c | 3 --- > > 1 file changed, 3 deletions(-) > > > > diff --git a/drivers/media/platform/ti/omap3isp/isp.c b/drivers/media/platform/ti/omap3isp/isp.c > > index f3aaa9e76492e..c2b222f7df892 100644 > > --- a/drivers/media/platform/ti/omap3isp/isp.c > > +++ b/drivers/media/platform/ti/omap3isp/isp.c > > @@ -2039,9 +2039,6 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async) > > } > > > > list_for_each_entry(sd, &v4l2_dev->subdevs, list) { > > - if (sd->notifier != &isp->notifier) > > - continue; > > I don't think this is quite right. You could have a chain of external > subdevs, in which case only the one connected directly to the ISP should > be linked to the ISP. You're right, in principle this could take place. Going forward sub-devices may be bound to multiple notifiers so this field should go. I'll see if we could have another test for this.
Hi Laurent, On Tue, May 30, 2023 at 05:52:22AM +0300, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Thu, May 25, 2023 at 12:15:52PM +0300, Sakari Ailus wrote: > > Make V4L2 async match information a struct, making it easier to use it > > elsewhere outside the scope of struct v4l2_async_subdev. > > > > Also remove an obsolete comment --- none of these fields are supposed to > > be touched by drivers. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/v4l2-core/v4l2-async.c | 20 +++++++------- > > include/media/v4l2-async.h | 41 ++++++++++++++++------------ > > 2 files changed, 33 insertions(+), 28 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > index 7c924faac4c10..7f56648e40c44 100644 > > --- a/drivers/media/v4l2-core/v4l2-async.c > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > @@ -212,7 +212,7 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier, > > > > list_for_each_entry(asd, ¬ifier->waiting, list) { > > /* bus_type has been verified valid before */ > > - switch (asd->match_type) { > > + switch (asd->match.type) { > > case V4L2_ASYNC_MATCH_I2C: > > match = match_i2c; > > break; > > @@ -237,10 +237,10 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier, > > static bool asd_equal(struct v4l2_async_subdev *asd_x, > > struct v4l2_async_subdev *asd_y) > > { > > - if (asd_x->match_type != asd_y->match_type) > > + if (asd_x->match.type != asd_y->match.type) > > return false; > > > > - switch (asd_x->match_type) { > > + switch (asd_x->match.type) { > > case V4L2_ASYNC_MATCH_I2C: > > return asd_x->match.i2c.adapter_id == > > asd_y->match.i2c.adapter_id && > > @@ -552,7 +552,7 @@ static int v4l2_async_nf_asd_valid(struct v4l2_async_notifier *notifier, > > { > > struct device *dev = notifier_dev(notifier); > > > > - switch (asd->match_type) { > > + switch (asd->match.type) { > > case V4L2_ASYNC_MATCH_I2C: > > case V4L2_ASYNC_MATCH_FWNODE: > > if (v4l2_async_nf_has_async_subdev(notifier, asd, skip_self)) { > > @@ -561,8 +561,8 @@ static int v4l2_async_nf_asd_valid(struct v4l2_async_notifier *notifier, > > } > > break; > > default: > > - dev_err(dev, "v4l2-async: Invalid match type %u on %p\n", > > - asd->match_type, asd); > > + dev_err(dev, "v4l2-asymc: Invalid match type %u on %p\n", > > Is this for asymmetrical notification ? Oops. > > With this fixed, > > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> Thank you! :-)
Hi Laurent, On Tue, May 30, 2023 at 06:02:15AM +0300, Laurent Pinchart wrote: > > @@ -234,20 +237,18 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier, > > } > > > > /* Compare two async sub-device descriptors for equivalence */ > > s/sub-device/match/ ? Yes. > > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> Thanks!
Hi Laurent, On Tue, May 30, 2023 at 08:08:48AM +0300, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Thu, May 25, 2023 at 12:15:56PM +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. > > > > This will briefly break the adv748x driver but it will be fixed later in > > the set, by patch "media: adv748x: Return to endpoint matching". > > I'm afraid I don't like this. This series is complex and has a high risk > of causing tricky issues. I would like to be able to bisect the changes. As we discussed separately, this has been tested on both rcar-vin + adv748x and i.MX6, I'm not overly concerned of this.
Hi Laurent, Thank you for the review, again. On Tue, May 30, 2023 at 08:50:03AM +0300, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Thu, May 25, 2023 at 12:15:57PM +0300, Sakari Ailus wrote: > > Rename v4l2_async_subdev as v4l2_async_connection, in order to > > differentiate between the sub-devices and their connections: one > > sub-device can have many connections but the V4L2 async framework has so > > far allowed just a single one. Connections in this context will later > > translate into either MC ancillary or data links. > > > > This patch prepares changing that relation by changing existing users of > > v4l2_async_subdev to switch to v4l2_async_connection. Async sub-devices > > themselves will not be needed anymore > > > > Additionally, __v4l2_async_nf_add_subdev() has been renamed as > > s/renamed as/renamed to/ > > > __v4l2_async_nf_add_connection(). Actually I think there should be no preposition at all here. > > I still don't like the name "connection" at all :-( It may seem fine as > you've been working on this extensively, but for people less familiar > with v4l2-async (myself included), I fear it will make the framework > more difficult to understand. > > At the very least I'd like a detailed and clear glossary, to explain > what a connection *is*. The v4l2-async documentation is fairly bad (and > what is currently documented in v4l2-subdev.rst should likely be moved > to v4l2-async.rst). That's a fair point. I'll add documentation on this. What I do like with this is that there's now a clear separation between the placeholder registered with the notifier and the sub-device itself. That used to be sometimes a bit confusing. > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > .../driver-api/media/v4l2-subdev.rst | 12 +- > > drivers/media/i2c/max9286.c | 9 +- > > drivers/media/i2c/st-mipid02.c | 8 +- > > drivers/media/i2c/tc358746.c | 6 +- > > drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 10 +- > > drivers/media/platform/atmel/atmel-isi.c | 8 +- > > drivers/media/platform/atmel/atmel-isi.h | 2 +- > > drivers/media/platform/cadence/cdns-csi2rx.c | 6 +- > > drivers/media/platform/intel/pxa_camera.c | 12 +- > > drivers/media/platform/marvell/cafe-driver.c | 5 +- > > drivers/media/platform/marvell/mcam-core.c | 4 +- > > drivers/media/platform/marvell/mmp-driver.c | 4 +- > > .../platform/microchip/microchip-csi2dc.c | 6 +- > > .../platform/microchip/microchip-isc-base.c | 4 +- > > .../media/platform/microchip/microchip-isc.h | 2 +- > > .../microchip/microchip-sama5d2-isc.c | 4 +- > > .../microchip/microchip-sama7g5-isc.c | 4 +- > > drivers/media/platform/nxp/imx-mipi-csis.c | 6 +- > > drivers/media/platform/nxp/imx7-media-csi.c | 6 +- > > .../platform/nxp/imx8-isi/imx8-isi-core.c | 8 +- > > drivers/media/platform/qcom/camss/camss.c | 2 +- > > drivers/media/platform/qcom/camss/camss.h | 2 +- > > drivers/media/platform/renesas/rcar-isp.c | 8 +- > > .../platform/renesas/rcar-vin/rcar-core.c | 44 ++--- > > .../platform/renesas/rcar-vin/rcar-csi2.c | 16 +- > > .../platform/renesas/rcar-vin/rcar-vin.h | 8 +- > > drivers/media/platform/renesas/rcar_drif.c | 8 +- > > drivers/media/platform/renesas/renesas-ceu.c | 6 +- > > .../platform/renesas/rzg2l-cru/rzg2l-core.c | 10 +- > > .../platform/renesas/rzg2l-cru/rzg2l-cru.h | 2 +- > > .../platform/renesas/rzg2l-cru/rzg2l-csi2.c | 8 +- > > .../platform/rockchip/rkisp1/rkisp1-common.h | 2 +- > > .../platform/rockchip/rkisp1/rkisp1-dev.c | 8 +- > > .../platform/samsung/exynos4-is/media-dev.c | 6 +- > > .../platform/samsung/exynos4-is/media-dev.h | 2 +- > > drivers/media/platform/st/stm32/stm32-dcmi.c | 8 +- > > .../platform/sunxi/sun4i-csi/sun4i_csi.c | 6 +- > > .../sunxi/sun6i-csi/sun6i_csi_bridge.c | 2 +- > > .../sunxi/sun6i-csi/sun6i_csi_bridge.h | 2 +- > > .../sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c | 6 +- > > .../sun8i_a83t_mipi_csi2.c | 6 +- > > .../media/platform/ti/am437x/am437x-vpfe.c | 5 +- > > .../media/platform/ti/am437x/am437x-vpfe.h | 2 +- > > drivers/media/platform/ti/cal/cal.c | 6 +- > > .../media/platform/ti/davinci/vpif_capture.c | 7 +- > > drivers/media/platform/ti/omap3isp/isp.h | 2 +- > > drivers/media/platform/video-mux.c | 6 +- > > drivers/media/platform/xilinx/xilinx-vipp.c | 22 +-- > > drivers/media/v4l2-core/v4l2-async.c | 159 +++++++++--------- > > drivers/media/v4l2-core/v4l2-fwnode.c | 8 +- > > .../media/deprecated/atmel/atmel-isc-base.c | 4 +- > > .../media/deprecated/atmel/atmel-isc.h | 2 +- > > .../deprecated/atmel/atmel-sama5d2-isc.c | 4 +- > > .../deprecated/atmel/atmel-sama7g5-isc.c | 4 +- > > drivers/staging/media/imx/imx-media-csi.c | 6 +- > > .../staging/media/imx/imx-media-dev-common.c | 2 +- > > 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 | 8 +- > > drivers/staging/media/imx/imx8mq-mipi-csi2.c | 6 +- > > .../media/sunxi/sun6i-isp/sun6i_isp_proc.c | 2 +- > > .../media/sunxi/sun6i-isp/sun6i_isp_proc.h | 2 +- > > drivers/staging/media/tegra-video/vi.c | 14 +- > > drivers/staging/media/tegra-video/vi.h | 2 +- > > include/media/davinci/vpif_types.h | 2 +- > > include/media/v4l2-async.h | 66 ++++---- > > include/media/v4l2-fwnode.h | 2 +- > > include/media/v4l2-subdev.h | 5 +- > > 68 files changed, 320 insertions(+), 322 deletions(-) > > > > diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst > > index ce8e9d0a332bc..83d3d29608136 100644 > > --- a/Documentation/driver-api/media/v4l2-subdev.rst > > +++ b/Documentation/driver-api/media/v4l2-subdev.rst > > @@ -214,14 +214,14 @@ notifier and further registers async sub-devices for lens and flash devices > > found in firmware. The notifier for the sub-device is unregistered with the > > async sub-device. > > > > -These functions allocate an async sub-device descriptor which is of type struct > > -:c:type:`v4l2_async_subdev` embedded in a driver-specific struct. The &struct > > -:c:type:`v4l2_async_subdev` shall be the first member of this struct: > > +These functions allocate an async connection descriptor which is of type struct > > +:c:type:`v4l2_async_connection` embedded in a driver-specific struct. The &struct > > +:c:type:`v4l2_async_connection` shall be the first member of this struct: > > > > .. code-block:: c > > > > struct my_async_subdev { > > - struct v4l2_async_subdev asd; > > + struct v4l2_async_connection asd; > > s/asd/asc/ Yes. There are other minor issues in the example, I'll address them in a separate patch. > > > ... > > }; > > > > @@ -244,10 +244,10 @@ notifier callback is called. After all subdevices have been located the > > system the .unbind() method is called. All three callbacks are optional. > > > > Drivers can store any type of custom data in their driver-specific > > -:c:type:`v4l2_async_subdev` wrapper. If any of that data requires special > > +:c:type:`v4l2_async_connection` wrapper. If any of that data requires special > > handling when the structure is freed, drivers must implement the ``.destroy()`` > > notifier callback. The framework will call it right before freeing the > > -:c:type:`v4l2_async_subdev`. > > +:c:type:`v4l2_async_connection`. > > > > Calling subdev operations > > ~~~~~~~~~~~~~~~~~~~~~~~~~ > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > > index 64f5c49cae776..44def5e3ba5d1 100644 > > --- a/drivers/media/i2c/max9286.c > > +++ b/drivers/media/i2c/max9286.c > > @@ -161,11 +161,12 @@ struct max9286_source { > > }; > > > > struct max9286_asd { > > This should be renamed to max9286_asc. I said in the review of a > previous version that I was fine keeping the name as-is, as the > v4l2_async_subdev structure was reintroduced later in the series, but > that's not the case anymore. This should be less confusing now than with v2 where the internal async sub-device struct was not the same object that was used by drivers previously. > > > - struct v4l2_async_subdev base; > > + struct v4l2_async_connection base; > > struct max9286_source *source; > > }; > > > > -static inline struct max9286_asd *to_max9286_asd(struct v4l2_async_subdev *asd) > > +static inline struct max9286_asd * > > +to_max9286_asd(struct v4l2_async_connection *asd) > > s/asd/asc/g > > There's more below, in most drivers. Yes, but is it worth renaming them? In all existing drivers there's no functional change here. If you'd like them to be renamed, I think it should be done after this set, that has already more than 32 patches and more than 2000 lines of changes. > > > { > > return container_of(asd, struct max9286_asd, base); > > } > > @@ -659,7 +660,7 @@ static int max9286_set_pixelrate(struct max9286_priv *priv) > > > > static int max9286_notify_bound(struct v4l2_async_notifier *notifier, > > struct v4l2_subdev *subdev, > > - struct v4l2_async_subdev *asd) > > + struct v4l2_async_connection *asd) > > { > > struct max9286_priv *priv = sd_to_max9286(notifier->sd); > > struct max9286_source *source = to_max9286_asd(asd)->source; > > @@ -721,7 +722,7 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier, > > > > static void max9286_notify_unbind(struct v4l2_async_notifier *notifier, > > struct v4l2_subdev *subdev, > > - struct v4l2_async_subdev *asd) > > + struct v4l2_async_connection *asd) > > { > > struct max9286_priv *priv = sd_to_max9286(notifier->sd); > > struct max9286_source *source = to_max9286_asd(asd)->source; > > [snip] > > > diff --git a/drivers/media/platform/atmel/atmel-isi.h b/drivers/media/platform/atmel/atmel-isi.h > > index 7ad3895a2c87e..58ce900ca4c90 100644 > > --- a/drivers/media/platform/atmel/atmel-isi.h > > +++ b/drivers/media/platform/atmel/atmel-isi.h > > @@ -121,7 +121,7 @@ > > #define ISI_DATAWIDTH_8 0x01 > > #define ISI_DATAWIDTH_10 0x02 > > > > -struct v4l2_async_subdev; > > +struct v4l2_async_connection; > > You can actually drop this, it's not used in this file. Ack. I think I'll add a new patch for this as these don't really belong here. > > > > > struct isi_platform_data { > > u8 has_emb_sync; > > [snip] > > > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h > > index cb206d3976ddf..c99d64e1cb01f 100644 > > --- a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h > > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h > > @@ -106,7 +106,7 @@ struct rvin_video_format { > > > > /** > > * struct rvin_parallel_entity - Parallel video input endpoint descriptor > > - * @asd: sub-device descriptor for async framework > > + * @asc: sub-device descriptor for async framework > > The description of the field should also be updated. I'll address that in v4. > > > * @subdev: subdevice matched using async framework > > * @mbus_type: media bus type > > * @bus: media bus parallel configuration > > @@ -115,7 +115,7 @@ struct rvin_video_format { > > * > > */ > > struct rvin_parallel_entity { > > - struct v4l2_async_subdev *asd; > > + struct v4l2_async_connection *asc; > > struct v4l2_subdev *subdev; > > > > enum v4l2_mbus_type mbus_type; > > @@ -275,7 +275,7 @@ struct rvin_dev { > > * @notifier: group notifier for CSI-2 async subdevices > > * @vin: VIN instances which are part of the group > > * @link_setup: Callback to create all links for the media graph > > - * @remotes: array of pairs of fwnode and subdev pointers > > + * @remotes: array of pairs of async connection and subdev pointers > > * to all remote subdevices. > > */ > > struct rvin_group { > > @@ -291,7 +291,7 @@ struct rvin_group { > > int (*link_setup)(struct rvin_dev *vin); > > > > struct { > > - struct v4l2_async_subdev *asd; > > + struct v4l2_async_connection *asc; > > struct v4l2_subdev *subdev; > > } remotes[RVIN_REMOTES_MAX]; > > }; > > [snip] > > > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h > > index 54f9f45ed3d8e..38d9d097fdb52 100644 > > --- a/include/media/v4l2-async.h > > +++ b/include/media/v4l2-async.h > > @@ -34,7 +34,7 @@ enum v4l2_async_match_type { > > }; > > > > /** > > - * struct v4l2_async_match_desc - async sub-device match information > > + * struct v4l2_async_match_desc - async connection match information > > * > > * @type: type of match that will be used > > * @fwnode: pointer to &struct fwnode_handle to be matched. > > @@ -62,21 +62,21 @@ struct v4l2_async_match_desc { > > }; > > > > /** > > - * struct v4l2_async_subdev - sub-device descriptor, as known to a bridge > > + * struct v4l2_async_connection - connection descriptor, as known to a bridge > > * > > * @match: struct of match type and per-bus type matching data sets > > - * @asd_entry: used to add struct v4l2_async_subdev objects to the > > - * master notifier @asd_entry > > - * @waiting_entry: used to link struct v4l2_async_subdev objects, waiting to be > > - * probed, to a notifier->waiting_list list > > + * @asc_entry: used to add struct v4l2_async_connection objects to the > > + * master notifier @asc_list > > + * @waiting_entry: used to link struct v4l2_async_connection objects, waiting to > > + * be probed, to a notifier->waiting_list list > > * > > * When this struct is used as a member in a driver specific struct, > > * the driver specific struct shall contain the &struct > > - * v4l2_async_subdev as its first member. > > + * v4l2_async_connection as its first member. > > */ > > -struct v4l2_async_subdev { > > +struct v4l2_async_connection { > > struct v4l2_async_match_desc match; > > - struct list_head asd_entry; > > + struct list_head asc_entry; > > struct list_head waiting_entry; > > }; > > > > @@ -86,17 +86,17 @@ struct v4l2_async_subdev { > > * @complete: All subdevices have been probed successfully. The complete > > * callback is only executed for the root notifier. > > * @unbind: a subdevice is leaving > > - * @destroy: the asd is about to be freed > > + * @destroy: the asc is about to be freed > > */ > > struct v4l2_async_notifier_operations { > > int (*bound)(struct v4l2_async_notifier *notifier, > > struct v4l2_subdev *subdev, > > - struct v4l2_async_subdev *asd); > > + struct v4l2_async_connection *asc); > > int (*complete)(struct v4l2_async_notifier *notifier); > > void (*unbind)(struct v4l2_async_notifier *notifier, > > struct v4l2_subdev *subdev, > > - struct v4l2_async_subdev *asd); > > - void (*destroy)(struct v4l2_async_subdev *asd); > > + struct v4l2_async_connection *asc); > > + void (*destroy)(struct v4l2_async_connection *asc); > > }; > > > > /** > > @@ -106,7 +106,7 @@ 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 > > + * @asc_list: master list of struct v4l2_async_subdev > > v4l2_async_subdev is no more. > > > * @waiting_list: list of struct v4l2_async_subdev, waiting for their drivers > > Same here. I'll check these for v4. > > > * @done_list: list of struct v4l2_subdev, already probed > > * @notifier_entry: member in a global list of notifiers > > @@ -116,7 +116,7 @@ struct v4l2_async_notifier { > > struct v4l2_device *v4l2_dev; > > struct v4l2_subdev *sd; > > struct v4l2_async_notifier *parent; > > - struct list_head asd_list; > > + struct list_head asc_list; > > struct list_head waiting_list; > > struct list_head done_list; > > struct list_head notifier_entry; > > @@ -134,53 +134,53 @@ void v4l2_async_debug_init(struct dentry *debugfs_dir); > > * > > * @notifier: pointer to &struct v4l2_async_notifier > > * > > - * This function initializes the notifier @asd_entry. It must be called > > + * This function initializes the notifier @asc_entry. It must be called > > This sounds like it should be asc_list. The issues was likely introduced > in an earlier patch in the series. I believe so, yes. For v4. > > > * before adding a subdevice to a notifier, using one of: > > * v4l2_async_nf_add_fwnode_remote(), v4l2_async_nf_add_fwnode() or > > * v4l2_async_nf_add_i2c(). > > */ > > void v4l2_async_nf_init(struct v4l2_async_notifier *notifier); > > > > -struct v4l2_async_subdev * > > +struct v4l2_async_connection * > > __v4l2_async_nf_add_fwnode(struct v4l2_async_notifier *notifier, > > struct fwnode_handle *fwnode, > > - unsigned int asd_struct_size); > > + unsigned int asc_struct_size); > > /** > > * v4l2_async_nf_add_fwnode - Allocate and add a fwnode async > > - * subdev to the notifier's master asd_entry. > > + * subdev to the notifier's master asc_entry. > > Same here. > > > * > > * @notifier: pointer to &struct v4l2_async_notifier > > * @fwnode: fwnode handle of the sub-device to be matched, pointer to > > * &struct fwnode_handle > > - * @type: Type of the driver's async sub-device struct. The &struct > > - * v4l2_async_subdev shall be the first member of the driver's async > > - * sub-device struct, i.e. both begin at the same memory address. > > + * @type: Type of the driver's async sub-device or connection struct. The > > + * &struct v4l2_async_connection shall be the first member of the > > + * driver's async struct, i.e. both begin at the same memory address. > > * > > - * Allocate a fwnode-matched asd of size asd_struct_size, and add it to the > > - * notifiers @asd_entry. The function also gets a reference of the fwnode which > > + * Allocate a fwnode-matched asc of size asc_struct_size, and add it to the > > + * notifiers @asc_entry. The function also gets a reference of the fwnode which > > Here too. > > > * is released later at notifier cleanup time. > > */ > > #define v4l2_async_nf_add_fwnode(notifier, fwnode, type) \ > > ((type *)__v4l2_async_nf_add_fwnode(notifier, fwnode, sizeof(type))) > > > > -struct v4l2_async_subdev * > > +struct v4l2_async_connection * > > __v4l2_async_nf_add_fwnode_remote(struct v4l2_async_notifier *notif, > > struct fwnode_handle *endpoint, > > - unsigned int asd_struct_size); > > + unsigned int asc_struct_size); > > /** > > * v4l2_async_nf_add_fwnode_remote - Allocate and add a fwnode > > * remote async subdev to the > > - * notifier's master asd_entry. > > + * notifier's master asc_entry. > > And here. > > > * > > * @notifier: pointer to &struct v4l2_async_notifier > > * @ep: local endpoint pointing to the remote sub-device to be matched, > > * pointer to &struct fwnode_handle > > * @type: Type of the driver's async sub-device struct. The &struct > > - * v4l2_async_subdev shall be the first member of the driver's async > > + * 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. > > * > > * Gets the remote endpoint of a given local endpoint, set it up for fwnode > > - * matching and adds the async sub-device to the notifier's @asd_entry. The > > + * matching and adds the async connection to the notifier's @asc_entry. The > > Here too again. > > > * function also gets a reference of the fwnode which is released later at > > * notifier cleanup time. > > * > > @@ -190,19 +190,19 @@ __v4l2_async_nf_add_fwnode_remote(struct v4l2_async_notifier *notif, > > #define v4l2_async_nf_add_fwnode_remote(notifier, ep, type) \ > > ((type *)__v4l2_async_nf_add_fwnode_remote(notifier, ep, sizeof(type))) > > > > -struct v4l2_async_subdev * > > +struct v4l2_async_connection * > > __v4l2_async_nf_add_i2c(struct v4l2_async_notifier *notifier, > > int adapter_id, unsigned short address, > > - unsigned int asd_struct_size); > > + unsigned int asc_struct_size); > > /** > > * v4l2_async_nf_add_i2c - Allocate and add an i2c async > > - * subdev to the notifier's master asd_entry. > > + * subdev to the notifier's master asc_entry. > > And finally here. > > > * > > * @notifier: pointer to &struct v4l2_async_notifier > > * @adapter: I2C adapter ID to be matched > > * @address: I2C address of sub-device to be matched > > * @type: Type of the driver's async sub-device struct. The &struct > > - * v4l2_async_subdev shall be the first member of the driver's async > > + * 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. > > s/sub-device/connection/ ? Please double-check if other instances of > subdev(ice) have been forgotten. Sure. > > > * > > * Same as v4l2_async_nf_add_fwnode() but for I2C matched > > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h > > index 855dae84b751d..4e4a6cf83097a 100644 > > --- a/include/media/v4l2-fwnode.h > > +++ b/include/media/v4l2-fwnode.h > > @@ -23,7 +23,7 @@ > > > > struct fwnode_handle; > > struct v4l2_async_notifier; > > -struct v4l2_async_subdev; > > +struct v4l2_async_connection; > > This is a sign the forward-declaration isn't needed. Actually I think they all can be now removed, probably due to Jacopo's patch. I'll address this for v4. > > > > > /** > > * struct v4l2_fwnode_endpoint - the endpoint data structure > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > > index 82e4cf3dd2e05..215fc8af87614 100644 > > --- a/include/media/v4l2-subdev.h > > +++ b/include/media/v4l2-subdev.h > > @@ -1022,8 +1022,7 @@ struct v4l2_subdev_platform_data { > > * either dev->of_node->fwnode or dev->fwnode (whichever is non-NULL). > > * @async_list: Links this subdev to a global subdev_entry or @notifier->done > > * list. > > - * @asd: Pointer to respective &struct v4l2_async_subdev. > > - * @notifier: Pointer to the managing notifier. > > Did you drop this field by mistake ? Oops. This indeed belongs to a later patch, not here. > > > + * @asd: Pointer to respective &struct v4l2_async_connection. > > * @subdev_notifier: A sub-device notifier implicitly registered for the sub- > > * device using v4l2_async_register_subdev_sensor(). > > * @pdata: common part of subdevice platform data > > @@ -1065,7 +1064,7 @@ struct v4l2_subdev { > > struct device *dev; > > struct fwnode_handle *fwnode; > > struct list_head async_list; > > - struct v4l2_async_subdev *asd; > > + struct v4l2_async_connection *asd; > > struct v4l2_async_notifier *notifier; > > struct v4l2_async_notifier *subdev_notifier; > > struct v4l2_subdev_platform_data *pdata; >
Hi Laurent, On Tue, May 30, 2023 at 08:52:24AM +0300, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Thu, May 25, 2023 at 12:15:58PM +0300, Sakari Ailus wrote: > > Add labels for error handling instead of doing it all in individual cases. > > Prepare for more functionality in this function. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/v4l2-core/v4l2-async.c | 21 ++++++++++++--------- > > 1 file changed, 12 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > index b1025dfc27a92..f51f0c37210c9 100644 > > --- a/drivers/media/v4l2-core/v4l2-async.c > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > @@ -320,10 +320,8 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier, > > return ret; > > > > ret = v4l2_async_nf_call_bound(notifier, sd, asc); > > - if (ret < 0) { > > - v4l2_device_unregister_subdev(sd); > > - return ret; > > - } > > + if (ret < 0) > > + goto err_unregister_subdev; > > > > /* > > * Depending of the function of the entities involved, we may want to > > @@ -332,11 +330,8 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier, > > * pad). > > */ > > ret = v4l2_async_create_ancillary_links(notifier, sd); > > - if (ret) { > > - v4l2_async_nf_call_unbind(notifier, sd, asc); > > - v4l2_device_unregister_subdev(sd); > > - return ret; > > - } > > + if (ret) > > + goto err_call_unbind; > > > > list_del(&asc->waiting_entry); > > sd->asd = asc; > > @@ -363,6 +358,14 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier, > > subdev_notifier->parent = notifier; > > > > return v4l2_async_nf_try_all_subdevs(subdev_notifier); > > Unrelated to this patch, but shoulnd't this have error handling too ? You're absolutely right. Bad things will happen if this fails. :-( > > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> Thank you! > > > + > > +err_call_unbind: > > + v4l2_async_nf_call_unbind(notifier, sd, asc); > > + > > +err_unregister_subdev: > > + v4l2_device_unregister_subdev(sd); > > + > > + return ret; > > } > > > > /* Test all async sub-devices in a notifier for a match. */
Hi Laurent, On Tue, May 30, 2023 at 09:01:15AM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Thu, May 25, 2023 at 12:15:59PM +0300, Sakari Ailus wrote: > > The connections are checked for duplicates already when the notifier is > > registered. This is effectively a sanity check for driver (and possibly > > obscure firmware) bugs. Don't do this when adding the connection. > > Isn't it better to have this sanity check when the connection is added, > instead of later when the notifier is registered ? The latter is more > difficult to debug. If you want to avoid duplicate checks, could we drop > the one at notifier registration time ? I've never seen or heard this check failing. I'm therefore not very concerned keeping it as easy to debug as possible, instead I prefer simpler implementation. Checking at the registration time is still necessary as the same match could have been added to another notifier while this one was being set up.
Hi all, (Resending v3, with a wider distribution. This set just touches so many drivers. It would be nice to have this tested with as much hardware as possible. I wouldn't mind reviews either.) 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. A connection later on translates to an MC ancillary or data link. 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. I didn't add the Tested-by: tags I got for v2 as there are significant changes, albeit only for better I hope. :-) Niklas has also tested this on rcar-vin + adv746x, it works now while on v2 it did not. So having also this version validated on i.MX6 would be nice. The code also can be found in my async-multi branch, on media tree master. since v2: - Drop the sub-devices (struct v4l2_async_subdev) earlier re-introduced in this patchset. They aren't necessary, instead we can use struct v4l2_subdev to store the information. This simplifies the code quite a bit, including removal of one of the global lists: only the sub-device and notifier lists are left now. The sub-device is assigned to the connection at the bound time: information on which sub-devices are connected via async connections is only actually available at the time of binding. - Address issues related to sub-device handling in the rcar-vin driver. Also other fixes to the rcar-vin driver (bugs introduced in v2). - Remove sub-device's notifier field, including an omap3isp patch removing an unnecessary test. - Drop support for async sub-device side endpoint matching. Convert NXP imx-mipi-csis. since v1: - Fixed object relation issues. The set has now been tested on an async sub-device with two pads connected by data links to sub-devices on a notifier (struct) device. (Multiple notifiers should work, too, but has not been tested.) - Add a function to obtain an async connection based on the sub-device. This is useful for drivers for accessing their own link specific data. - Improved documentation. Include a patch documenting v4l2_async_nf_add_fwnode(). - Return endpoint matching and address adv748x driver breakage in v1. It's a special case so other drivers can remain simpler. - Swap notifier initialisation arguments, by making the notifier the first argument. - Remove extra fwnode_handle_put() in max9286_v4l2_unregister(). - Make struct device available before notifier initialisation for consistent debug messages. - Simplify notifier and async sub-device linked lists. Consistent list head and list entry naming. - Drop leftovers from an early experimenation work in rkisp1 and omap3isp drivers. - Simplify xilinx-vipp sub-device binding. - Use if()s in notifier_dev() of v4l2-async.c. - Improved debug messages in v4l2-async.c, use v4l2-async prefix and generally with notifier device. - Call match types with macros V4L2_ASYNC_MATCH_TYPE_* (was V4L2_ASYNC_MATCH_*). - Create ancillary links only when the sub-device is registered, not when a connection is bound (which can take place more than once for a sub-device). - Rename struct v4l2_async_match as v4l2_async_match_desc. - Perform list initialisation in notifier init rather than registration. - Get rid of the "readd" parameter for v4l2_async_nf_unbind_all_subdevs(). - Check async sub-device validity on a notifier only when the notifier is registered. This removes extra list traversal and simplifies the code. - Remove extra list initialisation in v4l2_async_register_subdev(). - Drop v4l2_async_cleanup(). It was no longer useful, called from a single place. - Lots of kerneldoc fixes (mostly changed argument names). 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). Jacopo Mondi (1): media: v4l: async: Drop v4l2_async_nf_parse_fwnode_endpoints() Sakari Ailus (31): media: Documentation: v4l: Document missing async subdev function media: xilinx-vipp: Clean up bound async notifier callback media: omap3isp: Don't check for the sub-device's notifier media: v4l: async: Add some debug prints media: v4l: async: Clean up testing for duplicate async subdevs media: v4l: async: Drop unneeded list entry initialisation media: v4l: async: Don't check whether asd is NULL in validity check media: v4l: async: Make V4L2 async match information a struct media: v4l: async: Rename V4L2_ASYNC_MATCH_ macros, add TYPE_ media: v4l: async: Only pass match information for async subdev validation media: v4l: async: Clean up list heads and entries media: v4l: async: Simplify async sub-device fwnode matching media: v4l: async: Rename v4l2_async_subdev as v4l2_async_connection media: v4l: async: Clean up error handling in v4l2_async_match_notify media: v4l: async: Drop duplicate handling when adding connections media: v4l: async: Rework internal lists media: v4l: async: Obtain async connection based on sub-device media: v4l: async: Allow multiple connections between entities media: v4l: async: Try more connections media: v4l: async: Support fwnode endpoint list matching for subdevs media: adv748x: Return to endpoint matching media: pxa_camera: Fix probe error handling media: pxa_camera: Register V4L2 device early 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 media: Documentation: v4l: Document sub-device notifiers .../driver-api/media/v4l2-subdev.rst | 28 +- drivers/media/i2c/adv748x/adv748x-csi2.c | 13 +- drivers/media/i2c/max9286.c | 27 +- drivers/media/i2c/rdacm20.c | 16 +- drivers/media/i2c/rdacm21.c | 15 +- drivers/media/i2c/st-mipid02.c | 12 +- drivers/media/i2c/tc358746.c | 15 +- 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 | 77 +- drivers/media/platform/marvell/cafe-driver.c | 18 +- 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 | 17 +- drivers/media/platform/nxp/imx7-media-csi.c | 10 +- .../platform/nxp/imx8-isi/imx8-isi-core.c | 12 +- 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 | 52 +- .../platform/renesas/rcar-vin/rcar-csi2.c | 20 +- .../platform/renesas/rcar-vin/rcar-vin.h | 8 +- 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-common.h | 2 +- .../platform/rockchip/rkisp1/rkisp1-csi.c | 7 +- .../platform/rockchip/rkisp1/rkisp1-dev.c | 12 +- .../platform/rockchip/rkisp1/rkisp1-isp.c | 8 +- .../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 | 36 +- .../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 | 29 +- drivers/media/platform/ti/omap3isp/isp.h | 15 +- drivers/media/platform/ti/omap3isp/ispccdc.c | 13 +- drivers/media/platform/ti/omap3isp/ispccp2.c | 2 + drivers/media/platform/ti/omap3isp/ispcsi2.c | 2 + .../media/platform/ti/omap3isp/ispcsiphy.c | 15 +- drivers/media/platform/video-mux.c | 10 +- drivers/media/platform/xilinx/xilinx-vipp.c | 55 +- drivers/media/v4l2-core/v4l2-async.c | 685 ++++++++++-------- drivers/media/v4l2-core/v4l2-fwnode.c | 109 +-- drivers/media/v4l2-core/v4l2-subdev.c | 13 + .../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 | 9 +- drivers/staging/media/imx/imx-media-csi.c | 10 +- .../staging/media/imx/imx-media-dev-common.c | 6 +- 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 | 218 +++--- include/media/v4l2-fwnode.h | 68 +- include/media/v4l2-subdev.h | 17 +- 79 files changed, 973 insertions(+), 1072 deletions(-)