> On Sun, 25 Feb 2007 18:39:57 +0000 (GMT) James Simmons <[EMAIL PROTECTED]> > wrote: > > Here is the second round to the new display class. This is meant to unite > the various solutions to display units ie acpi output device, auxdisplay > and the defunct lcd class in the backlight directory. Please apply. >
Little things.. > +static ssize_t display_show_name(struct device *dev, struct device_attribute > *attr, > +static ssize_t display_show_type(struct device *dev, struct device_attribute > *attr, > +static ssize_t display_show_contrast(struct device *dev, struct > device_attribute *attr, this stuff looks el-crappo in an 80-col display. > +DECLARE_BITMAP(inuse, NUM_OF_DISPLAYS); This guy can have static scope. It's a bit awkward having a fixed size. I think lib/idr.c would suit for this. Also, we do set_bit() and clear_bit() on this, which are needlessly atomic. If we hold a suitable lock then we can use __set_bit() and __clear_bit(). And we do need a suitable lock. > + > +struct display_device *display_device_register(struct display_driver *driver, > + struct device *parent, void > *devdata) > +{ > + int idx = find_first_zero_bit(inuse, NUM_OF_DISPLAYS); > + struct display_device *new_dev = NULL; > + struct device *display_device = NULL; > + > + if (unlikely(!driver)) > + return ERR_PTR(-EINVAL); > + > + display_device = device_create(display_class, parent, 0, "display%d", > idx); > + if (IS_ERR(display_device)) > + return ERR_PTR(-ENOMEM); > + > + new_dev = kzalloc(sizeof(struct display_device), GFP_KERNEL); > + if (likely(new_dev) && unlikely(driver->probe(new_dev, devdata))) { > + dev_set_drvdata(display_device, new_dev); > + new_dev->dev = display_device; > + new_dev->parent = parent; > + new_dev->driver = driver; > + mutex_init(&new_dev->lock); > + set_bit(idx, inuse); > + new_dev->idx = idx; > + } else > + device_unregister(display_device); > + return new_dev; > +} > +EXPORT_SYMBOL(display_device_register); Because I think this function is racy in its handling of inuse? > +static int __init display_class_init(void) > +{ > + display_class = class_create(THIS_MODULE, "display"); > + if (IS_ERR(display_class)) { > + printk(KERN_ERR "Failed to create display class\n"); > + display_class = NULL; > + return -EINVAL; > + } > + display_class->dev_attrs = display_attrs; > + display_class->suspend = display_suspend; > + display_class->resume = display_resume; > + bitmap_zero(inuse, NUM_OF_DISPLAYS); > + return 0; > +} > + > +#ifdef MODULE > +module_init(display_class_init); > + > +static void __exit display_class_exit(void) > +{ > + class_destroy(display_class); > +} > +module_exit(display_class_exit); > +#else > +subsys_initcall(display_class_init); > +#endif Why all the ifdeffery? I think plain old static void __exit display_class_exit(void) { class_destroy(display_class); } module_exit(display_class_exit); module_init(display_class_init); would suit here. Unless there's some special reason why we need subsys_initcall() here if it's linked into vmlinux. If so, please add a comment so the next reader doesn't get all confused too. - 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/