Re: [PATCH 3/3] media: rc: nuvoton: Keep device enabled during reg init
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
[PATCH v2 3/3] media: rc: nuvoton: Keep device enabled during reg init
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 Cc: Jarod Wilson Cc: Sean Young --- 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
Re: [PATCH 3/3] media: rc: nuvoton: Keep device enabled during reg init
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
Re: [PATCH 1/3] media: rc: nuvoton: Tweak the interrupt enabling dance
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: sta...@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 > > Cc: Jarod Wilson > > Cc: Sean Young > > --- > > drivers/media/rc/nuvoton-cir.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-)
[PATCH 1/3] media: rc: nuvoton: Tweak the interrupt enabling dance
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 Cc: Jarod Wilson Cc: Sean Young --- 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
[PATCH 2/3] media: rc: nuvoton: Keep track of users on CIR enable/disable
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 Cc: Jarod Wilson Cc: Sean Young --- 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
[PATCH 3/3] media: rc: nuvoton: Keep device enabled during reg init
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 Cc: Jarod Wilson Cc: Sean Young --- 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