Re: [PATCH v4] Input: gpio_keys: Add level trigger support for GPIO keys

[Arnd Bergmann]

On Thu, Mar 1, 2018 at 2:59 PM, Andy Shevchenko
 wrote:
> On Thu, Mar 1, 2018 at 2:37 PM, Arnd Bergmann  wrote:
>> On Thu, Mar 1, 2018 at 1:09 PM, Andy Shevchenko
>>  wrote:
>>> On Thu, Mar 1, 2018 at 9:36 AM, Baolin Wang  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

Re: [PATCH v4] Input: gpio_keys: Add level trigger support for GPIO keys

[Arnd Bergmann]

On Thu, Mar 1, 2018 at 2:59 PM, Andy Shevchenko
 wrote:
> On Thu, Mar 1, 2018 at 2:37 PM, Arnd Bergmann  wrote:
>> On Thu, Mar 1, 2018 at 1:09 PM, Andy Shevchenko
>>  wrote:
>>> On Thu, Mar 1, 2018 at 9:36 AM, Baolin Wang  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

Re: [PATCH v4] Input: gpio_keys: Add level trigger support for GPIO keys

[Andy Shevchenko]

On Thu, Mar 1, 2018 at 2:37 PM, Arnd Bergmann  wrote:
> On Thu, Mar 1, 2018 at 1:09 PM, Andy Shevchenko
>  wrote:
>> On Thu, Mar 1, 2018 at 9:36 AM, Baolin Wang  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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4] Input: gpio_keys: Add level trigger support for GPIO keys

[Andy Shevchenko]

On Thu, Mar 1, 2018 at 2:37 PM, Arnd Bergmann  wrote:
> On Thu, Mar 1, 2018 at 1:09 PM, Andy Shevchenko
>  wrote:
>> On Thu, Mar 1, 2018 at 9:36 AM, Baolin Wang  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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4] Input: gpio_keys: Add level trigger support for GPIO keys

[Arnd Bergmann]

On Thu, Mar 1, 2018 at 1:09 PM, Andy Shevchenko
 wrote:
> On Thu, Mar 1, 2018 at 9:36 AM, Baolin Wang  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

Re: [PATCH v4] Input: gpio_keys: Add level trigger support for GPIO keys

[Arnd Bergmann]

On Thu, Mar 1, 2018 at 1:09 PM, Andy Shevchenko
 wrote:
> On Thu, Mar 1, 2018 at 9:36 AM, Baolin Wang  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

Re: [PATCH v4] Input: gpio_keys: Add level trigger support for GPIO keys

[Andy Shevchenko]

On Thu, Mar 1, 2018 at 9:36 AM, Baolin Wang  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?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4] Input: gpio_keys: Add level trigger support for GPIO keys

[Andy Shevchenko]

On Thu, Mar 1, 2018 at 9:36 AM, Baolin Wang  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?

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4] Input: gpio_keys: Add level trigger support for GPIO keys

[Baolin Wang]

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 
---
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(-)

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(>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", >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",
 >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;
 };
 
 /**
-- 
1.7.9.5

[PATCH v4] Input: gpio_keys: Add level trigger support for GPIO keys

[Baolin Wang]

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 
---
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(-)

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(>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", >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",
 >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;
 };
 
 /**
-- 
1.7.9.5