Messages in this thread | | | Date | Tue, 27 Feb 2007 12:25:31 -0800 | From | Andrew Morton <> | Subject | Re: [PATCH] display class |
| |
> On Sun, 25 Feb 2007 18:39:57 +0000 (GMT) James Simmons <jsimmons@infradead.org> 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |