Hi Dmitry,

On mer, 2010-01-27 at 10:33 -0800, Dmitry Torokhov wrote:

Hi Alberto,

On Wed, Jan 27, 2010 at 06:50:44PM +0100, Alberto Panizzo wrote:

quoted

The MXC family of Application Processors is shipped with a Keypad Port
supported now by this driver.

The peripheral can control up to an 8x8 matrix key pad where all the scanning
procedure is done via software.

The hardware provide two interrupts: one for a key pressed (KDI) and one for
all key releases (KRI). There is also a simple circuit for glitch reduction
(said for synchronization) made by two series of 3 D-latches clocked by the
keypad-clock that stabilize the interrupts sources.
KDI and KRI are fired only if the respective conditions are maintained for at
last 4 keypad-clock cycle.

Those simple synchronization circuits are used also for multiple key pressures:
between a KDI and a KRI the driver reset the sync circuit and re-enable the KDI
interrupt so after 3 keypad-clock cycle another KDI is fired making possible to
repeat the matrix scan operation.

This algorithm is done through the interrupt management code and delayed by a
proper (and longer) debounce interval controlled by the platform initialization.
If a key is pressed for a lot of time, the driver relaxes the interrupt re-enabling
procedure to not over load the cpu in a long time keypad interaction.

I was looking at the debounce logic and I am not quite sure about it.
Normally you have 2 ways for dealing with jitter:

1. You let interrupts to come in and reschedule the scan until they stop
   arriving. Then to tak ethe stable reading.

2. You inhibit interrupt, take a reading and schedule another reading in
   the future. If they match you decide that reading is stable otherwise
    you schedule another reading.

In your case you seem to be simply postponing the reading but this does
not guarantee that the reading is stable.

Yes, because of the glitch suppression circuit, I suppose that when
an interrupt arrive, it is a key pressure for sure.
Then I assume that the matrix will be stable after a proper debounce
time (test look fine with 20 ms).

1 should be a more accurate way, I can study an implementation.

I also do not think that yopu need 2 timers - you can easily requeue
currently running timer.

The first version I made was with one timer: if for too many repeating 
interrupts the matrix state do not change, the scanning procedure was 
scheduled with a summed delay.

It resulted in a degradation of responsiveness and more key pressure 
losing. It is a better choice to let the scanning procedure near the 
interrupt.

Changing the timer handler over the time? Would be acceptable?

BTW, you need to pay close attention to the races between re-enabling
intterrupts (form the timer context) and inhibiting interrupts when you
close the device. Currently there is a race - if you close the device
while scan is scheduled the timer will re-enable them again. You disable
all rows so I am not sure if it is possible for interrupts to be raised
again at this point, but if it is, then you porbbaly need a spinlock
there.

This is a true race condition that I've not caught!

quoted

+
+static void mxc_keypad_relax_timer_handler(unsigned long data)
+{
+	struct mxc_keypad *keypad = (struct mxc_keypad *) data;
+	unsigned short reg_val;
+
+	/* 10. Clear KPKD and KPKR status bits
+	 *     Set the KPKR sync chain and clear the KPKD sync chain */
+	reg_val = readw(keypad->mmio_base + KPSR);
+	reg_val |= KBD_STAT_KPKD | KBD_STAT_KPKR |
+		   KBD_STAT_KDSC | KBD_STAT_KRSS;
+	writew(reg_val, keypad->mmio_base + KPSR);
+
+	/* Re enable interrupts and clear sync reset bits.
+	 * Next KDI is used for detect multiple pressures. */
+	reg_val = readw(keypad->mmio_base + KPSR);
+	reg_val &= ~(KBD_STAT_KDSC | KBD_STAT_KRSS);
+	writew(reg_val, keypad->mmio_base + KPSR);
+
+	reg_val |= KBD_STAT_KDIE | KBD_STAT_KRIE;
+	if (keypad->irq_type == MXC_IRQ_KRI)
+		reg_val &= ~KBD_STAT_KRIE;

So we are keeping the press interrupt always... As far as I understand
this will cause us to effectively poll the matrix as long as at least
one key is pressed. Why do we even need to bother with release interrupt
then?

Thanks.

I always keep the press interrupt because of matrix-rescan in a long 
time pressure of a key, to find multiple key pressures.
Resetting the press interrupt synchronization chain will reset also 
the interrupt status bit, and if the press condition hold for the next
3*keypad_clock_periods, it is fired another press interrupt.

I can think at an architecture that after a key pressure event
continually reschedule a matrix scan in the future to look for changes
in the matrix until all keys are released.. but need a consistent 
analysis of timings.