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

[Opensource [Anthony Olech]]

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@xxxxxxxxx]
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/