> -----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);