Re: [PATCH v3 5/6] power: supply: add Huawei Matebook E Go psy driver
From: Sebastian Reichel
Date: Thu Feb 20 2025 - 20:34:49 EST
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@xxxxxxxxxxxxx> 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@xxxxxxxxx>
> > > ---
> > > .../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@xxxxxxxxx>
> > > +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
Attachment:
signature.asc
Description: PGP signature