* [PATCH 1/3] media: rc: nuvoton: Tweak the interrupt enabling dance
@ 2018-05-21 14:38 Michał Winiarski
2018-05-21 14:38 ` [PATCH 2/3] media: rc: nuvoton: Keep track of users on CIR enable/disable Michał Winiarski
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Michał Winiarski @ 2018-05-21 14:38 UTC (permalink / raw)
To: linux-media; +Cc: Michał Winiarski, Jarod Wilson, Sean Young
It appears that we need to enable CIR device before attempting to touch
some of the registers. Previously, this was not a big issue, since we
were rarely seeing nvt_close() getting called.
Unfortunately, since:
cb84343fced1 ("media: lirc: do not call close() or open() on unregistered devices")
The initial open() during probe from rc_setup_rx_device() is no longer
successful, which means that userspace clients will actually end up
calling nvt_open()/nvt_close().
And since nvt_open() is broken, the device doesn't seem to work as
expected.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=199597
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Jarod Wilson <jarod@redhat.com>
Cc: Sean Young <sean@mess.org>
---
drivers/media/rc/nuvoton-cir.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/media/rc/nuvoton-cir.c b/drivers/media/rc/nuvoton-cir.c
index 5e1d866a61a5..ce8949b6549d 100644
--- a/drivers/media/rc/nuvoton-cir.c
+++ b/drivers/media/rc/nuvoton-cir.c
@@ -922,6 +922,9 @@ static int nvt_open(struct rc_dev *dev)
struct nvt_dev *nvt = dev->priv;
unsigned long flags;
+ /* enable the CIR logical device */
+ nvt_enable_logical_dev(nvt, LOGICAL_DEV_CIR);
+
spin_lock_irqsave(&nvt->lock, flags);
/* set function enable flags */
@@ -937,9 +940,6 @@ static int nvt_open(struct rc_dev *dev)
spin_unlock_irqrestore(&nvt->lock, flags);
- /* enable the CIR logical device */
- nvt_enable_logical_dev(nvt, LOGICAL_DEV_CIR);
-
return 0;
}
--
2.17.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] media: rc: nuvoton: Keep track of users on CIR enable/disable
2018-05-21 14:38 [PATCH 1/3] media: rc: nuvoton: Tweak the interrupt enabling dance Michał Winiarski
@ 2018-05-21 14:38 ` Michał Winiarski
2018-05-21 14:38 ` [PATCH 3/3] media: rc: nuvoton: Keep device enabled during reg init Michał Winiarski
2018-05-21 15:54 ` [PATCH 1/3] media: rc: nuvoton: Tweak the interrupt enabling dance Sean Young
2 siblings, 0 replies; 11+ messages in thread
From: Michał Winiarski @ 2018-05-21 14:38 UTC (permalink / raw)
To: linux-media; +Cc: Michał Winiarski, Jarod Wilson, Sean Young
Core rc keeps track of the users - let's use it to tweak the code and
use the common code path on suspend/resume.
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Jarod Wilson <jarod@redhat.com>
Cc: Sean Young <sean@mess.org>
---
drivers/media/rc/nuvoton-cir.c | 82 +++++++++++++++-------------------
1 file changed, 36 insertions(+), 46 deletions(-)
diff --git a/drivers/media/rc/nuvoton-cir.c b/drivers/media/rc/nuvoton-cir.c
index ce8949b6549d..eebd6fef5602 100644
--- a/drivers/media/rc/nuvoton-cir.c
+++ b/drivers/media/rc/nuvoton-cir.c
@@ -543,27 +543,9 @@ static void nvt_cir_regs_init(struct nvt_dev *nvt)
nvt_cir_reg_write(nvt, CIR_FIFOCON_TX_TRIGGER_LEV |
CIR_FIFOCON_RX_TRIGGER_LEV, CIR_FIFOCON);
- /*
- * Enable TX and RX, specify carrier on = low, off = high, and set
- * sample period (currently 50us)
- */
- nvt_cir_reg_write(nvt,
- CIR_IRCON_TXEN | CIR_IRCON_RXEN |
- CIR_IRCON_RXINV | CIR_IRCON_SAMPLE_PERIOD_SEL,
- CIR_IRCON);
-
/* clear hardware rx and tx fifos */
nvt_clear_cir_fifo(nvt);
nvt_clear_tx_fifo(nvt);
-
- /* clear any and all stray interrupts */
- nvt_cir_reg_write(nvt, 0xff, CIR_IRSTS);
-
- /* and finally, enable interrupts */
- nvt_set_cir_iren(nvt);
-
- /* enable the CIR logical device */
- nvt_enable_logical_dev(nvt, LOGICAL_DEV_CIR);
}
static void nvt_cir_wake_regs_init(struct nvt_dev *nvt)
@@ -892,6 +874,32 @@ static irqreturn_t nvt_cir_isr(int irq, void *data)
return IRQ_HANDLED;
}
+static void nvt_enable_cir(struct nvt_dev *nvt)
+{
+ unsigned long flags;
+
+ /* enable the CIR logical device */
+ nvt_enable_logical_dev(nvt, LOGICAL_DEV_CIR);
+
+ spin_lock_irqsave(&nvt->lock, flags);
+
+ /*
+ * Enable TX and RX, specify carrier on = low, off = high, and set
+ * sample period (currently 50us)
+ */
+ nvt_cir_reg_write(nvt, CIR_IRCON_TXEN | CIR_IRCON_RXEN |
+ CIR_IRCON_RXINV | CIR_IRCON_SAMPLE_PERIOD_SEL,
+ CIR_IRCON);
+
+ /* clear all pending interrupts */
+ nvt_cir_reg_write(nvt, 0xff, CIR_IRSTS);
+
+ /* enable interrupts */
+ nvt_set_cir_iren(nvt);
+
+ spin_unlock_irqrestore(&nvt->lock, flags);
+}
+
static void nvt_disable_cir(struct nvt_dev *nvt)
{
unsigned long flags;
@@ -920,25 +928,8 @@ static void nvt_disable_cir(struct nvt_dev *nvt)
static int nvt_open(struct rc_dev *dev)
{
struct nvt_dev *nvt = dev->priv;
- unsigned long flags;
- /* enable the CIR logical device */
- nvt_enable_logical_dev(nvt, LOGICAL_DEV_CIR);
-
- spin_lock_irqsave(&nvt->lock, flags);
-
- /* set function enable flags */
- nvt_cir_reg_write(nvt, CIR_IRCON_TXEN | CIR_IRCON_RXEN |
- CIR_IRCON_RXINV | CIR_IRCON_SAMPLE_PERIOD_SEL,
- CIR_IRCON);
-
- /* clear all pending interrupts */
- nvt_cir_reg_write(nvt, 0xff, CIR_IRSTS);
-
- /* enable interrupts */
- nvt_set_cir_iren(nvt);
-
- spin_unlock_irqrestore(&nvt->lock, flags);
+ nvt_enable_cir(nvt);
return 0;
}
@@ -1093,19 +1084,13 @@ static void nvt_remove(struct pnp_dev *pdev)
static int nvt_suspend(struct pnp_dev *pdev, pm_message_t state)
{
struct nvt_dev *nvt = pnp_get_drvdata(pdev);
- unsigned long flags;
nvt_dbg("%s called", __func__);
- spin_lock_irqsave(&nvt->lock, flags);
-
- /* disable all CIR interrupts */
- nvt_cir_reg_write(nvt, 0, CIR_IREN);
-
- spin_unlock_irqrestore(&nvt->lock, flags);
-
- /* disable cir logical dev */
- nvt_disable_logical_dev(nvt, LOGICAL_DEV_CIR);
+ mutex_lock(&nvt->rdev->lock);
+ if (nvt->rdev->users)
+ nvt_disable_cir(nvt);
+ mutex_unlock(&nvt->rdev->lock);
/* make sure wake is enabled */
nvt_enable_wake(nvt);
@@ -1122,6 +1107,11 @@ static int nvt_resume(struct pnp_dev *pdev)
nvt_cir_regs_init(nvt);
nvt_cir_wake_regs_init(nvt);
+ mutex_lock(&nvt->rdev->lock);
+ if (nvt->rdev->users)
+ nvt_enable_cir(nvt);
+ mutex_unlock(&nvt->rdev->lock);
+
return 0;
}
--
2.17.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] media: rc: nuvoton: Keep device enabled during reg init
2018-05-21 14:38 [PATCH 1/3] media: rc: nuvoton: Tweak the interrupt enabling dance Michał Winiarski
2018-05-21 14:38 ` [PATCH 2/3] media: rc: nuvoton: Keep track of users on CIR enable/disable Michał Winiarski
@ 2018-05-21 14:38 ` Michał Winiarski
2018-05-24 11:31 ` Sean Young
2018-05-25 14:28 ` [PATCH v2 " Michał Winiarski
2018-05-21 15:54 ` [PATCH 1/3] media: rc: nuvoton: Tweak the interrupt enabling dance Sean Young
2 siblings, 2 replies; 11+ messages in thread
From: Michał Winiarski @ 2018-05-21 14:38 UTC (permalink / raw)
To: linux-media; +Cc: Michał Winiarski, Jarod Wilson, Sean Young
Doing writes when the device is disabled seems to be a NOOP.
Let's enable the device, write the values, and then disable it on init.
This changes the behavior for wake device, which is now being disabled
after init.
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Jarod Wilson <jarod@redhat.com>
Cc: Sean Young <sean@mess.org>
---
drivers/media/rc/nuvoton-cir.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/media/rc/nuvoton-cir.c b/drivers/media/rc/nuvoton-cir.c
index eebd6fef5602..61b68cde35f1 100644
--- a/drivers/media/rc/nuvoton-cir.c
+++ b/drivers/media/rc/nuvoton-cir.c
@@ -535,6 +535,8 @@ static void nvt_set_cir_iren(struct nvt_dev *nvt)
static void nvt_cir_regs_init(struct nvt_dev *nvt)
{
+ nvt_enable_logical_dev(nvt, LOGICAL_DEV_CIR);
+
/* set sample limit count (PE interrupt raised when reached) */
nvt_cir_reg_write(nvt, CIR_RX_LIMIT_COUNT >> 8, CIR_SLCH);
nvt_cir_reg_write(nvt, CIR_RX_LIMIT_COUNT & 0xff, CIR_SLCL);
@@ -546,10 +548,14 @@ static void nvt_cir_regs_init(struct nvt_dev *nvt)
/* clear hardware rx and tx fifos */
nvt_clear_cir_fifo(nvt);
nvt_clear_tx_fifo(nvt);
+
+ nvt_disable_logical_dev(nvt, LOGICAL_DEV_CIR);
}
static void nvt_cir_wake_regs_init(struct nvt_dev *nvt)
{
+ nvt_enable_logical_dev(nvt, LOGICAL_DEV_CIR_WAKE);
+
/*
* Disable RX, set specific carrier on = low, off = high,
* and sample period (currently 50us)
@@ -562,8 +568,7 @@ static void nvt_cir_wake_regs_init(struct nvt_dev *nvt)
/* clear any and all stray interrupts */
nvt_cir_wake_reg_write(nvt, 0xff, CIR_WAKE_IRSTS);
- /* enable the CIR WAKE logical device */
- nvt_enable_logical_dev(nvt, LOGICAL_DEV_CIR_WAKE);
+ nvt_disable_logical_dev(nvt, LOGICAL_DEV_CIR);
}
static void nvt_enable_wake(struct nvt_dev *nvt)
--
2.17.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] media: rc: nuvoton: Tweak the interrupt enabling dance
2018-05-21 14:38 [PATCH 1/3] media: rc: nuvoton: Tweak the interrupt enabling dance Michał Winiarski
2018-05-21 14:38 ` [PATCH 2/3] media: rc: nuvoton: Keep track of users on CIR enable/disable Michał Winiarski
2018-05-21 14:38 ` [PATCH 3/3] media: rc: nuvoton: Keep device enabled during reg init Michał Winiarski
@ 2018-05-21 15:54 ` Sean Young
2018-05-21 16:22 ` Michał Winiarski
2 siblings, 1 reply; 11+ messages in thread
From: Sean Young @ 2018-05-21 15:54 UTC (permalink / raw)
To: Michał Winiarski; +Cc: linux-media, Jarod Wilson
On Mon, May 21, 2018 at 04:38:01PM +0200, Michał Winiarski wrote:
> It appears that we need to enable CIR device before attempting to touch
> some of the registers. Previously, this was not a big issue, since we
> were rarely seeing nvt_close() getting called.
>
> Unfortunately, since:
> cb84343fced1 ("media: lirc: do not call close() or open() on unregistered devices")
>
> The initial open() during probe from rc_setup_rx_device() is no longer
> successful, which means that userspace clients will actually end up
> calling nvt_open()/nvt_close().
> And since nvt_open() is broken, the device doesn't seem to work as
> expected.
Since that commit was in v4.16, should we have the following:
Cc: stable@vger.kernel.org # v4.16+
On this commit (and not the other two, if I understand them correctly)?
Thanks,
Sean
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=199597
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Jarod Wilson <jarod@redhat.com>
> Cc: Sean Young <sean@mess.org>
> ---
> drivers/media/rc/nuvoton-cir.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/rc/nuvoton-cir.c b/drivers/media/rc/nuvoton-cir.c
> index 5e1d866a61a5..ce8949b6549d 100644
> --- a/drivers/media/rc/nuvoton-cir.c
> +++ b/drivers/media/rc/nuvoton-cir.c
> @@ -922,6 +922,9 @@ static int nvt_open(struct rc_dev *dev)
> struct nvt_dev *nvt = dev->priv;
> unsigned long flags;
>
> + /* enable the CIR logical device */
> + nvt_enable_logical_dev(nvt, LOGICAL_DEV_CIR);
> +
> spin_lock_irqsave(&nvt->lock, flags);
>
> /* set function enable flags */
> @@ -937,9 +940,6 @@ static int nvt_open(struct rc_dev *dev)
>
> spin_unlock_irqrestore(&nvt->lock, flags);
>
> - /* enable the CIR logical device */
> - nvt_enable_logical_dev(nvt, LOGICAL_DEV_CIR);
> -
> return 0;
> }
>
> --
> 2.17.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] media: rc: nuvoton: Tweak the interrupt enabling dance
2018-05-21 15:54 ` [PATCH 1/3] media: rc: nuvoton: Tweak the interrupt enabling dance Sean Young
@ 2018-05-21 16:22 ` Michał Winiarski
0 siblings, 0 replies; 11+ messages in thread
From: Michał Winiarski @ 2018-05-21 16:22 UTC (permalink / raw)
To: Sean Young; +Cc: linux-media, Jarod Wilson
On Mon, May 21, 2018 at 04:54:07PM +0100, Sean Young wrote:
> On Mon, May 21, 2018 at 04:38:01PM +0200, Michał Winiarski wrote:
> > It appears that we need to enable CIR device before attempting to touch
> > some of the registers. Previously, this was not a big issue, since we
> > were rarely seeing nvt_close() getting called.
> >
> > Unfortunately, since:
> > cb84343fced1 ("media: lirc: do not call close() or open() on unregistered devices")
> >
> > The initial open() during probe from rc_setup_rx_device() is no longer
> > successful, which means that userspace clients will actually end up
> > calling nvt_open()/nvt_close().
> > And since nvt_open() is broken, the device doesn't seem to work as
> > expected.
>
> Since that commit was in v4.16, should we have the following:
>
> Cc: stable@vger.kernel.org # v4.16+
>
> On this commit (and not the other two, if I understand them correctly)?
Correct. I even had it in the series attached to the bugzilla.
Dropped it because the bug reporters have not confirmed that it fixes their
problem yet. (works for me though...)
-Michał
>
> Thanks,
> Sean
>
> >
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=199597
> > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> > Cc: Jarod Wilson <jarod@redhat.com>
> > Cc: Sean Young <sean@mess.org>
> > ---
> > drivers/media/rc/nuvoton-cir.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] media: rc: nuvoton: Keep device enabled during reg init
2018-05-21 14:38 ` [PATCH 3/3] media: rc: nuvoton: Keep device enabled during reg init Michał Winiarski
@ 2018-05-24 11:31 ` Sean Young
2018-05-25 13:35 ` Michał Winiarski
2018-05-25 14:28 ` [PATCH v2 " Michał Winiarski
1 sibling, 1 reply; 11+ messages in thread
From: Sean Young @ 2018-05-24 11:31 UTC (permalink / raw)
To: Michał Winiarski; +Cc: linux-media, Jarod Wilson
On Mon, May 21, 2018 at 04:38:03PM +0200, Michał Winiarski wrote:
> Doing writes when the device is disabled seems to be a NOOP.
> Let's enable the device, write the values, and then disable it on init.
> This changes the behavior for wake device, which is now being disabled
> after init.
I don't have the datasheet so I might be misunderstanding this. We want
the IR wakeup to work fine even after kernel crash/power loss, right?
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Jarod Wilson <jarod@redhat.com>
> Cc: Sean Young <sean@mess.org>
> ---
> drivers/media/rc/nuvoton-cir.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/rc/nuvoton-cir.c b/drivers/media/rc/nuvoton-cir.c
> index eebd6fef5602..61b68cde35f1 100644
> --- a/drivers/media/rc/nuvoton-cir.c
> +++ b/drivers/media/rc/nuvoton-cir.c
> @@ -535,6 +535,8 @@ static void nvt_set_cir_iren(struct nvt_dev *nvt)
>
> static void nvt_cir_regs_init(struct nvt_dev *nvt)
> {
> + nvt_enable_logical_dev(nvt, LOGICAL_DEV_CIR);
> +
> /* set sample limit count (PE interrupt raised when reached) */
> nvt_cir_reg_write(nvt, CIR_RX_LIMIT_COUNT >> 8, CIR_SLCH);
> nvt_cir_reg_write(nvt, CIR_RX_LIMIT_COUNT & 0xff, CIR_SLCL);
> @@ -546,10 +548,14 @@ static void nvt_cir_regs_init(struct nvt_dev *nvt)
> /* clear hardware rx and tx fifos */
> nvt_clear_cir_fifo(nvt);
> nvt_clear_tx_fifo(nvt);
> +
> + nvt_disable_logical_dev(nvt, LOGICAL_DEV_CIR);
> }
>
> static void nvt_cir_wake_regs_init(struct nvt_dev *nvt)
> {
> + nvt_enable_logical_dev(nvt, LOGICAL_DEV_CIR_WAKE);
> +
> /*
> * Disable RX, set specific carrier on = low, off = high,
> * and sample period (currently 50us)
> @@ -562,8 +568,7 @@ static void nvt_cir_wake_regs_init(struct nvt_dev *nvt)
> /* clear any and all stray interrupts */
> nvt_cir_wake_reg_write(nvt, 0xff, CIR_WAKE_IRSTS);
>
> - /* enable the CIR WAKE logical device */
> - nvt_enable_logical_dev(nvt, LOGICAL_DEV_CIR_WAKE);
> + nvt_disable_logical_dev(nvt, LOGICAL_DEV_CIR);
The way I read this is that the CIR, not CIR_WAKE, is being disabled,
which seems contrary to what the commit message says.
Sean
> }
>
> static void nvt_enable_wake(struct nvt_dev *nvt)
> --
> 2.17.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] media: rc: nuvoton: Keep device enabled during reg init
2018-05-24 11:31 ` Sean Young
@ 2018-05-25 13:35 ` Michał Winiarski
2018-05-25 13:59 ` Sean Young
0 siblings, 1 reply; 11+ messages in thread
From: Michał Winiarski @ 2018-05-25 13:35 UTC (permalink / raw)
To: Sean Young; +Cc: linux-media, Jarod Wilson
On Thu, May 24, 2018 at 12:31:40PM +0100, Sean Young wrote:
> On Mon, May 21, 2018 at 04:38:03PM +0200, Michał Winiarski wrote:
> > Doing writes when the device is disabled seems to be a NOOP.
> > Let's enable the device, write the values, and then disable it on init.
> > This changes the behavior for wake device, which is now being disabled
> > after init.
>
> I don't have the datasheet so I might be misunderstanding this. We want
> the IR wakeup to work fine even after kernel crash/power loss, right?
[snip]
Right, that makes sense. I completely ignored this scenario.
> > - /* enable the CIR WAKE logical device */
> > - nvt_enable_logical_dev(nvt, LOGICAL_DEV_CIR_WAKE);
> > + nvt_disable_logical_dev(nvt, LOGICAL_DEV_CIR);
>
> The way I read this is that the CIR, not CIR_WAKE, is being disabled,
> which seems contrary to what the commit message says.
>
That's a typo. And by accident it makes the wake_device work correctly :)
I think that registers init logic was still broken though, operating under the
assumption that the device is enabled on module load...
I guess we should just remove disable(LOGICAL_DEV_CIR) from wake_regs_init.
Have you already included this in any non-rebasing tree?
Should I send a v2 or fixup on top?
-Michał
>
> Sean
>
> > }
> >
> > static void nvt_enable_wake(struct nvt_dev *nvt)
> > --
> > 2.17.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] media: rc: nuvoton: Keep device enabled during reg init
2018-05-25 13:35 ` Michał Winiarski
@ 2018-05-25 13:59 ` Sean Young
2018-05-25 14:42 ` Michał Winiarski
0 siblings, 1 reply; 11+ messages in thread
From: Sean Young @ 2018-05-25 13:59 UTC (permalink / raw)
To: Michał Winiarski; +Cc: linux-media, Jarod Wilson
On Fri, May 25, 2018 at 03:35:23PM +0200, Michał Winiarski wrote:
> On Thu, May 24, 2018 at 12:31:40PM +0100, Sean Young wrote:
> > On Mon, May 21, 2018 at 04:38:03PM +0200, Michał Winiarski wrote:
> > > Doing writes when the device is disabled seems to be a NOOP.
> > > Let's enable the device, write the values, and then disable it on init.
> > > This changes the behavior for wake device, which is now being disabled
> > > after init.
> >
> > I don't have the datasheet so I might be misunderstanding this. We want
> > the IR wakeup to work fine even after kernel crash/power loss, right?
>
> [snip]
>
> Right, that makes sense. I completely ignored this scenario.
>
> > > - /* enable the CIR WAKE logical device */
> > > - nvt_enable_logical_dev(nvt, LOGICAL_DEV_CIR_WAKE);
> > > + nvt_disable_logical_dev(nvt, LOGICAL_DEV_CIR);
> >
> > The way I read this is that the CIR, not CIR_WAKE, is being disabled,
> > which seems contrary to what the commit message says.
> >
>
> That's a typo. And by accident it makes the wake_device work correctly :)
> I think that registers init logic was still broken though, operating under the
> assumption that the device is enabled on module load...
>
> I guess we should just remove disable(LOGICAL_DEV_CIR) from wake_regs_init.
>
> Have you already included this in any non-rebasing tree?
Nothing has been applied yet.
> Should I send a v2 or fixup on top?
I don't have the hardware to test this, a v2 would be appreciated.
We're late in the release cycle and I'm wondering if this patch would also
solve the nuvoton probe problem:
https://patchwork.linuxtv.org/patch/49874/
Thanks,
Sean
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 3/3] media: rc: nuvoton: Keep device enabled during reg init
2018-05-21 14:38 ` [PATCH 3/3] media: rc: nuvoton: Keep device enabled during reg init Michał Winiarski
2018-05-24 11:31 ` Sean Young
@ 2018-05-25 14:28 ` Michał Winiarski
1 sibling, 0 replies; 11+ messages in thread
From: Michał Winiarski @ 2018-05-25 14:28 UTC (permalink / raw)
To: linux-media; +Cc: Michał Winiarski, Jarod Wilson, Sean Young
Doing writes when the device is disabled seems to be a NOOP.
For CIR device, we should enable it, intialize it, and then disable it
until it's opened. CIR_WAKE should always be enabled.
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Cc: Jarod Wilson <jarod@redhat.com>
Cc: Sean Young <sean@mess.org>
---
Changes since v1:
- Don't mix CIR_WAKE with CIR device
- Correct the commit message
---
drivers/media/rc/nuvoton-cir.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/media/rc/nuvoton-cir.c b/drivers/media/rc/nuvoton-cir.c
index eebd6fef5602..b8299c9a9744 100644
--- a/drivers/media/rc/nuvoton-cir.c
+++ b/drivers/media/rc/nuvoton-cir.c
@@ -535,6 +535,8 @@ static void nvt_set_cir_iren(struct nvt_dev *nvt)
static void nvt_cir_regs_init(struct nvt_dev *nvt)
{
+ nvt_enable_logical_dev(nvt, LOGICAL_DEV_CIR);
+
/* set sample limit count (PE interrupt raised when reached) */
nvt_cir_reg_write(nvt, CIR_RX_LIMIT_COUNT >> 8, CIR_SLCH);
nvt_cir_reg_write(nvt, CIR_RX_LIMIT_COUNT & 0xff, CIR_SLCL);
@@ -546,10 +548,14 @@ static void nvt_cir_regs_init(struct nvt_dev *nvt)
/* clear hardware rx and tx fifos */
nvt_clear_cir_fifo(nvt);
nvt_clear_tx_fifo(nvt);
+
+ nvt_disable_logical_dev(nvt, LOGICAL_DEV_CIR);
}
static void nvt_cir_wake_regs_init(struct nvt_dev *nvt)
{
+ nvt_enable_logical_dev(nvt, LOGICAL_DEV_CIR_WAKE);
+
/*
* Disable RX, set specific carrier on = low, off = high,
* and sample period (currently 50us)
@@ -561,9 +567,6 @@ static void nvt_cir_wake_regs_init(struct nvt_dev *nvt)
/* clear any and all stray interrupts */
nvt_cir_wake_reg_write(nvt, 0xff, CIR_WAKE_IRSTS);
-
- /* enable the CIR WAKE logical device */
- nvt_enable_logical_dev(nvt, LOGICAL_DEV_CIR_WAKE);
}
static void nvt_enable_wake(struct nvt_dev *nvt)
--
2.17.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] media: rc: nuvoton: Keep device enabled during reg init
2018-05-25 13:59 ` Sean Young
@ 2018-05-25 14:42 ` Michał Winiarski
2018-05-25 17:51 ` Sean Young
0 siblings, 1 reply; 11+ messages in thread
From: Michał Winiarski @ 2018-05-25 14:42 UTC (permalink / raw)
To: Sean Young; +Cc: linux-media, Jarod Wilson
On Fri, May 25, 2018 at 02:59:41PM +0100, Sean Young wrote:
> On Fri, May 25, 2018 at 03:35:23PM +0200, Michał Winiarski wrote:
> > On Thu, May 24, 2018 at 12:31:40PM +0100, Sean Young wrote:
> > > On Mon, May 21, 2018 at 04:38:03PM +0200, Michał Winiarski wrote:
> > > > Doing writes when the device is disabled seems to be a NOOP.
> > > > Let's enable the device, write the values, and then disable it on init.
> > > > This changes the behavior for wake device, which is now being disabled
> > > > after init.
> > >
> > > I don't have the datasheet so I might be misunderstanding this. We want
> > > the IR wakeup to work fine even after kernel crash/power loss, right?
> >
> > [snip]
> >
> > Right, that makes sense. I completely ignored this scenario.
> >
> > > > - /* enable the CIR WAKE logical device */
> > > > - nvt_enable_logical_dev(nvt, LOGICAL_DEV_CIR_WAKE);
> > > > + nvt_disable_logical_dev(nvt, LOGICAL_DEV_CIR);
> > >
> > > The way I read this is that the CIR, not CIR_WAKE, is being disabled,
> > > which seems contrary to what the commit message says.
> > >
> >
> > That's a typo. And by accident it makes the wake_device work correctly :)
> > I think that registers init logic was still broken though, operating under the
> > assumption that the device is enabled on module load...
> >
> > I guess we should just remove disable(LOGICAL_DEV_CIR) from wake_regs_init.
> >
> > Have you already included this in any non-rebasing tree?
>
> Nothing has been applied yet.
>
> > Should I send a v2 or fixup on top?
>
> I don't have the hardware to test this, a v2 would be appreciated.
>
> We're late in the release cycle and I'm wondering if this patch would also
> solve the nuvoton probe problem:
>
> https://patchwork.linuxtv.org/patch/49874/
It causes us to go back to previous behavior (we're refcounting open/close,
with your patch initial open on my system is coming from kbd_connect(), so
userspace close() doesn't propagate to nuvoton-cir).
It passes my test of "load the module with debug=1, see if I'm getting
interrupts".
If there's any scenario in which->close() would be called, it's still going to
be broken.
-Michał
>
> Thanks,
>
> Sean
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] media: rc: nuvoton: Keep device enabled during reg init
2018-05-25 14:42 ` Michał Winiarski
@ 2018-05-25 17:51 ` Sean Young
0 siblings, 0 replies; 11+ messages in thread
From: Sean Young @ 2018-05-25 17:51 UTC (permalink / raw)
To: Michał Winiarski; +Cc: linux-media, Jarod Wilson
On Fri, May 25, 2018 at 04:42:02PM +0200, Michał Winiarski wrote:
> On Fri, May 25, 2018 at 02:59:41PM +0100, Sean Young wrote:
> > On Fri, May 25, 2018 at 03:35:23PM +0200, Michał Winiarski wrote:
> > > On Thu, May 24, 2018 at 12:31:40PM +0100, Sean Young wrote:
> > > > On Mon, May 21, 2018 at 04:38:03PM +0200, Michał Winiarski wrote:
> > > > > Doing writes when the device is disabled seems to be a NOOP.
> > > > > Let's enable the device, write the values, and then disable it on init.
> > > > > This changes the behavior for wake device, which is now being disabled
> > > > > after init.
> > > >
> > > > I don't have the datasheet so I might be misunderstanding this. We want
> > > > the IR wakeup to work fine even after kernel crash/power loss, right?
> > >
> > > [snip]
> > >
> > > Right, that makes sense. I completely ignored this scenario.
> > >
> > > > > - /* enable the CIR WAKE logical device */
> > > > > - nvt_enable_logical_dev(nvt, LOGICAL_DEV_CIR_WAKE);
> > > > > + nvt_disable_logical_dev(nvt, LOGICAL_DEV_CIR);
> > > >
> > > > The way I read this is that the CIR, not CIR_WAKE, is being disabled,
> > > > which seems contrary to what the commit message says.
> > > >
> > >
> > > That's a typo. And by accident it makes the wake_device work correctly :)
> > > I think that registers init logic was still broken though, operating under the
> > > assumption that the device is enabled on module load...
> > >
> > > I guess we should just remove disable(LOGICAL_DEV_CIR) from wake_regs_init.
> > >
> > > Have you already included this in any non-rebasing tree?
> >
> > Nothing has been applied yet.
> >
> > > Should I send a v2 or fixup on top?
> >
> > I don't have the hardware to test this, a v2 would be appreciated.
> >
> > We're late in the release cycle and I'm wondering if this patch would also
> > solve the nuvoton probe problem:
> >
> > https://patchwork.linuxtv.org/patch/49874/
>
> It causes us to go back to previous behavior (we're refcounting open/close,
> with your patch initial open on my system is coming from kbd_connect(), so
> userspace close() doesn't propagate to nuvoton-cir).
>
> It passes my test of "load the module with debug=1, see if I'm getting
> interrupts".
>
> If there's any scenario in which->close() would be called, it's still going to
> be broken.
Great, thank you very much for testing that. I've created a pull request
for the v2 version.
Sean
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-05-25 17:51 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-21 14:38 [PATCH 1/3] media: rc: nuvoton: Tweak the interrupt enabling dance Michał Winiarski
2018-05-21 14:38 ` [PATCH 2/3] media: rc: nuvoton: Keep track of users on CIR enable/disable Michał Winiarski
2018-05-21 14:38 ` [PATCH 3/3] media: rc: nuvoton: Keep device enabled during reg init Michał Winiarski
2018-05-24 11:31 ` Sean Young
2018-05-25 13:35 ` Michał Winiarski
2018-05-25 13:59 ` Sean Young
2018-05-25 14:42 ` Michał Winiarski
2018-05-25 17:51 ` Sean Young
2018-05-25 14:28 ` [PATCH v2 " Michał Winiarski
2018-05-21 15:54 ` [PATCH 1/3] media: rc: nuvoton: Tweak the interrupt enabling dance Sean Young
2018-05-21 16:22 ` Michał Winiarski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.