[PATCH V3] thermal: qoriq: Add thermal management support

[Hongtao Jia]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Friday, September 25, 2015 1:12 PM
> To: Jia Hongtao-B38951
> Cc: edubezval at gmail.com; linux-pm at vger.kernel.org; linuxppc-
> dev at lists.ozlabs.org
> Subject: Re: [PATCH V3] thermal: qoriq: Add thermal management support
> 
> On Thu, 2015-09-24 at 22:09 -0500, Jia Hongtao-B38951 wrote:
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Friday, September 25, 2015 6:10 AM
> > > To: Jia Hongtao-B38951
> > > Cc: edubezval at gmail.com; linux-pm at vger.kernel.org; linuxppc-
> > > dev at lists.ozlabs.org
> > > Subject: Re: [PATCH V3] thermal: qoriq: Add thermal management
> support
> > >
> > > On Wed, 2015-09-23 at 16:28 +0800, Jia Hongtao wrote:
> > > > This driver add thermal management support by enabling TMU (Thermal
> > > > Monitoring Unit) on QorIQ platform.
> > > >
> > > > It's based on thermal of framework:
> > > > - Trip points defined in device tree.
> > > > - Cpufreq as cooling device registered in qoriq cpufreq driver.
> > >
> > > I don't see any cooling device registered in the qoriq cpufreq driver.
> > > Is this dependent on some other patch?
> >
> > It's not depend on any patch. But I saw your patch below:
> > [PATCH v3 5/5] cpufreq: qoriq: Don't look at clock implementation
> details
> > So I hold my patch waiting for your patch merged or there will be
> conflict.
> >
> > I could send it out too if you are fine with it.
> 
> Yes, rebase it on that patch and send it.  Mention below the --- that
> you're
> depending on that patch, and provide a patchwork/archive link.

Ok. But I need rebase again if you have further changes on that patch.
Anyway, this is not a big problem for me.

> 
> > > > +static int tmu_get_temp(void *p, int *temp) {
> > > > +     u8 val;
> > > > +     struct qoriq_tmu_data *data = p;
> > > > +
> > > > +     val = ioread32be(&data->regs->site[0].tritsr);
> > > > +     *temp = (int)val * 1000;
> > >
> > > Why don't you declare val as int in the first place?
> >
> > It's a 32bit register.
> > Only the least significant 8 bits represent the temperature.
> > The most significant bit shows the validness of the value.
> > I use u8 type to remove the rest 24bits influence.
> 
> That's even worse.  Use an explicit & operation to extract the field
> you're
> interested in.

Will change for next version.

> 
> > > > +     ret = qoriq_tmu_calibration(pdev);      /* TMU calibration */
> > > > +     if (ret < 0) {
> > > > +             dev_err(&pdev->dev, "TMU calibration failed.\n");
> > > > +             ret = -ENODEV;
> > > > +             goto err_iomap;
> > > > +     }
> > >
> > > That function returns negative when device tree properties are
> missing,
> > > not when a calibration procedure fails.
> >
> > What's your suggestion here to return then?
> 
> Remove this message and add a message in qoriq_tmu_calibration for each
> error
> condition.  Also have qoriq_tmu_calibration pass back a proper error code,
> not -1.

Yes, this is more reasonable.

> 
> > > > +static const struct of_device_id qoriq_tmu_match[] = {
> > > > +     { .compatible = "fsl,qoriq-tmu", },
> > > > +     {},
> > > > +};
> > >
> > > Binding?
> >
> > Not send out yet.
> 
> The binding needs to come before the driver that uses it.

Right. I will send out the whole patchset.

> 
> -Scott
>