linux-pm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 00/15] thermal: qcom: tsens: Add interrupt support
@ 2019-07-25 22:18 Amit Kucheria
  2019-07-25 22:18 ` [PATCH 01/15] drivers: thermal: tsens: Get rid of id field in tsens_sensor Amit Kucheria
                   ` (8 more replies)
  0 siblings, 9 replies; 42+ messages in thread
From: Amit Kucheria @ 2019-07-25 22:18 UTC (permalink / raw)
  To: linux-kernel, linux-arm-msm, bjorn.andersson, edubezval,
	andy.gross, Andy Gross, Daniel Lezcano, Mark Rutland,
	Rob Herring, Zhang Rui
  Cc: marc.w.gonzalez, masneyb, devicetree, linux-pm

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


^ permalink raw reply	[flat|nested] 42+ messages in thread

* [PATCH 01/15] drivers: thermal: tsens: Get rid of id field in tsens_sensor
  2019-07-25 22:18 [PATCH 00/15] thermal: qcom: tsens: Add interrupt support Amit Kucheria
@ 2019-07-25 22:18 ` Amit Kucheria
  2019-08-17  4:01   ` Stephen Boyd
  2019-08-19 11:55   ` Daniel Lezcano
  2019-07-25 22:18 ` [PATCH 02/15] drivers: thermal: tsens: Simplify code flow in tsens_probe Amit Kucheria
                   ` (7 subsequent siblings)
  8 siblings, 2 replies; 42+ messages in thread
From: Amit Kucheria @ 2019-07-25 22:18 UTC (permalink / raw)
  To: linux-kernel, linux-arm-msm, bjorn.andersson, edubezval,
	andy.gross, Andy Gross, Daniel Lezcano, Mark Rutland,
	Rob Herring, Zhang Rui
  Cc: linux-pm

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


^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [PATCH 02/15] drivers: thermal: tsens: Simplify code flow in tsens_probe
  2019-07-25 22:18 [PATCH 00/15] thermal: qcom: tsens: Add interrupt support Amit Kucheria
  2019-07-25 22:18 ` [PATCH 01/15] drivers: thermal: tsens: Get rid of id field in tsens_sensor Amit Kucheria
@ 2019-07-25 22:18 ` Amit Kucheria
  2019-08-17  4:02   ` Stephen Boyd
  2019-08-19 11:57   ` Daniel Lezcano
  2019-07-25 22:18 ` [PATCH 03/15] drivers: thermal: tsens: Add __func__ identifier to debug statements Amit Kucheria
                   ` (6 subsequent siblings)
  8 siblings, 2 replies; 42+ messages in thread
From: Amit Kucheria @ 2019-07-25 22:18 UTC (permalink / raw)
  To: linux-kernel, linux-arm-msm, bjorn.andersson, edubezval,
	andy.gross, Andy Gross, Daniel Lezcano, Mark Rutland,
	Rob Herring, Zhang Rui
  Cc: linux-pm

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


^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [PATCH 03/15] drivers: thermal: tsens: Add __func__ identifier to debug statements
  2019-07-25 22:18 [PATCH 00/15] thermal: qcom: tsens: Add interrupt support Amit Kucheria
  2019-07-25 22:18 ` [PATCH 01/15] drivers: thermal: tsens: Get rid of id field in tsens_sensor Amit Kucheria
  2019-07-25 22:18 ` [PATCH 02/15] drivers: thermal: tsens: Simplify code flow in tsens_probe Amit Kucheria
@ 2019-07-25 22:18 ` Amit Kucheria
  2019-08-17  4:03   ` Stephen Boyd
  2019-08-19 13:19   ` Daniel Lezcano
  2019-07-25 22:18 ` [PATCH 04/15] drivers: thermal: tsens: Add debugfs support Amit Kucheria
                   ` (5 subsequent siblings)
  8 siblings, 2 replies; 42+ messages in thread
From: Amit Kucheria @ 2019-07-25 22:18 UTC (permalink / raw)
  To: linux-kernel, linux-arm-msm, bjorn.andersson, edubezval,
	andy.gross, Andy Gross, Daniel Lezcano, Mark Rutland,
	Rob Herring, Zhang Rui
  Cc: linux-pm

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


^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [PATCH 04/15] drivers: thermal: tsens: Add debugfs support
  2019-07-25 22:18 [PATCH 00/15] thermal: qcom: tsens: Add interrupt support Amit Kucheria
                   ` (2 preceding siblings ...)
  2019-07-25 22:18 ` [PATCH 03/15] drivers: thermal: tsens: Add __func__ identifier to debug statements Amit Kucheria
@ 2019-07-25 22:18 ` Amit Kucheria
  2019-08-17  4:07   ` Stephen Boyd
  2019-07-25 22:18 ` [PATCH 07/15] dt: thermal: tsens: Document interrupt support in tsens driver Amit Kucheria
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 42+ messages in thread
From: Amit Kucheria @ 2019-07-25 22:18 UTC (permalink / raw)
  To: linux-kernel, linux-arm-msm, bjorn.andersson, edubezval,
	andy.gross, Andy Gross, Daniel Lezcano, Mark Rutland,
	Rob Herring, Zhang Rui
  Cc: linux-pm

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


^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [PATCH 07/15] dt: thermal: tsens: Document interrupt support in tsens driver
  2019-07-25 22:18 [PATCH 00/15] thermal: qcom: tsens: Add interrupt support Amit Kucheria
                   ` (3 preceding siblings ...)
  2019-07-25 22:18 ` [PATCH 04/15] drivers: thermal: tsens: Add debugfs support Amit Kucheria
@ 2019-07-25 22:18 ` Amit Kucheria
  2019-08-16 21:36   ` Rob Herring
  2019-07-25 22:18 ` [PATCH 14/15] drivers: thermal: tsens: Create function to return sign-extended temperature Amit Kucheria
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 42+ messages in thread
From: Amit Kucheria @ 2019-07-25 22:18 UTC (permalink / raw)
  To: linux-kernel, linux-arm-msm, bjorn.andersson, edubezval,
	andy.gross, Andy Gross, Daniel Lezcano, Mark Rutland,
	Rob Herring, Zhang Rui
  Cc: linux-pm, devicetree

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


^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [PATCH 14/15] drivers: thermal: tsens: Create function to return sign-extended temperature
  2019-07-25 22:18 [PATCH 00/15] thermal: qcom: tsens: Add interrupt support Amit Kucheria
                   ` (4 preceding siblings ...)
  2019-07-25 22:18 ` [PATCH 07/15] dt: thermal: tsens: Document interrupt support in tsens driver Amit Kucheria
@ 2019-07-25 22:18 ` Amit Kucheria
  2019-08-17  4:16   ` Stephen Boyd
  2019-07-25 22:18 ` [PATCH 15/15] drivers: thermal: tsens: Add interrupt support Amit Kucheria
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 42+ messages in thread
From: Amit Kucheria @ 2019-07-25 22:18 UTC (permalink / raw)
  To: linux-kernel, linux-arm-msm, bjorn.andersson, edubezval,
	andy.gross, Andy Gross, Daniel Lezcano, Mark Rutland,
	Rob Herring, Zhang Rui
  Cc: linux-pm

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


^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [PATCH 15/15] drivers: thermal: tsens: Add interrupt support
  2019-07-25 22:18 [PATCH 00/15] thermal: qcom: tsens: Add interrupt support Amit Kucheria
                   ` (5 preceding siblings ...)
  2019-07-25 22:18 ` [PATCH 14/15] drivers: thermal: tsens: Create function to return sign-extended temperature Amit Kucheria
@ 2019-07-25 22:18 ` Amit Kucheria
  2019-08-17  6:09   ` Stephen Boyd
  2019-07-26 10:36 ` [PATCH 00/15] thermal: qcom: " Brian Masney
  2019-08-08 13:04 ` Amit Kucheria
  8 siblings, 1 reply; 42+ messages in thread
From: Amit Kucheria @ 2019-07-25 22:18 UTC (permalink / raw)
  To: linux-kernel, linux-arm-msm, bjorn.andersson, edubezval,
	andy.gross, Andy Gross, Daniel Lezcano, Mark Rutland,
	Rob Herring, Zhang Rui
  Cc: linux-pm

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


^ permalink raw reply related	[flat|nested] 42+ messages in thread

* Re: [PATCH 00/15] thermal: qcom: tsens: Add interrupt support
  2019-07-25 22:18 [PATCH 00/15] thermal: qcom: tsens: Add interrupt support Amit Kucheria
                   ` (6 preceding siblings ...)
  2019-07-25 22:18 ` [PATCH 15/15] drivers: thermal: tsens: Add interrupt support Amit Kucheria
@ 2019-07-26 10:36 ` Brian Masney
  2019-07-26 11:10   ` Amit Kucheria
  2019-08-08 13:04 ` Amit Kucheria
  8 siblings, 1 reply; 42+ messages in thread
From: Brian Masney @ 2019-07-26 10:36 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: linux-kernel, linux-arm-msm, bjorn.andersson, edubezval,
	andy.gross, Andy Gross, Daniel Lezcano, Mark Rutland,
	Rob Herring, Zhang Rui, marc.w.gonzalez, devicetree, linux-pm

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 00/15] thermal: qcom: tsens: Add interrupt support
  2019-07-26 10:36 ` [PATCH 00/15] thermal: qcom: " Brian Masney
@ 2019-07-26 11:10   ` Amit Kucheria
  2019-07-26 11:29     ` Brian Masney
  0 siblings, 1 reply; 42+ messages in thread
From: Amit Kucheria @ 2019-07-26 11:10 UTC (permalink / raw)
  To: Brian Masney
  Cc: LKML, linux-arm-msm, Bjorn Andersson, Eduardo Valentin,
	Andy Gross, Andy Gross, Daniel Lezcano, Mark Rutland,
	Rob Herring, Zhang Rui, Marc Gonzalez,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux PM list

(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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 00/15] thermal: qcom: tsens: Add interrupt support
  2019-07-26 11:10   ` Amit Kucheria
@ 2019-07-26 11:29     ` Brian Masney
  2019-07-27  7:28       ` Amit Kucheria
  0 siblings, 1 reply; 42+ messages in thread
From: Brian Masney @ 2019-07-26 11:29 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: LKML, linux-arm-msm, Bjorn Andersson, Eduardo Valentin,
	Andy Gross, Andy Gross, Daniel Lezcano, Mark Rutland,
	Rob Herring, Zhang Rui, Marc Gonzalez,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux PM list

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 00/15] thermal: qcom: tsens: Add interrupt support
  2019-07-26 11:29     ` Brian Masney
@ 2019-07-27  7:28       ` Amit Kucheria
  2019-07-29  9:07         ` Brian Masney
  0 siblings, 1 reply; 42+ messages in thread
From: Amit Kucheria @ 2019-07-27  7:28 UTC (permalink / raw)
  To: Brian Masney
  Cc: LKML, linux-arm-msm, Bjorn Andersson, Eduardo Valentin,
	Andy Gross, Andy Gross, Daniel Lezcano, Mark Rutland,
	Rob Herring, Zhang Rui, Marc Gonzalez,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux PM list

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 00/15] thermal: qcom: tsens: Add interrupt support
  2019-07-27  7:28       ` Amit Kucheria
@ 2019-07-29  9:07         ` Brian Masney
  2019-07-29  9:32           ` Luca Weiss
  0 siblings, 1 reply; 42+ messages in thread
From: Brian Masney @ 2019-07-29  9:07 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: LKML, linux-arm-msm, Bjorn Andersson, Eduardo Valentin,
	Andy Gross, Andy Gross, Daniel Lezcano, Mark Rutland,
	Rob Herring, Zhang Rui, Marc Gonzalez,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux PM list

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 00/15] thermal: qcom: tsens: Add interrupt support
  2019-07-29  9:07         ` Brian Masney
@ 2019-07-29  9:32           ` Luca Weiss
  2019-07-29  9:50             ` Amit Kucheria
  0 siblings, 1 reply; 42+ messages in thread
From: Luca Weiss @ 2019-07-29  9:32 UTC (permalink / raw)
  To: linux-kernel
  Cc: Brian Masney, Amit Kucheria, linux-arm-msm, Bjorn Andersson,
	Eduardo Valentin, Andy Gross, Andy Gross, Daniel Lezcano,
	Mark Rutland, Rob Herring, Zhang Rui, Marc Gonzalez,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux PM list

[-- 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 --]

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 00/15] thermal: qcom: tsens: Add interrupt support
  2019-07-29  9:32           ` Luca Weiss
@ 2019-07-29  9:50             ` Amit Kucheria
  2019-08-12 15:28               ` Niklas Cassel
  0 siblings, 1 reply; 42+ messages in thread
From: Amit Kucheria @ 2019-07-29  9:50 UTC (permalink / raw)
  To: Luca Weiss, Niklas Cassel
  Cc: LKML, Brian Masney, linux-arm-msm, Bjorn Andersson,
	Eduardo Valentin, Andy Gross, Andy Gross, Daniel Lezcano,
	Mark Rutland, Rob Herring, Zhang Rui, Marc Gonzalez,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux PM list

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 00/15] thermal: qcom: tsens: Add interrupt support
  2019-07-25 22:18 [PATCH 00/15] thermal: qcom: tsens: Add interrupt support Amit Kucheria
                   ` (7 preceding siblings ...)
  2019-07-26 10:36 ` [PATCH 00/15] thermal: qcom: " Brian Masney
@ 2019-08-08 13:04 ` Amit Kucheria
  8 siblings, 0 replies; 42+ messages in thread
From: Amit Kucheria @ 2019-08-08 13:04 UTC (permalink / raw)
  To: LKML, linux-arm-msm, Bjorn Andersson, Eduardo Valentin,
	Andy Gross, Daniel Lezcano, Mark Rutland, Rob Herring, Zhang Rui,
	sivaa
  Cc: Marc Gonzalez, Brian Masney,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux PM list

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
>

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 00/15] thermal: qcom: tsens: Add interrupt support
  2019-07-29  9:50             ` Amit Kucheria
@ 2019-08-12 15:28               ` Niklas Cassel
  0 siblings, 0 replies; 42+ messages in thread
From: Niklas Cassel @ 2019-08-12 15:28 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: Luca Weiss, LKML, Brian Masney, linux-arm-msm, Bjorn Andersson,
	Eduardo Valentin, Andy Gross, Andy Gross, Daniel Lezcano,
	Mark Rutland, Rob Herring, Zhang Rui, Marc Gonzalez,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux PM list

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 07/15] dt: thermal: tsens: Document interrupt support in tsens driver
  2019-07-25 22:18 ` [PATCH 07/15] dt: thermal: tsens: Document interrupt support in tsens driver Amit Kucheria
@ 2019-08-16 21:36   ` Rob Herring
  2019-08-16 22:02     ` Amit Kucheria
  0 siblings, 1 reply; 42+ messages in thread
From: Rob Herring @ 2019-08-16 21:36 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: linux-kernel, linux-arm-msm, bjorn.andersson, edubezval,
	andy.gross, Andy Gross, Daniel Lezcano, Mark Rutland, Zhang Rui,
	linux-pm, devicetree

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
> 

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 07/15] dt: thermal: tsens: Document interrupt support in tsens driver
  2019-08-16 21:36   ` Rob Herring
@ 2019-08-16 22:02     ` Amit Kucheria
  2019-08-17  4:10       ` Stephen Boyd
  2019-08-17 19:25       ` Rob Herring
  0 siblings, 2 replies; 42+ messages in thread
From: Amit Kucheria @ 2019-08-16 22:02 UTC (permalink / raw)
  To: Rob Herring
  Cc: Linux Kernel Mailing List, linux-arm-msm, Bjorn Andersson,
	Eduardo Valentin, Andy Gross, Andy Gross, Daniel Lezcano,
	Mark Rutland, Zhang Rui, Linux PM list, DTML

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 01/15] drivers: thermal: tsens: Get rid of id field in tsens_sensor
  2019-07-25 22:18 ` [PATCH 01/15] drivers: thermal: tsens: Get rid of id field in tsens_sensor Amit Kucheria
@ 2019-08-17  4:01   ` Stephen Boyd
  2019-08-19 11:55   ` Daniel Lezcano
  1 sibling, 0 replies; 42+ messages in thread
From: Stephen Boyd @ 2019-08-17  4:01 UTC (permalink / raw)
  To: Amit Kucheria, Andy Gross, Daniel Lezcano, Mark Rutland,
	Rob Herring, Zhang Rui, andy.gross, bjorn.andersson, edubezval,
	linux-arm-msm, linux-kernel
  Cc: linux-pm

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>


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 02/15] drivers: thermal: tsens: Simplify code flow in tsens_probe
  2019-07-25 22:18 ` [PATCH 02/15] drivers: thermal: tsens: Simplify code flow in tsens_probe Amit Kucheria
@ 2019-08-17  4:02   ` Stephen Boyd
  2019-08-19 11:57   ` Daniel Lezcano
  1 sibling, 0 replies; 42+ messages in thread
From: Stephen Boyd @ 2019-08-17  4:02 UTC (permalink / raw)
  To: Amit Kucheria, Andy Gross, Daniel Lezcano, Mark Rutland,
	Rob Herring, Zhang Rui, andy.gross, bjorn.andersson, edubezval,
	linux-arm-msm, linux-kernel
  Cc: linux-pm

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>


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 03/15] drivers: thermal: tsens: Add __func__ identifier to debug statements
  2019-07-25 22:18 ` [PATCH 03/15] drivers: thermal: tsens: Add __func__ identifier to debug statements Amit Kucheria
@ 2019-08-17  4:03   ` Stephen Boyd
  2019-08-19 13:19   ` Daniel Lezcano
  1 sibling, 0 replies; 42+ messages in thread
From: Stephen Boyd @ 2019-08-17  4:03 UTC (permalink / raw)
  To: Amit Kucheria, Andy Gross, Daniel Lezcano, Mark Rutland,
	Rob Herring, Zhang Rui, andy.gross, bjorn.andersson, edubezval,
	linux-arm-msm, linux-kernel
  Cc: linux-pm

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>


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 04/15] drivers: thermal: tsens: Add debugfs support
  2019-07-25 22:18 ` [PATCH 04/15] drivers: thermal: tsens: Add debugfs support Amit Kucheria
@ 2019-08-17  4:07   ` Stephen Boyd
  2019-08-19  7:58     ` Amit Kucheria
  0 siblings, 1 reply; 42+ messages in thread
From: Stephen Boyd @ 2019-08-17  4:07 UTC (permalink / raw)
  To: Amit Kucheria, Andy Gross, Daniel Lezcano, Mark Rutland,
	Rob Herring, Zhang Rui, andy.gross, bjorn.andersson, edubezval,
	linux-arm-msm, linux-kernel
  Cc: linux-pm

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;
> +}

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 07/15] dt: thermal: tsens: Document interrupt support in tsens driver
  2019-08-16 22:02     ` Amit Kucheria
@ 2019-08-17  4:10       ` Stephen Boyd
  2019-08-17 19:25       ` Rob Herring
  1 sibling, 0 replies; 42+ messages in thread
From: Stephen Boyd @ 2019-08-17  4:10 UTC (permalink / raw)
  To: Amit Kucheria, Rob Herring
  Cc: Linux Kernel Mailing List, linux-arm-msm, Bjorn Andersson,
	Eduardo Valentin, Andy Gross, Andy Gross, Daniel Lezcano,
	Mark Rutland, Zhang Rui, Linux PM list, DTML

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.


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 14/15] drivers: thermal: tsens: Create function to return sign-extended temperature
  2019-07-25 22:18 ` [PATCH 14/15] drivers: thermal: tsens: Create function to return sign-extended temperature Amit Kucheria
@ 2019-08-17  4:16   ` Stephen Boyd
  2019-08-19 20:51     ` Amit Kucheria
  0 siblings, 1 reply; 42+ messages in thread
From: Stephen Boyd @ 2019-08-17  4:16 UTC (permalink / raw)
  To: Amit Kucheria, Andy Gross, Daniel Lezcano, Mark Rutland,
	Rob Herring, Zhang Rui, andy.gross, bjorn.andersson, edubezval,
	linux-arm-msm, linux-kernel
  Cc: linux-pm

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 15/15] drivers: thermal: tsens: Add interrupt support
  2019-07-25 22:18 ` [PATCH 15/15] drivers: thermal: tsens: Add interrupt support Amit Kucheria
@ 2019-08-17  6:09   ` Stephen Boyd
  2019-08-22 13:40     ` Amit Kucheria
  0 siblings, 1 reply; 42+ messages in thread
From: Stephen Boyd @ 2019-08-17  6:09 UTC (permalink / raw)
  To: Amit Kucheria, Andy Gross, Daniel Lezcano, Mark Rutland,
	Rob Herring, Zhang Rui, andy.gross, bjorn.andersson, edubezval,
	linux-arm-msm, linux-kernel
  Cc: linux-pm

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,

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 07/15] dt: thermal: tsens: Document interrupt support in tsens driver
  2019-08-16 22:02     ` Amit Kucheria
  2019-08-17  4:10       ` Stephen Boyd
@ 2019-08-17 19:25       ` Rob Herring
  2019-08-19  7:10         ` Amit Kucheria
  1 sibling, 1 reply; 42+ messages in thread
From: Rob Herring @ 2019-08-17 19:25 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: Linux Kernel Mailing List, linux-arm-msm, Bjorn Andersson,
	Eduardo Valentin, Andy Gross, Andy Gross, Daniel Lezcano,
	Mark Rutland, Zhang Rui, Linux PM list, DTML

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 07/15] dt: thermal: tsens: Document interrupt support in tsens driver
  2019-08-17 19:25       ` Rob Herring
@ 2019-08-19  7:10         ` Amit Kucheria
  2019-08-19 13:07           ` Rob Herring
  0 siblings, 1 reply; 42+ messages in thread
From: Amit Kucheria @ 2019-08-19  7:10 UTC (permalink / raw)
  To: Rob Herring
  Cc: Linux Kernel Mailing List, linux-arm-msm, Bjorn Andersson,
	Eduardo Valentin, Andy Gross, Andy Gross, Daniel Lezcano,
	Mark Rutland, Zhang Rui, Linux PM list, DTML

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 04/15] drivers: thermal: tsens: Add debugfs support
  2019-08-17  4:07   ` Stephen Boyd
@ 2019-08-19  7:58     ` Amit Kucheria
  2019-08-19 14:27       ` Stephen Boyd
  0 siblings, 1 reply; 42+ messages in thread
From: Amit Kucheria @ 2019-08-19  7:58 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Andy Gross, Daniel Lezcano, Mark Rutland, Rob Herring, Zhang Rui,
	Andy Gross, Bjorn Andersson, Eduardo Valentin, linux-arm-msm,
	LKML, Linux PM list

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;
> > +}

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 01/15] drivers: thermal: tsens: Get rid of id field in tsens_sensor
  2019-07-25 22:18 ` [PATCH 01/15] drivers: thermal: tsens: Get rid of id field in tsens_sensor Amit Kucheria
  2019-08-17  4:01   ` Stephen Boyd
@ 2019-08-19 11:55   ` Daniel Lezcano
  1 sibling, 0 replies; 42+ messages in thread
From: Daniel Lezcano @ 2019-08-19 11:55 UTC (permalink / raw)
  To: Amit Kucheria, linux-kernel, linux-arm-msm, bjorn.andersson,
	edubezval, andy.gross, Andy Gross, Mark Rutland, Rob Herring,
	Zhang Rui
  Cc: linux-pm

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


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 02/15] drivers: thermal: tsens: Simplify code flow in tsens_probe
  2019-07-25 22:18 ` [PATCH 02/15] drivers: thermal: tsens: Simplify code flow in tsens_probe Amit Kucheria
  2019-08-17  4:02   ` Stephen Boyd
@ 2019-08-19 11:57   ` Daniel Lezcano
  1 sibling, 0 replies; 42+ messages in thread
From: Daniel Lezcano @ 2019-08-19 11:57 UTC (permalink / raw)
  To: Amit Kucheria, linux-kernel, linux-arm-msm, bjorn.andersson,
	edubezval, andy.gross, Andy Gross, Mark Rutland, Rob Herring,
	Zhang Rui
  Cc: linux-pm

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


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 07/15] dt: thermal: tsens: Document interrupt support in tsens driver
  2019-08-19  7:10         ` Amit Kucheria
@ 2019-08-19 13:07           ` Rob Herring
  0 siblings, 0 replies; 42+ messages in thread
From: Rob Herring @ 2019-08-19 13:07 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: Linux Kernel Mailing List, linux-arm-msm, Bjorn Andersson,
	Eduardo Valentin, Andy Gross, Andy Gross, Daniel Lezcano,
	Mark Rutland, Zhang Rui, Linux PM list, DTML

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 03/15] drivers: thermal: tsens: Add __func__ identifier to debug statements
  2019-07-25 22:18 ` [PATCH 03/15] drivers: thermal: tsens: Add __func__ identifier to debug statements Amit Kucheria
  2019-08-17  4:03   ` Stephen Boyd
@ 2019-08-19 13:19   ` Daniel Lezcano
  1 sibling, 0 replies; 42+ messages in thread
From: Daniel Lezcano @ 2019-08-19 13:19 UTC (permalink / raw)
  To: Amit Kucheria, linux-kernel, linux-arm-msm, bjorn.andersson,
	edubezval, andy.gross, Andy Gross, Mark Rutland, Rob Herring,
	Zhang Rui
  Cc: linux-pm

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


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 04/15] drivers: thermal: tsens: Add debugfs support
  2019-08-19  7:58     ` Amit Kucheria
@ 2019-08-19 14:27       ` Stephen Boyd
  2019-08-21 12:55         ` Amit Kucheria
  0 siblings, 1 reply; 42+ messages in thread
From: Stephen Boyd @ 2019-08-19 14:27 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: Andy Gross, Daniel Lezcano, Mark Rutland, Rob Herring, Zhang Rui,
	Andy Gross, Bjorn Andersson, Eduardo Valentin, linux-arm-msm,
	LKML, Linux PM list

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;
> > > +

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 14/15] drivers: thermal: tsens: Create function to return sign-extended temperature
  2019-08-17  4:16   ` Stephen Boyd
@ 2019-08-19 20:51     ` Amit Kucheria
  0 siblings, 0 replies; 42+ messages in thread
From: Amit Kucheria @ 2019-08-19 20:51 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Andy Gross, Daniel Lezcano, Mark Rutland, Rob Herring, Zhang Rui,
	Andy Gross, Bjorn Andersson, Eduardo Valentin, linux-arm-msm,
	LKML, Linux PM list

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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 04/15] drivers: thermal: tsens: Add debugfs support
  2019-08-19 14:27       ` Stephen Boyd
@ 2019-08-21 12:55         ` Amit Kucheria
  2019-08-26 20:55           ` Stephen Boyd
  0 siblings, 1 reply; 42+ messages in thread
From: Amit Kucheria @ 2019-08-21 12:55 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Andy Gross, Daniel Lezcano, Mark Rutland, Rob Herring, Zhang Rui,
	Andy Gross, Bjorn Andersson, Eduardo Valentin, linux-arm-msm,
	LKML, Linux PM list

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;
> > > > +

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 15/15] drivers: thermal: tsens: Add interrupt support
  2019-08-17  6:09   ` Stephen Boyd
@ 2019-08-22 13:40     ` Amit Kucheria
  0 siblings, 0 replies; 42+ messages in thread
From: Amit Kucheria @ 2019-08-22 13:40 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Andy Gross, Daniel Lezcano, Mark Rutland, Rob Herring, Zhang Rui,
	Andy Gross, Bjorn Andersson, Eduardo Valentin, linux-arm-msm,
	Linux Kernel Mailing List, Linux PM list

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,

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 04/15] drivers: thermal: tsens: Add debugfs support
  2019-08-21 12:55         ` Amit Kucheria
@ 2019-08-26 20:55           ` Stephen Boyd
  0 siblings, 0 replies; 42+ messages in thread
From: Stephen Boyd @ 2019-08-26 20:55 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: Andy Gross, Daniel Lezcano, Mark Rutland, Rob Herring, Zhang Rui,
	Andy Gross, Bjorn Andersson, Eduardo Valentin, linux-arm-msm,
	LKML, Linux PM list

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.


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 00/15] thermal: qcom: tsens: Add interrupt support
  2019-10-16  8:35   ` Amit Kucheria
@ 2019-10-16  8:46     ` Daniel Lezcano
  0 siblings, 0 replies; 42+ messages in thread
From: Daniel Lezcano @ 2019-10-16  8:46 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: Linux Kernel Mailing List, linux-arm-msm, Bjorn Andersson,
	Eduardo Valentin, Andy Gross, Brian Masney, Stephen Boyd,
	Amit Kucheria, Mark Rutland, Rob Herring, Zhang Rui, DTML,
	Linux PM list

On 16/10/2019 10:35, Amit Kucheria wrote:
> On Wed, Oct 16, 2019 at 1:29 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 16/10/2019 09:33, Amit Kucheria wrote:
>>> Hi Thermal and MSM maintainers,
>>>
>>> I believe this series is now ready to be merged. The DT bindings and driver
>>> changes should go through the thermal tree and the changes to the DT files
>>> themselves should go through the MSM tree. There is no hard ordering
>>> dependency because we're adding a new property to the driver. It would help
>>> to soak in linux-next for a few weeks to catch anything on kernelci.org.
>>
>> So the ones going to thermal are:
>>
>> 1-7, 14, 15 right ?
> 
> 1-4, 7, 14, 15 => thermal tree
> 5, 6, 8-13 => msm tree
> 
> I guess I could have ordered it better for merging :-/

The patches are applied to:

 https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/log/?h=testing

If everything is fine, they will be moved to linux-next.

Thanks

  -- Daniel


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


^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 00/15] thermal: qcom: tsens: Add interrupt support
  2019-10-16  7:59 ` Daniel Lezcano
@ 2019-10-16  8:35   ` Amit Kucheria
  2019-10-16  8:46     ` Daniel Lezcano
  0 siblings, 1 reply; 42+ messages in thread
From: Amit Kucheria @ 2019-10-16  8:35 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: Linux Kernel Mailing List, linux-arm-msm, Bjorn Andersson,
	Eduardo Valentin, Andy Gross, Brian Masney, Stephen Boyd,
	Amit Kucheria, Mark Rutland, Rob Herring, Zhang Rui, DTML,
	Linux PM list

On Wed, Oct 16, 2019 at 1:29 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 16/10/2019 09:33, Amit Kucheria wrote:
> > Hi Thermal and MSM maintainers,
> >
> > I believe this series is now ready to be merged. The DT bindings and driver
> > changes should go through the thermal tree and the changes to the DT files
> > themselves should go through the MSM tree. There is no hard ordering
> > dependency because we're adding a new property to the driver. It would help
> > to soak in linux-next for a few weeks to catch anything on kernelci.org.
>
> So the ones going to thermal are:
>
> 1-7, 14, 15 right ?

1-4, 7, 14, 15 => thermal tree
5, 6, 8-13 => msm tree

I guess I could have ordered it better for merging :-/

> > Changes since v4:
> > - Change to of-thermal core[1] to force interrupts w/o changing polling-delay DT
> >   parameter
> > - Corresponding changes to DT files to remove the hunks setting the values
> >   to 0
> > - Collected reviews and acks
> >
> > Changes since v3:
> > - Fix up the YAML definitions based on Rob's review
> >
> > Changes since v2:
> > - Addressed Stephen's review comment
> > - Moved the dt-bindings to yaml (This throws up some new warnings in various QCOM
> > devicetrees. I'll send out a separate series to fix them up)
> > - Collected reviews and acks
> > - Added the dt-bindings to MAINTAINERS
> >
> > Changes since v1:
> > - Collected reviews and acks
> > - Addressed Stephen's review comments (hopefully I got them all).
> > - Completely removed critical interrupt infrastructure from this series.
> >   Will post that separately.
> > - Fixed a bug in sign-extension of temperature.
> > - Fixed DT bindings to use the name of the interrupt e.g. "uplow" and use
> >   platform_get_irq_byname().
> >
> > Add interrupt support to TSENS. The first 6 patches are general fixes and
> > cleanups to the driver before interrupt support is introduced.
> >
> > [1] https://lore.kernel.org/linux-arm-msm/1b53ef537203e629328285b4597a09e4a586d688.1571181041.git.amit.kucheria@linaro.org/
> >
> > 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-bindings: thermal: tsens: Convert over to a yaml schema
> >   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
> >   arm: 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           |  55 --
> >  .../bindings/thermal/qcom-tsens.yaml          | 168 ++++++
> >  MAINTAINERS                                   |   1 +
> >  arch/arm/boot/dts/qcom-msm8974.dtsi           |  92 +++
> >  arch/arm64/boot/dts/qcom/msm8916.dtsi         |   6 +-
> >  arch/arm64/boot/dts/qcom/msm8996.dtsi         |   4 +
> >  arch/arm64/boot/dts/qcom/msm8998.dtsi         |   6 +-
> >  arch/arm64/boot/dts/qcom/qcs404.dtsi          |   2 +
> >  arch/arm64/boot/dts/qcom/sdm845.dtsi          |   4 +
> >  drivers/thermal/qcom/tsens-8960.c             |   4 +-
> >  drivers/thermal/qcom/tsens-common.c           | 529 ++++++++++++++++--
> >  drivers/thermal/qcom/tsens-v0_1.c             |  11 +
> >  drivers/thermal/qcom/tsens-v1.c               |  29 +
> >  drivers/thermal/qcom/tsens-v2.c               |  13 +
> >  drivers/thermal/qcom/tsens.c                  |  58 +-
> >  drivers/thermal/qcom/tsens.h                  | 286 ++++++++--
> >  16 files changed, 1102 insertions(+), 166 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/thermal/qcom-tsens.txt
> >  create mode 100644 Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
> >
>
>
> --
>  <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
>

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 00/15] thermal: qcom: tsens: Add interrupt support
  2019-10-16  7:33 Amit Kucheria
@ 2019-10-16  7:59 ` Daniel Lezcano
  2019-10-16  8:35   ` Amit Kucheria
  0 siblings, 1 reply; 42+ messages in thread
From: Daniel Lezcano @ 2019-10-16  7:59 UTC (permalink / raw)
  To: Amit Kucheria, linux-kernel, linux-arm-msm, bjorn.andersson,
	edubezval, agross, masneyb, swboyd, Amit Kucheria, Mark Rutland,
	Rob Herring, Zhang Rui
  Cc: devicetree, linux-pm

On 16/10/2019 09:33, Amit Kucheria wrote:
> Hi Thermal and MSM maintainers,
> 
> I believe this series is now ready to be merged. The DT bindings and driver
> changes should go through the thermal tree and the changes to the DT files
> themselves should go through the MSM tree. There is no hard ordering
> dependency because we're adding a new property to the driver. It would help
> to soak in linux-next for a few weeks to catch anything on kernelci.org.

So the ones going to thermal are:

1-7, 14, 15 right ?

> Changes since v4:
> - Change to of-thermal core[1] to force interrupts w/o changing polling-delay DT
>   parameter
> - Corresponding changes to DT files to remove the hunks setting the values
>   to 0
> - Collected reviews and acks
> 
> Changes since v3:
> - Fix up the YAML definitions based on Rob's review
> 
> Changes since v2:
> - Addressed Stephen's review comment
> - Moved the dt-bindings to yaml (This throws up some new warnings in various QCOM
> devicetrees. I'll send out a separate series to fix them up)
> - Collected reviews and acks
> - Added the dt-bindings to MAINTAINERS
> 
> Changes since v1:
> - Collected reviews and acks
> - Addressed Stephen's review comments (hopefully I got them all).
> - Completely removed critical interrupt infrastructure from this series.
>   Will post that separately.
> - Fixed a bug in sign-extension of temperature.
> - Fixed DT bindings to use the name of the interrupt e.g. "uplow" and use
>   platform_get_irq_byname().
> 
> Add interrupt support to TSENS. The first 6 patches are general fixes and
> cleanups to the driver before interrupt support is introduced.
> 
> [1] https://lore.kernel.org/linux-arm-msm/1b53ef537203e629328285b4597a09e4a586d688.1571181041.git.amit.kucheria@linaro.org/
> 
> 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-bindings: thermal: tsens: Convert over to a yaml schema
>   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
>   arm: 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           |  55 --
>  .../bindings/thermal/qcom-tsens.yaml          | 168 ++++++
>  MAINTAINERS                                   |   1 +
>  arch/arm/boot/dts/qcom-msm8974.dtsi           |  92 +++
>  arch/arm64/boot/dts/qcom/msm8916.dtsi         |   6 +-
>  arch/arm64/boot/dts/qcom/msm8996.dtsi         |   4 +
>  arch/arm64/boot/dts/qcom/msm8998.dtsi         |   6 +-
>  arch/arm64/boot/dts/qcom/qcs404.dtsi          |   2 +
>  arch/arm64/boot/dts/qcom/sdm845.dtsi          |   4 +
>  drivers/thermal/qcom/tsens-8960.c             |   4 +-
>  drivers/thermal/qcom/tsens-common.c           | 529 ++++++++++++++++--
>  drivers/thermal/qcom/tsens-v0_1.c             |  11 +
>  drivers/thermal/qcom/tsens-v1.c               |  29 +
>  drivers/thermal/qcom/tsens-v2.c               |  13 +
>  drivers/thermal/qcom/tsens.c                  |  58 +-
>  drivers/thermal/qcom/tsens.h                  | 286 ++++++++--
>  16 files changed, 1102 insertions(+), 166 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/thermal/qcom-tsens.txt
>  create mode 100644 Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
> 


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


^ permalink raw reply	[flat|nested] 42+ messages in thread

* [PATCH 00/15] thermal: qcom: tsens: Add interrupt support
@ 2019-10-16  7:33 Amit Kucheria
  2019-10-16  7:59 ` Daniel Lezcano
  0 siblings, 1 reply; 42+ messages in thread
From: Amit Kucheria @ 2019-10-16  7:33 UTC (permalink / raw)
  To: linux-kernel, linux-arm-msm, bjorn.andersson, edubezval, agross,
	masneyb, swboyd, Amit Kucheria, Daniel Lezcano, Mark Rutland,
	Rob Herring, Zhang Rui
  Cc: devicetree, linux-pm

Hi Thermal and MSM maintainers,

I believe this series is now ready to be merged. The DT bindings and driver
changes should go through the thermal tree and the changes to the DT files
themselves should go through the MSM tree. There is no hard ordering
dependency because we're adding a new property to the driver. It would help
to soak in linux-next for a few weeks to catch anything on kernelci.org.

Regards,
Amit

Changes since v4:
- Change to of-thermal core[1] to force interrupts w/o changing polling-delay DT
  parameter
- Corresponding changes to DT files to remove the hunks setting the values
  to 0
- Collected reviews and acks

Changes since v3:
- Fix up the YAML definitions based on Rob's review

Changes since v2:
- Addressed Stephen's review comment
- Moved the dt-bindings to yaml (This throws up some new warnings in various QCOM
devicetrees. I'll send out a separate series to fix them up)
- Collected reviews and acks
- Added the dt-bindings to MAINTAINERS

Changes since v1:
- Collected reviews and acks
- Addressed Stephen's review comments (hopefully I got them all).
- Completely removed critical interrupt infrastructure from this series.
  Will post that separately.
- Fixed a bug in sign-extension of temperature.
- Fixed DT bindings to use the name of the interrupt e.g. "uplow" and use
  platform_get_irq_byname().

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

[1] https://lore.kernel.org/linux-arm-msm/1b53ef537203e629328285b4597a09e4a586d688.1571181041.git.amit.kucheria@linaro.org/

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-bindings: thermal: tsens: Convert over to a yaml schema
  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
  arm: 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           |  55 --
 .../bindings/thermal/qcom-tsens.yaml          | 168 ++++++
 MAINTAINERS                                   |   1 +
 arch/arm/boot/dts/qcom-msm8974.dtsi           |  92 +++
 arch/arm64/boot/dts/qcom/msm8916.dtsi         |   6 +-
 arch/arm64/boot/dts/qcom/msm8996.dtsi         |   4 +
 arch/arm64/boot/dts/qcom/msm8998.dtsi         |   6 +-
 arch/arm64/boot/dts/qcom/qcs404.dtsi          |   2 +
 arch/arm64/boot/dts/qcom/sdm845.dtsi          |   4 +
 drivers/thermal/qcom/tsens-8960.c             |   4 +-
 drivers/thermal/qcom/tsens-common.c           | 529 ++++++++++++++++--
 drivers/thermal/qcom/tsens-v0_1.c             |  11 +
 drivers/thermal/qcom/tsens-v1.c               |  29 +
 drivers/thermal/qcom/tsens-v2.c               |  13 +
 drivers/thermal/qcom/tsens.c                  |  58 +-
 drivers/thermal/qcom/tsens.h                  | 286 ++++++++--
 16 files changed, 1102 insertions(+), 166 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/thermal/qcom-tsens.txt
 create mode 100644 Documentation/devicetree/bindings/thermal/qcom-tsens.yaml

-- 
2.17.1


^ permalink raw reply	[flat|nested] 42+ messages in thread

end of thread, other threads:[~2019-10-16  8:46 UTC | newest]

Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-25 22:18 [PATCH 00/15] thermal: qcom: tsens: Add interrupt support Amit Kucheria
2019-07-25 22:18 ` [PATCH 01/15] drivers: thermal: tsens: Get rid of id field in tsens_sensor Amit Kucheria
2019-08-17  4:01   ` Stephen Boyd
2019-08-19 11:55   ` Daniel Lezcano
2019-07-25 22:18 ` [PATCH 02/15] drivers: thermal: tsens: Simplify code flow in tsens_probe Amit Kucheria
2019-08-17  4:02   ` Stephen Boyd
2019-08-19 11:57   ` Daniel Lezcano
2019-07-25 22:18 ` [PATCH 03/15] drivers: thermal: tsens: Add __func__ identifier to debug statements Amit Kucheria
2019-08-17  4:03   ` Stephen Boyd
2019-08-19 13:19   ` Daniel Lezcano
2019-07-25 22:18 ` [PATCH 04/15] drivers: thermal: tsens: Add debugfs support Amit Kucheria
2019-08-17  4:07   ` Stephen Boyd
2019-08-19  7:58     ` Amit Kucheria
2019-08-19 14:27       ` Stephen Boyd
2019-08-21 12:55         ` Amit Kucheria
2019-08-26 20:55           ` Stephen Boyd
2019-07-25 22:18 ` [PATCH 07/15] dt: thermal: tsens: Document interrupt support in tsens driver Amit Kucheria
2019-08-16 21:36   ` Rob Herring
2019-08-16 22:02     ` Amit Kucheria
2019-08-17  4:10       ` Stephen Boyd
2019-08-17 19:25       ` Rob Herring
2019-08-19  7:10         ` Amit Kucheria
2019-08-19 13:07           ` Rob Herring
2019-07-25 22:18 ` [PATCH 14/15] drivers: thermal: tsens: Create function to return sign-extended temperature Amit Kucheria
2019-08-17  4:16   ` Stephen Boyd
2019-08-19 20:51     ` Amit Kucheria
2019-07-25 22:18 ` [PATCH 15/15] drivers: thermal: tsens: Add interrupt support Amit Kucheria
2019-08-17  6:09   ` Stephen Boyd
2019-08-22 13:40     ` Amit Kucheria
2019-07-26 10:36 ` [PATCH 00/15] thermal: qcom: " Brian Masney
2019-07-26 11:10   ` Amit Kucheria
2019-07-26 11:29     ` Brian Masney
2019-07-27  7:28       ` Amit Kucheria
2019-07-29  9:07         ` Brian Masney
2019-07-29  9:32           ` Luca Weiss
2019-07-29  9:50             ` Amit Kucheria
2019-08-12 15:28               ` Niklas Cassel
2019-08-08 13:04 ` Amit Kucheria
2019-10-16  7:33 Amit Kucheria
2019-10-16  7:59 ` Daniel Lezcano
2019-10-16  8:35   ` Amit Kucheria
2019-10-16  8:46     ` Daniel Lezcano

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).