Message-ID: <20240810103540.03e758a5@jic23-huawei>
Date: Sat, 10 Aug 2024 10:35:40 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: David Lechner <dlechner@...libre.com>
Cc: Michael Hennerich <Michael.Hennerich@...log.com>, Nuno Sá
 <nuno.sa@...log.com>, Jonathan Corbet <corbet@....net>,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-doc@...r.kernel.org
Subject: Re: [PATCH 1/2] iio: adc: ad4695: implement triggered buffer

On Wed,  7 Aug 2024 15:02:10 -0500
David Lechner <dlechner@...libre.com> 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@...libre.com>

Main thing in here is I think you can use available_scan_masks
to avoid the need for the error path on just the temperature channel
being enabled.

Jonathan

> +/**
> + * ad4695_enter_advanced_sequencer_mode - Put the ADC in advanced sequencer mode
> + * @st: The driver state
> + * @n: The number of slots to use - must be >= 2, <= 128
> + *
> + * As per the datasheet, to enable advanced sequencer, we need to set
> + * STD_SEQ_EN=0, NUM_SLOTS_AS=n-1 and CYC_CTRL=0 (Table 15). Setting SPI_MODE=1
> + * triggers the first conversion using the channel in AS_SLOT0.
> + *
> + * Return: 0 on success, a negative error code on failure
> + */
> +static int ad4695_enter_advanced_sequencer_mode(struct ad4695_state *st, u32 n)
> +{
> +	u32 mask, val;
> +	int ret;
> +
> +	mask = AD4695_REG_SEQ_CTRL_STD_SEQ_EN;
> +	val = FIELD_PREP(AD4695_REG_SEQ_CTRL_STD_SEQ_EN, 0);
> +
> +	mask |= AD4695_REG_SEQ_CTRL_NUM_SLOTS_AS;
> +	val |= FIELD_PREP(AD4695_REG_SEQ_CTRL_NUM_SLOTS_AS, n - 1);
> +
> +	ret = regmap_update_bits(st->regmap, AD4695_REG_SEQ_CTRL, mask, val);
> +	if (ret)
> +		return ret;
> +
> +	mask = AD4695_REG_SETUP_SPI_MODE;
> +	val = FIELD_PREP(AD4695_REG_SETUP_SPI_MODE, 1);
> +
> +	mask |= AD4695_REG_SETUP_SPI_CYC_CTRL;
> +	val |= FIELD_PREP(AD4695_REG_SETUP_SPI_CYC_CTRL, 0);

I'd just combine the two parts of mask and val.  If it were a long
complex list then fair enough to keep them as individual parts, but
not needed for 2 items.

> +
> +	return regmap_update_bits(st->regmap, AD4695_REG_SETUP, mask, val);
> +}
> +
> +/**
> + * ad4695_exit_conversion_mode - Exit conversion mode
> + * @st: The AD4695 state
> + *
> + * Sends SPI command to exit conversion mode.
> + *
> + * Return: 0 on success, a negative error code on failure
> + */
> +static int ad4695_exit_conversion_mode(struct ad4695_state *st)
> +{
> +	struct spi_transfer xfer = { };
	struct spi_transfer xfer = {
		.tx_buf = &st->cnv_cmd2,
		.len = 1,
		.delay.value ...

	};

Might as well fill it in from the start.
Doesn't matter that the data is filled in just after this even if
that doesn't feel quite right, the code is so close I don't think
it will confuse readers and it's a common pattern.

> +
> +	st->cnv_cmd2 = AD4695_CMD_EXIT_CNV_MODE << 3;
> +	xfer.tx_buf = &st->cnv_cmd2;
> +	xfer.len = 1;
> +	xfer.delay.value = AD4695_T_REGCONFIG_NS;
> +	xfer.delay.unit = SPI_DELAY_UNIT_NSECS;
> +
> +	return spi_sync_transfer(st->spi, &xfer, 1);
> +}
> +
>  static int ad4695_set_ref_voltage(struct ad4695_state *st, int vref_mv)
>  {
>  	u8 val;
> @@ -296,6 +371,147 @@ static int ad4695_write_chn_cfg(struct ad4695_state *st,
>  				  mask, val);
>  }
>  
> +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) {

Can you use available_scanmasks to let the IIO core figure out it needs
to enable (and then hide) an extra channel?

Either that or spin up a channel to meet the requirement, just don't
capture it - Given this is an unlikely case, better to leave it to the
IIO core buffer demux handling than to bother handling locally.


> +		dev_err_ratelimited(&indio_dev->dev,
> +			"Buffered read requires at least 1 voltage channel enabled\n");
> +		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);
This is the line the bot didn't like. The local variables reg and
mask don't add anything anyway, so get rid of them and use the
values inline.

> +
> +	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;
> +	}
> +
> +	return 0;
> +}

Powered by blists - more mailing lists