Re: [PATCH v4] input: MXC: add mxc-keypad driver to support the Keypad Port present in the mxc application processors family.

[Alberto Panizzo <maramaopercheseimorto@gmail.com>]

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:
> > 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!
> 
> > +
> > +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.