[RFC PATCH 0/2] ASoC: remove cppchecks warnings on lm49453 and da732x

[RFC PATCH 0/2] ASoC: remove cppchecks warnings on lm49453 and da732x

[Pierre-Louis Bossart]

There are the last two patches in the cleanups, this time I am not
sure what the code does and what the proper fix might be. Feedback
welcome.

Pierre-Louis Bossart (2):
  ASoC: lm49453: fix useless assignment before return
  ASoC: da732x: simplify code

 sound/soc/codecs/da732x.c  | 17 ++++++-----------
 sound/soc/codecs/da732x.h  | 12 ++++--------
 sound/soc/codecs/lm49453.c |  2 --
 3 files changed, 10 insertions(+), 21 deletions(-)

-- 
2.25.1

[RFC PATCH 1/2] ASoC: lm49453: fix useless assignment before return

[Pierre-Louis Bossart]

Cppcheck warning:

sound/soc/codecs/lm49453.c:1210:11: style: Variable 'pll_clk' is
assigned a value that is never used. [unreadVariable]

  pll_clk = BIT(4);
          ^

FIXME: What is the correct fix?
	/* fll clk slection */
	pll_clk = BIT(4);
	return 0;

is the assignment redundant or the 'return 0' a mistake?

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/codecs/lm49453.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/sound/soc/codecs/lm49453.c b/sound/soc/codecs/lm49453.c
index eb3dd0bd80d9..fb0fb23537e7 100644
--- a/sound/soc/codecs/lm49453.c
+++ b/sound/soc/codecs/lm49453.c
@@ -1206,8 +1206,6 @@ static int lm49453_set_dai_sysclk(struct snd_soc_dai *dai, int clk_id,
 		break;
 	case 48000:
 	case 32576:
-		/* fll clk slection */
-		pll_clk = BIT(4);
 		return 0;
 	default:
 		return -EINVAL;
-- 
2.25.1

[RFC PATCH 2/2] ASoC: da732x: simplify code

[Pierre-Louis Bossart]

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@linux.intel.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

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

[Adam Thomson]

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@linux.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@diasemi.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

Re: [RFC PATCH 2/2] ASoC: da732x: simplify code

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 537 bytes --]

On Thu, Apr 15, 2021 at 04:00:48PM +0000, Adam Thomson wrote:
> On 26 March 2021 22:16, Pierre-Louis Bossart wrote:

> 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?

Unless it's actually getting in the way it's generally easier to just
leave the driver than try to figure out if anyone is updating a system
that uses it.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Adam Thomson]

On 15 April 2021 17:04, Mark Brown wrote:

> On Thu, Apr 15, 2021 at 04:00:48PM +0000, Adam Thomson wrote:
> > On 26 March 2021 22:16, Pierre-Louis Bossart wrote:
> 
> > 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?
> 
> Unless it's actually getting in the way it's generally easier to just
> leave the driver than try to figure out if anyone is updating a system
> that uses it.

Fair enough. Just don't want to waste people's time with unnecessary updates :)

Re: [RFC PATCH 0/2] ASoC: remove cppchecks warnings on lm49453 and da732x

[Mark Brown]

On Fri, 26 Mar 2021 17:16:17 -0500, Pierre-Louis Bossart wrote:
> There are the last two patches in the cleanups, this time I am not
> sure what the code does and what the proper fix might be. Feedback
> welcome.
> 
> Pierre-Louis Bossart (2):
>   ASoC: lm49453: fix useless assignment before return
>   ASoC: da732x: simplify code
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/2] ASoC: lm49453: fix useless assignment before return
      commit: 458c23c509f66c5950da7e5496ea952ad15128f7
[2/2] ASoC: da732x: simplify code
      commit: 945b0b58c5d7c6640f9aad2096e4675bc7f5371c

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark