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

[Jonathan Cameron <jic23@kernel.org>]

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;