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

[Eduardo Valentin <edubezval@gmail.com>]

On Tue, Mar 29, 2016 at 08:48:43PM +0200, Mason wrote:
> 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.

Well, I hope for the same, but the driver needs to be clarified and
properly documented, don't you think?

And now that you are here, and you have written this nice driver, it
would be good to make sure whoever comes to patch it later have enough
groundings to do the right thing.

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

How did you come up with this code then? How do I know it is actually
working? Can somebody else verify this and reply with a tested-by?

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

No failures in fact, that was just a pattern.

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

This is confusing now. You mention in your commit message that the SoC
has three sensor instances, and they are in the CPU, video decoder,
and PCIe controller. Now, you are mentioning location does not matter.

So, what to expect from this driver ?

> 
> > 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;
> 

That, as mentioned, is a coding suggestion, based on what I have been
seeing as pattern. 


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

Well, we are using timers, you are right, but we are still using the CPU
to check the hardware status. Is there a better way to do this? If it is
threshold based, does the hardware produces IRQs?

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

Does this hardware support reading temperature in a different way? I
must say this is an awkward way of doing this, even worst blindly without
documentation.

> 
> > 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?

I am just attempting to understand this code and if it makes sense at
all. Of course, without hardware doc all this is just guessing.

> 
> 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?

yes

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