linux-kernel - Re: [PATCH v8 2/2] iio: imu: smi240: add 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] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240928181121.0e62f0ad@jic23-huawei>
Date: Sat, 28 Sep 2024 18:11:21 +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 v8 2/2] iio: imu: smi240: add driver

On Mon, 23 Sep 2024 14:40:17 +0200
<Jianping.Shen@...bosch.com> wrote:

> From: Shen Jianping <Jianping.Shen@...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@...bosch.com>
At least one endian issue remaining ;(  

Make sure you run at least C=1 builds before sending patches to the list
  CHECK   drivers/iio/imu/smi240.c
drivers/iio/imu/smi240.c:223:14: warning: incorrect type in assignment (different base types)
drivers/iio/imu/smi240.c:223:14:    expected unsigned short [usertype]
drivers/iio/imu/smi240.c:223:14:    got restricted __le16 [usertype]


> +
> +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;
> +	u16 *val = val_buf;
> +	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 (reg_size != 1 || val_size != 2)
> +		return -EINVAL;
> +
> +	request = FIELD_PREP(SMI240_WRITE_BUS_ID_MASK, SMI240_BUS_ID);
> +	request |= FIELD_PREP(SMI240_WRITE_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;
> +
> +	*val = cpu_to_le16(FIELD_GET(SMI240_READ_DATA_MASK, response));
So this is line sparse doesn't like which is reasonable given you are forcing
an le16 value into a u16. 
Minimal fix is just to change type of val to __le16 *

I still find the endian handling in here mess and am not convinced
the complexity is strictly necessary or correct.

I'd expect the requirements of reordering to be same in read and write
directions (unless device is really crazy), so why do we need
a conversion to le16 here but not one from le16 in the write?


> +
> +	return 0;
> +}
> +
> +static int smi240_regmap_spi_write(void *context, const void *data,
> +				   size_t count)
> +{
> +	u8 reg_addr;
> +	u16 reg_data;
> +	u32 request;
> +	const u8 *data_ptr = data;
> +	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 = data_ptr[0];
> +	memcpy(&reg_data, &data_ptr[1], sizeof(reg_data));
> +
> +	request = FIELD_PREP(SMI240_WRITE_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);

This is built as fields in a native 32 bit register.
My gut feeling is that you don't want the REGMAP_ENDIAN_LITTLE but
rather use REGMAP_ENDIAN_NATIVE.

The explicit reorder to be32 is fine though as that is just
switching from the this native endian value to the byte ordering on
the bus.

> +	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 const struct regmap_bus smi240_regmap_bus = {
> +	.read = smi240_regmap_spi_read,
> +	.write = smi240_regmap_spi_write,
> +};
> +
> +static const struct regmap_config smi240_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +	.val_format_endian = REGMAP_ENDIAN_LITTLE,
> +};