Re: [PATCH 1/3] dt-bindings: thermal: amlogic: add the S4 CPU sensor

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dt-bindings: thermal: amlogic: missing bus clock constraint for S4 sensor
--

commit b2d42a2f90c7882650efb090daff31a9ed1ad3e0
Author: Christian Stefan Kövér-Draxl <ckoever@outlook.de>

dt-bindings: thermal: amlogic: add the S4 CPU sensor

This commit adds the amlogic,s4-cpu-thermal compatible string to the Amlogic
thermal binding schema for the S4 CPU temperature sensor.

> diff --git a/Documentation/devicetree/bindings/thermal/amlogic,thermal.yaml b/Documentation/devicetree/bindings/thermal/amlogic,thermal.yaml
> index e72cb39ac635d..5d966ab4d820e 100644
> --- a/Documentation/devicetree/bindings/thermal/amlogic,thermal.yaml
> +++ b/Documentation/devicetree/bindings/thermal/amlogic,thermal.yaml
> @@ -23,6 +23,7 @@ properties:
>            - const: amlogic,g12a-thermal
>        - enum:
>            - amlogic,a1-cpu-thermal
> +          - amlogic,s4-cpu-thermal
>            - amlogic,t7-thermal
>  
>    reg:

[Severity: Medium]
Should amlogic,s4-cpu-thermal also require two clocks (bus and core) in the
allOf conditional block, or use amlogic,t7-thermal as a fallback compatible?

In Documentation/devicetree/bindings/thermal/amlogic,thermal.yaml, the second
allOf conditional checks only for amlogic,t7-thermal:

  - if:
      properties:
        compatible:
          contains:
            const: amlogic,t7-thermal
    then:
      properties:
        clocks:
          minItems: 2
      required:
        - clock-names
    else:
      properties:
        clocks:
          maxItems: 1
        clock-names: false

Because amlogic,s4-cpu-thermal is omitted from that condition, it falls into
the else branch, which restricts clocks to a single entry and forbids
clock-names.

In commit 8195b945794c ("dt-bindings: thermal: amlogic: describe bus clock
for t7-thermal"), the single-clock else branch was retained only for older
bindings to avoid breaking existing device trees, whereas T7 requires both
the peripheral bus gate clock and the shared core clock.

Looking at drivers/clk/meson/s4-peripherals.c, S4 has the same clock topology
as T7, with dedicated per-sensor bus gate clocks (s4_ts_ddr / CLKID_TS_DDR
and s4_ts_pll / CLKID_TS_PLL) in addition to the shared functional clock
(s4_ts_clk / CLKID_TS):

drivers/clk/meson/s4-peripherals.c:
static S4_PCLK(s4_ts_ddr,	CLKCTRL_SYS_CLK_EN0_REG1, 15, CLK_IGNORE_UNUSED);
static S4_PCLK(s4_ts_pll,	CLKCTRL_SYS_CLK_EN0_REG1, 16, CLK_IGNORE_UNUSED);

Without allowing both clocks in the binding, device tree nodes using
amlogic,s4-cpu-thermal cannot describe the sensor's peripheral bus gate clock
for amlogic_thermal_probe() to manage via devm_clk_bulk_get_all(), leaving
the sensor reliant on the temporary CLK_IGNORE_UNUSED flag on s4_ts_pll.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025192244.209256-1-christian.koever-draxl@student.uibk.ac.at?part=1

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic