From: Adrian M. <ad...@ne...> - 2007-08-28 22:14:41
|
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). Any and all comments and testers welcome and/or required. Signed-off by Adrian McMenamin <ad...@mc...> diff --git a/arch/sh/Kconfig b/arch/sh/Kconfig index 54878f0..42e3d95 100644 --- a/arch/sh/Kconfig +++ b/arch/sh/Kconfig @@ -694,8 +694,20 @@ config CF_AREA5 config CF_AREA6 bool "Area6" + endchoice +config MAPLE + bool "Maple Bus Support" + depends on SH_DREAMCAST + ---help--- + The Maple Bus is SEGA's serial communication bus for peripherals - + a bit like USB for your Dreamcast. Without this bus support you + won't be able to get your Dreamcast keyboard etc to work, so you + probably want to say 'Y' here, though if you are just running + the Dreamcast via a serial cable or network connection you may + not need this. + config CF_BASE_ADDR hex depends on CF_ENABLER diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig index c97d5eb..1689f73 100644 --- a/drivers/input/keyboard/Kconfig +++ b/drivers/input/keyboard/Kconfig @@ -253,4 +253,14 @@ config KEYBOARD_GPIO To compile this driver as a module, choose M here: the module will be called gpio-keys. + +config KEYBOARD_MAPLE + tristate "Maple bus keyboard" + depends on SH_DREAMCAST && MAPLE + help + Say Y here if you have a DreamCast console running Linux and have + a keyboard attached to its Maple bus. + + To compile this driver as a module, choose M here: the + module will be called maple_keyb. endif diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile index 28d211b..3f775ed 100644 --- a/drivers/input/keyboard/Makefile +++ b/drivers/input/keyboard/Makefile @@ -21,4 +21,5 @@ obj-$(CONFIG_KEYBOARD_OMAP) += omap-keypad.o obj-$(CONFIG_KEYBOARD_PXA27x) += pxa27x_keyboard.o obj-$(CONFIG_KEYBOARD_AAED2000) += aaed2000_kbd.o obj-$(CONFIG_KEYBOARD_GPIO) += gpio_keys.o +obj-$(CONFIG_KEYBOARD_MAPLE) += maple_keyb.o diff --git a/drivers/input/keyboard/maple_keyb.c b/drivers/input/keyboard/maple_keyb.c new file mode 100644 index 0000000..3aae3aa --- /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 + */ + +#include <linux/kernel.h> +#include <linux/slab.h> +#include <linux/input.h> +#include <linux/module.h> +#include <linux/init.h> +#include <linux/timer.h> +#include <linux/maple.h> + +MODULE_AUTHOR + ("YAEGASHI Takeshi <t...@ke...>, Adrian McMenamin <ad...@mc..."); +MODULE_DESCRIPTION("SEGA Dreamcast keyboard driver"); +MODULE_LICENSE("GPL"); + +static unsigned char dc_kbd_keycode[256] = { + 0, 0, 0, 0, 30, 48, 46, 32, + 18, 33, 34, 35, 23, 36, 37, 38, + 50, 49, 24, 25, 16, 19, 31, 20, + 22, 47, 17, 45, 21, 44, 2, 3, + 4, 5, 6, 7, 8, 9, 10, 11, 28, + 1, 14, 15, 57, 12, 13, 26, + 27, 43, 43, 39, 40, 41, 51, + 52, 53, 58, 59, 60, 61, 62, 63, 64, + 65, 66, 67, 68, 87, 88, 99, + 70, 119, 110, 102, 104, 111, + 107, 109, 106, 105, 108, 103, + 69, 98, 55, 74, 78, 96, 79, 80, + 81, 75, 76, 77, 71, 72, 73, 82, 83, + 86, 127, 116, 117, 183, 184, 185, + 186, 187, 188, 189, 190, + 191, 192, 193, 194, 134, 138, 130, 132, + 128, 129, 131, 137, 133, 135, 136, 113, + 115, 114, 0, 0, 0, 121, 0, 89, 93, 124, + 92, 94, 95, 0, 0, 0, + 122, 123, 90, 91, 85, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, + 29, 42, 56, 125, 97, 54, 100, 126, + 164, 166, 165, 163, 161, 115, 114, 113, + 150, 158, 159, 128, 136, 177, 178, 176, 142, + 152, 173, 140 +}; + +struct dc_kbd { + struct input_dev *dev; + unsigned char new[8]; + unsigned char old[8]; +}; + +static void dc_scan_kbd(struct dc_kbd *kbd) +{ + int i; + struct input_dev *dev = kbd->dev; + for (i = 0; i < 8; i++) + input_report_key(dev, + dc_kbd_keycode[i + 224], + (kbd->new[0] >> i) & 1); + + for (i = 2; i < 8; i++) { + + if (kbd->old[i] > 3 + && memchr(kbd->new + 2, kbd->old[i], 6) == NULL) { + if (dc_kbd_keycode[kbd->old[i]]) + input_report_key(dev, + dc_kbd_keycode[kbd-> + old[i]], + 0); + else + printk + ("Unknown key (scancode %#x) released.", + kbd->old[i]); + } + + if (kbd->new[i] > 3 + && memchr(kbd->old + 2, kbd->new[i], 6) != NULL) { + 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]); + } + } + + input_sync(dev); + + memcpy(kbd->old, kbd->new, 8); +} + +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; + if (buf[1] == mapledev->function) { + memcpy(kbd->new, buf + 2, 8); + dc_scan_kbd(kbd); + } +} + +static int dc_kbd_connect(struct maple_device *dev) +{ + int i; + unsigned long data = be32_to_cpu(dev->devinfo.function_data[0]); + struct dc_kbd *kbd; + if (dev->function != MAPLE_FUNC_KEYBOARD) + return -EINVAL; + + kbd = kmalloc(sizeof(struct dc_kbd), GFP_KERNEL); + if (unlikely(!kbd)) + return -1; + memset(kbd, 0, sizeof(struct dc_kbd)); + + dev->private_data = kbd; + + kbd->dev = input_allocate_device(); + kbd->dev->evbit[0] = BIT(EV_KEY) | BIT(EV_REP); + + for (i = 0; i < 255; i++) + set_bit(dc_kbd_keycode[i], kbd->dev->keybit); + + clear_bit(0, kbd->dev->keybit); + + kbd->dev->private = kbd; + + kbd->dev->name = dev->product_name; + kbd->dev->id.bustype = BUS_MAPLE; + + input_register_device(kbd->dev); + + maple_getcond_callback(dev, dc_kbd_callback, (62 * HZ) / 1000, + MAPLE_FUNC_KEYBOARD); + + printk(KERN_INFO "input: keyboard(0x%lx): %s\n", data, + kbd->dev->name); + + return 0; +} + +static void dc_kbd_disconnect(struct maple_device *dev) +{ + struct dc_kbd *kbd = dev->private_data; + + input_unregister_device(kbd->dev); + kfree(kbd); +} + +/* 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", + .bus = &maple_bus_type, + .probe = probe_maple_kbd,}, +}; + +static int __init dc_kbd_init(void) +{ + int retval; + retval = driver_register(&dc_kbd_driver.drv); + if (retval) + return retval; + printk(KERN_INFO "Registered Dreamcast keyboard driver\n"); + return 0; +} + +static void __exit dc_kbd_exit(void) +{ + driver_unregister(&dc_kbd_driver.drv); +} + +module_init(dc_kbd_init); +module_exit(dc_kbd_exit); diff --git a/drivers/sh/Makefile b/drivers/sh/Makefile index 8a14389..f0a1f4f 100644 --- a/drivers/sh/Makefile +++ b/drivers/sh/Makefile @@ -3,4 +3,5 @@ # obj-$(CONFIG_SUPERHYWAY) += superhyway/ +obj-$(CONFIG_MAPLE) += maple/ 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 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 + */ + +#include <linux/init.h> +#include <linux/device.h> +#include <linux/moduleparam.h> +#include <linux/interrupt.h> +#include <linux/list.h> +#include <linux/io.h> +#include <linux/maple.h> +#include <asm/cacheflush.h> +#include <asm/dma.h> +#include <asm/dreamcast/sysasic.h> + +MODULE_AUTHOR("Paul Mundt, MR Brown, Adrian McMenamin"); +MODULE_DESCRIPTION("Maple bus driver for Dreamcast"); +MODULE_LICENSE("GPL"); +MODULE_SUPPORTED_DEVICE("{{SEGA, Dreamcast/Maple}}"); + +static void maple_dma_handler(struct work_struct *work); +static void maple_vblank_handler(struct work_struct *work); + +static DECLARE_WORK(maple_dma_process, maple_dma_handler); +static DECLARE_WORK(maple_vblank_process, maple_vblank_handler); + +static LIST_HEAD(maple_waitq); +static LIST_HEAD(maple_sentq); + +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; + + +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); + +static void maple_release_device(struct device *dev) +{ + if (likely(dev->type)) { + if (likely(dev->type->name)) + 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); +} + +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); + + return mq; +} + +static struct maple_device *maple_alloc_dev(int port, int unit) +{ + struct maple_device *dev; + + dev = kzalloc(sizeof(*dev), GFP_KERNEL); + if (unlikely(!dev)) + return NULL; + + dev->port = port; + dev->unit = unit; + dev->mq = maple_allocq(dev); + + if (unlikely(!dev->mq)) { + kfree(dev); + return NULL; + } + + return dev; +} + +static void maple_free_dev(struct maple_device *mdev) +{ + if (!mdev) + return; + if (likely(mdev->registered)) + maple_release_device(&mdev->dev); + kfree(mdev->mq); + kfree(mdev); +} + +static int maple_dma_done(void) +{ + return (ctrl_inl(MAPLE_STATE) & 1) == 0; +} + +static void maple_build_block(struct mapleq *mq) +{ + int port, unit, from, to, len; + unsigned long *lsendbuf = mq->sendbuf; + + port = mq->dev->port & 3; + unit = mq->dev->unit; + len = mq->length; + + from = port << 6; + to = (port << 6) | (unit > 0 ? (1 << (unit - 1)) & 0x1f : 0x20); + + *maple_lastptr &= 0x7fffffff; + maple_lastptr = maple_sendptr; + + *maple_sendptr++ = (port << 16) | len | 0x80000000; + *maple_sendptr++ = PHYSADDR(mq->recvbuf); + *maple_sendptr++ = + mq->command | (to << 8) | (from << 16) | (len << 24); + + while (len-- > 0) + *maple_sendptr++ = *lsendbuf++; +} + +void maple_send(void) +{ + int i; + int maple_packets; + struct mapleq *mq, *nmq; + unsigned long flags; + + if (!list_empty(&maple_sentq)) + return; + if (list_empty(&maple_waitq) || !maple_dma_done()) + return; + maple_packets = 0; + maple_sendptr = maple_lastptr = maple_sendbuf; + list_for_each_entry_safe(mq, nmq, &maple_waitq, list) { + maple_build_block(mq); + local_irq_save(flags); + list_move(&mq->list, &maple_sentq); + local_irq_restore(flags); + if (unlikely(maple_packets++ > MAPLE_MAXPACKETS)) + break; + } + if (maple_packets > 0) { + for (i = 0; i < (1 << MAPLE_DMA_PAGES); i++) + dma_cache_wback_inv(maple_sendbuf + i * PAGE_SIZE, + PAGE_SIZE); + } +} + +static int attach_matching_maple_driver(struct device_driver *driver, + void *devptr) +{ + struct maple_driver *maple_drv; + struct maple_device *mdev; + + mdev = devptr; + maple_drv = to_maple_driver(driver); + if (unlikely + (mdev->devinfo.function & be32_to_cpu(maple_drv->function))) { + if (likely(maple_drv->connect(mdev) == 0)) { + mdev->driver = maple_drv; + return 1; + } + } + return 0; +} + +static void maple_detach_driver(struct maple_device *dev) +{ + if (!dev) + return; + if (likely(dev->driver)) { + if (likely(dev->driver->disconnect)) + dev->driver->disconnect(dev); + } + dev->driver = NULL; + if (likely(&dev->registered)) + device_unregister(&dev->dev); + dev->registered = 0; + maple_free_dev(dev); +} + +static void maple_attach_driver(struct maple_device *dev) +{ + char *p; + + char *recvbuf; + unsigned long function; + int matched, retval; + + recvbuf = dev->mq->recvbuf; + memcpy(&dev->devinfo, recvbuf + 4, sizeof(dev->devinfo)); + memcpy(dev->product_name, dev->devinfo.product_name, 30); + memcpy(dev->product_licence, dev->devinfo.product_licence, 60); + dev->product_name[29] = '\0'; + dev->product_licence[59] = '\0'; + + for (p = dev->product_name + 28; dev->product_name <= p; p--) + if (*p == ' ') + *p = '\0'; + else + break; + + for (p = dev->product_licence + 58; dev->product_licence <= p; p--) + if (*p == ' ') + *p = '\0'; + else + break; + + function = be32_to_cpu(dev->devinfo.function); + + if (unlikely(function == 0xFFFFFFFF)) { + /* Do this silently - as not a real device */ + function = 0; + dev->driver = &maple_basic_driver; + sprintf(dev->dev.bus_id, "%d:0.port", dev->port); + } else { + printk(KERN_INFO + "Maple bus at (%d, %d): Connected function 0x%lX\n", + dev->port, dev->unit, function); + + matched = + bus_for_each_drv(&maple_bus_type, NULL, dev, + attach_matching_maple_driver); + + if (matched == 0) { + /* Driver does not exist yet */ + printk(KERN_INFO + "No maple driver found for this device\n"); + dev->driver = &maple_basic_driver; + } + + sprintf(dev->dev.bus_id, "%d:0%d.%lX", dev->port, + dev->unit, function); + } + dev->function = function; + dev->dev.bus = &maple_bus_type; + dev->dev.parent = &maple_bus; + dev->dev.release = &maple_release_device; + retval = device_register(&dev->dev); + if (retval) { + printk(KERN_INFO + "Maple bus: Attempt to register device (%x, %x) failed.\n", + dev->port, dev->unit); + maple_free_dev(dev); + } + dev->registered = 1; + +} + +static int detach_maple_device(struct device *device, void *portptr) +{ + struct device_specify *ds; + struct maple_device *mdev; + + ds = portptr; + mdev = to_maple_dev(device); + if (unlikely(mdev->port == ds->port && mdev->unit == ds->unit)) + return 1; + return 0; +} +static int setup_maple_commands(struct device *device, void *ignored) +{ + struct maple_device *maple_dev = to_maple_dev(device); + + if (unlikely(maple_dev->event)) + return 0; + if (likely(maple_dev->interval > 0)) { + if (likely(jiffies > maple_dev->when)) { + maple_dev->when = jiffies + maple_dev->interval; + maple_dev->mq->command = MAPLE_COMMAND_GETCOND; + maple_dev->mq->sendbuf = &maple_dev->function; + maple_dev->mq->length = 1; + maple_add_packet(maple_dev->mq); + INIT_LIST_HEAD(&maple_sentq); + } + } else { + if (jiffies >= maple_pnp_time) { + maple_dev->mq->command = MAPLE_COMMAND_DEVINFO; + maple_dev->mq->length = 0; + maple_add_packet(maple_dev->mq); + INIT_LIST_HEAD(&maple_sentq); + + } + } + + return 0; +} + +static void maple_vblank_handler(struct work_struct *work) +{ + bus_for_each_dev(&maple_bus_type, NULL, NULL, + setup_maple_commands); + if (jiffies >= maple_pnp_time) + maple_pnp_time = jiffies + MAPLE_PNP_INTERVAL; + maple_send(); +} + +static void maple_map_subunits(struct maple_device *mdev, int submask) +{ + int retval, k, devcheck; + struct maple_device *mdev_add; + struct device_specify ds; + + for (k = 0; k < 5; k++) { + ds.port = mdev->port; + ds.unit = k + 1; + retval = + bus_for_each_dev(&maple_bus_type, NULL, &ds, + detach_maple_device); + if (retval) { + submask = submask >> 1; + continue; + } + devcheck = submask & 0x01; + if (unlikely(devcheck)) { + mdev_add = maple_alloc_dev(mdev->port, k + 1); + mdev_add->mq->command = MAPLE_COMMAND_DEVINFO; + mdev_add->mq->length = 0; + maple_add_packet(mdev_add->mq); + scanning = 1; + } + submask = submask >> 1; + } +} + +static void maple_clean_submap(struct maple_device *mdev) +{ + int killbit; + + killbit = (mdev->unit > 0 ? (1 << (mdev->unit - 1)) & 0x1f : 0x20); + killbit = ~killbit; + killbit &= 0xFF; + subdevice_map[mdev->port] = subdevice_map[mdev->port] & killbit; +} + +static void maple_dma_handler(struct work_struct *work) +{ + struct mapleq *mq; + struct maple_device *dev; + char *recvbuf; + char submask; + int code; + + /* Be extra cautious and check new DMA cycle not already underway */ + if (!list_empty(&maple_sentq) && maple_dma_done()) { + list_for_each_entry(mq, &maple_sentq, list) { + recvbuf = mq->recvbuf; + code = recvbuf[0]; + dev = mq->dev; + + switch (code) { + case MAPLE_RESPONSE_NONE: + if (unlikely(!started)) { + maple_attach_driver(dev); + break; + } + if (unlikely(dev->unit != 0)) + maple_detach_driver(dev); + maple_clean_submap(dev); + break; + + case MAPLE_RESPONSE_DEVINFO: + dev->event = 0; + if ((unlikely(!started)) + || (unlikely(scanning == 2))) { + maple_attach_driver(dev); + break; + } + if (likely(dev->unit == 0)) { + submask = recvbuf[2] & 0x1F; + /* Test if subunits have been added or removed */ + if (unlikely + (submask ^ + subdevice_map[dev->port])) { + maple_map_subunits(dev, + submask); + subdevice_map[dev->port] = + submask; + } + } + break; + + case MAPLE_RESPONSE_DATATRF: + if (dev->callback) + dev->callback(mq); + break; + + default: + break; + } + } + + INIT_LIST_HEAD(&maple_sentq); + + if (unlikely(scanning == 1)) { + maple_send(); + scanning = 2; + } else + scanning = 0; + + if (unlikely(started == 0)) + started = 1; + } +} + +static irqreturn_t maplebus_dma_interrupt(int irq, void *dev_id) +{ + /* Load everything into the bottom half */ + schedule_work(&maple_dma_process); + return IRQ_HANDLED; +} + +static irqreturn_t maplebus_vblank_interrupt(int irq, void *dev_id) +{ + schedule_work(&maple_vblank_process); + return IRQ_HANDLED; +} + +static struct irqaction maple_dma_irq = { + .name = "maple bus DMA handler", + .handler = maplebus_dma_interrupt, + .flags = IRQF_SHARED, +}; + +static struct irqaction maple_vblank_irq = { + .name = "maple bus VBLANK handler", + .handler = maplebus_vblank_interrupt, + .flags = IRQF_SHARED, +}; + +static void maplebus_init_hardware(void) +{ + ctrl_outl(MAPLE_MAGIC, MAPLE_RESET); + /* set trig type to 0 for software trigger, 1 for hardware (VBLANK) */ + ctrl_outl(1, MAPLE_TRIGTYPE); + ctrl_outl(MAPLE_2MBPS | MAPLE_TIMEOUT(50000), MAPLE_SPEED); + ctrl_outl(PHYSADDR(maple_sendbuf), MAPLE_DMAADDR); + ctrl_outl(1, MAPLE_ENABLE); +} + +static int maple_set_dma_interrupt_handler(void) +{ + return setup_irq(HW_EVENT_MAPLE_DMA, &maple_dma_irq); +} + +static int maple_set_vblank_interrupt_handler(void) +{ + return setup_irq(HW_EVENT_VSYNC, &maple_vblank_irq); +} + + +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; +} + +static int match_maple_bus_driver(struct device *devptr, + struct device_driver *drvptr) +{ + struct maple_driver *maple_drv; + struct maple_device *maple_dev; + + 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; +} + +static int maple_bus_uevent(struct device *dev, char **envp, + int num_envp, char *buffer, int buffer_size) +{ + return 0; +} + +static void maple_bus_release(struct device *dev) +{ + printk(KERN_INFO "Releasing maple bus device\n"); +} + +static struct maple_driver maple_basic_driver = { + .drv = { + .name = "Maple_bus_basic_driver", + .bus = &maple_bus_type,}, +}; + +struct bus_type maple_bus_type = { + .name = "maple", + .match = match_maple_bus_driver, + .uevent = maple_bus_uevent, +}; + +EXPORT_SYMBOL(maple_bus_type); + +static struct device maple_bus = { + .bus_id = "maple0", + .release = maple_bus_release, +}; + +static int __init maple_bus_init(void) +{ + int retval, i; + struct maple_device *mdev; + ctrl_outl(0, MAPLE_STATE); + + 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; + mdev->mq->command = MAPLE_COMMAND_DEVINFO; + mdev->mq->length = 0; + maple_add_packet(mdev->mq); + subdevice_map[i] = 0; + } + + /* setup maplebus hardware */ + maplebus_init_hardware(); + + /* initial detection */ + maple_send(); + + + maple_pnp_time = 0; + + return 0; + +cleanup_bothirqs: + free_irq(HW_EVENT_VSYNC, 0); + +cleanup_irq: + free_irq(HW_EVENT_MAPLE_DMA, 0); + +cleanup_dma: + free_pages((unsigned long) maple_sendbuf, MAPLE_DMA_PAGES); + +cleanup_basic: + driver_unregister(&maple_basic_driver.drv); + +cleanup_bus: + bus_unregister(&maple_bus_type); + +cleanup_device: + device_unregister(&maple_bus); + +cleanup: + printk(KERN_INFO "Maple bus registration failed\n"); + return retval; +} + +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); +} + +/* use init call to ensure bus is registered ahead of devices */ +fs_initcall(maple_bus_init); +module_exit(maple_bus_exit); diff --git a/drivers/video/pvr2fb.c b/drivers/video/pvr2fb.c index 7d6c298..13de07f 100644 --- a/drivers/video/pvr2fb.c +++ b/drivers/video/pvr2fb.c @@ -890,7 +890,7 @@ static int __init pvr2fb_dc_init(void) pvr2_fix.mmio_start = 0xa05f8000; /* registers start here */ pvr2_fix.mmio_len = 0x2000; - if (request_irq(HW_EVENT_VSYNC, pvr2fb_interrupt, 0, + if (request_irq(HW_EVENT_VSYNC, pvr2fb_interrupt, IRQF_SHARED, "pvr2 VBL handler", fb_info)) { return -EBUSY; } 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 /* * Values describing the status of a force-feedback effect diff --git a/include/linux/maple.h b/include/linux/maple.h new file mode 100644 index 0000000..72d31ca --- /dev/null +++ b/include/linux/maple.h @@ -0,0 +1,125 @@ +/** + * maple.h + * + * porting to 2.6 driver model + * copyright Adrian McMenamin, 2007 + * + */ + +extern struct bus_type maple_bus_type; + +#define MAPLE_PORTS 4 +#define MAPLE_PNP_INTERVAL HZ +#define MAPLE_MAXPACKETS 8 +#define MAPLE_DMA_ORDER 14 +#define MAPLE_DMA_SIZE (1 << MAPLE_DMA_ORDER) +#define MAPLE_DMA_PAGES ((MAPLE_DMA_ORDER > PAGE_SHIFT) ? MAPLE_DMA_ORDER - PAGE_SHIFT : 0) + + +/* Maple Bus registers */ + +#define MAPLE_BASE 0xa05f6c00 +#define MAPLE_DMAADDR (MAPLE_BASE+0x04) +#define MAPLE_TRIGTYPE (MAPLE_BASE+0x10) +#define MAPLE_ENABLE (MAPLE_BASE+0x14) +#define MAPLE_STATE (MAPLE_BASE+0x18) +#define MAPLE_SPEED (MAPLE_BASE+0x80) +#define MAPLE_RESET (MAPLE_BASE+0x8c) + +#define MAPLE_MAGIC 0x6155404f +#define MAPLE_2MBPS 0 +#define MAPLE_TIMEOUT(n) ((n)<<16) + + +/* Maple Bus command and response codes */ + +#define MAPLE_RESPONSE_FILEERR -5 +#define MAPLE_RESPONSE_AGAIN -4 /* request should be retransmitted */ +#define MAPLE_RESPONSE_BADCMD -3 +#define MAPLE_RESPONSE_BADFUNC -2 +#define MAPLE_RESPONSE_NONE -1 /* unit didn't respond at all */ +#define MAPLE_COMMAND_DEVINFO 1 +#define MAPLE_COMMAND_ALLINFO 2 +#define MAPLE_COMMAND_RESET 3 +#define MAPLE_COMMAND_KILL 4 +#define MAPLE_RESPONSE_DEVINFO 5 +#define MAPLE_RESPONSE_ALLINFO 6 +#define MAPLE_RESPONSE_OK 7 +#define MAPLE_RESPONSE_DATATRF 8 +#define MAPLE_COMMAND_GETCOND 9 +#define MAPLE_COMMAND_GETMINFO 10 +#define MAPLE_COMMAND_BREAD 11 +#define MAPLE_COMMAND_BWRITE 12 +#define MAPLE_COMMAND_SETCOND 14 + + +/* Function codes */ + +#define MAPLE_FUNC_CONTROLLER 0x001 +#define MAPLE_FUNC_MEMCARD 0x002 +#define MAPLE_FUNC_LCD 0x004 +#define MAPLE_FUNC_CLOCK 0x008 +#define MAPLE_FUNC_MICROPHONE 0x010 +#define MAPLE_FUNC_ARGUN 0x020 +#define MAPLE_FUNC_KEYBOARD 0x040 +#define MAPLE_FUNC_LIGHTGUN 0x080 +#define MAPLE_FUNC_PURUPURU 0x100 +#define MAPLE_FUNC_MOUSE 0x200 + + +struct mapleq { + struct list_head list; + struct maple_device *dev; + void *sendbuf, *recvbuf; + unsigned char command, length; + unsigned char buf[1024 + 32]; +}; + +struct maple_devinfo { + unsigned long function; + unsigned long function_data[3]; + unsigned char area_code; + unsigned char connector_directon; + char product_name[30]; + char product_licence[60]; + unsigned short standby_power; + unsigned short max_power; +}; + +struct maple_device { + struct maple_driver *driver; + struct mapleq *mq; + void *private_data; + void (*callback) (struct mapleq * mq); + unsigned long when, interval, function; + int event; + struct maple_devinfo devinfo; + unsigned char port, unit; + char product_name[32]; + char product_licence[64]; + int registered; + struct device dev; +}; + +struct maple_driver { + unsigned long function; + int (*connect) (struct maple_device * dev); + void (*disconnect) (struct maple_device * dev); + struct device_driver drv; +}; + +struct device_specify { + int port; + int unit; +}; + + +void maple_getcond_callback(struct maple_device *dev, + void (*callback) (struct mapleq * mq), + unsigned long interval, + unsigned long function); + +void maple_setup_port_rescan(struct maple_device *mdev); + +#define to_maple_dev(n) container_of(n, struct maple_device, dev) +#define to_maple_driver(n) container_of(n, struct maple_driver, drv) |
From: Mike F. <va...@ge...> - 2007-08-29 01:35:05
|
On Tuesday 28 August 2007, Adrian McMenamin wrote: > Please find this patch below - they said it would never be done! very cool! general note ... looks like your e-mail client word wrapped things and you= =20 took the "80 cols" rule a little too seriously (or you just ran Lindent and= =20 didnt check the results) ... imo, breaking across things like pointers to=20 structures is excessive: kbd-> old[i] be nice if more funcs had overview comments, but i'd make that lower=20 priority ;) > --- a/arch/sh/Kconfig > +++ b/arch/sh/Kconfig > @@ -694,8 +694,20 @@ config CF_AREA5 > config CF_AREA6 > bool "Area6" > > + > endchoice spurious newline > +config MAPLE > + bool "Maple Bus Support" > + depends on SH_DREAMCAST > + ---help--- most people just use "help" i think, but not it really matters ;) > + The Maple Bus is SEGA's serial communication bus for peripherals - > + a bit like USB for your Dreamcast. Without this bus support you > + won't be able to get your Dreamcast keyboard etc to work, so you > + probably want to say 'Y' here, though if you are just running > + the Dreamcast via a serial cable or network connection you may > + not need this. help text should be indented via a tab and two spaces ... also, when=20 describing the Y/N, the wording tends to be inverted from what you have her= e,=20 so something like: The Maple Bus is SEGA's serial communication bus for peripherals - a bit li= ke=20 USB for your Dreamcast. Your Dreamcast keyboard/mouse/etc... need this in= =20 order to work. If you want to run a serial cable or network connection, yo= u=20 may not need this. If unsure, say 'Y' here. > --- a/drivers/input/keyboard/Kconfig > +++ b/drivers/input/keyboard/Kconfig > +config KEYBOARD_MAPLE > + tristate "Maple bus keyboard" > + depends on SH_DREAMCAST && MAPLE > + help > + Say Y here if you have a DreamCast console running Linux and have funny caps in Dreamcast > --- /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 you mean GPL-2 [or later] ? > +MODULE_AUTHOR > + ("YAEGASHI Takeshi <t...@ke...>, Adrian McMenamin > <ad...@mc..."); that's a weird wrapping > +static void dc_scan_kbd(struct dc_kbd *kbd) > ... > + printk > + ("Unknown key (scancode %#x) released.", > + kbd->old[i]); > ... > + printk > + ("Unknown key (scancode %#x) pressed.", missing kern log level > +static int dc_kbd_connect(struct maple_device *dev) > ... > + kbd =3D kmalloc(sizeof(struct dc_kbd), GFP_KERNEL); > + if (unlikely(!kbd)) > + return -1; you mean -ENOMEM ? > + memset(kbd, 0, sizeof(struct dc_kbd)); just use kzalloc() rather than kmalloc(); memset(0); > +static struct maple_driver dc_kbd_driver =3D { > + .function =3D MAPLE_FUNC_KEYBOARD, > + .connect =3D dc_kbd_connect, > + .disconnect =3D dc_kbd_disconnect, > + .drv =3D { > + .name =3D "Dreamcast_keyboard", > + .bus =3D &maple_bus_type, > + .probe =3D probe_maple_kbd,}, that last "}," shouldnt be cuddled like that > --- /dev/null > +++ b/drivers/sh/maple/maplebus.c > +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); > +} ugh, this is what locking mechanisms (like semaphores) are for > +static void maple_detach_driver(struct maple_device *dev) > +{ > + if (!dev) > + return; > + if (likely(dev->driver)) { > + if (likely(dev->driver->disconnect)) > + dev->driver->disconnect(dev); > + } some people would just make this one if statement ... > + dev->driver =3D NULL; > + if (likely(&dev->registered)) mmm is that even possible to be NULL ? > +static void maple_attach_driver(struct maple_device *dev) > +{ > + char *p; > + > + char *recvbuf; > + unsigned long function; > + int matched, retval; > + > + recvbuf =3D dev->mq->recvbuf; > + memcpy(&dev->devinfo, recvbuf + 4, sizeof(dev->devinfo)); > + memcpy(dev->product_name, dev->devinfo.product_name, 30); > + memcpy(dev->product_licence, dev->devinfo.product_licence, 60); > + dev->product_name[29] =3D '\0'; > + dev->product_licence[59] =3D '\0'; isnt there a snprintf() func ? > + for (p =3D dev->product_name + 28; dev->product_name <=3D p; p--) > + if (*p =3D=3D ' ') > + *p =3D '\0'; > + else > + break; > + > + for (p =3D dev->product_licence + 58; dev->product_licence <=3D p; p--) > + if (*p =3D=3D ' ') > + *p =3D '\0'; > + else > + break; be nice if these string constants didnt exist ... maybe abuse sizeof() so o= nly=20 the structure definition has the actual sizes ... > +static struct maple_driver maple_basic_driver =3D { > + .drv =3D { > + .name =3D "Maple_bus_basic_driver", > + .bus =3D &maple_bus_type,}, funky cuddling of }, again > +static struct device maple_bus =3D { > + .bus_id =3D "maple0", > + .release =3D maple_bus_release, > +}; what's that 0 for ? > +static int __init maple_bus_init(void) > ... > +cleanup: > + printk(KERN_INFO "Maple bus registration failed\n"); shouldnt that be a WARN or ERR or somethin ? > +/* use init call to ensure bus is registered ahead of devices */ > +fs_initcall(maple_bus_init); thought subsys_initcall() was what you wanted ? > +module_exit(maple_bus_exit); not sure how useful this is since the func already has the __exit attribute= =20 and you cant build the code as a module ... it doesnt have a detrimental=20 affect though > --- a/drivers/video/pvr2fb.c > +++ b/drivers/video/pvr2fb.c > @@ -890,7 +890,7 @@ static int __init pvr2fb_dc_init(void) > pvr2_fix.mmio_start =3D 0xa05f8000; /* registers start here */ > pvr2_fix.mmio_len =3D 0x2000; > > - if (request_irq(HW_EVENT_VSYNC, pvr2fb_interrupt, 0, > + if (request_irq(HW_EVENT_VSYNC, pvr2fb_interrupt, IRQF_SHARED, > "pvr2 VBL handler", fb_info)) { > return -EBUSY; > } mmm not relevant to this patch ? > --- /dev/null > +++ b/include/linux/maple.h > +/* Maple Bus command and response codes */ > + > +#define MAPLE_RESPONSE_FILEERR -5 > +#define MAPLE_RESPONSE_AGAIN -4 /* request should be retransmitted */ > +#define MAPLE_RESPONSE_BADCMD -3 > +#define MAPLE_RESPONSE_BADFUNC -2 > +#define MAPLE_RESPONSE_NONE -1 /* unit didn't respond at all */ > +#define MAPLE_COMMAND_DEVINFO 1 > +#define MAPLE_COMMAND_ALLINFO 2 > +#define MAPLE_COMMAND_RESET 3 > +#define MAPLE_COMMAND_KILL 4 > +#define MAPLE_RESPONSE_DEVINFO 5 > +#define MAPLE_RESPONSE_ALLINFO 6 > +#define MAPLE_RESPONSE_OK 7 > +#define MAPLE_RESPONSE_DATATRF 8 > +#define MAPLE_COMMAND_GETCOND 9 > +#define MAPLE_COMMAND_GETMINFO 10 > +#define MAPLE_COMMAND_BREAD 11 > +#define MAPLE_COMMAND_BWRITE 12 > +#define MAPLE_COMMAND_SETCOND 14 if actual numeric value doesnt have meaning, an enum might be appropriate .= =2E. =2Dmike |
From: Adrian M. <ad...@ne...> - 2007-08-29 07:02:27
|
On Tue, 2007-08-28 at 21:36 -0400, Mike Frysinger wrote: > On Tuesday 28 August 2007, Adrian McMenamin wrote: > > Please find this patch below - they said it would never be done! > > very cool! > Pleased to say most of your comments related to bits of code I copied from the old drivers and not the new stuff I'd written. Will refactor and resend in due course. Please note, though, that the PVR2 change is essential. Wothout it the maplebus driver will fail as it will not be able to hook the VBL interrupt. |
From: Paul M. <le...@li...> - 2007-08-29 07:08:26
|
On Wed, Aug 29, 2007 at 08:01:58AM +0100, Adrian McMenamin wrote: > On Tue, 2007-08-28 at 21:36 -0400, Mike Frysinger wrote: > > On Tuesday 28 August 2007, Adrian McMenamin wrote: > > > Please find this patch below - they said it would never be done! > > > > very cool! > > > Pleased to say most of your comments related to bits of code I copied > from the old drivers and not the new stuff I'd written. > > Will refactor and resend in due course. > > Please note, though, that the PVR2 change is essential. Wothout it the > maplebus driver will fail as it will not be able to hook the VBL > interrupt. That's fine, but as Mike pointed out, it's just unrelated to the rest of the code. Please send the pvr2 thing by itself, the maple bits can be applied on top of that as they're conceptually unrelated. |
From: Mike F. <va...@ge...> - 2007-08-29 11:25:27
|
On Wednesday 29 August 2007, Adrian McMenamin wrote: > On Tue, 2007-08-28 at 21:36 -0400, Mike Frysinger wrote: > > On Tuesday 28 August 2007, Adrian McMenamin wrote: > > > Please find this patch below - they said it would never be done! > > > > very cool! > > Pleased to say most of your comments related to bits of code I copied > from the old drivers and not the new stuff I'd written. > > Will refactor and resend in due course. > > Please note, though, that the PVR2 change is essential. Wothout it the > maplebus driver will fail as it will not be able to hook the VBL > interrupt. might want to include that in your commit message then ;) -mike |
From: Adrian M. <ad...@ne...> - 2007-08-29 09:27:16
|
On Wed, August 29, 2007 2:36 am, Mike Frysinger wrote: > On Tuesday 28 August 2007, Adrian McMenamin wrote: > general note ... looks like your e-mail client word wrapped things and you > took the "80 cols" rule a little too seriously (or you just ran Lindent > and > didnt check the results) ... imo, breaking across things like pointers to > structures is excessive: > kbd-> > old[i] > I was a bit OTT on that, it's true. And I'll post the refactored patch as an attachment next time. > be nice if more funcs had overview comments, but i'd make that lower > priority ;) > Will add some. A couple of points that clarification would be useful on... >> --- /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 > > you mean GPL-2 [or later] ? > Actually, I don't know. The original just said "GPL" and as it was submitted to the kernel we can assume it's at least v2 but there was no additional specification given. Shall I just leave it as is? >> +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,}, > > that last "}," shouldnt be cuddled like that > Really? It's what i've used before and it has been accepted into the kernel. Should it be .probe = probe_maple_kbd, }, then? >> +/* use init call to ensure bus is registered ahead of devices */ >> +fs_initcall(maple_bus_init); > > thought subsys_initcall() was what you wanted ? > I thought it would be better behaved to use a later call and as this is fs related (sys) it seemed appropriate. If this a faux pas I can change it... |
From: Mike F. <va...@ge...> - 2007-08-29 11:27:37
|
On Wednesday 29 August 2007, Adrian McMenamin wrote: > On Wed, August 29, 2007 2:36 am, Mike Frysinger wrote: > > On Tuesday 28 August 2007, Adrian McMenamin wrote: > >> --- /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 > > > > you mean GPL-2 [or later] ? > > Actually, I don't know. The original just said "GPL" and as it was > submitted to the kernel we can assume it's at least v2 but there was no > additional specification given. Shall I just leave it as is? if the old file only said "Licensed under the GPL" then i guess you dont ha= ve=20 much choice without contacting the people who wrote the original > >> +static struct maple_driver dc_kbd_driver =3D { > >> + .function =3D MAPLE_FUNC_KEYBOARD, > >> + .connect =3D dc_kbd_connect, > >> + .disconnect =3D dc_kbd_disconnect, > >> + .drv =3D { > >> + .name =3D "Dreamcast_keyboard", > >> + .bus =3D &maple_bus_type, > >> + .probe =3D probe_maple_kbd,}, > > > > that last "}," shouldnt be cuddled like that > > Really? It's what i've used before and it has been accepted into the > kernel. > > Should it be > .probe =3D probe_maple_kbd, > }, > > then? =2Edev =3D { .moo =3D cow, .foo =3D bar, }, > >> +/* use init call to ensure bus is registered ahead of devices */ > >> +fs_initcall(maple_bus_init); > > > > thought subsys_initcall() was what you wanted ? > > I thought it would be better behaved to use a later call and as this is fs > related (sys) it seemed appropriate. If this a faux pas I can change it... Paul would know =2Dmike |
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 :-) |
From: Adrian M. <ad...@ne...> - 2007-08-30 07:09:53
|
On Thu, 2007-08-30 at 15:39 +0900, Paul Mundt wrote: > 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. > I will have a stab at doing this. More generally, a lot of your comments are directed at code I inherited and I'll look at fixing them too. One or two other things though... > > +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. > I'm not sure I follow what you mean - are you saying I shouldn't initialise it with the bus type set? > > 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? > I can do it as a module I suppose... > > 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. > Yes, and none of you left any copyright messages. Very poor show! > > +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. > Actually, I'm not sure the locking is needed anymore at all - I inherited that. But assuming a kernel thread will not be scheduled against itself (am I safe to do that?), I cannot see how this could executed concurrently with other code that manipulates the queue. > > + function = be32_to_cpu(dev->devinfo.function); > > + > What exactly is the point of this? The function is big endian, really? > Err, yes. > 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? > Device model takes care of that ... the above is pretty redundant and should go. > > + > > +static void maple_bus_release(struct device *dev) > > +{ > > + printk(KERN_INFO "Releasing maple bus device\n"); > > +} > > + > Kill this. > I thought busses where required to have them ... that's what my book says. Oh well. > > > 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? > I believe so, yes. > > +/* Maple Bus command and response codes */ > > +/* Function codes */ > > These can use enums and opaque types. > These are real numbers though. > > +struct mapleq { > > +struct maple_devinfo { > [snip] > > > + char product_licence[60]; > > British variable names, if there's nothing in CodingStyle about that, > there should be :-) Not being British, what ever my passport says, I wouldn't know :) |
From: Paul M. <le...@li...> - 2007-08-30 07:20:22
|
On Thu, Aug 30, 2007 at 08:09:21AM +0100, Adrian McMenamin wrote: > On Thu, 2007-08-30 at 15:39 +0900, Paul Mundt wrote: > > In the long run these patches will have to be submitted through different > > maintainers, so the earlier they are split logically, the better. > > I will have a stab at doing this. More generally, a lot of your comments > are directed at code I inherited and I'll look at fixing them too. One > or two other things though... > The origin of the code in question has little to do with what needs to be fixed. A lot of it was crap from the beginning, that doesn't mean it doesn't have to be fixed up before it can be merged. > > > + .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. > > > > I'm not sure I follow what you mean - are you saying I shouldn't > initialise it with the bus type set? > Yes, you should have a maple registration function that sets the bus type for you. Most of the other busses do this. > > > --- /dev/null > > > +++ b/drivers/sh/maple/Makefile > > > @@ -0,0 +1,3 @@ > > > +#Makefile for Maple Bus > > > + > > > +obj-y := maplebus.o > > > > No module goodness? > > I can do it as a module I suppose... > Well, it can be a tristate, so obj-y is artificially limiting. > > > +/* 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. > > Yes, and none of you left any copyright messages. Very poor show! > I suppose no one wanted to be associated too closely with that particular bit of code.. at least that's the excuse I'm sticking with. ;-) This is probably something you can grovel through the CVS history for, though. > > > +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. > > Actually, I'm not sure the locking is needed anymore at all - I > inherited that. But assuming a kernel thread will not be scheduled > against itself (am I safe to do that?), I cannot see how this could > executed concurrently with other code that manipulates the queue. > Yes, it should be safe to drop the irq disabling around this. Remember you also have safe iterators if you are concerned about something disappearing from the list while you are looping over it. > > > + > > > +static void maple_bus_release(struct device *dev) > > > +{ > > > + printk(KERN_INFO "Releasing maple bus device\n"); > > > +} > > > + > > Kill this. > > > > I thought busses where required to have them ... that's what my book > says. Oh well. > The release function, yes, not the printk(). > > > +/* Maple Bus command and response codes */ > > > +/* Function codes */ > > > > These can use enums and opaque types. > > These are real numbers though. > As are enums. enums are also generally more readable for these sorts of things, too. |