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@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
