On Thu, 12 Sep 2024 17:54:35 +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>
Hi Mariel
A few minor comments from me. I'd like it to sit on the list a while
longer, but if there is nothing else I can make minor tweaks whilst
applying.
Jonathan
> diff --git a/drivers/iio/dac/ad8460.c b/drivers/iio/dac/ad8460.c
> new file mode 100644
> index 000000000000..9ce3a0f288ba
> --- /dev/null
> +++ b/drivers/iio/dac/ad8460.c
> @@ -0,0 +1,947 @@
> +static struct iio_chan_spec_ext_info ad8460_ext_info[] = {
> + AD8460_CHAN_EXT_INFO("raw0", 0, ad8460_dac_input_read,
> + ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("raw1", 1, ad8460_dac_input_read,
> + ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("raw2", 2, ad8460_dac_input_read,
> + ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("raw3", 3, ad8460_dac_input_read,
> + ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("raw4", 4, ad8460_dac_input_read,
> + ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("raw5", 5, ad8460_dac_input_read,
> + ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("raw6", 6, ad8460_dac_input_read,
> + ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("raw7", 7, ad8460_dac_input_read,
> + ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("raw8", 8, ad8460_dac_input_read,
> + ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("raw9", 9, ad8460_dac_input_read,
> + ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("raw10", 10, ad8460_dac_input_read,
> + ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("raw11", 11, ad8460_dac_input_read,
> + ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("raw12", 12, ad8460_dac_input_read,
> + ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("raw13", 13, ad8460_dac_input_read,
> + ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("raw14", 14, ad8460_dac_input_read,
> + ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("raw15", 15, ad8460_dac_input_read,
> + ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("toggle_en", 0, ad8460_read_toggle_en,
> + ad8460_write_toggle_en),
> + AD8460_CHAN_EXT_INFO("symbol", 0, ad8460_read_symbol,
> + ad8460_write_symbol),
> + AD8460_CHAN_EXT_INFO("powerdown", 0, ad8460_read_powerdown,
> + ad8460_write_powerdown),
> + IIO_ENUM("powerdown_mode", IIO_SEPARATE, &ad8460_powerdown_mode_enum),
> + IIO_ENUM_AVAILABLE("powerdown_mode", IIO_SHARED_BY_TYPE,
> + &ad8460_powerdown_mode_enum),
> + {}
{ }
is my style preference. Mostly I want consistency and happened to pick this
for IIO.
> +};
> +#define AD8460_TEMP_CHAN { \
> + .type = IIO_TEMP, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .output = 1, \
An output temperature channel? That's a heater which seems unlikely
on this device.
> + .indexed = 1, \
> + .channel = 0, \
> + .scan_index = -1, \
> + .event_spec = ad8460_events, \
> + .num_event_specs = 1, \
> +}
> +static int ad8460_probe(struct spi_device *spi)
> +{
> + struct ad8460_state *state;
> + struct iio_dev *indio_dev;
> + struct device *dev;
> + u32 tmp[2], temp;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + state = iio_priv(indio_dev);
> +
> + indio_dev->name = "ad8460";
> + indio_dev->info = &ad8460_info;
> +
> + state->spi = spi;
> + dev = &spi->dev;
Might as well do this one where you declare above.
struct device *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");
> +
> + devm_mutex_init(dev, &state->lock);
Check return value. devm registration can potentially fail.
...
> +
> + /* 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));
regmap_clear_bits() perhaps.
> + 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);
> +}
