ASoC: codecs: wsa88xx: fix codec initialisation

The soundwire update_status() callback may be called multiple times with the same ATTACHED status but initialisation should only be done when transitioning from UNATTACHED to ATTACHED.

This avoids repeated initialisation of the codecs during boot of machines like the Lenovo ThinkPad X13s:

[   11.614523] wsa883x-codec sdw:1:0:0217:0202:00:1: WSA883X Version 1_1, Variant: WSA8835_V2
[   11.618022] wsa883x-codec sdw:1:0:0217:0202:00:1: WSA883X Version 1_1, Variant: WSA8835_V2
[   11.621377] wsa883x-codec sdw:1:0:0217:0202:00:1: WSA883X Version 1_1, Variant: WSA8835_V2
[   11.624065] wsa883x-codec sdw:1:0:0217:0202:00:1: WSA883X Version 1_1, Variant: WSA8835_V2
[   11.631382] wsa883x-codec sdw:1:0:0217:0202:00:2: WSA883X Version 1_1, Variant: WSA8835_V2
[   11.634424] wsa883x-codec sdw:1:0:0217:0202:00:2: WSA883X Version 1_1, Variant: WSA8835_V2

Fixes 43b8c7dc85a1 ("ASoC: codecs: add wsa883x amplifier support")

Cc [email protected] # 6.0

Cc Srinivas Kandagatla <[email protected]>

The soundwire update_status() callback may be called multiple times with the same ATTACHED status but initialisation should only be done when transitioning from UNATTACHED to ATTACHED.

Fixes a0aab9e1404a ("ASoC: codecs: add wsa881x amplifier support")

Cc [email protected] # 5.6

Cc Srinivas Kandagatla <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---

The soundwire update_status() callback may be called multiple times with the same ATTACHED status but initialisation should only be done when transitioning from UNATTACHED to ATTACHED.

Fix the inverted hw_init flag which was set to false instead of true after initialisation which defeats its purpose and may result in repeated unnecessary initialisation.

Similarly, the initial state of the flag was also inverted so that the codec would only be initialised and brought out of regmap cache only mode if its status first transitions to UNATTACHED.

Quoted 7 lines · Johan Hovold

The soundwire update_status() callback may be called multiple times with the same ATTACHED status but initialisation should only be done when transitioning from UNATTACHED to ATTACHED.

Fix the inverted hw_init flag which was set to false instead of true after initialisation which defeats its purpose and may result in repeated unnecessary initialisation.

Either it results or it does not, not "may".

If the device moves to
UNATTACHED state flag should be probably set to "true". This is the bug.

Quoted 4 lines

Similarly, the initial state of the flag was also inverted so that the codec would only be initialised and brought out of regmap cache only mode if its status first transitions to UNATTACHED.

Maybe that's confusing wording but existing code was intentional and IMO almost correct. The flag is saying - we need hw init - that's why it is set to true in the probe and to false AFTER the proper hw initialization which is done after ATTACHED state.

Quoted 10 lines · Krzysztof Kozlowski

Quoted 7 lines · Johan Hovold

The soundwire update_status() callback may be called multiple times with the same ATTACHED status but initialisation should only be done when transitioning from UNATTACHED to ATTACHED.

Fix the inverted hw_init flag which was set to false instead of true after initialisation which defeats its purpose and may result in repeated unnecessary initialisation.

Either it results or it does not, not "may".

No, it depends on whether update_status() is called with the same status more than once. So "may" is correct here.

Quoted 2 lines

If the device moves to
UNATTACHED state flag should be probably set to "true". This is the bug.

No, update_status() has:

	if (wsa884x->hw_init || status != SDW_SLAVE_ATTACHED)
		return 0;

	...

	wsa884x_init(wsa884x);

so if you set hw_init to true then init is never called when status is changed to ATTACHED.

Quoted 6 lines

Quoted 5 lines

Similarly, the initial state of the flag was also inverted so that the codec would only be initialised and brought out of regmap cache only mode if its status first transitions to UNATTACHED.

Quoted 4 lines

Maybe that's confusing wording but existing code was intentional and IMO almost correct. The flag is saying - we need hw init - that's why it is set to true in the probe and to false AFTER the proper hw initialization which is done after ATTACHED state.

All other codec drivers have hw_init mean that init has been done and the check in update_status() reflects that too so this driver is still broken.

Quoted 19 lines · Johan Hovold

Quoted 8 lines · Krzysztof Kozlowski

Quoted 7 lines · Johan Hovold

The soundwire update_status() callback may be called multiple times with the same ATTACHED status but initialisation should only be done when transitioning from UNATTACHED to ATTACHED.

Fix the inverted hw_init flag which was set to false instead of true after initialisation which defeats its purpose and may result in repeated unnecessary initialisation.

No, update_status() has:

	if (wsa884x->hw_init || status != SDW_SLAVE_ATTACHED)
		return 0;

	...

	wsa884x_init(wsa884x);

Quoted 2 lines

so if you set hw_init to true then init is never called when status is changed to ATTACHED.

Uh, indeed, so this was supposed to be !wsa884x->hw_init... or indeed your meaning. This also means that this was never passing above if() and the init() was never called.

regcache was probably synced via runtime PM, so at least that part worked.

Did you test this driver on actual device how it affects the behavior?

Quoted 13 lines

Quoted 6 lines

	...

	wsa884x_init(wsa884x);

so if you set hw_init to true then init is never called when status is changed to ATTACHED.

Uh, indeed, so this was supposed to be !wsa884x->hw_init... or indeed your meaning. This also means that this was never passing above if() and the init() was never called.

regcache was probably synced via runtime PM, so at least that part worked.

Quoted 1 line

Did you test this driver on actual device how it affects the behavior?

No, I don't have a device that uses this codec anymore.

If you could give it a spin that would be great. It seems we have been depending on reset values or an UNATTACHED => ATTACHED transition so far.

The soundwire update_status() callback may be called multiple times with the same ATTACHED status but initialisation should only be done when transitioning from UNATTACHED to ATTACHED.

Fix the inverted hw_init flag which was set to false instead of true after initialisation which defeats its purpose and may result in repeated unnecessary initialisation.

Similarly, the initial state of the flag was also inverted so that the codec would only be initialised and brought out of regmap cache only mode if its status first transitions to UNATTACHED.

Fixes aa21a7d4f68a ("ASoC: codecs: wsa884x: Add WSA884x family of speakers")

Cc [email protected] # 6.5

Cc Krzysztof Kozlowski <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---

Drivers should generally be silent on successful probe.

Demote the codec variant printk to debug level and instead add a warning in case an unknown variant is ever encountered.

Signed-off-by Johan Hovold <[email protected]>

---
 sound/soc/codecs/wsa883x.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

The soundwire update_status() callback may be called multiple times with the same ATTACHED status but initialisation should only be done when transitioning from UNATTACHED to ATTACHED.

This series fixes the Qualcomm wsa88xx codec drivers that do unnecessary reinitialisation or potentially fail to initialise at all.

Included is also a related clean up suppressing a related codec variant printk.

Johan

Johan Hovold (4):
ASoC: codecs: wsa883x: fix unnecessary initialisation
ASoC: codecs: wsa881x: fix unnecessary initialisation
ASoC: codecs: wsa884x: fix codec initialisation
ASoC: codecs: wsa883x: suppress variant printk

 sound/soc/codecs/wsa881x.c |  9 +++++++++
 sound/soc/codecs/wsa883x.c | 26 ++++++++++++++++++--------
 sound/soc/codecs/wsa884x.c |  3 +--
 3 files changed, 28 insertions(+), 10 deletions(-)

The soundwire update_status() callback may be called multiple times with the same ATTACHED status but initialisation should only be done when transitioning from UNATTACHED to ATTACHED.

This series fixes the Qualcomm wsa88xx codec drivers that do unnecessary reinitialisation or potentially fail to initialise at all.

[...]