> -----Original Message----- > From: Felipe Balbi [mailto:balbi@xxxxxx] > Sent: Saturday, March 15, 2014 7:05 AM > To: Gupta Suresh-B42813 > Cc: balbi@xxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; > linux-kernel@xxxxxxxxxxxxxxx; Stefani Seibold > Subject: Re: [PATCH] USB: Gadget: fsl driver pullup fix > > Hi, > > (first of all, please fix your email client, we need the quotation marks. > See Documentation/email-clients.txt) > > On Fri, Mar 14, 2014 at 08:53:24PM +0000, suresh.gupta@xxxxxxxxxxxxx > wrote: > > > On Thu, Mar 13, 2014 at 06:40:55PM +0530, Suresh Gupta wrote: > > > > Attached is a small fix for the fsl usb gadget driver. This fix > > > > the driver in a way that the usb device will be only "pulled up" > > > > on requests like other usb gadget drivers do. > > > > This is necessary, because the device information is not always > > > > available until an application is up and running which provides > > > > this datas. > > > > > > > > Signed-off-by: Stefani Seibold <stefani@xxxxxxxxxxx> > > > > Signed-off-by: Suresh Gupta <suresh.gupta@xxxxxxxxxxxxx> > > > > --- > > > > drivers/usb/gadget/fsl_udc_core.c | 38 > > > > +++++++++++++++++++++----------------- > > > > 1 file changed, 21 insertions(+), 17 deletions(-) > > > > > > > > diff --git a/drivers/usb/gadget/fsl_udc_core.c > > > > b/drivers/usb/gadget/fsl_udc_core.c > > > > index 35cb972..9a93727 100644 > > > > --- a/drivers/usb/gadget/fsl_udc_core.c > > > > +++ b/drivers/usb/gadget/fsl_udc_core.c > > > > @@ -153,6 +153,21 @@ static inline void fsl_set_accessors(struct > > > > fsl_usb2_platform_data *pdata) {} > > > > > /******************************************************************** > > > > * Internal Used Function > > > > > > > > ****************************************************************** > > > > **/ > > > > +static int can_pullup(struct fsl_udc *udc) { > > > > + return udc->driver && udc->softconnect && udc->vbus_active; } > > > > + > > > > +static void set_pullup(struct fsl_udc *udc) { > > > > + if (can_pullup(udc)) > > > > + fsl_writel((fsl_readl(&dr_regs->usbcmd) | > USB_CMD_RUN_STOP), > > > > + &dr_regs->usbcmd); > > > > + else > > > > + fsl_writel((fsl_readl(&dr_regs->usbcmd) & > ~USB_CMD_RUN_STOP), > > > > + &dr_regs->usbcmd); > > > > +} > > > > > > why is this a "fix", you just re-factored some code into > set_pullup(). > > > > > [SuresH] I set udc->vbus_active and udc->softconnect to default value > > of 1 in struct_udc_setup. This was actual fix in this patch. The > > right, you see now why is it a problem to mix cleanups with fixes ? You > *never*, ever combine unrelated changes in a single patch. It makes it a > lot more difficult to see what you're actually changing. So, to start > with, this patch should (if it was correct) be split into two smaller > patches: one re-factoring the duplicated code into set_pullup() and > another which fixes vbus_active and softconnect flags. Agreed, I will resend the patches. > > But hang on, before you do that... > > > can_pullup function return false when these values was not set and > > intern the code return without enabling the pullup and gadget > > controller stops. > > So here's you mistake: the idea of can_pullup() (and thus, vbus_active > and softconnect flags) is to tell the driver "we're connet to a host, > it's safe to connect your pullups". > > When you set both those flags to true, you're telling the driver that > you, indeed, are connected to a host. This might not be true if you first > boot up your platform, load all drivers and only after connect the cable. > In essence, you're lying to your driver and, as my mommy used to say, > "nobody likes a liar, my boy". > > Curret situation isn't very good either since the driver is assuming that > cable is only plugged after driver is loaded, so it won't cope very well > with situation where cable is first plugged, then you apply power to your > board. > > What you *really* need to do here is ask the HW for initial states of > those flags. During your probe() routine - as the name says - you probe > your HW to check its state (or to initialize its state), then you ask > "Hey IP, is VBUS above session valid threshold ?" Then you use the HW's > reply to initialize both flags, the way you want. After your explanation and some code browsing, I think the exact place to set vbus_active is fsl_vbus_session which called on detecting vbus. Also fsl_pullup should return without doing anything if vbus_active is not set. Please suggest. I do not get the usage of softconnect. Do I set softconnect also in fsl_vbus_session. Please suggest. Thanks SuresH -- 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