On Tue, 14 Jan 2025, Pengyu Luo wrote: > On Tue, Jan 14, 2025 at 2:56 AM Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx> 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@xxxxxxxxx> > > > +/** > > > + * 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.
