> >> 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 > > > >> --- [...] > > > > 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:
signature.asc
Description: PGP signature
