| Message ID | 20240412032102.136071-1-kimseer.paller@analog.com |
|---|---|
| Headers | show |
| Series | Add driver for LTC2664 and LTC2672 | expand |
On Thu, Apr 11, 2024 at 10:21 PM Kim Seer Paller <kimseer.paller@analog.com> wrote: > > Define the sysfs interface for toggle capable channels. > > Toggle enabled channels will have: > > * out_voltageY_toggle_en 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 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? 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)? 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? > > 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. > * 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)? > * out_voltageY_scale > * out_voltageY_offset > > Co-developed-by: Michael Hennerich <michael.hennerich@analog.com> > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com> > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com> > --- > .../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@vger.kernel.org > +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@vger.kernel.org > +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@vger.kernel.org > +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@analog.com> > L: linux-iio@vger.kernel.org > 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 >
On Fri, 12 Apr 2024 16:26:17 -0500 David Lechner <dlechner@baylibre.com> wrote: > On Thu, Apr 11, 2024 at 10:21 PM Kim Seer Paller > <kimseer.paller@analog.com> 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@analog.com> > > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com> > > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com> > > --- > > .../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@vger.kernel.org > > +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@vger.kernel.org > > +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@vger.kernel.org > > +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@analog.com> > > L: linux-iio@vger.kernel.org > > 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 > >
On Fri, 12 Apr 2024 11:21:01 +0800 Kim Seer Paller <kimseer.paller@analog.com> wrote: > Define the sysfs interface for toggle capable channels. > > Toggle enabled channels will have: > > * out_currentY_toggle_en > * out_currentY_raw0 > * out_currentY_raw1 > * out_currentY_symbol > > The common interface present in all channels is: > > * out_currentY_raw (not present in toggle enabled channels) > * out_currentY_raw_available > * out_currentY_powerdown > * out_currentY_scale > * out_currentY_offset > > Co-developed-by: Michael Hennerich <michael.hennerich@analog.com> > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com> > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com> > --- > .../ABI/testing/sysfs-bus-iio-dac-ltc2672 | 30 +++++++++++++++++++ You can only have per device ABI defined if that is the only user of the ABI. That may actually be true here but given I've asked you to generalize the voltage equivalent, I think we've shown this is general enough that the current version should also be raised to sysfs-bus-iio-dac > MAINTAINERS | 1 + > 2 files changed, 31 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2672 > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2672 b/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2672 > new file mode 100644 > index 000000000..b984d92f7 > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2672 > @@ -0,0 +1,30 @@ > +What: /sys/bus/iio/devices/iio:deviceX/out_currentY_toggle_en > +KernelVersion: 5.18 > +Contact: linux-iio@vger.kernel.org > +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_currentY_raw0 and out_currentY_raw1; > + - enable toggle operation. > + > +What: /sys/bus/iio/devices/iio:deviceX/out_currentY_raw0 > +What: /sys/bus/iio/devices/iio:deviceX/out_currentY_raw1 > +KernelVersion: 5.18 > +Contact: linux-iio@vger.kernel.org > +Description: > + It has the same meaning as out_currentY_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_currentY_raw applies. > + > +What: /sys/bus/iio/devices/iio:deviceX/out_currentY_symbol > +KernelVersion: 5.18 > +Contact: linux-iio@vger.kernel.org > +Description: > + Performs a SW toggle. This attribute is specific to toggle > + enabled channels and allows to toggle between out_currentY_raw0 > + and out_currentY_raw1 through software. Writing 0 will select > + out_currentY_raw0 while 1 selects out_currentY_raw1. > diff --git a/MAINTAINERS b/MAINTAINERS > index 9ed00b364..fba8bacc0 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -12843,6 +12843,7 @@ L: linux-iio@vger.kernel.org > S: Supported > W: https://ez.analog.com/linux-software-drivers > F: Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2664 > +F: Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2672 > F: Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml > > LTC2688 IIO DAC DRIVER
On Fri, 12 Apr 2024 11:21:02 +0800 Kim Seer Paller <kimseer.paller@analog.com> wrote: > LTC2664 4 channel, 16 bit Voltage Output SoftSpan DAC > LTC2672 5 channel, 16 bit Current Output Softspan DAC > > Co-developed-by: Michael Hennerich <michael.hennerich@analog.com> > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com> > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com> Hi Most of the questions are in the earlier patch reviews in this series, so just minor stuff in here. Jonathan > diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile > index 55bf89739..62df8d7e4 100644 > --- a/drivers/iio/dac/Makefile > +++ b/drivers/iio/dac/Makefile > @@ -35,6 +35,7 @@ obj-$(CONFIG_DS4424) += ds4424.o > obj-$(CONFIG_LPC18XX_DAC) += lpc18xx_dac.o > obj-$(CONFIG_LTC1660) += ltc1660.o > obj-$(CONFIG_LTC2632) += ltc2632.o > +obj-$(CONFIG_LTC2664) += ltc2664.o > obj-$(CONFIG_LTC2688) += ltc2688.o > obj-$(CONFIG_M62332) += m62332.o > obj-$(CONFIG_MAX517) += max517.o > diff --git a/drivers/iio/dac/ltc2664.c b/drivers/iio/dac/ltc2664.c > new file mode 100644 > index 000000000..70c43fe17 > --- /dev/null > +++ b/drivers/iio/dac/ltc2664.c > @@ -0,0 +1,785 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * LTC2664 4 channel, 16 bit Voltage Output SoftSpan DAC driver > + * LTC2672 5 channel, 16 bit Current Output Softspan DAC driver > + * > + * Copyright 2024 Analog Devices Inc. > + */ > + > +#include <linux/bitfield.h> > +#include <linux/cleanup.h> > +#include <linux/device.h> > +#include <linux/gpio/consumer.h> > +#include <linux/iio/iio.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/mod_devicetable.h> > +#include <linux/mutex.h> > +#include <linux/property.h> > +#include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > +#include <linux/spi/spi.h> > + > +#define LTC2664_CMD_WRITE_N(n) (0x00 + (n)) > +#define LTC2664_CMD_UPDATE_N(n) (0x10 + (n)) > +#define LTC2664_CMD_WRITE_N_UPDATE_ALL 0x20 > +#define LTC2664_CMD_WRITE_N_UPDATE_N(n) (0x30 + (n)) > +#define LTC2664_CMD_POWER_DOWN_N(n) (0x40 + (n)) > +#define LTC2664_CMD_POWER_DOWN_ALL 0x50 > +#define LTC2664_CMD_SPAN_N(n) (0x60 + (n)) > +#define LTC2664_CMD_CONFIG 0x70 > +#define LTC2664_CMD_MUX 0xB0 > +#define LTC2664_CMD_TOGGLE_SEL 0xC0 > +#define LTC2664_CMD_GLOBAL_TOGGLE 0xD0 > +#define LTC2664_CMD_NO_OPERATION 0xF0 > +#define LTC2664_REF_DISABLE 0x0001 > +#define LTC2664_MSPAN_SOFTSPAN 7 > + > +#define LTC2672_MAX_CHANNEL 5 > +#define LTC2672_MAX_SPAN 7 > +#define LTC2672_OFFSET_CODE 384 Comment for OFFSET_CODE would be good. I'm not sure what it is or where the value comes from. > +#define LTC2672_SCALE_MULTIPLIER(n) (50 * BIT(n)) > + > +enum ltc2664_ids { > + LTC2664, > + LTC2672, I've mentioned it inline, but this is often a bad sign as it means that some control logic is used in place of data in a chip_info structure. Better to avoid tying what happens directly to IDs, but instead have the chip_info structure cover the particular set of things that a device does. That ends up a lot easier to extend when a driver gains additional part support. > +}; > + > +static const u16 ltc2664_mspan_lut[8][2] = { > + {LTC2664_SPAN_RANGE_M10V_10V, 32768}, /* MPS2=0, MPS1=0, MSP0=0 (0)*/ > + {LTC2664_SPAN_RANGE_M5V_5V, 32768}, /* MPS2=0, MPS1=0, MSP0=1 (1)*/ > + {LTC2664_SPAN_RANGE_M2V5_2V5, 32768}, /* MPS2=0, MPS1=1, MSP0=0 (2)*/ > + {LTC2664_SPAN_RANGE_0V_10V, 0}, /* MPS2=0, MPS1=1, MSP0=1 (3)*/ > + {LTC2664_SPAN_RANGE_0V_10V, 32768}, /* MPS2=1, MPS1=0, MSP0=0 (4)*/ > + {LTC2664_SPAN_RANGE_0V_5V, 0}, /* MPS2=1, MPS1=0, MSP0=1 (5)*/ > + {LTC2664_SPAN_RANGE_0V_5V, 32768}, /* MPS2=1, MPS1=1, MSP0=0 (6)*/ > + {LTC2664_SPAN_RANGE_0V_5V, 0} /* MPS2=1, MPS1=1, MSP0=1 (7)*/ As below, spaces after { and before } > +}; > + > +struct ltc2664_chip_info { > + enum ltc2664_ids id; > + const char *name; > + unsigned int num_channels; > + const struct iio_chan_spec *iio_chan; > + const int (*span_helper)[2]; > + unsigned int num_span; > +}; > + > +static const int ltc2664_span_helper[][2] = { > + {0, 5000}, {0, 10000}, {-5000, 5000}, {-10000, 10000}, {-2500, 2500}, > +}; > + > +static const int ltc2672_span_helper[][2] = { > + {0, 3125}, {0, 6250}, {0, 12500}, {0, 25000}, {0, 50000}, {0, 100000}, > + {0, 200000}, {0, 300000}, Spaces preferred and better on separate lines. { 0, 3125 }, { 0, 6250 }, etc as more readable. > +}; > + > +static int ltc2664_scale_get(const struct ltc2664_state *st, int c, int *val) > +{ > + const struct ltc2664_chan *chan = &st->channels[c]; > + const int (*span_helper)[2] = st->chip_info->span_helper; > + int span, fs; > + > + span = chan->span; > + if (span < 0) > + return span; > + > + switch (st->chip_info->id) { Make this 'data' rather than using an ID. There shouldn't be an ID in chip_info because it almost always means that a coding pattern has been used that isn't as extensible as either an appropriate callback or appropriate data. > + case LTC2664: > + fs = span_helper[span][1] - span_helper[span][0]; > + > + if (st->vref) How do we get here with st->vref == 0 ? > + *val = (fs / 2500) * st->vref; > + else > + *val = fs; > + > + return 0; > + case LTC2672: > + if (span == LTC2672_MAX_SPAN) > + *val = 4800 * (1000 * st->vref / st->rfsadj); > + else > + *val = LTC2672_SCALE_MULTIPLIER(span) * > + (1000 * st->vref / st->rfsadj); Seem to divide by same thing so add a local variable for that and use it in both paths. > + > + return 0; > + default: > + return -EINVAL; > + } > +} > + > +static int ltc2664_offset_get(const struct ltc2664_state *st, int c, int *val) > +{ > + const struct ltc2664_chan *chan = &st->channels[c]; > + int span; > + > + span = chan->span; > + if (span < 0) > + return span; > + > + if (st->chip_info->span_helper[span][0] < 0) > + *val = -32768; > + else if (chan->raw[0] >= LTC2672_OFFSET_CODE || > + chan->raw[1] >= LTC2672_OFFSET_CODE) This would benefit from a comment. I'm not sure what it is doing. > + *val = st->chip_info->span_helper[1][span] / 250; > + else > + *val = 0; > + > + return 0; > +} ... > +/* > + * For toggle mode we only expose the symbol attr (sw_toggle) in case a TGPx is > + * not provided in dts. > + */ > +static const struct iio_chan_spec_ext_info ltc2664_toggle_sym_ext_info[] = { > + LTC2664_CHAN_EXT_INFO("raw0", LTC2664_INPUT_A, IIO_SEPARATE, > + ltc2664_dac_input_read, ltc2664_dac_input_write), > + LTC2664_CHAN_EXT_INFO("raw1", LTC2664_INPUT_B, IIO_SEPARATE, > + ltc2664_dac_input_read, ltc2664_dac_input_write), > + LTC2664_CHAN_EXT_INFO("powerdown", LTC2664_POWERDOWN, IIO_SEPARATE, > + ltc2664_reg_bool_get, ltc2664_reg_bool_set), > + LTC2664_CHAN_EXT_INFO("symbol", LTC2664_GLOBAL_TOGGLE, IIO_SEPARATE, > + ltc2664_reg_bool_get, ltc2664_reg_bool_set), > + LTC2664_CHAN_EXT_INFO("toggle_en", LTC2664_TOGGLE_EN, > + IIO_SEPARATE, ltc2664_reg_bool_get, > + ltc2664_reg_bool_set), > + {} > +}; > + > +static const struct iio_chan_spec_ext_info ltc2664_ext_info[] = { > + LTC2664_CHAN_EXT_INFO("powerdown", LTC2664_POWERDOWN, IIO_SEPARATE, > + ltc2664_reg_bool_get, ltc2664_reg_bool_set), as mentioned in the ABI review, powerdown usually comes with a matching powerdown_mode to say what state the DAC output enters on power down. Here there is only one option, so should have a read only Ext info attribute to say it is always 42kohm_to_gnd You should also add that as an option to the main docs. Perhaps this is the point where we define that as a catch all rather than listing every value though. > + {} > +}; ... > +static int ltc2664_channel_config(struct ltc2664_state *st) > +{ > + const struct ltc2664_chip_info *chip_info = st->chip_info; > + struct device *dev = &st->spi->dev; > + u32 reg, tmp[2], mspan; > + int ret, span; > + > + mspan = LTC2664_MSPAN_SOFTSPAN; > + ret = device_property_read_u32(dev, "adi,manual-span-operation-config", > + &mspan); > + if (!ret) { > + if (chip_info->id != LTC2664) Check for a flag that says this config is supported or not, rather than ID. To support new devices we can just add the flag to their chip_info rather than having to change the ID checks throughout the code. > + return dev_err_probe(dev, -EINVAL, > + "adi,manual-span-operation-config not supported\n"); > + > + if (mspan > ARRAY_SIZE(ltc2664_mspan_lut)) > + return dev_err_probe(dev, -EINVAL, > + "adi,manual-span-operation-config not in range\n"); > + } > + > + st->rfsadj = 20000; > + ret = device_property_read_u32(dev, "adi,rfsadj-ohms", &st->rfsadj); > + if (!ret) { > + if (chip_info->id != LTC2672) As before. Use a flag, not an ID match. > + return dev_err_probe(dev, -EINVAL, > + "adi,rfsadj-ohms not supported\n"); > + > + if ((st->rfsadj < 19000 || st->rfsadj > 41000) || > + chip_info->id != LTC2672) You already checked the ID. > + return dev_err_probe(dev, -EINVAL, > + "adi,rfsadj-ohms not in range\n"); > + } > + > + device_for_each_child_node_scoped(dev, child) { > + struct ltc2664_chan *chan; > + > + ret = fwnode_property_read_u32(child, "reg", ®); > + if (ret) > + return dev_err_probe(dev, ret, > + "Failed to get reg property\n"); > + > + if (reg >= chip_info->num_channels) > + return dev_err_probe(dev, -EINVAL, > + "reg bigger than: %d\n", > + chip_info->num_channels); > + > + chan = &st->channels[reg]; > + > + if (fwnode_property_read_bool(child, "adi,toggle-mode")) { > + chan->toggle_chan = true; > + /* assume sw toggle ABI */ > + st->iio_channels[reg].ext_info = ltc2664_toggle_sym_ext_info; > + /* > + * Clear IIO_CHAN_INFO_RAW bit as toggle channels expose > + * out_voltage/current_raw{0|1} files. > + */ > + __clear_bit(IIO_CHAN_INFO_RAW, > + &st->iio_channels[reg].info_mask_separate); > + } > + > + chan->raw[0] = ltc2664_mspan_lut[mspan][1]; > + chan->raw[1] = ltc2664_mspan_lut[mspan][1]; > + > + chan->span = ltc2664_mspan_lut[mspan][0]; > + > + ret = fwnode_property_read_u32_array(child, "adi,output-range-microvolt", > + tmp, ARRAY_SIZE(tmp)); > + if (!ret && mspan == LTC2664_MSPAN_SOFTSPAN) { I don't like this pattern of doing things only on error. Probably easiet is to factor each of these blocks out as a function to call and then you can just return from that if !ret. Will end up more redable than this is. > + /* voltage type measurement */ > + st->iio_channels[reg].type = IIO_VOLTAGE; > + > + span = ltc2664_span_lookup(st, (int)tmp[0] / 1000, > + tmp[1] / 1000); > + if (span < 0) > + return dev_err_probe(dev, -EINVAL, > + "output range not valid"); > + > + ret = regmap_write(st->regmap, > + LTC2664_CMD_SPAN_N(reg), > + span); > + if (ret) > + return dev_err_probe(dev, -EINVAL, > + "failed to set chan settings\n"); > + > + chan->span = span; > + } > + > + ret = fwnode_property_read_u32(child, > + "adi,output-range-microamp", > + &tmp[0]); > + if (!ret) { > + /* current type measurement */ > + st->iio_channels[reg].type = IIO_CURRENT; > + > + span = ltc2664_span_lookup(st, 0, > + (int)tmp[0] / 1000); > + if (span < 0) > + return dev_err_probe(dev, -EINVAL, > + "output range not valid"); > + > + ret = regmap_write(st->regmap, > + LTC2664_CMD_SPAN_N(reg), > + span + 1); > + if (ret) > + return dev_err_probe(dev, -EINVAL, > + "failed to set chan settings\n"); > + > + chan->span = span; > + } > + } > + > + return 0; > +} > + > +static int ltc2664_setup(struct ltc2664_state *st, struct regulator *vref) > +{ > + const struct ltc2664_chip_info *chip_info = st->chip_info; > + struct gpio_desc *gpio; > + int ret; > + > + /* > + * If we have a clr/reset pin, use that to reset the chip. Single line comment style is /* If we have.. chip. */ > + */ > + gpio = devm_gpiod_get_optional(&st->spi->dev, "clr", GPIOD_OUT_HIGH); > + if (IS_ERR(gpio)) > + return dev_err_probe(&st->spi->dev, PTR_ERR(gpio), > + "Failed to get reset gpio"); > + if (gpio) { > + usleep_range(1000, 1200); > + /* bring device out of reset */ I'd argue that comment is obvious enough we don't need it. > + gpiod_set_value_cansleep(gpio, 0); > + } > + > + /* > + * Duplicate the default channel configuration as it can change during > + * @ltc2664_channel_config() > + */ > + st->iio_channels = devm_kmemdup(&st->spi->dev, chip_info->iio_chan, > + chip_info->num_channels * > + sizeof(*chip_info->iio_chan), > + GFP_KERNEL); Is that big enough? I'd expect 1 more entry as a null terminator. > + > + ret = ltc2664_channel_config(st); > + if (ret) > + return ret; > + > + if (!vref) > + return 0; > + > + return regmap_set_bits(st->regmap, LTC2664_CMD_CONFIG, LTC2664_REF_DISABLE); > +} ... > +static int ltc2664_probe(struct spi_device *spi) > +{ > + static const char * const regulators[] = { "vcc", "iovcc" }; > + const struct ltc2664_chip_info *chip_info; > + struct device *dev = &spi->dev; > + struct regulator *vref_reg; > + struct iio_dev *indio_dev; > + struct ltc2664_state *st; > + int ret; > + > + indio_dev = devm_iio_device_alloc(dev, sizeof(*st)); > + if (!indio_dev) > + return -ENOMEM; > + > + st = iio_priv(indio_dev); > + st->spi = spi; > + > + chip_info = spi_get_device_match_data(spi); > + if (!chip_info) > + return -ENOMEM; > + > + st->chip_info = chip_info; > + > + mutex_init(&st->lock); > + > + st->regmap = devm_regmap_init_spi(spi, <c2664_regmap_config); > + if (IS_ERR(st->regmap)) > + return dev_err_probe(dev, PTR_ERR(st->regmap), > + "Failed to init regmap"); > + > + ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulators), > + regulators); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to enable regulators\n"); > + > + vref_reg = devm_regulator_get_optional(dev, "vref"); > + if (IS_ERR(vref_reg)) { > + if (PTR_ERR(vref_reg) != -ENODEV) > + return dev_err_probe(dev, PTR_ERR(vref_reg), > + "Failed to get vref regulator"); > + > + vref_reg = NULL; > + > + /* internal reference */ > + if (chip_info->id == LTC2664) Push all chip specific data into the chip_info structure. Never match on ID value as it is extensible as you add more devices to a driver over time. st->vref = chip_info->internal_vref; or something like that. > + st->vref = 2500; > + else > + st->vref = 1250; > + } else { > + ret = regulator_enable(vref_reg); > + if (ret) > + return dev_err_probe(dev, ret, > + "Failed to enable vref regulators\n"); > + > + ret = devm_add_action_or_reset(dev, ltc2664_disable_regulator, > + vref_reg); > + if (ret) > + return ret; > + > + ret = regulator_get_voltage(vref_reg); > + if (ret < 0) > + return dev_err_probe(dev, ret, "Failed to get vref\n"); > + > + st->vref = ret / 1000; > + } > + > + ret = ltc2664_setup(st, vref_reg); > + if (ret) > + return ret; > + > + indio_dev->name = chip_info->name; > + indio_dev->info = <c2664_info; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->channels = st->iio_channels; > + indio_dev->num_channels = chip_info->num_channels; > + > + return devm_iio_device_register(dev, indio_dev); > +} > + > +static const struct spi_device_id ltc2664_id[] = { > + { "ltc2664", (kernel_ulong_t)<c2664_chip }, > + { "ltc2672", (kernel_ulong_t)<c2672_chip }, > + { }, No trailing commas needed or desirable on terminating entries like this. We will never add anything after them. Jonathan
On 4/13/24 10:25 AM, Jonathan Cameron wrote: > On Fri, 12 Apr 2024 16:26:17 -0500 > David Lechner <dlechner@baylibre.com> wrote: > >> On Thu, Apr 11, 2024 at 10:21 PM Kim Seer Paller >> <kimseer.paller@analog.com> 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@analog.com> >>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com> >>> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com> >>> --- >>> .../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@vger.kernel.org >>> +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@vger.kernel.org >>> +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@vger.kernel.org >>> +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@analog.com> >>> L: linux-iio@vger.kernel.org >>> 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.
On Sat, 2024-04-13 at 16:25 +0100, Jonathan Cameron wrote: > On Fri, 12 Apr 2024 16:26:17 -0500 > David Lechner <dlechner@baylibre.com> wrote: > > > On Thu, Apr 11, 2024 at 10:21 PM Kim Seer Paller > > <kimseer.paller@analog.com> 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. Things may be slightly different in these devices. But for ltc2688 and AFAIR, the symbol attribute is about toggling between A and B through SW (not really enabling the mode). That interface will only pop up if there's no HW (PWM for example) toggle present. > > > > > 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. > Yeah and for the ltc2688 we also had the dither mode that has it's own unique set of interfaces. Trying to deal with it all at runtime could be more confusing than beneficial I guess. - Nuno Sá >
> -----Original Message----- > From: Jonathan Cameron <jic23@kernel.org> > Sent: Saturday, April 13, 2024 11:27 PM > To: Paller, Kim Seer <KimSeer.Paller@analog.com> > Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux- > kernel@vger.kernel.org; Lars-Peter Clausen <lars@metafoo.de>; Rob Herring > <robh@kernel.org>; Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; > Conor Dooley <conor+dt@kernel.org>; Liam Girdwood > <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; David Lechner > <dlechner@baylibre.com>; Hennerich, Michael > <Michael.Hennerich@analog.com> > Subject: Re: [PATCH 3/4] iio: ABI: add ABI file for the LTC2672 DAC > > [External] > > On Fri, 12 Apr 2024 11:21:01 +0800 > Kim Seer Paller <kimseer.paller@analog.com> wrote: > > > Define the sysfs interface for toggle capable channels. > > > > Toggle enabled channels will have: > > > > * out_currentY_toggle_en > > * out_currentY_raw0 > > * out_currentY_raw1 > > * out_currentY_symbol > > > > The common interface present in all channels is: > > > > * out_currentY_raw (not present in toggle enabled channels) > > * out_currentY_raw_available > > * out_currentY_powerdown > > * out_currentY_scale > > * out_currentY_offset > > > > Co-developed-by: Michael Hennerich <michael.hennerich@analog.com> > > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com> > > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com> > > --- > > .../ABI/testing/sysfs-bus-iio-dac-ltc2672 | 30 +++++++++++++++++++ > > You can only have per device ABI defined if that is the only user > of the ABI. That may actually be true here but given I've asked you to > generalize > the voltage equivalent, I think we've shown this is general enough that the > current > version should also be raised to sysfs-bus-iio-dac I'm still getting familiar with ABI documentation. If I understand correctly, generalizing the documentation to cover both parts would also mean we remove the overlapping sections from the ltc2688 ABI. Is that the correct approach?
On Mon, 15 Apr 2024 14:45:52 +0200 Nuno Sá <noname.nuno@gmail.com> wrote: > On Sat, 2024-04-13 at 16:25 +0100, Jonathan Cameron wrote: > > On Fri, 12 Apr 2024 16:26:17 -0500 > > David Lechner <dlechner@baylibre.com> wrote: > > > > > On Thu, Apr 11, 2024 at 10:21 PM Kim Seer Paller > > > <kimseer.paller@analog.com> 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. > > Things may be slightly different in these devices. But for ltc2688 and AFAIR, > the symbol attribute is about toggling between A and B through SW (not really > enabling the mode). That interface will only pop up if there's no HW (PWM for > example) toggle present. I can't remember if we discussed it at the time of that driver, but from a userspace interface point of view, for a single channel there would be little point in this. I guess the key is it simultaneously switches a bunch of channels. Perhaps we can make that clearer in the ABI docs (if it isn't already clear enough!) So a software interface does seem appropriate. There is a fun question of whether the toggle select is useful to software. That is picking which of A or B each output uses for next toggle. At first glance I don't think so, but I'm open to people suggesting why that might need a userspace interface. Superficially feels like anything that can be done with that interface can also be done keeping all channels toggling to A or all to B at one time and potentially a few more register writes. Jonathan
On Tue, 16 Apr 2024 14:18:23 +0000 "Paller, Kim Seer" <KimSeer.Paller@analog.com> wrote: > > -----Original Message----- > > From: Jonathan Cameron <jic23@kernel.org> > > Sent: Saturday, April 13, 2024 11:27 PM > > To: Paller, Kim Seer <KimSeer.Paller@analog.com> > > Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux- > > kernel@vger.kernel.org; Lars-Peter Clausen <lars@metafoo.de>; Rob Herring > > <robh@kernel.org>; Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; > > Conor Dooley <conor+dt@kernel.org>; Liam Girdwood > > <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; David Lechner > > <dlechner@baylibre.com>; Hennerich, Michael > > <Michael.Hennerich@analog.com> > > Subject: Re: [PATCH 3/4] iio: ABI: add ABI file for the LTC2672 DAC > > > > [External] > > > > On Fri, 12 Apr 2024 11:21:01 +0800 > > Kim Seer Paller <kimseer.paller@analog.com> wrote: > > > > > Define the sysfs interface for toggle capable channels. > > > > > > Toggle enabled channels will have: > > > > > > * out_currentY_toggle_en > > > * out_currentY_raw0 > > > * out_currentY_raw1 > > > * out_currentY_symbol > > > > > > The common interface present in all channels is: > > > > > > * out_currentY_raw (not present in toggle enabled channels) > > > * out_currentY_raw_available > > > * out_currentY_powerdown > > > * out_currentY_scale > > > * out_currentY_offset > > > > > > Co-developed-by: Michael Hennerich <michael.hennerich@analog.com> > > > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com> > > > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com> > > > --- > > > .../ABI/testing/sysfs-bus-iio-dac-ltc2672 | 30 +++++++++++++++++++ > > > > You can only have per device ABI defined if that is the only user > > of the ABI. That may actually be true here but given I've asked you to > > generalize > > the voltage equivalent, I think we've shown this is general enough that the > > current > > version should also be raised to sysfs-bus-iio-dac > > I'm still getting familiar with ABI documentation. If I understand correctly, > generalizing the documentation to cover both parts would also mean we remove > the overlapping sections from the ltc2688 ABI. Is that the correct approach? > > Yes. To test this build the html docs. IIRC that will complain about duplicate ABI definitions. I'm sure there is a way to test just ABI docs build but I've never really looked into it. Jonathan
The LTC2664 is a 4 Channel, Voltage Output SoftSpan DAC while LTC2672 is a 5 channel, Current Output Softspan DAC. The ABI defined for the driver: LTC2664 - toggle mode channels: * out_voltageY_toggle_en * out_voltageY_raw0 * out_voltageY_raw1 * out_voltageY_symbol LTC2672 - toggle mode channels: * out_currentY_toggle_en * out_currentY_raw0 * out_currentY_raw1 * out_currentY_symbol Default channels won't have any of the above ABIs. Kim Seer Paller (4): dt-bindings: iio: dac: Add adi,ltc2664.yaml iio: ABI: add ABI file for the LTC2664 DAC iio: ABI: add ABI file for the LTC2672 DAC iio: dac: ltc2664: Add driver for LTC2664 and LTC2672 .../ABI/testing/sysfs-bus-iio-dac-ltc2664 | 30 + .../ABI/testing/sysfs-bus-iio-dac-ltc2672 | 30 + .../bindings/iio/dac/adi,ltc2664.yaml | 230 +++++ MAINTAINERS | 11 + drivers/iio/dac/Kconfig | 11 + drivers/iio/dac/Makefile | 1 + drivers/iio/dac/ltc2664.c | 785 ++++++++++++++++++ 7 files changed, 1098 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2664 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2672 create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml create mode 100644 drivers/iio/dac/ltc2664.c base-commit: 27eea4778db8268cd6dc80a5b853c599bd3099f1