Breana, Tiberiu A schrieb am 30.07.2015 um 11:07: >> -----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. > 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@xxxxxxxxxxxxxxx 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== > -- 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
