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

[Koichiro Den <koichiro.den@xxxxxxxxxxxxx>]

On Tue, Feb 04, 2025 at 11:41:16PM GMT, Koichiro Den wrote:
> On Tue, Feb 04, 2025 at 01:48:49PM GMT, Bartosz Golaszewski wrote:
> > On Mon, Feb 3, 2025 at 4:12 AM Koichiro Den <koichiro.den@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > > ---
> > >  drivers/gpio/gpio-aggregator.c | 549 ++++++++++++++++++++++++++++++++-
> > >  1 file changed, 548 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
> > > index 570cd1ff8cc2..c63cf3067ce7 100644
> > > --- a/drivers/gpio/gpio-aggregator.c
> > > +++ b/drivers/gpio/gpio-aggregator.c
> > > @@ -9,10 +9,14 @@
> > >
> > >  #include <linux/bitmap.h>
> > >  #include <linux/bitops.h>
> > > +#include <linux/completion.h>
> > > +#include <linux/configfs.h>
> > >  #include <linux/ctype.h>
> > >  #include <linux/delay.h>
> > >  #include <linux/idr.h>
> > >  #include <linux/kernel.h>
> > > +#include <linux/list.h>
> > > +#include <linux/lockdep.h>
> > >  #include <linux/mod_devicetable.h>
> > >  #include <linux/module.h>
> > >  #include <linux/mutex.h>
> > > @@ -34,11 +38,39 @@
> > >   */
> > >
> > >  struct gpio_aggregator {
> > > +       struct config_group group;
> > >         struct gpiod_lookup_table *lookups;
> > >         struct platform_device *pdev;
> > > +       struct mutex lock;
> > > +       int id;
> > > +
> > > +       /* Synchronize with probe */
> > > +       struct notifier_block bus_notifier;
> > > +       struct completion probe_completion;
> > > +       bool driver_bound;
> > > +
> > > +       /* List of gpio_aggregator_line. Always added in order */
> > > +       struct list_head list_head;
> > > +
> > >         char args[];
> > >  };
> > >
> > > +struct gpio_aggregator_line {
> > > +       struct config_group group;
> > > +       struct gpio_aggregator *parent;
> > > +       struct list_head entry;
> > > +
> > > +       /* Line index within the aggregator device */
> > > +       int idx;
> > > +
> > > +       /* GPIO chip label or line name */
> > > +       char *key;
> > > +       /* Can be negative to indicate lookup by line name */
> > > +       int offset;
> > > +
> > > +       enum gpio_lookup_flags flags;
> > > +};
> > > +
> > >  static DEFINE_MUTEX(gpio_aggregator_lock);     /* protects idr */
> > >  static DEFINE_IDR(gpio_aggregator_idr);
> > >
> > > @@ -61,6 +93,97 @@ static int aggr_add_gpio(struct gpio_aggregator *aggr, const char *key,
> > >         return 0;
> > >  }
> > >
> > > +static int aggr_notifier_call(struct notifier_block *nb, unsigned long action,
> > > +                             void *data)
> > > +{
> > > +       struct gpio_aggregator *aggr;
> > > +       struct device *dev = data;
> > > +
> > > +       aggr = container_of(nb, struct gpio_aggregator, bus_notifier);
> > > +       if (!device_match_name(dev, aggr->lookups->dev_id))
> > > +               return NOTIFY_DONE;
> > > +
> > > +       switch (action) {
> > > +       case BUS_NOTIFY_BOUND_DRIVER:
> > > +               aggr->driver_bound = true;
> > > +               break;
> > > +       case BUS_NOTIFY_DRIVER_NOT_BOUND:
> > > +               aggr->driver_bound = false;
> > > +               break;
> > > +       default:
> > > +               return NOTIFY_DONE;
> > > +       }
> > > +
> > > +       complete(&aggr->probe_completion);
> > > +       return NOTIFY_OK;
> > > +}
> > 
> > Suggestion: this is the third time we're seeing this mechanism being
> > used (after gpio-sim and gpio-virtuser). Maybe it's time to try and
> > abstract it as much of the code is shared?
> 
> That makes sense. I would add gpiolib-vgpio.[ch] and add an opaque
> * struct vgpio_common
> and two api functions as our first step:
> * vgpio_init(get_devname_cb) # bus notifier calls the cb to get devname
> * vgpio_register_pdev_sync(pdevinfo) # return -ENXIO when probe fails
> What do you think?

The previous comment was a bit too rough, sorry. I feel it's not a good
idea to write down detailed design plan here, so I'm thinking of having it
reviewed in v3. Please disregard the above comment.
I also noticed that this v2 patch series lacks 'select CONFIGFS_FS' for
CONFIG_GPIO_AGGREGATOR. I'll add it in v3.

In any case, I'll wait for further progress in the discussion at:
https://lore.kernel.org/all/CAMRc=Meb633zVgemPSeNtnm8oJmk=njcr2CQQbD5UJd=tBC5Zg@xxxxxxxxxxxxxx/

Thanks for the review.

Koichiro

> 
> Thanks,
> Koichiro
> 
> > 
> > The rest looks good to me.
> > 
> > Bart