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

[Pengyu Luo <mitltlatltl@gmail.com>]

On Mon, Dec 30, 2024 at 5:04 PM Aiqun(Maria) Yu <quic_aiquny@quicinc.com> wrote:
> On 12/28/2024 1:13 AM, Pengyu Luo wrote:
> > 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).

[...]

> > +#include <linux/mutex.h>
> > +#include <linux/version.h>
> > +
> > +#include <linux/platform_data/huawei-gaokun-ec.h>
> > +
> > +#define EC_EVENT             0x06
> > +
> > +/* Also can be found in ACPI specification 12.3 */
>
> It appears that the following EC commands are common to all ACPI-applied
> embedded controllers. Is it possible to standardize these commands and API?
>

No, I mentioned a little in kerneldoc, EC_READ only works for psy
related things.

> > +#define EC_READ                      0x80
> > +#define EC_WRITE             0x81
> > +#define EC_BURST             0x82
> > +#define EC_QUERY             0x84
> > +
> > +
> > +#define EC_EVENT_LID         0x81
> > +
> > +#define EC_LID_STATE         0x80
> > +#define EC_LID_OPEN          BIT(1)
> > +
> > +#define UCSI_REG_SIZE                7
> > +
> > +/* for tx, command sequences are arranged as
> > + * {master_cmd, slave_cmd, data_len, data_seq}
> > + */
> > +#define REQ_HDR_SIZE         3
> > +#define INPUT_SIZE_OFFSET    2
> > +#define INPUT_DATA_OFFSET    3
> > +
> > +/* for rx, data sequences are arranged as
> > + * {status, data_len(unreliable), data_seq}
> > + */
> > +#define RESP_HDR_SIZE                2
> > +#define DATA_OFFSET          2
> > +
> > +
> > +struct gaokun_ec {
> > +     struct i2c_client *client;
> > +     struct mutex lock;
> > +     struct blocking_notifier_head notifier_list;
> > +     struct input_dev *idev;
> > +     bool suspended;
> > +};
> > +
> > +static int gaokun_ec_request(struct gaokun_ec *ec, const u8 *req,
> > +                          size_t resp_len, u8 *resp)
> > +{
> > +     struct i2c_client *client = ec->client;
> > +     struct i2c_msg msgs[2] = {
> > +             {
> > +                     .addr = client->addr,
> > +                     .flags = client->flags,
> > +                     .len = req[INPUT_SIZE_OFFSET] + REQ_HDR_SIZE,
> > +                     .buf = req,
> > +             }, {
> > +                     .addr = client->addr,
> > +                     .flags = client->flags | I2C_M_RD,
> > +                     .len = resp_len,
> > +                     .buf = resp,
> > +             },
> > +     };
> > +
> > +     mutex_lock(&ec->lock);
> > +
> > +     i2c_transfer(client->adapter, msgs, 2);
>
> ARRAY_SIZE(msgs) is suggested instead of pure 2.
>

Agree

> > +     usleep_range(2000, 2500);
>
> Why is a sleep needed here? Is this information specified in any datasheet?
>

Have a break between 2 transaction. This sleep happens in acpi code, also
inside a critical region. I rearranged it.

Local7 = Acquire (\_SB.IC16.MUEC, 0x03E8)
...
write ops
...
Sleep (0x02)
...
read ops
...
Release (\_SB.IC16.MUEC)

> > +
> > +     mutex_unlock(&ec->lock);
> > +
> > +     return *resp;
> > +}
> > +
> > +/* -------------------------------------------------------------------------- */
> > +/* Common API */
> > +
> > +/**
> > + * gaokun_ec_read - read from EC
> > + * @ec: The gaokun_ec
> > + * @req: The sequence to request
> > + * @resp_len: The size to read
> > + * @resp: Where the data are read to
> > + *
> > + * This function is used to read data after writing a magic sequence to EC.
> > + * All EC operations dependent on this functions.
> > + *
> > + * 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.
> > + */
> > +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
> > + * @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 returnd, then use it.
> > + */
> > +int gaokun_ec_write(struct gaokun_ec *ec, u8 *req)
> > +{
> > +     u8 resp[RESP_HDR_SIZE];
> > +
> > +     return gaokun_ec_request(ec, req, sizeof(resp), resp);
> > +}
> > +EXPORT_SYMBOL_GPL(gaokun_ec_write);
> > +
> > +int gaokun_ec_read_byte(struct gaokun_ec *ec, u8 *req, u8 *byte)
> > +{
> > +     int ret;
> > +     u8 resp[RESP_HDR_SIZE + sizeof(*byte)];
> > +
> > +     ret = gaokun_ec_read(ec, req, sizeof(resp), resp);
> > +     *byte = resp[DATA_OFFSET];
> > +
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(gaokun_ec_read_byte);
> > +
> > +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);
> > +
> > +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 */
> > +
> > +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, };
>
> Could it be made more readable by specifying the macro names for 0x02
> and 1? This would help in understanding the meaning of these numbers.
>

I really don't know the meaning of master command 0x02, 1 is the size for
the data_seq behind of it. There are many possible sizes. It is not a good
idea to define a macro name for everyone.

> Also, please ensure the actual size of the request buffer is handled
> properly. In gaokun_ec_request(), the req is passed down directly, and
> the i2c_msg.len is used dynamically with req[INPUT_SIZE_OFFSET] +
> REQ_HDR_SIZE. This requires the caller to carefully manage the contents
> to avoid memory over-read, making the code difficult to read.
>
> Creating a defined macro can help you avoid manually defining the size.
> For example:
> #define REQ(size, data_0, data_1, args...) \
> u8 req[REQ_HDR_SIZE + size] = {data_0, data_1, size, args};
>

I think wrapping like this is not recommended, see '5)' in [1]

Best wishes,
Pengyu

[1] https://www.kernel.org/doc/html/v4.10/process/coding-style.html#macros-enums-and-rtl