2020. július 26., vasárnap 14:40 keltezéssel, Kristian Klausen írta: > On 26.07.2020 13.38, Barnabás Pőcze wrote: > > > Hello all, > > The asus-wmi, huawei-wmi, and thinkpad_acpi drivers all utilize the battery hooking system provided by the acpi/battery driver to provide functionality to specify the battery charge limits. > > The first two drivers create the following attributes for the battery to achieve the functionality: > > > > - charge_control_start_threshold (only huawei-wmi) > > - charge_control_end_threshold, > > > > while thinkpad_acpi uses the following names: > > > > - charge_start_threshold > > - charge_stop_threshold. > > > > The userspace utility TLP looks for the attributes defined by the thinkpad_acpi driver. In my opinion this inconsistency should be promptly resolved and the preferred names documented. > > The preferred names is already documented: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=813cab8f3994250e136819ae48fbd1c95d980466 > Thank you for bringing attention to this, I forgot to actually look for it in the documentation. Thanks to this most of my initial email can be disregarded, and the only significant part is that thinkpad_acpi does not use these documented attribute names. I guess it is because the attributes were added back in 2018 to the driver, more than a year before that commit. So that should be fixed, no? > > A further detail that's worth documenting is the accepted range of values. thinkpad_acpi accepts [0, 99] for the start value, and [1, 100] for the stop value. On the other hand asus-wmi, and huawei-wmi accept [0, 100] for the stop value, and huawei-wmi accepts [0, 100] for the start value. (asus-wmi does not support specifying a start value). > > I am in the process of writing a platform driver which provides this exact functionality, that is why I would like both of the aforementioned details to be set down. I'd prefer the names used by TLP and thinkpad_acpi, but it's really not important, I just want them to be the same for all drivers, and that these names be documented. > > Secondly, one thing I can imagine is having two another attributes: charge_start_threshold_range and charge_stop_threshold_range, that contain two integers "a b", which means the valid range of values is [a, b]. Then I guess there needs to be another value that is handled, something like "off", which restores the platform defaults. This is, of course, requires more complex implementation, but is quite flexible. Although I am not sure this flexibility is worth the complexity. > > I'm not aware of any platforms with defaults implemented in > firmware/hardware, besides start at 0% and stop at ~100%? > Me neither, but who knows what is/will be out there? > > Or a simpler option is accepting [0, 100] for both attributes and returning -EINVAL if the provided value is out of range or the driver cannot set it on that particular machine due to hardware limitations or something. Alternatively thinkpad_acpi behavior could be adopted as well ([0, 99] for start, and [1, 100] for stop). > > I don't think charge_{start,stop}_threshold_range is worth the effort, > lets KISS. > I tend to agree, the complexity is probably not worth it. > > In both of the previous cases the behavior of the edge cases should be agreed upon: for example, what does 0 for charge_stop_threshold mean? Does it restore the default, or does it really stop charging at 0%, keeping the battery empty? And so on. > > +1 for keeping the battery empty. > > > What do you think? > > Thank you for your attention > > Barnabás Pőcze
