Re: [PATCH v6 4/9] gpio: aggregator: introduce basic configfs interface

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Sat, Mar 15, 2025 at 5:41 PM 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 behavior is retained for now.
>
> This commit implements basic functionalities:
>
>   /config/gpio-aggregator/<name-of-your-choice>/
>   /config/gpio-aggregator/<name-of-your-choice>/live
>   /config/gpio-aggregator/<name-of-your-choice>/dev_name
>   /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
>   /config/gpio-aggregator/<name-of-your-choice>/<lineY>/name
>
> 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
>
> The corresponding gpio_device id can be identified as follows:
>
>   cd /sys/kernel/config/gpio-aggregator/<custom-name>
>   ls -d /sys/devices/platform/`cat dev_name`/gpiochip*
>
> Also, via configfs, custom GPIO line name can be set like this:
>
>   cd /sys/kernel/config/gpio-aggregator/<custom-name>
>   echo "abc"         > line1/name
>
> Signed-off-by: Koichiro Den <koichiro.den@xxxxxxxxxxxxx>
> ---
>
> @@ -90,6 +124,70 @@ static int aggr_add_gpio(struct gpio_aggregator *aggr, const char *key,
>         return 0;
>  }
>
> +static bool aggr_is_active(struct gpio_aggregator *aggr)

Series-wide: I would prefer a different prefix: why not
gpio_aggregator or at least gpio_aggr?

Other than that, looks good to me!

Bartosz