* Re: [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver
2024-12-27 17:13 ` [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver Pengyu Luo
@ 2024-12-27 18:21 ` Maya Matuszczyk
2024-12-28 5:42 ` Pengyu Luo
2024-12-28 9:58 ` Krzysztof Kozlowski
` (3 subsequent siblings)
4 siblings, 1 reply; 52+ messages in thread
From: Maya Matuszczyk @ 2024-12-27 18:21 UTC (permalink / raw)
To: Pengyu Luo
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Hans de Goede, Ilpo Järvinen,
Bryan O'Donoghue, Sebastian Reichel, Heikki Krogerus,
Greg Kroah-Hartman, devicetree, linux-kernel, linux-arm-msm,
platform-driver-x86, linux-pm, linux-usb, Dmitry Baryshkov,
Nikita Travkin
Good to see someone else doing EC drivers for arm64 laptops!
pt., 27 gru 2024 o 18:16 Pengyu Luo <mitltlatltl@gmail.com> napisał(a):
>
> 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).
>
> 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:
>
> - 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.
>
> Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
> ---
> drivers/platform/arm64/Kconfig | 19 +
> drivers/platform/arm64/Makefile | 2 +
> drivers/platform/arm64/huawei-gaokun-ec.c | 598 ++++++++++++++++++
> drivers/platform/arm64/huawei-gaokun-wmi.c | 283 +++++++++
> .../linux/platform_data/huawei-gaokun-ec.h | 90 +++
> 5 files changed, 992 insertions(+)
> create mode 100644 drivers/platform/arm64/huawei-gaokun-ec.c
> create mode 100644 drivers/platform/arm64/huawei-gaokun-wmi.c
> create mode 100644 include/linux/platform_data/huawei-gaokun-ec.h
>
> diff --git a/drivers/platform/arm64/Kconfig b/drivers/platform/arm64/Kconfig
> index f88395ea3..eb7fbacf0 100644
> --- a/drivers/platform/arm64/Kconfig
> +++ b/drivers/platform/arm64/Kconfig
> @@ -33,6 +33,25 @@ config EC_ACER_ASPIRE1
> laptop where this information is not properly exposed via the
> standard ACPI devices.
>
> +config EC_HUAWEI_GAOKUN
> + tristate "Huawei Matebook E Go (sc8280xp) Embedded Controller driver"
> + 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.
> +
> config EC_LENOVO_YOGA_C630
> tristate "Lenovo Yoga C630 Embedded Controller driver"
> depends on ARCH_QCOM || COMPILE_TEST
> diff --git a/drivers/platform/arm64/Makefile b/drivers/platform/arm64/Makefile
> index b2ae9114f..ed32ad6c0 100644
> --- a/drivers/platform/arm64/Makefile
> +++ b/drivers/platform/arm64/Makefile
> @@ -6,4 +6,6 @@
> #
>
> obj-$(CONFIG_EC_ACER_ASPIRE1) += acer-aspire1-ec.o
> +obj-$(CONFIG_EC_HUAWEI_GAOKUN) += huawei-gaokun-ec.o
> +obj-$(CONFIG_EC_HUAWEI_GAOKUN) += huawei-gaokun-wmi.o
> obj-$(CONFIG_EC_LENOVO_YOGA_C630) += lenovo-yoga-c630.o
> diff --git a/drivers/platform/arm64/huawei-gaokun-ec.c b/drivers/platform/arm64/huawei-gaokun-ec.c
> new file mode 100644
> index 000000000..c1c657f7b
> --- /dev/null
> +++ b/drivers/platform/arm64/huawei-gaokun-ec.c
> @@ -0,0 +1,598 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * huawei-gaokun-ec - An EC driver for HUAWEI Matebook E Go (sc8280xp)
> + *
> + * reference: drivers/platform/arm64/acer-aspire1-ec.c
> + * drivers/platform/arm64/lenovo-yoga-c630.c
> + *
> + * Copyright (C) 2024 Pengyu Luo <mitltlatltl@gmail.com>
> + */
> +
> +#include <linux/auxiliary_bus.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/notifier.h>
> +#include <linux/module.h>
> +#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 */
> +#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);
> + usleep_range(2000, 2500);
> +
> + 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, };
> +
> + 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 */
WMI is in ACPI, this laptop doesn't boot with ACPI so why are things
handled in WMI separate from rest of the driver?
> +
> +/* 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) {
> + 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;
> + 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)
> + 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;
> +
> + 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};
> +
> + 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;
> +
> + 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])
> +{
> + /* 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) {
> + 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);
> + }
> +
> + 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);
> + };
> +
> + ec->suspended = false;
> +
> + return 0;
> +}
> +
> +static void gaokun_aux_release(struct device *dev)
> +{
> + struct auxiliary_device *adev = to_auxiliary_dev(dev);
> +
> + kfree(adev);
> +}
> +
> +static void gaokun_aux_remove(void *data)
> +{
> + struct auxiliary_device *adev = data;
> +
> + auxiliary_device_delete(adev);
> + auxiliary_device_uninit(adev);
> +}
> +
> +static int gaokun_aux_init(struct device *parent, const char *name,
> + struct gaokun_ec *ec)
> +{
> + struct auxiliary_device *adev;
> + int ret;
> +
> + adev = kzalloc(sizeof(*adev), GFP_KERNEL);
> + if (!adev)
> + return -ENOMEM;
> +
> + adev->name = name;
> + adev->id = 0;
> + adev->dev.parent = parent;
> + adev->dev.release = gaokun_aux_release;
> + adev->dev.platform_data = ec;
> + /* Allow aux devices to access parent's DT nodes directly */
> + device_set_of_node_from_dev(&adev->dev, parent);
> +
> + ret = auxiliary_device_init(adev);
> + if (ret) {
> + kfree(adev);
> + return ret;
> + }
> +
> + ret = auxiliary_device_add(adev);
> + if (ret) {
> + auxiliary_device_uninit(adev);
> + return ret;
> + }
> +
> + return devm_add_action_or_reset(parent, gaokun_aux_remove, adev);
> +}
> +
> +/* -------------------------------------------------------------------------- */
> +/* EC */
> +
> +static irqreturn_t gaokun_ec_irq_handler(int irq, void *data)
> +{
> + struct gaokun_ec *ec = data;
> + u8 req[REQ_HDR_SIZE] = {EC_EVENT, EC_QUERY, 0};
> + u8 status, id;
> + int ret;
> +
> + ret = gaokun_ec_read_byte(ec, req, &id);
> + if (ret)
> + return IRQ_HANDLED;
The error should probably be logged instead of silently ignored
> +
> + switch (id) {
> + case 0x0: /* No event */
> + break;
> +
> + case EC_EVENT_LID:
> + gaokun_ec_psy_read_byte(ec, EC_LID_STATE, &status);
> + status = EC_LID_OPEN & status;
> + input_report_switch(ec->idev, SW_LID, !status);
> + input_sync(ec->idev);
> + break;
> +
> + default:
> + blocking_notifier_call_chain(&ec->notifier_list, id, ec);
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int gaokun_ec_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct gaokun_ec *ec;
> + int ret;
> +
> + ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL);
> + if (!ec)
> + return -ENOMEM;
> +
> + mutex_init(&ec->lock);
> + ec->client = client;
> + i2c_set_clientdata(client, ec);
> + BLOCKING_INIT_NOTIFIER_HEAD(&ec->notifier_list);
> +
> + /* Lid switch */
> + ec->idev = devm_input_allocate_device(dev);
> + if (!ec->idev)
> + return -ENOMEM;
> +
> + ec->idev->name = "LID";
> + ec->idev->phys = "gaokun-ec/input0";
> + input_set_capability(ec->idev, EV_SW, SW_LID);
> +
> + ret = input_register_device(ec->idev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to register input device\n");
> +
> + ret = gaokun_aux_init(dev, "psy", ec);
> + if (ret)
> + return ret;
> +
> + ret = gaokun_aux_init(dev, "wmi", ec);
> + if (ret)
> + return ret;
> +
> + ret = gaokun_aux_init(dev, "ucsi", ec);
> + if (ret)
> + return ret;
> +
> + ret = devm_request_threaded_irq(dev, client->irq, NULL,
> + gaokun_ec_irq_handler, IRQF_ONESHOT,
> + dev_name(dev), ec);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to request irq\n");
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id gaokun_ec_id[] = {
> + { "gaokun-ec", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, gaokun_ec_id);
> +
> +static const struct of_device_id gaokun_ec_of_match[] = {
> + { .compatible = "huawei,gaokun-ec", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, gaokun_ec_of_match);
> +
> +static const struct dev_pm_ops gaokun_ec_pm_ops = {
> + NOIRQ_SYSTEM_SLEEP_PM_OPS(gaokun_ec_suspend, gaokun_ec_resume)
> +};
> +
> +static struct i2c_driver gaokun_ec_driver = {
> + .driver = {
> + .name = "gaokun-ec",
> + .of_match_table = gaokun_ec_of_match,
> + .pm = &gaokun_ec_pm_ops,
> + },
> + .probe = gaokun_ec_probe,
> + .id_table = gaokun_ec_id,
> +};
> +module_i2c_driver(gaokun_ec_driver);
> +
> +MODULE_DESCRIPTION("HUAWEI Matebook E Go EC driver");
> +MODULE_AUTHOR("Pengyu Luo <mitltlatltl@gmail.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/platform/arm64/huawei-gaokun-wmi.c b/drivers/platform/arm64/huawei-gaokun-wmi.c
> new file mode 100644
> index 000000000..793cb1659
> --- /dev/null
> +++ b/drivers/platform/arm64/huawei-gaokun-wmi.c
> @@ -0,0 +1,283 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * huawei-gaokun-wmi - A WMI driver for HUAWEI Matebook E Go (sc8280xp)
Just because in ACPI it's done by WMI stuff doesn't mean the non-ACPI
driver has to reflect this
> + *
> + * reference: drivers/platform/x86/huawei-wmi.c
> + *
> + * Copyright (C) 2024 Pengyu Luo <mitltlatltl@gmail.com>
> + */
> +
> +#include <linux/auxiliary_bus.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/sysfs.h>
> +#include <linux/version.h>
> +
> +#include <linux/platform_data/huawei-gaokun-ec.h>
> +
> +struct gaokun_wmi {
> + struct gaokun_ec *ec;
> + struct device *dev;
> + struct platform_device *wmi;
> +};
> +
> +/* -------------------------------------------------------------------------- */
> +/* Battery charging threshold */
> +
> +enum gaokun_wmi_threshold_ind {
> + START = 1,
> + END = 2,
> +};
> +
> +static ssize_t charge_control_thresholds_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + int ret;
> + u8 start, end;
> + struct gaokun_wmi *ecwmi = dev_get_drvdata(dev);
> +
> + ret = gaokun_ec_wmi_get_threshold(ecwmi->ec, &start, START)
> + || gaokun_ec_wmi_get_threshold(ecwmi->ec, &end, END);
> + if (ret)
> + return ret;
> +
> + 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);
> + 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, "-");
> + value = -value;
> + }
> + len += sprintf(ptr + len, "%d.%d ", value / 10, value % 10);
> + }
> + len += sprintf(ptr + len, "\n");
> +
> + return len;
> +}
> +
> +static DEVICE_ATTR_RO(temperature);
> +
> +static struct attribute *gaokun_wmi_features_attrs[] = {
> + &dev_attr_charge_control_thresholds.attr,
> + &dev_attr_smart_charge_param.attr,
> + &dev_attr_smart_charge.attr,
> + &dev_attr_fn_lock_state.attr,
> + &dev_attr_temperature.attr,
> + NULL,
> +};
> +ATTRIBUTE_GROUPS(gaokun_wmi_features);
> +
> +static int gaokun_wmi_probe(struct auxiliary_device *adev,
> + const struct auxiliary_device_id *id)
> +{
> + struct gaokun_ec *ec = adev->dev.platform_data;
> + struct device *dev = &adev->dev;
> + struct gaokun_wmi *ecwmi;
> +
> + ecwmi = devm_kzalloc(&adev->dev, sizeof(*ecwmi), GFP_KERNEL);
> + if (!ecwmi)
> + return -ENOMEM;
> +
> + ecwmi->ec = ec;
> + ecwmi->dev = dev;
> +
> + auxiliary_set_drvdata(adev, ecwmi);
> +
> + /* make it under /sys/devices/platform, convenient for sysfs I/O,
> + * while adev is under
> + * /sys/devices/platform/soc@0/ac0000.geniqup/a9c000.i2c/i2c-15/15-0038/
> + */
> + ecwmi->wmi = platform_device_register_simple("gaokun-wmi", -1, NULL, 0);
> + if (IS_ERR(ecwmi->wmi))
> + return dev_err_probe(dev, PTR_ERR(ecwmi->wmi),
> + "Failed to register wmi platform device\n");
> +
> + platform_set_drvdata(ecwmi->wmi, ecwmi);
> +
> + return device_add_groups(&ecwmi->wmi->dev, gaokun_wmi_features_groups);
> +}
> +
> +static void gaokun_wmi_remove(struct auxiliary_device *adev)
> +{
> + struct gaokun_wmi *ecwmi = auxiliary_get_drvdata(adev);
> + struct platform_device *wmi = ecwmi->wmi;
> +
> + device_remove_groups(&wmi->dev, gaokun_wmi_features_groups);
> + platform_device_unregister(ecwmi->wmi);
> +}
> +
> +static const struct auxiliary_device_id gaokun_wmi_id_table[] = {
> + { .name = GAOKUN_MOD_NAME "." GAOKUN_DEV_WMI, },
> + {}
> +};
> +MODULE_DEVICE_TABLE(auxiliary, gaokun_wmi_id_table);
> +
> +static struct auxiliary_driver gaokun_wmi_driver = {
> + .name = GAOKUN_DEV_WMI,
> + .id_table = gaokun_wmi_id_table,
> + .probe = gaokun_wmi_probe,
> + .remove = gaokun_wmi_remove,
> +};
> +
> +module_auxiliary_driver(gaokun_wmi_driver);
> +
> +MODULE_DESCRIPTION("HUAWEI Matebook E Go WMI driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/platform_data/huawei-gaokun-ec.h b/include/linux/platform_data/huawei-gaokun-ec.h
> new file mode 100644
> index 000000000..a649e9ecf
> --- /dev/null
> +++ b/include/linux/platform_data/huawei-gaokun-ec.h
> @@ -0,0 +1,90 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Huawei Matebook E Go (sc8280xp) Embedded Controller
> + *
> + * Copyright (C) 2024 Pengyu Luo <mitltlatltl@gmail.com>
> + *
> + */
> +
> +#ifndef __HUAWEI_GAOKUN_EC_H__
> +#define __HUAWEI_GAOKUN_EC_H__
> +
> +#define GAOKUN_UCSI_CCI_SIZE 4
> +#define GAOKUN_UCSI_DATA_SIZE 16
> +#define GAOKUN_UCSI_READ_SIZE (GAOKUN_UCSI_CCI_SIZE + GAOKUN_UCSI_DATA_SIZE)
> +#define GAOKUN_UCSI_WRITE_SIZE 0x18
> +
> +#define GAOKUN_TZ_REG_NUM 20
> +#define GAOKUN_SMART_CHARGE_DATA_SIZE 4 /* mode, delay, start, end */
> +
> +/* -------------------------------------------------------------------------- */
> +
> +struct gaokun_ec;
> +struct notifier_block;
> +
> +#define GAOKUN_MOD_NAME "huawei_gaokun_ec"
> +#define GAOKUN_DEV_PSY "psy"
> +#define GAOKUN_DEV_WMI "wmi"
> +#define GAOKUN_DEV_UCSI "ucsi"
> +
> +/* -------------------------------------------------------------------------- */
> +/* Common API */
> +
> +int gaokun_ec_register_notify(struct gaokun_ec *ec,
> + struct notifier_block *nb);
> +void gaokun_ec_unregister_notify(struct gaokun_ec *ec,
> + struct notifier_block *nb);
> +
> +int gaokun_ec_read(struct gaokun_ec *ec, const u8 *req,
> + size_t resp_len, u8 *resp);
> +int gaokun_ec_write(struct gaokun_ec *ec, u8 *req);
> +int gaokun_ec_read_byte(struct gaokun_ec *ec, u8 *req, u8 *byte);
> +
> +/* -------------------------------------------------------------------------- */
> +/* API For PSY */
> +
> +int gaokun_ec_psy_multi_read(struct gaokun_ec *ec, u8 reg,
> + size_t resp_len, u8 *resp);
> +
> +static inline int gaokun_ec_psy_read_byte(struct gaokun_ec *ec,
> + u8 reg, u8 *byte)
> +{
> + return gaokun_ec_psy_multi_read(ec, reg, 1, byte);
> +}
> +
> +static inline int gaokun_ec_psy_read_word(struct gaokun_ec *ec,
> + u8 reg, u16 *word)
> +{
> + return gaokun_ec_psy_multi_read(ec, reg, 2, (u8 *)word);
> +}
> +
> +/* -------------------------------------------------------------------------- */
> +/* API For WMI */
> +
> +int gaokun_ec_wmi_get_threshold(struct gaokun_ec *ec, u8 *value, int ind);
> +int gaokun_ec_wmi_set_threshold(struct gaokun_ec *ec, u8 start, u8 end);
> +
> +int gaokun_ec_wmi_get_smart_charge_param(struct gaokun_ec *ec, u8 *value);
> +int gaokun_ec_wmi_set_smart_charge_param(struct gaokun_ec *ec, u8 value);
> +
> +int gaokun_ec_wmi_get_smart_charge(struct gaokun_ec *ec,
> + u8 data[GAOKUN_SMART_CHARGE_DATA_SIZE]);
> +int gaokun_ec_wmi_set_smart_charge(struct gaokun_ec *ec,
> + u8 data[GAOKUN_SMART_CHARGE_DATA_SIZE]);
> +
> +int gaokun_ec_wmi_get_fn_lock(struct gaokun_ec *ec, u8 *on);
> +int gaokun_ec_wmi_set_fn_lock(struct gaokun_ec *ec, u8 on);
> +
> +int gaokun_ec_wmi_get_temp(struct gaokun_ec *ec, s16 temp[GAOKUN_TZ_REG_NUM]);
> +
> +/* -------------------------------------------------------------------------- */
> +/* API For UCSI */
> +
> +int gaokun_ec_ucsi_read(struct gaokun_ec *ec, u8 resp[GAOKUN_UCSI_READ_SIZE]);
> +int gaokun_ec_ucsi_write(struct gaokun_ec *ec,
> + const u8 req[GAOKUN_UCSI_WRITE_SIZE]);
> +
> +int gaokun_ec_ucsi_get_reg(struct gaokun_ec *ec, u8 *ureg);
> +int gaokun_ec_ucsi_pan_ack(struct gaokun_ec *ec, int port_id);
> +
> +
> +#endif /* __HUAWEI_GAOKUN_EC_H__ */
> --
> 2.47.1
>
>
Best Regards,
Maya Matuszczyk
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver
2024-12-27 18:21 ` Maya Matuszczyk
@ 2024-12-28 5:42 ` Pengyu Luo
0 siblings, 0 replies; 52+ messages in thread
From: Pengyu Luo @ 2024-12-28 5:42 UTC (permalink / raw)
To: maccraft123mc
Cc: andersson, bryan.odonoghue, conor+dt, devicetree,
dmitry.baryshkov, gregkh, hdegoede, heikki.krogerus,
ilpo.jarvinen, konradybcio, krzk+dt, linux-arm-msm, linux-kernel,
linux-pm, linux-usb, mitltlatltl, nikita, platform-driver-x86,
robh, sre
On Sat, Dec 28, 2024 at 2:21 AM Maya Matuszczyk <maccraft123mc@gmail.com> wrote:
> Good to see someone else doing EC drivers for arm64 laptops!
>
Yeah, I have worked on it for a while. I really don't know why don't some
mechines use a pmic glink. AFAIK, there are some WOA devices without EC.
Mechines can definitely work without it in a way.
I am also glad that you are reviewing my code.
> pt., 27 gru 2024 o 18:16 Pengyu Luo <mitltlatltl@gmail.com> napisał(a):
> >
> > 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).
> >
> > 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:
> >
> > - 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.
> >
> > Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
> > ---
> > drivers/platform/arm64/Kconfig | 19 +
> > drivers/platform/arm64/Makefile | 2 +
> > drivers/platform/arm64/huawei-gaokun-ec.c | 598 ++++++++++++++++++
> > drivers/platform/arm64/huawei-gaokun-wmi.c | 283 +++++++++
> > .../linux/platform_data/huawei-gaokun-ec.h | 90 +++
> > 5 files changed, 992 insertions(+)
> > create mode 100644 drivers/platform/arm64/huawei-gaokun-ec.c
> > create mode 100644 drivers/platform/arm64/huawei-gaokun-wmi.c
> > create mode 100644 include/linux/platform_data/huawei-gaokun-ec.h
[...]
> > +/* -------------------------------------------------------------------------- */
> > +/* 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, };
> > +
> > + 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 */
> WMI is in ACPI, this laptop doesn't boot with ACPI so why are things
> handled in WMI separate from rest of the driver?
>
This driver reimplemented WMI functoins, and it perform a lot of
operations, to avoid naming it, just call it WMI, make life easier.
Once I considered keeping WMI things in this file, but it makes this file
bloated. Then I splitted every part into a module.
> > +
> > +/* 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) {
> > + 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;
> > + ret = gaokun_ec_write(ec, req);
> > + } else {
> > + return -EINVAL;
> > + }
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(gaokun_ec_wmi_set_threshold);
[...]
> > +/* -------------------------------------------------------------------------- */
> > +/* EC */
> > +
> > +static irqreturn_t gaokun_ec_irq_handler(int irq, void *data)
> > +{
> > + struct gaokun_ec *ec = data;
> > + u8 req[REQ_HDR_SIZE] = {EC_EVENT, EC_QUERY, 0};
> > + u8 status, id;
> > + int ret;
> > +
> > + ret = gaokun_ec_read_byte(ec, req, &id);
> > + if (ret)
> > + return IRQ_HANDLED;
> The error should probably be logged instead of silently ignored
>
Generally, one or two I/O errors don't crash anything, but actually if we
just do as ACPI methoda did, there should not be any error. It may be
necessary for debugging at the early stage of developemnt. If you suggest,
then we can add a report to the lower function (gaokun_ec_request) to check
every transaction. BTW, lenovo c630 also ignored them.
> > +
> > + switch (id) {
> > + case 0x0: /* No event */
> > + break;
> > +
> > + case EC_EVENT_LID:
> > + gaokun_ec_psy_read_byte(ec, EC_LID_STATE, &status);
> > + status = EC_LID_OPEN & status;
> > + input_report_switch(ec->idev, SW_LID, !status);
> > + input_sync(ec->idev);
> > + break;
> > +
> > + default:
> > + blocking_notifier_call_chain(&ec->notifier_list, id, ec);
> > + }
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
[...]
> > diff --git a/drivers/platform/arm64/huawei-gaokun-wmi.c b/drivers/platform/arm64/huawei-gaokun-wmi.c
> > new file mode 100644
> > index 000000000..793cb1659
> > --- /dev/null
> > +++ b/drivers/platform/arm64/huawei-gaokun-wmi.c
> > @@ -0,0 +1,283 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * huawei-gaokun-wmi - A WMI driver for HUAWEI Matebook E Go (sc8280xp)
> Just because in ACPI it's done by WMI stuff doesn't mean the non-ACPI
> driver has to reflect this
>
As I just said, and the WMI stuffs are all implemented by wrapping a EC
transaction in ACPI, at least in this machine.
> > + *
> > + * reference: drivers/platform/x86/huawei-wmi.c
> > + *
> > + * Copyright (C) 2024 Pengyu Luo <mitltlatltl@gmail.com>
> > + */
> > +
> > +#include <linux/auxiliary_bus.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/version.h>
> > +
> > +#include <linux/platform_data/huawei-gaokun-ec.h>
> > +
> >
[...]
> >
>
> Best Regards,
> Maya Matuszczyk
Best Wishes,
Pengyu
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver
2024-12-27 17:13 ` [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver Pengyu Luo
2024-12-27 18:21 ` Maya Matuszczyk
@ 2024-12-28 9:58 ` Krzysztof Kozlowski
2024-12-28 11:34 ` [PATCH 1/5] dt-bindings: platform: Add Huawei Matebook E Go EC Pengyu Luo
2024-12-28 12:33 ` [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver Bryan O'Donoghue
` (2 subsequent siblings)
4 siblings, 1 reply; 52+ messages in thread
From: Krzysztof Kozlowski @ 2024-12-28 9:58 UTC (permalink / raw)
To: Pengyu Luo, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Hans de Goede, Ilpo Järvinen,
Bryan O'Donoghue, Sebastian Reichel, Heikki Krogerus,
Greg Kroah-Hartman
Cc: devicetree, linux-kernel, linux-arm-msm, platform-driver-x86,
linux-pm, linux-usb, Dmitry Baryshkov, Nikita Travkin
On 27/12/2024 18:13, Pengyu Luo wrote:
> +
> +#include <linux/platform_data/huawei-gaokun-ec.h>
> +
> +#define EC_EVENT 0x06
> +
> +/* Also can be found in ACPI specification 12.3 */
> +#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
Use Linux style comments, see coding style.
> + * {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;
Missing doc. Run Checkpatch --strict, so you will know what is missing here.
> + struct blocking_notifier_head notifier_list;
> + struct input_dev *idev;
> + bool suspended;
> +};
> +
...
> +
> +static DEVICE_ATTR_RO(temperature);
> +
> +static struct attribute *gaokun_wmi_features_attrs[] = {
> + &dev_attr_charge_control_thresholds.attr,
> + &dev_attr_smart_charge_param.attr,
> + &dev_attr_smart_charge.attr,
> + &dev_attr_fn_lock_state.attr,
> + &dev_attr_temperature.attr,
> + NULL,
> +};
No, don't expose your own interface. Charging is already exposed by
power supply framework. Temperature by hwmon sensors. Drop all these and
never re-implement existing kernel user-space interfaces.
> diff --git a/include/linux/platform_data/huawei-gaokun-ec.h b/include/linux/platform_data/huawei-gaokun-ec.h
> new file mode 100644
> index 000000000..a649e9ecf
> --- /dev/null
> +++ b/include/linux/platform_data/huawei-gaokun-ec.h
> @@ -0,0 +1,90 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Huawei Matebook E Go (sc8280xp) Embedded Controller
> + *
> + * Copyright (C) 2024 Pengyu Luo <mitltlatltl@gmail.com>
> + *
> + */
> +
> +#ifndef __HUAWEI_GAOKUN_EC_H__
> +#define __HUAWEI_GAOKUN_EC_H__
> +
> +#define GAOKUN_UCSI_CCI_SIZE 4
> +#define GAOKUN_UCSI_DATA_SIZE 16
> +#define GAOKUN_UCSI_READ_SIZE (GAOKUN_UCSI_CCI_SIZE + GAOKUN_UCSI_DATA_SIZE)
> +#define GAOKUN_UCSI_WRITE_SIZE 0x18
> +
> +#define GAOKUN_TZ_REG_NUM 20
> +#define GAOKUN_SMART_CHARGE_DATA_SIZE 4 /* mode, delay, start, end */
> +
> +/* -------------------------------------------------------------------------- */
> +
> +struct gaokun_ec;
> +struct notifier_block;
Drop, include proper header instead.
> +
> +#define GAOKUN_MOD_NAME "huawei_gaokun_ec"
> +#define GAOKUN_DEV_PSY "psy"
> +#define GAOKUN_DEV_WMI "wmi"
> +#define GAOKUN_DEV_UCSI "ucsi"
> +
> +/* -------------------------------------------------------------------------- */
> +/* Common API */
> +
> +int gaokun_ec_register_notify(struct gaokun_ec *ec,
> + struct notifier_block *nb);
> +void gaokun_ec_unregister_notify(struct gaokun_ec *ec,
> + struct notifier_block *nb);
> +
> +int gaokun_ec_read(struct gaokun_ec *ec, const u8 *req,
> + size_t resp_len, u8 *resp);
> +int gaokun_ec_write(struct gaokun_ec *ec, u8 *req);
> +int gaokun_ec_read_byte(struct gaokun_ec *ec, u8 *req, u8 *byte);
You need kerneldoc, in the C file, for all exported functions.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH 1/5] dt-bindings: platform: Add Huawei Matebook E Go EC
2024-12-28 9:58 ` Krzysztof Kozlowski
@ 2024-12-28 11:34 ` Pengyu Luo
2024-12-29 4:08 ` Dmitry Baryshkov
2024-12-29 9:43 ` [PATCH 1/5] dt-bindings: platform: Add Huawei Matebook E Go EC Krzysztof Kozlowski
0 siblings, 2 replies; 52+ messages in thread
From: Pengyu Luo @ 2024-12-28 11:34 UTC (permalink / raw)
To: krzk
Cc: andersson, bryan.odonoghue, conor+dt, devicetree,
dmitry.baryshkov, gregkh, hdegoede, heikki.krogerus,
ilpo.jarvinen, konradybcio, krzk+dt, linux-arm-msm, linux-kernel,
linux-pm, linux-usb, mitltlatltl, nikita, platform-driver-x86,
robh, sre
> On Sat, Dec 28, 2024 at 5:58 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On 27/12/2024 18:13, Pengyu Luo wrote:
> > +
> > +#include <linux/platform_data/huawei-gaokun-ec.h>
> > +
> > +#define EC_EVENT 0x06
> > +
> > +/* Also can be found in ACPI specification 12.3 */
> > +#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
>
> Use Linux style comments, see coding style.
>
Agree
> > + * {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;
>
> Missing doc. Run Checkpatch --strict, so you will know what is missing here.
>
I see. A comment for mutex lock.
> > + struct blocking_notifier_head notifier_list;
> > + struct input_dev *idev;
> > + bool suspended;
> > +};
> > +
>
>
>
> ...
>
> > +
> > +static DEVICE_ATTR_RO(temperature);
> > +
> > +static struct attribute *gaokun_wmi_features_attrs[] = {
> > + &dev_attr_charge_control_thresholds.attr,
> > + &dev_attr_smart_charge_param.attr,
> > + &dev_attr_smart_charge.attr,
> > + &dev_attr_fn_lock_state.attr,
> > + &dev_attr_temperature.attr,
> > + NULL,
> > +};
>
>
> No, don't expose your own interface. Charging is already exposed by
> power supply framework. Temperature by hwmon sensors. Drop all these and
> never re-implement existing kernel user-space interfaces.
>
I don't quite understand what you mean. You mean I should use hwmon
interface like hwmon_device_register_with_groups to register it, right?
As for battery, get/set_propery allow us to handle charging thresholds
things, but there are smart_charge_param, smart_charge and fn_lock to handle.
>
> > diff --git a/include/linux/platform_data/huawei-gaokun-ec.h b/include/linux/platform_data/huawei-gaokun-ec.h
> > new file mode 100644
> > index 000000000..a649e9ecf
> > --- /dev/null
> > +++ b/include/linux/platform_data/huawei-gaokun-ec.h
> > @@ -0,0 +1,90 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Huawei Matebook E Go (sc8280xp) Embedded Controller
> > + *
> > + * Copyright (C) 2024 Pengyu Luo <mitltlatltl@gmail.com>
> > + *
> > + */
> > +
> > +#ifndef __HUAWEI_GAOKUN_EC_H__
> > +#define __HUAWEI_GAOKUN_EC_H__
> > +
> > +#define GAOKUN_UCSI_CCI_SIZE 4
> > +#define GAOKUN_UCSI_DATA_SIZE 16
> > +#define GAOKUN_UCSI_READ_SIZE (GAOKUN_UCSI_CCI_SIZE + GAOKUN_UCSI_DATA_SIZE)
> > +#define GAOKUN_UCSI_WRITE_SIZE 0x18
> > +
> > +#define GAOKUN_TZ_REG_NUM 20
> > +#define GAOKUN_SMART_CHARGE_DATA_SIZE 4 /* mode, delay, start, end */
> > +
> > +/* -------------------------------------------------------------------------- */
> > +
> > +struct gaokun_ec;
> > +struct notifier_block;
>
> Drop, include proper header instead.
>
I agree, I copy 'struct notifier_block;' from
include/linux/platform_data/lenovo-yoga-c630.h
> > +
> > +#define GAOKUN_MOD_NAME "huawei_gaokun_ec"
> > +#define GAOKUN_DEV_PSY "psy"
> > +#define GAOKUN_DEV_WMI "wmi"
> > +#define GAOKUN_DEV_UCSI "ucsi"
> > +
> > +/* -------------------------------------------------------------------------- */
> > +/* Common API */
> > +
> > +int gaokun_ec_register_notify(struct gaokun_ec *ec,
> > + struct notifier_block *nb);
> > +void gaokun_ec_unregister_notify(struct gaokun_ec *ec,
> > + struct notifier_block *nb);
> > +
> > +int gaokun_ec_read(struct gaokun_ec *ec, const u8 *req,
> > + size_t resp_len, u8 *resp);
> > +int gaokun_ec_write(struct gaokun_ec *ec, u8 *req);
> > +int gaokun_ec_read_byte(struct gaokun_ec *ec, u8 *req, u8 *byte);
>
>
> You need kerneldoc, in the C file, for all exported functions.
>
I got it.
>
>
> Best regards,
> Krzysztof
Best Wishes,
Pengyu
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH 1/5] dt-bindings: platform: Add Huawei Matebook E Go EC
2024-12-28 11:34 ` [PATCH 1/5] dt-bindings: platform: Add Huawei Matebook E Go EC Pengyu Luo
@ 2024-12-29 4:08 ` Dmitry Baryshkov
2024-12-29 9:04 ` [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver Pengyu Luo
2024-12-29 9:43 ` [PATCH 1/5] dt-bindings: platform: Add Huawei Matebook E Go EC Krzysztof Kozlowski
1 sibling, 1 reply; 52+ messages in thread
From: Dmitry Baryshkov @ 2024-12-29 4:08 UTC (permalink / raw)
To: Pengyu Luo
Cc: krzk, andersson, bryan.odonoghue, conor+dt, devicetree, gregkh,
hdegoede, heikki.krogerus, ilpo.jarvinen, konradybcio, krzk+dt,
linux-arm-msm, linux-kernel, linux-pm, linux-usb, nikita,
platform-driver-x86, robh, sre
On Sat, Dec 28, 2024 at 07:34:37PM +0800, Pengyu Luo wrote:
> > On Sat, Dec 28, 2024 at 5:58 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > On 27/12/2024 18:13, Pengyu Luo wrote:
> > > +
> > > +#include <linux/platform_data/huawei-gaokun-ec.h>
> > > +
> > > +#define EC_EVENT 0x06
> > > +
> > > +/* Also can be found in ACPI specification 12.3 */
> > > +#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
> >
> > Use Linux style comments, see coding style.
> >
>
> Agree
>
> > > + * {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;
> >
> > Missing doc. Run Checkpatch --strict, so you will know what is missing here.
> >
>
> I see. A comment for mutex lock.
>
> > > + struct blocking_notifier_head notifier_list;
> > > + struct input_dev *idev;
> > > + bool suspended;
> > > +};
> > > +
> >
> >
> >
> > ...
> >
> > > +
> > > +static DEVICE_ATTR_RO(temperature);
> > > +
> > > +static struct attribute *gaokun_wmi_features_attrs[] = {
> > > + &dev_attr_charge_control_thresholds.attr,
> > > + &dev_attr_smart_charge_param.attr,
> > > + &dev_attr_smart_charge.attr,
> > > + &dev_attr_fn_lock_state.attr,
> > > + &dev_attr_temperature.attr,
> > > + NULL,
> > > +};
> >
> >
> > No, don't expose your own interface. Charging is already exposed by
> > power supply framework. Temperature by hwmon sensors. Drop all these and
> > never re-implement existing kernel user-space interfaces.
> >
>
> I don't quite understand what you mean. You mean I should use hwmon
> interface like hwmon_device_register_with_groups to register it, right?
> As for battery, get/set_propery allow us to handle charging thresholds
> things, but there are smart_charge_param, smart_charge and fn_lock to handle.
Please push the smart_* to the PSY driver. At least it makes sense to
move those. I'm not sure about the fn_lock one. If you have a separate
EC-based input device, it should go to it. If not, let's keep it in the
base device.
>
> >
> > > diff --git a/include/linux/platform_data/huawei-gaokun-ec.h b/include/linux/platform_data/huawei-gaokun-ec.h
> > > new file mode 100644
> > > index 000000000..a649e9ecf
> > > --- /dev/null
> > > +++ b/include/linux/platform_data/huawei-gaokun-ec.h
> > > @@ -0,0 +1,90 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/* Huawei Matebook E Go (sc8280xp) Embedded Controller
> > > + *
> > > + * Copyright (C) 2024 Pengyu Luo <mitltlatltl@gmail.com>
> > > + *
> > > + */
> > > +
> > > +#ifndef __HUAWEI_GAOKUN_EC_H__
> > > +#define __HUAWEI_GAOKUN_EC_H__
> > > +
> > > +#define GAOKUN_UCSI_CCI_SIZE 4
> > > +#define GAOKUN_UCSI_DATA_SIZE 16
> > > +#define GAOKUN_UCSI_READ_SIZE (GAOKUN_UCSI_CCI_SIZE + GAOKUN_UCSI_DATA_SIZE)
> > > +#define GAOKUN_UCSI_WRITE_SIZE 0x18
> > > +
> > > +#define GAOKUN_TZ_REG_NUM 20
> > > +#define GAOKUN_SMART_CHARGE_DATA_SIZE 4 /* mode, delay, start, end */
> > > +
> > > +/* -------------------------------------------------------------------------- */
> > > +
> > > +struct gaokun_ec;
> > > +struct notifier_block;
> >
> > Drop, include proper header instead.
> >
>
> I agree, I copy 'struct notifier_block;' from
> include/linux/platform_data/lenovo-yoga-c630.h
Please don't pollute header files with extra dependencies. It's usually
better to just forware-declare the struct instead of adding unnecessary
include.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver
2024-12-29 4:08 ` Dmitry Baryshkov
@ 2024-12-29 9:04 ` Pengyu Luo
2024-12-29 9:44 ` Krzysztof Kozlowski
0 siblings, 1 reply; 52+ messages in thread
From: Pengyu Luo @ 2024-12-29 9:04 UTC (permalink / raw)
To: dmitry.baryshkov
Cc: andersson, bryan.odonoghue, conor+dt, devicetree, gregkh,
hdegoede, heikki.krogerus, ilpo.jarvinen, konradybcio, krzk+dt,
krzk, linux-arm-msm, linux-kernel, linux-pm, linux-usb,
mitltlatltl, nikita, platform-driver-x86, robh, sre
On Sun, Dec 29, 2024 at 12:08 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> On Sat, Dec 28, 2024 at 07:34:37PM +0800, Pengyu Luo wrote:
> > > On Sat, Dec 28, 2024 at 5:58 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > On 27/12/2024 18:13, Pengyu Luo wrote:
> > > > +
> > > > +#include <linux/platform_data/huawei-gaokun-ec.h>
> > > > +
> > > > +#define EC_EVENT 0x06
> > > > +
> > > > +/* Also can be found in ACPI specification 12.3 */
> > > > +#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
> > >
> > > Use Linux style comments, see coding style.
> > >
> >
> > Agree
> >
> > > > + * {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;
> > >
> > > Missing doc. Run Checkpatch --strict, so you will know what is missing here.
> > >
> >
> > I see. A comment for mutex lock.
> >
> > > > + struct blocking_notifier_head notifier_list;
> > > > + struct input_dev *idev;
> > > > + bool suspended;
> > > > +};
> > > > +
> > >
> > >
> > >
> > > ...
> > >
> > > > +
> > > > +static DEVICE_ATTR_RO(temperature);
> > > > +
> > > > +static struct attribute *gaokun_wmi_features_attrs[] = {
> > > > + &dev_attr_charge_control_thresholds.attr,
> > > > + &dev_attr_smart_charge_param.attr,
> > > > + &dev_attr_smart_charge.attr,
> > > > + &dev_attr_fn_lock_state.attr,
> > > > + &dev_attr_temperature.attr,
> > > > + NULL,
> > > > +};
> > >
> > >
> > > No, don't expose your own interface. Charging is already exposed by
> > > power supply framework. Temperature by hwmon sensors. Drop all these and
> > > never re-implement existing kernel user-space interfaces.
> > >
> >
> > I don't quite understand what you mean. You mean I should use hwmon
> > interface like hwmon_device_register_with_groups to register it, right?
> > As for battery, get/set_propery allow us to handle charging thresholds
> > things, but there are smart_charge_param, smart_charge and fn_lock to handle.
>
> Please push the smart_* to the PSY driver. At least it makes sense to
> move those. I'm not sure about the fn_lock one. If you have a separate
> EC-based input device, it should go to it. If not, let's keep it in the
> base device.
>
I see, so can I fix it in v2 like this?
- Using device_add_groups to register smart_* sysfs in PSY
- Using hwmon_device_register_with_groups to register thermal related sysfs in base driver
> >
> > >
> > > > diff --git a/include/linux/platform_data/huawei-gaokun-ec.h b/include/linux/platform_data/huawei-gaokun-ec.h
> > > > new file mode 100644
> > > > index 000000000..a649e9ecf
> > > > --- /dev/null
> > > > +++ b/include/linux/platform_data/huawei-gaokun-ec.h
> > > > @@ -0,0 +1,90 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > +/* Huawei Matebook E Go (sc8280xp) Embedded Controller
> > > > + *
> > > > + * Copyright (C) 2024 Pengyu Luo <mitltlatltl@gmail.com>
> > > > + *
> > > > + */
> > > > +
> > > > +#ifndef __HUAWEI_GAOKUN_EC_H__
> > > > +#define __HUAWEI_GAOKUN_EC_H__
> > > > +
> > > > +#define GAOKUN_UCSI_CCI_SIZE 4
> > > > +#define GAOKUN_UCSI_DATA_SIZE 16
> > > > +#define GAOKUN_UCSI_READ_SIZE (GAOKUN_UCSI_CCI_SIZE + GAOKUN_UCSI_DATA_SIZE)
> > > > +#define GAOKUN_UCSI_WRITE_SIZE 0x18
> > > > +
> > > > +#define GAOKUN_TZ_REG_NUM 20
> > > > +#define GAOKUN_SMART_CHARGE_DATA_SIZE 4 /* mode, delay, start, end */
> > > > +
> > > > +/* -------------------------------------------------------------------------- */
> > > > +
> > > > +struct gaokun_ec;
> > > > +struct notifier_block;
> > >
> > > Drop, include proper header instead.
> > >
> >
> > I agree, I copy 'struct notifier_block;' from
> > include/linux/platform_data/lenovo-yoga-c630.h
>
> Please don't pollute header files with extra dependencies. It's usually
> better to just forware-declare the struct instead of adding unnecessary
> include.
>
Both of you are resonable. So how?
BTW, Krzysztof said
> > You need kerneldoc, in the C file, for all exported functions.
So Dmitry is here, I want to check again, should I add kerneldoc for all
exported functions? C630 one never added all kerneldocs. In my driver,
some function names have already indicated many things, some complex
functions have been doced.
Best wishes,
Pengyu
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver
2024-12-29 9:04 ` [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver Pengyu Luo
@ 2024-12-29 9:44 ` Krzysztof Kozlowski
0 siblings, 0 replies; 52+ messages in thread
From: Krzysztof Kozlowski @ 2024-12-29 9:44 UTC (permalink / raw)
To: Pengyu Luo, dmitry.baryshkov
Cc: andersson, bryan.odonoghue, conor+dt, devicetree, gregkh,
hdegoede, heikki.krogerus, ilpo.jarvinen, konradybcio, krzk+dt,
linux-arm-msm, linux-kernel, linux-pm, linux-usb, nikita,
platform-driver-x86, robh, sre
On 29/12/2024 10:04, Pengyu Luo wrote:
>>>>> +
>>>>> +struct gaokun_ec;
>>>>> +struct notifier_block;
>>>>
>>>> Drop, include proper header instead.
>>>>
>>>
>>> I agree, I copy 'struct notifier_block;' from
>>> include/linux/platform_data/lenovo-yoga-c630.h
>>
>> Please don't pollute header files with extra dependencies. It's usually
>> better to just forware-declare the struct instead of adding unnecessary
>> include.
>>
>
> Both of you are resonable. So how?
>
> BTW, Krzysztof said
I would recommend differently, but I am fine with other approach if
other reviewers have their preference.
>
>>> You need kerneldoc, in the C file, for all exported functions.
>
> So Dmitry is here, I want to check again, should I add kerneldoc for all
> exported functions? C630 one never added all kerneldocs. In my driver,
> some function names have already indicated many things, some complex
> functions have been doced.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 1/5] dt-bindings: platform: Add Huawei Matebook E Go EC
2024-12-28 11:34 ` [PATCH 1/5] dt-bindings: platform: Add Huawei Matebook E Go EC Pengyu Luo
2024-12-29 4:08 ` Dmitry Baryshkov
@ 2024-12-29 9:43 ` Krzysztof Kozlowski
2024-12-29 10:28 ` Pengyu Luo
1 sibling, 1 reply; 52+ messages in thread
From: Krzysztof Kozlowski @ 2024-12-29 9:43 UTC (permalink / raw)
To: Pengyu Luo
Cc: andersson, bryan.odonoghue, conor+dt, devicetree,
dmitry.baryshkov, gregkh, hdegoede, heikki.krogerus,
ilpo.jarvinen, konradybcio, krzk+dt, linux-arm-msm, linux-kernel,
linux-pm, linux-usb, nikita, platform-driver-x86, robh, sre
On 28/12/2024 12:34, Pengyu Luo wrote:
>> On Sat, Dec 28, 2024 at 5:58 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> On 27/12/2024 18:13, Pengyu Luo wrote:
>>> +
>>> +#include <linux/platform_data/huawei-gaokun-ec.h>
>>> +
>>> +#define EC_EVENT 0x06
>>> +
>>> +/* Also can be found in ACPI specification 12.3 */
>>> +#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
>>
>> Use Linux style comments, see coding style.
>>
>
> Agree
>
>>> + * {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;
>>
>> Missing doc. Run Checkpatch --strict, so you will know what is missing here.
>>
>
> I see. A comment for mutex lock.
>
>>> + struct blocking_notifier_head notifier_list;
>>> + struct input_dev *idev;
>>> + bool suspended;
>>> +};
>>> +
>>
>>
>>
>> ...
>>
>>> +
>>> +static DEVICE_ATTR_RO(temperature);
>>> +
>>> +static struct attribute *gaokun_wmi_features_attrs[] = {
>>> + &dev_attr_charge_control_thresholds.attr,
>>> + &dev_attr_smart_charge_param.attr,
>>> + &dev_attr_smart_charge.attr,
>>> + &dev_attr_fn_lock_state.attr,
>>> + &dev_attr_temperature.attr,
>>> + NULL,
>>> +};
>>
>>
>> No, don't expose your own interface. Charging is already exposed by
>> power supply framework. Temperature by hwmon sensors. Drop all these and
>> never re-implement existing kernel user-space interfaces.
>>
>
> I don't quite understand what you mean. You mean I should use hwmon
> interface like hwmon_device_register_with_groups to register it, right?
You added sysfs interface, I think. My comment is: do not. We have
existing interfaces.
> As for battery, get/set_propery allow us to handle charging thresholds
> things, but there are smart_charge_param, smart_charge and fn_lock to handle.
So where is the ABI documentation? Where is any explanation why existing
interfaces are not enough?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH 1/5] dt-bindings: platform: Add Huawei Matebook E Go EC
2024-12-29 9:43 ` [PATCH 1/5] dt-bindings: platform: Add Huawei Matebook E Go EC Krzysztof Kozlowski
@ 2024-12-29 10:28 ` Pengyu Luo
2024-12-29 21:45 ` Krzysztof Kozlowski
0 siblings, 1 reply; 52+ messages in thread
From: Pengyu Luo @ 2024-12-29 10:28 UTC (permalink / raw)
To: krzk
Cc: andersson, bryan.odonoghue, conor+dt, devicetree,
dmitry.baryshkov, gregkh, hdegoede, heikki.krogerus,
ilpo.jarvinen, konradybcio, krzk+dt, linux-arm-msm, linux-kernel,
linux-pm, linux-usb, mitltlatltl, nikita, platform-driver-x86,
robh, sre
On Sun, Dec 29, 2024 at 5:43 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On 28/12/2024 12:34, Pengyu Luo wrote:
> >> On Sat, Dec 28, 2024 at 5:58 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >> On 27/12/2024 18:13, Pengyu Luo wrote:
> >>> +
> >>> +#include <linux/platform_data/huawei-gaokun-ec.h>
> >>> +
> >>> +#define EC_EVENT 0x06
> >>> +
> >>> +/* Also can be found in ACPI specification 12.3 */
> >>> +#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
> >>
> >> Use Linux style comments, see coding style.
> >>
> >
> > Agree
> >
> >>> + * {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;
> >>
> >> Missing doc. Run Checkpatch --strict, so you will know what is missing here.
> >>
> >
> > I see. A comment for mutex lock.
> >
> >>> + struct blocking_notifier_head notifier_list;
> >>> + struct input_dev *idev;
> >>> + bool suspended;
> >>> +};
> >>> +
> >>
> >>
> >>
> >> ...
> >>
> >>> +
> >>> +static DEVICE_ATTR_RO(temperature);
> >>> +
> >>> +static struct attribute *gaokun_wmi_features_attrs[] = {
> >>> + &dev_attr_charge_control_thresholds.attr,
> >>> + &dev_attr_smart_charge_param.attr,
> >>> + &dev_attr_smart_charge.attr,
> >>> + &dev_attr_fn_lock_state.attr,
> >>> + &dev_attr_temperature.attr,
> >>> + NULL,
> >>> +};
> >>
> >>
> >> No, don't expose your own interface. Charging is already exposed by
> >> power supply framework. Temperature by hwmon sensors. Drop all these and
> >> never re-implement existing kernel user-space interfaces.
> >>
> >
> > I don't quite understand what you mean. You mean I should use hwmon
> > interface like hwmon_device_register_with_groups to register it, right?
>
> You added sysfs interface, I think. My comment is: do not. We have
> existing interfaces.
>
I agree with you, but device_add_groups is used to add sysfs interface
everywhere, device_add_groups are wrapped in acpi_battery_hook, they
handle charge_control_thresholds like this, since qcom arm64 do not
support acpi on linux, we do not use acpi_battery_hook to implement it,
so it is reasonable to implement it in PSY drivers.
some examples:
drivers/platform/x86/thinkpad_acpi.c
> static struct attribute *tpacpi_battery_attrs[] = {
> &dev_attr_charge_control_start_threshold.attr,
> &dev_attr_charge_control_end_threshold.attr,
> &dev_attr_charge_start_threshold.attr,
> &dev_attr_charge_stop_threshold.attr,
> &dev_attr_charge_behaviour.attr,
> NULL,
> };
>
> ATTRIBUTE_GROUPS(tpacpi_battery);
>
> /* ACPI battery hooking */
>
> static int tpacpi_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
> {
> int batteryid = tpacpi_battery_get_id(battery->desc->name);
>
> if (tpacpi_battery_probe(batteryid))
> return -ENODEV;
> if (device_add_groups(&battery->dev, tpacpi_battery_groups))
> return -ENODEV;
> return 0;
> }
drivers/platform/x86/dell/dell-laptop.c
> static struct attribute *dell_battery_attrs[] = {
> &dev_attr_charge_control_start_threshold.attr,
> &dev_attr_charge_control_end_threshold.attr,
> &dev_attr_charge_types.attr,
> NULL,
> };
> ATTRIBUTE_GROUPS(dell_battery);
>
> static bool dell_battery_supported(struct power_supply *battery)
> {
> /* We currently only support the primary battery */
> return strcmp(battery->desc->name, "BAT0") == 0;
> }
>
> static int dell_battery_add(struct power_supply *battery,
> struct acpi_battery_hook *hook)
> {
> /* Return 0 instead of an error to avoid being unloaded */
> if (!dell_battery_supported(battery))
> return 0;
>
> return device_add_groups(&battery->dev, dell_battery_groups);
> }
> > As for battery, get/set_propery allow us to handle charging thresholds
> > things, but there are smart_charge_param, smart_charge and fn_lock to handle.
>
> So where is the ABI documentation? Where is any explanation why existing
> interfaces are not enough?
>
OK, if you insist, I will explain it in v2.
Best wishes,
Pengyu
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH 1/5] dt-bindings: platform: Add Huawei Matebook E Go EC
2024-12-29 10:28 ` Pengyu Luo
@ 2024-12-29 21:45 ` Krzysztof Kozlowski
0 siblings, 0 replies; 52+ messages in thread
From: Krzysztof Kozlowski @ 2024-12-29 21:45 UTC (permalink / raw)
To: Pengyu Luo
Cc: andersson, bryan.odonoghue, conor+dt, devicetree,
dmitry.baryshkov, gregkh, hdegoede, heikki.krogerus,
ilpo.jarvinen, konradybcio, krzk+dt, linux-arm-msm, linux-kernel,
linux-pm, linux-usb, nikita, platform-driver-x86, robh, sre
On 29/12/2024 11:28, Pengyu Luo wrote:
> On Sun, Dec 29, 2024 at 5:43 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> On 28/12/2024 12:34, Pengyu Luo wrote:
>>>> On Sat, Dec 28, 2024 at 5:58 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>> On 27/12/2024 18:13, Pengyu Luo wrote:
>>>>> +
>>>>> +#include <linux/platform_data/huawei-gaokun-ec.h>
>>>>> +
>>>>> +#define EC_EVENT 0x06
>>>>> +
>>>>> +/* Also can be found in ACPI specification 12.3 */
>>>>> +#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
>>>>
>>>> Use Linux style comments, see coding style.
>>>>
>>>
>>> Agree
>>>
>>>>> + * {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;
>>>>
>>>> Missing doc. Run Checkpatch --strict, so you will know what is missing here.
>>>>
>>>
>>> I see. A comment for mutex lock.
>>>
>>>>> + struct blocking_notifier_head notifier_list;
>>>>> + struct input_dev *idev;
>>>>> + bool suspended;
>>>>> +};
>>>>> +
>>>>
>>>>
>>>>
>>>> ...
>>>>
>>>>> +
>>>>> +static DEVICE_ATTR_RO(temperature);
>>>>> +
>>>>> +static struct attribute *gaokun_wmi_features_attrs[] = {
>>>>> + &dev_attr_charge_control_thresholds.attr,
>>>>> + &dev_attr_smart_charge_param.attr,
>>>>> + &dev_attr_smart_charge.attr,
>>>>> + &dev_attr_fn_lock_state.attr,
>>>>> + &dev_attr_temperature.attr,
>>>>> + NULL,
>>>>> +};
>>>>
>>>>
>>>> No, don't expose your own interface. Charging is already exposed by
>>>> power supply framework. Temperature by hwmon sensors. Drop all these and
>>>> never re-implement existing kernel user-space interfaces.
>>>>
>>>
>>> I don't quite understand what you mean. You mean I should use hwmon
>>> interface like hwmon_device_register_with_groups to register it, right?
>>
>> You added sysfs interface, I think. My comment is: do not. We have
>> existing interfaces.
>>
>
> I agree with you, but device_add_groups is used to add sysfs interface
> everywhere, device_add_groups are wrapped in acpi_battery_hook, they
> handle charge_control_thresholds like this, since qcom arm64 do not
> support acpi on linux, we do not use acpi_battery_hook to implement it,
> so it is reasonable to implement it in PSY drivers.
OK, then do not make it a platform driver sysfs ABI but power supply one
and document the ABI (see Documentation/ABI/)
>
> some examples:
>
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver
2024-12-27 17:13 ` [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver Pengyu Luo
2024-12-27 18:21 ` Maya Matuszczyk
2024-12-28 9:58 ` Krzysztof Kozlowski
@ 2024-12-28 12:33 ` Bryan O'Donoghue
2024-12-28 13:51 ` Pengyu Luo
2024-12-29 14:49 ` Markus Elfring
2024-12-30 9:04 ` Aiqun(Maria) Yu
4 siblings, 1 reply; 52+ messages in thread
From: Bryan O'Donoghue @ 2024-12-28 12:33 UTC (permalink / raw)
To: Pengyu Luo, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Hans de Goede, Ilpo Järvinen,
Sebastian Reichel, Heikki Krogerus, Greg Kroah-Hartman
Cc: devicetree, linux-kernel, linux-arm-msm, platform-driver-x86,
linux-pm, linux-usb, Dmitry Baryshkov, Nikita Travkin
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"
>
> Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
> ---
> drivers/platform/arm64/Kconfig | 19 +
> drivers/platform/arm64/Makefile | 2 +
> drivers/platform/arm64/huawei-gaokun-ec.c | 598 ++++++++++++++++++
> drivers/platform/arm64/huawei-gaokun-wmi.c | 283 +++++++++
> .../linux/platform_data/huawei-gaokun-ec.h | 90 +++
> 5 files changed, 992 insertions(+)
> create mode 100644 drivers/platform/arm64/huawei-gaokun-ec.c
> create mode 100644 drivers/platform/arm64/huawei-gaokun-wmi.c
> create mode 100644 include/linux/platform_data/huawei-gaokun-ec.h
>
> diff --git a/drivers/platform/arm64/Kconfig b/drivers/platform/arm64/Kconfig
> index f88395ea3..eb7fbacf0 100644
> --- a/drivers/platform/arm64/Kconfig
> +++ b/drivers/platform/arm64/Kconfig
> @@ -33,6 +33,25 @@ config EC_ACER_ASPIRE1
> laptop where this information is not properly exposed via the
> standard ACPI devices.
>
> +config EC_HUAWEI_GAOKUN
> + tristate "Huawei Matebook E Go (sc8280xp) Embedded Controller driver"
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.
> +
> config EC_LENOVO_YOGA_C630
> tristate "Lenovo Yoga C630 Embedded Controller driver"
> depends on ARCH_QCOM || COMPILE_TEST
> diff --git a/drivers/platform/arm64/Makefile b/drivers/platform/arm64/Makefile
> index b2ae9114f..ed32ad6c0 100644
> --- a/drivers/platform/arm64/Makefile
> +++ b/drivers/platform/arm64/Makefile
> @@ -6,4 +6,6 @@
> #
>
> obj-$(CONFIG_EC_ACER_ASPIRE1) += acer-aspire1-ec.o
> +obj-$(CONFIG_EC_HUAWEI_GAOKUN) += huawei-gaokun-ec.o
> +obj-$(CONFIG_EC_HUAWEI_GAOKUN) += huawei-gaokun-wmi.o
> obj-$(CONFIG_EC_LENOVO_YOGA_C630) += lenovo-yoga-c630.o
> diff --git a/drivers/platform/arm64/huawei-gaokun-ec.c b/drivers/platform/arm64/huawei-gaokun-ec.c
> new file mode 100644
> index 000000000..c1c657f7b
> --- /dev/null
> +++ b/drivers/platform/arm64/huawei-gaokun-ec.c
> @@ -0,0 +1,598 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * huawei-gaokun-ec - An EC driver for HUAWEI Matebook E Go (sc8280xp)
> + *
> + * reference: drivers/platform/arm64/acer-aspire1-ec.c
> + * drivers/platform/arm64/lenovo-yoga-c630.c
> + *
> + * Copyright (C) 2024 Pengyu Luo <mitltlatltl@gmail.com>
> + */
> +
> +#include <linux/auxiliary_bus.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/notifier.h>
> +#include <linux/module.h>
> +#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 */
> +#define EC_READ 0x80
> +#define EC_WRITE 0x81
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.
> +
> + ec->suspended = false;
> +
> + return 0;
> +}
> +
> +static void gaokun_aux_release(struct device *dev)
> +{
> + struct auxiliary_device *adev = to_auxiliary_dev(dev);
> +
> + kfree(adev);
> +}
> +
> +static void gaokun_aux_remove(void *data)
> +{
> + struct auxiliary_device *adev = data;
> +
> + auxiliary_device_delete(adev);
> + auxiliary_device_uninit(adev);
> +}
> +
> +static int gaokun_aux_init(struct device *parent, const char *name,
> + struct gaokun_ec *ec)
> +{
> + struct auxiliary_device *adev;
> + int ret;
> +
> + adev = kzalloc(sizeof(*adev), GFP_KERNEL);
> + if (!adev)
> + return -ENOMEM;
> +
> + adev->name = name;
> + adev->id = 0;
> + adev->dev.parent = parent;
> + adev->dev.release = gaokun_aux_release;
> + adev->dev.platform_data = ec;
> + /* Allow aux devices to access parent's DT nodes directly */
> + device_set_of_node_from_dev(&adev->dev, parent);
> +
> + ret = auxiliary_device_init(adev);
> + if (ret) {
> + kfree(adev);
> + return ret;
> + }
> +
> + ret = auxiliary_device_add(adev);
> + if (ret) {
> + auxiliary_device_uninit(adev);
> + return ret;
> + }
> +
> + return devm_add_action_or_reset(parent, gaokun_aux_remove, adev);
> +}
> +
> +/* -------------------------------------------------------------------------- */
> +/* EC */
> +
> +static irqreturn_t gaokun_ec_irq_handler(int irq, void *data)
> +{
> + struct gaokun_ec *ec = data;
> + u8 req[REQ_HDR_SIZE] = {EC_EVENT, EC_QUERY, 0};
> + u8 status, id;
> + int ret;
> +
> + ret = gaokun_ec_read_byte(ec, req, &id);
> + if (ret)
> + return IRQ_HANDLED;
> +
> + switch (id) {
> + case 0x0: /* No event */
> + break;
> +
> + case EC_EVENT_LID:
> + gaokun_ec_psy_read_byte(ec, EC_LID_STATE, &status);
> + status = EC_LID_OPEN & status;
> + input_report_switch(ec->idev, SW_LID, !status);
> + input_sync(ec->idev);
> + break;
> +
> + default:
> + blocking_notifier_call_chain(&ec->notifier_list, id, ec);
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int gaokun_ec_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct gaokun_ec *ec;
> + int ret;
> +
> + ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL);
> + if (!ec)
> + return -ENOMEM;
> +
> + mutex_init(&ec->lock);
> + ec->client = client;
> + i2c_set_clientdata(client, ec);
> + BLOCKING_INIT_NOTIFIER_HEAD(&ec->notifier_list);
> +
> + /* Lid switch */
> + ec->idev = devm_input_allocate_device(dev);
> + if (!ec->idev)
> + return -ENOMEM;
> +
> + ec->idev->name = "LID";
> + ec->idev->phys = "gaokun-ec/input0";
> + input_set_capability(ec->idev, EV_SW, SW_LID);
> +
> + ret = input_register_device(ec->idev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to register input device\n");
> +
> + ret = gaokun_aux_init(dev, "psy", ec);
> + if (ret)
> + return ret;
> +
> + ret = gaokun_aux_init(dev, "wmi", ec);
> + if (ret)
> + return ret;
> +
> + ret = gaokun_aux_init(dev, "ucsi", ec);
> + if (ret)
> + return ret;
> +
> + ret = devm_request_threaded_irq(dev, client->irq, NULL,
> + gaokun_ec_irq_handler, IRQF_ONESHOT,
> + dev_name(dev), ec);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to request irq\n");
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id gaokun_ec_id[] = {
> + { "gaokun-ec", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, gaokun_ec_id);
> +
> +static const struct of_device_id gaokun_ec_of_match[] = {
> + { .compatible = "huawei,gaokun-ec", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, gaokun_ec_of_match);
> +
> +static const struct dev_pm_ops gaokun_ec_pm_ops = {
> + NOIRQ_SYSTEM_SLEEP_PM_OPS(gaokun_ec_suspend, gaokun_ec_resume)
> +};
> +
> +static struct i2c_driver gaokun_ec_driver = {
> + .driver = {
> + .name = "gaokun-ec",
> + .of_match_table = gaokun_ec_of_match,
> + .pm = &gaokun_ec_pm_ops,
> + },
> + .probe = gaokun_ec_probe,
> + .id_table = gaokun_ec_id,
> +};
> +module_i2c_driver(gaokun_ec_driver);
> +
> +MODULE_DESCRIPTION("HUAWEI Matebook E Go EC driver");
> +MODULE_AUTHOR("Pengyu Luo <mitltlatltl@gmail.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/platform/arm64/huawei-gaokun-wmi.c b/drivers/platform/arm64/huawei-gaokun-wmi.c
> new file mode 100644
> index 000000000..793cb1659
> --- /dev/null
> +++ b/drivers/platform/arm64/huawei-gaokun-wmi.c
> @@ -0,0 +1,283 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * huawei-gaokun-wmi - A WMI driver for HUAWEI Matebook E Go (sc8280xp)
> + *
> + * reference: drivers/platform/x86/huawei-wmi.c
> + *
> + * Copyright (C) 2024 Pengyu Luo <mitltlatltl@gmail.com>
> + */
> +
> +#include <linux/auxiliary_bus.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/sysfs.h>
> +#include <linux/version.h>
> +
> +#include <linux/platform_data/huawei-gaokun-ec.h>
> +
> +struct gaokun_wmi {
> + struct gaokun_ec *ec;
> + struct device *dev;
> + struct platform_device *wmi;
> +};
> +
> +/* -------------------------------------------------------------------------- */
> +/* Battery charging threshold */
> +
> +enum gaokun_wmi_threshold_ind {
> + START = 1,
> + END = 2,
> +};
> +
> +static ssize_t charge_control_thresholds_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + int ret;
> + u8 start, end;
> + struct gaokun_wmi *ecwmi = dev_get_drvdata(dev);
> +
> + ret = gaokun_ec_wmi_get_threshold(ecwmi->ec, &start, START)
> + || gaokun_ec_wmi_get_threshold(ecwmi->ec, &end, END);
> + if (ret)
> + return ret;
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.
> + value = -value;
> + }
> + len += sprintf(ptr + len, "%d.%d ", value / 10, value % 10);
> + }
> + len += sprintf(ptr + len, "\n");
> +
> + return len;
> +}
> +
> +static DEVICE_ATTR_RO(temperature);
> +
> +static struct attribute *gaokun_wmi_features_attrs[] = {
> + &dev_attr_charge_control_thresholds.attr,
> + &dev_attr_smart_charge_param.attr,
> + &dev_attr_smart_charge.attr,
> + &dev_attr_fn_lock_state.attr,
> + &dev_attr_temperature.attr,
> + NULL,
> +};
> +ATTRIBUTE_GROUPS(gaokun_wmi_features);
> +
> +static int gaokun_wmi_probe(struct auxiliary_device *adev,
> + const struct auxiliary_device_id *id)
> +{
> + struct gaokun_ec *ec = adev->dev.platform_data;
> + struct device *dev = &adev->dev;
> + struct gaokun_wmi *ecwmi;
> +
> + ecwmi = devm_kzalloc(&adev->dev, sizeof(*ecwmi), GFP_KERNEL);
> + if (!ecwmi)
> + return -ENOMEM;
> +
> + ecwmi->ec = ec;
> + ecwmi->dev = dev;
> +
> + auxiliary_set_drvdata(adev, ecwmi);
> +
> + /* make it under /sys/devices/platform, convenient for sysfs I/O,
> + * while adev is under
> + * /sys/devices/platform/soc@0/ac0000.geniqup/a9c000.i2c/i2c-15/15-0038/
> + */
> + ecwmi->wmi = platform_device_register_simple("gaokun-wmi", -1, NULL, 0);
> + if (IS_ERR(ecwmi->wmi))
> + return dev_err_probe(dev, PTR_ERR(ecwmi->wmi),
> + "Failed to register wmi platform device\n");
> +
> + platform_set_drvdata(ecwmi->wmi, ecwmi);
> +
> + return device_add_groups(&ecwmi->wmi->dev, gaokun_wmi_features_groups);
> +}
> +
> +static void gaokun_wmi_remove(struct auxiliary_device *adev)
> +{
> + struct gaokun_wmi *ecwmi = auxiliary_get_drvdata(adev);
> + struct platform_device *wmi = ecwmi->wmi;
> +
> + device_remove_groups(&wmi->dev, gaokun_wmi_features_groups);
> + platform_device_unregister(ecwmi->wmi);
> +}
> +
> +static const struct auxiliary_device_id gaokun_wmi_id_table[] = {
> + { .name = GAOKUN_MOD_NAME "." GAOKUN_DEV_WMI, },
> + {}
> +};
> +MODULE_DEVICE_TABLE(auxiliary, gaokun_wmi_id_table);
> +
> +static struct auxiliary_driver gaokun_wmi_driver = {
> + .name = GAOKUN_DEV_WMI,
> + .id_table = gaokun_wmi_id_table,
> + .probe = gaokun_wmi_probe,
> + .remove = gaokun_wmi_remove,
> +};
> +
> +module_auxiliary_driver(gaokun_wmi_driver);
> +
> +MODULE_DESCRIPTION("HUAWEI Matebook E Go WMI driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/platform_data/huawei-gaokun-ec.h b/include/linux/platform_data/huawei-gaokun-ec.h
> new file mode 100644
> index 000000000..a649e9ecf
> --- /dev/null
> +++ b/include/linux/platform_data/huawei-gaokun-ec.h
> @@ -0,0 +1,90 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Huawei Matebook E Go (sc8280xp) Embedded Controller
> + *
> + * Copyright (C) 2024 Pengyu Luo <mitltlatltl@gmail.com>
> + *
> + */
> +
> +#ifndef __HUAWEI_GAOKUN_EC_H__
> +#define __HUAWEI_GAOKUN_EC_H__
> +
> +#define GAOKUN_UCSI_CCI_SIZE 4
> +#define GAOKUN_UCSI_DATA_SIZE 16
> +#define GAOKUN_UCSI_READ_SIZE (GAOKUN_UCSI_CCI_SIZE + GAOKUN_UCSI_DATA_SIZE)
> +#define GAOKUN_UCSI_WRITE_SIZE 0x18
> +
> +#define GAOKUN_TZ_REG_NUM 20
> +#define GAOKUN_SMART_CHARGE_DATA_SIZE 4 /* mode, delay, start, end */
> +
> +/* -------------------------------------------------------------------------- */
> +
> +struct gaokun_ec;
> +struct notifier_block;
> +
> +#define GAOKUN_MOD_NAME "huawei_gaokun_ec"
> +#define GAOKUN_DEV_PSY "psy"
> +#define GAOKUN_DEV_WMI "wmi"
> +#define GAOKUN_DEV_UCSI "ucsi"
> +
> +/* -------------------------------------------------------------------------- */
> +/* Common API */
> +
> +int gaokun_ec_register_notify(struct gaokun_ec *ec,
> + struct notifier_block *nb);
> +void gaokun_ec_unregister_notify(struct gaokun_ec *ec,
> + struct notifier_block *nb);
> +
> +int gaokun_ec_read(struct gaokun_ec *ec, const u8 *req,
> + size_t resp_len, u8 *resp);
> +int gaokun_ec_write(struct gaokun_ec *ec, u8 *req);
> +int gaokun_ec_read_byte(struct gaokun_ec *ec, u8 *req, u8 *byte);
> +
> +/* -------------------------------------------------------------------------- */
> +/* API For PSY */
> +
> +int gaokun_ec_psy_multi_read(struct gaokun_ec *ec, u8 reg,
> + size_t resp_len, u8 *resp);
> +
> +static inline int gaokun_ec_psy_read_byte(struct gaokun_ec *ec,
> + u8 reg, u8 *byte)
> +{
> + return gaokun_ec_psy_multi_read(ec, reg, 1, byte);
> +}
> +
> +static inline int gaokun_ec_psy_read_word(struct gaokun_ec *ec,
> + u8 reg, u16 *word)
> +{
> + return gaokun_ec_psy_multi_read(ec, reg, 2, (u8 *)word);
> +}
> +
> +/* -------------------------------------------------------------------------- */
> +/* API For WMI */
> +
> +int gaokun_ec_wmi_get_threshold(struct gaokun_ec *ec, u8 *value, int ind);
> +int gaokun_ec_wmi_set_threshold(struct gaokun_ec *ec, u8 start, u8 end);
> +
> +int gaokun_ec_wmi_get_smart_charge_param(struct gaokun_ec *ec, u8 *value);
> +int gaokun_ec_wmi_set_smart_charge_param(struct gaokun_ec *ec, u8 value);
> +
> +int gaokun_ec_wmi_get_smart_charge(struct gaokun_ec *ec,
> + u8 data[GAOKUN_SMART_CHARGE_DATA_SIZE]);
> +int gaokun_ec_wmi_set_smart_charge(struct gaokun_ec *ec,
> + u8 data[GAOKUN_SMART_CHARGE_DATA_SIZE]);
> +
> +int gaokun_ec_wmi_get_fn_lock(struct gaokun_ec *ec, u8 *on);
> +int gaokun_ec_wmi_set_fn_lock(struct gaokun_ec *ec, u8 on);
> +
> +int gaokun_ec_wmi_get_temp(struct gaokun_ec *ec, s16 temp[GAOKUN_TZ_REG_NUM]);
> +
> +/* -------------------------------------------------------------------------- */
> +/* API For UCSI */
> +
> +int gaokun_ec_ucsi_read(struct gaokun_ec *ec, u8 resp[GAOKUN_UCSI_READ_SIZE]);
> +int gaokun_ec_ucsi_write(struct gaokun_ec *ec,
> + const u8 req[GAOKUN_UCSI_WRITE_SIZE]);
> +
> +int gaokun_ec_ucsi_get_reg(struct gaokun_ec *ec, u8 *ureg);
> +int gaokun_ec_ucsi_pan_ack(struct gaokun_ec *ec, int port_id);
> +
> +
> +#endif /* __HUAWEI_GAOKUN_EC_H__ */
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver
2024-12-28 12:33 ` [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver Bryan O'Donoghue
@ 2024-12-28 13:51 ` Pengyu Luo
2024-12-29 15:32 ` Ilpo Järvinen
0 siblings, 1 reply; 52+ messages in thread
From: Pengyu Luo @ 2024-12-28 13:51 UTC (permalink / raw)
To: bryan.odonoghue
Cc: andersson, conor+dt, devicetree, dmitry.baryshkov, gregkh,
hdegoede, heikki.krogerus, ilpo.jarvinen, konradybcio, krzk+dt,
linux-arm-msm, linux-kernel, linux-pm, linux-usb, mitltlatltl,
nikita, platform-driver-x86, robh, sre
On Sat, Dec 28, 2024 at 8:33 PM Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote:
> 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.
>
You have mentioned many grammar and code issues.
So I explain something, I taught myself C, writing code is just my hobby.
I am also not a native speaker, I have never lived in an English
environment. Sometime I may use inappropriate words to express. I try my
best to express my idea clearly. I hope you can understand.
> > 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"
>
should be, I want to express I am guessing.
I guess the last one is also gaokun3. But anyways, this driver works
for the latter two.
> > 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"
>
This was copied from C630 patches, replaced with my devices.
> >
> > Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
> > ---
> > drivers/platform/arm64/Kconfig | 19 +
> > drivers/platform/arm64/Makefile | 2 +
> > drivers/platform/arm64/huawei-gaokun-ec.c | 598 ++++++++++++++++++
> > drivers/platform/arm64/huawei-gaokun-wmi.c | 283 +++++++++
> > .../linux/platform_data/huawei-gaokun-ec.h | 90 +++
> > 5 files changed, 992 insertions(+)
> > create mode 100644 drivers/platform/arm64/huawei-gaokun-ec.c
> > create mode 100644 drivers/platform/arm64/huawei-gaokun-wmi.c
> > create mode 100644 include/linux/platform_data/huawei-gaokun-ec.h
> >
> > diff --git a/drivers/platform/arm64/Kconfig b/drivers/platform/arm64/Kconfig
> > index f88395ea3..eb7fbacf0 100644
> > --- a/drivers/platform/arm64/Kconfig
> > +++ b/drivers/platform/arm64/Kconfig
> > @@ -33,6 +33,25 @@ config EC_ACER_ASPIRE1
> > laptop where this information is not properly exposed via the
> > standard ACPI devices.
> >
> > +config EC_HUAWEI_GAOKUN
> > + tristate "Huawei Matebook E Go (sc8280xp) Embedded Controller driver"
>
> Krzysztof already mentioned this but the "sc8280xp" is questionable, you
> should probably drop mention of qcom and sc8280xp from your compat and
> tristate here.
>
Agree, how about using sc8280xp-based for tristate?
> > + 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.
>
I see
> > +
> > config EC_LENOVO_YOGA_C630
> > tristate "Lenovo Yoga C630 Embedded Controller driver"
> > depends on ARCH_QCOM || COMPILE_TEST
> > diff --git a/drivers/platform/arm64/Makefile b/drivers/platform/arm64/Makefile
> > index b2ae9114f..ed32ad6c0 100644
> > --- a/drivers/platform/arm64/Makefile
> > +++ b/drivers/platform/arm64/Makefile
> > @@ -6,4 +6,6 @@
> > #
> >
> > obj-$(CONFIG_EC_ACER_ASPIRE1) += acer-aspire1-ec.o
> > +obj-$(CONFIG_EC_HUAWEI_GAOKUN) += huawei-gaokun-ec.o
> > +obj-$(CONFIG_EC_HUAWEI_GAOKUN) += huawei-gaokun-wmi.o
> > obj-$(CONFIG_EC_LENOVO_YOGA_C630) += lenovo-yoga-c630.o
> > diff --git a/drivers/platform/arm64/huawei-gaokun-ec.c b/drivers/platform/arm64/huawei-gaokun-ec.c
> > new file mode 100644
> > index 000000000..c1c657f7b
> > --- /dev/null
> > +++ b/drivers/platform/arm64/huawei-gaokun-ec.c
> > @@ -0,0 +1,598 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * huawei-gaokun-ec - An EC driver for HUAWEI Matebook E Go (sc8280xp)
> > + *
> > + * reference: drivers/platform/arm64/acer-aspire1-ec.c
> > + * drivers/platform/arm64/lenovo-yoga-c630.c
> > + *
> > + * Copyright (C) 2024 Pengyu Luo <mitltlatltl@gmail.com>
> > + */
> > +
> > +#include <linux/auxiliary_bus.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/i2c.h>
> > +#include <linux/input.h>
> > +#include <linux/notifier.h>
> > +#include <linux/module.h>
> > +#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 */
> > +#define EC_READ 0x80
> > +#define EC_WRITE 0x81
>
> Is this odd indentation ?
>
No, apply it then check the source, it is normal.
> > +#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 ?
>
Have a break between 2 transaction. This sleep happens in acpi code, also
inside a critical region. I rearrange it. We can sleep when using mutex
lock instead of spinlock.
Local7 = Acquire (\_SB.IC16.MUEC, 0x03E8)
...
read ops
...
Sleep (0x02)
...
write 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.
>
> depend on this
>
mistyping
> > + *
> > + * 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.
>
I will try to do it, but there are many magic sequences in any size, I
need pass them anyways.
> > +
> > + 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 ..
>
Yeah, this code was written with int. Later, I used u8 since one byte is
convenient for EC transaction(we don't need clear other 3 Bytes). After
chaging, I forgot to revaluate it.
>
> > + 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.
>
Agree
> > +
> > + 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 ?
>
Yes, we can.
> > +
> > + 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)
>
It is a constant. But later, as Krzysztof suggested, I will use interfaces
from hwmon, then reading one at a time.
> > + 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++;
>
A 2Bytes symbol number in little endian, ec return it like this, so
casting.
> > + }
> > +
> > + 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 ?
>
After resueming from suspend, it should write somting to EC, but EC may
not resume, if not, then retry after a break.
> Deserves a comment at least.
>
I see.
> > +
> > + ec->suspended = false;
> > +
> > + return 0;
> > +}
> > +
> > +static void gaokun_aux_release(struct device *dev)
> > +{
> > + struct auxiliary_device *adev = to_auxiliary_dev(dev);
> > +
> > + kfree(adev);
> > +}
> > +
> > +static void gaokun_aux_remove(void *data)
> > +{
> > + struct auxiliary_device *adev = data;
> > +
> > + auxiliary_device_delete(adev);
> > + auxiliary_device_uninit(adev);
> > +}
> > +
> > +static int gaokun_aux_init(struct device *parent, const char *name,
> > + struct gaokun_ec *ec)
> > +{
> > + struct auxiliary_device *adev;
> > + int ret;
> > +
> > + adev = kzalloc(sizeof(*adev), GFP_KERNEL);
> > + if (!adev)
> > + return -ENOMEM;
> > +
> > + adev->name = name;
> > + adev->id = 0;
> > + adev->dev.parent = parent;
> > + adev->dev.release = gaokun_aux_release;
> > + adev->dev.platform_data = ec;
> > + /* Allow aux devices to access parent's DT nodes directly */
> > + device_set_of_node_from_dev(&adev->dev, parent);
> > +
> > + ret = auxiliary_device_init(adev);
> > + if (ret) {
> > + kfree(adev);
> > + return ret;
> > + }
> > +
> > + ret = auxiliary_device_add(adev);
> > + if (ret) {
> > + auxiliary_device_uninit(adev);
> > + return ret;
> > + }
> > +
> > + return devm_add_action_or_reset(parent, gaokun_aux_remove, adev);
> > +}
> > +
> > +/* -------------------------------------------------------------------------- */
> > +/* EC */
> > +
> > +static irqreturn_t gaokun_ec_irq_handler(int irq, void *data)
> > +{
> > + struct gaokun_ec *ec = data;
> > + u8 req[REQ_HDR_SIZE] = {EC_EVENT, EC_QUERY, 0};
> > + u8 status, id;
> > + int ret;
> > +
> > + ret = gaokun_ec_read_byte(ec, req, &id);
> > + if (ret)
> > + return IRQ_HANDLED;
> > +
> > + switch (id) {
> > + case 0x0: /* No event */
> > + break;
> > +
> > + case EC_EVENT_LID:
> > + gaokun_ec_psy_read_byte(ec, EC_LID_STATE, &status);
> > + status = EC_LID_OPEN & status;
> > + input_report_switch(ec->idev, SW_LID, !status);
> > + input_sync(ec->idev);
> > + break;
> > +
> > + default:
> > + blocking_notifier_call_chain(&ec->notifier_list, id, ec);
> > + }
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int gaokun_ec_probe(struct i2c_client *client)
> > +{
> > + struct device *dev = &client->dev;
> > + struct gaokun_ec *ec;
> > + int ret;
> > +
> > + ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL);
> > + if (!ec)
> > + return -ENOMEM;
> > +
> > + mutex_init(&ec->lock);
> > + ec->client = client;
> > + i2c_set_clientdata(client, ec);
> > + BLOCKING_INIT_NOTIFIER_HEAD(&ec->notifier_list);
> > +
> > + /* Lid switch */
> > + ec->idev = devm_input_allocate_device(dev);
> > + if (!ec->idev)
> > + return -ENOMEM;
> > +
> > + ec->idev->name = "LID";
> > + ec->idev->phys = "gaokun-ec/input0";
> > + input_set_capability(ec->idev, EV_SW, SW_LID);
> > +
> > + ret = input_register_device(ec->idev);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed to register input device\n");
> > +
> > + ret = gaokun_aux_init(dev, "psy", ec);
> > + if (ret)
> > + return ret;
> > +
> > + ret = gaokun_aux_init(dev, "wmi", ec);
> > + if (ret)
> > + return ret;
> > +
> > + ret = gaokun_aux_init(dev, "ucsi", ec);
> > + if (ret)
> > + return ret;
> > +
> > + ret = devm_request_threaded_irq(dev, client->irq, NULL,
> > + gaokun_ec_irq_handler, IRQF_ONESHOT,
> > + dev_name(dev), ec);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed to request irq\n");
> > +
> > + return 0;
> > +}
> > +
> > +static const struct i2c_device_id gaokun_ec_id[] = {
> > + { "gaokun-ec", },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, gaokun_ec_id);
> > +
> > +static const struct of_device_id gaokun_ec_of_match[] = {
> > + { .compatible = "huawei,gaokun-ec", },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, gaokun_ec_of_match);
> > +
> > +static const struct dev_pm_ops gaokun_ec_pm_ops = {
> > + NOIRQ_SYSTEM_SLEEP_PM_OPS(gaokun_ec_suspend, gaokun_ec_resume)
> > +};
> > +
> > +static struct i2c_driver gaokun_ec_driver = {
> > + .driver = {
> > + .name = "gaokun-ec",
> > + .of_match_table = gaokun_ec_of_match,
> > + .pm = &gaokun_ec_pm_ops,
> > + },
> > + .probe = gaokun_ec_probe,
> > + .id_table = gaokun_ec_id,
> > +};
> > +module_i2c_driver(gaokun_ec_driver);
> > +
> > +MODULE_DESCRIPTION("HUAWEI Matebook E Go EC driver");
> > +MODULE_AUTHOR("Pengyu Luo <mitltlatltl@gmail.com>");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/platform/arm64/huawei-gaokun-wmi.c b/drivers/platform/arm64/huawei-gaokun-wmi.c
> > new file mode 100644
> > index 000000000..793cb1659
> > --- /dev/null
> > +++ b/drivers/platform/arm64/huawei-gaokun-wmi.c
> > @@ -0,0 +1,283 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * huawei-gaokun-wmi - A WMI driver for HUAWEI Matebook E Go (sc8280xp)
> > + *
> > + * reference: drivers/platform/x86/huawei-wmi.c
> > + *
> > + * Copyright (C) 2024 Pengyu Luo <mitltlatltl@gmail.com>
> > + */
> > +
> > +#include <linux/auxiliary_bus.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/version.h>
> > +
> > +#include <linux/platform_data/huawei-gaokun-ec.h>
> > +
> > +struct gaokun_wmi {
> > + struct gaokun_ec *ec;
> > + struct device *dev;
> > + struct platform_device *wmi;
> > +};
> > +
> > +/* -------------------------------------------------------------------------- */
> > +/* Battery charging threshold */
> > +
> > +enum gaokun_wmi_threshold_ind {
> > + START = 1,
> > + END = 2,
> > +};
> > +
> > +static ssize_t charge_control_thresholds_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + int ret;
> > + u8 start, end;
> > + struct gaokun_wmi *ecwmi = dev_get_drvdata(dev);
> > +
> > + ret = gaokun_ec_wmi_get_threshold(ecwmi->ec, &start, START)
> > + || gaokun_ec_wmi_get_threshold(ecwmi->ec, &end, END);
> > + if (ret)
> > + return ret;
>
> ick ouch.
>
> Please call these two functions and evaluate their result codes
> individually.
>
Agree
> > +
> > + 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.
>
I see
> > + 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.
>
EC's logic, it handles them as symbol numbers, and dealing negative with
bit ops. And I think not all sensors are for silicon.
{
BUF1 = \_SB.IC16.ECCD (0x02, 0x61, One, ISNB, 0x04)
TEMP [Zero] = DerefOf (BUF1 [0x02])
TEMP [One] = DerefOf (BUF1 [0x03])
Local1 = TEMP /* \GTMP.TEMP */
Local2 = DerefOf (TEMP [Zero])
Local3 = DerefOf (TEMP [One])
If ((Local2 == 0xFF))
{
TEMP = Zero
STAT = One
}
Else
{
Local4 = (Local3 << 0x08)
Local1 = (Local4 | Local2)
Local4 = (Local3 & 0x80)
If (Local4)
{
TFLG = One
Local5 = (Local1 - One)
Local1 = ~Local5
}
Divide (Local1, 0x0A, Local4, Local5)
TMPD = Local4
TMPI = Local5
STAT = Zero
}
}
> > + value = -value;
> > + }
> > + len += sprintf(ptr + len, "%d.%d ", value / 10, value % 10);
> > + }
> > + len += sprintf(ptr + len, "\n");
> > +
> > + return len;
> > +}
> > +
> > +static DEVICE_ATTR_RO(temperature);
> > +
> > +static struct attribute *gaokun_wmi_features_attrs[] = {
> > + &dev_attr_charge_control_thresholds.attr,
> > + &dev_attr_smart_charge_param.attr,
> > + &dev_attr_smart_charge.attr,
> > + &dev_attr_fn_lock_state.attr,
> > + &dev_attr_temperature.attr,
> > + NULL,
> > +};
> > +ATTRIBUTE_GROUPS(gaokun_wmi_features);
> > +
> > +static int gaokun_wmi_probe(struct auxiliary_device *adev,
> > + const struct auxiliary_device_id *id)
> > +{
> > + struct gaokun_ec *ec = adev->dev.platform_data;
> > + struct device *dev = &adev->dev;
> > + struct gaokun_wmi *ecwmi;
> > +
> > + ecwmi = devm_kzalloc(&adev->dev, sizeof(*ecwmi), GFP_KERNEL);
> > + if (!ecwmi)
> > + return -ENOMEM;
> > +
> > + ecwmi->ec = ec;
> > + ecwmi->dev = dev;
> > +
> > + auxiliary_set_drvdata(adev, ecwmi);
> > +
> > + /* make it under /sys/devices/platform, convenient for sysfs I/O,
> > + * while adev is under
> > + * /sys/devices/platform/soc@0/ac0000.geniqup/a9c000.i2c/i2c-15/15-0038/
> > + */
> > + ecwmi->wmi = platform_device_register_simple("gaokun-wmi", -1, NULL, 0);
> > + if (IS_ERR(ecwmi->wmi))
> > + return dev_err_probe(dev, PTR_ERR(ecwmi->wmi),
> > + "Failed to register wmi platform device\n");
> > +
> > + platform_set_drvdata(ecwmi->wmi, ecwmi);
> > +
> > + return device_add_groups(&ecwmi->wmi->dev, gaokun_wmi_features_groups);
> > +}
> > +
> > +static void gaokun_wmi_remove(struct auxiliary_device *adev)
> > +{
> > + struct gaokun_wmi *ecwmi = auxiliary_get_drvdata(adev);
> > + struct platform_device *wmi = ecwmi->wmi;
> > +
> > + device_remove_groups(&wmi->dev, gaokun_wmi_features_groups);
> > + platform_device_unregister(ecwmi->wmi);
> > +}
> > +
> > +static const struct auxiliary_device_id gaokun_wmi_id_table[] = {
> > + { .name = GAOKUN_MOD_NAME "." GAOKUN_DEV_WMI, },
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(auxiliary, gaokun_wmi_id_table);
> > +
> > +static struct auxiliary_driver gaokun_wmi_driver = {
> > + .name = GAOKUN_DEV_WMI,
> > + .id_table = gaokun_wmi_id_table,
> > + .probe = gaokun_wmi_probe,
> > + .remove = gaokun_wmi_remove,
> > +};
> > +
> > +module_auxiliary_driver(gaokun_wmi_driver);
> > +
> > +MODULE_DESCRIPTION("HUAWEI Matebook E Go WMI driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/platform_data/huawei-gaokun-ec.h b/include/linux/platform_data/huawei-gaokun-ec.h
> > new file mode 100644
> > index 000000000..a649e9ecf
> > --- /dev/null
> > +++ b/include/linux/platform_data/huawei-gaokun-ec.h
> > @@ -0,0 +1,90 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Huawei Matebook E Go (sc8280xp) Embedded Controller
> > + *
> > + * Copyright (C) 2024 Pengyu Luo <mitltlatltl@gmail.com>
> > + *
> > + */
> > +
> > +#ifndef __HUAWEI_GAOKUN_EC_H__
> > +#define __HUAWEI_GAOKUN_EC_H__
> > +
> > +#define GAOKUN_UCSI_CCI_SIZE 4
> > +#define GAOKUN_UCSI_DATA_SIZE 16
> > +#define GAOKUN_UCSI_READ_SIZE (GAOKUN_UCSI_CCI_SIZE + GAOKUN_UCSI_DATA_SIZE)
> > +#define GAOKUN_UCSI_WRITE_SIZE 0x18
> > +
> > +#define GAOKUN_TZ_REG_NUM 20
> > +#define GAOKUN_SMART_CHARGE_DATA_SIZE 4 /* mode, delay, start, end */
> > +
> > +/* -------------------------------------------------------------------------- */
> > +
> > +struct gaokun_ec;
> > +struct notifier_block;
> > +
> > +#define GAOKUN_MOD_NAME "huawei_gaokun_ec"
> > +#define GAOKUN_DEV_PSY "psy"
> > +#define GAOKUN_DEV_WMI "wmi"
> > +#define GAOKUN_DEV_UCSI "ucsi"
> > +
> > +/* -------------------------------------------------------------------------- */
> > +/* Common API */
> > +
> > +int gaokun_ec_register_notify(struct gaokun_ec *ec,
> > + struct notifier_block *nb);
> > +void gaokun_ec_unregister_notify(struct gaokun_ec *ec,
> > + struct notifier_block *nb);
> > +
> > +int gaokun_ec_read(struct gaokun_ec *ec, const u8 *req,
> > + size_t resp_len, u8 *resp);
> > +int gaokun_ec_write(struct gaokun_ec *ec, u8 *req);
> > +int gaokun_ec_read_byte(struct gaokun_ec *ec, u8 *req, u8 *byte);
> > +
> > +/* -------------------------------------------------------------------------- */
> > +/* API For PSY */
> > +
> > +int gaokun_ec_psy_multi_read(struct gaokun_ec *ec, u8 reg,
> > + size_t resp_len, u8 *resp);
> > +
> > +static inline int gaokun_ec_psy_read_byte(struct gaokun_ec *ec,
> > + u8 reg, u8 *byte)
> > +{
> > + return gaokun_ec_psy_multi_read(ec, reg, 1, byte);
> > +}
> > +
> > +static inline int gaokun_ec_psy_read_word(struct gaokun_ec *ec,
> > + u8 reg, u16 *word)
> > +{
> > + return gaokun_ec_psy_multi_read(ec, reg, 2, (u8 *)word);
> > +}
> > +
> > +/* -------------------------------------------------------------------------- */
> > +/* API For WMI */
> > +
> > +int gaokun_ec_wmi_get_threshold(struct gaokun_ec *ec, u8 *value, int ind);
> > +int gaokun_ec_wmi_set_threshold(struct gaokun_ec *ec, u8 start, u8 end);
> > +
> > +int gaokun_ec_wmi_get_smart_charge_param(struct gaokun_ec *ec, u8 *value);
> > +int gaokun_ec_wmi_set_smart_charge_param(struct gaokun_ec *ec, u8 value);
> > +
> > +int gaokun_ec_wmi_get_smart_charge(struct gaokun_ec *ec,
> > + u8 data[GAOKUN_SMART_CHARGE_DATA_SIZE]);
> > +int gaokun_ec_wmi_set_smart_charge(struct gaokun_ec *ec,
> > + u8 data[GAOKUN_SMART_CHARGE_DATA_SIZE]);
> > +
> > +int gaokun_ec_wmi_get_fn_lock(struct gaokun_ec *ec, u8 *on);
> > +int gaokun_ec_wmi_set_fn_lock(struct gaokun_ec *ec, u8 on);
> > +
> > +int gaokun_ec_wmi_get_temp(struct gaokun_ec *ec, s16 temp[GAOKUN_TZ_REG_NUM]);
> > +
> > +/* -------------------------------------------------------------------------- */
> > +/* API For UCSI */
> > +
> > +int gaokun_ec_ucsi_read(struct gaokun_ec *ec, u8 resp[GAOKUN_UCSI_READ_SIZE]);
> > +int gaokun_ec_ucsi_write(struct gaokun_ec *ec,
> > + const u8 req[GAOKUN_UCSI_WRITE_SIZE]);
> > +
> > +int gaokun_ec_ucsi_get_reg(struct gaokun_ec *ec, u8 *ureg);
> > +int gaokun_ec_ucsi_pan_ack(struct gaokun_ec *ec, int port_id);
> > +
> > +
> > +#endif /* __HUAWEI_GAOKUN_EC_H__ */
>
Best Wishes,
Pengyu
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver
2024-12-28 13:51 ` Pengyu Luo
@ 2024-12-29 15:32 ` Ilpo Järvinen
2024-12-29 15:55 ` Pengyu Luo
0 siblings, 1 reply; 52+ messages in thread
From: Ilpo Järvinen @ 2024-12-29 15:32 UTC (permalink / raw)
To: Pengyu Luo
Cc: bryan.odonoghue, andersson, conor+dt, devicetree,
dmitry.baryshkov, Greg Kroah-Hartman, Hans de Goede,
heikki.krogerus, konradybcio, krzk+dt, linux-arm-msm, LKML,
linux-pm, linux-usb, nikita, platform-driver-x86, robh, sre
[-- Attachment #1: Type: text/plain, Size: 20307 bytes --]
On Sat, 28 Dec 2024, Pengyu Luo wrote:
> On Sat, Dec 28, 2024 at 8:33 PM Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote:
> > 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.
> >
>
> You have mentioned many grammar and code issues.
> So I explain something, I taught myself C, writing code is just my hobby.
> I am also not a native speaker, I have never lived in an English
> environment. Sometime I may use inappropriate words to express. I try my
> best to express my idea clearly. I hope you can understand.
>
> > > 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"
> >
>
> should be, I want to express I am guessing.
> I guess the last one is also gaokun3. But anyways, this driver works
> for the latter two.
>
> > > 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"
> >
>
> This was copied from C630 patches, replaced with my devices.
>
> > >
> > > Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
> > > ---
> > > drivers/platform/arm64/Kconfig | 19 +
> > > drivers/platform/arm64/Makefile | 2 +
> > > drivers/platform/arm64/huawei-gaokun-ec.c | 598 ++++++++++++++++++
> > > drivers/platform/arm64/huawei-gaokun-wmi.c | 283 +++++++++
> > > .../linux/platform_data/huawei-gaokun-ec.h | 90 +++
> > > 5 files changed, 992 insertions(+)
> > > create mode 100644 drivers/platform/arm64/huawei-gaokun-ec.c
> > > create mode 100644 drivers/platform/arm64/huawei-gaokun-wmi.c
> > > create mode 100644 include/linux/platform_data/huawei-gaokun-ec.h
> > >
> > > diff --git a/drivers/platform/arm64/Kconfig b/drivers/platform/arm64/Kconfig
> > > index f88395ea3..eb7fbacf0 100644
> > > --- a/drivers/platform/arm64/Kconfig
> > > +++ b/drivers/platform/arm64/Kconfig
> > > @@ -33,6 +33,25 @@ config EC_ACER_ASPIRE1
> > > laptop where this information is not properly exposed via the
> > > standard ACPI devices.
> > >
> > > +config EC_HUAWEI_GAOKUN
> > > + tristate "Huawei Matebook E Go (sc8280xp) Embedded Controller driver"
> >
> > Krzysztof already mentioned this but the "sc8280xp" is questionable, you
> > should probably drop mention of qcom and sc8280xp from your compat and
> > tristate here.
> >
>
> Agree, how about using sc8280xp-based for tristate?
>
> > > + 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.
> >
>
> I see
>
> > > +
> > > config EC_LENOVO_YOGA_C630
> > > tristate "Lenovo Yoga C630 Embedded Controller driver"
> > > depends on ARCH_QCOM || COMPILE_TEST
> > > diff --git a/drivers/platform/arm64/Makefile b/drivers/platform/arm64/Makefile
> > > index b2ae9114f..ed32ad6c0 100644
> > > --- a/drivers/platform/arm64/Makefile
> > > +++ b/drivers/platform/arm64/Makefile
> > > @@ -6,4 +6,6 @@
> > > #
> > >
> > > obj-$(CONFIG_EC_ACER_ASPIRE1) += acer-aspire1-ec.o
> > > +obj-$(CONFIG_EC_HUAWEI_GAOKUN) += huawei-gaokun-ec.o
> > > +obj-$(CONFIG_EC_HUAWEI_GAOKUN) += huawei-gaokun-wmi.o
> > > obj-$(CONFIG_EC_LENOVO_YOGA_C630) += lenovo-yoga-c630.o
> > > diff --git a/drivers/platform/arm64/huawei-gaokun-ec.c b/drivers/platform/arm64/huawei-gaokun-ec.c
> > > new file mode 100644
> > > index 000000000..c1c657f7b
> > > --- /dev/null
> > > +++ b/drivers/platform/arm64/huawei-gaokun-ec.c
> > > @@ -0,0 +1,598 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * huawei-gaokun-ec - An EC driver for HUAWEI Matebook E Go (sc8280xp)
> > > + *
> > > + * reference: drivers/platform/arm64/acer-aspire1-ec.c
> > > + * drivers/platform/arm64/lenovo-yoga-c630.c
> > > + *
> > > + * Copyright (C) 2024 Pengyu Luo <mitltlatltl@gmail.com>
> > > + */
> > > +
> > > +#include <linux/auxiliary_bus.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/device.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/input.h>
> > > +#include <linux/notifier.h>
> > > +#include <linux/module.h>
> > > +#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 */
> > > +#define EC_READ 0x80
> > > +#define EC_WRITE 0x81
> >
> > Is this odd indentation ?
> >
>
> No, apply it then check the source, it is normal.
>
> > > +#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 ?
> >
>
> Have a break between 2 transaction. This sleep happens in acpi code, also
> inside a critical region. I rearrange it. We can sleep when using mutex
> lock instead of spinlock.
>
> Local7 = Acquire (\_SB.IC16.MUEC, 0x03E8)
> ...
> read ops
> ...
> Sleep (0x02)
> ...
> write 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.
> >
> > depend on this
> >
>
> mistyping
>
> > > + *
> > > + * 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.
> >
>
> I will try to do it, but there are many magic sequences in any size, I
> need pass them anyways.
>
> > > +
> > > + 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 ..
> >
>
> Yeah, this code was written with int. Later, I used u8 since one byte is
> convenient for EC transaction(we don't need clear other 3 Bytes). After
> chaging, I forgot to revaluate it.
>
> >
> > > + 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.
> >
>
> Agree
>
> > > +
> > > + 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 ?
> >
>
> Yes, we can.
>
> > > +
> > > + 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)
> >
>
> It is a constant. But later, as Krzysztof suggested, I will use interfaces
> from hwmon, then reading one at a time.
>
> > > + 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++;
>
> A 2Bytes symbol number in little endian, ec return it like this, so
> casting.
You should use __le16 and proper endianess conversion function then.
It's bit confusing that in the declaration you used RESP_HDR_SIZE and here
you do it with DATA_OFFSET instead. It feels DATA_OFFSET is unnecessary
duplicate of RESP_HDR_SIZE and will easily lead confusing variation such
as above.
--
i.
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver
2024-12-29 15:32 ` Ilpo Järvinen
@ 2024-12-29 15:55 ` Pengyu Luo
0 siblings, 0 replies; 52+ messages in thread
From: Pengyu Luo @ 2024-12-29 15:55 UTC (permalink / raw)
To: ilpo.jarvinen
Cc: andersson, bryan.odonoghue, conor+dt, devicetree,
dmitry.baryshkov, gregkh, hdegoede, heikki.krogerus, konradybcio,
krzk+dt, linux-arm-msm, linux-kernel, linux-pm, linux-usb,
mitltlatltl, nikita, platform-driver-x86, robh, sre
On Sun, Dec 29, 2024 at 11:33 PM Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> On Sat, 28 Dec 2024, Pengyu Luo wrote:
> > On Sat, Dec 28, 2024 at 8:33 PM Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote:
> > > 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.
[skipped]
> > > > +/* 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)
> > >
> >
> > It is a constant. But later, as Krzysztof suggested, I will use interfaces
> > from hwmon, then reading one at a time.
> >
> > > > + 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++;
> >
> > A 2Bytes symbol number in little endian, ec return it like this, so
> > casting.
>
> You should use __le16 and proper endianess conversion function then.
>
Agree
> It's bit confusing that in the declaration you used RESP_HDR_SIZE and here
> you do it with DATA_OFFSET instead. It feels DATA_OFFSET is unnecessary
> duplicate of RESP_HDR_SIZE and will easily lead confusing variation such
> as above.
>
I totally agree with you, it is duplicated.
In declaration, u8 resp[RESP_HDR_SIZE]; RESP_HDR_SIZE indicates the size.
When assigning, val = resp[DATA_OFFSET]; let us know we are extracting a
data from a response without thinking, so I added an alias. Removing it
is also fine for me.
Best wishes,
Pengyu
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver
2024-12-27 17:13 ` [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver Pengyu Luo
` (2 preceding siblings ...)
2024-12-28 12:33 ` [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver Bryan O'Donoghue
@ 2024-12-29 14:49 ` Markus Elfring
2024-12-30 9:04 ` Aiqun(Maria) Yu
4 siblings, 0 replies; 52+ messages in thread
From: Markus Elfring @ 2024-12-29 14:49 UTC (permalink / raw)
To: Pengyu Luo, platform-driver-x86, devicetree, linux-arm-msm,
linux-usb, linux-pm, Bjorn Andersson, Bryan O'Donoghue,
Conor Dooley, Greg Kroah-Hartman, Hans de Goede, Heikki Krogerus,
Ilpo Järvinen, Konrad Dybcio, Krzysztof Kozlowski,
Rob Herring, Sebastian Reichel
Cc: LKML, Dmitry Baryshkov, Nikita Travkin
…
> +++ b/drivers/platform/arm64/huawei-gaokun-ec.c
> @@ -0,0 +1,598 @@
…
> +static int gaokun_ec_request(struct gaokun_ec *ec, const u8 *req,
> + size_t resp_len, u8 *resp)
> +{
…
> + mutex_lock(&ec->lock);
> +
> + i2c_transfer(client->adapter, msgs, 2);
> + usleep_range(2000, 2500);
> +
> + mutex_unlock(&ec->lock);
> +
> + return *resp;
> +}
…
Under which circumstances would you become interested to apply a statement
like “guard(mutex)(&ec->lock);”?
https://elixir.bootlin.com/linux/v6.13-rc3/source/include/linux/mutex.h#L201
Regards,
Markus
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver
2024-12-27 17:13 ` [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver Pengyu Luo
` (3 preceding siblings ...)
2024-12-29 14:49 ` Markus Elfring
@ 2024-12-30 9:04 ` Aiqun(Maria) Yu
2024-12-30 10:44 ` Pengyu Luo
4 siblings, 1 reply; 52+ messages in thread
From: Aiqun(Maria) Yu @ 2024-12-30 9:04 UTC (permalink / raw)
To: Pengyu Luo, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Hans de Goede, Ilpo Järvinen,
Bryan O'Donoghue, Sebastian Reichel, Heikki Krogerus,
Greg Kroah-Hartman
Cc: devicetree, linux-kernel, linux-arm-msm, platform-driver-x86,
linux-pm, linux-usb, Dmitry Baryshkov, Nikita Travkin
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).
>
> 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:
>
> - 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.
>
> Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
> ---
> drivers/platform/arm64/Kconfig | 19 +
> drivers/platform/arm64/Makefile | 2 +
> drivers/platform/arm64/huawei-gaokun-ec.c | 598 ++++++++++++++++++
> drivers/platform/arm64/huawei-gaokun-wmi.c | 283 +++++++++
> .../linux/platform_data/huawei-gaokun-ec.h | 90 +++
> 5 files changed, 992 insertions(+)
> create mode 100644 drivers/platform/arm64/huawei-gaokun-ec.c
> create mode 100644 drivers/platform/arm64/huawei-gaokun-wmi.c
> create mode 100644 include/linux/platform_data/huawei-gaokun-ec.h
>
> diff --git a/drivers/platform/arm64/Kconfig b/drivers/platform/arm64/Kconfig
> index f88395ea3..eb7fbacf0 100644
> --- a/drivers/platform/arm64/Kconfig
> +++ b/drivers/platform/arm64/Kconfig
> @@ -33,6 +33,25 @@ config EC_ACER_ASPIRE1
> laptop where this information is not properly exposed via the
> standard ACPI devices.
>
> +config EC_HUAWEI_GAOKUN
> + tristate "Huawei Matebook E Go (sc8280xp) Embedded Controller driver"
> + 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.
> +
> config EC_LENOVO_YOGA_C630
> tristate "Lenovo Yoga C630 Embedded Controller driver"
> depends on ARCH_QCOM || COMPILE_TEST
> diff --git a/drivers/platform/arm64/Makefile b/drivers/platform/arm64/Makefile
> index b2ae9114f..ed32ad6c0 100644
> --- a/drivers/platform/arm64/Makefile
> +++ b/drivers/platform/arm64/Makefile
> @@ -6,4 +6,6 @@
> #
>
> obj-$(CONFIG_EC_ACER_ASPIRE1) += acer-aspire1-ec.o
> +obj-$(CONFIG_EC_HUAWEI_GAOKUN) += huawei-gaokun-ec.o
> +obj-$(CONFIG_EC_HUAWEI_GAOKUN) += huawei-gaokun-wmi.o
> obj-$(CONFIG_EC_LENOVO_YOGA_C630) += lenovo-yoga-c630.o
> diff --git a/drivers/platform/arm64/huawei-gaokun-ec.c b/drivers/platform/arm64/huawei-gaokun-ec.c
> new file mode 100644
> index 000000000..c1c657f7b
> --- /dev/null
> +++ b/drivers/platform/arm64/huawei-gaokun-ec.c
> @@ -0,0 +1,598 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * huawei-gaokun-ec - An EC driver for HUAWEI Matebook E Go (sc8280xp)
> + *
> + * reference: drivers/platform/arm64/acer-aspire1-ec.c
> + * drivers/platform/arm64/lenovo-yoga-c630.c
> + *
> + * Copyright (C) 2024 Pengyu Luo <mitltlatltl@gmail.com>
> + */
> +
> +#include <linux/auxiliary_bus.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/notifier.h>
> +#include <linux/module.h>
> +#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?
> +#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.
> + usleep_range(2000, 2500);
Why is a sleep needed here? Is this information specified in any datasheet?
> +
> + 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.
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};
> +
> + 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) {
> + 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;
> + 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)
> + 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;
> +
> + 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};
> +
> + 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;
> +
> + 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])
> +{
> + /* 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) {
> + 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);
> + }
> +
> + 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);
> + };
> +
> + ec->suspended = false;
> +
> + return 0;
> +}
> +
> +static void gaokun_aux_release(struct device *dev)
> +{
> + struct auxiliary_device *adev = to_auxiliary_dev(dev);
> +
> + kfree(adev);
> +}
> +
> +static void gaokun_aux_remove(void *data)
> +{
> + struct auxiliary_device *adev = data;
> +
> + auxiliary_device_delete(adev);
> + auxiliary_device_uninit(adev);
> +}
> +
> +static int gaokun_aux_init(struct device *parent, const char *name,
> + struct gaokun_ec *ec)
> +{
> + struct auxiliary_device *adev;
> + int ret;
> +
> + adev = kzalloc(sizeof(*adev), GFP_KERNEL);
> + if (!adev)
> + return -ENOMEM;
> +
> + adev->name = name;
> + adev->id = 0;
> + adev->dev.parent = parent;
> + adev->dev.release = gaokun_aux_release;
> + adev->dev.platform_data = ec;
> + /* Allow aux devices to access parent's DT nodes directly */
> + device_set_of_node_from_dev(&adev->dev, parent);
> +
> + ret = auxiliary_device_init(adev);
> + if (ret) {
> + kfree(adev);
> + return ret;
> + }
> +
> + ret = auxiliary_device_add(adev);
> + if (ret) {
> + auxiliary_device_uninit(adev);
> + return ret;
> + }
> +
> + return devm_add_action_or_reset(parent, gaokun_aux_remove, adev);
> +}
> +
> +/* -------------------------------------------------------------------------- */
> +/* EC */
> +
> +static irqreturn_t gaokun_ec_irq_handler(int irq, void *data)
> +{
> + struct gaokun_ec *ec = data;
> + u8 req[REQ_HDR_SIZE] = {EC_EVENT, EC_QUERY, 0};
> + u8 status, id;
> + int ret;
> +
> + ret = gaokun_ec_read_byte(ec, req, &id);
> + if (ret)
> + return IRQ_HANDLED;
> +
> + switch (id) {
> + case 0x0: /* No event */
> + break;
> +
> + case EC_EVENT_LID:
> + gaokun_ec_psy_read_byte(ec, EC_LID_STATE, &status);
> + status = EC_LID_OPEN & status;
> + input_report_switch(ec->idev, SW_LID, !status);
> + input_sync(ec->idev);
> + break;
> +
> + default:
> + blocking_notifier_call_chain(&ec->notifier_list, id, ec);
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int gaokun_ec_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct gaokun_ec *ec;
> + int ret;
> +
> + ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL);
> + if (!ec)
> + return -ENOMEM;
> +
> + mutex_init(&ec->lock);
> + ec->client = client;
> + i2c_set_clientdata(client, ec);
> + BLOCKING_INIT_NOTIFIER_HEAD(&ec->notifier_list);
> +
> + /* Lid switch */
> + ec->idev = devm_input_allocate_device(dev);
> + if (!ec->idev)
> + return -ENOMEM;
> +
> + ec->idev->name = "LID";
> + ec->idev->phys = "gaokun-ec/input0";
> + input_set_capability(ec->idev, EV_SW, SW_LID);
> +
> + ret = input_register_device(ec->idev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to register input device\n");
> +
> + ret = gaokun_aux_init(dev, "psy", ec);
> + if (ret)
> + return ret;
> +
> + ret = gaokun_aux_init(dev, "wmi", ec);
> + if (ret)
> + return ret;
> +
> + ret = gaokun_aux_init(dev, "ucsi", ec);
> + if (ret)
> + return ret;
> +
> + ret = devm_request_threaded_irq(dev, client->irq, NULL,
> + gaokun_ec_irq_handler, IRQF_ONESHOT,
> + dev_name(dev), ec);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to request irq\n");
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id gaokun_ec_id[] = {
> + { "gaokun-ec", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, gaokun_ec_id);
> +
> +static const struct of_device_id gaokun_ec_of_match[] = {
> + { .compatible = "huawei,gaokun-ec", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, gaokun_ec_of_match);
> +
> +static const struct dev_pm_ops gaokun_ec_pm_ops = {
> + NOIRQ_SYSTEM_SLEEP_PM_OPS(gaokun_ec_suspend, gaokun_ec_resume)
> +};
> +
> +static struct i2c_driver gaokun_ec_driver = {
> + .driver = {
> + .name = "gaokun-ec",
> + .of_match_table = gaokun_ec_of_match,
> + .pm = &gaokun_ec_pm_ops,
> + },
> + .probe = gaokun_ec_probe,
> + .id_table = gaokun_ec_id,
> +};
> +module_i2c_driver(gaokun_ec_driver);
> +
> +MODULE_DESCRIPTION("HUAWEI Matebook E Go EC driver");
> +MODULE_AUTHOR("Pengyu Luo <mitltlatltl@gmail.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/platform/arm64/huawei-gaokun-wmi.c b/drivers/platform/arm64/huawei-gaokun-wmi.c
> new file mode 100644
> index 000000000..793cb1659
> --- /dev/null
> +++ b/drivers/platform/arm64/huawei-gaokun-wmi.c
> @@ -0,0 +1,283 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * huawei-gaokun-wmi - A WMI driver for HUAWEI Matebook E Go (sc8280xp)
> + *
> + * reference: drivers/platform/x86/huawei-wmi.c
> + *
> + * Copyright (C) 2024 Pengyu Luo <mitltlatltl@gmail.com>
> + */
> +
> +#include <linux/auxiliary_bus.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/sysfs.h>
> +#include <linux/version.h>
> +
> +#include <linux/platform_data/huawei-gaokun-ec.h>
> +
> +struct gaokun_wmi {
> + struct gaokun_ec *ec;
> + struct device *dev;
> + struct platform_device *wmi;
> +};
> +
> +/* -------------------------------------------------------------------------- */
> +/* Battery charging threshold */
> +
> +enum gaokun_wmi_threshold_ind {
> + START = 1,
> + END = 2,
> +};
> +
> +static ssize_t charge_control_thresholds_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + int ret;
> + u8 start, end;
> + struct gaokun_wmi *ecwmi = dev_get_drvdata(dev);
> +
> + ret = gaokun_ec_wmi_get_threshold(ecwmi->ec, &start, START)
> + || gaokun_ec_wmi_get_threshold(ecwmi->ec, &end, END);
> + if (ret)
> + return ret;
> +
> + 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);
> + 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, "-");
> + value = -value;
> + }
> + len += sprintf(ptr + len, "%d.%d ", value / 10, value % 10);
> + }
> + len += sprintf(ptr + len, "\n");
> +
> + return len;
> +}
> +
> +static DEVICE_ATTR_RO(temperature);
> +
> +static struct attribute *gaokun_wmi_features_attrs[] = {
> + &dev_attr_charge_control_thresholds.attr,
> + &dev_attr_smart_charge_param.attr,
> + &dev_attr_smart_charge.attr,
> + &dev_attr_fn_lock_state.attr,
> + &dev_attr_temperature.attr,
> + NULL,
> +};
> +ATTRIBUTE_GROUPS(gaokun_wmi_features);
> +
> +static int gaokun_wmi_probe(struct auxiliary_device *adev,
> + const struct auxiliary_device_id *id)
> +{
> + struct gaokun_ec *ec = adev->dev.platform_data;
> + struct device *dev = &adev->dev;
> + struct gaokun_wmi *ecwmi;
> +
> + ecwmi = devm_kzalloc(&adev->dev, sizeof(*ecwmi), GFP_KERNEL);
> + if (!ecwmi)
> + return -ENOMEM;
> +
> + ecwmi->ec = ec;
> + ecwmi->dev = dev;
> +
> + auxiliary_set_drvdata(adev, ecwmi);
> +
> + /* make it under /sys/devices/platform, convenient for sysfs I/O,
> + * while adev is under
> + * /sys/devices/platform/soc@0/ac0000.geniqup/a9c000.i2c/i2c-15/15-0038/
> + */
> + ecwmi->wmi = platform_device_register_simple("gaokun-wmi", -1, NULL, 0);
> + if (IS_ERR(ecwmi->wmi))
> + return dev_err_probe(dev, PTR_ERR(ecwmi->wmi),
> + "Failed to register wmi platform device\n");
> +
> + platform_set_drvdata(ecwmi->wmi, ecwmi);
> +
> + return device_add_groups(&ecwmi->wmi->dev, gaokun_wmi_features_groups);
> +}
> +
> +static void gaokun_wmi_remove(struct auxiliary_device *adev)
> +{
> + struct gaokun_wmi *ecwmi = auxiliary_get_drvdata(adev);
> + struct platform_device *wmi = ecwmi->wmi;
> +
> + device_remove_groups(&wmi->dev, gaokun_wmi_features_groups);
> + platform_device_unregister(ecwmi->wmi);
> +}
> +
> +static const struct auxiliary_device_id gaokun_wmi_id_table[] = {
> + { .name = GAOKUN_MOD_NAME "." GAOKUN_DEV_WMI, },
> + {}
> +};
> +MODULE_DEVICE_TABLE(auxiliary, gaokun_wmi_id_table);
> +
> +static struct auxiliary_driver gaokun_wmi_driver = {
> + .name = GAOKUN_DEV_WMI,
> + .id_table = gaokun_wmi_id_table,
> + .probe = gaokun_wmi_probe,
> + .remove = gaokun_wmi_remove,
> +};
> +
> +module_auxiliary_driver(gaokun_wmi_driver);
> +
> +MODULE_DESCRIPTION("HUAWEI Matebook E Go WMI driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/platform_data/huawei-gaokun-ec.h b/include/linux/platform_data/huawei-gaokun-ec.h
> new file mode 100644
> index 000000000..a649e9ecf
> --- /dev/null
> +++ b/include/linux/platform_data/huawei-gaokun-ec.h
> @@ -0,0 +1,90 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Huawei Matebook E Go (sc8280xp) Embedded Controller
> + *
> + * Copyright (C) 2024 Pengyu Luo <mitltlatltl@gmail.com>
> + *
> + */
> +
> +#ifndef __HUAWEI_GAOKUN_EC_H__
> +#define __HUAWEI_GAOKUN_EC_H__
> +
> +#define GAOKUN_UCSI_CCI_SIZE 4
> +#define GAOKUN_UCSI_DATA_SIZE 16
> +#define GAOKUN_UCSI_READ_SIZE (GAOKUN_UCSI_CCI_SIZE + GAOKUN_UCSI_DATA_SIZE)
> +#define GAOKUN_UCSI_WRITE_SIZE 0x18
> +
> +#define GAOKUN_TZ_REG_NUM 20
> +#define GAOKUN_SMART_CHARGE_DATA_SIZE 4 /* mode, delay, start, end */
> +
> +/* -------------------------------------------------------------------------- */
> +
> +struct gaokun_ec;
> +struct notifier_block;
> +
> +#define GAOKUN_MOD_NAME "huawei_gaokun_ec"
> +#define GAOKUN_DEV_PSY "psy"
> +#define GAOKUN_DEV_WMI "wmi"
> +#define GAOKUN_DEV_UCSI "ucsi"
> +
> +/* -------------------------------------------------------------------------- */
> +/* Common API */
> +
> +int gaokun_ec_register_notify(struct gaokun_ec *ec,
> + struct notifier_block *nb);
> +void gaokun_ec_unregister_notify(struct gaokun_ec *ec,
> + struct notifier_block *nb);
> +
> +int gaokun_ec_read(struct gaokun_ec *ec, const u8 *req,
> + size_t resp_len, u8 *resp);
> +int gaokun_ec_write(struct gaokun_ec *ec, u8 *req);
> +int gaokun_ec_read_byte(struct gaokun_ec *ec, u8 *req, u8 *byte);
> +
> +/* -------------------------------------------------------------------------- */
> +/* API For PSY */
> +
> +int gaokun_ec_psy_multi_read(struct gaokun_ec *ec, u8 reg,
> + size_t resp_len, u8 *resp);
> +
> +static inline int gaokun_ec_psy_read_byte(struct gaokun_ec *ec,
> + u8 reg, u8 *byte)
> +{
> + return gaokun_ec_psy_multi_read(ec, reg, 1, byte);
> +}
> +
> +static inline int gaokun_ec_psy_read_word(struct gaokun_ec *ec,
> + u8 reg, u16 *word)
> +{
> + return gaokun_ec_psy_multi_read(ec, reg, 2, (u8 *)word);
> +}
> +
> +/* -------------------------------------------------------------------------- */
> +/* API For WMI */
> +
> +int gaokun_ec_wmi_get_threshold(struct gaokun_ec *ec, u8 *value, int ind);
> +int gaokun_ec_wmi_set_threshold(struct gaokun_ec *ec, u8 start, u8 end);
> +
> +int gaokun_ec_wmi_get_smart_charge_param(struct gaokun_ec *ec, u8 *value);
> +int gaokun_ec_wmi_set_smart_charge_param(struct gaokun_ec *ec, u8 value);
> +
> +int gaokun_ec_wmi_get_smart_charge(struct gaokun_ec *ec,
> + u8 data[GAOKUN_SMART_CHARGE_DATA_SIZE]);
> +int gaokun_ec_wmi_set_smart_charge(struct gaokun_ec *ec,
> + u8 data[GAOKUN_SMART_CHARGE_DATA_SIZE]);
> +
> +int gaokun_ec_wmi_get_fn_lock(struct gaokun_ec *ec, u8 *on);
> +int gaokun_ec_wmi_set_fn_lock(struct gaokun_ec *ec, u8 on);
> +
> +int gaokun_ec_wmi_get_temp(struct gaokun_ec *ec, s16 temp[GAOKUN_TZ_REG_NUM]);
> +
> +/* -------------------------------------------------------------------------- */
> +/* API For UCSI */
> +
> +int gaokun_ec_ucsi_read(struct gaokun_ec *ec, u8 resp[GAOKUN_UCSI_READ_SIZE]);
> +int gaokun_ec_ucsi_write(struct gaokun_ec *ec,
> + const u8 req[GAOKUN_UCSI_WRITE_SIZE]);
> +
> +int gaokun_ec_ucsi_get_reg(struct gaokun_ec *ec, u8 *ureg);
> +int gaokun_ec_ucsi_pan_ack(struct gaokun_ec *ec, int port_id);
> +
> +
> +#endif /* __HUAWEI_GAOKUN_EC_H__ */
--
Thx and BRs,
Aiqun(Maria) Yu
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver
2024-12-30 9:04 ` Aiqun(Maria) Yu
@ 2024-12-30 10:44 ` Pengyu Luo
2024-12-31 5:00 ` Aiqun(Maria) Yu
0 siblings, 1 reply; 52+ messages in thread
From: Pengyu Luo @ 2024-12-30 10:44 UTC (permalink / raw)
To: quic_aiquny
Cc: andersson, bryan.odonoghue, conor+dt, devicetree,
dmitry.baryshkov, gregkh, hdegoede, heikki.krogerus,
ilpo.jarvinen, konradybcio, krzk+dt, linux-arm-msm, linux-kernel,
linux-pm, linux-usb, mitltlatltl, nikita, platform-driver-x86,
robh, sre
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
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver
2024-12-30 10:44 ` Pengyu Luo
@ 2024-12-31 5:00 ` Aiqun(Maria) Yu
2024-12-31 7:44 ` Pengyu Luo
2025-01-01 11:27 ` Pengyu Luo
0 siblings, 2 replies; 52+ messages in thread
From: Aiqun(Maria) Yu @ 2024-12-31 5:00 UTC (permalink / raw)
To: Pengyu Luo
Cc: andersson, bryan.odonoghue, conor+dt, devicetree,
dmitry.baryshkov, gregkh, hdegoede, heikki.krogerus,
ilpo.jarvinen, konradybcio, krzk+dt, linux-arm-msm, linux-kernel,
linux-pm, linux-usb, nikita, platform-driver-x86, robh, sre
On 12/30/2024 6:44 PM, Pengyu Luo wrote:
> 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:
[...]
>>> + 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)
Could you please share the exact code snippet that is being referenced?
I'm a bit confused because it doesn't seem to align with the current
logic, which doesn't have read operations within the same mutex lock. I
also want to understand the background and necessity of the sleep function.
>
>>> +
>>> + mutex_unlock(&ec->lock);
>>> +
>>> + return *resp;
>>> +}
>>> +
>>> +/* -------------------------------------------------------------------------- */
>>> +/* Common API */
[...]
>>> + 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.
>
Perhaps you didn't get the "arg..." magic here. A single definition is
sufficient for all sizes.
>> 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
I believe that the consideration of namespace collisions is a valid concern.
Some examples can be like have a naming pattern as well:
/*To have a name pattern to reflect the size like reg0/reg1/reg2*/
#define REQ(variable_name, size, data_0, data_1, args...) \
u8 ##variable_name[REQ_HDR_SIZE + size] = {data_0, data_1, size, args};
/*u8 req1[REQ_HDR_SIZE + 1] = {0x02, EC_READ, 1, };*/
REQ(req, 1, 0x02, EC_READ);
/*u8 req2[REQ_HDR_SIZE + 2] = {0x02, 0x68, 2, 3, 0x5a}; */
REQ(req, 2, 0x02, 0x68, 3, 0x5a);
Please note that this is just an example and a suggestion to avoid the
current manual variable pattern setting. The final decision still
requires the current maintainers' agreement.
--
Thx and BRs,
Aiqun(Maria) Yu
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver
2024-12-31 5:00 ` Aiqun(Maria) Yu
@ 2024-12-31 7:44 ` Pengyu Luo
2024-12-31 11:09 ` Bryan O'Donoghue
2025-01-03 5:38 ` Dmitry Baryshkov
2025-01-01 11:27 ` Pengyu Luo
1 sibling, 2 replies; 52+ messages in thread
From: Pengyu Luo @ 2024-12-31 7:44 UTC (permalink / raw)
To: quic_aiquny
Cc: andersson, bryan.odonoghue, conor+dt, devicetree,
dmitry.baryshkov, gregkh, hdegoede, heikki.krogerus,
ilpo.jarvinen, konradybcio, krzk+dt, linux-arm-msm, linux-kernel,
linux-pm, linux-usb, mitltlatltl, nikita, platform-driver-x86,
robh, sre
On Tue, Dec 31, 2024 at 1:00 PM Aiqun(Maria) Yu <quic_aiquny@quicinc.com> wrote:
> On 12/30/2024 6:44 PM, Pengyu Luo wrote:
> > 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:
> [...]
> >>> + 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)
>
> Could you please share the exact code snippet that is being referenced?
> I'm a bit confused because it doesn't seem to align with the current
> logic, which doesn't have read operations within the same mutex lock. I
> also want to understand the background and necessity of the sleep function.
>
I mentioned I rearranged it to optimize it. In a EC transaction,
write sleep read => write read sleep, in this way, we sleep once a
transaction.
Please search
'device name + acpi table' on the internet, someone dumped it and uploaded
it, in SSDT, check ECCD. I am not sure if huawei allows users to dump it.
So I don't provide it here.
> >
> >>> +
> >>> + mutex_unlock(&ec->lock);
> >>> +
> >>> + return *resp;
> >>> +}
> >>> +
> >>> +/* -------------------------------------------------------------------------- */
> >>> +/* Common API */
> [...]
> >>> + 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.
> >
>
> Perhaps you didn't get the "arg..." magic here. A single definition is
> sufficient for all sizes.
>
You were talking using a macro to inline the varadic magic sequences, I was
talking defining macro for every constant number. If so, I got you now.
> >> 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
>
> I believe that the consideration of namespace collisions is a valid concern.
>
> Some examples can be like have a naming pattern as well:
> /*To have a name pattern to reflect the size like reg0/reg1/reg2*/
> #define REQ(variable_name, size, data_0, data_1, args...) \
> u8 ##variable_name[REQ_HDR_SIZE + size] = {data_0, data_1, size, args};
>
> /*u8 req1[REQ_HDR_SIZE + 1] = {0x02, EC_READ, 1, };*/
> REQ(req, 1, 0x02, EC_READ);
>
> /*u8 req2[REQ_HDR_SIZE + 2] = {0x02, 0x68, 2, 3, 0x5a}; */
> REQ(req, 2, 0x02, 0x68, 3, 0x5a);
>
> Please note that this is just an example and a suggestion to avoid the
> current manual variable pattern setting. The final decision still
> requires the current maintainers' agreement.
>
The main point I am against is hiding the data type, in some functions,
later we assign req[some_offset] = val; That makes things really weird.
I prefer to define all magic sequences, like
#define MAGIC_SEQ_1 {0x02, EC_READ, 1, 0} /* padding with 0 */
#define MAGIC_SEQ_2 {0x02, 0x68, 2, 3, 0x5a}
Gathering them makes things easy to manage, but I doubt if any source
file in Linux kernel doing it like this.
Another one alternative,
#define INLINE(REG0, REG1, SIZE) \
{ REG0, REG1, SIZE, [3 ... 2 + SIZE] = 0} /* GCC extension */
/* or just */
{ REG0, REG1, SIZE, [2 + SIZE] = 0}
u8 req[] = INLINE(0x02, 0x68, 2); /* Creating it */
/* not sure if we can make tricks to initial this in macro */
initial(req, 3, 0x5a); /* initial it */
Best wishes,
Pengyu
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver
2024-12-31 7:44 ` Pengyu Luo
@ 2024-12-31 11:09 ` Bryan O'Donoghue
2025-01-03 5:38 ` Dmitry Baryshkov
1 sibling, 0 replies; 52+ messages in thread
From: Bryan O'Donoghue @ 2024-12-31 11:09 UTC (permalink / raw)
To: Pengyu Luo, quic_aiquny
Cc: andersson, conor+dt, devicetree, dmitry.baryshkov, gregkh,
hdegoede, heikki.krogerus, ilpo.jarvinen, konradybcio, krzk+dt,
linux-arm-msm, linux-kernel, linux-pm, linux-usb, nikita,
platform-driver-x86, robh, sre
On 31/12/2024 07:44, Pengyu Luo wrote:
> Please search
> 'device name + acpi table' on the internet, someone dumped it and uploaded
> it, in SSDT, check ECCD. I am not sure if huawei allows users to dump it.
> So I don't provide it here.
There's a repository of ACPI dumps here:
https://github.com/aarch64-laptops/build/tree/master/misc
including the Huawei Matebook E - not sure if that should include the
"Matebook E Go"
https://github.com/aarch64-laptops/build/tree/master/misc/huawei-matebooke-2019
You could provide it there.
---
bod
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver
2024-12-31 7:44 ` Pengyu Luo
2024-12-31 11:09 ` Bryan O'Donoghue
@ 2025-01-03 5:38 ` Dmitry Baryshkov
2025-01-03 7:19 ` Pengyu Luo
1 sibling, 1 reply; 52+ messages in thread
From: Dmitry Baryshkov @ 2025-01-03 5:38 UTC (permalink / raw)
To: Pengyu Luo
Cc: quic_aiquny, andersson, bryan.odonoghue, conor+dt, devicetree,
gregkh, hdegoede, heikki.krogerus, ilpo.jarvinen, konradybcio,
krzk+dt, linux-arm-msm, linux-kernel, linux-pm, linux-usb, nikita,
platform-driver-x86, robh, sre
On Tue, Dec 31, 2024 at 03:44:36PM +0800, Pengyu Luo wrote:
> On Tue, Dec 31, 2024 at 1:00 PM Aiqun(Maria) Yu <quic_aiquny@quicinc.com> wrote:
> > On 12/30/2024 6:44 PM, Pengyu Luo wrote:
> > > 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:
> > [...]
> > >>> + 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)
> >
> > Could you please share the exact code snippet that is being referenced?
> > I'm a bit confused because it doesn't seem to align with the current
> > logic, which doesn't have read operations within the same mutex lock. I
> > also want to understand the background and necessity of the sleep function.
> >
>
> I mentioned I rearranged it to optimize it. In a EC transaction,
> write sleep read => write read sleep, in this way, we sleep once a
> transaction.
Sleeping between write and read is logical: it provides EC some time to
respond. Sleeping after read is complete doesn't seem to have any
reason.
>
> Please search
> 'device name + acpi table' on the internet, someone dumped it and uploaded
> it, in SSDT, check ECCD. I am not sure if huawei allows users to dump it.
> So I don't provide it here.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver
2025-01-03 5:38 ` Dmitry Baryshkov
@ 2025-01-03 7:19 ` Pengyu Luo
0 siblings, 0 replies; 52+ messages in thread
From: Pengyu Luo @ 2025-01-03 7:19 UTC (permalink / raw)
To: dmitry.baryshkov
Cc: andersson, bryan.odonoghue, conor+dt, devicetree, gregkh,
hdegoede, heikki.krogerus, ilpo.jarvinen, konradybcio, krzk+dt,
linux-arm-msm, linux-kernel, linux-pm, linux-usb, mitltlatltl,
nikita, platform-driver-x86, quic_aiquny, robh, sre
On Fri, Jan 3, 2025 at 1:38 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> On Tue, Dec 31, 2024 at 03:44:36PM +0800, Pengyu Luo wrote:
> > On Tue, Dec 31, 2024 at 1:00 PM Aiqun(Maria) Yu <quic_aiquny@quicinc.com> wrote:
> > > On 12/30/2024 6:44 PM, Pengyu Luo wrote:
> > > > 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:
> > > [...]
> > > >>> + 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)
> > >
> > > Could you please share the exact code snippet that is being referenced?
> > > I'm a bit confused because it doesn't seem to align with the current
> > > logic, which doesn't have read operations within the same mutex lock. I
> > > also want to understand the background and necessity of the sleep function.
> > >
> >
> > I mentioned I rearranged it to optimize it. In a EC transaction,
> > write sleep read => write read sleep, in this way, we sleep once a
> > transaction.
>
> Sleeping between write and read is logical: it provides EC some time to
> respond. Sleeping after read is complete doesn't seem to have any
> reason.
>
OK, if you are interested, I explain this in details
First, EC transaction in acpi on this device is doing like
======== this transaction =========
lock
...
write
...
sleep
...
read
...
release
======== this transaction =========
When there are intensive transactions, another sleep is added in
======== this transaction =========
...
======== this transaction =========
...
sleep
...
======== next transaction =========
...
======== next transaction =========
Can we eliminate this? I am not sure, I have not tested it.
Generally, the code in acpi is terrible, it can just do the jobs, so I did
some changes and tested.
The process(reading after writing) and data structure(cmd, count, data...)
are very similar to I2C_FUNC_SMBUS_BLOCK_PROC_CALL(see [1]), see also ACPI
Specification 13.3.7. (It like this in acpi, BUFF = VREG = BUFF)
So I tried to send two messages in one shot without a break. Why not using
a smbus API? Qualcomm I2C driver in kernel does not support it
(Fall back to i2c_smbus_xfer_emulated).
Why not using a I2C Block Read/Write API?
One transaction with this api would send 3 messages, and return the wrong
status in return buffer.
Write:
i2c_smbus_write_i2c_block_data(mcmd, ilen+2, {scmd, ilen, ibuf})
i2c_msg = {
.len = ilen + 3,
.buf = {mcmd, scmd, ilen, ibuf}
}
Read:
i2c_smbus_read_i2c_block_data(mcmd, olen)
i2c_msg[0] = {
.len = 1,
.buf = {mcmd},
};
i2c_msg[1] = {
.flags = I2C_M_RD,
.len = olen,
.buf = {}, /* the first byte return is wrong */
};
Best wishes,
Pengyu
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 2/5] platform: arm64: add Huawei Matebook E Go (sc8280xp) EC driver
2024-12-31 5:00 ` Aiqun(Maria) Yu
2024-12-31 7:44 ` Pengyu Luo
@ 2025-01-01 11:27 ` Pengyu Luo
1 sibling, 0 replies; 52+ messages in thread
From: Pengyu Luo @ 2025-01-01 11:27 UTC (permalink / raw)
To: quic_aiquny, bryan.odonoghue
Cc: andersson, conor+dt, devicetree, dmitry.baryshkov, gregkh,
hdegoede, heikki.krogerus, ilpo.jarvinen, konradybcio, krzk+dt,
linux-arm-msm, linux-kernel, linux-pm, linux-usb, mitltlatltl,
nikita, platform-driver-x86, robh, sre
On Tue, Dec 31, 2024 at 1:00 PM Aiqun(Maria) Yu <quic_aiquny@quicinc.com> wrote:
> On 12/30/2024 6:44 PM, Pengyu Luo wrote:
> > 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:
> [...]
> >>> + 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)
>
> Could you please share the exact code snippet that is being referenced?
> I'm a bit confused because it doesn't seem to align with the current
> logic, which doesn't have read operations within the same mutex lock. I
> also want to understand the background and necessity of the sleep function.
>
> >
> >>> +
> >>> + mutex_unlock(&ec->lock);
> >>> +
> >>> + return *resp;
> >>> +}
> >>> +
> >>> +/* -------------------------------------------------------------------------- */
> >>> +/* Common API */
> [...]
> >>> + 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.
> >
>
> Perhaps you didn't get the "arg..." magic here. A single definition is
> sufficient for all sizes.
>
> >> 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
>
> I believe that the consideration of namespace collisions is a valid concern.
>
> Some examples can be like have a naming pattern as well:
> /*To have a name pattern to reflect the size like reg0/reg1/reg2*/
> #define REQ(variable_name, size, data_0, data_1, args...) \
> u8 ##variable_name[REQ_HDR_SIZE + size] = {data_0, data_1, size, args};
>
> /*u8 req1[REQ_HDR_SIZE + 1] = {0x02, EC_READ, 1, };*/
> REQ(req, 1, 0x02, EC_READ);
>
> /*u8 req2[REQ_HDR_SIZE + 2] = {0x02, 0x68, 2, 3, 0x5a}; */
> REQ(req, 2, 0x02, 0x68, 3, 0x5a);
>
> Please note that this is just an example and a suggestion to avoid the
> current manual variable pattern setting. The final decision still
> requires the current maintainers' agreement.
>
I am gonna do this, Aiqun(Maria), Bryan, any suggestions?
/*
* 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
/*
* for rx, data sequences are arranged as
* {status, data_len(unreliable), data_seq}
*/
#define RESP_HDR_SIZE 2
#define MKREQ(REG0, REG1, SIZE, ...) \
{ \
/* ## will remove comma when no __VA_ARGS__ */ \
REG0, REG1, SIZE, ## __VA_ARGS__, \
/* make sure len(pkt[3:]) >= SIZE */ \
[3 + SIZE] = 0, \
}
#define MKRESP(SIZE) \
{ \
[RESP_HDR_SIZE + SIZE - 1] = 0, \
}
static inline void refill_req(u8 *dest, const u8 *src, size_t size)
{
int i;
for (i = 0; i < size; ++i)
dest[REQ_HDR_SIZE + i] = src[i];
}
static inline void extr_resp(u8 *dest, const u8 *src, size_t size)
{
int i;
for (i = 0; i < size; ++i)
dest[i] = src[RESP_HDR_SIZE + i];
}
[...]
example:
int gaokun_ec_psy_multi_read(struct gaokun_ec *ec, u8 reg,
size_t resp_len, u8 *resp)
{
int i, ret;
u8 _resp[] = MKRESP(1);
u8 req[] = MKREQ(0x02, EC_READ, 1);
for (i = 0; i < resp_len; ++i, ++reg) {
refill_req(req, ®, 1);
ret = gaokun_ec_read(ec, req, sizeof(_resp), _resp);
if (ret)
return ret;
extr_resp(&resp[i], _resp, 1);
}
return 0;
}
Best wishes,
Pengyu
^ permalink raw reply [flat|nested] 52+ messages in thread