Message ID | 20241219101531.35896-1-xy-jackie@139.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [1/2] platform/x86: ideapad-laptop: Support for mic and audio leds. | expand |
On Thu, 19 Dec 2024, Jackie Dong wrote: > Implement Lenovo utility data WMI calls needed to make LEDs > work on Ideapads that support this GUID. > This enables the mic and audio LEDs to be updated correctly. > > Tested on below samples. > ThinkBook 13X Gen4 IMH > ThinkBook 14 G6 ABP > ThinkBook 16p Gen4-21J8 > ThinkBook 16p Gen8-IRL > ThinkBook 16p G7+ ASP > > Suggested-by: Mark Pearson <mpearson-lenovo@squebb.ca> > Signed-off-by: Jackie Dong <xy-jackie@139.com> > Signed-off-by: Jackie Dong <dongeg1@lenovo.com> One signed off is enough from you. :-) Please use the one matching to From: address. If you want mails automatically to some other address too, you can always add a Cc: tag so the git send-email and various other tools will included that email address too. > --- > drivers/platform/x86/ideapad-laptop.c | 157 +++++++++++++++++++++++++- > 1 file changed, 156 insertions(+), 1 deletion(-) > > diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c > index c64dfc56651d..acea4aa8eac3 100644 > --- a/drivers/platform/x86/ideapad-laptop.c > +++ b/drivers/platform/x86/ideapad-laptop.c > @@ -32,6 +32,7 @@ > #include <linux/sysfs.h> > #include <linux/types.h> > #include <linux/wmi.h> > +#include <sound/control.h> > #include "ideapad-laptop.h" > > #include <acpi/video.h> > @@ -1298,6 +1299,15 @@ static const struct key_entry ideapad_keymap[] = { > { KE_END }, > }; > > +/* > + * Input parameters to mute/unmute audio LED and Mic LED > + */ Fits to one line. > +struct wmi_led_args { > + u8 ID; > + u8 SubID; > + u16 Value; Use only lowercase in variable names. > +}; > + > static int ideapad_input_init(struct ideapad_private *priv) > { > struct input_dev *inputdev; > @@ -2023,15 +2033,145 @@ static void ideapad_check_features(struct ideapad_private *priv) > /* > * WMI driver > */ > +#define IDEAPAD_ACPI_LED_MAX (((SNDRV_CTL_ELEM_ACCESS_MIC_LED -\ > + SNDRV_CTL_ELEM_ACCESS_SPK_LED) >> SNDRV_CTL_ELEM_ACCESS_LED_SHIFT) + 1) Hmm, so you fix the math bug (2-1 is not 2 but 1) with that +1 in the end? I think you would want something like this here (but I'm not entirely sure at this point of reading your change): FIELD_GET(SNDRV_CTL_ELEM_ACCESS_MIC_LED, SNDRV_CTL_ELEM_ACCESS_MIC_LED) (Remember to make sure you've include for FIELD_GET if that's the correct way to go here). > + > enum ideapad_wmi_event_type { > IDEAPAD_WMI_EVENT_ESC, > IDEAPAD_WMI_EVENT_FN_KEYS, > + IDEAPAD_WMI_EVENT_LUD_KEYS, > }; > > +#define WMI_LUD_GET_SUPPORT 1 > +#define WMI_LUD_SET_FEATURE 2 > + > +#define WMI_LUD_GET_MICMUTE_LED_VER 20 > +#define WMI_LUD_GET_AUDIOMUTE_LED_VER 26 > + > +#define WMI_LUD_SUPPORT_MICMUTE_LED_VER 25 > +#define WMI_LUD_SUPPORT_AUDIOMUTE_LED_VER 27 > + > struct ideapad_wmi_private { > enum ideapad_wmi_event_type event; > + struct led_classdev cdev[IDEAPAD_ACPI_LED_MAX]; > }; > > +static struct wmi_device *led_wdev; > + > +enum mute_led_type { > + MIC_MUTE, > + AUDIO_MUTE, > +}; > + > +static int ideapad_wmi_mute_led_set(enum mute_led_type led_type, struct led_classdev *led_cdev, > + enum led_brightness brightness) > + > +{ > + struct wmi_led_args led_arg = {0, 0, 0}; > + struct acpi_buffer input; > + acpi_status status; > + > + if (led_type == MIC_MUTE) > + led_arg.ID = brightness == LED_ON ? 1 : 2; > + else if (led_type == AUDIO_MUTE) > + led_arg.ID = brightness == LED_ON ? 4 : 5; Use named defines instead of literals. > + else > + return -EINVAL; > + > + input.length = sizeof(struct wmi_led_args); > + input.pointer = &led_arg; > + status = wmidev_evaluate_method(led_wdev, 0, WMI_LUD_SET_FEATURE, &input, NULL); > + > + if (ACPI_FAILURE(status)) Don't leave empty line between call and its error handling. > + return -EIO; > + > + return 0; > +} > + > +static int ideapad_wmi_audiomute_led_set(struct led_classdev *led_cdev, > + enum led_brightness brightness) > + > +{ > + return ideapad_wmi_mute_led_set(AUDIO_MUTE, led_cdev, brightness); > +} > + > +static int ideapad_wmi_micmute_led_set(struct led_classdev *led_cdev, > + enum led_brightness brightness) > +{ > + return ideapad_wmi_mute_led_set(MIC_MUTE, led_cdev, brightness); > +} > + > +static int ideapad_wmi_leds_init(enum mute_led_type led_type, struct device *dev) This seems to init only a single LED at a time but the function name says "leds" which is plural. > +{ > + struct ideapad_wmi_private *wpriv = dev_get_drvdata(dev); > + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; > + struct acpi_buffer input; > + union acpi_object *obj; > + int led_version, err = 0; > + unsigned int wmiarg; > + acpi_status status; > + > + if (led_type == MIC_MUTE) > + wmiarg = WMI_LUD_GET_MICMUTE_LED_VER; > + else if (led_type == AUDIO_MUTE) > + wmiarg = WMI_LUD_GET_AUDIOMUTE_LED_VER; > + else > + return -EINVAL; Use switch/case/default > + > + input.length = sizeof(wmiarg); > + input.pointer = &wmiarg; > + status = wmidev_evaluate_method(led_wdev, 0, WMI_LUD_GET_SUPPORT, &input, &output); > + if (ACPI_FAILURE(status)) { > + kfree(output.pointer); Is this kfree() correct thing to do in case of error?? > + return -EIO; > + } > + obj = output.pointer; > + led_version = obj->integer.value; > + > + wpriv->cdev[led_type].max_brightness = LED_ON; > + wpriv->cdev[led_type].dev = dev; > + wpriv->cdev[led_type].flags = LED_CORE_SUSPENDRESUME; > + > + if (led_type == MIC_MUTE) { These blocks too should use switch. > + if (led_version != WMI_LUD_SUPPORT_MICMUTE_LED_VER) { > + dev_info(dev, "This product doesn't support mic mute LED.\n"); If this is expected to trigger on some HW, it seems nuisance noise in the log for such machine. Do you have a memleak here? > + return -EIO; > + } > + wpriv->cdev[led_type].name = "platform::micmute"; > + wpriv->cdev[led_type].brightness_set_blocking = &ideapad_wmi_micmute_led_set; > + wpriv->cdev[led_type].default_trigger = "audio-micmute"; > + > + err = devm_led_classdev_register(dev, &wpriv->cdev[led_type]); > + if (err < 0) { > + dev_err(dev, "Could not register mic mute LED : %d\n", err); > + led_classdev_unregister(&wpriv->cdev[led_type]); This unregister could be put to a rollback path and you goto there. That way, both leds can share the unregister call. > + } > + } else { > + if (led_version != WMI_LUD_SUPPORT_AUDIOMUTE_LED_VER) { > + dev_info(dev, "This product doesn't support audio mute LED.\n"); Same here. > + return -EIO; > + } > + wpriv->cdev[led_type].name = "platform::mute"; > + wpriv->cdev[led_type].brightness_set_blocking = &ideapad_wmi_audiomute_led_set; > + wpriv->cdev[led_type].default_trigger = "audio-mute"; > + > + err = devm_led_classdev_register(dev, &wpriv->cdev[led_type]); > + if (err < 0) { > + dev_err(dev, "Could not register audio mute LED: %d\n", err); > + led_classdev_unregister(&wpriv->cdev[led_type]); > + } > + } > + > + kfree(obj); > + return err; > +} > + > +static void ideapad_wmi_leds_setup(struct device *dev) > +{ > + ideapad_wmi_leds_init(MIC_MUTE, dev); > + ideapad_wmi_leds_init(AUDIO_MUTE, dev); > +} > + > static int ideapad_wmi_probe(struct wmi_device *wdev, const void *context) > { > struct ideapad_wmi_private *wpriv; > @@ -2043,6 +2183,12 @@ static int ideapad_wmi_probe(struct wmi_device *wdev, const void *context) > *wpriv = *(const struct ideapad_wmi_private *)context; > > dev_set_drvdata(&wdev->dev, wpriv); > + > + if (wpriv->event == IDEAPAD_WMI_EVENT_LUD_KEYS) { > + led_wdev = wdev; > + ideapad_wmi_leds_setup(&wdev->dev); > + } > + > return 0; > } > > @@ -2088,6 +2234,9 @@ static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data) > data->integer.value | IDEAPAD_WMI_KEY); > > break; > + case IDEAPAD_WMI_EVENT_LUD_KEYS: > + break; > + > } > } > > @@ -2099,10 +2248,16 @@ static const struct ideapad_wmi_private ideapad_wmi_context_fn_keys = { > .event = IDEAPAD_WMI_EVENT_FN_KEYS > }; > > +static const struct ideapad_wmi_private ideapad_wmi_context_LUD_keys = { > + .event = IDEAPAD_WMI_EVENT_LUD_KEYS > +}; > + > static const struct wmi_device_id ideapad_wmi_ids[] = { > { "26CAB2E5-5CF1-46AE-AAC3-4A12B6BA50E6", &ideapad_wmi_context_esc }, /* Yoga 3 */ > { "56322276-8493-4CE8-A783-98C991274F5E", &ideapad_wmi_context_esc }, /* Yoga 700 */ > - { "8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", &ideapad_wmi_context_fn_keys }, /* Legion 5 */ > + { "8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", &ideapad_wmi_context_fn_keys }, /* FN keys */ How is this change related to adding LEDs ? You can always do a patch series if you want to change unrelated things. > + { "CE6C0974-0407-4F50-88BA-4FC3B6559AD8", &ideapad_wmi_context_LUD_keys }, /* Util data */ > + > {}, > }; > MODULE_DEVICE_TABLE(wmi, ideapad_wmi_ids); >
On 19. 12. 24 12:40, Ilpo Järvinen wrote: > On Thu, 19 Dec 2024, Jackie Dong wrote: ... >> +}; >> + >> static int ideapad_input_init(struct ideapad_private *priv) >> { >> struct input_dev *inputdev; >> @@ -2023,15 +2033,145 @@ static void ideapad_check_features(struct ideapad_private *priv) >> /* >> * WMI driver >> */ >> +#define IDEAPAD_ACPI_LED_MAX (((SNDRV_CTL_ELEM_ACCESS_MIC_LED -\ >> + SNDRV_CTL_ELEM_ACCESS_SPK_LED) >> SNDRV_CTL_ELEM_ACCESS_LED_SHIFT) + 1) > > Hmm, so you fix the math bug (2-1 is not 2 but 1) with that +1 in the end? > > I think you would want something like this here (but I'm not entirely > sure at this point of reading your change): > > FIELD_GET(SNDRV_CTL_ELEM_ACCESS_MIC_LED, SNDRV_CTL_ELEM_ACCESS_MIC_LED) > > (Remember to make sure you've include for FIELD_GET if that's the correct > way to go here). There's no reason to use SNDRV_CTL_ELEM_ACCESS definitions here (no direct connection to the sound control API). I would use direct value 2 here, because this extension controls only 2 LEDs. Jaroslav
Am 19.12.24 um 11:15 schrieb Jackie Dong: > Implement Lenovo utility data WMI calls needed to make LEDs > work on Ideapads that support this GUID. > This enables the mic and audio LEDs to be updated correctly. > > Tested on below samples. > ThinkBook 13X Gen4 IMH > ThinkBook 14 G6 ABP > ThinkBook 16p Gen4-21J8 > ThinkBook 16p Gen8-IRL > ThinkBook 16p G7+ ASP Hi, i am a bit confused regarding the role of the CE6C0974-0407-4F50-88BA-4FC3B6559AD8 WMI device: - is it a event or a method block? - is it in some way connected with the remaining WMI devices supported by the ideapad-laptop driver? Looking at the code it seems to me that the CE6C0974-0407-4F50-88BA-4FC3B6559AD8 WMI device is not a event device and is not directly connected with the remaining WMI devices (please correct me if i am wrong). Can you please write a separate driver for this WMI device? Getting the ideapad-laptop driver involved in this seems to be unreasonable since the audio led functionality does not interact with the remaining driver. This might be helpful: https://docs.kernel.org/wmi/driver-development-guide.html. > > Suggested-by: Mark Pearson <mpearson-lenovo@squebb.ca> > Signed-off-by: Jackie Dong <xy-jackie@139.com> > Signed-off-by: Jackie Dong <dongeg1@lenovo.com> Please keep only the Signed-of tag with the email address used for sending this patch. Besides that its always nice to see vendors getting involved with upstream :). Thanks, Armin Wolf > --- > drivers/platform/x86/ideapad-laptop.c | 157 +++++++++++++++++++++++++- > 1 file changed, 156 insertions(+), 1 deletion(-) > > diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c > index c64dfc56651d..acea4aa8eac3 100644 > --- a/drivers/platform/x86/ideapad-laptop.c > +++ b/drivers/platform/x86/ideapad-laptop.c > @@ -32,6 +32,7 @@ > #include <linux/sysfs.h> > #include <linux/types.h> > #include <linux/wmi.h> > +#include <sound/control.h> > #include "ideapad-laptop.h" > > #include <acpi/video.h> > @@ -1298,6 +1299,15 @@ static const struct key_entry ideapad_keymap[] = { > { KE_END }, > }; > > +/* > + * Input parameters to mute/unmute audio LED and Mic LED > + */ > +struct wmi_led_args { > + u8 ID; > + u8 SubID; > + u16 Value; > +}; > + > static int ideapad_input_init(struct ideapad_private *priv) > { > struct input_dev *inputdev; > @@ -2023,15 +2033,145 @@ static void ideapad_check_features(struct ideapad_private *priv) > /* > * WMI driver > */ > +#define IDEAPAD_ACPI_LED_MAX (((SNDRV_CTL_ELEM_ACCESS_MIC_LED -\ > + SNDRV_CTL_ELEM_ACCESS_SPK_LED) >> SNDRV_CTL_ELEM_ACCESS_LED_SHIFT) + 1) > + > enum ideapad_wmi_event_type { > IDEAPAD_WMI_EVENT_ESC, > IDEAPAD_WMI_EVENT_FN_KEYS, > + IDEAPAD_WMI_EVENT_LUD_KEYS, > }; > > +#define WMI_LUD_GET_SUPPORT 1 > +#define WMI_LUD_SET_FEATURE 2 > + > +#define WMI_LUD_GET_MICMUTE_LED_VER 20 > +#define WMI_LUD_GET_AUDIOMUTE_LED_VER 26 > + > +#define WMI_LUD_SUPPORT_MICMUTE_LED_VER 25 > +#define WMI_LUD_SUPPORT_AUDIOMUTE_LED_VER 27 > + > struct ideapad_wmi_private { > enum ideapad_wmi_event_type event; > + struct led_classdev cdev[IDEAPAD_ACPI_LED_MAX]; > }; > > +static struct wmi_device *led_wdev; > + > +enum mute_led_type { > + MIC_MUTE, > + AUDIO_MUTE, > +}; > + > +static int ideapad_wmi_mute_led_set(enum mute_led_type led_type, struct led_classdev *led_cdev, > + enum led_brightness brightness) > + > +{ > + struct wmi_led_args led_arg = {0, 0, 0}; > + struct acpi_buffer input; > + acpi_status status; > + > + if (led_type == MIC_MUTE) > + led_arg.ID = brightness == LED_ON ? 1 : 2; > + else if (led_type == AUDIO_MUTE) > + led_arg.ID = brightness == LED_ON ? 4 : 5; > + else > + return -EINVAL; > + > + input.length = sizeof(struct wmi_led_args); > + input.pointer = &led_arg; > + status = wmidev_evaluate_method(led_wdev, 0, WMI_LUD_SET_FEATURE, &input, NULL); > + > + if (ACPI_FAILURE(status)) > + return -EIO; > + > + return 0; > +} > + > +static int ideapad_wmi_audiomute_led_set(struct led_classdev *led_cdev, > + enum led_brightness brightness) > + > +{ > + return ideapad_wmi_mute_led_set(AUDIO_MUTE, led_cdev, brightness); > +} > + > +static int ideapad_wmi_micmute_led_set(struct led_classdev *led_cdev, > + enum led_brightness brightness) > +{ > + return ideapad_wmi_mute_led_set(MIC_MUTE, led_cdev, brightness); > +} > + > +static int ideapad_wmi_leds_init(enum mute_led_type led_type, struct device *dev) > +{ > + struct ideapad_wmi_private *wpriv = dev_get_drvdata(dev); > + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; > + struct acpi_buffer input; > + union acpi_object *obj; > + int led_version, err = 0; > + unsigned int wmiarg; > + acpi_status status; > + > + if (led_type == MIC_MUTE) > + wmiarg = WMI_LUD_GET_MICMUTE_LED_VER; > + else if (led_type == AUDIO_MUTE) > + wmiarg = WMI_LUD_GET_AUDIOMUTE_LED_VER; > + else > + return -EINVAL; > + > + input.length = sizeof(wmiarg); > + input.pointer = &wmiarg; > + status = wmidev_evaluate_method(led_wdev, 0, WMI_LUD_GET_SUPPORT, &input, &output); > + if (ACPI_FAILURE(status)) { > + kfree(output.pointer); > + return -EIO; > + } > + obj = output.pointer; > + led_version = obj->integer.value; > + > + wpriv->cdev[led_type].max_brightness = LED_ON; > + wpriv->cdev[led_type].dev = dev; > + wpriv->cdev[led_type].flags = LED_CORE_SUSPENDRESUME; > + > + if (led_type == MIC_MUTE) { > + if (led_version != WMI_LUD_SUPPORT_MICMUTE_LED_VER) { > + dev_info(dev, "This product doesn't support mic mute LED.\n"); > + return -EIO; > + } > + wpriv->cdev[led_type].name = "platform::micmute"; > + wpriv->cdev[led_type].brightness_set_blocking = &ideapad_wmi_micmute_led_set; > + wpriv->cdev[led_type].default_trigger = "audio-micmute"; > + > + err = devm_led_classdev_register(dev, &wpriv->cdev[led_type]); > + if (err < 0) { > + dev_err(dev, "Could not register mic mute LED : %d\n", err); > + led_classdev_unregister(&wpriv->cdev[led_type]); > + } > + } else { > + if (led_version != WMI_LUD_SUPPORT_AUDIOMUTE_LED_VER) { > + dev_info(dev, "This product doesn't support audio mute LED.\n"); > + return -EIO; > + } > + wpriv->cdev[led_type].name = "platform::mute"; > + wpriv->cdev[led_type].brightness_set_blocking = &ideapad_wmi_audiomute_led_set; > + wpriv->cdev[led_type].default_trigger = "audio-mute"; > + > + err = devm_led_classdev_register(dev, &wpriv->cdev[led_type]); > + if (err < 0) { > + dev_err(dev, "Could not register audio mute LED: %d\n", err); > + led_classdev_unregister(&wpriv->cdev[led_type]); > + } > + } > + > + kfree(obj); > + return err; > +} > + > +static void ideapad_wmi_leds_setup(struct device *dev) > +{ > + ideapad_wmi_leds_init(MIC_MUTE, dev); > + ideapad_wmi_leds_init(AUDIO_MUTE, dev); > +} > + > static int ideapad_wmi_probe(struct wmi_device *wdev, const void *context) > { > struct ideapad_wmi_private *wpriv; > @@ -2043,6 +2183,12 @@ static int ideapad_wmi_probe(struct wmi_device *wdev, const void *context) > *wpriv = *(const struct ideapad_wmi_private *)context; > > dev_set_drvdata(&wdev->dev, wpriv); > + > + if (wpriv->event == IDEAPAD_WMI_EVENT_LUD_KEYS) { > + led_wdev = wdev; > + ideapad_wmi_leds_setup(&wdev->dev); > + } > + > return 0; > } > > @@ -2088,6 +2234,9 @@ static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data) > data->integer.value | IDEAPAD_WMI_KEY); > > break; > + case IDEAPAD_WMI_EVENT_LUD_KEYS: > + break; > + > } > } > > @@ -2099,10 +2248,16 @@ static const struct ideapad_wmi_private ideapad_wmi_context_fn_keys = { > .event = IDEAPAD_WMI_EVENT_FN_KEYS > }; > > +static const struct ideapad_wmi_private ideapad_wmi_context_LUD_keys = { > + .event = IDEAPAD_WMI_EVENT_LUD_KEYS > +}; > + > static const struct wmi_device_id ideapad_wmi_ids[] = { > { "26CAB2E5-5CF1-46AE-AAC3-4A12B6BA50E6", &ideapad_wmi_context_esc }, /* Yoga 3 */ > { "56322276-8493-4CE8-A783-98C991274F5E", &ideapad_wmi_context_esc }, /* Yoga 700 */ > - { "8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", &ideapad_wmi_context_fn_keys }, /* Legion 5 */ > + { "8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", &ideapad_wmi_context_fn_keys }, /* FN keys */ > + { "CE6C0974-0407-4F50-88BA-4FC3B6559AD8", &ideapad_wmi_context_LUD_keys }, /* Util data */ > + > {}, > }; > MODULE_DEVICE_TABLE(wmi, ideapad_wmi_ids);
Dear Armin, CE6C0974-0407-4F50-88BA-4FC3B6559AD8 is a WMI interface method. From Lenovo keyboard WMI specification, I found it applicable to LNB products, i.e. YOGA/XiaoXin/Gaming/ThinkBook etc and performs the Lenovo customized hotkeys function for Consumer and SMB notebooks. I implemented the audio mute LED and mic mute LED function of IdeaPad notebook in ideapad_laptop driver, because I found the mute LED function of Thinkpad notebook is implemented by thinkpad_acpi. And ideapad_laptop driver has the similar function as thinkpad_acpi. Thanks, Jackie Dong -----Original Message----- From: Armin Wolf <W_Armin@gmx.de> Sent: Monday, December 23, 2024 6:34 AM To: Jackie Dong <xy-jackie@139.com>; ike.pan@canonical.com; hdegoede@redhat.com; ilpo.jarvinen@linux.intel.com; perex@perex.cz; tiwai@suse.com; bo.liu@senarytech.com; kovalev@altlinux.org; me@oldherl.one; jaroslaw.janik@gmail.com; cs@tuxedo.de; songxiebing@kylinos.cn; kailang@realtek.com; sbinding@opensource.cirrus.com; simont@opensource.cirrus.com; josh@joshuagrisham.com; rf@opensource.cirrus.com Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; linux-sound@vger.kernel.org; mpearson-lenovo@squebb.ca; waterflowdeg@gmail.com; Jackie EG1 Dong <dongeg1@lenovo.com> Subject: [External] Re: [PATCH 1/2] platform/x86: ideapad-laptop: Support for mic and audio leds. Am 19.12.24 um 11:15 schrieb Jackie Dong: > Implement Lenovo utility data WMI calls needed to make LEDs work on > Ideapads that support this GUID. > This enables the mic and audio LEDs to be updated correctly. > > Tested on below samples. > ThinkBook 13X Gen4 IMH > ThinkBook 14 G6 ABP > ThinkBook 16p Gen4-21J8 > ThinkBook 16p Gen8-IRL > ThinkBook 16p G7+ ASP Hi, i am a bit confused regarding the role of the CE6C0974-0407-4F50-88BA-4FC3B6559AD8 WMI device: - is it a event or a method block? - is it in some way connected with the remaining WMI devices supported by the ideapad-laptop driver? Looking at the code it seems to me that the CE6C0974-0407-4F50-88BA-4FC3B6559AD8 WMI device is not a event device and is not directly connected with the remaining WMI devices (please correct me if i am wrong). Can you please write a separate driver for this WMI device? Getting the ideapad-laptop driver involved in this seems to be unreasonable since the audio led functionality does not interact with the remaining driver. This might be helpful: https://docs.kernel.org/wmi/driver-development-guide.html. > > Suggested-by: Mark Pearson <mpearson-lenovo@squebb.ca> > Signed-off-by: Jackie Dong <xy-jackie@139.com> > Signed-off-by: Jackie Dong <dongeg1@lenovo.com> Please keep only the Signed-of tag with the email address used for sending this patch. Besides that its always nice to see vendors getting involved with upstream :). Thanks, Armin Wolf > --- > drivers/platform/x86/ideapad-laptop.c | 157 +++++++++++++++++++++++++- > 1 file changed, 156 insertions(+), 1 deletion(-) > > diff --git a/drivers/platform/x86/ideapad-laptop.c > b/drivers/platform/x86/ideapad-laptop.c > index c64dfc56651d..acea4aa8eac3 100644 > --- a/drivers/platform/x86/ideapad-laptop.c > +++ b/drivers/platform/x86/ideapad-laptop.c > @@ -32,6 +32,7 @@ > #include <linux/sysfs.h> > #include <linux/types.h> > #include <linux/wmi.h> > +#include <sound/control.h> > #include "ideapad-laptop.h" > > #include <acpi/video.h> > @@ -1298,6 +1299,15 @@ static const struct key_entry ideapad_keymap[] = { > { KE_END }, > }; > > +/* > + * Input parameters to mute/unmute audio LED and Mic LED */ struct > +wmi_led_args { > + u8 ID; > + u8 SubID; > + u16 Value; > +}; > + > static int ideapad_input_init(struct ideapad_private *priv) > { > struct input_dev *inputdev; > @@ -2023,15 +2033,145 @@ static void ideapad_check_features(struct ideapad_private *priv) > /* > * WMI driver > */ > +#define IDEAPAD_ACPI_LED_MAX (((SNDRV_CTL_ELEM_ACCESS_MIC_LED -\ > + SNDRV_CTL_ELEM_ACCESS_SPK_LED) >> SNDRV_CTL_ELEM_ACCESS_LED_SHIFT) > ++ 1) > + > enum ideapad_wmi_event_type { > IDEAPAD_WMI_EVENT_ESC, > IDEAPAD_WMI_EVENT_FN_KEYS, > + IDEAPAD_WMI_EVENT_LUD_KEYS, > }; > > +#define WMI_LUD_GET_SUPPORT 1 > +#define WMI_LUD_SET_FEATURE 2 > + > +#define WMI_LUD_GET_MICMUTE_LED_VER 20 > +#define WMI_LUD_GET_AUDIOMUTE_LED_VER 26 > + > +#define WMI_LUD_SUPPORT_MICMUTE_LED_VER 25 > +#define WMI_LUD_SUPPORT_AUDIOMUTE_LED_VER 27 > + > struct ideapad_wmi_private { > enum ideapad_wmi_event_type event; > + struct led_classdev cdev[IDEAPAD_ACPI_LED_MAX]; > }; > > +static struct wmi_device *led_wdev; > + > +enum mute_led_type { > + MIC_MUTE, > + AUDIO_MUTE, > +}; > + > +static int ideapad_wmi_mute_led_set(enum mute_led_type led_type, struct led_classdev *led_cdev, > + enum led_brightness brightness) > + > +{ > + struct wmi_led_args led_arg = {0, 0, 0}; > + struct acpi_buffer input; > + acpi_status status; > + > + if (led_type == MIC_MUTE) > + led_arg.ID = brightness == LED_ON ? 1 : 2; > + else if (led_type == AUDIO_MUTE) > + led_arg.ID = brightness == LED_ON ? 4 : 5; > + else > + return -EINVAL; > + > + input.length = sizeof(struct wmi_led_args); > + input.pointer = &led_arg; > + status = wmidev_evaluate_method(led_wdev, 0, WMI_LUD_SET_FEATURE, > +&input, NULL); > + > + if (ACPI_FAILURE(status)) > + return -EIO; > + > + return 0; > +} > + > +static int ideapad_wmi_audiomute_led_set(struct led_classdev *led_cdev, > + enum led_brightness brightness) > + > +{ > + return ideapad_wmi_mute_led_set(AUDIO_MUTE, led_cdev, brightness); } > + > +static int ideapad_wmi_micmute_led_set(struct led_classdev *led_cdev, > + enum led_brightness brightness) { > + return ideapad_wmi_mute_led_set(MIC_MUTE, led_cdev, brightness); } > + > +static int ideapad_wmi_leds_init(enum mute_led_type led_type, struct > +device *dev) { > + struct ideapad_wmi_private *wpriv = dev_get_drvdata(dev); > + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; > + struct acpi_buffer input; > + union acpi_object *obj; > + int led_version, err = 0; > + unsigned int wmiarg; > + acpi_status status; > + > + if (led_type == MIC_MUTE) > + wmiarg = WMI_LUD_GET_MICMUTE_LED_VER; > + else if (led_type == AUDIO_MUTE) > + wmiarg = WMI_LUD_GET_AUDIOMUTE_LED_VER; > + else > + return -EINVAL; > + > + input.length = sizeof(wmiarg); > + input.pointer = &wmiarg; > + status = wmidev_evaluate_method(led_wdev, 0, WMI_LUD_GET_SUPPORT, &input, &output); > + if (ACPI_FAILURE(status)) { > + kfree(output.pointer); > + return -EIO; > + } > + obj = output.pointer; > + led_version = obj->integer.value; > + > + wpriv->cdev[led_type].max_brightness = LED_ON; > + wpriv->cdev[led_type].dev = dev; > + wpriv->cdev[led_type].flags = LED_CORE_SUSPENDRESUME; > + > + if (led_type == MIC_MUTE) { > + if (led_version != WMI_LUD_SUPPORT_MICMUTE_LED_VER) { > + dev_info(dev, "This product doesn't support mic mute LED.\n"); > + return -EIO; > + } > + wpriv->cdev[led_type].name = "platform::micmute"; > + wpriv->cdev[led_type].brightness_set_blocking = &ideapad_wmi_micmute_led_set; > + wpriv->cdev[led_type].default_trigger = "audio-micmute"; > + > + err = devm_led_classdev_register(dev, &wpriv->cdev[led_type]); > + if (err < 0) { > + dev_err(dev, "Could not register mic mute LED : %d\n", err); > + led_classdev_unregister(&wpriv->cdev[led_type]); > + } > + } else { > + if (led_version != WMI_LUD_SUPPORT_AUDIOMUTE_LED_VER) { > + dev_info(dev, "This product doesn't support audio mute LED.\n"); > + return -EIO; > + } > + wpriv->cdev[led_type].name = "platform::mute"; > + wpriv->cdev[led_type].brightness_set_blocking = &ideapad_wmi_audiomute_led_set; > + wpriv->cdev[led_type].default_trigger = "audio-mute"; > + > + err = devm_led_classdev_register(dev, &wpriv->cdev[led_type]); > + if (err < 0) { > + dev_err(dev, "Could not register audio mute LED: %d\n", err); > + led_classdev_unregister(&wpriv->cdev[led_type]); > + } > + } > + > + kfree(obj); > + return err; > +} > + > +static void ideapad_wmi_leds_setup(struct device *dev) { > + ideapad_wmi_leds_init(MIC_MUTE, dev); > + ideapad_wmi_leds_init(AUDIO_MUTE, dev); } > + > static int ideapad_wmi_probe(struct wmi_device *wdev, const void *context) > { > struct ideapad_wmi_private *wpriv; > @@ -2043,6 +2183,12 @@ static int ideapad_wmi_probe(struct wmi_device *wdev, const void *context) > *wpriv = *(const struct ideapad_wmi_private *)context; > > dev_set_drvdata(&wdev->dev, wpriv); > + > + if (wpriv->event == IDEAPAD_WMI_EVENT_LUD_KEYS) { > + led_wdev = wdev; > + ideapad_wmi_leds_setup(&wdev->dev); > + } > + > return 0; > } > > @@ -2088,6 +2234,9 @@ static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data) > data->integer.value | IDEAPAD_WMI_KEY); > > break; > + case IDEAPAD_WMI_EVENT_LUD_KEYS: > + break; > + > } > } > > @@ -2099,10 +2248,16 @@ static const struct ideapad_wmi_private ideapad_wmi_context_fn_keys = { > .event = IDEAPAD_WMI_EVENT_FN_KEYS > }; > > +static const struct ideapad_wmi_private ideapad_wmi_context_LUD_keys = { > + .event = IDEAPAD_WMI_EVENT_LUD_KEYS > +}; > + > static const struct wmi_device_id ideapad_wmi_ids[] = { > { "26CAB2E5-5CF1-46AE-AAC3-4A12B6BA50E6", &ideapad_wmi_context_esc }, /* Yoga 3 */ > { "56322276-8493-4CE8-A783-98C991274F5E", &ideapad_wmi_context_esc }, /* Yoga 700 */ > - { "8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", &ideapad_wmi_context_fn_keys }, /* Legion 5 */ > + { "8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", &ideapad_wmi_context_fn_keys }, /* FN keys */ > + { "CE6C0974-0407-4F50-88BA-4FC3B6559AD8", > +&ideapad_wmi_context_LUD_keys }, /* Util data */ > + > {}, > }; > MODULE_DEVICE_TABLE(wmi, ideapad_wmi_ids);
Am 24.12.24 um 10:29 schrieb Jackie EG1 Dong: > Dear Armin, > CE6C0974-0407-4F50-88BA-4FC3B6559AD8 is a WMI interface method. > From Lenovo keyboard WMI specification, I found it applicable to LNB products, i.e. YOGA/XiaoXin/Gaming/ThinkBook etc and performs the Lenovo customized hotkeys function for Consumer and SMB notebooks. > I implemented the audio mute LED and mic mute LED function of IdeaPad notebook in ideapad_laptop driver, because I found the mute LED function of Thinkpad notebook is implemented by thinkpad_acpi. And ideapad_laptop driver has the similar function as thinkpad_acpi. > > Thanks, > Jackie Dong Please do not top-post, see https://subspace.kernel.org/etiquette.html for details. If the CE6C0974-0407-4F50-88BA-4FC3B6559AD8 is a WMI method device, when setting it up as a WMI event device is invalid. I will see if i can modify the WMI driver core to prevent this from happening in the future. Regarding the ideapad_laptop and thinkpad_acpi drivers: those drivers are using the legacy GUID-based WMI API, so they tend to handle multiple WMI GUIDs at the same time. This style of writing WMI drivers is deprecated, as it is prone to lifetime issues. I suggest you write a separate WMI driver for the CE6C0974-0407-4F50-88BA-4FC3B6559AD8 WMI device which just takes care of the LED devices. The documentation for writing WMI drivers also specifies a example driver which might be useful as a starting point. Looking forward for the second revision of the patch series :). Thanks, Armin Wolf > -----Original Message----- > From: Armin Wolf <W_Armin@gmx.de> > Sent: Monday, December 23, 2024 6:34 AM > To: Jackie Dong <xy-jackie@139.com>; ike.pan@canonical.com; hdegoede@redhat.com; ilpo.jarvinen@linux.intel.com; perex@perex.cz; tiwai@suse.com; bo.liu@senarytech.com; kovalev@altlinux.org; me@oldherl.one; jaroslaw.janik@gmail.com; cs@tuxedo.de; songxiebing@kylinos.cn; kailang@realtek.com; sbinding@opensource.cirrus.com; simont@opensource.cirrus.com; josh@joshuagrisham.com; rf@opensource.cirrus.com > Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; linux-sound@vger.kernel.org; mpearson-lenovo@squebb.ca; waterflowdeg@gmail.com; Jackie EG1 Dong <dongeg1@lenovo.com> > Subject: [External] Re: [PATCH 1/2] platform/x86: ideapad-laptop: Support for mic and audio leds. > > Am 19.12.24 um 11:15 schrieb Jackie Dong: > >> Implement Lenovo utility data WMI calls needed to make LEDs work on >> Ideapads that support this GUID. >> This enables the mic and audio LEDs to be updated correctly. >> >> Tested on below samples. >> ThinkBook 13X Gen4 IMH >> ThinkBook 14 G6 ABP >> ThinkBook 16p Gen4-21J8 >> ThinkBook 16p Gen8-IRL >> ThinkBook 16p G7+ ASP > Hi, > > i am a bit confused regarding the role of the CE6C0974-0407-4F50-88BA-4FC3B6559AD8 WMI device: > > - is it a event or a method block? > > - is it in some way connected with the remaining WMI devices supported by the ideapad-laptop driver? > > Looking at the code it seems to me that the CE6C0974-0407-4F50-88BA-4FC3B6559AD8 WMI device is not a event device and is not directly connected with the remaining WMI devices (please correct me if i am wrong). > > Can you please write a separate driver for this WMI device? Getting the ideapad-laptop driver involved in this seems to be unreasonable since the audio led functionality does not interact with the remaining driver. > > This might be helpful: https://docs.kernel.org/wmi/driver-development-guide.html. > >> Suggested-by: Mark Pearson <mpearson-lenovo@squebb.ca> >> Signed-off-by: Jackie Dong <xy-jackie@139.com> >> Signed-off-by: Jackie Dong <dongeg1@lenovo.com> > Please keep only the Signed-of tag with the email address used for sending this patch. > > Besides that its always nice to see vendors getting involved with upstream :). > > Thanks, > Armin Wolf > >> --- >> drivers/platform/x86/ideapad-laptop.c | 157 +++++++++++++++++++++++++- >> 1 file changed, 156 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/platform/x86/ideapad-laptop.c >> b/drivers/platform/x86/ideapad-laptop.c >> index c64dfc56651d..acea4aa8eac3 100644 >> --- a/drivers/platform/x86/ideapad-laptop.c >> +++ b/drivers/platform/x86/ideapad-laptop.c >> @@ -32,6 +32,7 @@ >> #include <linux/sysfs.h> >> #include <linux/types.h> >> #include <linux/wmi.h> >> +#include <sound/control.h> >> #include "ideapad-laptop.h" >> >> #include <acpi/video.h> >> @@ -1298,6 +1299,15 @@ static const struct key_entry ideapad_keymap[] = { >> { KE_END }, >> }; >> >> +/* >> + * Input parameters to mute/unmute audio LED and Mic LED */ struct >> +wmi_led_args { >> + u8 ID; >> + u8 SubID; >> + u16 Value; >> +}; >> + >> static int ideapad_input_init(struct ideapad_private *priv) >> { >> struct input_dev *inputdev; >> @@ -2023,15 +2033,145 @@ static void ideapad_check_features(struct ideapad_private *priv) >> /* >> * WMI driver >> */ >> +#define IDEAPAD_ACPI_LED_MAX (((SNDRV_CTL_ELEM_ACCESS_MIC_LED -\ >> + SNDRV_CTL_ELEM_ACCESS_SPK_LED) >> SNDRV_CTL_ELEM_ACCESS_LED_SHIFT) >> ++ 1) >> + >> enum ideapad_wmi_event_type { >> IDEAPAD_WMI_EVENT_ESC, >> IDEAPAD_WMI_EVENT_FN_KEYS, >> + IDEAPAD_WMI_EVENT_LUD_KEYS, >> }; >> >> +#define WMI_LUD_GET_SUPPORT 1 >> +#define WMI_LUD_SET_FEATURE 2 >> + >> +#define WMI_LUD_GET_MICMUTE_LED_VER 20 >> +#define WMI_LUD_GET_AUDIOMUTE_LED_VER 26 >> + >> +#define WMI_LUD_SUPPORT_MICMUTE_LED_VER 25 >> +#define WMI_LUD_SUPPORT_AUDIOMUTE_LED_VER 27 >> + >> struct ideapad_wmi_private { >> enum ideapad_wmi_event_type event; >> + struct led_classdev cdev[IDEAPAD_ACPI_LED_MAX]; >> }; >> >> +static struct wmi_device *led_wdev; >> + >> +enum mute_led_type { >> + MIC_MUTE, >> + AUDIO_MUTE, >> +}; >> + >> +static int ideapad_wmi_mute_led_set(enum mute_led_type led_type, struct led_classdev *led_cdev, >> + enum led_brightness brightness) >> + >> +{ >> + struct wmi_led_args led_arg = {0, 0, 0}; >> + struct acpi_buffer input; >> + acpi_status status; >> + >> + if (led_type == MIC_MUTE) >> + led_arg.ID = brightness == LED_ON ? 1 : 2; >> + else if (led_type == AUDIO_MUTE) >> + led_arg.ID = brightness == LED_ON ? 4 : 5; >> + else >> + return -EINVAL; >> + >> + input.length = sizeof(struct wmi_led_args); >> + input.pointer = &led_arg; >> + status = wmidev_evaluate_method(led_wdev, 0, WMI_LUD_SET_FEATURE, >> +&input, NULL); >> + >> + if (ACPI_FAILURE(status)) >> + return -EIO; >> + >> + return 0; >> +} >> + >> +static int ideapad_wmi_audiomute_led_set(struct led_classdev *led_cdev, >> + enum led_brightness brightness) >> + >> +{ >> + return ideapad_wmi_mute_led_set(AUDIO_MUTE, led_cdev, brightness); } >> + >> +static int ideapad_wmi_micmute_led_set(struct led_classdev *led_cdev, >> + enum led_brightness brightness) { >> + return ideapad_wmi_mute_led_set(MIC_MUTE, led_cdev, brightness); } >> + >> +static int ideapad_wmi_leds_init(enum mute_led_type led_type, struct >> +device *dev) { >> + struct ideapad_wmi_private *wpriv = dev_get_drvdata(dev); >> + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; >> + struct acpi_buffer input; >> + union acpi_object *obj; >> + int led_version, err = 0; >> + unsigned int wmiarg; >> + acpi_status status; >> + >> + if (led_type == MIC_MUTE) >> + wmiarg = WMI_LUD_GET_MICMUTE_LED_VER; >> + else if (led_type == AUDIO_MUTE) >> + wmiarg = WMI_LUD_GET_AUDIOMUTE_LED_VER; >> + else >> + return -EINVAL; >> + >> + input.length = sizeof(wmiarg); >> + input.pointer = &wmiarg; >> + status = wmidev_evaluate_method(led_wdev, 0, WMI_LUD_GET_SUPPORT, &input, &output); >> + if (ACPI_FAILURE(status)) { >> + kfree(output.pointer); >> + return -EIO; >> + } >> + obj = output.pointer; >> + led_version = obj->integer.value; >> + >> + wpriv->cdev[led_type].max_brightness = LED_ON; >> + wpriv->cdev[led_type].dev = dev; >> + wpriv->cdev[led_type].flags = LED_CORE_SUSPENDRESUME; >> + >> + if (led_type == MIC_MUTE) { >> + if (led_version != WMI_LUD_SUPPORT_MICMUTE_LED_VER) { >> + dev_info(dev, "This product doesn't support mic mute LED.\n"); >> + return -EIO; >> + } >> + wpriv->cdev[led_type].name = "platform::micmute"; >> + wpriv->cdev[led_type].brightness_set_blocking = &ideapad_wmi_micmute_led_set; >> + wpriv->cdev[led_type].default_trigger = "audio-micmute"; >> + >> + err = devm_led_classdev_register(dev, &wpriv->cdev[led_type]); >> + if (err < 0) { >> + dev_err(dev, "Could not register mic mute LED : %d\n", err); >> + led_classdev_unregister(&wpriv->cdev[led_type]); >> + } >> + } else { >> + if (led_version != WMI_LUD_SUPPORT_AUDIOMUTE_LED_VER) { >> + dev_info(dev, "This product doesn't support audio mute LED.\n"); >> + return -EIO; >> + } >> + wpriv->cdev[led_type].name = "platform::mute"; >> + wpriv->cdev[led_type].brightness_set_blocking = &ideapad_wmi_audiomute_led_set; >> + wpriv->cdev[led_type].default_trigger = "audio-mute"; >> + >> + err = devm_led_classdev_register(dev, &wpriv->cdev[led_type]); >> + if (err < 0) { >> + dev_err(dev, "Could not register audio mute LED: %d\n", err); >> + led_classdev_unregister(&wpriv->cdev[led_type]); >> + } >> + } >> + >> + kfree(obj); >> + return err; >> +} >> + >> +static void ideapad_wmi_leds_setup(struct device *dev) { >> + ideapad_wmi_leds_init(MIC_MUTE, dev); >> + ideapad_wmi_leds_init(AUDIO_MUTE, dev); } >> + >> static int ideapad_wmi_probe(struct wmi_device *wdev, const void *context) >> { >> struct ideapad_wmi_private *wpriv; >> @@ -2043,6 +2183,12 @@ static int ideapad_wmi_probe(struct wmi_device *wdev, const void *context) >> *wpriv = *(const struct ideapad_wmi_private *)context; >> >> dev_set_drvdata(&wdev->dev, wpriv); >> + >> + if (wpriv->event == IDEAPAD_WMI_EVENT_LUD_KEYS) { >> + led_wdev = wdev; >> + ideapad_wmi_leds_setup(&wdev->dev); >> + } >> + >> return 0; >> } >> >> @@ -2088,6 +2234,9 @@ static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data) >> data->integer.value | IDEAPAD_WMI_KEY); >> >> break; >> + case IDEAPAD_WMI_EVENT_LUD_KEYS: >> + break; >> + >> } >> } >> >> @@ -2099,10 +2248,16 @@ static const struct ideapad_wmi_private ideapad_wmi_context_fn_keys = { >> .event = IDEAPAD_WMI_EVENT_FN_KEYS >> }; >> >> +static const struct ideapad_wmi_private ideapad_wmi_context_LUD_keys = { >> + .event = IDEAPAD_WMI_EVENT_LUD_KEYS >> +}; >> + >> static const struct wmi_device_id ideapad_wmi_ids[] = { >> { "26CAB2E5-5CF1-46AE-AAC3-4A12B6BA50E6", &ideapad_wmi_context_esc }, /* Yoga 3 */ >> { "56322276-8493-4CE8-A783-98C991274F5E", &ideapad_wmi_context_esc }, /* Yoga 700 */ >> - { "8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", &ideapad_wmi_context_fn_keys }, /* Legion 5 */ >> + { "8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", &ideapad_wmi_context_fn_keys }, /* FN keys */ >> + { "CE6C0974-0407-4F50-88BA-4FC3B6559AD8", >> +&ideapad_wmi_context_LUD_keys }, /* Util data */ >> + >> {}, >> }; >> MODULE_DEVICE_TABLE(wmi, ideapad_wmi_ids);
On 2024/12/24 21:09, Armin Wolf wrote: > Am 24.12.24 um 10:29 schrieb Jackie EG1 Dong: > >> Dear Armin, >> CE6C0974-0407-4F50-88BA-4FC3B6559AD8 is a WMI interface method. >> From Lenovo keyboard WMI specification, I found it applicable to >> LNB products, i.e. YOGA/XiaoXin/Gaming/ThinkBook etc and performs the >> Lenovo customized hotkeys function for Consumer and SMB notebooks. >> I implemented the audio mute LED and mic mute LED function of >> IdeaPad notebook in ideapad_laptop driver, because I found the mute >> LED function of Thinkpad notebook is implemented by thinkpad_acpi. And >> ideapad_laptop driver has the similar function as thinkpad_acpi. >> >> Thanks, >> Jackie Dong > > Please do not top-post, see https://subspace.kernel.org/etiquette.html > for details. Thanks Armin for your link, I have read and follow it in future. > > If the CE6C0974-0407-4F50-88BA-4FC3B6559AD8 is a WMI method device, when > setting it up as a WMI event device is invalid. I will see if i can > modify the WMI driver core to prevent > this from happening in the future. > > Regarding the ideapad_laptop and thinkpad_acpi drivers: those drivers > are using the legacy GUID-based WMI API, so they tend to handle multiple > WMI GUIDs at the same time. > This style of writing WMI drivers is deprecated, as it is prone to > lifetime issues. > > I suggest you write a separate WMI driver for the > CE6C0974-0407-4F50-88BA-4FC3B6559AD8 WMI device which just takes care of > the LED devices. The documentation for writing WMI drivers > also specifies a example driver which might be useful as a starting point. > > Looking forward for the second revision of the patch series :).I'll write a separate WMI driver for this function if I don't get any other maintainers comment. Maybe it spend more time to finish it. Many thanks, Jackie Dong > > Thanks, > Armin Wolf > >> -----Original Message----- >> From: Armin Wolf <W_Armin@gmx.de> >> Sent: Monday, December 23, 2024 6:34 AM >> To: Jackie Dong <xy-jackie@139.com>; ike.pan@canonical.com; >> hdegoede@redhat.com; ilpo.jarvinen@linux.intel.com; perex@perex.cz; >> tiwai@suse.com; bo.liu@senarytech.com; kovalev@altlinux.org; >> me@oldherl.one; jaroslaw.janik@gmail.com; cs@tuxedo.de; >> songxiebing@kylinos.cn; kailang@realtek.com; >> sbinding@opensource.cirrus.com; simont@opensource.cirrus.com; >> josh@joshuagrisham.com; rf@opensource.cirrus.com >> Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; >> linux-sound@vger.kernel.org; mpearson-lenovo@squebb.ca; >> waterflowdeg@gmail.com; Jackie EG1 Dong <dongeg1@lenovo.com> >> Subject: [External] Re: [PATCH 1/2] platform/x86: ideapad-laptop: >> Support for mic and audio leds. >> >> Am 19.12.24 um 11:15 schrieb Jackie Dong: >> >>> Implement Lenovo utility data WMI calls needed to make LEDs work on >>> Ideapads that support this GUID. >>> This enables the mic and audio LEDs to be updated correctly. >>> >>> Tested on below samples. >>> ThinkBook 13X Gen4 IMH >>> ThinkBook 14 G6 ABP >>> ThinkBook 16p Gen4-21J8 >>> ThinkBook 16p Gen8-IRL >>> ThinkBook 16p G7+ ASP >> Hi, >> >> i am a bit confused regarding the role of the >> CE6C0974-0407-4F50-88BA-4FC3B6559AD8 WMI device: >> >> - is it a event or a method block? >> >> - is it in some way connected with the remaining WMI devices supported >> by the ideapad-laptop driver? >> >> Looking at the code it seems to me that the >> CE6C0974-0407-4F50-88BA-4FC3B6559AD8 WMI device is not a event device >> and is not directly connected with the remaining WMI devices (please >> correct me if i am wrong). >> >> Can you please write a separate driver for this WMI device? Getting >> the ideapad-laptop driver involved in this seems to be unreasonable >> since the audio led functionality does not interact with the remaining >> driver. >> >> This might be helpful: >> https://docs.kernel.org/wmi/driver-development-guide.html. >> >>> Suggested-by: Mark Pearson <mpearson-lenovo@squebb.ca> >>> Signed-off-by: Jackie Dong <xy-jackie@139.com> >>> Signed-off-by: Jackie Dong <dongeg1@lenovo.com> >> Please keep only the Signed-of tag with the email address used for >> sending this patch. >> >> Besides that its always nice to see vendors getting involved with >> upstream :). >> >> Thanks, >> Armin Wolf >> >>> --- >>> drivers/platform/x86/ideapad-laptop.c | 157 >>> +++++++++++++++++++++++++- >>> 1 file changed, 156 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/platform/x86/ideapad-laptop.c >>> b/drivers/platform/x86/ideapad-laptop.c >>> index c64dfc56651d..acea4aa8eac3 100644 >>> --- a/drivers/platform/x86/ideapad-laptop.c >>> +++ b/drivers/platform/x86/ideapad-laptop.c >>> @@ -32,6 +32,7 @@ >>> #include <linux/sysfs.h> >>> #include <linux/types.h> >>> #include <linux/wmi.h> >>> +#include <sound/control.h> >>> #include "ideapad-laptop.h" >>> >>> #include <acpi/video.h> >>> @@ -1298,6 +1299,15 @@ static const struct key_entry ideapad_keymap[] >>> = { >>> { KE_END }, >>> }; >>> >>> +/* >>> + * Input parameters to mute/unmute audio LED and Mic LED */ struct >>> +wmi_led_args { >>> + u8 ID; >>> + u8 SubID; >>> + u16 Value; >>> +}; >>> + >>> static int ideapad_input_init(struct ideapad_private *priv) >>> { >>> struct input_dev *inputdev; >>> @@ -2023,15 +2033,145 @@ static void ideapad_check_features(struct >>> ideapad_private *priv) >>> /* >>> * WMI driver >>> */ >>> +#define IDEAPAD_ACPI_LED_MAX (((SNDRV_CTL_ELEM_ACCESS_MIC_LED -\ >>> + SNDRV_CTL_ELEM_ACCESS_SPK_LED) >> >>> SNDRV_CTL_ELEM_ACCESS_LED_SHIFT) >>> ++ 1) >>> + >>> enum ideapad_wmi_event_type { >>> IDEAPAD_WMI_EVENT_ESC, >>> IDEAPAD_WMI_EVENT_FN_KEYS, >>> + IDEAPAD_WMI_EVENT_LUD_KEYS, >>> }; >>> >>> +#define WMI_LUD_GET_SUPPORT 1 >>> +#define WMI_LUD_SET_FEATURE 2 >>> + >>> +#define WMI_LUD_GET_MICMUTE_LED_VER 20 >>> +#define WMI_LUD_GET_AUDIOMUTE_LED_VER 26 >>> + >>> +#define WMI_LUD_SUPPORT_MICMUTE_LED_VER 25 >>> +#define WMI_LUD_SUPPORT_AUDIOMUTE_LED_VER 27 >>> + >>> struct ideapad_wmi_private { >>> enum ideapad_wmi_event_type event; >>> + struct led_classdev cdev[IDEAPAD_ACPI_LED_MAX]; >>> }; >>> >>> +static struct wmi_device *led_wdev; >>> + >>> +enum mute_led_type { >>> + MIC_MUTE, >>> + AUDIO_MUTE, >>> +}; >>> + >>> +static int ideapad_wmi_mute_led_set(enum mute_led_type led_type, >>> struct led_classdev *led_cdev, >>> + enum led_brightness brightness) >>> + >>> +{ >>> + struct wmi_led_args led_arg = {0, 0, 0}; >>> + struct acpi_buffer input; >>> + acpi_status status; >>> + >>> + if (led_type == MIC_MUTE) >>> + led_arg.ID = brightness == LED_ON ? 1 : 2; >>> + else if (led_type == AUDIO_MUTE) >>> + led_arg.ID = brightness == LED_ON ? 4 : 5; >>> + else >>> + return -EINVAL; >>> + >>> + input.length = sizeof(struct wmi_led_args); >>> + input.pointer = &led_arg; >>> + status = wmidev_evaluate_method(led_wdev, 0, WMI_LUD_SET_FEATURE, >>> +&input, NULL); >>> + >>> + if (ACPI_FAILURE(status)) >>> + return -EIO; >>> + >>> + return 0; >>> +} >>> + >>> +static int ideapad_wmi_audiomute_led_set(struct led_classdev *led_cdev, >>> + enum led_brightness brightness) >>> + >>> +{ >>> + return ideapad_wmi_mute_led_set(AUDIO_MUTE, led_cdev, >>> brightness); } >>> + >>> +static int ideapad_wmi_micmute_led_set(struct led_classdev *led_cdev, >>> + enum led_brightness brightness) { >>> + return ideapad_wmi_mute_led_set(MIC_MUTE, led_cdev, brightness); } >>> + >>> +static int ideapad_wmi_leds_init(enum mute_led_type led_type, struct >>> +device *dev) { >>> + struct ideapad_wmi_private *wpriv = dev_get_drvdata(dev); >>> + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; >>> + struct acpi_buffer input; >>> + union acpi_object *obj; >>> + int led_version, err = 0; >>> + unsigned int wmiarg; >>> + acpi_status status; >>> + >>> + if (led_type == MIC_MUTE) >>> + wmiarg = WMI_LUD_GET_MICMUTE_LED_VER; >>> + else if (led_type == AUDIO_MUTE) >>> + wmiarg = WMI_LUD_GET_AUDIOMUTE_LED_VER; >>> + else >>> + return -EINVAL; >>> + >>> + input.length = sizeof(wmiarg); >>> + input.pointer = &wmiarg; >>> + status = wmidev_evaluate_method(led_wdev, 0, >>> WMI_LUD_GET_SUPPORT, &input, &output); >>> + if (ACPI_FAILURE(status)) { >>> + kfree(output.pointer); >>> + return -EIO; >>> + } >>> + obj = output.pointer; >>> + led_version = obj->integer.value; >>> + >>> + wpriv->cdev[led_type].max_brightness = LED_ON; >>> + wpriv->cdev[led_type].dev = dev; >>> + wpriv->cdev[led_type].flags = LED_CORE_SUSPENDRESUME; >>> + >>> + if (led_type == MIC_MUTE) { >>> + if (led_version != WMI_LUD_SUPPORT_MICMUTE_LED_VER) { >>> + dev_info(dev, "This product doesn't support mic >>> mute LED.\n"); >>> + return -EIO; >>> + } >>> + wpriv->cdev[led_type].name = "platform::micmute"; >>> + wpriv->cdev[led_type].brightness_set_blocking = >>> &ideapad_wmi_micmute_led_set; >>> + wpriv->cdev[led_type].default_trigger = "audio-micmute"; >>> + >>> + err = devm_led_classdev_register(dev, >>> &wpriv->cdev[led_type]); >>> + if (err < 0) { >>> + dev_err(dev, "Could not register mic mute LED : >>> %d\n", err); >>> + led_classdev_unregister(&wpriv->cdev[led_type]); >>> + } >>> + } else { >>> + if (led_version != WMI_LUD_SUPPORT_AUDIOMUTE_LED_VER) { >>> + dev_info(dev, "This product doesn't support >>> audio mute LED.\n"); >>> + return -EIO; >>> + } >>> + wpriv->cdev[led_type].name = "platform::mute"; >>> + wpriv->cdev[led_type].brightness_set_blocking = >>> &ideapad_wmi_audiomute_led_set; >>> + wpriv->cdev[led_type].default_trigger = "audio-mute"; >>> + >>> + err = devm_led_classdev_register(dev, >>> &wpriv->cdev[led_type]); >>> + if (err < 0) { >>> + dev_err(dev, "Could not register audio mute >>> LED: %d\n", err); >>> + led_classdev_unregister(&wpriv->cdev[led_type]); >>> + } >>> + } >>> + >>> + kfree(obj); >>> + return err; >>> +} >>> + >>> +static void ideapad_wmi_leds_setup(struct device *dev) { >>> + ideapad_wmi_leds_init(MIC_MUTE, dev); >>> + ideapad_wmi_leds_init(AUDIO_MUTE, dev); } >>> + >>> static int ideapad_wmi_probe(struct wmi_device *wdev, const void >>> *context) >>> { >>> struct ideapad_wmi_private *wpriv; >>> @@ -2043,6 +2183,12 @@ static int ideapad_wmi_probe(struct wmi_device >>> *wdev, const void *context) >>> *wpriv = *(const struct ideapad_wmi_private *)context; >>> >>> dev_set_drvdata(&wdev->dev, wpriv); >>> + >>> + if (wpriv->event == IDEAPAD_WMI_EVENT_LUD_KEYS) { >>> + led_wdev = wdev; >>> + ideapad_wmi_leds_setup(&wdev->dev); >>> + } >>> + >>> return 0; >>> } >>> >>> @@ -2088,6 +2234,9 @@ static void ideapad_wmi_notify(struct >>> wmi_device *wdev, union acpi_object *data) >>> data->integer.value | >>> IDEAPAD_WMI_KEY); >>> >>> break; >>> + case IDEAPAD_WMI_EVENT_LUD_KEYS: >>> + break; >>> + >>> } >>> } >>> >>> @@ -2099,10 +2248,16 @@ static const struct ideapad_wmi_private >>> ideapad_wmi_context_fn_keys = { >>> .event = IDEAPAD_WMI_EVENT_FN_KEYS >>> }; >>> >>> +static const struct ideapad_wmi_private ideapad_wmi_context_LUD_keys >>> = { >>> + .event = IDEAPAD_WMI_EVENT_LUD_KEYS >>> +}; >>> + >>> static const struct wmi_device_id ideapad_wmi_ids[] = { >>> { "26CAB2E5-5CF1-46AE-AAC3-4A12B6BA50E6", >>> &ideapad_wmi_context_esc }, /* Yoga 3 */ >>> { "56322276-8493-4CE8-A783-98C991274F5E", >>> &ideapad_wmi_context_esc }, /* Yoga 700 */ >>> - { "8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", >>> &ideapad_wmi_context_fn_keys }, /* Legion 5 */ >>> + { "8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", >>> &ideapad_wmi_context_fn_keys }, /* FN keys */ >>> + { "CE6C0974-0407-4F50-88BA-4FC3B6559AD8", >>> +&ideapad_wmi_context_LUD_keys }, /* Util data */ >>> + >>> {}, >>> }; >>> MODULE_DEVICE_TABLE(wmi, ideapad_wmi_ids);
diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c index c64dfc56651d..acea4aa8eac3 100644 --- a/drivers/platform/x86/ideapad-laptop.c +++ b/drivers/platform/x86/ideapad-laptop.c @@ -32,6 +32,7 @@ #include <linux/sysfs.h> #include <linux/types.h> #include <linux/wmi.h> +#include <sound/control.h> #include "ideapad-laptop.h" #include <acpi/video.h> @@ -1298,6 +1299,15 @@ static const struct key_entry ideapad_keymap[] = { { KE_END }, }; +/* + * Input parameters to mute/unmute audio LED and Mic LED + */ +struct wmi_led_args { + u8 ID; + u8 SubID; + u16 Value; +}; + static int ideapad_input_init(struct ideapad_private *priv) { struct input_dev *inputdev; @@ -2023,15 +2033,145 @@ static void ideapad_check_features(struct ideapad_private *priv) /* * WMI driver */ +#define IDEAPAD_ACPI_LED_MAX (((SNDRV_CTL_ELEM_ACCESS_MIC_LED -\ + SNDRV_CTL_ELEM_ACCESS_SPK_LED) >> SNDRV_CTL_ELEM_ACCESS_LED_SHIFT) + 1) + enum ideapad_wmi_event_type { IDEAPAD_WMI_EVENT_ESC, IDEAPAD_WMI_EVENT_FN_KEYS, + IDEAPAD_WMI_EVENT_LUD_KEYS, }; +#define WMI_LUD_GET_SUPPORT 1 +#define WMI_LUD_SET_FEATURE 2 + +#define WMI_LUD_GET_MICMUTE_LED_VER 20 +#define WMI_LUD_GET_AUDIOMUTE_LED_VER 26 + +#define WMI_LUD_SUPPORT_MICMUTE_LED_VER 25 +#define WMI_LUD_SUPPORT_AUDIOMUTE_LED_VER 27 + struct ideapad_wmi_private { enum ideapad_wmi_event_type event; + struct led_classdev cdev[IDEAPAD_ACPI_LED_MAX]; }; +static struct wmi_device *led_wdev; + +enum mute_led_type { + MIC_MUTE, + AUDIO_MUTE, +}; + +static int ideapad_wmi_mute_led_set(enum mute_led_type led_type, struct led_classdev *led_cdev, + enum led_brightness brightness) + +{ + struct wmi_led_args led_arg = {0, 0, 0}; + struct acpi_buffer input; + acpi_status status; + + if (led_type == MIC_MUTE) + led_arg.ID = brightness == LED_ON ? 1 : 2; + else if (led_type == AUDIO_MUTE) + led_arg.ID = brightness == LED_ON ? 4 : 5; + else + return -EINVAL; + + input.length = sizeof(struct wmi_led_args); + input.pointer = &led_arg; + status = wmidev_evaluate_method(led_wdev, 0, WMI_LUD_SET_FEATURE, &input, NULL); + + if (ACPI_FAILURE(status)) + return -EIO; + + return 0; +} + +static int ideapad_wmi_audiomute_led_set(struct led_classdev *led_cdev, + enum led_brightness brightness) + +{ + return ideapad_wmi_mute_led_set(AUDIO_MUTE, led_cdev, brightness); +} + +static int ideapad_wmi_micmute_led_set(struct led_classdev *led_cdev, + enum led_brightness brightness) +{ + return ideapad_wmi_mute_led_set(MIC_MUTE, led_cdev, brightness); +} + +static int ideapad_wmi_leds_init(enum mute_led_type led_type, struct device *dev) +{ + struct ideapad_wmi_private *wpriv = dev_get_drvdata(dev); + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; + struct acpi_buffer input; + union acpi_object *obj; + int led_version, err = 0; + unsigned int wmiarg; + acpi_status status; + + if (led_type == MIC_MUTE) + wmiarg = WMI_LUD_GET_MICMUTE_LED_VER; + else if (led_type == AUDIO_MUTE) + wmiarg = WMI_LUD_GET_AUDIOMUTE_LED_VER; + else + return -EINVAL; + + input.length = sizeof(wmiarg); + input.pointer = &wmiarg; + status = wmidev_evaluate_method(led_wdev, 0, WMI_LUD_GET_SUPPORT, &input, &output); + if (ACPI_FAILURE(status)) { + kfree(output.pointer); + return -EIO; + } + obj = output.pointer; + led_version = obj->integer.value; + + wpriv->cdev[led_type].max_brightness = LED_ON; + wpriv->cdev[led_type].dev = dev; + wpriv->cdev[led_type].flags = LED_CORE_SUSPENDRESUME; + + if (led_type == MIC_MUTE) { + if (led_version != WMI_LUD_SUPPORT_MICMUTE_LED_VER) { + dev_info(dev, "This product doesn't support mic mute LED.\n"); + return -EIO; + } + wpriv->cdev[led_type].name = "platform::micmute"; + wpriv->cdev[led_type].brightness_set_blocking = &ideapad_wmi_micmute_led_set; + wpriv->cdev[led_type].default_trigger = "audio-micmute"; + + err = devm_led_classdev_register(dev, &wpriv->cdev[led_type]); + if (err < 0) { + dev_err(dev, "Could not register mic mute LED : %d\n", err); + led_classdev_unregister(&wpriv->cdev[led_type]); + } + } else { + if (led_version != WMI_LUD_SUPPORT_AUDIOMUTE_LED_VER) { + dev_info(dev, "This product doesn't support audio mute LED.\n"); + return -EIO; + } + wpriv->cdev[led_type].name = "platform::mute"; + wpriv->cdev[led_type].brightness_set_blocking = &ideapad_wmi_audiomute_led_set; + wpriv->cdev[led_type].default_trigger = "audio-mute"; + + err = devm_led_classdev_register(dev, &wpriv->cdev[led_type]); + if (err < 0) { + dev_err(dev, "Could not register audio mute LED: %d\n", err); + led_classdev_unregister(&wpriv->cdev[led_type]); + } + } + + kfree(obj); + return err; +} + +static void ideapad_wmi_leds_setup(struct device *dev) +{ + ideapad_wmi_leds_init(MIC_MUTE, dev); + ideapad_wmi_leds_init(AUDIO_MUTE, dev); +} + static int ideapad_wmi_probe(struct wmi_device *wdev, const void *context) { struct ideapad_wmi_private *wpriv; @@ -2043,6 +2183,12 @@ static int ideapad_wmi_probe(struct wmi_device *wdev, const void *context) *wpriv = *(const struct ideapad_wmi_private *)context; dev_set_drvdata(&wdev->dev, wpriv); + + if (wpriv->event == IDEAPAD_WMI_EVENT_LUD_KEYS) { + led_wdev = wdev; + ideapad_wmi_leds_setup(&wdev->dev); + } + return 0; } @@ -2088,6 +2234,9 @@ static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data) data->integer.value | IDEAPAD_WMI_KEY); break; + case IDEAPAD_WMI_EVENT_LUD_KEYS: + break; + } } @@ -2099,10 +2248,16 @@ static const struct ideapad_wmi_private ideapad_wmi_context_fn_keys = { .event = IDEAPAD_WMI_EVENT_FN_KEYS }; +static const struct ideapad_wmi_private ideapad_wmi_context_LUD_keys = { + .event = IDEAPAD_WMI_EVENT_LUD_KEYS +}; + static const struct wmi_device_id ideapad_wmi_ids[] = { { "26CAB2E5-5CF1-46AE-AAC3-4A12B6BA50E6", &ideapad_wmi_context_esc }, /* Yoga 3 */ { "56322276-8493-4CE8-A783-98C991274F5E", &ideapad_wmi_context_esc }, /* Yoga 700 */ - { "8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", &ideapad_wmi_context_fn_keys }, /* Legion 5 */ + { "8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", &ideapad_wmi_context_fn_keys }, /* FN keys */ + { "CE6C0974-0407-4F50-88BA-4FC3B6559AD8", &ideapad_wmi_context_LUD_keys }, /* Util data */ + {}, }; MODULE_DEVICE_TABLE(wmi, ideapad_wmi_ids);