On 4/13/24 10:25 AM, Jonathan Cameron wrote: > On Fri, 12 Apr 2024 16:26:17 -0500 > David Lechner <dlechner@xxxxxxxxxxxx> wrote: > >> On Thu, Apr 11, 2024 at 10:21 PM Kim Seer Paller >> <kimseer.paller@xxxxxxxxxx> wrote: >>> >>> Define the sysfs interface for toggle capable channels. >>> >>> Toggle enabled channels will have: >>> >>> * out_voltageY_toggle_en > 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 generalized > beyond 1 device (unless you include PSK / FSK DDS drivers that are 'still' after > 13+ years in staging!) > > This patch needs to be generalizing that documentation from the ltc2688. > > Probably in sysfs-bus-iio-dac > >> >> 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 >> > > This is presumably the tricky one as that hardware toggle may not be in > control of the host CPU. > >> 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? > > This one feels like it could be handled as a software optimisation over > just changing the DAC value directly. > >> >> 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)? > > 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. > > 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. > >> >> 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? > > Again, precedence should have been called out. 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. > >> >>> >>> 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. > > 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. 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. > >> >>> * 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)? > > It's there in Documentation/ABI/testing/sysfs-bus-iio > Different form simple enable (which came much later as ABI) because > it means entering a powerdown state in which a particular thing happens > on the output pin. It is defined alongside powerdown_mode which > defines what happens. (often a choice between different impedance / High Z etc) > > >> >> >>> * out_voltageY_scale >>> * out_voltageY_offset >>> >>> Co-developed-by: Michael Hennerich <michael.hennerich@xxxxxxxxxx> >>> Signed-off-by: Michael Hennerich <michael.hennerich@xxxxxxxxxx> >>> Signed-off-by: Kim Seer Paller <kimseer.paller@xxxxxxxxxx> >>> --- >>> .../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/Documentation/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@xxxxxxxxxxxxxxx >>> +Description: >>> + Toggle enable. Write 1 to enable toggle or 0 to disable it. 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@xxxxxxxxxxxxxxx >>> +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 (_raw0) and INPUT_B (_raw1). The same scale and offset >>> + as in out_voltageY_raw applies. >>> + >>> +What: /sys/bus/iio/devices/iio:deviceX/out_voltageY_symbol >>> +KernelVersion: 5.18 >>> +Contact: linux-iio@xxxxxxxxxxxxxxx >>> +Description: >>> + Performs a SW toggle. This attribute is specific to toggle >>> + enabled channels and allows to toggle between out_voltageY_raw0 >>> + and out_voltageY_raw1 through software. Writing 0 will select >>> + 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@xxxxxxxxxx> >>> L: linux-iio@xxxxxxxxxxxxxxx >>> 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 >>> > Clearly I have a lot to learn on this one! Thanks for all of the info.
