On Wed, 4 Sep 2024 16:05:06 +0200 <Jianping.Shen@xxxxxxxxxxxx> wrote: > From: Shen Jianping <Jianping.Shen@xxxxxxxxxxxx> > > Add the iio driver for bosch imu smi240. The smi240 is a combined > three axis angular rate and three axis acceleration sensor module > with a measurement range of +/-300°/s and up to 16g. A synchronous > acc and gyro sampling can be triggered by setting the capture bit > in spi read command. > > Implemented features: > * raw data access for each axis through sysfs > * tiggered buffer for continuous sampling > * synchronous acc and gyro data from tiggered buffer > > Signed-off-by: Shen Jianping <Jianping.Shen@xxxxxxxxxxxx> Hi Shen, A few additional comments inline, thanks, Jonathan > --- > drivers/iio/imu/Kconfig | 14 + > drivers/iio/imu/Makefile | 2 + > drivers/iio/imu/smi240.c | 578 +++++++++++++++++++++++++++++++++++++++ > 3 files changed, 594 insertions(+) > create mode 100644 drivers/iio/imu/smi240.c > > diff --git a/drivers/iio/imu/smi240.c b/drivers/iio/imu/smi240.c > new file mode 100644 > index 00000000000..e69d81545eb > --- /dev/null > +++ b/drivers/iio/imu/smi240.c > @@ -0,0 +1,578 @@ > +// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0 > +/* > + * Copyright (c) 2024 Robert Bosch GmbH. > + */ > +#include <asm/unaligned.h> Convention is to list asm includes (as more specific in some sense) as a separate block after the linux ones. > +#include <linux/bitfield.h> > +#include <linux/bits.h> > +#include <linux/delay.h> > +#include <linux/device.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> Also common to pull subsystem specific headers out in their own block but I don't insist on this. > +#include <linux/module.h> > +#include <linux/regmap.h> > +#include <linux/spi/spi.h> > +#include <linux/units.h> > + > +#define SMI240_CHIP_ID 0x0024 > + > +#define SMI240_SOFT_CONFIG_EOC_MASK BIT_MASK(0) > +#define SMI240_SOFT_CONFIG_GYR_BW_MASK BIT_MASK(1) > +#define SMI240_SOFT_CONFIG_ACC_BW_MASK BIT_MASK(2) > +#define SMI240_SOFT_CONFIG_BITE_AUTO_MASK BIT_MASK(3) > +#define SMI240_SOFT_CONFIG_BITE_REP_MASK GENMASK(6, 4) > + > +#define SMI240_CHIP_ID_REG 0x00 > +#define SMI240_SOFT_CONFIG_REG 0x0A > +#define SMI240_TEMP_CUR_REG 0x10 > +#define SMI240_ACCEL_X_CUR_REG 0x11 > +#define SMI240_GYRO_X_CUR_REG 0x14 > +#define SMI240_DATA_CAP_FIRST_REG 0x17 > +#define SMI240_CMD_REG 0x2F > + > +#define SMI240_SOFT_RESET_CMD 0xB6 > + > +#define SMI240_BITE_SEQUENCE_DELAY_US 140000 > +#define SMI240_FILTER_FLUSH_DELAY_US 60000 > +#define SMI240_DIGITAL_STARTUP_DELAY_US 120000 > +#define SMI240_MECH_STARTUP_DELAY_US 100000 > + > +#define SMI240_CRC_INIT 0x05 > +#define SMI240_CRC_POLY 0x0B > +#define SMI240_BUS_ID 0x00 > + > +#define SMI240_SD_BIT_MASK 0x80000000 > +#define SMI240_CS_BIT_MASK 0x00000008 > + > +#define SMI240_BUS_ID_MASK GENMASK(31, 30) > +#define SMI240_WRITE_ADDR_MASK GENMASK(29, 22) > +#define SMI240_WRITE_BIT_MASK 0x00200000 > +#define SMI240_WRITE_DATA_MASK GENMASK(18, 3) > +#define SMI240_CAP_BIT_MASK 0x00100000 > +#define SMI240_READ_DATA_MASK GENMASK(19, 4) > + > +/* > + * T°C = (temp / 256) + 25 > + * Tm°C = 1000 * ((temp * 100 / 25600) + 25) > + * scale: 100000 / 25600 = 3.906250 > + * offset: 25000 > + */ > +#define SMI240_TEMP_OFFSET 25000 > +#define SMI240_TEMP_SCALE 3906250 > + > +#define SMI240_LOW_BANDWIDTH_HZ 50 > +#define SMI240_HIGH_BANDWIDTH_HZ 400 > + > +#define SMI240_ACCEL_SCALE 2000 > +#define SMI240_GYRO_SCALE 100 > + > +#define SMI240_DATA_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_LOW_PASS_FILTER_3DB_FREQUENCY), \ > + .info_mask_shared_by_type_available = \ > + BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY), \ > + .scan_index = _index, \ > + .scan_type = { \ > + .sign = 's', \ > + .realbits = 16, \ > + .storagebits = 16, \ > + .endianness = IIO_LE, \ Is it little endian? There seems to be a be32_to_cpu() in the spi read that suggests it might be CPU endian? > + }, \ > +} > + > +#define SMI240_TEMP_CHANNEL(_index) { \ > + .type = IIO_TEMP, \ > + .modified = 1, \ > + .channel2 = IIO_MOD_TEMP_OBJECT, \ > + .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, \ > + }, \ > +} > + > +enum capture_mode { SMI240_CAPTURE_OFF = 0, SMI240_CAPTURE_ON = 1 }; > + > +struct smi240_data { > + struct regmap *regmap; > + u16 accel_filter_freq; > + u16 anglvel_filter_freq; > + u8 bite_reps; > + enum capture_mode capture; > + /* > + * Ensure natural alignment for timestamp if present. > + * Channel size: 2 bytes. > + * Max length needed: 2 * 3 channels + temp channel + 2 bytes padding + 8 byte ts. > + * If fewer channels are enabled, less space may be needed, as > + * long as the timestamp is still aligned to 8 bytes. > + */ > + s16 buf[12] __aligned(8); > + > + __be32 spi_buf __aligned(IIO_DMA_MINALIGN); > +}; > + > +static const int smi240_low_pass_freqs[] = { SMI240_LOW_BANDWIDTH_HZ, > + SMI240_HIGH_BANDWIDTH_HZ }; > + > +static int smi240_regmap_spi_read(void *context, const void *reg_buf, > + size_t reg_size, void *val_buf, > + size_t val_size) Could simplify this with the fact we know val_size. + sanity check it is indeed what we expect. Given that is known can use a local variable u16 *val = val_buf; and then assign without the memcpy fun below. > +{ > + int ret; > + u32 request, response; > + struct spi_device *spi = context; > + struct iio_dev *indio_dev = dev_get_drvdata(&spi->dev); > + struct smi240_data *iio_priv_data = iio_priv(indio_dev); > + > + request = FIELD_PREP(SMI240_BUS_ID_MASK, SMI240_BUS_ID); > + request |= FIELD_PREP(SMI240_CAP_BIT_MASK, iio_priv_data->capture); > + request |= FIELD_PREP(SMI240_WRITE_ADDR_MASK, *(u8 *)reg_buf); > + request |= smi240_crc3(request, SMI240_CRC_INIT, SMI240_CRC_POLY); > + > + iio_priv_data->spi_buf = cpu_to_be32(request); > + > + /* > + * SMI240 module consists of a 32Bit Out Of Frame (OOF) > + * SPI protocol, where the slave interface responds to > + * the Master request in the next frame. > + * CS signal must toggle (> 700 ns) between the frames. > + */ > + ret = spi_write(spi, &iio_priv_data->spi_buf, sizeof(request)); > + if (ret) > + return ret; > + > + ret = spi_read(spi, &iio_priv_data->spi_buf, sizeof(response)); > + if (ret) > + return ret; > + > + response = be32_to_cpu(iio_priv_data->spi_buf); > + > + if (!smi240_sensor_data_is_valid(response)) > + return -EIO; > + > + response = FIELD_GET(SMI240_READ_DATA_MASK, response); This is reusing response for something different. I'd use a separately local variable for this. > + memcpy(val_buf, &response, val_size); > + > + return 0; > +} > + > +static int smi240_regmap_spi_write(void *context, const void *data, > + size_t count) > +{ > + u32 request; > + struct spi_device *spi = context; > + struct iio_dev *indio_dev = dev_get_drvdata(&spi->dev); > + struct smi240_data *iio_priv_data = iio_priv(indio_dev); > + u8 reg_addr = ((u8 *)data)[0]; > + u16 reg_data = get_unaligned_le16(&((u8 *)data)[1]); Should probably harden code by checking count for what is supported. Might catch future bugs. > + > + request = FIELD_PREP(SMI240_BUS_ID_MASK, SMI240_BUS_ID); > + request |= FIELD_PREP(SMI240_WRITE_BIT_MASK, 1); > + request |= FIELD_PREP(SMI240_WRITE_ADDR_MASK, reg_addr); > + request |= FIELD_PREP(SMI240_WRITE_DATA_MASK, reg_data); > + request |= smi240_crc3(request, SMI240_CRC_INIT, SMI240_CRC_POLY); > + > + iio_priv_data->spi_buf = cpu_to_be32(request); > + > + return spi_write(spi, &iio_priv_data->spi_buf, sizeof(request)); > +} > +static int smi240_soft_config(struct smi240_data *data) > +{ > + int ret; > + u8 acc_bw, gyr_bw; > + u16 request; > + > + switch (data->accel_filter_freq) { > + case SMI240_LOW_BANDWIDTH_HZ: > + acc_bw = 0x1; > + break; > + case SMI240_HIGH_BANDWIDTH_HZ: > + acc_bw = 0x0; > + break; > + default: > + return -EINVAL; > + } > + > + switch (data->anglvel_filter_freq) { > + case SMI240_LOW_BANDWIDTH_HZ: > + gyr_bw = 0x1; > + break; > + case SMI240_HIGH_BANDWIDTH_HZ: > + gyr_bw = 0x0; > + break; > + default: > + return -EINVAL; > + } > + > + request = FIELD_PREP(SMI240_SOFT_CONFIG_EOC_MASK, 1); > + request |= FIELD_PREP(SMI240_SOFT_CONFIG_GYR_BW_MASK, gyr_bw); > + request |= FIELD_PREP(SMI240_SOFT_CONFIG_ACC_BW_MASK, acc_bw); > + request |= FIELD_PREP(SMI240_SOFT_CONFIG_BITE_AUTO_MASK, 1); > + request |= FIELD_PREP(SMI240_SOFT_CONFIG_BITE_REP_MASK, > + data->bite_reps - 1); > + > + ret = regmap_write(data->regmap, SMI240_SOFT_CONFIG_REG, request); > + if (ret) > + return ret; > + > + fsleep(SMI240_MECH_STARTUP_DELAY_US + > + data->bite_reps * SMI240_BITE_SEQUENCE_DELAY_US + bite? or byte? in both cases. > + SMI240_FILTER_FLUSH_DELAY_US); > + > + return 0; > +} > + > +static int smi240_get_low_pass_filter_freq(struct smi240_data *data, > + int chan_type, int *val) > +{ > + switch (chan_type) { > + case IIO_ACCEL: > + *val = data->accel_filter_freq; > + return 0; > + case IIO_ANGL_VEL: > + *val = data->anglvel_filter_freq; > + return 0; > + default: > + return -EINVAL; > + } > +} > + > +static int smi240_get_data(struct smi240_data *data, int chan_type, int axis, > + int *val) > +{ > + u8 reg; > + int ret, sample; > + > + if (chan_type == IIO_TEMP) Switch statement is easier to read. > + reg = SMI240_TEMP_CUR_REG; > + else if (chan_type == IIO_ACCEL) > + reg = SMI240_ACCEL_X_CUR_REG + (axis - IIO_MOD_X); > + else if (chan_type == IIO_ANGL_VEL) > + reg = SMI240_GYRO_X_CUR_REG + (axis - IIO_MOD_X); > + else > + return -EINVAL; > + > + ret = regmap_read(data->regmap, reg, &sample); > + if (ret) > + return ret; > + > + *val = sign_extend32(sample, 15); > + > + return 0; > +} > + > +static irqreturn_t smi240_trigger_handler(int irq, void *p) > +{ > + struct iio_poll_func *pf = p; > + struct iio_dev *indio_dev = pf->indio_dev; > + struct smi240_data *data = iio_priv(indio_dev); > + int ret, chan, i = 0; Don't mix declarations that assign with ones that don't on the same line. int ret, chan; int i = 0; is easier to read. This case was straight forward but with a long list and assignments for some elements, it can be easy to miss subtle bugs (though mostly the compiler will catch them). > + > + data->capture = SMI240_CAPTURE_ON; > + > + for_each_set_bit(chan, indio_dev->active_scan_mask, > + indio_dev->masklength) { Try building this on latest togreg branch of iio.git (or linux-next if that is easier for you). You an no longer access masklength directly like this. Use iio_for_each_active_channel() > + ret = regmap_read(data->regmap, > + SMI240_DATA_CAP_FIRST_REG + chan, > + (int *)&data->buf[i]); I'd put the i++ in there to make the point that it is moving on the indexing of the buffer. However, don't do this - use a local variable of the right type because regmap_read will write the full integer (so probably 32 bits) and so spill to later addresses which whilst not a bug (because of padding for the timestamp) is certainly ugly. int val; ret = regmap_read(data->regmap, SMI240_DATA_CAP_FIRST_REG + chan, &val); data->capture = SMI240_CAPTURE_OFF; if (ret) goto out; data->buf[i++] = val; > + data->capture = SMI240_CAPTURE_OFF; > + if (ret) > + goto out; > + i++; > + } > + > + 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 smi240_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int *val, > + int *val2, long mask) > +{ > + int ret; > + struct smi240_data *data = iio_priv(indio_dev); > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + if (iio_buffer_enabled(indio_dev)) This is racy as nothing stops the buffer being enabled immediately after you check if it alreadyis. We have iio_device_claim_direct_mode() and release() to close this race. > + return -EBUSY; > + ret = smi240_get_data(data, chan->type, chan->channel2, val); > + if (ret) > + return ret; > + return IIO_VAL_INT; > + > + case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: > + ret = smi240_get_low_pass_filter_freq(data, chan->type, val); > + if (ret) > + return ret; > + return IIO_VAL_INT; > + > + case IIO_CHAN_INFO_SCALE: > + if (chan->type == IIO_TEMP) { > + *val = SMI240_TEMP_SCALE / MEGA; > + *val2 = SMI240_TEMP_SCALE % MEGA; > + return IIO_VAL_INT_PLUS_MICRO; > + } else if (chan->type == IIO_ACCEL) { > + *val = SMI240_ACCEL_SCALE; > + return IIO_VAL_INT; > + } else if (chan->type == IIO_ANGL_VEL) { > + *val = SMI240_GYRO_SCALE; > + return IIO_VAL_INT; > + } else > + return -EINVAL; kernel coding style requires brackets for all branches if any of them need them for multiple lines. Fix throughout. Though in this case using a switch statement is probably cleaner. > + > + case IIO_CHAN_INFO_OFFSET: > + if (chan->type == IIO_TEMP) { > + *val = SMI240_TEMP_OFFSET; > + return IIO_VAL_INT; > + } else > + return -EINVAL; > + > + default: > + return -EINVAL; > + } > +} > + > +static int smi240_probe(struct spi_device *spi) > +{ > + struct device *dev; struct device *dev = &spi->dev; Usual convention is to combine declaration and assignment if the assignment is something the compiler can figure out trivially. Here it's just a fixed pointer offset. If there is a complex function, or one that can return an error code then assignment in the declaration block would not be a good idea. Sometimes long lines make it a bad idea as well. In this case it makes more sense to just do as suggested above. > + struct iio_dev *indio_dev; > + struct regmap *regmap; > + struct smi240_data *data; > + int ret, response; > + > + dev = &spi->dev; > +} > + > +static const struct spi_device_id smi240_spi_id[] = { { "smi240", 0 }, {} }; Format as: static const struct spi_device_id smi240_spi_id[] = { { "smi240" }, { } }; Dropping the unused 0 makes it obvious we don't care about the value yet. If support for additional devices is added in future then that value can be set appropriately. > +MODULE_DEVICE_TABLE(spi, smi240_spi_id); > + > +static const struct of_device_id smi240_of_match[] = { > + { .compatible = "bosch,smi240" }, > + {} Trivial but I'm standardizing IIO _id tables with { } Mostly so that long term I never have to give review comments on these. > +}; > +MODULE_DEVICE_TABLE(of, smi240_of_match);