Xilinx helpfully included a read-only "config" register that contains
configuration parameters. Discover our parameters from this register
instead of reading them from the device tree.
Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---
sound/soc/xilinx/xlnx_i2s.c | 32 +++++++++++---------------------
1 file changed, 11 insertions(+), 21 deletions(-)
diff --git a/sound/soc/xilinx/xlnx_i2s.c b/sound/soc/xilinx/xlnx_i2s.c
index ca915a001ad5..a2b426376676 100644
--- a/sound/soc/xilinx/xlnx_i2s.c
+++ b/sound/soc/xilinx/xlnx_i2s.c
@@ -17,6 +17,9 @@
#define DRV_NAME "xlnx_i2s"
+#define I2S_CORE_CFG 0x04
+#define I2S_CORE_CFG_DATA_24BIT BIT(16)
+#define I2S_CORE_CFG_CHANNELS GENMASK(11, 8)
#define I2S_CORE_CTRL_OFFSET 0x08
#define I2S_CORE_CTRL_32BIT_LRCLK BIT(3)
#define I2S_CORE_CTRL_ENABLE BIT(0)
@@ -172,7 +175,7 @@ static int xlnx_i2s_probe(struct platform_device *pdev)
{
struct xlnx_i2s_drv_data *drv_data;
int ret;
- u32 format;
+ u32 format, cfg;
struct device *dev = &pdev->dev;
struct device_node *node = dev->of_node;
@@ -184,27 +187,14 @@ static int xlnx_i2s_probe(struct platform_device *pdev)
if (IS_ERR(drv_data->base))
return PTR_ERR(drv_data->base);
- ret = of_property_read_u32(node, "xlnx,num-channels", &drv_data->channels);
- if (ret < 0) {
- dev_err(dev, "cannot get supported channels\n");
- return ret;
- }
- drv_data->channels *= 2;
-
- ret = of_property_read_u32(node, "xlnx,dwidth", &drv_data->data_width);
- if (ret < 0) {
- dev_err(dev, "cannot get data width\n");
- return ret;
- }
- switch (drv_data->data_width) {
- case 16:
- format = SNDRV_PCM_FMTBIT_S16_LE;
- break;
- case 24:
+ cfg = readl(drv_data->base + I2S_CORE_CFG);
+ drv_data->channels = FIELD_GET(I2S_CORE_CFG_CHANNELS, cfg);
+ if (cfg & I2S_CORE_CFG_DATA_24BIT) {
+ drv_data->data_width = 24;
format = SNDRV_PCM_FMTBIT_S24_LE;
- break;
- default:
- return -EINVAL;
+ } else {
+ drv_data->data_width = 16;
+ format = SNDRV_PCM_FMTBIT_S16_LE;
}
if (of_device_is_compatible(node, "xlnx,i2s-transmitter-1.0")) {
--
2.35.1.1320.gc452695387.dirtyHi Sean,
kernel test robot noticed the following build errors:
[auto build test ERROR on broonie-sound/for-next]
[also build test ERROR on broonie-spi/for-next linus/master v6.19-rc7 next-20260129]
[cannot apply to xilinx-xlnx/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Sean-Anderson/dt-bindings-sound-xlnx-i2s-Make-discoverable-parameters-optional/20260130-012955
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
patch link: https://lore.kernel.org/r/20260129172315.3871602-3-sean.anderson%40linux.dev
patch subject: [PATCH 2/2] ASoC: xilinx: xlnx_i2s: Discover parameters from registers
config: sh-allyesconfig (https://download.01.org/0day-ci/archive/20260130/202601301436.qPUffKmd-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 15.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260130/202601301436.qPUffKmd-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202601301436.qPUffKmd-lkp@intel.com/
All errors (new ones prefixed by >>):
sound/soc/xilinx/xlnx_i2s.c: In function 'xlnx_i2s_probe':
>> sound/soc/xilinx/xlnx_i2s.c:191:30: error: implicit declaration of function 'FIELD_GET' [-Wimplicit-function-declaration]
191 | drv_data->channels = FIELD_GET(I2S_CORE_CFG_CHANNELS, cfg);
| ^~~~~~~~~
vim +/FIELD_GET +191 sound/soc/xilinx/xlnx_i2s.c
173
174 static int xlnx_i2s_probe(struct platform_device *pdev)
175 {
176 struct xlnx_i2s_drv_data *drv_data;
177 int ret;
178 u32 format, cfg;
179 struct device *dev = &pdev->dev;
180 struct device_node *node = dev->of_node;
181
182 drv_data = devm_kzalloc(&pdev->dev, sizeof(*drv_data), GFP_KERNEL);
183 if (!drv_data)
184 return -ENOMEM;
185
186 drv_data->base = devm_platform_ioremap_resource(pdev, 0);
187 if (IS_ERR(drv_data->base))
188 return PTR_ERR(drv_data->base);
189
190 cfg = readl(drv_data->base + I2S_CORE_CFG);
> 191 drv_data->channels = FIELD_GET(I2S_CORE_CFG_CHANNELS, cfg);
192 if (cfg & I2S_CORE_CFG_DATA_24BIT) {
193 drv_data->data_width = 24;
194 format = SNDRV_PCM_FMTBIT_S24_LE;
195 } else {
196 drv_data->data_width = 16;
197 format = SNDRV_PCM_FMTBIT_S16_LE;
198 }
199
200 if (of_device_is_compatible(node, "xlnx,i2s-transmitter-1.0")) {
201 drv_data->dai_drv.name = "xlnx_i2s_playback";
202 drv_data->dai_drv.playback.stream_name = "Playback";
203 drv_data->dai_drv.playback.formats = format;
204 drv_data->dai_drv.playback.channels_min = drv_data->channels;
205 drv_data->dai_drv.playback.channels_max = drv_data->channels;
206 drv_data->dai_drv.playback.rates = SNDRV_PCM_RATE_8000_192000;
207 drv_data->dai_drv.ops = &xlnx_i2s_dai_ops;
208 } else if (of_device_is_compatible(node, "xlnx,i2s-receiver-1.0")) {
209 drv_data->dai_drv.name = "xlnx_i2s_capture";
210 drv_data->dai_drv.capture.stream_name = "Capture";
211 drv_data->dai_drv.capture.formats = format;
212 drv_data->dai_drv.capture.channels_min = drv_data->channels;
213 drv_data->dai_drv.capture.channels_max = drv_data->channels;
214 drv_data->dai_drv.capture.rates = SNDRV_PCM_RATE_8000_192000;
215 drv_data->dai_drv.ops = &xlnx_i2s_dai_ops;
216 } else {
217 return -ENODEV;
218 }
219 drv_data->is_32bit_lrclk = readl(drv_data->base + I2S_CORE_CTRL_OFFSET) &
220 I2S_CORE_CTRL_32BIT_LRCLK;
221
222 dev_set_drvdata(&pdev->dev, drv_data);
223
224 ret = devm_snd_soc_register_component(&pdev->dev, &xlnx_i2s_component,
225 &drv_data->dai_drv, 1);
226 if (ret) {
227 dev_err(&pdev->dev, "i2s component registration failed\n");
228 return ret;
229 }
230
231 dev_info(&pdev->dev, "%s DAI registered\n", drv_data->dai_drv.name);
232
233 return ret;
234 }
235
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
> - ret = of_property_read_u32(node, "xlnx,num-channels", &drv_data->channels);
> - if (ret < 0) {
> - dev_err(dev, "cannot get supported channels\n");
> - return ret;
> - }
I don't know this device at all, so i might be asking dumb
questions....
It is possible that the device supports multiple channels, but the use
case is mono, and so xlnx,num-channels is 1 in DT? Would that break
given your change?
> - ret = of_property_read_u32(node, "xlnx,dwidth", &drv_data->data_width);
Could it be the device supports 24 bits, but the use case only wants
16, and so has this property set to 16?
Andrew
On 1/29/26 12:37, Andrew Lunn wrote:
>> - ret = of_property_read_u32(node, "xlnx,num-channels", &drv_data->channels);
>> - if (ret < 0) {
>> - dev_err(dev, "cannot get supported channels\n");
>> - return ret;
>> - }
>
> I don't know this device at all, so i might be asking dumb
> questions....
>
> It is possible that the device supports multiple channels, but the use
> case is mono, and so xlnx,num-channels is 1 in DT? Would that break
> given your change?
drv_data->channels is multiplied by 2, so there is always an even number
of channels. Pairs of channels are always muxed together and AFAICT
there's no way to disable them individually.
>> - ret = of_property_read_u32(node, "xlnx,dwidth", &drv_data->data_width);
>
> Could it be the device supports 24 bits, but the use case only wants
> 16, and so has this property set to 16?
I don't think that's possible. There's an option to output 32-bit audio,
but none to reduce 24-bit audio to 16 bit.
For some perspective, this is a soft core and these properties reflect
the configuration chosen when the core was built. The data path is fixed
and these devicetree properties exist to tell the driver how the core
was configured. If you set xlnx,dwidth to 16 and the core was configured
for 24-bit audio, you will silently get 24-bit audio (and the clocks
will be incorrect).
--Sean
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.
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 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. --Sean
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 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. Does version 0.0 of this IP core have this register? Its not a new addition? Is there a synthesis option to disable this register? Andrew
On 1/29/26 14:58, Andrew Lunn 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 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. > > Does version 0.0 of this IP core have this register? Its not a new > addition? As far as I know, this register was present in 1.0 revision 0. I reviewed the changelog for the core as well as the product guide changelog and found no mention of any register additions. > Is there a synthesis option to disable this register? No. --Sean
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? > 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.
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
On Thu, Jan 29, 2026 at 01:17:45PM -0500, Sean Anderson wrote: > On 1/29/26 13:09, Mark Brown wrote: > >> - 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 > 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. Oh. If the properties are there it's reasonable and sensible to use them, them being redundant is a concern when specifying the binding but once things are there any discrepency should usually be resolved in favour of the binding. > >> 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. We're talking a couple of function calls with no error handling here, I'm not sure anyone concerned about that kind of impact is running Linux.
>>>>> "Mark" == Mark Brown <broonie@kernel.org> writes: Hi, >> 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. > Oh. If the properties are there it's reasonable and sensible to use > them, them being redundant is a concern when specifying the binding but > once things are there any discrepency should usually be resolved in > favour of the binding. I also think making the hardware registers take priority over the DTS makes sense (E.G. what this patch does), as the DTS can get out of sync with the (programmable) HW configuration if the FPGA config is changed and a DTS update is forgotten. >> I would rather remove it for the code size reduction and simplication. > We're talking a couple of function calls with no error handling here, > I'm not sure anyone concerned about that kind of impact is running > Linux. Agreed, but the HW registers should by definition always be in sync with the IP block configuration, where the DTS update involves a manual update so it may not, so the HW registers are more reliable than the DTS. -- Bye, Peter Korsgaard
+Katta, Vishal On 1/29/26 19:46, Mark Brown wrote: > On Thu, Jan 29, 2026 at 01:17:45PM -0500, Sean Anderson wrote: >> On 1/29/26 13:09, Mark Brown wrote: > >>>> - 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 > >> 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. > > Oh. If the properties are there it's reasonable and sensible to use > them, them being redundant is a concern when specifying the binding but > once things are there any discrepency should usually be resolved in > favour of the binding. Let me add our driver owner of this device to answer some questions. Katta: Can you please look at it? Thanks, Michal
© 2016 - 2026 Red Hat, Inc.