Re: [PATCH v5 1/3] hwmon: (ads7871) Fix endianness bug in 16-bit register reads

[Guenter Roeck]

On 5/1/26 01:49, David Laight wrote:
> On Fri,  1 May 2026 08:05:28 +0530
> Tabrez Ahmed <tabreztalks@xxxxxxxxx> wrote:
>
> > The ads7871_read_reg16() function relies on spi_w8r16() to read the
> > 16-bit sensor output. The ADS7871 device transmits the Least Significant
> > Byte (LSB) first.
> >
> > On Little-Endian architectures, spi_w8r16() correctly reconstructs the
> > 16-bit value. However, on Big-Endian architectures, the byte swapping
> > causes the first received byte (LSB) to be placed in the most significant
> > byte of the u16, resulting in corrupted voltage readings.
> >
> > Replace spi_w8r16() with a manual spi_write_then_read() into a byte array,
> > and safely reconstruct the integer using get_unaligned_le16() to ensure
> > correct behavior across all architectures. Additionally, use a u8
> > variable for the command byte to ensure the correct instruction is
> > transmitted on Big-Endian systems.
> >
> > Reported-by: Sashiko <sashiko-bot@xxxxxxxxxx>
> > Closes: https://sashiko.dev/#/patchset/20260418034601.90226-1-tabreztalks@xxxxxxxxx
> > Signed-off-by: Tabrez Ahmed <tabreztalks@xxxxxxxxx>
> > ---
> >   drivers/hwmon/ads7871.c | 14 +++++++++++---
> >   1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/hwmon/ads7871.c b/drivers/hwmon/ads7871.c
> > index 9bfdf9e6bcd77..d77eff430935b 100644
> > --- a/drivers/hwmon/ads7871.c
> > +++ b/drivers/hwmon/ads7871.c
> > @@ -59,9 +59,9 @@
> >   #include <linux/hwmon-sysfs.h>
> >   #include <linux/err.h>
> >   #include <linux/delay.h>
> > +#include <linux/unaligned.h>
> >   
> >   #define DEVICE_NAME	"ads7871"
> > -
> >   struct ads7871_data {
> >   	struct spi_device *spi;
> >   };
> > @@ -77,9 +77,17 @@ static int ads7871_read_reg8(struct spi_device *spi, int reg)
> >   static int ads7871_read_reg16(struct spi_device *spi, int reg)
> >   {
> >   	int ret;
> > +	u8 tx_cmd;
> > +	u8 rx_buf[2];
> > +
> >   	reg = reg | INST_READ_BM | INST_16BIT_BM;
> > -	ret = spi_w8r16(spi, reg);
> > -	return ret;
>
> Isn't it enough to just byteswap the result? so:
> 	return le16toh(ret);
> The whole thing can be:
> 	return le16toh(spi_w8r16(spi, reg | INST_READ_BM | INST_16BIT_BM));

This would convert error codes to some random value.

Guenter