| Message ID | 9ea2faf9c1cf8439f3a8ad495c6a2b49dacad7bd.1519889208.git.baolin.wang@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 1, 2018 at 9:36 AM, Baolin Wang <baolin.wang@linaro.org> wrote: > On some platforms (such as Spreadtrum platform), the GPIO keys can only > be triggered by level type. So this patch introduces one trigger_type to > indicate if the button's interrupt type is level trigger or edge trigger. > button->irq = > irq_of_parse_and_map(to_of_node(child), 0); AFAIU, 0 means NO_IRQ, thus, > + if (button->irq) > + button->trigger_type = > + irq_get_trigger_type(button->irq); irq_get_trigger_type(NO_IRQ) should return 0. Therefore, if (button->irq) is redundant. Did I miss anything?
On Thu, Mar 1, 2018 at 1:09 PM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Mar 1, 2018 at 9:36 AM, Baolin Wang <baolin.wang@linaro.org> wrote: >> On some platforms (such as Spreadtrum platform), the GPIO keys can only >> be triggered by level type. So this patch introduces one trigger_type to >> indicate if the button's interrupt type is level trigger or edge trigger. > >> button->irq = >> irq_of_parse_and_map(to_of_node(child), 0); > > AFAIU, 0 means NO_IRQ, thus, > >> + if (button->irq) >> + button->trigger_type = >> + irq_get_trigger_type(button->irq); > > irq_get_trigger_type(NO_IRQ) should return 0. > > Therefore, if (button->irq) is redundant. > Did I miss anything? "irq_of_parse_and_map(to_of_node(child), 0);" is the first interrupt. A more common way to express the same thing in modern drivers is 'platform_get_irq(pdev, 0)'. However, there is one more thing I'm missing: with my suggestion of emulating edge-triggered interrupts in the irqchip/gpio driver, the trigger type here should also be 'IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING' in the DT description, so I don't think we need to change the gpio-keys driver at all, just the gpio driver. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 1, 2018 at 2:37 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Thu, Mar 1, 2018 at 1:09 PM, Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: >> On Thu, Mar 1, 2018 at 9:36 AM, Baolin Wang <baolin.wang@linaro.org> wrote: >>> On some platforms (such as Spreadtrum platform), the GPIO keys can only >>> be triggered by level type. So this patch introduces one trigger_type to >>> indicate if the button's interrupt type is level trigger or edge trigger. >> >>> button->irq = >>> irq_of_parse_and_map(to_of_node(child), 0); >> >> AFAIU, 0 means NO_IRQ, thus, >> >>> + if (button->irq) >>> + button->trigger_type = >>> + irq_get_trigger_type(button->irq); >> >> irq_get_trigger_type(NO_IRQ) should return 0. >> >> Therefore, if (button->irq) is redundant. >> Did I miss anything? > > "irq_of_parse_and_map(to_of_node(child), 0);" is the first interrupt. A > more common way to express the same thing in modern drivers is > 'platform_get_irq(pdev, 0)'. Yes, though I'm talking about _returned_ value. > However, there is one more thing I'm missing: with my suggestion > of emulating edge-triggered interrupts in the irqchip/gpio driver, the > trigger type here should also be 'IRQF_TRIGGER_RISING | > IRQF_TRIGGER_FALLING' in the DT description, so I don't think > we need to change the gpio-keys driver at all, just the gpio driver. Sounds even better.
On Thu, Mar 1, 2018 at 2:59 PM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Mar 1, 2018 at 2:37 PM, Arnd Bergmann <arnd@arndb.de> wrote: >> On Thu, Mar 1, 2018 at 1:09 PM, Andy Shevchenko >> <andy.shevchenko@gmail.com> wrote: >>> On Thu, Mar 1, 2018 at 9:36 AM, Baolin Wang <baolin.wang@linaro.org> wrote: >>>> On some platforms (such as Spreadtrum platform), the GPIO keys can only >>>> be triggered by level type. So this patch introduces one trigger_type to >>>> indicate if the button's interrupt type is level trigger or edge trigger. >>> >>>> button->irq = >>>> irq_of_parse_and_map(to_of_node(child), 0); >>> >>> AFAIU, 0 means NO_IRQ, thus, >>> >>>> + if (button->irq) >>>> + button->trigger_type = >>>> + irq_get_trigger_type(button->irq); >>> >>> irq_get_trigger_type(NO_IRQ) should return 0. >>> >>> Therefore, if (button->irq) is redundant. >>> Did I miss anything? >> >> "irq_of_parse_and_map(to_of_node(child), 0);" is the first interrupt. A >> more common way to express the same thing in modern drivers is >> 'platform_get_irq(pdev, 0)'. > > Yes, though I'm talking about _returned_ value. Got it, yes you are right. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c index 87e613d..614ee29 100644 --- a/drivers/input/keyboard/gpio_keys.c +++ b/drivers/input/keyboard/gpio_keys.c @@ -566,7 +566,10 @@ static int gpio_keys_setup_key(struct platform_device *pdev, INIT_DELAYED_WORK(&bdata->work, gpio_keys_gpio_work_func); isr = gpio_keys_gpio_isr; - irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING; + if (button->trigger_type) + irqflags = button->trigger_type; + else + irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING; } else { if (!button->irq) { @@ -696,10 +699,15 @@ static void gpio_keys_close(struct input_dev *input) device_property_read_string(dev, "label", &pdata->name); device_for_each_child_node(dev, child) { - if (is_of_node(child)) + if (is_of_node(child)) { button->irq = irq_of_parse_and_map(to_of_node(child), 0); + if (button->irq) + button->trigger_type = + irq_get_trigger_type(button->irq); + } + if (fwnode_property_read_u32(child, "linux,code", &button->code)) { dev_err(dev, "Button without keycode\n"); diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h index d06bf77..eae87e2 100644 --- a/include/linux/gpio_keys.h +++ b/include/linux/gpio_keys.h @@ -18,6 +18,7 @@ * disable button via sysfs * @value: axis value for %EV_ABS * @irq: Irq number in case of interrupt keys + * @trigger_type: indicate the button's interrupt type */ struct gpio_keys_button { unsigned int code; @@ -30,6 +31,7 @@ struct gpio_keys_button { bool can_disable; int value; unsigned int irq; + unsigned int trigger_type; }; /**
On some platforms (such as Spreadtrum platform), the GPIO keys can only be triggered by level type. So this patch introduces one trigger_type to indicate if the button's interrupt type is level trigger or edge trigger. Signed-off-by: Baolin Wang <baolin.wang@linaro.org> --- Changes since v3: - Remove the reserse level logic into gpio irqchip. - Add one trigger_type to indicate teh button's trigger type. Changes since v2: - Use 'interrupt' property to indicate the irq type. Changes since v1: - Diable the GPIO irq until reversing the GPIO level type. --- drivers/input/keyboard/gpio_keys.c | 12 ++++++++++-- include/linux/gpio_keys.h | 2 ++ 2 files changed, 12 insertions(+), 2 deletions(-)