Date:   Sat, 1 Dec 2018 15:17:47 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     "Ardelean, Alexandru" <alexandru.Ardelean@...log.com>
Cc:     "giuliano.belinassi@...il.com" <giuliano.belinassi@...il.com>,
        "Popa, Stefan Serban" <StefanSerban.Popa@...log.com>,
        "kernel-usp@...glegroups.com" <kernel-usp@...glegroups.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "lars@...afoo.de" <lars@...afoo.de>,
        "knaack.h@....de" <knaack.h@....de>,
        "Hennerich, Michael" <Michael.Hennerich@...log.com>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
        "pmeerw@...erw.net" <pmeerw@...erw.net>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
Subject: Re: [PATCH] staging: iio: ad7780: Add gain & filter gpio support

On Thu, 29 Nov 2018 12:19:08 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@...log.com> wrote:

> On Tue, 2018-11-27 at 06:11 -0500, Popa, Stefan Serban wrote:
> > On Lu, 2018-11-26 at 17:24 -0200, Giuliano Belinassi wrote:
> > Hi, please see bellow
> >   
> 
> One note from me here.
> 
> > > Hi, thank you for the review
> > >   
> > > > 
> > > > On Thu, 22 Nov 2018 11:01:00 +0000
> > > > "Popa, Stefan Serban" <StefanSerban.Popa@...log.com> wrote:  
> > > > > 
> > > > > I think that instead of setting the gain directly, we should use
> > > > > the IIO_CHAN_INFO_SCALE attribute. At page 12 of the ad7780
> > > > > datasheet
> > > > > there
> > > > > is a formula from which the output code can be calculated:
> > > > > Code = 2^(N − 1)
> > > > > × [(AIN × Gain /VREF) + 1]. So, by setting the scale from user
> > > > > space,
> > > > > the
> > > > > driver can calculate the correct gain by using the formula above.
> > > > > Also, it
> > > > > would be useful to introduce scale available.
> > > > > Furthermore, there is a new
> > > > > ad7124 adc driver which does this exact thing. Take a look here:
> > > > > http
> > > > > s://gi
> > > > > thub.com/analogdevicesinc/linux/blob/master/drivers/iio/adc/ad7124.
> > > > > c#
> > > > > L337.  
> > > 
> > > We have some questions about the code you provided to us:
> > >   1-) What is exactly the inputs for the write_raw function?  
> > 
> > In your write_raw() function you need to add IIO_CHAN_INFO_SCALE case.
> > Setting the scale from user space looks something like this:  
> > root:/sys/bus/iio/devices/iio:device0> echo 0.000298 > in_voltage_scale .  
> > Furthermore, in your write_raw() function, val=0 and val2=298.
> > Knowing that full_scale_voltage = Vref / (gain * scale), we can calculate
> > the gain = Vref / (full_scale_voltage * scale). We only support two gains
> > (1 and 128), so we need to determine which one fits better with the
> > desired
> > scale. Finally, all we have left to do is to set the gain. 
> >    
> > >   2-) Could you give more details about the math around lines 346-348?
> > > Is it correct to assume that the multiplication at line 346 won't
> > > overflow? (vref is an uint)  
> > 
> > It is correct that Vref is in microvolts, so for example, Vref of 2.5V  =
> > 2500000000uV. It won't overflow since we use the Vref as nominator, while
> > full_scale_voltage and scale are the denominators.
> >   
> 
> [Regarding the AD7124] I guess I should be noted that the code can
> overflow, but in this case it shouldn't, because (according to the device
> datasheet) the maximum VREF can be 3600 mV.
> A user could specify (in the devicetree) something larger than 4300 mV (and
> that would overflow) but that would also make the readings useless as the
> external VREF would be invalid ; and there is also the risk of frying the
> chip in that case [something you really can't protect the user against].

Thanks Alex and Stefan for great detail here (glad you got to this before
I did ;)  One thing to add though is that if we have constraints like
this that would mess up the readings, it is sensible to check them
in the driver and at the very least put out a loud warning about the
problem, possibly even refuse to probe the driver.

The number of times I've spent hours chasing configuration problems
only to discover I was doing something stupid and the kernel silently
carried on regardlessly make me very keen on such commonsense protections.

I'm not going to review every driver to ensure they do this, but I'm keen
on them being checked where a possible problem has been identified!

Jonathan
> 
> The internal VREF however is 2500 mV, so things should be fine from that
> point of view.
> 
> Typically, in datasheets (at least from Analog Devices) it's good to take a
> look at the specifications sections.
> [For AD7124] You will see that the internal VREF [page 8] is 2.5V (with
> approximation of +/-0.2%) and for external reference it goes all the way up
> to AVDD, which has typical values of 2.9V - 3.6V.
> So, for u32 this code should be fine and not overflow.
> 
> One small thing that can be confusing about that code in AD7124 is that it
> gets multiplied with 1000000LL (which is signed long-long), but that should
> be fine, since the operation should be converted to u32 (unsigned int)
> representation [by being assigned to vref], which ends up being fine in the
> end.
> 
> > > 
> > > And regarding our code:
> > >   1-) The val in our write_raw function should be, in case of GAIN, a
> > > number that best approximate the actual gain value of 1 or 128? For
> > > instance, if the user inputs 126, we should default to 128?  
> > 
> > We should not allow the the user to input the gain, he needs to input the
> > scale (see the mail from Jonathan and the above explanation). However, if
> > the calculated gain is one that is not supported, such as 126, we will
> > set
> > the closest matching value, in this case 128.
> >   
> > >   2-) In the case of FILTER, is it the same? Is the user sending the
> > > value in mHz (milihertz)?  
> > 
> > Yes, it is the same with the FILTER. You need to add
> > a IIO_CHAN_INFO_SAMP_FREQ case in your write_raw() function. From user
> > space, the input value should be in Hz, something like this:  
> > root:/sys/bus/iio/devices/iio:device0> echo 10 >  
> > in_voltage_sampling_frequency
> >   
> > > 
> > > Thank you  

Powered by blists - more mailing lists