| Message ID | 5353df1d654eae0bbbf26bc1a1d172e611c385db.1545249666.git.mirq-linux@rere.qmqm.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: wm8904: prepare for use from audio-graph-card | expand |
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
On Fri, Dec 21, 2018 at 11:52:27AM +0000, Charles Keepax wrote: > On Wed, Dec 19, 2018 at 09:11:15PM +0100, Michał Mirosław wrote: > > + 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? The driver just shouldn't be making decisions like this at all, either generic code or a machine driver should do so.
On Fri, Dec 21, 2018 at 11:58:31AM +0000, Mark Brown wrote: > On Fri, Dec 21, 2018 at 11:52:27AM +0000, Charles Keepax wrote: > > On Wed, Dec 19, 2018 at 09:11:15PM +0100, Michał Mirosław wrote: > > > > + 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? > The driver just shouldn't be making decisions like this at all, either > generic code or a machine driver should do so. Sorry, I forgot about the hardcoded frequency. I'll have to rethink this patch. Will you take the other three patches as is, or should I resend the series? Best Regards, Michał Mirosław
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; + } + 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 #define WM8904_FLL_MCLK 1 #define WM8904_FLL_BCLK 2
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> --- sound/soc/codecs/wm8904.c | 21 ++++++++++++++++++--- sound/soc/codecs/wm8904.h | 2 +- 2 files changed, 19 insertions(+), 4 deletions(-)