Re: [PATCH] gpiolib: make struct comments into real kernel docs

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Feb 02, 2022 at 11:49:37AM +0100, Bartosz Golaszewski wrote:
> We have several comments that start with '/**' but don't conform to the
> kernel doc standard. Add proper detailed descriptions for the affected
> definitions and move the docs from the forward declarations to the
> struct definitions where applicable.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

A few comments below.

> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
> ---
>  drivers/gpio/gpiolib.h        | 31 +++++++++++++++++++++++++++++++
>  include/linux/gpio/consumer.h | 35 ++++++++++++++++-------------------
>  2 files changed, 47 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
> index 30bc3f80f83e..0025701b7641 100644
> --- a/drivers/gpio/gpiolib.h
> +++ b/drivers/gpio/gpiolib.h
> @@ -72,6 +72,20 @@ struct gpio_device {
>  /* gpio suffixes used for ACPI and device tree lookup */
>  static __maybe_unused const char * const gpio_suffixes[] = { "gpios", "gpio" };
>  
> +/**
> + * struct gpio_array - Opaque descriptor for a structure of GPIO array attributes

> + *

I dunno we need these blank lines.

> + * @desc:		Array of pointers to the GPIO descriptors
> + * @size:		Number of elements in desc

  in @desc

or

  in the array of the descriptor pointers

> + * @chip:		Parent GPIO chip
> + * @get_mask:		Get mask used in fastpath.
> + * @set_mask:		Set mask used in fastpath.
> + * @invert_mask:	Invert mask used in fastpath.

Why some of the descriptions are with grammar period and some w/o?

> + *
> + * This structure is attached to struct gpiod_descs obtained from
> + * gpiod_get_array() and can be passed back to get/set array functions in order
> + * to activate fast processing path if applicable.
> + */
>  struct gpio_array {
>  	struct gpio_desc	**desc;
>  	unsigned int		size;
> @@ -96,6 +110,23 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
>  extern spinlock_t gpio_lock;
>  extern struct list_head gpio_devices;
>  
> +
> +/**
> + * struct gpio_desc - Opaque descriptor for a GPIO
> + *
> + * @gdev:		Pointer to the parent GPIO device
> + * @flags:		Binary descriptor flags
> + * @label:		Name of the consumer
> + * @name:		Line name
> + * @hog:		Pointer to the device node that hogs this line (if any)
> + * @debounce_period_us:	Debounce period in microseconds
> + *
> + * These are obtained using gpiod_get() and are preferable to the old
> + * integer-based handles.
> + *
> + * Contrary to integers, a pointer to a gpio_desc is guaranteed to be valid

&struct gpio_desc

> + * until the GPIO is released.
> + */
>  struct gpio_desc {
>  	struct gpio_device	*gdev;
>  	unsigned long		flags;
> diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
> index 3ad67b4a72be..3ffb054fafbd 100644
> --- a/include/linux/gpio/consumer.h
> +++ b/include/linux/gpio/consumer.h
> @@ -8,27 +8,16 @@
>  #include <linux/err.h>
>  
>  struct device;
> -
> -/**
> - * Opaque descriptor for a GPIO. These are obtained using gpiod_get() and are
> - * preferable to the old integer-based handles.
> - *
> - * Contrary to integers, a pointer to a gpio_desc is guaranteed to be valid
> - * until the GPIO is released.
> - */
>  struct gpio_desc;
> -
> -/**
> - * Opaque descriptor for a structure of GPIO array attributes.  This structure
> - * is attached to struct gpiod_descs obtained from gpiod_get_array() and can be
> - * passed back to get/set array functions in order to activate fast processing
> - * path if applicable.
> - */
>  struct gpio_array;
>  
>  /**
> - * Struct containing an array of descriptors that can be obtained using
> - * gpiod_get_array().
> + * struct gpio_descs - Struct containing an array of descriptors that can be
> + *                     obtained using gpiod_get_array()
> + *
> + * @info:	Pointer to the opaque gpio_array structure
> + * @ndescs:	Number of held descriptors
> + * desc:	Array of pointers to GPIO descriptors

Missed @?

>   */
>  struct gpio_descs {
>  	struct gpio_array *info;
> @@ -43,8 +32,16 @@ struct gpio_descs {
>  #define GPIOD_FLAGS_BIT_NONEXCLUSIVE	BIT(4)
>  
>  /**
> - * Optional flags that can be passed to one of gpiod_* to configure direction
> - * and output value. These values cannot be OR'd.
> + * enum gpiod_flags - Optional flags that can be passed to one of gpiod_* to
> + *                    configure direction and output value. These values
> + *                    cannot be OR'd.
> + *
> + * @GPIOD_ASIS:			Don't change the direction

Don't change anything

(Not only direction, also the output value. Some hardware allows to change
output buffer value even if the line is configured as input).

> + * @GPIOD_IN:			Set lines to input mode
> + * @GPIOD_OUT_LOW:		Set lines to output and drive them low
> + * @GPIOD_OUT_HIGH:		Set lines to output and drive them high
> + * @GPIOD_OUT_LOW_OPEN_DRAIN:	Set lines to open-drain output and drive them low
> + * @GPIOD_OUT_HIGH_OPEN_DRAIN:	Set lines to open-drain output and drive them high
>   */
>  enum gpiod_flags {
>  	GPIOD_ASIS	= 0,
> -- 
> 2.30.1
> 

-- 
With Best Regards,
Andy Shevchenko