Re: [RFC] sound: cs35l41: Add support for Legion 7 16ACHg6 laptop

On 10/13/21 5:15 PM, Takashi Iwai tiwai@suse.de wrote:

On Tue, 12 Oct 2021 22:22:07 +0200, Takashi Iwai wrote:

...

On Fri, 08 Oct 2021 13:19:02 +0200, Lucas Tanure wrote:

...

Hi,

I would like to get some guidance about this solution to support the 16ACHg6 laptop.

Hardware:

• The 16ACHg6 laptop has two CS35L41 amplifiers, connected

to Realtek ALC287 by an I2S bus and by and direct I2C to the CPU.

• The ALC287 codec is connected to the CPU by an HDA bus.
• The CS35L41 has a DSP which will require firmware to be loaded.

Architecture:

• To load the firmware for CS35L41, this solution will require

the wm_adsp library, which requires regmap, header definitions and register tables.

• To minimize the duplication of the code, the HDA functions will

be placed inside the ASoC CS35L41 driver.

• Finally, HDA patch_realtek will access exposed functions from

ASoC CS35L41 driver to initialize the amplifiers, start and stop streams and load firmware.

Through a very quick glance, a potential problem is that this design would make the HD-audio codec driver dependent on those other ASoC ones. As the Realtek HD-audio codec driver is used by quite many other people, we'd like to reduce such dependency mess.

Maybe a dynamic binding with component framework can be used?

Alternatively, we may build up a stuff on top of ASoC like what SOF driver did. It'll be another lot of work, though.

Or, yet another (and easier) alternative would be to create a new codec driver that is specific to vendor+subsystem pair. We'll need to extend the hda_device_id and its matching mechanism, and the realtek codec driver needs to exclude the matching with the given SSID explicitly.

A patch below is an example.

Takashi

---

diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h index ae2e75d15b21..5558f2ba2fcf 100644 --- a/include/linux/mod_devicetable.h +++ b/include/linux/mod_devicetable.h @@ -248,6 +248,7 @@ struct serio_device_id {

struct hda_device_id { __u32 vendor_id;

• __u32 subsystem_id; __u32 rev_id; __u8 api_version; const char *name;

diff --git a/include/sound/hda_codec.h b/include/sound/hda_codec.h index 0e45963bb767..3e316a798361 100644 --- a/include/sound/hda_codec.h +++ b/include/sound/hda_codec.h @@ -79,10 +79,12 @@ typedef int (*hda_codec_patch_t)(struct hda_codec *); #define HDA_CODEC_ID_GENERIC_HDMI 0x00000101 #define HDA_CODEC_ID_GENERIC 0x00000201

-#define HDA_CODEC_REV_ENTRY(_vid, _rev, _name, _patch) \

• { .vendor_id = (_vid), .rev_id = (_rev), .name = (_name), \

•  .api_version = HDA_DEV_LEGACY, \
  

+#define HDA_CODEC_FULL_ENTRY(_vid, _subsystem, _rev, _name, _patch) \

• { .vendor_id = (_vid), .subsystem_id = (_subsystem), .rev_id = (_rev), \

•  .name = (_name), .api_version = HDA_DEV_LEGACY, \
  

  .driver_data = (unsigned long)(_patch) }

+#define HDA_CODEC_REV_ENTRY(_vid, _rev, _name, _patch) \

• HDA_CODEC_FULL_ENTRY(_vid, 0, _rev, _name, _patch) #define HDA_CODEC_ENTRY(_vid, _name, _patch) \ HDA_CODEC_REV_ENTRY(_vid, 0, _name, _patch)

diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c index cc3625617a0e..641b4f9bb2be 100644 --- a/scripts/mod/devicetable-offsets.c +++ b/scripts/mod/devicetable-offsets.c @@ -211,6 +211,7 @@ int main(void)

DEVID(hda_device_id); DEVID_FIELD(hda_device_id, vendor_id);

• DEVID_FIELD(hda_device_id, subsystem_id); DEVID_FIELD(hda_device_id, rev_id); DEVID_FIELD(hda_device_id, api_version);

diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c index 49aba862073e..d8faf0065c95 100644 --- a/scripts/mod/file2alias.c +++ b/scripts/mod/file2alias.c @@ -1255,15 +1255,17 @@ static int do_ulpi_entry(const char *filename, void *symval, return 1; }

-/* Looks like: hdaudio:vNrNaN */ +/* Looks like: hdaudio:vNsNrNaN */ static int do_hda_entry(const char *filename, void *symval, char *alias) { DEF_FIELD(symval, hda_device_id, vendor_id);

• DEF_FIELD(symval, hda_device_id, subsystem_id); DEF_FIELD(symval, hda_device_id, rev_id); DEF_FIELD(symval, hda_device_id, api_version);

  strcpy(alias, "hdaudio:"); ADD(alias, "v", vendor_id != 0, vendor_id);

• ADD(alias, "s", subsystem_id != 0, subsystem_id); ADD(alias, "r", rev_id != 0, rev_id); ADD(alias, "a", api_version != 0, api_version);

diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c index 3e9e9ac804f6..662abd40ca6a 100644 --- a/sound/hda/hdac_device.c +++ b/sound/hda/hdac_device.c @@ -206,8 +206,9 @@ EXPORT_SYMBOL_GPL(snd_hdac_device_set_chip_name); */ int snd_hdac_codec_modalias(struct hdac_device *codec, char *buf, size_t size) {

• return scnprintf(buf, size, "hdaudio:v%08Xr%08Xa%02X\n",

• 	codec->vendor_id, codec->revision_id, codec->type);
  

• return scnprintf(buf, size, "hdaudio:v%08Xs%08Xr%08Xa%02X\n",

• 	 codec->vendor_id, codec->subsystem_id,
  

• 	 codec->revision_id, codec->type);
  

  } EXPORT_SYMBOL_GPL(snd_hdac_codec_modalias);

diff --git a/sound/pci/hda/Makefile b/sound/pci/hda/Makefile index b8fa682ce66a..9f559773bf99 100644 --- a/sound/pci/hda/Makefile +++ b/sound/pci/hda/Makefile @@ -15,6 +15,7 @@ CFLAGS_hda_intel.o := -I$(src)

snd-hda-codec-generic-objs := hda_generic.o snd-hda-codec-realtek-objs := patch_realtek.o +snd-hda-codec-test-objs := patch_test.o snd-hda-codec-cmedia-objs := patch_cmedia.o snd-hda-codec-analog-objs := patch_analog.o snd-hda-codec-idt-objs := patch_sigmatel.o @@ -32,7 +33,7 @@ obj-$(CONFIG_SND_HDA) := snd-hda-codec.o

# codec drivers obj-$(CONFIG_SND_HDA_GENERIC) += snd-hda-codec-generic.o -obj-$(CONFIG_SND_HDA_CODEC_REALTEK) += snd-hda-codec-realtek.o +obj-$(CONFIG_SND_HDA_CODEC_REALTEK) += snd-hda-codec-realtek.o snd-hda-codec-test.o obj-$(CONFIG_SND_HDA_CODEC_CMEDIA) += snd-hda-codec-cmedia.o obj-$(CONFIG_SND_HDA_CODEC_ANALOG) += snd-hda-codec-analog.o obj-$(CONFIG_SND_HDA_CODEC_SIGMATEL) += snd-hda-codec-idt.o diff --git a/sound/pci/hda/hda_bind.c b/sound/pci/hda/hda_bind.c index 1c8bffc3eec6..367f220ec91e 100644 --- a/sound/pci/hda/hda_bind.c +++ b/sound/pci/hda/hda_bind.c @@ -200,7 +200,7 @@ static inline bool codec_probed(struct hda_codec *codec) static void request_codec_module(struct hda_codec *codec) { #ifdef MODULE

• char modalias[32];

• char modalias[64]; const char *mod = NULL;

  switch (codec->probe_id) {

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c index 22d27b12c4e7..993b49554457 100644 --- a/sound/pci/hda/patch_realtek.c +++ b/sound/pci/hda/patch_realtek.c @@ -16,6 +16,7 @@ #include <linux/pci.h> #include <linux/dmi.h> #include <linux/module.h> +#include <linux/export.h> #include <linux/input.h> #include <linux/leds.h> #include <sound/core.h> @@ -9510,7 +9511,7 @@ static void alc269_fill_coef(struct hda_codec *codec)

/* */ -static int patch_alc269(struct hda_codec *codec) +int snd_hda_codec_realtek_alc269_probe(struct hda_codec *codec) { struct alc_spec *spec; int err; @@ -9667,6 +9668,9 @@ static int patch_alc269(struct hda_codec *codec)

alc_pre_init(codec);

• if (codec->fixup_id != HDA_FIXUP_ID_NOT_SET)

• goto skip_pick_fixup;
  

• snd_hda_pick_fixup(codec, alc269_fixup_models, alc269_fixup_tbl, alc269_fixups); /* FIXME: both TX300 and ROG Strix G17 have the same SSID, and

@@ -9683,6 +9687,8 @@ static int patch_alc269(struct hda_codec *codec) snd_hda_pick_pin_fixup(codec, alc269_fallback_pin_fixup_tbl, alc269_fixups, false); snd_hda_pick_fixup(codec, NULL, alc269_fixup_vendor_tbl, alc269_fixups);

• skip_pick_fixup: snd_hda_apply_fixup(codec, HDA_FIXUP_ACT_PRE_PROBE);

  alc_auto_parse_customize_define(codec);

@@ -9709,6 +9715,18 @@ static int patch_alc269(struct hda_codec *codec) alc_free(codec); return err; } +EXPORT_SYMBOL(snd_hda_codec_realtek_alc269_probe);

+static int patch_alc269(struct hda_codec *codec) +{

• if (codec->core.vendor_id == 0x10ec0298 &&

•    codec->core.subsystem_id == 0x102806e5) {
  

• pr_info("XXX realtek codec driver: skipping\n");
  

• return -ENODEV;
  

• }
• return snd_hda_codec_realtek_alc269_probe(codec);

+}

/*

• ALC861

diff --git a/sound/pci/hda/patch_test.c b/sound/pci/hda/patch_test.c new file mode 100644 index 000000000000..9070cc075af0 --- /dev/null +++ b/sound/pci/hda/patch_test.c @@ -0,0 +1,31 @@ +#include <linux/init.h> +#include <linux/module.h> +#include <sound/core.h> +#include <sound/hda_codec.h>

+int snd_hda_codec_realtek_alc269_probe(struct hda_codec *codec);

+// static const struct hda_fixup test_fixup = { ... };

+static int test_probe(struct hda_codec *codec) +{

• pr_info("XXX forked driver\n");
• // codec->fixup_id = 0;
• // codec->fixup_list = &test_fixup;
• return snd_hda_codec_realtek_alc269_probe(codec);

+}

+static const struct hda_device_id snd_hda_id_test[] = {

• HDA_CODEC_FULL_ENTRY(0x10ec0298, 0x102806e5, 0, test_probe),
• {} /* terminator */

+}; +MODULE_DEVICE_TABLE(hdaudio, snd_hda_id_test);

+MODULE_LICENSE("GPL"); +MODULE_DESCRIPTION("Test HD-audio codec");

+static struct hda_codec_driver test_driver = {

• .id = snd_hda_id_test,

+};

+module_hda_codec_driver(test_driver);

We need the Realtek for the I2S of cs35l41, the audio goes from CPU to Realtek then cs35l41. So we don't want to skip realtek initializations and functions. I am finishing a second version that I will send soon.

Thanks Lucas