2023-08-11 09:33:28

by Linus Walleij

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

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



2023-08-11 10:13:26

by Lorenzo Bianconi

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

> 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 <[email protected]>
> ---
[...]
> 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
>


Attachments:
(No filename) (5.05 kB)
signature.asc (235.00 B)
Download all attachments

2023-08-11 19:06:26

by Linus Walleij

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

Hi Lorenzo!

thanks for the review!

On Fri, Aug 11, 2023 at 12:07 PM Lorenzo Bianconi <[email protected]> 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

2023-08-12 10:40:46

by Lorenzo Bianconi

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

> Hi Lorenzo!
>
> thanks for the review!
>
> On Fri, Aug 11, 2023 at 12:07 PM Lorenzo Bianconi <[email protected]> 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


Attachments:
(No filename) (1.95 kB)
signature.asc (235.00 B)
Download all attachments

2023-08-12 21:47:22

by Linus Walleij

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

On Sat, Aug 12, 2023 at 11:17 AM Lorenzo Bianconi <[email protected]> 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