Re: [PATCH v3] dt-bindings: clock: versal: Convert the xlnx,zynqmp-clk.txt to yaml

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

On 28/07/2023 18:47, Conor Dooley wrote:
> On Fri, Jul 28, 2023 at 06:39:23PM +0200, Krzysztof Kozlowski wrote:
>> On 28/07/2023 18:19, Conor Dooley wrote:
>>> On Fri, Jul 28, 2023 at 06:41:50AM +0000, Datta, Shubhrajyoti wrote:
>>>> [AMD Official Use Only - General]
>>>>
>>>>> -----Original Message-----
>>>>> From: Conor Dooley <conor@kernel.org>
>>>>> Sent: Wednesday, July 26, 2023 12:57 AM
>>>>> To: Datta, Shubhrajyoti <shubhrajyoti.datta@amd.com>
>>>>> Cc: devicetree@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>; linux-
>>>>> clk@vger.kernel.org; Simek, Michal <michal.simek@amd.com>;
>>>>> conor+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
>>>>> robh+dt@kernel.org; sboyd@kernel.org; mturquette@baylibre.com
>>>>> Subject: Re: [PATCH v3] dt-bindings: clock: versal: Convert the xlnx,zynqmp-
>>>>> clk.txt to yaml
>>>>>
>>>>> On Tue, Jul 25, 2023 at 05:28:07AM +0000, Datta, Shubhrajyoti wrote:
>>>>>> [AMD Official Use Only - General]
>>>>>>
>>>>
>>>> <snip>
>>>>>>>>    clocks:
>>>>>>>>      description: List of clock specifiers which are external input
>>>>>>>>        clocks to the given clock controller.
>>>>>>>> -    items:
>>>>>>>> -      - description: reference clock
>>>>>>>> -      - description: alternate reference clock
>>>>>>>> -      - description: alternate reference clock for programmable logic
>>>>>>>> +    minItems: 3
>>>>>>>> +    maxItems: 7
>>>>>>>
>>>>>>> This doesn't seem right to me. The original binding requires 5 clock
>>>>>>> inputs, but this will relax it such that only three are needed, no?
>>>>>>> You'll need to set constraints on a per compatible basis.
>>>>>>>
>>>>>> Does below look good.
>>>>>
>>>>> I don't think that you tested it with < 5 clocks (hint, if you remove one of the
>>>>> clocks from your example below, dt_binding_check should fail).
>>>>> All the constraints need to move into the `if` bits AFAIU.
>>>>
>>>>
>>>> https://lore.kernel.org/all/20230720113110.25047-1-shubhrajyoti.datta@amd.com/
>>>> Here I had it in the if .
>>>> Then what I understood from below is that
>>>>
>>>> https://lore.kernel.org/all/745fccb0-e49d-7da7-9556-eb28aee4a32b@linaro.org/
>>>> it should be dropped from the if and added to the above.
>>>>
>>>> Maybe I am missing something.
>>>
>>> (Background I got this mail once off-list and tried to make the
>>> binding's validation work)
>>>
>>> With the current conditions, validation is completely broken. You can
>>> put in just 1 clock and 1 clock-name and dt-binding-check will pass. The
>>> only way I could satisfy it, 
>>
>> I don't understand why you think 1 clock would work. The binding has
>> clear min/max constraints in top level and strict constraints per each
>> variant (through listing items). In my opinion this is correct, except
>> what I wrote - mismatched number of clocks for zynqmp (8 against 7).
> 
> I didn't expect it would work - I tried 4 w/ the zynqmp compatible,
> which passed, although it should not have. And then I took it to the
> extreme (1) which also passed. There's something wrong with either the
> binding or the example that I can't spot.
> 

Yep, select: false.

This is some junk code :(

Best regards,
Krzysztof