Re: [PATCH 3/3] media: rc: nuvoton: Keep device enabled during reg init

[Sean Young]

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

[Michał Winiarski]

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

[Sean Young]

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

[Michał Winiarski]

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

[Sean Young]

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

[PATCH 3/3] media: rc: nuvoton: Keep device enabled during reg init

[Michał Winiarski]

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