Re: [PATCH 3/4] dt-bindings: mmc: add brcmstb share register and hwlocks reference

[Krzysztof Kozlowski <krzk@kernel.org>]

On 11/07/2025 17:42, Kamal Dasu wrote:
> From: Kamal Dasu <kdasu@broadcom.com>
> 
> Adding optional controller share registers and hwspinlock reference fields
> to be used by sdhci-brcmstb driver.
> 
> Signed-off-by: Kamal Dasu <kamal.dasu@broadcom.com>
> ---
>  .../bindings/mmc/brcm,sdhci-brcmstb.yaml      | 29 +++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml b/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml
> index eee6be7a7867..fe9be7a7eca5 100644
> --- a/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml
> +++ b/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml
> @@ -27,15 +27,20 @@ properties:
>            - const: brcm,sdhci-brcmstb
>  
>    reg:
> -    maxItems: 2
> +    minItems: 2
> +    maxItems: 4
>  
>    reg-names:
> +    minItems: 2
>      items:
>        - const: host
>        - const: cfg
> +      - const: share       # Optional reg
> +      - const: flshr_ipis0 # Optional reg

Drop comments. Schema says these are optional, so no need to point obvious.

>  
>    interrupts:
> -    maxItems: 1
> +    minItems: 1
> +    maxItems: 2

You need to list items with minItem: 1.

Why all devices now got new reg and interrupts? What changed in EXISTING
hardware? All this has to be explained in the commit msg. If not all
devices got it, then you miss constraints.

>  
>    clocks:
>      minItems: 1
> @@ -60,6 +65,9 @@ properties:
>      type: boolean
>      description: Specifies that controller should use auto CMD12
>  
> +  hwlocks:
> +    maxItems: 1
> +
>  allOf:
>    - $ref: mmc-controller.yaml#
>    - if:
> @@ -115,3 +123,20 @@ examples:
>        clocks = <&scmi_clk 245>;
>        clock-names = "sw_sdio";
>      };
> +  - |
> +    mmc@84b1000 {
> +      mmc-ddr-1_8v;
> +      mmc-hs200-1_8v;
> +      mmc-hs400-1_8v;
> +      no-sd;
> +      no-sdio;
> +      non-removable;
> +      bus-width = <0x8>;
> +      compatible = "brcm,bcm74165b0-sdhci", "brcm,sdhci-brcmstb";
> +      reg = <0x84b1000 0x260>, <0x84b1300 0x200>,  <0x84b1600 0x10>, <0x84a5404 0x4>;
> +      reg-names = "host", "cfg", "share", "flshr_ipis0";
> +      hwlocks = <&hw_lock 0x0>;
> +      interrupts = <0x1 0x0 0x1f 0x4 0x1b 0x11>;

Undecipherable.

> +      clocks = <&scmi_clk 245>;
> +      clock-names = "sw_sdio";

Totally random order of properties. Please align it closely with DTS
coding style.

> +    };


Best regards,
Krzysztof