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

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

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