Re: [PATCH v4 08/10] iio: adc: ad_sigma_delta: Check for previous ready signals

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 27 Nov 2024 15:59:37 +0100
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> It can happen if a previous conversion was aborted the ADC pulls down
> the ̅R̅D̅Y line but the event wasn't handled before. In that case enabling
> the irq might immediately fire (depending on the irq controller's
> capabilities) and even with a rdy-gpio isn't identified as an unrelated
> one.
> 
> To cure that problem check for a pending event before the measurement is
> started and clear it if needed.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
Hi Uwe,

I think there is a DMA buffer safety issue hiding in the new code. Other than
that this looks good to me.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/ad_sigma_delta.c | 89 ++++++++++++++++++++++++++++++++
>  1 file changed, 89 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
> index 9891346c2d73..164a4d0b571d 100644
> --- a/drivers/iio/adc/ad_sigma_delta.c
> +++ b/drivers/iio/adc/ad_sigma_delta.c
> @@ -29,8 +29,11 @@
>  #define AD_SD_COMM_CHAN_MASK	0x3
>  
>  #define AD_SD_REG_COMM		0x00
> +#define AD_SD_REG_STATUS	0x00
>  #define AD_SD_REG_DATA		0x03
>  
> +#define AD_SD_REG_STATUS_RDY	0x80
> +
>  /**
>   * ad_sd_set_comm() - Set communications register
>   *
> @@ -222,6 +225,80 @@ static void ad_sd_enable_irq(struct ad_sigma_delta *sigma_delta)
>  	enable_irq(sigma_delta->irq_line);
>  }
>  
> +/* Called with `sigma_delta->bus_locked == true` only.  */

Trivial but bonus space before */

> +static int ad_sigma_delta_clear_pending_event(struct ad_sigma_delta *sigma_delta)
> +{
> +	bool pending_event;
> +	unsigned int data_read_len = BITS_TO_BYTES(sigma_delta->info->num_resetclks);
> +	u8 data[9];

SPI requires DMA safe buffers.  Either play games to get one in iio_priv via
__aligned(IIO_DMA_MINALIGN) or just kzalloc() this here.
Probably do that after the !pending_event check though so we only allocate
when there is a need for it.


> +	struct spi_transfer t[] = {
> +		{
> +			.tx_buf = data,
> +			.len = 1,
> +		}, {
> +			.tx_buf = data + 1,
> +			.len = data_read_len,
> +		}
> +	};
> +	struct spi_message m;
> +
> +	/*
> +	 * read RDY pin (if possible) or status register to check if there is an

* Read RDY...

> +	 * old event.
> +	 */
> +	if (sigma_delta->rdy_gpiod) {
> +		pending_event = gpiod_get_value(sigma_delta->rdy_gpiod);
> +	} else {
> +		int ret;
> +		unsigned status_reg;
> +
> +		ret = ad_sd_read_reg(sigma_delta, AD_SD_REG_STATUS, 1, &status_reg);
> +		if (ret)
> +			return ret;
> +
> +		pending_event = !(status_reg & AD_SD_REG_STATUS_RDY);
> +	}
> +
> +	if (!pending_event)
> +		return 0;
> +
> +	/*
> +	 * In general the size of the data register is unknown. It varies from
> +	 * device to device, might be one byte longer if CONTROL.DATA_STATUS is
> +	 * set and even varies on some devices depending on which input is
> +	 * selected. So send one byte to start reading the data register and
> +	 * then just clock for some bytes with DIN (aka MOSI) high to not
> +	 * confuse the register access state machine after the data register was
> +	 * completely read. Note however that the sequence length must be
> +	 * shorter than the reset procedure.
> +	 */
> +
> +	/* Oh, back out instead of overflowing data[] */
> +	if (data_read_len > sizeof(data) - 1)
> +		return -EINVAL;
> +
> +	spi_message_init(&m);
> +	if (sigma_delta->info->has_registers) {
> +		unsigned int data_reg = sigma_delta->info->data_reg ?: AD_SD_REG_DATA;
> +
> +		data[0] = data_reg << sigma_delta->info->addr_shift;
> +		data[0] |= sigma_delta->info->read_mask;
> +		data[0] |= sigma_delta->comm;
> +		spi_message_add_tail(&t[0], &m);
> +	}
> +
> +	/*
> +	 * The first transferred byte is part of the real data register,
> +	 * so this doesn't need to be 0xff. In the remaining
> +	 * `data_read_len - 1` bytes are less than $num_resetclks ones.
> +	 */
> +	data[1] = 0x00;
> +	memset(data + 2, 0xff, data_read_len - 1);
> +	spi_message_add_tail(&t[1], &m);
> +
> +	return spi_sync_locked(sigma_delta->spi, &m);
> +}
> +
>  int ad_sd_calibrate(struct ad_sigma_delta *sigma_delta,
>  	unsigned int mode, unsigned int channel)
>  {
> @@ -237,6 +314,10 @@ int ad_sd_calibrate(struct ad_sigma_delta *sigma_delta,
>  	sigma_delta->keep_cs_asserted = true;
>  	reinit_completion(&sigma_delta->completion);
>  
> +	ret = ad_sigma_delta_clear_pending_event(sigma_delta);
> +	if (ret)
> +		return ret;
> +
>  	ret = ad_sigma_delta_set_mode(sigma_delta, mode);
>  	if (ret < 0)
>  		goto out;
> @@ -310,6 +391,10 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
>  	sigma_delta->keep_cs_asserted = true;
>  	reinit_completion(&sigma_delta->completion);
>  
> +	ret = ad_sigma_delta_clear_pending_event(sigma_delta);
> +	if (ret)
> +		return ret;
> +
>  	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_SINGLE);
>  
>  	ad_sd_enable_irq(sigma_delta);
> @@ -406,6 +491,10 @@ static int ad_sd_buffer_postenable(struct iio_dev *indio_dev)
>  	sigma_delta->bus_locked = true;
>  	sigma_delta->keep_cs_asserted = true;
>  
> +	ret = ad_sigma_delta_clear_pending_event(sigma_delta);
> +	if (ret)
> +		return ret;
> +
>  	ret = ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_CONTINUOUS);
>  	if (ret)
>  		goto err_unlock;