[PATCH] iio: dac: adi-axi-dac: drop io_mode check

[PATCH] iio: dac: adi-axi-dac: drop io_mode check

[Angelo Dureghello]

From: Angelo Dureghello <adureghello@baylibre.com>

Drop mode check, producing the following robot test warning:

smatch warnings:
drivers/iio/dac/adi-axi-dac.c:731 axi_dac_bus_set_io_mode()
  warn: always true condition '(mode >= 0) => (0-u32max >= 0)'

The range check results not useful since these are the only
plausible modes for enum ad3552r_io_mode.

Fixes: 493122c53af1 ("iio: dac: adi-axi-dac: add bus mode setup")
Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
---
 drivers/iio/dac/adi-axi-dac.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c
index ac4c96c4ccf3..bcaf365feef4 100644
--- a/drivers/iio/dac/adi-axi-dac.c
+++ b/drivers/iio/dac/adi-axi-dac.c
@@ -728,9 +728,6 @@ static int axi_dac_bus_set_io_mode(struct iio_backend *back,
 	struct axi_dac_state *st = iio_backend_get_priv(back);
 	int ival, ret;
 
-	if (!(mode >= AD3552R_IO_MODE_SPI && mode <= AD3552R_IO_MODE_QSPI))
-		return -EINVAL;
-
 	guard(mutex)(&st->lock);
 
 	ret = regmap_update_bits(st->regmap, AXI_DAC_CUSTOM_CTRL_REG,

---
base-commit: c667ed738d7d1a01e9b946ef47cae113b0a05bee
change-id: 20250206-wip-bl-ad3552r-axi-v0-iio-testing-carlos-070ecab6a52a

Best regards,
-- 
Angelo Dureghello <adureghello@baylibre.com>

Re: [PATCH] iio: dac: adi-axi-dac: drop io_mode check

[Jonathan Cameron]

On Thu, 06 Feb 2025 09:36:14 +0100
Angelo Dureghello <adureghello@baylibre.com> wrote:

> From: Angelo Dureghello <adureghello@baylibre.com>
> 
> Drop mode check, producing the following robot test warning:
> 
> smatch warnings:
> drivers/iio/dac/adi-axi-dac.c:731 axi_dac_bus_set_io_mode()
>   warn: always true condition '(mode >= 0) => (0-u32max >= 0)'
> 
> The range check results not useful since these are the only
> plausible modes for enum ad3552r_io_mode.
> 
> Fixes: 493122c53af1 ("iio: dac: adi-axi-dac: add bus mode setup")
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
Ah. I missed this.  Anyhow made the same change directly so all is well
than ends well!

Thanks,

Jonathan

> ---
>  drivers/iio/dac/adi-axi-dac.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c
> index ac4c96c4ccf3..bcaf365feef4 100644
> --- a/drivers/iio/dac/adi-axi-dac.c
> +++ b/drivers/iio/dac/adi-axi-dac.c
> @@ -728,9 +728,6 @@ static int axi_dac_bus_set_io_mode(struct iio_backend *back,
>  	struct axi_dac_state *st = iio_backend_get_priv(back);
>  	int ival, ret;
>  
> -	if (!(mode >= AD3552R_IO_MODE_SPI && mode <= AD3552R_IO_MODE_QSPI))
> -		return -EINVAL;
> -
>  	guard(mutex)(&st->lock);
>  
>  	ret = regmap_update_bits(st->regmap, AXI_DAC_CUSTOM_CTRL_REG,
> 
> ---
> base-commit: c667ed738d7d1a01e9b946ef47cae113b0a05bee
> change-id: 20250206-wip-bl-ad3552r-axi-v0-iio-testing-carlos-070ecab6a52a
> 
> Best regards,

Re: [PATCH] iio: dac: adi-axi-dac: drop io_mode check

[Nuno Sá]

On Sat, 2025-02-08 at 15:45 +0000, Jonathan Cameron wrote:
> On Thu, 06 Feb 2025 09:36:14 +0100
> Angelo Dureghello <adureghello@baylibre.com> wrote:
> 
> > From: Angelo Dureghello <adureghello@baylibre.com>
> > 
> > Drop mode check, producing the following robot test warning:
> > 
> > smatch warnings:
> > drivers/iio/dac/adi-axi-dac.c:731 axi_dac_bus_set_io_mode()
> >   warn: always true condition '(mode >= 0) => (0-u32max >= 0)'
> > 
> > The range check results not useful since these are the only
> > plausible modes for enum ad3552r_io_mode.
> > 
> > Fixes: 493122c53af1 ("iio: dac: adi-axi-dac: add bus mode setup")
> > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> Ah. I missed this.  Anyhow made the same change directly so all is well
> than ends well!
> 

Hi Angelo, Jonathan,

I wanted to reply to this one when I saw it but I haven't done right away and
then totally forgot. Sorry about that!

I don't really agree with the "fix" in this patch. AFAIU, smatch is complaining
since the enum is apparently defaulting to an unsigned type which means doing
the >= 0 check is useless. But we should keep the upper bound...

- Nuno Sá

Re: [PATCH] iio: dac: adi-axi-dac: drop io_mode check

[Jonathan Cameron]

On Mon, 10 Feb 2025 10:05:47 +0000
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Sat, 2025-02-08 at 15:45 +0000, Jonathan Cameron wrote:
> > On Thu, 06 Feb 2025 09:36:14 +0100
> > Angelo Dureghello <adureghello@baylibre.com> wrote:
> >   
> > > From: Angelo Dureghello <adureghello@baylibre.com>
> > > 
> > > Drop mode check, producing the following robot test warning:
> > > 
> > > smatch warnings:
> > > drivers/iio/dac/adi-axi-dac.c:731 axi_dac_bus_set_io_mode()
> > >   warn: always true condition '(mode >= 0) => (0-u32max >= 0)'
> > > 
> > > The range check results not useful since these are the only
> > > plausible modes for enum ad3552r_io_mode.
> > > 
> > > Fixes: 493122c53af1 ("iio: dac: adi-axi-dac: add bus mode setup")
> > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>  
> > Ah. I missed this.  Anyhow made the same change directly so all is well
> > than ends well!
> >   
> 
> Hi Angelo, Jonathan,
> 
> I wanted to reply to this one when I saw it but I haven't done right away and
> then totally forgot. Sorry about that!
> 
> I don't really agree with the "fix" in this patch. AFAIU, smatch is complaining
> since the enum is apparently defaulting to an unsigned type which means doing
> the >= 0 check is useless. But we should keep the upper bound...

Why? It's an enum so unless we are messing around with deliberate casts the
compiler should always be able to spot this. The check may be needed on a future
date if we add more types to that enum.

So I agree the check wasn't terrible and perhaps acted as hardening but it
isn't strictly speaking doing anything today.

Jonathan


> 
> - Nuno Sá
> 

Re: [PATCH] iio: dac: adi-axi-dac: drop io_mode check

[Nuno Sá]

On Mon, 2025-02-10 at 19:13 +0000, Jonathan Cameron wrote:
> On Mon, 10 Feb 2025 10:05:47 +0000
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Sat, 2025-02-08 at 15:45 +0000, Jonathan Cameron wrote:
> > > On Thu, 06 Feb 2025 09:36:14 +0100
> > > Angelo Dureghello <adureghello@baylibre.com> wrote:
> > >   
> > > > From: Angelo Dureghello <adureghello@baylibre.com>
> > > > 
> > > > Drop mode check, producing the following robot test warning:
> > > > 
> > > > smatch warnings:
> > > > drivers/iio/dac/adi-axi-dac.c:731 axi_dac_bus_set_io_mode()
> > > >   warn: always true condition '(mode >= 0) => (0-u32max >= 0)'
> > > > 
> > > > The range check results not useful since these are the only
> > > > plausible modes for enum ad3552r_io_mode.
> > > > 
> > > > Fixes: 493122c53af1 ("iio: dac: adi-axi-dac: add bus mode setup")
> > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>  
> > > Ah. I missed this.  Anyhow made the same change directly so all is well
> > > than ends well!
> > >   
> > 
> > Hi Angelo, Jonathan,
> > 
> > I wanted to reply to this one when I saw it but I haven't done right away
> > and
> > then totally forgot. Sorry about that!
> > 
> > I don't really agree with the "fix" in this patch. AFAIU, smatch is
> > complaining
> > since the enum is apparently defaulting to an unsigned type which means
> > doing
> > the >= 0 check is useless. But we should keep the upper bound...
> 
> Why? It's an enum so unless we are messing around with deliberate casts the
> compiler should always be able to spot this. The check may be needed on a
> future

I do not think the compiler will catch this:

diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c
index c1dae58c1975..5234dd5e227d 100644
--- a/drivers/iio/dac/ad3552r-hs.c
+++ b/drivers/iio/dac/ad3552r-hs.c
@@ -293,7 +293,7 @@ static int ad3552r_hs_buffer_postenable(struct iio_dev
*indio_dev)
         * Back bus to simple SPI, this must be executed together with above
         * target mode unwind, and can be done only after it.
         */
-       st->data->bus_set_io_mode(st->back, AD3552R_IO_MODE_SPI);
+       st->data->bus_set_io_mode(st->back, -1);

A W=1 build (clang) did not complained at all... Maybe tools like smatch will.

> date if we add more types to that enum.
> 
> So I agree the check wasn't terrible and perhaps acted as hardening but it
> isn't strictly speaking doing anything today.
> 

It's not a very super important check, I agree... and being an enum will be
easier to spot a raw value being passed during a review but since we already had
the check, I don't see why we should remove it completely and not keep the upper
bound.

- Nuno Sá

Re: [PATCH] iio: dac: adi-axi-dac: drop io_mode check

[Jonathan Cameron]

On Tue, 11 Feb 2025 09:56:31 +0000
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Mon, 2025-02-10 at 19:13 +0000, Jonathan Cameron wrote:
> > On Mon, 10 Feb 2025 10:05:47 +0000
> > Nuno Sá <noname.nuno@gmail.com> wrote:
> >   
> > > On Sat, 2025-02-08 at 15:45 +0000, Jonathan Cameron wrote:  
> > > > On Thu, 06 Feb 2025 09:36:14 +0100
> > > > Angelo Dureghello <adureghello@baylibre.com> wrote:
> > > >     
> > > > > From: Angelo Dureghello <adureghello@baylibre.com>
> > > > > 
> > > > > Drop mode check, producing the following robot test warning:
> > > > > 
> > > > > smatch warnings:
> > > > > drivers/iio/dac/adi-axi-dac.c:731 axi_dac_bus_set_io_mode()
> > > > >   warn: always true condition '(mode >= 0) => (0-u32max >= 0)'
> > > > > 
> > > > > The range check results not useful since these are the only
> > > > > plausible modes for enum ad3552r_io_mode.
> > > > > 
> > > > > Fixes: 493122c53af1 ("iio: dac: adi-axi-dac: add bus mode setup")
> > > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>    
> > > > Ah. I missed this.  Anyhow made the same change directly so all is well
> > > > than ends well!
> > > >     
> > > 
> > > Hi Angelo, Jonathan,
> > > 
> > > I wanted to reply to this one when I saw it but I haven't done right away
> > > and
> > > then totally forgot. Sorry about that!
> > > 
> > > I don't really agree with the "fix" in this patch. AFAIU, smatch is
> > > complaining
> > > since the enum is apparently defaulting to an unsigned type which means
> > > doing
> > > the >= 0 check is useless. But we should keep the upper bound...  
> > 
> > Why? It's an enum so unless we are messing around with deliberate casts the
> > compiler should always be able to spot this. The check may be needed on a
> > future  
> 
> I do not think the compiler will catch this:
> 
> diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c
> index c1dae58c1975..5234dd5e227d 100644
> --- a/drivers/iio/dac/ad3552r-hs.c
> +++ b/drivers/iio/dac/ad3552r-hs.c
> @@ -293,7 +293,7 @@ static int ad3552r_hs_buffer_postenable(struct iio_dev
> *indio_dev)
>          * Back bus to simple SPI, this must be executed together with above
>          * target mode unwind, and can be done only after it.
>          */
> -       st->data->bus_set_io_mode(st->back, AD3552R_IO_MODE_SPI);
> +       st->data->bus_set_io_mode(st->back, -1);
> 
> A W=1 build (clang) did not complained at all... Maybe tools like smatch will.
> 
> > date if we add more types to that enum.
> > 
> > So I agree the check wasn't terrible and perhaps acted as hardening but it
> > isn't strictly speaking doing anything today.
> >   
> 
> It's not a very super important check, I agree... and being an enum will be
> easier to spot a raw value being passed during a review but since we already had
> the check, I don't see why we should remove it completely and not keep the upper
> bound.

ok.  I'd take a patch putting the upper bound back.   Enums checking is an interesting
hole to fall down.

Jonathan

> 
> - Nuno Sá