The DS442x DAC uses sign-magnitude encoding, so -128 cannot be
represented in hardware.

With the previous check, userspace could pass -128, which gets converted
to a magnitude of 128 and then truncated by the 7-bit DAC field. This
ends up programming a zero magnitude with the sign bit set, i.e. an
unintended output (effectively 0 mA instead of -128 steps).

Reject -128 to avoid silently producing the wrong current.

Fixes: d632a2bd8ffc ("iio: dac: ds4422/ds4424 dac driver")
Cc: <stable@vger.kernel.org>
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/iio/dac/ds4424.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/dac/ds4424.c b/drivers/iio/dac/ds4424.c
index 072b7e6672cf..9c24c37d3c42 100644
--- a/drivers/iio/dac/ds4424.c
+++ b/drivers/iio/dac/ds4424.c
@@ -143,7 +143,7 @@ static int ds4424_write_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		if (val < S8_MIN || val > S8_MAX)
+		if (val <= S8_MIN || val > S8_MAX)
 			return -EINVAL;
 
 		if (val > 0) {
-- 
2.47.3

On Mon, Jan 19, 2026 at 07:24:20PM +0100, Oleksij Rempel wrote:
> The DS442x DAC uses sign-magnitude encoding, so -128 cannot be
> represented in hardware.
> 
> With the previous check, userspace could pass -128, which gets converted
> to a magnitude of 128 and then truncated by the 7-bit DAC field. This
> ends up programming a zero magnitude with the sign bit set, i.e. an
> unintended output (effectively 0 mA instead of -128 steps).
> 
> Reject -128 to avoid silently producing the wrong current.

...

> -		if (val < S8_MIN || val > S8_MAX)
> +		if (val <= S8_MIN || val > S8_MAX)
>  			return -EINVAL;

Hmm... So the range is [ -127 .. 0 .. 127 ] ?

I think in such case the plain numbers would be more specific than
the type related limits.

-- 
With Best Regards,
Andy Shevchenko

On Mon, 19 Jan 2026 21:03:46 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Mon, Jan 19, 2026 at 07:24:20PM +0100, Oleksij Rempel wrote:
> > The DS442x DAC uses sign-magnitude encoding, so -128 cannot be
> > represented in hardware.
> > 
> > With the previous check, userspace could pass -128, which gets converted
> > to a magnitude of 128 and then truncated by the 7-bit DAC field. This
> > ends up programming a zero magnitude with the sign bit set, i.e. an
> > unintended output (effectively 0 mA instead of -128 steps).
> > 
> > Reject -128 to avoid silently producing the wrong current.  
> 
> ...
> 
> > -		if (val < S8_MIN || val > S8_MAX)
> > +		if (val <= S8_MIN || val > S8_MAX)
> >  			return -EINVAL;  
> 
> Hmm... So the range is [ -127 .. 0 .. 127 ] ?
> 
> I think in such case the plain numbers would be more specific than
> the type related limits.
> 

Check the abs(val) <= 127 given that's what we care about I think?
Or make it explicit and do
FIELD_FIT() against a mask that you then use to fill the register
value (another mask for the sign bit).

Btw use abs(val) to set raw.dx and drop it out of the conditional.
Even better get rid of the bitfield stuff and just add
two defines + fill val directly in this function using FIELD_PREP().
Then both the checking and the field filling use the same defines
and it should be easy to see what is going on.

Jonathan

On Fri, Jan 23, 2026 at 09:33:57AM +0000, Jonathan Cameron wrote:
> On Mon, 19 Jan 2026 21:03:46 +0200
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> 
> > On Mon, Jan 19, 2026 at 07:24:20PM +0100, Oleksij Rempel wrote:
> > > The DS442x DAC uses sign-magnitude encoding, so -128 cannot be
> > > represented in hardware.
> > > 
> > > With the previous check, userspace could pass -128, which gets converted
> > > to a magnitude of 128 and then truncated by the 7-bit DAC field. This
> > > ends up programming a zero magnitude with the sign bit set, i.e. an
> > > unintended output (effectively 0 mA instead of -128 steps).
> > > 
> > > Reject -128 to avoid silently producing the wrong current.  
> > 
> > ...
> > 
> > > -		if (val < S8_MIN || val > S8_MAX)
> > > +		if (val <= S8_MIN || val > S8_MAX)
> > >  			return -EINVAL;  
> > 
> > Hmm... So the range is [ -127 .. 0 .. 127 ] ?
> > 
> > I think in such case the plain numbers would be more specific than
> > the type related limits.
> > 
> 
> Check the abs(val) <= 127 given that's what we care about I think?
> Or make it explicit and do
> FIELD_FIT() against a mask that you then use to fill the register
> value (another mask for the sign bit).
> 
> Btw use abs(val) to set raw.dx and drop it out of the conditional.
> Even better get rid of the bitfield stuff and just add
> two defines + fill val directly in this function using FIELD_PREP().
> Then both the checking and the field filling use the same defines
> and it should be easy to see what is going on.

FIELD_* macros require compile-time constant masks. Since the next patch
adds support for variants with different data widths (making the mask a
runtime variable), I prefer using an implementation now that remains
consistent with the followup changes.

-- 
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 |