Re: [PATCH 3/7] iio:accel:stk8312: improve error handling

[Hartmut Knaack <knaack.h@gmx.de>]

Breana, Tiberiu A schrieb am 28.07.2015 um 15:38:
>> -----Original Message-----
>> From: Hartmut Knaack [mailto:knaack.h@gmx.de]
>> Sent: Tuesday, July 28, 2015 1:49 AM
>> To: linux-iio@vger.kernel.org
>> 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@gmx.de>
> 
> 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>