Re: [PATCH 0/3] Add support for LTC2688

Jonathan Cameron
2021-12-30T15:08:15Z

On Tue, 14 Dec 2021 17:56:05 +0100
Nuno Sá <nuno.sa@analog.com> 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
>