* [PATCH 0/2] Relax bitclk computation when using PLL
@ 2017-04-21 13:07 Daniel Baluta
2017-04-21 13:07 ` [PATCH 1/2] ASoC: codec: wm9860: avoid maybe-uninitialized warning Daniel Baluta
2017-04-21 13:07 ` [PATCH 2/2] ASoC: codec: wm8960: Relax bit clock computation when using PLL Daniel Baluta
0 siblings, 2 replies; 8+ messages in thread
From: Daniel Baluta @ 2017-04-21 13:07 UTC (permalink / raw)
To: broonie, tiwai, ckeepax, arnd, lgirdwood
Cc: patches, alsa-devel, linux-kernel, Daniel Baluta
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(-)
--
2.7.4
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] ASoC: codec: wm9860: avoid maybe-uninitialized warning
2017-04-21 13:07 [PATCH 0/2] Relax bitclk computation when using PLL Daniel Baluta
@ 2017-04-21 13:07 ` Daniel Baluta
2017-04-21 14:46 ` Arnd Bergmann
2017-04-21 13:07 ` [PATCH 2/2] ASoC: codec: wm8960: Relax bit clock computation when using PLL Daniel Baluta
1 sibling, 1 reply; 8+ messages in thread
From: Daniel Baluta @ 2017-04-21 13:07 UTC (permalink / raw)
To: broonie, tiwai, ckeepax, arnd, lgirdwood
Cc: patches, alsa-devel, linux-kernel, Daniel Baluta
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 */
--
2.7.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] ASoC: codec: wm8960: Relax bit clock computation when using PLL
2017-04-21 13:07 [PATCH 0/2] Relax bitclk computation when using PLL Daniel Baluta
2017-04-21 13:07 ` [PATCH 1/2] ASoC: codec: wm9860: avoid maybe-uninitialized warning Daniel Baluta
@ 2017-04-21 13:07 ` Daniel Baluta
2017-04-21 14:44 ` Arnd Bergmann
1 sibling, 1 reply; 8+ messages in thread
From: Daniel Baluta @ 2017-04-21 13:07 UTC (permalink / raw)
To: broonie, tiwai, ckeepax, arnd, lgirdwood
Cc: patches, alsa-devel, linux-kernel, Daniel Baluta
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;
--
2.7.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] ASoC: codec: wm8960: Relax bit clock computation when using PLL
2017-04-21 13:07 ` [PATCH 2/2] ASoC: codec: wm8960: Relax bit clock computation when using PLL Daniel Baluta
@ 2017-04-21 14:44 ` Arnd Bergmann
0 siblings, 0 replies; 8+ messages in thread
From: Arnd Bergmann @ 2017-04-21 14:44 UTC (permalink / raw)
To: Daniel Baluta
Cc: Mark Brown, tiwai, Charles Keepax, Liam Girdwood, patches,
alsa-devel, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] ASoC: codec: wm9860: avoid maybe-uninitialized warning
2017-04-21 13:07 ` [PATCH 1/2] ASoC: codec: wm9860: avoid maybe-uninitialized warning Daniel Baluta
@ 2017-04-21 14:46 ` Arnd Bergmann
2017-04-24 13:15 ` [alsa-devel] " Daniel Baluta
0 siblings, 1 reply; 8+ messages in thread
From: Arnd Bergmann @ 2017-04-21 14:46 UTC (permalink / raw)
To: Daniel Baluta
Cc: Mark Brown, tiwai, Charles Keepax, Liam Girdwood, patches,
alsa-devel, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [alsa-devel] [PATCH 1/2] ASoC: codec: wm9860: avoid maybe-uninitialized warning
2017-04-21 14:46 ` Arnd Bergmann
@ 2017-04-24 13:15 ` Daniel Baluta
2017-04-24 15:27 ` Arnd Bergmann
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Baluta @ 2017-04-24 13:15 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Daniel Baluta, alsa-devel, Linux Kernel Mailing List, patches,
Takashi Iwai, Liam Girdwood, Mark Brown, Charles Keepax
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [alsa-devel] [PATCH 1/2] ASoC: codec: wm9860: avoid maybe-uninitialized warning
2017-04-24 13:15 ` [alsa-devel] " Daniel Baluta
@ 2017-04-24 15:27 ` Arnd Bergmann
2017-04-25 10:17 ` Daniel Baluta
0 siblings, 1 reply; 8+ messages in thread
From: Arnd Bergmann @ 2017-04-24 15:27 UTC (permalink / raw)
To: Daniel Baluta
Cc: Daniel Baluta, alsa-devel, Linux Kernel Mailing List, patches,
Takashi Iwai, Liam Girdwood, Mark Brown, Charles Keepax
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [alsa-devel] [PATCH 1/2] ASoC: codec: wm9860: avoid maybe-uninitialized warning
2017-04-24 15:27 ` Arnd Bergmann
@ 2017-04-25 10:17 ` Daniel Baluta
0 siblings, 0 replies; 8+ messages in thread
From: Daniel Baluta @ 2017-04-25 10:17 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Daniel Baluta, alsa-devel, Linux Kernel Mailing List, patches,
Takashi Iwai, Liam Girdwood, Mark Brown, Charles Keepax
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-04-25 10:17 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-21 13:07 [PATCH 0/2] Relax bitclk computation when using PLL Daniel Baluta
2017-04-21 13:07 ` [PATCH 1/2] ASoC: codec: wm9860: avoid maybe-uninitialized warning Daniel Baluta
2017-04-21 14:46 ` Arnd Bergmann
2017-04-24 13:15 ` [alsa-devel] " Daniel Baluta
2017-04-24 15:27 ` Arnd Bergmann
2017-04-25 10:17 ` Daniel Baluta
2017-04-21 13:07 ` [PATCH 2/2] ASoC: codec: wm8960: Relax bit clock computation when using PLL Daniel Baluta
2017-04-21 14:44 ` Arnd Bergmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).