Hi Daniel,
Thanks for your feedback.
On 2023-02-08 12:06:37 +0100, Daniel Lezcano wrote:
> On 07/02/2023 18:10, Niklas Söderlund wrote:
> > The thermal zone is registered before the device is register and the
> > thermal coefficients are calculated, providing a window for very
> > incorrect readings.
> >
> > The reason why the zone was register before the device was fully
> > initialized was that the presence of the set_trips() callback is used to
> > determine if the driver supports interrupt or not, as it is not defined
> > if the device is incapable of interrupts.
> >
> > Fix this by using the operations structure in the private data instead
> > of the zone to determine if interrupts are available or not, and
> > initialize the device before registering the zone.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > ---
> > drivers/thermal/rcar_gen3_thermal.c | 25 ++++++++++++++-----------
> > 1 file changed, 14 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> > index bfa2ff20b945..1dedeece1a00 100644
> > --- a/drivers/thermal/rcar_gen3_thermal.c
> > +++ b/drivers/thermal/rcar_gen3_thermal.c
> > @@ -89,7 +89,8 @@ struct rcar_gen3_thermal_priv {
> > struct rcar_gen3_thermal_tsc *tscs[TSC_MAX_NUM];
> > struct thermal_zone_device_ops ops;
> > 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);
> > int ptat[3];
> > };
> > @@ -240,7 +241,7 @@ static irqreturn_t rcar_gen3_thermal_irq(int irq, void *data)
> > 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)
> > + if (status && priv->tscs[i]->zone)
> > thermal_zone_device_update(priv->tscs[i]->zone,
> > THERMAL_EVENT_UNSPECIFIED);
> > }
> > @@ -311,7 +312,8 @@ static bool rcar_gen3_thermal_read_fuses(struct rcar_gen3_thermal_priv *priv)
> > return true;
> > }
> > -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);
> > @@ -322,7 +324,7 @@ 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 (tsc->zone->ops->set_trips)
> > + if (priv->ops.set_trips)
> > rcar_gen3_thermal_write(tsc, REG_GEN3_IRQEN,
> > IRQ_TEMPD1 | IRQ_TEMP2);
> > @@ -338,7 +340,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;
> > @@ -350,7 +353,7 @@ 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 (tsc->zone->ops->set_trips)
> > + if (priv->ops.set_trips)
> > rcar_gen3_thermal_write(tsc, REG_GEN3_IRQEN,
> > IRQ_TEMPD1 | IRQ_TEMP2);
> > @@ -510,6 +513,9 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
> > for (i = 0; i < priv->num_tscs; i++) {
> > struct rcar_gen3_thermal_tsc *tsc = priv->tscs[i];
> > + priv->thermal_init(priv, tsc);
> > + rcar_gen3_thermal_calc_coefs(priv, tsc, *ths_tj_1);
> > +
> > zone = devm_thermal_of_zone_register(dev, i, tsc, &priv->ops);
> > if (IS_ERR(zone)) {
> > dev_err(dev, "Sensor %u: Can't register thermal zone\n", i);
> > @@ -518,9 +524,6 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
> > }
> > tsc->zone = zone;
> > - priv->thermal_init(tsc);
> > - rcar_gen3_thermal_calc_coefs(priv, tsc, *ths_tj_1);
> > -
> > tsc->zone->tzp->no_hwmon = false;
> > ret = thermal_add_hwmon_sysfs(tsc->zone);
> > if (ret)
> > @@ -559,8 +562,8 @@ static int __maybe_unused rcar_gen3_thermal_resume(struct device *dev)
> > struct rcar_gen3_thermal_tsc *tsc = priv->tscs[i];
> > struct thermal_zone_device *zone = tsc->zone;
> > - priv->thermal_init(tsc);
> > - if (zone->ops->set_trips)
> > + priv->thermal_init(priv, tsc);
> > + if (priv->ops.set_trips)
> > rcar_gen3_thermal_set_trips(zone, zone->prev_low_trip,
> > zone->prev_high_trip);
>
> This is not needed, at resume time, the thermal framework has a pm_notifier
> and calls thermal_zone_device_update() which in turn calls
> thermal_zone_set_trips(). If the ops is not set, it will continue.
>
> Actually, no call to set_trips should happen in the driver, just pass the
> ops the thermal framework and it will do the actions.
>
> The same happens when you call thermal_zone_device_register(), it calls
> thermal_zone_device_update(), then thermal_zone_set_trips().
I will send a v2 of this series addressing this before fixing the issue
addressed in this patch.
>
>
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
>
--
Kind Regards,
Niklas Söderlund
