[alsa-devel] [PATCH] [v2] ASoC: support all possible sample rates in the WM8776 driver
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,
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,@@ -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,-- 1.7.3.4
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
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.
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.
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.
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).
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.
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.
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.
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).
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.
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.
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.
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.
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?
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().
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".
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.
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).
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.
participants (4)
-
Lars-Peter Clausen -
Mark Brown -
Takashi Iwai -
Timur Tabi