> -----Original Message-----
> From: Simon Goldschmidt <[email protected]>
> Sent: mercredi 4 mars 2020 20:52
> To: Patrick DELAUNAY <[email protected]>; [email protected]
> Cc: [email protected]; [email protected]; Daniel Schwierzeck
> <[email protected]>; Marek Vasut <[email protected]>; Michal
> Suchanek <[email protected]>; Simon Glass <[email protected]>; U-Boot
> STM32 <[email protected]>
> Subject: Re: [PATCH v4 2/5] usb: host: dwc2: add phy support
> Importance: High
>
> Am 18.02.2020 um 09:35 schrieb Patrick Delaunay:
> > Use generic phy to initialize the PHY associated to the
> > DWC2 device and available in the device tree.
> >
> > This patch don't added dependency because when CONFIG_PHY is not
> > activated, the generic PHY function are stubbed.
> >
> > Signed-off-by: Patrick Delaunay <[email protected]>
> > ---
> >
> > Changes in v4: None
> > Changes in v3: None
> > Changes in v2:
> > - update dev_err
> > - update commit message
> > - change dev_err to dev_dbg for PHY function call
> > - treat dwc2_shutdown_phy error
> >
> > drivers/usb/host/dwc2.c | 66
> > +++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 66 insertions(+)
> >
> > diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c index
> > e4efaf1e59..5e7ffaddd9 100644
> > --- a/drivers/usb/host/dwc2.c
> > +++ b/drivers/usb/host/dwc2.c
> > @@ -8,6 +8,7 @@
> > #include <cpu_func.h>
> > #include <dm.h>
> > #include <errno.h>
> > +#include <generic-phy.h>
> > #include <usb.h>
> > #include <malloc.h>
> > #include <memalign.h>
> > @@ -37,6 +38,7 @@ struct dwc2_priv {
> > #ifdef CONFIG_DM_REGULATOR
> > struct udevice *vbus_supply;
> > #endif
> > + struct phy phy;
> > #else
> > uint8_t *aligned_buffer;
> > uint8_t *status_buffer;
> > @@ -1322,13 +1324,71 @@ static int dwc2_usb_ofdata_to_platdata(struct
> udevice *dev)
> > return 0;
> > }
> >
> > +static int dwc2_setup_phy(struct udevice *dev) {
> > + struct dwc2_priv *priv = dev_get_priv(dev);
> > + int ret;
> > +
> > + ret = generic_phy_get_by_index(dev, 0, &priv->phy);
> > + if (ret) {
> > + if (ret != -ENOENT) {
>
> Could you invert this logic and add a comment like "no PHY" or something?
Yes in V5, it is more clear
ret = generic_phy_get_by_index(dev, 0, &priv->phy);
if (ret) {
if (ret == -ENOENT)
return 0; /* no PHY, nothing to do */
dev_err(dev, "Failed to get USB PHY: %d.\n", ret);
return ret;
}
> > + dev_err(dev, "Failed to get USB PHY: %d.\n", ret);
> > + return ret;
> > + }
> > + return 0;
> > + }
> > +
> > + ret = generic_phy_init(&priv->phy);
> > + if (ret) {
> > + dev_dbg(dev, "Failed to init USB PHY: %d.\n", ret);
> > + return ret;
> > + }
> > +
> > + ret = generic_phy_power_on(&priv->phy);
> > + if (ret) {
> > + dev_dbg(dev, "Failed to power on USB PHY: %d.\n", ret);
> > + generic_phy_exit(&priv->phy);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int dwc2_shutdown_phy(struct udevice *dev) {
> > + struct dwc2_priv *priv = dev_get_priv(dev);
> > + int ret;
> > +
> > + if (!generic_phy_valid(&priv->phy))
>
> A comment saying that this is for platforms without a phy driver would be
> nice.
Yes in V5:
static int dwc2_shutdown_phy(struct udevice *dev)
{
struct dwc2_priv *priv = dev_get_priv(dev);
int ret;
/* PHY is not valid when generic_phy_get_by_index() = -ENOENT */
if (!generic_phy_valid(&priv->phy))
return 0; /* no PHY, nothing to do */
> Other than that:
> Reviewed-by: Simon Goldschmidt <[email protected]>
Thanks
Regards
Patrick
