Re: [PATCH v3] gpio: of: make it possible to name GPIO lines

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Tue, Apr 26, 2016 at 01:03:45PM +0200, Linus Walleij wrote:
> On Thu, Apr 21, 2016 at 7:06 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Thu, Apr 21, 2016 at 01:08:21PM +0200, Linus Walleij wrote:
> >>  /**
> >> + * of_gpiochip_set_names() - set up the names of the lines
> >> + * @chip: GPIO chip whose lines should be named, if possible
> >> + */
> >> +static void of_gpiochip_set_names(struct gpio_chip *gc)
> >> +{
> >> +     struct gpio_device *gdev = gc->gpiodev;
> >> +     struct device_node *np = gc->of_node;
> >> +     int i;
> >> +     int nstrings;
> >> +
> >> +     /* Do we even have the "gpio-line-names" property */
> >> +     if (!of_property_read_bool(np, "gpio-line-names"))
> >> +             return;
> >> +
> >> +     nstrings = of_property_count_strings(np, "gpio-line-names");
> >> +     /*
> >> +      * Make sure to not index beyond either the end of the
> >> +      * "gpio-names" array nor the number of descriptors of
> >> +      * the GPIO device.
> >> +      */
> >
> > I know you mentioned that it already been discussed much, but I am not
> > sure why we need to count the string (and validate that strings are
> > present by treating the property as boolean?),
> 
> I validate the property to be present to bail out quickly on controllers
> that have no names set (i.e. all currently deployed device trees).
> I can skip that part if you think it's too much optimization.

My main objection is use of of_property_read_bool() call to validate
presence of a property that is not really a boolean property.

But you are the maintainer so it is your decision.

> 
> However:
> 
> > I am not
> > sure why we need to count the string
> > when we could do
> > something like this (relying on the fact that
> > of_property_read_string_index() returns 0 or negative error, no positive
> > return codes):
> >
> >         for (i = 0; i < gdev->ngpio; i++) {
> >                 const char *name;
> >                 int error;
> >
> >                 error = of_property_read_string_index(np,
> >                                                       "gpio-line-names",
> >                                                       i,
> >                                                       &name);
> >                 if (error) {
> >                         if (error != -ENODATA)
> >                                 dev_err(&gdev->dev,
> >                                         "unable to name line %d: %d\n",
> >                                         i, error);
> >                         /*
> >                          * Either no more strings (-ENODATA), or
> >                          * other error, in any case we are done naming.
> >                          */
> >                         break;
> >                 }
> >
> >                 gdev->descs[i].name = name;
> >         }
> 
> Yeah I can do that, thanks I didn't look close enough at the semantics.
> 
> The big question whether the DT people are OK with naming producer
> names like this remains however.

Right, I was just commenting on the implementation.

Thanks.

-- 
Dmitry