Re: [PATCH] iio: adc: Revoke valid channel for error path

[Nuno Sá]

On Thu, 2025-04-17 at 08:53 -0400, Gabriel Shahrouzi wrote:
> On Thu, Apr 17, 2025 at 6:06 AM Nuno Sá <noname.nuno@xxxxxxxxx> wrote:
> > 
> > On Tue, 2025-04-15 at 14:20 -0400, Gabriel Shahrouzi wrote:
> > > According to the datasheet on page 9 under the channel selection table,
> > > all devices (AD7816/7/8) are able to use the channel marked as 7. This
> > > channel is used for diagnostic purposes by routing the internal 1.23V
> > > bandgap source through the MUX to the input of the ADC.
> > > 
> > > Replace checking for string equality with checking for the same chip ID
> > > to reduce time complexity.
> > > 
> > > Group invalid channels for all devices together because they are
> > > processed the same way.
> > > 
> > > Fixes: 7924425db04a ("staging: iio: adc: new driver for AD7816 devices")
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Signed-off-by: Gabriel Shahrouzi <gshahrouzi@xxxxxxxxx>
> > > ---
> > >  drivers/staging/iio/adc/ad7816.c | 15 +++++----------
> > >  1 file changed, 5 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/staging/iio/adc/ad7816.c
> > > b/drivers/staging/iio/adc/ad7816.c
> > > index 6c14d7bcdd675..d880fe0257697 100644
> > > --- a/drivers/staging/iio/adc/ad7816.c
> > > +++ b/drivers/staging/iio/adc/ad7816.c
> > > @@ -186,17 +186,12 @@ static ssize_t ad7816_store_channel(struct device
> > > *dev,
> > >       if (ret)
> > >               return ret;
> > > 
> > > -     if (data > AD7816_CS_MAX && data != AD7816_CS_MASK) {
> > > -             dev_err(&chip->spi_dev->dev, "Invalid channel id %lu for
> > > %s.\n",
> > > -                     data, indio_dev->name);
> > > -             return -EINVAL;
> > > -     } else if (strcmp(indio_dev->name, "ad7818") == 0 && data > 1) {
> > > -             dev_err(&chip->spi_dev->dev,
> > > -                     "Invalid channel id %lu for ad7818.\n", data);
> > > -             return -EINVAL;
> > > -     } else if (strcmp(indio_dev->name, "ad7816") == 0 && data > 0) {
> > > +     if (data != AD7816_CS_MASK &&
> > > +         (data > AD7816_CS_MAX ||
> > > +         (chip->id == ID_AD7818 && data > 1) ||
> > > +         (chip->id == ID_AD7816 && data > 0))) {
> > >               dev_err(&chip->spi_dev->dev,
> > > -                     "Invalid channel id %lu for ad7816.\n", data);
> > > +                     "Invalid channel id %lu for %s.\n", data, indio_dev-
> > > > name);
> > >               return -EINVAL;
> > >       }
> > 
> > Hmm, maybe I'm missing something but the code just looks the same as before
> > (from a functionality point of view)? I'm really not seeing any fix...
> I might have to change it for readability. From my understanding, if
> channel 7 is selected (AD7816_CS_MASK), it never enters the error path
> whereas in the old code, if the chip were either ad7816 or ad7818, it would
> end up returning an error because it skips all channels above either 0
> or 1.

Ahh, right!

One good refactor is to add a chip_info struct (renaming the existing one) with
let's say a name and max_channels. Then, the condition could be reduced to:

if (data > st->chip->max_channel && data != AD7816_CS_MASK {
	dev_err(...);
	return -EINVAL;
}

Being this in staging, I guess we don't care much about having the fix as the
first patch to make it easier to backport.

- Nuno Sá

> 
> > 
> > Having said the above, not sure if grouping helps with readability. But I do
> > agree with moving from string comparison to use chip->id. And we also have
> > redundants 'else'
> > 
> > - Nuno Sá
> >