Re: [PATCH 1/2] iio: adc: ad4695: implement triggered buffer

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 8/9/24 9:24 AM, Nuno Sá wrote:
> On Wed, 2024-08-07 at 15:02 -0500, David Lechner wrote:
>> This implements buffered reads for the ad4695 driver using the typical
>> triggered buffer implementation, including adding a soft timestamp
>> channel.
>>
>> The chip has 4 different modes for doing conversions. The driver is
>> using the advanced sequencer mode since that is the only mode that
>> allows individual configuration of all aspects each channel (e.g.
>> bipolar config currently and oversampling to be added in the future).
>>
>> Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx>
>> ---
> 
> Hi David,
> 
> Just two nit comments...
> 
> Reviewed-by: Nuno Sa <nuno.sa@xxxxxxxxxx>
> 
>>  drivers/iio/adc/ad4695.c | 233 ++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 230 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
>> index 007ecb951bc3..a3bd5be36134 100644
>> --- a/drivers/iio/adc/ad4695.c
>> +++ b/drivers/iio/adc/ad4695.c
> 
> ...
> 
>>
>>  
>> +static int ad4695_buffer_preenable(struct iio_dev *indio_dev)
>> +{
>> +	struct ad4695_state *st = iio_priv(indio_dev);
>> +	struct spi_transfer *xfer;
>> +	u8 temp_chan_bit = st->chip_info->num_voltage_inputs;
>> +	bool temp_chan_en = false;
>> +	u32 reg, mask, val, bit, num_xfer, num_slots;
>> +	int ret;
>> +
>> +	/*
>> +	 * We are using the advanced sequencer since it is the only way to read
>> +	 * multiple channels that allows individual configuration of each
>> +	 * voltage input channel. Slot 0 in the advanced sequencer is used to
>> +	 * account for the gap between trigger polls - we don't read data from
>> +	 * this slot. Each enabled voltage channel is assigned a slot starting
>> +	 * with slot 1.
>> +	 */
>> +	num_slots = 1;
>> +
>> +	memset(st->buf_read_xfer, 0, sizeof(st->buf_read_xfer));
>> +
>> +	/* First xfer is only to trigger conversion of slot 1, so no rx. */
>> +	xfer = &st->buf_read_xfer[0];
>> +	xfer->cs_change = 1;
>> +	xfer->delay.value = AD4695_T_CNVL_NS;
>> +	xfer->delay.unit = SPI_DELAY_UNIT_NSECS;
>> +	xfer->cs_change_delay.value = AD4695_T_CONVERT_NS;
>> +	xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
>> +	num_xfer = 1;
>> +
>> +	iio_for_each_active_channel(indio_dev, bit) {
>> +		xfer = &st->buf_read_xfer[num_xfer];
>> +		xfer->bits_per_word = 16;
>> +		xfer->rx_buf = &st->buf[(num_xfer - 1) * 2];
>> +		xfer->len = 2;
>> +		xfer->cs_change = 1;
>> +		xfer->cs_change_delay.value = AD4695_T_CONVERT_NS;
>> +		xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
>> +
>> +		if (bit == temp_chan_bit) {
>> +			temp_chan_en = true;
>> +		} else {
>> +			reg = AD4695_REG_AS_SLOT(num_slots);
>> +			val = FIELD_PREP(AD4695_REG_AS_SLOT_INX, bit);
>> +
>> +			ret = regmap_write(st->regmap, reg, val);
>> +			if (ret)
>> +				return ret;
>> +
>> +			num_slots++;
>> +		}
>> +
>> +		num_xfer++;
>> +	}
>> +
>> +	/*
>> +	 * Don't keep CS asserted after last xfer. Also triggers conversion of
>> +	 * slot 0.
>> +	 */
>> +	xfer->cs_change = 0;
>> +
>> +	/**
>> +	 * The advanced sequencer requires that at least 2 slots are enabled.
>> +	 * Since slot 0 is always used for other purposes, we need only 1
>> +	 * enabled voltage channel to meet this requirement. This error will
>> +	 * only happen if only the temperature channel is enabled.
>> +	 */
>> +	if (num_slots < 2) {
>> +		dev_err_ratelimited(&indio_dev->dev,
>> +			"Buffered read requires at least 1 voltage channel
>> enabled\n");
> 
> This one is intriguing... Why the ratelimited variant? Normally you'd use that in IRQ
> routines where the log could be flooded.

IIO Oscilloscope does a lot of retries of buffered reads very quickly,
so was getting a minor flood (10-20 repeats). I'm not sure that
ratelimited actually helped in this case though.

I suppose we could just drop this and expect people to read the docs
if they get an EINVAL when attempting to enable the buffer. Or just
make it dev_err() since it isn't 100s of repeats.

>> +		return -EINVAL;
>> +	}
>> +
>> +	/*
>> +	 * Temperature channel isn't included in the sequence, but rather
>> +	 * controlled by setting a bit in the TEMP_CTRL register.
>> +	 */
>> +
>> +	reg = AD4695_REG_TEMP_CTRL;
>> +	mask = AD4695_REG_TEMP_CTRL_TEMP_EN;
>> +	val = FIELD_PREP(mask, temp_chan_en ? 1 : 0);
>> +
>> +	ret = regmap_update_bits(st->regmap, reg, mask, val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	spi_message_init_with_transfers(&st->buf_read_msg, st->buf_read_xfer,
>> +					num_xfer);
>> +
>> +	ret = spi_optimize_message(st->spi, &st->buf_read_msg);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* This triggers conversion of slot 0. */
>> +	ret = ad4695_enter_advanced_sequencer_mode(st, num_slots);
>> +	if (ret) {
>> +		spi_unoptimize_message(&st->buf_read_msg);
>> +		return ret;
>> +	}
> 
> Could save one line with (unless ad4695_enter_advanced_sequencer_mode() does not
> return 0 on success)

sure

> 
> ret = ad4695_enter_advanced_sequencer_mode(st, num_slots);
> if (ret)
> 	spi_unoptimize_message(&st->buf_read_msg);
> 
> return ret;
> 
> - Nuno Sá
> 





[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux