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