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

Anjelique Melendez
2025-04-23T23:20:06Z

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