On Wednesday 04 April 2007 21:25, Li Yu wrote:
> Jiri Kosina wrote:
> > BTW as soon as you have some presentable code, could you please send
> > it so
> > that we could see what aproach you have taken? Debating over code is
> > usualy more efficient than just ranting random ideas :)
> >
> >
> There is a "presentable patch" in the attachment ;)
Some random notes without reading it all carefully...
> +static int hid_bus_match(struct device *dev, struct device_driver
> *drv) +{
> + struct hid_driver *hid_drv;
> + struct hid_device *hid_dev;
> +
> + hid_drv = to_hid_driver(drv);
> + hid_dev = to_hid_device(dev);
> +
> + if (is_hid_driver_sticky(hid_drv))
> + /* the sticky driver match device do not pass here. */
> + return 0;
> + if (hid_dev->bus != hid_drv->bus)
> + return 0;
How can this happen?
> + if (!hid_drv->match || hid_drv->match(hid_drv, hid_dev)) {
> + hid_dev->driver = hid_drv;
This usually done in bus->probe() function, when we know for sure that
driver binds to to the device.
> +static void hid_bus_release(struct device *dev)
> +{
> +}
> +
> +struct device hid_bus = {
> + .bus_id = "hidbus0",
> + .release = hid_bus_release
> +};
> +
> +static void hid_dev_release(struct device *dev)
> +{
> +}
> +
That will for sure raise Greg KH's blood pressure ;)
> + for (i=0; hid_dev->attrs && hid_dev->attrs[i]; ++i) {
> + ret = device_create_file(&hid_dev->device, hid_dev->attrs[i]);
> + if (ret)
> + break;
> +
That should be handled via bus's device attributes and not open coded...
> - * Copyright (c) 2000-2005 Vojtech Pavlik <[EMAIL PROTECTED]>
> - * Copyright (c) 2005 Michael Haboustak <[EMAIL PROTECTED]> for Concept2,
> Inc
> + * Copyright (c) 2000-2005 Vojtech Pavlik <vojtech@@suse.cz>
> + * Copyright (c) 2005 Michael Haboustak <mike-@@cinci.rr.com> for Concept2,
> Inc
> * Copyright (c) 2006 Jiri Kosina
Any particular reason for mangling addresses?
> + if (interrupt)
> + local_irq_save(flags);
> + spin_lock(&hid_lock);
> + list_for_each_entry(driver, &hid_sticky_drivers, sticky_link) {
> + hook = driver->hook;
> + if (hook && hook->raw_event) {
> + ret = hook->raw_event(hid, type, data, size, interrupt);
> + if (!ret)
> + break;
> + }
> + }
> + spin_unlock(&hid_lock);
> + if (interrupt)
> + local_irq_restore(flags);
> +
This is scary. spin_lock_irqsave() and be done with it.
> +int hid_open(struct hid_device *hid)
> +{
> + struct hid_transport *tl;
> + int ret;
> +
> + if (hid->driver->open)
> + return hid->driver->open(hid);
> + ret = 0;
> + spin_lock(&hid_lock);
> + tl = hid_transports[hid->bus];
> + if (tl->open)
> + ret = tl->open(hid);
> + spin_unlock(&hid_lock);
> + return ret;
> +}
Spinlock is not the best choise here, I'd expect most ->open()
implementation wait on some IO.
--
Dmitry
-
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/
