From: "Krzysztof Kozlowski" <krzk@xxxxxxxxxx>
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
