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 20:46:22 +0200
Armin Wolf <W_Armin@gmx.de> wrote:

> Am 26.07.24 um 20:42 schrieb 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@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 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
> >  
> I just read the documentation regarding the charge_type sysfs attribute and it states that:
> 
> Trickle:
> 	Extends battery lifespan, intended for users who
> 	primarily use their Chromebook while connected to AC.
> 
> So i think that "primarily_ac" should be mapped to POWER_SUPPLY_CHARGE_TYPE_TRICKLE.

Do you think it's worth keeping around the sysfs-class-power-dell
file? At this point it's basically just documenting the slight naming
differences:


                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"





-- 
I'm available for contract & employment work, please contact me if
interested.