Re: [PATCH v4 2/3] Documentation/ABI/testing: Add documentation for AD5766 new ABI

[Jonathan Cameron]

On Fri, 18 Dec 2020 19:12:30 +0200
Cristian Pop <cristian.pop@xxxxxxxxxx> wrote:

> New interface is proposed for dither functionality. This future allows
> composing  an external signals to the selected output channel.
> The dither signal can be turned on/off, scaled, inverted, or it can be
> selected from different sources.
> 
> Signed-off-by: Cristian Pop <cristian.pop@xxxxxxxxxx>
Coming together nicely though a few more thoughts came to mind when reading
this. Only significant one is whether we are better with decimal scaling
than percentage scaling for consistency with channel scales etc?

Jonathan

> ---
> Changelog v4:
> 	- Change to "in_voltageY_dither_enable"
> 	- Change scale to numbers + 100% for no scaling
> 	- Possible dither source values: 0 - N0, 1 - N1 
>  .../ABI/testing/sysfs-bus-iio-dac-ad5766      | 36 +++++++++++++++++++
>  1 file changed, 36 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dac-ad5766
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-dac-ad5766 b/Documentation/ABI/testing/sysfs-bus-iio-dac-ad5766
> new file mode 100644
> index 000000000000..72100edb88bb
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-dac-ad5766
> @@ -0,0 +1,36 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_dither_pwr

in_voltageY_dither_en would be more consistent with existing ABI naming.

> +KernelVersion:
> +Contact:	linux-iio@xxxxxxxxxxxxxxx
> +Description:
> +		Dither enable. Write 1 to enable dither or 0 to disable it.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_dither_invert
> +KernelVersion:
> +Contact:	linux-iio@xxxxxxxxxxxxxxx
> +Description:
> +		Inverts the dither applied to the selected DAC channel. Dither is not
> +		inverted by default. Write "1" to invert dither.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_dither_scale_available
> +KernelVersion:
> +Contact:	linux-iio@xxxxxxxxxxxxxxx
> +Description:
> +		Returns possible scalings available for the current channel:

Given this one is self defining and may well get generalized to other devices I'd
got with a description that doesn't provide values.

Returns possible scalings as percentages.

However, thinking more on this it's a bit inconsistent with other scale values we have
which are given in decimal.  Would "1 0.75 0.5 0.25" work as well here?

> +		"100 75 50 25" scaling.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_dither_scale
> +KernelVersion:
> +Contact:	linux-iio@xxxxxxxxxxxxxxx
> +Description:
> +		Scales the dither before it is applied to the selected channel:
> +		100 - No scaling
> +		75 - 75% scaling
> +		50 - 50% scaling
> +		25 - 25% scaling

As above, I'm don't really see an advantage in providing the values.  Any userspace
ought to cope with any (finding out what is there via _available).
"Scales the dither before it is applied to the selected channel.  1 corresponds
to no scaling".

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_dither_source
> +KernelVersion:

Please fill in the expected kernel version.  I'll fix it if this happens to merge near
the change over in a cycle, but it's easier to fix something that is there
than to add it from scratch :)

> +Contact:	linux-iio@xxxxxxxxxxxxxxx
> +Description:
> +		Selects dither source applied to the selected channel. Write "0" to
> +		select N0 source, write "1" to select N1 source.