[RFC PATCH 0/2] ASoC: remove cppchecks warnings on lm49453 and da732x
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(-)
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;
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
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;
val = DA732X_MCLK_RET_10_20MHZ;ret = DA732X_MCLK_VAL_10_20MHZ;
val = DA732X_MCLK_VAL_10_20MHZ;
val = DA732X_MCLK_RET_20_40MHZ;ret = DA732X_MCLK_VAL_20_40MHZ;
val = DA732X_MCLK_VAL_20_40MHZ;
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 :)
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
participants (3)
-
Adam Thomson -
Mark Brown -
Pierre-Louis Bossart