On Thursday 06 October 2005 16:15, Greg KH wrote:
> On Thu, Oct 06, 2005 at 03:54:34PM -0700, Mark Gross wrote:
> > On Thursday 06 October 2005 11:20, Greg KH wrote:
> > > > +?????????????printk(KERN_ERR" misc_register retruns %d \n", ret);
> > > > +?????????????ret = ?-EBUSY;
> > > > +?????????????goto out3;
> > > > +?????}
> > > > +?????class = tlclk_miscdev.class;
> > > > +?????class_device_create_file(class, &class_device_attr_current_ref);
> > > 
> > > Try registering a whole attribute group instead. ?It's much nicer than
> > > the 20 lines you have to register and unregister your devices (and you
> > > don't handle the error condition properly if something goes wrong half
> > > way through.)
> > > 
> > 
> > I couldn't find such an API that wasn't static to class.c, or
> > described in class.txt.  Any pointers on this would be helpful.
> 
> sysfs_create_group() is what you want.
> 
> > +ssize_t tlclk_read(struct file *filp, char __user *buf, size_t count,
> > +  loff_t *f_pos)
> > +{
> > + if (count < sizeof(struct tlclk_alarms))
> > +  return -EIO;
> > +
> > + wait_event_interruptible(wq, got_event);
> > + if (copy_to_user(buf, alarm_events, sizeof(struct tlclk_alarms)))
> > +  return -EFAULT;
> > +
> > + memset(alarm_events, 0, sizeof(struct tlclk_alarms));
> > + got_event = 0;
> > +
> > + return  sizeof(struct tlclk_alarms);
> > +}
> 
> Two spaces after the return :(
> 
> > +
> > +/* Read telecom clock IRQ number (Set by BIOS) */
> 
> Indent the comment please.
> 
> > + init_timer(&switchover_timer);
> > +/* switchover_timer.function = switchover_timeout; */
> > +/* switchover_timer.data = 0; */
> 
> Remove these commented out lines?
> 
> Other than these minor things, and the attribute group stuff, this looks
> good.
> 
> thanks,

I think I've gotten all the comments addressed with this version.

Most significantly I moved the driver from a misc_device to a platform_device.

Thanks,


-- 
--mgross
BTW: This may or may not be the opinion of my employer, more likely not.