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

[Jackie Dong <xy-jackie@139.com>]

On 7/8/25 18:07, 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.
>   
Hi Ilpo,
    I have deleted the Reviewed-by tag in next version patch.
>> ---
>> 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;
> 
It's good suggestion, I'll update it.
>> -	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).
> 
I have deleted the "now" in next version patch.
>> +			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.
> 
I have deleted the "now" in next version patch.
>> +			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);
>>
> 
Thanks a lot,
Jackie