[PATCH V1] da9052: ONKEY: use correct register bit for key status

[PATCH V1] da9052: ONKEY: use correct register bit for key status

[Anthony Olech <anthony.olech.opensource@diasemi.com>]

The wrong register bit of the DA9052/3 PMIC registers was
used to determine the status on the ONKEY.

Signed-off-by: Anthony Olech <anthony.olech.opensource@diasemi.com>
Signed-off-by: David Dajun Chen <david.chen@diasemi.com>
---

This patch is relative to linux-next repository tag next-20140206

The bug that this patch fixes affects only the DA9052 ONKEY driver.

The problem was detected whilst running a scripted set of functional
regression tests whilst investigating a different problem.

This patch has been test compiled on an amd64 server for both x86
and arm targets.

This patch has been spot verified using an SMDK6410 platform
fly-wired to a Dialog da9053 EVB.

 drivers/input/misc/da9052_onkey.c |   14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/input/misc/da9052_onkey.c b/drivers/input/misc/da9052_onkey.c
index 1f695f2..7e78334 100644
--- a/drivers/input/misc/da9052_onkey.c
+++ b/drivers/input/misc/da9052_onkey.c
@@ -27,19 +27,23 @@ struct da9052_onkey {
 
 static void da9052_onkey_query(struct da9052_onkey *onkey)
 {
-	int key_stat;
+	int ret, key_stat;
 
-	key_stat = da9052_reg_read(onkey->da9052, DA9052_EVENT_B_REG);
-	if (key_stat < 0) {
+	ret = da9052_reg_read(onkey->da9052, DA9052_STATUS_A_REG);
+	if (ret < 0) {
 		dev_err(onkey->da9052->dev,
-			"Failed to read onkey event %d\n", key_stat);
+			"Failed to read onkey event err=%d\n", ret);
+		key_stat = false;
 	} else {
 		/*
 		 * Since interrupt for deassertion of ONKEY pin is not
 		 * generated, onkey event state determines the onkey
 		 * button state.
 		 */
-		key_stat &= DA9052_EVENTB_ENONKEY;
+		if (ret & DA9052_STATUSA_NONKEY)
+			key_stat = false;
+		else
+			key_stat = true;
 		input_report_key(onkey->input, KEY_POWER, key_stat);
 		input_sync(onkey->input);
 	}
-- 
end-of-patch for da9052: ONKEY: use correct register bit for key status V1

Re: [PATCH V1] da9052: ONKEY: use correct register bit for key status

[Dmitry Torokhov]

Hi Anthony,

On Thu, Feb 06, 2014 at 03:19:49PM +0000, Anthony Olech <anthony.olech.opensource@diasemi.com> wrote:
> The wrong register bit of the DA9052/3 PMIC registers was
> used to determine the status on the ONKEY.
> 
> Signed-off-by: Anthony Olech <anthony.olech.opensource@diasemi.com>
> Signed-off-by: David Dajun Chen <david.chen@diasemi.com>
> ---
> 
> This patch is relative to linux-next repository tag next-20140206
> 
> The bug that this patch fixes affects only the DA9052 ONKEY driver.
> 
> The problem was detected whilst running a scripted set of functional
> regression tests whilst investigating a different problem.
> 
> This patch has been test compiled on an amd64 server for both x86
> and arm targets.
> 
> This patch has been spot verified using an SMDK6410 platform
> fly-wired to a Dialog da9053 EVB.
> 
>  drivers/input/misc/da9052_onkey.c |   14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/input/misc/da9052_onkey.c b/drivers/input/misc/da9052_onkey.c
> index 1f695f2..7e78334 100644
> --- a/drivers/input/misc/da9052_onkey.c
> +++ b/drivers/input/misc/da9052_onkey.c
> @@ -27,19 +27,23 @@ struct da9052_onkey {
>  
>  static void da9052_onkey_query(struct da9052_onkey *onkey)
>  {
> -	int key_stat;
> +	int ret, key_stat;
>  
> -	key_stat = da9052_reg_read(onkey->da9052, DA9052_EVENT_B_REG);
> -	if (key_stat < 0) {
> +	ret = da9052_reg_read(onkey->da9052, DA9052_STATUS_A_REG);
> +	if (ret < 0) {
>  		dev_err(onkey->da9052->dev,
> -			"Failed to read onkey event %d\n", key_stat);
> +			"Failed to read onkey event err=%d\n", ret);
> +		key_stat = false;

Why do you need this assignment? Also, key_stat is integer, why are we
using boolean values for it?

>  	} else {
>  		/*
>  		 * Since interrupt for deassertion of ONKEY pin is not
>  		 * generated, onkey event state determines the onkey
>  		 * button state.
>  		 */
> -		key_stat &= DA9052_EVENTB_ENONKEY;
> +		if (ret & DA9052_STATUSA_NONKEY)
> +			key_stat = false;
> +		else
> +			key_stat = true;

It seems to me that the relevant changes are replacement of
DA9052_EVENT_B_REG -> DA9052_STATUS_A_REG in da9052_reg_read() and doing:

		key_stat &= DA9052_STATUSA_NONKEY;
		input_report_key(onkey->input, KEY_POWER, !key_stat);

Right?

Thanks.

-- 
Dmitry 

RE: [PATCH V1] da9052: ONKEY: use correct register bit for key status

[Opensource [Anthony Olech]]

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: 11 February 2014 16:29
..[snip]..
> >  static void da9052_onkey_query(struct da9052_onkey *onkey)  {
> > -	int key_stat;
> > +	int ret, key_stat;
> >
> > -	key_stat = da9052_reg_read(onkey->da9052,
> DA9052_EVENT_B_REG);
> > -	if (key_stat < 0) {
> > +	ret = da9052_reg_read(onkey->da9052, DA9052_STATUS_A_REG);
> > +	if (ret < 0) {
> >  		dev_err(onkey->da9052->dev,
> > -			"Failed to read onkey event %d\n", key_stat);
> > +			"Failed to read onkey event err=%d\n", ret);
> > +		key_stat = false;
> 
> Why do you need this assignment? Also, key_stat is integer, why are we
> using boolean values for it?
> 
> >  	} else {
> >  		/*
> >  		 * Since interrupt for deassertion of ONKEY pin is not
> >  		 * generated, onkey event state determines the onkey
> >  		 * button state.
> >  		 */
> > -		key_stat &= DA9052_EVENTB_ENONKEY;
> > +		if (ret & DA9052_STATUSA_NONKEY)
> > +			key_stat = false;
> > +		else
> > +			key_stat = true;
> 
> It seems to me that the relevant changes are replacement of
> DA9052_EVENT_B_REG -> DA9052_STATUS_A_REG in da9052_reg_read()
> and doing:
> 
> 		key_stat &= DA9052_STATUSA_NONKEY;
> 		input_report_key(onkey->input, KEY_POWER, !key_stat);
> 
> Right?

Right as far as the register and the bit within it.

However, should the register read fail due to the PMIC not responding on the i2c bus then the previous code would just loop round the work queue.
By explicitly using key_stat as a boolean it makes the code very clear that in the case of failure a 'false' value is set. As it was the negative value of the error code would be treated as true, since -EFAULT being non-zero will be treated as true.

Tony Olech

> Thanks.
> Dmitry

Re: [PATCH V1] da9052: ONKEY: use correct register bit for key status

[Dmitry Torokhov]

On Tue, Feb 11, 2014 at 04:57:47PM +0000, Opensource [Anthony Olech] wrote:
> > -----Original Message-----
> > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> > Sent: 11 February 2014 16:29
> ..[snip]..
> > >  static void da9052_onkey_query(struct da9052_onkey *onkey)  {
> > > -	int key_stat;
> > > +	int ret, key_stat;
> > >
> > > -	key_stat = da9052_reg_read(onkey->da9052,
> > DA9052_EVENT_B_REG);
> > > -	if (key_stat < 0) {
> > > +	ret = da9052_reg_read(onkey->da9052, DA9052_STATUS_A_REG);
> > > +	if (ret < 0) {
> > >  		dev_err(onkey->da9052->dev,
> > > -			"Failed to read onkey event %d\n", key_stat);
> > > +			"Failed to read onkey event err=%d\n", ret);
> > > +		key_stat = false;
> > 
> > Why do you need this assignment? Also, key_stat is integer, why are we
> > using boolean values for it?
> > 
> > >  	} else {
> > >  		/*
> > >  		 * Since interrupt for deassertion of ONKEY pin is not
> > >  		 * generated, onkey event state determines the onkey
> > >  		 * button state.
> > >  		 */
> > > -		key_stat &= DA9052_EVENTB_ENONKEY;
> > > +		if (ret & DA9052_STATUSA_NONKEY)
> > > +			key_stat = false;
> > > +		else
> > > +			key_stat = true;
> > 
> > It seems to me that the relevant changes are replacement of
> > DA9052_EVENT_B_REG -> DA9052_STATUS_A_REG in da9052_reg_read()
> > and doing:
> > 
> > 		key_stat &= DA9052_STATUSA_NONKEY;
> > 		input_report_key(onkey->input, KEY_POWER, !key_stat);
> > 
> > Right?
> 
> Right as far as the register and the bit within it.
> 
> However, should the register read fail due to the PMIC not responding
> on the i2c bus then the previous code would just loop round the work
> queue.  By explicitly using key_stat as a boolean it makes the code
> very clear that in the case of failure a 'false' value is set.

Then it should be defined as boolean.

> As it
> was the negative value of the error code would be treated as true,
> since -EFAULT being non-zero will be treated as true.

This is however not what your patch description said it does; it only
mentioned the incorrect register assignment. Maybe we shoudl do the
following then:

	...
	} else {
		bool pressed = !(ret & DA9052_STATUSA_NONKEY);

		input_report_key(onkey->input, KEY_POWER, pressed);
		input_sync(onkey->input);

		if (pressed)
			schedule_work(...);
	}

And mention that we are changing polling (no longer re-poll on read
errors) in addition to changing register/bits used.

Thanks.

-- 
Dmitry