Re: [PATCH 1/4] thermal: Add driver for Armada 370/XP SoC thermal management

[Ezequiel Garcia <ezequiel.garcia@free-electrons.com>]

Hi Durgadoss,

On Mon, Mar 25, 2013 at 05:27:24PM +0000, R, Durgadoss wrote:
[...]
> > +static void armadaxp_init_sensor(struct armada_thermal_priv *priv)
> > +{
> > +	unsigned long reg;
> > +
> > +	/* ??? */
> > +	reg = readl_relaxed(priv->control);
> > +	reg |= PMU_TDC0_OTF_CAL_MASK;
> > +	writel(reg, priv->control);
> > +
> > +	/* Reference calibration value */
> > +	reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
> > +	reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
> > +	writel(reg, priv->control);
> 
> I see these two blocks of code being the same for the below
> function as well. Any specific reason for not making this block 
> as a common function and calling it from both the 
> _init_sensor functions ?
> 

I think it's more clear if we define one init_sensor function per SoC.
The common code is really little and factor that out seems to me like
too much modularization.

> > +
> > +	/* Reset the sensor */
> > +	reg = readl_relaxed(priv->control);
> > +	writel((reg | PMU_TDC0_SW_RST_MASK), priv->control);
> > +
> > +	writel(reg, priv->control);
> > +
> > +	/* Enable the sensor */
> > +	reg = readl_relaxed(priv->sensor);
> > +	reg &= ~PMU_TM_DISABLE_MASK;
> > +	writel(reg, priv->sensor);
> > +}
> > +
> > +static void armada370_init_sensor(struct armada_thermal_priv *priv)
> > +{
> > +	unsigned long reg;
> > +
> > +	/* ??? */
> > +	reg = readl_relaxed(priv->control);
> > +	reg |= PMU_TDC0_OTF_CAL_MASK;
> > +	writel(reg, priv->control);
> > +
> > +	/* Reference calibration value */
> > +	reg &= ~PMU_TDC0_REF_CAL_CNT_MASK;
> > +	reg |= (0xf1 << PMU_TDC0_REF_CAL_CNT_OFFS);
> > +	writel(reg, priv->control);
> > +
> > +	/* ??? */
> > +	reg &= ~PMU_TDC0_START_CAL_MASK;
> > +	writel(reg, priv->control);
> > +
> > +	/* FIXME: Why do we need this delay? */
> > +	mdelay(10);
> > +}
> > +
> > +static bool armada_is_valid(struct armada_thermal_priv *priv)
> > +{
> > +	unsigned long reg = readl_relaxed(priv->sensor);
> > +
> > +	return (reg >> THERMAL_VALID_OFFSET) & THERMAL_VALID_MASK;
> > +}
> > +
> > +static int armada_get_temp(struct thermal_zone_device *thermal,
> > +			  unsigned long *temp)
> > +{
> > +	struct armada_thermal_priv *priv = thermal->devdata;
> > +	unsigned long reg;
> > +
> > +	/* Valid check */
> > +	if (priv->ops->is_valid && !priv->ops->is_valid(priv)) {
> > +		dev_err(&thermal->device,
> > +			"Temperature sensor reading not valid\n");
> > +		return -EIO;
> > +	}
> > +
> > +	reg = (readl_relaxed(priv->sensor) >> THERMAL_TEMP_OFFSET) &
> 
> Can we have the readl_relaxed call as a separate statement ?
> 

Why would we want that? Do you think it'll be more readable?

> > +		THERMAL_TEMP_MASK;
> > +	*temp = (3153000000UL - (10000000UL*reg)) / 13825;
> 
> If I substitute 1 for 'reg' I get  227341.7721... 
> Does this mean the temperature is 227 C ??
> 

Yes, I guess so.

> If you have the info, can you add a comment on what is the
> valid range that 'reg' can take ?
>

No, I don't have the info. I guess that the valid range 'reg'
can take are the values that span a temperature between 25 ºC (or
lower if it's winter) and when your CPU is on fire :-)

> Also, Is the resulting temperature
> in MillidegreeCelsius ? If so, please add a comment saying so.
> 

Yes, the resulting temperature is in millidegree celsius,
as required by the thermal framework:

Documentation/thermal/sysfs-api.txt

Thanks for the review!
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com