Menu
▾
▴
Re: [PATCH] Add support for generic maple bus and for maple bus keyboard
|
From: Paul M. <le...@li...> - 2007-08-30 06:41:02
|
On Tue, Aug 28, 2007 at 11:14:00PM +0100, Adrian McMenamin wrote:
> Please find this patch below - they said it would never be done!
>
> Anyway, this adds new maplebus support to the kernel and a maple
> keyboard driver is also attached. There a couple of minor and
> consequential patches to other kernel subsystems attached that are
> required to get this to work (eg make the PVR2 VBLANK interrupt
> shareable).
>
It would be nice if you could split this up at least, so there's the PVR2
thing by itself, the patch adding the bus, and then the input driver
afterwards.
In the long run these patches will have to be submitted through different
maintainers, so the earlier they are split logically, the better.
> --- /dev/null
> +++ b/drivers/input/keyboard/maple_keyb.c
> @@ -0,0 +1,210 @@
> +/*
> + * SEGA Dreamcast keyboard driver
> + * Based on drivers/usb/usbkbd.c
> + * Copyright YAEGASHI Takeshi, 2001
> + * Porting to 2.6 Copyright Adrian McMenamin, 2007
> + * Licensed under the GPL
> + */
> +
The licensing is rather ambiguous here. From the MODULE_LICENSE() we can
infer that this is GPLv2 + any later version, so that's probably
something that should be verbosely noted.
> +MODULE_AUTHOR
> + ("YAEGASHI Takeshi <t...@ke...>, Adrian McMenamin
> <ad...@mc...");
This wrapping is rather broken..
> + else
> + printk
> + ("Unknown key (scancode %#x) released.",
> + kbd->old[i]);
Likewise.
> + }
> +
> + if (kbd->new[i] > 3
> + && memchr(kbd->old + 2, kbd->new[i], 6) != NULL) {
&&'s at the end of the first line.
> + if (dc_kbd_keycode[kbd->new[i]])
> + input_report_key(dev,
> + dc_kbd_keycode[kbd->
> + new[i]],
> + 1);
> + else
> + printk
> + ("Unknown key (scancode %#x) pressed.",
> + kbd->new[i]);
More strange wrapping.
> + memcpy(kbd->old, kbd->new, 8);
The size here should be calculated based off the size of kbd->old, rather
than hard coded. If you intend to hardcode it, at least use a define to
make the size descriptive.
> + kbd = kmalloc(sizeof(struct dc_kbd), GFP_KERNEL);
> + if (unlikely(!kbd))
> + return -1;
> + memset(kbd, 0, sizeof(struct dc_kbd));
> +
kzalloc()
> + input_register_device(kbd->dev);
> +
This can fail.
> +static struct maple_driver dc_kbd_driver = {
> + .function = MAPLE_FUNC_KEYBOARD,
> + .connect = dc_kbd_connect,
> + .disconnect = dc_kbd_disconnect,
> + .drv = {
> + .name = "Dreamcast_keyboard",
> + .bus = &maple_bus_type,
> + .probe = probe_maple_kbd,},
> +};
> +
I'm a bit confused as to why you don't have a registration function for a
struct maple_driver that automatically sets the bus type? This is the
convention most of the busses use.
> diff --git a/drivers/sh/maple/Makefile b/drivers/sh/maple/Makefile
> new file mode 100644
> index 0000000..f8c39f2
> --- /dev/null
> +++ b/drivers/sh/maple/Makefile
> @@ -0,0 +1,3 @@
> +#Makefile for Maple Bus
> +
> +obj-y := maplebus.o
No module goodness?
> diff --git a/drivers/sh/maple/maplebus.c b/drivers/sh/maple/maplebus.c
> new file mode 100644
> index 0000000..fc27543
> --- /dev/null
> +++ b/drivers/sh/maple/maplebus.c
> @@ -0,0 +1,641 @@
> +/* maplebus.c
> + * Core maple bus functionality
> + * Core code copyright
> + * Paul Mundt, M. R. Brown and others
> + * Porting to 2.6 Copyright Adrian McMenamin, 2007
> + * Licence: GNU GPL version 2
> + * http://www.gnu.org
> + */
> +
Both Marcus and I intended all of our code to be v2 only, so that should
probably be statically defined here. However, I believe Yaegashi-san
wrote a lot of this initial version, and I'm sure there were others that
hacked on it as well. So we may simply have to leave it as GPLv2 with the
unfortunate + any later version garbage.
> +static struct maple_driver maple_basic_driver;
> +static struct device maple_bus;
> +static int subdevice_map[MAPLE_PORTS];
> +unsigned long *maple_sendbuf, *maple_sendptr, *maple_lastptr;
> +unsigned long maple_pnp_time;
> +static int started;
> +static int scanning;
> +
Way too many global variables. None of these need to be global, and even
the statics you can likely largely do away with.
> +void maple_getcond_callback(struct maple_device *dev,
> + void (*callback) (struct mapleq *mq),
> + unsigned long interval, unsigned long function)
> +{
> + dev->callback = callback;
> + dev->interval = interval;
> + dev->function = cpu_to_be32(function);
> + dev->when = 0;
> +}
> +
> +EXPORT_SYMBOL(maple_getcond_callback);
> +
EXPORT_SYMBOL_GPL(), and no white space between that and the closing }.
> +static void maple_release_device(struct device *dev)
> +{
> + if (likely(dev->type)) {
> + if (likely(dev->type->name))
> + kfree(dev->type->name);
> + kfree(dev->type);
kfree() can handle NULL, so you can just do this as:
if (likely(dev->type)) {
kfree(dev->type->name);
kfree(dev->type);
}
...
> +void maple_add_packet(struct mapleq *mq)
> +{
> + unsigned long flags;
> +
> + local_irq_save(flags);
> + list_add((struct list_head *) mq, &maple_waitq);
> + local_irq_restore(flags);
> +}
> +
This can use some proper locking, and the cast is pointless. mq should
have a list_head in it somewhere that gets passed in rather than
expecting the cast to work out.
> +static struct mapleq *maple_allocq(struct maple_device *dev)
> +{
> + unsigned long buf;
> + struct mapleq *mq;
> +
> + mq = kmalloc(sizeof(*mq), GFP_KERNEL);
> + if (unlikely(!mq))
> + return NULL;
> +
> + mq->dev = dev;
> + buf = (unsigned long) mq->buf;
> + buf = (buf + 31) & ~31;
> + mq->recvbuf = (void *) P2SEGADDR(buf);
> +
This is a serious hack. I would suggest just using a slab cache for these
objects, and forcing cacheline alignment there. Then you can also set up
the uncached recvbuf in the slab ctor.
> + function = be32_to_cpu(dev->devinfo.function);
> +
What exactly is the point of this? The function is big endian, really?
> +static int maple_get_dma_buffer(void)
> +{
> + maple_sendbuf =
> + (void *) __get_dma_pages(GFP_KERNEL, MAPLE_DMA_PAGES);
> + if (unlikely(!maple_sendbuf))
> + return -ENOMEM;
> + memset(maple_sendbuf, 0, MAPLE_DMA_SIZE);
> + return 0;
> +}
> +
__get_free_pages(GFP_KERNEL | __GFP_ZERO, MAPLE_DMA_ORDER);
...
We don't even have a ZONE_DMA, so __get_dma_pages() is total
brain-damage.
> + maple_drv = container_of(drvptr, struct maple_driver, drv);
> + maple_dev = container_of(devptr, struct maple_device, dev);
> +
> + if (unlikely
> + (maple_dev->devinfo.
> + function & be32_to_cpu(maple_drv->function)))
> + return 1;
> + return 0;
Strange wrapping, and you can just do the return of that check directly,
rather than breaking this out in to 1/0.
> +static int maple_bus_uevent(struct device *dev, char **envp,
> + int num_envp, char *buffer, int buffer_size)
> +{
> + return 0;
> +}
How are you planning on notifying userspace when a device is
plugged/unplugged?
> +
> +static void maple_bus_release(struct device *dev)
> +{
> + printk(KERN_INFO "Releasing maple bus device\n");
> +}
> +
Kill this.
> +static struct maple_driver maple_basic_driver = {
> + .drv = {
> + .name = "Maple_bus_basic_driver",
> + .bus = &maple_bus_type,},
Strange brace cuddling. Even stranger driver name.
> +static struct device maple_bus = {
> + .bus_id = "maple0",
> + .release = maple_bus_release,
> +};
> +
What is the 0 for?
> + retval = device_register(&maple_bus);
> + if (unlikely(retval))
> + goto cleanup;
> +
> + retval = bus_register(&maple_bus_type);
> + if (unlikely(retval))
> + goto cleanup_device;
> +
> + printk(KERN_INFO "Maple bus core now registered.\n");
> +
> + retval = driver_register(&maple_basic_driver.drv);
> + if (unlikely(retval))
> + goto cleanup_bus;
> +
> + /* allocate memory for maple bus dma */
> + retval = maple_get_dma_buffer();
> + if (unlikely(retval)) {
> + printk(KERN_INFO
> + "Maple bus: Failed to allocate Maple DMA buffers\n");
> + goto cleanup_basic;
> + }
> +
> + /* set up DMA interrupt handler */
> + retval = maple_set_dma_interrupt_handler();
> + if (unlikely(retval)) {
> + printk(KERN_INFO
> + "Maple bus: Failed to grab maple DMA IRQ\n");
> + goto cleanup_dma;
> + }
> +
> + /* set up VBLANK interrupt handler */
> + retval = maple_set_vblank_interrupt_handler();
> + if (unlikely(retval)) {
> + printk(KERN_INFO "Maple bus: Failed to grab VBLANK IRQ\n");
> + goto cleanup_irq;
> + }
> +
> +
> + /* setup maple ports */
> + for (i = 0; i < MAPLE_PORTS; i++) {
> + mdev = maple_alloc_dev(i, 0);
> + mdev->registered = 0;
> + if (unlikely(!mdev))
> + goto cleanup_bothirqs;
i-1 mdev allocations will leak on failure. You've also just oopsed on the
->registered dereference if the allocation fails.
> +static void __exit maple_bus_exit(void)
> +{
> + driver_unregister(&maple_basic_driver.drv);
> + bus_unregister(&maple_bus_type);
> +
> + if (likely(maple_sendbuf))
> + free_pages((unsigned long) maple_sendbuf, MAPLE_DMA_PAGES);
> +
> + free_irq(HW_EVENT_MAPLE_DMA, 0);
> + free_irq(HW_EVENT_VSYNC, 0);
I would do these first. Bad things will happen if all of your state
disappears whilst IRQs are still in-flight.
> +/* use init call to ensure bus is registered ahead of devices */
> +fs_initcall(maple_bus_init);
Use subsys_initcall() here, it's a subsystem, so this is entirely
reasonable.
> diff --git a/include/linux/input.h b/include/linux/input.h
> index e02c6a6..2e65c88 100644
> --- a/include/linux/input.h
> +++ b/include/linux/input.h
> @@ -688,6 +688,7 @@ struct input_absinfo {
> #define BUS_HOST 0x19
> #define BUS_GSC 0x1A
> #define BUS_ATARI 0x1B
> +#define BUS_MAPLE 0x1C
>
Do we really need to still do this these days?
> +/* Maple Bus command and response codes */
> +/* Function codes */
These can use enums and opaque types.
> +struct mapleq {
> + struct list_head list;
> + struct maple_device *dev;
> + void *sendbuf, *recvbuf;
> + unsigned char command, length;
> + unsigned char buf[1024 + 32];
> +};
> +
That's pretty big not to be dynamically allocating.
> +struct maple_devinfo {
[snip]
> + char product_licence[60];
British variable names, if there's nothing in CodingStyle about that,
there should be :-)
|
