[PATCH v2 0/7] stk8312 fixes and cleanup

[PATCH v2 0/7] stk8312 fixes and cleanup

[Hartmut Knaack]

Fixing some issues I spotted during review.

Changes in V2:
  - put device back in active mode in case of an error

Hartmut Knaack (7):
  iio:accel:stk8312: add triggered buffer dependency
  iio:accel:stk8312: check for invalid value
  iio:accel:stk8312: improve error handling
  iio:accel:stk8312: rework macro definitions
  iio:accel:stk8312: use appropriate variable types
  iio:accel:stk8312: code style cleanup
  iio:accel:stk8312: drop local buffer

 drivers/iio/accel/Kconfig   |   2 +
 drivers/iio/accel/stk8312.c | 106 +++++++++++++++++++++++++-------------------
 2 files changed, 62 insertions(+), 46 deletions(-)

-- 
2.4.6

[PATCH v2 1/7] iio:accel:stk8312: add triggered buffer dependency

[Hartmut Knaack]

Add the still missing dependencies for triggered buffer support.

Fixes: 	95c12bba51c37 ("iio: accel: Add buffer mode for Sensortek STK8312")
Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
Reviewed-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
---
 drivers/iio/accel/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
index 00e7bcbdbe24..c99d8e242218 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -140,6 +140,8 @@ config MMA9553
 config STK8312
 	tristate "Sensortek STK8312 3-Axis Accelerometer Driver"
 	depends on I2C
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
 	help
 	  Say yes here to get support for the Sensortek STK8312 3-axis
 	  accelerometer.
-- 
2.4.6

[PATCH v2 2/7] iio:accel:stk8312: check for invalid value

[Hartmut Knaack]

Revision 1.2 of the datasheet recommends on page 22 to only write non-zero
values read from OTP register 0x70 into AFECTRL register.

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
Reviewed-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
---
 drivers/iio/accel/stk8312.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c
index c2bd1444d6da..6592be8e6377 100644
--- a/drivers/iio/accel/stk8312.c
+++ b/drivers/iio/accel/stk8312.c
@@ -150,6 +150,8 @@ static int stk8312_otp_init(struct stk8312_data *data)
 		goto exit_err;
 
 	ret = i2c_smbus_read_byte_data(client, STK8312_REG_OTPDATA);
+	if (ret == 0)
+		ret = -EINVAL;
 	if (ret < 0)
 		goto exit_err;
 
-- 
2.4.6

Re: [PATCH v2 2/7] iio:accel:stk8312: check for invalid value

[Crt Mori]

On 29 July 2015 at 23:39, Hartmut Knaack <knaack.h@gmx.de> wrote:
> Revision 1.2 of the datasheet recommends on page 22 to only write non-zero
> values read from OTP register 0x70 into AFECTRL register.
>
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
> Reviewed-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
> ---
>  drivers/iio/accel/stk8312.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c
> index c2bd1444d6da..6592be8e6377 100644
> --- a/drivers/iio/accel/stk8312.c
> +++ b/drivers/iio/accel/stk8312.c
> @@ -150,6 +150,8 @@ static int stk8312_otp_init(struct stk8312_data *data)
>                 goto exit_err;
>
>         ret = i2c_smbus_read_byte_data(client, STK8312_REG_OTPDATA);
> +       if (ret == 0)
> +               ret = -EINVAL;
This seems fishy. We have a macro value written to client which cannot really
give us EINVAL, except if we are checking the client, but then this would only
fail if we had some other i2c device on the line with stk8312 address, which
would ack other i2c commands above this.
>         if (ret < 0)
>                 goto exit_err;
>
> --
> 2.4.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/7] iio:accel:stk8312: check for invalid value

[Hartmut Knaack]

Crt Mori schrieb am 30.07.2015 um 09:09:
> On 29 July 2015 at 23:39, Hartmut Knaack <knaack.h@gmx.de> wrote:
>> Revision 1.2 of the datasheet recommends on page 22 to only write non-zero
>> values read from OTP register 0x70 into AFECTRL register.
>>
>> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
>> Reviewed-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
>> ---
>>  drivers/iio/accel/stk8312.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c
>> index c2bd1444d6da..6592be8e6377 100644
>> --- a/drivers/iio/accel/stk8312.c
>> +++ b/drivers/iio/accel/stk8312.c
>> @@ -150,6 +150,8 @@ static int stk8312_otp_init(struct stk8312_data *data)
>>                 goto exit_err;
>>
>>         ret = i2c_smbus_read_byte_data(client, STK8312_REG_OTPDATA);
>> +       if (ret == 0)
>> +               ret = -EINVAL;
> This seems fishy. We have a macro value written to client which cannot really
> give us EINVAL, except if we are checking the client, but then this would only
> fail if we had some other i2c device on the line with stk8312 address, which
> would ack other i2c commands above this.

I'm not sure to really understand your point, or if you actually understood
this patch. I'm not totally happy with EINVAL as error code, but it just
seemed to fit better than any other possible error code. Of course I would
be happy to get recommendations for better fitting codes.
A short explanation why this is done: in the sample code provided in the
data sheet, it is mentioned that if the value read from that register 0x70
is zero, then it should not be written into the AFECTRL register. So, that's
what I have addressed here.

>>         if (ret < 0)
>>                 goto exit_err;
>>
>> --
>> 2.4.6
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2 2/7] iio:accel:stk8312: check for invalid value

[Crt Mori]

On 1 August 2015 at 00:54, Hartmut Knaack <knaack.h@gmx.de> wrote:
> Crt Mori schrieb am 30.07.2015 um 09:09:
>> On 29 July 2015 at 23:39, Hartmut Knaack <knaack.h@gmx.de> wrote:
>>> Revision 1.2 of the datasheet recommends on page 22 to only write non-zero
>>> values read from OTP register 0x70 into AFECTRL register.
>>>
>>> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
>>> Reviewed-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
>>> ---
>>>  drivers/iio/accel/stk8312.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c
>>> index c2bd1444d6da..6592be8e6377 100644
>>> --- a/drivers/iio/accel/stk8312.c
>>> +++ b/drivers/iio/accel/stk8312.c
>>> @@ -150,6 +150,8 @@ static int stk8312_otp_init(struct stk8312_data *data)
>>>                 goto exit_err;
>>>
>>>         ret = i2c_smbus_read_byte_data(client, STK8312_REG_OTPDATA);
>>> +       if (ret == 0)
>>> +               ret = -EINVAL;
>> This seems fishy. We have a macro value written to client which cannot really
>> give us EINVAL, except if we are checking the client, but then this would only
>> fail if we had some other i2c device on the line with stk8312 address, which
>> would ack other i2c commands above this.
>
> I'm not sure to really understand your point, or if you actually understood
> this patch. I'm not totally happy with EINVAL as error code, but it just
> seemed to fit better than any other possible error code. Of course I would
> be happy to get recommendations for better fitting codes.
> A short explanation why this is done: in the sample code provided in the
> data sheet, it is mentioned that if the value read from that register 0x70
> is zero, then it should not be written into the AFECTRL register. So, that's
> what I have addressed here.
EINVAL is more like bad user input, so if this fails because of the last command
then it is correct, otherwise I would put more like -EBUSY or -EACCES

Thank you for the explanation.

>
>>>         if (ret < 0)
>>>                 goto exit_err;
>>>
>>> --
>>> 2.4.6
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>

Re: [PATCH v2 2/7] iio:accel:stk8312: check for invalid value

[Hartmut Knaack]

Crt Mori schrieb am 01.08.2015 um 09:35:
> On 1 August 2015 at 00:54, Hartmut Knaack <knaack.h@gmx.de> wrote:
>> Crt Mori schrieb am 30.07.2015 um 09:09:
>>> On 29 July 2015 at 23:39, Hartmut Knaack <knaack.h@gmx.de> wrote:
>>>> Revision 1.2 of the datasheet recommends on page 22 to only write non-zero
>>>> values read from OTP register 0x70 into AFECTRL register.
>>>>
>>>> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
>>>> Reviewed-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
>>>> ---
>>>>  drivers/iio/accel/stk8312.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c
>>>> index c2bd1444d6da..6592be8e6377 100644
>>>> --- a/drivers/iio/accel/stk8312.c
>>>> +++ b/drivers/iio/accel/stk8312.c
>>>> @@ -150,6 +150,8 @@ static int stk8312_otp_init(struct stk8312_data *data)
>>>>                 goto exit_err;
>>>>
>>>>         ret = i2c_smbus_read_byte_data(client, STK8312_REG_OTPDATA);
>>>> +       if (ret == 0)
>>>> +               ret = -EINVAL;
>>> This seems fishy. We have a macro value written to client which cannot really
>>> give us EINVAL, except if we are checking the client, but then this would only
>>> fail if we had some other i2c device on the line with stk8312 address, which
>>> would ack other i2c commands above this.
>>
>> I'm not sure to really understand your point, or if you actually understood
>> this patch. I'm not totally happy with EINVAL as error code, but it just
>> seemed to fit better than any other possible error code. Of course I would
>> be happy to get recommendations for better fitting codes.
>> A short explanation why this is done: in the sample code provided in the
>> data sheet, it is mentioned that if the value read from that register 0x70
>> is zero, then it should not be written into the AFECTRL register. So, that's
>> what I have addressed here.
> EINVAL is more like bad user input, so if this fails because of the last command
> then it is correct, otherwise I would put more like -EBUSY or -EACCES
> 

Tiberiu, I'd like to get your opinion on these alternative error codes. Do you
have any information why the read value could be zero and why that AFECTRL
register should not be set to zero?
Thanks,

Hartmut

> Thank you for the explanation.
> 
>>
>>>>         if (ret < 0)
>>>>                 goto exit_err;
>>>>
>>>> --
>>>> 2.4.6
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2 2/7] iio:accel:stk8312: check for invalid value

[Jonathan Cameron]

On 06/08/15 23:23, Hartmut Knaack wrote:
> Crt Mori schrieb am 01.08.2015 um 09:35:
>> On 1 August 2015 at 00:54, Hartmut Knaack <knaack.h@gmx.de> wrote:
>>> Crt Mori schrieb am 30.07.2015 um 09:09:
>>>> On 29 July 2015 at 23:39, Hartmut Knaack <knaack.h@gmx.de> wrote:
>>>>> Revision 1.2 of the datasheet recommends on page 22 to only write non-zero
>>>>> values read from OTP register 0x70 into AFECTRL register.
>>>>>
>>>>> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
>>>>> Reviewed-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
>>>>> ---
>>>>>  drivers/iio/accel/stk8312.c | 2 ++
>>>>>  1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c
>>>>> index c2bd1444d6da..6592be8e6377 100644
>>>>> --- a/drivers/iio/accel/stk8312.c
>>>>> +++ b/drivers/iio/accel/stk8312.c
>>>>> @@ -150,6 +150,8 @@ static int stk8312_otp_init(struct stk8312_data *data)
>>>>>                 goto exit_err;
>>>>>
>>>>>         ret = i2c_smbus_read_byte_data(client, STK8312_REG_OTPDATA);
>>>>> +       if (ret == 0)
>>>>> +               ret = -EINVAL;
>>>> This seems fishy. We have a macro value written to client which cannot really
>>>> give us EINVAL, except if we are checking the client, but then this would only
>>>> fail if we had some other i2c device on the line with stk8312 address, which
>>>> would ack other i2c commands above this.
>>>
>>> I'm not sure to really understand your point, or if you actually understood
>>> this patch. I'm not totally happy with EINVAL as error code, but it just
>>> seemed to fit better than any other possible error code. Of course I would
>>> be happy to get recommendations for better fitting codes.
>>> A short explanation why this is done: in the sample code provided in the
>>> data sheet, it is mentioned that if the value read from that register 0x70
>>> is zero, then it should not be written into the AFECTRL register. So, that's
>>> what I have addressed here.
>> EINVAL is more like bad user input, so if this fails because of the last command
>> then it is correct, otherwise I would put more like -EBUSY or -EACCES
>>
> 
> Tiberiu, I'd like to get your opinion on these alternative error codes. Do you
> have any information why the read value could be zero and why that AFECTRL
> register should not be set to zero?
> Thanks,
> 
> Hartmut
I already applied version 1 of this patch, so send an follow up if the decision
is that there is a better error code.

J
> 
>> Thank you for the explanation.
>>
>>>
>>>>>         if (ret < 0)
>>>>>                 goto exit_err;
>>>>>
>>>>> --
>>>>> 2.4.6
>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

RE: [PATCH v2 2/7] iio:accel:stk8312: check for invalid value

[Breana, Tiberiu A]

PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBKb25hdGhhbiBDYW1lcm9uIFtt
YWlsdG86amljMjNAa2VybmVsLm9yZ10NCj4gU2VudDogU2F0dXJkYXksIEF1Z3VzdCA4LCAyMDE1
IDI6MTggUE0NCj4gVG86IEhhcnRtdXQgS25hYWNrOyBDcnQgTW9yaQ0KPiBDYzogbGludXgtaWlv
QHZnZXIua2VybmVsLm9yZzsgTGFycy1QZXRlciBDbGF1c2VuOyBQZXRlciBNZWVyd2FsZDsgQnJl
YW5hLA0KPiBUaWJlcml1IEENCj4gU3ViamVjdDogUmU6IFtQQVRDSCB2MiAyLzddIGlpbzphY2Nl
bDpzdGs4MzEyOiBjaGVjayBmb3IgaW52YWxpZCB2YWx1ZQ0KPiANCj4gT24gMDYvMDgvMTUgMjM6
MjMsIEhhcnRtdXQgS25hYWNrIHdyb3RlOg0KPiA+IENydCBNb3JpIHNjaHJpZWIgYW0gMDEuMDgu
MjAxNSB1bSAwOTozNToNCj4gPj4gT24gMSBBdWd1c3QgMjAxNSBhdCAwMDo1NCwgSGFydG11dCBL
bmFhY2sgPGtuYWFjay5oQGdteC5kZT4gd3JvdGU6DQo+ID4+PiBDcnQgTW9yaSBzY2hyaWViIGFt
IDMwLjA3LjIwMTUgdW0gMDk6MDk6DQo+ID4+Pj4gT24gMjkgSnVseSAyMDE1IGF0IDIzOjM5LCBI
YXJ0bXV0IEtuYWFjayA8a25hYWNrLmhAZ214LmRlPiB3cm90ZToNCj4gPj4+Pj4gUmV2aXNpb24g
MS4yIG9mIHRoZSBkYXRhc2hlZXQgcmVjb21tZW5kcyBvbiBwYWdlIDIyIHRvIG9ubHkgd3JpdGUN
Cj4gPj4+Pj4gbm9uLXplcm8gdmFsdWVzIHJlYWQgZnJvbSBPVFAgcmVnaXN0ZXIgMHg3MCBpbnRv
IEFGRUNUUkwgcmVnaXN0ZXIuDQo+ID4+Pj4+DQo+ID4+Pj4+IFNpZ25lZC1vZmYtYnk6IEhhcnRt
dXQgS25hYWNrIDxrbmFhY2suaEBnbXguZGU+DQo+ID4+Pj4+IFJldmlld2VkLWJ5OiBUaWJlcml1
IEJyZWFuYSA8dGliZXJpdS5hLmJyZWFuYUBpbnRlbC5jb20+DQo+ID4+Pj4+IC0tLQ0KPiA+Pj4+
PiAgZHJpdmVycy9paW8vYWNjZWwvc3RrODMxMi5jIHwgMiArKw0KPiA+Pj4+PiAgMSBmaWxlIGNo
YW5nZWQsIDIgaW5zZXJ0aW9ucygrKQ0KPiA+Pj4+Pg0KPiA+Pj4+PiBkaWZmIC0tZ2l0IGEvZHJp
dmVycy9paW8vYWNjZWwvc3RrODMxMi5jDQo+ID4+Pj4+IGIvZHJpdmVycy9paW8vYWNjZWwvc3Rr
ODMxMi5jIGluZGV4IGMyYmQxNDQ0ZDZkYS4uNjU5MmJlOGU2Mzc3DQo+ID4+Pj4+IDEwMDY0NA0K
PiA+Pj4+PiAtLS0gYS9kcml2ZXJzL2lpby9hY2NlbC9zdGs4MzEyLmMNCj4gPj4+Pj4gKysrIGIv
ZHJpdmVycy9paW8vYWNjZWwvc3RrODMxMi5jDQo+ID4+Pj4+IEBAIC0xNTAsNiArMTUwLDggQEAg
c3RhdGljIGludCBzdGs4MzEyX290cF9pbml0KHN0cnVjdCBzdGs4MzEyX2RhdGENCj4gKmRhdGEp
DQo+ID4+Pj4+ICAgICAgICAgICAgICAgICBnb3RvIGV4aXRfZXJyOw0KPiA+Pj4+Pg0KPiA+Pj4+
PiAgICAgICAgIHJldCA9IGkyY19zbWJ1c19yZWFkX2J5dGVfZGF0YShjbGllbnQsDQo+ID4+Pj4+
IFNUSzgzMTJfUkVHX09UUERBVEEpOw0KPiA+Pj4+PiArICAgICAgIGlmIChyZXQgPT0gMCkNCj4g
Pj4+Pj4gKyAgICAgICAgICAgICAgIHJldCA9IC1FSU5WQUw7DQo+ID4+Pj4gVGhpcyBzZWVtcyBm
aXNoeS4gV2UgaGF2ZSBhIG1hY3JvIHZhbHVlIHdyaXR0ZW4gdG8gY2xpZW50IHdoaWNoDQo+ID4+
Pj4gY2Fubm90IHJlYWxseSBnaXZlIHVzIEVJTlZBTCwgZXhjZXB0IGlmIHdlIGFyZSBjaGVja2lu
ZyB0aGUgY2xpZW50LA0KPiA+Pj4+IGJ1dCB0aGVuIHRoaXMgd291bGQgb25seSBmYWlsIGlmIHdl
IGhhZCBzb21lIG90aGVyIGkyYyBkZXZpY2Ugb24NCj4gPj4+PiB0aGUgbGluZSB3aXRoIHN0azgz
MTIgYWRkcmVzcywgd2hpY2ggd291bGQgYWNrIG90aGVyIGkyYyBjb21tYW5kcw0KPiBhYm92ZSB0
aGlzLg0KPiA+Pj4NCj4gPj4+IEknbSBub3Qgc3VyZSB0byByZWFsbHkgdW5kZXJzdGFuZCB5b3Vy
IHBvaW50LCBvciBpZiB5b3UgYWN0dWFsbHkNCj4gPj4+IHVuZGVyc3Rvb2QgdGhpcyBwYXRjaC4g
SSdtIG5vdCB0b3RhbGx5IGhhcHB5IHdpdGggRUlOVkFMIGFzIGVycm9yDQo+ID4+PiBjb2RlLCBi
dXQgaXQganVzdCBzZWVtZWQgdG8gZml0IGJldHRlciB0aGFuIGFueSBvdGhlciBwb3NzaWJsZSBl
cnJvcg0KPiA+Pj4gY29kZS4gT2YgY291cnNlIEkgd291bGQgYmUgaGFwcHkgdG8gZ2V0IHJlY29t
bWVuZGF0aW9ucyBmb3IgYmV0dGVyDQo+IGZpdHRpbmcgY29kZXMuDQo+ID4+PiBBIHNob3J0IGV4
cGxhbmF0aW9uIHdoeSB0aGlzIGlzIGRvbmU6IGluIHRoZSBzYW1wbGUgY29kZSBwcm92aWRlZCBp
bg0KPiA+Pj4gdGhlIGRhdGEgc2hlZXQsIGl0IGlzIG1lbnRpb25lZCB0aGF0IGlmIHRoZSB2YWx1
ZSByZWFkIGZyb20gdGhhdA0KPiA+Pj4gcmVnaXN0ZXIgMHg3MCBpcyB6ZXJvLCB0aGVuIGl0IHNo
b3VsZCBub3QgYmUgd3JpdHRlbiBpbnRvIHRoZQ0KPiA+Pj4gQUZFQ1RSTCByZWdpc3Rlci4gU28s
IHRoYXQncyB3aGF0IEkgaGF2ZSBhZGRyZXNzZWQgaGVyZS4NCj4gPj4gRUlOVkFMIGlzIG1vcmUg
bGlrZSBiYWQgdXNlciBpbnB1dCwgc28gaWYgdGhpcyBmYWlscyBiZWNhdXNlIG9mIHRoZQ0KPiA+
PiBsYXN0IGNvbW1hbmQgdGhlbiBpdCBpcyBjb3JyZWN0LCBvdGhlcndpc2UgSSB3b3VsZCBwdXQg
bW9yZSBsaWtlDQo+ID4+IC1FQlVTWSBvciAtRUFDQ0VTDQo+ID4+DQo+ID4NCj4gPiBUaWJlcml1
LCBJJ2QgbGlrZSB0byBnZXQgeW91ciBvcGluaW9uIG9uIHRoZXNlIGFsdGVybmF0aXZlIGVycm9y
DQo+ID4gY29kZXMuIERvIHlvdSBoYXZlIGFueSBpbmZvcm1hdGlvbiB3aHkgdGhlIHJlYWQgdmFs
dWUgY291bGQgYmUgemVybw0KPiA+IGFuZCB3aHkgdGhhdCBBRkVDVFJMIHJlZ2lzdGVyIHNob3Vs
ZCBub3QgYmUgc2V0IHRvIHplcm8/DQo+ID4gVGhhbmtzLA0KPiA+DQo+ID4gSGFydG11dA0KDQpJ
IGRvbid0IGtub3cgd2h5IHRoZSByZWFkIHZhbHVlIGNvdWxkIGJlIDAuIFRoZSBvbmx5IGluZm9y
bWF0aW9uIEkgaGF2ZSBpcyB0aGUgZGF0YXNoZWV0LCBzYW1lIGFzIHlvdS4gSSB0aGluayBFSU5W
QUwgaXMgZmluZTsgRUJVU1kgd291bGQgYWxzbyBtYWtlIHNlbnNlLiBJdCdzIHVwIHRvIHlvdS4N
Cg0KVGliZXJpdQ0KDQo+IEkgYWxyZWFkeSBhcHBsaWVkIHZlcnNpb24gMSBvZiB0aGlzIHBhdGNo
LCBzbyBzZW5kIGFuIGZvbGxvdyB1cCBpZiB0aGUgZGVjaXNpb24gaXMNCj4gdGhhdCB0aGVyZSBp
cyBhIGJldHRlciBlcnJvciBjb2RlLg0KPiANCj4gSg0KPiA+DQo+ID4+IFRoYW5rIHlvdSBmb3Ig
dGhlIGV4cGxhbmF0aW9uLg0KPiA+Pg0KPiA+Pj4NCj4gPj4+Pj4gICAgICAgICBpZiAocmV0IDwg
MCkNCj4gPj4+Pj4gICAgICAgICAgICAgICAgIGdvdG8gZXhpdF9lcnI7DQo+ID4+Pj4+DQo+ID4+
Pj4+IC0tDQo+ID4+Pj4+IDIuNC42DQo+ID4+Pj4+DQo+ID4+Pj4+IC0tDQo+ID4+Pj4+IFRvIHVu
c3Vic2NyaWJlIGZyb20gdGhpcyBsaXN0OiBzZW5kIHRoZSBsaW5lICJ1bnN1YnNjcmliZQ0KPiA+
Pj4+PiBsaW51eC1paW8iIGluIHRoZSBib2R5IG9mIGEgbWVzc2FnZSB0byBtYWpvcmRvbW9Admdl
ci5rZXJuZWwub3JnDQo+ID4+Pj4+IE1vcmUgbWFqb3Jkb21vIGluZm8gYXQgIGh0dHA6Ly92Z2Vy
Lmtlcm5lbC5vcmcvbWFqb3Jkb21vLQ0KPiBpbmZvLmh0bWwNCg==

Re: [PATCH v2 2/7] iio:accel:stk8312: check for invalid value

[Hartmut Knaack]

Breana, Tiberiu A schrieb am 10.08.2015 um 11:24:
>> -----Original Message-----
>> From: Jonathan Cameron [mailto:jic23@kernel.org]
>> Sent: Saturday, August 8, 2015 2:18 PM
>> To: Hartmut Knaack; Crt Mori
>> Cc: linux-iio@vger.kernel.org; Lars-Peter Clausen; Peter Meerwald; Breana,
>> Tiberiu A
>> Subject: Re: [PATCH v2 2/7] iio:accel:stk8312: check for invalid value
>>
>> On 06/08/15 23:23, Hartmut Knaack wrote:
>>> Crt Mori schrieb am 01.08.2015 um 09:35:
>>>> On 1 August 2015 at 00:54, Hartmut Knaack <knaack.h@gmx.de> wrote:
>>>>> Crt Mori schrieb am 30.07.2015 um 09:09:
>>>>>> On 29 July 2015 at 23:39, Hartmut Knaack <knaack.h@gmx.de> wrote:
>>>>>>> Revision 1.2 of the datasheet recommends on page 22 to only write
>>>>>>> non-zero values read from OTP register 0x70 into AFECTRL register.
>>>>>>>
>>>>>>> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
>>>>>>> Reviewed-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
>>>>>>> ---
>>>>>>>  drivers/iio/accel/stk8312.c | 2 ++
>>>>>>>  1 file changed, 2 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/iio/accel/stk8312.c
>>>>>>> b/drivers/iio/accel/stk8312.c index c2bd1444d6da..6592be8e6377
>>>>>>> 100644
>>>>>>> --- a/drivers/iio/accel/stk8312.c
>>>>>>> +++ b/drivers/iio/accel/stk8312.c
>>>>>>> @@ -150,6 +150,8 @@ static int stk8312_otp_init(struct stk8312_data
>> *data)
>>>>>>>                 goto exit_err;
>>>>>>>
>>>>>>>         ret = i2c_smbus_read_byte_data(client,
>>>>>>> STK8312_REG_OTPDATA);
>>>>>>> +       if (ret == 0)
>>>>>>> +               ret = -EINVAL;
>>>>>> This seems fishy. We have a macro value written to client which
>>>>>> cannot really give us EINVAL, except if we are checking the client,
>>>>>> but then this would only fail if we had some other i2c device on
>>>>>> the line with stk8312 address, which would ack other i2c commands
>> above this.
>>>>>
>>>>> I'm not sure to really understand your point, or if you actually
>>>>> understood this patch. I'm not totally happy with EINVAL as error
>>>>> code, but it just seemed to fit better than any other possible error
>>>>> code. Of course I would be happy to get recommendations for better
>> fitting codes.
>>>>> A short explanation why this is done: in the sample code provided in
>>>>> the data sheet, it is mentioned that if the value read from that
>>>>> register 0x70 is zero, then it should not be written into the
>>>>> AFECTRL register. So, that's what I have addressed here.
>>>> EINVAL is more like bad user input, so if this fails because of the
>>>> last command then it is correct, otherwise I would put more like
>>>> -EBUSY or -EACCES
>>>>
>>>
>>> Tiberiu, I'd like to get your opinion on these alternative error
>>> codes. Do you have any information why the read value could be zero
>>> and why that AFECTRL register should not be set to zero?
>>> Thanks,
>>>
>>> Hartmut
> 
> I don't know why the read value could be 0. The only information I have is the datasheet, same as you. I think EINVAL is fine; EBUSY would also make sense. It's up to you.
> 
> Tiberiu
> 

I'm not feeling any strong preference for any other code. EBUSY however doesn't
seem so appropriate either, since the device indicated already by setting bit 7
of _REG_OTPCTRL that it is ready. I would prefer to leave it as is, and whoever
has good reason for another code shall feel free to change it.
Thanks

Hartmut

>> I already applied version 1 of this patch, so send an follow up if the decision is
>> that there is a better error code.
>>
>> J
>>>
>>>> Thank you for the explanation.
>>>>
>>>>>
>>>>>>>         if (ret < 0)
>>>>>>>                 goto exit_err;
>>>>>>>
>>>>>>> --
>>>>>>> 2.4.6
>>>>>>>
>>>>>>> --
>>>>>>> To unsubscribe from this list: send the line "unsubscribe
>>>>>>> linux-iio" in the body of a message to majordomo@vger.kernel.org
>>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-
>> info.html
> N�����r��y���b�X��ǧv�^�)޺{.n�+����{��*"��^n�r���z�\x1a��h����&��\x1e�G���h�\x03(�階�ݢj"��\x1a�^[m�����z�ޖ���f���h���~�mml==
> 

[PATCH v2 3/7] iio:accel:stk8312: improve error handling

[Hartmut Knaack]

Improve error handling in the following ways:
  - set return value on error condition to an appropriate error code
  - return error code immediately in case of an error (slightly changes
    code structure)
  - pass up real error code
  - add missing error handling
  - return 0 when error have been caught already
  - put device back in active mode after error occurs

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
---
 drivers/iio/accel/stk8312.c | 60 ++++++++++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 22 deletions(-)

diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c
index 6592be8e6377..4e1dda7e896d 100644
--- a/drivers/iio/accel/stk8312.c
+++ b/drivers/iio/accel/stk8312.c
@@ -146,8 +146,10 @@ static int stk8312_otp_init(struct stk8312_data *data)
 		count--;
 	} while (!(ret & 0x80) && count > 0);
 
-	if (count == 0)
+	if (count == 0) {
+		ret = -ETIMEDOUT;
 		goto exit_err;
+	}
 
 	ret = i2c_smbus_read_byte_data(client, STK8312_REG_OTPDATA);
 	if (ret == 0)
@@ -161,7 +163,7 @@ static int stk8312_otp_init(struct stk8312_data *data)
 		goto exit_err;
 	msleep(150);
 
-	return ret;
+	return 0;
 
 exit_err:
 	dev_err(&client->dev, "failed to initialize sensor\n");
@@ -205,8 +207,11 @@ static int stk8312_set_interrupts(struct stk8312_data *data, u8 int_mask)
 		return ret;
 
 	ret = i2c_smbus_write_byte_data(client, STK8312_REG_INTSU, int_mask);
-	if (ret < 0)
+	if (ret < 0) {
 		dev_err(&client->dev, "failed to set interrupts\n");
+		stk8312_set_mode(data, mode);
+		return ret;
+	}
 
 	return stk8312_set_mode(data, mode);
 }
@@ -230,7 +235,7 @@ static int stk8312_data_rdy_trigger_set_state(struct iio_trigger *trig,
 
 	data->dready_trigger_on = state;
 
-	return ret;
+	return 0;
 }
 
 static const struct iio_trigger_ops stk8312_trigger_ops = {
@@ -255,20 +260,24 @@ static int stk8312_set_sample_rate(struct stk8312_data *data, int rate)
 		return ret;
 
 	ret = i2c_smbus_read_byte_data(client, STK8312_REG_SR);
-	if (ret < 0) {
-		dev_err(&client->dev, "failed to set sampling rate\n");
-		return ret;
-	}
+	if (ret < 0)
+		goto err_activate;
 
 	masked_reg = (ret & (~STK8312_SR_MASK)) | rate;
 
 	ret = i2c_smbus_write_byte_data(client, STK8312_REG_SR, masked_reg);
 	if (ret < 0)
-		dev_err(&client->dev, "failed to set sampling rate\n");
-	else
-		data->sample_rate_idx = rate;
+		goto err_activate;
+
+	data->sample_rate_idx = rate;
 
 	return stk8312_set_mode(data, mode);
+
+err_activate:
+	dev_err(&client->dev, "failed to set sampling rate\n");
+	stk8312_set_mode(data, mode);
+
+	return ret;
 }
 
 static int stk8312_set_range(struct stk8312_data *data, u8 range)
@@ -290,21 +299,25 @@ static int stk8312_set_range(struct stk8312_data *data, u8 range)
 		return ret;
 
 	ret = i2c_smbus_read_byte_data(client, STK8312_REG_STH);
-	if (ret < 0) {
-		dev_err(&client->dev, "failed to change sensor range\n");
-		return ret;
-	}
+	if (ret < 0)
+		goto err_activate;
 
 	masked_reg = ret & (~STK8312_RNG_MASK);
 	masked_reg |= range << STK8312_RNG_SHIFT;
 
 	ret = i2c_smbus_write_byte_data(client, STK8312_REG_STH, masked_reg);
 	if (ret < 0)
-		dev_err(&client->dev, "failed to change sensor range\n");
-	else
-		data->range = range;
+		goto err_activate;
+
+	data->range = range;
 
 	return stk8312_set_mode(data, mode);
+
+err_activate:
+	dev_err(&client->dev, "failed to change sensor range\n");
+	stk8312_set_mode(data, mode);
+
+	return ret;
 }
 
 static int stk8312_read_accel(struct stk8312_data *data, u8 address)
@@ -337,18 +350,21 @@ static int stk8312_read_raw(struct iio_dev *indio_dev,
 		ret = stk8312_set_mode(data, data->mode | STK8312_MODE_ACTIVE);
 		if (ret < 0) {
 			mutex_unlock(&data->lock);
-			return -EINVAL;
+			return ret;
 		}
 		ret = stk8312_read_accel(data, chan->address);
 		if (ret < 0) {
 			stk8312_set_mode(data,
 					 data->mode & (~STK8312_MODE_ACTIVE));
 			mutex_unlock(&data->lock);
-			return -EINVAL;
+			return ret;
 		}
 		*val = sign_extend32(ret, 7);
-		stk8312_set_mode(data, data->mode & (~STK8312_MODE_ACTIVE));
+		ret = stk8312_set_mode(data,
+				       data->mode & (~STK8312_MODE_ACTIVE));
 		mutex_unlock(&data->lock);
+		if (ret < 0)
+			return ret;
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
 		*val = stk8312_scale_table[data->range - 1][0];
@@ -608,7 +624,7 @@ static int stk8312_probe(struct i2c_client *client,
 		goto err_buffer_cleanup;
 	}
 
-	return ret;
+	return 0;
 
 err_buffer_cleanup:
 	iio_triggered_buffer_cleanup(indio_dev);
-- 
2.4.6

Re: [PATCH v2 3/7] iio:accel:stk8312: improve error handling

[Crt Mori]

Hi, some comments/questions below:

On 29 July 2015 at 23:39, Hartmut Knaack <knaack.h@gmx.de> wrote:
> Improve error handling in the following ways:
>   - set return value on error condition to an appropriate error code
>   - return error code immediately in case of an error (slightly changes
>     code structure)
>   - pass up real error code
>   - add missing error handling
>   - return 0 when error have been caught already
>   - put device back in active mode after error occurs
>
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
> ---
>  drivers/iio/accel/stk8312.c | 60 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 38 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c
> index 6592be8e6377..4e1dda7e896d 100644
> --- a/drivers/iio/accel/stk8312.c
> +++ b/drivers/iio/accel/stk8312.c
> @@ -146,8 +146,10 @@ static int stk8312_otp_init(struct stk8312_data *data)
>                 count--;
>         } while (!(ret & 0x80) && count > 0);
>
> -       if (count == 0)
> +       if (count == 0) {
> +               ret = -ETIMEDOUT;
>                 goto exit_err;
> +       }
You dont need braces since it is a one word statement or add a dev_err() report.
>
>         ret = i2c_smbus_read_byte_data(client, STK8312_REG_OTPDATA);
>         if (ret == 0)
> @@ -161,7 +163,7 @@ static int stk8312_otp_init(struct stk8312_data *data)
>                 goto exit_err;
Why dont we return here as well?
>         msleep(150);
>
> -       return ret;
> +       return 0;
>
>  exit_err:
>         dev_err(&client->dev, "failed to initialize sensor\n");
> @@ -205,8 +207,11 @@ static int stk8312_set_interrupts(struct stk8312_data *data, u8 int_mask)
>                 return ret;
>
>         ret = i2c_smbus_write_byte_data(client, STK8312_REG_INTSU, int_mask);
> -       if (ret < 0)
> +       if (ret < 0) {
>                 dev_err(&client->dev, "failed to set interrupts\n");
> +               stk8312_set_mode(data, mode);
> +               return ret;
> +       }
>
>         return stk8312_set_mode(data, mode);
>  }
> @@ -230,7 +235,7 @@ static int stk8312_data_rdy_trigger_set_state(struct iio_trigger *trig,
>
>         data->dready_trigger_on = state;
>
> -       return ret;
> +       return 0;
>  }
>
>  static const struct iio_trigger_ops stk8312_trigger_ops = {
> @@ -255,20 +260,24 @@ static int stk8312_set_sample_rate(struct stk8312_data *data, int rate)
>                 return ret;
>
>         ret = i2c_smbus_read_byte_data(client, STK8312_REG_SR);
> -       if (ret < 0) {
> -               dev_err(&client->dev, "failed to set sampling rate\n");
> -               return ret;
> -       }
> +       if (ret < 0)
> +               goto err_activate;
>
>         masked_reg = (ret & (~STK8312_SR_MASK)) | rate;
>
>         ret = i2c_smbus_write_byte_data(client, STK8312_REG_SR, masked_reg);
>         if (ret < 0)
> -               dev_err(&client->dev, "failed to set sampling rate\n");
> -       else
> -               data->sample_rate_idx = rate;
> +               goto err_activate;
> +
> +       data->sample_rate_idx = rate;
>
>         return stk8312_set_mode(data, mode);
> +
> +err_activate:
> +       dev_err(&client->dev, "failed to set sampling rate\n");
> +       stk8312_set_mode(data, mode);
> +
> +       return ret;
>  }
>
>  static int stk8312_set_range(struct stk8312_data *data, u8 range)
> @@ -290,21 +299,25 @@ static int stk8312_set_range(struct stk8312_data *data, u8 range)
>                 return ret;
>
>         ret = i2c_smbus_read_byte_data(client, STK8312_REG_STH);
> -       if (ret < 0) {
> -               dev_err(&client->dev, "failed to change sensor range\n");
> -               return ret;
> -       }
> +       if (ret < 0)
> +               goto err_activate;
>
>         masked_reg = ret & (~STK8312_RNG_MASK);
>         masked_reg |= range << STK8312_RNG_SHIFT;
>
>         ret = i2c_smbus_write_byte_data(client, STK8312_REG_STH, masked_reg);
>         if (ret < 0)
So in case of error we print error, but continue without returning
error value, but if it is
a success then we go to err_activate and return a positive value?
Either label is
confusing or the whole process.
> -               dev_err(&client->dev, "failed to change sensor range\n");
> -       else
> -               data->range = range;
> +               goto err_activate;
> +
> +       data->range = range;
>
>         return stk8312_set_mode(data, mode);
> +
> +err_activate:
> +       dev_err(&client->dev, "failed to change sensor range\n");
> +       stk8312_set_mode(data, mode);
> +
> +       return ret;
>  }
>
>  static int stk8312_read_accel(struct stk8312_data *data, u8 address)
> @@ -337,18 +350,21 @@ static int stk8312_read_raw(struct iio_dev *indio_dev,
>                 ret = stk8312_set_mode(data, data->mode | STK8312_MODE_ACTIVE);
>                 if (ret < 0) {
>                         mutex_unlock(&data->lock);
> -                       return -EINVAL;
> +                       return ret;
>                 }
>                 ret = stk8312_read_accel(data, chan->address);
>                 if (ret < 0) {
>                         stk8312_set_mode(data,
>                                          data->mode & (~STK8312_MODE_ACTIVE));
>                         mutex_unlock(&data->lock);
> -                       return -EINVAL;
> +                       return ret;
>                 }
>                 *val = sign_extend32(ret, 7);
> -               stk8312_set_mode(data, data->mode & (~STK8312_MODE_ACTIVE));
> +               ret = stk8312_set_mode(data,
> +                                      data->mode & (~STK8312_MODE_ACTIVE));
>                 mutex_unlock(&data->lock);
> +               if (ret < 0)
> +                       return ret;
>                 return IIO_VAL_INT;
>         case IIO_CHAN_INFO_SCALE:
>                 *val = stk8312_scale_table[data->range - 1][0];
> @@ -608,7 +624,7 @@ static int stk8312_probe(struct i2c_client *client,
>                 goto err_buffer_cleanup;
>         }
>
> -       return ret;
> +       return 0;
>
>  err_buffer_cleanup:
>         iio_triggered_buffer_cleanup(indio_dev);
> --
> 2.4.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2 3/7] iio:accel:stk8312: improve error handling

[Breana, Tiberiu A]

PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBDcnQgTW9yaSBbbWFpbHRvOmNt
b0BtZWxleGlzLmNvbV0NCj4gU2VudDogVGh1cnNkYXksIEp1bHkgMzAsIDIwMTUgMTA6MDQgQU0N
Cj4gVG86IEhhcnRtdXQgS25hYWNrDQo+IENjOiBsaW51eC1paW9Admdlci5rZXJuZWwub3JnOyBK
b25hdGhhbiBDYW1lcm9uOyBMYXJzLVBldGVyIENsYXVzZW47IFBldGVyDQo+IE1lZXJ3YWxkOyBC
cmVhbmEsIFRpYmVyaXUgQQ0KPiBTdWJqZWN0OiBSZTogW1BBVENIIHYyIDMvN10gaWlvOmFjY2Vs
OnN0azgzMTI6IGltcHJvdmUgZXJyb3IgaGFuZGxpbmcNCj4gDQo+IEhpLCBzb21lIGNvbW1lbnRz
L3F1ZXN0aW9ucyBiZWxvdzoNCj4gDQo+IE9uIDI5IEp1bHkgMjAxNSBhdCAyMzozOSwgSGFydG11
dCBLbmFhY2sgPGtuYWFjay5oQGdteC5kZT4gd3JvdGU6DQo+ID4gSW1wcm92ZSBlcnJvciBoYW5k
bGluZyBpbiB0aGUgZm9sbG93aW5nIHdheXM6DQo+ID4gICAtIHNldCByZXR1cm4gdmFsdWUgb24g
ZXJyb3IgY29uZGl0aW9uIHRvIGFuIGFwcHJvcHJpYXRlIGVycm9yIGNvZGUNCj4gPiAgIC0gcmV0
dXJuIGVycm9yIGNvZGUgaW1tZWRpYXRlbHkgaW4gY2FzZSBvZiBhbiBlcnJvciAoc2xpZ2h0bHkg
Y2hhbmdlcw0KPiA+ICAgICBjb2RlIHN0cnVjdHVyZSkNCj4gPiAgIC0gcGFzcyB1cCByZWFsIGVy
cm9yIGNvZGUNCj4gPiAgIC0gYWRkIG1pc3NpbmcgZXJyb3IgaGFuZGxpbmcNCj4gPiAgIC0gcmV0
dXJuIDAgd2hlbiBlcnJvciBoYXZlIGJlZW4gY2F1Z2h0IGFscmVhZHkNCj4gPiAgIC0gcHV0IGRl
dmljZSBiYWNrIGluIGFjdGl2ZSBtb2RlIGFmdGVyIGVycm9yIG9jY3Vycw0KPiA+DQo+ID4gU2ln
bmVkLW9mZi1ieTogSGFydG11dCBLbmFhY2sgPGtuYWFjay5oQGdteC5kZT4NCg0KQ29tbWVudHMg
aW5saW5lLg0KDQo+ID4gLS0tDQo+ID4gIGRyaXZlcnMvaWlvL2FjY2VsL3N0azgzMTIuYyB8IDYw
DQo+ID4gKysrKysrKysrKysrKysrKysrKysrKysrKysrKy0tLS0tLS0tLS0tLS0tLS0tDQo+ID4g
IDEgZmlsZSBjaGFuZ2VkLCAzOCBpbnNlcnRpb25zKCspLCAyMiBkZWxldGlvbnMoLSkNCj4gPg0K
PiA+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2lpby9hY2NlbC9zdGs4MzEyLmMgYi9kcml2ZXJzL2lp
by9hY2NlbC9zdGs4MzEyLmMNCj4gPiBpbmRleCA2NTkyYmU4ZTYzNzcuLjRlMWRkYTdlODk2ZCAx
MDA2NDQNCj4gPiAtLS0gYS9kcml2ZXJzL2lpby9hY2NlbC9zdGs4MzEyLmMNCj4gPiArKysgYi9k
cml2ZXJzL2lpby9hY2NlbC9zdGs4MzEyLmMNCj4gPiBAQCAtMTQ2LDggKzE0NiwxMCBAQCBzdGF0
aWMgaW50IHN0azgzMTJfb3RwX2luaXQoc3RydWN0IHN0azgzMTJfZGF0YQ0KPiAqZGF0YSkNCj4g
PiAgICAgICAgICAgICAgICAgY291bnQtLTsNCj4gPiAgICAgICAgIH0gd2hpbGUgKCEocmV0ICYg
MHg4MCkgJiYgY291bnQgPiAwKTsNCj4gPg0KPiA+IC0gICAgICAgaWYgKGNvdW50ID09IDApDQo+
ID4gKyAgICAgICBpZiAoY291bnQgPT0gMCkgew0KPiA+ICsgICAgICAgICAgICAgICByZXQgPSAt
RVRJTUVET1VUOw0KPiA+ICAgICAgICAgICAgICAgICBnb3RvIGV4aXRfZXJyOw0KPiA+ICsgICAg
ICAgfQ0KPiBZb3UgZG9udCBuZWVkIGJyYWNlcyBzaW5jZSBpdCBpcyBhIG9uZSB3b3JkIHN0YXRl
bWVudCBvciBhZGQgYSBkZXZfZXJyKCkNCj4gcmVwb3J0Lg0KDQorMQ0KDQo+ID4NCj4gPiAgICAg
ICAgIHJldCA9IGkyY19zbWJ1c19yZWFkX2J5dGVfZGF0YShjbGllbnQsIFNUSzgzMTJfUkVHX09U
UERBVEEpOw0KPiA+ICAgICAgICAgaWYgKHJldCA9PSAwKQ0KPiA+IEBAIC0xNjEsNyArMTYzLDcg
QEAgc3RhdGljIGludCBzdGs4MzEyX290cF9pbml0KHN0cnVjdCBzdGs4MzEyX2RhdGENCj4gKmRh
dGEpDQo+ID4gICAgICAgICAgICAgICAgIGdvdG8gZXhpdF9lcnI7DQo+IFdoeSBkb250IHdlIHJl
dHVybiBoZXJlIGFzIHdlbGw/DQoNCkknbSBub3Qgc3VyZSBJIGZvbGxvdy4NCg0KPiA+ICAgICAg
ICAgbXNsZWVwKDE1MCk7DQo+ID4NCj4gPiAtICAgICAgIHJldHVybiByZXQ7DQo+ID4gKyAgICAg
ICByZXR1cm4gMDsNCj4gPg0KPiA+ICBleGl0X2VycjoNCj4gPiAgICAgICAgIGRldl9lcnIoJmNs
aWVudC0+ZGV2LCAiZmFpbGVkIHRvIGluaXRpYWxpemUgc2Vuc29yXG4iKTsgQEANCj4gPiAtMjA1
LDggKzIwNywxMSBAQCBzdGF0aWMgaW50IHN0azgzMTJfc2V0X2ludGVycnVwdHMoc3RydWN0IHN0
azgzMTJfZGF0YQ0KPiAqZGF0YSwgdTggaW50X21hc2spDQo+ID4gICAgICAgICAgICAgICAgIHJl
dHVybiByZXQ7DQo+ID4NCj4gPiAgICAgICAgIHJldCA9IGkyY19zbWJ1c193cml0ZV9ieXRlX2Rh
dGEoY2xpZW50LCBTVEs4MzEyX1JFR19JTlRTVSwNCj4gaW50X21hc2spOw0KPiA+IC0gICAgICAg
aWYgKHJldCA8IDApDQo+ID4gKyAgICAgICBpZiAocmV0IDwgMCkgew0KPiA+ICAgICAgICAgICAg
ICAgICBkZXZfZXJyKCZjbGllbnQtPmRldiwgImZhaWxlZCB0byBzZXQgaW50ZXJydXB0c1xuIik7
DQo+ID4gKyAgICAgICAgICAgICAgIHN0azgzMTJfc2V0X21vZGUoZGF0YSwgbW9kZSk7DQo+ID4g
KyAgICAgICAgICAgICAgIHJldHVybiByZXQ7DQo+ID4gKyAgICAgICB9DQo+ID4NCj4gPiAgICAg
ICAgIHJldHVybiBzdGs4MzEyX3NldF9tb2RlKGRhdGEsIG1vZGUpOyAgfSBAQCAtMjMwLDcgKzIz
NSw3IEBADQo+ID4gc3RhdGljIGludCBzdGs4MzEyX2RhdGFfcmR5X3RyaWdnZXJfc2V0X3N0YXRl
KHN0cnVjdCBpaW9fdHJpZ2dlcg0KPiA+ICp0cmlnLA0KPiA+DQo+ID4gICAgICAgICBkYXRhLT5k
cmVhZHlfdHJpZ2dlcl9vbiA9IHN0YXRlOw0KPiA+DQo+ID4gLSAgICAgICByZXR1cm4gcmV0Ow0K
PiA+ICsgICAgICAgcmV0dXJuIDA7DQo+ID4gIH0NCj4gPg0KPiA+ICBzdGF0aWMgY29uc3Qgc3Ry
dWN0IGlpb190cmlnZ2VyX29wcyBzdGs4MzEyX3RyaWdnZXJfb3BzID0geyBAQA0KPiA+IC0yNTUs
MjAgKzI2MCwyNCBAQCBzdGF0aWMgaW50IHN0azgzMTJfc2V0X3NhbXBsZV9yYXRlKHN0cnVjdA0K
PiBzdGs4MzEyX2RhdGEgKmRhdGEsIGludCByYXRlKQ0KPiA+ICAgICAgICAgICAgICAgICByZXR1
cm4gcmV0Ow0KPiA+DQo+ID4gICAgICAgICByZXQgPSBpMmNfc21idXNfcmVhZF9ieXRlX2RhdGEo
Y2xpZW50LCBTVEs4MzEyX1JFR19TUik7DQo+ID4gLSAgICAgICBpZiAocmV0IDwgMCkgew0KPiA+
IC0gICAgICAgICAgICAgICBkZXZfZXJyKCZjbGllbnQtPmRldiwgImZhaWxlZCB0byBzZXQgc2Ft
cGxpbmcgcmF0ZVxuIik7DQo+ID4gLSAgICAgICAgICAgICAgIHJldHVybiByZXQ7DQo+ID4gLSAg
ICAgICB9DQo+ID4gKyAgICAgICBpZiAocmV0IDwgMCkNCj4gPiArICAgICAgICAgICAgICAgZ290
byBlcnJfYWN0aXZhdGU7DQo+ID4NCj4gPiAgICAgICAgIG1hc2tlZF9yZWcgPSAocmV0ICYgKH5T
VEs4MzEyX1NSX01BU0spKSB8IHJhdGU7DQo+ID4NCj4gPiAgICAgICAgIHJldCA9IGkyY19zbWJ1
c193cml0ZV9ieXRlX2RhdGEoY2xpZW50LCBTVEs4MzEyX1JFR19TUiwNCj4gbWFza2VkX3JlZyk7
DQo+ID4gICAgICAgICBpZiAocmV0IDwgMCkNCj4gPiAtICAgICAgICAgICAgICAgZGV2X2Vycigm
Y2xpZW50LT5kZXYsICJmYWlsZWQgdG8gc2V0IHNhbXBsaW5nIHJhdGVcbiIpOw0KPiA+IC0gICAg
ICAgZWxzZQ0KPiA+IC0gICAgICAgICAgICAgICBkYXRhLT5zYW1wbGVfcmF0ZV9pZHggPSByYXRl
Ow0KPiA+ICsgICAgICAgICAgICAgICBnb3RvIGVycl9hY3RpdmF0ZTsNCj4gPiArDQo+ID4gKyAg
ICAgICBkYXRhLT5zYW1wbGVfcmF0ZV9pZHggPSByYXRlOw0KPiA+DQo+ID4gICAgICAgICByZXR1
cm4gc3RrODMxMl9zZXRfbW9kZShkYXRhLCBtb2RlKTsNCj4gPiArDQo+ID4gK2Vycl9hY3RpdmF0
ZToNCj4gPiArICAgICAgIGRldl9lcnIoJmNsaWVudC0+ZGV2LCAiZmFpbGVkIHRvIHNldCBzYW1w
bGluZyByYXRlXG4iKTsNCj4gPiArICAgICAgIHN0azgzMTJfc2V0X21vZGUoZGF0YSwgbW9kZSk7
DQo+ID4gKw0KPiA+ICsgICAgICAgcmV0dXJuIHJldDsNCj4gPiAgfQ0KPiA+DQo+ID4gIHN0YXRp
YyBpbnQgc3RrODMxMl9zZXRfcmFuZ2Uoc3RydWN0IHN0azgzMTJfZGF0YSAqZGF0YSwgdTggcmFu
Z2UpIEBADQo+ID4gLTI5MCwyMSArMjk5LDI1IEBAIHN0YXRpYyBpbnQgc3RrODMxMl9zZXRfcmFu
Z2Uoc3RydWN0IHN0azgzMTJfZGF0YQ0KPiAqZGF0YSwgdTggcmFuZ2UpDQo+ID4gICAgICAgICAg
ICAgICAgIHJldHVybiByZXQ7DQo+ID4NCj4gPiAgICAgICAgIHJldCA9IGkyY19zbWJ1c19yZWFk
X2J5dGVfZGF0YShjbGllbnQsIFNUSzgzMTJfUkVHX1NUSCk7DQo+ID4gLSAgICAgICBpZiAocmV0
IDwgMCkgew0KPiA+IC0gICAgICAgICAgICAgICBkZXZfZXJyKCZjbGllbnQtPmRldiwgImZhaWxl
ZCB0byBjaGFuZ2Ugc2Vuc29yIHJhbmdlXG4iKTsNCj4gPiAtICAgICAgICAgICAgICAgcmV0dXJu
IHJldDsNCj4gPiAtICAgICAgIH0NCj4gPiArICAgICAgIGlmIChyZXQgPCAwKQ0KPiA+ICsgICAg
ICAgICAgICAgICBnb3RvIGVycl9hY3RpdmF0ZTsNCj4gPg0KPiA+ICAgICAgICAgbWFza2VkX3Jl
ZyA9IHJldCAmICh+U1RLODMxMl9STkdfTUFTSyk7DQo+ID4gICAgICAgICBtYXNrZWRfcmVnIHw9
IHJhbmdlIDw8IFNUSzgzMTJfUk5HX1NISUZUOw0KPiA+DQo+ID4gICAgICAgICByZXQgPSBpMmNf
c21idXNfd3JpdGVfYnl0ZV9kYXRhKGNsaWVudCwgU1RLODMxMl9SRUdfU1RILA0KPiBtYXNrZWRf
cmVnKTsNCj4gPiAgICAgICAgIGlmIChyZXQgPCAwKQ0KPiBTbyBpbiBjYXNlIG9mIGVycm9yIHdl
IHByaW50IGVycm9yLCBidXQgY29udGludWUgd2l0aG91dCByZXR1cm5pbmcgZXJyb3IgdmFsdWUs
DQo+IGJ1dCBpZiBpdCBpcyBhIHN1Y2Nlc3MgdGhlbiB3ZSBnbyB0byBlcnJfYWN0aXZhdGUgYW5k
IHJldHVybiBhIHBvc2l0aXZlIHZhbHVlPw0KPiBFaXRoZXIgbGFiZWwgaXMNCj4gY29uZnVzaW5n
IG9yIHRoZSB3aG9sZSBwcm9jZXNzLg0KDQpJIHN1Z2dlc3QgeW91IHRha2UgYSBjbG9zZXIgbG9v
ay4NCg0KPiA+IC0gICAgICAgICAgICAgICBkZXZfZXJyKCZjbGllbnQtPmRldiwgImZhaWxlZCB0
byBjaGFuZ2Ugc2Vuc29yIHJhbmdlXG4iKTsNCj4gPiAtICAgICAgIGVsc2UNCj4gPiAtICAgICAg
ICAgICAgICAgZGF0YS0+cmFuZ2UgPSByYW5nZTsNCj4gPiArICAgICAgICAgICAgICAgZ290byBl
cnJfYWN0aXZhdGU7DQo+ID4gKw0KPiA+ICsgICAgICAgZGF0YS0+cmFuZ2UgPSByYW5nZTsNCj4g
Pg0KPiA+ICAgICAgICAgcmV0dXJuIHN0azgzMTJfc2V0X21vZGUoZGF0YSwgbW9kZSk7DQo+ID4g
Kw0KPiA+ICtlcnJfYWN0aXZhdGU6DQo+ID4gKyAgICAgICBkZXZfZXJyKCZjbGllbnQtPmRldiwg
ImZhaWxlZCB0byBjaGFuZ2Ugc2Vuc29yIHJhbmdlXG4iKTsNCj4gPiArICAgICAgIHN0azgzMTJf
c2V0X21vZGUoZGF0YSwgbW9kZSk7DQo+ID4gKw0KPiA+ICsgICAgICAgcmV0dXJuIHJldDsNCj4g
PiAgfQ0KPiA+DQo+ID4gIHN0YXRpYyBpbnQgc3RrODMxMl9yZWFkX2FjY2VsKHN0cnVjdCBzdGs4
MzEyX2RhdGEgKmRhdGEsIHU4IGFkZHJlc3MpDQo+ID4gQEAgLTMzNywxOCArMzUwLDIxIEBAIHN0
YXRpYyBpbnQgc3RrODMxMl9yZWFkX3JhdyhzdHJ1Y3QgaWlvX2Rldg0KPiAqaW5kaW9fZGV2LA0K
PiA+ICAgICAgICAgICAgICAgICByZXQgPSBzdGs4MzEyX3NldF9tb2RlKGRhdGEsIGRhdGEtPm1v
ZGUgfA0KPiBTVEs4MzEyX01PREVfQUNUSVZFKTsNCj4gPiAgICAgICAgICAgICAgICAgaWYgKHJl
dCA8IDApIHsNCj4gPiAgICAgICAgICAgICAgICAgICAgICAgICBtdXRleF91bmxvY2soJmRhdGEt
PmxvY2spOw0KPiA+IC0gICAgICAgICAgICAgICAgICAgICAgIHJldHVybiAtRUlOVkFMOw0KPiA+
ICsgICAgICAgICAgICAgICAgICAgICAgIHJldHVybiByZXQ7DQo+ID4gICAgICAgICAgICAgICAg
IH0NCj4gPiAgICAgICAgICAgICAgICAgcmV0ID0gc3RrODMxMl9yZWFkX2FjY2VsKGRhdGEsIGNo
YW4tPmFkZHJlc3MpOw0KPiA+ICAgICAgICAgICAgICAgICBpZiAocmV0IDwgMCkgew0KPiA+ICAg
ICAgICAgICAgICAgICAgICAgICAgIHN0azgzMTJfc2V0X21vZGUoZGF0YSwNCj4gPiAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGRhdGEtPm1vZGUgJiAoflNUSzgzMTJf
TU9ERV9BQ1RJVkUpKTsNCj4gPiAgICAgICAgICAgICAgICAgICAgICAgICBtdXRleF91bmxvY2so
JmRhdGEtPmxvY2spOw0KPiA+IC0gICAgICAgICAgICAgICAgICAgICAgIHJldHVybiAtRUlOVkFM
Ow0KPiA+ICsgICAgICAgICAgICAgICAgICAgICAgIHJldHVybiByZXQ7DQo+ID4gICAgICAgICAg
ICAgICAgIH0NCj4gPiAgICAgICAgICAgICAgICAgKnZhbCA9IHNpZ25fZXh0ZW5kMzIocmV0LCA3
KTsNCj4gPiAtICAgICAgICAgICAgICAgc3RrODMxMl9zZXRfbW9kZShkYXRhLCBkYXRhLT5tb2Rl
ICYNCj4gKH5TVEs4MzEyX01PREVfQUNUSVZFKSk7DQo+ID4gKyAgICAgICAgICAgICAgIHJldCA9
IHN0azgzMTJfc2V0X21vZGUoZGF0YSwNCj4gPiArICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICBkYXRhLT5tb2RlICYNCj4gPiArICh+U1RLODMxMl9NT0RFX0FDVElWRSkpOw0K
PiA+ICAgICAgICAgICAgICAgICBtdXRleF91bmxvY2soJmRhdGEtPmxvY2spOw0KPiA+ICsgICAg
ICAgICAgICAgICBpZiAocmV0IDwgMCkNCj4gPiArICAgICAgICAgICAgICAgICAgICAgICByZXR1
cm4gcmV0Ow0KDQpIYXJ0bXV0LCBJIHN0YW5kIGJ5IG15IGNvbW1lbnQgZnJvbSB0aGUgZmlyc3Qg
cGF0Y2ggdmVyc2lvbi4NCldlIHNob3VsZG4ndCByZXR1cm4gYSByZWFkIGVycm9yIGlmIHdlJ3Jl
IG5vdCBhYmxlIHRvIHB1dCB0aGUNCnNlbnNvciBiYWNrIGluIHN0YW5kYnkgbW9kZSBhZnRlciBy
ZWFkaW5nIHRoZSB2YWx1ZXMuDQoNCj4gPiAgICAgICAgICAgICAgICAgcmV0dXJuIElJT19WQUxf
SU5UOw0KPiA+ICAgICAgICAgY2FzZSBJSU9fQ0hBTl9JTkZPX1NDQUxFOg0KPiA+ICAgICAgICAg
ICAgICAgICAqdmFsID0gc3RrODMxMl9zY2FsZV90YWJsZVtkYXRhLT5yYW5nZSAtIDFdWzBdOyBA
QA0KPiA+IC02MDgsNyArNjI0LDcgQEAgc3RhdGljIGludCBzdGs4MzEyX3Byb2JlKHN0cnVjdCBp
MmNfY2xpZW50ICpjbGllbnQsDQo+ID4gICAgICAgICAgICAgICAgIGdvdG8gZXJyX2J1ZmZlcl9j
bGVhbnVwOw0KPiA+ICAgICAgICAgfQ0KPiA+DQo+ID4gLSAgICAgICByZXR1cm4gcmV0Ow0KPiA+
ICsgICAgICAgcmV0dXJuIDA7DQo+ID4NCj4gPiAgZXJyX2J1ZmZlcl9jbGVhbnVwOg0KPiA+ICAg
ICAgICAgaWlvX3RyaWdnZXJlZF9idWZmZXJfY2xlYW51cChpbmRpb19kZXYpOw0KPiA+IC0tDQo+
ID4gMi40LjYNCj4gPg0KPiA+IC0tDQo+ID4gVG8gdW5zdWJzY3JpYmUgZnJvbSB0aGlzIGxpc3Q6
IHNlbmQgdGhlIGxpbmUgInVuc3Vic2NyaWJlIGxpbnV4LWlpbyINCj4gPiBpbiB0aGUgYm9keSBv
ZiBhIG1lc3NhZ2UgdG8gbWFqb3Jkb21vQHZnZXIua2VybmVsLm9yZyBNb3JlDQo+IG1ham9yZG9t
bw0KPiA+IGluZm8gYXQgIGh0dHA6Ly92Z2VyLmtlcm5lbC5vcmcvbWFqb3Jkb21vLWluZm8uaHRt
bA0K

Re: [PATCH v2 3/7] iio:accel:stk8312: improve error handling

[Crt Mori]

On 30 July 2015 at 11:07, Breana, Tiberiu A <tiberiu.a.breana@intel.com> wrote:
>> -----Original Message-----
>> From: Crt Mori [mailto:cmo@melexis.com]
>> Sent: Thursday, July 30, 2015 10:04 AM
>> To: Hartmut Knaack
>> Cc: linux-iio@vger.kernel.org; Jonathan Cameron; Lars-Peter Clausen; Peter
>> Meerwald; Breana, Tiberiu A
>> Subject: Re: [PATCH v2 3/7] iio:accel:stk8312: improve error handling
>>
>> Hi, some comments/questions below:
>>
>> On 29 July 2015 at 23:39, Hartmut Knaack <knaack.h@gmx.de> wrote:
>> > Improve error handling in the following ways:
>> >   - set return value on error condition to an appropriate error code
>> >   - return error code immediately in case of an error (slightly changes
>> >     code structure)
>> >   - pass up real error code
>> >   - add missing error handling
>> >   - return 0 when error have been caught already
>> >   - put device back in active mode after error occurs
>> >
>> > Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
>
> Comments inline.
>
>> > ---
>> >  drivers/iio/accel/stk8312.c | 60
>> > ++++++++++++++++++++++++++++-----------------
>> >  1 file changed, 38 insertions(+), 22 deletions(-)
>> >
>> > diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c
>> > index 6592be8e6377..4e1dda7e896d 100644
>> > --- a/drivers/iio/accel/stk8312.c
>> > +++ b/drivers/iio/accel/stk8312.c
>> > @@ -146,8 +146,10 @@ static int stk8312_otp_init(struct stk8312_data
>> *data)
>> >                 count--;
>> >         } while (!(ret & 0x80) && count > 0);
>> >
>> > -       if (count == 0)
>> > +       if (count == 0) {
>> > +               ret = -ETIMEDOUT;
>> >                 goto exit_err;
>> > +       }
>> You dont need braces since it is a one word statement or add a dev_err()
>> report.
>
> +1
>
>> >
>> >         ret = i2c_smbus_read_byte_data(client, STK8312_REG_OTPDATA);
>> >         if (ret == 0)
>> > @@ -161,7 +163,7 @@ static int stk8312_otp_init(struct stk8312_data
>> *data)
>> >                 goto exit_err;
>> Why dont we return here as well?
>
> I'm not sure I follow.
>
>> >         msleep(150);
>> >
>> > -       return ret;
>> > +       return 0;
>> >
>> >  exit_err:
>> >         dev_err(&client->dev, "failed to initialize sensor\n"); @@
>> > -205,8 +207,11 @@ static int stk8312_set_interrupts(struct stk8312_data
>> *data, u8 int_mask)
>> >                 return ret;
>> >
>> >         ret = i2c_smbus_write_byte_data(client, STK8312_REG_INTSU,
>> int_mask);
>> > -       if (ret < 0)
>> > +       if (ret < 0) {
>> >                 dev_err(&client->dev, "failed to set interrupts\n");
>> > +               stk8312_set_mode(data, mode);
>> > +               return ret;
>> > +       }
>> >
>> >         return stk8312_set_mode(data, mode);  } @@ -230,7 +235,7 @@
>> > static int stk8312_data_rdy_trigger_set_state(struct iio_trigger
>> > *trig,
>> >
>> >         data->dready_trigger_on = state;
>> >
>> > -       return ret;
>> > +       return 0;
>> >  }
>> >
>> >  static const struct iio_trigger_ops stk8312_trigger_ops = { @@
>> > -255,20 +260,24 @@ static int stk8312_set_sample_rate(struct
>> stk8312_data *data, int rate)
>> >                 return ret;
>> >
>> >         ret = i2c_smbus_read_byte_data(client, STK8312_REG_SR);
>> > -       if (ret < 0) {
>> > -               dev_err(&client->dev, "failed to set sampling rate\n");
>> > -               return ret;
>> > -       }
>> > +       if (ret < 0)
>> > +               goto err_activate;
>> >
>> >         masked_reg = (ret & (~STK8312_SR_MASK)) | rate;
>> >
>> >         ret = i2c_smbus_write_byte_data(client, STK8312_REG_SR,
>> masked_reg);
>> >         if (ret < 0)
>> > -               dev_err(&client->dev, "failed to set sampling rate\n");
>> > -       else
>> > -               data->sample_rate_idx = rate;
>> > +               goto err_activate;
>> > +
>> > +       data->sample_rate_idx = rate;
>> >
>> >         return stk8312_set_mode(data, mode);
>> > +
>> > +err_activate:
>> > +       dev_err(&client->dev, "failed to set sampling rate\n");
>> > +       stk8312_set_mode(data, mode);
>> > +
>> > +       return ret;
>> >  }
>> >
>> >  static int stk8312_set_range(struct stk8312_data *data, u8 range) @@
>> > -290,21 +299,25 @@ static int stk8312_set_range(struct stk8312_data
>> *data, u8 range)
>> >                 return ret;
>> >
>> >         ret = i2c_smbus_read_byte_data(client, STK8312_REG_STH);
>> > -       if (ret < 0) {
>> > -               dev_err(&client->dev, "failed to change sensor range\n");
>> > -               return ret;
>> > -       }
>> > +       if (ret < 0)
>> > +               goto err_activate;
>> >
>> >         masked_reg = ret & (~STK8312_RNG_MASK);
>> >         masked_reg |= range << STK8312_RNG_SHIFT;
>> >
>> >         ret = i2c_smbus_write_byte_data(client, STK8312_REG_STH,
>> masked_reg);
>> >         if (ret < 0)
>> So in case of error we print error, but continue without returning error value,
>> but if it is a success then we go to err_activate and return a positive value?
>> Either label is
>> confusing or the whole process.
>
> I suggest you take a closer look.
>

Ow sorry, messed with - and + (combined them together actually). This is
OK.

>> > -               dev_err(&client->dev, "failed to change sensor range\n");
>> > -       else
>> > -               data->range = range;
>> > +               goto err_activate;
>> > +
>> > +       data->range = range;
>> >
>> >         return stk8312_set_mode(data, mode);
>> > +
>> > +err_activate:
>> > +       dev_err(&client->dev, "failed to change sensor range\n");
>> > +       stk8312_set_mode(data, mode);
>> > +
>> > +       return ret;
>> >  }
>> >
>> >  static int stk8312_read_accel(struct stk8312_data *data, u8 address)
>> > @@ -337,18 +350,21 @@ static int stk8312_read_raw(struct iio_dev
>> *indio_dev,
>> >                 ret = stk8312_set_mode(data, data->mode |
>> STK8312_MODE_ACTIVE);
>> >                 if (ret < 0) {
>> >                         mutex_unlock(&data->lock);
>> > -                       return -EINVAL;
>> > +                       return ret;
>> >                 }
>> >                 ret = stk8312_read_accel(data, chan->address);
>> >                 if (ret < 0) {
>> >                         stk8312_set_mode(data,
>> >                                          data->mode & (~STK8312_MODE_ACTIVE));
>> >                         mutex_unlock(&data->lock);
>> > -                       return -EINVAL;
>> > +                       return ret;
>> >                 }
>> >                 *val = sign_extend32(ret, 7);
>> > -               stk8312_set_mode(data, data->mode &
>> (~STK8312_MODE_ACTIVE));
>> > +               ret = stk8312_set_mode(data,
>> > +                                      data->mode &
>> > + (~STK8312_MODE_ACTIVE));
>> >                 mutex_unlock(&data->lock);
>> > +               if (ret < 0)
>> > +                       return ret;
>
> Hartmut, I stand by my comment from the first patch version.
> We shouldn't return a read error if we're not able to put the
> sensor back in standby mode after reading the values.
>
>> >                 return IIO_VAL_INT;
>> >         case IIO_CHAN_INFO_SCALE:
>> >                 *val = stk8312_scale_table[data->range - 1][0]; @@
>> > -608,7 +624,7 @@ static int stk8312_probe(struct i2c_client *client,
>> >                 goto err_buffer_cleanup;
>> >         }
>> >
>> > -       return ret;
>> > +       return 0;
>> >
>> >  err_buffer_cleanup:
>> >         iio_triggered_buffer_cleanup(indio_dev);
>> > --
>> > 2.4.6
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-iio"
>> > in the body of a message to majordomo@vger.kernel.org More
>> majordomo
>> > info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/7] iio:accel:stk8312: improve error handling

[Hartmut Knaack]

Breana, Tiberiu A schrieb am 30.07.2015 um 11:07:
>> -----Original Message-----
>> From: Crt Mori [mailto:cmo@melexis.com]
>> Sent: Thursday, July 30, 2015 10:04 AM
>> To: Hartmut Knaack
>> Cc: linux-iio@vger.kernel.org; Jonathan Cameron; Lars-Peter Clausen; Peter
>> Meerwald; Breana, Tiberiu A
>> Subject: Re: [PATCH v2 3/7] iio:accel:stk8312: improve error handling
>>
>> Hi, some comments/questions below:
>>
>> On 29 July 2015 at 23:39, Hartmut Knaack <knaack.h@gmx.de> wrote:
>>> Improve error handling in the following ways:
>>>   - set return value on error condition to an appropriate error code
>>>   - return error code immediately in case of an error (slightly changes
>>>     code structure)
>>>   - pass up real error code
>>>   - add missing error handling
>>>   - return 0 when error have been caught already
>>>   - put device back in active mode after error occurs
>>>
>>> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
> 
> Comments inline.
> 
>>> ---
>>>  drivers/iio/accel/stk8312.c | 60
>>> ++++++++++++++++++++++++++++-----------------
>>>  1 file changed, 38 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c
>>> index 6592be8e6377..4e1dda7e896d 100644
>>> --- a/drivers/iio/accel/stk8312.c
>>> +++ b/drivers/iio/accel/stk8312.c
>>> @@ -146,8 +146,10 @@ static int stk8312_otp_init(struct stk8312_data
>> *data)
>>>                 count--;
>>>         } while (!(ret & 0x80) && count > 0);
>>>
>>> -       if (count == 0)
>>> +       if (count == 0) {
>>> +               ret = -ETIMEDOUT;
>>>                 goto exit_err;
>>> +       }
>> You dont need braces since it is a one word statement or add a dev_err()
>> report.
> 

It's two lines of code to be executed if this condition is true, so
braces are needed. The dev_err() however is located under exit_err,
which is the reason for the goto, instead of directly returning.

> +1
> 
>>>
>>>         ret = i2c_smbus_read_byte_data(client, STK8312_REG_OTPDATA);
>>>         if (ret == 0)
>>> @@ -161,7 +163,7 @@ static int stk8312_otp_init(struct stk8312_data
>> *data)
>>>                 goto exit_err;
>> Why dont we return here as well?
> 
> I'm not sure I follow.
> 
>>>         msleep(150);
>>>
>>> -       return ret;
>>> +       return 0;
>>>
>>>  exit_err:
>>>         dev_err(&client->dev, "failed to initialize sensor\n"); @@
>>> -205,8 +207,11 @@ static int stk8312_set_interrupts(struct stk8312_data
>> *data, u8 int_mask)
>>>                 return ret;
>>>
>>>         ret = i2c_smbus_write_byte_data(client, STK8312_REG_INTSU,
>> int_mask);
>>> -       if (ret < 0)
>>> +       if (ret < 0) {
>>>                 dev_err(&client->dev, "failed to set interrupts\n");
>>> +               stk8312_set_mode(data, mode);
>>> +               return ret;
>>> +       }
>>>
>>>         return stk8312_set_mode(data, mode);  } @@ -230,7 +235,7 @@
>>> static int stk8312_data_rdy_trigger_set_state(struct iio_trigger
>>> *trig,
>>>
>>>         data->dready_trigger_on = state;
>>>
>>> -       return ret;
>>> +       return 0;
>>>  }
>>>
>>>  static const struct iio_trigger_ops stk8312_trigger_ops = { @@
>>> -255,20 +260,24 @@ static int stk8312_set_sample_rate(struct
>> stk8312_data *data, int rate)
>>>                 return ret;
>>>
>>>         ret = i2c_smbus_read_byte_data(client, STK8312_REG_SR);
>>> -       if (ret < 0) {
>>> -               dev_err(&client->dev, "failed to set sampling rate\n");
>>> -               return ret;
>>> -       }
>>> +       if (ret < 0)
>>> +               goto err_activate;
>>>
>>>         masked_reg = (ret & (~STK8312_SR_MASK)) | rate;
>>>
>>>         ret = i2c_smbus_write_byte_data(client, STK8312_REG_SR,
>> masked_reg);
>>>         if (ret < 0)
>>> -               dev_err(&client->dev, "failed to set sampling rate\n");
>>> -       else
>>> -               data->sample_rate_idx = rate;
>>> +               goto err_activate;
>>> +
>>> +       data->sample_rate_idx = rate;
>>>
>>>         return stk8312_set_mode(data, mode);
>>> +
>>> +err_activate:
>>> +       dev_err(&client->dev, "failed to set sampling rate\n");
>>> +       stk8312_set_mode(data, mode);
>>> +
>>> +       return ret;
>>>  }
>>>
>>>  static int stk8312_set_range(struct stk8312_data *data, u8 range) @@
>>> -290,21 +299,25 @@ static int stk8312_set_range(struct stk8312_data
>> *data, u8 range)
>>>                 return ret;
>>>
>>>         ret = i2c_smbus_read_byte_data(client, STK8312_REG_STH);
>>> -       if (ret < 0) {
>>> -               dev_err(&client->dev, "failed to change sensor range\n");
>>> -               return ret;
>>> -       }
>>> +       if (ret < 0)
>>> +               goto err_activate;
>>>
>>>         masked_reg = ret & (~STK8312_RNG_MASK);
>>>         masked_reg |= range << STK8312_RNG_SHIFT;
>>>
>>>         ret = i2c_smbus_write_byte_data(client, STK8312_REG_STH,
>> masked_reg);
>>>         if (ret < 0)
>> So in case of error we print error, but continue without returning error value,
>> but if it is a success then we go to err_activate and return a positive value?
>> Either label is
>> confusing or the whole process.
> 
> I suggest you take a closer look.
> 
>>> -               dev_err(&client->dev, "failed to change sensor range\n");
>>> -       else
>>> -               data->range = range;
>>> +               goto err_activate;
>>> +
>>> +       data->range = range;
>>>
>>>         return stk8312_set_mode(data, mode);
>>> +
>>> +err_activate:
>>> +       dev_err(&client->dev, "failed to change sensor range\n");
>>> +       stk8312_set_mode(data, mode);
>>> +
>>> +       return ret;
>>>  }
>>>
>>>  static int stk8312_read_accel(struct stk8312_data *data, u8 address)
>>> @@ -337,18 +350,21 @@ static int stk8312_read_raw(struct iio_dev
>> *indio_dev,
>>>                 ret = stk8312_set_mode(data, data->mode |
>> STK8312_MODE_ACTIVE);
>>>                 if (ret < 0) {
>>>                         mutex_unlock(&data->lock);
>>> -                       return -EINVAL;
>>> +                       return ret;
>>>                 }
>>>                 ret = stk8312_read_accel(data, chan->address);
>>>                 if (ret < 0) {
>>>                         stk8312_set_mode(data,
>>>                                          data->mode & (~STK8312_MODE_ACTIVE));
>>>                         mutex_unlock(&data->lock);
>>> -                       return -EINVAL;
>>> +                       return ret;
>>>                 }
>>>                 *val = sign_extend32(ret, 7);
>>> -               stk8312_set_mode(data, data->mode &
>> (~STK8312_MODE_ACTIVE));
>>> +               ret = stk8312_set_mode(data,
>>> +                                      data->mode &
>>> + (~STK8312_MODE_ACTIVE));
>>>                 mutex_unlock(&data->lock);
>>> +               if (ret < 0)
>>> +                       return ret;
> 
> Hartmut, I stand by my comment from the first patch version.
> We shouldn't return a read error if we're not able to put the
> sensor back in standby mode after reading the values.
> 

I don't know how often you would usually encounter problems
when setting the device in standby mode. Since it is just
a single I2C byte write, that should never fail, unless there
is some serious host adapter-, bus- or client-failure, which
qualifies to be reported. 
This is handled similarly in the following drivers, as well 
(probably some more):
https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/tree/drivers/iio/accel/bmc150-accel.c?h=testing#n643
https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/tree/drivers/iio/accel/kxcjk-1013.c?h=testing#n713
https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/tree/drivers/iio/gyro/bmg160.c?h=testing#n512
https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/tree/drivers/iio/light/jsa1212.c?h=testing#n218

>>>                 return IIO_VAL_INT;
>>>         case IIO_CHAN_INFO_SCALE:
>>>                 *val = stk8312_scale_table[data->range - 1][0]; @@
>>> -608,7 +624,7 @@ static int stk8312_probe(struct i2c_client *client,
>>>                 goto err_buffer_cleanup;
>>>         }
>>>
>>> -       return ret;
>>> +       return 0;
>>>
>>>  err_buffer_cleanup:
>>>         iio_triggered_buffer_cleanup(indio_dev);
>>> --
>>> 2.4.6
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio"
>>> in the body of a message to majordomo@vger.kernel.org More
>> majordomo
>>> info at  http://vger.kernel.org/majordomo-info.html
> N�����r��y���b�X��ǧv�^�)޺{.n�+����{��*"��^n�r���z�\x1a��h����&��\x1e�G���h�\x03(�階�ݢj"��\x1a�^[m�����z�ޖ���f���h���~�mml==
> 

Re: [PATCH v2 3/7] iio:accel:stk8312: improve error handling

[Crt Mori]

On 1 August 2015 at 01:45, Hartmut Knaack <knaack.h@gmx.de> wrote:
> Breana, Tiberiu A schrieb am 30.07.2015 um 11:07:
>>> -----Original Message-----
>>> From: Crt Mori [mailto:cmo@melexis.com]
>>> Sent: Thursday, July 30, 2015 10:04 AM
>>> To: Hartmut Knaack
>>> Cc: linux-iio@vger.kernel.org; Jonathan Cameron; Lars-Peter Clausen; Peter
>>> Meerwald; Breana, Tiberiu A
>>> Subject: Re: [PATCH v2 3/7] iio:accel:stk8312: improve error handling
>>>
>>> Hi, some comments/questions below:
>>>
>>> On 29 July 2015 at 23:39, Hartmut Knaack <knaack.h@gmx.de> wrote:
>>>> Improve error handling in the following ways:
>>>>   - set return value on error condition to an appropriate error code
>>>>   - return error code immediately in case of an error (slightly changes
>>>>     code structure)
>>>>   - pass up real error code
>>>>   - add missing error handling
>>>>   - return 0 when error have been caught already
>>>>   - put device back in active mode after error occurs
>>>>
>>>> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
>>
>> Comments inline.
>>
>>>> ---
>>>>  drivers/iio/accel/stk8312.c | 60
>>>> ++++++++++++++++++++++++++++-----------------
>>>>  1 file changed, 38 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c
>>>> index 6592be8e6377..4e1dda7e896d 100644
>>>> --- a/drivers/iio/accel/stk8312.c
>>>> +++ b/drivers/iio/accel/stk8312.c
>>>> @@ -146,8 +146,10 @@ static int stk8312_otp_init(struct stk8312_data
>>> *data)
>>>>                 count--;
>>>>         } while (!(ret & 0x80) && count > 0);
>>>>
>>>> -       if (count == 0)
>>>> +       if (count == 0) {
>>>> +               ret = -ETIMEDOUT;
>>>>                 goto exit_err;
>>>> +       }
>>> You dont need braces since it is a one word statement or add a dev_err()
>>> report.
>>
>> +1
>
> It's two lines of code to be executed if this condition is true, so
> braces are needed. The dev_err() however is located under exit_err,
> which is the reason for the goto, instead of directly returning.
>
Agreed - I have again misread the diff, same as Tiberiu. Sorry for this.
>>
>>>>
>>>>         ret = i2c_smbus_read_byte_data(client, STK8312_REG_OTPDATA);
>>>>         if (ret == 0)
>>>> @@ -161,7 +163,7 @@ static int stk8312_otp_init(struct stk8312_data
>>> *data)
>>>>                 goto exit_err;
>>> Why dont we return here as well?
>>
>> I'm not sure I follow.
>>
>>>>         msleep(150);
>>>>
>>>> -       return ret;
>>>> +       return 0;
>>>>
>>>>  exit_err:
>>>>         dev_err(&client->dev, "failed to initialize sensor\n"); @@
>>>> -205,8 +207,11 @@ static int stk8312_set_interrupts(struct stk8312_data
>>> *data, u8 int_mask)
>>>>                 return ret;
>>>>
>>>>         ret = i2c_smbus_write_byte_data(client, STK8312_REG_INTSU,
>>> int_mask);
>>>> -       if (ret < 0)
>>>> +       if (ret < 0) {
>>>>                 dev_err(&client->dev, "failed to set interrupts\n");
>>>> +               stk8312_set_mode(data, mode);
>>>> +               return ret;
>>>> +       }
>>>>
>>>>         return stk8312_set_mode(data, mode);  } @@ -230,7 +235,7 @@
>>>> static int stk8312_data_rdy_trigger_set_state(struct iio_trigger
>>>> *trig,
>>>>
>>>>         data->dready_trigger_on = state;
>>>>
>>>> -       return ret;
>>>> +       return 0;
>>>>  }
>>>>
>>>>  static const struct iio_trigger_ops stk8312_trigger_ops = { @@
>>>> -255,20 +260,24 @@ static int stk8312_set_sample_rate(struct
>>> stk8312_data *data, int rate)
>>>>                 return ret;
>>>>
>>>>         ret = i2c_smbus_read_byte_data(client, STK8312_REG_SR);
>>>> -       if (ret < 0) {
>>>> -               dev_err(&client->dev, "failed to set sampling rate\n");
>>>> -               return ret;
>>>> -       }
>>>> +       if (ret < 0)
>>>> +               goto err_activate;
>>>>
>>>>         masked_reg = (ret & (~STK8312_SR_MASK)) | rate;
>>>>
>>>>         ret = i2c_smbus_write_byte_data(client, STK8312_REG_SR,
>>> masked_reg);
>>>>         if (ret < 0)
>>>> -               dev_err(&client->dev, "failed to set sampling rate\n");
>>>> -       else
>>>> -               data->sample_rate_idx = rate;
>>>> +               goto err_activate;
>>>> +
>>>> +       data->sample_rate_idx = rate;
>>>>
>>>>         return stk8312_set_mode(data, mode);
>>>> +
>>>> +err_activate:
>>>> +       dev_err(&client->dev, "failed to set sampling rate\n");
>>>> +       stk8312_set_mode(data, mode);
>>>> +
>>>> +       return ret;
>>>>  }
>>>>
>>>>  static int stk8312_set_range(struct stk8312_data *data, u8 range) @@
>>>> -290,21 +299,25 @@ static int stk8312_set_range(struct stk8312_data
>>> *data, u8 range)
>>>>                 return ret;
>>>>
>>>>         ret = i2c_smbus_read_byte_data(client, STK8312_REG_STH);
>>>> -       if (ret < 0) {
>>>> -               dev_err(&client->dev, "failed to change sensor range\n");
>>>> -               return ret;
>>>> -       }
>>>> +       if (ret < 0)
>>>> +               goto err_activate;
>>>>
>>>>         masked_reg = ret & (~STK8312_RNG_MASK);
>>>>         masked_reg |= range << STK8312_RNG_SHIFT;
>>>>
>>>>         ret = i2c_smbus_write_byte_data(client, STK8312_REG_STH,
>>> masked_reg);
>>>>         if (ret < 0)
>>> So in case of error we print error, but continue without returning error value,
>>> but if it is a success then we go to err_activate and return a positive value?
>>> Either label is
>>> confusing or the whole process.
>>
>> I suggest you take a closer look.
>>
>>>> -               dev_err(&client->dev, "failed to change sensor range\n");
>>>> -       else
>>>> -               data->range = range;
>>>> +               goto err_activate;
>>>> +
>>>> +       data->range = range;
>>>>
>>>>         return stk8312_set_mode(data, mode);
>>>> +
>>>> +err_activate:
>>>> +       dev_err(&client->dev, "failed to change sensor range\n");
>>>> +       stk8312_set_mode(data, mode);
>>>> +
>>>> +       return ret;
>>>>  }
>>>>
>>>>  static int stk8312_read_accel(struct stk8312_data *data, u8 address)
>>>> @@ -337,18 +350,21 @@ static int stk8312_read_raw(struct iio_dev
>>> *indio_dev,
>>>>                 ret = stk8312_set_mode(data, data->mode |
>>> STK8312_MODE_ACTIVE);
>>>>                 if (ret < 0) {
>>>>                         mutex_unlock(&data->lock);
>>>> -                       return -EINVAL;
>>>> +                       return ret;
>>>>                 }
>>>>                 ret = stk8312_read_accel(data, chan->address);
>>>>                 if (ret < 0) {
>>>>                         stk8312_set_mode(data,
>>>>                                          data->mode & (~STK8312_MODE_ACTIVE));
>>>>                         mutex_unlock(&data->lock);
>>>> -                       return -EINVAL;
>>>> +                       return ret;
>>>>                 }
>>>>                 *val = sign_extend32(ret, 7);
>>>> -               stk8312_set_mode(data, data->mode &
>>> (~STK8312_MODE_ACTIVE));
>>>> +               ret = stk8312_set_mode(data,
>>>> +                                      data->mode &
>>>> + (~STK8312_MODE_ACTIVE));
>>>>                 mutex_unlock(&data->lock);
>>>> +               if (ret < 0)
>>>> +                       return ret;
>>
>> Hartmut, I stand by my comment from the first patch version.
>> We shouldn't return a read error if we're not able to put the
>> sensor back in standby mode after reading the values.
>>
>
> I don't know how often you would usually encounter problems
> when setting the device in standby mode. Since it is just
> a single I2C byte write, that should never fail, unless there
> is some serious host adapter-, bus- or client-failure, which
> qualifies to be reported.
> This is handled similarly in the following drivers, as well
> (probably some more):
> https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/tree/drivers/iio/accel/bmc150-accel.c?h=testing#n643
> https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/tree/drivers/iio/accel/kxcjk-1013.c?h=testing#n713
> https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/tree/drivers/iio/gyro/bmg160.c?h=testing#n512
> https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/tree/drivers/iio/light/jsa1212.c?h=testing#n218
>
>>>>                 return IIO_VAL_INT;
>>>>         case IIO_CHAN_INFO_SCALE:
>>>>                 *val = stk8312_scale_table[data->range - 1][0]; @@
>>>> -608,7 +624,7 @@ static int stk8312_probe(struct i2c_client *client,
>>>>                 goto err_buffer_cleanup;
>>>>         }
>>>>
>>>> -       return ret;
>>>> +       return 0;
>>>>
>>>>  err_buffer_cleanup:
>>>>         iio_triggered_buffer_cleanup(indio_dev);
>>>> --
>>>> 2.4.6
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-iio"
>>>> in the body of a message to majordomo@vger.kernel.org More
>>> majordomo
>>>> info at  http://vger.kernel.org/majordomo-info.html
>> N�����r��y���b�X��ǧv�^�)޺{.n�+����{��*"��^n�r���z� ��h����&�� �G���h� (�階�ݢj"�� � m�����z�ޖ���f���h���~�mml==
>>
>

RE: [PATCH v2 3/7] iio:accel:stk8312: improve error handling

[Breana, Tiberiu A]

PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBDcnQgTW9yaSBbbWFpbHRvOmNt
b0BtZWxleGlzLmNvbV0NCj4gU2VudDogU2F0dXJkYXksIEF1Z3VzdCAxLCAyMDE1IDEwOjQxIEFN
DQo+IFRvOiBIYXJ0bXV0IEtuYWFjaw0KPiBDYzogQnJlYW5hLCBUaWJlcml1IEE7IGxpbnV4LWlp
b0B2Z2VyLmtlcm5lbC5vcmc7IEpvbmF0aGFuIENhbWVyb247IExhcnMtDQo+IFBldGVyIENsYXVz
ZW47IFBldGVyIE1lZXJ3YWxkDQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggdjIgMy83XSBpaW86YWNj
ZWw6c3RrODMxMjogaW1wcm92ZSBlcnJvciBoYW5kbGluZw0KPiANCj4gT24gMSBBdWd1c3QgMjAx
NSBhdCAwMTo0NSwgSGFydG11dCBLbmFhY2sgPGtuYWFjay5oQGdteC5kZT4gd3JvdGU6DQo+ID4g
QnJlYW5hLCBUaWJlcml1IEEgc2NocmllYiBhbSAzMC4wNy4yMDE1IHVtIDExOjA3Og0KPiA+Pj4g
LS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gPj4+IEZyb206IENydCBNb3JpIFttYWlsdG86
Y21vQG1lbGV4aXMuY29tXQ0KPiA+Pj4gU2VudDogVGh1cnNkYXksIEp1bHkgMzAsIDIwMTUgMTA6
MDQgQU0NCj4gPj4+IFRvOiBIYXJ0bXV0IEtuYWFjaw0KPiA+Pj4gQ2M6IGxpbnV4LWlpb0B2Z2Vy
Lmtlcm5lbC5vcmc7IEpvbmF0aGFuIENhbWVyb247IExhcnMtUGV0ZXIgQ2xhdXNlbjsNCj4gPj4+
IFBldGVyIE1lZXJ3YWxkOyBCcmVhbmEsIFRpYmVyaXUgQQ0KPiA+Pj4gU3ViamVjdDogUmU6IFtQ
QVRDSCB2MiAzLzddIGlpbzphY2NlbDpzdGs4MzEyOiBpbXByb3ZlIGVycm9yDQo+ID4+PiBoYW5k
bGluZw0KPiA+Pj4NCj4gPj4+IEhpLCBzb21lIGNvbW1lbnRzL3F1ZXN0aW9ucyBiZWxvdzoNCj4g
Pj4+DQo+ID4+PiBPbiAyOSBKdWx5IDIwMTUgYXQgMjM6MzksIEhhcnRtdXQgS25hYWNrIDxrbmFh
Y2suaEBnbXguZGU+IHdyb3RlOg0KPiA+Pj4+IEltcHJvdmUgZXJyb3IgaGFuZGxpbmcgaW4gdGhl
IGZvbGxvd2luZyB3YXlzOg0KPiA+Pj4+ICAgLSBzZXQgcmV0dXJuIHZhbHVlIG9uIGVycm9yIGNv
bmRpdGlvbiB0byBhbiBhcHByb3ByaWF0ZSBlcnJvciBjb2RlDQo+ID4+Pj4gICAtIHJldHVybiBl
cnJvciBjb2RlIGltbWVkaWF0ZWx5IGluIGNhc2Ugb2YgYW4gZXJyb3IgKHNsaWdodGx5IGNoYW5n
ZXMNCj4gPj4+PiAgICAgY29kZSBzdHJ1Y3R1cmUpDQo+ID4+Pj4gICAtIHBhc3MgdXAgcmVhbCBl
cnJvciBjb2RlDQo+ID4+Pj4gICAtIGFkZCBtaXNzaW5nIGVycm9yIGhhbmRsaW5nDQo+ID4+Pj4g
ICAtIHJldHVybiAwIHdoZW4gZXJyb3IgaGF2ZSBiZWVuIGNhdWdodCBhbHJlYWR5DQo+ID4+Pj4g
ICAtIHB1dCBkZXZpY2UgYmFjayBpbiBhY3RpdmUgbW9kZSBhZnRlciBlcnJvciBvY2N1cnMNCj4g
Pj4+Pg0KPiA+Pj4+IFNpZ25lZC1vZmYtYnk6IEhhcnRtdXQgS25hYWNrIDxrbmFhY2suaEBnbXgu
ZGU+DQo+ID4+DQo+ID4+IENvbW1lbnRzIGlubGluZS4NCj4gPj4NCj4gPj4+PiAtLS0NCj4gPj4+
PiAgZHJpdmVycy9paW8vYWNjZWwvc3RrODMxMi5jIHwgNjANCj4gPj4+PiArKysrKysrKysrKysr
KysrKysrKysrKysrKysrLS0tLS0tLS0tLS0tLS0tLS0NCj4gPj4+PiAgMSBmaWxlIGNoYW5nZWQs
IDM4IGluc2VydGlvbnMoKyksIDIyIGRlbGV0aW9ucygtKQ0KPiA+Pj4+DQo+ID4+Pj4gZGlmZiAt
LWdpdCBhL2RyaXZlcnMvaWlvL2FjY2VsL3N0azgzMTIuYw0KPiA+Pj4+IGIvZHJpdmVycy9paW8v
YWNjZWwvc3RrODMxMi5jIGluZGV4IDY1OTJiZThlNjM3Ny4uNGUxZGRhN2U4OTZkDQo+ID4+Pj4g
MTAwNjQ0DQo+ID4+Pj4gLS0tIGEvZHJpdmVycy9paW8vYWNjZWwvc3RrODMxMi5jDQo+ID4+Pj4g
KysrIGIvZHJpdmVycy9paW8vYWNjZWwvc3RrODMxMi5jDQo+ID4+Pj4gQEAgLTE0Niw4ICsxNDYs
MTAgQEAgc3RhdGljIGludCBzdGs4MzEyX290cF9pbml0KHN0cnVjdA0KPiA+Pj4+IHN0azgzMTJf
ZGF0YQ0KPiA+Pj4gKmRhdGEpDQo+ID4+Pj4gICAgICAgICAgICAgICAgIGNvdW50LS07DQo+ID4+
Pj4gICAgICAgICB9IHdoaWxlICghKHJldCAmIDB4ODApICYmIGNvdW50ID4gMCk7DQo+ID4+Pj4N
Cj4gPj4+PiAtICAgICAgIGlmIChjb3VudCA9PSAwKQ0KPiA+Pj4+ICsgICAgICAgaWYgKGNvdW50
ID09IDApIHsNCj4gPj4+PiArICAgICAgICAgICAgICAgcmV0ID0gLUVUSU1FRE9VVDsNCj4gPj4+
PiAgICAgICAgICAgICAgICAgZ290byBleGl0X2VycjsNCj4gPj4+PiArICAgICAgIH0NCj4gPj4+
IFlvdSBkb250IG5lZWQgYnJhY2VzIHNpbmNlIGl0IGlzIGEgb25lIHdvcmQgc3RhdGVtZW50IG9y
IGFkZCBhDQo+ID4+PiBkZXZfZXJyKCkgcmVwb3J0Lg0KPiA+Pg0KPiA+PiArMQ0KPiA+DQo+ID4g
SXQncyB0d28gbGluZXMgb2YgY29kZSB0byBiZSBleGVjdXRlZCBpZiB0aGlzIGNvbmRpdGlvbiBp
cyB0cnVlLCBzbw0KPiA+IGJyYWNlcyBhcmUgbmVlZGVkLiBUaGUgZGV2X2VycigpIGhvd2V2ZXIg
aXMgbG9jYXRlZCB1bmRlciBleGl0X2VyciwNCj4gPiB3aGljaCBpcyB0aGUgcmVhc29uIGZvciB0
aGUgZ290bywgaW5zdGVhZCBvZiBkaXJlY3RseSByZXR1cm5pbmcuDQo+ID4NCj4gQWdyZWVkIC0g
SSBoYXZlIGFnYWluIG1pc3JlYWQgdGhlIGRpZmYsIHNhbWUgYXMgVGliZXJpdS4gU29ycnkgZm9y
IHRoaXMuDQo+ID4+DQo+ID4+Pj4NCj4gPj4+PiAgICAgICAgIHJldCA9IGkyY19zbWJ1c19yZWFk
X2J5dGVfZGF0YShjbGllbnQsIFNUSzgzMTJfUkVHX09UUERBVEEpOw0KPiA+Pj4+ICAgICAgICAg
aWYgKHJldCA9PSAwKQ0KPiA+Pj4+IEBAIC0xNjEsNyArMTYzLDcgQEAgc3RhdGljIGludCBzdGs4
MzEyX290cF9pbml0KHN0cnVjdCBzdGs4MzEyX2RhdGENCj4gPj4+ICpkYXRhKQ0KPiA+Pj4+ICAg
ICAgICAgICAgICAgICBnb3RvIGV4aXRfZXJyOw0KPiA+Pj4gV2h5IGRvbnQgd2UgcmV0dXJuIGhl
cmUgYXMgd2VsbD8NCj4gPj4NCj4gPj4gSSdtIG5vdCBzdXJlIEkgZm9sbG93Lg0KPiA+Pg0KPiA+
Pj4+ICAgICAgICAgbXNsZWVwKDE1MCk7DQo+ID4+Pj4NCj4gPj4+PiAtICAgICAgIHJldHVybiBy
ZXQ7DQo+ID4+Pj4gKyAgICAgICByZXR1cm4gMDsNCj4gPj4+Pg0KPiA+Pj4+ICBleGl0X2VycjoN
Cj4gPj4+PiAgICAgICAgIGRldl9lcnIoJmNsaWVudC0+ZGV2LCAiZmFpbGVkIHRvIGluaXRpYWxp
emUgc2Vuc29yXG4iKTsgQEANCj4gPj4+PiAtMjA1LDggKzIwNywxMSBAQCBzdGF0aWMgaW50IHN0
azgzMTJfc2V0X2ludGVycnVwdHMoc3RydWN0DQo+ID4+Pj4gc3RrODMxMl9kYXRhDQo+ID4+PiAq
ZGF0YSwgdTggaW50X21hc2spDQo+ID4+Pj4gICAgICAgICAgICAgICAgIHJldHVybiByZXQ7DQo+
ID4+Pj4NCj4gPj4+PiAgICAgICAgIHJldCA9IGkyY19zbWJ1c193cml0ZV9ieXRlX2RhdGEoY2xp
ZW50LCBTVEs4MzEyX1JFR19JTlRTVSwNCj4gPj4+IGludF9tYXNrKTsNCj4gPj4+PiAtICAgICAg
IGlmIChyZXQgPCAwKQ0KPiA+Pj4+ICsgICAgICAgaWYgKHJldCA8IDApIHsNCj4gPj4+PiAgICAg
ICAgICAgICAgICAgZGV2X2VycigmY2xpZW50LT5kZXYsICJmYWlsZWQgdG8gc2V0DQo+ID4+Pj4g
aW50ZXJydXB0c1xuIik7DQo+ID4+Pj4gKyAgICAgICAgICAgICAgIHN0azgzMTJfc2V0X21vZGUo
ZGF0YSwgbW9kZSk7DQo+ID4+Pj4gKyAgICAgICAgICAgICAgIHJldHVybiByZXQ7DQo+ID4+Pj4g
KyAgICAgICB9DQo+ID4+Pj4NCj4gPj4+PiAgICAgICAgIHJldHVybiBzdGs4MzEyX3NldF9tb2Rl
KGRhdGEsIG1vZGUpOyAgfSBAQCAtMjMwLDcgKzIzNSw3IEBADQo+ID4+Pj4gc3RhdGljIGludCBz
dGs4MzEyX2RhdGFfcmR5X3RyaWdnZXJfc2V0X3N0YXRlKHN0cnVjdCBpaW9fdHJpZ2dlcg0KPiA+
Pj4+ICp0cmlnLA0KPiA+Pj4+DQo+ID4+Pj4gICAgICAgICBkYXRhLT5kcmVhZHlfdHJpZ2dlcl9v
biA9IHN0YXRlOw0KPiA+Pj4+DQo+ID4+Pj4gLSAgICAgICByZXR1cm4gcmV0Ow0KPiA+Pj4+ICsg
ICAgICAgcmV0dXJuIDA7DQo+ID4+Pj4gIH0NCj4gPj4+Pg0KPiA+Pj4+ICBzdGF0aWMgY29uc3Qg
c3RydWN0IGlpb190cmlnZ2VyX29wcyBzdGs4MzEyX3RyaWdnZXJfb3BzID0geyBAQA0KPiA+Pj4+
IC0yNTUsMjAgKzI2MCwyNCBAQCBzdGF0aWMgaW50IHN0azgzMTJfc2V0X3NhbXBsZV9yYXRlKHN0
cnVjdA0KPiA+Pj4gc3RrODMxMl9kYXRhICpkYXRhLCBpbnQgcmF0ZSkNCj4gPj4+PiAgICAgICAg
ICAgICAgICAgcmV0dXJuIHJldDsNCj4gPj4+Pg0KPiA+Pj4+ICAgICAgICAgcmV0ID0gaTJjX3Nt
YnVzX3JlYWRfYnl0ZV9kYXRhKGNsaWVudCwgU1RLODMxMl9SRUdfU1IpOw0KPiA+Pj4+IC0gICAg
ICAgaWYgKHJldCA8IDApIHsNCj4gPj4+PiAtICAgICAgICAgICAgICAgZGV2X2VycigmY2xpZW50
LT5kZXYsICJmYWlsZWQgdG8gc2V0IHNhbXBsaW5nIHJhdGVcbiIpOw0KPiA+Pj4+IC0gICAgICAg
ICAgICAgICByZXR1cm4gcmV0Ow0KPiA+Pj4+IC0gICAgICAgfQ0KPiA+Pj4+ICsgICAgICAgaWYg
KHJldCA8IDApDQo+ID4+Pj4gKyAgICAgICAgICAgICAgIGdvdG8gZXJyX2FjdGl2YXRlOw0KPiA+
Pj4+DQo+ID4+Pj4gICAgICAgICBtYXNrZWRfcmVnID0gKHJldCAmICh+U1RLODMxMl9TUl9NQVNL
KSkgfCByYXRlOw0KPiA+Pj4+DQo+ID4+Pj4gICAgICAgICByZXQgPSBpMmNfc21idXNfd3JpdGVf
Ynl0ZV9kYXRhKGNsaWVudCwgU1RLODMxMl9SRUdfU1IsDQo+ID4+PiBtYXNrZWRfcmVnKTsNCj4g
Pj4+PiAgICAgICAgIGlmIChyZXQgPCAwKQ0KPiA+Pj4+IC0gICAgICAgICAgICAgICBkZXZfZXJy
KCZjbGllbnQtPmRldiwgImZhaWxlZCB0byBzZXQgc2FtcGxpbmcgcmF0ZVxuIik7DQo+ID4+Pj4g
LSAgICAgICBlbHNlDQo+ID4+Pj4gLSAgICAgICAgICAgICAgIGRhdGEtPnNhbXBsZV9yYXRlX2lk
eCA9IHJhdGU7DQo+ID4+Pj4gKyAgICAgICAgICAgICAgIGdvdG8gZXJyX2FjdGl2YXRlOw0KPiA+
Pj4+ICsNCj4gPj4+PiArICAgICAgIGRhdGEtPnNhbXBsZV9yYXRlX2lkeCA9IHJhdGU7DQo+ID4+
Pj4NCj4gPj4+PiAgICAgICAgIHJldHVybiBzdGs4MzEyX3NldF9tb2RlKGRhdGEsIG1vZGUpOw0K
PiA+Pj4+ICsNCj4gPj4+PiArZXJyX2FjdGl2YXRlOg0KPiA+Pj4+ICsgICAgICAgZGV2X2Vycigm
Y2xpZW50LT5kZXYsICJmYWlsZWQgdG8gc2V0IHNhbXBsaW5nIHJhdGVcbiIpOw0KPiA+Pj4+ICsg
ICAgICAgc3RrODMxMl9zZXRfbW9kZShkYXRhLCBtb2RlKTsNCj4gPj4+PiArDQo+ID4+Pj4gKyAg
ICAgICByZXR1cm4gcmV0Ow0KPiA+Pj4+ICB9DQo+ID4+Pj4NCj4gPj4+PiAgc3RhdGljIGludCBz
dGs4MzEyX3NldF9yYW5nZShzdHJ1Y3Qgc3RrODMxMl9kYXRhICpkYXRhLCB1OCByYW5nZSkNCj4g
Pj4+PiBAQA0KPiA+Pj4+IC0yOTAsMjEgKzI5OSwyNSBAQCBzdGF0aWMgaW50IHN0azgzMTJfc2V0
X3JhbmdlKHN0cnVjdCBzdGs4MzEyX2RhdGENCj4gPj4+ICpkYXRhLCB1OCByYW5nZSkNCj4gPj4+
PiAgICAgICAgICAgICAgICAgcmV0dXJuIHJldDsNCj4gPj4+Pg0KPiA+Pj4+ICAgICAgICAgcmV0
ID0gaTJjX3NtYnVzX3JlYWRfYnl0ZV9kYXRhKGNsaWVudCwgU1RLODMxMl9SRUdfU1RIKTsNCj4g
Pj4+PiAtICAgICAgIGlmIChyZXQgPCAwKSB7DQo+ID4+Pj4gLSAgICAgICAgICAgICAgIGRldl9l
cnIoJmNsaWVudC0+ZGV2LCAiZmFpbGVkIHRvIGNoYW5nZSBzZW5zb3IgcmFuZ2VcbiIpOw0KPiA+
Pj4+IC0gICAgICAgICAgICAgICByZXR1cm4gcmV0Ow0KPiA+Pj4+IC0gICAgICAgfQ0KPiA+Pj4+
ICsgICAgICAgaWYgKHJldCA8IDApDQo+ID4+Pj4gKyAgICAgICAgICAgICAgIGdvdG8gZXJyX2Fj
dGl2YXRlOw0KPiA+Pj4+DQo+ID4+Pj4gICAgICAgICBtYXNrZWRfcmVnID0gcmV0ICYgKH5TVEs4
MzEyX1JOR19NQVNLKTsNCj4gPj4+PiAgICAgICAgIG1hc2tlZF9yZWcgfD0gcmFuZ2UgPDwgU1RL
ODMxMl9STkdfU0hJRlQ7DQo+ID4+Pj4NCj4gPj4+PiAgICAgICAgIHJldCA9IGkyY19zbWJ1c193
cml0ZV9ieXRlX2RhdGEoY2xpZW50LCBTVEs4MzEyX1JFR19TVEgsDQo+ID4+PiBtYXNrZWRfcmVn
KTsNCj4gPj4+PiAgICAgICAgIGlmIChyZXQgPCAwKQ0KPiA+Pj4gU28gaW4gY2FzZSBvZiBlcnJv
ciB3ZSBwcmludCBlcnJvciwgYnV0IGNvbnRpbnVlIHdpdGhvdXQgcmV0dXJuaW5nDQo+ID4+PiBl
cnJvciB2YWx1ZSwgYnV0IGlmIGl0IGlzIGEgc3VjY2VzcyB0aGVuIHdlIGdvIHRvIGVycl9hY3Rp
dmF0ZSBhbmQgcmV0dXJuIGENCj4gcG9zaXRpdmUgdmFsdWU/DQo+ID4+PiBFaXRoZXIgbGFiZWwg
aXMNCj4gPj4+IGNvbmZ1c2luZyBvciB0aGUgd2hvbGUgcHJvY2Vzcy4NCj4gPj4NCj4gPj4gSSBz
dWdnZXN0IHlvdSB0YWtlIGEgY2xvc2VyIGxvb2suDQo+ID4+DQo+ID4+Pj4gLSAgICAgICAgICAg
ICAgIGRldl9lcnIoJmNsaWVudC0+ZGV2LCAiZmFpbGVkIHRvIGNoYW5nZSBzZW5zb3IgcmFuZ2Vc
biIpOw0KPiA+Pj4+IC0gICAgICAgZWxzZQ0KPiA+Pj4+IC0gICAgICAgICAgICAgICBkYXRhLT5y
YW5nZSA9IHJhbmdlOw0KPiA+Pj4+ICsgICAgICAgICAgICAgICBnb3RvIGVycl9hY3RpdmF0ZTsN
Cj4gPj4+PiArDQo+ID4+Pj4gKyAgICAgICBkYXRhLT5yYW5nZSA9IHJhbmdlOw0KPiA+Pj4+DQo+
ID4+Pj4gICAgICAgICByZXR1cm4gc3RrODMxMl9zZXRfbW9kZShkYXRhLCBtb2RlKTsNCj4gPj4+
PiArDQo+ID4+Pj4gK2Vycl9hY3RpdmF0ZToNCj4gPj4+PiArICAgICAgIGRldl9lcnIoJmNsaWVu
dC0+ZGV2LCAiZmFpbGVkIHRvIGNoYW5nZSBzZW5zb3IgcmFuZ2VcbiIpOw0KPiA+Pj4+ICsgICAg
ICAgc3RrODMxMl9zZXRfbW9kZShkYXRhLCBtb2RlKTsNCj4gPj4+PiArDQo+ID4+Pj4gKyAgICAg
ICByZXR1cm4gcmV0Ow0KPiA+Pj4+ICB9DQo+ID4+Pj4NCj4gPj4+PiAgc3RhdGljIGludCBzdGs4
MzEyX3JlYWRfYWNjZWwoc3RydWN0IHN0azgzMTJfZGF0YSAqZGF0YSwgdTgNCj4gPj4+PiBhZGRy
ZXNzKSBAQCAtMzM3LDE4ICszNTAsMjEgQEAgc3RhdGljIGludCBzdGs4MzEyX3JlYWRfcmF3KHN0
cnVjdA0KPiA+Pj4+IGlpb19kZXYNCj4gPj4+ICppbmRpb19kZXYsDQo+ID4+Pj4gICAgICAgICAg
ICAgICAgIHJldCA9IHN0azgzMTJfc2V0X21vZGUoZGF0YSwgZGF0YS0+bW9kZSB8DQo+ID4+PiBT
VEs4MzEyX01PREVfQUNUSVZFKTsNCj4gPj4+PiAgICAgICAgICAgICAgICAgaWYgKHJldCA8IDAp
IHsNCj4gPj4+PiAgICAgICAgICAgICAgICAgICAgICAgICBtdXRleF91bmxvY2soJmRhdGEtPmxv
Y2spOw0KPiA+Pj4+IC0gICAgICAgICAgICAgICAgICAgICAgIHJldHVybiAtRUlOVkFMOw0KPiA+
Pj4+ICsgICAgICAgICAgICAgICAgICAgICAgIHJldHVybiByZXQ7DQo+ID4+Pj4gICAgICAgICAg
ICAgICAgIH0NCj4gPj4+PiAgICAgICAgICAgICAgICAgcmV0ID0gc3RrODMxMl9yZWFkX2FjY2Vs
KGRhdGEsIGNoYW4tPmFkZHJlc3MpOw0KPiA+Pj4+ICAgICAgICAgICAgICAgICBpZiAocmV0IDwg
MCkgew0KPiA+Pj4+ICAgICAgICAgICAgICAgICAgICAgICAgIHN0azgzMTJfc2V0X21vZGUoZGF0
YSwNCj4gPj4+PiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGRhdGEt
Pm1vZGUgJiAoflNUSzgzMTJfTU9ERV9BQ1RJVkUpKTsNCj4gPj4+PiAgICAgICAgICAgICAgICAg
ICAgICAgICBtdXRleF91bmxvY2soJmRhdGEtPmxvY2spOw0KPiA+Pj4+IC0gICAgICAgICAgICAg
ICAgICAgICAgIHJldHVybiAtRUlOVkFMOw0KPiA+Pj4+ICsgICAgICAgICAgICAgICAgICAgICAg
IHJldHVybiByZXQ7DQo+ID4+Pj4gICAgICAgICAgICAgICAgIH0NCj4gPj4+PiAgICAgICAgICAg
ICAgICAgKnZhbCA9IHNpZ25fZXh0ZW5kMzIocmV0LCA3KTsNCj4gPj4+PiAtICAgICAgICAgICAg
ICAgc3RrODMxMl9zZXRfbW9kZShkYXRhLCBkYXRhLT5tb2RlICYNCj4gPj4+ICh+U1RLODMxMl9N
T0RFX0FDVElWRSkpOw0KPiA+Pj4+ICsgICAgICAgICAgICAgICByZXQgPSBzdGs4MzEyX3NldF9t
b2RlKGRhdGEsDQo+ID4+Pj4gKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ZGF0YS0+bW9kZSAmDQo+ID4+Pj4gKyAoflNUSzgzMTJfTU9ERV9BQ1RJVkUpKTsNCj4gPj4+PiAg
ICAgICAgICAgICAgICAgbXV0ZXhfdW5sb2NrKCZkYXRhLT5sb2NrKTsNCj4gPj4+PiArICAgICAg
ICAgICAgICAgaWYgKHJldCA8IDApDQo+ID4+Pj4gKyAgICAgICAgICAgICAgICAgICAgICAgcmV0
dXJuIHJldDsNCj4gPj4NCj4gPj4gSGFydG11dCwgSSBzdGFuZCBieSBteSBjb21tZW50IGZyb20g
dGhlIGZpcnN0IHBhdGNoIHZlcnNpb24uDQo+ID4+IFdlIHNob3VsZG4ndCByZXR1cm4gYSByZWFk
IGVycm9yIGlmIHdlJ3JlIG5vdCBhYmxlIHRvIHB1dCB0aGUgc2Vuc29yDQo+ID4+IGJhY2sgaW4g
c3RhbmRieSBtb2RlIGFmdGVyIHJlYWRpbmcgdGhlIHZhbHVlcy4NCj4gPj4NCj4gPg0KPiA+IEkg
ZG9uJ3Qga25vdyBob3cgb2Z0ZW4geW91IHdvdWxkIHVzdWFsbHkgZW5jb3VudGVyIHByb2JsZW1z
IHdoZW4NCj4gPiBzZXR0aW5nIHRoZSBkZXZpY2UgaW4gc3RhbmRieSBtb2RlLiBTaW5jZSBpdCBp
cyBqdXN0IGEgc2luZ2xlIEkyQyBieXRlDQo+ID4gd3JpdGUsIHRoYXQgc2hvdWxkIG5ldmVyIGZh
aWwsIHVubGVzcyB0aGVyZSBpcyBzb21lIHNlcmlvdXMgaG9zdA0KPiA+IGFkYXB0ZXItLCBidXMt
IG9yIGNsaWVudC1mYWlsdXJlLCB3aGljaCBxdWFsaWZpZXMgdG8gYmUgcmVwb3J0ZWQuDQo+ID4g
VGhpcyBpcyBoYW5kbGVkIHNpbWlsYXJseSBpbiB0aGUgZm9sbG93aW5nIGRyaXZlcnMsIGFzIHdl
bGwgKHByb2JhYmx5DQo+ID4gc29tZSBtb3JlKToNCj4gPiBodHRwczovL2dpdC5rZXJuZWwub3Jn
L2NnaXQvbGludXgva2VybmVsL2dpdC9qaWMyMy9paW8uZ2l0L3RyZWUvZHJpdmVyDQo+ID4gcy9p
aW8vYWNjZWwvYm1jMTUwLWFjY2VsLmM/aD10ZXN0aW5nI242NDMNCj4gPiBodHRwczovL2dpdC5r
ZXJuZWwub3JnL2NnaXQvbGludXgva2VybmVsL2dpdC9qaWMyMy9paW8uZ2l0L3RyZWUvZHJpdmVy
DQo+ID4gcy9paW8vYWNjZWwva3hjamstMTAxMy5jP2g9dGVzdGluZyNuNzEzDQo+ID4gaHR0cHM6
Ly9naXQua2VybmVsLm9yZy9jZ2l0L2xpbnV4L2tlcm5lbC9naXQvamljMjMvaWlvLmdpdC90cmVl
L2RyaXZlcg0KPiA+IHMvaWlvL2d5cm8vYm1nMTYwLmM/aD10ZXN0aW5nI241MTINCj4gPiBodHRw
czovL2dpdC5rZXJuZWwub3JnL2NnaXQvbGludXgva2VybmVsL2dpdC9qaWMyMy9paW8uZ2l0L3Ry
ZWUvZHJpdmVyDQo+ID4gcy9paW8vbGlnaHQvanNhMTIxMi5jP2g9dGVzdGluZyNuMjE4DQo+ID4N
Cg0KT2ssIGlmIHRoYXQncyB0aGUgc3RhbmRhcmQgcHJvY2VkdXJlLCB0aGVuIHRoaXMgc2hvdWxk
IGJlIG5vIGRpZmZlcmVudC4NCisxDQoNCj4gPj4+PiAgICAgICAgICAgICAgICAgcmV0dXJuIElJ
T19WQUxfSU5UOw0KPiA+Pj4+ICAgICAgICAgY2FzZSBJSU9fQ0hBTl9JTkZPX1NDQUxFOg0KPiA+
Pj4+ICAgICAgICAgICAgICAgICAqdmFsID0gc3RrODMxMl9zY2FsZV90YWJsZVtkYXRhLT5yYW5n
ZSAtIDFdWzBdOyBAQA0KPiA+Pj4+IC02MDgsNyArNjI0LDcgQEAgc3RhdGljIGludCBzdGs4MzEy
X3Byb2JlKHN0cnVjdCBpMmNfY2xpZW50ICpjbGllbnQsDQo+ID4+Pj4gICAgICAgICAgICAgICAg
IGdvdG8gZXJyX2J1ZmZlcl9jbGVhbnVwOw0KPiA+Pj4+ICAgICAgICAgfQ0KPiA+Pj4+DQo+ID4+
Pj4gLSAgICAgICByZXR1cm4gcmV0Ow0KPiA+Pj4+ICsgICAgICAgcmV0dXJuIDA7DQo+ID4+Pj4N
Cj4gPj4+PiAgZXJyX2J1ZmZlcl9jbGVhbnVwOg0KPiA+Pj4+ICAgICAgICAgaWlvX3RyaWdnZXJl
ZF9idWZmZXJfY2xlYW51cChpbmRpb19kZXYpOw0KPiA+Pj4+IC0tDQo+ID4+Pj4gMi40LjYNCj4g
Pj4+Pg0KPiA+Pj4+IC0tDQo+ID4+Pj4gVG8gdW5zdWJzY3JpYmUgZnJvbSB0aGlzIGxpc3Q6IHNl
bmQgdGhlIGxpbmUgInVuc3Vic2NyaWJlIGxpbnV4LWlpbyINCj4gPj4+PiBpbiB0aGUgYm9keSBv
ZiBhIG1lc3NhZ2UgdG8gbWFqb3Jkb21vQHZnZXIua2VybmVsLm9yZyBNb3JlDQo+ID4+PiBtYWpv
cmRvbW8NCj4gPj4+PiBpbmZvIGF0ICBodHRwOi8vdmdlci5rZXJuZWwub3JnL21ham9yZG9tby1p
bmZvLmh0bWwNCj4gPj4gTiAgICAgciAgeSAgIGIgWCAgx6d2IF4gKd66ey5uICsgICAgeyAgKiIg
IF5uIHIgICB6ICAgIGggICAgJiAgICBHICAgaA0KPiA+PiAoIOmajiDdomoiICAgICBtICAgICB6
IN6WICAgZiAgIGggICB+IG1tbD09DQo+ID4+DQo+ID4NCg==

Re: [PATCH v2 3/7] iio:accel:stk8312: improve error handling

[Jonathan Cameron]

On 03/08/15 09:05, Breana, Tiberiu A wrote:
>> -----Original Message-----
>> From: Crt Mori [mailto:cmo@melexis.com]
>> Sent: Saturday, August 1, 2015 10:41 AM
>> To: Hartmut Knaack
>> Cc: Breana, Tiberiu A; linux-iio@vger.kernel.org; Jonathan Cameron; Lars-
>> Peter Clausen; Peter Meerwald
>> Subject: Re: [PATCH v2 3/7] iio:accel:stk8312: improve error handling
>>
>> On 1 August 2015 at 01:45, Hartmut Knaack <knaack.h@gmx.de> wrote:
>>> Breana, Tiberiu A schrieb am 30.07.2015 um 11:07:
>>>>> -----Original Message-----
>>>>> From: Crt Mori [mailto:cmo@melexis.com]
>>>>> Sent: Thursday, July 30, 2015 10:04 AM
>>>>> To: Hartmut Knaack
>>>>> Cc: linux-iio@vger.kernel.org; Jonathan Cameron; Lars-Peter Clausen;
>>>>> Peter Meerwald; Breana, Tiberiu A
>>>>> Subject: Re: [PATCH v2 3/7] iio:accel:stk8312: improve error
>>>>> handling
>>>>>
>>>>> Hi, some comments/questions below:
>>>>>
>>>>> On 29 July 2015 at 23:39, Hartmut Knaack <knaack.h@gmx.de> wrote:
>>>>>> Improve error handling in the following ways:
>>>>>>   - set return value on error condition to an appropriate error code
>>>>>>   - return error code immediately in case of an error (slightly changes
>>>>>>     code structure)
>>>>>>   - pass up real error code
>>>>>>   - add missing error handling
>>>>>>   - return 0 when error have been caught already
>>>>>>   - put device back in active mode after error occurs
>>>>>>
>>>>>> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
>>>>
>>>> Comments inline.
>>>>
>>>>>> ---
>>>>>>  drivers/iio/accel/stk8312.c | 60
>>>>>> ++++++++++++++++++++++++++++-----------------
>>>>>>  1 file changed, 38 insertions(+), 22 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/iio/accel/stk8312.c
>>>>>> b/drivers/iio/accel/stk8312.c index 6592be8e6377..4e1dda7e896d
>>>>>> 100644
>>>>>> --- a/drivers/iio/accel/stk8312.c
>>>>>> +++ b/drivers/iio/accel/stk8312.c
>>>>>> @@ -146,8 +146,10 @@ static int stk8312_otp_init(struct
>>>>>> stk8312_data
>>>>> *data)
>>>>>>                 count--;
>>>>>>         } while (!(ret & 0x80) && count > 0);
>>>>>>
>>>>>> -       if (count == 0)
>>>>>> +       if (count == 0) {
>>>>>> +               ret = -ETIMEDOUT;
>>>>>>                 goto exit_err;
>>>>>> +       }
>>>>> You dont need braces since it is a one word statement or add a
>>>>> dev_err() report.
>>>>
>>>> +1
>>>
>>> It's two lines of code to be executed if this condition is true, so
>>> braces are needed. The dev_err() however is located under exit_err,
>>> which is the reason for the goto, instead of directly returning.
>>>
>> Agreed - I have again misread the diff, same as Tiberiu. Sorry for this.
>>>>
>>>>>>
>>>>>>         ret = i2c_smbus_read_byte_data(client, STK8312_REG_OTPDATA);
>>>>>>         if (ret == 0)
>>>>>> @@ -161,7 +163,7 @@ static int stk8312_otp_init(struct stk8312_data
>>>>> *data)
>>>>>>                 goto exit_err;
>>>>> Why dont we return here as well?
>>>>
>>>> I'm not sure I follow.
>>>>
>>>>>>         msleep(150);
>>>>>>
>>>>>> -       return ret;
>>>>>> +       return 0;
>>>>>>
>>>>>>  exit_err:
>>>>>>         dev_err(&client->dev, "failed to initialize sensor\n"); @@
>>>>>> -205,8 +207,11 @@ static int stk8312_set_interrupts(struct
>>>>>> stk8312_data
>>>>> *data, u8 int_mask)
>>>>>>                 return ret;
>>>>>>
>>>>>>         ret = i2c_smbus_write_byte_data(client, STK8312_REG_INTSU,
>>>>> int_mask);
>>>>>> -       if (ret < 0)
>>>>>> +       if (ret < 0) {
>>>>>>                 dev_err(&client->dev, "failed to set
>>>>>> interrupts\n");
>>>>>> +               stk8312_set_mode(data, mode);
>>>>>> +               return ret;
>>>>>> +       }
>>>>>>
>>>>>>         return stk8312_set_mode(data, mode);  } @@ -230,7 +235,7 @@
>>>>>> static int stk8312_data_rdy_trigger_set_state(struct iio_trigger
>>>>>> *trig,
>>>>>>
>>>>>>         data->dready_trigger_on = state;
>>>>>>
>>>>>> -       return ret;
>>>>>> +       return 0;
>>>>>>  }
>>>>>>
>>>>>>  static const struct iio_trigger_ops stk8312_trigger_ops = { @@
>>>>>> -255,20 +260,24 @@ static int stk8312_set_sample_rate(struct
>>>>> stk8312_data *data, int rate)
>>>>>>                 return ret;
>>>>>>
>>>>>>         ret = i2c_smbus_read_byte_data(client, STK8312_REG_SR);
>>>>>> -       if (ret < 0) {
>>>>>> -               dev_err(&client->dev, "failed to set sampling rate\n");
>>>>>> -               return ret;
>>>>>> -       }
>>>>>> +       if (ret < 0)
>>>>>> +               goto err_activate;
>>>>>>
>>>>>>         masked_reg = (ret & (~STK8312_SR_MASK)) | rate;
>>>>>>
>>>>>>         ret = i2c_smbus_write_byte_data(client, STK8312_REG_SR,
>>>>> masked_reg);
>>>>>>         if (ret < 0)
>>>>>> -               dev_err(&client->dev, "failed to set sampling rate\n");
>>>>>> -       else
>>>>>> -               data->sample_rate_idx = rate;
>>>>>> +               goto err_activate;
>>>>>> +
>>>>>> +       data->sample_rate_idx = rate;
>>>>>>
>>>>>>         return stk8312_set_mode(data, mode);
>>>>>> +
>>>>>> +err_activate:
>>>>>> +       dev_err(&client->dev, "failed to set sampling rate\n");
>>>>>> +       stk8312_set_mode(data, mode);
>>>>>> +
>>>>>> +       return ret;
>>>>>>  }
>>>>>>
>>>>>>  static int stk8312_set_range(struct stk8312_data *data, u8 range)
>>>>>> @@
>>>>>> -290,21 +299,25 @@ static int stk8312_set_range(struct stk8312_data
>>>>> *data, u8 range)
>>>>>>                 return ret;
>>>>>>
>>>>>>         ret = i2c_smbus_read_byte_data(client, STK8312_REG_STH);
>>>>>> -       if (ret < 0) {
>>>>>> -               dev_err(&client->dev, "failed to change sensor range\n");
>>>>>> -               return ret;
>>>>>> -       }
>>>>>> +       if (ret < 0)
>>>>>> +               goto err_activate;
>>>>>>
>>>>>>         masked_reg = ret & (~STK8312_RNG_MASK);
>>>>>>         masked_reg |= range << STK8312_RNG_SHIFT;
>>>>>>
>>>>>>         ret = i2c_smbus_write_byte_data(client, STK8312_REG_STH,
>>>>> masked_reg);
>>>>>>         if (ret < 0)
>>>>> So in case of error we print error, but continue without returning
>>>>> error value, but if it is a success then we go to err_activate and return a
>> positive value?
>>>>> Either label is
>>>>> confusing or the whole process.
>>>>
>>>> I suggest you take a closer look.
>>>>
>>>>>> -               dev_err(&client->dev, "failed to change sensor range\n");
>>>>>> -       else
>>>>>> -               data->range = range;
>>>>>> +               goto err_activate;
>>>>>> +
>>>>>> +       data->range = range;
>>>>>>
>>>>>>         return stk8312_set_mode(data, mode);
>>>>>> +
>>>>>> +err_activate:
>>>>>> +       dev_err(&client->dev, "failed to change sensor range\n");
>>>>>> +       stk8312_set_mode(data, mode);
>>>>>> +
>>>>>> +       return ret;
>>>>>>  }
>>>>>>
>>>>>>  static int stk8312_read_accel(struct stk8312_data *data, u8
>>>>>> address) @@ -337,18 +350,21 @@ static int stk8312_read_raw(struct
>>>>>> iio_dev
>>>>> *indio_dev,
>>>>>>                 ret = stk8312_set_mode(data, data->mode |
>>>>> STK8312_MODE_ACTIVE);
>>>>>>                 if (ret < 0) {
>>>>>>                         mutex_unlock(&data->lock);
>>>>>> -                       return -EINVAL;
>>>>>> +                       return ret;
>>>>>>                 }
>>>>>>                 ret = stk8312_read_accel(data, chan->address);
>>>>>>                 if (ret < 0) {
>>>>>>                         stk8312_set_mode(data,
>>>>>>                                          data->mode & (~STK8312_MODE_ACTIVE));
>>>>>>                         mutex_unlock(&data->lock);
>>>>>> -                       return -EINVAL;
>>>>>> +                       return ret;
>>>>>>                 }
>>>>>>                 *val = sign_extend32(ret, 7);
>>>>>> -               stk8312_set_mode(data, data->mode &
>>>>> (~STK8312_MODE_ACTIVE));
>>>>>> +               ret = stk8312_set_mode(data,
>>>>>> +                                      data->mode &
>>>>>> + (~STK8312_MODE_ACTIVE));
>>>>>>                 mutex_unlock(&data->lock);
>>>>>> +               if (ret < 0)
>>>>>> +                       return ret;
>>>>
>>>> Hartmut, I stand by my comment from the first patch version.
>>>> We shouldn't return a read error if we're not able to put the sensor
>>>> back in standby mode after reading the values.
>>>>
>>>
>>> I don't know how often you would usually encounter problems when
>>> setting the device in standby mode. Since it is just a single I2C byte
>>> write, that should never fail, unless there is some serious host
>>> adapter-, bus- or client-failure, which qualifies to be reported.
>>> This is handled similarly in the following drivers, as well (probably
>>> some more):
>>> https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/tree/driver
>>> s/iio/accel/bmc150-accel.c?h=testing#n643
>>> https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/tree/driver
>>> s/iio/accel/kxcjk-1013.c?h=testing#n713
>>> https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/tree/driver
>>> s/iio/gyro/bmg160.c?h=testing#n512
>>> https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/tree/driver
>>> s/iio/light/jsa1212.c?h=testing#n218
>>>
> 
> Ok, if that's the standard procedure, then this should be no different.
> +1
Having divided through thread, unless I have missed something
I think this is a good improvement on the existing situation.
Please submit any further changes on top of this.

Applied to the togreg branch of iio.git

Thanks,

Jonathan
> 
>>>>>>                 return IIO_VAL_INT;
>>>>>>         case IIO_CHAN_INFO_SCALE:
>>>>>>                 *val = stk8312_scale_table[data->range - 1][0]; @@
>>>>>> -608,7 +624,7 @@ static int stk8312_probe(struct i2c_client *client,
>>>>>>                 goto err_buffer_cleanup;
>>>>>>         }
>>>>>>
>>>>>> -       return ret;
>>>>>> +       return 0;
>>>>>>
>>>>>>  err_buffer_cleanup:
>>>>>>         iio_triggered_buffer_cleanup(indio_dev);
>>>>>> --
>>>>>> 2.4.6
>>>>>>
>>>>>> --
>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-iio"
>>>>>> in the body of a message to majordomo@vger.kernel.org More
>>>>> majordomo
>>>>>> info at  http://vger.kernel.org/majordomo-info.html
>>>> N     r  y   b X  ǧv ^ )޺{.n +    {  *"  ^n r   z    h    &    G   h
>>>> ( 階 ݢj"     m     z ޖ   f   h   ~ mml==
>>>>
>>>
> N�����r��y���b�X��ǧv�^�)޺{.n�+����{��*"��^n�r���z�\x1a��h����&��\x1e�G���h�\x03(�階�ݢj"��\x1a�^[m�����z�ޖ���f���h���~�mml==
> 

[PATCH v2 4/7] iio:accel:stk8312: rework macro definitions

[Hartmut Knaack]

Make use of BIT to describe register bits, GENMASK for consecutive
bitmasks, rename and sort existing definitions, replace magic value with
an expressive definition, drop an unused definition.

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
Reviewed-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
---
 drivers/iio/accel/stk8312.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c
index 4e1dda7e896d..8807927d4d0b 100644
--- a/drivers/iio/accel/stk8312.c
+++ b/drivers/iio/accel/stk8312.c
@@ -37,16 +37,16 @@
 #define STK8312_REG_OTPDATA		0x3E
 #define STK8312_REG_OTPCTRL		0x3F
 
-#define STK8312_MODE_ACTIVE		0x01
+#define STK8312_MODE_ACTIVE		BIT(0)
 #define STK8312_MODE_STANDBY		0x00
-#define STK8312_DREADY_BIT		0x10
-#define STK8312_INT_MODE		0xC0
-#define STK8312_RNG_MASK		0xC0
-#define STK8312_SR_MASK			0x07
-#define STK8312_SR_400HZ_IDX		0
+#define STK8312_MODE_INT_AH_PP		0xC0	/* active-high, push-pull */
+#define STK8312_DREADY_BIT		BIT(4)
+#define STK8312_RNG_6G			1
 #define STK8312_RNG_SHIFT		6
-#define STK8312_READ_RETRIES		16
-#define STK8312_ALL_CHANNEL_MASK	7
+#define STK8312_RNG_MASK		GENMASK(7, 6)
+#define STK8312_SR_MASK			GENMASK(2, 0)
+#define STK8312_SR_400HZ_IDX		0
+#define STK8312_ALL_CHANNEL_MASK	GENMASK(2, 0)
 #define STK8312_ALL_CHANNEL_SIZE	3
 
 #define STK8312_DRIVER_NAME		"stk8312"
@@ -144,7 +144,7 @@ static int stk8312_otp_init(struct stk8312_data *data)
 		if (ret < 0)
 			goto exit_err;
 		count--;
-	} while (!(ret & 0x80) && count > 0);
+	} while (!(ret & BIT(7)) && count > 0);
 
 	if (count == 0) {
 		ret = -ETIMEDOUT;
@@ -565,11 +565,12 @@ static int stk8312_probe(struct i2c_client *client,
 		return ret;
 	}
 	data->sample_rate_idx = STK8312_SR_400HZ_IDX;
-	ret = stk8312_set_range(data, 1);
+	ret = stk8312_set_range(data, STK8312_RNG_6G);
 	if (ret < 0)
 		return ret;
 
-	ret = stk8312_set_mode(data, STK8312_INT_MODE | STK8312_MODE_ACTIVE);
+	ret = stk8312_set_mode(data,
+			       STK8312_MODE_INT_AH_PP | STK8312_MODE_ACTIVE);
 	if (ret < 0)
 		return ret;
 
@@ -690,7 +691,7 @@ MODULE_DEVICE_TABLE(acpi, stk8312_acpi_id);
 
 static struct i2c_driver stk8312_driver = {
 	.driver = {
-		.name = "stk8312",
+		.name = STK8312_DRIVER_NAME,
 		.pm = STK8312_PM_OPS,
 		.acpi_match_table = ACPI_PTR(stk8312_acpi_id),
 	},
-- 
2.4.6

Re: [PATCH v2 4/7] iio:accel:stk8312: rework macro definitions

[Jonathan Cameron]

On 29/07/15 22:39, Hartmut Knaack wrote:
> Make use of BIT to describe register bits, GENMASK for consecutive
> bitmasks, rename and sort existing definitions, replace magic value with
> an expressive definition, drop an unused definition.
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
> Reviewed-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
Applied to the togreg branch of iio.git.

Thanks,

Jonathan
> ---
>  drivers/iio/accel/stk8312.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c
> index 4e1dda7e896d..8807927d4d0b 100644
> --- a/drivers/iio/accel/stk8312.c
> +++ b/drivers/iio/accel/stk8312.c
> @@ -37,16 +37,16 @@
>  #define STK8312_REG_OTPDATA		0x3E
>  #define STK8312_REG_OTPCTRL		0x3F
>  
> -#define STK8312_MODE_ACTIVE		0x01
> +#define STK8312_MODE_ACTIVE		BIT(0)
>  #define STK8312_MODE_STANDBY		0x00
> -#define STK8312_DREADY_BIT		0x10
> -#define STK8312_INT_MODE		0xC0
> -#define STK8312_RNG_MASK		0xC0
> -#define STK8312_SR_MASK			0x07
> -#define STK8312_SR_400HZ_IDX		0
> +#define STK8312_MODE_INT_AH_PP		0xC0	/* active-high, push-pull */
> +#define STK8312_DREADY_BIT		BIT(4)
> +#define STK8312_RNG_6G			1
>  #define STK8312_RNG_SHIFT		6
> -#define STK8312_READ_RETRIES		16
> -#define STK8312_ALL_CHANNEL_MASK	7
> +#define STK8312_RNG_MASK		GENMASK(7, 6)
> +#define STK8312_SR_MASK			GENMASK(2, 0)
> +#define STK8312_SR_400HZ_IDX		0
> +#define STK8312_ALL_CHANNEL_MASK	GENMASK(2, 0)
>  #define STK8312_ALL_CHANNEL_SIZE	3
>  
>  #define STK8312_DRIVER_NAME		"stk8312"
> @@ -144,7 +144,7 @@ static int stk8312_otp_init(struct stk8312_data *data)
>  		if (ret < 0)
>  			goto exit_err;
>  		count--;
> -	} while (!(ret & 0x80) && count > 0);
> +	} while (!(ret & BIT(7)) && count > 0);
>  
>  	if (count == 0) {
>  		ret = -ETIMEDOUT;
> @@ -565,11 +565,12 @@ static int stk8312_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  	data->sample_rate_idx = STK8312_SR_400HZ_IDX;
> -	ret = stk8312_set_range(data, 1);
> +	ret = stk8312_set_range(data, STK8312_RNG_6G);
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = stk8312_set_mode(data, STK8312_INT_MODE | STK8312_MODE_ACTIVE);
> +	ret = stk8312_set_mode(data,
> +			       STK8312_MODE_INT_AH_PP | STK8312_MODE_ACTIVE);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -690,7 +691,7 @@ MODULE_DEVICE_TABLE(acpi, stk8312_acpi_id);
>  
>  static struct i2c_driver stk8312_driver = {
>  	.driver = {
> -		.name = "stk8312",
> +		.name = STK8312_DRIVER_NAME,
>  		.pm = STK8312_PM_OPS,
>  		.acpi_match_table = ACPI_PTR(stk8312_acpi_id),
>  	},
> 

[PATCH v2 5/7] iio:accel:stk8312: use appropriate variable types

[Hartmut Knaack]

Adapt some variable types to reduce unnecessary casting.

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
Reviewed-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
---
 drivers/iio/accel/stk8312.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c
index 8807927d4d0b..8b9b3815aac2 100644
--- a/drivers/iio/accel/stk8312.c
+++ b/drivers/iio/accel/stk8312.c
@@ -69,8 +69,8 @@ static const int stk8312_scale_table[][2] = {
 };
 
 static const struct {
-	u16 val;
-	u32 val2;
+	int val;
+	int val2;
 } stk8312_samp_freq_table[] = {
 	{400, 0}, {200, 0}, {100, 0}, {50, 0}, {25, 0},
 	{12, 500000}, {6, 250000}, {3, 125000}
@@ -103,7 +103,7 @@ static const struct iio_chan_spec stk8312_channels[] = {
 struct stk8312_data {
 	struct i2c_client *client;
 	struct mutex lock;
-	int range;
+	u8 range;
 	u8 sample_rate_idx;
 	u8 mode;
 	struct iio_trigger *dready_trig;
@@ -243,7 +243,7 @@ static const struct iio_trigger_ops stk8312_trigger_ops = {
 	.owner = THIS_MODULE,
 };
 
-static int stk8312_set_sample_rate(struct stk8312_data *data, int rate)
+static int stk8312_set_sample_rate(struct stk8312_data *data, u8 rate)
 {
 	int ret;
 	u8 masked_reg;
-- 
2.4.6

Re: [PATCH v2 5/7] iio:accel:stk8312: use appropriate variable types

[Jonathan Cameron]

On 29/07/15 22:39, Hartmut Knaack wrote:
> Adapt some variable types to reduce unnecessary casting.
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
> Reviewed-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
Applied.
> ---
>  drivers/iio/accel/stk8312.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c
> index 8807927d4d0b..8b9b3815aac2 100644
> --- a/drivers/iio/accel/stk8312.c
> +++ b/drivers/iio/accel/stk8312.c
> @@ -69,8 +69,8 @@ static const int stk8312_scale_table[][2] = {
>  };
>  
>  static const struct {
> -	u16 val;
> -	u32 val2;
> +	int val;
> +	int val2;
>  } stk8312_samp_freq_table[] = {
>  	{400, 0}, {200, 0}, {100, 0}, {50, 0}, {25, 0},
>  	{12, 500000}, {6, 250000}, {3, 125000}
> @@ -103,7 +103,7 @@ static const struct iio_chan_spec stk8312_channels[] = {
>  struct stk8312_data {
>  	struct i2c_client *client;
>  	struct mutex lock;
> -	int range;
> +	u8 range;
>  	u8 sample_rate_idx;
>  	u8 mode;
>  	struct iio_trigger *dready_trig;
> @@ -243,7 +243,7 @@ static const struct iio_trigger_ops stk8312_trigger_ops = {
>  	.owner = THIS_MODULE,
>  };
>  
> -static int stk8312_set_sample_rate(struct stk8312_data *data, int rate)
> +static int stk8312_set_sample_rate(struct stk8312_data *data, u8 rate)
>  {
>  	int ret;
>  	u8 masked_reg;
> 

[PATCH v2 6/7] iio:accel:stk8312: code style cleanup

[Hartmut Knaack]

Adjust some indentation issues to make checkpatch.pl happy in strict mode.

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
---
 drivers/iio/accel/stk8312.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c
index 8b9b3815aac2..39eb9f4ea5c0 100644
--- a/drivers/iio/accel/stk8312.c
+++ b/drivers/iio/accel/stk8312.c
@@ -157,8 +157,7 @@ static int stk8312_otp_init(struct stk8312_data *data)
 	if (ret < 0)
 		goto exit_err;
 
-	ret = i2c_smbus_write_byte_data(data->client,
-			STK8312_REG_AFECTRL, ret);
+	ret = i2c_smbus_write_byte_data(data->client, STK8312_REG_AFECTRL, ret);
 	if (ret < 0)
 		goto exit_err;
 	msleep(150);
@@ -458,7 +457,7 @@ static irqreturn_t stk8312_trigger_handler(int irq, void *p)
 		data->buffer[2] = buffer[2];
 	} else {
 		for_each_set_bit(bit, indio_dev->active_scan_mask,
-			   indio_dev->masklength) {
+				 indio_dev->masklength) {
 			ret = stk8312_read_accel(data, bit);
 			if (ret < 0) {
 				mutex_unlock(&data->lock);
-- 
2.4.6

Re: [PATCH v2 6/7] iio:accel:stk8312: code style cleanup

[Jonathan Cameron]

On 29/07/15 22:39, Hartmut Knaack wrote:
> Adjust some indentation issues to make checkpatch.pl happy in strict mode.
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
Applied.
> ---
>  drivers/iio/accel/stk8312.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c
> index 8b9b3815aac2..39eb9f4ea5c0 100644
> --- a/drivers/iio/accel/stk8312.c
> +++ b/drivers/iio/accel/stk8312.c
> @@ -157,8 +157,7 @@ static int stk8312_otp_init(struct stk8312_data *data)
>  	if (ret < 0)
>  		goto exit_err;
>  
> -	ret = i2c_smbus_write_byte_data(data->client,
> -			STK8312_REG_AFECTRL, ret);
> +	ret = i2c_smbus_write_byte_data(data->client, STK8312_REG_AFECTRL, ret);
>  	if (ret < 0)
>  		goto exit_err;
>  	msleep(150);
> @@ -458,7 +457,7 @@ static irqreturn_t stk8312_trigger_handler(int irq, void *p)
>  		data->buffer[2] = buffer[2];
>  	} else {
>  		for_each_set_bit(bit, indio_dev->active_scan_mask,
> -			   indio_dev->masklength) {
> +				 indio_dev->masklength) {
>  			ret = stk8312_read_accel(data, bit);
>  			if (ret < 0) {
>  				mutex_unlock(&data->lock);
> 

[PATCH v2 7/7] iio:accel:stk8312: drop local buffer

[Hartmut Knaack]

Drop the local buffer in stk8312_trigger_handler() and use data->buffer
instead for bulk reads.

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
Reviewed-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
---
 drivers/iio/accel/stk8312.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c
index 39eb9f4ea5c0..c88baabe08e7 100644
--- a/drivers/iio/accel/stk8312.c
+++ b/drivers/iio/accel/stk8312.c
@@ -435,7 +435,6 @@ static irqreturn_t stk8312_trigger_handler(int irq, void *p)
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct stk8312_data *data = iio_priv(indio_dev);
 	int bit, ret, i = 0;
-	u8 buffer[STK8312_ALL_CHANNEL_SIZE];
 
 	mutex_lock(&data->lock);
 	/*
@@ -446,15 +445,12 @@ static irqreturn_t stk8312_trigger_handler(int irq, void *p)
 		ret = i2c_smbus_read_i2c_block_data(data->client,
 						    STK8312_REG_XOUT,
 						    STK8312_ALL_CHANNEL_SIZE,
-						    buffer);
+						    data->buffer);
 		if (ret < STK8312_ALL_CHANNEL_SIZE) {
 			dev_err(&data->client->dev, "register read failed\n");
 			mutex_unlock(&data->lock);
 			goto err;
 		}
-		data->buffer[0] = buffer[0];
-		data->buffer[1] = buffer[1];
-		data->buffer[2] = buffer[2];
 	} else {
 		for_each_set_bit(bit, indio_dev->active_scan_mask,
 				 indio_dev->masklength) {
-- 
2.4.6

Re: [PATCH v2 7/7] iio:accel:stk8312: drop local buffer

[Jonathan Cameron]

On 29/07/15 22:39, Hartmut Knaack wrote:
> Drop the local buffer in stk8312_trigger_handler() and use data->buffer
> instead for bulk reads.
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
> Reviewed-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
Applied.

Thanks,

Jonathan
> ---
>  drivers/iio/accel/stk8312.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c
> index 39eb9f4ea5c0..c88baabe08e7 100644
> --- a/drivers/iio/accel/stk8312.c
> +++ b/drivers/iio/accel/stk8312.c
> @@ -435,7 +435,6 @@ static irqreturn_t stk8312_trigger_handler(int irq, void *p)
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct stk8312_data *data = iio_priv(indio_dev);
>  	int bit, ret, i = 0;
> -	u8 buffer[STK8312_ALL_CHANNEL_SIZE];
>  
>  	mutex_lock(&data->lock);
>  	/*
> @@ -446,15 +445,12 @@ static irqreturn_t stk8312_trigger_handler(int irq, void *p)
>  		ret = i2c_smbus_read_i2c_block_data(data->client,
>  						    STK8312_REG_XOUT,
>  						    STK8312_ALL_CHANNEL_SIZE,
> -						    buffer);
> +						    data->buffer);
>  		if (ret < STK8312_ALL_CHANNEL_SIZE) {
>  			dev_err(&data->client->dev, "register read failed\n");
>  			mutex_unlock(&data->lock);
>  			goto err;
>  		}
> -		data->buffer[0] = buffer[0];
> -		data->buffer[1] = buffer[1];
> -		data->buffer[2] = buffer[2];
>  	} else {
>  		for_each_set_bit(bit, indio_dev->active_scan_mask,
>  				 indio_dev->masklength) {
>