Re: [PATCH] staging: iio: accel: adis16240: Improve readability on write_raw function

[Rodrigo Ribeiro]

Em dom, 11 de ago de 2019 Ãs 05:43, Jonathan Cameron
<jic23@xxxxxxxxxx> escreveu:
>
> On Sat, 10 Aug 2019 12:00:58 -0300
> Rodrigo <rodrigorsdc@xxxxxxxxx> wrote:
>
> > From: Rodrigo Carvalho <rodrigorsdc@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > ---
> >  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