| Message ID | 20240510064053.278257-1-Mariel.Tinaco@analog.com |
|---|---|
| Headers | show |
| Series | add AD8460 DAC driver | expand |
On Fri, May 10, 2024 at 1:42 AM Mariel Tinaco <Mariel.Tinaco@analog.com> wrote: > > The AD8460 is a “bits in, power out” high voltage, high-power, > highspeed driver optimized for large output current (up to ±1 A) high-speed > and high slew rate (up to ±1800 V/μs) at high voltage (up to ±40 V) > into capacitive loads. > > A digital engine implements user-configurable features: modes for > digital input, programmable supply current, and fault monitoring > and programmable protection settings for output current, > output voltage, and junction temperature. The AD8460 operates on > high voltage dual supplies up to ±55 V and a single low voltage > supply of 5 V. > > Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com> > --- ... > diff --git a/drivers/iio/dac/ad8460.c b/drivers/iio/dac/ad8460.c > new file mode 100644 > index 000000000..4049be0f5 > --- /dev/null > +++ b/drivers/iio/dac/ad8460.c ... > +static int ad8460_probe(struct spi_device *spi) > +{ > + struct ad8460_state *state; > + struct iio_dev *indio_dev; > + struct regulator *vrefio; > + int ret; > + > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state)); > + if (!indio_dev) > + return -ENOMEM; > + > + state = iio_priv(indio_dev); > + mutex_init(&state->lock); > + > + state->spi = spi; > + > + state->regmap = devm_regmap_init_spi(spi, &ad8460_regmap_config); > + if (IS_ERR(state->regmap)) > + return dev_err_probe(&spi->dev, PTR_ERR(state->regmap), > + "Failed to initialize regmap"); > + > + ret = devm_iio_dmaengine_buffer_setup_ext(&spi->dev, indio_dev, "tx", > + IIO_BUFFER_DIRECTION_OUT); > + if (ret) > + return dev_err_probe(&spi->dev, ret, > + "Failed to get DMA buffer\n"); It looks like the dmas property is missing in the DT bindings. However, as I suggested in my comments on that patch, it might make more sense to implement the parallel data part as an IIO backend. I assume this is using one of ADI's FPGA IP blocks to get the data? > + > + state->sync_clk = devm_clk_get_enabled(&spi->dev, "sync_clk"); The DT bindings don't specify clock-names, which is perfectly fine and perhaps even preferred since there is only one clock. But that means the ID here should be NULL instead of "sync_clk". > + if (IS_ERR(state->sync_clk)) > + return dev_err_probe(&spi->dev, PTR_ERR(state->sync_clk), > + "Failed to get sync clk\n"); > + > + vrefio = devm_regulator_get_optional(&spi->dev, "vrefio"); > + if (IS_ERR(vrefio)) { > + if (PTR_ERR(vrefio) != -ENODEV) > + return dev_err_probe(&spi->dev, PTR_ERR(vrefio), > + "Failed to get vref regulator\n"); > + > + state->vref_mv = 1200; > + > + } else { > + ret = regulator_enable(vrefio); > + if (ret) > + return dev_err_probe(&spi->dev, ret, > + "Failed to enable vrefio regulator\n"); > + > + ret = devm_add_action_or_reset(&spi->dev, > + ad8460_regulator_disable, > + vrefio); > + if (ret) > + return ret; > + > + ret = regulator_get_voltage(vrefio); > + if (ret < 0) > + return dev_err_probe(&spi->dev, ret, > + "Failed to get ref voltage\n"); > + > + if (!in_range(ret, AD8460_MIN_VREFIO_UV, AD8460_MAX_VREFIO_UV)) > + return dev_err_probe(&spi->dev, -EINVAL, > + "Invalid ref voltage range(%u) [%u, %u]\n", > + ret, AD8460_MIN_VREFIO_UV, > + AD8460_MAX_VREFIO_UV); > + > + state->vref_mv = ret / 1000; > + } FYI, if all goes well, starting with kernel 6.10-rc1 (we'll have to wait a few weeks for this), this regulator section can be simplified to: ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vrefio"); if (ret == -ENODEV) { /* no supply, using internal 1.2V reference */ state->vref_mv = 1200; } else if (ret < 0) { return dev_err_probe(&spi->dev, ret, "Failed to get reference voltage\n"); } else { /* external reference */ state->vref_mv = ret / 1000; } if (!in_range(... We can always fix it up later though if you don't want to wait. :-) > + > + ret = device_property_read_u32(&spi->dev, "adi,rset-ohms", > + &state->rset_ohms); Since 0 isn't an allowable value for R_SET, it seems like we need to return on error here or assign a default value if the property is missing. If we do a default value, the DT bindings need to be updated to reflect that as well. > + if (!ret) { > + if (!in_range(state->rset_ohms, AD8460_MIN_RSET_OHMS, > + AD8460_MAX_RSET_OHMS)) > + return dev_err_probe(&spi->dev, -EINVAL, > + "Invalid resistor set range(%u) [%u, %u]\n", > + state->rset_ohms, > + AD8460_MIN_RSET_OHMS, > + AD8460_MAX_RSET_OHMS); > + } > + > + ret = ad8460_reset(state); > + if (ret) > + return ret; > + > + /* Enables DAC by default */ > + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01), I was going to suggest giving names to the registers instead of using the number, but the datasheet doesn't do that! :-( Oh well, I guess this is the best we can do. > + AD8460_HVDAC_SLEEP_MSK, > + FIELD_PREP(AD8460_HVDAC_SLEEP_MSK, 0)); > + if (ret) > + return ret; > + > + indio_dev->name = "ad8460"; > + indio_dev->info = &ad8460_info; > + indio_dev->channels = ad8460_channels; > + indio_dev->num_channels = ARRAY_SIZE(ad8460_channels); > + indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_HARDWARE; devm_iio_dmaengine_buffer_setup_ext() already sets the INDIO_BUFFER_HARDWARE flag so this is redundant. Also, devm_iio_dmaengine_buffer_setup_ext() is usually called here rather than early in probe because it needs a few things in indio_dev populated, IIRC. > + indio_dev->setup_ops = &ad8460_buffer_setup_ops; > + > + ret = devm_iio_device_register(&spi->dev, indio_dev); > + if (ret) > + return ret; > + > + ad8460_debugfs_init(indio_dev); > + > + return 0; > +} > + ...
On Fri, May 10, 2024 at 1:42 AM Mariel Tinaco <Mariel.Tinaco@analog.com> wrote: > > The AD8460 is a 14-bit, high power +-40V 1A, high-speed DAC, > with dual digital input modes, programmable supply current and > fault monitoring and protection settings for output current, > output voltage and junction temperature. > > The fault monitoring and shutdown protection features were > supported in the earlier versions of the IIO driver but was > scrapped due to uncertainties if the functionalities belong to > the IIO driver. However, it would be best to implement it for > the device's quality of life. I'd like to know if it's better > suited as a stand-alone HWMON driver. > > The following are the configurable and readable parameters > through SPI that could be implemented on the HWMON driver: > * An enable bit to arm/protect the device on overcurrent, > overvoltage or overtemperature events. The device is shut down > upon detection. > * A configurable range/threshold for voltage, current and > temperature that raises alarm when exceeded while the device is > armed. > * Flags that can be polled to raise alarm upon detection of > overcurrent, overvoltage or overtemperature events, and apply > additional protective measures. > * Programmable quiescent current (optional) > * Thermal monitoring is done by measuring voltage on TMP pin > (unlikely to be included) > Adding myself to the cc: here since I'm interested to see what Jonathan (or anyone else) has to say about the fault monitoring.
On Fri, 10 May 2024 12:30:46 -0500 David Lechner <dlechner@baylibre.com> wrote: > On Fri, May 10, 2024 at 1:42 AM Mariel Tinaco <Mariel.Tinaco@analog.com> wrote: > > > > The AD8460 is a 14-bit, high power +-40V 1A, high-speed DAC, > > with dual digital input modes, programmable supply current and > > fault monitoring and protection settings for output current, > > output voltage and junction temperature. > > > > The fault monitoring and shutdown protection features were > > supported in the earlier versions of the IIO driver but was > > scrapped due to uncertainties if the functionalities belong to > > the IIO driver. However, it would be best to implement it for > > the device's quality of life. I'd like to know if it's better > > suited as a stand-alone HWMON driver. That probably doesn't make sense. This reply btw is based on the text here. I haven't yet looked in detail at the datasheet. Some fault conditions are relatively easy to map to threshold events on an input channel. The ones that are harder are things like short circuit detectors where it's hard to know what the actual threshold is. The other place we are limited is in multiple levels for the same thing. So warning and fault levels. That stuff is handled much better by hwmon where these things are more common. The issue we have is that the event encoding is a bit tight but we could see if there is a way to add these. > > > > The following are the configurable and readable parameters > > through SPI that could be implemented on the HWMON driver: > > * An enable bit to arm/protect the device on overcurrent, > > overvoltage or overtemperature events. The device is shut down > > upon detection. We don't have a good way to handle restarting from shutdown, but perhaps we could use another action to signal that - maybe even going as far as saying that the driver should be reloaded. As for the thresholds, they sound like the map to IIO events reasonably well. > > * A configurable range/threshold for voltage, current and > > temperature that raises alarm when exceeded while the device is > > armed. That maps fine to the IIO event threshold controls. > > * Flags that can be polled to raise alarm upon detection of > > overcurrent, overvoltage or overtemperature events, and apply > > additional protective measures. The specific need to poll is awkward but you could do it easily enough in driver and use the IIO event stuff to signal any events detected. > > * Programmable quiescent current (optional) Could probably figure out a suitable control for this, but I'm not entirely sure what it is :) > > * Thermal monitoring is done by measuring voltage on TMP pin > > (unlikely to be included) If you did want to, the usual trick for these is to include an optional use as a consumer of an IIO provider which would be a separate ADC. > > > > Adding myself to the cc: here since I'm interested to see what > Jonathan (or anyone else) has to say about the fault monitoring. Jonathan
On Fri, 10 May 2024 14:40:53 +0800 Mariel Tinaco <Mariel.Tinaco@analog.com> wrote: > The AD8460 is a “bits in, power out” high voltage, high-power, > highspeed driver optimized for large output current (up to ±1 A) > and high slew rate (up to ±1800 V/μs) at high voltage (up to ±40 V) > into capacitive loads. > > A digital engine implements user-configurable features: modes for > digital input, programmable supply current, and fault monitoring > and programmable protection settings for output current, > output voltage, and junction temperature. The AD8460 operates on > high voltage dual supplies up to ±55 V and a single low voltage > supply of 5 V. > > Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com> I'd like to see some ABI docs for the debugfs interface. The device is unusual enough that a general intro document or a lot more in the series cover letter would be useful. I'm not sure what the dmaengine usage in here is doing for example? Driving the parallel bus perhaps? David was correct that the binding should reflect that part as well. I was assuming you'd only implemented the spi part. How to handle the pattern generator is also an interesting question. That probably wants a version of the symbol interfaces we use for PSK and similar. We did have some DDS drivers a long time back in staging but they only did a few fixed waveforms so this is breaking new ground. Having looked a little at the datasheet, we may want to hard code some limits - or get them from DT. Probably a bit too easy to set things on fire otherwise. Also, seems that this is only partly implemented. Please make that clear in the patch description. Perhaps this should have been an RFC with a list of questions? Jonathan > diff --git a/drivers/iio/dac/ad8460.c b/drivers/iio/dac/ad8460.c > new file mode 100644 > index 000000000..4049be0f5 > --- /dev/null > +++ b/drivers/iio/dac/ad8460.c > @@ -0,0 +1,652 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * AD8460 Waveform generator DAC Driver > + * > + * Copyright (C) 2024 Analog Devices, Inc. > + */ > + > +#include <linux/bitfield.h> > +#include <linux/cleanup.h> > +#include <linux/clk.h> > +#include <linux/debugfs.h> > +#include <linux/delay.h> > +#include <linux/dmaengine.h> > +#include <linux/gpio/consumer.h> > +#include <linux/iio/buffer.h> > +#include <linux/iio/buffer-dma.h> > +#include <linux/iio/buffer-dmaengine.h> > +#include <linux/iio/iio.h> > +#include <linux/module.h> > +#include <linux/mod_devicetable.h> > +#include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > +#include <linux/spi/spi.h> > + > +#define AD8460_CTRL_REG(x) (x) > +#define AD8460_HVDAC_DATA_WORD_LOW(x) (0x60 + (2 * (x))) > +#define AD8460_HVDAC_DATA_WORD_HIGH(x) (0x61 + (2 * (x))) > + > +#define AD8460_HV_RESET_MSK BIT(7) > +#define AD8460_HV_SLEEP_MSK BIT(4) > +#define AD8460_WAVE_GEN_MODE_MSK BIT(0) > + > +#define AD8460_HVDAC_SLEEP_MSK BIT(3) > + > +#define AD8460_APG_MODE_ENABLE_MSK BIT(5) > +#define AD8460_PATTERN_DEPTH_MSK GENMASK(3, 0) > + > +#define AD8460_SHUTDOWN_FLAG_MSK BIT(7) > +#define AD8460_DATA_BYTE_LOW_MSK GENMASK(7, 0) > +#define AD8460_DATA_BYTE_HIGH_MSK GENMASK(5, 0) > + > +#define AD8460_NOMINAL_VOLTAGE_SPAN 80 > +#define AD8460_MIN_VREFIO_UV 120000 > +#define AD8460_MAX_VREFIO_UV 1200000 > +#define AD8460_MIN_RSET_OHMS 2000 > +#define AD8460_MAX_RSET_OHMS 20000 > + > +struct ad8460_state { > + struct spi_device *spi; > + struct regmap *regmap; > + struct clk *sync_clk; > + /* lock to protect against multiple access to the device and shared data */ > + struct mutex lock; > + u32 cache_apg_idx; > + u32 rset_ohms; > + int vref_mv; > +}; > + > + > +static const char * const ad8460_powerdown_modes[] = { > + "three_state", > +}; > + > +static int ad8460_get_powerdown_mode(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan) > +{ > + return 0; Why have the stubs in here? > +} > + > +static int ad8460_set_powerdown_mode(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + unsigned int type) > +{ > + return 0; > +} > + > +static int ad8460_get_hvdac_byte(struct ad8460_state *state, byte? Seems to be getting a 14 bit value. > + int index, > + int *val) > +{ > + unsigned int high, low; > + int ret; > + > + ret = regmap_read(state->regmap, AD8460_HVDAC_DATA_WORD_HIGH(index), > + &high); Use a bulk read? Then byteswap if necessary and mask the result. > + if (ret) > + return ret; > + > + ret = regmap_read(state->regmap, AD8460_HVDAC_DATA_WORD_LOW(index), > + &low); > + if (ret) > + return ret; > + > + *val = FIELD_GET(AD8460_DATA_BYTE_HIGH_MSK, high) << 8 | low; > + > + return ret; > +} > + > +static int ad8460_set_hvdac_byte(struct ad8460_state *state, > + int index, > + int val) > +{ > + int ret; > + > + ret = regmap_write(state->regmap, AD8460_HVDAC_DATA_WORD_LOW(index), > + (val & 0xFF)); > + if (ret) > + return ret; > + > + return regmap_write(state->regmap, AD8460_HVDAC_DATA_WORD_HIGH(index), > + ((val >> 8) & 0xFF)); bulk write? or do these need to be ordered? > +} > + > +static int ad8460_set_sample(struct ad8460_state *state, int val) > +{ > + int ret; > + > + ret = ad8460_enable_apg_mode(state, 1); > + if (ret) > + return ret; > + > + guard(mutex)(&state->lock); > + ret = ad8460_set_hvdac_byte(state, 0, val); > + if (ret) > + return ret; > + > + return regmap_update_bits(state->regmap, > + AD8460_CTRL_REG(0x02), > + AD8460_PATTERN_DEPTH_MSK, > + FIELD_PREP(AD8460_PATTERN_DEPTH_MSK, 0)); > +} > + > +static int ad8460_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int val, > + int val2, > + long mask) > +{ > + struct ad8460_state *state = iio_priv(indio_dev); > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) > + return ad8460_set_sample(state, val); > + unreachable(); > + default: > + return -EINVAL; > + } > +} > +static int ad8460_probe(struct spi_device *spi) > +{ > + struct ad8460_state *state; > + struct iio_dev *indio_dev; > + struct regulator *vrefio; > + int ret; > + > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state)); > + if (!indio_dev) > + return -ENOMEM; > + > + state = iio_priv(indio_dev); > + mutex_init(&state->lock); > + > + state->spi = spi; > + > + state->regmap = devm_regmap_init_spi(spi, &ad8460_regmap_config); > + if (IS_ERR(state->regmap)) > + return dev_err_probe(&spi->dev, PTR_ERR(state->regmap), > + "Failed to initialize regmap"); > + > + ret = devm_iio_dmaengine_buffer_setup_ext(&spi->dev, indio_dev, "tx", > + IIO_BUFFER_DIRECTION_OUT); Ah. I take back my binding comment. I assume this is mapping some non standard interface for the parallel data flow? > + if (ret) > + return dev_err_probe(&spi->dev, ret, > + "Failed to get DMA buffer\n"); > + > + state->sync_clk = devm_clk_get_enabled(&spi->dev, "sync_clk"); > + if (IS_ERR(state->sync_clk)) > + return dev_err_probe(&spi->dev, PTR_ERR(state->sync_clk), > + "Failed to get sync clk\n"); > + > + vrefio = devm_regulator_get_optional(&spi->dev, "vrefio"); > + if (IS_ERR(vrefio)) { > + if (PTR_ERR(vrefio) != -ENODEV) > + return dev_err_probe(&spi->dev, PTR_ERR(vrefio), > + "Failed to get vref regulator\n"); > + > + state->vref_mv = 1200; > + > + } else { > + ret = regulator_enable(vrefio); > + if (ret) > + return dev_err_probe(&spi->dev, ret, > + "Failed to enable vrefio regulator\n"); > + > + ret = devm_add_action_or_reset(&spi->dev, > + ad8460_regulator_disable, > + vrefio); > + if (ret) > + return ret; > + > + ret = regulator_get_voltage(vrefio); > + if (ret < 0) > + return dev_err_probe(&spi->dev, ret, > + "Failed to get ref voltage\n"); > + > + if (!in_range(ret, AD8460_MIN_VREFIO_UV, AD8460_MAX_VREFIO_UV)) > + return dev_err_probe(&spi->dev, -EINVAL, > + "Invalid ref voltage range(%u) [%u, %u]\n", > + ret, AD8460_MIN_VREFIO_UV, > + AD8460_MAX_VREFIO_UV); > + > + state->vref_mv = ret / 1000; > + } > + > + ret = device_property_read_u32(&spi->dev, "adi,rset-ohms", > + &state->rset_ohms); > + if (!ret) { > + if (!in_range(state->rset_ohms, AD8460_MIN_RSET_OHMS, > + AD8460_MAX_RSET_OHMS)) > + return dev_err_probe(&spi->dev, -EINVAL, > + "Invalid resistor set range(%u) [%u, %u]\n", > + state->rset_ohms, > + AD8460_MIN_RSET_OHMS, > + AD8460_MAX_RSET_OHMS); > + } > + > + ret = ad8460_reset(state); > + if (ret) > + return ret; > + > + /* Enables DAC by default */ > + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01), > + AD8460_HVDAC_SLEEP_MSK, > + FIELD_PREP(AD8460_HVDAC_SLEEP_MSK, 0)); > + if (ret) > + return ret; > + > + indio_dev->name = "ad8460"; > + indio_dev->info = &ad8460_info; > + indio_dev->channels = ad8460_channels; > + indio_dev->num_channels = ARRAY_SIZE(ad8460_channels); > + indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_HARDWARE; > + indio_dev->setup_ops = &ad8460_buffer_setup_ops; > + > + ret = devm_iio_device_register(&spi->dev, indio_dev); > + if (ret) > + return ret; > + > + ad8460_debugfs_init(indio_dev); > + > + return 0; > +} > + > +static const struct of_device_id ad8460_of_match[] = { > + { .compatible = "adi, ad8460" }, > + { }, No comma on 'null' terminators like this as we don't want to let people put anything after them. > +}; > +MODULE_DEVICE_TABLE(of, ad8460_of_match);
> -----Original Message----- > From: Jonathan Cameron <jic23@kernel.org> > Sent: Sunday, May 12, 2024 12:44 AM > To: Tinaco, Mariel <Mariel.Tinaco@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 <krzk+dt@kernel.org>; Conor Dooley > <conor+dt@kernel.org>; Liam Girdwood <lgirdwood@gmail.com>; Mark Brown > <broonie@kernel.org>; Hennerich, Michael <Michael.Hennerich@analog.com>; > Marcelo Schmitt <marcelo.schmitt1@gmail.com>; Dimitri Fedrau > <dima.fedrau@gmail.com>; Guenter Roeck <linux@roeck-us.net> > Subject: Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC > > [External] > > On Fri, 10 May 2024 14:40:53 +0800 > Mariel Tinaco <Mariel.Tinaco@analog.com> wrote: > > > The AD8460 is a “bits in, power out” high voltage, high-power, > > highspeed driver optimized for large output current (up to ±1 A) and > > high slew rate (up to ±1800 V/μs) at high voltage (up to ±40 V) into > > capacitive loads. > > > > A digital engine implements user-configurable features: modes for > > digital input, programmable supply current, and fault monitoring and > > programmable protection settings for output current, output voltage, > > and junction temperature. The AD8460 operates on high voltage dual > > supplies up to ±55 V and a single low voltage supply of 5 V. > > > > Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com> > > I'd like to see some ABI docs for the debugfs interface. > The device is unusual enough that a general intro document or a lot more in the > series cover letter would be useful. > > I'm not sure what the dmaengine usage in here is doing for example? > Driving the parallel bus perhaps? David was correct that the binding should > reflect that part as well. I was assuming you'd only implemented the spi part. > > How to handle the pattern generator is also an interesting question. > That probably wants a version of the symbol interfaces we use for PSK and > similar. We did have some DDS drivers a long time back in staging but they only > did a few fixed waveforms so this is breaking new ground. > > Having looked a little at the datasheet, we may want to hard code some limits - > or get them from DT. Probably a bit too easy to set things on fire otherwise. > > Also, seems that this is only partly implemented. Please make that clear in the > patch description. Perhaps this should have been an RFC with a list of > questions? > > Jonathan > > > diff --git a/drivers/iio/dac/ad8460.c b/drivers/iio/dac/ad8460.c new > > file mode 100644 index 000000000..4049be0f5 > > --- /dev/null > > +++ b/drivers/iio/dac/ad8460.c > > @@ -0,0 +1,652 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * AD8460 Waveform generator DAC Driver > > + * > > + * Copyright (C) 2024 Analog Devices, Inc. > > + */ > > + > > +#include <linux/bitfield.h> > > +#include <linux/cleanup.h> > > +#include <linux/clk.h> > > +#include <linux/debugfs.h> > > +#include <linux/delay.h> > > +#include <linux/dmaengine.h> > > +#include <linux/gpio/consumer.h> > > +#include <linux/iio/buffer.h> > > +#include <linux/iio/buffer-dma.h> > > +#include <linux/iio/buffer-dmaengine.h> #include <linux/iio/iio.h> > > +#include <linux/module.h> #include <linux/mod_devicetable.h> #include > > +<linux/regmap.h> #include <linux/regulator/consumer.h> #include > > +<linux/spi/spi.h> > > + > > +#define AD8460_CTRL_REG(x) (x) > > +#define AD8460_HVDAC_DATA_WORD_LOW(x) (0x60 + (2 * (x))) > > +#define AD8460_HVDAC_DATA_WORD_HIGH(x) (0x61 + (2 * (x))) > > + > > +#define AD8460_HV_RESET_MSK BIT(7) > > +#define AD8460_HV_SLEEP_MSK BIT(4) > > +#define AD8460_WAVE_GEN_MODE_MSK BIT(0) > > + > > +#define AD8460_HVDAC_SLEEP_MSK BIT(3) > > + > > +#define AD8460_APG_MODE_ENABLE_MSK BIT(5) > > +#define AD8460_PATTERN_DEPTH_MSK GENMASK(3, 0) > > + > > +#define AD8460_SHUTDOWN_FLAG_MSK BIT(7) > > +#define AD8460_DATA_BYTE_LOW_MSK GENMASK(7, 0) > > +#define AD8460_DATA_BYTE_HIGH_MSK GENMASK(5, 0) > > + > > +#define AD8460_NOMINAL_VOLTAGE_SPAN 80 > > +#define AD8460_MIN_VREFIO_UV 120000 > > +#define AD8460_MAX_VREFIO_UV 1200000 > > +#define AD8460_MIN_RSET_OHMS 2000 > > +#define AD8460_MAX_RSET_OHMS 20000 > > + > > +struct ad8460_state { > > + struct spi_device *spi; > > + struct regmap *regmap; > > + struct clk *sync_clk; > > + /* lock to protect against multiple access to the device and shared data > */ > > + struct mutex lock; > > + u32 cache_apg_idx; > > + u32 rset_ohms; > > + int vref_mv; > > +}; > > + > > > + > > +static const char * const ad8460_powerdown_modes[] = { > > + "three_state", > > +}; > > + > > +static int ad8460_get_powerdown_mode(struct iio_dev *indio_dev, > > + const struct iio_chan_spec *chan) { > > + return 0; > > Why have the stubs in here? Should I move the stubs to a different place in the code or remove them altogether since there is only a single powerdown mode available > > +} > > + > > +static int ad8460_set_powerdown_mode(struct iio_dev *indio_dev, > > + const struct iio_chan_spec *chan, > > + unsigned int type) > > +{ > > + return 0; > > +} > > + > > +static int ad8460_get_hvdac_byte(struct ad8460_state *state, > > byte? Seems to be getting a 14 bit value. > > > + int index, > > + int *val) > > +{ > > + unsigned int high, low; > > + int ret; > > + > > + ret = regmap_read(state->regmap, > AD8460_HVDAC_DATA_WORD_HIGH(index), > > + &high); I replaced the name with hvdac data word instead like in the datasheet > Use a bulk read? Then byteswap if necessary and mask the result. > > > + if (ret) > > + return ret; > > + > > + ret = regmap_read(state->regmap, > AD8460_HVDAC_DATA_WORD_LOW(index), > > + &low); > > + if (ret) > > + return ret; > > + > > + *val = FIELD_GET(AD8460_DATA_BYTE_HIGH_MSK, high) << 8 | low; > > + > > + return ret; > > +} > > + > > +static int ad8460_set_hvdac_byte(struct ad8460_state *state, > > + int index, > > + int val) > > +{ > > + int ret; > > + > > + ret = regmap_write(state->regmap, > AD8460_HVDAC_DATA_WORD_LOW(index), > > + (val & 0xFF)); > > + if (ret) > > + return ret; > > + > > + return regmap_write(state->regmap, > AD8460_HVDAC_DATA_WORD_HIGH(index), > > + ((val >> 8) & 0xFF)); > > bulk write? or do these need to be ordered? For this I used bulk read/write this way. static int ad8460_set_hvdac_word(struct ad8460_state *state, int index, int val) { u8 regvals[AD8460_DATA_BYTE_WORD_LENGTH]; regvals[0] = val & 0xFF; regvals[1] = (val >> 8) & 0xFF; return regmap_bulk_write(state->regmap, AD8460_HVDAC_DATA_WORD_LOW(index), regvals, AD8460_DATA_BYTE_WORD_LENGTH); } > > +} > > + > > +static int ad8460_set_sample(struct ad8460_state *state, int val) { > > + int ret; > > + > > + ret = ad8460_enable_apg_mode(state, 1); > > + if (ret) > > + return ret; > > + > > + guard(mutex)(&state->lock); > > + ret = ad8460_set_hvdac_byte(state, 0, val); > > + if (ret) > > + return ret; > > + > > + return regmap_update_bits(state->regmap, > > + AD8460_CTRL_REG(0x02), > > + AD8460_PATTERN_DEPTH_MSK, > > + FIELD_PREP(AD8460_PATTERN_DEPTH_MSK, > 0)); } > > + > > +static int ad8460_write_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, > > + int val, > > + int val2, > > + long mask) > > +{ > > + struct ad8460_state *state = iio_priv(indio_dev); > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) > > + return ad8460_set_sample(state, val); > > + unreachable(); > > + default: > > + return -EINVAL; > > + } > > +} > > > > > +static int ad8460_probe(struct spi_device *spi) { > > + struct ad8460_state *state; > > + struct iio_dev *indio_dev; > > + struct regulator *vrefio; > > + int ret; > > + > > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state)); > > + if (!indio_dev) > > + return -ENOMEM; > > + > > + state = iio_priv(indio_dev); > > + mutex_init(&state->lock); > > + > > + state->spi = spi; > > + > > + state->regmap = devm_regmap_init_spi(spi, &ad8460_regmap_config); > > + if (IS_ERR(state->regmap)) > > + return dev_err_probe(&spi->dev, PTR_ERR(state->regmap), > > + "Failed to initialize regmap"); > > + > > + ret = devm_iio_dmaengine_buffer_setup_ext(&spi->dev, indio_dev, "tx", > > + > IIO_BUFFER_DIRECTION_OUT); > > Ah. I take back my binding comment. I assume this is mapping some non > standard interface for the parallel data flow? Yes, the HDL side doesn't follow yet the standard IIO backend from which this driver was tested > > + if (ret) > > + return dev_err_probe(&spi->dev, ret, > > + "Failed to get DMA buffer\n"); > > + > > + state->sync_clk = devm_clk_get_enabled(&spi->dev, "sync_clk"); > > + if (IS_ERR(state->sync_clk)) > > + return dev_err_probe(&spi->dev, PTR_ERR(state->sync_clk), > > + "Failed to get sync clk\n"); > > + > > + vrefio = devm_regulator_get_optional(&spi->dev, "vrefio"); > > + if (IS_ERR(vrefio)) { > > + if (PTR_ERR(vrefio) != -ENODEV) > > + return dev_err_probe(&spi->dev, PTR_ERR(vrefio), > > + "Failed to get vref regulator\n"); > > + > > + state->vref_mv = 1200; > > + > > + } else { > > + ret = regulator_enable(vrefio); > > + if (ret) > > + return dev_err_probe(&spi->dev, ret, > > + "Failed to enable vrefio > regulator\n"); > > + > > + ret = devm_add_action_or_reset(&spi->dev, > > + ad8460_regulator_disable, > > + vrefio); > > + if (ret) > > + return ret; > > + > > + ret = regulator_get_voltage(vrefio); > > + if (ret < 0) > > + return dev_err_probe(&spi->dev, ret, > > + "Failed to get ref voltage\n"); > > + > > + if (!in_range(ret, AD8460_MIN_VREFIO_UV, > AD8460_MAX_VREFIO_UV)) > > + return dev_err_probe(&spi->dev, -EINVAL, > > + "Invalid ref voltage range(%u) [%u, > %u]\n", > > + ret, AD8460_MIN_VREFIO_UV, > > + AD8460_MAX_VREFIO_UV); > > + > > + state->vref_mv = ret / 1000; > > + } > > + > > + ret = device_property_read_u32(&spi->dev, "adi,rset-ohms", > > + &state->rset_ohms); > > + if (!ret) { > > + if (!in_range(state->rset_ohms, AD8460_MIN_RSET_OHMS, > > + AD8460_MAX_RSET_OHMS)) > > + return dev_err_probe(&spi->dev, -EINVAL, > > + "Invalid resistor set range(%u) [%u, > %u]\n", > > + state->rset_ohms, > > + AD8460_MIN_RSET_OHMS, > > + AD8460_MAX_RSET_OHMS); > > + } > > + > > + ret = ad8460_reset(state); > > + if (ret) > > + return ret; > > + > > + /* Enables DAC by default */ > > + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01), > > + AD8460_HVDAC_SLEEP_MSK, > > + FIELD_PREP(AD8460_HVDAC_SLEEP_MSK, > 0)); > > + if (ret) > > + return ret; > > + > > + indio_dev->name = "ad8460"; > > + indio_dev->info = &ad8460_info; > > + indio_dev->channels = ad8460_channels; > > + indio_dev->num_channels = ARRAY_SIZE(ad8460_channels); > > + indio_dev->modes = INDIO_DIRECT_MODE | > INDIO_BUFFER_HARDWARE; > > + indio_dev->setup_ops = &ad8460_buffer_setup_ops; > > + > > + ret = devm_iio_device_register(&spi->dev, indio_dev); > > + if (ret) > > + return ret; > > + > > + ad8460_debugfs_init(indio_dev); > > + > > + return 0; > > +} > > + > > +static const struct of_device_id ad8460_of_match[] = { > > + { .compatible = "adi, ad8460" }, > > + { }, > No comma on 'null' terminators like this as we don't want to let people put > anything after them. > > > +}; > > +MODULE_DEVICE_TABLE(of, ad8460_of_match);
> -----Original Message----- > From: Jonathan Cameron <jic23@kernel.org> > Sent: Sunday, May 12, 2024 12:22 AM > To: David Lechner <dlechner@baylibre.com> > Cc: Tinaco, Mariel <Mariel.Tinaco@analog.com>; 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 > <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Liam Girdwood > <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; Hennerich, > Michael <Michael.Hennerich@analog.com>; Marcelo Schmitt > <marcelo.schmitt1@gmail.com>; Dimitri Fedrau <dima.fedrau@gmail.com>; > Guenter Roeck <linux@roeck-us.net> > Subject: Re: [PATCH 0/2] add AD8460 DAC driver > > [External] > > On Fri, 10 May 2024 12:30:46 -0500 > David Lechner <dlechner@baylibre.com> wrote: > > > On Fri, May 10, 2024 at 1:42 AM Mariel Tinaco <Mariel.Tinaco@analog.com> > wrote: > > > > > > The AD8460 is a 14-bit, high power +-40V 1A, high-speed DAC, with > > > dual digital input modes, programmable supply current and fault > > > monitoring and protection settings for output current, output > > > voltage and junction temperature. > > > > > > The fault monitoring and shutdown protection features were supported > > > in the earlier versions of the IIO driver but was scrapped due to > > > uncertainties if the functionalities belong to the IIO driver. > > > However, it would be best to implement it for the device's quality > > > of life. I'd like to know if it's better suited as a stand-alone > > > HWMON driver. > > That probably doesn't make sense. This reply btw is based on the text here. I > haven't yet looked in detail at the datasheet. > > Some fault conditions are relatively easy to map to threshold events on an input > channel. The ones that are harder are things like short circuit detectors where it's > hard to know what the actual threshold is. Upon checking the IIO Threshold events, the fault conditions can easily be mapped. But I had to add Current and Temperature channels > The other place we are limited is in multiple levels for the same thing. So > warning and fault levels. That stuff is handled much better by hwmon where > these things are more common. > The issue we have is that the event encoding is a bit tight but we could see if > there is a way to add these. Fortunately, there weren't multiple levels of fault monitoring as checked in the datasheet > > > > > > The following are the configurable and readable parameters through > > > SPI that could be implemented on the HWMON driver: > > > * An enable bit to arm/protect the device on overcurrent, > > > overvoltage or overtemperature events. The device is shut down upon > > > detection. > > We don't have a good way to handle restarting from shutdown, but perhaps we > could use another action to signal that - maybe even going as far as saying that > the driver should be reloaded. > > As for the thresholds, they sound like the map to IIO events reasonably well. > > > > * A configurable range/threshold for voltage, current and > > > temperature that raises alarm when exceeded while the device is > > > armed. > > That maps fine to the IIO event threshold controls. > > > > * Flags that can be polled to raise alarm upon detection of > > > overcurrent, overvoltage or overtemperature events, and apply > > > additional protective measures. > The specific need to poll is awkward but you could do it easily enough in driver > and use the IIO event stuff to signal any events detected. > > > > > * Programmable quiescent current (optional) > Could probably figure out a suitable control for this, but I'm not entirely sure > what it is :) Thinking about it, wouldn't the raw attribute be a suitable control for this? This Value is relative to nominal supply current and acts as a "monotonic but nonlinear" multiplier. A register value maps to a current level from 0 to 2 times the nominal current supplied. I also thought that it could be hardware gain but the gain computation wasn't explicitly indicated in the datasheet and there is not yet "current_hardwaregain" attribute available in the ABI. So I settled with raw. I Think there would only be an issue of we expose the "processed" attribute Because it has a particular computation. But I'm not planning to expose the Processed attribute > > > * Thermal monitoring is done by measuring voltage on TMP pin > > > (unlikely to be included) > > If you did want to, the usual trick for these is to include an optional use as a > consumer of an IIO provider which would be a separate ADC. I included this in my current revision, thanks for the idea. Although the optional use Isn’t yet available in the consumer API. My question is, in case the ADC channel to read The TMP pin is not available, should I still make the temp raw value available and Set to 0? Or should the temp raw attribute be unavailable or unlisted completely from IIO Info. > > > > > > > Adding myself to the cc: here since I'm interested to see what > > Jonathan (or anyone else) has to say about the fault monitoring. > > Jonathan
> -----Original Message----- > From: Jonathan Cameron <jic23@kernel.org> > Sent: Sunday, May 12, 2024 12:44 AM > To: Tinaco, Mariel <Mariel.Tinaco@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 <krzk+dt@kernel.org>; Conor Dooley > <conor+dt@kernel.org>; Liam Girdwood <lgirdwood@gmail.com>; Mark Brown > <broonie@kernel.org>; Hennerich, Michael <Michael.Hennerich@analog.com>; > Marcelo Schmitt <marcelo.schmitt1@gmail.com>; Dimitri Fedrau > <dima.fedrau@gmail.com>; Guenter Roeck <linux@roeck-us.net> > Subject: Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC > > [External] > > On Fri, 10 May 2024 14:40:53 +0800 > Mariel Tinaco <Mariel.Tinaco@analog.com> wrote: > > > The AD8460 is a “bits in, power out” high voltage, high-power, > > highspeed driver optimized for large output current (up to ±1 A) and > > high slew rate (up to ±1800 V/μs) at high voltage (up to ±40 V) into > > capacitive loads. > > > > A digital engine implements user-configurable features: modes for > > digital input, programmable supply current, and fault monitoring and > > programmable protection settings for output current, output voltage, > > and junction temperature. The AD8460 operates on high voltage dual > > supplies up to ±55 V and a single low voltage supply of 5 V. > > > > Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com> > > I'd like to see some ABI docs for the debugfs interface. > The device is unusual enough that a general intro document or a lot more in the > series cover letter would be useful. > > I'm not sure what the dmaengine usage in here is doing for example? > Driving the parallel bus perhaps? David was correct that the binding should > reflect that part as well. I was assuming you'd only implemented the spi part. > > How to handle the pattern generator is also an interesting question. > That probably wants a version of the symbol interfaces we use for PSK and > similar. We did have some DDS drivers a long time back in staging but they only > did a few fixed waveforms so this is breaking new ground. I also thought about how should the pattern generator be handled. IN the last revision, there were two debug attributes that make up this feature. Pattern depth and pattern memory. Ultimately I found a way to combine these two attributes into one called "test_pattern". The attribute is a string containing an array of values with a maximum of 16 data words, which the DAC will cycle through to generate a pattern. The number of values within the string can be anywhere between 1 to 16 and have a space in between. I also added a "test_pattern_enable" debug attribute. For the ABI file, should I create one alongside other "debugfs-*" files and just mention the name of the part? e.g. "debugfs-driver-ad8460" > Having looked a little at the datasheet, we may want to hard code some limits - > or get them from DT. Probably a bit too easy to set things on fire otherwise. Yes, with the inclusion of the threshold events, I was able to input some limits onto the device and enable protection upon probing. > Also, seems that this is only partly implemented. Please make that clear in the > patch description. Perhaps this should have been an RFC with a list of > questions? > > Jonathan > > > diff --git a/drivers/iio/dac/ad8460.c b/drivers/iio/dac/ad8460.c new > > file mode 100644 index 000000000..4049be0f5 > > --- /dev/null > > +++ b/drivers/iio/dac/ad8460.c > > @@ -0,0 +1,652 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * AD8460 Waveform generator DAC Driver > > + * > > + * Copyright (C) 2024 Analog Devices, Inc. > > + */ > > + > > +#include <linux/bitfield.h> > > +#include <linux/cleanup.h> > > +#include <linux/clk.h> > > +#include <linux/debugfs.h> > > +#include <linux/delay.h> > > +#include <linux/dmaengine.h> > > +#include <linux/gpio/consumer.h> > > +#include <linux/iio/buffer.h> > > +#include <linux/iio/buffer-dma.h> > > +#include <linux/iio/buffer-dmaengine.h> #include <linux/iio/iio.h> > > +#include <linux/module.h> #include <linux/mod_devicetable.h> #include > > +<linux/regmap.h> #include <linux/regulator/consumer.h> #include > > +<linux/spi/spi.h> > > + > > +#define AD8460_CTRL_REG(x) (x) > > +#define AD8460_HVDAC_DATA_WORD_LOW(x) (0x60 + (2 * (x))) > > +#define AD8460_HVDAC_DATA_WORD_HIGH(x) (0x61 + (2 * (x))) > > + > > +#define AD8460_HV_RESET_MSK BIT(7) > > +#define AD8460_HV_SLEEP_MSK BIT(4) > > +#define AD8460_WAVE_GEN_MODE_MSK BIT(0) > > + > > +#define AD8460_HVDAC_SLEEP_MSK BIT(3) > > + > > +#define AD8460_APG_MODE_ENABLE_MSK BIT(5) > > +#define AD8460_PATTERN_DEPTH_MSK GENMASK(3, 0) > > + > > +#define AD8460_SHUTDOWN_FLAG_MSK BIT(7) > > +#define AD8460_DATA_BYTE_LOW_MSK GENMASK(7, 0) > > +#define AD8460_DATA_BYTE_HIGH_MSK GENMASK(5, 0) > > + > > +#define AD8460_NOMINAL_VOLTAGE_SPAN 80 > > +#define AD8460_MIN_VREFIO_UV 120000 > > +#define AD8460_MAX_VREFIO_UV 1200000 > > +#define AD8460_MIN_RSET_OHMS 2000 > > +#define AD8460_MAX_RSET_OHMS 20000 > > + > > +struct ad8460_state { > > + struct spi_device *spi; > > + struct regmap *regmap; > > + struct clk *sync_clk; > > + /* lock to protect against multiple access to the device and shared data > */ > > + struct mutex lock; > > + u32 cache_apg_idx; > > + u32 rset_ohms; > > + int vref_mv; > > +}; > > + > > > + > > +static const char * const ad8460_powerdown_modes[] = { > > + "three_state", > > +}; > > + > > +static int ad8460_get_powerdown_mode(struct iio_dev *indio_dev, > > + const struct iio_chan_spec *chan) { > > + return 0; > > Why have the stubs in here? > > > +} > > + > > +static int ad8460_set_powerdown_mode(struct iio_dev *indio_dev, > > + const struct iio_chan_spec *chan, > > + unsigned int type) > > +{ > > + return 0; > > +} > > + > > +static int ad8460_get_hvdac_byte(struct ad8460_state *state, > > byte? Seems to be getting a 14 bit value. > > > + int index, > > + int *val) > > +{ > > + unsigned int high, low; > > + int ret; > > + > > + ret = regmap_read(state->regmap, > AD8460_HVDAC_DATA_WORD_HIGH(index), > > + &high); > > Use a bulk read? Then byteswap if necessary and mask the result. > > > + if (ret) > > + return ret; > > + > > + ret = regmap_read(state->regmap, > AD8460_HVDAC_DATA_WORD_LOW(index), > > + &low); > > + if (ret) > > + return ret; > > + > > + *val = FIELD_GET(AD8460_DATA_BYTE_HIGH_MSK, high) << 8 | low; > > + > > + return ret; > > +} > > + > > +static int ad8460_set_hvdac_byte(struct ad8460_state *state, > > + int index, > > + int val) > > +{ > > + int ret; > > + > > + ret = regmap_write(state->regmap, > AD8460_HVDAC_DATA_WORD_LOW(index), > > + (val & 0xFF)); > > + if (ret) > > + return ret; > > + > > + return regmap_write(state->regmap, > AD8460_HVDAC_DATA_WORD_HIGH(index), > > + ((val >> 8) & 0xFF)); > > bulk write? or do these need to be ordered? > > > +} > > + > > +static int ad8460_set_sample(struct ad8460_state *state, int val) { > > + int ret; > > + > > + ret = ad8460_enable_apg_mode(state, 1); > > + if (ret) > > + return ret; > > + > > + guard(mutex)(&state->lock); > > + ret = ad8460_set_hvdac_byte(state, 0, val); > > + if (ret) > > + return ret; > > + > > + return regmap_update_bits(state->regmap, > > + AD8460_CTRL_REG(0x02), > > + AD8460_PATTERN_DEPTH_MSK, > > + FIELD_PREP(AD8460_PATTERN_DEPTH_MSK, > 0)); } > > + > > +static int ad8460_write_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, > > + int val, > > + int val2, > > + long mask) > > +{ > > + struct ad8460_state *state = iio_priv(indio_dev); > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) > > + return ad8460_set_sample(state, val); > > + unreachable(); > > + default: > > + return -EINVAL; > > + } > > +} > > > > > +static int ad8460_probe(struct spi_device *spi) { > > + struct ad8460_state *state; > > + struct iio_dev *indio_dev; > > + struct regulator *vrefio; > > + int ret; > > + > > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state)); > > + if (!indio_dev) > > + return -ENOMEM; > > + > > + state = iio_priv(indio_dev); > > + mutex_init(&state->lock); > > + > > + state->spi = spi; > > + > > + state->regmap = devm_regmap_init_spi(spi, &ad8460_regmap_config); > > + if (IS_ERR(state->regmap)) > > + return dev_err_probe(&spi->dev, PTR_ERR(state->regmap), > > + "Failed to initialize regmap"); > > + > > + ret = devm_iio_dmaengine_buffer_setup_ext(&spi->dev, indio_dev, "tx", > > + > IIO_BUFFER_DIRECTION_OUT); > > Ah. I take back my binding comment. I assume this is mapping some non > standard interface for the parallel data flow? > > > + if (ret) > > + return dev_err_probe(&spi->dev, ret, > > + "Failed to get DMA buffer\n"); > > + > > + state->sync_clk = devm_clk_get_enabled(&spi->dev, "sync_clk"); > > + if (IS_ERR(state->sync_clk)) > > + return dev_err_probe(&spi->dev, PTR_ERR(state->sync_clk), > > + "Failed to get sync clk\n"); > > + > > + vrefio = devm_regulator_get_optional(&spi->dev, "vrefio"); > > + if (IS_ERR(vrefio)) { > > + if (PTR_ERR(vrefio) != -ENODEV) > > + return dev_err_probe(&spi->dev, PTR_ERR(vrefio), > > + "Failed to get vref regulator\n"); > > + > > + state->vref_mv = 1200; > > + > > + } else { > > + ret = regulator_enable(vrefio); > > + if (ret) > > + return dev_err_probe(&spi->dev, ret, > > + "Failed to enable vrefio > regulator\n"); > > + > > + ret = devm_add_action_or_reset(&spi->dev, > > + ad8460_regulator_disable, > > + vrefio); > > + if (ret) > > + return ret; > > + > > + ret = regulator_get_voltage(vrefio); > > + if (ret < 0) > > + return dev_err_probe(&spi->dev, ret, > > + "Failed to get ref voltage\n"); > > + > > + if (!in_range(ret, AD8460_MIN_VREFIO_UV, > AD8460_MAX_VREFIO_UV)) > > + return dev_err_probe(&spi->dev, -EINVAL, > > + "Invalid ref voltage range(%u) [%u, > %u]\n", > > + ret, AD8460_MIN_VREFIO_UV, > > + AD8460_MAX_VREFIO_UV); > > + > > + state->vref_mv = ret / 1000; > > + } > > + > > + ret = device_property_read_u32(&spi->dev, "adi,rset-ohms", > > + &state->rset_ohms); > > + if (!ret) { > > + if (!in_range(state->rset_ohms, AD8460_MIN_RSET_OHMS, > > + AD8460_MAX_RSET_OHMS)) > > + return dev_err_probe(&spi->dev, -EINVAL, > > + "Invalid resistor set range(%u) [%u, > %u]\n", > > + state->rset_ohms, > > + AD8460_MIN_RSET_OHMS, > > + AD8460_MAX_RSET_OHMS); > > + } > > + > > + ret = ad8460_reset(state); > > + if (ret) > > + return ret; > > + > > + /* Enables DAC by default */ > > + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01), > > + AD8460_HVDAC_SLEEP_MSK, > > + FIELD_PREP(AD8460_HVDAC_SLEEP_MSK, > 0)); > > + if (ret) > > + return ret; > > + > > + indio_dev->name = "ad8460"; > > + indio_dev->info = &ad8460_info; > > + indio_dev->channels = ad8460_channels; > > + indio_dev->num_channels = ARRAY_SIZE(ad8460_channels); > > + indio_dev->modes = INDIO_DIRECT_MODE | > INDIO_BUFFER_HARDWARE; > > + indio_dev->setup_ops = &ad8460_buffer_setup_ops; > > + > > + ret = devm_iio_device_register(&spi->dev, indio_dev); > > + if (ret) > > + return ret; > > + > > + ad8460_debugfs_init(indio_dev); > > + > > + return 0; > > +} > > + > > +static const struct of_device_id ad8460_of_match[] = { > > + { .compatible = "adi, ad8460" }, > > + { }, > No comma on 'null' terminators like this as we don't want to let people put > anything after them. > > > +}; > > +MODULE_DEVICE_TABLE(of, ad8460_of_match);