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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8bb01a8946aaa5855b5ac15d79c0292a668eee59.camel@gmail.com>
Date: Fri, 09 Aug 2024 16:24:00 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: David Lechner <dlechner@...libre.com>, Jonathan Cameron
 <jic23@...nel.org>
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, 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@...libre.com>
> ---

Hi David,

Just two nit comments...

Reviewed-by: Nuno Sa <nuno.sa@...log.com>

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

ret = ad4695_enter_advanced_sequencer_mode(st, num_slots);
if (ret)
	spi_unoptimize_message(&st->buf_read_msg);

return ret;

- Nuno Sá