Re: [PATCH v2 3/4] staging: iio: ad5933: add ABI documentation

[Marcelo Schmitt]

On 03/03, Jonathan Cameron wrote:
> On Thu, 28 Feb 2019 23:53:14 -0300
> Marcelo Schmitt <marcelo.schmitt1@xxxxxxxxx> wrote:
> 
> > Add an ABI documentation for the ad5933 driver.
> > 
> > Signed-off-by: Marcelo Schmitt <marcelo.schmitt1@xxxxxxxxx>
> Hi Marcelo,
> 
> The ABI that you have defined which is actually new is mostly fine,
> however it seems that some of the existing ABI is not used correctly
> and that will need to be fixed.
> 
> Jonathan

Hi Jonathan,

Thanks for the comments.

> 
> > ---
> >  .../ABI/testing/sysfs-bus-iio-ad5933          | 91 +++++++++++++++++++
> >  1 file changed, 91 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-ad5933
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-ad5933 b/Documentation/ABI/testing/sysfs-bus-iio-ad5933
> > new file mode 100644
> > index 000000000000..81e3d3f6f724
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio-ad5933
> > @@ -0,0 +1,91 @@
> > +What:/sys/bus/iio/devices/iio:deviceX/out_voltage0_scale
> > +Date:		February 2019
> > +KernelVersion:	Kernel 4.19
> > +Contact:	linux-iio@xxxxxxxxxxxxxxx
> > +Description:
> > +		The output peak-to-peak voltage range. Writting 0 sets range
> > +		to 2.0V p-p typical, 1 sets range to 200mV p-p typical, 2 sets
> > +		range to 400mV p-p typical, 3 sets range to 1.0V p-p typical.
> > +		The p-p value of the ac output exitation voltage scales with 
> > +		supply voltage according to the following formula:
> > +		Output Excitation Voltage (V p-p) = normalized_3v3 × [VDD/3.3]
> > +		where normalized_3v3 is one of the four voltage range above and
> > +		VDD is the supply voltage.
> 
> Definitely not. The device must comply with standard ABI and this is not
> using it correctly (see below or the docs).
> 
> > +
> > +What:/sys/bus/iio/devices/iio:deviceX/out_voltage0_scale_available
> > +Date:		February 2019
> > +KernelVersion:	Kernel 4.19
> > +Contact:	linux-iio@xxxxxxxxxxxxxxx
> > +Description:	
> > +		Prints available peak-to-peak voltage range to buffer.
> 
> I really hope it doesn't.  Scale is a mulitplier not a range.  It should be
> printing out the value by which the _raw reading should be multiplied to
> get the voltage in the base units for voltage (millivolts)

As the driver is right now I believe that calling this device attribute will
print "1980, 970, 383, 198" which are the mean values of four distributions
(called range1 to range4) of possible actual millivolt values used as the AC
output excitation. Certainly they don't mean "the value by which the _raw
reading should be multiplied to get the voltage in the base units for voltage"
so, as you said, this ABI is probably being misused here. Hence, I guess
I ought to change this attribute's call to something different than 
out_voltage0_scale_available (as well as the out_voltage0_scale above). Maybe
out_voltage0_excit_available or out_voltage0_mean_excit_available, any other
suggestion?

> 
> > +
> > +What:/sys/bus/iio/devices/iio:deviceX/out_voltage0_freq_start
> > +Date:		February 2019
> > +KernelVersion:	Kernel 4.19
> > +Contact:	linux-iio@xxxxxxxxxxxxxxx
> > +Description:
> > +		The start frequency. Set this to define de frequency point at
> > +		which the device should start the next frequency sweep. Default
> > +		start frequency point set to 10000Hz.
> 
> No need to specify the default. It's trivial to read and any user of this
> chip should set it to the value they want.

OK, then I will remove these on the next patch.

> 
> > +
> > +What:/sys/bus/iio/devices/iio:deviceX/out_voltage0_freq_increment
> > +Date:		February 2019
> > +KernelVersion:	Kernel 4.19
> > +Contact:	linux-iio@xxxxxxxxxxxxxxx
> > +Description:
> > +		The frequency sweep increment. Set this to define at which rate
> > +		frequency sweep points are incremented. 
> Rate is a temporal term.  Perhaps "step by which the frequency is incremented"?

Maybe

"define the amount by which the frequency is incremented after each scan point."

would it be ok?

> 
> > After the measurement at
> > +		a frequency point is completed, the next measurement will be 
> > +		made with a frequency 'frequency increment'Hz higher than the
> > +		previous point until the defined number of increments has been
> > +		made. Default frequency increment set to 200Hz.
> 
> No need to specify the default. People can just read the file to find that out.
> 
> > +		
> > +
> > +What:/sys/bus/iio/devices/iio:deviceX/out_voltage0_freq_points
> > +Date:		February 2019
> > +KernelVersion:	Kernel 4.19
> > +Contact:	linux-iio@xxxxxxxxxxxxxxx
> > +Description:
> > +		The number of increments. This defines the number of frequency
> > +		points in the frequency sweep. Device stores a 9-bit integer
> > +		number in binary format for this so the maximum number of
> > +		increments that can be programmed is 511.
> I would relax this ABI description so that it doesn't describe the bit depth
> and ideally doesn't describe the range either.  If we want to provide the
> range, then provide an available attribute to pair with this.
> 
> The reason is that we should be looking to define interfaces in a way that
> allows them to be potentially generalised to similar devices in future.
> 
> I suspect a lot of this is generic to impedance measuring ICs.
> 

Then I think we would keep just the very beginning of it "The number of ... 
frequency sweep."

> > +
> > +What:/sys/bus/iio/devices/iio:deviceX/out_voltage0_settling_cycles
> > +Date:		February 2019
> > +KernelVersion:	Kernel 4.19
> > +Contact:	linux-iio@xxxxxxxxxxxxxxx
> > +Description:
> > +		Number of settling time cycles. This attribute is a 11 bit field
> > +		divided in two parts. The 9 least significant bit define the
> > +		number of output excitation cycles that are passed through the
> > +		unknown impedance, after the receipt of a start frequency sweep,
> > +		increment frequency, or repeat frequency command, before the ADC
> > +		is triggered to perform a conversion of the response signal. The
> > +		2 most significant bits define a multiplier for the number of
> > +		cycles obtained from de least significant bits. Let D10 and D9
> 
> de -> the 

oops, thanks for pointing it out.

> 
> > +		be these two bits, the resulting multiplier is defined as
> > +		follows.
> > +		D10 D9 = 0 0 => No. of cycles x 1 (default)
> > +		D10 D9 = 0 1 => No. of cycles x 2
> > +		D10 D9 = 1 0 => Reserved
> > +		D10 D9 = 1 1 => No. of cycles x 4
> > +
> > +		See the datasheet for detailed information.
> 
> Hmm. This is a bizare interface - but such is hardware.
> 
> However, just because it is bizare in the hardware doesn't mean the
> userspace interface should be.  Just hide all this complexity and
> map whatever number of cycles is input to the nearest possible value.
>

Trying to be generic I would say:

	Number of settling time cycles. This sets the delay between a start
	frequency sweep/increment frequency /repeat frequency to be proportional
	to the excitation signal frequency times the number of settling time
	cycles.
 
> > +
> > +What:/sys/bus/iio/devices/iio:deviceX/in_voltage0_scale
> > +Date:		February 2019
> > +KernelVersion:	Kernel 4.19
> > +Contact:	linux-iio@xxxxxxxxxxxxxxx
> > +Description:
> > +		The PGA gain amplifier for the response signal. Set this to 0
> > +		to gain the output of the current-to-voltage amplifier by a
> > +		factor of 5. Set to 1 (default) to amplify the response signal
> > +		into the ADC by a multiplication factor of x1.
> > +
> > +What:/sys/bus/iio/devices/iio:deviceX/in_voltage0_scale_available
> 
> There is no need to document standard ABI.  If this isn't documented in the
> main Documentation/ABI/testing/sysfs-bus-iio please add it there.

I guess this would be /sys/bus/iio/devices/iio:deviceX/in_voltage_scale.

> 
> > +Date:		February 2019
> > +KernelVersion:	Kernel 4.19
> > +Contact:	linux-iio@xxxxxxxxxxxxxxx
> > +Description:
> > +		Prints available PGA gain amplifier to buffer.
> Pga is one element that should effect this, but it's not the only
> one.  The resolution of the device also matters for example.
> 
>
 
Weren't this one described in /sys/.../iio:deviceX/in_voltageX_scale_available?