* Re: [RFC PATCH] ASoC: wm8960: Use physical width for bclk
@ 2017-03-15 9:19 ` Charles Keepax
0 siblings, 0 replies; 10+ messages in thread
From: Charles Keepax @ 2017-03-15 9:19 UTC (permalink / raw)
To: Daniel Baluta
Cc: lgirdwood, broonie, tiwai, patches, alsa-devel, linux-kernel,
shengjiu.wang, mihai.serban, viorel.suman
On Tue, Mar 14, 2017 at 06:57:02PM +0200, Daniel Baluta wrote:
> bclk is derived from sysclk with the help of bclk_divs. Anyhow, for
> S20_3LE format there is no bclk_divs that could be used to derive
> an exact bclk.
>
> We can fix this by using storage size instead the exact
> number of bits of the sample when computing bclk.
>
> With this approach we can play S20_3LE encoded files at the cost of
> some unused BCLK cycles for S20_3LE and S24_LE.
>
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> ---
> sound/soc/codecs/wm8960.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/sound/soc/codecs/wm8960.c b/sound/soc/codecs/wm8960.c
> index e1429e3..18ca5fc 100644
> --- a/sound/soc/codecs/wm8960.c
> +++ b/sound/soc/codecs/wm8960.c
> @@ -722,7 +722,8 @@ static int wm8960_hw_params(struct snd_pcm_substream *substream,
> bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
> int i;
>
> - wm8960->bclk = snd_soc_params_to_bclk(params);
> + wm8960->bclk = params_physical_width(params) *
> + params_channels(params) * params_rate(params);
> if (params_channels(params) == 1)
> wm8960->bclk *= 2;
Would quite like to see Mark's thoughts on this. Feels a bit to
me (although I am not certain) like we are solving the problem in
the wrong place, isn't this really setting the desired BCLK and
then in wm8960_configure_clocking we should set the lowest BCLK
we can that is greater than or equal to the desired.
Thanks,
Charles
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] ASoC: wm8960: Use physical width for bclk
2017-03-15 9:19 ` Charles Keepax
@ 2017-03-15 11:43 ` Daniel Baluta
-1 siblings, 0 replies; 10+ messages in thread
From: Daniel Baluta @ 2017-03-15 11:43 UTC (permalink / raw)
To: ckeepax@opensource.wolfsonmicro.com, broonie@kernel.org
Cc: shengjiu.wang@freescale.com, alsa-devel@alsa-project.org,
patches@opensource.wolfsonmicro.com, lgirdwood@gmail.com,
linux-kernel@vger.kernel.org, Viorel Suman, Mihai Serban,
tiwai@suse.com
On Mi, 2017-03-15 at 09:19 +0000, Charles Keepax wrote:
> On Tue, Mar 14, 2017 at 06:57:02PM +0200, Daniel Baluta wrote:
> >
> > bclk is derived from sysclk with the help of bclk_divs. Anyhow, for
> > S20_3LE format there is no bclk_divs that could be used to derive
> > an exact bclk.
> >
> > We can fix this by using storage size instead the exact
> > number of bits of the sample when computing bclk.
> >
> > With this approach we can play S20_3LE encoded files at the cost of
> > some unused BCLK cycles for S20_3LE and S24_LE.
> >
> > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> > ---
> > sound/soc/codecs/wm8960.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/sound/soc/codecs/wm8960.c b/sound/soc/codecs/wm8960.c
> > index e1429e3..18ca5fc 100644
> > --- a/sound/soc/codecs/wm8960.c
> > +++ b/sound/soc/codecs/wm8960.c
> > @@ -722,7 +722,8 @@ static int wm8960_hw_params(struct
> > snd_pcm_substream *substream,
> > bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
> > int i;
> >
> > - wm8960->bclk = snd_soc_params_to_bclk(params);
> > + wm8960->bclk = params_physical_width(params) *
> > + params_channels(params) * params_rate(params);
> > if (params_channels(params) == 1)
> > wm8960->bclk *= 2;
> Would quite like to see Mark's thoughts on this. Feels a bit to
> me (although I am not certain) like we are solving the problem in
> the wrong place, isn't this really setting the desired BCLK and
> then in wm8960_configure_clocking we should set the lowest BCLK
> we can that is greater than or equal to the desired.
Is there any standard way of doing this in ASoC? Couldn't find any
example where the bitclk is relaxed to lowest value greater than the
desired one.
I have prepared some patches with this approach, you can look at them
here:
http://pastebin.com/0xW0E82N - some refactoring of sysclk search
http://pastebin.com/QRt1UT7Q - relax bitclk value
Daniel.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] ASoC: wm8960: Use physical width for bclk
@ 2017-03-15 11:43 ` Daniel Baluta
0 siblings, 0 replies; 10+ messages in thread
From: Daniel Baluta @ 2017-03-15 11:43 UTC (permalink / raw)
To: ckeepax@opensource.wolfsonmicro.com, broonie@kernel.org
Cc: linux-kernel@vger.kernel.org, shengjiu.wang@freescale.com,
patches@opensource.wolfsonmicro.com, tiwai@suse.com,
lgirdwood@gmail.com, Viorel Suman, Mihai Serban,
alsa-devel@alsa-project.org
On Mi, 2017-03-15 at 09:19 +0000, Charles Keepax wrote:
> On Tue, Mar 14, 2017 at 06:57:02PM +0200, Daniel Baluta wrote:
> >
> > bclk is derived from sysclk with the help of bclk_divs. Anyhow, for
> > S20_3LE format there is no bclk_divs that could be used to derive
> > an exact bclk.
> >
> > We can fix this by using storage size instead the exact
> > number of bits of the sample when computing bclk.
> >
> > With this approach we can play S20_3LE encoded files at the cost of
> > some unused BCLK cycles for S20_3LE and S24_LE.
> >
> > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> > ---
> > sound/soc/codecs/wm8960.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/sound/soc/codecs/wm8960.c b/sound/soc/codecs/wm8960.c
> > index e1429e3..18ca5fc 100644
> > --- a/sound/soc/codecs/wm8960.c
> > +++ b/sound/soc/codecs/wm8960.c
> > @@ -722,7 +722,8 @@ static int wm8960_hw_params(struct
> > snd_pcm_substream *substream,
> > bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
> > int i;
> >
> > - wm8960->bclk = snd_soc_params_to_bclk(params);
> > + wm8960->bclk = params_physical_width(params) *
> > + params_channels(params) * params_rate(params);
> > if (params_channels(params) == 1)
> > wm8960->bclk *= 2;
> Would quite like to see Mark's thoughts on this. Feels a bit to
> me (although I am not certain) like we are solving the problem in
> the wrong place, isn't this really setting the desired BCLK and
> then in wm8960_configure_clocking we should set the lowest BCLK
> we can that is greater than or equal to the desired.
Is there any standard way of doing this in ASoC? Couldn't find any
example where the bitclk is relaxed to lowest value greater than the
desired one.
I have prepared some patches with this approach, you can look at them
here:
http://pastebin.com/0xW0E82N - some refactoring of sysclk search
http://pastebin.com/QRt1UT7Q - relax bitclk value
Daniel.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] ASoC: wm8960: Use physical width for bclk
2017-03-15 9:19 ` Charles Keepax
@ 2017-03-15 13:33 ` Mark Brown
-1 siblings, 0 replies; 10+ messages in thread
From: Mark Brown @ 2017-03-15 13:33 UTC (permalink / raw)
To: Charles Keepax
Cc: alsa-devel, shengjiu.wang, patches, linux-kernel, lgirdwood,
viorel.suman, mihai.serban, tiwai, Daniel Baluta
[-- Attachment #1.1: Type: text/plain, Size: 906 bytes --]
On Wed, Mar 15, 2017 at 09:19:01AM +0000, Charles Keepax wrote:
> On Tue, Mar 14, 2017 at 06:57:02PM +0200, Daniel Baluta wrote:
> > - wm8960->bclk = snd_soc_params_to_bclk(params);
> > + wm8960->bclk = params_physical_width(params) *
> > + params_channels(params) * params_rate(params);
> > if (params_channels(params) == 1)
> > wm8960->bclk *= 2;
> Would quite like to see Mark's thoughts on this. Feels a bit to
> me (although I am not certain) like we are solving the problem in
> the wrong place, isn't this really setting the desired BCLK and
> then in wm8960_configure_clocking we should set the lowest BCLK
> we can that is greater than or equal to the desired.
Exactly, this is the wrong fix in the wrong place - if this is something
we want to do we should be doing it in the shared functions so all
CODECs get the same behaviour but really I think Charles' suggestion
makes more sense.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] ASoC: wm8960: Use physical width for bclk
@ 2017-03-15 13:33 ` Mark Brown
0 siblings, 0 replies; 10+ messages in thread
From: Mark Brown @ 2017-03-15 13:33 UTC (permalink / raw)
To: Charles Keepax
Cc: Daniel Baluta, lgirdwood, tiwai, patches, alsa-devel,
linux-kernel, shengjiu.wang, mihai.serban, viorel.suman
[-- Attachment #1: Type: text/plain, Size: 906 bytes --]
On Wed, Mar 15, 2017 at 09:19:01AM +0000, Charles Keepax wrote:
> On Tue, Mar 14, 2017 at 06:57:02PM +0200, Daniel Baluta wrote:
> > - wm8960->bclk = snd_soc_params_to_bclk(params);
> > + wm8960->bclk = params_physical_width(params) *
> > + params_channels(params) * params_rate(params);
> > if (params_channels(params) == 1)
> > wm8960->bclk *= 2;
> Would quite like to see Mark's thoughts on this. Feels a bit to
> me (although I am not certain) like we are solving the problem in
> the wrong place, isn't this really setting the desired BCLK and
> then in wm8960_configure_clocking we should set the lowest BCLK
> we can that is greater than or equal to the desired.
Exactly, this is the wrong fix in the wrong place - if this is something
we want to do we should be doing it in the shared functions so all
CODECs get the same behaviour but really I think Charles' suggestion
makes more sense.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] ASoC: wm8960: Use physical width for bclk
2017-03-15 13:33 ` Mark Brown
@ 2017-03-15 15:11 ` Daniel Baluta
-1 siblings, 0 replies; 10+ messages in thread
From: Daniel Baluta @ 2017-03-15 15:11 UTC (permalink / raw)
To: Mark Brown
Cc: shengjiu.wang, alsa-devel, patches, lgirdwood,
Linux Kernel Mailing List, viorel.suman, Takashi Iwai,
mihai.serban, Charles Keepax, Daniel Baluta
On Wed, Mar 15, 2017 at 3:33 PM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Mar 15, 2017 at 09:19:01AM +0000, Charles Keepax wrote:
>> On Tue, Mar 14, 2017 at 06:57:02PM +0200, Daniel Baluta wrote:
>
>> > - wm8960->bclk = snd_soc_params_to_bclk(params);
>> > + wm8960->bclk = params_physical_width(params) *
>> > + params_channels(params) * params_rate(params);
>> > if (params_channels(params) == 1)
>> > wm8960->bclk *= 2;
>
>> Would quite like to see Mark's thoughts on this. Feels a bit to
>> me (although I am not certain) like we are solving the problem in
>> the wrong place, isn't this really setting the desired BCLK and
>> then in wm8960_configure_clocking we should set the lowest BCLK
>> we can that is greater than or equal to the desired.
>
> Exactly, this is the wrong fix in the wrong place - if this is something
> we want to do we should be doing it in the shared functions so all
> CODECs get the same behaviour but really I think Charles' suggestion
> makes more sense.
I see your point. Thanks a lot for feedback.
I already have an implementation for Charles' suggestion.
Will send the patches asap.
thanks,
Daniel.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [alsa-devel] [RFC PATCH] ASoC: wm8960: Use physical width for bclk
@ 2017-03-15 15:11 ` Daniel Baluta
0 siblings, 0 replies; 10+ messages in thread
From: Daniel Baluta @ 2017-03-15 15:11 UTC (permalink / raw)
To: Mark Brown
Cc: Charles Keepax, alsa-devel, shengjiu.wang, patches,
Linux Kernel Mailing List, lgirdwood, viorel.suman, mihai.serban,
Takashi Iwai, Daniel Baluta
On Wed, Mar 15, 2017 at 3:33 PM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Mar 15, 2017 at 09:19:01AM +0000, Charles Keepax wrote:
>> On Tue, Mar 14, 2017 at 06:57:02PM +0200, Daniel Baluta wrote:
>
>> > - wm8960->bclk = snd_soc_params_to_bclk(params);
>> > + wm8960->bclk = params_physical_width(params) *
>> > + params_channels(params) * params_rate(params);
>> > if (params_channels(params) == 1)
>> > wm8960->bclk *= 2;
>
>> Would quite like to see Mark's thoughts on this. Feels a bit to
>> me (although I am not certain) like we are solving the problem in
>> the wrong place, isn't this really setting the desired BCLK and
>> then in wm8960_configure_clocking we should set the lowest BCLK
>> we can that is greater than or equal to the desired.
>
> Exactly, this is the wrong fix in the wrong place - if this is something
> we want to do we should be doing it in the shared functions so all
> CODECs get the same behaviour but really I think Charles' suggestion
> makes more sense.
I see your point. Thanks a lot for feedback.
I already have an implementation for Charles' suggestion.
Will send the patches asap.
thanks,
Daniel.
^ permalink raw reply [flat|nested] 10+ messages in thread