Hi Eric,

thankf for the review.

On Wed, May 26, 2010 at 10:29:53PM +0800, Eric Miao wrote:

On Wed, May 26, 2010 at 9:57 PM, Luotao Fu [off-list ref] wrote:

quoted

This one adds support of a combined irq source for the whole matrix keypad.
This can be useful all rows and columns of the keypad is e.g. connected to
a GPIO expander, which only has one interrupt line for all events on every
single GPIO.

Signed-off-by: Luotao Fu <redacted>
---
 drivers/input/keyboard/matrix_keypad.c |   80 ++++++++++++++++++++++++++------
 include/linux/input/matrix_keypad.h    |    5 ++
 2 files changed, 70 insertions(+), 15 deletions(-)

diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
index b443e08..260dcb0 100644
--- a/drivers/input/keyboard/matrix_keypad.c
+++ b/drivers/input/keyboard/matrix_keypad.c

@@ -87,8 +87,12 @@ static void enable_row_irqs(struct matrix_keypad *keypad)

       const struct matrix_keypad_platform_data *pdata = keypad->pdata;
       int i;

-       for (i = 0; i < pdata->num_row_gpios; i++)
-               enable_irq(gpio_to_irq(pdata->row_gpios[i]));
+       if (pdata->clustered_irq > 0)
+               enable_irq(pdata->clustered_irq);
+       else {
+               for (i = 0; i < pdata->num_row_gpios; i++)
+                       enable_irq(gpio_to_irq(pdata->row_gpios[i]));
+       }
 }

 static void disable_row_irqs(struct matrix_keypad *keypad)

@@ -96,8 +100,12 @@ static void disable_row_irqs(struct matrix_keypad *keypad)

       const struct matrix_keypad_platform_data *pdata = keypad->pdata;
       int i;

-       for (i = 0; i < pdata->num_row_gpios; i++)
-               disable_irq_nosync(gpio_to_irq(pdata->row_gpios[i]));
+       if (pdata->clustered_irq > 0)
+               disable_irq_nosync(pdata->clustered_irq);
+       else {
+               for (i = 0; i < pdata->num_row_gpios; i++)
+                       disable_irq_nosync(gpio_to_irq(pdata->row_gpios[i]));
+       }
 }

 /*

@@ -226,6 +234,17 @@ static int matrix_keypad_suspend(struct device *dev)

       matrix_keypad_stop(keypad->input_dev);

       if (device_may_wakeup(&pdev->dev)) {
+               if (pdata->clustered_irq > 0) {
+                       unsigned int cirq = pdata->clustered_irq;
+
+                       if (enable_irq_wake(cirq) == 0) {
+                               for (i = 0; i < pdata->num_row_gpios; i++)
+                                       __set_bit(i, keypad->disabled_gpios);
+                       }
+
+                       goto out;
+               }
+

This might deserve a separate function, and what is the usage of disabled_gpios,
I seem to find nowhere (except in the resume routine).

indeed, will fix. The disable_gpios flag seems to be used to flag an
asleep/awake row/column. It's introduced by the original code. I think
that for the case with single irq I might better use another flag
instead since we have the situaiton "all or none" here and don't have to
mark every single lines.

quoted

               for (i = 0; i < pdata->num_row_gpios; i++) {
                       if (!test_bit(i, keypad->disabled_gpios)) {
                               unsigned int gpio = pdata->row_gpios[i];

@@ -235,7 +254,7 @@ static int matrix_keypad_suspend(struct device *dev)

                       }
               }
       }
-
+out:
       return 0;
 }

@@ -247,6 +266,17 @@ static int matrix_keypad_resume(struct device *dev)

       int i;

       if (device_may_wakeup(&pdev->dev)) {
+               if (pdata->clustered_irq > 0) {
+                       unsigned int cirq = pdata->clustered_irq;
+
+                       if (disable_irq_wake(cirq) == 0) {
+                               for (i = 0; i < pdata->num_row_gpios; i++)
+                                       clear_bit(i, keypad->disabled_gpios);
+                       }
+
+                       goto out;
+               }
+

Better a separate routine.

quoted

               for (i = 0; i < pdata->num_row_gpios; i++) {
                       if (test_and_clear_bit(i, keypad->disabled_gpios)) {
                               unsigned int gpio = pdata->row_gpios[i];

@@ -256,6 +286,7 @@ static int matrix_keypad_resume(struct device *dev)

               }
       }

+out:
       matrix_keypad_start(keypad->input_dev);

       return 0;

@@ -296,17 +327,31 @@ static int __devinit init_matrix_gpio(struct platform_device *pdev,

               gpio_direction_input(pdata->row_gpios[i]);
       }

-       for (i = 0; i < pdata->num_row_gpios; i++) {
-               err = request_irq(gpio_to_irq(pdata->row_gpios[i]),
+       if (pdata->clustered_irq > 0) {
+               err = request_irq(pdata->clustered_irq,
                               matrix_keypad_interrupt,
-                               IRQF_DISABLED |
-                               IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+                               pdata->clustered_irq_flags,
                               "matrix-keypad", keypad);
               if (err) {
                       dev_err(&pdev->dev,
-                               "Unable to acquire interrupt for GPIO line %i\n",
-                               pdata->row_gpios[i]);
-                       goto err_free_irqs;
+                               "Unable to acquire clustered interrupt\n");
+                       goto err_free_rows;
+               }
+       } else {
+               for (i = 0; i < pdata->num_row_gpios; i++) {
+                       err = request_irq(gpio_to_irq(pdata->row_gpios[i]),
+                                       matrix_keypad_interrupt,
+                                       IRQF_DISABLED |
+                                       IRQF_TRIGGER_RISING |
+                                       IRQF_TRIGGER_FALLING,
+                                       "matrix-keypad", keypad);
+                       if (err) {
+                               dev_err(&pdev->dev,
+                                       "Unable to acquire interrupt "
+                                       "for GPIO line %i\n",
+                                       pdata->row_gpios[i]);
+                               goto err_free_irqs;
+                       }

Ditto.

quoted

               }
       }

@@ -418,11 +463,16 @@ static int __devexit matrix_keypad_remove(struct platform_device *pdev)

       device_init_wakeup(&pdev->dev, 0);

-       for (i = 0; i < pdata->num_row_gpios; i++) {
-               free_irq(gpio_to_irq(pdata->row_gpios[i]), keypad);
-               gpio_free(pdata->row_gpios[i]);
+       if (pdata->clustered_irq > 0) {
+               free_irq(pdata->clustered_irq, keypad);
+       } else {
+               for (i = 0; i < pdata->num_row_gpios; i++)
+                       free_irq(gpio_to_irq(pdata->row_gpios[i]), keypad);
       }

+       for (i = 0; i < pdata->num_row_gpios; i++)
+               gpio_free(pdata->row_gpios[i]);
+
       for (i = 0; i < pdata->num_col_gpios; i++)
               gpio_free(pdata->col_gpios[i]);

diff --git a/include/linux/input/matrix_keypad.h b/include/linux/input/matrix_keypad.h
index c964cd7..8e8dc2e 100644
--- a/include/linux/input/matrix_keypad.h
+++ b/include/linux/input/matrix_keypad.h

@@ -63,6 +63,11 @@ struct matrix_keypad_platform_data {

       /* key debounce interval in milli-second */
       unsigned int    debounce_ms;

+       /* used if interrupts of all row/column GPIOs are bundled to one single
+        * irq */
+       unsigned int    clustered_irq;
+       unsigned int    clustered_irq_flags;
+
       bool            active_low;
       bool            wakeup;
       bool            no_autorepeat;
--
1.7.0

Otherwise looks OK to me.

Thanks

cheers
Luotao Fu
-- 
Pengutronix e.K.                           | Dipl.-Ing. Luotao Fu        |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |