(EXT) Re: [PATCH v2 1/3] dt-bindings: phy: imx8mq-usb-phy: Add imx8mp specific flags

[Alexander Stein]

Hello,

thanks for the review.

Am Dienstag, 21. Dezember 2021, 17:59:52 CET schrieb Rob Herring:
> On Thu, Dec 16, 2021 at 05:05:39PM +0100, Alexander Stein wrote:
> > This adds bindings for features only available on imx8mp. They allow
> > setting polarity of PWR and OC as well as disabling port power control.
> > Also permanently atteched can be annotated as well.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein at ew.tq-group.com>
> > ---
> > Adding properties specific to one compatible globally and disabling them
> > on
> > other compatibles is the way to go?
> > 
> > Are there any best practices on the usage of '-' and/or '_' in property
> > names?
> Yes, don't use '_'.

Alright, got it.

> >  .../bindings/phy/fsl,imx8mq-usb-phy.yaml      | 52 ++++++++++++++++++-
> >  1 file changed, 51 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/phy/fsl,imx8mq-usb-phy.yaml
> > b/Documentation/devicetree/bindings/phy/fsl,imx8mq-usb-phy.yaml index
> > 2936f3510a6a..1d28b7d1c413 100644
> > --- a/Documentation/devicetree/bindings/phy/fsl,imx8mq-usb-phy.yaml
> > +++ b/Documentation/devicetree/bindings/phy/fsl,imx8mq-usb-phy.yaml
> > 
> > @@ -16,7 +16,8 @@ properties:
> >        - fsl,imx8mp-usb-phy
> >    
> >    reg:
> > -    maxItems: 1
> > +    minItems: 1
> > +    maxItems: 2
> > 
> >    "#phy-cells":
> >      const: 0
> > 
> > @@ -32,6 +33,28 @@ properties:
> >      description:
> >        A phandle to the regulator for USB VBUS.
> > 
> > +  fsl,permanently-attached:
> > +    type: boolean
> > +    description:
> > +      Indicates if the device atached to a downstream port is
> > +      permanently attached.
> 
> Wouldn't just describing the downstream device be enough to indicate
> this? Though that is in the host controller rather than the phy.

You mean describing the downstream hub in device tree? I guess you are 
thinking about Documentation/devicetree/bindings/usb/usb-device.yaml, no?
I'll try using this.

This flag (and the others below) are used to set some specific flag in the 
host controller (not the PHY, see Li Jun's response). But I have to admit I do 
not know what they actually do. The description is pretty much everything 
written in the reference manual.

> > +
> > +  fsl,disable-port-power-control:
> > +    type: boolean
> > +    description:
> > +      Indicates whether the host controller implementation includes port
> > +      power control. Defines Bit 3 in capability register (HCCPARAMS).
> > +
> > +  fsl,over-current-active-low:
> > +    type: boolean
> > +    description:
> > +      Over current signal polarity is active low.
> > +
> > +  fsl,power-active-low:
> > +    type: boolean
> > +    description:
> > +      Power pad (PWR) polarity is active low.
> > +
> > 
> >  required:
> >    - compatible
> >    - reg
> > 
> > @@ -39,6 +62,33 @@ required:
> >    - clocks
> >    - clock-names
> > 
> > +if:
> > +  properties:
> > +    compatible:
> > +      contains:
> > +        enum:
> > +          - fsl,imx8mp-usb-phy
> > +
> > +then:
> > +  properties:
> > +    reg:
> > +      minItems: 2
> > +      maxItems: 2
> > +      items:
> > +        - description: PHY register base address
> > +        - description: Glue layer base address
> 
> Move 'items' to the top level and then here you only need 'minItems: 2'.
> 
> > +
> > +else:
> > +  properties:
> > +    reg:
> > +      maxItems: 1
> > +      items:
> > +        - description: PHY register base address
> 
> And just 'maxItems' here.

Thanks for the hints on how to write bindings, still fiddling with it.

Best regards,
Alexander