Hi Adrian,
On Sunday 09 September 2007 16:57, Adrian McMenamin wrote:
>
> Add maple bus keyboard support to the kernel.
>
> Signed-off by Adrian McMenamin <[email protected]>
>
Couple of more things:
> +
> +extern int maple_driver_register(struct device_driver *drv);
> +
No externs in *.c files, please. Isn't this defined in maple.h which is
being included?
> +
> +static void dc_kbd_callback(struct mapleq *mq)
> +{
> + struct maple_device *mapledev = mq->dev;
> + struct dc_kbd *kbd = mapledev->private_data;
> + unsigned long *buf = mq->recvbuf;
I very much prefer an empty line between variables and the code.
> + if (!mutex_trylock(&maple_keyb_mutex)) /* Can only be locked if
> already in cleanup */
> + return;
> + if (buf[1] == mapledev->function) {
> + memcpy(kbd->new, buf + 2, 8);
> + dc_scan_kbd(kbd);
> + }
> + mutex_unlock(&maple_keyb_mutex);
> +}
> +
> +static int dc_kbd_connect(struct maple_device *dev)
> +{
> + int i, retval = 0;
> + struct dc_kbd *kbd;
> + if (!(dev->function & MAPLE_FUNC_KEYBOARD))
> + return -EINVAL;
> +
> + kbd = kzalloc(sizeof(struct dc_kbd), GFP_KERNEL);
> + if (unlikely(!kbd))
> + return -ENOMEM;
> +
> + dev->private_data = kbd;
> + kbd->dev = input_allocate_device();
> + if (!kbd->dev){
> + retval = -ENODEV;
> + goto cleanup_memory;
> + }
> +
> + for (i = 0; i < NR_SCANCODES; i++)
> + kbd->keycode[i] = dc_kbd_keycode[i];
memcpy?
> + kbd->dev->evbit[0] = BIT(EV_KEY) | BIT(EV_REP);
> + kbd->dev->keycodesize = sizeof (unsigned char);
> + kbd->dev->keycodemax = ARRAY_SIZE(kbd->keycode);
> + kbd->dev->keycode = kbd->keycode;
> +
> +
> + for (i = 0; i < 255; i++)
Are you sure it is 255 and not NR_SCANCODES?
> + set_bit(dc_kbd_keycode[i], kbd->dev->keybit);
> +
> + clear_bit(0, kbd->dev->keybit);
> +
> + kbd->dev->private = kbd;
Please use input_set_drvdata() here and input_get_drvdata() when
accessing it.
> +
> + kbd->dev->name = dev->product_name;
> + kbd->dev->id.bustype = BUS_HOST;
> +
maple_device appears to be fully integrated in sysfs, please add:
kbd->dev->dev.parent = &dev->dev;
Btw, introducing a temp for kbd->dev should reduce generated code
somewhat.
> + retval = input_register_device(kbd->dev);
> + if (retval)
> + goto cleanup;
> + /* Maple polling is locked to VBLANK - which may be just 50/s */
> + maple_getcond_callback(dev, dc_kbd_callback, HZ/50,
> + MAPLE_FUNC_KEYBOARD);
> + return retval;
> +
> +cleanup:
> + input_free_device(kbd->dev);
> +cleanup_memory:
> + kfree(kbd);
> + return retval;
> +}
> +
> +static void dc_kbd_disconnect(struct maple_device *dev)
> +{
> + struct dc_kbd *kbd;
> + mutex_lock(&maple_keyb_mutex);
> + kbd = dev->private_data;
> +
> + input_unregister_device(kbd->dev);
> + kfree(kbd);
> + mutex_unlock(&maple_keyb_mutex);
> +}
> +
> +/* allow the keyboard to be used */
> +static int probe_maple_kbd(struct device *dev)
> +{
> + struct maple_device *mdev;
> + struct maple_driver *mdrv;
> + int proberes;
> +
> + mdev = to_maple_dev(dev);
> + mdrv = to_maple_driver(dev->driver);
> +
> + proberes = dc_kbd_connect(mdev);
> + if (unlikely(proberes))
> + return proberes;
> + mdev->driver = mdrv;
> + mdev->registered = 1;
> + return 0;
> +}
> +
> +static struct maple_driver dc_kbd_driver = {
> + .function = MAPLE_FUNC_KEYBOARD,
> + .connect = dc_kbd_connect,
> + .disconnect = dc_kbd_disconnect,
> + .drv = {
> + .name = "Dreamcast_keyboard",
> + .probe = probe_maple_kbd,
> + },
> +};
> +
> +static int __init dc_kbd_init(void)
> +{
> + int retval;
> + retval = maple_driver_register(&dc_kbd_driver.drv);
> + if (retval)
> + return retval;
> + return 0;
"return maple_driver_register(&dc_kbd_driver.drv);" is much shorter...
> +}
> +
> +static void __exit dc_kbd_exit(void)
> +{
> + driver_unregister(&dc_kbd_driver.drv);
> +}
> +
> +module_init(dc_kbd_init);
> +module_exit(dc_kbd_exit);
If these are fixed you may add:
Acked-by: Dmitry Torokhov <[email protected]>
Thank you.
--
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/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
