On 02/20/2013 07:28 AM, Stephen Warren wrote:

On 02/18/2013 04:30 AM, Wei Ni wrote:

quoted

Enable thermal driver in the dts file.
Set sensor as lm90 remote sensor, and set throttle data.

quoted

diff --git a/Documentation/devicetree/bindings/thermal/tegra3-thermal.txt b/Documentation/devicetree/bindings/thermal/tegra3-thermal.txt

The DT documentation should be either a separate patch before the one
which implements it in the driver, or part of the driver patch. It
shouldn't be part of the patch which instantiates the binding in the
board DT file.

Ok, I will do it.

quoted

+* Nvidia Tegra30 Thermal

NVIDIA is all capitals.

Got it.

It seems like a word is missing: Thermal Management? Control/Zone?

It's better to use "NVIDIA Tegra30 Thermal Zone".

quoted

+** Thermal node properties:
+
+- compatible : "nvidia,tegra30-thermal";
+- sensors: the sensor device node which we want to use in the thermal zone,
+  the arguments is the index of the sensor in sensor device node;

The arguments should be defined by the DT binding documentation for that
referenced device, not by the referencing device.

DT properties that are custom defined for this binding should include
the vendor prefix in their name. So, that'd be "nvidia,sensors". This
may apply less if a "sensors" becomes a more generic standard, but it
surely applies to all the other properties below.

So these properties should be introduced in the DT binding documentation
of the thermal framework.

quoted

+- passive-delay: passive delay;

What does this mean?

This is used for the thermal framework. As you said, this should be
introduced with the framework.

quoted

+- num-passive-trips : number of passive trip points, this is required, set
+  it 0 if none, if greater than 0, the following properties must be defined;
+- passive-trips : temperature of passive trip points;

Presumably "num-passive-trips" can be calculated simply by counting the
number of entries in "passive-trips"?

Oh, yes, I can remove it.

Please describe what passive and active trips are; someone who just
reads this binding document (perhaps in conjunction with some HW
documentation and/or other binding documentation) should be able to fill
in this binding without other knowledge.

Durgadoss's patches introduced these value in the framework
documentation, but he didn't touch the DT. So I will add it.

quoted

+- num-active-trips: number of active trip points, this is required, set
+  it 0 if none, if greater than 0, the following properties must be defined;
+- active-trips: temperature of active trip points;

Both comments above apply here too.

quoted

+- throt-tab-size: size of the throttle table, it's the max cooling state.
+- throt-tab: throttle table. the cooling state will be defined according to
+  this table.

Both comments above apply here too.

Also, what units are used for all these properties?

they use "int".
And according to our downstream dvfs driver, the throttle table use
"unsigned long".

Judging by the example below, this property is a list of tuples. The
meaning of each field in the tuple needs to be explained.

What happens when the CPU/SoC needs to be throttled? Must some clock or
voltage be lowered/limited? If so, you need properties that indicate
which clock/voltage/... needs to be acted upon.

This table which will be used by our DVFS driver, although it didn't be
upstreamed yet. The table set clock frequency limited value with
different cooling state. When the temperature touch the different
limited value, we will set difficult cooling state, find the limited
freq from this table and pass to the dvfs driver.
I think may be this table should be set for cooling device node. This
table is only for our tegra dvfs, so I think we can parse this table in
the tegra3_cooling.c, which will be the new driver for cooling device.

quoted

+Usually these properties are separated in board related dts files.

That's not really relevant.

Ok.