[PATCH 0/4] cec-pin/cec-gpio: improve irq handling

[PATCH 0/4] cec-pin/cec-gpio: improve irq handling

[Hans Verkuil]

The CEC interrupt is really only needed when the CEC device
is unconfigured and you want to monitor the CEC pin with
high precision.

The current code enables the irq for a short period when it
is first requested, and for a short while just before the
CEC device is configured. None of this is needed, so rework
the code to ensure it is only enabled when it is really needed.

In addition, the pin framework will now log the interrupt state
in the debugfs status file.

Hans Verkuil (4):
  cec-gpio: specify IRQF_NO_AUTOEN when requesting irq
  cec-pin: improve interrupt handling
  cec-gpio: drop the cec_gpio_free callback
  cec-pin: only enable interrupts when monitoring the CEC pin

 drivers/media/cec/core/cec-pin-priv.h         |  1 +
 drivers/media/cec/core/cec-pin.c              | 32 +++++++++++++------
 .../media/cec/platform/cec-gpio/cec-gpio.c    | 10 +-----
 3 files changed, 24 insertions(+), 19 deletions(-)

-- 
2.39.2

[PATCH 1/4] cec-gpio: specify IRQF_NO_AUTOEN when requesting irq

[Hans Verkuil]

Use IRQF_NO_AUTOEN rather than manually disabling the requested
interrupt.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/cec/platform/cec-gpio/cec-gpio.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/media/cec/platform/cec-gpio/cec-gpio.c b/drivers/media/cec/platform/cec-gpio/cec-gpio.c
index ff34490fd869..6413c0e8abcd 100644
--- a/drivers/media/cec/platform/cec-gpio/cec-gpio.c
+++ b/drivers/media/cec/platform/cec-gpio/cec-gpio.c
@@ -215,13 +215,11 @@ static int cec_gpio_probe(struct platform_device *pdev)
 		return PTR_ERR(cec->adap);
 
 	ret = devm_request_irq(dev, cec->cec_irq, cec_gpio_irq_handler,
-			       IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+			       IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_NO_AUTOEN,
 			       cec->adap->name, cec);
 	if (ret)
 		goto del_adap;
 
-	cec_gpio_disable_irq(cec->adap);
-
 	if (cec->hpd_gpio) {
 		cec->hpd_irq = gpiod_to_irq(cec->hpd_gpio);
 		ret = devm_request_threaded_irq(dev, cec->hpd_irq,
-- 
2.39.2

[PATCH 2/4] cec-pin: improve interrupt handling

[Hans Verkuil]

The CEC pin framework needs a bit more control over the interrupt
handling: make sure that the disable_irq op is called even if the
device node is unregistered, log the state of the interrupt in
debugfs, and disable the interrupt when the kernel thread is stopped.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/cec/core/cec-pin-priv.h |  1 +
 drivers/media/cec/core/cec-pin.c      | 28 +++++++++++++++++++--------
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/media/cec/core/cec-pin-priv.h b/drivers/media/cec/core/cec-pin-priv.h
index 8eb5819e6ccb..156a9f81be94 100644
--- a/drivers/media/cec/core/cec-pin-priv.h
+++ b/drivers/media/cec/core/cec-pin-priv.h
@@ -183,6 +183,7 @@ struct cec_pin {
 	u16				la_mask;
 	bool				monitor_all;
 	bool				rx_eom;
+	bool				enabled_irq;
 	bool				enable_irq_failed;
 	enum cec_pin_state		state;
 	struct cec_msg			tx_msg;
diff --git a/drivers/media/cec/core/cec-pin.c b/drivers/media/cec/core/cec-pin.c
index 68353c5dc501..8a3921fc9c99 100644
--- a/drivers/media/cec/core/cec-pin.c
+++ b/drivers/media/cec/core/cec-pin.c
@@ -1033,8 +1033,9 @@ static int cec_pin_thread_func(void *_adap)
 {
 	struct cec_adapter *adap = _adap;
 	struct cec_pin *pin = adap->pin;
-	bool irq_enabled = false;
 
+	pin->enabled_irq = false;
+	pin->enable_irq_failed = false;
 	for (;;) {
 		wait_event_interruptible(pin->kthread_waitq,
 					 kthread_should_stop() ||
@@ -1088,9 +1089,10 @@ static int cec_pin_thread_func(void *_adap)
 		switch (atomic_xchg(&pin->work_irq_change,
 				    CEC_PIN_IRQ_UNCHANGED)) {
 		case CEC_PIN_IRQ_DISABLE:
-			if (irq_enabled) {
-				call_void_pin_op(pin, disable_irq);
-				irq_enabled = false;
+			if (pin->enabled_irq) {
+				pin->ops->disable_irq(adap);
+				pin->enabled_irq = false;
+				pin->enable_irq_failed = false;
 			}
 			cec_pin_high(pin);
 			if (pin->state == CEC_ST_OFF)
@@ -1100,21 +1102,29 @@ static int cec_pin_thread_func(void *_adap)
 				      HRTIMER_MODE_REL);
 			break;
 		case CEC_PIN_IRQ_ENABLE:
-			if (irq_enabled)
+			if (pin->enabled_irq || !pin->ops->enable_irq ||
+			    pin->adap->devnode.unregistered)
 				break;
-			pin->enable_irq_failed = !call_pin_op(pin, enable_irq);
+			pin->enable_irq_failed = !pin->ops->enable_irq(adap);
 			if (pin->enable_irq_failed) {
 				cec_pin_to_idle(pin);
 				hrtimer_start(&pin->timer, ns_to_ktime(0),
 					      HRTIMER_MODE_REL);
 			} else {
-				irq_enabled = true;
+				pin->enabled_irq = true;
 			}
 			break;
 		default:
 			break;
 		}
 	}
+
+	if (pin->enabled_irq) {
+		pin->ops->disable_irq(pin->adap);
+		pin->enabled_irq = false;
+		pin->enable_irq_failed = false;
+		cec_pin_high(pin);
+	}
 	return 0;
 }
 
@@ -1215,7 +1225,9 @@ static void cec_pin_adap_status(struct cec_adapter *adap,
 	seq_printf(file, "cec pin: %d\n", call_pin_op(pin, read));
 	seq_printf(file, "cec pin events dropped: %u\n",
 		   pin->work_pin_events_dropped_cnt);
-	seq_printf(file, "irq failed: %d\n", pin->enable_irq_failed);
+	if (pin->ops->enable_irq)
+		seq_printf(file, "irq %s\n", pin->enabled_irq ? "enabled" :
+			   (pin->enable_irq_failed ? "failed" : "disabled"));
 	if (pin->timer_100us_overruns) {
 		seq_printf(file, "timer overruns > 100us: %u of %u\n",
 			   pin->timer_100us_overruns, pin->timer_cnt);
-- 
2.39.2

[PATCH 3/4] cec-gpio: drop the cec_gpio_free callback

[Hans Verkuil]

Since the CEC pin framework now keeps track of the interrupt
and calls disable_irq when the kthread stops, there is no
longer any need for the cec-gpio driver to do this in the
free callback. So drop this code.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/cec/platform/cec-gpio/cec-gpio.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/media/cec/platform/cec-gpio/cec-gpio.c b/drivers/media/cec/platform/cec-gpio/cec-gpio.c
index 6413c0e8abcd..98dacb0919b6 100644
--- a/drivers/media/cec/platform/cec-gpio/cec-gpio.c
+++ b/drivers/media/cec/platform/cec-gpio/cec-gpio.c
@@ -159,11 +159,6 @@ static int cec_gpio_read_5v(struct cec_adapter *adap)
 	return gpiod_get_value(cec->v5_gpio);
 }
 
-static void cec_gpio_free(struct cec_adapter *adap)
-{
-	cec_gpio_disable_irq(adap);
-}
-
 static const struct cec_pin_ops cec_gpio_pin_ops = {
 	.read = cec_gpio_read,
 	.low = cec_gpio_low,
@@ -171,7 +166,6 @@ static const struct cec_pin_ops cec_gpio_pin_ops = {
 	.enable_irq = cec_gpio_enable_irq,
 	.disable_irq = cec_gpio_disable_irq,
 	.status = cec_gpio_status,
-	.free = cec_gpio_free,
 	.read_hpd = cec_gpio_read_hpd,
 	.read_5v = cec_gpio_read_5v,
 };
-- 
2.39.2

[PATCH 4/4] cec-pin: only enable interrupts when monitoring the CEC pin

[Hans Verkuil]

The CEC interrupt is only needed if userspace wants to monitor
the CEC pin for an unconfigured CEC device. That gives it the
most precise CEC pin debugging results.

This avoids a corner case where the interrupt is enabled for
a short period when the adapter is about to be configured.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/cec/core/cec-pin.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/cec/core/cec-pin.c b/drivers/media/cec/core/cec-pin.c
index 8a3921fc9c99..330d5d5d86ab 100644
--- a/drivers/media/cec/core/cec-pin.c
+++ b/drivers/media/cec/core/cec-pin.c
@@ -982,7 +982,7 @@ static enum hrtimer_restart cec_pin_timer(struct hrtimer *timer)
 		}
 		if (pin->state != CEC_ST_IDLE || pin->ops->enable_irq == NULL ||
 		    pin->enable_irq_failed || adap->is_configuring ||
-		    adap->is_configured || adap->monitor_all_cnt)
+		    adap->is_configured || adap->monitor_all_cnt || !adap->monitor_pin_cnt)
 			break;
 		/* Switch to interrupt mode */
 		atomic_set(&pin->work_irq_change, CEC_PIN_IRQ_ENABLE);
@@ -1317,7 +1317,7 @@ void cec_pin_changed(struct cec_adapter *adap, bool value)
 
 	cec_pin_update(pin, value, false);
 	if (!value && (adap->is_configuring || adap->is_configured ||
-		       adap->monitor_all_cnt))
+		       adap->monitor_all_cnt || !adap->monitor_pin_cnt))
 		atomic_set(&pin->work_irq_change, CEC_PIN_IRQ_DISABLE);
 }
 EXPORT_SYMBOL_GPL(cec_pin_changed);
-- 
2.39.2