[PATCH 0/7] thermal: rcar_gen3_thermal: add support for interrupt triggerd trip points

[PATCH 0/7] thermal: rcar_gen3_thermal: add support for interrupt triggerd trip points

[Niklas Söderlund]

Hi,

This series adds support for hardware backed trip point windows. It is 
based on top of v4.11-rc1 and is tested on R-Car H3 and M3-W.

The series starts out by fixing three issues (1/7, 2/7, 3/7) that should 
not have been fixed by me before the initial driver where submitted to 
upstream. Sorry for not spotting the issues sooner.

The series then extends the rcar_gen3_thermal driver with hardware 
interrupts for trip point windows by implementing the .set_trips() 
callback of struct thermal_zone_of_device_ops (4/7, 5/7). It then adds 
suspend and resume handlers so that the hardware interrupts are 
preserved across suspend/resume cycles (6/7, 7/7).

Niklas Söderlund (7):
  thermal: rcar_gen3_thermal: add delay in .thermal_init on r8a7796
  thermal: rcar_gen3_thermal: fix probe error path
  thermal: rcar_gen3_thermal: remove unneeded mutex
  thermal: rcar_gen3_thermal: record more information about each TSC
  thermal: rcar_gen3_thermal: enable hardware interrupts for trip points
  thermal: rcar_gen3_thermal: store device match data in private
    structure
  thermal: rcar_gen3_thermal: add suspend and resume support

 drivers/thermal/rcar_gen3_thermal.c | 199 +++++++++++++++++++++++++++++++++---
 1 file changed, 187 insertions(+), 12 deletions(-)

-- 
2.12.0

[PATCH 1/7] thermal: rcar_gen3_thermal: add delay in .thermal_init on r8a7796

[Niklas Söderlund]

The .thermal_init needs to be delayed a short amount of 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.

This should have been done in the initial commit which adds the driver
as the same issue was found and corrected for r8a7795.

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 d33c845244b1d819..ec477d47d0bae8e5 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -222,6 +222,8 @@ static void r8a7796_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
 	reg_val = rcar_gen3_thermal_read(tsc, REG_GEN3_THCTR);
 	reg_val |= THCTR_THSST;
 	rcar_gen3_thermal_write(tsc, REG_GEN3_THCTR, reg_val);
+
+	usleep_range(1000, 2000);
 }
 
 static const struct rcar_gen3_thermal_data r8a7795_data = {
-- 
2.12.0

[PATCH 2/7] thermal: rcar_gen3_thermal: fix probe error path

[Niklas Söderlund]

If the memory resource for a TSC is unviable probe should fail not try
to go ahead with the remaining TSC. This fix is aligned with other
checks in probe where probe fails if they are unavailable.

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

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index ec477d47d0bae8e5..97958f91047b4c3a 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -289,8 +289,10 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 		}
 
 		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
-		if (!res)
-			break;
+		if (!res) {
+			ret = -ENODEV;
+			goto error_unregister;
+		}
 
 		tsc->base = devm_ioremap_resource(dev, res);
 		if (IS_ERR(tsc->base)) {
-- 
2.12.0

[PATCH 3/7] thermal: rcar_gen3_thermal: remove unneeded mutex

[Niklas Söderlund]

There is no point in protecting a register read with a lock. This is
most likely a leftover from when the driver was reworked before submitted
for upstream.

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

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index 97958f91047b4c3a..5d4a5483eb13e796 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -20,7 +20,6 @@
 #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>
@@ -72,7 +71,6 @@ 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 {
@@ -163,16 +161,12 @@ static int rcar_gen3_thermal_get_temp(void *devdata, int *temp)
 	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 = FIXPT_DIV(FIXPT_INT(reg) - tsc->coef.b1, tsc->coef.a1);
 	val2 = FIXPT_DIV(FIXPT_INT(reg) - tsc->coef.b2, tsc->coef.a2);
 	mcelsius = FIXPT_TO_MCELSIUS((val1 + val2) / 2);
 
-	mutex_unlock(&tsc->lock);
-
 	/* Make sure we are inside specifications */
 	if ((mcelsius < MCELSIUS(-40)) || (mcelsius > MCELSIUS(125)))
 		return -EIO;
@@ -301,7 +295,6 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 		}
 
 		priv->tscs[i] = tsc;
-		mutex_init(&tsc->lock);
 
 		match_data->thermal_init(tsc);
 		rcar_gen3_thermal_calc_coefs(&tsc->coef, ptat, thcode[i]);
-- 
2.12.0

[PATCH 4/7] thermal: rcar_gen3_thermal: record more information about each TSC

[Niklas Söderlund]

The struct rcar_gen3_thermal_tsc benefits from knowing which TSC it
represent. Record this at probe time before this information is lost.

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

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index 5d4a5483eb13e796..65f7204936a18278 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -68,6 +68,8 @@ struct equation_coefs {
 };
 
 struct rcar_gen3_thermal_tsc {
+	struct device *dev;
+	int num;
 	void __iomem *base;
 	struct thermal_zone_device *zone;
 	struct equation_coefs coef;
@@ -282,6 +284,9 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 			goto error_unregister;
 		}
 
+		tsc->dev = dev;
+		tsc->num = i;
+
 		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
 		if (!res) {
 			ret = -ENODEV;
-- 
2.12.0

[PATCH 5/7] thermal: rcar_gen3_thermal: enable hardware interrupts for trip points

[Niklas Söderlund]

Enable hardware trip points by implementing the set_trips callback. The
thermal core will take care of setting the initial trip point window and
to update it once the driver reports a TSC have moved outside it.

The interrupt structure for this device is a bit odd. There is not a
dedicated IRQ for each TSC, instead the interrupts are shared between
all TSC. IRQn is fired if the temp monitored in IRQTEMPn is reached in
any of the TSC, example IRQ3 is fired if temperature in IRQTEMP3 is
reached in either TSC0, TSC1 or TSC2.

For this reason the usage of interrupts in this driver is a all on or
all off design. When an interrupt happens all TSC are checked and all
thermal zones are updated. This could be refined to be more fine grained
but the thermal core takes care of only updating the thermal zones that
have left there trip point window.

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

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index 65f7204936a18278..e52181015e589a7f 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -23,8 +23,11 @@
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/spinlock.h>
 #include <linux/thermal.h>
 
+#include "thermal_core.h"
+
 /* Register offsets */
 #define REG_GEN3_IRQSTR		0x04
 #define REG_GEN3_IRQMSK		0x08
@@ -40,6 +43,14 @@
 #define REG_GEN3_THCODE2	0x54
 #define REG_GEN3_THCODE3	0x58
 
+/* IRQ{STR,MSK,EN} bits */
+#define IRQ_TEMP1		BIT(0)
+#define IRQ_TEMP2		BIT(1)
+#define IRQ_TEMP3		BIT(2)
+#define IRQ_TEMPD1		BIT(3)
+#define IRQ_TEMPD2		BIT(4)
+#define IRQ_TEMPD3		BIT(5)
+
 /* CTSR bits */
 #define CTSR_PONM	BIT(8)
 #define CTSR_AOUT	BIT(7)
@@ -76,6 +87,7 @@ struct rcar_gen3_thermal_tsc {
 };
 
 struct rcar_gen3_thermal_priv {
+	spinlock_t lock;
 	struct rcar_gen3_thermal_tsc *tscs[TSC_MAX_NUM];
 };
 
@@ -114,6 +126,7 @@ static inline void rcar_gen3_thermal_write(struct rcar_gen3_thermal_tsc *tsc,
 
 #define FIXPT_SHIFT 7
 #define FIXPT_INT(_x) ((_x) << FIXPT_SHIFT)
+#define INT_FIXPT(_x) ((_x) >> FIXPT_SHIFT)
 #define FIXPT_DIV(_a, _b) DIV_ROUND_CLOSEST(((_a) << FIXPT_SHIFT), (_b))
 #define FIXPT_TO_MCELSIUS(_x) ((_x) * 1000 >> FIXPT_SHIFT)
 
@@ -179,10 +192,99 @@ static int rcar_gen3_thermal_get_temp(void *devdata, int *temp)
 	return 0;
 }
 
+static int rcar_gen3_thermal_mcelsius_to_temp(struct rcar_gen3_thermal_tsc *tsc,
+					      int mcelsius)
+{
+	int celsius, val1, val2;
+
+	celsius = DIV_ROUND_CLOSEST(mcelsius, 1000);
+	val1 = celsius * tsc->coef.a1 + tsc->coef.b1;
+	val2 = celsius * tsc->coef.a2 + tsc->coef.b2;
+
+	return INT_FIXPT((val1 + val2) / 2);
+}
+
+static int rcar_gen3_thermal_set_trips(void *devdata, int low, int high)
+{
+	struct rcar_gen3_thermal_tsc *tsc = devdata;
+
+	if (low < -40000)
+		low = -40000;
+	if (high > 125000)
+		high = 125000;
+
+	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQTEMP1,
+				rcar_gen3_thermal_mcelsius_to_temp(tsc, low));
+
+	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQTEMP2,
+				rcar_gen3_thermal_mcelsius_to_temp(tsc, high));
+
+	dev_dbg(tsc->dev, "TSC%d: low: %d high: %d\n", tsc->num, low, high);
+
+	return 0;
+}
+
 static struct thermal_zone_of_device_ops rcar_gen3_tz_of_ops = {
 	.get_temp	= rcar_gen3_thermal_get_temp,
+	.set_trips	= rcar_gen3_thermal_set_trips,
 };
 
+static void rcar_thermal_irq_disable(struct rcar_gen3_thermal_priv *priv)
+{
+	int i;
+
+	for (i = 0; i < TSC_MAX_NUM; i++)
+		rcar_gen3_thermal_write(priv->tscs[i], REG_GEN3_IRQMSK, 0);
+}
+
+static void rcar_thermal_irq_enable(struct rcar_gen3_thermal_priv *priv)
+{
+	int i;
+
+	for (i = 0; i < TSC_MAX_NUM; i++)
+		rcar_gen3_thermal_write(priv->tscs[i], REG_GEN3_IRQMSK,
+					IRQ_TEMPD1 | IRQ_TEMP2);
+}
+
+static irqreturn_t rcar_gen3_thermal_irq(int irq, void *data)
+{
+	struct rcar_gen3_thermal_priv *priv = data;
+	unsigned long flags;
+	u32 status;
+	int i, ret = IRQ_HANDLED;
+
+	spin_lock_irqsave(&priv->lock, flags);
+	for (i = 0; i < TSC_MAX_NUM; i++) {
+		status = rcar_gen3_thermal_read(priv->tscs[i], REG_GEN3_IRQSTR);
+		rcar_gen3_thermal_write(priv->tscs[i], REG_GEN3_IRQSTR, 0);
+		if (status)
+			ret = IRQ_WAKE_THREAD;
+	}
+
+	if (ret == IRQ_WAKE_THREAD)
+		rcar_thermal_irq_disable(priv);
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	return ret;
+}
+
+static irqreturn_t rcar_gen3_thermal_irq_thread(int irq, void *data)
+{
+	struct rcar_gen3_thermal_priv *priv = data;
+	unsigned long flags;
+	int i;
+
+	for (i = 0; i < TSC_MAX_NUM; i++)
+		thermal_zone_device_update(priv->tscs[i]->zone,
+					   THERMAL_EVENT_UNSPECIFIED);
+
+	spin_lock_irqsave(&priv->lock, flags);
+	rcar_thermal_irq_enable(priv);
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	return IRQ_HANDLED;
+}
+
 static void r8a7795_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
 {
 	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,  CTSR_THBGR);
@@ -191,7 +293,11 @@ static void r8a7795_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
 	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_IRQMSK, 0);
+	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQEN, IRQ_TEMPD1 | IRQ_TEMP2);
+
 	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,
 				CTSR_PONM | CTSR_AOUT | CTSR_THBGR | CTSR_VMEN);
 
@@ -215,6 +321,9 @@ static void r8a7796_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
 	usleep_range(1000, 2000);
 
 	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQCTL, 0x3F);
+	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQMSK, 0);
+	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQEN, IRQ_TEMPD1 | IRQ_TEMP2);
+
 	reg_val = rcar_gen3_thermal_read(tsc, REG_GEN3_THCTR);
 	reg_val |= THCTR_THSST;
 	rcar_gen3_thermal_write(tsc, REG_GEN3_THCTR, reg_val);
@@ -270,6 +379,8 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 	if (!priv)
 		return -ENOMEM;
 
+	spin_lock_init(&priv->lock);
+
 	platform_set_drvdata(pdev, priv);
 
 	pm_runtime_enable(dev);
@@ -277,6 +388,8 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 
 	for (i = 0; i < TSC_MAX_NUM; i++) {
 		struct rcar_gen3_thermal_tsc *tsc;
+		char *irqname;
+		int irq;
 
 		tsc = devm_kzalloc(dev, sizeof(*tsc), GFP_KERNEL);
 		if (!tsc) {
@@ -299,6 +412,25 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 			goto error_unregister;
 		}
 
+		irq = platform_get_irq(pdev, i);
+		if (irq < 0) {
+			ret = irq;
+			goto error_unregister;
+		}
+
+		irqname = devm_kasprintf(dev, GFP_KERNEL, "%s:ch%d",
+					 dev_name(dev), i);
+		if (!irqname) {
+			ret = -ENOMEM;
+			goto error_unregister;
+		}
+
+		ret = devm_request_threaded_irq(dev, irq, rcar_gen3_thermal_irq,
+						rcar_gen3_thermal_irq_thread,
+						IRQF_SHARED, irqname, priv);
+		if (ret)
+			goto error_unregister;
+
 		priv->tscs[i] = tsc;
 
 		match_data->thermal_init(tsc);
@@ -312,8 +444,13 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 			goto error_unregister;
 		}
 		tsc->zone = zone;
+
+		dev_info(tsc->dev, "TSC%d: Loaded %d trip points\n", tsc->num,
+			 of_thermal_get_ntrips(tsc->zone));
 	}
 
+	rcar_thermal_irq_enable(priv);
+
 	return 0;
 
 error_unregister:
-- 
2.12.0

[PATCH 6/7] thermal: rcar_gen3_thermal: store device match data in private structure

[Niklas Söderlund]

The device match data needs to be accessible outside the probe function,
store it in the private data structure.

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

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index e52181015e589a7f..9168ac538f3b8cf5 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -87,6 +87,7 @@ struct rcar_gen3_thermal_tsc {
 };
 
 struct rcar_gen3_thermal_priv {
+	const struct rcar_gen3_thermal_data *data;
 	spinlock_t lock;
 	struct rcar_gen3_thermal_tsc *tscs[TSC_MAX_NUM];
 };
@@ -363,8 +364,6 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 	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 */
@@ -379,6 +378,8 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 	if (!priv)
 		return -ENOMEM;
 
+	priv->data = of_device_get_match_data(dev);
+
 	spin_lock_init(&priv->lock);
 
 	platform_set_drvdata(pdev, priv);
@@ -433,7 +434,7 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 
 		priv->tscs[i] = tsc;
 
-		match_data->thermal_init(tsc);
+		priv->data->thermal_init(tsc);
 		rcar_gen3_thermal_calc_coefs(&tsc->coef, ptat, thcode[i]);
 
 		zone = devm_thermal_zone_of_sensor_register(dev, i, tsc,
-- 
2.12.0

[PATCH 7/7] thermal: rcar_gen3_thermal: add suspend and resume support

[Niklas Söderlund]

To restore operation it's easiest to reinitialise all TSC. In order to
do this the current trip window needs to be stored in the TSC structure
so that it can be restored upon resume.

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

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index 9168ac538f3b8cf5..84642871a628cb2a 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -84,6 +84,8 @@ struct rcar_gen3_thermal_tsc {
 	void __iomem *base;
 	struct thermal_zone_device *zone;
 	struct equation_coefs coef;
+	int low;
+	int high;
 };
 
 struct rcar_gen3_thermal_priv {
@@ -220,6 +222,9 @@ static int rcar_gen3_thermal_set_trips(void *devdata, int low, int high)
 	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQTEMP2,
 				rcar_gen3_thermal_mcelsius_to_temp(tsc, high));
 
+	tsc->low = low;
+	tsc->high = high;
+
 	dev_dbg(tsc->dev, "TSC%d: low: %d high: %d\n", tsc->num, low, high);
 
 	return 0;
@@ -460,9 +465,39 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 	return ret;
 }
 
+static int __maybe_unused rcar_gen3_thermal_suspend(struct device *dev)
+{
+	struct rcar_gen3_thermal_priv *priv = dev_get_drvdata(dev);
+
+	rcar_thermal_irq_disable(priv);
+
+	return 0;
+}
+
+static int __maybe_unused rcar_gen3_thermal_resume(struct device *dev)
+{
+	struct rcar_gen3_thermal_priv *priv = dev_get_drvdata(dev);
+	int i;
+
+	for (i = 0; i < TSC_MAX_NUM; i++) {
+		struct rcar_gen3_thermal_tsc *tsc = priv->tscs[i];
+
+		priv->data->thermal_init(tsc);
+		rcar_gen3_thermal_set_trips(tsc, tsc->low, tsc->high);
+	}
+
+	rcar_thermal_irq_enable(priv);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(rcar_gen3_thermal_pm_ops, rcar_gen3_thermal_suspend,
+			 rcar_gen3_thermal_resume);
+
 static struct platform_driver rcar_gen3_thermal_driver = {
 	.driver	= {
 		.name	= "rcar_gen3_thermal",
+		.pm = &rcar_gen3_thermal_pm_ops,
 		.of_match_table = rcar_gen3_thermal_dt_ids,
 	},
 	.probe		= rcar_gen3_thermal_probe,
-- 
2.12.0

Re: [PATCH 1/7] thermal: rcar_gen3_thermal: add delay in .thermal_init on r8a7796

[Sergei Shtylyov]

On 3/6/2017 11:03 PM, Niklas Söderlund wrote:

> The .thermal_init needs to be delayed a short amount of to allow for the

    Amount of what? :-)

> TEMP register to contain something useful. If it's not delayed theses

    These.

> 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.
>
> This should have been done in the initial commit which adds the driver
> as the same issue was found and corrected for r8a7795.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
[...]

MBR, Sergei

Re: [PATCH 1/7] thermal: rcar_gen3_thermal: add delay in .thermal_init on r8a7796

On 2017-03-06 21:03:55 +0100, Niklas S�derlund wrote:
> The .thermal_init needs to be delayed a short amount of 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.
> 
> This should have been done in the initial commit which adds the driver
> as the same issue was found and corrected for r8a7795.
> 
> Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>

I forgot this tag for this patch, will update if there is a need for v2.

Fixes: 564e73d283af9d4c ("thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver")

> ---
>  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 d33c845244b1d819..ec477d47d0bae8e5 100644
> --- a/drivers/thermal/rcar_gen3_thermal.c
> +++ b/drivers/thermal/rcar_gen3_thermal.c
> @@ -222,6 +222,8 @@ static void r8a7796_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
>  	reg_val = rcar_gen3_thermal_read(tsc, REG_GEN3_THCTR);
>  	reg_val |= THCTR_THSST;
>  	rcar_gen3_thermal_write(tsc, REG_GEN3_THCTR, reg_val);
> +
> +	usleep_range(1000, 2000);
>  }
>  
>  static const struct rcar_gen3_thermal_data r8a7795_data = {
> -- 
> 2.12.0
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH 1/7] thermal: rcar_gen3_thermal: add delay in .thermal_init on r8a7796

[Niklas Söderlund]

On 2017-03-06 21:03:55 +0100, Niklas Söderlund wrote:
> The .thermal_init needs to be delayed a short amount of 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.
> 
> This should have been done in the initial commit which adds the driver
> as the same issue was found and corrected for r8a7795.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

I forgot this tag for this patch, will update if there is a need for v2.

Fixes: 564e73d283af9d4c ("thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver")

> ---
>  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 d33c845244b1d819..ec477d47d0bae8e5 100644
> --- a/drivers/thermal/rcar_gen3_thermal.c
> +++ b/drivers/thermal/rcar_gen3_thermal.c
> @@ -222,6 +222,8 @@ static void r8a7796_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
>  	reg_val = rcar_gen3_thermal_read(tsc, REG_GEN3_THCTR);
>  	reg_val |= THCTR_THSST;
>  	rcar_gen3_thermal_write(tsc, REG_GEN3_THCTR, reg_val);
> +
> +	usleep_range(1000, 2000);
>  }
>  
>  static const struct rcar_gen3_thermal_data r8a7795_data = {
> -- 
> 2.12.0
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH 1/7] thermal: rcar_gen3_thermal: add delay in .thermal_init on r8a7796

[Niklas Söderlund]

HI Sergei,

Thanks will update for v2.

On 2017-03-07 12:49:25 +0300, Sergei Shtylyov wrote:
> On 3/6/2017 11:03 PM, Niklas S�derlund wrote:
> 
> > The .thermal_init needs to be delayed a short amount of to allow for the
> 
>    Amount of what? :-)
> 
> > TEMP register to contain something useful. If it's not delayed theses
> 
>    These.
> 
> > 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.
> > 
> > This should have been done in the initial commit which adds the driver
> > as the same issue was found and corrected for r8a7795.
> > 
> > Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> [...]
> 
> MBR, Sergei
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH 1/7] thermal: rcar_gen3_thermal: add delay in .thermal_init on r8a7796

[Niklas Söderlund]

HI Sergei,

Thanks will update for v2.

On 2017-03-07 12:49:25 +0300, Sergei Shtylyov wrote:
> On 3/6/2017 11:03 PM, Niklas Söderlund wrote:
> 
> > The .thermal_init needs to be delayed a short amount of to allow for the
> 
>    Amount of what? :-)
> 
> > TEMP register to contain something useful. If it's not delayed theses
> 
>    These.
> 
> > 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.
> > 
> > This should have been done in the initial commit which adds the driver
> > as the same issue was found and corrected for r8a7795.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> [...]
> 
> MBR, Sergei
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH 2/7] thermal: rcar_gen3_thermal: fix probe error path

[Geert Uytterhoeven]

On Mon, Mar 6, 2017 at 9:03 PM, Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> If the memory resource for a TSC is unviable probe should fail not try

unavailable

> to go ahead with the remaining TSC. This fix is aligned with other

TSCs

> checks in probe where probe fails if they are unavailable.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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: [PATCH 3/7] thermal: rcar_gen3_thermal: remove unneeded mutex

[Geert Uytterhoeven]

On Mon, Mar 6, 2017 at 9:03 PM, Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> There is no point in protecting a register read with a lock. This is
> most likely a leftover from when the driver was reworked before submitted
> for upstream.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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: [PATCH 4/7] thermal: rcar_gen3_thermal: record more information about each TSC

[Geert Uytterhoeven]

On Mon, Mar 6, 2017 at 9:03 PM, Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> The struct rcar_gen3_thermal_tsc benefits from knowing which TSC it
> represent. Record this at probe time before this information is lost.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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: [PATCH 5/7] thermal: rcar_gen3_thermal: enable hardware interrupts for trip points

[Geert Uytterhoeven]

On Mon, Mar 6, 2017 at 9:03 PM, Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> Enable hardware trip points by implementing the set_trips callback. The
> thermal core will take care of setting the initial trip point window and
> to update it once the driver reports a TSC have moved outside it.

has moved

> The interrupt structure for this device is a bit odd. There is not a
> dedicated IRQ for each TSC, instead the interrupts are shared between
> all TSC. IRQn is fired if the temp monitored in IRQTEMPn is reached in

all TSCs

> any of the TSC, example IRQ3 is fired if temperature in IRQTEMP3 is

TSCs

> reached in either TSC0, TSC1 or TSC2.
>
> For this reason the usage of interrupts in this driver is a all on or

an all-on or all-off

> all off design. When an interrupt happens all TSC are checked and all

TSCs

> thermal zones are updated. This could be refined to be more fine grained
> but the thermal core takes care of only updating the thermal zones that
> have left there trip point window.

their

> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> ---
>  drivers/thermal/rcar_gen3_thermal.c | 137 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 137 insertions(+)
>
> diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> index 65f7204936a18278..e52181015e589a7f 100644
> --- a/drivers/thermal/rcar_gen3_thermal.c
> +++ b/drivers/thermal/rcar_gen3_thermal.c

> @@ -76,6 +87,7 @@ struct rcar_gen3_thermal_tsc {
>  };
>
>  struct rcar_gen3_thermal_priv {
> +       spinlock_t lock;

When declaring a spinlock, please add a comment what exactly is
protected by the lock.

> +static void rcar_thermal_irq_disable(struct rcar_gen3_thermal_priv *priv)
> +{
> +       int i;

unsigned int (for all positive loop counters)

> +
> +       for (i = 0; i < TSC_MAX_NUM; i++)
> +               rcar_gen3_thermal_write(priv->tscs[i], REG_GEN3_IRQMSK, 0);
> +}

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: [PATCH 6/7] thermal: rcar_gen3_thermal: store device match data in private structure

[Geert Uytterhoeven]

On Mon, Mar 6, 2017 at 9:04 PM, Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> The device match data needs to be accessible outside the probe function,
> store it in the private data structure.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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: [PATCH 7/7] thermal: rcar_gen3_thermal: add suspend and resume support

[Geert Uytterhoeven]

On Mon, Mar 6, 2017 at 9:04 PM, Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> To restore operation it's easiest to reinitialise all TSC. In order to

TSCs

> do this the current trip window needs to be stored in the TSC structure
> so that it can be restored upon resume.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> ---
>  drivers/thermal/rcar_gen3_thermal.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>
> diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> index 9168ac538f3b8cf5..84642871a628cb2a 100644
> --- a/drivers/thermal/rcar_gen3_thermal.c
> +++ b/drivers/thermal/rcar_gen3_thermal.c

> +static int __maybe_unused rcar_gen3_thermal_resume(struct device *dev)
> +{
> +       struct rcar_gen3_thermal_priv *priv = dev_get_drvdata(dev);
> +       int i;

unsigned int

> +
> +       for (i = 0; i < TSC_MAX_NUM; i++) {
> +               struct rcar_gen3_thermal_tsc *tsc = priv->tscs[i];
> +
> +               priv->data->thermal_init(tsc);
> +               rcar_gen3_thermal_set_trips(tsc, tsc->low, tsc->high);
> +       }
> +
> +       rcar_thermal_irq_enable(priv);
> +
> +       return 0;
> +}

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: [PATCH 1/7] thermal: rcar_gen3_thermal: add delay in .thermal_init on r8a7796

[Geert Uytterhoeven]

On Mon, Mar 6, 2017 at 9:03 PM, Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> The .thermal_init needs to be delayed a short amount of to allow for the

of ... (thx Sergei ;-)

> TEMP register to contain something useful. If it's not delayed theses

these

> 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

.get_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.
>
> This should have been done in the initial commit which adds the driver
> as the same issue was found and corrected for r8a7795.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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: [PATCH 4/7] thermal: rcar_gen3_thermal: record more information about each TSC

[Geert Uytterhoeven]

Hi Niklas,

On Mon, Mar 6, 2017 at 9:03 PM, Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> The struct rcar_gen3_thermal_tsc benefits from knowing which TSC it
> represent. Record this at probe time before this information is lost.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/thermal/rcar_gen3_thermal.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> index 5d4a5483eb13e796..65f7204936a18278 100644
> --- a/drivers/thermal/rcar_gen3_thermal.c
> +++ b/drivers/thermal/rcar_gen3_thermal.c
> @@ -68,6 +68,8 @@ struct equation_coefs {
>  };
>
>  struct rcar_gen3_thermal_tsc {
> +       struct device *dev;
> +       int num;

BTW, adding the "int" here will introduce a 4-byte gap on 64-bit platforms.
Note that it doesn't hurt much in this case, as the struct will be padded to a
multiple of 8 bytes anyway.

>         void __iomem *base;
>         struct thermal_zone_device *zone;
>         struct equation_coefs coef;

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: [PATCH 1/7] thermal: rcar_gen3_thermal: add delay in .thermal_init on r8a7796

[Wolfram Sang]

On Mon, Mar 06, 2017 at 09:03:55PM +0100, Niklas Söderlund wrote:
> The .thermal_init needs to be delayed a short amount of 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.
> 
> This should have been done in the initial commit which adds the driver
> as the same issue was found and corrected for r8a7795.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

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

Re: [PATCH 2/7] thermal: rcar_gen3_thermal: fix probe error path

[Wolfram Sang]

On Mon, Mar 06, 2017 at 09:03:56PM +0100, Niklas Söderlund wrote:
> If the memory resource for a TSC is unviable probe should fail not try
> to go ahead with the remaining TSC. This fix is aligned with other
> checks in probe where probe fails if they are unavailable.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

I disagree. There are likely SoCs in the future which have less than
TSC_MAX_NUM sensors (V3M shall have only 1 according to chapter 10C in
v0.52 documentation). So, the code exits the loop for this case. We
should move it before the devm_kzalloc(), however.

That also means that you can't really iterate over TSC_MAX_NUM in later
patches, but rather store the amount of instances in the main struct and
iterate over that value.

Re: [PATCH 3/7] thermal: rcar_gen3_thermal: remove unneeded mutex

[Wolfram Sang]

On Mon, Mar 06, 2017 at 09:03:57PM +0100, Niklas Söderlund wrote:
> There is no point in protecting a register read with a lock. This is
> most likely a leftover from when the driver was reworked before submitted
> for upstream.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Yes, a left over.

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

Re: [PATCH 4/7] thermal: rcar_gen3_thermal: record more information about each TSC

[Wolfram Sang]

On Mon, Mar 06, 2017 at 09:03:58PM +0100, Niklas Söderlund wrote:
> The struct rcar_gen3_thermal_tsc benefits from knowing which TSC it
> represent. Record this at probe time before this information is lost.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

See next review. I don't think this is needed.

Re: [PATCH 5/7] thermal: rcar_gen3_thermal: enable hardware interrupts for trip points

[Wolfram Sang]

Hi Niklas,

thanks for your work!

On Mon, Mar 06, 2017 at 09:03:59PM +0100, Niklas Söderlund wrote:
> Enable hardware trip points by implementing the set_trips callback. The
> thermal core will take care of setting the initial trip point window and
> to update it once the driver reports a TSC have moved outside it.
> 
> The interrupt structure for this device is a bit odd. There is not a
> dedicated IRQ for each TSC, instead the interrupts are shared between
> all TSC. IRQn is fired if the temp monitored in IRQTEMPn is reached in
> any of the TSC, example IRQ3 is fired if temperature in IRQTEMP3 is
> reached in either TSC0, TSC1 or TSC2.
> 
> For this reason the usage of interrupts in this driver is a all on or

s/a all/an all/

> all off design. When an interrupt happens all TSC are checked and all
> thermal zones are updated. This could be refined to be more fine grained
> but the thermal core takes care of only updating the thermal zones that
> have left there trip point window.

s/there/the/

> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/thermal/rcar_gen3_thermal.c | 137 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 137 insertions(+)
> 
> +static int rcar_gen3_thermal_set_trips(void *devdata, int low, int high)
> +{
> +	struct rcar_gen3_thermal_tsc *tsc = devdata;
> +
> +	if (low < -40000)
> +		low = -40000;
> +	if (high > 125000)
> +		high = 125000;

Use clamp_val()?

> +
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQTEMP1,
> +				rcar_gen3_thermal_mcelsius_to_temp(tsc, low));
> +
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQTEMP2,
> +				rcar_gen3_thermal_mcelsius_to_temp(tsc, high));
> +
> +	dev_dbg(tsc->dev, "TSC%d: low: %d high: %d\n", tsc->num, low, high);

Is there no sysfs/debugfs way to get this from thermal core? I'd bet
this is of generic interest. Bonus point: if we drop this dbg, we can
also drop the 'dev' and 'num' members from the struct again.

> +
> +	return 0;
> +}
> +
>  static struct thermal_zone_of_device_ops rcar_gen3_tz_of_ops = {
>  	.get_temp	= rcar_gen3_thermal_get_temp,
> +	.set_trips	= rcar_gen3_thermal_set_trips,
>  };
>  
> +static void rcar_thermal_irq_disable(struct rcar_gen3_thermal_priv *priv)
> +{
> +	int i;
> +
> +	for (i = 0; i < TSC_MAX_NUM; i++)
> +		rcar_gen3_thermal_write(priv->tscs[i], REG_GEN3_IRQMSK, 0);
> +}
> +
> +static void rcar_thermal_irq_enable(struct rcar_gen3_thermal_priv *priv)
> +{
> +	int i;
> +
> +	for (i = 0; i < TSC_MAX_NUM; i++)
> +		rcar_gen3_thermal_write(priv->tscs[i], REG_GEN3_IRQMSK,
> +					IRQ_TEMPD1 | IRQ_TEMP2);
> +}

Merge the two into one with a second parameter which is bool?
rcar_gen3_irq_enable(priv, true/false)?

>  	pm_runtime_enable(dev);
> @@ -277,6 +388,8 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
>  
>  	for (i = 0; i < TSC_MAX_NUM; i++) {
>  		struct rcar_gen3_thermal_tsc *tsc;
> +		char *irqname;
> +		int irq;
>  
>  		tsc = devm_kzalloc(dev, sizeof(*tsc), GFP_KERNEL);
>  		if (!tsc) {
> @@ -299,6 +412,25 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
>  			goto error_unregister;
>  		}
>  
> +		irq = platform_get_irq(pdev, i);

Hmmm, with reusing 'i' for getting the interrupt resource, you assume
that the number of TSC will always equal the number of interrupts. It's
somewhat reasonable, yet we have seen enough surprises in HW to better
not assume it IMO.

Another advantage of decoupling it into a seperate loop is that you only
need to register as much interrupts as the driver actually uses. It uses
2 interrupts, so no need for installing 3 handlers, no?

> +		if (irq < 0) {
> +			ret = irq;
> +			goto error_unregister;
> +		}

...

> +
> +		dev_info(tsc->dev, "TSC%d: Loaded %d trip points\n", tsc->num,
> +			 of_thermal_get_ntrips(tsc->zone));

I'd think the of_thermal_get_ntrips is a little bit hidden in the
dev_info. Would like it more explicitly. Also, it cannot fail? What
happens if DT provides broken trip points...

>  	}
>  
> +	rcar_thermal_irq_enable(priv);

... because we enable interrupts unconditionally here?

Regards,

   Wolfram

Re: [PATCH 6/7] thermal: rcar_gen3_thermal: store device match data in private structure

[Wolfram Sang]

On Mon, Mar 06, 2017 at 09:04:00PM +0100, Niklas Söderlund wrote:
> The device match data needs to be accessible outside the probe function,
> store it in the private data structure.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

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

Re: [PATCH 7/7] thermal: rcar_gen3_thermal: add suspend and resume support

[Wolfram Sang]

On Mon, Mar 06, 2017 at 09:04:01PM +0100, Niklas Söderlund wrote:
> To restore operation it's easiest to reinitialise all TSC. In order to
> do this the current trip window needs to be stored in the TSC structure
> so that it can be restored upon resume.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

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

Re: [PATCH 5/7] thermal: rcar_gen3_thermal: enable hardware interrupts for trip points

[Niklas Söderlund]

Hi Wolfram,

Thanks for your feedback.

On 2017-03-07 20:55:33 +0100, Wolfram Sang wrote:
> Hi Niklas,
> 
> thanks for your work!
> 
> On Mon, Mar 06, 2017 at 09:03:59PM +0100, Niklas S�derlund wrote:
> > Enable hardware trip points by implementing the set_trips callback. The
> > thermal core will take care of setting the initial trip point window and
> > to update it once the driver reports a TSC have moved outside it.
> > 
> > The interrupt structure for this device is a bit odd. There is not a
> > dedicated IRQ for each TSC, instead the interrupts are shared between
> > all TSC. IRQn is fired if the temp monitored in IRQTEMPn is reached in
> > any of the TSC, example IRQ3 is fired if temperature in IRQTEMP3 is
> > reached in either TSC0, TSC1 or TSC2.
> > 
> > For this reason the usage of interrupts in this driver is a all on or
> 
> s/a all/an all/

Will fix

> 
> > all off design. When an interrupt happens all TSC are checked and all
> > thermal zones are updated. This could be refined to be more fine grained
> > but the thermal core takes care of only updating the thermal zones that
> > have left there trip point window.
> 
> s/there/the/

Will fix

> 
> > 
> > Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/thermal/rcar_gen3_thermal.c | 137 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 137 insertions(+)
> > 
> > +static int rcar_gen3_thermal_set_trips(void *devdata, int low, int high)
> > +{
> > +	struct rcar_gen3_thermal_tsc *tsc = devdata;
> > +
> > +	if (low < -40000)
> > +		low = -40000;
> > +	if (high > 125000)
> > +		high = 125000;
> 
> Use clamp_val()?

Good idea, will use.

> 
> > +
> > +	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQTEMP1,
> > +				rcar_gen3_thermal_mcelsius_to_temp(tsc, low));
> > +
> > +	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQTEMP2,
> > +				rcar_gen3_thermal_mcelsius_to_temp(tsc, high));
> > +
> > +	dev_dbg(tsc->dev, "TSC%d: low: %d high: %d\n", tsc->num, low, high);
> 
> Is there no sysfs/debugfs way to get this from thermal core? I'd bet
> this is of generic interest. Bonus point: if we drop this dbg, we can
> also drop the 'dev' and 'num' members from the struct again.

I can't find any sysfs or debugfs knobs to show this information, what 
do exist is a dev_dbg in thermal_zone_set_trips() which can print 
similar information. The difference is that the trip values have not 
been clamped to the device specification in that printout since it 
happens before the call to the rcar_gen3_thermal_set_trips() call (where 
they are still clamped before anything is written to hardware):


thermal_zone_set_trips():
[    2.014173] thermal thermal_zone0: new temperature boundaries: -2147483647 < x < 120000

rcar_gen3_thermal_set_trips():
[    2.014180] rcar_gen3_thermal e6198000.thermal: TSC0: low: -40000 high: 120000

But this is for debugging so I see no issue whit dropping this debug 
printout from the driver and depend on the one from thermal core. Will 
remove this and the patch which stores tsc->dev and tsc->num form the 
next version.

> 
> > +
> > +	return 0;
> > +}
> > +
> >  static struct thermal_zone_of_device_ops rcar_gen3_tz_of_ops = {
> >  	.get_temp	= rcar_gen3_thermal_get_temp,
> > +	.set_trips	= rcar_gen3_thermal_set_trips,
> >  };
> >  
> > +static void rcar_thermal_irq_disable(struct rcar_gen3_thermal_priv *priv)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < TSC_MAX_NUM; i++)
> > +		rcar_gen3_thermal_write(priv->tscs[i], REG_GEN3_IRQMSK, 0);
> > +}
> > +
> > +static void rcar_thermal_irq_enable(struct rcar_gen3_thermal_priv *priv)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < TSC_MAX_NUM; i++)
> > +		rcar_gen3_thermal_write(priv->tscs[i], REG_GEN3_IRQMSK,
> > +					IRQ_TEMPD1 | IRQ_TEMP2);
> > +}
> 
> Merge the two into one with a second parameter which is bool?
> rcar_gen3_irq_enable(priv, true/false)?

OK

> 
> >  	pm_runtime_enable(dev);
> > @@ -277,6 +388,8 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
> >  
> >  	for (i = 0; i < TSC_MAX_NUM; i++) {
> >  		struct rcar_gen3_thermal_tsc *tsc;
> > +		char *irqname;
> > +		int irq;
> >  
> >  		tsc = devm_kzalloc(dev, sizeof(*tsc), GFP_KERNEL);
> >  		if (!tsc) {
> > @@ -299,6 +412,25 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
> >  			goto error_unregister;
> >  		}
> >  
> > +		irq = platform_get_irq(pdev, i);
> 
> Hmmm, with reusing 'i' for getting the interrupt resource, you assume
> that the number of TSC will always equal the number of interrupts. It's
> somewhat reasonable, yet we have seen enough surprises in HW to better
> not assume it IMO.
> 
> Another advantage of decoupling it into a seperate loop is that you only
> need to register as much interrupts as the driver actually uses. It uses
> 2 interrupts, so no need for installing 3 handlers, no?

That is correct, will update for next version.

> 
> > +		if (irq < 0) {
> > +			ret = irq;
> > +			goto error_unregister;
> > +		}
> 
> ...
> 
> > +
> > +		dev_info(tsc->dev, "TSC%d: Loaded %d trip points\n", tsc->num,
> > +			 of_thermal_get_ntrips(tsc->zone));
> 
> I'd think the of_thermal_get_ntrips is a little bit hidden in the
> dev_info. Would like it more explicitly. Also, it cannot fail? What
> happens if DT provides broken trip points...

All thermal zones are parsed by the thermal core by 
of_parse_thermal_zones() called from thermal_init() which is called 
using fs_initcall(thermal_init). Then the call chain

devm_thermal_zone_of_sensor_register()
    thermal_zone_of_sensor_register()
       thermal_zone_of_add_sensor()
          thermal_zone_get_zone_by_name()

Finds the zone parsed at init time, so if the zone is broken in some way 
it would not be available and devm_thermal_zone_of_sensor_register() 
would fail.

But you are correct of_thermal_get_ntrips() could return -ENODEV I will 
add check for this and error out if that is the case.

> 
> >  	}
> >  
> > +	rcar_thermal_irq_enable(priv);
> 
> ... because we enable interrupts unconditionally here?
> 
> Regards,
> 
>    Wolfram
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH 5/7] thermal: rcar_gen3_thermal: enable hardware interrupts for trip points

[Niklas Söderlund]

Hi Wolfram,

Thanks for your feedback.

On 2017-03-07 20:55:33 +0100, Wolfram Sang wrote:
> Hi Niklas,
> 
> thanks for your work!
> 
> On Mon, Mar 06, 2017 at 09:03:59PM +0100, Niklas Söderlund wrote:
> > Enable hardware trip points by implementing the set_trips callback. The
> > thermal core will take care of setting the initial trip point window and
> > to update it once the driver reports a TSC have moved outside it.
> > 
> > The interrupt structure for this device is a bit odd. There is not a
> > dedicated IRQ for each TSC, instead the interrupts are shared between
> > all TSC. IRQn is fired if the temp monitored in IRQTEMPn is reached in
> > any of the TSC, example IRQ3 is fired if temperature in IRQTEMP3 is
> > reached in either TSC0, TSC1 or TSC2.
> > 
> > For this reason the usage of interrupts in this driver is a all on or
> 
> s/a all/an all/

Will fix

> 
> > all off design. When an interrupt happens all TSC are checked and all
> > thermal zones are updated. This could be refined to be more fine grained
> > but the thermal core takes care of only updating the thermal zones that
> > have left there trip point window.
> 
> s/there/the/

Will fix

> 
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/thermal/rcar_gen3_thermal.c | 137 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 137 insertions(+)
> > 
> > +static int rcar_gen3_thermal_set_trips(void *devdata, int low, int high)
> > +{
> > +	struct rcar_gen3_thermal_tsc *tsc = devdata;
> > +
> > +	if (low < -40000)
> > +		low = -40000;
> > +	if (high > 125000)
> > +		high = 125000;
> 
> Use clamp_val()?

Good idea, will use.

> 
> > +
> > +	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQTEMP1,
> > +				rcar_gen3_thermal_mcelsius_to_temp(tsc, low));
> > +
> > +	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQTEMP2,
> > +				rcar_gen3_thermal_mcelsius_to_temp(tsc, high));
> > +
> > +	dev_dbg(tsc->dev, "TSC%d: low: %d high: %d\n", tsc->num, low, high);
> 
> Is there no sysfs/debugfs way to get this from thermal core? I'd bet
> this is of generic interest. Bonus point: if we drop this dbg, we can
> also drop the 'dev' and 'num' members from the struct again.

I can't find any sysfs or debugfs knobs to show this information, what 
do exist is a dev_dbg in thermal_zone_set_trips() which can print 
similar information. The difference is that the trip values have not 
been clamped to the device specification in that printout since it 
happens before the call to the rcar_gen3_thermal_set_trips() call (where 
they are still clamped before anything is written to hardware):


thermal_zone_set_trips():
[    2.014173] thermal thermal_zone0: new temperature boundaries: -2147483647 < x < 120000

rcar_gen3_thermal_set_trips():
[    2.014180] rcar_gen3_thermal e6198000.thermal: TSC0: low: -40000 high: 120000

But this is for debugging so I see no issue whit dropping this debug 
printout from the driver and depend on the one from thermal core. Will 
remove this and the patch which stores tsc->dev and tsc->num form the 
next version.

> 
> > +
> > +	return 0;
> > +}
> > +
> >  static struct thermal_zone_of_device_ops rcar_gen3_tz_of_ops = {
> >  	.get_temp	= rcar_gen3_thermal_get_temp,
> > +	.set_trips	= rcar_gen3_thermal_set_trips,
> >  };
> >  
> > +static void rcar_thermal_irq_disable(struct rcar_gen3_thermal_priv *priv)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < TSC_MAX_NUM; i++)
> > +		rcar_gen3_thermal_write(priv->tscs[i], REG_GEN3_IRQMSK, 0);
> > +}
> > +
> > +static void rcar_thermal_irq_enable(struct rcar_gen3_thermal_priv *priv)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < TSC_MAX_NUM; i++)
> > +		rcar_gen3_thermal_write(priv->tscs[i], REG_GEN3_IRQMSK,
> > +					IRQ_TEMPD1 | IRQ_TEMP2);
> > +}
> 
> Merge the two into one with a second parameter which is bool?
> rcar_gen3_irq_enable(priv, true/false)?

OK

> 
> >  	pm_runtime_enable(dev);
> > @@ -277,6 +388,8 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
> >  
> >  	for (i = 0; i < TSC_MAX_NUM; i++) {
> >  		struct rcar_gen3_thermal_tsc *tsc;
> > +		char *irqname;
> > +		int irq;
> >  
> >  		tsc = devm_kzalloc(dev, sizeof(*tsc), GFP_KERNEL);
> >  		if (!tsc) {
> > @@ -299,6 +412,25 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
> >  			goto error_unregister;
> >  		}
> >  
> > +		irq = platform_get_irq(pdev, i);
> 
> Hmmm, with reusing 'i' for getting the interrupt resource, you assume
> that the number of TSC will always equal the number of interrupts. It's
> somewhat reasonable, yet we have seen enough surprises in HW to better
> not assume it IMO.
> 
> Another advantage of decoupling it into a seperate loop is that you only
> need to register as much interrupts as the driver actually uses. It uses
> 2 interrupts, so no need for installing 3 handlers, no?

That is correct, will update for next version.

> 
> > +		if (irq < 0) {
> > +			ret = irq;
> > +			goto error_unregister;
> > +		}
> 
> ...
> 
> > +
> > +		dev_info(tsc->dev, "TSC%d: Loaded %d trip points\n", tsc->num,
> > +			 of_thermal_get_ntrips(tsc->zone));
> 
> I'd think the of_thermal_get_ntrips is a little bit hidden in the
> dev_info. Would like it more explicitly. Also, it cannot fail? What
> happens if DT provides broken trip points...

All thermal zones are parsed by the thermal core by 
of_parse_thermal_zones() called from thermal_init() which is called 
using fs_initcall(thermal_init). Then the call chain

devm_thermal_zone_of_sensor_register()
    thermal_zone_of_sensor_register()
       thermal_zone_of_add_sensor()
          thermal_zone_get_zone_by_name()

Finds the zone parsed at init time, so if the zone is broken in some way 
it would not be available and devm_thermal_zone_of_sensor_register() 
would fail.

But you are correct of_thermal_get_ntrips() could return -ENODEV I will 
add check for this and error out if that is the case.

> 
> >  	}
> >  
> > +	rcar_thermal_irq_enable(priv);
> 
> ... because we enable interrupts unconditionally here?
> 
> Regards,
> 
>    Wolfram
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH 5/7] thermal: rcar_gen3_thermal: enable hardware interrupts for trip points

[Niklas Söderlund]

Hi Geert,

Thanks for your feedback.

On 2017-03-07 16:36:00 +0100, Geert Uytterhoeven wrote:
> On Mon, Mar 6, 2017 at 9:03 PM, Niklas S�derlund
> <niklas.soderlund+renesas@ragnatech.se> wrote:
> > Enable hardware trip points by implementing the set_trips callback. The
> > thermal core will take care of setting the initial trip point window and
> > to update it once the driver reports a TSC have moved outside it.
> 
> has moved
> 
> > The interrupt structure for this device is a bit odd. There is not a
> > dedicated IRQ for each TSC, instead the interrupts are shared between
> > all TSC. IRQn is fired if the temp monitored in IRQTEMPn is reached in
> 
> all TSCs
> 
> > any of the TSC, example IRQ3 is fired if temperature in IRQTEMP3 is
> 
> TSCs
> 
> > reached in either TSC0, TSC1 or TSC2.
> >
> > For this reason the usage of interrupts in this driver is a all on or
> 
> an all-on or all-off
> 
> > all off design. When an interrupt happens all TSC are checked and all
> 
> TSCs
> 
> > thermal zones are updated. This could be refined to be more fine grained
> > but the thermal core takes care of only updating the thermal zones that
> > have left there trip point window.
> 
> their
> 
> > Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

I will fix all the spelling suggestions above, thanks! But I will not 
add your Reviewed-by since Wolframs comments suggest I need to update 
some parts of this patch.

> 
> > ---
> >  drivers/thermal/rcar_gen3_thermal.c | 137 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 137 insertions(+)
> >
> > diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> > index 65f7204936a18278..e52181015e589a7f 100644
> > --- a/drivers/thermal/rcar_gen3_thermal.c
> > +++ b/drivers/thermal/rcar_gen3_thermal.c
> 
> > @@ -76,6 +87,7 @@ struct rcar_gen3_thermal_tsc {
> >  };
> >
> >  struct rcar_gen3_thermal_priv {
> > +       spinlock_t lock;
> 
> When declaring a spinlock, please add a comment what exactly is
> protected by the lock.

Will do thanks.

> 
> > +static void rcar_thermal_irq_disable(struct rcar_gen3_thermal_priv *priv)
> > +{
> > +       int i;
> 
> unsigned int (for all positive loop counters)

Thanks, will fix.

> 
> > +
> > +       for (i = 0; i < TSC_MAX_NUM; i++)
> > +               rcar_gen3_thermal_write(priv->tscs[i], REG_GEN3_IRQMSK, 0);
> > +}
> 
> 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: [PATCH 5/7] thermal: rcar_gen3_thermal: enable hardware interrupts for trip points

[Niklas Söderlund]

Hi Geert,

Thanks for your feedback.

On 2017-03-07 16:36:00 +0100, Geert Uytterhoeven wrote:
> On Mon, Mar 6, 2017 at 9:03 PM, Niklas Söderlund
> <niklas.soderlund+renesas@ragnatech.se> wrote:
> > Enable hardware trip points by implementing the set_trips callback. The
> > thermal core will take care of setting the initial trip point window and
> > to update it once the driver reports a TSC have moved outside it.
> 
> has moved
> 
> > The interrupt structure for this device is a bit odd. There is not a
> > dedicated IRQ for each TSC, instead the interrupts are shared between
> > all TSC. IRQn is fired if the temp monitored in IRQTEMPn is reached in
> 
> all TSCs
> 
> > any of the TSC, example IRQ3 is fired if temperature in IRQTEMP3 is
> 
> TSCs
> 
> > reached in either TSC0, TSC1 or TSC2.
> >
> > For this reason the usage of interrupts in this driver is a all on or
> 
> an all-on or all-off
> 
> > all off design. When an interrupt happens all TSC are checked and all
> 
> TSCs
> 
> > thermal zones are updated. This could be refined to be more fine grained
> > but the thermal core takes care of only updating the thermal zones that
> > have left there trip point window.
> 
> their
> 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

I will fix all the spelling suggestions above, thanks! But I will not 
add your Reviewed-by since Wolframs comments suggest I need to update 
some parts of this patch.

> 
> > ---
> >  drivers/thermal/rcar_gen3_thermal.c | 137 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 137 insertions(+)
> >
> > diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> > index 65f7204936a18278..e52181015e589a7f 100644
> > --- a/drivers/thermal/rcar_gen3_thermal.c
> > +++ b/drivers/thermal/rcar_gen3_thermal.c
> 
> > @@ -76,6 +87,7 @@ struct rcar_gen3_thermal_tsc {
> >  };
> >
> >  struct rcar_gen3_thermal_priv {
> > +       spinlock_t lock;
> 
> When declaring a spinlock, please add a comment what exactly is
> protected by the lock.

Will do thanks.

> 
> > +static void rcar_thermal_irq_disable(struct rcar_gen3_thermal_priv *priv)
> > +{
> > +       int i;
> 
> unsigned int (for all positive loop counters)

Thanks, will fix.

> 
> > +
> > +       for (i = 0; i < TSC_MAX_NUM; i++)
> > +               rcar_gen3_thermal_write(priv->tscs[i], REG_GEN3_IRQMSK, 0);
> > +}
> 
> 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: [PATCH 2/7] thermal: rcar_gen3_thermal: fix probe error path

[Niklas Söderlund]

Hi Wolfram,

Thanks for your feedback.

On 2017-03-07 20:52:03 +0100, Wolfram Sang wrote:
> On Mon, Mar 06, 2017 at 09:03:56PM +0100, Niklas S�derlund wrote:
> > If the memory resource for a TSC is unviable probe should fail not try
> > to go ahead with the remaining TSC. This fix is aligned with other
> > checks in probe where probe fails if they are unavailable.
> > 
> > Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> 
> I disagree. There are likely SoCs in the future which have less than
> TSC_MAX_NUM sensors (V3M shall have only 1 according to chapter 10C in
> v0.52 documentation). So, the code exits the loop for this case. We
> should move it before the devm_kzalloc(), however.

Ahh I did not think of that. I will drop this patch and instead do as 
you suggest move this before the devm_kzalloc().

> 
> That also means that you can't really iterate over TSC_MAX_NUM in later
> patches, but rather store the amount of instances in the main struct and
> iterate over that value.
> 

I will see how to best solve this, probably we need then to recoded in 
struct rcar_gen3_thermal_priv how many zones are in use so we can 
iterate over all active zones.

-- 
Regards,
Niklas S�derlund

Re: [PATCH 2/7] thermal: rcar_gen3_thermal: fix probe error path

[Niklas Söderlund]

Hi Wolfram,

Thanks for your feedback.

On 2017-03-07 20:52:03 +0100, Wolfram Sang wrote:
> On Mon, Mar 06, 2017 at 09:03:56PM +0100, Niklas Söderlund wrote:
> > If the memory resource for a TSC is unviable probe should fail not try
> > to go ahead with the remaining TSC. This fix is aligned with other
> > checks in probe where probe fails if they are unavailable.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> I disagree. There are likely SoCs in the future which have less than
> TSC_MAX_NUM sensors (V3M shall have only 1 according to chapter 10C in
> v0.52 documentation). So, the code exits the loop for this case. We
> should move it before the devm_kzalloc(), however.

Ahh I did not think of that. I will drop this patch and instead do as 
you suggest move this before the devm_kzalloc().

> 
> That also means that you can't really iterate over TSC_MAX_NUM in later
> patches, but rather store the amount of instances in the main struct and
> iterate over that value.
> 

I will see how to best solve this, probably we need then to recoded in 
struct rcar_gen3_thermal_priv how many zones are in use so we can 
iterate over all active zones.

-- 
Regards,
Niklas Söderlund

Re: [PATCH 5/7] thermal: rcar_gen3_thermal: enable hardware interrupts for trip points

[Geert Uytterhoeven]

Hi Niklas,

On Fri, Mar 17, 2017 at 11:06 AM, Niklas Söderlund
<niklas.soderlund@ragnatech.se> wrote:
> On 2017-03-07 20:55:33 +0100, Wolfram Sang wrote:
>> On Mon, Mar 06, 2017 at 09:03:59PM +0100, Niklas Söderlund wrote:
>> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>> > ---
>> >  drivers/thermal/rcar_gen3_thermal.c | 137 ++++++++++++++++++++++++++++++++++++
>> >  1 file changed, 137 insertions(+)
>> >
>> > +static int rcar_gen3_thermal_set_trips(void *devdata, int low, int high)
>> > +{
>> > +   struct rcar_gen3_thermal_tsc *tsc = devdata;
>> > +
>> > +   if (low < -40000)
>> > +           low = -40000;
>> > +   if (high > 125000)
>> > +           high = 125000;
>>
>> Use clamp_val()?
>
> Good idea, will use.

Why not clamp()? Does the type checking stand in the way?

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