Message-ID: <Zn3281oDlhIvYrwy@debian-BULLSEYE-live-builder-AMD64>
Date: Thu, 27 Jun 2024 20:34:11 -0300
From: Marcelo Schmitt <marcelo.schmitt1@...il.com>
To: David Lechner <dlechner@...libre.com>
Cc: 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,
	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 06/26, David Lechner wrote:
> 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.
> 

Yes, but I'm thinking maybe this is not the best place to have a comment about
that at all. I'm removing this comment for v6. Maybe better to have it well
documented in dt and close to the transfer functions than having partial
explanation here.

> > +	AD4000_SDI_MOSI,
> > +	/* datasheet calls this "3-wire mode" (not related to SPI_3WIRE!) */
removing this comment too
> > +	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).
> 
Okay.
I have added for v6:
	case AD4000_SDI_GND:
		return dev_err_probe(dev, -EPROTONOSUPPORT,
				     "Unsupported connection mode\n");

> > +/*
> > + * 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.

If I'm correctly understanding the datasheet diagrams for 3-wire mode,
we should be reading the conversion triggered by the rising CS edge of the
preparatory/dummy transfer (that should happen when the dummy transfer finishes). 
So this is actually doing two samples. One sample gets triggered when the
dummy transfer ends and the other one is triggered when xfers[1] completes (this
second data sample is wasted because we are doing the extra dummy transfer to
avoid having data that might have been sampled long ago and to keep timestamps
close to the actual sampling time). 

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

I always thought we should not add to the kernel code that does nothing, even if
it might do something in the future. For example, when adding new drivers, I
think it is preferred only to add defines for registers that are used, in
contrast to adding defines for all registers a chip has. So, yeah, removed
turbo_mode for v6 and also some reg defines I noticed were unused too.

> 
> > + */
> > +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.

I got
d4a0055fdc22381fa256e345095e88d134e354c5 "spi: add devm_spi_optimize_message() helper"
7e74a45c7afdd8a9f82d14fd79ae0383bbaaed1e "spi: add EXPORT_SYMBOL_GPL(devm_spi_optimize_message)"
6ecdb0aa4dca62d236a659426e11e6cf302e8f18 "spi: axi-spi-engine: Add SPI_CS_HIGH support"
from https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git/log/?h=for-6.11
to apply and test the changes for v6.
Will mention those in the cover letter.

Thanks,
Marcelo

Powered by blists - more mailing lists