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

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

[PATCH] [media] ite-cir: finish tx before suspending

[Jarod Wilson]

Continuing with IR transmit after resuming from suspend seems fairly
useless, given that the only place we can actually end up suspending is
after IR has been send and we're simply mdelay'ing. Lets simplify the
resume path by just waiting on tx to complete in the suspend path, then
we know we can't be transmitting on resume, and reinitialization of the
hardware registers becomes more straight-forward.

CC: Juan Jesús García de Soria <skandalfo@gmail.com>
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
Nb: this patch relies upon my earlier patch to add the init_hardware
calls to the resume path in the first place.

 drivers/media/rc/ite-cir.c |   16 +++++++---------
 1 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/media/rc/ite-cir.c b/drivers/media/rc/ite-cir.c
index d1dec5c..e716b93 100644
--- a/drivers/media/rc/ite-cir.c
+++ b/drivers/media/rc/ite-cir.c
@@ -1650,6 +1650,9 @@ static int ite_suspend(struct pnp_dev *pdev, pm_message_t state)
 
 	ite_dbg("%s called", __func__);
 
+	/* wait for any transmission to end */
+	wait_event_interruptible(dev->tx_ended, !dev->transmitting);
+
 	spin_lock_irqsave(&dev->lock, flags);
 
 	/* disable all interrupts */
@@ -1670,15 +1673,10 @@ static int ite_resume(struct pnp_dev *pdev)
 
 	spin_lock_irqsave(&dev->lock, flags);
 
-	if (dev->transmitting) {
-		/* 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);
-	}
+	/* reinitialize hardware config registers */
+	dev->params.init_hardware(dev);
+	/* enable the receiver */
+	dev->params.enable_rx(dev);
 
 	spin_unlock_irqrestore(&dev->lock, flags);
 
-- 
1.7.1

Re: [PATCH] [media] ite-cir: finish tx before suspending

[Jarod Wilson]

Jarod Wilson wrote:
> Continuing with IR transmit after resuming from suspend seems fairly
> useless, given that the only place we can actually end up suspending is
> after IR has been send and we're simply mdelay'ing. Lets simplify the
> resume path by just waiting on tx to complete in the suspend path, then
> we know we can't be transmitting on resume, and reinitialization of the
> hardware registers becomes more straight-forward.
>
> CC: Juan Jesús García de Soria<skandalfo@gmail.com>
> Signed-off-by: Jarod Wilson<jarod@redhat.com>
> ---
> Nb: this patch relies upon my earlier patch to add the init_hardware
> calls to the resume path in the first place.

Also note: I don't have tx-capable hardware (or at least, there's no
tx hardware wired up), so I haven't actually tested this, but the code 
added to ite_suspend is more or less cloned from ite_close.

-- 
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.