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

[Gabriel Shahrouzi]

On Thu, Apr 17, 2025 at 10:02 AM Nuno Sá <noname.nuno@xxxxxxxxx> wrote:
>
> 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;
> }
Makes sense. I sent a V2 with the updates. Also included enum
ad7816_type as a member for chip_info but not sure if it is necessary.
Renamed the existing one to ad7816_state.
>
> 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.
In other words, combining the refactoring and fix into one patch is
fine but normally they would be split?

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