Re: [PATCH 2/2] ALSA : hda : Support for Ideapad hotkey mute LEDs

[Takashi Iwai <tiwai@xxxxxxx>]

On Fri, 20 Dec 2024 11:22:40 +0100,
Takashi Iwai wrote:
> 
> On Thu, 19 Dec 2024 11:15:31 +0100,
> Jackie Dong wrote:
> > 
> > New ideapad helper file with support for handling FN key mute LEDS.
> > Update conexant and realtec codec to add LED support.
> > 
> > Suggested-by: Mark Pearson <mpearson-lenovo@xxxxxxxxx>
> > Signed-off-by: Jackie Dong  <xy-jackie@xxxxxxx>
> > Signed-off-by: Jackie Dong  <dongeg1@xxxxxxxxxx>
> > ---
> >  sound/pci/hda/ideapad_hotkey_led_helper.c | 36 +++++++++++++++++++++++
> >  sound/pci/hda/patch_conexant.c            | 10 +++++++
> >  sound/pci/hda/patch_realtek.c             | 20 +++++++++++++
> >  3 files changed, 66 insertions(+)
> >  create mode 100644 sound/pci/hda/ideapad_hotkey_led_helper.c
> > 
> > diff --git a/sound/pci/hda/ideapad_hotkey_led_helper.c b/sound/pci/hda/ideapad_hotkey_led_helper.c
> > new file mode 100644
> > index 000000000000..e49765304cc0
> > --- /dev/null
> > +++ b/sound/pci/hda/ideapad_hotkey_led_helper.c
> > @@ -0,0 +1,36 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Ideapad helper functions for Lenovo Ideapad LED control,
> > + * It should be included from codec driver.
> > + */
> > +
> > +#if IS_ENABLED(CONFIG_IDEAPAD_LAPTOP)
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/leds.h>
> > +
> > +static bool is_ideapad(struct hda_codec *codec)
> > +{
> > +	return (codec->core.subsystem_id >> 16 == 0x17aa) &&
> > +	       (acpi_dev_found("LHK2019") || acpi_dev_found("VPC2004"));
> > +}
> > +
> > +static void hda_fixup_ideapad_acpi(struct hda_codec *codec,
> > +				   const struct hda_fixup *fix, int action)
> > +{
> > +	if (action == HDA_FIXUP_ACT_PRE_PROBE) {
> > +		if (!is_ideapad(codec))
> > +			return;
> > +		snd_hda_gen_add_mute_led_cdev(codec, NULL);
> > +		snd_hda_gen_add_micmute_led_cdev(codec, NULL);
> > +	}
> > +}
> > +
> > +#else /* CONFIG_IDEAPAD_LAPTOP */
> > +
> > +static void hda_fixup_ideapad_acpi(struct hda_codec *codec,
> > +				   const struct hda_fixup *fix, int action)
> > +{
> > +}
> > +
> > +#endif /* CONFIG_IDEAPAD_LAPTOP */
> > diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c
> > index 538c37a78a56..127f9a9565c9 100644
> > --- a/sound/pci/hda/patch_conexant.c
> > +++ b/sound/pci/hda/patch_conexant.c
> > @@ -291,6 +291,7 @@ enum {
> >  	CXT_FIXUP_GPIO1,
> >  	CXT_FIXUP_ASPIRE_DMIC,
> >  	CXT_FIXUP_THINKPAD_ACPI,
> > +	CXT_FIXUP_IDEAPAD_ACPI,
> >  	CXT_FIXUP_OLPC_XO,
> >  	CXT_FIXUP_CAP_MIX_AMP,
> >  	CXT_FIXUP_TOSHIBA_P105,
> > @@ -313,6 +314,9 @@ enum {
> >  /* for hda_fixup_thinkpad_acpi() */
> >  #include "thinkpad_helper.c"
> >  
> > +/* for hda_fixup_ideapad_acpi() */
> > +#include "ideapad_hotkey_led_helper.c"
> > +
> >  static void cxt_fixup_stereo_dmic(struct hda_codec *codec,
> >  				  const struct hda_fixup *fix, int action)
> >  {
> > @@ -928,6 +932,10 @@ static const struct hda_fixup cxt_fixups[] = {
> >  		.type = HDA_FIXUP_FUNC,
> >  		.v.func = hda_fixup_thinkpad_acpi,
> >  	},
> > +	[CXT_FIXUP_IDEAPAD_ACPI] = {
> > +		.type = HDA_FIXUP_FUNC,
> > +		.v.func = hda_fixup_ideapad_acpi,
> > +	},
> >  	[CXT_FIXUP_OLPC_XO] = {
> >  		.type = HDA_FIXUP_FUNC,
> >  		.v.func = cxt_fixup_olpc_xo,
> > @@ -1120,6 +1128,7 @@ static const struct hda_quirk cxt5066_fixups[] = {
> >  	SND_PCI_QUIRK(0x17aa, 0x3978, "Lenovo G50-70", CXT_FIXUP_STEREO_DMIC),
> >  	SND_PCI_QUIRK(0x17aa, 0x397b, "Lenovo S205", CXT_FIXUP_STEREO_DMIC),
> >  	SND_PCI_QUIRK_VENDOR(0x17aa, "Thinkpad", CXT_FIXUP_THINKPAD_ACPI),
> > +	SND_PCI_QUIRK_VENDOR(0x17aa, "IdeaPad", CXT_FIXUP_IDEAPAD_ACPI),
> 
> I'm afraid that this doesn't work.  The former entry with the vendor
> 17aa already hits and it's used as the matched quirk.
> 
> You'd need to create a chained quirk entry instead.

That is, add a new enum CXT_FIXUP_LENOVO_XPAD_ACPI, and define an
entry like:

	[CXT_FIXUP_LENOVO_XPAD_ACPI] = {
		.type = HDA_FIXUP_FUNC,
		.v.func = hda_fixup_ideapad_acpi,
		.chained = true,
		.chain_id = ALC269_FIXUP_THINKPAD_ACPI,
	},

then replace the line
 	SND_PCI_QUIRK_VENDOR(0x17aa, "Thinkpad", CXT_FIXUP_THINKPAD_ACPI),
with
 	SND_PCI_QUIRK_VENDOR(0x17aa, "Thinkpad/Ideapad", CXT_FIXUP_LENOVO_XPAD_ACPI),

Care to rewrite the patch in that way, and resubmit v2 patch?


thanks,

Takashi