Messages in this thread | | | Date | Sat, 10 Sep 2005 16:53:56 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH -mm 1/2] drivers/char/isicom old api rewritten |
| |
Jiri Slaby <jirislaby@gmail.com> wrote: > > This patch removes old api in pci probing and replaces it with new. > Firmware loading added. > > Signed-off-by: Jiri Slaby <xslaby@fi.muni.cz> > > drivers/char/isicom.c | 1056 > ++++++++++++++++++++++++------------------------- > include/linux/isicom.h | 54 -- > 2 files changed, 539 insertions(+), 571 deletions(-) > > Patch is here for its size (40 KiB): > http://www.fi.muni.cz/~xslaby/lnx/isi_main.txt
40k isn't large - please include such patches in the email.
> +sched_again: > + if (!re_schedule) > + {
coding style.
> +static void unregister_tty_driver(void) > +static int register_isr(struct isi_board* board, const int index)
Not a great choice of function names, IMO. Nicer to have "isicom" in the name. Your call.
> + > + for(t = jiffies + HZ/100; time_before(jiffies, t); ) > + ;
mdelay()
> + outw(0, base + 0x8); /* Reset */ > + > + for(j = 1; j <= 3; j++) { > + for(t = jiffies + HZ; time_before(jiffies, t); ) > + ;
mdelay()
> + > + switch (signature) { > + case 0xa5: > + name = "isi608.bin"; > + break;
We usually do this:
switch (...) { case N:
> + for(i = 0; i <= 0x2f; i++) /* a wee bit of delay */ > + ;
eep. udelay()
> + for(i = 0; i <= 0x0f; i++) /* another wee bit of delay */ > + ;
more eep.
> +/* XXX: should I test it by reading it back and comparing with original like > + * in load firmware package? */ > +#if 0
hrm
> + case MIOCTL_READ_FIRMWARE: > + if (!capable(CAP_SYS_ADMIN)) > + return -EPERM;
Is this the right place to be performing permission checks?
> + if(copy_from_user(&frame, argp, sizeof(bin_header)))
if (
> + return -EFAULT; > +
It's just as well this code if ifdefed out. Avoid using return statements buried deep in the logic. It's just asking for resource leaks, now or in the future. Aim for a simgle return point per function, or at least keep all the returns right at the end of the function.
> + if ((status=inw(base+0x4))!=0) { > + printk(KERN_WARNING "ISILoad:Card%d rejected verify header:\nAddress:0x%x \nCount:0x%x \nStatus:0x%x \n",
80 cols
> + for(i=0;i<=0x0f;i++); /* another wee bit of delay */
udelay()
> + > + if (card >= BOARD_COUNT) { > + return -EPERM; > + }
unneeded braces. embedded `return'
> + > +static void __devexit isicom_remove(struct pci_dev *pdev) > +{ > + int idx; > + > + for(idx = 0; idx < BOARD_COUNT; idx++) > + if (isi_card[idx].base == pci_resource_start(pdev, 3)) > + break; > +
whitespace weevils at work
> + printk(KERN_ERR "ISICOM: ISA not supported yet.");
newline
> + retval = pci_register_driver(&isicom_driver); > + if (retval < 0) { > + printk(KERN_ERR "ISICOM: Unable to register pci driver.\n"); > + goto error; > }
weevils
> +#define InterruptTheCard(base) (outw(0,(base)+0xc)) > +#define ClearInterrupt(base) (inw((base)+0x0a))
Somewhat risky identifiers for a header file.. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |