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.

On Wed, Dec 14, 2022 at 01:37:42PM +0100, Lukasz Majewski wrote:

Without this change the BTL speaker produces some "distortion" noise when test program (speaker-test -t waw) is ended with ctrl+c.

As our design uses speaker outputs to drive BTL speaker, it was necessary to also mute the speaker via the codec internal WM8940_SPKVOL register with setting WM8940_SPKMUTE bit.

@@ -465,9 +465,18 @@ static int wm8940_mute(struct snd_soc_dai *dai, int mute, int direction) {

• spkvol_reg &= ~WM8940_SPKMUTE;

• if (mute) { mute_reg |= 0x40;

• spkvol_reg |= WM8940_SPKMUTE;
  

• }

• ret = snd_soc_component_write(component, WM8940_SPKVOL, spkvol_reg);

• if (ret)

• return ret;
  
  

  return snd_soc_component_write(component, WM8940_DAC, mute_reg);

In addition to the issue Charles raised this is simply not what the mute callback should do, the mute callback should specifically mute the digital input (with the goal of masking any glitching on there while clocks are started/stopped). Looking at the driver the device supports analogue bypass paths to the speaker - these will be broken by your patch so if you genuinely need some workaround in this area I'd be looking at the Speaker Mixer PCM Playback Switch rather than muting the speaker as a whole. If the device just can't cope without an input then ignore_mdown_time might be what you're looking for, it looks like the device doesn't have any lengthy sleeps in the power up/down paths so that should be fine so long as it doesn't pop/click.

I'd also check there's not some other system configuration issue here which is more obvious when the input from the DAC stops getting input, check that you don't see similar issues when silence is played for example. It might be worth checking that none of the analogue bypass paths are enabled.

Hi Mark,

Hi Mark,

...

On Wed, Dec 14, 2022 at 01:37:42PM +0100, Lukasz Majewski wrote:

...

Without this change the BTL speaker produces some "distortion" noise when test program (speaker-test -t waw) is ended with ctrl+c.

...

As our design uses speaker outputs to drive BTL speaker, it was necessary to also mute the speaker via the codec internal WM8940_SPKVOL register with setting WM8940_SPKMUTE bit.

...

@@ -465,9 +465,18 @@ static int wm8940_mute(struct snd_soc_dai *dai, int mute, int direction) {

...

• spkvol_reg &= ~WM8940_SPKMUTE;
• if (mute) { mute_reg |= 0x40;

• spkvol_reg |= WM8940_SPKMUTE;
  

• }
• ret = snd_soc_component_write(component, WM8940_SPKVOL,

spkvol_reg);

• if (ret)

• return ret;
  
  

  return snd_soc_component_write(component, WM8940_DAC,

mute_reg);

In addition to the issue Charles raised this is simply not what the mute callback should do, the mute callback should specifically mute the digital input (with the goal of masking any glitching on there while clocks are started/stopped).

Ok

...

Looking at the driver the device supports analogue bypass paths to the speaker - these will be broken by your patch

I was not aware about this side effect. I just wanted to be sure that the speaker is muted.

...

so if you genuinely need some workaround in this area I'd be looking at the Speaker Mixer PCM Playback Switch rather than muting the speaker as a whole.

I would be more than happy if I could use for example the 'amixer' command to setup the audio correctly without this patch.

For example - on this system - before I run any speaker test I need to call: amixer -d set 'Speaker Mixer PCM',0 on

to unmute the system.

This patch steamed from 4.4 Linux. On the newer Linux version the "distortion" problem is not present anymore.

I will drop this patch.

...

If the device just can't cope without an input then ignore_mdown_time might be what you're looking for, it looks like the device doesn't have any lengthy sleeps in the power up/down paths so that should be fine so long as it doesn't pop/click.

Ok. I will check this as well.

...

I'd also check there's not some other system configuration issue here which is more obvious when the input from the DAC stops getting input, check that you don't see similar issues when silence is played for example. It might be worth checking that none of the analogue bypass paths are enabled.

Thanks for your hints. I will investigate it further.

It looks like this patch is some kind of a hack, to fix my system configuration and shall be dropped in v2.

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

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

This patch enables support for internal wm8940's PLL and proper divider to set proper value for 256x fs clock.

This approach is more flexible and replaces hardcoded clock values and makes the codec work with the simple-card driver. Card drivers calling set_pll() and set_clkdiv() directly are unaffected.

For the reference - code in this commit is based on: "ASoC: wm8974: configure pll and mclk divider automatically" (SHA1: 51b2bb3f2568e6d9d81a001d38b8d70c2ba4af99).

Signed-off-by: Lukasz Majewski lukma@denx.de --- Changes for v2: - Remove not required line add/remove - Rewrite the commit message to describe more precisely the problem --- sound/soc/codecs/wm8940.c | 101 +++++++++++++++++++++++++++++++------- 1 file changed, 82 insertions(+), 19 deletions(-)

diff --git a/sound/soc/codecs/wm8940.c b/sound/soc/codecs/wm8940.c index 0b59020d747f..1b02d5e8a007 100644 --- a/sound/soc/codecs/wm8940.c +++ b/sound/soc/codecs/wm8940.c @@ -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) @@ -611,24 +620,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 +644,78 @@ 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 | \

The wm8940 provides the chip ID information via I2C. In this patch this information is read and if not matching expected value, the probe function is aborted.

This prevents from using (i.e. inserting) other wm89* modules which use the same I2C bus address.

Signed-off-by: Lukasz Majewski lukma@denx.de Acked-by: Charles Keepax ckeepax@opensource.cirrus.com --- Changes for v2: - None --- sound/soc/codecs/wm8940.c | 11 +++++++++++ sound/soc/codecs/wm8940.h | 3 +++ 2 files changed, 14 insertions(+)

diff --git a/sound/soc/codecs/wm8940.c b/sound/soc/codecs/wm8940.c index 1b02d5e8a007..8eb4782c9232 100644 --- a/sound/soc/codecs/wm8940.c +++ b/sound/soc/codecs/wm8940.c @@ -760,6 +760,17 @@ static int wm8940_probe(struct snd_soc_component *component) int ret; u16 reg;

+ /* + * Check chip ID for wm8940 - value of 0x00 offset + * SOFTWARE_RESET on write + * CHIP_ID on read + */ + reg = snd_soc_component_read(component, WM8940_SOFTRESET); + if (reg != WM8940_CHIP_ID) { + dev_err(component->dev, "Wrong wm8940 chip ID: 0x%x\n", reg); + return -ENODEV; + } + ret = wm8940_reset(component); if (ret < 0) { dev_err(component->dev, "Failed to issue reset\n"); diff --git a/sound/soc/codecs/wm8940.h b/sound/soc/codecs/wm8940.h index 0d4f53ada2e6..86bbf902ef5a 100644 --- a/sound/soc/codecs/wm8940.h +++ b/sound/soc/codecs/wm8940.h @@ -95,5 +95,8 @@ struct wm8940_setup_data { #define WM8940_OPCLKDIV_3 2 #define WM8940_OPCLKDIV_4 3

+/* Chip ID */ +#define WM8940_CHIP_ID 0x8940 + #endif /* _WM8940_H */