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

[Eduardo Valentin <edubezval@gmail.com>]

Hello Mason,

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

Ok, I see. Then, following the design you chose to use (of-thermal
based), determining what each sensor represents is typically solved by
describing them in device tree.

You did already a good job for the sensor part, but your patch (even v6)
do not include any thermal zone definition. So, what to do from here?

whenever possible, it is common that, together with your driver code,
you also do the device tree changes. Typically, for thermal zones,
people have been adding them on .dtsi files, so that they can be reused
on board files (dts).

You should be able to get examples from 
Documentation/devicetree/bindings/thermal/thermal.txt

or do a git grep thermal-zone arch/arm/boot/dts/, and you should be able
to see how people are doing this, consistently. But some chips can get
several zones though.


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

Ok.

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

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

No problem. 

> 
> Regards.
>