[PATCH] Make top/rear speaker, mute and micmute leds work on HP x360 14-ea000 laptops that use Realtek 245 codec
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=210633 Signed-off-by: Jonathan Clarke jonathan.a.clarke@gmail.com --- sound/pci/hda/patch_realtek.c | 46 +++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)
diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c index 22d27b12c..e3c6d17ea 100644 --- a/sound/pci/hda/patch_realtek.c +++ b/sound/pci/hda/patch_realtek.c @@ -4409,6 +4409,13 @@ static void alc245_fixup_hp_x360_amp(struct hda_codec *codec, case HDA_FIXUP_ACT_PRE_PROBE: spec->gpio_mask |= 0x01; spec->gpio_dir |= 0x01; + + /* use only amp at 0x02 for bottom(front) speaker, + * otherwise it is set to use 0x02,0x03,0x06 and when used in conjunction + * with top(rear) speaker 0x14, gets locked at full volume */ + static const hda_nid_t conn1[] = { 0x02 }; + snd_hda_override_conn_list(codec, 0x17, ARRAY_SIZE(conn1), conn1); + break; case HDA_FIXUP_ACT_INIT: /* need to toggle GPIO to enable the amp */ @@ -4503,6 +4510,26 @@ static void alc236_fixup_hp_mute_led_coefbit(struct hda_codec *codec, } }
+static void alc245_fixup_hp_x360_mute_leds(struct hda_codec *codec, + const struct hda_fixup *fix, int action) +{ + struct alc_spec *spec = codec->spec; + if (action == HDA_FIXUP_ACT_PRE_PROBE) { + /* mic mute is set via gpio 0x04 */ + spec->micmute_led_polarity = 1; + codec->power_filter = led_power_filter; + alc_fixup_hp_gpio_led(codec, action, 0x00, 0x04); + + /* output mute is set via SET_COEF_INDEX,SET_PROC_COEF */ + spec->mute_led_polarity = 0; + spec->mute_led_coef.idx = 0x0b; + spec->mute_led_coef.mask = 0xffff; + spec->mute_led_coef.on = 0xa02f; + spec->mute_led_coef.off = 0x7774; + snd_hda_gen_add_mute_led_cdev(codec, coef_mute_led_set); + } +} + /* turn on/off mic-mute LED per capture hook by coef bit */ static int coef_micmute_led_set(struct led_classdev *led_cdev, enum led_brightness brightness) @@ -6557,6 +6584,8 @@ enum { ALC269_FIXUP_HP_DOCK_GPIO_MIC1_LED, ALC280_FIXUP_HP_9480M, ALC245_FIXUP_HP_X360_AMP, + ALC245_FIXUP_HP_X360_MUTE_LEDS, + ALC245_FIXUP_HP_X360_GPIO_TOP_SPEAKER, ALC288_FIXUP_DELL_HEADSET_MODE, ALC288_FIXUP_DELL1_MIC_NO_PRESENCE, ALC288_FIXUP_DELL_XPS_13, @@ -7293,6 +7322,21 @@ static const struct hda_fixup alc269_fixups[] = { [ALC245_FIXUP_HP_X360_AMP] = { .type = HDA_FIXUP_FUNC, .v.func = alc245_fixup_hp_x360_amp, + .chained = true, + .chain_id = ALC245_FIXUP_HP_X360_MUTE_LEDS + }, + [ALC245_FIXUP_HP_X360_MUTE_LEDS] = { + .type = HDA_FIXUP_FUNC, + .v.func = alc245_fixup_hp_x360_mute_leds, + .chained = true, + .chain_id = ALC245_FIXUP_HP_X360_GPIO_TOP_SPEAKER + }, + [ALC245_FIXUP_HP_X360_GPIO_TOP_SPEAKER] = { + .type = HDA_FIXUP_PINS, + .v.pins = (const struct hda_pintbl[]) { + { 0x14, 0x90170110 }, /* enable top(back) speaker in addition to bottom(front) speaker at 0x17 */ + { }, + } }, [ALC288_FIXUP_DELL_HEADSET_MODE] = { .type = HDA_FIXUP_FUNC, @@ -9003,6 +9047,8 @@ static const struct hda_model_fixup alc269_fixup_models[] = { {.id = ALC255_FIXUP_XIAOMI_HEADSET_MIC, .name = "alc255-xiaomi-headset"}, {.id = ALC274_FIXUP_HP_MIC, .name = "alc274-hp-mic-detect"}, {.id = ALC245_FIXUP_HP_X360_AMP, .name = "alc245-hp-x360-amp"}, + {.id = ALC245_FIXUP_HP_X360_MUTE_LEDS, .name = "alc245-hp-x360-mute-leds"}, + {.id = ALC245_FIXUP_HP_X360_GPIO_TOP_SPEAKER, .name = "alc245-hp-x360-gpio-top-speaker"}, {.id = ALC295_FIXUP_HP_OMEN, .name = "alc295-hp-omen"}, {.id = ALC285_FIXUP_HP_SPECTRE_X360, .name = "alc285-hp-spectre-x360"}, {.id = ALC287_FIXUP_IDEAPAD_BASS_SPK_AMP, .name = "alc287-ideapad-bass-spk-amp"},
On Fri, 29 Oct 2021 17:43:13 +0200, Jonathan Clarke wrote:
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=210633 Signed-off-by: Jonathan Clarke jonathan.a.clarke@gmail.com
Could you give more descriptions? The patch isn't trivial at all, and it needs more explanations.
sound/pci/hda/patch_realtek.c | 46 +++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)
diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c index 22d27b12c..e3c6d17ea 100644 --- a/sound/pci/hda/patch_realtek.c +++ b/sound/pci/hda/patch_realtek.c @@ -4409,6 +4409,13 @@ static void alc245_fixup_hp_x360_amp(struct hda_codec *codec, case HDA_FIXUP_ACT_PRE_PROBE: spec->gpio_mask |= 0x01; spec->gpio_dir |= 0x01;
/* use only amp at 0x02 for bottom(front) speaker,* otherwise it is set to use 0x02,0x03,0x06 and when used in conjunction* with top(rear) speaker 0x14, gets locked at full volume */static const hda_nid_t conn1[] = { 0x02 };snd_hda_override_conn_list(codec, 0x17, ARRAY_SIZE(conn1), conn1);- break; case HDA_FIXUP_ACT_INIT: /* need to toggle GPIO to enable the amp */
@@ -4503,6 +4510,26 @@ static void alc236_fixup_hp_mute_led_coefbit(struct hda_codec *codec, } }
+static void alc245_fixup_hp_x360_mute_leds(struct hda_codec *codec,
const struct hda_fixup *fix, int action)+{
- struct alc_spec *spec = codec->spec;
- if (action == HDA_FIXUP_ACT_PRE_PROBE) {
/* mic mute is set via gpio 0x04 */spec->micmute_led_polarity = 1;codec->power_filter = led_power_filter;alc_fixup_hp_gpio_led(codec, action, 0x00, 0x04);/* output mute is set via SET_COEF_INDEX,SET_PROC_COEF */spec->mute_led_polarity = 0;spec->mute_led_coef.idx = 0x0b;spec->mute_led_coef.mask = 0xffff;spec->mute_led_coef.on = 0xa02f;spec->mute_led_coef.off = 0x7774;snd_hda_gen_add_mute_led_cdev(codec, coef_mute_led_set);
I guess this COEF isn't only about mute-LED but actually does mute the output? IIRC, the bit 0x08 corresponds to the LED. If so, it's better to split. Basically this snd_hda_gen_add_mute_led_cdev() and mute_led_coef stuff are only for the mute LED control. e.g. you can change the mute LED independently via sysfs.
thanks,
Takashi
Thanks for taking a look at this patch so quickly, Takashi.
On Sat, Oct 30, 2021 at 12:01:03PM +0200, Takashi Iwai wrote:
Could you give more descriptions? The patch isn't trivial at all, and it needs more explanations.
Yes, will do.
/* output mute is set via SET_COEF_INDEX,SET_PROC_COEF */spec->mute_led_polarity = 0;spec->mute_led_coef.idx = 0x0b;spec->mute_led_coef.mask = 0xffff;spec->mute_led_coef.on = 0xa02f;spec->mute_led_coef.off = 0x7774;snd_hda_gen_add_mute_led_cdev(codec, coef_mute_led_set);I guess this COEF isn't only about mute-LED but actually does mute the output? IIRC, the bit 0x08 corresponds to the LED. If so, it's better to split. Basically this snd_hda_gen_add_mute_led_cdev() and mute_led_coef stuff are only for the mute LED control. e.g. you can change the mute LED independently via sysfs.
Thanks for suggesting this.
Having tested, I can confirm that setting this coef only affects the output mute LED, and does not affect output.
I will therefore assume that current implementation in my patch is OK, but let me know if it still needs changing (maybe I've misunderstood).
For reference to other users, the commands to test are: # output LED on hda-verb /dev/snd/hwC0D0 0x20 SET_COEF_INDEX 0x0b hda-verb /dev/snd/hwC0D0 0x20 SET_PROC_COEF 0xa02f
# output LED off hda-verb /dev/snd/hwC0D0 0x20 SET_COEF_INDEX 0x0b hda-verb /dev/snd/hwC0D0 0x20 SET_PROC_COEF 0x7774
Many thanks, Jonathan
On Mon, 01 Nov 2021 11:34:45 +0100, Jonathan Clarke wrote:
Thanks for taking a look at this patch so quickly, Takashi.
On Sat, Oct 30, 2021 at 12:01:03PM +0200, Takashi Iwai wrote:
Could you give more descriptions? The patch isn't trivial at all, and it needs more explanations.
Yes, will do.
/* output mute is set via SET_COEF_INDEX,SET_PROC_COEF */spec->mute_led_polarity = 0;spec->mute_led_coef.idx = 0x0b;spec->mute_led_coef.mask = 0xffff;spec->mute_led_coef.on = 0xa02f;spec->mute_led_coef.off = 0x7774;snd_hda_gen_add_mute_led_cdev(codec, coef_mute_led_set);I guess this COEF isn't only about mute-LED but actually does mute the output? IIRC, the bit 0x08 corresponds to the LED. If so, it's better to split. Basically this snd_hda_gen_add_mute_led_cdev() and mute_led_coef stuff are only for the mute LED control. e.g. you can change the mute LED independently via sysfs.
Thanks for suggesting this.
Having tested, I can confirm that setting this coef only affects the output mute LED, and does not affect output.
I will therefore assume that current implementation in my patch is OK, but let me know if it still needs changing (maybe I've misunderstood).
For reference to other users, the commands to test are: # output LED on hda-verb /dev/snd/hwC0D0 0x20 SET_COEF_INDEX 0x0b hda-verb /dev/snd/hwC0D0 0x20 SET_PROC_COEF 0xa02f
# output LED off hda-verb /dev/snd/hwC0D0 0x20 SET_COEF_INDEX 0x0b hda-verb /dev/snd/hwC0D0 0x20 SET_PROC_COEF 0x7774
Could you try just to flip the bit 0x08? At LED off state?
% hda-verb /dev/snd/hwC0D0 0x20 SET_COEF_INDEX 0x0b % hda-verb /dev/snd/hwC0D0 0x20 SET_PROC_COEF 0x77f4
That is, the implementation in alc286_fixup_hp_mute_led_coefbit(), which is used by many other HP laptops.
Takashi
On Tue, Nov 02, 2021 at 09:19:25AM +0100, Takashi Iwai wrote:
Could you try just to flip the bit 0x08? At LED off state?
% hda-verb /dev/snd/hwC0D0 0x20 SET_COEF_INDEX 0x0b % hda-verb /dev/snd/hwC0D0 0x20 SET_PROC_COEF 0x77f4
That is, the implementation in alc286_fixup_hp_mute_led_coefbit(), which is used by many other HP laptops.
Ah, I see what you mean.
Yes, you're right, changing from 0x77f4 to 0x77fc makes the mute led light up. The changes to the other proc_coef bits in my patch are not needed.
I will revise and revert in next few days. Thanks
Hi Jonathan,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tiwai-sound/for-next] [cannot apply to v5.15-rc7 next-20211029] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Jonathan-Clarke/Make-top-rear-speak... base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next config: nds32-randconfig-r033-20211031 (attached as .config) compiler: nds32le-linux-gcc (GCC) 11.2.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/a4b221979b51a8d3256056869efc55d4dc80... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Jonathan-Clarke/Make-top-rear-speaker-mute-and-micmute-leds-work-on-HP-x360-14-ea000-laptops-that-use-Realtek-245-codec/20211029-234508 git checkout a4b221979b51a8d3256056869efc55d4dc80a4ad # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=nds32
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All warnings (new ones prefixed by >>):
sound/pci/hda/patch_realtek.c: In function 'alc245_fixup_hp_x360_amp':
sound/pci/hda/patch_realtek.c:4417:17: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
4417 | static const hda_nid_t conn1[] = { 0x02 }; | ^~~~~~
vim +4417 sound/pci/hda/patch_realtek.c
4400 4401 /* HP Spectre x360 14 model needs a unique workaround for enabling the amp; 4402 * it needs to toggle the GPIO0 once on and off at each time (bko#210633) 4403 */ 4404 static void alc245_fixup_hp_x360_amp(struct hda_codec *codec, 4405 const struct hda_fixup *fix, int action) 4406 { 4407 struct alc_spec *spec = codec->spec; 4408 4409 switch (action) { 4410 case HDA_FIXUP_ACT_PRE_PROBE: 4411 spec->gpio_mask |= 0x01; 4412 spec->gpio_dir |= 0x01; 4413 4414 /* use only amp at 0x02 for bottom(front) speaker, 4415 * otherwise it is set to use 0x02,0x03,0x06 and when used in conjunction 4416 * with top(rear) speaker 0x14, gets locked at full volume */
4417 static const hda_nid_t conn1[] = { 0x02 };
4418 snd_hda_override_conn_list(codec, 0x17, ARRAY_SIZE(conn1), conn1); 4419 4420 break; 4421 case HDA_FIXUP_ACT_INIT: 4422 /* need to toggle GPIO to enable the amp */ 4423 alc_update_gpio_data(codec, 0x01, true); 4424 msleep(100); 4425 alc_update_gpio_data(codec, 0x01, false); 4426 break; 4427 } 4428 } 4429
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Jonathan,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on tiwai-sound/for-next] [cannot apply to v5.15-rc7 next-20211029] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Jonathan-Clarke/Make-top-rear-speak... base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next config: x86_64-allyesconfig (attached as .config) compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 reproduce (this is a W=1 build): # https://github.com/0day-ci/linux/commit/a4b221979b51a8d3256056869efc55d4dc80... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Jonathan-Clarke/Make-top-rear-speaker-mute-and-micmute-leds-work-on-HP-x360-14-ea000-laptops-that-use-Realtek-245-codec/20211029-234508 git checkout a4b221979b51a8d3256056869efc55d4dc80a4ad # save the attached .config to linux build tree make W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All errors (new ones prefixed by >>):
sound/pci/hda/patch_realtek.c: In function 'alc245_fixup_hp_x360_amp':
sound/pci/hda/patch_realtek.c:4417:3: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
4417 | static const hda_nid_t conn1[] = { 0x02 }; | ^~~~~~ cc1: all warnings being treated as errors
vim +4417 sound/pci/hda/patch_realtek.c
4400 4401 /* HP Spectre x360 14 model needs a unique workaround for enabling the amp; 4402 * it needs to toggle the GPIO0 once on and off at each time (bko#210633) 4403 */ 4404 static void alc245_fixup_hp_x360_amp(struct hda_codec *codec, 4405 const struct hda_fixup *fix, int action) 4406 { 4407 struct alc_spec *spec = codec->spec; 4408 4409 switch (action) { 4410 case HDA_FIXUP_ACT_PRE_PROBE: 4411 spec->gpio_mask |= 0x01; 4412 spec->gpio_dir |= 0x01; 4413 4414 /* use only amp at 0x02 for bottom(front) speaker, 4415 * otherwise it is set to use 0x02,0x03,0x06 and when used in conjunction 4416 * with top(rear) speaker 0x14, gets locked at full volume */
4417 static const hda_nid_t conn1[] = { 0x02 };
4418 snd_hda_override_conn_list(codec, 0x17, ARRAY_SIZE(conn1), conn1); 4419 4420 break; 4421 case HDA_FIXUP_ACT_INIT: 4422 /* need to toggle GPIO to enable the amp */ 4423 alc_update_gpio_data(codec, 0x01, true); 4424 msleep(100); 4425 alc_update_gpio_data(codec, 0x01, false); 4426 break; 4427 } 4428 } 4429
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
participants (3)
-
Jonathan Clarke -
kernel test robot -
Takashi Iwai