Message-ID: 
 <SJ0PR03MB62241801F72B21EEC9CDCCBD91D42@SJ0PR03MB6224.namprd03.prod.outlook.com>
Date: Mon, 24 Jun 2024 04:19:22 +0000
From: "Tinaco, Mariel" <Mariel.Tinaco@...log.com>
To: Jonathan Cameron <jic23@...nel.org>
CC: "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Lars-Peter
 Clausen <lars@...afoo.de>, Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski
	<krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Liam Girdwood
	<lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
        "Hennerich, Michael"
	<Michael.Hennerich@...log.com>,
        Marcelo Schmitt <marcelo.schmitt1@...il.com>,
        Dimitri Fedrau <dima.fedrau@...il.com>,
        Guenter Roeck <linux@...ck-us.net>
Subject: RE: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC



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

Powered by blists - more mailing lists