Received-SPF: pass (google.com: domain of linux-kernel+bounces-145082-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <79734a9f54e320cd09aab83015f1c0e6d93e700c.camel@gmail.com>
Subject: Re: [PATCH 2/4] iio: ABI: add ABI file for the LTC2664 DAC
From: Nuno =?ISO-8859-1?Q?S=E1?= <noname.nuno@gmail.com>
To: Jonathan Cameron <jic23@kernel.org>, David Lechner
 <dlechner@baylibre.com>
Cc: Kim Seer Paller <kimseer.paller@analog.com>, linux-iio@vger.kernel.org, 
 devicetree@vger.kernel.org, linux-kernel@vger.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>
Date: Mon, 15 Apr 2024 14:45:52 +0200
In-Reply-To: <20240413162517.4644b649@jic23-huawei>
References: <20240412032102.136071-1-kimseer.paller@analog.com>
	 <20240412032102.136071-3-kimseer.paller@analog.com>
	 <CAMknhBE9XihK27pRhyPwTNM3VQX=osYdDyCmjNspz1aqe_NVTw@mail.gmail.com>
	 <20240413162517.4644b649@jic23-huawei>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.52.0 
Precedence: bulk
MIME-Version: 1.0

On Sat, 2024-04-13 at 16:25 +0100, Jonathan Cameron wrote:
> On Fri, 12 Apr 2024 16:26:17 -0500
> David Lechner <dlechner@baylibre.com> wrote:
>=20
> > On Thu, Apr 11, 2024 at 10:21=E2=80=AFPM Kim Seer Paller
> > <kimseer.paller@analog.com> wrote:
> > >=20
> > > Define the sysfs interface for toggle capable channels.
> > >=20
> > > Toggle enabled channels will have:
> > >=20
> > > =C2=A0* out_voltageY_toggle_en=C2=A0=20
> The big missing thing in this ABI is a reference to existing precedence.
> You aren't actually defining anything new, it just hasn't yet been genera=
lized
> beyond 1 device (unless you include PSK / FSK DDS drivers that are 'still=
'
> after
> 13+ years in staging!)
>=20
> This patch needs to be generalizing that documentation from the ltc2688.
>=20
> Probably in sysfs-bus-iio-dac
>=20
> >=20
> > It looks like there are 3 toggle modes.
> >=20
> > Two involve the notion of "enabled" outputs that I assume this attribut=
e is
> > for:
> >=20
> > 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
> >=20
>=20
> This is presumably the tricky one as that hardware toggle may not be in
> control of the host CPU.
>=20
> > 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.
> >=20
> > 3. Toggling all pins to A or B based on the toggle select register. No
> > notion of enabled pins here.
> >=20
> > 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?
>=20
> This one feels like it could be handled as a software optimisation over
> just changing the DAC value directly.

Things may be slightly different in these devices. But for ltc2688 and AFAI=
R,
the symbol attribute is about toggling between A and B through SW (not real=
ly
enabling the mode). That interface will only pop up if there's no HW (PWM f=
or
example) toggle present.

>=20
> >=20
> > 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).
> >=20
> > > =C2=A0* out_voltageY_raw0
> > > =C2=A0* out_voltageY_raw1=C2=A0=20
> >=20
> > 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)?
>=20
> There is precedence for doing it like this (ltc2688)
> Note that we should only see these attribute for changes specifically
> configured for 'hardware' triggered toggling.
>=20
> Note that we cannot have duplicate documentation so we need to create
> a common docs file covering this and existing ltc2688 ABI that overlaps.
> That may need some generalising to cover both parts.
>=20
> >=20
> > Or just have 8 channels instead of 4 where even channels are register
> > A and odd channels are register B?
> >=20
> > > =C2=A0* out_voltageY_symbol=C2=A0=20
> >=20
> > "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?
>=20
> Again, precedence should have been called out.=C2=A0 It's not great ABI
> but it corresponds to earlier work on Frequency Shift Keying DDS devices
> (and I think Phase Shift Keying as well) in which this is call symbol.
> Hence the name.
>=20
> >=20
> > >=20
> > > The common interface present in all channels is:
> > >=20
> > > =C2=A0* out_voltageY_raw (not present in toggle enabled channels)=C2=
=A0=20
> >=20
> > As mentioned above, I don't think we need to have to make a
> > distinction between toggle enabled channels and not enabled channels.
>=20
> Was a while back but I think that last time this turned up we concluded
> we did need a different interface because the current 'toggle value'
> is not in our control.=C2=A0 Hence you are programming one channel that
> does different things - think of it as setting the Max and Min values
> for a generated waveform - perhaps the toggle pin is connected to a PWM
> for example.
>=20

Yeah and for the ltc2688 we also had the dither mode that has it's own uniq=
ue
set of interfaces. Trying to deal with it all at runtime could be more conf=
using
than beneficial I guess.

- Nuno S=C3=A1
>=20