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

[Jonathan Cameron]

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

> ---
>  .../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)

> +
> +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.

> +
> +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"?

> 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.

> +
> +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 

> +		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.

> +
> +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.

> +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.