stk8312 fixes and cleanup

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 <[email protected]>

Reviewed-by Tiberiu Breana <[email protected]>

---
 drivers/iio/accel/stk8312.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/iio/accel/stk8312.c +0 -0

index c2bd1444d6da..6592be8e6377 100644

drivers/iio/accel/stk8312.c +0 -1

        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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 16 lines

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 <[email protected]>

Reviewed-by Tiberiu Breana <[email protected]>

---
 drivers/iio/accel/stk8312.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/iio/accel/stk8312.c +0 -0

index c2bd1444d6da..6592be8e6377 100644
@@ -150,6 +150,8 @@ static int stk8312_otp_init(struct stk8312_data *data)
                goto exit_err;
 

drivers/iio/accel/stk8312.c static int stk8312_otp_init(struct stk8312_data *data) · line 154

Quoted 3 lines

drivers/iio/accel/stk8312.c +2 -0

        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.

Quoted 10 lines

drivers/iio/accel/stk8312.c +0 -1

        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 [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Crt Mori schrieb am 30.07.2015 um 09:09:

Quoted 17 lines · Hartmut Knaack

Quoted 16 lines

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 <[email protected]>

Reviewed-by Tiberiu Breana <[email protected]>

---
 drivers/iio/accel/stk8312.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/iio/accel/stk8312.c +0 -0

index c2bd1444d6da..6592be8e6377 100644
@@ -150,6 +150,8 @@ static int stk8312_otp_init(struct stk8312_data *data)
                goto exit_err;
 

Quoted 7 lines

drivers/iio/accel/stk8312.c static int stk8312_otp_init(struct stk8312_data *data) · line 154

Quoted 3 lines

drivers/iio/accel/stk8312.c +2 -0

        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.

Quoted 16 lines

Quoted 11 lines

drivers/iio/accel/stk8312.c +0 -1

        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 [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 26 lines

Crt Mori schrieb am 30.07.2015 um 09:09:

Quoted 17 lines · Hartmut Knaack

Quoted 16 lines

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 <[email protected]>

Reviewed-by Tiberiu Breana <[email protected]>

---
 drivers/iio/accel/stk8312.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/iio/accel/stk8312.c +0 -0

index c2bd1444d6da..6592be8e6377 100644
@@ -150,6 +150,8 @@ static int stk8312_otp_init(struct stk8312_data *data)
                goto exit_err;
 

Quoted 7 lines

drivers/iio/accel/stk8312.c static int stk8312_otp_init(struct stk8312_data *data) · line 154

Quoted 3 lines

drivers/iio/accel/stk8312.c +2 -0

        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.

Quoted 8 lines

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.

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

drivers/iio/accel/stk8312.c +0 -1

        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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 6 lines

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Crt Mori schrieb am 01.08.2015 um 09:35:

Quoted 35 lines · Hartmut Knaack

Quoted 26 lines

Crt Mori schrieb am 30.07.2015 um 09:09:

Quoted 17 lines · Hartmut Knaack

Quoted 16 lines

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 <[email protected]>

Reviewed-by Tiberiu Breana <[email protected]>

---
 drivers/iio/accel/stk8312.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/iio/accel/stk8312.c +0 -0

index c2bd1444d6da..6592be8e6377 100644
@@ -150,6 +150,8 @@ static int stk8312_otp_init(struct stk8312_data *data)
                goto exit_err;
 

Quoted 7 lines

drivers/iio/accel/stk8312.c static int stk8312_otp_init(struct stk8312_data *data) · line 154

Quoted 3 lines

drivers/iio/accel/stk8312.c +2 -0

        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.

Quoted 8 lines

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.

Quoted 3 lines

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

Quoted 20 lines

Thank you for the explanation.

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

drivers/iio/accel/stk8312.c +0 -1

        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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 6 lines

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 60 lines

-----Original Message-----
From: Jonathan Cameron [mailto:[email protected]]
Sent: Saturday, August 8, 2015 2:18 PM
To: Hartmut Knaack; Crt Mori
Cc: [email protected]; Lars-Peter Clausen; Peter Meerwald; Breana, Tiberiu A
Subject: Re: [PATCH v2 2/7] iio:accel:stk8312: check for invalid value

Quoted 13 lines · Hartmut Knaack

Crt Mori schrieb am 01.08.2015 um 09:35:

Quoted 12 lines · Hartmut Knaack

Quoted 11 lines

Crt Mori schrieb am 30.07.2015 um 09:09:

Quoted 10 lines · Hartmut Knaack

Quoted 9 lines

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 <[email protected]>

Reviewed-by Tiberiu Breana <[email protected]>

---
 drivers/iio/accel/stk8312.c | 2 ++
 1 file changed, 2 insertions(+)

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

drivers/iio/accel/stk8312.c static int stk8312_otp_init(struct stk8312_data

Quoted 6 lines

(quoted diff) +0 -0

b/drivers/iio/accel/stk8312.c index c2bd1444d6da..6592be8e6377 100644

drivers/iio/accel/stk8312.c +0 -0

@@ -150,6 +150,8 @@ static int stk8312_otp_init(struct stk8312_data

*data)

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

drivers/iio/accel/stk8312.c static int stk8312_otp_init(struct stk8312_data · line 155

Quoted 6 lines

drivers/iio/accel/stk8312.c +2 -0

                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.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

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.

Quoted 14 lines

Quoted 8 lines

Quoted 4 lines

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,

Quoted 1 line

Quoted 1 line

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

Quoted 11 lines

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

Quoted 7 lines

Quoted 7 lines

Thank you for the explanation.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

drivers/iio/accel/stk8312.c +0 -0

        if (ret < 0)
                goto exit_err;

Quoted 8 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

--
2.4.6

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected] 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==

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 <[email protected]>

---
 drivers/iio/accel/stk8312.c | 60 ++++++++++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 22 deletions(-)

drivers/iio/accel/stk8312.c +0 -0

index 6592be8e6377..4e1dda7e896d 100644

drivers/iio/accel/stk8312.c +17 -12

        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");

drivers/iio/accel/stk8312.c +16 -8

-               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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 8 lines

-----Original Message-----
From: Crt Mori [mailto:[email protected]]
Sent: Thursday, July 30, 2015 10:04 AM
To: Hartmut Knaack
Cc: [email protected]; Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Breana, Tiberiu A
Subject: Re: [PATCH v2 3/7] iio:accel:stk8312: improve error handling

Quoted 13 lines

Hi, some comments/questions below:

Quoted 10 lines · 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 <[email protected]>

Comments inline.

Quoted 14 lines

drivers/iio/accel/stk8312.c static int stk8312_otp_init(struct stk8312_data

Quoted 10 lines

---
 drivers/iio/accel/stk8312.c | 60
++++++++++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 22 deletions(-)

drivers/iio/accel/stk8312.c +0 -0

index 6592be8e6377..4e1dda7e896d 100644
@@ -146,8 +146,10 @@ static int stk8312_otp_init(struct stk8312_data

*data)

Quoted 3 lines

drivers/iio/accel/stk8312.c +0 -0

                count--;
        } while (!(ret & 0x80) && count > 0);
 

Quoted 7 lines

drivers/iio/accel/stk8312.c static int stk8312_otp_init(struct stk8312_data *data) · line 152

Quoted 5 lines

drivers/iio/accel/stk8312.c +3 -1

-       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

Quoted 7 lines

drivers/iio/accel/stk8312.c static int stk8312_otp_init(struct stk8312_data

Quoted 4 lines

drivers/iio/accel/stk8312.c +0 -0

        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)

drivers/iio/accel/stk8312.c static int stk8312_otp_init(struct stk8312_data · line 163

Quoted 1 line

drivers/iio/accel/stk8312.c +0 -0

                goto exit_err;

Why dont we return here as well?

I'm not sure I follow.

Quoted 81 lines

drivers/iio/accel/stk8312.c static int stk8312_otp_init(struct stk8312_data *data)

Quoted 8 lines

drivers/iio/accel/stk8312.c +1 -2

        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)

Quoted 3 lines

                return ret;

        ret = i2c_smbus_write_byte_data(client, STK8312_REG_INTSU,

int_mask);

Quoted 13 lines

drivers/iio/accel/stk8312.c +4 -1

-       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;

drivers/iio/accel/stk8312.c static int stk8312_data_rdy_trigger_set_state(struct iio_trigger *trig,

Quoted 6 lines

drivers/iio/accel/stk8312.c +1 -2

-       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)

drivers/iio/accel/stk8312.c static int stk8312_set_sample_rate(struct stk8312_data *data, int rate)

Quoted 13 lines

                return ret;

        ret = i2c_smbus_read_byte_data(client, STK8312_REG_SR);

drivers/iio/accel/stk8312.c +2 -4

-       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);

Quoted 9 lines

        if (ret < 0)

drivers/iio/accel/stk8312.c +3 -3

-               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);

drivers/iio/accel/stk8312.c static int stk8312_set_sample_rate(struct stk8312_data *data, int rate)

Quoted 10 lines

drivers/iio/accel/stk8312.c +6 -1

+
+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)

Quoted 14 lines

                return ret;

        ret = i2c_smbus_read_byte_data(client, STK8312_REG_STH);

drivers/iio/accel/stk8312.c +2 -4

-       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,

Quoted 6 lines

masked_reg);

Quoted 1 line

        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.

Quoted 36 lines

drivers/iio/accel/stk8312.c static int stk8312_read_raw(struct iio_dev

Quoted 18 lines

drivers/iio/accel/stk8312.c +9 -3

-               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,

drivers/iio/accel/stk8312.c static int stk8312_read_raw(struct iio_dev · line 350

Quoted 1 line

drivers/iio/accel/stk8312.c +0 -0

                ret = stk8312_set_mode(data, data->mode |

STK8312_MODE_ACTIVE);

Quoted 5 lines

drivers/iio/accel/stk8312.c +1 -1

                if (ret < 0) {
                        mutex_unlock(&data->lock);
-                       return -EINVAL;
+                       return ret;
                }

drivers/iio/accel/stk8312.c static int stk8312_read_raw(struct iio_dev *indio_dev,

Quoted 10 lines

drivers/iio/accel/stk8312.c +1 -2

                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 &

Quoted 7 lines

(~STK8312_MODE_ACTIVE));

Quoted 6 lines

drivers/iio/accel/stk8312.c +5 -0

+               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.

Quoted 20 lines

Quoted 15 lines

drivers/iio/accel/stk8312.c +1 -3

                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

Quoted 3 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected] More

majordomo

Quoted 1 line

info at http://vger.kernel.org/majordomo-info.html

Quoted 20 lines

-----Original Message-----
From: Crt Mori [mailto:[email protected]]
Sent: Thursday, July 30, 2015 10:04 AM
To: Hartmut Knaack
Cc: [email protected]; 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:

Quoted 9 lines · 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

Quoted 1 line

Quoted 1 line

Signed-off-by Hartmut Knaack <[email protected]>

Comments inline.

Quoted 21 lines

drivers/iio/accel/stk8312.c static int stk8312_otp_init(struct stk8312_data

Quoted 10 lines

---
 drivers/iio/accel/stk8312.c | 60
++++++++++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 22 deletions(-)

drivers/iio/accel/stk8312.c +0 -0

index 6592be8e6377..4e1dda7e896d 100644
@@ -146,8 +146,10 @@ static int stk8312_otp_init(struct stk8312_data

*data)

drivers/iio/accel/stk8312.c static int stk8312_otp_init(struct stk8312_data *data) · line 152

Quoted 8 lines

drivers/iio/accel/stk8312.c +3 -1

                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

Quoted 7 lines

drivers/iio/accel/stk8312.c static int stk8312_otp_init(struct stk8312_data

Quoted 4 lines

drivers/iio/accel/stk8312.c +0 -0

        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)

drivers/iio/accel/stk8312.c static int stk8312_otp_init(struct stk8312_data · line 163

Quoted 1 line

drivers/iio/accel/stk8312.c +0 -0

                goto exit_err;

Why dont we return here as well?

I'm not sure I follow.

Quoted 80 lines

drivers/iio/accel/stk8312.c static int stk8312_otp_init(struct stk8312_data *data)

Quoted 8 lines

drivers/iio/accel/stk8312.c +1 -2

        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)

Quoted 3 lines

                return ret;

        ret = i2c_smbus_write_byte_data(client, STK8312_REG_INTSU,

int_mask);

Quoted 13 lines

drivers/iio/accel/stk8312.c +4 -1

-       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;

drivers/iio/accel/stk8312.c static int stk8312_data_rdy_trigger_set_state(struct iio_trigger *trig,

Quoted 6 lines

drivers/iio/accel/stk8312.c +1 -2

-       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)

drivers/iio/accel/stk8312.c static int stk8312_set_sample_rate(struct stk8312_data *data, int rate)

Quoted 13 lines

                return ret;

        ret = i2c_smbus_read_byte_data(client, STK8312_REG_SR);

drivers/iio/accel/stk8312.c +2 -4

-       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);

Quoted 9 lines

        if (ret < 0)

drivers/iio/accel/stk8312.c +3 -3

-               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);

drivers/iio/accel/stk8312.c static int stk8312_set_sample_rate(struct stk8312_data *data, int rate)

Quoted 10 lines

drivers/iio/accel/stk8312.c +6 -1

+
+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)

Quoted 13 lines

                return ret;

        ret = i2c_smbus_read_byte_data(client, STK8312_REG_STH);

drivers/iio/accel/stk8312.c +2 -4

-       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;

Quoted 7 lines

Quoted 1 line

        ret = i2c_smbus_write_byte_data(client, STK8312_REG_STH,

masked_reg);

Quoted 1 line

        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.

Quoted 36 lines

drivers/iio/accel/stk8312.c static int stk8312_read_raw(struct iio_dev

Quoted 18 lines

drivers/iio/accel/stk8312.c +9 -3

-               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,

drivers/iio/accel/stk8312.c static int stk8312_read_raw(struct iio_dev · line 350

Quoted 1 line

drivers/iio/accel/stk8312.c +0 -0

                ret = stk8312_set_mode(data, data->mode |

STK8312_MODE_ACTIVE);

Quoted 5 lines

drivers/iio/accel/stk8312.c +1 -1

                if (ret < 0) {
                        mutex_unlock(&data->lock);
-                       return -EINVAL;
+                       return ret;
                }

drivers/iio/accel/stk8312.c static int stk8312_read_raw(struct iio_dev *indio_dev,

Quoted 10 lines

drivers/iio/accel/stk8312.c +1 -2

                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 &

Quoted 7 lines

(~STK8312_MODE_ACTIVE));

Quoted 6 lines

drivers/iio/accel/stk8312.c +5 -0

+               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.

Quoted 15 lines

Quoted 15 lines

drivers/iio/accel/stk8312.c +1 -3

                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

Quoted 5 lines

Quoted 3 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected] More

majordomo

Quoted 1 line

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==

Breana, Tiberiu A schrieb am 30.07.2015 um 11:07:

Quoted 43 lines

Quoted 8 lines

-----Original Message-----
From: Crt Mori [mailto:[email protected]]
Sent: Thursday, July 30, 2015 10:04 AM
To: Hartmut Knaack
Cc: [email protected]; Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Breana, Tiberiu A
Subject: Re: [PATCH v2 3/7] iio:accel:stk8312: improve error handling

Quoted 13 lines

Hi, some comments/questions below:

Quoted 10 lines · 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 <[email protected]>

Comments inline.

Quoted 19 lines

drivers/iio/accel/stk8312.c static int stk8312_otp_init(struct stk8312_data

Quoted 10 lines

---
 drivers/iio/accel/stk8312.c | 60
++++++++++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 22 deletions(-)

drivers/iio/accel/stk8312.c +0 -0

index 6592be8e6377..4e1dda7e896d 100644
@@ -146,8 +146,10 @@ static int stk8312_otp_init(struct stk8312_data

*data)

Quoted 8 lines

drivers/iio/accel/stk8312.c +3 -1

                count--;
        } while (!(ret & 0x80) && count > 0);
 
-       if (count == 0)
+       if (count == 0) {
+               ret = -ETIMEDOUT;
                goto exit_err;
+       }

Quoted 4 lines

Quoted 2 lines

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.

Quoted 145 lines

Quoted 8 lines

drivers/iio/accel/stk8312.c static int stk8312_otp_init(struct stk8312_data

Quoted 5 lines

drivers/iio/accel/stk8312.c +0 -0

        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)

drivers/iio/accel/stk8312.c static int stk8312_otp_init(struct stk8312_data · line 163

Quoted 1 line

drivers/iio/accel/stk8312.c +0 -0

                goto exit_err;

Why dont we return here as well?

I'm not sure I follow.

Quoted 80 lines

drivers/iio/accel/stk8312.c static int stk8312_otp_init(struct stk8312_data *data)

Quoted 8 lines

drivers/iio/accel/stk8312.c +1 -2

        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)

Quoted 3 lines

                return ret;

        ret = i2c_smbus_write_byte_data(client, STK8312_REG_INTSU,

int_mask);

Quoted 13 lines

drivers/iio/accel/stk8312.c +4 -1

-       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;

drivers/iio/accel/stk8312.c static int stk8312_data_rdy_trigger_set_state(struct iio_trigger *trig,

Quoted 6 lines

drivers/iio/accel/stk8312.c +1 -2

-       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)

drivers/iio/accel/stk8312.c static int stk8312_set_sample_rate(struct stk8312_data *data, int rate)

Quoted 13 lines

                return ret;

        ret = i2c_smbus_read_byte_data(client, STK8312_REG_SR);

drivers/iio/accel/stk8312.c +2 -4

-       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);

Quoted 9 lines

        if (ret < 0)

drivers/iio/accel/stk8312.c +3 -3

-               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);

drivers/iio/accel/stk8312.c static int stk8312_set_sample_rate(struct stk8312_data *data, int rate)

Quoted 10 lines

drivers/iio/accel/stk8312.c +6 -1

+
+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)

Quoted 13 lines

                return ret;

        ret = i2c_smbus_read_byte_data(client, STK8312_REG_STH);

drivers/iio/accel/stk8312.c +2 -4

-       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;

Quoted 7 lines

Quoted 1 line

        ret = i2c_smbus_write_byte_data(client, STK8312_REG_STH,

masked_reg);

Quoted 1 line

        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.

Quoted 36 lines

drivers/iio/accel/stk8312.c static int stk8312_read_raw(struct iio_dev

Quoted 18 lines

drivers/iio/accel/stk8312.c +9 -3

-               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,

drivers/iio/accel/stk8312.c static int stk8312_read_raw(struct iio_dev · line 350

Quoted 1 line

drivers/iio/accel/stk8312.c +0 -0

                ret = stk8312_set_mode(data, data->mode |

STK8312_MODE_ACTIVE);

Quoted 5 lines

drivers/iio/accel/stk8312.c +1 -1

                if (ret < 0) {
                        mutex_unlock(&data->lock);
-                       return -EINVAL;
+                       return ret;
                }

drivers/iio/accel/stk8312.c static int stk8312_read_raw(struct iio_dev *indio_dev,

Quoted 10 lines

drivers/iio/accel/stk8312.c +1 -2

                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 &

Quoted 7 lines

(~STK8312_MODE_ACTIVE));

Quoted 6 lines

drivers/iio/accel/stk8312.c +5 -0

+               ret = stk8312_set_mode(data,
+                                      data->mode &
+ (~STK8312_MODE_ACTIVE));
                mutex_unlock(&data->lock);
+               if (ret < 0)
+                       return ret;

Quoted 4 lines

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

Quoted 22 lines

Quoted 15 lines

Quoted 15 lines

drivers/iio/accel/stk8312.c +1 -3

                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

Quoted 5 lines

Quoted 3 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected] More

majordomo

Quoted 1 line

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==

Quoted 160 lines

-----Original Message-----
From: Crt Mori [mailto:[email protected]]
Sent: Saturday, August 1, 2015 10:41 AM
To: Hartmut Knaack
Cc: Breana, Tiberiu A; [email protected]; Jonathan Cameron; Lars- Peter Clausen; Peter Meerwald
Subject: Re: [PATCH v2 3/7] iio:accel:stk8312: improve error handling

Quoted 47 lines · Hartmut Knaack

Breana, Tiberiu A schrieb am 30.07.2015 um 11:07:

Quoted 46 lines

Quoted 9 lines

-----Original Message-----
From: Crt Mori [mailto:[email protected]]
Sent: Thursday, July 30, 2015 10:04 AM
To: Hartmut Knaack
Cc: [email protected]; Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Breana, Tiberiu A
Subject: Re: [PATCH v2 3/7] iio:accel:stk8312: improve error handling

Quoted 13 lines

Hi, some comments/questions below:

Quoted 10 lines · 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 <[email protected]>

Comments inline.

Quoted 21 lines

Quoted 11 lines

---
 drivers/iio/accel/stk8312.c | 60
++++++++++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 22 deletions(-)

(quoted diff) +0 -0

b/drivers/iio/accel/stk8312.c index 6592be8e6377..4e1dda7e896d 100644

drivers/iio/accel/stk8312.c +0 -0

@@ -146,8 +146,10 @@ static int stk8312_otp_init(struct

Quoted 1 line

stk8312_data

*data)

Quoted 8 lines

                count--;
        } while (!(ret & 0x80) && count > 0);

drivers/iio/accel/stk8312.c +3 -1

-       if (count == 0)
+       if (count == 0) {
+               ret = -ETIMEDOUT;
                goto exit_err;
+       }

Quoted 9 lines

Quoted 4 lines

Quoted 2 lines

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.

Quoted 94 lines

Quoted 94 lines

Quoted 8 lines

drivers/iio/accel/stk8312.c static int stk8312_otp_init(struct stk8312_data

Quoted 5 lines

        ret = i2c_smbus_read_byte_data(client, STK8312_REG_OTPDATA);
        if (ret == 0)

drivers/iio/accel/stk8312.c +0 -0

@@ -161,7 +163,7 @@ static int stk8312_otp_init(struct stk8312_data

*data)

drivers/iio/accel/stk8312.c static int stk8312_otp_init(struct stk8312_data · line 163

Quoted 1 line

drivers/iio/accel/stk8312.c +0 -0

                goto exit_err;

Why dont we return here as well?

I'm not sure I follow.

Quoted 83 lines

Quoted 9 lines

drivers/iio/accel/stk8312.c +1 -2

        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)

Quoted 3 lines

                return ret;

        ret = i2c_smbus_write_byte_data(client, STK8312_REG_INTSU,

int_mask);

Quoted 14 lines

drivers/iio/accel/stk8312.c +1 -1

-       if (ret < 0)
+       if (ret < 0) {
                dev_err(&client->dev, "failed to set

interrupts\n");

drivers/iio/accel/stk8312.c +3 -0

+               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;

drivers/iio/accel/stk8312.c static int stk8312_data_rdy_trigger_set_state(struct iio_trigger *trig,

Quoted 6 lines

drivers/iio/accel/stk8312.c +1 -2

-       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)

drivers/iio/accel/stk8312.c static int stk8312_set_sample_rate(struct stk8312_data *data, int rate)

Quoted 13 lines

                return ret;

        ret = i2c_smbus_read_byte_data(client, STK8312_REG_SR);

drivers/iio/accel/stk8312.c +2 -4

-       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);

Quoted 18 lines

        if (ret < 0)

drivers/iio/accel/stk8312.c +9 -3

-               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)

Quoted 2 lines

@@
-290,21 +299,25 @@ static int stk8312_set_range(struct stk8312_data

*data, u8 range)

Quoted 13 lines

                return ret;

        ret = i2c_smbus_read_byte_data(client, STK8312_REG_STH);

drivers/iio/accel/stk8312.c +2 -4

-       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;

Quoted 76 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

        ret = i2c_smbus_write_byte_data(client, STK8312_REG_STH,

masked_reg);

Quoted 1 line

        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?

Quoted 70 lines

Quoted 50 lines

Quoted 2 lines

Either label is
confusing or the whole process.

I suggest you take a closer look.

Quoted 37 lines

Quoted 17 lines

drivers/iio/accel/stk8312.c +9 -3

-               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

Quoted 2 lines

address) @@ -337,18 +350,21 @@ static int stk8312_read_raw(struct iio_dev

*indio_dev,

Quoted 1 line

                ret = stk8312_set_mode(data, data->mode |

STK8312_MODE_ACTIVE);

Quoted 5 lines

                if (ret < 0) {
                        mutex_unlock(&data->lock);

drivers/iio/accel/stk8312.c +1 -1

-                       return -EINVAL;
+                       return ret;
                }

drivers/iio/accel/stk8312.c static int stk8312_read_raw(struct iio_dev *indio_dev,

Quoted 10 lines

drivers/iio/accel/stk8312.c +1 -2

                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 &

Quoted 7 lines

(~STK8312_MODE_ACTIVE));

Quoted 6 lines

drivers/iio/accel/stk8312.c +5 -0

+               ret = stk8312_set_mode(data,
+                                      data->mode &
+ (~STK8312_MODE_ACTIVE));
                mutex_unlock(&data->lock);
+               if (ret < 0)
+                       return ret;

Quoted 4 lines

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

Quoted 19 lines

Quoted 19 lines

Quoted 19 lines

Quoted 19 lines

Quoted 19 lines

                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;
        }

drivers/iio/accel/stk8312.c +1 -2

-       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 [email protected] More

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

majordomo

Quoted 1 line

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���h����&���G���h�(�階�ݢj"���m�����z�ޖ���f���h���~�mml==

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 <[email protected]>

Reviewed-by Tiberiu Breana <[email protected]>

---
 drivers/iio/accel/stk8312.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

drivers/iio/accel/stk8312.c +13 -12

index 4e1dda7e896d..8807927d4d0b 100644
@@ -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),
 	},

Adapt some variable types to reduce unnecessary casting.

Signed-off-by Hartmut Knaack <[email protected]>

Reviewed-by Tiberiu Breana <[email protected]>

---
 drivers/iio/accel/stk8312.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/iio/accel/stk8312.c +4 -4

index 8807927d4d0b..8b9b3815aac2 100644
@@ -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;

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

Signed-off-by Hartmut Knaack <[email protected]>

---
 drivers/iio/accel/stk8312.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/iio/accel/stk8312.c +2 -3

index 8b9b3815aac2..39eb9f4ea5c0 100644
@@ -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);

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

Signed-off-by Hartmut Knaack <[email protected]>

Reviewed-by Tiberiu Breana <[email protected]>

---
 drivers/iio/accel/stk8312.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

drivers/iio/accel/stk8312.c +1 -5

index 39eb9f4ea5c0..c88baabe08e7 100644
@@ -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) {