Hi Vivek, Kishon, > From: gautamvivek1987@xxxxxxxxx [mailto:gautamvivek1987@xxxxxxxxx] On > Behalf Of Vivek Gautam > Sent: Tuesday, May 13, 2014 12:08 PM > To: Marek Szyprowski > Cc: Kamil Debski; Linux USB Mailing List; r.baldyga@xxxxxxxxxxx > Subject: Re: [PATCH] usb: gadget: s3c-hsotg: fix phy disable sequence > > Hi, > > > On Tue, May 13, 2014 at 12:30 PM, Marek Szyprowski > <m.szyprowski@xxxxxxxxxxx> wrote: > > Hello, > > > > > > On 2014-05-13 08:21, Vivek Gautam wrote: > >> > >> On Mon, May 12, 2014 at 10:19 PM, Kamil Debski <k.debski@xxxxxxxxxxx> > >> wrote: > >> > When the driver is removed s3c_hsotg_phy_disable is called three > >> > times instead of once. This results in decreasing of the phy > >> > reference counter below zero and thus consecutive inserts of the > module fails. > >> > > >> > This patch removes calls to s3c_hsotg_phy_disable from > >> > s3c_hsotg_remove and s3c_hsotg_udc_stop. > >> > > >> > s3c_hsotg_udc_stop is called from udc-core.c only after > >> > usb_gadget_disconnect, which in turn calls s3c_hsotg_pullup, which > >> > already calls s3c_hsotg_phy_disable. > >> > > >> > s3c_hsotg_remove must be called only after udc_stop, so there is > no > >> > point in disabling phy once again there. > >> > > >> > Signed-off-by: Kamil Debski <k.debski@xxxxxxxxxxx> > >> > --- > >> > drivers/usb/gadget/s3c-hsotg.c | 3 --- > > Please rebase on 'usb-next'. This file is moved to dwc2. > "drivers/usb/dwc2/gadget.c" I will, thank you for pointing this out. > > >> > 1 file changed, 3 deletions(-) > >> > > >> > diff --git a/drivers/usb/gadget/s3c-hsotg.c > >> > b/drivers/usb/gadget/s3c-hsotg.c index 2a9cb67..29d70a7 100644 > >> > --- a/drivers/usb/gadget/s3c-hsotg.c > >> > +++ b/drivers/usb/gadget/s3c-hsotg.c > >> > @@ -3078,8 +3078,6 @@ static int s3c_hsotg_udc_stop(struct > >> > usb_gadget *gadget, > >> > > >> > spin_lock_irqsave(&hsotg->lock, flags); > >> > > >> > - s3c_hsotg_phy_disable(hsotg); > >> > - > >> > if (!driver) > >> > hsotg->driver = NULL; > >> > > >> > @@ -3766,7 +3764,6 @@ static int s3c_hsotg_remove(struct > >> > platform_device > >> > *pdev) > >> > usb_gadget_unregister_driver(hsotg->driver); > >> > } > >> > > >> > - s3c_hsotg_phy_disable(hsotg); > >> > if (hsotg->phy) > >> > phy_exit(hsotg->phy); > >> > >> We may want to remove above two lines too, isn't it ? > > > > > > Hmm, I think that more appropriate will be to remove calls to > > phy_init()/phy_exit() > > from s3c_hsotg_phy_enable()/s3c_hsotg_phy_disable() functions and > > leave > > phy_init() > > and phy_exit() only in s3c_hsotg_probe()/s3c_hsotg_remove() pair. > > Yea, that's one way. > But i was just wondering, do the phy_init() and phy_exit() callbacks > have any PHY's suspend/resume related settings. > In that case we would still want to suspend phy when the driver is > suspending. > > Although for phy-samsung-usb2 driver on exynos systems phy_inti() and > phy_exit() are just no-ops. > Kishon, could you clarify us on this issue? I interpret the PHY documentation such that phy_init/exit should be called on consumer driver probe/remove. But I understand Vivek's point of view. Best wishes, -- Kamil Debski Samsung R&D Institute Poland -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
