Hi!
This is the USB host controller used on the Altera SoCFPGA and Raspbery Pi.
This code has three checkpatch warnings, but to make sure it stays at least readable and clear, these are not fixed. These bugs are in the USB request handling combinatorial logic, so any abstracting of those is out of question.
Not too bad. Minor comments below.
index c4f5157..c9d2ed5 100644 --- a/drivers/usb/host/Makefile +++ b/drivers/usb/host/Makefile @@ -45,3 +45,6 @@ obj-$(CONFIG_USB_EHCI_ZYNQ) += ehci-zynq.o obj-$(CONFIG_USB_XHCI) += xhci.o xhci-mem.o xhci-ring.o obj-$(CONFIG_USB_XHCI_EXYNOS) += xhci-exynos5.o obj-$(CONFIG_USB_XHCI_OMAP) += xhci-omap.o
+# designware +obj-$(CONFIG_USB_DWC2) += dwc2.o
Should this be sorted somehow?
+/**
- Initializes the FSLSPClkSel field of the HCFG register
- depending on the PHY type.
- */
+static void init_fslspclksel(struct dwc2_core_regs *regs) +{
- uint32_t phyclk;
+#ifdef CONFIG_DWC2_ULPI_FS_LS
Having more readable names for config options would be nice.
- uint32_t hwcfg2 = readl(®s->ghwcfg2);
- uint32_t hval = (ghwcfg2 & DWC2_HWCFG2_HS_PHY_TYPE_MASK) >>
DWC2_HWCFG2_HS_PHY_TYPE_OFFSET;- uint32_t fval = (ghwcfg2 & DWC2_HWCFG2_FS_PHY_TYPE_MASK) >>
DWC2_HWCFG2_FS_PHY_TYPE_OFFSET;- if ((hval == 2) && (fval == 1))
phyclk = DWC2_HCFG_FSLSPCLKSEL_48_MHZ; /* Full speed PHY */- else
+#endif
Ifdef ending in "else" is evil.
- /* Wait for 3 PHY Clocks */
- udelay(1);
Note this.
+/**
- Do core a soft reset of the core. Be careful with this because it
- resets all the internal state machines of the core.
- */
This is not valid kerneldoc -> should not use /** style.
- /* Wait for 3 PHY Clocks */
- mdelay(100);
Recall it. There's big difference between 1usec and 100msec, which is it?
- /* Program the HCSPLIT register for SPLITs */
- writel(0, &hc_regs->hcsplt);
+}
This is little un-conventional :-).
- if (usb_pipeint(pipe)) {
puts("Root-Hub submit IRQ: NOT implemented");
Missing \n?
- wValue = cpu_to_le16 (cmd->value);
- wLength = cpu_to_le16 (cmd->length);
Extra space.
- switch (bmRType_bReq) {
- case (USB_REQ_GET_STATUS << 8) | USB_DIR_IN:
*(__u16 *)buffer = cpu_to_le16(1);
Can we use u16 (not __u16) here?
- case (USB_REQ_SET_FEATURE << 8) | USB_DIR_OUT | USB_RECIP_OTHER | USB_TYPE_CLASS:
switch (wValue) {case USB_PORT_FEAT_SUSPEND:break;case USB_PORT_FEAT_RESET:clrsetbits_le32(®s->hprt0, DWC2_HPRT0_PRTENA |DWC2_HPRT0_PRTCONNDET |DWC2_HPRT0_PRTENCHNG |DWC2_HPRT0_PRTOVRCURRCHNG,DWC2_HPRT0_PRTRST);mdelay(50);clrbits_le32(®s->hprt0, DWC2_HPRT0_PRTRST);break;case USB_PORT_FEAT_POWER:clrsetbits_le32(®s->hprt0, DWC2_HPRT0_PRTENA |DWC2_HPRT0_PRTCONNDET |DWC2_HPRT0_PRTENCHNG |DWC2_HPRT0_PRTOVRCURRCHNG,DWC2_HPRT0_PRTRST);break;case USB_PORT_FEAT_ENABLE:break;}break;
This is getting bit. Would it be feasible to split it into functions?
- case (USB_REQ_GET_DESCRIPTOR << 8) | USB_DIR_IN | USB_TYPE_CLASS:
- {
__u32 temp = 0x00000001;
temp is never writtenn to. Can you just write 1 instead of it?
data[0] = 9; /* min length; */data[1] = 0x29;data[2] = temp & RH_A_NDP;data[3] = 0;if (temp & RH_A_PSM)data[3] |= 0x1;if (temp & RH_A_NOCP)data[3] |= 0x10;else if (temp & RH_A_OCPM)data[3] |= 0x8;/* corresponds to data[4-7] */data[5] = (temp & RH_A_POTPGT) >> 24;data[7] = temp & RH_B_DR;if (data[2] < 7) {data[8] = 0xff;} else {
Thus data[2] is always <2. What is going on here?
- default:
puts("unsupported root hub command");
\n?
/* TODO: no endless loop */while (1) {hcint_new = readl(&hc_regs->hcint);if (hcint != hcint_new)hcint = hcint_new;
What is going on here?
Move loop into function to keep complexity down?
- writel((uint32_t)status_buffer, &hc_regs->hcdma);
Can we get rid of the cast?
+#define DWC2_HWCFG1_EP_DIR6_MASK (0x3 << 12) +#define DWC2_HWCFG1_EP_DIR6_OFFSET 12
Quick grep shows this is unused. And all of these come in mask+offset variety. Can we get rid of some?
Best regards, Pavel