Re: Sharp CE-RH2 remote kernel driver

On Sun, 2007-01-21 at 00:55 -0800, Justin Patrin wrote:
> Ok, I've rewritten the majority of the code now. It's about the same
> length but it makes more sense to me now...
> 
> http://oe.reversefold.com/sharpsl-rc/sharpsl-rc-r1.patch
> 
> Some notes:
> 
> 1) I've kept the DPRINTK define as I can't figure out what dev I
> should pass into dev_dbg (no other input devices seem to use it).

You want to pass it a struct device * which you can get in the probe
function with &pdev->dev;. You probably want a struct device *dev in
struct sharpsl_rc for use by this.

> 3) Remote insert/removal is sent via a switch event, same as the
> keyboard driver does. Unfortunately, SPITZ_GPIO_AK_INT doesn't show
> anything when the remote is inserted so we have a duplicate of this
> info. SPITZ_GPIO_AK_INT works fine for normal headphones, the logic in
> this driver deals with the remote.
> 
> 3a) Knowing whether the headphone should be turned on is essentially a
> simple OR of the 2 switches. Setting up another instance of switchevd
> for the remote causes it to set the output correctly. The correct
> solution would be, I think, to listen to *both* switches, but check
> the values of both whenever an event is received. Or perhaps we need
> to combine the drivers somehow...

I'm not happy having two different switch events in the long run so we
need to combine the data somehow. I'm tempted to remove the logic from
spitzkbd and add it to the remote control but that means having the
remote driver loaded for everyone, even when they don't have the remote
control. It also complicates userspace with multiple input devices.

Tricky, and I don't know if there is a right answer, unless perhaps we
make the remote driver plug into the keyboard driver?...


> Index: linux-2.6.17/drivers/input/keyboard/sharpsl_rc.c
> ===================================================================
> --- /dev/null
> +++ linux-2.6.17/drivers/input/keyboard/sharpsl_rc.c
> +#include <asm/arch/spitz.h>
> +#include <asm/arch/hardware.h>
> +#include <asm/arch/pxa-regs.h>
> +#include <asm/hardware/scoop.h>
> +#include <asm/arch-pxa/sharpsl.h>

s/arch-pxa/arch/

> +#include <asm/hardware/sharpsl_pm.h>
> +
> +#define REMOTE_CONTROL_REL      1
> +#define REMOTE_CONTROL_PHONE    9
> +
> +#define DPRINTK(fmt, args...) printk("spitzsl_rc.c: " fmt "\n", ##args)
> +
> +struct remote_control_key {
> +	unsigned char min;
> +	unsigned char max;
> +	unsigned char key;
> +};
> +
> +struct remote_control_key spitz_remote_keys[] = {

static

> +	{ 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},
> +};
> +
> +static int get_remocon_raw(void)
> +{
> +	int i, val, key = 0;
> +
> +	val = sharpsl_pm_pxa_read_max1111(MAX1111_REMCOM);
> +	if (val >= RELEASE_HI) {
> +		/* Key release */
> +		key = REMOTE_CONTROL_REL;

return REMOTE_CONTROL_REL; ?

> +	} else if (val <= MAX_EARPHONE) {
> +		/* remote control unplugged */
> +		key = REMOTE_CONTROL_PHONE;

return REMOTE_CONTROL_PHONE; ?

> +	} else {
> +		for (i = 0; i < ARRAY_SIZE(spitz_remote_keys); ++i) {
> +			if (val >= spitz_remote_keys[i].min
> +				&& val <= spitz_remote_keys[i].max) {
> +				key = spitz_remote_keys[i].key;

a break here would be fractionally more efficient. In fact, just return
here too.

> +			}
> +		}
> +	}
> +	DPRINTK("VAL=%i, KEY=%i", val, key);
> +	return key;
> +}
> +
> +static int sharpsl_rc_numHandled = 0;

I'm presuming this is debugging and can be removed? Catting varios files
in proc will give you interrupt counts btw.

> +static irqreturn_t sharpsl_rc_interrupt(int irq, void *dev_id, struct pt_regs *regs)
> +{
> +	struct sharpsl_rc *data = dev_id;
> +
> +	DPRINTK("sharpsl_rc_interrupt %d", irq);
> +	if (!data->handling_press) {
> +		//disable finish callback (if we're within RC_FINISH_MS we don't want to run the finish callback)
> +		del_timer_sync(&data->rctimer_finish);
> +		++sharpsl_rc_numHandled;
> +		DPRINTK("handling interrupt %d", sharpsl_rc_numHandled);
> +		data->handling_press = 1;
> +		data->inserted = 0;
> +		data->noise = 0;
> +		data->state = 0;
> +		data->last_key = 0;
> +
> +		reset_scoop_gpio(&spitzscoop2_device.dev, SPITZ_SCP2_AKIN_PULLUP);
> +
> +		mod_timer(&data->rctimer, jiffies + msecs_to_jiffies(RC_POLL_MS));
> +		return IRQ_HANDLED;
> +	}
> +	return 0;

This should always return IRQ_HANDLED; 0 isn't a valid value for
irqreturn_t. If you don't handle it and spitzkbd wasn't loaded, you
would get spurious irq warnings.

> +static void sharpsl_rc_timer_callback(unsigned long dataPtr)
> +{
[...]
> +
> +		//send button press
> +		default:
> +			DPRINTK("key press detected %d, noise %d", data->last_key, data->noise);
> +			//if remote isn't set as inserted, force it
> +			if (!data->inserted) {
> +				DPRINTK("key press but not inserted");
> +				data->inserted = 1;
> +				DPRINTK("REMOTE_CONTROL_PHONE: release SW_HEADPHONE_INSERT 1");
> +				input_report_switch(data->input, SW_HEADPHONE_INSERT, 1);
> +			}
> +			input_report_key(data->input, data->last_key, 1);
> +			break;
> +		}
> +		++data->state;
> +
> +	//wait until key is released
> +	} else if (data->state < WAIT_STATE + 3) {

(WAIT_STATE * 2) ?

> 
> +		switch (data->last_key) {
> +		//special values won't be "held"
> +		case 0:
> +		case REMOTE_CONTROL_REL:
> +		case REMOTE_CONTROL_PHONE:
> +			++data->state;
> +			break;
> +		default:
> +			if (key == data->last_key
> +				&& data->noise < NOISE_THRESHOLD) {
> +				data->state = WAIT_STATE + 1;
> +				++data->noise;
> +			} else {
> +				++data->state;
> +			}
> +		}
> +
> +	//key is released, send event
> +	} else {
> +		switch (data->last_key) {
[...]
> +
> +static int __init sharpsl_rc_probe(struct platform_device *pdev)
> +{
> +	struct sharpsl_rc *sharpsl_rc;
> +	struct input_dev *input_dev;
[...]
> +	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 | SA_TRIGGER_FALLING | SA_SHIRQ,
> +					  "sharpsl_rc",
> +					  sharpsl_rc); // Should be rise or fall or both?

You should check out which is needed here as it will save some overhead
on the irq handler.

> +	if (ret < 0) {
> +		DPRINTK("Can't get IRQ: %d!", i);
> +		return ret;
> +	}

You need to unregister the input device and free memory if you're going
to return an error.


> Index: linux-2.6.17/arch/arm/mach-pxa/sharpsl_pm.c
> ===================================================================
> --- linux-2.6.17.orig/arch/arm/mach-pxa/sharpsl_pm.c
> +++ linux-2.6.17/arch/arm/mach-pxa/sharpsl_pm.c
> @@ -27,7 +27,7 @@
>  #include <asm/arch/pm.h>
>  #include <asm/arch/pxa-regs.h>
>  #include <asm/arch/sharpsl.h>
> -#include "sharpsl.h"
> +#include <asm/hardware/sharpsl_pm.h>

You still need to include "sharpsl.h" too...

> Index: linux-2.6.17/include/asm-arm/hardware/sharpsl_pm.h
> ===================================================================
> --- linux-2.6.17.orig/include/asm-arm/hardware/sharpsl_pm.h
> +++ linux-2.6.17/include/asm-arm/hardware/sharpsl_pm.h
> @@ -103,3 +103,18 @@ irqreturn_t sharpsl_ac_isr(int irq, void
>  irqreturn_t sharpsl_chrg_full_isr(int irq, void *dev_id, struct pt_regs *fp);
>  irqreturn_t sharpsl_fatal_isr(int irq, void *dev_id, struct pt_regs *fp);
>  
> +/* 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

Why do you need to move these definitions?

Its getting better each time :) I'm still giving the state engine some
thought...

Cheers,

Richard


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV