> -----Original Message----- > From: Crt Mori [mailto:cmo@xxxxxxxxxxx] > Sent: Thursday, July 30, 2015 10:04 AM > To: Hartmut Knaack > Cc: linux-iio@xxxxxxxxxxxxxxx; 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@xxxxxx> 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@xxxxxx> 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. > > - 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@xxxxxxxxxxxxxxx More > majordomo > > info at http://vger.kernel.org/majordomo-info.html ��.n��������+%������w��{.n�����{��(��)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
