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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Wed, Feb 2, 2022 at 12:19 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxxxxx>
>
> A few comments below.
>
> > Reported-by: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>
> > Signed-off-by: Bartosz Golaszewski <brgl@xxxxxxxx>
> > ---
> >  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.

I think it looks better. Some docs use it some don't, there's no standard.

>
> > + * @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
>
>

Fixed the rest. Thanks!

Bart