Re: [PATCH 3/4] gpiolib: Add support for GPIO "hog" nodes

[Nikita Yushchenko <nikita.yoush@cogentembedded.com>]

24.05.2017 02:25, Andrey Smirnov wrote:
> On Mon, May 22, 2017 at 11:52 PM, Nikita Yushchenko
> <nikita.yoush@cogentembedded.com> wrote:
>>> +     ret = of_property_read_u32(chip_np, "#gpio-cells", &gpio_cells);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     if (WARN_ON(gpio_cells != 2))
>>> +             return -ENOTSUPP;
>>> +
>>> +     ret = of_property_read_u32_index(np, "gpios", idx * gpio_cells,
>>> +                                      &gpio_num);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     ret = of_property_read_u32_index(np, "gpios", idx * gpio_cells + 1,
>>> +                                      &gpio_flags);
>>> +     if (ret)
>>> +             return ret;
>>
>> Doesn't this hardcode interpretation of device tree words in gpio
>> specification - while this is intended to be gpio-provider specific and
>> that's why #gpio-cells exist?
>>
> 
> It does and yes that's my understanding of the purpose of #gpio-cells
> as well. The reason I did in such a primitive way was because
> Barebox's GPIO subsystem doesn't have any translation plumbing to be
> able to handle anything more than a simple one dimensional offset.
> Given the fact that of_get_named_gpio_flags() make similar assumption
> I thought that there are no real consumers of that functionality and
> left proper implementation as a future improvement that can be made
> once the need arises.

Maybe then at least make this [wrong] thing done in single place?  I.e.
extract relevant code from of_get_named_gpio_flags() into separate
routine and call it from two places?  (And add a comment there, that it
is a stub assuming dump representation)

>>> +static int of_gpiochip_scan_gpios(struct gpio_chip *chip)
>>
>> Not best choice of name for routine that scans hogs?
>>
>> (although I understand that it comes from linux counterpart)
>>
> 
> Eh, I don't have any strong opinion on this one, I am more than happy
> to rename it if you think there are better alternatives.

of_gpiochip_scan_hogs() ?


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox