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

[Gabriel Shahrouzi]

On Fri, Apr 18, 2025 at 5:46 AM Nuno Sá <noname.nuno@xxxxxxxxx> wrote:
>
> On Thu, 2025-04-17 at 13:08 -0400, Gabriel Shahrouzi wrote:
> > 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?
>
> Yes, in theory we want to have the fixes first before any refactor because we might
> want to backport the fix and we do not want to backport more code than needed. Not
> totally sure but being this on staging we might not care that much about this.
Got it.
>
> - Nuno Sá
> >
> > >
> > > - 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á
> > > > >
>