Re: [PATCH 53/82] usb: dwc3: ep0: simplify dwc3_ep0_handle_feature()
Hi, John Younwrites: >> +static int dwc3_ep0_handle_device(struct dwc3 *dwc, >> struct usb_ctrlrequest *ctrl, int set) >> { >> -struct dwc3_ep *dep; >> -u32 recip; >> +enum usb_device_state state; >> u32 wValue; >> u32 wIndex; >> -u32 reg; >> -int ret; >> -enum usb_device_state state; >> +int ret = 0; >> >> wValue = le16_to_cpu(ctrl->wValue); >> wIndex = le16_to_cpu(ctrl->wIndex); >> -recip = ctrl->bRequestType & USB_RECIP_MASK; >> state = dwc->gadget.state; >> >> -switch (recip) { >> -case USB_RECIP_DEVICE: >> +switch (wValue) { >> +case USB_DEVICE_REMOTE_WAKEUP: >> +break; >> +/* >> + * 9.4.1 says only only for SS, in AddressState only for >> + * default control pipe >> + */ >> +case USB_DEVICE_U1_ENABLE: >> +ret = dwc3_ep0_handle_u1(dwc, state, set); >> +break; >> +case USB_DEVICE_U2_ENABLE: >> +ret = dwc3_ep0_handle_u2(dwc, state, set); >> +break; >> +case USB_DEVICE_LTM_ENABLE: >> +ret = -EINVAL; >> +break; >> +case USB_DEVICE_TEST_MODE: >> +ret = dwc3_ep0_handle_test(dwc, state, wIndex, set); > > Need a break here. > > Found with coverity. nice!! :-) thanks for letting me know. Here's a new version: 8<-- From a9f78f9dcb6698bee03ec9a8eb0c73f08f49c2ee Mon Sep 17 00:00:00 2001 From: Felipe Balbi Date: Mon, 3 Oct 2016 12:55:29 +0300 Subject: [PATCH] usb: dwc3: ep0: simplify dwc3_ep0_handle_feature() By extracting smaller functions from dwc3_ep0_handle_feature(), it becomes far easier to understand what's going on. Cleanup only, no functional changes. Signed-off-by: Felipe Balbi --- drivers/usb/dwc3/ep0.c | 257 +++-- 1 file changed, 164 insertions(+), 93 deletions(-) diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c index c562613ccd1a..a1f7c2b4b000 100644 --- a/drivers/usb/dwc3/ep0.c +++ b/drivers/usb/dwc3/ep0.c @@ -371,126 +371,197 @@ static int dwc3_ep0_handle_status(struct dwc3 *dwc, return __dwc3_gadget_ep0_queue(dep, >ep0_usb_req); } -static int dwc3_ep0_handle_feature(struct dwc3 *dwc, +static int dwc3_ep0_handle_u1(struct dwc3 *dwc, enum usb_device_state state, + int set) +{ + u32 reg; + + if (state != USB_STATE_CONFIGURED) + return -EINVAL; + if ((dwc->speed != DWC3_DSTS_SUPERSPEED) && + (dwc->speed != DWC3_DSTS_SUPERSPEED_PLUS)) + return -EINVAL; + + reg = dwc3_readl(dwc->regs, DWC3_DCTL); + if (set) + reg |= DWC3_DCTL_INITU1ENA; + else + reg &= ~DWC3_DCTL_INITU1ENA; + dwc3_writel(dwc->regs, DWC3_DCTL, reg); + + return 0; +} + +static int dwc3_ep0_handle_u2(struct dwc3 *dwc, enum usb_device_state state, + int set) +{ + u32 reg; + + + if (state != USB_STATE_CONFIGURED) + return -EINVAL; + if ((dwc->speed != DWC3_DSTS_SUPERSPEED) && + (dwc->speed != DWC3_DSTS_SUPERSPEED_PLUS)) + return -EINVAL; + + reg = dwc3_readl(dwc->regs, DWC3_DCTL); + if (set) + reg |= DWC3_DCTL_INITU2ENA; + else + reg &= ~DWC3_DCTL_INITU2ENA; + dwc3_writel(dwc->regs, DWC3_DCTL, reg); + + return 0; +} + +static int dwc3_ep0_handle_test(struct dwc3 *dwc, enum usb_device_state state, + u32 wIndex, int set) +{ + if ((wIndex & 0xff) != 0) + return -EINVAL; + if (!set) + return -EINVAL; + + switch (wIndex >> 8) { + case TEST_J: + case TEST_K: + case TEST_SE0_NAK: + case TEST_PACKET: + case TEST_FORCE_EN: + dwc->test_mode_nr = wIndex >> 8; + dwc->test_mode = true; + break; + default: + return -EINVAL; + } + + return 0; +} + +static int dwc3_ep0_handle_device(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl, int set) { - struct dwc3_ep *dep; - u32 recip; + enum usb_device_state state; u32 wValue; u32 wIndex; - u32 reg; - int ret; - enum usb_device_state state; + int ret = 0; wValue = le16_to_cpu(ctrl->wValue); wIndex = le16_to_cpu(ctrl->wIndex); - recip = ctrl->bRequestType & USB_RECIP_MASK; state = dwc->gadget.state; - switch (recip) { - case USB_RECIP_DEVICE: +
Re: [PATCH 53/82] usb: dwc3: ep0: simplify dwc3_ep0_handle_feature()
On 10/31/2016 3:51 AM, Felipe Balbi wrote: > By extracting smaller functions from > dwc3_ep0_handle_feature(), it becomes far easier to > understand what's going on. Cleanup only, no > functional changes. > > Signed-off-by: Felipe Balbi> --- > drivers/usb/dwc3/ep0.c | 256 > +++-- > 1 file changed, 163 insertions(+), 93 deletions(-) > > diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c > index c562613ccd1a..5e642d65a3b2 100644 > --- a/drivers/usb/dwc3/ep0.c > +++ b/drivers/usb/dwc3/ep0.c > @@ -371,126 +371,196 @@ static int dwc3_ep0_handle_status(struct dwc3 *dwc, > return __dwc3_gadget_ep0_queue(dep, >ep0_usb_req); > } > > -static int dwc3_ep0_handle_feature(struct dwc3 *dwc, > +static int dwc3_ep0_handle_u1(struct dwc3 *dwc, enum usb_device_state state, > + int set) > +{ > + u32 reg; > + > + if (state != USB_STATE_CONFIGURED) > + return -EINVAL; > + if ((dwc->speed != DWC3_DSTS_SUPERSPEED) && > + (dwc->speed != DWC3_DSTS_SUPERSPEED_PLUS)) > + return -EINVAL; > + > + reg = dwc3_readl(dwc->regs, DWC3_DCTL); > + if (set) > + reg |= DWC3_DCTL_INITU1ENA; > + else > + reg &= ~DWC3_DCTL_INITU1ENA; > + dwc3_writel(dwc->regs, DWC3_DCTL, reg); > + > + return 0; > +} > + > +static int dwc3_ep0_handle_u2(struct dwc3 *dwc, enum usb_device_state state, > + int set) > +{ > + u32 reg; > + > + > + if (state != USB_STATE_CONFIGURED) > + return -EINVAL; > + if ((dwc->speed != DWC3_DSTS_SUPERSPEED) && > + (dwc->speed != DWC3_DSTS_SUPERSPEED_PLUS)) > + return -EINVAL; > + > + reg = dwc3_readl(dwc->regs, DWC3_DCTL); > + if (set) > + reg |= DWC3_DCTL_INITU2ENA; > + else > + reg &= ~DWC3_DCTL_INITU2ENA; > + dwc3_writel(dwc->regs, DWC3_DCTL, reg); > + > + return 0; > +} > + > +static int dwc3_ep0_handle_test(struct dwc3 *dwc, enum usb_device_state > state, > + u32 wIndex, int set) > +{ > + if ((wIndex & 0xff) != 0) > + return -EINVAL; > + if (!set) > + return -EINVAL; > + > + switch (wIndex >> 8) { > + case TEST_J: > + case TEST_K: > + case TEST_SE0_NAK: > + case TEST_PACKET: > + case TEST_FORCE_EN: > + dwc->test_mode_nr = wIndex >> 8; > + dwc->test_mode = true; > + break; > + default: > + return -EINVAL; > + } > + > + return 0; > +} > + > +static int dwc3_ep0_handle_device(struct dwc3 *dwc, > struct usb_ctrlrequest *ctrl, int set) > { > - struct dwc3_ep *dep; > - u32 recip; > + enum usb_device_state state; > u32 wValue; > u32 wIndex; > - u32 reg; > - int ret; > - enum usb_device_state state; > + int ret = 0; > > wValue = le16_to_cpu(ctrl->wValue); > wIndex = le16_to_cpu(ctrl->wIndex); > - recip = ctrl->bRequestType & USB_RECIP_MASK; > state = dwc->gadget.state; > > - switch (recip) { > - case USB_RECIP_DEVICE: > + switch (wValue) { > + case USB_DEVICE_REMOTE_WAKEUP: > + break; > + /* > + * 9.4.1 says only only for SS, in AddressState only for > + * default control pipe > + */ > + case USB_DEVICE_U1_ENABLE: > + ret = dwc3_ep0_handle_u1(dwc, state, set); > + break; > + case USB_DEVICE_U2_ENABLE: > + ret = dwc3_ep0_handle_u2(dwc, state, set); > + break; > + case USB_DEVICE_LTM_ENABLE: > + ret = -EINVAL; > + break; > + case USB_DEVICE_TEST_MODE: > + ret = dwc3_ep0_handle_test(dwc, state, wIndex, set); Need a break here. Found with coverity. Regards, John > + default: > + ret = -EINVAL; > + } > > - switch (wValue) { > - case USB_DEVICE_REMOTE_WAKEUP: > - break; > - /* > - * 9.4.1 says only only for SS, in AddressState only for > - * default control pipe > - */ > - case USB_DEVICE_U1_ENABLE: > - if (state != USB_STATE_CONFIGURED) > - return -EINVAL; > - if ((dwc->speed != DWC3_DSTS_SUPERSPEED) && > - (dwc->speed != DWC3_DSTS_SUPERSPEED_PLUS)) > - return -EINVAL; > + return ret; > +} > > - reg = dwc3_readl(dwc->regs, DWC3_DCTL); > - if (set) > - reg |= DWC3_DCTL_INITU1ENA; > - else > - reg &= ~DWC3_DCTL_INITU1ENA; > - dwc3_writel(dwc->regs,
[PATCH 53/82] usb: dwc3: ep0: simplify dwc3_ep0_handle_feature()
By extracting smaller functions from dwc3_ep0_handle_feature(), it becomes far easier to understand what's going on. Cleanup only, no functional changes. Signed-off-by: Felipe Balbi--- drivers/usb/dwc3/ep0.c | 256 +++-- 1 file changed, 163 insertions(+), 93 deletions(-) diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c index c562613ccd1a..5e642d65a3b2 100644 --- a/drivers/usb/dwc3/ep0.c +++ b/drivers/usb/dwc3/ep0.c @@ -371,126 +371,196 @@ static int dwc3_ep0_handle_status(struct dwc3 *dwc, return __dwc3_gadget_ep0_queue(dep, >ep0_usb_req); } -static int dwc3_ep0_handle_feature(struct dwc3 *dwc, +static int dwc3_ep0_handle_u1(struct dwc3 *dwc, enum usb_device_state state, + int set) +{ + u32 reg; + + if (state != USB_STATE_CONFIGURED) + return -EINVAL; + if ((dwc->speed != DWC3_DSTS_SUPERSPEED) && + (dwc->speed != DWC3_DSTS_SUPERSPEED_PLUS)) + return -EINVAL; + + reg = dwc3_readl(dwc->regs, DWC3_DCTL); + if (set) + reg |= DWC3_DCTL_INITU1ENA; + else + reg &= ~DWC3_DCTL_INITU1ENA; + dwc3_writel(dwc->regs, DWC3_DCTL, reg); + + return 0; +} + +static int dwc3_ep0_handle_u2(struct dwc3 *dwc, enum usb_device_state state, + int set) +{ + u32 reg; + + + if (state != USB_STATE_CONFIGURED) + return -EINVAL; + if ((dwc->speed != DWC3_DSTS_SUPERSPEED) && + (dwc->speed != DWC3_DSTS_SUPERSPEED_PLUS)) + return -EINVAL; + + reg = dwc3_readl(dwc->regs, DWC3_DCTL); + if (set) + reg |= DWC3_DCTL_INITU2ENA; + else + reg &= ~DWC3_DCTL_INITU2ENA; + dwc3_writel(dwc->regs, DWC3_DCTL, reg); + + return 0; +} + +static int dwc3_ep0_handle_test(struct dwc3 *dwc, enum usb_device_state state, + u32 wIndex, int set) +{ + if ((wIndex & 0xff) != 0) + return -EINVAL; + if (!set) + return -EINVAL; + + switch (wIndex >> 8) { + case TEST_J: + case TEST_K: + case TEST_SE0_NAK: + case TEST_PACKET: + case TEST_FORCE_EN: + dwc->test_mode_nr = wIndex >> 8; + dwc->test_mode = true; + break; + default: + return -EINVAL; + } + + return 0; +} + +static int dwc3_ep0_handle_device(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl, int set) { - struct dwc3_ep *dep; - u32 recip; + enum usb_device_state state; u32 wValue; u32 wIndex; - u32 reg; - int ret; - enum usb_device_state state; + int ret = 0; wValue = le16_to_cpu(ctrl->wValue); wIndex = le16_to_cpu(ctrl->wIndex); - recip = ctrl->bRequestType & USB_RECIP_MASK; state = dwc->gadget.state; - switch (recip) { - case USB_RECIP_DEVICE: + switch (wValue) { + case USB_DEVICE_REMOTE_WAKEUP: + break; + /* +* 9.4.1 says only only for SS, in AddressState only for +* default control pipe +*/ + case USB_DEVICE_U1_ENABLE: + ret = dwc3_ep0_handle_u1(dwc, state, set); + break; + case USB_DEVICE_U2_ENABLE: + ret = dwc3_ep0_handle_u2(dwc, state, set); + break; + case USB_DEVICE_LTM_ENABLE: + ret = -EINVAL; + break; + case USB_DEVICE_TEST_MODE: + ret = dwc3_ep0_handle_test(dwc, state, wIndex, set); + default: + ret = -EINVAL; + } - switch (wValue) { - case USB_DEVICE_REMOTE_WAKEUP: - break; - /* -* 9.4.1 says only only for SS, in AddressState only for -* default control pipe -*/ - case USB_DEVICE_U1_ENABLE: - if (state != USB_STATE_CONFIGURED) - return -EINVAL; - if ((dwc->speed != DWC3_DSTS_SUPERSPEED) && - (dwc->speed != DWC3_DSTS_SUPERSPEED_PLUS)) - return -EINVAL; + return ret; +} - reg = dwc3_readl(dwc->regs, DWC3_DCTL); - if (set) - reg |= DWC3_DCTL_INITU1ENA; - else - reg &= ~DWC3_DCTL_INITU1ENA; - dwc3_writel(dwc->regs, DWC3_DCTL, reg); - break; +static int dwc3_ep0_handle_intf(struct dwc3 *dwc, + struct usb_ctrlrequest *ctrl, int set) +{ + enum usb_device_state state; +