Re: [PATCH] input: vt8500: Add power button keypad driver
On Mon, Dec 31, 2012 at 12:44:31PM +1300, Tony Prisk wrote:
>
> > > + status = readl(pmc_base + 0x14);
> > > + udelay(100);
> > > + writel(status, pmc_base + 0x14);
> > > +
> > > + if (status & BIT(14)) {
> > > + if (!power_button_pressed) {
> >
> > No need to do this check.
> >
> The hardware generates multiple interrupts when the button is held.
> Without the !power_button_pressed, it will generate multiple pressed
> events without releases, because the timer doesn't get to finish.
Input core will filter out duplicate events anyway though...
And your current way means that timer is never adjusted, so the key will
be released and reported as pressed again if I keep holding it, which is
not correct.
Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] input: vt8500: Add power button keypad driver
> > +Example:
> > + powerkey: pwrkey@0 {
> > + compatible = "wm,power-keypad";
> > + interrupts = <22>;
> > + keymap = <116>; /* KEY_POWER */
>
> Do we really need this in DT? I'd say just having it manageable from
> userspace is enough.
Just seemed easier this way. Will be changed.
> > +config KEYBOARD_WMT_POWER_KEYPAD
> > + tristate "Wondermedia Power keypad support"
> > + depends on ARCH_VT8500
>
> I'd feel safer if we added "depends on OF" as well.
ARCH_VT8500 always selects USE_OF, but I can add it as well if you think
its necessary.
> > +
> > +static void __iomem *pmc_base;
> > +static struct input_dev *kpad_power;
> > +static spinlock_t kpad_power_lock;
> > +static int power_button_pressed;
> > +static struct timer_list kpad_power_timer;
> > +static unsigned int kpad_power_code;
>
> Maybe wrap it in a structure?
Will do.
>
> > +
> > +static inline void kpad_power_timeout(unsigned long fcontext)
>
> Why inline? It can't be inlined anyway since its address is used.
>
Umm, no idea why this is inline. Will remove.
> > +{
> > + u32 status;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(_power_lock, flags);
> > +
> > + status = readl(pmc_base + 0x14);
> > +
> > + if (power_button_pressed) {
>
> This check is useless, you won't ever get here if button hasn't been
> pressed.
>
Hmm, correct. Will fix.
> > + status = readl(pmc_base + 0x14);
> > + udelay(100);
> > + writel(status, pmc_base + 0x14);
> > +
> > + if (status & BIT(14)) {
> > + if (!power_button_pressed) {
>
> No need to do this check.
>
The hardware generates multiple interrupts when the button is held.
Without the !power_button_pressed, it will generate multiple pressed
events without releases, because the timer doesn't get to finish.
The interrupt is non-maskable.
> > +static int vt8500_pwr_kpad_probe(struct platform_device *pdev)
> > +{
> > + struct device_node *np;
> > + u32 status;
> > + int err;
> > + int irq;
> > +
> > + np = of_find_compatible_node(NULL, NULL, "via,vt8500-pmc");
> > + if (!np) {
> > + dev_err(>dev, "pmc node not found\n");
> > + return -EINVAL;
> > + }
> > +
> > + pmc_base = of_iomap(np, 0);
> > + if (!pmc_base) {
> > + dev_err(>dev, "unable to map pmc memory\n");
> > + return -ENOMEM;
> > + }
> > +
> > + np = pdev->dev.of_node;
> > + if (!np) {
> > + dev_err(>dev, "devicenode not found\n");
> > + return -ENODEV;
> > + }
> > +
> > + err = of_property_read_u32(np, "keymap", _power_code);
> > + if (err) {
> > + dev_warn(>dev, "defaulting to KEY_POWER\n");
> > + kpad_power_code = KEY_POWER;
> > + }
> > +
> > + /* set power button to soft-power */
> > + status = readl(pmc_base + 0x54);
> > + writel(status | 1, pmc_base + 0x54);
> > +
> > + /* clear any pending interrupts */
> > + status = readl(pmc_base + 0x14);
> > + writel(status, pmc_base + 0x14);
> > +
> > + kpad_power = input_allocate_device();
> > + if (!kpad_power) {
> > + dev_err(>dev, "failed to allocate input device\n");
> > + return -ENOMEM;
> > + }
> > +
> > + spin_lock_init(_power_lock);
> > + setup_timer(_power_timer, kpad_power_timeout,
> > + (unsigned long)kpad_power);
> > +
> > + irq = irq_of_parse_and_map(np, 0);
> > + err = request_irq(irq, _power_isr, 0, "pwrbtn", NULL);
> > + if (err < 0) {
> > + dev_err(>dev, "failed to request irq\n");
> > + return err;
> > + }
> > +
> > + kpad_power->evbit[0] = BIT_MASK(EV_KEY);
> > + set_bit(kpad_power_code, kpad_power->keybit);
> > +
> > + kpad_power->name = "wmt_power_keypad";
> > + kpad_power->phys = "wmt_power_keypad";
> > + kpad_power->keycode = _power_code;
> > + kpad_power->keycodesize = sizeof(unsigned int);
> > + kpad_power->keycodemax = 1;
> > +
> > + err = input_register_device(kpad_power);
> > + if (err < 0) {
> > + dev_err(>dev, "failed to register input device\n");
>
> You either need to use managed resources or clean up after yourself;
> leaking memory and other resources is not nice. Given that you are using
> timer, which needs to be canceled, and I am not sure if your device
> allows enabling/disabling generating interrupts while keeping the
> interrupt handler registered, manual cleanup looks like the only option
> for you.
>
> BTW, where is your remove() method?
Eeek. Too much turkey :) I will tidy this up and resubmit.
Thanks for the quick review.
Regards
Tony P
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] input: vt8500: Add power button keypad driver
Hi Tony,
On Mon, Dec 31, 2012 at 11:01:02AM +1300, Tony Prisk wrote:
> This patch adds support for the Power Button keypad found on
> Wondermedia netbooks/tablets.
>
> A keymap property is exposed to allowing defining the key
> event to be generated when the power button is pressed.
>
> Signed-off-by: Tony Prisk
> ---
> .../bindings/input/vt8500-power-keypad.txt | 17 ++
> drivers/input/keyboard/Kconfig | 10 ++
> drivers/input/keyboard/Makefile|1 +
> drivers/input/keyboard/wmt_power_keypad.c | 174
>
> 4 files changed, 202 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/input/vt8500-power-keypad.txt
> create mode 100644 drivers/input/keyboard/wmt_power_keypad.c
>
> diff --git a/Documentation/devicetree/bindings/input/vt8500-power-keypad.txt
> b/Documentation/devicetree/bindings/input/vt8500-power-keypad.txt
> new file mode 100644
> index 000..bae6228
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/vt8500-power-keypad.txt
> @@ -0,0 +1,17 @@
> +* Wondermedia Power Keypad device tree bindings
> +
> +Wondermedia SoCs have a single irq-driven power button attached to
> +the power management controller.
> +
> +Required SoC Specific Properties:
> +- compatible: should be one of the following
> + - "wm,power-keypad": For all Wondermedia 8xxx-series SoCs.
> +- interrupts: should be the power management controller wakeup interrupt.
> +- keymap: linux keycode to generate when power button pressed.
> +
> +Example:
> + powerkey: pwrkey@0 {
> + compatible = "wm,power-keypad";
> + interrupts = <22>;
> + keymap = <116>; /* KEY_POWER */
Do we really need this in DT? I'd say just having it manageable from
userspace is enough.
> + };
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 5a240c6..c270f27 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -595,6 +595,16 @@ config KEYBOARD_TWL4030
> To compile this driver as a module, choose M here: the
> module will be called twl4030_keypad.
>
> +config KEYBOARD_WMT_POWER_KEYPAD
> + tristate "Wondermedia Power keypad support"
> + depends on ARCH_VT8500
I'd feel safer if we added "depends on OF" as well.
> + help
> + Say Y here to enable support for the power button keypad
> + on Wondermedia 8xxx-series SoCs.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called wmt_power_keypad.
> +
> config KEYBOARD_XTKBD
> tristate "XT keyboard"
> select SERIO
> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> index 44e7600..eea31af 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -52,5 +52,6 @@ obj-$(CONFIG_KEYBOARD_TC3589X) +=
> tc3589x-keypad.o
> obj-$(CONFIG_KEYBOARD_TEGRA) += tegra-kbc.o
> obj-$(CONFIG_KEYBOARD_TNETV107X) += tnetv107x-keypad.o
> obj-$(CONFIG_KEYBOARD_TWL4030) += twl4030_keypad.o
> +obj-$(CONFIG_KEYBOARD_WMT_POWER_KEYPAD) += wmt_power_keypad.o
> obj-$(CONFIG_KEYBOARD_XTKBD) += xtkbd.o
> obj-$(CONFIG_KEYBOARD_W90P910) += w90p910_keypad.o
> diff --git a/drivers/input/keyboard/wmt_power_keypad.c
> b/drivers/input/keyboard/wmt_power_keypad.c
> new file mode 100644
> index 000..ce8aa9a
> --- /dev/null
> +++ b/drivers/input/keyboard/wmt_power_keypad.c
> @@ -0,0 +1,174 @@
> +/* Wondermedia Power Keypad
> + *
> + * Copyright (C) 2012 Tony Prisk
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +static void __iomem *pmc_base;
> +static struct input_dev *kpad_power;
> +static spinlock_t kpad_power_lock;
> +static int power_button_pressed;
> +static struct timer_list kpad_power_timer;
> +static unsigned int kpad_power_code;
Maybe wrap it in a structure?
> +
> +static inline void kpad_power_timeout(unsigned long fcontext)
Why inline? It can't be inlined anyway since its address is used.
> +{
> + u32 status;
> + unsigned long flags;
> +
> + spin_lock_irqsave(_power_lock, flags);
> +
> + status = readl(pmc_base + 0x14);
> +
> + if (power_button_pressed) {
This check is useless, you won't ever get here if button hasn't been
pressed.
> +
Re: [PATCH] input: vt8500: Add power button keypad driver
Hi Tony,
On Mon, Dec 31, 2012 at 11:01:02AM +1300, Tony Prisk wrote:
This patch adds support for the Power Button keypad found on
Wondermedia netbooks/tablets.
A keymap property is exposed to allowing defining the key
event to be generated when the power button is pressed.
Signed-off-by: Tony Prisk li...@prisktech.co.nz
---
.../bindings/input/vt8500-power-keypad.txt | 17 ++
drivers/input/keyboard/Kconfig | 10 ++
drivers/input/keyboard/Makefile|1 +
drivers/input/keyboard/wmt_power_keypad.c | 174
4 files changed, 202 insertions(+)
create mode 100644
Documentation/devicetree/bindings/input/vt8500-power-keypad.txt
create mode 100644 drivers/input/keyboard/wmt_power_keypad.c
diff --git a/Documentation/devicetree/bindings/input/vt8500-power-keypad.txt
b/Documentation/devicetree/bindings/input/vt8500-power-keypad.txt
new file mode 100644
index 000..bae6228
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/vt8500-power-keypad.txt
@@ -0,0 +1,17 @@
+* Wondermedia Power Keypad device tree bindings
+
+Wondermedia SoCs have a single irq-driven power button attached to
+the power management controller.
+
+Required SoC Specific Properties:
+- compatible: should be one of the following
+ - wm,power-keypad: For all Wondermedia 8xxx-series SoCs.
+- interrupts: should be the power management controller wakeup interrupt.
+- keymap: linux keycode to generate when power button pressed.
+
+Example:
+ powerkey: pwrkey@0 {
+ compatible = wm,power-keypad;
+ interrupts = 22;
+ keymap = 116; /* KEY_POWER */
Do we really need this in DT? I'd say just having it manageable from
userspace is enough.
+ };
diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index 5a240c6..c270f27 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -595,6 +595,16 @@ config KEYBOARD_TWL4030
To compile this driver as a module, choose M here: the
module will be called twl4030_keypad.
+config KEYBOARD_WMT_POWER_KEYPAD
+ tristate Wondermedia Power keypad support
+ depends on ARCH_VT8500
I'd feel safer if we added depends on OF as well.
+ help
+ Say Y here to enable support for the power button keypad
+ on Wondermedia 8xxx-series SoCs.
+
+ To compile this driver as a module, choose M here: the
+ module will be called wmt_power_keypad.
+
config KEYBOARD_XTKBD
tristate XT keyboard
select SERIO
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index 44e7600..eea31af 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -52,5 +52,6 @@ obj-$(CONFIG_KEYBOARD_TC3589X) +=
tc3589x-keypad.o
obj-$(CONFIG_KEYBOARD_TEGRA) += tegra-kbc.o
obj-$(CONFIG_KEYBOARD_TNETV107X) += tnetv107x-keypad.o
obj-$(CONFIG_KEYBOARD_TWL4030) += twl4030_keypad.o
+obj-$(CONFIG_KEYBOARD_WMT_POWER_KEYPAD) += wmt_power_keypad.o
obj-$(CONFIG_KEYBOARD_XTKBD) += xtkbd.o
obj-$(CONFIG_KEYBOARD_W90P910) += w90p910_keypad.o
diff --git a/drivers/input/keyboard/wmt_power_keypad.c
b/drivers/input/keyboard/wmt_power_keypad.c
new file mode 100644
index 000..ce8aa9a
--- /dev/null
+++ b/drivers/input/keyboard/wmt_power_keypad.c
@@ -0,0 +1,174 @@
+/* Wondermedia Power Keypad
+ *
+ * Copyright (C) 2012 Tony Prisk li...@prisktech.co.nz
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include linux/module.h
+#include linux/err.h
+#include linux/io.h
+#include linux/input.h
+#include linux/interrupt.h
+#include linux/platform_device.h
+#include linux/bitops.h
+#include linux/of.h
+#include linux/of_address.h
+#include linux/of_irq.h
+#include linux/of_device.h
+
+static void __iomem *pmc_base;
+static struct input_dev *kpad_power;
+static spinlock_t kpad_power_lock;
+static int power_button_pressed;
+static struct timer_list kpad_power_timer;
+static unsigned int kpad_power_code;
Maybe wrap it in a structure?
+
+static inline void kpad_power_timeout(unsigned long fcontext)
Why inline? It can't be inlined anyway since its address is used.
+{
+ u32 status;
+ unsigned long flags;
+
+ spin_lock_irqsave(kpad_power_lock, flags);
+
+ status = readl(pmc_base + 0x14);
+
+ if (power_button_pressed) {
This check is useless, you won't ever
Re: [PATCH] input: vt8500: Add power button keypad driver
+Example:
+ powerkey: pwrkey@0 {
+ compatible = wm,power-keypad;
+ interrupts = 22;
+ keymap = 116; /* KEY_POWER */
Do we really need this in DT? I'd say just having it manageable from
userspace is enough.
Just seemed easier this way. Will be changed.
+config KEYBOARD_WMT_POWER_KEYPAD
+ tristate Wondermedia Power keypad support
+ depends on ARCH_VT8500
I'd feel safer if we added depends on OF as well.
ARCH_VT8500 always selects USE_OF, but I can add it as well if you think
its necessary.
+
+static void __iomem *pmc_base;
+static struct input_dev *kpad_power;
+static spinlock_t kpad_power_lock;
+static int power_button_pressed;
+static struct timer_list kpad_power_timer;
+static unsigned int kpad_power_code;
Maybe wrap it in a structure?
Will do.
+
+static inline void kpad_power_timeout(unsigned long fcontext)
Why inline? It can't be inlined anyway since its address is used.
Umm, no idea why this is inline. Will remove.
+{
+ u32 status;
+ unsigned long flags;
+
+ spin_lock_irqsave(kpad_power_lock, flags);
+
+ status = readl(pmc_base + 0x14);
+
+ if (power_button_pressed) {
This check is useless, you won't ever get here if button hasn't been
pressed.
Hmm, correct. Will fix.
+ status = readl(pmc_base + 0x14);
+ udelay(100);
+ writel(status, pmc_base + 0x14);
+
+ if (status BIT(14)) {
+ if (!power_button_pressed) {
No need to do this check.
The hardware generates multiple interrupts when the button is held.
Without the !power_button_pressed, it will generate multiple pressed
events without releases, because the timer doesn't get to finish.
The interrupt is non-maskable.
+static int vt8500_pwr_kpad_probe(struct platform_device *pdev)
+{
+ struct device_node *np;
+ u32 status;
+ int err;
+ int irq;
+
+ np = of_find_compatible_node(NULL, NULL, via,vt8500-pmc);
+ if (!np) {
+ dev_err(pdev-dev, pmc node not found\n);
+ return -EINVAL;
+ }
+
+ pmc_base = of_iomap(np, 0);
+ if (!pmc_base) {
+ dev_err(pdev-dev, unable to map pmc memory\n);
+ return -ENOMEM;
+ }
+
+ np = pdev-dev.of_node;
+ if (!np) {
+ dev_err(pdev-dev, devicenode not found\n);
+ return -ENODEV;
+ }
+
+ err = of_property_read_u32(np, keymap, kpad_power_code);
+ if (err) {
+ dev_warn(pdev-dev, defaulting to KEY_POWER\n);
+ kpad_power_code = KEY_POWER;
+ }
+
+ /* set power button to soft-power */
+ status = readl(pmc_base + 0x54);
+ writel(status | 1, pmc_base + 0x54);
+
+ /* clear any pending interrupts */
+ status = readl(pmc_base + 0x14);
+ writel(status, pmc_base + 0x14);
+
+ kpad_power = input_allocate_device();
+ if (!kpad_power) {
+ dev_err(pdev-dev, failed to allocate input device\n);
+ return -ENOMEM;
+ }
+
+ spin_lock_init(kpad_power_lock);
+ setup_timer(kpad_power_timer, kpad_power_timeout,
+ (unsigned long)kpad_power);
+
+ irq = irq_of_parse_and_map(np, 0);
+ err = request_irq(irq, kpad_power_isr, 0, pwrbtn, NULL);
+ if (err 0) {
+ dev_err(pdev-dev, failed to request irq\n);
+ return err;
+ }
+
+ kpad_power-evbit[0] = BIT_MASK(EV_KEY);
+ set_bit(kpad_power_code, kpad_power-keybit);
+
+ kpad_power-name = wmt_power_keypad;
+ kpad_power-phys = wmt_power_keypad;
+ kpad_power-keycode = kpad_power_code;
+ kpad_power-keycodesize = sizeof(unsigned int);
+ kpad_power-keycodemax = 1;
+
+ err = input_register_device(kpad_power);
+ if (err 0) {
+ dev_err(pdev-dev, failed to register input device\n);
You either need to use managed resources or clean up after yourself;
leaking memory and other resources is not nice. Given that you are using
timer, which needs to be canceled, and I am not sure if your device
allows enabling/disabling generating interrupts while keeping the
interrupt handler registered, manual cleanup looks like the only option
for you.
BTW, where is your remove() method?
Eeek. Too much turkey :) I will tidy this up and resubmit.
Thanks for the quick review.
Regards
Tony P
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] input: vt8500: Add power button keypad driver
On Mon, Dec 31, 2012 at 12:44:31PM +1300, Tony Prisk wrote:
+ status = readl(pmc_base + 0x14);
+ udelay(100);
+ writel(status, pmc_base + 0x14);
+
+ if (status BIT(14)) {
+ if (!power_button_pressed) {
No need to do this check.
The hardware generates multiple interrupts when the button is held.
Without the !power_button_pressed, it will generate multiple pressed
events without releases, because the timer doesn't get to finish.
Input core will filter out duplicate events anyway though...
And your current way means that timer is never adjusted, so the key will
be released and reported as pressed again if I keep holding it, which is
not correct.
Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
