| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <402b261b-e51d-7121-1e13-b1bc1f5d40f5@linux.intel.com>
Date: Mon, 13 Jan 2025 20:56:28 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Pengyu Luo <mitltlatltl@...il.com>
cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konradybcio@...nel.org>,
Hans de Goede <hdegoede@...hat.com>,
Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
Sebastian Reichel <sre@...nel.org>,
Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jean Delvare <jdelvare@...e.com>, Guenter Roeck <linux@...ck-us.net>,
devicetree@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
linux-arm-msm@...r.kernel.org, platform-driver-x86@...r.kernel.org,
linux-pm@...r.kernel.org, linux-usb@...r.kernel.org,
linux-hwmon@...r.kernel.org
Subject: Re: [PATCH v3 2/6] platform: arm64: add Huawei Matebook E Go EC
driver
On Tue, 14 Jan 2025, Pengyu Luo wrote:
> There are three variants of which Huawei released the first two
> simultaneously.
>
> Huawei Matebook E Go LTE(sc8180x), codename seems to be gaokun2.
> Huawei Matebook E Go(sc8280xp@...GHz), codename must be gaokun3. (see [1])
> Huawei Matebook E Go 2023(sc8280xp@...9GHz), codename should be also gaokun3.
>
> Adding support for the latter two variants for now, this driver should
> also work for the sc8180x variant according to acpi table files, but I
> don't have the device to test yet.
>
> Different from other Qualcomm Snapdragon sc8280xp based machines, the
> Huawei Matebook E Go uses an embedded controller while others use
> a system called PMIC GLink. This embedded controller can be used to
> perform a set of various functions, including, but not limited to:
>
> - Battery and charger monitoring;
> - Charge control and smart charge;
> - Fn_lock settings;
> - Tablet lid status;
> - Temperature sensors;
> - USB Type-C notifications (ports orientation, DP alt mode HPD);
> - USB Type-C PD (according to observation, up to 48w).
>
> Add a driver for the EC which creates devices for UCSI and power supply
> devices.
>
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=219645
>
> Signed-off-by: Pengyu Luo <mitltlatltl@...il.com>
> ---
> MAINTAINERS | 9 +
> drivers/platform/arm64/Kconfig | 21 +
> drivers/platform/arm64/Makefile | 1 +
> drivers/platform/arm64/huawei-gaokun-ec.c | 841 ++++++++++++++++++
> .../linux/platform_data/huawei-gaokun-ec.h | 80 ++
> 5 files changed, 952 insertions(+)
> create mode 100644 drivers/platform/arm64/huawei-gaokun-ec.c
> create mode 100644 include/linux/platform_data/huawei-gaokun-ec.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 520612b3d..0f9e0390a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10691,6 +10691,15 @@ S: Maintained
> F: Documentation/networking/device_drivers/ethernet/huawei/hinic.rst
> F: drivers/net/ethernet/huawei/hinic/
>
> +HUAWEI MATEBOOK E GO EMBEDDED CONTROLLER DRIVER
> +M: Pengyu Luo <mitltlatltl@...il.com>
> +S: Maintained
> +F: Documentation/devicetree/bindings/platform/huawei,gaokun-ec.yaml
> +F: drivers/platform/arm64/huawei-gaokun-ec.c
> +F: drivers/power/supply/huawei-gaokun-battery.c
> +F: drivers/usb/typec/ucsi/ucsi_huawei_gaokun.c
> +F: include/linux/platform_data/huawei-gaokun-ec.h
> +
> HUGETLB SUBSYSTEM
> M: Muchun Song <muchun.song@...ux.dev>
> L: linux-mm@...ck.org
> diff --git a/drivers/platform/arm64/Kconfig b/drivers/platform/arm64/Kconfig
> index f88395ea3..67dc7aa95 100644
> --- a/drivers/platform/arm64/Kconfig
> +++ b/drivers/platform/arm64/Kconfig
> @@ -33,6 +33,27 @@ 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 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 the Huawei Matebook E Go
> + which is a sc8280xp-based 2-in-1 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..46a99eba3 100644
> --- a/drivers/platform/arm64/Makefile
> +++ b/drivers/platform/arm64/Makefile
> @@ -6,4 +6,5 @@
> #
>
> obj-$(CONFIG_EC_ACER_ASPIRE1) += acer-aspire1-ec.o
> +obj-$(CONFIG_EC_HUAWEI_GAOKUN) += huawei-gaokun-ec.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..3cbecc81e
> --- /dev/null
> +++ b/drivers/platform/arm64/huawei-gaokun-ec.c
> @@ -0,0 +1,841 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * huawei-gaokun-ec - An EC driver for HUAWEI Matebook E Go
> + *
> + * reference: drivers/platform/arm64/acer-aspire1-ec.c
> + * drivers/platform/arm64/lenovo-yoga-c630.c
> + * drivers/platform/x86/huawei-wmi.c
What's the purpose of these? Should they rather be e.g. in the commit
message?
> + *
> + * Copyright (C) 2024 Pengyu Luo <mitltlatltl@...il.com>
-2025?
> + */
> +
> +#include <linux/auxiliary_bus.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/notifier.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_data/huawei-gaokun-ec.h>
> +#include <linux/version.h>
Why is version.h needed?
> +
> +#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_FN_LOCK_ON 0x5A
> +#define EC_FN_LOCK_OFF 0x55
> +
> +#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
For
> + * {master_cmd, slave_cmd, data_len, data_seq}
> + */
> +#define REQ_HDR_SIZE 3
> +#define INPUT_SIZE_OFFSET 2
> +#define REQ_LEN(req) (REQ_HDR_SIZE + req[INPUT_SIZE_OFFSET])
> +
> +/*
> + * for rx, data sequences are arranged as
For
> + * {status, data_len(unreliable), data_seq}
> + */
> +#define RESP_HDR_SIZE 2
> +
> +#define MKREQ(REG0, REG1, SIZE, ...) \
> +{ \
> + REG0, REG1, SIZE, \
> + /* ## will remove comma when SIZE is 0 */ \
> + ## __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)
> +{
> + memcpy(dest + REQ_HDR_SIZE, src, size);
> +}
> +
> +static inline void extr_resp(u8 *dest, const u8 *src, size_t size)
> +{
> + memcpy(dest, src + RESP_HDR_SIZE, size);
> +}
> +
> +struct gaokun_ec {
> + struct i2c_client *client;
> + struct mutex lock; /* EC transaction lock */
> + struct blocking_notifier_head notifier_list;
> + struct device *hwmon_dev;
> + 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_LEN(req),
> + .buf = req,
> + }, {
> + .addr = client->addr,
> + .flags = client->flags | I2C_M_RD,
> + .len = resp_len,
> + .buf = resp,
> + },
> + };
> +
> + mutex_lock(&ec->lock);
Use guard().
> +
> + i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> + usleep_range(2000, 2500); /* have a break, acpi did this */
ACPI
> +
> + mutex_unlock(&ec->lock);
> +
> + return *resp ? -EIO : 0;
> +}
> +
> +/* -------------------------------------------------------------------------- */
> +/* 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
> + *
> + * Return: 0 on success or negative error code.
> + *
> + * This function is used to read data after writing a magic sequence to EC.
> + * All EC operations depend on this function.
> + *
> + * Huawei uses magic sequences everywhere to complete various functions, all
> + * these sequences are passed to ECCD(a ACPI method which is quiet similar
> + * to gaokun_ec_request), there is no good abstraction to generalize these
> + * sequences, so just wrap it for now. Almost all magic sequences are kept
> + * in this file.
> + */
> +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
> + *
> + * Return: 0 on success or negative error code.
> + *
> + * This function has no big difference from gaokun_ec_read. When caller care
> + * only write status and no actual data are returned, then use it.
> + */
> +int gaokun_ec_write(struct gaokun_ec *ec, u8 *req)
> +{
> + u8 resp[] = MKRESP(0);
> +
> + 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[] = MKRESP(sizeof(*byte));
> +
> + ret = gaokun_ec_read(ec, req, sizeof(resp), resp);
> + extr_resp(byte, resp, sizeof(*byte));
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(gaokun_ec_read_byte);
> +
> +/**
> + * gaokun_ec_register_notify - Register a notifier callback for EC events.
> + * @ec: The gaokun_ec
> + * @nb: Notifier block pointer to register
> + *
> + * Return: 0 on success or negative error code.
> + */
> +int gaokun_ec_register_notify(struct gaokun_ec *ec, struct notifier_block *nb)
> +{
> + return blocking_notifier_chain_register(&ec->notifier_list, nb);
> +}
> +EXPORT_SYMBOL_GPL(gaokun_ec_register_notify);
> +
> +/**
> + * gaokun_ec_unregister_notify - Unregister notifier callback for EC events.
> + * @ec: The gaokun_ec
> + * @nb: Notifier block pointer to unregister
> + *
> + * Unregister a notifier callback that was previously registered with
> + * gaokun_ec_register_notify().
> + */
> +void gaokun_ec_unregister_notify(struct gaokun_ec *ec, struct notifier_block *nb)
> +{
> + blocking_notifier_chain_unregister(&ec->notifier_list, nb);
> +}
> +EXPORT_SYMBOL_GPL(gaokun_ec_unregister_notify);
> +
> +/* -------------------------------------------------------------------------- */
> +/* API For PSY */
> +
> +/**
> + * gaokun_ec_psy_multi_read - Read contiguous registers
> + * @ec: The gaokun_ec
> + * @reg: The start register
> + * @resp_len: The number of registers to be read
> + * @resp: Where the data are read to
> + *
> + * Return: 0 on success or negative error code.
> + */
> +int gaokun_ec_psy_multi_read(struct gaokun_ec *ec, u8 reg,
> + size_t resp_len, u8 *resp)
> +{
> + int i, ret;
> + u8 _resp[] = MKRESP(1);
> + u8 req[] = MKREQ(0x02, EC_READ, 1, 0);
> +
> + 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);
I'm not a big fan of having resp and _resp named variables.
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(gaokun_ec_psy_multi_read);
> +
> +/* Smart charge */
> +#define are_thresholds_valid(start, end) ((end != 0) && (start <= end) && (end <= 100))
Why this cannot be a static function?
> +
> +/**
> + * gaokun_ec_psy_get_smart_charge - read smart charge data from EC
> + * @ec: The gaokun_ec
> + * @data: Where the data are read to, (mode, delay, start, end)
> + *
> + * Return: 0 on success or negative error code.
> + */
> +int gaokun_ec_psy_get_smart_charge(struct gaokun_ec *ec,
> + u8 data[GAOKUN_SMART_CHARGE_DATA_SIZE])
> +{
> + /* GBCM */
> + u8 req[] = MKREQ(0x02, 0xE4, 0);
> + u8 resp[] = MKRESP(GAOKUN_SMART_CHARGE_DATA_SIZE);
> + int ret;
> +
> + ret = gaokun_ec_read(ec, req, sizeof(resp), resp);
> + if (ret)
> + return ret;
> +
> + extr_resp(data, resp, GAOKUN_SMART_CHARGE_DATA_SIZE);
So in the previous func you used _resp and resp as names, and here you
have resp and data. Any chance of being consistent in variable naming?
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(gaokun_ec_psy_get_smart_charge);
> +
> +/**
> + * gaokun_ec_psy_set_smart_charge - set smart charge data
> + * @ec: The gaokun_ec
> + * @data: The data are to be set
> + *
> + * Return: 0 on success or negative error code.
> + */
> +int gaokun_ec_psy_set_smart_charge(struct gaokun_ec *ec,
> + u8 data[GAOKUN_SMART_CHARGE_DATA_SIZE])
> +{
> + /* SBCM */
> + u8 req[] = MKREQ(0x02, 0XE3, GAOKUN_SMART_CHARGE_DATA_SIZE);
X -> x
> +
> + if (!are_thresholds_valid(data[2], data[3]))
> + return -EINVAL;
> +
> + refill_req(req, data, GAOKUN_SMART_CHARGE_DATA_SIZE);
> +
> + return gaokun_ec_write(ec, req);
> +}
> +EXPORT_SYMBOL_GPL(gaokun_ec_psy_set_smart_charge);
> +
> +/* Smart charge enable */
> +/**
> + * gaokun_ec_psy_get_smart_charge_enable - check if smart charge is enabled
> + * @ec: The gaokun_ec
> + * @on: The state
> + *
> + * Return: 0 on success or negative error code.
> + */
> +int gaokun_ec_psy_get_smart_charge_enable(struct gaokun_ec *ec, bool *on)
> +{
> + /* GBAC */
> + *on = 0; /* clear other 3 Bytes */
= false (as it's bool)
What that comment means??? The type is bool so what "3 Bytes" ???
> + return gaokun_ec_read_byte(ec, (u8 [])MKREQ(0x02, 0xE6, 0), (u8 *)on);
Don't cast bool * to u8 *.
Please cleanup the MKREQ mess out of this call like in the other
functions above.
> +}
> +EXPORT_SYMBOL_GPL(gaokun_ec_psy_get_smart_charge_enable);
> +
> +/**
> + * gaokun_ec_psy_set_smart_charge_enable - set smart charge state
> + * @ec: The gaokun_ec
> + * @on: The state
> + *
> + * Return: 0 on success or negative error code.
> + */
> +int gaokun_ec_psy_set_smart_charge_enable(struct gaokun_ec *ec, bool on)
> +{
> + /* SBAC */
> + return gaokun_ec_write(ec, (u8 [])MKREQ(0x02, 0xE5, 1, on));
Ditto.
> +}
> +EXPORT_SYMBOL_GPL(gaokun_ec_psy_set_smart_charge_enable);
> +
> +/* -------------------------------------------------------------------------- */
> +/* API For UCSI */
> +
> +/**
> + * gaokun_ec_ucsi_read - read ucsi data from EC, like CCI, MSGI
> + * @ec: The gaokun_ec
The gaokun_ec structure
> + * @resp: Where the data are read to
Not a very good kernel doc parameter description.
> + *
> + * Return: 0 on success or negative error code.
> + *
> + * UCSI subdriver uses this function to read CCI and MSGI.
> + */
> +int gaokun_ec_ucsi_read(struct gaokun_ec *ec,
> + u8 resp[GAOKUN_UCSI_READ_SIZE])
> +{
> + u8 req[] = MKREQ(0x03, 0xD5, 0);
> + u8 _resp[] = MKRESP(GAOKUN_UCSI_READ_SIZE);
> + int ret;
> +
> + ret = gaokun_ec_read(ec, req, sizeof(_resp), _resp);
> + if (ret)
> + return ret;
> +
> + extr_resp(resp, _resp, GAOKUN_UCSI_READ_SIZE);
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(gaokun_ec_ucsi_read);
> +
> +/**
> + * gaokun_ec_ucsi_write - write ucsi data to EC
> + * @ec: The gaokun_ec
> + * @req: The command
If you'd call it cmd, then you don't have req & req_ problem.
> + *
> + * Return: 0 on success or negative error code.
> + *
> + * UCSI subdriver uses this function to write control command.
This should be before Return: and perhaps it could be reworded such that
it describes what the function does, eg.:
Write control command (used by UCSI subdriver).
> + */
> +int gaokun_ec_ucsi_write(struct gaokun_ec *ec,
> + const u8 req[GAOKUN_UCSI_WRITE_SIZE])
> +{
> + u8 _req[] = MKREQ(0x03, 0xD4, GAOKUN_UCSI_WRITE_SIZE);
> +
> + refill_req(_req, req, GAOKUN_UCSI_WRITE_SIZE);
> + return gaokun_ec_write(ec, _req);
> +}
> +EXPORT_SYMBOL_GPL(gaokun_ec_ucsi_write);
> +
> +/**
> + * gaokun_ec_ucsi_get_reg - get ucsi register from EC
> + * @ec: The gaokun_ec
> + * @ureg: The gaokun ucsi register
> + *
> + * Return: 0 on success or negative error code.
> + *
> + * UCSI subdriver uses this function to get ucsi register from EC.
> + */
> +int gaokun_ec_ucsi_get_reg(struct gaokun_ec *ec,
> + struct gaokun_ucsi_reg *ureg)
Fits to one line.
> +{
> + u8 req[] = MKREQ(0x03, 0xD3, 0);
> + u8 _resp[] = MKRESP(UCSI_REG_SIZE);
> + int ret;
> +
> + ret = gaokun_ec_read(ec, req, sizeof(_resp), _resp);
> + if (ret)
> + return ret;
> +
> + extr_resp((u8 *)ureg, _resp, UCSI_REG_SIZE);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(gaokun_ec_ucsi_get_reg);
> +
> +/**
> + * gaokun_ec_ucsi_pan_ack - ack pin assignment notifications from EC
> + * @ec: The gaokun_ec
> + * @port_id: The id of port which just received and handled the notifications
@port_id: The port id receiving and handling the notifications
> + *
> + * Return: 0 on success or negative error code.
> + *
> + * UCSI subdriver uses this function to ack PANs from EC.
Place before Return, I suggest similar rephrasing as mentioned above.
> + */
> +int gaokun_ec_ucsi_pan_ack(struct gaokun_ec *ec, int port_id)
> +{
> + u8 req[] = MKREQ(0x03, 0xD2, 1);
> + u8 data = 1 << port_id;
> +
> + if (port_id == GAOKUN_UCSI_NO_PORT_UPDATE)
> + data = 0;
> +
> + refill_req(req, &data, 1);
> +
> + return gaokun_ec_write(ec, req);
> +}
> +EXPORT_SYMBOL_GPL(gaokun_ec_ucsi_pan_ack);
> +
> +/* -------------------------------------------------------------------------- */
> +/* EC Sysfs */
> +
> +/* Fn lock */
> +static int gaokun_ec_get_fn_lock(struct gaokun_ec *ec, bool *on)
> +{
> + /* GFRS */
> + u8 req[] = MKREQ(0x02, 0x6B, 0);
Does that random acronym map to one of the literal? In which case a define
would be more useful than a comment. (You seem to have a few similar
comments preceeding the req definitions)
> + int ret;
> + u8 val;
> +
> + ret = gaokun_ec_read_byte(ec, req, &val);
> + if (ret)
> + return ret;
> +
> + if (val == EC_FN_LOCK_ON)
> + *on = true;
> + else if (val == EC_FN_LOCK_OFF)
> + *on = false;
> + else
> + return -EIO;
> +
> + return 0;
> +}
> +
> +static int gaokun_ec_set_fn_lock(struct gaokun_ec *ec, bool on)
> +{
> + /* SFRS */
> + u8 req[] = MKREQ(0x02, 0x6C, 1);
> + u8 data;
> +
> + if (on)
> + data = EC_FN_LOCK_ON;
> + else
> + data = EC_FN_LOCK_OFF;
You could do:
u8 req[] = MKREQ(0x02, 0x6C, 1, on ? EC_FN_LOCK_ON : EC_FN_LOCK_OFF);
> +
> + refill_req(req, &data, 1);
> +
> + return gaokun_ec_write(ec, req);
> +}
> +
> +static ssize_t fn_lock_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct gaokun_ec *ec = dev_get_drvdata(dev);
> + bool on;
> + int ret;
> +
> + ret = gaokun_ec_get_fn_lock(ec, &on);
> + if (ret)
> + return ret;
> +
> + return sysfs_emit(buf, "%d\n", on);
> +}
> +
> +static ssize_t fn_lock_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t size)
> +{
> + struct gaokun_ec *ec = dev_get_drvdata(dev);
> + bool on;
> + int ret;
> +
> + if (kstrtobool(buf, &on))
> + return -EINVAL;
> +
> + ret = gaokun_ec_set_fn_lock(ec, on);
> + if (ret)
> + return ret;
> +
> + return size;
> +}
> +
> +static DEVICE_ATTR_RW(fn_lock);
> +
> +static struct attribute *gaokun_ec_attrs[] = {
> + &dev_attr_fn_lock.attr,
> + NULL,
> +};
> +ATTRIBUTE_GROUPS(gaokun_ec);
> +
> +/* Thermal Zone HwMon */
> +/* Range from 0 to 0x2C, partial valid */
partially?
> +static const u8 temp_reg[20] = {0x05, 0x07, 0x08, 0x0E, 0x0F, 0x12, 0x15, 0x1E,
> + 0x1F, 0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26,
> + 0x27, 0x28, 0x29, 0x2A};
> +
> +static int gaokun_ec_get_temp(struct gaokun_ec *ec, u8 idx, int *temp)
> +{
> + /* GTMP */
> + u8 req[] = MKREQ(0x02, 0x61, 1, temp_reg[idx]);
> + u8 resp[] = MKRESP(sizeof(__le16));
> + __le16 tmp;
> + int ret;
> +
> + ret = gaokun_ec_read(ec, req, sizeof(resp), resp);
> + if (ret)
> + return ret;
> +
> + extr_resp((u8 *)&tmp, resp, sizeof(tmp));
> + *temp = le16_to_cpu(tmp) * 100; /* convert to HwMon's unit */
extr_resp() does memcpy() but there should be no need to copy anything
here. You just want to have __le16 pointer of the response data data.
> + return 0;
> +}
> +
> +static ssize_t get_ec_tz_temp(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct gaokun_ec *ec = dev_get_drvdata(dev);
> + int idx, ret, temp;
> +
> + idx = (to_sensor_dev_attr(attr))->index - 1;
Extra parenthesis.
> + ret = gaokun_ec_get_temp(ec, idx, &temp);
> + if (ret)
> + return ret;
> +
> + return sysfs_emit(buf, "%d\n", temp);
> +}
> +
> +static ssize_t ec_tz_temp_label(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + int idx = (to_sensor_dev_attr(attr))->index - 1;
Ditto.
> +
> + return sysfs_emit(buf, "EC Thermal Zone %2d Temperature\n", idx);
> +}
> +
> +static SENSOR_DEVICE_ATTR(temp1_input, 0444, get_ec_tz_temp, NULL, 1);
> +static SENSOR_DEVICE_ATTR(temp1_label, 0444, ec_tz_temp_label, NULL, 1);
> +static SENSOR_DEVICE_ATTR(temp2_input, 0444, get_ec_tz_temp, NULL, 2);
> +static SENSOR_DEVICE_ATTR(temp2_label, 0444, ec_tz_temp_label, NULL, 2);
> +static SENSOR_DEVICE_ATTR(temp3_input, 0444, get_ec_tz_temp, NULL, 3);
> +static SENSOR_DEVICE_ATTR(temp3_label, 0444, ec_tz_temp_label, NULL, 3);
> +static SENSOR_DEVICE_ATTR(temp4_input, 0444, get_ec_tz_temp, NULL, 4);
> +static SENSOR_DEVICE_ATTR(temp4_label, 0444, ec_tz_temp_label, NULL, 4);
> +static SENSOR_DEVICE_ATTR(temp5_input, 0444, get_ec_tz_temp, NULL, 5);
> +static SENSOR_DEVICE_ATTR(temp5_label, 0444, ec_tz_temp_label, NULL, 5);
> +static SENSOR_DEVICE_ATTR(temp6_input, 0444, get_ec_tz_temp, NULL, 6);
> +static SENSOR_DEVICE_ATTR(temp6_label, 0444, ec_tz_temp_label, NULL, 6);
> +static SENSOR_DEVICE_ATTR(temp7_input, 0444, get_ec_tz_temp, NULL, 7);
> +static SENSOR_DEVICE_ATTR(temp7_label, 0444, ec_tz_temp_label, NULL, 7);
> +static SENSOR_DEVICE_ATTR(temp8_input, 0444, get_ec_tz_temp, NULL, 8);
> +static SENSOR_DEVICE_ATTR(temp8_label, 0444, ec_tz_temp_label, NULL, 8);
> +static SENSOR_DEVICE_ATTR(temp9_input, 0444, get_ec_tz_temp, NULL, 9);
> +static SENSOR_DEVICE_ATTR(temp9_label, 0444, ec_tz_temp_label, NULL, 9);
> +static SENSOR_DEVICE_ATTR(temp10_input, 0444, get_ec_tz_temp, NULL, 10);
> +static SENSOR_DEVICE_ATTR(temp10_label, 0444, ec_tz_temp_label, NULL, 10);
> +static SENSOR_DEVICE_ATTR(temp11_input, 0444, get_ec_tz_temp, NULL, 11);
> +static SENSOR_DEVICE_ATTR(temp11_label, 0444, ec_tz_temp_label, NULL, 11);
> +static SENSOR_DEVICE_ATTR(temp12_input, 0444, get_ec_tz_temp, NULL, 12);
> +static SENSOR_DEVICE_ATTR(temp12_label, 0444, ec_tz_temp_label, NULL, 12);
> +static SENSOR_DEVICE_ATTR(temp13_input, 0444, get_ec_tz_temp, NULL, 13);
> +static SENSOR_DEVICE_ATTR(temp13_label, 0444, ec_tz_temp_label, NULL, 13);
> +static SENSOR_DEVICE_ATTR(temp14_input, 0444, get_ec_tz_temp, NULL, 14);
> +static SENSOR_DEVICE_ATTR(temp14_label, 0444, ec_tz_temp_label, NULL, 14);
> +static SENSOR_DEVICE_ATTR(temp15_input, 0444, get_ec_tz_temp, NULL, 15);
> +static SENSOR_DEVICE_ATTR(temp15_label, 0444, ec_tz_temp_label, NULL, 15);
> +static SENSOR_DEVICE_ATTR(temp16_input, 0444, get_ec_tz_temp, NULL, 16);
> +static SENSOR_DEVICE_ATTR(temp16_label, 0444, ec_tz_temp_label, NULL, 16);
> +static SENSOR_DEVICE_ATTR(temp17_input, 0444, get_ec_tz_temp, NULL, 17);
> +static SENSOR_DEVICE_ATTR(temp17_label, 0444, ec_tz_temp_label, NULL, 17);
> +static SENSOR_DEVICE_ATTR(temp18_input, 0444, get_ec_tz_temp, NULL, 18);
> +static SENSOR_DEVICE_ATTR(temp18_label, 0444, ec_tz_temp_label, NULL, 18);
> +static SENSOR_DEVICE_ATTR(temp19_input, 0444, get_ec_tz_temp, NULL, 19);
> +static SENSOR_DEVICE_ATTR(temp19_label, 0444, ec_tz_temp_label, NULL, 19);
> +static SENSOR_DEVICE_ATTR(temp20_input, 0444, get_ec_tz_temp, NULL, 20);
> +static SENSOR_DEVICE_ATTR(temp20_label, 0444, ec_tz_temp_label, NULL, 20);
> +
> +static struct attribute *gaokun_ec_hwmon_attrs[] = {
> + &sensor_dev_attr_temp1_input.dev_attr.attr,
> + &sensor_dev_attr_temp1_label.dev_attr.attr,
> + &sensor_dev_attr_temp2_input.dev_attr.attr,
> + &sensor_dev_attr_temp2_label.dev_attr.attr,
> + &sensor_dev_attr_temp3_input.dev_attr.attr,
> + &sensor_dev_attr_temp3_label.dev_attr.attr,
> + &sensor_dev_attr_temp4_input.dev_attr.attr,
> + &sensor_dev_attr_temp4_label.dev_attr.attr,
> + &sensor_dev_attr_temp5_input.dev_attr.attr,
> + &sensor_dev_attr_temp5_label.dev_attr.attr,
> + &sensor_dev_attr_temp6_input.dev_attr.attr,
> + &sensor_dev_attr_temp6_label.dev_attr.attr,
> + &sensor_dev_attr_temp7_input.dev_attr.attr,
> + &sensor_dev_attr_temp7_label.dev_attr.attr,
> + &sensor_dev_attr_temp8_input.dev_attr.attr,
> + &sensor_dev_attr_temp8_label.dev_attr.attr,
> + &sensor_dev_attr_temp9_input.dev_attr.attr,
> + &sensor_dev_attr_temp9_label.dev_attr.attr,
> + &sensor_dev_attr_temp10_input.dev_attr.attr,
> + &sensor_dev_attr_temp10_label.dev_attr.attr,
> + &sensor_dev_attr_temp11_input.dev_attr.attr,
> + &sensor_dev_attr_temp11_label.dev_attr.attr,
> + &sensor_dev_attr_temp12_input.dev_attr.attr,
> + &sensor_dev_attr_temp12_label.dev_attr.attr,
> + &sensor_dev_attr_temp13_input.dev_attr.attr,
> + &sensor_dev_attr_temp13_label.dev_attr.attr,
> + &sensor_dev_attr_temp14_input.dev_attr.attr,
> + &sensor_dev_attr_temp14_label.dev_attr.attr,
> + &sensor_dev_attr_temp15_input.dev_attr.attr,
> + &sensor_dev_attr_temp15_label.dev_attr.attr,
> + &sensor_dev_attr_temp16_input.dev_attr.attr,
> + &sensor_dev_attr_temp16_label.dev_attr.attr,
> + &sensor_dev_attr_temp17_input.dev_attr.attr,
> + &sensor_dev_attr_temp17_label.dev_attr.attr,
> + &sensor_dev_attr_temp18_input.dev_attr.attr,
> + &sensor_dev_attr_temp18_label.dev_attr.attr,
> + &sensor_dev_attr_temp19_input.dev_attr.attr,
> + &sensor_dev_attr_temp19_label.dev_attr.attr,
> + &sensor_dev_attr_temp20_input.dev_attr.attr,
> + &sensor_dev_attr_temp20_label.dev_attr.attr,
> + NULL
> +};
> +ATTRIBUTE_GROUPS(gaokun_ec_hwmon);
> +
> +/* -------------------------------------------------------------------------- */
> +/* Modern Standby */
> +
> +static int gaokun_ec_suspend(struct device *dev)
> +{
> + struct gaokun_ec *ec = dev_get_drvdata(dev);
> + u8 req[] = MKREQ(0x02, 0xB2, 1, 0xDB);
> + int ret;
> +
> + if (ec->suspended)
> + return 0;
> +
> + ret = gaokun_ec_write(ec, req);
> +
> + if (ret)
Don't put empty line between call and its error handling.
> + 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[] = MKREQ(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 need time to resume */
So you are stacking delays in gaokun_ec_request() and another here.
> + };
> +
> + 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[] = MKREQ(EC_EVENT, EC_QUERY, 0);
Great, here you have named them. Could you name all of the other literals
too, please.
> + 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, "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");
IRQ
> +
> + ec->hwmon_dev = hwmon_device_register_with_groups(dev, "gaokun_ec_hwmon",
> + ec, gaokun_ec_hwmon_groups);
> + if (IS_ERR(ec->hwmon_dev)) {
> + dev_err(dev, "Failed to register hwmon device\n");
> + return PTR_ERR(ec->hwmon_dev);
> + }
> +
> + return 0;
> +}
> +
> +static void gaokun_ec_remove(struct i2c_client *client)
> +{
> + struct gaokun_ec *ec = i2c_get_clientdata(client);
> + hwmon_device_unregister(ec->hwmon_dev);
> +}
> +
> +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,gaokun3-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,
> + .dev_groups = gaokun_ec_groups,
> + },
> + .probe = gaokun_ec_probe,
> + .remove = gaokun_ec_remove,
> + .id_table = gaokun_ec_id,
> +};
> +module_i2c_driver(gaokun_ec_driver);
> +
> +MODULE_DESCRIPTION("HUAWEI Matebook E Go EC driver");
> +MODULE_AUTHOR("Pengyu Luo <mitltlatltl@...il.com>");
> +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..dfd177bd9
> --- /dev/null
> +++ b/include/linux/platform_data/huawei-gaokun-ec.h
> @@ -0,0 +1,80 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Huawei Matebook E Go Embedded Controller
> + *
> + * Copyright (C) 2024 Pengyu Luo <mitltlatltl@...il.com>
-2025 ?
> + */
> +
> +#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_UCSI_NO_PORT_UPDATE (-1)
> +
> +#define GAOKUN_SMART_CHARGE_DATA_SIZE 4 /* mode, delay, start, end */
> +
> +/* -------------------------------------------------------------------------- */
> +
> +struct gaokun_ec;
> +struct gaokun_ucsi_reg;
> +struct notifier_block;
> +
> +#define GAOKUN_MOD_NAME "huawei_gaokun_ec"
> +#define GAOKUN_DEV_PSY "psy"
> +#define GAOKUN_DEV_UCSI "ucsi"
So you defined these here but used literal strings for aux dev init calls?
> +/* -------------------------------------------------------------------------- */
> +/* 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 */
for
> +
> +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, sizeof(*byte), 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, sizeof(*word), (u8 *)word);
> +}
> +
> +int gaokun_ec_psy_get_smart_charge(struct gaokun_ec *ec,
> + u8 data[GAOKUN_SMART_CHARGE_DATA_SIZE]);
> +int gaokun_ec_psy_set_smart_charge(struct gaokun_ec *ec,
> + u8 data[GAOKUN_SMART_CHARGE_DATA_SIZE]);
> +
> +int gaokun_ec_psy_get_smart_charge_enable(struct gaokun_ec *ec, bool *on);
> +int gaokun_ec_psy_set_smart_charge_enable(struct gaokun_ec *ec, bool on);
> +
> +/* -------------------------------------------------------------------------- */
> +/* API For UCSI */
for
> +
> +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, struct gaokun_ucsi_reg *ureg);
> +int gaokun_ec_ucsi_pan_ack(struct gaokun_ec *ec, int port_id);
> +
> +
> +#endif /* __HUAWEI_GAOKUN_EC_H__ */
>
--
i.
Powered by blists - more mailing lists