[PATCH 0/3] thermal: rcar_gen3_thermal: Add support for trip points

[PATCH 0/3] thermal: rcar_gen3_thermal: Add support for trip points

[Niklas Söderlund]

Hi Daniel,

This small series adds support for the .set_trips() to the 
rcar_gen3_thermal driver on the platforms that supports it.

Patch 1/3 prepares for the new feature by expanding the OF match data 
while patch 2/3 adds the actual change. Patch 3/3 is a drive-by fix of a 
datatype found while working on this feature.

The work is based on-top of thermal/next [1] and tested on M3-N and V3U 
without any regressions or other issues.

1. fe6a6de6692e7f71 ("thermal/drivers/int340x/processor_thermal: Fix tcc setting")

Niklas Söderlund (3):
  thermal: rcar_gen3_thermal: Create struct for OF match data
  thermal: rcar_gen3_thermal: Add support for hardware trip points
  thermal: rcar_gen3_thermal: Fix datatype for loop counter

 drivers/thermal/rcar_gen3_thermal.c | 170 ++++++++++++++++++++++++----
 1 file changed, 150 insertions(+), 20 deletions(-)

-- 
2.32.0

[PATCH 1/3] thermal: rcar_gen3_thermal: Create struct for OF match data

[Niklas Söderlund]

Prepare for storing more information in the OF match data by using a
struct instead of a single integer value. There is no functional change.

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

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index fdf16aa34eb470c7..770f1daae5379053 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -79,6 +79,10 @@ struct equation_coefs {
 	int b2;
 };
 
+struct rcar_gen3_thermal_info {
+	int ths_tj_1;
+};
+
 struct rcar_gen3_thermal_tsc {
 	void __iomem *base;
 	struct thermal_zone_device *zone;
@@ -88,6 +92,7 @@ struct rcar_gen3_thermal_tsc {
 };
 
 struct rcar_gen3_thermal_priv {
+	const struct rcar_gen3_thermal_info *info;
 	struct rcar_gen3_thermal_tsc *tscs[TSC_MAX_NUM];
 	unsigned int num_tscs;
 	void (*thermal_init)(struct rcar_gen3_thermal_tsc *tsc);
@@ -243,44 +248,50 @@ static void rcar_gen3_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
 	usleep_range(1000, 2000);
 }
 
-static const int rcar_gen3_ths_tj_1 = 126;
-static const int rcar_gen3_ths_tj_1_m3_w = 116;
+static const struct rcar_gen3_thermal_info rcar_gen3_thermal_gen3 = {
+	.ths_tj_1 = 126,
+};
+
+static const struct rcar_gen3_thermal_info rcar_gen3_thermal_m3w = {
+	.ths_tj_1 = 116,
+};
+
 static const struct of_device_id rcar_gen3_thermal_dt_ids[] = {
 	{
 		.compatible = "renesas,r8a774a1-thermal",
-		.data = &rcar_gen3_ths_tj_1_m3_w,
+		.data = &rcar_gen3_thermal_m3w,
 	},
 	{
 		.compatible = "renesas,r8a774b1-thermal",
-		.data = &rcar_gen3_ths_tj_1,
+		.data = &rcar_gen3_thermal_gen3,
 	},
 	{
 		.compatible = "renesas,r8a774e1-thermal",
-		.data = &rcar_gen3_ths_tj_1,
+		.data = &rcar_gen3_thermal_gen3,
 	},
 	{
 		.compatible = "renesas,r8a7795-thermal",
-		.data = &rcar_gen3_ths_tj_1,
+		.data = &rcar_gen3_thermal_gen3,
 	},
 	{
 		.compatible = "renesas,r8a7796-thermal",
-		.data = &rcar_gen3_ths_tj_1_m3_w,
+		.data = &rcar_gen3_thermal_m3w,
 	},
 	{
 		.compatible = "renesas,r8a77961-thermal",
-		.data = &rcar_gen3_ths_tj_1_m3_w,
+		.data = &rcar_gen3_thermal_m3w,
 	},
 	{
 		.compatible = "renesas,r8a77965-thermal",
-		.data = &rcar_gen3_ths_tj_1,
+		.data = &rcar_gen3_thermal_gen3,
 	},
 	{
 		.compatible = "renesas,r8a77980-thermal",
-		.data = &rcar_gen3_ths_tj_1,
+		.data = &rcar_gen3_thermal_gen3,
 	},
 	{
 		.compatible = "renesas,r8a779a0-thermal",
-		.data = &rcar_gen3_ths_tj_1,
+		.data = &rcar_gen3_thermal_gen3,
 	},
 	{},
 };
@@ -307,7 +318,6 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 {
 	struct rcar_gen3_thermal_priv *priv;
 	struct device *dev = &pdev->dev;
-	const int *ths_tj_1 = of_device_get_match_data(dev);
 	struct resource *res;
 	struct thermal_zone_device *zone;
 	int ret, i;
@@ -320,6 +330,8 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 	if (!priv)
 		return -ENOMEM;
 
+	priv->info = of_device_get_match_data(dev);
+
 	priv->thermal_init = rcar_gen3_thermal_init;
 	if (soc_device_match(r8a7795es1))
 		priv->thermal_init = rcar_gen3_thermal_init_r8a7795es1;
@@ -352,7 +364,8 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 		priv->tscs[i] = tsc;
 
 		priv->thermal_init(tsc);
-		rcar_gen3_thermal_calc_coefs(tsc, ptat, thcodes[i], *ths_tj_1);
+		rcar_gen3_thermal_calc_coefs(tsc, ptat, thcodes[i],
+					     priv->info->ths_tj_1);
 
 		zone = devm_thermal_zone_of_sensor_register(dev, i, tsc,
 							    &rcar_gen3_tz_of_ops);
-- 
2.32.0

Re: [PATCH 1/3] thermal: rcar_gen3_thermal: Create struct for OF match data

[Geert Uytterhoeven]

On Wed, Jul 7, 2021 at 3:14 PM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> Prepare for storing more information in the OF match data by using a
> struct instead of a single integer value. There is no functional change.
>
> 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

[PATCH 2/3] thermal: rcar_gen3_thermal: Add support for hardware trip points

[Niklas Söderlund]

All supported hardware except V3U is capable of generating interrupts
to the CPU when the temperature go below or above a set value. Use this
to implement support for the set_trip() feature of the thermal core on
supported hardware.

The V3U have its interrupts routed to the ECM module and therefore can
not be used to implement set_trip() as the driver can't be made aware of
when the interrupt triggers.

Each TSC is capable of tracking up-to three different temperatures while
only two are needed to implement the tracking of the thermal window.

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

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index 770f1daae5379053..a438e05e7ca7f20e 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -81,6 +81,7 @@ struct equation_coefs {
 
 struct rcar_gen3_thermal_info {
 	int ths_tj_1;
+	bool have_irq;
 };
 
 struct rcar_gen3_thermal_tsc {
@@ -95,7 +96,8 @@ struct rcar_gen3_thermal_priv {
 	const struct rcar_gen3_thermal_info *info;
 	struct rcar_gen3_thermal_tsc *tscs[TSC_MAX_NUM];
 	unsigned int num_tscs;
-	void (*thermal_init)(struct rcar_gen3_thermal_tsc *tsc);
+	void (*thermal_init)(struct rcar_gen3_thermal_priv *priv,
+			     struct rcar_gen3_thermal_tsc *tsc);
 };
 
 static inline u32 rcar_gen3_thermal_read(struct rcar_gen3_thermal_tsc *tsc,
@@ -195,16 +197,75 @@ 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, val;
+
+	celsius = DIV_ROUND_CLOSEST(mcelsius, 1000);
+	if (celsius <= INT_FIXPT(tsc->tj_t))
+		val = celsius * tsc->coef.a1 + tsc->coef.b1;
+	else
+		val = celsius * tsc->coef.a2 + tsc->coef.b2;
+
+	return INT_FIXPT(val);
+}
+
+static int rcar_gen3_thermal_set_trips(void *devdata, int low, int high)
+{
+	struct rcar_gen3_thermal_tsc *tsc = devdata;
+	u32 irqmsk = 0;
+
+	if (low != -INT_MAX) {
+		irqmsk |= IRQ_TEMPD1;
+		rcar_gen3_thermal_write(tsc, REG_GEN3_IRQTEMP1,
+					rcar_gen3_thermal_mcelsius_to_temp(tsc, low));
+	}
+
+	if (high != INT_MAX) {
+		irqmsk |= IRQ_TEMP2;
+		rcar_gen3_thermal_write(tsc, REG_GEN3_IRQTEMP2,
+					rcar_gen3_thermal_mcelsius_to_temp(tsc, high));
+	}
+
+	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQMSK, irqmsk);
+
+	return 0;
+}
+
 static const 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 const struct thermal_zone_of_device_ops rcar_gen3_tz_of_ops_no_irq = {
+	.get_temp	= rcar_gen3_thermal_get_temp,
+};
+
+static irqreturn_t rcar_gen3_thermal_irq(int irq, void *data)
+{
+	struct rcar_gen3_thermal_priv *priv = data;
+	unsigned int i;
+	u32 status;
+
+	for (i = 0; i < priv->num_tscs; 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)
+			thermal_zone_device_update(priv->tscs[i]->zone,
+						   THERMAL_EVENT_UNSPECIFIED);
+	}
+
+	return IRQ_HANDLED;
+}
+
 static const struct soc_device_attribute r8a7795es1[] = {
 	{ .soc_id = "r8a7795", .revision = "ES1.*" },
 	{ /* sentinel */ }
 };
 
-static void rcar_gen3_thermal_init_r8a7795es1(struct rcar_gen3_thermal_tsc *tsc)
+static void rcar_gen3_thermal_init_r8a7795es1(struct rcar_gen3_thermal_priv *priv,
+					      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);
@@ -215,6 +276,9 @@ static void rcar_gen3_thermal_init_r8a7795es1(struct rcar_gen3_thermal_tsc *tsc)
 
 	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQCTL, 0x3F);
 	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQMSK, 0);
+	if (priv->info->have_irq)
+		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);
@@ -228,7 +292,8 @@ static void rcar_gen3_thermal_init_r8a7795es1(struct rcar_gen3_thermal_tsc *tsc)
 	usleep_range(1000, 2000);
 }
 
-static void rcar_gen3_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
+static void rcar_gen3_thermal_init(struct rcar_gen3_thermal_priv *priv,
+				   struct rcar_gen3_thermal_tsc *tsc)
 {
 	u32 reg_val;
 
@@ -240,6 +305,9 @@ static void rcar_gen3_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
 
 	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQCTL, 0);
 	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQMSK, 0);
+	if (priv->info->have_irq)
+		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;
@@ -250,10 +318,16 @@ static void rcar_gen3_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
 
 static const struct rcar_gen3_thermal_info rcar_gen3_thermal_gen3 = {
 	.ths_tj_1 = 126,
+	.have_irq = true,
 };
 
 static const struct rcar_gen3_thermal_info rcar_gen3_thermal_m3w = {
 	.ths_tj_1 = 116,
+	.have_irq = true,
+};
+
+static const struct rcar_gen3_thermal_info rcar_gen3_thermal_v3u = {
+	.ths_tj_1 = 126,
 };
 
 static const struct of_device_id rcar_gen3_thermal_dt_ids[] = {
@@ -291,7 +365,7 @@ static const struct of_device_id rcar_gen3_thermal_dt_ids[] = {
 	},
 	{
 		.compatible = "renesas,r8a779a0-thermal",
-		.data = &rcar_gen3_thermal_gen3,
+		.data = &rcar_gen3_thermal_v3u,
 	},
 	{},
 };
@@ -314,8 +388,37 @@ static void rcar_gen3_hwmon_action(void *data)
 	thermal_remove_hwmon_sysfs(zone);
 }
 
+static int rcar_gen3_thermal_request_irqs(struct rcar_gen3_thermal_priv *priv,
+					  struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	unsigned int i;
+	char *irqname;
+	int ret, irq;
+
+	for (i = 0; i < 2; i++) {
+		irq = platform_get_irq(pdev, i);
+		if (irq < 0)
+			return irq;
+
+		irqname = devm_kasprintf(dev, GFP_KERNEL, "%s:ch%d",
+					 dev_name(dev), i);
+		if (!irqname)
+			return -ENOMEM;
+
+		ret = devm_request_threaded_irq(dev, irq, NULL,
+						rcar_gen3_thermal_irq,
+						IRQF_ONESHOT, irqname, priv);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 {
+	const struct thermal_zone_of_device_ops *tz_of_ops;
 	struct rcar_gen3_thermal_priv *priv;
 	struct device *dev = &pdev->dev;
 	struct resource *res;
@@ -338,6 +441,15 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, priv);
 
+	tz_of_ops = &rcar_gen3_tz_of_ops_no_irq;
+	if (priv->info->have_irq) {
+		ret = rcar_gen3_thermal_request_irqs(priv, pdev);
+		if (ret)
+			return ret;
+
+		tz_of_ops = &rcar_gen3_tz_of_ops;
+	}
+
 	pm_runtime_enable(dev);
 	pm_runtime_get_sync(dev);
 
@@ -363,12 +475,12 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 
 		priv->tscs[i] = tsc;
 
-		priv->thermal_init(tsc);
+		priv->thermal_init(priv, tsc);
 		rcar_gen3_thermal_calc_coefs(tsc, ptat, thcodes[i],
 					     priv->info->ths_tj_1);
 
 		zone = devm_thermal_zone_of_sensor_register(dev, i, tsc,
-							    &rcar_gen3_tz_of_ops);
+							    tz_of_ops);
 		if (IS_ERR(zone)) {
 			dev_err(dev, "Can't register thermal zone\n");
 			ret = PTR_ERR(zone);
@@ -414,8 +526,12 @@ static int __maybe_unused rcar_gen3_thermal_resume(struct device *dev)
 
 	for (i = 0; i < priv->num_tscs; i++) {
 		struct rcar_gen3_thermal_tsc *tsc = priv->tscs[i];
+		struct thermal_zone_device *zone = tsc->zone;
 
-		priv->thermal_init(tsc);
+		priv->thermal_init(priv, tsc);
+		if (priv->info->have_irq)
+			rcar_gen3_thermal_set_trips(tsc, zone->prev_low_trip,
+						    zone->prev_high_trip);
 	}
 
 	return 0;
-- 
2.32.0

Re: [PATCH 2/3] thermal: rcar_gen3_thermal: Add support for hardware trip points

[Geert Uytterhoeven]

Hi Niklas,

On Wed, Jul 7, 2021 at 3:14 PM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> All supported hardware except V3U is capable of generating interrupts
> to the CPU when the temperature go below or above a set value. Use this
> to implement support for the set_trip() feature of the thermal core on
> supported hardware.
>
> The V3U have its interrupts routed to the ECM module and therefore can
> not be used to implement set_trip() as the driver can't be made aware of
> when the interrupt triggers.
>
> Each TSC is capable of tracking up-to three different temperatures while
> only two are needed to implement the tracking of the thermal window.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Thanks for your patch!

> --- a/drivers/thermal/rcar_gen3_thermal.c
> +++ b/drivers/thermal/rcar_gen3_thermal.c
> @@ -81,6 +81,7 @@ struct equation_coefs {
>
>  struct rcar_gen3_thermal_info {
>         int ths_tj_1;
> +       bool have_irq;

Do you need this flag? See below.

>  };
>
>  struct rcar_gen3_thermal_tsc {
> @@ -95,7 +96,8 @@ struct rcar_gen3_thermal_priv {
>         const struct rcar_gen3_thermal_info *info;
>         struct rcar_gen3_thermal_tsc *tscs[TSC_MAX_NUM];
>         unsigned int num_tscs;
> -       void (*thermal_init)(struct rcar_gen3_thermal_tsc *tsc);
> +       void (*thermal_init)(struct rcar_gen3_thermal_priv *priv,

Do you need priv? See below.

> +                            struct rcar_gen3_thermal_tsc *tsc);
>  };
>
>  static inline u32 rcar_gen3_thermal_read(struct rcar_gen3_thermal_tsc *tsc,
> @@ -195,16 +197,75 @@ static int rcar_gen3_thermal_get_temp(void *devdata, int *temp)

>  static const 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 const struct thermal_zone_of_device_ops rcar_gen3_tz_of_ops_no_irq = {
> +       .get_temp       = rcar_gen3_thermal_get_temp,
> +};

What about having a single non-const thermal_zone_of_device_ops,
and filling in .set_trip when interrupts are present?

> @@ -240,6 +305,9 @@ static void rcar_gen3_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
>
>         rcar_gen3_thermal_write(tsc, REG_GEN3_IRQCTL, 0);
>         rcar_gen3_thermal_write(tsc, REG_GEN3_IRQMSK, 0);
> +       if (priv->info->have_irq)

I think you can check for the presence of tsc->zone->ops->set_trips instead.

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

> @@ -314,8 +388,37 @@ static void rcar_gen3_hwmon_action(void *data)
>         thermal_remove_hwmon_sysfs(zone);
>  }
>
> +static int rcar_gen3_thermal_request_irqs(struct rcar_gen3_thermal_priv *priv,
> +                                         struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       unsigned int i;
> +       char *irqname;
> +       int ret, irq;
> +
> +       for (i = 0; i < 2; i++) {
> +               irq = platform_get_irq(pdev, i);

Would it make sense to use platform_get_irq_optional() instead,
to auto-detect variants with and without interrupt support?

> +               if (irq < 0)
> +                       return irq;
> +
> +               irqname = devm_kasprintf(dev, GFP_KERNEL, "%s:ch%d",
> +                                        dev_name(dev), i);
> +               if (!irqname)
> +                       return -ENOMEM;
> +
> +               ret = devm_request_threaded_irq(dev, irq, NULL,
> +                                               rcar_gen3_thermal_irq,
> +                                               IRQF_ONESHOT, irqname, priv);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       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 2/3] thermal: rcar_gen3_thermal: Add support for hardware trip points

[Niklas Söderlund]

Hi Geert,

Thanks for your feedback.

On 2021-07-07 16:58:33 +0200, Geert Uytterhoeven wrote:
> Hi Niklas,
> 
> On Wed, Jul 7, 2021 at 3:14 PM Niklas Söderlund
> <niklas.soderlund+renesas@ragnatech.se> wrote:
> > All supported hardware except V3U is capable of generating interrupts
> > to the CPU when the temperature go below or above a set value. Use this
> > to implement support for the set_trip() feature of the thermal core on
> > supported hardware.
> >
> > The V3U have its interrupts routed to the ECM module and therefore can
> > not be used to implement set_trip() as the driver can't be made aware of
> > when the interrupt triggers.
> >
> > Each TSC is capable of tracking up-to three different temperatures while
> > only two are needed to implement the tracking of the thermal window.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> Thanks for your patch!
> 
> > --- a/drivers/thermal/rcar_gen3_thermal.c
> > +++ b/drivers/thermal/rcar_gen3_thermal.c
> > @@ -81,6 +81,7 @@ struct equation_coefs {
> >
> >  struct rcar_gen3_thermal_info {
> >         int ths_tj_1;
> > +       bool have_irq;
> 
> Do you need this flag? See below.
> 
> >  };
> >
> >  struct rcar_gen3_thermal_tsc {
> > @@ -95,7 +96,8 @@ struct rcar_gen3_thermal_priv {
> >         const struct rcar_gen3_thermal_info *info;
> >         struct rcar_gen3_thermal_tsc *tscs[TSC_MAX_NUM];
> >         unsigned int num_tscs;
> > -       void (*thermal_init)(struct rcar_gen3_thermal_tsc *tsc);
> > +       void (*thermal_init)(struct rcar_gen3_thermal_priv *priv,
> 
> Do you need priv? See below.
> 
> > +                            struct rcar_gen3_thermal_tsc *tsc);
> >  };
> >
> >  static inline u32 rcar_gen3_thermal_read(struct rcar_gen3_thermal_tsc *tsc,
> > @@ -195,16 +197,75 @@ static int rcar_gen3_thermal_get_temp(void *devdata, int *temp)
> 
> >  static const 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 const struct thermal_zone_of_device_ops rcar_gen3_tz_of_ops_no_irq = {
> > +       .get_temp       = rcar_gen3_thermal_get_temp,
> > +};
> 
> What about having a single non-const thermal_zone_of_device_ops,
> and filling in .set_trip when interrupts are present?

I could, for some reason I like ops structs to be cost. It prevents me 
from modifying it's members after they have been used in a framework 
init function somewhere which judged the driver capabilities on its 
populated members ;-)

But this is a simple thing in this driver, I will give it a try for v2.

> 
> > @@ -240,6 +305,9 @@ static void rcar_gen3_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
> >
> >         rcar_gen3_thermal_write(tsc, REG_GEN3_IRQCTL, 0);
> >         rcar_gen3_thermal_write(tsc, REG_GEN3_IRQMSK, 0);
> > +       if (priv->info->have_irq)
> 
> I think you can check for the presence of tsc->zone->ops->set_trips instead.

Good idea!

> 
> > +               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;
> 
> > @@ -314,8 +388,37 @@ static void rcar_gen3_hwmon_action(void *data)
> >         thermal_remove_hwmon_sysfs(zone);
> >  }
> >
> > +static int rcar_gen3_thermal_request_irqs(struct rcar_gen3_thermal_priv *priv,
> > +                                         struct platform_device *pdev)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +       unsigned int i;
> > +       char *irqname;
> > +       int ret, irq;
> > +
> > +       for (i = 0; i < 2; i++) {
> > +               irq = platform_get_irq(pdev, i);
> 
> Would it make sense to use platform_get_irq_optional() instead,
> to auto-detect variants with and without interrupt support?

The bindings require interrupts be present for all variants but V3U. But 
since auto-detect is a good thing and with the method you describe it's 
easy to add, will give it a try for a v2.

> 
> > +               if (irq < 0)
> > +                       return irq;
> > +
> > +               irqname = devm_kasprintf(dev, GFP_KERNEL, "%s:ch%d",
> > +                                        dev_name(dev), i);
> > +               if (!irqname)
> > +                       return -ENOMEM;
> > +
> > +               ret = devm_request_threaded_irq(dev, irq, NULL,
> > +                                               rcar_gen3_thermal_irq,
> > +                                               IRQF_ONESHOT, irqname, priv);
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +
> > +       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

-- 
Regards,
Niklas Söderlund

[PATCH 3/3] thermal: rcar_gen3_thermal: Fix datatype for loop counter

[Niklas Söderlund]

The loop counter 'i' should be unsigned int to match struct
rcar_gen3_thermal_priv num_tscs where it's stored.

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

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index a438e05e7ca7f20e..8ca4bfee7bca4122 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -423,7 +423,8 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct resource *res;
 	struct thermal_zone_device *zone;
-	int ret, i;
+	unsigned int i;
+	int ret;
 
 	/* default values if FUSEs are missing */
 	/* TODO: Read values from hardware on supported platforms */
-- 
2.32.0

Re: [PATCH 3/3] thermal: rcar_gen3_thermal: Fix datatype for loop counter

[Geert Uytterhoeven]

Hi Niklas,

Thanks for your patch!

On Wed, Jul 7, 2021 at 3:14 PM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> The loop counter 'i' should be unsigned int to match struct
> rcar_gen3_thermal_priv num_tscs where it's stored.

It is also stored in rcar_gen3_thermal_tsc.id, which is still (signed) int.

> --- a/drivers/thermal/rcar_gen3_thermal.c
> +++ b/drivers/thermal/rcar_gen3_thermal.c
> @@ -423,7 +423,8 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
>         struct device *dev = &pdev->dev;
>         struct resource *res;
>         struct thermal_zone_device *zone;
> -       int ret, i;
> +       unsigned int i;
> +       int ret;
>
>         /* default values if FUSEs are missing */
>         /* TODO: Read values from hardware on supported platforms */

And perhaps:

-                dev_info(dev, "TSC%d: Loaded %d trip points\n", i, ret);
+                dev_info(dev, "TSC%u: Loaded %d trip points\n", i, ret);

?

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/3] thermal: rcar_gen3_thermal: Fix datatype for loop counter

[Niklas Söderlund]

Hi Geert,

Thanks for your feedback.

On 2021-07-07 16:42:10 +0200, Geert Uytterhoeven wrote:
> Hi Niklas,
> 
> Thanks for your patch!
> 
> On Wed, Jul 7, 2021 at 3:14 PM Niklas Söderlund
> <niklas.soderlund+renesas@ragnatech.se> wrote:
> > The loop counter 'i' should be unsigned int to match struct
> > rcar_gen3_thermal_priv num_tscs where it's stored.
> 
> It is also stored in rcar_gen3_thermal_tsc.id, which is still (signed) int.

Thanks.

> 
> > --- a/drivers/thermal/rcar_gen3_thermal.c
> > +++ b/drivers/thermal/rcar_gen3_thermal.c
> > @@ -423,7 +423,8 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
> >         struct device *dev = &pdev->dev;
> >         struct resource *res;
> >         struct thermal_zone_device *zone;
> > -       int ret, i;
> > +       unsigned int i;
> > +       int ret;
> >
> >         /* default values if FUSEs are missing */
> >         /* TODO: Read values from hardware on supported platforms */
> 
> And perhaps:
> 
> -                dev_info(dev, "TSC%d: Loaded %d trip points\n", i, ret);
> +                dev_info(dev, "TSC%u: Loaded %d trip points\n", i, ret);
> 
> ?

Good catch, why do my compiler not warn for this? Did you catch this by 
review or have you some compiler magic I'm missing?

> 
> 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 3/3] thermal: rcar_gen3_thermal: Fix datatype for loop counter

[Geert Uytterhoeven]

Hi Niklas,

On Wed, Jul 7, 2021 at 8:56 PM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> On 2021-07-07 16:42:10 +0200, Geert Uytterhoeven wrote:
> > On Wed, Jul 7, 2021 at 3:14 PM Niklas Söderlund
> > <niklas.soderlund+renesas@ragnatech.se> wrote:
> > > The loop counter 'i' should be unsigned int to match struct
> > > rcar_gen3_thermal_priv num_tscs where it's stored.
> >
> > It is also stored in rcar_gen3_thermal_tsc.id, which is still (signed) int.
>
> Thanks.
>
> >
> > > --- a/drivers/thermal/rcar_gen3_thermal.c
> > > +++ b/drivers/thermal/rcar_gen3_thermal.c
> > > @@ -423,7 +423,8 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
> > >         struct device *dev = &pdev->dev;
> > >         struct resource *res;
> > >         struct thermal_zone_device *zone;
> > > -       int ret, i;
> > > +       unsigned int i;
> > > +       int ret;
> > >
> > >         /* default values if FUSEs are missing */
> > >         /* TODO: Read values from hardware on supported platforms */
> >
> > And perhaps:
> >
> > -                dev_info(dev, "TSC%d: Loaded %d trip points\n", i, ret);
> > +                dev_info(dev, "TSC%u: Loaded %d trip points\n", i, ret);
> >
> > ?
>
> Good catch, why do my compiler not warn for this? Did you catch this by

Because the types are (somewhat) compatible.

> review or have you some compiler magic I'm missing?

Review. You changed the type of a variable, I checked all places where
the variable is used.

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