Re: [PATCH] lis3lv02d: support both one- and two-byte sensors

[Giuseppe Bilotta <giuseppe.bilotta@gmail.com>]

On Wed, Feb 11, 2009 at 1:16 AM, Éric Piel <Eric.Piel@tremplin-utc.net> wrote:
> Giuseppe Bilotta schreef:
>
>> As pointed out by Eric, there's no need to jump through hoops to obtain
>> a signed 16-bit number from a signed 8-bit number.
>>
>> This patch is good for -mm
>>
>> Note also that this patch is only about 8 vs 16 bit data, so the axis
>> orientation for HP notebooks still has to be fixed in a separate patch,
>> but at least now I should be able to check it out properly.
>>
> Whoops, sorry for not catching it before, this part is not nice:
>
>> @@ -121,6 +125,21 @@ void lis3lv02d_poweron(acpi_handle handle)
>>  {
>>        adev.is_on = 1;
>>        adev.init(handle);
>> +       adev.read(handle, WHO_AM_I, &adev.whoami);
>> +       /* Tell apart LISxLV02Dy from LISx02Dy family by checking the LSB
>> +        * (0x3A vs 0x3B). TODO More sophisticated checks on other
>> registers
>> +        * could be implemented, for example to see if we have 2 or 3
>> axes, and
>> +        * configure the joystick accordingly.
>> +        */
>> +       if (adev.whoami & 1) {
>> +               printk(KERN_INFO DRIVER_NAME ": 1-byte sensor found\n");
>> +               adev.lis3lv02d_read = lis3lv02d_read_8;
>> +               adev.mdps_max_val = 128;
>> +       } else {
>> +               printk(KERN_INFO DRIVER_NAME ": 2-byte sensor found\n");
>> +               adev.lis3lv02d_read = lis3lv02d_read_16;
>> +               adev.mdps_max_val = 2048;
>> +       }
>>  }
>
> poweron() is called each time the joystick is opened, no need to check (and
> do a printk) so often! You need to move all this check to lis3lv02d_add() in
> hp_accel.c . Moreover, don't do tricky things like (adev.whoami & 1): just
> do a:
> switch (adev.whoami) {
> LIS302DL_ID:
>        printk(KERN_INFO DRIVER_NAME ": 1-byte sensor found.\n");
>        adev.lis3lv02d_read = lis3lv02d_read_8;
>        adev.mdps_max_val = 128;
>        break;
> LIS3LV02DL_ID:
>        printk(KERN_INFO DRIVER_NAME ": 2-byte sensor found.\n");
>        adev.lis3lv02d_read = lis3lv02d_read_16;
>        adev.mdps_max_val = 2048;
>        break;
> default:
>        printk(KERN_ERR DRIVER_NAME ": unknown sensor found of type %d.\n",
> adev.whoami);
>        return -EINVAL;
> }

I'm on it. There's still some unclear separation between the two
files, I must say.

-- 
Giuseppe "Oblomov" Bilotta