Message-ID: <20250706175328.7207d847@jic23-huawei>
Date: Sun, 6 Jul 2025 17:53:28 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: <Jianping.Shen@...bosch.com>
Cc: <lars@...afoo.de>, <robh@...nel.org>, <krzk+dt@...nel.org>,
 <conor+dt@...nel.org>, <dima.fedrau@...il.com>,
 <marcelo.schmitt1@...il.com>, <linux-iio@...r.kernel.org>,
 <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <Christian.Lorenz3@...bosch.com>, <Ulrike.Frauendorf@...bosch.com>,
 <Kai.Dolde@...bosch.com>
Subject: Re: [PATCH v3 2/2] iio: imu: smi330: Add driver

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

> From: Jianping Shen <Jianping.Shen@...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@...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"

Powered by blists - more mailing lists