[PATCH 0/9] Remove use of "gpio-reset" from DT

[PATCH 0/9] Remove use of "gpio-reset" from DT

[Andrew F. Davis]

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(-)

-- 
2.15.0

[PATCH 1/9] ASoC: tlv320aic31xx: Fix typo in DT binding documentation

[Andrew F. Davis]

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 = <&regulator>;
 	SPRVDD-supply = <&regulator>;
 	SPLVDD-supply = <&regulator>;
-- 
2.15.0

[PATCH 2/9] ASoC: tlv320aic3x: Fix typo in DT binding documentation

[Andrew F. Davis]

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 = <&regulator>;
 	IOVDD-supply = <&regulator>;
 	DRVDD-supply = <&regulator>;
-- 
2.15.0

[PATCH 3/9] ASoC: cs42l56: bindings: sound: Fix reset GPIO name in example DT binding

[Andrew F. Davis]

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>;
-- 
2.15.0

[PATCH 4/9] ARM: dts: am335x-pepper: Fix the audio CODEC's reset pin

[Andrew F. Davis]

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>;
-- 
2.15.0

[PATCH 5/9] ARM: dts: imx6: RDU2: Fix the audio CODEC's reset pin

[Andrew F. Davis]

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 = <&reg_3p3v>;
 		IOVDD-supply = <&reg_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 = <&reg_3p3v>;
 		IOVDD-supply = <&reg_3p3v>;
 		DVDD-supply = <&vgen4_reg>;
-		gpio-reset = <&gpio1 0 GPIO_ACTIVE_HIGH>;
+		reset-gpio = <&gpio1 0 GPIO_ACTIVE_LOW>;
 	};
 
 	touchscreen@20 {
-- 
2.15.0

[PATCH 6/9] ARM: dts: imx: Fix the audio CODEC's reset pin

[Andrew F. Davis]

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 */
-- 
2.15.0

[PATCH 7/9] ARM: dts: omap3-n900: Fix the audio CODEC's reset pin

[Andrew F. Davis]

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>;
-- 
2.15.0

[PATCH 8/9] ASoC: tlv320aic31xx: Fix the reset GPIO OF name

[Andrew F. Davis]

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;
 }
-- 
2.15.0

[PATCH 9/9] ASoC: tlv320aic3x: Fix the reset GPIO OF name

[Andrew F. Davis]

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
-- 
2.15.0

Re: [PATCH 8/9] ASoC: tlv320aic31xx: Fix the reset GPIO OF name

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 573 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 8/9] ASoC: tlv320aic31xx: Fix the reset GPIO OF name

[Andrew F. Davis]

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.

Re: [PATCH 8/9] ASoC: tlv320aic31xx: Fix the reset GPIO OF name

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1310 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 8/9] ASoC: tlv320aic31xx: Fix the reset GPIO OF name

[Andrew F. Davis]

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.

Re: [PATCH 8/9] ASoC: tlv320aic31xx: Fix the reset GPIO OF name

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1629 bytes --]

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).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/9] ASoC: tlv320aic31xx: Fix typo in DT binding documentation

[Philipp Zabel]

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 = <&regulator>;
>  	SPRVDD-supply = <&regulator>;
>  	SPLVDD-supply = <&regulator>;

regards
Philipp

Re: [PATCH 4/9] ARM: dts: am335x-pepper: Fix the audio CODEC's reset pin

[Philipp Zabel]

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

Re: [PATCH 1/9] ASoC: tlv320aic31xx: Fix typo in DT binding documentation

[Andrew F. Davis]

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 = <&regulator>;
>>  	SPRVDD-supply = <&regulator>;
>>  	SPLVDD-supply = <&regulator>;
> 
> regards
> Philipp
> 

Re: [PATCH 4/9] ARM: dts: am335x-pepper: Fix the audio CODEC's reset pin

[Andrew F. Davis]

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
> 

Re: [alsa-devel] [PATCH 2/9] ASoC: tlv320aic3x: Fix typo in DT binding documentation

[Benoît Thébaudeau]

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 = <&regulator>;
>         IOVDD-supply = <&regulator>;
>         DRVDD-supply = <&regulator>;

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

Re: [alsa-devel] [PATCH 2/9] ASoC: tlv320aic3x: Fix typo in DT binding documentation

[Andrew F. Davis]

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 = <&regulator>;
>>         IOVDD-supply = <&regulator>;
>>         DRVDD-supply = <&regulator>;
> 
> 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
> 

Re: [PATCH 3/9] ASoC: cs42l56: bindings: sound: Fix reset GPIO name in example DT binding

[Rob Herring]

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
>