On Tue, Apr 10, 2018 at 09:44:31AM +0000, Yoshihiro Shimoda wrote: > Hi Simon-san, > > > From: Simon Horman, Sent: Tuesday, April 10, 2018 6:34 PM > > > > On Tue, Apr 10, 2018 at 01:28:33AM +0000, Yoshihiro Shimoda wrote: > > > Hi Simon-san, > > > > > > > From: Simon Horman, Sent: Monday, April 9, 2018 8:58 PM > > > > > > > > On Mon, Apr 02, 2018 at 09:21:34PM +0900, Yoshihiro Shimoda wrote: > > > > > This patch fixes an issue that this driver cannot call phy_init() > > > > > if a gadget driver is alreadly loaded because usb_add_gadget_udc() > > > > > might call renesas_usb3_start() via .udc_start. > > > > > > > > > > Fixes: 279d4bc64060 ("usb: gadget: udc: renesas_usb3: add support for generic phy") > > > > > Cc: <stable@xxxxxxxxxxxxxxx> # v4.15+ > > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx> > > > > > --- > > > > > drivers/usb/gadget/udc/renesas_usb3.c | 16 ++++++++-------- > > > > > 1 file changed, 8 insertions(+), 8 deletions(-) > > > > > > > > Reviewed-by: Simon Horman <horms+renesas@xxxxxxxxxxxx> > > > > > > > > > > > > Please see some suggestions for follow-up lower-priority patches below. > > > > > > Thank you for the review and suggestions! > > > > > > > > > > > > > diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c > > > > > index 738b734..671bac3 100644 > > > > > --- a/drivers/usb/gadget/udc/renesas_usb3.c > > > > > +++ b/drivers/usb/gadget/udc/renesas_usb3.c > > > > > @@ -2632,6 +2632,14 @@ static int renesas_usb3_probe(struct platform_device *pdev) > > > > > if (ret < 0) > > > > > goto err_alloc_prd; > > > > > > > > > > + /* > > > > > + * This is an optional. So, if this driver cannot get a phy, > > > > > + * this driver will not handle a phy anymore. > > > > > + */ > > > > > > > > nit: s/an optional/optional/ > > > > > > Oops. I will fix and submit v2 patches. > > > > > > > > + usb3->phy = devm_phy_get(&pdev->dev, "usb"); > > > > > + if (IS_ERR(usb3->phy)) > > > > > + usb3->phy = NULL; > > > > > > > > I think this will unintentionally ignore errors other than the > > > > non-existence of the device, f.e. a memory allocation failure > > > > in devm_phy_get(). > > > > > > > > So perhaps something like this, as per phy_optional_get(), would be better? > > > > > > > > usb3->phy = devm_phy_get(&pdev->dev, "usb"); > > > > if (IS_ERR(usb3->phy) && (PTR_ERR(usb3->phy) == -ENODEV)) > > > > usb3->phy = NULL; > > > > > > Thank you for the suggestion. I agree with you. > > > I think I should use devm_phy_optional_get() instead of dev_phy_get(), as you mentioned :) > > > So, I will fix the code by using devm_phy_optional_get() first, and then fix this. > > > > Thanks, I agree that would be a good fix. > > > > But perhaps it is better as a follow-up? > > Oops. Yes, I changed my mind after prepared the patch v2 and submitted v2. > I should have sent an email about this to you... > Anyway, thank you very much for reviewing the v2 patches! v2 looks good to me :)