RE: [RFC PATCH 2/2] ASoC: da732x: simplify code
On 15 April 2021 17:04, Mark Brown wrote: > On Thu, Apr 15, 2021 at 04:00:48PM +, 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 2/2] ASoC: da732x: simplify code
On Thu, Apr 15, 2021 at 04:00:48PM +, 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. signature.asc Description: PGP signature
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
> ---
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
> 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 2000
> #define DA732X_MCLK_40MHZ 4000
> #define DA732X_MCLK_54MHZ 5400
> -#define DA732X_MCLK_RET_0_10MHZ 0
> -#define DA732X_MCLK_VAL_0_10MHZ 1
> -#define DA732X_MCLK_RET_10_20MHZ1
> -#define DA732X_MCLK_VAL_10_20MHZ2
> -#define DA732X_MCLK_RET_20_40MHZ2
> -#define DA732X_MCLK_VAL_20_40MHZ4
> -#define DA732X_MCLK_RET_40_54MHZ3
> -#define DA732X_MCLK_VAL_40_54MHZ8
> +#define DA732X_MCLK_VAL_0_10MHZ 0
> +#define DA732X_MCLK_VAL_10_20MHZ1
> +#define DA732X_MCLK_VAL_20_40MHZ2
> +#define DA732X_MCLK_VAL_40_54MHZ3
> #define DA732X_DAI_ID1 0
> #define DA732X_DAI_ID2 1
> #define DA732X_SRCCLK_PLL 0
> --
> 2.25.1
[RFC PATCH 2/2] ASoC: da732x: simplify code
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
---
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 @@
#defineDA732X_MCLK_20MHZ 2000
#defineDA732X_MCLK_40MHZ 4000
#defineDA732X_MCLK_54MHZ 5400
-#defineDA732X_MCLK_RET_0_10MHZ 0
-#defineDA732X_MCLK_VAL_0_10MHZ 1
-#defineDA732X_MCLK_RET_10_20MHZ1
-#defineDA732X_MCLK_VAL_10_20MHZ2
-#defineDA732X_MCLK_RET_20_40MHZ2
-#defineDA732X_MCLK_VAL_20_40MHZ4
-#defineDA732X_MCLK_RET_40_54MHZ3
-#defineDA732X_MCLK_VAL_40_54MHZ8
+#defineDA732X_MCLK_VAL_0_10MHZ 0
+#defineDA732X_MCLK_VAL_10_20MHZ1
+#defineDA732X_MCLK_VAL_20_40MHZ2
+#defineDA732X_MCLK_VAL_40_54MHZ3
#defineDA732X_DAI_ID1 0
#defineDA732X_DAI_ID2 1
#defineDA732X_SRCCLK_PLL 0
--
2.25.1
