Re: [PATCH v5 11/13] media: v4l2-async: Discourage use of v4l2_async_notifier_add_subdev

[Mauro Carvalho Chehab <mchehab+huawei@kernel.org>]

Em Tue,  2 Feb 2021 15:56:09 +0200
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> From: Ezequiel Garcia <ezequiel@collabora.com>
> 
> Most -if not all- use-cases are expected to be covered by one of:
> v4l2_async_notifier_add_fwnode_subdev,
> v4l2_async_notifier_add_fwnode_remote_subdev or
> v4l2_async_notifier_add_i2c_subdev.

Actually, all cases outside V4L2 core:

$ git grep v4l2_async_notifier_add_subdev
Documentation/driver-api/media/v4l2-subdev.rst:using the :c:func:`v4l2_async_notifier_add_subdev` call. This function
drivers/media/v4l2-core/v4l2-async.c:int v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
drivers/media/v4l2-core/v4l2-async.c:EXPORT_SYMBOL_GPL(v4l2_async_notifier_add_subdev);
drivers/media/v4l2-core/v4l2-async.c:   ret = v4l2_async_notifier_add_subdev(notifier, asd);
drivers/media/v4l2-core/v4l2-async.c:   ret = v4l2_async_notifier_add_subdev(notifier, asd);
drivers/media/v4l2-core/v4l2-fwnode.c:  ret = v4l2_async_notifier_add_subdev(notifier, asd);
include/media/v4l2-async.h: * before the first call to @v4l2_async_notifier_add_subdev.
include/media/v4l2-async.h: * v4l2_async_notifier_add_subdev - Add an async subdev to the
include/media/v4l2-async.h:int v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
include/media/v4l2-async.h: * @v4l2_async_notifier_add_subdev,


> 
> We'd like to discourage drivers from using v4l2_async_notifier_add_subdev,
> so rename it as __v4l2_async_notifier_add_subdev. This is
> typically a good hint for drivers to avoid using the function.

IMO, the best here would be to create a header file:

	drivers/media/v4l2-core/v4l2-async-priv.h

and move v4l2_async_notifier_add_subdev from include/media/v4l2-async.h.

This will warrant that only the V4L2 core would have access to it.
Also, it is a lot better than adding something like this:

> + * \warning: Drivers should avoid using this function and instead use one of:
> + * @v4l2_async_notifier_add_fwnode_subdev,
> + * @v4l2_async_notifier_add_fwnode_remote_subdev or
> + * @v4l2_async_notifier_add_i2c_subdev.
> + *


Please submit a followup patch.

-

On a separate but related note, The names of the async notifiers are
too big, causing lots of coding style warnings, like:

+       asd = v4l2_async_notifier_add_fwnode_remote_subdev(
+               &fmd->subdev_notifier, of_fwnode_handle(ep), sizeof(*asd));
...
+                       asd = v4l2_async_notifier_add_fwnode_remote_subdev(
+                               &isp->notifier, ep, sizeof(*isd));

Ending a line with an open parenthesis is not natural: you won't see
any good written English texts (or on any other natural language) that would
have a line ending with a '('.

While I understand that the name describes precisely what those 
functions, such non-intuitive usage of parenthesis makes the line
obfuscated, for no good reason.

Also, the entire meaning of the V4L2 async API is to allow subdevs
to be registered later. So, IMHO, the namespace for those 3
calls could be simplified to:

	v4l2_async_notifier_add_fwnode(),
	v4l2_async_notifier_add_fwnode_remote()
	v4l2_async_notifier_add_i2c().

Also, we should place at least the first argument afer the
parenthesis, even if this would violate the 80 columns
coding style rule[1]. 

So, the above examples would be written as:


        asd = v4l2_async_notifier_add_fwnode_remote(&fmd->subdev_notifier,
						    of_fwnode_handle(ep),
						    sizeof(*asd));

and:

                        asd = v4l2_async_notifier_add_fwnode_remote(&isp->notifier,
								    ep,
								    sizeof(*isd));

Which better matches the Kernel coding style, and it is way easier to
review, as the brain will see the parenthesis just like it would on
a natural language.

[1] The 80 columns warning is a "soft" coding style violation. It serves
as a reference that either the function code is becoming too complex with too
many loops, or that the function names are becoming too big. As it produces
lots of false positives, and people were breaking strings or finishing
lines with open parenthesis, this rule got relaxed, and checkpatch now
warns only (by default) if the line has more than 100 columns.


> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Helen Koike <helen.koike@collabora.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/v4l2-core/v4l2-async.c  | 8 ++++----
>  drivers/media/v4l2-core/v4l2-fwnode.c | 2 +-
>  include/media/v4l2-async.h            | 9 +++++++--
>  3 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index ed603ae63cad..d848db962dc7 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -611,7 +611,7 @@ void v4l2_async_notifier_cleanup(struct v4l2_async_notifier *notifier)
>  }
>  EXPORT_SYMBOL_GPL(v4l2_async_notifier_cleanup);
>  
> -int v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
> +int __v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
>  				   struct v4l2_async_subdev *asd)
>  {
>  	int ret;
> @@ -628,7 +628,7 @@ int v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
>  	mutex_unlock(&list_lock);
>  	return ret;
>  }
> -EXPORT_SYMBOL_GPL(v4l2_async_notifier_add_subdev);
> +EXPORT_SYMBOL_GPL(__v4l2_async_notifier_add_subdev);
>  
>  struct v4l2_async_subdev *
>  v4l2_async_notifier_add_fwnode_subdev(struct v4l2_async_notifier *notifier,
> @@ -645,7 +645,7 @@ v4l2_async_notifier_add_fwnode_subdev(struct v4l2_async_notifier *notifier,
>  	asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
>  	asd->match.fwnode = fwnode_handle_get(fwnode);
>  
> -	ret = v4l2_async_notifier_add_subdev(notifier, asd);
> +	ret = __v4l2_async_notifier_add_subdev(notifier, asd);
>  	if (ret) {
>  		fwnode_handle_put(fwnode);
>  		kfree(asd);
> @@ -695,7 +695,7 @@ v4l2_async_notifier_add_i2c_subdev(struct v4l2_async_notifier *notifier,
>  	asd->match.i2c.adapter_id = adapter_id;
>  	asd->match.i2c.address = address;
>  
> -	ret = v4l2_async_notifier_add_subdev(notifier, asd);
> +	ret = __v4l2_async_notifier_add_subdev(notifier, asd);
>  	if (ret) {
>  		kfree(asd);
>  		return ERR_PTR(ret);
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index c1c2b3060532..63be16c8eb83 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -822,7 +822,7 @@ v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
>  	if (ret < 0)
>  		goto out_err;
>  
> -	ret = v4l2_async_notifier_add_subdev(notifier, asd);
> +	ret = __v4l2_async_notifier_add_subdev(notifier, asd);
>  	if (ret < 0) {
>  		/* not an error if asd already exists */
>  		if (ret == -EEXIST)
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index 8815e233677e..b113329582ff 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -133,17 +133,22 @@ void v4l2_async_debug_init(struct dentry *debugfs_dir);
>  void v4l2_async_notifier_init(struct v4l2_async_notifier *notifier);
>  
>  /**
> - * v4l2_async_notifier_add_subdev - Add an async subdev to the
> + * __v4l2_async_notifier_add_subdev - Add an async subdev to the
>   *				notifier's master asd list.
>   *
>   * @notifier: pointer to &struct v4l2_async_notifier
>   * @asd: pointer to &struct v4l2_async_subdev
>   *
> + * \warning: Drivers should avoid using this function and instead use one of:
> + * @v4l2_async_notifier_add_fwnode_subdev,
> + * @v4l2_async_notifier_add_fwnode_remote_subdev or
> + * @v4l2_async_notifier_add_i2c_subdev.
> + *

The markups here are violating kernel-doc. Functions should be declared
as:

    * v4l2_async_notifier_add_fwnode_subdev(),
    * v4l2_async_notifier_add_fwnode_remote_subdev() or
    * v4l2_async_notifier_add_i2c_subdev().

Please address it on a followup patch.


>   * Call this function before registering a notifier to link the provided @asd to
>   * the notifiers master @asd_list. The @asd must be allocated with k*alloc() as
>   * it will be freed by the framework when the notifier is destroyed.
>   */
> -int v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
> +int __v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
>  				   struct v4l2_async_subdev *asd);
>  
>  /**



Thanks,
Mauro