[PATCH] ASoC: arizona: Check clocking during hw_params rather than startup

[PATCH] ASoC: arizona: Check clocking during hw_params rather than startup

[Charles Keepax]

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,
 };
-- 
1.7.2.5

Re: [PATCH] ASoC: arizona: Check clocking during hw_params rather than startup

[Lars-Peter Clausen]

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

Re: [PATCH] ASoC: arizona: Check clocking during hw_params rather than startup

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1301 bytes --]

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: arizona: Check clocking during hw_params rather than startup

[Lars-Peter Clausen]

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

Re: [PATCH] ASoC: arizona: Check clocking during hw_params rather than startup

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1035 bytes --]

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: arizona: Check clocking during hw_params rather than startup

[Charles Keepax]

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