Re: [PATCH 1/6] dt-bindings: thermal: thermal-zones: add hardware-binning trip properties

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] dt-bindings: thermal: backward compatibility broken by oneOf temperature exclusivity
- [Medium] dt-bindings: thermal: ambiguous temperature selection for overlapping hw masks
- [Low] dt-bindings: thermal: missing min/max constraints on temperature-bin
--

commit b87ec3c80f04fa5778f69e6acbc48a25d9b23994
Author: Amit Kucheria <amit.kucheria@oss.qualcomm.com>

dt-bindings: thermal: thermal-zones: add hardware-binning trip properties

This commit adds new optional device tree properties, `temperature-bin` and
`thermal-hw-bin`, to allow specifying different thermal trip temperatures
based on hardware revisions encoded in fuses. It aims to support SoC SKUs
with different thermal characteristics.

> diff --git a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> index 999ad40a20d5c..cbe72c2ef87c0 100644
> --- a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> +++ b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> @@ -152,6 +152,51 @@ patternProperties:
[ ... ]
> +            temperature-bin:
> +              $ref: /schemas/types.yaml#/definitions/int32-array
> +              description: |
> +                An array of integers expressing trip temperatures in

[Severity: Low]
The original `temperature` property enforces logical bounding constraints
(such as minimum and maximum). This new `temperature-bin` array lacks the
corresponding `items` minimum and maximum constraints, which might allow
invalid or physically impossible temperature values to pass validation.

Should we add `items` constraints here to enforce valid temperature ranges?

[ ... ]
> +            thermal-hw-bin:
> +              $ref: /schemas/types.yaml#/definitions/uint32-matrix
> +              description: |
> +                Optional bitmask(s) describing the hardware revisions for
[ ... ]
> +                When used together with 'temperature-bin', the number of
> +                sub-groups must equal the number of entries in
> +                'temperature-bin'.

[Severity: Medium]
The binding specifies how a sub-group matches, but it doesn't specify a
precedence rule if multiple bitmask sub-groups evaluate to true at the same
time.

If a hardware revision sets multiple fuse bits and matches overlapping
masks, which entry in `temperature-bin` is selected?

Could we add a clear precedence rule here, such as the first matching
sub-group taking precedence, to avoid inconsistent thermal configurations?

[ ... ]
> @@ -179,8 +224,14 @@ patternProperties:
[ ... ]
> +          oneOf:
> +            - required:
> +                - temperature
> +            - required:
> +                - temperature-bin

[Severity: High]
This `oneOf` requirement strictly forbids the presence of both `temperature`
and `temperature-bin`.

If a device tree is updated to use `temperature-bin`, the author is forced
to remove `temperature`. When an older kernel (which only understands
`temperature`) boots with this updated device tree, it will fail to parse
the thermal trip point completely. Missing trip points result in a complete
loss of thermal protection, which can lead to hardware damage from
overheating.

Should we allow `temperature` as a fallback property for older OSes by
using `anyOf` instead of `oneOf`?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002-thermal_bin-v1-0-5a29be7e8ec5@oss.qualcomm.com?part=1