| Message ID | 20250320202408.3940777-4-anjelique.melendez@oss.qualcomm.com |
|---|---|
| State | New |
| Headers | show |
| Series | thermal: qcom-spmi-temp-alarm: Add support for new TEMP_ALARM subtypes | expand |
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(-) > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> >
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. 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 ? > static const struct spmi_temp_alarm_data spmi_temp_alarm_data = { > + .ops = &qpnp_tm_sensor_ops, > .temp_map = &temp_map_gen1, > + .configure_trip_temps = qpnp_tm_configure_trip_temp, > .get_temp_stage = qpnp_tm_gen1_get_temp_stage, > }; > > static const struct spmi_temp_alarm_data spmi_temp_alarm_gen2_data = { > + .ops = &qpnp_tm_sensor_ops, > .temp_map = &temp_map_gen1, > + .configure_trip_temps = qpnp_tm_configure_trip_temp, > .get_temp_stage = qpnp_tm_gen2_get_temp_stage, > }; > > static const struct spmi_temp_alarm_data spmi_temp_alarm_gen2_rev1_data = { > + .ops = &qpnp_tm_sensor_ops, > .temp_map = &temp_map_gen2_v1, > + .configure_trip_temps = qpnp_tm_configure_trip_temp, > .get_temp_stage = qpnp_tm_gen2_get_temp_stage, > }; > > @@ -336,7 +359,6 @@ static int qpnp_tm_init(struct qpnp_tm_chip *chip) > { > int ret; > u8 reg = 0; > - int crit_temp; > > mutex_lock(&chip->lock); > > @@ -355,15 +377,7 @@ static int qpnp_tm_init(struct qpnp_tm_chip *chip) > if (chip->stage) > chip->temp = qpnp_tm_decode_temp(chip, chip->stage); > > - 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); > - > - ret = qpnp_tm_update_critical_trip_temp(chip, crit_temp); > + ret = chip->data->configure_trip_temps(chip); > if (ret < 0) > goto out; > > @@ -483,7 +497,7 @@ static int qpnp_tm_probe(struct platform_device *pdev) > * before the hardware initialization is completed. > */ > chip->tz_dev = devm_thermal_of_zone_register( > - &pdev->dev, 0, chip, &qpnp_tm_sensor_ops); > + &pdev->dev, 0, chip, chip->data->ops); > if (IS_ERR(chip->tz_dev)) > return dev_err_probe(&pdev->dev, PTR_ERR(chip->tz_dev), > "failed to register sensor\n"); > -- > 2.34.1 >
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
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); +} + static const struct spmi_temp_alarm_data spmi_temp_alarm_data = { + .ops = &qpnp_tm_sensor_ops, .temp_map = &temp_map_gen1, + .configure_trip_temps = qpnp_tm_configure_trip_temp, .get_temp_stage = qpnp_tm_gen1_get_temp_stage, }; static const struct spmi_temp_alarm_data spmi_temp_alarm_gen2_data = { + .ops = &qpnp_tm_sensor_ops, .temp_map = &temp_map_gen1, + .configure_trip_temps = qpnp_tm_configure_trip_temp, .get_temp_stage = qpnp_tm_gen2_get_temp_stage, }; static const struct spmi_temp_alarm_data spmi_temp_alarm_gen2_rev1_data = { + .ops = &qpnp_tm_sensor_ops, .temp_map = &temp_map_gen2_v1, + .configure_trip_temps = qpnp_tm_configure_trip_temp, .get_temp_stage = qpnp_tm_gen2_get_temp_stage, }; @@ -336,7 +359,6 @@ static int qpnp_tm_init(struct qpnp_tm_chip *chip) { int ret; u8 reg = 0; - int crit_temp; mutex_lock(&chip->lock); @@ -355,15 +377,7 @@ static int qpnp_tm_init(struct qpnp_tm_chip *chip) if (chip->stage) chip->temp = qpnp_tm_decode_temp(chip, chip->stage); - 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); - - ret = qpnp_tm_update_critical_trip_temp(chip, crit_temp); + ret = chip->data->configure_trip_temps(chip); if (ret < 0) goto out; @@ -483,7 +497,7 @@ static int qpnp_tm_probe(struct platform_device *pdev) * before the hardware initialization is completed. */ chip->tz_dev = devm_thermal_of_zone_register( - &pdev->dev, 0, chip, &qpnp_tm_sensor_ops); + &pdev->dev, 0, chip, chip->data->ops); if (IS_ERR(chip->tz_dev)) return dev_err_probe(&pdev->dev, PTR_ERR(chip->tz_dev), "failed to register sensor\n");
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(-)