Message-ID: <d2a42fc7-37a9-3fcc-4c35-e542ddb112e8@linux.intel.com>
Date: Tue, 14 Jan 2025 17:40:51 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Pengyu Luo <mitltlatltl@...il.com>
cc: andersson@...nel.org, bryan.odonoghue@...aro.org, conor+dt@...nel.org, 
    devicetree@...r.kernel.org, 
    Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
    Hans de Goede <hdegoede@...hat.com>, heikki.krogerus@...ux.intel.com, 
    jdelvare@...e.com, konradybcio@...nel.org, krzk+dt@...nel.org, 
    linux-arm-msm@...r.kernel.org, linux-hwmon@...r.kernel.org, 
    LKML <linux-kernel@...r.kernel.org>, linux-pm@...r.kernel.org, 
    linux-usb@...r.kernel.org, linux@...ck-us.net, 
    platform-driver-x86@...r.kernel.org, robh@...nel.org, sre@...nel.org
Subject: Re: [PATCH v3 2/6] platform: arm64: add Huawei Matebook E Go EC
 driver

On Tue, 14 Jan 2025, Pengyu Luo wrote:
> On Tue, Jan 14, 2025 at 2:56 AM Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com> wrote:
> > On Tue, 14 Jan 2025, Pengyu Luo wrote:
> > 
> > > There are three variants of which Huawei released the first two
> > > simultaneously.
> > >
> > > Huawei Matebook E Go LTE(sc8180x), codename seems to be gaokun2.
> > > Huawei Matebook E Go(sc8280xp@...GHz), codename must be gaokun3. (see [1])
> > > Huawei Matebook E Go 2023(sc8280xp@...9GHz), codename should be also gaokun3.
> > >
> > > 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 to test yet.
> > >
> > > Different from other Qualcomm Snapdragon sc8280xp based machines, the
> > > Huawei Matebook E Go uses an embedded controller while others use
> > > a system called PMIC GLink. This embedded controller can be used to
> > > perform a set of various functions, including, but not limited to:
> > >
> > > - 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 a driver for the EC which creates devices for UCSI and power supply
> > > devices.
> > >
> > > [1] https://bugzilla.kernel.org/show_bug.cgi?id=219645
> > >
> > > Signed-off-by: Pengyu Luo <mitltlatltl@...il.com>

> > > +/**
> > > + * gaokun_ec_psy_get_smart_charge_enable - check if smart charge is enabled
> > > + * @ec: The gaokun_ec
> > > + * @on: The state
> > > + *
> > > + * Return: 0 on success or negative error code.
> > > + */
> > > +int gaokun_ec_psy_get_smart_charge_enable(struct gaokun_ec *ec, bool *on)
> > > +{
> > > +     /* GBAC */
> > > +     *on = 0; /* clear other 3 Bytes */
> > 
> > = false (as it's bool)
> > 
> > What that comment means??? The type is bool so what "3 Bytes" ???
> > 
> 
> We will write to the lowest Byte, the higher 3 Bytes are dirty, so clear it.

Are you saying you assume bool is 4 bytes long? I'd be cautious on making 
assumptions on sizeof(bool).
 
> We can also implememnt it like this
> 
> int ret;
> u8 resp;
> 
> ret = gaokun_ec_read_byte(.., &resp);
> if (ret)
>         return ret;
> 
> *on = !!resp;

Yes, I prefer explicit u8 -> bool conversion like this.

> > > +/* Fn lock */
> > > +static int gaokun_ec_get_fn_lock(struct gaokun_ec *ec, bool *on)
> > > +{
> > > +     /* GFRS */
> > > +     u8 req[] = MKREQ(0x02, 0x6B, 0);
> > 
> > Does that random acronym map to one of the literal? In which case a define
> > would be more useful than a comment. (You seem to have a few similar
> > comments preceeding the req definitions)
> > 
> 
> They are ACPI method names/identifiers, it will be useful if someone want
> to locate ACPI's implementations.

Okay, I guess it's fine as is then.


> > > +static int gaokun_ec_get_temp(struct gaokun_ec *ec, u8 idx, int *temp)
> > > +{
> > > +     /* GTMP */
> > > +     u8 req[] = MKREQ(0x02, 0x61, 1, temp_reg[idx]);
> > > +     u8 resp[] = MKRESP(sizeof(__le16));
> > > +     __le16 tmp;
> > > +     int ret;
> > > +
> > > +     ret = gaokun_ec_read(ec, req, sizeof(resp), resp);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     extr_resp((u8 *)&tmp, resp, sizeof(tmp));
> > > +     *temp = le16_to_cpu(tmp) * 100; /* convert to HwMon's unit */
> > 
> > extr_resp() does memcpy() but there should be no need to copy anything
> > here. You just want to have __le16 pointer of the response data data.
> > 
> 
> I think this would break abstraction, recently, these data are accessed by
> extr_resp() and refill_req() only.

If you want to keep doing it like that, not a big deal for me.

There are different ways to do the abstraction though, and not all require
memcpy() when changing a layer (e.g., a pointer advancing to the other 
layer).

> > > +/* -------------------------------------------------------------------------- */
> > > +/* EC */
> > > +
> > > +static irqreturn_t gaokun_ec_irq_handler(int irq, void *data)
> > > +{
> > > +     struct gaokun_ec *ec = data;
> > > +     u8 req[] = MKREQ(EC_EVENT, EC_QUERY, 0);
> > 
> > Great, here you have named them. Could you name all of the other literals
> > too, please.
> 
> I mentioned this in previous version. Most of them are magic, it is hard to
> generalize them. We could name partial scmd according to specific functions
> (sysfs functions), their function names have implied registers' meaning, and
> these registers would be never reused in other functions.

Fair (I didn't read every comment made to the previous version).

> > > +/* -------------------------------------------------------------------------- */
> > > +/* API For UCSI */
> > 
> > for
> > 
> 
> Agree

For me, you don't need to reply "Agree", "Ack" or something along those 
lines if you're going to act on the feedback. Just make sure you don't 
forget them :-). It'll save us both some time when we focus on points that 
need further discussion.

-- 
 i.

Powered by blists - more mailing lists