Re: [PATCH 2/2] Input: misc: introduce palmas-pwrbutton
Hi Dmitry
On 08/19/2014 12:23 AM, Dmitry Torokhov wrote:
Thanks for the review.
[...]
+
+/**
+ * pwron_irq() - button press isr
+ * @irq: irq
+ * @palmas_pwron: pwron struct
+ */
+static irqreturn_t pwron_irq(int irq, void *palmas_pwron)
+{
+ struct palmas_pwron *pwron = palmas_pwron;
+ struct input_dev *input_dev = pwron-input_dev;
+
+ cancel_delayed_work_sync(pwron-input_work);
+
+ pwron-current_state = PALMAS_PWR_KEY_PRESS;
+
+ input_report_key(input_dev, KEY_POWER, pwron-current_state);
+ pm_wakeup_event(input_dev-dev.parent, 0);
+ input_sync(input_dev);
+
+ schedule_delayed_work(pwron-input_work, 0);
Instead of cancel/schedule use mod_delayed_work. BTW, why do you need to
schedule immediately instead of waiting key_recheck_ms? Also, are there any
Good point, I had missed these. Will fix.
concerns about need to debounce?
I believe PMIC already takes care of debounce, let me see if there are
configuration registers possible. if yes, I think it might be nice to
add in.
[...]
+
+ irq = platform_get_irq(pdev, 0);
+
+ device_init_wakeup(dev, 1);
+
+ ret = devm_request_threaded_irq(dev, irq, NULL, pwron_irq,
+ IRQF_TRIGGER_HIGH |
+ IRQF_TRIGGER_LOW,
+ dev_name(dev),
+ pwron);
I am confused about this code sequence. Why do we get IRQ, then set up wakeup,
and then request irq? Normally you get irq number, and then you request it, and
then do other stuff.
Uggh.. right.. will fix.
+ if (ret 0) {
+ dev_err(dev, Can't get IRQ for pwron: %d\n, ret);
+ return ret;
+ }
+
+ enable_irq_wake(irq);
Shouldn't this be in suspend callback?
yes, it should have been.. my bad.. :( thanks for catching it.
+
+ ret = input_register_device(input_dev);
+ if (ret) {
+ dev_dbg(dev, Can't register power button: %d\n, ret);
+ goto out_irq_wake;
+ }
+ pwron-irq = irq;
+
+ pwron-key_recheck_ms = PALMAS_PWR_KEY_Q_TIME_MS;
+
+ platform_set_drvdata(pdev, pwron);
+
+ return 0;
+
+out_irq_wake:
+ disable_irq_wake(irq);
+
+ return ret;
+}
+
+static int palmas_pwron_remove(struct platform_device *pdev)
+{
+ struct palmas_pwron *pwron = platform_get_drvdata(pdev);
+
+ disable_irq_wake(pwron-irq);
Should be in resume callback().
yep.
+ input_unregister_device(pwron-input_dev);
With devm you do not need to unregister input device. However this has problem:
what will happen if interrupt arrives here and we schedule workqueue? You need
free interrupt then cancel work and then free input device. Similar needs to be
done in probe(). I'd recommend not use devm_* here as you need to manually
unwind anyway.
True. I will fix these as well.
+
+ return 0;
+}
+
+#ifdef CONFIG_PM
+/**
+ * palmas_pwron_suspend() - suspend handler
+ * @dev: power button device
+ *
+ * Cancel all pending work items for the power button
+ */
+static int palmas_pwron_suspend(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct palmas_pwron *pwron = platform_get_drvdata(pdev);
+
+ cancel_delayed_work_sync(pwron-input_work);
+
+ return 0;
+}
+
+static UNIVERSAL_DEV_PM_OPS(palmas_pwron_pm, palmas_pwron_suspend, NULL, NULL);
Why universal? Do they make sense for runtime pm?
+
+#else
+static UNIVERSAL_DEV_PM_OPS(palmas_pwron_pm, NULL, NULL, NULL);
+#endif
You do not need to protect these with #ifdef and have 2 versions, just pull
UNIVERSAL_DEV_PM_OPS (and change to SIMPLE_DEV_PM_OPS) out of #idef code.
I will just switch over to SIMPLE_DEV_PM_OPS here.. it is better here.
Thanks for the suggestion.
+
+#ifdef CONFIG_OF
+static struct of_device_id of_palmas_pwr_match[] = {
+ {.compatible = ti,palmas-pwrbutton},
+ {},
+};
+
+MODULE_DEVICE_TABLE(of, of_palmas_pwr_match);
+#endif
+
+static struct platform_driver palmas_pwron_driver = {
+ .probe = palmas_pwron_probe,
+ .remove = palmas_pwron_remove,
+ .driver = {
+ .name = palmas_pwrbutton,
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(of_palmas_pwr_match),
+ .pm = palmas_pwron_pm,
+ },
Weird indentation here.
Ugggh.. Lindent.. :(
---
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] Input: misc: introduce palmas-pwrbutton
Many palmas family of PMICs have support for interrupt based power
button. This allows the device to notify the processor of external
push button events over the shared palmas interrupt. However, this
event is generated only during a press operation. Software is
supposed to poll(sigh!) for detecting a release event.
The PMIC also supports ability to power off independent of the
software decisions when the button is pressed for a long duration if
the PMIC is appropriately configured on the platform.
Even though the function is similar to twl4030_pwrbutton, it is
substantially different in operation to belong to a new driver of it's
own.
Based on original work done by Girish S Ghongdemath giris...@ti.com
Signed-off-by: Nishanth Menon n...@ti.com
---
drivers/input/misc/Kconfig| 10 ++
drivers/input/misc/Makefile |1 +
drivers/input/misc/palmas-pwrbutton.c | 314 +
3 files changed, 325 insertions(+)
create mode 100644 drivers/input/misc/palmas-pwrbutton.c
diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 2ff4425..0224dcf 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -451,6 +451,16 @@ config HP_SDC_RTC
Say Y here if you want to support the built-in real time clock
of the HP SDC controller.
+config INPUT_PALMAS_PWRBUTTON
+ tristate Palmas Power button Driver
+ depends on MFD_PALMAS
+ help
+ Say Y here if you want to enable power key reporting via the
+ Palmas family of PMICs.
+
+ To compile this driver as a module, choose M here. The module will
+ be called palmas_pwrbutton.
+
config INPUT_PCF50633_PMU
tristate PCF50633 PMU events
depends on MFD_PCF50633
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index 4955ad3..e3d5efb 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -40,6 +40,7 @@ obj-$(CONFIG_INPUT_MAX8997_HAPTIC)+= max8997_haptic.o
obj-$(CONFIG_INPUT_MC13783_PWRBUTTON) += mc13783-pwrbutton.o
obj-$(CONFIG_INPUT_MMA8450)+= mma8450.o
obj-$(CONFIG_INPUT_MPU3050)+= mpu3050.o
+obj-$(CONFIG_INPUT_PALMAS_PWRBUTTON) += palmas-pwrbutton.o
obj-$(CONFIG_INPUT_PCAP) += pcap_keys.o
obj-$(CONFIG_INPUT_PCF50633_PMU) += pcf50633-input.o
obj-$(CONFIG_INPUT_PCF8574)+= pcf8574_keypad.o
diff --git a/drivers/input/misc/palmas-pwrbutton.c
b/drivers/input/misc/palmas-pwrbutton.c
new file mode 100644
index 000..35f3d68
--- /dev/null
+++ b/drivers/input/misc/palmas-pwrbutton.c
@@ -0,0 +1,314 @@
+/*
+ * Texas Instruments' Palmas Power Button Input Driver
+ *
+ * Copyright (C) 2012-2014 Texas Instruments Incorporated - http://www.ti.com/
+ * Girish S Ghongdemath
+ * Nishanth Menon
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed as is WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+#include linux/init.h
+#include linux/input.h
+#include linux/interrupt.h
+#include linux/kernel.h
+#include linux/mfd/palmas.h
+#include linux/module.h
+#include linux/of.h
+#include linux/platform_device.h
+#include linux/reboot.h
+#include linux/slab.h
+
+#define PALMAS_LPK_TIME_MASK 0x0c
+#define PALMAS_PWR_KEY_PRESS 0x01
+#define PALMAS_PWR_KEY_Q_TIME_MS 20
+
+/**
+ * struct palmas_pwron - Palmas power on data
+ * @palmas:pointer to palmas device
+ * @input_dev: pointer to input device
+ * @irq: irq that we are hooked on to
+ * @input_work:work for detecting release of key
+ * @current_state: key current state
+ * @key_recheck_ms:duration for recheck of key (in milli-seconds)
+ */
+struct palmas_pwron {
+ struct palmas *palmas;
+ struct input_dev *input_dev;
+ int irq;
+ struct delayed_work input_work;
+ int current_state;
+ u32 key_recheck_ms;
+};
+
+/**
+ * struct palmas_pwron_config - configuration of palmas power on
+ * @long_press_time_val: value for long press h/w shutdown event
+ */
+struct palmas_pwron_config {
+ u8 long_press_time_val;
+};
+
+/**
+ * palmas_get_pwr_state() - read button state
+ * @pwron: pointer to pwron struct
+ */
+static int palmas_get_pwr_state(struct palmas_pwron *pwron)
+{
+ struct input_dev *input_dev = pwron-input_dev;
+ struct device *dev = input_dev-dev.parent;
+ unsigned int reg = 0;
+ int ret;
+
+ ret = palmas_read(pwron-palmas, PALMAS_INTERRUPT_BASE,
+ PALMAS_INT1_LINE_STATE, reg);
+ if (ret) {
+ dev_err(dev, %s:Cannot
Re: [PATCH 2/2] Input: misc: introduce palmas-pwrbutton
Hi NIshanth,
On Mon, Aug 18, 2014 at 03:13:30PM -0500, Nishanth Menon wrote:
Many palmas family of PMICs have support for interrupt based power
button. This allows the device to notify the processor of external
push button events over the shared palmas interrupt. However, this
event is generated only during a press operation. Software is
supposed to poll(sigh!) for detecting a release event.
The PMIC also supports ability to power off independent of the
software decisions when the button is pressed for a long duration if
the PMIC is appropriately configured on the platform.
Even though the function is similar to twl4030_pwrbutton, it is
substantially different in operation to belong to a new driver of it's
own.
Based on original work done by Girish S Ghongdemath giris...@ti.com
Signed-off-by: Nishanth Menon n...@ti.com
---
drivers/input/misc/Kconfig| 10 ++
drivers/input/misc/Makefile |1 +
drivers/input/misc/palmas-pwrbutton.c | 314
+
3 files changed, 325 insertions(+)
create mode 100644 drivers/input/misc/palmas-pwrbutton.c
diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 2ff4425..0224dcf 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -451,6 +451,16 @@ config HP_SDC_RTC
Say Y here if you want to support the built-in real time clock
of the HP SDC controller.
+config INPUT_PALMAS_PWRBUTTON
+ tristate Palmas Power button Driver
+ depends on MFD_PALMAS
+ help
+ Say Y here if you want to enable power key reporting via the
+ Palmas family of PMICs.
+
+ To compile this driver as a module, choose M here. The module will
+ be called palmas_pwrbutton.
+
config INPUT_PCF50633_PMU
tristate PCF50633 PMU events
depends on MFD_PCF50633
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index 4955ad3..e3d5efb 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -40,6 +40,7 @@ obj-$(CONFIG_INPUT_MAX8997_HAPTIC) += max8997_haptic.o
obj-$(CONFIG_INPUT_MC13783_PWRBUTTON)+= mc13783-pwrbutton.o
obj-$(CONFIG_INPUT_MMA8450) += mma8450.o
obj-$(CONFIG_INPUT_MPU3050) += mpu3050.o
+obj-$(CONFIG_INPUT_PALMAS_PWRBUTTON) += palmas-pwrbutton.o
obj-$(CONFIG_INPUT_PCAP) += pcap_keys.o
obj-$(CONFIG_INPUT_PCF50633_PMU) += pcf50633-input.o
obj-$(CONFIG_INPUT_PCF8574) += pcf8574_keypad.o
diff --git a/drivers/input/misc/palmas-pwrbutton.c
b/drivers/input/misc/palmas-pwrbutton.c
new file mode 100644
index 000..35f3d68
--- /dev/null
+++ b/drivers/input/misc/palmas-pwrbutton.c
@@ -0,0 +1,314 @@
+/*
+ * Texas Instruments' Palmas Power Button Input Driver
+ *
+ * Copyright (C) 2012-2014 Texas Instruments Incorporated -
http://www.ti.com/
+ * Girish S Ghongdemath
+ * Nishanth Menon
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed as is WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+#include linux/init.h
+#include linux/input.h
+#include linux/interrupt.h
+#include linux/kernel.h
+#include linux/mfd/palmas.h
+#include linux/module.h
+#include linux/of.h
+#include linux/platform_device.h
+#include linux/reboot.h
+#include linux/slab.h
+
+#define PALMAS_LPK_TIME_MASK 0x0c
+#define PALMAS_PWR_KEY_PRESS 0x01
+#define PALMAS_PWR_KEY_Q_TIME_MS 20
+
+/**
+ * struct palmas_pwron - Palmas power on data
+ * @palmas: pointer to palmas device
+ * @input_dev: pointer to input device
+ * @irq: irq that we are hooked on to
+ * @input_work: work for detecting release of key
+ * @current_state: key current state
+ * @key_recheck_ms: duration for recheck of key (in milli-seconds)
+ */
+struct palmas_pwron {
+ struct palmas *palmas;
+ struct input_dev *input_dev;
+ int irq;
+ struct delayed_work input_work;
+ int current_state;
+ u32 key_recheck_ms;
+};
+
+/**
+ * struct palmas_pwron_config - configuration of palmas power on
+ * @long_press_time_val: value for long press h/w shutdown event
+ */
+struct palmas_pwron_config {
+ u8 long_press_time_val;
+};
+
+/**
+ * palmas_get_pwr_state() - read button state
+ * @pwron: pointer to pwron struct
+ */
+static int palmas_get_pwr_state(struct palmas_pwron *pwron)
+{
+ struct input_dev *input_dev = pwron-input_dev;
+ struct device *dev = input_dev-dev.parent;
+ unsigned int reg = 0;
+ int ret;
+
+ ret =
