diff mbox series

[v3,01/10] input: keyboard: adp5588-keys: support gpi key events as 'gpio keys'

Message ID 20220721080423.156151-2-nuno.sa@analog.com
State Superseded
Headers show
Series adp5588-keys refactor and fw properties support | expand

Commit Message

Nuno Sa July 21, 2022, 8:04 a.m. UTC
This change replaces the support for GPIs as key event generators.
Instead of reporting the events directly, we add a gpio based irqchip
so that these events can be consumed by keys defined in the gpio-keys
driver (as it's goal is indeed for keys on GPIOs capable of generating
interrupts). With this, the gpio-adp5588 driver can also be dropped.

The basic idea is that all the pins that are not being used as part of
the keymap matrix can be possibly requested as GPIOs by gpio-keys
(it's also fine to use these pins as plain interrupts though that's not
really the point).

Since the gpiochip now also has irqchip capabilities, we should only
remove it after we free the device interrupt (otherwise we could, in
theory, be handling GPIs interrupts while the gpiochip is concurrently
removed). Thus the call 'adp5588_gpio_add()' is moved and since the
setup phase also needs to come before making the gpios visible, we also
need to move 'adp5588_setup()'.

While at it, always select GPIOLIB so that we don't need to use #ifdef
guards.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/input/keyboard/Kconfig        |   2 +
 drivers/input/keyboard/adp5588-keys.c | 269 +++++++++++++-------------
 include/linux/platform_data/adp5588.h |   2 -
 3 files changed, 139 insertions(+), 134 deletions(-)

Comments

kernel test robot July 23, 2022, 4:56 a.m. UTC | #1
Hi "Nuno,

I love your patch! Perhaps something to improve:

[auto build test WARNING on dtor-input/next]
[also build test WARNING on next-20220722]
[cannot apply to brgl/gpio/for-next hid/for-next linus/master v5.19-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Nuno-S/adp5588-keys-refactor-and-fw-properties-support/20220721-160531
base:   https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
config: x86_64-randconfig-a005 (https://download.01.org/0day-ci/archive/20220723/202207231228.n8l077iB-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 12fbd2d377e396ad61bce56d71c98a1eb1bebfa9)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/357979f7c2525297178fb321c5793a4bd63dabb6
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Nuno-S/adp5588-keys-refactor-and-fw-properties-support/20220721-160531
        git checkout 357979f7c2525297178fb321c5793a4bd63dabb6
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/input/keyboard/ drivers/platform/x86/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/input/keyboard/adp5588-keys.c:336:9: warning: implicit conversion from 'unsigned long' to 'int' changes value from 18446744073709551615 to -1 [-Wconstant-conversion]
           return INVALID_HWIRQ;
           ~~~~~~ ^~~~~~~~~~~~~
   include/linux/irq.h:1245:24: note: expanded from macro 'INVALID_HWIRQ'
   #define INVALID_HWIRQ   (~0UL)
                            ^~~~
   1 warning generated.


vim +336 drivers/input/keyboard/adp5588-keys.c

   323	
   324	static int adp5588_gpiomap_get_hwirq(struct device *dev, const u8 *map,
   325					     unsigned int gpio, unsigned int ngpios)
   326	{
   327		unsigned int hwirq;
   328	
   329		for (hwirq = 0; hwirq < ngpios; hwirq++)
   330			if (map[hwirq] == gpio)
   331				return hwirq;
   332	
   333		/* should never happen */
   334		dev_warn_ratelimited(dev, "could not find the hwirq for gpio(%u)\n", gpio);
   335	
 > 336		return INVALID_HWIRQ;
   337	}
   338
Andy Shevchenko July 25, 2022, 8:31 p.m. UTC | #2
On Sat, Jul 23, 2022 at 6:57 AM kernel test robot <lkp@intel.com> wrote:


...

> All warnings (new ones prefixed by >>):
>
> >> drivers/input/keyboard/adp5588-keys.c:336:9: warning: implicit conversion from 'unsigned long' to 'int' changes value from 18446744073709551615 to -1 [-Wconstant-conversion]
>            return INVALID_HWIRQ;
>            ~~~~~~ ^~~~~~~~~~~~~
>    include/linux/irq.h:1245:24: note: expanded from macro 'INVALID_HWIRQ'
>    #define INVALID_HWIRQ   (~0UL)
>                             ^~~~

It requires irq.h, but hold on. Marc said that he wants to rather kill
that definition than having it spread over the kernel. So, please use
your own custom macro with an appropriate type.
Andy Shevchenko July 25, 2022, 8:34 p.m. UTC | #3
On Thu, Jul 21, 2022 at 10:03 AM Nuno Sá <nuno.sa@analog.com> wrote:
>
> This change replaces the support for GPIs as key event generators.
> Instead of reporting the events directly, we add a gpio based irqchip
> so that these events can be consumed by keys defined in the gpio-keys
> driver (as it's goal is indeed for keys on GPIOs capable of generating
> interrupts). With this, the gpio-adp5588 driver can also be dropped.
>
> The basic idea is that all the pins that are not being used as part of
> the keymap matrix can be possibly requested as GPIOs by gpio-keys
> (it's also fine to use these pins as plain interrupts though that's not
> really the point).
>
> Since the gpiochip now also has irqchip capabilities, we should only
> remove it after we free the device interrupt (otherwise we could, in
> theory, be handling GPIs interrupts while the gpiochip is concurrently
> removed). Thus the call 'adp5588_gpio_add()' is moved and since the
> setup phase also needs to come before making the gpios visible, we also
> need to move 'adp5588_setup()'.
>
> While at it, always select GPIOLIB so that we don't need to use #ifdef
> guards.

...

> +static void adp5588_irq_mask(struct irq_data *d)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct adp5588_kpad *kpad = gpiochip_get_data(gc);

Can it be

  irq_hw_number_t hwirq = irqd_to_hwirq(d);

?

> +       unsigned long real_irq = kpad->gpiomap[irqd_to_hwirq(d)];
> +
> +       kpad->irq_mask[ADP5588_BANK(real_irq)] &= ~ADP5588_BIT(real_irq);
> +       gpiochip_disable_irq(gc, irqd_to_hwirq(d));
> +}
> +
> +static void adp5588_irq_unmask(struct irq_data *d)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct adp5588_kpad *kpad = gpiochip_get_data(gc);
> +       unsigned long real_irq = kpad->gpiomap[irqd_to_hwirq(d)];
> +
> +       gpiochip_enable_irq(gc, irqd_to_hwirq(d));
> +       kpad->irq_mask[ADP5588_BANK(real_irq)] |= ADP5588_BIT(real_irq);

Ditto.

> +}

...

> +                       /* gpio line used as IRQ source */

GPIO
kernel test robot July 26, 2022, 10:30 a.m. UTC | #4
Hi "Nuno,

I love your patch! Perhaps something to improve:

[auto build test WARNING on dtor-input/next]
[also build test WARNING on next-20220725]
[cannot apply to brgl/gpio/for-next hid/for-next linus/master v5.19-rc8]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Nuno-S/adp5588-keys-refactor-and-fw-properties-support/20220721-160531
base:   https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
config: x86_64-randconfig-a013 (https://download.01.org/0day-ci/archive/20220726/202207261830.c3VuQ5P0-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/357979f7c2525297178fb321c5793a4bd63dabb6
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Nuno-S/adp5588-keys-refactor-and-fw-properties-support/20220721-160531
        git checkout 357979f7c2525297178fb321c5793a4bd63dabb6
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/input/keyboard/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/gpio/driver.h:6,
                    from drivers/input/keyboard/adp5588-keys.c:13:
   drivers/input/keyboard/adp5588-keys.c: In function 'adp5588_gpiomap_get_hwirq':
>> include/linux/irq.h:1245:25: warning: overflow in conversion from 'long unsigned int' to 'int' changes value from '18446744073709551615' to '-1' [-Woverflow]
    1245 | #define INVALID_HWIRQ   (~0UL)
         |                         ^
   drivers/input/keyboard/adp5588-keys.c:336:16: note: in expansion of macro 'INVALID_HWIRQ'
     336 |         return INVALID_HWIRQ;
         |                ^~~~~~~~~~~~~


vim +1245 include/linux/irq.h

2f75d9e1c90511 Thomas Gleixner 2017-09-13  1243  
d17bf24e695290 Qais Yousef     2015-12-08  1244  /* Contrary to Linux irqs, for hardware irqs the irq number 0 is valid */
d17bf24e695290 Qais Yousef     2015-12-08 @1245  #define INVALID_HWIRQ	(~0UL)
f9bce791ae2a1a Qais Yousef     2015-12-08  1246  irq_hw_number_t ipi_get_hwirq(unsigned int irq, unsigned int cpu);
3b8e29a82dd16c Qais Yousef     2015-12-08  1247  int __ipi_send_single(struct irq_desc *desc, unsigned int cpu);
3b8e29a82dd16c Qais Yousef     2015-12-08  1248  int __ipi_send_mask(struct irq_desc *desc, const struct cpumask *dest);
3b8e29a82dd16c Qais Yousef     2015-12-08  1249  int ipi_send_single(unsigned int virq, unsigned int cpu);
3b8e29a82dd16c Qais Yousef     2015-12-08  1250  int ipi_send_mask(unsigned int virq, const struct cpumask *dest);
d17bf24e695290 Qais Yousef     2015-12-08  1251
Nuno Sa Aug. 29, 2022, 11:20 a.m. UTC | #5
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Monday, July 25, 2022 10:34 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-input <linux-input@vger.kernel.org>; open list:GPIO
> SUBSYSTEM <linux-gpio@vger.kernel.org>; devicetree
> <devicetree@vger.kernel.org>; Linus Walleij
> <linus.walleij@linaro.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Bartosz Golaszewski
> <brgl@bgdev.pl>; Rob Herring <robh+dt@kernel.org>; Dmitry
> Torokhov <dmitry.torokhov@gmail.com>
> Subject: Re: [PATCH v3 01/10] input: keyboard: adp5588-keys: support
> gpi key events as 'gpio keys'
> 
> [External]
> 
> On Thu, Jul 21, 2022 at 10:03 AM Nuno Sá <nuno.sa@analog.com>
> wrote:
> >
> > This change replaces the support for GPIs as key event generators.
> > Instead of reporting the events directly, we add a gpio based irqchip
> > so that these events can be consumed by keys defined in the gpio-
> keys
> > driver (as it's goal is indeed for keys on GPIOs capable of generating
> > interrupts). With this, the gpio-adp5588 driver can also be dropped.
> >
> > The basic idea is that all the pins that are not being used as part of
> > the keymap matrix can be possibly requested as GPIOs by gpio-keys
> > (it's also fine to use these pins as plain interrupts though that's not
> > really the point).
> >
> > Since the gpiochip now also has irqchip capabilities, we should only
> > remove it after we free the device interrupt (otherwise we could, in
> > theory, be handling GPIs interrupts while the gpiochip is concurrently
> > removed). Thus the call 'adp5588_gpio_add()' is moved and since the
> > setup phase also needs to come before making the gpios visible, we
> also
> > need to move 'adp5588_setup()'.
> >
> > While at it, always select GPIOLIB so that we don't need to use #ifdef
> > guards.
> 
> ...
> 
> > +static void adp5588_irq_mask(struct irq_data *d)
> > +{
> > +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> > +       struct adp5588_kpad *kpad = gpiochip_get_data(gc);
> 
> Can it be
> 
>   irq_hw_number_t hwirq = irqd_to_hwirq(d);
> 
> ?
> 

Yeah, I thought about doing that way but in the end just
"inlined" the call. Anyways, no strong feeling so I can have it
as you prefer...

- Nuno Sá
Nuno Sa Aug. 29, 2022, 11:21 a.m. UTC | #6
> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Monday, July 25, 2022 10:32 PM
> To: kernel test robot <lkp@intel.com>
> Cc: Sa, Nuno <Nuno.Sa@analog.com>; linux-input <linux-
> input@vger.kernel.org>; open list:GPIO SUBSYSTEM <linux-
> gpio@vger.kernel.org>; devicetree <devicetree@vger.kernel.org>;
> llvm@lists.linux.dev; kbuild-all@lists.01.org; Linus Walleij
> <linus.walleij@linaro.org>; Krzysztof Kozlowski <krzk@kernel.org>;
> Hennerich, Michael <Michael.Hennerich@analog.com>; Bartosz
> Golaszewski <brgl@bgdev.pl>; Rob Herring <robh+dt@kernel.org>;
> Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Subject: Re: [PATCH v3 01/10] input: keyboard: adp5588-keys: support
> gpi key events as 'gpio keys'
> 
> [External]
> 
> On Sat, Jul 23, 2022 at 6:57 AM kernel test robot <lkp@intel.com>
> wrote:
> 
> 
> ...
> 
> > All warnings (new ones prefixed by >>):
> >
> > >> drivers/input/keyboard/adp5588-keys.c:336:9: warning: implicit
> conversion from 'unsigned long' to 'int' changes value from
> 18446744073709551615 to -1 [-Wconstant-conversion]
> >            return INVALID_HWIRQ;
> >            ~~~~~~ ^~~~~~~~~~~~~
> >    include/linux/irq.h:1245:24: note: expanded from macro
> 'INVALID_HWIRQ'
> >    #define INVALID_HWIRQ   (~0UL)
> >                             ^~~~
> 
> It requires irq.h, but hold on. Marc said that he wants to rather kill
> that definition than having it spread over the kernel. So, please use
> your own custom macro with an appropriate type.
> 

I see, I'll probably just replicate the macro with an "ADP5588" prefix...

Thx,
- Nuno Sá
Linus Walleij Aug. 31, 2022, 12:24 p.m. UTC | #7
On Thu, Jul 21, 2022 at 10:03 AM Nuno Sá <nuno.sa@analog.com> wrote:

> This change replaces the support for GPIs as key event generators.
> Instead of reporting the events directly, we add a gpio based irqchip
> so that these events can be consumed by keys defined in the gpio-keys
> driver (as it's goal is indeed for keys on GPIOs capable of generating
> interrupts). With this, the gpio-adp5588 driver can also be dropped.
>
> The basic idea is that all the pins that are not being used as part of
> the keymap matrix can be possibly requested as GPIOs by gpio-keys
> (it's also fine to use these pins as plain interrupts though that's not
> really the point).
>
> Since the gpiochip now also has irqchip capabilities, we should only
> remove it after we free the device interrupt (otherwise we could, in
> theory, be handling GPIs interrupts while the gpiochip is concurrently
> removed). Thus the call 'adp5588_gpio_add()' is moved and since the
> setup phase also needs to come before making the gpios visible, we also
> need to move 'adp5588_setup()'.
>
> While at it, always select GPIOLIB so that we don't need to use #ifdef
> guards.
>
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>

Is there any input-related content in the driver after this or
should the whole driver just be moved over to drivers/gpio
and replace the old GPIO driver that you delete in patch
2?

Yours,
Linus Walleij
Andy Shevchenko Aug. 31, 2022, 12:26 p.m. UTC | #8
On Wed, Aug 31, 2022 at 3:24 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Jul 21, 2022 at 10:03 AM Nuno Sá <nuno.sa@analog.com> wrote:

...

> Is there any input-related content in the driver after this or
> should the whole driver just be moved over to drivers/gpio
> and replace the old GPIO driver that you delete in patch
> 2?

Looking at the Kconfig description I would say this might be a
candidate for splitting and MFD.
Nuno Sa Aug. 31, 2022, 12:28 p.m. UTC | #9
> From: Linus Walleij <linus.walleij@linaro.org>
> Sent: Wednesday, August 31, 2022 2:24 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-input@vger.kernel.org; linux-gpio@vger.kernel.org;
> devicetree@vger.kernel.org; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Bartosz Golaszewski
> <brgl@bgdev.pl>; Andy Shevchenko <andy.shevchenko@gmail.com>;
> Rob Herring <robh+dt@kernel.org>; Dmitry Torokhov
> <dmitry.torokhov@gmail.com>
> Subject: Re: [PATCH v3 01/10] input: keyboard: adp5588-keys: support
> gpi key events as 'gpio keys'
> 
> [External]
> 
> On Thu, Jul 21, 2022 at 10:03 AM Nuno Sá <nuno.sa@analog.com>
> wrote:
> 
> > This change replaces the support for GPIs as key event generators.
> > Instead of reporting the events directly, we add a gpio based irqchip
> > so that these events can be consumed by keys defined in the gpio-
> keys
> > driver (as it's goal is indeed for keys on GPIOs capable of generating
> > interrupts). With this, the gpio-adp5588 driver can also be dropped.
> >
> > The basic idea is that all the pins that are not being used as part of
> > the keymap matrix can be possibly requested as GPIOs by gpio-keys
> > (it's also fine to use these pins as plain interrupts though that's not
> > really the point).
> >
> > Since the gpiochip now also has irqchip capabilities, we should only
> > remove it after we free the device interrupt (otherwise we could, in
> > theory, be handling GPIs interrupts while the gpiochip is concurrently
> > removed). Thus the call 'adp5588_gpio_add()' is moved and since the
> > setup phase also needs to come before making the gpios visible, we
> also
> > need to move 'adp5588_setup()'.
> >
> > While at it, always select GPIOLIB so that we don't need to use #ifdef
> > guards.
> >
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> 
> Is there any input-related content in the driver after this or
> should the whole driver just be moved over to drivers/gpio
> and replace the old GPIO driver that you delete in patch
> 2?
> 

Hi,

Yes there is... The main usage for the device is to use it as a
matrix keypad. However, all the keys that are not used will be then
exposed through a gpiochip to be consumed for example by gpio-keys

- Nuno Sá
Linus Walleij Aug. 31, 2022, 12:30 p.m. UTC | #10
On Wed, Aug 31, 2022 at 2:28 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:
>
> > From: Linus Walleij <linus.walleij@linaro.org>
> > Sent: Wednesday, August 31, 2022 2:24 PM
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: linux-input@vger.kernel.org; linux-gpio@vger.kernel.org;
> > devicetree@vger.kernel.org; Krzysztof Kozlowski
> > <krzysztof.kozlowski+dt@linaro.org>; Hennerich, Michael
> > <Michael.Hennerich@analog.com>; Bartosz Golaszewski
> > <brgl@bgdev.pl>; Andy Shevchenko <andy.shevchenko@gmail.com>;
> > Rob Herring <robh+dt@kernel.org>; Dmitry Torokhov
> > <dmitry.torokhov@gmail.com>
> > Subject: Re: [PATCH v3 01/10] input: keyboard: adp5588-keys: support
> > gpi key events as 'gpio keys'
> >
> > [External]
> >
> > On Thu, Jul 21, 2022 at 10:03 AM Nuno Sá <nuno.sa@analog.com>
> > wrote:
> >
> > > This change replaces the support for GPIs as key event generators.
> > > Instead of reporting the events directly, we add a gpio based irqchip
> > > so that these events can be consumed by keys defined in the gpio-
> > keys
> > > driver (as it's goal is indeed for keys on GPIOs capable of generating
> > > interrupts). With this, the gpio-adp5588 driver can also be dropped.
> > >
> > > The basic idea is that all the pins that are not being used as part of
> > > the keymap matrix can be possibly requested as GPIOs by gpio-keys
> > > (it's also fine to use these pins as plain interrupts though that's not
> > > really the point).
> > >
> > > Since the gpiochip now also has irqchip capabilities, we should only
> > > remove it after we free the device interrupt (otherwise we could, in
> > > theory, be handling GPIs interrupts while the gpiochip is concurrently
> > > removed). Thus the call 'adp5588_gpio_add()' is moved and since the
> > > setup phase also needs to come before making the gpios visible, we
> > also
> > > need to move 'adp5588_setup()'.
> > >
> > > While at it, always select GPIOLIB so that we don't need to use #ifdef
> > > guards.
> > >
> > > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> >
> > Is there any input-related content in the driver after this or
> > should the whole driver just be moved over to drivers/gpio
> > and replace the old GPIO driver that you delete in patch
> > 2?
> >
>
> Hi,
>
> Yes there is... The main usage for the device is to use it as a
> matrix keypad. However, all the keys that are not used will be then
> exposed through a gpiochip to be consumed for example by gpio-keys

OK then, just checking. Let's keep it here for now, consider the MFD
approach suggested by Andy (splitting out the gpio chip and input
parts in two files in drivers/gpio and drivers/input) for the long run.

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index a20ee693b22b..ca5cd5e520a7 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -40,6 +40,8 @@  config KEYBOARD_ADP5520
 config KEYBOARD_ADP5588
 	tristate "ADP5588/87 I2C QWERTY Keypad and IO Expander"
 	depends on I2C
+	select GPIOLIB
+	select GPIOLIB_IRQCHIP
 	help
 	  Say Y here if you want to use a ADP5588/87 attached to your
 	  system I2C bus.
diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
index 1a1a05d7cd42..c63f95f86ba2 100644
--- a/drivers/input/keyboard/adp5588-keys.c
+++ b/drivers/input/keyboard/adp5588-keys.c
@@ -46,15 +46,13 @@  struct adp5588_kpad {
 	ktime_t irq_time;
 	unsigned long delay;
 	unsigned short keycode[ADP5588_KEYMAPSIZE];
-	const struct adp5588_gpi_map *gpimap;
-	unsigned short gpimapsize;
-#ifdef CONFIG_GPIOLIB
 	unsigned char gpiomap[ADP5588_MAXGPIO];
 	struct gpio_chip gc;
 	struct mutex gpio_lock;	/* Protect cached dir, dat_out */
 	u8 dat_out[3];
 	u8 dir[3];
-#endif
+	u8 int_en[3];
+	u8 irq_mask[3];
 };
 
 static int adp5588_read(struct i2c_client *client, u8 reg)
@@ -72,7 +70,6 @@  static int adp5588_write(struct i2c_client *client, u8 reg, u8 val)
 	return i2c_smbus_write_byte_data(client, reg, val);
 }
 
-#ifdef CONFIG_GPIOLIB
 static int adp5588_gpio_get_value(struct gpio_chip *chip, unsigned off)
 {
 	struct adp5588_kpad *kpad = gpiochip_get_data(chip);
@@ -171,9 +168,6 @@  static int adp5588_build_gpiomap(struct adp5588_kpad *kpad,
 	for (i = 0; i < pdata->cols; i++)
 		pin_used[i + GPI_PIN_COL_BASE - GPI_PIN_BASE] = true;
 
-	for (i = 0; i < kpad->gpimapsize; i++)
-		pin_used[kpad->gpimap[i].pin - GPI_PIN_BASE] = true;
-
 	for (i = 0; i < ADP5588_MAXGPIO; i++)
 		if (!pin_used[i])
 			kpad->gpiomap[n_unused++] = i;
@@ -196,11 +190,77 @@  static void adp5588_gpio_do_teardown(void *_kpad)
 		dev_warn(&kpad->client->dev, "teardown failed %d\n", error);
 }
 
+static void adp5588_irq_bus_lock(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct adp5588_kpad *kpad = gpiochip_get_data(gc);
+
+	mutex_lock(&kpad->gpio_lock);
+}
+
+static void adp5588_irq_bus_sync_unlock(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct adp5588_kpad *kpad = gpiochip_get_data(gc);
+	int i;
+
+	for (i = 0; i <= ADP5588_BANK(ADP5588_MAXGPIO); i++) {
+		if (kpad->int_en[i] ^ kpad->irq_mask[i]) {
+			kpad->int_en[i] = kpad->irq_mask[i];
+			adp5588_write(kpad->client, GPI_EM1 + i, kpad->int_en[i]);
+		}
+	}
+
+	mutex_unlock(&kpad->gpio_lock);
+}
+
+static void adp5588_irq_mask(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct adp5588_kpad *kpad = gpiochip_get_data(gc);
+	unsigned long real_irq = kpad->gpiomap[irqd_to_hwirq(d)];
+
+	kpad->irq_mask[ADP5588_BANK(real_irq)] &= ~ADP5588_BIT(real_irq);
+	gpiochip_disable_irq(gc, irqd_to_hwirq(d));
+}
+
+static void adp5588_irq_unmask(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct adp5588_kpad *kpad = gpiochip_get_data(gc);
+	unsigned long real_irq = kpad->gpiomap[irqd_to_hwirq(d)];
+
+	gpiochip_enable_irq(gc, irqd_to_hwirq(d));
+	kpad->irq_mask[ADP5588_BANK(real_irq)] |= ADP5588_BIT(real_irq);
+}
+
+static int adp5588_irq_set_type(struct irq_data *d, unsigned int type)
+{
+	if (!(type & IRQ_TYPE_EDGE_BOTH))
+		return -EINVAL;
+
+	irq_set_handler_locked(d, handle_edge_irq);
+
+	return 0;
+}
+
+static const struct irq_chip adp5588_irq_chip = {
+	.name = "adp5588",
+	.irq_mask = adp5588_irq_mask,
+	.irq_unmask = adp5588_irq_unmask,
+	.irq_bus_lock = adp5588_irq_bus_lock,
+	.irq_bus_sync_unlock = adp5588_irq_bus_sync_unlock,
+	.irq_set_type = adp5588_irq_set_type,
+	.flags = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_IMMUTABLE,
+	GPIOCHIP_IRQ_RESOURCE_HELPERS,
+};
+
 static int adp5588_gpio_add(struct adp5588_kpad *kpad)
 {
 	struct device *dev = &kpad->client->dev;
 	const struct adp5588_kpad_platform_data *pdata = dev_get_platdata(dev);
 	const struct adp5588_gpio_platform_data *gpio_data = pdata->gpio_data;
+	struct gpio_irq_chip *girq;
 	int i, error;
 
 	if (!gpio_data)
@@ -212,6 +272,7 @@  static int adp5588_gpio_add(struct adp5588_kpad *kpad)
 		return 0;
 	}
 
+	kpad->gc.parent = &kpad->client->dev;
 	kpad->gc.direction_input = adp5588_gpio_direction_input;
 	kpad->gc.direction_output = adp5588_gpio_direction_output;
 	kpad->gc.get = adp5588_gpio_get_value;
@@ -223,6 +284,11 @@  static int adp5588_gpio_add(struct adp5588_kpad *kpad)
 	kpad->gc.owner = THIS_MODULE;
 	kpad->gc.names = gpio_data->names;
 
+	girq = &kpad->gc.irq;
+	gpio_irq_chip_set_chip(girq, &adp5588_irq_chip);
+	girq->handler = handle_bad_irq;
+	girq->threaded = true;
+
 	mutex_init(&kpad->gpio_lock);
 
 	error = devm_gpiochip_add_data(dev, &kpad->gc, kpad);
@@ -255,35 +321,72 @@  static int adp5588_gpio_add(struct adp5588_kpad *kpad)
 	return 0;
 }
 
-#else
-static inline int adp5588_gpio_add(struct adp5588_kpad *kpad)
+static int adp5588_gpiomap_get_hwirq(struct device *dev, const u8 *map,
+				     unsigned int gpio, unsigned int ngpios)
 {
-	return 0;
+	unsigned int hwirq;
+
+	for (hwirq = 0; hwirq < ngpios; hwirq++)
+		if (map[hwirq] == gpio)
+			return hwirq;
+
+	/* should never happen */
+	dev_warn_ratelimited(dev, "could not find the hwirq for gpio(%u)\n", gpio);
+
+	return INVALID_HWIRQ;
+}
+
+static void adp5588_gpio_irq_handle(struct adp5588_kpad *kpad, int key_val,
+				    int key_press)
+{
+	unsigned int irq, gpio = key_val - GPI_PIN_BASE, irq_type;
+	struct i2c_client *client = kpad->client;
+	struct irq_data *irqd;
+	int hwirq;
+
+	hwirq = adp5588_gpiomap_get_hwirq(&client->dev, kpad->gpiomap,
+					  gpio, kpad->gc.ngpio);
+	if (hwirq == INVALID_HWIRQ) {
+		dev_err(&client->dev, "Could not get hwirq for key(%u)\n", key_val);
+		return;
+	}
+
+	irq = irq_find_mapping(kpad->gc.irq.domain, hwirq);
+	if (!irq)
+		return;
+
+	irqd = irq_get_irq_data(irq);
+	if (!irqd) {
+		dev_err(&client->dev, "Could not get irq(%u) data\n", irq);
+		return;
+	}
+
+	irq_type = irqd_get_trigger_type(irqd);
+
+	/*
+	 * Default is active low which means key_press is asserted on
+	 * the falling edge.
+	 */
+	if ((irq_type & IRQ_TYPE_EDGE_RISING && !key_press) ||
+	    (irq_type & IRQ_TYPE_EDGE_FALLING && key_press))
+		handle_nested_irq(irq);
 }
-#endif
 
 static void adp5588_report_events(struct adp5588_kpad *kpad, int ev_cnt)
 {
-	int i, j;
+	int i;
 
 	for (i = 0; i < ev_cnt; i++) {
 		int key = adp5588_read(kpad->client, Key_EVENTA + i);
 		int key_val = key & KEY_EV_MASK;
+		int key_press = key & KEY_EV_PRESSED;
 
-		if (key_val >= GPI_PIN_BASE && key_val <= GPI_PIN_END) {
-			for (j = 0; j < kpad->gpimapsize; j++) {
-				if (key_val == kpad->gpimap[j].pin) {
-					input_report_switch(kpad->input,
-							kpad->gpimap[j].sw_evt,
-							key & KEY_EV_PRESSED);
-					break;
-				}
-			}
-		} else {
+		if (key_val >= GPI_PIN_BASE && key_val <= GPI_PIN_END)
+			/* gpio line used as IRQ source */
+			adp5588_gpio_irq_handle(kpad, key_val, key_press);
+		else
 			input_report_key(kpad->input,
-					 kpad->keycode[key_val - 1],
-					 key & KEY_EV_PRESSED);
-		}
+					 kpad->keycode[key_val - 1], key_press);
 	}
 }
 
@@ -341,7 +444,6 @@  static int adp5588_setup(struct i2c_client *client)
 			dev_get_platdata(&client->dev);
 	const struct adp5588_gpio_platform_data *gpio_data = pdata->gpio_data;
 	int i, ret;
-	unsigned char evt_mode1 = 0, evt_mode2 = 0, evt_mode3 = 0;
 
 	ret = adp5588_write(client, KP_GPIO1, KP_SEL(pdata->rows));
 	ret |= adp5588_write(client, KP_GPIO2, KP_SEL(pdata->cols) & 0xFF);
@@ -356,23 +458,6 @@  static int adp5588_setup(struct i2c_client *client)
 	for (i = 0; i < KEYP_MAX_EVENT; i++)
 		ret |= adp5588_read(client, Key_EVENTA);
 
-	for (i = 0; i < pdata->gpimapsize; i++) {
-		unsigned short pin = pdata->gpimap[i].pin;
-
-		if (pin <= GPI_PIN_ROW_END) {
-			evt_mode1 |= (1 << (pin - GPI_PIN_ROW_BASE));
-		} else {
-			evt_mode2 |= ((1 << (pin - GPI_PIN_COL_BASE)) & 0xFF);
-			evt_mode3 |= ((1 << (pin - GPI_PIN_COL_BASE)) >> 8);
-		}
-	}
-
-	if (pdata->gpimapsize) {
-		ret |= adp5588_write(client, GPI_EM1, evt_mode1);
-		ret |= adp5588_write(client, GPI_EM2, evt_mode2);
-		ret |= adp5588_write(client, GPI_EM3, evt_mode3);
-	}
-
 	if (gpio_data) {
 		for (i = 0; i <= ADP5588_BANK(ADP5588_MAXGPIO); i++) {
 			int pull_mask = gpio_data->pullup_dis_mask;
@@ -399,44 +484,6 @@  static int adp5588_setup(struct i2c_client *client)
 	return 0;
 }
 
-static void adp5588_report_switch_state(struct adp5588_kpad *kpad)
-{
-	int gpi_stat1 = adp5588_read(kpad->client, GPIO_DAT_STAT1);
-	int gpi_stat2 = adp5588_read(kpad->client, GPIO_DAT_STAT2);
-	int gpi_stat3 = adp5588_read(kpad->client, GPIO_DAT_STAT3);
-	int gpi_stat_tmp, pin_loc;
-	int i;
-
-	for (i = 0; i < kpad->gpimapsize; i++) {
-		unsigned short pin = kpad->gpimap[i].pin;
-
-		if (pin <= GPI_PIN_ROW_END) {
-			gpi_stat_tmp = gpi_stat1;
-			pin_loc = pin - GPI_PIN_ROW_BASE;
-		} else if ((pin - GPI_PIN_COL_BASE) < 8) {
-			gpi_stat_tmp = gpi_stat2;
-			pin_loc = pin - GPI_PIN_COL_BASE;
-		} else {
-			gpi_stat_tmp = gpi_stat3;
-			pin_loc = pin - GPI_PIN_COL_BASE - 8;
-		}
-
-		if (gpi_stat_tmp < 0) {
-			dev_err(&kpad->client->dev,
-				"Can't read GPIO_DAT_STAT switch %d default to OFF\n",
-				pin);
-			gpi_stat_tmp = 0;
-		}
-
-		input_report_switch(kpad->input,
-				    kpad->gpimap[i].sw_evt,
-				    !(gpi_stat_tmp & (1 << pin_loc)));
-	}
-
-	input_sync(kpad->input);
-}
-
-
 static int adp5588_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -469,37 +516,6 @@  static int adp5588_probe(struct i2c_client *client,
 		return -EINVAL;
 	}
 
-	if (!pdata->gpimap && pdata->gpimapsize) {
-		dev_err(&client->dev, "invalid gpimap from pdata\n");
-		return -EINVAL;
-	}
-
-	if (pdata->gpimapsize > ADP5588_GPIMAPSIZE_MAX) {
-		dev_err(&client->dev, "invalid gpimapsize\n");
-		return -EINVAL;
-	}
-
-	for (i = 0; i < pdata->gpimapsize; i++) {
-		unsigned short pin = pdata->gpimap[i].pin;
-
-		if (pin < GPI_PIN_BASE || pin > GPI_PIN_END) {
-			dev_err(&client->dev, "invalid gpi pin data\n");
-			return -EINVAL;
-		}
-
-		if (pin <= GPI_PIN_ROW_END) {
-			if (pin - GPI_PIN_ROW_BASE + 1 <= pdata->rows) {
-				dev_err(&client->dev, "invalid gpi row data\n");
-				return -EINVAL;
-			}
-		} else {
-			if (pin - GPI_PIN_COL_BASE + 1 <= pdata->cols) {
-				dev_err(&client->dev, "invalid gpi col data\n");
-				return -EINVAL;
-			}
-		}
-	}
-
 	if (!client->irq) {
 		dev_err(&client->dev, "no IRQ?\n");
 		return -EINVAL;
@@ -541,9 +557,6 @@  static int adp5588_probe(struct i2c_client *client,
 	memcpy(kpad->keycode, pdata->keymap,
 		pdata->keymapsize * input->keycodesize);
 
-	kpad->gpimap = pdata->gpimap;
-	kpad->gpimapsize = pdata->gpimapsize;
-
 	/* setup input device */
 	__set_bit(EV_KEY, input->evbit);
 
@@ -555,11 +568,6 @@  static int adp5588_probe(struct i2c_client *client,
 			__set_bit(kpad->keycode[i], input->keybit);
 	__clear_bit(KEY_RESERVED, input->keybit);
 
-	if (kpad->gpimapsize)
-		__set_bit(EV_SW, input->evbit);
-	for (i = 0; i < kpad->gpimapsize; i++)
-		__set_bit(kpad->gpimap[i].sw_evt, input->swbit);
-
 	error = input_register_device(input);
 	if (error) {
 		dev_err(&client->dev, "unable to register input device: %d\n",
@@ -567,6 +575,14 @@  static int adp5588_probe(struct i2c_client *client,
 		return error;
 	}
 
+	error = adp5588_setup(client);
+	if (error)
+		return error;
+
+	error = adp5588_gpio_add(kpad);
+	if (error)
+		return error;
+
 	error = devm_request_threaded_irq(&client->dev, client->irq,
 					  adp5588_hard_irq, adp5588_thread_irq,
 					  IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
@@ -577,17 +593,6 @@  static int adp5588_probe(struct i2c_client *client,
 		return error;
 	}
 
-	error = adp5588_setup(client);
-	if (error)
-		return error;
-
-	if (kpad->gpimapsize)
-		adp5588_report_switch_state(kpad);
-
-	error = adp5588_gpio_add(kpad);
-	if (error)
-		return error;
-
 	dev_info(&client->dev, "Rev.%d keypad, irq %d\n", revid, client->irq);
 	return 0;
 }
diff --git a/include/linux/platform_data/adp5588.h b/include/linux/platform_data/adp5588.h
index 6d3f7d911a92..82170ec8c266 100644
--- a/include/linux/platform_data/adp5588.h
+++ b/include/linux/platform_data/adp5588.h
@@ -147,8 +147,6 @@  struct adp5588_kpad_platform_data {
 	unsigned en_keylock:1;		/* Enable Key Lock feature */
 	unsigned short unlock_key1;	/* Unlock Key 1 */
 	unsigned short unlock_key2;	/* Unlock Key 2 */
-	const struct adp5588_gpi_map *gpimap;
-	unsigned short gpimapsize;
 	const struct adp5588_gpio_platform_data *gpio_data;
 };