On 27/12/2024 17:13, Pengyu Luo wrote:

There are 3 variants, Huawei released first 2 at the same time.

There are three variants of which Huawei released the first two 
simultaneously.

Huawei Matebook E Go LTE(sc8180x), codename should be gaokun2.
Huawei Matebook E Go(sc8280xp@3.0GHz), codename is gaokun3.

Choose either "codename should be" or "codename is"

Huawei Matebook E Go 2023(sc8280xp@2.69GHz).

Maybe say here "codename unknown"

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 yet.

Different from other Qualcomm Snapdragon sc8280xp based machines, the
Huawei Matebook E Go uses an embedded controller while others use
something called pmic glink. This embedded controller can be used to
perform a set of various functions, including, but not limited:

"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 the driver for the EC, that creates devices for UCSI, wmi and power
supply devices.

I'm a terrible man for the "," dropped all about the place but you're 
going too mad with the commans there ->

"Add a driver for the EC which creates devices for UCSI, WMI and power 
supply devices"

Krzysztof already mentioned this but the "sc8280xp" is questionable, you 
should probably drop mention of qcom and sc8280xp from your compat and 
tristate here.

+	depends on ARCH_QCOM || COMPILE_TEST
+	depends on I2C
+	depends on DRM
+	depends on POWER_SUPPLY
+	depends on INPUT
+	help
+	  Say Y here to enable the EC driver for Huawei Matebook E Go 2in1
+	  tablet. The driver handles battery(information, charge control) and
+	  USB Type-C DP HPD events as well as some misc functions like the lid
+	  sensor and temperature sensors, etc.
+
+	  This driver provides battery and AC status support for the mentioned
+	  laptop where this information is not properly exposed via the
+	  standard ACPI devices.
+
+	  Say M or Y here to include this support.

OTOH the help text is where you could mention the sc8280xp class of 
laptops/tablets.

Is this odd indentation ?

+#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);
+	usleep_range(2000, 2500);

What is this sleep about and why are you doing it inside of a critical 
section ?

+
+	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.

depend on this

+ *
+ * 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, };

Instead of constructing your packet inline like this, suggest a 
dedicated function to construct a request packet.

For example 1 @ INPUT_SIZE_OFFSET => the size of data a dedicated 
function will make the "stuffing" of the request frame more obvious to 
readers and make the construction of packets less error prone.

+
+	for (i = 0; i < resp_len; ++i) {
+		req[INPUT_DATA_OFFSET] = reg++;
+		ret = gaokun_ec_read(ec, req, sizeof(_resp), _resp);
+		if (ret)
+			return -EIO;
+		resp[i] = _resp[DATA_OFFSET];
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gaokun_ec_psy_multi_read);
+
+/* -------------------------------------------------------------------------- */
+/* API For WMI */
+
+/* Battery charging threshold */
+int gaokun_ec_wmi_get_threshold(struct gaokun_ec *ec, u8 *value, int ind)
+{
+	/* GBTT */
+	return gaokun_ec_read_byte(ec, (u8 []){0x02, 0x69, 1, ind}, value);
+}
+EXPORT_SYMBOL_GPL(gaokun_ec_wmi_get_threshold);
+
+int gaokun_ec_wmi_set_threshold(struct gaokun_ec *ec, u8 start, u8 end)
+{
+	/* SBTT */
+	int ret;
+	u8 req[REQ_HDR_SIZE + 2] = {0x02, 0x68, 2, 3, 0x5a};
+
+	ret = gaokun_ec_write(ec, req);
+	if (ret)
+		return -EIO;
+
+	if (start == 0 && end == 0)
+		return -EINVAL;
+
+	if (start >= 0 && start <= end && end <= 100) {

if start >= 0

is redundant no ? start is a u8 it can _only_ be >= 0 ..

+		req[INPUT_DATA_OFFSET] = 1;
+		req[INPUT_DATA_OFFSET + 1] = start;
+		ret = gaokun_ec_write(ec, req);
+		if (ret)
+			return -EIO;
+
+		req[INPUT_DATA_OFFSET] = 2;
+		req[INPUT_DATA_OFFSET + 1] = end;

again a function to construct a packet gets you out of the business of 
inlining and "just knowing" which offset is which within any give 
function which indexes an array.

+		ret = gaokun_ec_write(ec, req);
+	} else {
+		return -EINVAL;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(gaokun_ec_wmi_set_threshold);
+
+/* Smart charge param */
+int gaokun_ec_wmi_get_smart_charge_param(struct gaokun_ec *ec, u8 *value)
+{
+	/* GBAC */
+	return gaokun_ec_read_byte(ec, (u8 []){0x02, 0xE6, 0}, value);
+}
+EXPORT_SYMBOL_GPL(gaokun_ec_wmi_get_smart_charge_param);
+
+int gaokun_ec_wmi_set_smart_charge_param(struct gaokun_ec *ec, u8 value)
+{
+	/* SBAC */
+	if (value < 0 || value > 2)

value < 0 can never be true

+		return -EINVAL;
+
+	return gaokun_ec_write(ec, (u8 []){0x02, 0xE5, 1, value});
+}
+EXPORT_SYMBOL_GPL(gaokun_ec_wmi_set_smart_charge_param);
+
+/* Smart charge */
+int gaokun_ec_wmi_get_smart_charge(struct gaokun_ec *ec,
+				   u8 data[GAOKUN_SMART_CHARGE_DATA_SIZE])
+{
+	/* GBCM */
+	u8 req[REQ_HDR_SIZE] = {0x02, 0xE4, 0};
+	u8 resp[RESP_HDR_SIZE + 4];
+	int ret;
+
+	ret = gaokun_ec_read(ec, req, sizeof(resp), resp);
+	if (ret)
+		return -EIO;
+
+	data[0] = resp[DATA_OFFSET];
+	data[1] = resp[DATA_OFFSET + 1];
+	data[2] = resp[DATA_OFFSET + 2];
+	data[3] = resp[DATA_OFFSET + 3];
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gaokun_ec_wmi_get_smart_charge);
+
+int gaokun_ec_wmi_set_smart_charge(struct gaokun_ec *ec,
+				   u8 data[GAOKUN_SMART_CHARGE_DATA_SIZE])
+{
+	/* SBCM */
+	u8 req[REQ_HDR_SIZE + GAOKUN_SMART_CHARGE_DATA_SIZE] = {0x02, 0XE3, 4,};
+
+	if (!(data[2] >= 0 && data[2] <= data[3] && data[3] <= 100))
+		return -EINVAL;

Repeat of the clause above which was checking u8 >= 0 for the same 
values in the rest of the clause - including checking <= 100.

Certainly a candidate for functional decomposition, inline function or a 
define.

+
+	memcpy(req + INPUT_DATA_OFFSET, data, GAOKUN_SMART_CHARGE_DATA_SIZE);
+
+	return gaokun_ec_write(ec, req);
+}
+EXPORT_SYMBOL_GPL(gaokun_ec_wmi_set_smart_charge);
+
+/* Fn lock */
+int gaokun_ec_wmi_get_fn_lock(struct gaokun_ec *ec, u8 *on)
+{
+	/* GFRS */
+	int ret;
+	u8 val;
+	u8 req[REQ_HDR_SIZE] = {0x02, 0x6B, 0};

Reverse Christmas tree

u8 req[REQ_HDR_SIZE];
int ret;
u8 val;

Not required but nice to look at.

+
+	ret = gaokun_ec_read_byte(ec, req, &val);
+	if (val == 0x55)
+		*on = 0;
+	else if (val == 0x5A)
+		*on = 1;
+	else
+		return -EIO;
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(gaokun_ec_wmi_get_fn_lock);
+
+int gaokun_ec_wmi_set_fn_lock(struct gaokun_ec *ec, u8 on)
+{
+	/* SFRS */
+	u8 req[REQ_HDR_SIZE + 1] = {0x02, 0x6C, 1,};
+
+	if (on == 0)
+		req[INPUT_DATA_OFFSET] = 0x55;
+	else if (on == 1)
+		req[INPUT_DATA_OFFSET] = 0x5A;
+	else
+		return -EINVAL;

Why not use a bool for on ?

+
+	return gaokun_ec_write(ec, req);
+}
+EXPORT_SYMBOL_GPL(gaokun_ec_wmi_set_fn_lock);
+
+/* Thermal Zone */
+/* Range from 0 to 0x2C, partial valid */
+static const u8 temp_reg[] = {0x05, 0x07, 0x08, 0x0E, 0x0F, 0x12, 0x15, 0x1E,
+			      0x1F, 0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26,
+			      0x27, 0x28, 0x29, 0x2A};
+
+int gaokun_ec_wmi_get_temp(struct gaokun_ec *ec, s16 temp[GAOKUN_TZ_REG_NUM])

int gaokun_ec_wmi_get_temp(struct gaokun_ec *ec, s16 *temp, size_t 
temp_reg_num)

+{
+	/* GTMP */
+	u8 req[REQ_HDR_SIZE] = {0x02, 0x61, 1,};
+	u8 resp[RESP_HDR_SIZE + sizeof(s16)];
+	int ret, i = 0;
+
+	while (i < GAOKUN_TZ_REG_NUM) {

while (i < temp_reg_num)

+		req[INPUT_DATA_OFFSET] = temp_reg[i];
+		ret = gaokun_ec_read(ec, req, sizeof(resp), resp);
+		if (ret)
+			return -EIO;
+		temp[i++] = *(s16 *)(resp + DATA_OFFSET);

What's the point of the casting here ?

memcpy(temp, resp, sizeof(s16));
temp++;

+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gaokun_ec_wmi_get_temp);
+
+/* -------------------------------------------------------------------------- */
+/* API For UCSI */
+
+int gaokun_ec_ucsi_read(struct gaokun_ec *ec,
+			u8 resp[GAOKUN_UCSI_READ_SIZE])
+{
+	u8 req[REQ_HDR_SIZE] = {0x3, 0xD5, 0};
+	u8 _resp[RESP_HDR_SIZE + GAOKUN_UCSI_READ_SIZE];
+	int ret;
+
+	ret = gaokun_ec_read(ec, req, sizeof(_resp), _resp);
+	if (ret)
+		return ret;
+
+	memcpy(resp, _resp + DATA_OFFSET, GAOKUN_UCSI_READ_SIZE);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gaokun_ec_ucsi_read);
+
+int gaokun_ec_ucsi_write(struct gaokun_ec *ec,
+			 const u8 req[GAOKUN_UCSI_WRITE_SIZE])
+{
+	u8 _req[REQ_HDR_SIZE + GAOKUN_UCSI_WRITE_SIZE];
+
+	_req[0] = 0x03;
+	_req[1] = 0xD4;
+	_req[INPUT_SIZE_OFFSET] = GAOKUN_UCSI_WRITE_SIZE;
+	memcpy(_req + INPUT_DATA_OFFSET, req, GAOKUN_UCSI_WRITE_SIZE);
+
+	return gaokun_ec_write(ec, _req);
+}
+EXPORT_SYMBOL_GPL(gaokun_ec_ucsi_write);
+
+int gaokun_ec_ucsi_get_reg(struct gaokun_ec *ec, u8 *ureg)
+{
+	u8 req[REQ_HDR_SIZE] = {0x03, 0xD3, 0};
+	u8 _resp[RESP_HDR_SIZE + UCSI_REG_SIZE];
+	int ret;
+
+	ret = gaokun_ec_read(ec, req, sizeof(_resp), _resp);
+	if (ret)
+		return ret;
+
+	memcpy(ureg, _resp + DATA_OFFSET, UCSI_REG_SIZE);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gaokun_ec_ucsi_get_reg);
+
+int gaokun_ec_ucsi_pan_ack(struct gaokun_ec *ec, int port_id)
+{
+	u8 req[REQ_HDR_SIZE + 1] = {0x03, 0xD2, 1, 0};
+
+	if (port_id >= 0)
+		req[INPUT_DATA_OFFSET] = 1 << port_id;
+
+	return gaokun_ec_write(ec, req);
+}
+EXPORT_SYMBOL_GPL(gaokun_ec_ucsi_pan_ack);
+
+/* -------------------------------------------------------------------------- */
+/* Modern Standby */
+
+static int gaokun_ec_suspend(struct device *dev)
+{
+	struct gaokun_ec *ec = dev_get_drvdata(dev);
+	u8 req[REQ_HDR_SIZE + 1] = {0x02, 0xB2, 1, 0xDB};
+	int ret;
+
+	if (ec->suspended)
+		return 0;
+
+	ret = gaokun_ec_write(ec, req);
+
+	if (ret)
+		return ret;
+
+	ec->suspended = true;
+
+	return 0;
+}
+
+static int gaokun_ec_resume(struct device *dev)
+{
+	struct gaokun_ec *ec = dev_get_drvdata(dev);
+	u8 req[REQ_HDR_SIZE + 1] = {0x02, 0xB2, 1, 0xEB};
+	int ret;
+	int i;
+
+	if (!ec->suspended)
+		return 0;
+
+	for (i = 0; i < 3; i++) {
+		ret = gaokun_ec_write(ec, req);
+		if (ret == 0)
+			break;
+
+		msleep(100);
+	};

Write three times with a 100 millisecond sleep ?

Deserves a comment at least.

ick ouch.

Please call these two functions and evaluate their result codes 
individually.

+
+	return sysfs_emit(buf, "%d %d\n", start, end);
+}
+
+static ssize_t charge_control_thresholds_store(struct device *dev,
+					       struct device_attribute *attr,
+					       const char *buf, size_t size)
+{
+	int ret;
+	u8 start, end;
+	struct gaokun_wmi *ecwmi = dev_get_drvdata(dev);
+
+	if (sscanf(buf, "%hhd %hhd", &start, &end) != 2)
+		return -EINVAL;
+
+	ret = gaokun_ec_wmi_set_threshold(ecwmi->ec, start, end);
+	if (ret)
+		return ret;
+
+	return size;
+}
+
+static DEVICE_ATTR_RW(charge_control_thresholds);
+
+/* -------------------------------------------------------------------------- */
+/* Smart charge param */
+
+static ssize_t smart_charge_param_show(struct device *dev,
+				       struct device_attribute *attr,
+				       char *buf)
+{
+	int ret;
+	u8 value;
+	struct gaokun_wmi *ecwmi = dev_get_drvdata(dev);
+
+	ret = gaokun_ec_wmi_get_smart_charge_param(ecwmi->ec, &value);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%d\n", value);
+}
+
+static ssize_t smart_charge_param_store(struct device *dev,
+					struct device_attribute *attr,
+					const char *buf, size_t size)
+{
+	int ret;
+	u8 value;
+	struct gaokun_wmi *ecwmi = dev_get_drvdata(dev);
+
+	if (kstrtou8(buf, 10, &value))
+		return -EINVAL;
+
+	ret = gaokun_ec_wmi_set_smart_charge_param(ecwmi->ec, value);
+	if (ret)
+		return ret;
+
+	return size;
+}
+
+static DEVICE_ATTR_RW(smart_charge_param);
+
+/* -------------------------------------------------------------------------- */
+/* Smart charge */
+
+static ssize_t smart_charge_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	int ret;
+	u8 bf[GAOKUN_SMART_CHARGE_DATA_SIZE];
+	struct gaokun_wmi *ecwmi = dev_get_drvdata(dev);
+
+	ret = gaokun_ec_wmi_get_smart_charge(ecwmi->ec, bf);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%d %d %d %d\n",
+			  bf[0], bf[1], bf[2], bf[3]);
+}
+
+static ssize_t smart_charge_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf, size_t size)
+{
+	int ret;
+	u8 bf[GAOKUN_SMART_CHARGE_DATA_SIZE];
+	struct gaokun_wmi *ecwmi = dev_get_drvdata(dev);
+
+	if (sscanf(buf, "%hhd %hhd %hhd %hhd", bf, bf + 1, bf + 2, bf + 3) != 4)
+		return -EINVAL;
+
+	ret = gaokun_ec_wmi_set_smart_charge(ecwmi->ec, bf);
+	if (ret)
+		return ret;
+
+	return size;
+}
+
+static DEVICE_ATTR_RW(smart_charge);
+
+/* -------------------------------------------------------------------------- */
+/* Fn lock */
+
+static ssize_t fn_lock_state_show(struct device *dev,
+				  struct device_attribute *attr,
+				  char *buf)
+{
+	int ret;
+	u8 on;
+	struct gaokun_wmi *ecwmi = dev_get_drvdata(dev);
+
+	ret = gaokun_ec_wmi_get_fn_lock(ecwmi->ec, &on);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%d\n", on);
+}
+
+static ssize_t fn_lock_state_store(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t size)
+{
+	int ret;
+	u8 on;
+	struct gaokun_wmi *ecwmi = dev_get_drvdata(dev);
+
+	if (kstrtou8(buf, 10, &on))
+		return -EINVAL;
+
+	ret = gaokun_ec_wmi_set_fn_lock(ecwmi->ec, on);

I mentioned already you should pass on as a bool and then decide here in 
the input function if "on" as passed is a reasonable boolean state.

+	if (ret)
+		return ret;
+
+	return size;
+}
+
+static DEVICE_ATTR_RW(fn_lock_state);
+
+/* -------------------------------------------------------------------------- */
+/* Thermal Zone */
+
+static ssize_t temperature_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+
+	int ret, len, i;
+	char *ptr = buf;
+	s16 value;
+	s16 temp[GAOKUN_TZ_REG_NUM];
+	struct gaokun_wmi *ecwmi = dev_get_drvdata(dev);
+
+	ret = gaokun_ec_wmi_get_temp(ecwmi->ec, temp);
+	if (ret)
+		return ret;
+
+	i = 0;
+	len = 0;
+	while (i < GAOKUN_TZ_REG_NUM) {
+		value = temp[i++];
+		if (value < 0) {
+			len += sprintf(ptr + len, "-");

Have you ever seen a negative temperature value in running silicon ?
Looks suspicious to me.