From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1422722AbcFGRCV (ORCPT ); Tue, 7 Jun 2016 13:02:21 -0400 Received: from mx1.redhat.com ([209.132.183.28]:10325 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1161308AbcFGRCT (ORCPT ); Tue, 7 Jun 2016 13:02:19 -0400 Message-ID: <1465318938.7166.23.camel@redhat.com> Subject: Re: [PATCH] thinkpad_acpi: Add support for HKEY version 0x200 From: Lyude Paul To: "ibm-acpi-devel@lists.sourceforge.net" , Dennis Wassenberg Cc: Henrique de Moraes Holschuh , Darren Hart , platform-driver-x86@vger.kernel.org, "linux-kernel@vger.kernel.org" Date: Tue, 07 Jun 2016 13:02:18 -0400 Organization: Red Hat Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Tue, 07 Jun 2016 17:02:18 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Since nothing's really happened with this patch for a while I figured I'd take over trying to get this upstream. Regarding testing: This seems to work fine on the 60 series laptops, and works fine on previous generations. The one thing I haven't been able to test is an X1 carbon with an adaptive keyboard since I don't seem to have one readily available here. I'm doing a search around the office to try to find someone who didn't throw theirs away yet so hopefully I should be able to get back to you on that soon. To Dennis: I took the liberty of doing a review of your patch and some testing. There's a few things that need changing, I've outlined them below: (for the future, it's recommended to send patches for the kernel inline in emails to make them easier to review). > From 8a67f5db1d2918c46b7fa2168e3d0aab2ba92731 Mon Sep 17 00:00:00 2001 > From: Dennis Wassenberg > Date: Wed, 13 Apr 2016 13:46:55 +0200 > Subject: [PATCH] thinkpad_acpi: Add support for HKEY version 0x200 > > Lenovo Thinkpad devices T460, T460s, T460p, T560, X260 use > HKEY version 0x200 without adaptive keyboard. > > HKEY version 0x200 has method MHKA with one parameter value. > Passing parameter value 1 will get hotkey_all_mask (the same like > HKEY version 0x100 without parameter). Passing parameter value 2 to > MHKA method will retrieve hotkey_all_adaptive_mask. If 0 is returned in > that case there is no adaptive keyboard available. > > Signed-off-by: Dennis Wassenberg > --- >  drivers/platform/x86/thinkpad_acpi.c | 88 ++++++++++++++++++++++++++--------- > - >  1 file changed, 64 insertions(+), 24 deletions(-) > > diff --git a/drivers/platform/x86/thinkpad_acpi.c > b/drivers/platform/x86/thinkpad_acpi.c > index a268a7a..0e72857 100644 > --- a/drivers/platform/x86/thinkpad_acpi.c > +++ b/drivers/platform/x86/thinkpad_acpi.c > @@ -2043,6 +2043,7 @@ static int hotkey_autosleep_ack; >   >  static u32 hotkey_orig_mask; /* events the BIOS had enabled */ >  static u32 hotkey_all_mask; /* all events supported in fw */ > +static u32 hotkey_adaptive_all_mask; /* all adaptive events supported > in fw */ >  static u32 hotkey_reserved_mask; /* events better left disabled */ >  static u32 hotkey_driver_mask; /* events needed by the driver > */ >  static u32 hotkey_user_mask; /* events visible to userspace */ > @@ -2742,6 +2743,17 @@ static ssize_t hotkey_all_mask_show(struct device *dev, >   >  static DEVICE_ATTR_RO(hotkey_all_mask); >   > +/* sysfs hotkey all_mask ----------------------------------------------- */ > +static ssize_t hotkey_adaptive_all_mask_show(struct device *dev, > +    struct device_attribute *attr, > +    char *buf) > +{ > + return snprintf(buf, PAGE_SIZE, "0x%08x\n", > + hotkey_adaptive_all_mask | hotkey_source_mask); Make sure when you're indent function calls that split like this onto another line you indent it like this:         return snprintf(buf, PAGE_SIZE, "0x%08x\n",                         hotkey_adaptive_all_mask | hotkey_source_mask); So that "hotkey_adaptive_all_mask" starts on the column after the starting paranthesis. > +} > + > +static DEVICE_ATTR_RO(hotkey_adaptive_all_mask); > + >  /* sysfs hotkey recommended_mask --------------------------------------- */ >  static ssize_t hotkey_recommended_mask_show(struct device *dev, >       struct device_attribute *attr, > @@ -2985,6 +2997,7 @@ static struct attribute *hotkey_attributes[] __initdata > = { >   &dev_attr_wakeup_hotunplug_complete.attr, >   &dev_attr_hotkey_mask.attr, >   &dev_attr_hotkey_all_mask.attr, > + &dev_attr_hotkey_adaptive_all_mask.attr, >   &dev_attr_hotkey_recommended_mask.attr, >  #ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL >   &dev_attr_hotkey_source_mask.attr, > @@ -3321,20 +3334,6 @@ static int __init hotkey_init(struct ibm_init_struct > *iibm) >   if (!tp_features.hotkey) >   return 1; >   > - /* > -  * Check if we have an adaptive keyboard, like on the > -  * Lenovo Carbon X1 2014 (2nd Gen). > -  */ > - if (acpi_evalf(hkey_handle, &hkeyv, "MHKV", "qd")) { > - if ((hkeyv >> 8) == 2) { > - tp_features.has_adaptive_kbd = true; > - res = sysfs_create_group(&tpacpi_pdev->dev.kobj, > - &adaptive_kbd_attr_group); > - if (res) > - goto err_exit; > - } > - } > - >   quirks = tpacpi_check_quirks(tpacpi_hotkey_qtable, >        ARRAY_SIZE(tpacpi_hotkey_qtable)); >   > @@ -3357,30 +3356,71 @@ static int __init hotkey_init(struct ibm_init_struct > *iibm) >      A30, R30, R31, T20-22, X20-21, X22-24.  Detected by checking >      for HKEY interface version 0x100 */ >   if (acpi_evalf(hkey_handle, &hkeyv, "MHKV", "qd")) { > - if ((hkeyv >> 8) != 1) { > - pr_err("unknown version of the HKEY interface: > 0x%x\n", > -        hkeyv); > - pr_err("please report this to %s\n", TPACPI_MAIL); > - } else { > + vdbg_printk(TPACPI_DBG_INIT | TPACPI_DBG_HKEY, > + "firmware HKEY interface version: 0x%x\n", > + hkeyv); The indenting here wasn't correct to begin with, but since we're changing it we might as well fix it. > + switch (hkeyv >> 8) { > + case 1: >   /* >    * MHKV 0x100 in A31, R40, R40e, >    * T4x, X31, and later >    */ > - vdbg_printk(TPACPI_DBG_INIT | TPACPI_DBG_HKEY, > - "firmware HKEY interface version: 0x%x\n", > - hkeyv); >   >   /* Paranoia check AND init hotkey_all_mask */ >   if (!acpi_evalf(hkey_handle, &hotkey_all_mask, >   "MHKA", "qd")) { >   pr_err("missing MHKA handler, " > -        "please report this to %s\n", > -        TPACPI_MAIL); > +         "please report this to %s\n", > +         TPACPI_MAIL); The indenting here doesn't need to be changed > + /* Fallback: pre-init for FN+F3,F4,F12 */ > + hotkey_all_mask = 0x080cU; > + } else { > + tp_features.hotkey_mask = 1; > + } > + break; > + > + case 2: > + /* > +  * MHKV 0x200 in X1, T460s, X260, T560, X1 Tablet (2016) > +  */ > + > + /* Paranoia check AND init hotkey_all_mask */ > + if (!acpi_evalf(hkey_handle, &hotkey_all_mask, > + "MHKA", "dd", 1)) { > + pr_err("missing MHKA handler, " > + "please report this to %s\n", > + TPACPI_MAIL); This indenting needs to be fixed as well Once all those fixes are made and I get that extra testing done we should be ablew to send it upstream, assuming it doesn't break the X1… Cheers, Lyude >   /* Fallback: pre-init for FN+F3,F4,F12 */ >   hotkey_all_mask = 0x080cU; >   } else { >   tp_features.hotkey_mask = 1; >   } > + > + /* > +  * Check if we have an adaptive keyboard, like on the > +  * Lenovo Carbon X1 2014 (2nd Gen). > +  */ > + if (acpi_evalf(hkey_handle,&hotkey_adaptive_all_mask, > + "MHKA", "dd", 2)) { Indentation needs to be fixed here as well > + if (hotkey_adaptive_all_mask != 0) { > + tp_features.has_adaptive_kbd = true; > + res = sysfs_create_group( > + &tpacpi_pdev->dev.kobj, > + &adaptive_kbd_attr_group); > + if (res) > + goto err_exit; > + } > + } else { > + tp_features.has_adaptive_kbd = false; > + hotkey_adaptive_all_mask = 0x0U; > + } > + break; > + > + default: > + pr_err("unknown version of the HKEY interface: > 0x%x\n", > +        hkeyv); > + pr_err("please report this to %s\n", TPACPI_MAIL); > + break; >   } >   } >   > --  > 2.1.4