Re: [PATCH v5 4/4] ASoC: cdns: Add Cadence I2S-MC controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] ASoC: cdns: premature -EBUSY in cdns_i2s_mc_hw_params() when
  peer DAI is open
- [Low] ASoC: cdns: i2s-mc: un-gated data pin clocks in
  cdns_i2s_mc_dai_probe()
--

commit 020a5d5318e146daa2dd788c38c07f592ec9d4bb
Author: Joakim Zhang <joakim.zhang@cixtech.com>

ASoC: cdns: Add Cadence I2S-MC controller driver

This commit adds an ASoC driver for the Cadence I2S-MC multi-channel
controller in the CIX SKY1 audio subsystem.

> diff --git a/sound/soc/cdns/cdns-i2s-mc.c b/sound/soc/cdns/cdns-i2s-mc.c
> new file mode 100644
> index 0000000000000..5bbac98101449
> --- /dev/null
> +++ b/sound/soc/cdns/cdns-i2s-mc.c
> @@ -0,0 +1,1152 @@

[ ... ]

> +static bool cdns_i2s_mc_other_dai_active(struct snd_soc_dai *cpu_dai)
> +{
> +	struct snd_soc_dai *dai;
> +
> +	for_each_component_dais(cpu_dai->component, dai) {
> +		if (dai == cpu_dai)
> +			continue;
> +		if (snd_soc_dai_active(dai))
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static int cdns_i2s_mc_hw_params(struct snd_pcm_substream *substream,
> +				 struct snd_pcm_hw_params *params,
> +				 struct snd_soc_dai *cpu_dai)
> +{
> +	struct cdns_i2s_mc_priv *i2s_mc_priv = snd_soc_dai_get_drvdata(cpu_dai);
> +	struct device *dev = i2s_mc_priv->dev;
> +	u32 rate, sample_rate = 0;
> +	u32 slots, slot_width, resolution, ctrl;
> +	unsigned long i2s_clk_rate;
> +	u8 pin_tx_num, pin_rx_num;
> +	struct clk *clk_parent;
> +	bool is_master_mode;
> +	int ret;
> +
> +	rate = params_rate(params);
> +	slot_width = i2s_mc_priv->devtype_data->data_width;
> +	dev_dbg(dev, "rate = %d, slot_width = %d\n", rate, slot_width);
> +
> +	/* switch clk mux to select the appropriate clk parent */
> +	if (rate % 8000 == 0) {
> +		/* Sampling rate is a multiple of 8KHz, select "audio-clk0" */
> +		clk_parent = i2s_mc_priv->clks[AUDIO_CLK0];
> +	} else if (rate % 11025 == 0) {
> +		/* Sampling rate is a multiple of 11.025KHz, select "audio-clk2" */
> +		clk_parent = i2s_mc_priv->clks[AUDIO_CLK2];
> +	} else {
> +		dev_err(i2s_mc_priv->dev, "Invalid sample rate\n");
> +		return -EINVAL;
> +	}
> +
> +	if (cdns_i2s_mc_other_dai_active(cpu_dai) &&
> +	    !clk_is_match(clk_get_parent(i2s_mc_priv->clk_i2s), clk_parent)) {
> +		dev_err(dev,
> +			"i2s clock parent conflicts with active stream on the other DAI\n");
> +		return -EBUSY;
> +	}

[Severity: High]
Does checking snd_soc_dai_active(dai) here cause cdns_i2s_mc_hw_params() to
fail with -EBUSY whenever both playback and capture substreams are opened
before hw_params is configured?

__soc_pcm_open() increments the DAI active count via
snd_soc_runtime_activate() at open time and keeps it non-zero across
soc_pcm_hw_free() until close.  When an audio server opens both playback
and capture PCM substreams before calling snd_pcm_hw_params() on either,
both DAIs are already marked active; if the requested rate requires
switching clk_i2s to the other parent clock, cdns_i2s_mc_hw_params()
rejects both streams with -EBUSY.  Should cdns_i2s_mc_other_dai_active()
check whether the peer DAI actually has hardware parameters configured (for
example via snd_soc_dai_get_symmetric_rate(dai) or driver state tracked in
hw_params and hw_free) instead of snd_soc_dai_active()?

[ ... ]

> +static int cdns_i2s_mc_dai_probe(struct snd_soc_dai *cpu_dai)
> +{
> +	struct cdns_i2s_mc_priv *i2s_mc_priv = snd_soc_dai_get_drvdata(cpu_dai);
> +	u32 ctrl = 0, cid_ctrl = 0;
> +
> +	/*
> +	 * Transceiver disable
> +	 * Transceiver clock disable
> +	 * All interrupts masked
> +	 * Reset for transmitter synchronizing unit
> +	 * Reset for receiver synchronizing unit
> +	 */
> +	ctrl |= FIELD_PREP(I2S_CTRL_I2S_EN, 0);
> +	ctrl &= ~(I2S_CTRL_TSYNC_RST | I2S_CTRL_RSYNC_RST);
> +	cid_ctrl |= I2S_CID_CTRL_STROBE_TS | I2S_CID_CTRL_STROBE_RS;
> +	cid_ctrl &= ~I2S_CID_CTRL_INTREQ_MASK;
> +
> +	regmap_update_bits(i2s_mc_priv->regmap, I2S_CTRL,
> +			   I2S_CTRL_I2S_EN | I2S_CTRL_TSYNC_RST | I2S_CTRL_RSYNC_RST, ctrl);
> +	regmap_update_bits(i2s_mc_priv->regmap, I2S_CID_CTRL,
> +			   I2S_CID_CTRL_STROBE_TS | I2S_CID_CTRL_STROBE_RS |
> +			   I2S_CID_CTRL_INTREQ_MASK, cid_ctrl);

[Severity: Low]
Should cdns_i2s_mc_dai_probe() also set I2S_CID_CTRL_I2S_STROBE in
I2S_CID_CTRL to gate the per-pin clocks initially?

I2S_CID_CTRL resets to 0x00000000 (all clock strobes enabled), and
cdns_i2s_mc_rxtx_config() only clears and sets the I2S_CID_CTRL_I2S_STROBE
bits matching pin_tx_mask_adjust or pin_rx_mask_adjust, leaving unwired or
unused pins with their clock gates enabled.

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