Sekhar,

Nori, Sekhar wrote:
On Sat, Oct 03, 2009 at 00:32:30, [email protected] wrote:
From: Miguel Aguilar <[email protected]>

Adds the driver for enabling keypad support for DaVinci platforms.

DM365 is the only platform that uses this driver at the moment.

Signed-off-by: Miguel Aguilar <[email protected]>

[...]

+/* Initializing the kp Module */
+static int davinci_ks_initialize(struct davinci_ks *davinci_ks)
+{
+     struct device *dev = &davinci_ks->input->dev;
+     u8 strobe = davinci_ks->pdata->strobe;
+     u8 interval = davinci_ks->pdata->interval;
+     u8 rows = davinci_ks->pdata->rows;
+     u8 cols = davinci_ks->pdata->cols;
+     u8 matrix_type = 0;
+
+     /* Enable all interrupts */
+     davinci_ks_write(davinci_ks, DAVINCI_KEYSCAN_INT_ALL, 
DAVINCI_KEYSCAN_INTENA);
+
+     /* Clear interrupts if any */
+     davinci_ks_write(davinci_ks, DAVINCI_KEYSCAN_INT_ALL, 
DAVINCI_KEYSCAN_INTCLR);
+
+     /* Setup the scan period = strobe + interval */
+     davinci_ks_write(davinci_ks, strobe, DAVINCI_KEYSCAN_STRBWIDTH);
+     davinci_ks_write(davinci_ks, interval, DAVINCI_KEYSCAN_INTERVAL);
+     davinci_ks_write(davinci_ks, 0x01, DAVINCI_KEYSCAN_CONTTIME);
+
+     /* Define matrix type */
+     if ((rows == 4) && (cols == 4))
+             matrix_type = 0;
+     else if ((rows == 3) && (cols == 5))
+             matrix_type = 1;
+     else {
+             dev_err(dev, "davinci_scan: wrong matrix dimension\n");

dev_err automatically inserts the driver name. No need
to do it again.
Driver name in this case is input0 not davinci_keyscan, because struct device *dev = &davinci_ks->input->dev returns input0, so "davinci_keyscan" this is needed for clarification, this is different from the other instances in the probe function.

+             return -EINVAL;
+     }
+
+     /* Enable key scan module and set matrix type */
+     davinci_ks_write(davinci_ks, DAVINCI_KEYSCAN_AUTODET | 
DAVINCI_KEYSCAN_KEYEN
+                         | (matrix_type << 6), DAVINCI_KEYSCAN_KEYCTRL);
+
+     return 0;
+}
+
+static irqreturn_t davinci_ks_interrupt(int irq, void *dev_id)
+{
+     struct davinci_ks *davinci_ks = dev_id;
+     struct device *dev = &davinci_ks->input->dev;
+     unsigned short *keymap = davinci_ks->keymap;
+     int keymapsize = davinci_ks->pdata->keymapsize;
+     u32 prev_status, new_status, changed;
+     bool release;
+     int keycode = KEY_UNKNOWN;
+     int i;
+
+     /* Disable interrupt */
+     davinci_ks_write(davinci_ks, 0x0, DAVINCI_KEYSCAN_INTENA);
+
+     /* Reading previous and new status of the key scan */
+     prev_status = davinci_ks_read(davinci_ks, DAVINCI_KEYSCAN_PREVSTATE);
+     new_status = davinci_ks_read(davinci_ks, DAVINCI_KEYSCAN_CURRENTST);
+
+     changed = prev_status ^ new_status;
+
+     if(changed) {
+             /*
+              * It goes go through all bits in changed to ensure
+              * that no key changes are being missed
+              */
+             for(i = 0 ; i < keymapsize; i++) {
+                     if((changed>>i) & 0x1) {
+                             keycode = keymap[i];
+                             release = (new_status >> i) & 0x1;
+                             dev_dbg(dev, "davinci_keyscan: key %d %s\n",
+                                 keycode, release ? "released" : "pressed");

dev_dbg adds the driver name as well. Please
look for other instances of this in the patch.
[MA] Same.

+                             input_report_key(davinci_ks->input, keycode,
+                                                 !release);
+                             input_sync(davinci_ks->input);
+                     }
+             }
+             /* Clearing interrupt */
+             davinci_ks_write(davinci_ks, DAVINCI_KEYSCAN_INT_ALL,
+                                 DAVINCI_KEYSCAN_INTCLR);
+     }
+
+     /* Enable interrupts */
+     davinci_ks_write(davinci_ks, 0x1, DAVINCI_KEYSCAN_INTENA);
+
+     return IRQ_HANDLED;

I see you have retracted the earlier implementation using
ffs(). I think you said the device can handle only one key
state change per interrupt, the loop is not needed if that's
the case.
The current implementation checks the state of all the state bits which is better than just checking one bit with ffs.

Also now you are not returning IRQ_NONE if you don't see a
key state change.

Can you explain why this was changed?
In this case I addressed a comment from Dmitry, where he explained me that IRQ_NONE is not needed.

Thanks,
Sekhar





_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source

Reply via email to