[alsa-devel] [PATCH] ASoC: arizona: Check clocking during hw_params rather than startup
It is a fairly common usage to configure SYSCLK as part of the hw_params callback in the machine driver. The current driver applies PCM constraints based on the value of SYSCLK during the startup callback however because many systems will have not configured SYSCLK yet this will often use the SYSCLK value that was used for the last stream.
The patch checks the clocking constraints as part of the hw_params callback, which will be after any clocking changes have been made by the machine driver.
Change-Id: I48e16256e9ed4df2f4ef1350c356b8b331d288bf Signed-off-by: Charles Keepax ckeepax@opensource.wolfsonmicro.com --- sound/soc/codecs/arizona.c | 79 +++++++------------------------------------ 1 files changed, 13 insertions(+), 66 deletions(-)
diff --git a/sound/soc/codecs/arizona.c b/sound/soc/codecs/arizona.c index e4295fe..ae5031b 100644 --- a/sound/soc/codecs/arizona.c +++ b/sound/soc/codecs/arizona.c @@ -990,29 +990,6 @@ static const int arizona_48k_bclk_rates[] = { 24576000, };
-static const unsigned int arizona_48k_rates[] = { - 12000, - 24000, - 48000, - 96000, - 192000, - 384000, - 768000, - 4000, - 8000, - 16000, - 32000, - 64000, - 128000, - 256000, - 512000, -}; - -static const struct snd_pcm_hw_constraint_list arizona_48k_constraint = { - .count = ARRAY_SIZE(arizona_48k_rates), - .list = arizona_48k_rates, -}; - static const int arizona_44k1_bclk_rates[] = { -1, 44100, @@ -1035,21 +1012,6 @@ static const int arizona_44k1_bclk_rates[] = { 22579200, };
-static const unsigned int arizona_44k1_rates[] = { - 11025, - 22050, - 44100, - 88200, - 176400, - 352800, - 705600, -}; - -static const struct snd_pcm_hw_constraint_list arizona_44k1_constraint = { - .count = ARRAY_SIZE(arizona_44k1_rates), - .list = arizona_44k1_rates, -}; - static int arizona_sr_vals[] = { 0, 12000, @@ -1077,13 +1039,15 @@ static int arizona_sr_vals[] = { 512000, };
-static int arizona_startup(struct snd_pcm_substream *substream, - struct snd_soc_dai *dai) +static int arizona_hw_params_rate(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params, + struct snd_soc_dai *dai) { struct snd_soc_codec *codec = dai->codec; struct arizona_priv *priv = snd_soc_codec_get_drvdata(codec); struct arizona_dai_priv *dai_priv = &priv->dai[dai->id - 1]; - const struct snd_pcm_hw_constraint_list *constraint; + int base = dai->driver->base; + int i, sr_val; unsigned int base_rate;
switch (dai_priv->clk) { @@ -1094,31 +1058,16 @@ static int arizona_startup(struct snd_pcm_substream *substream, base_rate = priv->asyncclk; break; default: - return 0; + arizona_aif_err(dai, "Unknown clock: %d\n", dai_priv->clk); + return -EINVAL; }
- if (base_rate == 0) - return 0; - - if (base_rate % 8000) - constraint = &arizona_44k1_constraint; - else - constraint = &arizona_48k_constraint; - - return snd_pcm_hw_constraint_list(substream->runtime, 0, - SNDRV_PCM_HW_PARAM_RATE, - constraint); -} - -static int arizona_hw_params_rate(struct snd_pcm_substream *substream, - struct snd_pcm_hw_params *params, - struct snd_soc_dai *dai) -{ - struct snd_soc_codec *codec = dai->codec; - struct arizona_priv *priv = snd_soc_codec_get_drvdata(codec); - struct arizona_dai_priv *dai_priv = &priv->dai[dai->id - 1]; - int base = dai->driver->base; - int i, sr_val; + if (!!(base_rate % 8000) != !!(params_rate(params) % 8000)) { + arizona_aif_err(dai, + "Rate %dHz not supported off %dHz clock\n", + params_rate(params), base_rate); + return -EINVAL; + }
/* * We will need to be more flexible than this in future, @@ -1308,7 +1257,6 @@ static int arizona_set_tristate(struct snd_soc_dai *dai, int tristate) }
const struct snd_soc_dai_ops arizona_dai_ops = { - .startup = arizona_startup, .set_fmt = arizona_set_fmt, .hw_params = arizona_hw_params, .set_sysclk = arizona_dai_set_sysclk, @@ -1317,7 +1265,6 @@ const struct snd_soc_dai_ops arizona_dai_ops = { EXPORT_SYMBOL_GPL(arizona_dai_ops);
const struct snd_soc_dai_ops arizona_simple_dai_ops = { - .startup = arizona_startup, .hw_params = arizona_hw_params_rate, .set_sysclk = arizona_dai_set_sysclk, };
On 01/24/2014 05:59 PM, Charles Keepax wrote:
It is a fairly common usage to configure SYSCLK as part of the hw_params callback in the machine driver. The current driver applies PCM constraints based on the value of SYSCLK during the startup callback however because many systems will have not configured SYSCLK yet this will often use the SYSCLK value that was used for the last stream.
The patch checks the clocking constraints as part of the hw_params callback, which will be after any clocking changes have been made by the machine driver.
The problem with this approach is that if the sysclk is fixed instead to e.g. falling back to rate conversion you'll get an error instead when calling hw_params. The typical idiom here is to only apply the rate constraints in the CODEC driver if the sysclk is non zero. This driver already seems to do this. The problem that you probably see is that if the machine driver changes the sysclk value in hw_params the constraints that are setup are still setup with the previous sysclk in mind. I think instead of removing the constraints support altogether from the driver a better solution is to introduce a dynamic_sysclk or similar attribute to the DAI link and not constrain the supported rates if this attribute is set to true.
- Lars
On Fri, Jan 24, 2014 at 06:25:06PM +0100, Lars-Peter Clausen wrote:
The problem with this approach is that if the sysclk is fixed instead to e.g. falling back to rate conversion you'll get an error instead when calling hw_params. The typical idiom here is to only apply the rate constraints in the CODEC driver if the sysclk is non zero. This driver already seems to do this. The problem that you probably see is that if the machine driver changes the sysclk value in hw_params the constraints that
Thanks, that's about what I was going to write. The current theory is that setting the sysclk to zero is the equivalent of a dynamic SYSCLK flag - with the extensive use of charge pumps and so on in modern devices on the fly reclocking is normally difficult to do safely so the idea is that if the machine driver is in a position to reclock it should set the clock to zero.
are setup are still setup with the previous sysclk in mind. I think instead of removing the constraints support altogether from the driver a better solution is to introduce a dynamic_sysclk or similar attribute to the DAI link and not constrain the supported rates if this attribute is set to true.
It would be useful to have this more in the core rather than open coded, as with the sample size stuff.
On 01/24/2014 06:37 PM, Mark Brown wrote:
On Fri, Jan 24, 2014 at 06:25:06PM +0100, Lars-Peter Clausen wrote:
The problem with this approach is that if the sysclk is fixed instead to e.g. falling back to rate conversion you'll get an error instead when calling hw_params. The typical idiom here is to only apply the rate constraints in the CODEC driver if the sysclk is non zero. This driver already seems to do this. The problem that you probably see is that if the machine driver changes the sysclk value in hw_params the constraints that
Thanks, that's about what I was going to write. The current theory is that setting the sysclk to zero is the equivalent of a dynamic SYSCLK flag - with the extensive use of charge pumps and so on in modern devices on the fly reclocking is normally difficult to do safely so the idea is that if the machine driver is in a position to reclock it should set the clock to zero.
It's a bit ugly though to set the clock to 0 in the startup callback of the machine driver and then set it to the actual sysclk in the hw_params callback.
are setup are still setup with the previous sysclk in mind. I think instead of removing the constraints support altogether from the driver a better solution is to introduce a dynamic_sysclk or similar attribute to the DAI link and not constrain the supported rates if this attribute is set to true.
It would be useful to have this more in the core rather than open coded, as with the sample size stuff.
Agreed. It probably fits in nicely with the sample size stuff. Set constraints for the sysclk in the machine driver (i.e. the list of possible values). And then have the CODEC driver figure out which sample rates it can support based on the sysclk constraints and use these has the sample rate constraints.
- Lars
On Mon, Jan 27, 2014 at 08:17:18AM +0100, Lars-Peter Clausen wrote:
On 01/24/2014 06:37 PM, Mark Brown wrote:
Thanks, that's about what I was going to write. The current theory is that setting the sysclk to zero is the equivalent of a dynamic SYSCLK flag - with the extensive use of charge pumps and so on in modern devices on the fly reclocking is normally difficult to do safely so the idea is that if the machine driver is in a position to reclock it should set the clock to zero.
It's a bit ugly though to set the clock to 0 in the startup callback of the machine driver and then set it to the actual sysclk in the hw_params callback.
If something is doing this I'd expect it to set the clock to zero when it is idled rather than during startup() - set_bias_level() is usually a good place to do this, or possibly a DAPM widget supplying the clocks in the device if the clocks are visible in DAPM. That's a bit nicer and doing it on startup runs into issues with things like bypass paths anyway.
On Mon, Jan 27, 2014 at 10:55:08AM +0000, Mark Brown wrote:
On Mon, Jan 27, 2014 at 08:17:18AM +0100, Lars-Peter Clausen wrote:
On 01/24/2014 06:37 PM, Mark Brown wrote:
Thanks, that's about what I was going to write. The current theory is that setting the sysclk to zero is the equivalent of a dynamic SYSCLK flag - with the extensive use of charge pumps and so on in modern devices on the fly reclocking is normally difficult to do safely so the idea is that if the machine driver is in a position to reclock it should set the clock to zero.
It's a bit ugly though to set the clock to 0 in the startup callback of the machine driver and then set it to the actual sysclk in the hw_params callback.
If something is doing this I'd expect it to set the clock to zero when it is idled rather than during startup() - set_bias_level() is usually a good place to do this, or possibly a DAPM widget supplying the clocks in the device if the clocks are visible in DAPM. That's a bit nicer and doing it on startup runs into issues with things like bypass paths anyway.
Yeah I think the existing support is actually likely sufficent looks likely this was actually an issue with the machine driver in question, apologies for the noise.
Thanks, Charles
participants (3)
-
Charles Keepax -
Lars-Peter Clausen -
Mark Brown