Re: [PATCH v8 2/6] platform: arm64: Add driver for EC found on Qualcomm reference devices

[Anvesh Jain P <anvesh.p@oss.qualcomm.com>]

On 5/11/2026 5:37 PM, Ilpo Järvinen wrote:
> On Mon, 11 May 2026, Anvesh Jain P wrote:
> 
>>
>>
>> On 5/8/2026 11:03 PM, Ilpo Järvinen wrote:
>>> On Mon, 27 Apr 2026, Anvesh Jain P wrote:
>>>
>>>> From: Sibi Sankar <sibi.sankar@oss.qualcomm.com>
>>>>
>>>> Add Embedded controller driver support for Hamoa/Purwa/Glymur qualcomm
>>>> reference boards. It handles fan control, temperature sensors, access
>>>> to EC state changes and supports reporting suspend entry/exit to the
>>>> EC.
>>>
>>> Thanks, this seems mostly ready now. A few minor things still noted below.
>>>
>>
>> Hi Ilpo,
>>
>> Thanks for the review! Addressed all points below.
>>>> Co-developed-by: Maya Matuszczyk <maccraft123mc@gmail.com>
>>>> Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
>>>> Signed-off-by: Sibi Sankar <sibi.sankar@oss.qualcomm.com>
>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>>>> Acked-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>> Tested-by: Akhil P Oommen <akhilpo@oss.qualcomm.com>
>>>> Co-developed-by: Anvesh Jain P <anvesh.p@oss.qualcomm.com>
>>>> Signed-off-by: Anvesh Jain P <anvesh.p@oss.qualcomm.com>
>>>> ---
>>>>  MAINTAINERS                            |   8 +
>>>>  drivers/platform/arm64/Kconfig         |  13 +
>>>>  drivers/platform/arm64/Makefile        |   1 +
>>>>  drivers/platform/arm64/qcom-hamoa-ec.c | 452 +++++++++++++++++++++++++++++++++
>>>>  4 files changed, 474 insertions(+)
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 62b89d0013d2..0bf0d6d55550 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -21980,6 +21980,14 @@ F:	Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
>>>>  F:	drivers/misc/fastrpc.c
>>>>  F:	include/uapi/misc/fastrpc.h
>>>>  
>>>> +QUALCOMM HAMOA EMBEDDED CONTROLLER DRIVER
>>>> +M:	Anvesh Jain P <anvesh.p@oss.qualcomm.com>
>>>> +M:	Sibi Sankar <sibi.sankar@oss.qualcomm.com>
>>>> +L:	linux-arm-msm@vger.kernel.org
>>>> +S:	Maintained
>>>> +F:	Documentation/devicetree/bindings/embedded-controller/qcom,hamoa-crd-ec.yaml
>>>> +F:	drivers/platform/arm64/qcom-hamoa-ec.c
>>>> +
>>>>  QUALCOMM HEXAGON ARCHITECTURE
>>>>  M:	Brian Cain <brian.cain@oss.qualcomm.com>
>>>>  L:	linux-hexagon@vger.kernel.org
>>>> diff --git a/drivers/platform/arm64/Kconfig b/drivers/platform/arm64/Kconfig
>>>> index 10f905d7d6bf..e32e01b2a9bd 100644
>>>> --- a/drivers/platform/arm64/Kconfig
>>>> +++ b/drivers/platform/arm64/Kconfig
>>>> @@ -90,4 +90,17 @@ config EC_LENOVO_THINKPAD_T14S
>>>>  
>>>>  	  Say M or Y here to include this support.
>>>>  
>>>> +config EC_QCOM_HAMOA
>>>> +	tristate "Embedded Controller driver for Qualcomm Hamoa/Glymur reference devices"
>>>> +	depends on ARCH_QCOM || COMPILE_TEST
>>>> +	depends on I2C
>>>> +	depends on THERMAL || THERMAL=n
>>>> +	help
>>>> +	  Say M or Y here to enable the Embedded Controller driver for Qualcomm
>>>> +	  Snapdragon-based Hamoa/Glymur reference devices. The driver handles fan
>>>> +	  control, temperature sensors, access to EC state changes and supports
>>>> +	  reporting suspend entry/exit to the EC.
>>>> +
>>>> +	  This driver currently supports Hamoa/Purwa/Glymur reference devices.
>>>> +
>>>>  endif # ARM64_PLATFORM_DEVICES
>>>> diff --git a/drivers/platform/arm64/Makefile b/drivers/platform/arm64/Makefile
>>>> index 60c131cff6a1..7681be4a46e9 100644
>>>> --- a/drivers/platform/arm64/Makefile
>>>> +++ b/drivers/platform/arm64/Makefile
>>>> @@ -9,3 +9,4 @@ 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
>>>>  obj-$(CONFIG_EC_LENOVO_THINKPAD_T14S) += lenovo-thinkpad-t14s.o
>>>> +obj-$(CONFIG_EC_QCOM_HAMOA) += qcom-hamoa-ec.o
>>>> diff --git a/drivers/platform/arm64/qcom-hamoa-ec.c b/drivers/platform/arm64/qcom-hamoa-ec.c
>>>> new file mode 100644
>>>> index 000000000000..253f927c9aca
>>>> --- /dev/null
>>>> +++ b/drivers/platform/arm64/qcom-hamoa-ec.c
>>>> @@ -0,0 +1,452 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + * Copyright (c) 2024 Maya Matuszczyk <maccraft123mc@gmail.com>
>>>> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
>>>> + */
>>>> +
>>>> +#include <linux/bitfield.h>
>>>> +#include <linux/bits.h>
>>>> +#include <linux/device.h>
>>>> +#include <linux/dev_printk.h>
>>>> +#include <linux/err.h>
>>>> +#include <linux/i2c.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/pm.h>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/thermal.h>
>>>> +
>>>> +#define EC_SCI_EVT_READ_CMD	0x05
>>>> +#define EC_FW_VERSION_CMD	0x0e
>>>> +#define EC_MODERN_STANDBY_CMD	0x23
>>>> +#define EC_FAN_DBG_CONTROL_CMD	0x30
>>>> +#define EC_SCI_EVT_CONTROL_CMD	0x35
>>>> +#define EC_THERMAL_CAP_CMD	0x42
>>>> +
>>>> +#define EC_FW_VERSION_RESP_LEN	4
>>>> +#define EC_THERMAL_CAP_RESP_LEN	3
>>>> +#define EC_FAN_DEBUG_CMD_LEN	6
>>>> +#define EC_FAN_SPEED_DATA_SIZE	4
>>>> +
>>>> +#define EC_MODERN_STANDBY_ENTER	0x01
>>>> +#define EC_MODERN_STANDBY_EXIT	0x00
>>>> +
>>>> +#define EC_FAN_DEBUG_MODE_OFF   0
>>>> +#define EC_FAN_DEBUG_MODE_ON    BIT(0)
>>>> +#define EC_FAN_ON               BIT(1)
>>>> +#define EC_FAN_DEBUG_TYPE_PWM   BIT(2)
>>>> +#define EC_MAX_FAN_CNT		2
>>>> +#define EC_FAN_NAME_SIZE	20
>>>> +#define EC_FAN_MAX_PWM		255
>>>> +
>>>> +enum qcom_ec_sci_events {
>>>> +	EC_FAN1_STATUS_CHANGE_EVT = 0x30,
>>>> +	EC_FAN2_STATUS_CHANGE_EVT,
>>>> +	EC_FAN1_SPEED_CHANGE_EVT,
>>>> +	EC_FAN2_SPEED_CHANGE_EVT,
>>>> +	EC_NEW_LUT_SET_EVT,
>>>> +	EC_FAN_PROFILE_SWITCH_EVT,
>>>> +	EC_THERMISTOR_1_THRESHOLD_CROSS_EVT,
>>>> +	EC_THERMISTOR_2_THRESHOLD_CROSS_EVT,
>>>> +	EC_THERMISTOR_3_THRESHOLD_CROSS_EVT,
>>>> +	/* Reserved: 0x39 - 0x3c/0x3f */
>>>> +	EC_RECOVERED_FROM_RESET_EVT = 0x3d,
>>>> +};
>>>> +
>>>> +struct qcom_ec_version {
>>>> +	u8 main_version;
>>>> +	u8 sub_version;
>>>> +	u8 test_version;
>>>> +};
>>>> +
>>>> +struct qcom_ec_thermal_cap {
>>>> +#define EC_THERMAL_FAN_CNT(x)		(FIELD_GET(GENMASK(1, 0), (x)))
>>>> +#define EC_THERMAL_FAN_TYPE(x)		(FIELD_GET(GENMASK(4, 2), (x)))
>>>> +#define EC_THERMAL_THERMISTOR_MASK(x)	(FIELD_GET(GENMASK(7, 0), (x)))
>>>> +	u8 fan_cnt;
>>>> +	u8 fan_type;
>>>> +	u8 thermistor_mask;
>>>> +};
>>>> +
>>>> +struct qcom_ec_cooling_dev {
>>>> +	struct thermal_cooling_device *cdev;
>>>> +	struct device *parent_dev;
>>>> +	u8 fan_id;
>>>> +	u8 state;
>>>> +};
>>>> +
>>>> +struct qcom_ec {
>>>> +	struct qcom_ec_cooling_dev *ec_cdev;
>>>> +	struct qcom_ec_thermal_cap thermal_cap;
>>>> +	struct qcom_ec_version version;
>>>> +	struct i2c_client *client;
>>>> +};
>>>> +
>>>> +static int qcom_ec_read(struct qcom_ec *ec, u8 cmd, u8 resp_len, u8 *resp)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	ret = i2c_smbus_read_i2c_block_data(ec->client, cmd, resp_len, resp);
>>>> +
>>>> +	if (ret < 0)
>>>
>>> Call and its error handling belong together so please remove the empty line.
>>>
>>
>> Ack, will remove the blank line in the next revision.
>>
>>>> +		return ret;
>>>> +	else if (ret == 0 || ret == 0xff)
>>>> +		return -EOPNOTSUPP;
>>>> +
>>>> +	if (resp[0] >= resp_len)
>>>> +		return -EINVAL;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +/*
>>>> + * EC Device Firmware Version:
>>>> + *
>>>> + * Read Response:
>>>> + * ----------------------------------------------------------------------
>>>> + * | Offset	| Name		| Description				|
>>>> + * ----------------------------------------------------------------------
>>>> + * | 0x00	| Byte count	| Number of bytes in response		|
>>>> + * |		|		| (excluding byte count)		|
>>>> + * ----------------------------------------------------------------------
>>>> + * | 0x01	| Test-version	| Test-version of EC firmware		|
>>>> + * ----------------------------------------------------------------------
>>>> + * | 0x02	| Sub-version	| Sub-version of EC firmware		|
>>>> + * ----------------------------------------------------------------------
>>>> + * | 0x03	| Main-version	| Main-version of EC firmware		|
>>>> + * ----------------------------------------------------------------------
>>>> + *
>>>> + */
>>>> +static int qcom_ec_read_fw_version(struct device *dev)
>>>> +{
>>>> +	struct i2c_client *client = to_i2c_client(dev);
>>>> +	struct qcom_ec *ec = i2c_get_clientdata(client);
>>>> +	struct qcom_ec_version *version = &ec->version;
>>>> +	u8 resp[EC_FW_VERSION_RESP_LEN];
>>>> +	int ret;
>>>> +
>>>> +	ret = qcom_ec_read(ec, EC_FW_VERSION_CMD, EC_FW_VERSION_RESP_LEN, resp);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	version->main_version = resp[3];
>>>> +	version->sub_version = resp[2];
>>>> +	version->test_version = resp[1];
>>>> +
>>>> +	dev_dbg(dev, "EC Version %d.%d.%d\n",
>>>> +		version->main_version, version->sub_version, version->test_version);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +/*
>>>> + * EC Device Thermal Capabilities:
>>>> + *
>>>> + * Read Response:
>>>> + * ------------------------------------------------------------------------------
>>>> + * | Offset		| Name		| Description				|
>>>> + * ------------------------------------------------------------------------------
>>>> + * | 0x00		| Byte count	| Number of bytes in response		|
>>>> + * |			|		| (excluding byte count)		|
>>>> + * ------------------------------------------------------------------------------
>>>> + * | 0x02 (LSB)	| EC Thermal	| Bit 0-1: Number of fans		|
>>>> + * | 0x03		| Capabilities	| Bit 2-4: Type of fan			|
>>>> + * |			|		| Bit 5-6: Reserved			|
>>>> + * |			|		| Bit 7: Data Valid/Invalid		|
>>>> + * |			|		|	 (Valid - 1, Invalid - 0)	|
>>>> + * |			|		| Bit 8-15: Thermistor 0 - 7 presence	|
>>>> + * |			|		|	    (1 present, 0 absent)	|
>>>> + * ------------------------------------------------------------------------------
>>>> + *
>>>> + */
>>>> +static int qcom_ec_thermal_capabilities(struct device *dev)
>>>> +{
>>>> +	struct i2c_client *client = to_i2c_client(dev);
>>>> +	struct qcom_ec *ec = i2c_get_clientdata(client);
>>>> +	struct qcom_ec_thermal_cap *cap = &ec->thermal_cap;
>>>> +	u8 resp[EC_THERMAL_CAP_RESP_LEN];
>>>> +	int ret;
>>>> +
>>>> +	ret = qcom_ec_read(ec, EC_THERMAL_CAP_CMD, EC_THERMAL_CAP_RESP_LEN, resp);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	cap->fan_cnt = min(EC_MAX_FAN_CNT, EC_THERMAL_FAN_CNT(resp[1]));
>>>> +	cap->fan_type = EC_THERMAL_FAN_TYPE(resp[1]);
>>>> +	cap->thermistor_mask = EC_THERMAL_THERMISTOR_MASK(resp[2]);
>>>> +
>>>> +	dev_dbg(dev, "Fan count: %d Fan Type: %d Thermistor Mask: %x\n",
>>>> +		cap->fan_cnt, cap->fan_type, cap->thermistor_mask);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static irqreturn_t qcom_ec_irq(int irq, void *data)
>>>> +{
>>>> +	struct qcom_ec *ec = data;
>>>> +	struct device *dev = &ec->client->dev;
>>>> +	int val;
>>>> +
>>>> +	val = i2c_smbus_read_byte_data(ec->client, EC_SCI_EVT_READ_CMD);
>>>> +	if (val < 0) {
>>>> +		dev_err_ratelimited(dev, "Failed to read EC SCI Event: %d\n", val);
>>>> +		return IRQ_HANDLED;
>>>> +	}
>>>> +
>>>> +	switch (val) {
>>>> +	case EC_FAN1_STATUS_CHANGE_EVT:
>>>> +		dev_dbg_ratelimited(dev, "Fan1 status changed\n");
>>>> +		break;
>>>> +	case EC_FAN2_STATUS_CHANGE_EVT:
>>>> +		dev_dbg_ratelimited(dev, "Fan2 status changed\n");
>>>> +		break;
>>>> +	case EC_FAN1_SPEED_CHANGE_EVT:
>>>> +		dev_dbg_ratelimited(dev, "Fan1 speed crossed low/high trip point\n");
>>>> +		break;
>>>> +	case EC_FAN2_SPEED_CHANGE_EVT:
>>>> +		dev_dbg_ratelimited(dev, "Fan2 speed crossed low/high trip point\n");
>>>> +		break;
>>>> +	case EC_NEW_LUT_SET_EVT:
>>>> +		dev_dbg_ratelimited(dev, "New LUT set\n");
>>>> +		break;
>>>> +	case EC_FAN_PROFILE_SWITCH_EVT:
>>>> +		dev_dbg_ratelimited(dev, "FAN Profile switched\n");
>>>> +		break;
>>>> +	case EC_THERMISTOR_1_THRESHOLD_CROSS_EVT:
>>>> +		dev_dbg_ratelimited(dev, "Thermistor 1 threshold crossed\n");
>>>> +		break;
>>>> +	case EC_THERMISTOR_2_THRESHOLD_CROSS_EVT:
>>>> +		dev_dbg_ratelimited(dev, "Thermistor 2 threshold crossed\n");
>>>> +		break;
>>>> +	case EC_THERMISTOR_3_THRESHOLD_CROSS_EVT:
>>>> +		dev_dbg_ratelimited(dev, "Thermistor 3 threshold crossed\n");
>>>> +		break;
>>>> +	case EC_RECOVERED_FROM_RESET_EVT:
>>>> +		dev_dbg_ratelimited(dev, "EC recovered from reset\n");
>>>> +		break;
>>>> +	default:
>>>> +		dev_notice_ratelimited(dev, "Unknown EC event: %d\n", val);
>>>> +		break;
>>>> +	}
>>>> +
>>>> +	return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static int qcom_ec_sci_evt_control(struct device *dev, bool enable)
>>>> +{
>>>> +	struct i2c_client *client = to_i2c_client(dev);
>>>> +
>>>> +	return i2c_smbus_write_byte_data(client, EC_SCI_EVT_CONTROL_CMD, !!enable);
>>>
>>> This converts bool -> u8 using C's implicit conversion rules (plus on top 
>>> of that does unnecessary !!). Please write the conversion explicitly, you 
>>> can use ?: syntax for brevity.
>>>
>>
>> The original v4 code used `enable ? 1 : 0` explicitly, but Konrad Dybcio
>> suggested switching to `!!enable` during his review [1]. Happy to revert
>> to the explicit form if that's the preferred style — just want to flag
>> the conflict so everyone is aligned.
>>
>> [1]
>> https://lore.kernel.org/all/6eb3a173-c364-431f-93e4-7bbb7a32431e@oss.qualcomm.com/
> 
> I prefer to have the boolean to binary conversion done explicitly 
> (with the platform drivers maintainer hat on).
>

Understood, will update to `enable ? 1 : 0` in the next revision.

-- 
Best Regards,
Anvesh