Messages in this thread | | | From | Dmitry Torokhov <> | Subject | Re: [PATCH 2/3] Maple bus support for the Sega Dreamcast - keyboard support | Date | Sun, 9 Sep 2007 23:50:46 -0400 |
| |
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 <adrian@mcmen.demon.co.uk> >
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 <dtor@mail.ru>
Thank you.
-- 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/
| |