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.

...

+
+#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?

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