[PATCH] ASoC: wm8741: Remove unneeded startup() callback

[PATCH] ASoC: wm8741: Remove unneeded startup() callback

[Sergej Sawazki]

Do not apply rate constraints in the startup() callback. The machine driver
can change the sysclk and hence the supported frame rates in its hw_params().
This callback is unneeded since commit e369bd006fd6 ("ASoC: wm8741: Allow
master clock switching").

Signed-off-by: Sergej Sawazki <ce3a@gmx.de>
---
 sound/soc/codecs/wm8741.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/sound/soc/codecs/wm8741.c b/sound/soc/codecs/wm8741.c
index b8c1940..d6e540a 100644
--- a/sound/soc/codecs/wm8741.c
+++ b/sound/soc/codecs/wm8741.c
@@ -176,20 +176,6 @@ static const struct snd_pcm_hw_constraint_list constraints_36864 = {
 	.list	= rates_36864,
 };
 
-static int wm8741_startup(struct snd_pcm_substream *substream,
-			  struct snd_soc_dai *dai)
-{
-	struct snd_soc_codec *codec = dai->codec;
-	struct wm8741_priv *wm8741 = snd_soc_codec_get_drvdata(codec);
-
-	if (wm8741->sysclk)
-		snd_pcm_hw_constraint_list(substream->runtime, 0,
-				SNDRV_PCM_HW_PARAM_RATE,
-				wm8741->sysclk_constraints);
-
-	return 0;
-}
-
 static int wm8741_hw_params(struct snd_pcm_substream *substream,
 			    struct snd_pcm_hw_params *params,
 			    struct snd_soc_dai *dai)
@@ -360,7 +346,6 @@ static int wm8741_set_dai_fmt(struct snd_soc_dai *codec_dai,
 			SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE)
 
 static const struct snd_soc_dai_ops wm8741_dai_ops = {
-	.startup	= wm8741_startup,
 	.hw_params	= wm8741_hw_params,
 	.set_sysclk	= wm8741_set_dai_sysclk,
 	.set_fmt	= wm8741_set_dai_fmt,
-- 
2.7.4

Re: [PATCH] ASoC: wm8741: Remove unneeded startup() callback

[Charles Keepax]

On Sat, Jan 28, 2017 at 12:35:45AM +0100, Sergej Sawazki wrote:
> Do not apply rate constraints in the startup() callback. The machine driver
> can change the sysclk and hence the supported frame rates in its hw_params().
> This callback is unneeded since commit e369bd006fd6 ("ASoC: wm8741: Allow
> master clock switching").
> 
> Signed-off-by: Sergej Sawazki <ce3a@gmx.de>
> ---
>  sound/soc/codecs/wm8741.c | 15 ---------------
>  1 file changed, 15 deletions(-)
> 
> diff --git a/sound/soc/codecs/wm8741.c b/sound/soc/codecs/wm8741.c
> index b8c1940..d6e540a 100644
> --- a/sound/soc/codecs/wm8741.c
> +++ b/sound/soc/codecs/wm8741.c
> @@ -176,20 +176,6 @@ static const struct snd_pcm_hw_constraint_list constraints_36864 = {
>  	.list	= rates_36864,
>  };
>  
> -static int wm8741_startup(struct snd_pcm_substream *substream,
> -			  struct snd_soc_dai *dai)
> -{
> -	struct snd_soc_codec *codec = dai->codec;
> -	struct wm8741_priv *wm8741 = snd_soc_codec_get_drvdata(codec);
> -
> -	if (wm8741->sysclk)
> -		snd_pcm_hw_constraint_list(substream->runtime, 0,
> -				SNDRV_PCM_HW_PARAM_RATE,
> -				wm8741->sysclk_constraints);
> -
> -	return 0;
> -}
> -

This function should not be removed it is performing a useful
function. If a sysclk has already been configured by the machine
driver then we should inform user-space of the rates we can
support from that clock. Without this that information is not
available to user-space, so instead of user-space being able to
resample appropriately it would just error out from hw_params.

Are you perhaps missing a call to clear the sysclk from your
machine driver? Usually a dai_set_sysclk call with a rate of
zero, also I find if it is a machine driver supporting multiple
rates you are best to use ignore_pmdown_time on the DAI link,
assuming the devices don't have long bring up times.

Thanks,
Charles

Re: [PATCH] ASoC: wm8741: Remove unneeded startup() callback

[Sergej Sawazki]

On Mon, 30 Jan 2017 09:22:59 +0000, Charles Keepax wrote:
> On Sat, Jan 28, 2017 at 12:35:45AM +0100, Sergej Sawazki wrote:
>> Do not apply rate constraints in the startup() callback. The machine driver
>> can change the sysclk and hence the supported frame rates in its hw_params().
>> This callback is unneeded since commit e369bd006fd6 ("ASoC: wm8741: Allow
>> master clock switching").

> This function should not be removed it is performing a useful
> function. If a sysclk has already been configured by the machine
> driver then we should inform user-space of the rates we can
> support from that clock. Without this that information is not
> available to user-space, so instead of user-space being able to
> resample appropriately it would just error out from hw_params.
> 
> Are you perhaps missing a call to clear the sysclk from your
> machine driver? Usually a dai_set_sysclk call with a rate of
> zero, also I find if it is a machine driver supporting multiple
> rates you are best to use ignore_pmdown_time on the DAI link,
> assuming the devices don't have long bring up times.

After clearing the sysclk, the codec should 'pretend' to support all
rates, right? (no sysclk -> no rate constrains)

After applying rate constraints using snd_pcm_hw_constraint_list(...),
what would be a clean way to remove the constraints from the rates
parameter?

Thanks,
Sergej

Re: [PATCH] ASoC: wm8741: Remove unneeded startup() callback

[Charles Keepax]

On Mon, Jan 30, 2017 at 11:56:52PM +0100, Sergej Sawazki wrote:
> On Mon, 30 Jan 2017 09:22:59 +0000, Charles Keepax wrote:
> > On Sat, Jan 28, 2017 at 12:35:45AM +0100, Sergej Sawazki wrote:
> >> Do not apply rate constraints in the startup() callback. The machine driver
> >> can change the sysclk and hence the supported frame rates in its hw_params().
> >> This callback is unneeded since commit e369bd006fd6 ("ASoC: wm8741: Allow
> >> master clock switching").
> 
> > This function should not be removed it is performing a useful
> > function. If a sysclk has already been configured by the machine
> > driver then we should inform user-space of the rates we can
> > support from that clock. Without this that information is not
> > available to user-space, so instead of user-space being able to
> > resample appropriately it would just error out from hw_params.
> > 
> > Are you perhaps missing a call to clear the sysclk from your
> > machine driver? Usually a dai_set_sysclk call with a rate of
> > zero, also I find if it is a machine driver supporting multiple
> > rates you are best to use ignore_pmdown_time on the DAI link,
> > assuming the devices don't have long bring up times.
> 
> After clearing the sysclk, the codec should 'pretend' to support all
> rates, right? (no sysclk -> no rate constrains)
> 

Indeed yes, so a normal pattern would often be setting a sysclk
rate in set_bias_level(on the way up)/hw_params and clearing it in
set_bias_level(on the way down). That way whilst the device is
powered up it is fixed at a certain rate so as not to disrupt any
running audio but whilst powered down you are free to start it up
at any rate.

> After applying rate constraints using snd_pcm_hw_constraint_list(...),
> what would be a clean way to remove the constraints from the rates
> parameter?
> 

I am not sure I fully follow here, normally one would close the
stream and open a new one at the new rate. Hence no need to clear
the constraits as its a new stream. Are you intending to keep the
stream open but reconfigure it for new rates?

Thanks,
Charles

Re: [PATCH] ASoC: wm8741: Remove unneeded startup() callback

[Sergej Sawazki]

On Tue, 31 Jan 2017 09:49:31 +0000, Charles Keepax wrote:
> On Mon, Jan 30, 2017 at 11:56:52PM +0100, Sergej Sawazki wrote:

>> After clearing the sysclk, the codec should 'pretend' to support all
>> rates, right? (no sysclk -> no rate constrains)

> Indeed yes, so a normal pattern would often be setting a sysclk
> rate in set_bias_level(on the way up)/hw_params and clearing it in
> set_bias_level(on the way down). That way whilst the device is
> powered up it is fixed at a certain rate so as not to disrupt any
> running audio but whilst powered down you are free to start it up
> at any rate.

Thanks Charles, it is clear now, sorry for the noise.

Sergej