lkml.org 
[lkml]   [2010]   [May]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] matrix_keypad: add support for clustered irq
    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 <l.fu@pengutronix.de> wrote:
    > > 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 <l.fu@pengutronix.de>
    > > ---
    > >  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.

    >
    > >                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.
    >
    > >                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.
    >
    > >                }
    > >        }
    > >
    > > @@ -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 |
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2010-05-26 16:41    [W:0.066 / U:33.672 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site