[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 <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(-)
  

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

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
  

> 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
  

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
  

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