Re: [PATCH v2 10/10] Documentation: gpio: document configfs interface for gpio-aggregator

[Koichiro Den <koichiro.den@xxxxxxxxxxxxx>]

On Wed, Feb 12, 2025 at 04:55:24PM GMT, Geert Uytterhoeven wrote:
> Hi Den-san.
> 
> On Mon, 3 Feb 2025 at 04:14, Koichiro Den <koichiro.den@xxxxxxxxxxxxx> wrote:
> > Add documentation for the newly added configfs-based interface for GPIO
> > aggregator.
> >
> > Signed-off-by: Koichiro Den <koichiro.den@xxxxxxxxxxxxx>
> 
> Thanks for your patch!
> 
> > --- a/Documentation/admin-guide/gpio/gpio-aggregator.rst
> > +++ b/Documentation/admin-guide/gpio/gpio-aggregator.rst
> > @@ -69,6 +69,99 @@ write-only attribute files in sysfs.
> >                     $ echo gpio-aggregator.0 > delete_device
> >
> >
> > +Aggregating GPIOs using Configfs
> > +--------------------------------
> > +
> > +**Group:** ``/config/gpio-aggregator``
> > +
> > +    This is the root directory of the gpio-aggregator configfs tree.
> > +
> > +**Group:** ``/config/gpio-aggregator/<example-name>``
> > +
> > +    This directory represents a GPIO aggregator device. You can assign any
> > +    name to ``<example-name>`` (e.g., ``agg0``), except names starting with
> > +    ``_auto`` prefix, which are reserved for auto-generated configfs
> > +    entries corresponding to devices created via Sysfs.
> > +
> > +**Attribute:** ``/config/gpio-aggregator/<example-name>/live``
> > +
> > +    The ``live`` attribute allows to trigger the actual creation of the device
> > +    once it's fully configured. The accepted values are: ``1`` to enable the
> > +    virtual device and ``0`` to disable and tear it down.
> 
> As the code uses kstrtobool(), it accepts variants of
> yes/true/on/no/false/off, too.

Thanks for pointing that out. I'll modify this part.

> 
> > +
> > +**Attribute:** ``/config/gpio-aggregator/<example-name>/dev_name``
> > +
> > +    The read-only ``dev_name`` attribute exposes the name of the device as it
> > +    will appear in the system on the platform bus (e.g. ``gpio-aggregator.0``).
> > +    This is useful for identifying a character device for the newly created
> > +    aggregator. If it's ``gpio-aggregator.0``,
> > +    ``/sys/devices/platform/gpio-aggregator.0/gpiochipX`` path tells you that the
> > +    GPIO device id is ``X``.
> > +
> > +You must create subdirectories for each virtual line you want to
> > +instantiate, named exactly as ``line0``, ``line1``, ..., ``lineY``, when
> > +you want to instantiate ``Y+1`` (Y >= 0) lines.  Configure all lines before
> > +activating the device by setting ``live`` to 1.
> > +
> > +**Group:** ``/config/gpio-aggregator/<example-name>/<lineY>/``
> > +
> > +    This directory represents a GPIO line to include in the aggregator.
> > +
> > +**Attribute:** ``/config/gpio-aggregator/<example-name>/<lineY>/key``
> > +
> > +**Attribute:** ``/config/gpio-aggregator/<example-name>/<lineY>/offset``
> > +
> > +    If ``offset`` is >= 0:
> > +        * ``key`` specifies the name of the chip this GPIO belongs to
> > +        * ``offset`` specifies the line offset within that chip.
> > +    If ``offset`` is <0:
> 
> Missing space before 0.
> Please add "(default)", so the user knows he can skip writing to this
> file when specifying a GPIO line name.

That makes sense. Thanks for the review!

Koichiro

> 
> > +        * ``key`` needs to specify the GPIO line name.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds