RE: [PATCH 2/4] devicetree/bindings: add support for CP110 UTMI driver

[Kostya Porotchkin]

Hi, Lubomir,

Thank you for your review!

> On Wed, Jan 27, 2021 at 01:27:17PM +0200, kostap@xxxxxxxxxxx wrote:
> > From: Konstantin Porotchkin <kostap@xxxxxxxxxxx>
> >
> > Add DTS binding for Marvell CP110 UTMI driver
> >
> > Signed-off-by: Konstantin Porotchkin <kostap@xxxxxxxxxxx>
> 
> Any chance you could convert the document to YAML so that it could be used
> for automatic validation?
> 
[KP] I believe it is possible, but probably should be done by a separate patch.
Here I am extending the existing documentation.

> > ---
> >  Documentation/devicetree/bindings/phy/phy-mvebu-utmi.txt | 69
> > ++++++++++++++++++--
> >  1 file changed, 63 insertions(+), 6 deletions(-)

...
> >  Required Properties:
> >
> >  - compatible: Should be one of:
> >  	      * "marvell,a3700-utmi-host-phy" for the PHY connected to
> > -	        the USB2 host-only controller.
> > +	        the USB2 host-only controller (for Armada3700 only).
> >  	      * "marvell,a3700-utmi-otg-phy" for the PHY connected to
> > -	        the USB3 and USB2 OTG capable controller.
> > +	        the USB3 and USB2 OTG capable controller (for Armada3700 only.
> > +	      * "marvell,cp110-utmi-phy" (for Armada 7k/8k or CN913x only).
> >  - reg: PHY IP register range.
> >  - marvell,usb-misc-reg: handle on the "USB miscellaneous registers" shared
> >  			region covering registers related to both the host
> > -			controller and the PHY.
> > -- #phy-cells: Standard property (Documentation: phy-bindings.txt) Should be
> 0.
> > +			controller and the PHY (for Armada3700 only).
> > +- marvell,system-controller: should contain a phandle to the system
> > +			     controller node (for Armada 7k/8k or CN913x only)
> 
> I guess this is okay, but referring to a syscon is done in a multitude ways across
> various other bindings; with the most popular being just:
> 
>   syscon = <&syscon>;
> 
> Perhaps consider doing that?
[KP] I was not sure that I can use a generic name inside the PHY entry 
if it is not defined as part of the generic PHY properties. 
I just did not see a good example of such in PHY bindings documentation.
If it is legal, I can change this entry name to just "syscon".
> 
> > +- #phy-cells: Standard property (Documentation: phy-bindings.txt.
> > +		Should be 0 (for Armada3700 only).
> > +
> > +
> > +Required properties (child nodes, for Armada 7k/8k/CN913x only):
> > +
> > +- reg: UTMI PHY port ID (0 or 1).
> > +- #phy-cells : Should be 0.
> > +
> > +
> > +Optional Properties (child nodes, for Armada 7k/8k/CN913x only)::
> >
> > +- marvell,cp110-utmi-device-mode: request the driver to connect the UTMI
> PHY
> > +				  port to USB device controller.
> 
> Do you need a separate property for this? Could the driver look at "dr_mode"
> property of the USB controller node to see if it's supposed to be in
> device/peripheral mode?
[KP] Yes, it seems I missed this option. I will try to change the code to support it in version 2.

> 
> >  Example:
> >
> > +Armada3700
> >  	usb2_utmi_host_phy: phy@5f000 {
> >  		compatible = "marvell,armada-3700-utmi-host-phy";
> >  		reg = <0x5f000 0x800>;
> > @@ -36,3 +67,29 @@ Example:
> > --
> > 2.17.1
> >
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> https://urldefense.proofpoint.com/v2/url?u=http-
> 3A__lists.infradead.org_mailman_listinfo_linux-2Darm-
> 2Dkernel&d=DwICAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=-
> N9sN4p5NSr0JGQoQ_2UCOgAqajG99W1EbSOww0WU8o&m=_ZOAKZShBT3Qj
> uT3RZIld2HoLnlvv6gkbHW9gSvEfI4&s=ggCBpvhDLJ8M6-
> Q41qbt8GRxryUo_mHxLMkUl8Ao5mA&e=