Re: [PATCH v4 2/3] dt-bindings: arm: stm32: allow TAMP nvram child node for syscon

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] dt-bindings: arm: stm32: requiring ranges on tamp breaks childless nodes
- [Medium] dt-bindings: arm: stm32: unwieldy conditionals and #size-cells relaxation
- [Low] dt-bindings: arm: stm32: truncated sentence in commit message
--

commit 955fce49004502ff39ca0c0307ffc1b3b2ac4184
Author: Patrice Chotard <patrice.chotard@foss.st.com>

dt-bindings: arm: stm32: allow TAMP nvram child node for syscon

This commit updates Documentation/devicetree/bindings/arm/stm32/st,stm32-syscon.yaml
to allow addressing properties (#address-cells, #size-cells, and ranges) and
an nvram child node on st,stm32-tamp nodes.

>     dt-bindings: arm: stm32: allow TAMP nvram child node for syscon
>     
>     The STM32 TAMP node (compatible "st,stm32-tamp", "syscon", "simple-mfd")
>     exposes its backup registers through an nvram child node described by
>     nvmem/st,stm32-tamp-nvram.yaml. That requires the TAMP node to carry
>     "#address-cells", "#size-cells", "ranges" and an "nvram@..." child.
>     
>     st,stm32-syscon.yaml uses "additionalProperties: false" and only allowed
>     into the DTSI made "make dtbs_check" fail on every STM32MP TAMP node:
>     
>       tamp@5c00a000 (st,stm32-tamp): 'nvram@5c00a100', 'ranges' do not match
>       any of the regexes: '^pinctrl-[0-9]+$'
>     
>     Allow the addressing properties and the nvram child, gated on the
>     st,stm32-tamp compatible so the other syscon variants stay strict.
>     "#size-cells" may be 1 or 2: the STM32MP21 soc bus uses two size cells
>     (an empty "ranges" requires the node to match its parent) while the
>     other STM32MP SoCs use one.
>     
>     Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>

[Severity: Low]
This isn't a bug, but it looks like a line starting with #address-cells was
stripped as a comment by git commit in the second paragraph, leaving a
truncated sentence ("and only allowed into the DTSI made").

> diff --git a/Documentation/devicetree/bindings/arm/stm32/st,stm32-syscon.yaml b/Documentation/devicetree/bindings/arm/stm32/st,stm32-syscon.yaml
> index a559f8b03b842..51cb565e05edb 100644
> --- a/Documentation/devicetree/bindings/arm/stm32/st,stm32-syscon.yaml
> +++ b/Documentation/devicetree/bindings/arm/stm32/st,stm32-syscon.yaml
> @@ -43,13 +43,19 @@ properties:
>      const: 1
>  
>    "#size-cells":
> -    const: 1
> +    enum: [1, 2]
> +
> +  ranges: true
>  
>  patternProperties:
>    "^phy@2[48]00$":
>      type: object
>      $ref: /schemas/phy/st,stm32-usb2phy.yaml
>  
> +  "^nvram@[0-9a-f]+$":
> +    type: object
> +    $ref: /schemas/nvmem/st,stm32-tamp-nvram.yaml
> +
>  required:
>    - compatible
>    - reg
> @@ -84,11 +90,37 @@ allOf:
>          - phy@2800
>      else:
>        properties:
> -        '#address-cells': false
>          '#clock-cells': false
> -        '#size-cells': false
>          phy@2400: false
>          phy@2800: false
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: st,stm32-tamp
> +    then:
> +      required:
> +        - "#address-cells"
> +        - "#size-cells"
> +        - ranges

[Severity: Medium]
Does unconditionally requiring #address-cells, #size-cells, and ranges for
st,stm32-tamp cause a dtbs_check regression on existing device trees without
child nodes?

In arch/arm/boot/dts/st/stm32mp151.dtsi, the tamp@5c00a000 node has no child
nodes and does not specify #address-cells, #size-cells, or ranges:

		tamp: tamp@5c00a000 {
			compatible = "st,stm32-tamp", "syscon", "simple-mfd";
			reg = <0x5c00a000 0x400>;
		};

Additionally, properties.compatible in st,stm32-syscon.yaml allows the
two-item form without simple-mfd:

      - items:
          - enum:
              - st,stm32-tamp
              ...
          - const: syscon

where bus addressing properties would not apply when no child node is
present. Should #address-cells, #size-cells, and ranges remain optional (or
only be required when child nodes are present) so childless st,stm32-tamp
nodes stay valid?

> +    else:
> +      properties:
> +        ranges: false
> +      patternProperties:
> +        "^nvram@[0-9a-f]+$": false
> +  - if:
> +      not:
> +        properties:
> +          compatible:
> +            contains:
> +              enum:
> +                - st,stm32-tamp
> +                - st,stm32mp23-syscfg
> +                - st,stm32mp25-syscfg
> +    then:
> +      properties:
> +        '#address-cells': false
> +        '#size-cells': false

[Severity: Medium]
Because top-level #size-cells in properties was widened from const: 1 to
enum: [1, 2] without restricting 2 to st,stm32-tamp, doesn't this also allow
#size-cells = <2> on st,stm32mp23-syscfg and st,stm32mp25-syscfg?

Also, with st,stm32-tamp now needing its own ranges, nvram@... child pattern,
and #size-cells rules alongside the existing USB2 PHY conditionals for
st,stm32mp23-syscfg and st,stm32mp25-syscfg, would it be cleaner to split
st,stm32-tamp into a separate binding schema document rather than adding more
per-compatible if-then blocks to st,stm32-syscon.yaml?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-upstream_nvram_reworked-v4-0-144024496a44@foss.st.com?part=2