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

[Anvesh Jain P]

On 7/29/2026 4:29 PM, Konrad Dybcio wrote:
> On 7/28/26 7:44 PM, Anvesh Jain P wrote:
>> 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.
>> ---
> 
> Missing sign-off, have you run b4 prep --check?
>

Thanks for reviewing my series.

Ack, I missed to add sign-off while splitting the patches. I verified b4
prep --check did pass clean here. Will add the trailer in v2.
> [...]
> 
> 
>> +	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);
> 
> Does it make more sense to simply stick a guard(mutex)(&ec->io_lock)
> at the beginning of qcom_ec_read()?
>

For this call site it'd be equivalent, but a few other callers (e.g.
qcom_ec_update_profile_from_power_supply(), qcom_ec_fan_calibrate())
hold io_lock across multiple qcom_ec_read()/qcom_ec_write() calls plus
state checks in between, for atomicity. Pushing the lock into
qcom_ec_read()/qcom_ec_write() themselves would self-deadlock those
callers unless their outer locking is also removed, which would then
narrow the lock scope to a single command and break that atomicity.
Keeping it at the call site here for consistency with the rest of the file.

> [...]
> 
>> +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);
>> +	}
> 
> I'm not sure that's the intended use of the API, but this is NHI
> of_thermal_zone_find()
>

You're right, this duplicates of_thermal_zone_find()'s algorithm almost
exactly. It's static in drivers/thermal/thermal_of.c though, so it's not
callable from here as-is — keeping the local copy rather than touching
that file.

> [...]
> 
>> +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);
>>  }
> 
> This should be a separate fix. FWIW suspending and resuming on
> linux-next/master currently gives me:
> 
> [   43.754787] geni_i2c a84000.i2c: error turning SE resources:-13
> [   43.754811] qcom-hamoa-ec 3-0076: Failed to read EC SCI Event: -13
> 
> Konrad

Agreed, will split the devm_add_action_or_reset() conversion into its
own commit — it's an independent correctness fix (also disables SCI
events on partial probe failure, not just on remove()), unrelated to SoC
Tj reporting.

On the -13 during suspend/resume: that looks like the SCI IRQ firing (or
its threaded handler still running) while the I2C SE resources are down
for suspend. Will dig into whether the IRQ needs to be quiesced/disabled
around suspend/resume here and follow up.

-- 
Best Regards,
Anvesh