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

[Mason <slash.tmp@free.fr>]

On 29/03/2016 04:00, Eduardo Valentin wrote:

> Again, only minor comments, as follows.

I do hope we can get this driver sorted out in time for the 4.7
merge window.

>> +The SMP8758 SoC includes 3 instances of this temperature sensor
>> +(in the CPU, video decoder, and PCIe controller).
> 
> Would you be able to add a link to sensor documentation?

I couldn't find any documentation online, even on Chinese sites.

>> +	cpu_temp: thermal@920100 {
>> +		compatible = "sigma,smp8758-thermal";
>> +		#thermal-sensor-cells = <0>;
>> +		reg = <0x920100 12>;
> 
> I believe you have to put #thermal-sensor-cells as last entry.

I've been using the node as-is in my DT, which works as expected.
What kinds of failure did you have in mind?

>> +	  Enable the Tango temperature sensor driver, which provides support
>> +	  for the sensors used since the SMP8758.
> 
> Please add a better description, e.g. which sensors are supported,
> locations, accuracy, and more meaningful info an engineer could read out
> of the help section.

I will try to give a few more details, but AFAICS the majority
of entries for other SoCs are as short as mine...

What do you mean by "which sensors"?

Locations isn't really relevant here because different SoCs
will have them in different places.

> No license/author/contact section here?

License is given by MODULE_LICENSE("GPL");
author and contact are given by git log or git blame.

>> +	writel(thresh_idx << 8 | CMD_READ, base + TEMPSI_CMD);
>> +	usleep_range(100, 200);
>
> nip: blank line here.

Did you mean "nit"? :-)

>> +	return readl(base + TEMPSI_RES);

Hmm, I don't see this rule in the CodingStyle document.

There are even examples to the contrary:

		kfree(foo->bar);
		kfree(foo);
		return ret;

>> +	if (temp_above_thresh(base, idx)) {
>> +		/* Upward linear search, increment thresh */
>> +		while (idx < IDX_MAX && temp_above_thresh(base, ++idx))
>> +			cpu_relax();
>> +		idx = idx - 1; /* always return lower bound */
>> +	} else {
>> +		/* Downward linear search, decrement thresh */
>> +		while (idx > IDX_MIN && !temp_above_thresh(base, --idx))
>> +			cpu_relax();
>> +	}
> 
> Did I understand this right? We can only read if the temperature is
> above an index or not, right?

Right, the hardware's output is a single bit:
"Is the temperature above the programmed threshold?" yes/no.

> and we spin for 100-200us after every 
> attempt to check if temperature is above and index.

Nit: we don't spin, we sleep.

100-200 µs is a bit conservative, the hardware doesn't need
that much. But I wanted to give Linux the opportunity to
group multiple checks together, and even switch to a different
task if necessary. I assumed that it doesn't make sense to
context switch for only tens of microseconds?

> So, worst case,
> if we start from IDX_MIN, and temp is at IDX_MAX, we spin for
> 40 * (100 .. 200) us (with cpu_relax in between).

On my system, cpu_relax is just a compiler barrier, so barely
a blip. And the worst case doesn't occur. But I will change
IDX_MIN to 10, because I don't care about sub-zero temperatures.

> Does the chip support different temperature read strategy?

I'm not sure what you mean.

> How does this perform in average case?

Typical idle temps are index 18-20 (43-52 °C)
When I load the CPU with cpuburn, the temp climbs to
index 22-24 (61-70 °C).
But obviously, these numbers depend on the thermal solution.

> How about a local search within +-2 indexes of thresh_idx?

What problem would that solve?

>> +	priv->zone = thermal_zone_of_sensor_register(dev, 0, priv, &ops);
> 
> Would it make sense to use the devm_thermal_zone_of_sensor_register
> version instead?

Does that automatically call thermal_zone_of_sensor_unregister
in the remove() method?

I prefer having the symmetry of register/unregister, but
I guess it's your call as maintainer.

Regards.