> The ISM330 sensors (wai 0x6b) has a temperature channel that can
> be read out. Modify the lsm6dsx core to accomodate temperature
> readout on these sensors:
>
> - Make headspace for three members in the channels and odr_table,
> - Support offset
> - Add code to avoid configuring the ODR of the temperature
> sensor, it has no real ODR control register.
>
> This is investigated because the hardware has important real-life
> use cases using the Linux kernel.
Hi Linus,
please seem my comments inline below.
Regards,
Lorenzo
>
> Signed-off-by: Linus Walleij <[email protected]>
> ---
> The ISM330DHCX is used in the SpaceX Starlink terminals which
> are running a realtime patched close-to-mainline Linux kernel so
> let's support temperature readout on it because why not.
> SpaceX is using the temperature.
> ---
> Changes in v2:
> - Put to RFC because I can't test changes.
> - Added some mail addresses to SpaceX to the header. Maybe you
> guys can check if this works for you. Or forward to designated
> open source ambassador or whatever can help. (Addresses found
> in SpaceX code drop.)
> - Drop the code with strings for ism330dhc as we concluded that
> this is probably ism330dhcx which is already supported.
> (Would be nice if SpaceX can confirm.)
> - Don't write in nonsense register 0x0a for temperature sensor
> - More elaborate code to just avoid writing ODR for the temperature
> sensor and instead rely on gyro or accelerometer to drive
> the odr
> - Link to v1: https://lore.kernel.org/r/[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 | 79 +++++++++++++++++++++++++-
> 3 files changed, 102 insertions(+), 5 deletions(-)
>
[...]
> },
> .drdy_mask = {
> .addr = 0x13,
> @@ -869,6 +878,17 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
> .odr_avl[6] = { 833000, 0x07 },
> .odr_len = 7,
> },
> + [ST_LSM6DSX_ID_TEMP] = {
> + /*
> + * NOTE: this ODR will be capped and controllerd by the
> + * gyro and accelerometer don't have any reg to configure
> + * this ODR.
> + */
> + .odr_avl[0] = { 12500, 0x01 },
> + .odr_avl[1] = { 26000, 0x02 },
> + .odr_avl[2] = { 52000, 0x03 },
> + .odr_len = 3,
please consider we do not support low-power mode iirc (just high-performance -
bit 4 in CTRL6_C (15h)), so even enabling accel sensor, the temp sensor will
always runs at 52Hz. Here we should add just one entry, like:
.odr_avl[0] = { 52000, 0x03 },
.odr_len = 1,
> + },
> },
> .fs_table = {
> [ST_LSM6DSX_ID_ACC] = {
> @@ -937,6 +957,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,
> @@ -1618,8 +1642,8 @@ int st_lsm6dsx_check_odr(struct st_lsm6dsx_sensor *sensor, u32 odr, u8 *val)
> odr_table = &sensor->hw->settings->odr_table[sensor->id];
> for (i = 0; i < odr_table->odr_len; i++) {
> /*
> - * ext devices can run at different odr respect to
> - * accel sensor
> + * ext devices and temp sensor can run at different odr
> + * respect to accel sensor
> */
> if (odr_table->odr_avl[i].milli_hz >= odr)
> break;
> @@ -1661,6 +1685,21 @@ 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:
> + /*
> + * According to the application note AN5398 Rev 3
> + * for ISM330DHCX, section 10, page 109
> + * the ODR for the temperature sensor is equal to the
> + * accelerometer ODR if the gyroscope is powered-down,
> + * up to 52 Hz, but constant 52 Hz if the gyroscope
> + * is powered on. It never goes above 52 Hz.
> + */
> + ref_sensor = iio_priv(hw->iio_devs[ST_LSM6DSX_ID_GYRO]);
> + if ((req_odr > 0) && (hw->enable_mask |= BIT(ref_sensor->id)))
is there a typo here?
> + /* Gyro is on! ODR fixed to 52 Hz */
> + return 0;
> + /* We fall through and activate accelerometer if need be */
> + fallthrough;
I do not think this approach works since please consider what happens with the
sequence of events reported below:
- user enables gyro sensor
- user enables temp sensor
- user disables gyro sensor
IIUC the accel sensor is not enabled in this case so even the temp one will be
powered-down. Is it correct?
I think for the temp sensor we should introduce a dependency from the gyro one,
doing something like (just compiled, not tested):
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index 6a18b363cf73..ccd0d48bfb35 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -1633,19 +1633,36 @@ int st_lsm6dsx_check_odr(struct st_lsm6dsx_sensor *sensor, u32 odr, u8 *val)
}
static int
-st_lsm6dsx_check_odr_dependency(struct st_lsm6dsx_hw *hw, u32 odr,
- enum st_lsm6dsx_sensor_id id)
+st_lsm6dsx_check_odr_dependency(struct st_lsm6dsx_sensor *sensor,
+ enum st_lsm6dsx_sensor_id *odr_map,
+ int odr_map_size, u32 req_odr)
{
- struct st_lsm6dsx_sensor *ref = iio_priv(hw->iio_devs[id]);
+ struct st_lsm6dsx_hw *hw = sensor->hw;
+ int i;
- if (odr > 0) {
- if (hw->enable_mask & BIT(id))
- return max_t(u32, ref->odr, odr);
- else
- return odr;
- } else {
- return (hw->enable_mask & BIT(id)) ? ref->odr : 0;
+ for (i = 0; i < odr_map_size; i++) {
+ enum st_lsm6dsx_sensor_id id = odr_map[i];
+ struct st_lsm6dsx_sensor *ref = iio_priv(hw->iio_devs[id]);
+ u32 odr;
+
+ if (!hw->iio_devs[id] || id == sensor->id)
+ continue;
+
+ if (req_odr) {
+ if (hw->enable_mask & BIT(id))
+ odr = max_t(u32, ref->odr, req_odr);
+ else
+ odr = req_odr;
+ } else {
+ odr = hw->enable_mask & BIT(id) ? ref->odr : 0;
+ }
+
+ if (odr != req_odr)
+ /* device already configured */
+ return -EBUSY;
}
+
+ return 0;
}
static int
@@ -1659,14 +1676,30 @@ st_lsm6dsx_set_odr(struct st_lsm6dsx_sensor *sensor, u32 req_odr)
int err;
switch (sensor->id) {
- case ST_LSM6DSX_ID_GYRO:
+ case ST_LSM6DSX_ID_TEMP:
+ case ST_LSM6DSX_ID_GYRO: {
+ enum st_lsm6dsx_sensor_id odr_dep_map[] = {
+ ST_LSM6DSX_ID_GYRO,
+ ST_LSM6DSX_ID_TEMP,
+ };
+
+ ref_sensor = iio_priv(hw->iio_devs[ST_LSM6DSX_ID_GYRO]);
+ if (st_lsm6dsx_check_odr_dependency(sensor, odr_dep_map,
+ ARRAY_SIZE(odr_dep_map),
+ req_odr))
+ return 0;
break;
+ }
case ST_LSM6DSX_ID_EXT0:
case ST_LSM6DSX_ID_EXT1:
case ST_LSM6DSX_ID_EXT2:
case ST_LSM6DSX_ID_ACC: {
- u32 odr;
- int i;
+ enum st_lsm6dsx_sensor_id odr_dep_map[] = {
+ ST_LSM6DSX_ID_ACC,
+ ST_LSM6DSX_ID_EXT0,
+ ST_LSM6DSX_ID_EXT1,
+ ST_LSM6DSX_ID_EXT2,
+ };
/*
* i2c embedded controller relies on the accelerometer sensor as
@@ -1675,15 +1708,10 @@ st_lsm6dsx_set_odr(struct st_lsm6dsx_sensor *sensor, u32 req_odr)
* communicate with i2c slave devices
*/
ref_sensor = iio_priv(hw->iio_devs[ST_LSM6DSX_ID_ACC]);
- for (i = ST_LSM6DSX_ID_ACC; i < ST_LSM6DSX_ID_MAX; i++) {
- if (!hw->iio_devs[i] || i == sensor->id)
- continue;
-
- odr = st_lsm6dsx_check_odr_dependency(hw, req_odr, i);
- if (odr != req_odr)
- /* device already configured */
- return 0;
- }
+ if (st_lsm6dsx_check_odr_dependency(sensor, odr_dep_map,
+ ARRAY_SIZE(odr_dep_map),
+ req_odr))
+ return 0;
break;
}
default: /* should never occur */
What do you think? If you agree I can post it as preliminary patch (removing temp dependency).
Regards,
Lorenzo
> case ST_LSM6DSX_ID_EXT0:
> case ST_LSM6DSX_ID_EXT1:
> case ST_LSM6DSX_ID_EXT2:
> @@ -1800,6 +1839,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;
> @@ -2106,6 +2149,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 +2438,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 +2461,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;
> }
>
> ---
> base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
> change-id: 20230811-iio-spacex-lsm6ds0-33c9365e94bf
>
> Best regards,
> --
> Linus Walleij <[email protected]>
>