Re: [PATCH 35/40] iio: adc: ti_am335x_adc: Wait the idle state to avoid stalls

Jonathan Cameron
2021-08-30T14:19:17Z

On Wed, 25 Aug 2021 17:25:13 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> A simple:
> $ cat /sys/bus/iio/devices/iio\:deviceX/in_voltage*_raw
> can stall forever. It seems that it comes from the fact that the
> internal state machine does not have enough time to return to its idle
> state in this situation before receiving another request, leading to an
> internal stall.

Interesting and non obvious... Good work figuring that out.

>
> Add a tiadc_wait_idle() helper to ensure no new conversion is requested
> while the FSM is still busy.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
> drivers/iio/adc/ti_am335x_adc.c | 24 ++++++++++++++++++++++--
> 1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index ebf6326af60d..d4619fd6fd54 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -17,6 +17,7 @@
> #include <linux/of_device.h>
> #include <linux/iio/machine.h>
> #include <linux/iio/driver.h>
> +#include <linux/iopoll.h>
>
> #include <linux/mfd/ti_am335x_tscadc.h>
> #include <linux/iio/buffer.h>
> @@ -94,6 +95,15 @@ static u32 get_adc_step_bit(struct tiadc_device *adc_dev, int chan)
> return 1 << adc_dev->channel_step[chan];
> }
>
> +static int tiadc_wait_idle(struct tiadc_device *adc_dev)
> +{
> + u32 val;
> +
> + return readl_poll_timeout(adc_dev->mfd_tscadc->tscadc_base + REG_ADCFSM,
> + val, !(val & SEQ_STATUS), 10,
> + IDLE_TIMEOUT_MS * 1000 * adc_dev->channels);
> +}
> +
> static void tiadc_step_config(struct iio_dev *indio_dev)
> {
> struct tiadc_device *adc_dev = iio_priv(indio_dev);
> @@ -268,6 +278,11 @@ static int tiadc_buffer_preenable(struct iio_dev *indio_dev)
> {
> struct tiadc_device *adc_dev = iio_priv(indio_dev);
> int i, fifo1count;
> + int ret;
> +
> + ret = tiadc_wait_idle(adc_dev);
> + if (ret)
> + return ret;
>
> tiadc_writel(adc_dev, REG_IRQCLR,
> IRQENB_FIFO1THRES | IRQENB_FIFO1OVRRUN |
> @@ -419,12 +434,12 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
> long mask)
> {
> struct tiadc_device *adc_dev = iio_priv(indio_dev);
> - int ret = IIO_VAL_INT;
> int i, map_val;
> unsigned int fifo1count, read, stepid;
> bool found = false;
> u32 step_en;
> unsigned long timeout;
> + int ret;
>
> if (iio_buffer_enabled(indio_dev))
> return -EBUSY;
> @@ -434,6 +449,11 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
> return -EINVAL;
>
> mutex_lock(&adc_dev->fifo1_lock);
> +
> + ret = tiadc_wait_idle(adc_dev);
> + if (ret)
> + goto err_unlock;
> +
> fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> while (fifo1count--)
> tiadc_readl(adc_dev, REG_FIFO1);
> @@ -482,7 +502,7 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
>
> err_unlock:
> mutex_unlock(&adc_dev->fifo1_lock);
> - return ret;
> + return ret ? ret : IIO_VAL_INT;
> }
>
> static const struct iio_info tiadc_info = {