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@xxxxxx> 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@xxxxxx>
>> ---
>>  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
>