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 <ti...@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
