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

[Hans de Goede <hdegoede@redhat.com>]

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