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

Alexander Stein
2022-01-07T13:51:12Z

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@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