Messages in this thread |  | | From | Dmitry Torokhov <> | Subject | Re: [PATCH] Add Cobalt button interface driver support | Date | Thu, 15 Feb 2007 23:09:43 -0500 |
| |
On Thursday 15 February 2007 22:36, Yoichi Yuasa wrote: > Hi, > > This patch adds support for the back panel buttons on Cobalt server. > It's tested on the Cobalt Qube2. >
Hi,
Thank you for your patch. Couple of comments:
> + > + button++; > + } > + > + mod_timer(&buttons_timer, jiffies + HZ / BUTTONS_POLL_FREQUENCY);
May I suggest using msecs_to_jiffies() to avoid direct computations on HZ?
> + > + bdev->input = input; > + bdev->reg = ioremap(res->start, res->end - res->start + 1); > + dev_set_drvdata(&pdev->dev, bdev); > + > + setup_timer(&buttons_timer, handle_buttons, (unsigned long)bdev); > + mod_timer(&buttons_timer, jiffies + HZ / BUTTONS_POLL_FREQUENCY);
Please implement cobalt_buttons_open() and cobalt_buttons_close() methods and start/stop timer from there - there is no point in polling hardware if noone is listening to the events.
> + > + return retval; > +} > + > +static int __devexit cobalt_buttons_remove(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct buttons_dev *bdev = dev_get_drvdata(dev); > + > + del_timer(&buttons_timer);
del_timer_sync? Is there any possibility it may run SMP?
> + > +static void __exit cobalt_buttons_exit(void) > +{ > + platform_driver_unregister(&cobalt_buttons_driver);
You are not allowed to unregister statically allocated devices - if there are references left and your module goes away then these references become invalid and kernel goes boom.
Please convert to platform_device_alloc/platform_device_add.
Is there a point of moving platform device creation code into platform-specific code? Is there a possibility of this driver being used elsewhere?
> + platform_device_unregister(&cobalt_buttons_device); > +} > + > +module_init(cobalt_buttons_init); > +module_exit(cobalt_buttons_exit); >
-- Dmitry - 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/
|  |