Re: [PATCH 2/4] ASoC: wm8904: Automatically enable FLL when selected

[Charles Keepax <ckeepax@opensource.cirrus.com>]

On Wed, Dec 19, 2018 at 09:11:15PM +0100, Michał Mirosław wrote:
> This makes FLL the clock used from audio-graph-card platform driver
> (it explicitly uses clock id 0).  Other platform drivers select the
> clock manually.
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---

I wonder a little if this part is really suitable for use with
simple card.

>  sound/soc/codecs/wm8904.c | 21 ++++++++++++++++++---
>  sound/soc/codecs/wm8904.h |  2 +-
>  2 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/codecs/wm8904.c b/sound/soc/codecs/wm8904.c
> index 2a3e5fbd04e4..f8a17fcdfdeb 100644
> --- a/sound/soc/codecs/wm8904.c
> +++ b/sound/soc/codecs/wm8904.c
> @@ -315,6 +315,9 @@ static bool wm8904_readable_register(struct device *dev, unsigned int reg)
>  	}
>  }
>  
> +static int wm8904_set_fll(struct snd_soc_component *component, int fll_id, int source,
> +			  unsigned int Fref, unsigned int Fout);
> +
>  static int wm8904_configure_clocking(struct snd_soc_component *component)
>  {
>  	struct wm8904_priv *wm8904 = snd_soc_component_get_drvdata(component);
> @@ -339,6 +342,13 @@ static int wm8904_configure_clocking(struct snd_soc_component *component)
>  		break;
>  
>  	case WM8904_CLK_FLL:
> +		if (!wm8904->fll_fout) {
> +			int ret = wm8904_set_fll(component, WM8904_FLL_MCLK, WM8904_FLL_MCLK,
> +						 clk_get_rate(wm8904->mclk), 12288000);
> +			if (ret)
> +				return ret;
> +		}

What is your thinking on selecting a 12.28MHz clock? Will this
not cause issues with say 44.1k playback?

> +
>  		dev_dbg(component->dev, "Using %dHz FLL clock\n",
>  			wm8904->fll_fout);
>  
> @@ -1675,10 +1685,9 @@ static int fll_factors(struct _fll_div *fll_div, unsigned int Fref,
>  	return 0;
>  }
>  
> -static int wm8904_set_fll(struct snd_soc_dai *dai, int fll_id, int source,
> +static int wm8904_set_fll(struct snd_soc_component *component, int fll_id, int source,
>  			  unsigned int Fref, unsigned int Fout)
>  {
> -	struct snd_soc_component *component = dai->component;
>  	struct wm8904_priv *wm8904 = snd_soc_component_get_drvdata(component);
>  	struct _fll_div fll_div;
>  	int ret, val;
> @@ -1814,6 +1823,12 @@ static int wm8904_set_fll(struct snd_soc_dai *dai, int fll_id, int source,
>  	return 0;
>  }
>  
> +static int wm8904_set_dai_fll(struct snd_soc_dai *dai, int fll_id, int source,
> +			      unsigned int Fref, unsigned int Fout)
> +{
> +	return wm8904_set_fll(dai->component, fll_id, source, Fref, Fout);
> +}
> +
>  static int wm8904_digital_mute(struct snd_soc_dai *codec_dai, int mute)
>  {
>  	struct snd_soc_component *component = codec_dai->component;
> @@ -1921,7 +1936,7 @@ static const struct snd_soc_dai_ops wm8904_dai_ops = {
>  	.set_sysclk = wm8904_set_sysclk,
>  	.set_fmt = wm8904_set_fmt,
>  	.set_tdm_slot = wm8904_set_tdm_slot,
> -	.set_pll = wm8904_set_fll,
> +	.set_pll = wm8904_set_dai_fll,
>  	.hw_params = wm8904_hw_params,
>  	.digital_mute = wm8904_digital_mute,
>  };
> diff --git a/sound/soc/codecs/wm8904.h b/sound/soc/codecs/wm8904.h
> index c29a0e8131ca..ed3260bcae62 100644
> --- a/sound/soc/codecs/wm8904.h
> +++ b/sound/soc/codecs/wm8904.h
> @@ -13,8 +13,8 @@
>  #ifndef _WM8904_H
>  #define _WM8904_H
>  
> +#define WM8904_CLK_FLL  0
>  #define WM8904_CLK_MCLK 1
> -#define WM8904_CLK_FLL  2

A little nervous about making CLK_FLL 0 that means that there is
no longer any concept of an undefined sysclk_src, perhaps we
should also initialise things in the driver to maintain that
concept.

>  
>  #define WM8904_FLL_MCLK          1
>  #define WM8904_FLL_BCLK          2
> -- 
> 2.19.2

Thanks,
Charles
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel