Re: [PATCH v2 3/8] dt-bindings: PCI: renesas-pci-usb: Allow multiple clocks

Rob Herring
2022-04-20T13:25:16Z

On Wed, Apr 20, 2022 at 03:07:59PM +0200, Herve Codina wrote:
> Hi Geert, Rob,
>
> On Thu, 14 Apr 2022 10:35:07 +0200
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> > Hi Hervé,
> >
> > On Thu, Apr 14, 2022 at 9:40 AM Herve Codina <herve.codina@bootlin.com> wrote:
> > > Define that multiple clocks can be present at clocks property.
> > >
> > > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> >
> > Thanks for your patch!
> >
> > > --- a/Documentation/devicetree/bindings/pci/renesas,pci-usb.yaml
> > > +++ b/Documentation/devicetree/bindings/pci/renesas,pci-usb.yaml
> > > @@ -54,7 +54,8 @@ properties:
> > > Standard property that helps to define the interrupt mapping.
> > >
> > > clocks:
> > > - description: The reference to the device clock.
> > > + description:
> > > + The references to the device clocks (several clocks can be referenced).
> >
> > Please describe the clocks, and add the missing "clock-names" property.
> >
> > >
> > > bus-range:
> > > description: |
> >
> > I think it would be better to combine this with [PATCH v2 4/8], as the
> > additional clocks are only present on RZ/N1.
> >
> > Then you can easily add json-schema logic to enforce the correct
> > number of clocks, depending on the compatible value.
>
> Sure.
>
> Is there a way to have the clocks description depending on the compatible value.
> I mean something like:
> --- 8< ---
> properties:
> clocks:
> maxItems: 1

This would need to cover both cases:

minItems: 1
maxItems: 3

>
> if:
> properties:
> compatible:
> contains:
> enum:
> - renesas,pci-r9a06g032
> - renesas,pci-rzn1
>
> then:
> properties:
> clocks:
> items:
> - description: Internal bus clock (AHB) for HOST
> - description: Internal bus clock (AHB) Power Management
> - description: PCI clock for USB subsystem
> minItems: 3
> maxItems: 3

Don't need minItems or maxItems here. 3 is the default size based on
'items' length.

>
> else:
> properties:
> items:

I think you meant for this to be under 'clocks'.

> - description: Device clock
> clocks:
> minItems: 1
> maxItems: 1

Just 'maxItems' is enough.

> --- 8< ---
>
> In fact, I would like to describe the 3 clocks only for the r9a06g032 SOC
> and the rzn1 family and have an other description for the other 'compatible'.
>
> I cannot succeed to do it.
>
> The only thing I can do is to leave the description of the 3 clocks out of the
> conditional part. This leads to :
>
> --- 8< ---
> properties:
> clocks:
> items:
> - description: Internal bus clock (AHB) for HOST
> - description: Internal bus clock (AHB) Power Management
> - description: PCI clock for USB subsystem
> minItems: 1
>
> if:
> properties:
> compatible:
> contains:
> enum:
> - renesas,pci-r9a06g032
> - renesas,pci-rzn1
>
> then:
> properties:
> clocks:
> minItems: 3
> maxItems: 3

minItems is enough.

>
> else:
> properties:
> clocks:
> minItems: 1
> maxItems: 1

This doesn't seem right as the description of the first clock is wrong
for this case.

I would go with the first way.

Rob