Re: [PATCH v5 2/6] dt-bindings: usb: dwc3: Add the Amlogic A1 Family DWC3 Glue Bindings

From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
To: Hanjie Lin <hanjie.lin@amlogic.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Rob Herring <robh@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Kevin Hilman <khilman@baylibre.com>,
	Yue Wang <yue.wang@amlogic.com>,
	linux-amlogic@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org, linux-usb@vger.kernel.org,
	devicetree@vger.kernel.org, Carlo Caione <carlo@caione.org>,
	Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Liang Yang <liang.yang@amlogic.com>,
	Jianxin Pan <jianxin.pan@amlogic.com>,
	Qiufang Dai <qiufang.dai@amlogic.com>,
	Jian Hu <jian.hu@amlogic.com>,
	Victor Wan <victor.wan@amlogic.com>,
	Xingyu Chen <xingyu.chen@amlogic.com>
Subject: Re: [PATCH v5 2/6] dt-bindings: usb: dwc3: Add the Amlogic A1 Family DWC3 Glue Bindings
Date: Tue, 14 Jan 2020 21:10:41 +0100	[thread overview]
Message-ID: <CAFBinCDtPoKQh64VKmqOe=U0BLPE_Ef+scmK3h1uV9hLhT6Q1g@mail.gmail.com> (raw)
In-Reply-To: <52566105-cbee-01d6-9110-f9954eabe22f@amlogic.com>

Hi Hanjie,

On Mon, Jan 13, 2020 at 2:23 AM Hanjie Lin <hanjie.lin@amlogic.com> wrote:
>
>
>
> On 2020/1/12 4:50, Martin Blumenstingl wrote:
> > Hi Hanjie,
> >
> > On Fri, Jan 10, 2020 at 6:43 AM Hanjie Lin <hanjie.lin@amlogic.com> wrote:
> > [...]
> >> @@ -37,6 +43,11 @@ properties:
> >>
> >>    clocks:
> >>      minItems: 1
> >> +    maxItems: 4
> > the driver parses one clock for G12A/G12B/SM1 and three clocks for A1
> > if there is a fourth clock: do we need to manage it in the driver?
> > (note: dt-bindings always represent the hardware, so if there's a
> > fourth clock which the driver doesn't need then it's perfectly valid
> > to describe it here. a comment which clock this is helps in the
> > code-review process)
> >
>
> Hi Martin,
>
> Sorry for this confusing, I moved xtal_usb_phy clock from glue driver to phy,
> but I missed this binding modification.
> So actually A1 do only need these three clocks and no fourth clock exist, clock
> and clock-names maxItems should be three here for A1.
I see, thank you for clarifying this!

[...]
> Do you mean something like this:
>
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          enum:
> +            - amlogic,meson-g12a-usb-ctrl
> +
> +    then:
> +      properties:
> +        clocks:
> +         minItems: 1
> +
> +  - if:
> +      properties:
> +        compatible:
> +          enum:
> +            - amlogic,meson-a1-usb-ctrl
> +
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            minItems: 3
> +       clock-names:
> +          items:
> +            - const: usb_ctrl
> +            - const: usb_bus
> +            - const: xtal_usb_ctrl
> +      required:
> +        - clock-names
this looks good to me (but keep in mind that I'm no expert on these
yaml schemas)
I wonder if we are allowed to shorten this by having one clocks
property with minItems: 1 and maxItems: 3 (like you have in the
original patch) and then only have a
amlogic,meson-a1-usb-ctrl-specific "clock-names" property (and make
that mandatory for A1 SoCs)

Martin