| 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:
<TYZPR02MB7725DC0F673983A6D3CFDD1CD8ADA@TYZPR02MB7725.apcprd02.prod.outlook.com>
Date: Mon, 15 Dec 2025 03:26:25 +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 the patch we sent you was unclear.
AAEON is one of our customers, and GENE_TWL7 is a project of their team.
We confirmed with other customers a few days ago that only AAEON will not require further changes, so we decided to proceed with AAEON’s patch first.
> @@ -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 */
We found AC_AMP_SET_OUTPUT | AC_AMP_SET_RIGHT is not 0xa0 and AC_AMP_SET_OUTPUT | AC_AMP_SET_LEFT is not 0x90, so replaced the symbols with numbers.
Please let me know if there is a better way to express this.
> +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},
> + {}
> +};
> +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},
> + {}
> +};
> +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},
> + {}
> +};
The IC design team told us that these settings are related to the internal operation of the IC and are somewhat complex, so they only provided us with the intended purpose.
/*
* To save power, AD/CLK is turned off.
*/
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},
{}
};
/*
* 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},
{0x46, CM9825_VERB_SET_P3BCP, 0x20},
{}
};
/*
* Enable DAC to start playback.
*/
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},
{}
};
/*
* Disable DAC and enable de-pop noise mechanism.
*/
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},
{}
};
Above are the added comments. Please let me know if this is acceptable or not.
> 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;
In the GENE_TWL7 project, 0x36 pin widget is used as the line-out pin widget. We will add another pin widget specifically for line-out with a distinct name to avoid confusion with the headphone (HP) pin.
> 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);
Got it. We will distinguish them by using null and not-null values and we will figure out a better way to instead of check ssid at each place.
> +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);
This setting is used to enable support for the CTIA/OMTP interface, which was missing in the previous patch.
We will add this setting directly in the probe.
> +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;
> + }
Got it. We will set up the hook conditionally.
> 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);
> 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);
Got it, We will improve it.
> }
>
> 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;
Got it. We will modify it to return error.
Thanks,
Leo Tsai #853
________________________________________
寄件者: Takashi Iwai <tiwai@...e.de>
寄件日期: 2025年12月14日 下午 06:30
收件者: 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 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@...il.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
『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