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 <[email protected]>
---
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 */
+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)
{
- 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");
ret = -EEXIST;
goto err_unlock;
}
--
2.7.4
Em Mon, 9 Jul 2018 15:39:02 -0700
Steve Longerbeam <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>'
(the some comes with Signed-off-by: Steve Longerbeam <[email protected]>)
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
Hi Mauro,
On 09/24/2018 10:06 AM, Mauro Carvalho Chehab wrote:
> Em Mon, 9 Jul 2018 15:39:02 -0700
> Steve Longerbeam <[email protected]> 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 <[email protected]>
>> ---
>> 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
Ok, I will change the comment to read:
/* Compare two async subdevice descriptors for equivalence */
>
>> +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(
Will fix.
>
> 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 <[email protected]>'
>
> (the some comes with Signed-off-by: Steve Longerbeam <[email protected]>)
>
> I suspect that other patches on this series will suffer from the same issue.
Will fix when submitting v7.
> <snip>
>
>> @@ -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.
I will change the message to read:
"subdev descriptor already listed in this or other notifiers".
Steve
Em Tue, 25 Sep 2018 14:04:21 -0700
Steve Longerbeam <[email protected]> escreveu:
> Hi Mauro,
>
>
> On 09/24/2018 10:06 AM, Mauro Carvalho Chehab wrote:
> > Em Mon, 9 Jul 2018 15:39:02 -0700
> > Steve Longerbeam <[email protected]> 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 <[email protected]>
> >> ---
> >> 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
>
> Ok, I will change the comment to read:
>
> /* Compare two async subdevice descriptors for equivalence */
OK
>
> >
> >> +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(
>
> Will fix.
>
> >
> > 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 <[email protected]>'
> >
> > (the some comes with Signed-off-by: Steve Longerbeam <[email protected]>)
> >
> > I suspect that other patches on this series will suffer from the same issue.
>
> Will fix when submitting v7.
>
> > <snip>
> >
> >> @@ -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.
>
> I will change the message to read:
>
> "subdev descriptor already listed in this or other notifiers".
Perfect!
Thanks!
Mauro
On 09/25/2018 03:20 PM, Mauro Carvalho Chehab wrote:
> Em Tue, 25 Sep 2018 14:04:21 -0700
> Steve Longerbeam <[email protected]> escreveu:
>
>>>
>>>> @@ -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.
>> I will change the message to read:
>>
>> "subdev descriptor already listed in this or other notifiers".
> Perfect!
But the error message is removed in the subsequent patch
"[PATCH 03/17] media: v4l2: async: Add v4l2_async_notifier_add_subdev".
I could bring it back as a dev_dbg() in v4l2_async_notifier_asd_valid(), but
this shouldn't be a dev_err() anymore since it is up to the media platform
to decide whether an already existing subdev descriptor is an error.
Steve
Em Tue, 25 Sep 2018 18:05:36 -0700
Steve Longerbeam <[email protected]> escreveu:
> On 09/25/2018 03:20 PM, Mauro Carvalho Chehab wrote:
> > Em Tue, 25 Sep 2018 14:04:21 -0700
> > Steve Longerbeam <[email protected]> escreveu:
> >
> >>>
> >>>> @@ -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.
> >> I will change the message to read:
> >>
> >> "subdev descriptor already listed in this or other notifiers".
> > Perfect!
>
> But the error message is removed in the subsequent patch
> "[PATCH 03/17] media: v4l2: async: Add v4l2_async_notifier_add_subdev".
>
> I could bring it back as a dev_dbg() in v4l2_async_notifier_asd_valid(), but
> this shouldn't be a dev_err() anymore since it is up to the media platform
> to decide whether an already existing subdev descriptor is an error.
Hmm... that's an interesting discussion... what cases do you think it
would be fine to try to register twice an asd notifier?
Haven't write myself any piece of code using async framework, on a first
glance, trying to register twice sounds like an error to me.
Sakari, what do you think?
Thanks,
Mauro
Hi Mauro, Steve,
On Wed, Sep 26, 2018 at 06:33:35AM -0300, Mauro Carvalho Chehab wrote:
> Em Tue, 25 Sep 2018 18:05:36 -0700
> Steve Longerbeam <[email protected]> escreveu:
>
> > On 09/25/2018 03:20 PM, Mauro Carvalho Chehab wrote:
> > > Em Tue, 25 Sep 2018 14:04:21 -0700
> > > Steve Longerbeam <[email protected]> escreveu:
> > >
> > >>>
> > >>>> @@ -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.
> > >> I will change the message to read:
> > >>
> > >> "subdev descriptor already listed in this or other notifiers".
> > > Perfect!
> >
> > But the error message is removed in the subsequent patch
> > "[PATCH 03/17] media: v4l2: async: Add v4l2_async_notifier_add_subdev".
> >
> > I could bring it back as a dev_dbg() in v4l2_async_notifier_asd_valid(), but
> > this shouldn't be a dev_err() anymore since it is up to the media platform
> > to decide whether an already existing subdev descriptor is an error.
>
> Hmm... that's an interesting discussion... what cases do you think it
> would be fine to try to register twice an asd notifier?
Only the error message is removed; this case is still considered an error.
I think it'd be better to keep this error message; it helps debugging.
>
> Haven't write myself any piece of code using async framework, on a first
> glance, trying to register twice sounds like an error to me.
>
> Sakari, what do you think?
--
Regards,
Sakari Ailus
[email protected]
Hi Mauro, Sakari,
On 09/26/2018 03:40 AM, Sakari Ailus wrote:
> Hi Mauro, Steve,
>
> On Wed, Sep 26, 2018 at 06:33:35AM -0300, Mauro Carvalho Chehab wrote:
>> Em Tue, 25 Sep 2018 18:05:36 -0700
>> Steve Longerbeam <[email protected]> escreveu:
>>
>>> On 09/25/2018 03:20 PM, Mauro Carvalho Chehab wrote:
>>>> Em Tue, 25 Sep 2018 14:04:21 -0700
>>>> Steve Longerbeam <[email protected]> escreveu:
>>>>
>>>>>>
>>>>>>> @@ -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.
>>>>> I will change the message to read:
>>>>>
>>>>> "subdev descriptor already listed in this or other notifiers".
>>>> Perfect!
>>> But the error message is removed in the subsequent patch
>>> "[PATCH 03/17] media: v4l2: async: Add v4l2_async_notifier_add_subdev".
>>>
>>> I could bring it back as a dev_dbg() in v4l2_async_notifier_asd_valid(), but
>>> this shouldn't be a dev_err() anymore since it is up to the media platform
>>> to decide whether an already existing subdev descriptor is an error.
>> Hmm... that's an interesting discussion... what cases do you think it
>> would be fine to try to register twice an asd notifier?
It should be a fairly common case that a sub-device has multiple fwnode
output ports. In that case it's possible multiple sub-devices downstream
from it will each encounter it when parsing the fwnode graph, and attempt
to add it to their notifiers asd_list multiple times. That isn't an
error, any
attempt to add it after the first add should be ignored.
imx-media is an example, there is a CSI-2 transmitter with four fwnode
output ports for each CSI-2 virtual channel. Those channels each go to
one of four Camera Sensor Interface in the imx6 IPU. So each CSI will
encounter the CSI-2 transmitter when parsing its fwnode ports.
> Only the error message is removed; this case is still considered an error.
> I think it'd be better to keep this error message; it helps debugging.
Ok I will add it back, but it should be a dev_dbg().
Steve
>
>> Haven't write myself any piece of code using async framework, on a first
>> glance, trying to register twice sounds like an error to me.
>>
>> Sakari, what do you think?
On Wed, Sep 26, 2018 at 10:49:18AM -0700, Steve Longerbeam wrote:
> Hi Mauro, Sakari,
>
>
> On 09/26/2018 03:40 AM, Sakari Ailus wrote:
> > Hi Mauro, Steve,
> >
> > On Wed, Sep 26, 2018 at 06:33:35AM -0300, Mauro Carvalho Chehab wrote:
> > > Em Tue, 25 Sep 2018 18:05:36 -0700
> > > Steve Longerbeam <[email protected]> escreveu:
> > >
> > > > On 09/25/2018 03:20 PM, Mauro Carvalho Chehab wrote:
> > > > > Em Tue, 25 Sep 2018 14:04:21 -0700
> > > > > Steve Longerbeam <[email protected]> escreveu:
> > > > > > > > @@ -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.
> > > > > > I will change the message to read:
> > > > > >
> > > > > > "subdev descriptor already listed in this or other notifiers".
> > > > > Perfect!
> > > > But the error message is removed in the subsequent patch
> > > > "[PATCH 03/17] media: v4l2: async: Add v4l2_async_notifier_add_subdev".
> > > >
> > > > I could bring it back as a dev_dbg() in v4l2_async_notifier_asd_valid(), but
> > > > this shouldn't be a dev_err() anymore since it is up to the media platform
> > > > to decide whether an already existing subdev descriptor is an error.
> > > Hmm... that's an interesting discussion... what cases do you think it
> > > would be fine to try to register twice an asd notifier?
>
> It should be a fairly common case that a sub-device has multiple fwnode
> output ports. In that case it's possible multiple sub-devices downstream
> from it will each encounter it when parsing the fwnode graph, and attempt
> to add it to their notifiers asd_list multiple times. That isn't an error,
> any
> attempt to add it after the first add should be ignored.
>
> imx-media is an example, there is a CSI-2 transmitter with four fwnode
> output ports for each CSI-2 virtual channel. Those channels each go to
> one of four Camera Sensor Interface in the imx6 IPU. So each CSI will
> encounter the CSI-2 transmitter when parsing its fwnode ports.
>
>
> > Only the error message is removed; this case is still considered an error.
> > I think it'd be better to keep this error message; it helps debugging.
>
> Ok I will add it back, but it should be a dev_dbg().
Fine for me.
Could you address especially the author vs. SoB line difference in the set,
and re-post to the list, please?
I've pushed the latest set including my fwnode patches (which you can
ignore) to my linuxtv.org tree v4l2-fwnode branch; feel free to use these
as the basis. I've fixed a few conflicts in there in rebasing on current
media tree master.
--
Regards,
Sakari Ailus
e-mail: [email protected]
Hi Sakari,
On 09/28/2018 05:16 AM, Sakari Ailus wrote:
> On Wed, Sep 26, 2018 at 10:49:18AM -0700, Steve Longerbeam wrote:
>> Hi Mauro, Sakari,
>>
>>
>> On 09/26/2018 03:40 AM, Sakari Ailus wrote:
>>> Hi Mauro, Steve,
>>>
>>> On Wed, Sep 26, 2018 at 06:33:35AM -0300, Mauro Carvalho Chehab wrote:
>>>> Em Tue, 25 Sep 2018 18:05:36 -0700
>>>> Steve Longerbeam <[email protected]> escreveu:
>>>>
>>>>> On 09/25/2018 03:20 PM, Mauro Carvalho Chehab wrote:
>>>>>> Em Tue, 25 Sep 2018 14:04:21 -0700
>>>>>> Steve Longerbeam <[email protected]> escreveu:
>>>>>>>>> @@ -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.
>>>>>>> I will change the message to read:
>>>>>>>
>>>>>>> "subdev descriptor already listed in this or other notifiers".
>>>>>> Perfect!
>>>>> But the error message is removed in the subsequent patch
>>>>> "[PATCH 03/17] media: v4l2: async: Add v4l2_async_notifier_add_subdev".
>>>>>
>>>>> I could bring it back as a dev_dbg() in v4l2_async_notifier_asd_valid(), but
>>>>> this shouldn't be a dev_err() anymore since it is up to the media platform
>>>>> to decide whether an already existing subdev descriptor is an error.
>>>> Hmm... that's an interesting discussion... what cases do you think it
>>>> would be fine to try to register twice an asd notifier?
>> It should be a fairly common case that a sub-device has multiple fwnode
>> output ports. In that case it's possible multiple sub-devices downstream
>> from it will each encounter it when parsing the fwnode graph, and attempt
>> to add it to their notifiers asd_list multiple times. That isn't an error,
>> any
>> attempt to add it after the first add should be ignored.
>>
>> imx-media is an example, there is a CSI-2 transmitter with four fwnode
>> output ports for each CSI-2 virtual channel. Those channels each go to
>> one of four Camera Sensor Interface in the imx6 IPU. So each CSI will
>> encounter the CSI-2 transmitter when parsing its fwnode ports.
>>
>>
>>> Only the error message is removed; this case is still considered an error.
>>> I think it'd be better to keep this error message; it helps debugging.
>> Ok I will add it back, but it should be a dev_dbg().
> Fine for me.
>
> Could you address especially the author vs. SoB line difference in the set,
> and re-post to the list, please?
Will do!
>
> I've pushed the latest set including my fwnode patches (which you can
> ignore) to my linuxtv.org tree v4l2-fwnode branch; feel free to use these
> as the basis. I've fixed a few conflicts in there in rebasing on current
> media tree master.
Yes in drivers/media/platform/ti-vpe/cal.c, due to
58513d4849 (" media: platform: remove redundant null pointer check
before of_node_put")
I fixed that conflict too, but in a different way that retains the above
change.
Since my conflict fix is different from yours, I will push v7 against
current
media tree master. Let me know if the fixup looks ok to you.
Steve