On Fri, May 10, 2024 at 1:42 AM 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) high-speed > 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> > --- ... > 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 ... > +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); > + if (ret) > + return dev_err_probe(&spi->dev, ret, > + "Failed to get DMA buffer\n"); It looks like the dmas property is missing in the DT bindings. However, as I suggested in my comments on that patch, it might make more sense to implement the parallel data part as an IIO backend. I assume this is using one of ADI's FPGA IP blocks to get the data? > + > + state->sync_clk = devm_clk_get_enabled(&spi->dev, "sync_clk"); The DT bindings don't specify clock-names, which is perfectly fine and perhaps even preferred since there is only one clock. But that means the ID here should be NULL instead of "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; > + } FYI, if all goes well, starting with kernel 6.10-rc1 (we'll have to wait a few weeks for this), this regulator section can be simplified to: ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vrefio"); if (ret == -ENODEV) { /* no supply, using internal 1.2V reference */ state->vref_mv = 1200; } else if (ret < 0) { return dev_err_probe(&spi->dev, ret, "Failed to get reference voltage\n"); } else { /* external reference */ state->vref_mv = ret / 1000; } if (!in_range(... We can always fix it up later though if you don't want to wait. :-) > + > + ret = device_property_read_u32(&spi->dev, "adi,rset-ohms", > + &state->rset_ohms); Since 0 isn't an allowable value for R_SET, it seems like we need to return on error here or assign a default value if the property is missing. If we do a default value, the DT bindings need to be updated to reflect that as well. > + 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), I was going to suggest giving names to the registers instead of using the number, but the datasheet doesn't do that! :-( Oh well, I guess this is the best we can do. > + 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; devm_iio_dmaengine_buffer_setup_ext() already sets the INDIO_BUFFER_HARDWARE flag so this is redundant. Also, devm_iio_dmaengine_buffer_setup_ext() is usually called here rather than early in probe because it needs a few things in indio_dev populated, IIRC. > + 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; > +} > + ...