[BUG] Re: [PATCH] iio: imu: st_lsm6dsx: remove invalid gain value for LSM9DS1

[Martin Kepplinger <martin.kepplinger@puri.sm>]

On 27.08.19 22:08, Jonathan Cameron wrote:
> On Tue, 27 Aug 2019 10:26:35 +0200
> Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> 
>> Get rid of invalid sensitivity value for LSM9DS1 gyro sensor
>>
>> Fixes: 687a60feb9c6 ("iio: imu: st_lsm6dsx: add support for accel/gyro unit of lsm9ds1")
>> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> The zero degree scale is certainly odd otherwise, so good to tidy
> this up.
> 
> Applied to the togreg branch of iio.git.
> 

Hi Jon,

you have applied this too quickly. I've left that zero value in there
for a reason, see below:


> Thanks,
> 
> Jonathan
> 
>> ---
>>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 17 +++++++++++------
>>  1 file changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> index fd152fff0a8c..c85c8be3a024 100644
>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> @@ -151,10 +151,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>>  					.addr = 0x10,
>>  					.mask = GENMASK(4, 3),
>>  				},
>> -				.fs_avl[0] = { IIO_DEGREE_TO_RAD(245), 0x0 },
>> -				.fs_avl[1] = { IIO_DEGREE_TO_RAD(500), 0x1 },
>> -				.fs_avl[2] = { IIO_DEGREE_TO_RAD(0), 0x2 },
>> -				.fs_avl[3] = { IIO_DEGREE_TO_RAD(2000), 0x3 },
>> +				.fs_avl[0] = {  IIO_DEGREE_TO_RAD(245), 0x0 },
>> +				.fs_avl[1] = {  IIO_DEGREE_TO_RAD(500), 0x1 },
>> +				.fs_avl[2] = { IIO_DEGREE_TO_RAD(2000), 0x3 },
>>  			},
>>  		},
>>  	},
>> @@ -1196,13 +1195,19 @@ static ssize_t st_lsm6dsx_sysfs_scale_avail(struct device *dev,
>>  					    char *buf)
>>  {
>>  	struct st_lsm6dsx_sensor *sensor = iio_priv(dev_get_drvdata(dev));
>> +	const struct st_lsm6dsx_fs_table_entry *fs_table;
>>  	enum st_lsm6dsx_sensor_id id = sensor->id;
>>  	struct st_lsm6dsx_hw *hw = sensor->hw;
>>  	int i, len = 0;
>>  
>> -	for (i = 0; i < ST_LSM6DSX_FS_LIST_SIZE; i++)
>> +	fs_table = &hw->settings->fs_table[id];
>> +	for (i = 0; i < ST_LSM6DSX_FS_LIST_SIZE; i++) {
>> +		if (!fs_table->fs_avl[i].gain)
>> +			break;

You Ooops here and it's pretty obvious! You don't have
ST_LSM6DSX_FS_LIST_SIZE number of elements in the array anymore, but you
try to access it (the 4th).

I suggest reverting this (if not able to delete it entirely) and start
over in case this "invalid" value thing hurts and needs to get fixed.

I any case, there _is_ something we should do because it's not too
obvious what constraints the st_lsm6dsx_sensor_settings struct
definition has. It should be mostly clear when looking at the header but
a few inline comments might help.

thanks,

                          martin



>> +
>>  		len += scnprintf(buf + len, PAGE_SIZE - len, "0.%06u ",
>> -				 hw->settings->fs_table[id].fs_avl[i].gain);
>> +				 fs_table->fs_avl[i].gain);
>> +	}
>>  	buf[len - 1] = '\n';
>>  
>>  	return len;
>