| Message ID | 1458936731-13223-1-git-send-email-eric@nelint.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Eric, 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/ Regards, Peng. > >Signed-off-by: Eric Nelson <eric@nelint.com> >--- > drivers/gpio/gpio-uclass.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > >diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c >index b58d4e6..6d30612 100644 >--- a/drivers/gpio/gpio-uclass.c >+++ b/drivers/gpio/gpio-uclass.c >@@ -6,6 +6,7 @@ > > #include <common.h> > #include <dm.h> >+#include <dt-bindings/gpio/gpio.h> > #include <errno.h> > #include <fdtdec.h> > #include <malloc.h> >@@ -118,12 +119,16 @@ static int gpio_find_and_xlate(struct gpio_desc *desc, > { > struct dm_gpio_ops *ops = gpio_get_ops(desc->dev); > >+ desc->flags = 0; > /* Use the first argument as the offset by default */ >- if (args->args_count > 0) >+ if (args->args_count > 0) { > desc->offset = args->args[0]; >+ if ((args->args_count > 1) && >+ (args->args[1] & GPIO_ACTIVE_LOW)) >+ desc->flags = GPIOD_ACTIVE_LOW; >+ } > else > desc->offset = -1; >- desc->flags = 0; > > return ops->xlate ? ops->xlate(desc->dev, desc, args) : 0; > } >-- >2.6.2 > >_______________________________________________ >U-Boot mailing list >U-Boot@lists.denx.de >http://lists.denx.de/mailman/listinfo/u-boot
Hi Peng, On 03/28/2016 09:57 PM, Peng Fan wrote: > Hi Eric, > > 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, Eric
As Peng pointed out in [1], GPIO_ACTIVE_LOW is currently being parsed by driver-specific xlate routines, and an NXP/mxc-specific patch ([2]) to do the same on those processors is pending. Upon further inspection, I found that many arch-specific xlate routines were present only to parse either or both offset and the GPIO_ACTIVE_LOW flag, both of which can be handled at a global level. This series adds global support for GPIO_ACTIVE_LOW and removes the architecture-specific gpio xlate routine from five drivers. It also removes the handling of flags in the tegra gpio driver, though a custom xlate is still needed. [1] - http://lists.denx.de/pipermail/u-boot/2016-March/thread.html#249946 [2] - https://patchwork.ozlabs.org/patch/597363/ Eric Nelson (7): dm: gpio: handle GPIO_ACTIVE_LOW flag in DT gpio: intel_broadwell: remove gpio_xlate routine gpio: omap: remove gpio_xlate routine gpio: pic32: remove gpio_xlate routine gpio: rk: remove gpio_xlate routine gpio: exynos(s5p): remove gpio_xlate routine gpio: tegra: remove flags parsing in xlate routine drivers/gpio/gpio-uclass.c | 12 ++++++++---- drivers/gpio/intel_broadwell_gpio.c | 10 ---------- drivers/gpio/omap_gpio.c | 10 ---------- drivers/gpio/pic32_gpio.c | 9 --------- drivers/gpio/rk_gpio.c | 10 ---------- drivers/gpio/s5p_gpio.c | 10 ---------- drivers/gpio/tegra_gpio.c | 1 - 7 files changed, 8 insertions(+), 54 deletions(-)
Hi Eric, On Thu, Mar 31, 2016 at 01:41:04PM -0700, Eric Nelson wrote: >Hi Peng, > >On 03/28/2016 09:57 PM, Peng Fan wrote: >> Hi Eric, >> >> 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? 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. Also I do no think delete the xlate implementation is not a good idea. Simon may give more comments on this. Regards, Peng. > > >Eric
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
On 04/02/2016 09:13 AM, Eric Nelson wrote: > 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'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.
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
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.
Hi, On 4 April 2016 at 11:50, Stephen Warren <swarren@wwwdotorg.org> 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
Hi Simon, On 04/09/2016 11:33 AM, Simon Glass wrote: > On 4 April 2016 at 11:50, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 04/03/2016 08:07 AM, Eric Nelson wrote: >>> 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. >>>>>>>> <snip> >> >> 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? > Got it. I'm just not sure about where to start (before or after the patch set you sent) and whether to also remove offset parsing from gpio_find_and_xlate().
Hi Eric, On 10 April 2016 at 08:48, Eric Nelson <eric@nelint.com> wrote: > Hi Simon, > > On 04/09/2016 11:33 AM, Simon Glass wrote: >> On 4 April 2016 at 11:50, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> On 04/03/2016 08:07 AM, Eric Nelson wrote: >>>> 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. >>>>>>>>> > > <snip> > >>> >>> 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? >> > > Got it. I'm just not sure about where to start (before or after > the patch set you sent) and whether to also remove offset parsing > from gpio_find_and_xlate(). > Which patch did I send? My understanding is: - Add my review/ack tag to the patches as necessary - Drop the tegra patch - Update gpio_find_and_xlate() to call a default function if there is no xlate() method - Resend the series I'm not sure about removing the existing functionality from gpio_find_and_xlate(), but my guess is that it is best to move it to your default function, so that gpio_find_and_xlate() doesn't include any default behaviour in the case where there is a xlate() method. Regards, Simon
Hi Simon, On 04/11/2016 07:47 AM, Simon Glass wrote: > Hi Eric, > > On 10 April 2016 at 08:48, Eric Nelson <eric@nelint.com> wrote: >> Hi Simon, >> >> On 04/09/2016 11:33 AM, Simon Glass wrote: >>> On 4 April 2016 at 11:50, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>> On 04/03/2016 08:07 AM, Eric Nelson wrote: >>>>> 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. >>>>>>>>>> >> >> <snip> >> >>>> >>>> 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? >>> >> >> Got it. I'm just not sure about where to start (before or after >> the patch set you sent) and whether to also remove offset parsing >> from gpio_find_and_xlate(). >> > > Which patch did I send? My understanding is: > At the time I sent this, you had just submitted the patch set adding more driver-model support for block devices. http://lists.denx.de/pipermail/u-boot/2016-April/251095.html > - Add my review/ack tag to the patches as necessary > - Drop the tegra patch > - Update gpio_find_and_xlate() to call a default function if there is > no xlate() method > - Resend the series > > I'm not sure about removing the existing functionality from > gpio_find_and_xlate(), but my guess is that it is best to move it to > your default function, so that gpio_find_and_xlate() doesn't include > any default behaviour in the case where there is a xlate() method. > Reviewing the use of the offset field did yield some information about the broken sunxi support and also that Vybrid was also missing the xlate routine. Since reviewing your patch sets (driver model updates for blk and also driver model updates for mmc) will take some time, so I'll base an updated patch set on master. My guess is that any merge issues will be trivial. I'll remove your acks in the updated patch set, since the updates to the drivers won't drop the xlate field, but will connect them to the common (__maybe_unused) routine. This will prevent the code from leaking into machines like Tegra that don't need the common code. Regards, Eric
Hi Eric, On 11 April 2016 at 08:55, Eric Nelson <eric@nelint.com> wrote: > Hi Simon, > > On 04/11/2016 07:47 AM, Simon Glass wrote: >> Hi Eric, >> >> On 10 April 2016 at 08:48, Eric Nelson <eric@nelint.com> wrote: >>> Hi Simon, >>> >>> On 04/09/2016 11:33 AM, Simon Glass wrote: >>>> On 4 April 2016 at 11:50, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>>> On 04/03/2016 08:07 AM, Eric Nelson wrote: >>>>>> 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. >>>>>>>>>>> >>> >>> <snip> >>> >>>>> >>>>> 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? >>>> >>> >>> Got it. I'm just not sure about where to start (before or after >>> the patch set you sent) and whether to also remove offset parsing >>> from gpio_find_and_xlate(). >>> >> >> Which patch did I send? My understanding is: >> > > At the time I sent this, you had just submitted the patch set adding > more driver-model support for block devices. > > http://lists.denx.de/pipermail/u-boot/2016-April/251095.html > >> - Add my review/ack tag to the patches as necessary >> - Drop the tegra patch >> - Update gpio_find_and_xlate() to call a default function if there is >> no xlate() method >> - Resend the series >> >> I'm not sure about removing the existing functionality from >> gpio_find_and_xlate(), but my guess is that it is best to move it to >> your default function, so that gpio_find_and_xlate() doesn't include >> any default behaviour in the case where there is a xlate() method. >> > > Reviewing the use of the offset field did yield some information about > the broken sunxi support and also that Vybrid was also missing > the xlate routine. > > Since reviewing your patch sets (driver model updates for blk and also > driver model updates for mmc) will take some time, so I'll base an > updated patch set on master. My guess is that any merge issues will > be trivial. Yes, that's right. > > I'll remove your acks in the updated patch set, since the updates > to the drivers won't drop the xlate field, but will connect them > to the common (__maybe_unused) routine. This will prevent the code > from leaking into machines like Tegra that don't need the common code. I'm pretty sure you can drop the xlate() implementations from the functions, though, and those at the patches I acked. I don't think you need __maybe_unused static int gpio_find_and_xlate(...) { get ops... if (ops->xlate) return ops->xlate(....) else return gpio_default_xlate()... } gpio_default_xlate() (or whatever name you use) should be exported so drivers can use it. Regards, Simon
Hi Simon, On 04/11/2016 07:59 AM, Simon Glass wrote: > On 11 April 2016 at 08:55, Eric Nelson <eric@nelint.com> wrote: >> On 04/11/2016 07:47 AM, Simon Glass wrote: >>> On 10 April 2016 at 08:48, Eric Nelson <eric@nelint.com> wrote: >>>> On 04/09/2016 11:33 AM, Simon Glass wrote: >>>>> On 4 April 2016 at 11:50, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>>>> On 04/03/2016 08:07 AM, Eric Nelson wrote: >>>>>>> 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. >>>>>>>>>>>> >>>> >>>> <snip> >>>> >>>>>> >>>>>> 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? >>>>> >>>> >>>> Got it. I'm just not sure about where to start (before or after >>>> the patch set you sent) and whether to also remove offset parsing >>>> from gpio_find_and_xlate(). >>>> >>> >>> Which patch did I send? My understanding is: >>> >> >> At the time I sent this, you had just submitted the patch set adding >> more driver-model support for block devices. >> >> http://lists.denx.de/pipermail/u-boot/2016-April/251095.html >> >>> - Add my review/ack tag to the patches as necessary >>> - Drop the tegra patch >>> - Update gpio_find_and_xlate() to call a default function if there is >>> no xlate() method >>> - Resend the series >>> >>> I'm not sure about removing the existing functionality from >>> gpio_find_and_xlate(), but my guess is that it is best to move it to >>> your default function, so that gpio_find_and_xlate() doesn't include >>> any default behaviour in the case where there is a xlate() method. >>> >> >> Reviewing the use of the offset field did yield some information about >> the broken sunxi support and also that Vybrid was also missing >> the xlate routine. >> >> Since reviewing your patch sets (driver model updates for blk and also >> driver model updates for mmc) will take some time, so I'll base an >> updated patch set on master. My guess is that any merge issues will >> be trivial. > > Yes, that's right. >> >> I'll remove your acks in the updated patch set, since the updates >> to the drivers won't drop the xlate field, but will connect them >> to the common (__maybe_unused) routine. This will prevent the code >> from leaking into machines like Tegra that don't need the common code. > > I'm pretty sure you can drop the xlate() implementations from the > functions, though, and those at the patches I acked. > > I don't think you need __maybe_unused > > static int gpio_find_and_xlate(...) > { > get ops... > > if (ops->xlate) > return ops->xlate(....) > else > return gpio_default_xlate()... > } > > gpio_default_xlate() (or whatever name you use) should be exported so > drivers can use it. > This will leak gpio_default_xlate (locally named gpio_xlate_offs_flags) into machines that don't need it. I can go the route you suggest above, but it will cost the tegra and sandbox builds ~64 bytes ;)
Hi Eric, On 11 April 2016 at 09:10, Eric Nelson <eric@nelint.com> wrote: > Hi Simon, > > On 04/11/2016 07:59 AM, Simon Glass wrote: >> On 11 April 2016 at 08:55, Eric Nelson <eric@nelint.com> wrote: >>> On 04/11/2016 07:47 AM, Simon Glass wrote: >>>> On 10 April 2016 at 08:48, Eric Nelson <eric@nelint.com> wrote: >>>>> On 04/09/2016 11:33 AM, Simon Glass wrote: >>>>>> On 4 April 2016 at 11:50, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>>>>> On 04/03/2016 08:07 AM, Eric Nelson wrote: >>>>>>>> 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. >>>>>>>>>>>>> >>>>> >>>>> <snip> >>>>> >>>>>>> >>>>>>> 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? >>>>>> >>>>> >>>>> Got it. I'm just not sure about where to start (before or after >>>>> the patch set you sent) and whether to also remove offset parsing >>>>> from gpio_find_and_xlate(). >>>>> >>>> >>>> Which patch did I send? My understanding is: >>>> >>> >>> At the time I sent this, you had just submitted the patch set adding >>> more driver-model support for block devices. >>> >>> http://lists.denx.de/pipermail/u-boot/2016-April/251095.html >>> >>>> - Add my review/ack tag to the patches as necessary >>>> - Drop the tegra patch >>>> - Update gpio_find_and_xlate() to call a default function if there is >>>> no xlate() method >>>> - Resend the series >>>> >>>> I'm not sure about removing the existing functionality from >>>> gpio_find_and_xlate(), but my guess is that it is best to move it to >>>> your default function, so that gpio_find_and_xlate() doesn't include >>>> any default behaviour in the case where there is a xlate() method. >>>> >>> >>> Reviewing the use of the offset field did yield some information about >>> the broken sunxi support and also that Vybrid was also missing >>> the xlate routine. >>> >>> Since reviewing your patch sets (driver model updates for blk and also >>> driver model updates for mmc) will take some time, so I'll base an >>> updated patch set on master. My guess is that any merge issues will >>> be trivial. >> >> Yes, that's right. >>> >>> I'll remove your acks in the updated patch set, since the updates >>> to the drivers won't drop the xlate field, but will connect them >>> to the common (__maybe_unused) routine. This will prevent the code >>> from leaking into machines like Tegra that don't need the common code. >> >> I'm pretty sure you can drop the xlate() implementations from the >> functions, though, and those at the patches I acked. >> >> I don't think you need __maybe_unused >> >> static int gpio_find_and_xlate(...) >> { >> get ops... >> >> if (ops->xlate) >> return ops->xlate(....) >> else >> return gpio_default_xlate()... >> } >> >> gpio_default_xlate() (or whatever name you use) should be exported so >> drivers can use it. >> > > This will leak gpio_default_xlate (locally named gpio_xlate_offs_flags) > into machines that don't need it. > > I can go the route you suggest above, but it will cost the tegra > and sandbox builds ~64 bytes ;) > Sure, but we can live with that. Regards, Simon
On 04/11/2016 09:12 AM, Simon Glass wrote: > Hi Eric, > > On 11 April 2016 at 09:10, Eric Nelson <eric@nelint.com> wrote: >> Hi Simon, >> >> On 04/11/2016 07:59 AM, Simon Glass wrote: >>> On 11 April 2016 at 08:55, Eric Nelson <eric@nelint.com> wrote: >>>> On 04/11/2016 07:47 AM, Simon Glass wrote: >>>>> On 10 April 2016 at 08:48, Eric Nelson <eric@nelint.com> wrote: >>>>>> On 04/09/2016 11:33 AM, Simon Glass wrote: >>>>>>> On 4 April 2016 at 11:50, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>>>>>> On 04/03/2016 08:07 AM, Eric Nelson wrote: >>>>>>>>> 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. >>>>>>>>>>>>>> >>>>>> >>>>>> <snip> >>>>>> >>>>>>>> >>>>>>>> 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? >>>>>>> >>>>>> >>>>>> Got it. I'm just not sure about where to start (before or after >>>>>> the patch set you sent) and whether to also remove offset parsing >>>>>> from gpio_find_and_xlate(). >>>>>> >>>>> >>>>> Which patch did I send? My understanding is: >>>>> >>>> >>>> At the time I sent this, you had just submitted the patch set adding >>>> more driver-model support for block devices. >>>> >>>> http://lists.denx.de/pipermail/u-boot/2016-April/251095.html >>>> >>>>> - Add my review/ack tag to the patches as necessary >>>>> - Drop the tegra patch >>>>> - Update gpio_find_and_xlate() to call a default function if there is >>>>> no xlate() method >>>>> - Resend the series >>>>> >>>>> I'm not sure about removing the existing functionality from >>>>> gpio_find_and_xlate(), but my guess is that it is best to move it to >>>>> your default function, so that gpio_find_and_xlate() doesn't include >>>>> any default behaviour in the case where there is a xlate() method. >>>>> >>>> >>>> Reviewing the use of the offset field did yield some information about >>>> the broken sunxi support and also that Vybrid was also missing >>>> the xlate routine. >>>> >>>> Since reviewing your patch sets (driver model updates for blk and also >>>> driver model updates for mmc) will take some time, so I'll base an >>>> updated patch set on master. My guess is that any merge issues will >>>> be trivial. >>> >>> Yes, that's right. >>>> >>>> I'll remove your acks in the updated patch set, since the updates >>>> to the drivers won't drop the xlate field, but will connect them >>>> to the common (__maybe_unused) routine. This will prevent the code >>>> from leaking into machines like Tegra that don't need the common code. >>> >>> I'm pretty sure you can drop the xlate() implementations from the >>> functions, though, and those at the patches I acked. >>> >>> I don't think you need __maybe_unused >>> >>> static int gpio_find_and_xlate(...) >>> { >>> get ops... >>> >>> if (ops->xlate) >>> return ops->xlate(....) >>> else >>> return gpio_default_xlate()... >>> } >>> >>> gpio_default_xlate() (or whatever name you use) should be exported so >>> drivers can use it. >>> >> >> This will leak gpio_default_xlate (locally named gpio_xlate_offs_flags) >> into machines that don't need it. >> >> I can go the route you suggest above, but it will cost the tegra >> and sandbox builds ~64 bytes ;) >> > > Sure, but we can live with that. You can avoid that by requiring that ops->xlate always be non-NULL, and simply referencing the default function from drivers that want to use it. Not a big deal either way though.
Hi, On 11 April 2016 at 10:10, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 04/11/2016 09:12 AM, Simon Glass wrote: >> >> Hi Eric, >> >> On 11 April 2016 at 09:10, Eric Nelson <eric@nelint.com> wrote: >>> >>> Hi Simon, >>> >>> On 04/11/2016 07:59 AM, Simon Glass wrote: >>>> >>>> On 11 April 2016 at 08:55, Eric Nelson <eric@nelint.com> wrote: >>>>> >>>>> On 04/11/2016 07:47 AM, Simon Glass wrote: >>>>>> >>>>>> On 10 April 2016 at 08:48, Eric Nelson <eric@nelint.com> wrote: >>>>>>> >>>>>>> On 04/09/2016 11:33 AM, Simon Glass wrote: >>>>>>>> >>>>>>>> On 4 April 2016 at 11:50, Stephen Warren <swarren@wwwdotorg.org> >>>>>>>> wrote: >>>>>>>>> >>>>>>>>> On 04/03/2016 08:07 AM, Eric Nelson wrote: >>>>>>>>>> >>>>>>>>>> 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. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>> >>>>>>> <snip> >>>>>>> >>>>>>>>> >>>>>>>>> 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? >>>>>>>> >>>>>>> >>>>>>> Got it. I'm just not sure about where to start (before or after >>>>>>> the patch set you sent) and whether to also remove offset parsing >>>>>>> from gpio_find_and_xlate(). >>>>>>> >>>>>> >>>>>> Which patch did I send? My understanding is: >>>>>> >>>>> >>>>> At the time I sent this, you had just submitted the patch set adding >>>>> more driver-model support for block devices. >>>>> >>>>> http://lists.denx.de/pipermail/u-boot/2016-April/251095.html >>>>> >>>>>> - Add my review/ack tag to the patches as necessary >>>>>> - Drop the tegra patch >>>>>> - Update gpio_find_and_xlate() to call a default function if there is >>>>>> no xlate() method >>>>>> - Resend the series >>>>>> >>>>>> I'm not sure about removing the existing functionality from >>>>>> gpio_find_and_xlate(), but my guess is that it is best to move it to >>>>>> your default function, so that gpio_find_and_xlate() doesn't include >>>>>> any default behaviour in the case where there is a xlate() method. >>>>>> >>>>> >>>>> Reviewing the use of the offset field did yield some information about >>>>> the broken sunxi support and also that Vybrid was also missing >>>>> the xlate routine. >>>>> >>>>> Since reviewing your patch sets (driver model updates for blk and also >>>>> driver model updates for mmc) will take some time, so I'll base an >>>>> updated patch set on master. My guess is that any merge issues will >>>>> be trivial. >>>> >>>> >>>> Yes, that's right. >>>>> >>>>> >>>>> I'll remove your acks in the updated patch set, since the updates >>>>> to the drivers won't drop the xlate field, but will connect them >>>>> to the common (__maybe_unused) routine. This will prevent the code >>>>> from leaking into machines like Tegra that don't need the common code. >>>> >>>> >>>> I'm pretty sure you can drop the xlate() implementations from the >>>> functions, though, and those at the patches I acked. >>>> >>>> I don't think you need __maybe_unused >>>> >>>> static int gpio_find_and_xlate(...) >>>> { >>>> get ops... >>>> >>>> if (ops->xlate) >>>> return ops->xlate(....) >>>> else >>>> return gpio_default_xlate()... >>>> } >>>> >>>> gpio_default_xlate() (or whatever name you use) should be exported so >>>> drivers can use it. >>>> >>> >>> This will leak gpio_default_xlate (locally named gpio_xlate_offs_flags) >>> into machines that don't need it. >>> >>> I can go the route you suggest above, but it will cost the tegra >>> and sandbox builds ~64 bytes ;) >>> >> >> Sure, but we can live with that. > > > You can avoid that by requiring that ops->xlate always be non-NULL, and > simply referencing the default function from drivers that want to use it. > Not a big deal either way though. I'd prefer not to do that. It just adds cruft, the removal of which is the purpose of Eric's series. Regards, Simon
As Peng pointed out in [1], GPIO_ACTIVE_LOW is currently being parsed by driver-specific xlate routines, and an NXP/mxc-specific patch ([2]) to do the same on those processors is pending. This patch series takes a different approach and provides a default routine for xlate that handles the most common case of GPIO device tree node parsing: <&gpio1 2 GPIO_ACTIVE_LOW> The first routine adds the default gpio_xlate_offs_flags() routine, and the remainder of the patches remove the driver-specific versions from the intel_broadwell, omap, pic32, rk, and s5p drivers. V2 of this patch set removes parsing of offset from the gpio_find_and_xlate routine, and only parses the offset and GPIO_ACTIVE_LOW flag when a driver-specific xlate is unavailable. V2 also drops the update to the tegra_gpio driver. Eric Nelson (6): dm: gpio: add a default gpio xlate routine gpio: intel_broadwell: remove gpio_xlate routine gpio: omap: remove gpio_xlate routine gpio: pic32: remove gpio_xlate routine gpio: rk: remove gpio_xlate routine gpio: exynos(s5p): remove gpio_xlate routine drivers/gpio/gpio-uclass.c | 26 +++++++++++++++++++------- drivers/gpio/intel_broadwell_gpio.c | 10 ---------- drivers/gpio/omap_gpio.c | 11 ----------- drivers/gpio/pic32_gpio.c | 10 ---------- drivers/gpio/rk_gpio.c | 11 ----------- drivers/gpio/s5p_gpio.c | 11 ----------- include/asm-generic/gpio.h | 19 ++++++++++++++----- 7 files changed, 33 insertions(+), 65 deletions(-)
On 04/11/2016 09:53 AM, Simon Glass wrote: > Hi, > > On 11 April 2016 at 10:10, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 04/11/2016 09:12 AM, Simon Glass wrote: >>> >>> Hi Eric, >>> >>> On 11 April 2016 at 09:10, Eric Nelson <eric@nelint.com> wrote: >>>> <snip> >>>>> I don't think you need __maybe_unused >>>>> >>>>> static int gpio_find_and_xlate(...) >>>>> { >>>>> get ops... >>>>> >>>>> if (ops->xlate) >>>>> return ops->xlate(....) >>>>> else >>>>> return gpio_default_xlate()... >>>>> } >>>>> >>>>> gpio_default_xlate() (or whatever name you use) should be exported so >>>>> drivers can use it. >>>>> >>>> >>>> This will leak gpio_default_xlate (locally named gpio_xlate_offs_flags) >>>> into machines that don't need it. >>>> >>>> I can go the route you suggest above, but it will cost the tegra >>>> and sandbox builds ~64 bytes ;) >>>> >>> >>> Sure, but we can live with that. >> >> >> You can avoid that by requiring that ops->xlate always be non-NULL, and >> simply referencing the default function from drivers that want to use it. >> Not a big deal either way though. > > I'd prefer not to do that. It just adds cruft, the removal of which is > the purpose of Eric's series. > We must be close to the goal now, since we're picking apart stuff like this! Since I've done it both ways locally, I'll try to recap. Requiring an xlate puts a greater demand on the drivers, and requires an extra patch to get Vybrid working (adding xlate to vybrid_gpio.c) but does make it clearer which drivers need updates to handle DT parsing. There are a lot of them: altera_pio at91_gpio atmel_pio4 axp_gpio bcm2835_gpio dwapb_gpio gpio-uniphier hi6220_gpio intel_ich6_gpio lpc32xx_gpio msm_gpio mvebu_gpio pm8916_gpio None of these have dts files in either U-Boot or the kernel, so this doesn't appear to be a problem. Calling gpio_xlate_offs_flags as done in the V2 I just sent adds 64 bytes of code to the output for all machines, but transparently adds support for machines like vybrid and mxc. Regards, Eric
Hi Eric, On 11 April 2016 at 11:17, Eric Nelson <eric@nelint.com> wrote: > On 04/11/2016 09:53 AM, Simon Glass wrote: >> Hi, >> >> On 11 April 2016 at 10:10, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> On 04/11/2016 09:12 AM, Simon Glass wrote: >>>> >>>> Hi Eric, >>>> >>>> On 11 April 2016 at 09:10, Eric Nelson <eric@nelint.com> wrote: >>>>> > > <snip> > >>>>>> I don't think you need __maybe_unused >>>>>> >>>>>> static int gpio_find_and_xlate(...) >>>>>> { >>>>>> get ops... >>>>>> >>>>>> if (ops->xlate) >>>>>> return ops->xlate(....) >>>>>> else >>>>>> return gpio_default_xlate()... >>>>>> } >>>>>> >>>>>> gpio_default_xlate() (or whatever name you use) should be exported so >>>>>> drivers can use it. >>>>>> >>>>> >>>>> This will leak gpio_default_xlate (locally named gpio_xlate_offs_flags) >>>>> into machines that don't need it. >>>>> >>>>> I can go the route you suggest above, but it will cost the tegra >>>>> and sandbox builds ~64 bytes ;) >>>>> >>>> >>>> Sure, but we can live with that. >>> >>> >>> You can avoid that by requiring that ops->xlate always be non-NULL, and >>> simply referencing the default function from drivers that want to use it. >>> Not a big deal either way though. >> >> I'd prefer not to do that. It just adds cruft, the removal of which is >> the purpose of Eric's series. >> > > We must be close to the goal now, since we're picking apart stuff like > this! > > Since I've done it both ways locally, I'll try to recap. > > Requiring an xlate puts a greater demand on the drivers, and requires > an extra patch to get Vybrid working (adding xlate to vybrid_gpio.c) > but does make it clearer which drivers need updates to handle DT > parsing. > > There are a lot of them: > altera_pio > at91_gpio > atmel_pio4 > axp_gpio > bcm2835_gpio > dwapb_gpio > gpio-uniphier > hi6220_gpio > intel_ich6_gpio > lpc32xx_gpio > msm_gpio > mvebu_gpio > pm8916_gpio > > None of these have dts files in either U-Boot or the kernel, so this > doesn't appear to be a problem. > > Calling gpio_xlate_offs_flags as done in the V2 I just sent adds 64 > bytes of code to the output for all machines, but transparently adds > support for machines like vybrid and mxc. Yes that seems OK to me. Can you please send a new version of the series? Regards, Simon
Hi Simon, On 04/20/2016 07:40 AM, Simon Glass wrote: > Hi Eric, > > On 11 April 2016 at 11:17, Eric Nelson <eric@nelint.com> wrote: >> On 04/11/2016 09:53 AM, Simon Glass wrote: >>> On 11 April 2016 at 10:10, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>> On 04/11/2016 09:12 AM, Simon Glass wrote: >>>>> On 11 April 2016 at 09:10, Eric Nelson <eric@nelint.com> wrote: >>>>>> >> <snip> >> None of these have dts files in either U-Boot or the kernel, so this >> doesn't appear to be a problem. >> >> Calling gpio_xlate_offs_flags as done in the V2 I just sent adds 64 >> bytes of code to the output for all machines, but transparently adds >> support for machines like vybrid and mxc. > > Yes that seems OK to me. Can you please send a new version of the series? > Sure. I'll re-send shortly.
diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c index b58d4e6..6d30612 100644 --- a/drivers/gpio/gpio-uclass.c +++ b/drivers/gpio/gpio-uclass.c @@ -6,6 +6,7 @@ #include <common.h> #include <dm.h> +#include <dt-bindings/gpio/gpio.h> #include <errno.h> #include <fdtdec.h> #include <malloc.h> @@ -118,12 +119,16 @@ static int gpio_find_and_xlate(struct gpio_desc *desc, { struct dm_gpio_ops *ops = gpio_get_ops(desc->dev); + desc->flags = 0; /* Use the first argument as the offset by default */ - if (args->args_count > 0) + if (args->args_count > 0) { desc->offset = args->args[0]; + if ((args->args_count > 1) && + (args->args[1] & GPIO_ACTIVE_LOW)) + desc->flags = GPIOD_ACTIVE_LOW; + } else desc->offset = -1; - desc->flags = 0; return ops->xlate ? ops->xlate(desc->dev, desc, args) : 0; }
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. Signed-off-by: Eric Nelson <eric@nelint.com> --- drivers/gpio/gpio-uclass.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)