[PATCH 1/2] media: dt-bindings: renesas,fcp: add top-level constraints

[PATCH 1/2] media: dt-bindings: renesas,fcp: add top-level constraints

[Krzysztof Kozlowski]

Properties with variable number of items per each device are expected to
have widest constraints in top-level "properties:" block and further
customized (narrowed) in "if:then:".  Add missing top-level constraints
for clocks and clock-names.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../devicetree/bindings/media/renesas,fcp.yaml    | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/renesas,fcp.yaml b/Documentation/devicetree/bindings/media/renesas,fcp.yaml
index c6abe719881b..68c088821b22 100644
--- a/Documentation/devicetree/bindings/media/renesas,fcp.yaml
+++ b/Documentation/devicetree/bindings/media/renesas,fcp.yaml
@@ -34,9 +34,15 @@ properties:
   reg:
     maxItems: 1
 
-  clocks: true
+  clocks:
+    minItems: 1
+    maxItems: 3
 
-  clock-names: true
+  clock-names:
+    items:
+      - const: aclk
+      - const: pclk
+      - const: vclk
 
   iommus:
     maxItems: 1
@@ -71,11 +77,6 @@ allOf:
             - description: Main clock
             - description: Register access clock
             - description: Video clock
-        clock-names:
-          items:
-            - const: aclk
-            - const: pclk
-            - const: vclk
       required:
         - clock-names
     else:
-- 
2.43.0

[PATCH 2/2] media: dt-bindings: renesas,vsp1: add top-level constraints

[Krzysztof Kozlowski]

Properties with variable number of items per each device are expected to
have widest constraints in top-level "properties:" block and further
customized (narrowed) in "if:then:".  Add missing top-level constraints
for clocks and clock-names.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../devicetree/bindings/media/renesas,vsp1.yaml  | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
index 3265e922647c..94c0ab9a5be2 100644
--- a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
+++ b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
@@ -32,8 +32,15 @@ properties:
   interrupts:
     maxItems: 1
 
-  clocks: true
-  clock-names: true
+  clocks:
+    minItems: 1
+    maxItems: 3
+
+  clock-names:
+    items:
+      - const: aclk
+      - const: pclk
+      - const: vclk
 
   power-domains:
     maxItems: 1
@@ -81,11 +88,6 @@ allOf:
             - description: Main clock
             - description: Register access clock
             - description: Video clock
-        clock-names:
-          items:
-            - const: aclk
-            - const: pclk
-            - const: vclk
       required:
         - clock-names
     else:
-- 
2.43.0

Re: [PATCH 1/2] media: dt-bindings: renesas,fcp: add top-level constraints

[Laurent Pinchart]

Hi Krzysztof,

Thank you for the patch.

On Sun, Aug 18, 2024 at 07:29:36PM +0200, Krzysztof Kozlowski wrote:
> Properties with variable number of items per each device are expected to
> have widest constraints in top-level "properties:" block and further
> customized (narrowed) in "if:then:".  Add missing top-level constraints
> for clocks and clock-names.

In this specific case I think it's fine, but generally speaking, how do
you handle that rule when different variants have completely different
clocks, not just lack some of the clocks ?

> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>  .../devicetree/bindings/media/renesas,fcp.yaml    | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/renesas,fcp.yaml b/Documentation/devicetree/bindings/media/renesas,fcp.yaml
> index c6abe719881b..68c088821b22 100644
> --- a/Documentation/devicetree/bindings/media/renesas,fcp.yaml
> +++ b/Documentation/devicetree/bindings/media/renesas,fcp.yaml
> @@ -34,9 +34,15 @@ properties:
>    reg:
>      maxItems: 1
>  
> -  clocks: true
> +  clocks:
> +    minItems: 1
> +    maxItems: 3
>  
> -  clock-names: true
> +  clock-names:
> +    items:
> +      - const: aclk
> +      - const: pclk
> +      - const: vclk
>  
>    iommus:
>      maxItems: 1
> @@ -71,11 +77,6 @@ allOf:
>              - description: Main clock
>              - description: Register access clock
>              - description: Video clock
> -        clock-names:
> -          items:
> -            - const: aclk
> -            - const: pclk
> -            - const: vclk

Any specific reason to move the clock names but not the descriptions ?
The assymetry bothers me.

>        required:
>          - clock-names
>      else:

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] media: dt-bindings: renesas,vsp1: add top-level constraints

[Laurent Pinchart]

Hi Krzysztof,

Thank you for the patch.

On Sun, Aug 18, 2024 at 07:29:37PM +0200, Krzysztof Kozlowski wrote:
> Properties with variable number of items per each device are expected to
> have widest constraints in top-level "properties:" block and further
> customized (narrowed) in "if:then:".  Add missing top-level constraints
> for clocks and clock-names.

Same comments here as for patch 1/2.

> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>  .../devicetree/bindings/media/renesas,vsp1.yaml  | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> index 3265e922647c..94c0ab9a5be2 100644
> --- a/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> +++ b/Documentation/devicetree/bindings/media/renesas,vsp1.yaml
> @@ -32,8 +32,15 @@ properties:
>    interrupts:
>      maxItems: 1
>  
> -  clocks: true
> -  clock-names: true
> +  clocks:
> +    minItems: 1
> +    maxItems: 3
> +
> +  clock-names:
> +    items:
> +      - const: aclk
> +      - const: pclk
> +      - const: vclk
>  
>    power-domains:
>      maxItems: 1
> @@ -81,11 +88,6 @@ allOf:
>              - description: Main clock
>              - description: Register access clock
>              - description: Video clock
> -        clock-names:
> -          items:
> -            - const: aclk
> -            - const: pclk
> -            - const: vclk
>        required:
>          - clock-names
>      else:

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] media: dt-bindings: renesas,fcp: add top-level constraints

[Krzysztof Kozlowski]

On 18/08/2024 19:37, Laurent Pinchart wrote:
> Hi Krzysztof,
> 
> Thank you for the patch.
> 
> On Sun, Aug 18, 2024 at 07:29:36PM +0200, Krzysztof Kozlowski wrote:
>> Properties with variable number of items per each device are expected to
>> have widest constraints in top-level "properties:" block and further
>> customized (narrowed) in "if:then:".  Add missing top-level constraints
>> for clocks and clock-names.
> 
> In this specific case I think it's fine, but generally speaking, how do
> you handle that rule when different variants have completely different
> clocks, not just lack some of the clocks ?

I don't understand the problem. We handle it as usual, as in all
bindings. Here there is no such case, thus names go to the top.

>>  
>> -  clock-names: true
>> +  clock-names:
>> +    items:
>> +      - const: aclk
>> +      - const: pclk
>> +      - const: vclk
>>  
>>    iommus:
>>      maxItems: 1
>> @@ -71,11 +77,6 @@ allOf:
>>              - description: Main clock
>>              - description: Register access clock
>>              - description: Video clock
>> -        clock-names:
>> -          items:
>> -            - const: aclk
>> -            - const: pclk
>> -            - const: vclk
> 
> Any specific reason to move the clock names but not the descriptions ?
> The assymetry bothers me.

The other variant does not have description of the first clock, so
moving it would be incorrect. Moving names is correct, because other
variant does not have clock-names at all.

Best regards,
Krzysztof

Re: [PATCH 1/2] media: dt-bindings: renesas,fcp: add top-level constraints

[Laurent Pinchart]

On Sun, Aug 18, 2024 at 07:45:55PM +0200, Krzysztof Kozlowski wrote:
> On 18/08/2024 19:37, Laurent Pinchart wrote:
> > On Sun, Aug 18, 2024 at 07:29:36PM +0200, Krzysztof Kozlowski wrote:
> >> Properties with variable number of items per each device are expected to
> >> have widest constraints in top-level "properties:" block and further
> >> customized (narrowed) in "if:then:".  Add missing top-level constraints
> >> for clocks and clock-names.
> > 
> > In this specific case I think it's fine, but generally speaking, how do
> > you handle that rule when different variants have completely different
> > clocks, not just lack some of the clocks ?
> 
> I don't understand the problem. We handle it as usual, as in all
> bindings. Here there is no such case, thus names go to the top.

That answers the question, the clock names would still be
variant-specific in that case.

While the change here won't cause validation failures, I think it's
confusing to define the clock names at the top level, knowing they don't
apply to some of the variants, if we don't also define the description
there. I'd move either both or neither.

> >>  
> >> -  clock-names: true
> >> +  clock-names:
> >> +    items:
> >> +      - const: aclk
> >> +      - const: pclk
> >> +      - const: vclk
> >>  
> >>    iommus:
> >>      maxItems: 1
> >> @@ -71,11 +77,6 @@ allOf:
> >>              - description: Main clock
> >>              - description: Register access clock
> >>              - description: Video clock
> >> -        clock-names:
> >> -          items:
> >> -            - const: aclk
> >> -            - const: pclk
> >> -            - const: vclk
> > 
> > Any specific reason to move the clock names but not the descriptions ?
> > The assymetry bothers me.
> 
> The other variant does not have description of the first clock, so
> moving it would be incorrect. Moving names is correct, because other
> variant does not have clock-names at all.

I don't think it's incorrect, when the FCP has a single clock, it's the
main clock.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] media: dt-bindings: renesas,fcp: add top-level constraints

[Krzysztof Kozlowski]

On 18/08/2024 19:50, Laurent Pinchart wrote:
> On Sun, Aug 18, 2024 at 07:45:55PM +0200, Krzysztof Kozlowski wrote:
>> On 18/08/2024 19:37, Laurent Pinchart wrote:
>>> On Sun, Aug 18, 2024 at 07:29:36PM +0200, Krzysztof Kozlowski wrote:
>>>> Properties with variable number of items per each device are expected to
>>>> have widest constraints in top-level "properties:" block and further
>>>> customized (narrowed) in "if:then:".  Add missing top-level constraints
>>>> for clocks and clock-names.
>>>
>>> In this specific case I think it's fine, but generally speaking, how do
>>> you handle that rule when different variants have completely different
>>> clocks, not just lack some of the clocks ?
>>
>> I don't understand the problem. We handle it as usual, as in all
>> bindings. Here there is no such case, thus names go to the top.
> 
> That answers the question, the clock names would still be
> variant-specific in that case.
> 
> While the change here won't cause validation failures, I think it's
> confusing to define the clock names at the top level, knowing they don't
> apply to some of the variants, if we don't also define the description
> there. I'd move either both or neither.

First, they apply to ALL variants using clock-names.
Second, we want such lists, like clocks/resets/interrupts, to share as
much as possible between variants, e.g. keep the same order. Having
clock-names listed at top-level encourages this and prevents people from
adding new binding with:

"vclk", "aclk", "pclk",
"new_clock_but_i_want_to_mess_order_of_everything_because_i_can"

> 
>>>>  
>>>> -  clock-names: true
>>>> +  clock-names:
>>>> +    items:
>>>> +      - const: aclk
>>>> +      - const: pclk
>>>> +      - const: vclk
>>>>  
>>>>    iommus:
>>>>      maxItems: 1
>>>> @@ -71,11 +77,6 @@ allOf:
>>>>              - description: Main clock
>>>>              - description: Register access clock
>>>>              - description: Video clock
>>>> -        clock-names:
>>>> -          items:
>>>> -            - const: aclk
>>>> -            - const: pclk
>>>> -            - const: vclk
>>>
>>> Any specific reason to move the clock names but not the descriptions ?
>>> The assymetry bothers me.
>>
>> The other variant does not have description of the first clock, so
>> moving it would be incorrect. Moving names is correct, because other
>> variant does not have clock-names at all.
> 
> I don't think it's incorrect, when the FCP has a single clock, it's the
> main clock.

Could be main clock, could be something else for me - I did not
investigate enough. If it is main clock, I will move the description as
well.

Best regards,
Krzysztof

Re: [PATCH 1/2] media: dt-bindings: renesas,fcp: add top-level constraints

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 454 bytes --]

On Sun, Aug 18, 2024 at 07:29:36PM +0200, Krzysztof Kozlowski wrote:
> Properties with variable number of items per each device are expected to
> have widest constraints in top-level "properties:" block and further
> customized (narrowed) in "if:then:".  Add missing top-level constraints
> for clocks and clock-names.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/2] media: dt-bindings: renesas,vsp1: add top-level constraints

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 454 bytes --]

On Sun, Aug 18, 2024 at 07:29:37PM +0200, Krzysztof Kozlowski wrote:
> Properties with variable number of items per each device are expected to
> have widest constraints in top-level "properties:" block and further
> customized (narrowed) in "if:then:".  Add missing top-level constraints
> for clocks and clock-names.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]