Re: [PATCH v3 2/6] platform: arm64: add Huawei Matebook E Go EC driver

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 5290 bytes --]

On Tue, 14 Jan 2025, Pengyu Luo wrote:
> On Tue, Jan 14, 2025 at 2:56 AM Ilpo Järvinen <ilpo.jarvinen@linux.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@3.0GHz), codename must be gaokun3. (see [1])
> > > Huawei Matebook E Go 2023(sc8280xp@2.69GHz), 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@gmail.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.