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