Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1615190 > unrolled thread

Re: [alsa-devel][PATCH v2 2/2] ASoC: wm8960: Let wm8960 driver configure its bit clock and frame clock

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [alsa-devel][PATCH v2 2/2] ASoC: wm8960: Let wm8960 driver  configure its bit clock and frame clock Daniel Baluta <daniel.baluta@gmail.com> - 2017-04-03 15:20 +0200
    Re: [alsa-devel][PATCH v2 2/2] ASoC: wm8960: Let wm8960 driver  configure its bit clock and frame clock Daniel Baluta <daniel.baluta@gmail.com> - 2017-04-03 15:40 +0200
      Re: [alsa-devel][PATCH v2 2/2] ASoC: wm8960: Let wm8960 driver  configure its bit clock and frame clock Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2017-04-03 16:00 +0200
        Re: [alsa-devel][PATCH v2 2/2] ASoC: wm8960: Let wm8960 driver  configure its bit clock and frame clock Daniel Baluta <daniel.baluta@gmail.com> - 2017-04-04 10:00 +0200
          Re: [alsa-devel][PATCH v2 2/2] ASoC: wm8960: Let wm8960 driver  configure its bit clock and frame clock Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2017-04-04 11:00 +0200
    Re: [alsa-devel][PATCH v2 2/2] ASoC: wm8960: Let wm8960 driver  configure its bit clock and frame clock Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2017-04-03 15:40 +0200

#1615190 — Re: [alsa-devel][PATCH v2 2/2] ASoC: wm8960: Let wm8960 driver configure its bit clock and frame clock

On Thu, Jan 15, 2015 at 3:34 PM, Zidan Wang <b50113@freescale.com> wrote:
> On Wed, Jan 14, 2015 at 07:27:03PM +0000, Mark Brown wrote:
>> On Wed, Jan 07, 2015 at 03:31:45PM +0800, Zidan Wang wrote:
>>
>> > +   for (i = 0; i < ARRAY_SIZE(dac_divs); ++i) {
>> > +           if (wm8960->sysclk == lrclk * dac_divs[i]) {
>> > +                   for (j = 0; j < ARRAY_SIZE(bclk_divs); ++j) {
>> > +                           if (wm8960->sysclk ==  wm8960->bclk *
>> > +                                           bclk_divs[j] / 10) {
>> > +                                   goto config_clock;
>> > +                           }
>> > +                   }
>> > +           }
>> > +   }
>> > +
>> > +   dev_err(codec->dev, "Unsupported sysclk %d\n", wm8960->sysclk);
>> > +   return;
>>
>> It's a bit awkward using the goto like this.  A more common way of
>> writing this is to change the above block to be
>>
>>       if (i == ARRAY_SIZE(dac_divs))
>>               /* return error */
>>
>> rather than skipping over the error.  Otherwise this looks good.
>
> Hi Mark,
>
> I found it can't generate bclk for S20_3LE data format.
>
> For 2 channel S20_3LE data format:
>
> bclk = fs * 20 * 2
> Sysclk = BCLKDIV * bclk = BCLKDIV * fs * 40
> Sysclk = DACDIV * fs * 256
>
> BCLKDIV/DACDIV = 256/40 = 32/5
>
> But BCLKDIV/DACDIV can't be 32/5. So I want to support tdm slot.
>
> bclk = fs * slot_width * slots * channal.
>
> Do you think it make sense, or any other ideas?

Reviving this question after two years :).

After "ASoC: codec: wm8960: Relax bit clock computation" patch

https://patchwork.kernel.org/patch/9636769/

we can now support S20_3LE for round rates like 8000, 16000,
32000 and 48000.

But not for 11025, 22050, 441000. Do you think it's worth exploring
"tdm slot" idea? I don't know exactly what it implies.

Another idea, is to completely remove support for S20_3LE since it
is not trivial to derive bitclk from sysclk.

What do you guys think?

Daniel.

[toc] | [next] | [standalone]

#1615205

On Mon, Apr 3, 2017 at 4:34 PM, Charles Keepax
<ckeepax@opensource.wolfsonmicro.com> wrote:
> On Mon, Apr 03, 2017 at 04:16:23PM +0300, Daniel Baluta wrote:
>> On Thu, Jan 15, 2015 at 3:34 PM, Zidan Wang <b50113@freescale.com> wrote:
>> > On Wed, Jan 14, 2015 at 07:27:03PM +0000, Mark Brown wrote:
>> >> On Wed, Jan 07, 2015 at 03:31:45PM +0800, Zidan Wang wrote:
>> > I found it can't generate bclk for S20_3LE data format.
>> >
>> > For 2 channel S20_3LE data format:
>> >
>> > bclk = fs * 20 * 2
>> > Sysclk = BCLKDIV * bclk = BCLKDIV * fs * 40
>> > Sysclk = DACDIV * fs * 256
>> >
>> > BCLKDIV/DACDIV = 256/40 = 32/5
>> >
>> > But BCLKDIV/DACDIV can't be 32/5. So I want to support tdm slot.
>> >
>> > bclk = fs * slot_width * slots * channal.
>> >
>> > Do you think it make sense, or any other ideas?
>>
>> Reviving this question after two years :).
>>
>> After "ASoC: codec: wm8960: Relax bit clock computation" patch
>>
>> https://patchwork.kernel.org/patch/9636769/
>>
>> we can now support S20_3LE for round rates like 8000, 16000,
>> 32000 and 48000.
>>
>> But not for 11025, 22050, 441000. Do you think it's worth exploring
>> "tdm slot" idea? I don't know exactly what it implies.
>>
>> Another idea, is to completely remove support for S20_3LE since it
>> is not trivial to derive bitclk from sysclk.
>>
>> What do you guys think?
>
> Does this problem still remain after the relaxed clock
> computation? The maths you quote depends on the derived BCLK
> being exactly the correct speed for the audio, that is no longer
> the case anymore.
>
> I would have thought the patch would cover both situations, as in
> if we can produce a suitable LRCLK, then we just pick a BCLK we

That!

The problem for remaining rates is that we cannot derive the LRCLK

<snip>
+ for (j = 0; j < ARRAY_SIZE(dac_divs); ++j) {
+ if (sysclk != dac_divs[j] * lrclk)
+ continue;
</snip>


> can produce that is higher than we need. I don't see why that
> depends on things being a 48k based rate there. Am I missing
> something?

[toc] | [prev] | [next] | [standalone]

#1615221

On Mon, Apr 03, 2017 at 04:39:40PM +0300, Daniel Baluta wrote:
> On Mon, Apr 3, 2017 at 4:34 PM, Charles Keepax
> <ckeepax@opensource.wolfsonmicro.com> wrote:
> > On Mon, Apr 03, 2017 at 04:16:23PM +0300, Daniel Baluta wrote:
> > Does this problem still remain after the relaxed clock
> > computation? The maths you quote depends on the derived BCLK
> > being exactly the correct speed for the audio, that is no longer
> > the case anymore.
> >
> > I would have thought the patch would cover both situations, as in
> > if we can produce a suitable LRCLK, then we just pick a BCLK we
> 
> That!
> 
> The problem for remaining rates is that we cannot derive the LRCLK
> 
> <snip>
> + for (j = 0; j < ARRAY_SIZE(dac_divs); ++j) {
> + if (sysclk != dac_divs[j] * lrclk)
> + continue;
> </snip>
> 

If you can't generate the LRCLK you either need a different
source clock or to use the PLL. You don't want to be trying to
pull 44.1k audio over a link that is clocked on a 48k based
clock.

Is the problem here that the PLL part of the code is making the
same assumption as the direct part of the code was, that the bclk
should be exact?

Thanks,
Charles

[toc] | [prev] | [next] | [standalone]

#1615762

<Removing Zidan from thread because the address no longer exists>

On Mon, Apr 3, 2017 at 4:54 PM, Charles Keepax
<ckeepax@opensource.wolfsonmicro.com> wrote:
> On Mon, Apr 03, 2017 at 04:39:40PM +0300, Daniel Baluta wrote:
>> On Mon, Apr 3, 2017 at 4:34 PM, Charles Keepax
>> <ckeepax@opensource.wolfsonmicro.com> wrote:
>> > On Mon, Apr 03, 2017 at 04:16:23PM +0300, Daniel Baluta wrote:
>> > Does this problem still remain after the relaxed clock
>> > computation? The maths you quote depends on the derived BCLK
>> > being exactly the correct speed for the audio, that is no longer
>> > the case anymore.
>> >
>> > I would have thought the patch would cover both situations, as in
>> > if we can produce a suitable LRCLK, then we just pick a BCLK we
>>
>> That!
>>
>> The problem for remaining rates is that we cannot derive the LRCLK
>>
>> <snip>
>> + for (j = 0; j < ARRAY_SIZE(dac_divs); ++j) {
>> + if (sysclk != dac_divs[j] * lrclk)
>> + continue;
>> </snip>
>>
>
> If you can't generate the LRCLK you either need a different
> source clock or to use the PLL. You don't want to be trying to
> pull 44.1k audio over a link that is clocked on a 48k based
> clock.

Yup, this makes sense to me.

>
> Is the problem here that the PLL part of the code is making the
> same assumption as the direct part of the code was, that the bclk
> should be exact?

Yes.


After wm8960_configure_sysclk fails to find a LRCLK, we try to use the
PLL.

Anyhow, here we don't even reach to check if the PLL can be used because
there is no solution for the following system:

freq_out = sysclk * sysclk_divs[i];
sysclk = lrclk * dac_divs[j];
sysclk == bclk * bclk_divs[k]


Perhaps, we can also try here to relax bitclk computation like we did for when
sysclk was directly derived from mclk.

thanks,
Daniel.

[toc] | [prev] | [next] | [standalone]

#1615799

On Tue, Apr 04, 2017 at 10:55:00AM +0300, Daniel Baluta wrote:
> <Removing Zidan from thread because the address no longer exists>
> 
> On Mon, Apr 3, 2017 at 4:54 PM, Charles Keepax
> <ckeepax@opensource.wolfsonmicro.com> wrote:
> > On Mon, Apr 03, 2017 at 04:39:40PM +0300, Daniel Baluta wrote:
> >> On Mon, Apr 3, 2017 at 4:34 PM, Charles Keepax
> >> <ckeepax@opensource.wolfsonmicro.com> wrote:
> > Is the problem here that the PLL part of the code is making the
> > same assumption as the direct part of the code was, that the bclk
> > should be exact?
> 
> Yes.
> 
> 
> After wm8960_configure_sysclk fails to find a LRCLK, we try to use the
> PLL.
> 
> Anyhow, here we don't even reach to check if the PLL can be used because
> there is no solution for the following system:
> 
> freq_out = sysclk * sysclk_divs[i];
> sysclk = lrclk * dac_divs[j];
> sysclk == bclk * bclk_divs[k]
> 
> 
> Perhaps, we can also try here to relax bitclk computation like we did for when
> sysclk was directly derived from mclk.

Exactly that is what I am saying it looks like the PLL part
of the process still assumes it requires bclk to be an exact
frequency if we relax that, the same way we did for the direct
MCLK then we should be good.

Thanks,
Charles

[toc] | [prev] | [next] | [standalone]

#1615208

On Mon, Apr 03, 2017 at 04:16:23PM +0300, Daniel Baluta wrote:
> On Thu, Jan 15, 2015 at 3:34 PM, Zidan Wang <b50113@freescale.com> wrote:
> > On Wed, Jan 14, 2015 at 07:27:03PM +0000, Mark Brown wrote:
> >> On Wed, Jan 07, 2015 at 03:31:45PM +0800, Zidan Wang wrote:
> > I found it can't generate bclk for S20_3LE data format.
> >
> > For 2 channel S20_3LE data format:
> >
> > bclk = fs * 20 * 2
> > Sysclk = BCLKDIV * bclk = BCLKDIV * fs * 40
> > Sysclk = DACDIV * fs * 256
> >
> > BCLKDIV/DACDIV = 256/40 = 32/5
> >
> > But BCLKDIV/DACDIV can't be 32/5. So I want to support tdm slot.
> >
> > bclk = fs * slot_width * slots * channal.
> >
> > Do you think it make sense, or any other ideas?
> 
> Reviving this question after two years :).
> 
> After "ASoC: codec: wm8960: Relax bit clock computation" patch
> 
> https://patchwork.kernel.org/patch/9636769/
> 
> we can now support S20_3LE for round rates like 8000, 16000,
> 32000 and 48000.
> 
> But not for 11025, 22050, 441000. Do you think it's worth exploring
> "tdm slot" idea? I don't know exactly what it implies.
> 
> Another idea, is to completely remove support for S20_3LE since it
> is not trivial to derive bitclk from sysclk.
> 
> What do you guys think?

Does this problem still remain after the relaxed clock
computation? The maths you quote depends on the derived BCLK
being exactly the correct speed for the audio, that is no longer
the case anymore.

I would have thought the patch would cover both situations, as in
if we can produce a suitable LRCLK, then we just pick a BCLK we
can produce that is higher than we need. I don't see why that
depends on things being a 48k based rate there. Am I missing
something?

Thanks,
Charles

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web