All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] ir-core: one-line bugfix, add RC5x, NECx, RC6
@ 2010-04-08 23:04 David Härdeman
  2010-04-08 23:04   ` David Härdeman
                   ` (3 more replies)
  0 siblings, 4 replies; 19+ messages in thread
From: David Härdeman @ 2010-04-08 23:04 UTC (permalink / raw)
  To: mchehab; +Cc: linux-input, linux-media

The following series implements support for a couple of new protocols
now that we finally reached an agreement on how durations should be
handled in ir-raw-event.

Also included is a one-line fix that got missed in my previous conversion
of drivers/media/dvb/ttpci/budget-ci.c.

(not to mention that this is my first test run with stgit)

---

David Härdeman (4):
      Fix the drivers/media/dvb/ttpci/budget-ci.c conversion to ir-core
      Add RC5x support to ir-core
      Add NECx support to ir-core
      Add RC6 support to ir-core


 drivers/media/IR/Kconfig            |    9 +
 drivers/media/IR/Makefile           |    1 
 drivers/media/IR/ir-core-priv.h     |    7 +
 drivers/media/IR/ir-nec-decoder.c   |    3 
 drivers/media/IR/ir-raw-event.c     |    1 
 drivers/media/IR/ir-rc5-decoder.c   |   78 +++++--
 drivers/media/IR/ir-rc6-decoder.c   |  412 +++++++++++++++++++++++++++++++++++
 drivers/media/IR/ir-sysfs.c         |    2 
 drivers/media/dvb/ttpci/budget-ci.c |    2 
 include/media/rc-map.h              |    1 
 10 files changed, 494 insertions(+), 22 deletions(-)
 create mode 100644 drivers/media/IR/ir-rc6-decoder.c


^ permalink raw reply	[flat|nested] 19+ messages in thread
* [PATCH 1/4] Fix the drivers/media/dvb/ttpci/budget-ci.c conversion to ir-core
  2010-04-08 23:04 [PATCH 0/4] ir-core: one-line bugfix, add RC5x, NECx, RC6 David Härdeman
@ 2010-04-08 23:04   ` David Härdeman
  2010-04-08 23:04   ` David Härdeman
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 19+ messages in thread
From: David Härdeman @ 2010-04-08 23:04 UTC (permalink / raw)
  To: mchehab; +Cc: linux-input, linux-media

When I converted drivers/media/dvb/ttpci/budget-ci.c to use ir-core
I missed one line. This patch fixes that mistake.

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/dvb/ttpci/budget-ci.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/media/dvb/ttpci/budget-ci.c b/drivers/media/dvb/ttpci/budget-ci.c
index 8950df1..4617143 100644
--- a/drivers/media/dvb/ttpci/budget-ci.c
+++ b/drivers/media/dvb/ttpci/budget-ci.c
@@ -225,8 +225,6 @@ static int msp430_ir_init(struct budget_ci *budget_ci)
 		break;
 	}
 
-	ir_input_init(input_dev, &budget_ci->ir.state, IR_TYPE_RC5);
-
 	error = ir_input_register(input_dev, ir_codes, NULL, MODULE_NAME);
 	if (error) {
 		printk(KERN_ERR "budget_ci: could not init driver for IR device (code %d)\n", error);


^ permalink raw reply related	[flat|nested] 19+ messages in thread
* [PATCH 1/4] Fix the drivers/media/dvb/ttpci/budget-ci.c conversion to ir-core
@ 2010-04-08 23:04   ` David Härdeman
  0 siblings, 0 replies; 19+ messages in thread
From: David Härdeman @ 2010-04-08 23:04 UTC (permalink / raw)
  To: mchehab; +Cc: linux-input, linux-media

When I converted drivers/media/dvb/ttpci/budget-ci.c to use ir-core
I missed one line. This patch fixes that mistake.

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/dvb/ttpci/budget-ci.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/media/dvb/ttpci/budget-ci.c b/drivers/media/dvb/ttpci/budget-ci.c
index 8950df1..4617143 100644
--- a/drivers/media/dvb/ttpci/budget-ci.c
+++ b/drivers/media/dvb/ttpci/budget-ci.c
@@ -225,8 +225,6 @@ static int msp430_ir_init(struct budget_ci *budget_ci)
 		break;
 	}
 
-	ir_input_init(input_dev, &budget_ci->ir.state, IR_TYPE_RC5);
-
 	error = ir_input_register(input_dev, ir_codes, NULL, MODULE_NAME);
 	if (error) {
 		printk(KERN_ERR "budget_ci: could not init driver for IR device (code %d)\n", error);

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 19+ messages in thread
* [PATCH 2/4] Add RC5x support to ir-core
  2010-04-08 23:04 [PATCH 0/4] ir-core: one-line bugfix, add RC5x, NECx, RC6 David Härdeman
@ 2010-04-08 23:04   ` David Härdeman
  2010-04-08 23:04   ` David Härdeman
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 19+ messages in thread
From: David Härdeman @ 2010-04-08 23:04 UTC (permalink / raw)
  To: mchehab; +Cc: linux-input, linux-media

This patch adds RC5x support to drivers/media/IR/ir-rc5-decoder.c

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/IR/ir-rc5-decoder.c |   78 ++++++++++++++++++++++++++++---------
 1 files changed, 59 insertions(+), 19 deletions(-)

diff --git a/drivers/media/IR/ir-rc5-decoder.c b/drivers/media/IR/ir-rc5-decoder.c
index 59bcaa9..c970ab2 100644
--- a/drivers/media/IR/ir-rc5-decoder.c
+++ b/drivers/media/IR/ir-rc5-decoder.c
@@ -1,4 +1,4 @@
-/* ir-rc5-decoder.c - handle RC-5 IR Pulse/Space protocol
+/* ir-rc5-decoder.c - handle RC5(x) IR Pulse/Space protocol
  *
  * Copyright (C) 2010 by Mauro Carvalho Chehab <mchehab@redhat.com>
  *
@@ -13,15 +13,19 @@
  */
 
 /*
- * This code only handles 14 bits RC-5 protocols. There are other variants
- * that use a different number of bits. This is currently unsupported
- * It considers a carrier of 36 kHz, with a total of 14 bits, where
+ * This code handles 14 bits RC5 protocols and 20 bits RC5x protocols.
+ * There are other variants that use a different number of bits.
+ * This is currently unsupported.
+ * It considers a carrier of 36 kHz, with a total of 14/20 bits, where
  * the first two bits are start bits, and a third one is a filing bit
  */
 
 #include "ir-core-priv.h"
 
 #define RC5_NBITS		14
+#define RC5X_NBITS		20
+#define CHECK_RC5X_NBITS	8
+#define RC5X_SPACE		SPACE(4)
 #define RC5_UNIT		888888 /* ns */
 
 /* Used to register rc5_decoder clients */
@@ -32,6 +36,7 @@ enum rc5_state {
 	STATE_INACTIVE,
 	STATE_BIT_START,
 	STATE_BIT_END,
+	STATE_CHECK_RC5X,
 	STATE_FINISHED,
 };
 
@@ -45,6 +50,7 @@ struct decoder_data {
 	u32			rc5_bits;
 	int			last_unit;
 	unsigned		count;
+	unsigned		wanted_bits;
 };
 
 
@@ -126,7 +132,7 @@ static int ir_rc5_decode(struct input_dev *input_dev, s64 duration)
 {
 	struct decoder_data *data;
 	struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
-	u8 command, system, toggle;
+	u8 toggle;
 	u32 scancode;
 	int u;
 
@@ -147,7 +153,7 @@ static int ir_rc5_decode(struct input_dev *input_dev, s64 duration)
 		goto out;
 
 again:
-	IR_dprintk(2, "RC5 decode started at state %i (%i units, %ius)\n",
+	IR_dprintk(2, "RC5(x) decode started at state %i (%i units, %ius)\n",
 		   data->state, u, TO_US(duration));
 
 	if (DURATION(u) == 0 && data->state != STATE_FINISHED)
@@ -159,6 +165,8 @@ again:
 		if (IS_PULSE(u)) {
 			data->state = STATE_BIT_START;
 			data->count = 1;
+			/* We just need enough bits to get to STATE_CHECK_RC5X */
+			data->wanted_bits = RC5X_NBITS;
 			DECREASE_DURATION(u, 1);
 			goto again;
 		}
@@ -176,7 +184,7 @@ again:
 			 * If the last bit is zero, a space will merge
 			 * with the silence after the command.
 			 */
-			if (IS_PULSE(u) && data->count == RC5_NBITS) {
+			if (IS_PULSE(u) && data->count == data->wanted_bits) {
 				data->state = STATE_FINISHED;
 				goto again;
 			}
@@ -188,8 +196,10 @@ again:
 
 	case STATE_BIT_END:
 		if (IS_TRANSITION(u, data->last_unit)) {
-			if (data->count == RC5_NBITS)
+			if (data->count == data->wanted_bits)
 				data->state = STATE_FINISHED;
+			else if (data->count == CHECK_RC5X_NBITS)
+				data->state = STATE_CHECK_RC5X;
 			else
 				data->state = STATE_BIT_START;
 
@@ -198,22 +208,52 @@ again:
 		}
 		break;
 
+	case STATE_CHECK_RC5X:
+		if (IS_SPACE(u) && DURATION(u) >= DURATION(RC5X_SPACE)) {
+			/* RC5X */
+			data->wanted_bits = RC5X_NBITS;
+			DECREASE_DURATION(u, DURATION(RC5X_SPACE));
+		} else {
+			/* RC5 */
+			data->wanted_bits = RC5_NBITS;
+		}
+		data->state = STATE_BIT_START;
+		goto again;
+
 	case STATE_FINISHED:
-		command  = (data->rc5_bits & 0x0003F) >> 0;
-		system   = (data->rc5_bits & 0x007C0) >> 6;
-		toggle   = (data->rc5_bits & 0x00800) ? 1 : 0;
-		command += (data->rc5_bits & 0x01000) ? 0 : 0x40;
-		scancode = system << 8 | command;
-
-		IR_dprintk(1, "RC5 scancode 0x%04x (toggle: %u)\n",
-			   scancode, toggle);
+		if (data->wanted_bits == RC5X_NBITS) {
+			/* RC5X */
+			u8 xdata, command, system;
+			xdata    = (data->rc5_bits & 0x0003F) >> 0;
+			command  = (data->rc5_bits & 0x00FC0) >> 6;
+			system   = (data->rc5_bits & 0x1F000) >> 12;
+			toggle   = (data->rc5_bits & 0x20000) ? 1 : 0;
+			command += (data->rc5_bits & 0x01000) ? 0 : 0x40;
+			scancode = system << 16 | command << 8 | xdata;
+
+			IR_dprintk(1, "RC5X scancode 0x%06x (toggle: %u)\n",
+				   scancode, toggle);
+
+		} else {
+			/* RC5 */
+			u8 command, system;
+			command  = (data->rc5_bits & 0x0003F) >> 0;
+			system   = (data->rc5_bits & 0x007C0) >> 6;
+			toggle   = (data->rc5_bits & 0x00800) ? 1 : 0;
+			command += (data->rc5_bits & 0x01000) ? 0 : 0x40;
+			scancode = system << 8 | command;
+
+			IR_dprintk(1, "RC5 scancode 0x%04x (toggle: %u)\n",
+				   scancode, toggle);
+		}
+
 		ir_keydown(input_dev, scancode, toggle);
 		data->state = STATE_INACTIVE;
 		return 0;
 	}
 
 out:
-	IR_dprintk(1, "RC5 decode failed at state %i (%i units, %ius)\n",
+	IR_dprintk(1, "RC5(x) decode failed at state %i (%i units, %ius)\n",
 		   data->state, u, TO_US(duration));
 	data->state = STATE_INACTIVE;
 	return -EINVAL;
@@ -273,7 +313,7 @@ static int __init ir_rc5_decode_init(void)
 {
 	ir_raw_handler_register(&rc5_handler);
 
-	printk(KERN_INFO "IR RC-5 protocol handler initialized\n");
+	printk(KERN_INFO "IR RC5(x) protocol handler initialized\n");
 	return 0;
 }
 
@@ -288,4 +328,4 @@ module_exit(ir_rc5_decode_exit);
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Mauro Carvalho Chehab <mchehab@redhat.com>");
 MODULE_AUTHOR("Red Hat Inc. (http://www.redhat.com)");
-MODULE_DESCRIPTION("RC-5 IR protocol decoder");
+MODULE_DESCRIPTION("RC5(x) IR protocol decoder");


^ permalink raw reply related	[flat|nested] 19+ messages in thread
* [PATCH 2/4] Add RC5x support to ir-core
@ 2010-04-08 23:04   ` David Härdeman
  0 siblings, 0 replies; 19+ messages in thread
From: David Härdeman @ 2010-04-08 23:04 UTC (permalink / raw)
  To: mchehab; +Cc: linux-input, linux-media

This patch adds RC5x support to drivers/media/IR/ir-rc5-decoder.c

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/IR/ir-rc5-decoder.c |   78 ++++++++++++++++++++++++++++---------
 1 files changed, 59 insertions(+), 19 deletions(-)

diff --git a/drivers/media/IR/ir-rc5-decoder.c b/drivers/media/IR/ir-rc5-decoder.c
index 59bcaa9..c970ab2 100644
--- a/drivers/media/IR/ir-rc5-decoder.c
+++ b/drivers/media/IR/ir-rc5-decoder.c
@@ -1,4 +1,4 @@
-/* ir-rc5-decoder.c - handle RC-5 IR Pulse/Space protocol
+/* ir-rc5-decoder.c - handle RC5(x) IR Pulse/Space protocol
  *
  * Copyright (C) 2010 by Mauro Carvalho Chehab <mchehab@redhat.com>
  *
@@ -13,15 +13,19 @@
  */
 
 /*
- * This code only handles 14 bits RC-5 protocols. There are other variants
- * that use a different number of bits. This is currently unsupported
- * It considers a carrier of 36 kHz, with a total of 14 bits, where
+ * This code handles 14 bits RC5 protocols and 20 bits RC5x protocols.
+ * There are other variants that use a different number of bits.
+ * This is currently unsupported.
+ * It considers a carrier of 36 kHz, with a total of 14/20 bits, where
  * the first two bits are start bits, and a third one is a filing bit
  */
 
 #include "ir-core-priv.h"
 
 #define RC5_NBITS		14
+#define RC5X_NBITS		20
+#define CHECK_RC5X_NBITS	8
+#define RC5X_SPACE		SPACE(4)
 #define RC5_UNIT		888888 /* ns */
 
 /* Used to register rc5_decoder clients */
@@ -32,6 +36,7 @@ enum rc5_state {
 	STATE_INACTIVE,
 	STATE_BIT_START,
 	STATE_BIT_END,
+	STATE_CHECK_RC5X,
 	STATE_FINISHED,
 };
 
@@ -45,6 +50,7 @@ struct decoder_data {
 	u32			rc5_bits;
 	int			last_unit;
 	unsigned		count;
+	unsigned		wanted_bits;
 };
 
 
@@ -126,7 +132,7 @@ static int ir_rc5_decode(struct input_dev *input_dev, s64 duration)
 {
 	struct decoder_data *data;
 	struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
-	u8 command, system, toggle;
+	u8 toggle;
 	u32 scancode;
 	int u;
 
@@ -147,7 +153,7 @@ static int ir_rc5_decode(struct input_dev *input_dev, s64 duration)
 		goto out;
 
 again:
-	IR_dprintk(2, "RC5 decode started at state %i (%i units, %ius)\n",
+	IR_dprintk(2, "RC5(x) decode started at state %i (%i units, %ius)\n",
 		   data->state, u, TO_US(duration));
 
 	if (DURATION(u) == 0 && data->state != STATE_FINISHED)
@@ -159,6 +165,8 @@ again:
 		if (IS_PULSE(u)) {
 			data->state = STATE_BIT_START;
 			data->count = 1;
+			/* We just need enough bits to get to STATE_CHECK_RC5X */
+			data->wanted_bits = RC5X_NBITS;
 			DECREASE_DURATION(u, 1);
 			goto again;
 		}
@@ -176,7 +184,7 @@ again:
 			 * If the last bit is zero, a space will merge
 			 * with the silence after the command.
 			 */
-			if (IS_PULSE(u) && data->count == RC5_NBITS) {
+			if (IS_PULSE(u) && data->count == data->wanted_bits) {
 				data->state = STATE_FINISHED;
 				goto again;
 			}
@@ -188,8 +196,10 @@ again:
 
 	case STATE_BIT_END:
 		if (IS_TRANSITION(u, data->last_unit)) {
-			if (data->count == RC5_NBITS)
+			if (data->count == data->wanted_bits)
 				data->state = STATE_FINISHED;
+			else if (data->count == CHECK_RC5X_NBITS)
+				data->state = STATE_CHECK_RC5X;
 			else
 				data->state = STATE_BIT_START;
 
@@ -198,22 +208,52 @@ again:
 		}
 		break;
 
+	case STATE_CHECK_RC5X:
+		if (IS_SPACE(u) && DURATION(u) >= DURATION(RC5X_SPACE)) {
+			/* RC5X */
+			data->wanted_bits = RC5X_NBITS;
+			DECREASE_DURATION(u, DURATION(RC5X_SPACE));
+		} else {
+			/* RC5 */
+			data->wanted_bits = RC5_NBITS;
+		}
+		data->state = STATE_BIT_START;
+		goto again;
+
 	case STATE_FINISHED:
-		command  = (data->rc5_bits & 0x0003F) >> 0;
-		system   = (data->rc5_bits & 0x007C0) >> 6;
-		toggle   = (data->rc5_bits & 0x00800) ? 1 : 0;
-		command += (data->rc5_bits & 0x01000) ? 0 : 0x40;
-		scancode = system << 8 | command;
-
-		IR_dprintk(1, "RC5 scancode 0x%04x (toggle: %u)\n",
-			   scancode, toggle);
+		if (data->wanted_bits == RC5X_NBITS) {
+			/* RC5X */
+			u8 xdata, command, system;
+			xdata    = (data->rc5_bits & 0x0003F) >> 0;
+			command  = (data->rc5_bits & 0x00FC0) >> 6;
+			system   = (data->rc5_bits & 0x1F000) >> 12;
+			toggle   = (data->rc5_bits & 0x20000) ? 1 : 0;
+			command += (data->rc5_bits & 0x01000) ? 0 : 0x40;
+			scancode = system << 16 | command << 8 | xdata;
+
+			IR_dprintk(1, "RC5X scancode 0x%06x (toggle: %u)\n",
+				   scancode, toggle);
+
+		} else {
+			/* RC5 */
+			u8 command, system;
+			command  = (data->rc5_bits & 0x0003F) >> 0;
+			system   = (data->rc5_bits & 0x007C0) >> 6;
+			toggle   = (data->rc5_bits & 0x00800) ? 1 : 0;
+			command += (data->rc5_bits & 0x01000) ? 0 : 0x40;
+			scancode = system << 8 | command;
+
+			IR_dprintk(1, "RC5 scancode 0x%04x (toggle: %u)\n",
+				   scancode, toggle);
+		}
+
 		ir_keydown(input_dev, scancode, toggle);
 		data->state = STATE_INACTIVE;
 		return 0;
 	}
 
 out:
-	IR_dprintk(1, "RC5 decode failed at state %i (%i units, %ius)\n",
+	IR_dprintk(1, "RC5(x) decode failed at state %i (%i units, %ius)\n",
 		   data->state, u, TO_US(duration));
 	data->state = STATE_INACTIVE;
 	return -EINVAL;
@@ -273,7 +313,7 @@ static int __init ir_rc5_decode_init(void)
 {
 	ir_raw_handler_register(&rc5_handler);
 
-	printk(KERN_INFO "IR RC-5 protocol handler initialized\n");
+	printk(KERN_INFO "IR RC5(x) protocol handler initialized\n");
 	return 0;
 }
 
@@ -288,4 +328,4 @@ module_exit(ir_rc5_decode_exit);
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Mauro Carvalho Chehab <mchehab@redhat.com>");
 MODULE_AUTHOR("Red Hat Inc. (http://www.redhat.com)");
-MODULE_DESCRIPTION("RC-5 IR protocol decoder");
+MODULE_DESCRIPTION("RC5(x) IR protocol decoder");

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 19+ messages in thread
* [PATCH 3/4] Add NECx support to ir-core
  2010-04-08 23:04 [PATCH 0/4] ir-core: one-line bugfix, add RC5x, NECx, RC6 David Härdeman
  2010-04-08 23:04   ` David Härdeman
  2010-04-08 23:04   ` David Härdeman
@ 2010-04-08 23:04 ` David Härdeman
  2010-04-08 23:04 ` [PATCH 4/4] Add RC6 " David Härdeman
  3 siblings, 0 replies; 19+ messages in thread
From: David Härdeman @ 2010-04-08 23:04 UTC (permalink / raw)
  To: mchehab; +Cc: linux-input, linux-media

This patch adds NECx support to drivers/media/IR/ir-nec-decoder.c

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/IR/ir-nec-decoder.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/media/IR/ir-nec-decoder.c b/drivers/media/IR/ir-nec-decoder.c
index f22d1af..d128c19 100644
--- a/drivers/media/IR/ir-nec-decoder.c
+++ b/drivers/media/IR/ir-nec-decoder.c
@@ -18,6 +18,7 @@
 #define NEC_NBITS		32
 #define NEC_UNIT		562500  /* ns */
 #define NEC_HEADER_PULSE	PULSE(16)
+#define NECX_HEADER_PULSE	PULSE(8) /* Less common NEC variant */
 #define NEC_HEADER_SPACE	SPACE(8)
 #define NEC_REPEAT_SPACE	SPACE(4)
 #define NEC_BIT_PULSE		PULSE(1)
@@ -152,7 +153,7 @@ static int ir_nec_decode(struct input_dev *input_dev, s64 duration)
 	switch (data->state) {
 
 	case STATE_INACTIVE:
-		if (u == NEC_HEADER_PULSE) {
+		if (u == NEC_HEADER_PULSE || u == NECX_HEADER_PULSE) {
 			data->count = 0;
 			data->state = STATE_HEADER_SPACE;
 		}


^ permalink raw reply related	[flat|nested] 19+ messages in thread
* [PATCH 4/4] Add RC6 support to ir-core
  2010-04-08 23:04 [PATCH 0/4] ir-core: one-line bugfix, add RC5x, NECx, RC6 David Härdeman
                   ` (2 preceding siblings ...)
  2010-04-08 23:04 ` [PATCH 3/4] Add NECx " David Härdeman
@ 2010-04-08 23:04 ` David Härdeman
  2010-04-09  8:17     ` Andreas Oberritter
  2010-04-10  1:12   ` Andy Walls
  3 siblings, 2 replies; 19+ messages in thread
From: David Härdeman @ 2010-04-08 23:04 UTC (permalink / raw)
  To: mchehab; +Cc: linux-input, linux-media

This patch adds an RC6 decoder (modes 0 and 6A) to ir-core.

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/IR/Kconfig          |    9 +
 drivers/media/IR/Makefile         |    1 
 drivers/media/IR/ir-core-priv.h   |    7 +
 drivers/media/IR/ir-raw-event.c   |    1 
 drivers/media/IR/ir-rc6-decoder.c |  412 +++++++++++++++++++++++++++++++++++++
 drivers/media/IR/ir-sysfs.c       |    2 
 include/media/rc-map.h            |    1 
 7 files changed, 433 insertions(+), 0 deletions(-)
 create mode 100644 drivers/media/IR/ir-rc6-decoder.c

diff --git a/drivers/media/IR/Kconfig b/drivers/media/IR/Kconfig
index ba81bda..28d336d 100644
--- a/drivers/media/IR/Kconfig
+++ b/drivers/media/IR/Kconfig
@@ -27,3 +27,12 @@ config IR_RC5_DECODER
 	---help---
 	   Enable this option if you have IR with RC-5 protocol, and
 	   if the IR is decoded in software
+
+config IR_RC6_DECODER
+	tristate "Enable IR raw decoder for the RC6 protocol"
+	depends on IR_CORE
+	default y
+
+	---help---
+	   Enable this option if you have an infrared remote control which
+	   uses the RC6 protocol, and you need software decoding support.
diff --git a/drivers/media/IR/Makefile b/drivers/media/IR/Makefile
index 62e12d5..792d9ca 100644
--- a/drivers/media/IR/Makefile
+++ b/drivers/media/IR/Makefile
@@ -7,3 +7,4 @@ obj-$(CONFIG_IR_CORE) += ir-core.o
 obj-$(CONFIG_VIDEO_IR) += ir-common.o
 obj-$(CONFIG_IR_NEC_DECODER) += ir-nec-decoder.o
 obj-$(CONFIG_IR_RC5_DECODER) += ir-rc5-decoder.o
+obj-$(CONFIG_IR_RC6_DECODER) += ir-rc6-decoder.o
diff --git a/drivers/media/IR/ir-core-priv.h b/drivers/media/IR/ir-core-priv.h
index ab785bc..853ec80 100644
--- a/drivers/media/IR/ir-core-priv.h
+++ b/drivers/media/IR/ir-core-priv.h
@@ -109,4 +109,11 @@ void ir_raw_init(void);
 #define load_rc5_decode()	0
 #endif
 
+/* from ir-rc6-decoder.c */
+#ifdef CONFIG_IR_RC5_DECODER_MODULE
+#define load_rc6_decode()	request_module("ir-rc6-decoder")
+#else
+#define load_rc6_decode()	0
+#endif
+
 #endif /* _IR_RAW_EVENT */
diff --git a/drivers/media/IR/ir-raw-event.c b/drivers/media/IR/ir-raw-event.c
index 2efc051..ef0f231 100644
--- a/drivers/media/IR/ir-raw-event.c
+++ b/drivers/media/IR/ir-raw-event.c
@@ -224,6 +224,7 @@ static void init_decoders(struct work_struct *work)
 
 	load_nec_decode();
 	load_rc5_decode();
+	load_rc6_decode();
 
 	/* If needed, we may later add some init code. In this case,
 	   it is needed to change the CONFIG_MODULE test at ir-core.h
diff --git a/drivers/media/IR/ir-rc6-decoder.c b/drivers/media/IR/ir-rc6-decoder.c
new file mode 100644
index 0000000..ccc5be2
--- /dev/null
+++ b/drivers/media/IR/ir-rc6-decoder.c
@@ -0,0 +1,412 @@
+/* ir-rc6-decoder.c - A decoder for the RC6 IR protocol
+ *
+ * Copyright (C) 2010 by David Härdeman <david@hardeman.nu>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include "ir-core-priv.h"
+
+/*
+ * This decoder currently supports:
+ * RC6-0-16	(standard toggle bit in header)
+ * RC6-6A-24	(no toggle bit)
+ * RC6-6A-32	(MCE version with toggle bit in body)
+ */
+
+#define RC6_UNIT		444444	/* us */
+#define RC6_HEADER_NBITS	4	/* not including toggle bit */
+#define RC6_0_NBITS		16
+#define RC6_6A_SMALL_NBITS	24
+#define RC6_6A_LARGE_NBITS	32
+#define RC6_PREFIX_PULSE	PULSE(6)
+#define RC6_PREFIX_SPACE	SPACE(2)
+#define RC6_MODE_MASK		0x07	/* for the header bits */
+#define RC6_STARTBIT_MASK	0x08	/* for the header bits */
+#define RC6_6A_MCE_TOGGLE_MASK	0x8000	/* for the body bits */
+
+/* Used to register rc6_decoder clients */
+static LIST_HEAD(decoder_list);
+static DEFINE_SPINLOCK(decoder_lock);
+
+enum rc6_mode {
+	RC6_MODE_0,
+	RC6_MODE_6A,
+	RC6_MODE_UNKNOWN,
+};
+
+enum rc6_state {
+	STATE_INACTIVE,
+	STATE_PREFIX_SPACE,
+	STATE_HEADER_BIT_START,
+	STATE_HEADER_BIT_END,
+	STATE_TOGGLE_START,
+	STATE_TOGGLE_END,
+	STATE_BODY_BIT_START,
+	STATE_BODY_BIT_END,
+	STATE_FINISHED,
+};
+
+struct decoder_data {
+	struct list_head	list;
+	struct ir_input_dev	*ir_dev;
+	int			enabled:1;
+
+	/* State machine control */
+	enum rc6_state		state;
+	u8			header;
+	u32			body;
+	int			last_unit;
+	bool			toggle;
+	unsigned		count;
+	unsigned		wanted_bits;
+};
+
+
+/**
+ * get_decoder_data()	- gets decoder data
+ * @input_dev:	input device
+ *
+ * Returns the struct decoder_data that corresponds to a device
+ */
+static struct decoder_data *get_decoder_data(struct  ir_input_dev *ir_dev)
+{
+	struct decoder_data *data = NULL;
+
+	spin_lock(&decoder_lock);
+	list_for_each_entry(data, &decoder_list, list) {
+		if (data->ir_dev == ir_dev)
+			break;
+	}
+	spin_unlock(&decoder_lock);
+	return data;
+}
+
+static ssize_t store_enabled(struct device *d,
+			     struct device_attribute *mattr,
+			     const char *buf,
+			     size_t len)
+{
+	unsigned long value;
+	struct ir_input_dev *ir_dev = dev_get_drvdata(d);
+	struct decoder_data *data = get_decoder_data(ir_dev);
+
+	if (!data)
+		return -EINVAL;
+
+	if (strict_strtoul(buf, 10, &value) || value > 1)
+		return -EINVAL;
+
+	data->enabled = value;
+
+	return len;
+}
+
+static ssize_t show_enabled(struct device *d,
+			     struct device_attribute *mattr, char *buf)
+{
+	struct ir_input_dev *ir_dev = dev_get_drvdata(d);
+	struct decoder_data *data = get_decoder_data(ir_dev);
+
+	if (!data)
+		return -EINVAL;
+
+	if (data->enabled)
+		return sprintf(buf, "1\n");
+	else
+	return sprintf(buf, "0\n");
+}
+
+static DEVICE_ATTR(enabled, S_IRUGO | S_IWUSR, show_enabled, store_enabled);
+
+static struct attribute *decoder_attributes[] = {
+	&dev_attr_enabled.attr,
+	NULL
+};
+
+static struct attribute_group decoder_attribute_group = {
+	.name	= "rc6_decoder",
+	.attrs	= decoder_attributes,
+};
+
+static enum rc6_mode rc6_mode(struct decoder_data *data) {
+	switch (data->header & RC6_MODE_MASK) {
+	case 0:
+		return RC6_MODE_0;
+	case 6:
+		if (!data->toggle)
+			return RC6_MODE_6A;
+		/* fall through */
+	default:
+		return RC6_MODE_UNKNOWN;
+	}
+}
+
+/**
+ * ir_rc6_decode() - Decode one RC6 pulse or space
+ * @input_dev:	the struct input_dev descriptor of the device
+ * @duration:	duration of pulse/space in ns
+ *
+ * This function returns -EINVAL if the pulse violates the state machine
+ */
+static int ir_rc6_decode(struct input_dev *input_dev, s64 duration)
+{
+	struct decoder_data *data;
+	struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
+	u32 scancode;
+	u8 toggle;
+	int u;
+
+	data = get_decoder_data(ir_dev);
+	if (!data)
+		return -EINVAL;
+
+	if (!data->enabled)
+		return 0;
+
+	if (IS_RESET(duration)) {
+		data->state = STATE_INACTIVE;
+		return 0;
+	}
+
+	u =  TO_UNITS(duration, RC6_UNIT);
+	if (DURATION(u) == 0)
+		goto out;
+
+again:
+	IR_dprintk(2, "RC6 decode started at state %i (%i units, %ius)\n",
+		   data->state, u, TO_US(duration));
+
+	if (DURATION(u) == 0 && data->state != STATE_FINISHED)
+		return 0;
+
+	switch (data->state) {
+
+	case STATE_INACTIVE:
+		if (u >= RC6_PREFIX_PULSE - 1 && u <= RC6_PREFIX_PULSE + 1) {
+			data->state = STATE_PREFIX_SPACE;
+			data->count = 0;
+			return 0;
+		}
+		break;
+
+	case STATE_PREFIX_SPACE:
+		if (u == RC6_PREFIX_SPACE) {
+			data->state = STATE_HEADER_BIT_START;
+			return 0;
+		}
+		break;
+
+	case STATE_HEADER_BIT_START:
+		if (DURATION(u) == 1) {
+			data->header <<= 1;
+			if (IS_PULSE(u))
+				data->header |= 1;
+			data->count++;
+			data->last_unit = u;
+			data->state = STATE_HEADER_BIT_END;
+			return 0;
+		}
+		break;
+
+	case STATE_HEADER_BIT_END:
+		if (IS_TRANSITION(u, data->last_unit)) {
+			if (data->count == RC6_HEADER_NBITS)
+				data->state = STATE_TOGGLE_START;
+			else
+				data->state = STATE_HEADER_BIT_START;
+
+			DECREASE_DURATION(u, 1);
+			goto again;
+		}
+		break;
+
+	case STATE_TOGGLE_START:
+		if (DURATION(u) == 2) {
+			data->toggle = IS_PULSE(u);
+			data->last_unit = u;
+			data->state = STATE_TOGGLE_END;
+			return 0;
+		}
+		break;
+
+	case STATE_TOGGLE_END:
+		if (IS_TRANSITION(u, data->last_unit) && DURATION(u) >= 2) {
+			data->state = STATE_BODY_BIT_START;
+			data->last_unit = u;
+			DECREASE_DURATION(u, 2);
+			data->count = 0;
+
+			if (!(data->header & RC6_STARTBIT_MASK)) {
+				IR_dprintk(1, "RC6 invalid start bit\n");
+				break;
+			}
+
+			switch (rc6_mode(data)) {
+			case RC6_MODE_0:
+				data->wanted_bits = RC6_0_NBITS;
+				break;
+			case RC6_MODE_6A:
+				/* This might look weird, but we basically
+				   check the value of the first body bit to
+				   determine the number of bits in mode 6A */
+				if ((DURATION(u) == 0 && IS_SPACE(data->last_unit)) || DURATION(u) > 0)
+					data->wanted_bits = RC6_6A_LARGE_NBITS;
+				else
+					data->wanted_bits = RC6_6A_SMALL_NBITS;
+				break;
+			default:
+				IR_dprintk(1, "RC6 unknown mode\n");
+				goto out;
+			}
+			goto again;
+		}
+		break;
+
+	case STATE_BODY_BIT_START:
+		if (DURATION(u) == 1) {
+			data->body <<= 1;
+			if (IS_PULSE(u))
+				data->body |= 1;
+			data->count++;
+			data->last_unit = u;
+
+			/*
+			 * If the last bit is one, a space will merge
+			 * with the silence after the command.
+			 */
+			if (IS_PULSE(u) && data->count == data->wanted_bits) {
+				data->state = STATE_FINISHED;
+				goto again;
+			}
+
+			data->state = STATE_BODY_BIT_END;
+			return 0;
+		}
+		break;
+
+	case STATE_BODY_BIT_END:
+		if (IS_TRANSITION(u, data->last_unit)) {
+			if (data->count == data->wanted_bits)
+				data->state = STATE_FINISHED;
+			else
+				data->state = STATE_BODY_BIT_START;
+
+			DECREASE_DURATION(u, 1);
+			goto again;
+		}
+		break;
+
+	case STATE_FINISHED:
+		switch (rc6_mode(data)) {
+		case RC6_MODE_0:
+			scancode = data->body & 0xffff;
+			toggle = data->toggle;
+			IR_dprintk(1, "RC6(0) scancode 0x%04x (toggle: %u)\n",
+				   scancode, toggle);
+			break;
+		case RC6_MODE_6A:
+			if (data->wanted_bits == RC6_6A_LARGE_NBITS) {
+				toggle = data->body & RC6_6A_MCE_TOGGLE_MASK ? 1 : 0;
+				scancode = data->body & ~RC6_6A_MCE_TOGGLE_MASK;
+			} else {
+				toggle = 0;
+				scancode = data->body & 0xffffff;
+			}
+
+			IR_dprintk(1, "RC6(6A) scancode 0x%08x (toggle: %u)\n",
+				   scancode, toggle);
+			break;
+		default:
+			IR_dprintk(1, "RC6 unknown mode\n");
+			goto out;
+		}
+
+		ir_keydown(input_dev, scancode, toggle);
+		data->state = STATE_INACTIVE;
+		return 0;
+	}
+
+out:
+	IR_dprintk(1, "RC6 decode failed at state %i (%i units, %ius)\n",
+		   data->state, u, TO_US(duration));
+	data->state = STATE_INACTIVE;
+	return -EINVAL;
+}
+
+static int ir_rc6_register(struct input_dev *input_dev)
+{
+	struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
+	struct decoder_data *data;
+	int rc;
+
+	rc = sysfs_create_group(&ir_dev->dev.kobj, &decoder_attribute_group);
+	if (rc < 0)
+		return rc;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data) {
+		sysfs_remove_group(&ir_dev->dev.kobj, &decoder_attribute_group);
+		return -ENOMEM;
+	}
+
+	data->ir_dev = ir_dev;
+	data->enabled = 1;
+
+	spin_lock(&decoder_lock);
+	list_add_tail(&data->list, &decoder_list);
+	spin_unlock(&decoder_lock);
+
+	return 0;
+}
+
+static int ir_rc6_unregister(struct input_dev *input_dev)
+{
+	struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
+	static struct decoder_data *data;
+
+	data = get_decoder_data(ir_dev);
+	if (!data)
+		return 0;
+
+	sysfs_remove_group(&ir_dev->dev.kobj, &decoder_attribute_group);
+
+	spin_lock(&decoder_lock);
+	list_del(&data->list);
+	spin_unlock(&decoder_lock);
+
+	return 0;
+}
+
+static struct ir_raw_handler rc6_handler = {
+	.decode		= ir_rc6_decode,
+	.raw_register	= ir_rc6_register,
+	.raw_unregister	= ir_rc6_unregister,
+};
+
+static int __init ir_rc6_decode_init(void)
+{
+	ir_raw_handler_register(&rc6_handler);
+
+	printk(KERN_INFO "IR RC6 protocol handler initialized\n");
+	return 0;
+}
+
+static void __exit ir_rc6_decode_exit(void)
+{
+	ir_raw_handler_unregister(&rc6_handler);
+}
+
+module_init(ir_rc6_decode_init);
+module_exit(ir_rc6_decode_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("David Härdeman <david@hardeman.nu>");
+MODULE_DESCRIPTION("RC6 IR protocol decoder");
diff --git a/drivers/media/IR/ir-sysfs.c b/drivers/media/IR/ir-sysfs.c
index a222d4f..d7ec973 100644
--- a/drivers/media/IR/ir-sysfs.c
+++ b/drivers/media/IR/ir-sysfs.c
@@ -60,6 +60,8 @@ static ssize_t show_protocol(struct device *d,
 		s = "pulse-distance";
 	else if (ir_type == IR_TYPE_NEC)
 		s = "nec";
+	else if (ir_type == IR_TYPE_RC6)
+		s = "rc6";
 	else
 		s = "other";
 
diff --git a/include/media/rc-map.h b/include/media/rc-map.h
index 3b7fe5a..11f6618 100644
--- a/include/media/rc-map.h
+++ b/include/media/rc-map.h
@@ -15,6 +15,7 @@
 #define IR_TYPE_RC5	(1  << 0)	/* Philips RC5 protocol */
 #define IR_TYPE_PD	(1  << 1)	/* Pulse distance encoded IR */
 #define IR_TYPE_NEC	(1  << 2)
+#define IR_TYPE_RC6	(1  << 3)	/* Philips RC6 protocol */
 #define IR_TYPE_OTHER	(1u << 31)
 
 struct ir_scancode {


^ permalink raw reply related	[flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] Fix the drivers/media/dvb/ttpci/budget-ci.c conversion to ir-core
  2010-04-08 23:04   ` David Härdeman
@ 2010-04-08 23:11     ` Mauro Carvalho Chehab
  -1 siblings, 0 replies; 19+ messages in thread
From: Mauro Carvalho Chehab @ 2010-04-08 23:11 UTC (permalink / raw)
  To: David Härdeman; +Cc: linux-input, linux-media

Hi David,

David Härdeman wrote:
> When I converted drivers/media/dvb/ttpci/budget-ci.c to use ir-core
> I missed one line. This patch fixes that mistake.

I did this already (I merged with your commit, at v4l-dvb.git), to avoid 
breaking git bisect. I'll backport it to the ir.git tree after finishing
to process my loooong patchwork queue. (I'll probably just create a new
branch there from v4l-dvb.git - since I've merged all ir-related patches
there already).


Cheers,
Mauro

PS.: As you can see, stgit did the right thing with inline ;)
> 
> Signed-off-by: David Härdeman <david@hardeman.nu>
> ---
>  drivers/media/dvb/ttpci/budget-ci.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/dvb/ttpci/budget-ci.c b/drivers/media/dvb/ttpci/budget-ci.c
> index 8950df1..4617143 100644
> --- a/drivers/media/dvb/ttpci/budget-ci.c
> +++ b/drivers/media/dvb/ttpci/budget-ci.c
> @@ -225,8 +225,6 @@ static int msp430_ir_init(struct budget_ci *budget_ci)
>  		break;
>  	}
>  
> -	ir_input_init(input_dev, &budget_ci->ir.state, IR_TYPE_RC5);
> -
>  	error = ir_input_register(input_dev, ir_codes, NULL, MODULE_NAME);
>  	if (error) {
>  		printk(KERN_ERR "budget_ci: could not init driver for IR device (code %d)\n", error);
> 


-- 

Cheers,
Mauro

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] Fix the drivers/media/dvb/ttpci/budget-ci.c conversion to ir-core
@ 2010-04-08 23:11     ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 19+ messages in thread
From: Mauro Carvalho Chehab @ 2010-04-08 23:11 UTC (permalink / raw)
  To: David Härdeman; +Cc: linux-input, linux-media

Hi David,

David Härdeman wrote:
> When I converted drivers/media/dvb/ttpci/budget-ci.c to use ir-core
> I missed one line. This patch fixes that mistake.

I did this already (I merged with your commit, at v4l-dvb.git), to avoid 
breaking git bisect. I'll backport it to the ir.git tree after finishing
to process my loooong patchwork queue. (I'll probably just create a new
branch there from v4l-dvb.git - since I've merged all ir-related patches
there already).


Cheers,
Mauro

PS.: As you can see, stgit did the right thing with inline ;)
> 
> Signed-off-by: David Härdeman <david@hardeman.nu>
> ---
>  drivers/media/dvb/ttpci/budget-ci.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/dvb/ttpci/budget-ci.c b/drivers/media/dvb/ttpci/budget-ci.c
> index 8950df1..4617143 100644
> --- a/drivers/media/dvb/ttpci/budget-ci.c
> +++ b/drivers/media/dvb/ttpci/budget-ci.c
> @@ -225,8 +225,6 @@ static int msp430_ir_init(struct budget_ci *budget_ci)
>  		break;
>  	}
>  
> -	ir_input_init(input_dev, &budget_ci->ir.state, IR_TYPE_RC5);
> -
>  	error = ir_input_register(input_dev, ir_codes, NULL, MODULE_NAME);
>  	if (error) {
>  		printk(KERN_ERR "budget_ci: could not init driver for IR device (code %d)\n", error);
> 


-- 

Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] Add RC6 support to ir-core
  2010-04-08 23:04 ` [PATCH 4/4] Add RC6 " David Härdeman
@ 2010-04-09  8:17     ` Andreas Oberritter
  2010-04-10  1:12   ` Andy Walls
  1 sibling, 0 replies; 19+ messages in thread
From: Andreas Oberritter @ 2010-04-09  8:17 UTC (permalink / raw)
  To: David Härdeman; +Cc: mchehab, linux-input, linux-media

Hello David,

David Härdeman wrote:
> --- a/drivers/media/IR/ir-core-priv.h
> +++ b/drivers/media/IR/ir-core-priv.h
> @@ -109,4 +109,11 @@ void ir_raw_init(void);
>  #define load_rc5_decode()	0
>  #endif
>  
> +/* from ir-rc6-decoder.c */
> +#ifdef CONFIG_IR_RC5_DECODER_MODULE
> +#define load_rc6_decode()	request_module("ir-rc6-decoder")
> +#else
> +#define load_rc6_decode()	0
> +#endif
> +
>  #endif /* _IR_RAW_EVENT */

you probably intended to use CONFIG_IR_RC6_DECODER_MODULE instead.

Regards,
Andreas

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] Add RC6 support to ir-core
@ 2010-04-09  8:17     ` Andreas Oberritter
  0 siblings, 0 replies; 19+ messages in thread
From: Andreas Oberritter @ 2010-04-09  8:17 UTC (permalink / raw)
  To: David Härdeman; +Cc: mchehab, linux-input, linux-media

Hello David,

David Härdeman wrote:
> --- a/drivers/media/IR/ir-core-priv.h
> +++ b/drivers/media/IR/ir-core-priv.h
> @@ -109,4 +109,11 @@ void ir_raw_init(void);
>  #define load_rc5_decode()	0
>  #endif
>  
> +/* from ir-rc6-decoder.c */
> +#ifdef CONFIG_IR_RC5_DECODER_MODULE
> +#define load_rc6_decode()	request_module("ir-rc6-decoder")
> +#else
> +#define load_rc6_decode()	0
> +#endif
> +
>  #endif /* _IR_RAW_EVENT */

you probably intended to use CONFIG_IR_RC6_DECODER_MODULE instead.

Regards,
Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] Add RC6 support to ir-core
  2010-04-09  8:17     ` Andreas Oberritter
  (?)
@ 2010-04-09  8:31     ` David Härdeman
  2010-04-09 12:43         ` Mauro Carvalho Chehab
  -1 siblings, 1 reply; 19+ messages in thread
From: David Härdeman @ 2010-04-09  8:31 UTC (permalink / raw)
  To: Andreas Oberritter; +Cc: mchehab, linux-input, linux-media

On Fri, April 9, 2010 10:17, Andreas Oberritter wrote:
>> +/* from ir-rc6-decoder.c */
>> +#ifdef CONFIG_IR_RC5_DECODER_MODULE
>
> you probably intended to use CONFIG_IR_RC6_DECODER_MODULE instead.

Of course, thanks for noticing.

Mauro, do you want a new patch or will you fix it yourself? (and by the
way, Mauro, how come the patches went straight to the v4l-dvb.git tree? I
assume they'll be merged back to your ir.git tree at a later stage?)

-- 
David Härdeman


^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] Add RC6 support to ir-core
  2010-04-09  8:31     ` David Härdeman
@ 2010-04-09 12:43         ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 19+ messages in thread
From: Mauro Carvalho Chehab @ 2010-04-09 12:43 UTC (permalink / raw)
  To: David Härdeman; +Cc: Andreas Oberritter, linux-input, linux-media

David Härdeman wrote:
> On Fri, April 9, 2010 10:17, Andreas Oberritter wrote:
>>> +/* from ir-rc6-decoder.c */
>>> +#ifdef CONFIG_IR_RC5_DECODER_MODULE
>> you probably intended to use CONFIG_IR_RC6_DECODER_MODULE instead.

Andreas, thanks for pointing it!

> Of course, thanks for noticing.
> 
> Mauro, do you want a new patch or will you fix it yourself? 

Fixed. Just sent the correction patches upstream. As, in this case, it
doesn't break bisect, I just added a one-line change patch.

> (and by the
> way, Mauro, how come the patches went straight to the v4l-dvb.git tree? I
> assume they'll be merged back to your ir.git tree at a later stage?)

I just merged it back at the ir.git tree.

Cheers,
Mauro

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] Add RC6 support to ir-core
@ 2010-04-09 12:43         ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 19+ messages in thread
From: Mauro Carvalho Chehab @ 2010-04-09 12:43 UTC (permalink / raw)
  To: David Härdeman; +Cc: Andreas Oberritter, linux-input, linux-media

David Härdeman wrote:
> On Fri, April 9, 2010 10:17, Andreas Oberritter wrote:
>>> +/* from ir-rc6-decoder.c */
>>> +#ifdef CONFIG_IR_RC5_DECODER_MODULE
>> you probably intended to use CONFIG_IR_RC6_DECODER_MODULE instead.

Andreas, thanks for pointing it!

> Of course, thanks for noticing.
> 
> Mauro, do you want a new patch or will you fix it yourself? 

Fixed. Just sent the correction patches upstream. As, in this case, it
doesn't break bisect, I just added a one-line change patch.

> (and by the
> way, Mauro, how come the patches went straight to the v4l-dvb.git tree? I
> assume they'll be merged back to your ir.git tree at a later stage?)

I just merged it back at the ir.git tree.

Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] Add RC6 support to ir-core
  2010-04-08 23:04 ` [PATCH 4/4] Add RC6 " David Härdeman
  2010-04-09  8:17     ` Andreas Oberritter
@ 2010-04-10  1:12   ` Andy Walls
  2010-04-10 19:52     ` David Härdeman
  2010-04-10 22:23     ` David Härdeman
  1 sibling, 2 replies; 19+ messages in thread
From: Andy Walls @ 2010-04-10  1:12 UTC (permalink / raw)
  To: David Härdeman; +Cc: mchehab, linux-input, linux-media

On Fri, 2010-04-09 at 01:04 +0200, David Härdeman wrote:
> This patch adds an RC6 decoder (modes 0 and 6A) to ir-core.
>
> Signed-off-by: David Härdeman <david@hardeman.nu>

David,

Overall, a nice job of implementing RC6 decoder logic.  I have a few
comments below (along with some of me reasoning to myself out loud).


> ---
>  drivers/media/IR/Kconfig          |    9 +
>  drivers/media/IR/Makefile         |    1 
>  drivers/media/IR/ir-core-priv.h   |    7 +
>  drivers/media/IR/ir-raw-event.c   |    1 
>  drivers/media/IR/ir-rc6-decoder.c |  412 +++++++++++++++++++++++++++++++++++++
>  drivers/media/IR/ir-sysfs.c       |    2 
>  include/media/rc-map.h            |    1 
>  7 files changed, 433 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/media/IR/ir-rc6-decoder.c
> 
> diff --git a/drivers/media/IR/Kconfig b/drivers/media/IR/Kconfig
> index ba81bda..28d336d 100644
> --- a/drivers/media/IR/Kconfig
> +++ b/drivers/media/IR/Kconfig
> @@ -27,3 +27,12 @@ config IR_RC5_DECODER
>  	---help---
>  	   Enable this option if you have IR with RC-5 protocol, and
>  	   if the IR is decoded in software
> +
> +config IR_RC6_DECODER
> +	tristate "Enable IR raw decoder for the RC6 protocol"
> +	depends on IR_CORE
> +	default y
> +
> +	---help---
> +	   Enable this option if you have an infrared remote control which
> +	   uses the RC6 protocol, and you need software decoding support.
> diff --git a/drivers/media/IR/Makefile b/drivers/media/IR/Makefile
> index 62e12d5..792d9ca 100644
> --- a/drivers/media/IR/Makefile
> +++ b/drivers/media/IR/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_IR_CORE) += ir-core.o
>  obj-$(CONFIG_VIDEO_IR) += ir-common.o
>  obj-$(CONFIG_IR_NEC_DECODER) += ir-nec-decoder.o
>  obj-$(CONFIG_IR_RC5_DECODER) += ir-rc5-decoder.o
> +obj-$(CONFIG_IR_RC6_DECODER) += ir-rc6-decoder.o
> diff --git a/drivers/media/IR/ir-core-priv.h b/drivers/media/IR/ir-core-priv.h
> index ab785bc..853ec80 100644
> --- a/drivers/media/IR/ir-core-priv.h
> +++ b/drivers/media/IR/ir-core-priv.h
> @@ -109,4 +109,11 @@ void ir_raw_init(void);
>  #define load_rc5_decode()	0
>  #endif
>  
> +/* from ir-rc6-decoder.c */
> +#ifdef CONFIG_IR_RC5_DECODER_MODULE
> +#define load_rc6_decode()	request_module("ir-rc6-decoder")
> +#else
> +#define load_rc6_decode()	0
> +#endif
> +
>  #endif /* _IR_RAW_EVENT */
> diff --git a/drivers/media/IR/ir-raw-event.c b/drivers/media/IR/ir-raw-event.c
> index 2efc051..ef0f231 100644
> --- a/drivers/media/IR/ir-raw-event.c
> +++ b/drivers/media/IR/ir-raw-event.c
> @@ -224,6 +224,7 @@ static void init_decoders(struct work_struct *work)
>  
>  	load_nec_decode();
>  	load_rc5_decode();
> +	load_rc6_decode();
>  
>  	/* If needed, we may later add some init code. In this case,
>  	   it is needed to change the CONFIG_MODULE test at ir-core.h
> diff --git a/drivers/media/IR/ir-rc6-decoder.c b/drivers/media/IR/ir-rc6-decoder.c
> new file mode 100644
> index 0000000..ccc5be2
> --- /dev/null
> +++ b/drivers/media/IR/ir-rc6-decoder.c
> @@ -0,0 +1,412 @@
> +/* ir-rc6-decoder.c - A decoder for the RC6 IR protocol
> + *
> + * Copyright (C) 2010 by David Härdeman <david@hardeman.nu>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include "ir-core-priv.h"
> +
> +/*
> + * This decoder currently supports:
> + * RC6-0-16	(standard toggle bit in header)
> + * RC6-6A-24	(no toggle bit)
> + * RC6-6A-32	(MCE version with toggle bit in body)
> + */

Just for reference for review:

http://slycontrol.ru/scr/kb/rc6.htm
http://www.picbasic.nl/info_rc6_uk.htm


RC6 Mode 0:

prefix mark:  111111
prefix space: 00
start bit:    10           (biphase encoding of '1')
mode bits:    010101       (biphase encoding of '000')
toggle bits:  0011 or 1100 (double duration biphase coding of '0' or '1')
system byte:  xxxxxxxxxxxxxxxx (biphase encoding of 8 bits)
command byte: yyyyyyyyyyyyyyyy (biphase encoding of 8 bits)

RC6 Mode 6A:

prefix mark:   111111
prefix space:  00
start bit:     10           (biphase encoding of '1')
mode bits:     101001       (biphase encoding of '110' for '6') 
trailer bits:  0011         (double duration biphase encoding of '0' for 'A')
customer len:  01 or 10     (biphase encoding of '0' for 7 bits or '1' for 15 bits)
customer bits: xxxxxxxxxxxxxx (biphase encoding of 7 bits for a short customer code)
		or
               xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx (biphase encoding of 15 bits for a long customer code)
system byte:   yyyyyyyyyyyyyyyy (biphase encoding of 8 bits)
command byte:  zzzzzzzzzzzzzzzz (biphase encoding of 8 bits)


> +#define RC6_UNIT		444444	/* us */
> +#define RC6_HEADER_NBITS	4	/* not including toggle bit */
> +#define RC6_0_NBITS		16
> +#define RC6_6A_SMALL_NBITS	24
> +#define RC6_6A_LARGE_NBITS	32

According to the slycontrol.ru website, the data length is, in theory
OEM dependent for Mode 6A, limited to a max of 24 bits (3 bytes) after a
short customer code and 128 bits (16 bytes) after a long customer code.

I don't know what the reality is for existing remotes.

Would it be better to look for the signal free time of 6 RC6_UNITs to
declare the end of reception, instead of a bit count?


> +#define RC6_PREFIX_PULSE	PULSE(6)
> +#define RC6_PREFIX_SPACE	SPACE(2)
> +#define RC6_MODE_MASK		0x07	/* for the header bits */
> +#define RC6_STARTBIT_MASK	0x08	/* for the header bits */
> +#define RC6_6A_MCE_TOGGLE_MASK	0x8000	/* for the body bits */

That's an OEM specific toggle bit.  It is likely more properly named
RC6_6A_MS_TOGGLE_MASK.  See slide 6 of:

http://download.microsoft.com/download/9/8/f/98f3fe47-dfc3-4e74-92a3-088782200fe7/TWEN05007_WinHEC05.ppt

(Although in reality, every remote that wants to work with stock MS
drivers will use it.)

> +
> +/* Used to register rc6_decoder clients */
> +static LIST_HEAD(decoder_list);
> +static DEFINE_SPINLOCK(decoder_lock);
> +
> +enum rc6_mode {
> +	RC6_MODE_0,
> +	RC6_MODE_6A,
> +	RC6_MODE_UNKNOWN,
> +};
> +
> +enum rc6_state {
> +	STATE_INACTIVE,
> +	STATE_PREFIX_SPACE,
> +	STATE_HEADER_BIT_START,
> +	STATE_HEADER_BIT_END,
> +	STATE_TOGGLE_START,
> +	STATE_TOGGLE_END,
> +	STATE_BODY_BIT_START,
> +	STATE_BODY_BIT_END,
> +	STATE_FINISHED,
> +};
> +
> +struct decoder_data {
> +	struct list_head	list;
> +	struct ir_input_dev	*ir_dev;
> +	int			enabled:1;
> +
> +	/* State machine control */
> +	enum rc6_state		state;
> +	u8			header;
> +	u32			body;
> +	int			last_unit;
> +	bool			toggle;
> +	unsigned		count;
> +	unsigned		wanted_bits;
> +};
> +
> +
> +/**
> + * get_decoder_data()	- gets decoder data
> + * @input_dev:	input device
> + *
> + * Returns the struct decoder_data that corresponds to a device
> + */
> +static struct decoder_data *get_decoder_data(struct  ir_input_dev *ir_dev)
> +{
> +	struct decoder_data *data = NULL;
> +
> +	spin_lock(&decoder_lock);
> +	list_for_each_entry(data, &decoder_list, list) {
> +		if (data->ir_dev == ir_dev)
> +			break;
> +	}
> +	spin_unlock(&decoder_lock);
> +	return data;
> +}
> +
> +static ssize_t store_enabled(struct device *d,
> +			     struct device_attribute *mattr,
> +			     const char *buf,
> +			     size_t len)
> +{
> +	unsigned long value;
> +	struct ir_input_dev *ir_dev = dev_get_drvdata(d);
> +	struct decoder_data *data = get_decoder_data(ir_dev);
> +
> +	if (!data)
> +		return -EINVAL;
> +
> +	if (strict_strtoul(buf, 10, &value) || value > 1)
> +		return -EINVAL;
> +
> +	data->enabled = value;
> +
> +	return len;
> +}
> +
> +static ssize_t show_enabled(struct device *d,
> +			     struct device_attribute *mattr, char *buf)
> +{
> +	struct ir_input_dev *ir_dev = dev_get_drvdata(d);
> +	struct decoder_data *data = get_decoder_data(ir_dev);
> +
> +	if (!data)
> +		return -EINVAL;
> +
> +	if (data->enabled)
> +		return sprintf(buf, "1\n");
> +	else
> +	return sprintf(buf, "0\n");
> +}
> +
> +static DEVICE_ATTR(enabled, S_IRUGO | S_IWUSR, show_enabled, store_enabled);
> +
> +static struct attribute *decoder_attributes[] = {
> +	&dev_attr_enabled.attr,
> +	NULL
> +};
> +
> +static struct attribute_group decoder_attribute_group = {
> +	.name	= "rc6_decoder",
> +	.attrs	= decoder_attributes,
> +};
> +
> +static enum rc6_mode rc6_mode(struct decoder_data *data) {
> +	switch (data->header & RC6_MODE_MASK) {
> +	case 0:
> +		return RC6_MODE_0;
> +	case 6:
> +		if (!data->toggle)
> +			return RC6_MODE_6A;
> +		/* fall through */
> +	default:
> +		return RC6_MODE_UNKNOWN;
> +	}
> +}
> +
> +/**
> + * ir_rc6_decode() - Decode one RC6 pulse or space
> + * @input_dev:	the struct input_dev descriptor of the device
> + * @duration:	duration of pulse/space in ns
> + *
> + * This function returns -EINVAL if the pulse violates the state machine
> + */
> +static int ir_rc6_decode(struct input_dev *input_dev, s64 duration)
> +{
> +	struct decoder_data *data;
> +	struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
> +	u32 scancode;
> +	u8 toggle;
> +	int u;
> +
> +	data = get_decoder_data(ir_dev);
> +	if (!data)
> +		return -EINVAL;
> +
> +	if (!data->enabled)
> +		return 0;
> +
> +	if (IS_RESET(duration)) {
> +		data->state = STATE_INACTIVE;
> +		return 0;
> +	}
> +
> +	u =  TO_UNITS(duration, RC6_UNIT);
> +	if (DURATION(u) == 0)
> +		goto out;
> +
> +again:
> +	IR_dprintk(2, "RC6 decode started at state %i (%i units, %ius)\n",
> +		   data->state, u, TO_US(duration));
> +
> +	if (DURATION(u) == 0 && data->state != STATE_FINISHED)
> +		return 0;

Isn't there a better way to structure the logic to break up two adjacent
pulse units than with goto's out of the switch back up to here?

A do {} while() loop would have been much clearer.



> +	switch (data->state) {
> +
> +	case STATE_INACTIVE:
> +		if (u >= RC6_PREFIX_PULSE - 1 && u <= RC6_PREFIX_PULSE + 1) {
> +			data->state = STATE_PREFIX_SPACE;
> +			data->count = 0;
> +			return 0;
> +		}
> +		break;
> +
> +	case STATE_PREFIX_SPACE:
> +		if (u == RC6_PREFIX_SPACE) {
> +			data->state = STATE_HEADER_BIT_START;
> +			return 0;
> +		}
> +		break;
> +
> +	case STATE_HEADER_BIT_START:
> +		if (DURATION(u) == 1) {
> +			data->header <<= 1;
> +			if (IS_PULSE(u))
> +				data->header |= 1;
> +			data->count++;
> +			data->last_unit = u;
> +			data->state = STATE_HEADER_BIT_END;
> +			return 0;
> +		}
> +		break;
> +
> +	case STATE_HEADER_BIT_END:
> +		if (IS_TRANSITION(u, data->last_unit)) {
> +			if (data->count == RC6_HEADER_NBITS)
> +				data->state = STATE_TOGGLE_START;
> +			else
> +				data->state = STATE_HEADER_BIT_START;
> +
> +			DECREASE_DURATION(u, 1);
> +			goto again;
> +		}
> +		break;
> +
> +	case STATE_TOGGLE_START:
> +		if (DURATION(u) == 2) {
> +			data->toggle = IS_PULSE(u);
> +			data->last_unit = u;
> +			data->state = STATE_TOGGLE_END;
> +			return 0;
> +		}
> +		break;
> +
> +	case STATE_TOGGLE_END:
> +		if (IS_TRANSITION(u, data->last_unit) && DURATION(u) >= 2) {
> +			data->state = STATE_BODY_BIT_START;
> +			data->last_unit = u;
> +			DECREASE_DURATION(u, 2);
> +			data->count = 0;
> +
> +			if (!(data->header & RC6_STARTBIT_MASK)) {
> +				IR_dprintk(1, "RC6 invalid start bit\n");
> +				break;
> +			}
> +
> +			switch (rc6_mode(data)) {
> +			case RC6_MODE_0:
> +				data->wanted_bits = RC6_0_NBITS;
> +				break;
> +			case RC6_MODE_6A:
> +				/* This might look weird, but we basically
> +				   check the value of the first body bit to
> +				   determine the number of bits in mode 6A */

This bit basically only tells you the length of the customer code: 7 or
15 bits.  


> +				if ((DURATION(u) == 0 && IS_SPACE(data->last_unit)) || DURATION(u) > 0)
> +					data->wanted_bits = RC6_6A_LARGE_NBITS;
> +				else
> +					data->wanted_bits = RC6_6A_SMALL_NBITS;
> +				break;
> +			default:
> +				IR_dprintk(1, "RC6 unknown mode\n");
> +				goto out;
> +			}
> +			goto again;
> +		}
> +		break;
> +
> +	case STATE_BODY_BIT_START:
> +		if (DURATION(u) == 1) {
> +			data->body <<= 1;
> +			if (IS_PULSE(u))
> +				data->body |= 1;
> +			data->count++;
> +			data->last_unit = u;
> +
> +			/*
> +			 * If the last bit is one, a space will merge
> +			 * with the silence after the command.
> +			 */
> +			if (IS_PULSE(u) && data->count == data->wanted_bits) {
> +				data->state = STATE_FINISHED;
> +				goto again;
> +			}
> +
> +			data->state = STATE_BODY_BIT_END;
> +			return 0;
> +		}
> +		break;
> +
> +	case STATE_BODY_BIT_END:
> +		if (IS_TRANSITION(u, data->last_unit)) {
> +			if (data->count == data->wanted_bits)
> +				data->state = STATE_FINISHED;
> +			else
> +				data->state = STATE_BODY_BIT_START;
> +
> +			DECREASE_DURATION(u, 1);
> +			goto again;
> +		}
> +		break;
> +
> +	case STATE_FINISHED:
> +		switch (rc6_mode(data)) {
> +		case RC6_MODE_0:
> +			scancode = data->body & 0xffff;
> +			toggle = data->toggle;
> +			IR_dprintk(1, "RC6(0) scancode 0x%04x (toggle: %u)\n",
> +				   scancode, toggle);
> +			break;
> +		case RC6_MODE_6A:
> +			if (data->wanted_bits == RC6_6A_LARGE_NBITS) {
> +				toggle = data->body & RC6_6A_MCE_TOGGLE_MASK ? 1 : 0;
> +				scancode = data->body & ~RC6_6A_MCE_TOGGLE_MASK;

Technically this depends on the OEM.  In reality, every RC6 Mode 6A
remote that wants to work with Microsoft stock drivers will likely use
this bit as a toggle.

Regards,
Andy

> +			} else {
> +				toggle = 0;
> +				scancode = data->body & 0xffffff;
> +			}
> +
> +			IR_dprintk(1, "RC6(6A) scancode 0x%08x (toggle: %u)\n",
> +				   scancode, toggle);
> +			break;
> +		default:
> +			IR_dprintk(1, "RC6 unknown mode\n");
> +			goto out;
> +		}
> +
> +		ir_keydown(input_dev, scancode, toggle);
> +		data->state = STATE_INACTIVE;
> +		return 0;
> +	}
> +
> +out:
> +	IR_dprintk(1, "RC6 decode failed at state %i (%i units, %ius)\n",
> +		   data->state, u, TO_US(duration));
> +	data->state = STATE_INACTIVE;
> +	return -EINVAL;
> +}
> +
> +static int ir_rc6_register(struct input_dev *input_dev)
> +{
> +	struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
> +	struct decoder_data *data;
> +	int rc;
> +
> +	rc = sysfs_create_group(&ir_dev->dev.kobj, &decoder_attribute_group);
> +	if (rc < 0)
> +		return rc;
> +
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	if (!data) {
> +		sysfs_remove_group(&ir_dev->dev.kobj, &decoder_attribute_group);
> +		return -ENOMEM;
> +	}
> +
> +	data->ir_dev = ir_dev;
> +	data->enabled = 1;
> +
> +	spin_lock(&decoder_lock);
> +	list_add_tail(&data->list, &decoder_list);
> +	spin_unlock(&decoder_lock);
> +
> +	return 0;
> +}
> +
> +static int ir_rc6_unregister(struct input_dev *input_dev)
> +{
> +	struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
> +	static struct decoder_data *data;
> +
> +	data = get_decoder_data(ir_dev);
> +	if (!data)
> +		return 0;
> +
> +	sysfs_remove_group(&ir_dev->dev.kobj, &decoder_attribute_group);
> +
> +	spin_lock(&decoder_lock);
> +	list_del(&data->list);
> +	spin_unlock(&decoder_lock);
> +
> +	return 0;
> +}
> +
> +static struct ir_raw_handler rc6_handler = {
> +	.decode		= ir_rc6_decode,
> +	.raw_register	= ir_rc6_register,
> +	.raw_unregister	= ir_rc6_unregister,
> +};
> +
> +static int __init ir_rc6_decode_init(void)
> +{
> +	ir_raw_handler_register(&rc6_handler);
> +
> +	printk(KERN_INFO "IR RC6 protocol handler initialized\n");
> +	return 0;
> +}
> +
> +static void __exit ir_rc6_decode_exit(void)
> +{
> +	ir_raw_handler_unregister(&rc6_handler);
> +}
> +
> +module_init(ir_rc6_decode_init);
> +module_exit(ir_rc6_decode_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("David Härdeman <david@hardeman.nu>");
> +MODULE_DESCRIPTION("RC6 IR protocol decoder");
> diff --git a/drivers/media/IR/ir-sysfs.c b/drivers/media/IR/ir-sysfs.c
> index a222d4f..d7ec973 100644
> --- a/drivers/media/IR/ir-sysfs.c
> +++ b/drivers/media/IR/ir-sysfs.c
> @@ -60,6 +60,8 @@ static ssize_t show_protocol(struct device *d,
>  		s = "pulse-distance";
>  	else if (ir_type == IR_TYPE_NEC)
>  		s = "nec";
> +	else if (ir_type == IR_TYPE_RC6)
> +		s = "rc6";
>  	else
>  		s = "other";
>  
> diff --git a/include/media/rc-map.h b/include/media/rc-map.h
> index 3b7fe5a..11f6618 100644
> --- a/include/media/rc-map.h
> +++ b/include/media/rc-map.h
> @@ -15,6 +15,7 @@
>  #define IR_TYPE_RC5	(1  << 0)	/* Philips RC5 protocol */
>  #define IR_TYPE_PD	(1  << 1)	/* Pulse distance encoded IR */
>  #define IR_TYPE_NEC	(1  << 2)
> +#define IR_TYPE_RC6	(1  << 3)	/* Philips RC6 protocol */
>  #define IR_TYPE_OTHER	(1u << 31)
>  
>  struct ir_scancode {
> 
> --
> 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
> 


^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] Add RC6 support to ir-core
  2010-04-10  1:12   ` Andy Walls
@ 2010-04-10 19:52     ` David Härdeman
  2010-04-10 21:19         ` Andy Walls
  2010-04-10 22:23     ` David Härdeman
  1 sibling, 1 reply; 19+ messages in thread
From: David Härdeman @ 2010-04-10 19:52 UTC (permalink / raw)
  To: Andy Walls; +Cc: mchehab, linux-input, linux-media

On Fri, Apr 09, 2010 at 09:12:08PM -0400, Andy Walls wrote:
> On Fri, 2010-04-09 at 01:04 +0200, David Härdeman wrote:
> > diff --git a/drivers/media/IR/ir-rc6-decoder.c 
> > b/drivers/media/IR/ir-rc6-decoder.c
> > new file mode 100644
> > index 0000000..ccc5be2
> > --- /dev/null
> > +++ b/drivers/media/IR/ir-rc6-decoder.c
> > @@ -0,0 +1,412 @@
> > +/* ir-rc6-decoder.c - A decoder for the RC6 IR protocol
> > + *
> > + * Copyright (C) 2010 by David Härdeman <david@hardeman.nu>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation version 2 of the License.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include "ir-core-priv.h"
> > +
> > +/*
> > + * This decoder currently supports:
> > + * RC6-0-16	(standard toggle bit in header)
> > + * RC6-6A-24	(no toggle bit)
> > + * RC6-6A-32	(MCE version with toggle bit in body)
> > + */
> 
> Just for reference for review:
> 
> http://slycontrol.ru/scr/kb/rc6.htm
> http://www.picbasic.nl/info_rc6_uk.htm
> 
> 
> RC6 Mode 0:
> 
> prefix mark:  111111
> prefix space: 00
> start bit:    10           (biphase encoding of '1')
> mode bits:    010101       (biphase encoding of '000')
> toggle bits:  0011 or 1100 (double duration biphase coding of '0' or '1')
> system byte:  xxxxxxxxxxxxxxxx (biphase encoding of 8 bits)
> command byte: yyyyyyyyyyyyyyyy (biphase encoding of 8 bits)
> 
> RC6 Mode 6A:
> 
> prefix mark:   111111
> prefix space:  00
> start bit:     10           (biphase encoding of '1')
> mode bits:     101001       (biphase encoding of '110' for '6') 
> trailer bits:  0011         (double duration biphase encoding of '0' for 'A')
> customer len:  01 or 10     (biphase encoding of '0' for 7 bits or '1' for 15 bits)
> customer bits: xxxxxxxxxxxxxx (biphase encoding of 7 bits for a short customer code)
> 		or
>                xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx (biphase encoding of 15 bits for a long customer code)
> system byte:   yyyyyyyyyyyyyyyy (biphase encoding of 8 bits)
> command byte:  zzzzzzzzzzzzzzzz (biphase encoding of 8 bits)
> 
> 
> > +#define RC6_UNIT		444444	/* us */
> > +#define RC6_HEADER_NBITS	4	/* not including toggle bit */
> > +#define RC6_0_NBITS		16
> > +#define RC6_6A_SMALL_NBITS	24
> > +#define RC6_6A_LARGE_NBITS	32
> 
> According to the slycontrol.ru website, the data length is, in theory
> OEM dependent for Mode 6A, limited to a max of 24 bits (3 bytes) after a
> short customer code and 128 bits (16 bytes) after a long customer code.
> 
> I don't know what the reality is for existing remotes.
> 
> Would it be better to look for the signal free time of 6 RC6_UNITs to
> declare the end of reception, instead of a bit count?

Yes, it might be better from a correctness point of view, and I think it 
might be a worthwhile change if we want to support a few more odd 
remotes (although the only one I'm aware of - even after trawling 
through lots of lirc configs, decodeir.dll configs and remotecentral 
configs - is the "Sky" remote which seems to use a short customer code 
and a 12 bit body).

The thing is though, that with the different 32 bit scancodes, we can't 
anyway represent anything beyond a 32 bit message body (including the 
customer code, which should be included in the scancode). I have a 
proposal on changing the scancodes used in ir-code, but I haven't 
written up the details yet.

It's also interesting to note that the ProntoEdit NG program, which is 
written by Philips, only allows a 24/32 bit body (including the customer 
bits).

> > +#define RC6_PREFIX_PULSE	PULSE(6)
> > +#define RC6_PREFIX_SPACE	SPACE(2)
> > +#define RC6_MODE_MASK		0x07	/* for the header bits */
> > +#define RC6_STARTBIT_MASK	0x08	/* for the header bits */
> > +#define RC6_6A_MCE_TOGGLE_MASK	0x8000	/* for the body bits */
> 
> That's an OEM specific toggle bit.

Umm, yes I know? That's why the define includes the "_MCE_" part and 
also what the comment in the beginning of the decoder says:

	* RC6-6A-32	(MCE version with toggle bit in body)

>  It is likely more properly named
> RC6_6A_MS_TOGGLE_MASK.  See slide 6 of:
> 
> http://download.microsoft.com/download/9/8/f/98f3fe47-dfc3-4e74-92a3-088782200fe7/TWEN05007_WinHEC05.ppt
> 
> (Although in reality, every remote that wants to work with stock MS
> drivers will use it.)

I'm not sure what your point is...it's already called 
RC6_6A_MCE_TOGGLE_MASK, as in "RC6 6A Windows Media Center Edition 
Toggle Mask".
 
> > +
> > +/* Used to register rc6_decoder clients */
> > +static LIST_HEAD(decoder_list);
> > +static DEFINE_SPINLOCK(decoder_lock);
> > +
> > +enum rc6_mode {
> > +	RC6_MODE_0,
> > +	RC6_MODE_6A,
> > +	RC6_MODE_UNKNOWN,
> > +};
> > +
> > +enum rc6_state {
> > +	STATE_INACTIVE,
> > +	STATE_PREFIX_SPACE,
> > +	STATE_HEADER_BIT_START,
> > +	STATE_HEADER_BIT_END,
> > +	STATE_TOGGLE_START,
> > +	STATE_TOGGLE_END,
> > +	STATE_BODY_BIT_START,
> > +	STATE_BODY_BIT_END,
> > +	STATE_FINISHED,
> > +};
> > +
> > +struct decoder_data {
> > +	struct list_head	list;
> > +	struct ir_input_dev	*ir_dev;
> > +	int			enabled:1;
> > +
> > +	/* State machine control */
> > +	enum rc6_state		state;
> > +	u8			header;
> > +	u32			body;
> > +	int			last_unit;
> > +	bool			toggle;
> > +	unsigned		count;
> > +	unsigned		wanted_bits;
> > +};
> > +
> > +
> > +/**
> > + * get_decoder_data()	- gets decoder data
> > + * @input_dev:	input device
> > + *
> > + * Returns the struct decoder_data that corresponds to a device
> > + */
> > +static struct decoder_data *get_decoder_data(struct  ir_input_dev *ir_dev)
> > +{
> > +	struct decoder_data *data = NULL;
> > +
> > +	spin_lock(&decoder_lock);
> > +	list_for_each_entry(data, &decoder_list, list) {
> > +		if (data->ir_dev == ir_dev)
> > +			break;
> > +	}
> > +	spin_unlock(&decoder_lock);
> > +	return data;
> > +}
> > +
> > +static ssize_t store_enabled(struct device *d,
> > +			     struct device_attribute *mattr,
> > +			     const char *buf,
> > +			     size_t len)
> > +{
> > +	unsigned long value;
> > +	struct ir_input_dev *ir_dev = dev_get_drvdata(d);
> > +	struct decoder_data *data = get_decoder_data(ir_dev);
> > +
> > +	if (!data)
> > +		return -EINVAL;
> > +
> > +	if (strict_strtoul(buf, 10, &value) || value > 1)
> > +		return -EINVAL;
> > +
> > +	data->enabled = value;
> > +
> > +	return len;
> > +}
> > +
> > +static ssize_t show_enabled(struct device *d,
> > +			     struct device_attribute *mattr, char *buf)
> > +{
> > +	struct ir_input_dev *ir_dev = dev_get_drvdata(d);
> > +	struct decoder_data *data = get_decoder_data(ir_dev);
> > +
> > +	if (!data)
> > +		return -EINVAL;
> > +
> > +	if (data->enabled)
> > +		return sprintf(buf, "1\n");
> > +	else
> > +	return sprintf(buf, "0\n");
> > +}
> > +
> > +static DEVICE_ATTR(enabled, S_IRUGO | S_IWUSR, show_enabled, store_enabled);
> > +
> > +static struct attribute *decoder_attributes[] = {
> > +	&dev_attr_enabled.attr,
> > +	NULL
> > +};
> > +
> > +static struct attribute_group decoder_attribute_group = {
> > +	.name	= "rc6_decoder",
> > +	.attrs	= decoder_attributes,
> > +};
> > +
> > +static enum rc6_mode rc6_mode(struct decoder_data *data) {
> > +	switch (data->header & RC6_MODE_MASK) {
> > +	case 0:
> > +		return RC6_MODE_0;
> > +	case 6:
> > +		if (!data->toggle)
> > +			return RC6_MODE_6A;
> > +		/* fall through */
> > +	default:
> > +		return RC6_MODE_UNKNOWN;
> > +	}
> > +}
> > +
> > +/**
> > + * ir_rc6_decode() - Decode one RC6 pulse or space
> > + * @input_dev:	the struct input_dev descriptor of the device
> > + * @duration:	duration of pulse/space in ns
> > + *
> > + * This function returns -EINVAL if the pulse violates the state machine
> > + */
> > +static int ir_rc6_decode(struct input_dev *input_dev, s64 duration)
> > +{
> > +	struct decoder_data *data;
> > +	struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
> > +	u32 scancode;
> > +	u8 toggle;
> > +	int u;
> > +
> > +	data = get_decoder_data(ir_dev);
> > +	if (!data)
> > +		return -EINVAL;
> > +
> > +	if (!data->enabled)
> > +		return 0;
> > +
> > +	if (IS_RESET(duration)) {
> > +		data->state = STATE_INACTIVE;
> > +		return 0;
> > +	}
> > +
> > +	u =  TO_UNITS(duration, RC6_UNIT);
> > +	if (DURATION(u) == 0)
> > +		goto out;
> > +
> > +again:
> > +	IR_dprintk(2, "RC6 decode started at state %i (%i units, %ius)\n",
> > +		   data->state, u, TO_US(duration));
> > +
> > +	if (DURATION(u) == 0 && data->state != STATE_FINISHED)
> > +		return 0;
> 
> Isn't there a better way to structure the logic to break up two adjacent
> pulse units than with goto's out of the switch back up to here?
> 
> A do {} while() loop would have been much clearer.
> 
> 
> > +	switch (data->state) {
> > +
> > +	case STATE_INACTIVE:
> > +		if (u >= RC6_PREFIX_PULSE - 1 && u <= RC6_PREFIX_PULSE + 1) {
> > +			data->state = STATE_PREFIX_SPACE;
> > +			data->count = 0;
> > +			return 0;
> > +		}
> > +		break;
> > +
> > +	case STATE_PREFIX_SPACE:
> > +		if (u == RC6_PREFIX_SPACE) {
> > +			data->state = STATE_HEADER_BIT_START;
> > +			return 0;
> > +		}
> > +		break;
> > +
> > +	case STATE_HEADER_BIT_START:
> > +		if (DURATION(u) == 1) {
> > +			data->header <<= 1;
> > +			if (IS_PULSE(u))
> > +				data->header |= 1;
> > +			data->count++;
> > +			data->last_unit = u;
> > +			data->state = STATE_HEADER_BIT_END;
> > +			return 0;
> > +		}
> > +		break;
> > +
> > +	case STATE_HEADER_BIT_END:
> > +		if (IS_TRANSITION(u, data->last_unit)) {
> > +			if (data->count == RC6_HEADER_NBITS)
> > +				data->state = STATE_TOGGLE_START;
> > +			else
> > +				data->state = STATE_HEADER_BIT_START;
> > +
> > +			DECREASE_DURATION(u, 1);
> > +			goto again;
> > +		}
> > +		break;
> > +
> > +	case STATE_TOGGLE_START:
> > +		if (DURATION(u) == 2) {
> > +			data->toggle = IS_PULSE(u);
> > +			data->last_unit = u;
> > +			data->state = STATE_TOGGLE_END;
> > +			return 0;
> > +		}
> > +		break;
> > +
> > +	case STATE_TOGGLE_END:
> > +		if (IS_TRANSITION(u, data->last_unit) && DURATION(u) >= 2) {
> > +			data->state = STATE_BODY_BIT_START;
> > +			data->last_unit = u;
> > +			DECREASE_DURATION(u, 2);
> > +			data->count = 0;
> > +
> > +			if (!(data->header & RC6_STARTBIT_MASK)) {
> > +				IR_dprintk(1, "RC6 invalid start bit\n");
> > +				break;
> > +			}
> > +
> > +			switch (rc6_mode(data)) {
> > +			case RC6_MODE_0:
> > +				data->wanted_bits = RC6_0_NBITS;
> > +				break;
> > +			case RC6_MODE_6A:
> > +				/* This might look weird, but we basically
> > +				   check the value of the first body bit to
> > +				   determine the number of bits in mode 6A */
> 
> This bit basically only tells you the length of the customer code: 7 or
> 15 bits.  

I know.

> > +				if ((DURATION(u) == 0 && IS_SPACE(data->last_unit)) || DURATION(u) > 0)
> > +					data->wanted_bits = RC6_6A_LARGE_NBITS;
> > +				else
> > +					data->wanted_bits = RC6_6A_SMALL_NBITS;
> > +				break;
> > +			default:
> > +				IR_dprintk(1, "RC6 unknown mode\n");
> > +				goto out;
> > +			}
> > +			goto again;
> > +		}
> > +		break;
> > +
> > +	case STATE_BODY_BIT_START:
> > +		if (DURATION(u) == 1) {
> > +			data->body <<= 1;
> > +			if (IS_PULSE(u))
> > +				data->body |= 1;
> > +			data->count++;
> > +			data->last_unit = u;
> > +
> > +			/*
> > +			 * If the last bit is one, a space will merge
> > +			 * with the silence after the command.
> > +			 */
> > +			if (IS_PULSE(u) && data->count == data->wanted_bits) {
> > +				data->state = STATE_FINISHED;
> > +				goto again;
> > +			}
> > +
> > +			data->state = STATE_BODY_BIT_END;
> > +			return 0;
> > +		}
> > +		break;
> > +
> > +	case STATE_BODY_BIT_END:
> > +		if (IS_TRANSITION(u, data->last_unit)) {
> > +			if (data->count == data->wanted_bits)
> > +				data->state = STATE_FINISHED;
> > +			else
> > +				data->state = STATE_BODY_BIT_START;
> > +
> > +			DECREASE_DURATION(u, 1);
> > +			goto again;
> > +		}
> > +		break;
> > +
> > +	case STATE_FINISHED:
> > +		switch (rc6_mode(data)) {
> > +		case RC6_MODE_0:
> > +			scancode = data->body & 0xffff;
> > +			toggle = data->toggle;
> > +			IR_dprintk(1, "RC6(0) scancode 0x%04x (toggle: %u)\n",
> > +				   scancode, toggle);
> > +			break;
> > +		case RC6_MODE_6A:
> > +			if (data->wanted_bits == RC6_6A_LARGE_NBITS) {
> > +				toggle = data->body & RC6_6A_MCE_TOGGLE_MASK ? 1 : 0;
> > +				scancode = data->body & ~RC6_6A_MCE_TOGGLE_MASK;
> 
> Technically this depends on the OEM.  

I know, again it's mentioned in one of the first comments of the code.

> In reality, every RC6 Mode 6A
> remote that wants to work with Microsoft stock drivers will likely use
> this bit as a toggle.

We might want to check if the (long) customer code matches 0x800F (if I 
remember correctly) and apply the MCE behaviour only then, but it might 
also come back to bite us in the ass if, for example, different 
customers have implemented the same definition of the body.

Then again, as far as I can remeber, the Philips Pronto raw code for MCE 
commands doesn't include the customer code, which would indicate that 
it's fixed.

Another question is: if the customer code doesn't match, what do we do?  
The body could be defined as any random gibberish by vendor XYZ, 
including toggles and checksums which means that the scancode would 
change for each keypress if we blindly report the entire body as a 32 
bit scancode.

-- 
David Härdeman

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] Add RC6 support to ir-core
  2010-04-10 19:52     ` David Härdeman
@ 2010-04-10 21:19         ` Andy Walls
  0 siblings, 0 replies; 19+ messages in thread
From: Andy Walls @ 2010-04-10 21:19 UTC (permalink / raw)
  To: David Härdeman; +Cc: mchehab, linux-input, linux-media

On Sat, 2010-04-10 at 21:52 +0200, David Härdeman wrote:
> On Fri, Apr 09, 2010 at 09:12:08PM -0400, Andy Walls wrote:
> > On Fri, 2010-04-09 at 01:04 +0200, David Härdeman wrote:
> > > diff --git a/drivers/media/IR/ir-rc6-decoder.c 
> > > b/drivers/media/IR/ir-rc6-decoder.c
> > > new file mode 100644
> > > index 0000000..ccc5be2
> > > --- /dev/null
> > > +++ b/drivers/media/IR/ir-rc6-decoder.c
> > > @@ -0,0 +1,412 @@
> > > +/* ir-rc6-decoder.c - A decoder for the RC6 IR protocol
> > > + *
> > > + * Copyright (C) 2010 by David Härdeman <david@hardeman.nu>
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License as published by
> > > + * the Free Software Foundation version 2 of the License.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > + * GNU General Public License for more details.
> > > + */
> > > +
> > > +#include "ir-core-priv.h"
> > > +
> > > +/*
> > > + * This decoder currently supports:
> > > + * RC6-0-16	(standard toggle bit in header)
> > > + * RC6-6A-24	(no toggle bit)
> > > + * RC6-6A-32	(MCE version with toggle bit in body)
> > > + */
> > 
> > Just for reference for review:
> > 
> > http://slycontrol.ru/scr/kb/rc6.htm
> > http://www.picbasic.nl/info_rc6_uk.htm
> > 
> > 
> > RC6 Mode 0:
> > 
> > prefix mark:  111111
> > prefix space: 00
> > start bit:    10           (biphase encoding of '1')
> > mode bits:    010101       (biphase encoding of '000')
> > toggle bits:  0011 or 1100 (double duration biphase coding of '0' or '1')
> > system byte:  xxxxxxxxxxxxxxxx (biphase encoding of 8 bits)
> > command byte: yyyyyyyyyyyyyyyy (biphase encoding of 8 bits)
> > 
> > RC6 Mode 6A:
> > 
> > prefix mark:   111111
> > prefix space:  00
> > start bit:     10           (biphase encoding of '1')
> > mode bits:     101001       (biphase encoding of '110' for '6') 
> > trailer bits:  0011         (double duration biphase encoding of '0' for 'A')
> > customer len:  01 or 10     (biphase encoding of '0' for 7 bits or '1' for 15 bits)
> > customer bits: xxxxxxxxxxxxxx (biphase encoding of 7 bits for a short customer code)
> > 		or
> >                xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx (biphase encoding of 15 bits for a long customer code)
> > system byte:   yyyyyyyyyyyyyyyy (biphase encoding of 8 bits)
> > command byte:  zzzzzzzzzzzzzzzz (biphase encoding of 8 bits)
> > 
> > 
> > > +#define RC6_UNIT		444444	/* us */
> > > +#define RC6_HEADER_NBITS	4	/* not including toggle bit */
> > > +#define RC6_0_NBITS		16
> > > +#define RC6_6A_SMALL_NBITS	24
> > > +#define RC6_6A_LARGE_NBITS	32
> > 
> > According to the slycontrol.ru website, the data length is, in theory
> > OEM dependent for Mode 6A, limited to a max of 24 bits (3 bytes) after a
> > short customer code and 128 bits (16 bytes) after a long customer code.
> > 
> > I don't know what the reality is for existing remotes.
> > 
> > Would it be better to look for the signal free time of 6 RC6_UNITs to
> > declare the end of reception, instead of a bit count?
> 
> Yes, it might be better from a correctness point of view, and I think it 
> might be a worthwhile change if we want to support a few more odd 
> remotes (although the only one I'm aware of - even after trawling 
> through lots of lirc configs, decodeir.dll configs and remotecentral 
> configs - is the "Sky" remote which seems to use a short customer code 
> and a 12 bit body).
>
> The thing is though, that with the different 32 bit scancodes, we can't 
> anyway represent anything beyond a 32 bit message body (including the 
> customer code, which should be included in the scancode).

Yup.

> I have a 
> proposal on changing the scancodes used in ir-code, but I haven't 
> written up the details yet.
> 
> It's also interesting to note that the ProntoEdit NG program, which is 
> written by Philips, only allows a 24/32 bit body (including the customer 
> bits).

So maybe forget my suggesting of coding to the theoretical limits, if
we'll never really encounter one in practice.


> > > +#define RC6_PREFIX_PULSE	PULSE(6)
> > > +#define RC6_PREFIX_SPACE	SPACE(2)
> > > +#define RC6_MODE_MASK		0x07	/* for the header bits */
> > > +#define RC6_STARTBIT_MASK	0x08	/* for the header bits */
> > > +#define RC6_6A_MCE_TOGGLE_MASK	0x8000	/* for the body bits */
> > 
> > That's an OEM specific toggle bit.
> 
> Umm, yes I know? That's why the define includes the "_MCE_" part and 
> also what the comment in the beginning of the decoder says:
> 
> 	* RC6-6A-32	(MCE version with toggle bit in body)
> 
> >  It is likely more properly named
> > RC6_6A_MS_TOGGLE_MASK.  See slide 6 of:
> > 
> > http://download.microsoft.com/download/9/8/f/98f3fe47-dfc3-4e74-92a3-088782200fe7/TWEN05007_WinHEC05.ppt
> > 
> > (Although in reality, every remote that wants to work with stock MS
> > drivers will use it.)
> 
> I'm not sure what your point is...it's already called 
> RC6_6A_MCE_TOGGLE_MASK, as in "RC6 6A Windows Media Center Edition 
> Toggle Mask".

My point was that that toggle is OEM defined, since the OEM customer
defines their RC-6 Mode 6A payload.

(BTW I have an HP OEM'ed MCE USB receiver in front of me, which is
probably why I don't implcitly associate "MCE" with Microsoft.)

If Microsoft's OEM payload is the only OEM payload we ever think will
make an RC-6 Mode 6A remote that outputs a long customer code, then OK.
Microsoft is Customer ID 0x800f.  Who are 0x8000 through 0x800e?



> > > +again:
> > > +	IR_dprintk(2, "RC6 decode started at state %i (%i units, %ius)\n",
> > > +		   data->state, u, TO_US(duration));
> > > +
> > > +	if (DURATION(u) == 0 && data->state != STATE_FINISHED)
> > > +		return 0;
> > 
> > Isn't there a better way to structure the logic to break up two adjacent
> > pulse units than with goto's out of the switch back up to here?
> > 
> > A do {} while() loop would have been much clearer.
> > 



> > > +		case RC6_MODE_6A:
> > > +			if (data->wanted_bits == RC6_6A_LARGE_NBITS) {
> > > +				toggle = data->body & RC6_6A_MCE_TOGGLE_MASK ? 1 : 0;
> > > +				scancode = data->body & ~RC6_6A_MCE_TOGGLE_MASK;
> > 
> > Technically this depends on the OEM.  
> 
> I know, again it's mentioned in one of the first comments of the code.
> 
> > In reality, every RC6 Mode 6A
> > remote that wants to work with Microsoft stock drivers will likely use
> > this bit as a toggle.
> 
> We might want to check if the (long) customer code matches 0x800F (if I 
> remember correctly) and apply the MCE behaviour only then, but it might 
> also come back to bite us in the ass if, for example, different 
> customers have implemented the same definition of the body.
> 
> Then again, as far as I can remeber, the Philips Pronto raw code for MCE 
> commands doesn't include the customer code, which would indicate that 
> it's fixed.
> 
> Another question is: if the customer code doesn't match, what do we do?  
> The body could be defined as any random gibberish by vendor XYZ, 
> including toggles and checksums which means that the scancode would 
> change for each keypress if we blindly report the entire body as a 32 
> bit scancode.

This is a standards problem that Phillips and the OEMs have left us with
(since OEM encodings will likely be confidential/proprietary).

Maybe this code should check for long customer id 0x800f, and if the ID
doesn't match, log a warning, but keep on processing as if it were.
Users will report the warning to get it removed for their remote.

Or is that too user-unfriendly?

Regards,
Andy


^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] Add RC6 support to ir-core
@ 2010-04-10 21:19         ` Andy Walls
  0 siblings, 0 replies; 19+ messages in thread
From: Andy Walls @ 2010-04-10 21:19 UTC (permalink / raw)
  To: David Härdeman; +Cc: mchehab, linux-input, linux-media

On Sat, 2010-04-10 at 21:52 +0200, David Härdeman wrote:
> On Fri, Apr 09, 2010 at 09:12:08PM -0400, Andy Walls wrote:
> > On Fri, 2010-04-09 at 01:04 +0200, David Härdeman wrote:
> > > diff --git a/drivers/media/IR/ir-rc6-decoder.c 
> > > b/drivers/media/IR/ir-rc6-decoder.c
> > > new file mode 100644
> > > index 0000000..ccc5be2
> > > --- /dev/null
> > > +++ b/drivers/media/IR/ir-rc6-decoder.c
> > > @@ -0,0 +1,412 @@
> > > +/* ir-rc6-decoder.c - A decoder for the RC6 IR protocol
> > > + *
> > > + * Copyright (C) 2010 by David Härdeman <david@hardeman.nu>
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License as published by
> > > + * the Free Software Foundation version 2 of the License.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > + * GNU General Public License for more details.
> > > + */
> > > +
> > > +#include "ir-core-priv.h"
> > > +
> > > +/*
> > > + * This decoder currently supports:
> > > + * RC6-0-16	(standard toggle bit in header)
> > > + * RC6-6A-24	(no toggle bit)
> > > + * RC6-6A-32	(MCE version with toggle bit in body)
> > > + */
> > 
> > Just for reference for review:
> > 
> > http://slycontrol.ru/scr/kb/rc6.htm
> > http://www.picbasic.nl/info_rc6_uk.htm
> > 
> > 
> > RC6 Mode 0:
> > 
> > prefix mark:  111111
> > prefix space: 00
> > start bit:    10           (biphase encoding of '1')
> > mode bits:    010101       (biphase encoding of '000')
> > toggle bits:  0011 or 1100 (double duration biphase coding of '0' or '1')
> > system byte:  xxxxxxxxxxxxxxxx (biphase encoding of 8 bits)
> > command byte: yyyyyyyyyyyyyyyy (biphase encoding of 8 bits)
> > 
> > RC6 Mode 6A:
> > 
> > prefix mark:   111111
> > prefix space:  00
> > start bit:     10           (biphase encoding of '1')
> > mode bits:     101001       (biphase encoding of '110' for '6') 
> > trailer bits:  0011         (double duration biphase encoding of '0' for 'A')
> > customer len:  01 or 10     (biphase encoding of '0' for 7 bits or '1' for 15 bits)
> > customer bits: xxxxxxxxxxxxxx (biphase encoding of 7 bits for a short customer code)
> > 		or
> >                xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx (biphase encoding of 15 bits for a long customer code)
> > system byte:   yyyyyyyyyyyyyyyy (biphase encoding of 8 bits)
> > command byte:  zzzzzzzzzzzzzzzz (biphase encoding of 8 bits)
> > 
> > 
> > > +#define RC6_UNIT		444444	/* us */
> > > +#define RC6_HEADER_NBITS	4	/* not including toggle bit */
> > > +#define RC6_0_NBITS		16
> > > +#define RC6_6A_SMALL_NBITS	24
> > > +#define RC6_6A_LARGE_NBITS	32
> > 
> > According to the slycontrol.ru website, the data length is, in theory
> > OEM dependent for Mode 6A, limited to a max of 24 bits (3 bytes) after a
> > short customer code and 128 bits (16 bytes) after a long customer code.
> > 
> > I don't know what the reality is for existing remotes.
> > 
> > Would it be better to look for the signal free time of 6 RC6_UNITs to
> > declare the end of reception, instead of a bit count?
> 
> Yes, it might be better from a correctness point of view, and I think it 
> might be a worthwhile change if we want to support a few more odd 
> remotes (although the only one I'm aware of - even after trawling 
> through lots of lirc configs, decodeir.dll configs and remotecentral 
> configs - is the "Sky" remote which seems to use a short customer code 
> and a 12 bit body).
>
> The thing is though, that with the different 32 bit scancodes, we can't 
> anyway represent anything beyond a 32 bit message body (including the 
> customer code, which should be included in the scancode).

Yup.

> I have a 
> proposal on changing the scancodes used in ir-code, but I haven't 
> written up the details yet.
> 
> It's also interesting to note that the ProntoEdit NG program, which is 
> written by Philips, only allows a 24/32 bit body (including the customer 
> bits).

So maybe forget my suggesting of coding to the theoretical limits, if
we'll never really encounter one in practice.


> > > +#define RC6_PREFIX_PULSE	PULSE(6)
> > > +#define RC6_PREFIX_SPACE	SPACE(2)
> > > +#define RC6_MODE_MASK		0x07	/* for the header bits */
> > > +#define RC6_STARTBIT_MASK	0x08	/* for the header bits */
> > > +#define RC6_6A_MCE_TOGGLE_MASK	0x8000	/* for the body bits */
> > 
> > That's an OEM specific toggle bit.
> 
> Umm, yes I know? That's why the define includes the "_MCE_" part and 
> also what the comment in the beginning of the decoder says:
> 
> 	* RC6-6A-32	(MCE version with toggle bit in body)
> 
> >  It is likely more properly named
> > RC6_6A_MS_TOGGLE_MASK.  See slide 6 of:
> > 
> > http://download.microsoft.com/download/9/8/f/98f3fe47-dfc3-4e74-92a3-088782200fe7/TWEN05007_WinHEC05.ppt
> > 
> > (Although in reality, every remote that wants to work with stock MS
> > drivers will use it.)
> 
> I'm not sure what your point is...it's already called 
> RC6_6A_MCE_TOGGLE_MASK, as in "RC6 6A Windows Media Center Edition 
> Toggle Mask".

My point was that that toggle is OEM defined, since the OEM customer
defines their RC-6 Mode 6A payload.

(BTW I have an HP OEM'ed MCE USB receiver in front of me, which is
probably why I don't implcitly associate "MCE" with Microsoft.)

If Microsoft's OEM payload is the only OEM payload we ever think will
make an RC-6 Mode 6A remote that outputs a long customer code, then OK.
Microsoft is Customer ID 0x800f.  Who are 0x8000 through 0x800e?



> > > +again:
> > > +	IR_dprintk(2, "RC6 decode started at state %i (%i units, %ius)\n",
> > > +		   data->state, u, TO_US(duration));
> > > +
> > > +	if (DURATION(u) == 0 && data->state != STATE_FINISHED)
> > > +		return 0;
> > 
> > Isn't there a better way to structure the logic to break up two adjacent
> > pulse units than with goto's out of the switch back up to here?
> > 
> > A do {} while() loop would have been much clearer.
> > 



> > > +		case RC6_MODE_6A:
> > > +			if (data->wanted_bits == RC6_6A_LARGE_NBITS) {
> > > +				toggle = data->body & RC6_6A_MCE_TOGGLE_MASK ? 1 : 0;
> > > +				scancode = data->body & ~RC6_6A_MCE_TOGGLE_MASK;
> > 
> > Technically this depends on the OEM.  
> 
> I know, again it's mentioned in one of the first comments of the code.
> 
> > In reality, every RC6 Mode 6A
> > remote that wants to work with Microsoft stock drivers will likely use
> > this bit as a toggle.
> 
> We might want to check if the (long) customer code matches 0x800F (if I 
> remember correctly) and apply the MCE behaviour only then, but it might 
> also come back to bite us in the ass if, for example, different 
> customers have implemented the same definition of the body.
> 
> Then again, as far as I can remeber, the Philips Pronto raw code for MCE 
> commands doesn't include the customer code, which would indicate that 
> it's fixed.
> 
> Another question is: if the customer code doesn't match, what do we do?  
> The body could be defined as any random gibberish by vendor XYZ, 
> including toggles and checksums which means that the scancode would 
> change for each keypress if we blindly report the entire body as a 32 
> bit scancode.

This is a standards problem that Phillips and the OEMs have left us with
(since OEM encodings will likely be confidential/proprietary).

Maybe this code should check for long customer id 0x800f, and if the ID
doesn't match, log a warning, but keep on processing as if it were.
Users will report the warning to get it removed for their remote.

Or is that too user-unfriendly?

Regards,
Andy

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] Add RC6 support to ir-core
  2010-04-10  1:12   ` Andy Walls
  2010-04-10 19:52     ` David Härdeman
@ 2010-04-10 22:23     ` David Härdeman
  1 sibling, 0 replies; 19+ messages in thread
From: David Härdeman @ 2010-04-10 22:23 UTC (permalink / raw)
  To: Andy Walls; +Cc: mchehab, linux-input, linux-media

On Fri, Apr 09, 2010 at 09:12:08PM -0400, Andy Walls wrote:
> On Fri, 2010-04-09 at 01:04 +0200, David Härdeman wrote:
> > +again:
> > +	IR_dprintk(2, "RC6 decode started at state %i (%i units, %ius)\n",
> > +		   data->state, u, TO_US(duration));
> > +
> > +	if (DURATION(u) == 0 && data->state != STATE_FINISHED)
> > +		return 0;
> 
> Isn't there a better way to structure the logic to break up two adjacent
> pulse units than with goto's out of the switch back up to here?
> 
> A do {} while() loop would have been much clearer.

I just tried it, and I'm not convinced. The main problem is that you'll 
end up with:

do {
	switch(b) {
	case c:
		if (x)
			break;
		else if (y)
			continue;
while(a);

Where the break statement will affect the switch() and the continue 
statement will affect the do-while() loop which is kinda confusing.

Especially if you're so far down in the function body that the 
do-while() and switch() statements aren't visible any more.


-- 
David Härdeman

^ permalink raw reply	[flat|nested] 19+ messages in thread
end of thread, other threads:[~2010-04-10 22:23 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-04-08 23:04 [PATCH 0/4] ir-core: one-line bugfix, add RC5x, NECx, RC6 David Härdeman
2010-04-08 23:04 ` [PATCH 1/4] Fix the drivers/media/dvb/ttpci/budget-ci.c conversion to ir-core David Härdeman
2010-04-08 23:04   ` David Härdeman
2010-04-08 23:11   ` Mauro Carvalho Chehab
2010-04-08 23:11     ` Mauro Carvalho Chehab
2010-04-08 23:04 ` [PATCH 2/4] Add RC5x support " David Härdeman
2010-04-08 23:04   ` David Härdeman
2010-04-08 23:04 ` [PATCH 3/4] Add NECx " David Härdeman
2010-04-08 23:04 ` [PATCH 4/4] Add RC6 " David Härdeman
2010-04-09  8:17   ` Andreas Oberritter
2010-04-09  8:17     ` Andreas Oberritter
2010-04-09  8:31     ` David Härdeman
2010-04-09 12:43       ` Mauro Carvalho Chehab
2010-04-09 12:43         ` Mauro Carvalho Chehab
2010-04-10  1:12   ` Andy Walls
2010-04-10 19:52     ` David Härdeman
2010-04-10 21:19       ` Andy Walls
2010-04-10 21:19         ` Andy Walls
2010-04-10 22:23     ` David Härdeman

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.