| Message ID | 1492780055-4892-2-git-send-email-daniel.baluta@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 21, 2017 at 3:07 PM, Daniel Baluta <daniel.baluta@nxp.com> wrote: > The new PLL configuration code triggers a harmless warning: > > sound/soc/codecs/wm8960.c: In function 'wm8960_configure_clocking': > sound/soc/codecs/wm8960.c:735:3: error: 'best_freq_out' may be used > uninitialized in this function [-Werror=maybe-uninitialized] > wm8960_set_pll(codec, freq_in, best_freq_out); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > sound/soc/codecs/wm8960.c:699:12: note: 'best_freq_out' was declared > here > > Fixes: 84fdc00d519f ("ASoC: codec: wm9860: Refactor PLL out freq search") > Fixes: 303e8954af8d ("ASoC: codec: wm8960: Stop when a matching PLL freq is found") > Suggested-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com> > --- > Arnd, > > I agree that your code was more both humans and gcc anyhow > for consistency with wm8960_configure_sysclk function I preferred > to keep the "if(..) break" statements. How about changing both functions the same way then? Arnd
On Fri, Apr 21, 2017 at 5:46 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Fri, Apr 21, 2017 at 3:07 PM, Daniel Baluta <daniel.baluta@nxp.com> wrote: >> The new PLL configuration code triggers a harmless warning: >> >> sound/soc/codecs/wm8960.c: In function 'wm8960_configure_clocking': >> sound/soc/codecs/wm8960.c:735:3: error: 'best_freq_out' may be used >> uninitialized in this function [-Werror=maybe-uninitialized] >> wm8960_set_pll(codec, freq_in, best_freq_out); >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> sound/soc/codecs/wm8960.c:699:12: note: 'best_freq_out' was declared >> here >> >> Fixes: 84fdc00d519f ("ASoC: codec: wm9860: Refactor PLL out freq search") >> Fixes: 303e8954af8d ("ASoC: codec: wm8960: Stop when a matching PLL freq is found") >> Suggested-by: Arnd Bergmann <arnd@arndb.de> >> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com> >> --- >> Arnd, >> >> I agree that your code was more both humans and gcc anyhow >> for consistency with wm8960_configure_sysclk function I preferred >> to keep the "if(..) break" statements. > > How about changing both functions the same way then? I've tried but I couldn't find any solution. For clarity here is how the code actually looks like. The git diff is a little bit misleading. Here is how wm8960_configure_pll code looks like: https://pastebin.com/naGdVNQz static int wm8960_configure_pll(struct snd_soc_codec *codec, int freq_in, » » » int *sysclk_idx, int *dac_idx, int *bclk_idx) { » struct wm8960_priv *wm8960 = snd_soc_codec_get_drvdata(codec); » int sysclk, bclk, lrclk, freq_out; » int diff, closest, best_freq_out; » int i, j, k; » bclk = wm8960->bclk; » lrclk = wm8960->lrclk; » closest = freq_in; » best_freq_out = -EINVAL; » *sysclk_idx = *dac_idx = *bclk_idx = -1; » for (i = 0; i < ARRAY_SIZE(sysclk_divs); ++i) { » » if (sysclk_divs[i] == -1) » » » continue; » » for (j = 0; j < ARRAY_SIZE(dac_divs); ++j) { » » » sysclk = lrclk * dac_divs[j]; » » » freq_out = sysclk * sysclk_divs[i]; » » » for (k = 0; k < ARRAY_SIZE(bclk_divs); ++k) { » » » » if (!is_pll_freq_available(freq_in, freq_out)) » » » » » continue; » » » » diff = sysclk - bclk * bclk_divs[k] / 10; » » » » if (diff == 0) { » » » » » *sysclk_idx = i; » » » » » *dac_idx = j; » » » » » *bclk_idx = k; » » » » » best_freq_out = freq_out; » » » » » break; » » » » } » » » » if (diff > 0 && closest > diff) { » » » » » *sysclk_idx = i; » » » » » *dac_idx = j; » » » » » *bclk_idx = k; » » » » » closest = diff; » » » » » best_freq_out = freq_out; » » » » } » » » } » » » if (k != ARRAY_SIZE(bclk_divs)) » » » » break; » » } » » if (j != ARRAY_SIZE(dac_divs)) » » » break; » } » return best_freq_out; } In my opinion this is a compiler false positive. Any clue on how to rework this would be welcomed :). I couldn't find any decent solution. Daniel.
On Mon, Apr 24, 2017 at 3:15 PM, Daniel Baluta <daniel.baluta@gmail.com> wrote: > On Fri, Apr 21, 2017 at 5:46 PM, Arnd Bergmann <arnd@arndb.de> wrote: >> On Fri, Apr 21, 2017 at 3:07 PM, Daniel Baluta <daniel.baluta@nxp.com> wrote: >>> The new PLL configuration code triggers a harmless warning: >>> >>> sound/soc/codecs/wm8960.c: In function 'wm8960_configure_clocking': >>> sound/soc/codecs/wm8960.c:735:3: error: 'best_freq_out' may be used >>> uninitialized in this function [-Werror=maybe-uninitialized] >>> wm8960_set_pll(codec, freq_in, best_freq_out); >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> sound/soc/codecs/wm8960.c:699:12: note: 'best_freq_out' was declared >>> here >>> >>> Fixes: 84fdc00d519f ("ASoC: codec: wm9860: Refactor PLL out freq search") >>> Fixes: 303e8954af8d ("ASoC: codec: wm8960: Stop when a matching PLL freq is found") >>> Suggested-by: Arnd Bergmann <arnd@arndb.de> >>> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com> >>> --- >>> Arnd, >>> >>> I agree that your code was more both humans and gcc anyhow >>> for consistency with wm8960_configure_sysclk function I preferred >>> to keep the "if(..) break" statements. >> >> How about changing both functions the same way then? > > I've tried but I couldn't find any solution. For clarity here is how > the code actually looks like. > > The git diff is a little bit misleading. Here is how wm8960_configure_pll code > looks like: > > https://pastebin.com/naGdVNQz > > static > int wm8960_configure_pll(struct snd_soc_codec *codec, int freq_in, > » » » int *sysclk_idx, int *dac_idx, int *bclk_idx) > { > » struct wm8960_priv *wm8960 = snd_soc_codec_get_drvdata(codec); > » int sysclk, bclk, lrclk, freq_out; > » int diff, closest, best_freq_out; > » int i, j, k; > > » bclk = wm8960->bclk; > » lrclk = wm8960->lrclk; > » closest = freq_in; > > » best_freq_out = -EINVAL; > » *sysclk_idx = *dac_idx = *bclk_idx = -1; > > » for (i = 0; i < ARRAY_SIZE(sysclk_divs); ++i) { > » » if (sysclk_divs[i] == -1) > » » » continue; > » » for (j = 0; j < ARRAY_SIZE(dac_divs); ++j) { > » » » sysclk = lrclk * dac_divs[j]; > » » » freq_out = sysclk * sysclk_divs[i]; > > » » » for (k = 0; k < ARRAY_SIZE(bclk_divs); ++k) { > » » » » if (!is_pll_freq_available(freq_in, freq_out)) > » » » » » continue; > > » » » » diff = sysclk - bclk * bclk_divs[k] / 10; > » » » » if (diff == 0) { > » » » » » *sysclk_idx = i; > » » » » » *dac_idx = j; > » » » » » *bclk_idx = k; > » » » » » best_freq_out = freq_out; > » » » » » break; > » » » » } > » » » » if (diff > 0 && closest > diff) { > » » » » » *sysclk_idx = i; > » » » » » *dac_idx = j; > » » » » » *bclk_idx = k; > » » » » » closest = diff; > » » » » » best_freq_out = freq_out; > » » » » } > » » » } > » » » if (k != ARRAY_SIZE(bclk_divs)) > » » » » break; > » » } > » » if (j != ARRAY_SIZE(dac_divs)) > » » » break; > » } > > » return best_freq_out; > } > > In my opinion this is a compiler false positive. Any clue on how to rework this > would be welcomed :). I couldn't find any decent solution. Actually I think in this case the compiler is supposed to warn if best_freq_out is not initialized, as we would never set it in case is_pll_freq_available() returns false for all inputs or sysclk_divs[] is -1 for all fields. I'd leave the initialization then, and only replace the breaks with a goto (not tested): > » for (i = 0; i < ARRAY_SIZE(sysclk_divs); ++i) { > » » if (sysclk_divs[i] == -1) > » » » continue; > » » for (j = 0; j < ARRAY_SIZE(dac_divs); ++j) { > » » » sysclk = lrclk * dac_divs[j]; > » » » freq_out = sysclk * sysclk_divs[i]; > > » » » for (k = 0; k < ARRAY_SIZE(bclk_divs); ++k) { > » » » » if (!is_pll_freq_available(freq_in, freq_out)) > » » » » » continue; > > » » » » diff = sysclk - bclk * bclk_divs[k] / 10; > » » » » if (diff == 0) { > » » » » » *sysclk_idx = i; > » » » » » *dac_idx = j; > » » » » » *bclk_idx = k; > » » » » » best_freq_out = freq_out; > » » » » » goto out; > » » » » } > » » » » if (diff > 0 && closest > diff) { > » » » » » *sysclk_idx = i; > » » » » » *dac_idx = j; > » » » » » *bclk_idx = k; > » » » » » closest = diff; > » » » » » best_freq_out = freq_out; > » » » » } > » » » } > » » } > » } >out: > » return best_freq_out; > } Arnd
On Mon, Apr 24, 2017 at 6:27 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Mon, Apr 24, 2017 at 3:15 PM, Daniel Baluta <daniel.baluta@gmail.com> wrote: >> On Fri, Apr 21, 2017 at 5:46 PM, Arnd Bergmann <arnd@arndb.de> wrote: >>> On Fri, Apr 21, 2017 at 3:07 PM, Daniel Baluta <daniel.baluta@nxp.com> wrote: >>>> The new PLL configuration code triggers a harmless warning: >>>> >>>> sound/soc/codecs/wm8960.c: In function 'wm8960_configure_clocking': >>>> sound/soc/codecs/wm8960.c:735:3: error: 'best_freq_out' may be used >>>> uninitialized in this function [-Werror=maybe-uninitialized] >>>> wm8960_set_pll(codec, freq_in, best_freq_out); >>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>> sound/soc/codecs/wm8960.c:699:12: note: 'best_freq_out' was declared >>>> here >>>> >>>> Fixes: 84fdc00d519f ("ASoC: codec: wm9860: Refactor PLL out freq search") >>>> Fixes: 303e8954af8d ("ASoC: codec: wm8960: Stop when a matching PLL freq is found") >>>> Suggested-by: Arnd Bergmann <arnd@arndb.de> >>>> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com> >>>> --- >>>> Arnd, >>>> >>>> I agree that your code was more both humans and gcc anyhow >>>> for consistency with wm8960_configure_sysclk function I preferred >>>> to keep the "if(..) break" statements. >>> >>> How about changing both functions the same way then? >> >> I've tried but I couldn't find any solution. For clarity here is how >> the code actually looks like. >> >> The git diff is a little bit misleading. Here is how wm8960_configure_pll code >> looks like: >> >> https://pastebin.com/naGdVNQz >> >> static >> int wm8960_configure_pll(struct snd_soc_codec *codec, int freq_in, >> » » » int *sysclk_idx, int *dac_idx, int *bclk_idx) >> { >> » struct wm8960_priv *wm8960 = snd_soc_codec_get_drvdata(codec); >> » int sysclk, bclk, lrclk, freq_out; >> » int diff, closest, best_freq_out; >> » int i, j, k; >> >> » bclk = wm8960->bclk; >> » lrclk = wm8960->lrclk; >> » closest = freq_in; >> >> » best_freq_out = -EINVAL; >> » *sysclk_idx = *dac_idx = *bclk_idx = -1; >> >> » for (i = 0; i < ARRAY_SIZE(sysclk_divs); ++i) { >> » » if (sysclk_divs[i] == -1) >> » » » continue; >> » » for (j = 0; j < ARRAY_SIZE(dac_divs); ++j) { >> » » » sysclk = lrclk * dac_divs[j]; >> » » » freq_out = sysclk * sysclk_divs[i]; >> >> » » » for (k = 0; k < ARRAY_SIZE(bclk_divs); ++k) { >> » » » » if (!is_pll_freq_available(freq_in, freq_out)) >> » » » » » continue; >> >> » » » » diff = sysclk - bclk * bclk_divs[k] / 10; >> » » » » if (diff == 0) { >> » » » » » *sysclk_idx = i; >> » » » » » *dac_idx = j; >> » » » » » *bclk_idx = k; >> » » » » » best_freq_out = freq_out; >> » » » » » break; >> » » » » } >> » » » » if (diff > 0 && closest > diff) { >> » » » » » *sysclk_idx = i; >> » » » » » *dac_idx = j; >> » » » » » *bclk_idx = k; >> » » » » » closest = diff; >> » » » » » best_freq_out = freq_out; >> » » » » } >> » » » } >> » » » if (k != ARRAY_SIZE(bclk_divs)) >> » » » » break; >> » » } >> » » if (j != ARRAY_SIZE(dac_divs)) >> » » » break; >> » } >> >> » return best_freq_out; >> } >> >> In my opinion this is a compiler false positive. Any clue on how to rework this >> would be welcomed :). I couldn't find any decent solution. > > Actually I think in this case the compiler is supposed to warn if > best_freq_out is not initialized, as we would never set it > in case is_pll_freq_available() returns false for all inputs or > sysclk_divs[] is -1 for all fields. > I'd leave the initialization then, and only replace the breaks > with a goto (not tested): > >> » for (i = 0; i < ARRAY_SIZE(sysclk_divs); ++i) { >> » » if (sysclk_divs[i] == -1) >> » » » continue; >> » » for (j = 0; j < ARRAY_SIZE(dac_divs); ++j) { >> » » » sysclk = lrclk * dac_divs[j]; >> » » » freq_out = sysclk * sysclk_divs[i]; >> >> » » » for (k = 0; k < ARRAY_SIZE(bclk_divs); ++k) { >> » » » » if (!is_pll_freq_available(freq_in, freq_out)) >> » » » » » continue; >> >> » » » » diff = sysclk - bclk * bclk_divs[k] / 10; >> » » » » if (diff == 0) { >> » » » » » *sysclk_idx = i; >> » » » » » *dac_idx = j; >> » » » » » *bclk_idx = k; >> » » » » » best_freq_out = freq_out; >> » » » » » goto out; >> » » » » } >> » » » » if (diff > 0 && closest > diff) { >> » » » » » *sysclk_idx = i; >> » » » » » *dac_idx = j; >> » » » » » *bclk_idx = k; >> » » » » » closest = diff; >> » » » » » best_freq_out = freq_out; >> » » » » } >> » » » } >> » » } >> » } >>out: >> » return best_freq_out; >> } Sure, this looks reasonable. I will send v2. Daniel.
diff --git a/sound/soc/codecs/wm8960.c b/sound/soc/codecs/wm8960.c index ace69da..8c87153 100644 --- a/sound/soc/codecs/wm8960.c +++ b/sound/soc/codecs/wm8960.c @@ -702,7 +702,7 @@ int wm8960_configure_pll(struct snd_soc_codec *codec, int freq_in, bclk = wm8960->bclk; lrclk = wm8960->lrclk; - *bclk_idx = -1; + best_freq_out = -EINVAL; for (i = 0; i < ARRAY_SIZE(sysclk_divs); ++i) { if (sysclk_divs[i] == -1) @@ -731,10 +731,7 @@ int wm8960_configure_pll(struct snd_soc_codec *codec, int freq_in, break; } - if (*bclk_idx != -1) - wm8960_set_pll(codec, freq_in, best_freq_out); - - return *bclk_idx; + return best_freq_out; } static int wm8960_configure_clocking(struct snd_soc_codec *codec) { @@ -783,11 +780,12 @@ static int wm8960_configure_clocking(struct snd_soc_codec *codec) } } - ret = wm8960_configure_pll(codec, freq_in, &i, &j, &k); - if (ret < 0) { + freq_out = wm8960_configure_pll(codec, freq_in, &i, &j, &k); + if (freq_out < 0) { dev_err(codec->dev, "failed to configure clock via PLL\n"); - return -EINVAL; + return freq_out; } + wm8960_set_pll(codec, freq_in, freq_out); configure_clock: /* configure sysclk clock */
The new PLL configuration code triggers a harmless warning: sound/soc/codecs/wm8960.c: In function 'wm8960_configure_clocking': sound/soc/codecs/wm8960.c:735:3: error: 'best_freq_out' may be used uninitialized in this function [-Werror=maybe-uninitialized] wm8960_set_pll(codec, freq_in, best_freq_out); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ sound/soc/codecs/wm8960.c:699:12: note: 'best_freq_out' was declared here Fixes: 84fdc00d519f ("ASoC: codec: wm9860: Refactor PLL out freq search") Fixes: 303e8954af8d ("ASoC: codec: wm8960: Stop when a matching PLL freq is found") Suggested-by: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com> --- Arnd, I agree that your code was more both humans and gcc anyhow for consistency with wm8960_configure_sysclk function I preferred to keep the "if(..) break" statements. sound/soc/codecs/wm8960.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)