[06/18] media: v4l: async: Only pass match information for async subdev validation
Commit Message
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.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/v4l2-core/v4l2-async.c | 93 ++++++++++++++--------------
1 file changed, 46 insertions(+), 47 deletions(-)
Comments
Hi Sakari
On Thu, Mar 30, 2023 at 02:58:41PM +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.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/v4l2-core/v4l2-async.c | 93 ++++++++++++++--------------
> 1 file changed, 46 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index fc9ae22e2b47..224ebf50f2d0 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -62,14 +62,14 @@ 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 *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
> @@ -84,26 +84,26 @@ 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 *match)
> {
> struct fwnode_handle *asd_dev_fwnode;
> bool ret;
>
> dev_dbg(sd->dev, "async: fwnode match: need %pfw, trying %pfw\n",
> - sd_fwnode, asd->match.fwnode);
> + sd_fwnode, match->fwnode);
>
> - if (sd_fwnode == asd->match.fwnode) {
> + if (sd_fwnode == match->fwnode) {
> dev_dbg(sd->dev, "async: direct match found\n");
> return true;
> }
>
> - if (!fwnode_graph_is_endpoint(asd->match.fwnode)) {
> + if (!fwnode_graph_is_endpoint(match->fwnode)) {
> dev_dbg(sd->dev,
> "async: async subdev fwnode not endpoint, no match\n");
> return false;
> }
>
> - asd_dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode);
> + asd_dev_fwnode = fwnode_graph_get_port_parent(match->fwnode);
>
> ret = sd_fwnode == asd_dev_fwnode;
>
> @@ -116,12 +116,12 @@ 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 *match)
> {
> dev_dbg(sd->dev, "async: matching for notifier %pfw, sd %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. */
> @@ -130,7 +130,7 @@ static bool match_fwnode(struct v4l2_async_notifier *notifier,
>
> dev_dbg(sd->dev, "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);
> @@ -142,7 +142,7 @@ 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 *match);
> struct v4l2_async_subdev *asd;
>
> list_for_each_entry(asd, ¬ifier->waiting, list) {
> @@ -161,7 +161,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;
> }
>
> @@ -169,20 +169,18 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier,
> }
>
> /* Compare two async sub-device descriptors for equivalence */
> -static bool asd_equal(struct v4l2_async_subdev *asd_x,
> - struct v4l2_async_subdev *asd_y)
> +static bool asd_equal(struct v4l2_async_match *match1,
> + struct v4l2_async_match *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_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_FWNODE:
> - return asd_x->match.fwnode == asd_y->match.fwnode;
> + return match1->fwnode == match2->fwnode;
> default:
> break;
> }
> @@ -434,20 +432,20 @@ 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)
> + struct v4l2_async_match *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 (asd_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 (asd_equal(&sd->asd->match, match))
> return true;
> }
>
> @@ -460,49 +458,50 @@ __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier,
> */
> static bool
> v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier,
> - struct v4l2_async_subdev *asd, bool skip_self)
> + struct v4l2_async_match *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 (asd == asd_y)
> + list_for_each_entry(asd, ¬ifier->asd_list, asd_list) {
> + if (&asd->match == match)
> break;
> - if (asd_equal(asd, asd_y))
> + if (asd_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_subdev(notifier, match))
> return true;
>
> return false;
> }
>
> static int v4l2_async_nf_asd_valid(struct v4l2_async_notifier *notifier,
> - struct v4l2_async_subdev *asd,
> + struct v4l2_async_match *match,
I would have kept the asd here, but I presume having match here makes
things easier in the next patches
> bool skip_self)
> {
> struct device *dev =
> notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL;
>
> - if (!asd)
> + if (!match)
> return -EINVAL;
Match cannot be null, as it's a member of struct v4l2_async_subdev
>
> - switch (asd->match.type) {
> + switch (match->type) {
> case V4L2_ASYNC_MATCH_I2C:
> case V4L2_ASYNC_MATCH_FWNODE:
> - if (v4l2_async_nf_has_async_subdev(notifier, asd, skip_self)) {
> + if (v4l2_async_nf_has_async_subdev(notifier, match,
> + skip_self)) {
> dev_dbg(dev, "subdev descriptor already listed in this or other notifiers\n");
> return -EEXIST;
> }
> break;
> default:
> - dev_err(dev, "Invalid match type %u on %p\n",
> - asd->match.type, asd);
> + dev_err(dev, "Invalid match type %u on %p\n", match->type,
> + match);
> return -EINVAL;
> }
>
> @@ -526,7 +525,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_asd_valid(notifier, &asd->match, true);
> if (ret)
> goto err_unlock;
>
> @@ -659,7 +658,7 @@ 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_asd_valid(notifier, &asd->match, false);
> if (ret)
> goto unlock;
>
> @@ -846,15 +845,15 @@ 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)
> + struct v4l2_async_match *match)
> {
> - switch (asd->match.type) {
> + switch (match->type) {
> case V4L2_ASYNC_MATCH_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_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) :
> @@ -891,7 +890,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_subdev(s, &asd->match);
> }
>
> mutex_unlock(&list_lock);
> --
> 2.30.2
>
Hi Jacopo,
On Fri, Apr 14, 2023 at 09:15:23AM +0200, Jacopo Mondi wrote:
> > static int v4l2_async_nf_asd_valid(struct v4l2_async_notifier *notifier,
> > - struct v4l2_async_subdev *asd,
> > + struct v4l2_async_match *match,
>
> I would have kept the asd here, but I presume having match here makes
> things easier in the next patches
The other fields won't be needed there, that's why I'd pass just the match
information.
>
>
> > bool skip_self)
> > {
> > struct device *dev =
> > notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL;
> >
> > - if (!asd)
> > + if (!match)
> > return -EINVAL;
>
> Match cannot be null, as it's a member of struct v4l2_async_subdev
Indeed. I'll add another patch to drop the check, it doesn't seem to be
needed to begin with.
Hi Sakari,
Thank you for the patch.
On Thu, Mar 30, 2023 at 02:58:41PM +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.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/v4l2-core/v4l2-async.c | 93 ++++++++++++++--------------
> 1 file changed, 46 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index fc9ae22e2b47..224ebf50f2d0 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -62,14 +62,14 @@ 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 *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
> @@ -84,26 +84,26 @@ 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 *match)
> {
> struct fwnode_handle *asd_dev_fwnode;
match_dev_node, to remove references to asd ?
> bool ret;
>
> dev_dbg(sd->dev, "async: fwnode match: need %pfw, trying %pfw\n",
> - sd_fwnode, asd->match.fwnode);
> + sd_fwnode, match->fwnode);
>
> - if (sd_fwnode == asd->match.fwnode) {
> + if (sd_fwnode == match->fwnode) {
> dev_dbg(sd->dev, "async: direct match found\n");
> return true;
> }
>
> - if (!fwnode_graph_is_endpoint(asd->match.fwnode)) {
> + if (!fwnode_graph_is_endpoint(match->fwnode)) {
> dev_dbg(sd->dev,
> "async: async subdev fwnode not endpoint, no match\n");
The reference to "subdev" could be dropped here too.
> return false;
> }
>
> - asd_dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode);
> + asd_dev_fwnode = fwnode_graph_get_port_parent(match->fwnode);
>
> ret = sd_fwnode == asd_dev_fwnode;
>
> @@ -116,12 +116,12 @@ 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 *match)
> {
> dev_dbg(sd->dev, "async: matching for notifier %pfw, sd %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. */
> @@ -130,7 +130,7 @@ static bool match_fwnode(struct v4l2_async_notifier *notifier,
>
> dev_dbg(sd->dev, "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);
> @@ -142,7 +142,7 @@ 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 *match);
> struct v4l2_async_subdev *asd;
>
> list_for_each_entry(asd, ¬ifier->waiting, list) {
> @@ -161,7 +161,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;
> }
>
> @@ -169,20 +169,18 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier,
> }
>
> /* Compare two async sub-device descriptors for equivalence */
> -static bool asd_equal(struct v4l2_async_subdev *asd_x,
> - struct v4l2_async_subdev *asd_y)
> +static bool asd_equal(struct v4l2_async_match *match1,
> + struct v4l2_async_match *match2)
The function doesn't deal with asds anymore, let's rename it.
v4l2_async_match_equal() could be a good name. Please also update the
comment above the function.
> {
> - 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_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_FWNODE:
> - return asd_x->match.fwnode == asd_y->match.fwnode;
> + return match1->fwnode == match2->fwnode;
> default:
> break;
> }
> @@ -434,20 +432,20 @@ 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)
> + struct v4l2_async_match *match)
This function should be renamed to drop "subdev" as well.
__v4l2_async_nf_has_match() is a candidate,
__v4l2_async_nf_has_match_entry() is also an option to avoid implying
that the function tests for a match between a subdev and a match entry.
> {
> - 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 (asd_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 (asd_equal(&sd->asd->match, match))
> return true;
> }
>
> @@ -460,49 +458,50 @@ __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier,
> */
> static bool
> v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier,
> - struct v4l2_async_subdev *asd, bool skip_self)
> + struct v4l2_async_match *match, bool skip_self)
Same here.
> {
> - 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 (asd == asd_y)
> + list_for_each_entry(asd, ¬ifier->asd_list, asd_list) {
> + if (&asd->match == match)
> break;
> - if (asd_equal(asd, asd_y))
> + if (asd_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_subdev(notifier, match))
> return true;
>
> return false;
> }
>
> static int v4l2_async_nf_asd_valid(struct v4l2_async_notifier *notifier,
> - struct v4l2_async_subdev *asd,
> + struct v4l2_async_match *match,
> bool skip_self)
And here.
> {
> struct device *dev =
> notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL;
>
> - if (!asd)
> + if (!match)
> return -EINVAL;
>
> - switch (asd->match.type) {
> + switch (match->type) {
> case V4L2_ASYNC_MATCH_I2C:
> case V4L2_ASYNC_MATCH_FWNODE:
> - if (v4l2_async_nf_has_async_subdev(notifier, asd, skip_self)) {
> + if (v4l2_async_nf_has_async_subdev(notifier, match,
> + skip_self)) {
> dev_dbg(dev, "subdev descriptor already listed in this or other notifiers\n");
"match descriptor" ?
> return -EEXIST;
> }
> break;
> default:
> - dev_err(dev, "Invalid match type %u on %p\n",
> - asd->match.type, asd);
> + dev_err(dev, "Invalid match type %u on %p\n", match->type,
> + match);
> return -EINVAL;
> }
>
> @@ -526,7 +525,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_asd_valid(notifier, &asd->match, true);
> if (ret)
> goto err_unlock;
>
> @@ -659,7 +658,7 @@ 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_asd_valid(notifier, &asd->match, false);
> if (ret)
> goto unlock;
>
> @@ -846,15 +845,15 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
> EXPORT_SYMBOL(v4l2_async_unregister_subdev);
>
> static void print_waiting_subdev(struct seq_file *s,
Same here. v4l2_async_print_match() or v4l2_async_print_match_entry()
could be good names.
In general, it would be useful to have a glossary of terms used in
v4l2-async, and make sure they're used consistently.
> - struct v4l2_async_subdev *asd)
> + struct v4l2_async_match *match)
> {
> - switch (asd->match.type) {
> + switch (match->type) {
> case V4L2_ASYNC_MATCH_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_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) :
> @@ -891,7 +890,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_subdev(s, &asd->match);
> }
>
> mutex_unlock(&list_lock);
Hi Laurent,
On Tue, Apr 25, 2023 at 04:24:26AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
Thank you for the review.
>
> On Thu, Mar 30, 2023 at 02:58:41PM +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.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > drivers/media/v4l2-core/v4l2-async.c | 93 ++++++++++++++--------------
> > 1 file changed, 46 insertions(+), 47 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index fc9ae22e2b47..224ebf50f2d0 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -62,14 +62,14 @@ 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 *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
> > @@ -84,26 +84,26 @@ 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 *match)
> > {
> > struct fwnode_handle *asd_dev_fwnode;
>
> match_dev_node, to remove references to asd ?
Sounds good.
>
> > bool ret;
> >
> > dev_dbg(sd->dev, "async: fwnode match: need %pfw, trying %pfw\n",
> > - sd_fwnode, asd->match.fwnode);
> > + sd_fwnode, match->fwnode);
> >
> > - if (sd_fwnode == asd->match.fwnode) {
> > + if (sd_fwnode == match->fwnode) {
> > dev_dbg(sd->dev, "async: direct match found\n");
> > return true;
> > }
> >
> > - if (!fwnode_graph_is_endpoint(asd->match.fwnode)) {
> > + if (!fwnode_graph_is_endpoint(match->fwnode)) {
> > dev_dbg(sd->dev,
> > "async: async subdev fwnode not endpoint, no match\n");
>
> The reference to "subdev" could be dropped here too.
This will get changed in an earlier patch to "v4l2-async: direct match
failed".
>
> > return false;
> > }
> >
> > - asd_dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode);
> > + asd_dev_fwnode = fwnode_graph_get_port_parent(match->fwnode);
> >
> > ret = sd_fwnode == asd_dev_fwnode;
> >
> > @@ -116,12 +116,12 @@ 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 *match)
> > {
> > dev_dbg(sd->dev, "async: matching for notifier %pfw, sd %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. */
> > @@ -130,7 +130,7 @@ static bool match_fwnode(struct v4l2_async_notifier *notifier,
> >
> > dev_dbg(sd->dev, "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);
> > @@ -142,7 +142,7 @@ 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 *match);
> > struct v4l2_async_subdev *asd;
> >
> > list_for_each_entry(asd, ¬ifier->waiting, list) {
> > @@ -161,7 +161,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;
> > }
> >
> > @@ -169,20 +169,18 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier,
> > }
> >
> > /* Compare two async sub-device descriptors for equivalence */
> > -static bool asd_equal(struct v4l2_async_subdev *asd_x,
> > - struct v4l2_async_subdev *asd_y)
> > +static bool asd_equal(struct v4l2_async_match *match1,
> > + struct v4l2_async_match *match2)
>
> The function doesn't deal with asds anymore, let's rename it.
> v4l2_async_match_equal() could be a good name. Please also update the
> comment above the function.
Seems fine.
>
> > {
> > - 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_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_FWNODE:
> > - return asd_x->match.fwnode == asd_y->match.fwnode;
> > + return match1->fwnode == match2->fwnode;
> > default:
> > break;
> > }
> > @@ -434,20 +432,20 @@ 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)
> > + struct v4l2_async_match *match)
>
> This function should be renamed to drop "subdev" as well.
> __v4l2_async_nf_has_match() is a candidate,
> __v4l2_async_nf_has_match_entry() is also an option to avoid implying
> that the function tests for a match between a subdev and a match entry.
Seems fine to me.
>
> > {
> > - 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 (asd_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 (asd_equal(&sd->asd->match, match))
> > return true;
> > }
> >
> > @@ -460,49 +458,50 @@ __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier,
> > */
> > static bool
> > v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier,
> > - struct v4l2_async_subdev *asd, bool skip_self)
> > + struct v4l2_async_match *match, bool skip_self)
>
> Same here.
>
> > {
> > - 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 (asd == asd_y)
> > + list_for_each_entry(asd, ¬ifier->asd_list, asd_list) {
> > + if (&asd->match == match)
> > break;
> > - if (asd_equal(asd, asd_y))
> > + if (asd_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_subdev(notifier, match))
> > return true;
> >
> > return false;
> > }
> >
> > static int v4l2_async_nf_asd_valid(struct v4l2_async_notifier *notifier,
> > - struct v4l2_async_subdev *asd,
> > + struct v4l2_async_match *match,
> > bool skip_self)
>
> And here.
>
> > {
> > struct device *dev =
> > notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL;
> >
> > - if (!asd)
> > + if (!match)
> > return -EINVAL;
> >
> > - switch (asd->match.type) {
> > + switch (match->type) {
> > case V4L2_ASYNC_MATCH_I2C:
> > case V4L2_ASYNC_MATCH_FWNODE:
> > - if (v4l2_async_nf_has_async_subdev(notifier, asd, skip_self)) {
> > + if (v4l2_async_nf_has_async_subdev(notifier, match,
> > + skip_self)) {
> > dev_dbg(dev, "subdev descriptor already listed in this or other notifiers\n");
>
> "match descriptor" ?
Yes!
>
> > return -EEXIST;
> > }
> > break;
> > default:
> > - dev_err(dev, "Invalid match type %u on %p\n",
> > - asd->match.type, asd);
> > + dev_err(dev, "Invalid match type %u on %p\n", match->type,
> > + match);
> > return -EINVAL;
> > }
> >
> > @@ -526,7 +525,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_asd_valid(notifier, &asd->match, true);
> > if (ret)
> > goto err_unlock;
> >
> > @@ -659,7 +658,7 @@ 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_asd_valid(notifier, &asd->match, false);
> > if (ret)
> > goto unlock;
> >
> > @@ -846,15 +845,15 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
> > EXPORT_SYMBOL(v4l2_async_unregister_subdev);
> >
> > static void print_waiting_subdev(struct seq_file *s,
>
> Same here. v4l2_async_print_match() or v4l2_async_print_match_entry()
> could be good names.
v4l2_async_print_waiting_matches()?
>
> In general, it would be useful to have a glossary of terms used in
> v4l2-async, and make sure they're used consistently.
I think this should be already outside this set. I'm all for consolidated
terms though. I'll go through the set in this respect as well.
>
> > - struct v4l2_async_subdev *asd)
> > + struct v4l2_async_match *match)
> > {
> > - switch (asd->match.type) {
> > + switch (match->type) {
> > case V4L2_ASYNC_MATCH_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_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) :
> > @@ -891,7 +890,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_subdev(s, &asd->match);
> > }
> >
> > mutex_unlock(&list_lock);
>
@@ -62,14 +62,14 @@ 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 *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
@@ -84,26 +84,26 @@ 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 *match)
{
struct fwnode_handle *asd_dev_fwnode;
bool ret;
dev_dbg(sd->dev, "async: fwnode match: need %pfw, trying %pfw\n",
- sd_fwnode, asd->match.fwnode);
+ sd_fwnode, match->fwnode);
- if (sd_fwnode == asd->match.fwnode) {
+ if (sd_fwnode == match->fwnode) {
dev_dbg(sd->dev, "async: direct match found\n");
return true;
}
- if (!fwnode_graph_is_endpoint(asd->match.fwnode)) {
+ if (!fwnode_graph_is_endpoint(match->fwnode)) {
dev_dbg(sd->dev,
"async: async subdev fwnode not endpoint, no match\n");
return false;
}
- asd_dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode);
+ asd_dev_fwnode = fwnode_graph_get_port_parent(match->fwnode);
ret = sd_fwnode == asd_dev_fwnode;
@@ -116,12 +116,12 @@ 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 *match)
{
dev_dbg(sd->dev, "async: matching for notifier %pfw, sd %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. */
@@ -130,7 +130,7 @@ static bool match_fwnode(struct v4l2_async_notifier *notifier,
dev_dbg(sd->dev, "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);
@@ -142,7 +142,7 @@ 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 *match);
struct v4l2_async_subdev *asd;
list_for_each_entry(asd, ¬ifier->waiting, list) {
@@ -161,7 +161,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;
}
@@ -169,20 +169,18 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier,
}
/* Compare two async sub-device descriptors for equivalence */
-static bool asd_equal(struct v4l2_async_subdev *asd_x,
- struct v4l2_async_subdev *asd_y)
+static bool asd_equal(struct v4l2_async_match *match1,
+ struct v4l2_async_match *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_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_FWNODE:
- return asd_x->match.fwnode == asd_y->match.fwnode;
+ return match1->fwnode == match2->fwnode;
default:
break;
}
@@ -434,20 +432,20 @@ 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)
+ struct v4l2_async_match *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 (asd_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 (asd_equal(&sd->asd->match, match))
return true;
}
@@ -460,49 +458,50 @@ __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier,
*/
static bool
v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier,
- struct v4l2_async_subdev *asd, bool skip_self)
+ struct v4l2_async_match *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 (asd == asd_y)
+ list_for_each_entry(asd, ¬ifier->asd_list, asd_list) {
+ if (&asd->match == match)
break;
- if (asd_equal(asd, asd_y))
+ if (asd_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_subdev(notifier, match))
return true;
return false;
}
static int v4l2_async_nf_asd_valid(struct v4l2_async_notifier *notifier,
- struct v4l2_async_subdev *asd,
+ struct v4l2_async_match *match,
bool skip_self)
{
struct device *dev =
notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL;
- if (!asd)
+ if (!match)
return -EINVAL;
- switch (asd->match.type) {
+ switch (match->type) {
case V4L2_ASYNC_MATCH_I2C:
case V4L2_ASYNC_MATCH_FWNODE:
- if (v4l2_async_nf_has_async_subdev(notifier, asd, skip_self)) {
+ if (v4l2_async_nf_has_async_subdev(notifier, match,
+ skip_self)) {
dev_dbg(dev, "subdev descriptor already listed in this or other notifiers\n");
return -EEXIST;
}
break;
default:
- dev_err(dev, "Invalid match type %u on %p\n",
- asd->match.type, asd);
+ dev_err(dev, "Invalid match type %u on %p\n", match->type,
+ match);
return -EINVAL;
}
@@ -526,7 +525,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_asd_valid(notifier, &asd->match, true);
if (ret)
goto err_unlock;
@@ -659,7 +658,7 @@ 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_asd_valid(notifier, &asd->match, false);
if (ret)
goto unlock;
@@ -846,15 +845,15 @@ 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)
+ struct v4l2_async_match *match)
{
- switch (asd->match.type) {
+ switch (match->type) {
case V4L2_ASYNC_MATCH_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_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) :
@@ -891,7 +890,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_subdev(s, &asd->match);
}
mutex_unlock(&list_lock);