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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 24 Jun 2024 04:51:39 -0400 (EDT)
From: Elinor Montmasson <elinor.montmasson@...oirfairelinux.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>, 
	Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Shawn Guo <shawnguo@...nel.org>, 
	Sascha Hauer <s.hauer@...gutronix.de>, 
	Fabio Estevam <festevam@...il.com>, 
	Russell King <linux@...linux.org.uk>, 
	Catalin Marinas <catalin.marinas@....com>, 
	Will Deacon <will@...nel.org>, Jaroslav Kysela <perex@...ex.cz>, 
	Takashi Iwai <tiwai@...e.com>, 
	shengjiu wang <shengjiu.wang@...il.com>, 
	Xiubo Lee <Xiubo.Lee@...il.com>, 
	Nicolin Chen <nicoleotsuka@...il.com>, 
	Pengutronix Kernel Team <kernel@...gutronix.de>, 
	linux-sound <linux-sound@...r.kernel.org>, 
	devicetree <devicetree@...r.kernel.org>, imx@...ts.linux.dev, 
	linux-arm-kernel@...ts.infradead.org, 
	linux-kernel <linux-kernel@...r.kernel.org>, 
	alsa-devel <alsa-devel@...a-project.org>, 
	linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>, 
	Philip-Dylan Gleonec <philip-dylan.gleonec@...oirfairelinux.com>
Subject: Re: [PATCHv5 6/9] ASoC: dt-bindings: fsl-asoc-card: add compatible
 string for spdif

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