[alsa-devel] [PATCH 0/2] Relax bitclk computation when using PLL
Using strict bitclk requirements we cannot support all promised rates and formats. For this reason we relax bitclk computation by choosing the best available bitclk.
First patch in the series is based on Arnd's patch:
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-April/119899.html
Second one does the actual bitclk relaxation.
Daniel Baluta (2): ASoC: codec: wm9860: avoid maybe-uninitialized warning ASoC: codec: wm8960: Relax bit clock computation when using PLL
sound/soc/codecs/wm8960.c | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-)
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(-)
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 */
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:
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:
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:
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.
Bitclk is derived from sysclk using bclk_divs. Sysclk can be derived in two ways: (1) directly from MLCK (2) MCLK via PLL
Commit 3c01b9ee2ab9d0d ("ASoC: codec: wm8960: Relax bit clock computation") relaxed bitclk computation when sysclk is directly derived from MCLK.
Lets do the same thing when sysclk is derived via PLL.
Signed-off-by: Daniel Baluta daniel.baluta@nxp.com --- Here, I forced the following harmless initialization:
*sysclk_idx = *dac_idx = *bclk_idx = -1;
otherwise I would trigger a gcc false positive warning:
sound/soc/codecs/wm8960.c: In function 'wm8960_configure_clocking': sound/soc/codecs/wm8960.c:810:46: warning: 'j' may be used uninitialized in this function [-Wmaybe-uninitialized] snd_soc_update_bits(codec, WM8960_CLOCK1, 0x7 << 6, j << 6); ~~^~~~ sound/soc/codecs/wm8960.c:806:44: warning: 'i' may be used uninitialized in this function [-Wmaybe-uninitialized] snd_soc_update_bits(codec, WM8960_CLOCK1, 3 << 1, i << 1); ~~^~~~
sound/soc/codecs/wm8960.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/sound/soc/codecs/wm8960.c b/sound/soc/codecs/wm8960.c index 8c87153..60700d5 100644 --- a/sound/soc/codecs/wm8960.c +++ b/sound/soc/codecs/wm8960.c @@ -679,6 +679,10 @@ int wm8960_configure_sysclk(struct wm8960_priv *wm8960, int mclk, * - freq_out = sysclk * sysclk_divs * - 10 * sysclk = bclk * bclk_divs * + * If we cannot find an exact match for (sysclk, lrclk, bclk) + * triplet, we relax the bclk such that bclk is chosen as the + * closest available frequency greater than expected bclk. + * * @codec: codec structure * @freq_in: input frequency used to derive freq out via PLL * @sysclk_idx: sysclk_divs index for found sysclk @@ -696,13 +700,15 @@ int wm8960_configure_pll(struct snd_soc_codec *codec, int freq_in, { struct wm8960_priv *wm8960 = snd_soc_codec_get_drvdata(codec); int sysclk, bclk, lrclk, freq_out; - int diff, best_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) @@ -723,6 +729,13 @@ int wm8960_configure_pll(struct snd_soc_codec *codec, int freq_in, 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;
On Fri, Apr 21, 2017 at 3:07 PM, Daniel Baluta daniel.baluta@nxp.com wrote:
Bitclk is derived from sysclk using bclk_divs. Sysclk can be derived in two ways: (1) directly from MLCK (2) MCLK via PLL
Commit 3c01b9ee2ab9d0d ("ASoC: codec: wm8960: Relax bit clock computation") relaxed bitclk computation when sysclk is directly derived from MCLK.
Lets do the same thing when sysclk is derived via PLL.
Signed-off-by: Daniel Baluta daniel.baluta@nxp.com
Here, I forced the following harmless initialization:
*sysclk_idx = *dac_idx = *bclk_idx = -1;otherwise I would trigger a gcc false positive warning:
sound/soc/codecs/wm8960.c: In function 'wm8960_configure_clocking': sound/soc/codecs/wm8960.c:810:46: warning: 'j' may be used uninitialized in this function [-Wmaybe-uninitialized] snd_soc_update_bits(codec, WM8960_CLOCK1, 0x7 << 6, j << 6); ~~^~~~ sound/soc/codecs/wm8960.c:806:44: warning: 'i' may be used uninitialized in this function [-Wmaybe-uninitialized] snd_soc_update_bits(codec, WM8960_CLOCK1, 3 << 1, i << 1); ~~^~~~
I saw the same warning earlier, but it was gone after the rework I posted the other day. Please try if that works for you as well, I think that would be better than a bogus initialization.
Arnd
participants (3)
-
Arnd Bergmann -
Daniel Baluta -
Daniel Baluta