Message-ID: <7la3x6f733ju4szqlzmtr277ah7c7lb4d4gcmjgu2rjj5uzpyd@43dmenbpczjs>
Date: Fri, 21 Feb 2025 02:33:16 +0100
From: Sebastian Reichel <sebastian.reichel@...labora.com>
To: Pengyu Luo <mitltlatltl@...il.com>
Cc: linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH v3 5/6] power: supply: add Huawei Matebook E Go psy driver

Hi,

On Thu, Feb 20, 2025 at 02:43:20PM +0800, Pengyu Luo wrote:
> On Thu, Feb 20, 2025 at 8:24 AM Sebastian Reichel <sebastian.reichel@...labora.com> wrote: 
> > On Tue, Jan 14, 2025 at 01:51:27AM +0800, Pengyu Luo wrote:
> > > On the Huawei Matebook E Go tablet the EC provides access to the adapter
> > > and battery status. Add the driver to read power supply status on the
> > > tablet.
> > >
> > > Signed-off-by: Pengyu Luo <mitltlatltl@...il.com>
> > > ---
> > >  .../ABI/testing/sysfs-class-power-gaokun      |  47 ++
> > >  drivers/power/supply/Kconfig                  |  10 +
> > >  drivers/power/supply/Makefile                 |   1 +
> > >  drivers/power/supply/huawei-gaokun-battery.c  | 548 ++++++++++++++++++
> > >  4 files changed, 606 insertions(+)
> > >  create mode 100644 Documentation/ABI/testing/sysfs-class-power-gaokun
> > >  create mode 100644 drivers/power/supply/huawei-gaokun-battery.c
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-class-power-gaokun b/Documentation/ABI/testing/sysfs-class-power-gaokun
> > > new file mode 100644
> > > index 000000000..b1eb9e8d7
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-class-power-gaokun
> > > @@ -0,0 +1,47 @@
> > > +What:                /sys/class/power_supply/gaokun-ec-battery/smart_charge
> > > +Date:                January 2025
> > > +KernelVersion:       6.12
> > > +Contact:     Pengyu Luo <mitltlatltl@...il.com>
> > > +Description:
> > > +             This entry allows configuration of smart charging behavior with
> > > +             four parameters. The format is: <mode> <delay> <start> <stop>.
> > > +
> > > +             - mode: Defines the charging mode (1 or 4). Mode 4 enables delay,
> > > +                     while mode 1 does not.
> > > +             - delay: Specifies the delay in hours (non-negative). This is
> > > +                     only used when 'mode' is set to 4.
> > > +             - start: The battery percentage at which charging starts (0-100).
> > > +             - stop: The battery percentage at which charging stops (1-100).
> > > +
> > > +              When the laptop is connected to a power adapter, it starts
> > > +              charging if the battery level is below the 'start' value. It
> > > +              continues charging until the battery reaches the 'stop' level.
> > > +              If the battery is already above the 'stop' level, charging is
> > > +              paused.
> > > +
> > > +              When the power adapter is always connected, charging will
> > > +              begin if the battery level falls below 'start', and charging
> > > +              will stop once the battery reaches 'stop'.
> > > +
> > > +              If mode is set to 4, the above charging mode will only occur
> > > +              after the specified delay in hours. If mode is 1, there is
> > > +              no delay.
> > > +
> > > +             Access: Read, Write
> > > +
> > > +             Valid values:
> > > +                     - mode: integer value (1 or 4)
> > > +                     - delay: integer value, delay in hours (non-negative)
> > > +                     - start: integer value, battery percentage (0-100)
> > > +                     - stop: integer value, battery percentage (1-100)
> > 
> > There are common properties for start and stop charging percentage,
> > which should be used:
> > 
> > * POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD
> > * POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD
> > 
> 
> Agree, but at least, we should pass delay, start, end. EC only
> providedone interface to set mode and delay, that requires 4
> arguments, we can handle it with 3 arguments, as you suggested
> below. but if we treat start and end separated, then if we want
> to set smart charge, we set start, set end, set delay(read start
> read end, then set them again). It is a bit redundant.

Yes, if these are separate properties you won't get atomic updates.
But is that really a problem? Using the standard properties means
that you get UI support in the future. I know at least the GNOME
people are working on this.

> > For the charge mode it seems there is no need to expose anything.
> > You can have a single property for the charge delay in hours. If
> > '0' is written to it there is no delay, so you can use mode 1 and
> > otherwise you can use mode 4. There is no need for this multi-value
> > mess. The delay thing seems to be quite specific to this EC, so a
> > custom property for that is fine.
> > 
> 
> Agree, mentioned above

[...]

> > > +static void gaokun_psy_init(struct gaokun_psy *ecbat)
> > > +{
> > > +     gaokun_psy_get_bat_present(ecbat);
> > 
> > why?
> > 
> 
> EC provided a way to check if battery is presented, if there is no
> battery, then we don't fetch battery info, but other info
> (i.e. adapter) is still available.

nevermind, I miss-read and wondered why gaokun_psy_bat_present is
being called twice.

> > > +     if (!gaokun_psy_bat_present(ecbat))
> > > +             return;
> > 
> > You only call it in your probe function, so the following will
> > remain uninitialized if the battery was not present at boot time.
> 
> mentioned above, keep them uninitialized is unharmful.

Does the battery not support hot-plug?

-- Sebastian

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists