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

[Eric Nelson <eric@nelint.com>]

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

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.

Regards,


Eric