On 10/13/05, Mark Gross <[email protected]> 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 <[email protected]>
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 [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]