From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Nelson Date: Sun, 3 Apr 2016 07:07:14 -0700 Subject: [U-Boot] [PATCH] dm: gpio: handle GPIO_ACTIVE_LOW flag in DT In-Reply-To: <57008FE7.7000904@wwwdotorg.org> References: <1458936731-13223-1-git-send-email-eric@nelint.com> <20160329045729.GA32493@linux-7smt.suse> <56FD8B60.8060103@nelint.com> <20160402054612.GA27255@linux-7smt.suse> <56FFE1B3.6070608@nelint.com> <57008FE7.7000904@wwwdotorg.org> Message-ID: <57012392.9070505@nelint.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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