Re: [PATCH 0/2] [RFC] platform/x86: Fixes for Toshiba Z830

[Arvid Norlander <lkml@vorpal.se>]

On 2022-08-26 14:07, Hans de Goede wrote:
> Hi,
> 
> On 8/24/22 14:31, Arvid Norlander wrote:
>> On 2022-08-22 13:39, Hans de Goede wrote:
> 
> <snip>
> 
>>> For 2. you can actually just copy and paste a lot of this email,
>>> I believe that having the info in this email in a
>>> Documentation/admin-guide/laptops/toshiba_acpi.rst file
>>> will make it a lot easier to find in the future then only having
>>> it in the mailinglist archives.
>>>
>>>> * For the hardware buttons I describe below, is a solution specific to
>>>>   toshiba_acpi preferred, or should additional effort be spent on
>>>>   investigating a generic solution?
>>>
>>> Ok, this is interesting there actually is a specification from
>>> Microsoft for this:
>>> http://download.microsoft.com/download/9/c/5/9c5b2167-8017-4bae-9fde-d599bac8184a/dirapplaunch.docx
>>>
>>> And there was a previous attempt to add support for the PNP0C32 devices:
>>> https://marc.info/?l=linux-acpi&m=120550727131007
>>> https://lkml.org/lkml/2010/5/28/327
>>>
>>> And this even made it into drivers/staging for a while, if you do:
>>> git revert 0be013e3dc2ee79ffab8a438bbb4e216837e3d52
>>> you will get a: drivers/staging/quickstart/quickstart.c file.
>>>
>>> Note this is not great code:
>>>
>>> 1. If you do:
>>>   ls /sys/bus/platform/devices
>>>   You should already see a couple of PNP0C32 platform devices there and the
>>>   driver should simply bind to those rather then creating its own platform device
>>> 2. As mentioned this really should use the standard /dev/input/event interface
>>>   for event reporting and allow userspace to change the scancode to EV_KEY*
>>>   mapping. You can do this e.g. by using a sparse_keymap for the scancode to
>>>   EV_KEY* mapping which will give you this for free.
>>
>> I have yet to have time to look at it. However this seems to suggest that
>> these buttons should work when the laptop is off. That is not the case on
>> the Z830. They only do anything when the computer is on and I can't find
>> any settings to change that.
> 
> Not necessarily fully off, but maybe when suspended ?

Tested it. Nope. In fact there is no code in the DSDT to handle the wakeup
case. Or rather, only some partial lines of code are left over from that.
That functionality is definitely non-functional on this laptop.

In addition the Microsoft specification lists _PRW as being a required
method. This is missing. Only _STA (which does something funky based on
variables set based on _OSI, haven't bothered figuring that out yet), _HID,
_UID and GHID exist as methods on the button objects.

> 
>> Looking at the specification it also mentions several different
>> notification codes for the button. The only one used on the Z830 is 0x80.
>> That is, as far as I can tell from the decompilation of the DSDT.
>>
>> Thus I worry I will not be able to test any sort of generic implementation
>> very well, if the Z830 only implements a small subset of the functionality.
> 
> Right I understand still I think there should be a separate
> 
> drivers/platform/x86/acpi_pnp0c32_buttons.c 
> 
> driver for this IMHO. If it is only tested on your one model that
> is fine (should be documented with a comment in the code though).
> 
> Then at least we have something to serve as a basis for if people
> want to add support for this on more laptop models.
> 
> Does that sound reasonable ?

Sure, we may have talked past each other, as this is what I also believe I
suggested. I just don't see how I can possibly implement the wakeup
handling part of this, as no laptop I own has that. Unless you know some
else who has a laptop that would allow testing that part.

Would a sensible option be to only implement support for key presses while
the laptop is awake? If someone comes along with a laptop that has the
support for these buttons waking from sleep they can add that missing
functionality at that point. (It seems rather unlikely that will happen
though: I get the feeling that this type of button never became a hit and
is mostly a forgotten relic of the past. And if no one came along and
bothered to add support in the decade+ since it was introduced, it was
probably quite rare back then as well.)

<snip>

>>>> 4. Battery charge mode [implemented in patch 2]
>>>> ======================
>>>>
>>>> This laptop supports not charging the battery fully in order to prolong
>>>> battery life. Unlike for example ThinkPads where this control is granular
>>>> here it is just off/on. When off it charges to 100%. When on it charges to
>>>> about 80%.
>>>>
>>>> According to the Windows program used to control the feature the setting
>>>> will not take effect until the battery has been discharged to around 50%.
>>>> However, in my testing it takes effect as soon as the charge drops below
>>>> 80%. On Windows Toshiba branded this feature as "Eco charging"
>>>>
>>>> In the following example ACPI calls I will use the following newly defined
>>>> constants:
>>>> #define HCI_BATTERY_CHARGE_MODE 0xba
>>>> #define BATTERY_CHARGE_FULL 0
>>>> #define BATTERY_CHARGE_80_PERCENT 1
>>>>
>>>> To set the feature:
>>>>   {HCI_SET, HCI_BATTERY_CHARGE_MODE, charge_mode, 0, 0, 0}
>>>> To query for the existence of the feature:
>>>>   {HCI_GET, HCI_BATTERY_CHARGE_MODE, 0, 0, 0, 0}
>>>> To read the feature:
>>>>   {HCI_GET, HCI_BATTERY_CHARGE_MODE, 0, 0, 0, 1}
>>>>
>>>> The read may need to be retried if TOS_DATA_NOT_AVAILABLE is returned as
>>>> the status code. This rarely happens (I have never observed it on Linux),
>>>> but I have seen it happen under Windows once, and the software did retry
>>>> it.
>>>
>>> Hmm, this is interesting if you look at:
>>>
>>> Documentation/ABI/testing/sysfs-class-power
>>>
>>> You will see there already is a standard API for this in the form of
>>> adding a "charge_control_end_threshold" attribute to the standard
>>> ACPI /sys/class/power_supply/BAT*/ sysfs interface. See e.g.
>>> drivers/platform/x86/thinkpad_acpi.c
>>>
>>> For an example of how to add sysfs attributes to a battery
>>> which is managed by the standard drivers/acpi/battery.c driver.
>>>
>>> I think you can use this standard attribute enabling eco charging
>>> for any writes with a value <= 90 and disabling it for values
>>>> 90 (90 being halfway between 80 and 100).
>>>
>>> While always showing 80 or 100 on read.
>>>
>>> You should then also write a patch for:
>>>
>>> Documentation/ABI/testing/sysfs-class-power
>>>
>>> Adding something like this to the "charge_control_end_threshold"
>>> section:
>>>
>>> "not all hardware is capable of setting this to an arbitrary
>>> percentage. Drivers will round written values to the nearest
>>> supported value. Reading back the value will show the actual
>>> threshold set by the driver."
>>>
>>> (feel free to copy verbatim, but maybe you can do better)
>>>
>>>
>>
>> This makes perfect sense, but I don't know if it is guaranteed to be 80%
>> on all Toshiba laptops. Do you know of any other Toshiba laptops that
>> have/had this feature, and if so, what the limits are? The Windows driver
>> for this laptop does not document exactly what the limit is. 80% is simply
>> what I have observed in practice.
> 
> Right, the idea is to document that the hw/fw/driver may only
> support some fixed values and that written values will be
> rounded to one of the supported fixed values. There is no need
> to document what those fixed values are.  The idea is that
> userspace consumers will read back the value to see what
> they actually got.

I think we might be slightly talking past each other here. I absolutely
agree with your idea. My only worry is that toshiba_acpi returning 80%
might not be the right choice. This could be model dependent.

Since I only have a sample size of one, it could even depend on the current
condition of the battery for all I know (though that is probably not
likely). The Windows software and the manual do not specify any sort of
percentage. It is just documented as a mode that prolongs battery life
while reducing full charge basically. Without putting any qualifiers on
"how much".

An option would be to return 80% for the Z830, and the string "unknown" for
other models. Though I guess there is a risk of breaking user space
software that only expects numeric values, so it may be a no-go.

I guess the user space software using this interface consists of *at least*
"tlp" and whatever KDE uses to handle it ("powerdevil" I belive?).

> 
> Regards,
> 
> Hans
> 

Best regards,
Arvid Norlander