On 03/08/15 09:05, Breana, Tiberiu A wrote: >> -----Original Message----- >> From: Crt Mori [mailto:cmo@xxxxxxxxxxx] >> Sent: Saturday, August 1, 2015 10:41 AM >> To: Hartmut Knaack >> Cc: Breana, Tiberiu A; linux-iio@xxxxxxxxxxxxxxx; 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@xxxxxx> wrote: >>> 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. >>>> >>>> +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@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== >>>> >>> > 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
