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

[Ajay Kumar Nandam <ajay.nandam@oss.qualcomm.com>]

On 9/28/2026 11:06 AM, joakim.zhang@cixtech.com wrote:
> From: Joakim Zhang <joakim.zhang@cixtech.com>
> 
> Add support for the Cadence I2S-SC controller found in the CIX SKY1
> audio subsystem.
> 
> The controller provides full-duplex stereo playback and capture in
> standard I2S, left/right-justified and DSP modes, with mono operation
> mapped onto the left audio channel. It also supports TDM operation
> with up to 16 slots per frame, where the PCM channels map onto the
> active slots of the stream direction while the bit clock covers the
> whole frame.
> 
> The transmitter and receiver share the transceiver enable sequence,
> so the start/stop state machine is serialized with a spinlock against
> independently triggered playback and capture PCMs. The driver
> registers the dmaengine PCM helper, selects the audio reference clock
> parent for the 8 kHz or 11.025 kHz sample rate family and keeps the
> minimum functional-clock to bit-clock ratio required for safe clock
> domain crossing. Runtime and system suspend/resume restore the
> registers through the regcache.
> 
> Signed-off-by: Joakim Zhang <joakim.zhang@cixtech.com>
> ---

> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <sound/dmaengine_pcm.h>
> +#include <sound/pcm_params.h>
> +#include <sound/soc.h>
> +

> +static int cdns_i2s_sc_clks_enable(struct cdns_i2s_sc_priv *i2s_sc_priv)
> +{
> +	int ret;
> +
> +	ret = clk_prepare_enable(i2s_sc_priv->clk_hst);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(i2s_sc_priv->clk_i2s);
> +	if (ret)
> +		clk_disable_unprepare(i2s_sc_priv->clk_hst);
> +
> +	return ret;
> +}
> +
> +static void cdns_i2s_sc_clks_disable(struct cdns_i2s_sc_priv *i2s_sc_priv)
> +{
> +	clk_disable_unprepare(i2s_sc_priv->clk_i2s);
> +	clk_disable_unprepare(i2s_sc_priv->clk_hst);
> +}
> +
> +static void cdns_i2s_sc_rxtx_common_config(struct cdns_i2s_sc_priv *i2s_sc_priv, bool on)
> +{
> +	if (on) {
> +		/* Full-duplex mode enable */
> +		regmap_update_bits(i2s_sc_priv->regmap, I2S_CTRL_FDX,
> +				   I2S_CTRL_FDX_FULL_DUPLEX, I2S_CTRL_FDX_FULL_DUPLEX);
> +
> +		if (i2s_sc_priv->is_tdm_mode) {
> +			/* TDM mode enable */
> +			regmap_update_bits(i2s_sc_priv->regmap, I2S_TDM_CTRL,
> +					   I2S_TDM_CTRL_TDM_EN, I2S_TDM_CTRL_TDM_EN);
> +
> +			/* Number of supported audio channels in TDM mode */
> +			regmap_update_bits(i2s_sc_priv->regmap, I2S_TDM_CTRL,
> +					   I2S_TDM_CTRL_CHN_NO,
> +					   FIELD_PREP(I2S_TDM_CTRL_CHN_NO,
> +						      i2s_sc_priv->tdm_config.slots - 1));

This driver uses FIELD_PREP() in several register programming paths, but 
it does not include <linux/bitfield.h>. A local targeted build with

  M=sound/soc/cdns W=1 fails with:

  error: implicit declaration of function 'FIELD_PREP'

Please add the explicit bitfield.h include. After adding that include 
locally, cdns-i2s-sc.c gets past this compile error.

> +
> +			/* TDM mode channels enable */
> +			regmap_update_bits(i2s_sc_priv->regmap, I2S_TDM_CTRL, I2S_TDM_CTRL_CHN_EN,
> +					   FIELD_PREP(I2S_TDM_CTRL_CHN_EN,
> +						      i2s_sc_priv->tdm_config.rx_mask |
> +						      i2s_sc_priv->tdm_config.tx_mask));
> +		}

> +	ret = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "Failed to register dmaengine component\n");
> +
> +	pm_runtime_get_noresume(&pdev->dev);
> +	pm_runtime_set_active(&pdev->dev);
> +	ret = devm_pm_runtime_enable(&pdev->dev);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "Failed to enable runtime PM\n");
> +
> +	ret = cdns_i2s_sc_clks_enable(i2s_sc_priv);
> +	if (ret) {
> +		dev_err_probe(&pdev->dev, ret, "Failed to enable clocks\n");
> +		pm_runtime_put_noidle(&pdev->dev);
> +		return ret;
> +	}
> +

After cdns_i2s_sc_clks_enable() succeeds, the platform_get_irq() failure 
path returns directly without disabling the clocks or dropping the 
runtime PM usage count. The devm_request_irq() failure path has a 
similar issue: it calls pm_runtime_put_noidle(), but that will not run 
the runtime suspend callback, so the clocks enabled manually above 
remain prepared/enabled.

Thanks
Ajay Kumar Nandam

> +	cdns_i2s_sc_rst(i2s_sc_priv);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +	i2s_sc_priv->irq = irq;
> +
> +	ret = devm_request_irq(&pdev->dev, irq, cdns_i2s_sc_isr, 0,
> +			       dev_name(&pdev->dev), i2s_sc_priv);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to request irq: %d\n", ret);
> +		pm_runtime_put_noidle(&pdev->dev);
> +		return ret;
> +	}