Received-SPF: pass (google.com: domain of linux-kernel+bounces-143380-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240412032102.136071-1-kimseer.paller@analog.com> <20240412032102.136071-3-kimseer.paller@analog.com>
In-Reply-To: <20240412032102.136071-3-kimseer.paller@analog.com>
From: David Lechner <dlechner@baylibre.com>
Date: Fri, 12 Apr 2024 16:26:17 -0500
Message-ID: <CAMknhBE9XihK27pRhyPwTNM3VQX=osYdDyCmjNspz1aqe_NVTw@mail.gmail.com>
Subject: Re: [PATCH 2/4] iio: ABI: add ABI file for the LTC2664 DAC
To: Kim Seer Paller <kimseer.paller@analog.com>
Cc: linux-iio@vger.kernel.org, devicetree@vger.kernel.org, 
	linux-kernel@vger.kernel.org, Jonathan Cameron <jic23@kernel.org>, 
	Lars-Peter Clausen <lars@metafoo.de>, Rob Herring <robh@kernel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, 
	Liam Girdwood <lgirdwood@gmail.com>, Mark Brown <broonie@kernel.org>, 
	Michael Hennerich <michael.hennerich@analog.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Apr 11, 2024 at 10:21=E2=80=AFPM Kim Seer Paller
<kimseer.paller@analog.com> wrote:
>
> Define the sysfs interface for toggle capable channels.
>
> Toggle enabled channels will have:
>
>  * out_voltageY_toggle_en

It looks like there are 3 toggle modes.

Two involve the notion of "enabled" outputs that I assume this attribute is=
 for:

1. Toggling all enabled pins at the same time using a software trigger
(global toggle bit)
2. Toggling all enabled pins at the same time using a hardware trigger
(TGP pin) and toggling pins

The third mode though looks like it uses the same toggle select
register for selecting A or B for each channel instead of enabling or
disabling each channel.

3. Toggling all pins to A or B based on the toggle select register. No
notion of enabled pins here.

I haven't looked at the driver implementation, but it sounds like
out_voltageY_toggle_en and out_voltageY_symbol would be writing to the
same register in conflicting ways. So maybe we need yet another custom
attribute to select the currently active toggle mode?

In any case, it would be helpful if the documentation below explained
a bit better the intention and conditions required to use each
attribute (or add a .rst documentation file for these chips to explain
how to use it in more detail since this is rather complex feature).

>  * out_voltageY_raw0
>  * out_voltageY_raw1

I guess there is no enum iio_modifier that fits this. It seems like we
could still have out_voltageY_raw for register A so that users that
don't need to do any toggling can use standard ABI. And maybe
out_voltageY_raw_toggled for register B (question for Jonathan)?

Or just have 8 channels instead of 4 where even channels are register
A and odd channels are register B?

>  * out_voltageY_symbol

"symbol" is a confusing name. It sounds like this just supports
toggling one channel individually so _toggle_select would make more
sense to me. Are there plans for supporting toggling multiple channels
at the same time using a software trigger as well?

>
> The common interface present in all channels is:
>
>  * out_voltageY_raw (not present in toggle enabled channels)

As mentioned above, I don't think we need to have to make a
distinction between toggle enabled channels and not enabled channels.

>  * out_voltageY_raw_available
>  * out_voltageY_powerdown

Is _powerdown a standard attribute? I don't see it documented
anywhere. Perhaps you meant _en (via IIO_CHAN_INFO_ENABLE)?


>  * out_voltageY_scale
>  * out_voltageY_offset
>
> Co-developed-by: Michael Hennerich <michael.hennerich@analog.com>
> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> ---
>  .../ABI/testing/sysfs-bus-iio-dac-ltc2664     | 30 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 31 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2664
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2664 b/Docume=
ntation/ABI/testing/sysfs-bus-iio-dac-ltc2664
> new file mode 100644
> index 000000000..4b656b7af
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2664
> @@ -0,0 +1,30 @@
> +What:          /sys/bus/iio/devices/iio:deviceX/out_voltageY_toggle_en
> +KernelVersion: 5.18
> +Contact:       linux-iio@vger.kernel.org
> +Description:
> +               Toggle enable. Write 1 to enable toggle or 0 to disable i=
t. This is
> +               useful when one wants to change the DAC output codes. The=
 way it should
> +               be done is:
> +
> +               - disable toggle operation;
> +               - change out_voltageY_raw0 and out_voltageY_raw1;
> +               - enable toggle operation.
> +
> +What:          /sys/bus/iio/devices/iio:deviceX/out_voltageY_raw0
> +What:          /sys/bus/iio/devices/iio:deviceX/out_voltageY_raw1
> +KernelVersion: 5.18
> +Contact:       linux-iio@vger.kernel.org
> +Description:
> +               It has the same meaning as out_voltageY_raw. This attribu=
te is
> +               specific to toggle enabled channels and refers to the DAC=
 output
> +               code in INPUT_A (_raw0) and INPUT_B (_raw1). The same sca=
le and offset
> +               as in out_voltageY_raw applies.
> +
> +What:          /sys/bus/iio/devices/iio:deviceX/out_voltageY_symbol
> +KernelVersion: 5.18
> +Contact:       linux-iio@vger.kernel.org
> +Description:
> +               Performs a SW toggle. This attribute is specific to toggl=
e
> +               enabled channels and allows to toggle between out_voltage=
Y_raw0
> +               and out_voltageY_raw1 through software. Writing 0 will se=
lect
> +               out_voltageY_raw0 while 1 selects out_voltageY_raw1.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index bd8645f6e..9ed00b364 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12842,6 +12842,7 @@ M:      Kim Seer Paller <kimseer.paller@analog.co=
m>
>  L:     linux-iio@vger.kernel.org
>  S:     Supported
>  W:     https://ez.analog.com/linux-software-drivers
> +F:     Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2664
>  F:     Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml
>
>  LTC2688 IIO DAC DRIVER
> --
> 2.34.1
>