Re: [PATCH] platform/x86: fujitsu-laptop: Fix radio LED detection

[Harvey]

Am 25.10.2017 um 08:33 schrieb Jonathan Woithe:
> On Wed, Oct 25, 2017 at 06:29:46AM +0200, Micha?? K??pie?? wrote:
>> Radio LED detection method implemented in commit 4f62568c1fcf
>> ("fujitsu-laptop: Support radio LED") turned out to be incorrect as it
>> causes a radio LED to be erroneously detected on a Fujitsu Lifebook E751
>> which has a slide switch (and thus no radio LED).  Use bit 17 of
>> flags_supported (the value returned by method S000 of ACPI device
>> FUJ02E3) to determine whether a radio LED is present as it seems to be a
>> more reliable indicator, based on comparing DSDT tables of four Fujitsu
>> Lifebook models (E744, E751, S7110, S8420).
>>
>> Reported-by: Heinrich Siebmanns <harv@xxxxxx>
>> Signed-off-by: Micha?? K??pie?? <kernel@xxxxxxxxxx>
> 
> This seems to be a reasonable approach given the most recent set of
> observations.  Assuming it tests ok on the E751:
> 
>   Reviewed-by: Jonathan Woithe <jwoithe@xxxxxxxxxx>

It does. I applied the patch against an Archlinux vanilla 4.13.9 kernel.
The resulting module is clean and the error messages are gone, while
anything else seems to work as expected.

So this looks ok for me.

Tested-by: Heinrich Siebmanns <harv@xxxxxx>

> Regards
>   jonathan
> 
>> ---
>> I do not have a Fujitsu laptop with a radio LED for testing, so I was
>> only able to check that this patch still does not cause a radio LED to
>> be detected on a Lifebook S7020.
>>
>> Harvey, could you please try this patch on your Lifebook E751 and see
>> whether the log messages you reported disappear?  I will be happy to
>> assist you off-list in case you need help with it.
>>
>>  drivers/platform/x86/fujitsu-laptop.c | 14 +++++++++-----
>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
>> index 56a8195096a2..2cfbd3fa5136 100644
>> --- a/drivers/platform/x86/fujitsu-laptop.c
>> +++ b/drivers/platform/x86/fujitsu-laptop.c
>> @@ -691,6 +691,7 @@ static enum led_brightness eco_led_get(struct led_classdev *cdev)
>>  
>>  static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
>>  {
>> +	struct fujitsu_laptop *priv = acpi_driver_data(device);
>>  	struct led_classdev *led;
>>  	int result;
>>  
>> @@ -724,12 +725,15 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
>>  	}
>>  
>>  	/*
>> -	 * BTNI bit 24 seems to indicate the presence of a radio toggle
>> -	 * button in place of a slide switch, and all such machines appear
>> -	 * to also have an RF LED.  Therefore use bit 24 as an indicator
>> -	 * that an RF LED is present.
>> +	 * Some Fujitsu laptops have a radio toggle button in place of a slide
>> +	 * switch and all such machines appear to also have an RF LED.  Based on
>> +	 * comparing DSDT tables of four Fujitsu Lifebook models (E744, E751,
>> +	 * S7110, S8420; the first one has a radio toggle button, the other
>> +	 * three have slide switches), bit 17 of flags_supported (the value
>> +	 * returned by method S000 of ACPI device FUJ02E3) seems to indicate
>> +	 * whether given model has a radio toggle button.
>>  	 */
>> -	if (call_fext_func(device, FUNC_BUTTONS, 0x0, 0x0, 0x0) & BIT(24)) {
>> +	if (priv->flags_supported & BIT(17)) {
>>  		led = devm_kzalloc(&device->dev, sizeof(*led), GFP_KERNEL);
>>  		if (!led)
>>  			return -ENOMEM;
>> -- 
>> 2.14.2
> 

-- 
I am root. If you see me laughing, you'd better have a backup!

Attachment: signature.asc
Description: OpenPGP digital signature