[PATCH v4 0/3] drm/vc4: Fixup DT and DT binding issues from recent patchset

[PATCH v4 0/3] drm/vc4: Fixup DT and DT binding issues from recent patchset

[Dave Stevenson]

I missed the DT errors from the recent patchset[1] (DT patches
in linux-next via Florian, DRM bindings patches on dri-misc-next)
as Rob's bot report got spam filtered, so this is a fixup set.

Largely it was changes to number of interrupts or clocks in the
bindings, so those are now covered.

I've fixed up the missing "interrupt-controller" flags for 2711
and 2712 whilst here.

I can't get my head around what is meant to happen with ranges:
"soc@107c000000: firmware: 'ranges' is a required property"
The meaning seems obvious.

However if I add it then I get:
"firmware: '#address-cells', '#size-cells', 'dma-ranges', 'ranges' do
not match any of the regexes: 'pinctrl-[0-9]+'
from schema $id: http://devicetree.org/schemas/arm/bcm/raspberrypi,bcm2835-firmware.yaml#

There's obviously some other flag I need to set in the bindings,
but I can't work it out. We have similar errors for all the Pi
platforms for one or more nodes.
Please advise and I'll happily fix them all.

Thanks
  Dave

[1] https://lore.kernel.org/linux-arm-kernel/20241025-drm-vc4-2712-support-v2-0-35efa83c8fc0@raspberrypi.com/

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
Changes in v4:
- 1/7 Removed the duplication
- 2/7 Added "minItems: 2" and added to the commit text that the clock
  and interrupt names are required for bcm2712
- 3/7 Already applied by Bartosz, and 5-7/7 applied by Florian
- 4/7 added Krzysztof Acked-by
- Link to v3: https://lore.kernel.org/r/20241212-dt-bcm2712-fixes-v3-0-44a7f3390331@raspberrypi.com

Thanks Krzysztof for your comments. Terms such as "widest constraints"
may be obvious to those regularly doing dtschemas, but not to me who
does them once in a blue-moon.

Changes in v3:
- Fixed up indentation on 1/7. (I fixed it once, but obviously reworked
  things and lost it).
- Link to v2: https://lore.kernel.org/r/20241212-dt-bcm2712-fixes-v2-0-35986e04d0f4@raspberrypi.com

Thanks to Stefan and Krzysztof for their reviews.
Hopefully I've addressed all points raised in the correct manner.

Changes in v2:
- Commits have now be merged from drm-misc-next to linux-next, so all
  commit hashes are valid on linux-next.
- 1/7 Removed references to "previous commit". Fixed up indentation.
  Added maxItems
- 2/7 Defined widest constraints
- 3/7 Added maxItems and removed reference to Linux
- 4/7 Described the errors. Split into two for fix of node name vs addr
  being wrong.
- Added new patch removing "required" for interrupt-controller and
  interrupt-cells for bcm2836-l1-intc
- 5/7 (now 7/7) Removed the intc node for 2712 - it's irrelevant on 64bit systems
- 6/7 dropped as updating the binding is the correct answer
- 7/7 dropped. simple-bus claims ranges is required, but adding it
  creates other errors. I'm unclear as to the right solution.

- Link to v1: https://lore.kernel.org/r/20241202-dt-bcm2712-fixes-v1-0-fac67cc2f98a@raspberrypi.com

---
Dave Stevenson (3):
      dt-bindings: display: bcm2711-hdmi: Add interrupt details for BCM2712
      dt-bindings: display: Fix BCM2835 HVS bindings for BCM2712
      dt-bindings: interrupt-controller: brcm,bcm2836-l1-intc: Drop interrupt-controller requirement

 .../bindings/display/brcm,bcm2711-hdmi.yaml        | 81 +++++++++++++++++----
 .../bindings/display/brcm,bcm2835-hvs.yaml         | 84 ++++++++++++++++++----
 .../interrupt-controller/brcm,bcm2836-l1-intc.yaml |  2 -
 3 files changed, 137 insertions(+), 30 deletions(-)
---
base-commit: 29978a81e62128d2cee56f066ec92de584f4ab5f
change-id: 20241128-dt-bcm2712-fixes-afb0e8a0a476

Best regards,
-- 
Dave Stevenson <dave.stevenson@raspberrypi.com>

[PATCH v4 1/3] dt-bindings: display: bcm2711-hdmi: Add interrupt details for BCM2712

[Dave Stevenson]

Commit 62948c62abca ("dt-bindings: display: Add BCM2712 HDMI bindings")
added the compatible strings for BCM2712, but missed out that the
number of interrupts changed.

Update the schema to include the interrupt requirements.

Fixes: 62948c62abca ("dt-bindings: display: Add BCM2712 HDMI bindings")
Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 .../bindings/display/brcm,bcm2711-hdmi.yaml        | 81 ++++++++++++++++++----
 1 file changed, 67 insertions(+), 14 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/brcm,bcm2711-hdmi.yaml b/Documentation/devicetree/bindings/display/brcm,bcm2711-hdmi.yaml
index 6d11f5955b51..83c058728ef1 100644
--- a/Documentation/devicetree/bindings/display/brcm,bcm2711-hdmi.yaml
+++ b/Documentation/devicetree/bindings/display/brcm,bcm2711-hdmi.yaml
@@ -56,22 +56,12 @@ properties:
       - const: cec
 
   interrupts:
-    items:
-      - description: CEC TX interrupt
-      - description: CEC RX interrupt
-      - description: CEC stuck at low interrupt
-      - description: Wake-up interrupt
-      - description: Hotplug connected interrupt
-      - description: Hotplug removed interrupt
+    minItems: 5
+    maxItems: 6
 
   interrupt-names:
-    items:
-      - const: cec-tx
-      - const: cec-rx
-      - const: cec-low
-      - const: wakeup
-      - const: hpd-connected
-      - const: hpd-removed
+    minItems: 5
+    maxItems: 6
 
   ddc:
     $ref: /schemas/types.yaml#/definitions/phandle
@@ -112,6 +102,66 @@ required:
 
 additionalProperties: false
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - brcm,bcm2711-hdmi0
+              - brcm,bcm2711-hdmi1
+    then:
+      properties:
+        interrupts:
+          items:
+            - description: CEC TX interrupt
+            - description: CEC RX interrupt
+            - description: CEC stuck at low interrupt
+            - description: Wake-up interrupt
+            - description: Hotplug connected interrupt
+            - description: Hotplug removed interrupt
+        interrupt-names:
+          items:
+            - const: cec-tx
+            - const: cec-rx
+            - const: cec-low
+            - const: wakeup
+            - const: hpd-connected
+            - const: hpd-removed
+
+
+      required:
+        - interrupts
+        - interrupt-names
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - brcm,bcm2712-hdmi0
+              - brcm,bcm2712-hdmi1
+    then:
+      properties:
+        interrupts:
+          items:
+            - description: CEC TX interrupt
+            - description: CEC RX interrupt
+            - description: CEC stuck at low interrupt
+            - description: Hotplug connected interrupt
+            - description: Hotplug removed interrupt
+        interrupts-names:
+          items:
+            - const: cec-tx
+            - const: cec-rx
+            - const: cec-low
+            - const: hpd-connected
+            - const: hpd-removed
+
+      required:
+        - interrupts
+        - interrupt-names
+
 examples:
   - |
     hdmi0: hdmi@7ef00700 {
@@ -136,6 +186,9 @@ examples:
                     "hd";
         clocks = <&firmware_clocks 13>, <&firmware_clocks 14>, <&dvp 1>, <&clk_27MHz>;
         clock-names = "hdmi", "bvb", "audio", "cec";
+        interrupts = <0>, <1>, <2>, <3>, <4>, <5>;
+        interrupt-names = "cec-tx", "cec-rx", "cec-low", "wakeup",
+                "hpd-connected", "hpd-removed";
         resets = <&dvp 0>;
         ddc = <&ddc0>;
     };

-- 
2.34.1

[PATCH v4 2/3] dt-bindings: display: Fix BCM2835 HVS bindings for BCM2712

[Dave Stevenson]

Commit 6cfcbe548a3a ("dt-bindings: display: Add BCM2712 HVS bindings")
added the compatible string for BCM2712, but missed out that
the number of interrupts and clocks changed too, and both need to be
named.

Update to validate clock, interrupts, and their names for the variants.

Fixes: 6cfcbe548a3a ("dt-bindings: display: Add BCM2712 HVS bindings")
Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 .../bindings/display/brcm,bcm2835-hvs.yaml         | 84 ++++++++++++++++++----
 1 file changed, 70 insertions(+), 14 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/brcm,bcm2835-hvs.yaml b/Documentation/devicetree/bindings/display/brcm,bcm2835-hvs.yaml
index f91c9dce2a44..fd25ee5ce301 100644
--- a/Documentation/devicetree/bindings/display/brcm,bcm2835-hvs.yaml
+++ b/Documentation/devicetree/bindings/display/brcm,bcm2835-hvs.yaml
@@ -20,11 +20,20 @@ properties:
     maxItems: 1
 
   interrupts:
-    maxItems: 1
+    minItems: 1
+    maxItems: 3
+
+  interrupt-names:
+    minItems: 1
+    maxItems: 3
 
   clocks:
-    maxItems: 1
-    description: Core Clock
+    minItems: 1
+    maxItems: 2
+
+  clock-names:
+    minItems: 1
+    maxItems: 2
 
 required:
   - compatible
@@ -33,17 +42,64 @@ required:
 
 additionalProperties: false
 
-if:
-  properties:
-    compatible:
-      contains:
-        enum:
-          - brcm,bcm2711-hvs
-          - brcm,bcm2712-hvs
-
-then:
-  required:
-    - clocks
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: brcm,bcm2711-hvs
+
+    then:
+      properties:
+        clocks:
+          items:
+            - description: Core Clock
+        interrupts:
+          maxItems: 1
+
+      required:
+        - clocks
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: brcm,bcm2712-hvs
+
+    then:
+      properties:
+        clocks:
+          minItems: 2
+          maxItems: 2
+        clock-names:
+          items:
+            - const: core
+            - const: disp
+        interrupts:
+          items:
+            - description: Channel 0 End of frame
+            - description: Channel 1 End of frame
+            - description: Channel 2 End of frame
+        interrupt-names:
+          items:
+            - const: ch0-eof
+            - const: ch1-eof
+            - const: ch2-eof
+      required:
+        - clocks
+        - clock-names
+        - interrupt-names
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: brcm,bcm2835-hvs
+
+    then:
+      properties:
+        interrupts:
+          maxItems: 1
 
 examples:
   - |

-- 
2.34.1

[PATCH v4 3/3] dt-bindings: interrupt-controller: brcm,bcm2836-l1-intc: Drop interrupt-controller requirement

[Dave Stevenson]

Since commit 88bbe85dcd37 ("irqchip: bcm2836: Move SMP startup code to
arch/arm (v2)") the bcm2836-l1-intc block on bcm2711 is only used as a
base address for the smp_boot_secondary hook on 32 bit kernels. It is
not used as an interrupt controller.

Drop the binding requirement for interrupt-controller and interrupt-cells
to satisfy validation on this platform.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../devicetree/bindings/interrupt-controller/brcm,bcm2836-l1-intc.yaml  | 2 --
 1 file changed, 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2836-l1-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2836-l1-intc.yaml
index 5fda626c80ce..2ff390c1705b 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2836-l1-intc.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2836-l1-intc.yaml
@@ -34,8 +34,6 @@ properties:
 required:
   - compatible
   - reg
-  - interrupt-controller
-  - '#interrupt-cells'
 
 additionalProperties: false
 

-- 
2.34.1

Re: [PATCH v4 3/3] dt-bindings: interrupt-controller: brcm,bcm2836-l1-intc: Drop interrupt-controller requirement

[Florian Fainelli]

On 12/18/24 06:48, Dave Stevenson wrote:
> Since commit 88bbe85dcd37 ("irqchip: bcm2836: Move SMP startup code to
> arch/arm (v2)") the bcm2836-l1-intc block on bcm2711 is only used as a
> base address for the smp_boot_secondary hook on 32 bit kernels. It is
> not used as an interrupt controller.
> 
> Drop the binding requirement for interrupt-controller and interrupt-cells
> to satisfy validation on this platform.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH v4 1/3] dt-bindings: display: bcm2711-hdmi: Add interrupt details for BCM2712

[Krzysztof Kozlowski]

On Wed, Dec 18, 2024 at 02:48:32PM +0000, Dave Stevenson wrote:
> Commit 62948c62abca ("dt-bindings: display: Add BCM2712 HDMI bindings")
> added the compatible strings for BCM2712, but missed out that the
> number of interrupts changed.
> 
> Update the schema to include the interrupt requirements.
> 
> Fixes: 62948c62abca ("dt-bindings: display: Add BCM2712 HDMI bindings")
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  .../bindings/display/brcm,bcm2711-hdmi.yaml        | 81 ++++++++++++++++++----
>  1 file changed, 67 insertions(+), 14 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/brcm,bcm2711-hdmi.yaml b/Documentation/devicetree/bindings/display/brcm,bcm2711-hdmi.yaml
> index 6d11f5955b51..83c058728ef1 100644
> --- a/Documentation/devicetree/bindings/display/brcm,bcm2711-hdmi.yaml
> +++ b/Documentation/devicetree/bindings/display/brcm,bcm2711-hdmi.yaml
> @@ -56,22 +56,12 @@ properties:
>        - const: cec
>  
>    interrupts:
> -    items:
> -      - description: CEC TX interrupt
> -      - description: CEC RX interrupt
> -      - description: CEC stuck at low interrupt
> -      - description: Wake-up interrupt
> -      - description: Hotplug connected interrupt
> -      - description: Hotplug removed interrupt
> +    minItems: 5
> +    maxItems: 6
>  
>    interrupt-names:
> -    items:
> -      - const: cec-tx
> -      - const: cec-rx
> -      - const: cec-low
> -      - const: wakeup
> -      - const: hpd-connected
> -      - const: hpd-removed
> +    minItems: 5
> +    maxItems: 6
>  
>    ddc:
>      $ref: /schemas/types.yaml#/definitions/phandle
> @@ -112,6 +102,66 @@ required:
>  
>  additionalProperties: false
>  
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - brcm,bcm2711-hdmi0
> +              - brcm,bcm2711-hdmi1
> +    then:
> +      properties:
> +        interrupts:
> +          items:
> +            - description: CEC TX interrupt
> +            - description: CEC RX interrupt
> +            - description: CEC stuck at low interrupt
> +            - description: Wake-up interrupt
> +            - description: Hotplug connected interrupt
> +            - description: Hotplug removed interrupt
> +        interrupt-names:
> +          items:
> +            - const: cec-tx
> +            - const: cec-rx
> +            - const: cec-low
> +            - const: wakeup
> +            - const: hpd-connected
> +            - const: hpd-removed
> +
> +

Only one blank line

> +      required:
> +        - interrupts
> +        - interrupt-names

But anyway this is unusual. Why this was added? Nothing in commit msg
explains this.

> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - brcm,bcm2712-hdmi0
> +              - brcm,bcm2712-hdmi1
> +    then:
> +      properties:
> +        interrupts:
> +          items:
> +            - description: CEC TX interrupt
> +            - description: CEC RX interrupt
> +            - description: CEC stuck at low interrupt
> +            - description: Hotplug connected interrupt
> +            - description: Hotplug removed interrupt
> +        interrupts-names:
> +          items:
> +            - const: cec-tx
> +            - const: cec-rx
> +            - const: cec-low
> +            - const: hpd-connected
> +            - const: hpd-removed
> +
> +      required:
> +        - interrupts
> +        - interrupt-names

Same question.

> +
>  examples:
>    - |
>      hdmi0: hdmi@7ef00700 {
> @@ -136,6 +186,9 @@ examples:
>                      "hd";
>          clocks = <&firmware_clocks 13>, <&firmware_clocks 14>, <&dvp 1>, <&clk_27MHz>;
>          clock-names = "hdmi", "bvb", "audio", "cec";
> +        interrupts = <0>, <1>, <2>, <3>, <4>, <5>;
> +        interrupt-names = "cec-tx", "cec-rx", "cec-low", "wakeup",
> +                "hpd-connected", "hpd-removed";

Fix alignment with opening " from earlier line (see DTS coding style).

Best regards,
Krzysztof

Re: [PATCH v4 2/3] dt-bindings: display: Fix BCM2835 HVS bindings for BCM2712

[Krzysztof Kozlowski]

On Wed, Dec 18, 2024 at 02:48:33PM +0000, Dave Stevenson wrote:
> Commit 6cfcbe548a3a ("dt-bindings: display: Add BCM2712 HVS bindings")
> added the compatible string for BCM2712, but missed out that
> the number of interrupts and clocks changed too, and both need to be
> named.
> 
> Update to validate clock, interrupts, and their names for the variants.
> 
> Fixes: 6cfcbe548a3a ("dt-bindings: display: Add BCM2712 HVS bindings")
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  .../bindings/display/brcm,bcm2835-hvs.yaml         | 84 ++++++++++++++++++----
>  1 file changed, 70 insertions(+), 14 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/brcm,bcm2835-hvs.yaml b/Documentation/devicetree/bindings/display/brcm,bcm2835-hvs.yaml
> index f91c9dce2a44..fd25ee5ce301 100644
> --- a/Documentation/devicetree/bindings/display/brcm,bcm2835-hvs.yaml
> +++ b/Documentation/devicetree/bindings/display/brcm,bcm2835-hvs.yaml
> @@ -20,11 +20,20 @@ properties:
>      maxItems: 1
>  
>    interrupts:
> -    maxItems: 1
> +    minItems: 1
> +    maxItems: 3
> +
> +  interrupt-names:
> +    minItems: 1
> +    maxItems: 3
>  
>    clocks:
> -    maxItems: 1
> -    description: Core Clock
> +    minItems: 1
> +    maxItems: 2
> +
> +  clock-names:
> +    minItems: 1
> +    maxItems: 2
>  
>  required:
>    - compatible
> @@ -33,17 +42,64 @@ required:
>  
>  additionalProperties: false
>  
> -if:
> -  properties:
> -    compatible:
> -      contains:
> -        enum:
> -          - brcm,bcm2711-hvs
> -          - brcm,bcm2712-hvs
> -
> -then:
> -  required:
> -    - clocks
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: brcm,bcm2711-hvs
> +
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: Core Clock
> +        interrupts:
> +          maxItems: 1


clock-names and interrupt-names: false, unless driver needs them but all
this should be explained in the commit msg because it would be a change
to the binding.

> +
> +      required:
> +        - clocks
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: brcm,bcm2712-hvs
> +
> +    then:
> +      properties:
> +        clocks:
> +          minItems: 2
> +          maxItems: 2
> +        clock-names:
> +          items:
> +            - const: core
> +            - const: disp
> +        interrupts:
> +          items:
> +            - description: Channel 0 End of frame
> +            - description: Channel 1 End of frame
> +            - description: Channel 2 End of frame
> +        interrupt-names:
> +          items:
> +            - const: ch0-eof
> +            - const: ch1-eof
> +            - const: ch2-eof
> +      required:
> +        - clocks
> +        - clock-names
> +        - interrupt-names

My previous comment still stands. Reply to people's feedback instead of
ignoring it.

Best regards,
Krzysztof

Re: [PATCH v4 2/3] dt-bindings: display: Fix BCM2835 HVS bindings for BCM2712

[Dave Stevenson]

Hi Krzysztof

On Thu, 19 Dec 2024 at 08:42, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Wed, Dec 18, 2024 at 02:48:33PM +0000, Dave Stevenson wrote:
> > Commit 6cfcbe548a3a ("dt-bindings: display: Add BCM2712 HVS bindings")
> > added the compatible string for BCM2712, but missed out that
> > the number of interrupts and clocks changed too, and both need to be
> > named.
> >
> > Update to validate clock, interrupts, and their names for the variants.
> >
> > Fixes: 6cfcbe548a3a ("dt-bindings: display: Add BCM2712 HVS bindings")
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  .../bindings/display/brcm,bcm2835-hvs.yaml         | 84 ++++++++++++++++++----
> >  1 file changed, 70 insertions(+), 14 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/display/brcm,bcm2835-hvs.yaml b/Documentation/devicetree/bindings/display/brcm,bcm2835-hvs.yaml
> > index f91c9dce2a44..fd25ee5ce301 100644
> > --- a/Documentation/devicetree/bindings/display/brcm,bcm2835-hvs.yaml
> > +++ b/Documentation/devicetree/bindings/display/brcm,bcm2835-hvs.yaml
> > @@ -20,11 +20,20 @@ properties:
> >      maxItems: 1
> >
> >    interrupts:
> > -    maxItems: 1
> > +    minItems: 1
> > +    maxItems: 3
> > +
> > +  interrupt-names:
> > +    minItems: 1
> > +    maxItems: 3
> >
> >    clocks:
> > -    maxItems: 1
> > -    description: Core Clock
> > +    minItems: 1
> > +    maxItems: 2
> > +
> > +  clock-names:
> > +    minItems: 1
> > +    maxItems: 2
> >
> >  required:
> >    - compatible
> > @@ -33,17 +42,64 @@ required:
> >
> >  additionalProperties: false
> >
> > -if:
> > -  properties:
> > -    compatible:
> > -      contains:
> > -        enum:
> > -          - brcm,bcm2711-hvs
> > -          - brcm,bcm2712-hvs
> > -
> > -then:
> > -  required:
> > -    - clocks
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: brcm,bcm2711-hvs
> > +
> > +    then:
> > +      properties:
> > +        clocks:
> > +          items:
> > +            - description: Core Clock
> > +        interrupts:
> > +          maxItems: 1
>
>
> clock-names and interrupt-names: false, unless driver needs them but all
> this should be explained in the commit msg because it would be a change
> to the binding.

False it is then.

Is there actually a full guide to binding requirements?
https://www.kernel.org/doc/html/latest/devicetree/bindings/writing-schema.html
is the closest I've found, but it doesn't obviously cover these types
of things.

> > +
> > +      required:
> > +        - clocks
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: brcm,bcm2712-hvs
> > +
> > +    then:
> > +      properties:
> > +        clocks:
> > +          minItems: 2
> > +          maxItems: 2
> > +        clock-names:
> > +          items:
> > +            - const: core
> > +            - const: disp
> > +        interrupts:
> > +          items:
> > +            - description: Channel 0 End of frame
> > +            - description: Channel 1 End of frame
> > +            - description: Channel 2 End of frame
> > +        interrupt-names:
> > +          items:
> > +            - const: ch0-eof
> > +            - const: ch1-eof
> > +            - const: ch2-eof
> > +      required:
> > +        - clocks
> > +        - clock-names
> > +        - interrupt-names
>
> My previous comment still stands. Reply to people's feedback instead of
> ignoring it.

Your previous comment was
"Why requiring last two names? Commit msg does not explain that."

I didn't ignore it. The driver needs them, and the commit msg states
" but missed out that the number of interrupts and clocks changed too,
and *both need to be
named*.
Update to validate clock, interrupts, and *their names* for the variants."

Is that insufficient?

  Dave

> Best regards,
> Krzysztof
>

Re: [PATCH v4 2/3] dt-bindings: display: Fix BCM2835 HVS bindings for BCM2712

[Krzysztof Kozlowski]

On 19/12/2024 12:54, Dave Stevenson wrote:
> Hi Krzysztof
> 
> On Thu, 19 Dec 2024 at 08:42, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On Wed, Dec 18, 2024 at 02:48:33PM +0000, Dave Stevenson wrote:
>>> Commit 6cfcbe548a3a ("dt-bindings: display: Add BCM2712 HVS bindings")
>>> added the compatible string for BCM2712, but missed out that
>>> the number of interrupts and clocks changed too, and both need to be
>>> named.
>>>
>>> Update to validate clock, interrupts, and their names for the variants.
>>>
>>> Fixes: 6cfcbe548a3a ("dt-bindings: display: Add BCM2712 HVS bindings")
>>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>> ---
>>>  .../bindings/display/brcm,bcm2835-hvs.yaml         | 84 ++++++++++++++++++----
>>>  1 file changed, 70 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/brcm,bcm2835-hvs.yaml b/Documentation/devicetree/bindings/display/brcm,bcm2835-hvs.yaml
>>> index f91c9dce2a44..fd25ee5ce301 100644
>>> --- a/Documentation/devicetree/bindings/display/brcm,bcm2835-hvs.yaml
>>> +++ b/Documentation/devicetree/bindings/display/brcm,bcm2835-hvs.yaml
>>> @@ -20,11 +20,20 @@ properties:
>>>      maxItems: 1
>>>
>>>    interrupts:
>>> -    maxItems: 1
>>> +    minItems: 1
>>> +    maxItems: 3
>>> +
>>> +  interrupt-names:
>>> +    minItems: 1
>>> +    maxItems: 3
>>>
>>>    clocks:
>>> -    maxItems: 1
>>> -    description: Core Clock
>>> +    minItems: 1
>>> +    maxItems: 2
>>> +
>>> +  clock-names:
>>> +    minItems: 1
>>> +    maxItems: 2
>>>
>>>  required:
>>>    - compatible
>>> @@ -33,17 +42,64 @@ required:
>>>
>>>  additionalProperties: false
>>>
>>> -if:
>>> -  properties:
>>> -    compatible:
>>> -      contains:
>>> -        enum:
>>> -          - brcm,bcm2711-hvs
>>> -          - brcm,bcm2712-hvs
>>> -
>>> -then:
>>> -  required:
>>> -    - clocks
>>> +allOf:
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const: brcm,bcm2711-hvs
>>> +
>>> +    then:
>>> +      properties:
>>> +        clocks:
>>> +          items:
>>> +            - description: Core Clock
>>> +        interrupts:
>>> +          maxItems: 1
>>
>>
>> clock-names and interrupt-names: false, unless driver needs them but all
>> this should be explained in the commit msg because it would be a change
>> to the binding.
> 
> False it is then.
> 
> Is there actually a full guide to binding requirements?
> https://www.kernel.org/doc/html/latest/devicetree/bindings/writing-schema.html
> is the closest I've found, but it doesn't obviously cover these types
> of things.
> 
>>> +
>>> +      required:
>>> +        - clocks
>>> +
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const: brcm,bcm2712-hvs
>>> +
>>> +    then:
>>> +      properties:
>>> +        clocks:
>>> +          minItems: 2
>>> +          maxItems: 2
>>> +        clock-names:
>>> +          items:
>>> +            - const: core
>>> +            - const: disp
>>> +        interrupts:
>>> +          items:
>>> +            - description: Channel 0 End of frame
>>> +            - description: Channel 1 End of frame
>>> +            - description: Channel 2 End of frame
>>> +        interrupt-names:
>>> +          items:
>>> +            - const: ch0-eof
>>> +            - const: ch1-eof
>>> +            - const: ch2-eof
>>> +      required:
>>> +        - clocks
>>> +        - clock-names
>>> +        - interrupt-names
>>
>> My previous comment still stands. Reply to people's feedback instead of
>> ignoring it.
> 
> Your previous comment was
> "Why requiring last two names? Commit msg does not explain that."
> 
> I didn't ignore it. The driver needs them, and the commit msg states

Uh, so someone added undocumented ABI. How this passed any checks? Or no
one cared to run validation?


> " but missed out that the number of interrupts and clocks changed too,
> and *both need to be
> named*.

Ah, I really did not get this.

> Update to validate clock, interrupts, and *their names* for the variants."

Please say explicitly that since some commit driver needs this. There
should be a clear reason in the commit msg why you are adding this ABI.


Best regards,
Krzysztof

Re: [PATCH v4 2/3] dt-bindings: display: Fix BCM2835 HVS bindings for BCM2712

[Dave Stevenson]

On Thu, 19 Dec 2024 at 12:14, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 19/12/2024 12:54, Dave Stevenson wrote:
> > Hi Krzysztof
> >
> > On Thu, 19 Dec 2024 at 08:42, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>
> >> On Wed, Dec 18, 2024 at 02:48:33PM +0000, Dave Stevenson wrote:
> >>> Commit 6cfcbe548a3a ("dt-bindings: display: Add BCM2712 HVS bindings")
> >>> added the compatible string for BCM2712, but missed out that
> >>> the number of interrupts and clocks changed too, and both need to be
> >>> named.
> >>>
> >>> Update to validate clock, interrupts, and their names for the variants.
> >>>
> >>> Fixes: 6cfcbe548a3a ("dt-bindings: display: Add BCM2712 HVS bindings")
> >>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >>> ---
> >>>  .../bindings/display/brcm,bcm2835-hvs.yaml         | 84 ++++++++++++++++++----
> >>>  1 file changed, 70 insertions(+), 14 deletions(-)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/display/brcm,bcm2835-hvs.yaml b/Documentation/devicetree/bindings/display/brcm,bcm2835-hvs.yaml
> >>> index f91c9dce2a44..fd25ee5ce301 100644
> >>> --- a/Documentation/devicetree/bindings/display/brcm,bcm2835-hvs.yaml
> >>> +++ b/Documentation/devicetree/bindings/display/brcm,bcm2835-hvs.yaml
> >>> @@ -20,11 +20,20 @@ properties:
> >>>      maxItems: 1
> >>>
> >>>    interrupts:
> >>> -    maxItems: 1
> >>> +    minItems: 1
> >>> +    maxItems: 3
> >>> +
> >>> +  interrupt-names:
> >>> +    minItems: 1
> >>> +    maxItems: 3
> >>>
> >>>    clocks:
> >>> -    maxItems: 1
> >>> -    description: Core Clock
> >>> +    minItems: 1
> >>> +    maxItems: 2
> >>> +
> >>> +  clock-names:
> >>> +    minItems: 1
> >>> +    maxItems: 2
> >>>
> >>>  required:
> >>>    - compatible
> >>> @@ -33,17 +42,64 @@ required:
> >>>
> >>>  additionalProperties: false
> >>>
> >>> -if:
> >>> -  properties:
> >>> -    compatible:
> >>> -      contains:
> >>> -        enum:
> >>> -          - brcm,bcm2711-hvs
> >>> -          - brcm,bcm2712-hvs
> >>> -
> >>> -then:
> >>> -  required:
> >>> -    - clocks
> >>> +allOf:
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          contains:
> >>> +            const: brcm,bcm2711-hvs
> >>> +
> >>> +    then:
> >>> +      properties:
> >>> +        clocks:
> >>> +          items:
> >>> +            - description: Core Clock
> >>> +        interrupts:
> >>> +          maxItems: 1
> >>
> >>
> >> clock-names and interrupt-names: false, unless driver needs them but all
> >> this should be explained in the commit msg because it would be a change
> >> to the binding.
> >
> > False it is then.
> >
> > Is there actually a full guide to binding requirements?
> > https://www.kernel.org/doc/html/latest/devicetree/bindings/writing-schema.html
> > is the closest I've found, but it doesn't obviously cover these types
> > of things.
> >
> >>> +
> >>> +      required:
> >>> +        - clocks
> >>> +
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          contains:
> >>> +            const: brcm,bcm2712-hvs
> >>> +
> >>> +    then:
> >>> +      properties:
> >>> +        clocks:
> >>> +          minItems: 2
> >>> +          maxItems: 2
> >>> +        clock-names:
> >>> +          items:
> >>> +            - const: core
> >>> +            - const: disp
> >>> +        interrupts:
> >>> +          items:
> >>> +            - description: Channel 0 End of frame
> >>> +            - description: Channel 1 End of frame
> >>> +            - description: Channel 2 End of frame
> >>> +        interrupt-names:
> >>> +          items:
> >>> +            - const: ch0-eof
> >>> +            - const: ch1-eof
> >>> +            - const: ch2-eof
> >>> +      required:
> >>> +        - clocks
> >>> +        - clock-names
> >>> +        - interrupt-names
> >>
> >> My previous comment still stands. Reply to people's feedback instead of
> >> ignoring it.
> >
> > Your previous comment was
> > "Why requiring last two names? Commit msg does not explain that."
> >
> > I didn't ignore it. The driver needs them, and the commit msg states
>
> Uh, so someone added undocumented ABI. How this passed any checks? Or no
> one cared to run validation?

See the cover letter:
"I missed the DT errors from the recent patchset[1] (DT patches
in linux-next via Florian, DRM bindings patches on dri-misc-next)
as Rob's bot report got spam filtered, so this is a fixup set."

> > " but missed out that the number of interrupts and clocks changed too,
> > and *both need to be
> > named*.
>
> Ah, I really did not get this.
>
> > Update to validate clock, interrupts, and *their names* for the variants."
>
> Please say explicitly that since some commit driver needs this. There
> should be a clear reason in the commit msg why you are adding this ABI.

OK, will do.

  Dave

> Best regards,
> Krzysztof