[PATCH 00/15] thermal: qcom: tsens: Add interrupt support

[PATCH 00/15] thermal: qcom: tsens: Add interrupt support

[Amit Kucheria]

Add interrupt support to TSENS. The first 6 patches are general fixes and
cleanups to the driver before interrupt support is introduced.

This series has been developed against qcs404 and sdm845 and then tested on
msm8916. Testing on msm8998 and msm8974 would be appreciated since I don't
have hardware handy. Further, I plan to test on msm8996 and also submit to
kernelci.

I'm sending this out for more review to get help with testing.

Amit Kucheria (15):
  drivers: thermal: tsens: Get rid of id field in tsens_sensor
  drivers: thermal: tsens: Simplify code flow in tsens_probe
  drivers: thermal: tsens: Add __func__ identifier to debug statements
  drivers: thermal: tsens: Add debugfs support
  arm: dts: msm8974: thermal: Add thermal zones for each sensor
  arm64: dts: msm8916: thermal: Fixup HW ids for cpu sensors
  dt: thermal: tsens: Document interrupt support in tsens driver
  arm64: dts: sdm845: thermal: Add interrupt support
  arm64: dts: msm8996: thermal: Add interrupt support
  arm64: dts: msm8998: thermal: Add interrupt support
  arm64: dts: qcs404: thermal: Add interrupt support
  arm64: dts: msm8974: thermal: Add interrupt support
  arm64: dts: msm8916: thermal: Add interrupt support
  drivers: thermal: tsens: Create function to return sign-extended
    temperature
  drivers: thermal: tsens: Add interrupt support

 .../bindings/thermal/qcom-tsens.txt           |   5 +
 arch/arm/boot/dts/qcom-msm8974.dtsi           | 108 +++-
 arch/arm64/boot/dts/qcom/msm8916.dtsi         |  26 +-
 arch/arm64/boot/dts/qcom/msm8996.dtsi         |  60 +-
 arch/arm64/boot/dts/qcom/msm8998.dtsi         |  82 +--
 arch/arm64/boot/dts/qcom/qcs404.dtsi          |  42 +-
 arch/arm64/boot/dts/qcom/sdm845.dtsi          |  88 +--
 drivers/thermal/qcom/tsens-8960.c             |   4 +-
 drivers/thermal/qcom/tsens-common.c           | 610 +++++++++++++++++-
 drivers/thermal/qcom/tsens-v0_1.c             |  11 +
 drivers/thermal/qcom/tsens-v1.c               |  29 +
 drivers/thermal/qcom/tsens-v2.c               |  18 +
 drivers/thermal/qcom/tsens.c                  |  52 +-
 drivers/thermal/qcom/tsens.h                  | 285 +++++++-
 14 files changed, 1214 insertions(+), 206 deletions(-)

-- 
2.17.1

[PATCH 01/15] drivers: thermal: tsens: Get rid of id field in tsens_sensor

[Amit Kucheria]

There are two fields - id and hw_id - to track what sensor an action was
to performed on. This was because the sensors connected to a TSENS IP
might not be contiguous i.e. 1, 2, 4, 5 with 3 being skipped.

This causes confusion in the code which uses hw_id sometimes and id
other times (tsens_get_temp, tsens_get_trend).

Switch to only using the hw_id field to track the physical ID of the
sensor. When we iterate through all the sensors connected to an IP
block, we use an index i to loop through the list of sensors, and then
return the actual hw_id that is registered on that index.

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
 drivers/thermal/qcom/tsens-8960.c   |  4 ++--
 drivers/thermal/qcom/tsens-common.c | 16 +++++++++-------
 drivers/thermal/qcom/tsens.c        | 11 +++++------
 drivers/thermal/qcom/tsens.h        | 10 ++++------
 4 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/drivers/thermal/qcom/tsens-8960.c b/drivers/thermal/qcom/tsens-8960.c
index 8d9b721dadb6..3e1436fda1eb 100644
--- a/drivers/thermal/qcom/tsens-8960.c
+++ b/drivers/thermal/qcom/tsens-8960.c
@@ -243,11 +243,11 @@ static inline int code_to_mdegC(u32 adc_code, const struct tsens_sensor *s)
 	return adc_code * slope + offset;
 }
 
-static int get_temp_8960(struct tsens_priv *priv, int id, int *temp)
+static int get_temp_8960(struct tsens_sensor *s, int *temp)
 {
 	int ret;
 	u32 code, trdy;
-	const struct tsens_sensor *s = &priv->sensor[id];
+	struct tsens_priv *priv = s->priv;
 	unsigned long timeout;
 
 	timeout = jiffies + usecs_to_jiffies(TIMEOUT_US);
diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c
index 528df8801254..c037bdf92c66 100644
--- a/drivers/thermal/qcom/tsens-common.c
+++ b/drivers/thermal/qcom/tsens-common.c
@@ -83,11 +83,12 @@ static inline int code_to_degc(u32 adc_code, const struct tsens_sensor *s)
 	return degc;
 }
 
-int get_temp_tsens_valid(struct tsens_priv *priv, int i, int *temp)
+int get_temp_tsens_valid(struct tsens_sensor *s, int *temp)
 {
-	struct tsens_sensor *s = &priv->sensor[i];
-	u32 temp_idx = LAST_TEMP_0 + s->hw_id;
-	u32 valid_idx = VALID_0 + s->hw_id;
+	struct tsens_priv *priv = s->priv;
+	int hw_id = s->hw_id;
+	u32 temp_idx = LAST_TEMP_0 + hw_id;
+	u32 valid_idx = VALID_0 + hw_id;
 	u32 last_temp = 0, valid, mask;
 	int ret;
 
@@ -123,12 +124,13 @@ int get_temp_tsens_valid(struct tsens_priv *priv, int i, int *temp)
 	return 0;
 }
 
-int get_temp_common(struct tsens_priv *priv, int i, int *temp)
+int get_temp_common(struct tsens_sensor *s, int *temp)
 {
-	struct tsens_sensor *s = &priv->sensor[i];
+	struct tsens_priv *priv = s->priv;
+	int hw_id = s->hw_id;
 	int last_temp = 0, ret;
 
-	ret = regmap_field_read(priv->rf[LAST_TEMP_0 + s->hw_id], &last_temp);
+	ret = regmap_field_read(priv->rf[LAST_TEMP_0 + hw_id], &last_temp);
 	if (ret)
 		return ret;
 
diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
index 0627d8615c30..6ed687a6e53c 100644
--- a/drivers/thermal/qcom/tsens.c
+++ b/drivers/thermal/qcom/tsens.c
@@ -14,19 +14,19 @@
 
 static int tsens_get_temp(void *data, int *temp)
 {
-	const struct tsens_sensor *s = data;
+	struct tsens_sensor *s = data;
 	struct tsens_priv *priv = s->priv;
 
-	return priv->ops->get_temp(priv, s->id, temp);
+	return priv->ops->get_temp(s, temp);
 }
 
 static int tsens_get_trend(void *data, int trip, enum thermal_trend *trend)
 {
-	const struct tsens_sensor *s = data;
+	struct tsens_sensor *s = data;
 	struct tsens_priv *priv = s->priv;
 
 	if (priv->ops->get_trend)
-		return priv->ops->get_trend(priv, s->id, trend);
+		return priv->ops->get_trend(s, trend);
 
 	return -ENOTSUPP;
 }
@@ -86,8 +86,7 @@ static int tsens_register(struct tsens_priv *priv)
 
 	for (i = 0;  i < priv->num_sensors; i++) {
 		priv->sensor[i].priv = priv;
-		priv->sensor[i].id = i;
-		tzd = devm_thermal_zone_of_sensor_register(priv->dev, i,
+		tzd = devm_thermal_zone_of_sensor_register(priv->dev, priv->sensor[i].hw_id,
 							   &priv->sensor[i],
 							   &tsens_of_ops);
 		if (IS_ERR(tzd))
diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
index 2fd94997245b..d022e726d074 100644
--- a/drivers/thermal/qcom/tsens.h
+++ b/drivers/thermal/qcom/tsens.h
@@ -31,7 +31,6 @@ enum tsens_ver {
  * @priv: tsens device instance that this sensor is connected to
  * @tzd: pointer to the thermal zone that this sensor is in
  * @offset: offset of temperature adjustment curve
- * @id: Sensor ID
  * @hw_id: HW ID can be used in case of platform-specific IDs
  * @slope: slope of temperature adjustment curve
  * @status: 8960-specific variable to track 8960 and 8660 status register offset
@@ -40,7 +39,6 @@ struct tsens_sensor {
 	struct tsens_priv		*priv;
 	struct thermal_zone_device	*tzd;
 	int				offset;
-	unsigned int			id;
 	unsigned int			hw_id;
 	int				slope;
 	u32				status;
@@ -61,13 +59,13 @@ struct tsens_ops {
 	/* mandatory callbacks */
 	int (*init)(struct tsens_priv *priv);
 	int (*calibrate)(struct tsens_priv *priv);
-	int (*get_temp)(struct tsens_priv *priv, int i, int *temp);
+	int (*get_temp)(struct tsens_sensor *s, int *temp);
 	/* optional callbacks */
 	int (*enable)(struct tsens_priv *priv, int i);
 	void (*disable)(struct tsens_priv *priv);
 	int (*suspend)(struct tsens_priv *priv);
 	int (*resume)(struct tsens_priv *priv);
-	int (*get_trend)(struct tsens_priv *priv, int i, enum thermal_trend *trend);
+	int (*get_trend)(struct tsens_sensor *s, enum thermal_trend *trend);
 };
 
 #define REG_FIELD_FOR_EACH_SENSOR11(_name, _offset, _startbit, _stopbit) \
@@ -313,8 +311,8 @@ struct tsens_priv {
 char *qfprom_read(struct device *dev, const char *cname);
 void compute_intercept_slope(struct tsens_priv *priv, u32 *pt1, u32 *pt2, u32 mode);
 int init_common(struct tsens_priv *priv);
-int get_temp_tsens_valid(struct tsens_priv *priv, int i, int *temp);
-int get_temp_common(struct tsens_priv *priv, int i, int *temp);
+int get_temp_tsens_valid(struct tsens_sensor *s, int *temp);
+int get_temp_common(struct tsens_sensor *s, int *temp);
 
 /* TSENS target */
 extern const struct tsens_plat_data data_8960;
-- 
2.17.1

Re: [PATCH 01/15] drivers: thermal: tsens: Get rid of id field in tsens_sensor

[Stephen Boyd]

Quoting Amit Kucheria (2019-07-25 15:18:36)
> There are two fields - id and hw_id - to track what sensor an action was
> to performed on. This was because the sensors connected to a TSENS IP
> might not be contiguous i.e. 1, 2, 4, 5 with 3 being skipped.
> 
> This causes confusion in the code which uses hw_id sometimes and id
> other times (tsens_get_temp, tsens_get_trend).
> 
> Switch to only using the hw_id field to track the physical ID of the
> sensor. When we iterate through all the sensors connected to an IP
> block, we use an index i to loop through the list of sensors, and then
> return the actual hw_id that is registered on that index.
> 
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---

Nice cleanup!

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH 01/15] drivers: thermal: tsens: Get rid of id field in tsens_sensor

[Daniel Lezcano]

On 26/07/2019 00:18, Amit Kucheria wrote:
> There are two fields - id and hw_id - to track what sensor an action was
> to performed on. This was because the sensors connected to a TSENS IP
> might not be contiguous i.e. 1, 2, 4, 5 with 3 being skipped.
> 
> This causes confusion in the code which uses hw_id sometimes and id
> other times (tsens_get_temp, tsens_get_trend).
> 
> Switch to only using the hw_id field to track the physical ID of the
> sensor. When we iterate through all the sensors connected to an IP
> block, we use an index i to loop through the list of sensors, and then
> return the actual hw_id that is registered on that index.
> 
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>

Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>




-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH 02/15] drivers: thermal: tsens: Simplify code flow in tsens_probe

[Amit Kucheria]

Move platform_set_drvdata up to avoid an extra 'if (ret)' check after
the call to tsens_register.

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
 drivers/thermal/qcom/tsens.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
index 6ed687a6e53c..542a7f8c3d96 100644
--- a/drivers/thermal/qcom/tsens.c
+++ b/drivers/thermal/qcom/tsens.c
@@ -149,6 +149,8 @@ static int tsens_probe(struct platform_device *pdev)
 	priv->feat = data->feat;
 	priv->fields = data->fields;
 
+	platform_set_drvdata(pdev, priv);
+
 	if (!priv->ops || !priv->ops->init || !priv->ops->get_temp)
 		return -EINVAL;
 
@@ -167,11 +169,7 @@ static int tsens_probe(struct platform_device *pdev)
 		}
 	}
 
-	ret = tsens_register(priv);
-
-	platform_set_drvdata(pdev, priv);
-
-	return ret;
+	return tsens_register(priv);
 }
 
 static int tsens_remove(struct platform_device *pdev)
-- 
2.17.1

Re: [PATCH 02/15] drivers: thermal: tsens: Simplify code flow in tsens_probe

[Stephen Boyd]

Quoting Amit Kucheria (2019-07-25 15:18:37)
> Move platform_set_drvdata up to avoid an extra 'if (ret)' check after
> the call to tsens_register.
> 
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH 02/15] drivers: thermal: tsens: Simplify code flow in tsens_probe

[Daniel Lezcano]

On 26/07/2019 00:18, Amit Kucheria wrote:
> Move platform_set_drvdata up to avoid an extra 'if (ret)' check after
> the call to tsens_register.
> 
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>

Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>

[ ... ]


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH 03/15] drivers: thermal: tsens: Add __func__ identifier to debug statements

[Amit Kucheria]

Printing the function name when enabling debugging makes logs easier to
read.

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
 drivers/thermal/qcom/tsens-common.c | 8 ++++----
 drivers/thermal/qcom/tsens.c        | 6 +++---
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c
index c037bdf92c66..7437bfe196e5 100644
--- a/drivers/thermal/qcom/tsens-common.c
+++ b/drivers/thermal/qcom/tsens-common.c
@@ -42,8 +42,8 @@ void compute_intercept_slope(struct tsens_priv *priv, u32 *p1,
 
 	for (i = 0; i < priv->num_sensors; i++) {
 		dev_dbg(priv->dev,
-			"sensor%d - data_point1:%#x data_point2:%#x\n",
-			i, p1[i], p2[i]);
+			"%s: sensor%d - data_point1:%#x data_point2:%#x\n",
+			__func__, i, p1[i], p2[i]);
 
 		priv->sensor[i].slope = SLOPE_DEFAULT;
 		if (mode == TWO_PT_CALIB) {
@@ -60,7 +60,7 @@ void compute_intercept_slope(struct tsens_priv *priv, u32 *p1,
 		priv->sensor[i].offset = (p1[i] * SLOPE_FACTOR) -
 				(CAL_DEGC_PT1 *
 				priv->sensor[i].slope);
-		dev_dbg(priv->dev, "offset:%d\n", priv->sensor[i].offset);
+		dev_dbg(priv->dev, "%s: offset:%d\n", __func__, priv->sensor[i].offset);
 	}
 }
 
@@ -209,7 +209,7 @@ int __init init_common(struct tsens_priv *priv)
 	if (ret)
 		goto err_put_device;
 	if (!enabled) {
-		dev_err(dev, "tsens device is not enabled\n");
+		dev_err(dev, "%s: device not enabled\n", __func__);
 		ret = -ENODEV;
 		goto err_put_device;
 	}
diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
index 542a7f8c3d96..06c6bbd69a1a 100644
--- a/drivers/thermal/qcom/tsens.c
+++ b/drivers/thermal/qcom/tsens.c
@@ -127,7 +127,7 @@ static int tsens_probe(struct platform_device *pdev)
 		of_property_read_u32(np, "#qcom,sensors", &num_sensors);
 
 	if (num_sensors <= 0) {
-		dev_err(dev, "invalid number of sensors\n");
+		dev_err(dev, "%s: invalid number of sensors\n", __func__);
 		return -EINVAL;
 	}
 
@@ -156,7 +156,7 @@ static int tsens_probe(struct platform_device *pdev)
 
 	ret = priv->ops->init(priv);
 	if (ret < 0) {
-		dev_err(dev, "tsens init failed\n");
+		dev_err(dev, "%s: init failed\n", __func__);
 		return ret;
 	}
 
@@ -164,7 +164,7 @@ static int tsens_probe(struct platform_device *pdev)
 		ret = priv->ops->calibrate(priv);
 		if (ret < 0) {
 			if (ret != -EPROBE_DEFER)
-				dev_err(dev, "tsens calibration failed\n");
+				dev_err(dev, "%s: calibration failed\n", __func__);
 			return ret;
 		}
 	}
-- 
2.17.1

Re: [PATCH 03/15] drivers: thermal: tsens: Add __func__ identifier to debug statements

[Stephen Boyd]

Quoting Amit Kucheria (2019-07-25 15:18:38)
> Printing the function name when enabling debugging makes logs easier to
> read.
> 
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH 03/15] drivers: thermal: tsens: Add __func__ identifier to debug statements

[Daniel Lezcano]

On 26/07/2019 00:18, Amit Kucheria wrote:
> Printing the function name when enabling debugging makes logs easier to
> read.
> 
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---

Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>



-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH 04/15] drivers: thermal: tsens: Add debugfs support

[Amit Kucheria]

Dump some basic version info and sensor details into debugfs

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
 drivers/thermal/qcom/tsens-common.c | 85 +++++++++++++++++++++++++++++
 drivers/thermal/qcom/tsens.c        |  2 +
 drivers/thermal/qcom/tsens.h        |  6 ++
 3 files changed, 93 insertions(+)

diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c
index 7437bfe196e5..7ab2e740a1da 100644
--- a/drivers/thermal/qcom/tsens-common.c
+++ b/drivers/thermal/qcom/tsens-common.c
@@ -3,6 +3,7 @@
  * Copyright (c) 2015, The Linux Foundation. All rights reserved.
  */
 
+#include <linux/debugfs.h>
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/nvmem-consumer.h>
@@ -139,6 +140,79 @@ int get_temp_common(struct tsens_sensor *s, int *temp)
 	return 0;
 }
 
+#ifdef CONFIG_DEBUG_FS
+static int dbg_sensors_show(struct seq_file *s, void *data)
+{
+	struct platform_device *pdev = s->private;
+	struct tsens_priv *priv = platform_get_drvdata(pdev);
+	int i;
+
+	seq_printf(s, "max: %2d\nnum: %2d\n\n",
+		   priv->feat->max_sensors, priv->num_sensors);
+
+	seq_puts(s, "      id   slope  offset\n------------------------\n");
+	for (i = 0;  i < priv->num_sensors; i++) {
+		seq_printf(s, "%8d%8d%8d\n", priv->sensor[i].hw_id,
+			   priv->sensor[i].slope, priv->sensor[i].offset);
+	}
+
+	return 0;
+}
+
+static int dbg_version_show(struct seq_file *s, void *data)
+{
+	struct platform_device *pdev = s->private;
+	struct tsens_priv *priv = platform_get_drvdata(pdev);
+	u32 maj_ver, min_ver, step_ver;
+	int ret;
+
+	if (tsens_ver(priv) > VER_0_1) {
+		ret = regmap_field_read(priv->rf[VER_MAJOR], &maj_ver);
+		if (ret)
+			return ret;
+		ret = regmap_field_read(priv->rf[VER_MINOR], &min_ver);
+		if (ret)
+			return ret;
+		ret = regmap_field_read(priv->rf[VER_STEP], &step_ver);
+		if (ret)
+			return ret;
+		seq_printf(s, "%d.%d.%d\n", maj_ver, min_ver, step_ver);
+	} else {
+		seq_puts(s, "0.1.0\n");
+	}
+
+	return 0;
+}
+
+DEFINE_SHOW_ATTRIBUTE(dbg_version);
+DEFINE_SHOW_ATTRIBUTE(dbg_sensors);
+
+static void tsens_debug_init(struct platform_device *pdev)
+{
+	struct tsens_priv *priv = platform_get_drvdata(pdev);
+	struct dentry *root, *file;
+
+	root = debugfs_lookup("tsens", NULL);
+	if (!root)
+		priv->debug_root = debugfs_create_dir("tsens", NULL);
+	else
+		priv->debug_root = root;
+
+	file = debugfs_lookup("version", priv->debug_root);
+	if (!file)
+		debugfs_create_file("version", 0444, priv->debug_root,
+				    pdev, &dbg_version_fops);
+
+	/* A directory for each instance of the TSENS IP */
+	priv->debug = debugfs_create_dir(dev_name(&pdev->dev), priv->debug_root);
+	debugfs_create_file("sensors", 0444, priv->debug, pdev, &dbg_sensors_fops);
+
+	return;
+}
+#else
+static inline void tsens_debug_init(struct platform_device *pdev) {}
+#endif
+
 static const struct regmap_config tsens_config = {
 	.name		= "tm",
 	.reg_bits	= 32,
@@ -199,6 +273,15 @@ int __init init_common(struct tsens_priv *priv)
 		goto err_put_device;
 	}
 
+	if (tsens_ver(priv) > VER_0_1) {
+		for (i = VER_MAJOR; i <= VER_STEP; i++) {
+			priv->rf[i] = devm_regmap_field_alloc(dev, priv->srot_map,
+							      priv->fields[i]);
+			if (IS_ERR(priv->rf[i]))
+				return PTR_ERR(priv->rf[i]);
+		}
+	}
+
 	priv->rf[TSENS_EN] = devm_regmap_field_alloc(dev, priv->srot_map,
 						     priv->fields[TSENS_EN]);
 	if (IS_ERR(priv->rf[TSENS_EN])) {
@@ -238,6 +321,8 @@ int __init init_common(struct tsens_priv *priv)
 		}
 	}
 
+	tsens_debug_init(op);
+
 	return 0;
 
 err_put_device:
diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
index 06c6bbd69a1a..772aa76b50e1 100644
--- a/drivers/thermal/qcom/tsens.c
+++ b/drivers/thermal/qcom/tsens.c
@@ -3,6 +3,7 @@
  * Copyright (c) 2015, The Linux Foundation. All rights reserved.
  */
 
+#include <linux/debugfs.h>
 #include <linux/err.h>
 #include <linux/module.h>
 #include <linux/of.h>
@@ -176,6 +177,7 @@ static int tsens_remove(struct platform_device *pdev)
 {
 	struct tsens_priv *priv = platform_get_drvdata(pdev);
 
+	debugfs_remove_recursive(priv->debug_root);
 	if (priv->ops->disable)
 		priv->ops->disable(priv);
 
diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
index d022e726d074..e1d6af71b2b9 100644
--- a/drivers/thermal/qcom/tsens.h
+++ b/drivers/thermal/qcom/tsens.h
@@ -292,6 +292,8 @@ struct tsens_context {
  * @feat: features of the IP
  * @fields: bitfield locations
  * @ops: pointer to list of callbacks supported by this device
+ * @debug_root: pointer to debugfs dentry for all tsens
+ * @debug: pointer to debugfs dentry for tsens controller
  * @sensor: list of sensors attached to this device
  */
 struct tsens_priv {
@@ -305,6 +307,10 @@ struct tsens_priv {
 	const struct tsens_features	*feat;
 	const struct reg_field		*fields;
 	const struct tsens_ops		*ops;
+
+	struct dentry			*debug_root;
+	struct dentry			*debug;
+
 	struct tsens_sensor		sensor[0];
 };
 
-- 
2.17.1

Re: [PATCH 04/15] drivers: thermal: tsens: Add debugfs support

[Stephen Boyd]

Quoting Amit Kucheria (2019-07-25 15:18:39)
> Dump some basic version info and sensor details into debugfs
> 

Maybe you can put some sample output in the commit text.

> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---
>  drivers/thermal/qcom/tsens-common.c | 85 +++++++++++++++++++++++++++++
>  drivers/thermal/qcom/tsens.c        |  2 +
>  drivers/thermal/qcom/tsens.h        |  6 ++
>  3 files changed, 93 insertions(+)
> 
> diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c
> index 7437bfe196e5..7ab2e740a1da 100644
> --- a/drivers/thermal/qcom/tsens-common.c
> +++ b/drivers/thermal/qcom/tsens-common.c
> @@ -3,6 +3,7 @@
>   * Copyright (c) 2015, The Linux Foundation. All rights reserved.
>   */
>  
> +#include <linux/debugfs.h>
>  #include <linux/err.h>
>  #include <linux/io.h>
>  #include <linux/nvmem-consumer.h>
> @@ -139,6 +140,79 @@ int get_temp_common(struct tsens_sensor *s, int *temp)
>         return 0;
>  }
>  
> +#ifdef CONFIG_DEBUG_FS
> +static int dbg_sensors_show(struct seq_file *s, void *data)
> +{
> +       struct platform_device *pdev = s->private;
> +       struct tsens_priv *priv = platform_get_drvdata(pdev);
> +       int i;
> +
> +       seq_printf(s, "max: %2d\nnum: %2d\n\n",
> +                  priv->feat->max_sensors, priv->num_sensors);
> +
> +       seq_puts(s, "      id   slope  offset\n------------------------\n");
> +       for (i = 0;  i < priv->num_sensors; i++) {
> +               seq_printf(s, "%8d%8d%8d\n", priv->sensor[i].hw_id,

Does this not have spaces between the digits on purpose?

> +                          priv->sensor[i].slope, priv->sensor[i].offset);
> +       }
> +
> +       return 0;
> +}
> +
> +static int dbg_version_show(struct seq_file *s, void *data)
> +{
> +       struct platform_device *pdev = s->private;
> +       struct tsens_priv *priv = platform_get_drvdata(pdev);
> +       u32 maj_ver, min_ver, step_ver;
> +       int ret;
> +
> +       if (tsens_ver(priv) > VER_0_1) {
> +               ret = regmap_field_read(priv->rf[VER_MAJOR], &maj_ver);
> +               if (ret)
> +                       return ret;
> +               ret = regmap_field_read(priv->rf[VER_MINOR], &min_ver);
> +               if (ret)
> +                       return ret;
> +               ret = regmap_field_read(priv->rf[VER_STEP], &step_ver);
> +               if (ret)
> +                       return ret;
> +               seq_printf(s, "%d.%d.%d\n", maj_ver, min_ver, step_ver);
> +       } else {
> +               seq_puts(s, "0.1.0\n");
> +       }
> +
> +       return 0;
> +}
> +
> +DEFINE_SHOW_ATTRIBUTE(dbg_version);
> +DEFINE_SHOW_ATTRIBUTE(dbg_sensors);
> +
> +static void tsens_debug_init(struct platform_device *pdev)
> +{
> +       struct tsens_priv *priv = platform_get_drvdata(pdev);
> +       struct dentry *root, *file;
> +
> +       root = debugfs_lookup("tsens", NULL);

Does this get created many times? Why doesn't tsens have a pointer to
the root saved away somewhere globally?

> +       if (!root)
> +               priv->debug_root = debugfs_create_dir("tsens", NULL);
> +       else
> +               priv->debug_root = root;
> +
> +       file = debugfs_lookup("version", priv->debug_root);
> +       if (!file)
> +               debugfs_create_file("version", 0444, priv->debug_root,
> +                                   pdev, &dbg_version_fops);
> +
> +       /* A directory for each instance of the TSENS IP */
> +       priv->debug = debugfs_create_dir(dev_name(&pdev->dev), priv->debug_root);
> +       debugfs_create_file("sensors", 0444, priv->debug, pdev, &dbg_sensors_fops);
> +
> +       return;
> +}

Re: [PATCH 04/15] drivers: thermal: tsens: Add debugfs support

[Amit Kucheria]

On Sat, Aug 17, 2019 at 9:37 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Amit Kucheria (2019-07-25 15:18:39)
> > Dump some basic version info and sensor details into debugfs
> >
>
> Maybe you can put some sample output in the commit text.

Will do.

> > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> > ---
> >  drivers/thermal/qcom/tsens-common.c | 85 +++++++++++++++++++++++++++++
> >  drivers/thermal/qcom/tsens.c        |  2 +
> >  drivers/thermal/qcom/tsens.h        |  6 ++
> >  3 files changed, 93 insertions(+)
> >
> > diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c
> > index 7437bfe196e5..7ab2e740a1da 100644
> > --- a/drivers/thermal/qcom/tsens-common.c
> > +++ b/drivers/thermal/qcom/tsens-common.c
> > @@ -3,6 +3,7 @@
> >   * Copyright (c) 2015, The Linux Foundation. All rights reserved.
> >   */
> >
> > +#include <linux/debugfs.h>
> >  #include <linux/err.h>
> >  #include <linux/io.h>
> >  #include <linux/nvmem-consumer.h>
> > @@ -139,6 +140,79 @@ int get_temp_common(struct tsens_sensor *s, int *temp)
> >         return 0;
> >  }
> >
> > +#ifdef CONFIG_DEBUG_FS
> > +static int dbg_sensors_show(struct seq_file *s, void *data)
> > +{
> > +       struct platform_device *pdev = s->private;
> > +       struct tsens_priv *priv = platform_get_drvdata(pdev);
> > +       int i;
> > +
> > +       seq_printf(s, "max: %2d\nnum: %2d\n\n",
> > +                  priv->feat->max_sensors, priv->num_sensors);
> > +
> > +       seq_puts(s, "      id   slope  offset\n------------------------\n");
> > +       for (i = 0;  i < priv->num_sensors; i++) {
> > +               seq_printf(s, "%8d%8d%8d\n", priv->sensor[i].hw_id,
>
> Does this not have spaces between the digits on purpose?

Nice catch. The 8-char width above hid the problem. Will add.

> > +                          priv->sensor[i].slope, priv->sensor[i].offset);
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int dbg_version_show(struct seq_file *s, void *data)
> > +{
> > +       struct platform_device *pdev = s->private;
> > +       struct tsens_priv *priv = platform_get_drvdata(pdev);
> > +       u32 maj_ver, min_ver, step_ver;
> > +       int ret;
> > +
> > +       if (tsens_ver(priv) > VER_0_1) {
> > +               ret = regmap_field_read(priv->rf[VER_MAJOR], &maj_ver);
> > +               if (ret)
> > +                       return ret;
> > +               ret = regmap_field_read(priv->rf[VER_MINOR], &min_ver);
> > +               if (ret)
> > +                       return ret;
> > +               ret = regmap_field_read(priv->rf[VER_STEP], &step_ver);
> > +               if (ret)
> > +                       return ret;
> > +               seq_printf(s, "%d.%d.%d\n", maj_ver, min_ver, step_ver);
> > +       } else {
> > +               seq_puts(s, "0.1.0\n");
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +DEFINE_SHOW_ATTRIBUTE(dbg_version);
> > +DEFINE_SHOW_ATTRIBUTE(dbg_sensors);
> > +
> > +static void tsens_debug_init(struct platform_device *pdev)
> > +{
> > +       struct tsens_priv *priv = platform_get_drvdata(pdev);
> > +       struct dentry *root, *file;
> > +
> > +       root = debugfs_lookup("tsens", NULL);
>
> Does this get created many times? Why doesn't tsens have a pointer to
> the root saved away somewhere globally?
>

I guess we could call the statement below to create the root dir and
save away the pointer. I was trying to avoid #ifdef CONFIG_DEBUG_FS in
init_common() and instead have all of it in a single function that
gets called once per instance of the tsens controller.

> > +       if (!root)
> > +               priv->debug_root = debugfs_create_dir("tsens", NULL);
> > +       else
> > +               priv->debug_root = root;
> > +
> > +       file = debugfs_lookup("version", priv->debug_root);
> > +       if (!file)
> > +               debugfs_create_file("version", 0444, priv->debug_root,
> > +                                   pdev, &dbg_version_fops);
> > +
> > +       /* A directory for each instance of the TSENS IP */
> > +       priv->debug = debugfs_create_dir(dev_name(&pdev->dev), priv->debug_root);
> > +       debugfs_create_file("sensors", 0444, priv->debug, pdev, &dbg_sensors_fops);
> > +
> > +       return;
> > +}

Re: [PATCH 04/15] drivers: thermal: tsens: Add debugfs support

[Stephen Boyd]

Quoting Amit Kucheria (2019-08-19 00:58:23)
> On Sat, Aug 17, 2019 at 9:37 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > +
> > > +static void tsens_debug_init(struct platform_device *pdev)
> > > +{
> > > +       struct tsens_priv *priv = platform_get_drvdata(pdev);
> > > +       struct dentry *root, *file;
> > > +
> > > +       root = debugfs_lookup("tsens", NULL);
> >
> > Does this get created many times? Why doesn't tsens have a pointer to
> > the root saved away somewhere globally?
> >
> 
> I guess we could call the statement below to create the root dir and
> save away the pointer. I was trying to avoid #ifdef CONFIG_DEBUG_FS in
> init_common() and instead have all of it in a single function that
> gets called once per instance of the tsens controller.

Or call this code many times and try to create the tsens node if
!tsens_root exists where the variable is some global.

> 
> > > +       if (!root)
> > > +               priv->debug_root = debugfs_create_dir("tsens", NULL);
> > > +       else
> > > +               priv->debug_root = root;
> > > +

Re: [PATCH 04/15] drivers: thermal: tsens: Add debugfs support

[Amit Kucheria]

On Mon, Aug 19, 2019 at 7:57 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Amit Kucheria (2019-08-19 00:58:23)
> > On Sat, Aug 17, 2019 at 9:37 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > +
> > > > +static void tsens_debug_init(struct platform_device *pdev)
> > > > +{
> > > > +       struct tsens_priv *priv = platform_get_drvdata(pdev);
> > > > +       struct dentry *root, *file;
> > > > +
> > > > +       root = debugfs_lookup("tsens", NULL);
> > >
> > > Does this get created many times? Why doesn't tsens have a pointer to
> > > the root saved away somewhere globally?
> > >
> >
> > I guess we could call the statement below to create the root dir and
> > save away the pointer. I was trying to avoid #ifdef CONFIG_DEBUG_FS in
> > init_common() and instead have all of it in a single function that
> > gets called once per instance of the tsens controller.
>
> Or call this code many times and try to create the tsens node if
> !tsens_root exists where the variable is some global.

So I didn't quite understand this statement. The change you're
requesting is that the 'root' variable below should be a global?

tsens_probe() will get called twice on platforms with two instances of
the controller. So I will need to check some place if the 'tsens' root
dir already exists in debugfs, no? That is what I'm doing below.

> >
> > > > +       if (!root)
> > > > +               priv->debug_root = debugfs_create_dir("tsens", NULL);
> > > > +       else
> > > > +               priv->debug_root = root;
> > > > +

Re: [PATCH 04/15] drivers: thermal: tsens: Add debugfs support

[Stephen Boyd]

Quoting Amit Kucheria (2019-08-21 05:55:39)
> On Mon, Aug 19, 2019 at 7:57 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Amit Kucheria (2019-08-19 00:58:23)
> > > On Sat, Aug 17, 2019 at 9:37 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > > +
> > > > > +static void tsens_debug_init(struct platform_device *pdev)
> > > > > +{
> > > > > +       struct tsens_priv *priv = platform_get_drvdata(pdev);
> > > > > +       struct dentry *root, *file;
> > > > > +
> > > > > +       root = debugfs_lookup("tsens", NULL);
> > > >
> > > > Does this get created many times? Why doesn't tsens have a pointer to
> > > > the root saved away somewhere globally?
> > > >
> > >
> > > I guess we could call the statement below to create the root dir and
> > > save away the pointer. I was trying to avoid #ifdef CONFIG_DEBUG_FS in
> > > init_common() and instead have all of it in a single function that
> > > gets called once per instance of the tsens controller.
> >
> > Or call this code many times and try to create the tsens node if
> > !tsens_root exists where the variable is some global.
> 
> So I didn't quite understand this statement. The change you're
> requesting is that the 'root' variable below should be a global?
> 
> tsens_probe() will get called twice on platforms with two instances of
> the controller. So I will need to check some place if the 'tsens' root
> dir already exists in debugfs, no? That is what I'm doing below.
> 

Yeah. I was suggesting making a global instead of doing the lookup, but
I guess the lookup is fine and avoids a global variable. It's all
debugfs so it doesn't really matter. Sorry! Do whatever then.

[PATCH 05/15] arm: dts: msm8974: thermal: Add thermal zones for each sensor

[Amit Kucheria]

msm8974 has 11 sensors connected to a single TSENS IP. Define a thermal
zone for each of those sensors to expose the temperature of each zone.

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
Cc: masneyb@onstation.org

 arch/arm/boot/dts/qcom-msm8974.dtsi | 90 +++++++++++++++++++++++++++++
 1 file changed, 90 insertions(+)

diff --git a/arch/arm/boot/dts/qcom-msm8974.dtsi b/arch/arm/boot/dts/qcom-msm8974.dtsi
index 369e58f64145..d32f639505f1 100644
--- a/arch/arm/boot/dts/qcom-msm8974.dtsi
+++ b/arch/arm/boot/dts/qcom-msm8974.dtsi
@@ -217,6 +217,96 @@
 				};
 			};
 		};
+
+		q6-dsp-thermal {
+			polling-delay-passive = <250>;
+			polling-delay = <1000>;
+
+			thermal-sensors = <&tsens 1>;
+
+			trips {
+				q6_dsp_alert0: trip-point0 {
+					temperature = <90000>;
+					hysteresis = <2000>;
+					type = "hot";
+				};
+			};
+		};
+
+		modemtx-thermal {
+			polling-delay-passive = <250>;
+			polling-delay = <1000>;
+
+			thermal-sensors = <&tsens 2>;
+
+			trips {
+				modemtx_alert0: trip-point0 {
+					temperature = <90000>;
+					hysteresis = <2000>;
+					type = "hot";
+				};
+			};
+		};
+
+		video-thermal {
+			polling-delay-passive = <0>;
+			polling-delay = <1000>;
+
+			thermal-sensors = <&tsens 3>;
+
+			trips {
+				video_alert0: trip-point0 {
+					temperature = <95000>;
+					hysteresis = <2000>;
+					type = "hot";
+				};
+			};
+		};
+
+		wlan-thermal {
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
+
+			thermal-sensors = <&tsens 4>;
+
+			trips {
+				wlan_alert0: trip-point0 {
+					temperature = <105000>;
+					hysteresis = <2000>;
+					type = "hot";
+				};
+			};
+		};
+
+		gpu-thermal-top {
+			polling-delay-passive = <250>;
+			polling-delay = <1000>;
+
+			thermal-sensors = <&tsens 9>;
+
+			trips {
+				gpu1_alert0: trip-point0 {
+					temperature = <90000>;
+					hysteresis = <2000>;
+					type = "hot";
+				};
+			};
+		};
+
+		gpu-thermal-bottom {
+			polling-delay-passive = <250>;
+			polling-delay = <1000>;
+
+			thermal-sensors = <&tsens 10>;
+
+			trips {
+				gpu2_alert0: trip-point0 {
+					temperature = <90000>;
+					hysteresis = <2000>;
+					type = "hot";
+				};
+			};
+		};
 	};
 
 	cpu-pmu {
-- 
2.17.1

Re: [PATCH 05/15] arm: dts: msm8974: thermal: Add thermal zones for each sensor

[Brian Masney]

On Fri, Jul 26, 2019 at 03:48:40AM +0530, Amit Kucheria wrote:
> msm8974 has 11 sensors connected to a single TSENS IP. Define a thermal
> zone for each of those sensors to expose the temperature of each zone.
> 
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>

Tested-by: Brian Masney <masneyb@onstation.org> # msm8974

[PATCH 06/15] arm64: dts: msm8916: thermal: Fixup HW ids for cpu sensors

[Amit Kucheria]

msm8916 uses sensors 0, 1, 2, 4 and 5. Sensor 3 is NOT used. Fixup the
device tree so that the correct sensor ID is used and as a result we can
actually check the temperature for the cpu2_3 sensor.

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
 arch/arm64/boot/dts/qcom/msm8916.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
index 5ea9fb8f2f87..8686e101905c 100644
--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -179,7 +179,7 @@
 			polling-delay-passive = <250>;
 			polling-delay = <1000>;
 
-			thermal-sensors = <&tsens 4>;
+			thermal-sensors = <&tsens 5>;
 
 			trips {
 				cpu0_1_alert0: trip-point@0 {
@@ -209,7 +209,7 @@
 			polling-delay-passive = <250>;
 			polling-delay = <1000>;
 
-			thermal-sensors = <&tsens 3>;
+			thermal-sensors = <&tsens 4>;
 
 			trips {
 				cpu2_3_alert0: trip-point@0 {
-- 
2.17.1

Re: [PATCH 06/15] arm64: dts: msm8916: thermal: Fixup HW ids for cpu sensors

[Daniel Lezcano]

On 26/07/2019 00:18, Amit Kucheria wrote:
> msm8916 uses sensors 0, 1, 2, 4 and 5. Sensor 3 is NOT used. Fixup the
> device tree so that the correct sensor ID is used and as a result we can
> actually check the temperature for the cpu2_3 sensor.
> 
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>

This change matches the documentation available in the Qcom website.

Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>

> ---
>  arch/arm64/boot/dts/qcom/msm8916.dtsi | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> index 5ea9fb8f2f87..8686e101905c 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> @@ -179,7 +179,7 @@
>  			polling-delay-passive = <250>;
>  			polling-delay = <1000>;
>  
> -			thermal-sensors = <&tsens 4>;
> +			thermal-sensors = <&tsens 5>;
>  
>  			trips {
>  				cpu0_1_alert0: trip-point@0 {
> @@ -209,7 +209,7 @@
>  			polling-delay-passive = <250>;
>  			polling-delay = <1000>;
>  
> -			thermal-sensors = <&tsens 3>;
> +			thermal-sensors = <&tsens 4>;
>  
>  			trips {
>  				cpu2_3_alert0: trip-point@0 {
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH 07/15] dt: thermal: tsens: Document interrupt support in tsens driver

[Amit Kucheria]

Define two new required properties to define interrupts and
interrupt-names for tsens.

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
 Documentation/devicetree/bindings/thermal/qcom-tsens.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
index 673cc1831ee9..3d3dd5dc6d36 100644
--- a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
+++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
@@ -22,6 +22,8 @@ Required properties:
 
 - #thermal-sensor-cells : Should be 1. See ./thermal.txt for a description.
 - #qcom,sensors: Number of sensors in tsens block
+- interrupts: Interrupts generated from Always-On subsystem (AOSS)
+- interrupt-names: The name of the interrupt e.g. "tsens0", "tsens1"
 - Refer to Documentation/devicetree/bindings/nvmem/nvmem.txt to know how to specify
 nvmem cells
 
@@ -40,6 +42,9 @@ tsens0: thermal-sensor@c263000 {
 		reg = <0xc263000 0x1ff>, /* TM */
 			<0xc222000 0x1ff>; /* SROT */
 		#qcom,sensors = <13>;
+		interrupts = <GIC_SPI 506 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-names = "tsens0";
+
 		#thermal-sensor-cells = <1>;
 	};
 
-- 
2.17.1

Re: [PATCH 07/15] dt: thermal: tsens: Document interrupt support in tsens driver

[Rob Herring]

On Fri, Jul 26, 2019 at 03:48:42AM +0530, Amit Kucheria wrote:
> Define two new required properties to define interrupts and
> interrupt-names for tsens.
> 
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---
>  Documentation/devicetree/bindings/thermal/qcom-tsens.txt | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
> index 673cc1831ee9..3d3dd5dc6d36 100644
> --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
> +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
> @@ -22,6 +22,8 @@ Required properties:
>  
>  - #thermal-sensor-cells : Should be 1. See ./thermal.txt for a description.
>  - #qcom,sensors: Number of sensors in tsens block
> +- interrupts: Interrupts generated from Always-On subsystem (AOSS)
> +- interrupt-names: The name of the interrupt e.g. "tsens0", "tsens1"

How many interrupts? A name with just indices isn't too useful.

>  - Refer to Documentation/devicetree/bindings/nvmem/nvmem.txt to know how to specify
>  nvmem cells
>  
> @@ -40,6 +42,9 @@ tsens0: thermal-sensor@c263000 {
>  		reg = <0xc263000 0x1ff>, /* TM */
>  			<0xc222000 0x1ff>; /* SROT */
>  		#qcom,sensors = <13>;
> +		interrupts = <GIC_SPI 506 IRQ_TYPE_LEVEL_HIGH>;
> +		interrupt-names = "tsens0";
> +
>  		#thermal-sensor-cells = <1>;
>  	};
>  
> -- 
> 2.17.1
> 

Re: [PATCH 07/15] dt: thermal: tsens: Document interrupt support in tsens driver

[Amit Kucheria]

On Sat, Aug 17, 2019 at 3:06 AM Rob Herring <robh@kernel.org> wrote:
>
> On Fri, Jul 26, 2019 at 03:48:42AM +0530, Amit Kucheria wrote:
> > Define two new required properties to define interrupts and
> > interrupt-names for tsens.
> >
> > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/thermal/qcom-tsens.txt | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
> > index 673cc1831ee9..3d3dd5dc6d36 100644
> > --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
> > +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
> > @@ -22,6 +22,8 @@ Required properties:
> >
> >  - #thermal-sensor-cells : Should be 1. See ./thermal.txt for a description.
> >  - #qcom,sensors: Number of sensors in tsens block
> > +- interrupts: Interrupts generated from Always-On subsystem (AOSS)
> > +- interrupt-names: The name of the interrupt e.g. "tsens0", "tsens1"
>
> How many interrupts? A name with just indices isn't too useful.

Depending on the version of the tsens IP, there can be 1 (upper/lower
threshold), 2 (upper/lower + critical threshold) or 3 (upper/lower +
critical + zero degree) interrupts. This patch series only introduces
support for a single interrupt (upper/lower).

I used the names tsens0, tsens1 to encapsulate the controller instance
since some SoCs have 1 controller, others have two. So we'll end up
with something like the following in DT:

tsens0: thermal-sensor@c263000 {
                        compatible = "qcom,sdm845-tsens", "qcom,tsens-v2";
                        reg = <0 0x0c263000 0 0x1ff>, /* TM */
                              <0 0x0c222000 0 0x1ff>; /* SROT */
                        #qcom,sensors = <13>;
                        interrupts = <GIC_SPI 506 IRQ_TYPE_LEVEL_HIGH>,
                                     <GIC_SPI 508 IRQ_TYPE_LEVEL_HIGH>;
                        interrupt-names = "tsens0", "tsens0-critical";
                        #thermal-sensor-cells = <1>;
};

tsens1: thermal-sensor@c265000 {
                        compatible = "qcom,sdm845-tsens", "qcom,tsens-v2";
                        reg = <0 0x0c265000 0 0x1ff>, /* TM */
                              <0 0x0c223000 0 0x1ff>; /* SROT */
                        #qcom,sensors = <8>;
                        interrupts = <GIC_SPI 507 IRQ_TYPE_LEVEL_HIGH>,
                                     <GIC_SPI 509 IRQ_TYPE_LEVEL_HIGH>;
                        interrupt-names = "tsens1", "tsens1-critical";
                        #thermal-sensor-cells = <1>;
}

Does that work?

Regards,
Amit

> >  - Refer to Documentation/devicetree/bindings/nvmem/nvmem.txt to know how to specify
> >  nvmem cells
> >
> > @@ -40,6 +42,9 @@ tsens0: thermal-sensor@c263000 {
> >               reg = <0xc263000 0x1ff>, /* TM */
> >                       <0xc222000 0x1ff>; /* SROT */
> >               #qcom,sensors = <13>;
> > +             interrupts = <GIC_SPI 506 IRQ_TYPE_LEVEL_HIGH>;
> > +             interrupt-names = "tsens0";
> > +
> >               #thermal-sensor-cells = <1>;
> >       };
> >
> > --
> > 2.17.1
> >

Re: [PATCH 07/15] dt: thermal: tsens: Document interrupt support in tsens driver

[Stephen Boyd]

Quoting Amit Kucheria (2019-08-16 15:02:08)
> 
> Depending on the version of the tsens IP, there can be 1 (upper/lower
> threshold), 2 (upper/lower + critical threshold) or 3 (upper/lower +
> critical + zero degree) interrupts. This patch series only introduces
> support for a single interrupt (upper/lower).
> 
> I used the names tsens0, tsens1 to encapsulate the controller instance
> since some SoCs have 1 controller, others have two. So we'll end up
> with something like the following in DT:
> 
> tsens0: thermal-sensor@c263000 {
>                         compatible = "qcom,sdm845-tsens", "qcom,tsens-v2";
>                         reg = <0 0x0c263000 0 0x1ff>, /* TM */
>                               <0 0x0c222000 0 0x1ff>; /* SROT */
>                         #qcom,sensors = <13>;
>                         interrupts = <GIC_SPI 506 IRQ_TYPE_LEVEL_HIGH>,
>                                      <GIC_SPI 508 IRQ_TYPE_LEVEL_HIGH>;
>                         interrupt-names = "tsens0", "tsens0-critical";
>                         #thermal-sensor-cells = <1>;
> };
> 
> tsens1: thermal-sensor@c265000 {
>                         compatible = "qcom,sdm845-tsens", "qcom,tsens-v2";
>                         reg = <0 0x0c265000 0 0x1ff>, /* TM */
>                               <0 0x0c223000 0 0x1ff>; /* SROT */
>                         #qcom,sensors = <8>;
>                         interrupts = <GIC_SPI 507 IRQ_TYPE_LEVEL_HIGH>,
>                                      <GIC_SPI 509 IRQ_TYPE_LEVEL_HIGH>;
>                         interrupt-names = "tsens1", "tsens1-critical";
>                         #thermal-sensor-cells = <1>;
> }
> 
> Does that work?
> 

Can you convert this binding to YAML? Then it looks like we can enforce
the number of interrupts based on the compatible string.

Re: [PATCH 07/15] dt: thermal: tsens: Document interrupt support in tsens driver

[Rob Herring]

On Fri, Aug 16, 2019 at 5:02 PM Amit Kucheria <amit.kucheria@linaro.org> wrote:
>
> On Sat, Aug 17, 2019 at 3:06 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Fri, Jul 26, 2019 at 03:48:42AM +0530, Amit Kucheria wrote:
> > > Define two new required properties to define interrupts and
> > > interrupt-names for tsens.
> > >
> > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> > > ---
> > >  Documentation/devicetree/bindings/thermal/qcom-tsens.txt | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
> > > index 673cc1831ee9..3d3dd5dc6d36 100644
> > > --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
> > > +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
> > > @@ -22,6 +22,8 @@ Required properties:
> > >
> > >  - #thermal-sensor-cells : Should be 1. See ./thermal.txt for a description.
> > >  - #qcom,sensors: Number of sensors in tsens block
> > > +- interrupts: Interrupts generated from Always-On subsystem (AOSS)
> > > +- interrupt-names: The name of the interrupt e.g. "tsens0", "tsens1"
> >
> > How many interrupts? A name with just indices isn't too useful.
>
> Depending on the version of the tsens IP, there can be 1 (upper/lower
> threshold), 2 (upper/lower + critical threshold) or 3 (upper/lower +
> critical + zero degree) interrupts. This patch series only introduces
> support for a single interrupt (upper/lower).

I would expect a different compatible for each possibility.

> I used the names tsens0, tsens1 to encapsulate the controller instance
> since some SoCs have 1 controller, others have two. So we'll end up
> with something like the following in DT:

That's not really how *-names is supposed to work. The name is for
identifying what is at each index. Or to put it another way, a driver
should be able to use platform_get_irq_by_name(). So 'critical',
'zero' and something for the first one.

> tsens0: thermal-sensor@c263000 {
>                         compatible = "qcom,sdm845-tsens", "qcom,tsens-v2";
>                         reg = <0 0x0c263000 0 0x1ff>, /* TM */
>                               <0 0x0c222000 0 0x1ff>; /* SROT */
>                         #qcom,sensors = <13>;
>                         interrupts = <GIC_SPI 506 IRQ_TYPE_LEVEL_HIGH>,
>                                      <GIC_SPI 508 IRQ_TYPE_LEVEL_HIGH>;
>                         interrupt-names = "tsens0", "tsens0-critical";
>                         #thermal-sensor-cells = <1>;
> };
>
> tsens1: thermal-sensor@c265000 {
>                         compatible = "qcom,sdm845-tsens", "qcom,tsens-v2";
>                         reg = <0 0x0c265000 0 0x1ff>, /* TM */
>                               <0 0x0c223000 0 0x1ff>; /* SROT */
>                         #qcom,sensors = <8>;
>                         interrupts = <GIC_SPI 507 IRQ_TYPE_LEVEL_HIGH>,
>                                      <GIC_SPI 509 IRQ_TYPE_LEVEL_HIGH>;
>                         interrupt-names = "tsens1", "tsens1-critical";
>                         #thermal-sensor-cells = <1>;
> }
>
> Does that work?
>
> Regards,
> Amit
>
> > >  - Refer to Documentation/devicetree/bindings/nvmem/nvmem.txt to know how to specify
> > >  nvmem cells
> > >
> > > @@ -40,6 +42,9 @@ tsens0: thermal-sensor@c263000 {
> > >               reg = <0xc263000 0x1ff>, /* TM */
> > >                       <0xc222000 0x1ff>; /* SROT */
> > >               #qcom,sensors = <13>;
> > > +             interrupts = <GIC_SPI 506 IRQ_TYPE_LEVEL_HIGH>;
> > > +             interrupt-names = "tsens0";
> > > +
> > >               #thermal-sensor-cells = <1>;
> > >       };
> > >
> > > --
> > > 2.17.1
> > >

Re: [PATCH 07/15] dt: thermal: tsens: Document interrupt support in tsens driver

[Amit Kucheria]

On Sun, Aug 18, 2019 at 12:55 AM Rob Herring <robh@kernel.org> wrote:
>
> On Fri, Aug 16, 2019 at 5:02 PM Amit Kucheria <amit.kucheria@linaro.org> wrote:
> >
> > On Sat, Aug 17, 2019 at 3:06 AM Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Fri, Jul 26, 2019 at 03:48:42AM +0530, Amit Kucheria wrote:
> > > > Define two new required properties to define interrupts and
> > > > interrupt-names for tsens.
> > > >
> > > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> > > > ---
> > > >  Documentation/devicetree/bindings/thermal/qcom-tsens.txt | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
> > > > index 673cc1831ee9..3d3dd5dc6d36 100644
> > > > --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
> > > > +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
> > > > @@ -22,6 +22,8 @@ Required properties:
> > > >
> > > >  - #thermal-sensor-cells : Should be 1. See ./thermal.txt for a description.
> > > >  - #qcom,sensors: Number of sensors in tsens block
> > > > +- interrupts: Interrupts generated from Always-On subsystem (AOSS)
> > > > +- interrupt-names: The name of the interrupt e.g. "tsens0", "tsens1"
> > >
> > > How many interrupts? A name with just indices isn't too useful.
> >
> > Depending on the version of the tsens IP, there can be 1 (upper/lower
> > threshold), 2 (upper/lower + critical threshold) or 3 (upper/lower +
> > critical + zero degree) interrupts. This patch series only introduces
> > support for a single interrupt (upper/lower).
>
> I would expect a different compatible for each possibility.

We're currently using the 'qcom,tsens-v1' and 'qcom,tsens-v2'
compatibles to broadly capture the feature (and register map)
differences.

By defining the following, I should be able to check at runtime (using
platform_get_irq_by_name() as suggested) if a particular interrupt
type is available on the platform, no? So do we really require three
different compatibles?

    interrupt-names = "uplow", "crit", "cold"

[1] Respin of older SoC with a newer version of IP

> > I used the names tsens0, tsens1 to encapsulate the controller instance
> > since some SoCs have 1 controller, others have two. So we'll end up
> > with something like the following in DT:
>
> That's not really how *-names is supposed to work. The name is for
> identifying what is at each index. Or to put it another way, a driver
> should be able to use platform_get_irq_by_name(). So 'critical',
> 'zero' and something for the first one.

Fair point. I'll rework it to use "uplow", "crit" and "cold" or
something to the effect.

Is there another way I get the controller instance index in the name
in /proc/interrupts?

> > tsens0: thermal-sensor@c263000 {
> >                         compatible = "qcom,sdm845-tsens", "qcom,tsens-v2";
> >                         reg = <0 0x0c263000 0 0x1ff>, /* TM */
> >                               <0 0x0c222000 0 0x1ff>; /* SROT */
> >                         #qcom,sensors = <13>;
> >                         interrupts = <GIC_SPI 506 IRQ_TYPE_LEVEL_HIGH>,
> >                                      <GIC_SPI 508 IRQ_TYPE_LEVEL_HIGH>;
> >                         interrupt-names = "tsens0", "tsens0-critical";
> >                         #thermal-sensor-cells = <1>;
> > };
> >
> > tsens1: thermal-sensor@c265000 {
> >                         compatible = "qcom,sdm845-tsens", "qcom,tsens-v2";
> >                         reg = <0 0x0c265000 0 0x1ff>, /* TM */
> >                               <0 0x0c223000 0 0x1ff>; /* SROT */
> >                         #qcom,sensors = <8>;
> >                         interrupts = <GIC_SPI 507 IRQ_TYPE_LEVEL_HIGH>,
> >                                      <GIC_SPI 509 IRQ_TYPE_LEVEL_HIGH>;
> >                         interrupt-names = "tsens1", "tsens1-critical";
> >                         #thermal-sensor-cells = <1>;
> > }
> >
> > Does that work?
> >
> > Regards,
> > Amit
> >
> > > >  - Refer to Documentation/devicetree/bindings/nvmem/nvmem.txt to know how to specify
> > > >  nvmem cells
> > > >
> > > > @@ -40,6 +42,9 @@ tsens0: thermal-sensor@c263000 {
> > > >               reg = <0xc263000 0x1ff>, /* TM */
> > > >                       <0xc222000 0x1ff>; /* SROT */
> > > >               #qcom,sensors = <13>;
> > > > +             interrupts = <GIC_SPI 506 IRQ_TYPE_LEVEL_HIGH>;
> > > > +             interrupt-names = "tsens0";
> > > > +
> > > >               #thermal-sensor-cells = <1>;
> > > >       };
> > > >
> > > > --
> > > > 2.17.1
> > > >

Re: [PATCH 07/15] dt: thermal: tsens: Document interrupt support in tsens driver

[Rob Herring]

On Mon, Aug 19, 2019 at 2:10 AM Amit Kucheria <amit.kucheria@linaro.org> wrote:
>
> On Sun, Aug 18, 2019 at 12:55 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Fri, Aug 16, 2019 at 5:02 PM Amit Kucheria <amit.kucheria@linaro.org> wrote:
> > >
> > > On Sat, Aug 17, 2019 at 3:06 AM Rob Herring <robh@kernel.org> wrote:
> > > >
> > > > On Fri, Jul 26, 2019 at 03:48:42AM +0530, Amit Kucheria wrote:
> > > > > Define two new required properties to define interrupts and
> > > > > interrupt-names for tsens.
> > > > >
> > > > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> > > > > ---
> > > > >  Documentation/devicetree/bindings/thermal/qcom-tsens.txt | 5 +++++
> > > > >  1 file changed, 5 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
> > > > > index 673cc1831ee9..3d3dd5dc6d36 100644
> > > > > --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
> > > > > +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
> > > > > @@ -22,6 +22,8 @@ Required properties:
> > > > >
> > > > >  - #thermal-sensor-cells : Should be 1. See ./thermal.txt for a description.
> > > > >  - #qcom,sensors: Number of sensors in tsens block
> > > > > +- interrupts: Interrupts generated from Always-On subsystem (AOSS)
> > > > > +- interrupt-names: The name of the interrupt e.g. "tsens0", "tsens1"
> > > >
> > > > How many interrupts? A name with just indices isn't too useful.
> > >
> > > Depending on the version of the tsens IP, there can be 1 (upper/lower
> > > threshold), 2 (upper/lower + critical threshold) or 3 (upper/lower +
> > > critical + zero degree) interrupts. This patch series only introduces
> > > support for a single interrupt (upper/lower).
> >
> > I would expect a different compatible for each possibility.
>
> We're currently using the 'qcom,tsens-v1' and 'qcom,tsens-v2'
> compatibles to broadly capture the feature (and register map)
> differences.
>
> By defining the following, I should be able to check at runtime (using
> platform_get_irq_by_name() as suggested) if a particular interrupt
> type is available on the platform, no? So do we really require three
> different compatibles?

Yes and no. I would assume the SoC specific compatibles would meet
this, but the driver can ignore that and just use
platform_get_irq_by_name() or count the number of interrupts.

>     interrupt-names = "uplow", "crit", "cold"
>
> [1] Respin of older SoC with a newer version of IP
>
> > > I used the names tsens0, tsens1 to encapsulate the controller instance
> > > since some SoCs have 1 controller, others have two. So we'll end up
> > > with something like the following in DT:
> >
> > That's not really how *-names is supposed to work. The name is for
> > identifying what is at each index. Or to put it another way, a driver
> > should be able to use platform_get_irq_by_name(). So 'critical',
> > 'zero' and something for the first one.
>
> Fair point. I'll rework it to use "uplow", "crit" and "cold" or
> something to the effect.
>
> Is there another way I get the controller instance index in the name
> in /proc/interrupts?

Not sure offhand. Add the dev_name() into the IRQ name perhaps.

Rob

[PATCH 08/15] arm64: dts: sdm845: thermal: Add interrupt support

[Amit Kucheria]

Register upper-lower interrupts for each of the two tsens controllers.

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 88 +++++++++++++++-------------
 1 file changed, 46 insertions(+), 42 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 4babff5f19b5..43c06e54b69d 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -2386,6 +2386,8 @@
 			reg = <0 0x0c263000 0 0x1ff>, /* TM */
 			      <0 0x0c222000 0 0x1ff>; /* SROT */
 			#qcom,sensors = <13>;
+			interrupts = <GIC_SPI 506 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "tsens0";
 			#thermal-sensor-cells = <1>;
 		};
 
@@ -2394,6 +2396,8 @@
 			reg = <0 0x0c265000 0 0x1ff>, /* TM */
 			      <0 0x0c223000 0 0x1ff>; /* SROT */
 			#qcom,sensors = <8>;
+			interrupts = <GIC_SPI 507 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "tsens1";
 			#thermal-sensor-cells = <1>;
 		};
 
@@ -2712,8 +2716,8 @@
 
 	thermal-zones {
 		cpu0-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 1>;
 
@@ -2756,8 +2760,8 @@
 		};
 
 		cpu1-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 2>;
 
@@ -2800,8 +2804,8 @@
 		};
 
 		cpu2-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 3>;
 
@@ -2844,8 +2848,8 @@
 		};
 
 		cpu3-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 4>;
 
@@ -2888,8 +2892,8 @@
 		};
 
 		cpu4-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 7>;
 
@@ -2932,8 +2936,8 @@
 		};
 
 		cpu5-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 8>;
 
@@ -2976,8 +2980,8 @@
 		};
 
 		cpu6-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 9>;
 
@@ -3020,8 +3024,8 @@
 		};
 
 		cpu7-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 10>;
 
@@ -3064,8 +3068,8 @@
 		};
 
 		aoss0-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 0>;
 
@@ -3079,8 +3083,8 @@
 		};
 
 		cluster0-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 5>;
 
@@ -3099,8 +3103,8 @@
 		};
 
 		cluster1-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 6>;
 
@@ -3119,8 +3123,8 @@
 		};
 
 		gpu-thermal-top {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 11>;
 
@@ -3134,8 +3138,8 @@
 		};
 
 		gpu-thermal-bottom {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 12>;
 
@@ -3149,8 +3153,8 @@
 		};
 
 		aoss1-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens1 0>;
 
@@ -3164,8 +3168,8 @@
 		};
 
 		q6-modem-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens1 1>;
 
@@ -3179,8 +3183,8 @@
 		};
 
 		mem-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens1 2>;
 
@@ -3194,8 +3198,8 @@
 		};
 
 		wlan-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens1 3>;
 
@@ -3209,8 +3213,8 @@
 		};
 
 		q6-hvx-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens1 4>;
 
@@ -3224,8 +3228,8 @@
 		};
 
 		camera-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens1 5>;
 
@@ -3239,8 +3243,8 @@
 		};
 
 		video-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens1 6>;
 
@@ -3254,8 +3258,8 @@
 		};
 
 		modem-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens1 7>;
 
-- 
2.17.1

[PATCH 09/15] arm64: dts: msm8996: thermal: Add interrupt support

[Amit Kucheria]

Register upper-lower interrupts for each of the two tsens controllers.

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
 arch/arm64/boot/dts/qcom/msm8996.dtsi | 60 ++++++++++++++-------------
 1 file changed, 32 insertions(+), 28 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index 96c0a481f454..7325eba42d19 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -175,8 +175,8 @@
 
 	thermal-zones {
 		cpu0-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 3>;
 
@@ -196,8 +196,8 @@
 		};
 
 		cpu1-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 5>;
 
@@ -217,8 +217,8 @@
 		};
 
 		cpu2-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 8>;
 
@@ -238,8 +238,8 @@
 		};
 
 		cpu3-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 10>;
 
@@ -259,8 +259,8 @@
 		};
 
 		gpu-thermal-top {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens1 6>;
 
@@ -274,8 +274,8 @@
 		};
 
 		gpu-thermal-bottom {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens1 7>;
 
@@ -289,8 +289,8 @@
 		};
 
 		m4m-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 1>;
 
@@ -304,8 +304,8 @@
 		};
 
 		l3-or-venus-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 2>;
 
@@ -319,8 +319,8 @@
 		};
 
 		cluster0-l2-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 7>;
 
@@ -334,8 +334,8 @@
 		};
 
 		cluster1-l2-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 12>;
 
@@ -349,8 +349,8 @@
 		};
 
 		camera-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens1 1>;
 
@@ -364,8 +364,8 @@
 		};
 
 		q6-dsp-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens1 2>;
 
@@ -379,8 +379,8 @@
 		};
 
 		mem-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens1 3>;
 
@@ -394,8 +394,8 @@
 		};
 
 		modemtx-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens1 4>;
 
@@ -591,6 +591,8 @@
 			reg = <0x4a9000 0x1000>, /* TM */
 			      <0x4a8000 0x1000>; /* SROT */
 			#qcom,sensors = <13>;
+			interrupts = <GIC_SPI 458 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "tsens0";
 			#thermal-sensor-cells = <1>;
 		};
 
@@ -599,6 +601,8 @@
 			reg = <0x4ad000 0x1000>, /* TM */
 			      <0x4ac000 0x1000>; /* SROT */
 			#qcom,sensors = <8>;
+			interrupts = <GIC_SPI 184 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "tsens1";
 			#thermal-sensor-cells = <1>;
 		};
 
-- 
2.17.1

[PATCH 10/15] arm64: dts: msm8998: thermal: Add interrupt support

[Amit Kucheria]

Register upper-lower interrupts for each of the two tsens controllers.

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
Cc: marc.w.gonzalez@free.fr

 arch/arm64/boot/dts/qcom/msm8998.dtsi | 82 ++++++++++++++-------------
 1 file changed, 42 insertions(+), 40 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
index c13ed7aeb1e0..f9abd652a544 100644
--- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
@@ -440,8 +440,8 @@
 
 	thermal-zones {
 		cpu0-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 1>;
 
@@ -461,8 +461,8 @@
 		};
 
 		cpu1-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 2>;
 
@@ -482,8 +482,8 @@
 		};
 
 		cpu2-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 3>;
 
@@ -503,8 +503,8 @@
 		};
 
 		cpu3-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 4>;
 
@@ -524,8 +524,8 @@
 		};
 
 		cpu4-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 7>;
 
@@ -545,8 +545,8 @@
 		};
 
 		cpu5-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 8>;
 
@@ -566,8 +566,8 @@
 		};
 
 		cpu6-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 9>;
 
@@ -587,8 +587,8 @@
 		};
 
 		cpu7-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 10>;
 
@@ -608,8 +608,8 @@
 		};
 
 		gpu-thermal-bottom {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 12>;
 
@@ -623,8 +623,8 @@
 		};
 
 		gpu-thermal-top {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 13>;
 
@@ -638,8 +638,8 @@
 		};
 
 		clust0-mhm-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 5>;
 
@@ -653,8 +653,8 @@
 		};
 
 		clust1-mhm-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 6>;
 
@@ -668,8 +668,8 @@
 		};
 
 		cluster1-l2-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens0 11>;
 
@@ -683,8 +683,8 @@
 		};
 
 		modem-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens1 1>;
 
@@ -698,8 +698,8 @@
 		};
 
 		mem-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens1 2>;
 
@@ -713,8 +713,8 @@
 		};
 
 		wlan-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens1 3>;
 
@@ -728,8 +728,8 @@
 		};
 
 		q6-dsp-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens1 4>;
 
@@ -743,8 +743,8 @@
 		};
 
 		camera-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens1 5>;
 
@@ -758,8 +758,8 @@
 		};
 
 		multimedia-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens1 6>;
 
@@ -845,8 +845,9 @@
 			compatible = "qcom,msm8998-tsens", "qcom,tsens-v2";
 			reg = <0x10ab000 0x1000>, /* TM */
 			      <0x10aa000 0x1000>; /* SROT */
-
 			#qcom,sensors = <14>;
+			interrupts = <GIC_SPI 458 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "tsens0";
 			#thermal-sensor-cells = <1>;
 		};
 
@@ -854,8 +855,9 @@
 			compatible = "qcom,msm8998-tsens", "qcom,tsens-v2";
 			reg = <0x10ae000 0x1000>, /* TM */
 			      <0x10ad000 0x1000>; /* SROT */
-
 			#qcom,sensors = <8>;
+			interrupts = <GIC_SPI 184 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "tsens1";
 			#thermal-sensor-cells = <1>;
 		};
 
-- 
2.17.1

[PATCH 11/15] arm64: dts: qcs404: thermal: Add interrupt support

[Amit Kucheria]

Register upper-lower interrupt for the tsens controller.

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
 arch/arm64/boot/dts/qcom/qcs404.dtsi | 42 +++++++++++++++-------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi
index 3d0789775009..0c715f9cc011 100644
--- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
+++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
@@ -280,6 +280,8 @@
 			nvmem-cells = <&tsens_caldata>;
 			nvmem-cell-names = "calib";
 			#qcom,sensors = <10>;
+			interrupts = <GIC_SPI 184 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "tsens";
 			#thermal-sensor-cells = <1>;
 		};
 
@@ -1071,8 +1073,8 @@
 
 	thermal-zones {
 		aoss-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens 0>;
 
@@ -1086,8 +1088,8 @@
 		};
 
 		q6-hvx-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens 1>;
 
@@ -1101,8 +1103,8 @@
 		};
 
 		lpass-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens 2>;
 
@@ -1116,8 +1118,8 @@
 		};
 
 		wlan-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens 3>;
 
@@ -1131,8 +1133,8 @@
 		};
 
 		cluster-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens 4>;
 
@@ -1165,8 +1167,8 @@
 		};
 
 		cpu0-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens 5>;
 
@@ -1199,8 +1201,8 @@
 		};
 
 		cpu1-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens 6>;
 
@@ -1233,8 +1235,8 @@
 		};
 
 		cpu2-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens 7>;
 
@@ -1267,8 +1269,8 @@
 		};
 
 		cpu3-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens 8>;
 
@@ -1301,8 +1303,8 @@
 		};
 
 		gpu-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens 9>;
 
-- 
2.17.1

[PATCH 12/15] arm64: dts: msm8974: thermal: Add interrupt support

[Amit Kucheria]

Register upper-lower interrupt for the tsens controller.

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
Cc: masneyb@onstation.org

 arch/arm/boot/dts/qcom-msm8974.dtsi | 36 +++++++++++++++--------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/arch/arm/boot/dts/qcom-msm8974.dtsi b/arch/arm/boot/dts/qcom-msm8974.dtsi
index d32f639505f1..d10d47d20ab8 100644
--- a/arch/arm/boot/dts/qcom-msm8974.dtsi
+++ b/arch/arm/boot/dts/qcom-msm8974.dtsi
@@ -139,8 +139,8 @@
 
 	thermal-zones {
 		cpu-thermal0 {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens 5>;
 
@@ -159,8 +159,8 @@
 		};
 
 		cpu-thermal1 {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens 6>;
 
@@ -179,8 +179,8 @@
 		};
 
 		cpu-thermal2 {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens 7>;
 
@@ -199,8 +199,8 @@
 		};
 
 		cpu-thermal3 {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens 8>;
 
@@ -219,8 +219,8 @@
 		};
 
 		q6-dsp-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens 1>;
 
@@ -234,8 +234,8 @@
 		};
 
 		modemtx-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens 2>;
 
@@ -250,7 +250,7 @@
 
 		video-thermal {
 			polling-delay-passive = <0>;
-			polling-delay = <1000>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens 3>;
 
@@ -279,8 +279,8 @@
 		};
 
 		gpu-thermal-top {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens 9>;
 
@@ -294,8 +294,8 @@
 		};
 
 		gpu-thermal-bottom {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens 10>;
 
@@ -531,6 +531,8 @@
 			nvmem-cells = <&tsens_calib>, <&tsens_backup>;
 			nvmem-cell-names = "calib", "calib_backup";
 			#qcom,sensors = <11>;
+			interrupts = <GIC_SPI 184 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "tsens";
 			#thermal-sensor-cells = <1>;
 		};
 
-- 
2.17.1

Re: [PATCH 12/15] arm64: dts: msm8974: thermal: Add interrupt support

[Luca Weiss]

[-- Attachment #1: Type: text/plain, Size: 416 bytes --]

On Freitag, 26. Juli 2019 00:18:47 CEST Amit Kucheria wrote:
> Register upper-lower interrupt for the tsens controller.
> 
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---
> Cc: masneyb@onstation.org
> 
>  arch/arm/boot/dts/qcom-msm8974.dtsi | 36 +++++++++++++++--------------
>  1 file changed, 19 insertions(+), 17 deletions(-)
> 

Hi, the title of this patch should be "arm" and not "arm64".

Luca

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 12/15] arm64: dts: msm8974: thermal: Add interrupt support

[Amit Kucheria]

On Mon, Jul 29, 2019 at 2:33 PM Luca Weiss <luca@z3ntu.xyz> wrote:
>
> On Freitag, 26. Juli 2019 00:18:47 CEST Amit Kucheria wrote:
> > Register upper-lower interrupt for the tsens controller.
> >
> > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> > ---
> > Cc: masneyb@onstation.org
> >
> >  arch/arm/boot/dts/qcom-msm8974.dtsi | 36 +++++++++++++++--------------
> >  1 file changed, 19 insertions(+), 17 deletions(-)
> >
>
> Hi, the title of this patch should be "arm" and not "arm64".

Good catch! Copy-paste error, will fix.

Re: [PATCH 12/15] arm64: dts: msm8974: thermal: Add interrupt support

[Brian Masney]

On Fri, Jul 26, 2019 at 03:48:47AM +0530, Amit Kucheria wrote:
> Register upper-lower interrupt for the tsens controller.
> 
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>

Tested-by: Brian Masney <masneyb@onstation.org>

[PATCH 13/15] arm64: dts: msm8916: thermal: Add interrupt support

[Amit Kucheria]

Register upper-lower interrupt for the tsens controller.

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
 arch/arm64/boot/dts/qcom/msm8916.dtsi | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
index 8686e101905c..400045a100ca 100644
--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -176,8 +176,8 @@
 
 	thermal-zones {
 		cpu0_1-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens 5>;
 
@@ -206,8 +206,8 @@
 		};
 
 		cpu2_3-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens 4>;
 
@@ -236,8 +236,8 @@
 		};
 
 		gpu-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens 2>;
 
@@ -256,8 +256,8 @@
 		};
 
 		camera-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens 1>;
 
@@ -271,8 +271,8 @@
 		};
 
 		modem-thermal {
-			polling-delay-passive = <250>;
-			polling-delay = <1000>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
 
 			thermal-sensors = <&tsens 0>;
 
@@ -816,6 +816,8 @@
 			nvmem-cells = <&tsens_caldata>, <&tsens_calsel>;
 			nvmem-cell-names = "calib", "calib_sel";
 			#qcom,sensors = <5>;
+			interrupts = <GIC_SPI 184 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "tsens";
 			#thermal-sensor-cells = <1>;
 		};
 
-- 
2.17.1

[PATCH 14/15] drivers: thermal: tsens: Create function to return sign-extended temperature

[Amit Kucheria]

Hide the details of how to convert values read from TSENS HW to mCelsius
behind a function. All versions of the IP can be supported as a result.

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
 drivers/thermal/qcom/tsens-common.c | 34 ++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c
index 7ab2e740a1da..13a875b99094 100644
--- a/drivers/thermal/qcom/tsens-common.c
+++ b/drivers/thermal/qcom/tsens-common.c
@@ -84,13 +84,35 @@ static inline int code_to_degc(u32 adc_code, const struct tsens_sensor *s)
 	return degc;
 }
 
+/**
+ * tsens_hw_to_mC - Return properly sign extended temperature in mCelsius,
+ * whether in ADC code or deciCelsius depending on IP version.
+ * This function handles the different widths of the signed integer across IPs.
+ */
+static int tsens_hw_to_mC(char *str, struct tsens_sensor *s, int field, int temp)
+{
+	struct tsens_priv *priv = s->priv;
+	u32 mask;
+
+	if (priv->feat->adc) {
+		/* Convert temperature from ADC code to milliCelsius */
+		return code_to_degc(temp, s) * 1000;
+	} else {
+		mask = GENMASK(priv->fields[field].msb,
+			       priv->fields[field].lsb) >> priv->fields[field].lsb;
+		dev_dbg(priv->dev, "%s: mask: %d\n", str, fls(mask));
+		/* Convert temperature from deciCelsius to milliCelsius */
+		return sign_extend32(temp, fls(mask) - 1) * 100;
+	}
+}
+
 int get_temp_tsens_valid(struct tsens_sensor *s, int *temp)
 {
 	struct tsens_priv *priv = s->priv;
 	int hw_id = s->hw_id;
 	u32 temp_idx = LAST_TEMP_0 + hw_id;
 	u32 valid_idx = VALID_0 + hw_id;
-	u32 last_temp = 0, valid, mask;
+	u32 last_temp = 0, valid;
 	int ret;
 
 	ret = regmap_field_read(priv->rf[valid_idx], &valid);
@@ -112,15 +134,7 @@ int get_temp_tsens_valid(struct tsens_sensor *s, int *temp)
 	if (ret)
 		return ret;
 
-	if (priv->feat->adc) {
-		/* Convert temperature from ADC code to milliCelsius */
-		*temp = code_to_degc(last_temp, s) * 1000;
-	} else {
-		mask = GENMASK(priv->fields[LAST_TEMP_0].msb,
-			       priv->fields[LAST_TEMP_0].lsb);
-		/* Convert temperature from deciCelsius to milliCelsius */
-		*temp = sign_extend32(last_temp, fls(mask) - 1) * 100;
-	}
+	*temp = tsens_hw_to_mC("get_temp", s, LAST_TEMP_0, last_temp);
 
 	return 0;
 }
-- 
2.17.1

Re: [PATCH 14/15] drivers: thermal: tsens: Create function to return sign-extended temperature

[Stephen Boyd]

Quoting Amit Kucheria (2019-07-25 15:18:49)
> diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c
> index 7ab2e740a1da..13a875b99094 100644
> --- a/drivers/thermal/qcom/tsens-common.c
> +++ b/drivers/thermal/qcom/tsens-common.c
> @@ -84,13 +84,35 @@ static inline int code_to_degc(u32 adc_code, const struct tsens_sensor *s)
>         return degc;
>  }
>  
> +/**
> + * tsens_hw_to_mC - Return properly sign extended temperature in mCelsius,

Can you make this proper kernel-doc? Describe the arguments and have a
"Return:" section.

> + * whether in ADC code or deciCelsius depending on IP version.
> + * This function handles the different widths of the signed integer across IPs.
> + */
> +static int tsens_hw_to_mC(char *str, struct tsens_sensor *s, int field, int temp)
> +{
> +       struct tsens_priv *priv = s->priv;
> +       u32 mask;
> +
> +       if (priv->feat->adc) {
> +               /* Convert temperature from ADC code to milliCelsius */
> +               return code_to_degc(temp, s) * 1000;
> +       } else {

Please deindent and drop the else because there's a return above.

> +               mask = GENMASK(priv->fields[field].msb,
> +                              priv->fields[field].lsb) >> priv->fields[field].lsb;

Why is the mask generated, shifted right, sent into fls(), and then
passed to sign_extend32? Shoudln't it be something like 

	sign_extend32(temp, priv->fields[field].msg - priv->fiels[field].lsb - 1)

> +               dev_dbg(priv->dev, "%s: mask: %d\n", str, fls(mask));
> +               /* Convert temperature from deciCelsius to milliCelsius */
> +               return sign_extend32(temp, fls(mask) - 1) * 100;
> +       }
> +}
> +
> @@ -112,15 +134,7 @@ int get_temp_tsens_valid(struct tsens_sensor *s, int *temp)
>         if (ret)
>                 return ret;
>  
> -       if (priv->feat->adc) {
> -               /* Convert temperature from ADC code to milliCelsius */
> -               *temp = code_to_degc(last_temp, s) * 1000;
> -       } else {
> -               mask = GENMASK(priv->fields[LAST_TEMP_0].msb,
> -                              priv->fields[LAST_TEMP_0].lsb);
> -               /* Convert temperature from deciCelsius to milliCelsius */
> -               *temp = sign_extend32(last_temp, fls(mask) - 1) * 100;

Oh the code is copied. Seems really complicated still.

> -       }
> +       *temp = tsens_hw_to_mC("get_temp", s, LAST_TEMP_0, last_temp);

Re: [PATCH 14/15] drivers: thermal: tsens: Create function to return sign-extended temperature

[Amit Kucheria]

On Sat, Aug 17, 2019 at 9:46 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Amit Kucheria (2019-07-25 15:18:49)
> > diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c
> > index 7ab2e740a1da..13a875b99094 100644
> > --- a/drivers/thermal/qcom/tsens-common.c
> > +++ b/drivers/thermal/qcom/tsens-common.c
> > @@ -84,13 +84,35 @@ static inline int code_to_degc(u32 adc_code, const struct tsens_sensor *s)
> >         return degc;
> >  }
> >
> > +/**
> > + * tsens_hw_to_mC - Return properly sign extended temperature in mCelsius,
>
> Can you make this proper kernel-doc? Describe the arguments and have a
> "Return:" section.

Will fix.

> > + * whether in ADC code or deciCelsius depending on IP version.
> > + * This function handles the different widths of the signed integer across IPs.
> > + */
> > +static int tsens_hw_to_mC(char *str, struct tsens_sensor *s, int field, int temp)
> > +{
> > +       struct tsens_priv *priv = s->priv;
> > +       u32 mask;
> > +
> > +       if (priv->feat->adc) {
> > +               /* Convert temperature from ADC code to milliCelsius */
> > +               return code_to_degc(temp, s) * 1000;
> > +       } else {
>
> Please deindent and drop the else because there's a return above.

Will fix.


> > +               mask = GENMASK(priv->fields[field].msb,
> > +                              priv->fields[field].lsb) >> priv->fields[field].lsb;
>
> Why is the mask generated, shifted right, sent into fls(), and then
> passed to sign_extend32? Shoudln't it be something like
>
>         sign_extend32(temp, priv->fields[field].msg - priv->fiels[field].lsb - 1)

Yes, that should work and greatly simply the function. Will fix.

> > +               dev_dbg(priv->dev, "%s: mask: %d\n", str, fls(mask));
> > +               /* Convert temperature from deciCelsius to milliCelsius */
> > +               return sign_extend32(temp, fls(mask) - 1) * 100;
> > +       }
> > +}
> > +
> > @@ -112,15 +134,7 @@ int get_temp_tsens_valid(struct tsens_sensor *s, int *temp)
> >         if (ret)
> >                 return ret;
> >
> > -       if (priv->feat->adc) {
> > -               /* Convert temperature from ADC code to milliCelsius */
> > -               *temp = code_to_degc(last_temp, s) * 1000;
> > -       } else {
> > -               mask = GENMASK(priv->fields[LAST_TEMP_0].msb,
> > -                              priv->fields[LAST_TEMP_0].lsb);
> > -               /* Convert temperature from deciCelsius to milliCelsius */
> > -               *temp = sign_extend32(last_temp, fls(mask) - 1) * 100;
>
> Oh the code is copied. Seems really complicated still.
>
> > -       }
> > +       *temp = tsens_hw_to_mC("get_temp", s, LAST_TEMP_0, last_temp);

[PATCH 15/15] drivers: thermal: tsens: Add interrupt support

[Amit Kucheria]

Depending on the IP version, TSENS supports upper, lower, max, min and
critical threshold interrupts. We only add support for upper and lower
threshold interrupts for now.

TSENSv2 has an irq [status|clear|mask] bit tuple for each sensor while
earlier versions only have a single bit per sensor to denote status and
clear. At each interrupt, we reprogram the new upper and lower threshold
in the .set_trip callback.

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
 drivers/thermal/qcom/tsens-common.c | 467 ++++++++++++++++++++++++++++
 drivers/thermal/qcom/tsens-v0_1.c   |  11 +
 drivers/thermal/qcom/tsens-v1.c     |  29 ++
 drivers/thermal/qcom/tsens-v2.c     |  18 ++
 drivers/thermal/qcom/tsens.c        |  25 +-
 drivers/thermal/qcom/tsens.h        | 269 +++++++++++++++-
 6 files changed, 806 insertions(+), 13 deletions(-)

diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c
index 13a875b99094..f94ef79c37bc 100644
--- a/drivers/thermal/qcom/tsens-common.c
+++ b/drivers/thermal/qcom/tsens-common.c
@@ -13,6 +13,22 @@
 #include <linux/regmap.h>
 #include "tsens.h"
 
+/* IRQ state, mask and clear */
+struct tsens_irq_data {
+	u32 up_viol;
+	int up_thresh;
+	u32 up_irq_mask;
+	u32 up_irq_clear;
+	u32 low_viol;
+	int low_thresh;
+	u32 low_irq_mask;
+	u32 low_irq_clear;
+	u32 crit_viol;
+	u32 crit_thresh;
+	u32 crit_irq_mask;
+	u32 crit_irq_clear;
+};
+
 char *qfprom_read(struct device *dev, const char *cname)
 {
 	struct nvmem_cell *cell;
@@ -65,6 +81,18 @@ void compute_intercept_slope(struct tsens_priv *priv, u32 *p1,
 	}
 }
 
+static inline u32 degc_to_code(int degc, const struct tsens_sensor *sensor)
+{
+	u32 code = (degc * sensor->slope + sensor->offset) / SLOPE_FACTOR;
+
+	if (code > THRESHOLD_MAX_ADC_CODE)
+		code = THRESHOLD_MAX_ADC_CODE;
+	else if (code < THRESHOLD_MIN_ADC_CODE)
+		code = THRESHOLD_MIN_ADC_CODE;
+	pr_debug("%s: raw_code: 0x%x, degc:%d\n", __func__, code, degc);
+	return code;
+}
+
 static inline int code_to_degc(u32 adc_code, const struct tsens_sensor *s)
 {
 	int degc, num, den;
@@ -106,6 +134,363 @@ static int tsens_hw_to_mC(char *str, struct tsens_sensor *s, int field, int temp
 	}
 }
 
+/**
+ * tsens_mC_to_hw - Return correct value to be written to threshold
+ * registers, whether in ADC code or deciCelsius depending on IP version
+ */
+static int tsens_mC_to_hw(struct tsens_sensor *s, int temp)
+{
+	struct tsens_priv *priv = s->priv;
+
+	if (priv->feat->adc) {
+		/* milli to C to adc code */
+		return degc_to_code(temp / 1000, s);
+	} else {
+		/* milli to deci C */
+		return (temp / 100);
+	}
+}
+
+static inline unsigned int tsens_ver(struct tsens_priv *priv)
+{
+	return priv->feat->ver_major;
+}
+
+static inline u32 irq_mask(u32 hw_id)
+{
+	return 1 << hw_id;
+}
+
+/**
+ * tsens_set_interrupt_v1 - Disable an interrupt (enable = false)
+ *                          Re-enable an interrupt (enable = true)
+ */
+static void tsens_set_interrupt_v1(struct tsens_priv *priv, const struct tsens_irq_data d,
+				   u32 hw_id, enum tsens_irq_type irq_type, bool enable)
+{
+	if (enable) {
+		switch (irq_type) {
+		case UPPER:
+			regmap_field_write(priv->rf[UP_INT_CLEAR_0 + hw_id], 0);
+			break;
+		case LOWER:
+			regmap_field_write(priv->rf[LOW_INT_CLEAR_0 + hw_id], 0);
+			break;
+		default:
+			dev_err(priv->dev, "%s: Invalid irq_type\n", __func__);
+			break;
+		}
+	} else {
+		switch (irq_type) {
+		case UPPER:
+			regmap_field_write(priv->rf[UP_INT_CLEAR_0 + hw_id], 1);
+			break;
+		case LOWER:
+			regmap_field_write(priv->rf[LOW_INT_CLEAR_0 + hw_id], 1);
+			break;
+		default:
+			dev_err(priv->dev, "%s: Invalid irq_type\n", __func__);
+			break;
+		}
+	}
+}
+
+/**
+ * tsens_set_interrupt_v2 - Disable an interrupt (enable = false)
+ *                          Re-enable an interrupt (enable = true)
+ */
+static void tsens_set_interrupt_v2(struct tsens_priv *priv, const struct tsens_irq_data d,
+				   u32 hw_id, enum tsens_irq_type irq_type, bool enable)
+{
+	if (enable) {
+		switch (irq_type) {
+		case UPPER:
+			regmap_field_write(priv->rf[UP_INT_MASK_0 + hw_id], 0);
+			break;
+		case LOWER:
+			regmap_field_write(priv->rf[LOW_INT_MASK_0 + hw_id], 0);
+			break;
+		case CRITICAL:
+			regmap_field_write(priv->rf[CRIT_INT_MASK_0 + hw_id], 0);
+			break;
+		default:
+			dev_err(priv->dev, "%s: Invalid irq_type\n", __func__);
+			break;
+		}
+	} else {
+		/* To disable the interrupt flag for a sensor:
+		 *  1. Mask further interrupts for this sensor
+		 *  2. Write 1 followed by 0 to clear the interrupt
+		 */
+		switch (irq_type) {
+		case UPPER:
+			regmap_field_write(priv->rf[UP_INT_MASK_0 + hw_id], 1);
+			regmap_field_write(priv->rf[UP_INT_CLEAR_0 + hw_id], 1);
+			regmap_field_write(priv->rf[UP_INT_CLEAR_0 + hw_id], 0);
+			break;
+		case LOWER:
+			regmap_field_write(priv->rf[LOW_INT_MASK_0 + hw_id], 1);
+			regmap_field_write(priv->rf[LOW_INT_CLEAR_0 + hw_id], 1);
+			regmap_field_write(priv->rf[LOW_INT_CLEAR_0 + hw_id], 0);
+			break;
+		case CRITICAL:
+			regmap_field_write(priv->rf[CRIT_INT_MASK_0 + hw_id], 1);
+			regmap_field_write(priv->rf[CRIT_INT_CLEAR_0 + hw_id], 1);
+			regmap_field_write(priv->rf[CRIT_INT_CLEAR_0 + hw_id], 0);
+			break;
+		default:
+			dev_err(priv->dev, "%s: Invalid irq_type\n", __func__);
+			break;
+		}
+	}
+}
+
+/**
+ * tsens_set_interrupt - Disable an interrupt (enable = false)
+ *                       Re-enable an interrupt (enable = true)
+ */
+static void tsens_set_interrupt(struct tsens_priv *priv, const struct tsens_irq_data d,
+				u32 hw_id, enum tsens_irq_type irq_type, bool enable)
+{
+	/* FIXME: remove tsens_irq_data */
+	dev_dbg(priv->dev, "[%u] %s: %s -> %s\n", hw_id, __func__,
+		irq_type ? ((irq_type == 1) ? "UP" : "CRITICAL") : "LOW",
+		enable ? "en" : "dis");
+	if (tsens_ver(priv) > VER_1_X)
+		tsens_set_interrupt_v2(priv, d, hw_id, irq_type, enable);
+	else
+		tsens_set_interrupt_v1(priv, d, hw_id, irq_type, enable);
+}
+
+static int tsens_read_irq_state(struct tsens_priv *priv, u32 hw_id,
+				struct tsens_sensor *s, struct tsens_irq_data *d)
+{
+	int ret, up_temp, low_temp;
+
+	if (hw_id > priv->num_sensors) {
+		dev_err(priv->dev, "%s Invalid hw_id\n", __func__);
+		return -EINVAL;
+	}
+
+	ret = regmap_field_read(priv->rf[UPPER_STATUS_0 + hw_id], &d->up_viol);
+	if (ret)
+		return ret;
+	ret = regmap_field_read(priv->rf[UP_THRESH_0 + hw_id], &up_temp);
+	if (ret)
+		return ret;
+	ret = regmap_field_read(priv->rf[LOWER_STATUS_0 + hw_id], &d->low_viol);
+	if (ret)
+		return ret;
+	ret = regmap_field_read(priv->rf[LOW_THRESH_0 + hw_id], &low_temp);
+	if (ret)
+		return ret;
+	ret = regmap_field_read(priv->rf[UP_INT_CLEAR_0 + hw_id], &d->up_irq_clear);
+	if (ret)
+		return ret;
+	ret = regmap_field_read(priv->rf[LOW_INT_CLEAR_0 + hw_id], &d->low_irq_clear);
+	if (ret)
+		return ret;
+	if (tsens_ver(priv) > VER_1_X) {
+		ret = regmap_field_read(priv->rf[UP_INT_MASK_0 + hw_id], &d->up_irq_mask);
+		if (ret)
+			return ret;
+		ret = regmap_field_read(priv->rf[LOW_INT_MASK_0 + hw_id], &d->low_irq_mask);
+		if (ret)
+			return ret;
+	} else {
+		/* No mask register on older TSENS */
+		d->up_irq_mask = 0;
+		d->low_irq_mask = 0;
+	}
+
+	d->up_thresh = tsens_hw_to_mC("upthresh", s, UP_THRESH_0, up_temp);
+	d->low_thresh = tsens_hw_to_mC("lowthresh", s, LOW_THRESH_0, low_temp);
+
+	dev_dbg(priv->dev, "[%u] %s%s: status(%u|%u) | clr(%u|%u) | mask(%u|%u)\n",
+		hw_id, __func__, (d->up_viol || d->low_viol) ? "(V)" : "",
+		d->low_viol, d->up_viol, d->low_irq_clear, d->up_irq_clear,
+		d->low_irq_mask, d->up_irq_mask);
+	dev_dbg(priv->dev, "[%u] %s%s: thresh: (%d:%d)\n", hw_id, __func__,
+		(d->up_viol || d->low_viol) ? "(violation)" : "",
+		d->low_thresh, d->up_thresh);
+
+	return 0;
+}
+
+static inline u32 masked_irq(u32 hw_id, u32 mask, enum tsens_ver ver)
+{
+	if (ver > VER_1_X) {
+		return mask & (1 << hw_id);
+	} else {
+		/* v1, v0.1 don't have a irq mask register */
+		return 0;
+	}
+}
+
+static unsigned long tsens_filter_active_sensors(struct tsens_priv *priv)
+{
+	int i, ret, up, low;
+	unsigned long mask = 0;
+
+	for (i = 0; i < priv->num_sensors; i++) {
+		struct tsens_sensor *s = &priv->sensor[i];
+		u32 hw_id = s->hw_id;
+
+		if (IS_ERR(priv->sensor[i].tzd))
+			continue;
+		ret = regmap_field_read(priv->rf[UPPER_STATUS_0 + hw_id], &up);
+		if (ret)
+			return ret;
+		ret = regmap_field_read(priv->rf[LOWER_STATUS_0 + hw_id], &low);
+		if (ret)
+			return ret;
+		if (up || low)
+			set_bit(hw_id, &mask);
+	}
+	dev_dbg(priv->dev, "%s: hw_id mask: 0x%lx\n",  __func__, mask);
+
+	return mask;
+}
+
+irqreturn_t tsens_irq_thread(int irq, void *data)
+{
+	struct tsens_priv *priv = data;
+	int temp, ret, i;
+	unsigned long flags;
+	bool enable = true, disable = false;
+	unsigned long mask = tsens_filter_active_sensors(priv);
+
+	if (!mask) {
+		dev_err(priv->dev, "%s: Spurious interrupt?\n", __func__);
+		return IRQ_NONE;
+	}
+
+	/* Check if any sensor raised an IRQ - for each sensor connected to the
+	 * TSENS block if it set the threshold violation bit.
+	 */
+	for (i = 0; i < priv->num_sensors; i++) {
+		struct tsens_sensor *s = &priv->sensor[i];
+		struct tsens_irq_data d;
+		u32 hw_id = s->hw_id;
+		bool trigger = 0;
+
+		if (!test_bit(hw_id, &mask))
+			continue;
+		if (IS_ERR(priv->sensor[i].tzd))
+			continue;
+		ret = get_temp_tsens_valid(s, &temp);
+		if (ret) {
+			dev_err(priv->dev, "[%u] %s: error reading sensor\n", hw_id, __func__);
+			continue;
+		}
+
+		spin_lock_irqsave(&priv->ul_lock, flags);
+
+		tsens_read_irq_state(priv, hw_id, s, &d);
+
+		if (d.up_viol &&
+		    !masked_irq(hw_id, d.up_irq_mask, tsens_ver(priv))) {
+			tsens_set_interrupt(priv, d, hw_id, UPPER, disable);
+			if (d.up_thresh > temp) {
+				dev_dbg(priv->dev, "[%u] %s: re-arm upper\n",
+					priv->sensor[i].hw_id, __func__);
+				/* unmask the interrupt for this sensor */
+				tsens_set_interrupt(priv, d, hw_id, UPPER, enable);
+			} else {
+				trigger = 1;
+				/* Keep irq masked */
+			}
+		} else if (d.low_viol &&
+			   !masked_irq(hw_id, d.low_irq_mask, tsens_ver(priv))) {
+			tsens_set_interrupt(priv, d, hw_id, LOWER, disable);
+			if (d.low_thresh < temp) {
+				dev_dbg(priv->dev, "[%u] %s: re-arm low\n",
+					priv->sensor[i].hw_id, __func__);
+				/* unmask the interrupt for this sensor */
+				tsens_set_interrupt(priv, d, hw_id, LOWER, enable);
+			} else {
+				trigger = 1;
+				/* Keep irq masked */
+			}
+		}
+
+		/* TODO: (amit) REALLY??? */
+		mb();
+
+		spin_unlock_irqrestore(&priv->ul_lock, flags);
+
+		if (trigger) {
+			dev_dbg(priv->dev, "[%u] %s: TZ update trigger (%d mC)\n",
+				hw_id, __func__, temp);
+			thermal_zone_device_update(priv->sensor[i].tzd,
+						   THERMAL_EVENT_UNSPECIFIED);
+		} else {
+			dev_dbg(priv->dev, "[%u] %s: no violation:  %d\n",
+				hw_id, __func__, temp);
+		}
+	}
+
+	return IRQ_HANDLED;
+}
+
+int tsens_set_trips(void *_sensor, int low, int high)
+{
+	struct tsens_sensor *s = _sensor;
+	struct tsens_priv *priv = s->priv;
+	struct device *dev = priv->dev;
+	struct tsens_irq_data d;
+	unsigned long flags;
+	int high_val, low_val, cl_high, cl_low;
+	bool enable = true;
+	u32 hw_id = s->hw_id;
+
+	dev_dbg(dev, "[%u] %s: proposed thresholds: (%d:%d)\n",
+		hw_id, __func__, low, high);
+
+	cl_high = clamp_val(high, -40000, 120000);
+	cl_low  = clamp_val(low, -40000, 120000);
+
+	high_val = tsens_mC_to_hw(s, cl_high);
+	low_val  = tsens_mC_to_hw(s, cl_low);
+
+	spin_lock_irqsave(&priv->ul_lock, flags);
+
+	tsens_read_irq_state(priv, hw_id, s, &d);
+
+	/* Write the new thresholds and clear the status */
+	regmap_field_write(priv->rf[LOW_THRESH_0 + hw_id], low_val);
+	regmap_field_write(priv->rf[UP_THRESH_0 + hw_id], high_val);
+	tsens_set_interrupt(priv, d, hw_id, LOWER, enable);
+	tsens_set_interrupt(priv, d, hw_id, UPPER, enable);
+
+	/* TODO: (amit) REALLY??? */
+	mb();
+
+	spin_unlock_irqrestore(&priv->ul_lock, flags);
+
+	dev_dbg(dev, "[%u] %s: (%d:%d)->(%d:%d)\n",
+		s->hw_id, __func__, d.low_thresh, d.up_thresh, cl_low, cl_high);
+
+	return 0;
+}
+
+int tsens_enable_irq(struct tsens_priv *priv)
+{
+	int ret;
+	int val = (tsens_ver(priv) > VER_1_X) ? 7 : 1;
+
+	ret = regmap_field_write(priv->rf[INT_EN], val);
+	if (ret < 0)
+		dev_err(priv->dev, "%s: failed to enable interrupts\n", __func__);
+
+	return ret;
+}
+
+void tsens_disable_irq(struct tsens_priv *priv)
+{
+	regmap_field_write(priv->rf[INT_EN], 0);
+}
+
 int get_temp_tsens_valid(struct tsens_sensor *s, int *temp)
 {
 	struct tsens_priv *priv = s->priv;
@@ -334,6 +719,88 @@ int __init init_common(struct tsens_priv *priv)
 			goto err_put_device;
 		}
 	}
+	for (i = 0, j = UPPER_STATUS_0; i < priv->feat->max_sensors; i++, j++) {
+		priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map,
+						      priv->fields[j]);
+		if (IS_ERR(priv->rf[j])) {
+			ret = PTR_ERR(priv->rf[j]);
+			goto err_put_device;
+		}
+	}
+	for (i = 0, j = LOWER_STATUS_0; i < priv->feat->max_sensors; i++, j++) {
+		priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map,
+						      priv->fields[j]);
+		if (IS_ERR(priv->rf[j])) {
+			ret = PTR_ERR(priv->rf[j]);
+			goto err_put_device;
+		}
+	}
+	for (i = 0, j = CRITICAL_STATUS_0; i < priv->feat->max_sensors; i++, j++) {
+		priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map,
+						      priv->fields[j]);
+		if (IS_ERR(priv->rf[j])) {
+			ret = PTR_ERR(priv->rf[j]);
+			goto err_put_device;
+		}
+	}
+	for (i = 0, j = UP_THRESH_0; i < priv->feat->max_sensors; i++, j++) {
+		priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map,
+						      priv->fields[j]);
+		if (IS_ERR(priv->rf[j])) {
+			ret = PTR_ERR(priv->rf[j]);
+			goto err_put_device;
+		}
+	}
+	for (i = 0, j = LOW_THRESH_0; i < priv->feat->max_sensors; i++, j++) {
+		priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map,
+						      priv->fields[j]);
+		if (IS_ERR(priv->rf[j])) {
+			ret = PTR_ERR(priv->rf[j]);
+			goto err_put_device;
+		}
+	}
+	for (i = 0, j = UP_INT_CLEAR_0; i < priv->feat->max_sensors; i++, j++) {
+		priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map,
+						      priv->fields[j]);
+		if (IS_ERR(priv->rf[j])) {
+			ret = PTR_ERR(priv->rf[j]);
+			goto err_put_device;
+		}
+	}
+	for (i = 0, j = LOW_INT_CLEAR_0; i < priv->feat->max_sensors; i++, j++) {
+		priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map,
+						      priv->fields[j]);
+		if (IS_ERR(priv->rf[j])) {
+			ret = PTR_ERR(priv->rf[j]);
+			goto err_put_device;
+		}
+	}
+	for (i = 0, j = UP_INT_MASK_0; i < priv->feat->max_sensors; i++, j++) {
+		priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map,
+						      priv->fields[j]);
+		if (IS_ERR(priv->rf[j])) {
+			ret = PTR_ERR(priv->rf[j]);
+			goto err_put_device;
+		}
+	}
+	for (i = 0, j = LOW_INT_MASK_0; i < priv->feat->max_sensors; i++, j++) {
+		priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map,
+						      priv->fields[j]);
+		if (IS_ERR(priv->rf[j])) {
+			ret = PTR_ERR(priv->rf[j]);
+			goto err_put_device;
+		}
+	}
+
+	priv->rf[INT_EN] = devm_regmap_field_alloc(dev, priv->tm_map,
+						   priv->fields[INT_EN]);
+	if (IS_ERR(priv->rf[INT_EN])) {
+		ret = PTR_ERR(priv->rf[INT_EN]);
+		goto err_put_device;
+	}
+
+	tsens_enable_irq(priv);
+	spin_lock_init(&priv->ul_lock);
 
 	tsens_debug_init(op);
 
diff --git a/drivers/thermal/qcom/tsens-v0_1.c b/drivers/thermal/qcom/tsens-v0_1.c
index 6f26fadf4c27..d36e16697173 100644
--- a/drivers/thermal/qcom/tsens-v0_1.c
+++ b/drivers/thermal/qcom/tsens-v0_1.c
@@ -339,9 +339,20 @@ static const struct reg_field tsens_v0_1_regfields[MAX_REGFIELDS] = {
 	/* INTERRUPT ENABLE */
 	[INT_EN] = REG_FIELD(TM_INT_EN_OFF, 0, 0),
 
+	/* UPPER/LOWER TEMPERATURE THRESHOLDS */
+	REG_FIELD_FOR_EACH_SENSOR11(LOW_THRESH,    TM_Sn_UPPER_LOWER_STATUS_CTRL_OFF,  0,  9),
+	REG_FIELD_FOR_EACH_SENSOR11(UP_THRESH,     TM_Sn_UPPER_LOWER_STATUS_CTRL_OFF, 10, 19),
+
+	/* UPPER/LOWER INTERRUPTS [CLEAR/STATUS] */
+	REG_FIELD_FOR_EACH_SENSOR11(LOW_INT_CLEAR, TM_Sn_UPPER_LOWER_STATUS_CTRL_OFF, 20, 20),
+	REG_FIELD_FOR_EACH_SENSOR11(UP_INT_CLEAR,  TM_Sn_UPPER_LOWER_STATUS_CTRL_OFF, 21, 21),
+
+	/* NO CRITICAL INTERRUPT SUPPORT on v1 */
+
 	/* Sn_STATUS */
 	REG_FIELD_FOR_EACH_SENSOR11(LAST_TEMP,    TM_Sn_STATUS_OFF,  0,  9),
 	/* No VALID field on v0.1 */
+	/* xxx_STATUS bits: 1 == threshold violated */
 	REG_FIELD_FOR_EACH_SENSOR11(MIN_STATUS,   TM_Sn_STATUS_OFF, 10, 10),
 	REG_FIELD_FOR_EACH_SENSOR11(LOWER_STATUS, TM_Sn_STATUS_OFF, 11, 11),
 	REG_FIELD_FOR_EACH_SENSOR11(UPPER_STATUS, TM_Sn_STATUS_OFF, 12, 12),
diff --git a/drivers/thermal/qcom/tsens-v1.c b/drivers/thermal/qcom/tsens-v1.c
index 10b595d4f619..86259c9821be 100644
--- a/drivers/thermal/qcom/tsens-v1.c
+++ b/drivers/thermal/qcom/tsens-v1.c
@@ -17,6 +17,8 @@
 #define TM_Sn_UPPER_LOWER_STATUS_CTRL_OFF	0x0004
 #define TM_Sn_STATUS_OFF			0x0044
 #define TM_TRDY_OFF				0x0084
+#define TM_HIGH_LOW_INT_STATUS_OFF		0x0088
+#define TM_HIGH_LOW_Sn_INT_THRESHOLD_OFF	0x0090
 
 /* eeprom layout data for qcs404/405 (v1) */
 #define BASE0_MASK	0x000007f8
@@ -167,9 +169,36 @@ static const struct reg_field tsens_v1_regfields[MAX_REGFIELDS] = {
 	/* INTERRUPT ENABLE */
 	[INT_EN]     = REG_FIELD(TM_INT_EN_OFF, 0, 0),
 
+	/* UPPER/LOWER TEMPERATURE THRESHOLDS */
+	REG_FIELD_FOR_EACH_SENSOR11(LOW_THRESH,    TM_Sn_UPPER_LOWER_STATUS_CTRL_OFF,  0,  9),
+	REG_FIELD_FOR_EACH_SENSOR11(UP_THRESH,     TM_Sn_UPPER_LOWER_STATUS_CTRL_OFF, 10, 19),
+
+	/* UPPER/LOWER INTERRUPTS [CLEAR/STATUS] */
+	REG_FIELD_FOR_EACH_SENSOR11(LOW_INT_CLEAR, TM_Sn_UPPER_LOWER_STATUS_CTRL_OFF, 20, 20),
+	REG_FIELD_FOR_EACH_SENSOR11(UP_INT_CLEAR,  TM_Sn_UPPER_LOWER_STATUS_CTRL_OFF, 21, 21),
+	[LOW_INT_STATUS_0] = REG_FIELD(TM_HIGH_LOW_INT_STATUS_OFF,  0,  0),
+	[LOW_INT_STATUS_1] = REG_FIELD(TM_HIGH_LOW_INT_STATUS_OFF,  1,  1),
+	[LOW_INT_STATUS_2] = REG_FIELD(TM_HIGH_LOW_INT_STATUS_OFF,  2,  2),
+	[LOW_INT_STATUS_3] = REG_FIELD(TM_HIGH_LOW_INT_STATUS_OFF,  3,  3),
+	[LOW_INT_STATUS_4] = REG_FIELD(TM_HIGH_LOW_INT_STATUS_OFF,  4,  4),
+	[LOW_INT_STATUS_5] = REG_FIELD(TM_HIGH_LOW_INT_STATUS_OFF,  5,  5),
+	[LOW_INT_STATUS_6] = REG_FIELD(TM_HIGH_LOW_INT_STATUS_OFF,  6,  6),
+	[LOW_INT_STATUS_7] = REG_FIELD(TM_HIGH_LOW_INT_STATUS_OFF,  7,  7),
+	[UP_INT_STATUS_0]  = REG_FIELD(TM_HIGH_LOW_INT_STATUS_OFF,  8,  8),
+	[UP_INT_STATUS_1]  = REG_FIELD(TM_HIGH_LOW_INT_STATUS_OFF,  9,  9),
+	[UP_INT_STATUS_2]  = REG_FIELD(TM_HIGH_LOW_INT_STATUS_OFF, 10, 10),
+	[UP_INT_STATUS_3]  = REG_FIELD(TM_HIGH_LOW_INT_STATUS_OFF, 11, 11),
+	[UP_INT_STATUS_4]  = REG_FIELD(TM_HIGH_LOW_INT_STATUS_OFF, 12, 12),
+	[UP_INT_STATUS_5]  = REG_FIELD(TM_HIGH_LOW_INT_STATUS_OFF, 13, 13),
+	[UP_INT_STATUS_6]  = REG_FIELD(TM_HIGH_LOW_INT_STATUS_OFF, 14, 14),
+	[UP_INT_STATUS_7]  = REG_FIELD(TM_HIGH_LOW_INT_STATUS_OFF, 15, 15),
+
+	/* NO CRITICAL INTERRUPT SUPPORT on v1 */
+
 	/* Sn_STATUS */
 	REG_FIELD_FOR_EACH_SENSOR11(LAST_TEMP,    TM_Sn_STATUS_OFF,  0,  9),
 	REG_FIELD_FOR_EACH_SENSOR11(VALID,        TM_Sn_STATUS_OFF, 14, 14),
+	/* xxx_STATUS bits: 1 == threshold violated */
 	REG_FIELD_FOR_EACH_SENSOR11(MIN_STATUS,   TM_Sn_STATUS_OFF, 10, 10),
 	REG_FIELD_FOR_EACH_SENSOR11(LOWER_STATUS, TM_Sn_STATUS_OFF, 11, 11),
 	REG_FIELD_FOR_EACH_SENSOR11(UPPER_STATUS, TM_Sn_STATUS_OFF, 12, 12),
diff --git a/drivers/thermal/qcom/tsens-v2.c b/drivers/thermal/qcom/tsens-v2.c
index 0a4f2b8fcab6..96e29ba14eaf 100644
--- a/drivers/thermal/qcom/tsens-v2.c
+++ b/drivers/thermal/qcom/tsens-v2.c
@@ -50,9 +50,27 @@ static const struct reg_field tsens_v2_regfields[MAX_REGFIELDS] = {
 	/* v2 has separate enables for UPPER/LOWER/CRITICAL interrupts */
 	[INT_EN]  = REG_FIELD(TM_INT_EN_OFF, 0, 2),
 
+	/* UPPER/LOWER TEMPERATURE THRESHOLDS */
+	REG_FIELD_FOR_EACH_SENSOR16(LOW_THRESH, TM_Sn_UPPER_LOWER_THRESHOLD_OFF,  0,  11),
+	REG_FIELD_FOR_EACH_SENSOR16(UP_THRESH,  TM_Sn_UPPER_LOWER_THRESHOLD_OFF, 12,  23),
+
+	/* UPPER/LOWER INTERRUPTS [CLEAR/STATUS/MASK] */
+	REG_FIELD_SPLIT_BITS_0_15(LOW_INT_STATUS, TM_UPPER_LOWER_INT_STATUS_OFF),
+	REG_FIELD_SPLIT_BITS_0_15(LOW_INT_CLEAR,  TM_UPPER_LOWER_INT_CLEAR_OFF),
+	REG_FIELD_SPLIT_BITS_0_15(LOW_INT_MASK,   TM_UPPER_LOWER_INT_MASK_OFF),
+	REG_FIELD_SPLIT_BITS_16_31(UP_INT_STATUS, TM_UPPER_LOWER_INT_STATUS_OFF),
+	REG_FIELD_SPLIT_BITS_16_31(UP_INT_CLEAR,  TM_UPPER_LOWER_INT_CLEAR_OFF),
+	REG_FIELD_SPLIT_BITS_16_31(UP_INT_MASK,   TM_UPPER_LOWER_INT_MASK_OFF),
+
+	/* CRITICAL_INTERRUPT */
+	REG_FIELD_SPLIT_BITS_0_15(CRIT_INT_STATUS, TM_CRITICAL_INT_STATUS_OFF),
+	REG_FIELD_SPLIT_BITS_0_15(CRIT_INT_CLEAR,  TM_CRITICAL_INT_CLEAR_OFF),
+	REG_FIELD_SPLIT_BITS_0_15(CRIT_INT_MASK,   TM_CRITICAL_INT_MASK_OFF),
+
 	/* Sn_STATUS */
 	REG_FIELD_FOR_EACH_SENSOR16(LAST_TEMP,       TM_Sn_STATUS_OFF,  0,  11),
 	REG_FIELD_FOR_EACH_SENSOR16(VALID,           TM_Sn_STATUS_OFF, 21,  21),
+	/* xxx_STATUS bits: 1 == threshold violated */
 	REG_FIELD_FOR_EACH_SENSOR16(MIN_STATUS,      TM_Sn_STATUS_OFF, 16,  16),
 	REG_FIELD_FOR_EACH_SENSOR16(LOWER_STATUS,    TM_Sn_STATUS_OFF, 17,  17),
 	REG_FIELD_FOR_EACH_SENSOR16(UPPER_STATUS,    TM_Sn_STATUS_OFF, 18,  18),
diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
index 772aa76b50e1..bc83e40ac0cf 100644
--- a/drivers/thermal/qcom/tsens.c
+++ b/drivers/thermal/qcom/tsens.c
@@ -7,6 +7,7 @@
 #include <linux/err.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/pm.h>
 #include <linux/slab.h>
@@ -78,12 +79,15 @@ MODULE_DEVICE_TABLE(of, tsens_table);
 static const struct thermal_zone_of_device_ops tsens_of_ops = {
 	.get_temp = tsens_get_temp,
 	.get_trend = tsens_get_trend,
+	.set_trips = tsens_set_trips,
 };
 
 static int tsens_register(struct tsens_priv *priv)
 {
-	int i;
+	int i, ret, irq;
 	struct thermal_zone_device *tzd;
+	struct resource *res;
+	struct platform_device *op = of_find_device_by_node(priv->dev->of_node);
 
 	for (i = 0;  i < priv->num_sensors; i++) {
 		priv->sensor[i].priv = priv;
@@ -96,7 +100,25 @@ static int tsens_register(struct tsens_priv *priv)
 		if (priv->ops->enable)
 			priv->ops->enable(priv, i);
 	}
+
+	res = platform_get_resource(op, IORESOURCE_IRQ, 0);
+	if (res) {
+		irq = res->start;
+		ret = devm_request_threaded_irq(&op->dev, irq,
+						NULL, tsens_irq_thread,
+						IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
+						res->name, priv);
+		if (ret) {
+			dev_err(&op->dev, "%s: failed to get irq\n", __func__);
+			goto err_put_device;
+		}
+		enable_irq_wake(irq);
+	}
 	return 0;
+
+err_put_device:
+	put_device(&op->dev);
+	return ret;
 }
 
 static int tsens_probe(struct platform_device *pdev)
@@ -178,6 +200,7 @@ static int tsens_remove(struct platform_device *pdev)
 	struct tsens_priv *priv = platform_get_drvdata(pdev);
 
 	debugfs_remove_recursive(priv->debug_root);
+	tsens_disable_irq(priv);
 	if (priv->ops->disable)
 		priv->ops->disable(priv);
 
diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
index e1d6af71b2b9..aab47337b797 100644
--- a/drivers/thermal/qcom/tsens.h
+++ b/drivers/thermal/qcom/tsens.h
@@ -13,8 +13,10 @@
 #define CAL_DEGC_PT2		120
 #define SLOPE_FACTOR		1000
 #define SLOPE_DEFAULT		3200
+#define THRESHOLD_MAX_ADC_CODE	0x3ff
+#define THRESHOLD_MIN_ADC_CODE	0x0
 
-
+#include <linux/interrupt.h>
 #include <linux/thermal.h>
 #include <linux/regmap.h>
 
@@ -26,6 +28,12 @@ enum tsens_ver {
 	VER_2_X,
 };
 
+enum tsens_irq_type {
+	LOWER,
+	UPPER,
+	CRITICAL,
+};
+
 /**
  * struct tsens_sensor - data for each sensor connected to the tsens device
  * @priv: tsens device instance that this sensor is connected to
@@ -99,22 +107,62 @@ struct tsens_ops {
 	[_name##_##14] = REG_FIELD(_offset + 56, _startbit, _stopbit), \
 	[_name##_##15] = REG_FIELD(_offset + 60, _startbit, _stopbit)
 
+#define REG_FIELD_SPLIT_BITS_0_15(_name, _offset)		\
+	[_name##_##0]  = REG_FIELD(_offset,  0,  0),		\
+	[_name##_##1]  = REG_FIELD(_offset,  1,  1),	\
+	[_name##_##2]  = REG_FIELD(_offset,  2,  2),	\
+	[_name##_##3]  = REG_FIELD(_offset,  3,  3),	\
+	[_name##_##4]  = REG_FIELD(_offset,  4,  4),	\
+	[_name##_##5]  = REG_FIELD(_offset,  5,  5),	\
+	[_name##_##6]  = REG_FIELD(_offset,  6,  6),	\
+	[_name##_##7]  = REG_FIELD(_offset,  7,  7),	\
+	[_name##_##8]  = REG_FIELD(_offset,  8,  8),	\
+	[_name##_##9]  = REG_FIELD(_offset,  9,  9),	\
+	[_name##_##10] = REG_FIELD(_offset, 10, 10),	\
+	[_name##_##11] = REG_FIELD(_offset, 11, 11),	\
+	[_name##_##12] = REG_FIELD(_offset, 12, 12),	\
+	[_name##_##13] = REG_FIELD(_offset, 13, 13),	\
+	[_name##_##14] = REG_FIELD(_offset, 14, 14),	\
+	[_name##_##15] = REG_FIELD(_offset, 15, 15)
+
+#define REG_FIELD_SPLIT_BITS_16_31(_name, _offset)		\
+	[_name##_##0]  = REG_FIELD(_offset, 16, 16),		\
+	[_name##_##1]  = REG_FIELD(_offset, 17, 17),	\
+	[_name##_##2]  = REG_FIELD(_offset, 18, 18),	\
+	[_name##_##3]  = REG_FIELD(_offset, 19, 19),	\
+	[_name##_##4]  = REG_FIELD(_offset, 20, 20),	\
+	[_name##_##5]  = REG_FIELD(_offset, 21, 21),	\
+	[_name##_##6]  = REG_FIELD(_offset, 22, 22),	\
+	[_name##_##7]  = REG_FIELD(_offset, 23, 23),	\
+	[_name##_##8]  = REG_FIELD(_offset, 24, 24),	\
+	[_name##_##9]  = REG_FIELD(_offset, 25, 25),	\
+	[_name##_##10] = REG_FIELD(_offset, 26, 26),	\
+	[_name##_##11] = REG_FIELD(_offset, 27, 27),	\
+	[_name##_##12] = REG_FIELD(_offset, 28, 28),	\
+	[_name##_##13] = REG_FIELD(_offset, 29, 29),	\
+	[_name##_##14] = REG_FIELD(_offset, 30, 30),	\
+	[_name##_##15] = REG_FIELD(_offset, 31, 31)
+
 /* reg_field IDs to use as an index into an array */
 enum regfield_ids {
 	/* ----- SROT ------ */
 	/* HW_VER */
-	VER_MAJOR = 0,
+	VER_MAJOR,
 	VER_MINOR,
 	VER_STEP,
 	/* CTRL_OFFSET */
-	TSENS_EN =  3,
+	TSENS_EN,
 	TSENS_SW_RST,
 	SENSOR_EN,
 	CODE_OR_TEMP,
 
 	/* ----- TM ------ */
+	/* TRDY */
+	TRDY,
+	/* INTERRUPT ENABLE */
+	INT_EN,	/* v2+ has separate enables for crit, upper and lower irq */
 	/* STATUS */
-	LAST_TEMP_0 = 7,	/* Last temperature reading */
+	LAST_TEMP_0,	/* Last temperature reading */
 	LAST_TEMP_1,
 	LAST_TEMP_2,
 	LAST_TEMP_3,
@@ -130,7 +178,7 @@ enum regfield_ids {
 	LAST_TEMP_13,
 	LAST_TEMP_14,
 	LAST_TEMP_15,
-	VALID_0 = 23,		/* VALID reading or not */
+	VALID_0,		/* VALID reading or not */
 	VALID_1,
 	VALID_2,
 	VALID_3,
@@ -226,13 +274,202 @@ enum regfield_ids {
 	CRITICAL_STATUS_13,
 	CRITICAL_STATUS_14,
 	CRITICAL_STATUS_15,
-	/* TRDY */
-	TRDY,
-	/* INTERRUPT ENABLE */
-	INT_EN,	/* Pre-V1, V1.x */
-	LOW_INT_EN,	/* V2.x */
-	UP_INT_EN,	/* V2.x */
-	CRIT_INT_EN,	/* V2.x */
+	/* INTERRUPT_STATUS */
+	LOW_INT_STATUS_0,
+	LOW_INT_STATUS_1,
+	LOW_INT_STATUS_2,
+	LOW_INT_STATUS_3,
+	LOW_INT_STATUS_4,
+	LOW_INT_STATUS_5,
+	LOW_INT_STATUS_6,
+	LOW_INT_STATUS_7,
+	LOW_INT_STATUS_8,
+	LOW_INT_STATUS_9,
+	LOW_INT_STATUS_10,
+	LOW_INT_STATUS_11,
+	LOW_INT_STATUS_12,
+	LOW_INT_STATUS_13,
+	LOW_INT_STATUS_14,
+	LOW_INT_STATUS_15,
+	UP_INT_STATUS_0,
+	UP_INT_STATUS_1,
+	UP_INT_STATUS_2,
+	UP_INT_STATUS_3,
+	UP_INT_STATUS_4,
+	UP_INT_STATUS_5,
+	UP_INT_STATUS_6,
+	UP_INT_STATUS_7,
+	UP_INT_STATUS_8,
+	UP_INT_STATUS_9,
+	UP_INT_STATUS_10,
+	UP_INT_STATUS_11,
+	UP_INT_STATUS_12,
+	UP_INT_STATUS_13,
+	UP_INT_STATUS_14,
+	UP_INT_STATUS_15,
+	CRIT_INT_STATUS_0,
+	CRIT_INT_STATUS_1,
+	CRIT_INT_STATUS_2,
+	CRIT_INT_STATUS_3,
+	CRIT_INT_STATUS_4,
+	CRIT_INT_STATUS_5,
+	CRIT_INT_STATUS_6,
+	CRIT_INT_STATUS_7,
+	CRIT_INT_STATUS_8,
+	CRIT_INT_STATUS_9,
+	CRIT_INT_STATUS_10,
+	CRIT_INT_STATUS_11,
+	CRIT_INT_STATUS_12,
+	CRIT_INT_STATUS_13,
+	CRIT_INT_STATUS_14,
+	CRIT_INT_STATUS_15,
+	/* INTERRUPT_CLEAR */
+	LOW_INT_CLEAR_0,
+	LOW_INT_CLEAR_1,
+	LOW_INT_CLEAR_2,
+	LOW_INT_CLEAR_3,
+	LOW_INT_CLEAR_4,
+	LOW_INT_CLEAR_5,
+	LOW_INT_CLEAR_6,
+	LOW_INT_CLEAR_7,
+	LOW_INT_CLEAR_8,
+	LOW_INT_CLEAR_9,
+	LOW_INT_CLEAR_10,
+	LOW_INT_CLEAR_11,
+	LOW_INT_CLEAR_12,
+	LOW_INT_CLEAR_13,
+	LOW_INT_CLEAR_14,
+	LOW_INT_CLEAR_15,
+	UP_INT_CLEAR_0,
+	UP_INT_CLEAR_1,
+	UP_INT_CLEAR_2,
+	UP_INT_CLEAR_3,
+	UP_INT_CLEAR_4,
+	UP_INT_CLEAR_5,
+	UP_INT_CLEAR_6,
+	UP_INT_CLEAR_7,
+	UP_INT_CLEAR_8,
+	UP_INT_CLEAR_9,
+	UP_INT_CLEAR_10,
+	UP_INT_CLEAR_11,
+	UP_INT_CLEAR_12,
+	UP_INT_CLEAR_13,
+	UP_INT_CLEAR_14,
+	UP_INT_CLEAR_15,
+	CRIT_INT_CLEAR_0,
+	CRIT_INT_CLEAR_1,
+	CRIT_INT_CLEAR_2,
+	CRIT_INT_CLEAR_3,
+	CRIT_INT_CLEAR_4,
+	CRIT_INT_CLEAR_5,
+	CRIT_INT_CLEAR_6,
+	CRIT_INT_CLEAR_7,
+	CRIT_INT_CLEAR_8,
+	CRIT_INT_CLEAR_9,
+	CRIT_INT_CLEAR_10,
+	CRIT_INT_CLEAR_11,
+	CRIT_INT_CLEAR_12,
+	CRIT_INT_CLEAR_13,
+	CRIT_INT_CLEAR_14,
+	CRIT_INT_CLEAR_15,
+	/* INTERRUPT_MASK */
+	LOW_INT_MASK_0,
+	LOW_INT_MASK_1,
+	LOW_INT_MASK_2,
+	LOW_INT_MASK_3,
+	LOW_INT_MASK_4,
+	LOW_INT_MASK_5,
+	LOW_INT_MASK_6,
+	LOW_INT_MASK_7,
+	LOW_INT_MASK_8,
+	LOW_INT_MASK_9,
+	LOW_INT_MASK_10,
+	LOW_INT_MASK_11,
+	LOW_INT_MASK_12,
+	LOW_INT_MASK_13,
+	LOW_INT_MASK_14,
+	LOW_INT_MASK_15,
+	UP_INT_MASK_0,
+	UP_INT_MASK_1,
+	UP_INT_MASK_2,
+	UP_INT_MASK_3,
+	UP_INT_MASK_4,
+	UP_INT_MASK_5,
+	UP_INT_MASK_6,
+	UP_INT_MASK_7,
+	UP_INT_MASK_8,
+	UP_INT_MASK_9,
+	UP_INT_MASK_10,
+	UP_INT_MASK_11,
+	UP_INT_MASK_12,
+	UP_INT_MASK_13,
+	UP_INT_MASK_14,
+	UP_INT_MASK_15,
+	CRIT_INT_MASK_0,
+	CRIT_INT_MASK_1,
+	CRIT_INT_MASK_2,
+	CRIT_INT_MASK_3,
+	CRIT_INT_MASK_4,
+	CRIT_INT_MASK_5,
+	CRIT_INT_MASK_6,
+	CRIT_INT_MASK_7,
+	CRIT_INT_MASK_8,
+	CRIT_INT_MASK_9,
+	CRIT_INT_MASK_10,
+	CRIT_INT_MASK_11,
+	CRIT_INT_MASK_12,
+	CRIT_INT_MASK_13,
+	CRIT_INT_MASK_14,
+	CRIT_INT_MASK_15,
+	/* THRESHOLD */
+	LOW_THRESH_0,
+	LOW_THRESH_1,
+	LOW_THRESH_2,
+	LOW_THRESH_3,
+	LOW_THRESH_4,
+	LOW_THRESH_5,
+	LOW_THRESH_6,
+	LOW_THRESH_7,
+	LOW_THRESH_8,
+	LOW_THRESH_9,
+	LOW_THRESH_10,
+	LOW_THRESH_11,
+	LOW_THRESH_12,
+	LOW_THRESH_13,
+	LOW_THRESH_14,
+	LOW_THRESH_15,
+	UP_THRESH_0,
+	UP_THRESH_1,
+	UP_THRESH_2,
+	UP_THRESH_3,
+	UP_THRESH_4,
+	UP_THRESH_5,
+	UP_THRESH_6,
+	UP_THRESH_7,
+	UP_THRESH_8,
+	UP_THRESH_9,
+	UP_THRESH_10,
+	UP_THRESH_11,
+	UP_THRESH_12,
+	UP_THRESH_13,
+	UP_THRESH_14,
+	UP_THRESH_15,
+	CRIT_THRESH_0,
+	CRIT_THRESH_1,
+	CRIT_THRESH_2,
+	CRIT_THRESH_3,
+	CRIT_THRESH_4,
+	CRIT_THRESH_5,
+	CRIT_THRESH_6,
+	CRIT_THRESH_7,
+	CRIT_THRESH_8,
+	CRIT_THRESH_9,
+	CRIT_THRESH_10,
+	CRIT_THRESH_11,
+	CRIT_THRESH_12,
+	CRIT_THRESH_13,
+	CRIT_THRESH_14,
+	CRIT_THRESH_15,
 
 	/* Keep last */
 	MAX_REGFIELDS
@@ -302,6 +539,10 @@ struct tsens_priv {
 	struct regmap			*tm_map;
 	struct regmap			*srot_map;
 	u32				tm_offset;
+
+	/* lock for upper/lower threshold interrupts */
+	spinlock_t			ul_lock;
+
 	struct regmap_field		*rf[MAX_REGFIELDS];
 	struct tsens_context		ctx;
 	const struct tsens_features	*feat;
@@ -319,6 +560,10 @@ void compute_intercept_slope(struct tsens_priv *priv, u32 *pt1, u32 *pt2, u32 mo
 int init_common(struct tsens_priv *priv);
 int get_temp_tsens_valid(struct tsens_sensor *s, int *temp);
 int get_temp_common(struct tsens_sensor *s, int *temp);
+int tsens_enable_irq(struct tsens_priv *priv);
+void tsens_disable_irq(struct tsens_priv *priv);
+int tsens_set_trips(void *_sensor, int low, int high);
+irqreturn_t tsens_irq_thread(int irq, void *data);
 
 /* TSENS target */
 extern const struct tsens_plat_data data_8960;
-- 
2.17.1

Re: [PATCH 15/15] drivers: thermal: tsens: Add interrupt support

[Stephen Boyd]

Quoting Amit Kucheria (2019-07-25 15:18:50)
> diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c
> index 13a875b99094..f94ef79c37bc 100644
> --- a/drivers/thermal/qcom/tsens-common.c
> +++ b/drivers/thermal/qcom/tsens-common.c
> @@ -13,6 +13,22 @@
>  #include <linux/regmap.h>
>  #include "tsens.h"
>  
> +/* IRQ state, mask and clear */
> +struct tsens_irq_data {
> +       u32 up_viol;

Is viol a violation? Maybe some kernel doc would be useful here.

> +       int up_thresh;
> +       u32 up_irq_mask;
> +       u32 up_irq_clear;
> +       u32 low_viol;
> +       int low_thresh;
> +       u32 low_irq_mask;
> +       u32 low_irq_clear;
> +       u32 crit_viol;
> +       u32 crit_thresh;
> +       u32 crit_irq_mask;
> +       u32 crit_irq_clear;
> +};
> +
>  char *qfprom_read(struct device *dev, const char *cname)
>  {
>         struct nvmem_cell *cell;
> @@ -65,6 +81,18 @@ void compute_intercept_slope(struct tsens_priv *priv, u32 *p1,
>         }
>  }
>  
> +static inline u32 degc_to_code(int degc, const struct tsens_sensor *sensor)
> +{
> +       u32 code = (degc * sensor->slope + sensor->offset) / SLOPE_FACTOR;

This can't overflow 32-bits with the multiply and add?

> +
> +       if (code > THRESHOLD_MAX_ADC_CODE)
> +               code = THRESHOLD_MAX_ADC_CODE;
> +       else if (code < THRESHOLD_MIN_ADC_CODE)
> +               code = THRESHOLD_MIN_ADC_CODE;

Looks like

	return clamp(code, THRESHOLD_MIN_ADC_CODE, THRESHOLD_MAX_ADC_CODE);

> +       pr_debug("%s: raw_code: 0x%x, degc:%d\n", __func__, code, degc);
> +       return code;
> +}
> +
>  static inline int code_to_degc(u32 adc_code, const struct tsens_sensor *s)
>  {
>         int degc, num, den;
> @@ -106,6 +134,363 @@ static int tsens_hw_to_mC(char *str, struct tsens_sensor *s, int field, int temp
>         }
>  }
>  
> +/**
> + * tsens_mC_to_hw - Return correct value to be written to threshold
> + * registers, whether in ADC code or deciCelsius depending on IP version
> + */
> +static int tsens_mC_to_hw(struct tsens_sensor *s, int temp)
> +{
> +       struct tsens_priv *priv = s->priv;
> +
> +       if (priv->feat->adc) {
> +               /* milli to C to adc code */
> +               return degc_to_code(temp / 1000, s);
> +       } else {
> +               /* milli to deci C */
> +               return (temp / 100);
> +       }

Drop the else and just return without parenthesis.

> +}
> +
> +static inline unsigned int tsens_ver(struct tsens_priv *priv)
> +{
> +       return priv->feat->ver_major;
> +}
> +
> +static inline u32 irq_mask(u32 hw_id)

Is this used?

> +{
> +       return 1 << hw_id;
> +}
> +
> +/**
> + * tsens_set_interrupt_v1 - Disable an interrupt (enable = false)
> + *                          Re-enable an interrupt (enable = true)
> + */
> +static void tsens_set_interrupt_v1(struct tsens_priv *priv, const struct tsens_irq_data d,
> +                                  u32 hw_id, enum tsens_irq_type irq_type, bool enable)
> +{
> +       if (enable) {
> +               switch (irq_type) {
> +               case UPPER:
> +                       regmap_field_write(priv->rf[UP_INT_CLEAR_0 + hw_id], 0);
> +                       break;
> +               case LOWER:
> +                       regmap_field_write(priv->rf[LOW_INT_CLEAR_0 + hw_id], 0);
> +                       break;
> +               default:
> +                       dev_err(priv->dev, "%s: Invalid irq_type\n", __func__);
> +                       break;
> +               }
> +       } else {
> +               switch (irq_type) {
> +               case UPPER:
> +                       regmap_field_write(priv->rf[UP_INT_CLEAR_0 + hw_id], 1);
> +                       break;
> +               case LOWER:
> +                       regmap_field_write(priv->rf[LOW_INT_CLEAR_0 + hw_id], 1);
> +                       break;
> +               default:
> +                       dev_err(priv->dev, "%s: Invalid irq_type\n", __func__);
> +                       break;
> +               }
> +       }
> +}
> +
> +/**
> + * tsens_set_interrupt_v2 - Disable an interrupt (enable = false)
> + *                          Re-enable an interrupt (enable = true)
> + */
> +static void tsens_set_interrupt_v2(struct tsens_priv *priv, const struct tsens_irq_data d,
> +                                  u32 hw_id, enum tsens_irq_type irq_type, bool enable)
> +{
> +       if (enable) {
> +               switch (irq_type) {
> +               case UPPER:
> +                       regmap_field_write(priv->rf[UP_INT_MASK_0 + hw_id], 0);

Maybe just have a variable like mask_reg that is equal to UP_INT_MASK,
etc? And then one regmap_field_write() call outside the switch that does
the write to 0?

> +                       break;
> +               case LOWER:
> +                       regmap_field_write(priv->rf[LOW_INT_MASK_0 + hw_id], 0);
> +                       break;
> +               case CRITICAL:
> +                       regmap_field_write(priv->rf[CRIT_INT_MASK_0 + hw_id], 0);
> +                       break;
> +               default:
> +                       dev_err(priv->dev, "%s: Invalid irq_type\n", __func__);
> +                       break;
> +               }
> +       } else {
> +               /* To disable the interrupt flag for a sensor:
> +                *  1. Mask further interrupts for this sensor
> +                *  2. Write 1 followed by 0 to clear the interrupt
> +                */
> +               switch (irq_type) {
> +               case UPPER:
> +                       regmap_field_write(priv->rf[UP_INT_MASK_0 + hw_id], 1);
> +                       regmap_field_write(priv->rf[UP_INT_CLEAR_0 + hw_id], 1);
> +                       regmap_field_write(priv->rf[UP_INT_CLEAR_0 + hw_id], 0);

Same comment here. Use a local variable for mask and clear and then
write the register with three regmap_field_write() calls?

> +                       break;
> +               case LOWER:
> +                       regmap_field_write(priv->rf[LOW_INT_MASK_0 + hw_id], 1);
> +                       regmap_field_write(priv->rf[LOW_INT_CLEAR_0 + hw_id], 1);
> +                       regmap_field_write(priv->rf[LOW_INT_CLEAR_0 + hw_id], 0);
> +                       break;
> +               case CRITICAL:
> +                       regmap_field_write(priv->rf[CRIT_INT_MASK_0 + hw_id], 1);
> +                       regmap_field_write(priv->rf[CRIT_INT_CLEAR_0 + hw_id], 1);
> +                       regmap_field_write(priv->rf[CRIT_INT_CLEAR_0 + hw_id], 0);
> +                       break;
> +               default:

I'm not sure we actually need this. Modern compilers check for not
catching an enum value in a switch case so this shouldn't even really
matter.

> +                       dev_err(priv->dev, "%s: Invalid irq_type\n", __func__);
> +                       break;
> +               }
> +       }
> +}
> +
> +/**
> + * tsens_set_interrupt - Disable an interrupt (enable = false)
> + *                       Re-enable an interrupt (enable = true)
> + */
> +static void tsens_set_interrupt(struct tsens_priv *priv, const struct tsens_irq_data d,

Why not pass a pointer to tsens_irq_data?

> +                               u32 hw_id, enum tsens_irq_type irq_type, bool enable)
> +{
> +       /* FIXME: remove tsens_irq_data */
> +       dev_dbg(priv->dev, "[%u] %s: %s -> %s\n", hw_id, __func__,
> +               irq_type ? ((irq_type == 1) ? "UP" : "CRITICAL") : "LOW",
> +               enable ? "en" : "dis");
> +       if (tsens_ver(priv) > VER_1_X)
> +               tsens_set_interrupt_v2(priv, d, hw_id, irq_type, enable);
> +       else
> +               tsens_set_interrupt_v1(priv, d, hw_id, irq_type, enable);
> +}
> +
> +static int tsens_read_irq_state(struct tsens_priv *priv, u32 hw_id,
> +                               struct tsens_sensor *s, struct tsens_irq_data *d)
> +{
> +       int ret, up_temp, low_temp;
> +
> +       if (hw_id > priv->num_sensors) {
> +               dev_err(priv->dev, "%s Invalid hw_id\n", __func__);
> +               return -EINVAL;
> +       }
> +
> +       ret = regmap_field_read(priv->rf[UPPER_STATUS_0 + hw_id], &d->up_viol);
> +       if (ret)
> +               return ret;
> +       ret = regmap_field_read(priv->rf[UP_THRESH_0 + hw_id], &up_temp);
> +       if (ret)
> +               return ret;
> +       ret = regmap_field_read(priv->rf[LOWER_STATUS_0 + hw_id], &d->low_viol);
> +       if (ret)
> +               return ret;
> +       ret = regmap_field_read(priv->rf[LOW_THRESH_0 + hw_id], &low_temp);
> +       if (ret)
> +               return ret;
> +       ret = regmap_field_read(priv->rf[UP_INT_CLEAR_0 + hw_id], &d->up_irq_clear);
> +       if (ret)
> +               return ret;
> +       ret = regmap_field_read(priv->rf[LOW_INT_CLEAR_0 + hw_id], &d->low_irq_clear);
> +       if (ret)
> +               return ret;
> +       if (tsens_ver(priv) > VER_1_X) {
> +               ret = regmap_field_read(priv->rf[UP_INT_MASK_0 + hw_id], &d->up_irq_mask);
> +               if (ret)
> +                       return ret;
> +               ret = regmap_field_read(priv->rf[LOW_INT_MASK_0 + hw_id], &d->low_irq_mask);
> +               if (ret)
> +                       return ret;
> +       } else {
> +               /* No mask register on older TSENS */
> +               d->up_irq_mask = 0;
> +               d->low_irq_mask = 0;
> +       }
> +
> +       d->up_thresh = tsens_hw_to_mC("upthresh", s, UP_THRESH_0, up_temp);
> +       d->low_thresh = tsens_hw_to_mC("lowthresh", s, LOW_THRESH_0, low_temp);
> +
> +       dev_dbg(priv->dev, "[%u] %s%s: status(%u|%u) | clr(%u|%u) | mask(%u|%u)\n",
> +               hw_id, __func__, (d->up_viol || d->low_viol) ? "(V)" : "",
> +               d->low_viol, d->up_viol, d->low_irq_clear, d->up_irq_clear,
> +               d->low_irq_mask, d->up_irq_mask);
> +       dev_dbg(priv->dev, "[%u] %s%s: thresh: (%d:%d)\n", hw_id, __func__,
> +               (d->up_viol || d->low_viol) ? "(violation)" : "",
> +               d->low_thresh, d->up_thresh);
> +
> +       return 0;
> +}
> +
> +static inline u32 masked_irq(u32 hw_id, u32 mask, enum tsens_ver ver)
> +{
> +       if (ver > VER_1_X) {
> +               return mask & (1 << hw_id);
> +       } else {
> +               /* v1, v0.1 don't have a irq mask register */
> +               return 0;
> +       }

Same return comment.

> +}
> +
> +static unsigned long tsens_filter_active_sensors(struct tsens_priv *priv)
> +{
> +       int i, ret, up, low;
> +       unsigned long mask = 0;
> +
> +       for (i = 0; i < priv->num_sensors; i++) {
> +               struct tsens_sensor *s = &priv->sensor[i];
> +               u32 hw_id = s->hw_id;
> +
> +               if (IS_ERR(priv->sensor[i].tzd))
> +                       continue;
> +               ret = regmap_field_read(priv->rf[UPPER_STATUS_0 + hw_id], &up);
> +               if (ret)
> +                       return ret;

Probably don't want to return ret here given that this returns a mask.
Maybe push this into the callsite loop and then break out or return an
error from there instead. Making a mask via a loop and then using that
again the second time to test the bit is more complicated.

> +               ret = regmap_field_read(priv->rf[LOWER_STATUS_0 + hw_id], &low);
> +               if (ret)
> +                       return ret;
> +               if (up || low)
> +                       set_bit(hw_id, &mask);
> +       }
> +       dev_dbg(priv->dev, "%s: hw_id mask: 0x%lx\n",  __func__, mask);
> +
> +       return mask;
> +}
> +
> +irqreturn_t tsens_irq_thread(int irq, void *data)
> +{
> +       struct tsens_priv *priv = data;
> +       int temp, ret, i;
> +       unsigned long flags;
> +       bool enable = true, disable = false;
> +       unsigned long mask = tsens_filter_active_sensors(priv);
> +
> +       if (!mask) {
> +               dev_err(priv->dev, "%s: Spurious interrupt?\n", __func__);

Do we need the spurious irq print? Doesn't genirq already tell us about
spurious interrupts and shuts them down?

> +               return IRQ_NONE;
> +       }
> +
> +       /* Check if any sensor raised an IRQ - for each sensor connected to the

/*
 * Please make multiline comments
 * like this
 */

> +        * TSENS block if it set the threshold violation bit.
> +        */
> +       for (i = 0; i < priv->num_sensors; i++) {
> +               struct tsens_sensor *s = &priv->sensor[i];
> +               struct tsens_irq_data d;
> +               u32 hw_id = s->hw_id;
> +               bool trigger = 0;
> +
> +               if (!test_bit(hw_id, &mask))
> +                       continue;
> +               if (IS_ERR(priv->sensor[i].tzd))
> +                       continue;
> +               ret = get_temp_tsens_valid(s, &temp);
> +               if (ret) {
> +                       dev_err(priv->dev, "[%u] %s: error reading sensor\n", hw_id, __func__);
> +                       continue;
> +               }
> +
> +               spin_lock_irqsave(&priv->ul_lock, flags);
> +
> +               tsens_read_irq_state(priv, hw_id, s, &d);
> +
> +               if (d.up_viol &&
> +                   !masked_irq(hw_id, d.up_irq_mask, tsens_ver(priv))) {
> +                       tsens_set_interrupt(priv, d, hw_id, UPPER, disable);
> +                       if (d.up_thresh > temp) {
> +                               dev_dbg(priv->dev, "[%u] %s: re-arm upper\n",
> +                                       priv->sensor[i].hw_id, __func__);
> +                               /* unmask the interrupt for this sensor */
> +                               tsens_set_interrupt(priv, d, hw_id, UPPER, enable);
> +                       } else {
> +                               trigger = 1;
> +                               /* Keep irq masked */
> +                       }
> +               } else if (d.low_viol &&
> +                          !masked_irq(hw_id, d.low_irq_mask, tsens_ver(priv))) {
> +                       tsens_set_interrupt(priv, d, hw_id, LOWER, disable);
> +                       if (d.low_thresh < temp) {
> +                               dev_dbg(priv->dev, "[%u] %s: re-arm low\n",
> +                                       priv->sensor[i].hw_id, __func__);
> +                               /* unmask the interrupt for this sensor */
> +                               tsens_set_interrupt(priv, d, hw_id, LOWER, enable);
> +                       } else {
> +                               trigger = 1;
> +                               /* Keep irq masked */
> +                       }
> +               }
> +
> +               /* TODO: (amit) REALLY??? */

Remove it, and the mb?

> +               mb();
> +
> +               spin_unlock_irqrestore(&priv->ul_lock, flags);
> +
> +               if (trigger) {
> +                       dev_dbg(priv->dev, "[%u] %s: TZ update trigger (%d mC)\n",
> +                               hw_id, __func__, temp);
> +                       thermal_zone_device_update(priv->sensor[i].tzd,
> +                                                  THERMAL_EVENT_UNSPECIFIED);
> +               } else {
> +                       dev_dbg(priv->dev, "[%u] %s: no violation:  %d\n",
> +                               hw_id, __func__, temp);
> +               }
> +       }
> +
> +       return IRQ_HANDLED;
> +}
> +
> +int tsens_set_trips(void *_sensor, int low, int high)
> +{
> +       struct tsens_sensor *s = _sensor;
> +       struct tsens_priv *priv = s->priv;
> +       struct device *dev = priv->dev;
> +       struct tsens_irq_data d;
> +       unsigned long flags;
> +       int high_val, low_val, cl_high, cl_low;
> +       bool enable = true;
> +       u32 hw_id = s->hw_id;
> +
> +       dev_dbg(dev, "[%u] %s: proposed thresholds: (%d:%d)\n",
> +               hw_id, __func__, low, high);
> +
> +       cl_high = clamp_val(high, -40000, 120000);
> +       cl_low  = clamp_val(low, -40000, 120000);
> +
> +       high_val = tsens_mC_to_hw(s, cl_high);
> +       low_val  = tsens_mC_to_hw(s, cl_low);
> +
> +       spin_lock_irqsave(&priv->ul_lock, flags);
> +
> +       tsens_read_irq_state(priv, hw_id, s, &d);
> +
> +       /* Write the new thresholds and clear the status */
> +       regmap_field_write(priv->rf[LOW_THRESH_0 + hw_id], low_val);
> +       regmap_field_write(priv->rf[UP_THRESH_0 + hw_id], high_val);
> +       tsens_set_interrupt(priv, d, hw_id, LOWER, enable);
> +       tsens_set_interrupt(priv, d, hw_id, UPPER, enable);
> +
> +       /* TODO: (amit) REALLY??? */
> +       mb();

Again!

> +
> +       spin_unlock_irqrestore(&priv->ul_lock, flags);
> +
> +       dev_dbg(dev, "[%u] %s: (%d:%d)->(%d:%d)\n",
> +               s->hw_id, __func__, d.low_thresh, d.up_thresh, cl_low, cl_high);
> +
> +       return 0;
> +}
> +
> +int tsens_enable_irq(struct tsens_priv *priv)
> +{
> +       int ret;
> +       int val = (tsens_ver(priv) > VER_1_X) ? 7 : 1;

Drop useless parenthesis.

> +
> +       ret = regmap_field_write(priv->rf[INT_EN], val);
> +       if (ret < 0)
> +               dev_err(priv->dev, "%s: failed to enable interrupts\n", __func__);
> +
> +       return ret;
> +}
> +
> +void tsens_disable_irq(struct tsens_priv *priv)
> +{
> +       regmap_field_write(priv->rf[INT_EN], 0);
> +}
> +
>  int get_temp_tsens_valid(struct tsens_sensor *s, int *temp)
>  {
>         struct tsens_priv *priv = s->priv;
> @@ -334,6 +719,88 @@ int __init init_common(struct tsens_priv *priv)
>                         goto err_put_device;
>                 }
>         }
> +       for (i = 0, j = UPPER_STATUS_0; i < priv->feat->max_sensors; i++, j++) {
> +               priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map,
> +                                                     priv->fields[j]);
> +               if (IS_ERR(priv->rf[j])) {
> +                       ret = PTR_ERR(priv->rf[j]);
> +                       goto err_put_device;
> +               }
> +       }
> +       for (i = 0, j = LOWER_STATUS_0; i < priv->feat->max_sensors; i++, j++) {
> +               priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map,
> +                                                     priv->fields[j]);
> +               if (IS_ERR(priv->rf[j])) {
> +                       ret = PTR_ERR(priv->rf[j]);
> +                       goto err_put_device;
> +               }
> +       }
> +       for (i = 0, j = CRITICAL_STATUS_0; i < priv->feat->max_sensors; i++, j++) {
> +               priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map,
> +                                                     priv->fields[j]);
> +               if (IS_ERR(priv->rf[j])) {
> +                       ret = PTR_ERR(priv->rf[j]);
> +                       goto err_put_device;
> +               }
> +       }

Can you make a function for this? Takes priv, and a 'j' and then does
the allocation and returns failure if something went bad? Then it's a
simple

	devm_tsens_regmap_field_alloc(dev, priv, CRITICAL_STATUS_0);

pile of calls. Maybe it could even be iterated through for j too in
another loop, not sure how the registers are setup though.

> +       for (i = 0, j = UP_THRESH_0; i < priv->feat->max_sensors; i++, j++) {
> +               priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map,
> +                                                     priv->fields[j]);
> +               if (IS_ERR(priv->rf[j])) {
> +                       ret = PTR_ERR(priv->rf[j]);
> +                       goto err_put_device;
> +               }
> +       }
> +       for (i = 0, j = LOW_THRESH_0; i < priv->feat->max_sensors; i++, j++) {
> +               priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map,
> +                                                     priv->fields[j]);
> +               if (IS_ERR(priv->rf[j])) {
> +                       ret = PTR_ERR(priv->rf[j]);
> +                       goto err_put_device;
> +               }
> +       }
> +       for (i = 0, j = UP_INT_CLEAR_0; i < priv->feat->max_sensors; i++, j++) {
> +               priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map,
> +                                                     priv->fields[j]);
> +               if (IS_ERR(priv->rf[j])) {
> +                       ret = PTR_ERR(priv->rf[j]);
> +                       goto err_put_device;
> +               }
> +       }
> +       for (i = 0, j = LOW_INT_CLEAR_0; i < priv->feat->max_sensors; i++, j++) {
> +               priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map,
> +                                                     priv->fields[j]);
> +               if (IS_ERR(priv->rf[j])) {
> +                       ret = PTR_ERR(priv->rf[j]);
> +                       goto err_put_device;
> +               }
> +       }
> +       for (i = 0, j = UP_INT_MASK_0; i < priv->feat->max_sensors; i++, j++) {
> +               priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map,
> +                                                     priv->fields[j]);
> +               if (IS_ERR(priv->rf[j])) {
> +                       ret = PTR_ERR(priv->rf[j]);
> +                       goto err_put_device;
> +               }
> +       }
> +       for (i = 0, j = LOW_INT_MASK_0; i < priv->feat->max_sensors; i++, j++) {
> +               priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map,
> +                                                     priv->fields[j]);
> +               if (IS_ERR(priv->rf[j])) {
> +                       ret = PTR_ERR(priv->rf[j]);
> +                       goto err_put_device;
> +               }
> +       }
> +
> +       priv->rf[INT_EN] = devm_regmap_field_alloc(dev, priv->tm_map,
> +                                                  priv->fields[INT_EN]);
> +       if (IS_ERR(priv->rf[INT_EN])) {
> +               ret = PTR_ERR(priv->rf[INT_EN]);
> +               goto err_put_device;
> +       }
> +
> +       tsens_enable_irq(priv);
> +       spin_lock_init(&priv->ul_lock);

Maybe you should initialize the spinlock before requesting the irq.

>  
>         tsens_debug_init(op);
>  
> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> index 772aa76b50e1..bc83e40ac0cf 100644
> --- a/drivers/thermal/qcom/tsens.c
> +++ b/drivers/thermal/qcom/tsens.c
> @@ -7,6 +7,7 @@
>  #include <linux/err.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm.h>
>  #include <linux/slab.h>
> @@ -78,12 +79,15 @@ MODULE_DEVICE_TABLE(of, tsens_table);
>  static const struct thermal_zone_of_device_ops tsens_of_ops = {
>         .get_temp = tsens_get_temp,
>         .get_trend = tsens_get_trend,
> +       .set_trips = tsens_set_trips,
>  };
>  
>  static int tsens_register(struct tsens_priv *priv)
>  {
> -       int i;
> +       int i, ret, irq;
>         struct thermal_zone_device *tzd;
> +       struct resource *res;
> +       struct platform_device *op = of_find_device_by_node(priv->dev->of_node);

What if this fails?

>  
>         for (i = 0;  i < priv->num_sensors; i++) {
>                 priv->sensor[i].priv = priv;
> @@ -96,7 +100,25 @@ static int tsens_register(struct tsens_priv *priv)
>                 if (priv->ops->enable)
>                         priv->ops->enable(priv, i);
>         }
> +
> +       res = platform_get_resource(op, IORESOURCE_IRQ, 0);
> +       if (res) {
> +               irq = res->start;
> +               ret = devm_request_threaded_irq(&op->dev, irq,
> +                                               NULL, tsens_irq_thread,
> +                                               IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> +                                               res->name, priv);
> +               if (ret) {
> +                       dev_err(&op->dev, "%s: failed to get irq\n", __func__);
> +                       goto err_put_device;
> +               }
> +               enable_irq_wake(irq);
> +       }
>         return 0;
> +
> +err_put_device:
> +       put_device(&op->dev);
> +       return ret;
>  }
>  
>  static int tsens_probe(struct platform_device *pdev)
> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
> index e1d6af71b2b9..aab47337b797 100644
> --- a/drivers/thermal/qcom/tsens.h
> +++ b/drivers/thermal/qcom/tsens.h
> @@ -13,8 +13,10 @@
>  #define CAL_DEGC_PT2           120
>  #define SLOPE_FACTOR           1000
>  #define SLOPE_DEFAULT          3200
> +#define THRESHOLD_MAX_ADC_CODE 0x3ff
> +#define THRESHOLD_MIN_ADC_CODE 0x0
>  
> -
> +#include <linux/interrupt.h>

Include this in the C driver?

>  #include <linux/thermal.h>
>  #include <linux/regmap.h>
>  
> @@ -26,6 +28,12 @@ enum tsens_ver {
>         VER_2_X,
>  };
>  
> +enum tsens_irq_type {
> +       LOWER,
> +       UPPER,

Re: [PATCH 15/15] drivers: thermal: tsens: Add interrupt support

[Amit Kucheria]

Hi Stephen,

Thanks for the thorough review.

On Sat, Aug 17, 2019 at 11:39 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Amit Kucheria (2019-07-25 15:18:50)
> > diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c
> > index 13a875b99094..f94ef79c37bc 100644
> > --- a/drivers/thermal/qcom/tsens-common.c
> > +++ b/drivers/thermal/qcom/tsens-common.c
> > @@ -13,6 +13,22 @@
> >  #include <linux/regmap.h>
> >  #include "tsens.h"
> >
> > +/* IRQ state, mask and clear */
> > +struct tsens_irq_data {
> > +       u32 up_viol;
>
> Is viol a violation? Maybe some kernel doc would be useful here.

Will fix.

> > +       int up_thresh;
> > +       u32 up_irq_mask;
> > +       u32 up_irq_clear;
> > +       u32 low_viol;
> > +       int low_thresh;
> > +       u32 low_irq_mask;
> > +       u32 low_irq_clear;
> > +       u32 crit_viol;
> > +       u32 crit_thresh;
> > +       u32 crit_irq_mask;
> > +       u32 crit_irq_clear;
> > +};
> > +
> >  char *qfprom_read(struct device *dev, const char *cname)
> >  {
> >         struct nvmem_cell *cell;
> > @@ -65,6 +81,18 @@ void compute_intercept_slope(struct tsens_priv *priv, u32 *p1,
> >         }
> >  }
> >
> > +static inline u32 degc_to_code(int degc, const struct tsens_sensor *sensor)
> > +{
> > +       u32 code = (degc * sensor->slope + sensor->offset) / SLOPE_FACTOR;
>
> This can't overflow 32-bits with the multiply and add?

Most of the public HW doesn't have any calibration data and the
defaults values seem to avoid the problem. I'll check again.

Perhaps best to just declare this as u64 and then clamp below?

> > +
> > +       if (code > THRESHOLD_MAX_ADC_CODE)
> > +               code = THRESHOLD_MAX_ADC_CODE;
> > +       else if (code < THRESHOLD_MIN_ADC_CODE)
> > +               code = THRESHOLD_MIN_ADC_CODE;
>
> Looks like
>
>         return clamp(code, THRESHOLD_MIN_ADC_CODE, THRESHOLD_MAX_ADC_CODE);

clamp_val works better with the #defines, but yes. Will fix.

> > +       pr_debug("%s: raw_code: 0x%x, degc:%d\n", __func__, code, degc);
> > +       return code;
> > +}
> > +
> >  static inline int code_to_degc(u32 adc_code, const struct tsens_sensor *s)
> >  {
> >         int degc, num, den;
> > @@ -106,6 +134,363 @@ static int tsens_hw_to_mC(char *str, struct tsens_sensor *s, int field, int temp
> >         }
> >  }
> >
> > +/**
> > + * tsens_mC_to_hw - Return correct value to be written to threshold
> > + * registers, whether in ADC code or deciCelsius depending on IP version
> > + */
> > +static int tsens_mC_to_hw(struct tsens_sensor *s, int temp)
> > +{
> > +       struct tsens_priv *priv = s->priv;
> > +
> > +       if (priv->feat->adc) {
> > +               /* milli to C to adc code */
> > +               return degc_to_code(temp / 1000, s);
> > +       } else {
> > +               /* milli to deci C */
> > +               return (temp / 100);
> > +       }
>
> Drop the else and just return without parenthesis.

Will fix.

> > +}
> > +
> > +static inline unsigned int tsens_ver(struct tsens_priv *priv)
> > +{
> > +       return priv->feat->ver_major;
> > +}
> > +
> > +static inline u32 irq_mask(u32 hw_id)
>
> Is this used?

Removed.

> > +{
> > +       return 1 << hw_id;
> > +}
> > +
> > +/**
> > + * tsens_set_interrupt_v1 - Disable an interrupt (enable = false)
> > + *                          Re-enable an interrupt (enable = true)
> > + */
> > +static void tsens_set_interrupt_v1(struct tsens_priv *priv, const struct tsens_irq_data d,
> > +                                  u32 hw_id, enum tsens_irq_type irq_type, bool enable)
> > +{
> > +       if (enable) {
> > +               switch (irq_type) {
> > +               case UPPER:
> > +                       regmap_field_write(priv->rf[UP_INT_CLEAR_0 + hw_id], 0);
> > +                       break;
> > +               case LOWER:
> > +                       regmap_field_write(priv->rf[LOW_INT_CLEAR_0 + hw_id], 0);
> > +                       break;
> > +               default:
> > +                       dev_err(priv->dev, "%s: Invalid irq_type\n", __func__);
> > +                       break;
> > +               }
> > +       } else {
> > +               switch (irq_type) {
> > +               case UPPER:
> > +                       regmap_field_write(priv->rf[UP_INT_CLEAR_0 + hw_id], 1);
> > +                       break;
> > +               case LOWER:
> > +                       regmap_field_write(priv->rf[LOW_INT_CLEAR_0 + hw_id], 1);
> > +                       break;
> > +               default:
> > +                       dev_err(priv->dev, "%s: Invalid irq_type\n", __func__);
> > +                       break;
> > +               }
> > +       }
> > +}
> > +
> > +/**
> > + * tsens_set_interrupt_v2 - Disable an interrupt (enable = false)
> > + *                          Re-enable an interrupt (enable = true)
> > + */
> > +static void tsens_set_interrupt_v2(struct tsens_priv *priv, const struct tsens_irq_data d,
> > +                                  u32 hw_id, enum tsens_irq_type irq_type, bool enable)
> > +{
> > +       if (enable) {
> > +               switch (irq_type) {
> > +               case UPPER:
> > +                       regmap_field_write(priv->rf[UP_INT_MASK_0 + hw_id], 0);
>
> Maybe just have a variable like mask_reg that is equal to UP_INT_MASK,
> etc? And then one regmap_field_write() call outside the switch that does
> the write to 0?

Agreed. Makes the code nicer.

> > +                       break;
> > +               case LOWER:
> > +                       regmap_field_write(priv->rf[LOW_INT_MASK_0 + hw_id], 0);
> > +                       break;
> > +               case CRITICAL:
> > +                       regmap_field_write(priv->rf[CRIT_INT_MASK_0 + hw_id], 0);
> > +                       break;
> > +               default:
> > +                       dev_err(priv->dev, "%s: Invalid irq_type\n", __func__);
> > +                       break;
> > +               }
> > +       } else {
> > +               /* To disable the interrupt flag for a sensor:
> > +                *  1. Mask further interrupts for this sensor
> > +                *  2. Write 1 followed by 0 to clear the interrupt
> > +                */
> > +               switch (irq_type) {
> > +               case UPPER:
> > +                       regmap_field_write(priv->rf[UP_INT_MASK_0 + hw_id], 1);
> > +                       regmap_field_write(priv->rf[UP_INT_CLEAR_0 + hw_id], 1);
> > +                       regmap_field_write(priv->rf[UP_INT_CLEAR_0 + hw_id], 0);
>
> Same comment here. Use a local variable for mask and clear and then
> write the register with three regmap_field_write() calls?

Will fix.

> > +                       break;
> > +               case LOWER:
> > +                       regmap_field_write(priv->rf[LOW_INT_MASK_0 + hw_id], 1);
> > +                       regmap_field_write(priv->rf[LOW_INT_CLEAR_0 + hw_id], 1);
> > +                       regmap_field_write(priv->rf[LOW_INT_CLEAR_0 + hw_id], 0);
> > +                       break;
> > +               case CRITICAL:
> > +                       regmap_field_write(priv->rf[CRIT_INT_MASK_0 + hw_id], 1);
> > +                       regmap_field_write(priv->rf[CRIT_INT_CLEAR_0 + hw_id], 1);
> > +                       regmap_field_write(priv->rf[CRIT_INT_CLEAR_0 + hw_id], 0);
> > +                       break;
> > +               default:
>
> I'm not sure we actually need this. Modern compilers check for not
> catching an enum value in a switch case so this shouldn't even really
> matter.

I didn't know that about enums. Will fix.

> > +                       dev_err(priv->dev, "%s: Invalid irq_type\n", __func__);
> > +                       break;
> > +               }
> > +       }
> > +}
> > +
> > +/**
> > + * tsens_set_interrupt - Disable an interrupt (enable = false)
> > + *                       Re-enable an interrupt (enable = true)
> > + */
> > +static void tsens_set_interrupt(struct tsens_priv *priv, const struct tsens_irq_data d,
>
> Why not pass a pointer to tsens_irq_data?

I actually wanted to remove tsens_irq_data completely - see comment
below. I needed it in a previous iteration but now I noticed the data
isn't used. So we can remove the parameter completely.

> > +                               u32 hw_id, enum tsens_irq_type irq_type, bool enable)
> > +{
> > +       /* FIXME: remove tsens_irq_data */
> > +       dev_dbg(priv->dev, "[%u] %s: %s -> %s\n", hw_id, __func__,
> > +               irq_type ? ((irq_type == 1) ? "UP" : "CRITICAL") : "LOW",
> > +               enable ? "en" : "dis");
> > +       if (tsens_ver(priv) > VER_1_X)
> > +               tsens_set_interrupt_v2(priv, d, hw_id, irq_type, enable);
> > +       else
> > +               tsens_set_interrupt_v1(priv, d, hw_id, irq_type, enable);
> > +}
> > +
> > +static int tsens_read_irq_state(struct tsens_priv *priv, u32 hw_id,
> > +                               struct tsens_sensor *s, struct tsens_irq_data *d)
> > +{
> > +       int ret, up_temp, low_temp;
> > +
> > +       if (hw_id > priv->num_sensors) {
> > +               dev_err(priv->dev, "%s Invalid hw_id\n", __func__);
> > +               return -EINVAL;
> > +       }
> > +
> > +       ret = regmap_field_read(priv->rf[UPPER_STATUS_0 + hw_id], &d->up_viol);
> > +       if (ret)
> > +               return ret;
> > +       ret = regmap_field_read(priv->rf[UP_THRESH_0 + hw_id], &up_temp);
> > +       if (ret)
> > +               return ret;
> > +       ret = regmap_field_read(priv->rf[LOWER_STATUS_0 + hw_id], &d->low_viol);
> > +       if (ret)
> > +               return ret;
> > +       ret = regmap_field_read(priv->rf[LOW_THRESH_0 + hw_id], &low_temp);
> > +       if (ret)
> > +               return ret;
> > +       ret = regmap_field_read(priv->rf[UP_INT_CLEAR_0 + hw_id], &d->up_irq_clear);
> > +       if (ret)
> > +               return ret;
> > +       ret = regmap_field_read(priv->rf[LOW_INT_CLEAR_0 + hw_id], &d->low_irq_clear);
> > +       if (ret)
> > +               return ret;
> > +       if (tsens_ver(priv) > VER_1_X) {
> > +               ret = regmap_field_read(priv->rf[UP_INT_MASK_0 + hw_id], &d->up_irq_mask);
> > +               if (ret)
> > +                       return ret;
> > +               ret = regmap_field_read(priv->rf[LOW_INT_MASK_0 + hw_id], &d->low_irq_mask);
> > +               if (ret)
> > +                       return ret;
> > +       } else {
> > +               /* No mask register on older TSENS */
> > +               d->up_irq_mask = 0;
> > +               d->low_irq_mask = 0;
> > +       }
> > +
> > +       d->up_thresh = tsens_hw_to_mC("upthresh", s, UP_THRESH_0, up_temp);
> > +       d->low_thresh = tsens_hw_to_mC("lowthresh", s, LOW_THRESH_0, low_temp);
> > +
> > +       dev_dbg(priv->dev, "[%u] %s%s: status(%u|%u) | clr(%u|%u) | mask(%u|%u)\n",
> > +               hw_id, __func__, (d->up_viol || d->low_viol) ? "(V)" : "",
> > +               d->low_viol, d->up_viol, d->low_irq_clear, d->up_irq_clear,
> > +               d->low_irq_mask, d->up_irq_mask);
> > +       dev_dbg(priv->dev, "[%u] %s%s: thresh: (%d:%d)\n", hw_id, __func__,
> > +               (d->up_viol || d->low_viol) ? "(violation)" : "",
> > +               d->low_thresh, d->up_thresh);
> > +
> > +       return 0;
> > +}
> > +
> > +static inline u32 masked_irq(u32 hw_id, u32 mask, enum tsens_ver ver)
> > +{
> > +       if (ver > VER_1_X) {
> > +               return mask & (1 << hw_id);
> > +       } else {
> > +               /* v1, v0.1 don't have a irq mask register */
> > +               return 0;
> > +       }
>
> Same return comment.

Will fix.

> > +}
> > +
> > +static unsigned long tsens_filter_active_sensors(struct tsens_priv *priv)
> > +{
> > +       int i, ret, up, low;
> > +       unsigned long mask = 0;
> > +
> > +       for (i = 0; i < priv->num_sensors; i++) {
> > +               struct tsens_sensor *s = &priv->sensor[i];
> > +               u32 hw_id = s->hw_id;
> > +
> > +               if (IS_ERR(priv->sensor[i].tzd))
> > +                       continue;
> > +               ret = regmap_field_read(priv->rf[UPPER_STATUS_0 + hw_id], &up);
> > +               if (ret)
> > +                       return ret;
>
> Probably don't want to return ret here given that this returns a mask.
> Maybe push this into the callsite loop and then break out or return an
> error from there instead. Making a mask via a loop and then using that
> again the second time to test the bit is more complicated.

Right. I only wanted to iterate over sensors that had crossed their
thresholds in the irq thread. Will move it back to the irq_thread
function and split tsens_read_irq_state into two parts - one to just
check if the sensor crossed a threshold, the other reading all other
irq information.

> > +               ret = regmap_field_read(priv->rf[LOWER_STATUS_0 + hw_id], &low);
> > +               if (ret)
> > +                       return ret;
> > +               if (up || low)
> > +                       set_bit(hw_id, &mask);
> > +       }
> > +       dev_dbg(priv->dev, "%s: hw_id mask: 0x%lx\n",  __func__, mask);
> > +
> > +       return mask;
> > +}
> > +
> > +irqreturn_t tsens_irq_thread(int irq, void *data)
> > +{
> > +       struct tsens_priv *priv = data;
> > +       int temp, ret, i;
> > +       unsigned long flags;
> > +       bool enable = true, disable = false;
> > +       unsigned long mask = tsens_filter_active_sensors(priv);
> > +
> > +       if (!mask) {
> > +               dev_err(priv->dev, "%s: Spurious interrupt?\n", __func__);
>
> Do we need the spurious irq print? Doesn't genirq already tell us about
> spurious interrupts and shuts them down?

Already got rid of this. I realised it isn't a spurious interrupt but
the irq being level triggered and sometimes the temperature gets back
to below the thresholds in the time where the irq thread is schduled.

> > +               return IRQ_NONE;

we return IRQ_HANDLED here, in that case.

> > +       }
> > +
> > +       /* Check if any sensor raised an IRQ - for each sensor connected to the
>
> /*
>  * Please make multiline comments
>  * like this
>  */

Done.

> > +        * TSENS block if it set the threshold violation bit.
> > +        */
> > +       for (i = 0; i < priv->num_sensors; i++) {
> > +               struct tsens_sensor *s = &priv->sensor[i];
> > +               struct tsens_irq_data d;
> > +               u32 hw_id = s->hw_id;
> > +               bool trigger = 0;
> > +
> > +               if (!test_bit(hw_id, &mask))
> > +                       continue;
> > +               if (IS_ERR(priv->sensor[i].tzd))
> > +                       continue;
> > +               ret = get_temp_tsens_valid(s, &temp);
> > +               if (ret) {
> > +                       dev_err(priv->dev, "[%u] %s: error reading sensor\n", hw_id, __func__);
> > +                       continue;
> > +               }
> > +
> > +               spin_lock_irqsave(&priv->ul_lock, flags);
> > +
> > +               tsens_read_irq_state(priv, hw_id, s, &d);
> > +
> > +               if (d.up_viol &&
> > +                   !masked_irq(hw_id, d.up_irq_mask, tsens_ver(priv))) {
> > +                       tsens_set_interrupt(priv, d, hw_id, UPPER, disable);
> > +                       if (d.up_thresh > temp) {
> > +                               dev_dbg(priv->dev, "[%u] %s: re-arm upper\n",
> > +                                       priv->sensor[i].hw_id, __func__);
> > +                               /* unmask the interrupt for this sensor */
> > +                               tsens_set_interrupt(priv, d, hw_id, UPPER, enable);
> > +                       } else {
> > +                               trigger = 1;
> > +                               /* Keep irq masked */
> > +                       }
> > +               } else if (d.low_viol &&
> > +                          !masked_irq(hw_id, d.low_irq_mask, tsens_ver(priv))) {
> > +                       tsens_set_interrupt(priv, d, hw_id, LOWER, disable);
> > +                       if (d.low_thresh < temp) {
> > +                               dev_dbg(priv->dev, "[%u] %s: re-arm low\n",
> > +                                       priv->sensor[i].hw_id, __func__);
> > +                               /* unmask the interrupt for this sensor */
> > +                               tsens_set_interrupt(priv, d, hw_id, LOWER, enable);
> > +                       } else {
> > +                               trigger = 1;
> > +                               /* Keep irq masked */
> > +                       }
> > +               }
> > +
> > +               /* TODO: (amit) REALLY??? */
>
> Remove it, and the mb?

Already removed, this shouldn't have snuck through in v1.

> > +               mb();
> > +
> > +               spin_unlock_irqrestore(&priv->ul_lock, flags);
> > +
> > +               if (trigger) {
> > +                       dev_dbg(priv->dev, "[%u] %s: TZ update trigger (%d mC)\n",
> > +                               hw_id, __func__, temp);
> > +                       thermal_zone_device_update(priv->sensor[i].tzd,
> > +                                                  THERMAL_EVENT_UNSPECIFIED);
> > +               } else {
> > +                       dev_dbg(priv->dev, "[%u] %s: no violation:  %d\n",
> > +                               hw_id, __func__, temp);
> > +               }
> > +       }
> > +
> > +       return IRQ_HANDLED;
> > +}
> > +
> > +int tsens_set_trips(void *_sensor, int low, int high)
> > +{
> > +       struct tsens_sensor *s = _sensor;
> > +       struct tsens_priv *priv = s->priv;
> > +       struct device *dev = priv->dev;
> > +       struct tsens_irq_data d;
> > +       unsigned long flags;
> > +       int high_val, low_val, cl_high, cl_low;
> > +       bool enable = true;
> > +       u32 hw_id = s->hw_id;
> > +
> > +       dev_dbg(dev, "[%u] %s: proposed thresholds: (%d:%d)\n",
> > +               hw_id, __func__, low, high);
> > +
> > +       cl_high = clamp_val(high, -40000, 120000);
> > +       cl_low  = clamp_val(low, -40000, 120000);
> > +
> > +       high_val = tsens_mC_to_hw(s, cl_high);
> > +       low_val  = tsens_mC_to_hw(s, cl_low);
> > +
> > +       spin_lock_irqsave(&priv->ul_lock, flags);
> > +
> > +       tsens_read_irq_state(priv, hw_id, s, &d);
> > +
> > +       /* Write the new thresholds and clear the status */
> > +       regmap_field_write(priv->rf[LOW_THRESH_0 + hw_id], low_val);
> > +       regmap_field_write(priv->rf[UP_THRESH_0 + hw_id], high_val);
> > +       tsens_set_interrupt(priv, d, hw_id, LOWER, enable);
> > +       tsens_set_interrupt(priv, d, hw_id, UPPER, enable);
> > +
> > +       /* TODO: (amit) REALLY??? */
> > +       mb();
>
> Again!

Done

> > +
> > +       spin_unlock_irqrestore(&priv->ul_lock, flags);
> > +
> > +       dev_dbg(dev, "[%u] %s: (%d:%d)->(%d:%d)\n",
> > +               s->hw_id, __func__, d.low_thresh, d.up_thresh, cl_low, cl_high);
> > +
> > +       return 0;
> > +}
> > +
> > +int tsens_enable_irq(struct tsens_priv *priv)
> > +{
> > +       int ret;
> > +       int val = (tsens_ver(priv) > VER_1_X) ? 7 : 1;
>
> Drop useless parenthesis.

Will fix.

> > +
> > +       ret = regmap_field_write(priv->rf[INT_EN], val);
> > +       if (ret < 0)
> > +               dev_err(priv->dev, "%s: failed to enable interrupts\n", __func__);
> > +
> > +       return ret;
> > +}
> > +
> > +void tsens_disable_irq(struct tsens_priv *priv)
> > +{
> > +       regmap_field_write(priv->rf[INT_EN], 0);
> > +}
> > +
> >  int get_temp_tsens_valid(struct tsens_sensor *s, int *temp)
> >  {
> >         struct tsens_priv *priv = s->priv;
> > @@ -334,6 +719,88 @@ int __init init_common(struct tsens_priv *priv)
> >                         goto err_put_device;
> >                 }
> >         }
> > +       for (i = 0, j = UPPER_STATUS_0; i < priv->feat->max_sensors; i++, j++) {
> > +               priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map,
> > +                                                     priv->fields[j]);
> > +               if (IS_ERR(priv->rf[j])) {
> > +                       ret = PTR_ERR(priv->rf[j]);
> > +                       goto err_put_device;
> > +               }
> > +       }
> > +       for (i = 0, j = LOWER_STATUS_0; i < priv->feat->max_sensors; i++, j++) {
> > +               priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map,
> > +                                                     priv->fields[j]);
> > +               if (IS_ERR(priv->rf[j])) {
> > +                       ret = PTR_ERR(priv->rf[j]);
> > +                       goto err_put_device;
> > +               }
> > +       }
> > +       for (i = 0, j = CRITICAL_STATUS_0; i < priv->feat->max_sensors; i++, j++) {
> > +               priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map,
> > +                                                     priv->fields[j]);
> > +               if (IS_ERR(priv->rf[j])) {
> > +                       ret = PTR_ERR(priv->rf[j]);
> > +                       goto err_put_device;
> > +               }
> > +       }
>
> Can you make a function for this? Takes priv, and a 'j' and then does
> the allocation and returns failure if something went bad? Then it's a
> simple
>
>         devm_tsens_regmap_field_alloc(dev, priv, CRITICAL_STATUS_0);
>
> pile of calls. Maybe it could even be iterated through for j too in
> another loop, not sure how the registers are setup though.

Yes, I will fix this. Some history for why I have this silly
implementation: I was trying to figure out what bit fields were
present in different versions of the IP while trying to save some
bytes by only alloc'ing things are present on the version of the IP. I
have reached the conclusion that we should just alloc in a loop is a
lot simpler at the cost of some extra bytes.

I will revisit this again.

Thanks again for the review.

Regards,
Amit

> > +       for (i = 0, j = UP_THRESH_0; i < priv->feat->max_sensors; i++, j++) {
> > +               priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map,
> > +                                                     priv->fields[j]);
> > +               if (IS_ERR(priv->rf[j])) {
> > +                       ret = PTR_ERR(priv->rf[j]);
> > +                       goto err_put_device;
> > +               }
> > +       }
> > +       for (i = 0, j = LOW_THRESH_0; i < priv->feat->max_sensors; i++, j++) {
> > +               priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map,
> > +                                                     priv->fields[j]);
> > +               if (IS_ERR(priv->rf[j])) {
> > +                       ret = PTR_ERR(priv->rf[j]);
> > +                       goto err_put_device;
> > +               }
> > +       }
> > +       for (i = 0, j = UP_INT_CLEAR_0; i < priv->feat->max_sensors; i++, j++) {
> > +               priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map,
> > +                                                     priv->fields[j]);
> > +               if (IS_ERR(priv->rf[j])) {
> > +                       ret = PTR_ERR(priv->rf[j]);
> > +                       goto err_put_device;
> > +               }
> > +       }
> > +       for (i = 0, j = LOW_INT_CLEAR_0; i < priv->feat->max_sensors; i++, j++) {
> > +               priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map,
> > +                                                     priv->fields[j]);
> > +               if (IS_ERR(priv->rf[j])) {
> > +                       ret = PTR_ERR(priv->rf[j]);
> > +                       goto err_put_device;
> > +               }
> > +       }
> > +       for (i = 0, j = UP_INT_MASK_0; i < priv->feat->max_sensors; i++, j++) {
> > +               priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map,
> > +                                                     priv->fields[j]);
> > +               if (IS_ERR(priv->rf[j])) {
> > +                       ret = PTR_ERR(priv->rf[j]);
> > +                       goto err_put_device;
> > +               }
> > +       }
> > +       for (i = 0, j = LOW_INT_MASK_0; i < priv->feat->max_sensors; i++, j++) {
> > +               priv->rf[j] = devm_regmap_field_alloc(dev, priv->tm_map,
> > +                                                     priv->fields[j]);
> > +               if (IS_ERR(priv->rf[j])) {
> > +                       ret = PTR_ERR(priv->rf[j]);
> > +                       goto err_put_device;
> > +               }
> > +       }
> > +
> > +       priv->rf[INT_EN] = devm_regmap_field_alloc(dev, priv->tm_map,
> > +                                                  priv->fields[INT_EN]);
> > +       if (IS_ERR(priv->rf[INT_EN])) {
> > +               ret = PTR_ERR(priv->rf[INT_EN]);
> > +               goto err_put_device;
> > +       }
> > +
> > +       tsens_enable_irq(priv);
> > +       spin_lock_init(&priv->ul_lock);
>
> Maybe you should initialize the spinlock before requesting the irq.
>
> >
> >         tsens_debug_init(op);
> >
> > diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> > index 772aa76b50e1..bc83e40ac0cf 100644
> > --- a/drivers/thermal/qcom/tsens.c
> > +++ b/drivers/thermal/qcom/tsens.c
> > @@ -7,6 +7,7 @@
> >  #include <linux/err.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> > +#include <linux/of_platform.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pm.h>
> >  #include <linux/slab.h>
> > @@ -78,12 +79,15 @@ MODULE_DEVICE_TABLE(of, tsens_table);
> >  static const struct thermal_zone_of_device_ops tsens_of_ops = {
> >         .get_temp = tsens_get_temp,
> >         .get_trend = tsens_get_trend,
> > +       .set_trips = tsens_set_trips,
> >  };
> >
> >  static int tsens_register(struct tsens_priv *priv)
> >  {
> > -       int i;
> > +       int i, ret, irq;
> >         struct thermal_zone_device *tzd;
> > +       struct resource *res;
> > +       struct platform_device *op = of_find_device_by_node(priv->dev->of_node);
>
> What if this fails?
>
> >
> >         for (i = 0;  i < priv->num_sensors; i++) {
> >                 priv->sensor[i].priv = priv;
> > @@ -96,7 +100,25 @@ static int tsens_register(struct tsens_priv *priv)
> >                 if (priv->ops->enable)
> >                         priv->ops->enable(priv, i);
> >         }
> > +
> > +       res = platform_get_resource(op, IORESOURCE_IRQ, 0);
> > +       if (res) {
> > +               irq = res->start;
> > +               ret = devm_request_threaded_irq(&op->dev, irq,
> > +                                               NULL, tsens_irq_thread,
> > +                                               IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> > +                                               res->name, priv);
> > +               if (ret) {
> > +                       dev_err(&op->dev, "%s: failed to get irq\n", __func__);
> > +                       goto err_put_device;
> > +               }
> > +               enable_irq_wake(irq);
> > +       }
> >         return 0;
> > +
> > +err_put_device:
> > +       put_device(&op->dev);
> > +       return ret;
> >  }
> >
> >  static int tsens_probe(struct platform_device *pdev)
> > diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
> > index e1d6af71b2b9..aab47337b797 100644
> > --- a/drivers/thermal/qcom/tsens.h
> > +++ b/drivers/thermal/qcom/tsens.h
> > @@ -13,8 +13,10 @@
> >  #define CAL_DEGC_PT2           120
> >  #define SLOPE_FACTOR           1000
> >  #define SLOPE_DEFAULT          3200
> > +#define THRESHOLD_MAX_ADC_CODE 0x3ff
> > +#define THRESHOLD_MIN_ADC_CODE 0x0
> >
> > -
> > +#include <linux/interrupt.h>
>
> Include this in the C driver?
>
> >  #include <linux/thermal.h>
> >  #include <linux/regmap.h>
> >
> > @@ -26,6 +28,12 @@ enum tsens_ver {
> >         VER_2_X,
> >  };
> >
> > +enum tsens_irq_type {
> > +       LOWER,
> > +       UPPER,

Re: [PATCH 00/15] thermal: qcom: tsens: Add interrupt support

[Brian Masney]

Hi Amit,

On Fri, Jul 26, 2019 at 03:48:35AM +0530, Amit Kucheria wrote:
> Add interrupt support to TSENS. The first 6 patches are general fixes and
> cleanups to the driver before interrupt support is introduced.
> 
> This series has been developed against qcs404 and sdm845 and then tested on
> msm8916. Testing on msm8998 and msm8974 would be appreciated since I don't
> have hardware handy. Further, I plan to test on msm8996 and also submit to
> kernelci.
> 
> I'm sending this out for more review to get help with testing.

I can test this on msm8974 for you using a Nexus 5. Here's what I've
done so far:

The device tree nodes appear in sysfs:

/ # ls -1 /sys/class/thermal/
cooling_device0
cooling_device1
thermal_zone0
thermal_zone1
thermal_zone2
thermal_zone3
thermal_zone4
thermal_zone5
thermal_zone6
thermal_zone7
thermal_zone8
thermal_zone9

The various temperatures were in the upper 40s and I threw some work at
all four CPU cores to warm up the phone and watched the various
temperatures rise:

/ # for i in $(seq 0 9) ; do
> TYPE=$(cat /sys/class/thermal/thermal_zone$i/type)
> TEMP=$(cat /sys/class/thermal/thermal_zone$i/temp)
> echo "$TYPE = $TEMP"
> done
cpu-thermal0 = 66000
cpu-thermal1 = 66000
cpu-thermal2 = 66000
cpu-thermal3 = 66000
q6-dsp-thermal = 60000
modemtx-thermal = 57000
video-thermal = 61000
wlan-thermal = 65000
gpu-thermal-top = 61000
gpu-thermal-bottom = 59000

To test the interrupt support, I lowered all of the temperature trips to
51C but I'm not sure where to read that notification. I assume one of
the cooling devices or a governor should be started? Sorry but I haven't
done any work in the thermal subsystem yet and I'm short on time this
morning to investigate right now.

Brian

Re: [PATCH 00/15] thermal: qcom: tsens: Add interrupt support

[Amit Kucheria]

(Resending from a desktop client because mobile gmail apparently sends
html that gets rejected by all lists)

On Fri, Jul 26, 2019 at 4:06 PM Brian Masney <masneyb@onstation.org> wrote:
>
> Hi Amit,
>
> On Fri, Jul 26, 2019 at 03:48:35AM +0530, Amit Kucheria wrote:
> > Add interrupt support to TSENS. The first 6 patches are general fixes and
> > cleanups to the driver before interrupt support is introduced.
> >
> > This series has been developed against qcs404 and sdm845 and then tested on
> > msm8916. Testing on msm8998 and msm8974 would be appreciated since I don't
> > have hardware handy. Further, I plan to test on msm8996 and also submit to
> > kernelci.
> >
> > I'm sending this out for more review to get help with testing.
>
> I can test this on msm8974 for you using a Nexus 5. Here's what I've
> done so far:

Thanks. I was hoping that would be the case given all your effort
getting Nexus 5 supported. :-)

> The device tree nodes appear in sysfs:
>
> / # ls -1 /sys/class/thermal/
> cooling_device0
> cooling_device1
> thermal_zone0
> thermal_zone1
> thermal_zone2
> thermal_zone3
> thermal_zone4
> thermal_zone5
> thermal_zone6
> thermal_zone7
> thermal_zone8
> thermal_zone9

Looks good. What are the contents of the files inside the two
cooling_device directories? The output of the following command would
be nice:

$ grep "" cooling_device?/*

> The various temperatures were in the upper 40s and I threw some work at
> all four CPU cores to warm up the phone and watched the various
> temperatures rise:
>
> / # for i in $(seq 0 9) ; do
> > TYPE=$(cat /sys/class/thermal/thermal_zone$i/type)
> > TEMP=$(cat /sys/class/thermal/thermal_zone$i/temp)
> > echo "$TYPE = $TEMP"
> > done
> cpu-thermal0 = 66000
> cpu-thermal1 = 66000
> cpu-thermal2 = 66000
> cpu-thermal3 = 66000
> q6-dsp-thermal = 60000
> modemtx-thermal = 57000
> video-thermal = 61000
> wlan-thermal = 65000
> gpu-thermal-top = 61000
> gpu-thermal-bottom = 59000
>
> To test the interrupt support, I lowered all of the temperature trips to
> 51C but I'm not sure where to read that notification. I assume one of
> the cooling devices or a governor should be started? Sorry but I haven't
> done any work in the thermal subsystem yet and I'm short on time this
> morning to investigate right now.

For now, just checking if the tsens interrupt in /proc/interrupts
fires should be fine. I have another patch to add some information to
debugs that I'll send at some point.

How well does cpufreq work on 8974? I haven't looked at it yet but
we'll need it for thermal throttling.

> Brian

Re: [PATCH 00/15] thermal: qcom: tsens: Add interrupt support

[Brian Masney]

Hi Amit,

On Fri, Jul 26, 2019 at 04:40:16PM +0530, Amit Kucheria wrote:
> > The device tree nodes appear in sysfs:
> >
> > / # ls -1 /sys/class/thermal/
> > cooling_device0
> > cooling_device1
> > thermal_zone0
> > thermal_zone1
> > thermal_zone2
> > thermal_zone3
> > thermal_zone4
> > thermal_zone5
> > thermal_zone6
> > thermal_zone7
> > thermal_zone8
> > thermal_zone9
> 
> Looks good. What are the contents of the files inside the two
> cooling_device directories? The output of the following command would
> be nice:
> 
> $ grep "" cooling_device?/*

/sys/class/thermal # grep "" cooling_device?/*
cooling_device0/cur_state:100000
cooling_device0/max_state:2500000
cooling_device0/type:smbb-usbin
cooling_device1/cur_state:500000
cooling_device1/max_state:2500000
cooling_device1/type:smbb-dcin

> > The various temperatures were in the upper 40s and I threw some work at
> > all four CPU cores to warm up the phone and watched the various
> > temperatures rise:
> >
> > / # for i in $(seq 0 9) ; do
> > > TYPE=$(cat /sys/class/thermal/thermal_zone$i/type)
> > > TEMP=$(cat /sys/class/thermal/thermal_zone$i/temp)
> > > echo "$TYPE = $TEMP"
> > > done
> > cpu-thermal0 = 66000
> > cpu-thermal1 = 66000
> > cpu-thermal2 = 66000
> > cpu-thermal3 = 66000
> > q6-dsp-thermal = 60000
> > modemtx-thermal = 57000
> > video-thermal = 61000
> > wlan-thermal = 65000
> > gpu-thermal-top = 61000
> > gpu-thermal-bottom = 59000
> >
> > To test the interrupt support, I lowered all of the temperature trips to
> > 51C but I'm not sure where to read that notification. I assume one of
> > the cooling devices or a governor should be started? Sorry but I haven't
> > done any work in the thermal subsystem yet and I'm short on time this
> > morning to investigate right now.
> 
> For now, just checking if the tsens interrupt in /proc/interrupts
> fires should be fine. I have another patch to add some information to
> debugs that I'll send at some point.

An interrupt fires as each thermal zone exceeds the trip temperature and
an interrupt fires again when it goes below that temperature.
Here's my new test script:

for i in $(seq 0 9) ; do
	TYPE=$(cat /sys/class/thermal/thermal_zone$i/type)
	TEMP=$(cat /sys/class/thermal/thermal_zone$i/temp)
	TRIP=$(cat /sys/class/thermal/thermal_zone$i/trip_point_0_temp)
	echo "$TYPE = $TEMP. trip = $TRIP"
done

# Warm the phone up

/sys/class/thermal # /temp.sh 
cpu-thermal0 = 57000. trip = 51000
cpu-thermal1 = 56000. trip = 51000
cpu-thermal2 = 57000. trip = 51000
cpu-thermal3 = 56000. trip = 51000
q6-dsp-thermal = 51000. trip = 51000
modemtx-thermal = 49000. trip = 51000
video-thermal = 53000. trip = 51000
wlan-thermal = 55000. trip = 51000
gpu-thermal-top = 53000. trip = 51000
gpu-thermal-bottom = 52000. trip = 51000

/sys/class/thermal # grep tsens /proc/interrupts 
 27:          8          0          0          0     GIC-0 216 Level     tsens

# Let the phone cool off

/sys/class/thermal # /temp.sh 
cpu-thermal0 = 48000. trip = 51000
cpu-thermal1 = 48000. trip = 51000
cpu-thermal2 = 49000. trip = 51000
cpu-thermal3 = 48000. trip = 51000
q6-dsp-thermal = 47000. trip = 51000
modemtx-thermal = 45000. trip = 51000
video-thermal = 48000. trip = 51000
wlan-thermal = 48000. trip = 51000
gpu-thermal-top = 48000. trip = 51000
gpu-thermal-bottom = 47000. trip = 51000

/sys/class/thermal # grep tsens /proc/interrupts 
 27:         19          0          0          0     GIC-0 216 Level     tsens

> How well does cpufreq work on 8974? I haven't looked at it yet but
> we'll need it for thermal throttling.

I'm not sure how to tell if the frequency is dynamically changed during
runtime on arm. x86-64 shows this information in /proc/cpuinfo. Here's
the /proc/cpuinfo on the Nexus 5:

/sys/class/thermal # cat /proc/cpuinfo 
processor       : 0
model name      : ARMv7 Processor rev 0 (v7l)
BogoMIPS        : 38.40
Features        : half thumb fastmult vfp edsp neon vfpv3 tls vfpv4 idiva idivt vfpd32 evtstrm 
CPU implementer : 0x51
CPU architecture: 7
CPU variant     : 0x2
CPU part        : 0x06f
CPU revision    : 0

# 3 more CPUs like 0....

Hardware        : Generic DT based system
Revision        : 0000
Serial          : 0000000000000000

Brian

Re: [PATCH 00/15] thermal: qcom: tsens: Add interrupt support

[Amit Kucheria]

On Fri, Jul 26, 2019 at 4:59 PM Brian Masney <masneyb@onstation.org> wrote:
>
> Hi Amit,
>
> On Fri, Jul 26, 2019 at 04:40:16PM +0530, Amit Kucheria wrote:
> > > The device tree nodes appear in sysfs:
> > >
> > > / # ls -1 /sys/class/thermal/
> > > cooling_device0
> > > cooling_device1
> > > thermal_zone0
> > > thermal_zone1
> > > thermal_zone2
> > > thermal_zone3
> > > thermal_zone4
> > > thermal_zone5
> > > thermal_zone6
> > > thermal_zone7
> > > thermal_zone8
> > > thermal_zone9
> >
> > Looks good. What are the contents of the files inside the two
> > cooling_device directories? The output of the following command would
> > be nice:
> >
> > $ grep "" cooling_device?/*
>
> /sys/class/thermal # grep "" cooling_device?/*
> cooling_device0/cur_state:100000
> cooling_device0/max_state:2500000
> cooling_device0/type:smbb-usbin
> cooling_device1/cur_state:500000
> cooling_device1/max_state:2500000
> cooling_device1/type:smbb-dcin
>
> > > The various temperatures were in the upper 40s and I threw some work at
> > > all four CPU cores to warm up the phone and watched the various
> > > temperatures rise:
> > >
> > > / # for i in $(seq 0 9) ; do
> > > > TYPE=$(cat /sys/class/thermal/thermal_zone$i/type)
> > > > TEMP=$(cat /sys/class/thermal/thermal_zone$i/temp)
> > > > echo "$TYPE = $TEMP"
> > > > done
> > > cpu-thermal0 = 66000
> > > cpu-thermal1 = 66000
> > > cpu-thermal2 = 66000
> > > cpu-thermal3 = 66000
> > > q6-dsp-thermal = 60000
> > > modemtx-thermal = 57000
> > > video-thermal = 61000
> > > wlan-thermal = 65000
> > > gpu-thermal-top = 61000
> > > gpu-thermal-bottom = 59000
> > >
> > > To test the interrupt support, I lowered all of the temperature trips to
> > > 51C but I'm not sure where to read that notification. I assume one of
> > > the cooling devices or a governor should be started? Sorry but I haven't
> > > done any work in the thermal subsystem yet and I'm short on time this
> > > morning to investigate right now.
> >
> > For now, just checking if the tsens interrupt in /proc/interrupts
> > fires should be fine. I have another patch to add some information to
> > debugs that I'll send at some point.
>
> An interrupt fires as each thermal zone exceeds the trip temperature and
> an interrupt fires again when it goes below that temperature.
> Here's my new test script:
>
> for i in $(seq 0 9) ; do
>         TYPE=$(cat /sys/class/thermal/thermal_zone$i/type)
>         TEMP=$(cat /sys/class/thermal/thermal_zone$i/temp)
>         TRIP=$(cat /sys/class/thermal/thermal_zone$i/trip_point_0_temp)
>         echo "$TYPE = $TEMP. trip = $TRIP"
> done
>
> # Warm the phone up
>
> /sys/class/thermal # /temp.sh
> cpu-thermal0 = 57000. trip = 51000
> cpu-thermal1 = 56000. trip = 51000
> cpu-thermal2 = 57000. trip = 51000
> cpu-thermal3 = 56000. trip = 51000
> q6-dsp-thermal = 51000. trip = 51000
> modemtx-thermal = 49000. trip = 51000
> video-thermal = 53000. trip = 51000
> wlan-thermal = 55000. trip = 51000
> gpu-thermal-top = 53000. trip = 51000
> gpu-thermal-bottom = 52000. trip = 51000
>
> /sys/class/thermal # grep tsens /proc/interrupts
>  27:          8          0          0          0     GIC-0 216 Level     tsens
>
> # Let the phone cool off
>
> /sys/class/thermal # /temp.sh
> cpu-thermal0 = 48000. trip = 51000
> cpu-thermal1 = 48000. trip = 51000
> cpu-thermal2 = 49000. trip = 51000
> cpu-thermal3 = 48000. trip = 51000
> q6-dsp-thermal = 47000. trip = 51000
> modemtx-thermal = 45000. trip = 51000
> video-thermal = 48000. trip = 51000
> wlan-thermal = 48000. trip = 51000
> gpu-thermal-top = 48000. trip = 51000
> gpu-thermal-bottom = 47000. trip = 51000
>
> /sys/class/thermal # grep tsens /proc/interrupts
>  27:         19          0          0          0     GIC-0 216 Level     tsens

OK, seems reasonable. I'll finish up a debugfs patch that'll dump more
state transition information to give more insight.

> > How well does cpufreq work on 8974? I haven't looked at it yet but
> > we'll need it for thermal throttling.
>
> I'm not sure how to tell if the frequency is dynamically changed during
> runtime on arm. x86-64 shows this information in /proc/cpuinfo. Here's
> the /proc/cpuinfo on the Nexus 5:

Nah. /proc/cpuinfo won't show what we need.

Try the following:

$ grep "" /sys/devices/system/cpu/cpufreq/policy?/*

More specifically, the following files have the information you need.
Run watch -n1 on them.

$ grep "" /sys/devices/system/cpu/cpufreq/policy?/scaling_*_freq

Thanks for your help.

Regards,
Amit

Re: [PATCH 00/15] thermal: qcom: tsens: Add interrupt support

[Brian Masney]

On Sat, Jul 27, 2019 at 12:58:54PM +0530, Amit Kucheria wrote:
> On Fri, Jul 26, 2019 at 4:59 PM Brian Masney <masneyb@onstation.org> wrote:
> > On Fri, Jul 26, 2019 at 04:40:16PM +0530, Amit Kucheria wrote:
> > > How well does cpufreq work on 8974? I haven't looked at it yet but
> > > we'll need it for thermal throttling.
> >
> > I'm not sure how to tell if the frequency is dynamically changed during
> > runtime on arm. x86-64 shows this information in /proc/cpuinfo. Here's
> > the /proc/cpuinfo on the Nexus 5:
> 
> Nah. /proc/cpuinfo won't show what we need.
> 
> Try the following:
> 
> $ grep "" /sys/devices/system/cpu/cpufreq/policy?/*
> 
> More specifically, the following files have the information you need.
> Run watch -n1 on them.
> 
> $ grep "" /sys/devices/system/cpu/cpufreq/policy?/scaling_*_freq

There's no cpufreq directory on msm8974:

    # ls -1 /sys/devices/system/cpu/
    cpu0
    cpu1
    cpu2
    cpu3
    cpuidle
    hotplug
    isolated
    kernel_max
    modalias
    offline
    online
    possible
    power
    present
    smt
    uevent

I'm using qcom_defconfig.

Brian

Re: [PATCH 00/15] thermal: qcom: tsens: Add interrupt support

[Luca Weiss]

[-- Attachment #1: Type: text/plain, Size: 1591 bytes --]

On Montag, 29. Juli 2019 11:07:35 CEST Brian Masney wrote:
> On Sat, Jul 27, 2019 at 12:58:54PM +0530, Amit Kucheria wrote:
> > On Fri, Jul 26, 2019 at 4:59 PM Brian Masney <masneyb@onstation.org> wrote:
> > > On Fri, Jul 26, 2019 at 04:40:16PM +0530, Amit Kucheria wrote:
> > > > How well does cpufreq work on 8974? I haven't looked at it yet but
> > > > we'll need it for thermal throttling.
> > > 
> > > I'm not sure how to tell if the frequency is dynamically changed during
> > > runtime on arm. x86-64 shows this information in /proc/cpuinfo. Here's
> > 
> > > the /proc/cpuinfo on the Nexus 5:
> > Nah. /proc/cpuinfo won't show what we need.
> > 
> > Try the following:
> > 
> > $ grep "" /sys/devices/system/cpu/cpufreq/policy?/*
> > 
> > More specifically, the following files have the information you need.
> > Run watch -n1 on them.
> > 
> > $ grep "" /sys/devices/system/cpu/cpufreq/policy?/scaling_*_freq
> 
> There's no cpufreq directory on msm8974:
> 
>     # ls -1 /sys/devices/system/cpu/
>     cpu0
>     cpu1
>     cpu2
>     cpu3
>     cpuidle
>     hotplug
>     isolated
>     kernel_max
>     modalias
>     offline
>     online
>     possible
>     power
>     present
>     smt
>     uevent
> 
> I'm using qcom_defconfig.
> 
> Brian

Hi Brian,
cpufreq isn't supported on msm8974 yet.
I have these patches [0] in my tree but I'm not sure they work correctly, but I haven't tested much with them. Feel free to try them on hammerhead.

Luca

[0] https://github.com/z3ntu/linux/compare/b0917f53ada0e929896a094b451219cd8091366e...6459ca6aff498c9d12acd35709b4903effc4c3f8

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 00/15] thermal: qcom: tsens: Add interrupt support

[Amit Kucheria]

On Mon, Jul 29, 2019 at 3:03 PM Luca Weiss <luca@z3ntu.xyz> wrote:
>
> On Montag, 29. Juli 2019 11:07:35 CEST Brian Masney wrote:
> > On Sat, Jul 27, 2019 at 12:58:54PM +0530, Amit Kucheria wrote:
> > > On Fri, Jul 26, 2019 at 4:59 PM Brian Masney <masneyb@onstation.org> wrote:
> > > > On Fri, Jul 26, 2019 at 04:40:16PM +0530, Amit Kucheria wrote:
> > > > > How well does cpufreq work on 8974? I haven't looked at it yet but
> > > > > we'll need it for thermal throttling.
> > > >
> > > > I'm not sure how to tell if the frequency is dynamically changed during
> > > > runtime on arm. x86-64 shows this information in /proc/cpuinfo. Here's
> > >
> > > > the /proc/cpuinfo on the Nexus 5:
> > > Nah. /proc/cpuinfo won't show what we need.
> > >
> > > Try the following:
> > >
> > > $ grep "" /sys/devices/system/cpu/cpufreq/policy?/*
> > >
> > > More specifically, the following files have the information you need.
> > > Run watch -n1 on them.
> > >
> > > $ grep "" /sys/devices/system/cpu/cpufreq/policy?/scaling_*_freq
> >
> > There's no cpufreq directory on msm8974:
> >
> >     # ls -1 /sys/devices/system/cpu/
> >     cpu0
> >     cpu1
> >     cpu2
> >     cpu3
> >     cpuidle
> >     hotplug
> >     isolated
> >     kernel_max
> >     modalias
> >     offline
> >     online
> >     possible
> >     power
> >     present
> >     smt
> >     uevent
> >
> > I'm using qcom_defconfig.
> >
> > Brian
>
> Hi Brian,
> cpufreq isn't supported on msm8974 yet.
> I have these patches [0] in my tree but I'm not sure they work correctly, but I haven't tested much with them. Feel free to try them on hammerhead.
>
> Luca
>
> [0] https://github.com/z3ntu/linux/compare/b0917f53ada0e929896a094b451219cd8091366e...6459ca6aff498c9d12acd35709b4903effc4c3f8

Niklas is working on refactoring some of the Krait code[1]. I'm not
sure if he looked at 8974 directly as part of the refactor adding him
here to get a better sense of the state of cpufreq on 8974.

[1] https://lore.kernel.org/linux-arm-msm/20190726080823.xwhxagv5iuhudmic@vireshk-i7/T/#t

Re: [PATCH 00/15] thermal: qcom: tsens: Add interrupt support

[Niklas Cassel]

On Mon, Jul 29, 2019 at 03:20:11PM +0530, Amit Kucheria wrote:
> On Mon, Jul 29, 2019 at 3:03 PM Luca Weiss <luca@z3ntu.xyz> wrote:
> >
> > On Montag, 29. Juli 2019 11:07:35 CEST Brian Masney wrote:
> > > On Sat, Jul 27, 2019 at 12:58:54PM +0530, Amit Kucheria wrote:
> > > > On Fri, Jul 26, 2019 at 4:59 PM Brian Masney <masneyb@onstation.org> wrote:
> > > > > On Fri, Jul 26, 2019 at 04:40:16PM +0530, Amit Kucheria wrote:
> > > > > > How well does cpufreq work on 8974? I haven't looked at it yet but
> > > > > > we'll need it for thermal throttling.
> > > > >
> > > > > I'm not sure how to tell if the frequency is dynamically changed during
> > > > > runtime on arm. x86-64 shows this information in /proc/cpuinfo. Here's
> > > >
> > > > > the /proc/cpuinfo on the Nexus 5:
> > > > Nah. /proc/cpuinfo won't show what we need.
> > > >
> > > > Try the following:
> > > >
> > > > $ grep "" /sys/devices/system/cpu/cpufreq/policy?/*
> > > >
> > > > More specifically, the following files have the information you need.
> > > > Run watch -n1 on them.
> > > >
> > > > $ grep "" /sys/devices/system/cpu/cpufreq/policy?/scaling_*_freq
> > >
> > > There's no cpufreq directory on msm8974:
> > >
> > >     # ls -1 /sys/devices/system/cpu/
> > >     cpu0
> > >     cpu1
> > >     cpu2
> > >     cpu3
> > >     cpuidle
> > >     hotplug
> > >     isolated
> > >     kernel_max
> > >     modalias
> > >     offline
> > >     online
> > >     possible
> > >     power
> > >     present
> > >     smt
> > >     uevent
> > >
> > > I'm using qcom_defconfig.
> > >
> > > Brian
> >
> > Hi Brian,
> > cpufreq isn't supported on msm8974 yet.
> > I have these patches [0] in my tree but I'm not sure they work correctly, but I haven't tested much with them. Feel free to try them on hammerhead.
> >
> > Luca
> >
> > [0] https://github.com/z3ntu/linux/compare/b0917f53ada0e929896a094b451219cd8091366e...6459ca6aff498c9d12acd35709b4903effc4c3f8
> 
> Niklas is working on refactoring some of the Krait code[1]. I'm not
> sure if he looked at 8974 directly as part of the refactor adding him
> here to get a better sense of the state of cpufreq on 8974.

Hello,

I took and cleaned up Sricharans commit
"cpufreq: qcom: Re-organise kryo cpufreq to use it for other nvmem based qcom socs"
from his Krait cpufreq series.

The commit renames and refactors the Kryo cpufreq driver.

This commit is now in linux-next:
https://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git/commit/?h=cpufreq/arm/linux-next&id=106b976debd36b0e61847769f8edd71bfea56ed7


I also added Qualcomm A53 support to this driver.

However, Krait CPUs are different from both Kryo and Qualcomm A53,
so you will need to take Sricharans patch series and rebase it
on top of linux-next.

Kind regards,
Niklas

> 
> [1] https://lore.kernel.org/linux-arm-msm/20190726080823.xwhxagv5iuhudmic@vireshk-i7/T/#t

Re: [PATCH 00/15] thermal: qcom: tsens: Add interrupt support

[Amit Kucheria]

On Fri, Jul 26, 2019 at 3:48 AM Amit Kucheria <amit.kucheria@linaro.org> wrote:
>
> Add interrupt support to TSENS. The first 6 patches are general fixes and
> cleanups to the driver before interrupt support is introduced.
>
> This series has been developed against qcs404 and sdm845 and then tested on
> msm8916. Testing on msm8998 and msm8974 would be appreciated since I don't
> have hardware handy. Further, I plan to test on msm8996 and also submit to
> kernelci.

Gentle nudge for reviews. This has now been successfully tested on
8974 (along with 8916, qcs404, sdm845). Testing on msm8998 would be
much appreciated.

> I'm sending this out for more review to get help with testing.
>
> Amit Kucheria (15):
>   drivers: thermal: tsens: Get rid of id field in tsens_sensor
>   drivers: thermal: tsens: Simplify code flow in tsens_probe
>   drivers: thermal: tsens: Add __func__ identifier to debug statements
>   drivers: thermal: tsens: Add debugfs support
>   arm: dts: msm8974: thermal: Add thermal zones for each sensor
>   arm64: dts: msm8916: thermal: Fixup HW ids for cpu sensors
>   dt: thermal: tsens: Document interrupt support in tsens driver
>   arm64: dts: sdm845: thermal: Add interrupt support
>   arm64: dts: msm8996: thermal: Add interrupt support
>   arm64: dts: msm8998: thermal: Add interrupt support
>   arm64: dts: qcs404: thermal: Add interrupt support
>   arm64: dts: msm8974: thermal: Add interrupt support
>   arm64: dts: msm8916: thermal: Add interrupt support
>   drivers: thermal: tsens: Create function to return sign-extended
>     temperature
>   drivers: thermal: tsens: Add interrupt support
>
>  .../bindings/thermal/qcom-tsens.txt           |   5 +
>  arch/arm/boot/dts/qcom-msm8974.dtsi           | 108 +++-
>  arch/arm64/boot/dts/qcom/msm8916.dtsi         |  26 +-
>  arch/arm64/boot/dts/qcom/msm8996.dtsi         |  60 +-
>  arch/arm64/boot/dts/qcom/msm8998.dtsi         |  82 +--
>  arch/arm64/boot/dts/qcom/qcs404.dtsi          |  42 +-
>  arch/arm64/boot/dts/qcom/sdm845.dtsi          |  88 +--
>  drivers/thermal/qcom/tsens-8960.c             |   4 +-
>  drivers/thermal/qcom/tsens-common.c           | 610 +++++++++++++++++-
>  drivers/thermal/qcom/tsens-v0_1.c             |  11 +
>  drivers/thermal/qcom/tsens-v1.c               |  29 +
>  drivers/thermal/qcom/tsens-v2.c               |  18 +
>  drivers/thermal/qcom/tsens.c                  |  52 +-
>  drivers/thermal/qcom/tsens.h                  | 285 +++++++-
>  14 files changed, 1214 insertions(+), 206 deletions(-)
>
> --
> 2.17.1
>