| Message ID | 20170822055710.26515-3-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2017-08-22 at 07:57 +0200, Takashi Iwai wrote: > This provides a new input driver for supporting the power button on > Dollar Cove TI PMIC, found on Cherrytrail-based devices. > The patch is based on the original work by Intel, found at: > https://github.com/01org/ProductionKernelQuilts For now we have similar driver under drivers/platform/x86. I think when we move to regmap API there we might even reuse intel_mid_powerbtn.c. Or other way around, move that one to input subsystem (which I personally see less fit).
On Tue, 22 Aug 2017 11:42:08 +0200, Andy Shevchenko wrote: > > On Tue, 2017-08-22 at 07:57 +0200, Takashi Iwai wrote: > > This provides a new input driver for supporting the power button on > > Dollar Cove TI PMIC, found on Cherrytrail-based devices. > > The patch is based on the original work by Intel, found at: > > https://github.com/01org/ProductionKernelQuilts > > For now we have similar driver under drivers/platform/x86. > > I think when we move to regmap API there we might even reuse > intel_mid_powerbtn.c. Ah, OK, that can be a better place. I'll move to there. Also, from comparison with intel_mid_powerbtn.c, I noticed that the irq wake isn't cleared at removal. Will address it as well. > Or other way around, move that one to input subsystem (which I > personally see less fit). I don't think it's worth, either. I updated the patches and now pushed to topic/dollar-cove-ti-4.13-v2 branch. Will resubmit v2 (tomorrow or later) once after gathering reviews. thanks, Takashi -- 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 Tue, Aug 22, 2017 at 12:58:07PM +0200, Takashi Iwai wrote: > I updated the patches and now pushed to topic/dollar-cove-ti-4.13-v2 > branch. Will resubmit v2 (tomorrow or later) once after gathering > reviews. FWIW I tested current Linus's master + topic/dollar-cove-ti-4.13-v2 + topic/soc-cx2072x-4.13 + my test patches, no observable difference to topic/dollar-cove-ti-4.13 on E200HA. Still hoping someone would give me a hint about possible causes for the SoC entering S0i1 only instead of S0i3? (https://bugzilla.kernel.org/show_bug.cgi?id=193891) Where do I start looking? Thanks, Johannes -- 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 Takashi, On Tue, Aug 22, 2017 at 07:57:09AM +0200, Takashi Iwai wrote: > This provides a new input driver for supporting the power button on > Dollar Cove TI PMIC, found on Cherrytrail-based devices. > The patch is based on the original work by Intel, found at: > https://github.com/01org/ProductionKernelQuilts > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=193891 > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- > drivers/input/keyboard/Kconfig | 7 +++ > drivers/input/keyboard/Makefile | 1 + > drivers/input/keyboard/dc_ti_pwrbtn.c | 85 +++++++++++++++++++++++++++++++++++ Not sure if it ends up in drivers/input/keyboard, or drivers/input/misc/ (where most power buttons live) or in platform drivers, still a few comments below. > 3 files changed, 93 insertions(+) > create mode 100644 drivers/input/keyboard/dc_ti_pwrbtn.c > > diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig > index 4c4ab1ced235..673748b3cc34 100644 > --- a/drivers/input/keyboard/Kconfig > +++ b/drivers/input/keyboard/Kconfig > @@ -756,4 +756,11 @@ config KEYBOARD_BCM > To compile this driver as a module, choose M here: the > module will be called bcm-keypad. > > +config KEYBOARD_DC_TI_PWRBTN > + tristate "Dollar Cove TI power button driver" > + depends on INTEL_SOC_PMIC_DC_TI > + help > + Say Y here fi you want to have a power button driver for > + Dollar Cove TI PMIC. If keeping in input we customarily call out the module name (see a few lines above). > + > endif > diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile > index d2338bacdad1..fa473d241e5c 100644 > --- a/drivers/input/keyboard/Makefile > +++ b/drivers/input/keyboard/Makefile > @@ -66,3 +66,4 @@ obj-$(CONFIG_KEYBOARD_TM2_TOUCHKEY) += tm2-touchkey.o > obj-$(CONFIG_KEYBOARD_TWL4030) += twl4030_keypad.o > obj-$(CONFIG_KEYBOARD_XTKBD) += xtkbd.o > obj-$(CONFIG_KEYBOARD_W90P910) += w90p910_keypad.o > +obj-$(CONFIG_KEYBOARD_DC_TI_PWRBTN) += dc_ti_pwrbtn.o > diff --git a/drivers/input/keyboard/dc_ti_pwrbtn.c b/drivers/input/keyboard/dc_ti_pwrbtn.c > new file mode 100644 > index 000000000000..a0900a440c92 > --- /dev/null > +++ b/drivers/input/keyboard/dc_ti_pwrbtn.c > @@ -0,0 +1,85 @@ > +/* > + * Power button driver for Dollar Cove TI PMIC > + * Copyright (C) 2014 Intel Corp > + * Copyright (c) 2017 Takashi Iwai <tiwai@suse.de> > + */ > + > +#include <linux/module.h> > +#include <linux/init.h> > +#include <linux/interrupt.h> > +#include <linux/slab.h> > +#include <linux/device.h> > +#include <linux/platform_device.h> > +#include <linux/input.h> > +#include <linux/mfd/intel_soc_pmic.h> > + > +#define DC_TI_SIRQ_REG 0x3 > +#define SIRQ_PWRBTN_REL (1 << 0) BIT()? > + > +#define DRIVER_NAME "dc_ti_pwrbtn" > + > +static irqreturn_t dc_ti_pwrbtn_interrupt(int irq, void *dev_id) > +{ > + struct input_dev *pwrbtn_input = dev_id; > + struct device *dev = pwrbtn_input->dev.parent; > + struct regmap *regmap = dev_get_drvdata(dev); > + int state; > + > + if (!regmap_read(regmap, DC_TI_SIRQ_REG, &state)) { > + dev_dbg(dev, "SIRQ_REG=0x%x\n", state); > + state &= SIRQ_PWRBTN_REL; > + input_event(pwrbtn_input, EV_KEY, KEY_POWER, !state); Why not input_report_key(pwrbtn_input, KEY_POWER, !(state & SIRQ_PWRBTN_REL)); > + input_sync(pwrbtn_input); > + } > + > + return IRQ_HANDLED; > +} > + > +static int dc_ti_pwrbtn_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct intel_soc_pmic *pmic = dev_get_drvdata(dev->parent); > + struct input_dev *pwrbtn_input; > + int irq; > + int ret; > + > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) > + return -EINVAL; Why do you clobber return value? Simply return "irq". > + pwrbtn_input = devm_input_allocate_device(dev); > + if (!pwrbtn_input) > + return -ENOMEM; > + pwrbtn_input->name = pdev->name; > + pwrbtn_input->phys = "dc-ti-power/input0"; > + pwrbtn_input->id.bustype = BUS_HOST; > + pwrbtn_input->dev.parent = dev; Not needed since devm_input_allocate_device() does it for us. > + input_set_capability(pwrbtn_input, EV_KEY, KEY_POWER); > + ret = input_register_device(pwrbtn_input); > + if (ret) > + return ret; If staying in input, can we please call this variable err or error? > + > + dev_set_drvdata(dev, pmic->regmap); > + > + ret = devm_request_threaded_irq(dev, irq, NULL, dc_ti_pwrbtn_interrupt, > + 0, KBUILD_MODNAME, pwrbtn_input); > + if (ret) > + return ret; > + > + ret = enable_irq_wake(irq); > + if (ret) > + dev_warn(dev, "Can't enable IRQ as wake source: %d\n", ret); We do not normally enable wake IRQs in probe, but instead do: device_init_wakeup(&pdev->dev, true); in probe() and then check it in suspend/resume: if (device_may_wakeup(dev)) { err = enable_irq_wake(XXX->irq); if (!err) XXX->irq_wake_enabled = true; } ... if (XXX->irq_wake_enabled) disable_irq_wake(XXX->irq); This allows userspace to inhibit wakeup, if needed. > + > + return 0; > +} > + > +static struct platform_driver dc_ti_pwrbtn_driver = { > + .driver = { > + .name = DRIVER_NAME, > + }, > + .probe = dc_ti_pwrbtn_probe, > +}; > + > +module_platform_driver(dc_ti_pwrbtn_driver); > + > +MODULE_LICENSE("GPL v2"); > +MODULE_ALIAS("platform:" DRIVER_NAME); > -- > 2.14.0 > Thanks!
On Thu, 31 Aug 2017 20:33:55 +0200, Dmitry Torokhov wrote: > > Hi Takashi, > > On Tue, Aug 22, 2017 at 07:57:09AM +0200, Takashi Iwai wrote: > > This provides a new input driver for supporting the power button on > > Dollar Cove TI PMIC, found on Cherrytrail-based devices. > > The patch is based on the original work by Intel, found at: > > https://github.com/01org/ProductionKernelQuilts > > > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=193891 > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > --- > > drivers/input/keyboard/Kconfig | 7 +++ > > drivers/input/keyboard/Makefile | 1 + > > drivers/input/keyboard/dc_ti_pwrbtn.c | 85 +++++++++++++++++++++++++++++++++++ > > Not sure if it ends up in drivers/input/keyboard, or drivers/input/misc/ > (where most power buttons live) or in platform drivers, still a few > comments below. Oh sorry, I forgot to put you to Cc after v2 patch. Per Andy's suggestion, the driver was moved to drivers/platform/x86 now. For v3 patchset, see the following thread. https://marc.info/?i=20170825134443.14843-1-tiwai%40suse.de > > +config KEYBOARD_DC_TI_PWRBTN > > + tristate "Dollar Cove TI power button driver" > > + depends on INTEL_SOC_PMIC_DC_TI > > + help > > + Say Y here fi you want to have a power button driver for > > + Dollar Cove TI PMIC. > > If keeping in input we customarily call out the module name (see a few > lines above). I changed the driver and config names to follow the platform driver. > > +#define DC_TI_SIRQ_REG 0x3 > > +#define SIRQ_PWRBTN_REL (1 << 0) > > BIT()? OK. > > +#define DRIVER_NAME "dc_ti_pwrbtn" > > + > > +static irqreturn_t dc_ti_pwrbtn_interrupt(int irq, void *dev_id) > > +{ > > + struct input_dev *pwrbtn_input = dev_id; > > + struct device *dev = pwrbtn_input->dev.parent; > > + struct regmap *regmap = dev_get_drvdata(dev); > > + int state; > > + > > + if (!regmap_read(regmap, DC_TI_SIRQ_REG, &state)) { > > + dev_dbg(dev, "SIRQ_REG=0x%x\n", state); > > + state &= SIRQ_PWRBTN_REL; > > + input_event(pwrbtn_input, EV_KEY, KEY_POWER, !state); > > Why not > > input_report_key(pwrbtn_input, KEY_POWER, > !(state & SIRQ_PWRBTN_REL)); Sounds better, indeed. > > + input_sync(pwrbtn_input); > > + } > > + > > + return IRQ_HANDLED; > > +} > > + > > +static int dc_ti_pwrbtn_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct intel_soc_pmic *pmic = dev_get_drvdata(dev->parent); > > + struct input_dev *pwrbtn_input; > > + int irq; > > + int ret; > > + > > + irq = platform_get_irq(pdev, 0); > > + if (irq < 0) > > + return -EINVAL; > > Why do you clobber return value? Simply return "irq". OK. > > + pwrbtn_input = devm_input_allocate_device(dev); > > + if (!pwrbtn_input) > > + return -ENOMEM; > > + pwrbtn_input->name = pdev->name; > > + pwrbtn_input->phys = "dc-ti-power/input0"; > > + pwrbtn_input->id.bustype = BUS_HOST; > > + pwrbtn_input->dev.parent = dev; > > Not needed since devm_input_allocate_device() does it for us. OK. > > + input_set_capability(pwrbtn_input, EV_KEY, KEY_POWER); > > + ret = input_register_device(pwrbtn_input); > > + if (ret) > > + return ret; > > If staying in input, can we please call this variable err or error? OK, I don't mind to change it. > > + > > + dev_set_drvdata(dev, pmic->regmap); > > + > > + ret = devm_request_threaded_irq(dev, irq, NULL, dc_ti_pwrbtn_interrupt, > > + 0, KBUILD_MODNAME, pwrbtn_input); > > + if (ret) > > + return ret; > > + > > + ret = enable_irq_wake(irq); > > + if (ret) > > + dev_warn(dev, "Can't enable IRQ as wake source: %d\n", ret); > > We do not normally enable wake IRQs in probe, but instead do: > > device_init_wakeup(&pdev->dev, true); > in probe() Right, this was already changed in the later version. But... > and then check it in suspend/resume: > > if (device_may_wakeup(dev)) { > err = enable_irq_wake(XXX->irq); > if (!err) > XXX->irq_wake_enabled = true; > } > > ... > > if (XXX->irq_wake_enabled) > disable_irq_wake(XXX->irq); > > This allows userspace to inhibit wakeup, if needed. ... this wasn't. Will put it. Thank your for the review! Takashi -- 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 Thu, 2017-08-31 at 11:33 -0700, Dmitry Torokhov wrote: > > + > > + ret = enable_irq_wake(irq); > > + if (ret) > > + dev_warn(dev, "Can't enable IRQ as wake source: > > %d\n", ret); > > We do not normally enable wake IRQs in probe, but instead do: > > device_init_wakeup(&pdev->dev, true); > > in probe() and then check it in suspend/resume: > > if (device_may_wakeup(dev)) { > err = enable_irq_wake(XXX->irq); > if (!err) > XXX->irq_wake_enabled = true; > } > > ... > > if (XXX->irq_wake_enabled) No need to duplicate a flag which IRQ core already has. See, for example, commit aef3ad103a68 ("serial: core: remove unneeded irq_wake flag") > disable_irq_wake(XXX->irq); > > This allows userspace to inhibit wakeup, if needed.
On Fri, 01 Sep 2017 13:09:36 +0200, Andy Shevchenko wrote: > > On Thu, 2017-08-31 at 11:33 -0700, Dmitry Torokhov wrote: > > > > + > > > + ret = enable_irq_wake(irq); > > > + if (ret) > > > + dev_warn(dev, "Can't enable IRQ as wake source: > > > %d\n", ret); > > > > We do not normally enable wake IRQs in probe, but instead do: > > > > device_init_wakeup(&pdev->dev, true); > > > > in probe() and then check it in suspend/resume: > > > > if (device_may_wakeup(dev)) { > > err = enable_irq_wake(XXX->irq); > > if (!err) > > XXX->irq_wake_enabled = true; > > } > > > > ... > > > > if (XXX->irq_wake_enabled) > > No need to duplicate a flag which IRQ core already has. > > See, for example, commit > aef3ad103a68 ("serial: core: remove unneeded irq_wake flag") I couldn't find the commit, but checked the web search. So everything is set up and done by dev_pm_set_wake_irq(), no need to create the own PM ops just for that, right? That stuff was already in v3 patch. thanks, Takashi -- 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
diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig index 4c4ab1ced235..673748b3cc34 100644 --- a/drivers/input/keyboard/Kconfig +++ b/drivers/input/keyboard/Kconfig @@ -756,4 +756,11 @@ config KEYBOARD_BCM To compile this driver as a module, choose M here: the module will be called bcm-keypad. +config KEYBOARD_DC_TI_PWRBTN + tristate "Dollar Cove TI power button driver" + depends on INTEL_SOC_PMIC_DC_TI + help + Say Y here fi you want to have a power button driver for + Dollar Cove TI PMIC. + endif diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile index d2338bacdad1..fa473d241e5c 100644 --- a/drivers/input/keyboard/Makefile +++ b/drivers/input/keyboard/Makefile @@ -66,3 +66,4 @@ obj-$(CONFIG_KEYBOARD_TM2_TOUCHKEY) += tm2-touchkey.o obj-$(CONFIG_KEYBOARD_TWL4030) += twl4030_keypad.o obj-$(CONFIG_KEYBOARD_XTKBD) += xtkbd.o obj-$(CONFIG_KEYBOARD_W90P910) += w90p910_keypad.o +obj-$(CONFIG_KEYBOARD_DC_TI_PWRBTN) += dc_ti_pwrbtn.o diff --git a/drivers/input/keyboard/dc_ti_pwrbtn.c b/drivers/input/keyboard/dc_ti_pwrbtn.c new file mode 100644 index 000000000000..a0900a440c92 --- /dev/null +++ b/drivers/input/keyboard/dc_ti_pwrbtn.c @@ -0,0 +1,85 @@ +/* + * Power button driver for Dollar Cove TI PMIC + * Copyright (C) 2014 Intel Corp + * Copyright (c) 2017 Takashi Iwai <tiwai@suse.de> + */ + +#include <linux/module.h> +#include <linux/init.h> +#include <linux/interrupt.h> +#include <linux/slab.h> +#include <linux/device.h> +#include <linux/platform_device.h> +#include <linux/input.h> +#include <linux/mfd/intel_soc_pmic.h> + +#define DC_TI_SIRQ_REG 0x3 +#define SIRQ_PWRBTN_REL (1 << 0) + +#define DRIVER_NAME "dc_ti_pwrbtn" + +static irqreturn_t dc_ti_pwrbtn_interrupt(int irq, void *dev_id) +{ + struct input_dev *pwrbtn_input = dev_id; + struct device *dev = pwrbtn_input->dev.parent; + struct regmap *regmap = dev_get_drvdata(dev); + int state; + + if (!regmap_read(regmap, DC_TI_SIRQ_REG, &state)) { + dev_dbg(dev, "SIRQ_REG=0x%x\n", state); + state &= SIRQ_PWRBTN_REL; + input_event(pwrbtn_input, EV_KEY, KEY_POWER, !state); + input_sync(pwrbtn_input); + } + + return IRQ_HANDLED; +} + +static int dc_ti_pwrbtn_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct intel_soc_pmic *pmic = dev_get_drvdata(dev->parent); + struct input_dev *pwrbtn_input; + int irq; + int ret; + + irq = platform_get_irq(pdev, 0); + if (irq < 0) + return -EINVAL; + pwrbtn_input = devm_input_allocate_device(dev); + if (!pwrbtn_input) + return -ENOMEM; + pwrbtn_input->name = pdev->name; + pwrbtn_input->phys = "dc-ti-power/input0"; + pwrbtn_input->id.bustype = BUS_HOST; + pwrbtn_input->dev.parent = dev; + input_set_capability(pwrbtn_input, EV_KEY, KEY_POWER); + ret = input_register_device(pwrbtn_input); + if (ret) + return ret; + + dev_set_drvdata(dev, pmic->regmap); + + ret = devm_request_threaded_irq(dev, irq, NULL, dc_ti_pwrbtn_interrupt, + 0, KBUILD_MODNAME, pwrbtn_input); + if (ret) + return ret; + + ret = enable_irq_wake(irq); + if (ret) + dev_warn(dev, "Can't enable IRQ as wake source: %d\n", ret); + + return 0; +} + +static struct platform_driver dc_ti_pwrbtn_driver = { + .driver = { + .name = DRIVER_NAME, + }, + .probe = dc_ti_pwrbtn_probe, +}; + +module_platform_driver(dc_ti_pwrbtn_driver); + +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:" DRIVER_NAME);
This provides a new input driver for supporting the power button on Dollar Cove TI PMIC, found on Cherrytrail-based devices. The patch is based on the original work by Intel, found at: https://github.com/01org/ProductionKernelQuilts Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=193891 Signed-off-by: Takashi Iwai <tiwai@suse.de> --- drivers/input/keyboard/Kconfig | 7 +++ drivers/input/keyboard/Makefile | 1 + drivers/input/keyboard/dc_ti_pwrbtn.c | 85 +++++++++++++++++++++++++++++++++++ 3 files changed, 93 insertions(+) create mode 100644 drivers/input/keyboard/dc_ti_pwrbtn.c