On Tue, 17 Sep 2024 13:13:57 +0000
"Shen Jianping (ME-SE/EAD2)" <Jianping.Shen@xxxxxxxxxxxx> 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
> >
> Hi Jonathan,
>
> we check the regmap behavior with the following tests
>
> 1. host : little endian val_format_endian ==REGMAP_BIG_ENDIAN regmap_write(data->regmap, REG_ADDR, 0x12AB)
> then in smi240_regmap_spi_write(void *context, const void *data, size_t count) we have data[0]== REG_ADDR, data[1]==0xAB , data[2]==0x12
>
> 2. host : little endian val_format_endian ==REGMAP_BIG_LITTLE regmap_write(data->regmap, REG_ADDR, 0x12AB)
> then in smi240_regmap_spi_write(void *context, const void *data, size_t count) we have data[0]== REG_ADDR, data[1]==0x12 , data[2]==0xAB
>
> 3. host : little endian val_format_endian ==REGMAP_BIG_NATIVE regmap_write(data->regmap, REG_ADDR, 0x12AB)
> then in smi240_regmap_spi_write(void *context, const void *data, size_t count) we have data[0]== REG_ADDR, data[1]==0x12 , data[2]==0xAB
>
> when regmap_write passes the reg-value to the underlaying spi-write function, it flips the bytes if val_format_endian != cpu_endian
>
> Since we prepare the request and the reg-value (as part of the request) with cpu_endian, we need to make sure that the cpu_endian keeps untouched until we pass the whole request to spi buffer using "iio_priv_data->spi_buf = cpu_to_be32(request)"
> Therefore we need to remove the change of cpu_endian during the request preparation.
>
> 1. Instead get_unaligned_le16 now we use memcpy to take the reg-value without changing the cpu_endian.
> 2, we use REGMAP_BIG_NATIVE on val_format_endian to make sure when regmap_write passes the reg-value to the underlaying spi-write function the cpu_endian kept untouched.
>
> This makes our driver be able to work properly on both little endian and big endian host. We tested the new changes on little endian host it works properly. Big endian host case is not tested yet, since today the big endian processors are almost dead.
>
> The next version will looks like that.
>
> static const struct regmap_config smi240_regmap_config = {
> .reg_bits = 8,
> .val_bits = 16,
> .val_format_endian = REGMAP_ENDIAN_NATIVE,
You say REGMAP_BIG_NATIVE above by which I think you mean REGMAP_ENDIAN_BIG
(which seems correct to me) but then set REGMAP_ENDIAN_NATIVE here.
Other than that this looks correct and thanks for working through
the test cases above.
>
> };
>
> 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];
> memcpy(®_data, &((u8 *)data)[1], 2);
>
> 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));
> }
>
> Is the new version now correct ?
>
> Best regards
>
> Jianping Shen
>
>
>
>
>
>
>
>
>
>
