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

[Andres Salomon <dilinger@queued.net>]

On Fri, 26 Jul 2024 02:04:09 +0200
Pali Rohár <pali@kernel.org> wrote:

> On Friday 26 July 2024 01:48:50 Armin Wolf wrote:
> > Am 26.07.24 um 00:15 schrieb Pali Rohár:
> >   
> > > On Thursday 25 July 2024 16:24:57 Andres Salomon wrote:  
> > > > On Thu, 25 Jul 2024 01:01:58 +0200
> > > > Pali Rohár <pali@kernel.org> wrote:
> > > >   
> > > > > On Wednesday 24 July 2024 18:23:18 Andres Salomon wrote:  
[...]
> > > > > 
> > > > > The issue here is: how to tell kernel that the particular
> > > > > dell_battery_hook has to be bound with the primary battery?
> > > > >   
> > > > So from userspace, we've got the expectation that multiple batteries
> > > > would show up as /sys/class/power_supply/BAT0, /sys/class/power_supply/BAT1,
> > > > and so on.  
> > > Yes, I hope so.
> > >   
> > > > The current BAT0 entry shows things like 'capacity' even without this
> > > > patch, and we're just piggybacking off of that to add charge_type and
> > > > other entries. So there shouldn't be any confusion there, agreed?  
> > > I have not looked at the battery_hook_register() code yet (seems that I
> > > would have to properly read it and understand it). But does it mean that
> > > battery_hook_register() is adding hook just for "BAT0"?
> > > 
> > > What I mean: cannot that hook be registered to "BAT1" too? Because if
> > > yes then we should prevent it. Otherwise this hook which is for "Dell
> > > Primary Battery" could be registered also for secondary battery "BAT1".
> > > (I hope that now it is more clear what I mean).  
> > 
> > Hi,
> > 
> > the battery hook is being registered to all ACPI batteries present on a given system,
> > so you need to do some manual filtering when .add_battery() is called.  
> 
> Ok. So it means that the filtering based on the primary battery in
> add_battery callback is needed.
> 

Thanks for the explanations. Seems simple enough to fix that, as some of
the other drivers are checking battery->desc->name for "BAT0".


One thing that I keep coming back to, and was reinforced as I looked at
include/linux/power_supply.h; the generic power supply charge_type has
values that are very close to Dells, but with different names. I could
shoehorn them in, though, with the following mappings:

POWER_SUPPLY_CHARGE_TYPE_FAST,         => "express" (aka ExpressCharge)
POWER_SUPPLY_CHARGE_TYPE_STANDARD,     => "standard"
POWER_SUPPLY_CHARGE_TYPE_ADAPTIVE,     => "adaptive"
POWER_SUPPLY_CHARGE_TYPE_CUSTOM,       => "custom"
POWER_SUPPLY_CHARGE_TYPE_LONGLIFE,     => "primarily_ac"

The main difference is that Primarily AC is described and documented as
slightly different than Long Life, but I suspect the result is roughly
the same thing. And the naming "Fast" and "Long Life" wouldn't match the
BIOS naming of "ExpressCharge" and "Primarily AC".

Until now I've opted to match the BIOS naming, but I'm curious what others
think before I send V3 of the patches.


-- 
I'm available for contract & employment work, see:
https://spindle.queued.net/~dilinger/resume-tech.pdf