Message-ID: <ZPBa40RHJ93proj0@lore-desk>
Date:   Thu, 31 Aug 2023 11:18:27 +0200
From:   Lorenzo Bianconi <lorenzo@...nel.org>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     Kevin Bosien <kbosien@...cex.com>, Jim Gruen <jgruen@...cex.com>,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Mario Tesi <mario.tesi@...com>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] RFC: iio: lsm6dsx: Support temperature channel on
 some devices

> 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@...aro.org>
> ---
> 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@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   | 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@...aro.org>
> 

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists