Date: Wed, 26 Jun 2024 11:56:22 -0500
From: David Lechner <dlechner@...libre.com>
To: Marcelo Schmitt <marcelo.schmitt@...log.com>, broonie@...nel.org,
 lars@...afoo.de, Michael.Hennerich@...log.com, jic23@...nel.org,
 robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
 nuno.sa@...log.com, corbet@....net, marcelo.schmitt1@...il.com
Cc: linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-spi@...r.kernel.org, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 6/7] iio: adc: Add support for AD4000

On 6/25/24 4:55 PM, Marcelo Schmitt wrote:

> +
> +enum ad4000_sdi {
> +	/* datasheet calls this "4-wire mode" (controller CS goes to ADC SDI!) */

It looks like this comment was meant for AD4000_SDI_CS.

> +	AD4000_SDI_MOSI,
> +	/* datasheet calls this "3-wire mode" (not related to SPI_3WIRE!) */
> +	AD4000_SDI_VIO,
> +	AD4000_SDI_CS,
> +};
> +
> +/* maps adi,sdi-pin property value to enum */
> +static const char * const ad4000_sdi_pin[] = {
> +	[AD4000_SDI_MOSI] = "",
> +	[AD4000_SDI_VIO] = "high",
> +	[AD4000_SDI_CS] = "cs",
> +};

Should we go ahead and add "low" here too even though it isn't supported
yet? We could give a different error message in this case. (not supported
mode vs. invalid value).

> +/*
> + * This executes a data sample transfer for when the device connections are
> + * in "3-wire" mode, selected when the adi,sdi-pin device tree property is
> + * absent or set to "high". In this connection mode, the ADC SDI pin is
> + * connected to MOSI or to VIO and ADC CNV pin is connected either to a SPI
> + * controller CS or to a GPIO.
> + * AD4000 series of devices initiate conversions on the rising edge of CNV pin.
> + *
> + * If the CNV pin is connected to an SPI controller CS line (which is by default
> + * active low), the ADC readings would have a latency (delay) of one read.
> + * Moreover, since we also do ADC sampling for filling the buffer on triggered
> + * buffer mode, the timestamps of buffer readings would be disarranged.
> + * To prevent the read latency and reduce the time discrepancy between the
> + * sample read request and the time of actual sampling by the ADC, do a
> + * preparatory transfer to pulse the CS/CNV line.

This description doesn't sound quite correct. When st->turbo_mode is true
the shorter delay will cause a read during conversion, so we would be 
reading the sample from the previous conversion trigger, not the current one.

The description sounds like this function always does a read during
aquisition. So if that is the actual intent (and I agree it should be),
maybe the best thing to do would be to just remove st->turbo_mode for
now? Then we can add it back when we do SPI offload support that actually
needs it to achieve max sample rate. Then the function will match the
description as-is.

st->turbo_mode is never set to true currently anyway. So removing it
for now seems best.

> + */
> +static int ad4000_prepare_3wire_mode_message(struct ad4000_state *st,
> +					     const struct iio_chan_spec *chan)
> +{
> +	unsigned int cnv_pulse_time = st->turbo_mode ? AD4000_TQUIET1_NS
> +						     : AD4000_TCONV_NS;
> +	struct spi_transfer *xfers = st->xfers;
> +
> +	xfers[0].cs_change = 1;
> +	xfers[0].cs_change_delay.value = cnv_pulse_time;
> +	xfers[0].cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
> +
> +	xfers[1].rx_buf = &st->scan.data;
> +	xfers[1].len = BITS_TO_BYTES(chan->scan_type.storagebits);
> +	xfers[1].delay.value = AD4000_TQUIET2_NS;
> +	xfers[1].delay.unit = SPI_DELAY_UNIT_NSECS;
> +
> +	spi_message_init_with_transfers(&st->msg, st->xfers, 2);
> +
> +	return devm_spi_optimize_message(st->spi, &st->msg);

In the cover letter or after --- in this patch we should mention the
dependency since this is a new API and depends on the tag from Mark.

> +}
> +

Powered by blists - more mailing lists