[alsa-devel] [PATCH 0/9] Remove use of "gpio-reset" from DT
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(-)
The property used to specify a GPIO intended for reset is "reset-gpio", this binding uses "gpio-reset", as almost all other bindings use the former name this use of the latter is certainly not intended and was a typo. It is not compatible with newer methods used to fetch GPIO pins and to prevent the spread of this error to other bindings lets fix this here.
We also standardize the pin as active-low, different device trees have marked the GPIO different ways, luckily the driver currently uses the low-level GPIO set function which does not respect the active-low flag, but future changes may change this. This is an active-low reset, mark it as such.
Lastly, add an example of use for this property.
Fixes: e00447fafbf7 ("ASoC: tlv320aic31xx: Add basic codec driver implementation")
Signed-off-by: Andrew F. Davis afd@ti.com --- Documentation/devicetree/bindings/sound/tlv320aic31xx.txt | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/sound/tlv320aic31xx.txt b/Documentation/devicetree/bindings/sound/tlv320aic31xx.txt index 6fbba562eaa7..4c4e77f97d87 100644 --- a/Documentation/devicetree/bindings/sound/tlv320aic31xx.txt +++ b/Documentation/devicetree/bindings/sound/tlv320aic31xx.txt @@ -22,7 +22,7 @@ Required properties:
Optional properties:
-- gpio-reset - gpio pin number used for codec reset +- reset-gpio - GPIO specification for the active low RESET input. - ai31xx-micbias-vg - MicBias Voltage setting 1 or MICBIAS_2_0V - MICBIAS output is powered to 2.0V 2 or MICBIAS_2_5V - MICBIAS output is powered to 2.5V @@ -48,6 +48,7 @@ CODEC input pins: The pins can be used in referring sound node's audio-routing property.
Example: +#include <dt-bindings/gpio/gpio.h> #include <dt-bindings/sound/tlv320aic31xx-micbias.h>
tlv320aic31xx: tlv320aic31xx@18 { @@ -56,6 +57,8 @@ tlv320aic31xx: tlv320aic31xx@18 {
ai31xx-micbias-vg = <MICBIAS_OFF>;
+ reset-gpio = <&gpio1 17 GPIO_ACTIVE_LOW>; + HPVDD-supply = <®ulator>; SPRVDD-supply = <®ulator>; SPLVDD-supply = <®ulator>;
Hi Andrew,
On Wed, 2017-11-08 at 15:24 -0600, Andrew F. Davis wrote:
The property used to specify a GPIO intended for reset is "reset-gpio", this binding uses "gpio-reset", as almost all other bindings use the former name this use of the latter is certainly not intended and was a typo. It is not compatible with newer methods used to fetch GPIO pins and to prevent the spread of this error to other bindings lets fix this here.
We also standardize the pin as active-low, different device trees have marked the GPIO different ways, luckily the driver currently uses the low-level GPIO set function which does not respect the active-low flag, but future changes may change this. This is an active-low reset, mark it as such.
Lastly, add an example of use for this property.
Fixes: e00447fafbf7 ("ASoC: tlv320aic31xx: Add basic codec driver implementation")
Signed-off-by: Andrew F. Davis afd@ti.com
Documentation/devicetree/bindings/sound/tlv320aic31xx.txt | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/sound/tlv320aic31xx.txt b/Documentation/devicetree/bindings/sound/tlv320aic31xx.txt index 6fbba562eaa7..4c4e77f97d87 100644 --- a/Documentation/devicetree/bindings/sound/tlv320aic31xx.txt +++ b/Documentation/devicetree/bindings/sound/tlv320aic31xx.txt @@ -22,7 +22,7 @@ Required properties:
Optional properties:
-- gpio-reset - gpio pin number used for codec reset
I would move this into a "Deprecated properties:" section instead of just hiding it away.
+- reset-gpio - GPIO specification for the active low RESET input.
This should be "reset-gpios". For reference, see Documentation/devicetree/bindings/gpio/gpio.txt.
- ai31xx-micbias-vg - MicBias Voltage setting 1 or MICBIAS_2_0V - MICBIAS output is powered to 2.0V 2 or MICBIAS_2_5V - MICBIAS output is powered to 2.5V
@@ -48,6 +48,7 @@ CODEC input pins: The pins can be used in referring sound node's audio-routing property.
Example: +#include <dt-bindings/gpio/gpio.h> #include <dt-bindings/sound/tlv320aic31xx-micbias.h>
tlv320aic31xx: tlv320aic31xx@18 { @@ -56,6 +57,8 @@ tlv320aic31xx: tlv320aic31xx@18 {
ai31xx-micbias-vg = <MICBIAS_OFF>;
- reset-gpio = <&gpio1 17 GPIO_ACTIVE_LOW>;
- HPVDD-supply = <®ulator>; SPRVDD-supply = <®ulator>; SPLVDD-supply = <®ulator>;
regards Philipp
On 11/09/2017 08:25 AM, Philipp Zabel wrote:
Hi Andrew,
On Wed, 2017-11-08 at 15:24 -0600, Andrew F. Davis wrote:
The property used to specify a GPIO intended for reset is "reset-gpio", this binding uses "gpio-reset", as almost all other bindings use the former name this use of the latter is certainly not intended and was a typo. It is not compatible with newer methods used to fetch GPIO pins and to prevent the spread of this error to other bindings lets fix this here.
We also standardize the pin as active-low, different device trees have marked the GPIO different ways, luckily the driver currently uses the low-level GPIO set function which does not respect the active-low flag, but future changes may change this. This is an active-low reset, mark it as such.
Lastly, add an example of use for this property.
Fixes: e00447fafbf7 ("ASoC: tlv320aic31xx: Add basic codec driver implementation")
Signed-off-by: Andrew F. Davis afd@ti.com
Documentation/devicetree/bindings/sound/tlv320aic31xx.txt | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/sound/tlv320aic31xx.txt b/Documentation/devicetree/bindings/sound/tlv320aic31xx.txt index 6fbba562eaa7..4c4e77f97d87 100644 --- a/Documentation/devicetree/bindings/sound/tlv320aic31xx.txt +++ b/Documentation/devicetree/bindings/sound/tlv320aic31xx.txt @@ -22,7 +22,7 @@ Required properties:
Optional properties:
-- gpio-reset - gpio pin number used for codec reset
I would move this into a "Deprecated properties:" section instead of just hiding it away.
The hope was to fix this mistake and pretend it never existed :)
I'll add a Deprecated section in case this breaks for someone they can see that it was changed.
+- reset-gpio - GPIO specification for the active low RESET input.
This should be "reset-gpios". For reference, see Documentation/devicetree/bindings/gpio/gpio.txt.
Yup, will fix for v2.
- ai31xx-micbias-vg - MicBias Voltage setting 1 or MICBIAS_2_0V - MICBIAS output is powered to 2.0V 2 or MICBIAS_2_5V - MICBIAS output is powered to 2.5V
@@ -48,6 +48,7 @@ CODEC input pins: The pins can be used in referring sound node's audio-routing property.
Example: +#include <dt-bindings/gpio/gpio.h> #include <dt-bindings/sound/tlv320aic31xx-micbias.h>
tlv320aic31xx: tlv320aic31xx@18 { @@ -56,6 +57,8 @@ tlv320aic31xx: tlv320aic31xx@18 {
ai31xx-micbias-vg = <MICBIAS_OFF>;
- reset-gpio = <&gpio1 17 GPIO_ACTIVE_LOW>;
- HPVDD-supply = <®ulator>; SPRVDD-supply = <®ulator>; SPLVDD-supply = <®ulator>;
regards Philipp
The property used to specify a GPIO intended for reset is "reset-gpio", this binding uses "gpio-reset", as almost all other bindings use the former name this use of the latter is certainly not intended and was a typo. It is not compatible with newer methods used to fetch GPIO pins and to prevent the spread of this error to other bindings lets fix this here.
We also standardize the pin as active-low, different device trees have marked the GPIO different ways, luckily the driver currently uses the low-level GPIO set function which does not respect the active-low flag, but future changes may change this. This is an active-low reset, mark it as such.
Lastly, add an example of use for this property.
Fixes: c24fdc886fde ("ASoC: tlv320aic3x: Add device tree bindings")
Signed-off-by: Andrew F. Davis afd@ti.com --- Documentation/devicetree/bindings/sound/tlv320aic3x.txt | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/sound/tlv320aic3x.txt b/Documentation/devicetree/bindings/sound/tlv320aic3x.txt index ba5b45c483f5..9e8eaa08ce90 100644 --- a/Documentation/devicetree/bindings/sound/tlv320aic3x.txt +++ b/Documentation/devicetree/bindings/sound/tlv320aic3x.txt @@ -17,7 +17,7 @@ Required properties:
Optional properties:
-- gpio-reset - gpio pin number used for codec reset +- reset-gpio - GPIO specification for the active low RESET input. - ai3x-gpio-func - <array of 2 int> - AIC3X_GPIO1 & AIC3X_GPIO2 Functionality - Not supported on tlv320aic3104 - ai3x-micbias-vg - MicBias Voltage required. @@ -61,10 +61,14 @@ The pins can be used in referring sound node's audio-routing property.
Example:
+#include <dt-bindings/gpio/gpio.h> + tlv320aic3x: tlv320aic3x@1b { compatible = "ti,tlv320aic3x"; reg = <0x1b>;
+ reset-gpio = <&gpio1 17 GPIO_ACTIVE_LOW>; + AVDD-supply = <®ulator>; IOVDD-supply = <®ulator>; DRVDD-supply = <®ulator>;
Dear Andrew F. Davis,
On Wed, Nov 8, 2017 at 10:24 PM, Andrew F. Davis afd@ti.com wrote:
The property used to specify a GPIO intended for reset is "reset-gpio", this binding uses "gpio-reset", as almost all other bindings use the former name this use of the latter is certainly not intended and was a typo. It is not compatible with newer methods used to fetch GPIO pins and to prevent the spread of this error to other bindings lets fix this here.
We also standardize the pin as active-low, different device trees have marked the GPIO different ways, luckily the driver currently uses the low-level GPIO set function which does not respect the active-low flag, but future changes may change this. This is an active-low reset, mark it as such.
Lastly, add an example of use for this property.
Fixes: c24fdc886fde ("ASoC: tlv320aic3x: Add device tree bindings")
Signed-off-by: Andrew F. Davis afd@ti.com
Documentation/devicetree/bindings/sound/tlv320aic3x.txt | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/sound/tlv320aic3x.txt b/Documentation/devicetree/bindings/sound/tlv320aic3x.txt index ba5b45c483f5..9e8eaa08ce90 100644 --- a/Documentation/devicetree/bindings/sound/tlv320aic3x.txt +++ b/Documentation/devicetree/bindings/sound/tlv320aic3x.txt @@ -17,7 +17,7 @@ Required properties:
Optional properties:
-- gpio-reset - gpio pin number used for codec reset +- reset-gpio - GPIO specification for the active low RESET input.
- ai3x-gpio-func - <array of 2 int> - AIC3X_GPIO1 & AIC3X_GPIO2 Functionality - Not supported on tlv320aic3104
- ai3x-micbias-vg - MicBias Voltage required.
@@ -61,10 +61,14 @@ The pins can be used in referring sound node's audio-routing property.
Example:
+#include <dt-bindings/gpio/gpio.h>
tlv320aic3x: tlv320aic3x@1b { compatible = "ti,tlv320aic3x"; reg = <0x1b>;
reset-gpio = <&gpio1 17 GPIO_ACTIVE_LOW>;AVDD-supply = <®ulator>; IOVDD-supply = <®ulator>; DRVDD-supply = <®ulator>;
According to Documentation/devicetree/bindings/gpio/gpio.txt: "GPIO properties should be named "[<name>-]gpios", with <name> being the purpose of this GPIO for the device. While a non-existent <name> is considered valid for compatibility reasons (resolving to the "gpios" property), it is not allowed for new bindings. Also, GPIO properties named "[<name>-]gpio" are valid and old bindings use it, but are only supported for compatibility reasons and should not be used for newer bindings since it has been deprecated."
So it should be "reset-gpios" for new bindings.
Also, sound/soc/codecs/tlv320aic3x.c still uses "gpio-reset", and I do not see any patch in your series changing this: ret = of_get_named_gpio(np, "gpio-reset", 0);
Same remarks for your "[PATCH 6/9] ARM: dts: imx: Fix the audio CODEC's reset pin". I don't know if there are other DTS files using this, but they should all be updated accordingly. This would anyway break all of the out-of-tree DTS files using this, which I think is not allowed, so the driver should be changed in a way still allowing the legacy naming besides the new one.
Best regards, Benoît
On 11/09/2017 02:26 PM, Benoît Thébaudeau wrote:
Dear Andrew F. Davis,
On Wed, Nov 8, 2017 at 10:24 PM, Andrew F. Davis afd@ti.com wrote:
The property used to specify a GPIO intended for reset is "reset-gpio", this binding uses "gpio-reset", as almost all other bindings use the former name this use of the latter is certainly not intended and was a typo. It is not compatible with newer methods used to fetch GPIO pins and to prevent the spread of this error to other bindings lets fix this here.
We also standardize the pin as active-low, different device trees have marked the GPIO different ways, luckily the driver currently uses the low-level GPIO set function which does not respect the active-low flag, but future changes may change this. This is an active-low reset, mark it as such.
Lastly, add an example of use for this property.
Fixes: c24fdc886fde ("ASoC: tlv320aic3x: Add device tree bindings")
Signed-off-by: Andrew F. Davis afd@ti.com
Documentation/devicetree/bindings/sound/tlv320aic3x.txt | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/sound/tlv320aic3x.txt b/Documentation/devicetree/bindings/sound/tlv320aic3x.txt index ba5b45c483f5..9e8eaa08ce90 100644 --- a/Documentation/devicetree/bindings/sound/tlv320aic3x.txt +++ b/Documentation/devicetree/bindings/sound/tlv320aic3x.txt @@ -17,7 +17,7 @@ Required properties:
Optional properties:
-- gpio-reset - gpio pin number used for codec reset +- reset-gpio - GPIO specification for the active low RESET input.
- ai3x-gpio-func - <array of 2 int> - AIC3X_GPIO1 & AIC3X_GPIO2 Functionality - Not supported on tlv320aic3104
- ai3x-micbias-vg - MicBias Voltage required.
@@ -61,10 +61,14 @@ The pins can be used in referring sound node's audio-routing property.
Example:
+#include <dt-bindings/gpio/gpio.h>
tlv320aic3x: tlv320aic3x@1b { compatible = "ti,tlv320aic3x"; reg = <0x1b>;
reset-gpio = <&gpio1 17 GPIO_ACTIVE_LOW>;AVDD-supply = <®ulator>; IOVDD-supply = <®ulator>; DRVDD-supply = <®ulator>;According to Documentation/devicetree/bindings/gpio/gpio.txt: "GPIO properties should be named "[<name>-]gpios", with <name> being the purpose of this GPIO for the device. While a non-existent <name> is considered valid for compatibility reasons (resolving to the "gpios" property), it is not allowed for new bindings. Also, GPIO properties named "[<name>-]gpio" are valid and old bindings use it, but are only supported for compatibility reasons and should not be used for newer bindings since it has been deprecated."
So it should be "reset-gpios" for new bindings.
You are the third person to comment this. I wish people would have been this observant when the original patch adding the completely backwards "gpio-reset" was posted... :)
Also, sound/soc/codecs/tlv320aic3x.c still uses "gpio-reset", and I do not see any patch in your series changing this: ret = of_get_named_gpio(np, "gpio-reset", 0);
Check patch #9
Same remarks for your "[PATCH 6/9] ARM: dts: imx: Fix the audio CODEC's reset pin". I don't know if there are other DTS files using this, but they should all be updated accordingly. This would anyway break all of the out-of-tree DTS files using this, which I think is not allowed, so the driver should be changed in a way still allowing the legacy naming besides the new one.
I have updated every in-tree use. I know we should work to avoid breaking out-of-tree DTS files, but I'm asking for a small exception here for reasons I've described in the cover-letter and other patches.
I just can't see this breaking anything, if there actually exists a real out-of-tree user of this with a DTB that is not being updated with the kernel version and this patch causes any functionality change for them I will literally eat a hat.
Thanks, Andrew
Best regards, Benoît
The binding states the reset GPIO property shall be named "cirrus,gpio-nreset" and this is what the driver looks for, but the example uses "gpio-reset". Fix this here.
Fixes: 3bb40619aca8 ("ASoC: cs42l56: bindings: sound: Add bindings for CS42L56 CODEC")
Signed-off-by: Andrew F. Davis afd@ti.com --- Documentation/devicetree/bindings/sound/cs42l56.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/sound/cs42l56.txt b/Documentation/devicetree/bindings/sound/cs42l56.txt index 4feb0eb27ea4..4ba520a28ae8 100644 --- a/Documentation/devicetree/bindings/sound/cs42l56.txt +++ b/Documentation/devicetree/bindings/sound/cs42l56.txt @@ -55,7 +55,7 @@ Example: codec: codec@4b { compatible = "cirrus,cs42l56"; reg = <0x4b>; - gpio-reset = <&gpio 10 0>; + cirrus,gpio-nreset = <&gpio 10 0>; cirrus,chgfreq-divisor = <0x05>; cirrus.ain1_ref_cfg; cirrus,micbias-lvl = <5>;
On Wed, Nov 08, 2017 at 03:24:59PM -0600, Andrew F. Davis wrote:
The binding states the reset GPIO property shall be named "cirrus,gpio-nreset" and this is what the driver looks for, but the example uses "gpio-reset". Fix this here.
Fixes: 3bb40619aca8 ("ASoC: cs42l56: bindings: sound: Add bindings for CS42L56 CODEC")
Signed-off-by: Andrew F. Davis afd@ti.com
Documentation/devicetree/bindings/sound/cs42l56.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/sound/cs42l56.txt b/Documentation/devicetree/bindings/sound/cs42l56.txt index 4feb0eb27ea4..4ba520a28ae8 100644 --- a/Documentation/devicetree/bindings/sound/cs42l56.txt +++ b/Documentation/devicetree/bindings/sound/cs42l56.txt @@ -55,7 +55,7 @@ Example: codec: codec@4b { compatible = "cirrus,cs42l56"; reg = <0x4b>;
- gpio-reset = <&gpio 10 0>;
- cirrus,gpio-nreset = <&gpio 10 0>;
Can we deprecate this and use reset-gpios instead?
cirrus,chgfreq-divisor = <0x05>; cirrus.ain1_ref_cfg; cirrus,micbias-lvl = <5>; -- 2.15.0
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>; AVDD-supply = <&ldo3_reg>; IOVDD-supply = <&ldo3_reg>; DRVDD-supply = <&ldo3_reg>;
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
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
The correct DT property for specifying a GPIO used for reset is "reset-gpio", fix this here.
Fixes: d763762e3b58 ("ARM: dts: imx6: add ZII RDU2 boards")
Signed-off-by: Andrew F. Davis afd@ti.com --- arch/arm/boot/dts/imx6qdl-zii-rdu2.dtsi | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/imx6qdl-zii-rdu2.dtsi b/arch/arm/boot/dts/imx6qdl-zii-rdu2.dtsi index eeb7679fd348..261e6896b286 100644 --- a/arch/arm/boot/dts/imx6qdl-zii-rdu2.dtsi +++ b/arch/arm/boot/dts/imx6qdl-zii-rdu2.dtsi @@ -337,7 +337,7 @@ AVDD-supply = <®_3p3v>; IOVDD-supply = <®_3p3v>; DVDD-supply = <&vgen4_reg>; - gpio-reset = <&gpio1 2 GPIO_ACTIVE_HIGH>; + reset-gpio = <&gpio1 2 GPIO_ACTIVE_LOW>; };
accel@1c { @@ -525,7 +525,7 @@ AVDD-supply = <®_3p3v>; IOVDD-supply = <®_3p3v>; DVDD-supply = <&vgen4_reg>; - gpio-reset = <&gpio1 0 GPIO_ACTIVE_HIGH>; + reset-gpio = <&gpio1 0 GPIO_ACTIVE_LOW>; };
touchscreen@20 {
The correct DT property for specifying a GPIO used for reset is "reset-gpio", fix this here.
Fixes: 50bffb78e2ee ("ARM: dts: imx: add Gateworks Ventana GW5903 support")
Signed-off-by: Andrew F. Davis afd@ti.com --- arch/arm/boot/dts/imx6qdl-gw5903.dtsi | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/imx6qdl-gw5903.dtsi b/arch/arm/boot/dts/imx6qdl-gw5903.dtsi index 444425153fc7..b41ba46508db 100644 --- a/arch/arm/boot/dts/imx6qdl-gw5903.dtsi +++ b/arch/arm/boot/dts/imx6qdl-gw5903.dtsi @@ -310,7 +310,7 @@ tlv320aic3105: codec@18 { compatible = "ti,tlv320aic3x"; reg = <0x18>; - gpio-reset = <&gpio5 17 GPIO_ACTIVE_LOW>; + reset-gpio = <&gpio5 17 GPIO_ACTIVE_LOW>; clocks = <&clks IMX6QDL_CLK_CKO>; ai3x-micbias-vg = <2>; /* MICBIAS_2_5V */ /* Regulators */
The correct DT property for specifying a GPIO used for reset is "reset-gpio", fix this here.
Fixes: 14e3e295b2b9 ("ARM: dts: omap3-n900: Add TLV320AIC3X support")
Signed-off-by: Andrew F. Davis afd@ti.com --- arch/arm/boot/dts/omap3-n900.dts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/omap3-n900.dts b/arch/arm/boot/dts/omap3-n900.dts index 4acd32a1c4ef..6fbe6e095c3f 100644 --- a/arch/arm/boot/dts/omap3-n900.dts +++ b/arch/arm/boot/dts/omap3-n900.dts @@ -558,7 +558,7 @@ tlv320aic3x: tlv320aic3x@18 { compatible = "ti,tlv320aic3x"; reg = <0x18>; - gpio-reset = <&gpio2 28 GPIO_ACTIVE_HIGH>; /* 60 */ + reset-gpio = <&gpio2 28 GPIO_ACTIVE_LOW>; /* 60 */ ai3x-gpio-func = < 0 /* AIC3X_GPIO1_FUNC_DISABLED */ 5 /* AIC3X_GPIO2_FUNC_DIGITAL_MIC_INPUT */ @@ -575,7 +575,7 @@ tlv320aic3x_aux: tlv320aic3x@19 { compatible = "ti,tlv320aic3x"; reg = <0x19>; - gpio-reset = <&gpio2 28 GPIO_ACTIVE_HIGH>; /* 60 */ + reset-gpio = <&gpio2 28 GPIO_ACTIVE_LOW>; /* 60 */
AVDD-supply = <&vmmc2>; DRVDD-supply = <&vmmc2>;
The correct DT property for specifying a GPIO used for reset is "reset-gpio", fix this here.
Fixes: e00447fafbf7 ("ASoC: tlv320aic31xx: Add basic codec driver implementation")
Signed-off-by: Andrew F. Davis afd@ti.com --- sound/soc/codecs/tlv320aic31xx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/tlv320aic31xx.c b/sound/soc/codecs/tlv320aic31xx.c index 54a87a905eb6..359eba15bb89 100644 --- a/sound/soc/codecs/tlv320aic31xx.c +++ b/sound/soc/codecs/tlv320aic31xx.c @@ -1279,7 +1279,7 @@ static void aic31xx_pdata_from_of(struct aic31xx_priv *aic31xx) aic31xx->pdata.micbias_vg = MICBIAS_2_0V; }
- ret = of_get_named_gpio(np, "gpio-reset", 0); + ret = of_get_named_gpio(np, "reset-gpio", 0); if (ret > 0) aic31xx->pdata.gpio_reset = ret; }
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.
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.
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).
The correct DT property for specifying a GPIO used for reset is "reset-gpio", fix this here.
Fixes: c24fdc886fde ("ASoC: tlv320aic3x: Add device tree bindings")
Signed-off-by: Andrew F. Davis afd@ti.com --- sound/soc/codecs/tlv320aic3x.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c index 06f92571eba4..6a8b450d2431 100644 --- a/sound/soc/codecs/tlv320aic3x.c +++ b/sound/soc/codecs/tlv320aic3x.c @@ -1804,7 +1804,7 @@ static int aic3x_i2c_probe(struct i2c_client *i2c, if (!ai3x_setup) return -ENOMEM;
- ret = of_get_named_gpio(np, "gpio-reset", 0); + ret = of_get_named_gpio(np, "reset-gpio", 0); if (ret >= 0) aic3x->gpio_reset = ret; else
participants (5)
-
Andrew F. Davis -
Benoît Thébaudeau -
Mark Brown -
Philipp Zabel -
Rob Herring