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

[Linus Walleij <linus.walleij@linaro.org>]

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.

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.

Yours,
Linus Walleij