On Sat, 26 Oct 2024 19:01:53 -0500 David Lechner <dlechner@xxxxxxxxxxxx> wrote: > On 10/26/24 11:00 AM, Jonathan Cameron wrote: > > On Wed, 23 Oct 2024 15:59:22 -0500 > > David Lechner <dlechner@xxxxxxxxxxxx> wrote: > > > >> Add support for SPI offload to the ad4695 driver. SPI offload allows > >> sampling data at the max sample rate (500kSPS or 1MSPS). > >> > >> This is developed and tested against the ADI example FPGA design for > >> this family of ADCs [1]. > >> > >> [1]: http://analogdevicesinc.github.io/hdl/projects/ad469x_fmc/index.html > >> > >> Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx> > > A few questions inline. In general looks ok, but it's complex code and I'm > > too snowed under for a very close look at the whole thing for a least a few weeks. > > > > Jonathan > > > >> --- > >> drivers/iio/adc/Kconfig | 1 + > >> drivers/iio/adc/ad4695.c | 470 +++++++++++++++++++++++++++++++++++++++++++---- > >> 2 files changed, 440 insertions(+), 31 deletions(-) > >> > >> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > >> index 92dfb495a8ce..f76a3f62a9ad 100644 > >> --- a/drivers/iio/adc/Kconfig > >> +++ b/drivers/iio/adc/Kconfig > >> @@ -53,6 +53,7 @@ config AD4695 > >> depends on SPI > >> select REGMAP_SPI > >> select IIO_BUFFER > >> + select IIO_BUFFER_DMAENGINE > >> select IIO_TRIGGERED_BUFFER > >> help > >> Say yes here to build support for Analog Devices AD4695 and similar > > > >> +static int ad4695_offload_buffer_postenable(struct iio_dev *indio_dev) > >> +{ > >> + struct ad4695_state *st = iio_priv(indio_dev); > >> + struct spi_offload_trigger_config config = { > >> + .type = SPI_OFFLOAD_TRIGGER_DATA_READY, > >> + }; > >> + struct spi_transfer *xfer = &st->buf_read_xfer[0]; > >> + struct pwm_state state; > >> + u8 temp_chan_bit = st->chip_info->num_voltage_inputs; > >> + u8 num_slots = 0; > >> + u8 temp_en = 0; > >> + unsigned int bit; > >> + int ret; > >> + > >> + iio_for_each_active_channel(indio_dev, bit) { > >> + if (bit == temp_chan_bit) { > >> + temp_en = 1; > >> + continue; > >> + } > >> + > >> + ret = regmap_write(st->regmap, AD4695_REG_AS_SLOT(num_slots), > >> + FIELD_PREP(AD4695_REG_AS_SLOT_INX, bit)); > >> + if (ret) > >> + return ret; > >> + > >> + num_slots++; > >> + } > >> + > >> + /* > >> + * For non-offload, we could discard data to work around this > >> + * restriction, but with offload, that is not possible. > >> + */ > >> + if (num_slots < 2) { > >> + dev_err(&st->spi->dev, > >> + "At least two voltage channels must be enabled.\n"); > >> + return -EINVAL; > >> + } > >> + > >> + ret = regmap_update_bits(st->regmap, AD4695_REG_TEMP_CTRL, > >> + AD4695_REG_TEMP_CTRL_TEMP_EN, > >> + FIELD_PREP(AD4695_REG_TEMP_CTRL_TEMP_EN, > >> + temp_en)); > >> + if (ret) > >> + return ret; > >> + > >> + /* Each BUSY event means just one sample for one channel is ready. */ > >> + memset(xfer, 0, sizeof(*xfer)); > >> + xfer->offload_flags = SPI_OFFLOAD_XFER_RX_STREAM; > >> + xfer->bits_per_word = 16; > >> + xfer->len = 2; > >> + > >> + spi_message_init_with_transfers(&st->buf_read_msg, xfer, 1); > >> + st->buf_read_msg.offload = st->offload; > >> + > >> + st->spi->max_speed_hz = st->spi_max_speed_hz; > >> + ret = spi_optimize_message(st->spi, &st->buf_read_msg); > >> + st->spi->max_speed_hz = AD4695_REG_ACCESS_SCLK_HZ; > >> + if (ret) > >> + return ret; > >> + > >> + /* > >> + * NB: technically, this is part the SPI offload trigger enable, but it > >> + * doesn't work to call it from the offload trigger enable callback > >> + * due to issues with ordering with respect to entering/exiting > >> + * conversion mode. > > Give some detail on the operations order. > > > >> + */ > >> + ret = regmap_set_bits(st->regmap, AD4695_REG_GP_MODE, > >> + AD4695_REG_GP_MODE_BUSY_GP_EN); > >> + if (ret) > >> + goto err_unoptimize_message; > >> + > >> + ret = spi_offload_trigger_enable(st->offload, st->offload_trigger, > >> + &config); > >> + if (ret) > >> + goto err_disable_busy_output; > >> + > >> + ret = ad4695_enter_advanced_sequencer_mode(st, num_slots); > >> + if (ret) > >> + goto err_offload_trigger_disable; > >> + > >> + guard(mutex)(&st->cnv_pwm_lock); > >> + pwm_get_state(st->cnv_pwm, &state); > >> + /* > >> + * PWM subsystem generally rounds down, so requesting 2x minimum high > >> + * time ensures that we meet the minimum high time in any case. > >> + */ > >> + state.duty_cycle = AD4695_T_CNVH_NS * 2; > >> + ret = pwm_apply_might_sleep(st->cnv_pwm, &state); > >> + if (ret) > >> + goto err_offload_exit_conversion_mode; > >> + > >> + return 0; > >> + > >> +err_offload_exit_conversion_mode: > >> + /* have to unwind in a different order to avoid triggering offload */ > > > > Needs more details here. > > > >> + spi_offload_trigger_disable(st->offload, st->offload_trigger); > >> + ad4695_cnv_manual_trigger(st); > >> + ad4695_exit_conversion_mode(st); > >> + goto err_disable_busy_output; > >> + > >> +err_offload_trigger_disable: > >> + spi_offload_trigger_disable(st->offload, st->offload_trigger); > >> + > >> +err_disable_busy_output: > >> + regmap_clear_bits(st->regmap, AD4695_REG_GP_MODE, > >> + AD4695_REG_GP_MODE_BUSY_GP_EN); > >> + > >> +err_unoptimize_message: > >> + spi_unoptimize_message(&st->buf_read_msg); > >> + > >> + return ret; > >> +} > > > >> + > >> static int ad4695_write_raw(struct iio_dev *indio_dev, > >> struct iio_chan_spec const *chan, > >> int val, int val2, long mask) > >> @@ -779,6 +992,17 @@ static int ad4695_write_raw(struct iio_dev *indio_dev, > >> default: > >> return -EINVAL; > >> } > >> + case IIO_CHAN_INFO_SAMP_FREQ: { > >> + struct pwm_state state; > >> + > >> + if (val <= 0) > >> + return -EINVAL; > >> + > >> + guard(mutex)(&st->cnv_pwm_lock); > >> + pwm_get_state(st->cnv_pwm, &state); > > > > What limits this to rates the ADC can cope with? > > > >> + state.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, val); > >> + return pwm_apply_might_sleep(st->cnv_pwm, &state); > >> + } > >> default: > >> return -EINVAL; > >> } > > > >> static int ad4695_probe(struct spi_device *spi) > >> { > >> struct device *dev = &spi->dev; > >> struct ad4695_state *st; > >> struct iio_dev *indio_dev; > >> - struct gpio_desc *cnv_gpio; > >> bool use_internal_ldo_supply; > >> bool use_internal_ref_buffer; > >> int ret; > >> > >> - cnv_gpio = devm_gpiod_get_optional(dev, "cnv", GPIOD_OUT_LOW); > >> - if (IS_ERR(cnv_gpio)) > >> - return dev_err_probe(dev, PTR_ERR(cnv_gpio), > >> - "Failed to get CNV GPIO\n"); > >> - > >> - /* Driver currently requires CNV pin to be connected to SPI CS */ > >> - if (cnv_gpio) > >> - return dev_err_probe(dev, -ENODEV, > >> - "CNV GPIO is not supported\n"); > >> - > >> indio_dev = devm_iio_device_alloc(dev, sizeof(*st)); > >> if (!indio_dev) > >> return -ENOMEM; > >> @@ -1002,8 +1374,13 @@ static int ad4695_probe(struct spi_device *spi) > >> return -EINVAL; > >> > >> /* Registers cannot be read at the max allowable speed */ > >> + st->spi_max_speed_hz = spi->max_speed_hz; > >> spi->max_speed_hz = AD4695_REG_ACCESS_SCLK_HZ; > >> > >> + ret = devm_add_action_or_reset(dev, ad4695_restore_spi_max_speed_hz, st); > > > > Why do you need to put it back in devm? What happens after this but without > > a driver restart that uses that faster rate? > > > I should have added a comment here as this was a weird bug to trace. > > The core SPI framework sets the initial value of spi->max_speed_hz > to the minimum of the controller max rate and the max rate specified > by the devicetree. > > The SPI device lives beyond this driver, so if we bind the driver > and set spi->max_speed_hz to something other than what the SPI core > set it, then the next time we bind the driver, we don't get the > the max rate from the SPI core, but rather we changed it to when > the driver unbound. > > So on the second bind, the max rate would be the slow register > read rate instead of the actual max allowable rate. > > So we need to reset spi->max_speed_hz to what it was originally > on driver unbind so that everything works as expected on the > next bind. > > (Or we call this a SPI core bug and fix it there instead). Definitely a question to ask. Directly accessing spi_max_speed_hz may be the fundamental issue as I don't think the driver is generally expected to touch that in a dynamic fashion. Should we be instead setting it per transfer for the ones that need it controlled? Jonathan >
