Re: [PATCH RFC v4 15/15] iio: adc: ad4695: Add support for SPI offload

[David Lechner <dlechner@xxxxxxxxxxxx>]

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).