Re: [PATCHv5 6/9] ASoC: dt-bindings: fsl-asoc-card: add compatible string for spdif

[Elinor Montmasson <elinor.montmasson@savoirfairelinux.com>]

From: "Krzysztof Kozlowski" <krzk@kernel.org>
Sent: Sunday, 23 June, 2024 13:07:46
> On 20/06/2024 15:25, Elinor Montmasson wrote:
>> The S/PDIF audio card support was merged from imx-spdif into the
>> fsl-asoc-card driver, making it possible to use an S/PDIF with an ASRC.
>> Add the new compatible and update properties.
> 
> Please use standard email subjects, so with the PATCH keyword in the
> title. `git format-patch -v5` helps here to create proper versioned
> patches. Another useful tool is b4.
>


Acknowledged, I did not know this option and will be careful about it
for next versions.


>> @@ -33,6 +33,7 @@ properties:
>>        - items:
>>            - enum:
>>                - fsl,imx-sgtl5000
>> +              - fsl,imx-sabreauto-spdif
>>                - fsl,imx25-pdk-sgtl5000
>>                - fsl,imx53-cpuvo-sgtl5000
>>                - fsl,imx51-babbage-sgtl5000
>> @@ -54,6 +55,7 @@ properties:
>>                - fsl,imx6q-ventana-sgtl5000
>>                - fsl,imx6sl-evk-wm8962
>>                - fsl,imx6sx-sdb-mqs
>> +              - fsl,imx6sx-sdb-spdif
>>                - fsl,imx6sx-sdb-wm8962
>>                - fsl,imx7d-evk-wm8960
>>                - karo,tx53-audio-sgtl5000
>> @@ -65,6 +67,7 @@ properties:
>>                - fsl,imx-audio-sgtl5000
>>                - fsl,imx-audio-wm8960
>>                - fsl,imx-audio-wm8962
>> +              - fsl,imx-audio-spdif
> 
> This does not look right. It's quite generic, so now you allow any
> variant to be used with this fallback.
> 
> Please do not grow more this list of all possible combinations and
> instead add specific lists. Otherwise, please explain why this is valid
> hardware:
> "fsl,imx7d-evk-wm8960", "fsl,imx-audio-spdif"


I wanted to follow the current style of this documentation file,
but I agree it's better to prevent using "fsl,imx-audio-spdif"
with any compatible, I can use a specific list for the next version.


> 
> 
>>        - items:
>>            - enum:
>>                - fsl,imx-audio-ac97
>> @@ -81,6 +84,7 @@ properties:
>>                - fsl,imx-audio-wm8960
>>                - fsl,imx-audio-wm8962
>>                - fsl,imx-audio-wm8958
>> +              - fsl,imx-audio-spdif
> 
> Fallbacks should not be used alone. Why this is needed?


"fsl,imx-audio-spdif" is used alone in most DTS files.
"fsl,imx-sabreauto-spdif" and "fsl,imx6sx-sdb-spdif" are used with
"fsl,imx-audio-spdif" in only 2 specific DTS files. 


> The compatible is already documented, so now you create duplicated binding.
> 
> This is very confusing.


The double compatible documentation is only temporary, next commit (7/9)
removes the previous binding in "fsl,imx-audio-spdif.yaml".
I separated these changes in multiple commit to ease git history searching
by subject/file.
If required, I can merge commits 6/9 and 7/9.


>> @@ -195,3 +208,12 @@ examples:
>>               "AIN2L", "Line In Jack",
>>               "AIN2R", "Line In Jack";
>>      };
>> +
>> +  - |
>> +    sound-spdif-asrc {
>> +      compatible = "fsl,imx-audio-spdif";
>> +      model = "spdif-asrc-audio";
>> +      audio-cpu = <&spdif>;
>> +      audio-asrc = <&easrc>;
>> +      audio-codec = <&spdifdit>, <&spdifdir>;
>> +    };
> 
> Do not introduce another indentation style. Look what is above.


Ack, I'll correct this for next version.


Best regards,
Elinor Montmasson