Re: [PATCH v2 02/10] gpio: aggregator: introduce basic configfs interface

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Den-san,

On Thu, 13 Feb 2025 at 15:13, Koichiro Den <koichiro.den@canonical.com> wrote:
> On Wed, Feb 12, 2025 at 02:48:12PM GMT, Geert Uytterhoeven wrote:
> > On Mon, 3 Feb 2025 at 04:12, Koichiro Den <koichiro.den@canonical.com> wrote:
> > > The existing sysfs 'new_device' interface has several limitations:
> > > * No way to determine when GPIO aggregator creation is complete.
> > > * No way to retrieve errors when creating a GPIO aggregator.
> > > * No way to trace a GPIO line of an aggregator back to its
> > >   corresponding physical device.
> > > * The 'new_device' echo does not indicate which virtual gpiochip<N>
> > >   was created.
> > > * No way to assign names to GPIO lines exported through an aggregator.
> > >
> > > Introduce the new configfs interface for gpio-aggregator to address
> > > these limitations. It provides a more streamlined, modern, and
> > > extensible configuration method. For backward compatibility, the
> > > 'new_device' interface and its behaviour is retained for now.
> > >
> > > This commit implements minimal functionalities:
> > >
> > >   /config/gpio-aggregator/<name-of-your-choice>/
> > >   /config/gpio-aggregator/<name-of-your-choice>/live
> > >   /config/gpio-aggregator/<name-of-your-choice>/<lineY>/
> > >   /config/gpio-aggregator/<name-of-your-choice>/<lineY>/key
> > >   /config/gpio-aggregator/<name-of-your-choice>/<lineY>/offset
> > >
> > > Basic setup flow is:
> > > 1. Create a directory for a GPIO aggregator.
> > > 2. Create subdirectories for each line you want to instantiate.
> > > 3. In each line directory, configure the key and offset.
> > >    The key/offset semantics are as follows:
> > >    * 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:
> > >      - key needs to specify the GPIO line name.
> > > 4. Return to the aggregator's root directory and write '1' to the live
> > >    attribute.
> > >
> > > For example, the command in the existing kernel doc:
> > >
> > >   echo 'e6052000.gpio 19 e6050000.gpio 20-21' > new_device
> > >
> > > is equivalent to:
> > >
> > >   mkdir /sys/kernel/config/gpio-aggregator/<custom-name>
> > >   # Change <custom-name> to name of your choice (e.g. "aggr0")
> > >   cd /sys/kernel/config/gpio-aggregator/<custom-name>
> > >   mkdir line0 line1 line2  # Only "line<Y>" naming allowed.
> > >   echo e6052000.gpio > line0/key
> > >   echo 19            > line0/offset
> > >   echo e6050000.gpio > line1/key
> > >   echo 20            > line1/offset
> > >   echo e6050000.gpio > line2/key
> > >   echo 21            > line2/offset
> > >   echo 1             > live
> > >
> > > Signed-off-by: Koichiro Den <koichiro.den@canonical.com>

> > > --- a/drivers/gpio/gpio-aggregator.c
> > > +++ b/drivers/gpio/gpio-aggregator.c

> > > +       /*
> > > +        * Undepend is required only if device disablement (live == 0)
> >
> > s/Undepend/Lock-up/?
>
> I must admit that I couldn't find a best suitable antonym to 'depend'.
> IMO Lock-up sounds a bit misleading. How about 'Unlock'?

I wrote "Lock-up" to match the "lockup" in aggr_lockup_configfs() below.
But now I understand why you wrote "Undepend": when passed "false",
aggr_lockup_configfs() calls configfs_undepend_item_unlocked(),
so "Undepend" is fine.

> > > +        * succeeds or if device enablement (live == 1) fails.
> > > +        */
> > > +       if (live == !!ret)
> > > +               aggr_lockup_configfs(aggr, false);
> > > +
> > > +       return ret ?: count;
> > > +}

> > > +static struct config_group *
> > > +gpio_aggr_make_group(struct config_group *group, const char *name)
> > > +{
> > > +       /* arg space is unneeded */
> > > +       struct gpio_aggregator *aggr = kzalloc(sizeof(*aggr), GFP_KERNEL);
> > > +       if (!aggr)
> > > +               return ERR_PTR(-ENOMEM);
> > > +
> > > +       mutex_lock(&gpio_aggregator_lock);
> > > +       aggr->id = idr_alloc(&gpio_aggregator_idr, aggr, 0, 0, GFP_KERNEL);
> > > +       mutex_unlock(&gpio_aggregator_lock);
> > > +
> > > +       if (aggr->id < 0) {
> > > +               kfree(aggr);
> > > +               return ERR_PTR(aggr->id);
> > > +       }
> >
> > The above more or less duplicates the existing code in
> > new_device_store().
>
> I'll factor out the common part and add new funcs gpio_alloc()/gpio_free().
> Please let me know if any objections.

Thanks, that would be fine!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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