linux-kernel - Re: [PATCH 3/6] iio: adc: ad7606: use realbits for sign-extending in scan

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+GgBR-eKUir7FGOfimv0rE2yP3P3awMZSG8=6t202GAthi=Mg@mail.gmail.com>
Date: Tue, 22 Oct 2024 09:09:45 +0300
From: Alexandru Ardelean <aardelean@...libre.com>
To: David Lechner <dlechner@...libre.com>
Cc: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	devicetree@...r.kernel.org, jic23@...nel.org, krzk+dt@...nel.org, 
	robh@...nel.org, lars@...afoo.de, michael.hennerich@...log.com, 
	gstols@...libre.com
Subject: Re: [PATCH 3/6] iio: adc: ad7606: use realbits for sign-extending in scan_direct

On Mon, Oct 21, 2024 at 8:19 PM David Lechner <dlechner@...libre.com> wrote:
>
> On 10/21/24 8:02 AM, Alexandru Ardelean wrote:
> > Currently the 'ad7606' driver supports parts with 18 and 16 bits
> > resolutions.
> > But when adding support for AD7607 (which has a 14-bit resolution) we
> > should check for the 'realbits' field, to be able to sign-extend correctly.
> >
> > Signed-off-by: Alexandru Ardelean <aardelean@...libre.com>
> > ---
> >  drivers/iio/adc/ad7606.c | 24 +++++++++++++++++++-----
> >  1 file changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> > index d0fe9fd65f3f..a1f0c2feb04a 100644
> > --- a/drivers/iio/adc/ad7606.c
> > +++ b/drivers/iio/adc/ad7606.c
> > @@ -568,7 +568,7 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
> >                             int *val)
> >  {
> >       struct ad7606_state *st = iio_priv(indio_dev);
> > -     unsigned int storagebits = st->chip_info->channels[1].scan_type.storagebits;
> > +     unsigned int realbits = st->chip_info->channels[1].scan_type.realbits;
> >       const struct iio_chan_spec *chan;
> >       int ret;
> >
> > @@ -603,15 +603,29 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
> >
> >       chan = &indio_dev->channels[ch + 1];
> >       if (chan->scan_type.sign == 'u') {
> > -             if (storagebits > 16)
>
> I think it would be simpler to keep this if statement for choosing
> which data.bufXX to use since there are only 2 choices.
>
>
> > +             switch (realbits) {
> > +             case 18:
> >                       *val = st->data.buf32[ch];
> > -             else
> > +                     break;
> > +             case 16:
> >                       *val = st->data.buf16[ch];
> > +                     break;
> > +             default:
> > +                     ret = -EINVAL;
> > +                     break;
> > +             }
> >       } else {
> > -             if (storagebits > 16)
> > +             switch (realbits) {
> > +             case 18:
> >                       *val = sign_extend32(st->data.buf32[ch], 17);
> > -             else
> > +                     break;
> > +             case 16:
> >                       *val = sign_extend32(st->data.buf16[ch], 15);
> > +                     break;
> > +             default:
> > +                     ret = -EINVAL;
> > +                     break;
> > +             }
>
> We can change this to:
>
>         *val = sign_extend32(st->data.buf16[ch], reablbits - 1);
>
> Then no additional changes should be needed to support 14-bit chips.
>
> And similarly, we could avoid the need to use the more verbose
> switch statement.

Ack.

>
> >       }
> >
> >  error_ret:
>
>