From patchwork Sat Mar 10 18:15:09 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dmitry Torokhov X-Patchwork-Id: 10273799 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 99B52601A0 for ; Sat, 10 Mar 2018 18:15:17 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 6ADA9296E5 for ; Sat, 10 Mar 2018 18:15:17 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 49F9D2974F; Sat, 10 Mar 2018 18:15:17 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=2.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 09FA4296E5 for ; Sat, 10 Mar 2018 18:15:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751212AbeCJSPO (ORCPT ); Sat, 10 Mar 2018 13:15:14 -0500 Received: from mail-io0-f193.google.com ([209.85.223.193]:45203 "EHLO mail-io0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751199AbeCJSPN (ORCPT ); Sat, 10 Mar 2018 13:15:13 -0500 Received: by mail-io0-f193.google.com with SMTP id m22so6975248iob.12; Sat, 10 Mar 2018 10:15:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=8XPkr+GA6YSTN3L3OMmIDgswhQepTf0svmuV0noq714=; b=jl2bmM9iudd7OoAHyAOZcVJRokoV4NidvRwnvjiQn3N+eycO/XKhGNTyJaXwgvRd+2 LLQVMgxZJ5sPgX5ltM2nP/iXMO8PRHNvcmQAtq5QilC6jWBNuwNsgOqSeKvy66sGe7YV t6ApOOuk4kGxg5CKNIX7+E6QyfQJbgcwJ19Pfxi68pCch4DAaw7d1KywSpK6m79YIZip SIRCSbcJid0roDkQe9bO0T9Rp9mMGShIxYbucaTCHUxV/yuVpxnflQvqmK+31KFhlmNS 1pWiP7bbhmgrALdVboblz+q6z4xRP+BRK0RdalfRPwDNcXqk+oJQtFQlzkDGtJJx8oBj R64Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=8XPkr+GA6YSTN3L3OMmIDgswhQepTf0svmuV0noq714=; b=q+8B+jJUYYWa3GzUj4dItN+2JVYdzN3dEfqdqoI4mD9O8cFoZb9c3nQidN0XhwN0WC 81GTwt5AOIakAp+8ZvUi5EbqMG7BZ08PPhztRpxdVX0Ow3uWAoHncWMhSTasSDHWqiM5 54zCsvl51hZfujzshMaP68zef7yGwo78xLsOoz3boYtGzubwSSSD6YfQEbN48roZxVfM J1Z5bdUQX59tUbPjivuj6pAJrAjrRbpEjqOdksXSmTSM20crRFaSoOfiDyVrQqJ0fyjw LbelT99bOoGbRyCEGw4YW2HgAv/fxEYVi7mgxrfa6Yv9U5bHEv2p3J98aSBGOUq8EB+Y SKMA== X-Gm-Message-State: AElRT7GDnMk51fPubpw73d0rdMsPErkSsWwTl1UJ+NFXy+SUjV72gtt6 lS1EUusdFQpTg77iw4bi7qs= X-Google-Smtp-Source: AG47ELtEtJUsCfhvPuUCbq+jyaoVuv0+wl6VG3tbqjVdKaiXFxTgbzhDaxUZJCgpZN0Ybe7cyAP3KA== X-Received: by 10.107.165.146 with SMTP id o140mr2938674ioe.52.1520705712191; Sat, 10 Mar 2018 10:15:12 -0800 (PST) Received: from dtor-ws ([2620:0:1000:1511:8de6:27a8:ed13:2ef5]) by smtp.gmail.com with ESMTPSA id t64sm2553706ioe.58.2018.03.10.10.15.10 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sat, 10 Mar 2018 10:15:11 -0800 (PST) Date: Sat, 10 Mar 2018 10:15:09 -0800 From: Dmitry Torokhov To: Jeffy Chen Cc: linux-kernel@vger.kernel.org, briannorris@google.com, heiko@sntech.de, dtor@google.com, dianders@google.com, devicetree@vger.kernel.org, Guenter Roeck , Thomas Gleixner , Joseph Lo , stephen lu , Rob Herring , Kate Stewart , linux-input@vger.kernel.org, Greg Kroah-Hartman , Mark Rutland , Philippe Ombredanne , Arvind Yadav Subject: Re: [PATCH v5 1/3] Input: gpio-keys - add support for wakeup event action Message-ID: <20180310181509.GB260013@dtor-ws> References: <20180308073213.8419-1-jeffy.chen@rock-chips.com> <20180308073213.8419-2-jeffy.chen@rock-chips.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20180308073213.8419-2-jeffy.chen@rock-chips.com> User-Agent: Mutt/1.9.2 (2017-12-15) Sender: linux-input-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-input@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Hi Jeffy, On Thu, Mar 08, 2018 at 03:32:11PM +0800, Jeffy Chen wrote: > Add support for specifying event actions to trigger wakeup when using > the gpio-keys input device as a wakeup source. > > This would allow the device to configure when to wakeup the system. For > example a gpio-keys input device for pen insert, may only want to wakeup > the system when ejecting the pen. > > Suggested-by: Brian Norris > Signed-off-by: Jeffy Chen > --- > > Changes in v5: > Remove unneeded irq_wake flag as Andy suggested. > > Changes in v4: > Add dt-binding gpio-keys.h, stop saving irq trigger type, add enable/disable wakeup helpers as Dmitry suggested. > > Changes in v3: > Adding more comments as Brian suggested. > > Changes in v2: > Specify wakeup event action instead of irq trigger type as Brian > suggested. > > drivers/input/keyboard/gpio_keys.c | 63 +++++++++++++++++++++++++++++++++-- > include/dt-bindings/input/gpio-keys.h | 13 ++++++++ > include/linux/gpio_keys.h | 2 ++ > 3 files changed, 75 insertions(+), 3 deletions(-) > create mode 100644 include/dt-bindings/input/gpio-keys.h > > diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c > index 87e613dc33b8..f6d5cfd44833 100644 > --- a/drivers/input/keyboard/gpio_keys.c > +++ b/drivers/input/keyboard/gpio_keys.c > @@ -30,6 +30,7 @@ > #include > #include > #include > +#include > > struct gpio_button_data { > const struct gpio_keys_button *button; > @@ -45,6 +46,7 @@ struct gpio_button_data { > unsigned int software_debounce; /* in msecs, for GPIO-driven buttons */ > > unsigned int irq; > + unsigned int wakeup_trigger_type; > spinlock_t lock; > bool disabled; > bool key_pressed; > @@ -540,6 +542,8 @@ static int gpio_keys_setup_key(struct platform_device *pdev, > } > > if (bdata->gpiod) { > + int active_low = gpiod_is_active_low(bdata->gpiod); > + > if (button->debounce_interval) { > error = gpiod_set_debounce(bdata->gpiod, > button->debounce_interval * 1000); > @@ -568,6 +572,24 @@ static int gpio_keys_setup_key(struct platform_device *pdev, > isr = gpio_keys_gpio_isr; > irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING; > > + switch (button->wakeup_event_action) { > + case EV_ACT_ASSERTED: > + bdata->wakeup_trigger_type = active_low ? > + IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING; > + break; > + case EV_ACT_DEASSERTED: > + bdata->wakeup_trigger_type = active_low ? > + IRQ_TYPE_EDGE_RISING : IRQ_TYPE_EDGE_FALLING; > + break; > + case EV_ACT_ANY: > + /* fall through */ > + default: > + /* > + * For other cases, we are OK letting suspend/resume > + * not reconfigure the trigger type. > + */ > + break; > + } > } else { > if (!button->irq) { > dev_err(dev, "Found button without gpio or irq\n"); > @@ -586,6 +608,11 @@ static int gpio_keys_setup_key(struct platform_device *pdev, > > isr = gpio_keys_irq_isr; > irqflags = 0; > + > + /* > + * For IRQ buttons, there is no interrupt for release. > + * So we don't need to reconfigure the trigger type for wakeup. > + */ > } > > bdata->code = &ddata->keymap[idx]; > @@ -718,6 +745,9 @@ gpio_keys_get_devtree_pdata(struct device *dev) > /* legacy name */ > fwnode_property_read_bool(child, "gpio-key,wakeup"); > > + fwnode_property_read_u32(child, "wakeup-event-action", > + &button->wakeup_event_action); > + > button->can_disable = > fwnode_property_read_bool(child, "linux,can-disable"); > > @@ -845,6 +875,31 @@ static int gpio_keys_probe(struct platform_device *pdev) > return 0; > } > > +static int gpio_keys_enable_wakeup(struct gpio_button_data *bdata) These new helpers need to be __maybe_unused in case we compile on system without suspend support. I also wanted a bit more of error handling, so I ended up with the version of the patch below. Can you please try it and let me know if I broke it. Thanks. Tested-by: Jeffy Chen diff --git a/Documentation/devicetree/bindings/input/gpio-keys.txt b/Documentation/devicetree/bindings/input/gpio-keys.txt index a94940481e55a..996ce84352cbf 100644 --- a/Documentation/devicetree/bindings/input/gpio-keys.txt +++ b/Documentation/devicetree/bindings/input/gpio-keys.txt @@ -26,6 +26,14 @@ Optional subnode-properties: If not specified defaults to 5. - wakeup-source: Boolean, button can wake-up the system. (Legacy property supported: "gpio-key,wakeup") + - wakeup-event-action: Specifies whether the key should wake the + system when asserted, when deasserted, or both. This property is + only valid for keys that wake up the system (e.g., when the + "wakeup-source" property is also provided). + Supported values are defined in linux-event-codes.h: + EV_ACT_ASSERTED - asserted + EV_ACT_DEASSERTED - deasserted + EV_ACT_ANY - both asserted and deasserted - linux,can-disable: Boolean, indicates that button is connected to dedicated (not shared) interrupt which can be disabled to suppress events from the button. diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c index 87e613dc33b80..966428da6bc71 100644 --- a/drivers/input/keyboard/gpio_keys.c +++ b/drivers/input/keyboard/gpio_keys.c @@ -30,6 +30,7 @@ #include #include #include +#include struct gpio_button_data { const struct gpio_keys_button *button; @@ -45,6 +46,7 @@ struct gpio_button_data { unsigned int software_debounce; /* in msecs, for GPIO-driven buttons */ unsigned int irq; + unsigned int wakeup_trigger_type; spinlock_t lock; bool disabled; bool key_pressed; @@ -540,6 +542,8 @@ static int gpio_keys_setup_key(struct platform_device *pdev, } if (bdata->gpiod) { + bool active_low = gpiod_is_active_low(bdata->gpiod); + if (button->debounce_interval) { error = gpiod_set_debounce(bdata->gpiod, button->debounce_interval * 1000); @@ -568,6 +572,24 @@ static int gpio_keys_setup_key(struct platform_device *pdev, isr = gpio_keys_gpio_isr; irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING; + switch (button->wakeup_event_action) { + case EV_ACT_ASSERTED: + bdata->wakeup_trigger_type = active_low ? + IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING; + break; + case EV_ACT_DEASSERTED: + bdata->wakeup_trigger_type = active_low ? + IRQ_TYPE_EDGE_RISING : IRQ_TYPE_EDGE_FALLING; + break; + case EV_ACT_ANY: + /* fall through */ + default: + /* + * For other cases, we are OK letting suspend/resume + * not reconfigure the trigger type. + */ + break; + } } else { if (!button->irq) { dev_err(dev, "Found button without gpio or irq\n"); @@ -586,6 +608,11 @@ static int gpio_keys_setup_key(struct platform_device *pdev, isr = gpio_keys_irq_isr; irqflags = 0; + + /* + * For IRQ buttons, there is no interrupt for release. + * So we don't need to reconfigure the trigger type for wakeup. + */ } bdata->code = &ddata->keymap[idx]; @@ -718,6 +745,9 @@ gpio_keys_get_devtree_pdata(struct device *dev) /* legacy name */ fwnode_property_read_bool(child, "gpio-key,wakeup"); + fwnode_property_read_u32(child, "wakeup-event-action", + &button->wakeup_event_action); + button->can_disable = fwnode_property_read_bool(child, "linux,can-disable"); @@ -845,19 +875,112 @@ static int gpio_keys_probe(struct platform_device *pdev) return 0; } +static int __maybe_unused +gpio_keys_button_enable_wakeup(struct gpio_button_data *bdata) +{ + int error; + + error = enable_irq_wake(bdata->irq); + if (error) { + dev_err(bdata->input->dev.parent, + "failed to configure IRQ %d as wakeup source: %d\n", + bdata->irq, error); + return error; + } + + if (bdata->wakeup_trigger_type) { + error = irq_set_irq_type(bdata->irq, + bdata->wakeup_trigger_type); + if (error) { + dev_err(bdata->input->dev.parent, + "failed to set wakeup trigger %08x for IRQ %d: %d\n", + bdata->wakeup_trigger_type, bdata->irq, error); + disable_irq_wake(bdata->irq); + return error; + } + } + + return 0; +} + +static void __maybe_unused +gpio_keys_button_disable_wakeup(struct gpio_button_data *bdata) +{ + int error; + + /* + * The trigger type is always both edges for gpio-based keys and we do + * not support changing wakeup trigger for interrupt-based keys. + */ + if (bdata->wakeup_trigger_type) { + error = irq_set_irq_type(bdata->irq, IRQ_TYPE_EDGE_BOTH); + if (error) + dev_warn(bdata->input->dev.parent, + "failed to restore interrupt trigger for IRQ %d: %d\n", + bdata->irq, error); + } + + error = disable_irq_wake(bdata->irq); + if (error) + dev_warn(bdata->input->dev.parent, + "failed to disable IRQ %d as wake source: %d\n", + bdata->irq, error); +} + +static int __maybe_unused +gpio_keys_enable_wakeup(struct gpio_keys_drvdata *ddata) +{ + struct gpio_button_data *bdata; + int error; + int i; + + for (i = 0; i < ddata->pdata->nbuttons; i++) { + bdata = &ddata->data[i]; + if (bdata->button->wakeup) { + error = gpio_keys_button_enable_wakeup(bdata); + if (error) + goto err_out; + } + bdata->suspended = true; + } + + return 0; + +err_out: + while (--i >= 0) { + bdata = &ddata->data[i]; + if (bdata->button->wakeup) + gpio_keys_button_disable_wakeup(bdata); + bdata->suspended = false; + } + + return error; +} + +static void __maybe_unused +gpio_keys_disable_wakeup(struct gpio_keys_drvdata *ddata) +{ + struct gpio_button_data *bdata; + int i; + + for (i = 0; i < ddata->pdata->nbuttons; i++) { + bdata = &ddata->data[i]; + bdata->suspended = false; + if (irqd_is_wakeup_set(irq_get_irq_data(bdata->irq))) + gpio_keys_button_disable_wakeup(bdata); + } +} + static int __maybe_unused gpio_keys_suspend(struct device *dev) { struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev); struct input_dev *input = ddata->input; - int i; + int error; if (device_may_wakeup(dev)) { - for (i = 0; i < ddata->pdata->nbuttons; i++) { - struct gpio_button_data *bdata = &ddata->data[i]; - if (bdata->button->wakeup) - enable_irq_wake(bdata->irq); - bdata->suspended = true; - } + error = gpio_keys_enable_wakeup(ddata); + if (error) + return error; } else { mutex_lock(&input->mutex); if (input->users) @@ -873,15 +996,9 @@ static int __maybe_unused gpio_keys_resume(struct device *dev) struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev); struct input_dev *input = ddata->input; int error = 0; - int i; if (device_may_wakeup(dev)) { - for (i = 0; i < ddata->pdata->nbuttons; i++) { - struct gpio_button_data *bdata = &ddata->data[i]; - if (bdata->button->wakeup) - disable_irq_wake(bdata->irq); - bdata->suspended = false; - } + gpio_keys_disable_wakeup(ddata); } else { mutex_lock(&input->mutex); if (input->users) diff --git a/include/dt-bindings/input/gpio-keys.h b/include/dt-bindings/input/gpio-keys.h new file mode 100644 index 0000000000000..8962df79e753b --- /dev/null +++ b/include/dt-bindings/input/gpio-keys.h @@ -0,0 +1,13 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * This header provides constants for gpio keys bindings. + */ + +#ifndef _DT_BINDINGS_GPIO_KEYS_H +#define _DT_BINDINGS_GPIO_KEYS_H + +#define EV_ACT_ANY 0x00 /* asserted or deasserted */ +#define EV_ACT_ASSERTED 0x01 /* asserted */ +#define EV_ACT_DEASSERTED 0x02 /* deasserted */ + +#endif /* _DT_BINDINGS_GPIO_KEYS_H */ diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h index d06bf77400f16..7160df54a6fe3 100644 --- a/include/linux/gpio_keys.h +++ b/include/linux/gpio_keys.h @@ -13,6 +13,7 @@ struct device; * @desc: label that will be attached to button's gpio * @type: input event type (%EV_KEY, %EV_SW, %EV_ABS) * @wakeup: configure the button as a wake-up source + * @wakeup_event_action: event action to trigger wakeup * @debounce_interval: debounce ticks interval in msecs * @can_disable: %true indicates that userspace is allowed to * disable button via sysfs @@ -26,6 +27,7 @@ struct gpio_keys_button { const char *desc; unsigned int type; int wakeup; + int wakeup_event_action; int debounce_interval; bool can_disable; int value;