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
