iio: adc: tsc2046: rework the trigger state machine

Initially this was designed to:
| Fix sleeping in atomic context warning and a deadlock after iio_trigger_poll()
| call
|
| If iio_trigger_poll() is called after IRQ was disabled, we will call
| reenable_trigger() directly from hard IRQ or hrtimer context instead of
| IRQ thread. In this case we will run in to multiple issue as sleeping in atomic
| context and a deadlock.
|
| To avoid this issue, rework the trigger to use state machine. All state
| changes are done over the hrtimer, so it allows us to drop fsleep() and
| avoid the deadlock.

Since this issue was fixed by: 9020ef659885 ("iio: trigger: Fix a scheduling
whilst atomic issue seen on tsc2046"). This patch is a cleanup to make
state machine easier to follow.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Link: https://lore.kernel.org/r/20220111130402.3404769-1-o.rempel@pengutronix.de
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

diff --git a/drivers/iio/adc/ti-tsc2046.c b/drivers/iio/adc/ti-tsc2046.c
index d84ae6b008c1b024a55d4f1bf670e73ec20cd232..098dbca49176d34722809727491877b5a1112ac8 100644 (file)
--- a/drivers/iio/adc/ti-tsc2046.c
+++ b/drivers/iio/adc/ti-tsc2046.c
@@ -82,6 +82,10 @@
 #define TI_TSC2046_DATA_12BIT                  GENMASK(14, 3)
 
 #define TI_TSC2046_MAX_CHAN                    8
+#define TI_TSC2046_MIN_POLL_CNT                        3
+#define TI_TSC2046_EXT_POLL_CNT                        3
+#define TI_TSC2046_POLL_CNT \
+       (TI_TSC2046_MIN_POLL_CNT + TI_TSC2046_EXT_POLL_CNT)
 
 /* Represents a HW sample */
 struct tsc2046_adc_atom {
@@ -123,14 +127,23 @@ struct tsc2046_adc_ch_cfg {
        unsigned int oversampling_ratio;
 };
 
+enum tsc2046_state {
+       TSC2046_STATE_SHUTDOWN,
+       TSC2046_STATE_STANDBY,
+       TSC2046_STATE_POLL,
+       TSC2046_STATE_POLL_IRQ_DISABLE,
+       TSC2046_STATE_ENABLE_IRQ,
+};
+
 struct tsc2046_adc_priv {
        struct spi_device *spi;
        const struct tsc2046_adc_dcfg *dcfg;
 
        struct iio_trigger *trig;
        struct hrtimer trig_timer;
-       spinlock_t trig_lock;
-       unsigned int trig_more_count;
+       enum tsc2046_state state;
+       int poll_cnt;
+       spinlock_t state_lock;
 
        struct spi_transfer xfer;
        struct spi_message msg;
@@ -411,21 +424,63 @@ static const struct iio_info tsc2046_adc_info = {
        .update_scan_mode = tsc2046_adc_update_scan_mode,
 };
 
-static enum hrtimer_restart tsc2046_adc_trig_more(struct hrtimer *hrtimer)
+static enum hrtimer_restart tsc2046_adc_timer(struct hrtimer *hrtimer)
 {
        struct tsc2046_adc_priv *priv = container_of(hrtimer,
                                                     struct tsc2046_adc_priv,
                                                     trig_timer);
        unsigned long flags;
 
-       spin_lock_irqsave(&priv->trig_lock, flags);
-
-       disable_irq_nosync(priv->spi->irq);
-
-       priv->trig_more_count++;
-       iio_trigger_poll(priv->trig);
+       /*
+        * This state machine should address following challenges :
+        * - the interrupt source is based on level shifter attached to the X
+        *   channel of ADC. It will change the state every time we switch
+        *   between channels. So, we need to disable IRQ if we do
+        *   iio_trigger_poll().
+        * - we should do iio_trigger_poll() at some reduced sample rate
+        * - we should still trigger for some amount of time after last
+        *   interrupt with enabled IRQ was processed.
+        */
 
-       spin_unlock_irqrestore(&priv->trig_lock, flags);
+       spin_lock_irqsave(&priv->state_lock, flags);
+       switch (priv->state) {
+       case TSC2046_STATE_ENABLE_IRQ:
+               if (priv->poll_cnt < TI_TSC2046_POLL_CNT) {
+                       priv->poll_cnt++;
+                       hrtimer_start(&priv->trig_timer,
+                                     ns_to_ktime(priv->scan_interval_us *
+                                                 NSEC_PER_USEC),
+                                     HRTIMER_MODE_REL_SOFT);
+
+                       if (priv->poll_cnt >= TI_TSC2046_MIN_POLL_CNT) {
+                               priv->state = TSC2046_STATE_POLL_IRQ_DISABLE;
+                               enable_irq(priv->spi->irq);
+                       } else {
+                               priv->state = TSC2046_STATE_POLL;
+                       }
+               } else {
+                       priv->state = TSC2046_STATE_STANDBY;
+                       enable_irq(priv->spi->irq);
+               }
+               break;
+       case TSC2046_STATE_POLL_IRQ_DISABLE:
+               disable_irq_nosync(priv->spi->irq);
+               fallthrough;
+       case TSC2046_STATE_POLL:
+               priv->state = TSC2046_STATE_ENABLE_IRQ;
+               /* iio_trigger_poll() starts hrtimer */
+               iio_trigger_poll(priv->trig);
+               break;
+       case TSC2046_STATE_SHUTDOWN:
+               break;
+       case TSC2046_STATE_STANDBY:
+               fallthrough;
+       default:
+               dev_warn(&priv->spi->dev, "Got unexpected state: %i\n",
+                        priv->state);
+               break;
+       }
+       spin_unlock_irqrestore(&priv->state_lock, flags);
 
        return HRTIMER_NORESTART;
 }
@@ -434,16 +489,20 @@ static irqreturn_t tsc2046_adc_irq(int irq, void *dev_id)
 {
        struct iio_dev *indio_dev = dev_id;
        struct tsc2046_adc_priv *priv = iio_priv(indio_dev);
-
-       spin_lock(&priv->trig_lock);
+       unsigned long flags;
 
        hrtimer_try_to_cancel(&priv->trig_timer);
 
-       priv->trig_more_count = 0;
-       disable_irq_nosync(priv->spi->irq);
-       iio_trigger_poll(priv->trig);
+       spin_lock_irqsave(&priv->state_lock, flags);
+       if (priv->state != TSC2046_STATE_SHUTDOWN) {
+               priv->state = TSC2046_STATE_ENABLE_IRQ;
+               priv->poll_cnt = 0;
 
-       spin_unlock(&priv->trig_lock);
+               /* iio_trigger_poll() starts hrtimer */
+               disable_irq_nosync(priv->spi->irq);
+               iio_trigger_poll(priv->trig);
+       }
+       spin_unlock_irqrestore(&priv->state_lock, flags);
 
        return IRQ_HANDLED;
 }
@@ -452,49 +511,42 @@ static void tsc2046_adc_reenable_trigger(struct iio_trigger *trig)
 {
        struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
        struct tsc2046_adc_priv *priv = iio_priv(indio_dev);
-       unsigned long flags;
-       int delta;
+       ktime_t tim;
 
        /*
         * We can sample it as fast as we can, but usually we do not need so
         * many samples. Reduce the sample rate for default (touchscreen) use
         * case.
-        * Currently we do not need a highly precise sample rate. It is enough
-        * to have calculated numbers.
         */
-       delta = priv->scan_interval_us - priv->time_per_scan_us;
-       if (delta > 0)
-               fsleep(delta);
-
-       spin_lock_irqsave(&priv->trig_lock, flags);
-
-       /*
-        * We need to trigger at least one extra sample to detect state
-        * difference on ADC side.
-        */
-       if (!priv->trig_more_count) {
-               int timeout_ms = DIV_ROUND_UP(priv->scan_interval_us,
-                                             USEC_PER_MSEC);
-
-               hrtimer_start(&priv->trig_timer, ms_to_ktime(timeout_ms),
-                             HRTIMER_MODE_REL_SOFT);
-       }
-
-       enable_irq(priv->spi->irq);
-
-       spin_unlock_irqrestore(&priv->trig_lock, flags);
+       tim = ns_to_ktime((priv->scan_interval_us - priv->time_per_scan_us) *
+                         NSEC_PER_USEC);
+       hrtimer_start(&priv->trig_timer, tim, HRTIMER_MODE_REL_SOFT);
 }
 
 static int tsc2046_adc_set_trigger_state(struct iio_trigger *trig, bool enable)
 {
        struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
        struct tsc2046_adc_priv *priv = iio_priv(indio_dev);
+       unsigned long flags;
 
        if (enable) {
-               enable_irq(priv->spi->irq);
+               spin_lock_irqsave(&priv->state_lock, flags);
+               if (priv->state == TSC2046_STATE_SHUTDOWN) {
+                       priv->state = TSC2046_STATE_STANDBY;
+                       enable_irq(priv->spi->irq);
+               }
+               spin_unlock_irqrestore(&priv->state_lock, flags);
        } else {
-               disable_irq(priv->spi->irq);
-               hrtimer_try_to_cancel(&priv->trig_timer);
+               spin_lock_irqsave(&priv->state_lock, flags);
+
+               if (priv->state == TSC2046_STATE_STANDBY ||
+                   priv->state == TSC2046_STATE_POLL_IRQ_DISABLE)
+                       disable_irq_nosync(priv->spi->irq);
+
+               priv->state = TSC2046_STATE_SHUTDOWN;
+               spin_unlock_irqrestore(&priv->state_lock, flags);
+
+               hrtimer_cancel(&priv->trig_timer);
        }
 
        return 0;
@@ -668,10 +720,11 @@ static int tsc2046_adc_probe(struct spi_device *spi)
        iio_trigger_set_drvdata(trig, indio_dev);
        trig->ops = &tsc2046_adc_trigger_ops;
 
-       spin_lock_init(&priv->trig_lock);
+       spin_lock_init(&priv->state_lock);
+       priv->state = TSC2046_STATE_SHUTDOWN;
        hrtimer_init(&priv->trig_timer, CLOCK_MONOTONIC,
                     HRTIMER_MODE_REL_SOFT);
-       priv->trig_timer.function = tsc2046_adc_trig_more;
+       priv->trig_timer.function = tsc2046_adc_timer;
 
        ret = devm_iio_trigger_register(dev, trig);
        if (ret) {