Re: [PATCH 1/3] iio: common: st_sensors: honour channel endianness in read_axis_data
From: me
Date: Mon Jun 15 2026 - 08:49:19 EST
On 2026-06-05 19:47, Andy Shevchenko wrote:
On Fri, Jun 05, 2026 at 12:08:41PM +0200, Herman van Hazendonk wrote:Good points, both applied as you suggested in v2. Single u32 tmp captures
st_sensors_read_axis_data() unconditionally decoded multi-byte
results with get_unaligned_le16() / get_unaligned_le24() regardless
of the channel's declared scan_type.endianness.
For every ST sensor that has used this helper since it was introduced
this happened to be fine because the ST IMU/accel/gyro/pressure
families publish their data registers as little-endian and the
channel specs in those drivers declare IIO_LE accordingly.
The LSM303DLH magnetometer however publishes its X/Y/Z output as a
pair of big-endian bytes (the H register sits at the lower address,
0x03/0x05/0x07, and the L register immediately after), and its
channel specs in st_magn_core.c correctly declare IIO_BE -- but
read_axis_data() ignored that and decoded as little-endian, swapping
the high and low bytes of every magnetometer sample.
The bug is most visible on a stationary chip: in earth's field the
true X reading is small and the high byte sits at 0x00, so swapping
the bytes pins sysfs X at exactly the low byte's pattern (e.g. 0x00F0
= 240). Y and Z still appear "to vary" because their magnitudes are
larger and the noise in the low byte produces big swings in the
swapped high byte:
before (chip flat, sysfs in_magn_*_raw):
X=240 (stuck), Y= 12032..23296, Z=-16128..-9728
after (direct i2c-dev big-endian decode, same chip same orientation):
X≈-4096, Y≈210, Z≈80 (sensible values reflecting earth's
ambient field at low gauss range)
Fix read_axis_data() to dispatch on ch->scan_type.endianness and
call get_unaligned_be16() / get_unaligned_be24() when the channel
declares IIO_BE. Existing IIO_LE consumers (st_accel, st_gyro,
st_pressure, st_lsm6dsx and others) are unaffected because their
channel specs already declare IIO_LE and the LE path is unchanged.
...
- if (byte_for_channel == 1)
+ if (byte_for_channel == 1) {
*data = (s8)*outdata;
Maybe for the consistency's sake use sign_extend32() everywhere?
+ } else if (byte_for_channel == 2) {
+ if (ch->scan_type.endianness == IIO_BE)
+ *data = (s16)get_unaligned_be16(outdata);
+ else
+ *data = (s16)get_unaligned_le16(outdata);
+ } else if (byte_for_channel == 3) {
+ if (ch->scan_type.endianness == IIO_BE)
+ *data = (s32)sign_extend32(get_unaligned_be24(outdata),
+ 23);
+ else
+ *data = (s32)sign_extend32(get_unaligned_le24(outdata),
+ 23);
Why do you need casting here? sign_extend32() should return signed type.
With this being addressed, you can make them one-liners.
+ }
u32 tmp;
...
if (byte_for_channel == 1) {
// this way is done to show the below variant
tmp = *outdata;
*data = sign_extend32(tmp, 7);
} else if (byte_for_channel == 2) {
if (ch->scan_type.endianness == IIO_BE)
tmp = get_unaligned_be16(outdata);
else
tmp = get_unaligned_le16(outdata);
*data = sign_extend32(tmp, 15);
} else if (byte_for_channel == 3) {
if (ch->scan_type.endianness == IIO_BE)
tmp = get_unaligned_be24(outdata);
else
tmp = get_unaligned_le24(outdata);
*data = sign_extend32(tmp, 23);
}
Or even
if (byte_for_channel == 1) {
tmp = *outdata;
} else if (byte_for_channel == 2) {
if (ch->scan_type.endianness == IIO_BE)
tmp = get_unaligned_be16(outdata);
else
tmp = get_unaligned_le16(outdata);
} else if (byte_for_channel == 3) {
if (ch->scan_type.endianness == IIO_BE)
tmp = get_unaligned_be24(outdata);
else
tmp = get_unaligned_le24(outdata);
} else {
...error...
}
*data = sign_extend32(tmp, BYTES_TO_BITS(byte_for_channel) - 1);
the per-width decode, then one sign_extend32(tmp, BYTES_TO_BITS(byte_for_channel)
- 1) at the bottom handles all three widths. Casts are gone. I also turned
the previously-silent 0/≥4 fall-through into an explicit return -EINVAL so
a malformed channel spec is caught instead of leaving *data uninitialis
Thanks,
Herman