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

[Armin Wolf]

Am 26.07.24 um 06:25 schrieb Andres Salomon:

> On Fri, 26 Jul 2024 02:04:09 +0200
> Pali Rohár <pali@xxxxxxxxxx> 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@xxxxxxxxxx> 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 agree that POWER_SUPPLY_CHARGE_TYPE_FAST should be mapped the ExpressCharge,
but i think that "primarily_ac" should become a official power supply charging mode.

The reason is that for example the wilco-charger driver also supports such a charging mode
(currently reported as POWER_SUPPLY_CHARGE_TYPE_TRICKLE) and the charging mode seems to be
both sufficiently different from POWER_SUPPLY_CHARGE_TYPE_LONGLIFE/POWER_SUPPLY_CHARGE_TYPE_TRICKLE
and sufficiently generic to be supported by a wide array of devices.

Thanks,
Armin Wolf