Re: [PATCH v7] thermal: add temperature sensor support for tango SoC

[Eduardo Valentin <edubezval@gmail.com>]

Hello Mason,

On Tue, Apr 05, 2016 at 04:58:22PM +0200, Mason wrote:
> On 04/04/2016 13:49, Mason wrote:
> 
> > +static bool temp_above_thresh(void __iomem *base, int thresh_idx)
> > +{
> > +	writel(CMD_READ | thresh_idx << 8, base + TEMPSI_CMD);
> > +	usleep_range(10, 20);
> > +	return readl(base + TEMPSI_RES);
> > +}
> 
> A colleague suggested a tweak for temp_above_thresh() which does not
> require messing with TEMPSI_CFG => v8 required.

OK.

> 
> > +static int tango_thermal_probe(struct platform_device *pdev)
> > +{
> > +	struct resource *res;
> > +	struct tango_thermal_priv *priv;
> > +
> > +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	priv->base = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(priv->base))
> > +		return PTR_ERR(priv->base);
> > +
> > +	writel(CMD_ON, priv->base + TEMPSI_CMD);
> > +	writel(CYCLE_COUNT, priv->base + TEMPSI_CFG);
> > +
> > +	priv->zone = thermal_zone_of_sensor_register(&pdev->dev, 0, priv, &ops);
> > +	if (IS_ERR(priv->zone))
> > +		return PTR_ERR(priv->zone);
> > +
> > +	priv->thresh_idx = IDX_MIN;
> > +	platform_set_drvdata(pdev, priv);
> > +
> > +	return 0;
> > +}
> 
> It seems tango_get_temp() is called (at least once) before the end of tango_thermal_probe()
> 
> Does that mean that the device must be fully initialized *BEFORE*
> thermal_zone_of_sensor_register() is called?
> (I assume it's that function that triggers the call to get_temp)
> 
> In that case, I need to move priv->thresh_idx = IDX_MIN; earlier.
> 


This is correct understanding. The driver is expected to be ready to
answer any of the .ops calls during the registration. Specially if you
have cooling devices that may be bound to the thermal zone, then you
would have more than one calls.

With that said, I would suggest to move the line
+	priv->thresh_idx = IDX_MIN;

up, yes.

> 
> Also, can get_temp() be called concurrently?
> For example, from user-space reading /sys/class/thermal/thermal_zone0/temp
> and from the kernel thread polling the sensor?
> Is locking taken care of in higher levels?


Yes, .get_temp() is called under tz->lock, in both cases, sysfs and
thread polling.  Of course, if you have extra data used in other context
that thermal core cannot see, than you would need extra locking. But if
you are only using your data in .get_temp(), which seams the case for
you, I would not see a need for an extra lock.

> 
> 
> For the record, I test the driver with the following script.
> Is it good enough?
> 
> TEMP="/sys/class/thermal/thermal_zone0/temp"
> 
> echo RUN IDLE
> for ((I=0; I<30; ++I)); do cat $TEMP; sleep 2; done
> 
> echo RUN HEAVY LOAD
> cpuburn-a9 &
> cpuburn-a9 &
> for ((I=0; I<60; ++I)); do cat $TEMP; sleep 2; done
> 
> echo KILL HEAVY LOAD
> kill $(jobs -p)
> for ((I=0; I<30; ++I)); do cat $TEMP; sleep 2; done

Well, for temperature testing, that should be enough to see if driver is
returning temperature. 

As for the temperature value validation, you would
probably need to instrument your setup with either thermal couples or ir
sensing to make sure your driver is reporting correct values.

Apart from temperature testing, do you have any cooling device registered in your system?

Do you see cooling taking action?

> 
> 
> Regards.
>