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
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 <[email protected]>
---
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 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
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 <[email protected]>
> ---
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 <[email protected]>
> 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 :)