| Message ID | 20220708093448.42617-2-nuno.sa@analog.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | adp5588-keys refactor and fw properties support | expand |
On Fri, Jul 8, 2022 at 11:36 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. ... > u8 dat_out[3]; > u8 dir[3]; > + u8 int_en[3]; > + u8 irq_mask[3]; Wondering why these can't be converted to natural bitmaps. (See gpio-pca953x as an example). ... > +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[d->hwirq]; > + > + kpad->irq_mask[ADP5588_BANK(real_irq)] &= ~ADP5588_BIT(real_irq); > +} > + > +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[d->hwirq]; > + > + kpad->irq_mask[ADP5588_BANK(real_irq)] |= ADP5588_BIT(real_irq); > +} Please follow the suitable example from here: https://www.kernel.org/doc/html/latest/driver-api/gpio/driver.html#infrastructure-helpers-for-gpio-irqchips ... > + kpad->gc.parent = &kpad->client->dev; > + kpad->gc.of_node = kpad->client->dev.of_node; We are going to remove of_node from GPIO. Moreover the parent device and its node is a duplication, just drop the latter and GPIO library will take care of it. ... > + irq_chip->name = "adp5588"; > + irq_chip->irq_mask = adp5588_irq_mask; > + irq_chip->irq_unmask = adp5588_irq_unmask; > + irq_chip->irq_bus_lock = adp5588_irq_bus_lock; > + irq_chip->irq_bus_sync_unlock = adp5588_irq_bus_sync_unlock; > + irq_chip->irq_set_type = adp5588_irq_set_type; > + irq_chip->flags = IRQCHIP_SKIP_SET_WAKE; > + girq = &kpad->gc.irq; > + girq->chip = irq_chip; > + girq->handler = handle_simple_irq; By default it should be handle_bad_irq() and locked in the ->irq_set_type(). > + girq->threaded = true; See documentation above. ... > +static int adp5588_gpiomap_get_hwirq(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 */ Then why it's here? > + WARN_ON_ONCE(hwirq == ngpios); > + > + return -ENOENT; > +} I don't know this code, can you summarize why this additional mapping is needed? ... > +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, hwirq; > + struct i2c_client *client = kpad->client; > + struct irq_data *desc; > + > + hwirq = adp5588_gpiomap_get_hwirq(kpad->gpiomap, gpio, kpad->gc.ngpio); > + if (hwirq < 0) { > + 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 <= 0) > + return; > + > + desc = irq_get_irq_data(irq); > + if (!desc) { > + dev_err(&client->dev, "Could not get irq(%u) data\n", irq); > + return; > + } > + > + irq_type = irqd_get_trigger_type(desc); > + > + /* > + * 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)) This is dup from ->irq_set_type(). Or how this can be not like this? > + handle_nested_irq(irq); There is new helpers I believe for getting domain and handle an IRQ. Grep for the recent (one-two last cycles) changes in the GPIO drivers. > }
> -----Original Message----- > From: Andy Shevchenko <andy.shevchenko@gmail.com> > Sent: Friday, July 8, 2022 4:18 PM > To: Sa, Nuno <Nuno.Sa@analog.com> > Cc: devicetree <devicetree@vger.kernel.org>; open list:GPIO > SUBSYSTEM <linux-gpio@vger.kernel.org>; linux-input <linux- > input@vger.kernel.org>; Dmitry Torokhov > <dmitry.torokhov@gmail.com>; Bartosz Golaszewski > <brgl@bgdev.pl>; Hennerich, Michael > <Michael.Hennerich@analog.com>; Rob Herring > <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Linus Walleij > <linus.walleij@linaro.org> > Subject: Re: [PATCH 01/10] input: keyboard: adp5588-keys: support gpi > key events as 'gpio keys' > > [External] > > On Fri, Jul 8, 2022 at 11:36 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. > > ... > > > u8 dat_out[3]; > > u8 dir[3]; > > > + u8 int_en[3]; > > + u8 irq_mask[3]; > > Wondering why these can't be converted to natural bitmaps. (See > gpio-pca953x as an example). > I kind of replied to this in a previous email (to you). It naturally can, but I would rather prefer to do that in another series... I could have done it here but it would not be consistent with the rest of the driver. > ... > > > +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[d->hwirq]; > > + > > + kpad->irq_mask[ADP5588_BANK(real_irq)] &= > ~ADP5588_BIT(real_irq); > > +} > > + > > +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[d->hwirq]; > > + > > + kpad->irq_mask[ADP5588_BANK(real_irq)] |= > ADP5588_BIT(real_irq); > > +} > > Please follow the suitable example from here: > https://urldefense.com/v3/__https://www.kernel.org/doc/html/lates > t/driver-api/gpio/driver.html*infrastructure-helpers-for-gpio- > irqchips__;Iw!!A3Ni8CS0y2Y!4eMLD5XT2REpOPWl6B9IxYxEgbMfxiW87 > XJu-4KmjeVywSLrobIZqEZpcVJ0dBNbZDGPBpHXvx3xP-HzGS4jIwvu$ > > ... > > > + kpad->gc.parent = &kpad->client->dev; > > > + kpad->gc.of_node = kpad->client->dev.of_node; > > We are going to remove of_node from GPIO. Moreover the parent > device > and its node is a duplication, just drop the latter and GPIO library > will take care of it. > Well the of_node was set so that I had a proper name in the IRQ domain IIRC. Will this be handled in the GPIO lib in the future? The parent assignment was also to make things neater in /sys/kernel/debug/gpio. > ... > > > + irq_chip->name = "adp5588"; > > + irq_chip->irq_mask = adp5588_irq_mask; > > + irq_chip->irq_unmask = adp5588_irq_unmask; > > + irq_chip->irq_bus_lock = adp5588_irq_bus_lock; > > + irq_chip->irq_bus_sync_unlock = > adp5588_irq_bus_sync_unlock; > > + irq_chip->irq_set_type = adp5588_irq_set_type; > > + irq_chip->flags = IRQCHIP_SKIP_SET_WAKE; > > + girq = &kpad->gc.irq; > > + girq->chip = irq_chip; > > > + girq->handler = handle_simple_irq; > > By default it should be handle_bad_irq() and locked in the - > >irq_set_type(). > > > + girq->threaded = true; > > See documentation above. I see... I will look at Docs. In practice I don't think this matters much since this handler should never really be called (I think) as we just call handle_nested_irq(). > ... > > > +static int adp5588_gpiomap_get_hwirq(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 */ > > Then why it's here? because I do not trust the HW to always cooperate :). In theory, we can get some invalid 'gpio' from it. > > + WARN_ON_ONCE(hwirq == ngpios); > > + > > + return -ENOENT; > > +} > > I don't know this code, can you summarize why this additional mapping > is needed? > You have 18 possible pins to use as GPIOs (and hence be IRQ sources). Now, if you just want to use pins 16 and 17 that will map int hwirq 0 and 1. But what we get from the device in 'key_val - GPI_PIN_BASE' is, for example, 16 and so we need to get the hwirq which will be 0. It's pretty much the reverse of what it's being done in the GPIOs callbacks. > ... > > > +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, > hwirq; > > + struct i2c_client *client = kpad->client; > > + struct irq_data *desc; > > + > > + hwirq = adp5588_gpiomap_get_hwirq(kpad->gpiomap, gpio, > kpad->gc.ngpio); > > + if (hwirq < 0) { > > + 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 <= 0) > > + return; > > + > > + desc = irq_get_irq_data(irq); > > + if (!desc) { > > + dev_err(&client->dev, "Could not get irq(%u) data\n", irq); > > + return; > > + } > > + > > + irq_type = irqd_get_trigger_type(desc); > > + > > + /* > > + * 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)) > > This is dup from ->irq_set_type(). Or how this can be not like this? We get here if we get a key press (falling edge) or a key release (rising edge). The events are given by the device and it might be that in some cases we just want to handle/propagate key presses (not sure if it makes sense). So we need to match it with the appropriate irq_type requested. Note that this kind of controlling the IRQ from SW as there's no way from doing it in the device. That is why we don't do more than just making sure the IRQ types are valid in irq_set_type. > > + handle_nested_irq(irq); > > There is new helpers I believe for getting domain and handle an IRQ. > Grep for the recent (one-two last cycles) changes in the GPIO drivers. > Hmm, I think I saw it but somehow I though I could not use it (because of the previous calls to get the irq_type). Hmmm... - Nuno Sá
On Fri, Jul 8, 2022 at 4:55 PM Sa, Nuno <Nuno.Sa@analog.com> wrote: > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > Sent: Friday, July 8, 2022 4:18 PM > > On Fri, Jul 8, 2022 at 11:36 AM Nuno Sá <nuno.sa@analog.com> wrote: ... > > > + kpad->gc.parent = &kpad->client->dev; > > > > > + kpad->gc.of_node = kpad->client->dev.of_node; > > > > We are going to remove of_node from GPIO. Moreover the parent > > device > > and its node is a duplication, just drop the latter and GPIO library > > will take care of it. > > > > Well the of_node was set so that I had a proper name in the IRQ domain > IIRC. Will this be handled in the GPIO lib in the future? In your case it's a dup. So in _your_ case it will be handled in the future. For the rest we already have an fwnode member. > The parent assignment was also to make things neater in > /sys/kernel/debug/gpio. Sure. ... > > > + girq->handler = handle_simple_irq; > > > > By default it should be handle_bad_irq() and locked in the - > > >irq_set_type(). > > > > > + girq->threaded = true; > > > > See documentation above. > > I see... I will look at Docs. In practice I don't think this matters much > since this handler should never really be called (I think) as we just > call handle_nested_irq(). There are two different comments, one about handler, another about how to properly write IRQ chip data structure and mask()/unmask() callbacks. ... > > > + /* should never happen */ > > > > Then why it's here? > > because I do not trust the HW to always cooperate :). In theory, > we can get some invalid 'gpio' from it. > > > > + WARN_ON_ONCE(hwirq == ngpios); On some setups this can lead to panic. Why? Is this so critical error? hardware can't anymore function? ... > > I don't know this code, can you summarize why this additional mapping > > is needed? > > You have 18 possible pins to use as GPIOs (and hence be IRQ sources). Now, > if you just want to use pins 16 and 17 that will map int hwirq 0 and 1. But > what we get from the device in 'key_val - GPI_PIN_BASE' is, for example, > 16 and so we need to get the hwirq which will be 0. It's pretty much the > reverse of what it's being done in the GPIOs callbacks. Any reason why irq_valid_mask can't be used for that? ... > > > + /* > > > + * 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)) > > > > This is dup from ->irq_set_type(). Or how this can be not like this? > > We get here if we get a key press (falling edge) or a key release (rising > edge). The events are given by the device and it might be that in some > cases we just want to handle/propagate key presses > (not sure if it makes sense). So we need to match it with the > appropriate irq_type requested. Note that this kind of controlling the IRQ > from SW as there's no way from doing it in the device. That is why we don't > do more than just making sure the IRQ types are valid in irq_set_type. I see, thanks for elaboration. ... > > > + handle_nested_irq(irq); > > > > There is new helpers I believe for getting domain and handle an IRQ. > > Grep for the recent (one-two last cycles) changes in the GPIO drivers. > > > > Hmm, I think I saw it but somehow I though I could not use it (because > of the previous calls to get the irq_type). Hmmm... Maybe you can double check?
> -----Original Message----- > From: Andy Shevchenko <andy.shevchenko@gmail.com> > Sent: Friday, July 8, 2022 5:05 PM > To: Sa, Nuno <Nuno.Sa@analog.com> > Cc: devicetree <devicetree@vger.kernel.org>; open list:GPIO > SUBSYSTEM <linux-gpio@vger.kernel.org>; linux-input <linux- > input@vger.kernel.org>; Dmitry Torokhov > <dmitry.torokhov@gmail.com>; Bartosz Golaszewski > <brgl@bgdev.pl>; Hennerich, Michael > <Michael.Hennerich@analog.com>; Rob Herring > <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Linus Walleij > <linus.walleij@linaro.org> > Subject: Re: [PATCH 01/10] input: keyboard: adp5588-keys: support gpi > key events as 'gpio keys' > > [External] > > On Fri, Jul 8, 2022 at 4:55 PM Sa, Nuno <Nuno.Sa@analog.com> wrote: > > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > > Sent: Friday, July 8, 2022 4:18 PM > > > On Fri, Jul 8, 2022 at 11:36 AM Nuno Sá <nuno.sa@analog.com> > wrote: > > ... > > > > > + kpad->gc.parent = &kpad->client->dev; > > > > > > > + kpad->gc.of_node = kpad->client->dev.of_node; > > > > > > We are going to remove of_node from GPIO. Moreover the parent > > > device > > > and its node is a duplication, just drop the latter and GPIO library > > > will take care of it. > > > > > > > Well the of_node was set so that I had a proper name in the IRQ > domain > > IIRC. Will this be handled in the GPIO lib in the future? > > In your case it's a dup. So in _your_ case it will be handled in the > future. For the rest we already have an fwnode member. ok, I will drop the assignment... > > The parent assignment was also to make things neater in > > /sys/kernel/debug/gpio. > > Sure. > > ... > > > > > + girq->handler = handle_simple_irq; > > > > > > By default it should be handle_bad_irq() and locked in the - > > > >irq_set_type(). > > > > > > > + girq->threaded = true; > > > > > > See documentation above. > > > > I see... I will look at Docs. In practice I don't think this matters much > > since this handler should never really be called (I think) as we just > > call handle_nested_irq(). > > There are two different comments, one about handler, another about > how > to properly write IRQ chip data structure and mask()/unmask() > callbacks. > > ... > > > > > + /* should never happen */ > > > > > > Then why it's here? > > > > because I do not trust the HW to always cooperate :). In theory, > > we can get some invalid 'gpio' from it. > > > > > > + WARN_ON_ONCE(hwirq == ngpios); > > On some setups this can lead to panic. Why? Is this so critical error? Ahh, you're right. Forgot that in some cases WARN can actually panic. > hardware can't anymore function? If we get in here, the device is probably in a very bad state but that does not mean that the system is... I will just move to dev_warn(). Thanks for the remainder! > ... > > > > I don't know this code, can you summarize why this additional > mapping > > > is needed? > > > > You have 18 possible pins to use as GPIOs (and hence be IRQ > sources). Now, > > if you just want to use pins 16 and 17 that will map int hwirq 0 and 1. > But > > what we get from the device in 'key_val - GPI_PIN_BASE' is, for > example, > > 16 and so we need to get the hwirq which will be 0. It's pretty much > the > > reverse of what it's being done in the GPIOs callbacks. > > Any reason why irq_valid_mask can't be used for that? I will have to look at irq_valid_mask. > ... > > > > > + /* > > > > + * 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)) > > > > > > This is dup from ->irq_set_type(). Or how this can be not like this? > > > > We get here if we get a key press (falling edge) or a key release > (rising > > edge). The events are given by the device and it might be that in > some > > cases we just want to handle/propagate key presses > > (not sure if it makes sense). So we need to match it with the > > appropriate irq_type requested. Note that this kind of controlling the > IRQ > > from SW as there's no way from doing it in the device. That is why we > don't > > do more than just making sure the IRQ types are valid in > irq_set_type. > > I see, thanks for elaboration. > > ... > > > > > + handle_nested_irq(irq); > > > > > > There is new helpers I believe for getting domain and handle an > IRQ. > > > Grep for the recent (one-two last cycles) changes in the GPIO > drivers. > > > > > > > Hmm, I think I saw it but somehow I though I could not use it > (because > > of the previous calls to get the irq_type). Hmmm... > > Maybe you can double check? Sure, I think the helper can be used... - Nuno Sá
Hi "Nuno,
I love your patch! Yet something to improve:
[auto build test ERROR on dtor-input/next]
[also build test ERROR on next-20220708]
[cannot apply to brgl/gpio/for-next hid/for-next linus/master v5.19-rc5]
[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/20220708-173730
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/20220709/202207091223.nBzeL6dk-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/64267ff775fd4b945fb916a10187be1c15faa165
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/20220708-173730
git checkout 64267ff775fd4b945fb916a10187be1c15faa165
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/input/keyboard/adp5588-keys.c: In function 'adp5588_gpio_add':
>> drivers/input/keyboard/adp5588-keys.c:263:18: error: 'struct gpio_chip' has no member named 'of_node'; did you mean 'fwnode'?
263 | kpad->gc.of_node = kpad->client->dev.of_node;
| ^~~~~~~
| fwnode
vim +263 drivers/input/keyboard/adp5588-keys.c
243
244 static int adp5588_gpio_add(struct adp5588_kpad *kpad)
245 {
246 struct irq_chip *irq_chip = &kpad->irq_chip;
247 struct device *dev = &kpad->client->dev;
248 const struct adp5588_kpad_platform_data *pdata = dev_get_platdata(dev);
249 const struct adp5588_gpio_platform_data *gpio_data = pdata->gpio_data;
250 struct gpio_irq_chip *girq;
251 int i, error;
252
253 if (!gpio_data)
254 return 0;
255
256 kpad->gc.ngpio = adp5588_build_gpiomap(kpad, pdata);
257 if (kpad->gc.ngpio == 0) {
258 dev_info(dev, "No unused gpios left to export\n");
259 return 0;
260 }
261
262 kpad->gc.parent = &kpad->client->dev;
> 263 kpad->gc.of_node = kpad->client->dev.of_node;
264 kpad->gc.direction_input = adp5588_gpio_direction_input;
265 kpad->gc.direction_output = adp5588_gpio_direction_output;
266 kpad->gc.get = adp5588_gpio_get_value;
267 kpad->gc.set = adp5588_gpio_set_value;
268 kpad->gc.can_sleep = 1;
269
270 kpad->gc.base = gpio_data->gpio_start;
271 kpad->gc.label = kpad->client->name;
272 kpad->gc.owner = THIS_MODULE;
273 kpad->gc.names = gpio_data->names;
274
275 irq_chip->name = "adp5588";
276 irq_chip->irq_mask = adp5588_irq_mask;
277 irq_chip->irq_unmask = adp5588_irq_unmask;
278 irq_chip->irq_bus_lock = adp5588_irq_bus_lock;
279 irq_chip->irq_bus_sync_unlock = adp5588_irq_bus_sync_unlock;
280 irq_chip->irq_set_type = adp5588_irq_set_type;
281 irq_chip->flags = IRQCHIP_SKIP_SET_WAKE;
282 girq = &kpad->gc.irq;
283 girq->chip = irq_chip;
284 girq->handler = handle_simple_irq;
285 girq->threaded = true;
286
287 mutex_init(&kpad->gpio_lock);
288
289 error = devm_gpiochip_add_data(dev, &kpad->gc, kpad);
290 if (error) {
291 dev_err(dev, "gpiochip_add failed: %d\n", error);
292 return error;
293 }
294
295 for (i = 0; i <= ADP5588_BANK(ADP5588_MAXGPIO); i++) {
296 kpad->dat_out[i] = adp5588_read(kpad->client,
297 GPIO_DAT_OUT1 + i);
298 kpad->dir[i] = adp5588_read(kpad->client, GPIO_DIR1 + i);
299 }
300
301 if (gpio_data->setup) {
302 error = gpio_data->setup(kpad->client,
303 kpad->gc.base, kpad->gc.ngpio,
304 gpio_data->context);
305 if (error)
306 dev_warn(dev, "setup failed: %d\n", error);
307 }
308
309 if (gpio_data->teardown) {
310 error = devm_add_action(dev, adp5588_gpio_do_teardown, kpad);
311 if (error)
312 dev_warn(dev, "failed to schedule teardown: %d\n",
313 error);
314 }
315
316 return 0;
317 }
318
On Sat, Jul 9, 2022 at 6:22 AM kernel test robot <lkp@intel.com> wrote: > > Hi "Nuno, > > I love your patch! Yet something to improve: > > [auto build test ERROR on dtor-input/next] > [also build test ERROR on next-20220708] > [cannot apply to brgl/gpio/for-next hid/for-next linus/master v5.19-rc5] > [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/20220708-173730 > 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/20220709/202207091223.nBzeL6dk-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/64267ff775fd4b945fb916a10187be1c15faa165 > 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/20220708-173730 > git checkout 64267ff775fd4b945fb916a10187be1c15faa165 > # save the config file > mkdir build_dir && cp config build_dir/.config > make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash > > If you fix the issue, kindly add following tag where applicable > Reported-by: kernel test robot <lkp@intel.com> > > All errors (new ones prefixed by >>): > > drivers/input/keyboard/adp5588-keys.c: In function 'adp5588_gpio_add': > >> drivers/input/keyboard/adp5588-keys.c:263:18: error: 'struct gpio_chip' has no member named 'of_node'; did you mean 'fwnode'? > 263 | kpad->gc.of_node = kpad->client->dev.of_node; > | ^~~~~~~ > | fwnode Yes, exactly the point why of_node is bad to have. In legacy code like this you need to guard access to it with #ifdef CONFIG_OF_GPIO IIRC.
On Fri, 2022-07-08 at 15:24 +0000, Sa, Nuno wrote: > > > > -----Original Message----- > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > Sent: Friday, July 8, 2022 5:05 PM > > To: Sa, Nuno <Nuno.Sa@analog.com> > > Cc: devicetree <devicetree@vger.kernel.org>; open list:GPIO > > SUBSYSTEM <linux-gpio@vger.kernel.org>; linux-input <linux- > > input@vger.kernel.org>; Dmitry Torokhov > > <dmitry.torokhov@gmail.com>; Bartosz Golaszewski > > <brgl@bgdev.pl>; Hennerich, Michael > > <Michael.Hennerich@analog.com>; Rob Herring > > <robh+dt@kernel.org>; Krzysztof Kozlowski > > <krzysztof.kozlowski+dt@linaro.org>; Linus Walleij > > <linus.walleij@linaro.org> > > Subject: Re: [PATCH 01/10] input: keyboard: adp5588-keys: support > > gpi > > key events as 'gpio keys' > > > > [External] > > > > On Fri, Jul 8, 2022 at 4:55 PM Sa, Nuno <Nuno.Sa@analog.com> wrote: > > > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > > > Sent: Friday, July 8, 2022 4:18 PM > > > > On Fri, Jul 8, 2022 at 11:36 AM Nuno Sá <nuno.sa@analog.com> > > wrote: > > > > ... > > > > > > > + kpad->gc.parent = &kpad->client->dev; > > > > > > > > > + kpad->gc.of_node = kpad->client->dev.of_node; > > > > > > > > We are going to remove of_node from GPIO. Moreover the parent > > > > device > > > > and its node is a duplication, just drop the latter and GPIO > > > > library > > > > will take care of it. > > > > > > > > > > Well the of_node was set so that I had a proper name in the IRQ > > domain > > > IIRC. Will this be handled in the GPIO lib in the future? > > > > In your case it's a dup. So in _your_ case it will be handled in > > the > > future. For the rest we already have an fwnode member. > > ok, I will drop the assignment... > > > > The parent assignment was also to make things neater in > > > /sys/kernel/debug/gpio. > > > > Sure. > > > > ... > > > > > > > + girq->handler = handle_simple_irq; > > > > > > > > By default it should be handle_bad_irq() and locked in the - > > > > > irq_set_type(). > > > > > > > > > + girq->threaded = true; > > > > > > > > See documentation above. > > > > > > I see... I will look at Docs. In practice I don't think this > > > matters much > > > since this handler should never really be called (I think) as we > > > just > > > call handle_nested_irq(). > > > > There are two different comments, one about handler, another about > > how > > to properly write IRQ chip data structure and mask()/unmask() > > callbacks. > > So I think I have most of stuff understood for v2. About the handler, I don't think we really need to set 'handle_edge_irq()' in 'irq_set_type()' as this is a nested threaded interrupt and so, the desc->handle_irq() should never be called (I think, not 100% if there's any strange case where it might). That said, if you still think that I should do it (for correctness), fine by me. > > ... > > > > > > > + /* should never happen */ > > > > > > > > Then why it's here? > > > > > > because I do not trust the HW to always cooperate :). In theory, > > > we can get some invalid 'gpio' from it. > > > > > > > > + WARN_ON_ONCE(hwirq == ngpios); > > > > On some setups this can lead to panic. Why? Is this so critical > > error? > > Ahh, you're right. Forgot that in some cases WARN can actually panic. > > > hardware can't anymore function? > > If we get in here, the device is probably in a very bad state but > that > does not mean that the system is... > > I will just move to dev_warn(). Thanks for the remainder! > > > ... > > > > > > I don't know this code, can you summarize why this additional > > mapping > > > > is needed? > > > > > > You have 18 possible pins to use as GPIOs (and hence be IRQ > > sources). Now, > > > if you just want to use pins 16 and 17 that will map int hwirq 0 > > > and 1. > > But > > > what we get from the device in 'key_val - GPI_PIN_BASE' is, for > > example, > > > 16 and so we need to get the hwirq which will be 0. It's pretty > > > much > > the > > > reverse of what it's being done in the GPIOs callbacks. > > > > Any reason why irq_valid_mask can't be used for that? > > I will have to look at irq_valid_mask. > > > ... > > > > > > > + /* > > > > > + * 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)) > > > > > > > > This is dup from ->irq_set_type(). Or how this can be not like > > > > this? > > > > > > We get here if we get a key press (falling edge) or a key release > > (rising > > > edge). The events are given by the device and it might be that in > > some > > > cases we just want to handle/propagate key presses > > > (not sure if it makes sense). So we need to match it with the > > > appropriate irq_type requested. Note that this kind of > > > controlling the > > IRQ > > > from SW as there's no way from doing it in the device. That is > > > why we > > don't > > > do more than just making sure the IRQ types are valid in > > irq_set_type. > > > > I see, thanks for elaboration. > > > > ... > > > > > > > + handle_nested_irq(irq); > > > > > > > > There is new helpers I believe for getting domain and handle an > > IRQ. > > > > Grep for the recent (one-two last cycles) changes in the GPIO > > drivers. > > > > > > > > > > Hmm, I think I saw it but somehow I though I could not use it > > (because > > > of the previous calls to get the irq_type). Hmmm... > > > > Maybe you can double check? > > Sure, I think the helper can be used... > So I did looked and I think you are thinking about 'generic_handle_domain_irq()'. For nested threaded I could not find a similar one (maybe a new helper to be added). - Nuno Sá
Hi "Nuno,
I love your patch! Perhaps something to improve:
[auto build test WARNING on dtor-input/next]
[also build test WARNING on next-20220711]
[cannot apply to brgl/gpio/for-next hid/for-next linus/master v5.19-rc6]
[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/20220708-173730
base: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
config: i386-randconfig-m021 (https://download.01.org/0day-ci/archive/20220712/202207121357.JpS5DGdP-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
smatch warnings:
drivers/input/keyboard/adp5588-keys.c:342 adp5588_gpio_irq_handle() warn: unsigned 'hwirq' is never less than zero.
vim +/hwirq +342 drivers/input/keyboard/adp5588-keys.c
333
334 static void adp5588_gpio_irq_handle(struct adp5588_kpad *kpad, int key_val,
335 int key_press)
336 {
337 unsigned int irq, gpio = key_val - GPI_PIN_BASE, irq_type, hwirq;
338 struct i2c_client *client = kpad->client;
339 struct irq_data *desc;
340
341 hwirq = adp5588_gpiomap_get_hwirq(kpad->gpiomap, gpio, kpad->gc.ngpio);
> 342 if (hwirq < 0) {
343 dev_err(&client->dev, "Could not get hwirq for key(%u)\n", key_val);
344 return;
345 }
346
347 irq = irq_find_mapping(kpad->gc.irq.domain, hwirq);
348 if (irq <= 0)
349 return;
350
351 desc = irq_get_irq_data(irq);
352 if (!desc) {
353 dev_err(&client->dev, "Could not get irq(%u) data\n", irq);
354 return;
355 }
356
357 irq_type = irqd_get_trigger_type(desc);
358
359 /*
360 * Default is active low which means key_press is asserted on
361 * the falling edge.
362 */
363 if ((irq_type & IRQ_TYPE_EDGE_RISING && !key_press) ||
364 (irq_type & IRQ_TYPE_EDGE_FALLING && key_press))
365 handle_nested_irq(irq);
366 }
367
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..dad8862f6c76 100644 --- a/drivers/input/keyboard/adp5588-keys.c +++ b/drivers/input/keyboard/adp5588-keys.c @@ -46,15 +46,14 @@ 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 irq_chip irq_chip; 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 +71,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 +169,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 +191,63 @@ 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[d->hwirq]; + + kpad->irq_mask[ADP5588_BANK(real_irq)] &= ~ADP5588_BIT(real_irq); +} + +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[d->hwirq]; + + 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; + + return 0; +} + static int adp5588_gpio_add(struct adp5588_kpad *kpad) { + struct irq_chip *irq_chip = &kpad->irq_chip; 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 +259,8 @@ static int adp5588_gpio_add(struct adp5588_kpad *kpad) return 0; } + kpad->gc.parent = &kpad->client->dev; + kpad->gc.of_node = kpad->client->dev.of_node; 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 +272,18 @@ static int adp5588_gpio_add(struct adp5588_kpad *kpad) kpad->gc.owner = THIS_MODULE; kpad->gc.names = gpio_data->names; + irq_chip->name = "adp5588"; + irq_chip->irq_mask = adp5588_irq_mask; + irq_chip->irq_unmask = adp5588_irq_unmask; + irq_chip->irq_bus_lock = adp5588_irq_bus_lock; + irq_chip->irq_bus_sync_unlock = adp5588_irq_bus_sync_unlock; + irq_chip->irq_set_type = adp5588_irq_set_type; + irq_chip->flags = IRQCHIP_SKIP_SET_WAKE; + girq = &kpad->gc.irq; + girq->chip = irq_chip; + girq->handler = handle_simple_irq; + girq->threaded = true; + mutex_init(&kpad->gpio_lock); error = devm_gpiochip_add_data(dev, &kpad->gc, kpad); @@ -255,35 +316,70 @@ 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(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 */ + WARN_ON_ONCE(hwirq == ngpios); + + return -ENOENT; +} + +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, hwirq; + struct i2c_client *client = kpad->client; + struct irq_data *desc; + + hwirq = adp5588_gpiomap_get_hwirq(kpad->gpiomap, gpio, kpad->gc.ngpio); + if (hwirq < 0) { + 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 <= 0) + return; + + desc = irq_get_irq_data(irq); + if (!desc) { + dev_err(&client->dev, "Could not get irq(%u) data\n", irq); + return; + } + + irq_type = irqd_get_trigger_type(desc); + + /* + * 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 +437,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 +451,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 +477,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 +509,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 +550,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 +561,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 +568,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 +586,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; };
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 | 262 +++++++++++++------------- include/linux/platform_data/adp5588.h | 2 - 3 files changed, 132 insertions(+), 134 deletions(-)