ir-core sysfs protocol selection simplification

With the current logic, each raw decoder needs to add a copy of the exact same sysfs code. This is both unnecessary and also means that (re)loading an IR driver after raw decoder modules have been loaded won't work as expected.

This patch moves that logic into ir-raw-event and adds a single sysfs file per device.

Reading that file returns something like:

	"rc5 [rc6] nec jvc [sony]"

(with enabled protocols in [] brackets)

Writing either "+protocol" or "-protocol" to that file will enable or disable the according protocol decoder.

An additional benefit is that the disabling of a decoder will be remembered across module removal/insertion so a previously disabled decoder won't suddenly be activated again. The default setting is to enable all decoders.

This is also necessary for the next patch which moves even more decoder state into the central raw decoding structs.

Hi David,

Quoted 22 lines · David Härdeman

With the current logic, each raw decoder needs to add a copy of the exact same sysfs code. This is both unnecessary and also means that (re)loading an IR driver after raw decoder modules have been loaded won't work as expected.

This patch moves that logic into ir-raw-event and adds a single sysfs file per device.

Reading that file returns something like:

	"rc5 [rc6] nec jvc [sony]"

(with enabled protocols in [] brackets)

Writing either "+protocol" or "-protocol" to that file will enable or disable the according protocol decoder.

An additional benefit is that the disabling of a decoder will be remembered across module removal/insertion so a previously disabled decoder won't suddenly be activated again. The default setting is to enable all decoders.

Quoted 2 lines

This is also necessary for the next patch which moves even more decoder state into the central raw decoding structs.

I liked the idea of your redesign, but I didn't like the removal of a per-decoder sysfs entry. As already discussed, there are cases where we'll need a per-decoder sysfs entry (lirc_dev is probably one of those cases - also Jarod's imon driver is currently implementing a modprobe parameter that needs to be moved to the driver).

So, while we can implement some core support at the raw event core, we should keep the decoder attributes internally inside the driver.

So, each decoder
may have his own code like:

static struct attribute *decoder_attributes[] = {
       &dev_attr_enabled.attr,
       &dev_attr_bar1.attr,
       &dev_attr_bar2.attr,
       &dev_attr_bar3.attr,
       NULL
};

static struct attribute_group decoder_attribute_group = {
.name = "foo_decoder",
.attrs = decoder_attributes,
};

As the attr_enabled is common to all, maybe we can have a default enable code at the .h or inside the core.

This patch moves the state from each raw decoder into the ir_raw_event_ctrl struct.

This allows the removal of code like this:

        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;

which is currently run for each decoder on each event in order to get the client-specific decoding state data.

In addition, ir decoding modules and ir driver module load order is now independent. Centralizing the data also allows for a nice code reduction of about 30% per raw decoder as client lists and client registration callbacks are no longer necessary.

Out-of-tree modules can still use a similar trick to what the raw decoders did before this patch until they are merged.

Signed-off-by David Härdeman <[email protected]>

---
 drivers/media/IR/ir-core-priv.h    |   37 ++++++++++++-
 drivers/media/IR/ir-jvc-decoder.c  |   90 ++-----------------------------
 drivers/media/IR/ir-nec-decoder.c  |   89 +++----------------------------
 drivers/media/IR/ir-raw-event.c    |   48 +++--------------
 drivers/media/IR/ir-rc5-decoder.c  |  103 +++++-------------------------------
 drivers/media/IR/ir-rc6-decoder.c  |   92 ++------------------------------
 drivers/media/IR/ir-sony-decoder.c |   93 +++------------------------------
 7 files changed, 87 insertions(+), 465 deletions(-)

drivers/media/IR/ir-core-priv.h +35 -2

index 821d012..1e9464a 100644
@@ -23,8 +23,6 @@ struct ir_raw_handler {
 
 	u64 protocols; /* which are handled by this handler */
 	int (*decode)(struct input_dev *input_dev, struct ir_raw_event event);
-	int (*raw_register)(struct input_dev *input_dev);
-	int (*raw_unregister)(struct input_dev *input_dev);
 };
 
 struct ir_raw_event_ctrl {

@@ -34,6 +32,41 @@ struct ir_raw_event_ctrl {
 	enum raw_event_type		last_type;	/* last event type */
 	struct input_dev		*input_dev;	/* pointer to the parent input_dev */
 	u64				enabled_protocols; /* enabled raw protocol decoders */
+
+	/* raw decoder state follows */
+	struct ir_raw_event prev_ev;
+	struct nec_dec {
+		int state;
+		unsigned count;
+		u32 bits;
+	} nec;
+	struct rc5_dec {
+		int state;
+		u32 bits;
+		unsigned count;
+		unsigned wanted_bits;
+	} rc5;
+	struct rc6_dec {
+		int state;
+		u8 header;
+		u32 body;
+		bool toggle;
+		unsigned count;
+		unsigned wanted_bits;
+	} rc6;
+	struct sony_dec {
+		int state;
+		u32 bits;
+		unsigned count;
+	} sony;
+	struct jvc_dec {
+		int state;
+		u16 bits;
+		u16 old_bits;
+		unsigned count;
+		bool first;
+		bool toggle;
+	} jvc;
 };
 
 /* macros for IR decoders */

drivers/media/IR/ir-jvc-decoder.c +7 -83

index 1055de4..8894d8b 100644
@@ -25,10 +25,6 @@
 #define JVC_TRAILER_PULSE	(1  * JVC_UNIT)
 #define	JVC_TRAILER_SPACE	(35 * JVC_UNIT)
 
-/* Used to register jvc_decoder clients */
-static LIST_HEAD(decoder_list);
-DEFINE_SPINLOCK(decoder_lock);
-
 enum jvc_state {
 	STATE_INACTIVE,
 	STATE_HEADER_SPACE,

@@ -38,39 +34,6 @@ enum jvc_state {
 	STATE_TRAILER_SPACE,
 };
 
-struct decoder_data {
-	struct list_head	list;
-	struct ir_input_dev	*ir_dev;
-
-	/* State machine control */
-	enum jvc_state		state;
-	u16			jvc_bits;
-	u16			jvc_old_bits;
-	unsigned		count;
-	bool			first;
-	bool			toggle;
-};
-
-
-/**
- * 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;
-}
-
 /**
  * ir_jvc_decode() - Decode one JVC pulse or space
  * @input_dev:	the struct input_dev descriptor of the device

@@ -80,12 +43,8 @@ static struct decoder_data *get_decoder_data(struct  ir_input_dev *ir_dev)
  */
 static int ir_jvc_decode(struct input_dev *input_dev, struct ir_raw_event ev)
 {
-	struct decoder_data *data;
 	struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
-
-	data = get_decoder_data(ir_dev);
-	if (!data)
-		return -EINVAL;
+	struct jvc_dec *data = &ir_dev->raw->jvc;
 
 	if (!(ir_dev->raw->enabled_protocols & IR_TYPE_JVC))
 		return 0;

@@ -140,9 +99,9 @@ static int ir_jvc_decode(struct input_dev *input_dev, struct ir_raw_event ev)
 		if (ev.pulse)
 			break;
 
-		data->jvc_bits <<= 1;
+		data->bits <<= 1;
 		if (eq_margin(ev.duration, JVC_BIT_1_SPACE, JVC_UNIT / 2)) {
-			data->jvc_bits |= 1;
+			data->bits |= 1;
 			decrease_duration(&ev, JVC_BIT_1_SPACE);
 		} else if (eq_margin(ev.duration, JVC_BIT_0_SPACE, JVC_UNIT / 2))
 			decrease_duration(&ev, JVC_BIT_0_SPACE);

@@ -175,13 +134,13 @@ static int ir_jvc_decode(struct input_dev *input_dev, struct ir_raw_event ev)
 
 		if (data->first) {
 			u32 scancode;
-			scancode = (bitrev8((data->jvc_bits >> 8) & 0xff) << 8) |
-				   (bitrev8((data->jvc_bits >> 0) & 0xff) << 0);
+			scancode = (bitrev8((data->bits >> 8) & 0xff) << 8) |
+				   (bitrev8((data->bits >> 0) & 0xff) << 0);
 			IR_dprintk(1, "JVC scancode 0x%04x\n", scancode);
 			ir_keydown(input_dev, scancode, data->toggle);
 			data->first = false;
-			data->jvc_old_bits = data->jvc_bits;
-		} else if (data->jvc_bits == data->jvc_old_bits) {
+			data->old_bits = data->bits;
+		} else if (data->bits == data->old_bits) {
 			IR_dprintk(1, "JVC repeat\n");
 			ir_repeat(input_dev);
 		} else {

@@ -201,44 +160,9 @@ out:
 	return -EINVAL;
 }
 
-static int ir_jvc_register(struct input_dev *input_dev)
-{
-	struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
-	struct decoder_data *data;
-
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
-	data->ir_dev = ir_dev;
-	spin_lock(&decoder_lock);
-	list_add_tail(&data->list, &decoder_list);
-	spin_unlock(&decoder_lock);
-
-	return 0;
-}
-
-static int ir_jvc_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;
-
-	spin_lock(&decoder_lock);
-	list_del(&data->list);
-	spin_unlock(&decoder_lock);
-
-	return 0;
-}
-
 static struct ir_raw_handler jvc_handler = {
 	.protocols	= IR_TYPE_JVC,
 	.decode		= ir_jvc_decode,
-	.raw_register	= ir_jvc_register,
-	.raw_unregister	= ir_jvc_unregister,
 };
 
 static int __init ir_jvc_decode_init(void)

drivers/media/IR/ir-nec-decoder.c +8 -81

index 2cc2b92..52e0f37 100644
@@ -27,10 +27,6 @@
 #define	NEC_TRAILER_PULSE	(1  * NEC_UNIT)
 #define	NEC_TRAILER_SPACE	(10 * NEC_UNIT) /* even longer in reality */
 
-/* Used to register nec_decoder clients */
-static LIST_HEAD(decoder_list);
-static DEFINE_SPINLOCK(decoder_lock);
-
 enum nec_state {
 	STATE_INACTIVE,
 	STATE_HEADER_SPACE,

@@ -40,36 +36,6 @@ enum nec_state {
 	STATE_TRAILER_SPACE,
 };
 
-struct decoder_data {
-	struct list_head	list;
-	struct ir_input_dev	*ir_dev;
-
-	/* State machine control */
-	enum nec_state		state;
-	u32			nec_bits;
-	unsigned		count;
-};
-
-
-/**
- * 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;
-}
-
 /**
  * ir_nec_decode() - Decode one NEC pulse or space
  * @input_dev:	the struct input_dev descriptor of the device

@@ -79,15 +45,11 @@ static struct decoder_data *get_decoder_data(struct  ir_input_dev *ir_dev)
  */
 static int ir_nec_decode(struct input_dev *input_dev, struct ir_raw_event ev)
 {
-	struct decoder_data *data;
 	struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
+	struct nec_dec *data = &ir_dev->raw->nec;
 	u32 scancode;
 	u8 address, not_address, command, not_command;
 
-	data = get_decoder_data(ir_dev);
-	if (!data)
-		return -EINVAL;
-
 	if (!(ir_dev->raw->enabled_protocols & IR_TYPE_NEC))
 		return 0;
 

@@ -143,9 +105,9 @@ static int ir_nec_decode(struct input_dev *input_dev, struct ir_raw_event ev)
 		if (ev.pulse)
 			break;
 
-		data->nec_bits <<= 1;
+		data->bits <<= 1;
 		if (eq_margin(ev.duration, NEC_BIT_1_SPACE, NEC_UNIT / 2))
-			data->nec_bits |= 1;
+			data->bits |= 1;
 		else if (!eq_margin(ev.duration, NEC_BIT_0_SPACE, NEC_UNIT / 2))
 			break;
 		data->count++;

@@ -174,14 +136,14 @@ static int ir_nec_decode(struct input_dev *input_dev, struct ir_raw_event ev)
 		if (!geq_margin(ev.duration, NEC_TRAILER_SPACE, NEC_UNIT / 2))
 			break;
 
-		address     = bitrev8((data->nec_bits >> 24) & 0xff);
-		not_address = bitrev8((data->nec_bits >> 16) & 0xff);
-		command	    = bitrev8((data->nec_bits >>  8) & 0xff);
-		not_command = bitrev8((data->nec_bits >>  0) & 0xff);
+		address     = bitrev8((data->bits >> 24) & 0xff);
+		not_address = bitrev8((data->bits >> 16) & 0xff);
+		command	    = bitrev8((data->bits >>  8) & 0xff);
+		not_command = bitrev8((data->bits >>  0) & 0xff);
 
 		if ((command ^ not_command) != 0xff) {
 			IR_dprintk(1, "NEC checksum error: received 0x%08x\n",
-				   data->nec_bits);
+				   data->bits);
 			break;
 		}
 

@@ -208,44 +170,9 @@ static int ir_nec_decode(struct input_dev *input_dev, struct ir_raw_event ev)
 	return -EINVAL;
 }
 
-static int ir_nec_register(struct input_dev *input_dev)
-{
-	struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
-	struct decoder_data *data;
-
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
-	data->ir_dev = ir_dev;
-	spin_lock(&decoder_lock);
-	list_add_tail(&data->list, &decoder_list);
-	spin_unlock(&decoder_lock);
-
-	return 0;
-}
-
-static int ir_nec_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;
-
-	spin_lock(&decoder_lock);
-	list_del(&data->list);
-	spin_unlock(&decoder_lock);
-
-	return 0;
-}
-
 static struct ir_raw_handler nec_handler = {
 	.protocols	= IR_TYPE_NEC,
 	.decode		= ir_nec_decode,
-	.raw_register	= ir_nec_register,
-	.raw_unregister	= ir_nec_unregister,
 };
 
 static int __init ir_nec_decode_init(void)

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

index eeca8a8..eb77378 100644
@@ -25,32 +25,6 @@ static DEFINE_SPINLOCK(ir_raw_handler_lock);
 static LIST_HEAD(ir_raw_handler_list);
 static u64 available_protocols;
 
-/**
- * RUN_DECODER()	- runs an operation on all IR decoders
- * @ops:	IR raw handler operation to be called
- * @arg:	arguments to be passed to the callback
- *
- * Calls ir_raw_handler::ops for all registered IR handlers. It prevents
- * new decode addition/removal while running, by locking ir_raw_handler_lock
- * mutex. If an error occurs, it stops the ops. Otherwise, it returns a sum
- * of the return codes.
- */
-#define RUN_DECODER(ops, ...) ({					    \
-	struct ir_raw_handler		*_ir_raw_handler;		    \
-	int _sumrc = 0, _rc;						    \
-	spin_lock(&ir_raw_handler_lock);				    \
-	list_for_each_entry(_ir_raw_handler, &ir_raw_handler_list, list) {  \
-		if (_ir_raw_handler->ops) {				    \
-			_rc = _ir_raw_handler->ops(__VA_ARGS__);	    \
-			if (_rc < 0)					    \
-				break;					    \
-			_sumrc += _rc;					    \
-		}							    \
-	}								    \
-	spin_unlock(&ir_raw_handler_lock);				    \
-	_sumrc;								    \
-})
-
 #ifdef MODULE
 /* Used to load the decoders */
 static struct work_struct wq_load;

@@ -59,11 +33,17 @@ static struct work_struct wq_load;
 static void ir_raw_event_work(struct work_struct *work)
 {
 	struct ir_raw_event ev;
+	struct ir_raw_handler *handler;
 	struct ir_raw_event_ctrl *raw =
 		container_of(work, struct ir_raw_event_ctrl, rx_work);
 
-	while (kfifo_out(&raw->kfifo, &ev, sizeof(ev)) == sizeof(ev))
-		RUN_DECODER(decode, raw->input_dev, ev);
+	while (kfifo_out(&raw->kfifo, &ev, sizeof(ev)) == sizeof(ev)) {
+		spin_lock(&ir_raw_handler_lock);
+		list_for_each_entry(handler, &ir_raw_handler_list, list)
+			handler->decode(raw->input_dev, ev);
+		spin_unlock(&ir_raw_handler_lock);
+		raw->prev_ev = ev;
+	}
 }
 
 /**

@@ -193,15 +173,7 @@ int ir_raw_event_register(struct input_dev *input_dev)
 		return rc;
 	}
 
-	rc = RUN_DECODER(raw_register, input_dev);
-	if (rc < 0) {
-		kfifo_free(&ir->raw->kfifo);
-		kfree(ir->raw);
-		ir->raw = NULL;
-		return rc;
-	}
-
-	return rc;
+	return 0;
 }
 
 void ir_raw_event_unregister(struct input_dev *input_dev)

@@ -212,8 +184,6 @@ void ir_raw_event_unregister(struct input_dev *input_dev)
 		return;
 
 	cancel_work_sync(&ir->raw->rx_work);
-	RUN_DECODER(raw_unregister, input_dev);
-
 	kfifo_free(&ir->raw->kfifo);
 	kfree(ir->raw);
 	ir->raw = NULL;

drivers/media/IR/ir-rc5-decoder.c +13 -90

index 1be8981..7af656d 100644
@@ -30,10 +30,6 @@
 #define RC5_BIT_END		(1 * RC5_UNIT)
 #define RC5X_SPACE		(4 * RC5_UNIT)
 
-/* Used to register rc5_decoder clients */
-static LIST_HEAD(decoder_list);
-static DEFINE_SPINLOCK(decoder_lock);
-
 enum rc5_state {
 	STATE_INACTIVE,
 	STATE_BIT_START,

@@ -42,39 +38,6 @@ enum rc5_state {
 	STATE_FINISHED,
 };
 
-struct decoder_data {
-	struct list_head	list;
-	struct ir_input_dev	*ir_dev;
-
-	/* State machine control */
-	enum rc5_state		state;
-	u32			rc5_bits;
-	struct ir_raw_event	prev_ev;
-	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;
-}
-
 /**
  * ir_rc5_decode() - Decode one RC-5 pulse or space
  * @input_dev:	the struct input_dev descriptor of the device

@@ -84,15 +47,11 @@ static struct decoder_data *get_decoder_data(struct  ir_input_dev *ir_dev)
  */
 static int ir_rc5_decode(struct input_dev *input_dev, struct ir_raw_event ev)
 {
-	struct decoder_data *data;
 	struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
+	struct rc5_dec *data = &ir_dev->raw->rc5;
 	u8 toggle;
 	u32 scancode;
 
-	data = get_decoder_data(ir_dev);
-	if (!data)
-		return -EINVAL;
-
 	if (!(ir_dev->raw->enabled_protocols & IR_TYPE_RC5))
 		return 0;
 

@@ -128,16 +87,15 @@ again:
 		if (!eq_margin(ev.duration, RC5_BIT_START, RC5_UNIT / 2))
 			break;
 
-		data->rc5_bits <<= 1;
+		data->bits <<= 1;
 		if (!ev.pulse)
-			data->rc5_bits |= 1;
+			data->bits |= 1;
 		data->count++;
-		data->prev_ev = ev;
 		data->state = STATE_BIT_END;
 		return 0;
 
 	case STATE_BIT_END:
-		if (!is_transition(&ev, &data->prev_ev))
+		if (!is_transition(&ev, &ir_dev->raw->prev_ev))
 			break;
 
 		if (data->count == data->wanted_bits)

@@ -169,11 +127,11 @@ again:
 		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;
+			xdata    = (data->bits & 0x0003F) >> 0;
+			command  = (data->bits & 0x00FC0) >> 6;
+			system   = (data->bits & 0x1F000) >> 12;
+			toggle   = (data->bits & 0x20000) ? 1 : 0;
+			command += (data->bits & 0x01000) ? 0 : 0x40;
 			scancode = system << 16 | command << 8 | xdata;
 
 			IR_dprintk(1, "RC5X scancode 0x%06x (toggle: %u)\n",

@@ -182,10 +140,10 @@ again:
 		} 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;
+			command  = (data->bits & 0x0003F) >> 0;
+			system   = (data->bits & 0x007C0) >> 6;
+			toggle   = (data->bits & 0x00800) ? 1 : 0;
+			command += (data->bits & 0x01000) ? 0 : 0x40;
 			scancode = system << 8 | command;
 
 			IR_dprintk(1, "RC5 scancode 0x%04x (toggle: %u)\n",

@@ -204,44 +162,9 @@ out:
 	return -EINVAL;
 }
 
-static int ir_rc5_register(struct input_dev *input_dev)
-{
-	struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
-	struct decoder_data *data;
-
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
-	data->ir_dev = ir_dev;
-	spin_lock(&decoder_lock);
-	list_add_tail(&data->list, &decoder_list);
-	spin_unlock(&decoder_lock);
-
-	return 0;
-}
-
-static int ir_rc5_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;
-
-	spin_lock(&decoder_lock);
-	list_del(&data->list);
-	spin_unlock(&decoder_lock);
-
-	return 0;
-}
-
 static struct ir_raw_handler rc5_handler = {
 	.protocols	= IR_TYPE_RC5,
 	.decode		= ir_rc5_decode,
-	.raw_register	= ir_rc5_register,
-	.raw_unregister	= ir_rc5_unregister,
 };
 
 static int __init ir_rc5_decode_init(void)

drivers/media/IR/ir-rc6-decoder.c +5 -87

index 5e940a8..a562952 100644
@@ -36,10 +36,6 @@
 #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,

@@ -58,41 +54,7 @@ enum rc6_state {
 	STATE_FINISHED,
 };
 
-struct decoder_data {
-	struct list_head	list;
-	struct ir_input_dev	*ir_dev;
-
-	/* State machine control */
-	enum rc6_state		state;
-	u8			header;
-	u32			body;
-	struct ir_raw_event	prev_ev;
-	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 enum rc6_mode rc6_mode(struct decoder_data *data) {
+static enum rc6_mode rc6_mode(struct rc6_dec *data) {
 	switch (data->header & RC6_MODE_MASK) {
 	case 0:
 		return RC6_MODE_0;

@@ -114,15 +76,11 @@ static enum rc6_mode rc6_mode(struct decoder_data *data) {
  */
 static int ir_rc6_decode(struct input_dev *input_dev, struct ir_raw_event ev)
 {
-	struct decoder_data *data;
 	struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
+	struct rc6_dec *data = &ir_dev->raw->rc6;
 	u32 scancode;
 	u8 toggle;
 
-	data = get_decoder_data(ir_dev);
-	if (!data)
-		return -EINVAL;
-
 	if (!(ir_dev->raw->enabled_protocols & IR_TYPE_RC6))
 		return 0;
 

@@ -175,12 +133,11 @@ again:
 		if (ev.pulse)
 			data->header |= 1;
 		data->count++;
-		data->prev_ev = ev;
 		data->state = STATE_HEADER_BIT_END;
 		return 0;
 
 	case STATE_HEADER_BIT_END:
-		if (!is_transition(&ev, &data->prev_ev))
+		if (!is_transition(&ev, &ir_dev->raw->prev_ev))
 			break;
 
 		if (data->count == RC6_HEADER_NBITS)

@@ -196,12 +153,11 @@ again:
 			break;
 
 		data->toggle = ev.pulse;
-		data->prev_ev = ev;
 		data->state = STATE_TOGGLE_END;
 		return 0;
 
 	case STATE_TOGGLE_END:
-		if (!is_transition(&ev, &data->prev_ev) ||
+		if (!is_transition(&ev, &ir_dev->raw->prev_ev) ||
 		    !geq_margin(ev.duration, RC6_TOGGLE_END, RC6_UNIT / 2))
 			break;
 

@@ -211,7 +167,6 @@ again:
 		}
 
 		data->state = STATE_BODY_BIT_START;
-		data->prev_ev = ev;
 		decrease_duration(&ev, RC6_TOGGLE_END);
 		data->count = 0;
 

@@ -243,13 +198,11 @@ again:
 		if (ev.pulse)
 			data->body |= 1;
 		data->count++;
-		data->prev_ev = ev;
-
 		data->state = STATE_BODY_BIT_END;
 		return 0;
 
 	case STATE_BODY_BIT_END:
-		if (!is_transition(&ev, &data->prev_ev))
+		if (!is_transition(&ev, &ir_dev->raw->prev_ev))
 			break;
 
 		if (data->count == data->wanted_bits)

@@ -300,44 +253,9 @@ out:
 	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;
-
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
-	data->ir_dev = ir_dev;
-	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;
-
-	spin_lock(&decoder_lock);
-	list_del(&data->list);
-	spin_unlock(&decoder_lock);
-
-	return 0;
-}
-
 static struct ir_raw_handler rc6_handler = {
 	.protocols	= IR_TYPE_RC6,
 	.decode		= ir_rc6_decode,
-	.raw_register	= ir_rc6_register,
-	.raw_unregister	= ir_rc6_unregister,
 };
 
 static int __init ir_rc6_decode_init(void)

drivers/media/IR/ir-sony-decoder.c +10 -84

index 8afd16a..b9074f0 100644
@@ -23,10 +23,6 @@
 #define SONY_BIT_SPACE		(1 * SONY_UNIT)
 #define SONY_TRAILER_SPACE	(10 * SONY_UNIT) /* minimum */
 
-/* Used to register sony_decoder clients */
-static LIST_HEAD(decoder_list);
-static DEFINE_SPINLOCK(decoder_lock);
-
 enum sony_state {
 	STATE_INACTIVE,
 	STATE_HEADER_SPACE,

@@ -35,36 +31,6 @@ enum sony_state {
 	STATE_FINISHED,
 };
 
-struct decoder_data {
-	struct list_head	list;
-	struct ir_input_dev	*ir_dev;
-
-	/* State machine control */
-	enum sony_state		state;
-	u32			sony_bits;
-	unsigned		count;
-};
-
-
-/**
- * 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;
-}
-
 /**
  * ir_sony_decode() - Decode one Sony pulse or space
  * @input_dev:	the struct input_dev descriptor of the device

@@ -74,15 +40,11 @@ static struct decoder_data *get_decoder_data(struct  ir_input_dev *ir_dev)
  */
 static int ir_sony_decode(struct input_dev *input_dev, struct ir_raw_event ev)
 {
-	struct decoder_data *data;
 	struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
+	struct sony_dec *data = &ir_dev->raw->sony;
 	u32 scancode;
 	u8 device, subdevice, function;
 
-	data = get_decoder_data(ir_dev);
-	if (!data)
-		return -EINVAL;
-
 	if (!(ir_dev->raw->enabled_protocols & IR_TYPE_SONY))
 		return 0;
 

@@ -124,9 +86,9 @@ static int ir_sony_decode(struct input_dev *input_dev, struct ir_raw_event ev)
 		if (!ev.pulse)
 			break;
 
-		data->sony_bits <<= 1;
+		data->bits <<= 1;
 		if (eq_margin(ev.duration, SONY_BIT_1_PULSE, SONY_UNIT / 2))
-			data->sony_bits |= 1;
+			data->bits |= 1;
 		else if (!eq_margin(ev.duration, SONY_BIT_0_PULSE, SONY_UNIT / 2))
 			break;
 

@@ -160,19 +122,19 @@ static int ir_sony_decode(struct input_dev *input_dev, struct ir_raw_event ev)
 
 		switch (data->count) {
 		case 12:
-			device    = bitrev8((data->sony_bits <<  3) & 0xF8);
+			device    = bitrev8((data->bits <<  3) & 0xF8);
 			subdevice = 0;
-			function  = bitrev8((data->sony_bits >>  4) & 0xFE);
+			function  = bitrev8((data->bits >>  4) & 0xFE);
 			break;
 		case 15:
-			device    = bitrev8((data->sony_bits >>  0) & 0xFF);
+			device    = bitrev8((data->bits >>  0) & 0xFF);
 			subdevice = 0;
-			function  = bitrev8((data->sony_bits >>  7) & 0xFD);
+			function  = bitrev8((data->bits >>  7) & 0xFD);
 			break;
 		case 20:
-			device    = bitrev8((data->sony_bits >>  5) & 0xF8);
-			subdevice = bitrev8((data->sony_bits >>  0) & 0xFF);
-			function  = bitrev8((data->sony_bits >> 12) & 0xFE);
+			device    = bitrev8((data->bits >>  5) & 0xF8);
+			subdevice = bitrev8((data->bits >>  0) & 0xFF);
+			function  = bitrev8((data->bits >> 12) & 0xFE);
 			break;
 		default:
 			IR_dprintk(1, "Sony invalid bitcount %u\n", data->count);

@@ -193,44 +155,9 @@ out:
 	return -EINVAL;
 }
 
-static int ir_sony_register(struct input_dev *input_dev)
-{
-	struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
-	struct decoder_data *data;
-
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
-	data->ir_dev = ir_dev;
-	spin_lock(&decoder_lock);
-	list_add_tail(&data->list, &decoder_list);
-	spin_unlock(&decoder_lock);
-
-	return 0;
-}
-
-static int ir_sony_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;
-
-	spin_lock(&decoder_lock);
-	list_del(&data->list);
-	spin_unlock(&decoder_lock);
-
-	return 0;
-}
-
 static struct ir_raw_handler sony_handler = {
 	.protocols	= IR_TYPE_SONY,
 	.decode		= ir_sony_decode,
-	.raw_register	= ir_sony_register,
-	.raw_unregister	= ir_sony_unregister,
 };
 
 static int __init ir_sony_decode_init(void)
 
--

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

Quoted 13 lines · David Härdeman

This patch moves the state from each raw decoder into the ir_raw_event_ctrl struct.

This allows the removal of code like this:

        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;

Quoted 8 lines

which is currently run for each decoder on each event in order to get the client-specific decoding state data.

In addition, ir decoding modules and ir driver module load order is now independent. Centralizing the data also allows for a nice code reduction of about 30% per raw decoder as client lists and client registration callbacks are no longer necessary.

The registration callbacks will likely still be needed by lirc, as you need to create/delete lirc_dev interfaces, when the module is registered, but I might be wrong. It would be interesting to add lirc_dev first, in order to be sure about the better interfaces for it.

Also, from one side, you reduced the code size, but, on the other hand, you've increased the memory usage, as now the protocol data will be stored even for protocols that weren't compiled/loaded.

Quoted 6 lines

Quoted 5 lines · Andy Walls

Encoding pulse vs space with a negative sign, even if now hidden with macros, is still just using a sign instead of a boolean. Memory in modern computers (and now microcontrollers) is cheap and only getting cheaper. Don't give up readability, flexibility, or mainatainability, for the sake of saving memory.

That was my point since the beginning: the amount of saved memory doesn't justify the lack of readability.

Probably, the code size savings are big enough to justify the runtime memory footprint, at least with the current decoders. Not sure how big this will become when we add lirc_dev and other decoders that might be missing.

Quoted 23 lines · Mauro Carvalho Chehab

Quoted 13 lines · David Härdeman

This patch moves the state from each raw decoder into the ir_raw_event_ctrl struct.

This allows the removal of code like this:

        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;

Quoted 8 lines

which is currently run for each decoder on each event in order to get the client-specific decoding state data.

In addition, ir decoding modules and ir driver module load order is now independent. Centralizing the data also allows for a nice code reduction of about 30% per raw decoder as client lists and client registration callbacks are no longer necessary.

Quoted 5 lines

The registration callbacks will likely still be needed by lirc, as you need to create/delete lirc_dev interfaces, when the module is registered, but I might be wrong. It would be interesting to add lirc_dev first, in order to be sure about the better interfaces for it.

Or the lirc_dev patch can add whatever interfaces it needs. Anyway, the current interfaces are not good enough since it'll break if lirc_dev is loaded after the hardware modules.

In addition, random module load order is currently broken (try loading decoders first and hardware later and you'll see). With this patch, it works again.

Anyway, I'll post a new patch series this evening and then we can go back to our regular arguing :)

Quoted 30 lines · David Härdeman

Quoted 23 lines · Mauro Carvalho Chehab

Quoted 13 lines · David Härdeman

This patch moves the state from each raw decoder into the ir_raw_event_ctrl struct.

This allows the removal of code like this:

        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;

Quoted 8 lines

which is currently run for each decoder on each event in order to get the client-specific decoding state data.

In addition, ir decoding modules and ir driver module load order is now independent. Centralizing the data also allows for a nice code reduction of about 30% per raw decoder as client lists and client registration callbacks are no longer necessary.

Quoted 5 lines

The registration callbacks will likely still be needed by lirc, as you need to create/delete lirc_dev interfaces, when the module is registered, but I might be wrong. It would be interesting to add lirc_dev first, in order to be sure about the better interfaces for it.

Quoted 3 lines

Or the lirc_dev patch can add whatever interfaces it needs. Anyway, the current interfaces are not good enough since it'll break if lirc_dev is loaded after the hardware modules.

This is something I've been meaning to mention myself. On system boot, if an mceusb device is connected, it pretty regularly only has the NEC decoder available to use. I have to reload mceusb, or make sure ir-core is explicitly loaded, wait a bit, then load mceusb, if I want to have all of the protocol handlers available -- which includes the needed-by-default rc6 one. I've only briefly tinkered with trying to fix it, sounds like you may already have fixage within this patchset.

Quoted 3 lines

In addition, random module load order is currently broken (try loading decoders first and hardware later and you'll see). With this patch, it works again.

Want.

Quoted 35 lines · Jarod Wilson

Quoted 30 lines · David Härdeman

Quoted 23 lines · Mauro Carvalho Chehab

Quoted 13 lines · David Härdeman

This patch moves the state from each raw decoder into the ir_raw_event_ctrl struct.

This allows the removal of code like this:

        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;

Quoted 8 lines

which is currently run for each decoder on each event in order to get the client-specific decoding state data.

In addition, ir decoding modules and ir driver module load order is now independent. Centralizing the data also allows for a nice code reduction of about 30% per raw decoder as client lists and client registration callbacks are no longer necessary.

Quoted 5 lines

The registration callbacks will likely still be needed by lirc, as you need to create/delete lirc_dev interfaces, when the module is registered, but I might be wrong. It would be interesting to add lirc_dev first, in order to be sure about the better interfaces for it.

Quoted 3 lines

Or the lirc_dev patch can add whatever interfaces it needs. Anyway, the current interfaces are not good enough since it'll break if lirc_dev is loaded after the hardware modules.

Quoted 7 lines

This is something I've been meaning to mention myself. On system boot, if an mceusb device is connected, it pretty regularly only has the NEC decoder available to use. I have to reload mceusb, or make sure ir-core is explicitly loaded, wait a bit, then load mceusb, if I want to have all of the protocol handlers available -- which includes the needed-by-default rc6 one. I've only briefly tinkered with trying to fix it, sounds like you may already have fixage within this patchset.

The problem is that without the patchset, each decoder is expected to carry it's own list of datastructures for each hardware receiver. Hardware receiver addition/removal is signalled through a callback to the decoder, but the callback will (naturally) not be invoked if the hardware driver is already loaded when the decoder is. So loading a decoder "late" or reloading a decoder will mean that it doesn't know about pre-existing hardware.

Quoted 4 lines

Quoted 3 lines

In addition, random module load order is currently broken (try loading decoders first and hardware later and you'll see).  With this patch, it works again.

Quoted 1 line

Want.

Then please help me with two things:

a) Test the patches I just sent (especially 6/8 and 7/8, they should be independent from the rest)

b) Mauro mentioned in <[email protected]> that:

       I liked the idea of your redesign, but I didn't like the removal of a per-decoder sysfs entry. As already discussed, there are cases where we'll need a per-decoder sysfs entry (lirc_dev is probably one of those cases - also Jarod's imon driver is currently implementing a modprobe parameter that needs to be moved to the driver).

  could you please confirm if your lirc and/or imon drivers would be negatively affected by the proposed patches?

Quoted 56 lines · David Härdeman

Quoted 35 lines · Jarod Wilson

Quoted 30 lines · David Härdeman

Quoted 23 lines · Mauro Carvalho Chehab

Quoted 13 lines · David Härdeman

This patch moves the state from each raw decoder into the ir_raw_event_ctrl struct.

This allows the removal of code like this:

        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;

Quoted 8 lines

which is currently run for each decoder on each event in order to get the client-specific decoding state data.

In addition, ir decoding modules and ir driver module load order is now independent. Centralizing the data also allows for a nice code reduction of about 30% per raw decoder as client lists and client registration callbacks are no longer necessary.

Quoted 5 lines

The registration callbacks will likely still be needed by lirc, as you need to create/delete lirc_dev interfaces, when the module is registered, but I might be wrong. It would be interesting to add lirc_dev first, in order to be sure about the better interfaces for it.

Quoted 3 lines

Or the lirc_dev patch can add whatever interfaces it needs. Anyway, the current interfaces are not good enough since it'll break if lirc_dev is loaded after the hardware modules.

Quoted 7 lines

This is something I've been meaning to mention myself. On system boot, if an mceusb device is connected, it pretty regularly only has the NEC decoder available to use. I have to reload mceusb, or make sure ir-core is explicitly loaded, wait a bit, then load mceusb, if I want to have all of the protocol handlers available -- which includes the needed-by-default rc6 one. I've only briefly tinkered with trying to fix it, sounds like you may already have fixage within this patchset.

The problem is that without the patchset, each decoder is expected to carry it's own list of datastructures for each hardware receiver. Hardware receiver addition/removal is signalled through a callback to the decoder, but the callback will (naturally) not be invoked if the hardware driver is already loaded when the decoder is. So loading a decoder "late" or reloading a decoder will mean that it doesn't know about pre-existing hardware.

Quoted 4 lines

Quoted 3 lines

In addition, random module load order is currently broken (try loading decoders first and hardware later and you'll see).  With this patch, it works again.

Quoted 6 lines

Quoted 1 line

Want.

Then please help me with two things:

a) Test the patches I just sent (especially 6/8 and 7/8, they should be independent from the rest)

Working on merging them into a tree here locally. There's been a bit of churn, so the last few didn't apply cleanly, but I'm almost there.

Quoted 9 lines

b) Mauro mentioned in <[email protected]> that:

       I liked the idea of your redesign, but I didn't like the removal of a per-decoder sysfs entry. As already discussed, there are cases where we'll need a per-decoder sysfs entry (lirc_dev is probably one of those cases - also Jarod's imon driver is currently implementing a modprobe parameter that needs to be moved to the driver).

Quoted 2 lines

  could you please confirm if your lirc and/or imon drivers would be negatively affected by the proposed patches?

Will do so once I get them wedged in on top.

Quoted 10 lines · Jarod Wilson

Quoted 9 lines · David Härdeman

b) Mauro mentioned in <[email protected]> that:

       I liked the idea of your redesign, but I didn't like the removal of a per-decoder sysfs entry. As already discussed, there are cases where we'll need a per-decoder sysfs entry (lirc_dev is probably one of those cases - also Jarod's imon driver is currently implementing a modprobe parameter that needs to be moved to the driver).

Quoted 4 lines

Quoted 2 lines

  could you please confirm if your lirc and/or imon drivers would be negatively affected by the proposed patches?

Will do so once I get them wedged in on top.

Got it all merged and compiling, but not yet runtime tested. Compiling alone sheds some light on things though...

So this definitely negatively impacts my ir-core-to-lirc_dev (ir-lirc-codec.c) bridge driver, as it was doing the lirc_dev device registration work in its register function. However, if (after your patchset) we add a new pair of callbacks replacing raw_register and raw_unregister, which are optional, that work could be done there instead, so I don't think this is an insurmountable obstacle for the lirc bits.

As for the imon driver, the modprobe parameter in question (iirc) was already removed from the driver, as its functionality is replaced by implementing a change_protocol callback. However, there's a request to add it (or something like it) back to the driver to allow disabling the IR part altogether, and there are a few other modparams that might be better suited as sysfs entries. However, its actually not relevant to the case of registering raw protocol handlers, as the imon devices do their decoding in hardware. I can see the possibility for protocol-specific knobs in sysfs though. But I think the same optional callbacks I'd use to keep the lirc bits working could also be used for this. Can't think of a good name for these yet, probably need more coffee first... ;)

Quoted 19 lines · Jarod Wilson

Quoted 10 lines · Jarod Wilson

Quoted 9 lines · David Härdeman

b) Mauro mentioned in <[email protected]> that:

       I liked the idea of your redesign, but I didn't like the removal of a per-decoder sysfs entry. As already discussed, there are cases where we'll need a per-decoder sysfs entry (lirc_dev is probably one of those cases - also Jarod's imon driver is currently implementing a modprobe parameter that needs to be moved to the driver).

Quoted 4 lines

Quoted 2 lines

  could you please confirm if your lirc and/or imon drivers would be negatively affected by the proposed patches?

Will do so once I get them wedged in on top.

Got it all merged and compiling, but not yet runtime tested. Compiling alone sheds some light on things though...

Quoted 6 lines

So this definitely negatively impacts my ir-core-to-lirc_dev (ir-lirc-codec.c) bridge driver, as it was doing the lirc_dev device registration work in its register function. However, if (after your patchset) we add a new pair of callbacks replacing raw_register and raw_unregister, which are optional, that work could be done there instead, so I don't think this is an insurmountable obstacle for the lirc bits.

While I'm not sure exactly what callbacks you're suggesting,

it still
sounds like the callbacks would have the exact same problems that the current code has (i.e. the decoder will be blissfully unaware of hardware which exists before the decoder is loaded). Right?

Quoted 11 lines

As for the imon driver, the modprobe parameter in question (iirc) was already removed from the driver, as its functionality is replaced by implementing a change_protocol callback. However, there's a request to add it (or something like it) back to the driver to allow disabling the IR part altogether, and there are a few other modparams that might be better suited as sysfs entries. However, its actually not relevant to the case of registering raw protocol handlers, as the imon devices do their decoding in hardware. I can see the possibility for protocol-specific knobs in sysfs though. But I think the same optional callbacks I'd use to keep the lirc bits working could also be used for this. Can't think of a good name for these yet, probably need more coffee first... ;)

But those sysfs entries wouldn't be
per-decoder-per-hardware-device....they'd just be
per-hardware-device...right?

Quoted 1 line · David Härdeman

On Wed, Jun 09, 2010 at 09:29:08AM -0400, Jarod Wilson wrote:

Quoted 8 lines

Quoted 6 lines

So this definitely negatively impacts my ir-core-to-lirc_dev (ir-lirc-codec.c) bridge driver, as it was doing the lirc_dev device registration work in its register function. However, if (after your patchset) we add a new pair of callbacks replacing raw_register and raw_unregister, which are optional, that work could be done there instead, so I don't think this is an insurmountable obstacle for the lirc bits.

While I'm not sure exactly what callbacks you're suggesting,

Essentially:

.setup_other_crap
.tear_down_other_crap

...which in the ir-lirc-codec case, register ir-lirc-codec for a specific hardware receiver as an lirc_dev client, and conversely, tear it down.

Quoted 4 lines

it still
sounds like the callbacks would have the exact same problems that the current code has (i.e. the decoder will be blissfully unaware of hardware which exists before the decoder is loaded). Right?

In my head, this was going to work out, but you're correct, I still have the exact same problem -- its not in ir_raw_handler_list yet when ir_raw_event_register runs, and thus the callback never fires, so lirc_dev never actually gets wired up to ir-lirc-codec. It now knows about the lirc decoder, but its completely useless. Narf.

Quoted 2 lines · Jarod Wilson

Quoted 1 line · David Härdeman

On Wed, Jun 09, 2010 at 09:29:08AM -0400, Jarod Wilson wrote:

...

Quoted 22 lines

Quoted 8 lines

Quoted 6 lines

So this definitely negatively impacts my ir-core-to-lirc_dev (ir-lirc-codec.c) bridge driver, as it was doing the lirc_dev device registration work in its register function. However, if (after your patchset) we add a new pair of callbacks replacing raw_register and raw_unregister, which are optional, that work could be done there instead, so I don't think this is an insurmountable obstacle for the lirc bits.

While I'm not sure exactly what callbacks you're suggesting,

Essentially:

.setup_other_crap
.tear_down_other_crap

...which in the ir-lirc-codec case, register ir-lirc-codec for a specific hardware receiver as an lirc_dev client, and conversely, tear it down.

Quoted 4 lines

it still
sounds like the callbacks would have the exact same problems that the current code has (i.e. the decoder will be blissfully unaware of hardware which exists before the decoder is loaded). Right?

Quoted 5 lines

In my head, this was going to work out, but you're correct, I still have the exact same problem -- its not in ir_raw_handler_list yet when ir_raw_event_register runs, and thus the callback never fires, so lirc_dev never actually gets wired up to ir-lirc-codec. It now knows about the lirc decoder, but its completely useless. Narf.

And now I have it working atop your patches. Its a bit of a nasty-ish hack, at least for the lirc case, but its working, even in the case where the decoder drivers aren't actually loaded until after the device driver. I've added one extra param to each protocol-specific struct in ir-core-priv.h (bool initialized) and hooked into the protocol-specific decode functions to both determine whether a protocol should be enabled or disabled by default, and to run any additionally required initialization (such as in the ir-lirc-codec case).

So initially, mceusb comes up with all decoders enabled. Then when ir comes in, every protocol-specific decoder fires. Each of them check for whether or not they've been fully initialized, and if not, we check the loaded keymap, and if it doesn't match, we disable that decoder (bringing back the "disable protocol handlers we don't need" functionality that disappeared w/this patchset). In the lirc case, we actually do all the work needed to wire up the connection over to lirc_dev.

This works perfectly fine for all the in-kernel decoders, but has one minor shortcoming for ir-lirc-codec, in that /dev/lirc0 won't actually exist until the first incoming ir signal is seen. lircd can handle this case just fine, it'll wait for /dev/lirc0 to show up, but it doesn't come up fast enough to catch and decode the very first incoming ir signal. Subsequent ones work perfectly fine though. This need to initialize the link via incoming ir is a bit problematic if you're using a device for transmit-only (e.g., and mceusb device hooked to a mythtv backend in the closet or something), as there would be a strong possibility of /dev/lirc0 never getting hooked up. I can think of a few workarounds, but none are particularly clean and/or automagic.

Not sure how palatable it is, but here it is:

Quoted 64 lines · Jarod Wilson

Quoted 2 lines · Jarod Wilson

Quoted 1 line · David Härdeman

On Wed, Jun 09, 2010 at 09:29:08AM -0400, Jarod Wilson wrote:

...

Quoted 22 lines

Quoted 8 lines

Quoted 6 lines

So this definitely negatively impacts my ir-core-to-lirc_dev (ir-lirc-codec.c) bridge driver, as it was doing the lirc_dev device registration work in its register function. However, if (after your patchset) we add a new pair of callbacks replacing raw_register and raw_unregister, which are optional, that work could be done there instead, so I don't think this is an insurmountable obstacle for the lirc bits.

While I'm not sure exactly what callbacks you're suggesting,

Essentially:

.setup_other_crap
.tear_down_other_crap

...which in the ir-lirc-codec case, register ir-lirc-codec for a specific hardware receiver as an lirc_dev client, and conversely, tear it down.

Quoted 4 lines

it still
sounds like the callbacks would have the exact same problems that the current code has (i.e. the decoder will be blissfully unaware of hardware which exists before the decoder is loaded). Right?

Quoted 5 lines

In my head, this was going to work out, but you're correct, I still have the exact same problem -- its not in ir_raw_handler_list yet when ir_raw_event_register runs, and thus the callback never fires, so lirc_dev never actually gets wired up to ir-lirc-codec. It now knows about the lirc decoder, but its completely useless. Narf.

And now I have it working atop your patches. Its a bit of a nasty-ish hack, at least for the lirc case, but its working, even in the case where the decoder drivers aren't actually loaded until after the device driver. I've added one extra param to each protocol-specific struct in ir-core-priv.h (bool initialized) and hooked into the protocol-specific decode functions to both determine whether a protocol should be enabled or disabled by default, and to run any additionally required initialization (such as in the ir-lirc-codec case).

So initially, mceusb comes up with all decoders enabled. Then when ir comes in, every protocol-specific decoder fires. Each of them check for whether or not they've been fully initialized, and if not, we check the loaded keymap, and if it doesn't match, we disable that decoder (bringing back the "disable protocol handlers we don't need" functionality that disappeared w/this patchset). In the lirc case, we actually do all the work needed to wire up the connection over to lirc_dev.

This works perfectly fine for all the in-kernel decoders, but has one minor shortcoming for ir-lirc-codec, in that /dev/lirc0 won't actually exist until the first incoming ir signal is seen. lircd can handle this case just fine, it'll wait for /dev/lirc0 to show up, but it doesn't come up fast enough to catch and decode the very first incoming ir signal. Subsequent ones work perfectly fine though. This need to initialize the link via incoming ir is a bit problematic if you're using a device for transmit-only (e.g., and mceusb device hooked to a mythtv backend in the closet or something), as there would be a strong possibility of /dev/lirc0 never getting hooked up. I can think of a few workarounds, but none are particularly clean and/or automagic.

Quoted 1 line

Not sure how palatable it is, but here it is:

I think it sounds pretty awful :)

I have another suggestion, let's keep the client register/unregister callbacks for decoders (but add a comment that they're only used for lirc). Then teach drivers/media/IR/ir-raw-event.c to keep track of the raw clients so that it can pass all pre-existing clients to newly added decoders.

I'll post two patches (compile tested only) in a few seconds to show what I mean.

Quoted 8 lines

I have another suggestion, let's keep the client register/unregister callbacks for decoders (but add a comment that they're only used for lirc). Then teach drivers/media/IR/ir-raw-event.c to keep track of the raw clients so that it can pass all pre-existing clients to newly added decoders.

I'll post two patches (compile tested only) in a few seconds to show what I mean.

Consider them now runtime tested as well. They appear to do the trick, the lirc bridge comes up just fine, even when ir-lirc-codec isn't loaded until after mceusb. *Much* better implementation than my ugly trick. I'll ack your patches and submit a series on top of them for lirc support, hopefully this evening (in addition to a few other fixes that aren't dependent on any of them).

On Wed, Jun 16, 2010 at 4:04 PM, Jarod Wilson <[email protected]> wrote:
...

Quoted 9 lines

Quoted 8 lines

I have another suggestion, let's keep the client register/unregister callbacks for decoders (but add a comment that they're only used for lirc). Then teach drivers/media/IR/ir-raw-event.c to keep track of the raw clients so that it can pass all pre-existing clients to newly added decoders.

I'll post two patches (compile tested only) in a few seconds to show what I mean.

Quoted 6 lines

Consider them now runtime tested as well. They appear to do the trick, the lirc bridge comes up just fine, even when ir-lirc-codec isn't loaded until after mceusb. *Much* better implementation than my ugly trick. I'll ack your patches and submit a series on top of them for lirc support, hopefully this evening (in addition to a few other fixes that aren't dependent on any of them).

A fully functional tree carrying both of David's patches and the entire stack of other patches I've submitted today, based on top of the linuxtv staging/rc branch, can be found here:

http://git.wilsonet.com/linux-2.6-ir-wip.git/?a=shortlog;h=refs/heads/patches

Also includes the lirc patches that I believe are ready to be submitted for actual consideration (note that they're dependent on David's two patches).

Quoted 24 lines · Jarod Wilson

On Wed, Jun 16, 2010 at 4:04 PM, Jarod Wilson <[email protected]> wrote:
...

Quoted 9 lines

Quoted 8 lines

I have another suggestion, let's keep the client register/unregister callbacks for decoders (but add a comment that they're only used for lirc). Then teach drivers/media/IR/ir-raw-event.c to keep track of the raw clients so that it can pass all pre-existing clients to newly added decoders.

I'll post two patches (compile tested only) in a few seconds to show what I mean.

Quoted 6 lines

Consider them now runtime tested as well. They appear to do the trick, the lirc bridge comes up just fine, even when ir-lirc-codec isn't loaded until after mceusb. *Much* better implementation than my ugly trick. I'll ack your patches and submit a series on top of them for lirc support, hopefully this evening (in addition to a few other fixes that aren't dependent on any of them).

A fully functional tree carrying both of David's patches and the entire stack of other patches I've submitted today, based on top of the linuxtv staging/rc branch, can be found here:

http://git.wilsonet.com/linux-2.6-ir-wip.git/?a=shortlog;h=refs/heads/patches

Quoted 3 lines

Also includes the lirc patches that I believe are ready to be submitted for actual consideration (note that they're dependent on David's two patches).

I'll try and play with this this weekend along with some cx23885 cleanup.

Quoted 11 lines · Andy Walls

Quoted 9 lines

A fully functional tree carrying both of David's patches and the entire stack of other patches I've submitted today, based on top of the linuxtv staging/rc branch, can be found here:

http://git.wilsonet.com/linux-2.6-ir-wip.git/?a=shortlog;h=refs/heads/patches

Also includes the lirc patches that I believe are ready to be submitted for actual consideration (note that they're dependent on David's two patches).

Quoted 2 lines

I'll try and play with this this weekend along with some cx23885 cleanup.

Excellent. A few things to note...

Many of the lirc_dev ioctls are
currently commented out, and haven't in any way been wired up to tx callbacks,

I've only enabled the minimum necessary for mceusb. The
ioctls are all using __u32 params, which, if you're on x86_64, will require a patched lirc userspace build to make the ioctl types match. I'm using this patch atm:

http://wilsonet.com/jarod/lirc_misc/lirc-0.8.6-make-ioctls-u32.patch

(In the future, lirc userspace should obviously just build against <media/lirc.h>).

Quoted 13 lines · Jarod Wilson

Quoted 11 lines · Andy Walls

Quoted 9 lines

A fully functional tree carrying both of David's patches and the entire stack of other patches I've submitted today, based on top of the linuxtv staging/rc branch, can be found here:

http://git.wilsonet.com/linux-2.6-ir-wip.git/?a=shortlog;h=refs/heads/patches

Also includes the lirc patches that I believe are ready to be submitted for actual consideration (note that they're dependent on David's two patches).

Quoted 4 lines

Quoted 2 lines

I'll try and play with this this weekend along with some cx23885 cleanup.

Excellent. A few things to note...

Jarrod,

I was unable to get this task completed in the time I had available this weekend.  A power supply failure, unexpected hard drive replacement, and my inability to build/install a kernel from a git tree that would actually boot my Fedora 12 installation didn't help.  (My productivity has tanked since v4l-dvb went to GIT for CM, and the last time I built a real kernel without rpmbuild was for RedHat 9. I'm still working out processes for doing basic things, sorry.)

I'll have time on Thursday night to try again.

On Sun, Jun 20, 2010 at 8:47 PM, Andy Walls <[email protected]> wrote:

As for the building your own kernel thing... I've been doing my work mainly on a pair of x86_64 systems, one a ThinkPad T61 running Fedora 13, and the other an HP xw4400 workstation running RHEL6. In both cases, I copied a distro kernel's, config file out of /boot/, and then ran make oldconfig over it and build straight from what's in my tree, which works well enough on both setups.

Quoted 1 line

I'll have time on Thursday night to try again.

No rush yet, we've got a while before the merge window still. Christoph (Bartelmus) helped me out with a bunch of ioctl documentation this weekend, so I've got that to add to the tree, then I think I'll be prepared to resubmit the lirc bits. I'll shoot for doing that next weekend, and hopefully, that'll give you a chance to try 'em out before then and provide any necessary feedback/fixes/etc. (Not that we can't also just fix things up as needed post-merge). I'm still up in the air as to what I should work on next... So many lirc drivers left to port still... Maybe zilog, maybe streamzap... Maybe the MCE IR keyboard...

Quoted 9 lines · Jarod Wilson

On Sun, Jun 20, 2010 at 8:47 PM, Andy Walls <[email protected]> wrote:

As for the building your own kernel thing... I've been doing my work mainly on a pair of x86_64 systems, one a ThinkPad T61 running Fedora 13, and the other an HP xw4400 workstation running RHEL6. In both cases, I copied a distro kernel's, config file out of /boot/, and then ran make oldconfig over it and build straight from what's in my tree, which works well enough on both setups.

I pulled the config out of the kernel*.src.rpm after 'rpmbuild -bp' (I only saw the configs in /boot after doing that :P ), and then ran 'make oldconfig'.

The first time around I forgot to do a modules_install and the ext[234] modules weren't in the initramfs.  That made it hard for the kernel to read the /boot and / filesystems. ;)

After fixing that idiocy, it now hangs in early boot - just a blinking cursor.  I'm speculating it is a problem with support for my old-ish ATI Radeon Xpress 200 video chipset with a vanilla kernel.  I'll work it out eventually.

Quoted 12 lines

Quoted 1 line

I'll have time on Thursday night to try again.

No rush yet, we've got a while before the merge window still. Christoph (Bartelmus) helped me out with a bunch of ioctl documentation this weekend, so I've got that to add to the tree, then I think I'll be prepared to resubmit the lirc bits. I'll shoot for doing that next weekend, and hopefully, that'll give you a chance to try 'em out before then and provide any necessary feedback/fixes/etc. (Not that we can't also just fix things up as needed post-merge). I'm still up in the air as to what I should work on next... So many lirc drivers left to port still... Maybe zilog, maybe streamzap... Maybe the MCE IR keyboard...

I've got a PVR-150 and HVR-1600 both with the Zilog Z8's on them.  If I ever get my act together, I'll at least be able to test that and integrate any changes into the ivtv & cx18 drivers.   I've recently seen users having trouble on IRC, BTW.