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

On 1/18/07, Richard Purdie <rpurdie-Fm38FmjxZ/[email protected]> 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