Re: [PATCH] sound/soc/codecs/wm8978: add missing BCLK divider setup

[Charles Keepax <ckeepax@opensource.cirrus.com>]

On Fri, Oct 03, 2025 at 05:13:04PM +0800, Brian Sune wrote:

Patch title should be tweaked to match the other patches to this
driver:

ASoC: wm8978: ...

> The original WM8978 codec driver did not set the BCLK (bit clock)
> divider, which can cause audio clocks to be incorrect or unstable
> depending on the sample rate and word length.

This isn't totally accurate, the driver expects it will be set
through the set_clkdiv callback. Which one could probably argue
is a bit of a more legacy approach, but probably worth calling
that out here.

> This patch adds proper calculation and configuration of the BCLK
> divider based on the sample rate and word width, ensuring the
> codec generates the correct bit clock for all supported rates.
> 
> Signed-off-by: Brian Sune <briansune@gmail.com>
> ---
>  sound/soc/codecs/wm8978.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/sound/soc/codecs/wm8978.c b/sound/soc/codecs/wm8978.c
> index 8c45ba6fc4c3..2109c84f33df 100644
> --- a/sound/soc/codecs/wm8978.c
> +++ b/sound/soc/codecs/wm8978.c
> @@ -717,6 +717,11 @@ static int wm8978_hw_params(struct snd_pcm_substream *substream,
>  			    struct snd_pcm_hw_params *params,
>  			    struct snd_soc_dai *dai)
>  {
> +	unsigned int bclk, bclkdiv = 0, min_diff = UINT_MAX;
> +	unsigned int target_bclk = params_rate(params) * params_width(params) * 2;

Probably better to use snd_soc_params_to_bclk or similar helper.

> +	/* WM8978 supports divisors */
> +	static const int bclk_divs[] = {1, 2, 4, 8, 16, 32};
> +
>  	struct snd_soc_component *component = dai->component;
>  	struct wm8978_priv *wm8978 = snd_soc_component_get_drvdata(component);
>  	/* Word length mask = 0x60 */
> @@ -820,6 +825,21 @@ static int wm8978_hw_params(struct snd_pcm_substream *substream,
>  	/* MCLK divisor mask = 0xe0 */
>  	snd_soc_component_update_bits(component, WM8978_CLOCKING, 0xe0, best << 5);
>  
> +	for (i = 0; i < ARRAY_SIZE(bclk_divs); i++) {
> +		bclk = wm8978->f_256fs / bclk_divs[i];
> +		if (abs(bclk - target_bclk) < min_diff) {
> +			min_diff = abs(bclk - target_bclk);
> +			bclkdiv = i;
> +		}
> +	}
> +
> +	dev_dbg(component->dev, "%s: fs=%u width=%u -> target BCLK=%u, using div #%u\n",
> +		__func__, params_rate(params), params_width(params), target_bclk,
> +		bclk_divs[bclkdiv]);
> +
> +	/* BCLKDIV divisor mask = 0x1c */
> +	snd_soc_component_update_bits(component, WM8978_CLOCKING, 0x1c, bclkdiv << 2);

We should probably add something to handle the interaction with
set_clkdiv and skip this if a fixed divider has been set. Well
either that or remove the set_clkdiv option, although that is a
little braver.

Thanks,
Charles