lkml.org 
[lkml]   [2019]   [Aug]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] staging: iio: accel: adis16240: Improve readability on write_raw function
Em dom, 11 de ago de 2019 às 05:43, Jonathan Cameron
<jic23@kernel.org> escreveu:
>
> On Sat, 10 Aug 2019 12:00:58 -0300
> Rodrigo <rodrigorsdc@gmail.com> wrote:
>
> > From: Rodrigo Carvalho <rodrigorsdc@gmail.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@gmail.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

\
 
 \ /
  Last update: 2019-08-11 18:48    [W:0.629 / U:0.436 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site