Re: [PATCH 4/4] Add RC6 support to ir-core

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxx>

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@xxxxxxxxxxx>
> + *
> + * 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@xxxxxxxxxxx>");
> +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@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux