linux-kernel - Re: [PATCH] staging: iio: accel: adis16240: Improve readability on write

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]

Date:   Sun, 11 Aug 2019 13:47:04 -0300
From:   Rodrigo Ribeiro <rodrigorsdc@...il.com>
To:     Jonathan Cameron <jic23@...nel.org>
Cc:     Lars-Peter Clausen <lars@...afoo.de>,
        Michael Hennerich <Michael.Hennerich@...log.com>,
        Stefan Popa <stefan.popa@...log.com>,
        Hartmut Knaack <knaack.h@....de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-iio@...r.kernel.org, devel@...verdev.osuosl.org,
        LKML <linux-kernel@...r.kernel.org>, kernel-usp@...glegroups.com
Subject: Re: [PATCH] staging: iio: accel: adis16240: Improve readability on
 write_raw function

Em dom, 11 de ago de 2019 às 05:43, Jonathan Cameron
<jic23@...nel.org> escreveu:
>
> On Sat, 10 Aug 2019 12:00:58 -0300
> Rodrigo <rodrigorsdc@...il.com> wrote:
>
> > From: Rodrigo Carvalho <rodrigorsdc@...il.com>
> >
> > Improve readability by using GENMASK macro, changing switch statement
> > by if statement and removing unnecessary local variables.
>

Hi Jonathan. Thanks for reviewing!

> From your description it sounds like multiple changes in one patch.
> Always preferable to have one type of change in a patch and more
> small patches.
>
> Based on comments below, I would leave the switch statement alone,
> but put in your GENMASK change as that one is good and gets
> rid of the odd local variable 'bits' as well :)
>
> Thanks,
>
> Jonathan
>
>
> >
> > Signed-off-by: Rodrigo Ribeiro Carvalho <rodrigorsdc@...il.com>
> > ---
> >  drivers/staging/iio/accel/adis16240.c | 16 +++++++---------
> >  1 file changed, 7 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/staging/iio/accel/adis16240.c b/drivers/staging/iio/accel/adis16240.c
> > index 62f4b3b1b457..68f165501389 100644
> > --- a/drivers/staging/iio/accel/adis16240.c
> > +++ b/drivers/staging/iio/accel/adis16240.c
> > @@ -309,17 +309,15 @@ static int adis16240_write_raw(struct iio_dev *indio_dev,
> >                              long mask)
> >  {
> >       struct adis *st = iio_priv(indio_dev);
> > -     int bits = 10;
> > -     s16 val16;
> > +     int m;
> >       u8 addr;
> >
> > -     switch (mask) {
> > -     case IIO_CHAN_INFO_CALIBBIAS:
> > -             val16 = val & ((1 << bits) - 1);
> > -             addr = adis16240_addresses[chan->scan_index][0];
> > -             return adis_write_reg_16(st, addr, val16);
> > -     }
> > -     return -EINVAL;
> > +     if (mask != IIO_CHAN_INFO_CALIBBIAS)
> > +             return -EINVAL;
>
> Hmm. We generally encourage the use of switch statements in these
> cases because they reduce churn as new features are added.
>
> In this particular case, we don't have any control of sampling frequency
> in the driver, but the hardware appears to support it (table 23 on the
> datasheet).

On drivers of same kind out of staging (adis16209 and adis16201), sampling
frequency writing are not implemented, even though datasheets suggest a register
writing for this. I can try to implement if it is a good one.

> > +
> > +     m = GENMASK(9, 0);
> > +     addr = adis16240_addresses[chan->scan_index][0];
> > +     return adis_write_reg_16(st, addr, val & m);
> Why the local variable m?  Can we not just do
>
>         return adis_write_reg_16(st, addr, val & GENMASK(9, 0));
>
> If anything I think that is a little more readable than your
> version.  There is a reasonable argument for just having
> addr inline as well.
>
>         return adis_write_reg_16(st,
>                                  adis16240_addresses[chan->scan_index][0],
>                                  val & GENMASK(9, 0));
>
> However, given I'm suggesting you leave it as a switch statement, it
> will be too long with addr inline.
>
> >  }
> >
> >  static const struct iio_chan_spec adis16240_channels[] = {
>

Regards,
Rodrigo