Re: [PATCH, v2] input/imx2x: generic keypad handler for i.MX SoCs

[Martin Fuzzey <mfuzzey@gmail.com>]

Hi Holger,

I've got your keypad driver running on my i.MX21 board - I had a few
problems mainly when trying to use it as a module -comments below.

> --- linux.orig/arch/arm/mach-mx2/devices.c
> +++ linux/arch/arm/mach-mx2/devices.c
...
> +
> +#ifdef CONFIG_KEYBOARD_MXC

For module needs to be
#if defined(CONFIG_KEYBOARD_MXC) || defined(CONFIG_KEYBOARD_MXC_MODULE)

But  I notice the other resource definitions in this file SDHC etc
aren't conditionally compiled at all - what is the policy on this?

> +static struct resource mxc_resource_keypad[] = {
> +       [0] = {
> +               .start  = 0x10008000,
> +               .end    = 0x10008014,

Why not use KPP_BASE_ADDR?


> +++ linux/drivers/input/keyboard/mxc_keypad.c
> +static void mxc_keypad_scan(unsigned long data)
> +{
> +       struct mxc_keypad *kp = (struct mxc_keypad *)data;

I changed this to directly take a struct mxc_keypad *  rather than a
long with a cast


> +       /* Quick-discharge by setting to totem-pole, */
> +       /* then turn back to open-drain */
> +       __raw_writew(kp->pdata->output_pins, kp->base + KPCR);
> +       wmb();
> +       udelay(2);
> +       __raw_writew(kp->pdata->output_pins |
> +               kp->pdata->input_pins, kp->base + KPCR);
> +

I think the first write should be input_pins (need to set the output
pins bits to zero in KPCR to switch to totem pole mode.
Where does the udelay(2) come from?

Also (more for my information - I'm a bit sketchy on this) why
__raw_writel + wmb() rather than plain writel ?


> +               if (col_bit & kp->pdata->output_pins) {
> +                       __raw_writew(~col_bit, kp->base + KPDR);
> +                       wmb();
> +                       udelay(2);
> +                       reg = ~__raw_readw(kp->base + KPDR);
> +                       keystate_cur[snum] = reg & kp->pdata->input_pins;
> +
> +                       if (snum++ >= MAX_INPUTS)

I was getting bounces with this - increasing the udelay(2) to 10 fixed
it but I preferred to do :

		if (col_bit & kp->pdata->output_pins) {
			u16 last_reg = 0;
			int debounce = 0;

			__raw_writew(~col_bit, kp->base + KPDR);
			wmb();

			while (debounce < 3) {
				udelay(1);
				reg = ~__raw_readw(kp->base + KPDR);
				if (reg == last_reg)
					debounce++;
				else
					debounce = 0;
				last_reg = reg;
			}
			keystate_cur[snum] = reg & kp->pdata->input_pins;

			if (snum++ >= MAX_INPUTS)


On my keyboard this exits after 4-6 loops.

> +static irqreturn_t mxc_keypad_irq_handler(int irq, void *data)
> +{
> +       struct mxc_keypad *kp = data;
> +       u16 stat;
> +
...
> +
> +       mxc_keypad_scan((unsigned long)kp);
> +
mxc_keypad_scan(kp);  as mentionned above?


> +static int __devinit mxc_keypad_probe(struct platform_device *pdev)
> +{
...
> +       error = request_irq(irq, mxc_keypad_irq_handler,
> +               IRQF_DISABLED, pdev->name, kp);
>
..
> +failed_free_irq:
> +       free_irq(irq, pdev);

This needs to be free_irq(irq, kp)  to match the request_irq

> +static int __devexit mxc_keypad_remove(struct platform_device *pdev)
> +{
..
> +               free_irq(kp->irq, pdev);

Idem - this caused module ulooad / reload to fail since the IRQ was
not freed and was seen as busy the second time;

Regards,

Martin
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html