> 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 <linus.walleij@xxxxxxxxxx> > --- > 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/20230811-iio-spacex-lsm6ds0-v1-0-e953a440170d@xxxxxxxxxx > --- > 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 <linus.walleij@xxxxxxxxxx> >
Attachment:
signature.asc
Description: PGP signature
