[PATCH 0/8] Extend Qualcomm reference device EC driver with fan LUT, profile and SoC Tj support

[PATCH 0/8] Extend Qualcomm reference device EC driver with fan LUT, profile and SoC Tj support

[Anvesh Jain P]

This series extends the EC driver for Qualcomm reference devices
introduced in the earlier series to support auto-scaling of fan speed
based on the average SoC junction temperature. The commands added in
this series exist to facilitate that: querying fan RPM and programming
the fan speed LUT, switching between fan profiles, and periodically
reporting the SoC junction temperature to the EC.

Fan profile switching is driven by a power supply notifier that
selects best performance on AC and better battery on battery. The
fan LUT is programmed on probe and on each profile switch using
board-specific configurations for hamoa (16 entries) and glymur
(8 entries).

SoC Tj temperature reporting averages readings across the
thermal-sensors listed for the EC node and sends the result to the
EC every 125ms, paused during suspend. Glymur does not send this
temperature to the EC, since the EC on glymur is capable of obtaining
it itself.

EC command reads and writes are retried a bounded number of times
on NACK, since the EC NACKs the bus instead of clock-stretching
while busy.

---
Anvesh Jain P (8):
      dt-bindings: embedded-controller: qcom,hamoa-crd-ec: Add qcom,tsens
      platform: arm64: qcom-hamoa-ec: Add SoC junction temperature reporting
      platform: arm64: qcom-hamoa-ec: Switch fan profile based on power supply state
      platform: arm64: qcom-hamoa-ec: Add fan RPM query and LUT calibration
      platform: arm64: qcom-hamoa-ec: Verify required I2C adapter functionality
      platform: arm64: qcom-hamoa-ec: Retry I2C transfers on NACK
      arm64: dts: qcom: x1p42100-crd: Add qcom,tsens for EC fan thermal management
      arm64: dts: qcom: x1e80100-crd: Add qcom,tsens for EC fan thermal management

 .../embedded-controller/qcom,hamoa-crd-ec.yaml     |   31 +
 arch/arm64/boot/dts/qcom/x1e80100-crd.dts          |    6 +
 arch/arm64/boot/dts/qcom/x1p42100-crd.dts          |    6 +
 drivers/platform/arm64/qcom-hamoa-ec.c             | 1014 +++++++++++++++++++-
 4 files changed, 1020 insertions(+), 37 deletions(-)
---
base-commit: 8e9685d3c41c35dd1b37df70d854137abcb2fbac
change-id: 20260507-ec_add_more_commands-9a6513363375
prerequisite-message-id: <20260703-ec_support_for_purwa_evk-v2-0-c59aac5c6aac@oss.qualcomm.com>
prerequisite-patch-id: f5031e367e940109367f5fff1a679ea5ee453a8b
prerequisite-patch-id: 890ce0b8b86b3027b3bca383c85e9861398e6dc5

Best regards,
-- 
Anvesh Jain P <anvesh.p@oss.qualcomm.com>

[PATCH 1/8] dt-bindings: embedded-controller: qcom,hamoa-crd-ec: Add qcom,tsens

[Anvesh Jain P]

Add the qcom,tsens property so Hamoa-based boards can list the tsens
providers, and how many leading sensor IDs on each, whose readings the
driver averages to compute the SoC junction temperature reported to
the EC for fan control.

The generic thermal-sensors property has no standard semantic for
selecting a range of sensor IDs from a provider without enumerating
each one, so a vendor-specific phandle-array with an explicit sensor
count is used instead.

Signed-off-by: Anvesh Jain P <anvesh.p@oss.qualcomm.com>
---
 .../embedded-controller/qcom,hamoa-crd-ec.yaml     | 31 ++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/Documentation/devicetree/bindings/embedded-controller/qcom,hamoa-crd-ec.yaml b/Documentation/devicetree/bindings/embedded-controller/qcom,hamoa-crd-ec.yaml
index c07483aa5937..ec809fcd03ab 100644
--- a/Documentation/devicetree/bindings/embedded-controller/qcom,hamoa-crd-ec.yaml
+++ b/Documentation/devicetree/bindings/embedded-controller/qcom,hamoa-crd-ec.yaml
@@ -33,11 +33,40 @@ properties:
   interrupts:
     maxItems: 1
 
+  # A generic "thermal-sensors" phandle only identifies individual sensor
+  # IDs, and thermal zones are a Linux-side grouping of those sensors, not
+  # a devicetree concept the EC cares about. What the driver actually needs
+  # is raw readings from a contiguous range of leading sensor IDs on each
+  # tsens provider, averaged together, so qcom,tsens pairs each provider
+  # phandle with a count instead of enumerating every sensor cell.
+  qcom,tsens:
+    description:
+      List of tsens providers and, for each, the number of leading sensor
+      IDs on that provider whose readings are averaged to compute the SoC
+      junction temperature reported to the EC for fan control. Only
+      applicable for Hamoa based devices.
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    minItems: 1
+    maxItems: 2
+    items:
+      items:
+        - description: phandle to a tsens thermal-sensor provider
+        - description: number of leading sensor IDs on that provider to average
+
 required:
   - compatible
   - reg
   - interrupts
 
+if:
+  properties:
+    compatible:
+      contains:
+        const: qcom,glymur-crd-ec
+then:
+  properties:
+    qcom,tsens: false
+
 additionalProperties: false
 
 examples:
@@ -52,6 +81,8 @@ examples:
             reg = <0x76>;
 
             interrupts-extended = <&tlmm 66 IRQ_TYPE_LEVEL_HIGH>;
+
+            qcom,tsens = <&tsens3 2>;
         };
     };
 ...

-- 
2.34.1

Re: [PATCH 1/8] dt-bindings: embedded-controller: qcom,hamoa-crd-ec: Add qcom,tsens

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The custom `qcom,tsens` property encodes a driver-specific software algorithm into Devicetree and redefines standard phandle specifier semantics to avoid standard sensor enumeration.
--

--- Patch [1]: [PATCH 1/8] dt-bindings: embedded-controller: qcom,hamoa-crd-ec: Add qcom,tsens ---
commit d7c232898faf92bef60389e6c46fc016182bd22a
Author: Anvesh Jain P <anvesh.p@oss.qualcomm.com>

dt-bindings: embedded-controller: qcom,hamoa-crd-ec: Add qcom,tsens

This commit adds the qcom,tsens property to the Hamoa-based boards embedded
controller binding. It pairs tsens provider phandles with a count of leading
sensor IDs, whose readings are averaged to compute the SoC junction
temperature.

> diff --git a/Documentation/devicetree/bindings/embedded-controller/qcom,hamoa-crd-ec.yaml b/Documentation/devicetree/bindings/embedded-controller/qcom,hamoa-crd-ec.yaml
> --- a/Documentation/devicetree/bindings/embedded-controller/qcom,hamoa-crd-ec.yaml
> +++ b/Documentation/devicetree/bindings/embedded-controller/qcom,hamoa-crd-ec.yaml
[ ... ]
> @@ -32,11 +32,40 @@ properties:
>    interrupts:
>      maxItems: 1
>  
> +  # A generic "thermal-sensors" phandle only identifies individual sensor
> +  # IDs, and thermal zones are a Linux-side grouping of those sensors, not
> +  # a devicetree concept the EC cares about. What the driver actually needs
> +  # is raw readings from a contiguous range of leading sensor IDs on each
> +  # tsens provider, averaged together, so qcom,tsens pairs each provider
> +  # phandle with a count instead of enumerating every sensor cell.
> +  qcom,tsens:
> +    description:
> +      List of tsens providers and, for each, the number of leading sensor
> +      IDs on that provider whose readings are averaged to compute the SoC
> +      junction temperature reported to the EC for fan control. Only
> +      applicable for Hamoa based devices.

[Severity: Medium]
Does this property encode a driver-specific software algorithm into the
device tree? Device tree is strictly designed to describe hardware topology,
not software policies like averaging sensor readings.

By bypassing standard properties like thermal-sensors and redefining the
phandle specifier semantics to represent a count rather than enumerating the
specific hardware sensor IDs, does this create an incorrectly designed ABI?
It might be better to rely on standard hardware enumeration and leave the
averaging policy to the software drivers.
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-ec_add_more_commands-v1-0-771abd65ee1a@oss.qualcomm.com?part=1

[PATCH 2/8] platform: arm64: qcom-hamoa-ec: Add SoC junction temperature reporting

[Anvesh Jain P]

Add the EC command definitions and handler function for reporting the
SoC junction temperature (Tj) to the EC.

Discover the platform's thermal sensor to zone mapping via the
qcom,tsens device tree property, average the junction temperatures
across the mapped zones, and periodically report the result to the EC
over SMBus using a delayed work item. Serialize this and the existing
EC command sequences (firmware version read, thermal capability read,
SCI event control, and the SCI IRQ handler) under a new io_lock mutex,
since the delayed work item now runs concurrently with those paths.

Re-arm the periodic report on resume and cancel it on suspend to avoid
racing with the modern standby transition.
---
 drivers/platform/arm64/qcom-hamoa-ec.c | 257 +++++++++++++++++++++++++++++++--
 1 file changed, 242 insertions(+), 15 deletions(-)

diff --git a/drivers/platform/arm64/qcom-hamoa-ec.c b/drivers/platform/arm64/qcom-hamoa-ec.c
index 5ca7308c6077..4c745b78322c 100644
--- a/drivers/platform/arm64/qcom-hamoa-ec.c
+++ b/drivers/platform/arm64/qcom-hamoa-ec.c
@@ -6,6 +6,8 @@
 
 #include <linux/bitfield.h>
 #include <linux/bits.h>
+#include <linux/delay.h>
+#include <linux/devm-helpers.h>
 #include <linux/device.h>
 #include <linux/dev_printk.h>
 #include <linux/err.h>
@@ -13,17 +15,25 @@
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.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_SOC_TEMP_CMD	0x20
 #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_SOC_TEMP_SOURCE_TJ			0x1
+#define EC_SOC_TEMP_SOURCE_TSKIN		0x2
+#define EC_SOC_TEMP_DATA_SIZE			2
+
 #define EC_FW_VERSION_RESP_LEN	4
 #define EC_THERMAL_CAP_RESP_LEN	3
 #define EC_FAN_DEBUG_CMD_LEN	6
@@ -32,14 +42,16 @@
 #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_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
 
+#define EC_SOC_TJ_TEMP_POLL_JIFFIES	msecs_to_jiffies(125)
+
 enum qcom_ec_sci_events {
 	EC_FAN1_STATUS_CHANGE_EVT = 0x30,
 	EC_FAN2_STATUS_CHANGE_EVT,
@@ -77,10 +89,14 @@ struct qcom_ec_cooling_dev {
 };
 
 struct qcom_ec {
+	struct i2c_client *client;
 	struct qcom_ec_cooling_dev *ec_cdev;
+	struct thermal_zone_device **soc_tj_zones;
+	struct delayed_work soc_tj_work;
+	struct mutex io_lock; /* serializes EC command sequences */
 	struct qcom_ec_thermal_cap thermal_cap;
 	struct qcom_ec_version version;
-	struct i2c_client *client;
+	int num_soc_tj_zones;
 };
 
 static int qcom_ec_read(struct qcom_ec *ec, u8 cmd, u8 resp_len, u8 *resp)
@@ -125,7 +141,9 @@ static int qcom_ec_read_fw_version(struct device *dev)
 	u8 resp[EC_FW_VERSION_RESP_LEN];
 	int ret;
 
+	mutex_lock(&ec->io_lock);
 	ret = qcom_ec_read(ec, EC_FW_VERSION_CMD, EC_FW_VERSION_RESP_LEN, resp);
+	mutex_unlock(&ec->io_lock);
 	if (ret < 0)
 		return ret;
 
@@ -167,7 +185,9 @@ static int qcom_ec_thermal_capabilities(struct device *dev)
 	u8 resp[EC_THERMAL_CAP_RESP_LEN];
 	int ret;
 
+	mutex_lock(&ec->io_lock);
 	ret = qcom_ec_read(ec, EC_THERMAL_CAP_CMD, EC_THERMAL_CAP_RESP_LEN, resp);
+	mutex_unlock(&ec->io_lock);
 	if (ret < 0)
 		return ret;
 
@@ -181,13 +201,165 @@ static int qcom_ec_thermal_capabilities(struct device *dev)
 	return 0;
 }
 
+static struct thermal_zone_device *
+qcom_ec_sensor_to_zone(struct device_node *sensor_np, u32 sensor_id)
+{
+	struct device_node *tz_np __free(device_node) =
+		of_find_node_by_name(NULL, "thermal-zones");
+
+	if (!tz_np)
+		return ERR_PTR(-ENODEV);
+
+	for_each_available_child_of_node_scoped(tz_np, child) {
+		struct of_phandle_args args;
+
+		if (of_parse_phandle_with_args(child, "thermal-sensors",
+					       "#thermal-sensor-cells", 0, &args))
+			continue;
+
+		of_node_put(args.np);
+
+		if (args.np == sensor_np &&
+		    sensor_id == (args.args_count ? args.args[0] : 0))
+			return thermal_zone_get_zone_by_name(child->name);
+	}
+
+	return ERR_PTR(-ENODEV);
+}
+
+static int qcom_ec_setup_soc_tj_zones(struct qcom_ec *ec)
+{
+	struct device *dev = &ec->client->dev;
+	struct device_node *np = dev->of_node;
+	int nproviders, total, i;
+
+	nproviders = of_property_count_elems_of_size(np, "qcom,tsens",
+						     2 * sizeof(u32));
+	if (nproviders <= 0)
+		return 0;
+
+	total = 0;
+	for (i = 0; i < nproviders; i++) {
+		struct of_phandle_args args;
+
+		if (of_parse_phandle_with_fixed_args(np, "qcom,tsens", 1, i, &args))
+			return -EINVAL;
+
+		of_node_put(args.np);
+		total += args.args[0];
+	}
+
+	ec->soc_tj_zones = devm_kcalloc(dev, total, sizeof(*ec->soc_tj_zones),
+					GFP_KERNEL);
+	if (!ec->soc_tj_zones)
+		return -ENOMEM;
+
+	for (i = 0; i < nproviders; i++) {
+		struct of_phandle_args args;
+		struct platform_device *sensor_pdev;
+		u32 id;
+
+		if (of_parse_phandle_with_fixed_args(np, "qcom,tsens", 1, i, &args))
+			return -EINVAL;
+
+		sensor_pdev = of_find_device_by_node(args.np);
+		if (!sensor_pdev) {
+			of_node_put(args.np);
+			return -EPROBE_DEFER;
+		}
+
+		/* Ensure we unbind before the sensor frees its zones */
+		if (!device_link_add(dev, &sensor_pdev->dev, DL_FLAG_AUTOREMOVE_CONSUMER)) {
+			dev_err(dev, "Failed to link to sensor %pOF\n", args.np);
+			put_device(&sensor_pdev->dev);
+			of_node_put(args.np);
+			return -ENODEV;
+		}
+		put_device(&sensor_pdev->dev);
+
+		for (id = 0; id < args.args[0]; id++) {
+			struct thermal_zone_device *tz;
+
+			tz = qcom_ec_sensor_to_zone(args.np, id);
+			if (IS_ERR(tz))
+				continue;
+
+			ec->soc_tj_zones[ec->num_soc_tj_zones++] = tz;
+		}
+		of_node_put(args.np);
+	}
+
+	return 0;
+}
+
+/*
+ * SoC to EC Temperature command:
+ *
+ * Command Payload:
+ * -------------------------------------------------------------------------------
+ * | Offset	| Name		| Description					|
+ * -------------------------------------------------------------------------------
+ * | 0x00	| Command = 0x20| SoC to EC Temperature command			|
+ * -------------------------------------------------------------------------------
+ * | 0x01	| Temp Source	| 0x1 : CPU junction temperature (SoC Tj)	|
+ * |		|		| 0x2 : Skin Temperature (Tskin)		|
+ * -------------------------------------------------------------------------------
+ * | 0x02	| Byte count = 2| Number of data bytes for temperature value	|
+ * -------------------------------------------------------------------------------
+ * | 0x03 (LSB)| Temperature	| Temperature value in 0.1 deg C units		|
+ * | 0x04	|		| (e.g. 651 => 65.1 deg C)			|
+ * -------------------------------------------------------------------------------
+ *
+ */
+
+static int qcom_ec_send_soc_tj_temp(struct qcom_ec *ec)
+{
+	int total = 0;
+	int valid = 0;
+	int ret = 0;
+	u16 temp;
+	u8 request[4];
+
+	for (int i = 0; i < ec->num_soc_tj_zones; i++) {
+		int zone_temp;
+
+		if (thermal_zone_get_temp(ec->soc_tj_zones[i], &zone_temp))
+			continue;
+
+		total += zone_temp;
+		valid++;
+	}
+
+	if (!valid)
+		return 0;
+
+	/* Convert millidegrees Celsius to 0.1 deg C units as required by EC */
+	temp = clamp(total / valid / 100, 0, (int)U16_MAX);
+	request[0] = EC_SOC_TEMP_SOURCE_TJ;
+	request[1] = EC_SOC_TEMP_DATA_SIZE;
+	request[2] = temp & 0xff;
+	request[3] = (temp >> 8) & 0xff;
+
+	mutex_lock(&ec->io_lock);
+	ret = i2c_smbus_write_i2c_block_data(ec->client, EC_SOC_TEMP_CMD,
+					     sizeof(request), request);
+	mutex_unlock(&ec->io_lock);
+	if (ret < 0)
+		dev_err_ratelimited(&ec->client->dev,
+				    "Failed to send SoC Tj temperature: %d\n", ret);
+
+	return ret;
+}
+
 static irqreturn_t qcom_ec_irq(int irq, void *data)
 {
 	struct qcom_ec *ec = data;
 	struct device *dev = &ec->client->dev;
 	int val;
 
+	mutex_lock(&ec->io_lock);
 	val = i2c_smbus_read_byte_data(ec->client, EC_SCI_EVT_READ_CMD);
+	mutex_unlock(&ec->io_lock);
 	if (val < 0) {
 		dev_err_ratelimited(dev, "Failed to read EC SCI Event: %d\n", val);
 		return IRQ_HANDLED;
@@ -235,8 +407,24 @@ static irqreturn_t qcom_ec_irq(int irq, void *data)
 static int qcom_ec_sci_evt_control(struct device *dev, bool enable)
 {
 	struct i2c_client *client = to_i2c_client(dev);
+	struct qcom_ec *ec = i2c_get_clientdata(client);
+	int ret;
+
+	mutex_lock(&ec->io_lock);
+	ret = i2c_smbus_write_byte_data(client, EC_SCI_EVT_CONTROL_CMD, enable ? 1 : 0);
+	mutex_unlock(&ec->io_lock);
 
-	return i2c_smbus_write_byte_data(client, EC_SCI_EVT_CONTROL_CMD, enable ? 1 : 0);
+	return ret;
+}
+
+static void qcom_ec_sci_evt_disable(void *data)
+{
+	struct device *dev = data;
+	int ret;
+
+	ret = qcom_ec_sci_evt_control(dev, false);
+	if (ret < 0)
+		dev_err(dev, "Failed to disable SCI events: %d\n", ret);
 }
 
 static int qcom_ec_fan_get_max_state(struct thermal_cooling_device *cdev, unsigned long *state)
@@ -326,20 +514,48 @@ static const struct thermal_cooling_device_ops qcom_ec_thermal_ops = {
 	.set_cur_state = qcom_ec_fan_set_cur_state,
 };
 
+static void qcom_ec_soc_tj_work_fn(struct work_struct *work)
+{
+	struct qcom_ec *ec = container_of(work, struct qcom_ec, soc_tj_work.work);
+
+	qcom_ec_send_soc_tj_temp(ec);
+
+	queue_delayed_work(system_percpu_wq, &ec->soc_tj_work, EC_SOC_TJ_TEMP_POLL_JIFFIES);
+}
+
 static int qcom_ec_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
+	struct qcom_ec *ec = i2c_get_clientdata(client);
+	int ret;
+
+	mutex_lock(&ec->io_lock);
+	ret = i2c_smbus_write_byte_data(client, EC_MODERN_STANDBY_CMD,
+					EC_MODERN_STANDBY_EXIT);
+	mutex_unlock(&ec->io_lock);
+	if (ret)
+		return ret;
+
+	enable_delayed_work(&ec->soc_tj_work);
+	queue_delayed_work(system_percpu_wq, &ec->soc_tj_work, EC_SOC_TJ_TEMP_POLL_JIFFIES);
 
-	return i2c_smbus_write_byte_data(client, EC_MODERN_STANDBY_CMD,
-					 EC_MODERN_STANDBY_EXIT);
+	return 0;
 }
 
 static int qcom_ec_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
+	struct qcom_ec *ec = i2c_get_clientdata(client);
+	int ret;
 
-	return i2c_smbus_write_byte_data(client, EC_MODERN_STANDBY_CMD,
-					 EC_MODERN_STANDBY_ENTER);
+	disable_delayed_work_sync(&ec->soc_tj_work);
+
+	mutex_lock(&ec->io_lock);
+	ret = i2c_smbus_write_byte_data(client, EC_MODERN_STANDBY_CMD,
+					EC_MODERN_STANDBY_ENTER);
+	mutex_unlock(&ec->io_lock);
+
+	return ret;
 }
 
 static int qcom_ec_probe(struct i2c_client *client)
@@ -354,6 +570,7 @@ static int qcom_ec_probe(struct i2c_client *client)
 		return -ENOMEM;
 
 	ec->client = client;
+	mutex_init(&ec->io_lock);
 
 	ret = devm_request_threaded_irq(dev, client->irq, NULL, qcom_ec_irq,
 					IRQF_ONESHOT, "qcom_ec", ec);
@@ -370,10 +587,24 @@ static int qcom_ec_probe(struct i2c_client *client)
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "Failed to enable SCI events\n");
 
+	ret = devm_add_action_or_reset(dev, qcom_ec_sci_evt_disable, dev);
+	if (ret)
+		return ret;
+
 	ret = qcom_ec_thermal_capabilities(dev);
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "Failed to read thermal capabilities\n");
 
+	ret = qcom_ec_setup_soc_tj_zones(ec);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to setup SoC Tj thermal zones\n");
+
+	ret = devm_delayed_work_autocancel(dev, &ec->soc_tj_work, qcom_ec_soc_tj_work_fn);
+	if (ret)
+		return ret;
+
+	queue_delayed_work(system_percpu_wq, &ec->soc_tj_work, EC_SOC_TJ_TEMP_POLL_JIFFIES);
+
 	if (ec->thermal_cap.fan_cnt == 0) {
 		dev_warn(dev, FW_BUG "Failed to get fan count, firmware update required\n");
 		return 0;
@@ -405,12 +636,8 @@ static int qcom_ec_probe(struct i2c_client *client)
 static void qcom_ec_remove(struct i2c_client *client)
 {
 	struct qcom_ec *ec = i2c_get_clientdata(client);
-	struct device *dev = &client->dev;
-	int ret;
 
-	ret = qcom_ec_sci_evt_control(dev, false);
-	if (ret < 0)
-		dev_err(dev, "Failed to disable SCI events: %d\n", ret);
+	disable_delayed_work_sync(&ec->soc_tj_work);
 
 	for (int i = 0; i < ec->thermal_cap.fan_cnt; i++) {
 		struct qcom_ec_cooling_dev *ec_cdev = &ec->ec_cdev[i];

-- 
2.34.1

Re: [PATCH 2/8] platform: arm64: qcom-hamoa-ec: Add SoC junction temperature reporting

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Race condition in probe leading to NULL pointer dereference in IRQ handler
- [High] Incorrect error code on device_link_add failure causes permanent probe failure
- [Medium] Missing `io_lock` acquisition in fan control functions breaks EC command serialization
- [Medium] Thermal polling delayed work unconditionally requeues itself even if no thermal zones were found
- [Medium] Suspend error path permanently disables thermal polling
- [Medium] Integer overflow when accumulating args.args[0] into total leads to heap buffer overflow
--

--- Patch [2]: [PATCH 2/8] platform: arm64: qcom-hamoa-ec: Add SoC junction temperature reporting ---
commit d5d00275c6c452ac414dd45c11ca38e64c17b24a
Author: Anvesh Jain P <anvesh.p@oss.qualcomm.com>

platform: arm64: qcom-hamoa-ec: Add SoC junction temperature reporting
    
Add the EC command definitions and handler function for reporting the
SoC junction temperature (Tj) to the EC.

[ ... ]

> diff --git a/drivers/platform/arm64/qcom-hamoa-ec.c b/drivers/platform/arm64/qcom-hamoa-ec.c
> index 5ca7308c60774..4c745b78322c4 100644
> --- a/drivers/platform/arm64/qcom-hamoa-ec.c
> +++ b/drivers/platform/arm64/qcom-hamoa-ec.c

[ ... ]

> @@ -77,10 +89,14 @@ struct qcom_ec_cooling_dev {
>  };
>  
>  struct qcom_ec {
> +	struct i2c_client *client;
>  	struct qcom_ec_cooling_dev *ec_cdev;
> +	struct thermal_zone_device **soc_tj_zones;
> +	struct delayed_work soc_tj_work;
> +	struct mutex io_lock; /* serializes EC command sequences */

[Severity: Medium]
Does this mutex also need to be acquired in existing fan control functions
like qcom_ec_set_fan_profile()?

Because the thermal framework can invoke fan control at any time, these
commands might interleave with the new background thermal polling delayed
work, which could violate the intended EC firmware serialization.

>  	struct qcom_ec_thermal_cap thermal_cap;
>  	struct qcom_ec_version version;
> -	struct i2c_client *client;
> +	int num_soc_tj_zones;
>  };

[ ... ]

> +static int qcom_ec_setup_soc_tj_zones(struct qcom_ec *ec)
> +{
> +	struct device *dev = &ec->client->dev;
> +	struct device_node *np = dev->of_node;
> +	int nproviders, total, i;
> +
> +	nproviders = of_property_count_elems_of_size(np, "qcom,tsens",
> +						     2 * sizeof(u32));
> +	if (nproviders <= 0)
> +		return 0;
> +
> +	total = 0;
> +	for (i = 0; i < nproviders; i++) {
> +		struct of_phandle_args args;
> +
> +		if (of_parse_phandle_with_fixed_args(np, "qcom,tsens", 1, i, &args))
> +			return -EINVAL;
> +
> +		of_node_put(args.np);
> +		total += args.args[0];

[Severity: Medium]
Is it possible for the total accumulation to overflow if the device tree
provides large values for the qcom,tsens property?

If the sum surpasses the maximum integer value, devm_kcalloc() would allocate
an undersized array, and the subsequent loop could cause a heap buffer
overflow when writing to ec->soc_tj_zones.

> +	}
> +
> +	ec->soc_tj_zones = devm_kcalloc(dev, total, sizeof(*ec->soc_tj_zones),
> +					GFP_KERNEL);
> +	if (!ec->soc_tj_zones)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < nproviders; i++) {
> +		struct of_phandle_args args;
> +		struct platform_device *sensor_pdev;
> +		u32 id;
> +
> +		if (of_parse_phandle_with_fixed_args(np, "qcom,tsens", 1, i, &args))
> +			return -EINVAL;
> +
> +		sensor_pdev = of_find_device_by_node(args.np);
> +		if (!sensor_pdev) {
> +			of_node_put(args.np);
> +			return -EPROBE_DEFER;
> +		}
> +
> +		/* Ensure we unbind before the sensor frees its zones */
> +		if (!device_link_add(dev, &sensor_pdev->dev, DL_FLAG_AUTOREMOVE_CONSUMER)) {
> +			dev_err(dev, "Failed to link to sensor %pOF\n", args.np);
> +			put_device(&sensor_pdev->dev);
> +			of_node_put(args.np);
> +			return -ENODEV;

[Severity: High]
Should this return -EPROBE_DEFER instead of -ENODEV?

If the EC driver is probing and the sensor is not yet bound to a driver,
device_link_add() refuses to create a managed link and returns NULL.
Returning -ENODEV causes the probe to fail permanently, whereas returning
-EPROBE_DEFER would allow it to try again once the sensor driver binds.

> +		}
> +		put_device(&sensor_pdev->dev);

[ ... ]

> +static void qcom_ec_soc_tj_work_fn(struct work_struct *work)
> +{
> +	struct qcom_ec *ec = container_of(work, struct qcom_ec, soc_tj_work.work);
> +
> +	qcom_ec_send_soc_tj_temp(ec);
> +
> +	queue_delayed_work(system_percpu_wq, &ec->soc_tj_work, EC_SOC_TJ_TEMP_POLL_JIFFIES);

[Severity: Medium]
Should the work item only be requeued if thermal zones were actually found?

If ec->num_soc_tj_zones is 0, qcom_ec_send_soc_tj_temp() evaluates valid as 0
and does nothing, and then the work unconditionally requeues itself every
125ms, creating a permanent timer tick on systems without these sensors.

> +}
> +
>  static int qcom_ec_resume(struct device *dev)

[ ... ]

>  static int qcom_ec_suspend(struct device *dev)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
> +	struct qcom_ec *ec = i2c_get_clientdata(client);
> +	int ret;
>  
> -	return i2c_smbus_write_byte_data(client, EC_MODERN_STANDBY_CMD,
> -					 EC_MODERN_STANDBY_ENTER);
> +	disable_delayed_work_sync(&ec->soc_tj_work);
> +
> +	mutex_lock(&ec->io_lock);
> +	ret = i2c_smbus_write_byte_data(client, EC_MODERN_STANDBY_CMD,
> +					EC_MODERN_STANDBY_ENTER);
> +	mutex_unlock(&ec->io_lock);
> +
> +	return ret;

[Severity: Medium]
If the I2C write for the modern standby command fails, does the error path
leave the thermal polling permanently disabled?

The PM core aborts the system suspend on error but does not call the resume
callback for the device that failed suspend, so enable_delayed_work() won't
be called. Should the error path re-enable the delayed work before returning?

>  }
>  
>  static int qcom_ec_probe(struct i2c_client *client)

[ ... ]

> @@ -370,10 +587,24 @@ static int qcom_ec_probe(struct i2c_client *client)
>  	if (ret < 0)
>  		return dev_err_probe(dev, ret, "Failed to enable SCI events\n");
>  
> +	ret = devm_add_action_or_reset(dev, qcom_ec_sci_evt_disable, dev);
> +	if (ret)
> +		return ret;
> +
>  	ret = qcom_ec_thermal_capabilities(dev);
>  	if (ret < 0)
>  		return dev_err_probe(dev, ret, "Failed to read thermal capabilities\n");
>  
> +	ret = qcom_ec_setup_soc_tj_zones(ec);

[Severity: High]
Can an SCI event trigger the IRQ handler before the probe finishes, causing
a NULL pointer dereference?

The IRQ is requested and SCI events are enabled on the EC prior to calling
qcom_ec_setup_soc_tj_zones(), which allocates ec->ec_cdev further down in
the probe function.

If an event fires in this widened window, or if the setup fails and returns
an error, qcom_ec_irq() can execute before devres disables the IRQ. The
handler checks if the fan_id is less than ec->thermal_cap.fan_cnt, which
would be true, and then dereferences the unallocated ec_cdev array.

> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to setup SoC Tj thermal zones\n");
> +
> +	ret = devm_delayed_work_autocancel(dev, &ec->soc_tj_work, qcom_ec_soc_tj_work_fn);
> +	if (ret)
> +		return ret;
> +
> +	queue_delayed_work(system_percpu_wq, &ec->soc_tj_work, EC_SOC_TJ_TEMP_POLL_JIFFIES);

[Severity: Medium]
Could we avoid queueing the work initially if ec->num_soc_tj_zones is 0?

Similar to the requeue path, this starts a background polling process even
if no thermal zones were found during setup.

> +
>  	if (ec->thermal_cap.fan_cnt == 0) {
>  		dev_warn(dev, FW_BUG "Failed to get fan count, firmware update required\n");
>  		return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-ec_add_more_commands-v1-0-771abd65ee1a@oss.qualcomm.com?part=2

[PATCH 3/8] platform: arm64: qcom-hamoa-ec: Switch fan profile based on power supply state

[Anvesh Jain P]

Add the EC command definitions and handler functions for switching and
reading the EC fan profile.

Register a power supply notifier so the driver can react to AC/battery
transitions: on each PSY_EVENT_PROP_CHANGED notification, queue work
that checks power_supply_is_system_supplied() and, if the AC/battery
state has actually changed, switches the fan profile to the
performance profile on AC or the battery-saver profile on battery.
Track the last-applied state in on_ac_power to avoid redundant EC
writes. Run the initial sync once at probe time, and defer the actual
switch to a work item since the notifier callback runs in atomic
context.

Serialize the fan profile write under the existing io_lock mutex,
alongside the other EC command sequences.

Signed-off-by: Anvesh Jain P <anvesh.p@oss.qualcomm.com>
---
 drivers/platform/arm64/qcom-hamoa-ec.c | 158 +++++++++++++++++++++++++++++++++
 1 file changed, 158 insertions(+)

diff --git a/drivers/platform/arm64/qcom-hamoa-ec.c b/drivers/platform/arm64/qcom-hamoa-ec.c
index 4c745b78322c..f6ff77d4e8f6 100644
--- a/drivers/platform/arm64/qcom-hamoa-ec.c
+++ b/drivers/platform/arm64/qcom-hamoa-ec.c
@@ -19,6 +19,7 @@
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/pm.h>
+#include <linux/power_supply.h>
 #include <linux/slab.h>
 #include <linux/thermal.h>
 
@@ -26,10 +27,14 @@
 #define EC_FW_VERSION_CMD	0x0e
 #define EC_SOC_TEMP_CMD	0x20
 #define EC_MODERN_STANDBY_CMD	0x23
+#define EC_FAN_PROFILE_CMD	0x24
 #define EC_FAN_DBG_CONTROL_CMD	0x30
 #define EC_SCI_EVT_CONTROL_CMD	0x35
 #define EC_THERMAL_CAP_CMD	0x42
 
+#define EC_FAN_PROFILE_BEST_PERF_PLUGGED_IN	0x6
+#define EC_FAN_PROFILE_BETTER_BATT_ON_BATT	0x3
+
 #define EC_SOC_TEMP_SOURCE_TJ			0x1
 #define EC_SOC_TEMP_SOURCE_TSKIN		0x2
 #define EC_SOC_TEMP_DATA_SIZE			2
@@ -92,11 +97,14 @@ struct qcom_ec {
 	struct i2c_client *client;
 	struct qcom_ec_cooling_dev *ec_cdev;
 	struct thermal_zone_device **soc_tj_zones;
+	struct notifier_block psy_nb;
 	struct delayed_work soc_tj_work;
+	struct work_struct psy_work;
 	struct mutex io_lock; /* serializes EC command sequences */
 	struct qcom_ec_thermal_cap thermal_cap;
 	struct qcom_ec_version version;
 	int num_soc_tj_zones;
+	int on_ac_power; /* -1 = unknown, 0 = battery, 1 = AC */
 };
 
 static int qcom_ec_read(struct qcom_ec *ec, u8 cmd, u8 resp_len, u8 *resp)
@@ -351,6 +359,140 @@ static int qcom_ec_send_soc_tj_temp(struct qcom_ec *ec)
 	return ret;
 }
 
+/*
+ * SoC to EC Fan Profile switch command:
+ *
+ * Command Payload:
+ * -----------------------------------------------------------------------
+ * | Offset	| Name		| Description				|
+ * -----------------------------------------------------------------------
+ * | 0x00	| Command = 0x24| SoC to EC Fan Profile switch command	|
+ * -----------------------------------------------------------------------
+ * | 0x01	| Fan Profile ID| Bit 0-3: Fan Profile			|
+ * |		|		| 0x0 : Invalid Profile			|
+ * |		|		| 0x1 : Battery saver profile		|
+ * |		|		| 0x2 : Better Battery with charger	|
+ * |		|		|       plugged in			|
+ * |		|		| 0x3 : Better Battery with charger	|
+ * |		|		|       removed				|
+ * |		|		| 0x4 : Better Performance with charger	|
+ * |		|		|       plugged in			|
+ * |		|		| 0x5 : Better Performance with charger	|
+ * |		|		|       plugged out			|
+ * |		|		| 0x6 : Best Performance with charger	|
+ * |		|		|       plugged in (default)		|
+ * |		|		| 0x7 : Best Performance with charger	|
+ * |		|		|       plugged out			|
+ * |		|		| 0x8 - 0xF : Reserved			|
+ * |		|		| Bit 4-7: Reserved			|
+ * -----------------------------------------------------------------------
+ *
+ */
+static int qcom_ec_set_fan_profile(struct qcom_ec *ec, u8 profile_id)
+{
+	int ret;
+
+	ret = i2c_smbus_write_byte_data(ec->client, EC_FAN_PROFILE_CMD, profile_id);
+	if (ret < 0)
+		dev_err(&ec->client->dev, "Failed to set fan profile 0x%x: %d\n",
+			profile_id, ret);
+	else
+		dev_dbg(&ec->client->dev, "Fan profile set to 0x%x\n", profile_id);
+
+	return ret;
+}
+
+/*
+ * SoC to EC Fan Profile read command:
+ *
+ * Read Response:
+ * -----------------------------------------------------------------------
+ * | Offset	| Name		| Description				|
+ * -----------------------------------------------------------------------
+ * | 0x00	| Fan Profile ID| Bit 0-3: Fan Profile			|
+ * |		|		| 0x0 : Invalid Profile			|
+ * |		|		| 0x1 : Battery saver profile		|
+ * |		|		| 0x2 : Better Battery with charger	|
+ * |		|		|       plugged in			|
+ * |		|		| 0x3 : Better Battery with charger	|
+ * |		|		|       removed				|
+ * |		|		| 0x4 : Better Performance with charger	|
+ * |		|		|       plugged in			|
+ * |		|		| 0x5 : Better Performance with charger	|
+ * |		|		|       plugged out			|
+ * |		|		| 0x6 : Best Performance with charger	|
+ * |		|		|       plugged in (default)		|
+ * |		|		| 0x7 : Best Performance with charger	|
+ * |		|		|       plugged out			|
+ * |		|		| 0x8 - 0xF : Reserved			|
+ * |		|		| Bit 4-7: Fan ID			|
+ * |		|		| 0x1 : Fan 1				|
+ * |		|		| 0x2 : Fan 2				|
+ * -----------------------------------------------------------------------
+ *
+ */
+static int qcom_ec_get_fan_profile(struct qcom_ec *ec, u8 *profile_id)
+{
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(ec->client, EC_FAN_PROFILE_CMD);
+	if (ret < 0)
+		return ret;
+
+	*profile_id = (u8)ret & GENMASK(3, 0);
+
+	return 0;
+}
+
+static int qcom_ec_update_profile_from_power_supply(struct qcom_ec *ec)
+{
+	int on_ac_power;
+	u8 profile;
+	int ret = 0;
+
+	on_ac_power = power_supply_is_system_supplied() > 0 ? 1 : 0;
+
+	profile = on_ac_power ? EC_FAN_PROFILE_BEST_PERF_PLUGGED_IN :
+				EC_FAN_PROFILE_BETTER_BATT_ON_BATT;
+
+	mutex_lock(&ec->io_lock);
+
+	if (ec->on_ac_power != on_ac_power) {
+		ret = qcom_ec_set_fan_profile(ec, profile);
+		if (!ret)
+			ec->on_ac_power = on_ac_power;
+	}
+
+	mutex_unlock(&ec->io_lock);
+
+	return ret;
+}
+
+static void qcom_ec_unreg_psy_notifier(void *data)
+{
+	power_supply_unreg_notifier(data);
+}
+
+static void qcom_ec_psy_work_fn(struct work_struct *work)
+{
+	struct qcom_ec *ec = container_of(work, struct qcom_ec, psy_work);
+
+	qcom_ec_update_profile_from_power_supply(ec);
+}
+
+static int qcom_ec_psy_notifier(struct notifier_block *nb,
+				unsigned long event, void *ptr)
+{
+	struct qcom_ec *ec = container_of(nb, struct qcom_ec, psy_nb);
+
+	if (event != PSY_EVENT_PROP_CHANGED)
+		return NOTIFY_DONE;
+
+	queue_work(system_long_wq, &ec->psy_work);
+
+	return NOTIFY_OK;
+}
+
 static irqreturn_t qcom_ec_irq(int irq, void *data)
 {
 	struct qcom_ec *ec = data;
@@ -570,8 +712,13 @@ static int qcom_ec_probe(struct i2c_client *client)
 		return -ENOMEM;
 
 	ec->client = client;
+	ec->on_ac_power = -1;
 	mutex_init(&ec->io_lock);
 
+	ret = devm_work_autocancel(dev, &ec->psy_work, qcom_ec_psy_work_fn);
+	if (ret)
+		return ret;
+
 	ret = devm_request_threaded_irq(dev, client->irq, NULL, qcom_ec_irq,
 					IRQF_ONESHOT, "qcom_ec", ec);
 	if (ret < 0)
@@ -630,6 +777,17 @@ static int qcom_ec_probe(struct i2c_client *client)
 		}
 	}
 
+	ec->psy_nb.notifier_call = qcom_ec_psy_notifier;
+	ret = power_supply_reg_notifier(&ec->psy_nb);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to register power supply notifier\n");
+
+	ret = devm_add_action_or_reset(dev, qcom_ec_unreg_psy_notifier, &ec->psy_nb);
+	if (ret)
+		return ret;
+
+	qcom_ec_update_profile_from_power_supply(ec);
+
 	return 0;
 }
 

-- 
2.34.1

Re: [PATCH 3/8] platform: arm64: qcom-hamoa-ec: Switch fan profile based on power supply state

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing synchronization for `psy_work` during suspend causes I2C transfers in standby state.
- [Low] Missing Kconfig dependency on POWER_SUPPLY causes build failures.
- [Low] Unused static function `qcom_ec_get_fan_profile` triggers compiler warnings.
--

--- Patch [3]: [PATCH 3/8] platform: arm64: qcom-hamoa-ec: Switch fan profile based on power supply state ---
commit 14490e0de249214f9c1b00a4aad8f89c2d05843e
Author: Anvesh Jain P <anvesh.p@oss.qualcomm.com>

platform: arm64: qcom-hamoa-ec: Switch fan profile based on power supply state

This commit adds EC command definitions and handlers to switch the EC fan
profile. It registers a power supply notifier to track AC/battery state
transitions and queues a work item to apply the corresponding fan profile.

> diff --git a/drivers/platform/arm64/qcom-hamoa-ec.c b/drivers/platform/arm64/qcom-hamoa-ec.c
> index 4c745b78322c4..f6ff77d4e8f69 100644
> --- a/drivers/platform/arm64/qcom-hamoa-ec.c
> +++ b/drivers/platform/arm64/qcom-hamoa-ec.c

[ ... ]

> @@ -351,6 +359,140 @@ static int qcom_ec_send_soc_tj_temp(struct qcom_ec *ec)
>  	return ret;
>  }
>  

[ ... ]

> +static int qcom_ec_get_fan_profile(struct qcom_ec *ec, u8 *profile_id)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(ec->client, EC_FAN_PROFILE_CMD);

[Severity: Low]
Is this function called anywhere in the driver? It appears to be added
but never used. 

Because it is static and lacks the __maybe_unused attribute, will this
trigger a -Wunused-function compiler warning?

> +	if (ret < 0)
> +		return ret;
> +
> +	*profile_id = (u8)ret & GENMASK(3, 0);
> +
> +	return 0;
> +}

[ ... ]

> +static int qcom_ec_psy_notifier(struct notifier_block *nb,
> +				unsigned long event, void *ptr)
> +{
> +	struct qcom_ec *ec = container_of(nb, struct qcom_ec, psy_nb);
> +
> +	if (event != PSY_EVENT_PROP_CHANGED)
> +		return NOTIFY_DONE;
> +
> +	queue_work(system_long_wq, &ec->psy_work);

[Severity: High]
Is it safe to leave this work item un-synchronized during suspend?

Looking at the suspend path, soc_tj_work is properly disabled before the EC
enters modern standby, but psy_work is not:

qcom_ec_suspend() {
    disable_delayed_work_sync(&ec->soc_tj_work);

    mutex_lock(&ec->io_lock);
    ret = i2c_smbus_write_byte_data(client, EC_MODERN_STANDBY_CMD,
                                    EC_MODERN_STANDBY_ENTER);
    mutex_unlock(&ec->io_lock);
    ...
}

Since psy_work is queued on system_long_wq (which is not freezable), if a
power supply event like an AC unplug occurs right before or during suspend,
psy_work could execute concurrently with or after the EC is put into standby.

If the work runs, it sends the EC_FAN_PROFILE_CMD over I2C. Will this wake
or corrupt the EC state, or potentially deadlock if the I2C controller has
already suspended? 

Should psy_work be disabled during suspend as well?

> +
> +	return NOTIFY_OK;
> +}

[ ... ]

> @@ -630,6 +777,17 @@ static int qcom_ec_probe(struct i2c_client *client)
>  		}
>  	}
>  
> +	ec->psy_nb.notifier_call = qcom_ec_psy_notifier;
> +	ret = power_supply_reg_notifier(&ec->psy_nb);

[Severity: Low]
Building without CONFIG_POWER_SUPPLY will lead to link errors here because
power_supply_reg_notifier() and power_supply_unreg_notifier() are called
unconditionally, but have no stub implementations in the power_supply.h
header.

Does this driver need a Kconfig dependency on POWER_SUPPLY to prevent build
failures?

> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to register power supply notifier\n");
> +
> +	ret = devm_add_action_or_reset(dev, qcom_ec_unreg_psy_notifier, &ec->psy_nb);
> +	if (ret)
> +		return ret;
> +
> +	qcom_ec_update_profile_from_power_supply(ec);
> +
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-ec_add_more_commands-v1-0-771abd65ee1a@oss.qualcomm.com?part=3

[PATCH 4/8] platform: arm64: qcom-hamoa-ec: Add fan RPM query and LUT calibration

[Anvesh Jain P]

Add the EC command definitions and handler functions for querying fan
RPM and programming per-fan LUTs (lookup tables mapping SoC
temperature to target RPM), and introduce a raw i2c_transfer()-based
transport for the two commands, since their variable-length,
sub-command-addressed payloads don't fit the smbus block-data model
used by the existing commands.

Fan LUT geometry and temperature breakpoints vary by board, so encode
them in per-compatible qcom_ec_lut_config data selected via
device_get_match_data(). Add configs for the Hamoa CRD, Hamoa/Purwa
IOT EVK, and Glymur CRD ECs; IOT EVK boards share the Hamoa LUT
geometry but are bring-up platforms without a calibrated fan curve,
so skip_lut_set suppresses the LUT-set command on those boards.

Since the LUT values depend on each fan's maximum RPM, which varies
per board assembly, calibrate it at runtime: drive the fan to full
PWM, wait for it to spin up, and read back the achieved RPM via the
new query command. Run calibration in a work item at probe time and
after EC reset recovery, and make it abortable via a completion so
suspend can cut it short. Track calibration completion state so the
LUT is programmed once calibration finishes and again on every
subsequent power-supply-driven profile switch.

Guard all of the above behind the presence of lut_cfg, since boards
without match data (or without a populated fan_cnt) have no LUT to
calibrate or program. Add a new state_lock mutex to guard the
per-fan cooling device state (current PWM state and calibrated
max_rpm) now that it can be read and written from more than one
context.

Signed-off-by: Anvesh Jain P <anvesh.p@oss.qualcomm.com>
---
 drivers/platform/arm64/qcom-hamoa-ec.c | 600 ++++++++++++++++++++++++++++++---
 1 file changed, 561 insertions(+), 39 deletions(-)

diff --git a/drivers/platform/arm64/qcom-hamoa-ec.c b/drivers/platform/arm64/qcom-hamoa-ec.c
index f6ff77d4e8f6..d0c45d9970ee 100644
--- a/drivers/platform/arm64/qcom-hamoa-ec.c
+++ b/drivers/platform/arm64/qcom-hamoa-ec.c
@@ -6,6 +6,7 @@
 
 #include <linux/bitfield.h>
 #include <linux/bits.h>
+#include <linux/completion.h>
 #include <linux/delay.h>
 #include <linux/devm-helpers.h>
 #include <linux/device.h>
@@ -20,14 +21,17 @@
 #include <linux/platform_device.h>
 #include <linux/pm.h>
 #include <linux/power_supply.h>
+#include <linux/property.h>
 #include <linux/slab.h>
 #include <linux/thermal.h>
 
 #define EC_SCI_EVT_READ_CMD	0x05
 #define EC_FW_VERSION_CMD	0x0e
 #define EC_SOC_TEMP_CMD	0x20
+#define EC_FAN_RPM_QUERY_CMD	0x22
 #define EC_MODERN_STANDBY_CMD	0x23
 #define EC_FAN_PROFILE_CMD	0x24
+#define EC_SET_FAN_LUT_CMD	0x28
 #define EC_FAN_DBG_CONTROL_CMD	0x30
 #define EC_SCI_EVT_CONTROL_CMD	0x35
 #define EC_THERMAL_CAP_CMD	0x42
@@ -41,8 +45,11 @@
 
 #define EC_FW_VERSION_RESP_LEN	4
 #define EC_THERMAL_CAP_RESP_LEN	3
+#define EC_FAN_RPM_RESP_LEN	3
 #define EC_FAN_DEBUG_CMD_LEN	6
 #define EC_FAN_SPEED_DATA_SIZE	4
+#define EC_LUT_MAX_DATA_SIZE	64
+#define EC_MAX_LUT_ENTRIES	16
 
 #define EC_MODERN_STANDBY_ENTER	0x01
 #define EC_MODERN_STANDBY_EXIT	0x00
@@ -71,6 +78,92 @@ enum qcom_ec_sci_events {
 	EC_RECOVERED_FROM_RESET_EVT = 0x3d,
 };
 
+struct qcom_ec_fan_lut_entry {
+	u8 target_rpm;	/* units of 100 RPM */
+	u8 temp_high;
+	u8 temp_low;
+};
+
+struct qcom_ec_fan_lut_temp_entry {
+	u8 temp_high;
+	u8 temp_low;
+};
+
+struct qcom_ec_lut_config {
+	u8 lut_data_size;
+	u8 lut_entry_stride;
+	u8 max_lut_entries;
+	const struct qcom_ec_fan_lut_temp_entry *temps;
+	u8 num_temps;
+	bool send_soc_tj;
+	bool skip_lut_set;
+};
+
+static const struct qcom_ec_fan_lut_temp_entry hamoa_lut_temps[] = {
+	{ 32,  25  },
+	{ 35,  32  },
+	{ 42,  35  },
+	{ 45,  42  },
+	{ 52,  45  },
+	{ 55,  52  },
+	{ 60,  55  },
+	{ 65,  60  },
+	{ 70,  65  },
+	{ 75,  70  },
+	{ 80,  75  },
+	{ 85,  80  },
+	{ 90,  85  },
+	{ 95,  90  },
+	{ 100, 95  },
+	{ 105, 100 },
+};
+
+static const struct qcom_ec_fan_lut_temp_entry glymur_lut_temps[] = {
+	{ 40, 35 },
+	{ 50, 45 },
+	{ 55, 50 },
+	{ 60, 55 },
+	{ 65, 60 },
+	{ 70, 65 },
+	{ 75, 70 },
+	{ 85, 80 },
+};
+
+static const struct qcom_ec_lut_config hamoa_crd_lut_config = {
+	.lut_data_size            = 64,
+	.lut_entry_stride         = 4,
+	.max_lut_entries          = 16,
+	.temps                    = hamoa_lut_temps,
+	.num_temps                = ARRAY_SIZE(hamoa_lut_temps),
+	.send_soc_tj              = true,
+};
+
+/*
+ * IOT EVK boards share the Hamoa LUT geometry but are bring-up platforms
+ * without a calibrated fan curve, so the LUT-set command is not issued.
+ */
+static const struct qcom_ec_lut_config hamoa_iot_lut_config = {
+	.lut_data_size            = 64,
+	.lut_entry_stride         = 4,
+	.max_lut_entries          = 16,
+	.temps                    = hamoa_lut_temps,
+	.num_temps                = ARRAY_SIZE(hamoa_lut_temps),
+	.send_soc_tj              = true,
+	.skip_lut_set             = true,
+};
+
+static const struct qcom_ec_lut_config glymur_lut_config = {
+	.lut_data_size            = 24,
+	.lut_entry_stride         = 3,
+	.max_lut_entries          = 8,
+	.temps                    = glymur_lut_temps,
+	.num_temps                = ARRAY_SIZE(glymur_lut_temps),
+	.send_soc_tj              = false,
+};
+
+static_assert(ARRAY_SIZE(hamoa_lut_temps) <= EC_MAX_LUT_ENTRIES);
+static_assert(ARRAY_SIZE(glymur_lut_temps) <= EC_MAX_LUT_ENTRIES);
+
 struct qcom_ec_version {
 	u8 main_version;
 	u8 sub_version;
@@ -91,36 +184,125 @@ struct qcom_ec_cooling_dev {
 	struct device *parent_dev;
 	u8 fan_id;
 	u8 state;
+	u16 max_rpm;
+	struct qcom_ec_fan_lut_entry lut_entries[EC_MAX_LUT_ENTRIES];
 };
 
 struct qcom_ec {
 	struct i2c_client *client;
+	const struct qcom_ec_lut_config *lut_cfg;
 	struct qcom_ec_cooling_dev *ec_cdev;
 	struct thermal_zone_device **soc_tj_zones;
 	struct notifier_block psy_nb;
 	struct delayed_work soc_tj_work;
+	struct work_struct fan_calib_work;
 	struct work_struct psy_work;
-	struct mutex io_lock; /* serializes EC command sequences */
+	struct completion calib_abort;
+	struct mutex io_lock; /* serializes EC command sequences and calibrating */
+	struct mutex state_lock; /* guards per-fan cooling device state */
 	struct qcom_ec_thermal_cap thermal_cap;
 	struct qcom_ec_version version;
 	int num_soc_tj_zones;
+	u8 fan_profile;
 	int on_ac_power; /* -1 = unknown, 0 = battery, 1 = AC */
+	bool calibrating;
+	bool calib_done;
 };
 
-static int qcom_ec_read(struct qcom_ec *ec, u8 cmd, u8 resp_len, u8 *resp)
+static int qcom_ec_write(struct qcom_ec *ec, u8 cmd, const u8 *data, size_t len)
 {
+	struct i2c_msg msg;
 	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 (len > U16_MAX - 1)
+		return -EINVAL;
 
-	if (resp[0] >= resp_len)
+	if (!data && len)
 		return -EINVAL;
 
-	return 0;
+	msg = (struct i2c_msg){
+		.addr  = ec->client->addr,
+		.flags = I2C_M_DMA_SAFE,
+		.len   = 1 + len,
+	};
+
+	msg.buf = kmalloc(1 + len, GFP_KERNEL);
+	if (!msg.buf)
+		return -ENOMEM;
+
+	msg.buf[0] = cmd;
+	memcpy(&msg.buf[1], data, len);
+
+	ret = i2c_transfer(ec->client->adapter, &msg, 1);
+
+	kfree(msg.buf);
+
+	return ret == 1 ? 0 : (ret < 0 ? ret : -EIO);
+}
+
+static int qcom_ec_read(struct qcom_ec *ec, u8 cmd, const u8 *subcmd,
+			u8 subcmd_len, u8 resp_len, u8 *resp)
+{
+	struct i2c_client *client = ec->client;
+	struct i2c_msg msgs[2];
+	u8 *write_buf, *read_buf;
+	int ret;
+
+	if (subcmd_len > 3)
+		return -EINVAL;
+
+	if (!subcmd && subcmd_len)
+		return -EINVAL;
+
+	if (!resp || !resp_len)
+		return -EINVAL;
+
+	write_buf = kmalloc(1 + subcmd_len, GFP_KERNEL);
+	if (!write_buf)
+		return -ENOMEM;
+
+	read_buf = kmalloc(resp_len, GFP_KERNEL);
+	if (!read_buf) {
+		kfree(write_buf);
+		return -ENOMEM;
+	}
+
+	write_buf[0] = cmd;
+	if (subcmd && subcmd_len)
+		memcpy(&write_buf[1], subcmd, subcmd_len);
+
+	msgs[0].addr  = client->addr;
+	msgs[0].flags = I2C_M_DMA_SAFE;
+	msgs[0].len   = 1 + subcmd_len;
+	msgs[0].buf   = write_buf;
+
+	msgs[1].addr  = client->addr;
+	msgs[1].flags = I2C_M_RD | I2C_M_DMA_SAFE;
+	msgs[1].len   = resp_len;
+	msgs[1].buf   = read_buf;
+
+	ret = i2c_transfer(client->adapter, msgs, 2);
+	if (ret != 2) {
+		ret = ret < 0 ? ret : -EIO;
+		goto out;
+	}
+
+	if (read_buf[0] == 0 || read_buf[0] == 0xff) {
+		ret = -EOPNOTSUPP;
+		goto out;
+	}
+
+	if (read_buf[0] != resp_len - 1) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	memcpy(resp, read_buf, resp_len);
+	ret = 0;
+out:
+	kfree(write_buf);
+	kfree(read_buf);
+	return ret;
 }
 
 /*
@@ -150,7 +332,7 @@ static int qcom_ec_read_fw_version(struct device *dev)
 	int ret;
 
 	mutex_lock(&ec->io_lock);
-	ret = qcom_ec_read(ec, EC_FW_VERSION_CMD, EC_FW_VERSION_RESP_LEN, resp);
+	ret = qcom_ec_read(ec, EC_FW_VERSION_CMD, NULL, 0, EC_FW_VERSION_RESP_LEN, resp);
 	mutex_unlock(&ec->io_lock);
 	if (ret < 0)
 		return ret;
@@ -194,7 +376,7 @@ static int qcom_ec_thermal_capabilities(struct device *dev)
 	int ret;
 
 	mutex_lock(&ec->io_lock);
-	ret = qcom_ec_read(ec, EC_THERMAL_CAP_CMD, EC_THERMAL_CAP_RESP_LEN, resp);
+	ret = qcom_ec_read(ec, EC_THERMAL_CAP_CMD, NULL, 0, EC_THERMAL_CAP_RESP_LEN, resp);
 	mutex_unlock(&ec->io_lock);
 	if (ret < 0)
 		return ret;
@@ -444,6 +626,133 @@ static int qcom_ec_get_fan_profile(struct qcom_ec *ec, u8 *profile_id)
 	return 0;
 }
 
+/*
+ * Setting a Fan LUT from SoC to EC:
+ *
+ * Request (SoC -> EC):
+ * -------------------------------------------------------------------------------
+ * | Offset		| Name		| Description				|
+ * -------------------------------------------------------------------------------
+ * | 0x00		| Command = 0x28| Command to set Fan LUT from EC	|
+ * -------------------------------------------------------------------------------
+ * | 0x01		| Fan and	| Bit 0-3: Fan ID (1 - 2)		|
+ * |			| Profile ID	| Bit 4-7: Fan Profile ID (1 - 7)	|
+ * -------------------------------------------------------------------------------
+ * | 0x02		| Temp Source	| 0x1 : SoC Tj				|
+ * |			|		| 0x2 : Tskin				|
+ * |			|		| 0x3 - 0xFF : Reserved			|
+ * -------------------------------------------------------------------------------
+ * | 0x03		| Byte count=64	| EC LUT payload byte count		|
+ * -------------------------------------------------------------------------------
+ * | 0x04 - (0x04 +	| EC LUT data	| N* entries x 3 bytes:			|
+ * | (N* x 3))		|		| Byte 1: Target RPM (units of 100 RPM)	|
+ * |			|		| Byte 2: Temp High			|
+ * |			|		| Byte 3: Temp Low			|
+ * -------------------------------------------------------------------------------
+ *
+ * N* - Indicates the number of entries in the LUT.
+ *
+ */
+static int qcom_ec_set_fan_lut(struct qcom_ec *ec, u8 fan_id, u8 profile_id,
+			       u8 temp_source, const struct qcom_ec_fan_lut_entry *entries)
+{
+	const struct qcom_ec_lut_config *lut_cfg = ec->lut_cfg;
+	u8 write_buf[3 + EC_LUT_MAX_DATA_SIZE] = {};
+	int i, ret;
+
+	if (!lut_cfg)
+		return -EOPNOTSUPP;
+
+	if (lut_cfg->max_lut_entries == 0 ||
+	    lut_cfg->lut_entry_stride < 3 ||
+	    lut_cfg->lut_data_size > EC_LUT_MAX_DATA_SIZE ||
+	    (lut_cfg->max_lut_entries - 1) * lut_cfg->lut_entry_stride + 3 > lut_cfg->lut_data_size)
+		return -EINVAL;
+
+	write_buf[0] = (fan_id & 0x0f) | ((profile_id & 0x0f) << 4);
+	write_buf[1] = temp_source;
+	write_buf[2] = lut_cfg->lut_data_size;
+
+	for (i = 0; i < lut_cfg->max_lut_entries; i++) {
+		u8 *dst = &write_buf[3 + i * lut_cfg->lut_entry_stride];
+
+		dst[0] = entries[i].target_rpm;
+		dst[1] = entries[i].temp_high;
+		dst[2] = entries[i].temp_low;
+	}
+
+	ret = qcom_ec_write(ec, EC_SET_FAN_LUT_CMD, write_buf, 3 + lut_cfg->lut_data_size);
+	if (ret < 0)
+		dev_err(&ec->client->dev, "Failed to set fan LUT: %d\n", ret);
+
+	return ret;
+}
+
+/*
+ * EC Fan RPM Query:
+ *
+ * Request (SoC -> EC):
+ * ---------------------------------------------------------------
+ * | Offset	| Name		| Description			|
+ * ---------------------------------------------------------------
+ * | 0x00	| Command = 0x22| EC Fan RPM Query		|
+ * ---------------------------------------------------------------
+ * | 0x01	| Fan ID	| 0x1 : Fan 1			|
+ * |		|		| 0x2 : Fan 2			|
+ * ---------------------------------------------------------------
+ *
+ * Response (EC -> SoC):
+ * ---------------------------------------------------------------
+ * | Offset	| Name		| Description			|
+ * ---------------------------------------------------------------
+ * | 0x00	| Byte count = 2| Byte count for fan RPM value	|
+ * ---------------------------------------------------------------
+ * | 0x01 (LSB)| Fan Speed	| Fan speed in the resolution	|
+ * | 0x02	|		| of 1 RPM			|
+ * ---------------------------------------------------------------
+ */
+static int qcom_ec_get_fan_rpm(struct qcom_ec *ec, u8 fan_id, u16 *rpm)
+{
+	u8 resp[EC_FAN_RPM_RESP_LEN];
+	int ret;
+
+	ret = qcom_ec_read(ec, EC_FAN_RPM_QUERY_CMD, &fan_id, 1, sizeof(resp), resp);
+	if (ret < 0)
+		return ret;
+
+	*rpm = resp[1] | (resp[2] << 8);
+
+	return 0;
+}
+
+static int qcom_ec_set_lut_for_profile(struct qcom_ec *ec, u8 profile_id)
+{
+	const struct qcom_ec_lut_config *lut_cfg = ec->lut_cfg;
+	int i, ret;
+
+	if (!lut_cfg)
+		return -EOPNOTSUPP;
+
+	if (lut_cfg->skip_lut_set)
+		return 0;
+
+	for (i = 0; i < ec->thermal_cap.fan_cnt; i++) {
+		struct qcom_ec_cooling_dev *ec_cdev = &ec->ec_cdev[i];
+
+		ret = qcom_ec_set_fan_lut(ec, ec_cdev->fan_id, profile_id,
+					  EC_SOC_TEMP_SOURCE_TJ,
+					  ec_cdev->lut_entries);
+		if (ret < 0) {
+			dev_err(&ec->client->dev,
+				"Failed to set LUT for fan%u profile 0x%x: %d\n",
+				ec_cdev->fan_id, profile_id, ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
 static int qcom_ec_update_profile_from_power_supply(struct qcom_ec *ec)
 {
 	int on_ac_power;
@@ -457,8 +766,10 @@ static int qcom_ec_update_profile_from_power_supply(struct qcom_ec *ec)
 
 	mutex_lock(&ec->io_lock);
 
-	if (ec->on_ac_power != on_ac_power) {
+	if (ec->on_ac_power != on_ac_power && !ec->calibrating) {
 		ret = qcom_ec_set_fan_profile(ec, profile);
+		if (!ret && ec->lut_cfg && ec->calib_done)
+			ret = qcom_ec_set_lut_for_profile(ec, profile);
 		if (!ret)
 			ec->on_ac_power = on_ac_power;
 	}
@@ -473,6 +784,24 @@ static void qcom_ec_unreg_psy_notifier(void *data)
 	power_supply_unreg_notifier(data);
 }
 
+static void qcom_ec_restore_fan_state(struct qcom_ec *ec)
+{
+	bool calib_done;
+
+	if (!ec->lut_cfg || !ec->thermal_cap.fan_cnt)
+		return;
+
+	mutex_lock(&ec->io_lock);
+	ec->on_ac_power = -1;
+	calib_done = ec->calib_done;
+	mutex_unlock(&ec->io_lock);
+
+	qcom_ec_update_profile_from_power_supply(ec);
+
+	if (!calib_done)
+		queue_work(system_long_wq, &ec->fan_calib_work);
+}
+
 static void qcom_ec_psy_work_fn(struct work_struct *work)
 {
 	struct qcom_ec *ec = container_of(work, struct qcom_ec, psy_work);
@@ -537,6 +866,7 @@ static irqreturn_t qcom_ec_irq(int irq, void *data)
 		break;
 	case EC_RECOVERED_FROM_RESET_EVT:
 		dev_dbg_ratelimited(dev, "EC recovered from reset\n");
+		qcom_ec_restore_fan_state(ec);
 		break;
 	default:
 		dev_notice_ratelimited(dev, "Unknown EC event: %d\n", val);
@@ -569,20 +899,30 @@ static void qcom_ec_sci_evt_disable(void *data)
 		dev_err(dev, "Failed to disable SCI events: %d\n", ret);
 }
 
-static int qcom_ec_fan_get_max_state(struct thermal_cooling_device *cdev, unsigned long *state)
-{
-	*state = EC_FAN_MAX_PWM;
-
-	return 0;
-}
-
 static int qcom_ec_fan_get_cur_state(struct thermal_cooling_device *cdev, unsigned long *state)
 {
 	struct qcom_ec_cooling_dev *ec_cdev = cdev->devdata;
+	struct i2c_client *client = to_i2c_client(ec_cdev->parent_dev);
+	struct qcom_ec *ec = i2c_get_clientdata(client);
+	struct device *dev = ec_cdev->parent_dev;
+	u16 rpm;
+	int ret;
+
+	mutex_lock(&ec->io_lock);
+	ret = qcom_ec_get_fan_rpm(ec, ec_cdev->fan_id, &rpm);
+	mutex_unlock(&ec->io_lock);
+	if (ret < 0)
+		dev_err(dev, "Failed to read fan%u RPM: %d\n", ec_cdev->fan_id, ret);
 
+	mutex_lock(&ec->state_lock);
+
+	if (ret >= 0 && ec_cdev->max_rpm)
+		ec_cdev->state = min_t(u32, (rpm * EC_FAN_MAX_PWM) / ec_cdev->max_rpm,
+				       EC_FAN_MAX_PWM);
 	*state = ec_cdev->state;
+	mutex_unlock(&ec->state_lock);
 
-	return 0;
+	return ret < 0 ? ret : 0;
 }
 
 /*
@@ -628,24 +968,116 @@ static int qcom_ec_fan_debug_mode_off(struct qcom_ec_cooling_dev *ec_cdev)
 	return ret;
 }
 
-static int qcom_ec_fan_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state)
+static int qcom_ec_fan_write_pwm(struct qcom_ec_cooling_dev *ec_cdev, u8 pwm)
 {
-	struct qcom_ec_cooling_dev *ec_cdev = cdev->devdata;
 	struct device *dev = ec_cdev->parent_dev;
 	struct i2c_client *client = to_i2c_client(dev);
+	struct qcom_ec *ec = i2c_get_clientdata(client);
 	u8 request[6] = { ec_cdev->fan_id, EC_FAN_SPEED_DATA_SIZE,
 			  EC_FAN_DEBUG_MODE_ON | EC_FAN_ON | EC_FAN_DEBUG_TYPE_PWM,
-			  0, 0, state };
+			  0, 0, pwm };
 	int ret;
 
-	ret = i2c_smbus_write_i2c_block_data(client, EC_FAN_DBG_CONTROL_CMD,
-					     sizeof(request), request);
-	if (ret) {
+	ret = qcom_ec_write(ec, EC_FAN_DBG_CONTROL_CMD, request, sizeof(request));
+	if (ret)
 		dev_err(dev, "Failed to set fan pwm: %d\n", ret);
+
+	return ret;
+}
+
+static int qcom_ec_fan_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state)
+{
+	struct qcom_ec_cooling_dev *ec_cdev = cdev->devdata;
+	struct i2c_client *client = to_i2c_client(ec_cdev->parent_dev);
+	struct qcom_ec *ec = i2c_get_clientdata(client);
+	int ret;
+
+	if (state > EC_FAN_MAX_PWM)
+		return -EINVAL;
+
+	mutex_lock(&ec->io_lock);
+
+	if (ec->calibrating) {
+		mutex_unlock(&ec->io_lock);
+		return -EBUSY;
+	}
+
+	ret = qcom_ec_fan_write_pwm(ec_cdev, state);
+	if (!ret) {
+		mutex_lock(&ec->state_lock);
+		ec_cdev->state = state;
+		mutex_unlock(&ec->state_lock);
+	}
+
+	mutex_unlock(&ec->io_lock);
+
+	return ret;
+}
+
+static int qcom_ec_fan_calibrate(struct qcom_ec *ec, struct qcom_ec_cooling_dev *ec_cdev)
+{
+	struct device *dev = ec_cdev->parent_dev;
+	u16 rpm;
+	int ret;
+
+	mutex_lock(&ec->io_lock);
+	ret = qcom_ec_fan_write_pwm(ec_cdev, EC_FAN_MAX_PWM);
+	mutex_unlock(&ec->io_lock);
+	if (ret) {
+		dev_err(dev, "Failed to set fan%u to max state: %d\n", ec_cdev->fan_id, ret);
 		return ret;
 	}
 
-	ec_cdev->state = state;
+	mutex_lock(&ec->state_lock);
+	ec_cdev->state = EC_FAN_MAX_PWM;
+	mutex_unlock(&ec->state_lock);
+
+	ret = wait_for_completion_interruptible_timeout(&ec->calib_abort, msecs_to_jiffies(10000));
+
+	mutex_lock(&ec->io_lock);
+
+	if (ret != 0) {
+		qcom_ec_fan_debug_mode_off(ec_cdev);
+		mutex_unlock(&ec->io_lock);
+		return ret < 0 ? ret : -ECANCELED;
+	}
+
+	ret = qcom_ec_get_fan_rpm(ec, ec_cdev->fan_id, &rpm);
+	if (ret) {
+		dev_err(dev, "Failed to read fan%u RPM at max state: %d\n", ec_cdev->fan_id, ret);
+		qcom_ec_fan_debug_mode_off(ec_cdev);
+		mutex_unlock(&ec->io_lock);
+		return ret;
+	}
+
+	if (!rpm) {
+		dev_err(dev, "Fan%u reported 0 RPM at max state, fan may be blocked or faulty\n",
+			ec_cdev->fan_id);
+		qcom_ec_fan_debug_mode_off(ec_cdev);
+		mutex_unlock(&ec->io_lock);
+		return -ENODEV;
+	}
+
+	qcom_ec_fan_debug_mode_off(ec_cdev);
+	mutex_unlock(&ec->io_lock);
+
+	mutex_lock(&ec->state_lock);
+	ec_cdev->max_rpm = rpm;
+
+	for (int i = 0; i < ec->lut_cfg->max_lut_entries; i++) {
+		ec_cdev->lut_entries[i].target_rpm =
+			min_t(u32, rpm * i / (ec->lut_cfg->max_lut_entries - 1) / 100, U8_MAX);
+		ec_cdev->lut_entries[i].temp_high = ec->lut_cfg->temps[i].temp_high;
+		ec_cdev->lut_entries[i].temp_low  = ec->lut_cfg->temps[i].temp_low;
+	}
+	mutex_unlock(&ec->state_lock);
+
+	return 0;
+}
+
+static int qcom_ec_fan_get_max_state(struct thermal_cooling_device *cdev, unsigned long *state)
+{
+	*state = EC_FAN_MAX_PWM;
 
 	return 0;
 }
@@ -665,6 +1097,49 @@ static void qcom_ec_soc_tj_work_fn(struct work_struct *work)
 	queue_delayed_work(system_percpu_wq, &ec->soc_tj_work, EC_SOC_TJ_TEMP_POLL_JIFFIES);
 }
 
+static void qcom_ec_fan_calib_work_fn(struct work_struct *work)
+{
+	struct qcom_ec *ec = container_of(work, struct qcom_ec, fan_calib_work);
+	struct device *dev = &ec->client->dev;
+	int i, ret;
+
+	reinit_completion(&ec->calib_abort);
+
+	mutex_lock(&ec->io_lock);
+	ec->calibrating = true;
+	mutex_unlock(&ec->io_lock);
+
+	for (i = 0; i < ec->thermal_cap.fan_cnt; i++) {
+		ret = qcom_ec_fan_calibrate(ec, &ec->ec_cdev[i]);
+		if (ret) {
+			dev_err(dev, "Failed to calibrate fan%d: %d\n", i, ret);
+			goto out;
+		}
+	}
+
+	mutex_lock(&ec->io_lock);
+	ret = qcom_ec_get_fan_profile(ec, &ec->fan_profile);
+	if (ret < 0) {
+		dev_err(dev, "Failed to read current fan profile: %d\n", ret);
+		mutex_unlock(&ec->io_lock);
+		goto out;
+	}
+
+	ec->calib_done = true;
+	ret = qcom_ec_set_lut_for_profile(ec, ec->fan_profile);
+	mutex_unlock(&ec->io_lock);
+
+	if (ret < 0)
+		dev_err(dev, "Failed to set LUT for profile 0x%x: %d\n", ec->fan_profile, ret);
+
+out:
+	mutex_lock(&ec->io_lock);
+	ec->calibrating = false;
+	mutex_unlock(&ec->io_lock);
+
+	qcom_ec_update_profile_from_power_supply(ec);
+}
+
 static int qcom_ec_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
@@ -678,9 +1153,13 @@ static int qcom_ec_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	enable_delayed_work(&ec->soc_tj_work);
-	queue_delayed_work(system_percpu_wq, &ec->soc_tj_work, EC_SOC_TJ_TEMP_POLL_JIFFIES);
+	if (ec->lut_cfg && ec->thermal_cap.fan_cnt && !ec->calib_done)
+		queue_work(system_long_wq, &ec->fan_calib_work);
 
+	if (ec->lut_cfg && ec->lut_cfg->send_soc_tj) {
+		enable_delayed_work(&ec->soc_tj_work);
+		queue_delayed_work(system_percpu_wq, &ec->soc_tj_work, EC_SOC_TJ_TEMP_POLL_JIFFIES);
+	}
 	return 0;
 }
 
@@ -690,7 +1169,16 @@ static int qcom_ec_suspend(struct device *dev)
 	struct qcom_ec *ec = i2c_get_clientdata(client);
 	int ret;
 
-	disable_delayed_work_sync(&ec->soc_tj_work);
+	if (ec->lut_cfg && ec->thermal_cap.fan_cnt) {
+		complete_all(&ec->calib_abort);
+		cancel_work_sync(&ec->fan_calib_work);
+	}
+
+	if (ec->lut_cfg)
+		cancel_work_sync(&ec->psy_work);
+
+	if (ec->lut_cfg && ec->lut_cfg->send_soc_tj)
+		disable_delayed_work_sync(&ec->soc_tj_work);
 
 	mutex_lock(&ec->io_lock);
 	ret = i2c_smbus_write_byte_data(client, EC_MODERN_STANDBY_CMD,
@@ -714,6 +1202,22 @@ static int qcom_ec_probe(struct i2c_client *client)
 	ec->client = client;
 	ec->on_ac_power = -1;
 	mutex_init(&ec->io_lock);
+	mutex_init(&ec->state_lock);
+	init_completion(&ec->calib_abort);
+	ec->lut_cfg = device_get_match_data(dev);
+
+	if (ec->lut_cfg &&
+	    (ec->lut_cfg->max_lut_entries < 2 ||
+	     ec->lut_cfg->max_lut_entries > EC_MAX_LUT_ENTRIES ||
+	     ec->lut_cfg->max_lut_entries > ec->lut_cfg->num_temps))
+		return dev_err_probe(dev, -EINVAL,
+				     "Invalid LUT config: max_lut_entries=%u, num_temps=%u, limit=%d\n",
+				     ec->lut_cfg->max_lut_entries, ec->lut_cfg->num_temps,
+				     EC_MAX_LUT_ENTRIES);
+
+	ret = devm_work_autocancel(dev, &ec->fan_calib_work, qcom_ec_fan_calib_work_fn);
+	if (ret)
+		return ret;
 
 	ret = devm_work_autocancel(dev, &ec->psy_work, qcom_ec_psy_work_fn);
 	if (ret)
@@ -742,15 +1246,18 @@ static int qcom_ec_probe(struct i2c_client *client)
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "Failed to read thermal capabilities\n");
 
-	ret = qcom_ec_setup_soc_tj_zones(ec);
-	if (ret < 0)
-		return dev_err_probe(dev, ret, "Failed to setup SoC Tj thermal zones\n");
+	if (ec->lut_cfg && ec->lut_cfg->send_soc_tj) {
+		ret = qcom_ec_setup_soc_tj_zones(ec);
+		if (ret < 0)
+			return dev_err_probe(dev, ret, "Failed to setup SoC Tj thermal zones\n");
 
-	ret = devm_delayed_work_autocancel(dev, &ec->soc_tj_work, qcom_ec_soc_tj_work_fn);
-	if (ret)
-		return ret;
+		ret = devm_delayed_work_autocancel(dev, &ec->soc_tj_work,
+						   qcom_ec_soc_tj_work_fn);
+		if (ret)
+			return ret;
 
-	queue_delayed_work(system_percpu_wq, &ec->soc_tj_work, EC_SOC_TJ_TEMP_POLL_JIFFIES);
+		queue_delayed_work(system_percpu_wq, &ec->soc_tj_work, EC_SOC_TJ_TEMP_POLL_JIFFIES);
+	}
 
 	if (ec->thermal_cap.fan_cnt == 0) {
 		dev_warn(dev, FW_BUG "Failed to get fan count, firmware update required\n");
@@ -777,6 +1284,9 @@ static int qcom_ec_probe(struct i2c_client *client)
 		}
 	}
 
+	if (!ec->lut_cfg)
+		return 0;
+
 	ec->psy_nb.notifier_call = qcom_ec_psy_notifier;
 	ret = power_supply_reg_notifier(&ec->psy_nb);
 	if (ret)
@@ -788,6 +1298,7 @@ static int qcom_ec_probe(struct i2c_client *client)
 
 	qcom_ec_update_profile_from_power_supply(ec);
 
+	queue_work(system_long_wq, &ec->fan_calib_work);
 	return 0;
 }
 
@@ -795,17 +1306,28 @@ static void qcom_ec_remove(struct i2c_client *client)
 {
 	struct qcom_ec *ec = i2c_get_clientdata(client);
 
-	disable_delayed_work_sync(&ec->soc_tj_work);
+	if (ec->lut_cfg && ec->thermal_cap.fan_cnt) {
+		complete_all(&ec->calib_abort);
+		disable_work_sync(&ec->fan_calib_work);
+	}
+
+	if (ec->lut_cfg && ec->lut_cfg->send_soc_tj)
+		disable_delayed_work_sync(&ec->soc_tj_work);
 
 	for (int i = 0; i < ec->thermal_cap.fan_cnt; i++) {
 		struct qcom_ec_cooling_dev *ec_cdev = &ec->ec_cdev[i];
 
+		mutex_lock(&ec->io_lock);
 		qcom_ec_fan_debug_mode_off(ec_cdev);
+		mutex_unlock(&ec->io_lock);
 	}
 }
 
 static const struct of_device_id qcom_ec_of_match[] = {
-	{ .compatible = "qcom,hamoa-crd-ec" },
+	{ .compatible = "qcom,hamoa-iot-evk-ec", .data = &hamoa_iot_lut_config },
+	{ .compatible = "qcom,purwa-iot-evk-ec", .data = &hamoa_iot_lut_config },
+	{ .compatible = "qcom,hamoa-crd-ec",     .data = &hamoa_crd_lut_config },
+	{ .compatible = "qcom,glymur-crd-ec",    .data = &glymur_lut_config    },
 	{}
 };
 MODULE_DEVICE_TABLE(of, qcom_ec_of_match);

-- 
2.34.1

Re: [PATCH 4/8] platform: arm64: qcom-hamoa-ec: Add fan RPM query and LUT calibration

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Race condition between suspend/remove paths and `fan_calib_work_fn` can cause a 10 to 20-second hang.
- [High] `psy_work` can be queued and executed while the device is in suspend, leading to I2C transfer failures.
--

--- Patch [4]: [PATCH 4/8] platform: arm64: qcom-hamoa-ec: Add fan RPM query and LUT calibration ---
commit e49733d2c63b28585a159eb3f465f9613c0aec7d
Author: Anvesh Jain P <anvesh.p@oss.qualcomm.com>

platform: arm64: qcom-hamoa-ec: Add fan RPM query and LUT calibration

This commit adds EC command definitions and handler functions for querying
fan RPM and programming per-fan LUTs mapping SoC temperature to target RPM.
It implements a runtime calibration work item to determine the maximum RPM,
and programs the LUT once calibration finishes and upon power-supply changes.

> diff --git a/drivers/platform/arm64/qcom-hamoa-ec.c b/drivers/platform/arm64/qcom-hamoa-ec.c
> index f6ff77d4e8f69..d0c45d9970eee 100644
> --- a/drivers/platform/arm64/qcom-hamoa-ec.c
> +++ b/drivers/platform/arm64/qcom-hamoa-ec.c
[ ... ]
> @@ -665,6 +1097,49 @@ static void qcom_ec_soc_tj_work_fn(struct work_struct *work)
>  	queue_delayed_work(system_percpu_wq, &ec->soc_tj_work, EC_SOC_TJ_TEMP_POLL_JIFFIES);
>  }
>  
> +static void qcom_ec_fan_calib_work_fn(struct work_struct *work)
> +{
> +	struct qcom_ec *ec = container_of(work, struct qcom_ec, fan_calib_work);
> +	struct device *dev = &ec->client->dev;
> +	int i, ret;
> +
> +	reinit_completion(&ec->calib_abort);

[Severity: High]
Can this race with the suspend or remove paths and cause a long delay?

If qcom_ec_fan_calib_work_fn() begins executing concurrently with
qcom_ec_suspend() or qcom_ec_remove(), the suspend/remove thread calls
complete_all(&ec->calib_abort) to signal the worker to abort.

However, if the worker executes reinit_completion() immediately after the
suspend thread's complete_all(), the abort signal is erased. The suspend
thread then blocks in cancel_work_sync(), waiting for the worker to finish,
and the worker will block in wait_for_completion_interruptible_timeout()
for the full timeout per fan (up to 10-20 seconds total).

> +
> +	mutex_lock(&ec->io_lock);
> +	ec->calibrating = true;
> +	mutex_unlock(&ec->io_lock);
> +
[ ... ]
> @@ -690,7 +1169,16 @@ static int qcom_ec_suspend(struct device *dev)
>  	struct qcom_ec *ec = i2c_get_clientdata(client);
>  	int ret;
>  
> -	disable_delayed_work_sync(&ec->soc_tj_work);
> +	if (ec->lut_cfg && ec->thermal_cap.fan_cnt) {
> +		complete_all(&ec->calib_abort);
> +		cancel_work_sync(&ec->fan_calib_work);
> +	}
> +
> +	if (ec->lut_cfg)
> +		cancel_work_sync(&ec->psy_work);

[Severity: High]
Does this allow psy_work to be re-queued and executed during suspend?

While cancel_work_sync() stops currently pending work, the system-wide
power supply notifier qcom_ec_psy_notifier() is not unregistered during
suspend. 

If a power supply event occurs during the suspend process after
cancel_work_sync() has run, the notifier can immediately queue psy_work
back to the non-freezable system_long_wq.

This could cause the work to run and attempt I2C transfers while the
I2C controller is suspended or the EC is in Modern Standby, potentially
leading to bus lockups or suspend failures.

> +
> +	if (ec->lut_cfg && ec->lut_cfg->send_soc_tj)
> +		disable_delayed_work_sync(&ec->soc_tj_work);
>  
>  	mutex_lock(&ec->io_lock);
>  	ret = i2c_smbus_write_byte_data(client, EC_MODERN_STANDBY_CMD,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-ec_add_more_commands-v1-0-771abd65ee1a@oss.qualcomm.com?part=4

[PATCH 5/8] platform: arm64: qcom-hamoa-ec: Verify required I2C adapter functionality

[Anvesh Jain P]

Check the underlying I2C adapter supports both the raw I2C transfers
used by the fan RPM/LUT commands and the SMBus byte and block-write
transfers used by the other EC commands, and fail probe early with a
clear error if it doesn't.

Signed-off-by: Anvesh Jain P <anvesh.p@oss.qualcomm.com>
---
 drivers/platform/arm64/qcom-hamoa-ec.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/platform/arm64/qcom-hamoa-ec.c b/drivers/platform/arm64/qcom-hamoa-ec.c
index d0c45d9970ee..a420a180a194 100644
--- a/drivers/platform/arm64/qcom-hamoa-ec.c
+++ b/drivers/platform/arm64/qcom-hamoa-ec.c
@@ -1195,6 +1195,11 @@ static int qcom_ec_probe(struct i2c_client *client)
 	unsigned int i;
 	int ret;
 
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_I2C | I2C_FUNC_SMBUS_BYTE_DATA |
+				     I2C_FUNC_SMBUS_WRITE_I2C_BLOCK))
+		return -EOPNOTSUPP;
+
 	ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL);
 	if (!ec)
 		return -ENOMEM;

-- 
2.34.1

Re: [PATCH 5/8] platform: arm64: qcom-hamoa-ec: Verify required I2C adapter functionality

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The IRQ handler is requested and SCI events are enabled before the `ec->ec_cdev` array is allocated, creating a race condition that can lead to a NULL pointer dereference during `qcom_ec_probe()`.
--

--- Patch [5]: [PATCH 5/8] platform: arm64: qcom-hamoa-ec: Verify required I2C adapter functionality ---
commit a6f49e31946f2ee0f2b44aae1db005b085a4d77b
Author: Anvesh Jain P <anvesh.p@oss.qualcomm.com>

platform: arm64: qcom-hamoa-ec: Verify required I2C adapter functionality

Check the underlying I2C adapter supports both the raw I2C transfers
used by the fan RPM/LUT commands and the SMBus byte and block-write
transfers used by the other EC commands, and fail probe early with a
clear error if it doesn't.

> diff --git a/drivers/platform/arm64/qcom-hamoa-ec.c b/drivers/platform/arm64/qcom-hamoa-ec.c
> index d0c45d9970eee..a420a180a1948 100644
> --- a/drivers/platform/arm64/qcom-hamoa-ec.c
> +++ b/drivers/platform/arm64/qcom-hamoa-ec.c
> @@ -1195,6 +1195,11 @@ static int qcom_ec_probe(struct i2c_client *client)
>  	unsigned int i;
>  	int ret;
>  
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_I2C | I2C_FUNC_SMBUS_BYTE_DATA |
> +				     I2C_FUNC_SMBUS_WRITE_I2C_BLOCK))
> +		return -EOPNOTSUPP;
> +
>  	ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL);
>  	if (!ec)
>  		return -ENOMEM;
[Severity: High]
Could this regression result in a race condition where the interrupt handler is
requested and sci events are enabled before the ec->ec_cdev array is
allocated?

If an interrupt fires immediately after devm_request_threaded_irq() and
qcom_ec_sci_evt_control(dev, true) enable it, the handler could execute:

drivers/platform/arm64/qcom-hamoa-ec.c:qcom_ec_irq()
  -> qcom_ec_restore_fan_state()
    -> queues ec->fan_calib_work

The asynchronous workqueue function would then run:

drivers/platform/arm64/qcom-hamoa-ec.c:qcom_ec_fan_calib_work_fn() {
    ...
    ret = qcom_ec_fan_calibrate(ec, &ec->ec_cdev[i]);
    ...
}

If devm_kcalloc() hasn't yet allocated the ec->ec_cdev array later in
qcom_ec_probe(), couldn't this cause a null pointer dereference when
qcom_ec_fan_calibrate() accesses ec_cdev->parent_dev?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-ec_add_more_commands-v1-0-771abd65ee1a@oss.qualcomm.com?part=5

[PATCH 6/8] platform: arm64: qcom-hamoa-ec: Retry I2C transfers on NACK

[Anvesh Jain P]

The EC firmware NACKs the bus instead of clock-stretching while it is
busy servicing internal work (e.g. right after boot, when it is also
emitting a burst of SCI events). A transfer issued in that window fails
with -ENXIO even though the EC is otherwise healthy.

This isn't visible with the smbus-only command set used previously; it
surfaced while stress-testing the new fan LUT command on Hamoa, which
issues raw i2c_transfer() calls more aggressively and is thus more
likely to land in the NACK window right after boot or during an SCI
event burst.

Retry qcom_ec_write() and qcom_ec_read() a bounded number of times with
a short delay when the EC NACKs the transfer, before giving up.

Signed-off-by: Anvesh Jain P <anvesh.p@oss.qualcomm.com>
---
 drivers/platform/arm64/qcom-hamoa-ec.c | 40 +++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/arm64/qcom-hamoa-ec.c b/drivers/platform/arm64/qcom-hamoa-ec.c
index a420a180a194..122ae27c2b48 100644
--- a/drivers/platform/arm64/qcom-hamoa-ec.c
+++ b/drivers/platform/arm64/qcom-hamoa-ec.c
@@ -64,6 +64,18 @@
 
 #define EC_SOC_TJ_TEMP_POLL_JIFFIES	msecs_to_jiffies(125)
 
+/*
+ * The EC firmware NACKs the bus instead of clock-stretching while it is
+ * busy servicing internal work (e.g. right after boot, when it is also
+ * emitting a burst of SCI events). This NACK only occurs at the address
+ * phase, so retrying the whole transfer is safe. Attempt the transfer a
+ * bounded number of times on NACK before giving up; a genuinely missing
+ * or unpowered device pays a worst-case added latency of
+ * (EC_I2C_NACK_MAX_ATTEMPTS - 1) * EC_I2C_NACK_RETRY_DELAY_MS.
+ */
+#define EC_I2C_NACK_MAX_ATTEMPTS	5
+#define EC_I2C_NACK_RETRY_DELAY_MS	20
+
 enum qcom_ec_sci_events {
 	EC_FAN1_STATUS_CHANGE_EVT = 0x30,
 	EC_FAN2_STATUS_CHANGE_EVT,
@@ -233,11 +245,20 @@ static int qcom_ec_write(struct qcom_ec *ec, u8 cmd, const u8 *data, size_t len)
 	msg.buf[0] = cmd;
 	memcpy(&msg.buf[1], data, len);
 
-	ret = i2c_transfer(ec->client->adapter, &msg, 1);
+	for (int i = 0; i < EC_I2C_NACK_MAX_ATTEMPTS; i++) {
+		ret = i2c_transfer(ec->client->adapter, &msg, 1);
+		ret = ret == 1 ? 0 : (ret < 0 ? ret : -EIO);
+		if (ret != -ENXIO || i == EC_I2C_NACK_MAX_ATTEMPTS - 1)
+			break;
+
+		dev_dbg(&ec->client->dev,
+			"EC busy (NACK), retrying write of cmd 0x%02x\n", cmd);
+		msleep(EC_I2C_NACK_RETRY_DELAY_MS);
+	}
 
 	kfree(msg.buf);
 
-	return ret == 1 ? 0 : (ret < 0 ? ret : -EIO);
+	return ret;
 }
 
 static int qcom_ec_read(struct qcom_ec *ec, u8 cmd, const u8 *subcmd,
@@ -281,11 +302,18 @@ static int qcom_ec_read(struct qcom_ec *ec, u8 cmd, const u8 *subcmd,
 	msgs[1].len   = resp_len;
 	msgs[1].buf   = read_buf;
 
-	ret = i2c_transfer(client->adapter, msgs, 2);
-	if (ret != 2) {
-		ret = ret < 0 ? ret : -EIO;
-		goto out;
+	for (int i = 0; i < EC_I2C_NACK_MAX_ATTEMPTS; i++) {
+		ret = i2c_transfer(client->adapter, msgs, 2);
+		ret = ret == 2 ? 0 : (ret < 0 ? ret : -EIO);
+		if (ret != -ENXIO || i == EC_I2C_NACK_MAX_ATTEMPTS - 1)
+			break;
+
+		dev_dbg(&client->dev,
+			"EC busy (NACK), retrying read of cmd 0x%02x\n", cmd);
+		msleep(EC_I2C_NACK_RETRY_DELAY_MS);
 	}
+	if (ret)
+		goto out;
 
 	if (read_buf[0] == 0 || read_buf[0] == 0xff) {
 		ret = -EOPNOTSUPP;

-- 
2.34.1

Re: [PATCH 6/8] platform: arm64: qcom-hamoa-ec: Retry I2C transfers on NACK

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] The patch adds retry logic for EC NACKs to `qcom_ec_write()` and `qcom_ec_read()`, but fails to add equivalent retries for the numerous `i2c_smbus_*` calls made throughout the driver, leaving them vulnerable to the exact same failure.
- [High] Race condition during device removal where cooling device callbacks can execute after debug mode is disabled.
- [Low] Initialized arrays of type `struct i2c_device_id` must use named initializers rather than positional initializers.
--

--- Patch [6]: [PATCH 6/8] platform: arm64: qcom-hamoa-ec: Retry I2C transfers on NACK ---
commit 42a41f4860b9d5a24932df8c69069cb7fd89b4d1
Author: Anvesh Jain P <anvesh.p@oss.qualcomm.com>

platform: arm64: qcom-hamoa-ec: Retry I2C transfers on NACK

The patch adds bounded retry logic to qcom_ec_write() and qcom_ec_read()
to handle EC firmware NACKs that occur during busy windows right after
boot or during SCI event bursts.

> The EC firmware NACKs the bus instead of clock-stretching while it is
> busy servicing internal work (e.g. right after boot, when it is also
> emitting a burst of SCI events). A transfer issued in that window fails
> with -ENXIO even though the EC is otherwise healthy.

[Severity: High]
This isn't a bug introduced by this patch, but since the EC NACKs the bus
right after boot and during SCI event bursts, will the un-retried i2c_smbus
calls also fail?

For example, qcom_ec_sci_evt_control() is called directly during probe and
uses i2c_smbus_write_byte_data() without retry logic:

drivers/platform/arm64/qcom-hamoa-ec.c:qcom_ec_sci_evt_control() {
    ...
    ret = i2c_smbus_write_byte_data(client, EC_SCI_EVT_CONTROL_CMD, enable ? 1 : 0);
    ...
}

Similarly, qcom_ec_irq() reads events during an SCI burst using
i2c_smbus_read_byte_data():

drivers/platform/arm64/qcom-hamoa-ec.c:qcom_ec_irq() {
    ...
    val = i2c_smbus_read_byte_data(ec->client, EC_SCI_EVT_READ_CMD);
    ...
}

Should these SMBus calls also be updated to retry on NACK?

> diff --git a/drivers/platform/arm64/qcom-hamoa-ec.c b/drivers/platform/arm64/qcom-hamoa-ec.c
> index a420a180a1948..122ae27c2b483 100644
> --- a/drivers/platform/arm64/qcom-hamoa-ec.c
> +++ b/drivers/platform/arm64/qcom-hamoa-ec.c

[ ... ]

> @@ -281,11 +302,18 @@ static int qcom_ec_read(struct qcom_ec *ec, u8 cmd, const u8 *subcmd,
>  	msgs[1].len   = resp_len;
>  	msgs[1].buf   = read_buf;
>  
> -	ret = i2c_transfer(client->adapter, msgs, 2);
> -	if (ret != 2) {
> -		ret = ret < 0 ? ret : -EIO;
> -		goto out;
> +	for (int i = 0; i < EC_I2C_NACK_MAX_ATTEMPTS; i++) {
> +		ret = i2c_transfer(client->adapter, msgs, 2);
> +		ret = ret == 2 ? 0 : (ret < 0 ? ret : -EIO);
> +		if (ret != -ENXIO || i == EC_I2C_NACK_MAX_ATTEMPTS - 1)
> +			break;
> +
> +		dev_dbg(&client->dev,
> +			"EC busy (NACK), retrying read of cmd 0x%02x\n", cmd);
> +		msleep(EC_I2C_NACK_RETRY_DELAY_MS);
>  	}
> +	if (ret)
> +		goto out;
>  
>  	if (read_buf[0] == 0 || read_buf[0] == 0xff) {
>  		ret = -EOPNOTSUPP;

[Severity: High]
This is a pre-existing issue, but does the cooling device teardown have a
race condition that could leave the fan stuck in debug (manual PWM) mode?

In qcom_ec_remove(), the driver turns off debug mode:

drivers/platform/arm64/qcom-hamoa-ec.c:qcom_ec_remove() {
    ...
    qcom_ec_fan_debug_mode_off(ec_cdev);
    ...
}

However, since the thermal cooling device is managed by devres, it remains
registered until after remove() completes. Can the thermal core invoke
qcom_ec_fan_set_cur_state() during this window, which re-enables debug mode?

drivers/platform/arm64/qcom-hamoa-ec.c:qcom_ec_fan_set_cur_state() {
    ...
    ret = qcom_ec_fan_write_pwm(ec_cdev, state);
    ...
}

[Severity: Low]
This is a pre-existing issue, but does qcom_ec_i2c_id_table violate the I2C
subsystem API guidelines by using positional initializers?

drivers/platform/arm64/qcom-hamoa-ec.c:
static const struct i2c_device_id qcom_ec_i2c_id_table[] = {
	{ "qcom-hamoa-ec", },
	{}
};

The subsystem guidelines state that initialized arrays of type
struct i2c_device_id must use named initializers.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-ec_add_more_commands-v1-0-771abd65ee1a@oss.qualcomm.com?part=6

[PATCH 7/8] arm64: dts: qcom: x1p42100-crd: Add qcom,tsens for EC fan thermal management

[Anvesh Jain P]

Add the qcom,tsens property to the embedded-controller node, pointing
at tsens2 and covering its first 11 sensor IDs (AOSS, NSP, GPU and
camera) whose readings are averaged to compute the SoC junction
temperature periodically reported to the EC for fan control.

The x1p42100 is a reduced part: purwa.dtsi disables tsens3 and deletes
the CPU cluster-2, CPUSS, AOSS and upper GPU thermal zones, remapping
the NSP, lower GPU and camera sensors onto tsens2. Only those sensors
are available to contribute to the SoC Tj average on this board.

Signed-off-by: Anvesh Jain P <anvesh.p@oss.qualcomm.com>
---
 arch/arm64/boot/dts/qcom/x1p42100-crd.dts | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/x1p42100-crd.dts b/arch/arm64/boot/dts/qcom/x1p42100-crd.dts
index 7ed4116b9590..b46b67c5e436 100644
--- a/arch/arm64/boot/dts/qcom/x1p42100-crd.dts
+++ b/arch/arm64/boot/dts/qcom/x1p42100-crd.dts
@@ -19,3 +19,9 @@ / {
 &gpu_zap_shader {
 	firmware-name = "qcom/x1p42100/gen71500_zap.mbn";
 };
+
+&i2c5 {
+	embedded-controller@76 {
+		qcom,tsens = <&tsens2 11>;
+	};
+};

-- 
2.34.1

Re: [PATCH 7/8] arm64: dts: qcom: x1p42100-crd: Add qcom,tsens for EC fan thermal management

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The `embedded-controller@76` node is instantiated in `x1p42100-crd.dts` without essential base properties (`compatible`, `reg`, `interrupts`), resulting in devicetree compiler warnings and driver binding failure.
--

--- Patch [7]: [PATCH 7/8] arm64: dts: qcom: x1p42100-crd: Add qcom,tsens for EC fan thermal management ---
commit 6a0028e3f0e9078f36aed5052ae5c17e337676ff
Author: Anvesh Jain P <anvesh.p@oss.qualcomm.com>

arm64: dts: qcom: x1p42100-crd: Add qcom,tsens for EC fan thermal management

This commit adds the qcom,tsens property to the embedded-controller node in
the x1p42100-crd device tree for fan thermal management.

> diff --git a/arch/arm64/boot/dts/qcom/x1p42100-crd.dts b/arch/arm64/boot/dts/qcom/x1p42100-crd.dts
> index 7ed4116b95904..b46b67c5e4366 100644
> --- a/arch/arm64/boot/dts/qcom/x1p42100-crd.dts
> +++ b/arch/arm64/boot/dts/qcom/x1p42100-crd.dts

[ ... ]

> +&i2c5 {
> +	embedded-controller@76 {
> +		qcom,tsens = <&tsens2 11>;
> +	};
> +};

[Severity: High]
Does declaring the embedded-controller@76 node here create a new, incomplete
node?

Since this node isn't defined in the included base files, it appears to be
missing the required "compatible" and "reg" properties. Will this lead to a
devicetree compiler warning about a unit name without a reg property, and
prevent the qcom-hamoa-ec driver from binding?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-ec_add_more_commands-v1-0-771abd65ee1a@oss.qualcomm.com?part=7

[PATCH 8/8] arm64: dts: qcom: x1e80100-crd: Add qcom,tsens for EC fan thermal management

[Anvesh Jain P]

Add the qcom,tsens property to the embedded-controller node, pointing
at tsens2 and tsens3 and covering their first 11 and 15 sensor IDs
respectively (CPU cluster, AOSS, NSP, GPU and camera) used to compute
the average SoC junction temperature reported to the EC for fan
control.

Signed-off-by: Anvesh Jain P <anvesh.p@oss.qualcomm.com>
---
 arch/arm64/boot/dts/qcom/x1e80100-crd.dts | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
index 429deffcf3e9..4639340e716c 100644
--- a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
+++ b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
@@ -20,3 +20,9 @@ &gpu_zap_shader {
 &iris {
 	status = "okay";
 };
+
+&i2c5 {
+	embedded-controller@76 {
+		qcom,tsens = <&tsens2 11>, <&tsens3 15>;
+	};
+};

-- 
2.34.1

Re: [PATCH 8/8] arm64: dts: qcom: x1e80100-crd: Add qcom,tsens for EC fan thermal management

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The newly added `embedded-controller@76` node lacks the mandatory `compatible` and `reg` properties, resulting in an incomplete and non-functional device tree node.
--

--- Patch [8]: [PATCH 8/8] arm64: dts: qcom: x1e80100-crd: Add qcom,tsens for EC fan thermal management ---
commit 59ab25e0b30620df90dae53f1497861f1847b925
Author: Anvesh Jain P <anvesh.p@oss.qualcomm.com>

arm64: dts: qcom: x1e80100-crd: Add qcom,tsens for EC fan thermal management

This commit adds the qcom,tsens property to the embedded-controller node
in the x1e80100-crd device tree to report average SoC junction temperature
to the EC for fan control.

> diff --git a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
> index 429deffcf3e9d..4639340e716c6 100644
> --- a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
> +++ b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
> @@ -20,3 +20,9 @@ &gpu_zap_shader {
>  &iris {
>  	status = "okay";
>  };
> +
> +&i2c5 {
> +	embedded-controller@76 {
> +		qcom,tsens = <&tsens2 11>, <&tsens3 15>;
> +	};
> +};

[Severity: Medium]
Since this defines a new embedded-controller@76 node directly under &i2c5
rather than referencing an existing phandle, does it need compatible and reg
properties?

Without them, dtc will emit a unit_address_vs_reg warning because the @76
unit address lacks a corresponding reg = <0x76> property.

Furthermore, the I2C subsystem will not instantiate the device without a
compatible string and reg address, which would prevent the driver from
probing and leave the thermal management non-functional.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-ec_add_more_commands-v1-0-771abd65ee1a@oss.qualcomm.com?part=8