[PATCH v2 6/8] iio: dac: ds4424: fix -128 rejection and refactor raw access

[PATCH v2 6/8] iio: dac: ds4424: fix -128 rejection and refactor raw access

[Oleksij Rempel]

The DS442x DAC uses sign-magnitude encoding, so -128 cannot be represented.
Previously, passing -128 resulted in a truncated value that programmed 0mA.

Fix this by validating the input against the 7-bit magnitude limit.
Additionally, refactor the raw access logic to use symmetrical bitwise
operations, replacing the union structure.

Fixes: d632a2bd8ffc ("iio: dac: ds4422/ds4424 dac driver")
Cc: <stable@vger.kernel.org>
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
changes v2:
- Replace S8_MIN/MAX checks with abs() > DS4424_DAC_MASK to enforce the
  correct [-127, 127] physical range.
- Refactor read_raw/write_raw to use symmetrical bitwise operations,
  removing the custom bitfield union.
- Rebase on top of regmap port
---
 drivers/iio/dac/ds4424.c | 50 +++++++++++++++-------------------------
 1 file changed, 18 insertions(+), 32 deletions(-)

diff --git a/drivers/iio/dac/ds4424.c b/drivers/iio/dac/ds4424.c
index f9ab7f4b97ff..8110ca7f062f 100644
--- a/drivers/iio/dac/ds4424.c
+++ b/drivers/iio/dac/ds4424.c
@@ -20,9 +20,10 @@
 #define DS4422_MAX_DAC_CHANNELS		2
 #define DS4424_MAX_DAC_CHANNELS		4
 
+#define DS4424_DAC_MASK			GENMASK(6, 0)
+#define DS4424_DAC_SOURCE		BIT(7)
+
 #define DS4424_DAC_ADDR(chan)   ((chan) + 0xf8)
-#define DS4424_SOURCE_I		1
-#define DS4424_SINK_I		0
 
 #define DS4424_CHANNEL(chan) { \
 	.type = IIO_CURRENT, \
@@ -32,22 +33,6 @@
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
 }
 
-/*
- * DS4424 DAC control register 8 bits
- * [7]		0: to sink; 1: to source
- * [6:0]	steps to sink/source
- * bit[7] looks like a sign bit, but the value of the register is
- * not a two's complement code considering the bit[6:0] is a absolute
- * distance from the zero point.
- */
-union ds4424_raw_data {
-	struct {
-		u8 dx:7;
-		u8 source_bit:1;
-	};
-	u8 bits;
-};
-
 enum ds4424_device_ids {
 	ID_DS4402,
 	ID_DS4404,
@@ -127,7 +112,6 @@ static int ds4424_read_raw(struct iio_dev *indio_dev,
 			   int *val, int *val2, long mask)
 {
 	struct ds4424_data *data = iio_priv(indio_dev);
-	union ds4424_raw_data raw;
 	unsigned int regval;
 	int ret;
 
@@ -140,10 +124,11 @@ static int ds4424_read_raw(struct iio_dev *indio_dev,
 						__func__, ret);
 			return ret;
 		}
-		raw.bits = regval;
-		*val = raw.dx;
-		if (raw.source_bit == DS4424_SINK_I)
+
+		*val = regval & DS4424_DAC_MASK;
+		if (!(regval & DS4424_DAC_SOURCE))
 			*val = -*val;
+
 		return IIO_VAL_INT;
 
 	default:
@@ -156,26 +141,27 @@ static int ds4424_write_raw(struct iio_dev *indio_dev,
 			     int val, int val2, long mask)
 {
 	struct ds4424_data *data = iio_priv(indio_dev);
-	union ds4424_raw_data raw;
+	unsigned int abs_val;
 
 	if (val2 != 0)
 		return -EINVAL;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		if (val < S8_MIN || val > S8_MAX)
+		abs_val = abs(val);
+
+		if (abs_val > DS4424_DAC_MASK)
 			return -EINVAL;
 
-		if (val > 0) {
-			raw.source_bit = DS4424_SOURCE_I;
-			raw.dx = val;
-		} else {
-			raw.source_bit = DS4424_SINK_I;
-			raw.dx = -val;
-		}
+		/*
+		 * Currents exiting the IC (Source) are positive.
+		 * Canonicalize 0 to sink; datasheet treats sign as don't-care.
+		 */
+		if (val > 0)
+			abs_val |= DS4424_DAC_SOURCE;
 
 		return regmap_write(data->regmap, DS4424_DAC_ADDR(chan->channel),
-				    raw.bits);
+				    abs_val);
 
 	default:
 		return -EINVAL;
-- 
2.47.3

Re: [PATCH v2 6/8] iio: dac: ds4424: fix -128 rejection and refactor raw access

[Andy Shevchenko]

On Tue, Jan 27, 2026 at 07:09:37AM +0100, Oleksij Rempel wrote:
> The DS442x DAC uses sign-magnitude encoding, so -128 cannot be represented.
> Previously, passing -128 resulted in a truncated value that programmed 0mA.
> 
> Fix this by validating the input against the 7-bit magnitude limit.
> Additionally, refactor the raw access logic to use symmetrical bitwise
> operations, replacing the union structure.

> Fixes: d632a2bd8ffc ("iio: dac: ds4422/ds4424 dac driver")

Usually fixes go first in the series...

...

> +#define DS4424_DAC_MASK			GENMASK(6, 0)
> +#define DS4424_DAC_SOURCE		BIT(7)

+ bits.h ?

...

>  	case IIO_CHAN_INFO_RAW:
> -		if (val < S8_MIN || val > S8_MAX)
> +		abs_val = abs(val);

> +

Redundant blank line.

> +		if (abs_val > DS4424_DAC_MASK)
>  			return -EINVAL;

...

> +		/*
> +		 * Currents exiting the IC (Source) are positive.
> +		 * Canonicalize 0 to sink; datasheet treats sign as don't-care.
> +		 */
> +		if (val > 0)
> +			abs_val |= DS4424_DAC_SOURCE;

Hmm... Maybe 0 should be excluded as invalid?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/8] iio: dac: ds4424: fix -128 rejection and refactor raw access

[Oleksij Rempel]

On Tue, Jan 27, 2026 at 12:42:24PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 27, 2026 at 07:09:37AM +0100, Oleksij Rempel wrote:
> > The DS442x DAC uses sign-magnitude encoding, so -128 cannot be represented.
> > Previously, passing -128 resulted in a truncated value that programmed 0mA.
> > 
> > Fix this by validating the input against the 7-bit magnitude limit.
> > Additionally, refactor the raw access logic to use symmetrical bitwise
> > operations, replacing the union structure.
> 
> > Fixes: d632a2bd8ffc ("iio: dac: ds4422/ds4424 dac driver")
> > +		/*
> > +		 * Currents exiting the IC (Source) are positive.
> > +		 * Canonicalize 0 to sink; datasheet treats sign as don't-care.
> > +		 */
> > +		if (val > 0)
> > +			abs_val |= DS4424_DAC_SOURCE;
> 
> Hmm... Maybe 0 should be excluded as invalid?

0 is valid value for no current flow (power off). The direction bit
DS4424_DAC_SOURCE  will just make no difference if value is 0.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 6/8] iio: dac: ds4424: fix -128 rejection and refactor raw access

[Andy Shevchenko]

On Tue, Jan 27, 2026 at 11:49:05AM +0100, Oleksij Rempel wrote:
> On Tue, Jan 27, 2026 at 12:42:24PM +0200, Andy Shevchenko wrote:
> > On Tue, Jan 27, 2026 at 07:09:37AM +0100, Oleksij Rempel wrote:

...

> > > +		/*
> > > +		 * Currents exiting the IC (Source) are positive.
> > > +		 * Canonicalize 0 to sink; datasheet treats sign as don't-care.
> > > +		 */
> > > +		if (val > 0)
> > > +			abs_val |= DS4424_DAC_SOURCE;
> > 
> > Hmm... Maybe 0 should be excluded as invalid?
> 
> 0 is valid value for no current flow (power off). The direction bit
> DS4424_DAC_SOURCE  will just make no difference if value is 0.

Perhaps elaborate this in the comment above?

-- 
With Best Regards,
Andy Shevchenko