Re: Fwd: Telecom Clock Driver for MPCBL0010 ATCA computer blade

[Greg KH <greg@kroah.com>]

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