Re: [PATCH v3] ASoC: cs43130: Take the frame layout from set_tdm_slot()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Medium] ASoC: cs43130: unchecked error returns in cs43130_hw_params()
  [1]
--

commit 7ef1cbcfb870223ab41a2a71fc03df6192352302
Author: Olivier Louvignes <olivier@mgcrea.io>

ASoC: cs43130: Take the frame layout from set_tdm_slot()

This commit adds a set_tdm_slot() callback to the cs43130 PCM DAI so
cards can specify the TDM frame layout directly instead of deriving the
slot width from the set_sysclk() frequency.

> diff --git a/sound/soc/codecs/cs43130.c b/sound/soc/codecs/cs43130.c
> index 7fb23522f081c..3a1588645b405 100644
> --- a/sound/soc/codecs/cs43130.c
> +++ b/sound/soc/codecs/cs43130.c

[ ... ]

> @@ -960,18 +967,24 @@ static int cs43130_hw_params(struct snd_pcm_substream *substream,
>  		break;
>  	}
>  
> -	if (!sclk && cs43130->dais[dai->id].dai_mode == SND_SOC_DAIFMT_CBP_CFP)
> -		/* Calculate SCLK in master mode if unassigned */
> -		sclk = params_rate(params) * bitwidth_dai *
> -		       params_channels(params);
> +	if (cs43130->dais[dai->id].slot_width) {
> +		/* The slot width set with set_tdm_slot() */
> +		bitwidth_sclk = cs43130->dais[dai->id].slot_width;
> +	} else {
> +		if (!sclk && cs43130->dais[dai->id].dai_mode == SND_SOC_DAIFMT_CBP_CFP)
> +			/* Calculate SCLK in master mode if unassigned */
> +			sclk = params_rate(params) * bitwidth_dai *
> +			       params_channels(params);
> +
> +		if (!sclk) {
> +			/* at this point, SCLK must be set */
> +			dev_err(cs43130->dev, "SCLK freq is not set\n");
> +			return -EINVAL;
> +		}
>  
> -	if (!sclk) {
> -		/* at this point, SCLK must be set */
> -		dev_err(cs43130->dev, "SCLK freq is not set\n");
> -		return -EINVAL;
> +		bitwidth_sclk = (sclk / params_rate(params)) /
> +				params_channels(params);
>  	}
> -
> -	bitwidth_sclk = (sclk / params_rate(params)) / params_channels(params);
>  	if (bitwidth_sclk < bitwidth_dai) {
>  		dev_err(cs43130->dev, "Format not supported: SCLK freq is too low\n");
>  		return -EINVAL;

[Severity: Medium]
This problem wasn't introduced by this patch, but should cs43130_hw_params()
check the return values of its configuration helpers [1]?

sound/soc/codecs/cs43130.c:cs43130_hw_params() {
	...
			cs43130_set_pll(component, 0, 0, cs43130->mclk, required_clk);
			if (cs43130->pll_bypass)
				cs43130_change_clksrc(component, CS43130_MCLK_SRC_EXT);
			else
				cs43130_change_clksrc(component, CS43130_MCLK_SRC_PLL);
	...
	cs43130_set_bitwidth(dai->id, bitwidth_dai, cs43130->regmap);
	cs43130_set_sp_fmt(dai->id, bitwidth_sclk, params, cs43130);

	return 0;
}

If cs43130_set_sp_fmt() rejects frm_size or fails to find a matching
cs43130_get_clk_gen() entry, it returns -EINVAL after writing the frame and
clock configuration registers but before programming the clock generator.
Similarly, cs43130_set_pll(), cs43130_change_clksrc(), and
cs43130_set_bitwidth() can fail with -EINVAL or -ETIMEDOUT, yet
cs43130_hw_params() still returns 0 and leaves the codec unclocked or
partially configured.

[1] https://sashiko.dev/#/bug/linux-d87cb6e4-e153-44f3-bf43-c8028f4ce12f

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109174514.84903-1-olivier@mgcrea.io?part=1