Re: [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver

[Pengyu Luo <mitltlatltl@gmail.com>]

On Sat, Dec 28, 2024 at 2:21 AM Maya Matuszczyk <maccraft123mc@gmail.com> wrote:
> Good to see someone else doing EC drivers for arm64 laptops!
>

Yeah, I have worked on it for a while. I really don't know why don't some
mechines use a pmic glink. AFAIK, there are some WOA devices without EC.
Mechines can definitely work without it in a way.

I am also glad that you are reviewing my code.

> pt., 27 gru 2024 o 18:16 Pengyu Luo <mitltlatltl@gmail.com> napisał(a):
> >
> > There are 3 variants, Huawei released first 2 at the same time.
> > Huawei Matebook E Go LTE(sc8180x), codename should be gaokun2.
> > Huawei Matebook E Go(sc8280xp@3.0GHz), codename is gaokun3.
> > Huawei Matebook E Go 2023(sc8280xp@2.69GHz).
> >
> > Adding support for the latter two variants for now, this driver should
> > also work for the sc8180x variant according to acpi table files, but I
> > don't have the device yet.
> >
> > Different from other Qualcomm Snapdragon sc8280xp based machines, the
> > Huawei Matebook E Go uses an embedded controller while others use
> > something called pmic glink. This embedded controller can be used to
> > perform a set of various functions, including, but not limited:
> >
> > - Battery and charger monitoring;
> > - Charge control and smart charge;
> > - Fn_lock settings;
> > - Tablet lid status;
> > - Temperature sensors;
> > - USB Type-C notifications (ports orientation,  DP alt mode HPD);
> > - USB Type-C PD (according to observation, up to 48w).
> >
> > Add the driver for the EC, that creates devices for UCSI, wmi and power
> > supply devices.
> >
> > Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
> > ---
> >  drivers/platform/arm64/Kconfig                |  19 +
> >  drivers/platform/arm64/Makefile               |   2 +
> >  drivers/platform/arm64/huawei-gaokun-ec.c     | 598 ++++++++++++++++++
> >  drivers/platform/arm64/huawei-gaokun-wmi.c    | 283 +++++++++
> >  .../linux/platform_data/huawei-gaokun-ec.h    |  90 +++
> >  5 files changed, 992 insertions(+)
> >  create mode 100644 drivers/platform/arm64/huawei-gaokun-ec.c
> >  create mode 100644 drivers/platform/arm64/huawei-gaokun-wmi.c
> >  create mode 100644 include/linux/platform_data/huawei-gaokun-ec.h

[...]

> > +/* -------------------------------------------------------------------------- */
> > +/* API For PSY */
> > +
> > +int gaokun_ec_psy_multi_read(struct gaokun_ec *ec, u8 reg,
> > +                            size_t resp_len, u8 *resp)
> > +{
> > +       int i, ret;
> > +       u8 _resp[RESP_HDR_SIZE + 1];
> > +       u8 req[REQ_HDR_SIZE + 1] = {0x02, EC_READ, 1, };
> > +
> > +       for (i = 0; i < resp_len; ++i) {
> > +               req[INPUT_DATA_OFFSET] = reg++;
> > +               ret = gaokun_ec_read(ec, req, sizeof(_resp), _resp);
> > +               if (ret)
> > +                       return -EIO;
> > +               resp[i] = _resp[DATA_OFFSET];
> > +       }
> > +
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(gaokun_ec_psy_multi_read);
> > +
> > +/* -------------------------------------------------------------------------- */
> > +/* API For WMI */
> WMI is in ACPI, this laptop doesn't boot with ACPI so why are things
> handled in WMI separate from rest of the driver?
>

This driver reimplemented WMI functoins, and it perform a lot of
operations, to avoid naming it, just call it WMI, make life easier.

Once I considered keeping WMI things in this file, but it makes this file
bloated. Then I splitted every part into a module.

> > +
> > +/* Battery charging threshold */
> > +int gaokun_ec_wmi_get_threshold(struct gaokun_ec *ec, u8 *value, int ind)
> > +{
> > +       /* GBTT */
> > +       return gaokun_ec_read_byte(ec, (u8 []){0x02, 0x69, 1, ind}, value);
> > +}
> > +EXPORT_SYMBOL_GPL(gaokun_ec_wmi_get_threshold);
> > +
> > +int gaokun_ec_wmi_set_threshold(struct gaokun_ec *ec, u8 start, u8 end)
> > +{
> > +       /* SBTT */
> > +       int ret;
> > +       u8 req[REQ_HDR_SIZE + 2] = {0x02, 0x68, 2, 3, 0x5a};
> > +
> > +       ret = gaokun_ec_write(ec, req);
> > +       if (ret)
> > +               return -EIO;
> > +
> > +       if (start == 0 && end == 0)
> > +               return -EINVAL;
> > +
> > +       if (start >= 0 && start <= end && end <= 100) {
> > +               req[INPUT_DATA_OFFSET] = 1;
> > +               req[INPUT_DATA_OFFSET + 1] = start;
> > +               ret = gaokun_ec_write(ec, req);
> > +               if (ret)
> > +                       return -EIO;
> > +
> > +               req[INPUT_DATA_OFFSET] = 2;
> > +               req[INPUT_DATA_OFFSET + 1] = end;
> > +               ret = gaokun_ec_write(ec, req);
> > +       } else {
> > +               return -EINVAL;
> > +       }
> > +
> > +       return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(gaokun_ec_wmi_set_threshold);

[...]

> > +/* -------------------------------------------------------------------------- */
> > +/* EC */
> > +
> > +static irqreturn_t gaokun_ec_irq_handler(int irq, void *data)
> > +{
> > +       struct gaokun_ec *ec = data;
> > +       u8 req[REQ_HDR_SIZE] = {EC_EVENT, EC_QUERY, 0};
> > +       u8 status, id;
> > +       int ret;
> > +
> > +       ret = gaokun_ec_read_byte(ec, req, &id);
> > +       if (ret)
> > +               return IRQ_HANDLED;
> The error should probably be logged instead of silently ignored
>

Generally, one or two I/O errors don't crash anything, but actually if we
just do as ACPI methoda did, there should not be any error. It may be
necessary for debugging at the early stage of developemnt. If you suggest,
then we can add a report to the lower function (gaokun_ec_request) to check
every transaction. BTW, lenovo c630 also ignored them.

> > +
> > +       switch (id) {
> > +       case 0x0: /* No event */
> > +               break;
> > +
> > +       case EC_EVENT_LID:
> > +               gaokun_ec_psy_read_byte(ec, EC_LID_STATE, &status);
> > +               status = EC_LID_OPEN & status;
> > +               input_report_switch(ec->idev, SW_LID, !status);
> > +               input_sync(ec->idev);
> > +               break;
> > +
> > +       default:
> > +               blocking_notifier_call_chain(&ec->notifier_list, id, ec);
> > +       }
> > +
> > +       return IRQ_HANDLED;
> > +}
> > +

[...]

> > diff --git a/drivers/platform/arm64/huawei-gaokun-wmi.c b/drivers/platform/arm64/huawei-gaokun-wmi.c
> > new file mode 100644
> > index 000000000..793cb1659
> > --- /dev/null
> > +++ b/drivers/platform/arm64/huawei-gaokun-wmi.c
> > @@ -0,0 +1,283 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * huawei-gaokun-wmi - A WMI driver for HUAWEI Matebook E Go (sc8280xp)
> Just because in ACPI it's done by WMI stuff doesn't mean the non-ACPI
> driver has to reflect this
>

As I just said, and the WMI stuffs are all implemented by wrapping a EC
transaction in ACPI, at least in this machine.

> > + *
> > + * reference: drivers/platform/x86/huawei-wmi.c
> > + *
> > + * Copyright (C) 2024 Pengyu Luo <mitltlatltl@gmail.com>
> > + */
> > +
> > +#include <linux/auxiliary_bus.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/version.h>
> > +
> > +#include <linux/platform_data/huawei-gaokun-ec.h>
> > +
> >

[...]

> >
>
> Best Regards,
> Maya Matuszczyk

Best Wishes,
Pengyu