RE: [PATCH v3 1/2] iio: adc: Add driver for Texas Instruments ADS131E0x ADC family

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux