Re: [PATCH 1/1] platform/x86: ideapad: Expose charge_types

[Jelle van der Waa <jvanderw@redhat.com>]

On 5/12/25 00:54, Armin Wolf wrote:
> Am 11.05.25 um 13:30 schrieb Jelle van der Waa:
> 
>> From: Jelle van der Waa <jvanderwaa@redhat.com>
>>
>> Some Ideapad models support a battery conservation mode which limits the
>> battery charge threshold for longer battery longevity. This is currently
>> exposed via a custom conservation_mode attribute in sysfs.
>>
>> The newly introduced charge_types sysfs attribute is a standardized
>> replacement for laptops with a fixed end charge threshold. Setting it to
>> `Long Life` would enable battery conservation mode. The standardized
>> user space API would allow applications such as UPower to detect laptops
>> which support this battery longevity mode and set it.
>>
>> Tested on an Lenovo ideapad U330p.
> 
> Hi,
> 
> i like the idea behind this patch series, the charge_types attribute is 
> indeed
> exactly what we need in this case.

Thanks! Credit where credit is due, this idea came from Hans de Goede 
(who also added charge_types). I only wrote the code.

>> ---
>>   .../ABI/testing/sysfs-platform-ideapad-laptop |   2 +
>>   drivers/platform/x86/ideapad-laptop.c         | 126 +++++++++++++++++-
>>   2 files changed, 125 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-platform-ideapad-laptop 
>> b/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
>> index 4989ab266682..83eca4c14503 100644
>> --- a/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
>> +++ b/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
>> @@ -32,6 +32,8 @@ Date:        Aug 2017
>>   KernelVersion:    4.14
>>   Contact:    platform-driver-x86@vger.kernel.org
>>   Description:
>> +        This interface is deprecated; please use /sys/class/ 
>> power_supply/*/charge_types.
>> +
> 
> Maybe it would make sense to move this attribute to Documentation/ABI/ 
> obsolete?

I am not sure what the normal workflow is so I've applied this 
suggestion in v2.

>> +                       void *ext_data,
>> +                       enum power_supply_property psp,
>> +                       const union power_supply_propval *val)
>> +{
>> +    struct ideapad_private *priv = ext_data;
>> +    int err;
>> +
>> +    if (psp != POWER_SUPPLY_PROP_CHARGE_TYPES)
>> +        return -EINVAL;
>> +
>> +    err = exec_sbmc(priv->adev->handle,
>> +            (val->intval == POWER_SUPPLY_CHARGE_TYPE_LONGLIFE ?
>> +             SBMC_CONSERVATION_ON : SBMC_CONSERVATION_OFF));
> 
> AFAIK the power supply core does not check if val->intval holds a 
> supported charge type value.
> Please use a switch case to return -EINVAL in such cases.

 From my testing checking val->intval wasn't needed, you'll get an 
"write error: invalid argument" when trying to set "Long lifeee".

I believe that is handled in power_supply_store_property, but reading 
the code I don't really believe that's true? So I've simply switched it 
over to a switch/case.

Thanks,

Jelle