[PATCH] [v2] ASoC: support all possible sample rates in the WM8776 driver

[PATCH] [v2] ASoC: support all possible sample rates in the WM8776 driver

[Timur Tabi]

The .set_sysclk() function in ASoC codec drivers can be used to tell the
driver what the frequency of master clock (mclk) is.  Some codecs use a
divider on this clock to determine the sample rate.  The WM8776 is one such
codec.

So instead of hard-coding a list of specific sample rates supported, these
codec drivers can specify SNDRV_PCM_RATE_CONTINUOUS instead, clamping the
range to the absolute limits of the hardware.  Then the .hw_params() function
can use the mclk rate to determine whether any requested rate is actually
supported.

Although the WM8776 driver includes a .set_sysclk function, it was also
hard-coding the list of supported sample rates.  We change the hard-coded
list to a range within the capabilities of the WM8776 itself.

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 sound/soc/codecs/wm8776.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/sound/soc/codecs/wm8776.c b/sound/soc/codecs/wm8776.c
index ad6f0fa..ca8a593 100644
--- a/sound/soc/codecs/wm8776.c
+++ b/sound/soc/codecs/wm8776.c
@@ -321,11 +321,6 @@ static int wm8776_set_bias_level(struct snd_soc_codec *codec,
 	return 0;
 }
 
-#define WM8776_RATES (SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 |\
-		      SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_88200 |\
-		      SNDRV_PCM_RATE_96000)
-
-
 #define WM8776_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE |\
 			SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE)
 
@@ -350,7 +345,9 @@ static struct snd_soc_dai_driver wm8776_dai[] = {
 			.stream_name = "Playback",
 			.channels_min = 2,
 			.channels_max = 2,
-			.rates = WM8776_RATES,
+			.rates = SNDRV_PCM_RATE_CONTINUOUS,
+			.rate_min = 32000,
+			.rate_max = 192000,
 			.formats = WM8776_FORMATS,
 		},
 		.ops = &wm8776_dac_ops,
@@ -362,7 +359,9 @@ static struct snd_soc_dai_driver wm8776_dai[] = {
 			.stream_name = "Capture",
 			.channels_min = 2,
 			.channels_max = 2,
-			.rates = WM8776_RATES,
+			.rates = SNDRV_PCM_RATE_CONTINUOUS,
+			.rate_min = 32000,
+			.rate_max = 96000,
 			.formats = WM8776_FORMATS,
 		},
 		.ops = &wm8776_adc_ops,
-- 
1.7.3.4

Re: [PATCH] [v2] ASoC: support all possible sample rates in the WM8776 driver

[Takashi Iwai]

At Fri, 16 Sep 2011 09:16:54 -0500,
Timur Tabi wrote:
> 
> The .set_sysclk() function in ASoC codec drivers can be used to tell the
> driver what the frequency of master clock (mclk) is.  Some codecs use a
> divider on this clock to determine the sample rate.  The WM8776 is one such
> codec.
> 
> So instead of hard-coding a list of specific sample rates supported, these
> codec drivers can specify SNDRV_PCM_RATE_CONTINUOUS instead, clamping the
> range to the absolute limits of the hardware.  Then the .hw_params() function
> can use the mclk rate to determine whether any requested rate is actually
> supported.
> 
> Although the WM8776 driver includes a .set_sysclk function, it was also
> hard-coding the list of supported sample rates.  We change the hard-coded
> list to a range within the capabilities of the WM8776 itself.

It's not optimal from some aspects.  Basically this should be resolved
in hw_constraints, not in hw_params, so that the configurator can find
the possible rates.  Otherwise you have no idea what rate would be
accepted.

Since a few other codecs need the similar constraint, it may make
sense to have a common helper hw_constraint for such a case.


thanks,

Takashi

> 
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
>  sound/soc/codecs/wm8776.c |   13 ++++++-------
>  1 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/sound/soc/codecs/wm8776.c b/sound/soc/codecs/wm8776.c
> index ad6f0fa..ca8a593 100644
> --- a/sound/soc/codecs/wm8776.c
> +++ b/sound/soc/codecs/wm8776.c
> @@ -321,11 +321,6 @@ static int wm8776_set_bias_level(struct snd_soc_codec *codec,
>  	return 0;
>  }
>  
> -#define WM8776_RATES (SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 |\
> -		      SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_88200 |\
> -		      SNDRV_PCM_RATE_96000)
> -
> -
>  #define WM8776_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE |\
>  			SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE)
>  
> @@ -350,7 +345,9 @@ static struct snd_soc_dai_driver wm8776_dai[] = {
>  			.stream_name = "Playback",
>  			.channels_min = 2,
>  			.channels_max = 2,
> -			.rates = WM8776_RATES,
> +			.rates = SNDRV_PCM_RATE_CONTINUOUS,
> +			.rate_min = 32000,
> +			.rate_max = 192000,
>  			.formats = WM8776_FORMATS,
>  		},
>  		.ops = &wm8776_dac_ops,
> @@ -362,7 +359,9 @@ static struct snd_soc_dai_driver wm8776_dai[] = {
>  			.stream_name = "Capture",
>  			.channels_min = 2,
>  			.channels_max = 2,
> -			.rates = WM8776_RATES,
> +			.rates = SNDRV_PCM_RATE_CONTINUOUS,
> +			.rate_min = 32000,
> +			.rate_max = 96000,
>  			.formats = WM8776_FORMATS,
>  		},
>  		.ops = &wm8776_adc_ops,
> -- 
> 1.7.3.4
> 
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [PATCH] [v2] ASoC: support all possible sample rates in the WM8776 driver

[Timur Tabi]

Takashi Iwai wrote:

> It's not optimal from some aspects.  Basically this should be resolved
> in hw_constraints, not in hw_params, so that the configurator can find
> the possible rates.  Otherwise you have no idea what rate would be
> accepted.

Yes, I was concerned about that.  So should I be calling one of the
snd_pcm_hw_constraint_xxx functions in the codec's .startup function?  That
would require ASoC to call the machine driver's .startup function *before*
calling the codec driver's .startup function, since the machine driver's
.startup function is where I call the codec to tell it what the mclk frequency is.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] [v2] ASoC: support all possible sample rates in the WM8776 driver

[Mark Brown]

On Fri, Sep 16, 2011 at 10:47:39AM -0500, Timur Tabi wrote:

> Yes, I was concerned about that.  So should I be calling one of the
> snd_pcm_hw_constraint_xxx functions in the codec's .startup function?  That
> would require ASoC to call the machine driver's .startup function *before*
> calling the codec driver's .startup function, since the machine driver's
> .startup function is where I call the codec to tell it what the mclk frequency is.

This isn't good for systems which can dynamically configure the clocks
based on the sample rate, they will wish to reconfigure things after the
user selected the sample rate in hw_params().  I've said several times
that this is the reason we don't actually advertise rates based on the
current rates.

Machine drivers are currently best placed to set constraints if the
clocking is limited.

Re: [PATCH] [v2] ASoC: support all possible sample rates in the WM8776 driver

[Timur Tabi]

Mark Brown wrote:
> Machine drivers are currently best placed to set constraints if the
> clocking is limited.

But how is the machine driver supposed to know what those sample rates are?  It
would need to know how which dividers that codec uses.  That would mean that the
machine driver has to be hard-codec with information on the internals of the codec.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] [v2] ASoC: support all possible sample rates in the WM8776 driver

[Mark Brown]

On Fri, Sep 16, 2011 at 11:33:38AM -0500, Timur Tabi wrote:
> Mark Brown wrote:
> > Machine drivers are currently best placed to set constraints if the
> > clocking is limited.

> But how is the machine driver supposed to know what those sample rates
> are?  It would need to know how which dividers that codec uses.  That
> would mean that the machine driver has to be hard-codec with
> information on the internals of the codec.

You'd have to look at the CODEC datasheet and figure out what it's
capable of given the clocks you're able to give it.  Depending on the
flexibility you've got and power you're willing to spend there may be no
need to do anything as you may be able to generate any clocks the device
may need (for example with devices that have FLLs).

Re: [PATCH] [v2] ASoC: support all possible sample rates in the WM8776 driver

[Timur Tabi]

Mark Brown wrote:
>> > But how is the machine driver supposed to know what those sample rates
>> > are?  It would need to know how which dividers that codec uses.  That
>> > would mean that the machine driver has to be hard-codec with
>> > information on the internals of the codec.

> You'd have to look at the CODEC datasheet and figure out what it's
> capable of given the clocks you're able to give it.  Depending on the
> flexibility you've got and power you're willing to spend there may be no
> need to do anything as you may be able to generate any clocks the device
> may need (for example with devices that have FLLs).

Well, my point was that this introduces a lot of information that's specific to
the internals of the codec into the machine driver.  Today, the only thing the
machine driver needs to know is the name of the DAIs:

	mdata->dai[0].codec_dai_name = "wm8776-hifi-playback";
	mdata->dai[1].codec_dai_name = "wm8776-hifi-capture";

and it might even be possible to avoid this.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] [v2] ASoC: support all possible sample rates in the WM8776 driver

[Lars-Peter Clausen]

On 09/16/2011 05:47 PM, Timur Tabi wrote:
> Takashi Iwai wrote:
> 
>> It's not optimal from some aspects.  Basically this should be resolved
>> in hw_constraints, not in hw_params, so that the configurator can find
>> the possible rates.  Otherwise you have no idea what rate would be
>> accepted.
> 
> Yes, I was concerned about that.  So should I be calling one of the
> snd_pcm_hw_constraint_xxx functions in the codec's .startup function?  That
> would require ASoC to call the machine driver's .startup function *before*
> calling the codec driver's .startup function, since the machine driver's
> .startup function is where I call the codec to tell it what the mclk frequency is.
> 

You mentioned in an earlyer mail that you can switch the sysclk rate at
runtime. So setting the constraints based on the current sysclk rate won't
really work. I think you need some mechanism to specify the supported rates on
a per machine driver basis.

Re: [PATCH] [v2] ASoC: support all possible sample rates in the WM8776 driver

[Mark Brown]

On Fri, Sep 16, 2011 at 06:27:14PM +0200, Lars-Peter Clausen wrote:

> You mentioned in an earlyer mail that you can switch the sysclk rate at
> runtime. So setting the constraints based on the current sysclk rate won't
> really work. I think you need some mechanism to specify the supported rates on
> a per machine driver basis.

Yes, exactly.  It's not something the CODEC driver can do.

Re: [PATCH] [v2] ASoC: support all possible sample rates in the WM8776 driver

[Timur Tabi]

Lars-Peter Clausen wrote:
> You mentioned in an earlyer mail that you can switch the sysclk rate at
> runtime. So setting the constraints based on the current sysclk rate won't
> really work. I think you need some mechanism to specify the supported rates on
> a per machine driver basis.

Exactly.  There are some other problems with getting the dynamic sysclk feature
working.  On the board where this is supported, the same clock is connected to
adcmclk and dacmclk, so I can't support playback at 48KHz and capture at
44.1KHz.  However, there's nothing stopping a customer from creating a board
that has two independent clocks.

So in the meantime, I'd really just like the ability to specify in the codec
driver's .startup function exactly which sample rates are supported (based on
the current mclk).

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] [v2] ASoC: support all possible sample rates in the WM8776 driver

[Lars-Peter Clausen]

On 09/16/2011 06:34 PM, Timur Tabi wrote:
> Lars-Peter Clausen wrote:
>> You mentioned in an earlyer mail that you can switch the sysclk rate at
>> runtime. So setting the constraints based on the current sysclk rate won't
>> really work. I think you need some mechanism to specify the supported rates on
>> a per machine driver basis.
> 
> Exactly.  There are some other problems with getting the dynamic sysclk feature
> working.  On the board where this is supported, the same clock is connected to
> adcmclk and dacmclk, so I can't support playback at 48KHz and capture at
> 44.1KHz.  However, there's nothing stopping a customer from creating a board
> that has two independent clocks.
> 
> So in the meantime, I'd really just like the ability to specify in the codec
> driver's .startup function exactly which sample rates are supported (based on
> the current mclk).
> 

You can already do this. Though you'd limit your CODEC driver to the use-case
where only one fixed sysclk is used. This might be a problem if other people
were using the same CODEC and want to use it without this limitation.

>> Machine drivers are currently best placed to set constraints if the
>> clocking is limited.
>
> But how is the machine driver supposed to know what those sample rates are? >
It would need to know how which dividers that codec uses.  That would mean
> that the machine driver has to be hard-codec with information on the
> internals of the codec.

How do you know which sysclk rate to select for a given sample rate, if you
don't know the samples rates the CODEC can produces for a given sysclk rate?

So ideally the CODEC driver would have callback which you could pass a set of
sysclk ranges the machine driver can supply and the CODEC driver would return a
list of sample rates it can support for this sysclk rate range. And then you
could use this updated list in your machine driver to decide which sysclk rate
to select. But I'm not sure if this isn't to over-engineered and hard-coding
this table into the machine driver wouldn't be better.

Re: [PATCH] [v2] ASoC: support all possible sample rates in the WM8776 driver

[Timur Tabi]

Lars-Peter Clausen wrote:
> How do you know which sysclk rate to select for a given sample rate, if you
> don't know the samples rates the CODEC can produces for a given sysclk rate?

You're right that supporting this feature would require the machine driver to
know more about the internals of the codec.  But I still think we need a better
mechanism for the codec driver to tell ASoC which sample rates are actually
supported.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] [v2] ASoC: support all possible sample rates in the WM8776 driver

[Mark Brown]

On Fri, Sep 16, 2011 at 07:46:13PM +0200, Lars-Peter Clausen wrote:

> So ideally the CODEC driver would have callback which you could pass a set of
> sysclk ranges the machine driver can supply and the CODEC driver would return a
> list of sample rates it can support for this sysclk rate range. And then you
> could use this updated list in your machine driver to decide which sysclk rate
> to select. But I'm not sure if this isn't to over-engineered and hard-coding
> this table into the machine driver wouldn't be better.

Yes, that's basically the issue - we could do something about this but
the effort involved seems disproportionate to the value.

Re: [PATCH] [v2] ASoC: support all possible sample rates in the WM8776 driver

[Mark Brown]

On Fri, Sep 16, 2011 at 09:16:54AM -0500, Timur Tabi wrote:

> Although the WM8776 driver includes a .set_sysclk function, it was also
> hard-coding the list of supported sample rates.  We change the hard-coded
> list to a range within the capabilities of the WM8776 itself.

No, it doesn't - it just blindly stores the sample rate in the private
data.  There's no list at all there, the only checking that's done is
for the DAI ID.

Anyway, I've applied with a rewritten changelog.

Re: [PATCH] [v2] ASoC: support all possible sample rates in the WM8776 driver

[Timur Tabi]

Mark Brown wrote:
> No, it doesn't - it just blindly stores the sample rate in the private
> data.  There's no list at all there, the only checking that's done is
> for the DAI ID.

Wait, what?  This is the list I am talking about:

#define WM8776_RATES (SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 |\
		      SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_88200 |\
		      SNDRV_PCM_RATE_96000)

Is this not a list of sample rates that the driver says it supports?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] [v2] ASoC: support all possible sample rates in the WM8776 driver

[Mark Brown]

On Fri, Sep 16, 2011 at 11:47:05AM -0500, Timur Tabi wrote:
> Mark Brown wrote:
> > No, it doesn't - it just blindly stores the sample rate in the private
> > data.  There's no list at all there, the only checking that's done is
> > for the DAI ID.

> Wait, what?  This is the list I am talking about:

> #define WM8776_RATES (SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 |\
> 		      SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_88200 |\
> 		      SNDRV_PCM_RATE_96000)

> Is this not a list of sample rates that the driver says it supports?

It is.  It's not referenced at all in set_sysclk().

Re: [PATCH] [v2] ASoC: support all possible sample rates in the WM8776 driver

[Timur Tabi]

Mark Brown wrote:
>> > Is this not a list of sample rates that the driver says it supports?

> It is.  It's not referenced at all in set_sysclk().

But it is reference in ASoC.  That's the point I was trying to make:

"Although the WM8776 driver includes a .set_sysclk function, it was also
hard-coding the list of supported sample rates."

Here, I say that the WM8776 driver was hard-coding a list of supported sample
rates.  I don't understand how this is not true.

Unless you're saying that the "it" refers to the set_sysclk function, rather
than "the WM8776 driver".

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] [v2] ASoC: support all possible sample rates in the WM8776 driver

[Mark Brown]

On Fri, Sep 16, 2011 at 11:56:08AM -0500, Timur Tabi wrote:

> But it is reference in ASoC.  That's the point I was trying to make:

> "Although the WM8776 driver includes a .set_sysclk function, it was also
> hard-coding the list of supported sample rates."

Right, but the two things aren't connected.  As I've *repeatedly* said
the list of supported sample rates in the DAI should be the full set
that the device can support, unrelated to any clock setup.

> Unless you're saying that the "it" refers to the set_sysclk function, rather
> than "the WM8776 driver".

That is actually how I parsed it, though in any case there's still the
thing with dynamic sysclk configuration being unrelated to rate lists.

Re: [PATCH] [v2] ASoC: support all possible sample rates in the WM8776 driver

[Timur Tabi]

Mark Brown wrote:

>> "Although the WM8776 driver includes a .set_sysclk function, it was also
>> hard-coding the list of supported sample rates."
> 
> Right, but the two things aren't connected.  As I've *repeatedly* said
> the list of supported sample rates in the DAI should be the full set
> that the device can support, unrelated to any clock setup.

But I could hook up any frequency to audmclk and dacmclk.  What if I want
40000Hz as a sample rate?  I could then then set mclk to 15.36MHz.

I don't see how the codec driver can know what sample rates it supports until
*after* set_sysclk() is called (i.e. when the driver is told the value of mclk).

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] [v2] ASoC: support all possible sample rates in the WM8776 driver

[Mark Brown]

On Fri, Sep 16, 2011 at 01:25:08PM -0500, Timur Tabi wrote:
> Mark Brown wrote:

> > Right, but the two things aren't connected.  As I've *repeatedly* said
> > the list of supported sample rates in the DAI should be the full set
> > that the device can support, unrelated to any clock setup.

> But I could hook up any frequency to audmclk and dacmclk.  What if I want
> 40000Hz as a sample rate?  I could then then set mclk to 15.36MHz.

If the CODEC supports continuous rates it'll say so and nothing in the
CODEC driver will stop that being selected.

> I don't see how the codec driver can know what sample rates it supports until
> *after* set_sysclk() is called (i.e. when the driver is told the value of mclk).

The part will have been specified and qualified for a set of sample
rates.  These will be listed in the datasheet and are the rates the
driver should advertise.