Re: [PATCH V1 06/10] of: Add bindings of hw-trips for soctherm

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2527 bytes --]

Cc'ing devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, please make sure to always Cc that
list on device tree binding patches.

On Wed, Jan 13, 2016 at 04:00:22PM +0800, Wei Ni wrote:
> Add hw-trips sub-node for soctherm, which is
> used to describe the hardware trip points for
> each soctherm sensors.

Perhaps you should mention Tegra in the subject and the commit message?

Also please use all of the 72 columns that are at your disposal when
writing commit messages.

> Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  .../devicetree/bindings/thermal/tegra-soctherm.txt | 29 ++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/tegra-soctherm.txt b/Documentation/devicetree/bindings/thermal/tegra-soctherm.txt
> index 6b68cd150405..77aef0b4968b 100644
> --- a/Documentation/devicetree/bindings/thermal/tegra-soctherm.txt
> +++ b/Documentation/devicetree/bindings/thermal/tegra-soctherm.txt
> @@ -26,6 +26,18 @@ Required properties :
>      of this property. See <dt-bindings/thermal/tegra124-soctherm.h> for a
>      list of valid values when referring to thermal sensors.
>  
> +Optional properties:
> +- hw-trips : A sub-node which is a container of hardware trip points
> +    for each sensors.

Perhaps call the node "trip-points"? "hw-" seems kind of redundant since
DT is a hardware description, and "trips" isn't a very good abbreviation
for "trip-points".

> +  * sensors: Sub-nodes which are used to describe the HW trip points info,
> +        must be named as "cpu", "gpu", "mem", "pll".
> +      Properties:
> +      - therm-temp : Shutdown temperature in millicelsius, once the
> +        temperature of this sensor is higher than the therm-temp, the system
> +        will be reset or shutdown.

Why not call the property "shutdown-temperature" if that's what it is.

> +      - throt-temp : HW throttle temperature in millicelsius, once the
> +        temperature of this sensor is higher than the throt-temp, the HW
> +        throttle will be triggered in soctherm. (Not implement yet)

"throttle-temperature" would be a better name in my opinion. More typing
perhaps but less chance of getting the abbreviations wrong. Also there's
no need for the "(Not implement yet)" because the binding isn't about an
implementation, it's about the hardware description. If you have no code
that uses this, a safer option might be to just drop it for now.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]