| Message ID | 20171108212505.28320-3-afd@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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>;
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(-)