Breana, Tiberiu A schrieb am 28.07.2015 um 15:38: >> -----Original Message----- >> From: Hartmut Knaack [mailto:knaack.h@xxxxxx] >> Sent: Tuesday, July 28, 2015 1:49 AM >> To: linux-iio@xxxxxxxxxxxxxxx >> Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Breana, Tiberiu >> A >> Subject: [PATCH 3/7] iio:accel:stk8312: improve error handling >> >> 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 >> >> Signed-off-by: Hartmut Knaack <knaack.h@xxxxxx> > > Comments inline. > Hi, <...> >> +267,12 @@ static int stk8312_set_sample_rate(struct stk8312_data *data, >> int rate) >> masked_reg = (ret & (~STK8312_SR_MASK)) | rate; >> >> ret = i2c_smbus_write_byte_data(client, STK8312_REG_SR, >> masked_reg); >> - if (ret < 0) >> + if (ret < 0) { >> dev_err(&client->dev, "failed to set sampling rate\n"); >> - else >> - data->sample_rate_idx = rate; >> + return ret; > > We can't just return here. Bear in mind that earlier in the function we set the > sensor's operation mode to standby. Return to active mode before returning. > This should be added @258 as well. > OK, I will respin with a common error path to put the device back in active mode. >> + } >> + >> + data->sample_rate_idx = rate; >> >> return stk8312_set_mode(data, mode); >> } >> @@ -299,10 +305,12 @@ static int stk8312_set_range(struct stk8312_data >> *data, u8 range) >> masked_reg |= range << STK8312_RNG_SHIFT; >> >> ret = i2c_smbus_write_byte_data(client, STK8312_REG_STH, >> masked_reg); >> - if (ret < 0) >> + if (ret < 0) { >> dev_err(&client->dev, "failed to change sensor range\n"); >> - else >> - data->range = range; >> + return ret; >> + } >> + >> + data->range = range; >> >> return stk8312_set_mode(data, mode); >> } >> @@ -337,18 +345,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; > > I don't agree with this. We've succesfully read the data, we shouldn't return an > error if the sensor couldn't be put back to standby. I suggest leaving it as it is. > Well, encountering an error should be an exceptional case, which should not happen in normal operation (and thus do no harm). But something goes wrong, then better report it, even a data sample has been read successfully. Thanks, Hartmut >> return IIO_VAL_INT; >> case IIO_CHAN_INFO_SCALE: >> *val = stk8312_scale_table[data->range - 1][0]; @@ -608,7 >> +619,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 > -- 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
