Re: [PATCH v2 8/8] msi-wmi: Add MSI Wind support

[Anisse Astier <anisse@astier.eu>]

On Fri, 30 Nov 2012 20:36:48 +0200, Maxim Mikityanskiy <maxtram95@gmail.com> wrote :

> 2012/11/30 Anisse Astier <anisse@astier.eu>:

[snip]
> >> +
> >> +enum wind_scancodes {
> >> +     /* Fn+F3 touchpad toggle */
> >> +     WIND_KEY_TOUCHPAD       = 0x08,
> >> +     /* Fn+F11 Bluetooth toggle */
> >> +     WIND_KEY_BLUETOOTH      = 0x56,
> >> +     /* Fn+F6 webcam toggle */
> >> +     WIND_KEY_CAMERA         = 0x57,
> >> +     /* Fn+F11 Wi-Fi toggle */
> >> +     WIND_KEY_WLAN           = 0x5f,
> >> +     /* Fn+F10 turbo mode toggle */
> >> +     WIND_KEY_TURBO          = 0x60,
> >> +     /* Fn+F10 ECO mode toggle */
> >> +     WIND_KEY_ECO            = 0x69,
> >> +};
> >
> > Looking at the scancode numbers and the key function, I see no collision
> > that necessitate using a separate sparse keymap, or a separate enum.
> > Merging them would simplify a lot of your patch.
> 
> By merging keymaps we will lose information about what laptop
> corresponds the key to. Should I add this information in comments or
> is it better to use some other way?

I don't think this information would be useful to the driver. Just add it
in a comment.



[snip]

> >> +
> >> +static struct msi_wmi_table_entry msi_wmi_table[] = {
> >> +     {
> >> +             .guid                   = MSIWMI_MSI_EVENT_GUID,
> >> +             .keymap                 = msi_wmi_keymap,
> >> +             .quirk_last_pressed     = true,
> >> +             .scancode_base          = MSI_SCANCODE_BASE,
> >> +             .scancode_count         = ARRAY_SIZE(msi_wmi_keymap)-1,
> >> +     },
> >> +     {
> >> +             .guid                   = MSIWMI_WIND_EVENT_GUID,
> >> +             .keymap                 = wind_wmi_keymap,
> >> +     },
> >> +     {}
> >> +};
> > If you merge them you would only need the quirk_last_pressed variable,
> > and could do away with this struct.
> 
> Yes, you're right. But what if I would completely remove
> quirk_last_pressed and would use last_pressed if event 0xD0 - 0xD4
> happens, otherwise skip last_pressed check? Is it good way too?

Hum, I don't know, this doesn't seem like a good design, hardcoding
values this way (even if the net-effect would be the same).


> 
> >
> >> +
> >> +static struct msi_wmi_table_entry *wmi;
> >> +
> >>  static int msi_wmi_query_block(int instance, int *ret)
> >>  {
> >>       acpi_status status;
> >> @@ -165,11 +231,15 @@ static void msi_wmi_notify(u32 value, void *context)
> >>               pr_debug("Eventcode: 0x%x\n", eventcode);
> >>               key = sparse_keymap_entry_from_scancode(msi_wmi_input_dev,
> >>                               eventcode);
> >> -             if (key) {
> >> +             if (!key) {
> >> +                     pr_info("Unknown key pressed - %x\n", eventcode);
> >> +                     goto msi_wmi_notify_exit;
> >> +             }
> >> +             if (wmi->quirk_last_pressed) {
> >>                       ktime_t diff;
> >>                       cur = ktime_get_real();
> >>                       diff = ktime_sub(cur, last_pressed[key->code -
> >> -                                     SCANCODE_BASE]);
> >> +                                     wmi->scancode_base]);
> >>                       /* Ignore event if the same event happened in a 50 ms
> >>                          timeframe -> Key press may result in 10-20 GPEs */
> >>                       if (ktime_to_us(diff) < 1000 * 50) {
> >> @@ -178,21 +248,19 @@ static void msi_wmi_notify(u32 value, void *context)
> >>                                        key->code, ktime_to_us(diff));
> >>                               goto msi_wmi_notify_exit;
> >>                       }
> >> -                     last_pressed[key->code - SCANCODE_BASE] = cur;
> >> -
> >> -                     if (key->type == KE_KEY &&
> >> -                     /* Brightness is served via acpi video driver */
> >> -                     (!acpi_video_backlight_support() ||
> >
> > You modify behaviour by removing this test. Backlight might be
> > registered, but we may not want to modify it directly from here because
> > the acpi video driver does that for us.
> 
> I don't remove the test. I only change !acpi_video_backlight_support()
> condition by !backlight condition (actually, it's a typo, condition
> have to look like 'backlight', not '!backlight', I'm sorry for this).
> There is a check in msi_wmi_backlight_setup(). I call
> acpi_video_backlight_support() from there and backlight will be NULL
> if acpi video driver supports backlight. So checking backlight != NULL
> is the same as checking acpi_video_backlight_support() plus fallback
> if acpi video driver doesn't support backlight and WMI driver failed
> to initialize backlight.

You're right. This is true because acpi_video_backlight_support's result
doesn't change at runtime.

[snip]

> >>  static int __init msi_wmi_input_setup(void)
> >>  {
> >>       int err;
> >>
> >> +     if (wmi->quirk_last_pressed) {
> >> +             last_pressed = kcalloc(wmi->scancode_count,
> >> +                     sizeof(last_pressed[0]), GFP_KERNEL);
> >
> > I see why you separated the keymaps. I'm pretty sure we can have an
> > unified keymap while keeping the last_pressed array working.
> 
> I think I could check in msi_wmi_notify if event is between 0xD0 and
> 0xD4 and if so, use last_pressed.

As I said, not a good idea to hardcode the range.

[snip]

> >
> > There's a lot of restructuring in here that aren't directly related to
> > adding support for new hardware (enums, init path for backlight, quirk,
> > etc.), which make this patch hard to review. You should try to put these
> > in separate patches.
> 
> OK, I will do that and resend later, when I'll receive comments on
> other patches and fix all needed things.
> 

Thanks,

Anisse