| Message ID | 7fc26df245d56fd2014532b56e67630e76e2c513.1518316248.git.baolin.wang@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Feb 11, 2018 at 02:55:04PM +0800, 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 property to > indicate if the GPIO trigger type is level trigger or edge trigger. If the parent interrupt controller only supports a certain trigger, then it should ignore setting the trigger type. > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > --- > Changes since v1: > - Diable the GPIO irq until reversing the GPIO level type. > --- > .../devicetree/bindings/input/gpio-keys.txt | 2 ++ > drivers/input/keyboard/gpio_keys.c | 26 +++++++++++++++++++- > include/linux/gpio_keys.h | 1 + > 3 files changed, 28 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/input/gpio-keys.txt b/Documentation/devicetree/bindings/input/gpio-keys.txt > index a949404..e3104bd 100644 > --- a/Documentation/devicetree/bindings/input/gpio-keys.txt > +++ b/Documentation/devicetree/bindings/input/gpio-keys.txt > @@ -29,6 +29,8 @@ Optional subnode-properties: > - linux,can-disable: Boolean, indicates that button is connected > to dedicated (not shared) interrupt which can be disabled to > suppress events from the button. > + - gpio-key,level-trigger: Boolean, indicates that button's interrupt > + type is level trigger. Otherwise it is edge trigger as default. No. Just use 'interrupts' instead of 'gpios' and specify the trigger type. Or put both if you need to read the state. > > Example nodes: > > diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c > index 87e613d..218698a 100644 > --- a/drivers/input/keyboard/gpio_keys.c > +++ b/drivers/input/keyboard/gpio_keys.c > @@ -385,6 +385,20 @@ static void gpio_keys_gpio_work_func(struct work_struct *work) > struct gpio_button_data *bdata = > container_of(work, struct gpio_button_data, work.work); > > + if (bdata->button->level_trigger) { > + unsigned int trigger = > + irq_get_trigger_type(bdata->irq) & ~IRQF_TRIGGER_MASK; > + int state = gpiod_get_raw_value_cansleep(bdata->gpiod); > + > + if (state) > + trigger |= IRQF_TRIGGER_LOW; > + else > + trigger |= IRQF_TRIGGER_HIGH; > + > + irq_set_irq_type(bdata->irq, trigger); > + enable_irq(bdata->irq); > + } > + > gpio_keys_gpio_report_event(bdata); > > if (bdata->button->wakeup) > @@ -397,6 +411,9 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id) > > BUG_ON(irq != bdata->irq); > > + if (bdata->button->level_trigger) > + disable_irq_nosync(bdata->irq); > + > if (bdata->button->wakeup) { > const struct gpio_keys_button *button = bdata->button; > > @@ -566,7 +583,11 @@ 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->level_trigger) > + irqflags = gpiod_is_active_low(bdata->gpiod) ? > + IRQF_TRIGGER_LOW : IRQF_TRIGGER_HIGH; > + else > + irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING; > > } else { > if (!button->irq) { > @@ -721,6 +742,9 @@ static void gpio_keys_close(struct input_dev *input) > button->can_disable = > fwnode_property_read_bool(child, "linux,can-disable"); > > + button->level_trigger = > + fwnode_property_read_bool(child, "gpio-key,level-trigger"); > + > if (fwnode_property_read_u32(child, "debounce-interval", > &button->debounce_interval)) > button->debounce_interval = 5; > diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h > index d06bf77..5095645 100644 > --- a/include/linux/gpio_keys.h > +++ b/include/linux/gpio_keys.h > @@ -28,6 +28,7 @@ struct gpio_keys_button { > int wakeup; > int debounce_interval; > bool can_disable; > + bool level_trigger; > int value; > unsigned int irq; > }; > -- > 1.7.9.5 > -- 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
Hi Rob, On 20 February 2018 at 02:11, Rob Herring <robh@kernel.org> wrote: > On Sun, Feb 11, 2018 at 02:55:04PM +0800, 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 property to >> indicate if the GPIO trigger type is level trigger or edge trigger. > > If the parent interrupt controller only supports a certain trigger, then > it should ignore setting the trigger type. We still need to set high level type trigger or low level type trigger if it only supports level trigger. > >> >> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> >> --- >> Changes since v1: >> - Diable the GPIO irq until reversing the GPIO level type. >> --- >> .../devicetree/bindings/input/gpio-keys.txt | 2 ++ >> drivers/input/keyboard/gpio_keys.c | 26 +++++++++++++++++++- >> include/linux/gpio_keys.h | 1 + >> 3 files changed, 28 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/input/gpio-keys.txt b/Documentation/devicetree/bindings/input/gpio-keys.txt >> index a949404..e3104bd 100644 >> --- a/Documentation/devicetree/bindings/input/gpio-keys.txt >> +++ b/Documentation/devicetree/bindings/input/gpio-keys.txt >> @@ -29,6 +29,8 @@ Optional subnode-properties: >> - linux,can-disable: Boolean, indicates that button is connected >> to dedicated (not shared) interrupt which can be disabled to >> suppress events from the button. >> + - gpio-key,level-trigger: Boolean, indicates that button's interrupt >> + type is level trigger. Otherwise it is edge trigger as default. > > No. Just use 'interrupts' instead of 'gpios' and specify the trigger > type. Or put both if you need to read the state. Okay, so something as below to get the level type from the 'interrupts' property. if (fwnode_property_read_u32(child, "interrupts", &button->level_type)) button->level_type = IRQ_TYPE_NONE;
Hi Rob, On 21 February 2018 at 19:35, Baolin Wang <baolin.wang@linaro.org> wrote: > Hi Rob, > > On 20 February 2018 at 02:11, Rob Herring <robh@kernel.org> wrote: >> On Sun, Feb 11, 2018 at 02:55:04PM +0800, 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 property to >>> indicate if the GPIO trigger type is level trigger or edge trigger. >> >> If the parent interrupt controller only supports a certain trigger, then >> it should ignore setting the trigger type. > > We still need to set high level type trigger or low level type trigger > if it only supports level trigger. > >> >>> >>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> >>> --- >>> Changes since v1: >>> - Diable the GPIO irq until reversing the GPIO level type. >>> --- >>> .../devicetree/bindings/input/gpio-keys.txt | 2 ++ >>> drivers/input/keyboard/gpio_keys.c | 26 +++++++++++++++++++- >>> include/linux/gpio_keys.h | 1 + >>> 3 files changed, 28 insertions(+), 1 deletion(-) >>> >>> diff --git a/Documentation/devicetree/bindings/input/gpio-keys.txt b/Documentation/devicetree/bindings/input/gpio-keys.txt >>> index a949404..e3104bd 100644 >>> --- a/Documentation/devicetree/bindings/input/gpio-keys.txt >>> +++ b/Documentation/devicetree/bindings/input/gpio-keys.txt >>> @@ -29,6 +29,8 @@ Optional subnode-properties: >>> - linux,can-disable: Boolean, indicates that button is connected >>> to dedicated (not shared) interrupt which can be disabled to >>> suppress events from the button. >>> + - gpio-key,level-trigger: Boolean, indicates that button's interrupt >>> + type is level trigger. Otherwise it is edge trigger as default. >> >> No. Just use 'interrupts' instead of 'gpios' and specify the trigger >> type. Or put both if you need to read the state. > > Okay, so something as below to get the level type from the > 'interrupts' property. > if (fwnode_property_read_u32(child, "interrupts", &button->level_type)) > button->level_type = IRQ_TYPE_NONE; After more thinking, if we use 'interrupts' to indicate the irq type for this case, we cannot specify the irq number due to the irq number should be get by gpiod_to_irq(). So the device nodes look weird, since we should define the index of the interrupt controller instead of the irq type if the #interrupt_cells is set to 1 according to the interrupt controller documentation. What do you think about this? Thanks. gpio-keys { compatible = "gpio-keys"; key-volumedown { label = "Volume Down Key"; linux,code = <KEY_VOLUMEDOWN>; gpios = <&ap_eic_debounce 2 GPIO_ACTIVE_LOW>; debounce-interval = <2>; wakeup-source; interrupts = <IRQ_TYPE_LEVEL_LOW>; }; key-volumeup { label = "Volume Up Key"; linux,code = <KEY_VOLUMEUP>; gpios = <&pmic_eic 10 GPIO_ACTIVE_HIGH>; debounce-interval = <2>; wakeup-source; interrupts = <IRQ_TYPE_LEVEL_HIGH>; }; key-power { label = "Power Key"; linux,code = <KEY_POWER>; gpios = <&pmic_eic 1 GPIO_ACTIVE_HIGH>; wakeup-source; interrupts = <IRQ_TYPE_LEVEL_HIGH>; }; };
On Mon, Feb 26, 2018 at 7:24 AM, Baolin Wang <baolin.wang@linaro.org> wrote: > On 21 February 2018 at 19:35, Baolin Wang <baolin.wang@linaro.org> wrote: >> On 20 February 2018 at 02:11, Rob Herring <robh@kernel.org> wrote: >>>> diff --git a/Documentation/devicetree/bindings/input/gpio-keys.txt b/Documentation/devicetree/bindings/input/gpio-keys.txt >>>> index a949404..e3104bd 100644 >>>> --- a/Documentation/devicetree/bindings/input/gpio-keys.txt >>>> +++ b/Documentation/devicetree/bindings/input/gpio-keys.txt >>>> @@ -29,6 +29,8 @@ Optional subnode-properties: >>>> - linux,can-disable: Boolean, indicates that button is connected >>>> to dedicated (not shared) interrupt which can be disabled to >>>> suppress events from the button. >>>> + - gpio-key,level-trigger: Boolean, indicates that button's interrupt >>>> + type is level trigger. Otherwise it is edge trigger as default. >>> >>> No. Just use 'interrupts' instead of 'gpios' and specify the trigger >>> type. Or put both if you need to read the state. >> >> Okay, so something as below to get the level type from the >> 'interrupts' property. >> if (fwnode_property_read_u32(child, "interrupts", &button->level_type)) >> button->level_type = IRQ_TYPE_NONE; > > After more thinking, if we use 'interrupts' to indicate the irq type > for this case, we cannot specify the irq number due to the irq number > should be get by gpiod_to_irq(). So the device nodes look weird, since > we should define the index of the interrupt controller instead of the > irq type if the #interrupt_cells is set to 1 according to the > interrupt controller documentation. What do you think about this? I think what you're ultimately seeing is a bad fit between this GPIO/irq-controller and the Linux gpio keys driver, it doesn't have very much to do with the device tree bindings. What I think is appropriate is to try to create a new input driver in Linux that just takes an interrupt, not a GPIO, and let the GPIO chip only act as an irqchip for this. This avoid the complicated step of converting a GPIO to an interrupt in order to use it, when all you really want to do is use an interrupt, you don't really care about the GPIO here, correct? So we would create drivers/input/keyboard/interrupt_keys.c however I suspect a bunch of code would need to be shared with gpio_keys.c so maybe it is necessary to break out the parts of gpio_keys.c into its own helper file. Or maybe even have the pure interrupt handling as part of gpio_keys.c, i.e. if the driver can't find any associated GPIO, it goes on to check if there is an interrupt assigned to the device node and use that directly instead. Either way, Dmitry must be involved. Yours, Linus Walleij -- 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 28 February 2018 at 20:53, Linus Walleij <linus.walleij@linaro.org> wrote: > On Mon, Feb 26, 2018 at 7:24 AM, Baolin Wang <baolin.wang@linaro.org> wrote: >> On 21 February 2018 at 19:35, Baolin Wang <baolin.wang@linaro.org> wrote: >>> On 20 February 2018 at 02:11, Rob Herring <robh@kernel.org> wrote: > >>>>> diff --git a/Documentation/devicetree/bindings/input/gpio-keys.txt b/Documentation/devicetree/bindings/input/gpio-keys.txt >>>>> index a949404..e3104bd 100644 >>>>> --- a/Documentation/devicetree/bindings/input/gpio-keys.txt >>>>> +++ b/Documentation/devicetree/bindings/input/gpio-keys.txt >>>>> @@ -29,6 +29,8 @@ Optional subnode-properties: >>>>> - linux,can-disable: Boolean, indicates that button is connected >>>>> to dedicated (not shared) interrupt which can be disabled to >>>>> suppress events from the button. >>>>> + - gpio-key,level-trigger: Boolean, indicates that button's interrupt >>>>> + type is level trigger. Otherwise it is edge trigger as default. >>>> >>>> No. Just use 'interrupts' instead of 'gpios' and specify the trigger >>>> type. Or put both if you need to read the state. >>> >>> Okay, so something as below to get the level type from the >>> 'interrupts' property. >>> if (fwnode_property_read_u32(child, "interrupts", &button->level_type)) >>> button->level_type = IRQ_TYPE_NONE; >> >> After more thinking, if we use 'interrupts' to indicate the irq type >> for this case, we cannot specify the irq number due to the irq number >> should be get by gpiod_to_irq(). So the device nodes look weird, since >> we should define the index of the interrupt controller instead of the >> irq type if the #interrupt_cells is set to 1 according to the >> interrupt controller documentation. What do you think about this? > > I think what you're ultimately seeing is a bad fit between this > GPIO/irq-controller and the Linux gpio keys driver, it doesn't > have very much to do with the device tree bindings. > > What I think is appropriate is to try to create a new input driver > in Linux that just takes an interrupt, not a GPIO, and let the > GPIO chip only act as an irqchip for this. > > This avoid the complicated step of converting a GPIO to an > interrupt in order to use it, when all you really want to > do is use an interrupt, you don't really care about the > GPIO here, correct? Sometimes we should set the GPIO debounce, and read GPIO value to report the event, so I think we can not remove GPIO. > > So we would create > drivers/input/keyboard/interrupt_keys.c > however I suspect a bunch of code would need to be shared > with gpio_keys.c so maybe it is necessary to break out the > parts of gpio_keys.c into its own helper file. > > Or maybe even have the > pure interrupt handling as part of gpio_keys.c, i.e. if the > driver can't find any associated GPIO, it goes on to > check if there is an interrupt assigned to the device node > and use that directly instead. > > Either way, Dmitry must be involved. > > Yours, > Linus Walleij
diff --git a/Documentation/devicetree/bindings/input/gpio-keys.txt b/Documentation/devicetree/bindings/input/gpio-keys.txt index a949404..e3104bd 100644 --- a/Documentation/devicetree/bindings/input/gpio-keys.txt +++ b/Documentation/devicetree/bindings/input/gpio-keys.txt @@ -29,6 +29,8 @@ Optional subnode-properties: - linux,can-disable: Boolean, indicates that button is connected to dedicated (not shared) interrupt which can be disabled to suppress events from the button. + - gpio-key,level-trigger: Boolean, indicates that button's interrupt + type is level trigger. Otherwise it is edge trigger as default. Example nodes: diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c index 87e613d..218698a 100644 --- a/drivers/input/keyboard/gpio_keys.c +++ b/drivers/input/keyboard/gpio_keys.c @@ -385,6 +385,20 @@ static void gpio_keys_gpio_work_func(struct work_struct *work) struct gpio_button_data *bdata = container_of(work, struct gpio_button_data, work.work); + if (bdata->button->level_trigger) { + unsigned int trigger = + irq_get_trigger_type(bdata->irq) & ~IRQF_TRIGGER_MASK; + int state = gpiod_get_raw_value_cansleep(bdata->gpiod); + + if (state) + trigger |= IRQF_TRIGGER_LOW; + else + trigger |= IRQF_TRIGGER_HIGH; + + irq_set_irq_type(bdata->irq, trigger); + enable_irq(bdata->irq); + } + gpio_keys_gpio_report_event(bdata); if (bdata->button->wakeup) @@ -397,6 +411,9 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id) BUG_ON(irq != bdata->irq); + if (bdata->button->level_trigger) + disable_irq_nosync(bdata->irq); + if (bdata->button->wakeup) { const struct gpio_keys_button *button = bdata->button; @@ -566,7 +583,11 @@ 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->level_trigger) + irqflags = gpiod_is_active_low(bdata->gpiod) ? + IRQF_TRIGGER_LOW : IRQF_TRIGGER_HIGH; + else + irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING; } else { if (!button->irq) { @@ -721,6 +742,9 @@ static void gpio_keys_close(struct input_dev *input) button->can_disable = fwnode_property_read_bool(child, "linux,can-disable"); + button->level_trigger = + fwnode_property_read_bool(child, "gpio-key,level-trigger"); + if (fwnode_property_read_u32(child, "debounce-interval", &button->debounce_interval)) button->debounce_interval = 5; diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h index d06bf77..5095645 100644 --- a/include/linux/gpio_keys.h +++ b/include/linux/gpio_keys.h @@ -28,6 +28,7 @@ struct gpio_keys_button { int wakeup; int debounce_interval; bool can_disable; + bool level_trigger; int value; unsigned int irq; };
On some platforms (such as Spreadtrum platform), the GPIO keys can only be triggered by level type. So this patch introduces one property to indicate if the GPIO trigger type is level trigger or edge trigger. Signed-off-by: Baolin Wang <baolin.wang@linaro.org> --- Changes since v1: - Diable the GPIO irq until reversing the GPIO level type. --- .../devicetree/bindings/input/gpio-keys.txt | 2 ++ drivers/input/keyboard/gpio_keys.c | 26 +++++++++++++++++++- include/linux/gpio_keys.h | 1 + 3 files changed, 28 insertions(+), 1 deletion(-)