Message-ID: <CAMRc=Mdim2aSBs+JsL8dECfG0Vvrvcq6CHaGHbBoVhNhSZn9Kg@mail.gmail.com>
Date: Thu, 30 Jan 2025 11:30:59 +0100
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Koichiro Den <koichiro.den@...onical.com>
Cc: linux-gpio@...r.kernel.org, geert+renesas@...der.be, 
	linus.walleij@...aro.org, maciej.borzecki@...onical.com, 
	linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 0/2] Introduce configfs-based interface for gpio-aggregator

On Wed, Jan 29, 2025 at 4:56 PM Koichiro Den <koichiro.den@...onical.com> 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!

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

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.

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. 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.

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

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

Powered by blists - more mailing lists