Re: [PATCH v3] lenovo-wmi-hotkey: Fixed a kernel error report for some Lenovo non-ThinkPad devices

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4203 bytes --]

In addition, the shortlog should mention the error relates to not 
supported mute LEDs.

I'd also drop "some Lenovo" from to make it shorter, I think the 
explanation that is already in the changelog covers that (we cannot 
put everything to the shortlog to keep it relatively short).

-- 
 i.

On Tue, 8 Jul 2025, Ilpo Järvinen wrote:

> On Tue, 8 Jul 2025, Jackie Dong wrote:
> 
> > Not all of Lenovo non-ThinkPad devices support both mic mute LED (on F4)
> > and audio mute LED (on F1). Some of them only support one mute LED, some
> > of them don't have any mute LED. Add a decision to judge this device
> > support mute LED or not. Without this decision, not support both of mic
> > mute LED and audio mute LED Lenovo non-ThinkPad brand devices (including
> > Ideapad/Yoga/Xiaoxin/Gaming/ThinkBook, etc.) will report a failed message
> > with error -5.
> > 
> > Signed-off-by: Jackie Dong <xy-jackie@139.com>
> > Suggested-by: Hans de Goede <hansg@kernel.org>
> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 
> Hi Jackie,
> 
> Please don't add Reviewed-by nor Tested-by tags unless they were 
> explicitly given to you.
>  
> > ---
> > Changes in v3:
> >  - Reverse orignal logic  (obj && obj->type == ACPI_TYPE_INTEGER) 
> >    and add new decision for led_version == 0.
> >  - Optimize the descriptions based on reviewer comments.
> > 
> > Changes in v2:
> >  - Add warning message and then return 0 if the device support mute LED
> >    abnormaly, based on Hans suggestion and Armin previous patch.
> > 
> > 
> >  .../x86/lenovo-wmi-hotkey-utilities.c         | 24 ++++++++++++++-----
> >  1 file changed, 18 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/lenovo-wmi-hotkey-utilities.c b/drivers/platform/x86/lenovo-wmi-hotkey-utilities.c
> > index 89153afd7015..1850992f2ea8 100644
> > --- a/drivers/platform/x86/lenovo-wmi-hotkey-utilities.c
> > +++ b/drivers/platform/x86/lenovo-wmi-hotkey-utilities.c
> > @@ -127,21 +127,30 @@ static int lenovo_super_hotkey_wmi_led_init(enum mute_led_type led_type, struct
> >  	else
> >  		return -EIO;
> 
> The logic was not reversed as requested. Please change this code to:
> 
> 	union acpi_object *obj __free(kfree) = output.pointer;
> 	if (!obj || obj->type != ACPI_TYPE_INTEGER)
> 		return -EIO;
> 
> 	led_version = obj->integer.value;
> 
> > -	wpriv->cdev[led_type].max_brightness = LED_ON;
> > -	wpriv->cdev[led_type].flags = LED_CORE_SUSPENDRESUME;
> > +	/*
> > +	 * Output parameters define: 0 means mute LED is not supported, Non-zero means
> > +	 * mute LED can be supported.
> > +	 */
> > +	if (led_version == 0)
> > +		return 0;
> > +
> >  
> >  	switch (led_type) {
> >  	case MIC_MUTE:
> > -		if (led_version != WMI_LUD_SUPPORT_MICMUTE_LED_VER)
> > -			return -EIO;
> > +		if (led_version != WMI_LUD_SUPPORT_MICMUTE_LED_VER) {
> > +			pr_warn("The MIC_MUTE LED of this device isn't supported now.\n");
> 
> Drop "now" (or change it to more precise explanation why).
> 
> > +			return 0;
> > +		}
> >  
> >  		wpriv->cdev[led_type].name = "platform::micmute";
> >  		wpriv->cdev[led_type].brightness_set_blocking = &lsh_wmi_micmute_led_set;
> >  		wpriv->cdev[led_type].default_trigger = "audio-micmute";
> >  		break;
> >  	case AUDIO_MUTE:
> > -		if (led_version != WMI_LUD_SUPPORT_AUDIOMUTE_LED_VER)
> > -			return -EIO;
> > +		if (led_version != WMI_LUD_SUPPORT_AUDIOMUTE_LED_VER) {
> > +			pr_warn("The AUDIO_MUTE LED of this device isn't supported now.\n");
> 
> Ditto.
> 
> > +			return 0;
> > +		}
> >  
> >  		wpriv->cdev[led_type].name = "platform::mute";
> >  		wpriv->cdev[led_type].brightness_set_blocking = &lsh_wmi_audiomute_led_set;
> > @@ -152,6 +161,9 @@ static int lenovo_super_hotkey_wmi_led_init(enum mute_led_type led_type, struct
> >  		return -EINVAL;
> >  	}
> >  
> > +	wpriv->cdev[led_type].max_brightness = LED_ON;
> > +	wpriv->cdev[led_type].flags = LED_CORE_SUSPENDRESUME;
> > +
> >  	err = devm_led_classdev_register(dev, &wpriv->cdev[led_type]);
> >  	if (err < 0) {
> >  		dev_err(dev, "Could not register mute LED %d : %d\n", led_type, err);
> > 
> 
>