[PATCH 1/3] ASoC: wm8940: Remove warning when no plat data
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 440d048ef0c0..7cea54720436 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)
Without this change, the wm8940 driver is not working when set_sysclk callback (wm8940_set_dai_sysclk) is called with freqency 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 7cea54720436..6fb1c3780439 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 *wm8940 = 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;
+ wm8940->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 *component = dai->component; + struct wm8940_priv *priv = snd_soc_component_get_drvdata(component); + 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 *component = dai->component; + struct wm8940_priv *priv = snd_soc_component_get_drvdata(component); + + 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 | \
On Mon, Jun 06, 2022 at 05:44:40PM +0200, Lukasz Majewski wrote:
Without this change, the wm8940 driver is not working when set_sysclk callback (wm8940_set_dai_sysclk) is called with freqency 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.
I don't entirely follow the above - in what way might the core adjust the clocking, and why would we want to allow the use of invalid clocks? Surely that just makes error checking worse.
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.
Hi Mark,
On Mon, Jun 06, 2022 at 05:44:40PM +0200, Lukasz Majewski wrote:
Without this change, the wm8940 driver is not working when set_sysclk callback (wm8940_set_dai_sysclk) is called with freqency 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.
I don't entirely follow the above - in what way might the core adjust the clocking, and why would we want to allow the use of invalid clocks? Surely that just makes error checking worse.
Hmm, it is a bit complicated.
When I enabed wm8940 support in mainline - the first call to wm8940_set_dai_sysclk (the set_sysclk callback) required mclk = 11997070 frequency.
With the current code [1] the initialization of the codec returns -EINVAL and the codec is not supported in the system:
asoc-simple-card: probe of sound failed with error -22
The approach used in this patch set to fix the above issue is based on one already present in-tree for wm8974 codec.
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.
Ok, I will add some more verbose description. The aforementioned SHA1 is referring to commit already in-tree, so you would find it easily even without the Internet.
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.
+1
Links:
[1] - https://elixir.bootlin.com/linux/v5.18.1/source/sound/soc/codecs/wm8940.c#L6...
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
On Tue, Jun 07, 2022 at 02:13:09PM +0200, Lukasz Majewski wrote:
On Mon, Jun 06, 2022 at 05:44:40PM +0200, Lukasz Majewski wrote:
I don't entirely follow the above - in what way might the core adjust the clocking, and why would we want to allow the use of invalid clocks? Surely that just makes error checking worse.
Hmm, it is a bit complicated.
When I enabed wm8940 support in mainline - the first call to wm8940_set_dai_sysclk (the set_sysclk callback) required mclk = 11997070 frequency.
With the current code [1] the initialization of the codec returns -EINVAL and the codec is not supported in the system:
asoc-simple-card: probe of sound failed with error -22
Well, that looks like a bug in either simple-card or it's configuration which should be fixed then (you should probably use audio-graph-card for new things BTW). If a machine driver just randomly sets a clock rate that the system can't support and doesn't want then that's a problem, presuambly it's getting that rate from somewhere. Note that this is the machine driver trying to set a clock rate, not the core.
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 6fb1c3780439..a8596f4089dd 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 */
On Mon, Jun 06, 2022 at 05:44:41PM +0200, 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.
This will not interact well with both the user visible control of the speaker volume via the Speaker Playback Volume control and the analog bypass paths that the device has - it'll change the state of the control without generating any events, and cut off any bypassed audio that's mixed in.
You can probably achieve a similar effect by making the control an _AUTODISABLE one which will allow the core to mute the control when it's not being used in a way that's not visible to userspace.
Hi Mark,
On Mon, Jun 06, 2022 at 05:44:41PM +0200, 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.
This will not interact well with both the user visible control of the speaker volume via the Speaker Playback Volume control and the analog bypass paths that the device has - it'll change the state of the control without generating any events, and cut off any bypassed audio that's mixed in.
I'm wondering why it is safe to call DAI's .digital_mute() callback, which explicitly changes state of the "DAC soft mute enable" bit (DACMU) ?
And on the other hand it is not correct to just mute the speakers?
You can probably achieve a similar effect by making the control an _AUTODISABLE one which will allow the core to mute the control when it's not being used in a way that's not visible to userspace.
The exact definition for the event, which I'm forcing above:
SOC_SINGLE("Speaker Playback Switch", WM8940_SPKVOL, 6, 1, 1),
And there is no SOC_SINGLE_AUTODISABLE() macro available.
The issue I'm trying to fix:
- The mclk clock is stopped (after some time) by imx SOC when I end 'speaker-test' program with ctrl+c.
- When the clock is not provided (after ~1sec) I do hear a single short noise from speakers.
- The other solution (which also works) would be to enable clock once (during probe) and then do not disable it till system is powered off (yes it is a hack :-) ).
I'm wondering if this can be fixed by some 'amixer' user space switch?
Thanks in advance for help.
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
On Fri, Jun 10, 2022 at 11:23:31AM +0200, Lukasz Majewski wrote:
On Mon, Jun 06, 2022 at 05:44:41PM +0200, 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.
This will not interact well with both the user visible control of the speaker volume via the Speaker Playback Volume control and the analog bypass paths that the device has - it'll change the state of the control without generating any events, and cut off any bypassed audio that's mixed in.
I'm wondering why it is safe to call DAI's .digital_mute() callback, which explicitly changes state of the "DAC soft mute enable" bit (DACMU) ?
If there's a user visible control for the same register bit that's a bug. If there's no user visible control for it then there's nothing to conflict with.
And on the other hand it is not correct to just mute the speakers?
No, that's not what we're muting playback for - the digital mute is there specifically to deal with issues with host controllers outputing noise during startup/teardown. If there are issues with the speaker output then they need to be addressed at that point, especially given that the device has bypass paths.
You can probably achieve a similar effect by making the control an _AUTODISABLE one which will allow the core to mute the control when it's not being used in a way that's not visible to userspace.
The exact definition for the event, which I'm forcing above:
SOC_SINGLE("Speaker Playback Switch", WM8940_SPKVOL, 6, 1, 1),
And there is no SOC_SINGLE_AUTODISABLE() macro available.
That seems solvable? Though if the issue isn't triggered in connection with a DAPM event (which sounds like the case) then it's probably not going to help.
The issue I'm trying to fix:
- The mclk clock is stopped (after some time) by imx SOC when I end 'speaker-test' program with ctrl+c.
- When the clock is not provided (after ~1sec) I do hear a single short noise from speakers.
- The other solution (which also works) would be to enable clock once (during probe) and then do not disable it till system is powered off (yes it is a hack :-) ).
If the issue is triggered by the MCLK being disabled prematurely then the simplest fix is probably to wire up the CODEC MCLK to the clock API and manage it during set_bias_level() (probably on transition out of and into _STANDBY) - that should have a similar effect to leaving it enabled all the time.
On Mon, Jun 06, 2022 at 05:44:39PM +0200, 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
Given that the device requires configuration and doesn't appear to have any other firmware interface support that's rather a strong statement...
Hi Mark,
On Mon, Jun 06, 2022 at 05:44:39PM +0200, 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
Given that the device requires configuration and doesn't appear to have any other firmware interface support that's rather a strong statement...
Thanks for the comment :-)
My point is that - similar codec - wm8974 don't display such warnings. (this code was not updated/refactored for a quite long time).
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
On Mon, Jun 06, 2022 at 06:17:31PM +0200, Lukasz Majewski wrote:
On Mon, Jun 06, 2022 at 05:44:39PM +0200, Lukasz Majewski wrote:
The lack of platform data in the contemporary Linux shall not be the reason to display warnings to the kernel logs.
Given that the device requires configuration and doesn't appear to have any other firmware interface support that's rather a strong statement...
My point is that - similar codec - wm8974 don't display such warnings. (this code was not updated/refactored for a quite long time).
Perhaps those drivers are buggy, or those devices lack this specific configuration that's being adjusted? The changelog should at least address why the driver was warning about configuration being required but it's safe to ignore that.
Hi Mark,
On Mon, Jun 06, 2022 at 06:17:31PM +0200, Lukasz Majewski wrote:
On Mon, Jun 06, 2022 at 05:44:39PM +0200, Lukasz Majewski wrote:
The lack of platform data in the contemporary Linux shall not be the reason to display warnings to the kernel logs.
Given that the device requires configuration and doesn't appear to have any other firmware interface support that's rather a strong statement...
My point is that - similar codec - wm8974 don't display such warnings. (this code was not updated/refactored for a quite long time).
Perhaps those drivers are buggy, or those devices lack this specific configuration that's being adjusted? The changelog should at least address why the driver was warning about configuration being required but it's safe to ignore that.
With v4.4 from which I forward port those changes only the PXA 'stargate2' mach is using this codec.
In this version there is no reference to 'vroi'.
With newest Linux - there is no reference to this codec (even to any DTS file), so we can assume that from at least v4.4 there is no reference to platform data for it.
I guess that one can provide the 'vroi' information via DTS nowadays if required.
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
On Tue, Jun 07, 2022 at 02:30:39PM +0200, Lukasz Majewski wrote:
On Mon, Jun 06, 2022 at 06:17:31PM +0200, Lukasz Majewski wrote:
On Mon, Jun 06, 2022 at 05:44:39PM +0200, Lukasz Majewski wrote:
My point is that - similar codec - wm8974 don't display such warnings. (this code was not updated/refactored for a quite long time).
Perhaps those drivers are buggy, or those devices lack this specific configuration that's being adjusted? The changelog should at least address why the driver was warning about configuration being required but it's safe to ignore that.
With v4.4 from which I forward port those changes only the PXA 'stargate2' mach is using this codec.
In this version there is no reference to 'vroi'.
That's equivalent to setting a value of 0 given the way platform data works.
I guess that one can provide the 'vroi' information via DTS nowadays if required.
Yes, that's what I'd expect.
participants (2)
-
Lukasz Majewski -
Mark Brown