Re: [PATCH v3 2/3] iio: adc: Add ad7124 support

[Jonathan Cameron]

On Thu, 8 Nov 2018 16:48:02 +0000
"Popa, Stefan Serban" <StefanSerban.Popa@xxxxxxxxxx> wrote:

> On Jo, 2018-11-08 at 10:34 -0600, Rob Herring wrote:
> > On Thu, Nov 8, 2018 at 9:02 AM Popa, Stefan Serban
> > <StefanSerban.Popa@xxxxxxxxxx> wrote:  
> > > 
> > > 
> > > On Sb, 2018-11-03 at 12:16 +0000, Jonathan Cameron wrote:  
> > > > 
> > > > On Mon, 29 Oct 2018 18:38:31 +0200
> > > > Stefan Popa <stefan.popa@xxxxxxxxxx> wrote:
> > > >   
> > > > > 
> > > > > 
> > > > > The ad7124-4 and ad7124-8 are a family of 4 and 8 channel sigma-
> > > > > delta
> > > > > ADCs
> > > > > with 24-bit precision and reference.
> > > > > 
> > > > > Three power modes are available which in turn affect the output
> > > > > data
> > > > > rate:
> > > > > Â* Full power: 9.38 SPS to 19,200 SPS
> > > > > Â* Mid power: 2.34 SPS to 4800 SPS
> > > > > Â* Low power: 1.17 SPS to 2400 SPS
> > > > > 
> > > > > The ad7124-4 can be configured to have four differential inputs,
> > > > > while
> > > > > ad7124-8 can have 8. Moreover, ad7124 also supports per channel
> > > > > configuration. Each configuration consists of gain, reference
> > > > > source,
> > > > > output data rate and bipolar/unipolar configuration.
> > > > > 
> > > > > Datasheets:
> > > > > Link: http://www.analog.com/media/en/technical-documentation/data-s
> > > > > heet
> > > > > s/AD7124-4.pdf
> > > > > Link: http://www.analog.com/media/en/technical-documentation/data-s
> > > > > heet
> > > > > s/ad7124-8.pdf
> > > > > 
> > > > > Signed-off-by: Stefan Popa <stefan.popa@xxxxxxxxxx>  
> > > > Hi Stefan,
> > > > 
> > > > The discussion around the DT binding has gotten me looking at bit
> > > > more closely at that for this version.
> > > > 
> > > > Some most comments in that section.ÂÂAlso a really minor ordering
> > > > issue
> > > > in
> > > > remove which I'd just have fixed if we weren't going around again for
> > > > the binding.
> > > > 
> > > > Main binding thing is I don't think the odr value belongs in DT.
> > > > Gain is more marginal (unless the part can actually be damaged by
> > > > a wrong value - which I hope it can't!).ÂÂI'm not that fussed
> > > > as there are definitely reasons to specify a default scale to
> > > > cover the reasonable range on a pin.
> > > > 
> > > > Thanks,
> > > > 
> > > > Jonathan  
> > > Hi Jonathan,
> > > 
> > > Thank you for the review! So, how should I proceed?
> > > 
> > > First, we need an adc.txt file where "bipolar" and something like
> > > "diff-
> > > channels" should be documented. Should the file be placed under
> > > Documentation/devicetree/bindings/iio/adc?  
> > Yes.
> >   
> > > 
> > > Regarding the "odr-hz" property, it totally makes sense to remove it
> > > from
> > > the DT. How about the "gain"? Should we leave it in the DT and also add
> > > the
> > > possibility to be configured from user space?  
> > Look at other bindings. I think there are others having gain. If not,
> > then it probably should only be user space configurable. If so, then
> > can it be a common property too.
> > 
> > Rob
> >   
> 
> Hi Rob,
> 
> I found only a couple of examples using gain in other bindings, so I guess
> it's not common practice. I will remove the gain as well from the DT and
> set it with the default of 1.
> 
> @Jonathan: I think thatÂIIO_CHAN_INFO_HARDWAREGAIN is the attribute that
> can be used in user space?
Sorry, I missed this.  Guess you will see my review anyway around now.
Nope, hardwaregain is an oddity for devices where we aren't controlling
the thing being measured, but something like the amplifier of a
time of flight device.

There is some argument to potentially provide sane limits on gain in DT
(particularly if a device really doesn't like going out of range) but
in general I'm not keen on it as it is rather an application specific
question so best left to userspace.

Jonathan

> 
> Thank you!
> -Stefan