linux-kernel - Re: [PATCH] [PATCH v5] ALSA: hda/cm9825: Add GENE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <875xa6fxie.wl-tiwai@suse.de>
Date: Tue, 16 Dec 2025 12:59:21 +0100
From: Takashi Iwai <tiwai@...e.de>
To: Leo Tsai <antivirus621@...il.com>
Cc: perex@...ex.cz,
	tiwai@...e.com,
	rf@...nsource.cirrus.com,
	leo.tsai@...dia.com.tw,
	linux-sound@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] [PATCH v5] ALSA: hda/cm9825: Add GENE_TWL7 support for AAEON

On Tue, 16 Dec 2025 09:01:48 +0100,
Leo Tsai wrote:
> 
> The GENE_TWL7 project is an AAEON platform with a fixed audio
> configuration consisting of line-out, line-in, and mic-in.
> The audio routing and pin assignments are defined according to
> the board-level hardware design and are not intended to be
> dynamically changed.

Now it's getting butter, but you can give a bit more details, too.
e.g. mention about the workarounds and the extra code that are added
for this model, and explain why they are needed and how they work.

> +/*
> + * Enable CLK/ADC to start recording.
> + */
> +static const struct hda_verb cm9825_gene_twl7_d0_verbs[] = {
> +	{0x34, AC_VERB_SET_EAPD_BTLENABLE, 0x02},
> +	{0x43, CM9825_VERB_SET_SNR, 0x38},
> +	{0x43, CM9825_VERB_SET_PLL, 0x00},
> +	{0x43, CM9825_VERB_SET_ADCL, 0xcf},
> +	{0x43, CM9825_VERB_SET_DACL, 0xaa},
> +	{0x43, CM9825_VERB_SET_MBIAS, 0x1c},
> +	{0x43, CM9825_VERB_SET_VNEG, 0x56},
> +	{0x43, CM9825_VERB_SET_D2S, 0x62},
> +	{0x43, CM9825_VERB_SET_DACTRL, 0x00},
> +	{0x43, CM9825_VERB_SET_PDNEG, 0x0c},
> +	{0x43, CM9825_VERB_SET_CDALR, 0xf4},
> +	{0x43, CM9825_VERB_SET_OTP, 0xcd},
> +	{0x43, CM9825_VERB_SET_MTCBA, 0x61},
> +	{0x43, CM9825_VERB_SET_OCP, 0x33},
> +	{0x43, CM9825_VERB_SET_GAD, 0x07},
> +	{0x43, CM9825_VERB_SET_TMOD, 0x26},
> +	{0x43, CM9825_VERB_SET_HPF_1, 0x40},
> +	{0x43, CM9825_VERB_SET_HPF_2, 0x40},
> +	{0x40, AC_VERB_SET_CONNECT_SEL, 0x00},
> +	{0x3d, AC_VERB_SET_CONNECT_SEL, 0x01},

What do these connections do?  IOW, why it wouldn't work without those
explicit setups?

> +
> +static bool cm9825_jd_cap(struct hda_codec *codec, hda_nid_t pin_id)
> +{
> +	int cap;
> +
> +	cap =
> +	    snd_hdac_read_parm(&codec->core, pin_id,
> +			       AC_PAR_PIN_CAP) & AC_PINCAP_PRES_DETECT;
> +
> +	codec_dbg(codec, "%s, cap 0x%X, line%d\n", __func__, cap, __LINE__);
> +
> +	return (bool)cap;
> +}

I guess you can use the existing is_jack_detectable() function.
It covers more tests.

And, instead of bool, you can keep the pin id in jd_cap_hp, and co.
Then, cm9825_unsol_hp_delayed() can be more simplified like:

	if (cb->nid == spec->jd_cap_hp)
		schedule_delayed_work(&spec->unsol_hp_work,
				      msecs_to_jiffies(200));
	else if (cb->nid == spec->jp_cap_lineout)
		schedule_delayed_work(&spec->unsol_lineout_work,
				      msecs_to_jiffies(200));
	....


> +static void cm9825_unsol_inputs_delayed(struct work_struct *work)
> +{
> +	struct cmi_spec *spec =
> +	    container_of(to_delayed_work(work), struct cmi_spec,
> +			 unsol_inputs_work);
> +	struct hda_jack_tbl *jack;
> +	int i;
> +	bool input_jack_plugin;
> +
> +	for (i = 0; i < spec->gen.autocfg.num_inputs; i++) {
> +		if (!spec->jd_cap_inputs[i])
> +			continue;
> +
> +		input_jack_plugin =
> +		    snd_hda_jack_detect(spec->codec,
> +					spec->gen.autocfg.inputs[i].pin);
> +		jack =
> +		    snd_hda_jack_tbl_get(spec->codec,
> +					 spec->gen.autocfg.inputs[i].pin);
> +		if (jack) {
> +			jack->block_report = 0;
> +			snd_hda_jack_report_sync(spec->codec);
> +		}
> +
> +		codec_dbg(spec->codec,
> +			  "%s, input_jack_plugin %d, linein_pin 0x%X, line%d\n",
> +			  __func__, (int)input_jack_plugin,
> +			  spec->gen.autocfg.inputs[i].pin, __LINE__);
> +	}

This code pattern

	jack_plugin = snd_hda_jack_detect(codec, pin);
	jack = snd_hda_jack_tbl_get(codec, pin);
	if (jack) {
		jack->block_report = 0;
		snd_hda_jack_report_sync(codec);
	}
	codec_dbug(codec, ....);
	
is used in multiple places in your patch, so it can be a common
function.

> @@ -242,13 +504,15 @@ static int cm9825_probe(struct hda_codec *codec, const struct hda_device_id *id)
>  {
>  	struct cmi_spec *spec;
>  	struct auto_pin_cfg *cfg;
> -	int err;
> +	int err = 0;
>  
>  	spec = kzalloc(sizeof(*spec), GFP_KERNEL);
>  	if (spec == NULL)
>  		return -ENOMEM;
>  
> -	INIT_DELAYED_WORK(&spec->unsol_hp_work, cm9825_unsol_hp_delayed);
> +	codec_dbg(codec, "chip_name: %s, ssid: 0x%X\n",
> +		  codec->core.chip_name, codec->core.subsystem_id);
> +
>  	codec->spec = spec;
>  	spec->codec = codec;
>  	cfg = &spec->gen.autocfg;
> @@ -258,6 +522,36 @@ static int cm9825_probe(struct hda_codec *codec, const struct hda_device_id *id)
>  	spec->chip_hp_present_verbs = cm9825_hp_present_verbs;
>  	spec->chip_hp_remove_verbs = cm9825_hp_remove_verbs;
>  
> +	switch (codec->core.subsystem_id) {
> +	case QUIRK_CM_STD:
> +		snd_hda_codec_set_name(codec, "CM9825 STD");
> +		INIT_DELAYED_WORK(&spec->unsol_hp_work,
> +				  cm9825_unsol_hp_delayed);
> +		spec->chip_d0_verbs = cm9825_std_d0_verbs;
> +		spec->chip_d3_verbs = cm9825_std_d3_verbs;
> +		spec->chip_hp_present_verbs = cm9825_hp_present_verbs;
> +		spec->chip_hp_remove_verbs = cm9825_hp_remove_verbs;
> +		break;
> +	case QUIRK_GENE_TWL7_SSID:
> +		snd_hda_codec_set_name(codec, "CM9825 GENE_TWL7");
> +		INIT_DELAYED_WORK(&spec->unsol_inputs_work,
> +				  cm9825_unsol_inputs_delayed);
> +		INIT_DELAYED_WORK(&spec->unsol_lineout_work,
> +				  cm9825_unsol_lineout_delayed);

Basically all those delayed works can be initialized unconditionally.
It's safer than leaving as uninitialized.


thanks,

Takashi