On Tue, 14 Dec 2021 17:56:05 +0100 Nuno Sá <nuno.sa@xxxxxxxxxx> wrote: > The ABI defined for this driver has some subtleties that were previously > discussed in this RFC [1]. This might not be the final state but, > hopefully, we are close to it: > > toggle mode channels: > > * out_voltageY_toggle_en > * out_voltageY_raw1 > * out_voltageY_symbol > > dither mode channels: > > * out_voltageY_dither_en > * out_voltageY_dither_raw > * out_voltageY_dither_raw_available > * out_voltageY_dither_frequency > * out_voltageY_dither_frequency_available > * out_voltageY_dither_phase > * out_voltageY_dither_phase_available > > Default channels won't have any of the above ABIs. A channel is toggle > capable if the devicetree 'adi,toggle-mode' flag is set. For dither, the > assumption is more silent. If 'adi,toggle-mode' is not given and a > channel is associated with a TGPx pin through 'adi,toggle-dither-input', > then the channel is assumed to be dither capable (there's no point in > having a dither capable channel without an input clock). > > There are some stuff where I'm still not 100% convinced though: > > 1. out_voltageY_dither_raw refers to the dither amplitude. There are some > differences but in essence, the same scale as the raw attr applies. That > is not true for the offset as it's always 0. This is stated in the ABI > file and being an amplitude is more or less obvious. However, I'm not > sure if it's still valuable to have an ut_voltageY_dither_offset? I think if we have out_voltageY_offset then we should have out_voltageY_dither_offset to avoid any potential confusion + appropriate ABI docs text to make it clear that that the more specific _offset takes precedence. I have some vague recollection we had a debate about a similar case in the past where we had in_voltageX_offset that covered lots of channels and in_voltage99_offset (number made up) that just happened to be different. Not sure any driver takes advantage of ABI perhaps allowing (not sure it's written down) a more specific attribute to override a generic one... > > 2. For now, if 'adi,toggle-dither-input' is given, a correspondent clock > as to be given as well. While this makes sense for dither channels, I'm > not so sure for toggle ones. I can easily see a toggled channel being > controlled by, for example, an host GPIO. I agree. But I think we can relax this when needed rather than it being necessary to allow for more complex toggle conditions from the start. Generating a clock driven set of voltages is probably a common usecase for toggle mode so fine to just support that one until another usecase comes along. > > 3. Dither capable channels are being silently "assumed" by the driver. > Not sure if an "adi,mode" dt property would make sense. Having this > explicitly could make it easier to express some dependencies in the > bindings file. I kind of like the assumed default of toggle if the pin is wired up, but if you prefer an explicit control it becomes a question of whether Rob (and maybe others) think the binding is sane or not. > > 4. For now the clocks property is not part of the channels object. > The reason for this is that we only have 3 possible clocks for 16 > channels so I wanted to avoid getting and enabling the same clock more > than once. But that is not really an issue and together with 3) it > could, again, make it easier to express some dependencies in the bindings > file. That said, I'm pending in doing this property a channel one (as it > truly is) unless I get feedback otherwise. Interesting question on how to do this. Maybe a questions where Rob's input would be particularly useful. Likely to be similar cases somewhere else from a dt-binding point of view. Jonathan > > [1]: https://marc.info/?l=linux-iio&m=163662843603265&w=2 > > Nuno Sá (3): > iio: dac: add support for ltc2688 > iio: ABI: add ABI file for the LTC2688 DAC > dt-bindings: iio: Add ltc2688 documentation > > .../ABI/testing/sysfs-bus-iio-dac-ltc2688 | 67 + > .../bindings/iio/dac/adi,ltc2688.yaml | 146 +++ > MAINTAINERS | 9 + > drivers/iio/dac/Kconfig | 11 + > drivers/iio/dac/Makefile | 1 + > drivers/iio/dac/ltc2688.c | 1081 +++++++++++++++++ > 6 files changed, 1315 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2688 > create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yaml > create mode 100644 drivers/iio/dac/ltc2688.c >
