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

[Jesper Juhl]

On 10/13/05, Mark Gross <mgross@xxxxxxxxxxxxxxx> wrote:
> On Wednesday 12 October 2005 18:14, Greg KH wrote:
> > On Wed, Oct 12, 2005 at 04:36:29PM -0700, Mark Gross wrote:
> > > No, but I'm glad I tested that otherwise the my problem with using dev_dbg
> > > with the kobj->dev devices I got from the misc_device class could have
> gotten
> > > by me.
> >
> > Yeah, it's always good to test the code to make sure it compiles :)
> >
> > This patch looks good, I have no objections to it.
> >
> > thanks,
> >
>
> One minor update, after testing with misc_class device for udev I found that
> it would be nice to have the sysfs file inodes all use the same base name
> "teleco_clock".
>
> Please consider including this driver in the MM tree.
>
> See attached.
>

Hi Mark,

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)) {
		printk(KERN_ERR "telco_clock: Interrupt can't be reserved!\n");
		return -EBUSY;
	}
	inb(TLCLK_REG6);	/* Clear interrupt events */

	return 0;
}

And btw, what about the other error return values that request_irq can return?
You might get back -ENOMEM or -EINVAL...  So shouldn't you rather be
doing something like

result = request_irq(...);
if (result < 0)
   /* handle error */

?????

(which then of course would bring the "result" variable back into play ;)


+ssize_t tlclk_read(struct file *filp, char __user *buf, size_t count,
...
+	return  sizeof(struct tlclk_alarms);

Why do you have 2 spaces here between "return" and "sizeof..." ?


+static DEVICE_ATTR(current_ref, S_IRUGO, show_current_ref, NULL);
+
+
+static ssize_t show_interrupt_switch(struct device *d,

Surely a single space between these two lines should be enough ;) (ok,
I'm nitpicking, I admit it).


+	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?


Maybe I'm missing something, but in tlclk_init() you are calling
request_region() and in case of failure you can end up exiting via the
out3: label which will result in release_region() being called... What
now prevents the release region() in tlclk_cleanup() from being called
on an already released region?




--
Jesper Juhl <jesper.juhl@xxxxxxxxx>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/