diff mbox

[2/9] IR: minor fixes:

Message ID 1280330051-27732-3-git-send-email-maximlevitsky@gmail.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Maxim Levitsky July 28, 2010, 3:14 p.m. UTC 
  * lirc: Don't propagate reset event to userspace
* lirc: Remove strange logic from lirc that would make first sample always be pulse
* Make TO_US macro actualy print what it should.

Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
---
 drivers/media/IR/ir-core-priv.h  |    3 +--
 drivers/media/IR/ir-lirc-codec.c |   14 ++++++++------
 drivers/media/IR/ir-raw-event.c  |    3 +++
 3 files changed, 12 insertions(+), 8 deletions(-)

Comments

Mauro Carvalho Chehab July 28, 2010, 4:01 p.m. UTC | #1 
Em 28-07-2010 12:14, Maxim Levitsky escreveu:
> * lirc: Don't propagate reset event to userspace
> * lirc: Remove strange logic from lirc that would make first sample always be pulse
> * Make TO_US macro actualy print what it should.
> 
> Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> ---
>  drivers/media/IR/ir-core-priv.h  |    3 +--
>  drivers/media/IR/ir-lirc-codec.c |   14 ++++++++------
>  drivers/media/IR/ir-raw-event.c  |    3 +++
>  3 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/IR/ir-core-priv.h b/drivers/media/IR/ir-core-priv.h
> index babd520..8ce80e4 100644
> --- a/drivers/media/IR/ir-core-priv.h
> +++ b/drivers/media/IR/ir-core-priv.h
> @@ -76,7 +76,6 @@ struct ir_raw_event_ctrl {
>  	struct lirc_codec {
>  		struct ir_input_dev *ir_dev;
>  		struct lirc_driver *drv;
> -		int lircdata;
>  	} lirc;
>  };
>  
> @@ -104,7 +103,7 @@ static inline void decrease_duration(struct ir_raw_event *ev, unsigned duration)
>  		ev->duration -= duration;
>  }
>  
> -#define TO_US(duration)			(((duration) + 500) / 1000)
> +#define TO_US(duration)			((duration) / 1000)

It is better to keep rounding the duration to its closest value.

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
Maxim Levitsky July 28, 2010, 4:38 p.m. UTC | #2 
On Wed, 2010-07-28 at 13:01 -0300, Mauro Carvalho Chehab wrote:
> Em 28-07-2010 12:14, Maxim Levitsky escreveu:
> > * lirc: Don't propagate reset event to userspace
> > * lirc: Remove strange logic from lirc that would make first sample always be pulse
> > * Make TO_US macro actualy print what it should.
> > 
> > Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> > ---
> >  drivers/media/IR/ir-core-priv.h  |    3 +--
> >  drivers/media/IR/ir-lirc-codec.c |   14 ++++++++------
> >  drivers/media/IR/ir-raw-event.c  |    3 +++
> >  3 files changed, 12 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/media/IR/ir-core-priv.h b/drivers/media/IR/ir-core-priv.h
> > index babd520..8ce80e4 100644
> > --- a/drivers/media/IR/ir-core-priv.h
> > +++ b/drivers/media/IR/ir-core-priv.h
> > @@ -76,7 +76,6 @@ struct ir_raw_event_ctrl {
> >  	struct lirc_codec {
> >  		struct ir_input_dev *ir_dev;
> >  		struct lirc_driver *drv;
> > -		int lircdata;
> >  	} lirc;
> >  };
> >  
> > @@ -104,7 +103,7 @@ static inline void decrease_duration(struct ir_raw_event *ev, unsigned duration)
> >  		ev->duration -= duration;
> >  }
> >  
> > -#define TO_US(duration)			(((duration) + 500) / 1000)
> > +#define TO_US(duration)			((duration) / 1000)
> 
> It is better to keep rounding the duration to its closest value.

But that breaks if duration is already at maximum. At that case,
you see usual signed int wrap from positive to negative, and abnormally
large space...

I didn't notice though that you do rounding here.
I replace that with something better.

Best regards,
	Maxim Levitsky


--
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 28, 2010, 5:23 p.m. UTC | #3 
On Wed, Jul 28, 2010 at 06:14:04PM +0300, Maxim Levitsky wrote:
> * lirc: Don't propagate reset event to userspace
> * lirc: Remove strange logic from lirc that would make first sample always be pulse
> * Make TO_US macro actualy print what it should.
> 
> Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>

For the ir-lirc-codec-specific bits:

Acked-by: Jarod Wilson <jarod@redhat.com>

I'm inclined to pull them into my tree now, and the IR_dprintk and TO_US
portions can be handled separately.
Mauro Carvalho Chehab July 28, 2010, 5:59 p.m. UTC | #4 
Em 28-07-2010 13:38, Maxim Levitsky escreveu:
> On Wed, 2010-07-28 at 13:01 -0300, Mauro Carvalho Chehab wrote:
>> Em 28-07-2010 12:14, Maxim Levitsky escreveu:
>>> * lirc: Don't propagate reset event to userspace
>>> * lirc: Remove strange logic from lirc that would make first sample always be pulse
>>> * Make TO_US macro actualy print what it should.
>>>
>>> Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
>>> ---
>>>  drivers/media/IR/ir-core-priv.h  |    3 +--
>>>  drivers/media/IR/ir-lirc-codec.c |   14 ++++++++------
>>>  drivers/media/IR/ir-raw-event.c  |    3 +++
>>>  3 files changed, 12 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/media/IR/ir-core-priv.h b/drivers/media/IR/ir-core-priv.h
>>> index babd520..8ce80e4 100644
>>> --- a/drivers/media/IR/ir-core-priv.h
>>> +++ b/drivers/media/IR/ir-core-priv.h
>>> @@ -76,7 +76,6 @@ struct ir_raw_event_ctrl {
>>>  	struct lirc_codec {
>>>  		struct ir_input_dev *ir_dev;
>>>  		struct lirc_driver *drv;
>>> -		int lircdata;
>>>  	} lirc;
>>>  };
>>>  
>>> @@ -104,7 +103,7 @@ static inline void decrease_duration(struct ir_raw_event *ev, unsigned duration)
>>>  		ev->duration -= duration;
>>>  }
>>>  
>>> -#define TO_US(duration)			(((duration) + 500) / 1000)
>>> +#define TO_US(duration)			((duration) / 1000)
>>
>> It is better to keep rounding the duration to its closest value.
> 
> But that breaks if duration is already at maximum. At that case,
> you see usual signed int wrap from positive to negative, and abnormally
> large space...
> 
> I didn't notice though that you do rounding here.
> I replace that with something better.

There's a function for rounding at kernel. The better is to use it, as it should already
solve the signal wrap.

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
diff mbox

Patch

diff --git a/drivers/media/IR/ir-core-priv.h b/drivers/media/IR/ir-core-priv.h
index babd520..8ce80e4 100644
--- a/drivers/media/IR/ir-core-priv.h
+++ b/drivers/media/IR/ir-core-priv.h
@@ -76,7 +76,6 @@  struct ir_raw_event_ctrl {
 	struct lirc_codec {
 		struct ir_input_dev *ir_dev;
 		struct lirc_driver *drv;
-		int lircdata;
 	} lirc;
 };
 
@@ -104,7 +103,7 @@  static inline void decrease_duration(struct ir_raw_event *ev, unsigned duration)
 		ev->duration -= duration;
 }
 
-#define TO_US(duration)			(((duration) + 500) / 1000)
+#define TO_US(duration)			((duration) / 1000)
 #define TO_STR(is_pulse)		((is_pulse) ? "pulse" : "space")
 #define IS_RESET(ev)			(ev.duration == 0)
 
diff --git a/drivers/media/IR/ir-lirc-codec.c b/drivers/media/IR/ir-lirc-codec.c
index 3ba482d..8ca01fd 100644
--- a/drivers/media/IR/ir-lirc-codec.c
+++ b/drivers/media/IR/ir-lirc-codec.c
@@ -32,6 +32,7 @@ 
 static int ir_lirc_decode(struct input_dev *input_dev, struct ir_raw_event ev)
 {
 	struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
+	int sample;
 
 	if (!(ir_dev->raw->enabled_protocols & IR_TYPE_LIRC))
 		return 0;
@@ -39,18 +40,21 @@  static int ir_lirc_decode(struct input_dev *input_dev, struct ir_raw_event ev)
 	if (!ir_dev->raw->lirc.drv || !ir_dev->raw->lirc.drv->rbuf)
 		return -EINVAL;
 
+	if (IS_RESET(ev))
+		return 0;
+
 	IR_dprintk(2, "LIRC data transfer started (%uus %s)\n",
 		   TO_US(ev.duration), TO_STR(ev.pulse));
 
-	ir_dev->raw->lirc.lircdata += ev.duration / 1000;
+
+	sample = ev.duration / 1000;
 	if (ev.pulse)
-		ir_dev->raw->lirc.lircdata |= PULSE_BIT;
+		sample |= PULSE_BIT;
 
 	lirc_buffer_write(ir_dev->raw->lirc.drv->rbuf,
-			  (unsigned char *) &ir_dev->raw->lirc.lircdata);
+			  (unsigned char *) &sample);
 	wake_up(&ir_dev->raw->lirc.drv->rbuf->wait_poll);
 
-	ir_dev->raw->lirc.lircdata = 0;
 
 	return 0;
 }
@@ -224,8 +228,6 @@  static int ir_lirc_register(struct input_dev *input_dev)
 
 	ir_dev->raw->lirc.drv = drv;
 	ir_dev->raw->lirc.ir_dev = ir_dev;
-	ir_dev->raw->lirc.lircdata = PULSE_MASK;
-
 	return 0;
 
 lirc_register_failed:
diff --git a/drivers/media/IR/ir-raw-event.c b/drivers/media/IR/ir-raw-event.c
index 6f192ef..ab9c4da 100644
--- a/drivers/media/IR/ir-raw-event.c
+++ b/drivers/media/IR/ir-raw-event.c
@@ -66,6 +66,9 @@  int ir_raw_event_store(struct input_dev *input_dev, struct ir_raw_event *ev)
 	if (!ir->raw)
 		return -EINVAL;
 
+	IR_dprintk(2, "sample: (05%dus %s)\n", TO_US(ev->duration),
+							TO_STR(ev->pulse));
+
 	if (kfifo_in(&ir->raw->kfifo, ev, sizeof(*ev)) != sizeof(*ev))
 		return -ENOMEM;