From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Sat, 9 Apr 2016 12:33:45 -0600 Subject: [U-Boot] [PATCH] dm: gpio: handle GPIO_ACTIVE_LOW flag in DT In-Reply-To: <5702A980.3040400@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> <57012392.9070505@nelint.com> <5702A980.3040400@wwwdotorg.org> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi, On 4 April 2016 at 11:50, Stephen Warren wrote: > 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. This seems a bit pedantic, but since Linux does it this way I think we should follow along. Eric you still get to remove the code from all the GPIO drivers - the difference is just creating a common function to call when no xlate() method is available. Can you please take a look at what Stephen suggests? Regards, Simon