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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 15 Sep 2025 18:09:34 +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>

Applied to the testing branch of iio.git (which will be rebased on rc1)
with following diff;

diff --git a/drivers/iio/imu/smi330/Kconfig b/drivers/iio/imu/smi330/Kconfig
index 95b06f2317e8..915a5b6fb974 100644
--- a/drivers/iio/imu/smi330/Kconfig
+++ b/drivers/iio/imu/smi330/Kconfig
@@ -7,6 +7,7 @@ config SMI330
        tristate "Bosch Sensor SMI330 Inertial Measurement Unit"
        select IIO_BUFFER
        select IIO_TRIGGERED_BUFFER
+       depends on I2C || SPI
        help
          Enable support for the Bosch SMI330 IMU.
 
diff --git a/drivers/iio/imu/smi330/smi330_core.c b/drivers/iio/imu/smi330/smi330_core.c
index fdbc251d1f60..8571414d2a55 100644
--- a/drivers/iio/imu/smi330/smi330_core.c
+++ b/drivers/iio/imu/smi330/smi330_core.c
@@ -491,8 +491,6 @@ static int smi330_read_avail(struct iio_dev *indio_dev,
        default:
                return -EINVAL;
        }
-
-       return -EINVAL;
 }
 
 static int smi330_read_raw(struct iio_dev *indio_dev,

> 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
Why expose this as an option, whether or not either bus is supported?
At minimum this should be 
	depends on I2C || SPI

Often we just don't make this a selectable symbol though that
complicates the select statements here hence I've just made this tweak.

> +	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/smi330_core.c b/drivers/iio/imu/smi330/smi330_core.c
> new file mode 100644
> index 00000000000..fdbc251d1f6
> --- /dev/null
> +++ b/drivers/iio/imu/smi330/smi330_core.c

> +/* Register mask */
> +#define SMI330_CHIP_ID_MASK GENMASK(7, 0)
> +#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_ACC_GYR_DRDY_MASK GENMASK(13, 12)
> +#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_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_MAP2_ACC_DRDY_MASK GENMASK(11, 10)
> +#define SMI330_INT_MAP2_GYR_DRDY_MASK GENMASK(9, 8)
> +#define SMI330_INT_MAP2_DRDY_MASK \
> +	(SMI330_INT_MAP2_ACC_DRDY_MASK | SMI330_INT_MAP2_GYR_DRDY_MASK)

Trivial but I'm not sure this composite mask is useful. I would just
have used the two masks inline. I didn't bother modifying this though.



> +
> +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;
> +	}
> +
Unreachable code. I'm a bit surprised the compiler didn't moan about this.

> +	return -EINVAL;
> +}