Re: [RFC PATCH 0/2] Introduce configfs-based interface for gpio-aggregator

[Koichiro Den <koichiro.den@xxxxxxxxxxxxx>]

On Thu, Jan 30, 2025 at 11:30:59AM GMT, Bartosz Golaszewski wrote:
> On Wed, Jan 29, 2025 at 4:56 PM Koichiro Den <koichiro.den@xxxxxxxxxxxxx> wrote:
> >
> > This RFC patch series proposes adding a configfs-based interface to
> > gpio-aggregator to address limitations in the existing 'new_device'
> > interface.
> >
> > The existing 'new_device' interface has several limitations:
> >
> >   #1. No way to determine when GPIO aggregator creation is complete.
> >   #2. No way to retrieve errors when creating a GPIO aggregator.
> >   #3. No way to trace a GPIO line of an aggregator back to its
> >       corresponding physical device.
> >   #4. The 'new_device' echo does not indicate which virtual gpiochip.<N>
> >       was created.
> >   #5. No way to assign names to GPIO lines exported through an aggregator.
> >
> > Although issues #1 to #3 could technically be resolved easily without
> > configfs, using configfs offers a streamlined, modern, and extensible
> > approach, especially since gpio-sim and gpio-virtuser already utilize
> > configfs.
> >
> > This RFC patch series includes two commits:
> >
> > * [PATCH 1/2] implements the configfs interface and resolves the above
> >   issues:
> >   - #1, Wait for probe completion using a platform bus notifier,
> >         in the same manner as gpio-virtuser.
> >   - #2, Introduce a 'live' attribute (like gpio-virtuser/gpio-sim),
> >         returning -ENXIO when probe fails.
> >   - #3, Structure configfs directories to clearly map virtual lines to
> >         physical ones.
> >   - #4, Add a read-only 'dev_name' attribute exposing the platform bus
> >         device name.
> >   - #5, Allow users to set custom line names via a 'name' attribute.
> >
> > * [PATCH 2/2] provides documentation on using the new interface.
> >
> >
> > Koichiro Den (2):
> >   gpio: aggregator: Introduce configfs interface
> >   Documentation: gpio: document configfs interface for gpio-aggregator
> >
> >  .../admin-guide/gpio/gpio-aggregator.rst      |  86 +++
> >  drivers/gpio/gpio-aggregator.c                | 673 +++++++++++++++++-
> >  2 files changed, 757 insertions(+), 2 deletions(-)
> >
> > --
> > 2.45.2
> >
> 
> Hi!

Hi, thank you for reviewing.

> 
> I love the idea! In fact I think I floated it in a discussion with
> Geert some time ago but never got around to working on it.
> 
> I just glanced at the code and it looks nice and clean. I'd love to
> see some more improvements like using a common prefix for all internal
> symbols but it can be addressed in a separate series.
> 
> I played a bit with the module and this is where I noticed some issues:
> 
> 1. The sysfs interface must keep on working. The same command that
> works with mainline, fails for me with your patch. There's no error
> propagated to user-space, write() returns success and I only see:
> 
> gpio-aggregator.0: probe with driver gpio-aggregator failed with error -12

It looks like the issue is caused by gpiochip_fwd_line_names(). I'll fix it.

> 
> in the kernel log.
> 
> 2. I couldn't verify that it's not the case already but the code does
> not suggest it: IMO devices created with sysfs should appear in
> configfs.

That makes sense, I'll add the implementation.

> 
> 3. I don't think the user should need to specify the number of lines
> to aggregate. That information should be automatically inferred from
> the number of lineX attributes they created instead. [...]

I agree that it's essentially unnecessary, but considering the current
state of gpio-sim's configfs, having the user set num_lines doesn't seem
too unnatural to me. What do you think?

> [...] Also: if I create
> a line attribute without setting num_lines, the driver just crashes.
> In fact it seems any discrepancy between the number of lines specified
> and the naming convention of the line attribute causes a crash.

My bad.. thanks for pointing it out.

> 
> 4. Writing 1 to live, when no lines to aggregate were specified, should fail.

Agreed, I'll address this.

> 
> There's probably more but I haven't had a lot of time.
> 
> In short: I'm very much in favor of adding this but it will require some work.
> 
> Thanks,
> Bartosz

I'd appreciate a response to one question above. I'll prepare v2 after.

Thanks again!

-Koichiro