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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3503 bytes --]

On Tue, 13 May 2025, Jelle van der Waa wrote:

> 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.

Add a Suggested-by tag then so the relevant people are easier to find, 
lets say, after 5 years from now.

> > >   .../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
> 

-- 
 i.