mceusb: Improve parser to get codes sent together with IR RX data

In file included from drivers/media/video/cx231xx/cx231xx-audio.c:40:
drivers/media/video/cx231xx/cx231xx.h:559: error: field ‘frontends’ has incomplete type make[4]: ** [drivers/media/video/cx231xx/cx231xx-audio.o] Erro 1
make[3]: ** [drivers/media/video/cx231xx] Erro 2
make[2]: ** [drivers/media/video] Erro 2
make[1]: ** [drivers/media] Erro 2
make: ** [drivers] Erro 2

Reported-by Marcio Araujo Alves <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

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

index 0d59ef7..a06a07e 100644

On cx231xx, sometimes, several control messages are sent together with data. Improves the parser to also handle those cases.

For example:

[38777.211690] mceusb 7-6:1.0: rx data: 9f 14 01 9f 15 00 00 80  (length=8)
[38777.211696] mceusb 7-6:1.0: Got long-range receive sensor in use
[38777.211700] mceusb 7-6:1.0: Received pulse count is 0
[38777.211703] mceusb 7-6:1.0: IR data len = 0
[38777.211707] mceusb 7-6:1.0: New data. rem: 0x1f, cmd: 0x80

Before this patch, only the first message would be displayed, as the parser would be stopping at "9f 14 01".

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/IR/mceusb.c +0 -0

index a726f63..609bf3d 100644

Add a parser for polaris mce. On this device, sometimes, a control data appears together with the IR data, causing problems at the parser. Also, it signalizes the end of a data with a 0x80 value. The normal parser would believe that this is a time with 0x1f size, but cx231xx provides just one byte for it.

I'm not sure if the new parser would work for other devices (probably, it will), but the better is to just write it as a new parser, to avoid breaking support for other supported IR devices.

drivers/media/IR/mceusb.c +11 -0

+			i++;
+			if (i >= buf_len)
+				return;
+
+			cmd = ir->buf_in[i];	/* sub cmd */
+			i++;
+			switch (cmd) {
+			case 0x08:
+			case 0x14:
+			case 0x17:
+				i += 1;

Quoted 10 lines

Add a parser for polaris mce. On this device, sometimes, a control data appears together with the IR data, causing problems at the parser. Also, it signalizes the end of a data with a 0x80 value. The normal parser would believe that this is a time with 0x1f size, but cx231xx provides just one byte for it.

I'm not sure if the new parser would work for other devices (probably, it will), but the better is to just write it as a new parser, to avoid breaking support for other supported IR devices.

After staring at it for a while, I think it would work okay for all 2nd and 3rd-gen mceusb devices, but it would almost certainly break 1st-gen, as it can have distinct IR data packets split across urb -- that's the whole reason for the if rem == 0 check in the existing routine.

Ultimately though, this routine isn't that much different, and I *think* I see a way to extend the existing routine with some of the code from this one to make it work better for the polaris device.

Will still go ahead with some review comments here though.

drivers/media/IR/mceusb.c struct mceusb_dev { · line 269

Quoted 10 lines

drivers/media/IR/mceusb.c +1 -0

index 609bf3d..7210760 100644
@@ -265,6 +265,7 @@ struct mceusb_dev {
 		u32 connected:1;
 		u32 tx_mask_inverted:1;
 		u32 microsoft_gen1:1;
+		u32 is_polaris:1;
 		u32 reserved:29;

reserved should be decremented by 1 here if adding another flag.

Quoted 6 lines

drivers/media/IR/mceusb.c +0 -0

 	} flags;
 

@@ -697,6 +698,90 @@ static int mceusb_set_tx_carrier(void *priv, u32 carrier)
 	return carrier;
 }
 

drivers/media/IR/mceusb.c static int mceusb_set_tx_carrier(void *priv, u32 carrier) · line 707

Quoted 7 lines

drivers/media/IR/mceusb.c +7 -0

+static void mceusb_parse_polaris(struct mceusb_dev *ir, int buf_len)
+{
+	struct ir_raw_event rawir;
+	int i;
+	u8 cmd;
+
+	while (i < buf_len) {

i is being used uninitialized here.

drivers/media/IR/mceusb.c static int mceusb_set_tx_carrier(void *priv, u32 carrier) · line 712

Quoted 5 lines

drivers/media/IR/mceusb.c +5 -0

+		cmd = ir->buf_in[i];
+
+		/* Discard any non-IR cmd */
+
+		if ((cmd & 0xe0) >> 5 != 4) {

I'd probably just stick with if ((cmd & 0xe0) != 0x80), or even != MCE_PULSE_BIT, since we have a #define for 0x80 already. (Though its not quite an accurate name in this case).

Quoted 15 lines

drivers/media/IR/mceusb.c +15 -0

+			i++;
+			if (i >= buf_len)
+				return;
+
+			cmd = ir->buf_in[i];	/* sub cmd */
+			i++;
+			switch (cmd) {
+			case 0x08:
+			case 0x14:
+			case 0x17:
+				i += 1;
+				break;
+			case 0x11:
+				i += 5;
+				break;

drivers/media/IR/mceusb.c static int mceusb_set_tx_carrier(void *priv, u32 carrier) · line 733

Quoted 6 lines

drivers/media/IR/mceusb.c +6 -0

+			case 0x06:
+			case 0x81:
+			case 0x15:
+			case 0x16:
+				i += 2;
+				break;

#define's for each of these hex values would be good, if we can determine what they actually are.

Quoted 6 lines

drivers/media/IR/mceusb.c +6 -0

+		} else if (cmd == 0x80) {
+			/*
+			 * Special case: timeout event on cx231xx
+			 * Is it needed to check if is_polaris?
+			 */
+			rawir.pulse = 0;

drivers/media/IR/mceusb.c static int mceusb_set_tx_carrier(void *priv, u32 carrier) · line 746

Quoted 6 lines

drivers/media/IR/mceusb.c +6 -0

+			rawir.duration = IR_MAX_DURATION;
+			dev_dbg(ir->dev, "Storing %s with duration %d\n",
+				rawir.pulse ? "pulse" : "space",
+				rawir.duration);
+
+			ir_raw_event_store(ir->idev, &rawir);

I think this and the prior hunk are really the only things that need to be grafted into the existing routine to make it behave with this device. Lemme see what I can come up with...

Em 21-10-2010 18:06, Jarod Wilson escreveu:

Quoted 18 lines · Mauro Carvalho Chehab

Quoted 10 lines

Add a parser for polaris mce. On this device, sometimes, a control data appears together with the IR data, causing problems at the parser. Also, it signalizes the end of a data with a 0x80 value. The normal parser would believe that this is a time with 0x1f size, but cx231xx provides just one byte for it.

I'm not sure if the new parser would work for other devices (probably, it will), but the better is to just write it as a new parser, to avoid breaking support for other supported IR devices.

After staring at it for a while, I think it would work okay for all 2nd and 3rd-gen mceusb devices, but it would almost certainly break 1st-gen, as it can have distinct IR data packets split across urb -- that's the whole reason for the if rem == 0 check in the existing routine.

Ultimately though, this routine isn't that much different, and I *think* I see a way to extend the existing routine with some of the code from this one to make it work better for the polaris device.

Will still go ahead with some review comments here though.

Quoted 12 lines

drivers/media/IR/mceusb.c struct mceusb_dev { · line 269

Quoted 10 lines

drivers/media/IR/mceusb.c +1 -0

index 609bf3d..7210760 100644
@@ -265,6 +265,7 @@ struct mceusb_dev {
 		u32 connected:1;
 		u32 tx_mask_inverted:1;
 		u32 microsoft_gen1:1;
+		u32 is_polaris:1;
 		u32 reserved:29;

reserved should be decremented by 1 here if adding another flag.

Ok. By curiosity, why are you reserving space on a bit array like that?

Quoted 15 lines

Quoted 15 lines

drivers/media/IR/mceusb.c +15 -0

+			i++;
+			if (i >= buf_len)
+				return;
+
+			cmd = ir->buf_in[i];	/* sub cmd */
+			i++;
+			switch (cmd) {
+			case 0x08:
+			case 0x14:
+			case 0x17:
+				i += 1;
+				break;
+			case 0x11:
+				i += 5;
+				break;

Quoted 8 lines

drivers/media/IR/mceusb.c static int mceusb_set_tx_carrier(void *priv, u32 carrier) · line 733

Quoted 6 lines

drivers/media/IR/mceusb.c +6 -0

+			case 0x06:
+			case 0x81:
+			case 0x15:
+			case 0x16:
+				i += 2;
+				break;

#define's for each of these hex values would be good, if we can determine what they actually are.

Maybe we can determine a few of them, as they also occur at the debug parsing loop.

Quoted 6 lines

Quoted 6 lines

drivers/media/IR/mceusb.c +6 -0

+		} else if (cmd == 0x80) {
+			/*
+			 * Special case: timeout event on cx231xx
+			 * Is it needed to check if is_polaris?
+			 */
+			rawir.pulse = 0;

Quoted 8 lines

drivers/media/IR/mceusb.c static int mceusb_set_tx_carrier(void *priv, u32 carrier) · line 746

Quoted 6 lines

drivers/media/IR/mceusb.c +6 -0

+			rawir.duration = IR_MAX_DURATION;
+			dev_dbg(ir->dev, "Storing %s with duration %d\n",
+				rawir.pulse ? "pulse" : "space",
+				rawir.duration);
+
+			ir_raw_event_store(ir->idev, &rawir);

I think this and the prior hunk are really the only things that need to be grafted into the existing routine to make it behave with this device. Lemme see what I can come up with...

True, although the double loop at the original logic is a bit confusing. I suspect you did it to handle the (cmd & 0xe0) != 0x80 condition, probably at the gen1 times, and then you modified it to handle other devices.

Quoted 12 lines

Em 21-10-2010 18:06, Jarod Wilson escreveu:

Quoted 8 lines

drivers/media/IR/mceusb.c struct mceusb_dev { · line 269

Quoted 6 lines

drivers/media/IR/mceusb.c +1 -0

@@ -265,6 +265,7 @@ struct mceusb_dev {
 		u32 connected:1;
 		u32 tx_mask_inverted:1;
 		u32 microsoft_gen1:1;
+		u32 is_polaris:1;
 		u32 reserved:29;

reserved should be decremented by 1 here if adding another flag.

Quoted 1 line

Ok. By curiosity, why are you reserving space on a bit array like that?

Legacy, mostly, I guess. Its been that way going back ages. I suppose we could just convert them all to bools and/or leave them as is and drop the reserved part entirely...

Yeah, this sorta grew from merging the old lirc_mceusb and lirc_mceusb2 drivers into one, and believe it or not, its actually cleaner than it used to be... :)

You've got a test patch in your inbox that attempts to merge the logic in this patch into the existing loop, and I'll see if I can beat on it with all three generations of stand-alone mceusb devices tonight...