| Message ID | 20210326221619.949961-3-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 945b0b58c5d7c6640f9aad2096e4675bc7f5371c |
| Headers | show |
| Series | ASoC: remove cppchecks warnings on lm49453 and da732x | expand |
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
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.
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 :)
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
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(-)