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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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