Teach drivers/media/IR/ir-raw-event.c to use durations

Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline; filename=use-pulse-space-timings-in-ir-raw

drivers/media/IR/ir-raw-event.c is currently written with the assumption that all "raw" hardware will generate events only on state change (i.e. when a pulse or space starts).

However, some hardware (like mceusb, probably the most popular IR receiver out there) only generates duration data (and that data is buffered so using any kind of timing on the data is futile).

This patch (which is not tested since I haven't yet converted a driver for any of my hardware to ir-core yet, will do soon) is a RFC on my proposed interface change...once I get the green light on the interface change itself I'll make sure that the decoders actually work :)

The rc5 decoder has also gained rc5x support and the use of kfifo's for intermediate storage is gone (since there is no need for it).

-int ir_raw_event_store(struct input_dev *input_dev, enum raw_event_type type)

drivers/media/IR/ir-raw-event.c +11 -3

-int ir_raw_event_handle(struct input_dev *input_dev)
+/**
+ * ir_raw_event_edge() - notify raw ir decoders of the start of a pulse/space
+ * @input_dev:	the struct input_dev device descriptor
+ * @type:	the type of the event that has occurred
+ *
+ * This routine is used to notify the raw ir decoders on the beginning of an
+ * ir pulse or space (or the start/end of ir reception). This is used by
+ * hardware which does not provide durations directly but only interrupts
+ * (or similar events) on state change.
+ */
+void ir_raw_event_edge(struct input_dev *input_dev, enum raw_event_type type)
 {
-	struct ir_input_dev		*ir = input_get_drvdata(input_dev);
-	int				rc;

drivers/media/IR/ir-raw-event.c +3 -12

-
-	for (i = 0; i < len; i++) {
-		rc = kfifo_out(&ir->raw->kfifo, &ev, sizeof(ev));
-		if (rc != sizeof(ev)) {
-			IR_dprintk(1, "overflow error: received %d instead of %zd\n",
-				   rc, sizeof(ev));
-			return -EINVAL;
-		}
-		IR_dprintk(2, "event type %d, time before event: %07luus\n",
-			ev.type, (ev.delta.tv_nsec + 500) / 1000);
-		rc = RUN_DECODER(decode, input_dev, &ev);
-	}
+	struct ir_input_dev	*ir = input_get_drvdata(input_dev);
+	ktime_t			now;
+	s64			delta; /* us */

Hi David,

I won't comment every single bits of the change, since we're more interested on the conceptual aspects.

Quoted 1 line

-int ir_raw_event_store(struct input_dev *input_dev, enum raw_event_type type)

Don't remove the raw_event_store. It is needed by the hardware that gets events from IRQ/polling. For sure another interface is needed, for the cases where the hardware pass their own time measure, like cx18 (http://linuxtv.org/hg/~awalls/cx23885-ir2/rev/2cfef53b95a2).

For those, we need something like:

int ir_raw_event_time_store(struct input_dev *input_dev, enum raw_event_type type, u32 nsecs)

Where, instead of using ktime_get_ts(), it will use the timing provided by the hardware.

Quoted 10 lines · Mauro Carvalho Chehab

I won't comment every single bits of the change, since we're more interested on the conceptual
aspects.

Quoted 1 line

-int ir_raw_event_store(struct input_dev *input_dev, enum raw_event_type type)

Don't remove the raw_event_store. It is needed by the hardware that gets events from IRQ/polling. For sure another interface is needed, for the cases where the hardware pass their own time measure, like cx18 (http://linuxtv.org/hg/~awalls/cx23885-ir2/rev/2cfef53b95a2).

Quoted 5 lines

For those, we need something like:

int ir_raw_event_time_store(struct input_dev *input_dev, enum raw_event_type type, u32 nsecs)

Where, instead of using ktime_get_ts(), it will use the timing provided by the hardware.

Just to clarify what Conexant hardware, and my current driver for it, is capable of:

1. It provides raw pulse (and space) width measurements.

2. Those measurements can have very high resoltuion (~37 ns IIRC) or very low resolution (usec or msec IIRC) depending on how the hardware clock dividers are set up.

3. The hardware provides a timeout when the measurment timer overflows, meaning that no edge happened for a very long time. This generates a special "overflow" measurment value and a receiver timeout interrupt.

4. The hardware has an 8 measurement deep FIFO, which the driver starts to drain whenever it is half full (i.e. pulse measurement data is delayed). This happens in response to a hardware FIFO service request interrupt.

5. The hardware FIFO is drained by the driver whenever an interrupt is received and the available measruement data is placed into a kfifo(). This will then signal a work handler that it has work to do.

6. Measurements are scaled to standard time units (i.e. ns) by the driver when they are read out of the kfifo() by a work handler. (No sense in doing time conversions in an interrupt handler).

7. The work handler then begins passing whatever measurements it has, one at a time, over to a pulse stream decoder.

8. If the pulse stream decoder actually decodes something, it is passed over to the input subsystem.

I suspect this device's behavior is much closer to what the MCE-USB device does, than the raw GPIO handlers, but I don't really know the MCE-USB.

Quoted 1 line

The question is though, is the kfifo and work handler really

I had to rework this portion of code several times in the IR code I posted.

I had the core provide input_ir_queue() which was legal to call from interrupt context. Calling from interrupt context was an important aspect I missed in the first versions. I made this a common routine so that the code didn't get copied into all of the drivers. This code should have used kfifo but I didn't know about kfifo.

Quoted 1 line

Quoted 1 line

The question is though, is the kfifo and work handler really

necessary?

Yes, otherwise it will get duplicated into all of the drivers that run in interrupt context like this GPIO one. Put this common code into the core so that the individual drivers writers don't mess it up.

void input_ir_queue(struct input_dev *dev, int sample)

{
	unsigned int next;

	spin_lock(dev->ir->queue.lock);
	dev->ir->queue.samples[dev->ir->queue.head] = sample;
	next = dev->ir->queue.head + 1;
	dev->ir->queue.head = (next >= MAX_SAMPLES ? 0 : next);
	spin_unlock(dev->ir->queue.lock);

	schedule_work(&dev->ir->work);
}

My GPIO implementation simply call input_it_queue() with the timing data. I collapsed multiple long space interrupts into one very long space. If you are using protocol engines, there is no need to detect the long trailing space. The protocol engine will trigger on the last pulse of the signal.

On the other hand, LIRC in user space needs the last long space to know when to flush the buffer from kernel space into user space. The timeout for this flush should be implemented in the LIRC compatibility driver, not ir-core. In this case my GPIO driver doesn't ever generate an event for the long space at the end of the message (because it doesn't end). Instead the LIRC compatibility layer should start a timer and flush when no data has been received for 200ms or whatever.

static irqreturn_t dpeak_ir_irq(int irq, void *_ir)

{
	struct ir_gpt *ir_gpt = _ir;
	int sample, count, delta, bit, wrap;

	sample = in_be32(&ir_gpt->regs->status);
	out_be32(&ir_gpt->regs->status, 0xF);

	count = sample >> 16;
	wrap = (sample >> 12) & 7;
	bit = (sample >> 8) & 1;

	delta = count - ir_gpt->previous;
	delta += wrap * 0x10000;

	ir_gpt->previous = count;

	if (bit)
		delta = -delta;

	input_ir_queue(ir_gpt->input, delta);

	return IRQ_HANDLED;
}

For MSMCE I converted their format back into simple delays and fed it into input_ir_queue(). This was not done in interrupt context because of the way USB works. input_ir_queue() doesn't care - it works correctly when called from either context.

				if (ir->last.command == 0x80) {
					bit = ((ir->buf_in[i] & MCE_PULSE_BIT) != 0);
					delta = (ir->buf_in[i] & MCE_PULSE_MASK) * MCE_TIME_BASE;

					if ((ir->buf_in[i] & MCE_PULSE_MASK) == 0x7f) {
						if (ir->last.bit == bit)
							ir->last.delta += delta;
						else {
							ir->last.delta = delta;
							ir->last.bit = bit;
						}
						continue;
					}
					delta += ir->last.delta;
					ir->last.delta = 0;
					ir->last.bit = bit;

					dev_dbg(&ir->usbdev->dev, "bit %d delta %d\n", bit, delta);
					if (bit)
						delta = -delta;

					input_ir_queue(ir->input, delta);
				}

These delay messages are then fed into the protocol engines which process the pulses in parallel. Processing in parallel works, because that's how IR receivers work. When you shine a remote on an equipment rack, all of the equipment sees the command in parallel. The protocols are designed so that parallel decode works properly.

On the implementation I did, each event is passed to each decoder serialized (yet, as one keystroke is a series of events, it behaves as if they are processed in parallel). We might create separate kthreads for each decoder, and use a spinlock at kfifo, but I suspect that the end result will be very close and we'll have more threads interfering at the samples collect, especially on those (broken) hardware that don't have IRQ's to indicate a state transition, so the driver needs to poll the samples.

--

Cheers,
Mauro

Quoted 46 lines · Andy Walls

Quoted 10 lines · Mauro Carvalho Chehab

I won't comment every single bits of the change, since we're more interested on the conceptual
aspects.

Quoted 1 line

-int ir_raw_event_store(struct input_dev *input_dev, enum raw_event_type type)

Don't remove the raw_event_store. It is needed by the hardware that gets events from IRQ/polling. For sure another interface is needed, for the cases where the hardware pass their own time measure, like cx18 (http://linuxtv.org/hg/~awalls/cx23885-ir2/rev/2cfef53b95a2).

Quoted 5 lines

For those, we need something like:

int ir_raw_event_time_store(struct input_dev *input_dev, enum raw_event_type type, u32 nsecs)

Where, instead of using ktime_get_ts(), it will use the timing provided by the hardware.

Just to clarify what Conexant hardware, and my current driver for it, is capable of:

1. It provides raw pulse (and space) width measurements.

2. Those measurements can have very high resoltuion (~37 ns IIRC) or very low resolution (usec or msec IIRC) depending on how the hardware clock dividers are set up.

3. The hardware provides a timeout when the measurment timer overflows, meaning that no edge happened for a very long time. This generates a special "overflow" measurment value and a receiver timeout interrupt.

4. The hardware has an 8 measurement deep FIFO, which the driver starts to drain whenever it is half full (i.e. pulse measurement data is delayed). This happens in response to a hardware FIFO service request interrupt.

5. The hardware FIFO is drained by the driver whenever an interrupt is received and the available measruement data is placed into a kfifo(). This will then signal a work handler that it has work to do.

6. Measurements are scaled to standard time units (i.e. ns) by the driver when they are read out of the kfifo() by a work handler. (No sense in doing time conversions in an interrupt handler).

7. The work handler then begins passing whatever measurements it has, one at a time, over to a pulse stream decoder.

Quoted 6 lines

8. If the pulse stream decoder actually decodes something, it is passed over to the input subsystem.

I suspect this device's behavior is much closer to what the MCE-USB device does, than the raw GPIO handlers, but I don't really know the MCE-USB.

This sounds very similar to winbond-cir (the hardware parts that is, basically until and including item 5, line 1). The mceusb HW does something similar...it sends usb packets with a couple of pulse/space duration measurements (of 50us resolution IIRC)...and it automatically enters an inactive state after 10000 us of silence.

The ir_raw_event_duration() function of my patch is intended for exactly this kind of hardware (which I mentioned in my reply to Mauro which I just sent out).

The question is though, is the kfifo and work handler really necessary?

Hi David,

Em Tue, 6 Apr 2010 12:48:11 +0200
David Härdeman <[email protected]> escreveu:

Quoted 2 lines

Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline; filename=use-pulse-space-timings-in-ir-raw

Thunderbird 2 really don't like this. It considers the entire body as a file, and refuses to quote it.

Quoted 7 lines

drivers/media/IR/ir-raw-event.c is currently written with the assumption that all "raw" hardware will generate events only on state change (i.e. when a pulse or space starts).

However, some hardware (like mceusb, probably the most popular IR receiver out there) only generates duration data (and that data is buffered so using any kind of timing on the data is futile).

Am I understanding right and this hardware is not capable of indicating if the event is a pulse or a space? It seems hard to auto-detect what is pulse or space, but IMO such code should belong to mceusb driver and not to the decoders.

Based on the code changes you did, I suspect that one of the things the hardware provides is a "machine reset" state, right? If you just need to add a code to reset the state machines, this could be done as easily as adding an event at kfifo with IR_STOP_EVENT. A three line addition at the decoders event handler would be enough to use it to reset the state machine:

	if (ev->type & IR_STOP_EVENT) {
		data->state = STATE_INACTIVE;
		return;
	}

This event were not added yet, since no hardware currently ported needs it. Eventually, we may rename it to IR_RESET_STATE, if you think it is clearer.

Quoted 5 lines

This patch (which is not tested since I haven't yet converted a driver for
any of my hardware to ir-core yet, will do soon) is a RFC on my proposed interface change...once I get the green light on the interface change itself I'll make sure that the decoders actually work :)

Yes, better to discuss before changing everything ;)

Quoted 3 lines

The rc5 decoder has also gained rc5x support and the use of kfifo's for
intermediate storage is gone (since there is no need for it).

The RC-5X addition is welcome, but the better is to add it as a separate patch.

I won't comment every single bits of the change, since we're more interested on the conceptual aspects.

Quoted 1 line

-int ir_raw_event_store(struct input_dev *input_dev, enum raw_event_type type)

Don't remove the raw_event_store. It is needed by the hardware that gets events from IRQ/polling.

For sure another interface is needed, for the cases where the hardware
pass their
own time measure, like cx18 (http://linuxtv.org/hg/~awalls/cx23885-ir2/rev/2cfef53b95a2).

For those, we need something like:

int ir_raw_event_time_store(struct input_dev *input_dev, enum raw_event_type type, u32 nsecs)

Where, instead of using ktime_get_ts(), it will use the timing provided by the hardware.

Quoted 22 lines

drivers/media/IR/ir-raw-event.c +11 -9

-int ir_raw_event_handle(struct input_dev *input_dev)
+/**
+ * ir_raw_event_edge() - notify raw ir decoders of the start of a pulse/space
+ * @input_dev:	the struct input_dev device descriptor
+ * @type:	the type of the event that has occurred
+ *
+ * This routine is used to notify the raw ir decoders on the beginning of an
+ * ir pulse or space (or the start/end of ir reception). This is used by
+ * hardware which does not provide durations directly but only interrupts
+ * (or similar events) on state change.
+ */
+void ir_raw_event_edge(struct input_dev *input_dev, enum raw_event_type type)
 {
-	struct ir_input_dev		*ir = input_get_drvdata(input_dev);
-	int				rc;
-	struct ir_raw_event		ev;
-	int 				len, i;
-
-	/*
-	 * Store the events into a temporary buffer. This allows calling more than
-	 * one decoder to deal with the received data

drivers/media/IR/ir-raw-event.c line 150

Quoted 4 lines

drivers/media/IR/ir-raw-event.c +0 -4

-	 */
-	len = kfifo_len(&ir->raw->kfifo) / sizeof(ev);
-	if (!len)
-		return 0;

The removal of kfifo is not a good idea. On several drivers, the event is generated during IRQ time, or on a very expensive polling loop. So, buffering is needed to release the IRQ as soon as possible and not adding too much processing during polling.

Quoted 19 lines

drivers/media/IR/ir-raw-event.c +6 -12

-
-	for (i = 0; i < len; i++) {
-		rc = kfifo_out(&ir->raw->kfifo, &ev, sizeof(ev));
-		if (rc != sizeof(ev)) {
-			IR_dprintk(1, "overflow error: received %d instead of %zd\n",
-				   rc, sizeof(ev));
-			return -EINVAL;
-		}
-		IR_dprintk(2, "event type %d, time before event: %07luus\n",
-			ev.type, (ev.delta.tv_nsec + 500) / 1000);
-		rc = RUN_DECODER(decode, input_dev, &ev);
-	}
+	struct ir_input_dev	*ir = input_get_drvdata(input_dev);
+	ktime_t			now;
+	s64			delta; /* us */
+
+	if (!ir->raw)
+		return;

drivers/media/IR/ir-raw-event.c line 141

Quoted 6 lines

drivers/media/IR/ir-raw-event.c +2 -4

-	/*
-	 * Call all ir decoders. This allows decoding the same event with
-	 * more than one protocol handler.
-	 */
+	now = ktime_get();
+	delta = ktime_us_delta(now, ir->raw->last_event);

This won't work, in the cases where the hardware is providing its own timings.

drivers/media/IR/ir-raw-event.c line 146

Quoted 6 lines

drivers/media/IR/ir-raw-event.c +4 -1

-	return rc;
+	/* Check for a long duration since last event or if we're
+	   being called for the first time */
+	if (delta > USEC_PER_SEC || !ir->raw->last_type)
+		type |= IR_START_EVENT;

The "long duration" concept would be better implemented at the driver, since it may vary with the IR carrier and with the protocol details.

drivers/media/IR/ir-raw-event.c line 153

Quoted 7 lines

drivers/media/IR/ir-raw-event.c +7 -0

+
+	if (type & IR_START_EVENT)
+		ir_raw_event_reset(input_dev);
+	else if (ir->raw->last_type & IR_SPACE)
+		ir_raw_event_duration(input_dev, (int)-delta);
+	else if (ir->raw->last_type & IR_PULSE)
+		ir_raw_event_duration(input_dev, (int)delta);

Please, don't use a signal to identify between pulse and space. The IR decoding logic is tricky enough. Just pass the type to the decoder and let it explicitly check if it is pulse or space.

Quoted 6 lines

Index: ir/drivers/media/IR/ir-nec-decoder.c
===================================================================

drivers/media/IR/ir-nec-decoder.c +0 -0

@@ -14,22 +14,25 @@
 

drivers/media/IR/ir-nec-decoder.c line 20

Quoted 6 lines

drivers/media/IR/ir-nec-decoder.c +4 -0

 #include <media/ir-core.h>
 
+/*
+ * Regarding NEC_HEADER_MARK: some NEC remotes use 16, some 8,
+ * some receivers are also good at missing part of the first pulse.
+ */

The NEC decoder improvements should be on a separate patch.

drivers/media/IR/ir-nec-decoder.c line 22

Quoted 9 lines

drivers/media/IR/ir-nec-decoder.c +1 -7

 #define NEC_NBITS		32
-#define NEC_UNIT		559979 /* ns */
-#define NEC_HEADER_MARK		(16 * NEC_UNIT)
-#define NEC_HEADER_SPACE	(8 * NEC_UNIT)
-#define NEC_REPEAT_SPACE	(4 * NEC_UNIT)
-#define NEC_MARK		(NEC_UNIT)
-#define NEC_0_SPACE		(NEC_UNIT)
-#define NEC_1_SPACE		(3 * NEC_UNIT)
+#define NEC_UNIT		562	/* us */

Why changing to microseconds? ktime_t also handles time on nanosseconds.

drivers/media/IR/ir-nec-decoder.c line 28

Quoted 6 lines

drivers/media/IR/ir-nec-decoder.c +6 -0

+#define NEC_HEADER_MARK		6
+#define NEC_HEADER_SPACE	-8
+#define NEC_REPEAT_SPACE	-4
+#define NEC_MARK		1
+#define NEC_0_SPACE		-1
+#define NEC_1_SPACE		-3

Those negative values here is really weird... I can't stop thinking on time travels, when I see a negative time ;)

Seriously, this change obfuscates the logic, as it is using the mantissa of a number to indicate a time duration, and the signal to indicate the presence or absence of a carrier. Encoding two different measures into a number is not a good thing. I used to do such tricks when programming in Z80 assembly, back on eighties, basically due to the absolute lack of enough memory on those machines with a few kilobytes of RAM. After a few weeks, returning back to the same code to fix were really hard. We don't have such memory constraints anymore. So, let's keep the code as clearer as possible.

I've stripped the decoders code, since they're basically implementing the architectural
changes you're proposing Let's first finish the discussions about the changes.

Btw, I noticed that you've added some improvements to the decoders, like the changes to support RC-5X. The better is to send it as a separate patch, due to a few reasons:

- RC-5X addition has nothing to do with "Teach drivers/media/IR/ir-raw-event.c
to use durations" (the subject of this RFC patch);

- Bigger patches have more chances of getting nacked, since they touch on more parts
of the code. So, you'll need to rework more code;

- The addition of RC-5X shouldn't break RC-5. By having it as a separate patch,
it is easier to test the changes;

- If later discovered a regression, it would be easier to bisect and see if the
changes were introduced by RC-5X or by the architectural changes, if the changes are broken into two patches.

drivers/media/IR/ir-nec-decoder.c line 25

Quoted 1 line

drivers/media/IR/ir-nec-decoder.c +0 -1

-static unsigned int ir_rc5_remote_gap = 888888;

The idea of static int is that, on saa7134, this value can be adjustable from userspace. probably, some hardware use a non-standard carrier, so we'll need to export it also via sysfs, to avoid regressions.

Quoted 5 lines · Mauro Carvalho Chehab

Hi David,

Em Tue, 6 Apr 2010 12:48:11 +0200
David Härdeman <[email protected]> escreveu:

Quoted 4 lines

Quoted 2 lines

Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline; filename=use-pulse-space-timings-in-ir-raw

Thunderbird 2 really don't like this. It considers the entire body as a file, and refuses to quote it.

Never had people complain when I use quilt before but I'll see what I can do.

Quoted 10 lines

Quoted 7 lines

drivers/media/IR/ir-raw-event.c is currently written with the assumption that all "raw" hardware will generate events only on state change (i.e. when a pulse or space starts).

However, some hardware (like mceusb, probably the most popular IR receiver out there) only generates duration data (and that data is buffered so using any kind of timing on the data is futile).

Am I understanding right and this hardware is not capable of indicating if the event is a pulse or a space? It seems hard to auto-detect what is pulse or space, but IMO such code should belong to mceusb driver and not to the decoders.

No, the driver for mceusb sends a usb packet which contains a couple of pulse/space durations in the form of signed integers representing pulse (positive) and space (negative) durations in microseconds. It's a pretty common arrangement. winbond-cir also has a mode (which is the one I'm planning on using in the future) where pulse/space durations are accumulated in the UART buffer and an IRQ is generated once the buffer level reaches a threshold.

Quoted 6 lines

Based on the code changes you did, I suspect that one of the things the hardware provides is a "machine reset" state, right? If you just need to add a code to reset the state machines, this could be done as easily as adding an event at kfifo with IR_STOP_EVENT. A three line addition at the decoders event handler would be enough to use it to reset the state machine:

Quoted 7 lines

	if (ev->type & IR_STOP_EVENT) {
		data->state = STATE_INACTIVE;
		return;
	}

This event were not added yet, since no hardware currently ported needs it. Eventually, we may rename it to IR_RESET_STATE, if you think it is clearer.

Not a particular state per se, I just added a function which the hardware can use to reset the state machines when necessary (think hardware reset, suspend/resume, switching from RX to TX and back again, etc).

I think this:

	/* Hardware has been reset, notify ir-core */
	ir_raw_event_reset(input_dev);

is a hell lot clearer than this (your current code):

	/* Hardware has been reset, notify ir-core */
	rc = ir_raw_event_store(input_dev, IR_STOP_EVENT);
	if (rc) {
		/* Uh oh, what do we do now? */
		...

	}
	rc = ir_raw_event_handle(input_dev);
	if (rc) {
		/* Not again... */
		...
	}

Quoted 10 lines

Quoted 5 lines

This patch (which is not tested since I haven't yet converted a driver for
any of my hardware to ir-core yet, will do soon) is a RFC on my proposed interface change...once I get the green light on the interface change itself I'll make sure that the decoders actually work :)

Yes, better to discuss before changing everything ;)

Quoted 3 lines

The rc5 decoder has also gained rc5x support and the use of kfifo's for
intermediate storage is gone (since there is no need for it).

Quoted 1 line

The RC-5X addition is welcome, but the better is to add it as a separate patch.

Using durations (instead of a combination of struct timespec and enum raw_event_type) as the argument to the decoder necessitates rewriting most of the decoders, so it seemed like a good time to add it. RC5X or not will anyway only mean a couple of lines of difference but I can send it as a separate patch if that helps you.

Quoted 6 lines

I won't comment every single bits of the change, since we're more interested on the conceptual aspects.

Quoted 1 line

-int ir_raw_event_store(struct input_dev *input_dev, enum raw_event_type type)

Don't remove the raw_event_store. It is needed by the hardware that gets events from IRQ/polling.

See the comments for kfifo below.

Quoted 4 lines

For sure another interface is needed, for the cases where the hardware
pass their
own time measure, like cx18 (http://linuxtv.org/hg/~awalls/cx23885-ir2/rev/2cfef53b95a2).

Quoted 5 lines

For those, we need something like:

int ir_raw_event_time_store(struct input_dev *input_dev, enum raw_event_type type, u32 nsecs)

Where, instead of using ktime_get_ts(), it will use the timing provided by the hardware.

Um, this sounds exactly like ir_raw_event_duration() which was the main point of my patch.

Quoted 20 lines

Quoted 20 lines

drivers/media/IR/ir-raw-event.c +11 -8

-int ir_raw_event_handle(struct input_dev *input_dev)
+/**
+ * ir_raw_event_edge() - notify raw ir decoders of the start of a pulse/space
+ * @input_dev:	the struct input_dev device descriptor
+ * @type:	the type of the event that has occurred
+ *
+ * This routine is used to notify the raw ir decoders on the beginning of an
+ * ir pulse or space (or the start/end of ir reception). This is used by
+ * hardware which does not provide durations directly but only interrupts
+ * (or similar events) on state change.
+ */
+void ir_raw_event_edge(struct input_dev *input_dev, enum raw_event_type type)
 {
-	struct ir_input_dev		*ir = input_get_drvdata(input_dev);
-	int				rc;
-	struct ir_raw_event		ev;
-	int 				len, i;
-
-	/*
-	 * Store the events into a temporary buffer. This allows calling more than

Quoted 8 lines

drivers/media/IR/ir-raw-event.c line 150

Quoted 5 lines

drivers/media/IR/ir-raw-event.c +0 -5

-	 * one decoder to deal with the received data
-	 */
-	len = kfifo_len(&ir->raw->kfifo) / sizeof(ev);
-	if (!len)
-		return 0;

The removal of kfifo is not a good idea. On several drivers, the event is generated during IRQ time, or on a very expensive polling loop. So, buffering is needed to release the IRQ as soon as possible and not adding too much processing during polling.

Have you seen any real case where this is a problem or is this just conjecture on your behalf? I've written ir decoders for embedded hardware which pass the "event" (duration) through the state machines directly and it works great on hardware with a fraction of the computing power compared to the machines you're using.

The state machines shouldn't have to do much more than rounding of the duration followed by a couple of integer comparisons (and possibly some bitops). I fail to see how using a kfifo would provide any real improvement.

Quoted 19 lines

Quoted 19 lines

drivers/media/IR/ir-raw-event.c +6 -12

-
-	for (i = 0; i < len; i++) {
-		rc = kfifo_out(&ir->raw->kfifo, &ev, sizeof(ev));
-		if (rc != sizeof(ev)) {
-			IR_dprintk(1, "overflow error: received %d instead of %zd\n",
-				   rc, sizeof(ev));
-			return -EINVAL;
-		}
-		IR_dprintk(2, "event type %d, time before event: %07luus\n",
-			ev.type, (ev.delta.tv_nsec + 500) / 1000);
-		rc = RUN_DECODER(decode, input_dev, &ev);
-	}
+	struct ir_input_dev	*ir = input_get_drvdata(input_dev);
+	ktime_t			now;
+	s64			delta; /* us */
+
+	if (!ir->raw)
+		return;

Quoted 9 lines

drivers/media/IR/ir-raw-event.c line 141

Quoted 6 lines

drivers/media/IR/ir-raw-event.c +2 -4

-	/*
-	 * Call all ir decoders. This allows decoding the same event with
-	 * more than one protocol handler.
-	 */
+	now = ktime_get();
+	delta = ktime_us_delta(now, ir->raw->last_event);

This won't work, in the cases where the hardware is providing its own timings.

Again, see ir_raw_event_duration()

Quoted 8 lines

drivers/media/IR/ir-raw-event.c line 146

Quoted 6 lines

drivers/media/IR/ir-raw-event.c +4 -1

-	return rc;
+	/* Check for a long duration since last event or if we're
+	   being called for the first time */
+	if (delta > USEC_PER_SEC || !ir->raw->last_type)
+		type |= IR_START_EVENT;

The "long duration" concept would be better implemented at the driver, since it may vary with the IR carrier and with the protocol details.

Now you're criticizing your own code...this was one of the few concepts I carried over from your original code. Specifically, ir-raw-event.c, lines 116 - 129 from your current tree:

	ktime_get_ts(&ts);

	if (timespec_equal(&ir->raw->last_event, &event.delta))
		event.type |= IR_START_EVENT;
	else
		event.delta = timespec_sub(ts, ir->raw->last_event);

	memcpy(&ir->raw->last_event, &ts, sizeof(ts));

	if (event.delta.tv_sec) {
		event.type |= IR_START_EVENT;
		event.delta.tv_sec = 0;
		event.delta.tv_nsec = 0;
	}

(Note the check on event.delta.tv_sec)

Quoted 10 lines

drivers/media/IR/ir-raw-event.c line 153

Quoted 7 lines

drivers/media/IR/ir-raw-event.c +7 -0

+
+	if (type & IR_START_EVENT)
+		ir_raw_event_reset(input_dev);
+	else if (ir->raw->last_type & IR_SPACE)
+		ir_raw_event_duration(input_dev, (int)-delta);
+	else if (ir->raw->last_type & IR_PULSE)
+		ir_raw_event_duration(input_dev, (int)delta);

Please, don't use a signal to identify between pulse and space. The IR decoding logic is tricky enough. Just pass the type to the decoder and let it explicitly check if it is pulse or space.

No idea what you mean here. Why would it be clearer to put the equivalent code in every single decoder instead of adding it once to the place which calls the decoders?

Quoted 6 lines

Quoted 6 lines

Index: ir/drivers/media/IR/ir-nec-decoder.c
===================================================================

drivers/media/IR/ir-nec-decoder.c +0 -0

@@ -14,22 +14,25 @@
 

Quoted 7 lines

drivers/media/IR/ir-nec-decoder.c line 20

Quoted 6 lines

drivers/media/IR/ir-nec-decoder.c +4 -0

 #include <media/ir-core.h>
 
+/*
+ * Regarding NEC_HEADER_MARK: some NEC remotes use 16, some 8,
+ * some receivers are also good at missing part of the first pulse.
+ */

The NEC decoder improvements should be on a separate patch.

They can't since the entire patch is about changing the core raw logic over to primarily use durations, which means the existing nec and rc5 decoders need to change in the same patch or bisectability will be broken.

Quoted 10 lines

drivers/media/IR/ir-nec-decoder.c line 22

Quoted 9 lines

drivers/media/IR/ir-nec-decoder.c +1 -7

 #define NEC_NBITS		32
-#define NEC_UNIT		559979 /* ns */
-#define NEC_HEADER_MARK		(16 * NEC_UNIT)
-#define NEC_HEADER_SPACE	(8 * NEC_UNIT)
-#define NEC_REPEAT_SPACE	(4 * NEC_UNIT)
-#define NEC_MARK		(NEC_UNIT)
-#define NEC_0_SPACE		(NEC_UNIT)
-#define NEC_1_SPACE		(3 * NEC_UNIT)
+#define NEC_UNIT		562	/* us */

Why changing to microseconds? ktime_t also handles time on nanosseconds.

Interesting question but you managed to turn it on its head. Why not use femtoseconds? Because it's complete and utter overkill. Not a single IR protocol needs that kind of precision. Both LIRC and the Microsoft IR API's are based on microseconds. Programmable IR hardware (like the Philips Pronto) is based on microseconds. The proper question to ask is why you'd use nanoseconds...

Quoted 9 lines

drivers/media/IR/ir-nec-decoder.c line 28

Quoted 7 lines

drivers/media/IR/ir-nec-decoder.c +6 -0

+#define NEC_HEADER_MARK		6
+#define NEC_HEADER_SPACE	-8
+#define NEC_REPEAT_SPACE	-4
+#define NEC_MARK		1
+#define NEC_0_SPACE		-1
+#define NEC_1_SPACE		-3

Those negative values here is really weird... I can't stop thinking on time travels, when I see a negative time ;)

Better get used to it, if you're going to maintain ir-core you're going to see it in mailing list discussions once ir-core gains a userbase.

Quoted 7 lines

Seriously, this change obfuscates the logic, as it is using the mantissa of a number to indicate a time duration, and the signal to indicate the presence or absence of a carrier. Encoding two different measures into a number is not a good thing. I used to do such tricks when programming in Z80 assembly, back on eighties, basically due to the absolute lack of enough memory on those machines with a few kilobytes of RAM. After a few weeks, returning back to the same code to fix were really hard. We don't have such memory constraints anymore. So, let's keep the code as clearer as possible.

Describing a received ir signal as a number of signed integers describing the duration of pulses (positive) and spaces (negative) in microseconds is pretty much standard (to the extent that any standard exists) in any discussion of IR protocols.

The decoders Jon Smirl implemented used signed integers, the decoders I've implemented use signed integers. Microsoft's IR API uses signed integers. LIRC uses signed integers (kinda, 23 bits of microsecond duration, one bit for pulse or space - as far as I can remember from LIRC_MODE_MODE2). Other projects also use it (e.g. the developers of decodeir.dll which is probably one of the most used IR API's on the MS platform outside of Microsoft's own API).

Instead you want to go with a model where you pass in total three arguments to the decoders (struct timespec with nsec and sec + enum type). I do not understand how you would consider this clearer or better.

Quoted 16 lines

I've stripped the decoders code, since they're basically implementing the architectural
changes you're proposing Let's first finish the discussions about the changes.

Btw, I noticed that you've added some improvements to the decoders, like the changes to support RC-5X. The better is to send it as a separate patch, due to a few reasons:

- RC-5X addition has nothing to do with "Teach drivers/media/IR/ir-raw-event.c
to use durations" (the subject of this RFC patch);

- Bigger patches have more chances of getting nacked, since they touch on more parts
of the code. So, you'll need to rework more code;

- The addition of RC-5X shouldn't break RC-5. By having it as a separate patch,
it is easier to test the changes;

Quoted 4 lines

- If later discovered a regression, it would be easier to bisect and see if the
changes were introduced by RC-5X or by the architectural changes, if the changes are broken into two patches.

I've already addressed RC5X above.

Quoted 4 lines

drivers/media/IR/ir-nec-decoder.c line 25

Quoted 1 line

drivers/media/IR/ir-nec-decoder.c +0 -1

-static unsigned int ir_rc5_remote_gap = 888888;

The idea of static int is that, on saa7134, this value can be adjustable from userspace. probably, some hardware use a non-standard carrier, so we'll need to export it also via sysfs, to avoid regressions.

The decoders in my patch have a +/-50% tolerance for pulse/space unit durations, if it turns out to be insufficient, then it's time to look at solutions, doing it now is premature.