RE: [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC

[Tinaco, Mariel]

> -----Original Message-----
> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: Sunday, September 8, 2024 1:15 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>; Hennerich,
> Michael <Michael.Hennerich@xxxxxxxxxx>; Conor Dooley
> <conor+dt@xxxxxxxxxx>; Marcelo Schmitt <marcelo.schmitt1@xxxxxxxxx>;
> Dimitri Fedrau <dima.fedrau@xxxxxxxxx>; David Lechner
> <dlechner@xxxxxxxxxxxx>; Nuno Sá <noname.nuno@xxxxxxxxx>
> Subject: Re: [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC
> 
> [External]
> 
> On Wed, 4 Sep 2024 10:30:40 +0800
> Mariel Tinaco <Mariel.Tinaco@xxxxxxxxxx> wrote:
> 
> > The AD8460 is a “bits in, power out” high voltage, high-power,
> > high-speed 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>
> A few comments inline.
> 
> Jonathan
> 
> 
> >  obj-$(CONFIG_ADI_AXI_DAC) += adi-axi-dac.o diff --git
> > a/drivers/iio/dac/ad8460.c b/drivers/iio/dac/ad8460.c new file mode
> > 100644 index 000000000000..6428bfaf0bd7
> > --- /dev/null
> > +++ b/drivers/iio/dac/ad8460.c
> > @@ -0,0 +1,932 @@
> > +// 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/consumer.h> #include <linux/iio/events.h> #include
> > +<linux/iio/iio.h>
> 
> Given there are lots of IIO specific includes, probably a case where pulling
> them out as a separate block after the main includes makes sense.
> 
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h> #include <linux/spi/spi.h>
> >
> ...
> 
> 
> > +
> > +	state = iio_priv(indio_dev);
> > +	mutex_init(&state->lock);
> 
> Trivial but there is no a devm_mutex_init() that deals with the obscure debug
> case where mutex uninit makes sense. Might as well use it here.
> 
> > +
> > +	indio_dev->name = "ad8460";
> > +	indio_dev->info = &ad8460_info;
> > +
> > +	state->spi = spi;
> > +	dev = &spi->dev;
> > +
> > +	state->regmap = devm_regmap_init_spi(spi,
> &ad8460_regmap_config);
> > +	if (IS_ERR(state->regmap))
> > +		return dev_err_probe(dev, PTR_ERR(state->regmap),
> > +				     "Failed to initialize regmap");
> > +
> > +	state->sync_clk = devm_clk_get_enabled(dev, NULL);
> > +	if (IS_ERR(state->sync_clk))
> > +		return dev_err_probe(dev, PTR_ERR(state->sync_clk),
> > +				     "Failed to get sync clk\n");
> > +
> > +	state->tmp_adc_channel = devm_iio_channel_get(dev, "ad8460-
> tmp");
> > +	if (IS_ERR_OR_NULL(state->tmp_adc_channel)) {
> > +		indio_dev->channels = ad8460_channels;
> > +		indio_dev->num_channels = ARRAY_SIZE(ad8460_channels);
> > +	} else {
> > +		indio_dev->channels = ad8460_channels_with_tmp_adc;
> > +		indio_dev->num_channels =
> ARRAY_SIZE(ad8460_channels_with_tmp_adc);
> > +	}
> > +
> Add and enable the various other supplies. They are probably always on in
> which case the regulator framework will work it's magic to avoid use having to
> care that they aren't in the dts.

If the other supplies are added, do they need to be tied up as well to the
Private structure just like ref_1p2v? Or do I just apply the 
devm_regulator_get_enable_read_voltage to it?

ret = devm_regulator_get_enable_read_voltage(&client->dev, "vdd");
If (ret < 0 && ret != -ENODEV)
	return dev_err_probe(ltc2309->dev, ret,
				"failed to get vref voltage\n");

> 
> > +	ret = devm_regulator_get_enable_read_voltage(dev, "refio_1p2v");
> > +	if (ret == -ENODEV)
> > +		state->refio_1p2v_mv = 1200;
> > +	else if (ret >= 0)
> > +		state->refio_1p2v_mv = ret / 1000;
> > +	else
> > +		return dev_err_probe(dev, ret, "Failed to get reference
> > +voltage\n");
> > +
> ...
> 
> > +	ret = device_property_read_u32_array(dev, "adi,range-microamp",
> > +					     tmp, ARRAY_SIZE(tmp));
> > +	if (!ret) {
> > +		if (in_range(tmp[1], 0,
> AD8460_ABS_MAX_OVERCURRENT_UA))
> > +			regmap_write(state->regmap,
> AD8460_CTRL_REG(0x08),
> > +				     FIELD_PREP(AD8460_FAULT_ARM_MSK, 1)
> |
> > +				     AD8460_CURRENT_LIMIT_CONV(tmp[1]));
> 
> Your binding has a fixed set of values.  Yet this is anything in range.
> Which is correct?  Based on datasheet I think it is flexible but that you have
> listed the example values instead of the limits.
> 
> 
> > +
> > +		if (in_range(tmp[0], -AD8460_ABS_MAX_OVERCURRENT_UA,
> 0))
> > +			regmap_write(state->regmap,
> AD8460_CTRL_REG(0x09),
> > +				     FIELD_PREP(AD8460_FAULT_ARM_MSK, 1)
> |
> > +
> AD8460_CURRENT_LIMIT_CONV(abs(tmp[0])));
> > +	}
> > +
> > +	ret = device_property_read_u32_array(dev, "adi,range-microvolt",
> > +					     tmp, ARRAY_SIZE(tmp));
> > +	if (!ret) {
> > +		if (in_range(tmp[1], 0,
> AD8460_ABS_MAX_OVERVOLTAGE_UV))
> > +			regmap_write(state->regmap,
> AD8460_CTRL_REG(0x0A),
> > +				     FIELD_PREP(AD8460_FAULT_ARM_MSK, 1)
> |
> > +				     AD8460_VOLTAGE_LIMIT_CONV(tmp[1]));
> > +
> > +		if (in_range(tmp[0], -AD8460_ABS_MAX_OVERVOLTAGE_UV,
> 0))
> > +			regmap_write(state->regmap,
> AD8460_CTRL_REG(0x0B),
> > +				     FIELD_PREP(AD8460_FAULT_ARM_MSK, 1)
> |
> > +
> AD8460_VOLTAGE_LIMIT_CONV(abs(tmp[0])));
> > +	}
> > +
> > +	ret = device_property_read_u32(dev, "adi,max-millicelsius", &temp);
> > +	if (!ret) {
> > +		if (in_range(temp, AD8460_MIN_OVERTEMPERATURE_MC,
> > +			     AD8460_MAX_OVERTEMPERATURE_MC))
> > +			regmap_write(state->regmap,
> AD8460_CTRL_REG(0x0C),
> > +				     FIELD_PREP(AD8460_FAULT_ARM_MSK, 1)
> |
> > +				     AD8460_TEMP_LIMIT_CONV(abs(temp)));
> > +	}
> > +
> > +	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->modes = INDIO_DIRECT_MODE;
> > +	indio_dev->setup_ops = &ad8460_buffer_setup_ops;
> > +
> > +	ret = devm_iio_dmaengine_buffer_setup_ext(dev, indio_dev, "tx",
> > +
> IIO_BUFFER_DIRECTION_OUT);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret,
> > +				     "Failed to get DMA buffer\n");
> > +
> > +	return devm_iio_device_register(dev, indio_dev); }