Re: [PATCH 12/12] thermal: qoriq: Add support for multiple thremal sites

[Andrey Smirnov]

On Wed, Feb 20, 2019 at 4:57 PM Eduardo Valentin <edubezval@xxxxxxxxx> wrote:
>
> On Mon, Feb 18, 2019 at 11:11:41AM -0800, Andrey Smirnov wrote:
> > TMU IP block provides temerature measurement for up to 16 sites,
> > current implementation of the driver however hardcodes a single site
> > ID. Change the code so it would be possible to reference multiple
> > sites as indivudial sensors and get their temperature readings.
>
> small typo: indivudial/individual.
>
> >
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@xxxxxxxxx>
> > Cc: Chris Healy <cphealy@xxxxxxxxx>
> > Cc: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
> > Cc: Zhang Rui <rui.zhang@xxxxxxxxx>
> > Cc: Eduardo Valentin <edubezval@xxxxxxxxx>
> > Cc: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> > Cc: linux-imx@xxxxxxx
> > Cc: linux-pm@xxxxxxxxxxxxxxx
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > ---
> >  drivers/thermal/qoriq_thermal.c | 132 ++++++++++++++++++++------------
> >  1 file changed, 84 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/thermal/qoriq_thermal.c b/drivers/thermal/qoriq_thermal.c
> > index f746c62789b0..6cc6e6b36fb0 100644
> > --- a/drivers/thermal/qoriq_thermal.c
> > +++ b/drivers/thermal/qoriq_thermal.c
> > @@ -24,6 +24,8 @@
> >  #define TMR_DISABLE  0x0
> >  #define TMR_ME               0x80000000
> >  #define TMR_ALPF     0x0c000000
> > +#define TMR_ALPF_MASK        GENMASK(27, 26)
> > +
> >
> >  #define REGS_TMTMIR  0x008   /* Temperature measurement interval Register */
> >  #define TMTMIR_DEFAULT       0x0000000f
> > @@ -41,21 +43,32 @@
> >  #define REGS_TTRnCR(n)       (0xf10 + 4 * (n)) /* Temperature Range n
> >                                          * Control Register
> >                                          */
> > +struct qoriq_tmu_sensor {
> > +     struct thermal_zone_device *tz;
> > +     int id;
> > +};
> > +
> >  /*
> >   * Thermal zone data
> >   */
> >  struct qoriq_tmu_data {
> >       struct thermal_zone_device *tz;
> >       struct regmap *regmap;
> > -     int sensor_id;
> > +     struct qoriq_tmu_sensor sensors[SITES_MAX];
> >  };
> >
> > +static struct qoriq_tmu_data *qoriq_sensor_to_data(struct qoriq_tmu_sensor *s)
> > +{
> > +     return container_of(s, struct qoriq_tmu_data, sensors[s->id]);
> > +}
> > +
> >  static int tmu_get_temp(void *p, int *temp)
> >  {
> >       u32 val;
> > -     struct qoriq_tmu_data *data = p;
> > +     struct qoriq_tmu_sensor *s = p;
> > +     struct qoriq_tmu_data *data = qoriq_sensor_to_data(s);
> >
> > -     regmap_read(data->regmap, REGS_TRITSR(data->sensor_id), &val);
> > +     regmap_read(data->regmap, REGS_TRITSR(s->id), &val);
> >       if (!(val & TRITSR_V))
> >               return -ENODATA;
> >
> > @@ -63,38 +76,86 @@ static int tmu_get_temp(void *p, int *temp)
> >       return 0;
> >  }
> >
> > -static int qoriq_tmu_get_sensor_id(void)
> > +static const struct thermal_zone_of_device_ops tmu_tz_ops = {
> > +     .get_temp = tmu_get_temp,
> > +};
> > +
> > +static int qoriq_tmu_populate_sensors(struct device *dev,
> > +                                   struct qoriq_tmu_data *data)
> >  {
> > -     int ret, id;
> > +     int ret, id, i, count = 0;
> >       struct of_phandle_args sensor_specs;
> >       struct device_node *np, *sensor_np;
> > +     struct qoriq_tmu_sensor *s;
> >
> >       np = of_find_node_by_name(NULL, "thermal-zones");
> >       if (!np)
> >               return -ENODEV;
> >
> > -     sensor_np = of_get_next_child(np, NULL);
> > -     ret = of_parse_phandle_with_args(sensor_np, "thermal-sensors",
> > -                     "#thermal-sensor-cells",
> > -                     0, &sensor_specs);
> > -     if (ret) {
> > -             id = ret;
> > -             goto out;
> > +     for_each_child_of_node(np, sensor_np) {
> > +             u16 msite;
> > +
> > +             ret = of_parse_phandle_with_args(sensor_np, "thermal-sensors",
> > +                                              "#thermal-sensor-cells",
> > +                                              0, &sensor_specs);
> > +             if (ret)
> > +                     break;
> > +
> > +             if (sensor_specs.args_count != 1) {
> > +                     pr_err("%pOFn: Invalid number of cells in sensor specifier %d\n",
> > +                            sensor_specs.np, sensor_specs.args_count);
> > +                     ret = -EINVAL;
> > +                     break;
> > +             }
> > +
> > +             id = sensor_specs.args[0];
> > +             if (id >= SITES_MAX) {
> > +                     ret = -EINVAL;
> > +                     dev_err(dev, "Sensor id %d is out of range\n", id);
> > +                     break;
> > +             }
> > +
> > +             msite = BIT(15 - id);
> > +             /* Enable monitoring of that particular sensor*/
> > +             regmap_update_bits(data->regmap, REGS_TMR, msite, msite);
> > +
> > +             s = &data->sensors[id];
> > +             s->id = id;
> > +             count++;
>
> Not sure I follow why you need to bother about thermal-sensor-cells..
>

devm_thermal_zone_of_sensor_register() will call
.get_temp()/tmu_get_temp(), so all present sensors need to be enabled
before that to avoid returning bogus data. The first loop going over
thermal-sensor-cells does exactly that as well as count # of sensors
present to avoid having to loop over all 16 possible sensors in the
code that follow.

Given how there's already a patch adding multiple sensors support, not
a lot of this patch will probably survive, so we can pretty much
disregard it.

Thanks,
Andrey Smirnov