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

[Conor Dooley <conor@kernel.org>]

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

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, while keeping 7 as the maximum number of
clocks, was moving the constraints into the if/else. My guess was that 7
being fewer than the number of clocks in the items: list is part of the
problem.

Thanks,
Conor.

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