Re: [PATCH -mm 1/2] drivers/char/isicom old api rewritten

[Andrew Morton <akpm@osdl.org>]

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..