diff mbox

[2/9,media] mceusb: give hardware time to reply to cmds

Message ID 1310681394-3530-3-git-send-email-jarod@redhat.com (mailing list archive)
State Superseded
Headers show

Commit Message

Jarod Wilson July 14, 2011, 10:09 p.m. UTC
Sometimes the init routine is blasting commands out to the hardware
faster than it can reply. Throw a brief delay in there to give the
hardware a chance to reply before we send the next command.

Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/media/rc/mceusb.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

Comments

Mauro Carvalho Chehab July 14, 2011, 11:30 p.m. UTC | #1
Em 14-07-2011 19:09, Jarod Wilson escreveu:
> Sometimes the init routine is blasting commands out to the hardware
> faster than it can reply. Throw a brief delay in there to give the
> hardware a chance to reply before we send the next command.
> 
> Signed-off-by: Jarod Wilson <jarod@redhat.com>
> ---
>  drivers/media/rc/mceusb.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/rc/mceusb.c b/drivers/media/rc/mceusb.c
> index 111bead..13a853b 100644
> --- a/drivers/media/rc/mceusb.c
> +++ b/drivers/media/rc/mceusb.c
> @@ -37,6 +37,7 @@
>  #include <linux/slab.h>
>  #include <linux/usb.h>
>  #include <linux/usb/input.h>
> +#include <linux/delay.h>
>  #include <media/rc-core.h>
>  
>  #define DRIVER_VERSION	"1.91"
> @@ -735,6 +736,7 @@ static void mce_request_packet(struct mceusb_dev *ir, unsigned char *data,
>  static void mce_async_out(struct mceusb_dev *ir, unsigned char *data, int size)
>  {
>  	mce_request_packet(ir, data, size, MCEUSB_TX);
> +	mdelay(10);

Can't it be a msleep() instead? Delays spend more power, and keeps the CPU busy while
running.

>  }
>  
>  static void mce_flush_rx_buffer(struct mceusb_dev *ir, int size)

Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarod Wilson July 18, 2011, 5:47 p.m. UTC | #2
Mauro Carvalho Chehab wrote:
> Em 14-07-2011 19:09, Jarod Wilson escreveu:
>> Sometimes the init routine is blasting commands out to the hardware
>> faster than it can reply. Throw a brief delay in there to give the
>> hardware a chance to reply before we send the next command.
>>
>> Signed-off-by: Jarod Wilson<jarod@redhat.com>
>> ---
>>   drivers/media/rc/mceusb.c |    2 ++
>>   1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/media/rc/mceusb.c b/drivers/media/rc/mceusb.c
>> index 111bead..13a853b 100644
>> --- a/drivers/media/rc/mceusb.c
>> +++ b/drivers/media/rc/mceusb.c
>> @@ -37,6 +37,7 @@
>>   #include<linux/slab.h>
>>   #include<linux/usb.h>
>>   #include<linux/usb/input.h>
>> +#include<linux/delay.h>
>>   #include<media/rc-core.h>
>>
>>   #define DRIVER_VERSION	"1.91"
>> @@ -735,6 +736,7 @@ static void mce_request_packet(struct mceusb_dev *ir, unsigned char *data,
>>   static void mce_async_out(struct mceusb_dev *ir, unsigned char *data, int size)
>>   {
>>   	mce_request_packet(ir, data, size, MCEUSB_TX);
>> +	mdelay(10);
>
> Can't it be a msleep() instead? Delays spend more power, and keeps the CPU busy while
> running.

I think I was thinking we'd end up sleeping in an interrupt handler when 
we shouldn't be, but upon closer code inspection and actual testing, 
that's not the case, so yeah, those can be msleeps. While testing all 
code paths, I also discovered that patch 6 in the series breaks lirc tx 
support (the lirc dev is registered before the tx function pointers are 
filled in), so I'll resend at least patches 2 and 6...
Jarod Wilson July 18, 2011, 6:11 p.m. UTC | #3
Jarod Wilson wrote:
> Mauro Carvalho Chehab wrote:
>> Em 14-07-2011 19:09, Jarod Wilson escreveu:
>>> Sometimes the init routine is blasting commands out to the hardware
>>> faster than it can reply. Throw a brief delay in there to give the
>>> hardware a chance to reply before we send the next command.
>>>
>>> Signed-off-by: Jarod Wilson<jarod@redhat.com>
>>> ---
>>> drivers/media/rc/mceusb.c | 2 ++
>>> 1 files changed, 2 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/media/rc/mceusb.c b/drivers/media/rc/mceusb.c
>>> index 111bead..13a853b 100644
>>> --- a/drivers/media/rc/mceusb.c
>>> +++ b/drivers/media/rc/mceusb.c
>>> @@ -37,6 +37,7 @@
>>> #include<linux/slab.h>
>>> #include<linux/usb.h>
>>> #include<linux/usb/input.h>
>>> +#include<linux/delay.h>
>>> #include<media/rc-core.h>
>>>
>>> #define DRIVER_VERSION "1.91"
>>> @@ -735,6 +736,7 @@ static void mce_request_packet(struct mceusb_dev
>>> *ir, unsigned char *data,
>>> static void mce_async_out(struct mceusb_dev *ir, unsigned char *data,
>>> int size)
>>> {
>>> mce_request_packet(ir, data, size, MCEUSB_TX);
>>> + mdelay(10);
>>
>> Can't it be a msleep() instead? Delays spend more power, and keeps the
>> CPU busy while
>> running.
>
> I think I was thinking we'd end up sleeping in an interrupt handler when
> we shouldn't be, but upon closer code inspection and actual testing,
> that's not the case, so yeah, those can be msleeps. While testing all
> code paths, I also discovered that patch 6 in the series breaks lirc tx
> support (the lirc dev is registered before the tx function pointers are
> filled in), so I'll resend at least patches 2 and 6...

I'll just resend an entire v2 series, the changes to patch 2 have 
cascading impact on multiple patches in the rest of the series.
diff mbox

Patch

diff --git a/drivers/media/rc/mceusb.c b/drivers/media/rc/mceusb.c
index 111bead..13a853b 100644
--- a/drivers/media/rc/mceusb.c
+++ b/drivers/media/rc/mceusb.c
@@ -37,6 +37,7 @@ 
 #include <linux/slab.h>
 #include <linux/usb.h>
 #include <linux/usb/input.h>
+#include <linux/delay.h>
 #include <media/rc-core.h>
 
 #define DRIVER_VERSION	"1.91"
@@ -735,6 +736,7 @@  static void mce_request_packet(struct mceusb_dev *ir, unsigned char *data,
 static void mce_async_out(struct mceusb_dev *ir, unsigned char *data, int size)
 {
 	mce_request_packet(ir, data, size, MCEUSB_TX);
+	mdelay(10);
 }
 
 static void mce_flush_rx_buffer(struct mceusb_dev *ir, int size)