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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
