Re: [PATCH v3 07/13] iio: adc: ad4134: Support buffered data read

[Marcelo Schmitt <marcelo.schmitt1@gmail.com>]

On 09/29, sashiko-bot@kernel.org wrote:
...
> > +static irqreturn_t ad4134_trigger_handler(int irq, void *p)
> > +{
> > +	IIO_DECLARE_BUFFER_WITH_TS(u32, channels, AD4134_NUM_CHANNELS) = { };
> > +	struct iio_poll_func *pf = p;
> > +	struct iio_dev *indio_dev = pf->indio_dev;
> > +	struct ad4134_state *st = iio_priv(indio_dev);
> > +	struct spi_transfer xfer = {
> > +		.rx_buf = st->rx_buf,
> 
> [Severity: High]
> If userspace reads or writes ADC registers via debugfs while the triggered
> buffer is running in minimum I/O mode (NO_CS), ad4134_debugfs_reg_access()
> can run concurrently with ad4134_trigger_handler().
> 
Yeah, debugfs is not safe to use like that. I believe claiming direct mode
would solve the issue, although, the conclusion a while ago was that enforcing
that for debug interface was not the way to go, IIRC. 

I'll ignore complaints about issues related to debugfs unless asked otherwise.

> [Severity: High]
> When the driver is configured in 4-wire mode (adi,spi-mode = "4-wire") and
> falls back to the software trigger handler, this flag explicitly instructs
> the SPI controller to leave the chip select line inactive (high) for the
> duration of the transfer.
> 
> Since the AD4134 requires an active (low) chip select to frame the data in
> 4-wire mode, does this setting break 4-wire mode communication and prevent
> the device from outputting valid sample data?

Doesn't apply to this specific device in 4-wire operation mode since the AD4134
data interface is read with CS off.

> 
> > +		.len = BITS_TO_BYTES(AD4134_CHAN_PRECISION_BITS),
> > +	};
> > +	int ret;
> > +
> > +	gpiod_set_value_cansleep(st->odr_gpio, 1);
> > +	fsleep(1);
> > +	gpiod_set_value_cansleep(st->odr_gpio, 0);
> > +
> > +	/* Execute transfers for all channels so the entire data frame is read. */
> > +	for (int ch = 0, i = 0; ch < AD4134_NUM_CHANNELS; ch++) {
> > +		ret = spi_sync_transfer(st->spi, &xfer, 1);
> 
> [Severity: High]
> In 4-wire mode, the AD4134 resets its data output frame if the chip select
> goes high or if the DCLK is idle for more than 250ns.

I have no idea where it supposedly found such information.

> 
> Does calling spi_sync_transfer() in a loop for each channel introduce
> software delays far exceeding 250ns and toggle the CS line between calls?
> 
> Could this violate the device's continuous clocking and CS timing
> requirements, causing the frame to reset on every iteration and reading
> Channel 0 repeatedly instead of streaming consecutive channels?
> 
> Should a single bulk SPI transfer be used here instead of a loop?

Anyways, running a longer transfer to get data all at once can indeed be done
to minimize delays. So, I may do that on a v4.