[PATCH v4 0/3] iio: imu: st_lsm6dsx: Add support for LSM9DS1

[PATCH v4 0/3] iio: imu: st_lsm6dsx: Add support for LSM9DS1

[Martin Kepplinger]

Add basic functionality for LSM9DS1. This has become a trivial addition
by now.

revision history
----------------
v4: rebase on top of today's iio testing branch with Lorenzo's recent work
v3: rebase and add Lorenzo's patches in order to apply to the iio testing brach
v2: further simplifications based on Lorenzo's feedback
v1: initial change for adding lsm9ds1 support


Martin Kepplinger (3):
  iio: imu: st_lsm6sdx: move register definitions to sensor_settings
    struct
  iio: imu: st_lsm6dsx: add support for accel/gyro unit of lsm9sd1
  dt-bindings: iio: imu: st_lsm6dsx: add lsm9ds1 device bindings

 .../bindings/iio/imu/st_lsm6dsx.txt           |   1 +
 drivers/iio/imu/st_lsm6dsx/Kconfig            |   2 +-
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h       |   8 ++
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c  | 118 ++++++++++++++++--
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c   |   5 +
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c   |   5 +
 6 files changed, 129 insertions(+), 10 deletions(-)

-- 
2.20.1

[PATCH v4 1/3] iio: imu: st_lsm6sdx: move register definitions to sensor_settings struct

[Martin Kepplinger]

Move some register definitions to the per-device array of struct
st_lsm6dsx_sensor_settings in order to simplify adding new sensor
devices to the driver.

Also, remove completely unused register definitions.

Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
---
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  6 ++++
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 31 ++++++++++++++------
 2 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
index 4e8e67ae1632..c8f333902eb7 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
@@ -200,6 +200,9 @@ struct st_lsm6dsx_ext_dev_settings {
 /**
  * struct st_lsm6dsx_settings - ST IMU sensor settings
  * @wai: Sensor WhoAmI default value.
+ * @int1_addr: Control Register address for INT1
+ * @int2_addr: Control Register address for INT2
+ * @reset_addr: register address for reset/reboot
  * @max_fifo_size: Sensor max fifo length in FIFO words.
  * @id: List of hw id/device name supported by the driver configuration.
  * @channels: IIO channels supported by the device.
@@ -213,6 +216,9 @@ struct st_lsm6dsx_ext_dev_settings {
  */
 struct st_lsm6dsx_settings {
 	u8 wai;
+	u8 int1_addr;
+	u8 int2_addr;
+	u8 reset_addr;
 	u16 max_fifo_size;
 	struct {
 		enum st_lsm6dsx_hw_id hw_id;
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index 85824d6739ee..56e1c5262a2c 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -49,17 +49,12 @@
 
 #include "st_lsm6dsx.h"
 
-#define ST_LSM6DSX_REG_INT1_ADDR		0x0d
-#define ST_LSM6DSX_REG_INT2_ADDR		0x0e
 #define ST_LSM6DSX_REG_FIFO_FTH_IRQ_MASK	BIT(3)
 #define ST_LSM6DSX_REG_WHOAMI_ADDR		0x0f
-#define ST_LSM6DSX_REG_RESET_ADDR		0x12
 #define ST_LSM6DSX_REG_RESET_MASK		BIT(0)
 #define ST_LSM6DSX_REG_BOOT_MASK		BIT(7)
 #define ST_LSM6DSX_REG_BDU_ADDR			0x12
 #define ST_LSM6DSX_REG_BDU_MASK			BIT(6)
-#define ST_LSM6DSX_REG_INT2_ON_INT1_ADDR	0x13
-#define ST_LSM6DSX_REG_INT2_ON_INT1_MASK	BIT(5)
 
 static const struct iio_chan_spec st_lsm6dsx_acc_channels[] = {
 	ST_LSM6DSX_CHANNEL(IIO_ACCEL, 0x28, IIO_MOD_X, 0),
@@ -78,6 +73,9 @@ static const struct iio_chan_spec st_lsm6dsx_gyro_channels[] = {
 static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 	{
 		.wai = 0x69,
+		.int1_addr = 0x0d,
+		.int2_addr = 0x0e,
+		.reset_addr = 0x12,
 		.max_fifo_size = 1365,
 		.id = {
 			{
@@ -186,6 +184,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 	},
 	{
 		.wai = 0x69,
+		.int1_addr = 0x0d,
+		.int2_addr = 0x0e,
+		.reset_addr = 0x12,
 		.max_fifo_size = 682,
 		.id = {
 			{
@@ -294,6 +295,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 	},
 	{
 		.wai = 0x6a,
+		.int1_addr = 0x0d,
+		.int2_addr = 0x0e,
+		.reset_addr = 0x12,
 		.max_fifo_size = 682,
 		.id = {
 			{
@@ -411,6 +415,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 	},
 	{
 		.wai = 0x6c,
+		.int1_addr = 0x0d,
+		.int2_addr = 0x0e,
+		.reset_addr = 0x12,
 		.max_fifo_size = 512,
 		.id = {
 			{
@@ -540,6 +547,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 	},
 	{
 		.wai = 0x6b,
+		.int1_addr = 0x0d,
+		.int2_addr = 0x0e,
+		.reset_addr = 0x12,
 		.max_fifo_size = 512,
 		.id = {
 			{
@@ -640,6 +650,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 	},
 	{
 		.wai = 0x6b,
+		.int1_addr = 0x0d,
+		.int2_addr = 0x0e,
+		.reset_addr = 0x12,
 		.max_fifo_size = 512,
 		.id = {
 			{
@@ -1166,10 +1179,10 @@ static int st_lsm6dsx_get_drdy_reg(struct st_lsm6dsx_hw *hw, u8 *drdy_reg)
 
 	switch (drdy_pin) {
 	case 1:
-		*drdy_reg = ST_LSM6DSX_REG_INT1_ADDR;
+		*drdy_reg = hw->settings->int1_addr;
 		break;
 	case 2:
-		*drdy_reg = ST_LSM6DSX_REG_INT2_ADDR;
+		*drdy_reg = hw->settings->int2_addr;
 		break;
 	default:
 		dev_err(hw->dev, "unsupported data ready pin\n");
@@ -1269,7 +1282,7 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
 	int err;
 
 	/* device sw reset */
-	err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_RESET_ADDR,
+	err = regmap_update_bits(hw->regmap, hw->settings->reset_addr,
 				 ST_LSM6DSX_REG_RESET_MASK,
 				 FIELD_PREP(ST_LSM6DSX_REG_RESET_MASK, 1));
 	if (err < 0)
@@ -1278,7 +1291,7 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
 	msleep(50);
 
 	/* reload trimming parameter */
-	err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_RESET_ADDR,
+	err = regmap_update_bits(hw->regmap, hw->settings->reset_addr,
 				 ST_LSM6DSX_REG_BOOT_MASK,
 				 FIELD_PREP(ST_LSM6DSX_REG_BOOT_MASK, 1));
 	if (err < 0)
-- 
2.20.1

Re: [PATCH v4 1/3] iio: imu: st_lsm6sdx: move register definitions to sensor_settings struct

[Lorenzo Bianconi]

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

> Move some register definitions to the per-device array of struct
> st_lsm6dsx_sensor_settings in order to simplify adding new sensor
> devices to the driver.
> 
> Also, remove completely unused register definitions.
> 
> Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>

Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>

> ---
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  6 ++++
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 31 ++++++++++++++------
>  2 files changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> index 4e8e67ae1632..c8f333902eb7 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> @@ -200,6 +200,9 @@ struct st_lsm6dsx_ext_dev_settings {
>  /**
>   * struct st_lsm6dsx_settings - ST IMU sensor settings
>   * @wai: Sensor WhoAmI default value.
> + * @int1_addr: Control Register address for INT1
> + * @int2_addr: Control Register address for INT2
> + * @reset_addr: register address for reset/reboot
>   * @max_fifo_size: Sensor max fifo length in FIFO words.
>   * @id: List of hw id/device name supported by the driver configuration.
>   * @channels: IIO channels supported by the device.
> @@ -213,6 +216,9 @@ struct st_lsm6dsx_ext_dev_settings {
>   */
>  struct st_lsm6dsx_settings {
>  	u8 wai;
> +	u8 int1_addr;
> +	u8 int2_addr;
> +	u8 reset_addr;
>  	u16 max_fifo_size;
>  	struct {
>  		enum st_lsm6dsx_hw_id hw_id;
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> index 85824d6739ee..56e1c5262a2c 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -49,17 +49,12 @@
>  
>  #include "st_lsm6dsx.h"
>  
> -#define ST_LSM6DSX_REG_INT1_ADDR		0x0d
> -#define ST_LSM6DSX_REG_INT2_ADDR		0x0e
>  #define ST_LSM6DSX_REG_FIFO_FTH_IRQ_MASK	BIT(3)
>  #define ST_LSM6DSX_REG_WHOAMI_ADDR		0x0f
> -#define ST_LSM6DSX_REG_RESET_ADDR		0x12
>  #define ST_LSM6DSX_REG_RESET_MASK		BIT(0)
>  #define ST_LSM6DSX_REG_BOOT_MASK		BIT(7)
>  #define ST_LSM6DSX_REG_BDU_ADDR			0x12
>  #define ST_LSM6DSX_REG_BDU_MASK			BIT(6)
> -#define ST_LSM6DSX_REG_INT2_ON_INT1_ADDR	0x13
> -#define ST_LSM6DSX_REG_INT2_ON_INT1_MASK	BIT(5)
>  
>  static const struct iio_chan_spec st_lsm6dsx_acc_channels[] = {
>  	ST_LSM6DSX_CHANNEL(IIO_ACCEL, 0x28, IIO_MOD_X, 0),
> @@ -78,6 +73,9 @@ static const struct iio_chan_spec st_lsm6dsx_gyro_channels[] = {
>  static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>  	{
>  		.wai = 0x69,
> +		.int1_addr = 0x0d,
> +		.int2_addr = 0x0e,
> +		.reset_addr = 0x12,
>  		.max_fifo_size = 1365,
>  		.id = {
>  			{
> @@ -186,6 +184,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>  	},
>  	{
>  		.wai = 0x69,
> +		.int1_addr = 0x0d,
> +		.int2_addr = 0x0e,
> +		.reset_addr = 0x12,
>  		.max_fifo_size = 682,
>  		.id = {
>  			{
> @@ -294,6 +295,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>  	},
>  	{
>  		.wai = 0x6a,
> +		.int1_addr = 0x0d,
> +		.int2_addr = 0x0e,
> +		.reset_addr = 0x12,
>  		.max_fifo_size = 682,
>  		.id = {
>  			{
> @@ -411,6 +415,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>  	},
>  	{
>  		.wai = 0x6c,
> +		.int1_addr = 0x0d,
> +		.int2_addr = 0x0e,
> +		.reset_addr = 0x12,
>  		.max_fifo_size = 512,
>  		.id = {
>  			{
> @@ -540,6 +547,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>  	},
>  	{
>  		.wai = 0x6b,
> +		.int1_addr = 0x0d,
> +		.int2_addr = 0x0e,
> +		.reset_addr = 0x12,
>  		.max_fifo_size = 512,
>  		.id = {
>  			{
> @@ -640,6 +650,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>  	},
>  	{
>  		.wai = 0x6b,
> +		.int1_addr = 0x0d,
> +		.int2_addr = 0x0e,
> +		.reset_addr = 0x12,
>  		.max_fifo_size = 512,
>  		.id = {
>  			{
> @@ -1166,10 +1179,10 @@ static int st_lsm6dsx_get_drdy_reg(struct st_lsm6dsx_hw *hw, u8 *drdy_reg)
>  
>  	switch (drdy_pin) {
>  	case 1:
> -		*drdy_reg = ST_LSM6DSX_REG_INT1_ADDR;
> +		*drdy_reg = hw->settings->int1_addr;
>  		break;
>  	case 2:
> -		*drdy_reg = ST_LSM6DSX_REG_INT2_ADDR;
> +		*drdy_reg = hw->settings->int2_addr;
>  		break;
>  	default:
>  		dev_err(hw->dev, "unsupported data ready pin\n");
> @@ -1269,7 +1282,7 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
>  	int err;
>  
>  	/* device sw reset */
> -	err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_RESET_ADDR,
> +	err = regmap_update_bits(hw->regmap, hw->settings->reset_addr,
>  				 ST_LSM6DSX_REG_RESET_MASK,
>  				 FIELD_PREP(ST_LSM6DSX_REG_RESET_MASK, 1));
>  	if (err < 0)
> @@ -1278,7 +1291,7 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
>  	msleep(50);
>  
>  	/* reload trimming parameter */
> -	err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_RESET_ADDR,
> +	err = regmap_update_bits(hw->regmap, hw->settings->reset_addr,
>  				 ST_LSM6DSX_REG_BOOT_MASK,
>  				 FIELD_PREP(ST_LSM6DSX_REG_BOOT_MASK, 1));
>  	if (err < 0)
> -- 
> 2.20.1
> 

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

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

[Martin Kepplinger]

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>
---
 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"
 
 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,
 	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@st.com>
@@ -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),
+};
+
 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",
+		.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

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

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 8646 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

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

>  	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@st.com>
> @@ -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?

>  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"?

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

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

[Martin Kepplinger]

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@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
> 
>> ---
>>  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@st.com>
>> @@ -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
>>

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

[Lorenzo Bianconi]

[-- 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 --]

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

[Jonathan Cameron]

On Wed, 21 Aug 2019 11:08:58 +0200
Lorenzo Bianconi <lorenzo@kernel.org> 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@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?

Hmm. Not totally obvious what to do here.  We need a name that makes it
clear this only supports part of the device.  IMU gets used for cases
including the magnetometer by some manufacturers, so not ideal, but
perhaps it's the best we can do.

Jonathan

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

[PATCH v4 3/3] dt-bindings: iio: imu: st_lsm6dsx: add lsm9ds1 device bindings

[Martin Kepplinger]

Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
---
 Documentation/devicetree/bindings/iio/imu/st_lsm6dsx.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/iio/imu/st_lsm6dsx.txt b/Documentation/devicetree/bindings/iio/imu/st_lsm6dsx.txt
index 87407d110bb7..0b0a74d9ab89 100644
--- a/Documentation/devicetree/bindings/iio/imu/st_lsm6dsx.txt
+++ b/Documentation/devicetree/bindings/iio/imu/st_lsm6dsx.txt
@@ -13,6 +13,7 @@ Required properties:
   "st,lsm6dsr"
   "st,lsm6ds3tr-c"
   "st,ism330dhcx"
+  "st,lsm9ds1"
 - reg: i2c address of the sensor / spi cs line
 
 Optional properties:
-- 
2.20.1

Re: [PATCH v4 0/3] iio: imu: st_lsm6dsx: Add support for LSM9DS1

[Jonathan Cameron]

On Tue, 13 Aug 2019 09:35:30 +0200
Martin Kepplinger <martin.kepplinger@puri.sm> wrote:

> Add basic functionality for LSM9DS1. This has become a trivial addition
> by now.
> 

To me these look fine, but I'd ideally like an Ack or reviewed-by from
Lorenzo.

Thanks,

Jonathan


> revision history
> ----------------
> v4: rebase on top of today's iio testing branch with Lorenzo's recent work
> v3: rebase and add Lorenzo's patches in order to apply to the iio testing brach
> v2: further simplifications based on Lorenzo's feedback
> v1: initial change for adding lsm9ds1 support
> 
> 
> Martin Kepplinger (3):
>   iio: imu: st_lsm6sdx: move register definitions to sensor_settings
>     struct
>   iio: imu: st_lsm6dsx: add support for accel/gyro unit of lsm9sd1
>   dt-bindings: iio: imu: st_lsm6dsx: add lsm9ds1 device bindings
> 
>  .../bindings/iio/imu/st_lsm6dsx.txt           |   1 +
>  drivers/iio/imu/st_lsm6dsx/Kconfig            |   2 +-
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h       |   8 ++
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c  | 118 ++++++++++++++++--
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c   |   5 +
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c   |   5 +
>  6 files changed, 129 insertions(+), 10 deletions(-)
>