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

[Scott Wood <scottwood@freescale.com>]

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@gmail.com; linux-pm@vger.kernel.org; linuxppc-
> > dev@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.

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

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

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

-Scott