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

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

On 3/16/2026 3:57 PM, Konrad Dybcio wrote:
> On 3/13/26 11:29 AM, 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>
>> Co-developed-by: Anvesh Jain P <anvesh.p@oss.qualcomm.com>
>> Signed-off-by: Anvesh Jain P <anvesh.p@oss.qualcomm.com>
>> ---
> 
> [...]
> 
>> + * ------------------------------------------------------------------------------
>> + * | Offset		| Name		| Description				|
>> + * ------------------------------------------------------------------------------
>> + * | 0x00		| Byte count	| Number of bytes in response		|
>> + * |			|		| (exluding byte count)			|
>> + * ------------------------------------------------------------------------------
>> + * | 0x02 (LSB)	| EC Thermal	| Bit 0-1: Number of fans		|
>> + * | 0x3		| Capabilities	| Bit 2-4: Type of fan			|
>> + * |			|		| Bit 5-6: Reserved			|
>> + * |			|		| Bit 7: Data Valid/Invalid		|
>> + * |			|		|	 (Valid - 1, Invalid - 0)	|
>> + * |			|		| Bit 8-15: Thermistor 0 - 7 presence	|
>> + * |			|		|	    (0 present, 1 absent)	|
> 						     ^ huh??
> 
> I see that it's not currently used, but I think flipping these
> bits would make it easier to comprehend down the line
> 
> [...]
> 

Thanks for taking time to review the series

The 0=present encoding was taken directly from the EC firmware spec, but
on closer inspection this appears to be a typo in the spec — the correct
behavior is 1=present, 0=absent. Will update the comment in v5.

>> +	default:
>> +		dev_dbg(dev, "Unknown EC event: %d\n", val);
> 
> Maybe dev_notice(), this would be good to log
>

Agreed, will switch to dev_notice for unknown EC events in v5.

>> +		break;
>> +	}
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int qcom_ec_sci_evt_control(struct device *dev, bool enable)
>> +{
>> +	struct i2c_client *client = to_i2c_client(dev);
>> +	u8 control = enable ? 1 : 0;
>> +	int ret;
>> +
>> +	ret = i2c_smbus_write_byte_data(client, EC_SCI_EVT_CONTROL_CMD, control);
>> +
>> +	return ret;
> 
> return i2c_smbus_write_byte_data(client, EC_SCI_EVT_CONTROL_CMD, !!enable);
> 
> [...]
> 

Agreed, will simplify in v5

>> +static int qcom_ec_resume(struct device *dev)
>> +{
>> +	struct i2c_client *client = to_i2c_client(dev);
>> +	int ret;
>> +
>> +	ret = i2c_smbus_write_byte_data(client, EC_MODERN_STANDBY_CMD, EC_MODERN_STANDBY_ENTER);
>> +
>> +	return ret;
> 
> In a pattern like this, unless you have some error checking/logging to
> do, you can just return directly
> 
> [...]
> 

Agreed, will simplify in v5.

>> +	ret = qcom_ec_read_fw_version(dev);
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, ret, "Failed to read ec firmware version\n");
> 
> "EC"
> 
> [...]
> 

Agreed, will fix in v5

>> +	for (i = 0; i < ec->thermal_cap.fan_cnt; i++) {
>> +		struct qcom_ec_cooling_dev *ec_cdev = &ec->ec_cdev[i];
>> +		char name[EC_FAN_NAME_SIZE];
>> +
>> +		snprintf(name, EC_FAN_NAME_SIZE, "qcom_ec_fan_%d", i);
>> +		ec_cdev->fan_id = i + 1;
>> +		ec_cdev->parent_dev = dev;
>> +
>> +		ec_cdev->cdev = thermal_cooling_device_register(name, ec_cdev,
>> +								&qcom_ec_thermal_ops);
>> +		if (IS_ERR(ec_cdev->cdev)) {
>> +			dev_err_probe(dev, PTR_ERR(cdev),
>> +				      "Thermal cooling device registration failed\n");
>> +			ret = -EINVAL;
>> +			goto unroll_cooling_dev;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +
>> +unroll_cooling_dev:
> 
> For those cases, one would usually add add a devres-managed version of
> the helper (devm_foo_bar()), removing the need to repeat this in the 
> driver remove callback as well
> 
> Konrad
Agreed, will switch to devm_thermal_of_cooling_device_register in v5

-- 
Best Regards,
Anvesh