Subject: Re: Sharp CE-RH2 remote kernel driver - msg#00079

Previous Message by Date: click to view message preview

Re: [Openzaurus-devel] Sharp CE-RH2 remote kernel driver

On Thu, 2007-01-18 at 12:31 -0800, Justin Patrin wrote: > I finally got another hint and have a working kernel driver for the > CE-RH2 in-line audio remote. It outputs the correct keys. Both xev and > showkey only show 3 of the keys, the other 4 show some strange output. Any idea why? I presume the kernel is doing the right thing and that's some kind of userspace issue? > However, gizmod (compiled on the Z due to cross-compile problems) sees > all 7 keys just fine and some quick hacking has it controlling mpd for > me (although volume control is still complicated). Still complicated? > There are still some issues with it, of course, it was pretty much > copied from the 2.4 kernel and will need some fix-ups to be merged > upstream but I'm willing to work on it if I can get some pointers. > > There is a problem with the IRQ handling as well as I see this in dmesg: > enable_irq(45) unbalanced from bf1483e0 Keep in mind the effects of enable_irq and disable_irq are cumulative i.e. if you call disable_irq 4 times, you have to call enable_irq 4 times too. It may well be better to leave the interrupt handler enabled all the time, I'm undecided... > Any help in making this patch better would be appreciated. I've inlined the code below and commented LKML style... > --- /dev/null > +++ linux-2.6.17/drivers/input/keyboard/sharpsl_rc.c > @@ -0,0 +1,529 @@ > +/* > +12:13 < RP> JustinP: There is going to be an issue with it fighting the > sound system for the headphone interrupt > +12:14 < RP> JustinP: I'm about to rewrite the headphone interrupt handling > anyway so the best soution might be to just comment it out in the sound code Its been rewritten and isn't going to change now. See below. > + > +#define DPRINTK(fmt, args...) //printk(KERN_ERR "sharpsl_rc, %s: " > fmt,__FUNCTION__,## args) > + > +#define MPRINTK(fmt, args...) //printk(fmt,## args) > + > +#define MDPRINTK(fmt, args...) printk(fmt,## args) These should all be replaced with things like dev_dbg() and dev_warn(). > +#define NR_SCANCODES 10 > + > +#define SCAN_INTERVAL (50) /* ms */ > +#define HINGE_SCAN_INTERVAL (250) /* ms */ Unneeded defines. (s/[NR_SCANCODES]/[]/) Better still how about: struct remote_control_key { unsigned char min; unsigned char max; unsigned char key; }; struct remote_control_key spitz_remote_keys[] = { { 25, 35, KEY_STOPCD}, { 55, 65, KEY_PLAYPAUSE}, { 85, 95, KEY_NEXTSONG}, { 115, 125, KEY_VOLUMEUP}, { 145, 155, KEY_PREVIOUSSONG}, { 180, 190, KEY_MUTE}, { 215, 225, KEY_VOLUMEDOWN}, }; #define RELEASE_HI 230 #define MAX_EARPHONE 6 static int get_remocon_raw(void) { int val, key = 0; val = sharpsl_pm_pxa_read_max1111(MAX1111_REMCOM); if (val >= RELEASE_HI) /* Key release */ else if (val <= MAX_EARPHONE) /* remote control unplugged */ else /* iterate through spitz_remote_keys, until find entry within range, extract keycode */ /* send key event */ } > +#define MAX1111_REMCOM 0u //FIXME > +#define MAX1111_BATT_VOLT 4u > +#define MAX1111_BATT_TEMP 2u > +#define MAX1111_ACIN_VOLT 6u > + > + > +/* MAX1111 Commands */ > +#define MAXCTRL_PD0 1u << 0 > +#define MAXCTRL_PD1 1u << 1 > +#define MAXCTRL_SGL 1u << 2 > +#define MAXCTRL_UNI 1u << 3 > +#define MAXCTRL_SEL_SH 4 > +#define MAXCTRL_STR 1u << 7 The PM code could share a common function to read a given MAX1111 channel, so it would then just be a question of calling that with the channel number. sharpsl_pm_pxa_read_max1111() already exists, its just a question of exporting it and moving the prototype from arch/arm/mach-pxa/sharpsl.h to include/asm-arm/hardware/sharpsl_pm.h > +#define SLKEY_RCREL 128 //95 for non-spitz > +#define HPJACK_STATE_NONE (0) > +#define HPJACK_STATE_REMOCON (2) > +#define HPJACK_STATE_HEADPHONE (1) > +#define RC_POLL_TIMER (HZ/100) > + > +#define SPITZ_SCP_AKIN_PULLUP SCOOP_GPCR_PA16 > + > +static int remocon_dev_stat = HPJACK_STATE_NONE; > +static int remocon_scan_state = 8; > +static int read_first = REMOTE_CONTROL_REL; > +static int last_key = SLKEY_RCREL; > +static int button_type = SLKEY_RCREL; > +static int remocon_noise_count = 0; Do we need all these variables? If so they should be merged into struct sharpsl_rc. > +static int sharpsl_rc_strobes[] = { > + SPITZ_GPIO_KEY_STROBE0, > + SPITZ_GPIO_KEY_STROBE1, > + SPITZ_GPIO_KEY_STROBE2, > + SPITZ_GPIO_KEY_STROBE3, > + SPITZ_GPIO_KEY_STROBE4, > + SPITZ_GPIO_KEY_STROBE5, > + SPITZ_GPIO_KEY_STROBE6, > + SPITZ_GPIO_KEY_STROBE7, > + SPITZ_GPIO_KEY_STROBE8, > + SPITZ_GPIO_KEY_STROBE9, > + SPITZ_GPIO_KEY_STROBE10, > +}; This code shouldn't be here, delete. > +static int sharpsl_pm_pxa_read_max1111(int channel) > +{ > + int ssp_corgi; > + ssp_corgi = corgi_ssp_max1111_get((channel << MAXCTRL_SEL_SH) | > MAXCTRL_PD0 | MAXCTRL_PD1 > + | MAXCTRL_SGL | MAXCTRL_UNI | MAXCTRL_STR); > + //MPRINTK("ssp (2.4): %d, ssp_corgi (2.6): %d\n", ssp, ssp_corgi); > + return ssp_corgi; > +} See above. > +static void sharpsl_rc_timer_callback(unsigned long data) > +{ [...] We can deal with this function in later version, it makes me head hurt atm ;-). Looks like it could be written much more neatly. > +} > + > + > + > +static struct sharpsl_rc *sharpsl_rc_data_ptr; Doesn't do anything. > +static int sharpsl_rc_suspend(struct platform_device *dev, pm_message_t > state) > +{ > + int i; > + struct sharpsl_rc *sharpsl_rc = platform_get_drvdata(dev); > + > + MPRINTK("sharpsl_rc_suspend state: %d\n", state); > + > + sharpsl_rc->suspended = 1; > + /* strobe 0 is the power key so this can't be made an input for > + powersaving therefore i = 1 */ > + for (i = 1; i < SPITZ_KEY_STROBE_NUM; i++) > + pxa_gpio_mode(sharpsl_rc_strobes[i] | GPIO_IN); Code copied from spitzkbd, not needed, needs removing (all of this function really). > + return 0; > +} > + > +static int sharpsl_rc_resume(struct platform_device *dev) > +{ > + int i; > + struct sharpsl_rc *sharpsl_rc = platform_get_drvdata(dev); > + > + MPRINTK("sharpsl_rc_resume\n"); > + > + for (i = 1; i < SPITZ_KEY_STROBE_NUM; i++) > + pxa_gpio_mode(sharpsl_rc_strobes[i] | GPIO_OUT | > GPIO_DFLT_HIGH); > + > + /* Upon resume, ignore the suspend key for a short while */ > + sharpsl_rc->suspend_jiffies=jiffies; > + sharpsl_rc->suspended = 0; ditto, useless. > +//static struct platform_device *sharpsl_rc_device; > + > +static int __init sharpsl_rc_probe(struct platform_device *pdev) > +{ > + struct sharpsl_rc *sharpsl_rc; > + struct input_dev *input_dev; > + int i, ret; > + > + MPRINTK("sharpsl_rc_probe\n"); > + > + sharpsl_rc = kzalloc(sizeof(struct sharpsl_rc), GFP_KERNEL); > + input_dev = input_allocate_device(); > + if (!sharpsl_rc || !input_dev) { > + kfree(sharpsl_rc); > + input_free_device(input_dev); > + return -ENOMEM; > + } Will need converting to the new input device allocation in recent kernels. > + platform_set_drvdata(pdev, sharpsl_rc); > + > + sharpsl_rc->input = input_dev; > + spin_lock_init(&sharpsl_rc->lock); > + > + /* Init Remote Control Timer */ > + init_timer(&sharpsl_rc->rctimer); > + sharpsl_rc->rctimer.function = sharpsl_rc_timer_callback; > + sharpsl_rc->rctimer.data = (unsigned long) sharpsl_rc; > + > + //memcpy(sharpsl_rc->keycode, sharpsl_rc_keycode, > sizeof(sharpsl_rc->keycode)); > + > + input_dev->name = "Spitz Remote Control"; > + input_dev->phys = "sharpsl_rc/input0"; > + input_dev->id.bustype = BUS_HOST; > + input_dev->id.vendor = 0x0001; > + input_dev->id.product = 0x0001; > + input_dev->id.version = 0x0100; > + input_dev->cdev.dev = &pdev->dev; > + input_dev->private = sharpsl_rc; > + > + input_dev->evbit[0] = BIT(EV_KEY);// | BIT(EV_REP); > + //input_dev->keycode = sharpsl_rc->keycode; // fixme - you need a new > table here > + //input_dev->keycodesize = sizeof(unsigned char); > + //input_dev->keycodemax = ARRAY_SIZE(sharpsl_rc_keycode); > + > + for (i = 2; i <= 8 /*ARRAY_SIZE(sharpsl_rc_keycode)*/; i++) > + set_bit(sharpsl_rc_keycode[i], input_dev->keybit); > + > + //clear_bit(0, input_dev->keybit); > + > + input_register_device(sharpsl_rc->input); > + > + pxa_gpio_mode(SPITZ_GPIO_AK_INT | GPIO_IN); > + ret = request_irq(SPITZ_IRQ_GPIO_AK_INT, sharpsl_rc_interrupt, > SA_INTERRUPT /* | SA_TRIGGER_RISING*/, "sharpsl_rc", sharpsl_rc); // Should > be rise or fall or both? > + if (ret < 0) { > + MPRINTK(/*KERN_ERR*/ "sharpsl_rc: Can't get IRQ: %d!\n", i); > + return ret; > + } > + > + set_irq_type(SPITZ_IRQ_GPIO_AK_INT, IRQT_BOTHEDGE); set_irq_type can be done through request_irq now. > +/* > + platform_set_drvdata(spitz_snd_device, &spitz_snd_devdata); > + spitz_snd_devdata.dev = &spitz_snd_device->dev; > + ret = platform_device_add(spitz_snd_device); > + if (ret) > + platform_device_put(spitz_snd_device); > +*/ > + sharpsl_rc_data_ptr = sharpsl_rc; dead code. > --- linux-2.6.17.orig/drivers/input/keyboard/spitzkbd.c > +++ linux-2.6.17/drivers/input/keyboard/spitzkbd.c > @@ -436,9 +436,9 @@ static int __init spitzkbd_probe(struct > request_irq(SPITZ_IRQ_GPIO_SWB, spitzkbd_hinge_isr, > SA_INTERRUPT | SA_TRIGGER_RISING | SA_TRIGGER_FALLING, > "Spitzkbd SWB", spitzkbd); > - request_irq(SPITZ_IRQ_GPIO_AK_INT, spitzkbd_hinge_isr, > - SA_INTERRUPT | SA_TRIGGER_RISING | SA_TRIGGER_FALLING, > - "Spitzkbd HP", spitzkbd); > + //request_irq(SPITZ_IRQ_GPIO_AK_INT, spitzkbd_hinge_isr, > + // SA_INTERRUPT | SA_TRIGGER_RISING | SA_TRIGGER_FALLING, > + // "Spitzkbd HP", spitzkbd); We need to teach both drivers to share this interrupt (SA_SHARED iirc). Also, is this patch intended to work on the c7x0 too? If so, you'll need to look at abstracting the machine specifics into spitz.c. Obviously, after these cleanups there are more to deal such as cleaning up the logic of that state code (timer function) but this should be enough to be getting on with ;-). Cheers, Richard

Next Message by Date: click to view message preview

Re: [Openzaurus-devel] Sharp CE-RH2 remote kernel driver

On 1/18/07, Richard Purdie <rpurdie-Fm38FmjxZ/leoWH0uzbU5w@xxxxxxxxxxxxxxxx> wrote: On Thu, 2007-01-18 at 12:31 -0800, Justin Patrin wrote: > I finally got another hint and have a working kernel driver for the > CE-RH2 in-line audio remote. It outputs the correct keys. > > There is a problem with the IRQ handling as well as I see this in dmesg: > enable_irq(45) unbalanced from bf1483e0 Keep in mind the effects of enable_irq and disable_irq are cumulative i.e. if you call disable_irq 4 times, you have to call enable_irq 4 times too. It may well be better to leave the interrupt handler enabled all the time, I'm undecided... Not taken care of yet, will look further when I work on the big state function. > Any help in making this patch better would be appreciated. I've inlined the code below and commented LKML style... > --- /dev/null > +++ linux-2.6.17/drivers/input/keyboard/sharpsl_rc.c > @@ -0,0 +1,529 @@ > +/* > +12:13 < RP> JustinP: There is going to be an issue with it fighting the sound system for the headphone interrupt > +12:14 < RP> JustinP: I'm about to rewrite the headphone interrupt handling anyway so the best soution might be to just comment it out in the sound code Its been rewritten and isn't going to change now. See below. > + > +#define DPRINTK(fmt, args...) //printk(KERN_ERR "sharpsl_rc, %s: " fmt,__FUNCTION__,## args) > + > +#define MPRINTK(fmt, args...) //printk(fmt,## args) > + > +#define MDPRINTK(fmt, args...) printk(fmt,## args) These should all be replaced with things like dev_dbg() and dev_warn(). > +#define NR_SCANCODES 10 > + > +#define SCAN_INTERVAL (50) /* ms */ > +#define HINGE_SCAN_INTERVAL (250) /* ms */ Unneeded defines. (s/[NR_SCANCODES]/[]/) Better still how about: struct remote_control_key { unsigned char min; unsigned char max; unsigned char key; }; struct remote_control_key spitz_remote_keys[] = { { 25, 35, KEY_STOPCD}, { 55, 65, KEY_PLAYPAUSE}, { 85, 95, KEY_NEXTSONG}, { 115, 125, KEY_VOLUMEUP}, { 145, 155, KEY_PREVIOUSSONG}, { 180, 190, KEY_MUTE}, { 215, 225, KEY_VOLUMEDOWN}, }; #define RELEASE_HI 230 #define MAX_EARPHONE 6 static int get_remocon_raw(void) { int val, key = 0; val = sharpsl_pm_pxa_read_max1111(MAX1111_REMCOM); if (val >= RELEASE_HI) /* Key release */ else if (val <= MAX_EARPHONE) /* remote control unplugged */ else /* iterate through spitz_remote_keys, until find entry within range, extract keycode */ /* send key event */ } Thanks much for the suggestion, done. > +#define MAX1111_REMCOM 0u //FIXME > +#define MAX1111_BATT_VOLT 4u > +#define MAX1111_BATT_TEMP 2u > +#define MAX1111_ACIN_VOLT 6u > + > + > +/* MAX1111 Commands */ > +#define MAXCTRL_PD0 1u << 0 > +#define MAXCTRL_PD1 1u << 1 > +#define MAXCTRL_SGL 1u << 2 > +#define MAXCTRL_UNI 1u << 3 > +#define MAXCTRL_SEL_SH 4 > +#define MAXCTRL_STR 1u << 7 The PM code could share a common function to read a given MAX1111 channel, so it would then just be a question of calling that with the channel number. sharpsl_pm_pxa_read_max1111() already exists, its just a question of exporting it and moving the prototype from arch/arm/mach-pxa/sharpsl.h to include/asm-arm/hardware/sharpsl_pm.h Moved. It's all in the patch (perhaps the max1111 move should be in its own patch but for now it's in sharpsl-rc.patch). > +#define SLKEY_RCREL 128 //95 for non-spitz > +#define HPJACK_STATE_NONE (0) > +#define HPJACK_STATE_REMOCON (2) > +#define HPJACK_STATE_HEADPHONE (1) > +#define RC_POLL_TIMER (HZ/100) > + > +#define SPITZ_SCP_AKIN_PULLUP SCOOP_GPCR_PA16 > + > +static int remocon_dev_stat = HPJACK_STATE_NONE; > +static int remocon_scan_state = 8; > +static int read_first = REMOTE_CONTROL_REL; > +static int last_key = SLKEY_RCREL; > +static int button_type = SLKEY_RCREL; > +static int remocon_noise_count = 0; Do we need all these variables? If so they should be merged into struct sharpsl_rc. I'll tackle those later with the state thing. > +static int sharpsl_rc_strobes[] = { > + SPITZ_GPIO_KEY_STROBE0, > + SPITZ_GPIO_KEY_STROBE1, > + SPITZ_GPIO_KEY_STROBE2, > + SPITZ_GPIO_KEY_STROBE3, > + SPITZ_GPIO_KEY_STROBE4, > + SPITZ_GPIO_KEY_STROBE5, > + SPITZ_GPIO_KEY_STROBE6, > + SPITZ_GPIO_KEY_STROBE7, > + SPITZ_GPIO_KEY_STROBE8, > + SPITZ_GPIO_KEY_STROBE9, > + SPITZ_GPIO_KEY_STROBE10, > +}; This code shouldn't be here, delete. Gone. > +static int sharpsl_pm_pxa_read_max1111(int channel) > +{ > + int ssp_corgi; > + ssp_corgi = corgi_ssp_max1111_get((channel << MAXCTRL_SEL_SH) | MAXCTRL_PD0 | MAXCTRL_PD1 > + | MAXCTRL_SGL | MAXCTRL_UNI | MAXCTRL_STR); > + //MPRINTK("ssp (2.4): %d, ssp_corgi (2.6): %d\n", ssp, ssp_corgi); > + return ssp_corgi; > +} See above. > +static void sharpsl_rc_timer_callback(unsigned long data) > +{ [...] We can deal with this function in later version, it makes me head hurt atm ;-). Looks like it could be written much more neatly. Hurts my head too. Don't lose any sleep over it, I'll try to get something less crazy implemented. > +} > + > + > + > +static struct sharpsl_rc *sharpsl_rc_data_ptr; Doesn't do anything. Gone. > +static int sharpsl_rc_suspend(struct platform_device *dev, pm_message_t state) > +{ > + int i; > + struct sharpsl_rc *sharpsl_rc = platform_get_drvdata(dev); > + > + MPRINTK("sharpsl_rc_suspend state: %d\n", state); > + > + sharpsl_rc->suspended = 1; > + /* strobe 0 is the power key so this can't be made an input for > + powersaving therefore i = 1 */ > + for (i = 1; i < SPITZ_KEY_STROBE_NUM; i++) > + pxa_gpio_mode(sharpsl_rc_strobes[i] | GPIO_IN); Code copied from spitzkbd, not needed, needs removing (all of this function really). Yep, gone. A lot of this stuff I may have re-copied in when I was trying to get it to work. > + return 0; > +} > + > +static int sharpsl_rc_resume(struct platform_device *dev) > +{ > + int i; > + struct sharpsl_rc *sharpsl_rc = platform_get_drvdata(dev); > + > + MPRINTK("sharpsl_rc_resume\n"); > + > + for (i = 1; i < SPITZ_KEY_STROBE_NUM; i++) > + pxa_gpio_mode(sharpsl_rc_strobes[i] | GPIO_OUT | GPIO_DFLT_HIGH); > + > + /* Upon resume, ignore the suspend key for a short while */ > + sharpsl_rc->suspend_jiffies=jiffies; > + sharpsl_rc->suspended = 0; ditto, useless. ditto. > +//static struct platform_device *sharpsl_rc_device; > + > +static int __init sharpsl_rc_probe(struct platform_device *pdev) > +{ > + struct sharpsl_rc *sharpsl_rc; > + struct input_dev *input_dev; > + int i, ret; > + > + MPRINTK("sharpsl_rc_probe\n"); > + > + sharpsl_rc = kzalloc(sizeof(struct sharpsl_rc), GFP_KERNEL); > + input_dev = input_allocate_device(); > + if (!sharpsl_rc || !input_dev) { > + kfree(sharpsl_rc); > + input_free_device(input_dev); > + return -ENOMEM; > + } Will need converting to the new input device allocation in recent kernels. Could you give me some pointers? The atkbd.c seems to do the same thing in my kernel tree (I'm using a 2.6.17 kernel as I'm doing this for OZ 3.5.4.2). > + platform_set_drvdata(pdev, sharpsl_rc); > + > + sharpsl_rc->input = input_dev; > + spin_lock_init(&sharpsl_rc->lock); > + > + /* Init Remote Control Timer */ > + init_timer(&sharpsl_rc->rctimer); > + sharpsl_rc->rctimer.function = sharpsl_rc_timer_callback; > + sharpsl_rc->rctimer.data = (unsigned long) sharpsl_rc; > + > + //memcpy(sharpsl_rc->keycode, sharpsl_rc_keycode, sizeof(sharpsl_rc->keycode)); > + > + input_dev->name = "Spitz Remote Control"; > + input_dev->phys = "sharpsl_rc/input0"; > + input_dev->id.bustype = BUS_HOST; > + input_dev->id.vendor = 0x0001; > + input_dev->id.product = 0x0001; > + input_dev->id.version = 0x0100; > + input_dev->cdev.dev = &pdev->dev; > + input_dev->private = sharpsl_rc; > + > + input_dev->evbit[0] = BIT(EV_KEY);// | BIT(EV_REP); > + //input_dev->keycode = sharpsl_rc->keycode; // fixme - you need a new table here > + //input_dev->keycodesize = sizeof(unsigned char); > + //input_dev->keycodemax = ARRAY_SIZE(sharpsl_rc_keycode); > + > + for (i = 2; i <= 8 /*ARRAY_SIZE(sharpsl_rc_keycode)*/; i++) > + set_bit(sharpsl_rc_keycode[i], input_dev->keybit); > + > + //clear_bit(0, input_dev->keybit); > + > + input_register_device(sharpsl_rc->input); > + > + pxa_gpio_mode(SPITZ_GPIO_AK_INT | GPIO_IN); > + ret = request_irq(SPITZ_IRQ_GPIO_AK_INT, sharpsl_rc_interrupt, SA_INTERRUPT /* | SA_TRIGGER_RISING*/, "sharpsl_rc", sharpsl_rc); // Should be rise or fall or both? > + if (ret < 0) { > + MPRINTK(/*KERN_ERR*/ "sharpsl_rc: Can't get IRQ: %d!\n", i); > + return ret; > + } > + > + set_irq_type(SPITZ_IRQ_GPIO_AK_INT, IRQT_BOTHEDGE); set_irq_type can be done through request_irq now. Ok, done I think. I think that I ended up doing this because the kernel version I was originally working with didn't have SA_TRIGGER_*. > +/* > + platform_set_drvdata(spitz_snd_device, &spitz_snd_devdata); > + spitz_snd_devdata.dev = &spitz_snd_device->dev; > + ret = platform_device_add(spitz_snd_device); > + if (ret) > + platform_device_put(spitz_snd_device); > +*/ > + sharpsl_rc_data_ptr = sharpsl_rc; dead code. Gone. > --- linux-2.6.17.orig/drivers/input/keyboard/spitzkbd.c > +++ linux-2.6.17/drivers/input/keyboard/spitzkbd.c > @@ -436,9 +436,9 @@ static int __init spitzkbd_probe(struct > request_irq(SPITZ_IRQ_GPIO_SWB, spitzkbd_hinge_isr, > SA_INTERRUPT | SA_TRIGGER_RISING | SA_TRIGGER_FALLING, > "Spitzkbd SWB", spitzkbd); > - request_irq(SPITZ_IRQ_GPIO_AK_INT, spitzkbd_hinge_isr, > - SA_INTERRUPT | SA_TRIGGER_RISING | SA_TRIGGER_FALLING, > - "Spitzkbd HP", spitzkbd); > + //request_irq(SPITZ_IRQ_GPIO_AK_INT, spitzkbd_hinge_isr, > + // SA_INTERRUPT | SA_TRIGGER_RISING | SA_TRIGGER_FALLING, > + // "Spitzkbd HP", spitzkbd); We need to teach both drivers to share this interrupt (SA_SHARED iirc). SA_SHIRQ. Done. Also, is this patch intended to work on the c7x0 too? If so, you'll need to look at abstracting the machine specifics into spitz.c. That would be good, yes, as they're essentially the same devices. They'll need different high/low numbers and, I assume, some different calls and devices, but it should be about the same. Obviously, after these cleanups there are more to deal such as cleaning up the logic of that state code (timer function) but this should be enough to be getting on with ;-). Yep. I'll move on to that. -- Justin Patrin

Previous Message by Thread: click to view message preview

Re: [Openzaurus-devel] Sharp CE-RH2 remote kernel driver

On Thu, 2007-01-18 at 12:31 -0800, Justin Patrin wrote: > I finally got another hint and have a working kernel driver for the > CE-RH2 in-line audio remote. It outputs the correct keys. Both xev and > showkey only show 3 of the keys, the other 4 show some strange output. Any idea why? I presume the kernel is doing the right thing and that's some kind of userspace issue? > However, gizmod (compiled on the Z due to cross-compile problems) sees > all 7 keys just fine and some quick hacking has it controlling mpd for > me (although volume control is still complicated). Still complicated? > There are still some issues with it, of course, it was pretty much > copied from the 2.4 kernel and will need some fix-ups to be merged > upstream but I'm willing to work on it if I can get some pointers. > > There is a problem with the IRQ handling as well as I see this in dmesg: > enable_irq(45) unbalanced from bf1483e0 Keep in mind the effects of enable_irq and disable_irq are cumulative i.e. if you call disable_irq 4 times, you have to call enable_irq 4 times too. It may well be better to leave the interrupt handler enabled all the time, I'm undecided... > Any help in making this patch better would be appreciated. I've inlined the code below and commented LKML style... > --- /dev/null > +++ linux-2.6.17/drivers/input/keyboard/sharpsl_rc.c > @@ -0,0 +1,529 @@ > +/* > +12:13 < RP> JustinP: There is going to be an issue with it fighting the > sound system for the headphone interrupt > +12:14 < RP> JustinP: I'm about to rewrite the headphone interrupt handling > anyway so the best soution might be to just comment it out in the sound code Its been rewritten and isn't going to change now. See below. > + > +#define DPRINTK(fmt, args...) //printk(KERN_ERR "sharpsl_rc, %s: " > fmt,__FUNCTION__,## args) > + > +#define MPRINTK(fmt, args...) //printk(fmt,## args) > + > +#define MDPRINTK(fmt, args...) printk(fmt,## args) These should all be replaced with things like dev_dbg() and dev_warn(). > +#define NR_SCANCODES 10 > + > +#define SCAN_INTERVAL (50) /* ms */ > +#define HINGE_SCAN_INTERVAL (250) /* ms */ Unneeded defines. (s/[NR_SCANCODES]/[]/) Better still how about: struct remote_control_key { unsigned char min; unsigned char max; unsigned char key; }; struct remote_control_key spitz_remote_keys[] = { { 25, 35, KEY_STOPCD}, { 55, 65, KEY_PLAYPAUSE}, { 85, 95, KEY_NEXTSONG}, { 115, 125, KEY_VOLUMEUP}, { 145, 155, KEY_PREVIOUSSONG}, { 180, 190, KEY_MUTE}, { 215, 225, KEY_VOLUMEDOWN}, }; #define RELEASE_HI 230 #define MAX_EARPHONE 6 static int get_remocon_raw(void) { int val, key = 0; val = sharpsl_pm_pxa_read_max1111(MAX1111_REMCOM); if (val >= RELEASE_HI) /* Key release */ else if (val <= MAX_EARPHONE) /* remote control unplugged */ else /* iterate through spitz_remote_keys, until find entry within range, extract keycode */ /* send key event */ } > +#define MAX1111_REMCOM 0u //FIXME > +#define MAX1111_BATT_VOLT 4u > +#define MAX1111_BATT_TEMP 2u > +#define MAX1111_ACIN_VOLT 6u > + > + > +/* MAX1111 Commands */ > +#define MAXCTRL_PD0 1u << 0 > +#define MAXCTRL_PD1 1u << 1 > +#define MAXCTRL_SGL 1u << 2 > +#define MAXCTRL_UNI 1u << 3 > +#define MAXCTRL_SEL_SH 4 > +#define MAXCTRL_STR 1u << 7 The PM code could share a common function to read a given MAX1111 channel, so it would then just be a question of calling that with the channel number. sharpsl_pm_pxa_read_max1111() already exists, its just a question of exporting it and moving the prototype from arch/arm/mach-pxa/sharpsl.h to include/asm-arm/hardware/sharpsl_pm.h > +#define SLKEY_RCREL 128 //95 for non-spitz > +#define HPJACK_STATE_NONE (0) > +#define HPJACK_STATE_REMOCON (2) > +#define HPJACK_STATE_HEADPHONE (1) > +#define RC_POLL_TIMER (HZ/100) > + > +#define SPITZ_SCP_AKIN_PULLUP SCOOP_GPCR_PA16 > + > +static int remocon_dev_stat = HPJACK_STATE_NONE; > +static int remocon_scan_state = 8; > +static int read_first = REMOTE_CONTROL_REL; > +static int last_key = SLKEY_RCREL; > +static int button_type = SLKEY_RCREL; > +static int remocon_noise_count = 0; Do we need all these variables? If so they should be merged into struct sharpsl_rc. > +static int sharpsl_rc_strobes[] = { > + SPITZ_GPIO_KEY_STROBE0, > + SPITZ_GPIO_KEY_STROBE1, > + SPITZ_GPIO_KEY_STROBE2, > + SPITZ_GPIO_KEY_STROBE3, > + SPITZ_GPIO_KEY_STROBE4, > + SPITZ_GPIO_KEY_STROBE5, > + SPITZ_GPIO_KEY_STROBE6, > + SPITZ_GPIO_KEY_STROBE7, > + SPITZ_GPIO_KEY_STROBE8, > + SPITZ_GPIO_KEY_STROBE9, > + SPITZ_GPIO_KEY_STROBE10, > +}; This code shouldn't be here, delete. > +static int sharpsl_pm_pxa_read_max1111(int channel) > +{ > + int ssp_corgi; > + ssp_corgi = corgi_ssp_max1111_get((channel << MAXCTRL_SEL_SH) | > MAXCTRL_PD0 | MAXCTRL_PD1 > + | MAXCTRL_SGL | MAXCTRL_UNI | MAXCTRL_STR); > + //MPRINTK("ssp (2.4): %d, ssp_corgi (2.6): %d\n", ssp, ssp_corgi); > + return ssp_corgi; > +} See above. > +static void sharpsl_rc_timer_callback(unsigned long data) > +{ [...] We can deal with this function in later version, it makes me head hurt atm ;-). Looks like it could be written much more neatly. > +} > + > + > + > +static struct sharpsl_rc *sharpsl_rc_data_ptr; Doesn't do anything. > +static int sharpsl_rc_suspend(struct platform_device *dev, pm_message_t > state) > +{ > + int i; > + struct sharpsl_rc *sharpsl_rc = platform_get_drvdata(dev); > + > + MPRINTK("sharpsl_rc_suspend state: %d\n", state); > + > + sharpsl_rc->suspended = 1; > + /* strobe 0 is the power key so this can't be made an input for > + powersaving therefore i = 1 */ > + for (i = 1; i < SPITZ_KEY_STROBE_NUM; i++) > + pxa_gpio_mode(sharpsl_rc_strobes[i] | GPIO_IN); Code copied from spitzkbd, not needed, needs removing (all of this function really). > + return 0; > +} > + > +static int sharpsl_rc_resume(struct platform_device *dev) > +{ > + int i; > + struct sharpsl_rc *sharpsl_rc = platform_get_drvdata(dev); > + > + MPRINTK("sharpsl_rc_resume\n"); > + > + for (i = 1; i < SPITZ_KEY_STROBE_NUM; i++) > + pxa_gpio_mode(sharpsl_rc_strobes[i] | GPIO_OUT | > GPIO_DFLT_HIGH); > + > + /* Upon resume, ignore the suspend key for a short while */ > + sharpsl_rc->suspend_jiffies=jiffies; > + sharpsl_rc->suspended = 0; ditto, useless. > +//static struct platform_device *sharpsl_rc_device; > + > +static int __init sharpsl_rc_probe(struct platform_device *pdev) > +{ > + struct sharpsl_rc *sharpsl_rc; > + struct input_dev *input_dev; > + int i, ret; > + > + MPRINTK("sharpsl_rc_probe\n"); > + > + sharpsl_rc = kzalloc(sizeof(struct sharpsl_rc), GFP_KERNEL); > + input_dev = input_allocate_device(); > + if (!sharpsl_rc || !input_dev) { > + kfree(sharpsl_rc); > + input_free_device(input_dev); > + return -ENOMEM; > + } Will need converting to the new input device allocation in recent kernels. > + platform_set_drvdata(pdev, sharpsl_rc); > + > + sharpsl_rc->input = input_dev; > + spin_lock_init(&sharpsl_rc->lock); > + > + /* Init Remote Control Timer */ > + init_timer(&sharpsl_rc->rctimer); > + sharpsl_rc->rctimer.function = sharpsl_rc_timer_callback; > + sharpsl_rc->rctimer.data = (unsigned long) sharpsl_rc; > + > + //memcpy(sharpsl_rc->keycode, sharpsl_rc_keycode, > sizeof(sharpsl_rc->keycode)); > + > + input_dev->name = "Spitz Remote Control"; > + input_dev->phys = "sharpsl_rc/input0"; > + input_dev->id.bustype = BUS_HOST; > + input_dev->id.vendor = 0x0001; > + input_dev->id.product = 0x0001; > + input_dev->id.version = 0x0100; > + input_dev->cdev.dev = &pdev->dev; > + input_dev->private = sharpsl_rc; > + > + input_dev->evbit[0] = BIT(EV_KEY);// | BIT(EV_REP); > + //input_dev->keycode = sharpsl_rc->keycode; // fixme - you need a new > table here > + //input_dev->keycodesize = sizeof(unsigned char); > + //input_dev->keycodemax = ARRAY_SIZE(sharpsl_rc_keycode); > + > + for (i = 2; i <= 8 /*ARRAY_SIZE(sharpsl_rc_keycode)*/; i++) > + set_bit(sharpsl_rc_keycode[i], input_dev->keybit); > + > + //clear_bit(0, input_dev->keybit); > + > + input_register_device(sharpsl_rc->input); > + > + pxa_gpio_mode(SPITZ_GPIO_AK_INT | GPIO_IN); > + ret = request_irq(SPITZ_IRQ_GPIO_AK_INT, sharpsl_rc_interrupt, > SA_INTERRUPT /* | SA_TRIGGER_RISING*/, "sharpsl_rc", sharpsl_rc); // Should > be rise or fall or both? > + if (ret < 0) { > + MPRINTK(/*KERN_ERR*/ "sharpsl_rc: Can't get IRQ: %d!\n", i); > + return ret; > + } > + > + set_irq_type(SPITZ_IRQ_GPIO_AK_INT, IRQT_BOTHEDGE); set_irq_type can be done through request_irq now. > +/* > + platform_set_drvdata(spitz_snd_device, &spitz_snd_devdata); > + spitz_snd_devdata.dev = &spitz_snd_device->dev; > + ret = platform_device_add(spitz_snd_device); > + if (ret) > + platform_device_put(spitz_snd_device); > +*/ > + sharpsl_rc_data_ptr = sharpsl_rc; dead code. > --- linux-2.6.17.orig/drivers/input/keyboard/spitzkbd.c > +++ linux-2.6.17/drivers/input/keyboard/spitzkbd.c > @@ -436,9 +436,9 @@ static int __init spitzkbd_probe(struct > request_irq(SPITZ_IRQ_GPIO_SWB, spitzkbd_hinge_isr, > SA_INTERRUPT | SA_TRIGGER_RISING | SA_TRIGGER_FALLING, > "Spitzkbd SWB", spitzkbd); > - request_irq(SPITZ_IRQ_GPIO_AK_INT, spitzkbd_hinge_isr, > - SA_INTERRUPT | SA_TRIGGER_RISING | SA_TRIGGER_FALLING, > - "Spitzkbd HP", spitzkbd); > + //request_irq(SPITZ_IRQ_GPIO_AK_INT, spitzkbd_hinge_isr, > + // SA_INTERRUPT | SA_TRIGGER_RISING | SA_TRIGGER_FALLING, > + // "Spitzkbd HP", spitzkbd); We need to teach both drivers to share this interrupt (SA_SHARED iirc). Also, is this patch intended to work on the c7x0 too? If so, you'll need to look at abstracting the machine specifics into spitz.c. Obviously, after these cleanups there are more to deal such as cleaning up the logic of that state code (timer function) but this should be enough to be getting on with ;-). Cheers, Richard

Next Message by Thread: click to view message preview

Re: [Openzaurus-devel] Sharp CE-RH2 remote kernel driver

On 1/18/07, Richard Purdie <rpurdie-Fm38FmjxZ/leoWH0uzbU5w@xxxxxxxxxxxxxxxx> wrote: On Thu, 2007-01-18 at 12:31 -0800, Justin Patrin wrote: > I finally got another hint and have a working kernel driver for the > CE-RH2 in-line audio remote. It outputs the correct keys. > > There is a problem with the IRQ handling as well as I see this in dmesg: > enable_irq(45) unbalanced from bf1483e0 Keep in mind the effects of enable_irq and disable_irq are cumulative i.e. if you call disable_irq 4 times, you have to call enable_irq 4 times too. It may well be better to leave the interrupt handler enabled all the time, I'm undecided... Not taken care of yet, will look further when I work on the big state function. > Any help in making this patch better would be appreciated. I've inlined the code below and commented LKML style... > --- /dev/null > +++ linux-2.6.17/drivers/input/keyboard/sharpsl_rc.c > @@ -0,0 +1,529 @@ > +/* > +12:13 < RP> JustinP: There is going to be an issue with it fighting the sound system for the headphone interrupt > +12:14 < RP> JustinP: I'm about to rewrite the headphone interrupt handling anyway so the best soution might be to just comment it out in the sound code Its been rewritten and isn't going to change now. See below. > + > +#define DPRINTK(fmt, args...) //printk(KERN_ERR "sharpsl_rc, %s: " fmt,__FUNCTION__,## args) > + > +#define MPRINTK(fmt, args...) //printk(fmt,## args) > + > +#define MDPRINTK(fmt, args...) printk(fmt,## args) These should all be replaced with things like dev_dbg() and dev_warn(). > +#define NR_SCANCODES 10 > + > +#define SCAN_INTERVAL (50) /* ms */ > +#define HINGE_SCAN_INTERVAL (250) /* ms */ Unneeded defines. (s/[NR_SCANCODES]/[]/) Better still how about: struct remote_control_key { unsigned char min; unsigned char max; unsigned char key; }; struct remote_control_key spitz_remote_keys[] = { { 25, 35, KEY_STOPCD}, { 55, 65, KEY_PLAYPAUSE}, { 85, 95, KEY_NEXTSONG}, { 115, 125, KEY_VOLUMEUP}, { 145, 155, KEY_PREVIOUSSONG}, { 180, 190, KEY_MUTE}, { 215, 225, KEY_VOLUMEDOWN}, }; #define RELEASE_HI 230 #define MAX_EARPHONE 6 static int get_remocon_raw(void) { int val, key = 0; val = sharpsl_pm_pxa_read_max1111(MAX1111_REMCOM); if (val >= RELEASE_HI) /* Key release */ else if (val <= MAX_EARPHONE) /* remote control unplugged */ else /* iterate through spitz_remote_keys, until find entry within range, extract keycode */ /* send key event */ } Thanks much for the suggestion, done. > +#define MAX1111_REMCOM 0u //FIXME > +#define MAX1111_BATT_VOLT 4u > +#define MAX1111_BATT_TEMP 2u > +#define MAX1111_ACIN_VOLT 6u > + > + > +/* MAX1111 Commands */ > +#define MAXCTRL_PD0 1u << 0 > +#define MAXCTRL_PD1 1u << 1 > +#define MAXCTRL_SGL 1u << 2 > +#define MAXCTRL_UNI 1u << 3 > +#define MAXCTRL_SEL_SH 4 > +#define MAXCTRL_STR 1u << 7 The PM code could share a common function to read a given MAX1111 channel, so it would then just be a question of calling that with the channel number. sharpsl_pm_pxa_read_max1111() already exists, its just a question of exporting it and moving the prototype from arch/arm/mach-pxa/sharpsl.h to include/asm-arm/hardware/sharpsl_pm.h Moved. It's all in the patch (perhaps the max1111 move should be in its own patch but for now it's in sharpsl-rc.patch). > +#define SLKEY_RCREL 128 //95 for non-spitz > +#define HPJACK_STATE_NONE (0) > +#define HPJACK_STATE_REMOCON (2) > +#define HPJACK_STATE_HEADPHONE (1) > +#define RC_POLL_TIMER (HZ/100) > + > +#define SPITZ_SCP_AKIN_PULLUP SCOOP_GPCR_PA16 > + > +static int remocon_dev_stat = HPJACK_STATE_NONE; > +static int remocon_scan_state = 8; > +static int read_first = REMOTE_CONTROL_REL; > +static int last_key = SLKEY_RCREL; > +static int button_type = SLKEY_RCREL; > +static int remocon_noise_count = 0; Do we need all these variables? If so they should be merged into struct sharpsl_rc. I'll tackle those later with the state thing. > +static int sharpsl_rc_strobes[] = { > + SPITZ_GPIO_KEY_STROBE0, > + SPITZ_GPIO_KEY_STROBE1, > + SPITZ_GPIO_KEY_STROBE2, > + SPITZ_GPIO_KEY_STROBE3, > + SPITZ_GPIO_KEY_STROBE4, > + SPITZ_GPIO_KEY_STROBE5, > + SPITZ_GPIO_KEY_STROBE6, > + SPITZ_GPIO_KEY_STROBE7, > + SPITZ_GPIO_KEY_STROBE8, > + SPITZ_GPIO_KEY_STROBE9, > + SPITZ_GPIO_KEY_STROBE10, > +}; This code shouldn't be here, delete. Gone. > +static int sharpsl_pm_pxa_read_max1111(int channel) > +{ > + int ssp_corgi; > + ssp_corgi = corgi_ssp_max1111_get((channel << MAXCTRL_SEL_SH) | MAXCTRL_PD0 | MAXCTRL_PD1 > + | MAXCTRL_SGL | MAXCTRL_UNI | MAXCTRL_STR); > + //MPRINTK("ssp (2.4): %d, ssp_corgi (2.6): %d\n", ssp, ssp_corgi); > + return ssp_corgi; > +} See above. > +static void sharpsl_rc_timer_callback(unsigned long data) > +{ [...] We can deal with this function in later version, it makes me head hurt atm ;-). Looks like it could be written much more neatly. Hurts my head too. Don't lose any sleep over it, I'll try to get something less crazy implemented. > +} > + > + > + > +static struct sharpsl_rc *sharpsl_rc_data_ptr; Doesn't do anything. Gone. > +static int sharpsl_rc_suspend(struct platform_device *dev, pm_message_t state) > +{ > + int i; > + struct sharpsl_rc *sharpsl_rc = platform_get_drvdata(dev); > + > + MPRINTK("sharpsl_rc_suspend state: %d\n", state); > + > + sharpsl_rc->suspended = 1; > + /* strobe 0 is the power key so this can't be made an input for > + powersaving therefore i = 1 */ > + for (i = 1; i < SPITZ_KEY_STROBE_NUM; i++) > + pxa_gpio_mode(sharpsl_rc_strobes[i] | GPIO_IN); Code copied from spitzkbd, not needed, needs removing (all of this function really). Yep, gone. A lot of this stuff I may have re-copied in when I was trying to get it to work. > + return 0; > +} > + > +static int sharpsl_rc_resume(struct platform_device *dev) > +{ > + int i; > + struct sharpsl_rc *sharpsl_rc = platform_get_drvdata(dev); > + > + MPRINTK("sharpsl_rc_resume\n"); > + > + for (i = 1; i < SPITZ_KEY_STROBE_NUM; i++) > + pxa_gpio_mode(sharpsl_rc_strobes[i] | GPIO_OUT | GPIO_DFLT_HIGH); > + > + /* Upon resume, ignore the suspend key for a short while */ > + sharpsl_rc->suspend_jiffies=jiffies; > + sharpsl_rc->suspended = 0; ditto, useless. ditto. > +//static struct platform_device *sharpsl_rc_device; > + > +static int __init sharpsl_rc_probe(struct platform_device *pdev) > +{ > + struct sharpsl_rc *sharpsl_rc; > + struct input_dev *input_dev; > + int i, ret; > + > + MPRINTK("sharpsl_rc_probe\n"); > + > + sharpsl_rc = kzalloc(sizeof(struct sharpsl_rc), GFP_KERNEL); > + input_dev = input_allocate_device(); > + if (!sharpsl_rc || !input_dev) { > + kfree(sharpsl_rc); > + input_free_device(input_dev); > + return -ENOMEM; > + } Will need converting to the new input device allocation in recent kernels. Could you give me some pointers? The atkbd.c seems to do the same thing in my kernel tree (I'm using a 2.6.17 kernel as I'm doing this for OZ 3.5.4.2). > + platform_set_drvdata(pdev, sharpsl_rc); > + > + sharpsl_rc->input = input_dev; > + spin_lock_init(&sharpsl_rc->lock); > + > + /* Init Remote Control Timer */ > + init_timer(&sharpsl_rc->rctimer); > + sharpsl_rc->rctimer.function = sharpsl_rc_timer_callback; > + sharpsl_rc->rctimer.data = (unsigned long) sharpsl_rc; > + > + //memcpy(sharpsl_rc->keycode, sharpsl_rc_keycode, sizeof(sharpsl_rc->keycode)); > + > + input_dev->name = "Spitz Remote Control"; > + input_dev->phys = "sharpsl_rc/input0"; > + input_dev->id.bustype = BUS_HOST; > + input_dev->id.vendor = 0x0001; > + input_dev->id.product = 0x0001; > + input_dev->id.version = 0x0100; > + input_dev->cdev.dev = &pdev->dev; > + input_dev->private = sharpsl_rc; > + > + input_dev->evbit[0] = BIT(EV_KEY);// | BIT(EV_REP); > + //input_dev->keycode = sharpsl_rc->keycode; // fixme - you need a new table here > + //input_dev->keycodesize = sizeof(unsigned char); > + //input_dev->keycodemax = ARRAY_SIZE(sharpsl_rc_keycode); > + > + for (i = 2; i <= 8 /*ARRAY_SIZE(sharpsl_rc_keycode)*/; i++) > + set_bit(sharpsl_rc_keycode[i], input_dev->keybit); > + > + //clear_bit(0, input_dev->keybit); > + > + input_register_device(sharpsl_rc->input); > + > + pxa_gpio_mode(SPITZ_GPIO_AK_INT | GPIO_IN); > + ret = request_irq(SPITZ_IRQ_GPIO_AK_INT, sharpsl_rc_interrupt, SA_INTERRUPT /* | SA_TRIGGER_RISING*/, "sharpsl_rc", sharpsl_rc); // Should be rise or fall or both? > + if (ret < 0) { > + MPRINTK(/*KERN_ERR*/ "sharpsl_rc: Can't get IRQ: %d!\n", i); > + return ret; > + } > + > + set_irq_type(SPITZ_IRQ_GPIO_AK_INT, IRQT_BOTHEDGE); set_irq_type can be done through request_irq now. Ok, done I think. I think that I ended up doing this because the kernel version I was originally working with didn't have SA_TRIGGER_*. > +/* > + platform_set_drvdata(spitz_snd_device, &spitz_snd_devdata); > + spitz_snd_devdata.dev = &spitz_snd_device->dev; > + ret = platform_device_add(spitz_snd_device); > + if (ret) > + platform_device_put(spitz_snd_device); > +*/ > + sharpsl_rc_data_ptr = sharpsl_rc; dead code. Gone. > --- linux-2.6.17.orig/drivers/input/keyboard/spitzkbd.c > +++ linux-2.6.17/drivers/input/keyboard/spitzkbd.c > @@ -436,9 +436,9 @@ static int __init spitzkbd_probe(struct > request_irq(SPITZ_IRQ_GPIO_SWB, spitzkbd_hinge_isr, > SA_INTERRUPT | SA_TRIGGER_RISING | SA_TRIGGER_FALLING, > "Spitzkbd SWB", spitzkbd); > - request_irq(SPITZ_IRQ_GPIO_AK_INT, spitzkbd_hinge_isr, > - SA_INTERRUPT | SA_TRIGGER_RISING | SA_TRIGGER_FALLING, > - "Spitzkbd HP", spitzkbd); > + //request_irq(SPITZ_IRQ_GPIO_AK_INT, spitzkbd_hinge_isr, > + // SA_INTERRUPT | SA_TRIGGER_RISING | SA_TRIGGER_FALLING, > + // "Spitzkbd HP", spitzkbd); We need to teach both drivers to share this interrupt (SA_SHARED iirc). SA_SHIRQ. Done. Also, is this patch intended to work on the c7x0 too? If so, you'll need to look at abstracting the machine specifics into spitz.c. That would be good, yes, as they're essentially the same devices. They'll need different high/low numbers and, I assume, some different calls and devices, but it should be about the same. Obviously, after these cleanups there are more to deal such as cleaning up the logic of that state code (timer function) but this should be enough to be getting on with ;-). Yep. I'll move on to that. -- Justin Patrin