[2/4] ASoC: wm8940: Rewrite code to set proper clocks
Commit Message
Without this change, the wm8940 driver is not working when
set_sysclk callback (wm8940_set_dai_sysclk) is called with
frequency not listed in the switch clause.
This change adjusts this driver to allow non-standard frequency
set (just after the boot) being adjusted afterwards by the sound
system core code.
Moreover, support for internal wm8940's PLL is provided, so it
can generate clocks when HOST system is not able to do it.
Code in this commit is based on previous change done for wm8974
(SHA1: 51b2bb3f2568e6d9d81a001d38b8d70c2ba4af99).
Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
sound/soc/codecs/wm8940.c | 103 ++++++++++++++++++++++++++++++--------
1 file changed, 83 insertions(+), 20 deletions(-)
Comments
On Wed, Dec 14, 2022 at 01:37:41PM +0100, Lukasz Majewski wrote:
> Without this change, the wm8940 driver is not working when
> set_sysclk callback (wm8940_set_dai_sysclk) is called with
> frequency not listed in the switch clause.
>
> This change adjusts this driver to allow non-standard frequency
> set (just after the boot) being adjusted afterwards by the sound
> system core code.
>
> Moreover, support for internal wm8940's PLL is provided, so it
> can generate clocks when HOST system is not able to do it.
>
> Code in this commit is based on previous change done for wm8974
> (SHA1: 51b2bb3f2568e6d9d81a001d38b8d70c2ba4af99).
>
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> ---
> struct snd_soc_component *component = dai->component;
> + struct wm8940_priv *priv = snd_soc_component_get_drvdata(component);
> u16 iface = snd_soc_component_read(component, WM8940_IFACE) & 0xFD9F;
> u16 addcntrl = snd_soc_component_read(component, WM8940_ADDCNTRL) & 0xFFF1;
> u16 companding = snd_soc_component_read(component,
> WM8940_COMPANDINGCTL) & 0xFFDF;
> int ret;
>
> + priv->fs = params_rate(params);
> + ret = wm8940_update_clocks(dai);
> + if (ret)
> + return ret;
> +
I think this all looks mostly good, my only slight concern would
be the interaction with the manual functions for settings the PLL
etc. I guess under this code, whatever manual settings were
configured will be overwritten with the new auto settings, I
think this should be fine as the PLL wants to be run in a pretty
narrow band anyway, so the settings are likely identical. Do you
have any thoughts?
Thanks,
Charles
On Wed, Dec 14, 2022 at 01:37:41PM +0100, Lukasz Majewski wrote:
> Without this change, the wm8940 driver is not working when
> set_sysclk callback (wm8940_set_dai_sysclk) is called with
> frequency not listed in the switch clause.
Your commit log doesn't actually describe what this change is...
> This change adjusts this driver to allow non-standard frequency
> set (just after the boot) being adjusted afterwards by the sound
> system core code.
This doesn't appear to correspond to the commit. The change will result
in the driver automatically configuring it's PLL.
> Code in this commit is based on previous change done for wm8974
> (SHA1: 51b2bb3f2568e6d9d81a001d38b8d70c2ba4af99).
Please include human readable descriptions of things like commits and
issues being discussed in e-mail in your mails, this makes them much
easier for humans to read especially when they have no internet access.
I do frequently catch up on my mail on flights or while otherwise
travelling so this is even more pressing for me than just being about
making things a bit easier to read.
> @@ -496,7 +505,6 @@ static int wm8940_set_bias_level(struct snd_soc_component *component,
> return ret;
> }
> }
> -
> /* ensure bufioen and biasen */
> pwr_reg |= (1 << 2) | (1 << 3);
> /* set vmid to 300k for standby */
Unrelated (and questionable) whitespace change.
Hi Charles,
> On Wed, Dec 14, 2022 at 01:37:41PM +0100, Lukasz Majewski wrote:
> > Without this change, the wm8940 driver is not working when
> > set_sysclk callback (wm8940_set_dai_sysclk) is called with
> > frequency not listed in the switch clause.
> >
> > This change adjusts this driver to allow non-standard frequency
> > set (just after the boot) being adjusted afterwards by the sound
> > system core code.
> >
> > Moreover, support for internal wm8940's PLL is provided, so it
> > can generate clocks when HOST system is not able to do it.
> >
> > Code in this commit is based on previous change done for wm8974
> > (SHA1: 51b2bb3f2568e6d9d81a001d38b8d70c2ba4af99).
> >
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > ---
> > struct snd_soc_component *component = dai->component;
> > + struct wm8940_priv *priv =
> > snd_soc_component_get_drvdata(component); u16 iface =
> > snd_soc_component_read(component, WM8940_IFACE) & 0xFD9F; u16
> > addcntrl = snd_soc_component_read(component, WM8940_ADDCNTRL) &
> > 0xFFF1; u16 companding = snd_soc_component_read(component,
> > WM8940_COMPANDINGCTL) & 0xFFDF; int ret;
> >
> > + priv->fs = params_rate(params);
> > + ret = wm8940_update_clocks(dai);
> > + if (ret)
> > + return ret;
> > +
>
> I think this all looks mostly good, my only slight concern would
> be the interaction with the manual functions for settings the PLL
> etc. I guess under this code, whatever manual settings were
> configured
At least on my system - those settings are not set manually. Everythig
is done in the kernel.
This is important, as I do may use several other wm89* codecs, which
drivers are inserted as modules.
> will be overwritten with the new auto settings, I
> think this should be fine as the PLL wants to be run in a pretty
> narrow band anyway, so the settings are likely identical. Do you
> have any thoughts?
This code just follows changes done for WM8974 codec. I would leave the
code as it is in this patch.
>
> Thanks,
> Charles
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Mark,
> On Wed, Dec 14, 2022 at 01:37:41PM +0100, Lukasz Majewski wrote:
>
> > Without this change, the wm8940 driver is not working when
> > set_sysclk callback (wm8940_set_dai_sysclk) is called with
> > frequency not listed in the switch clause.
>
> Your commit log doesn't actually describe what this change is...
>
> > This change adjusts this driver to allow non-standard frequency
> > set (just after the boot) being adjusted afterwards by the sound
> > system core code.
>
> This doesn't appear to correspond to the commit. The change will
> result in the driver automatically configuring it's PLL.
>
Yes, indeed - I will update the description.
The problem with the old code was that after boot up - the frequency
was not in the 'switch' values, so the driver aborted in early init.
> > Code in this commit is based on previous change done for wm8974
> > (SHA1: 51b2bb3f2568e6d9d81a001d38b8d70c2ba4af99).
>
> Please include human readable descriptions of things like commits and
> issues being discussed in e-mail in your mails, this makes them much
> easier for humans to read especially when they have no internet
> access. I do frequently catch up on my mail on flights or while
> otherwise travelling so this is even more pressing for me than just
> being about making things a bit easier to read.
Ok. I will provide proper description.
>
> > @@ -496,7 +505,6 @@ static int wm8940_set_bias_level(struct
> > snd_soc_component *component, return ret;
> > }
> > }
> > -
> > /* ensure bufioen and biasen */
> > pwr_reg |= (1 << 2) | (1 << 3);
> > /* set vmid to 300k for standby */
>
> Unrelated (and questionable) whitespace change.
Ok. I will remove it.
Thanks for the review.
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
@@ -37,7 +37,9 @@
#include "wm8940.h"
struct wm8940_priv {
- unsigned int sysclk;
+ unsigned int mclk;
+ unsigned int fs;
+
struct regmap *regmap;
};
@@ -387,17 +389,24 @@ static int wm8940_set_dai_fmt(struct snd_soc_dai *codec_dai,
return 0;
}
+static int wm8940_update_clocks(struct snd_soc_dai *dai);
static int wm8940_i2s_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params,
struct snd_soc_dai *dai)
{
struct snd_soc_component *component = dai->component;
+ struct wm8940_priv *priv = snd_soc_component_get_drvdata(component);
u16 iface = snd_soc_component_read(component, WM8940_IFACE) & 0xFD9F;
u16 addcntrl = snd_soc_component_read(component, WM8940_ADDCNTRL) & 0xFFF1;
u16 companding = snd_soc_component_read(component,
WM8940_COMPANDINGCTL) & 0xFFDF;
int ret;
+ priv->fs = params_rate(params);
+ ret = wm8940_update_clocks(dai);
+ if (ret)
+ return ret;
+
/* LoutR control */
if (substream->stream == SNDRV_PCM_STREAM_CAPTURE
&& params_channels(params) == 2)
@@ -496,7 +505,6 @@ static int wm8940_set_bias_level(struct snd_soc_component *component,
return ret;
}
}
-
/* ensure bufioen and biasen */
pwr_reg |= (1 << 2) | (1 << 3);
/* set vmid to 300k for standby */
@@ -611,24 +619,6 @@ static int wm8940_set_dai_pll(struct snd_soc_dai *codec_dai, int pll_id,
return 0;
}
-static int wm8940_set_dai_sysclk(struct snd_soc_dai *codec_dai,
- int clk_id, unsigned int freq, int dir)
-{
- struct snd_soc_component *component = codec_dai->component;
- struct wm8940_priv *wm8940 = snd_soc_component_get_drvdata(component);
-
- switch (freq) {
- case 11289600:
- case 12000000:
- case 12288000:
- case 16934400:
- case 18432000:
- wm8940->sysclk = freq;
- return 0;
- }
- return -EINVAL;
-}
-
static int wm8940_set_dai_clkdiv(struct snd_soc_dai *codec_dai,
int div_id, int div)
{
@@ -653,6 +643,79 @@ static int wm8940_set_dai_clkdiv(struct snd_soc_dai *codec_dai,
return ret;
}
+static unsigned int wm8940_get_mclkdiv(unsigned int f_in, unsigned int f_out,
+ int *mclkdiv)
+{
+ unsigned int ratio = 2 * f_in / f_out;
+
+ if (ratio <= 2) {
+ *mclkdiv = WM8940_MCLKDIV_1;
+ ratio = 2;
+ } else if (ratio == 3) {
+ *mclkdiv = WM8940_MCLKDIV_1_5;
+ } else if (ratio == 4) {
+ *mclkdiv = WM8940_MCLKDIV_2;
+ } else if (ratio <= 6) {
+ *mclkdiv = WM8940_MCLKDIV_3;
+ ratio = 6;
+ } else if (ratio <= 8) {
+ *mclkdiv = WM8940_MCLKDIV_4;
+ ratio = 8;
+ } else if (ratio <= 12) {
+ *mclkdiv = WM8940_MCLKDIV_6;
+ ratio = 12;
+ } else if (ratio <= 16) {
+ *mclkdiv = WM8940_MCLKDIV_8;
+ ratio = 16;
+ } else {
+ *mclkdiv = WM8940_MCLKDIV_12;
+ ratio = 24;
+ }
+
+ return f_out * ratio / 2;
+}
+
+static int wm8940_update_clocks(struct snd_soc_dai *dai)
+{
+ struct snd_soc_component *codec = dai->component;
+ struct wm8940_priv *priv = snd_soc_component_get_drvdata(codec);
+ unsigned int fs256;
+ unsigned int fpll = 0;
+ unsigned int f;
+ int mclkdiv;
+
+ if (!priv->mclk || !priv->fs)
+ return 0;
+
+ fs256 = 256 * priv->fs;
+
+ f = wm8940_get_mclkdiv(priv->mclk, fs256, &mclkdiv);
+ if (f != priv->mclk) {
+ /* The PLL performs best around 90MHz */
+ fpll = wm8940_get_mclkdiv(22500000, fs256, &mclkdiv);
+ }
+
+ wm8940_set_dai_pll(dai, 0, 0, priv->mclk, fpll);
+ wm8940_set_dai_clkdiv(dai, WM8940_MCLKDIV, mclkdiv);
+
+ return 0;
+}
+
+static int wm8940_set_dai_sysclk(struct snd_soc_dai *dai, int clk_id,
+ unsigned int freq, int dir)
+{
+ struct snd_soc_component *codec = dai->component;
+ struct wm8940_priv *priv = snd_soc_component_get_drvdata(codec);
+
+ if (dir != SND_SOC_CLOCK_IN)
+ return -EINVAL;
+
+ priv->mclk = freq;
+
+ return wm8940_update_clocks(dai);
+}
+
+
#define WM8940_RATES SNDRV_PCM_RATE_8000_48000
#define WM8940_FORMATS (SNDRV_PCM_FMTBIT_S8 | \