On Wed, Mar 19, 2014 at 02:23:59PM +0000, suresh.gupta@xxxxxxxxxxxxx wrote: > > > > -----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. looks like softconnect should be set when pullups are connected. -- balbi
Attachment:
signature.asc
Description: Digital signature
