[v2,3/3] dt-bindings: Input: da9062 - fix dlg,disable-key-power description
diff mbox series

Message ID 20191127132304.22924-4-m.felsch@pengutronix.de
State Changes Requested
Headers show
Series
  • DA9063 Onkey Improvements and Fixes
Related show

Checks

Context Check Description
robh/checkpatch success

Commit Message

Marco Felsch Nov. 27, 2019, 1:23 p.m. UTC
There was a bug within the driver since commit f889beaaab1c ("Input:
da9063 - report KEY_POWER instead of KEY_SLEEP during power
key-press"). Since we fixed the bug the KEY_POWER will be reported
always so we need to adapt the dt-bindings too. Make the description
more precise while on it.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
v2:
- change description according Dmitry's suggestion.

 Documentation/devicetree/bindings/input/da9062-onkey.txt | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Adam Thomson Dec. 2, 2019, 12:15 p.m. UTC | #1
On 27 November 2019 13:23, Marco Felsch wrote:

> There was a bug within the driver since commit f889beaaab1c ("Input:
> da9063 - report KEY_POWER instead of KEY_SLEEP during power
> key-press"). Since we fixed the bug the KEY_POWER will be reported
> always so we need to adapt the dt-bindings too. Make the description
> more precise while on it.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> v2:
> - change description according Dmitry's suggestion.
> 
>  Documentation/devicetree/bindings/input/da9062-onkey.txt | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/input/da9062-onkey.txt
> b/Documentation/devicetree/bindings/input/da9062-onkey.txt
> index 0005b2bdcdd7..9f895454179d 100644
> --- a/Documentation/devicetree/bindings/input/da9062-onkey.txt
> +++ b/Documentation/devicetree/bindings/input/da9062-onkey.txt
> @@ -15,9 +15,8 @@ Required properties:
> 
>  Optional properties:
> 
> -- dlg,disable-key-power : Disable power-down using a long key-press. If this
> -    entry exists the OnKey driver will remove support for the KEY_POWER key
> -    press when triggered using a long press of the OnKey.
> +- dlg,disable-key-power : If this property is present, the host will not be
> +    issuing shutdown command over I2C in response to a long key-press.

This also changes behaviour of button press reporting as the driver will not
report a longer press (i.e. a button hold where the driver polls for release).
It will only report a short key press to user-space with this property provided.

The question here is do we still want to support long press reporting but
without the I2C sequence for shutdown? If so the driver needs to be updated to
work this way as right now it doesn't.

> 
>  - dlg,key-opmode : Set the nONKEY behaviour. This value is initial set by the
>      otp values. See nONKEY_PIN register description for more information.
> --
> 2.20.1
Rob Herring Dec. 5, 2019, 9:38 p.m. UTC | #2
On Wed, Nov 27, 2019 at 02:23:04PM +0100, Marco Felsch wrote:
> There was a bug within the driver since commit f889beaaab1c ("Input:
> da9063 - report KEY_POWER instead of KEY_SLEEP during power
> key-press"). Since we fixed the bug the KEY_POWER will be reported
> always so we need to adapt the dt-bindings too. Make the description
> more precise while on it.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> v2:
> - change description according Dmitry's suggestion.
> 
>  Documentation/devicetree/bindings/input/da9062-onkey.txt | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/input/da9062-onkey.txt b/Documentation/devicetree/bindings/input/da9062-onkey.txt
> index 0005b2bdcdd7..9f895454179d 100644
> --- a/Documentation/devicetree/bindings/input/da9062-onkey.txt
> +++ b/Documentation/devicetree/bindings/input/da9062-onkey.txt
> @@ -15,9 +15,8 @@ Required properties:
>  
>  Optional properties:
>  
> -- dlg,disable-key-power : Disable power-down using a long key-press. If this
> -    entry exists the OnKey driver will remove support for the KEY_POWER key
> -    press when triggered using a long press of the OnKey.
> +- dlg,disable-key-power : If this property is present, the host will not be
> +    issuing shutdown command over I2C in response to a long key-press.

This seems odd. Typically a long key press is used when the system is 
locked up and would have to be a h/w mechanism. 

Also, the new wording sounds like s/w policy, not h/w configuration. 
What the OS does in response to KEY_POWER doesn't belong in DT.

>  
>  - dlg,key-opmode : Set the nONKEY behaviour. This value is initial set by the
>      otp values. See nONKEY_PIN register description for more information.
> -- 
> 2.20.1
>
Marco Felsch Dec. 10, 2019, 8:27 a.m. UTC | #3
Hi Adam,

On 19-12-02 12:15, Adam Thomson wrote:
> On 27 November 2019 13:23, Marco Felsch wrote:
> 
> > There was a bug within the driver since commit f889beaaab1c ("Input:
> > da9063 - report KEY_POWER instead of KEY_SLEEP during power
> > key-press"). Since we fixed the bug the KEY_POWER will be reported
> > always so we need to adapt the dt-bindings too. Make the description
> > more precise while on it.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> > v2:
> > - change description according Dmitry's suggestion.
> > 
> >  Documentation/devicetree/bindings/input/da9062-onkey.txt | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/input/da9062-onkey.txt
> > b/Documentation/devicetree/bindings/input/da9062-onkey.txt
> > index 0005b2bdcdd7..9f895454179d 100644
> > --- a/Documentation/devicetree/bindings/input/da9062-onkey.txt
> > +++ b/Documentation/devicetree/bindings/input/da9062-onkey.txt
> > @@ -15,9 +15,8 @@ Required properties:
> > 
> >  Optional properties:
> > 
> > -- dlg,disable-key-power : Disable power-down using a long key-press. If this
> > -    entry exists the OnKey driver will remove support for the KEY_POWER key
> > -    press when triggered using a long press of the OnKey.
> > +- dlg,disable-key-power : If this property is present, the host will not be
> > +    issuing shutdown command over I2C in response to a long key-press.
> 
> This also changes behaviour of button press reporting as the driver will not
> report a longer press (i.e. a button hold where the driver polls for release).
> It will only report a short key press to user-space with this property provided.
> 
> The question here is do we still want to support long press reporting but
> without the I2C sequence for shutdown? If so the driver needs to be updated to
> work this way as right now it doesn't.

Good point. I checked the driver and the documentation for the
da9062/3 again and it seems that we interrupt the pmic hw by doing the
shutdown by itself. As the documentation says:

DA9063:
If the hardware reset was initiated by a (debounced) press of nONKEY (or
GPIO14 and GPIO15 together) longer than SD_DELAY, the DA9063 initially
only asserts control bit KEY_RESET in the fault register and signals a
non-maskable interrupt allowing the host to clear the armed reset
sequence within 1 s. If the host does not clear KEY_RESET then a
shutdown to RESET mode is executed. KEY_SD_MODE determines if normal
power sequence timing or a fast shutdown is implemented.

DA9062:
If the reset was initiated by a user’s long press of nONKEY, initially
only KEY_RESET is set and the nIRQ port will be asserted. KEY_RESET
signals the host that a shutdown sequence is started. If the host does
not then clear KEY_RESET within 1 s by writing a 1 to the related bit in
register FAULT_LOG, the shutdown sequence will complete. When the reset
condition has disappeared, DA9062 requires a supply (VSYS >
VDD_FAULT_UPPER) that provides enough power to start-up from the
POWERDOWN mode.

So we don't need to check for the KEY_RESET? This would cleanup the code
a bit.

Regards,
  Marco 

> > 
> >  - dlg,key-opmode : Set the nONKEY behaviour. This value is initial set by the
> >      otp values. See nONKEY_PIN register description for more information.
> > --
> > 2.20.1
> 
>
Adam Thomson Dec. 12, 2019, 10:08 a.m. UTC | #4
On 10 December 2019 08:28, Marco Felsch wrote:

> Hi Adam,
>
> On 19-12-02 12:15, Adam Thomson wrote:
> > On 27 November 2019 13:23, Marco Felsch wrote:
> >
> > > There was a bug within the driver since commit f889beaaab1c ("Input:
> > > da9063 - report KEY_POWER instead of KEY_SLEEP during power
> > > key-press"). Since we fixed the bug the KEY_POWER will be reported
> > > always so we need to adapt the dt-bindings too. Make the description
> > > more precise while on it.
> > >
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > ---
> > > v2:
> > > - change description according Dmitry's suggestion.
> > >
> > >  Documentation/devicetree/bindings/input/da9062-onkey.txt | 5 ++---
> > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/input/da9062-onkey.txt
> > > b/Documentation/devicetree/bindings/input/da9062-onkey.txt
> > > index 0005b2bdcdd7..9f895454179d 100644
> > > --- a/Documentation/devicetree/bindings/input/da9062-onkey.txt
> > > +++ b/Documentation/devicetree/bindings/input/da9062-onkey.txt
> > > @@ -15,9 +15,8 @@ Required properties:
> > >
> > >  Optional properties:
> > >
> > > -- dlg,disable-key-power : Disable power-down using a long key-press. If this
> > > -    entry exists the OnKey driver will remove support for the KEY_POWER key
> > > -    press when triggered using a long press of the OnKey.
> > > +- dlg,disable-key-power : If this property is present, the host will not be
> > > +    issuing shutdown command over I2C in response to a long key-press.
> >
> > This also changes behaviour of button press reporting as the driver will not
> > report a longer press (i.e. a button hold where the driver polls for release).
> > It will only report a short key press to user-space with this property provided.
> >
> > The question here is do we still want to support long press reporting but
> > without the I2C sequence for shutdown? If so the driver needs to be updated
> to
> > work this way as right now it doesn't.
>
> Good point. I checked the driver and the documentation for the
> da9062/3 again and it seems that we interrupt the pmic hw by doing the
> shutdown by itself. As the documentation says:
>
> DA9063:
> If the hardware reset was initiated by a (debounced) press of nONKEY (or
> GPIO14 and GPIO15 together) longer than SD_DELAY, the DA9063 initially
> only asserts control bit KEY_RESET in the fault register and signals a
> non-maskable interrupt allowing the host to clear the armed reset
> sequence within 1 s. If the host does not clear KEY_RESET then a
> shutdown to RESET mode is executed. KEY_SD_MODE determines if normal
> power sequence timing or a fast shutdown is implemented.
>
> DA9062:
> If the reset was initiated by a user’s long press of nONKEY, initially
> only KEY_RESET is set and the nIRQ port will be asserted. KEY_RESET
> signals the host that a shutdown sequence is started. If the host does
> not then clear KEY_RESET within 1 s by writing a 1 to the related bit in
> register FAULT_LOG, the shutdown sequence will complete. When the reset
> condition has disappeared, DA9062 requires a supply (VSYS >
> VDD_FAULT_UPPER) that provides enough power to start-up from the
> POWERDOWN mode.
>
> So we don't need to check for the KEY_RESET? This would cleanup the code
> a bit.

We could remove that but I think the intention originally was to have this code
there in case there was some way to more gracefully shutdown the system in this
scenario. Right now that's not the case as the code simply calls to shutdown
the pmic via a manual register call, but maybe in the future this could be made
more gracful. Another slight advantage of leaving that code in is that we're
not waiting for another second delay for the PMIC to pull the plug.

With regards to this patch and the subsequent one to update the OnKey driver, I
think we should deprecate this property. The OnKey mode binding you added in
a different patch will configure the OnKey according to how you want it to
behave in hardware so I don't think this property makes much sense anymore. We
can then remove the need for 'key_power' usage in the driver.

>
> Regards,
>   Marco
>
> > >
> > >  - dlg,key-opmode : Set the nONKEY behaviour. This value is initial set by the
> > >      otp values. See nONKEY_PIN register description for more information.
> > > --
> > > 2.20.1
> >
> >
>
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
Marco Felsch Jan. 7, 2020, 8:08 a.m. UTC | #5
Hi Adam, Dmitry,

On 19-12-12 10:08, Adam Thomson wrote:
> On 10 December 2019 08:28, Marco Felsch wrote:
> 
> > Hi Adam,
> >
> > On 19-12-02 12:15, Adam Thomson wrote:
> > > On 27 November 2019 13:23, Marco Felsch wrote:
> > >
> > > > There was a bug within the driver since commit f889beaaab1c ("Input:
> > > > da9063 - report KEY_POWER instead of KEY_SLEEP during power
> > > > key-press"). Since we fixed the bug the KEY_POWER will be reported
> > > > always so we need to adapt the dt-bindings too. Make the description
> > > > more precise while on it.
> > > >
> > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > ---
> > > > v2:
> > > > - change description according Dmitry's suggestion.
> > > >
> > > >  Documentation/devicetree/bindings/input/da9062-onkey.txt | 5 ++---
> > > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/input/da9062-onkey.txt
> > > > b/Documentation/devicetree/bindings/input/da9062-onkey.txt
> > > > index 0005b2bdcdd7..9f895454179d 100644
> > > > --- a/Documentation/devicetree/bindings/input/da9062-onkey.txt
> > > > +++ b/Documentation/devicetree/bindings/input/da9062-onkey.txt
> > > > @@ -15,9 +15,8 @@ Required properties:
> > > >
> > > >  Optional properties:
> > > >
> > > > -- dlg,disable-key-power : Disable power-down using a long key-press. If this
> > > > -    entry exists the OnKey driver will remove support for the KEY_POWER key
> > > > -    press when triggered using a long press of the OnKey.
> > > > +- dlg,disable-key-power : If this property is present, the host will not be
> > > > +    issuing shutdown command over I2C in response to a long key-press.
> > >
> > > This also changes behaviour of button press reporting as the driver will not
> > > report a longer press (i.e. a button hold where the driver polls for release).
> > > It will only report a short key press to user-space with this property provided.
> > >
> > > The question here is do we still want to support long press reporting but
> > > without the I2C sequence for shutdown? If so the driver needs to be updated
> > to
> > > work this way as right now it doesn't.
> >
> > Good point. I checked the driver and the documentation for the
> > da9062/3 again and it seems that we interrupt the pmic hw by doing the
> > shutdown by itself. As the documentation says:
> >
> > DA9063:
> > If the hardware reset was initiated by a (debounced) press of nONKEY (or
> > GPIO14 and GPIO15 together) longer than SD_DELAY, the DA9063 initially
> > only asserts control bit KEY_RESET in the fault register and signals a
> > non-maskable interrupt allowing the host to clear the armed reset
> > sequence within 1 s. If the host does not clear KEY_RESET then a
> > shutdown to RESET mode is executed. KEY_SD_MODE determines if normal
> > power sequence timing or a fast shutdown is implemented.
> >
> > DA9062:
> > If the reset was initiated by a user’s long press of nONKEY, initially
> > only KEY_RESET is set and the nIRQ port will be asserted. KEY_RESET
> > signals the host that a shutdown sequence is started. If the host does
> > not then clear KEY_RESET within 1 s by writing a 1 to the related bit in
> > register FAULT_LOG, the shutdown sequence will complete. When the reset
> > condition has disappeared, DA9062 requires a supply (VSYS >
> > VDD_FAULT_UPPER) that provides enough power to start-up from the
> > POWERDOWN mode.
> >
> > So we don't need to check for the KEY_RESET? This would cleanup the code
> > a bit.
> 
> We could remove that but I think the intention originally was to have this code
> there in case there was some way to more gracefully shutdown the system in this
> scenario. Right now that's not the case as the code simply calls to shutdown
> the pmic via a manual register call, but maybe in the future this could be made
> more gracful. Another slight advantage of leaving that code in is that we're
> not waiting for another second delay for the PMIC to pull the plug.
> 
> With regards to this patch and the subsequent one to update the OnKey driver, I
> think we should deprecate this property. The OnKey mode binding you added in
> a different patch will configure the OnKey according to how you want it to
> behave in hardware so I don't think this property makes much sense anymore. We
> can then remove the need for 'key_power' usage in the driver.

Is it okay for you both to drop this patch from this series and open a
new 'rework' series?

Regards,
  Marco

> 
> >
> > Regards,
> >   Marco
> >
> > > >
> > > >  - dlg,key-opmode : Set the nONKEY behaviour. This value is initial set by the
> > > >      otp values. See nONKEY_PIN register description for more information.
> > > > --
> > > > 2.20.1
> > >
> > >
> >
> > --
> > Pengutronix e.K.                           |                             |
> > Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> > 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> > Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
Adam Thomson Jan. 8, 2020, 10:05 a.m. UTC | #6
On 07 January 2020 08:09, Marco Felsch wrote:

> Hi Adam, Dmitry,
> 
> On 19-12-12 10:08, Adam Thomson wrote:
> > On 10 December 2019 08:28, Marco Felsch wrote:
> >
> > > Hi Adam,
> > >
> > > On 19-12-02 12:15, Adam Thomson wrote:
> > > > On 27 November 2019 13:23, Marco Felsch wrote:
> > > >
> > > > > There was a bug within the driver since commit f889beaaab1c ("Input:
> > > > > da9063 - report KEY_POWER instead of KEY_SLEEP during power
> > > > > key-press"). Since we fixed the bug the KEY_POWER will be reported
> > > > > always so we need to adapt the dt-bindings too. Make the description
> > > > > more precise while on it.
> > > > >
> > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > > ---
> > > > > v2:
> > > > > - change description according Dmitry's suggestion.
> > > > >
> > > > >  Documentation/devicetree/bindings/input/da9062-onkey.txt | 5 ++---
> > > > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/input/da9062-onkey.txt
> > > > > b/Documentation/devicetree/bindings/input/da9062-onkey.txt
> > > > > index 0005b2bdcdd7..9f895454179d 100644
> > > > > --- a/Documentation/devicetree/bindings/input/da9062-onkey.txt
> > > > > +++ b/Documentation/devicetree/bindings/input/da9062-onkey.txt
> > > > > @@ -15,9 +15,8 @@ Required properties:
> > > > >
> > > > >  Optional properties:
> > > > >
> > > > > -- dlg,disable-key-power : Disable power-down using a long key-press. If
> this
> > > > > -    entry exists the OnKey driver will remove support for the KEY_POWER
> key
> > > > > -    press when triggered using a long press of the OnKey.
> > > > > +- dlg,disable-key-power : If this property is present, the host will not be
> > > > > +    issuing shutdown command over I2C in response to a long key-press.
> > > >
> > > > This also changes behaviour of button press reporting as the driver will not
> > > > report a longer press (i.e. a button hold where the driver polls for release).
> > > > It will only report a short key press to user-space with this property
> provided.
> > > >
> > > > The question here is do we still want to support long press reporting but
> > > > without the I2C sequence for shutdown? If so the driver needs to be
> updated
> > > to
> > > > work this way as right now it doesn't.
> > >
> > > Good point. I checked the driver and the documentation for the
> > > da9062/3 again and it seems that we interrupt the pmic hw by doing the
> > > shutdown by itself. As the documentation says:
> > >
> > > DA9063:
> > > If the hardware reset was initiated by a (debounced) press of nONKEY (or
> > > GPIO14 and GPIO15 together) longer than SD_DELAY, the DA9063 initially
> > > only asserts control bit KEY_RESET in the fault register and signals a
> > > non-maskable interrupt allowing the host to clear the armed reset
> > > sequence within 1 s. If the host does not clear KEY_RESET then a
> > > shutdown to RESET mode is executed. KEY_SD_MODE determines if normal
> > > power sequence timing or a fast shutdown is implemented.
> > >
> > > DA9062:
> > > If the reset was initiated by a user’s long press of nONKEY, initially
> > > only KEY_RESET is set and the nIRQ port will be asserted. KEY_RESET
> > > signals the host that a shutdown sequence is started. If the host does
> > > not then clear KEY_RESET within 1 s by writing a 1 to the related bit in
> > > register FAULT_LOG, the shutdown sequence will complete. When the reset
> > > condition has disappeared, DA9062 requires a supply (VSYS >
> > > VDD_FAULT_UPPER) that provides enough power to start-up from the
> > > POWERDOWN mode.
> > >
> > > So we don't need to check for the KEY_RESET? This would cleanup the code
> > > a bit.
> >
> > We could remove that but I think the intention originally was to have this code
> > there in case there was some way to more gracefully shutdown the system in
> this
> > scenario. Right now that's not the case as the code simply calls to shutdown
> > the pmic via a manual register call, but maybe in the future this could be made
> > more gracful. Another slight advantage of leaving that code in is that we're
> > not waiting for another second delay for the PMIC to pull the plug.
> >
> > With regards to this patch and the subsequent one to update the OnKey driver,
> I
> > think we should deprecate this property. The OnKey mode binding you added
> in
> > a different patch will configure the OnKey according to how you want it to
> > behave in hardware so I don't think this property makes much sense anymore.
> We
> > can then remove the need for 'key_power' usage in the driver.
> 
> Is it okay for you both to drop this patch from this series and open a
> new 'rework' series?

I have no issues with this.

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/input/da9062-onkey.txt b/Documentation/devicetree/bindings/input/da9062-onkey.txt
index 0005b2bdcdd7..9f895454179d 100644
--- a/Documentation/devicetree/bindings/input/da9062-onkey.txt
+++ b/Documentation/devicetree/bindings/input/da9062-onkey.txt
@@ -15,9 +15,8 @@  Required properties:
 
 Optional properties:
 
-- dlg,disable-key-power : Disable power-down using a long key-press. If this
-    entry exists the OnKey driver will remove support for the KEY_POWER key
-    press when triggered using a long press of the OnKey.
+- dlg,disable-key-power : If this property is present, the host will not be
+    issuing shutdown command over I2C in response to a long key-press.
 
 - dlg,key-opmode : Set the nONKEY behaviour. This value is initial set by the
     otp values. See nONKEY_PIN register description for more information.