| Message ID | 20241219101531.35896-1-xy-jackie@139.com (mailing list archive) |
|---|---|
| State | New |
| 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
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);