Message-ID: <3a88c45a-5f26-3a14-3ae8-aa09c00b1431@linux.intel.com>
Date: Fri, 17 Jan 2025 17:51:36 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
cc: Pengyu Luo <mitltlatltl@...il.com>, andersson@...nel.org, 
    conor+dt@...nel.org, devicetree@...r.kernel.org, 
    Hans de Goede <hdegoede@...hat.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@...ck-us.net, platform-driver-x86@...r.kernel.org, robh@...nel.org
Subject: Re: [PATCH v4 2/3] platform: arm64: add Huawei Matebook E Go EC
 driver

On Fri, 17 Jan 2025, Bryan O'Donoghue wrote:

> On 16/01/2025 18:15, Pengyu Luo wrote:
> > On Fri, Jan 17, 2025 at 1:31 AM Bryan O'Donoghue
> > <bryan.odonoghue@...aro.org> wrote:
> > > On 16/01/2025 11:15, Pengyu Luo wrote:
> > > > +
> > > > +     guard(mutex)(&ec->lock);
> > > > +     i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> > > 
> > > You should trap the result code of i2c_transfer() and push it up the
> > > call stack.
> > > 
> > 
> > This EC uses SMBus Protocol, I guess. Qualcomm I2C driver doesn't support
> > this though. The response structure define by SMBus I mentioned them above
> > (Please also check ACPI specification 13.2.5)
> 
> What difference does that make ? The i2c controller itself can return error
> codes via i2c_transfer().
> 
> You should trap those error codes and take action if they happen.
> 
> > 
> > +/*
> > + * For rx, data sequences are arranged as
> > + * {status, data_len(unreliable), data_seq}
> > + */
> > 
> > So the first byte is status code.
> > 
> > > > +     usleep_range(2000, 2500); /* have a break, ACPI did this */
> > > > +
> > > > +     return *resp ? -EIO : 0;
> > > 
> > > If the value @ *resp is non-zero return -EIO ?
> > > 
> > > Why ?
> > > 
> > 
> > Mentioned above.
> 
> Right, please try to take the result code of i2c_transfer() and if it
> indicates error, transmit that error up the call stack.
> 
> 
> > 
> > > > +}
> > > > +
> > > > +/*
> > > > --------------------------------------------------------------------------
> > > > */
> > > > +/* Common API */
> > > > +
> > > > +/**
> > > > + * gaokun_ec_read - Read from EC
> > > > + * @ec: The gaokun_ec structure
> > > > + * @req: The sequence to request
> > > > + * @resp_len: The size to read
> > > > + * @resp: The buffer to store response sequence
> > > > + *
> > > > + * This function is used to read data after writing a magic sequence to
> > > > EC.
> > > > + * All EC operations depend on this function.
> > > > + *
> > > > + * Huawei uses magic sequences everywhere to complete various
> > > > functions, all
> > > > + * these sequences are passed to ECCD(a ACPI method which is quiet
> > > > similar
> > > > + * to gaokun_ec_request), there is no good abstraction to generalize
> > > > these
> > > > + * sequences, so just wrap it for now. Almost all magic sequences are
> > > > kept
> > > > + * in this file.
> > > > + *
> > > > + * Return: 0 on success or negative error code.
> > > > + */
> > > > +int gaokun_ec_read(struct gaokun_ec *ec, const u8 *req,
> > > > +                size_t resp_len, u8 *resp)
> > > > +{
> > > > +     return gaokun_ec_request(ec, req, resp_len, resp);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(gaokun_ec_read);
> > > > +
> > > > +/**
> > > > + * gaokun_ec_write - Write to EC
> > > > + * @ec: The gaokun_ec structure
> > > > + * @req: The sequence to request
> > > > + *
> > > > + * This function has no big difference from gaokun_ec_read. When caller
> > > > care
> > > > + * only write status and no actual data are returned, then use it.
> > > > + *
> > > > + * Return: 0 on success or negative error code.
> > > > + */
> > > > +int gaokun_ec_write(struct gaokun_ec *ec, const u8 *req)
> > > > +{
> > > > +     u8 ec_resp[] = MKRESP(0);
> > > > +
> > > > +     return gaokun_ec_request(ec, req, sizeof(ec_resp), ec_resp);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(gaokun_ec_write);
> > > > +
> > > > +int gaokun_ec_read_byte(struct gaokun_ec *ec, const u8 *req, u8 *byte)
> > > > +{
> > > > +     int ret;
> > > > +     u8 ec_resp[] = MKRESP(sizeof(*byte));
> > > > +
> > > > +     ret = gaokun_ec_read(ec, req, sizeof(ec_resp), ec_resp);
> > > > +     extr_resp_byte(byte, ec_resp);
> > > > +
> > > > +     return ret;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(gaokun_ec_read_byte);
> > > > +
> > > > +/**
> > > > + * gaokun_ec_register_notify - Register a notifier callback for EC
> > > > events.
> > > > + * @ec: The gaokun_ec structure
> > > > + * @nb: Notifier block pointer to register
> > > > + *
> > > > + * Return: 0 on success or negative error code.
> > > > + */
> > > > +int gaokun_ec_register_notify(struct gaokun_ec *ec, struct
> > > > notifier_block *nb)
> > > > +{
> > > > +     return blocking_notifier_chain_register(&ec->notifier_list, nb);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(gaokun_ec_register_notify);
> > > > +
> > > > +/**
> > > > + * gaokun_ec_unregister_notify - Unregister notifier callback for EC
> > > > events.
> > > > + * @ec: The gaokun_ec structure
> > > > + * @nb: Notifier block pointer to unregister
> > > > + *
> > > > + * Unregister a notifier callback that was previously registered with
> > > > + * gaokun_ec_register_notify().
> > > > + */
> > > > +void gaokun_ec_unregister_notify(struct gaokun_ec *ec, struct
> > > > notifier_block *nb)
> > > > +{
> > > > +     blocking_notifier_chain_unregister(&ec->notifier_list, nb);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(gaokun_ec_unregister_notify);
> > > > +
> > > > +/*
> > > > --------------------------------------------------------------------------
> > > > */
> > > > +/* API for PSY */
> > > > +
> > > > +/**
> > > > + * gaokun_ec_psy_multi_read - Read contiguous registers
> > > > + * @ec: The gaokun_ec structure
> > > > + * @reg: The start register
> > > > + * @resp_len: The number of registers to be read
> > > > + * @resp: The buffer to store response sequence
> > > > + *
> > > > + * Return: 0 on success or negative error code.
> > > > + */
> > > > +int gaokun_ec_psy_multi_read(struct gaokun_ec *ec, u8 reg,
> > > > +                          size_t resp_len, u8 *resp)
> > > > +{
> > > > +     u8 ec_req[] = MKREQ(0x02, EC_READ, 1, 0);
> > > > +     u8 ec_resp[] = MKRESP(1);
> > > > +     int i, ret;
> > > > +
> > > > +     for (i = 0; i < resp_len; ++i, reg++) {
> > > > +             refill_req_byte(ec_req, &reg);
> > > > +             ret = gaokun_ec_read(ec, ec_req, sizeof(ec_resp),
> > > > ec_resp);
> > > > +             if (ret)
> > > > +                     return ret;
> > > > +             extr_resp_byte(&resp[i], ec_resp);
> > > > +     }
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(gaokun_ec_psy_multi_read);
> > > > +
> > > > +/* Smart charge */
> > > > +
> > > > +/**
> > > > + * gaokun_ec_psy_get_smart_charge - Get smart charge data from EC
> > > > + * @ec: The gaokun_ec structure
> > > > + * @resp: The buffer to store response sequence (mode, delay, start,
> > > > end)
> > > > + *
> > > > + * Return: 0 on success or negative error code.
> > > > + */
> > > > +int gaokun_ec_psy_get_smart_charge(struct gaokun_ec *ec,
> > > > +                                u8 resp[GAOKUN_SMART_CHARGE_DATA_SIZE])
> > > > +{
> > > > +     /* GBCM */
> > > > +     u8 ec_req[] = MKREQ(0x02, 0xE4, 0);
> > > > +     u8 ec_resp[] = MKRESP(GAOKUN_SMART_CHARGE_DATA_SIZE);
> > > > +     int ret;
> > > > +
> > > > +     ret = gaokun_ec_read(ec, ec_req, sizeof(ec_resp), ec_resp);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     extr_resp(resp, ec_resp, GAOKUN_SMART_CHARGE_DATA_SIZE);
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(gaokun_ec_psy_get_smart_charge);
> > > > +
> > > > +static inline bool are_thresholds_valid(u8 start, u8 end)
> > > > +{
> > > > +     return end != 0 && start <= end && end <= 100;
> > > 
> > > Why 100 ? Still feels like an arbitrary number.
> > > 
> > > Could you add a comment to explain where 100 comes from ?
> > > 
> > 
> > You may don't get it. It is just a battery percentage, greater than 100 is
> > invalid.
> 
> 100 meaning maximum capacity, good stuff.
> 
> Please use a define with a descriptive name. That way the meaning is obvious.
> 
> In fact if the name of the function related to battery capacity then the
> meaning of the numbers would be more obvious.
> 
> static inline bool validate_battery_threshold_range(u8 start, u8 end) {
> 	return end != 0 && start <= end && end <= 100;
> }

I suggest going with this latter option. 100% tends to be in its literal 
form elsewhere too. I suppose we don't even have a generic define for 
"100%", at least I don't recall coming across one nor found any with a 
quick git grep underneath include/.

But I agree the naming of this function could be improved like you 
suggest.

-- 
 i.

> > 
> > start: The battery percentage at which charging starts (0-100).
> > stop: The battery percentage at which charging stops (1-100).
> 
> Or just add this comment directly above the function.
> 
> ---
> bod
> 

Powered by blists - more mailing lists