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

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 4 Sep 2024 10:30:40 +0800
Mariel Tinaco <Mariel.Tinaco@analog.com> 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@analog.com>
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.

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