diff mbox series

[v4,3/7] dt-bindings: display: renesas,du: Add missing maxItems

Message ID 20241213-rcar-gh-dsi-v4-3-f8e41425207b@ideasonboard.com (mailing list archive)
State New
Headers show
Series drm: Add DSI/DP support for Renesas r8a779h0 V4M and grey-hawk board | expand

Commit Message

Tomi Valkeinen Dec. 13, 2024, 2:02 p.m. UTC 
From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

The binding is missing maxItems for all renesas,cmms and renesas,vsps
properties. As the amount of cmms or vsps is always a fixed amount, set
the maxItems to match the minItems.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 Documentation/devicetree/bindings/display/renesas,du.yaml | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Laurent Pinchart Dec. 13, 2024, 4:45 p.m. UTC | #1 
Hi Tomi,

Thank you for the patch.

On Fri, Dec 13, 2024 at 04:02:59PM +0200, Tomi Valkeinen wrote:
> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> 
> The binding is missing maxItems for all renesas,cmms and renesas,vsps
> properties. As the amount of cmms or vsps is always a fixed amount, set
> the maxItems to match the minItems.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
>  Documentation/devicetree/bindings/display/renesas,du.yaml | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/renesas,du.yaml b/Documentation/devicetree/bindings/display/renesas,du.yaml
> index c5b9e6812bce..e5fbc4ffe29c 100644
> --- a/Documentation/devicetree/bindings/display/renesas,du.yaml
> +++ b/Documentation/devicetree/bindings/display/renesas,du.yaml
> @@ -489,9 +489,11 @@ allOf:
>  
>          renesas,cmms:
>            minItems: 4
> +          maxItems: 4
>  
>          renesas,vsps:
>            minItems: 4
> +          maxItems: 4
>  
>        required:
>          - clock-names
> @@ -558,9 +560,11 @@ allOf:
>  
>          renesas,cmms:
>            minItems: 3
> +          maxItems: 3
>  
>          renesas,vsps:
>            minItems: 3
> +          maxItems: 3
>  
>        required:
>          - clock-names
> @@ -627,9 +631,11 @@ allOf:
>  
>          renesas,cmms:
>            minItems: 3
> +          maxItems: 3
>  
>          renesas,vsps:
>            minItems: 3
> +          maxItems: 3
>  
>        required:
>          - clock-names
> @@ -684,6 +690,7 @@ allOf:
>  
>          renesas,vsps:
>            minItems: 1
> +          maxItems: 1
>  
>        required:
>          - clock-names
> @@ -746,9 +753,11 @@ allOf:
>  
>          renesas,cmms:
>            minItems: 2
> +          maxItems: 2
>  
>          renesas,vsps:
>            minItems: 2
> +          maxItems: 2
>  
>        required:
>          - clock-names
> @@ -799,6 +808,7 @@ allOf:
>  
>          renesas,vsps:
>            minItems: 2
> +          maxItems: 2
>  
>        required:
>          - clock-names
Krzysztof Kozlowski Dec. 16, 2024, 7:58 a.m. UTC | #2 
On Fri, Dec 13, 2024 at 04:02:59PM +0200, Tomi Valkeinen wrote:
> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> 
> The binding is missing maxItems for all renesas,cmms and renesas,vsps
> properties. As the amount of cmms or vsps is always a fixed amount, set
> the maxItems to match the minItems.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> ---
>  Documentation/devicetree/bindings/display/renesas,du.yaml | 10 ++++++++++
>  1 file changed, 10 insertions(+)

The top level property should define widest constraints as well.

Best regards,
Krzysztof
Laurent Pinchart Dec. 16, 2024, 8:32 a.m. UTC | #3 
Hi Krzysztof,

On Mon, Dec 16, 2024 at 08:58:49AM +0100, Krzysztof Kozlowski wrote:
> On Fri, Dec 13, 2024 at 04:02:59PM +0200, Tomi Valkeinen wrote:
> > From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> > 
> > The binding is missing maxItems for all renesas,cmms and renesas,vsps
> > properties. As the amount of cmms or vsps is always a fixed amount, set
> > the maxItems to match the minItems.
> > 
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> > ---
> >  Documentation/devicetree/bindings/display/renesas,du.yaml | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> 
> The top level property should define widest constraints as well.

I'm curious, why is that ? I understand why a top-level default would
make sense when it's optionally overridden by model-specific values, but
in this case there's no such default. Every SoC has its own fixed value.
Krzysztof Kozlowski Dec. 16, 2024, 10:42 a.m. UTC | #4 
On 16/12/2024 09:32, Laurent Pinchart wrote:
> Hi Krzysztof,
> 
> On Mon, Dec 16, 2024 at 08:58:49AM +0100, Krzysztof Kozlowski wrote:
>> On Fri, Dec 13, 2024 at 04:02:59PM +0200, Tomi Valkeinen wrote:
>>> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>>>
>>> The binding is missing maxItems for all renesas,cmms and renesas,vsps
>>> properties. As the amount of cmms or vsps is always a fixed amount, set
>>> the maxItems to match the minItems.
>>>
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>>> ---
>>>  Documentation/devicetree/bindings/display/renesas,du.yaml | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>
>> The top level property should define widest constraints as well.
> 
> I'm curious, why is that ? I understand why a top-level default would
> make sense when it's optionally overridden by model-specific values, but
> in this case there's no such default. Every SoC has its own fixed value.

Because otherwise top level property does not have proper description
and we expect properties to be defined at top-level.

Best regards,
Krzysztof
Tomi Valkeinen Dec. 16, 2024, 10:54 a.m. UTC | #5 
Hi,

On 16/12/2024 12:42, Krzysztof Kozlowski wrote:
> On 16/12/2024 09:32, Laurent Pinchart wrote:
>> Hi Krzysztof,
>>
>> On Mon, Dec 16, 2024 at 08:58:49AM +0100, Krzysztof Kozlowski wrote:
>>> On Fri, Dec 13, 2024 at 04:02:59PM +0200, Tomi Valkeinen wrote:
>>>> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>>>>
>>>> The binding is missing maxItems for all renesas,cmms and renesas,vsps
>>>> properties. As the amount of cmms or vsps is always a fixed amount, set
>>>> the maxItems to match the minItems.
>>>>
>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>>>> ---
>>>>   Documentation/devicetree/bindings/display/renesas,du.yaml | 10 ++++++++++
>>>>   1 file changed, 10 insertions(+)
>>>
>>> The top level property should define widest constraints as well.
>>
>> I'm curious, why is that ? I understand why a top-level default would
>> make sense when it's optionally overridden by model-specific values, but
>> in this case there's no such default. Every SoC has its own fixed value.
> 
> Because otherwise top level property does not have proper description
> and we expect properties to be defined at top-level.

As we don't know what is the maximum number of items for future SoCs, 
should we then use a number that'll surely be big enough? At the moment 
the max cmms seems to be 4, so maybe 16 would be safely big enough. But 
is it then better to be extra safe, and use, say, maxItems 256?

  Tomi
Laurent Pinchart Dec. 16, 2024, 11 a.m. UTC | #6 
On Mon, Dec 16, 2024 at 11:42:30AM +0100, Krzysztof Kozlowski wrote:
> On 16/12/2024 09:32, Laurent Pinchart wrote:
> > On Mon, Dec 16, 2024 at 08:58:49AM +0100, Krzysztof Kozlowski wrote:
> >> On Fri, Dec 13, 2024 at 04:02:59PM +0200, Tomi Valkeinen wrote:
> >>> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> >>>
> >>> The binding is missing maxItems for all renesas,cmms and renesas,vsps
> >>> properties. As the amount of cmms or vsps is always a fixed amount, set
> >>> the maxItems to match the minItems.
> >>>
> >>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> >>> ---
> >>>  Documentation/devicetree/bindings/display/renesas,du.yaml | 10 ++++++++++
> >>>  1 file changed, 10 insertions(+)
> >>
> >> The top level property should define widest constraints as well.
> > 
> > I'm curious, why is that ? I understand why a top-level default would
> > make sense when it's optionally overridden by model-specific values, but
> > in this case there's no such default. Every SoC has its own fixed value.
> 
> Because otherwise top level property does not have proper description
> and we expect properties to be defined at top-level.

Is it invalid YAML schema to have

  renesas,cmms:
    description: ......

with the min/maxItems in the conditional blocks ? Or did you mean, by
proper description, not just the description field ? We could have

  renesas,cmms:
    description: ......
    minItems: 1
    maxItems: 256

but I really don't see what that would bring from a documentation point
of view. Are there tools that depend on minItems and maxItems being
present at the top level ?
Tomi Valkeinen Dec. 16, 2024, 11:02 a.m. UTC | #7 
Hi,

On 16/12/2024 10:32, Laurent Pinchart wrote:
> Hi Krzysztof,
> 
> On Mon, Dec 16, 2024 at 08:58:49AM +0100, Krzysztof Kozlowski wrote:
>> On Fri, Dec 13, 2024 at 04:02:59PM +0200, Tomi Valkeinen wrote:
>>> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>>>
>>> The binding is missing maxItems for all renesas,cmms and renesas,vsps
>>> properties. As the amount of cmms or vsps is always a fixed amount, set
>>> the maxItems to match the minItems.
>>>
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>>> ---
>>>   Documentation/devicetree/bindings/display/renesas,du.yaml | 10 ++++++++++
>>>   1 file changed, 10 insertions(+)
>>
>> The top level property should define widest constraints as well.
> 
> I'm curious, why is that ? I understand why a top-level default would
> make sense when it's optionally overridden by model-specific values, but
> in this case there's no such default. Every SoC has its own fixed value.

Looking at the file, shouldn't we have minItems == maxItems for 
interrupts and resets too? Well, I guess for interrupts we could in 
theory run with just some of the interrupt lines connected. I'm not sure 
if that's the case for resets.

  Tomi
Laurent Pinchart Dec. 16, 2024, 11:09 a.m. UTC | #8 
On Mon, Dec 16, 2024 at 01:02:26PM +0200, Tomi Valkeinen wrote:
> On 16/12/2024 10:32, Laurent Pinchart wrote:
> > On Mon, Dec 16, 2024 at 08:58:49AM +0100, Krzysztof Kozlowski wrote:
> >> On Fri, Dec 13, 2024 at 04:02:59PM +0200, Tomi Valkeinen wrote:
> >>> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> >>>
> >>> The binding is missing maxItems for all renesas,cmms and renesas,vsps
> >>> properties. As the amount of cmms or vsps is always a fixed amount, set
> >>> the maxItems to match the minItems.
> >>>
> >>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> >>> ---
> >>>   Documentation/devicetree/bindings/display/renesas,du.yaml | 10 ++++++++++
> >>>   1 file changed, 10 insertions(+)
> >>
> >> The top level property should define widest constraints as well.
> > 
> > I'm curious, why is that ? I understand why a top-level default would
> > make sense when it's optionally overridden by model-specific values, but
> > in this case there's no such default. Every SoC has its own fixed value.
> 
> Looking at the file, shouldn't we have minItems == maxItems for 
> interrupts and resets too? Well, I guess for interrupts we could in 
> theory run with just some of the interrupt lines connected. I'm not sure 
> if that's the case for resets.

Unless there's some magic handling of min/maxItems for those that I
wouldn't be aware of, I think it makes sense.
Krzysztof Kozlowski Dec. 16, 2024, 11:22 a.m. UTC | #9 
On 16/12/2024 11:54, Tomi Valkeinen wrote:
> Hi,
> 
> On 16/12/2024 12:42, Krzysztof Kozlowski wrote:
>> On 16/12/2024 09:32, Laurent Pinchart wrote:
>>> Hi Krzysztof,
>>>
>>> On Mon, Dec 16, 2024 at 08:58:49AM +0100, Krzysztof Kozlowski wrote:
>>>> On Fri, Dec 13, 2024 at 04:02:59PM +0200, Tomi Valkeinen wrote:
>>>>> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>>>>>
>>>>> The binding is missing maxItems for all renesas,cmms and renesas,vsps
>>>>> properties. As the amount of cmms or vsps is always a fixed amount, set
>>>>> the maxItems to match the minItems.
>>>>>
>>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>>>>> ---
>>>>>   Documentation/devicetree/bindings/display/renesas,du.yaml | 10 ++++++++++
>>>>>   1 file changed, 10 insertions(+)
>>>>
>>>> The top level property should define widest constraints as well.
>>>
>>> I'm curious, why is that ? I understand why a top-level default would
>>> make sense when it's optionally overridden by model-specific values, but
>>> in this case there's no such default. Every SoC has its own fixed value.
>>
>> Because otherwise top level property does not have proper description
>> and we expect properties to be defined at top-level.
> 
> As we don't know what is the maximum number of items for future SoCs, 
> should we then use a number that'll surely be big enough? At the moment 
> the max cmms seems to be 4, so maybe 16 would be safely big enough. But 
> is it then better to be extra safe, and use, say, maxItems 256?

No, look at all other bindings. Widest constraints for ONLY these
devices. We do not talk about future SoCs here.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/display/renesas,du.yaml b/Documentation/devicetree/bindings/display/renesas,du.yaml
index c5b9e6812bce..e5fbc4ffe29c 100644
--- a/Documentation/devicetree/bindings/display/renesas,du.yaml
+++ b/Documentation/devicetree/bindings/display/renesas,du.yaml
@@ -489,9 +489,11 @@  allOf:
 
         renesas,cmms:
           minItems: 4
+          maxItems: 4
 
         renesas,vsps:
           minItems: 4
+          maxItems: 4
 
       required:
         - clock-names
@@ -558,9 +560,11 @@  allOf:
 
         renesas,cmms:
           minItems: 3
+          maxItems: 3
 
         renesas,vsps:
           minItems: 3
+          maxItems: 3
 
       required:
         - clock-names
@@ -627,9 +631,11 @@  allOf:
 
         renesas,cmms:
           minItems: 3
+          maxItems: 3
 
         renesas,vsps:
           minItems: 3
+          maxItems: 3
 
       required:
         - clock-names
@@ -684,6 +690,7 @@  allOf:
 
         renesas,vsps:
           minItems: 1
+          maxItems: 1
 
       required:
         - clock-names
@@ -746,9 +753,11 @@  allOf:
 
         renesas,cmms:
           minItems: 2
+          maxItems: 2
 
         renesas,vsps:
           minItems: 2
+          maxItems: 2
 
       required:
         - clock-names
@@ -799,6 +808,7 @@  allOf:
 
         renesas,vsps:
           minItems: 2
+          maxItems: 2
 
       required:
         - clock-names