[PATCH 0/2] iio: imu: lsm6dsx: Support temperature and ism330dhc

[PATCH 0/2] iio: imu: lsm6dsx: Support temperature and ism330dhc

[Linus Walleij]

The ISM330DHC is used in the SpaceX Starlink terminals which
are running a realtime patched close-to-mainline Linux kernel so
let's support it because why not.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Linus Walleij (2):
      iio: lsm6dsx: Support temperature channel
      iio: imu: lsm6dsx: Add support for ism330dhc

 drivers/iio/imu/st_lsm6dsx/Kconfig             |  8 +--
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h        | 26 ++++++++-
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c |  4 ++
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c   | 73 ++++++++++++++++++++++++--
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c    |  5 ++
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c    |  4 ++
 6 files changed, 109 insertions(+), 11 deletions(-)
---
base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
change-id: 20230811-iio-spacex-lsm6ds0-33c9365e94bf

Best regards,
-- 
Linus Walleij <linus.walleij@linaro.org>

[PATCH 1/2] iio: lsm6dsx: Support temperature channel

[Linus Walleij]

The LSM6DSX (possibly other ST IMUs as well) has a temperature
channel that can be read out. Modify the lsm6dsx core to
accomodate temperature readout: make headspace for three
members in the channels and odr_table, support offset and make
the available milli_hz frequency resolution optional.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h        | 24 ++++++++-
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c |  4 ++
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c   | 67 ++++++++++++++++++++++++--
 3 files changed, 89 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
index c19237717e81..4d013889c287 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
@@ -118,6 +118,23 @@ enum st_lsm6dsx_hw_id {
 	.ext_info = st_lsm6dsx_ext_info,				\
 }
 
+#define ST_LSM6DSX_TEMP(chan_type, addr, scan_idx)			\
+{									\
+	.type = chan_type,						\
+	.address = addr,						\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
+				BIT(IIO_CHAN_INFO_SCALE) |		\
+				BIT(IIO_CHAN_INFO_OFFSET),		\
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
+	.scan_index = scan_idx,						\
+	.scan_type = {							\
+		.sign = 's',						\
+		.realbits = 16,						\
+		.storagebits = 16,					\
+		.endianness = IIO_LE,					\
+	},								\
+}
+
 struct st_lsm6dsx_reg {
 	u8 addr;
 	u8 mask;
@@ -320,7 +337,7 @@ struct st_lsm6dsx_settings {
 	struct {
 		const struct iio_chan_spec *chan;
 		int len;
-	} channels[2];
+	} channels[3];
 	struct {
 		struct st_lsm6dsx_reg irq1;
 		struct st_lsm6dsx_reg irq2;
@@ -332,7 +349,7 @@ struct st_lsm6dsx_settings {
 		struct st_lsm6dsx_reg od;
 	} irq_config;
 	struct st_lsm6dsx_reg drdy_mask;
-	struct st_lsm6dsx_odr_table_entry odr_table[2];
+	struct st_lsm6dsx_odr_table_entry odr_table[3];
 	struct st_lsm6dsx_samples_to_discard samples_to_discard[2];
 	struct st_lsm6dsx_fs_table_entry fs_table[2];
 	struct st_lsm6dsx_reg decimator[ST_LSM6DSX_MAX_ID];
@@ -346,6 +363,7 @@ struct st_lsm6dsx_settings {
 enum st_lsm6dsx_sensor_id {
 	ST_LSM6DSX_ID_GYRO,
 	ST_LSM6DSX_ID_ACC,
+	ST_LSM6DSX_ID_TEMP,
 	ST_LSM6DSX_ID_EXT0,
 	ST_LSM6DSX_ID_EXT1,
 	ST_LSM6DSX_ID_EXT2,
@@ -364,6 +382,7 @@ enum st_lsm6dsx_fifo_mode {
  * @hw: Pointer to instance of struct st_lsm6dsx_hw.
  * @gain: Configured sensor sensitivity.
  * @odr: Output data rate of the sensor [Hz].
+ * @offset: Constant offset of the sensor
  * @samples_to_discard: Number of samples to discard for filters settling time.
  * @watermark: Sensor watermark level.
  * @decimator: Sensor decimation factor.
@@ -378,6 +397,7 @@ struct st_lsm6dsx_sensor {
 
 	u32 gain;
 	u32 odr;
+	u32 offset;
 
 	u16 samples_to_discard;
 	u16 watermark;
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
index 066fe561c5e8..c588451e2ddf 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
@@ -61,6 +61,7 @@ struct st_lsm6dsx_decimator_entry {
 enum st_lsm6dsx_fifo_tag {
 	ST_LSM6DSX_GYRO_TAG = 0x01,
 	ST_LSM6DSX_ACC_TAG = 0x02,
+	ST_LSM6DSX_TEMP_TAG = 0x03,
 	ST_LSM6DSX_TS_TAG = 0x04,
 	ST_LSM6DSX_EXT0_TAG = 0x0f,
 	ST_LSM6DSX_EXT1_TAG = 0x10,
@@ -532,6 +533,9 @@ st_lsm6dsx_push_tagged_data(struct st_lsm6dsx_hw *hw, u8 tag,
 	case ST_LSM6DSX_ACC_TAG:
 		iio_dev = hw->iio_devs[ST_LSM6DSX_ID_ACC];
 		break;
+	case ST_LSM6DSX_TEMP_TAG:
+		iio_dev = hw->iio_devs[ST_LSM6DSX_ID_TEMP];
+		break;
 	case ST_LSM6DSX_EXT0_TAG:
 		if (hw->enable_mask & BIT(ST_LSM6DSX_ID_EXT0))
 			iio_dev = hw->iio_devs[ST_LSM6DSX_ID_EXT0];
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index 6a18b363cf73..c743c4871ad6 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -100,6 +100,11 @@ static const struct iio_chan_spec st_lsm6ds0_gyro_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(3),
 };
 
+static const struct iio_chan_spec st_lsm6dsx_temp_channels[] = {
+	ST_LSM6DSX_TEMP(IIO_TEMP, 0x20, 0),
+	IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
 static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 	{
 		.reset = {
@@ -835,6 +840,10 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.chan = st_lsm6dsx_gyro_channels,
 				.len = ARRAY_SIZE(st_lsm6dsx_gyro_channels),
 			},
+			[ST_LSM6DSX_ID_TEMP] = {
+				.chan = st_lsm6dsx_temp_channels,
+				.len = ARRAY_SIZE(st_lsm6dsx_temp_channels),
+			},
 		},
 		.drdy_mask = {
 			.addr = 0x13,
@@ -869,6 +878,15 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.odr_avl[6] = { 833000, 0x07 },
 				.odr_len = 7,
 			},
+			[ST_LSM6DSX_ID_TEMP] = {
+				.reg = {
+					.addr = 0x0A,
+					.mask = GENMASK(5, 4),
+				},
+				.odr_avl[0] = {  26000, 0x02 },
+				.odr_avl[1] = {  52000, 0x03 },
+				.odr_len = 2,
+			},
 		},
 		.fs_table = {
 			[ST_LSM6DSX_ID_ACC] = {
@@ -937,6 +955,10 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.addr = 0x09,
 				.mask = GENMASK(7, 4),
 			},
+			[ST_LSM6DSX_ID_TEMP] = {
+				.addr = 0x0A,
+				.mask = GENMASK(5, 4),
+			},
 		},
 		.fifo_ops = {
 			.update_fifo = st_lsm6dsx_update_fifo,
@@ -1661,6 +1683,7 @@ st_lsm6dsx_set_odr(struct st_lsm6dsx_sensor *sensor, u32 req_odr)
 	switch (sensor->id) {
 	case ST_LSM6DSX_ID_GYRO:
 		break;
+	case ST_LSM6DSX_ID_TEMP:
 	case ST_LSM6DSX_ID_EXT0:
 	case ST_LSM6DSX_ID_EXT1:
 	case ST_LSM6DSX_ID_EXT2:
@@ -1800,6 +1823,10 @@ static int st_lsm6dsx_read_raw(struct iio_dev *iio_dev,
 		*val2 = sensor->gain;
 		ret = IIO_VAL_INT_PLUS_NANO;
 		break;
+	case IIO_CHAN_INFO_OFFSET:
+		*val = sensor->offset;
+		ret = IIO_VAL_INT;
+		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -2016,9 +2043,11 @@ st_lsm6dsx_sysfs_sampling_frequency_avail(struct device *dev,
 
 	odr_table = &sensor->hw->settings->odr_table[sensor->id];
 	for (i = 0; i < odr_table->odr_len; i++)
-		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%03d ",
-				 odr_table->odr_avl[i].milli_hz / 1000,
-				 odr_table->odr_avl[i].milli_hz % 1000);
+		if (odr_table->odr_avl[i].milli_hz) {
+			len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%03d ",
+					 odr_table->odr_avl[i].milli_hz / 1000,
+					 odr_table->odr_avl[i].milli_hz % 1000);
+		}
 	buf[len - 1] = '\n';
 
 	return len;
@@ -2106,6 +2135,22 @@ static const struct iio_info st_lsm6dsx_gyro_info = {
 	.write_raw_get_fmt = st_lsm6dsx_write_raw_get_fmt,
 };
 
+static struct attribute *st_lsm6dsx_temp_attributes[] = {
+	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group st_lsm6dsx_temp_attribute_group = {
+	.attrs = st_lsm6dsx_temp_attributes,
+};
+
+static const struct iio_info st_lsm6dsx_temp_info = {
+	.attrs = &st_lsm6dsx_temp_attribute_group,
+	.read_raw = st_lsm6dsx_read_raw,
+	.write_raw = st_lsm6dsx_write_raw,
+	.hwfifo_set_watermark = st_lsm6dsx_set_watermark,
+};
+
 static int st_lsm6dsx_get_drdy_pin(struct st_lsm6dsx_hw *hw, int *drdy_pin)
 {
 	struct device *dev = hw->dev;
@@ -2379,7 +2424,16 @@ static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
 	sensor->id = id;
 	sensor->hw = hw;
 	sensor->odr = hw->settings->odr_table[id].odr_avl[0].milli_hz;
-	sensor->gain = hw->settings->fs_table[id].fs_avl[0].gain;
+	if (id == ST_LSM6DSX_ID_TEMP) {
+		/*
+		 * The temperature sensor has a fixed scale and offset such
+		 * that: temp_C = (raw / 256) + 25
+		 */
+		sensor->gain = 3906;
+		sensor->offset = 25;
+	} else {
+		sensor->gain = hw->settings->fs_table[id].fs_avl[0].gain;
+	}
 	sensor->watermark = 1;
 
 	switch (id) {
@@ -2393,6 +2447,11 @@ static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
 		scnprintf(sensor->name, sizeof(sensor->name), "%s_gyro",
 			  name);
 		break;
+	case ST_LSM6DSX_ID_TEMP:
+		iio_dev->info = &st_lsm6dsx_temp_info;
+		scnprintf(sensor->name, sizeof(sensor->name), "%s_temp",
+			  name);
+		break;
 	default:
 		return NULL;
 	}

-- 
2.34.1

Re: [PATCH 1/2] iio: lsm6dsx: Support temperature channel

[Lorenzo Bianconi]

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

> The LSM6DSX (possibly other ST IMUs as well) has a temperature
> channel that can be read out. Modify the lsm6dsx core to
> accomodate temperature readout: make headspace for three
> members in the channels and odr_table, support offset and make
> the available milli_hz frequency resolution optional.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
[...]
>  static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>  	{
>  		.reset = {
> @@ -835,6 +840,10 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>  				.chan = st_lsm6dsx_gyro_channels,
>  				.len = ARRAY_SIZE(st_lsm6dsx_gyro_channels),
>  			},
> +			[ST_LSM6DSX_ID_TEMP] = {
> +				.chan = st_lsm6dsx_temp_channels,
> +				.len = ARRAY_SIZE(st_lsm6dsx_temp_channels),
> +			},
>  		},
>  		.drdy_mask = {
>  			.addr = 0x13,
> @@ -869,6 +878,15 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>  				.odr_avl[6] = { 833000, 0x07 },
>  				.odr_len = 7,
>  			},
> +			[ST_LSM6DSX_ID_TEMP] = {
> +				.reg = {
> +					.addr = 0x0A,
> +					.mask = GENMASK(5, 4),
> +				},

looking at the ISM330DHCX datasheet, the temperature sensor ODR is just 52Hz,
while values in 0x0A register are used only for FIFO decimation, they are not
values you can configure the sensor e.g. for read_one_shot().

> +				.odr_avl[0] = {  26000, 0x02 },
> +				.odr_avl[1] = {  52000, 0x03 },
> +				.odr_len = 2,
> +			},
>  		},
>  		.fs_table = {
>  			[ST_LSM6DSX_ID_ACC] = {
> @@ -937,6 +955,10 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>  				.addr = 0x09,
>  				.mask = GENMASK(7, 4),
>  			},
> +			[ST_LSM6DSX_ID_TEMP] = {
> +				.addr = 0x0A,
> +				.mask = GENMASK(5, 4),
> +			},
>  		},
>  		.fifo_ops = {
>  			.update_fifo = st_lsm6dsx_update_fifo,
> @@ -1661,6 +1683,7 @@ st_lsm6dsx_set_odr(struct st_lsm6dsx_sensor *sensor, u32 req_odr)
>  	switch (sensor->id) {
>  	case ST_LSM6DSX_ID_GYRO:
>  		break;
> +	case ST_LSM6DSX_ID_TEMP:
>  	case ST_LSM6DSX_ID_EXT0:
>  	case ST_LSM6DSX_ID_EXT1:
>  	case ST_LSM6DSX_ID_EXT2:
> @@ -1800,6 +1823,10 @@ static int st_lsm6dsx_read_raw(struct iio_dev *iio_dev,
>  		*val2 = sensor->gain;
>  		ret = IIO_VAL_INT_PLUS_NANO;
>  		break;
> +	case IIO_CHAN_INFO_OFFSET:
> +		*val = sensor->offset;
> +		ret = IIO_VAL_INT;
> +		break;
>  	default:
>  		ret = -EINVAL;
>  		break;
> @@ -2016,9 +2043,11 @@ st_lsm6dsx_sysfs_sampling_frequency_avail(struct device *dev,
>  
>  	odr_table = &sensor->hw->settings->odr_table[sensor->id];
>  	for (i = 0; i < odr_table->odr_len; i++)
> -		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%03d ",
> -				 odr_table->odr_avl[i].milli_hz / 1000,
> -				 odr_table->odr_avl[i].milli_hz % 1000);
> +		if (odr_table->odr_avl[i].milli_hz) {
> +			len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%03d ",
> +					 odr_table->odr_avl[i].milli_hz / 1000,
> +					 odr_table->odr_avl[i].milli_hz % 1000);
> +		}
>  	buf[len - 1] = '\n';
>  
>  	return len;
> @@ -2106,6 +2135,22 @@ static const struct iio_info st_lsm6dsx_gyro_info = {
>  	.write_raw_get_fmt = st_lsm6dsx_write_raw_get_fmt,
>  };
>  
> +static struct attribute *st_lsm6dsx_temp_attributes[] = {
> +	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group st_lsm6dsx_temp_attribute_group = {
> +	.attrs = st_lsm6dsx_temp_attributes,
> +};
> +
> +static const struct iio_info st_lsm6dsx_temp_info = {
> +	.attrs = &st_lsm6dsx_temp_attribute_group,
> +	.read_raw = st_lsm6dsx_read_raw,
> +	.write_raw = st_lsm6dsx_write_raw,
> +	.hwfifo_set_watermark = st_lsm6dsx_set_watermark,
> +};
> +
>  static int st_lsm6dsx_get_drdy_pin(struct st_lsm6dsx_hw *hw, int *drdy_pin)
>  {
>  	struct device *dev = hw->dev;
> @@ -2379,7 +2424,16 @@ static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
>  	sensor->id = id;
>  	sensor->hw = hw;
>  	sensor->odr = hw->settings->odr_table[id].odr_avl[0].milli_hz;

here ODR should be 52 I guess

> -	sensor->gain = hw->settings->fs_table[id].fs_avl[0].gain;
> +	if (id == ST_LSM6DSX_ID_TEMP) {
> +		/*
> +		 * The temperature sensor has a fixed scale and offset such
> +		 * that: temp_C = (raw / 256) + 25
> +		 */
> +		sensor->gain = 3906;
> +		sensor->offset = 25;


I was wondering about the 25 costant value since ISM330DHCX ds does not provide
a typical value. Maybe we should provide a way for the user to configure it?

> +	} else {
> +		sensor->gain = hw->settings->fs_table[id].fs_avl[0].gain;
> +	}
>  	sensor->watermark = 1;
>  
>  	switch (id) {
> @@ -2393,6 +2447,11 @@ static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
>  		scnprintf(sensor->name, sizeof(sensor->name), "%s_gyro",
>  			  name);
>  		break;
> +	case ST_LSM6DSX_ID_TEMP:
> +		iio_dev->info = &st_lsm6dsx_temp_info;
> +		scnprintf(sensor->name, sizeof(sensor->name), "%s_temp",
> +			  name);

Regards,
Lorenzo

> +		break;
>  	default:
>  		return NULL;
>  	}
> 
> -- 
> 2.34.1
> 

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

Re: [PATCH 1/2] iio: lsm6dsx: Support temperature channel

[Linus Walleij]

Hi Lorenzo!

thanks for the review!

On Fri, Aug 11, 2023 at 12:07 PM Lorenzo Bianconi <lorenzo@kernel.org> wrote:

> > +                     [ST_LSM6DSX_ID_TEMP] = {
> > +                             .reg = {
> > +                                     .addr = 0x0A,
> > +                                     .mask = GENMASK(5, 4),
> > +                             },
>
> looking at the ISM330DHCX datasheet, the temperature sensor ODR is just 52Hz,
> while values in 0x0A register are used only for FIFO decimation, they are not
> values you can configure the sensor e.g. for read_one_shot().
>
> > +                             .odr_avl[0] = {  26000, 0x02 },
> > +                             .odr_avl[1] = {  52000, 0x03 },
> > +                             .odr_len = 2,

I look at page 44, paragraph 9.6 about bits 4-5:

ODR_T_BATCH_[1:0]
Selects batch data rate (write frequency in FIFO) for temperature data
(00: Temperature not batched in FIFO (default);
01: 1.6 Hz;
10: 12.5 Hz;
11: 52 Hz)

That reads to me that I should actually add the odr for 1.6 and 12.5 Hz
and the above 26 Hz is wrong but the .odr_avl[1] = {  52000, 0x03 },
52000 milli-Hz is fine?

Yours,
Linus Walleij

Re: [PATCH 1/2] iio: lsm6dsx: Support temperature channel

[Lorenzo Bianconi]

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

> Hi Lorenzo!
> 
> thanks for the review!
> 
> On Fri, Aug 11, 2023 at 12:07 PM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> 
> > > +                     [ST_LSM6DSX_ID_TEMP] = {
> > > +                             .reg = {
> > > +                                     .addr = 0x0A,
> > > +                                     .mask = GENMASK(5, 4),
> > > +                             },
> >
> > looking at the ISM330DHCX datasheet, the temperature sensor ODR is just 52Hz,
> > while values in 0x0A register are used only for FIFO decimation, they are not
> > values you can configure the sensor e.g. for read_one_shot().
> >
> > > +                             .odr_avl[0] = {  26000, 0x02 },
> > > +                             .odr_avl[1] = {  52000, 0x03 },
> > > +                             .odr_len = 2,
> 
> I look at page 44, paragraph 9.6 about bits 4-5:
> 
> ODR_T_BATCH_[1:0]
> Selects batch data rate (write frequency in FIFO) for temperature data
> (00: Temperature not batched in FIFO (default);
> 01: 1.6 Hz;
> 10: 12.5 Hz;
> 11: 52 Hz)

AFAIR the batch register is used to sub-sample sensor data before queueing them
into the FIFO (please check st_lsm6dsx_set_fifo_odr()), but it does not refer
to the configured sensor ODR.
Looking at the device application-note [0], the temperature sensor ODR depends
on the accel/gyro one:

- temperature sensor ODR == accel sensor ODR if accel ODR is < 52Hz and the
  gyro is in power-down
- temperature sensor ODR = 52Hz if accel ODR > 52Hz or if the gyro is not in
  power-down

Regards,
Lorenzo

[0] https://www.st.com/resource/en/application_note/an5398-ism330dhcx-alwayson-3d-accelerometer-and-3d-gyroscope-with-digital-output-for-industrial-applications-stmicroelectronics.pdf

> 
> That reads to me that I should actually add the odr for 1.6 and 12.5 Hz
> and the above 26 Hz is wrong but the .odr_avl[1] = {  52000, 0x03 },
> 52000 milli-Hz is fine?
> 
> Yours,
> Linus Walleij

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

Re: [PATCH 1/2] iio: lsm6dsx: Support temperature channel

[Linus Walleij]

On Sat, Aug 12, 2023 at 11:17 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:

> > > looking at the ISM330DHCX datasheet, the temperature sensor ODR is just 52Hz,
> > > while values in 0x0A register are used only for FIFO decimation, they are not
> > > values you can configure the sensor e.g. for read_one_shot().

BTW looked at this and the read_one_shot() call uses
register 0x20/0x21 as appropriate.

> > >
> > > > +                             .odr_avl[0] = {  26000, 0x02 },
> > > > +                             .odr_avl[1] = {  52000, 0x03 },
> > > > +                             .odr_len = 2,
> >
> > I look at page 44, paragraph 9.6 about bits 4-5:
> >
> > ODR_T_BATCH_[1:0]
> > Selects batch data rate (write frequency in FIFO) for temperature data
> > (00: Temperature not batched in FIFO (default);
> > 01: 1.6 Hz;
> > 10: 12.5 Hz;
> > 11: 52 Hz)
>
> AFAIR the batch register is used to sub-sample sensor data before queueing them
> into the FIFO (please check st_lsm6dsx_set_fifo_odr()), but it does not refer
> to the configured sensor ODR.
> Looking at the device application-note [0], the temperature sensor ODR depends
> on the accel/gyro one:
>
> - temperature sensor ODR == accel sensor ODR if accel ODR is < 52Hz and the
>   gyro is in power-down
> - temperature sensor ODR = 52Hz if accel ODR > 52Hz or if the gyro is not in
>   power-down

We handle the TEMP along with the EXT channels in
st_lsm6dsx_set_odr() which actually makes sure to match
the data rate of the accelerometer.

It looks as nobody cared to look into the issue with the
gyroscope though :/ It feels like a whole separate issue,
I expect more channels to be affected by that...

Yours,
Linus Walleij

Re: [PATCH 1/2] iio: lsm6dsx: Support temperature channel

[Jonathan Cameron]

On Sat, 12 Aug 2023 22:40:19 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Sat, Aug 12, 2023 at 11:17 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> 
> > > > looking at the ISM330DHCX datasheet, the temperature sensor ODR is just 52Hz,
> > > > while values in 0x0A register are used only for FIFO decimation, they are not
> > > > values you can configure the sensor e.g. for read_one_shot().  
> 
> BTW looked at this and the read_one_shot() call uses
> register 0x20/0x21 as appropriate.
> 
> > > >  
> > > > > +                             .odr_avl[0] = {  26000, 0x02 },
> > > > > +                             .odr_avl[1] = {  52000, 0x03 },
> > > > > +                             .odr_len = 2,  
> > >
> > > I look at page 44, paragraph 9.6 about bits 4-5:
> > >
> > > ODR_T_BATCH_[1:0]
> > > Selects batch data rate (write frequency in FIFO) for temperature data
> > > (00: Temperature not batched in FIFO (default);
> > > 01: 1.6 Hz;
> > > 10: 12.5 Hz;
> > > 11: 52 Hz)  
> >
> > AFAIR the batch register is used to sub-sample sensor data before queueing them
> > into the FIFO (please check st_lsm6dsx_set_fifo_odr()), but it does not refer
> > to the configured sensor ODR.
> > Looking at the device application-note [0], the temperature sensor ODR depends
> > on the accel/gyro one:
> >
> > - temperature sensor ODR == accel sensor ODR if accel ODR is < 52Hz and the
> >   gyro is in power-down
> > - temperature sensor ODR = 52Hz if accel ODR > 52Hz or if the gyro is not in
> >   power-down  
> 
> We handle the TEMP along with the EXT channels in
> st_lsm6dsx_set_odr() which actually makes sure to match
> the data rate of the accelerometer.
> 
> It looks as nobody cared to look into the issue with the
> gyroscope though :/ It feels like a whole separate issue,
> I expect more channels to be affected by that...
> 
> Yours,
> Linus Walleij
FWIW this seems crazy.  Ah well, Lorenzo is the expert on these beasts.

Jonathna

Re: [PATCH 1/2] iio: lsm6dsx: Support temperature channel

[Linus Walleij]

On Mon, Aug 28, 2023 at 6:43 PM Jonathan Cameron <jic23@kernel.org> wrote:

> FWIW this seems crazy.  Ah well, Lorenzo is the expert on these beasts.

There is some crazy duct-tape code in st_lsm6dsx_set_odr() already...
If you request the external channels, the accelerometer is turned on
to provide a "tick".

I made a patch but was icky about it since I cannot test it, I'll try to
find some people from the sources to put on CC and send it out
as an RFC.

Yours,
Linus Walleij

[PATCH 2/2] iio: imu: lsm6dsx: Add support for ism330dhc

[Linus Walleij]

The ISM330DHC sensor is used in the SpaceX Starlink ground
terminals.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/iio/imu/st_lsm6dsx/Kconfig           | 8 ++++----
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      | 2 ++
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 6 +++++-
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c  | 5 +++++
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c  | 4 ++++
 5 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/imu/st_lsm6dsx/Kconfig b/drivers/iio/imu/st_lsm6dsx/Kconfig
index 5865a295a4df..fbb5da92f954 100644
--- a/drivers/iio/imu/st_lsm6dsx/Kconfig
+++ b/drivers/iio/imu/st_lsm6dsx/Kconfig
@@ -12,10 +12,10 @@ config IIO_ST_LSM6DSX
 	help
 	  Say yes here to build support for STMicroelectronics LSM6DSx imu
 	  sensor. Supported devices: lsm6ds3, lsm6ds3h, lsm6dsl, lsm6dsm,
-	  ism330dlc, lsm6dso, lsm6dsox, asm330lhh, asm330lhhx, lsm6dsr,
-	  lsm6ds3tr-c, ism330dhcx, lsm6dsrx, lsm6ds0, lsm6dsop, lsm6dstx,
-	  lsm6dsv, lsm6dsv16x, lsm6dso16is, ism330is, asm330lhb, lsm6dst
-	  and the accelerometer/gyroscope of lsm9ds1.
+	  ism330dlc, ism330dhc, lsm6dso, lsm6dsox, asm330lhh, asm330lhhx,
+	  lsm6dsr, lsm6ds3tr-c, ism330dhcx, lsm6dsrx, lsm6ds0, lsm6dsop,
+	  lsm6dstx, lsm6dsv, lsm6dsv16x, lsm6dso16is, ism330is, asm330lhb,
+	  lsm6dst 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 4d013889c287..b702ec768067 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
@@ -20,6 +20,7 @@
 #define ST_LSM6DSL_DEV_NAME	"lsm6dsl"
 #define ST_LSM6DSM_DEV_NAME	"lsm6dsm"
 #define ST_ISM330DLC_DEV_NAME	"ism330dlc"
+#define ST_ISM330DHC_DEV_NAME	"ism330dhc"
 #define ST_LSM6DSO_DEV_NAME	"lsm6dso"
 #define ST_ASM330LHH_DEV_NAME	"asm330lhh"
 #define ST_LSM6DSOX_DEV_NAME	"lsm6dsox"
@@ -45,6 +46,7 @@ enum st_lsm6dsx_hw_id {
 	ST_LSM6DSL_ID,
 	ST_LSM6DSM_ID,
 	ST_ISM330DLC_ID,
+	ST_ISM330DHC_ID,
 	ST_LSM6DSO_ID,
 	ST_ASM330LHH_ID,
 	ST_LSM6DSOX_ID,
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index c743c4871ad6..e39651ac86fb 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -20,7 +20,7 @@
  *   - Gyroscope supported full-scale [dps]: +-125/+-245/+-500/+-1000/+-2000
  *   - FIFO size: 8KB
  *
- * - LSM6DS3H/LSM6DSL/LSM6DSM/ISM330DLC/LSM6DS3TR-C:
+ * - LSM6DS3H/LSM6DSL/LSM6DSM/ISM330DLC/ISM330DHC/LSM6DS3TR-C:
  *   - Accelerometer/Gyroscope supported ODR [Hz]: 12.5, 26, 52, 104, 208, 416
  *   - Accelerometer supported full-scale [g]: +-2/+-4/+-8/+-16
  *   - Gyroscope supported full-scale [dps]: +-125/+-245/+-500/+-1000/+-2000
@@ -809,6 +809,10 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.hw_id = ST_LSM6DSRX_ID,
 				.name = ST_LSM6DSRX_DEV_NAME,
 				.wai = 0x6b,
+			}, {
+				.hw_id = ST_ISM330DHC_ID,
+				.name = ST_ISM330DHC_DEV_NAME,
+				.wai = 0x6b,
 			}, {
 				.hw_id = ST_LSM6DSO_ID,
 				.name = ST_LSM6DSO_DEV_NAME,
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c
index 911444ec57c0..506a5a146bd5 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c
@@ -62,6 +62,10 @@ static const struct of_device_id st_lsm6dsx_i2c_of_match[] = {
 		.compatible = "st,ism330dlc",
 		.data = (void *)ST_ISM330DLC_ID,
 	},
+	{
+		.compatible = "st,ism330dhc",
+		.data = (void *)ST_ISM330DHC_ID,
+	},
 	{
 		.compatible = "st,lsm6dso",
 		.data = (void *)ST_LSM6DSO_ID,
@@ -150,6 +154,7 @@ static const struct i2c_device_id st_lsm6dsx_i2c_id_table[] = {
 	{ ST_LSM6DSL_DEV_NAME, ST_LSM6DSL_ID },
 	{ ST_LSM6DSM_DEV_NAME, ST_LSM6DSM_ID },
 	{ ST_ISM330DLC_DEV_NAME, ST_ISM330DLC_ID },
+	{ ST_ISM330DHC_DEV_NAME, ST_ISM330DHC_ID },
 	{ ST_LSM6DSO_DEV_NAME, ST_LSM6DSO_ID },
 	{ ST_ASM330LHH_DEV_NAME, ST_ASM330LHH_ID },
 	{ ST_LSM6DSOX_DEV_NAME, ST_LSM6DSOX_ID },
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c
index f56c170c41a9..5ebe59cdc35f 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c
@@ -57,6 +57,10 @@ static const struct of_device_id st_lsm6dsx_spi_of_match[] = {
 		.compatible = "st,ism330dlc",
 		.data = (void *)ST_ISM330DLC_ID,
 	},
+	{
+		.compatible = "st,ism330dhc",
+		.data = (void *)ST_ISM330DHC_ID,
+	},
 	{
 		.compatible = "st,lsm6dso",
 		.data = (void *)ST_LSM6DSO_ID,

-- 
2.34.1

Re: [PATCH 2/2] iio: imu: lsm6dsx: Add support for ism330dhc

[Lorenzo Bianconi]

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

> The ISM330DHC sensor is used in the SpaceX Starlink ground
> terminals.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/iio/imu/st_lsm6dsx/Kconfig           | 8 ++++----
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      | 2 ++
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 6 +++++-
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c  | 5 +++++
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c  | 4 ++++
>  5 files changed, 20 insertions(+), 5 deletions(-)

Hi Linus,

on st website we have just ISM330DHCX and not ISM330DHC. Should we rename it in
ISM330DHCX?

> 
> diff --git a/drivers/iio/imu/st_lsm6dsx/Kconfig b/drivers/iio/imu/st_lsm6dsx/Kconfig
> index 5865a295a4df..fbb5da92f954 100644
> --- a/drivers/iio/imu/st_lsm6dsx/Kconfig
> +++ b/drivers/iio/imu/st_lsm6dsx/Kconfig
> @@ -12,10 +12,10 @@ config IIO_ST_LSM6DSX
>  	help
>  	  Say yes here to build support for STMicroelectronics LSM6DSx imu
>  	  sensor. Supported devices: lsm6ds3, lsm6ds3h, lsm6dsl, lsm6dsm,
> -	  ism330dlc, lsm6dso, lsm6dsox, asm330lhh, asm330lhhx, lsm6dsr,
> -	  lsm6ds3tr-c, ism330dhcx, lsm6dsrx, lsm6ds0, lsm6dsop, lsm6dstx,
> -	  lsm6dsv, lsm6dsv16x, lsm6dso16is, ism330is, asm330lhb, lsm6dst
> -	  and the accelerometer/gyroscope of lsm9ds1.
> +	  ism330dlc, ism330dhc, lsm6dso, lsm6dsox, asm330lhh, asm330lhhx,
> +	  lsm6dsr, lsm6ds3tr-c, ism330dhcx, lsm6dsrx, lsm6ds0, lsm6dsop,
> +	  lsm6dstx, lsm6dsv, lsm6dsv16x, lsm6dso16is, ism330is, asm330lhb,
> +	  lsm6dst 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 4d013889c287..b702ec768067 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> @@ -20,6 +20,7 @@
>  #define ST_LSM6DSL_DEV_NAME	"lsm6dsl"
>  #define ST_LSM6DSM_DEV_NAME	"lsm6dsm"
>  #define ST_ISM330DLC_DEV_NAME	"ism330dlc"
> +#define ST_ISM330DHC_DEV_NAME	"ism330dhc"
>  #define ST_LSM6DSO_DEV_NAME	"lsm6dso"
>  #define ST_ASM330LHH_DEV_NAME	"asm330lhh"
>  #define ST_LSM6DSOX_DEV_NAME	"lsm6dsox"
> @@ -45,6 +46,7 @@ enum st_lsm6dsx_hw_id {
>  	ST_LSM6DSL_ID,
>  	ST_LSM6DSM_ID,
>  	ST_ISM330DLC_ID,
> +	ST_ISM330DHC_ID,
>  	ST_LSM6DSO_ID,
>  	ST_ASM330LHH_ID,
>  	ST_LSM6DSOX_ID,
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> index c743c4871ad6..e39651ac86fb 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -20,7 +20,7 @@
>   *   - Gyroscope supported full-scale [dps]: +-125/+-245/+-500/+-1000/+-2000
>   *   - FIFO size: 8KB
>   *
> - * - LSM6DS3H/LSM6DSL/LSM6DSM/ISM330DLC/LSM6DS3TR-C:
> + * - LSM6DS3H/LSM6DSL/LSM6DSM/ISM330DLC/ISM330DHC/LSM6DS3TR-C:
>   *   - Accelerometer/Gyroscope supported ODR [Hz]: 12.5, 26, 52, 104, 208, 416
>   *   - Accelerometer supported full-scale [g]: +-2/+-4/+-8/+-16
>   *   - Gyroscope supported full-scale [dps]: +-125/+-245/+-500/+-1000/+-2000
> @@ -809,6 +809,10 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>  				.hw_id = ST_LSM6DSRX_ID,
>  				.name = ST_LSM6DSRX_DEV_NAME,
>  				.wai = 0x6b,
> +			}, {
> +				.hw_id = ST_ISM330DHC_ID,
> +				.name = ST_ISM330DHC_DEV_NAME,
> +				.wai = 0x6b,
>  			}, {
>  				.hw_id = ST_LSM6DSO_ID,
>  				.name = ST_LSM6DSO_DEV_NAME,
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c
> index 911444ec57c0..506a5a146bd5 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c
> @@ -62,6 +62,10 @@ static const struct of_device_id st_lsm6dsx_i2c_of_match[] = {
>  		.compatible = "st,ism330dlc",
>  		.data = (void *)ST_ISM330DLC_ID,
>  	},
> +	{
> +		.compatible = "st,ism330dhc",
> +		.data = (void *)ST_ISM330DHC_ID,
> +	},
>  	{
>  		.compatible = "st,lsm6dso",
>  		.data = (void *)ST_LSM6DSO_ID,
> @@ -150,6 +154,7 @@ static const struct i2c_device_id st_lsm6dsx_i2c_id_table[] = {
>  	{ ST_LSM6DSL_DEV_NAME, ST_LSM6DSL_ID },
>  	{ ST_LSM6DSM_DEV_NAME, ST_LSM6DSM_ID },
>  	{ ST_ISM330DLC_DEV_NAME, ST_ISM330DLC_ID },
> +	{ ST_ISM330DHC_DEV_NAME, ST_ISM330DHC_ID },
>  	{ ST_LSM6DSO_DEV_NAME, ST_LSM6DSO_ID },
>  	{ ST_ASM330LHH_DEV_NAME, ST_ASM330LHH_ID },
>  	{ ST_LSM6DSOX_DEV_NAME, ST_LSM6DSOX_ID },
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c
> index f56c170c41a9..5ebe59cdc35f 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c
> @@ -57,6 +57,10 @@ static const struct of_device_id st_lsm6dsx_spi_of_match[] = {
>  		.compatible = "st,ism330dlc",
>  		.data = (void *)ST_ISM330DLC_ID,
>  	},
> +	{
> +		.compatible = "st,ism330dhc",
> +		.data = (void *)ST_ISM330DHC_ID,
> +	},
>  	{
>  		.compatible = "st,lsm6dso",
>  		.data = (void *)ST_LSM6DSO_ID,

here we are missing st_lsm6dsx_spi_id_table[] configuration.

Regards,
Lorenzo

> 
> -- 
> 2.34.1
> 

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

Re: [PATCH 0/2] iio: imu: lsm6dsx: Support temperature and ism330dhc

[Lorenzo Bianconi]

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

> The ISM330DHC is used in the SpaceX Starlink terminals which
> are running a realtime patched close-to-mainline Linux kernel so
> let's support it because why not.

Hi Linus,

this is cool, thx for working on this. Some comments in patch 1/2 and 2/2.

Regards,
Lorenzo

> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Linus Walleij (2):
>       iio: lsm6dsx: Support temperature channel
>       iio: imu: lsm6dsx: Add support for ism330dhc
> 
>  drivers/iio/imu/st_lsm6dsx/Kconfig             |  8 +--
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h        | 26 ++++++++-
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c |  4 ++
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c   | 73 ++++++++++++++++++++++++--
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c    |  5 ++
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c    |  4 ++
>  6 files changed, 109 insertions(+), 11 deletions(-)
> ---
> base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
> change-id: 20230811-iio-spacex-lsm6ds0-33c9365e94bf
> 
> Best regards,
> -- 
> Linus Walleij <linus.walleij@linaro.org>
> 

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