| Message ID | 20241218-dt-bcm2712-fixes-v4-2-54cc88b6c229@raspberrypi.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/vc4: Fixup DT and DT binding issues from recent patchset | expand |
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
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 >
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
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
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: - |
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(-)