| Message ID | 20240131114324.3722428-4-xu.yang_2@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v5,1/8] dt-bindings: usb: usbmisc-imx: add fsl,imx8ulp-usbmisc compatible | expand |
On 31/01/2024 12:43, Xu Yang wrote: > Change reg, interrupts, clock and clock-names as common properties and add > restrictions on them for different compatibles. > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com> > > --- > Changes in v4: > - new patch since v3's discussion > - split the reg, interrupts, clock and clock-names properties into > common part and device-specific > Changes in v5: > - keep common property unchanged > - make if-then more readable > - remove non imx part > --- > .../devicetree/bindings/usb/ci-hdrc-usb2.yaml | 118 ++++++++++++++++++ > 1 file changed, 118 insertions(+) > > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml > index 3b56e0edb1c6..6ad3582051b8 100644 > --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml > @@ -412,6 +412,124 @@ allOf: > samsung,picophy-pre-emp-curr-control: false > samsung,picophy-dc-vol-level-adjust: false > > + - if: > + properties: > + compatible: > + const: fsl,imx27-usb > + then: > + properties: > + clocks: > + minItems: 3 > + maxItems: 3 > + clock-names: > + minItems: 3 > + maxItems: 3 > + items: > + anyOf: > + - const: ipg > + - const: ahb > + - const: per This would be just: enum: [ipg, ahb, per], but in both cases I question why the order should be flexible? Nothing in commit msg explains it. Plus I will repeat myself from your v4. I don't think this is helping, because the file will soon grow to umnanageable chunk. I prefer to fix it at beginning, before we reach snps-schema level of complexities. Please define common schema, reference in this file and move IMX to own file. In any case, please provide short comment in the code why you have such duplicated if:then: Best regards, Krzysztof
Hi Krzysztof, > > On 31/01/2024 12:43, Xu Yang wrote: > > Change reg, interrupts, clock and clock-names as common properties and add > > restrictions on them for different compatibles. > > > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com> > > > > --- > > Changes in v4: > > - new patch since v3's discussion > > - split the reg, interrupts, clock and clock-names properties into > > common part and device-specific > > Changes in v5: > > - keep common property unchanged > > - make if-then more readable > > - remove non imx part > > --- > > .../devicetree/bindings/usb/ci-hdrc-usb2.yaml | 118 ++++++++++++++++++ > > 1 file changed, 118 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml b/Documentation/devicetree/bindings/usb/ci- > hdrc-usb2.yaml > > index 3b56e0edb1c6..6ad3582051b8 100644 > > --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml > > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml > > @@ -412,6 +412,124 @@ allOf: > > samsung,picophy-pre-emp-curr-control: false > > samsung,picophy-dc-vol-level-adjust: false > > > > + - if: > > + properties: > > + compatible: > > + const: fsl,imx27-usb > > + then: > > + properties: > > + clocks: > > + minItems: 3 > > + maxItems: 3 > > + clock-names: > > + minItems: 3 > > + maxItems: 3 > > + items: > > + anyOf: > > + - const: ipg > > + - const: ahb > > + - const: per > > This would be just: enum: [ipg, ahb, per], but in both cases I question > why the order should be flexible? Nothing in commit msg explains it. The driver will get the clock by clock-name, then the order should not matter. However, these three clock-names should be present at the same time. I should use enum then. > > Plus I will repeat myself from your v4. I don't think this is helping, > because the file will soon grow to umnanageable chunk. I prefer to fix > it at beginning, before we reach snps-schema level of complexities. > > Please define common schema, reference in this file and move IMX to own > file. I'm not that familiar with dt-bindings architecture. If I define a common schema, then should I create imx, qcom, nvidia and other dt-binding files too? > > In any case, please provide short comment in the code why you have such > duplicated if:then: Okay. > > Best regards, > Krzysztof
On 02/02/2024 10:10, Xu Yang wrote: > Hi Krzysztof, > >> >> On 31/01/2024 12:43, Xu Yang wrote: >>> Change reg, interrupts, clock and clock-names as common properties and add >>> restrictions on them for different compatibles. >>> >>> Signed-off-by: Xu Yang <xu.yang_2@nxp.com> >>> >>> --- >>> Changes in v4: >>> - new patch since v3's discussion >>> - split the reg, interrupts, clock and clock-names properties into >>> common part and device-specific >>> Changes in v5: >>> - keep common property unchanged >>> - make if-then more readable >>> - remove non imx part >>> --- >>> .../devicetree/bindings/usb/ci-hdrc-usb2.yaml | 118 ++++++++++++++++++ >>> 1 file changed, 118 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml b/Documentation/devicetree/bindings/usb/ci- >> hdrc-usb2.yaml >>> index 3b56e0edb1c6..6ad3582051b8 100644 >>> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml >>> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml >>> @@ -412,6 +412,124 @@ allOf: >>> samsung,picophy-pre-emp-curr-control: false >>> samsung,picophy-dc-vol-level-adjust: false >>> >>> + - if: >>> + properties: >>> + compatible: >>> + const: fsl,imx27-usb >>> + then: >>> + properties: >>> + clocks: >>> + minItems: 3 >>> + maxItems: 3 >>> + clock-names: >>> + minItems: 3 >>> + maxItems: 3 >>> + items: >>> + anyOf: >>> + - const: ipg >>> + - const: ahb >>> + - const: per >> >> This would be just: enum: [ipg, ahb, per], but in both cases I question >> why the order should be flexible? Nothing in commit msg explains it. > > The driver will get the clock by clock-name, then the order should not > matter. However, these three clock-names should be present at the same > time. I should use enum then. > >> >> Plus I will repeat myself from your v4. I don't think this is helping, >> because the file will soon grow to umnanageable chunk. I prefer to fix >> it at beginning, before we reach snps-schema level of complexities. >> >> Please define common schema, reference in this file and move IMX to own >> file. > > I'm not that familiar with dt-bindings architecture. If I define a common > schema, then should I create imx, qcom, nvidia and other dt-binding files > too? No, the rest you can leave here. Someone, maybe me, will move them some time. The point is to move at least IMX to its own file. Best regards, Krzysztof
On 02/02/2024 10:10, Xu Yang wrote: > Hi Krzysztof, > >> >> On 31/01/2024 12:43, Xu Yang wrote: >>> Change reg, interrupts, clock and clock-names as common properties and add >>> restrictions on them for different compatibles. >>> >>> Signed-off-by: Xu Yang <xu.yang_2@nxp.com> >>> >>> --- >>> Changes in v4: >>> - new patch since v3's discussion >>> - split the reg, interrupts, clock and clock-names properties into >>> common part and device-specific >>> Changes in v5: >>> - keep common property unchanged >>> - make if-then more readable >>> - remove non imx part >>> --- >>> .../devicetree/bindings/usb/ci-hdrc-usb2.yaml | 118 ++++++++++++++++++ >>> 1 file changed, 118 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml b/Documentation/devicetree/bindings/usb/ci- >> hdrc-usb2.yaml >>> index 3b56e0edb1c6..6ad3582051b8 100644 >>> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml >>> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml >>> @@ -412,6 +412,124 @@ allOf: >>> samsung,picophy-pre-emp-curr-control: false >>> samsung,picophy-dc-vol-level-adjust: false >>> >>> + - if: >>> + properties: >>> + compatible: >>> + const: fsl,imx27-usb >>> + then: >>> + properties: >>> + clocks: >>> + minItems: 3 >>> + maxItems: 3 >>> + clock-names: >>> + minItems: 3 >>> + maxItems: 3 >>> + items: >>> + anyOf: >>> + - const: ipg >>> + - const: ahb >>> + - const: per >> >> This would be just: enum: [ipg, ahb, per], but in both cases I question >> why the order should be flexible? Nothing in commit msg explains it. > > The driver will get the clock by clock-name, then the order should not > matter. However, these three clock-names should be present at the same > time. I should use enum then. Forgot to answer to this. Which driver? U-boot? OpenBSD? The one in my custom Linux kernel fork (open-source and published on github)? Did you check all of them? The order should be strict (or "fixed" as in non-flexible), unless there is some reason against. If one driver does not care about order, is not the reason. This comment applies to all bindings so to all your future contributions. I really keep repeating it... Best regards, Krzysztof
On Fri, Feb 02, 2024 at 12:04:51PM +0100, Krzysztof Kozlowski wrote: > On 02/02/2024 10:10, Xu Yang wrote: > > Hi Krzysztof, > > > >> > >> On 31/01/2024 12:43, Xu Yang wrote: > >>> Change reg, interrupts, clock and clock-names as common properties and add > >>> restrictions on them for different compatibles. > >>> > >>> Signed-off-by: Xu Yang <xu.yang_2@nxp.com> > >>> > >>> --- > >>> Changes in v4: > >>> - new patch since v3's discussion > >>> - split the reg, interrupts, clock and clock-names properties into > >>> common part and device-specific > >>> Changes in v5: > >>> - keep common property unchanged > >>> - make if-then more readable > >>> - remove non imx part > >>> --- > >>> .../devicetree/bindings/usb/ci-hdrc-usb2.yaml | 118 ++++++++++++++++++ > >>> 1 file changed, 118 insertions(+) > >>> > >>> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml b/Documentation/devicetree/bindings/usb/ci- > >> hdrc-usb2.yaml > >>> index 3b56e0edb1c6..6ad3582051b8 100644 > >>> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml > >>> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml > >>> @@ -412,6 +412,124 @@ allOf: > >>> samsung,picophy-pre-emp-curr-control: false > >>> samsung,picophy-dc-vol-level-adjust: false > >>> > >>> + - if: > >>> + properties: > >>> + compatible: > >>> + const: fsl,imx27-usb > >>> + then: > >>> + properties: > >>> + clocks: > >>> + minItems: 3 > >>> + maxItems: 3 > >>> + clock-names: > >>> + minItems: 3 > >>> + maxItems: 3 > >>> + items: > >>> + anyOf: > >>> + - const: ipg > >>> + - const: ahb > >>> + - const: per > >> > >> This would be just: enum: [ipg, ahb, per], but in both cases I question > >> why the order should be flexible? Nothing in commit msg explains it. > > > > The driver will get the clock by clock-name, then the order should not > > matter. However, these three clock-names should be present at the same > > time. I should use enum then. > > > >> > >> Plus I will repeat myself from your v4. I don't think this is helping, > >> because the file will soon grow to umnanageable chunk. I prefer to fix > >> it at beginning, before we reach snps-schema level of complexities. > >> > >> Please define common schema, reference in this file and move IMX to own > >> file. > > > > I'm not that familiar with dt-bindings architecture. If I define a common > > schema, then should I create imx, qcom, nvidia and other dt-binding files > > too? > > No, the rest you can leave here. Someone, maybe me, will move them some > time. The point is to move at least IMX to its own file. That will only work as long as i.MX doesn't define any extra properties which it already has. We have to have at a minium common, imx, and everything else schema docs. Rob
> > On 02/02/2024 10:10, Xu Yang wrote: > > Hi Krzysztof, > > > >> > >> On 31/01/2024 12:43, Xu Yang wrote: > >>> Change reg, interrupts, clock and clock-names as common properties and add > >>> restrictions on them for different compatibles. > >>> > >>> Signed-off-by: Xu Yang <xu.yang_2@nxp.com> > >>> > >>> --- > >>> Changes in v4: > >>> - new patch since v3's discussion > >>> - split the reg, interrupts, clock and clock-names properties into > >>> common part and device-specific > >>> Changes in v5: > >>> - keep common property unchanged > >>> - make if-then more readable > >>> - remove non imx part > >>> --- > >>> .../devicetree/bindings/usb/ci-hdrc-usb2.yaml | 118 ++++++++++++++++++ > >>> 1 file changed, 118 insertions(+) > >>> > >>> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml b/Documentation/devicetree/bindings/usb/ci- > >> hdrc-usb2.yaml > >>> index 3b56e0edb1c6..6ad3582051b8 100644 > >>> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml > >>> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml > >>> @@ -412,6 +412,124 @@ allOf: > >>> samsung,picophy-pre-emp-curr-control: false > >>> samsung,picophy-dc-vol-level-adjust: false > >>> > >>> + - if: > >>> + properties: > >>> + compatible: > >>> + const: fsl,imx27-usb > >>> + then: > >>> + properties: > >>> + clocks: > >>> + minItems: 3 > >>> + maxItems: 3 > >>> + clock-names: > >>> + minItems: 3 > >>> + maxItems: 3 > >>> + items: > >>> + anyOf: > >>> + - const: ipg > >>> + - const: ahb > >>> + - const: per > >> > >> This would be just: enum: [ipg, ahb, per], but in both cases I question > >> why the order should be flexible? Nothing in commit msg explains it. > > > > The driver will get the clock by clock-name, then the order should not > > matter. However, these three clock-names should be present at the same > > time. I should use enum then. > > Forgot to answer to this. > > Which driver? U-boot? OpenBSD? The one in my custom Linux kernel fork > (open-source and published on github)? Did you check all of them? I mean below driver in linux kernel tree. https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git/tree/drivers/usb/chipidea/ci_hdrc_imx.c?h=linux-6.1.y#n191 Thanks, Xu Yang
diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml index 3b56e0edb1c6..6ad3582051b8 100644 --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml @@ -412,6 +412,124 @@ allOf: samsung,picophy-pre-emp-curr-control: false samsung,picophy-dc-vol-level-adjust: false + - if: + properties: + compatible: + const: fsl,imx27-usb + then: + properties: + clocks: + minItems: 3 + maxItems: 3 + clock-names: + minItems: 3 + maxItems: 3 + items: + anyOf: + - const: ipg + - const: ahb + - const: per + + - if: + properties: + compatible: + contains: + enum: + - fsl,imx25-usb + - fsl,imx35-usb + then: + properties: + clocks: + minItems: 3 + maxItems: 3 + clock-names: + minItems: 3 + maxItems: 3 + items: + anyOf: + - const: ipg + - const: ahb + - const: per + + - if: + properties: + compatible: + contains: + enum: + - qcom,ci-hdrc + then: + properties: + reg: + minItems: 2 + maxItems: 2 + interrupts: + minItems: 1 + maxItems: 2 + clocks: + minItems: 2 + maxItems: 3 + clock-names: + minItems: 2 + maxItems: 3 + items: + anyOf: + - const: core + - const: iface + - const: fs + description: optional + + - if: + properties: + compatible: + contains: + enum: + - chipidea,usb2 + - fsl,imx23-usb + - fsl,imx28-usb + - fsl,imx7d-usb + - fsl,vf610-usb + then: + properties: + clocks: + minItems: 1 + maxItems: 1 + clock-names: + minItems: 1 + maxItems: 1 + + - if: + properties: + compatible: + contains: + oneOf: + - pattern: '^fsl,imx5[0-3]+-usb$' + - pattern: '^fsl,imx6[a-z]+-usb$' + then: + properties: + clocks: + minItems: 1 + maxItems: 1 + clock-names: + minItems: 1 + maxItems: 1 + + - if: + properties: + compatible: + contains: + enum: + - chipidea,usb2 + - fsl,imx27-usb + - fsl,imx6ul-usb + then: + properties: + reg: + minItems: 1 + maxItems: 1 + interrupts: + minItems: 1 + maxItems: 1 + unevaluatedProperties: false examples:
Change reg, interrupts, clock and clock-names as common properties and add restrictions on them for different compatibles. Signed-off-by: Xu Yang <xu.yang_2@nxp.com> --- Changes in v4: - new patch since v3's discussion - split the reg, interrupts, clock and clock-names properties into common part and device-specific Changes in v5: - keep common property unchanged - make if-then more readable - remove non imx part --- .../devicetree/bindings/usb/ci-hdrc-usb2.yaml | 118 ++++++++++++++++++ 1 file changed, 118 insertions(+)