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

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 28 Sep 2024 18:11:21 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Mon, 23 Sep 2024 14:40:17 +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>  
> At least one endian issue remaining ;(  
I'd unintentionally left this on my tree after the build issue.
Dropped now from the testing branch of iio.git.

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