drivers/sensor/st: Fix wrong data byte swap for be

[avisconti]

A 16-bit value built using byte shifts and ORs from a given
couple of lsb and msb bytes will result to be the same on both
little-endian and big-endian architectures, e.g.

uint8_t lsb, msb;
int16_t val;

/* val is the same number on both le and be archs, but has
   different layout in memory */
val = (msb << 8) | lsb;

All the xyz_raw_get() APIs of stmemsc sensor module build the sensor
data using the above method and DO NOT hence require (it actually leads
to wrong values on big-endian machines) to use any le/be swap routines,
such as sys_le16_to_cpu().

Fix #75758

[yperess]

I'm a little confused, it's fine if we want access to the raw numbers, but the get function should convert things to the system endianess.

[avisconti]

I'm a little confused, it's fine if we want access to the raw numbers, but the get function should convert things to the system endianess.

@yperess
I removed all the calls to sys_le16_to_cpu() when the data is already in the right system endianness. And this happens when the int16_t or int32_t are constructed from lsb/msb bytes with the shift operands. In some cases the constructions happens inside the raw_get() APIs, e.g. :

        iis3dhhc_acceleration_raw_get(data->ctx, raw_accel);
-       data->acc[0] = sys_le16_to_cpu(raw_accel[0]);
-       data->acc[1] = sys_le16_to_cpu(raw_accel[1]);
-       data->acc[2] = sys_le16_to_cpu(raw_accel[2]);
+       data->acc[0] = raw_accel[0];
+       data->acc[1] = raw_accel[1];
+       data->acc[2] = raw_accel[2];

with

int32_t iis3dhhc_acceleration_raw_get(const stmdev_ctx_t *ctx, int16_t *val)                                                                        
{
  uint8_t buff[6];
  int32_t ret; 

  ret = iis3dhhc_read_reg(ctx, IIS3DHHC_OUT_X_L_XL, buff, 6);
  val[0] = (int16_t)buff[1];
  val[0] = (val[0] * 256) + (int16_t)buff[0];
  val[1] = (int16_t)buff[3];
  val[1] = (val[1] * 256) + (int16_t)buff[2];
  val[2] = (int16_t)buff[5];
  val[2] = (val[2] * 256) + (int16_t)buff[4];

  return ret; 
}

If we do not remove the sys_le16_to_cpu() call, then in the big endian case the data will be swapped resulting in lsb/msb in the wrong place.
Let me know if it makes sense to you.

[avisconti]

@yperess still any doubt?

[avisconti]

Any feedback on this fix?

[avisconti]

@kartben do you possibly have any feedback on this fix? Thanks

[kartben]

@avisconti sorry, I am not really an expert on that stuff and was hoping maybe other sensor maintainers/collaborators could chime in to confirm it's the right approach. Thanks!

[avisconti]

Even if big endian architectures are not so frequent I would like to see whether this approach is correct or not. Who can be added to the list of reviewers? @fabiobaltieri Fabio, do you know if there is anyone with some expertise on endianity? Thanks

[ubieda]

If I understand this correctly: we're reverting these BE/LE omitting these sys_le16_to_cpu conversions because the ST HAL is already taking care of it.

May we capture that in the drivers code? Personally, I wouldn't have inferred it, had I not looked at the GH Issue.

[avisconti]

If I understand this correctly: we're reverting these BE/LE omitting these sys_le16_to_cpu conversions because the ST HAL is already taking care of it.

Yes, correct.

May we capture that in the drivers code? Personally, I wouldn't have inferred it, had I not looked at the GH Issue.

Do you mean if we can reproduce it on a big endian system? If so, unfortunately I do not have access to such h/w setup.
And yes, the endianness it is always confusing me, but I think that in this case the issue is really there.

[MaureenHelm]

The code changes look fine. It's clear that the byteswapping helpers aren't needed when the byte manipulation is done in the zephyr driver; what's a little less clear is when the ST HAL handles it under the hood. It would get repetitive to comment in every driver, so could you at least mention it in the commit message?

[ubieda]

May we capture that in the drivers code? Personally, I wouldn't have inferred it, had I not looked at the GH Issue.

Do you mean if we can reproduce it on a big endian system? If so, unfortunately I do not have access to such h/w setup.

Sorry - I just meant if we could drop a comment explaining. Just what Maureen said.

[ubieda]

As a side-note: A few months back I was able to compile/run big-endian on Linux with QEMU (using qemu-mips and mips-linux-gnu-gcc).

[avisconti]

As a side-note: A few months back I was able to compile/run big-endian on Linux with QEMU (using qemu-mips and mips-linux-gnu-gcc).

Yeah, I was looking into something like that in fact, just to make some compile tests and clear my mind against any possible doubt. But I think that theory is pretty clear. So I will re-elaborate the commit message adding some more info as requested by Maureen and you.

[avisconti]

Added a better commit header and copied here on this PR as well.

[avisconti]

repushed just to give a kick to ci to run again