linux-kernel - RE: [RFC PATCH 2/2] ASoC: da732x: simplify code

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]

Date:   Thu, 15 Apr 2021 16:00:48 +0000
From:   Adam Thomson <Adam.Thomson.Opensource@...semi.com>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>
CC:     "tiwai@...e.de" <tiwai@...e.de>,
        "broonie@...nel.org" <broonie@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Support Opensource <Support.Opensource@...semi.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>
Subject: RE: [RFC PATCH 2/2] ASoC: da732x: simplify code

On 26 March 2021 22:16, Pierre-Louis Bossart wrote:

> cppcheck reports a false positive:
> 
> sound/soc/codecs/da732x.c:1161:25: warning: Either the condition
> 'indiv<0' is redundant or there is division by zero at line
> 1161. [zerodivcond]
>  fref = (da732x->sysclk / indiv);
>                         ^
> sound/soc/codecs/da732x.c:1158:12: note: Assuming that condition
> 'indiv<0' is not redundant
>  if (indiv < 0)
>            ^
> sound/soc/codecs/da732x.c:1161:25: note: Division by zero
>  fref = (da732x->sysclk / indiv);
>                         ^
> 
> The code is awfully convoluted/confusing and can be simplified with a
> single variable and the BIT macro.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
> ---

Apologies for the delay in getting to this. The change looks fine to me,
although this part was EOL some time back, and I find it hard to believe anyone
out there has a board with this on. Wondering if it would make sense to remove
the driver permanently?

For the change at hand though:

Reviewed-by: Adam Thomson <Adam.Thomson.Opensource@...semi.com>

>  sound/soc/codecs/da732x.c | 17 ++++++-----------
>  sound/soc/codecs/da732x.h | 12 ++++--------
>  2 files changed, 10 insertions(+), 19 deletions(-)
> 
> diff --git a/sound/soc/codecs/da732x.c b/sound/soc/codecs/da732x.c
> index d43ee7159ae0..42d6a3fc3af5 100644
> --- a/sound/soc/codecs/da732x.c
> +++ b/sound/soc/codecs/da732x.c
> @@ -168,30 +168,25 @@ static const struct reg_default da732x_reg_cache[] = {
>  static inline int da732x_get_input_div(struct snd_soc_component *component,
> int sysclk)
>  {
>  	int val;
> -	int ret;
> 
>  	if (sysclk < DA732X_MCLK_10MHZ) {
> -		val = DA732X_MCLK_RET_0_10MHZ;
> -		ret = DA732X_MCLK_VAL_0_10MHZ;
> +		val = DA732X_MCLK_VAL_0_10MHZ;
>  	} else if ((sysclk >= DA732X_MCLK_10MHZ) &&
>  	    (sysclk < DA732X_MCLK_20MHZ)) {
> -		val = DA732X_MCLK_RET_10_20MHZ;
> -		ret = DA732X_MCLK_VAL_10_20MHZ;
> +		val = DA732X_MCLK_VAL_10_20MHZ;
>  	} else if ((sysclk >= DA732X_MCLK_20MHZ) &&
>  	    (sysclk < DA732X_MCLK_40MHZ)) {
> -		val = DA732X_MCLK_RET_20_40MHZ;
> -		ret = DA732X_MCLK_VAL_20_40MHZ;
> +		val = DA732X_MCLK_VAL_20_40MHZ;
>  	} else if ((sysclk >= DA732X_MCLK_40MHZ) &&
>  	    (sysclk <= DA732X_MCLK_54MHZ)) {
> -		val = DA732X_MCLK_RET_40_54MHZ;
> -		ret = DA732X_MCLK_VAL_40_54MHZ;
> +		val = DA732X_MCLK_VAL_40_54MHZ;
>  	} else {
>  		return -EINVAL;
>  	}
> 
>  	snd_soc_component_write(component, DA732X_REG_PLL_CTRL, val);
> 
> -	return ret;
> +	return val;
>  }
> 
>  static void da732x_set_charge_pump(struct snd_soc_component *component,
> int state)
> @@ -1158,7 +1153,7 @@ static int da732x_set_dai_pll(struct
> snd_soc_component *component, int pll_id,
>  	if (indiv < 0)
>  		return indiv;
> 
> -	fref = (da732x->sysclk / indiv);
> +	fref = da732x->sysclk / BIT(indiv);
>  	div_hi = freq_out / fref;
>  	frac_div = (u64)(freq_out % fref) * 8192ULL;
>  	do_div(frac_div, fref);
> diff --git a/sound/soc/codecs/da732x.h b/sound/soc/codecs/da732x.h
> index c5af17ee1516..c2f784c3f359 100644
> --- a/sound/soc/codecs/da732x.h
> +++ b/sound/soc/codecs/da732x.h
> @@ -48,14 +48,10 @@
>  #define	DA732X_MCLK_20MHZ		20000000
>  #define	DA732X_MCLK_40MHZ		40000000
>  #define	DA732X_MCLK_54MHZ		54000000
> -#define	DA732X_MCLK_RET_0_10MHZ		0
> -#define	DA732X_MCLK_VAL_0_10MHZ		1
> -#define	DA732X_MCLK_RET_10_20MHZ	1
> -#define	DA732X_MCLK_VAL_10_20MHZ	2
> -#define	DA732X_MCLK_RET_20_40MHZ	2
> -#define	DA732X_MCLK_VAL_20_40MHZ	4
> -#define	DA732X_MCLK_RET_40_54MHZ	3
> -#define	DA732X_MCLK_VAL_40_54MHZ	8
> +#define	DA732X_MCLK_VAL_0_10MHZ		0
> +#define	DA732X_MCLK_VAL_10_20MHZ	1
> +#define	DA732X_MCLK_VAL_20_40MHZ	2
> +#define	DA732X_MCLK_VAL_40_54MHZ	3
>  #define	DA732X_DAI_ID1			0
>  #define	DA732X_DAI_ID2			1
>  #define	DA732X_SRCCLK_PLL		0
> --
> 2.25.1