From: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
To: Steve Longerbeam <slongerbeam@gmail.com>
Cc: linux-media@vger.kernel.org,
"Steve Longerbeam" <steve_longerbeam@mentor.com>,
"Mauro Carvalho Chehab" <mchehab@kernel.org>,
"Sakari Ailus" <sakari.ailus@linux.intel.com>,
"Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>,
"Hans Verkuil" <hans.verkuil@cisco.com>,
"Sebastian Reichel" <sre@kernel.org>,
linux-kernel@vger.kernel.org (open list)
Subject: Re: [PATCH v6 02/17] media: v4l2: async: Allow searching for asd of any type
Date: Mon, 24 Sep 2018 14:06:04 -0300 [thread overview]
Message-ID: <20180924140604.23e2b56f@coco.lan> (raw)
In-Reply-To: <1531175957-1973-3-git-send-email-steve_longerbeam@mentor.com>
Em Mon, 9 Jul 2018 15:39:02 -0700
Steve Longerbeam <slongerbeam@gmail.com> escreveu:
> Generalize v4l2_async_notifier_fwnode_has_async_subdev() to allow
> searching for any type of async subdev, not just fwnodes. Rename to
> v4l2_async_notifier_has_async_subdev() and pass it an asd pointer.
>
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
> Changes since v5:
> - none
> Changes since v4:
> - none
> Changes since v3:
> - removed TODO to support asd compare with CUSTOM match type in
> asd_equal().
> Changes since v2:
> - code optimization in asd_equal(), and remove unneeded braces,
> suggested by Sakari Ailus.
> Changes since v1:
> - none
> ---
> drivers/media/v4l2-core/v4l2-async.c | 73 +++++++++++++++++++++---------------
> 1 file changed, 43 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 2b08d03..0e7e529 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -124,6 +124,31 @@ static struct v4l2_async_subdev *v4l2_async_find_match(
> return NULL;
> }
>
> +/* Compare two asd's for equivalence */
Please, on comments, instead of "asd" prefer to use what this 3 random
letters mean, e. g.:
asd -> asynchronous subdevice
> +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)
> + return false;
> +
> + switch (asd_x->match_type) {
> + case V4L2_ASYNC_MATCH_DEVNAME:
> + return strcmp(asd_x->match.device_name,
> + asd_y->match.device_name) == 0;
> + 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;
> + case V4L2_ASYNC_MATCH_FWNODE:
> + return asd_x->match.fwnode == asd_y->match.fwnode;
> + default:
> + break;
> + }
> +
> + return false;
> +}
> +
> /* Find the sub-device notifier registered by a sub-device driver. */
> static struct v4l2_async_notifier *v4l2_async_find_subdev_notifier(
> struct v4l2_subdev *sd)
> @@ -308,29 +333,22 @@ static void v4l2_async_notifier_unbind_all_subdevs(
> notifier->parent = NULL;
> }
>
> -/* See if an fwnode can be found in a notifier's lists. */
> -static bool __v4l2_async_notifier_fwnode_has_async_subdev(
> - struct v4l2_async_notifier *notifier, struct fwnode_handle *fwnode)
> +/* See if an async sub-device can be found in a notifier's lists. */
> +static bool __v4l2_async_notifier_has_async_subdev(
> + struct v4l2_async_notifier *notifier, struct v4l2_async_subdev *asd)
This is a minor issue, but checkpatch complains (with reason)
(with --strict) about the above:
CHECK: Lines should not end with a '('
#63: FILE: drivers/media/v4l2-core/v4l2-async.c:337:
+static bool __v4l2_async_notifier_has_async_subdev(
Better to declare it, instead, as:
static bool
__v4l2_async_notifier_has_async_subdev(struct v4l2_async_notifier *notifier,
struct v4l2_async_subdev *asd)
Similar warnings appear on other places:
CHECK: Lines should not end with a '('
#102: FILE: drivers/media/v4l2-core/v4l2-async.c:362:
+static bool v4l2_async_notifier_has_async_subdev(
CHECK: Lines should not end with a '('
#141: FILE: drivers/media/v4l2-core/v4l2-async.c:410:
+ if (v4l2_async_notifier_has_async_subdev(
Btw, Checkpatch also complains that the author's email is different
than the SOB's one:
WARNING: Missing Signed-off-by: line by nominal patch author 'Steve Longerbeam <slongerbeam@gmail.com>'
(the some comes with Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>)
I suspect that other patches on this series will suffer from the same issue.
> {
> - struct v4l2_async_subdev *asd;
> + struct v4l2_async_subdev *asd_y;
> struct v4l2_subdev *sd;
>
> - list_for_each_entry(asd, ¬ifier->waiting, list) {
> - if (asd->match_type != V4L2_ASYNC_MATCH_FWNODE)
> - continue;
> -
> - if (asd->match.fwnode == fwnode)
> + list_for_each_entry(asd_y, ¬ifier->waiting, list)
> + if (asd_equal(asd, asd_y))
> return true;
> - }
>
> list_for_each_entry(sd, ¬ifier->done, async_list) {
> if (WARN_ON(!sd->asd))
> continue;
>
> - if (sd->asd->match_type != V4L2_ASYNC_MATCH_FWNODE)
> - continue;
> -
> - if (sd->asd->match.fwnode == fwnode)
> + if (asd_equal(asd, sd->asd))
> return true;
> }
>
> @@ -338,32 +356,28 @@ static bool __v4l2_async_notifier_fwnode_has_async_subdev(
> }
>
> /*
> - * Find out whether an async sub-device was set up for an fwnode already or
> + * Find out whether an async sub-device was set up already or
> * whether it exists in a given notifier before @this_index.
> */
> -static bool v4l2_async_notifier_fwnode_has_async_subdev(
> - struct v4l2_async_notifier *notifier, struct fwnode_handle *fwnode,
> +static bool v4l2_async_notifier_has_async_subdev(
> + struct v4l2_async_notifier *notifier, struct v4l2_async_subdev *asd,
> unsigned int this_index)
> {
> unsigned int j;
>
> lockdep_assert_held(&list_lock);
>
> - /* Check that an fwnode is not being added more than once. */
> + /* Check that an asd is not being added more than once. */
> for (j = 0; j < this_index; j++) {
> - struct v4l2_async_subdev *asd = notifier->subdevs[this_index];
> - struct v4l2_async_subdev *other_asd = notifier->subdevs[j];
> + struct v4l2_async_subdev *asd_y = notifier->subdevs[j];
>
> - if (other_asd->match_type == V4L2_ASYNC_MATCH_FWNODE &&
> - asd->match.fwnode ==
> - other_asd->match.fwnode)
> + if (asd_equal(asd, asd_y))
> return true;
> }
>
> - /* Check than an fwnode did not exist in other notifiers. */
> + /* Check that an asd does not exist in other notifiers. */
> list_for_each_entry(notifier, ¬ifier_list, list)
> - if (__v4l2_async_notifier_fwnode_has_async_subdev(
> - notifier, fwnode))
> + if (__v4l2_async_notifier_has_async_subdev(notifier, asd))
> return true;
>
> return false;
> @@ -392,12 +406,11 @@ static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
> case V4L2_ASYNC_MATCH_CUSTOM:
> case V4L2_ASYNC_MATCH_DEVNAME:
> case V4L2_ASYNC_MATCH_I2C:
> - break;
> case V4L2_ASYNC_MATCH_FWNODE:
> - if (v4l2_async_notifier_fwnode_has_async_subdev(
> - notifier, asd->match.fwnode, i)) {
> + if (v4l2_async_notifier_has_async_subdev(
> + notifier, asd, i)) {
> dev_err(dev,
> - "fwnode has already been registered or in notifier's subdev list\n");
> + "asd has already been registered or in notifier's subdev list\n");
Please, never use "asd" on messages printed to the user. While someone
may understand it while reading the source code, for a poor use,
"asd" is just a random sequence of 3 characters.
> ret = -EEXIST;
> goto err_unlock;
> }
Thanks,
Mauro
next prev parent reply other threads:[~2018-09-24 17:06 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1531175957-1973-1-git-send-email-steve_longerbeam@mentor.com>
2018-07-09 22:39 ` [PATCH v6 01/17] media: v4l2-fwnode: ignore endpoints that have no remote port parent Steve Longerbeam
2018-07-09 22:39 ` [PATCH v6 02/17] media: v4l2: async: Allow searching for asd of any type Steve Longerbeam
2018-09-24 17:06 ` Mauro Carvalho Chehab [this message]
2018-09-25 21:04 ` Steve Longerbeam
2018-09-25 22:20 ` Mauro Carvalho Chehab
2018-09-26 1:05 ` Steve Longerbeam
2018-09-26 9:33 ` Mauro Carvalho Chehab
2018-09-26 10:40 ` Sakari Ailus
2018-09-26 17:49 ` Steve Longerbeam
2018-09-28 12:16 ` Sakari Ailus
2018-09-29 17:40 ` Steve Longerbeam
2018-07-09 22:39 ` [PATCH v6 03/17] media: v4l2: async: Add v4l2_async_notifier_add_subdev Steve Longerbeam
2018-08-03 15:13 ` jacopo mondi
2018-08-04 16:39 ` Steve Longerbeam
2018-07-09 22:39 ` [PATCH v6 04/17] media: v4l2: async: Add convenience functions to allocate and add asd's Steve Longerbeam
2018-07-09 22:39 ` [PATCH v6 05/17] media: v4l2-fwnode: Switch to v4l2_async_notifier_add_subdev Steve Longerbeam
2018-07-09 22:39 ` [PATCH v6 06/17] media: v4l2-fwnode: Add a convenience function for registering subdevs with notifiers Steve Longerbeam
2018-09-13 10:37 ` jacopo mondi
2018-09-13 12:44 ` Sakari Ailus
2018-09-13 12:58 ` jacopo mondi
2018-09-14 0:57 ` Steve Longerbeam
2018-07-09 22:39 ` [PATCH v6 07/17] media: platform: video-mux: Register a subdev notifier Steve Longerbeam
2018-07-09 22:39 ` [PATCH v6 08/17] media: imx: csi: " Steve Longerbeam
2018-07-09 22:39 ` [PATCH v6 09/17] media: imx: mipi csi-2: " Steve Longerbeam
2018-07-09 22:39 ` [PATCH v6 10/17] media: staging/imx: of: Remove recursive graph walk Steve Longerbeam
2018-07-09 22:39 ` [PATCH v6 11/17] media: staging/imx: Loop through all registered subdevs for media links Steve Longerbeam
2018-07-09 22:39 ` [PATCH v6 12/17] media: staging/imx: Rename root notifier Steve Longerbeam
2018-07-09 22:39 ` [PATCH v6 13/17] media: staging/imx: Switch to v4l2_async_notifier_add_*_subdev Steve Longerbeam
2018-07-09 22:39 ` [PATCH v6 14/17] media: staging/imx: TODO: Remove one assumption about OF graph parsing Steve Longerbeam
2018-07-09 22:39 ` [PATCH v6 16/17] media: v4l2: async: Remove notifier subdevs array Steve Longerbeam
2018-07-23 12:35 ` Sakari Ailus
2018-07-23 16:44 ` Steve Longerbeam
2018-07-23 17:24 ` Sakari Ailus
2018-08-04 10:33 ` jacopo mondi
2018-08-04 17:20 ` Steve Longerbeam
2018-07-09 22:39 ` [PATCH v6 17/17] [media] v4l2-subdev.rst: Update doc regarding subdev descriptors Steve Longerbeam
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180924140604.23e2b56f@coco.lan \
--to=mchehab+samsung@kernel.org \
--cc=hans.verkuil@cisco.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=niklas.soderlund+renesas@ragnatech.se \
--cc=sakari.ailus@linux.intel.com \
--cc=slongerbeam@gmail.com \
--cc=sre@kernel.org \
--cc=steve_longerbeam@mentor.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).