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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 16 Sep 2024 20:32:56 +0000
"Shen Jianping (ME-SE/EAD2)" <Jianping.Shen@de.bosch.com> wrote:

> >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()
> >  
> >> +
> >> +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.  
> 
> Yes, you are right. We shall use memcpy to keep the be CPU endian.  Just using memcpy may be not enough.
> 
> Shall we also change regmap_config.val_format_endian  from REGMAP_ENDIAN_LITTLE to  REGMAP_ENDIAN_NATIVE ?
> 
> This is to make sure that regmap_write passes the reg-value to smi240_regmap_spi_write without changing the CPU endian.
>
Hmm. I'd missed that control.  If the register data needs to be little endian
then it is correct to leave that set as REGMAP_ENDIAN_LITTLE as then
the regmap core will do the endian swap for you on Big endian systems.

If I follow that bit of regmap correctly it will then have the data
in the right order so the above still wants to just be a memcpy.

As it stands, on a Big endian host, regmap will use the val_format_endian
to decide to flip the bytes.  This code then flips them back again and
the value written is big endian which is not what you intend!

Easy way to check this will be to set it, on your little endian
host, to REGMAP_BIG_ENDIAN and see what you get in the value.
Then consider if you'd had get_unaligned_be16
then it would end up as little endian again.  This should mirror
the current situation if this driver runs on a big endian host.

Hope that confusing set of comments helps!

Jonathan