Re: Sharp CE-RH2 remote kernel driver

On 1/21/07, Justin Patrin <[email protected]> wrote:
> On 1/21/07, Justin Patrin <[email protected]> wrote:
> > On 1/21/07, Richard Purdie <[email protected]> wrote:
> > > 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
> >
> > New version:
> > http://oe.reversefold.com/sharpsl-rc/sharpsl-rc-r2.patch
>
> http://oe.reversefold.com/sharpsl-rc/sharpsl-rc-r3.patch
>

One more revision today:
http://oe.reversefold.com/sharpsl-rc/sharpsl-rc-r4.patch

This last one adds some code to the wakeup callback in spitz_pm.c that
allows remote command to wake up the Z. I realize that the code is not
entirely correct but it does seem to work. I remember that this was
supported in the 2.4 kernels and figured I'd try to make it work here
too. One problem with this implementation is that anything being
inserted into or removed from the headphone port also wakes up the Z.

I would think that something similar could also wake up the Z when the
hinge button is pressed....

> >
> > > >
> > > > 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.
> > >
> >
> > Ah, ok, done.
> >
> > > > 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?...
> > >
> >
> > Thanks for your and XorA's help on IRC this is now taken care of. The
> > AK_INT value only lets us know if headphones are inserted but not a
> > remote. The HP_IN value lets us know if anything is inserted. This
> > patch includes a switch from AK_INT to HP_IN in spitzkbd.c.
> >
> > Now we also know that we can detect heaphone or remote based on these 2 values.
> > AK=1&HP=1 == heaphones
> > AK=0&HP=1 == remote
>
> I've readded the AK_INT switch to spitzkbd.c to allow for userspace
> programs to see the remote status as well. The switch is
> SW_REMOTE_INSERT and is 1 if the remote is inserted, 0 if it is not
> (not the same as AK_INT which is 1 if heapdhones are inserted, 0
> otherwise).
>
> I've also added a workqueue which calls a simple funciton to load the
> remote module if a remote is inserted. Thanks to RP for letting me
> know there was such a simple way to do this. I didn't see a way to
> remove a module in kmod.h so the module will stay inserted
> indefinately. This should not be a problem, however, as it will not
> report any input events unless a remote is inserted (and buttons are
> pressed).
>
> >
> > I've now removed all of the crazy insert/remove logic from the driver
> > and it's much simpler. :-)
> >
> > >
> > > > 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/
> >
> > Done.
> >
> > >
> > > > +#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
> >
> > Done.
> >
> > >
> > > > +     { 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.
> >
> > Done and simplified, we now get a key or 0.
> >
> > >
> > > > +                     }
> > > > +             }
> > > > +     }
> > > > +     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.
> >
> > Yep, was debugging. I'd forgotten /proc/interrupts. Removed.
> >
> > >
> > > > +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.
> >
> > Done.
> >
> > >
> > > > +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) ?
> >
> > I thought about it but hadn't done it. Good call, though, done.
> >
> > >
> > > >
> > > > +             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.
> >
> > I'm not sure which exactly is needed...I'll try doing it with only one
> > or the other and see if it makes a difference.
>
> The module didn't work with only SA_TRIGGER_FALLING ad works fine
> without it so this version of the patch does not include it.
>
> >
> > >
> > > > +     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.
> >
> > Done.
> >
> > >
> > >
> > > > 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...
> >
> > Well, it builds and works fine with this change but I'll just go ahead
> > and change it back as my alteration here isn't needed. Thanks.
> >
> > >
> > > > 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?
> >
> > You're right, I don't, thanks for the catch.
> >
> > >
> > > Its getting better each time :) I'm still giving the state engine some
> > > thought...
> > >
> >
> > It should be much more clear now.
> >
>
> This driver is now 100% working for the CE-RH2 on the spitz. Thanks
> much to RP and XorA for their help.
>
> The Sharp 2.4 driver had some different calls for the Akita and
> different values/logic for the corgi (cxx0 which use the CE-RH1
> remote). Any ideas on what may have to be changed in the 2.6 kernel to
> support these other devices would be appreciated.
>
> --
> Justin Patrin
>


-- 
Justin Patrin

-------------------------------------------------------------------------
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