| Message ID | 20171108212505.28320-1-afd@ti.com |
|---|---|
| Headers | show |
| Series | Remove use of "gpio-reset" from DT | expand |
On Wed, Nov 08, 2017 at 03:25:04PM -0600, Andrew F. Davis wrote: > - ret = of_get_named_gpio(np, "gpio-reset", 0); > + ret = of_get_named_gpio(np, "reset-gpio", 0); This is obviously an incompatible change in the binding which will break any production DTs relying on the current behaviour. You need to keep support for the existing property. It also doesn't look like a good fix if we're aiming for conformance with DT naming conventions as unless things changed all GPIO related properties are supposed to end -gpios even if they can only ever specify a single GPIO.
On 11/08/2017 03:36 PM, Mark Brown wrote: > On Wed, Nov 08, 2017 at 03:25:04PM -0600, Andrew F. Davis wrote: > >> - ret = of_get_named_gpio(np, "gpio-reset", 0); >> + ret = of_get_named_gpio(np, "reset-gpio", 0); > > This is obviously an incompatible change in the binding which will break > any production DTs relying on the current behaviour. You need to keep > support for the existing property. > I understand the reasons not to change driver behavior wrt DT, but this driver did not make functional use of this gpio, only going forward will this gpio be used for actually reseting the device (in some patches I will post soon). So I would like to fix this incorrect binding *before* fixing it will cause behavior incompatibilities. > It also doesn't look like a good fix if we're aiming for conformance > with DT naming conventions as unless things changed all GPIO related > properties are supposed to end -gpios even if they can only ever specify > a single GPIO. > If that is the new standard I can fix this patch to use -gpios. I know we cant change all messed up DT bindings, but I hope an exception can be make in this case for sanity sake. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 08, 2017 at 03:53:51PM -0600, Andrew F. Davis wrote: > On 11/08/2017 03:36 PM, Mark Brown wrote: > > On Wed, Nov 08, 2017 at 03:25:04PM -0600, Andrew F. Davis wrote: > > This is obviously an incompatible change in the binding which will break > > any production DTs relying on the current behaviour. You need to keep > > support for the existing property. > I understand the reasons not to change driver behavior wrt DT, but this > driver did not make functional use of this gpio, only going forward will > this gpio be used for actually reseting the device (in some patches I > will post soon). So I would like to fix this incorrect binding *before* > fixing it will cause behavior incompatibilities. There is code in the driver to use the GPIO, including in the probe where the GPIO is requested and set to high (which will bring it out of reset if the default state was low). At least the probe seems rather likely to have a concrete effect. > > It also doesn't look like a good fix if we're aiming for conformance > > with DT naming conventions as unless things changed all GPIO related > > properties are supposed to end -gpios even if they can only ever specify > > a single GPIO. > If that is the new standard I can fix this patch to use -gpios. It's always been the standard AFAIK.
On 11/08/2017 04:18 PM, Mark Brown wrote: > On Wed, Nov 08, 2017 at 03:53:51PM -0600, Andrew F. Davis wrote: >> On 11/08/2017 03:36 PM, Mark Brown wrote: >>> On Wed, Nov 08, 2017 at 03:25:04PM -0600, Andrew F. Davis wrote: > >>> This is obviously an incompatible change in the binding which will break >>> any production DTs relying on the current behaviour. You need to keep >>> support for the existing property. > >> I understand the reasons not to change driver behavior wrt DT, but this >> driver did not make functional use of this gpio, only going forward will >> this gpio be used for actually reseting the device (in some patches I >> will post soon). So I would like to fix this incorrect binding *before* >> fixing it will cause behavior incompatibilities. > > There is code in the driver to use the GPIO, including in the probe > where the GPIO is requested and set to high (which will bring it out of > reset if the default state was low). At least the probe seems rather > likely to have a concrete effect. > None of the 4 boards that defined this gpio have this pulled low in the pin control, boards should have an external pull-up on the reset line. At any-rate, I'm pushing this fix to allow the driver to use new kernel frameworks that depend on the correct binding being used. This is not a case of a badly designed binding or trying to add incompatible functionality, it is a typo fix. If we have to keep this driver back using old frameworks and needlessly bloat the code solely for the sake of compatibility with a typo, then DT "stability" here is causing more issues than it solves. </rant> I guess the alternative would be to have of_find_gpio() also consider prefixing, the 'gpio_suffixes' to 'con_id' and checking for those. That is rather ugly and probably encourages the spread of this bad binding property, but whatever the best fix is it cannot be to force bloat into the driver. >>> It also doesn't look like a good fix if we're aiming for conformance >>> with DT naming conventions as unless things changed all GPIO related >>> properties are supposed to end -gpios even if they can only ever specify >>> a single GPIO. > >> If that is the new standard I can fix this patch to use -gpios. > > It's always been the standard AFAIK. > Will fix for v2. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 08, 2017 at 05:25:20PM -0600, Andrew F. Davis wrote: > On 11/08/2017 04:18 PM, Mark Brown wrote: > > There is code in the driver to use the GPIO, including in the probe > > where the GPIO is requested and set to high (which will bring it out of > > reset if the default state was low). At least the probe seems rather > > likely to have a concrete effect. > None of the 4 boards that defined this gpio have this pulled low in the > pin control, boards should have an external pull-up on the reset line. None of the boards in mainline... > At any-rate, I'm pushing this fix to allow the driver to use new kernel > frameworks that depend on the correct binding being used. This is not a > case of a badly designed binding or trying to add incompatible > functionality, it is a typo fix. If we have to keep this driver back > using old frameworks and needlessly bloat the code solely for the sake > of compatibility with a typo, then DT "stability" here is causing more > issues than it solves. </rant> This isn't a typographical error, that'd be a spelling mistake or something. > I guess the alternative would be to have of_find_gpio() also consider > prefixing, the 'gpio_suffixes' to 'con_id' and checking for those. That > is rather ugly and probably encourages the spread of this bad binding > property, but whatever the best fix is it cannot be to force bloat into > the driver. Another option is to add an interface for telling the DT code about renaming properties then the core code can do the fallback transparently to the upper layers (when asked for X if it's not there then try the legacy name Y).
On Wed, 2017-11-08 at 15:25 -0600, Andrew F. Davis wrote: > The correct DT property for specifying a GPIO used for reset > is "reset-gpio", fix this here. > > Fixes: 4341881d0562 ("ARM: dts: Add devicetree for Gumstix Pepper board") > > Signed-off-by: Andrew F. Davis <afd@ti.com> > --- > arch/arm/boot/dts/am335x-pepper.dts | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/boot/dts/am335x-pepper.dts b/arch/arm/boot/dts/am335x-pepper.dts > index 03c7d77023c6..d652abd76333 100644 > --- a/arch/arm/boot/dts/am335x-pepper.dts > +++ b/arch/arm/boot/dts/am335x-pepper.dts > @@ -139,7 +139,7 @@ > &audio_codec { > status = "okay"; > > - gpio-reset = <&gpio1 16 GPIO_ACTIVE_LOW>; > + reset-gpio = <&gpio1 16 GPIO_ACTIVE_LOW>; This potentially breaks audio on am335x-pepper until the driver patches are applied, same for the other device trees. To make this bisectable, add support for the new property name to the driver before changing the device trees. regards Philipp -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/09/2017 08:25 AM, Philipp Zabel wrote: > On Wed, 2017-11-08 at 15:25 -0600, Andrew F. Davis wrote: >> The correct DT property for specifying a GPIO used for reset >> is "reset-gpio", fix this here. >> >> Fixes: 4341881d0562 ("ARM: dts: Add devicetree for Gumstix Pepper board") >> >> Signed-off-by: Andrew F. Davis <afd@ti.com> >> --- >> arch/arm/boot/dts/am335x-pepper.dts | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/arm/boot/dts/am335x-pepper.dts b/arch/arm/boot/dts/am335x-pepper.dts >> index 03c7d77023c6..d652abd76333 100644 >> --- a/arch/arm/boot/dts/am335x-pepper.dts >> +++ b/arch/arm/boot/dts/am335x-pepper.dts >> @@ -139,7 +139,7 @@ >> &audio_codec { >> status = "okay"; >> >> - gpio-reset = <&gpio1 16 GPIO_ACTIVE_LOW>; >> + reset-gpio = <&gpio1 16 GPIO_ACTIVE_LOW>; > > This potentially breaks audio on am335x-pepper until the driver patches > are applied, same for the other device trees. To make this bisectable, > add support for the new property name to the driver before changing the > device trees. AFAIK this change doesn't break anything as this reset was not used in a functional way (it was just pulled high in probe if provided, but the pin_mux and/or and on-board resistor should have done the same already, but a series I just posted will actually get some use out of it and so I want to get this fixed before then). As controversial as it may be, my plan is to simply not support the old binding at all, supporting it would add unnecessary bloat to the driver, prevent moving to new frameworks that expect the correct binding (fwnode_get_named_gpiod), and in this case provide no benefit as far as I can tell. Andrew > > regards > Philipp > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello all, I was working on fixing up the tlv320aic31xx driver when I noticed its gpio reset was not working, it was due to this driver looking for "gpio-reset" instead of the usual "reset-gpio". A quick check shows this mistake is rare and only copied by one other audio CODECs: $ git grep "reset-gpio" | wc -l 630 $ git grep "gpio-reset" | wc -l 6 Luckliy the two effected drivers only use this reset line when power has been cut to the device, so not only were these optional properties but they had no real functional effect anyway. Lets just fix this typo before is spreads to drivers were it matters. I've also added fixes tags to each patch so it can be individually back-ported to where this bug was introduced if one wanted to. I'm hoping this can sit on next for a while to get the most testing, just in case I'm wrong about this not breaking anything :) Thanks, Andrew Andrew F. Davis (9): ASoC: tlv320aic31xx: Fix typo in DT binding documentation ASoC: tlv320aic3x: Fix typo in DT binding documentation ASoC: cs42l56: bindings: sound: Fix reset GPIO name in example DT binding ARM: dts: am335x-pepper: Fix the audio CODEC's reset pin ARM: dts: imx6: RDU2: Fix the audio CODEC's reset pin ARM: dts: imx: Fix the audio CODEC's reset pin ARM: dts: omap3-n900: Fix the audio CODEC's reset pin ASoC: tlv320aic31xx: Fix the reset GPIO OF name ASoC: tlv320aic3x: Fix the reset GPIO OF name Documentation/devicetree/bindings/sound/cs42l56.txt | 2 +- Documentation/devicetree/bindings/sound/tlv320aic31xx.txt | 5 ++++- Documentation/devicetree/bindings/sound/tlv320aic3x.txt | 6 +++++- arch/arm/boot/dts/am335x-pepper.dts | 2 +- arch/arm/boot/dts/imx6qdl-gw5903.dtsi | 2 +- arch/arm/boot/dts/imx6qdl-zii-rdu2.dtsi | 4 ++-- arch/arm/boot/dts/omap3-n900.dts | 4 ++-- sound/soc/codecs/tlv320aic31xx.c | 2 +- sound/soc/codecs/tlv320aic3x.c | 2 +- 9 files changed, 18 insertions(+), 11 deletions(-)