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

[Andrey Smirnov <andrew.smirnov@gmail.com>]

On Tue, May 23, 2017 at 11:43 PM, Nikita Yushchenko
<nikita.yoush@cogentembedded.com> wrote:
>
>
> 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)
>

The code of the two doesn't have much, if anything, in common.
Of_get_named_gpio_flags is expecting a phandle to the gpio node be a
part of the field it parses, whereas gpio specifier in hog nodes omits
that. I don't think I can have any meaningful code sharing here.

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

Sure, I'll do that in v2.

Thanks,
Andrey Smirnov

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