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

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4647 bytes --]

> >> 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@puri.sm>
> > 
> > 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
> > 
> >> ---

[...]

> > 
> > 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,
> >>  };
> >>  

[...]

> >> +};
> >> +
> > 
> > why not st_lsm6ds0_gyro_channels?
> 
> Would be ok with me. I'll remember this if I do a new iteration.
> 
> > 

[...]

> >> 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...
> 

I have not a strong opinion on it but IMU means 'Inertial Measurement Unit' and
it used to indicate a device that runs a combination of accelerometer and
gyroscope. I think using LSM9DS1 as device name it is weird since I would
expect even the magn in this case (all supported devices by st_lsm6dsx are just
IMU). I am not sure if "st,lsm9ds1_imu" is the right compatible string but I
would indicate this node does not run a magn device.

@Jonathan: what do you think?

Regards,
Lorenzo

> > 
> >> +		.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
> >>
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]