ASoC: intel: sof_sdw: Add CS42L43 CODEC support #4546

bardliao · 2023-08-25T03:29:13Z

Add cs42l43 codec support to sof_sdw machine driver.
The first 6 commits are applied by upstream and the 7th commit is for old codec version only.
Only the last 2 commits need to review.

sound/soc/intel/boards/sof_sdw_cs42l43.c

charleskeepax · 2023-08-25T08:53:44Z

sound/soc/intel/boards/sof_sdw_cs42l43.c

+};
+
+static const struct snd_soc_dapm_route sof_map[] = {
+	{ "Speaker", NULL, "cs42l43 AMP1_OUT_P" },


Slight question on what makes sense here? Does your setup use the Cohen speakers or is it only using Jamerson speakers? Although I guess given the way this stuff works this would be shared across systems so probably should have a superset routing in it.

And if it does use the Cohen ones, at minimum this should be linked to AMP1_OUT_N as well, probably AMP2_OUT_P/N as well assuming both speakers are being used.

I don't have a good understanding of how this 'sof_sdw' machine driver might handle the Cohen+Jamerson combination, maybe it's an entirely different machine driver really since it's a mix of SPI + SoundWire IIRC?

On second thought, I think we don't need the map and the widget at all. The purpose of those maps in machine driver is to switch on/off the devices. But we have one pcm per device, the devices will be on/off when we open/close the pcm.

The configuration is using Cohen's headset and DMIC + Jamerson's speakers. @plbossart In our usage, both Cohen and Jamerson are SoundWire.

I dont mind skipping them for now, but really machine drivers should generally have representation of the board level audio stuff, like mics and speakers. For example if you mark the card as fully_routed nothing will turn on if its not attached to a proper input/output.

@charleskeepax Understand. I will add the widgets and the map back. BTW, do you have recommended INPUT/OUTPUT for headset and DMIC? For example, PDM1_DIN and PDM2_DIN for DMIC.

This is what I currently have internally:

static const struct snd_soc_dapm_widget sof_widgets[] = { SND_SOC_DAPM_HP("Headphone", NULL), SND_SOC_DAPM_MIC("Headset Mic", NULL), SND_SOC_DAPM_SPK("Left Speaker", NULL), SND_SOC_DAPM_SPK("Right Speaker", NULL), SND_SOC_DAPM_MIC("Left Mic", NULL), SND_SOC_DAPM_MIC("Right Mic", NULL), }; static const struct snd_soc_dapm_route sof_map[] = { { "Headphone", NULL, "cs42l43 AMP3_OUT" }, { "Headphone", NULL, "cs42l43 AMP4_OUT" }, { "cs42l43 ADC1_IN1_P", NULL, "Headset Mic" }, { "cs42l43 ADC1_IN1_N", NULL, "Headset Mic" }, { "Left Speaker", NULL, "cs42l43 AMP1_OUT_P" }, { "Left Speaker", NULL, "cs42l43 AMP1_OUT_N" }, { "Right Speaker", NULL, "cs42l43 AMP2_OUT_P" }, { "Right Speaker", NULL, "cs42l43 AMP2_OUT_N" }, { "cs42l43 PDM2_DIN", NULL, "Left Mic" }, { "cs42l43 PDM2_DIN", NULL, "Right Mic" }, };

Although doing separate left and rights might be excessive, bit of a judgement call there, but certainly happy to drop those if you prefer. I guess that is still missing a route for the stereo microphone jack case, but we can probably ignore that for now as its pretty niche.

On the plus side, I think this routing will be pretty standard everyone seems to be sticking with the connections recommended by the apps guys here.

In fact thinking about it merging left and rights is probably better since it allows you to have a single pin switch control for both if one wishes.

charleskeepax · 2023-08-25T08:54:52Z

sound/soc/intel/boards/sof_sdw_cs42l43.c

+#include "sof_sdw_common.h"
+
+static const struct snd_soc_dapm_widget sof_widgets[] = {
+	SND_SOC_DAPM_SPK("Speaker", NULL),


Should probably add widgets for the headphone, headset mic, on board mics, since we added DAIs for them all.

sound/soc/intel/boards/sof_sdw_cs42l43.c

plbossart · 2023-08-25T18:14:49Z

sound/soc/intel/boards/sof_sdw_cs42l43.c

+};
+
+static const struct snd_soc_dapm_route sof_map[] = {
+	{ "Speaker", NULL, "cs42l43 AMP1_OUT_P" },


I don't have a good understanding of how this 'sof_sdw' machine driver might handle the Cohen+Jamerson combination, maybe it's an entirely different machine driver really since it's a mix of SPI + SoundWire IIRC?

bardliao · 2023-08-28T11:23:21Z

@plbossart @charleskeepax There is an open in my mind. Currently, I don't add Cohen's speaker DAI in codec_info_list[] because we don't use for now. But, if a customer decides to use Cohen's speaker, we should add it in codec_info_list[].
How should we know if a device uses Cohen's speaker or not? For example,
Device A uses Cohen's Headset, DMIC, and Speaker.
Device B uses Cohen's Headset, DMIC and Jameson's Speaker
Device C uses Cohen's Headset, DMIC, Speaker combines with Jameson's Speaker.
How do we know what exactly a device use?

charleskeepax · 2023-08-28T12:26:49Z

How should we know if a device uses Cohen's speaker or not?

Yeah I don't have a fully formed plan of that either yet, I still have a few things to work through certainly this week maybe slipping into next and then I am probably going to be spending a bit more time on those sorts of issues. For now my best guess is it probably ends up being some sort of quirk.

I don't have a good understanding of how this 'sof_sdw' machine driver might handle the Cohen+Jamerson combination, maybe it's an entirely different machine driver really since it's a mix of SPI + SoundWire IIRC?

Not all Cohen + Jamerson is that way, most of the systems will be both devices on SoundWire which is simple enough. But some of them will have that odd configuration with the Jamersons hidden behind the Cohen. The current plan there was to add some things to the machine driver to allow creating some codec to codec links on the card, but the patch set isn't really ready for review yet.

bardliao · 2023-08-29T03:16:58Z

Rebased.
Changes: https://github.com/thesofproject/linux/compare/44a93a6702f2497749ce1d017da570c7f6b56dd3..d04ad53a57ad96499aceb5fda26946a3555717e3

plbossart

question for @charleskeepax

plbossart · 2023-08-29T14:44:59Z

sound/soc/intel/boards/sof_sdw_cs42l43.c

+	int ret;
+
+	card->components = devm_kasprintf(card->dev, GFP_KERNEL, "%s mic:cs42l43-dmic",
+					  card->components);


this is a bit weird, it will unconditionally tell userspace/UCM about the presence of dmics attached to the Cirrus Logic codec.

What happens if the dmics are attached to the PCH, as done on Chromebooks? Is this not a supported configuration?

this is a bit weird, it will unconditionally tell userspace/UCM about the presence of dmics attached to the Cirrus Logic codec.

What happens if the dmics are attached to the PCH, as done on Chromebooks? Is this not a supported configuration?

@plbossart I think the question is how do we know what functions are used in a multi-function codec? Maybe use a mask, but where can we get the mask? Using a dmi quirk? That's something we need to think about. My idea is that we can create dai links based on what functions are used. Then, the card->components will be added only when the function is used.
Btw, I think it will be better if a codec can use different SDW ADR to tell what functions are supported, and customers select the codec that fits their requirement. Like, rt712 supports jack, dmic, and speaker, and rt713 supports jack and dmic. So that we can assume that the speaker will be used when a device uses rt712 instead of rt713.
Maybe using different unique id is a way to tell what functions are used?

That would involve OTPing the parts differently would it not? I suspect the hardware folks are very unlikely to go for that plan. The part should have all the disco stuff in ACPI I suspect the best plan might be to parse that to see which bits are being used. I will have a bit of a look and see where I get to on that.

I have limited hopes that _DSD properties would help, most BIOS vendors don't get this right usually. I think all the DisCo plans are vaporware for now, see how well it went for the version 1.0, the only thing we use is the _ADR and even that is wrong on all Intel development devices and some HP/NUC commercial devices derived from the Intel reference.

And anyways the PCH-attached DMIC is not really covered in such properties, it's part of the other Intel NHLT stuff.

When we have a discrete microphone codec present, it's relatively easy to make a decision, but if the Cirrus codec may or may not have microphones attached then it's more difficult, isn't it?

It looks like Windows is making more of a move to use the disco stuff now so that should to some extent help it along, but if we dont want to trust it for now, I think that mostly leaves us with quirks as an option. I guess another option might be to hook up all the interfaces regardless of if they are attached to something although I suppose that doesnt give user-space any info about what it can and cant do.

IIRC we have a restriction in the machine driver, I think we are assuming that either the intel PCH DMIC is used or the SoundWire MIC is used, not both.

Maybe best to check with @bardliao

BTW This isn't a random academic problem I am asking. The RT711 is capable of handling headset and local mics, but we've always used the PCH-DMIC.

IIRC we have a restriction in the machine driver, I think we are assuming that either the intel PCH DMIC is used or the SoundWire MIC is used, not both.

Maybe best to check with @bardliao

I think the machine driver can have both SoundWire MIC and PCH DMIC. But, we never have such configuration.

BTW This isn't a random academic problem I am asking. The RT711 is capable of handling headset and local mics, but we've always used the PCH-DMIC.

Right, but I feel that we will face the issue soon when people want to use Cohen's speaker.

plbossart

we'll have to figure out the DMIC problem at some point, but let's merge this for now.

plbossart · 2023-09-06T13:06:31Z

@bardliao can you rebase and check the build issues?

Add support for the Cirrus Logic CS42L43 using SoundWire. Signed-off-by: Lucas Tanure <[email protected]> Signed-off-by: Charles Keepax <[email protected]> Signed-off-by: Bard Liao <[email protected]>

…l56-eight-c This patch adds acpi match table for cdb35l56-eight-c AIC board from Cirrus Logic. The codec layout is configured as: - Link0: CS42L43 Jack - Link1: 2x CS35L56 Speaker - Link2: 2x CS35L56 Speaker Signed-off-by: Chao Song <[email protected]>

bardliao · 2023-09-07T10:07:10Z

@bardliao can you rebase and check the build issues?

Done

plbossart · 2023-09-07T17:16:46Z

@bardliao I'll let you merge this when @charleskeepax re-adds his review

bardliao requested review from charleskeepax, plbossart and aiChaoSONG August 25, 2023 03:29

charleskeepax suggested changes Aug 25, 2023

View reviewed changes

plbossart reviewed Aug 25, 2023

View reviewed changes

bardliao force-pushed the for-cs42l43 branch from b3e3d2e to 44a93a6 Compare August 28, 2023 11:00

bardliao force-pushed the for-cs42l43 branch 2 times, most recently from d04ad53 to e6f3ba8 Compare August 29, 2023 03:16

bardliao marked this pull request as ready for review August 29, 2023 03:17

bardliao requested review from lgirdwood, ranj063, kv2019i, dbaluta, ujfalusi, vijendarmukunda, kuanhsuncheng and yaochunhung as code owners August 29, 2023 03:17

charleskeepax previously approved these changes Aug 29, 2023

View reviewed changes

plbossart reviewed Aug 29, 2023

View reviewed changes

plbossart previously approved these changes Sep 6, 2023

View reviewed changes

bardliao force-pushed the for-cs42l43 branch from e6f3ba8 to de6a513 Compare September 7, 2023 07:30

bardliao dismissed stale reviews from plbossart and charleskeepax via 4a884f7 September 7, 2023 08:40

bardliao force-pushed the for-cs42l43 branch from de6a513 to 4a884f7 Compare September 7, 2023 08:40

bardliao and others added 2 commits September 7, 2023 16:47

ASoC: intel: sof_sdw: Add CS42L43 CODEC support

b941ab4

Add support for the Cirrus Logic CS42L43 using SoundWire. Signed-off-by: Lucas Tanure <[email protected]> Signed-off-by: Charles Keepax <[email protected]> Signed-off-by: Bard Liao <[email protected]>

plbossart approved these changes Sep 7, 2023

View reviewed changes

plbossart requested a review from charleskeepax September 7, 2023 13:45

charleskeepax approved these changes Sep 7, 2023

View reviewed changes

bardliao merged commit dd52647 into thesofproject:topic/sof-dev Sep 8, 2023
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ASoC: intel: sof_sdw: Add CS42L43 CODEC support #4546

ASoC: intel: sof_sdw: Add CS42L43 CODEC support #4546

bardliao commented Aug 25, 2023

charleskeepax Aug 25, 2023

plbossart Aug 25, 2023

bardliao Aug 28, 2023

bardliao Aug 28, 2023

charleskeepax Aug 28, 2023

bardliao Aug 28, 2023

charleskeepax Aug 28, 2023

charleskeepax Aug 28, 2023

charleskeepax Aug 25, 2023

plbossart Aug 25, 2023

bardliao commented Aug 28, 2023

charleskeepax commented Aug 28, 2023

bardliao commented Aug 29, 2023

plbossart left a comment

plbossart Aug 29, 2023

bardliao Aug 30, 2023

charleskeepax Aug 30, 2023

plbossart Aug 30, 2023

charleskeepax Aug 30, 2023

plbossart Aug 30, 2023

bardliao Aug 31, 2023

plbossart left a comment

plbossart commented Sep 6, 2023

bardliao commented Sep 7, 2023

plbossart commented Sep 7, 2023

ASoC: intel: sof_sdw: Add CS42L43 CODEC support #4546

ASoC: intel: sof_sdw: Add CS42L43 CODEC support #4546

Conversation

bardliao commented Aug 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bardliao commented Aug 28, 2023

charleskeepax commented Aug 28, 2023

bardliao commented Aug 29, 2023

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plbossart left a comment

Choose a reason for hiding this comment

plbossart commented Sep 6, 2023

bardliao commented Sep 7, 2023

plbossart commented Sep 7, 2023