Re: [PATCH] ALSA: hda/cm9825: Add new project GENE_TWL7 for AAEON

[Takashi Iwai <tiwai@suse.de>]

On Fri, 12 Dec 2025 10:31:57 +0100,
Leo Tsai wrote:
> 
> Update CM9825 to support GENE_TWL7 which supports Line-out, Line-in, and Mic-in.
> 
> Signed-off-by: Leo Tsai <antivirus621@gmail.com>

Thanks for the update.  But it still doesn't look good enough.

First off, you gave still too few descriptions.  What is GENE_TWL7 at
all?  And, why there are tons of code changes just for add the support
line-out, line-in and mic-in?  Those need more clarifications.

And, judging from the previous patch, I guess this is only a part of
the whole changes.  If more changes are planned, it's often worth to
mention it.

More about the code changes:

> @@ -80,10 +87,8 @@ static const struct hda_verb cm9825_std_d0_verbs[] = {
>  	{0x43, CM9825_VERB_SET_OCP, 0x33},	/* OTP set */
>  	{0x43, CM9825_VERB_SET_GAD, 0x07},	/* ADC -3db */
>  	{0x43, CM9825_VERB_SET_TMOD, 0x26},	/* Class D clk */
> -	{0x3C, AC_VERB_SET_AMP_GAIN_MUTE |
> -		AC_AMP_SET_OUTPUT | AC_AMP_SET_RIGHT, 0x2d},	/* Gain set */
> -	{0x3C, AC_VERB_SET_AMP_GAIN_MUTE |
> -		AC_AMP_SET_OUTPUT | AC_AMP_SET_LEFT, 0x2d},	/* Gain set */
> +	{0x3c, AC_VERB_SET_AMP_GAIN_MUTE | 0xa0, 0x2d},	/* Gain set */
> +	{0x3c, AC_VERB_SET_AMP_GAIN_MUTE | 0x90, 0x2d},	/* Gain set */

Why you replaced the symbols with numbers?  That worsens the code
readability.

> +static const struct hda_verb cm9825_gene_twl7_d3_verbs[] = {
> +	{0x43, CM9825_VERB_SET_D2S, 0x62},
> +	{0x43, CM9825_VERB_SET_PLL, 0x01},
> +	{0x43, CM9825_VERB_SET_NEG, 0xc2},
> +	{0x43, CM9825_VERB_SET_ADCL, 0x00},
> +	{0x43, CM9825_VERB_SET_DACL, 0x02},
> +	{0x43, CM9825_VERB_SET_MBIAS, 0x00},
> +	{0x43, CM9825_VERB_SET_VNEG, 0x50},
> +	{0x43, CM9825_VERB_SET_PDNEG, 0x04},
> +	{0x43, CM9825_VERB_SET_CDALR, 0xf6},
> +	{0x43, CM9825_VERB_SET_OTP, 0xcd},
> +	{}
> +};

What does those verbs do?

> +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},
> +	{0x46, CM9825_VERB_SET_P3BCP, 0x20},
> +	{}
> +};

Ditto.

> +static const struct hda_verb cm9825_gene_twl7_playback_start_verbs[] = {
> +	{0x43, CM9825_VERB_SET_D2S, 0xf2},
> +	{0x43, CM9825_VERB_SET_VDO, 0xd4},
> +	{0x43, CM9825_VERB_SET_SNR, 0x30},
> +	{}
> +};
> +
> +static const struct hda_verb cm9825_gene_twl7_playback_stop_verbs[] = {
> +	{0x43, CM9825_VERB_SET_VDO, 0xc0},
> +	{0x43, CM9825_VERB_SET_D2S, 0x62},
> +	{0x43, CM9825_VERB_SET_VDO, 0xd0},
> +	{0x43, CM9825_VERB_SET_SNR, 0x38},
> +	{}
> +};

... and those, too.  Please give comments what those verbs do and why
they are needed explicitly.

>  static void cm9825_unsol_hp_delayed(struct work_struct *work)
>  {
>  	struct cmi_spec *spec =
> @@ -120,6 +179,9 @@ static void cm9825_unsol_hp_delayed(struct work_struct *work)
>  	bool hp_jack_plugin = false;
>  	int err = 0;
>  
> +	if (spec->codec->core.subsystem_id == QUIRK_GENE_TWL7_SSID)
> +		hp_pin = 0x36;

Is it 100% sure that the headphone pin is fixed only to this node?
And why this pin isn't assigned to the autocfg.hp_pins[0] at the first
place?  Is BIOS broken?

>  	hp_jack_plugin = snd_hda_jack_detect(spec->codec, hp_pin);
>  
>  	codec_dbg(spec->codec, "hp_jack_plugin %d, hp_pin 0x%X\n",
> @@ -132,10 +194,13 @@ static void cm9825_unsol_hp_delayed(struct work_struct *work)
>  		if (err)
>  			codec_dbg(spec->codec, "codec_write err %d\n", err);
>  
> -		snd_hda_sequence_write(spec->codec, spec->chip_hp_remove_verbs);
> +		if (spec->codec->core.subsystem_id == QUIRK_CM_STD)
> +			snd_hda_sequence_write(spec->codec,
> +					       spec->chip_hp_remove_verbs);
>  	} else {
> -		snd_hda_sequence_write(spec->codec,
> -				       spec->chip_hp_present_verbs);
> +		if (spec->codec->core.subsystem_id == QUIRK_CM_STD)
> +			snd_hda_sequence_write(spec->codec,
> +					       spec->chip_hp_present_verbs);

The check of subsystem_id is too ugly here and there.
If this is a conditional setup, set spec->chip_hp_present_verbs to
non-NULL only for the required model, and just check NULL instead of
subsystem_id.

> +static void cm9825_unsol_line_delayed(struct work_struct *work)
> +{
> +	struct cmi_spec *spec =
> +	    container_of(to_delayed_work(work), struct cmi_spec,
> +			 unsol_line_work);
> +	struct hda_jack_tbl *jack;
> +	hda_nid_t lineout_pin = 0x3b;

Again, a fixed node ID.  That's a very bad way.

>  static void cm9825_setup_unsol(struct hda_codec *codec)
>  {
>  	struct cmi_spec *spec = codec->spec;
>  
>  	hda_nid_t hp_pin = spec->gen.autocfg.hp_pins[0];
>  
> +	if (codec->core.subsystem_id == QUIRK_GENE_TWL7_SSID)
> +		hp_pin = 0x36;

Here again....

>  	snd_hda_jack_detect_enable_callback(codec, hp_pin, hp_callback);
> +
> +	if (codec->core.subsystem_id == QUIRK_GENE_TWL7_SSID) {

This could be in better way instead of checking subsystem_id at each
place.  It's error-prone.

> +		hda_nid_t linein_pin = 0x3b;

Here...

> +static void cm9825_init_hook(struct hda_codec *codec)
> +{
> +	unsigned int val;
> +
> +	codec_dbg(codec, "init hook\n");
> +
> +	/* OMTP */
> +	val = snd_hda_codec_read(codec, 0x46, 0, 0xfec, 0x0);
> +	snd_hda_codec_write(codec, 0x46, 0, 0x7ef, (val >> 24) & 0x7f);

What does this more exactly?
And this sequence wasn't present for STD model before the patch.
Is it safe to apply unconditionally?

> +	// link reset
> +	if (codec->core.subsystem_id == QUIRK_GENE_TWL7_SSID)
> +		snd_hda_sequence_write(codec, cm9825_gene_twl7_d0_verbs);

Need more comment.  Also, you should reconsider whether subsystem_id
check is the best way here, too.


> +static void cm9825_playback_pcm_hook(struct hda_pcm_stream *hinfo,
> +				     struct hda_codec *codec,
> +				     struct snd_pcm_substream *substream,
> +				     int action)
> +{
> +	struct cmi_spec *spec = codec->spec;
> +
> +	switch (action) {
> +	case HDA_GEN_PCM_ACT_PREPARE:
> +		if (codec->core.subsystem_id == QUIRK_GENE_TWL7_SSID) {
> +			snd_hda_sequence_write(spec->codec,
> +					       cm9825_gene_twl7_playback_start_verbs);
> +		}
> +		break;
> +	case HDA_GEN_PCM_ACT_CLEANUP:
> +		if (codec->core.subsystem_id == QUIRK_GENE_TWL7_SSID) {
> +			snd_hda_sequence_write(spec->codec,
> +					       cm9825_gene_twl7_playback_stop_verbs);
> +		}
> +		break;
> +	default:
> +		return;
> +	}

If it's model-specific, rather set up the hook conditionally, instead
of checking subsystem_id in the callback.

>  static int cm9825_init(struct hda_codec *codec)
> @@ -184,6 +340,7 @@ static void cm9825_remove(struct hda_codec *codec)
>  	struct cmi_spec *spec = codec->spec;
>  
>  	cancel_delayed_work_sync(&spec->unsol_hp_work);
> +	cancel_delayed_work_sync(&spec->unsol_line_work);

You're calling here unconditionally, while...

>  	snd_hda_gen_remove(codec);
>  }
>  
> @@ -195,6 +352,9 @@ static int cm9825_suspend(struct hda_codec *codec)
>  
>  	snd_hda_sequence_write(codec, spec->chip_d3_verbs);
>  
> +	if (codec->core.subsystem_id == QUIRK_GENE_TWL7_SSID)
> +		cancel_delayed_work_sync(&spec->unsol_line_work);

... here conditionally.  And I believe your patch breaks STD model
when the module is unloaded, because the work isn't initialized but
calling cancel_delayed_work_sync() in the above.

And, again, the conditional call with the subsystem_id check is
error-prone.
> @@ -213,7 +373,7 @@ static int cm9825_resume(struct hda_codec *codec)
>  
>  	msleep(150);		/* for depop noise */
>  
> -	snd_hda_codec_init(codec);
> +	snd_hda_sequence_write(codec, spec->chip_d0_verbs);

Why is this change...?  It'll certainly break other things.


>  	hp_pin = spec->gen.autocfg.hp_pins[0];
>  	hp_jack_plugin = snd_hda_jack_detect(spec->codec, hp_pin);
> @@ -229,7 +389,9 @@ static int cm9825_resume(struct hda_codec *codec)
>  		if (err)
>  			codec_dbg(codec, "codec_write err %d\n", err);
>  
> -		snd_hda_sequence_write(codec, cm9825_hp_remove_verbs);
> +		if (codec->core.subsystem_id == QUIRK_CM_STD)
> +			snd_hda_sequence_write(codec,
> +					       spec->chip_hp_remove_verbs);

Here again ugly subystem_id conditional...

>  	}
>  
>  	snd_hda_regmap_sync(codec);
> @@ -248,9 +410,13 @@ static int cm9825_probe(struct hda_codec *codec, const struct hda_device_id *id)
>  	if (spec == NULL)
>  		return -ENOMEM;
>  
> +	codec_dbg(codec, "chip_name: %s, ssid: 0x%X\n",
> +		   codec->core.chip_name, codec->core.subsystem_id);
> +
>  	INIT_DELAYED_WORK(&spec->unsol_hp_work, cm9825_unsol_hp_delayed);
>  	codec->spec = spec;
>  	spec->codec = codec;
> +	spec->gen.init_hook = cm9825_init_hook;
>  	cfg = &spec->gen.autocfg;
>  	snd_hda_gen_spec_init(&spec->gen);
>  	spec->chip_d0_verbs = cm9825_std_d0_verbs;
> @@ -258,6 +424,41 @@ 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");
> +		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_line_work,
> +				  cm9825_unsol_line_delayed);
> +		spec->gen.hp_mic = 0;
> +		cfg->line_outs = 1;
> +		cfg->line_out_pins[0] = 0x36;
> +		cfg->line_out_type = AUTO_PIN_LINE_OUT;
> +		cfg->num_inputs = 2;
> +		cfg->inputs[0].pin = 0x3b;
> +		cfg->inputs[0].type = AUTO_PIN_LINE_IN;
> +		cfg->inputs[1].pin = 0x37;
> +		cfg->inputs[1].type = AUTO_PIN_MIC;
> +		cfg->inputs[1].is_headphone_mic = 1;
> +		spec->chip_d0_verbs = cm9825_gene_twl7_d0_verbs;
> +		spec->chip_d3_verbs = cm9825_gene_twl7_d3_verbs;
> +		spec->gen.pcm_playback_hook = cm9825_playback_pcm_hook;
> +		snd_hda_codec_set_pincfg(codec, 0x37, 0x24A70100);
> +		break;
> +	default:
> +		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;

The default should be QUIRK_CM_STD, or handle it as an error.  There
is no reason to define differently for default.


thanks,

Takashi