[PATCH v3 0/3] hwmon: (ads7871) Modernize and fix DMA safety

[PATCH v3 0/3] hwmon: (ads7871) Modernize and fix DMA safety

[Tabrez Ahmed]

This series modernizes the ads7871 driver by migrating it to the
hwmon_device_register_with_info() API and moving the SPI transfer
buffer into the driver's private data structure to ensure it is
DMA-safe.

Changes in v3:
- Added Patch 1 to fix a pre-existing Endianness bug in the 16-bit
  register read logic, flagged by Sashiko AI during v2 review.
- Fixed multiple formatting and alignment issues caught by checkpatch
  --strict, as requested by Guenter.
- Added "While at it, fix checkpatch violations" to Patch 2 commit message.

Changes in v2:
- Dropped custom mutex in favor of native hwmon core serialization.
- Split API migration and DMA fix into separate, logical patches.
- Corrected output scaling and sign extension to meet hwmon ABI.

Tabrez Ahmed (3):
  hwmon: (ads7871) Fix endianness bug in 16-bit register reads
  hwmon: (ads7871) Convert to hwmon_device_register_with_info
  hwmon: (ads7871) Use DMA-safe buffer for SPI writes
Tabrez Ahmed (3):
  hwmon: (ads7871) Fix endianness bug in 16-bit register reads
  hwmon: (ads7871) Convert to hwmon_device_register_with_info
  hwmon: (ads7871) Use DMA-safe buffer for SPI writes

 drivers/hwmon/ads7871.c | 118 +++++++++++++++++++++++-----------------
 1 file changed, 68 insertions(+), 50 deletions(-)

-- 
2.43.0

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

[Tabrez Ahmed]

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.

Reported-by: Sashiko <sashiko@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260329073352.270451-1-tabreztalks%40gmail.com
Signed-off-by: Tabrez Ahmed <tabreztalks@gmail.com>
---
 drivers/hwmon/ads7871.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/ads7871.c b/drivers/hwmon/ads7871.c
index 9bfdf9e6bcd77..9b52aa496d522 100644
--- a/drivers/hwmon/ads7871.c
+++ b/drivers/hwmon/ads7871.c
@@ -59,6 +59,7 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/err.h>
 #include <linux/delay.h>
+#include <asm/unaligned.h>
 
 #define DEVICE_NAME	"ads7871"
 
@@ -77,9 +78,14 @@ 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 buf[2];
+
 	reg = reg | INST_READ_BM | INST_16BIT_BM;
-	ret = spi_w8r16(spi, reg);
-	return ret;
+	ret = spi_write_then_read(spi, &reg, 1, buf, 2);
+	if (ret < 0)
+		return ret;
+
+	return get_unaligned_le16(buf);
 }
 
 static int ads7871_write_reg8(struct spi_device *spi, int reg, u8 val)
-- 
2.43.0

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

[sashiko-bot]

Sashiko AI review found 1 potential issue(s):
- [High] Passing a pointer to an `int` variable (`&reg`) to `spi_write_then_read()` for a 1-byte transmission causes the wrong byte to be sent on Big-Endian architectures.
--

commit 4f7407427267b9ca42b213449235bc095cff57b3
Author: Tabrez Ahmed <tabreztalks@gmail.com>

hwmon: (ads7871) Fix endianness bug in 16-bit register reads

This commit replaces spi_w8r16() with a manual spi_write_then_read() into
a byte array to correctly handle 16-bit sensor reads. It then reconstructs
the integer using get_unaligned_le16() to ensure correct behavior across
all architectures.

Closes: https://sashiko.dev/#/patchset/20260329073352.270451-1-tabreztalks%40gmail.com

> diff --git a/drivers/hwmon/ads7871.c b/drivers/hwmon/ads7871.c
> index 9bfdf9e6bcd77..9b52aa496d522 100644
> --- a/drivers/hwmon/ads7871.c
> +++ b/drivers/hwmon/ads7871.c

[ ... ]

> @@ -77,9 +78,14 @@ 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 buf[2];
> +
>  	reg = reg | INST_READ_BM | INST_16BIT_BM;
> -	ret = spi_w8r16(spi, reg);
> -	return ret;
> +	ret = spi_write_then_read(spi, &reg, 1, buf, 2);

Will this transmit the wrong instruction byte on Big-Endian architectures?

The variable reg is a 32-bit int, and spi_write_then_read() reads
sequentially from the memory address provided. On Little-Endian
architectures, the lowest memory address correctly contains the least
significant byte, which is the intended 8-bit command.

However, on Big-Endian architectures, the lowest memory address holds the
most significant byte of the int, which is 0x00. Does this result in 0x00
being transmitted over the SPI bus instead of the correct register
instruction?

> +	if (ret < 0)
> +		return ret;
> +
> +	return get_unaligned_le16(buf);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260418034601.90226-1-tabreztalks@gmail.com?part=1

[PATCH v3 2/3] hwmon: (ads7871) Convert to hwmon_device_register_with_info

[Tabrez Ahmed]

Convert the ads7871 driver from the legacy hwmon_device_register() to the
modern hwmon_device_register_with_info() API. This migration simplifies
the driver by using the structured hwmon_channel_info approach and
prepares the codebase for the transition to a shared DMA-safe buffer.
While at it, fix checkpatch violations.

Signed-off-by: Tabrez Ahmed <tabreztalks@gmail.com>
---
 drivers/hwmon/ads7871.c | 75 ++++++++++++++++++++++-------------------
 1 file changed, 40 insertions(+), 35 deletions(-)

diff --git a/drivers/hwmon/ads7871.c b/drivers/hwmon/ads7871.c
index 9b52aa496d522..851b70e9f6ba2 100644
--- a/drivers/hwmon/ads7871.c
+++ b/drivers/hwmon/ads7871.c
@@ -56,7 +56,6 @@
 #include <linux/init.h>
 #include <linux/spi/spi.h>
 #include <linux/hwmon.h>
-#include <linux/hwmon-sysfs.h>
 #include <linux/err.h>
 #include <linux/delay.h>
 #include <asm/unaligned.h>
@@ -67,6 +66,16 @@ struct ads7871_data {
 	struct spi_device *spi;
 };
 
+static umode_t ads7871_is_visible(const void *data,
+				  enum hwmon_sensor_types type,
+				  u32 attr, int channel)
+{
+	if (type == hwmon_in && attr == hwmon_in_input)
+		return 0444;
+
+	return 0;
+}
+
 static int ads7871_read_reg8(struct spi_device *spi, int reg)
 {
 	int ret;
@@ -91,19 +100,20 @@ static int ads7871_read_reg16(struct spi_device *spi, int reg)
 static int ads7871_write_reg8(struct spi_device *spi, int reg, u8 val)
 {
 	u8 tmp[2] = {reg, val};
+
 	return spi_write(spi, tmp, sizeof(tmp));
 }
 
-static ssize_t voltage_show(struct device *dev, struct device_attribute *da,
-			    char *buf)
+static int ads7871_read(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long *val)
 {
 	struct ads7871_data *pdata = dev_get_drvdata(dev);
 	struct spi_device *spi = pdata->spi;
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
-	int ret, val, i = 0;
-	uint8_t channel, mux_cnv;
+	int ret, raw_val, i = 0;
+	u8 mux_cnv;
 
-	channel = attr->index;
+	if (type != hwmon_in || attr != hwmon_in_input)
+		return -EOPNOTSUPP;
 	/*
 	 * TODO: add support for conversions
 	 * other than single ended with a gain of 1
@@ -133,39 +143,34 @@ static ssize_t voltage_show(struct device *dev, struct device_attribute *da,
 	}
 
 	if (mux_cnv == 0) {
-		val = ads7871_read_reg16(spi, REG_LS_BYTE);
-		if (val < 0)
-			return val;
+		raw_val = ads7871_read_reg16(spi, REG_LS_BYTE);
+		if (raw_val < 0)
+			return raw_val;
+
 		/*result in volts*10000 = (val/8192)*2.5*10000*/
-		val = ((val >> 2) * 25000) / 8192;
-		return sysfs_emit(buf, "%d\n", val);
+		*val = ((raw_val >> 2) * 25000) / 8192;
+		return 0;
 	}
 
 	return -ETIMEDOUT;
 }
 
-static SENSOR_DEVICE_ATTR_RO(in0_input, voltage, 0);
-static SENSOR_DEVICE_ATTR_RO(in1_input, voltage, 1);
-static SENSOR_DEVICE_ATTR_RO(in2_input, voltage, 2);
-static SENSOR_DEVICE_ATTR_RO(in3_input, voltage, 3);
-static SENSOR_DEVICE_ATTR_RO(in4_input, voltage, 4);
-static SENSOR_DEVICE_ATTR_RO(in5_input, voltage, 5);
-static SENSOR_DEVICE_ATTR_RO(in6_input, voltage, 6);
-static SENSOR_DEVICE_ATTR_RO(in7_input, voltage, 7);
-
-static struct attribute *ads7871_attrs[] = {
-	&sensor_dev_attr_in0_input.dev_attr.attr,
-	&sensor_dev_attr_in1_input.dev_attr.attr,
-	&sensor_dev_attr_in2_input.dev_attr.attr,
-	&sensor_dev_attr_in3_input.dev_attr.attr,
-	&sensor_dev_attr_in4_input.dev_attr.attr,
-	&sensor_dev_attr_in5_input.dev_attr.attr,
-	&sensor_dev_attr_in6_input.dev_attr.attr,
-	&sensor_dev_attr_in7_input.dev_attr.attr,
+static const struct hwmon_channel_info * const ads7871_info[] = {
+	HWMON_CHANNEL_INFO(in,
+			   HWMON_I_INPUT, HWMON_I_INPUT, HWMON_I_INPUT, HWMON_I_INPUT,
+			   HWMON_I_INPUT, HWMON_I_INPUT, HWMON_I_INPUT, HWMON_I_INPUT),
 	NULL
 };
 
-ATTRIBUTE_GROUPS(ads7871);
+static const struct hwmon_ops ads7871_hwmon_ops = {
+	.is_visible = ads7871_is_visible,
+	.read = ads7871_read,
+};
+
+static const struct hwmon_chip_info ads7871_chip_info = {
+	.ops = &ads7871_hwmon_ops,
+	.info = ads7871_info,
+};
 
 static int ads7871_probe(struct spi_device *spi)
 {
@@ -200,10 +205,10 @@ static int ads7871_probe(struct spi_device *spi)
 		return -ENOMEM;
 
 	pdata->spi = spi;
-
-	hwmon_dev = devm_hwmon_device_register_with_groups(dev, spi->modalias,
-							   pdata,
-							   ads7871_groups);
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, spi->modalias,
+							 pdata,
+							 &ads7871_chip_info,
+							 NULL);
 	return PTR_ERR_OR_ZERO(hwmon_dev);
 }
 
-- 
2.43.0

[PATCH v3 3/3] hwmon: (ads7871) Use DMA-safe buffer for SPI writes

[Tabrez Ahmed]

The driver currently passes a stack-allocated buffer to spi_write(),
which is incompatible with DMA on systems with CONFIG_VMAP_STACK
enabled.

Move the transfer buffer into the driver's private data structure
to ensure it is DMA-safe. Since this shared buffer now requires
serialization, this change depends on the previous commit which
migrated the driver to the hwmon 'with_info' API.

While moving the logic, also:
- Corrected the sign extension for 14-bit data by casting to s16.
- Scaled the output to millivolts (2500mV full scale
) to comply with the hwmon ABI.

Signed-off-by: Tabrez Ahmed <tabreztalks@gmail.com>
---
 drivers/hwmon/ads7871.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/drivers/hwmon/ads7871.c b/drivers/hwmon/ads7871.c
index 851b70e9f6ba2..d0f1eec8effbf 100644
--- a/drivers/hwmon/ads7871.c
+++ b/drivers/hwmon/ads7871.c
@@ -64,6 +64,7 @@
 
 struct ads7871_data {
 	struct spi_device *spi;
+	u8 tx_buf[2] ____cacheline_aligned;
 };
 
 static umode_t ads7871_is_visible(const void *data,
@@ -97,11 +98,12 @@ static int ads7871_read_reg16(struct spi_device *spi, int reg)
 	return get_unaligned_le16(buf);
 }
 
-static int ads7871_write_reg8(struct spi_device *spi, int reg, u8 val)
+static int ads7871_write_reg8(struct ads7871_data *pdata, int reg, u8 val)
 {
-	u8 tmp[2] = {reg, val};
+	pdata->tx_buf[0] = reg;
+	pdata->tx_buf[1] = val;
 
-	return spi_write(spi, tmp, sizeof(tmp));
+	return spi_write(pdata->spi, pdata->tx_buf, 2);
 }
 
 static int ads7871_read(struct device *dev, enum hwmon_sensor_types type,
@@ -120,7 +122,7 @@ static int ads7871_read(struct device *dev, enum hwmon_sensor_types type,
 	 */
 	/*MUX_M3_BM forces single ended*/
 	/*This is also where the gain of the PGA would be set*/
-	ret = ads7871_write_reg8(spi, REG_GAIN_MUX,
+	ret = ads7871_write_reg8(pdata, REG_GAIN_MUX,
 				 (MUX_CNV_BM | MUX_M3_BM | channel));
 	if (ret < 0)
 		return ret;
@@ -128,6 +130,7 @@ static int ads7871_read(struct device *dev, enum hwmon_sensor_types type,
 	ret = ads7871_read_reg8(spi, REG_GAIN_MUX);
 	if (ret < 0)
 		return ret;
+
 	mux_cnv = ((ret & MUX_CNV_BM) >> MUX_CNV_BV);
 	/*
 	 * on 400MHz arm9 platform the conversion
@@ -147,8 +150,11 @@ static int ads7871_read(struct device *dev, enum hwmon_sensor_types type,
 		if (raw_val < 0)
 			return raw_val;
 
-		/*result in volts*10000 = (val/8192)*2.5*10000*/
-		*val = ((raw_val >> 2) * 25000) / 8192;
+		/*
+		 * Use (s16) to ensure the sign bit is preserved during the shift.
+		 * Report millivolts (2.5V = 2500mV).
+		 */
+		*val = ((s16)raw_val >> 2) * 2500 / 8192;
 		return 0;
 	}
 
@@ -185,11 +191,17 @@ static int ads7871_probe(struct spi_device *spi)
 	spi->bits_per_word = 8;
 	spi_setup(spi);
 
-	ads7871_write_reg8(spi, REG_SER_CONTROL, 0);
-	ads7871_write_reg8(spi, REG_AD_CONTROL, 0);
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+
+	pdata->spi = spi;
+
+	ads7871_write_reg8(pdata, REG_SER_CONTROL, 0);
+	ads7871_write_reg8(pdata, REG_AD_CONTROL, 0);
 
 	val = (OSC_OSCR_BM | OSC_OSCE_BM | OSC_REFE_BM | OSC_BUFE_BM);
-	ads7871_write_reg8(spi, REG_OSC_CONTROL, val);
+	ads7871_write_reg8(pdata, REG_OSC_CONTROL, val);
 	ret = ads7871_read_reg8(spi, REG_OSC_CONTROL);
 
 	dev_dbg(dev, "REG_OSC_CONTROL write:%x, read:%x\n", val, ret);
@@ -200,11 +212,6 @@ static int ads7871_probe(struct spi_device *spi)
 	if (val != ret)
 		return -ENODEV;
 
-	pdata = devm_kzalloc(dev, sizeof(struct ads7871_data), GFP_KERNEL);
-	if (!pdata)
-		return -ENOMEM;
-
-	pdata->spi = spi;
 	hwmon_dev = devm_hwmon_device_register_with_info(dev, spi->modalias,
 							 pdata,
 							 &ads7871_chip_info,
-- 
2.43.0

Re: [PATCH v3 3/3] hwmon: (ads7871) Use DMA-safe buffer for SPI writes

[sashiko-bot]

Sashiko AI review found 1 potential issue(s):
- [High] This problem wasn't introduced by this patch, but in `ads7871_read_reg16`, a pointer to a 32-bit `int` (`&reg`) is passed to `spi_write_then_read` to transmit a 1-byte command, which breaks register reads on big-endian architectures.
--

commit 4c76d6fe3d1664a2cf635cd569d8b01aeed70678
Author: Tabrez Ahmed <tabreztalks@gmail.com>

hwmon: (ads7871) Use DMA-safe buffer for SPI writes

This commit migrates the SPI write operations in the ads7871 driver to use a
DMA-safe, cacheline-aligned buffer allocated in the driver's private data
structure instead of using a stack-allocated buffer. It also adjusts the scale
calculation for 14-bit data.

> diff --git a/drivers/hwmon/ads7871.c b/drivers/hwmon/ads7871.c
> --- a/drivers/hwmon/ads7871.c
> +++ b/drivers/hwmon/ads7871.c
> @@ -97,11 +98,12 @@ static int ads7871_read_reg16(struct spi_device *spi, int reg)
>  	return get_unaligned_le16(buf);
>  }

[ ... ]

This wasn't introduced by this patch, but since you are fixing
architecture-specific compatibility issues here, I noticed something in
ads7871_read_reg16() just above this diff.

Does passing a pointer to a 32-bit int (&reg) to spi_write_then_read()
for transmitting a 1-byte command break register reads on big-endian
architectures?

static int ads7871_read_reg16(struct spi_device *spi, int reg)
{
        ...
        reg = reg | INST_READ_BM | INST_16BIT_BM;
        ret = spi_write_then_read(spi, &reg, 1, buf, 2);
        ...
}

On little-endian architectures, this correctly reads the least significant
byte containing the command. On big-endian architectures, wouldn't it read
the most significant byte (0x00) instead, sending an invalid command to
the hardware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260418034601.90226-1-tabreztalks@gmail.com?part=3