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

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

On 3/30/2026 12:14 PM, Anvesh Jain P wrote:
> 
> 
> On 3/27/2026 5:20 PM, Ilpo Järvinen wrote:
>> On Fri, 27 Mar 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.
>>>
>>> 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>
>>> 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         |  12 +
>>>  drivers/platform/arm64/Makefile        |   1 +
>>>  drivers/platform/arm64/qcom-hamoa-ec.c | 451 +++++++++++++++++++++++++++++++++
>>>  4 files changed, 472 insertions(+)
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 30ca84404976..536dfd9adff4 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -21804,6 +21804,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-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..025cdf091f9e 100644
>>> --- a/drivers/platform/arm64/Kconfig
>>> +++ b/drivers/platform/arm64/Kconfig
>>> @@ -90,4 +90,16 @@ 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
>>> +	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..0f883130ac9a
>>> --- /dev/null
>>> +++ b/drivers/platform/arm64/qcom-hamoa-ec.c
>>> @@ -0,0 +1,451 @@
>>> +// 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/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)
>>
>> Add include for BIT().
>>
> 
> Added <linux/bits.h> in v6 to address this.
> 
>>> +#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)
>>> +		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		|
>>> + * | 0x3		| Capabilities	| Bit 2-4: Type of fan			|
>>
>> 0x03 ?
>>
> 
> Typo, Will fix to 0x03 in next respin.
> 
>>> + * |			|		| 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",
>>
>> Please add include for dev_dbg().
>>
>> It seems you've missed at least some of my comments to v5, please recheck
>> those comments. I won't look further for now.
>>
>> --
>>  i.
>>
> 
> Added <linux/device.h> for dev_dbg() in v6, will include
> <linux/dev_printk.h> as well in in v8. That said, all your v5 comments
> have been addressed, here is a summary of what was fixed:
> 
> - Add <linux/bits.h> for BIT()
> - Add <linux/err.h> for IS_ERR()
> - Switch thermistor mask format specifier from %d to %x
> - Add missing braces
> - Remove empty line within variable declarations
> - Change loop counter i to unsigned int
> - Replace snprintf() with scnprintf()
> - Use sizeof(name) instead of the EC_FAN_NAME_SIZE macro directly
> - Condense devm_thermal_of_cooling_device_register() to 2 lines
> 
> Apologies if it appeared otherwise. If anything was missed, please do
> point it out and I will address it in the next respin.
> 

Hi Ilpo,

Could you please take a look at this when you get a chance?

-- 
Best Regards,
Anvesh