| Message ID | 20240729231259.2122976-1-quic_amelende@quicinc.com |
|---|---|
| Headers | show |
| Series | thermal: qcom-spmi-temp-alarm: add support for new TEMP_ALARM subtypes | expand |
On Mon, Jul 29, 2024 at 04:12:58PM GMT, Anjelique Melendez wrote: > Add support for TEMP_ALARM GEN2 PMIC peripherals with digital major > revision 2. This revision utilizes individual temp DAC registers > to set the threshold temperature for over-temperature stages 1, > 2, and 3 instead of a single register to specify a set of > thresholds. > > Co-developed-by: David Collins <quic_collinsd@quicinc.com> > Signed-off-by: David Collins <quic_collinsd@quicinc.com> > Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com> > --- > drivers/thermal/qcom/qcom-spmi-temp-alarm.c | 195 ++++++++++++++++++-- > 1 file changed, 184 insertions(+), 11 deletions(-) > > diff --git a/drivers/thermal/qcom/qcom-spmi-temp-alarm.c b/drivers/thermal/qcom/qcom-spmi-temp-alarm.c > index de4a2d99f0d5..1f56acd8f637 100644 > --- a/drivers/thermal/qcom/qcom-spmi-temp-alarm.c > +++ b/drivers/thermal/qcom/qcom-spmi-temp-alarm.c > @@ -24,6 +24,10 @@ > #define QPNP_TM_REG_STATUS 0x08 > #define QPNP_TM_REG_SHUTDOWN_CTRL1 0x40 > #define QPNP_TM_REG_ALARM_CTRL 0x46 > +/* TEMP_DAC_* registers are only present for TEMP_GEN2 v2.0 */ > +#define QPNP_TM_REG_TEMP_DAC_STG1 0x47 > +#define QPNP_TM_REG_TEMP_DAC_STG2 0x48 > +#define QPNP_TM_REG_TEMP_DAC_STG3 0x49 > > #define QPNP_TM_TYPE 0x09 > #define QPNP_TM_SUBTYPE_GEN1 0x08 > @@ -65,13 +69,42 @@ static const long temp_map_gen2_v1[THRESH_COUNT][STAGE_COUNT] = { > > #define TEMP_STAGE_HYSTERESIS 2000 > > +/* > + * For TEMP_GEN2 v2.0, TEMP_DAC_STG1/2/3 registers are used to set the threshold > + * for each stage independently. > + * TEMP_DAC_STG* = 0 --> 80 C > + * Each 8 step increase in TEMP_DAC_STG* value corresponds to 5 C (5000 mC). > + */ > +#define TEMP_DAC_MIN 80000 > +#define TEMP_DAC_SCALE_NUM 8 > +#define TEMP_DAC_SCALE_DEN 5000 > + > +#define TEMP_DAC_TEMP_TO_REG(temp) \ > + (((temp) - TEMP_DAC_MIN) * TEMP_DAC_SCALE_NUM / TEMP_DAC_SCALE_DEN) > +#define TEMP_DAC_REG_TO_TEMP(reg) \ > + (TEMP_DAC_MIN + (reg) * TEMP_DAC_SCALE_DEN / TEMP_DAC_SCALE_NUM) > + > +static const long temp_dac_max[STAGE_COUNT] = { > + 119375, 159375, 159375 > +}; > + > /* Temperature in Milli Celsius reported during stage 0 if no ADC is present */ > #define DEFAULT_TEMP 37000 > > +struct qpnp_tm_chip; > + > +struct spmi_temp_alarm_data { > + const struct thermal_zone_device_ops *ops; > + bool has_temp_dac; > + int (*setup)(struct qpnp_tm_chip *chip); > + int (*update_trip_temps)(struct qpnp_tm_chip *chip); > +}; > + > struct qpnp_tm_chip { > struct regmap *map; > struct device *dev; > struct thermal_zone_device *tz_dev; > + const struct spmi_temp_alarm_data *data; > unsigned int subtype; > unsigned int dig_revision; > long temp; > @@ -85,6 +118,8 @@ struct qpnp_tm_chip { > > struct iio_channel *adc; > const long (*temp_map)[THRESH_COUNT][STAGE_COUNT]; > + > + long temp_dac_map[STAGE_COUNT]; > }; > > /* This array maps from GEN2 alarm state to GEN1 alarm stage */ > @@ -118,6 +153,13 @@ static int qpnp_tm_write(struct qpnp_tm_chip *chip, u16 addr, u8 data) > */ > static long qpnp_tm_decode_temp(struct qpnp_tm_chip *chip, unsigned int stage) > { > + if (chip->data->has_temp_dac) { > + if (stage == 0 || stage > STAGE_COUNT) > + return 0; > + > + return chip->temp_dac_map[stage - 1]; > + } > + > if (!chip->temp_map || chip->thresh >= THRESH_COUNT || stage == 0 || > stage > STAGE_COUNT) > return 0; > @@ -219,6 +261,34 @@ static int qpnp_tm_get_temp(struct thermal_zone_device *tz, int *temp) > return 0; > } > > +static int qpnp_tm_gen2_rev2_set_temp_thresh(struct qpnp_tm_chip *chip, int trip, > + int temp) > +{ > + int ret, temp_cfg; > + u8 reg; > + > + if (trip < 0 || trip >= STAGE_COUNT) { > + dev_err(chip->dev, "invalid TEMP_DAC trip = %d\n", trip); > + return -EINVAL; > + } else if (temp < TEMP_DAC_MIN || temp > temp_dac_max[trip]) { > + dev_err(chip->dev, "invalid TEMP_DAC temp = %d\n", temp); > + return -EINVAL; > + } > + > + reg = TEMP_DAC_TEMP_TO_REG(temp); > + temp_cfg = TEMP_DAC_REG_TO_TEMP(reg); > + > + ret = qpnp_tm_write(chip, QPNP_TM_REG_TEMP_DAC_STG1 + trip, reg); > + if (ret < 0) { > + dev_err(chip->dev, "TEMP_DAC_STG write failed, ret=%d\n", ret); > + return ret; > + } > + > + chip->temp_dac_map[trip] = temp_cfg; > + > + return 0; > +} > + > static int qpnp_tm_update_critical_trip_temp(struct qpnp_tm_chip *chip, > int temp) > { > @@ -286,6 +356,24 @@ static const struct thermal_zone_device_ops qpnp_tm_sensor_ops = { > .set_trip_temp = qpnp_tm_set_trip_temp, > }; > > +static int qpnp_tm_gen2_rev2_set_trip_temp(struct thermal_zone_device *tz, > + int trip, int temp) > +{ > + struct qpnp_tm_chip *chip = tz->devdata; > + int ret; > + > + mutex_lock(&chip->lock); > + ret = qpnp_tm_gen2_rev2_set_temp_thresh(chip, trip, temp); > + mutex_unlock(&chip->lock); > + > + return ret; > +} > + > +static const struct thermal_zone_device_ops qpnp_tm_gen2_rev2_sensor_ops = { > + .get_temp = qpnp_tm_get_temp, > + .set_trip_temp = qpnp_tm_gen2_rev2_set_trip_temp, > +}; > + > static irqreturn_t qpnp_tm_isr(int irq, void *data) > { > struct qpnp_tm_chip *chip = data; > @@ -313,6 +401,71 @@ static int qpnp_tm_get_critical_trip_temp(struct qpnp_tm_chip *chip) > return THERMAL_TEMP_INVALID; > } > > +/* Configure TEMP_DAC registers based on DT thermal_zone trips */ > +static int qpnp_tm_gen2_rev2_update_trip_temps(struct qpnp_tm_chip *chip) > +{ > + struct thermal_trip trip = {0}; > + int ret, ntrips, i; > + > + ntrips = thermal_zone_get_num_trips(chip->tz_dev); > + /* Keep hardware defaults if no DT trips are defined. */ > + if (ntrips <= 0) > + return 0; > + > + for (i = 0; i < ntrips; i++) { > + ret = thermal_zone_get_trip(chip->tz_dev, i, &trip); > + if (ret < 0) > + return ret; > + > + ret = qpnp_tm_gen2_rev2_set_temp_thresh(chip, i, trip.temperature); > + if (ret < 0) > + return ret; > + } > + > + /* Verify that trips are strictly increasing. */ There is no such requirement in the DT bindings. Please don't invent artificial restrictions, especially if they are undocumented. > + for (i = 1; i < STAGE_COUNT; i++) { > + if (chip->temp_dac_map[i] <= chip->temp_dac_map[i - 1]) { > + dev_err(chip->dev, "Threshold %d=%ld <= threshold %d=%ld\n", > + i, chip->temp_dac_map[i], i - 1, > + chip->temp_dac_map[i - 1]); > + return -EINVAL; > + } > + } > + > + return 0; > +} > + > +/* Read the hardware default TEMP_DAC stage threshold temperatures */ > +static int qpnp_tm_gen2_rev2_init(struct qpnp_tm_chip *chip) > +{ > + int ret, i; > + u8 reg = 0; > + > + for (i = 0; i < STAGE_COUNT; i++) { > + ret = qpnp_tm_read(chip, QPNP_TM_REG_TEMP_DAC_STG1 + i, ®); > + if (ret < 0) > + return ret; > + > + chip->temp_dac_map[i] = TEMP_DAC_REG_TO_TEMP(reg); > + } > + > + return 0; > +} > + > +static const struct spmi_temp_alarm_data spmi_temp_alarm_data = { > + .ops = &qpnp_tm_sensor_ops, > + .has_temp_dac = false, > + .setup = NULL, > + .update_trip_temps = NULL, No need to init to NULL, that's the default (and false too). > +}; > + > +static const struct spmi_temp_alarm_data spmi_temp_alarm_gen2_rev2_data = { > + .ops = &qpnp_tm_gen2_rev2_sensor_ops, > + .has_temp_dac = true, > + .setup = qpnp_tm_gen2_rev2_init, > + .update_trip_temps = qpnp_tm_gen2_rev2_update_trip_temps, > +}; > + > /* > * This function initializes the internal temp value based on only the > * current thermal stage and threshold. Setup threshold control and > @@ -339,21 +492,27 @@ static int qpnp_tm_init(struct qpnp_tm_chip *chip) > goto out; > chip->stage = ret; > > - stage = chip->subtype == QPNP_TM_SUBTYPE_GEN1 > + stage = (chip->subtype == QPNP_TM_SUBTYPE_GEN1) > ? chip->stage : alarm_state_map[chip->stage]; > > if (stage) > chip->temp = qpnp_tm_decode_temp(chip, stage); > > - mutex_unlock(&chip->lock); > + if (chip->data->update_trip_temps) { > + ret = chip->data->update_trip_temps(chip); > + if (ret < 0) > + goto out; > + } else { > + mutex_unlock(&chip->lock); > > - crit_temp = qpnp_tm_get_critical_trip_temp(chip); > + crit_temp = qpnp_tm_get_critical_trip_temp(chip); > > - mutex_lock(&chip->lock); > + mutex_lock(&chip->lock); > > - ret = qpnp_tm_update_critical_trip_temp(chip, crit_temp); > - if (ret < 0) > - goto out; > + ret = qpnp_tm_update_critical_trip_temp(chip, crit_temp); > + if (ret < 0) > + goto out; Can you move this to the update_trip_temps() callback? Then there is no need to have ifs here. > + } > > /* Enable the thermal alarm PMIC module in always-on mode. */ > reg = ALARM_CTRL_FORCE_ENABLE; > @@ -380,6 +539,10 @@ static int qpnp_tm_probe(struct platform_device *pdev) > if (!chip) > return -ENOMEM; > > + chip->data = of_device_get_match_data(&pdev->dev); > + if (!chip->data) > + return -EINVAL; > + > dev_set_drvdata(&pdev->dev, chip); > chip->dev = &pdev->dev; > > @@ -455,18 +618,21 @@ static int qpnp_tm_probe(struct platform_device *pdev) > } > > chip->subtype = subtype; > - if (subtype == QPNP_TM_SUBTYPE_GEN2 && dig_major >= 1) > + if (subtype == QPNP_TM_SUBTYPE_GEN2 && dig_major == 1) > chip->temp_map = &temp_map_gen2_v1; > - else > + else if (subtype == QPNP_TM_SUBTYPE_GEN1) > chip->temp_map = &temp_map_gen1; If you already have per-compatible match data, why do you need to have check revisions? Please be consistent. > > + if (chip->data->setup) > + chip->data->setup(chip); > + > /* > * Register the sensor before initializing the hardware to be able to > * read the trip points. get_temp() returns the default temperature > * 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"); > @@ -488,7 +654,14 @@ static int qpnp_tm_probe(struct platform_device *pdev) > } > > static const struct of_device_id qpnp_tm_match_table[] = { > - { .compatible = "qcom,spmi-temp-alarm" }, > + { > + .compatible = "qcom,spmi-temp-alarm", > + .data = &spmi_temp_alarm_data, > + }, > + { > + .compatible = "qcom,spmi-temp-alarm-gen2-rev2", > + .data = &spmi_temp_alarm_gen2_rev2_data, > + }, > { } > }; > MODULE_DEVICE_TABLE(of, qpnp_tm_match_table); > -- > 2.34.1 >
On Mon, Jul 29, 2024 at 04:12:59PM GMT, Anjelique Melendez wrote: > Add support for TEMP_ALARM LITE PMIC peripherals. This subtype > utilizes a pair of registers to configure a warning interrupt > threshold temperature and an automatic hardware shutdown > threshold temperature. > > Co-developed-by: David Collins <quic_collinsd@quicinc.com> > Signed-off-by: David Collins <quic_collinsd@quicinc.com> > Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com> > --- > drivers/thermal/qcom/qcom-spmi-temp-alarm.c | 208 +++++++++++++++++++- > 1 file changed, 202 insertions(+), 6 deletions(-) > > diff --git a/drivers/thermal/qcom/qcom-spmi-temp-alarm.c b/drivers/thermal/qcom/qcom-spmi-temp-alarm.c > index 1f56acd8f637..e50ce66ec096 100644 > --- a/drivers/thermal/qcom/qcom-spmi-temp-alarm.c > +++ b/drivers/thermal/qcom/qcom-spmi-temp-alarm.c > @@ -4,6 +4,7 @@ > * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved. > */ > > +#include <linux/bitfield.h> > #include <linux/bitops.h> > #include <linux/delay.h> > #include <linux/err.h> > @@ -22,21 +23,28 @@ > #define QPNP_TM_REG_TYPE 0x04 > #define QPNP_TM_REG_SUBTYPE 0x05 > #define QPNP_TM_REG_STATUS 0x08 > +#define QPNP_TM_REG_IRQ_STATUS 0x10 > #define QPNP_TM_REG_SHUTDOWN_CTRL1 0x40 > #define QPNP_TM_REG_ALARM_CTRL 0x46 > /* TEMP_DAC_* registers are only present for TEMP_GEN2 v2.0 */ > #define QPNP_TM_REG_TEMP_DAC_STG1 0x47 > #define QPNP_TM_REG_TEMP_DAC_STG2 0x48 > #define QPNP_TM_REG_TEMP_DAC_STG3 0x49 > +#define QPNP_TM_REG_LITE_TEMP_CFG1 0x50 > +#define QPNP_TM_REG_LITE_TEMP_CFG2 0x51 > > #define QPNP_TM_TYPE 0x09 > #define QPNP_TM_SUBTYPE_GEN1 0x08 > #define QPNP_TM_SUBTYPE_GEN2 0x09 > +#define QPNP_TM_SUBTYPE_LITE 0xC0 > > #define STATUS_GEN1_STAGE_MASK GENMASK(1, 0) > #define STATUS_GEN2_STATE_MASK GENMASK(6, 4) > #define STATUS_GEN2_STATE_SHIFT 4 > > +/* IRQ status only needed for TEMP_ALARM_LITE */ > +#define IRQ_STATUS_MASK BIT(0) > + > #define SHUTDOWN_CTRL1_OVERRIDE_S2 BIT(6) > #define SHUTDOWN_CTRL1_THRESHOLD_MASK GENMASK(1, 0) > > @@ -44,6 +52,8 @@ > > #define ALARM_CTRL_FORCE_ENABLE BIT(7) > > +#define LITE_TEMP_CFG_THRESHOLD_MASK GENMASK(3, 2) > + > #define THRESH_COUNT 4 > #define STAGE_COUNT 3 > > @@ -88,6 +98,19 @@ static const long temp_dac_max[STAGE_COUNT] = { > 119375, 159375, 159375 > }; > > +/* > + * TEMP_ALARM_LITE has two stages: warning and shutdown with independently > + * configured threshold temperatures. > + */ > + > +static const long temp_map_lite_warning[THRESH_COUNT] = { > + 115000, 125000, 135000, 145000 > +}; > + > +static const long temp_map_lite_shutdown[THRESH_COUNT] = { > + 135000, 145000, 160000, 175000 > +}; > + > /* Temperature in Milli Celsius reported during stage 0 if no ADC is present */ > #define DEFAULT_TEMP 37000 > > @@ -171,19 +194,26 @@ static long qpnp_tm_decode_temp(struct qpnp_tm_chip *chip, unsigned int stage) > * qpnp_tm_get_temp_stage() - return over-temperature stage > * @chip: Pointer to the qpnp_tm chip > * > - * Return: stage (GEN1) or state (GEN2) on success, or errno on failure. > + * Return: stage (GEN1), state (GEN2), or alarm interrupt state (LITE) on > + * success; or errno on failure. > */ > static int qpnp_tm_get_temp_stage(struct qpnp_tm_chip *chip) > { > int ret; > + u16 addr = QPNP_TM_REG_STATUS; > u8 reg = 0; > > - ret = qpnp_tm_read(chip, QPNP_TM_REG_STATUS, ®); > + if (chip->subtype == QPNP_TM_SUBTYPE_LITE) > + addr = QPNP_TM_REG_IRQ_STATUS; > + > + ret = qpnp_tm_read(chip, addr, ®); > if (ret < 0) > return ret; > > if (chip->subtype == QPNP_TM_SUBTYPE_GEN1) > ret = reg & STATUS_GEN1_STAGE_MASK; > + else if (chip->subtype == QPNP_TM_SUBTYPE_LITE) > + ret = reg & IRQ_STATUS_MASK; > else > ret = (reg & STATUS_GEN2_STATE_MASK) >> STATUS_GEN2_STATE_SHIFT; > > @@ -206,7 +236,8 @@ static int qpnp_tm_update_temp_no_adc(struct qpnp_tm_chip *chip) > return ret; > stage = ret; > > - if (chip->subtype == QPNP_TM_SUBTYPE_GEN1) { > + if (chip->subtype == QPNP_TM_SUBTYPE_GEN1 > + || chip->subtype == QPNP_TM_SUBTYPE_LITE) { > stage_new = stage; > stage_old = chip->stage; > } else { > @@ -289,6 +320,78 @@ static int qpnp_tm_gen2_rev2_set_temp_thresh(struct qpnp_tm_chip *chip, int trip > return 0; > } > > +static int qpnp_tm_lite_set_temp_thresh(struct qpnp_tm_chip *chip, int trip, > + int temp) > +{ > + int ret, temp_cfg, i; > + const long *temp_map; > + u16 addr; > + u8 reg, thresh; > + > + if (trip < 0 || trip >= STAGE_COUNT) { > + dev_err(chip->dev, "invalid TEMP_LITE trip = %d\n", trip); > + return -EINVAL; > + } > + > + switch (trip) { > + case 0: > + temp_map = temp_map_lite_warning; > + addr = QPNP_TM_REG_LITE_TEMP_CFG1; > + break; > + case 1: > + /* > + * The second trip point is purely in software to facilitate > + * a controlled shutdown after the warning threshold is crossed > + * but before the automatic hardware shutdown threshold is > + * crossed. > + */ > + return 0; > + case 2: > + temp_map = temp_map_lite_shutdown; > + addr = QPNP_TM_REG_LITE_TEMP_CFG2; > + break; > + default: > + return 0; > + } > + > + if (temp < temp_map[THRESH_MIN] || temp > temp_map[THRESH_MAX]) { > + dev_err(chip->dev, "invalid TEMP_LITE temp = %d\n", temp); > + return -EINVAL; > + } > + > + thresh = 0; > + temp_cfg = temp_map[thresh]; > + for (i = THRESH_MAX; i >= THRESH_MIN; i--) { > + if (temp >= temp_map[i]) { > + thresh = i; > + temp_cfg = temp_map[i]; > + break; > + } > + } > + > + if (temp_cfg == chip->temp_dac_map[trip]) > + return 0; > + > + ret = qpnp_tm_read(chip, addr, ®); > + if (ret < 0) { > + dev_err(chip->dev, "LITE_TEMP_CFG read failed, ret=%d\n", ret); > + return ret; > + } > + > + reg &= ~LITE_TEMP_CFG_THRESHOLD_MASK; > + reg |= FIELD_PREP(LITE_TEMP_CFG_THRESHOLD_MASK, thresh); > + > + ret = qpnp_tm_write(chip, addr, reg); > + if (ret < 0) { > + dev_err(chip->dev, "LITE_TEMP_CFG write failed, ret=%d\n", ret); > + return ret; > + } > + > + chip->temp_dac_map[trip] = temp_cfg; > + > + return 0; > +} > + > static int qpnp_tm_update_critical_trip_temp(struct qpnp_tm_chip *chip, > int temp) > { > @@ -374,6 +477,24 @@ static const struct thermal_zone_device_ops qpnp_tm_gen2_rev2_sensor_ops = { > .set_trip_temp = qpnp_tm_gen2_rev2_set_trip_temp, > }; > > +static int qpnp_tm_lite_set_trip_temp(struct thermal_zone_device *tz, > + int trip, int temp) > +{ > + struct qpnp_tm_chip *chip = tz->devdata; > + int ret; > + > + mutex_lock(&chip->lock); > + ret = qpnp_tm_lite_set_temp_thresh(chip, trip, temp); > + mutex_unlock(&chip->lock); > + > + return ret; > +} > + > +static const struct thermal_zone_device_ops qpnp_tm_lite_sensor_ops = { > + .get_temp = qpnp_tm_get_temp, > + .set_trip_temp = qpnp_tm_lite_set_trip_temp, > +}; > + > static irqreturn_t qpnp_tm_isr(int irq, void *data) > { > struct qpnp_tm_chip *chip = data; > @@ -452,6 +573,68 @@ static int qpnp_tm_gen2_rev2_init(struct qpnp_tm_chip *chip) > return 0; > } > > +/* Configure TEMP_LITE registers based on DT thermal_zone trips */ > +static int qpnp_tm_lite_update_trip_temps(struct qpnp_tm_chip *chip) > +{ > + struct thermal_trip trip = {0}; > + int ret, ntrips, i; > + > + ntrips = thermal_zone_get_num_trips(chip->tz_dev); > + /* Keep hardware defaults if no DT trips are defined. */ > + if (ntrips <= 0) > + return 0; > + > + for (i = 0; i < ntrips; i++) { > + ret = thermal_zone_get_trip(chip->tz_dev, i, &trip); > + if (ret < 0) > + return ret; > + > + ret = qpnp_tm_lite_set_temp_thresh(chip, i, trip.temperature); > + if (ret < 0) > + return ret; > + } > + > + /* Verify that trips are strictly increasing. */ Again, this looks like artificial limitations. > + if (chip->temp_dac_map[2] <= chip->temp_dac_map[0]) { > + dev_err(chip->dev, "Threshold 2=%ld <= threshold 0=%ld\n", > + chip->temp_dac_map[2], chip->temp_dac_map[0]); > + return -EINVAL; > + } > + > + return 0; > +} > + > +/* Read the hardware default TEMP_LITE stage threshold temperatures */ > +static int qpnp_tm_lite_init(struct qpnp_tm_chip *chip) > +{ > + int ret, thresh; > + u8 reg = 0; > + > + /* > + * Store the warning trip temp in temp_dac_map[0] and the shutdown trip > + * temp in temp_dac_map[2]. The second trip point is purely in software > + * to facilitate a controlled shutdown after the warning threshold is > + * crossed but before the automatic hardware shutdown threshold is > + * crossed. Thus, there is no register to read for the second trip > + * point. > + */ What if the DT define 4 trip points? Or two? > + ret = qpnp_tm_read(chip, QPNP_TM_REG_LITE_TEMP_CFG1, ®); > + if (ret < 0) > + return ret; > + > + thresh = FIELD_GET(LITE_TEMP_CFG_THRESHOLD_MASK, reg); > + chip->temp_dac_map[0] = temp_map_lite_warning[thresh]; > + > + ret = qpnp_tm_read(chip, QPNP_TM_REG_LITE_TEMP_CFG2, ®); > + if (ret < 0) > + return ret; > + > + thresh = FIELD_GET(LITE_TEMP_CFG_THRESHOLD_MASK, reg); > + chip->temp_dac_map[2] = temp_map_lite_shutdown[thresh]; > + > + return 0; > +} > + > static const struct spmi_temp_alarm_data spmi_temp_alarm_data = { > .ops = &qpnp_tm_sensor_ops, > .has_temp_dac = false, > @@ -466,6 +649,13 @@ static const struct spmi_temp_alarm_data spmi_temp_alarm_gen2_rev2_data = { > .update_trip_temps = qpnp_tm_gen2_rev2_update_trip_temps, > }; > > +static const struct spmi_temp_alarm_data spmi_temp_alarm_lite_data = { > + .ops = &qpnp_tm_lite_sensor_ops, > + .has_temp_dac = true, > + .setup = qpnp_tm_lite_init, > + .update_trip_temps = qpnp_tm_lite_update_trip_temps, > +}; > + > /* > * This function initializes the internal temp value based on only the > * current thermal stage and threshold. Setup threshold control and > @@ -492,8 +682,9 @@ static int qpnp_tm_init(struct qpnp_tm_chip *chip) > goto out; > chip->stage = ret; > > - stage = (chip->subtype == QPNP_TM_SUBTYPE_GEN1) > - ? chip->stage : alarm_state_map[chip->stage]; > + stage = (chip->subtype == QPNP_TM_SUBTYPE_GEN1 > + || chip->subtype == QPNP_TM_SUBTYPE_LITE) > + ? chip->stage : alarm_state_map[chip->stage]; Even more play with revisions. Please replace this with match data. > > if (stage) > chip->temp = qpnp_tm_decode_temp(chip, stage); > @@ -611,7 +802,8 @@ static int qpnp_tm_probe(struct platform_device *pdev) > } > > if (type != QPNP_TM_TYPE || (subtype != QPNP_TM_SUBTYPE_GEN1 > - && subtype != QPNP_TM_SUBTYPE_GEN2)) { > + && subtype != QPNP_TM_SUBTYPE_GEN2 > + && subtype != QPNP_TM_SUBTYPE_LITE)) { > dev_err(&pdev->dev, "invalid type 0x%02x or subtype 0x%02x\n", > type, subtype); > return -ENODEV; > @@ -662,6 +854,10 @@ static const struct of_device_id qpnp_tm_match_table[] = { > .compatible = "qcom,spmi-temp-alarm-gen2-rev2", > .data = &spmi_temp_alarm_gen2_rev2_data, > }, > + { > + .compatible = "qcom,spmi-temp-alarm-lite", > + .data = &spmi_temp_alarm_lite_data, > + }, > { } > }; > MODULE_DEVICE_TABLE(of, qpnp_tm_match_table); > -- > 2.34.1 >
>> >> +/* Configure TEMP_DAC registers based on DT thermal_zone trips */ >> +static int qpnp_tm_gen2_rev2_update_trip_temps(struct qpnp_tm_chip *chip) >> +{ >> + struct thermal_trip trip = {0}; >> + int ret, ntrips, i; >> + >> + ntrips = thermal_zone_get_num_trips(chip->tz_dev); >> + /* Keep hardware defaults if no DT trips are defined. */ >> + if (ntrips <= 0) >> + return 0; >> + >> + for (i = 0; i < ntrips; i++) { >> + ret = thermal_zone_get_trip(chip->tz_dev, i, &trip); >> + if (ret < 0) >> + return ret; >> + >> + ret = qpnp_tm_gen2_rev2_set_temp_thresh(chip, i, trip.temperature); >> + if (ret < 0) >> + return ret; >> + } >> + >> + /* Verify that trips are strictly increasing. */ > > There is no such requirement in the DT bindings. Please don't invent > artificial restrictions, especially if they are undocumented. > This is not an entirely new restirction. Currently the temp alarm driver has hardcoded temperature thresholds options which are "strictly increasing" (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thermal/qcom/qcom-spmi-temp-alarm.c?h=v6.11-rc1#n44). The threshold values are initially configured based on the stage 2 critical trip temperature. For newer PMICs, we have individual temperature registers for stage 1, 2, and 3, so we instead configure each threshold temperature as defined in DT. In general since stage 1 = warning, stage 2 = system should shut down, stage 3 = emergency shutdown, we would expect for temperature thresholds to increase for each stage (https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/thermal?h=v5.4.281&id=f1599f9e4cd6f1dd0cad202853fb830854f4e944). I agree that we are missing some documentation but since the trips are defined in the thermal_zone node what is the best way to mention this requirement? Will adding a few sentences to qcom,spmi-temp-alarm.yaml description be enough? Do we need to make changes to thermal_zone.yaml so that dt_binding_check catches this requirement? >> + for (i = 1; i < STAGE_COUNT; i++) { >> + if (chip->temp_dac_map[i] <= chip->temp_dac_map[i - 1]) { >> + dev_err(chip->dev, "Threshold %d=%ld <= threshold %d=%ld\n", >> + i, chip->temp_dac_map[i], i - 1, >> + chip->temp_dac_map[i - 1]); >> + return -EINVAL; >> + } >> + } >> + >> + return 0; Thanks, Anjelique
On Wed, 31 Jul 2024 at 01:44, Anjelique Melendez <quic_amelende@quicinc.com> wrote: > > >> > >> +/* Configure TEMP_DAC registers based on DT thermal_zone trips */ > >> +static int qpnp_tm_gen2_rev2_update_trip_temps(struct qpnp_tm_chip *chip) > >> +{ > >> + struct thermal_trip trip = {0}; > >> + int ret, ntrips, i; > >> + > >> + ntrips = thermal_zone_get_num_trips(chip->tz_dev); > >> + /* Keep hardware defaults if no DT trips are defined. */ > >> + if (ntrips <= 0) > >> + return 0; > >> + > >> + for (i = 0; i < ntrips; i++) { > >> + ret = thermal_zone_get_trip(chip->tz_dev, i, &trip); > >> + if (ret < 0) > >> + return ret; > >> + > >> + ret = qpnp_tm_gen2_rev2_set_temp_thresh(chip, i, trip.temperature); > >> + if (ret < 0) > >> + return ret; > >> + } > >> + > >> + /* Verify that trips are strictly increasing. */ > > > > There is no such requirement in the DT bindings. Please don't invent > > artificial restrictions, especially if they are undocumented. > > > > This is not an entirely new restirction. Currently the temp alarm driver > has hardcoded temperature thresholds options which are "strictly increasing" > (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thermal/qcom/qcom-spmi-temp-alarm.c?h=v6.11-rc1#n44). > The threshold values are initially configured based on the stage 2 critical trip > temperature. > For newer PMICs, we have individual temperature registers for stage 1, 2, and 3, > so we instead configure each threshold temperature as defined in DT. In general > since stage 1 = warning, stage 2 = system should shut down, stage 3 = emergency shutdown, > we would expect for temperature thresholds to increase for each stage > (https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/thermal?h=v5.4.281&id=f1599f9e4cd6f1dd0cad202853fb830854f4e944). > > I agree that we are missing some documentation but since the trips are defined in the > thermal_zone node what is the best way to mention this requirement? Will adding a > few sentences to qcom,spmi-temp-alarm.yaml description be enough? Do we need > to make changes to thermal_zone.yaml so that dt_binding_check catches this requirement? In my opinion the driver needs to be fixed to compile the state after looking at all trip points, but Daniel / Amit / Thara can have a more qualified opinion. > > >> + for (i = 1; i < STAGE_COUNT; i++) { > >> + if (chip->temp_dac_map[i] <= chip->temp_dac_map[i - 1]) { > >> + dev_err(chip->dev, "Threshold %d=%ld <= threshold %d=%ld\n", > >> + i, chip->temp_dac_map[i], i - 1, > >> + chip->temp_dac_map[i - 1]); > >> + return -EINVAL; > >> + } > >> + } > >> + > >> + return 0; > Thanks, > Anjelique
Add support in the qcom-spmi-temp-alarm driver for the new PMIC TEMP_ALARM peripheral subtypes: GEN2 rev 2 and LITE. The GEN2 rev 2 subtype provides greater flexibility in temperature threshold specification by using an independent register value to configure each of the three thresholds. The LITE subtype utilizes a simplified set of control registers to configure two thresholds: warning and shutdown. Also add support to avoid a potential issue on certain versions of the TEMP_ALARM GEN2 subtype when automatic stage 2 partial shutdown is disabled. This patch series is a continuation of the original series from 1/2023: https://lore.kernel.org/lkml/cover.1674602698.git.quic_collinsd@quicinc.com/ Anjelique Melendez (4): dt-bindings: qcom,spmi-temp-alarm: Add compatible for GEN2 rev2 temp alarm dt-bindings: qcom,spmi-temp-alarm: Add compatible for lite temp alarm thermal: qcom-spmi-temp-alarm: add support for GEN2 rev 2 PMIC peripherals thermal: qcom-spmi-temp-alarm: add support for LITE PMIC peripherals David Collins (1): thermal: qcom-spmi-temp-alarm: enable stage 2 shutdown when required .../thermal/qcom,spmi-temp-alarm.yaml | 50 +- drivers/thermal/qcom/qcom-spmi-temp-alarm.c | 433 +++++++++++++++++- 2 files changed, 463 insertions(+), 20 deletions(-)