Messages in this thread | | | Date | Thu, 13 Oct 2005 15:40:42 -0700 | From | Greg KH <> | Subject | Re: Fwd: Telecom Clock Driver for MPCBL0010 ATCA computer blade |
| |
On Fri, Oct 14, 2005 at 12:08:28AM +0200, Jesper Juhl wrote: > > I just took a new look at your patch and I have (again) a few small comments... > > > +static int tlclk_open(struct inode *inode, struct file *filp) > +{ > + int result; > + > + /* Make sure there is no interrupt pending while > + * initialising interrupt handler */ > + inb(TLCLK_REG6); > + > + /* This device is wired through the FPGA IO space of the ATCA blade > + * we can't share this IRQ */ > + result = request_irq(telclk_interrupt, &tlclk_interrupt, > + SA_INTERRUPT, "telco_clock", tlclk_interrupt); > + if (result == -EBUSY) { > + printk(KERN_ERR "telco_clock: Interrupt can't be reserved!\n"); > + return -EBUSY; > + } > + inb(TLCLK_REG6); /* Clear interrupt events */ > + > + return 0; > +} > > It seems to me that you can get rid of the "result" variable here by > rewriting the funcion like this : > > static int tlclk_open(struct inode *inode, struct file *filp) > { > /* Make sure there is no interrupt pending while > * initialising interrupt handler */ > inb(TLCLK_REG6); > > /* This device is wired through the FPGA IO space of the ATCA blade > * we can't share this IRQ */ > if (-EBUSY == request_irq(telclk_interrupt, &tlclk_interrupt, > SA_INTERRUPT, "telco_clock", tlclk_interrupt)) {
Ick, no, that's a mess. Stick with the original version.
Don't call functions within a if() statement, it's harder to read.
> + unsigned long tmp; > + unsigned char val; > + unsigned long flags; > + > + sscanf(buf, "%lX", &tmp); > + dev_dbg(d, "tmp = 0x%lX\n", tmp); > + > + val = (unsigned char)tmp; > > You do this a lot, I'm wondering why you don't read directly into > "val" and then get rid of the "tmp" variable?
Because you want to cast it.
thanks,
greg k-h - 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/
| |