Re: [PATCH v5 2/8] 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 e6c1079f034dff65c4f96319b3e627caa16315a4
Author: Patrice Chotard <patrice.chotard@foss.st.com>

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

This commit updates the STM32 syscon DT binding schema to allow an nvram
child node and bus addressing properties (#address-cells, #size-cells, and
ranges) for the st,stm32-tamp compatible.

>     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]
As reported by Sashiko on v4 [1], was a line starting with #address-cells
stripped by git commit in the middle of the second paragraph between "and
only allowed" and "into the DTSI", leaving the sentence incomplete?

> 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]
As reported by Sashiko on v4 [2], does unconditionally requiring
#address-cells, #size-cells, and ranges in allOf whenever compatible
contains st,stm32-tamp cause a validation regression for childless TAMP
nodes and non-MFD TAMP compatibles?

The ^nvram@[0-9a-f]+$ child node is optional, and properties.compatible in
Documentation/devicetree/bindings/arm/stm32/st,stm32-syscon.yaml permits both
the two-item non-MFD form (st,stm32-tamp, syscon) and the three-item MFD form
(st,stm32-tamp, syscon, simple-mfd):

  compatible:
    oneOf:
      - items:
          - enum:
              - st,stm32-tamp
              ...
          - const: syscon
      - items:
          - enum:
              - st,stm32-tamp
              ...
          - const: syscon
          - const: simple-mfd

Existing device trees such as arch/arm/boot/dts/st/stm32mp151.dtsi define a
childless tamp@5c00a000 node without #address-cells, #size-cells, or ranges:

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

Should #address-cells, #size-cells, and ranges only be allowed (or required
via dependencies when a child node is present) rather than unconditionally
required on all st,stm32-tamp nodes?

> +    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]
As reported by Sashiko on v4 [3], does widening the top-level #size-cells in
properties from const: 1 to enum: [1, 2] also relax validation for
st,stm32mp23-syscfg and st,stm32mp25-syscfg so that they now accept
#size-cells = <2>, even though their phy@2400 and phy@2800 child nodes only
use 1 size cell?

Also, since st,stm32-syscon.yaml now accumulates multiple per-compatible
allOf conditional blocks (including this negated multi-compatible if-not
block across st,stm32-tamp, st,stm32mp23-syscfg, and st,stm32mp25-syscfg) for
divergent child nodes and addressing rules, would it be cleaner to split
st,stm32-tamp into its own binding schema document rather than piling more
variant conditionals onto st,stm32-syscon.yaml?

[1] https://lore.kernel.org/r/20261104-upstream_nvram_reworked-v4-2-144024496a44@foss.st.com
[2] https://lore.kernel.org/r/20261104-upstream_nvram_reworked-v4-2-144024496a44@foss.st.com
[3] https://lore.kernel.org/r/20261104-upstream_nvram_reworked-v4-2-144024496a44@foss.st.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-upstream_nvram_reworked-v5-0-ddf7dbf1b298@foss.st.com?part=2