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

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

On 30/03/2016 02:05, Eduardo Valentin wrote:

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

I do have access to an "Objective" data sheet (Product status: Development,
as opposed to a Production data sheet). It's lacking important details,
unfortunately. (The data sheet comes from NXP, and mentions C045LP_TS.)

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

The SMP8758 has the 3. The next SoC has 5, in different HW blocks.
HW team likes to move them around, apparently.

> So, what to expect from this driver ?

One should expect this device driver to support the temperature sensor
used in Tango chips since the SMP8758, by using the "sigma,smp8758-thermal"
compatible string in DT nodes.

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

That I know for sure: there are no IRQ lines coming out of the
HW block.

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

The documentation states:

"The temp sensor uses a bandgap type of circuit to compare a voltage which
has a negative temperature coefficient with a voltage that is proportional
to absolute temperature. A resistor bank allows 40 different temperature
thresholds to be selected and the logic output 'out_temperature' will then
indicate whether the actual die temperature lies above or below the selected
threshold."

[NB: there are, in fact, 41 thresholds. The data sheet is inaccurate in a few places. ]

Characteristics

Symbol      Parameter             Min  Typ  Max  Unit

(Operating conditions)
Tjunc      Junction temperature   -40   25   125  °C
Vdd        Supply voltage         1.0  1.1  1.26   V

(Normal operating mode)
Idd         Supply current              50    60  μA
Vbandgapref Ref output voltage   0.72  0.8  0.88   V
∆outtemp    Absolute Temp               ±2   ±10  °C
            threshold error
T_res       Temp resolution        3    4.5    7  °C

I think the expected use-case was to program a "critical" threshold,
and have the 1-bit output signal "Oh no, we are melting down!".
But the HW guys thought "Hey, let's use this as a thermometer."

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

This is what the HW guys recommend:

1) Set the thresh to the desired value
2) Wait unknown amount of time
3) Read the 1-bit result

I will try to address most of your comments in a v6 in the next few days.

Thanks for your reviews, by the way.

Regards.