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

[Koichiro Den <koichiro.den@xxxxxxxxxxxxx>]

On Wed, Feb 12, 2025 at 03:44:46PM GMT, Geert Uytterhoeven wrote:
> 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

You're absolutely right, I'll fix this commit message, along with the
needless codes you mentioned below.

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

Yes. This is unreasonable. I'll fix this in v3. Thanks.

> 
> > +               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?

Thanks for pointing it out. It was just my oversight. I'll drop the
needless parts in v3.

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

Will fix in v3.

Thank you!

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