Messages in this thread Patch in this message | | | Date | Thu, 24 Jul 2008 13:14:50 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH] x86: usb debug port early console v3 |
| |
very nice feature!
The code structure looks good to me, here's a few minor style nits:
> + /* Now that we have observed the completed transaction, > + * clear the done bit. > + */
while i understand that this is cut & pasted code, please use standard comment style:
/* * Comment ... * ... line. */
(ditto the same mistake in other places too)
> + /* Read the result */ > + ret = dbgp_bulk_read(devnum, 0, data, size); > + return ret;
do:
return dbgp_bulk_read(devnum, 0, data, size);
> + if (!(read_pci_config_16(num, slot, func, PCI_STATUS) & > + PCI_STATUS_CAP_LIST)) > + return 0; > + pos = read_pci_config_byte(num, slot, func, PCI_CAPABILITY_LIST);
it's generally nicer to the eyes to add an extra newline after a block with return in it.
> + for (bytes = 0; bytes < 48 && pos >= 0x40; bytes++) { > + u8 id; > + pos &= ~3;
please put a newline between variable definitions and first statement.
> + > +static __u32 __init find_dbgp(int ehci_num, unsigned *rbus, unsigned *rslot, > + unsigned *rfunc) > +{ > + unsigned bus, slot, func; > + > + for (bus = 0; bus < 256; bus++) { > + for (slot = 0; slot < 32; slot++) { > + for (func = 0; func < 8; func++) { > + u32 class; > + unsigned cap; > + > + class = read_pci_config(bus, slot, func, > + PCI_CLASS_REVISION); > + if ((class >> 8) != PCI_CLASS_SERIAL_USB_EHCI) > + continue; > + cap = find_cap(bus, slot, func, > + PCI_CAP_ID_EHCI_DEBUG);
the line 80 breaks you had to add here show that the nesting is too deep here - i'd suggest a helper __find_dbgp() function to put the iterator into.
> + if ((ctrl & DBGP_CLAIM) != DBGP_CLAIM) { > + dbgp_printk("No device in debug port\n"); > + writel(ctrl & ~DBGP_CLAIM, &ehci_debug->control); > + return -1; > + > + }
stray newline.
> +static int __init early_dbgp_init(char *s) > +{ > + struct usb_debug_descriptor dbgp_desc; > + void __iomem *ehci_bar; > + unsigned ctrl, devnum; > + unsigned bus, slot, func, cap; > + unsigned debug_port, bar, offset; > + unsigned bar_val; > + char *e; > + int ret; > + unsigned dbgp_num;
use an explicit integer type please instead of 'unsigned'. Also, try to use reverse christmas-tree ordering for same-type entries (and where possible, between different types as well):
> + struct usb_debug_descriptor dbgp_desc; > + unsigned int debug_port, bar, offset; > + unsigned int bus, slot, func, cap; > + unsigned int ctrl, devnum; > + unsigned int dbgp_num; > + unsigned int bar_val; > + void __iomem *ehci_bar; > + char *e; > + int ret;
here:
> + dbgp_num = 0; > + if (*s) > + dbgp_num = simple_strtoul(s, &e, 10); > + dbgp_printk("dbgp_num: %d\n", dbgp_num); > + cap = find_dbgp(dbgp_num, &bus, &slot, &func); > + if (!cap) > + return -1; > + > + dbgp_printk("Found EHCI debug port\n");
i'd suggest a newline after the first dbgp_printk(), to make the two sections stand out better.
> + } > + > +
stray newline.
> + /* FIXME I don't have the bar size so just guess PAGE_SIZE is more > + * than enough. 1K is the biggest I have seen. > + */
comment style.
> + ret = ehci_setup(); > + if (ret < 0) { > + dbgp_printk("ehci_setup failed\n"); > + ehci_debug = 0; > + return -1;
please put newlines before return statements, to make sure there's a hickup in the visual flow during review. (which hickup return statements should cause, they must not be glossed over)
> + } > + > +
stray newline.
> Index: linux-2.6/drivers/usb/host/ehci.h > =================================================================== > --- linux-2.6.orig/drivers/usb/host/ehci.h > +++ linux-2.6/drivers/usb/host/ehci.h > @@ -210,146 +210,11 @@ timer_action (struct ehci_hcd *ehci, enu
i suggest you make this code movement a separate patch. In the unlikely event of there being any regression it's an easier bisection target.
looks good to me otherwise.
Ingo
| |