[PATCH 0/5] thermal: qcom-spmi-temp-alarm: add support for new TEMP_ALARM subtypes

[PATCH 0/5] thermal: qcom-spmi-temp-alarm: add support for new TEMP_ALARM subtypes

[Anjelique Melendez]

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(-)

-- 
2.34.1

[PATCH 1/5] dt-bindings: qcom,spmi-temp-alarm: Add compatible for GEN2 rev2 temp alarm

[Anjelique Melendez]

Add compatible "qcom,spmi-temp-alarm-gen2-rev2" for SPMI temp alarm GEN2
revision 2 peripherals. GEN2 rev2 peripherals have individual temp DAC
registers to set temperature thresholds for over-temperature stages 1-3.
Registers are configured based on thermal zone trip definition.

Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
---
 .../devicetree/bindings/thermal/qcom,spmi-temp-alarm.yaml   | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/thermal/qcom,spmi-temp-alarm.yaml b/Documentation/devicetree/bindings/thermal/qcom,spmi-temp-alarm.yaml
index 30b22151aa82..f9af88d51c2d 100644
--- a/Documentation/devicetree/bindings/thermal/qcom,spmi-temp-alarm.yaml
+++ b/Documentation/devicetree/bindings/thermal/qcom,spmi-temp-alarm.yaml
@@ -12,14 +12,16 @@ maintainers:
 description:
   QPNP temperature alarm peripherals are found inside of Qualcomm PMIC chips
   that utilize the Qualcomm SPMI implementation. These peripherals provide an
-  interrupt signal and status register to identify high PMIC die temperature.
+  interrupt signal and status registers to identify high PMIC die temperature.
 
 allOf:
   - $ref: thermal-sensor.yaml#
 
 properties:
   compatible:
-    const: qcom,spmi-temp-alarm
+    enum:
+      - qcom,spmi-temp-alarm
+      - qcom,spmi-temp-alarm-gen2-rev2
 
   reg:
     maxItems: 1
-- 
2.34.1

[PATCH 2/5] dt-bindings: qcom,spmi-temp-alarm: Add compatible for lite temp alarm

[Anjelique Melendez]

Add compatible "qcom,spmi-temp-alarm-lite" for Temp alarm lite
peripherals. Temp alarm lite peripherals have two stages: warning and
shutdown, and use a pair of registers to configure warning interrupt
threshold temperature and an automatic hardware shutdown threshold
temperature.

When defining thermal zone trips for a temp alarm lite device the first
thermal zone trip is for warning alarm IRQ in HW, the second thermal
zone trip is purely for software to perform a controlled shutdown (no HW
support) and the third thermal zone trip is for automatic hardware
shutdown.

Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
---
 .../thermal/qcom,spmi-temp-alarm.yaml         | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/Documentation/devicetree/bindings/thermal/qcom,spmi-temp-alarm.yaml b/Documentation/devicetree/bindings/thermal/qcom,spmi-temp-alarm.yaml
index f9af88d51c2d..bbf201cad16b 100644
--- a/Documentation/devicetree/bindings/thermal/qcom,spmi-temp-alarm.yaml
+++ b/Documentation/devicetree/bindings/thermal/qcom,spmi-temp-alarm.yaml
@@ -22,6 +22,7 @@ properties:
     enum:
       - qcom,spmi-temp-alarm
       - qcom,spmi-temp-alarm-gen2-rev2
+      - qcom,spmi-temp-alarm-lite
 
   reg:
     maxItems: 1
@@ -84,3 +85,46 @@ examples:
             };
         };
     };
+
+  - |
+
+    pmic {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        pm8550b_lite_tz: pm8550b-temp-alarm-lite@c00 {
+            compatible = "qcom,spmi-temp-alarm-lite";
+            reg = <0xc00>;
+            interrupts = <0x1 0xa 0x0 IRQ_TYPE_EDGE_BOTH>;
+            #thermal-sensor-cells = <0>;
+        };
+    };
+
+    thermal-zones {
+        pm8550b_lite_temp_alarm: pm8550blite-thermal {
+            polling-delay-passive = <100>;
+            polling-delay = <0>;
+            thermal-sensors = <&pm8550b_lite_tz>;
+
+            trips {
+                pm8550b_lite_trip0: trip0 {
+                    temperature = <125000>;
+                    hysteresis = <0>;
+                    type = "passive";
+                };
+
+                pm8550b_lite_trip1: trip1 {
+                    temperature = <135000>;
+                    hysteresis = <0>;
+                    type = "passive";
+                };
+
+                trip2 {
+                    temperature = <145000>;
+                    hysteresis = <0>;
+                    type = "critical";
+                };
+
+            };
+        };
+    };
-- 
2.34.1

[PATCH 3/5] thermal: qcom-spmi-temp-alarm: enable stage 2 shutdown when required

[Anjelique Melendez]

From: David Collins <quic_collinsd@quicinc.com>

Certain TEMP_ALARM GEN2 PMIC peripherals need over-temperature
stage 2 automatic PMIC partial shutdown to be enabled in order to
avoid repeated faults in the event of reaching over-temperature
stage 3.  Modify the stage 2 shutdown control logic to ensure that
stage 2 shutdown is enabled on all affected PMICs.  Read the
digital major and minor revision registers to identify these
PMICs.

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 | 32 +++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/qcom/qcom-spmi-temp-alarm.c b/drivers/thermal/qcom/qcom-spmi-temp-alarm.c
index 96daad28b0c0..de4a2d99f0d5 100644
--- a/drivers/thermal/qcom/qcom-spmi-temp-alarm.c
+++ b/drivers/thermal/qcom/qcom-spmi-temp-alarm.c
@@ -1,6 +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.
  */
 
 #include <linux/bitops.h>
@@ -16,6 +17,7 @@
 
 #include "../thermal_hwmon.h"
 
+#define QPNP_TM_REG_DIG_MINOR		0x00
 #define QPNP_TM_REG_DIG_MAJOR		0x01
 #define QPNP_TM_REG_TYPE		0x04
 #define QPNP_TM_REG_SUBTYPE		0x05
@@ -71,6 +73,7 @@ struct qpnp_tm_chip {
 	struct device			*dev;
 	struct thermal_zone_device	*tz_dev;
 	unsigned int			subtype;
+	unsigned int			dig_revision;
 	long				temp;
 	unsigned int			thresh;
 	unsigned int			stage;
@@ -78,6 +81,7 @@ struct qpnp_tm_chip {
 	/* protects .thresh, .stage and chip registers */
 	struct mutex			lock;
 	bool				initialized;
+	bool				require_s2_shutdown;
 
 	struct iio_channel		*adc;
 	const long			(*temp_map)[THRESH_COUNT][STAGE_COUNT];
@@ -255,7 +259,7 @@ static int qpnp_tm_update_critical_trip_temp(struct qpnp_tm_chip *chip,
 
 skip:
 	reg |= chip->thresh;
-	if (disable_s2_shutdown)
+	if (disable_s2_shutdown && !chip->require_s2_shutdown)
 		reg |= SHUTDOWN_CTRL1_OVERRIDE_S2;
 
 	return qpnp_tm_write(chip, QPNP_TM_REG_SHUTDOWN_CTRL1, reg);
@@ -366,7 +370,7 @@ static int qpnp_tm_probe(struct platform_device *pdev)
 {
 	struct qpnp_tm_chip *chip;
 	struct device_node *node;
-	u8 type, subtype, dig_major;
+	u8 type, subtype, dig_major, dig_minor;
 	u32 res;
 	int ret, irq;
 
@@ -419,6 +423,30 @@ static int qpnp_tm_probe(struct platform_device *pdev)
 		return dev_err_probe(&pdev->dev, ret,
 				     "could not read dig_major\n");
 
+	ret = qpnp_tm_read(chip, QPNP_TM_REG_DIG_MINOR, &dig_minor);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "could not read dig_minor\n");
+		return ret;
+	}
+
+	chip->dig_revision = (dig_major << 8) | dig_minor;
+
+	if (chip->subtype == QPNP_TM_SUBTYPE_GEN2) {
+		/*
+		 * Check if stage 2 automatic partial shutdown must remain
+		 * enabled to avoid potential repeated faults upon reaching
+		 * over-temperature stage 3.
+		 */
+		switch (chip->dig_revision) {
+		case 0x0001:
+		case 0x0002:
+		case 0x0100:
+		case 0x0101:
+			chip->require_s2_shutdown = true;
+			break;
+		}
+	}
+
 	if (type != QPNP_TM_TYPE || (subtype != QPNP_TM_SUBTYPE_GEN1
 				     && subtype != QPNP_TM_SUBTYPE_GEN2)) {
 		dev_err(&pdev->dev, "invalid type 0x%02x or subtype 0x%02x\n",
-- 
2.34.1

[PATCH 4/5] thermal: qcom-spmi-temp-alarm: add support for GEN2 rev 2 PMIC peripherals

[Anjelique Melendez]

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. */
+	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, &reg);
+		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,
+};
+
+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;
+	}
 
 	/* 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 (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

[PATCH 5/5] thermal: qcom-spmi-temp-alarm: add support for LITE PMIC peripherals

[Anjelique Melendez]

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, &reg);
+	if (chip->subtype == QPNP_TM_SUBTYPE_LITE)
+		addr = QPNP_TM_REG_IRQ_STATUS;
+
+	ret = qpnp_tm_read(chip, addr, &reg);
 	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, &reg);
+	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. */
+	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.
+	 */
+	ret = qpnp_tm_read(chip, QPNP_TM_REG_LITE_TEMP_CFG1, &reg);
+	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, &reg);
+	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];
 
 	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

Re: [PATCH 4/5] thermal: qcom-spmi-temp-alarm: add support for GEN2 rev 2 PMIC peripherals

[Dmitry Baryshkov]

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, &reg);
> +		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
> 

-- 
With best wishes
Dmitry

Re: [PATCH 5/5] thermal: qcom-spmi-temp-alarm: add support for LITE PMIC peripherals

[Dmitry Baryshkov]

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, &reg);
> +	if (chip->subtype == QPNP_TM_SUBTYPE_LITE)
> +		addr = QPNP_TM_REG_IRQ_STATUS;
> +
> +	ret = qpnp_tm_read(chip, addr, &reg);
>  	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, &reg);
> +	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, &reg);
> +	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, &reg);
> +	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
> 

-- 
With best wishes
Dmitry

Re: [PATCH 1/5] dt-bindings: qcom,spmi-temp-alarm: Add compatible for GEN2 rev2 temp alarm

[Krzysztof Kozlowski]

On 30/07/2024 01:12, Anjelique Melendez wrote:
> Add compatible "qcom,spmi-temp-alarm-gen2-rev2" for SPMI temp alarm GEN2
> revision 2 peripherals. GEN2 rev2 peripherals have individual temp DAC
> registers to set temperature thresholds for over-temperature stages 1-3.
> Registers are configured based on thermal zone trip definition.
> 
> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
> ---
>  .../devicetree/bindings/thermal/qcom,spmi-temp-alarm.yaml   | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/qcom,spmi-temp-alarm.yaml b/Documentation/devicetree/bindings/thermal/qcom,spmi-temp-alarm.yaml
> index 30b22151aa82..f9af88d51c2d 100644
> --- a/Documentation/devicetree/bindings/thermal/qcom,spmi-temp-alarm.yaml
> +++ b/Documentation/devicetree/bindings/thermal/qcom,spmi-temp-alarm.yaml
> @@ -12,14 +12,16 @@ maintainers:
>  description:
>    QPNP temperature alarm peripherals are found inside of Qualcomm PMIC chips
>    that utilize the Qualcomm SPMI implementation. These peripherals provide an
> -  interrupt signal and status register to identify high PMIC die temperature.
> +  interrupt signal and status registers to identify high PMIC die temperature.
>  
>  allOf:
>    - $ref: thermal-sensor.yaml#
>  
>  properties:
>    compatible:
> -    const: qcom,spmi-temp-alarm
> +    enum:
> +      - qcom,spmi-temp-alarm
> +      - qcom,spmi-temp-alarm-gen2-rev2

Nah, no. I have no clue what is gen2 rev2 and no one would be able to
decipher it, even with usermanual. Do not invent some random versions.
If you want to use them, document them and make them available for public.

Use SoC compatibles. ONLY.

Best regards,
Krzysztof

Re: [PATCH 2/5] dt-bindings: qcom,spmi-temp-alarm: Add compatible for lite temp alarm

[Krzysztof Kozlowski]

On 30/07/2024 01:12, Anjelique Melendez wrote:
> Add compatible "qcom,spmi-temp-alarm-lite" for Temp alarm lite
> peripherals. Temp alarm lite peripherals have two stages: warning and
> shutdown, and use a pair of registers to configure warning interrupt
> threshold temperature and an automatic hardware shutdown threshold
> temperature.
> 
> When defining thermal zone trips for a temp alarm lite device the first
> thermal zone trip is for warning alarm IRQ in HW, the second thermal
> zone trip is purely for software to perform a controlled shutdown (no HW
> support) and the third thermal zone trip is for automatic hardware
> shutdown.
> 
> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
> ---
>  .../thermal/qcom,spmi-temp-alarm.yaml         | 44 +++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/qcom,spmi-temp-alarm.yaml b/Documentation/devicetree/bindings/thermal/qcom,spmi-temp-alarm.yaml
> index f9af88d51c2d..bbf201cad16b 100644
> --- a/Documentation/devicetree/bindings/thermal/qcom,spmi-temp-alarm.yaml
> +++ b/Documentation/devicetree/bindings/thermal/qcom,spmi-temp-alarm.yaml
> @@ -22,6 +22,7 @@ properties:
>      enum:
>        - qcom,spmi-temp-alarm
>        - qcom,spmi-temp-alarm-gen2-rev2
> +      - qcom,spmi-temp-alarm-lite

Uhu, it's growing, now we have "lite". No, this is way too ambiguous.

NAK. Use PMIC compatibles.

>  
>    reg:
>      maxItems: 1
> @@ -84,3 +85,46 @@ examples:
>              };
>          };
>      };
> +
> +  - |
> +
> +    pmic {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        pm8550b_lite_tz: pm8550b-temp-alarm-lite@c00 {

There is no difference in this example, drop.

> +            compatible = "qcom,spmi-temp-alarm-lite";
> +            reg = <0xc00>;
> +            interrupts = <0x1 0xa 0x0 IRQ_TYPE_EDGE_BOTH>;
> +            #thermal-sensor-cells = <0>;
> +        };
> +    };
> +
> +    thermal-zones {
> +        pm8550b_lite_temp_alarm: pm8550blite-thermal {

Drop as well.


Best regards,
Krzysztof

Re: [PATCH 1/5] dt-bindings: qcom,spmi-temp-alarm: Add compatible for GEN2 rev2 temp alarm

[Krzysztof Kozlowski]

On 30/07/2024 08:19, Krzysztof Kozlowski wrote:
> On 30/07/2024 01:12, Anjelique Melendez wrote:
>> Add compatible "qcom,spmi-temp-alarm-gen2-rev2" for SPMI temp alarm GEN2
>> revision 2 peripherals. GEN2 rev2 peripherals have individual temp DAC
>> registers to set temperature thresholds for over-temperature stages 1-3.
>> Registers are configured based on thermal zone trip definition.
>>
>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
>> ---
>>  .../devicetree/bindings/thermal/qcom,spmi-temp-alarm.yaml   | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/thermal/qcom,spmi-temp-alarm.yaml b/Documentation/devicetree/bindings/thermal/qcom,spmi-temp-alarm.yaml
>> index 30b22151aa82..f9af88d51c2d 100644
>> --- a/Documentation/devicetree/bindings/thermal/qcom,spmi-temp-alarm.yaml
>> +++ b/Documentation/devicetree/bindings/thermal/qcom,spmi-temp-alarm.yaml
>> @@ -12,14 +12,16 @@ maintainers:
>>  description:
>>    QPNP temperature alarm peripherals are found inside of Qualcomm PMIC chips
>>    that utilize the Qualcomm SPMI implementation. These peripherals provide an
>> -  interrupt signal and status register to identify high PMIC die temperature.
>> +  interrupt signal and status registers to identify high PMIC die temperature.
>>  
>>  allOf:
>>    - $ref: thermal-sensor.yaml#
>>  
>>  properties:
>>    compatible:
>> -    const: qcom,spmi-temp-alarm
>> +    enum:
>> +      - qcom,spmi-temp-alarm
>> +      - qcom,spmi-temp-alarm-gen2-rev2
> 
> Nah, no. I have no clue what is gen2 rev2 and no one would be able to
> decipher it, even with usermanual. Do not invent some random versions.
> If you want to use them, document them and make them available for public.
> 
> Use SoC compatibles. ONLY.

SoC->PMIC, obviously.

Best regards,
Krzysztof

Re: [PATCH 4/5] thermal: qcom-spmi-temp-alarm: add support for GEN2 rev 2 PMIC peripherals

[Anjelique Melendez]

>>  
>> +/* 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

Re: [PATCH 4/5] thermal: qcom-spmi-temp-alarm: add support for GEN2 rev 2 PMIC peripherals

[Dmitry Baryshkov]

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



-- 
With best wishes
Dmitry