| Message ID | 20220606154441.20848-1-lukma@denx.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] ASoC: wm8940: Remove warning when no plat data | expand |
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...
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.
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 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#L614 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.
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.
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)
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(-)