Re: [PATCH 3/3] media: rc: nuvoton: Keep device enabled during reg init
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
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
Re: [PATCH 3/3] media: rc: nuvoton: Keep device enabled during reg init
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
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 3/3] media: rc: nuvoton: Keep device enabled during reg init
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 > 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); 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