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

http://oe.reversefold.com/sharpsl-rc/sharpsl-rc-2.6.18-r0.patch

A patch for kernel 2.6.18. This works just fine but I had to comment
out the spitzkbd.c use of AK_INT as it didn't want ot share the IRQ
with sharpsl-rc.

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


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