Re: [PATCH v2 2/3] iio: ABI: add ABI file for the LTC2688 DAC

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 15 Jan 2022 10:27:04 +0100
Nuno Sá <nuno.sa@analog.com> wrote:

> Define the sysfs interface for toggle or dither capable channels. Dither
> capable channels will have the extended interface:
> 
>  * out_voltageY_dither_en
>  * out_voltageY_dither_raw
>  * out_voltageY_dither_offset
>  * out_voltageY_dither_raw_available
>  * out_voltageY_dither_frequency
>  * out_voltageY_dither_frequency_available
>  * out_voltageY_dither_phase
>  * out_voltageY_dither_phase_available
> 
> Toggle enabled channels will have:
> 
>  * out_voltageY_toggle_en
>  * out_voltageY_raw0
>  * out_voltageY_raw1
>  * out_voltageY_symbol

Maybe worth just stating the normal interface here as well because
it's not clear from these examples if
out_voltageY_raw still exists for toggle enabled channels (I'm assuming not?)

> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
ABI seems good to me, just a few comments on details of the documentation.

Thanks,

Jonathan
> ---
>  .../ABI/testing/sysfs-bus-iio-dac-ltc2688     | 80 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 81 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2688
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2688 b/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2688
> new file mode 100644
> index 000000000000..38d1df81c6dc
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2688
> @@ -0,0 +1,80 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_en
> +KernelVersion:	5.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Dither enable. Write 1 to enable dither or 0 to disable it.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_raw
> +KernelVersion:	5.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		This raw, unscaled value refers to the dither signal amplitude.
> +		The same scale as in out_voltageY_raw applies. However, the
> +		offset might be different as it's always 0 for this attribute.

We'll have to be careful if we ever generalize these docs but what you have here
is fine whilst it applies to just this device.

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_raw_available
> +KernelVersion:	5.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Available range for dither raw amplitude values.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_offset
> +KernelVersion:	5.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Offset applied to out_voltageY_dither_raw. Read only attribute
> +		always set to 0.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_frequency
> +KernelVersion:	5.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Sets the dither signal frequency.
Units.

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_frequency_available
> +KernelVersion:	5.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Returns the available values for the dither frequency.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_phase
> +KernelVersion:	5.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Sets the dither signal phase.

Units.  Radians, or refer to the main _phase docs and say it's the same.

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_dither_phase_available
> +KernelVersion:	5.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Returns the available values for the dither phase.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_toggle_en
> +KernelVersion:	5.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Toggle enable. Write 1 to enable toggle or 0 to disable it.

Say why this is useful (presumably toggle with a clock rather than via _symbol)

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_raw0
> +KernelVersion:	5.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		It has the same meaning as out_voltageY_raw. This attribute is
> +		specific to toggle enabled channels and refers to the DAC output
> +		code in INPUT_A. The same scale, offset, etc applies.

Same as what?

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_raw1
> +KernelVersion:	5.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Same as out_voltageY_raw0 but referring to the DAC output code
> +		in INPUT_B.

You could combine this with previous and have two what lines.  Might allow
a slightly more compact clear description.


> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_symbol

Ah. That answers one of my binding related questions :)  You have kept
software control as an option for toggle.

> +KernelVersion:	5.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Performs a SW toggle. This attribute is specific to toggle
> +		enabled channels and allows to toggle between out_voltageY_raw

_raw0

> +		and out_voltageY_raw1 through software. Writing 0 will select
> +		out_voltageY_raw while 1 selects out_voltageY_raw1.
_raw0

> diff --git a/MAINTAINERS b/MAINTAINERS
> index 16e344d52b1e..16a7fd7f98ee 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11187,6 +11187,7 @@ M:	Nuno Sá <nuno.sa@analog.com>
>  L:	linux-iio@vger.kernel.org
>  S:	Supported
>  W:	http://ez.analog.com/community/linux-device-drivers
> +F:	Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2688
>  F:	drivers/iio/dac/ltc2688.c
>  
>  LTC2947 HARDWARE MONITOR DRIVER