[PATCH v2 0/5] thermal: tsens: Prepare for version 2 of TSENS IP

[PATCH v2 0/5] thermal: tsens: Prepare for version 2 of TSENS IP

[Amit Kucheria]

This series is a mixed bag: Some code moves to deal with version 2 of the
TSENS IP in common functions, new platform support (sdm845), a cleanup
patch and a DT change to have a common way to deal with the SROT and TM
registers despite slightly different features across the IP family and
different register offsets.

I can merge the tsens-8996.c and tsens-sdm845.c files into a tsens-v2.c if
desired.

Changes since v1:
- Move get_temp() from tsens-8996 to tsens-common and rename
- Change 8996 DT entry to allow init_common() to work across sdm845 and
  8996 due to different offsets

Amit Kucheria (5):
  thermal: tsens: Get rid of unused fields in structure
  dt: qcom: 8996: thermal: Move to DT initialisation
  thermal: tsens: Move 8996 get_temp() to common code for reuse
  thermal: tsens: Add support for SDM845
  thermal: tsens: Check if we have valid data before reading

 .../devicetree/bindings/thermal/qcom-tsens.txt     |  1 +
 arch/arm64/boot/dts/qcom/msm8996.dtsi              | 12 +++-
 drivers/thermal/qcom/Makefile                      |  2 +-
 drivers/thermal/qcom/tsens-8996.c                  | 74 +-------------------
 drivers/thermal/qcom/tsens-common.c                | 78 +++++++++++++++++++---
 drivers/thermal/qcom/tsens-sdm845.c                | 15 +++++
 drivers/thermal/qcom/tsens.c                       |  3 +
 drivers/thermal/qcom/tsens.h                       |  8 ++-
 8 files changed, 107 insertions(+), 86 deletions(-)
 create mode 100644 drivers/thermal/qcom/tsens-sdm845.c

-- 
2.7.4

[PATCH v2 1/5] thermal: tsens: Get rid of unused fields in structure

[Amit Kucheria]

status_field and trdy are unused in any of the tsens drivers. Remove them.

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Acked-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/thermal/qcom/tsens.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
index 911c197..dc56e1e 100644
--- a/drivers/thermal/qcom/tsens.h
+++ b/drivers/thermal/qcom/tsens.h
@@ -77,9 +77,7 @@ struct tsens_device {
 	struct device			*dev;
 	u32				num_sensors;
 	struct regmap			*map;
-	struct regmap_field		*status_field;
 	struct tsens_context		ctx;
-	bool				trdy;
 	const struct tsens_ops		*ops;
 	struct tsens_sensor		sensor[0];
 };
-- 
2.7.4

[PATCH v2 2/5] dt: qcom: 8996: thermal: Move to DT initialisation

[Amit Kucheria]

We also split up the regmap address space into two, one for the TM
registers, the other for the SROT registers. This was required to deal with
different address offsets for the TM and SROT registers across different
SoC families.

Since tsens-common.c/init_common() currently only registers one address space, the order is important (TM before SROT).This is OK since the code doesn't really use the SROT functionality yet.

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
 arch/arm64/boot/dts/qcom/msm8996.dtsi | 12 +++++++++++-
 drivers/thermal/qcom/tsens-8996.c     |  1 -
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index 410ae78..b4aab18 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -451,7 +451,17 @@
 
 		tsens0: thermal-sensor@4a8000 {
 			compatible = "qcom,msm8996-tsens";
-			reg = <0x4a8000 0x2000>;
+			reg = <0x4a9000 0x1000>, /* TM */
+			      <0x4a8000 0x1000>; /* SROT */
+			#qcom,sensors = <13>;
+			#thermal-sensor-cells = <1>;
+		};
+
+		tsens1: thermal-sensor@4ac000 {
+			compatible = "qcom,msm8996-tsens";
+			reg = <0x4ad000 0x1000>, /* TM */
+			      <0x4ac000 0x1000>; /* SROT */
+			#qcom,sensors = <8>;
 			#thermal-sensor-cells = <1>;
 		};
 
diff --git a/drivers/thermal/qcom/tsens-8996.c b/drivers/thermal/qcom/tsens-8996.c
index e1f7781..6e59078 100644
--- a/drivers/thermal/qcom/tsens-8996.c
+++ b/drivers/thermal/qcom/tsens-8996.c
@@ -79,6 +79,5 @@ static const struct tsens_ops ops_8996 = {
 };
 
 const struct tsens_data data_8996 = {
-	.num_sensors	= 13,
 	.ops		= &ops_8996,
 };
-- 
2.7.4

[PATCH v2 3/5] thermal: tsens: Move 8996 get_temp() to common code for reuse

[Amit Kucheria]

The TSENS block inside the 8996 is internally classified as version 2.
Several other SoC families use this block and can share this code. Rename
get_temp() to reflect that it can be used across the v2 family.

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
 drivers/thermal/qcom/tsens-8996.c   | 73 ++-----------------------------------
 drivers/thermal/qcom/tsens-common.c | 69 ++++++++++++++++++++++++++++++-----
 drivers/thermal/qcom/tsens.h        |  1 +
 3 files changed, 63 insertions(+), 80 deletions(-)

diff --git a/drivers/thermal/qcom/tsens-8996.c b/drivers/thermal/qcom/tsens-8996.c
index 6e59078..a0d9096 100644
--- a/drivers/thermal/qcom/tsens-8996.c
+++ b/drivers/thermal/qcom/tsens-8996.c
@@ -1,81 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * Copyright (c) 2015, The Linux Foundation. All rights reserved.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 and
- * only version 2 as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
+ * Copyright (c) 2018, Linaro Limited
  */
 
-#include <linux/platform_device.h>
-#include <linux/regmap.h>
 #include "tsens.h"
 
-#define STATUS_OFFSET	0x10a0
-#define LAST_TEMP_MASK	0xfff
-#define STATUS_VALID_BIT	BIT(21)
-#define CODE_SIGN_BIT		BIT(11)
-
-static int get_temp_8996(struct tsens_device *tmdev, int id, int *temp)
-{
-	struct tsens_sensor *s = &tmdev->sensor[id];
-	u32 code;
-	unsigned int sensor_addr;
-	int last_temp = 0, last_temp2 = 0, last_temp3 = 0, ret;
-
-	sensor_addr = STATUS_OFFSET + s->hw_id * 4;
-	ret = regmap_read(tmdev->map, sensor_addr, &code);
-	if (ret)
-		return ret;
-	last_temp = code & LAST_TEMP_MASK;
-	if (code & STATUS_VALID_BIT)
-		goto done;
-
-	/* Try a second time */
-	ret = regmap_read(tmdev->map, sensor_addr, &code);
-	if (ret)
-		return ret;
-	if (code & STATUS_VALID_BIT) {
-		last_temp = code & LAST_TEMP_MASK;
-		goto done;
-	} else {
-		last_temp2 = code & LAST_TEMP_MASK;
-	}
-
-	/* Try a third/last time */
-	ret = regmap_read(tmdev->map, sensor_addr, &code);
-	if (ret)
-		return ret;
-	if (code & STATUS_VALID_BIT) {
-		last_temp = code & LAST_TEMP_MASK;
-		goto done;
-	} else {
-		last_temp3 = code & LAST_TEMP_MASK;
-	}
-
-	if (last_temp == last_temp2)
-		last_temp = last_temp2;
-	else if (last_temp2 == last_temp3)
-		last_temp = last_temp3;
-done:
-	/* Code sign bit is the sign extension for a negative value */
-	if (last_temp & CODE_SIGN_BIT)
-		last_temp |= ~CODE_SIGN_BIT;
-
-	/* Temperatures are in deciCelicius */
-	*temp = last_temp * 100;
-
-	return 0;
-}
-
 static const struct tsens_ops ops_8996 = {
 	.init		= init_common,
-	.get_temp	= get_temp_8996,
+	.get_temp	= get_temp_tsens_v2,
 };
 
 const struct tsens_data data_8996 = {
diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c
index b1449ad..961ace4 100644
--- a/drivers/thermal/qcom/tsens-common.c
+++ b/drivers/thermal/qcom/tsens-common.c
@@ -1,15 +1,7 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * Copyright (c) 2015, The Linux Foundation. All rights reserved.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 and
- * only version 2 as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
+ * Copyright (c) 2018, Linaro Limited
  */
 
 #include <linux/err.h>
@@ -117,6 +109,63 @@ int get_temp_common(struct tsens_device *tmdev, int id, int *temp)
 	return 0;
 }
 
+#define STATUS_OFFSET		0xa0
+#define LAST_TEMP_MASK		0xfff
+#define STATUS_VALID_BIT	BIT(21)
+#define CODE_SIGN_BIT		BIT(11)
+
+int get_temp_tsens_v2(struct tsens_device *tmdev, int id, int *temp)
+{
+	struct tsens_sensor *s = &tmdev->sensor[id];
+	u32 code;
+	unsigned int sensor_addr;
+	int last_temp = 0, last_temp2 = 0, last_temp3 = 0, ret;
+
+	sensor_addr = STATUS_OFFSET + s->hw_id * 4;
+	ret = regmap_read(tmdev->map, sensor_addr, &code);
+	if (ret)
+		return ret;
+	last_temp = code & LAST_TEMP_MASK;
+	if (code & STATUS_VALID_BIT)
+		goto done;
+
+	/* Try a second time */
+	ret = regmap_read(tmdev->map, sensor_addr, &code);
+	if (ret)
+		return ret;
+	if (code & STATUS_VALID_BIT) {
+		last_temp = code & LAST_TEMP_MASK;
+		goto done;
+	} else {
+		last_temp2 = code & LAST_TEMP_MASK;
+	}
+
+	/* Try a third/last time */
+	ret = regmap_read(tmdev->map, sensor_addr, &code);
+	if (ret)
+		return ret;
+	if (code & STATUS_VALID_BIT) {
+		last_temp = code & LAST_TEMP_MASK;
+		goto done;
+	} else {
+		last_temp3 = code & LAST_TEMP_MASK;
+	}
+
+	if (last_temp == last_temp2)
+		last_temp = last_temp2;
+	else if (last_temp2 == last_temp3)
+		last_temp = last_temp3;
+done:
+	/* Code sign bit is the sign extension for a negative value */
+	if (last_temp & CODE_SIGN_BIT)
+		last_temp |= ~CODE_SIGN_BIT;
+
+	/* Temperatures are in deciCelicius */
+	*temp = last_temp * 100;
+
+	return 0;
+}
+
 static const struct regmap_config tsens_config = {
 	.reg_bits	= 32,
 	.val_bits	= 32,
diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
index dc56e1e..80b273d 100644
--- a/drivers/thermal/qcom/tsens.h
+++ b/drivers/thermal/qcom/tsens.h
@@ -86,6 +86,7 @@ char *qfprom_read(struct device *, const char *);
 void compute_intercept_slope(struct tsens_device *, u32 *, u32 *, u32);
 int init_common(struct tsens_device *);
 int get_temp_common(struct tsens_device *, int, int *);
+int get_temp_tsens_v2(struct tsens_device *, int, int *);
 
 extern const struct tsens_data data_8916, data_8974, data_8960, data_8996;
 
-- 
2.7.4

[PATCH v2 4/5] thermal: tsens: Add support for SDM845

[Amit Kucheria]

SDM845 uses the TSENS v2 IP block

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
 Documentation/devicetree/bindings/thermal/qcom-tsens.txt |  1 +
 drivers/thermal/qcom/Makefile                            |  2 +-
 drivers/thermal/qcom/tsens-sdm845.c                      | 15 +++++++++++++++
 drivers/thermal/qcom/tsens.c                             |  3 +++
 drivers/thermal/qcom/tsens.h                             |  5 ++++-
 5 files changed, 24 insertions(+), 2 deletions(-)
 create mode 100644 drivers/thermal/qcom/tsens-sdm845.c

diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
index 292ed89..8652499 100644
--- a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
+++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
@@ -5,6 +5,7 @@ Required properties:
  - "qcom,msm8916-tsens" : For 8916 Family of SoCs
  - "qcom,msm8974-tsens" : For 8974 Family of SoCs
  - "qcom,msm8996-tsens" : For 8996 Family of SoCs
+ - "qcom,sdm845-tsens"  : For SDM845 Family of SoCs
 
 - reg: Address range of the thermal registers
 - #thermal-sensor-cells : Should be 1. See ./thermal.txt for a description.
diff --git a/drivers/thermal/qcom/Makefile b/drivers/thermal/qcom/Makefile
index 2cc2193..dc9f169 100644
--- a/drivers/thermal/qcom/Makefile
+++ b/drivers/thermal/qcom/Makefile
@@ -1,2 +1,2 @@
 obj-$(CONFIG_QCOM_TSENS)	+= qcom_tsens.o
-qcom_tsens-y			+= tsens.o tsens-common.o tsens-8916.o tsens-8974.o tsens-8960.o tsens-8996.o
+qcom_tsens-y			+= tsens.o tsens-common.o tsens-8916.o tsens-8974.o tsens-8960.o tsens-8996.o tsens-sdm845.o
diff --git a/drivers/thermal/qcom/tsens-sdm845.c b/drivers/thermal/qcom/tsens-sdm845.c
new file mode 100644
index 0000000..a647265
--- /dev/null
+++ b/drivers/thermal/qcom/tsens-sdm845.c
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, Linaro Limited
+ */
+
+#include "tsens.h"
+
+static const struct tsens_ops ops_sdm845 = {
+	.init		= init_common,
+	.get_temp	= get_temp_tsens_v2,
+};
+
+const struct tsens_data data_sdm845 = {
+	.ops		= &ops_sdm845,
+};
diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
index 3f9fe6a..314a20f 100644
--- a/drivers/thermal/qcom/tsens.c
+++ b/drivers/thermal/qcom/tsens.c
@@ -72,6 +72,9 @@ static const struct of_device_id tsens_table[] = {
 	}, {
 		.compatible = "qcom,msm8996-tsens",
 		.data = &data_8996,
+	}, {
+		.compatible = "qcom,sdm845-tsens",
+		.data = &data_sdm845,
 	},
 	{}
 };
diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
index 80b273d..e4c0a38 100644
--- a/drivers/thermal/qcom/tsens.h
+++ b/drivers/thermal/qcom/tsens.h
@@ -88,6 +88,9 @@ int init_common(struct tsens_device *);
 int get_temp_common(struct tsens_device *, int, int *);
 int get_temp_tsens_v2(struct tsens_device *, int, int *);
 
-extern const struct tsens_data data_8916, data_8974, data_8960, data_8996;
+/* TSENS v1 targets */
+extern const struct tsens_data data_8916, data_8974, data_8960;
+/* TSENS v2 targets */
+extern const struct tsens_data data_8996, data_sdm845;
 
 #endif /* __QCOM_TSENS_H__ */
-- 
2.7.4

[PATCH v2 5/5] thermal: tsens: Check if we have valid data before reading

[Amit Kucheria]

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

diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c
index 961ace4..f510e61 100644
--- a/drivers/thermal/qcom/tsens-common.c
+++ b/drivers/thermal/qcom/tsens-common.c
@@ -114,6 +114,9 @@ int get_temp_common(struct tsens_device *tmdev, int id, int *temp)
 #define STATUS_VALID_BIT	BIT(21)
 #define CODE_SIGN_BIT		BIT(11)
 
+#define TRDY_OFFSET     	0xe4
+#define TRDY_READY_BIT  	BIT(0)
+
 int get_temp_tsens_v2(struct tsens_device *tmdev, int id, int *temp)
 {
 	struct tsens_sensor *s = &tmdev->sensor[id];
@@ -121,6 +124,12 @@ int get_temp_tsens_v2(struct tsens_device *tmdev, int id, int *temp)
 	unsigned int sensor_addr;
 	int last_temp = 0, last_temp2 = 0, last_temp3 = 0, ret;
 
+	ret = regmap_read(tmdev->map, TRDY_OFFSET, &code);
+	if (ret)
+		return ret;
+	if (code & TRDY_READY_BIT)
+		return -ENODATA;
+
 	sensor_addr = STATUS_OFFSET + s->hw_id * 4;
 	ret = regmap_read(tmdev->map, sensor_addr, &code);
 	if (ret)
-- 
2.7.4

Re: [PATCH v2 4/5] thermal: tsens: Add support for SDM845

[Rob Herring]

On Tue, Jun 12, 2018 at 01:54:56PM +0300, Amit Kucheria wrote:
> SDM845 uses the TSENS v2 IP block
> 
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---
>  Documentation/devicetree/bindings/thermal/qcom-tsens.txt |  1 +

Acked-by: Rob Herring <robh@kernel.org>

>  drivers/thermal/qcom/Makefile                            |  2 +-
>  drivers/thermal/qcom/tsens-sdm845.c                      | 15 +++++++++++++++
>  drivers/thermal/qcom/tsens.c                             |  3 +++
>  drivers/thermal/qcom/tsens.h                             |  5 ++++-
>  5 files changed, 24 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/thermal/qcom/tsens-sdm845.c

Re: [PATCH v2 2/5] dt: qcom: 8996: thermal: Move to DT initialisation

[Bjorn Andersson]

On Tue 12 Jun 03:54 PDT 2018, Amit Kucheria wrote:

> We also split up the regmap address space into two, one for the TM
> registers, the other for the SROT registers. This was required to deal with
> different address offsets for the TM and SROT registers across different
> SoC families.
> 
> Since tsens-common.c/init_common() currently only registers one address space, the order is important (TM before SROT).This is OK since the code doesn't really use the SROT functionality yet.

Please line wrap this.

> 
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/msm8996.dtsi | 12 +++++++++++-
>  drivers/thermal/qcom/tsens-8996.c     |  1 -
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
> index 410ae78..b4aab18 100644
> --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
> @@ -451,7 +451,17 @@
>  
>  		tsens0: thermal-sensor@4a8000 {
>  			compatible = "qcom,msm8996-tsens";
> -			reg = <0x4a8000 0x2000>;
> +			reg = <0x4a9000 0x1000>, /* TM */
> +			      <0x4a8000 0x1000>; /* SROT */
> +			#qcom,sensors = <13>;
> +			#thermal-sensor-cells = <1>;
> +		};
> +
> +		tsens1: thermal-sensor@4ac000 {
> +			compatible = "qcom,msm8996-tsens";
> +			reg = <0x4ad000 0x1000>, /* TM */
> +			      <0x4ac000 0x1000>; /* SROT */
> +			#qcom,sensors = <8>;
>  			#thermal-sensor-cells = <1>;
>  		};
>  
> diff --git a/drivers/thermal/qcom/tsens-8996.c b/drivers/thermal/qcom/tsens-8996.c
> index e1f7781..6e59078 100644
> --- a/drivers/thermal/qcom/tsens-8996.c
> +++ b/drivers/thermal/qcom/tsens-8996.c
> @@ -79,6 +79,5 @@ static const struct tsens_ops ops_8996 = {
>  };
>  
>  const struct tsens_data data_8996 = {
> -	.num_sensors	= 13,

This will cause the current 8996 dts to fail probing the tsens. I think
you should just leave this as is, because specifying qcom,sensors in dts
will overwrite this number regardless.

It also would make this change dts specific, which is convenient as it
breaks the interdependency between the different subsystems.

>  	.ops		= &ops_8996,

Regards,
Bjorn

Re: [PATCH v2 3/5] thermal: tsens: Move 8996 get_temp() to common code for reuse

[Bjorn Andersson]

On Tue 12 Jun 03:54 PDT 2018, Amit Kucheria wrote:
> diff --git a/drivers/thermal/qcom/tsens-8996.c b/drivers/thermal/qcom/tsens-8996.c
[..]
>  static const struct tsens_ops ops_8996 = {
>  	.init		= init_common,
> -	.get_temp	= get_temp_8996,
> +	.get_temp	= get_temp_tsens_v2,
>  };
>  
>  const struct tsens_data data_8996 = {
> diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c
[..]
> +int get_temp_tsens_v2(struct tsens_device *tmdev, int id, int *temp)

I like the function name, but it's not really common for tsens, it's
just common for tsens v2. Also as patch 4 shows we end up adding a set
of essentially empty platform specific files for referencing this
function.

I would suggest that you instead rename tsens-8996.c to tsens-v2.c,
rename ops_8996 to ops_v2 and either add new tsens_data for each
platform or simply rename that too to data_v2 which we point to from
tsens_table.


I think we should take it once step further and add "qcom,tsens-v2" as a
valid compatible in tsens_table and make the dts do:

	comaptible = "qcom,msm8996-tsens", "qcom,tsens-v2";

and
	compatible = "qcom,sdm845-tsens", "qcom,tsens-v2";

Regards,
Bjorn

Re: [PATCH v2 5/5] thermal: tsens: Check if we have valid data before reading

[Bjorn Andersson]

On Tue 12 Jun 03:54 PDT 2018, Amit Kucheria wrote:

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

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  drivers/thermal/qcom/tsens-common.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c
> index 961ace4..f510e61 100644
> --- a/drivers/thermal/qcom/tsens-common.c
> +++ b/drivers/thermal/qcom/tsens-common.c
> @@ -114,6 +114,9 @@ int get_temp_common(struct tsens_device *tmdev, int id, int *temp)
>  #define STATUS_VALID_BIT	BIT(21)
>  #define CODE_SIGN_BIT		BIT(11)
>  
> +#define TRDY_OFFSET     	0xe4
> +#define TRDY_READY_BIT  	BIT(0)
> +
>  int get_temp_tsens_v2(struct tsens_device *tmdev, int id, int *temp)
>  {
>  	struct tsens_sensor *s = &tmdev->sensor[id];
> @@ -121,6 +124,12 @@ int get_temp_tsens_v2(struct tsens_device *tmdev, int id, int *temp)
>  	unsigned int sensor_addr;
>  	int last_temp = 0, last_temp2 = 0, last_temp3 = 0, ret;
>  
> +	ret = regmap_read(tmdev->map, TRDY_OFFSET, &code);
> +	if (ret)
> +		return ret;
> +	if (code & TRDY_READY_BIT)
> +		return -ENODATA;
> +
>  	sensor_addr = STATUS_OFFSET + s->hw_id * 4;
>  	ret = regmap_read(tmdev->map, sensor_addr, &code);
>  	if (ret)
> -- 
> 2.7.4
> 

Re: [PATCH v2 3/5] thermal: tsens: Move 8996 get_temp() to common code for reuse

[Amit Kucheria]

On Tue, Jun 12, 2018 at 10:43 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Tue 12 Jun 03:54 PDT 2018, Amit Kucheria wrote:
> > diff --git a/drivers/thermal/qcom/tsens-8996.c b/drivers/thermal/qcom/tsens-8996.c
> [..]
> >  static const struct tsens_ops ops_8996 = {
> >       .init           = init_common,
> > -     .get_temp       = get_temp_8996,
> > +     .get_temp       = get_temp_tsens_v2,
> >  };
> >
> >  const struct tsens_data data_8996 = {
> > diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c
> [..]
> > +int get_temp_tsens_v2(struct tsens_device *tmdev, int id, int *temp)
>
> I like the function name, but it's not really common for tsens, it's
> just common for tsens v2. Also as patch 4 shows we end up adding a set
> of essentially empty platform specific files for referencing this
> function.

> I would suggest that you instead rename tsens-8996.c to tsens-v2.c,
> rename ops_8996 to ops_v2 and either add new tsens_data for each
> platform or simply rename that too to data_v2 which we point to from
> tsens_table.

I was thinking of tsens-common.c as a library of tsens functions (v1
and v2). Do you want tsens-common.c to essentially become tsens-v1.c?
We'll end up with quite a bit of duplicated code in that case.

IIUC what you are suggesting, we'll still need to compile in
tsens-common.c even on v2 platforms.

> I think we should take it once step further and add "qcom,tsens-v2" as a
> valid compatible in tsens_table and make the dts do:

I like that idea.

>         comaptible = "qcom,msm8996-tsens", "qcom,tsens-v2";
>
> and
>         compatible = "qcom,sdm845-tsens", "qcom,tsens-v2";
>
> Regards,
> Bjorn

Re: [PATCH v2 2/5] dt: qcom: 8996: thermal: Move to DT initialisation

[Amit Kucheria]

On Tue, Jun 12, 2018 at 10:35 PM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> On Tue 12 Jun 03:54 PDT 2018, Amit Kucheria wrote:
>
>> We also split up the regmap address space into two, one for the TM
>> registers, the other for the SROT registers. This was required to deal with
>> different address offsets for the TM and SROT registers across different
>> SoC families.
>>
>> Since tsens-common.c/init_common() currently only registers one address space, the order is important (TM before SROT).This is OK since the code doesn't really use the SROT functionality yet.
>
> Please line wrap this.
>
>>
>> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
>> ---
>>  arch/arm64/boot/dts/qcom/msm8996.dtsi | 12 +++++++++++-
>>  drivers/thermal/qcom/tsens-8996.c     |  1 -
>>  2 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
>> index 410ae78..b4aab18 100644
>> --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
>> @@ -451,7 +451,17 @@
>>
>>               tsens0: thermal-sensor@4a8000 {
>>                       compatible = "qcom,msm8996-tsens";
>> -                     reg = <0x4a8000 0x2000>;
>> +                     reg = <0x4a9000 0x1000>, /* TM */
>> +                           <0x4a8000 0x1000>; /* SROT */
>> +                     #qcom,sensors = <13>;
>> +                     #thermal-sensor-cells = <1>;
>> +             };
>> +
>> +             tsens1: thermal-sensor@4ac000 {
>> +                     compatible = "qcom,msm8996-tsens";
>> +                     reg = <0x4ad000 0x1000>, /* TM */
>> +                           <0x4ac000 0x1000>; /* SROT */
>> +                     #qcom,sensors = <8>;
>>                       #thermal-sensor-cells = <1>;
>>               };
>>
>> diff --git a/drivers/thermal/qcom/tsens-8996.c b/drivers/thermal/qcom/tsens-8996.c
>> index e1f7781..6e59078 100644
>> --- a/drivers/thermal/qcom/tsens-8996.c
>> +++ b/drivers/thermal/qcom/tsens-8996.c
>> @@ -79,6 +79,5 @@ static const struct tsens_ops ops_8996 = {
>>  };
>>
>>  const struct tsens_data data_8996 = {
>> -     .num_sensors    = 13,
>
> This will cause the current 8996 dts to fail probing the tsens. I think
> you should just leave this as is, because specifying qcom,sensors in dts
> will overwrite this number regardless.

Ack, I didn't consider backword compatility of the code with the
current dts. Will fix.

> It also would make this change dts specific, which is convenient as it
> breaks the interdependency between the different subsystems.
>
>>       .ops            = &ops_8996,
>
> Regards,
> Bjorn

Re: [PATCH v2 4/5] thermal: tsens: Add support for SDM845

[Vivek Gautam]

Hi Amit,

On Tue, Jun 12, 2018 at 4:24 PM, Amit Kucheria <amit.kucheria@linaro.org> wrote:
> SDM845 uses the TSENS v2 IP block
>
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---
>  Documentation/devicetree/bindings/thermal/qcom-tsens.txt |  1 +
>  drivers/thermal/qcom/Makefile                            |  2 +-
>  drivers/thermal/qcom/tsens-sdm845.c                      | 15 +++++++++++++++
>  drivers/thermal/qcom/tsens.c                             |  3 +++
>  drivers/thermal/qcom/tsens.h                             |  5 ++++-
>  5 files changed, 24 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/thermal/qcom/tsens-sdm845.c
>
> diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
> index 292ed89..8652499 100644
> --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
> +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.txt
> @@ -5,6 +5,7 @@ Required properties:
>   - "qcom,msm8916-tsens" : For 8916 Family of SoCs
>   - "qcom,msm8974-tsens" : For 8974 Family of SoCs
>   - "qcom,msm8996-tsens" : For 8996 Family of SoCs
> + - "qcom,sdm845-tsens"  : For SDM845 Family of SoCs
>
>  - reg: Address range of the thermal registers
>  - #thermal-sensor-cells : Should be 1. See ./thermal.txt for a description.
> diff --git a/drivers/thermal/qcom/Makefile b/drivers/thermal/qcom/Makefile
> index 2cc2193..dc9f169 100644
> --- a/drivers/thermal/qcom/Makefile
> +++ b/drivers/thermal/qcom/Makefile
> @@ -1,2 +1,2 @@
>  obj-$(CONFIG_QCOM_TSENS)       += qcom_tsens.o
> -qcom_tsens-y                   += tsens.o tsens-common.o tsens-8916.o tsens-8974.o tsens-8960.o tsens-8996.o
> +qcom_tsens-y                   += tsens.o tsens-common.o tsens-8916.o tsens-8974.o tsens-8960.o tsens-8996.o tsens-sdm845.o
> diff --git a/drivers/thermal/qcom/tsens-sdm845.c b/drivers/thermal/qcom/tsens-sdm845.c
> new file mode 100644
> index 0000000..a647265
> --- /dev/null
> +++ b/drivers/thermal/qcom/tsens-sdm845.c
> @@ -0,0 +1,15 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, Linaro Limited
> + */
> +
> +#include "tsens.h"
> +
> +static const struct tsens_ops ops_sdm845 = {
> +       .init           = init_common,
> +       .get_temp       = get_temp_tsens_v2,
> +};
> +
> +const struct tsens_data data_sdm845 = {

Just a minor nit. 'static' here?

> +       .ops            = &ops_sdm845,
> +};

[snip]

Thanks & Regards
Vivek


-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v2 4/5] thermal: tsens: Add support for SDM845

[Amit Kucheria]

On Thu, Jun 14, 2018 at 9:48 AM, Vivek Gautam
<vivek.gautam@codeaurora.org> wrote:
> Hi Amit,
>
> On Tue, Jun 12, 2018 at 4:24 PM, Amit Kucheria <amit.kucheria@linaro.org> wrote:
>> SDM845 uses the TSENS v2 IP block
>>
>> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
>> ---

<snip>

>> +
>> +static const struct tsens_ops ops_sdm845 = {
>> +       .init           = init_common,
>> +       .get_temp       = get_temp_tsens_v2,
>> +};
>> +
>> +const struct tsens_data data_sdm845 = {
>
> Just a minor nit. 'static' here?

Thanks for the review.

This file just went away in favour of a common tsens-v2.c that will
support all v2 SoCs. I'll be sending out an updated patchset soon.

>
>> +       .ops            = &ops_sdm845,
>> +};
>
> [snip]
>
> Thanks & Regards
> Vivek