> -----Original Message-----
> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: Sunday, May 12, 2024 12:44 AM
> To: Tinaco, Mariel <Mariel.Tinaco@xxxxxxxxxx>
> Cc: linux-iio@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Lars-Peter Clausen <lars@xxxxxxxxxx>; Rob Herring
> <robh@xxxxxxxxxx>; Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley
> <conor+dt@xxxxxxxxxx>; Liam Girdwood <lgirdwood@xxxxxxxxx>; Mark Brown
> <broonie@xxxxxxxxxx>; Hennerich, Michael <Michael.Hennerich@xxxxxxxxxx>;
> Marcelo Schmitt <marcelo.schmitt1@xxxxxxxxx>; Dimitri Fedrau
> <dima.fedrau@xxxxxxxxx>; Guenter Roeck <linux@xxxxxxxxxxxx>
> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
>
> 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);
