Re: [PATCH 2/2] ASoC: xilinx: xlnx_i2s: Discover parameters from registers

[Sean Anderson <sean.anderson@linux.dev>]

On 1/29/26 13:09, Mark Brown wrote:
> On Thu, Jan 29, 2026 at 12:46:27PM -0500, Sean Anderson wrote:
>> On 1/29/26 12:27, Mark Brown wrote:
>> > On Thu, Jan 29, 2026 at 12:23:15PM -0500, Sean Anderson wrote:
> 
>> >> -	ret = of_property_read_u32(node, "xlnx,num-channels", &drv_data->channels);
> 
>> >> -	ret = of_property_read_u32(node, "xlnx,dwidth", &drv_data->data_width);
> 
>> > Given that the properties already exist it seems wise to continue to
>> > parse them if available and prefer them over what we read from the
>> > hardware, it would not shock me to discover that hardware exists where
>> > the registers are inaccurate or need overriding due to bugs.
> 
>> I would be surprised if such hardware exists. These properties are
>> automatically generated by Xilinx's tools based on the HDL core's
>> properties. This has a few consequences:
> 
>> - They always exactly match the hardware unless someone has gone in and
>>   modified them. I think this is unlikely in this case because they
>>   directly reflect parameters that should not need to be adjusted.
>> - Driver authors tend to use them even when there are hardware registers
>>   available with the same information, as Xilinx has not always been
>>   consistent in adding such registers.
> 
> I'm not sure I follow your second point - driver authors tend to use
> what?  

Authors look at the devicetree node and see something like 

	i2s0_tx: i2s_transmitter@80120000 {
		aud_mclk = <99999001>;
		clock-names = "aud_mclk", "s_axi_ctrl_aclk", "s_axis_aud_aclk";
		clocks = <&zynqmp_clk 74>, <&zynqmp_clk 71>, <&zynqmp_clk 71>;
		compatible = "xlnx,i2s-transmitter-1.0", "xlnx,i2s-transmitter-1.0";
		interrupt-names = "irq";
		interrupt-parent = <&gic>;
		interrupts = <0 105 4>;
		reg = <0x0 0x80120000 0x0 0x10000>;
		xlnx,depth = <0x80>;
		xlnx,dwidth = <0x18>;
		xlnx,num-channels = <0x1>;
		xlnx,snd-pcm = <&i2s0_dma>;
	};

and go "Ah, there are the properties I need." On some Xilinx cores this
is the only way to discover certain properties, so people have gotten into
the habit of using them even when these properties can be read from the
device itself.

>> I am not aware of any errata regarding incorrect generation of
>> properties for this device or cases where the number of channels or bit
>> depth was incorrect.
> 
> I'd still rather see the properties get used if present, worst case
> they're redundant best case we avoid regressing a currently working
> system.  The code is already there, it just needs tweaking to make parse
> failures non-fatal.

I would rather remove it for the code size reduction and simplication.

--Sean