| Message ID | 20241023-dlech-mainline-spi-engine-offload-2-v4-15-f8125b99f5a1@baylibre.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | spi: axi-spi-engine: add offload support | expand |
On Wed, 23 Oct 2024 15:59:22 -0500 David Lechner <dlechner@baylibre.com> 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@baylibre.com> 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? > + if (ret) > + return ret; > + > st->regmap = devm_regmap_init_spi(spi, &ad4695_regmap_config); > if (IS_ERR(st->regmap)) > return dev_err_probe(dev, PTR_ERR(st->regmap), > @@ -1014,6 +1391,11 @@ static int ad4695_probe(struct spi_device *spi) > return dev_err_probe(dev, PTR_ERR(st->regmap16), > "Failed to initialize regmap16\n"); > > + st->cnv_gpio = devm_gpiod_get_optional(dev, "cnv", GPIOD_OUT_LOW); > + if (IS_ERR(st->cnv_gpio)) > + return dev_err_probe(dev, PTR_ERR(st->cnv_gpio), > + "Failed to get CNV GPIO\n"); > + > ret = devm_regulator_bulk_get_enable(dev, > ARRAY_SIZE(ad4695_power_supplies), > ad4695_power_supplies); > @@ -1139,14 +1521,39 @@ static int ad4695_probe(struct spi_device *spi) > indio_dev->info = &ad4695_info; > indio_dev->modes = INDIO_DIRECT_MODE; > indio_dev->channels = st->iio_chan; > - indio_dev->num_channels = st->chip_info->num_voltage_inputs + 2; > > - ret = devm_iio_triggered_buffer_setup(dev, indio_dev, > - iio_pollfunc_store_time, > - ad4695_trigger_handler, > - &ad4695_buffer_setup_ops); > - if (ret) > - return ret; > + static const struct spi_offload_config ad4695_offload_config = { > + .capability_flags = SPI_OFFLOAD_CAP_TRIGGER > + | SPI_OFFLOAD_CAP_RX_STREAM_DMA, > + }; > + > + st->offload = devm_spi_offload_get(dev, spi, &ad4695_offload_config); > + ret = PTR_ERR_OR_ZERO(st->offload); > + if (ret && ret != -ENODEV) > + return dev_err_probe(dev, ret, "failed to get SPI offload\n"); > + > + if (ret == -ENODEV) { > + /* If no SPI offload, fall back to low speed usage. */ > + dev_info(dev, "SPI offload not available\n"); As previous. Too noisy for a normal thing that might happen. Maybe if we can't figure it out from anything userspace an see we could add a print on the 'offload is enabled' side of things. > + > + /* Driver currently requires CNV pin to be connected to SPI CS */ > + if (st->cnv_gpio) > + return dev_err_probe(dev, -EINVAL, > + "CNV GPIO is not supported\n"); > + > + indio_dev->num_channels = st->chip_info->num_voltage_inputs + 2; > + > + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, > + iio_pollfunc_store_time, > + ad4695_trigger_handler, > + &ad4695_buffer_setup_ops); > + if (ret) > + return ret; > + } else { > + ret = ad4695_probe_spi_offload(indio_dev, st); > + if (ret) > + return ret; > + }
On 10/26/24 11:00 AM, Jonathan Cameron wrote: > On Wed, 23 Oct 2024 15:59:22 -0500 > David Lechner <dlechner@baylibre.com> 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@baylibre.com> > 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).
On 10/26/24 11:00 AM, Jonathan Cameron wrote: > On Wed, 23 Oct 2024 15:59:22 -0500 > David Lechner <dlechner@baylibre.com> wrote: > ... >> 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? > Nothing at the moment. The "obvious" thing to do would be to limit this to the max rate from the datasheet. But that feels a little too strict to me since maybe the PWM can't get exactly the max rate, but can get the max rate + 1% or so. It seems like we should allow that too. It's not like the ADC is going to not work if we go a few Hz over the datasheet rating. Maybe limit it to max + 10% or something like that?
On Sat, 26 Oct 2024 19:01:53 -0500 David Lechner <dlechner@baylibre.com> wrote: > On 10/26/24 11:00 AM, Jonathan Cameron wrote: > > On Wed, 23 Oct 2024 15:59:22 -0500 > > David Lechner <dlechner@baylibre.com> 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@baylibre.com> > > 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 >
On Sat, 26 Oct 2024 19:05:44 -0500 David Lechner <dlechner@baylibre.com> wrote: > On 10/26/24 11:00 AM, Jonathan Cameron wrote: > > On Wed, 23 Oct 2024 15:59:22 -0500 > > David Lechner <dlechner@baylibre.com> wrote: > > > > ... > > >> 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? > > > > Nothing at the moment. The "obvious" thing to do would > be to limit this to the max rate from the datasheet. > > But that feels a little too strict to me since maybe the > PWM can't get exactly the max rate, but can get the max > rate + 1% or so. It seems like we should allow that too. > It's not like the ADC is going to not work if we go a > few Hz over the datasheet rating. > > Maybe limit it to max + 10% or something like that? Clamp it at datasheet value. That's what is presumably verified not 10% over. If that needs relaxing in future, the datasheet should be updated to reflect the higher verified value. Jonathan
On 10/27/24 4:12 AM, Jonathan Cameron wrote: > On Sat, 26 Oct 2024 19:01:53 -0500 > David Lechner <dlechner@baylibre.com> wrote: > >> On 10/26/24 11:00 AM, Jonathan Cameron wrote: >>> On Wed, 23 Oct 2024 15:59:22 -0500 >>> David Lechner <dlechner@baylibre.com> wrote: >>> ... >>> >>>> 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 > The problem is that we are using regmap and that doesn't have a way to specify the max frequency for register reads that is different from other uses of the SPI bus (i.e. reading sample data). So we could fix it in the generic SPI regmap (not exactly trivial) or we could write our own regmap read/write callbacks in this driver that properly sets the per-transfer max speed.
On Sun, 27 Oct 2024 14:52:17 -0500 David Lechner <dlechner@baylibre.com> wrote: > On 10/27/24 4:12 AM, Jonathan Cameron wrote: > > On Sat, 26 Oct 2024 19:01:53 -0500 > > David Lechner <dlechner@baylibre.com> wrote: > > > >> On 10/26/24 11:00 AM, Jonathan Cameron wrote: > >>> On Wed, 23 Oct 2024 15:59:22 -0500 > >>> David Lechner <dlechner@baylibre.com> wrote: > >>> > > ... > > >>> > >>>> 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 > > > > The problem is that we are using regmap and that doesn't have > a way to specify the max frequency for register reads that is > different from other uses of the SPI bus (i.e. reading sample > data). So we could fix it in the generic SPI regmap (not exactly > trivial) or we could write our own regmap read/write callbacks > in this driver that properly sets the per-transfer max speed. Custom read / write callbacks seems the best approach at first glance, given this is pretty rare thing to do. > >
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 diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c index 595ec4158e73..c150851d1fb1 100644 --- a/drivers/iio/adc/ad4695.c +++ b/drivers/iio/adc/ad4695.c @@ -19,14 +19,18 @@ #include <linux/device.h> #include <linux/err.h> #include <linux/gpio/consumer.h> +#include <linux/iio/buffer-dmaengine.h> #include <linux/iio/buffer.h> #include <linux/iio/iio.h> #include <linux/iio/triggered_buffer.h> #include <linux/iio/trigger_consumer.h> #include <linux/minmax.h> +#include <linux/mutex.h> #include <linux/property.h> +#include <linux/pwm.h> #include <linux/regmap.h> #include <linux/regulator/consumer.h> +#include <linux/spi/spi-offload.h> #include <linux/spi/spi.h> #include <linux/units.h> @@ -66,6 +70,8 @@ #define AD4695_REG_STD_SEQ_CONFIG 0x0024 #define AD4695_REG_GPIO_CTRL 0x0026 #define AD4695_REG_GP_MODE 0x0027 +#define AD4695_REG_GP_MODE_BUSY_GP_SEL BIT(5) +#define AD4695_REG_GP_MODE_BUSY_GP_EN BIT(1) #define AD4695_REG_TEMP_CTRL 0x0029 #define AD4695_REG_TEMP_CTRL_TEMP_EN BIT(0) #define AD4695_REG_CONFIG_IN(n) (0x0030 | (n)) @@ -87,6 +93,7 @@ /* timing specs */ #define AD4695_T_CONVERT_NS 415 +#define AD4695_T_CNVH_NS 10 #define AD4695_T_WAKEUP_HW_MS 3 #define AD4695_T_WAKEUP_SW_MS 3 #define AD4695_T_REFBUF_MS 100 @@ -121,9 +128,18 @@ struct ad4695_channel_config { struct ad4695_state { struct spi_device *spi; + struct spi_offload *offload; + struct spi_offload_trigger *offload_trigger; + u32 spi_max_speed_hz; struct regmap *regmap; struct regmap *regmap16; struct gpio_desc *reset_gpio; + /* currently PWM CNV only supported with SPI offload use */ + struct pwm_device *cnv_pwm; + /* protects against concurrent use of cnv_pwm */ + struct mutex cnv_pwm_lock; + /* offload also requires separate gpio to manually control CNV */ + struct gpio_desc *cnv_gpio; /* voltages channels plus temperature and timestamp */ struct iio_chan_spec iio_chan[AD4695_MAX_CHANNELS + 2]; struct ad4695_channel_config channels_cfg[AD4695_MAX_CHANNELS]; @@ -571,6 +587,167 @@ static irqreturn_t ad4695_trigger_handler(int irq, void *p) return IRQ_HANDLED; } +static void ad4695_cnv_manual_trigger(struct ad4695_state *st) +{ + gpiod_set_value_cansleep(st->cnv_gpio, 1); + ndelay(10); + gpiod_set_value_cansleep(st->cnv_gpio, 0); +} + +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. + */ + 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 */ + 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_offload_buffer_predisable(struct iio_dev *indio_dev) +{ + struct ad4695_state *st = iio_priv(indio_dev); + struct pwm_state state; + int ret; + + scoped_guard(mutex, &st->cnv_pwm_lock) { + pwm_get_state(st->cnv_pwm, &state); + state.duty_cycle = 0; + ret = pwm_apply_might_sleep(st->cnv_pwm, &state); + if (ret) + return ret; + } + + spi_offload_trigger_disable(st->offload, st->offload_trigger); + + /* + * We have to trigger on more conversion to ensure that the exit + * conversion mode command works. + */ + ad4695_cnv_manual_trigger(st); + + ret = ad4695_exit_conversion_mode(st); + if (ret) + return ret; + + ret = regmap_clear_bits(st->regmap, AD4695_REG_GP_MODE, + AD4695_REG_GP_MODE_BUSY_GP_EN); + if (ret) + return ret; + + spi_unoptimize_message(&st->buf_read_msg); + + return 0; +} + +static const struct iio_buffer_setup_ops ad4695_offload_buffer_setup_ops = { + .postenable = ad4695_offload_buffer_postenable, + .predisable = ad4695_offload_buffer_predisable, +}; + /** * ad4695_read_one_sample - Read a single sample using single-cycle mode * @st: The AD4695 state @@ -583,13 +760,20 @@ static irqreturn_t ad4695_trigger_handler(int irq, void *p) */ static int ad4695_read_one_sample(struct ad4695_state *st, unsigned int address) { - struct spi_transfer xfer[2] = { }; - int ret, i = 0; + struct spi_transfer xfer = { }; + int ret; ret = ad4695_set_single_cycle_mode(st, address); if (ret) return ret; + /* + * If CNV is connected to CS, the previous function will have triggered + * the conversion, otherwise, we do it manually. + */ + if (st->cnv_gpio) + ad4695_cnv_manual_trigger(st); + /* * Setting the first channel to the temperature channel isn't supported * in single-cycle mode, so we have to do an extra xfer to read the @@ -598,23 +782,29 @@ static int ad4695_read_one_sample(struct ad4695_state *st, unsigned int address) if (address == AD4695_CMD_TEMP_CHAN) { /* We aren't reading, so we can make this a short xfer. */ st->cnv_cmd2 = AD4695_CMD_TEMP_CHAN << 3; - xfer[0].tx_buf = &st->cnv_cmd2; - xfer[0].len = 1; - xfer[0].cs_change = 1; - xfer[0].cs_change_delay.value = AD4695_T_CONVERT_NS; - xfer[0].cs_change_delay.unit = SPI_DELAY_UNIT_NSECS; + xfer.tx_buf = &st->cnv_cmd2; + xfer.len = 1; + + ret = spi_sync_transfer(st->spi, &xfer, 1); + if (ret) + return ret; - i = 1; + /* + * If CNV is connected to CS, the previous function will have + * triggered the conversion, otherwise, we do it manually. + */ + if (st->cnv_gpio) + ad4695_cnv_manual_trigger(st); } /* Then read the result and exit conversion mode. */ st->cnv_cmd = AD4695_CMD_EXIT_CNV_MODE << 11; - xfer[i].bits_per_word = 16; - xfer[i].tx_buf = &st->cnv_cmd; - xfer[i].rx_buf = &st->raw_data; - xfer[i].len = 2; + xfer.bits_per_word = 16; + xfer.tx_buf = &st->cnv_cmd; + xfer.rx_buf = &st->raw_data; + xfer.len = 2; - return spi_sync_transfer(st->spi, xfer, i + 1); + return spi_sync_transfer(st->spi, &xfer, 1); } static int ad4695_read_raw(struct iio_dev *indio_dev, @@ -721,11 +911,34 @@ static int ad4695_read_raw(struct iio_dev *indio_dev, default: return -EINVAL; } + case IIO_CHAN_INFO_SAMP_FREQ: { + struct pwm_state state; + + ret = pwm_get_state_hw(st->cnv_pwm, &state); + if (ret) + return ret; + + *val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, state.period); + + return IIO_VAL_INT; + } default: return -EINVAL; } } +static int ad4695_write_raw_get_fmt(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + long mask) +{ + switch (mask) { + case IIO_CHAN_INFO_SAMP_FREQ: + return IIO_VAL_INT; + default: + return IIO_VAL_INT_PLUS_MICRO; + } +} + 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); + state.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, val); + return pwm_apply_might_sleep(st->cnv_pwm, &state); + } default: return -EINVAL; } @@ -857,6 +1081,7 @@ static int ad4695_debugfs_reg_access(struct iio_dev *indio_dev, static const struct iio_info ad4695_info = { .read_raw = &ad4695_read_raw, + .write_raw_get_fmt = &ad4695_write_raw_get_fmt, .write_raw = &ad4695_write_raw, .read_avail = &ad4695_read_avail, .debugfs_reg_access = &ad4695_debugfs_reg_access, @@ -970,26 +1195,173 @@ static int ad4695_parse_channel_cfg(struct ad4695_state *st) return 0; } +static bool ad4695_offload_trigger_match(void *priv, + enum spi_offload_trigger_type type, + u64 *args, u32 nargs) +{ + if (type != SPI_OFFLOAD_TRIGGER_DATA_READY) + return false; + + // TODO: create macros for args[0] and args[1] + // args[0] is the trigger signal, 0 == BUSY + // args[1] is the GPIO pin number, 0 == GP0, 3 == GP3 + if (nargs != 2 || args[0] != 0) + return false; + + return true; +} + +static int ad4695_offload_trigger_request(void *priv, + enum spi_offload_trigger_type type, + u64 *args, u32 nargs) +{ + struct ad4695_state *st = priv; + + /* Should already be validated by match, but just in case. */ + if (nargs != 2) + return -EINVAL; + + /* DT tells us if BUSY signal uses GP0 or GP3 */ + if (args[1] == 3) + return regmap_set_bits(st->regmap, AD4695_REG_GP_MODE, + AD4695_REG_GP_MODE_BUSY_GP_SEL); + + return regmap_clear_bits(st->regmap, AD4695_REG_GPIO_CTRL, + AD4695_REG_GP_MODE_BUSY_GP_SEL); +} + +static int ad4695_offload_trigger_validate(void *priv, struct spi_offload_trigger_config *config) +{ + if (config->type != SPI_OFFLOAD_TRIGGER_DATA_READY) + return -EINVAL; + + return 0; +} + +/* + * NB: There are no enable/disable callbacks here due to requiring a SPI + * message to enable or disable the BUSY output on the ADC. + */ +static const struct spi_offload_trigger_ops ad4695_offload_trigger_ops = { + .match = ad4695_offload_trigger_match, + .request = ad4695_offload_trigger_request, + .validate = ad4695_offload_trigger_validate, +}; + +static void ad4695_restore_spi_max_speed_hz(void *data) +{ + struct ad4695_state *st = data; + + st->spi->max_speed_hz = st->spi_max_speed_hz; +} + +static void ad4695_pwm_disable(void *pwm) +{ + pwm_disable(pwm); +} + +static int ad4695_probe_spi_offload(struct iio_dev *indio_dev, + struct ad4695_state *st) +{ + struct device *dev = &st->spi->dev; + struct spi_offload_trigger_info trigger_info = { + .name = "data-ready", + .id = 0, + .parent = dev, + .fwnode = dev_fwnode(dev), + .ops = &ad4695_offload_trigger_ops, + }; + struct spi_offload_trigger *trigger; + struct pwm_state pwm_state; + struct dma_chan *rx_dma; + int ret, i; + + indio_dev->num_channels = st->chip_info->num_voltage_inputs + 1; + indio_dev->setup_ops = &ad4695_offload_buffer_setup_ops; + + if (!st->cnv_gpio) + return dev_err_probe(dev, -ENODEV, + "CNV GPIO is required for SPI offload\n"); + + trigger = devm_spi_offload_trigger_alloc(dev, &trigger_info); + if (IS_ERR(trigger)) + return dev_err_probe(dev, PTR_ERR(trigger), + "failed to allocate offload trigger\n"); + + ret = devm_spi_offload_trigger_register(dev, trigger, st); + if (ret) + return dev_err_probe(dev, ret, + "failed to register offload trigger\n"); + + st->offload_trigger = devm_spi_offload_trigger_get(dev, st->offload, + SPI_OFFLOAD_TRIGGER_DATA_READY); + if (IS_ERR(st->offload_trigger)) + return dev_err_probe(dev, PTR_ERR(st->offload_trigger), + "failed to get offload trigger\n"); + + /* Currently, only self-provided trigger is supported. */ + if (st->offload_trigger != trigger) + return dev_err_probe(dev, -EINVAL, "offload trigger mismatch\n"); + + ret = devm_mutex_init(dev, &st->cnv_pwm_lock); + if (ret) + return ret; + + st->cnv_pwm = devm_pwm_get(dev, NULL); + if (IS_ERR(st->cnv_pwm)) + return dev_err_probe(dev, PTR_ERR(st->cnv_pwm), + "failed to get CNV PWM\n"); + + pwm_init_state(st->cnv_pwm, &pwm_state); + + /* If firmware didn't provide default rate, use 10kHz (arbitrary). */ + if (pwm_state.period == 0) + pwm_state.period = 100 * MILLI; + + pwm_state.enabled = true; + + ret = pwm_apply_might_sleep(st->cnv_pwm, &pwm_state); + if (ret) + return dev_err_probe(dev, ret, "failed to apply CNV PWM\n"); + + ret = devm_add_action_or_reset(dev, ad4695_pwm_disable, st->cnv_pwm); + if (ret) + return ret; + + rx_dma = devm_spi_offload_rx_stream_request_dma_chan(dev, st->offload); + if (IS_ERR(rx_dma)) + return dev_err_probe(dev, PTR_ERR(rx_dma), + "failed to get offload RX DMA\n"); + + /* + * REVISIT: ideally, we would ask the RX DMA stream what the + * buffer layout is. Right now, the only supported offload is + * the ADI ad469x HDL project which always uses 32-bit word + * size for data values, regardless of the SPI bits per word. + */ + + for (i = 0; i < indio_dev->num_channels; i++) { + struct iio_chan_spec *chan = &st->iio_chan[i]; + + /* update storagebits to match offload capabilities */ + chan->scan_type.storagebits = 32; + /* add sample frequency for PWM CNV trigger */ + chan->info_mask_separate |= BIT(IIO_CHAN_INFO_SAMP_FREQ); + } + + return devm_iio_dmaengine_buffer_setup_ext2(dev, indio_dev, rx_dma, + IIO_BUFFER_DIRECTION_IN); +} + 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); + if (ret) + return ret; + st->regmap = devm_regmap_init_spi(spi, &ad4695_regmap_config); if (IS_ERR(st->regmap)) return dev_err_probe(dev, PTR_ERR(st->regmap), @@ -1014,6 +1391,11 @@ static int ad4695_probe(struct spi_device *spi) return dev_err_probe(dev, PTR_ERR(st->regmap16), "Failed to initialize regmap16\n"); + st->cnv_gpio = devm_gpiod_get_optional(dev, "cnv", GPIOD_OUT_LOW); + if (IS_ERR(st->cnv_gpio)) + return dev_err_probe(dev, PTR_ERR(st->cnv_gpio), + "Failed to get CNV GPIO\n"); + ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(ad4695_power_supplies), ad4695_power_supplies); @@ -1139,14 +1521,39 @@ static int ad4695_probe(struct spi_device *spi) indio_dev->info = &ad4695_info; indio_dev->modes = INDIO_DIRECT_MODE; indio_dev->channels = st->iio_chan; - indio_dev->num_channels = st->chip_info->num_voltage_inputs + 2; - ret = devm_iio_triggered_buffer_setup(dev, indio_dev, - iio_pollfunc_store_time, - ad4695_trigger_handler, - &ad4695_buffer_setup_ops); - if (ret) - return ret; + static const struct spi_offload_config ad4695_offload_config = { + .capability_flags = SPI_OFFLOAD_CAP_TRIGGER + | SPI_OFFLOAD_CAP_RX_STREAM_DMA, + }; + + st->offload = devm_spi_offload_get(dev, spi, &ad4695_offload_config); + ret = PTR_ERR_OR_ZERO(st->offload); + if (ret && ret != -ENODEV) + return dev_err_probe(dev, ret, "failed to get SPI offload\n"); + + if (ret == -ENODEV) { + /* If no SPI offload, fall back to low speed usage. */ + dev_info(dev, "SPI offload not available\n"); + + /* Driver currently requires CNV pin to be connected to SPI CS */ + if (st->cnv_gpio) + return dev_err_probe(dev, -EINVAL, + "CNV GPIO is not supported\n"); + + indio_dev->num_channels = st->chip_info->num_voltage_inputs + 2; + + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, + iio_pollfunc_store_time, + ad4695_trigger_handler, + &ad4695_buffer_setup_ops); + if (ret) + return ret; + } else { + ret = ad4695_probe_spi_offload(indio_dev, st); + if (ret) + return ret; + } return devm_iio_device_register(dev, indio_dev); } @@ -1183,3 +1590,4 @@ MODULE_AUTHOR("Ramona Gradinariu <ramona.gradinariu@analog.com>"); MODULE_AUTHOR("David Lechner <dlechner@baylibre.com>"); MODULE_DESCRIPTION("Analog Devices AD4695 ADC driver"); MODULE_LICENSE("GPL"); +MODULE_IMPORT_NS(IIO_DMAENGINE_BUFFER);
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@baylibre.com> --- drivers/iio/adc/Kconfig | 1 + drivers/iio/adc/ad4695.c | 470 +++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 440 insertions(+), 31 deletions(-)