Hi Dmitry, Thanks for your patience on this one. The Dialog OnKey for DA9062 is a fairly complicated set of interrupts and register read and writes. I've tried to explain the best I can below. On 17 April 2015 17:12, Dmitry Torokhov [mailto:dmitry.torokhov@xxxxxxxxx] wrote: [...] > > diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile > > index 403a1a5..a631283 100644 > > --- a/drivers/input/misc/Makefile > > +++ b/drivers/input/misc/Makefile > > @@ -25,6 +25,7 @@ obj-$(CONFIG_INPUT_CMA3000_I2C) += > cma3000_d0x_i2c.o > > obj-$(CONFIG_INPUT_COBALT_BTNS) += cobalt_btns.o > > obj-$(CONFIG_INPUT_DA9052_ONKEY) += da9052_onkey.o > > obj-$(CONFIG_INPUT_DA9055_ONKEY) += da9055_onkey.o > > +obj-$(CONFIG_INPUT_DA9062_ONKEY) += da9062-onkey.o > > Can we maybe keep the same naming convention for all of these? Also, any > chance all of them or some of them can be combined? Yes. I will rename so it uses an underscore. Unfortunately, the 55 and 52 drivers are not functionally similar. However the DA9063 and DA9062 OnKey drivers *are* functionally similar and so I will make an effort to combine those two drivers in future. I would like to submit DA9063 first and drop this DA9062 until I can combine it. However, since the DA9063 driver is identical -- your comments will be useful for my DA9063 submission attempt. I have added my replies here below which will also relate to DA9063. [...] > > +static void da9062_poll_on(struct work_struct *work) > > +{ > > + struct da9062_onkey *onkey = container_of(work, struct > da9062_onkey, > > + work.work); > > + unsigned int val; > > + int fault_log = 0; > > + bool poll = true; > > + int ret; > > + > > + /* poll to see when the pin is released */ > > + ret = regmap_read(onkey->hw->regmap, DA9062AA_STATUS_A, > &val); > > + if (ret < 0) { > > + dev_err(&onkey->input->dev, > > + "Failed to read ON status: %d\n", ret); > > + goto err_poll; > > + } > > + > > + if (!(val & DA9062AA_NONKEY_MASK)) { > > + ret = regmap_update_bits(onkey->hw->regmap, > > + DA9062AA_CONTROL_B, > > + DA9062AA_NONKEY_LOCK_MASK, > 0); > > + if (ret < 0) { > > + dev_err(&onkey->input->dev, > > + "Failed to reset the Key Delay %d\n", ret); > > + goto err_poll; > > + } > > + > > + input_report_key(onkey->input, KEY_POWER, 0); > > + input_sync(onkey->input); > > + > > + poll = false; > > + } > > + > > + /* if the fault log KEY_RESET is detected, > > + * then clear it and shutdown DA9062 via I2C > > + */ > > + ret = regmap_read(onkey->hw->regmap, DA9062AA_FAULT_LOG, > &fault_log); > > + if (ret < 0) > > + dev_warn(&onkey->input->dev, "Cannot read > FAULT_LOG\n"); > > + > > + if (fault_log & DA9062AA_KEY_RESET_MASK) { > > + ret = regmap_write(onkey->hw->regmap, > > + DA9062AA_FAULT_LOG, > > + DA9062AA_KEY_RESET_MASK); > > + if (ret < 0) > > + dev_warn(&onkey->input->dev, > > + "Cannot reset KEY_RESET fault log\n"); > > + else { > > + /* at this point we do any S/W housekeeping > > + * and then send shutdown command > > + */ > > + dev_info(&onkey->input->dev, > > + "Sending SHUTDOWN to DA9062 ...\n"); > > + ret = regmap_write(onkey->hw->regmap, > > + DA9062AA_CONTROL_F, > > + DA9062AA_SHUTDOWN_MASK); > > + if (ret < 0) > > + dev_err(&onkey->input->dev, > > + "Cannot SHUTDOWN DA9062\n"); > > + } > > + } > > This entire block seems to not belong to the input portion of MFD. Why > do we do it here? > There are four modes of OnKey operation here, but only the first three are handled in software. This block relates to case (3). (1) short press & release -- SLEEP (2) long press & release -- POWER (3) long-long press (no release) -- software power cut (4) long-long press (no release) -- hardware power cut For cases (1) and (2) the OnKey driver will handle the events using the Linux framework. And for case (4) this is completed totally in hardware and is the emergency power off for holding down the OnKey until the power is cut by the PMIC -- this is not handled here and in any case it is designed to be used when the software is not responding: H/W shutdown There is one more case and this is covered by point (3). This is when the user holds down the OnKey as though they were attempting an emergency power off. This is the same operation as point (4) except that software *is* still able to respond. At this point it sends the power off signal to the PMIC in a similar way as the hardware power cut operation would do -- but it does this 1 second before the hardware has chance to step-in and force the PMIC to go down. There are several reasons for case (3). - It is to cover the usecase of a user pressing the OnKey until the device goes off as their main way of turning the power off to their device. This software power cut allows any critical house-keeping to be completed before the power switch is flicked and so it mitigates the possibility for loss of any data. - It also ensures the FAULT_LOG is cleared. The FAULT_LOG is a persistent register in the PMIC and holds its information between resets -- in this case the reason the device was powered off. In case (4) If the software was not responsive, then the hardware shutdown would happen 1 second later and the FAULT_LOG would persist enough information to be able to tell the difference between a software power cut and a hardware power cut when the device was restarted. If the software had not responded, then due to the FAULT_LOG persistence, it would be possible to take action the next time the device was turned on again (maybe in the bootloader -- e.g. say to complete memory checks or put the device into a safe mode). > > + > > +err_poll: > > + if (poll) > > + schedule_delayed_work(&onkey->work, 50); > > +} > > + > > +static irqreturn_t da9062_onkey_irq_handler(int irq, void *data) > > +{ > > + struct da9062_onkey *onkey = data; > > + unsigned int val; > > + int ret; > > + > > + ret = regmap_read(onkey->hw->regmap, DA9062AA_STATUS_A, > &val); > > + if (onkey->key_power && (ret >= 0) && (val & > DA9062AA_NONKEY_MASK)) { > > + input_report_key(onkey->input, KEY_POWER, 1); > > + input_sync(onkey->input); > > + schedule_delayed_work(&onkey->work, 0); > > + dev_notice(&onkey->input->dev, "KEY_POWER > pressed.\n"); > > + } else { > > + input_report_key(onkey->input, KEY_SLEEP, 1); > > + input_sync(onkey->input); > > + input_report_key(onkey->input, KEY_SLEEP, 0); > > + input_sync(onkey->input); > > + dev_notice(&onkey->input->dev, "KEY_SLEEP pressed.\n"); > > + } > > Why do we handle KEY_POWER and KEY_SLEEP completely differently? > The reason for the keys being handled differently is due to the way with the interrupt is being triggered differently in the OnKey. This depends on whether the key-press is a short (SLEEP) or a long (POWER) key-press. For case (1) (1) short-press & release -- SLEEP The *release* of the OnKey is the trigger of the interrupt. So we know the device has a press-release operation. This was the intention of the two key reports, one for press and the other for release. There is no way to detect the key press here, only the key release, but both must have happened. input_report_key(onkey->input, KEY_SLEEP, 1); input_report_key(onkey->input, KEY_SLEEP, 0); For the next case (2) (2) long-press & release -- POWER The key is pressed but not released for a period of time (say longer than 2 seconds), and once this time-limit has been passed without the key being released an interrupt is triggered. This is different to case (1) in that the key release was not the trigger of the interrupt, it was a time-out trip-line -- hence the single key report, because at this point we know that the key was pressed, just not released yet. input_report_key(onkey->input, KEY_POWER, 1); The second key report is triggered when the key is released, and this can only be detected during a polling operation and examination of the register DA9063_REG_STATUS_A. Once the key release is detected, then the second key report is sent out input_report_key(onkey->input, KEY_SLEEP, 0); This last operation happens inside the da9063_poll_on() function. It is this polling function that also detects for the key never being released and so is also able to handle case (3) (3) long-long press (no release) -- software power cut [...] > > +static int da9062_onkey_probe(struct platform_device *pdev) > > +{ > > + struct da9062 *chip = dev_get_drvdata(pdev->dev.parent); > > + struct da9062_onkey *onkey; > > + bool kp_tmp = true; > > + int ret = 0; > > + > > + kp_tmp = of_property_read_bool((&pdev->dev)->of_node, > > + "dlg,disable-key-power"); > > + kp_tmp = !kp_tmp; > > Should we just allow specifying the keycode instead of hardcoding > KEY_POWER/KEY_SLEEP? > The reason I have hard-coded the key-power and key-sleep is because this is the configuration written into the default PMIC one-time-programming registers. Perhaps I am not quite understanding your question properly here. [...] > > +static int da9062_onkey_remove(struct platform_device *pdev) > > +{ > > + struct da9062_onkey *onkey = platform_get_drvdata(pdev); > > + > > + free_irq(onkey->irq, onkey); > > + cancel_delayed_work_sync(&onkey->work); > > + input_unregister_device(onkey->input); > > No need to unregister explicitly if you allocated input device with > devm. > Yep. I can alter this by removing the function input_unregister_device() as described. So, just to finish this off. I would like to reiterate that I am going to drop this patch from my next submission attempt of the DA9062. However because the DA9063 OnKey component is functionally identical, the comments from above will be used to modify my DA9063 driver submission. http://www.spinics.net/lists/linux-input/msg38034.html http://www.spinics.net/lists/linux-input/msg38137.html Regards, Steve -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
