lkml.org 
[lkml]   [2005]   [Oct]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: 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/

\
 
 \ /
  Last update: 2005-10-14 00:43    [W:0.047 / U:0.780 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site