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

[Stephen Warren]

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.