Re: [PATCH 2/2] iio: adc: adding support for MCP3564 ADC

[Marius.Cristea]

Hi Jonathan,

   Please see my comments bellow.

Thanks,
Marius

On Sat, 2023-05-20 at 16:15 +0100, Jonathan Cameron wrote:
> > +
> > +What:               
> > /sys/bus/iio/devices/iio:deviceX/calibscale_available
> > +KernelVersion:       6.4
> > +Contact:     linux-iio@xxxxxxxxxxxxxxx
> > +Description:
> > +             Reading returns a range with the possible values for
> > the gain
> > +             error digital calibration register.
> > +
> > +What:               
> > /sys/bus/iio/devices/iio:deviceX/boost_current
> > +KernelVersion:       6.4
> > +Contact:     linux-iio@xxxxxxxxxxxxxxx
> > +Description:
> > +             This attribute is used to set the biasing circuit of
> > the
> > +             Delta-Sigma modulator. The different BOOST settings
> > are applied
> > +             to the entire modulator circuit, including the
> > voltage reference
> > +             buffers.
> > +
> > +What:               
> > /sys/bus/iio/devices/iio:deviceX/boost_current_available
> > +KernelVersion:       6.4
> > +Contact:     linux-iio@xxxxxxxxxxxxxxx
> > +Description:
> > +             Reading returns a list with the possible values for
> > +             the current biasing circuit of the Delta-Sigma
> > modulator.
> > +
> > +             "x0.5",   - ADC channel has current x 0.5
> 
> Keep just numbers in the attr.  It should be named in a fashion that
> makes it apparent that it's a multiplier, not an absolute current.
> 
> New ABI like this is best avoided if we can. I see from a quick
> glance at the
> datasheet that there is advice on controlling this to allow for
> different
> clock settings, but I'm not sure if that's a simple relationship or
> not.
> From
> https://ww1.microchip.com/downloads/aemDocuments/documents/APID/ProductDocuments/DataSheets/MCP3461-2-4-Two-Four-Eight-Channel-153.6-ksps-Low-Noise-16-Bit-Delta-Sigma-ADC-Data-Sheet-20006180D.pdf
> figures 2-20 onwards, it looks like this effectively trades off power
> consumption
> against max frequency, so maybe we could set it automatically?
> 

Here being a trade off between power consumption and bandwidth I would
like it to be somehow programmable and let the user to set it. Maybe
the user wants to "monitor" a channel (have low power consumption) and
other channels to be benefit from a larger bandwidth. It don't think it
can be set automatically.

> 
> > +
> > +             "x0.66",  - ADC channel has current x 0.66
> > +
> > +             "x1",     - ADC channel has current x 1 (default)
> > +
> > +             "x2"      - ADC channel has current x 2
> > +
> > +What:                /sys/bus/iio/devices/iio:deviceX/current_bias
> 
> Another bit of custom ABI with the normal problems of it being
> unknown
> to standard userspace software.  You could perhaps use an output
> channel for
> this and just treat it like a current DAC, with a label that makes
> it's relationship
> to the inputs obvious.


I could use an output channel to setup the current. Is it OK to have
both ADC and "DAC" functionality in "iio/adc"?

> 
> > +KernelVersion:       6.4
> > +Contact:     linux-iio@xxxxxxxxxxxxxxx
> > +Description:
> > +             This attribute is used to set the current Source/Sink
> > +             values for sensor bias. The ADC inputs, VIN-/VIN+,
> > feature a
> > +             selectable burnout current source, which enables open
> > or
> > +             short-circuit detection, as well as biasing very low-
> > current
> > +             external sensors. The bias current is sourced on the
> > VIN+ pin of
> > +             the ADC (noninverting output of the analog
> > multiplexer)
> > +             and sunk on the VIN- pin of the ADC (inverting output
> > of
> > +             the analog multiplexer). Since the same current flows
> > +             at the VIN-/VIN+ pins of the ADC, it can sense the
> > +             impedance of an externally connected sensor that
> > +             would be connected between the selected inputs of the
> > +             multiplexer.
> > +
> > +What:               
> > /sys/bus/iio/devices/iio:deviceX/current_bias_available
> > +KernelVersion:       6.4
> > +Contact:     linux-iio@xxxxxxxxxxxxxxx
> > +Description:
> > +             Reading returns a list with the possible values for
> > +             the current source/sink selection values for sensor
> > bias.
> > +
> > +             "15_uA"        - 15 uA is applied to the ADC inputs.
> Match to standard IIO units for a current then drop the postfix.
> > +
> > +             "3.7_uA"       - 3.7 uA is applied to the ADC inputs.
> > +
> > +             "0.9_uA"       - 0.9 uA is applied to the ADC inputs.
> > +
> > +             "no_current"   - "No current source is applied to the
> > ADC
> > +             inputs (default)"
> 
> 0 would seem self explanatory for this an is easier for userspace
> software
> to deal with.  Also, this doc should not include the values, merely
> that
> there is a list.
> 
> 
> > +
> > +What:               
> > /sys/bus/iio/devices/iio:deviceX/enable_auto_zeroing_mux
> > +KernelVersion:       6.4
> > +Contact:     linux-iio@xxxxxxxxxxxxxxx
> > +Description:
> > +             This attribute is used to enable the analog input
> > multiplexer
> > +             auto-zeroing algorithm. Write '1' to enable it, write
> > '0' to
> > +             disable it.
> If you can explain what auto zeroing is that would be great. It's not
> something
> I recall seeing in other drivers.

I will add this description:
       "This attribute is used to enable the chopping algorithm for the
internal voltage reference buffer. This setting has no effect
when external voltage reference is selected.
Internal voltage reference buffer injects a certain quantity of
1/f noise into the system that can be modulated with the
incoming input signals and can limit the SNR performance at
higher Oversampling Ratio values (over 256). To overcome this
limitation, the buffer includes an auto-zeroing algorithm that
greatly reduces (cancels out) the 1/f noise and cancels the
offset value of the reference buffer. As a result, the SNR of
the system is not affected by this 1/f noise component of the
reference buffer, even at maximum oversampling ratio values.
Write '1' to enable it, write '0' to disable it."

> > +
> > +What:               
> > /sys/bus/iio/devices/iio:deviceX/enable_auto_zeroing_ref
> > +KernelVersion:       6.4
> > +Contact:     linux-iio@xxxxxxxxxxxxxxx
> > +Description:
> > +             This attribute is used to enable the chopping
> > algorithm for the
> > +             internal voltage reference buffer.  Write '1' to
> > enable it,
> > +             write '0' to disable it.
> > +
> > +What:                /sys/bus/iio/devices/iio:deviceX/hardwaregain
> > +KernelVersion:       6.4
> > +Contact:     linux-iio@xxxxxxxxxxxxxxx
> > +Description:
> > +             This attribute is used to set the hardware applied
> > gain factor.
> > +             It is shared across all channels.
> This attr is pretty much only used when the gain is not directly
> related to the
> value being used.  An example being a time of flight sensor where the
> amplitude
> of the measured signal doesn't directly matter as we are looking for
> the timing of the
> peak, we just need it to be big enough to measure. Otherwise scale is
> what you want.
> 

The MCP3564 has internally an input amplifier with programmable gain.
Because we can measure a difference between 2 channels, we have clients
that measure the output of load cells. This output is quite small am we
need to amplify it before conversion is done.

I was thinking that because we name it "hardwaregain" this will be be
somehow connected to a configurable gain that could be set in hardware.
Is better to have a separate info strictly related to  hardware
amplification.

>