[PATCHv5 0/5] thermal: add driver for R-Car Gen3

[PATCHv5 0/5] thermal: add driver for R-Car Gen3

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Hi all,

The series adds thermal support to Renesas R-Car Gen3 SoCs. It's tested 
on Salvator-X H3 and M3-W SoC.

Wolfram asked me to have a look at the comments for v4 and to try and 
rework the temperature formulas to work with only ints and if it worked 
out resend the series. I have reworked code in 2/5 but kept Wolfram as 
the author and added my SoB, hope this is OK.

Reworking the formulas involved moving from s64 to int as the data type 
used to store coefficients used in decimal scaled fixed point 
calculations. By examining the largest values which could be produced in 
these calculations a factor of 100 is the maximum scaling which is 
possible and still fit inside an int, previously with s64 the scaling 
was 1000.

Reducing the decimal scaling also reduces the accuracy of the 
calculations. In my tests this was hardly noticeable as the granularity 
of the reported temperature to user-space is 0.5C, comparing the value 
reported in the s64 vs int implementation only differ very slightly when 
it rounded the value to the next 0.5C level.

However the formulas used to calculate the coefficients are not 
documented and none obvious (at lest not to me) so given different 
initial values to calculate the coefficients the error might become 
larger. Therefore I would like to ask Morimoto-san and/or Khiem to 
provide or proxy testing of this less accurate formula and feedback if 
it's OK, let me know if there is anything I can do to help out.

Changes since v4:
- Use only 32 bit ints to convert from register value to temperature.
- Merge and simplify temp calculation functions.
- Document what I known about the temprature conversion formulas.
- Add new patch 5/5 which fixes a waring printout caused by trying to get 
  temp before hardware is ready on r8a7795.
- Fixed a few checkpatch warnings.

Niklas Söderlund (1):
  thermal: rcar_gen3_thermal: Add delay in .thermal_init on r8a7795

Wolfram Sang (4):
  thermal: rcar_gen3_thermal: Document the R-Car Gen3
  thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver
  arm64: dts: r8a7795: Add R-Car Gen3 thermal support
  arm64: dts: r8a7796: Add R-Car Gen3 thermal support

 .../bindings/thermal/rcar-gen3-thermal.txt         |  56 ++++
 arch/arm64/boot/dts/renesas/r8a7795.dtsi           |  58 ++++
 arch/arm64/boot/dts/renesas/r8a7796.dtsi           |  58 ++++
 drivers/thermal/Kconfig                            |   9 +
 drivers/thermal/Makefile                           |   1 +
 drivers/thermal/rcar_gen3_thermal.c                | 330 +++++++++++++++++++++
 6 files changed, 512 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/thermal/rcar-gen3-thermal.txt
 create mode 100644 drivers/thermal/rcar_gen3_thermal.c

-- 
2.10.2

[PATCHv5 1/5] thermal: rcar_gen3_thermal: Document the R-Car Gen3

[Niklas Söderlund]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

Signed-off-by: Hien Dang <hien.dang.eb@renesas.com>
Signed-off-by: Khiem Nguyen <khiem.nguyen.xt@renesas.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 .../bindings/thermal/rcar-gen3-thermal.txt         | 56 ++++++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/thermal/rcar-gen3-thermal.txt

diff --git a/Documentation/devicetree/bindings/thermal/rcar-gen3-thermal.txt b/Documentation/devicetree/bindings/thermal/rcar-gen3-thermal.txt
new file mode 100644
index 0000000..07a9713
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/rcar-gen3-thermal.txt
@@ -0,0 +1,56 @@
+* DT bindings for Renesas R-Car Gen3 Thermal Sensor driver
+
+On R-Car Gen3 SoCs, the thermal sensor controllers (TSC) control the thermal
+sensors (THS) which are the analog circuits for measuring temperature (Tj)
+inside the LSI.
+
+Required properties:
+- compatible		: "renesas,<soctype>-thermal",
+			  Examples with soctypes are:
+			    - "renesas,r8a7795-thermal" (R-Car H3)
+			    - "renesas,r8a7796-thermal" (R-Car M3-W)
+- reg			: Address ranges of the thermal registers. Each sensor
+			  needs one address range. Sorting must be done in
+			  increasing order according to datasheet, i.e.
+			  TSC1, TSC2, ...
+- clocks		: Must contain a reference to the functional clock.
+- #thermal-sensor-cells : must be <1>.
+
+Optional properties:
+
+- interrupts           : interrupts routed to the TSC (3 for H3 and M3-W)
+- power-domain		: Must contain a reference to the power domain. This
+			  property is mandatory if the thermal sensor instance
+			  is part of a controllable power domain.
+
+Example:
+
+	tsc: thermal@e6198000 {
+		compatible = "renesas,r8a7795-thermal";
+		reg = <0 0xe6198000 0 0x68>,
+		      <0 0xe61a0000 0 0x5c>,
+		      <0 0xe61a8000 0 0x5c>;
+		interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cpg CPG_MOD 522>;
+		power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
+		#thermal-sensor-cells = <1>;
+		status = "okay";
+	};
+
+	thermal-zones {
+		sensor_thermal1: sensor-thermal1 {
+			polling-delay-passive = <250>;
+			polling-delay = <1000>;
+			thermal-sensors = <&tsc 0>;
+
+			trips {
+				sensor1_crit: sensor1-crit {
+					temperature = <90000>;
+					hysteresis = <2000>;
+					type = "critical";
+				};
+			};
+		};
+	};
-- 
2.10.2

[PATCHv5 2/5] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver

[Niklas Söderlund]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

Add support for R-Car Gen3 thermal sensors. Polling only for now,
interrupts will be added incrementally. Same goes for reading fuses.
This is documented already, but no hardware available for now.

Signed-off-by: Hien Dang <hien.dang.eb@renesas.com>
Signed-off-by: Thao Nguyen <thao.nguyen.yb@rvc.renesas.com>
Signed-off-by: Khiem Nguyen <khiem.nguyen.xt@renesas.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
[Niklas: document and rework temperature calculation]
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/thermal/Kconfig             |   9 +
 drivers/thermal/Makefile            |   1 +
 drivers/thermal/rcar_gen3_thermal.c | 328 ++++++++++++++++++++++++++++++++++++
 3 files changed, 338 insertions(+)
 create mode 100644 drivers/thermal/rcar_gen3_thermal.c

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index a13541b..da71f94 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -243,6 +243,15 @@ config RCAR_THERMAL
 	  Enable this to plug the R-Car thermal sensor driver into the Linux
 	  thermal framework.
 
+config RCAR_GEN3_THERMAL
+	tristate "Renesas R-Car Gen3 thermal driver"
+	depends on ARCH_RENESAS || COMPILE_TEST
+	depends on HAS_IOMEM
+	depends on OF
+	help
+	  Enable this to plug the R-Car Gen3 thermal sensor driver into the Linux
+	  thermal framework.
+
 config KIRKWOOD_THERMAL
 	tristate "Temperature sensor on Marvell Kirkwood SoCs"
 	depends on MACH_KIRKWOOD || COMPILE_TEST
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index c92eb22..1216fb3 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_QCOM_SPMI_TEMP_ALARM)	+= qcom-spmi-temp-alarm.o
 obj-$(CONFIG_SPEAR_THERMAL)	+= spear_thermal.o
 obj-$(CONFIG_ROCKCHIP_THERMAL)	+= rockchip_thermal.o
 obj-$(CONFIG_RCAR_THERMAL)	+= rcar_thermal.o
+obj-$(CONFIG_RCAR_GEN3_THERMAL)	+= rcar_gen3_thermal.o
 obj-$(CONFIG_KIRKWOOD_THERMAL)  += kirkwood_thermal.o
 obj-y				+= samsung/
 obj-$(CONFIG_DOVE_THERMAL)  	+= dove_thermal.o
diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
new file mode 100644
index 0000000..7d78498
--- /dev/null
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -0,0 +1,328 @@
+/*
+ *  R-Car Gen3 THS thermal sensor driver
+ *  Based on rcar_thermal.c and work from Hien Dang and Khiem Nguyen.
+ *
+ * Copyright (C) 2016 Renesas Electronics Corporation.
+ * Copyright (C) 2016 Sang Engineering
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; version 2 of the License.
+ *
+ *  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.
+ *
+ */
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/thermal.h>
+
+/* Register offsets */
+#define REG_GEN3_IRQSTR		0x04
+#define REG_GEN3_IRQMSK		0x08
+#define REG_GEN3_IRQCTL		0x0C
+#define REG_GEN3_IRQEN		0x10
+#define REG_GEN3_IRQTEMP1	0x14
+#define REG_GEN3_IRQTEMP2	0x18
+#define REG_GEN3_IRQTEMP3	0x1C
+#define REG_GEN3_CTSR		0x20
+#define REG_GEN3_THCTR		0x20
+#define REG_GEN3_TEMP		0x28
+#define REG_GEN3_THCODE1	0x50
+#define REG_GEN3_THCODE2	0x54
+#define REG_GEN3_THCODE3	0x58
+
+/* CTSR bits */
+#define CTSR_PONM	BIT(8)
+#define CTSR_AOUT	BIT(7)
+#define CTSR_THBGR	BIT(5)
+#define CTSR_VMEN	BIT(4)
+#define CTSR_VMST	BIT(1)
+#define CTSR_THSST	BIT(0)
+
+/* THCTR bits */
+#define THCTR_PONM	BIT(6)
+#define THCTR_THSST	BIT(0)
+
+#define CTEMP_MASK	0xFFF
+
+#define MCELSIUS(temp)	((temp) * 1000)
+#define GEN3_FUSE_MASK	0xFFF
+
+#define TSC_MAX_NUM	3
+
+/* Structure for thermal temperature calculation */
+struct equation_coefs {
+	int a1;
+	int b1;
+	int a2;
+	int b2;
+};
+
+struct rcar_gen3_thermal_tsc {
+	void __iomem *base;
+	struct thermal_zone_device *zone;
+	struct equation_coefs coef;
+	struct mutex lock;
+};
+
+struct rcar_gen3_thermal_priv {
+	struct rcar_gen3_thermal_tsc *tscs[TSC_MAX_NUM];
+};
+
+struct rcar_gen3_thermal_data {
+	void (*thermal_init)(struct rcar_gen3_thermal_tsc *tsc);
+};
+
+static inline u32 rcar_gen3_thermal_read(struct rcar_gen3_thermal_tsc *tsc,
+					 u32 reg)
+{
+	return ioread32(tsc->base + reg);
+}
+
+static inline void rcar_gen3_thermal_write(struct rcar_gen3_thermal_tsc *tsc,
+					   u32 reg, u32 data)
+{
+	iowrite32(data, tsc->base + reg);
+}
+
+/*
+ * Linear approximation for temperature
+ *
+ * [reg] = [temp] * a + b => [temp] = ([reg] - b) / a
+ *
+ * The constants a and b are calculated using two triplets of int values PTAT
+ * and THCODE. PTAT and THCODE can either be read from hardware or use hard
+ * coded values from driver. The formula to calculate a and b are taken from
+ * BSP and sparsely documented and understood.
+ *
+ * Examining the linear formula and the formula used to calculate constants a
+ * and b while knowing that the span for PTAT and THCODE values are between
+ * 0x000 and 0xfff the largest integer possible is 0xfff * 0xfff == 0xffe001.
+ * Integer also needs to be signed so that leaves 7 bits for decimal
+ * fixed point scaling, which amounts to a decimal scaling factor of 100.
+ */
+
+#define SCALE_FACTOR 100
+#define SCALE_INT(_x) ((_x) * SCALE_FACTOR)
+#define SCALE_MUL(_a, _b) (((_a)*(_b)) / SCALE_FACTOR)
+#define SCALE_DIV(_a, _b) (((_a)*SCALE_FACTOR)/(_b))
+#define SCALE_TO_MCELSIUS(_x) ((_x) * 10)
+
+#define RCAR3_THERMAL_GRAN 500 /* mili Celsius */
+
+/* no idea where these constants come from */
+#define TJ_1 SCALE_INT(96)
+#define TJ_3 SCALE_INT(-41)
+
+static void rcar_gen3_thermal_calc_coefs(struct equation_coefs *coef,
+					 int *ptat, int *thcode)
+{
+	int tj_2;
+
+	/* TODO: Find documentation and document constant calculation formula */
+
+	tj_2 = SCALE_DIV(SCALE_MUL(SCALE_INT(ptat[1] - ptat[2]), SCALE_INT(137)),
+			 SCALE_INT(ptat[0] - ptat[2])) - SCALE_INT(41);
+
+	coef->a1 = SCALE_DIV(SCALE_INT(thcode[1] - thcode[2]), tj_2 - TJ_3);
+	coef->b1 = SCALE_INT(thcode[2]) - SCALE_MUL(coef->a1, TJ_3);
+
+	coef->a2 = SCALE_DIV(SCALE_INT(thcode[1] - thcode[0]), tj_2 - TJ_1);
+	coef->b2 = SCALE_INT(thcode[0]) - SCALE_MUL(coef->a2, TJ_1);
+}
+
+static int rcar_gen3_thermal_round(int temp)
+{
+	int result, round_offs;
+
+	round_offs = temp >= 0 ? RCAR3_THERMAL_GRAN / 2 :
+		-RCAR3_THERMAL_GRAN / 2;
+	result = (temp + round_offs) / RCAR3_THERMAL_GRAN;
+	return result * RCAR3_THERMAL_GRAN;
+}
+
+static int rcar_gen3_thermal_get_temp(void *devdata, int *temp)
+{
+	struct rcar_gen3_thermal_tsc *tsc = devdata;
+	int mcelsius, val1, val2;
+	u32 reg;
+
+	/* Read register and convert to mili Celsius */
+	mutex_lock(&tsc->lock);
+
+	reg = rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK;
+
+	val1 = SCALE_DIV(SCALE_INT(reg) - tsc->coef.b1, tsc->coef.a1);
+	val2 = SCALE_DIV(SCALE_INT(reg) - tsc->coef.b2, tsc->coef.a2);
+	mcelsius = SCALE_TO_MCELSIUS((val1 + val2) / 2);
+
+	mutex_unlock(&tsc->lock);
+
+	/* Make sure we are inside specifications */
+	if ((mcelsius < MCELSIUS(-40)) || (mcelsius > MCELSIUS(125)))
+		return -EIO;
+
+	/* Round value to device granularity setting */
+	*temp = rcar_gen3_thermal_round(mcelsius);
+
+	return 0;
+}
+
+static struct thermal_zone_of_device_ops rcar_gen3_tz_of_ops = {
+	.get_temp	= rcar_gen3_thermal_get_temp,
+};
+
+static void r8a7795_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
+{
+	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,  CTSR_THBGR);
+	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,  0x0);
+
+	usleep_range(1000, 2000);
+
+	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR, CTSR_PONM);
+	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQCTL, 0x3F);
+	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,
+				CTSR_PONM | CTSR_AOUT | CTSR_THBGR | CTSR_VMEN);
+
+	usleep_range(100, 200);
+
+	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,
+				CTSR_PONM | CTSR_AOUT | CTSR_THBGR | CTSR_VMEN |
+				CTSR_VMST | CTSR_THSST);
+}
+
+static void r8a7796_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
+{
+	u32 reg_val;
+
+	reg_val = rcar_gen3_thermal_read(tsc, REG_GEN3_THCTR);
+	reg_val &= ~THCTR_PONM;
+	rcar_gen3_thermal_write(tsc, REG_GEN3_THCTR, reg_val);
+
+	usleep_range(1000, 2000);
+
+	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQCTL, 0x3F);
+	reg_val = rcar_gen3_thermal_read(tsc, REG_GEN3_THCTR);
+	reg_val |= THCTR_THSST;
+	rcar_gen3_thermal_write(tsc, REG_GEN3_THCTR, reg_val);
+}
+
+static const struct rcar_gen3_thermal_data r8a7795_data = {
+	.thermal_init = r8a7795_thermal_init,
+};
+
+static const struct rcar_gen3_thermal_data r8a7796_data = {
+	.thermal_init = r8a7796_thermal_init,
+};
+
+static const struct of_device_id rcar_gen3_thermal_dt_ids[] = {
+	{ .compatible = "renesas,r8a7795-thermal", .data = &r8a7795_data},
+	{ .compatible = "renesas,r8a7796-thermal", .data = &r8a7796_data},
+	{},
+};
+MODULE_DEVICE_TABLE(of, rcar_gen3_thermal_dt_ids);
+
+static int rcar_gen3_thermal_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+
+	pm_runtime_put(dev);
+	pm_runtime_disable(dev);
+
+	return 0;
+}
+
+static int rcar_gen3_thermal_probe(struct platform_device *pdev)
+{
+	struct rcar_gen3_thermal_priv *priv;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct thermal_zone_device *zone;
+	int ret, i;
+	const struct rcar_gen3_thermal_data *match_data =
+		of_device_get_match_data(dev);
+
+	/* default values if FUSEs are missing */
+	/* TODO: Read values from hardware on supported platforms */
+	int ptat[3] = { 2351, 1509, 435 };
+	int thcode[TSC_MAX_NUM][3] = {
+		{ 3248, 2800, 2221 },
+		{ 3245, 2795, 2216 },
+		{ 3250, 2805, 2237 },
+	};
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, priv);
+
+	pm_runtime_enable(dev);
+	pm_runtime_get_sync(dev);
+
+	for (i = 0; i < TSC_MAX_NUM; i++) {
+		struct rcar_gen3_thermal_tsc *tsc;
+
+		tsc = devm_kzalloc(dev, sizeof(*tsc), GFP_KERNEL);
+		if (!tsc) {
+			ret = -ENOMEM;
+			goto error_unregister;
+		}
+
+		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
+		if (!res)
+			break;
+
+		tsc->base = devm_ioremap_resource(dev, res);
+		if (IS_ERR(tsc->base)) {
+			ret = PTR_ERR(tsc->base);
+			goto error_unregister;
+		}
+
+		priv->tscs[i] = tsc;
+		mutex_init(&tsc->lock);
+
+		match_data->thermal_init(tsc);
+		rcar_gen3_thermal_calc_coefs(&tsc->coef, ptat, thcode[i]);
+
+		zone = devm_thermal_zone_of_sensor_register(dev, i, tsc,
+							    &rcar_gen3_tz_of_ops);
+		if (IS_ERR(zone)) {
+			dev_err(dev, "Can't register thermal zone\n");
+			ret = PTR_ERR(zone);
+			goto error_unregister;
+		}
+		tsc->zone = zone;
+	}
+
+	return 0;
+
+error_unregister:
+	rcar_gen3_thermal_remove(pdev);
+
+	return ret;
+}
+
+static struct platform_driver rcar_gen3_thermal_driver = {
+	.driver	= {
+		.name	= "rcar_gen3_thermal",
+		.of_match_table = rcar_gen3_thermal_dt_ids,
+	},
+	.probe		= rcar_gen3_thermal_probe,
+	.remove		= rcar_gen3_thermal_remove,
+};
+module_platform_driver(rcar_gen3_thermal_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("R-Car Gen3 THS thermal sensor driver");
+MODULE_AUTHOR("Wolfram Sang <wsa+renesas@sang-engineering.com>");
-- 
2.10.2

[PATCHv5 3/5] arm64: dts: r8a7795: Add R-Car Gen3 thermal support

[Niklas Söderlund]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

Signed-off-by: Hien Dang <hien.dang.eb@renesas.com>
Signed-off-by: Thao Nguyen <thao.nguyen.yb@rvc.renesas.com>
Signed-off-by: Khiem Nguyen <khiem.nguyen.xt@renesas.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 arch/arm64/boot/dts/renesas/r8a7795.dtsi | 58 ++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
index 8c15040..0699e04 100644
--- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
@@ -1541,5 +1541,63 @@
 				};
 			};
 		};
+
+		tsc: thermal@e6198000 {
+			compatible = "renesas,r8a7795-thermal";
+			reg = <0 0xe6198000 0 0x68>,
+			      <0 0xe61a0000 0 0x5c>,
+			      <0 0xe61a8000 0 0x5c>;
+			interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD 522>;
+			power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
+			#thermal-sensor-cells = <1>;
+			status = "okay";
+		};
+
+		thermal-zones {
+			sensor_thermal1: sensor-thermal1 {
+				polling-delay-passive = <250>;
+				polling-delay = <1000>;
+				thermal-sensors = <&tsc 0>;
+
+				trips {
+					sensor1_crit: sensor1-crit {
+						temperature = <90000>;
+						hysteresis = <2000>;
+						type = "critical";
+					};
+				};
+			};
+
+			sensor_thermal2: sensor-thermal2 {
+				polling-delay-passive = <250>;
+				polling-delay = <1000>;
+				thermal-sensors = <&tsc 1>;
+
+				trips {
+					sensor2_crit: sensor2-crit {
+						temperature = <90000>;
+						hysteresis = <2000>;
+						type = "critical";
+					};
+				};
+			};
+
+			sensor_thermal3: sensor-thermal3 {
+				polling-delay-passive = <250>;
+				polling-delay = <1000>;
+				thermal-sensors = <&tsc 2>;
+
+				trips {
+					sensor3_crit: sensor3-crit {
+						temperature = <90000>;
+						hysteresis = <2000>;
+						type = "critical";
+					};
+				};
+			};
+		};
 	};
 };
-- 
2.10.2

[PATCHv5 4/5] arm64: dts: r8a7796: Add R-Car Gen3 thermal support

[Niklas Söderlund]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

Signed-off-by: Hien Dang <hien.dang.eb@renesas.com>
Signed-off-by: Thao Nguyen <thao.nguyen.yb@rvc.renesas.com>
Signed-off-by: Khiem Nguyen <khiem.nguyen.xt@renesas.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 arch/arm64/boot/dts/renesas/r8a7796.dtsi | 58 ++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a7796.dtsi b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
index 9217da9..5261f0f 100644
--- a/arch/arm64/boot/dts/renesas/r8a7796.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
@@ -251,5 +251,63 @@
 			power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
 			status = "disabled";
 		};
+
+		tsc: thermal@e6198000 {
+			compatible = "renesas,r8a7796-thermal";
+			reg = <0 0xe6198000 0 0x68>,
+			      <0 0xe61a0000 0 0x5c>,
+			      <0 0xe61a8000 0 0x5c>;
+			interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD 522>;
+			power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
+			#thermal-sensor-cells = <1>;
+			status = "okay";
+		};
+
+		thermal-zones {
+			sensor_thermal1: sensor-thermal1 {
+				polling-delay-passive = <250>;
+				polling-delay = <1000>;
+				thermal-sensors = <&tsc 0>;
+
+				trips {
+					sensor1_crit: sensor1-crit {
+						temperature = <90000>;
+						hysteresis = <2000>;
+						type = "critical";
+					};
+				};
+			};
+
+			sensor_thermal2: sensor-thermal2 {
+				polling-delay-passive = <250>;
+				polling-delay = <1000>;
+				thermal-sensors = <&tsc 1>;
+
+				trips {
+					sensor2_crit: sensor2-crit {
+						temperature = <90000>;
+						hysteresis = <2000>;
+						type = "critical";
+					};
+				};
+			};
+
+			sensor_thermal3: sensor-thermal3 {
+				polling-delay-passive = <250>;
+				polling-delay = <1000>;
+				thermal-sensors = <&tsc 2>;
+
+				trips {
+					sensor3_crit: sensor3-crit {
+						temperature = <90000>;
+						hysteresis = <2000>;
+						type = "critical";
+					};
+				};
+			};
+		};
 	};
 };
-- 
2.10.2

[PATCHv5 5/5] thermal: rcar_gen3_thermal: Add delay in .thermal_init on r8a7795

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

The .thermal_init needs to be delayed a short amount of time after
setting REG_GEN3_CTSR to allow for the TEMP register to contain
something useful. If it's not delayed theses warnings are common during
boot:

thermal thermal_zone0: failed to read out thermal zone (-5)
thermal thermal_zone1: failed to read out thermal zone (-5)
thermal thermal_zone2: failed to read out thermal zone (-5)

The warnings are triggered by the first call to .get_temp while the TEMP
register contains 0 and rcar_gen3_thermal_get_temp() returns -EIO since
a TEMP value of 0 will result in a temperature reading which is out of
specifications.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/thermal/rcar_gen3_thermal.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index 7d78498..085daec 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -199,6 +199,8 @@ static void r8a7795_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
 	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,
 				CTSR_PONM | CTSR_AOUT | CTSR_THBGR | CTSR_VMEN |
 				CTSR_VMST | CTSR_THSST);
+
+	usleep_range(1000, 2000);
 }
 
 static void r8a7796_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
-- 
2.10.2

Re: [PATCHv5 2/5] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver

[Wolfram Sang]

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

Hi Niklas,

thanks for this work!

> +/*
> + * Linear approximation for temperature
> + *
> + * [reg] = [temp] * a + b => [temp] = ([reg] - b) / a
> + *
> + * The constants a and b are calculated using two triplets of int values PTAT
> + * and THCODE. PTAT and THCODE can either be read from hardware or use hard
> + * coded values from driver. The formula to calculate a and b are taken from
> + * BSP and sparsely documented and understood.
> + *
> + * Examining the linear formula and the formula used to calculate constants a
> + * and b while knowing that the span for PTAT and THCODE values are between
> + * 0x000 and 0xfff the largest integer possible is 0xfff * 0xfff == 0xffe001.
> + * Integer also needs to be signed so that leaves 7 bits for decimal
> + * fixed point scaling, which amounts to a decimal scaling factor of 100.
> + */
> +
> +#define SCALE_FACTOR 100
> +#define SCALE_INT(_x) ((_x) * SCALE_FACTOR)
> +#define SCALE_MUL(_a, _b) (((_a)*(_b)) / SCALE_FACTOR)
> +#define SCALE_DIV(_a, _b) (((_a)*SCALE_FACTOR)/(_b))
> +#define SCALE_TO_MCELSIUS(_x) ((_x) * 10)

Spaces around operators everywhere, please.

I wonder about SCALE_MUL; isn't that more like "unscaling" because _a
and _b are already scaled?

And since _b is always a constant, couldn't we simply drop this macro
and simply do _a * _b (with _a being scaled already and _b not)?

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCHv5 5/5] thermal: rcar_gen3_thermal: Add delay in .thermal_init on r8a7795

[Eduardo Valentin]

On Mon, Dec 12, 2016 at 03:18:05PM +0100, Niklas S�derlund wrote:
> From: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> 
> The .thermal_init needs to be delayed a short amount of time after
> setting REG_GEN3_CTSR to allow for the TEMP register to contain
> something useful. If it's not delayed theses warnings are common during
> boot:
> 
> thermal thermal_zone0: failed to read out thermal zone (-5)
> thermal thermal_zone1: failed to read out thermal zone (-5)
> thermal thermal_zone2: failed to read out thermal zone (-5)
> 
> The warnings are triggered by the first call to .get_temp while the TEMP
> register contains 0 and rcar_gen3_thermal_get_temp() returns -EIO since
> a TEMP value of 0 will result in a temperature reading which is out of
> specifications.

I think this one can be folded on patch 2/5.

Re: [PATCHv5 5/5] thermal: rcar_gen3_thermal: Add delay in .thermal_init on r8a7795

[Eduardo Valentin]

On Mon, Dec 12, 2016 at 03:18:05PM +0100, Niklas Söderlund wrote:
> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> The .thermal_init needs to be delayed a short amount of time after
> setting REG_GEN3_CTSR to allow for the TEMP register to contain
> something useful. If it's not delayed theses warnings are common during
> boot:
> 
> thermal thermal_zone0: failed to read out thermal zone (-5)
> thermal thermal_zone1: failed to read out thermal zone (-5)
> thermal thermal_zone2: failed to read out thermal zone (-5)
> 
> The warnings are triggered by the first call to .get_temp while the TEMP
> register contains 0 and rcar_gen3_thermal_get_temp() returns -EIO since
> a TEMP value of 0 will result in a temperature reading which is out of
> specifications.

I think this one can be folded on patch 2/5.

Re: [PATCHv5 2/5] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver

[Eduardo Valentin]

Hey,

On Mon, Dec 12, 2016 at 03:18:02PM +0100, Niklas S�derlund wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Add support for R-Car Gen3 thermal sensors. Polling only for now,
> interrupts will be added incrementally. Same goes for reading fuses.
> This is documented already, but no hardware available for now.
> 
> Signed-off-by: Hien Dang <hien.dang.eb@renesas.com>
> Signed-off-by: Thao Nguyen <thao.nguyen.yb@rvc.renesas.com>
> Signed-off-by: Khiem Nguyen <khiem.nguyen.xt@renesas.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> [Niklas: document and rework temperature calculation]
> Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/thermal/Kconfig             |   9 +
>  drivers/thermal/Makefile            |   1 +
>  drivers/thermal/rcar_gen3_thermal.c | 328 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 338 insertions(+)
>  create mode 100644 drivers/thermal/rcar_gen3_thermal.c
> 
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index a13541b..da71f94 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -243,6 +243,15 @@ config RCAR_THERMAL
>  	  Enable this to plug the R-Car thermal sensor driver into the Linux
>  	  thermal framework.
>  
> +config RCAR_GEN3_THERMAL
> +	tristate "Renesas R-Car Gen3 thermal driver"
> +	depends on ARCH_RENESAS || COMPILE_TEST

cool that we have got COMPILE_TEST to work again!

thanks for that, I appreciate it.

> +	depends on HAS_IOMEM
> +	depends on OF
> +	help
> +	  Enable this to plug the R-Car Gen3 thermal sensor driver into the Linux
> +	  thermal framework.
> +
>  config KIRKWOOD_THERMAL
>  	tristate "Temperature sensor on Marvell Kirkwood SoCs"
>  	depends on MACH_KIRKWOOD || COMPILE_TEST
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index c92eb22..1216fb3 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -30,6 +30,7 @@ obj-$(CONFIG_QCOM_SPMI_TEMP_ALARM)	+= qcom-spmi-temp-alarm.o
>  obj-$(CONFIG_SPEAR_THERMAL)	+= spear_thermal.o
>  obj-$(CONFIG_ROCKCHIP_THERMAL)	+= rockchip_thermal.o
>  obj-$(CONFIG_RCAR_THERMAL)	+= rcar_thermal.o
> +obj-$(CONFIG_RCAR_GEN3_THERMAL)	+= rcar_gen3_thermal.o
>  obj-$(CONFIG_KIRKWOOD_THERMAL)  += kirkwood_thermal.o
>  obj-y				+= samsung/
>  obj-$(CONFIG_DOVE_THERMAL)  	+= dove_thermal.o
> diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> new file mode 100644
> index 0000000..7d78498
> --- /dev/null
> +++ b/drivers/thermal/rcar_gen3_thermal.c
> @@ -0,0 +1,328 @@
> +/*
> + *  R-Car Gen3 THS thermal sensor driver
> + *  Based on rcar_thermal.c and work from Hien Dang and Khiem Nguyen.
> + *
> + * Copyright (C) 2016 Renesas Electronics Corporation.
> + * Copyright (C) 2016 Sang Engineering
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; version 2 of the License.
> + *
> + *  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.
> + *
> + */
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/thermal.h>
> +
> +/* Register offsets */
> +#define REG_GEN3_IRQSTR		0x04
> +#define REG_GEN3_IRQMSK		0x08
> +#define REG_GEN3_IRQCTL		0x0C
> +#define REG_GEN3_IRQEN		0x10
> +#define REG_GEN3_IRQTEMP1	0x14
> +#define REG_GEN3_IRQTEMP2	0x18
> +#define REG_GEN3_IRQTEMP3	0x1C
> +#define REG_GEN3_CTSR		0x20
> +#define REG_GEN3_THCTR		0x20
> +#define REG_GEN3_TEMP		0x28
> +#define REG_GEN3_THCODE1	0x50
> +#define REG_GEN3_THCODE2	0x54
> +#define REG_GEN3_THCODE3	0x58
> +
> +/* CTSR bits */
> +#define CTSR_PONM	BIT(8)
> +#define CTSR_AOUT	BIT(7)
> +#define CTSR_THBGR	BIT(5)
> +#define CTSR_VMEN	BIT(4)
> +#define CTSR_VMST	BIT(1)
> +#define CTSR_THSST	BIT(0)
> +
> +/* THCTR bits */
> +#define THCTR_PONM	BIT(6)
> +#define THCTR_THSST	BIT(0)
> +
> +#define CTEMP_MASK	0xFFF
> +
> +#define MCELSIUS(temp)	((temp) * 1000)
> +#define GEN3_FUSE_MASK	0xFFF
> +
> +#define TSC_MAX_NUM	3
> +
> +/* Structure for thermal temperature calculation */
> +struct equation_coefs {
> +	int a1;
> +	int b1;
> +	int a2;
> +	int b2;

a little description of each coeff would be welcome.

> +};
> +
> +struct rcar_gen3_thermal_tsc {
> +	void __iomem *base;
> +	struct thermal_zone_device *zone;
> +	struct equation_coefs coef;
> +	struct mutex lock;
> +};
> +
> +struct rcar_gen3_thermal_priv {
> +	struct rcar_gen3_thermal_tsc *tscs[TSC_MAX_NUM];
> +};
> +
> +struct rcar_gen3_thermal_data {
> +	void (*thermal_init)(struct rcar_gen3_thermal_tsc *tsc);
> +};
> +
> +static inline u32 rcar_gen3_thermal_read(struct rcar_gen3_thermal_tsc *tsc,
> +					 u32 reg)
> +{
> +	return ioread32(tsc->base + reg);
> +}
> +
> +static inline void rcar_gen3_thermal_write(struct rcar_gen3_thermal_tsc *tsc,
> +					   u32 reg, u32 data)
> +{
> +	iowrite32(data, tsc->base + reg);
> +}
> +
> +/*
> + * Linear approximation for temperature
> + *
> + * [reg] = [temp] * a + b => [temp] = ([reg] - b) / a

the type of equation described above can be described using
of-thermal/DT. Have you tried using the coefficients DT property to get
those from DT?

> + *
> + * The constants a and b are calculated using two triplets of int values PTAT
> + * and THCODE. PTAT and THCODE can either be read from hardware or use hard
> + * coded values from driver. The formula to calculate a and b are taken from
> + * BSP and sparsely documented and understood.
> + *

hmmm.. OK. that gets a bit more interesting. 

So you can get a and b queried from hardware. cool.

but you can also get those hardcoded in the code. In that case, I would
suggest hardcode them in DT, instead, using the coefficients property.

> + * Examining the linear formula and the formula used to calculate constants a
> + * and b while knowing that the span for PTAT and THCODE values are between
> + * 0x000 and 0xfff the largest integer possible is 0xfff * 0xfff == 0xffe001.
> + * Integer also needs to be signed so that leaves 7 bits for decimal
> + * fixed point scaling, which amounts to a decimal scaling factor of 100.
> + */


I see.

> +
> +#define SCALE_FACTOR 100
> +#define SCALE_INT(_x) ((_x) * SCALE_FACTOR)
> +#define SCALE_MUL(_a, _b) (((_a)*(_b)) / SCALE_FACTOR)
> +#define SCALE_DIV(_a, _b) (((_a)*SCALE_FACTOR)/(_b))
> +#define SCALE_TO_MCELSIUS(_x) ((_x) * 10)
> +
> +#define RCAR3_THERMAL_GRAN 500 /* mili Celsius */
> +
> +/* no idea where these constants come from */

thermal simulation, maybe?

> +#define TJ_1 SCALE_INT(96)
> +#define TJ_3 SCALE_INT(-41)
> +
> +static void rcar_gen3_thermal_calc_coefs(struct equation_coefs *coef,
> +					 int *ptat, int *thcode)
> +{
> +	int tj_2;
> +
> +	/* TODO: Find documentation and document constant calculation formula */
> +
> +	tj_2 = SCALE_DIV(SCALE_MUL(SCALE_INT(ptat[1] - ptat[2]), SCALE_INT(137)),
> +			 SCALE_INT(ptat[0] - ptat[2])) - SCALE_INT(41);
> +
> +	coef->a1 = SCALE_DIV(SCALE_INT(thcode[1] - thcode[2]), tj_2 - TJ_3);
> +	coef->b1 = SCALE_INT(thcode[2]) - SCALE_MUL(coef->a1, TJ_3);
> +
> +	coef->a2 = SCALE_DIV(SCALE_INT(thcode[1] - thcode[0]), tj_2 - TJ_1);
> +	coef->b2 = SCALE_INT(thcode[0]) - SCALE_MUL(coef->a2, TJ_1);
> +}
> +
> +static int rcar_gen3_thermal_round(int temp)
> +{
> +	int result, round_offs;
> +
> +	round_offs = temp >= 0 ? RCAR3_THERMAL_GRAN / 2 :
> +		-RCAR3_THERMAL_GRAN / 2;
> +	result = (temp + round_offs) / RCAR3_THERMAL_GRAN;
> +	return result * RCAR3_THERMAL_GRAN;
> +}
> +
> +static int rcar_gen3_thermal_get_temp(void *devdata, int *temp)
> +{
> +	struct rcar_gen3_thermal_tsc *tsc = devdata;
> +	int mcelsius, val1, val2;
> +	u32 reg;
> +
> +	/* Read register and convert to mili Celsius */
> +	mutex_lock(&tsc->lock);
> +
> +	reg = rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK;
> +
> +	val1 = SCALE_DIV(SCALE_INT(reg) - tsc->coef.b1, tsc->coef.a1);
> +	val2 = SCALE_DIV(SCALE_INT(reg) - tsc->coef.b2, tsc->coef.a2);

I see. there are actually two sensors here, and 

> +	mcelsius = SCALE_TO_MCELSIUS((val1 + val2) / 2);

the driver always output the average of them.

based on previous driver history, people would actually expose all
sensors. Unless you want to be really strict to always average the two.

which to me seams a bit odd also, based on other drivers I have seen.
Hardware folks would typically ask for caring about all sensors. For
example, care about max, not average. But average is also fine, as long
as temperature does not move fast enough in one of the sides.

For example, you see a spike on one, and the other, still fine (taking
some thermal resistance before seen the heat), you would already take an
action. And here you will first average (filter the spike out), before
taking any action. That essentially means, you are delaying any action
further until the rise is seen by the average / filter.

Is that really what you want?

> +
> +	mutex_unlock(&tsc->lock);
> +
> +	/* Make sure we are inside specifications */
> +	if ((mcelsius < MCELSIUS(-40)) || (mcelsius > MCELSIUS(125)))
> +		return -EIO;
> +
> +	/* Round value to device granularity setting */
> +	*temp = rcar_gen3_thermal_round(mcelsius);
> +
> +	return 0;
> +}
> +
> +static struct thermal_zone_of_device_ops rcar_gen3_tz_of_ops = {
> +	.get_temp	= rcar_gen3_thermal_get_temp,
> +};
> +
> +static void r8a7795_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
> +{
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,  CTSR_THBGR);
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,  0x0);
> +
> +	usleep_range(1000, 2000);
> +
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR, CTSR_PONM);
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQCTL, 0x3F);
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,
> +				CTSR_PONM | CTSR_AOUT | CTSR_THBGR | CTSR_VMEN);
> +
> +	usleep_range(100, 200);
> +
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,
> +				CTSR_PONM | CTSR_AOUT | CTSR_THBGR | CTSR_VMEN |
> +				CTSR_VMST | CTSR_THSST);
> +}
> +
> +static void r8a7796_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
> +{
> +	u32 reg_val;
> +
> +	reg_val = rcar_gen3_thermal_read(tsc, REG_GEN3_THCTR);
> +	reg_val &= ~THCTR_PONM;
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_THCTR, reg_val);
> +
> +	usleep_range(1000, 2000);
> +
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQCTL, 0x3F);
> +	reg_val = rcar_gen3_thermal_read(tsc, REG_GEN3_THCTR);
> +	reg_val |= THCTR_THSST;
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_THCTR, reg_val);
> +}
> +
> +static const struct rcar_gen3_thermal_data r8a7795_data = {
> +	.thermal_init = r8a7795_thermal_init,
> +};
> +
> +static const struct rcar_gen3_thermal_data r8a7796_data = {
> +	.thermal_init = r8a7796_thermal_init,
> +};
> +
> +static const struct of_device_id rcar_gen3_thermal_dt_ids[] = {
> +	{ .compatible = "renesas,r8a7795-thermal", .data = &r8a7795_data},
> +	{ .compatible = "renesas,r8a7796-thermal", .data = &r8a7796_data},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, rcar_gen3_thermal_dt_ids);
> +
> +static int rcar_gen3_thermal_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +
> +	pm_runtime_put(dev);
> +	pm_runtime_disable(dev);
> +
> +	return 0;
> +}
> +
> +static int rcar_gen3_thermal_probe(struct platform_device *pdev)
> +{
> +	struct rcar_gen3_thermal_priv *priv;
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	struct thermal_zone_device *zone;
> +	int ret, i;
> +	const struct rcar_gen3_thermal_data *match_data =
> +		of_device_get_match_data(dev);
> +
> +	/* default values if FUSEs are missing */
> +	/* TODO: Read values from hardware on supported platforms */
> +	int ptat[3] = { 2351, 1509, 435 };
> +	int thcode[TSC_MAX_NUM][3] = {
> +		{ 3248, 2800, 2221 },
> +		{ 3245, 2795, 2216 },
> +		{ 3250, 2805, 2237 },
> +	};

Now that I understand a bit what is going on here, I believe the above
hard coded values may be moved to DT, as long as you are willing to
describe more than one sensor and attach each one thermal zone.

> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	pm_runtime_enable(dev);
> +	pm_runtime_get_sync(dev);
> +
> +	for (i = 0; i < TSC_MAX_NUM; i++) {
> +		struct rcar_gen3_thermal_tsc *tsc;
> +
> +		tsc = devm_kzalloc(dev, sizeof(*tsc), GFP_KERNEL);
> +		if (!tsc) {
> +			ret = -ENOMEM;
> +			goto error_unregister;
> +		}
> +
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> +		if (!res)
> +			break;
> +
> +		tsc->base = devm_ioremap_resource(dev, res);
> +		if (IS_ERR(tsc->base)) {
> +			ret = PTR_ERR(tsc->base);
> +			goto error_unregister;
> +		}
> +
> +		priv->tscs[i] = tsc;
> +		mutex_init(&tsc->lock);
> +
> +		match_data->thermal_init(tsc);
> +		rcar_gen3_thermal_calc_coefs(&tsc->coef, ptat, thcode[i]);

oh, the thcode's are currently not read then?

> +
> +		zone = devm_thermal_zone_of_sensor_register(dev, i, tsc,
> +							    &rcar_gen3_tz_of_ops);

and you are already doing it, therefore those coeff could really come
from the DT, no?

> +		if (IS_ERR(zone)) {
> +			dev_err(dev, "Can't register thermal zone\n");
> +			ret = PTR_ERR(zone);
> +			goto error_unregister;
> +		}
> +		tsc->zone = zone;
> +	}
> +
> +	return 0;
> +
> +error_unregister:
> +	rcar_gen3_thermal_remove(pdev);
> +
> +	return ret;
> +}
> +
> +static struct platform_driver rcar_gen3_thermal_driver = {
> +	.driver	= {
> +		.name	= "rcar_gen3_thermal",
> +		.of_match_table = rcar_gen3_thermal_dt_ids,
> +	},
> +	.probe		= rcar_gen3_thermal_probe,
> +	.remove		= rcar_gen3_thermal_remove,
> +};
> +module_platform_driver(rcar_gen3_thermal_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("R-Car Gen3 THS thermal sensor driver");
> +MODULE_AUTHOR("Wolfram Sang <wsa+renesas@sang-engineering.com>");
> -- 
> 2.10.2
> 

Re: [PATCHv5 2/5] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver

[Eduardo Valentin]

Hey,

On Mon, Dec 12, 2016 at 03:18:02PM +0100, Niklas Söderlund wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Add support for R-Car Gen3 thermal sensors. Polling only for now,
> interrupts will be added incrementally. Same goes for reading fuses.
> This is documented already, but no hardware available for now.
> 
> Signed-off-by: Hien Dang <hien.dang.eb@renesas.com>
> Signed-off-by: Thao Nguyen <thao.nguyen.yb@rvc.renesas.com>
> Signed-off-by: Khiem Nguyen <khiem.nguyen.xt@renesas.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> [Niklas: document and rework temperature calculation]
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/thermal/Kconfig             |   9 +
>  drivers/thermal/Makefile            |   1 +
>  drivers/thermal/rcar_gen3_thermal.c | 328 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 338 insertions(+)
>  create mode 100644 drivers/thermal/rcar_gen3_thermal.c
> 
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index a13541b..da71f94 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -243,6 +243,15 @@ config RCAR_THERMAL
>  	  Enable this to plug the R-Car thermal sensor driver into the Linux
>  	  thermal framework.
>  
> +config RCAR_GEN3_THERMAL
> +	tristate "Renesas R-Car Gen3 thermal driver"
> +	depends on ARCH_RENESAS || COMPILE_TEST

cool that we have got COMPILE_TEST to work again!

thanks for that, I appreciate it.

> +	depends on HAS_IOMEM
> +	depends on OF
> +	help
> +	  Enable this to plug the R-Car Gen3 thermal sensor driver into the Linux
> +	  thermal framework.
> +
>  config KIRKWOOD_THERMAL
>  	tristate "Temperature sensor on Marvell Kirkwood SoCs"
>  	depends on MACH_KIRKWOOD || COMPILE_TEST
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index c92eb22..1216fb3 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -30,6 +30,7 @@ obj-$(CONFIG_QCOM_SPMI_TEMP_ALARM)	+= qcom-spmi-temp-alarm.o
>  obj-$(CONFIG_SPEAR_THERMAL)	+= spear_thermal.o
>  obj-$(CONFIG_ROCKCHIP_THERMAL)	+= rockchip_thermal.o
>  obj-$(CONFIG_RCAR_THERMAL)	+= rcar_thermal.o
> +obj-$(CONFIG_RCAR_GEN3_THERMAL)	+= rcar_gen3_thermal.o
>  obj-$(CONFIG_KIRKWOOD_THERMAL)  += kirkwood_thermal.o
>  obj-y				+= samsung/
>  obj-$(CONFIG_DOVE_THERMAL)  	+= dove_thermal.o
> diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> new file mode 100644
> index 0000000..7d78498
> --- /dev/null
> +++ b/drivers/thermal/rcar_gen3_thermal.c
> @@ -0,0 +1,328 @@
> +/*
> + *  R-Car Gen3 THS thermal sensor driver
> + *  Based on rcar_thermal.c and work from Hien Dang and Khiem Nguyen.
> + *
> + * Copyright (C) 2016 Renesas Electronics Corporation.
> + * Copyright (C) 2016 Sang Engineering
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; version 2 of the License.
> + *
> + *  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.
> + *
> + */
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/thermal.h>
> +
> +/* Register offsets */
> +#define REG_GEN3_IRQSTR		0x04
> +#define REG_GEN3_IRQMSK		0x08
> +#define REG_GEN3_IRQCTL		0x0C
> +#define REG_GEN3_IRQEN		0x10
> +#define REG_GEN3_IRQTEMP1	0x14
> +#define REG_GEN3_IRQTEMP2	0x18
> +#define REG_GEN3_IRQTEMP3	0x1C
> +#define REG_GEN3_CTSR		0x20
> +#define REG_GEN3_THCTR		0x20
> +#define REG_GEN3_TEMP		0x28
> +#define REG_GEN3_THCODE1	0x50
> +#define REG_GEN3_THCODE2	0x54
> +#define REG_GEN3_THCODE3	0x58
> +
> +/* CTSR bits */
> +#define CTSR_PONM	BIT(8)
> +#define CTSR_AOUT	BIT(7)
> +#define CTSR_THBGR	BIT(5)
> +#define CTSR_VMEN	BIT(4)
> +#define CTSR_VMST	BIT(1)
> +#define CTSR_THSST	BIT(0)
> +
> +/* THCTR bits */
> +#define THCTR_PONM	BIT(6)
> +#define THCTR_THSST	BIT(0)
> +
> +#define CTEMP_MASK	0xFFF
> +
> +#define MCELSIUS(temp)	((temp) * 1000)
> +#define GEN3_FUSE_MASK	0xFFF
> +
> +#define TSC_MAX_NUM	3
> +
> +/* Structure for thermal temperature calculation */
> +struct equation_coefs {
> +	int a1;
> +	int b1;
> +	int a2;
> +	int b2;

a little description of each coeff would be welcome.

> +};
> +
> +struct rcar_gen3_thermal_tsc {
> +	void __iomem *base;
> +	struct thermal_zone_device *zone;
> +	struct equation_coefs coef;
> +	struct mutex lock;
> +};
> +
> +struct rcar_gen3_thermal_priv {
> +	struct rcar_gen3_thermal_tsc *tscs[TSC_MAX_NUM];
> +};
> +
> +struct rcar_gen3_thermal_data {
> +	void (*thermal_init)(struct rcar_gen3_thermal_tsc *tsc);
> +};
> +
> +static inline u32 rcar_gen3_thermal_read(struct rcar_gen3_thermal_tsc *tsc,
> +					 u32 reg)
> +{
> +	return ioread32(tsc->base + reg);
> +}
> +
> +static inline void rcar_gen3_thermal_write(struct rcar_gen3_thermal_tsc *tsc,
> +					   u32 reg, u32 data)
> +{
> +	iowrite32(data, tsc->base + reg);
> +}
> +
> +/*
> + * Linear approximation for temperature
> + *
> + * [reg] = [temp] * a + b => [temp] = ([reg] - b) / a

the type of equation described above can be described using
of-thermal/DT. Have you tried using the coefficients DT property to get
those from DT?

> + *
> + * The constants a and b are calculated using two triplets of int values PTAT
> + * and THCODE. PTAT and THCODE can either be read from hardware or use hard
> + * coded values from driver. The formula to calculate a and b are taken from
> + * BSP and sparsely documented and understood.
> + *

hmmm.. OK. that gets a bit more interesting. 

So you can get a and b queried from hardware. cool.

but you can also get those hardcoded in the code. In that case, I would
suggest hardcode them in DT, instead, using the coefficients property.

> + * Examining the linear formula and the formula used to calculate constants a
> + * and b while knowing that the span for PTAT and THCODE values are between
> + * 0x000 and 0xfff the largest integer possible is 0xfff * 0xfff == 0xffe001.
> + * Integer also needs to be signed so that leaves 7 bits for decimal
> + * fixed point scaling, which amounts to a decimal scaling factor of 100.
> + */


I see.

> +
> +#define SCALE_FACTOR 100
> +#define SCALE_INT(_x) ((_x) * SCALE_FACTOR)
> +#define SCALE_MUL(_a, _b) (((_a)*(_b)) / SCALE_FACTOR)
> +#define SCALE_DIV(_a, _b) (((_a)*SCALE_FACTOR)/(_b))
> +#define SCALE_TO_MCELSIUS(_x) ((_x) * 10)
> +
> +#define RCAR3_THERMAL_GRAN 500 /* mili Celsius */
> +
> +/* no idea where these constants come from */

thermal simulation, maybe?

> +#define TJ_1 SCALE_INT(96)
> +#define TJ_3 SCALE_INT(-41)
> +
> +static void rcar_gen3_thermal_calc_coefs(struct equation_coefs *coef,
> +					 int *ptat, int *thcode)
> +{
> +	int tj_2;
> +
> +	/* TODO: Find documentation and document constant calculation formula */
> +
> +	tj_2 = SCALE_DIV(SCALE_MUL(SCALE_INT(ptat[1] - ptat[2]), SCALE_INT(137)),
> +			 SCALE_INT(ptat[0] - ptat[2])) - SCALE_INT(41);
> +
> +	coef->a1 = SCALE_DIV(SCALE_INT(thcode[1] - thcode[2]), tj_2 - TJ_3);
> +	coef->b1 = SCALE_INT(thcode[2]) - SCALE_MUL(coef->a1, TJ_3);
> +
> +	coef->a2 = SCALE_DIV(SCALE_INT(thcode[1] - thcode[0]), tj_2 - TJ_1);
> +	coef->b2 = SCALE_INT(thcode[0]) - SCALE_MUL(coef->a2, TJ_1);
> +}
> +
> +static int rcar_gen3_thermal_round(int temp)
> +{
> +	int result, round_offs;
> +
> +	round_offs = temp >= 0 ? RCAR3_THERMAL_GRAN / 2 :
> +		-RCAR3_THERMAL_GRAN / 2;
> +	result = (temp + round_offs) / RCAR3_THERMAL_GRAN;
> +	return result * RCAR3_THERMAL_GRAN;
> +}
> +
> +static int rcar_gen3_thermal_get_temp(void *devdata, int *temp)
> +{
> +	struct rcar_gen3_thermal_tsc *tsc = devdata;
> +	int mcelsius, val1, val2;
> +	u32 reg;
> +
> +	/* Read register and convert to mili Celsius */
> +	mutex_lock(&tsc->lock);
> +
> +	reg = rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK;
> +
> +	val1 = SCALE_DIV(SCALE_INT(reg) - tsc->coef.b1, tsc->coef.a1);
> +	val2 = SCALE_DIV(SCALE_INT(reg) - tsc->coef.b2, tsc->coef.a2);

I see. there are actually two sensors here, and 

> +	mcelsius = SCALE_TO_MCELSIUS((val1 + val2) / 2);

the driver always output the average of them.

based on previous driver history, people would actually expose all
sensors. Unless you want to be really strict to always average the two.

which to me seams a bit odd also, based on other drivers I have seen.
Hardware folks would typically ask for caring about all sensors. For
example, care about max, not average. But average is also fine, as long
as temperature does not move fast enough in one of the sides.

For example, you see a spike on one, and the other, still fine (taking
some thermal resistance before seen the heat), you would already take an
action. And here you will first average (filter the spike out), before
taking any action. That essentially means, you are delaying any action
further until the rise is seen by the average / filter.

Is that really what you want?

> +
> +	mutex_unlock(&tsc->lock);
> +
> +	/* Make sure we are inside specifications */
> +	if ((mcelsius < MCELSIUS(-40)) || (mcelsius > MCELSIUS(125)))
> +		return -EIO;
> +
> +	/* Round value to device granularity setting */
> +	*temp = rcar_gen3_thermal_round(mcelsius);
> +
> +	return 0;
> +}
> +
> +static struct thermal_zone_of_device_ops rcar_gen3_tz_of_ops = {
> +	.get_temp	= rcar_gen3_thermal_get_temp,
> +};
> +
> +static void r8a7795_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
> +{
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,  CTSR_THBGR);
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,  0x0);
> +
> +	usleep_range(1000, 2000);
> +
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR, CTSR_PONM);
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQCTL, 0x3F);
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,
> +				CTSR_PONM | CTSR_AOUT | CTSR_THBGR | CTSR_VMEN);
> +
> +	usleep_range(100, 200);
> +
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,
> +				CTSR_PONM | CTSR_AOUT | CTSR_THBGR | CTSR_VMEN |
> +				CTSR_VMST | CTSR_THSST);
> +}
> +
> +static void r8a7796_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
> +{
> +	u32 reg_val;
> +
> +	reg_val = rcar_gen3_thermal_read(tsc, REG_GEN3_THCTR);
> +	reg_val &= ~THCTR_PONM;
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_THCTR, reg_val);
> +
> +	usleep_range(1000, 2000);
> +
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQCTL, 0x3F);
> +	reg_val = rcar_gen3_thermal_read(tsc, REG_GEN3_THCTR);
> +	reg_val |= THCTR_THSST;
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_THCTR, reg_val);
> +}
> +
> +static const struct rcar_gen3_thermal_data r8a7795_data = {
> +	.thermal_init = r8a7795_thermal_init,
> +};
> +
> +static const struct rcar_gen3_thermal_data r8a7796_data = {
> +	.thermal_init = r8a7796_thermal_init,
> +};
> +
> +static const struct of_device_id rcar_gen3_thermal_dt_ids[] = {
> +	{ .compatible = "renesas,r8a7795-thermal", .data = &r8a7795_data},
> +	{ .compatible = "renesas,r8a7796-thermal", .data = &r8a7796_data},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, rcar_gen3_thermal_dt_ids);
> +
> +static int rcar_gen3_thermal_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +
> +	pm_runtime_put(dev);
> +	pm_runtime_disable(dev);
> +
> +	return 0;
> +}
> +
> +static int rcar_gen3_thermal_probe(struct platform_device *pdev)
> +{
> +	struct rcar_gen3_thermal_priv *priv;
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	struct thermal_zone_device *zone;
> +	int ret, i;
> +	const struct rcar_gen3_thermal_data *match_data =
> +		of_device_get_match_data(dev);
> +
> +	/* default values if FUSEs are missing */
> +	/* TODO: Read values from hardware on supported platforms */
> +	int ptat[3] = { 2351, 1509, 435 };
> +	int thcode[TSC_MAX_NUM][3] = {
> +		{ 3248, 2800, 2221 },
> +		{ 3245, 2795, 2216 },
> +		{ 3250, 2805, 2237 },
> +	};

Now that I understand a bit what is going on here, I believe the above
hard coded values may be moved to DT, as long as you are willing to
describe more than one sensor and attach each one thermal zone.

> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	pm_runtime_enable(dev);
> +	pm_runtime_get_sync(dev);
> +
> +	for (i = 0; i < TSC_MAX_NUM; i++) {
> +		struct rcar_gen3_thermal_tsc *tsc;
> +
> +		tsc = devm_kzalloc(dev, sizeof(*tsc), GFP_KERNEL);
> +		if (!tsc) {
> +			ret = -ENOMEM;
> +			goto error_unregister;
> +		}
> +
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> +		if (!res)
> +			break;
> +
> +		tsc->base = devm_ioremap_resource(dev, res);
> +		if (IS_ERR(tsc->base)) {
> +			ret = PTR_ERR(tsc->base);
> +			goto error_unregister;
> +		}
> +
> +		priv->tscs[i] = tsc;
> +		mutex_init(&tsc->lock);
> +
> +		match_data->thermal_init(tsc);
> +		rcar_gen3_thermal_calc_coefs(&tsc->coef, ptat, thcode[i]);

oh, the thcode's are currently not read then?

> +
> +		zone = devm_thermal_zone_of_sensor_register(dev, i, tsc,
> +							    &rcar_gen3_tz_of_ops);

and you are already doing it, therefore those coeff could really come
from the DT, no?

> +		if (IS_ERR(zone)) {
> +			dev_err(dev, "Can't register thermal zone\n");
> +			ret = PTR_ERR(zone);
> +			goto error_unregister;
> +		}
> +		tsc->zone = zone;
> +	}
> +
> +	return 0;
> +
> +error_unregister:
> +	rcar_gen3_thermal_remove(pdev);
> +
> +	return ret;
> +}
> +
> +static struct platform_driver rcar_gen3_thermal_driver = {
> +	.driver	= {
> +		.name	= "rcar_gen3_thermal",
> +		.of_match_table = rcar_gen3_thermal_dt_ids,
> +	},
> +	.probe		= rcar_gen3_thermal_probe,
> +	.remove		= rcar_gen3_thermal_remove,
> +};
> +module_platform_driver(rcar_gen3_thermal_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("R-Car Gen3 THS thermal sensor driver");
> +MODULE_AUTHOR("Wolfram Sang <wsa+renesas@sang-engineering.com>");
> -- 
> 2.10.2
> 

Re: [PATCHv5 2/5] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver

[Wolfram Sang]

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

Hi,

> > + *
> > + * The constants a and b are calculated using two triplets of int values PTAT
> > + * and THCODE. PTAT and THCODE can either be read from hardware or use hard
> > + * coded values from driver. The formula to calculate a and b are taken from
> > + * BSP and sparsely documented and understood.
> > + *
> 
> hmmm.. OK. that gets a bit more interesting. 
> 
> So you can get a and b queried from hardware. cool.
> 
> but you can also get those hardcoded in the code. In that case, I would
> suggest hardcode them in DT, instead, using the coefficients property.

It is only the engineering samples which have the coeffs hardcoded. All
future revisions of the same SoC will have the values as fuses from
registers. Sadly, we won't have access to newer versions for a while. To
avoid having seperate DTSI per engineering sample revision, I planned
for using the new soc_device_match() mechanism which was introduced for
exactly such use cases.

> > +static int rcar_gen3_thermal_get_temp(void *devdata, int *temp)
> > +{
> > +	struct rcar_gen3_thermal_tsc *tsc = devdata;
> > +	int mcelsius, val1, val2;
> > +	u32 reg;
> > +
> > +	/* Read register and convert to mili Celsius */
> > +	mutex_lock(&tsc->lock);
> > +
> > +	reg = rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK;
> > +
> > +	val1 = SCALE_DIV(SCALE_INT(reg) - tsc->coef.b1, tsc->coef.a1);
> > +	val2 = SCALE_DIV(SCALE_INT(reg) - tsc->coef.b2, tsc->coef.a2);
> 
> I see. there are actually two sensors here, and 

Really? val1 and val2 are both based on 'reg'. Also, the datasheet
explicitly mentions 'three sensors in the LSI'.

Thanks for the fast response!

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCHv5 2/5] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver

[Geert Uytterhoeven]

Hi Niklas,

On Mon, Dec 12, 2016 at 3:18 PM, Niklas Söderlund
<niklas.soderlund@ragnatech.se> wrote:
> +/*
> + * Linear approximation for temperature
> + *
> + * [reg] = [temp] * a + b => [temp] = ([reg] - b) / a
> + *
> + * The constants a and b are calculated using two triplets of int values PTAT
> + * and THCODE. PTAT and THCODE can either be read from hardware or use hard
> + * coded values from driver. The formula to calculate a and b are taken from
> + * BSP and sparsely documented and understood.
> + *
> + * Examining the linear formula and the formula used to calculate constants a
> + * and b while knowing that the span for PTAT and THCODE values are between
> + * 0x000 and 0xfff the largest integer possible is 0xfff * 0xfff == 0xffe001.
> + * Integer also needs to be signed so that leaves 7 bits for decimal
> + * fixed point scaling, which amounts to a decimal scaling factor of 100.
> + */
> +
> +#define SCALE_FACTOR 100

What about using 128 instead?
Fixed point is much easier with shifts
(the compiler will turn multiplications in shifts where appropriate).

> +#define SCALE_INT(_x) ((_x) * SCALE_FACTOR)
> +#define SCALE_MUL(_a, _b) (((_a)*(_b)) / SCALE_FACTOR)
> +#define SCALE_DIV(_a, _b) (((_a)*SCALE_FACTOR)/(_b))

DIV_ROUND_CLOSEST()

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCHv5 2/5] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver

[Geert Uytterhoeven]

On Mon, Dec 12, 2016 at 10:45 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>> +/*
>> + * Linear approximation for temperature
>> + *
>> + * [reg] = [temp] * a + b => [temp] = ([reg] - b) / a
>> + *
>> + * The constants a and b are calculated using two triplets of int values PTAT
>> + * and THCODE. PTAT and THCODE can either be read from hardware or use hard
>> + * coded values from driver. The formula to calculate a and b are taken from
>> + * BSP and sparsely documented and understood.
>> + *
>> + * Examining the linear formula and the formula used to calculate constants a
>> + * and b while knowing that the span for PTAT and THCODE values are between
>> + * 0x000 and 0xfff the largest integer possible is 0xfff * 0xfff == 0xffe001.
>> + * Integer also needs to be signed so that leaves 7 bits for decimal
>> + * fixed point scaling, which amounts to a decimal scaling factor of 100.
>> + */
>> +
>> +#define SCALE_FACTOR 100
>> +#define SCALE_INT(_x) ((_x) * SCALE_FACTOR)
>> +#define SCALE_MUL(_a, _b) (((_a)*(_b)) / SCALE_FACTOR)
>> +#define SCALE_DIV(_a, _b) (((_a)*SCALE_FACTOR)/(_b))
>> +#define SCALE_TO_MCELSIUS(_x) ((_x) * 10)
>
> Spaces around operators everywhere, please.
>
> I wonder about SCALE_MUL; isn't that more like "unscaling" because _a
> and _b are already scaled?

No, it's a standard fixed point multiplication, where you have to compensate
for the double scaling due to the multiplication.

Perhaps the macros should be called e.g. FIXPT_MUL() and FIXPT_DIV()?

> And since _b is always a constant, couldn't we simply drop this macro
> and simply do _a * _b (with _a being scaled already and _b not)?

Yes, for multiplication by an integer (not a fixed point number), you can
just do the multiplication. Which also avoids having to care about rounding.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCHv5 2/5] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver

[Wolfram Sang]

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


> Perhaps the macros should be called e.g. FIXPT_MUL() and FIXPT_DIV()?

Yes, that naming would be more readable to me.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCHv5 5/5] thermal: rcar_gen3_thermal: Add delay in .thermal_init on r8a7795

[Niklas Söderlund]

On 2016-12-12 19:31:26 -0800, Eduardo Valentin wrote:
> On Mon, Dec 12, 2016 at 03:18:05PM +0100, Niklas S�derlund wrote:
> > From: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> > 
> > The .thermal_init needs to be delayed a short amount of time after
> > setting REG_GEN3_CTSR to allow for the TEMP register to contain
> > something useful. If it's not delayed theses warnings are common during
> > boot:
> > 
> > thermal thermal_zone0: failed to read out thermal zone (-5)
> > thermal thermal_zone1: failed to read out thermal zone (-5)
> > thermal thermal_zone2: failed to read out thermal zone (-5)
> > 
> > The warnings are triggered by the first call to .get_temp while the TEMP
> > register contains 0 and rcar_gen3_thermal_get_temp() returns -EIO since
> > a TEMP value of 0 will result in a temperature reading which is out of
> > specifications.
> 
> I think this one can be folded on patch 2/5.

OK, will do this in next version.

-- 
Regards,
Niklas S�derlund

Re: [PATCHv5 5/5] thermal: rcar_gen3_thermal: Add delay in .thermal_init on r8a7795

[Niklas Söderlund]

On 2016-12-12 19:31:26 -0800, Eduardo Valentin wrote:
> On Mon, Dec 12, 2016 at 03:18:05PM +0100, Niklas Söderlund wrote:
> > From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > 
> > The .thermal_init needs to be delayed a short amount of time after
> > setting REG_GEN3_CTSR to allow for the TEMP register to contain
> > something useful. If it's not delayed theses warnings are common during
> > boot:
> > 
> > thermal thermal_zone0: failed to read out thermal zone (-5)
> > thermal thermal_zone1: failed to read out thermal zone (-5)
> > thermal thermal_zone2: failed to read out thermal zone (-5)
> > 
> > The warnings are triggered by the first call to .get_temp while the TEMP
> > register contains 0 and rcar_gen3_thermal_get_temp() returns -EIO since
> > a TEMP value of 0 will result in a temperature reading which is out of
> > specifications.
> 
> I think this one can be folded on patch 2/5.

OK, will do this in next version.

-- 
Regards,
Niklas Söderlund

Re: [PATCHv5 2/5] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver

[Niklas Söderlund]

Hi Eduardo,

Thanks for your feedback. I will skip commenting where Wolfram already 
have.

On 2016-12-12 19:50:54 -0800, Eduardo Valentin wrote:

<snip>

> > +/* Structure for thermal temperature calculation */
> > +struct equation_coefs {
> > +	int a1;
> > +	int b1;
> > +	int a2;
> > +	int b2;
> 
> a little description of each coeff would be welcome.

Yes, I too would like to have better documentation of the formulas and 
the meaning of the coefficients.

<snip, Wolfram already covert this>

> 
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	platform_set_drvdata(pdev, priv);
> > +
> > +	pm_runtime_enable(dev);
> > +	pm_runtime_get_sync(dev);
> > +
> > +	for (i = 0; i < TSC_MAX_NUM; i++) {
> > +		struct rcar_gen3_thermal_tsc *tsc;
> > +
> > +		tsc = devm_kzalloc(dev, sizeof(*tsc), GFP_KERNEL);
> > +		if (!tsc) {
> > +			ret = -ENOMEM;
> > +			goto error_unregister;
> > +		}
> > +
> > +		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> > +		if (!res)
> > +			break;
> > +
> > +		tsc->base = devm_ioremap_resource(dev, res);
> > +		if (IS_ERR(tsc->base)) {
> > +			ret = PTR_ERR(tsc->base);
> > +			goto error_unregister;
> > +		}
> > +
> > +		priv->tscs[i] = tsc;
> > +		mutex_init(&tsc->lock);
> > +
> > +		match_data->thermal_init(tsc);
> > +		rcar_gen3_thermal_calc_coefs(&tsc->coef, ptat, thcode[i]);
> 
> oh, the thcode's are currently not read then?

No as Wolfram commented, reading THCODE and PTAT from hardware is not 
possible on the boards we have to test on so I think this needs to be 
added once we can test it. Do you agree this is the best option?

> 
> > +
> > +		zone = devm_thermal_zone_of_sensor_register(dev, i, tsc,
> > +							    &rcar_gen3_tz_of_ops);
> 
> and you are already doing it, therefore those coeff could really come
> from the DT, no?
> 
> > +		if (IS_ERR(zone)) {
> > +			dev_err(dev, "Can't register thermal zone\n");
> > +			ret = PTR_ERR(zone);
> > +			goto error_unregister;
> > +		}
> > +		tsc->zone = zone;
> > +	}
> > +
> > +	return 0;
> > +
> > +error_unregister:
> > +	rcar_gen3_thermal_remove(pdev);
> > +
> > +	return ret;
> > +}
> > +
> > +static struct platform_driver rcar_gen3_thermal_driver = {
> > +	.driver	= {
> > +		.name	= "rcar_gen3_thermal",
> > +		.of_match_table = rcar_gen3_thermal_dt_ids,
> > +	},
> > +	.probe		= rcar_gen3_thermal_probe,
> > +	.remove		= rcar_gen3_thermal_remove,
> > +};
> > +module_platform_driver(rcar_gen3_thermal_driver);
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("R-Car Gen3 THS thermal sensor driver");
> > +MODULE_AUTHOR("Wolfram Sang <wsa+renesas@sang-engineering.com>");
> > -- 
> > 2.10.2
> > 

-- 
Regards,
Niklas S�derlund

Re: [PATCHv5 2/5] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver

[Niklas Söderlund]

Hi Eduardo,

Thanks for your feedback. I will skip commenting where Wolfram already 
have.

On 2016-12-12 19:50:54 -0800, Eduardo Valentin wrote:

<snip>

> > +/* Structure for thermal temperature calculation */
> > +struct equation_coefs {
> > +	int a1;
> > +	int b1;
> > +	int a2;
> > +	int b2;
> 
> a little description of each coeff would be welcome.

Yes, I too would like to have better documentation of the formulas and 
the meaning of the coefficients.

<snip, Wolfram already covert this>

> 
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	platform_set_drvdata(pdev, priv);
> > +
> > +	pm_runtime_enable(dev);
> > +	pm_runtime_get_sync(dev);
> > +
> > +	for (i = 0; i < TSC_MAX_NUM; i++) {
> > +		struct rcar_gen3_thermal_tsc *tsc;
> > +
> > +		tsc = devm_kzalloc(dev, sizeof(*tsc), GFP_KERNEL);
> > +		if (!tsc) {
> > +			ret = -ENOMEM;
> > +			goto error_unregister;
> > +		}
> > +
> > +		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> > +		if (!res)
> > +			break;
> > +
> > +		tsc->base = devm_ioremap_resource(dev, res);
> > +		if (IS_ERR(tsc->base)) {
> > +			ret = PTR_ERR(tsc->base);
> > +			goto error_unregister;
> > +		}
> > +
> > +		priv->tscs[i] = tsc;
> > +		mutex_init(&tsc->lock);
> > +
> > +		match_data->thermal_init(tsc);
> > +		rcar_gen3_thermal_calc_coefs(&tsc->coef, ptat, thcode[i]);
> 
> oh, the thcode's are currently not read then?

No as Wolfram commented, reading THCODE and PTAT from hardware is not 
possible on the boards we have to test on so I think this needs to be 
added once we can test it. Do you agree this is the best option?

> 
> > +
> > +		zone = devm_thermal_zone_of_sensor_register(dev, i, tsc,
> > +							    &rcar_gen3_tz_of_ops);
> 
> and you are already doing it, therefore those coeff could really come
> from the DT, no?
> 
> > +		if (IS_ERR(zone)) {
> > +			dev_err(dev, "Can't register thermal zone\n");
> > +			ret = PTR_ERR(zone);
> > +			goto error_unregister;
> > +		}
> > +		tsc->zone = zone;
> > +	}
> > +
> > +	return 0;
> > +
> > +error_unregister:
> > +	rcar_gen3_thermal_remove(pdev);
> > +
> > +	return ret;
> > +}
> > +
> > +static struct platform_driver rcar_gen3_thermal_driver = {
> > +	.driver	= {
> > +		.name	= "rcar_gen3_thermal",
> > +		.of_match_table = rcar_gen3_thermal_dt_ids,
> > +	},
> > +	.probe		= rcar_gen3_thermal_probe,
> > +	.remove		= rcar_gen3_thermal_remove,
> > +};
> > +module_platform_driver(rcar_gen3_thermal_driver);
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("R-Car Gen3 THS thermal sensor driver");
> > +MODULE_AUTHOR("Wolfram Sang <wsa+renesas@sang-engineering.com>");
> > -- 
> > 2.10.2
> > 

-- 
Regards,
Niklas Söderlund

Re: [PATCHv5 2/5] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver

[Niklas Söderlund]

Hi Geert,

Thanks for your feedback.

On 2016-12-13 09:19:53 +0100, Geert Uytterhoeven wrote:
> Hi Niklas,
> 
> On Mon, Dec 12, 2016 at 3:18 PM, Niklas S�derlund
> <niklas.soderlund@ragnatech.se> wrote:
> > +/*
> > + * Linear approximation for temperature
> > + *
> > + * [reg] = [temp] * a + b => [temp] = ([reg] - b) / a
> > + *
> > + * The constants a and b are calculated using two triplets of int values PTAT
> > + * and THCODE. PTAT and THCODE can either be read from hardware or use hard
> > + * coded values from driver. The formula to calculate a and b are taken from
> > + * BSP and sparsely documented and understood.
> > + *
> > + * Examining the linear formula and the formula used to calculate constants a
> > + * and b while knowing that the span for PTAT and THCODE values are between
> > + * 0x000 and 0xfff the largest integer possible is 0xfff * 0xfff == 0xffe001.
> > + * Integer also needs to be signed so that leaves 7 bits for decimal
> > + * fixed point scaling, which amounts to a decimal scaling factor of 100.
> > + */
> > +
> > +#define SCALE_FACTOR 100
> 
> What about using 128 instead?
> Fixed point is much easier with shifts
> (the compiler will turn multiplications in shifts where appropriate).

I tried using binary scaled instead of decimal scaled fixed point but 
noticed that the diff I got compared to the original formula which used 
decimal scaled (factor 1000) where larger so I picked decimal scaling.  

Maybe with feedback from Morimoto-san or Khiem we can switch to binary 
scaled number as it should over all increase the accuracy of the 
calculations, but then maybe some of the other hard coded constant also 
should be updated?

> 
> > +#define SCALE_INT(_x) ((_x) * SCALE_FACTOR)
> > +#define SCALE_MUL(_a, _b) (((_a)*(_b)) / SCALE_FACTOR)
> > +#define SCALE_DIV(_a, _b) (((_a)*SCALE_FACTOR)/(_b))
> 
> DIV_ROUND_CLOSEST()

Don't DIV_ROUND_CLOSEST() require the divisor to be a positive integer?  

  tj_2 = SCALE_DIV(SCALE_MUL(SCALE_INT(ptat[1] - ptat[2]), SCALE_INT(137)),
                   SCALE_INT(ptat[0] - ptat[2])) - SCALE_INT(41);

In this case if ptat[0] < (ptat[2] + 41) the divisor is negative so I 
think DIV_ROUND_CLOSEST() can't be used, or am I misunderstanding?

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

-- 
Regards,
Niklas S�derlund

Re: [PATCHv5 2/5] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver

[Niklas Söderlund]

Hi Geert,

Thanks for your feedback.

On 2016-12-13 09:19:53 +0100, Geert Uytterhoeven wrote:
> Hi Niklas,
> 
> On Mon, Dec 12, 2016 at 3:18 PM, Niklas Söderlund
> <niklas.soderlund@ragnatech.se> wrote:
> > +/*
> > + * Linear approximation for temperature
> > + *
> > + * [reg] = [temp] * a + b => [temp] = ([reg] - b) / a
> > + *
> > + * The constants a and b are calculated using two triplets of int values PTAT
> > + * and THCODE. PTAT and THCODE can either be read from hardware or use hard
> > + * coded values from driver. The formula to calculate a and b are taken from
> > + * BSP and sparsely documented and understood.
> > + *
> > + * Examining the linear formula and the formula used to calculate constants a
> > + * and b while knowing that the span for PTAT and THCODE values are between
> > + * 0x000 and 0xfff the largest integer possible is 0xfff * 0xfff == 0xffe001.
> > + * Integer also needs to be signed so that leaves 7 bits for decimal
> > + * fixed point scaling, which amounts to a decimal scaling factor of 100.
> > + */
> > +
> > +#define SCALE_FACTOR 100
> 
> What about using 128 instead?
> Fixed point is much easier with shifts
> (the compiler will turn multiplications in shifts where appropriate).

I tried using binary scaled instead of decimal scaled fixed point but 
noticed that the diff I got compared to the original formula which used 
decimal scaled (factor 1000) where larger so I picked decimal scaling.  

Maybe with feedback from Morimoto-san or Khiem we can switch to binary 
scaled number as it should over all increase the accuracy of the 
calculations, but then maybe some of the other hard coded constant also 
should be updated?

> 
> > +#define SCALE_INT(_x) ((_x) * SCALE_FACTOR)
> > +#define SCALE_MUL(_a, _b) (((_a)*(_b)) / SCALE_FACTOR)
> > +#define SCALE_DIV(_a, _b) (((_a)*SCALE_FACTOR)/(_b))
> 
> DIV_ROUND_CLOSEST()

Don't DIV_ROUND_CLOSEST() require the divisor to be a positive integer?  

  tj_2 = SCALE_DIV(SCALE_MUL(SCALE_INT(ptat[1] - ptat[2]), SCALE_INT(137)),
                   SCALE_INT(ptat[0] - ptat[2])) - SCALE_INT(41);

In this case if ptat[0] < (ptat[2] + 41) the divisor is negative so I 
think DIV_ROUND_CLOSEST() can't be used, or am I misunderstanding?

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

-- 
Regards,
Niklas Söderlund

Re: [PATCHv5 2/5] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver

[Niklas Söderlund]

On 2016-12-13 09:39:04 +0100, Wolfram Sang wrote:
> 
> > Perhaps the macros should be called e.g. FIXPT_MUL() and FIXPT_DIV()?
> 
> Yes, that naming would be more readable to me.
> 

FIXPT_ is a much better prefix, will update in next version. Thanks for 
the suggestion Geert.

-- 
Regards,
Niklas S�derlund

Re: [PATCHv5 2/5] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver

[Niklas Söderlund]

On 2016-12-13 09:39:04 +0100, Wolfram Sang wrote:
> 
> > Perhaps the macros should be called e.g. FIXPT_MUL() and FIXPT_DIV()?
> 
> Yes, that naming would be more readable to me.
> 

FIXPT_ is a much better prefix, will update in next version. Thanks for 
the suggestion Geert.

-- 
Regards,
Niklas Söderlund

Re: [PATCHv5 2/5] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver

[Geert Uytterhoeven]

Hi Niklas,

On Tue, Dec 13, 2016 at 10:53 AM, Niklas Söderlund
<niklas.soderlund@ragnatech.se> wrote:
> On 2016-12-13 09:19:53 +0100, Geert Uytterhoeven wrote:
>> On Mon, Dec 12, 2016 at 3:18 PM, Niklas Söderlund
>> <niklas.soderlund@ragnatech.se> wrote:
>> > +/*
>> > + * Linear approximation for temperature
>> > + *
>> > + * [reg] = [temp] * a + b => [temp] = ([reg] - b) / a
>> > + *
>> > + * The constants a and b are calculated using two triplets of int values PTAT
>> > + * and THCODE. PTAT and THCODE can either be read from hardware or use hard
>> > + * coded values from driver. The formula to calculate a and b are taken from
>> > + * BSP and sparsely documented and understood.
>> > + *
>> > + * Examining the linear formula and the formula used to calculate constants a
>> > + * and b while knowing that the span for PTAT and THCODE values are between
>> > + * 0x000 and 0xfff the largest integer possible is 0xfff * 0xfff == 0xffe001.
>> > + * Integer also needs to be signed so that leaves 7 bits for decimal
>> > + * fixed point scaling, which amounts to a decimal scaling factor of 100.
>> > + */
>> > +
>> > +#define SCALE_FACTOR 100
>>
>> What about using 128 instead?
>> Fixed point is much easier with shifts
>> (the compiler will turn multiplications in shifts where appropriate).
>
> I tried using binary scaled instead of decimal scaled fixed point but
> noticed that the diff I got compared to the original formula which used
> decimal scaled (factor 1000) where larger so I picked decimal scaling.

Interesting...

>> > +#define SCALE_INT(_x) ((_x) * SCALE_FACTOR)
>> > +#define SCALE_MUL(_a, _b) (((_a)*(_b)) / SCALE_FACTOR)
>> > +#define SCALE_DIV(_a, _b) (((_a)*SCALE_FACTOR)/(_b))
>>
>> DIV_ROUND_CLOSEST()
>
> Don't DIV_ROUND_CLOSEST() require the divisor to be a positive integer?

Right.

>   tj_2 = SCALE_DIV(SCALE_MUL(SCALE_INT(ptat[1] - ptat[2]), SCALE_INT(137)),
>                    SCALE_INT(ptat[0] - ptat[2])) - SCALE_INT(41);
>
> In this case if ptat[0] < (ptat[2] + 41) the divisor is negative so I
> think DIV_ROUND_CLOSEST() can't be used, or am I misunderstanding?

Then we need our own macro that can handle that. Or enhance
DIV_ROUND_CLOSEST().

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCHv5 2/5] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver

[Eduardo Valentin]

hey,

On Tue, Dec 13, 2016 at 10:43:40AM +0100, Niklas S�derlund wrote:
> Hi Eduardo,
> 
> Thanks for your feedback. I will skip commenting where Wolfram already 
> have.
> 
> On 2016-12-12 19:50:54 -0800, Eduardo Valentin wrote:
> 
> <snip>
> 
> > > +/* Structure for thermal temperature calculation */
> > > +struct equation_coefs {
> > > +	int a1;
> > > +	int b1;
> > > +	int a2;
> > > +	int b2;
> > 
> > a little description of each coeff would be welcome.
> 
> Yes, I too would like to have better documentation of the formulas and 
> the meaning of the coefficients.
> 
> <snip, Wolfram already covert this>

Ok..

> 
> > 
> > > +
> > > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > +	if (!priv)
> > > +		return -ENOMEM;
> > > +
> > > +	platform_set_drvdata(pdev, priv);
> > > +
> > > +	pm_runtime_enable(dev);
> > > +	pm_runtime_get_sync(dev);
> > > +
> > > +	for (i = 0; i < TSC_MAX_NUM; i++) {
> > > +		struct rcar_gen3_thermal_tsc *tsc;
> > > +
> > > +		tsc = devm_kzalloc(dev, sizeof(*tsc), GFP_KERNEL);
> > > +		if (!tsc) {
> > > +			ret = -ENOMEM;
> > > +			goto error_unregister;
> > > +		}
> > > +
> > > +		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> > > +		if (!res)
> > > +			break;
> > > +
> > > +		tsc->base = devm_ioremap_resource(dev, res);
> > > +		if (IS_ERR(tsc->base)) {
> > > +			ret = PTR_ERR(tsc->base);
> > > +			goto error_unregister;
> > > +		}
> > > +
> > > +		priv->tscs[i] = tsc;
> > > +		mutex_init(&tsc->lock);
> > > +
> > > +		match_data->thermal_init(tsc);
> > > +		rcar_gen3_thermal_calc_coefs(&tsc->coef, ptat, thcode[i]);
> > 
> > oh, the thcode's are currently not read then?
> 
> No as Wolfram commented, reading THCODE and PTAT from hardware is not 
> possible on the boards we have to test on so I think this needs to be 
> added once we can test it. Do you agree this is the best option?

I agree here. I was under the impression that would be both types of
chips out there, with and without thcode. But looks like, at the end,
only a few engineers will have access to those without it, right?

If you still think those without the support need right support, I would
suggest moving the hardcoded values to DT. Specially if those hardcoded
values change across chip version (so you can better describe using DT).
But, in case you think this population of chips wont grow, then might be
ok to keep the way it is, even though looks not so nice in the code :-).

Re: [PATCHv5 2/5] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver

[Eduardo Valentin]

hey,

On Tue, Dec 13, 2016 at 10:43:40AM +0100, Niklas Söderlund wrote:
> Hi Eduardo,
> 
> Thanks for your feedback. I will skip commenting where Wolfram already 
> have.
> 
> On 2016-12-12 19:50:54 -0800, Eduardo Valentin wrote:
> 
> <snip>
> 
> > > +/* Structure for thermal temperature calculation */
> > > +struct equation_coefs {
> > > +	int a1;
> > > +	int b1;
> > > +	int a2;
> > > +	int b2;
> > 
> > a little description of each coeff would be welcome.
> 
> Yes, I too would like to have better documentation of the formulas and 
> the meaning of the coefficients.
> 
> <snip, Wolfram already covert this>

Ok..

> 
> > 
> > > +
> > > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > +	if (!priv)
> > > +		return -ENOMEM;
> > > +
> > > +	platform_set_drvdata(pdev, priv);
> > > +
> > > +	pm_runtime_enable(dev);
> > > +	pm_runtime_get_sync(dev);
> > > +
> > > +	for (i = 0; i < TSC_MAX_NUM; i++) {
> > > +		struct rcar_gen3_thermal_tsc *tsc;
> > > +
> > > +		tsc = devm_kzalloc(dev, sizeof(*tsc), GFP_KERNEL);
> > > +		if (!tsc) {
> > > +			ret = -ENOMEM;
> > > +			goto error_unregister;
> > > +		}
> > > +
> > > +		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> > > +		if (!res)
> > > +			break;
> > > +
> > > +		tsc->base = devm_ioremap_resource(dev, res);
> > > +		if (IS_ERR(tsc->base)) {
> > > +			ret = PTR_ERR(tsc->base);
> > > +			goto error_unregister;
> > > +		}
> > > +
> > > +		priv->tscs[i] = tsc;
> > > +		mutex_init(&tsc->lock);
> > > +
> > > +		match_data->thermal_init(tsc);
> > > +		rcar_gen3_thermal_calc_coefs(&tsc->coef, ptat, thcode[i]);
> > 
> > oh, the thcode's are currently not read then?
> 
> No as Wolfram commented, reading THCODE and PTAT from hardware is not 
> possible on the boards we have to test on so I think this needs to be 
> added once we can test it. Do you agree this is the best option?

I agree here. I was under the impression that would be both types of
chips out there, with and without thcode. But looks like, at the end,
only a few engineers will have access to those without it, right?

If you still think those without the support need right support, I would
suggest moving the hardcoded values to DT. Specially if those hardcoded
values change across chip version (so you can better describe using DT).
But, in case you think this population of chips wont grow, then might be
ok to keep the way it is, even though looks not so nice in the code :-).