Re: [PATCH] ASoC: wm8960: Fix clock configuration on slave mode

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

On Tue, Sep 07, 2021 at 05:11:09PM +0800, Shengjiu Wang wrote:
> There is a noise issue for 8kHz sample rate on slave mode.
> Compared with master mode, the difference is the DACDIV
> setting, after correcting the DACDIV, the noise is gone.
> 
> There is no noise issue for 48kHz sample rate, because
> the default value of DACDIV is correct for 48kHz.
> 
> So wm8960_configure_clocking() should be functional for
> ADC and DAC function even if it is slave mode.
> 
> In order to be compatible for old use case, just add
> condition for checking that sysclk is zero with
> slave mode.
> 
> Fixes: 0e50b51aa22f ("ASoC: wm8960: Let wm8960 driver configure its bit clock and frame clock")
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> ---
>  sound/soc/codecs/wm8960.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/soc/codecs/wm8960.c b/sound/soc/codecs/wm8960.c
> index 9e621a254392..9c6af76a60fd 100644
> --- a/sound/soc/codecs/wm8960.c
> +++ b/sound/soc/codecs/wm8960.c
> @@ -742,7 +742,7 @@ static int wm8960_configure_clocking(struct snd_soc_component *component)
>  	int i, j, k;
>  	int ret;
>  
> -	if (!(iface1 & (1<<6))) {
> +	if (!(iface1 & (1 << 6)) && !wm8960->sysclk) {
>  		dev_dbg(component->dev,
>  			"Codec is slave mode, no need to configure clock\n");

Looking through the datasheet it just looks like this if
statement has always been non-sense, it looks pretty clear the
clocking should still be configured in slave mode (apart from
BCLKDIV which is presumably ignored in slave mode).

I would be slightly inclined to suggest it would be better
to just fixup any systems not setting sysclk for slave mode,
but I am assuming you are talking primarily about out of tree
systems. So I think we need to at least update the message here
as well, it should probably change to a warning and state that
the we are proceeding with no clock configuration, rather than
erroneously saying it doesn't need one.

Thanks,
Charles