Re: [PATCH] usb: dwc2: Implement set_selfpowered()

From: John Keeping <john@metanate.com>
To: Minas Harutyunyan <Minas.Harutyunyan@synopsys.com>
Cc: Felipe Balbi <balbi@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	<linux-usb@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] usb: dwc2: Implement set_selfpowered()
Date: Wed, 5 Feb 2020 11:56:56 +0000	[thread overview]
Message-ID: <20200205115656.3c698385.john@metanate.com> (raw)
In-Reply-To: <34b74e48-a3ea-f68f-540e-121ae98afb31@synopsys.com>

Hi Minas,

On Wed, 5 Feb 2020 07:59:48 +0000
Minas Harutyunyan <Minas.Harutyunyan@synopsys.com> wrote:

> On 2/4/2020 7:29 PM, John Keeping wrote:
> > dwc2 always reports as self-powered in response to a device status
> > request.  Implement the set_selfpowered() operations so that the gadget
> > can report as bus-powered when appropriate.
> > 
> > This is modelled on the dwc3 implementation.
> > 
> > Signed-off-by: John Keeping <john@metanate.com>  
> 
> Good catch. Just one concern. Your patch partially fix my patch this is 
> why I think you need to add Fixes tag otherwise it can create merge 
> conflict if your patch will be merged to next earlier than my.

I don't think this is actually a fix for your patch, it's a separate fix
which happens to touch the same code.

Since dwc2 has never supported the set_selfpowered() operation, I'm not
really sure if this counts as a bugfix or a feature.

I'm happy to re-send with a fixes tag if you think it's necessary, but I
don't think it's accurate in this case - your patch did not introduce a
bug here :-)

Regards,
John

> > ---
> >   drivers/usb/dwc2/gadget.c | 24 +++++++++++++++++++++++-
> >   1 file changed, 23 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> > index 2717f4401b97..76c0a5242175 100644
> > --- a/drivers/usb/dwc2/gadget.c
> > +++ b/drivers/usb/dwc2/gadget.c
> > @@ -1646,7 +1646,8 @@ static int dwc2_hsotg_process_req_status(struct dwc2_hsotg *hsotg,
> >   
> >   	switch (ctrl->bRequestType & USB_RECIP_MASK) {
> >   	case USB_RECIP_DEVICE:
> > -		status = 1 << USB_DEVICE_SELF_POWERED;
> > +		status = hsotg->gadget.is_selfpowered <<
> > +			 USB_DEVICE_SELF_POWERED;
> >   		status |= hsotg->remote_wakeup_allowed <<
> >   			  USB_DEVICE_REMOTE_WAKEUP;
> >   		reply = cpu_to_le16(status);
> > @@ -4530,6 +4531,26 @@ static int dwc2_hsotg_gadget_getframe(struct usb_gadget *gadget)
> >   	return dwc2_hsotg_read_frameno(to_hsotg(gadget));
> >   }
> >   
> > +/**
> > + * dwc2_hsotg_set_selfpowered - set if device is self/bus powered
> > + * @gadget: The usb gadget state
> > + * @is_selfpowered: Whether the device is self-powered
> > + *
> > + * Set if the device is self or bus powered.
> > + */
> > +static int dwc2_hsotg_set_selfpowered(struct usb_gadget *gadget,
> > +				      int is_selfpowered)
> > +{
> > +	struct dwc2_hsotg *hsotg = to_hsotg(gadget);
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&hsotg->lock, flags);
> > +	gadget->is_selfpowered = !!is_selfpowered;
> > +	spin_unlock_irqrestore(&hsotg->lock, flags);
> > +
> > +	return 0;
> > +}
> > +
> >   /**
> >    * dwc2_hsotg_pullup - connect/disconnect the USB PHY
> >    * @gadget: The usb gadget state
> > @@ -4621,6 +4642,7 @@ static int dwc2_hsotg_vbus_draw(struct usb_gadget *gadget, unsigned int mA)
> >   
> >   static const struct usb_gadget_ops dwc2_hsotg_gadget_ops = {
> >   	.get_frame	= dwc2_hsotg_gadget_getframe,
> > +	.set_selfpowered	= dwc2_hsotg_set_selfpowered,
> >   	.udc_start		= dwc2_hsotg_udc_start,
> >   	.udc_stop		= dwc2_hsotg_udc_stop,
> >   	.pullup                 = dwc2_hsotg_pullup,
> >   