[PATCH] [media] ite-cir: make IR receive work after resume

[PATCH] [media] ite-cir: make IR receive work after resume

[Jarod Wilson]

Just recently acquired an Asus Eee Box PC with an onboard IR receiver
driven by ite-cir (ITE8713 sub-variant). Works out of the box with the
ite-cir driver in 2.6.39, but stops working after a suspend/resume
cycle. Its fixed by simply reinitializing registers after resume,
similar to what's done in the nuvoton-cir driver. I've not tested with
any other ITE variant, but code inspection suggests this should be safe
on all variants.

Reported-by: Stephan Raue <sraue@openelec.tv>
CC: Juan Jesús García de Soria <skandalfo@gmail.com>
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/media/rc/ite-cir.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/media/rc/ite-cir.c b/drivers/media/rc/ite-cir.c
index 43908a7..8488e53 100644
--- a/drivers/media/rc/ite-cir.c
+++ b/drivers/media/rc/ite-cir.c
@@ -1684,6 +1684,8 @@ static int ite_resume(struct pnp_dev *pdev)
 		/* wake up the transmitter */
 		wake_up_interruptible(&dev->tx_queue);
 	} else {
+		/* reinitialize hardware config registers */
+		itdev->params.init_hardware(itdev);
 		/* enable the receiver */
 		dev->params.enable_rx(dev);
 	}
-- 
1.7.1

Re: [PATCH] [media] ite-cir: make IR receive work after resume

[Jarod Wilson]

Jarod Wilson wrote:
> Just recently acquired an Asus Eee Box PC with an onboard IR receiver
> driven by ite-cir (ITE8713 sub-variant). Works out of the box with the
> ite-cir driver in 2.6.39, but stops working after a suspend/resume
> cycle. Its fixed by simply reinitializing registers after resume,
> similar to what's done in the nuvoton-cir driver. I've not tested with
> any other ITE variant, but code inspection suggests this should be safe
> on all variants.
>
> Reported-by: Stephan Raue<sraue@openelec.tv>
> CC: Juan Jesús García de Soria<skandalfo@gmail.com>
> Signed-off-by: Jarod Wilson<jarod@redhat.com>
> ---
>   drivers/media/rc/ite-cir.c |    2 ++
>   1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/media/rc/ite-cir.c b/drivers/media/rc/ite-cir.c
> index 43908a7..8488e53 100644
> --- a/drivers/media/rc/ite-cir.c
> +++ b/drivers/media/rc/ite-cir.c
> @@ -1684,6 +1684,8 @@ static int ite_resume(struct pnp_dev *pdev)
>   		/* wake up the transmitter */
>   		wake_up_interruptible(&dev->tx_queue);
>   	} else {
> +		/* reinitialize hardware config registers */
> +		itdev->params.init_hardware(itdev);
>   		/* enable the receiver */
>   		dev->params.enable_rx(dev);


Gah. I've obviously screwed this one up. Tested a locally built version 
of the module on the machine itself, then did a copy/paste of the 
init_hardware line from elsewhere in the driver where struct ite_dev was 
called itdev instead of just dev. v2 momentarily.

-- 
Jarod Wilson
jarod@redhat.com

[PATCH v2] [media] ite-cir: make IR receive work after resume

[Jarod Wilson]

Just recently acquired an Asus Eee Box PC with an onboard IR receiver
driven by ite-cir (ITE8713 sub-variant). Works out of the box with the
ite-cir driver in 2.6.39, but stops working after a suspend/resume
cycle. Its fixed by simply reinitializing registers after resume,
similar to what's done in the nuvoton-cir driver. I've not tested with
any other ITE variant, but code inspection suggests this should be safe
on all variants.

Reported-by: Stephan Raue <sraue@openelec.tv>
CC: Juan Jesús García de Soria <skandalfo@gmail.com>
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
v2: fix copy/paste thinko

 drivers/media/rc/ite-cir.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/media/rc/ite-cir.c b/drivers/media/rc/ite-cir.c
index 43908a7..253837e 100644
--- a/drivers/media/rc/ite-cir.c
+++ b/drivers/media/rc/ite-cir.c
@@ -1684,6 +1684,8 @@ static int ite_resume(struct pnp_dev *pdev)
 		/* wake up the transmitter */
 		wake_up_interruptible(&dev->tx_queue);
 	} else {
+		/* reinitialize hardware config registers */
+		dev->params.init_hardware(dev);
 		/* enable the receiver */
 		dev->params.enable_rx(dev);
 	}
-- 
1.7.1

Re: [PATCH] [media] ite-cir: make IR receive work after resume

[Juan Jesús García de Soria Lucena]

Hi Jarod, and thanks for looking at this.

El 09/05/2011 20:28, "Jarod Wilson" <jarod@redhat.com> escribió:
> --- a/drivers/media/rc/ite-cir.c
> +++ b/drivers/media/rc/ite-cir.c
> @@ -1684,6 +1684,8 @@ static int ite_resume(struct pnp_dev *pdev)
>                /* wake up the transmitter */
>                wake_up_interruptible(&dev->tx_queue);
>        } else {
> +               /* reinitialize hardware config registers */
> +               dev->params.init_hardware(dev);
>                /* enable the receiver */
>                dev->params.enable_rx(dev);
>        }
> --
> 1.7.1
>

But... wouldn't the hardware initialization be required too if the hardware got suspended while doing TX? I mean, probably the call to init_hardware() should be done in any case, just before the if... (forgive me if I'm off target; I'm looking at the code as I remember it, since I don't have it before me right now).

Best regards,

   Juan Jesús.

Re: [PATCH] [media] ite-cir: make IR receive work after resume

[Jarod Wilson]

Juan Jesús García de Soria Lucena wrote:
> Hi Jarod, and thanks for looking at this.
>
> El 09/05/2011 20:28, "Jarod Wilson"<jarod@redhat.com>  escribió:
>> --- a/drivers/media/rc/ite-cir.c
>> +++ b/drivers/media/rc/ite-cir.c
>> @@ -1684,6 +1684,8 @@ static int ite_resume(struct pnp_dev *pdev)
>>                 /* wake up the transmitter */
>>                 wake_up_interruptible(&dev->tx_queue);
>>         } else {
>> +               /* reinitialize hardware config registers */
>> +               dev->params.init_hardware(dev);
>>                 /* enable the receiver */
>>                 dev->params.enable_rx(dev);
>>         }
>> --
>> 1.7.1
>>
>
> But... wouldn't the hardware initialization be required too if the hardware got suspended while doing TX? I mean, probably the call to init_hardware() should be done in any case, just before the if... (forgive me if I'm off target; I'm looking at the code as I remember it, since I don't have it before me right now).

Well, looking at the resume function, I wasn't sure if I wanted to
mess with things while it was possibly trying to finish up tx, but
upon closer inspection, I don't think we can ever get into the
state where we're actually doing anything in the tx handler where
it would matter. If dev->transmitting is true and we're actually
able to grab the spinlock, it means we're just in the middle of
the mdelay for remaining_us, and everything done after that is
partially redundant with init_hardware anyway. So yeah, it looks
safe to me to just put in the init_hardware unconditionally above
the check for dev->transmitting.

On a related note though... what are the actual chances that we are
suspending in the middle of tx, and what are the chances it would
actually be of any use to resume that tx after waking up?

So what I'm now thinking is this: add a wait_event_interruptible on
tx_ended in the suspend path if dev->transmitting to let tx finish
before we suspend. Then in resume, we're never resuming in the
middle of tx and the whole conditional goes away.

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH] [media] ite-cir: make IR receive work after resume

[Juan Jesús García de Soria Lucena]

Hi again,

El lun, 09-05-2011 a las 15:45 -0400, Jarod Wilson escribió:
> Well, looking at the resume function, I wasn't sure if I wanted to
> mess with things while it was possibly trying to finish up tx, but
> upon closer inspection, I don't think we can ever get into the
> state where we're actually doing anything in the tx handler where
> it would matter. If dev->transmitting is true and we're actually
> able to grab the spinlock, it means we're just in the middle of
> the mdelay for remaining_us, and everything done after that is
> partially redundant with init_hardware anyway. So yeah, it looks
> safe to me to just put in the init_hardware unconditionally above
> the check for dev->transmitting.
> 
> On a related note though... what are the actual chances that we are
> suspending in the middle of tx, and what are the chances it would
> actually be of any use to resume that tx after waking up?
> 
> So what I'm now thinking is this: add a wait_event_interruptible on
> tx_ended in the suspend path if dev->transmitting to let tx finish
> before we suspend. Then in resume, we're never resuming in the
> middle of tx and the whole conditional goes away.

That looks like an approach way nicer than the current one. That way
sent codes wouldn't be interrupted during transmission by a concurrent
suspend request and, yes, the resume function is more elegant too.

Please go ahead with this idea if you wish :-)


Best regards,

    Juan Jesús.