[U-Boot] [PATCH] dm: gpio: handle GPIO_ACTIVE_LOW flag in DT

[Simon Glass]

Hi,

On 4 April 2016 at 11:50, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 04/03/2016 08:07 AM, Eric Nelson wrote:
>>
>> Hi Stephen and Peng,
>>
>> On 04/02/2016 08:37 PM, Stephen Warren wrote:
>>>
>>> On 04/02/2016 09:13 AM, Eric Nelson wrote:
>>>>
>>>> On 04/01/2016 10:46 PM, Peng Fan wrote:
>>>>>
>>>>> On Thu, Mar 31, 2016 at 01:41:04PM -0700, Eric Nelson wrote:
>>>>>>
>>>>>> On 03/28/2016 09:57 PM, Peng Fan wrote:
>>>>>>>
>>>>>>> On Fri, Mar 25, 2016 at 01:12:11PM -0700, Eric Nelson wrote:
>>>>>>>>
>>>>>>>> Device tree parsing of GPIO nodes is currently ignoring flags.
>>>>>>>>
>>>>>>>> Add support for GPIO_ACTIVE_LOW by checking for the presence
>>>>>>>> of the flag and setting the desc->flags field to the driver
>>>>>>>> model constant GPIOD_ACTIVE_LOW.
>>>>>>>
>>>>>>>
>>>>>>> You may need to try this: https://patchwork.ozlabs.org/patch/597363/
>>>>>>>
>>>>>> Thanks for pointing this out.
>>>>>>
>>>>>> This patch also works, but it has me confused.
>>>>>>
>>>>>> How/why is parsing the ACTIVE_LOW flag specific to MXC?
>>>>>>
>>>>>> This is a general-purpose flag in the kernel, not something machine-
>>>>>> specific.
>>>>>>
>>>>>> It also appears that there are a bunch of other copies
>>>>>> of this same bit of code in the various mach_xlate() routines:
>>>>>>
>>>>>> desc->flags = args->args[1] & GPIO_ACTIVE_LOW ? GPIOD_ACTIVE_LOW : 0;
>>>>>>
>>>>>> If it's done in gpio-uclass, this isn't needed and xlate can
>>>>>> be removed from mxc-gpio and quite a few other architectures.
>>>>>>
>>>>>> Please advise,
>>>>>
>>>>>
>>>>> I saw you have posted a patch set to convert other gpio drivers.
>>>>> But actually the translation of gpio property should be done by
>>>>> each gpio driver. Alought we have gpio-cells=<2> for most gpio
>>>>> drivers, but if there is one case that gpio-cells=<3>, and it have
>>>>> different meaning for each cell from other most drivers?
>>>>
>>>>
>>>> Which case has gpio-cells=<3>?
>>>>
>>>> As far as I can tell, only tegra and sandbox have something other
>>>> than parsing of offset and the GPIO_ACTIVE_LOW flag.
>>>>
>>>> Tegra seems to have gpio-cells=<2> and sandbox has either 0 or 1.
>>>>
>>>>> So I suggest we follow the linux way,
>>>>>
>>>>> 434         if (!chip->of_xlate) {
>>>>> 435                 chip->of_gpio_n_cells = 2;
>>>>> 436                 chip->of_xlate = of_gpio_simple_xlate;
>>>>> 437         }
>>>>>
>>>>> If gpio drivers does not provide xlate function, then use a simple
>>>>> xlate
>>>>> function. If gpio drivers have their own xlate function, then use their
>>>>> own way.
>>>>
>>>>
>>>> The recommendation in device-tree-bindings/gpio/gpio.txt is to have
>>>> the GPIO_ACTIVE_LOW/HIGH flag in the second cell, so parsing that
>>>> directly in gpio_find_and_xlate() seems right.
>>>
>>>
>>> That's a recommendation when designing GPIO controller bindings, not a
>>> definition of something that is categorically true for all bindings.
>>> Each binding is free to represent its flags (if any) in whatever way it
>>> wants, and so as Peng says, each driver has be in full control over its
>>> own of_xlate functionality. Now, for drivers that happen to use a common
>>> flag representation, we can plug in a common implementation of the xlate
>>> function.
>>
>>
>> Isn't that what this patch-set does?
>>         http://lists.denx.de/pipermail/u-boot/2016-April/250228.html
>
>
> No, I don't believe so. Rather, it forcibly decodes the common layout in the
> common code, in such a way that it's always used. Then, the driver-specific
> of_xlate is called, which could fix up (i.e. undo) the incorrect results if
> they weren't appropriate, and then apply the correct translation.
>
> Better would be to never decode the results incorrectly in the first place,
> yet allow each driver to use the common decoder function if it's
> appropriate.
>
> gpio_find_and_xlate() should do either exactly:
>
> a)
>
> return ops->xlate(desc->dev, desc, args);
>
> In this case, .xlate could never be NULL, and all drivers would need to
> provide some implementation. We could provide a common implementation
> gpio_xlate_common() that all drivers (that use the common DT specifier
> layout) can plug into their .xlate pointer.
>
> b)
>
> if (ops->xlate)
>     return ops->xlate(desc->dev, desc, args);
> else
>     return gpio_xlate_common(desc->dev, desc, args);
>
> Making that else clause call a separate function allows any custom
> ops->xlate implementation to call it too, assuming the code there is valid
> but simply needs extension rather than completely custom replacement.
>
>> For the cost of a couple of lines of code in gpio-uclass, we remove
>> ~50 lines from existing implementations, essentially allowing them
>> to use the common (or default) implementation. Nothing in the patch
>> prevents completely custom handling by a driver.
>>
>> If we want to be pedantic about requiring each driver to have function
>> xlate, then we should get rid of gpio_find_and_xlate entirely from
>> gpio-uclass.c.
>
>
> The intent of the change is good.
>
> I'm not sure why we need to remove gpio_find_and_xlate(); it provides an API
> for clients so they don't need to know how to access driver functionality
> through the ops pointer, which I think is an internal/private implementation
> detail. Is that detail exposed to clients in other places? If so, removing
> the wrapper seems fine. If not, I suspect it's a deliberate abstraction.

This seems a bit pedantic, but since Linux does it this way I think we
should follow along.

Eric you still get to remove the code from all the GPIO drivers - the
difference is just creating a common function to call when no xlate()
method is available.

Can you please take a look at what Stephen suggests?

Regards,
Simon