Re: [PATCH AUTOSEL 4.20 008/117] platform/x86: asus-wmi: Tell the EC the OS will handle the display off hotkey

[Sasha Levin]

On Tue, Jan 08, 2019 at 04:49:18PM -0800, João Paulo Rechi Vita wrote:
> Hello Sasha,
>
> On Tue, Jan 8, 2019 at 11:26 AM Sasha Levin <sashal@xxxxxxxxxx> wrote:
> > From: João Paulo Rechi Vita <jprvita@xxxxxxxxx>
> >
> > [ Upstream commit 78f3ac76d9e5219589718b9e4733bee21627b3f5 ]
> >
> > In the past, Asus firmwares would change the panel backlight directly
> > through the EC when the display off hotkey (Fn+F7) was pressed, and
> > only notify the OS of such change, with 0x33 when the LCD was ON and
> > 0x34 when the LCD was OFF. These are currently mapped to
> > KEY_DISPLAYTOGGLE and KEY_DISPLAY_OFF, respectively.
> >
> > Most recently the EC on Asus most machines lost ability to toggle the
> > LCD backlight directly, but unless the OS informs the firmware it is
> > going to handle the display toggle hotkey events, the firmware still
> > tries change the brightness through the EC, to no effect. The end result
> > is a long list (at Endless we counted 11) of Asus laptop models where
> > the display toggle hotkey does not perform any action. Our firmware
> > engineers contacts at Asus were surprised that there were still machines
> > out there with the old behavior.
> >
> > Calling WMNB(ASUS_WMI_DEVID_BACKLIGHT==0x00050011, 2) on the _WDG device
> > tells the firmware that it should let the OS handle the display toggle
> > event, in which case it will simply notify the OS of a key press with
> > 0x35, as shown by the DSDT excerpts bellow.
> >
> >  Scope (_SB)
> >  {
> >      (...)
> >
> >      Device (ATKD)
> >      {
> >          (...)
> >
> >          Name (_WDG, Buffer (0x28)
> >          {
> >              /* 0000 */  0xD0, 0x5E, 0x84, 0x97, 0x6D, 0x4E, 0xDE, 0x11,
> >              /* 0008 */  0x8A, 0x39, 0x08, 0x00, 0x20, 0x0C, 0x9A, 0x66,
> >              /* 0010 */  0x4E, 0x42, 0x01, 0x02, 0x35, 0xBB, 0x3C, 0x0B,
> >              /* 0018 */  0xC2, 0xE3, 0xED, 0x45, 0x91, 0xC2, 0x4C, 0x5A,
> >              /* 0020 */  0x6D, 0x19, 0x5D, 0x1C, 0xFF, 0x00, 0x01, 0x08
> >          })
> >          Method (WMNB, 3, Serialized)
> >          {
> >              CreateDWordField (Arg2, Zero, IIA0)
> >              CreateDWordField (Arg2, 0x04, IIA1)
> >              Local0 = (Arg1 & 0xFFFFFFFF)
> >
> >              (...)
> >
> >              If ((Local0 == 0x53564544))
> >              {
> >                  (...)
> >
> >                  If ((IIA0 == 0x00050011))
> >                  {
> >                      If ((IIA1 == 0x02))
> >                      {
> >                          ^^PCI0.SBRG.EC0.SPIN (0x72, One)
> >                          ^^PCI0.SBRG.EC0.BLCT = One
> >                      }
> >
> >                      Return (One)
> >                  }
> >              }
> >              (...)
> >          }
> >          (...)
> >      }
> >      (...)
> >  }
> >  (...)
> >
> >  Scope (_SB.PCI0.SBRG.EC0)
> >  {
> >      (...)
> >
> >      Name (BLCT, Zero)
> >
> >      (...)
> >
> >      Method (_Q10, 0, NotSerialized)  // _Qxx: EC Query
> >      {
> >          If ((BLCT == Zero))
> >          {
> >              Local0 = One
> >              Local0 = RPIN (0x72)
> >              Local0 ^= One
> >              SPIN (0x72, Local0)
> >              If (ATKP)
> >              {
> >                  Local0 = (0x34 - Local0)
> >                  ^^^^ATKD.IANE (Local0)
> >              }
> >          }
> >          ElseIf ((BLCT == One))
> >          {
> >              If (ATKP)
> >              {
> >                  ^^^^ATKD.IANE (0x35)
> >              }
> >          }
> >      }
> >      (...)
> >  }
> >
> > Signed-off-by: João Paulo Rechi Vita <jprvita@xxxxxxxxxxxx>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
>
> I am not entirely sure this is linux-stable material. This patch makes
> the "turn off the display backlight" hotkey work on some Asus machines
> where, without this patch, the key would simply do nothing. It seems
> to me this is more of a new feature support than a bug fix. That said,
> if you or Andy think this should go to stable after this short
> explanation, I'll not object it.

I think I'll keep this in. It looks a lot like a quirk to make this
device work.

--
Thanks,
Sasha