[PATCH] ASoC: cs35l56: Disconnect ASP1 TX sources when ASP1 DAI is hooked up

[PATCH] ASoC: cs35l56: Disconnect ASP1 TX sources when ASP1 DAI is hooked up

[Richard Fitzgerald]

If the ASP1 DAI is hooked up by the machine driver the ASP TX mixer
sources should be initialized to disconnected.

The silicon default is for the mixer source registers to default to
a collection of monitoring sources. The problem with this is that it
causes the DAPM graph to initialize with the capture path connected
to a valid source widget, even though nothing setup a path. When the
ASP DAI is connected as a codec-to-codec link this will cause the other
codec to power-up even though nothing is using it.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
Fixes: dfd2ffb37399 ("ASoC: cs35l56: Prevent overwriting firmware ASP config")
---
 sound/soc/codecs/cs35l56-shared.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/sound/soc/codecs/cs35l56-shared.c b/sound/soc/codecs/cs35l56-shared.c
index 8af89a263594..30497152e02a 100644
--- a/sound/soc/codecs/cs35l56-shared.c
+++ b/sound/soc/codecs/cs35l56-shared.c
@@ -215,6 +215,10 @@ static const struct reg_sequence cs35l56_asp1_defaults[] = {
 	REG_SEQ0(CS35L56_ASP1_FRAME_CONTROL5,	0x00020100),
 	REG_SEQ0(CS35L56_ASP1_DATA_CONTROL1,	0x00000018),
 	REG_SEQ0(CS35L56_ASP1_DATA_CONTROL5,	0x00000018),
+	REG_SEQ0(CS35L56_ASP1TX1_INPUT,		0x00000000),
+	REG_SEQ0(CS35L56_ASP1TX2_INPUT,		0x00000000),
+	REG_SEQ0(CS35L56_ASP1TX3_INPUT,		0x00000000),
+	REG_SEQ0(CS35L56_ASP1TX4_INPUT,		0x00000000),
 };
 
 /*
-- 
2.39.2

Re: [PATCH] ASoC: cs35l56: Disconnect ASP1 TX sources when ASP1 DAI is hooked up

[Mark Brown]

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

On Tue, Jun 11, 2024 at 03:57:46PM +0100, Richard Fitzgerald wrote:
> If the ASP1 DAI is hooked up by the machine driver the ASP TX mixer
> sources should be initialized to disconnected.
> 
> The silicon default is for the mixer source registers to default to
> a collection of monitoring sources. The problem with this is that it
> causes the DAPM graph to initialize with the capture path connected
> to a valid source widget, even though nothing setup a path. When the
> ASP DAI is connected as a codec-to-codec link this will cause the other
> codec to power-up even though nothing is using it.
> 
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> Fixes: dfd2ffb37399 ("ASoC: cs35l56: Prevent overwriting firmware ASP config")

This doesn't seem particularly different to any other unhelpful chip
default, I'm not sure why it'd be so urgent that we'd hard code a
default?  There were some other devices with things like bypass routes
set up.  The capture path getting spuriously triggered feels like
something that should just be sorted in general (TBH I thought that
worked OK but it's been quite some time since I looked properly).

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

Re: [PATCH] ASoC: cs35l56: Disconnect ASP1 TX sources when ASP1 DAI is hooked up

[Charles Keepax]

On Tue, Jun 11, 2024 at 05:12:06PM +0100, Mark Brown wrote:
> On Tue, Jun 11, 2024 at 03:57:46PM +0100, Richard Fitzgerald wrote:
> > If the ASP1 DAI is hooked up by the machine driver the ASP TX mixer
> > sources should be initialized to disconnected.
> > 
> > The silicon default is for the mixer source registers to default to
> > a collection of monitoring sources. The problem with this is that it
> > causes the DAPM graph to initialize with the capture path connected
> > to a valid source widget, even though nothing setup a path. When the
> > ASP DAI is connected as a codec-to-codec link this will cause the other
> > codec to power-up even though nothing is using it.
> > 
> > Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> > Fixes: dfd2ffb37399 ("ASoC: cs35l56: Prevent overwriting firmware ASP config")
> 
> This doesn't seem particularly different to any other unhelpful chip
> default, I'm not sure why it'd be so urgent that we'd hard code a
> default?  There were some other devices with things like bypass routes
> set up.  The capture path getting spuriously triggered feels like
> something that should just be sorted in general (TBH I thought that
> worked OK but it's been quite some time since I looked properly).

Mostly the problem here is it causes a bunch of errors in the
kernel log. The cs42l43 can only be clocked from the soundwire,
and the rate of that is only passed to the cs42l43 when audio plays.
When the ALSA control restore runs, you end up with a temporary route
(cs42l43 VMON -> cs35l56 ASP -> cs42l43 ASP -> cs42l43 Speaker). But
as there is no audio at that point there are no settings for the
PLL. I don't think it causes any lasting issues, but it does cause a
bunch of fat warnings in the log which people then complain about.
Plus also having things not routed by default is just nicer,
especially when the defaults are things that are an actual source
so likely to cause things to power up inadvertantly.

Thanks,
Charles

Re: [PATCH] ASoC: cs35l56: Disconnect ASP1 TX sources when ASP1 DAI is hooked up

[Richard Fitzgerald]

On 11/06/2024 17:12, Mark Brown wrote:
> On Tue, Jun 11, 2024 at 03:57:46PM +0100, Richard Fitzgerald wrote:
>> If the ASP1 DAI is hooked up by the machine driver the ASP TX mixer
>> sources should be initialized to disconnected.
>>
>> The silicon default is for the mixer source registers to default to
>> a collection of monitoring sources. The problem with this is that it
>> causes the DAPM graph to initialize with the capture path connected
>> to a valid source widget, even though nothing setup a path. When the
>> ASP DAI is connected as a codec-to-codec link this will cause the other
>> codec to power-up even though nothing is using it.
>>
>> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
>> Fixes: dfd2ffb37399 ("ASoC: cs35l56: Prevent overwriting firmware ASP config")
> 
> This doesn't seem particularly different to any other unhelpful chip
> default, I'm not sure why it'd be so urgent that we'd hard code a

I'm not sure I understand the objection here. Are you objecting to the
patch itself, or that I marked it as a Fixes?

> default?  There were some other devices with things like bypass routes
> set up.  The capture path getting spuriously triggered feels like
> something that should just be sorted in general (TBH I thought that
> worked OK but it's been quite some time since I looked properly).

Re: [PATCH] ASoC: cs35l56: Disconnect ASP1 TX sources when ASP1 DAI is hooked up

[Mark Brown]

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

On Wed, Jun 12, 2024 at 10:19:06AM +0100, Richard Fitzgerald wrote:
> On 11/06/2024 17:12, Mark Brown wrote:

> > This doesn't seem particularly different to any other unhelpful chip
> > default, I'm not sure why it'd be so urgent that we'd hard code a

> I'm not sure I understand the objection here. Are you objecting to the
> patch itself, or that I marked it as a Fixes?

Both I guess, but mainly the patch itself - we generally have an
extremely high bar for setting defaults different to the chip defaults
to avoid issues with differing use cases.

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

Re: [PATCH] ASoC: cs35l56: Disconnect ASP1 TX sources when ASP1 DAI is hooked up

[Richard Fitzgerald]

On 12/06/2024 11:19, Mark Brown wrote:
> On Wed, Jun 12, 2024 at 10:19:06AM +0100, Richard Fitzgerald wrote:
>> On 11/06/2024 17:12, Mark Brown wrote:
> 
>>> This doesn't seem particularly different to any other unhelpful chip
>>> default, I'm not sure why it'd be so urgent that we'd hard code a
> 
>> I'm not sure I understand the objection here. Are you objecting to the
>> patch itself, or that I marked it as a Fixes?
> 
> Both I guess, but mainly the patch itself - we generally have an
> extremely high bar for setting defaults different to the chip defaults
> to avoid issues with differing use cases.

But that's somewhat part of the problem. There's no such thing as a chip
default for the cs35l56 because for Windows reasons the firmware patches
up all the registers on boot to setup a hardcoded use-case configuration
so that the Windows SDCA drivers can be generic and don't need knowledge
of chip-specific registers. The firmware is customized per end-product,
so the initial register state can vary.

The driver currently attempts to sync-up the mixer registers with what
the firmware set them to, which means that the initial starting state
of the ALSA controls can vary between products using the cs35l56, and
in theory even between revisions of the firmware if there was a need
to change the routing setup for Windows. We don't really need to do
that on Linux - it has a use-case manager and ALSA controls for this
stuff, and the code to do all the syncing-up adds a lot of driver
complexity. We want to remove that complexity, and instead patch the
registers back to silicon defaults - obliterate the Windows use-case
setup and use standard Linux mechanisms to setup what is required.

However, the mixer settings were already causing a bunch of log
warnings. So I decided to split this fix off so it's not dependent on a
big code revert.

I can send a V2 of this patch with this long explanation of the
background. I didn't put it all in V1 because it never occurred to me
that defaulting the mixer sources would be controversial.

Re: [PATCH] ASoC: cs35l56: Disconnect ASP1 TX sources when ASP1 DAI is hooked up

[Mark Brown]

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

On Wed, Jun 12, 2024 at 11:42:15AM +0100, Richard Fitzgerald wrote:

> I can send a V2 of this patch with this long explanation of the
> background. I didn't put it all in V1 because it never occurred to me

Yes, please - it just needs to mention that due to the firmware we don't
have hardware defaults here.  I think it would probably be more robust
for the driver to just set all the documented registers rather than
hoping it's set all the important values and needing to keep adding to
that piecemeal.

> that defaulting the mixer sources would be controversial.

There's probably a reason you have all these use case specific
variations set in firmware...

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