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

[Jonathan Cameron <jic23@kernel.org>]

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
>