Hi, On 8/27/22 13:42, Arvid Norlander wrote: > 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. Ah ok, good :) > I just don't see how I can possibly implement the wakeup > handling part of this, as no laptop I own has that. Right, I fully agree. > 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? Yes that is fine. > 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. Ack. > (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.) Also ack. >>>>> 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. Ah I see. 80% seems to be pretty common as max-charge value for reducing the wear on the battery so I believe that just harcoding 80% in toshiba_acpi is fine. If people com[plain that it actually is say 85% on some models then we can see from there. > 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). Right that seems unlikely. > 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. Right, reporting non-numeric values here is not allowed, so lets not do that. > I guess the user space software using this interface consists of *at least* > "tlp" and whatever KDE uses to handle it ("powerdevil" I belive?). Right and upower / GNOME is also working towards using these. Regards, Hans
