On Wed, 27 Nov 2024 15:59:37 +0100
Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
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;
