Re: [ibm-acpi-devel] [PATCH] thinkpad_acpi: Add support for HKEY version 0x200

Lyude Paul
2016-06-07T21:10:56Z

Managed to find someone in the office with one of these laptops and it looks
like the adaptive keyboard works perfectly with this patch, so I can give my t-
b:

Tested-by: Lyude Paul <cpaul@redhat.com>

Cheers,
Lyude

On Tue, 2016-06-07 at 13:02 -0400, Lyude Paul wrote:
> 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 <dennis.wassenberg@secunet.com>
> > 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 <dennis.wassenberg@secunet.com>
> > ---
> >  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
> ------------------------------------------------------------------------------
> What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
> patterns at an interface-level. Reveals which users, apps, and protocols are
> consuming the most bandwidth. Provides multi-vendor support for NetFlow,
> J-Flow, sFlow and other flows. Make informed decisions using capacity
> planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e
> _______________________________________________
> ibm-acpi-devel mailing list
> ibm-acpi-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel