[PATCH v3 0/2] iio: imu: smi330: add bosch smi330 driver

[PATCH v3 0/2] iio: imu: smi330: add bosch smi330 driver

[Jianping.Shen]

From: Jianping Shen <Jianping.Shen@de.bosch.com>

Add the iio driver for bosch imu smi330. The smi330 is a combined
three axis angular rate and three axis acceleration sensor module.
This driver provides raw data access for each axis through sysfs, 
and tiggered buffer for continuous sampling.

dt-bindings:
v1 -> v2
    - Add missing type to drive-open-drain
    - Adapt description of drive-open-drain

v2 -> v3
    - No Changes

imu driver:
v1 -> v2
    - Strip back to a more minimal initial driver

v2 -> v3
    - reorganize the driver as 1 core module, 1 I2C module, and 1 SPI module.
    - remove build time INT pin choice
    - change temperature channel definition
    - improved reading data from sensor
    - simplified timestamp acquisition
    - some other minor finding fixes

Jianping Shen (2):
  dt-bindings: iio: imu: smi330: Add binding
  iio: imu: smi330: Add driver

 .../bindings/iio/imu/bosch,smi330.yaml        |  90 ++
 drivers/iio/imu/Kconfig                       |   1 +
 drivers/iio/imu/Makefile                      |   1 +
 drivers/iio/imu/smi330/Kconfig                |  39 +
 drivers/iio/imu/smi330/Makefile               |   7 +
 drivers/iio/imu/smi330/smi330.h               | 240 ++++++
 drivers/iio/imu/smi330/smi330_core.c          | 779 ++++++++++++++++++
 drivers/iio/imu/smi330/smi330_i2c.c           | 136 +++
 drivers/iio/imu/smi330/smi330_spi.c           |  91 ++
 9 files changed, 1384 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/imu/bosch,smi330.yaml
 create mode 100644 drivers/iio/imu/smi330/Kconfig
 create mode 100644 drivers/iio/imu/smi330/Makefile
 create mode 100644 drivers/iio/imu/smi330/smi330.h
 create mode 100644 drivers/iio/imu/smi330/smi330_core.c
 create mode 100644 drivers/iio/imu/smi330/smi330_i2c.c
 create mode 100644 drivers/iio/imu/smi330/smi330_spi.c

-- 
2.34.1

[PATCH v3 1/2] dt-bindings: iio: imu: smi330: Add binding

[Jianping.Shen]

From: Jianping Shen <Jianping.Shen@de.bosch.com>

Add devicetree binding for Bosch imu smi330.
The smi330 is a combined three axis angular rate and
three axis acceleration sensor module.

Signed-off-by: Jianping Shen <Jianping.Shen@de.bosch.com>
---
 .../bindings/iio/imu/bosch,smi330.yaml        | 90 +++++++++++++++++++
 1 file changed, 90 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/imu/bosch,smi330.yaml

diff --git a/Documentation/devicetree/bindings/iio/imu/bosch,smi330.yaml b/Documentation/devicetree/bindings/iio/imu/bosch,smi330.yaml
new file mode 100644
index 00000000000..0270ca456d2
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/imu/bosch,smi330.yaml
@@ -0,0 +1,90 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/imu/bosch,smi330.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Bosch SMI330 6-Axis IMU
+
+maintainers:
+  - Stefan Gutmann <stefam.gutmann@de.bosch.com>
+
+description:
+  SMI330 is a 6-axis inertial measurement unit that supports acceleration and
+  gyroscopic measurements with hardware fifo buffering. Sensor also provides
+  events information such as motion, no-motion and tilt detection.
+
+properties:
+  compatible:
+    const: bosch,smi330
+
+  reg:
+    maxItems: 1
+
+  vdd-supply:
+    description: provide VDD power to the sensor.
+
+  vddio-supply:
+    description: provide VDD IO power to the sensor.
+
+  interrupts:
+    minItems: 1
+    maxItems: 2
+
+  interrupt-names:
+    minItems: 1
+    maxItems: 2
+    items:
+      enum:
+        - INT1
+        - INT2
+
+  drive-open-drain:
+    type: boolean
+    description:
+      set if the interrupt pin(s) should be configured as
+      open drain. If not set, defaults to push-pull.
+
+required:
+  - compatible
+  - reg
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    // Example for I2C
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        imu@68 {
+            compatible = "bosch,smi330";
+            reg = <0x68>;
+            vddio-supply = <&vddio>;
+            vdd-supply = <&vdd>;
+            interrupt-parent = <&gpio>;
+            interrupts = <26 IRQ_TYPE_EDGE_RISING>;
+            interrupt-names = "INT1";
+        };
+    };
+
+    // Example for SPI
+    #include <dt-bindings/interrupt-controller/irq.h>
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        imu@0 {
+            compatible = "bosch,smi330";
+            reg = <0>;
+            spi-max-frequency = <10000000>;
+            interrupt-parent = <&gpio>;
+            interrupts = <26 IRQ_TYPE_EDGE_RISING>;
+            interrupt-names = "INT1";
+        };
+    };
-- 
2.34.1

Re: [PATCH v3 1/2] dt-bindings: iio: imu: smi330: Add binding

[Krzysztof Kozlowski]

On 03/07/2025 17:38, Jianping.Shen@de.bosch.com wrote:
> From: Jianping Shen <Jianping.Shen@de.bosch.com>
> 
> Add devicetree binding for Bosch imu smi330.
> The smi330 is a combined three axis angular rate and
> three axis acceleration sensor module.
> 
> Signed-off-by: Jianping Shen <Jianping.Shen@de.bosch.com>
<form letter>
This is a friendly reminder during the review process.

It looks like you received a tag and forgot to add it.

If you do not know the process, here is a short explanation:
Please add Acked-by/Reviewed-by/Tested-by tags when posting new versions
of patchset, under or above your Signed-off-by tag, unless patch changed
significantly (e.g. new properties added to the DT bindings). Tag is
"received", when provided in a message replied to you on the mailing
list. Tools like b4 can help here. However, there's no need to repost
patches *only* to add the tags. The upstream maintainer will do that for
tags received on the version they apply.

Please read:
https://elixir.bootlin.com/linux/v6.12-rc3/source/Documentation/process/submitting-patches.rst#L577

If a tag was not added on purpose, please state why and what changed.
</form letter>

Best regards,
Krzysztof

[PATCH v3 2/2] iio: imu: smi330: Add driver

[Jianping.Shen]

From: Jianping Shen <Jianping.Shen@de.bosch.com>

Add the iio driver for bosch imu smi330. The smi330 is a combined
three axis angular rate and three axis acceleration sensor.

Signed-off-by: Jianping Shen <Jianping.Shen@de.bosch.com>
---
 drivers/iio/imu/Kconfig              |   1 +
 drivers/iio/imu/Makefile             |   1 +
 drivers/iio/imu/smi330/Kconfig       |  39 ++
 drivers/iio/imu/smi330/Makefile      |   7 +
 drivers/iio/imu/smi330/smi330.h      | 240 +++++++++
 drivers/iio/imu/smi330/smi330_core.c | 779 +++++++++++++++++++++++++++
 drivers/iio/imu/smi330/smi330_i2c.c  | 136 +++++
 drivers/iio/imu/smi330/smi330_spi.c  |  91 ++++
 8 files changed, 1294 insertions(+)
 create mode 100644 drivers/iio/imu/smi330/Kconfig
 create mode 100644 drivers/iio/imu/smi330/Makefile
 create mode 100644 drivers/iio/imu/smi330/smi330.h
 create mode 100644 drivers/iio/imu/smi330/smi330_core.c
 create mode 100644 drivers/iio/imu/smi330/smi330_i2c.c
 create mode 100644 drivers/iio/imu/smi330/smi330_spi.c

diff --git a/drivers/iio/imu/Kconfig b/drivers/iio/imu/Kconfig
index 15612f0f189..01d21e4034c 100644
--- a/drivers/iio/imu/Kconfig
+++ b/drivers/iio/imu/Kconfig
@@ -124,6 +124,7 @@ config SMI240
 	  This driver can also be built as a module. If so, the module will be
 	  called smi240.
 
+source "drivers/iio/imu/smi330/Kconfig"
 source "drivers/iio/imu/st_lsm6dsx/Kconfig"
 source "drivers/iio/imu/st_lsm9ds0/Kconfig"
 
diff --git a/drivers/iio/imu/Makefile b/drivers/iio/imu/Makefile
index e901aea498d..25948247df9 100644
--- a/drivers/iio/imu/Makefile
+++ b/drivers/iio/imu/Makefile
@@ -31,5 +31,6 @@ obj-$(CONFIG_KMX61) += kmx61.o
 
 obj-$(CONFIG_SMI240) += smi240.o
 
+obj-y += smi330/
 obj-y += st_lsm6dsx/
 obj-y += st_lsm9ds0/
diff --git a/drivers/iio/imu/smi330/Kconfig b/drivers/iio/imu/smi330/Kconfig
new file mode 100644
index 00000000000..95b06f2317e
--- /dev/null
+++ b/drivers/iio/imu/smi330/Kconfig
@@ -0,0 +1,39 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# SMI330 IMU driver
+#
+
+config SMI330
+	tristate "Bosch Sensor SMI330 Inertial Measurement Unit"
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	help
+	  Enable support for the Bosch SMI330 IMU.
+
+	  The driver supports different operation modes like polling,
+	  data ready or fifo mode and advanced features like no-motion,
+	  no-motion, any-motion or tilt detection.
+
+config SMI330_I2C
+	tristate "Bosch SMI330 I2C driver"
+	depends on I2C
+	select SMI330
+	select REGMAP_I2C
+	help
+	  Enable support for the Bosch SMI330 6-Axis IMU connected to I2C
+	  interface.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called smi330_i2c.
+
+config SMI330_SPI
+	tristate "Bosch SMI330 SPI driver"
+	depends on SPI
+	select SMI330
+	select REGMAP_SPI
+	help
+	  Enable support for the Bosch SMI330 6-Axis IMU connected to SPI
+	  interface.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called smi330_spi.
diff --git a/drivers/iio/imu/smi330/Makefile b/drivers/iio/imu/smi330/Makefile
new file mode 100644
index 00000000000..c663dcb5a9f
--- /dev/null
+++ b/drivers/iio/imu/smi330/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for Bosch SMI330 IMU
+#
+obj-$(CONFIG_SMI330) += smi330_core.o
+obj-$(CONFIG_SMI330_I2C) += smi330_i2c.o
+obj-$(CONFIG_SMI330_SPI) += smi330_spi.o
diff --git a/drivers/iio/imu/smi330/smi330.h b/drivers/iio/imu/smi330/smi330.h
new file mode 100644
index 00000000000..bd896e86977
--- /dev/null
+++ b/drivers/iio/imu/smi330/smi330.h
@@ -0,0 +1,240 @@
+/* SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0 */
+/*
+ * Copyright (c) 2025 Robert Bosch GmbH.
+ */
+#ifndef _SMI330_H
+#define _SMI330_H
+
+#include <linux/bits.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/regmap.h>
+#include <linux/types.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/trigger.h>
+
+#define SMI330_NO_ERROR_MASK (BIT(2) | BIT(0))
+#define SMI330_ST_SUCCESS_MASK GENMASK(6, 0)
+
+#define SMI330_ALL_CHAN_MSK GENMASK(6, 0)
+
+#define SMI330_CHIP_ID 0x42
+
+#define SMI330_SPI_WR_MASK GENMASK(6, 0)
+#define SMI330_SPI_RD_MASK BIT(7)
+
+#define SMI330_SOFT_RESET_DELAY 2000
+
+/* Register map */
+#define SMI330_CHIP_ID_REG 0x00
+#define SMI330_ERR_REG 0x01
+#define SMI330_STATUS_REG 0x02
+#define SMI330_ACCEL_X_REG 0x03
+#define SMI330_GYRO_X_REG 0x06
+#define SMI330_TEMP_REG 0x09
+#define SMI330_INT1_STATUS_REG 0x0D
+#define SMI330_FEATURE_IO0_REG 0x10
+#define SMI330_FEATURE_IO1_REG 0x11
+#define SMI330_FEATURE_IO2_REG 0x12
+#define SMI330_FEATURE_IO_STATUS_REG 0x14
+#define SMI330_FIFO_FILL_LEVEL_REG 0x15
+#define SMI330_FIFO_DATA_REG 0x16
+#define SMI330_ACCEL_CFG_REG 0x20
+#define SMI330_GYRO_CFG_REG 0x21
+#define SMI330_ALT_ACCEL_CFG_REG 0x28
+#define SMI330_ALT_GYRO_CFG_REG 0x29
+#define SMI330_ALT_CONF_REG 0x2A
+#define SMI330_ALT_STATUS_REG 0x2B
+#define SMI330_FIFO_WATERMARK_REG 0x35
+#define SMI330_FIFO_CONF_REG 0x36
+#define SMI330_FIFO_CTRL_REG 0x37
+#define SMI330_IO_INT_CTRL_REG 0x38
+#define SMI330_INT_CONF_REG 0x39
+#define SMI330_INT_MAP1_REG 0x3A
+#define SMI330_INT_MAP2_REG 0x3B
+#define SMI330_FEATURE_CTRL_REG 0x40
+#define SMI330_FEATURE_DATA_ADDR_REG 0x41
+#define SMI330_FEATURE_DATA_TX_REG 0x42
+#define SMI330_FEATURE_DATA_STATUS_REG 0x43
+#define SMI330_CMD_REG 0x7E
+#define SMI330_RES_CFG_REG 0x7F
+
+/* Register mask */
+#define SMI330_ERR_FATAL_MASK BIT(0)
+#define SMI330_ERR_ACC_CONF_MASK BIT(5)
+#define SMI330_ERR_GYR_CONF_MASK BIT(6)
+#define SMI330_STATUS_POR_MASK BIT(0)
+#define SMI330_INT_STATUS_NOMO_MASK BIT(0)
+#define SMI330_INT_STATUS_ANYMO_MASK BIT(1)
+#define SMI330_INT_STATUS_TILT_MASK BIT(7)
+#define SMI330_INT_STATUS_ACC_GYR_DRDY_MASK GENMASK(13, 12)
+#define SMI330_INT_STATUS_FWM_MASK BIT(14)
+#define SMI330_INT_STATUS_FFULL_MASK BIT(15)
+#define SMI330_FEATURE_IO0_NOMO_X_EN_MASK BIT(0)
+#define SMI330_FEATURE_IO0_NOMO_Y_EN_MASK BIT(1)
+#define SMI330_FEATURE_IO0_NOMO_Z_EN_MASK BIT(2)
+#define SMI330_FEATURE_IO0_ANYMO_X_EN_MASK BIT(3)
+#define SMI330_FEATURE_IO0_ANYMO_Y_EN_MASK BIT(4)
+#define SMI330_FEATURE_IO0_ANYMO_Z_EN_MASK BIT(5)
+#define SMI330_FEATURE_IO0_TILT_EN_MASK BIT(11)
+#define SMI330_FEATURE_IO1_ERROR_MASK GENMASK(3, 0)
+#define SMI330_FEATURE_IO1_SC_COMPLETE_MASK BIT(4)
+#define SMI330_FEATURE_IO1_ST_RESULT_MASK BIT(6)
+#define SMI330_FEATURE_IO1_STATE_MASK GENMASK(12, 11)
+#define SMI330_FEATURE_IO_STATUS_MASK BIT(0)
+#define SMI330_FIFO_FILL_LEVEL_MASK GENMASK(10, 0)
+#define SMI330_CFG_MASK GENMASK(15, 0)
+#define SMI330_CFG_ODR_MASK GENMASK(3, 0)
+#define SMI330_CFG_RANGE_MASK GENMASK(6, 4)
+#define SMI330_CFG_BW_MASK BIT(7)
+#define SMI330_CFG_AVG_NUM_MASK GENMASK(10, 8)
+#define SMI330_CFG_MODE_MASK GENMASK(14, 12)
+#define SMI330_ALT_CONF_ACC_EN_MASK BIT(0)
+#define SMI330_ALT_CONF_GYR_EN_MASK BIT(4)
+#define SMI330_ALT_CONF_EN_MASK \
+	(SMI330_ALT_CONF_ACC_EN_MASK | SMI330_ALT_CONF_GYR_EN_MASK)
+#define SMI330_ALT_CONF_RST_CONF_EN_MASK BIT(8)
+#define SMI330_FIFO_WATERMARK_MASK GENMASK(9, 0)
+#define SMI330_FIFO_CONF_MASK GENMASK(11, 9)
+#define SMI330_FIFO_CONF_TEMP_MASK BIT(11)
+#define SMI330_FIFO_CONF_GYR_MASK BIT(10)
+#define SMI330_FIFO_CONF_ACC_MASK BIT(9)
+#define SMI330_FIFO_CTRL_FLUSH_MASK BIT(0)
+#define SMI330_IO_INT_CTRL_INT1_MASK GENMASK(2, 0)
+#define SMI330_IO_INT_CTRL_INT2_MASK GENMASK(10, 8)
+#define SMI330_INT_CONF_LATCH_MASK BIT(0)
+#define SMI330_INT_MAP1_TILT_MASK GENMASK(15, 14)
+#define SMI330_INT_MAP1_ANYMO_MASK GENMASK(3, 2)
+#define SMI330_INT_MAP1_NOMO_MASK GENMASK(1, 0)
+#define SMI330_INT_MAP2_FIFO_FULL_MASK GENMASK(15, 14)
+#define SMI330_INT_MAP2_FIFO_WM_MASK GENMASK(13, 12)
+#define SMI330_INT_MAP2_ACC_DRDY_MASK GENMASK(11, 10)
+#define SMI330_INT_MAP2_GYR_DRDY_MASK GENMASK(9, 8)
+#define SMI330_INT_MAP2_FIFO_MASK \
+	(SMI330_INT_MAP2_FIFO_FULL_MASK | SMI330_INT_MAP2_FIFO_WM_MASK)
+#define SMI330_INT_MAP2_DRDY_MASK \
+	(SMI330_INT_MAP2_ACC_DRDY_MASK | SMI330_INT_MAP2_GYR_DRDY_MASK)
+#define SMI330_FEATURE_DATA_STATUS_TX_READY_MASK BIT(1)
+
+/* Register values */
+#define SMI330_FEATURE_IO2_STARTUP_CONFIG 0x012C
+#define SMI330_IO_INT_CTRL_LVL BIT(0)
+#define SMI330_IO_INT_CTRL_OD BIT(1)
+#define SMI330_IO_INT_CTRL_EN BIT(2)
+#define SMI330_FEATURE_CTRL_ENABLE BIT(0)
+#define SMI330_CMD_SELF_CALIBRATION (BIT(0) | BIT(8))
+#define SMI330_CMD_SELF_TEST BIT(8)
+#define SMI330_CMD_SOFT_RESET 0xDEAF
+
+/* T°C = (temp / 512) + 23 */
+#define SMI330_TEMP_OFFSET 11776 /* 23 * 512 */
+#define SMI330_TEMP_SCALE 1953125 /* (1 / 512) * 1e9 */
+
+enum {
+	SMI330_SCAN_ACCEL_X,
+	SMI330_SCAN_ACCEL_Y,
+	SMI330_SCAN_ACCEL_Z,
+	SMI330_SCAN_GYRO_X,
+	SMI330_SCAN_GYRO_Y,
+	SMI330_SCAN_GYRO_Z,
+	SMI330_SCAN_TEMPERATURE,
+	SMI330_SCAN_TIMESTAMP,
+	SMI330_SCAN_LEN,
+};
+
+enum smi330_accel_range {
+	SMI330_ACCEL_RANGE_2G = 0x00,
+	SMI330_ACCEL_RANGE_4G = 0x01,
+	SMI330_ACCEL_RANGE_8G = 0x02,
+	SMI330_ACCEL_RANGE_16G = 0x03
+};
+
+enum smi330_gyro_range {
+	SMI330_GYRO_RANGE_125 = 0x0,
+	SMI330_GYRO_RANGE_250 = 0x01,
+	SMI330_GYRO_RANGE_500 = 0x02
+};
+
+enum smi330_odr {
+	SMI330_ODR_12_5_HZ = 0x05,
+	SMI330_ODR_25_HZ = 0x06,
+	SMI330_ODR_50_HZ = 0x07,
+	SMI330_ODR_100_HZ = 0x08,
+	SMI330_ODR_200_HZ = 0x09,
+	SMI330_ODR_400_HZ = 0x0A,
+	SMI330_ODR_800_HZ = 0x0B,
+	SMI330_ODR_1600_HZ = 0x0C,
+	SMI330_ODR_3200_HZ = 0x0D,
+	SMI330_ODR_6400_HZ = 0x0E
+};
+
+enum smi330_avg_num {
+	SMI330_AVG_NUM_1 = 0x00,
+	SMI330_AVG_NUM_2 = 0x01,
+	SMI330_AVG_NUM_4 = 0x02,
+	SMI330_AVG_NUM_8 = 0x03,
+	SMI330_AVG_NUM_16 = 0x04,
+	SMI330_AVG_NUM_32 = 0x05,
+	SMI330_AVG_NUM_64 = 0x06
+};
+
+enum smi330_mode {
+	SMI330_MODE_SUSPEND = 0x00,
+	SMI330_MODE_GYRO_DRIVE = 0x01,
+	SMI330_MODE_LOW_POWER = 0x03,
+	SMI330_MODE_NORMAL = 0x04,
+	SMI330_MODE_HIGH_PERF = 0x07
+};
+
+enum smi330_bw {
+	SMI330_BW_2 = 0x00, /* ODR/2 */
+	SMI330_BW_4 = 0x01 /* ODR/4 */
+};
+
+enum smi330_operation_mode {
+	SMI330_POLLING,
+	SMI330_DATA_READY,
+};
+
+enum smi330_sensor {
+	SMI330_ACCEL,
+	SMI330_GYRO,
+};
+
+enum smi330_sensor_conf_select {
+	SMI330_ODR,
+	SMI330_RANGE,
+	SMI330_BW,
+	SMI330_AVG_NUM,
+};
+
+enum smi330_int_out {
+	SMI330_INT_DISABLED,
+	SMI330_INT_1,
+	SMI330_INT_2,
+};
+
+struct smi330_attributes {
+	int *reg_vals;
+	int *vals;
+	int len;
+	int type;
+	int mask;
+};
+
+struct smi330_cfg {
+	enum smi330_operation_mode op_mode;
+	enum smi330_int_out data_irq;
+};
+
+struct smi330_data {
+	struct regmap *regmap;
+	struct smi330_cfg cfg;
+	struct iio_trigger *trig;
+	IIO_DECLARE_BUFFER_WITH_TS(s16, buf, SMI330_SCAN_LEN);
+};
+
+int smi330_core_probe(struct device *dev, struct regmap *regmap);
+
+#endif /* _SMI330_H */
diff --git a/drivers/iio/imu/smi330/smi330_core.c b/drivers/iio/imu/smi330/smi330_core.c
new file mode 100644
index 00000000000..23e65c1ed64
--- /dev/null
+++ b/drivers/iio/imu/smi330/smi330_core.c
@@ -0,0 +1,779 @@
+// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
+/*
+ * Copyright (c) 2025 Robert Bosch GmbH.
+ */
+#include <linux/bitfield.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/string.h>
+#include <linux/units.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#include "smi330.h"
+
+#define SMI330_ACCEL_CHANNEL(_type, _axis, _index) {			\
+	.type = _type,							\
+	.modified = 1,							\
+	.channel2 = IIO_MOD_##_axis,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
+	.info_mask_shared_by_type =					\
+		BIT(IIO_CHAN_INFO_SCALE) |				\
+		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |			\
+		BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),	\
+	.info_mask_shared_by_type_available =				\
+		BIT(IIO_CHAN_INFO_SCALE) |				\
+		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |			\
+		BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),	\
+	.info_mask_shared_by_dir =					\
+		BIT(IIO_CHAN_INFO_SAMP_FREQ),				\
+	.info_mask_shared_by_dir_available =				\
+		BIT(IIO_CHAN_INFO_SAMP_FREQ),				\
+	.scan_index = _index,						\
+	.scan_type = {							\
+		.sign = 's',						\
+		.realbits = 16,						\
+		.storagebits = 16,					\
+		.endianness = IIO_LE,					\
+	},								\
+}
+
+#define SMI330_GYRO_CHANNEL(_type, _axis, _index) {			\
+	.type = _type,							\
+	.modified = 1,							\
+	.channel2 = IIO_MOD_##_axis,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
+	.info_mask_shared_by_type =					\
+		BIT(IIO_CHAN_INFO_SCALE) |				\
+		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |			\
+		BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),	\
+	.info_mask_shared_by_type_available =				\
+		BIT(IIO_CHAN_INFO_SCALE) |				\
+		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |			\
+		BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),	\
+	.info_mask_shared_by_dir =					\
+		BIT(IIO_CHAN_INFO_SAMP_FREQ),				\
+	.info_mask_shared_by_dir_available =				\
+		BIT(IIO_CHAN_INFO_SAMP_FREQ),				\
+	.scan_index = _index,						\
+	.scan_type = {							\
+		.sign = 's',						\
+		.realbits = 16,						\
+		.storagebits = 16,					\
+		.endianness = IIO_LE,					\
+	},								\
+}
+
+#define SMI330_TEMP_CHANNEL(_index) {			\
+	.type = IIO_TEMP,				\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
+		BIT(IIO_CHAN_INFO_OFFSET) |		\
+		BIT(IIO_CHAN_INFO_SCALE),		\
+	.scan_index = _index,				\
+	.scan_type = {					\
+		.sign = 's',				\
+		.realbits = 16,				\
+		.storagebits = 16,			\
+		.endianness = IIO_LE,			\
+	},						\
+}
+
+static const struct iio_chan_spec smi330_channels[] = {
+	SMI330_ACCEL_CHANNEL(IIO_ACCEL, X, SMI330_SCAN_ACCEL_X),
+	SMI330_ACCEL_CHANNEL(IIO_ACCEL, Y, SMI330_SCAN_ACCEL_Y),
+	SMI330_ACCEL_CHANNEL(IIO_ACCEL, Z, SMI330_SCAN_ACCEL_Z),
+	SMI330_GYRO_CHANNEL(IIO_ANGL_VEL, X, SMI330_SCAN_GYRO_X),
+	SMI330_GYRO_CHANNEL(IIO_ANGL_VEL, Y, SMI330_SCAN_GYRO_Y),
+	SMI330_GYRO_CHANNEL(IIO_ANGL_VEL, Z, SMI330_SCAN_GYRO_Z),
+	SMI330_TEMP_CHANNEL(SMI330_SCAN_TEMPERATURE),
+	IIO_CHAN_SOFT_TIMESTAMP(SMI330_SCAN_TIMESTAMP),
+};
+
+static const struct smi330_attributes smi330_accel_scale_attr = {
+	.reg_vals = (int[]){ SMI330_ACCEL_RANGE_2G, SMI330_ACCEL_RANGE_4G,
+			     SMI330_ACCEL_RANGE_8G, SMI330_ACCEL_RANGE_16G },
+	.vals = (int[]){ 0, 61035, 0, 122070, 0, 244140, 0, 488281 },
+	.len = 8,
+	.type = IIO_VAL_INT_PLUS_NANO,
+	.mask = SMI330_CFG_RANGE_MASK
+};
+
+static const struct smi330_attributes smi330_gyro_scale_attr = {
+	.reg_vals = (int[]){ SMI330_GYRO_RANGE_125, SMI330_GYRO_RANGE_250,
+			     SMI330_GYRO_RANGE_500 },
+	.vals = (int[]){ 0, 3814697, 0, 7629395, 0, 15258789 },
+	.len = 6,
+	.type = IIO_VAL_INT_PLUS_NANO,
+	.mask = SMI330_CFG_RANGE_MASK
+};
+
+static const struct smi330_attributes smi330_average_attr = {
+	.reg_vals = (int[]){ SMI330_AVG_NUM_1, SMI330_AVG_NUM_2,
+			     SMI330_AVG_NUM_4, SMI330_AVG_NUM_8,
+			     SMI330_AVG_NUM_16, SMI330_AVG_NUM_32,
+			     SMI330_AVG_NUM_64 },
+	.vals = (int[]){ 1, 2, 4, 8, 16, 32, 64 },
+	.len = 7,
+	.type = IIO_VAL_INT,
+	.mask = SMI330_CFG_AVG_NUM_MASK
+};
+
+static const struct smi330_attributes smi330_bandwidth_attr = {
+	.reg_vals = (int[]){ SMI330_BW_2, SMI330_BW_4 },
+	.vals = (int[]){ 2, 4 },
+	.len = 2,
+	.type = IIO_VAL_INT,
+	.mask = SMI330_CFG_BW_MASK
+};
+
+static const struct smi330_attributes smi330_odr_attr = {
+	.reg_vals = (int[]){ SMI330_ODR_12_5_HZ, SMI330_ODR_25_HZ,
+			     SMI330_ODR_50_HZ, SMI330_ODR_100_HZ,
+			     SMI330_ODR_200_HZ, SMI330_ODR_400_HZ,
+			     SMI330_ODR_800_HZ, SMI330_ODR_1600_HZ,
+			     SMI330_ODR_3200_HZ, SMI330_ODR_6400_HZ },
+	.vals = (int[]){ 12, 25, 50, 100, 200, 400, 800, 1600, 3200, 6400 },
+	.len = 10,
+	.type = IIO_VAL_INT,
+	.mask = SMI330_CFG_ODR_MASK
+};
+
+static int smi330_get_attributes(enum smi330_sensor_conf_select config,
+				 enum smi330_sensor sensor,
+				 const struct smi330_attributes **attr)
+{
+	switch (config) {
+	case SMI330_ODR:
+		*attr = &smi330_odr_attr;
+		return 0;
+	case SMI330_RANGE:
+		if (sensor == SMI330_ACCEL)
+			*attr = &smi330_accel_scale_attr;
+		else
+			*attr = &smi330_gyro_scale_attr;
+		return 0;
+	case SMI330_BW:
+		*attr = &smi330_bandwidth_attr;
+		return 0;
+	case SMI330_AVG_NUM:
+		*attr = &smi330_average_attr;
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int smi330_get_config_reg(enum smi330_sensor sensor, int *reg)
+{
+	switch (sensor) {
+	case SMI330_ACCEL:
+		*reg = SMI330_ACCEL_CFG_REG;
+		return 0;
+	case SMI330_GYRO:
+		*reg = SMI330_GYRO_CFG_REG;
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int smi330_get_sensor_config(struct smi330_data *data,
+				    enum smi330_sensor sensor,
+				    enum smi330_sensor_conf_select config,
+				    int *value)
+
+{
+	int ret, reg, reg_val, i;
+	const struct smi330_attributes *attr;
+
+	ret = smi330_get_config_reg(sensor, &reg);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(data->regmap, reg, &reg_val);
+	if (ret)
+		return ret;
+
+	ret = smi330_get_attributes(config, sensor, &attr);
+	if (ret)
+		return ret;
+
+	/* FIELD_GET is not possible with non-const mask */
+	reg_val = (reg_val & attr->mask) >> (__builtin_ffs(attr->mask) - 1);
+
+	if (attr->type == IIO_VAL_INT) {
+		for (i = 0; i < attr->len; i++) {
+			if (attr->reg_vals[i] == reg_val) {
+				*value = attr->vals[i];
+				return 0;
+			}
+		}
+	} else {
+		for (i = 0; i < attr->len / 2; i++) {
+			if (attr->reg_vals[i] == reg_val) {
+				*value = attr->vals[2 * i + 1];
+				return 0;
+			}
+		}
+	}
+
+	return -EINVAL;
+}
+
+static int smi330_set_sensor_config(struct smi330_data *data,
+				    enum smi330_sensor sensor,
+				    enum smi330_sensor_conf_select config,
+				    int value)
+{
+	int ret, i, reg, reg_val, error;
+	const struct smi330_attributes *attr;
+
+	ret = smi330_get_attributes(config, sensor, &attr);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < attr->len; i++) {
+		if (attr->vals[i] == value) {
+			if (attr->type == IIO_VAL_INT)
+				reg_val = attr->reg_vals[i];
+			else
+				reg_val = attr->reg_vals[i / 2];
+			break;
+		}
+	}
+	if (i == attr->len)
+		return -EINVAL;
+
+	ret = smi330_get_config_reg(sensor, &reg);
+	if (ret)
+		return ret;
+
+	/* FIELD_PREP is not possible with non-const mask */
+	reg_val = ((reg_val << (__builtin_ffs(attr->mask) - 1)) & attr->mask);
+
+	ret = regmap_update_bits(data->regmap, reg, attr->mask, reg_val);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(data->regmap, SMI330_ERR_REG, &error);
+	if (ret)
+		return ret;
+
+	if (FIELD_GET(SMI330_ERR_ACC_CONF_MASK, error) ||
+	    FIELD_GET(SMI330_ERR_GYR_CONF_MASK, error))
+		return -EIO;
+
+	return 0;
+}
+
+static int smi330_get_data(struct smi330_data *data, int chan_type, int axis,
+			   int *val)
+{
+	u8 reg;
+	int ret, sample;
+
+	switch (chan_type) {
+	case IIO_ACCEL:
+		reg = SMI330_ACCEL_X_REG + (axis - IIO_MOD_X);
+		break;
+	case IIO_ANGL_VEL:
+		reg = SMI330_GYRO_X_REG + (axis - IIO_MOD_X);
+		break;
+	case IIO_TEMP:
+		reg = SMI330_TEMP_REG;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = regmap_read(data->regmap, reg, &sample);
+	if (ret)
+		return ret;
+
+	*val = sign_extend32(sample, 15);
+
+	return 0;
+}
+
+static int smi330_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan, const int **vals,
+			     int *type, int *length, long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		if (chan->type == IIO_ACCEL) {
+			*vals = smi330_accel_scale_attr.vals;
+			*length = smi330_accel_scale_attr.len;
+			*type = smi330_accel_scale_attr.type;
+		} else {
+			*vals = smi330_gyro_scale_attr.vals;
+			*length = smi330_gyro_scale_attr.len;
+			*type = smi330_gyro_scale_attr.type;
+		}
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		*vals = smi330_average_attr.vals;
+		*length = smi330_average_attr.len;
+		*type = smi330_average_attr.type;
+		*type = IIO_VAL_INT;
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		*vals = smi330_bandwidth_attr.vals;
+		*length = smi330_bandwidth_attr.len;
+		*type = smi330_bandwidth_attr.type;
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*vals = smi330_odr_attr.vals;
+		*length = smi330_odr_attr.len;
+		*type = smi330_odr_attr.type;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+
+	return -EINVAL;
+}
+
+static int smi330_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan, int *val,
+			   int *val2, long mask)
+{
+	int ret;
+	struct smi330_data *data = iio_priv(indio_dev);
+	enum smi330_sensor sensor;
+
+	/* valid for all channel types */
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (!iio_device_claim_direct(indio_dev))
+			return -EBUSY;
+		ret = smi330_get_data(data, chan->type, chan->channel2, val);
+		iio_device_release_direct(indio_dev);
+		return ret ? ret : IIO_VAL_INT;
+	default:
+		break;
+	}
+
+	switch (chan->type) {
+	case IIO_ACCEL:
+		sensor = SMI330_ACCEL;
+		break;
+	case IIO_ANGL_VEL:
+		sensor = SMI330_GYRO;
+		break;
+	case IIO_TEMP:
+		switch (mask) {
+		case IIO_CHAN_INFO_SCALE:
+			*val = SMI330_TEMP_SCALE / GIGA;
+			*val2 = SMI330_TEMP_SCALE % GIGA;
+			return IIO_VAL_INT_PLUS_NANO;
+		case IIO_CHAN_INFO_OFFSET:
+			*val = SMI330_TEMP_OFFSET;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+
+	/* valid for acc and gyro channels */
+	switch (mask) {
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		ret = smi330_get_sensor_config(data, sensor, SMI330_AVG_NUM,
+					       val);
+		return ret ? ret : IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		ret = smi330_get_sensor_config(data, sensor, SMI330_BW, val);
+		return ret ? ret : IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = smi330_get_sensor_config(data, sensor, SMI330_ODR, val);
+		return ret ? ret : IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		*val = 0;
+		ret = smi330_get_sensor_config(data, sensor, SMI330_RANGE,
+					       val2);
+		return ret ? ret : IIO_VAL_INT_PLUS_NANO;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int smi330_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan, int val, int val2,
+			    long mask)
+{
+	int ret;
+	struct smi330_data *data = iio_priv(indio_dev);
+	enum smi330_sensor sensor;
+
+	switch (chan->type) {
+	case IIO_ACCEL:
+		sensor = SMI330_ACCEL;
+		break;
+	case IIO_ANGL_VEL:
+		sensor = SMI330_GYRO;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		return smi330_set_sensor_config(data, sensor, SMI330_RANGE,
+						val2);
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		return smi330_set_sensor_config(data, sensor, SMI330_AVG_NUM,
+						val);
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		return smi330_set_sensor_config(data, sensor, SMI330_BW, val);
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = smi330_set_sensor_config(data, SMI330_ACCEL, SMI330_ODR,
+					       val);
+		if (ret)
+			return ret;
+
+		return smi330_set_sensor_config(data, SMI330_GYRO, SMI330_ODR,
+						val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int smi330_write_raw_get_fmt(struct iio_dev *indio_dev,
+				    struct iio_chan_spec const *chan, long info)
+{
+	switch (info) {
+	case IIO_CHAN_INFO_SCALE:
+		return IIO_VAL_INT_PLUS_NANO;
+	default:
+		return IIO_VAL_INT_PLUS_MICRO;
+	}
+}
+
+static int smi330_soft_reset(struct smi330_data *data)
+{
+	int ret, dummy_byte;
+
+	ret = regmap_write(data->regmap, SMI330_CMD_REG, SMI330_CMD_SOFT_RESET);
+	if (ret)
+		return ret;
+	fsleep(SMI330_SOFT_RESET_DELAY);
+
+	/* Performing a dummy read after a soft-reset */
+	regmap_read(data->regmap, SMI330_CHIP_ID_REG, &dummy_byte);
+
+	return 0;
+}
+
+static irqreturn_t smi330_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct smi330_data *data = iio_priv(indio_dev);
+	int ret, chan;
+	int i = 0;
+
+	ret = regmap_bulk_read(data->regmap, SMI330_ACCEL_X_REG, data->buf,
+			       ARRAY_SIZE(smi330_channels));
+	if (ret)
+		goto out;
+
+	if (*indio_dev->active_scan_mask != SMI330_ALL_CHAN_MSK) {
+		iio_for_each_active_channel(indio_dev, chan)
+			data->buf[i++] = data->buf[chan];
+	}
+
+	iio_push_to_buffers_with_timestamp(indio_dev, data->buf, pf->timestamp);
+
+out:
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t smi330_irq_thread_handler(int irq, void *indio_dev_)
+{
+	int ret, int_stat;
+	s16 int_status[2] = { 0 };
+	struct iio_dev *indio_dev = indio_dev_;
+	struct smi330_data *data = iio_priv(indio_dev);
+
+	ret = regmap_bulk_read(data->regmap, SMI330_INT1_STATUS_REG, int_status, 2);
+	if (ret)
+		return IRQ_NONE;
+
+	int_stat = int_status[0] | int_status[1];
+
+	if (FIELD_GET(SMI330_INT_STATUS_ACC_GYR_DRDY_MASK, int_stat)) {
+		indio_dev->pollfunc->timestamp = iio_get_time_ns(indio_dev);
+		iio_trigger_poll_nested(data->trig);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int smi330_set_int_pin_config(struct smi330_data *data,
+				     enum smi330_int_out irq_num,
+				     bool active_high, bool open_drain,
+				     bool latch)
+{
+	int ret, val;
+
+	val = active_high ? SMI330_IO_INT_CTRL_LVL : 0;
+	val |= open_drain ? SMI330_IO_INT_CTRL_OD : 0;
+	val |= SMI330_IO_INT_CTRL_EN;
+
+	switch (irq_num) {
+	case SMI330_INT_1:
+		val = FIELD_PREP(SMI330_IO_INT_CTRL_INT1_MASK, val);
+		ret = regmap_update_bits(data->regmap, SMI330_IO_INT_CTRL_REG,
+					 SMI330_IO_INT_CTRL_INT1_MASK, val);
+		if (ret)
+			return ret;
+		break;
+	case SMI330_INT_2:
+		val = FIELD_PREP(SMI330_IO_INT_CTRL_INT2_MASK, val);
+		ret = regmap_update_bits(data->regmap, SMI330_IO_INT_CTRL_REG,
+					 SMI330_IO_INT_CTRL_INT2_MASK, val);
+		if (ret)
+			return ret;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return regmap_update_bits(data->regmap, SMI330_INT_CONF_REG,
+				  SMI330_INT_CONF_LATCH_MASK,
+				  FIELD_PREP(SMI330_INT_CONF_LATCH_MASK,
+					     latch));
+}
+
+static int smi330_setup_irq(struct device *dev, struct iio_dev *indio_dev,
+			    int irq, enum smi330_int_out irq_num)
+{
+	int ret, irq_type;
+	bool open_drain, active_high, latch;
+	struct smi330_data *data = iio_priv(indio_dev);
+	struct irq_data *desc;
+
+	desc = irq_get_irq_data(irq);
+	if (!desc)
+		return -EINVAL;
+
+	irq_type = irqd_get_trigger_type(desc);
+	switch (irq_type) {
+	case IRQF_TRIGGER_RISING:
+		latch = false;
+		active_high = true;
+		break;
+	case IRQF_TRIGGER_HIGH:
+		latch = true;
+		active_high = true;
+		break;
+	case IRQF_TRIGGER_FALLING:
+		latch = false;
+		active_high = false;
+		break;
+	case IRQF_TRIGGER_LOW:
+		latch = true;
+		active_high = false;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	open_drain = device_property_read_bool(dev, "drive-open-drain");
+
+	ret = smi330_set_int_pin_config(data, irq_num, active_high, open_drain,
+					latch);
+	if (ret)
+		return ret;
+
+	return devm_request_threaded_irq(dev, irq, NULL,
+					 smi330_irq_thread_handler,
+					 irq_type | IRQF_ONESHOT,
+					 indio_dev->name, indio_dev);
+}
+
+static int smi330_register_irq(struct device *dev, struct iio_dev *indio_dev)
+{
+	int ret, irq;
+	struct smi330_data *data = iio_priv(indio_dev);
+	struct fwnode_handle *fwnode;
+
+	fwnode = dev_fwnode(dev);
+	if (!fwnode)
+		return -ENODEV;
+
+	data->cfg.data_irq = SMI330_INT_DISABLED;
+
+	irq = fwnode_irq_get_byname(fwnode, "INT1");
+	if (irq > 0) {
+		ret = smi330_setup_irq(dev, indio_dev, irq, SMI330_INT_1);
+		if (ret)
+			return ret;
+		data->cfg.data_irq = SMI330_INT_1;
+	} else {
+		irq = fwnode_irq_get_byname(fwnode, "INT2");
+		if (irq > 0) {
+			ret = smi330_setup_irq(dev, indio_dev, irq,
+					       SMI330_INT_2);
+			if (ret)
+				return ret;
+			data->cfg.data_irq = SMI330_INT_2;
+		}
+	}
+
+	return 0;
+}
+
+static int smi330_set_drdy_trigger_state(struct iio_trigger *trig, bool enable)
+{
+	int val;
+	struct smi330_data *data = iio_trigger_get_drvdata(trig);
+
+	if (enable)
+		data->cfg.op_mode = SMI330_DATA_READY;
+	else
+		data->cfg.op_mode = SMI330_POLLING;
+
+	val = FIELD_PREP(SMI330_INT_MAP2_ACC_DRDY_MASK,
+			 enable ? data->cfg.data_irq : 0);
+	val |= FIELD_PREP(SMI330_INT_MAP2_GYR_DRDY_MASK,
+			  enable ? data->cfg.data_irq : 0);
+	return regmap_update_bits(data->regmap, SMI330_INT_MAP2_REG,
+				  SMI330_INT_MAP2_DRDY_MASK, val);
+}
+
+static const struct iio_trigger_ops smi330_trigger_ops = {
+	.set_trigger_state = &smi330_set_drdy_trigger_state,
+};
+
+static struct iio_info smi330_info = {
+	.read_avail = smi330_read_avail,
+	.read_raw = smi330_read_raw,
+	.write_raw = smi330_write_raw,
+	.write_raw_get_fmt = smi330_write_raw_get_fmt,
+};
+
+static int smi330_dev_init(struct smi330_data *data)
+{
+	int ret, chip_id, val, mode;
+	struct device *dev = regmap_get_device(data->regmap);
+
+	ret = regmap_read(data->regmap, SMI330_CHIP_ID_REG, &chip_id);
+	if (ret)
+		return ret;
+
+	chip_id &= 0x00FF;
+
+	if (chip_id != SMI330_CHIP_ID)
+		dev_info(dev, "Unknown chip id: 0x%04x\n", chip_id);
+
+	ret = regmap_read(data->regmap, SMI330_ERR_REG, &val);
+	if (ret)
+		return ret;
+	if (FIELD_GET(SMI330_ERR_FATAL_MASK, val))
+		return -ENODEV;
+
+	ret = regmap_read(data->regmap, SMI330_STATUS_REG, &val);
+	if (ret)
+		return ret;
+	if (FIELD_GET(SMI330_STATUS_POR_MASK, val) == 0)
+		return -ENODEV;
+
+	mode = FIELD_PREP(SMI330_CFG_MODE_MASK, SMI330_MODE_NORMAL);
+
+	ret = regmap_update_bits(data->regmap, SMI330_ACCEL_CFG_REG,
+				 SMI330_CFG_MODE_MASK, mode);
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(data->regmap, SMI330_GYRO_CFG_REG,
+				  SMI330_CFG_MODE_MASK, mode);
+}
+
+int smi330_core_probe(struct device *dev, struct regmap *regmap)
+{
+	int ret;
+	struct iio_dev *indio_dev;
+	struct smi330_data *data;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	data->regmap = regmap;
+
+	ret = smi330_soft_reset(data);
+	if (ret)
+		return dev_err_probe(dev, ret, "Soft reset failed\n");
+
+	indio_dev->channels = smi330_channels;
+	indio_dev->num_channels = ARRAY_SIZE(smi330_channels);
+	indio_dev->name = "smi330";
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &smi330_info;
+
+	data->cfg.op_mode = SMI330_POLLING;
+
+	ret = smi330_dev_init(data);
+	if (ret)
+		return dev_err_probe(dev, ret, "Init failed\n");
+
+	ret = smi330_register_irq(dev, indio_dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Register IRQ failed\n");
+
+	if (data->cfg.data_irq != SMI330_INT_DISABLED) {
+		data->trig = devm_iio_trigger_alloc(dev, "%s-drdy-trigger",
+						    indio_dev->name);
+		if (!data->trig)
+			return -ENOMEM;
+
+		data->trig->ops = &smi330_trigger_ops;
+		iio_trigger_set_drvdata(data->trig, data);
+
+		ret = devm_iio_trigger_register(dev, data->trig);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "IIO register trigger failed\n");
+
+		/* Set default operation mode to data ready. */
+		indio_dev->trig = iio_trigger_get(data->trig);
+	}
+
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+					      iio_pollfunc_store_time,
+					      smi330_trigger_handler, NULL);
+	if (ret)
+		return dev_err_probe(dev, ret, "IIO buffer setup failed\n");
+
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Register IIO device failed\n");
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(smi330_core_probe, "IIO_SMI330");
+
+MODULE_AUTHOR("Stefan Gutmann <stefan.gutmann@de.bosch.com>");
+MODULE_AUTHOR("Roman Huber <roman.huber@de.bosch.com>");
+MODULE_AUTHOR("Filip Andrei <Andrei.Filip@ro.bosch.com>");
+MODULE_AUTHOR("Drimbarean Avram Andrei <Avram-Andrei.Drimbarean@ro.bosch.com>");
+MODULE_DESCRIPTION("Bosch SMI330 IMU driver");
+MODULE_LICENSE("Dual BSD/GPL");
diff --git a/drivers/iio/imu/smi330/smi330_i2c.c b/drivers/iio/imu/smi330/smi330_i2c.c
new file mode 100644
index 00000000000..76b88bbd7d2
--- /dev/null
+++ b/drivers/iio/imu/smi330/smi330_i2c.c
@@ -0,0 +1,136 @@
+// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
+/*
+ * Copyright (c) 2025 Robert Bosch GmbH.
+ */
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+
+#include "smi330.h"
+
+#define SMI330_NUM_DUMMY_BYTES 2
+#define SMI330_I2C_MAX_RX_BUFFER_SIZE \
+	(SMI330_NUM_DUMMY_BYTES + SMI330_SCAN_LEN * sizeof(s16))
+
+struct smi330_i2c_priv {
+	struct i2c_client *i2c;
+	u8 rx_buffer[SMI330_I2C_MAX_RX_BUFFER_SIZE];
+};
+
+static int smi330_regmap_i2c_read(void *context, const void *reg_buf,
+				  size_t reg_size, void *val_buf,
+				  size_t val_size)
+{
+	struct smi330_i2c_priv *priv = context;
+	int ret, retry;
+
+	/*
+	 * SMI330 I2C read frame:
+	 * <Slave address[6:0], RnW> <x, Register address[6:0]>
+	 * <Slave address[6:0], RnW> <Dummy[7:0]> <Dummy[7:0]> <Data_0[7:0]> <Data_1[15:8]>...
+	 *                                                     <Data_N[7:0]> <Data_N[15:8]>
+	 * Remark: Slave address is not considered part of the frame in the following definitions
+	 */
+	struct i2c_msg msgs[] = {
+		{
+			.addr = priv->i2c->addr,
+			.flags = priv->i2c->flags,
+			.len = reg_size,
+			.buf = (u8 *)reg_buf,
+		},
+		{
+			.addr = priv->i2c->addr,
+			.flags = priv->i2c->flags | I2C_M_RD,
+			.len = SMI330_NUM_DUMMY_BYTES + val_size,
+			.buf = priv->rx_buffer,
+		},
+	};
+
+	ret = i2c_transfer(priv->i2c->adapter, msgs, ARRAY_SIZE(msgs));
+	if (ret < 0)
+		return ret;
+
+	memcpy(val_buf, priv->rx_buffer + SMI330_NUM_DUMMY_BYTES, val_size);
+
+	return 0;
+}
+
+static int smi330_regmap_i2c_write(void *context, const void *data,
+				   size_t count)
+{
+	struct smi330_i2c_priv *priv = context;
+	u8 reg;
+
+	/*
+	 * SMI330 I2C write frame:
+	 * <Slave address[6:0], RnW> <x, Register address[6:0]> <Data_0[7:0]> <Data_1[15:8]>...
+	 *                                                      <Data_N[7:0]> <Data_N[15:8]>
+	 * Remark: Slave address is not considered part of the frame in the following definitions
+	 */
+	reg = *(u8 *)data;
+	return i2c_smbus_write_i2c_block_data(priv->i2c, reg,
+					      count - sizeof(u8),
+					      data + sizeof(u8));
+}
+
+static const struct regmap_bus smi330_regmap_bus = {
+	.read = smi330_regmap_i2c_read,
+	.write = smi330_regmap_i2c_write,
+};
+
+static const struct regmap_config smi330_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+	.val_format_endian = REGMAP_ENDIAN_LITTLE,
+};
+
+static int smi330_i2c_probe(struct i2c_client *i2c)
+{
+	struct device *dev = &i2c->dev;
+	struct smi330_i2c_priv *priv;
+	struct regmap *regmap;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->i2c = i2c;
+	regmap = devm_regmap_init(dev, &smi330_regmap_bus, priv,
+				  &smi330_regmap_config);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap),
+				     "Failed to initialize I2C Regmap\n");
+
+	return smi330_core_probe(dev, regmap);
+}
+
+static const struct i2c_device_id smi330_i2c_device_id[] = {
+	{ .name = "smi330" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, smi330_i2c_device_id);
+
+static const struct of_device_id smi330_of_match[] = {
+	{ .compatible = "bosch,smi330" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, smi330_of_match);
+
+static struct i2c_driver smi330_i2c_driver = {
+	.probe = smi330_i2c_probe,
+	.id_table = smi330_i2c_device_id,
+	.driver = {
+		.of_match_table = smi330_of_match,
+		.name = "smi330_i2c",
+	},
+};
+module_i2c_driver(smi330_i2c_driver);
+
+MODULE_AUTHOR("Stefan Gutmann <stefan.gutmann@de.bosch.com>");
+MODULE_AUTHOR("Roman Huber <roman.huber@de.bosch.com>");
+MODULE_AUTHOR("Filip Andrei <Andrei.Filip@ro.bosch.com>");
+MODULE_AUTHOR("Drimbarean Avram Andrei <Avram-Andrei.Drimbarean@ro.bosch.com>");
+MODULE_DESCRIPTION("Bosch SMI330 I2C driver");
+MODULE_LICENSE("Dual BSD/GPL");
+MODULE_IMPORT_NS("IIO_SMI330");
diff --git a/drivers/iio/imu/smi330/smi330_spi.c b/drivers/iio/imu/smi330/smi330_spi.c
new file mode 100644
index 00000000000..5b5aaaf0c5d
--- /dev/null
+++ b/drivers/iio/imu/smi330/smi330_spi.c
@@ -0,0 +1,91 @@
+// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
+/*
+ * Copyright (c) 2025 Robert Bosch GmbH.
+ */
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+
+#include "smi330.h"
+
+static int smi330_regmap_spi_read(void *context, const void *reg_buf,
+				  size_t reg_size, void *val_buf,
+				  size_t val_size)
+{
+	struct spi_device *spi = context;
+
+	/* Insert pad byte for reading */
+	u8 reg[] = { *(u8 *)reg_buf, 0 };
+
+	if (reg_size + 1 != ARRAY_SIZE(reg)) {
+		dev_err(&spi->dev, "Invalid register size %zu\n", reg_size);
+		return -EINVAL;
+	}
+
+	return spi_write_then_read(spi, reg, ARRAY_SIZE(reg), val_buf,
+				   val_size);
+}
+
+static int smi330_regmap_spi_write(void *context, const void *data,
+				   size_t count)
+{
+	struct spi_device *spi = context;
+
+	return spi_write(spi, data, count);
+}
+
+static const struct regmap_bus smi330_regmap_bus = {
+	.read = smi330_regmap_spi_read,
+	.write = smi330_regmap_spi_write,
+	.read_flag_mask = 0x80,
+};
+
+static const struct regmap_config smi330_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+	.val_format_endian = REGMAP_ENDIAN_LITTLE,
+};
+
+static int smi330_spi_probe(struct spi_device *spi)
+{
+	struct regmap *regmap;
+
+	regmap = devm_regmap_init(&spi->dev, &smi330_regmap_bus, &spi->dev,
+				  &smi330_regmap_config);
+	if (IS_ERR(regmap))
+		return dev_err_probe(&spi->dev, PTR_ERR(regmap),
+				     "Failed to initialize SPI Regmap\n");
+
+	return smi330_core_probe(&spi->dev, regmap);
+}
+
+static const struct spi_device_id smi330_spi_device_id[] = {
+	{ .name = "smi330" },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, smi330_spi_device_id);
+
+static const struct of_device_id smi330_of_match[] = {
+	{ .compatible = "bosch,smi330" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, smi330_of_match);
+
+static struct spi_driver smi330_spi_driver = {
+	.probe = smi330_spi_probe,
+	.id_table = smi330_spi_device_id,
+	.driver = {
+		.of_match_table = smi330_of_match,
+		.name = "smi330_spi",
+	},
+};
+module_spi_driver(smi330_spi_driver);
+
+MODULE_AUTHOR("Stefan Gutmann <stefan.gutmann@de.bosch.com>");
+MODULE_AUTHOR("Roman Huber <roman.huber@de.bosch.com>");
+MODULE_AUTHOR("Filip Andrei <Andrei.Filip@ro.bosch.com>");
+MODULE_AUTHOR("Drimbarean Avram Andrei <Avram-Andrei.Drimbarean@ro.bosch.com>");
+MODULE_DESCRIPTION("Bosch SMI330 SPI driver");
+MODULE_LICENSE("Dual BSD/GPL");
+MODULE_IMPORT_NS("IIO_SMI330");
-- 
2.34.1

Re: [PATCH v3 2/2] iio: imu: smi330: Add driver

[kernel test robot]

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on linus/master v6.16-rc4 next-20250704]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jianping-Shen-de-bosch-com/dt-bindings-iio-imu-smi330-Add-binding/20250703-234441
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20250703153823.806073-3-Jianping.Shen%40de.bosch.com
patch subject: [PATCH v3 2/2] iio: imu: smi330: Add driver
config: microblaze-randconfig-r073-20250704 (https://download.01.org/0day-ci/archive/20250704/202507042238.UDo16CzT-lkp@intel.com/config)
compiler: microblaze-linux-gcc (GCC) 8.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250704/202507042238.UDo16CzT-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507042238.UDo16CzT-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/iio/imu/smi330/smi330_i2c.c: In function 'smi330_regmap_i2c_read':
>> drivers/iio/imu/smi330/smi330_i2c.c:26:11: warning: unused variable 'retry' [-Wunused-variable]
     int ret, retry;
              ^~~~~


vim +/retry +26 drivers/iio/imu/smi330/smi330_i2c.c

    20	
    21	static int smi330_regmap_i2c_read(void *context, const void *reg_buf,
    22					  size_t reg_size, void *val_buf,
    23					  size_t val_size)
    24	{
    25		struct smi330_i2c_priv *priv = context;
  > 26		int ret, retry;
    27	
    28		/*
    29		 * SMI330 I2C read frame:
    30		 * <Slave address[6:0], RnW> <x, Register address[6:0]>
    31		 * <Slave address[6:0], RnW> <Dummy[7:0]> <Dummy[7:0]> <Data_0[7:0]> <Data_1[15:8]>...
    32		 *                                                     <Data_N[7:0]> <Data_N[15:8]>
    33		 * Remark: Slave address is not considered part of the frame in the following definitions
    34		 */
    35		struct i2c_msg msgs[] = {
    36			{
    37				.addr = priv->i2c->addr,
    38				.flags = priv->i2c->flags,
    39				.len = reg_size,
    40				.buf = (u8 *)reg_buf,
    41			},
    42			{
    43				.addr = priv->i2c->addr,
    44				.flags = priv->i2c->flags | I2C_M_RD,
    45				.len = SMI330_NUM_DUMMY_BYTES + val_size,
    46				.buf = priv->rx_buffer,
    47			},
    48		};
    49	
    50		ret = i2c_transfer(priv->i2c->adapter, msgs, ARRAY_SIZE(msgs));
    51		if (ret < 0)
    52			return ret;
    53	
    54		memcpy(val_buf, priv->rx_buffer + SMI330_NUM_DUMMY_BYTES, val_size);
    55	
    56		return 0;
    57	}
    58	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 2/2] iio: imu: smi330: Add driver

[Jonathan Cameron]

On Thu, 3 Jul 2025 17:38:23 +0200
<Jianping.Shen@de.bosch.com> wrote:

> From: Jianping Shen <Jianping.Shen@de.bosch.com>
> 
> Add the iio driver for bosch imu smi330. The smi330 is a combined
> three axis angular rate and three axis acceleration sensor.
> 
> Signed-off-by: Jianping Shen <Jianping.Shen@de.bosch.com>
Hi Jianping,

This is coming together nicely.   A few things inline.

Jonathan


> diff --git a/drivers/iio/imu/smi330/smi330.h b/drivers/iio/imu/smi330/smi330.h
> new file mode 100644
> index 00000000000..bd896e86977
> --- /dev/null
> +++ b/drivers/iio/imu/smi330/smi330.h
> @@ -0,0 +1,240 @@
> +/* SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0 */
> +/*
> + * Copyright (c) 2025 Robert Bosch GmbH.
> + */
> +#ifndef _SMI330_H
> +#define _SMI330_H
> +
> +#include <linux/bits.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/trigger.h>

Where possible use forwards declarations and keep the include
to where it is needed.  e.g.
struct iio_trigger;
should be enough for this one.

> +
> +#define SMI330_NO_ERROR_MASK (BIT(2) | BIT(0))
> +#define SMI330_ST_SUCCESS_MASK GENMASK(6, 0)
> +
> +#define SMI330_ALL_CHAN_MSK GENMASK(6, 0)
> +
> +#define SMI330_CHIP_ID 0x42
> +
> +#define SMI330_SPI_WR_MASK GENMASK(6, 0)
> +#define SMI330_SPI_RD_MASK BIT(7)
> +
> +#define SMI330_SOFT_RESET_DELAY 2000
> +
> +/* Register map */
> +#define SMI330_CHIP_ID_REG 0x00


I missed this until now, but you have done a good job of keeping all registers
accesses etc to the core driver, not the i2c and spi parts.  As such
why do we need all these defines in the header?  Push them down into the
C file (a the top) and keep only those things needed by both bus drivers
and the core driver in the header.

> +#define SMI330_ERR_REG 0x01
> +#define SMI330_STATUS_REG 0x02
> +#define SMI330_ACCEL_X_REG 0x03

> +
> +int smi330_core_probe(struct device *dev, struct regmap *regmap);
> +
> +#endif /* _SMI330_H */
> diff --git a/drivers/iio/imu/smi330/smi330_core.c b/drivers/iio/imu/smi330/smi330_core.c
> new file mode 100644
> index 00000000000..23e65c1ed64
> --- /dev/null
> +++ b/drivers/iio/imu/smi330/smi330_core.c

> +
> +static irqreturn_t smi330_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct smi330_data *data = iio_priv(indio_dev);
> +	int ret, chan;
> +	int i = 0;
> +
> +	ret = regmap_bulk_read(data->regmap, SMI330_ACCEL_X_REG, data->buf,
> +			       ARRAY_SIZE(smi330_channels));
> +	if (ret)
> +		goto out;
> +
> +	if (*indio_dev->active_scan_mask != SMI330_ALL_CHAN_MSK) {
> +		iio_for_each_active_channel(indio_dev, chan)
> +			data->buf[i++] = data->buf[chan];

If I follow this correctly you are reading all the channels and just copying
out the ones you want.  Just let the IIO core do that for you by setting
iio_dev->available_scan_masks = {  SMI330_ALL_CHAN_MSK, 0 }; and push the
whole buffer every time.

The handling the core code is reasonably sophisticated and will use bulk
copying where appropriate.

If there is a strong reason to not use that, add a comment here so we don't
have anyone 'fix' this code in future.

> +	}
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, data->buf, pf->timestamp);
> +
> +out:
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}

> +static int smi330_dev_init(struct smi330_data *data)
> +{
> +	int ret, chip_id, val, mode;
> +	struct device *dev = regmap_get_device(data->regmap);
> +
> +	ret = regmap_read(data->regmap, SMI330_CHIP_ID_REG, &chip_id);
> +	if (ret)
> +		return ret;
> +
> +	chip_id &= 0x00FF;

Perhaps GENMASK(7, 0) for that - perhaps with a define associated with
the CHIP_ID_REG define as this is a register field. When you've done
that apply FIELD_GET() so we don't need to know it's the lowest bits
in the register.

> +
> +	if (chip_id != SMI330_CHIP_ID)
> +		dev_info(dev, "Unknown chip id: 0x%04x\n", chip_id);
> +
> +	ret = regmap_read(data->regmap, SMI330_ERR_REG, &val);
> +	if (ret)
> +		return ret;
> +	if (FIELD_GET(SMI330_ERR_FATAL_MASK, val))
> +		return -ENODEV;
> +
> +	ret = regmap_read(data->regmap, SMI330_STATUS_REG, &val);
> +	if (ret)
> +		return ret;
> +	if (FIELD_GET(SMI330_STATUS_POR_MASK, val) == 0)
> +		return -ENODEV;
> +
> +	mode = FIELD_PREP(SMI330_CFG_MODE_MASK, SMI330_MODE_NORMAL);
> +
> +	ret = regmap_update_bits(data->regmap, SMI330_ACCEL_CFG_REG,
> +				 SMI330_CFG_MODE_MASK, mode);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_update_bits(data->regmap, SMI330_GYRO_CFG_REG,
> +				  SMI330_CFG_MODE_MASK, mode);
> +}


> diff --git a/drivers/iio/imu/smi330/smi330_i2c.c b/drivers/iio/imu/smi330/smi330_i2c.c
> new file mode 100644
> index 00000000000..76b88bbd7d2
> --- /dev/null
> +++ b/drivers/iio/imu/smi330/smi330_i2c.c
> @@ -0,0 +1,136 @@
> +// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
> +/*
> + * Copyright (c) 2025 Robert Bosch GmbH.
> + */
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
As for spi driver below, drop this and include
mod_devicetable.h instead.

> +#include <linux/regmap.h>
> +
> +#include "smi330.h"
> +
> +#define SMI330_NUM_DUMMY_BYTES 2
> +#define SMI330_I2C_MAX_RX_BUFFER_SIZE \
> +	(SMI330_NUM_DUMMY_BYTES + SMI330_SCAN_LEN * sizeof(s16))
> +
> +struct smi330_i2c_priv {
> +	struct i2c_client *i2c;
> +	u8 rx_buffer[SMI330_I2C_MAX_RX_BUFFER_SIZE];
> +};
> +
> +static int smi330_regmap_i2c_read(void *context, const void *reg_buf,
> +				  size_t reg_size, void *val_buf,
> +				  size_t val_size)
> +{
> +	struct smi330_i2c_priv *priv = context;
> +	int ret, retry;
> +
> +	/*
> +	 * SMI330 I2C read frame:
> +	 * <Slave address[6:0], RnW> <x, Register address[6:0]>
> +	 * <Slave address[6:0], RnW> <Dummy[7:0]> <Dummy[7:0]> <Data_0[7:0]> <Data_1[15:8]>...
> +	 *                                                     <Data_N[7:0]> <Data_N[15:8]>
> +	 * Remark: Slave address is not considered part of the frame in the following definitions
> +	 */
> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr = priv->i2c->addr,
> +			.flags = priv->i2c->flags,
> +			.len = reg_size,
> +			.buf = (u8 *)reg_buf,
> +		},
> +		{
> +			.addr = priv->i2c->addr,
> +			.flags = priv->i2c->flags | I2C_M_RD,
> +			.len = SMI330_NUM_DUMMY_BYTES + val_size,

It might be worth adding a sanity check in ehre that this is never more
than SMI330_I2C_MAX_RX_BUFFER_SIZE.
That is obviously true, but none the less a check may make it 'locally'
clear that we can rely on that.

> +			.buf = priv->rx_buffer,
> +		},
> +	};
> +
> +	ret = i2c_transfer(priv->i2c->adapter, msgs, ARRAY_SIZE(msgs));
> +	if (ret < 0)
> +		return ret;
> +
> +	memcpy(val_buf, priv->rx_buffer + SMI330_NUM_DUMMY_BYTES, val_size);
> +
> +	return 0;
> +}

> diff --git a/drivers/iio/imu/smi330/smi330_spi.c b/drivers/iio/imu/smi330/smi330_spi.c
> new file mode 100644
> index 00000000000..5b5aaaf0c5d
> --- /dev/null
> +++ b/drivers/iio/imu/smi330/smi330_spi.c
> @@ -0,0 +1,91 @@
> +// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
> +/*
> + * Copyright (c) 2025 Robert Bosch GmbH.
> + */
> +#include <linux/module.h>
> +#include <linux/of.h>

There aren't any calls that you need of.h for in here. If there had been
I'd probably be telling you to user property.h but its very simple so
no problem not including either.

What you probably want is the id tables which are in
linux/mod_devicetable.h which you should include here.

> +#include <linux/regmap.h>
> +#include <linux/spi/spi.h>
> +
> +#include "smi330.h"

AW: [PATCH v3 2/2] iio: imu: smi330: Add driver

[Shen Jianping (ME-SE/EAD2)]

Hi Jonathan,

"available_scan_masks" works not as expected.  We test it using kernel version v6.16. see the test result inline.

Best Regards 
Jianping 

>> +
>> +static irqreturn_t smi330_trigger_handler(int irq, void *p) {
>> +	struct iio_poll_func *pf = p;
>> +	struct iio_dev *indio_dev = pf->indio_dev;
>> +	struct smi330_data *data = iio_priv(indio_dev);
>> +	int ret, chan;
>> +	int i = 0;
>> +
>> +	ret = regmap_bulk_read(data->regmap, SMI330_ACCEL_X_REG, data-
>>buf,
>> +			       ARRAY_SIZE(smi330_channels));
>> +	if (ret)
>> +		goto out;
>> +
>> +	if (*indio_dev->active_scan_mask != SMI330_ALL_CHAN_MSK) {
>> +		iio_for_each_active_channel(indio_dev, chan)
>> +			data->buf[i++] = data->buf[chan];
>
>If I follow this correctly you are reading all the channels and just copying out the
>ones you want.  Just let the IIO core do that for you by setting iio_dev-
>>available_scan_masks = {  SMI330_ALL_CHAN_MSK, 0 }; and push the whole
>buffer every time.

For the most frequent use cases, we define available_scan_masks = { SMI330_ALL_CHAN_MSK, SMI330_ACC_XYZ_MSK, SMI330_GYRO_XYZ_MSK, 0 }; and push the whole buffer every time.
From the user space we just enable 3 channels gyro_x, gyro_y, and gyro_z. Then we enable buffer and expect that only the gyro values and timestamp in iio_buffer. Nevertheless, we have 3 accelerometer values and the timestamp in iio_buffer.
It seems that the iio core does not take care which channel is enabled,  just copy the first 3 values (acc x,y,z) into iio_buffer.  Our driver code still needs to take care and just copy the enabled channel value to buffer.

Another side effect after using available_scan_masks is that the active_scan_masks sometimes does not reflect current channel activation status.

Is some step missing to properly use available_scan_masks ?  How can a user find out from user space which channel combination is defined in available_scan_masks ?

>
>The handling the core code is reasonably sophisticated and will use bulk
>copying where appropriate.
>
>If there is a strong reason to not use that, add a comment here so we don't
>have anyone 'fix' this code in future.
>
>> +	}
>> +
>> +	iio_push_to_buffers_with_timestamp(indio_dev, data->buf,
>> +pf->timestamp);
>> +
>> +out:
>> +	iio_trigger_notify_done(indio_dev->trig);
>> +
>> +	return IRQ_HANDLED;
>> +}

Re: [PATCH v3 2/2] iio: imu: smi330: Add driver

[Jonathan Cameron]

On Wed, 9 Jul 2025 19:38:18 +0000
"Shen Jianping (ME-SE/EAD2)" <Jianping.Shen@de.bosch.com> wrote:

> Hi Jonathan,
> 
> "available_scan_masks" works not as expected.  We test it using kernel version v6.16. see the test result inline.
> 
> Best Regards 
> Jianping 
> 
> >> +
> >> +static irqreturn_t smi330_trigger_handler(int irq, void *p) {
> >> +	struct iio_poll_func *pf = p;
> >> +	struct iio_dev *indio_dev = pf->indio_dev;
> >> +	struct smi330_data *data = iio_priv(indio_dev);
> >> +	int ret, chan;
> >> +	int i = 0;
> >> +
> >> +	ret = regmap_bulk_read(data->regmap, SMI330_ACCEL_X_REG, data-
> >>buf,
> >> +			       ARRAY_SIZE(smi330_channels));
> >> +	if (ret)
> >> +		goto out;
> >> +
> >> +	if (*indio_dev->active_scan_mask != SMI330_ALL_CHAN_MSK) {
> >> +		iio_for_each_active_channel(indio_dev, chan)
> >> +			data->buf[i++] = data->buf[chan];  
> >
> >If I follow this correctly you are reading all the channels and just copying out the
> >ones you want.  Just let the IIO core do that for you by setting iio_dev-  
> >>available_scan_masks = {  SMI330_ALL_CHAN_MSK, 0 }; and push the whole  
> >buffer every time.  
> 
> For the most frequent use cases, we define available_scan_masks = { SMI330_ALL_CHAN_MSK, SMI330_ACC_XYZ_MSK, SMI330_GYRO_XYZ_MSK, 0 }; and push the whole buffer every time.
> From the user space we just enable 3 channels gyro_x, gyro_y, and gyro_z. Then we enable buffer and expect that only the gyro values and timestamp in iio_buffer. Nevertheless, we have 3 accelerometer values and the timestamp in iio_buffer.

> It seems that the iio core does not take care which channel is enabled,  just copy the first 3 values (acc x,y,z) into iio_buffer.  Our driver code still needs to take care and just copy the enabled channel value to buffer.

Look again at how it works.  If you provide ACC_XYZ_MSK, then your driver has to handle it.
available_scan_masks is saying what your driver supports. The driver can check active_scan_mask
to find out what is enabled.  So right option here is only
{ SMI330_ALL_CHAN_MSK, 0, }  In that case the driver never needs to check as there is only
one option.

Then if any subset of channels is enabled the IIO core copy out just the data that
is relevant.


> 
> Another side effect after using available_scan_masks is that the active_scan_masks sometimes does not reflect current channel activation status.
> 
> Is some step missing to properly use available_scan_masks ?  How can a user find out from user space which channel combination is defined in available_scan_masks ?

Why would userspace want to?  Userspace requested a subset of channels
and it gets that subset.  So it if asks for the channels that make up
SMI330_ACC_XYZ_MSK, if available_scan_mask == { SMI330_ALL_CHAN_MSK, 0 } then
the IIO core handling selects SMI330_ALL_CHAN_MSK (smallest available mask that
is superset of what we asked for) and sets active_scan_mask to that.  The driver
follows what active_scan_mask specifies and passes all channel data via
the iio_push_to_buffers*() call. The demux in the IIO core than takes that
'scan' and repacks it so that userspace receives just the data it asked for
formatting exactly as the driver would have done it if you had handled
each channels separately in the driver.

So the aim is that userspace never knows anything about this.  Just set
what channels you want and get that data. 

Jonathan


> 
> >
> >The handling the core code is reasonably sophisticated and will use bulk
> >copying where appropriate.
> >
> >If there is a strong reason to not use that, add a comment here so we don't
> >have anyone 'fix' this code in future.
> >  
> >> +	}
> >> +
> >> +	iio_push_to_buffers_with_timestamp(indio_dev, data->buf,
> >> +pf->timestamp);
> >> +
> >> +out:
> >> +	iio_trigger_notify_done(indio_dev->trig);
> >> +
> >> +	return IRQ_HANDLED;
> >> +}  
> 
> 

AW: [PATCH v3 2/2] iio: imu: smi330: Add driver

[Shen Jianping (ME-SE/EAD2)]

Hi Jonathan,

as you suggested we just set available_scan_masks = {  SMI330_ALL_CHAN_MSK, 0 }; and push the whole buffer every time.
We enable a subset (3 channels) from user space. This time the channel data is correct in iio buffer, nevertheless invalid timestamp.
See test result inline

Best Regards
Jianping

>>
>> >> +
>> >> +static irqreturn_t smi330_trigger_handler(int irq, void *p) {
>> >> +	struct iio_poll_func *pf = p;
>> >> +	struct iio_dev *indio_dev = pf->indio_dev;
>> >> +	struct smi330_data *data = iio_priv(indio_dev);
>> >> +	int ret, chan;
>> >> +	int i = 0;
>> >> +
>> >> +	ret = regmap_bulk_read(data->regmap, SMI330_ACCEL_X_REG, data-
>> >>buf,
>> >> +			       ARRAY_SIZE(smi330_channels));
>> >> +	if (ret)
>> >> +		goto out;
>> >> +
>> >> +	if (*indio_dev->active_scan_mask != SMI330_ALL_CHAN_MSK) {
>> >> +		iio_for_each_active_channel(indio_dev, chan)
>> >> +			data->buf[i++] = data->buf[chan];
>> >
>> >If I follow this correctly you are reading all the channels and just
>> >copying out the ones you want.  Just let the IIO core do that for you
>> >by setting iio_dev-
>> >>available_scan_masks = {  SMI330_ALL_CHAN_MSK, 0 }; and push the
>> >>whole
>> >buffer every time.
>>
>> For the most frequent use cases, we define available_scan_masks = {
>SMI330_ALL_CHAN_MSK, SMI330_ACC_XYZ_MSK, SMI330_GYRO_XYZ_MSK,
>0 }; and push the whole buffer every time.
>> From the user space we just enable 3 channels gyro_x, gyro_y, and gyro_z.
>Then we enable buffer and expect that only the gyro values and timestamp in
>iio_buffer. Nevertheless, we have 3 accelerometer values and the timestamp in
>iio_buffer.
>
>> It seems that the iio core does not take care which channel is enabled,  just
>copy the first 3 values (acc x,y,z) into iio_buffer.  Our driver code still needs to
>take care and just copy the enabled channel value to buffer.
>
>Look again at how it works.  If you provide ACC_XYZ_MSK, then your driver has
>to handle it.
>available_scan_masks is saying what your driver supports. The driver can check
>active_scan_mask to find out what is enabled.  So right option here is only {
>SMI330_ALL_CHAN_MSK, 0, }  In that case the driver never needs to check as
>there is only one option.
>
>Then if any subset of channels is enabled the IIO core copy out just the data
>that is relevant.
>
>
>>
>> Another side effect after using available_scan_masks is that the
>active_scan_masks sometimes does not reflect current channel activation
>status.
>>
>> Is some step missing to properly use available_scan_masks ?  How can a user
>find out from user space which channel combination is defined in
>available_scan_masks ?
>
>Why would userspace want to?  Userspace requested a subset of channels and
>it gets that subset.  So it if asks for the channels that make up
>SMI330_ACC_XYZ_MSK, if available_scan_mask == { SMI330_ALL_CHAN_MSK,
>0 } then the IIO core handling selects SMI330_ALL_CHAN_MSK (smallest
>available mask that is superset of what we asked for) and sets
>active_scan_mask to that.  The driver follows what active_scan_mask specifies
>and passes all channel data via the iio_push_to_buffers*() call. The demux in
>the IIO core than takes that 'scan' and repacks it so that userspace receives just
>the data it asked for formatting exactly as the driver would have done it if you
>had handled each channels separately in the driver.
>

Set available_scan_masks = {  SMI330_ALL_CHAN_MSK, 0 } and push the whole buffer. iio_push_to_buffers_with_timestamp (indio_dev, data->buf, pf->timestamp);
We enable the accX, accY, and accZ from userspace. And expect 3 acc values and the timestamp in iio buffer.

Raw iio buffer data:
00000000: 3c00 d6ff 7510 0000 6100 f3ff 0000 0000  <...u...a.......
00000010: 3f00 d2ff 8910 0000 0300 f6ff 0000 0000  ?...............
00000020: 4900 dcff 7a10 0000 caff 0100 0000 0000  I...z...........
00000030: 4c00 d9ff 7910 0000 2f00 f8ff 0000 0000  L...y.../.......
00000040: 4b00 d9ff 8410 0000 1f00 0800 0000 0000  K...............
00000050: 4700 daff 7f10 0000 3b00 eeff 0000 0000  G.......;.......
00000060: 3f00 d8ff 8410 0000 0c00 0900 0000 0000  ?...............
00000070: 4600 d9ff 8010 0000 0e00 0800 0000 0000  F...............
00000080: 4700 d7ff 7d10 0000 3400 feff 0000 0000  G...}...4.......
00000090: 4b00 d4ff 8010 0000 3e00 1200 0000 0000  K.......>.......
000000a0: 4600 d6ff 8d10 0000 4300 0000 0000 0000  F.......C.......
000000b0: 4900 d6ff 7710 0000 2500 f0ff 0000 0000  I...w...%.......

Converted value
0.015625 -0.009277 1.024411 589929
0.015869 -0.009521 1.040769 4294901719
0.020508 -0.008301 1.025632 458712
0.018799 -0.006836 1.032956 851960
0.019287 -0.009521 1.033201 4294836275
0.015625 -0.010498 1.031003 4293328982
0.015137 -0.010498 1.031980 4293853176
0.015869 -0.009521 1.031492 4293722141
0.018555 -0.011475 1.033445 4294311886

The 3 acc values is correct in buffer.  Nevertheless, invalid timestamp. The timestamp is actually the value of the gyroscope, which directly followed by acc values.
If we enable the gyroX, gyroY, and gyroZ from userspace, then all the data is correct. Since the gyro values are the last 3 values and flowed by timestamp.

Conclusion: Setting available_scan_masks = {  SMI330_ALL_CHAN_MSK, 0 }, the iio core is able to correct handle the enabled channel data, but not the timestamp.
The working solution for now is that our driver takes care and just copys the enabled channel value to buffer without using available_scan_masks.

>So the aim is that userspace never knows anything about this.  Just set what
>channels you want and get that data.
>
>Jonathan
>
>
>>
>> >
>> >The handling the core code is reasonably sophisticated and will use
>> >bulk copying where appropriate.
>> >
>> >If there is a strong reason to not use that, add a comment here so we
>> >don't have anyone 'fix' this code in future.
>> >
>> >> +	}
>> >> +
>> >> +	iio_push_to_buffers_with_timestamp(indio_dev, data->buf,
>> >> +pf->timestamp);
>> >> +
>> >> +out:
>> >> +	iio_trigger_notify_done(indio_dev->trig);
>> >> +
>> >> +	return IRQ_HANDLED;
>> >> +}
>>

Re: [PATCH v3 2/2] iio: imu: smi330: Add driver

[Jonathan Cameron]

On Tue, 15 Jul 2025 18:35:48 +0000
"Shen Jianping (ME-SE/EAD2)" <Jianping.Shen@de.bosch.com> wrote:

> Hi Jonathan,
> 
> as you suggested we just set available_scan_masks = {  SMI330_ALL_CHAN_MSK, 0 }; and push the whole buffer every time.
> We enable a subset (3 channels) from user space. This time the channel data is correct in iio buffer, nevertheless invalid timestamp.
> See test result inline
> 
> Best Regards
> Jianping
> 
> >>  
> >> >> +
> >> >> +static irqreturn_t smi330_trigger_handler(int irq, void *p) {
> >> >> +	struct iio_poll_func *pf = p;
> >> >> +	struct iio_dev *indio_dev = pf->indio_dev;
> >> >> +	struct smi330_data *data = iio_priv(indio_dev);
> >> >> +	int ret, chan;
> >> >> +	int i = 0;
> >> >> +
> >> >> +	ret = regmap_bulk_read(data->regmap, SMI330_ACCEL_X_REG, data-
> >> >>buf,
> >> >> +			       ARRAY_SIZE(smi330_channels));
> >> >> +	if (ret)
> >> >> +		goto out;
> >> >> +
> >> >> +	if (*indio_dev->active_scan_mask != SMI330_ALL_CHAN_MSK) {
> >> >> +		iio_for_each_active_channel(indio_dev, chan)
> >> >> +			data->buf[i++] = data->buf[chan];  
> >> >
> >> >If I follow this correctly you are reading all the channels and just
> >> >copying out the ones you want.  Just let the IIO core do that for you
> >> >by setting iio_dev-  
> >> >>available_scan_masks = {  SMI330_ALL_CHAN_MSK, 0 }; and push the
> >> >>whole  
> >> >buffer every time.  
> >>
> >> For the most frequent use cases, we define available_scan_masks = {  
> >SMI330_ALL_CHAN_MSK, SMI330_ACC_XYZ_MSK, SMI330_GYRO_XYZ_MSK,
> >0 }; and push the whole buffer every time.  
> >> From the user space we just enable 3 channels gyro_x, gyro_y, and gyro_z.  
> >Then we enable buffer and expect that only the gyro values and timestamp in
> >iio_buffer. Nevertheless, we have 3 accelerometer values and the timestamp in
> >iio_buffer.
> >  
> >> It seems that the iio core does not take care which channel is enabled,  just  
> >copy the first 3 values (acc x,y,z) into iio_buffer.  Our driver code still needs to
> >take care and just copy the enabled channel value to buffer.
> >
> >Look again at how it works.  If you provide ACC_XYZ_MSK, then your driver has
> >to handle it.
> >available_scan_masks is saying what your driver supports. The driver can check
> >active_scan_mask to find out what is enabled.  So right option here is only {
> >SMI330_ALL_CHAN_MSK, 0, }  In that case the driver never needs to check as
> >there is only one option.
> >
> >Then if any subset of channels is enabled the IIO core copy out just the data
> >that is relevant.
> >
> >  
> >>
> >> Another side effect after using available_scan_masks is that the  
> >active_scan_masks sometimes does not reflect current channel activation
> >status.  
> >>
> >> Is some step missing to properly use available_scan_masks ?  How can a user  
> >find out from user space which channel combination is defined in
> >available_scan_masks ?
> >
> >Why would userspace want to?  Userspace requested a subset of channels and
> >it gets that subset.  So it if asks for the channels that make up
> >SMI330_ACC_XYZ_MSK, if available_scan_mask == { SMI330_ALL_CHAN_MSK,
> >0 } then the IIO core handling selects SMI330_ALL_CHAN_MSK (smallest
> >available mask that is superset of what we asked for) and sets
> >active_scan_mask to that.  The driver follows what active_scan_mask specifies
> >and passes all channel data via the iio_push_to_buffers*() call. The demux in
> >the IIO core than takes that 'scan' and repacks it so that userspace receives just
> >the data it asked for formatting exactly as the driver would have done it if you
> >had handled each channels separately in the driver.
> >  
> 
> Set available_scan_masks = {  SMI330_ALL_CHAN_MSK, 0 } and push the whole buffer. iio_push_to_buffers_with_timestamp (indio_dev, data->buf, pf->timestamp);
> We enable the accX, accY, and accZ from userspace. And expect 3 acc values and the timestamp in iio buffer.
> 
> Raw iio buffer data:
> 00000000: 3c00 d6ff 7510 0000 6100 f3ff 0000 0000  <...u...a.......
            ACCX ACCY ACCZ PAD_ TIMESTAMP__________
				4093587712
> 00000010: 3f00 d2ff 8910 0000 0300 f6ff 0000 0000  ?...............
				4143907584
> 00000020: 4900 dcff 7a10 0000 caff 0100 0000 0000  I...z...........
				So this one looks bad.

> 00000030: 4c00 d9ff 7910 0000 2f00 f8ff 0000 0000  L...y.../.......
				4177473280

> 00000040: 4b00 d9ff 8410 0000 1f00 0800 0000 0000  K...............
				also bad.
> 00000050: 4700 daff 7f10 0000 3b00 eeff 0000 0000  G.......;.......
> 00000060: 3f00 d8ff 8410 0000 0c00 0900 0000 0000  ?...............
> 00000070: 4600 d9ff 8010 0000 0e00 0800 0000 0000  F...............
> 00000080: 4700 d7ff 7d10 0000 3400 feff 0000 0000  G...}...4.......
> 00000090: 4b00 d4ff 8010 0000 3e00 1200 0000 0000  K.......>.......
> 000000a0: 4600 d6ff 8d10 0000 4300 0000 0000 0000  F.......C.......
> 000000b0: 4900 d6ff 7710 0000 2500 f0ff 0000 0000  I...w...%.......
> 
> Converted value
I guess this is different data as doesn't seem to line up with the above?

> 0.015625 -0.009277 1.024411 589929
> 0.015869 -0.009521 1.040769 4294901719
> 0.020508 -0.008301 1.025632 458712
> 0.018799 -0.006836 1.032956 851960
> 0.019287 -0.009521 1.033201 4294836275
> 0.015625 -0.010498 1.031003 4293328982
> 0.015137 -0.010498 1.031980 4293853176
> 0.015869 -0.009521 1.031492 4293722141
> 0.018555 -0.011475 1.033445 4294311886
> 
> The 3 acc values is correct in buffer.  Nevertheless, invalid timestamp. The timestamp is actually the value of the gyroscope, which directly followed by acc values.
> If we enable the gyroX, gyroY, and gyroZ from userspace, then all the data is correct. Since the gyro values are the last 3 values and flowed by timestamp.

Ok. That's odd and we should debug that.  This code is used in a
lot of drivers so if it is not working correctly we need to figure
out why asap and fix it.

However, I'm not seeing what looks to be gyro data in bytes 8-15 etc
It isn't the stable sequence we'd expect for a timestamp though
some specific values might be plausible.

Looking again at the code, the IIO_DECLARE_BUFFER_WITH_TS()
is the wrong size.  That should not include channel space for
the timestamp. That should make it too big though which shouldn't be a problem.
Also wrong type - should be using __le16 not s16 for the buffer elements
given your channel declarations.

Please could you add a print to your code alongside the iio_push_buffer_with_timestamp()
to verify that the value in the pf->timestamp is reasonable looking for
a timestamp.

For reference this is the code that handles the timestamp entry creation
in the demux tables.
https://elixir.bootlin.com/linux/v6.15.1/source/drivers/iio/industrialio-buffer.c#L1086

Jonathan


> 
> Conclusion: Setting available_scan_masks = {  SMI330_ALL_CHAN_MSK, 0 }, the iio core is able to correct handle the enabled channel data, but not the timestamp.
> The working solution for now is that our driver takes care and just copys the enabled channel value to buffer without using available_scan_masks.
> 
> >So the aim is that userspace never knows anything about this.  Just set what
> >channels you want and get that data.
> >
> >Jonathan
> >
> >  
> >>  
> >> >
> >> >The handling the core code is reasonably sophisticated and will use
> >> >bulk copying where appropriate.
> >> >
> >> >If there is a strong reason to not use that, add a comment here so we
> >> >don't have anyone 'fix' this code in future.
> >> >  
> >> >> +	}
> >> >> +
> >> >> +	iio_push_to_buffers_with_timestamp(indio_dev, data->buf,
> >> >> +pf->timestamp);
> >> >> +
> >> >> +out:
> >> >> +	iio_trigger_notify_done(indio_dev->trig);
> >> >> +
> >> >> +	return IRQ_HANDLED;
> >> >> +}  
> >>  
> 

AW: [PATCH v3 2/2] iio: imu: smi330: Add driver

[Shen Jianping (ME-SE/EAD2)]

Hi Jonathan,

we find out the reason why the timestamp is invalid in the iio buffer.

https://elixir.bootlin.com/linux/v6.15.1/source/drivers/iio/industrialio-buffer.c#L1093

In "iio_buffer_update_demux" to copy the timestamp, the address calculation is the root causes.

1083  in_loc += length;
....
1093  in_loc = roundup(in_loc, length);

When finish to copy the channel data, in_loc is just incremented and used as address of timestamp. This is correct only when the channel direct before timestamp is enabled.

If there is a gap between the last enabled channel and timestamp, then iio core will copy the wrong data.

We have a fix to this issue,

1093 in_loc = (indio_dev->scan_bytes / sizeof(int64_t) - 1) * length;

just not sure, if there will be any side-effects with this fix.

Are you going to fix this finding, or shall we create a new patch for that?

Best regards
Jianping Shen


>>
>> >>
>> >> >> +
>> >> >> +static irqreturn_t smi330_trigger_handler(int irq, void *p) {
>> >> >> +      struct iio_poll_func *pf = p;
>> >> >> +      struct iio_dev *indio_dev = pf->indio_dev;
>> >> >> +      struct smi330_data *data = iio_priv(indio_dev);
>> >> >> +      int ret, chan;
>> >> >> +      int i = 0;
>> >> >> +
>> >> >> +      ret = regmap_bulk_read(data->regmap,
>SMI330_ACCEL_X_REG, data-
>> >> >>buf,
>> >> >> +                             ARRAY_SIZE(smi330_channels));
>> >> >> +      if (ret)
>> >> >> +              goto out;
>> >> >> +
>> >> >> +      if (*indio_dev->active_scan_mask != SMI330_ALL_CHAN_MSK)
>{
>> >> >> +              iio_for_each_active_channel(indio_dev, chan)
>> >> >> +                      data->buf[i++] = data->buf[chan];
>> >> >
>> >> >If I follow this correctly you are reading all the channels and
>> >> >just copying out the ones you want.  Just let the IIO core do that
>> >> >for you by setting iio_dev-
>> >> >>available_scan_masks = {  SMI330_ALL_CHAN_MSK, 0 }; and push the
>> >> >>whole
>> >> >buffer every time.
>> >>
>> >> For the most frequent use cases, we define available_scan_masks = {
>> >SMI330_ALL_CHAN_MSK, SMI330_ACC_XYZ_MSK,
>SMI330_GYRO_XYZ_MSK,
>> >0 }; and push the whole buffer every time.
>> >> From the user space we just enable 3 channels gyro_x, gyro_y, and gyro_z.
>> >Then we enable buffer and expect that only the gyro values and
>> >timestamp in iio_buffer. Nevertheless, we have 3 accelerometer values
>> >and the timestamp in iio_buffer.
>> >
>> >> It seems that the iio core does not take care which channel is
>> >> enabled,  just
>> >copy the first 3 values (acc x,y,z) into iio_buffer.  Our driver code
>> >still needs to take care and just copy the enabled channel value to buffer.
>> >
>> >Look again at how it works.  If you provide ACC_XYZ_MSK, then your
>> >driver has to handle it.
>> >available_scan_masks is saying what your driver supports. The driver
>> >can check active_scan_mask to find out what is enabled.  So right
>> >option here is only { SMI330_ALL_CHAN_MSK, 0, }  In that case the
>> >driver never needs to check as there is only one option.
>> >
>> >Then if any subset of channels is enabled the IIO core copy out just
>> >the data that is relevant.
>> >
>> >
>> >>
>> >> Another side effect after using available_scan_masks is that the
>> >active_scan_masks sometimes does not reflect current channel
>> >activation status.
>> >>
>> >> Is some step missing to properly use available_scan_masks ?  How
>> >> can a user
>> >find out from user space which channel combination is defined in
>> >available_scan_masks ?
>> >
>> >Why would userspace want to?  Userspace requested a subset of
>> >channels and it gets that subset.  So it if asks for the channels
>> >that make up SMI330_ACC_XYZ_MSK, if available_scan_mask == {
>> >SMI330_ALL_CHAN_MSK,
>> >0 } then the IIO core handling selects SMI330_ALL_CHAN_MSK (smallest
>> >available mask that is superset of what we asked for) and sets
>> >active_scan_mask to that.  The driver follows what active_scan_mask
>> >specifies and passes all channel data via the iio_push_to_buffers*()
>> >call. The demux in the IIO core than takes that 'scan' and repacks it
>> >so that userspace receives just the data it asked for formatting
>> >exactly as the driver would have done it if you had handled each channels
>separately in the driver.
>> >
>>
>> Set available_scan_masks = {  SMI330_ALL_CHAN_MSK, 0 } and push the
>> whole buffer. iio_push_to_buffers_with_timestamp (indio_dev, data->buf, pf-
>>timestamp); We enable the accX, accY, and accZ from userspace. And expect 3
>acc values and the timestamp in iio buffer.
>>
>> Raw iio buffer data:
>> 00000000: 3c00 d6ff 7510 0000 6100 f3ff 0000 0000  <...u...a.......
>            ACCX ACCY ACCZ PAD_ TIMESTAMP__________
>                               4093587712
>> 00000010: 3f00 d2ff 8910 0000 0300 f6ff 0000 0000  ?...............
>                               4143907584
>> 00000020: 4900 dcff 7a10 0000 caff 0100 0000 0000  I...z...........
>                               So this one looks bad.
>
>> 00000030: 4c00 d9ff 7910 0000 2f00 f8ff 0000 0000  L...y.../.......
>                               4177473280
>
>> 00000040: 4b00 d9ff 8410 0000 1f00 0800 0000 0000  K...............
>                               also bad.
>> 00000050: 4700 daff 7f10 0000 3b00 eeff 0000 0000  G.......;.......
>> 00000060: 3f00 d8ff 8410 0000 0c00 0900 0000 0000  ?...............
>> 00000070: 4600 d9ff 8010 0000 0e00 0800 0000 0000  F...............
>> 00000080: 4700 d7ff 7d10 0000 3400 feff 0000 0000  G...}...4.......
>> 00000090: 4b00 d4ff 8010 0000 3e00 1200 0000 0000  K.......>.......
>> 000000a0: 4600 d6ff 8d10 0000 4300 0000 0000 0000  F.......C.......
>> 000000b0: 4900 d6ff 7710 0000 2500 f0ff 0000 0000  I...w...%.......
>>
>> Converted value
>I guess this is different data as doesn't seem to line up with the above?
>
>> 0.015625 -0.009277 1.024411 589929
>> 0.015869 -0.009521 1.040769 4294901719
>> 0.020508 -0.008301 1.025632 458712
>> 0.018799 -0.006836 1.032956 851960
>> 0.019287 -0.009521 1.033201 4294836275
>> 0.015625 -0.010498 1.031003 4293328982
>> 0.015137 -0.010498 1.031980 4293853176
>> 0.015869 -0.009521 1.031492 4293722141
>> 0.018555 -0.011475 1.033445 4294311886
>>
>> The 3 acc values is correct in buffer.  Nevertheless, invalid timestamp. The
>timestamp is actually the value of the gyroscope, which directly followed by acc
>values.
>> If we enable the gyroX, gyroY, and gyroZ from userspace, then all the data is
>correct. Since the gyro values are the last 3 values and flowed by timestamp.
>
>Ok. That's odd and we should debug that.  This code is used in a lot of drivers
>so if it is not working correctly we need to figure out why asap and fix it.
>




>However, I'm not seeing what looks to be gyro data in bytes 8-15 etc It isn't the
>stable sequence we'd expect for a timestamp though some specific values
>might be plausible.
>
>Looking again at the code, the IIO_DECLARE_BUFFER_WITH_TS() is the wrong
>size.  That should not include channel space for the timestamp. That should
>make it too big though which shouldn't be a problem.
>Also wrong type - should be using __le16 not s16 for the buffer elements given
>your channel declarations.
>
>Please could you add a print to your code alongside the
>iio_push_buffer_with_timestamp() to verify that the value in the pf-
>>timestamp is reasonable looking for a timestamp.
>
>For reference this is the code that handles the timestamp entry creation in the
>demux tables.
>https://elixir.b/
>ootlin.com%2Flinux%2Fv6.15.1%2Fsource%2Fdrivers%2Fiio%2Findustrialio-
>buffer.c%23L1086&data=05%7C02%7CJianping.Shen%40de.bosch.com%7Cf0
>9eaf03f8e44dd1e6fe08ddc53ae596%7C0ae51e1907c84e4bbb6d648ee5841
>0f4%7C0%7C0%7C638883578931715207%7CUnknown%7CTWFpbGZsb3d8
>eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIj
>oiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=s53tTw6o%2F2guA
>iH3J9jBRd0%2Bj6UmcmgyhtBCuKK1HE0%3D&reserved=0
>
>Jonathan
>
>
>>
>> Conclusion: Setting available_scan_masks = {  SMI330_ALL_CHAN_MSK, 0 },
>the iio core is able to correct handle the enabled channel data, but not the
>timestamp.
>> The working solution for now is that our driver takes care and just copys the
>enabled channel value to buffer without using available_scan_masks.
>>
>> >So the aim is that userspace never knows anything about this.  Just
>> >set what channels you want and get that data.
>> >
>> >Jonathan
>> >
>> >
>> >>
>> >> >
>> >> >The handling the core code is reasonably sophisticated and will
>> >> >use bulk copying where appropriate.
>> >> >
>> >> >If there is a strong reason to not use that, add a comment here so
>> >> >we don't have anyone 'fix' this code in future.
>> >> >
>> >> >> +      }
>> >> >> +
>> >> >> +      iio_push_to_buffers_with_timestamp(indio_dev, data->buf,
>> >> >> +pf->timestamp);
>> >> >> +
>> >> >> +out:
>> >> >> +      iio_trigger_notify_done(indio_dev->trig);
>> >> >> +
>> >> >> +      return IRQ_HANDLED;
>> >> >> +}
>> >>
>>

Re: [PATCH v3 2/2] iio: imu: smi330: Add driver

[Jonathan Cameron]

On Wed, 23 Jul 2025 09:46:47 +0000
"Shen Jianping (ME-SE/EAD2)" <Jianping.Shen@de.bosch.com> wrote:

> Hi Jonathan,
> 
> we find out the reason why the timestamp is invalid in the iio buffer.
> 
> https://elixir.bootlin.com/linux/v6.15.1/source/drivers/iio/industrialio-buffer.c#L1093
> 
> In "iio_buffer_update_demux" to copy the timestamp, the address calculation is the root causes.
> 
> 1083  in_loc += length;
> ....
> 1093  in_loc = roundup(in_loc, length);
> 
> When finish to copy the channel data, in_loc is just incremented and used as address of timestamp. This is correct only when the channel direct before timestamp is enabled.
> 
> If there is a gap between the last enabled channel and timestamp, then iio core will copy the wrong data.
> 
> We have a fix to this issue,
> 
> 1093 in_loc = (indio_dev->scan_bytes / sizeof(int64_t) - 1) * length;

That looks correct, but I'm not seeing why the roundup above doesn't land us
in the same place.  I'm not that keen on handling the timestamp even more differently
to other channels.


If we imagine an active scan with
2-byte chanel we want, 2 2-byte channels we don't a 2-byte gap and an 8-byte timestamp
struct scan {
	u16 wanted;
	u16 notwanted[2];
	... gap...
	aligned_s64 timestamp;
	


Cutting down to the parts that change in_loc only.

	for_each_set_bit(out_ind, buffer->scan_mask, masklength) {
		in_ind = find_next_bit(indio_dev->active_scan_mask,
				       masklength, in_ind + 1);
		while (in_ind != out_ind) {
... length is the length of current channel
.. We never enter here in the example.
			/* Make sure we are aligned */
			in_loc = roundup(in_loc, length) + length;

			in_ind = find_next_bit(indio_dev->active_scan_mask,
					       masklength, in_ind + 1);
		}

... length is the length of the current channel.  Get here on first hit.

		in_loc = roundup(in_loc, length);

		in_loc += length;
.. in loc = 2
	}
	/* Relies on scan_timestamp being last */
	if (buffer->scan_timestamp) {

... length is 8 ...

		in_loc = roundup(in_loc, length);
.. I think in_lock = 8?
		ret = iio_buffer_add_demux(buffer, &p, in_loc, out_loc, length);

	}

Perhaps you can talk through the example that is failing?

> 
> just not sure, if there will be any side-effects with this fix.
> 
> Are you going to fix this finding, or shall we create a new patch for that?

Fine to send the proposed fix but first we need to step through why the
current code isn't working.


Thanks,

Jonathan

> 
> Best regards
> Jianping Shen
> 
> 
> >>  
> >> >>  
> >> >> >> +
> >> >> >> +static irqreturn_t smi330_trigger_handler(int irq, void *p) {
> >> >> >> +      struct iio_poll_func *pf = p;
> >> >> >> +      struct iio_dev *indio_dev = pf->indio_dev;
> >> >> >> +      struct smi330_data *data = iio_priv(indio_dev);
> >> >> >> +      int ret, chan;
> >> >> >> +      int i = 0;
> >> >> >> +
> >> >> >> +      ret = regmap_bulk_read(data->regmap,  
> >SMI330_ACCEL_X_REG, data-  
> >> >> >>buf,
> >> >> >> +                             ARRAY_SIZE(smi330_channels));
> >> >> >> +      if (ret)
> >> >> >> +              goto out;
> >> >> >> +
> >> >> >> +      if (*indio_dev->active_scan_mask != SMI330_ALL_CHAN_MSK)  
> >{  
> >> >> >> +              iio_for_each_active_channel(indio_dev, chan)
> >> >> >> +                      data->buf[i++] = data->buf[chan];  
> >> >> >
> >> >> >If I follow this correctly you are reading all the channels and
> >> >> >just copying out the ones you want.  Just let the IIO core do that
> >> >> >for you by setting iio_dev-  
> >> >> >>available_scan_masks = {  SMI330_ALL_CHAN_MSK, 0 }; and push the
> >> >> >>whole  
> >> >> >buffer every time.  
> >> >>
> >> >> For the most frequent use cases, we define available_scan_masks = {  
> >> >SMI330_ALL_CHAN_MSK, SMI330_ACC_XYZ_MSK,  
> >SMI330_GYRO_XYZ_MSK,  
> >> >0 }; and push the whole buffer every time.  
> >> >> From the user space we just enable 3 channels gyro_x, gyro_y, and gyro_z.  
> >> >Then we enable buffer and expect that only the gyro values and
> >> >timestamp in iio_buffer. Nevertheless, we have 3 accelerometer values
> >> >and the timestamp in iio_buffer.
> >> >  
> >> >> It seems that the iio core does not take care which channel is
> >> >> enabled,  just  
> >> >copy the first 3 values (acc x,y,z) into iio_buffer.  Our driver code
> >> >still needs to take care and just copy the enabled channel value to buffer.
> >> >
> >> >Look again at how it works.  If you provide ACC_XYZ_MSK, then your
> >> >driver has to handle it.
> >> >available_scan_masks is saying what your driver supports. The driver
> >> >can check active_scan_mask to find out what is enabled.  So right
> >> >option here is only { SMI330_ALL_CHAN_MSK, 0, }  In that case the
> >> >driver never needs to check as there is only one option.
> >> >
> >> >Then if any subset of channels is enabled the IIO core copy out just
> >> >the data that is relevant.
> >> >
> >> >  
> >> >>
> >> >> Another side effect after using available_scan_masks is that the  
> >> >active_scan_masks sometimes does not reflect current channel
> >> >activation status.  
> >> >>
> >> >> Is some step missing to properly use available_scan_masks ?  How
> >> >> can a user  
> >> >find out from user space which channel combination is defined in
> >> >available_scan_masks ?
> >> >
> >> >Why would userspace want to?  Userspace requested a subset of
> >> >channels and it gets that subset.  So it if asks for the channels
> >> >that make up SMI330_ACC_XYZ_MSK, if available_scan_mask == {
> >> >SMI330_ALL_CHAN_MSK,
> >> >0 } then the IIO core handling selects SMI330_ALL_CHAN_MSK (smallest
> >> >available mask that is superset of what we asked for) and sets
> >> >active_scan_mask to that.  The driver follows what active_scan_mask
> >> >specifies and passes all channel data via the iio_push_to_buffers*()
> >> >call. The demux in the IIO core than takes that 'scan' and repacks it
> >> >so that userspace receives just the data it asked for formatting
> >> >exactly as the driver would have done it if you had handled each channels  
> >separately in the driver.  
> >> >  
> >>
> >> Set available_scan_masks = {  SMI330_ALL_CHAN_MSK, 0 } and push the
> >> whole buffer. iio_push_to_buffers_with_timestamp (indio_dev, data->buf, pf-
> >>timestamp); We enable the accX, accY, and accZ from userspace. And expect 3  
> >acc values and the timestamp in iio buffer.  
> >>
> >> Raw iio buffer data:
> >> 00000000: 3c00 d6ff 7510 0000 6100 f3ff 0000 0000  <...u...a.......  
> >            ACCX ACCY ACCZ PAD_ TIMESTAMP__________
> >                               4093587712  
> >> 00000010: 3f00 d2ff 8910 0000 0300 f6ff 0000 0000  ?...............  
> >                               4143907584  
> >> 00000020: 4900 dcff 7a10 0000 caff 0100 0000 0000  I...z...........  
> >                               So this one looks bad.
> >  
> >> 00000030: 4c00 d9ff 7910 0000 2f00 f8ff 0000 0000  L...y.../.......  
> >                               4177473280
> >  
> >> 00000040: 4b00 d9ff 8410 0000 1f00 0800 0000 0000  K...............  
> >                               also bad.  
> >> 00000050: 4700 daff 7f10 0000 3b00 eeff 0000 0000  G.......;.......
> >> 00000060: 3f00 d8ff 8410 0000 0c00 0900 0000 0000  ?...............
> >> 00000070: 4600 d9ff 8010 0000 0e00 0800 0000 0000  F...............
> >> 00000080: 4700 d7ff 7d10 0000 3400 feff 0000 0000  G...}...4.......
> >> 00000090: 4b00 d4ff 8010 0000 3e00 1200 0000 0000  K.......>.......
> >> 000000a0: 4600 d6ff 8d10 0000 4300 0000 0000 0000  F.......C.......
> >> 000000b0: 4900 d6ff 7710 0000 2500 f0ff 0000 0000  I...w...%.......
> >>
> >> Converted value  
> >I guess this is different data as doesn't seem to line up with the above?
> >  
> >> 0.015625 -0.009277 1.024411 589929
> >> 0.015869 -0.009521 1.040769 4294901719
> >> 0.020508 -0.008301 1.025632 458712
> >> 0.018799 -0.006836 1.032956 851960
> >> 0.019287 -0.009521 1.033201 4294836275
> >> 0.015625 -0.010498 1.031003 4293328982
> >> 0.015137 -0.010498 1.031980 4293853176
> >> 0.015869 -0.009521 1.031492 4293722141
> >> 0.018555 -0.011475 1.033445 4294311886
> >>
> >> The 3 acc values is correct in buffer.  Nevertheless, invalid timestamp. The  
> >timestamp is actually the value of the gyroscope, which directly followed by acc
> >values.  
> >> If we enable the gyroX, gyroY, and gyroZ from userspace, then all the data is  
> >correct. Since the gyro values are the last 3 values and flowed by timestamp.
> >
> >Ok. That's odd and we should debug that.  This code is used in a lot of drivers
> >so if it is not working correctly we need to figure out why asap and fix it.
> >  
> 
> 
> 
> 
> >However, I'm not seeing what looks to be gyro data in bytes 8-15 etc It isn't the
> >stable sequence we'd expect for a timestamp though some specific values
> >might be plausible.
> >
> >Looking again at the code, the IIO_DECLARE_BUFFER_WITH_TS() is the wrong
> >size.  That should not include channel space for the timestamp. That should
> >make it too big though which shouldn't be a problem.
> >Also wrong type - should be using __le16 not s16 for the buffer elements given
> >your channel declarations.
> >
> >Please could you add a print to your code alongside the
> >iio_push_buffer_with_timestamp() to verify that the value in the pf-  
> >>timestamp is reasonable looking for a timestamp.  
> >
> >For reference this is the code that handles the timestamp entry creation in the
> >demux tables.
> >https://elixir.b/
> >ootlin.com%2Flinux%2Fv6.15.1%2Fsource%2Fdrivers%2Fiio%2Findustrialio-
> >buffer.c%23L1086&data=05%7C02%7CJianping.Shen%40de.bosch.com%7Cf0
> >9eaf03f8e44dd1e6fe08ddc53ae596%7C0ae51e1907c84e4bbb6d648ee5841
> >0f4%7C0%7C0%7C638883578931715207%7CUnknown%7CTWFpbGZsb3d8
> >eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIj
> >oiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=s53tTw6o%2F2guA
> >iH3J9jBRd0%2Bj6UmcmgyhtBCuKK1HE0%3D&reserved=0
> >
> >Jonathan
> >
> >  
> >>
> >> Conclusion: Setting available_scan_masks = {  SMI330_ALL_CHAN_MSK, 0 },  
> >the iio core is able to correct handle the enabled channel data, but not the
> >timestamp.  
> >> The working solution for now is that our driver takes care and just copys the  
> >enabled channel value to buffer without using available_scan_masks.  
> >>  
> >> >So the aim is that userspace never knows anything about this.  Just
> >> >set what channels you want and get that data.
> >> >
> >> >Jonathan
> >> >
> >> >  
> >> >>  
> >> >> >
> >> >> >The handling the core code is reasonably sophisticated and will
> >> >> >use bulk copying where appropriate.
> >> >> >
> >> >> >If there is a strong reason to not use that, add a comment here so
> >> >> >we don't have anyone 'fix' this code in future.
> >> >> >  
> >> >> >> +      }
> >> >> >> +
> >> >> >> +      iio_push_to_buffers_with_timestamp(indio_dev, data->buf,
> >> >> >> +pf->timestamp);
> >> >> >> +
> >> >> >> +out:
> >> >> >> +      iio_trigger_notify_done(indio_dev->trig);
> >> >> >> +
> >> >> >> +      return IRQ_HANDLED;
> >> >> >> +}  
> >> >>  
> >>  
> 
> 

AW: [PATCH v3 2/2] iio: imu: smi330: Add driver

[Shen Jianping (ME-SE/EAD2)]

Hi Jonathan,

let's think about a typical IMU, which has 6 2-byte channels (acc x,y,z  gyro x,y,z)	

3 2-byte channel we want, 3 2-byte channels we don't a 4-byte gap and an 8-byte timestamp struct scan {
	u16 wanted[3];
	u16 notwanted[3];
	... gap...
	aligned_s64 timestamp;
	
Hint: indio_dev->scan_bytes is 24, if we use available_scan_mask with all channels set (ref. https://elixir.bootlin.com/linux/v6.15.1/source/drivers/iio/industrialio-buffer.c#L975)

Cutting down to the parts that change in_loc only.

	for_each_set_bit(out_ind, buffer->scan_mask, masklength) {
		in_ind = find_next_bit(indio_dev->active_scan_mask,
				       masklength, in_ind + 1);
		while (in_ind != out_ind) {
... length is the length of current channel .. We never enter here in the example.
			/* Make sure we are aligned */
			in_loc = roundup(in_loc, length) + length;

			in_ind = find_next_bit(indio_dev->active_scan_mask,
					       masklength, in_ind + 1);
		}

... length is the length of the current channel.  Get here on first hit.

		in_loc = roundup(in_loc, length);

		in_loc += length;
.. in loc = 2 + 2 + 2 = 6
	}
	/* Relies on scan_timestamp being last */
	if (buffer->scan_timestamp) {

... length is 8 

		in_loc = roundup(in_loc, length);
.. in_loc = 8 (should be 16 to match timestamp offset: https://elixir.bootlin.com/linux/v6.15.1/source/include/linux/iio/buffer.h#L37)
		ret = iio_buffer_add_demux(buffer, &p, in_loc, out_loc, length);

	}

Best Regards

Jianping Shen

>
>> Hi Jonathan,
>>
>> we find out the reason why the timestamp is invalid in the iio buffer.
>>
>> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Felix
>> ir.bootlin.com%2Flinux%2Fv6.15.1%2Fsource%2Fdrivers%2Fiio%2Findustrial
>> io-
>buffer.c%23L1093&data=05%7C02%7CJianping.Shen%40de.bosch.com%7Cd
>84c
>>
>234178ee4bae6a2a08ddcac3e80a%7C0ae51e1907c84e4bbb6d648ee58410f
>4%7C0%7C
>>
>0%7C638889664948004786%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1
>hcGkiOnRydWU
>>
>sIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3
>D%
>>
>7C0%7C%7C%7C&sdata=DAYIrdjPq4RrrvH7tudEjOlAavn%2B2qlpGiRp2UXTh2
>c%3D&re
>> served=0
>>
>> In "iio_buffer_update_demux" to copy the timestamp, the address calculation
>is the root causes.
>>
>> 1083  in_loc += length;
>> ....
>> 1093  in_loc = roundup(in_loc, length);
>>
>> When finish to copy the channel data, in_loc is just incremented and used as
>address of timestamp. This is correct only when the channel direct before
>timestamp is enabled.
>>
>> If there is a gap between the last enabled channel and timestamp, then iio
>core will copy the wrong data.
>>
>> We have a fix to this issue,
>>
>> 1093 in_loc = (indio_dev->scan_bytes / sizeof(int64_t) - 1) * length;
>
>That looks correct, but I'm not seeing why the roundup above doesn't land us
>in the same place.  I'm not that keen on handling the timestamp even more
>differently to other channels.
>
>
>If we imagine an active scan with
>2-byte chanel we want, 2 2-byte channels we don't a 2-byte gap and an 8-byte
>timestamp struct scan {
>	u16 wanted;
>	u16 notwanted[2];
>	... gap...
>	aligned_s64 timestamp;
>
>
>
>Cutting down to the parts that change in_loc only.
>
>	for_each_set_bit(out_ind, buffer->scan_mask, masklength) {
>		in_ind = find_next_bit(indio_dev->active_scan_mask,
>				       masklength, in_ind + 1);
>		while (in_ind != out_ind) {
>... length is the length of current channel .. We never enter here in the example.
>			/* Make sure we are aligned */
>			in_loc = roundup(in_loc, length) + length;
>
>			in_ind = find_next_bit(indio_dev->active_scan_mask,
>					       masklength, in_ind + 1);
>		}
>
>... length is the length of the current channel.  Get here on first hit.
>
>		in_loc = roundup(in_loc, length);
>
>		in_loc += length;
>.. in loc = 2
>	}
>	/* Relies on scan_timestamp being last */
>	if (buffer->scan_timestamp) {
>
>... length is 8 ...
>
>		in_loc = roundup(in_loc, length);
>.. I think in_lock = 8?
>		ret = iio_buffer_add_demux(buffer, &p, in_loc, out_loc, length);
>
>	}
>
>Perhaps you can talk through the example that is failing?
>
>>
>> just not sure, if there will be any side-effects with this fix.
>>
>> Are you going to fix this finding, or shall we create a new patch for that?
>
>Fine to send the proposed fix but first we need to step through why the current
>code isn't working.
>
>
>Thanks,
>
>Jonathan
>
>>
>> Best regards
>> Jianping Shen
>>
>>
>> >>
>> >> >>
>> >> >> >> +
>> >> >> >> +static irqreturn_t smi330_trigger_handler(int irq, void *p) {
>> >> >> >> +      struct iio_poll_func *pf = p;
>> >> >> >> +      struct iio_dev *indio_dev = pf->indio_dev;
>> >> >> >> +      struct smi330_data *data = iio_priv(indio_dev);
>> >> >> >> +      int ret, chan;
>> >> >> >> +      int i = 0;
>> >> >> >> +
>> >> >> >> +      ret = regmap_bulk_read(data->regmap,
>> >SMI330_ACCEL_X_REG, data-
>> >> >> >>buf,
>> >> >> >> +                             ARRAY_SIZE(smi330_channels));
>> >> >> >> +      if (ret)
>> >> >> >> +              goto out;
>> >> >> >> +
>> >> >> >> +      if (*indio_dev->active_scan_mask !=
>> >> >> >> + SMI330_ALL_CHAN_MSK)
>> >{
>> >> >> >> +              iio_for_each_active_channel(indio_dev, chan)
>> >> >> >> +                      data->buf[i++] = data->buf[chan];
>> >> >> >
>> >> >> >If I follow this correctly you are reading all the channels and
>> >> >> >just copying out the ones you want.  Just let the IIO core do
>> >> >> >that for you by setting iio_dev-
>> >> >> >>available_scan_masks = {  SMI330_ALL_CHAN_MSK, 0 }; and push
>> >> >> >>the whole
>> >> >> >buffer every time.
>> >> >>
>> >> >> For the most frequent use cases, we define available_scan_masks
>> >> >> = {
>> >> >SMI330_ALL_CHAN_MSK, SMI330_ACC_XYZ_MSK,
>> >SMI330_GYRO_XYZ_MSK,
>> >> >0 }; and push the whole buffer every time.
>> >> >> From the user space we just enable 3 channels gyro_x, gyro_y, and
>gyro_z.
>> >> >Then we enable buffer and expect that only the gyro values and
>> >> >timestamp in iio_buffer. Nevertheless, we have 3 accelerometer
>> >> >values and the timestamp in iio_buffer.
>> >> >
>> >> >> It seems that the iio core does not take care which channel is
>> >> >> enabled,  just
>> >> >copy the first 3 values (acc x,y,z) into iio_buffer.  Our driver
>> >> >code still needs to take care and just copy the enabled channel value to
>buffer.
>> >> >
>> >> >Look again at how it works.  If you provide ACC_XYZ_MSK, then your
>> >> >driver has to handle it.
>> >> >available_scan_masks is saying what your driver supports. The
>> >> >driver can check active_scan_mask to find out what is enabled.  So
>> >> >right option here is only { SMI330_ALL_CHAN_MSK, 0, }  In that
>> >> >case the driver never needs to check as there is only one option.
>> >> >
>> >> >Then if any subset of channels is enabled the IIO core copy out
>> >> >just the data that is relevant.
>> >> >
>> >> >
>> >> >>
>> >> >> Another side effect after using available_scan_masks is that the
>> >> >active_scan_masks sometimes does not reflect current channel
>> >> >activation status.
>> >> >>
>> >> >> Is some step missing to properly use available_scan_masks ?  How
>> >> >> can a user
>> >> >find out from user space which channel combination is defined in
>> >> >available_scan_masks ?
>> >> >
>> >> >Why would userspace want to?  Userspace requested a subset of
>> >> >channels and it gets that subset.  So it if asks for the channels
>> >> >that make up SMI330_ACC_XYZ_MSK, if available_scan_mask == {
>> >> >SMI330_ALL_CHAN_MSK,
>> >> >0 } then the IIO core handling selects SMI330_ALL_CHAN_MSK
>> >> >(smallest available mask that is superset of what we asked for)
>> >> >and sets active_scan_mask to that.  The driver follows what
>> >> >active_scan_mask specifies and passes all channel data via the
>> >> >iio_push_to_buffers*() call. The demux in the IIO core than takes
>> >> >that 'scan' and repacks it so that userspace receives just the
>> >> >data it asked for formatting exactly as the driver would have done
>> >> >it if you had handled each channels
>> >separately in the driver.
>> >> >
>> >>
>> >> Set available_scan_masks = {  SMI330_ALL_CHAN_MSK, 0 } and push the
>> >>whole buffer. iio_push_to_buffers_with_timestamp (indio_dev,
>> >>data->buf, pf- timestamp); We enable the accX, accY, and accZ from
>> >>userspace. And expect 3
>> >acc values and the timestamp in iio buffer.
>> >>
>> >> Raw iio buffer data:
>> >> 00000000: 3c00 d6ff 7510 0000 6100 f3ff 0000 0000  <...u...a.......
>> >            ACCX ACCY ACCZ PAD_ TIMESTAMP__________
>> >                               4093587712
>> >> 00000010: 3f00 d2ff 8910 0000 0300 f6ff 0000 0000  ?...............
>> >                               4143907584
>> >> 00000020: 4900 dcff 7a10 0000 caff 0100 0000 0000  I...z...........
>> >                               So this one looks bad.
>> >
>> >> 00000030: 4c00 d9ff 7910 0000 2f00 f8ff 0000 0000  L...y.../.......
>> >                               4177473280
>> >
>> >> 00000040: 4b00 d9ff 8410 0000 1f00 0800 0000 0000  K...............
>> >                               also bad.
>> >> 00000050: 4700 daff 7f10 0000 3b00 eeff 0000 0000  G.......;.......
>> >> 00000060: 3f00 d8ff 8410 0000 0c00 0900 0000 0000  ?...............
>> >> 00000070: 4600 d9ff 8010 0000 0e00 0800 0000 0000  F...............
>> >> 00000080: 4700 d7ff 7d10 0000 3400 feff 0000 0000  G...}...4.......
>> >> 00000090: 4b00 d4ff 8010 0000 3e00 1200 0000 0000  K.......>.......
>> >> 000000a0: 4600 d6ff 8d10 0000 4300 0000 0000 0000  F.......C.......
>> >> 000000b0: 4900 d6ff 7710 0000 2500 f0ff 0000 0000  I...w...%.......
>> >>
>> >> Converted value
>> >I guess this is different data as doesn't seem to line up with the above?
>> >
>> >> 0.015625 -0.009277 1.024411 589929
>> >> 0.015869 -0.009521 1.040769 4294901719
>> >> 0.020508 -0.008301 1.025632 458712
>> >> 0.018799 -0.006836 1.032956 851960
>> >> 0.019287 -0.009521 1.033201 4294836275
>> >> 0.015625 -0.010498 1.031003 4293328982
>> >> 0.015137 -0.010498 1.031980 4293853176
>> >> 0.015869 -0.009521 1.031492 4293722141
>> >> 0.018555 -0.011475 1.033445 4294311886
>> >>
>> >> The 3 acc values is correct in buffer.  Nevertheless, invalid
>> >> timestamp. The
>> >timestamp is actually the value of the gyroscope, which directly
>> >followed by acc values.
>> >> If we enable the gyroX, gyroY, and gyroZ from userspace, then all
>> >> the data is
>> >correct. Since the gyro values are the last 3 values and flowed by timestamp.
>> >
>> >Ok. That's odd and we should debug that.  This code is used in a lot
>> >of drivers so if it is not working correctly we need to figure out why asap and
>fix it.
>> >
>>
>>
>>
>>
>> >However, I'm not seeing what looks to be gyro data in bytes 8-15 etc
>> >It isn't the stable sequence we'd expect for a timestamp though some
>> >specific values might be plausible.
>> >
>> >Looking again at the code, the IIO_DECLARE_BUFFER_WITH_TS() is the
>> >wrong size.  That should not include channel space for the timestamp.
>> >That should make it too big though which shouldn't be a problem.
>> >Also wrong type - should be using __le16 not s16 for the buffer
>> >elements given your channel declarations.
>> >
>> >Please could you add a print to your code alongside the
>> >iio_push_buffer_with_timestamp() to verify that the value in the pf-
>> >>timestamp is reasonable looking for a timestamp.
>> >
>> >For reference this is the code that handles the timestamp entry
>> >creation in the demux tables.
>> >https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Feli
>>
>>xir.b%2F&data=05%7C02%7CJianping.Shen%40de.bosch.com%7Cd84c2341
>78ee4b
>>
>>ae6a2a08ddcac3e80a%7C0ae51e1907c84e4bbb6d648ee58410f4%7C0%7C
>0%7C63888
>>
>>9664948038159%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRy
>dWUsIlYiOiI
>>
>>wLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7
>C0%7C%
>>
>>7C%7C&sdata=7kNAXwi9fkp5XocdJ2K5W2Cp9%2BQW4Wq6GLr5reGqies%3
>D&reserved
>> >=0
>>
>>ootlin.com%2Flinux%2Fv6.15.1%2Fsource%2Fdrivers%2Fiio%2Findustrialio-
>>
>>buffer.c%23L1086&data=05%7C02%7CJianping.Shen%40de.bosch.com%7Cf
>0
>>
>>9eaf03f8e44dd1e6fe08ddc53ae596%7C0ae51e1907c84e4bbb6d648ee584
>1
>>
>>0f4%7C0%7C0%7C638883578931715207%7CUnknown%7CTWFpbGZsb3d
>8
>>
>>eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOI
>j
>>
>>oiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=s53tTw6o%2F2gu
>A
>> >iH3J9jBRd0%2Bj6UmcmgyhtBCuKK1HE0%3D&reserved=0
>> >
>> >Jonathan
>> >
>> >
>> >>
>> >> Conclusion: Setting available_scan_masks = {  SMI330_ALL_CHAN_MSK,
>> >> 0 },
>> >the iio core is able to correct handle the enabled channel data, but
>> >not the timestamp.
>> >> The working solution for now is that our driver takes care and just
>> >> copys the
>> >enabled channel value to buffer without using available_scan_masks.
>> >>
>> >> >So the aim is that userspace never knows anything about this.
>> >> >Just set what channels you want and get that data.
>> >> >
>> >> >Jonathan
>> >> >
>> >> >
>> >> >>
>> >> >> >
>> >> >> >The handling the core code is reasonably sophisticated and will
>> >> >> >use bulk copying where appropriate.
>> >> >> >
>> >> >> >If there is a strong reason to not use that, add a comment here
>> >> >> >so we don't have anyone 'fix' this code in future.
>> >> >> >
>> >> >> >> +      }
>> >> >> >> +
>> >> >> >> +      iio_push_to_buffers_with_timestamp(indio_dev,
>> >> >> >> + data->buf,
>> >> >> >> +pf->timestamp);
>> >> >> >> +
>> >> >> >> +out:
>> >> >> >> +      iio_trigger_notify_done(indio_dev->trig);
>> >> >> >> +
>> >> >> >> +      return IRQ_HANDLED;
>> >> >> >> +}
>> >> >>
>> >>
>>
>>

Re: [PATCH v3 2/2] iio: imu: smi330: Add driver

[Jonathan Cameron]

On Mon, 28 Jul 2025 12:14:55 +0000
"Shen Jianping (ME-SE/EAD2)" <Jianping.Shen@de.bosch.com> wrote:

> Hi Jonathan,
> 
> let's think about a typical IMU, which has 6 2-byte channels (acc x,y,z  gyro x,y,z)	
> 
> 3 2-byte channel we want, 3 2-byte channels we don't a 4-byte gap and an 8-byte timestamp struct scan {
> 	u16 wanted[3];
> 	u16 notwanted[3];
> 	... gap...
> 	aligned_s64 timestamp;
> 	
> Hint: indio_dev->scan_bytes is 24, if we use available_scan_mask with all channels set (ref. https://elixir.bootlin.com/linux/v6.15.1/source/drivers/iio/industrialio-buffer.c#L975)
> 
> Cutting down to the parts that change in_loc only.
> 
> 	for_each_set_bit(out_ind, buffer->scan_mask, masklength) {
> 		in_ind = find_next_bit(indio_dev->active_scan_mask,
> 				       masklength, in_ind + 1);
> 		while (in_ind != out_ind) {
> ... length is the length of current channel .. We never enter here in the example.
> 			/* Make sure we are aligned */
> 			in_loc = roundup(in_loc, length) + length;
> 
> 			in_ind = find_next_bit(indio_dev->active_scan_mask,
> 					       masklength, in_ind + 1);
> 		}
> 
> ... length is the length of the current channel.  Get here on first hit.
> 
> 		in_loc = roundup(in_loc, length);
> 
> 		in_loc += length;
> .. in loc = 2 + 2 + 2 = 6
> 	}
> 	/* Relies on scan_timestamp being last */
> 	if (buffer->scan_timestamp) {
> 
> ... length is 8 
> 
> 		in_loc = roundup(in_loc, length);
> .. in_loc = 8 (should be 16 to match timestamp offset: https://elixir.bootlin.com/linux/v6.15.1/source/include/linux/iio/buffer.h#L37)
> 		ret = iio_buffer_add_demux(buffer, &p, in_loc, out_loc, length);
> 
> 	}
> 
> Best Regards
Ah. Got it - I foolishly didn't try a big enough example.  Thanks for all your
work chasing this down!  I somewhat surprised we never hit this before :(

Ok, so in that case the fix (to keep in line with existing code approach) is to walk
the gap.  Completely untested, but something like:

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index a80f7cc25a27..f58a7ce481f5 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1082,6 +1082,21 @@ static int iio_buffer_update_demux(struct iio_dev *indio_dev,
                out_loc += length;
                in_loc += length;
        }
+       /* Walk remaining bits of active_scan_mask */
+       in_ind = find_next_bit(indio_dev->active_scan_mask, masklength,
+                              in_ind + 1);
+       while (in_ind != masklength) {
+               ret = iio_storage_bytes_for_si(indio_dev, in_ind);
+               if (ret < 0)
+                       goto error_clear_mux_table;
+
+               length = ret;
+               /* Make sure we are aligned */
+               in_loc = roundup(in_loc, length) + length;
+               in_ind = find_next_bit(indio_dev->active_scan_mask,
+                                      masklength, in_ind + 1);
+
+       }
        /* Relies on scan_timestamp being last */
        if (buffer->scan_timestamp) {
                ret = iio_storage_bytes_for_timestamp(indio_dev);

Obviously quite a bit more complex than your solution, but consistent with
the surrounding code.

We could make this more efficient by moving it under the
if (buffer->scan_timestamp).
Could potentially also use a for_each_bit_set_from() but then the
code looks quite different to the other cases.

What do you think?

Jonathan

> 
> Jianping Shen
> 
> >  
> >> Hi Jonathan,
> >>
> >> we find out the reason why the timestamp is invalid in the iio buffer.
> >>
> >> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Felix
> >> ir.bootlin.com%2Flinux%2Fv6.15.1%2Fsource%2Fdrivers%2Fiio%2Findustrial
> >> io-  
> >buffer.c%23L1093&data=05%7C02%7CJianping.Shen%40de.bosch.com%7Cd
> >84c  
> >>  
> >234178ee4bae6a2a08ddcac3e80a%7C0ae51e1907c84e4bbb6d648ee58410f
> >4%7C0%7C  
> >>  
> >0%7C638889664948004786%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1
> >hcGkiOnRydWU  
> >>  
> >sIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3
> >D%  
> >>  
> >7C0%7C%7C%7C&sdata=DAYIrdjPq4RrrvH7tudEjOlAavn%2B2qlpGiRp2UXTh2
> >c%3D&re  
> >> served=0
> >>
> >> In "iio_buffer_update_demux" to copy the timestamp, the address calculation  
> >is the root causes.  
> >>
> >> 1083  in_loc += length;
> >> ....
> >> 1093  in_loc = roundup(in_loc, length);
> >>
> >> When finish to copy the channel data, in_loc is just incremented and used as  
> >address of timestamp. This is correct only when the channel direct before
> >timestamp is enabled.  
> >>
> >> If there is a gap between the last enabled channel and timestamp, then iio  
> >core will copy the wrong data.  
> >>
> >> We have a fix to this issue,
> >>
> >> 1093 in_loc = (indio_dev->scan_bytes / sizeof(int64_t) - 1) * length;  
> >
> >That looks correct, but I'm not seeing why the roundup above doesn't land us
> >in the same place.  I'm not that keen on handling the timestamp even more
> >differently to other channels.
> >
> >
> >If we imagine an active scan with
> >2-byte chanel we want, 2 2-byte channels we don't a 2-byte gap and an 8-byte
> >timestamp struct scan {
> >	u16 wanted;
> >	u16 notwanted[2];
> >	... gap...
> >	aligned_s64 timestamp;
> >
> >
> >
> >Cutting down to the parts that change in_loc only.
> >
> >	for_each_set_bit(out_ind, buffer->scan_mask, masklength) {
> >		in_ind = find_next_bit(indio_dev->active_scan_mask,
> >				       masklength, in_ind + 1);
> >		while (in_ind != out_ind) {
> >... length is the length of current channel .. We never enter here in the example.
> >			/* Make sure we are aligned */
> >			in_loc = roundup(in_loc, length) + length;
> >
> >			in_ind = find_next_bit(indio_dev->active_scan_mask,
> >					       masklength, in_ind + 1);
> >		}
> >
> >... length is the length of the current channel.  Get here on first hit.
> >
> >		in_loc = roundup(in_loc, length);
> >
> >		in_loc += length;
> >.. in loc = 2
> >	}
> >	/* Relies on scan_timestamp being last */
> >	if (buffer->scan_timestamp) {
> >
> >... length is 8 ...
> >
> >		in_loc = roundup(in_loc, length);
> >.. I think in_lock = 8?
> >		ret = iio_buffer_add_demux(buffer, &p, in_loc, out_loc, length);
> >
> >	}
> >
> >Perhaps you can talk through the example that is failing?
> >  
> >>
> >> just not sure, if there will be any side-effects with this fix.
> >>
> >> Are you going to fix this finding, or shall we create a new patch for that?  
> >
> >Fine to send the proposed fix but first we need to step through why the current
> >code isn't working.
> >
> >
> >Thanks,
> >
> >Jonathan
> >  
> >>
> >> Best regards
> >> Jianping Shen
> >>
> >>  
> >> >>  
> >> >> >>  
> >> >> >> >> +
> >> >> >> >> +static irqreturn_t smi330_trigger_handler(int irq, void *p) {
> >> >> >> >> +      struct iio_poll_func *pf = p;
> >> >> >> >> +      struct iio_dev *indio_dev = pf->indio_dev;
> >> >> >> >> +      struct smi330_data *data = iio_priv(indio_dev);
> >> >> >> >> +      int ret, chan;
> >> >> >> >> +      int i = 0;
> >> >> >> >> +
> >> >> >> >> +      ret = regmap_bulk_read(data->regmap,  
> >> >SMI330_ACCEL_X_REG, data-  
> >> >> >> >>buf,
> >> >> >> >> +                             ARRAY_SIZE(smi330_channels));
> >> >> >> >> +      if (ret)
> >> >> >> >> +              goto out;
> >> >> >> >> +
> >> >> >> >> +      if (*indio_dev->active_scan_mask !=
> >> >> >> >> + SMI330_ALL_CHAN_MSK)  
> >> >{  
> >> >> >> >> +              iio_for_each_active_channel(indio_dev, chan)
> >> >> >> >> +                      data->buf[i++] = data->buf[chan];  
> >> >> >> >
> >> >> >> >If I follow this correctly you are reading all the channels and
> >> >> >> >just copying out the ones you want.  Just let the IIO core do
> >> >> >> >that for you by setting iio_dev-  
> >> >> >> >>available_scan_masks = {  SMI330_ALL_CHAN_MSK, 0 }; and push
> >> >> >> >>the whole  
> >> >> >> >buffer every time.  
> >> >> >>
> >> >> >> For the most frequent use cases, we define available_scan_masks
> >> >> >> = {  
> >> >> >SMI330_ALL_CHAN_MSK, SMI330_ACC_XYZ_MSK,  
> >> >SMI330_GYRO_XYZ_MSK,  
> >> >> >0 }; and push the whole buffer every time.  
> >> >> >> From the user space we just enable 3 channels gyro_x, gyro_y, and  
> >gyro_z.  
> >> >> >Then we enable buffer and expect that only the gyro values and
> >> >> >timestamp in iio_buffer. Nevertheless, we have 3 accelerometer
> >> >> >values and the timestamp in iio_buffer.
> >> >> >  
> >> >> >> It seems that the iio core does not take care which channel is
> >> >> >> enabled,  just  
> >> >> >copy the first 3 values (acc x,y,z) into iio_buffer.  Our driver
> >> >> >code still needs to take care and just copy the enabled channel value to  
> >buffer.  
> >> >> >
> >> >> >Look again at how it works.  If you provide ACC_XYZ_MSK, then your
> >> >> >driver has to handle it.
> >> >> >available_scan_masks is saying what your driver supports. The
> >> >> >driver can check active_scan_mask to find out what is enabled.  So
> >> >> >right option here is only { SMI330_ALL_CHAN_MSK, 0, }  In that
> >> >> >case the driver never needs to check as there is only one option.
> >> >> >
> >> >> >Then if any subset of channels is enabled the IIO core copy out
> >> >> >just the data that is relevant.
> >> >> >
> >> >> >  
> >> >> >>
> >> >> >> Another side effect after using available_scan_masks is that the  
> >> >> >active_scan_masks sometimes does not reflect current channel
> >> >> >activation status.  
> >> >> >>
> >> >> >> Is some step missing to properly use available_scan_masks ?  How
> >> >> >> can a user  
> >> >> >find out from user space which channel combination is defined in
> >> >> >available_scan_masks ?
> >> >> >
> >> >> >Why would userspace want to?  Userspace requested a subset of
> >> >> >channels and it gets that subset.  So it if asks for the channels
> >> >> >that make up SMI330_ACC_XYZ_MSK, if available_scan_mask == {
> >> >> >SMI330_ALL_CHAN_MSK,
> >> >> >0 } then the IIO core handling selects SMI330_ALL_CHAN_MSK
> >> >> >(smallest available mask that is superset of what we asked for)
> >> >> >and sets active_scan_mask to that.  The driver follows what
> >> >> >active_scan_mask specifies and passes all channel data via the
> >> >> >iio_push_to_buffers*() call. The demux in the IIO core than takes
> >> >> >that 'scan' and repacks it so that userspace receives just the
> >> >> >data it asked for formatting exactly as the driver would have done
> >> >> >it if you had handled each channels  
> >> >separately in the driver.  
> >> >> >  
> >> >>
> >> >> Set available_scan_masks = {  SMI330_ALL_CHAN_MSK, 0 } and push the
> >> >>whole buffer. iio_push_to_buffers_with_timestamp (indio_dev,
> >> >>data->buf, pf- timestamp); We enable the accX, accY, and accZ from
> >> >>userspace. And expect 3  
> >> >acc values and the timestamp in iio buffer.  
> >> >>
> >> >> Raw iio buffer data:
> >> >> 00000000: 3c00 d6ff 7510 0000 6100 f3ff 0000 0000  <...u...a.......  
> >> >            ACCX ACCY ACCZ PAD_ TIMESTAMP__________
> >> >                               4093587712  
> >> >> 00000010: 3f00 d2ff 8910 0000 0300 f6ff 0000 0000  ?...............  
> >> >                               4143907584  
> >> >> 00000020: 4900 dcff 7a10 0000 caff 0100 0000 0000  I...z...........  
> >> >                               So this one looks bad.
> >> >  
> >> >> 00000030: 4c00 d9ff 7910 0000 2f00 f8ff 0000 0000  L...y.../.......  
> >> >                               4177473280
> >> >  
> >> >> 00000040: 4b00 d9ff 8410 0000 1f00 0800 0000 0000  K...............  
> >> >                               also bad.  
> >> >> 00000050: 4700 daff 7f10 0000 3b00 eeff 0000 0000  G.......;.......
> >> >> 00000060: 3f00 d8ff 8410 0000 0c00 0900 0000 0000  ?...............
> >> >> 00000070: 4600 d9ff 8010 0000 0e00 0800 0000 0000  F...............
> >> >> 00000080: 4700 d7ff 7d10 0000 3400 feff 0000 0000  G...}...4.......
> >> >> 00000090: 4b00 d4ff 8010 0000 3e00 1200 0000 0000  K.......>.......
> >> >> 000000a0: 4600 d6ff 8d10 0000 4300 0000 0000 0000  F.......C.......
> >> >> 000000b0: 4900 d6ff 7710 0000 2500 f0ff 0000 0000  I...w...%.......
> >> >>
> >> >> Converted value  
> >> >I guess this is different data as doesn't seem to line up with the above?
> >> >  
> >> >> 0.015625 -0.009277 1.024411 589929
> >> >> 0.015869 -0.009521 1.040769 4294901719
> >> >> 0.020508 -0.008301 1.025632 458712
> >> >> 0.018799 -0.006836 1.032956 851960
> >> >> 0.019287 -0.009521 1.033201 4294836275
> >> >> 0.015625 -0.010498 1.031003 4293328982
> >> >> 0.015137 -0.010498 1.031980 4293853176
> >> >> 0.015869 -0.009521 1.031492 4293722141
> >> >> 0.018555 -0.011475 1.033445 4294311886
> >> >>
> >> >> The 3 acc values is correct in buffer.  Nevertheless, invalid
> >> >> timestamp. The  
> >> >timestamp is actually the value of the gyroscope, which directly
> >> >followed by acc values.  
> >> >> If we enable the gyroX, gyroY, and gyroZ from userspace, then all
> >> >> the data is  
> >> >correct. Since the gyro values are the last 3 values and flowed by timestamp.
> >> >
> >> >Ok. That's odd and we should debug that.  This code is used in a lot
> >> >of drivers so if it is not working correctly we need to figure out why asap and  
> >fix it.  
> >> >  
> >>
> >>
> >>
> >>  
> >> >However, I'm not seeing what looks to be gyro data in bytes 8-15 etc
> >> >It isn't the stable sequence we'd expect for a timestamp though some
> >> >specific values might be plausible.
> >> >
> >> >Looking again at the code, the IIO_DECLARE_BUFFER_WITH_TS() is the
> >> >wrong size.  That should not include channel space for the timestamp.
> >> >That should make it too big though which shouldn't be a problem.
> >> >Also wrong type - should be using __le16 not s16 for the buffer
> >> >elements given your channel declarations.
> >> >
> >> >Please could you add a print to your code alongside the
> >> >iio_push_buffer_with_timestamp() to verify that the value in the pf-  
> >> >>timestamp is reasonable looking for a timestamp.  
> >> >
> >> >For reference this is the code that handles the timestamp entry
> >> >creation in the demux tables.
> >> >https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Feli  
> >>
> >>xir.b%2F&data=05%7C02%7CJianping.Shen%40de.bosch.com%7Cd84c2341  
> >78ee4b  
> >>
> >>ae6a2a08ddcac3e80a%7C0ae51e1907c84e4bbb6d648ee58410f4%7C0%7C  
> >0%7C63888  
> >>
> >>9664948038159%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRy  
> >dWUsIlYiOiI  
> >>
> >>wLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7  
> >C0%7C%  
> >>
> >>7C%7C&sdata=7kNAXwi9fkp5XocdJ2K5W2Cp9%2BQW4Wq6GLr5reGqies%3  
> >D&reserved  
> >> >=0  
> >>
> >>ootlin.com%2Flinux%2Fv6.15.1%2Fsource%2Fdrivers%2Fiio%2Findustrialio-
> >>
> >>buffer.c%23L1086&data=05%7C02%7CJianping.Shen%40de.bosch.com%7Cf  
> >0  
> >>
> >>9eaf03f8e44dd1e6fe08ddc53ae596%7C0ae51e1907c84e4bbb6d648ee584  
> >1  
> >>
> >>0f4%7C0%7C0%7C638883578931715207%7CUnknown%7CTWFpbGZsb3d  
> >8  
> >>
> >>eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOI  
> >j  
> >>
> >>oiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=s53tTw6o%2F2gu  
> >A  
> >> >iH3J9jBRd0%2Bj6UmcmgyhtBCuKK1HE0%3D&reserved=0
> >> >
> >> >Jonathan
> >> >
> >> >  
> >> >>
> >> >> Conclusion: Setting available_scan_masks = {  SMI330_ALL_CHAN_MSK,
> >> >> 0 },  
> >> >the iio core is able to correct handle the enabled channel data, but
> >> >not the timestamp.  
> >> >> The working solution for now is that our driver takes care and just
> >> >> copys the  
> >> >enabled channel value to buffer without using available_scan_masks.  
> >> >>  
> >> >> >So the aim is that userspace never knows anything about this.
> >> >> >Just set what channels you want and get that data.
> >> >> >
> >> >> >Jonathan
> >> >> >
> >> >> >  
> >> >> >>  
> >> >> >> >
> >> >> >> >The handling the core code is reasonably sophisticated and will
> >> >> >> >use bulk copying where appropriate.
> >> >> >> >
> >> >> >> >If there is a strong reason to not use that, add a comment here
> >> >> >> >so we don't have anyone 'fix' this code in future.
> >> >> >> >  
> >> >> >> >> +      }
> >> >> >> >> +
> >> >> >> >> +      iio_push_to_buffers_with_timestamp(indio_dev,
> >> >> >> >> + data->buf,
> >> >> >> >> +pf->timestamp);
> >> >> >> >> +
> >> >> >> >> +out:
> >> >> >> >> +      iio_trigger_notify_done(indio_dev->trig);
> >> >> >> >> +
> >> >> >> >> +      return IRQ_HANDLED;
> >> >> >> >> +}  
> >> >> >>  
> >> >>  
> >>
> >>  
> 
> 

AW: [PATCH v3 2/2] iio: imu: smi330: Add driver

[Shen Jianping (ME-SE/EAD2)]

Hi Jonathan,

let's take your fix.  You fix is more consistent with the existing code. We already tested on our HW.  Now everything works properly.
When the fix becomes available in "togreg", so that we can continue with our driver upstreaming.

best Regards
Jianping


>> Hi Jonathan,
>>
>> let's think about a typical IMU, which has 6 2-byte channels (acc x,y,z  gyro
>x,y,z)
>>
>> 3 2-byte channel we want, 3 2-byte channels we don't a 4-byte gap and an 8-
>byte timestamp struct scan {
>>      u16 wanted[3];
>>      u16 notwanted[3];
>>      ... gap...
>>      aligned_s64 timestamp;
>>
>> Hint: indio_dev->scan_bytes is 24, if we use available_scan_mask with
>> all channels set (ref.
>> https://elix/
>> ir.bootlin.com%2Flinux%2Fv6.15.1%2Fsource%2Fdrivers%2Fiio%2Findustrial
>> io-
>buffer.c%23L975&data=05%7C02%7CJianping.Shen%40de.bosch.com%7C80
>10a
>>
>2f19e554cacae2b08ddcdd7ad3e%7C0ae51e1907c84e4bbb6d648ee58410f4
>%7C0%7C0
>>
>%7C638893048385267556%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1h
>cGkiOnRydWUs
>>
>IlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3
>D%7
>>
>C0%7C%7C%7C&sdata=tkxZ390MWrVRBYHECSugENRULI%2FT8cS%2BUX%2B
>rNNVo6%2F8%
>> 3D&reserved=0)
>>
>> Cutting down to the parts that change in_loc only.
>>
>>      for_each_set_bit(out_ind, buffer->scan_mask, masklength) {
>>              in_ind = find_next_bit(indio_dev->active_scan_mask,
>>                                     masklength, in_ind + 1);
>>              while (in_ind != out_ind) {
>> ... length is the length of current channel .. We never enter here in the
>example.
>>                      /* Make sure we are aligned */
>>                      in_loc = roundup(in_loc, length) + length;
>>
>>                      in_ind = find_next_bit(indio_dev->active_scan_mask,
>>                                             masklength, in_ind + 1);
>>              }
>>
>> ... length is the length of the current channel.  Get here on first hit.
>>
>>              in_loc = roundup(in_loc, length);
>>
>>              in_loc += length;
>> .. in loc = 2 + 2 + 2 = 6
>>      }
>>      /* Relies on scan_timestamp being last */
>>      if (buffer->scan_timestamp) {
>>
>> ... length is 8
>>
>>              in_loc = roundup(in_loc, length);
>> .. in_loc = 8 (should be 16 to match timestamp offset:
>https://elixir.b/
>ootlin.com%2Flinux%2Fv6.15.1%2Fsource%2Finclude%2Flinux%2Fiio%2Fbuff
>er.h%23L37&data=05%7C02%7CJianping.Shen%40de.bosch.com%7C8010a2f
>19e554cacae2b08ddcdd7ad3e%7C0ae51e1907c84e4bbb6d648ee58410f4%
>7C0%7C0%7C638893048385299918%7CUnknown%7CTWFpbGZsb3d8eyJFb
>XB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWF
>pbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=O2F6UcNaE%2BqBUO0T
>%2F6qAe6dEruhkN%2BeVGJzIBbX8Mn0%3D&reserved=0)
>>              ret = iio_buffer_add_demux(buffer, &p, in_loc, out_loc, length);
>>
>>      }
>>
>> Best Regards
>Ah. Got it - I foolishly didn't try a big enough example.  Thanks for all your work
>chasing this down!  I somewhat surprised we never hit this before :(
>
>Ok, so in that case the fix (to keep in line with existing code approach) is to walk
>the gap.  Completely untested, but something like:
>
>diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
>index a80f7cc25a27..f58a7ce481f5 100644
>--- a/drivers/iio/industrialio-buffer.c
>+++ b/drivers/iio/industrialio-buffer.c
>@@ -1082,6 +1082,21 @@ static int iio_buffer_update_demux(struct iio_dev
>*indio_dev,
>                out_loc += length;
>                in_loc += length;
>        }
>+       /* Walk remaining bits of active_scan_mask */
>+       in_ind = find_next_bit(indio_dev->active_scan_mask, masklength,
>+                              in_ind + 1);
>+       while (in_ind != masklength) {
>+               ret = iio_storage_bytes_for_si(indio_dev, in_ind);
>+               if (ret < 0)
>+                       goto error_clear_mux_table;
>+
>+               length = ret;
>+               /* Make sure we are aligned */
>+               in_loc = roundup(in_loc, length) + length;
>+               in_ind = find_next_bit(indio_dev->active_scan_mask,
>+                                      masklength, in_ind + 1);
>+
>+       }
>        /* Relies on scan_timestamp being last */
>        if (buffer->scan_timestamp) {
>                ret = iio_storage_bytes_for_timestamp(indio_dev);
>
>Obviously quite a bit more complex than your solution, but consistent with the
>surrounding code.
>
>We could make this more efficient by moving it under the if (buffer-
>>scan_timestamp).
>Could potentially also use a for_each_bit_set_from() but then the code looks
>quite different to the other cases.
>
>What do you think?
>
>Jonathan
>
>>
>> Jianping Shen
>>
>> >
>> >> Hi Jonathan,
>> >>
>> >> we find out the reason why the timestamp is invalid in the iio buffer.
>> >>
>> >> https://e/
>> >>
>lix%2F&data=05%7C02%7CJianping.Shen%40de.bosch.com%7C8010a2f19e5
>54c
>> >>
>acae2b08ddcdd7ad3e%7C0ae51e1907c84e4bbb6d648ee58410f4%7C0%7C
>0%7C638
>> >>
>893048385320261%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnR
>ydWUsIlY
>> >>
>iOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%
>7
>> >>
>C0%7C%7C%7C&sdata=DvB9srb9ft9UZ9sbHqpWWTyAexxKbG93qTajtWhzxN
>o%3D&re
>> >> served=0
>> >>
>ir.bootlin.com%2Flinux%2Fv6.15.1%2Fsource%2Fdrivers%2Fiio%2Findustr
>> >> ial
>> >> io-
>>
>>buffer.c%23L1093&data=05%7C02%7CJianping.Shen%40de.bosch.com%7C
>d
>> >84c
>> >>
>>
>>234178ee4bae6a2a08ddcac3e80a%7C0ae51e1907c84e4bbb6d648ee5841
>0f
>> >4%7C0%7C
>> >>
>>
>>0%7C638889664948004786%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU
>1
>> >hcGkiOnRydWU
>> >>
>>
>>sIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%
>3
>> >D%
>> >>
>>
>>7C0%7C%7C%7C&sdata=DAYIrdjPq4RrrvH7tudEjOlAavn%2B2qlpGiRp2UXTh
>2
>> >c%3D&re
>> >> served=0
>> >>
>> >> In "iio_buffer_update_demux" to copy the timestamp, the address
>> >> calculation
>> >is the root causes.
>> >>
>> >> 1083  in_loc += length;
>> >> ....
>> >> 1093  in_loc = roundup(in_loc, length);
>> >>
>> >> When finish to copy the channel data, in_loc is just incremented
>> >> and used as
>> >address of timestamp. This is correct only when the channel direct
>> >before timestamp is enabled.
>> >>
>> >> If there is a gap between the last enabled channel and timestamp,
>> >> then iio
>> >core will copy the wrong data.
>> >>
>> >> We have a fix to this issue,
>> >>
>> >> 1093 in_loc = (indio_dev->scan_bytes / sizeof(int64_t) - 1) *
>> >> length;
>> >
>> >That looks correct, but I'm not seeing why the roundup above doesn't
>> >land us in the same place.  I'm not that keen on handling the
>> >timestamp even more differently to other channels.
>> >
>> >
>> >If we imagine an active scan with
>> >2-byte chanel we want, 2 2-byte channels we don't a 2-byte gap and an
>> >8-byte timestamp struct scan {
>> >    u16 wanted;
>> >    u16 notwanted[2];
>> >    ... gap...
>> >    aligned_s64 timestamp;
>> >
>> >
>> >
>> >Cutting down to the parts that change in_loc only.
>> >
>> >    for_each_set_bit(out_ind, buffer->scan_mask, masklength) {
>> >            in_ind = find_next_bit(indio_dev->active_scan_mask,
>> >                                   masklength, in_ind + 1);
>> >            while (in_ind != out_ind) {
>> >... length is the length of current channel .. We never enter here in the
>example.
>> >                    /* Make sure we are aligned */
>> >                    in_loc = roundup(in_loc, length) + length;
>> >
>> >                    in_ind = find_next_bit(indio_dev->active_scan_mask,
>> >                                           masklength, in_ind + 1);
>> >            }
>> >
>> >... length is the length of the current channel.  Get here on first hit.
>> >
>> >            in_loc = roundup(in_loc, length);
>> >
>> >            in_loc += length;
>> >.. in loc = 2
>> >    }
>> >    /* Relies on scan_timestamp being last */
>> >    if (buffer->scan_timestamp) {
>> >
>> >... length is 8 ...
>> >
>> >            in_loc = roundup(in_loc, length);
>> >.. I think in_lock = 8?
>> >            ret = iio_buffer_add_demux(buffer, &p, in_loc, out_loc, length);
>> >
>> >    }
>> >
>> >Perhaps you can talk through the example that is failing?
>> >
>> >>
>> >> just not sure, if there will be any side-effects with this fix.
>> >>
>> >> Are you going to fix this finding, or shall we create a new patch for that?
>> >
>> >Fine to send the proposed fix but first we need to step through why
>> >the current code isn't working.
>> >
>> >
>> >Thanks,
>> >
>> >Jonathan
>> >
>> >>
>> >> Best regards
>> >> Jianping Shen
>> >>
>> >>
>> >> >>
>> >> >> >>
>> >> >> >> >> +
>> >> >> >> >> +static irqreturn_t smi330_trigger_handler(int irq, void *p) {
>> >> >> >> >> +      struct iio_poll_func *pf = p;
>> >> >> >> >> +      struct iio_dev *indio_dev = pf->indio_dev;
>> >> >> >> >> +      struct smi330_data *data = iio_priv(indio_dev);
>> >> >> >> >> +      int ret, chan;
>> >> >> >> >> +      int i = 0;
>> >> >> >> >> +
>> >> >> >> >> +      ret = regmap_bulk_read(data->regmap,
>> >> >SMI330_ACCEL_X_REG, data-
>> >> >> >> >>buf,
>> >> >> >> >> +                             ARRAY_SIZE(smi330_channels));
>> >> >> >> >> +      if (ret)
>> >> >> >> >> +              goto out;
>> >> >> >> >> +
>> >> >> >> >> +      if (*indio_dev->active_scan_mask !=
>> >> >> >> >> + SMI330_ALL_CHAN_MSK)
>> >> >{
>> >> >> >> >> +              iio_for_each_active_channel(indio_dev, chan)
>> >> >> >> >> +                      data->buf[i++] = data->buf[chan];
>> >> >> >> >
>> >> >> >> >If I follow this correctly you are reading all the channels
>> >> >> >> >and just copying out the ones you want.  Just let the IIO
>> >> >> >> >core do that for you by setting iio_dev-
>> >> >> >> >>available_scan_masks = {  SMI330_ALL_CHAN_MSK, 0 }; and
>> >> >> >> >>push the whole
>> >> >> >> >buffer every time.
>> >> >> >>
>> >> >> >> For the most frequent use cases, we define
>> >> >> >> available_scan_masks = {
>> >> >> >SMI330_ALL_CHAN_MSK, SMI330_ACC_XYZ_MSK,
>> >> >SMI330_GYRO_XYZ_MSK,
>> >> >> >0 }; and push the whole buffer every time.
>> >> >> >> From the user space we just enable 3 channels gyro_x, gyro_y,
>> >> >> >> and
>> >gyro_z.
>> >> >> >Then we enable buffer and expect that only the gyro values and
>> >> >> >timestamp in iio_buffer. Nevertheless, we have 3 accelerometer
>> >> >> >values and the timestamp in iio_buffer.
>> >> >> >
>> >> >> >> It seems that the iio core does not take care which channel
>> >> >> >> is enabled,  just
>> >> >> >copy the first 3 values (acc x,y,z) into iio_buffer.  Our
>> >> >> >driver code still needs to take care and just copy the enabled
>> >> >> >channel value to
>> >buffer.
>> >> >> >
>> >> >> >Look again at how it works.  If you provide ACC_XYZ_MSK, then
>> >> >> >your driver has to handle it.
>> >> >> >available_scan_masks is saying what your driver supports. The
>> >> >> >driver can check active_scan_mask to find out what is enabled.
>> >> >> >So right option here is only { SMI330_ALL_CHAN_MSK, 0, }  In
>> >> >> >that case the driver never needs to check as there is only one option.
>> >> >> >
>> >> >> >Then if any subset of channels is enabled the IIO core copy out
>> >> >> >just the data that is relevant.
>> >> >> >
>> >> >> >
>> >> >> >>
>> >> >> >> Another side effect after using available_scan_masks is that
>> >> >> >> the
>> >> >> >active_scan_masks sometimes does not reflect current channel
>> >> >> >activation status.
>> >> >> >>
>> >> >> >> Is some step missing to properly use available_scan_masks ?
>> >> >> >> How can a user
>> >> >> >find out from user space which channel combination is defined
>> >> >> >in available_scan_masks ?
>> >> >> >
>> >> >> >Why would userspace want to?  Userspace requested a subset of
>> >> >> >channels and it gets that subset.  So it if asks for the
>> >> >> >channels that make up SMI330_ACC_XYZ_MSK, if
>> >> >> >available_scan_mask == { SMI330_ALL_CHAN_MSK,
>> >> >> >0 } then the IIO core handling selects SMI330_ALL_CHAN_MSK
>> >> >> >(smallest available mask that is superset of what we asked for)
>> >> >> >and sets active_scan_mask to that.  The driver follows what
>> >> >> >active_scan_mask specifies and passes all channel data via the
>> >> >> >iio_push_to_buffers*() call. The demux in the IIO core than
>> >> >> >takes that 'scan' and repacks it so that userspace receives
>> >> >> >just the data it asked for formatting exactly as the driver
>> >> >> >would have done it if you had handled each channels
>> >> >separately in the driver.
>> >> >> >
>> >> >>
>> >> >> Set available_scan_masks = {  SMI330_ALL_CHAN_MSK, 0 } and push
>> >> >>the whole buffer. iio_push_to_buffers_with_timestamp (indio_dev,
>> >> >>data->buf, pf- timestamp); We enable the accX, accY, and accZ
>> >> >>data->from
>> >> >>userspace. And expect 3
>> >> >acc values and the timestamp in iio buffer.
>> >> >>
>> >> >> Raw iio buffer data:
>> >> >> 00000000: 3c00 d6ff 7510 0000 6100 f3ff 0000 0000  <...u...a.......
>> >> >            ACCX ACCY ACCZ PAD_ TIMESTAMP__________
>> >> >                               4093587712
>> >> >> 00000010: 3f00 d2ff 8910 0000 0300 f6ff 0000 0000  ?...............
>> >> >                               4143907584
>> >> >> 00000020: 4900 dcff 7a10 0000 caff 0100 0000 0000  I...z...........
>> >> >                               So this one looks bad.
>> >> >
>> >> >> 00000030: 4c00 d9ff 7910 0000 2f00 f8ff 0000 0000  L...y.../.......
>> >> >                               4177473280
>> >> >
>> >> >> 00000040: 4b00 d9ff 8410 0000 1f00 0800 0000 0000  K...............
>> >> >                               also bad.
>> >> >> 00000050: 4700 daff 7f10 0000 3b00 eeff 0000 0000  G.......;.......
>> >> >> 00000060: 3f00 d8ff 8410 0000 0c00 0900 0000 0000  ?...............
>> >> >> 00000070: 4600 d9ff 8010 0000 0e00 0800 0000 0000  F...............
>> >> >> 00000080: 4700 d7ff 7d10 0000 3400 feff 0000 0000  G...}...4.......
>> >> >> 00000090: 4b00 d4ff 8010 0000 3e00 1200 0000 0000  K.......>.......
>> >> >> 000000a0: 4600 d6ff 8d10 0000 4300 0000 0000 0000  F.......C.......
>> >> >> 000000b0: 4900 d6ff 7710 0000 2500 f0ff 0000 0000  I...w...%.......
>> >> >>
>> >> >> Converted value
>> >> >I guess this is different data as doesn't seem to line up with the above?
>> >> >
>> >> >> 0.015625 -0.009277 1.024411 589929
>> >> >> 0.015869 -0.009521 1.040769 4294901719
>> >> >> 0.020508 -0.008301 1.025632 458712
>> >> >> 0.018799 -0.006836 1.032956 851960
>> >> >> 0.019287 -0.009521 1.033201 4294836275
>> >> >> 0.015625 -0.010498 1.031003 4293328982
>> >> >> 0.015137 -0.010498 1.031980 4293853176
>> >> >> 0.015869 -0.009521 1.031492 4293722141
>> >> >> 0.018555 -0.011475 1.033445 4294311886
>> >> >>
>> >> >> The 3 acc values is correct in buffer.  Nevertheless, invalid
>> >> >> timestamp. The
>> >> >timestamp is actually the value of the gyroscope, which directly
>> >> >followed by acc values.
>> >> >> If we enable the gyroX, gyroY, and gyroZ from userspace, then
>> >> >> all the data is
>> >> >correct. Since the gyro values are the last 3 values and flowed by
>timestamp.
>> >> >
>> >> >Ok. That's odd and we should debug that.  This code is used in a
>> >> >lot of drivers so if it is not working correctly we need to figure
>> >> >out why asap and
>> >fix it.
>> >> >
>> >>
>> >>
>> >>
>> >>
>> >> >However, I'm not seeing what looks to be gyro data in bytes 8-15
>> >> >etc It isn't the stable sequence we'd expect for a timestamp
>> >> >though some specific values might be plausible.
>> >> >
>> >> >Looking again at the code, the IIO_DECLARE_BUFFER_WITH_TS() is the
>> >> >wrong size.  That should not include channel space for the timestamp.
>> >> >That should make it too big though which shouldn't be a problem.
>> >> >Also wrong type - should be using __le16 not s16 for the buffer
>> >> >elements given your channel declarations.
>> >> >
>> >> >Please could you add a print to your code alongside the
>> >> >iio_push_buffer_with_timestamp() to verify that the value in the
>> >> >pf-
>> >> >>timestamp is reasonable looking for a timestamp.
>> >> >
>> >> >For reference this is the code that handles the timestamp entry
>> >> >creation in the demux tables.
>> >> >https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2F
>> >>
>>eli%2F&data=05%7C02%7CJianping.Shen%40de.bosch.com%7C8010a2f19e
>554
>> >>
>>cacae2b08ddcdd7ad3e%7C0ae51e1907c84e4bbb6d648ee58410f4%7C0%7
>C0%7C6
>> >>
>>38893048385340779%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGki
>OnRydWUs
>> >>
>>IlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%
>> >>
>>3D%7C0%7C%7C%7C&sdata=517Jq7JnKUifmv8GPLa8Inc1HJLkKCSdu9c%2F
>einjQi
>> >> >o%3D&reserved=0
>> >>
>>
>>>xir.b%2F&data=05%7C02%7CJianping.Shen%40de.bosch.com%7Cd84c234
>1
>> >78ee4b
>> >>
>>
>>>ae6a2a08ddcac3e80a%7C0ae51e1907c84e4bbb6d648ee58410f4%7C0%7
>C
>> >0%7C63888
>> >>
>>
>>>9664948038159%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnR
>y
>> >dWUsIlYiOiI
>> >>
>>
>>>wLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7
>> >C0%7C%
>> >>
>>
>>>7C%7C&sdata=7kNAXwi9fkp5XocdJ2K5W2Cp9%2BQW4Wq6GLr5reGqies%
>3
>> >D&reserved
>> >> >=0
>> >>
>>
>>>ootlin.com%2Flinux%2Fv6.15.1%2Fsource%2Fdrivers%2Fiio%2Findustrialio
>> >>-
>> >>
>>
>>>buffer.c%23L1086&data=05%7C02%7CJianping.Shen%40de.bosch.com%7C
>f
>> >0
>> >>
>>
>>>9eaf03f8e44dd1e6fe08ddc53ae596%7C0ae51e1907c84e4bbb6d648ee58
>4
>> >1
>> >>
>>
>>>0f4%7C0%7C0%7C638883578931715207%7CUnknown%7CTWFpbGZsb3
>d
>> >8
>> >>
>>
>>>eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkF
>OI
>> >j
>> >>
>>
>>>oiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=s53tTw6o%2F2g
>u
>> >A
>> >> >iH3J9jBRd0%2Bj6UmcmgyhtBCuKK1HE0%3D&reserved=0
>> >> >
>> >> >Jonathan
>> >> >
>> >> >
>> >> >>
>> >> >> Conclusion: Setting available_scan_masks = {
>> >> >> SMI330_ALL_CHAN_MSK,
>> >> >> 0 },
>> >> >the iio core is able to correct handle the enabled channel data,
>> >> >but not the timestamp.
>> >> >> The working solution for now is that our driver takes care and
>> >> >> just copys the
>> >> >enabled channel value to buffer without using available_scan_masks.
>> >> >>
>> >> >> >So the aim is that userspace never knows anything about this.
>> >> >> >Just set what channels you want and get that data.
>> >> >> >
>> >> >> >Jonathan
>> >> >> >
>> >> >> >
>> >> >> >>
>> >> >> >> >
>> >> >> >> >The handling the core code is reasonably sophisticated and
>> >> >> >> >will use bulk copying where appropriate.
>> >> >> >> >
>> >> >> >> >If there is a strong reason to not use that, add a comment
>> >> >> >> >here so we don't have anyone 'fix' this code in future.
>> >> >> >> >
>> >> >> >> >> +      }
>> >> >> >> >> +
>> >> >> >> >> +      iio_push_to_buffers_with_timestamp(indio_dev,
>> >> >> >> >> + data->buf,
>> >> >> >> >> +pf->timestamp);
>> >> >> >> >> +
>> >> >> >> >> +out:
>> >> >> >> >> +      iio_trigger_notify_done(indio_dev->trig);
>> >> >> >> >> +
>> >> >> >> >> +      return IRQ_HANDLED; }
>> >> >> >>
>> >> >>
>> >>
>> >>
>>
>>