| 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
| ||
|
Message-ID:
<TYZPR02MB7725C8E46B509527BD25AB24D8AAA@TYZPR02MB7725.apcprd02.prod.outlook.com>
Date: Tue, 16 Dec 2025 02:15:01 +0000
From: CM-Tsai Leo - 蔡紘紳 <leo.tsai@...dia.com.tw>
To: Takashi Iwai <tiwai@...e.de>, Leo Tsai <antivirus621@...il.com>
CC: "perex@...ex.cz" <perex@...ex.cz>, "tiwai@...e.com" <tiwai@...e.com>,
"rf@...nsource.cirrus.com" <rf@...nsource.cirrus.com>,
"linux-sound@...r.kernel.org" <linux-sound@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject:
回覆: [PATCH] ALSA: hda/cm9825: Add new project GENE_TWL7 for AAEON
Dear Takashi,
Sorry for missing these points.
Our validation team performs full functional testing before submit patch.
Our validation team did not reveal any issues after verification.
Forgot the version prefix in the subject, it is my bad, sorry.
This patch only adds support for the GENE_TWL7 model and does not include any fixes for existing models.
Fixed patch will be submitted separately.
> + {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},
Form our IC design team feedback, this is based on a customer hardware requirement that involves re-connection.
> +static void cm9825_unsol_linein_delayed(struct work_struct *work)
> +{
> + struct cmi_spec *spec =
> + container_of(to_delayed_work(work), struct cmi_spec,
> + unsol_linein_work);
> + struct hda_jack_tbl *jack;
> +
> + hda_nid_t linein_pin = spec->gen.autocfg.inputs[1].pin;
Mic-in doesn't need the delayed jack detection. We will fix the logic used here in the next revision.
> +static void linein_callback(struct hda_codec *codec,
> + struct hda_jack_callback *cb)
> +{
> + struct cmi_spec *spec = codec->spec;
> + struct hda_jack_tbl *tbl;
> +
> + /* Delay enabling the line, to let the linein-detection
> + * state machine run.
> + */
> +
> + codec_dbg(codec, "%s, cb->nid 0x%X, line%d\n", __func__,
> + (int)cb->nid, __LINE__);
> +
> + tbl = snd_hda_jack_tbl_get(codec, cb->nid);
> + if (tbl)
> + tbl->block_report = 1;
> + schedule_delayed_work(&spec->unsol_linein_work, msecs_to_jiffies(200));
> +}
We will try to integrate hp, line-out, line-in in the next revision.
> static int cm9825_init(struct hda_codec *codec)
> @@ -179,26 +378,61 @@ static int cm9825_init(struct hda_codec *codec)
> return 0;
> }
>
> -static void cm9825_remove(struct hda_codec *codec)
> +static void cm9825_cm_std_remove(struct hda_codec *codec)
> {
> struct cmi_spec *spec = codec->spec;
>
> cancel_delayed_work_sync(&spec->unsol_hp_work);
> +}
> +
> +static void cm9825_gene_twl7_remove(struct hda_codec *codec)
> +{
> + struct cmi_spec *spec = codec->spec;
> +
> + cancel_delayed_work_sync(&spec->unsol_linein_work);
> + cancel_delayed_work_sync(&spec->unsol_lineout_work);
We will add a check for the pin capabilities to decide whether to cancel.
> -static int cm9825_resume(struct hda_codec *codec)
> +static int cm9825_cm_std_resume(struct hda_codec *codec)
> {
> struct cmi_spec *spec = codec->spec;
> hda_nid_t hp_pin = 0;
> @@ -213,7 +447,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);
We confirmed that other vendors have the same behavior, and therefore we will temporarily restore it.
> @@ -258,6 +506,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;
> + cm9825_setup_unsol(codec);
Sorry, we will remove here's cm9825_setup_unsol().
> + break;
> + case QUIRK_GENE_TWL7_SSID:
> + snd_hda_codec_set_name(codec, "CM9825 GENE_TWL7");
> + INIT_DELAYED_WORK(&spec->unsol_linein_work,
> + cm9825_unsol_linein_delayed);
> + INIT_DELAYED_WORK(&spec->unsol_lineout_work,
> + cm9825_unsol_lineout_delayed);
> + 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);
Got it, we will add the comments as suggested.
Thanks,
Leo Tsai #853
________________________________________
寄件者: Takashi Iwai <tiwai@...e.de>
寄件日期: 2025年12月15日 下午 08:15
收件者: Leo Tsai <antivirus621@...il.com>
副本: perex@...ex.cz <perex@...ex.cz>; tiwai@...e.com <tiwai@...e.com>; rf@...nsource.cirrus.com <rf@...nsource.cirrus.com>; CM-Tsai Leo - 蔡紘紳 <leo.tsai@...dia.com.tw>; linux-sound@...r.kernel.org <linux-sound@...r.kernel.org>; linux-kernel@...r.kernel.org <linux-kernel@...r.kernel.org>
主旨: Re: [PATCH] ALSA: hda/cm9825: Add new project GENE_TWL7 for AAEON
On Mon, 15 Dec 2025 12:22:46 +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.
>
> Signed-off-by: Leo Tsai <antivirus621@...il.com>
It's good that you react quickly, and the patch looks much better
now. But don't rush; we have time.
Before resubmission, re-read your patch again and again. At each
change, you might have broken something else, too.
And, verify whether the patch really works as expected, as well as
verify it doesn't break other models.
Last but not least, please re-read what the previous reviews told.
You forgot the version prefix in the subject again, for example.
(And if there are fixes of the existing models, rather split it as
another fix patch as a preliminary.)
About the code changes:
> +/*
> + * Enable CLK/ADC to start recording.
> + */
> +static const struct hda_verb cm9825_gene_twl7_d0_verbs[] = {
> + {0x34, AC_VERB_SET_EAPD_BTLENABLE, 0x02},
In general, we set EAPD at init phase. It's OK to do it dynamically
here, though.
> + {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's the reason to tweak the node connections here?
Isn't it wired properly beforehand?
> +static void cm9825_unsol_linein_delayed(struct work_struct *work)
> +{
> + struct cmi_spec *spec =
> + container_of(to_delayed_work(work), struct cmi_spec,
> + unsol_linein_work);
> + struct hda_jack_tbl *jack;
> +
> + hda_nid_t linein_pin = spec->gen.autocfg.inputs[1].pin;
This blindly assumes that this second input is the line-in.
It means that the primary input is mic-in, and mic-in doesn't need the
delayed jack detection...?
> +static void linein_callback(struct hda_codec *codec,
> + struct hda_jack_callback *cb)
> +{
> + struct cmi_spec *spec = codec->spec;
> + struct hda_jack_tbl *tbl;
> +
> + /* Delay enabling the line, to let the linein-detection
> + * state machine run.
> + */
> +
> + codec_dbg(codec, "%s, cb->nid 0x%X, line%d\n", __func__,
> + (int)cb->nid, __LINE__);
> +
> + tbl = snd_hda_jack_tbl_get(codec, cb->nid);
> + if (tbl)
> + tbl->block_report = 1;
> + schedule_delayed_work(&spec->unsol_linein_work, msecs_to_jiffies(200));
> +}
Basically for all three (hp, line-out, line-in), the code itself is
identical except for the pin ID. They can be unified somehow better.
> 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];
>
> - snd_hda_jack_detect_enable_callback(codec, hp_pin, hp_callback);
> + hda_nid_t lineout_pin = spec->gen.autocfg.line_out_pins[0];
> +
> + hda_nid_t linein_pin = spec->gen.autocfg.inputs[1].pin;
Again, taking inputs[1] is flaky.
> static int cm9825_init(struct hda_codec *codec)
> @@ -179,26 +378,61 @@ static int cm9825_init(struct hda_codec *codec)
> return 0;
> }
>
> -static void cm9825_remove(struct hda_codec *codec)
> +static void cm9825_cm_std_remove(struct hda_codec *codec)
> {
> struct cmi_spec *spec = codec->spec;
>
> cancel_delayed_work_sync(&spec->unsol_hp_work);
> +}
> +
> +static void cm9825_gene_twl7_remove(struct hda_codec *codec)
> +{
> + struct cmi_spec *spec = codec->spec;
> +
> + cancel_delayed_work_sync(&spec->unsol_linein_work);
> + cancel_delayed_work_sync(&spec->unsol_lineout_work);
The initializations of those work rather depend on the pin
configuration, and strictly speaking, they aren't tied with the
models. e.g. user/BIOS may override the pin configurations.
That said, the cancel calls should be conditional for each work
instatiation. You can introduce some flags, for example.
> -static int cm9825_resume(struct hda_codec *codec)
> +static int cm9825_cm_std_resume(struct hda_codec *codec)
> {
> struct cmi_spec *spec = codec->spec;
> hda_nid_t hp_pin = 0;
> @@ -213,7 +447,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);
This really changes the behavior. Are you sure that you don't miss
anything? e.g. snd_hda_codec_init() calls snd_hda_gen_init(), which
will be lost by your patch.
> @@ -258,6 +506,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;
> + cm9825_setup_unsol(codec);
Why it's called here; cm9825_setup_unsol() is called later, no?
> + break;
> + case QUIRK_GENE_TWL7_SSID:
> + snd_hda_codec_set_name(codec, "CM9825 GENE_TWL7");
> + INIT_DELAYED_WORK(&spec->unsol_linein_work,
> + cm9825_unsol_linein_delayed);
> + INIT_DELAYED_WORK(&spec->unsol_lineout_work,
> + cm9825_unsol_lineout_delayed);
> + 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);
Don't put a magic number. I guess it's a mic pin? Define it properly
with a comment.
thanks,
Takashi
『The information contained in this email and attachment is confidential and is for the use of the intended recipient only. Any disclosure, copying or distribution of this email and attachment without the sender's consent is strictly prohibited. If you are not the intended recipient, please promptly notify the sender and delete this email and attachment entirely without using, retaining, or disclosing any of its contents. The recipient is responsible for ensuring that this email is virus free and the sender accepts no liability for any damages caused by virus transmitted by this email.』
Powered by blists - more mailing lists