Re: [PATCH v3 3/5 RESEND] thermal: qcom-spmi-temp-alarm: Prepare to support additional Temp Alarm subtypes

[Anjelique Melendez <anjelique.melendez@oss.qualcomm.com>]

On 4/18/2025 4:15 AM, Daniel Lezcano wrote:
> On Thu, Mar 20, 2025 at 01:24:06PM -0700, Anjelique Melendez wrote:
>> In preparation to support newer temp alarm subtypes, add the "ops" and
>> "configure_trip_temps" references to spmi_temp_alarm_data. This will
>> allow for each Temp Alarm subtype to define its own
>> thermal_zone_device_ops and properly configure thermal trip temperature.
>>
>> Signed-off-by: Anjelique Melendez <anjelique.melendez@oss.qualcomm.com>
>> ---
>>   drivers/thermal/qcom/qcom-spmi-temp-alarm.c | 38 ++++++++++++++-------
>>   1 file changed, 26 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/thermal/qcom/qcom-spmi-temp-alarm.c b/drivers/thermal/qcom/qcom-spmi-temp-alarm.c
>> index 1cc9369ca9e1..514772e94a28 100644
>> --- a/drivers/thermal/qcom/qcom-spmi-temp-alarm.c
>> +++ b/drivers/thermal/qcom/qcom-spmi-temp-alarm.c
>> @@ -1,7 +1,7 @@
>>   // SPDX-License-Identifier: GPL-2.0-only
>>   /*
>>    * Copyright (c) 2011-2015, 2017, 2020, The Linux Foundation. All rights reserved.
>> - * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved.
>> + * Copyright (c) 2024-2025, Qualcomm Innovation Center, Inc. All rights reserved.
>>    */
>>   
>>   #include <linux/bitfield.h>
>> @@ -71,8 +71,10 @@ static const long temp_map_gen2_v1[THRESH_COUNT][STAGE_COUNT] = {
>>   struct qpnp_tm_chip;
>>   
>>   struct spmi_temp_alarm_data {
>> +	const struct thermal_zone_device_ops *ops;
>>   	const long (*temp_map)[THRESH_COUNT][STAGE_COUNT];
>>   	int (*get_temp_stage)(struct qpnp_tm_chip *chip);
>> +	int (*configure_trip_temps)(struct qpnp_tm_chip *chip);
>>   };
>>   
>>   struct qpnp_tm_chip {
>> @@ -312,18 +314,39 @@ static irqreturn_t qpnp_tm_isr(int irq, void *data)
>>   	return IRQ_HANDLED;
>>   }
>>   
>> +static int qpnp_tm_configure_trip_temp(struct qpnp_tm_chip *chip)
>> +{
>> +	int crit_temp, ret;
>> +
>> +	mutex_unlock(&chip->lock);
>> +
>> +	ret = thermal_zone_get_crit_temp(chip->tz_dev, &crit_temp);
>> +	if (ret)
>> +		crit_temp = THERMAL_TEMP_INVALID;
>> +
>> +	mutex_lock(&chip->lock);
>> +
>> +	return qpnp_tm_update_critical_trip_temp(chip, crit_temp);
>> +}
> 
> The qpnp_tm_configure_trip_temp() is called with the lock held which is really
> unusual to have this assymetry when dealing with the locks.
> 
This change is simply moving these lines from init() into their own 
configure_trip_temp() function. configure_trip_temp() is only called 
from within init() so functionally this is the same as what the driver 
was doing before. As new temp_alarm types are introduced (like in patch 
4&5) they may need to configure trip temps differently.

Specifically the mutex_unlock() and mutex_lock() guards were added in 
this change: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/thermal/qcom/qcom-spmi-temp-alarm.c?h=v6.15-rc3&id=59edcd91d852f88ef7d208029503f9b5310d0603

> In the other side, this code assume it is ok the userspace can change the
> critical temperature of the board. Is it really a good idea ?

Sorry, I think I might be a little confused on what you mean from this 
comment. This driver has supported setting critical temperature from 
userspace for many years now.. 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thermal/qcom/qcom-spmi-temp-alarm.c?h=v6.15-rc3#n264. 
This patch is just reworking driver, there are no functional/behavioral 
changes.

Thanks,
Anjelique