Re: dell-laptop and separate AC timeouts on some Dell systems

["Pali Rohár" <pali.rohar@gmail.com>]

[-- Attachment #1: Type: Text/Plain, Size: 4448 bytes --]

On Saturday 08 April 2017 00:56:24 Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > Sent: Friday, April 7, 2017 4:55 PM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > Cc: platform-driver-x86@vger.kernel.org
> > Subject: Re: dell-laptop and separate AC timeouts on some Dell
> > systems
> > 
> > Hi!
> > 
> > On Friday 07 April 2017 21:33:53 Mario.Limonciello@dell.com wrote:
> > > Pali,
> > > 
> > > For some time there have been folks reporting some issues where
> > > keyboard backlight setting is failing on various Dell systems
> > > (https://github.com/dell/libsmbios/issues/3).  I've been circling
> > > around internally to find out what's going on. This affects a
> > > number of systems from the last year or so.
> > 
> > Great!
> > 
> > > What is happening is that some platforms support an alternate
> > > keyboard timeout while on AC.  The old "timeout" value is
> > > treated as just a battery timeout and the separate value is the
> > > AC timeout. Unfortunately if the AC timeout is /not/ set in the
> > > timeout setting/update request, this will fail.
> > 
> > So in case notebook is running on AC and we want to change timeout
> > for keyboard backlight, we need to tell smbios two timeouts, one
> > for battery and one for AC?
> 
> Yes, that's correct.  The reason for the failures so far has been
> that the field for AC has not been getting sent.  Since the buffer
> is zero'ed out before starting the undefined value "0" for AC
> timeout is sent to EC and rejected.
> 
> > > As a result I prototyped a few changes for this at the libsmbios
> > > branch here:
> > > https://github.com/dell/libsmbios/commits/fix-g8-keyboard-backlig
> > > ht
> > 
> > Can you update also documentation which is at the end of file? It
> > would help for kernel implementation.
> 
> OK, just added more on this.
> 
> > > It's not yet requested for merging because I still don't know why
> > > the request to supported features returns "Always On" still
> > > (that shouldn't be supported).
> > 
> > It looks like that "Always On" mode is reported by smbios by
> > supported, but setting it will cause failure. Any idea where is a
> > problem? Probably Dell firmware bug? Or needs to be that mode
> > handled specially?
> 
> It's a behavior outside of spec, but I'm still clarifying if it's
> intended and spec wasn't updated.
> 
> Continue to ignore this bit if it's set unless I hear otherwise.
> 
> > > As far as I can tell this doesn't really map well to how the
> > > keyboard backlight driver in the kernel works today, so I wanted
> > > to raise this to you to discuss what the best way to handle it
> > > is. One of these systems can be detected by the presence of the
> > > token 0x451. When that is found, the additional timeout unit and
> > > value should be sent with the requests.
> > > 
> > > Can you let me know your thoughts?
> > 
> > I think that extending struct kbd_state should work.
> > 
> > There are already functions dell_send_intensity() and
> > dell_get_intensity() which handle state based on battery/AC mode.
> 
> The difference is that intensity it's obvious that it changes from 50
> to 100 percent.  With different timeouts, shouldn't this be more
> complex?

Currently for keyboard backlight settings is used procedure:
get current status --> update --> set new status.

So we just need to have two timeouts in struct kbd_state and get/set 
functions needs to handle it. I do not thing this change would be 
complex, it should be straightforward.

> Would you just keep two different nodes as accessible to userspace?

And this is another question... For a first step we should just fix 
timeout sysfs node to work. And probably the best fix would be that this 
node will manage timeout which belongs to current AC or battery state. 
Like for dell_get_intensity(). When notebook is running on battery we 
will export timeout configured for battery and when on AC we will export 
timeout configured for AC. Changing timeout via sysfs just affect only 
one (current) configuration (ac or battery).

IIRC we do not provide a sysfs node for changing AC display panel 
brightness level when notebook is running on battery and vice versa. So 
do we need it for keyboard backlight timeout at all?

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]