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
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 <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Daniel Baluta <[email protected]>
---
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
On Fri, Apr 21, 2017 at 3:07 PM, Daniel Baluta <[email protected]> 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 <[email protected]>
> Signed-off-by: Daniel Baluta <[email protected]>
> ---
> 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 3:07 PM, Daniel Baluta <[email protected]> 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 <[email protected]>
> ---
> 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
On Fri, Apr 21, 2017 at 5:46 PM, Arnd Bergmann <[email protected]> wrote:
> On Fri, Apr 21, 2017 at 3:07 PM, Daniel Baluta <[email protected]> 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 <[email protected]>
>> Signed-off-by: Daniel Baluta <[email protected]>
>> ---
>> 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 <[email protected]> wrote:
> On Fri, Apr 21, 2017 at 5:46 PM, Arnd Bergmann <[email protected]> wrote:
>> On Fri, Apr 21, 2017 at 3:07 PM, Daniel Baluta <[email protected]> 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 <[email protected]>
>>> Signed-off-by: Daniel Baluta <[email protected]>
>>> ---
>>> 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 <[email protected]> wrote:
> On Mon, Apr 24, 2017 at 3:15 PM, Daniel Baluta <[email protected]> wrote:
>> On Fri, Apr 21, 2017 at 5:46 PM, Arnd Bergmann <[email protected]> wrote:
>>> On Fri, Apr 21, 2017 at 3:07 PM, Daniel Baluta <[email protected]> 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 <[email protected]>
>>>> Signed-off-by: Daniel Baluta <[email protected]>
>>>> ---
>>>> 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.