Re: [PATCH v3 1/2] platform/x86:dell-laptop: Add knobs to change battery charge settings

Hans de Goede
2024-08-24T12:35:59Z

Hi,

On 8/24/24 4:39 AM, Andres Salomon wrote:
> On Mon, 19 Aug 2024 15:59:45 +0200
> Hans de Goede <hdegoede@redhat.com> wrote:
>
>> Hi,
>>
>> On 8/16/24 1:28 AM, Andres Salomon wrote:
> [...]
>>
>>> ---
>>> Changes in v3:
>>> - switch tokenid and class types
>>> - be stricter with results from both userspace and BIOS
>>> - no longer allow failed BIOS reads
>>> - rename/move dell_send_request_by_token_loc, and add helper function
>>> - only allow registration for BAT0
>>> - rename charge_type modes to match power_supply names
>>> Changes in v2, based on extensive feedback from Pali Rohár <pali@kernel.org>:
>>> - code style changes
>>> - change battery write API to use token->value instead of passed value
>>> - stop caching current mode, instead querying SMBIOS as needed
>>> - drop the separate list of charging modes enum
>>> - rework the list of charging mode strings
>>> - query SMBIOS for supported charging modes
>>> - split dell_battery_custom_set() up
>>> ---
>>> .../ABI/testing/sysfs-class-power-dell | 33 ++
>>> drivers/platform/x86/dell/Kconfig | 1 +
>>> drivers/platform/x86/dell/dell-laptop.c | 316 ++++++++++++++++++
>>> drivers/platform/x86/dell/dell-smbios.h | 7 +
>>> 4 files changed, 357 insertions(+)
>>> create mode 100644 Documentation/ABI/testing/sysfs-class-power-dell
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-class-power-dell b/Documentation/ABI/testing/sysfs-class-power-dell
>>> new file mode 100644
>>> index 000000000000..d8c542177558
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-class-power-dell
>>> @@ -0,0 +1,33 @@
>>> +What: /sys/class/power_supply/<supply_name>/charge_type
>>> +Date: August 2024
>>> +KernelVersion: 6.12
>>> +Contact: linux-pm@vger.kernel.org
>>> +Description:
>>> + Select the charging algorithm to use for the (primary)
>>> + battery.
>>> +
>>> + Standard:
>>> + Fully charge the battery at a moderate rate.
>>> + Fast:
>>> + Quickly charge the battery using fast-charge
>>> + technology. This is harder on the battery than
>>> + standard charging and may lower its lifespan.
>>> + The Dell BIOS calls this ExpressCharge™.
>>> + Trickle:
>>> + Users who primarily operate the system while
>>> + plugged into an external power source can extend
>>> + battery life with this mode. The Dell BIOS calls
>>> + this "Primarily AC Use".
>>> + Adaptive:
>>> + Automatically optimize battery charge rate based
>>> + on typical usage pattern.
>>> + Custom:
>>> + Use the charge_control_* properties to determine
>>> + when to start and stop charging. Advanced users
>>> + can use this to drastically extend battery life.
>>> +
>>> + Access: Read, Write
>>> + Valid values:
>>> + "Standard", "Fast", "Trickle",
>>> + "Adaptive", "Custom"
>>> +
>>
>> As the kernel test robot reports:
>>
>> Warning: /sys/class/power_supply/<supply_name>/charge_type is defined 2 times: ./Documentation/ABI/testing/sysfs-class-power-dell:0 ./Documentation/ABI/testing/sysfs-class-power:375
>>
>> So please drop this. What you could do is instead submit (as a separate) patch
>> an update to Documentation/ABI/testing/sysfs-class-power:375 to use your IMHO
>> more readable version.
>>
>> And when doing so I think it would best to replace "The Dell BIOS calls this ..."
>> with "In vendor tooling this may also be called ...".
>>
>>
>
> Is this what you had in mind? I don't see many users of charge_type, and
> I'm not sure whether the assumptions I'm making there are correct.
>
> https://lore.kernel.org/lkml/20240820041942.30ed42f3@5400/

Yes that is what I head in mind, thank you for doing this.

I'll try to review that patch soon-ish.

I'll review and likely merge your new v4 "Add knobs" patch on Monday.

Regards,

Hans