Menu
▾
▴
Re: [PATCH] Add support for generic maple bus and for maple bus keyboard
|
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
|
