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-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
