[PATCH 0/4] ASoC: Fixes for WM8940 codec

[PATCH 0/4] ASoC: Fixes for WM8940 codec

[Lukasz Majewski]

This patch series provides fixes for WM8940 codec.

The most notable change is the clock rewrite, so this driver now
can either generate proper clock frequency by itself or use one
provided from the clock subsystem of the SoC.

Lukasz Majewski (4):
  ASoC: wm8940: Remove warning when no plat data present
  ASoC: wm8940: Rewrite code to set proper clocks
  ASoC: wm8940: Mute also the speaker output
  ASoC: wm8940: Read chip ID when wm8940 codec probing

 sound/soc/codecs/wm8940.c | 129 +++++++++++++++++++++++++++++++-------
 sound/soc/codecs/wm8940.h |   6 ++
 2 files changed, 111 insertions(+), 24 deletions(-)

-- 
2.20.1

[PATCH 1/4] ASoC: wm8940: Remove warning when no plat data present

[Lukasz Majewski]

The lack of platform data in the contemporary Linux
shall not be the reason to display warnings to the
kernel logs.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
 sound/soc/codecs/wm8940.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/sound/soc/codecs/wm8940.c b/sound/soc/codecs/wm8940.c
index 8dac9fd88547..0b59020d747f 100644
--- a/sound/soc/codecs/wm8940.c
+++ b/sound/soc/codecs/wm8940.c
@@ -709,9 +709,7 @@ static int wm8940_probe(struct snd_soc_component *component)
 	if (ret < 0)
 		return ret;
 
-	if (!pdata)
-		dev_warn(component->dev, "No platform data supplied\n");
-	else {
+	if (pdata) {
 		reg = snd_soc_component_read(component, WM8940_OUTPUTCTL);
 		ret = snd_soc_component_write(component, WM8940_OUTPUTCTL, reg | pdata->vroi);
 		if (ret < 0)
-- 
2.20.1

[PATCH 2/4] ASoC: wm8940: Rewrite code to set proper clocks

[Lukasz Majewski]

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(-)

diff --git a/sound/soc/codecs/wm8940.c b/sound/soc/codecs/wm8940.c
index 0b59020d747f..094e74905df9 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)
@@ -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 |				\
-- 
2.20.1

[PATCH 3/4] ASoC: wm8940: Mute also the speaker output

[Lukasz Majewski]

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.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
 sound/soc/codecs/wm8940.c | 11 ++++++++++-
 sound/soc/codecs/wm8940.h |  3 +++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/wm8940.c b/sound/soc/codecs/wm8940.c
index 094e74905df9..13cb57210b4b 100644
--- a/sound/soc/codecs/wm8940.c
+++ b/sound/soc/codecs/wm8940.c
@@ -465,9 +465,18 @@ static int wm8940_mute(struct snd_soc_dai *dai, int mute, int direction)
 {
 	struct snd_soc_component *component = dai->component;
 	u16 mute_reg = snd_soc_component_read(component, WM8940_DAC) & 0xffbf;
+	u16 spkvol_reg = snd_soc_component_read(component, WM8940_SPKVOL);
+	int ret;
 
-	if (mute)
+	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);
 }
diff --git a/sound/soc/codecs/wm8940.h b/sound/soc/codecs/wm8940.h
index 0d4f53ada2e6..eb051ed29bb8 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
 
+/* Bit definitions */
+#define WM8940_SPKMUTE BIT(6)
+
 #endif /* _WM8940_H */
 
-- 
2.20.1

[PATCH 4/4] ASoC: wm8940: Read chip ID when wm8940 codec probing

[Lukasz Majewski]

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 sam I2C bus address.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
 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 13cb57210b4b..244998ebab4d 100644
--- a/sound/soc/codecs/wm8940.c
+++ b/sound/soc/codecs/wm8940.c
@@ -769,6 +769,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 eb051ed29bb8..8fbddcaa7449 100644
--- a/sound/soc/codecs/wm8940.h
+++ b/sound/soc/codecs/wm8940.h
@@ -98,5 +98,8 @@ struct wm8940_setup_data {
 /* Bit definitions */
 #define WM8940_SPKMUTE BIT(6)
 
+/* Chip ID */
+#define WM8940_CHIP_ID 0x8940
+
 #endif /* _WM8940_H */
 
-- 
2.20.1

Re: [PATCH 3/4] ASoC: wm8940: Mute also the speaker output

[Charles Keepax]

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.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> ---
> +	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;

This bit is also controlled by the "Speaker Playback Switch" so
you probably need some locking between them to stop them
clobbering each other.

Thanks,
Charles

Re: [PATCH 4/4] ASoC: wm8940: Read chip ID when wm8940 codec probing

[Charles Keepax]

On Wed, Dec 14, 2022 at 01:37:43PM +0100, Lukasz Majewski wrote:
> 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 sam I2C bus address.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> ---

Acked-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Thanks,
Charles

Re: [PATCH 1/4] ASoC: wm8940: Remove warning when no plat data present

[Charles Keepax]

On Wed, Dec 14, 2022 at 01:37:40PM +0100, Lukasz Majewski wrote:
> The lack of platform data in the contemporary Linux
> shall not be the reason to display warnings to the
> kernel logs.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> ---

Acked-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Thanks,
Charles

Re: [PATCH 2/4] ASoC: wm8940: Rewrite code to set proper clocks

[Charles Keepax]

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

Re: [PATCH 2/4] ASoC: wm8940: Rewrite code to set proper clocks

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1374 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/4] ASoC: wm8940: Mute also the speaker output

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1881 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/4] ASoC: wm8940: Mute also the speaker output

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 2852 bytes --]

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.

>  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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/4] ASoC: wm8940: Rewrite code to set proper clocks

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 2326 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/4] ASoC: wm8940: Rewrite code to set proper clocks

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 2043 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/4] ASoC: wm8940: Mute also the speaker output

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 3469 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2 1/3] ASoC: wm8940: Remove warning when no plat data present

[Lukasz Majewski]

The lack of platform data in the contemporary Linux
shall not be the reason to display warnings to the
kernel logs.

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 | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/sound/soc/codecs/wm8940.c b/sound/soc/codecs/wm8940.c
index 8dac9fd88547..0b59020d747f 100644
--- a/sound/soc/codecs/wm8940.c
+++ b/sound/soc/codecs/wm8940.c
@@ -709,9 +709,7 @@ static int wm8940_probe(struct snd_soc_component *component)
 	if (ret < 0)
 		return ret;
 
-	if (!pdata)
-		dev_warn(component->dev, "No platform data supplied\n");
-	else {
+	if (pdata) {
 		reg = snd_soc_component_read(component, WM8940_OUTPUTCTL);
 		ret = snd_soc_component_write(component, WM8940_OUTPUTCTL, reg | pdata->vroi);
 		if (ret < 0)
-- 
2.20.1

[PATCH v2 2/3] ASoC: wm8940: Rewrite code to set proper clocks

[Lukasz Majewski]

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 |				\
-- 
2.20.1

[PATCH v2 3/3] ASoC: wm8940: Read chip ID when wm8940 codec probing

[Lukasz Majewski]

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 */
 
-- 
2.20.1

Re: [PATCH v2 2/3] ASoC: wm8940: Rewrite code to set proper clocks

[Charles Keepax]

On Thu, Dec 15, 2022 at 10:36:14AM +0100, Lukasz Majewski wrote:
> 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).

Minor nit this doesn't quite match the commit <12-chars of SHA>
("<description>") format in Submitting-patches.rst.

> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> ---

But the patch looks good to me:

Acked-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Thanks,
Charles

Re: [PATCH v2 1/3] ASoC: wm8940: Remove warning when no plat data present

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 475 bytes --]

On Thu, Dec 15, 2022 at 10:36:13AM +0100, Lukasz Majewski wrote:
> The lack of platform data in the contemporary Linux
> shall not be the reason to display warnings to the
> kernel logs.

Please don't send new patches in reply to old patches or serieses, this
makes it harder for both people and tools to understand what is going
on - it can bury things in mailboxes and make it difficult to keep track
of what current patches are, both for the new patches and the old ones.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/3] ASoC: wm8940: Remove warning when no plat data present

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 818 bytes --]

Hi Mark,

> On Thu, Dec 15, 2022 at 10:36:13AM +0100, Lukasz Majewski wrote:
> > The lack of platform data in the contemporary Linux
> > shall not be the reason to display warnings to the
> > kernel logs.  
> 
> Please don't send new patches in reply to old patches or serieses,
> this makes it harder for both people and tools to understand what is
> going on - it can bury things in mailboxes and make it difficult to
> keep track of what current patches are, both for the new patches and
> the old ones.

Ok. I will not use --in-reply-to.


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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: (subset) [PATCH 0/4] ASoC: Fixes for WM8940 codec

[Mark Brown]

On Wed, 14 Dec 2022 13:37:39 +0100, Lukasz Majewski wrote:
> This patch series provides fixes for WM8940 codec.
> 
> The most notable change is the clock rewrite, so this driver now
> can either generate proper clock frequency by itself or use one
> provided from the clock subsystem of the SoC.
> 
> Lukasz Majewski (4):
>   ASoC: wm8940: Remove warning when no plat data present
>   ASoC: wm8940: Rewrite code to set proper clocks
>   ASoC: wm8940: Mute also the speaker output
>   ASoC: wm8940: Read chip ID when wm8940 codec probing
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/4] ASoC: wm8940: Remove warning when no plat data present
      commit: 5dc5e76b4c41fc8cdd9ed77653b2ce453974fb30
[4/4] ASoC: wm8940: Read chip ID when wm8940 codec probing
      commit: a5c26ee572d94337baf9c944b7b4881a2db62d37

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH 0/4] ASoC: Fixes for WM8940 codec

[Mark Brown]

On Wed, 14 Dec 2022 13:37:39 +0100, Lukasz Majewski wrote:
> This patch series provides fixes for WM8940 codec.
> 
> The most notable change is the clock rewrite, so this driver now
> can either generate proper clock frequency by itself or use one
> provided from the clock subsystem of the SoC.
> 
> Lukasz Majewski (4):
>   ASoC: wm8940: Remove warning when no plat data present
>   ASoC: wm8940: Rewrite code to set proper clocks
>   ASoC: wm8940: Mute also the speaker output
>   ASoC: wm8940: Read chip ID when wm8940 codec probing
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/3] ASoC: wm8940: Remove warning when no plat data present
      commit: 5dc5e76b4c41fc8cdd9ed77653b2ce453974fb30
[2/3] ASoC: wm8940: Rewrite code to set proper clocks
      commit: 294833fc9eb4e9d6c69f8d158cd991d641e59908
[3/3] ASoC: wm8940: Read chip ID when wm8940 codec probing
      commit: a5c26ee572d94337baf9c944b7b4881a2db62d37

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark