Re: [PATCH v4 2/3] iio: imu: st_lsm6dsx: add support for accel/gyro unit of lsm9sd1

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 19.08.19 11:48, Lorenzo Bianconi wrote:
>> The LSM9DS1's accelerometer / gyroscope unit and it's magnetometer (separately
>> supported in iio/magnetometer/st_magn*) are located on a separate i2c addresses
>> on the bus.
>>
>> For the datasheet, see https://www.st.com/resource/en/datasheet/lsm9ds1.pdf
>>
>> Treat it just like the LSM6* devices and, despite it's name, hook it up
>> to the st_lsm6dsx driver, using it's basic functionality.
>>
>> accelerometer and gyroscope are not independently clocked. It runs at the gyro
>> frequencies if both are enabled, see chapter 7.12 of the datasheet.
>> We could have handled this as a single IIO device but we have split
>> it up to be more consistent with the other more flexible devices.
>>
>> Signed-off-by: Martin Kepplinger <martin.kepplinger@xxxxxxx>
> 
> Hi Martin,
> 
> most of comments are nitpicks (inline), the only issue I can see here is we can enable
> hw fifo for lsm6ds0/lsm9ds1 and read_fifo routine pointer is not currently
> initialized so we will end up with a NULL pointer dereference. Since we will
> need a different update FIFO routine for lsm6ds0/lsm9ds1 I am adding an
> update_fifo function pointer in fifo_ops in order to fix this issue.
> 
> Regards,
> Lorenzo
> 
>> ---
>>  drivers/iio/imu/st_lsm6dsx/Kconfig           |  2 +-
>>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  2 +
>>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 87 ++++++++++++++++++++
>>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c  |  5 ++
>>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c  |  5 ++
>>  5 files changed, 100 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/imu/st_lsm6dsx/Kconfig b/drivers/iio/imu/st_lsm6dsx/Kconfig
>> index 939058b27746..77aa0e77212d 100644
>> --- a/drivers/iio/imu/st_lsm6dsx/Kconfig
>> +++ b/drivers/iio/imu/st_lsm6dsx/Kconfig
>> @@ -12,7 +12,7 @@ config IIO_ST_LSM6DSX
>>  	  Say yes here to build support for STMicroelectronics LSM6DSx imu
>>  	  sensor. Supported devices: lsm6ds3, lsm6ds3h, lsm6dsl, lsm6dsm,
>>  	  ism330dlc, lsm6dso, lsm6dsox, asm330lhh, lsm6dsr, lsm6ds3tr-c,
>> -	  ism330dhcx
>> +	  ism330dhcx and the accelerometer/gyroscope of lsm9ds1.
>>  
>>  	  To compile this driver as a module, choose M here: the module
>>  	  will be called st_lsm6dsx.
>> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
>> index c8f333902eb7..d03b5a2d8549 100644
>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
>> @@ -24,6 +24,7 @@
>>  #define ST_LSM6DSR_DEV_NAME	"lsm6dsr"
>>  #define ST_LSM6DS3TRC_DEV_NAME	"lsm6ds3tr-c"
>>  #define ST_ISM330DHCX_DEV_NAME	"ism330dhcx"
>> +#define ST_LSM9DS1_DEV_NAME	"lsm9ds1"
> 
> should be called 'lsm9ds1_imu' since lsm9ds1 is a 9-axis device? what do you
> think?
> 
>>  
>>  enum st_lsm6dsx_hw_id {
>>  	ST_LSM6DS3_ID,
>> @@ -37,6 +38,7 @@ enum st_lsm6dsx_hw_id {
>>  	ST_LSM6DSR_ID,
>>  	ST_LSM6DS3TRC_ID,
>>  	ST_ISM330DHCX_ID,
>> +	ST_LSM9DS1_ID,
> 
> same here..ST_LSM9DS1_IMU_ID

I wouldn't add "imu" to the actual part name, see below...

> 
>>  	ST_LSM6DSX_MAX_ID,
>>  };
>>  
>> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> index 56e1c5262a2c..f038bb06f635 100644
>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> @@ -10,6 +10,8 @@
>>   * +-125/+-245/+-500/+-1000/+-2000 dps
>>   * LSM6DSx series has an integrated First-In-First-Out (FIFO) buffer
>>   * allowing dynamic batching of sensor data.
>> + * LSM9DSx series is similar but includes an additional magnetometer, handled
>> + * by a different driver.
>>   *
>>   * Supported sensors:
>>   * - LSM6DS3:
>> @@ -30,6 +32,13 @@
>>   *   - Gyroscope supported full-scale [dps]: +-125/+-245/+-500/+-1000/+-2000
>>   *   - FIFO size: 3KB
>>   *
>> + * - LSM9DS1:
>> + *   - Accelerometer supported ODR [Hz]: 10, 50, 119, 238, 476, 952
>> + *   - Accelerometer supported full-scale [g]: +-2/+-4/+-8/+-16
>> + *   - Gyroscope supported ODR [Hz]: 15, 60, 119, 238, 476, 952
>> + *   - Gyroscope supported full-scale [dps]: +-245/+-500/+-2000
>> + *   - FIFO size: 32
>> + *
>>   * Copyright 2016 STMicroelectronics Inc.
>>   *
>>   * Lorenzo Bianconi <lorenzo.bianconi@xxxxxx>
>> @@ -70,7 +79,85 @@ static const struct iio_chan_spec st_lsm6dsx_gyro_channels[] = {
>>  	IIO_CHAN_SOFT_TIMESTAMP(3),
>>  };
>>  
>> +static const struct iio_chan_spec st_lsm9dsx_gyro_channels[] = {
>> +	ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, 0x18, IIO_MOD_X, 0),
>> +	ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, 0x1a, IIO_MOD_Y, 1),
>> +	ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, 0x1c, IIO_MOD_Z, 2),
>> +	IIO_CHAN_SOFT_TIMESTAMP(3),
>> +};
>> +
> 
> why not st_lsm6ds0_gyro_channels?

Would be ok with me. I'll remember this if I do a new iteration.

> 
>>  static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>> +	{
>> +		.wai = 0x68,
>> +		.int1_addr = 0x0c,
>> +		.int2_addr = 0x0d,
>> +		.reset_addr = 0x22,
>> +		.max_fifo_size = 32,
>> +		.id = {
>> +			{
>> +				.hw_id = ST_LSM9DS1_ID,
>> +				.name = ST_LSM9DS1_DEV_NAME,
>> +			},
>> +		},
>> +		.channels = {
>> +			[ST_LSM6DSX_ID_ACC] = {
>> +				.chan = st_lsm6dsx_acc_channels,
>> +				.len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
>> +			},
>> +			[ST_LSM6DSX_ID_GYRO] = {
>> +				.chan = st_lsm9dsx_gyro_channels,
>> +				.len = ARRAY_SIZE(st_lsm9dsx_gyro_channels),
>> +			},
>> +		},
>> +		.odr_table = {
>> +			[ST_LSM6DSX_ID_ACC] = {
>> +				.reg = {
>> +					.addr = 0x20,
>> +					.mask = GENMASK(7, 5),
>> +				},
>> +				.odr_avl[0] = {  10, 0x01 },
>> +				.odr_avl[1] = {  50, 0x02 },
>> +				.odr_avl[2] = { 119, 0x03 },
>> +				.odr_avl[3] = { 238, 0x04 },
>> +				.odr_avl[4] = { 476, 0x05 },
>> +				.odr_avl[5] = { 952, 0x06 },
>> +			},
>> +			[ST_LSM6DSX_ID_GYRO] = {
>> +				.reg = {
>> +					.addr = 0x10,
>> +					.mask = GENMASK(7, 5),
>> +				},
>> +				.odr_avl[0] = {  15, 0x01 },
>> +				.odr_avl[1] = {  60, 0x02 },
>> +				.odr_avl[2] = { 119, 0x03 },
>> +				.odr_avl[3] = { 238, 0x04 },
>> +				.odr_avl[4] = { 476, 0x05 },
>> +				.odr_avl[5] = { 952, 0x06 },
>> +			},
>> +		},
>> +		.fs_table = {
>> +			[ST_LSM6DSX_ID_ACC] = {
>> +				.reg = {
>> +					.addr = 0x20,
>> +					.mask = GENMASK(4, 3),
>> +				},
>> +				.fs_avl[0] = {  599, 0x0 },
>> +				.fs_avl[1] = { 1197, 0x2 },
>> +				.fs_avl[2] = { 2394, 0x3 },
>> +				.fs_avl[3] = { 4788, 0x1 },
>> +			},
>> +			[ST_LSM6DSX_ID_GYRO] = {
>> +				.reg = {
>> +					.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 },
>> +			},
>> +		},
>> +	},
>>  	{
>>  		.wai = 0x69,
>>  		.int1_addr = 0x0d,
>> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c
>> index 15c6aa5b6caa..2f1b30ff083b 100644
>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c
>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c
>> @@ -83,6 +83,10 @@ static const struct of_device_id st_lsm6dsx_i2c_of_match[] = {
>>  		.compatible = "st,ism330dhcx",
>>  		.data = (void *)ST_ISM330DHCX_ID,
>>  	},
>> +	{
>> +		.compatible = "st,lsm9ds1",
> 
> same here, what is the right compatible string? "st,lsm9ds1 or
> "st,lsm9ds1_imu"?

well, I'm open for this change, but "imu" doesn't really mean much
technically, so I would stick with the device name. "imu" is not part of
the "part" name...

> 
>> +		.data = (void *)ST_LSM9DS1_ID,
>> +	},
>>  	{},
>>  };
>>  MODULE_DEVICE_TABLE(of, st_lsm6dsx_i2c_of_match);
>> @@ -99,6 +103,7 @@ static const struct i2c_device_id st_lsm6dsx_i2c_id_table[] = {
>>  	{ ST_LSM6DSR_DEV_NAME, ST_LSM6DSR_ID },
>>  	{ ST_LSM6DS3TRC_DEV_NAME, ST_LSM6DS3TRC_ID },
>>  	{ ST_ISM330DHCX_DEV_NAME, ST_ISM330DHCX_ID },
>> +	{ ST_LSM9DS1_DEV_NAME, ST_LSM9DS1_ID },
>>  	{},
>>  };
>>  MODULE_DEVICE_TABLE(i2c, st_lsm6dsx_i2c_id_table);
>> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c
>> index a8430ee11310..421ce704f346 100644
>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c
>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c
>> @@ -83,6 +83,10 @@ static const struct of_device_id st_lsm6dsx_spi_of_match[] = {
>>  		.compatible = "st,ism330dhcx",
>>  		.data = (void *)ST_ISM330DHCX_ID,
>>  	},
>> +	{
>> +		.compatible = "st,lsm9ds1",
>> +		.data = (void *)ST_LSM9DS1_ID,
>> +	},
>>  	{},
>>  };
>>  MODULE_DEVICE_TABLE(of, st_lsm6dsx_spi_of_match);
>> @@ -99,6 +103,7 @@ static const struct spi_device_id st_lsm6dsx_spi_id_table[] = {
>>  	{ ST_LSM6DSR_DEV_NAME, ST_LSM6DSR_ID },
>>  	{ ST_LSM6DS3TRC_DEV_NAME, ST_LSM6DS3TRC_ID },
>>  	{ ST_ISM330DHCX_DEV_NAME, ST_ISM330DHCX_ID },
>> +	{ ST_LSM9DS1_DEV_NAME, ST_LSM9DS1_ID },
>>  	{},
>>  };
>>  MODULE_DEVICE_TABLE(spi, st_lsm6dsx_spi_id_table);
>> -- 
>> 2.20.1
>>




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux