linux-kernel - Re: [PATCH v4 2/2] iio: imu: smi240: imu driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20240828144333.0000386b@Huawei.com>
Date: Wed, 28 Aug 2024 14:43:33 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: <Jianping.Shen@...bosch.com>
CC: <jic23@...nel.org>, <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 v4 2/2] iio: imu: smi240: imu driver

On Mon, 26 Aug 2024 16:05:45 +0200
<Jianping.Shen@...bosch.com> wrote:

> From: Shen Jianping <Jianping.Shen@...bosch.com>
> 
> This patch adds 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.
> Smi240 does not support interrupt. 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@...bosch.com>
As per the other review, I'd really prefer this as one simple file.
I know you want to keep it looking like other IMUs, but it is a much
simpler devices, so the complexity they need is not appropriate here.

A few things inline,

Jonathan

> diff --git a/drivers/iio/imu/smi240/smi240_core.c b/drivers/iio/imu/smi240/smi240_core.c
> new file mode 100644
> index 00000000000..fc0226af843
> --- /dev/null
> +++ b/drivers/iio/imu/smi240/smi240_core.c
> @@ -0,0 +1,385 @@
...

> +#define SMI240_DATA_CHANNEL(_type, _axis, _index)              \
> +	{                                                          \
> +		.type = _type,                                         \
> +		.modified = 1,                                         \
> +		.channel2 = IIO_MOD_##_axis,                           \
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),          \

No scaling?  I'd expect the offset + scale for an accelerometer or gyro
channel to be well defined in the datasheet.


> +		.info_mask_shared_by_type =                            \
> +			BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),  \
> +		.info_mask_shared_by_type_available =                  \
> +			BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),  \


> diff --git a/drivers/iio/imu/smi240/smi240_spi.c b/drivers/iio/imu/smi240/smi240_spi.c
> new file mode 100644
> index 00000000000..d308f6407da
> --- /dev/null
> +++ b/drivers/iio/imu/smi240/smi240_spi.c
> @@ -0,0 +1,172 @@
> +// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
> +/*
> + * Copyright (c) 2024 Robert Bosch GmbH.
> + */
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/device.h>
> +#include <linux/regmap.h>
> +#include <linux/spi/spi.h>
> +#include <linux/bitfield.h>
> +#include <linux/iio/iio.h>
> +#include <asm/unaligned.h>

Alphabetical order preferred.


> +static int smi240_regmap_spi_read(void *context, const void *reg_buf,
> +				  size_t reg_size, void *val_buf,
> +				  size_t val_size)
> +{
> +	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 = SMI240_BUS_ID << 30;
FIELD_PREP for this field as well (and define the mask etc).

> +	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);
> +	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]);
> +
> +	request = SMI240_BUS_ID << 30;

FIELD_PREP() for this one as well with appropriate mask.

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