Message-ID: <20250221060143.201963-1-mitltlatltl@gmail.com>
Date: Fri, 21 Feb 2025 14:01:04 +0800
From: Pengyu Luo <mitltlatltl@...il.com>
To: sebastian.reichel@...labora.com
Cc: linux-kernel@...r.kernel.org,
	linux-pm@...r.kernel.org,
	mitltlatltl@...il.com
Subject: Re: [PATCH v3 5/6] power: supply: add Huawei Matebook E Go psy driver

On Fri, Feb 21, 2025 at 9:33 AM Sebastian Reichel <sebastian.reichel@...labora.com> wrote:
> 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.
>

On my another x86_64 device with end threshold supported, KDE Plasma
supports showing this as

> Battery is configured to charge up to aproximately <value>%

it doesn't support setting things. So, can I keep passing delay, start,
end when setting, but also setting start and end as battery properties?

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

I am not sure, this is a tablet shipped with a qualcomm chip, we can
take it as an embedded device. I just took normal usage and using
without battery into account.

Best wishes,
Pengyu

Powered by blists - more mailing lists