Re: [PATCH v2 05/10] gpio: aggregator: expose custom line names to forwarder gpio_chip

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Den-san,

Thanks for your patch!

On Mon, 3 Feb 2025 at 04:12, Koichiro Den <koichiro.den@xxxxxxxxxxxxx> wrote:
> Previously, GPIO lines in the aggregator had empty names. Now that the

That is only true for aggregators created through the sysfs interface,
right?  When created from DT, gpio-line-names is already supported:
https://elixir.bootlin.com/linux/v6.13.2/source/Documentation/admin-guide/gpio/gpio-aggregator.rst#L72

> configfs interface supports custom names, update the GPIO forwarder to
> use them.
>
> Signed-off-by: Koichiro Den <koichiro.den@xxxxxxxxxxxxx>

> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -425,6 +425,20 @@ static int gpiochip_fwd_setup_delay_line(struct device *dev, struct gpio_chip *c
>  }
>  #endif /* !CONFIG_OF_GPIO */
>
> +static int gpiochip_fwd_line_names(struct device *dev, const char **names, int len)
> +{
> +       int num = device_property_string_array_count(dev, "gpio-line-names");
> +       if (!num)
> +               return 0;
> +       if (num > len) {
> +               pr_warn("gpio-line-names contains %d lines while %d expected",
> +                       num, len);
> +               num = len;
> +       }
> +       return device_property_read_string_array(dev, "gpio-line-names",
> +                                                names, num);
> +}
> +
>  /**
>   * gpiochip_fwd_create() - Create a new GPIO forwarder
>   * @dev: Parent device pointer
> @@ -447,6 +461,7 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
>  {
>         const char *label = dev_name(dev);
>         struct gpiochip_fwd *fwd;
> +       const char **line_names;
>         struct gpio_chip *chip;
>         unsigned int i;
>         int error;
> @@ -458,6 +473,16 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
>
>         chip = &fwd->chip;
>
> +       if (!dev_of_node(dev)) {
> +               line_names = devm_kcalloc(dev, sizeof(*line_names), ngpios, GFP_KERNEL);

So this is always allocated, even when no names are specified?

> +               if (!line_names)
> +                       return ERR_PTR(-ENOMEM);
> +
> +               error = gpiochip_fwd_line_names(dev, line_names, ngpios);
> +               if (error < 0)
> +                       return ERR_PTR(-ENOMEM);
> +       }
> +
>         /*
>          * If any of the GPIO lines are sleeping, then the entire forwarder
>          * will be sleeping.
> @@ -491,6 +516,9 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
>         chip->ngpio = ngpios;
>         fwd->descs = descs;
>
> +       if (!dev_of_node(dev))
> +               chip->names = line_names;
> +

Do you actually need to collect the names yourself?
Below, the driver does:

     error = devm_gpiochip_add_data(dev, chip, fwd);

and gpiochip_add_data_with_key() already calls gpiochip_set_names()
to retrieve the names from the gpio-line-names property.
What is missing to make that work?

>         if (chip->can_sleep)
>                 mutex_init(&fwd->mlock);
>         else
> @@ -530,10 +558,40 @@ to_gpio_aggregator_line(struct config_item *item)
>         return container_of(group, struct gpio_aggregator_line, group);
>  }
>
> +static struct fwnode_handle *aggr_make_device_swnode(struct gpio_aggregator *aggr)
> +{
> +       char **line_names __free(kfree) = NULL;

const (needed when gpio_aggregator_line.name becomes const)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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