Re: [PATCH v3 2/4] ASoC: cdns: Add Cadence I2S-SC controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] asoc: inverted master/slave clock provider configuration in set_fmt
- [High] regmap: racy read-modify-write on volatile interrupt status register I2S_STAT
- [Medium] pm: missing devm action or .remove callback leaves clocks permanently enabled
- [Low] kconfig: missing select REGMAP_MMIO causes build failure
--

commit c5bda621f50576e786b53bd1505239571117bd15
Author: Joakim Zhang <joakim.zhang@cixtech.com>
Subject: ASoC: cdns: Add Cadence I2S-SC controller driver

Add support for the Cadence I2S-SC controller found in the CIX SKY1
audio subsystem.

> diff --git a/sound/soc/cdns/Kconfig b/sound/soc/cdns/Kconfig
> new file mode 100644
> index 0000000000000..631e2e0764cb9
> --- /dev/null
> +++ b/sound/soc/cdns/Kconfig
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +config SND_SOC_CDNS_I2S_SC
> +	tristate "Cadence I2S-SC controller support"
> +	depends on ARCH_CIX || COMPILE_TEST
> +	select SND_SOC_GENERIC_DMAENGINE_PCM

[Severity: Low]
Since this driver unconditionally calls devm_regmap_init_mmio() to manage its
registers, should this also select REGMAP_MMIO?

If it is built (e.g., via COMPILE_TEST) without another driver selecting it,
it could cause an undefined reference to __devm_regmap_init_mmio_clk during
the link stage.

> diff --git a/sound/soc/cdns/cdns-i2s-sc.c b/sound/soc/cdns/cdns-i2s-sc.c
> new file mode 100644
> index 0000000000000..160a0ec573d72
> --- /dev/null
> +++ b/sound/soc/cdns/cdns-i2s-sc.c
[ ... ]
> +static irqreturn_t cdns_i2s_sc_isr(int irq, void *devid)
> +{
> +	struct cdns_i2s_sc_priv *i2s_sc_priv = (struct cdns_i2s_sc_priv *)devid;
> +	struct device *dev = i2s_sc_priv->dev;
> +	u32 stat;
> +
> +	if (regmap_read(i2s_sc_priv->regmap, I2S_STAT, &stat))
> +		return IRQ_NONE;
> +
> +	stat &= I2S_STAT_TDATA_UNDERR | I2S_STAT_RDATA_OVERR;
> +	if (!stat)
> +		return IRQ_NONE;
> +
> +	/* Clear the status */
> +	regmap_update_bits(i2s_sc_priv->regmap, I2S_STAT, stat, 0);

[Severity: High]
Is it safe to use regmap_update_bits() here to clear the interrupt status?

I2S_STAT is correctly marked as a volatile register in the regmap
configuration, but regmap_update_bits() performs a non-atomic physical
read-modify-write cycle.

If the hardware uses Write-1-to-Clear (W1C) or Write-0-to-Clear (W0C)
semantics, this read-modify-write might accidentally clear unhandled events
or permanently drop new interrupts asserted between the read and write.

Could we use a direct regmap_write() here instead?

[ ... ]
> +static int cdns_i2s_sc_set_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt)
> +{
[ ... ]
> +	/* DAI hardware clock masters */
> +	switch (fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) {
> +	case SND_SOC_DAIFMT_CBC_CFC:
> +		ctrl &= ~I2S_CTRL_MS_CFG;
> +		i2s_sc_priv->is_master_mode = false;
> +		break;
> +	case SND_SOC_DAIFMT_CBP_CFP:
> +		ctrl |= I2S_CTRL_MS_CFG;
> +		i2s_sc_priv->is_master_mode = true;
> +		break;

[Severity: High]
Are these clock provider configurations inverted?

In the ASoC framework, SND_SOC_DAIFMT_CBC_CFC means the Codec is the
Bitclock/Frame Consumer, which makes the CPU DAI (this controller) the
Provider (Master).

SND_SOC_DAIFMT_CBP_CFP means the Codec is the Provider, so the CPU DAI must
be the Consumer (Slave).

The driver logic seems to do the exact opposite. Will this cause bus
contention by actively driving clocks when the codec is already doing so?

[ ... ]
> +static int cdns_i2s_sc_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	/*
> +	 * Let pm_runtime_put_sync() disable the clocks, and take regcache effect.
> +	 * If CONFIG_PM is not enabled, the clock will stay powered.
> +	 */
> +	pm_runtime_put_sync(&pdev->dev);
> +
> +	ret = devm_snd_soc_register_component(&pdev->dev, &cdns_i2s_sc_component,
> +					      &cdns_i2s_sc_dai, 1);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "Failed to register soc component\n");

[Severity: Medium]
If devm_snd_soc_register_component() fails here, the probe function returns
directly, bypassing the err_disable_clks label.

Does this leak the clocks if CONFIG_PM is disabled (since
pm_runtime_put_sync() would be a no-op)?

[ ... ]
> +static struct platform_driver cdns_i2s_sc_driver = {
> +	.probe = cdns_i2s_sc_probe,
> +	.driver = {
> +		.name = DRV_NAME,
> +		.pm = pm_ptr(&cdns_i2s_sc_pm_ops),
> +		.of_match_table = cdns_i2s_sc_of_match,
> +	},
> +};

[Severity: Medium]
I noticed there is no .remove callback or devm_add_action_or_reset() cleanup
for the hardware clocks enabled during probe.

During driver unbind, the driver core calls pm_runtime_get_sync() (waking the
device and enabling clocks if they were suspended) followed by
pm_runtime_disable().

Without a .remove callback to force suspend or manually disable them, does
this leave the clocks permanently enabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009092552.2377960-1-joakim.zhang@cixtech.com?part=2