[v3] iio: dac: ds4424: add DS4402/DS4404 support and scale

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

Posted by Oleksij Rempel 1 week, 5 days ago

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 v3:
- Remove "Rebase on top of regmap" note as this is now patch 1/8.
- Add #include <linux/bits.h>
- Clarify 0mA sink/source behavior in comments
- Remove redundant blank line in write_raw
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 | 55 +++++++++++++++-------------------------
 1 file changed, 21 insertions(+), 34 deletions(-)

diff --git a/drivers/iio/dac/ds4424.c b/drivers/iio/dac/ds4424.c
index a8198ba4f98a..596ff5999271 100644
--- a/drivers/iio/dac/ds4424.c
+++ b/drivers/iio/dac/ds4424.c
@@ -5,6 +5,7 @@
  * Copyright (C) 2017 Maxim Integrated
  */
 
+#include <linux/bits.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/i2c.h>
@@ -19,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, \
@@ -31,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_DS4422,
 	ID_DS4424,
@@ -108,21 +94,21 @@ static int ds4424_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan,
 			   int *val, int *val2, long mask)
 {
-	union ds4424_raw_data raw;
-	int ret;
+	int ret, regval;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		ret = ds4424_get_value(indio_dev, val, chan->channel);
+		ret = ds4424_get_value(indio_dev, &regval, chan->channel);
 		if (ret < 0) {
 			pr_err("%s : ds4424_get_value returned %d\n",
 							__func__, ret);
 			return ret;
 		}
-		raw.bits = *val;
-		*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:
@@ -134,25 +120,26 @@ static int ds4424_write_raw(struct iio_dev *indio_dev,
 			     struct iio_chan_spec const *chan,
 			     int val, int val2, long mask)
 {
-	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. 0 is a valid
+		 * value for no current flow; the direction bit (Source vs Sink)
+		 * is treated as don't-care by the hardware at 0.
+		 */
+		if (val > 0)
+			abs_val |= DS4424_DAC_SOURCE;
 
-		return ds4424_set_value(indio_dev, raw.bits, chan);
+		return ds4424_set_value(indio_dev, abs_val, chan);
 
 	default:
 		return -EINVAL;
-- 
2.47.3

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

Posted by Jonathan Cameron 1 week, 4 days ago

On Wed, 28 Jan 2026 16:38:17 +0100
Oleksij Rempel <o.rempel@pengutronix.de> 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")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Hi Olkesij

This is good stuff but, this fails the test of being the minimal fix
suited for a trivial backport.

The right solution here is split it.  Just apply the correct
limit in the fix patch, then the refactors in a patch on top of
that which most likely won't be backported for stable.

Thanks,

Jonathan


> ---
> changes v3:
> - Remove "Rebase on top of regmap" note as this is now patch 1/8.
> - Add #include <linux/bits.h>
> - Clarify 0mA sink/source behavior in comments
> - Remove redundant blank line in write_raw
> 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 | 55 +++++++++++++++-------------------------
>  1 file changed, 21 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/iio/dac/ds4424.c b/drivers/iio/dac/ds4424.c
> index a8198ba4f98a..596ff5999271 100644
> --- a/drivers/iio/dac/ds4424.c
> +++ b/drivers/iio/dac/ds4424.c
> @@ -5,6 +5,7 @@
>   * Copyright (C) 2017 Maxim Integrated
>   */
>  
> +#include <linux/bits.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/i2c.h>
> @@ -19,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, \
> @@ -31,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_DS4422,
>  	ID_DS4424,
> @@ -108,21 +94,21 @@ static int ds4424_read_raw(struct iio_dev *indio_dev,
>  			   struct iio_chan_spec const *chan,
>  			   int *val, int *val2, long mask)
>  {
> -	union ds4424_raw_data raw;
> -	int ret;
> +	int ret, regval;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> -		ret = ds4424_get_value(indio_dev, val, chan->channel);
> +		ret = ds4424_get_value(indio_dev, &regval, chan->channel);
>  		if (ret < 0) {
>  			pr_err("%s : ds4424_get_value returned %d\n",
>  							__func__, ret);
>  			return ret;
>  		}
> -		raw.bits = *val;
> -		*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:
> @@ -134,25 +120,26 @@ static int ds4424_write_raw(struct iio_dev *indio_dev,
>  			     struct iio_chan_spec const *chan,
>  			     int val, int val2, long mask)
>  {
> -	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;

Just this bit belongs in fix patch.


>  
> -		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. 0 is a valid
> +		 * value for no current flow; the direction bit (Source vs Sink)
> +		 * is treated as don't-care by the hardware at 0.
> +		 */
> +		if (val > 0)
> +			abs_val |= DS4424_DAC_SOURCE;
>  
> -		return ds4424_set_value(indio_dev, raw.bits, chan);
> +		return ds4424_set_value(indio_dev, abs_val, chan);
>  
>  	default:
>  		return -EINVAL;

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

Posted by Oleksij Rempel 1 week, 1 day ago

Hi Jonathan,

On Thu, Jan 29, 2026 at 05:58:19PM +0000, Jonathan Cameron wrote:
> On Wed, 28 Jan 2026 16:38:17 +0100
> Oleksij Rempel <o.rempel@pengutronix.de> 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")
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> Hi Olkesij
> 
> This is good stuff but, this fails the test of being the minimal fix
> suited for a trivial backport.
> 
> The right solution here is split it.  Just apply the correct
> limit in the fix patch, then the refactors in a patch on top of
> that which most likely won't be backported for stable.

The v1 of this patch was implemented according to the fix patch
requirements:
https://lore.kernel.org/all/20260119182424.1660601-5-o.rempel@pengutronix.de/

May be keep v1 as is and rebase v3 as refactoring stage on top of it?

Best Regards,
Oleksij
-- 
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 v3 1/8] iio: dac: ds4424: fix -128 rejection and refactor raw access

Posted by Jonathan Cameron 1 week, 1 day ago

On Sun, 1 Feb 2026 13:40:42 +0100
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> Hi Jonathan,
> 
> On Thu, Jan 29, 2026 at 05:58:19PM +0000, Jonathan Cameron wrote:
> > On Wed, 28 Jan 2026 16:38:17 +0100
> > Oleksij Rempel <o.rempel@pengutronix.de> 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")
> > > Cc: <stable@vger.kernel.org>
> > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>  
> > Hi Olkesij
> > 
> > This is good stuff but, this fails the test of being the minimal fix
> > suited for a trivial backport.
> > 
> > The right solution here is split it.  Just apply the correct
> > limit in the fix patch, then the refactors in a patch on top of
> > that which most likely won't be backported for stable.  
> 
> The v1 of this patch was implemented according to the fix patch
> requirements:
> https://lore.kernel.org/all/20260119182424.1660601-5-o.rempel@pengutronix.de/
> 
> May be keep v1 as is and rebase v3 as refactoring stage on top of it?
Perfect.

Thanks!

Jonathan

> 
> Best Regards,
> Oleksij

[PATCH v3 1/8] iio: dac: ds4424: fix -128 rejection and refactor raw access
[PATCH v3 2/8] iio: dac: ds4424: ratelimit read errors and use device context
[PATCH v3 3/8] iio: dac: ds4424: sort headers alphabetically
[PATCH v3 4/8] dt-bindings: iio: dac: maxim,ds4424: add ds4402/ds4404
[PATCH v3 5/8] dt-bindings: iio: dac: maxim,ds4424: add maxim,rfs-ohms property
[PATCH v3 6/8] iio: dac: ds4424: add DS4402/DS4404 device IDs
[PATCH v3 7/8] iio: dac: ds4424: convert to regmap
[PATCH v3 8/8] iio: dac: ds4424: add Rfs-based scale and per-variant limits