| Message ID | 20211214165608.7903-1-nuno.sa@analog.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add support for LTC2688 | expand |
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 >
Hi Jonathan, Somehow I failed to see these replies and only when I was about to send a v2, I remembered to double check :). So let's settle some remaining points and I'll send the v2 next week. > From: Jonathan Cameron <jic23@kernel.org> > Sent: Thursday, December 30, 2021 4:14 PM > To: Sa, Nuno <Nuno.Sa@analog.com> > Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; Rob > Herring <robh+dt@kernel.org>; Lars-Peter Clausen > <lars@metafoo.de>; Hennerich, Michael > <Michael.Hennerich@analog.com> > Subject: Re: [PATCH 0/3] Add support for LTC2688 > > [External] > > 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... Ok. out_voltageY_dither_offset works for me. I will add it to the dither ABI and will just return 0 (which might or might *not* be equal to out_voltageX_offset). > > > > 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. Fair enough. Let's then let it as-is. Even though not the most neat thing to do, we can always use a fixed clock to overcome the constrain anyways :) . > > > > 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. We can leave it as is for now. Honestly the main motivation for it was to express some dependencies in the bindings. Like the following pseudo code: if adi,mode == dither; then adi,toggle-dither-input depends on clocks This is naturally only possible if we make clocks a property of the channels object... > > > > 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. > I do think it might make sense to have clocks as part of the channel object as it is indeed a property of the channel. And as we are leaving the constrain for toggle mode channels (of providing a clock if a tgpx pin is provided) one neat thing to have in the bindings would be: dependencies: adi,toggle-dither-input ["clocks"] Rob, any input on the above? - Nuno Sá