linux-kernel - Re: [RFC PATCH 2/2] ASoC: codec: wm8960: Relax bit clock computation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170315171703.GK6986@localhost.localdomain>
Date:   Wed, 15 Mar 2017 17:17:03 +0000
From:   Charles Keepax <ckeepax@...nsource.wolfsonmicro.com>
To:     Daniel Baluta <daniel.baluta@....com>
CC:     <lgirdwood@...il.com>, <broonie@...nel.org>, <tiwai@...e.com>,
        <patches@...nsource.wolfsonmicro.com>,
        <alsa-devel@...a-project.org>, <linux-kernel@...r.kernel.org>,
        <shengjiu.wang@...escale.com>, <mihai.serban@....com>,
        <viorel.suman@....com>
Subject: Re: [RFC PATCH 2/2] ASoC: codec: wm8960: Relax bit clock computation

On Wed, Mar 15, 2017 at 05:33:06PM +0200, Daniel Baluta wrote:
> WM8960 derives bit clock from sysclock using BCLKDIV[3:0] of R8
> clocking register (See WM8960 datasheet, page 71).
> 
> There are use cases, like this:
> aplay -Dhw:0,0 -r 48000 -c 1 -f S20_3LE -t raw audio48k20b_3LE1c.pcm
> 
> where no BCLKDIV applied to sysclock can give us the exact requested
> bitclk, so driver fails to configure clocking and aplay fails to run.
> 
> Fix this by relaxing bitclk computation, so that when no exact value
> can be derived from sysclk pick the closest value greater than
> expected bitclk.
> 
> Suggested-by: Charles Keepax <ckeepax@...nsource.wolfsonmicro.com>
> Signed-off-by: Daniel Baluta <daniel.baluta@....com>
> ---
>  sound/soc/codecs/wm8960.c | 39 +++++++++++++++++++++++++++++++++------
>  1 file changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/sound/soc/codecs/wm8960.c b/sound/soc/codecs/wm8960.c
> index cb2ff2d..1669b45 100644
> --- a/sound/soc/codecs/wm8960.c
> +++ b/sound/soc/codecs/wm8960.c
> @@ -611,6 +611,10 @@ static const int bclk_divs[] = {
>   *		- lrclk      = sysclk / dac_divs
>   *		- 10 * bclk  = sysclk / bclk_divs
>   *
> + *	If we cannot find an exact match for (sysclk, lrclk, bclk)
> + *	triplet, we relax the bclk such that bclk is chosen as the
> + *	closest available frequency greater than expected bclk.
> + *
>   * @wm8960_priv: wm8960 codec private data
>   * @mclk: MCLK used to derive sysclk
>   * @_i: sysclk_divs index for found sysclk
> @@ -620,14 +624,14 @@ static const int bclk_divs[] = {
>   * Returns:
>   * -1, in case no sysclk frequency available found
>   *  0, in case an exact match is found. See @_i, @_j, @_k
> - *
> + * >0, in case a relaxed match is found. See @_i, @_j, @_k
>   */
>  int wm8960_configure_sysclk(struct wm8960_priv *wm8960, int mclk,
>  		      int *_i, int *_j, int *_k)
>  {
>  	int sysclk, bclk, lrclk;
>  	int i, j, k;
> -	int diff;
> +	int diff, closest = mclk;

Don't you need to initialise diff here too?

>  
>  	bclk = wm8960->bclk;
>  	lrclk = wm8960->lrclk;
> @@ -648,6 +652,12 @@ int wm8960_configure_sysclk(struct wm8960_priv *wm8960, int mclk,
>  					*_k = k;
>  					break;
>  				}
> +				if (diff > 0 && closest > diff) {
> +					*_i = i;
> +					*_j = j;
> +					*_k = k;
> +					closest = diff;
> +				}
>  			}
>  			if (k != ARRAY_SIZE(bclk_divs))
>  				break;
> @@ -656,10 +666,16 @@ int wm8960_configure_sysclk(struct wm8960_priv *wm8960, int mclk,
>  			break;
>  	}
>  
> +	/* exact match */
>  	if (i != ARRAY_SIZE(sysclk_divs))
>  		return 0;
>  
> -	return -1;
> +	/* no match */
> +	if (closest == mclk)
> +		return -1;
> +
> +	/* relaxed match */
> +	return 1;

Do we need to differenciate between relaxed and an actual match?

>  }
>  
>  static int wm8960_configure_clocking(struct snd_soc_codec *codec)
> @@ -668,6 +684,7 @@ static int wm8960_configure_clocking(struct snd_soc_codec *codec)
>  	int sysclk, bclk, lrclk, freq_out, freq_in;
>  	u16 iface1 = snd_soc_read(codec, WM8960_IFACE1);
>  	int i, j, k;
> +	int best_sysclk_div, best_dac_div, best_bclk_div = -1;
>  	int ret;
>  
>  	if (!(iface1 & (1<<6))) {
> @@ -705,10 +722,14 @@ static int wm8960_configure_clocking(struct snd_soc_codec *codec)
>  		ret = wm8960_configure_sysclk(wm8960, freq_out, &i, &j, &k);
>  		if (ret == 0)
>  			goto configure_clock;
> -		else if (wm8960->clk_id != WM8960_SYSCLK_AUTO) {
> +		else if (ret < 0 && wm8960->clk_id != WM8960_SYSCLK_AUTO) {
>  			dev_err(codec->dev, "failed to configure clock\n");
>  			return -EINVAL;
>  		}
> +		/* there is still hope, keep this if no PLL out available */
> +		best_sysclk_div	= i;
> +		best_dac_div	= j;
> +		best_bclk_div	= k;

Enabling the PLL just to avoid running the BCLK a little fast
doesn't really seem worth it and it would make the code much more
obvious.

>  	}
>  	/* get a available pll out frequency and set pll */
>  	for (i = 0; i < ARRAY_SIZE(sysclk_divs); ++i) {
> @@ -736,8 +757,14 @@ static int wm8960_configure_clocking(struct snd_soc_codec *codec)
>  	}
>  
>  	if (i == ARRAY_SIZE(sysclk_divs)) {
> -		dev_err(codec->dev, "failed to configure clock\n");
> -		return -EINVAL;
> +		if (best_bclk_div != -1) {
> +			i = best_sysclk_div;
> +			j = best_dac_div;
> +			k = best_bclk_div;
> +		} else {
> +			dev_err(codec->dev, "failed to configure clock\n");
> +			return -EINVAL;
> +		}
>  	}
>  
>  configure_clock:
> -- 
> 2.7.4

Thanks,
Charles