| Message ID | 20240611145746.1454663-1-rf@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: cs35l56: Disconnect ASP1 TX sources when ASP1 DAI is hooked up | expand |
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).
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
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).
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.
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.
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...
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), }; /*
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(+)