Hi Andy,
First, thanks for the great review. I agree with all of your comments, just one question/comment inline.
Best Regards,
Tomislav
> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Sent: 21 January 2021 17:42
> To: Denis, Tomislav AVL DiTEST <Tomislav.Denis@xxxxxxx>
> Cc: Jonathan Cameron <jic23@xxxxxxxxxx>; linux-iio <linux-iio@xxxxxxxxxxxxxxx>;
> devicetree <devicetree@xxxxxxxxxxxxxxx>
> Subject: Re: [PATCH v3 1/2] iio: adc: Add driver for Texas Instruments
> ADS131E0x ADC family
>
> On Tue, Jan 12, 2021 at 2:37 PM <tomislav.denis@xxxxxxx> wrote:
> >
> > From: Tomislav Denis <tomislav.denis@xxxxxxx>
> >
> > The ADS131E0x are a family of multichannel, simultaneous sampling,
> > 24-bit, delta-sigma, analog-to-digital converters (ADCs) with a
> > built-in programmable gain amplifier (PGA), internal reference and an
> > onboard oscillator.
>
...
>
> > +static const struct ads131e08_data_rate_desc ads131e08_data_rate_tbl[] = {
> > + { .rate = 64, .reg = 0x00 },
> > + { .rate = 32, .reg = 0x01 },
> > + { .rate = 16, .reg = 0x02 },
> > + { .rate = 8, .reg = 0x03 },
> > + { .rate = 4, .reg = 0x04 },
> > + { .rate = 2, .reg = 0x05 },
> > + { .rate = 1, .reg = 0x06 },
> > +};
>
> Can't you use simple bit operations on this?
>
> rate = BIT(6 - index)
> reg = index
>
> ...
>
> > +static const struct ads131e08_pga_gain_desc ads131e08_pga_gain_tbl[] = {
> > + { .gain = 1, .reg = 0x01 },
> > + { .gain = 2, .reg = 0x02 },
>
> reg == 3 valid?
>
> > + { .gain = 4, .reg = 0x04 },
> > + { .gain = 8, .reg = 0x05 },
> > + { .gain = 12, .reg = 0x06 },
> > +};
>
> Also can be changed by formula, but I remember that in some cases tables are
> preferable.
>
>
I would like to keep those tables as they are, because I find them more intuitive and easy understandable!?
>
> > +static int ads131e08_exec_cmd(struct ads131e08_state *st, u8 cmd) {
> > + int ret;
> > +
....
>
> --
> With Best Regards,
> Andy Shevchenko
