Ssp common module - codec type detection #4536

brentlu · 2023-08-21T07:57:25Z

Create an new common module to host functionalities for all ssp machine drivers:

codec and amplifier type detection
DAI link generation

Machine drivers sof_rt5682, sof_cs42l42, and sof_ssp_amp are tested on ADL Chromebooks. Check headphone playback/capture, speaker playback, DMIC capture, and HDMI playback function.

Brya
[ 5.941973] sof_rt5682 adl_mx98360_rt5682: codec RT5682 found
[ 5.941986] sof_rt5682 adl_mx98360_rt5682: amp MAX98360A found
[ 5.941988] sof_rt5682 adl_mx98360_rt5682: create dai links, link_order 0x7654321
[ 5.941989] sof_rt5682 adl_mx98360_rt5682: link 0: codec RT5682, ssp 0
[ 5.941990] sof_rt5682 adl_mx98360_rt5682: link 1: dmic01
[ 5.941991] sof_rt5682 adl_mx98360_rt5682: link 2: dmic16k
[ 5.941991] sof_rt5682 adl_mx98360_rt5682: link 3: intel hdmi, hdmi id 1
[ 5.941992] sof_rt5682 adl_mx98360_rt5682: link 4: intel hdmi, hdmi id 2
[ 5.941993] sof_rt5682 adl_mx98360_rt5682: link 5: intel hdmi, hdmi id 3
[ 5.941994] sof_rt5682 adl_mx98360_rt5682: link 6: intel hdmi, hdmi id 4
[ 5.941995] sof_rt5682 adl_mx98360_rt5682: link 7: amp MAX98360A, ssp 1
[ 5.941996] sof_rt5682 adl_mx98360_rt5682: link 8: bt, ssp 2

Vell
[ 6.475233] sof_ssp_amp adl_cs35l41: amp CS35L41 found
[ 6.475240] sof_ssp_amp adl_cs35l41: create dai links, link_order 0x643257
[ 6.475243] sof_ssp_amp adl_cs35l41: link 0: amp CS35L41, ssp 1
[ 6.475265] sof_ssp_amp adl_cs35l41: link 1: dmic01
[ 6.475266] sof_ssp_amp adl_cs35l41: link 2: dmic16k
[ 6.475268] sof_ssp_amp adl_cs35l41: link 3: intel hdmi, hdmi id 1
[ 6.475270] sof_ssp_amp adl_cs35l41: link 4: intel hdmi, hdmi id 2
[ 6.475272] sof_ssp_amp adl_cs35l41: link 5: intel hdmi, hdmi id 3
[ 6.475274] sof_ssp_amp adl_cs35l41: link 6: intel hdmi, hdmi id 4
[ 6.475276] sof_ssp_amp adl_cs35l41: link 7: bt, ssp 2

Crota
[ 7.727384] sof_cs42l42 adl_mx98360a_cs4242: codec CS42L42 found
[ 7.727414] sof_cs42l42 adl_mx98360a_cs4242: amp MAX98360A found
[ 7.727416] sof_cs42l42 adl_mx98360a_cs4242: create dai links, link_order 0x7654321
[ 7.727418] sof_cs42l42 adl_mx98360a_cs4242: link 0: codec CS42L42, ssp 0
[ 7.727420] sof_cs42l42 adl_mx98360a_cs4242: link 1: dmic01
[ 7.727421] sof_cs42l42 adl_mx98360a_cs4242: link 2: dmic16k
[ 7.727422] sof_cs42l42 adl_mx98360a_cs4242: link 3: intel hdmi, hdmi id 1
[ 7.727424] sof_cs42l42 adl_mx98360a_cs4242: link 4: intel hdmi, hdmi id 2
[ 7.727426] sof_cs42l42 adl_mx98360a_cs4242: link 5: intel hdmi, hdmi id 3
[ 7.727428] sof_cs42l42 adl_mx98360a_cs4242: link 6: intel hdmi, hdmi id 4
[ 7.727430] sof_cs42l42 adl_mx98360a_cs4242: link 7: amp MAX98360A, ssp 1
[ 7.727434] sof_cs42l42 adl_mx98360a_cs4242: link 8: bt, ssp 2

plbossart

The idea of removing codec quirks is good, but I think you went too far in the refactoring of codec @brentlu

sound/soc/intel/boards/sof_realtek_common.h

plbossart · 2023-08-21T14:23:28Z

sound/soc/intel/boards/sof_ssp_common.c

+#include <sound/soc-acpi.h>
+#include "sof_cirrus_common.h"
+#include "sof_maxim_common.h"
+#include "sof_realtek_common.h"


do we really need to include all these vendor-specific headers? That sounds like a partitioning of code that's not quite right?

The ACPI HID of amplifiers are defined in sof_vendor_common.h so those headers are included here.

comment is still valid, I think you just need to include sof_vendor_common.h ?

I mean the sof_cirrus_common.h, sof_maxim_common.h, and sof_realtek_common.h. Unless we define the HID definition in sof_ssp_common.h and leave those file untouched.

sound/soc/intel/boards/Kconfig

sound/soc/intel/boards/sof_ssp_amp.c

brentlu · 2023-08-22T07:38:31Z

Upload new patchset to support codec detection only

plbossart

I only have minor comments below @brentlu but that PR is moving in the right direction. Removing these quirks is a good thing (tm), thanks for suggesting it.

@bardliao @RanderWang please review if you have spare cycles

sound/soc/intel/boards/sof_realtek_common.h

plbossart · 2023-08-22T17:49:37Z

sound/soc/intel/boards/sof_ssp_common.c

+#include <sound/soc-acpi.h>
+#include "sof_cirrus_common.h"
+#include "sof_maxim_common.h"
+#include "sof_realtek_common.h"


comment is still valid, I think you just need to include sof_vendor_common.h ?

bardliao

Thanks @brentlu. A question about removing SOF_RT5682_SSP_AMP(1). Is it intentional?

sound/soc/intel/boards/sof_rt5682.c

bardliao · 2023-08-23T06:53:53Z

sound/soc/intel/boards/sof_rt5682.c

+			/* no codec conf required */
+			break;
+		case CODEC_MAX98360A:
+			/* no codec conf required */


nitpick: We can group all those codecs together as

case CODEC_MAX98357A: case CODEC_MAX98360A: case CODEC_RT1019P: case CODEC_RT5650: /* no codec conf required */ break;

yes, agree with @bardliao let's have a common block here.

plbossart

Almost good @brentlu, I added two comments below. One more round and we should be good to go.

sound/soc/intel/boards/sof_ssp_common.h

plbossart · 2023-08-23T14:37:53Z

sound/soc/intel/boards/sof_rt5682.c

+			/* no codec conf required */
+			break;
+		case CODEC_MAX98360A:
+			/* no codec conf required */


yes, agree with @bardliao let's have a common block here.

plbossart

one more cosmetic comment from me, thanks @brentlu

sound/soc/intel/boards/sof_nau8825.c

ranj063 · 2023-08-23T16:26:56Z

sound/soc/intel/boards/sof_ssp_common.h

+#define RT5682S_ACPI_HID	"RTL5682"
+
+enum sof_ssp_codec {
+	CODEC_NONE = 0,


is the = 0 really important?

According to the bible, "if no enumerators with = appear, then the values of the corresponding constants begin at 0 and increase by 1 as the declaration is read from left to right". So I think you are right, the = 0 here is more than a habit. Will remove it.

plbossart

LGTM, nice cleanup @brentlu

RanderWang

good work! it can be better.

RanderWang · 2023-08-24T08:27:35Z

sound/soc/intel/boards/sof_ssp_common.c

+ * Codec probe function
+ */
+#define CODEC_MAP_ENTRY(n, h, t)	\
+	{					\


align the \

RanderWang · 2023-08-24T08:42:04Z

sound/soc/intel/boards/sof_nau8825.c

 	if (!dai_links)
 		return -ENOMEM;

 	sof_audio_card_nau8825.dai_link = dai_links;

+	/* update codec_conf */
+	if (ctx->amp_type != CODEC_NONE) {


why not remove this if and use it like

switch (ctx->amp_type) { ... case CODEC_NONE: break; default: ... }

RanderWang · 2023-08-24T08:42:47Z

sound/soc/intel/boards/sof_ssp_amp.c

 	if (!dai_links)
 		return -ENOMEM;

 	sof_ssp_amp_card.dai_link = dai_links;

 	/* update codec_conf */
-	if (sof_ssp_amp_quirk & SOF_CS35L41_SPEAKER_AMP_PRESENT) {
-		cs35l41_set_codec_conf(&sof_ssp_amp_card);
+	if (ctx->amp_type != CODEC_NONE) {


convert this if to case

sound/soc/intel/boards/sof_ssp_common.c

Create a new common module to host functions which could be shared among SSP machine drivers. Add functions to detect headphone codec and speaker amplifier via ACPI system at runtime in order to remove codec type quirks in machine drivers. Signed-off-by: Brent Lu <[email protected]>

Use ACPI HID definition in ssp-common header for device name macros. No functional change here. Signed-off-by: Brent Lu <[email protected]>

Use ssp-common module to detect codec and amplifier type in driver probe function and remove all quirks about codec and amplifier type. Due to codec detection feature, we could remove HP Dooly's DMI quirk safely. Signed-off-by: Brent Lu <[email protected]>

Use ssp-common module to detect codec and amplifier type in driver probe function and remove all quirks about codec and amplifier type. Signed-off-by: Brent Lu <[email protected]>

RanderWang

LGTM

brentlu · 2023-08-28T14:28:08Z

Could we merge this PR? Thanks.

plbossart · 2023-08-28T15:46:02Z

merging but we'll send this upstream after the merge window closes in two weeks.

brentlu requested review from lgirdwood, plbossart, ranj063, kv2019i, dbaluta, bardliao, ujfalusi, vijendarmukunda, kuanhsuncheng and yaochunhung as code owners August 21, 2023 07:57

plbossart requested changes Aug 21, 2023

View reviewed changes

brentlu force-pushed the ssp-common-module branch 3 times, most recently from 5407b35 to 6d7ddfc Compare August 22, 2023 07:35

plbossart reviewed Aug 22, 2023

View reviewed changes

brentlu force-pushed the ssp-common-module branch from 6d7ddfc to 627fd64 Compare August 23, 2023 01:49

bardliao reviewed Aug 23, 2023

View reviewed changes

bardliao previously approved these changes Aug 23, 2023

View reviewed changes

plbossart requested changes Aug 23, 2023

View reviewed changes

brentlu dismissed bardliao’s stale review via 0df1f57 August 23, 2023 16:25

brentlu force-pushed the ssp-common-module branch from 627fd64 to 0df1f57 Compare August 23, 2023 16:25

plbossart requested changes Aug 23, 2023

View reviewed changes

sound/soc/intel/boards/sof_nau8825.c Outdated Show resolved Hide resolved

brentlu force-pushed the ssp-common-module branch from 0df1f57 to 45bdd38 Compare August 23, 2023 16:30

ranj063 reviewed Aug 23, 2023

View reviewed changes

plbossart previously approved these changes Aug 23, 2023

View reviewed changes

brentlu dismissed plbossart’s stale review via a052bb3 August 23, 2023 16:51

brentlu force-pushed the ssp-common-module branch from 45bdd38 to a052bb3 Compare August 23, 2023 16:51

plbossart previously approved these changes Aug 23, 2023

View reviewed changes

bardliao previously approved these changes Aug 24, 2023

View reviewed changes

RanderWang reviewed Aug 24, 2023

View reviewed changes

brentlu dismissed stale reviews from bardliao and plbossart via ef53db3 August 24, 2023 09:01

brentlu force-pushed the ssp-common-module branch from a052bb3 to ef53db3 Compare August 24, 2023 09:01

brentlu added 6 commits August 25, 2023 00:52

ASoC: Intel: use ACPI HID definition in ssp-common

9387af1

Use ACPI HID definition in ssp-common header for device name macros. No functional change here. Signed-off-by: Brent Lu <[email protected]>

ASoC: Intel: sof_cs42l42: use ssp-common module to detect codec

f1dd7b8

Use ssp-common module to detect codec and amplifier type in driver probe function and remove all quirks about codec and amplifier type. Signed-off-by: Brent Lu <[email protected]>

ASoC: Intel: sof_ssp_amp: use ssp-common module to detect codec

564c7a9

Use ssp-common module to detect codec and amplifier type in driver probe function and remove all quirks about codec and amplifier type. Signed-off-by: Brent Lu <[email protected]>

ASoC: Intel: sof_nau8825: use ssp-common module to detect codec

ef53db3

Use ssp-common module to detect codec and amplifier type in driver probe function and remove all quirks about codec and amplifier type. Signed-off-by: Brent Lu <[email protected]>

RanderWang approved these changes Aug 25, 2023

View reviewed changes

plbossart approved these changes Aug 25, 2023

View reviewed changes

plbossart requested review from ranj063 and bardliao August 25, 2023 18:51

bardliao approved these changes Aug 28, 2023

View reviewed changes

brentlu changed the title ~~Ssp common module~~ Ssp common module - codec type detection Aug 28, 2023

plbossart merged commit 8e7f9a3 into thesofproject:topic/sof-dev Aug 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ssp common module - codec type detection #4536

Ssp common module - codec type detection #4536

brentlu commented Aug 21, 2023

plbossart left a comment

plbossart Aug 21, 2023

brentlu Aug 22, 2023 •

edited

Loading

plbossart Aug 22, 2023

brentlu Aug 23, 2023

brentlu commented Aug 22, 2023

plbossart left a comment

plbossart Aug 22, 2023

bardliao left a comment

bardliao Aug 23, 2023

plbossart Aug 23, 2023

brentlu Aug 23, 2023

plbossart left a comment

plbossart Aug 23, 2023

plbossart left a comment

ranj063 Aug 23, 2023

brentlu Aug 23, 2023

plbossart left a comment

RanderWang left a comment

RanderWang Aug 24, 2023

brentlu Aug 24, 2023

RanderWang Aug 24, 2023

brentlu Aug 24, 2023

RanderWang Aug 24, 2023

brentlu Aug 24, 2023

RanderWang left a comment

brentlu commented Aug 28, 2023

plbossart commented Aug 28, 2023

Ssp common module - codec type detection #4536

Ssp common module - codec type detection #4536

Conversation

brentlu commented Aug 21, 2023

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brentlu Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brentlu commented Aug 22, 2023

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bardliao 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

plbossart left a 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

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

RanderWang 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

RanderWang left a comment

Choose a reason for hiding this comment

brentlu commented Aug 28, 2023

plbossart commented Aug 28, 2023

brentlu Aug 22, 2023 •

edited

Loading