Re: [PATCH v7 2/2] iio: imu: smi240: add driver

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 13 Sep 2024 12:00:11 +0200
<Jianping.Shen@de.bosch.com> wrote:

> From: Shen Jianping <Jianping.Shen@de.bosch.com>
> 
> 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@de.bosch.com>

Hi Shen,

I suspect I led you astray.  regmap core seems unlikely to feed us
little endian buffers on writes (they should be CPU endian I think)
so there should be memcpy() for that not a get_unaligned_le16()

A few other minor comments inline that I missed before.
I'd probably have just tidied those up whilst applying if it wasn't
for the above question.

Jonathan


> diff --git a/drivers/iio/imu/smi240.c b/drivers/iio/imu/smi240.c
> new file mode 100644
> index 00000000000..892e14f3f60
> --- /dev/null
> +++ b/drivers/iio/imu/smi240.c
> @@ -0,0 +1,611 @@
> +// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
> +/*
> + * Copyright (c) 2024 Robert Bosch GmbH.
> + */
> +#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>
Conventionally this goes..
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/spi/spi.h>
> +#include <linux/units.h>

here.

> +
> +#include <asm/unaligned.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)
BIT() only

You aren't applying these to a kernel bitmap which is an array
of unsigned longs (hence these macros have % BITS_PER_LONG
which makes no sense here).

> +#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
BIT() as mentioned below for these as well.

> +
> +#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

BIT(20) ?
Same for other single bit masks.

> +#define SMI240_READ_DATA_MASK GENMASK(19, 4)
> +
> +/* T°C = (temp / 256) + 25 */
> +#define SMI240_TEMP_OFFSET 6400   // 25 * 256
> +#define SMI240_TEMP_SCALE 3906250 // (1 / 256) * 1e9
> +
> +#define SMI240_ACCEL_SCALE 500  // (1 / 2000) * 1e6
> +#define SMI240_GYRO_SCALE 10000 // (1 /  100) * 1e6


Even in this case, use traditional /* */ style comments.

> +

> +}
> +
> +static int smi240_regmap_spi_write(void *context, const void *data,
> +				   size_t count)
> +{
> +	u8 reg_addr;
> +	u16 reg_data;
> +	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);
> +
> +	if (count < 2)
> +		return -EINVAL;
> +
> +	reg_addr = ((u8 *)data)[0];
> +	reg_data = get_unaligned_le16(&((u8 *)data)[1]);

Why is the regmap core giving us an le16?
I probably sent you wrong way with this earlier :(
memcpy probably the correct choice here.


> +
> +	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));
> +}