Re: [PATCH 1/1] x86/platform: classmate-laptop: Add cmpc_keys support for new classmate laptops

[Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>]

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

On Wed, Jun 22, 2011 at 08:43:25PM +0200, Carlos Alberto Lopez Perez wrote:
> On 06/21/2011 10:50 PM, Thadeu Lima de Souza Cascardo wrote:
> > On Tue, Jun 21, 2011 at 10:24:57PM +0200, Carlos Alberto Lopez Perez wrote:
> >> On 06/21/2011 09:50 PM, Thadeu Lima de Souza Cascardo wrote:
> >>> On Tue, Jun 21, 2011 at 09:40:01PM +0200, Carlos Alberto Lopez Perez wrote:
> >>>> The cmpc_keys ACPI driver from the classmate-laptop module fails to
> >>>> recognize the device on the 4th generation of classmate laptops (Atom N450)
> >>>> because the device ID is named "FNBT0000" (uppercase N) on this laptops.
> >>>>
> >>>> MODALIAS=acpi:FNBT0000:
> >>>>
> >>>> This patch makes the driver cmpc_keys recognize it:
> >>>>
> >>>> input: cmpc_keys as /devices/LNXSYSTM:00/device:00/FNBT0000:00/input/input18
> >>>>
> >>>> Signed-off-by: Carlos Alberto Lopez Perez <clopez@igalia.com>
> >>>> ---
> >>>>  drivers/platform/x86/classmate-laptop.c |    3 +++
> >>>>  1 files changed, 3 insertions(+), 0 deletions(-)
> >>>>
> >>>> diff --git a/drivers/platform/x86/classmate-laptop.c b/drivers/platform/x86/classmate-laptop.c
> >>>> index 94f93b6..03896cd 100644
> >>>> --- a/drivers/platform/x86/classmate-laptop.c
> >>>> +++ b/drivers/platform/x86/classmate-laptop.c
> >>>> @@ -40,6 +40,7 @@ struct cmpc_accel {
> >>>>  #define CMPC_TABLET_HID		"TBLT0000"
> >>>>  #define CMPC_IPML_HID	"IPML200"
> >>>>  #define CMPC_KEYS_HID		"FnBT0000"
> >>>> +#define CMPC_KEYS_4GEN_HID	"FNBT0000"
> >>>>  
> >>>>  /*
> >>>>   * Generic input device code.
> >>>> @@ -683,6 +684,7 @@ static int cmpc_keys_remove(struct acpi_device *acpi, int type)
> >>>>  
> >>>>  static const struct acpi_device_id cmpc_keys_device_ids[] = {
> >>>>  	{CMPC_KEYS_HID, 0},
> >>>> +	{CMPC_KEYS_4GEN_HID, 0},
> >>>>  	{"", 0}
> >>>>  };
> >>>>  
> >>>> @@ -754,6 +756,7 @@ static const struct acpi_device_id cmpc_device_ids[] = {
> >>>>  	{CMPC_TABLET_HID, 0},
> >>>>  	{CMPC_IPML_HID, 0},
> >>>>  	{CMPC_KEYS_HID, 0},
> >>>> +	{CMPC_KEYS_4GEN_HID, 0},
> >>>>  	{"", 0}
> >>>>  };
> >>>>  
> >>>> -- 
> >>>> 1.7.5.3
> >>>>
> >>>
> >>> Not-Acked-By: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>
> >>>
> >>> Recent Linux versions have "upper-cased" the device names, because ACPI
> >>> spec says devices must have upper-case letters. This is probably related
> >>> to this change, not because the device name has changed in recent
> >>> versions of the device. So the correct fix would be to either revert
> >>> this change in Linux or use only a single uppercase name for the device.
> >>>
> >>> Thanks anyway for this report. I have received one report in private a
> >>> while back, but was too lazy to discuss this on the list. Any pointers
> >>> on that? Easist way to go is to simply replace FnBT for FNBT.
> >>>
> >>> Regards,
> >>> Cascardo.
> >>
> >> The ACPI specification [1] says the following:
> >>
> >> """
> >> A valid PNP ID must be of the form “AAA####” where A is an uppercase letter
> >> and # is a hex digit.
> >> A valid ACPI ID must be of the form “ACPI####” where # is a hex digit.
> >> """
> >>
> >> It don't says that the ACPI ID must be uppercase.
> >>
> >> I don't have an older generation of the classmate laptop to test if the
> >> ACPI ID is FnBT or FNBT, but I guess that if it was working until now, is
> >> because the older generations have FnBT instead of FNBT.
> >>
> >> However I just found a previous discussion about this issue on LKML [2] and
> >> seems that the patch is included (commit: 72638f5).
> >>
> >> So... this means that the cmpc_keys module was not working for any
> >> classmate laptop since ~2.6.29 ?
> >>
> >> If that is the case then replacing FnBT with FNBT will be fine.
> >>
> >> Thanks!
> >>
> >> Best regards!
> >>
> > 
> > Commit 77b23f712bc40a65160e7d02b045f1562bb43ff1 has changed this
> > behaviour between 2.6.36 and 2.6.37. Could you try using 2.6.36 and see
> > if the device appears as FnBT? Also, you could try using acpidump and
> > look at the decompiled DSDT so we can confirm whether it is FnBT or
> > FNBT. Attached is a patch that, when comparing device and driver names
> > for ACPI, will do it case-insensitivily.
> > 
> 
> You are right. The real ACPI ID of the device is FnBT0000 but the kernel
> is uppercasing it.
> 
> # acpidump | grep -i -C1 FNBT0000
>   0b30: 00 0a 00 0a 14 5b 82 34 46 4e 42 54 08 5f 48 49  .....[.4FNBT._HI
>   0b40: 44 0d 46 6e 42 54 30 30 30 30 00 08 5f 55 49 44  D.FnBT0000.._UID
>   0b50: 0a 00 08 46 4e 4c 4b 0a 01 14 11 46 4e 43 54 01  ...FNLK....FNCT.
> 
> 
> [*] In kernels <=  2.6.36 (Tested in 2.6.32 and 2.6.36) the ACPI ID is
> registered as FnBT0000
> 
> # cat /sys/bus/acpi/devices/FnBT0000\:00/modalias
> acpi:FnBT0000:
> 
> And the cmpc_keys driver of the classmate-laptop module recognized the
> device as expected:
> 
> input: cmpc_keys as /devices/LNXSYSTM:00/LNXSYBUS:00/FnBT0000:00/input/input5
> 
> 
> 
> 
> [*] In kernels => 2.6.37 (Tested in 2.6.37 and 2.6.39) the ACPI ID is
> registered uppercased as FNBT0000 and therefore the cmpc_keys driver fails
> to recognize it.
> 
> # cat /sys/bus/acpi/devices/FNBT0000\:00/modalias
> acpi:FNBT0000:
> 
> 
> 
> [*] With the kernel 2.6.39.1 and your patch for drivers/acpi/scan.c the
> ACPI ID is registered uppercased as FNBT0000 *but* the cmpc_keys driver
> recognizes it.
> 
> # cat /sys/bus/acpi/devices/FNBT0000\:00/modalias
> acpi:FNBT0000:
> 
> input: cmpc_keys as /devices/LNXSYSTM:00/device:00/FNBT0000:00/input/input5
> 
> 
> [*] With the kernel 2.6.39.1 and the commit
> 77b23f712bc40a65160e7d02b045f1562bb43ff1 reverted happens the same that
> with kernels <= 2.6.36 (the ACPI ID is registered as FnBT0000)
> 
> 
> > So, we have three options here. Revert an ACPICA commit, apply a
> > different matching code for ACPI bus in Linux or tell all drivers they
> > must use an uppper-case name instead of the device name as it appears in
> > the ACPI tables.
> > 
> 
> Yes, we have three options here to solve the problem but I'm not sure which
> one is the correct.
> 
> At least I think it has been demonstrated that the ACPI ID is *not
> required* to be uppercase by the ACPI specification as you can see in the
> acpidump of this device (FnBT0000).
> 
> So probably the "orthodox" way of solving this problem is reverting the
> commit 77b23f712bc40a65160e7d02b045f1562bb43ff1 which is doing a wrong
> assumption (its not true that the ACPI ID _must_ be uppercase).
> 
> 
> Regards!
> 

I am copying the acpi list and Len Brown to see if they can tell us
which one should be the right solution:

1) reverting the patch in acpica that puts all names in uppercase
2) using a case-insensitive match in acpi bus driver
3) using uppercase name in classmate-laptop driver

Regards,
Cascardo.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]