Re: [PATCH v2 3/3] arm64: dts: qcom: sm8650-hdk: Enable I2S for HDMI

On 7/31/26 10:22, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] The SoC's native I2S and MCLK pinctrl configurations (`i2s0_default_state`, `audio_mclk0_default_state`) are incorrectly placed in the `pinctrl-0` property of the external `lt9611_codec` I2C bridge node, instead of being managed by the sound card or CPU DAI.
> - [Low] The `dai@16` node uses an incorrect hexadecimal unit address of `16` (22 in decimal), which does not match the decimal value of its `reg` property `PRIMARY_MI2S_RX` (16, which is 0x10). The node should be named `dai@10`.
> --
> 
> --- Patch [3]: [PATCH v2 3/3] arm64: dts: qcom: sm8650-hdk: Enable I2S for HDMI ---
> commit b95de62e9eeaef3502a18beec3e036edf4b8259f
> Author: Neil Armstrong <[email protected]>
> 
> arm64: dts: qcom: sm8650-hdk: Enable I2S for HDMI
> 
> Add the necessary nodes to configure the right I2S interface
> to output audio via the DSI HDMI bridge.
> 
>> diff --git a/arch/arm64/boot/dts/qcom/sm8650-hdk.dts b/arch/arm64/boot/dts/qcom/sm8650-hdk.dts
>> index eabc828c05b4c..b62d387b9afa3 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8650-hdk.dts
>> +++ b/arch/arm64/boot/dts/qcom/sm8650-hdk.dts
> 
> [ ... ]
> 
>> @@ -861,7 +878,10 @@ lt9611_codec: hdmi-bridge@2b {
>>   		vdd-supply = <&lt9611_1v2>;
>>   		vcc-supply = <&lt9611_3v3>;
>>   
>> -		pinctrl-0 = <&lt9611_irq_pin>, <&lt9611_rst_pin>;
>> +		pinctrl-0 = <&lt9611_irq_pin>,
>> +			    <&lt9611_rst_pin>,
>> +			    <&i2s0_default_state>,
>> +			    <&audio_mclk0_default_state>;
> 
> [Severity: Medium]
> Does this place the SoC's native I2S and MCLK pinctrl configurations in the
> external I2C bridge node?
> 
> By placing the SoC I2S pin states in the default property of the external I2C
> codec node, the pins might be unconditionally driven from the moment the I2C
> bridge driver probes. This could prevent the ALSA/DAPM subsystem from toggling
> the pins based on active audio streams.
> 
> Should these be managed by the sound card or CPU DAI instead?


No

> 
>>   		pinctrl-names = "default";
>>   
>>   		ports {
> 
> [ ... ]
> 
>> @@ -1072,6 +1092,19 @@ &pon_resin {
>>   	status = "okay";
>>   };
>>   
>> +&q6apmbedai {
>> +	#address-cells = <1>;
>> +	#size-cells = <0>;
>> +
>> +	dai@16 {
> 
> [Severity: Low]
> Does this trigger a unit_address_vs_reg Device Tree Compiler warning?
> 
> Since PRIMARY_MI2S_RX is 16 in decimal (0x10), should the node be named
> dai@10 to match the hex-formatted first address of the reg property?

Damn !

> 
>> +		reg = <PRIMARY_MI2S_RX>;
>> +		clocks = <&q6prmcc LPASS_CLK_ID_PRI_MI2S_IBIT LPASS_CLK_ATTRIBUTE_COUPLE_NO>,
>> +			 <&q6prmcc LPASS_CLK_ID_MCLK_1 LPASS_CLK_ATTRIBUTE_COUPLE_NO>;
>> +		clock-names = "bclk",
>> +			      "mclk";
>> +	};
>> +};
>