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

[Eric Nelson <eric@nelint.com>]

Hi Peng,

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 way, driver-specific parsing only needs to handle additional
flags or needs as is the case with tegra.

> Also I do no think delete the xlate implementation is not a good idea.
> 

Which xlate routine? All of those that my patch set removes?

Since none of them do anything besides parsing the offset and
GPIO_ACTIVE_LOW flag, this seems like code bloat.

Please advise,


Eric