[PATCH 1/4] [media] cx231xx: Fix compilation breakage if DVB is not selected

[PATCH 1/4] [media] cx231xx: Fix compilation breakage if DVB is not selected

[Mauro Carvalho Chehab]

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 <froooozen@gmail.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

diff --git a/drivers/media/video/cx231xx/cx231xx.h b/drivers/media/video/cx231xx/cx231xx.h
index 9c98886..d067df9 100644
--- a/drivers/media/video/cx231xx/cx231xx.h
+++ b/drivers/media/video/cx231xx/cx231xx.h
@@ -35,10 +35,7 @@
 #include <media/videobuf-vmalloc.h>
 #include <media/v4l2-device.h>
 #include <media/ir-core.h>
-#if defined(CONFIG_VIDEO_CX231XX_DVB) || \
-	defined(CONFIG_VIDEO_CX231XX_DVB_MODULE)
 #include <media/videobuf-dvb.h>
-#endif
 
 #include "cx231xx-reg.h"
 #include "cx231xx-pcb-cfg.h"
-- 
1.7.1

Re: [PATCH 1/4] [media] cx231xx: Fix compilation breakage if DVB is not selected

[Jarod Wilson]

On Thu, Oct 21, 2010 at 10:07 AM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> 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 <froooozen@gmail.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

Acked-by: Jarod Wilson <jarod@redhat.com>

-- 
Jarod Wilson
jarod@wilsonet.com

[PATCH 2/4] [media] ir-raw-event: Fix a stupid error at a printk

[Mauro Carvalho Chehab]

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

diff --git a/drivers/media/IR/ir-raw-event.c b/drivers/media/IR/ir-raw-event.c
index 0d59ef7..a06a07e 100644
--- a/drivers/media/IR/ir-raw-event.c
+++ b/drivers/media/IR/ir-raw-event.c
@@ -89,7 +89,7 @@ int ir_raw_event_store(struct input_dev *input_dev, struct ir_raw_event *ev)
 	if (!ir->raw)
 		return -EINVAL;
 
-	IR_dprintk(2, "sample: (05%dus %s)\n",
+	IR_dprintk(2, "sample: (%05dus %s)\n",
 		TO_US(ev->duration), TO_STR(ev->pulse));
 
 	if (kfifo_in(&ir->raw->kfifo, ev, sizeof(*ev)) != sizeof(*ev))
-- 
1.7.1

Re: [PATCH 2/4] [media] ir-raw-event: Fix a stupid error at a printk

[Jarod Wilson]

On Thu, Oct 21, 2010 at 10:07 AM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>
> diff --git a/drivers/media/IR/ir-raw-event.c b/drivers/media/IR/ir-raw-event.c
> index 0d59ef7..a06a07e 100644
> --- a/drivers/media/IR/ir-raw-event.c
> +++ b/drivers/media/IR/ir-raw-event.c
> @@ -89,7 +89,7 @@ int ir_raw_event_store(struct input_dev *input_dev, struct ir_raw_event *ev)
>        if (!ir->raw)
>                return -EINVAL;
>
> -       IR_dprintk(2, "sample: (05%dus %s)\n",
> +       IR_dprintk(2, "sample: (%05dus %s)\n",
>                TO_US(ev->duration), TO_STR(ev->pulse));
>
>        if (kfifo_in(&ir->raw->kfifo, ev, sizeof(*ev)) != sizeof(*ev))


Acked-by: Jarod Wilson <jarod@redhat.com>

-- 
Jarod Wilson
jarod@wilsonet.com

[PATCH 3/4] [media] mceusb: Improve parser to get codes sent together with IR RX data

[Mauro Carvalho Chehab]

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 <mchehab@redhat.com>

diff --git a/drivers/media/IR/mceusb.c b/drivers/media/IR/mceusb.c
index a726f63..609bf3d 100644
--- a/drivers/media/IR/mceusb.c
+++ b/drivers/media/IR/mceusb.c
@@ -343,99 +343,130 @@ static void mceusb_dev_printdata(struct mceusb_dev *ir, char *buf,
 	else
 		strcpy(inout, "Got\0");
 
-	cmd    = buf[idx] & 0xff;
-	subcmd = buf[idx + 1] & 0xff;
-	data1  = buf[idx + 2] & 0xff;
-	data2  = buf[idx + 3] & 0xff;
+	while (idx < len) {
+		cmd    = buf[idx] & 0xff;
+		subcmd = buf[idx + 1] & 0xff;
+		data1  = buf[idx + 2] & 0xff;
+		data2  = buf[idx + 3] & 0xff;
 
-	switch (cmd) {
-	case 0x00:
-		if (subcmd == 0xff && data1 == 0xaa)
-			dev_info(dev, "Device reset requested\n");
-		else
-			dev_info(dev, "Unknown command 0x%02x 0x%02x\n",
-				 cmd, subcmd);
-		break;
-	case 0xff:
-		switch (subcmd) {
-		case 0x0b:
-			if (len == 2)
-				dev_info(dev, "Get hw/sw rev?\n");
-			else
-				dev_info(dev, "hw/sw rev 0x%02x 0x%02x "
-					 "0x%02x 0x%02x\n", data1, data2,
-					 buf[idx + 4], buf[idx + 5]);
-			break;
-		case 0xaa:
-			dev_info(dev, "Device reset requested\n");
-			break;
-		case 0xfe:
-			dev_info(dev, "Previous command not supported\n");
-			break;
-		case 0x18:
-		case 0x1b:
-		default:
-			dev_info(dev, "Unknown command 0x%02x 0x%02x\n",
-				 cmd, subcmd);
-			break;
-		}
-		break;
-	case 0x9f:
-		switch (subcmd) {
-		case 0x03:
-			dev_info(dev, "Ping\n");
-			break;
-		case 0x04:
-			dev_info(dev, "Resp to 9f 05 of 0x%02x 0x%02x\n",
-				 data1, data2);
-			break;
-		case 0x06:
-			dev_info(dev, "%s carrier mode and freq of "
-				 "0x%02x 0x%02x\n", inout, data1, data2);
-			break;
-		case 0x07:
-			dev_info(dev, "Get carrier mode and freq\n");
-			break;
-		case 0x08:
-			dev_info(dev, "%s transmit blaster mask of 0x%02x\n",
-				 inout, data1);
-			break;
-		case 0x0c:
-			/* value is in units of 50us, so x*50/100 or x/2 ms */
-			dev_info(dev, "%s receive timeout of %d ms\n",
-				 inout, ((data1 << 8) | data2) / 2);
-			break;
-		case 0x0d:
-			dev_info(dev, "Get receive timeout\n");
-			break;
-		case 0x13:
-			dev_info(dev, "Get transmit blaster mask\n");
-			break;
-		case 0x14:
-			dev_info(dev, "%s %s-range receive sensor in use\n",
-				 inout, data1 == 0x02 ? "short" : "long");
-			break;
-		case 0x15:
-			if (len == 2)
-				dev_info(dev, "Get receive sensor\n");
+		/*
+		 * skip command/subcommand
+		 * The size of each package at the protocol depends
+		 * on the given command/subcommand
+		 */
+		idx += 2;
+
+		switch (cmd) {
+		case 0x00:
+			if (subcmd == 0xff && data1 == 0xaa)
+				dev_info(dev, "Device reset requested\n");
 			else
-				dev_info(dev, "Received pulse count is %d\n",
-					 ((data1 << 8) | data2));
+				dev_info(dev, "Unknown command 0x%02x 0x%02x\n",
+					 cmd, subcmd);
+			idx = len;
+			break;
+		case 0xff:
+			switch (subcmd) {
+			case 0x0b:
+				if (len == 2)
+					dev_info(dev, "Get hw/sw rev?\n");
+				else
+					dev_info(dev, "hw/sw rev 0x%02x 0x%02x "
+						 "0x%02x 0x%02x\n", data1, data2,
+						 buf[idx + 4], buf[idx + 5]);
+				idx = len;
+				break;
+			case 0xaa:
+				dev_info(dev, "Device reset requested\n");
+				break;
+			case 0xfe:
+				dev_info(dev, "Previous command not supported\n");
+				break;
+			case 0x18:
+			case 0x1b:
+			default:
+				dev_info(dev, "Unknown command 0x%02x 0x%02x\n",
+					 cmd, subcmd);
+				break;
+			}
+			idx = len;
+			break;
+		case 0x9f:
+			switch (subcmd) {
+			case 0x03:
+				dev_info(dev, "Ping\n");
+				break;
+			case 0x04:
+				dev_info(dev, "Resp to 9f 05 of 0x%02x 0x%02x\n",
+					 data1, data2);
+				idx += 2;
+				break;
+			case 0x06:
+				dev_info(dev, "%s carrier mode and freq of "
+					 "0x%02x 0x%02x\n", inout, data1, data2);
+				idx += 2;
+				break;
+			case 0x07:
+				dev_info(dev, "Get carrier mode and freq\n");
+				break;
+			case 0x08:
+				dev_info(dev, "%s transmit blaster mask of 0x%02x\n",
+					 inout, data1);
+				idx += 1;
+				break;
+			case 0x0c:
+				/* value is in units of 50us, so x*50/100 or x/2 ms */
+				dev_info(dev, "%s receive timeout of %d ms\n",
+					 inout, ((data1 << 8) | data2) / 2);
+				idx += 2;
+				break;
+			case 0x0d:
+				dev_info(dev, "Get receive timeout\n");
+				break;
+			case 0x13:
+				dev_info(dev, "Get transmit blaster mask\n");
+				break;
+			case 0x14:
+				dev_info(dev, "%s %s-range receive sensor in use\n",
+					 inout, data1 == 0x02 ? "short" : "long");
+				idx += 1;
+				break;
+			case 0x15:
+				if (!(len - idx))
+					dev_info(dev, "Get receive sensor\n");
+				else {
+					dev_info(dev, "Received pulse count is %d\n",
+						 ((data1 << 8) | data2));
+					idx += 2;
+				}
+				break;
+			case 0xfe:
+				dev_info(dev, "Error! Hardware is likely wedged...\n");
+				break;
+			case 0x05:
+			case 0x09:
+			case 0x0f:
+			default:
+				dev_info(dev, "Unknown command 0x%02x 0x%02x\n",
+					 cmd, subcmd);
+				break;
+			}
 			break;
-		case 0xfe:
-			dev_info(dev, "Error! Hardware is likely wedged...\n");
-			break;
-		case 0x05:
-		case 0x09:
-		case 0x0f:
 		default:
-			dev_info(dev, "Unknown command 0x%02x 0x%02x\n",
-				 cmd, subcmd);
+			if (((cmd & 0xe0) >> 5) == 4) {
+				/* IR data */
+				int datalen = cmd & 0x1f;
+
+				if (len == 0x1f) {
+					dev_info(dev, "IR cmd = %d\n", subcmd);
+					idx = len;	/* FIXME */
+				} else {
+					dev_info(dev, "IR data len = %d", datalen);
+					idx += datalen - 1; /* No subcmd */
+				}
+			}
 			break;
 		}
-		break;
-	default:
-		break;
 	}
 }
 
@@ -724,9 +755,9 @@ static void mceusb_process_ir_data(struct mceusb_dev *ir, int buf_len)
 		if (ir->buf_in[i] == 0x80 || ir->buf_in[i] == 0x9f)
 			ir->rem = 0;
 
-		dev_dbg(ir->dev, "calling ir_raw_event_handle\n");
-		ir_raw_event_handle(ir->idev);
 	}
+	dev_dbg(ir->dev, "calling ir_raw_event_handle\n");
+	ir_raw_event_handle(ir->idev);
 }
 
 static void mceusb_dev_recv(struct urb *urb, struct pt_regs *regs)
-- 
1.7.1

Re: [PATCH 3/4] [media] mceusb: Improve parser to get codes sent together with IR RX data

[Jarod Wilson]

On Oct 21, 2010, at 10:07 AM, Mauro Carvalho Chehab wrote:

> 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 <mchehab@redhat.com>

So two minor issues with this patch...

First, the debug spew looks slightly confusing, though through no fault of yours, really. Now that you're parsing all three chunks of that final sequence, you see a "New data." message that actually comes from the first chunk of the three, but after you've already seen the other three. We should probably just drop that dev_dbg and rely on the 'IR data len' one.

The second issue is the very last hunk of the patch that moves the call to ir_raw_event_handle, which should be unrelated to the rest of this patch (and may, iirc, cause issues for the first-gen transceiver -- I'd have to re-test to be sure).

But otherwise, looks good to me.


> diff --git a/drivers/media/IR/mceusb.c b/drivers/media/IR/mceusb.c
> index a726f63..609bf3d 100644
> --- a/drivers/media/IR/mceusb.c
> +++ b/drivers/media/IR/mceusb.c
...
> @@ -724,9 +755,9 @@ static void mceusb_process_ir_data(struct mceusb_dev *ir, int buf_len)
> 		if (ir->buf_in[i] == 0x80 || ir->buf_in[i] == 0x9f)
> 			ir->rem = 0;
> 
> -		dev_dbg(ir->dev, "calling ir_raw_event_handle\n");
> -		ir_raw_event_handle(ir->idev);
> 	}
> +	dev_dbg(ir->dev, "calling ir_raw_event_handle\n");
> +	ir_raw_event_handle(ir->idev);
> }
> 
> static void mceusb_dev_recv(struct urb *urb, struct pt_regs *regs)

^^^ the unrelated hunk.

-- 
Jarod Wilson
jarod@wilsonet.com

[PATCH 4/4] [media] mceusb: Fix parser for Polaris

[Mauro Carvalho Chehab]

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.

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

diff --git a/drivers/media/IR/mceusb.c b/drivers/media/IR/mceusb.c
index 609bf3d..7210760 100644
--- a/drivers/media/IR/mceusb.c
+++ b/drivers/media/IR/mceusb.c
@@ -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;
 	} flags;
 
@@ -697,6 +698,90 @@ static int mceusb_set_tx_carrier(void *priv, u32 carrier)
 	return carrier;
 }
 
+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) {
+		cmd = ir->buf_in[i];
+
+		/* Discard any non-IR cmd */
+
+		if ((cmd & 0xe0) >> 5 != 4) {
+			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;
+			case 0x06:
+			case 0x81:
+			case 0x15:
+			case 0x16:
+				i += 2;
+				break;
+			}
+		} else if (cmd == 0x80) {
+			/*
+			 * Special case: timeout event on cx231xx
+			 * Is it needed to check if is_polaris?
+			 */
+			rawir.pulse = 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);
+		} else {
+			ir->rem = (cmd & MCE_PACKET_LENGTH_MASK);
+			ir->cmd = (cmd & ~MCE_PACKET_LENGTH_MASK);
+			dev_dbg(ir->dev, "New data. rem: 0x%02x, cmd: 0x%02x\n",
+				ir->rem, ir->cmd);
+			i++;
+			for (; (ir->rem > 0) && (i < buf_len); i++) {
+				ir->rem--;
+
+				rawir.pulse = ((ir->buf_in[i] & MCE_PULSE_BIT) != 0);
+				rawir.duration = (ir->buf_in[i] & MCE_PULSE_MASK)
+						 * MCE_TIME_UNIT * 1000;
+
+				if ((ir->buf_in[i] & MCE_PULSE_MASK) == 0x7f) {
+					if (ir->rawir.pulse == rawir.pulse)
+						ir->rawir.duration += rawir.duration;
+					else {
+						ir->rawir.duration = rawir.duration;
+						ir->rawir.pulse = rawir.pulse;
+					}
+					continue;
+				}
+				rawir.duration += ir->rawir.duration;
+				ir->rawir.duration = 0;
+				ir->rawir.pulse = rawir.pulse;
+
+				dev_dbg(ir->dev, "Storing %s with duration %d\n",
+					rawir.pulse ? "pulse" : "space",
+					rawir.duration);
+
+				ir_raw_event_store(ir->idev, &rawir);
+			}
+		}
+	}
+	dev_dbg(ir->dev, "calling ir_raw_event_handle\n");
+	ir_raw_event_handle(ir->idev);
+}
+
 static void mceusb_process_ir_data(struct mceusb_dev *ir, int buf_len)
 {
 	DEFINE_IR_RAW_EVENT(rawir);
@@ -707,6 +792,11 @@ static void mceusb_process_ir_data(struct mceusb_dev *ir, int buf_len)
 	if (ir->flags.microsoft_gen1)
 		start_index = 2;
 
+	if (ir->flags.is_polaris) {
+		mceusb_parse_polaris(ir, buf_len);
+		return;
+	}
+
 	for (i = start_index; i < buf_len;) {
 		if (ir->rem == 0) {
 			/* decode mce packets of the form (84),AA,BB,CC,DD */
@@ -1044,6 +1134,7 @@ static int __devinit mceusb_dev_probe(struct usb_interface *intf,
 	ir->len_in = maxp;
 	ir->flags.microsoft_gen1 = is_microsoft_gen1;
 	ir->flags.tx_mask_inverted = tx_mask_inverted;
+	ir->flags.is_polaris = is_polaris;
 	init_ir_raw_event(&ir->rawir);
 
 	/* Saving usb interface data for use by the transmitter routine */
-- 
1.7.1

Re: [PATCH 4/4] [media] mceusb: Fix parser for Polaris

[Jarod Wilson]

On Oct 21, 2010, at 10:07 AM, Mauro Carvalho Chehab wrote:

> 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.

> diff --git a/drivers/media/IR/mceusb.c b/drivers/media/IR/mceusb.c
> index 609bf3d..7210760 100644
> --- a/drivers/media/IR/mceusb.c
> +++ b/drivers/media/IR/mceusb.c
> @@ -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.


> 	} flags;
> 
> @@ -697,6 +698,90 @@ static int mceusb_set_tx_carrier(void *priv, u32 carrier)
> 	return carrier;
> }
> 
> +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.


> +		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).


> +			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;
> +			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.


> +		} else if (cmd == 0x80) {
> +			/*
> +			 * Special case: timeout event on cx231xx
> +			 * Is it needed to check if is_polaris?
> +			 */
> +			rawir.pulse = 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...


-- 
Jarod Wilson
jarod@wilsonet.com

Re: [PATCH 4/4] [media] mceusb: Fix parser for Polaris

[Mauro Carvalho Chehab]

Em 21-10-2010 18:06, Jarod Wilson escreveu:
> On Oct 21, 2010, at 10:07 AM, Mauro Carvalho Chehab wrote:
> 
>> 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.
> 
>> diff --git a/drivers/media/IR/mceusb.c b/drivers/media/IR/mceusb.c
>> index 609bf3d..7210760 100644
>> --- a/drivers/media/IR/mceusb.c
>> +++ b/drivers/media/IR/mceusb.c
>> @@ -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?


>> 	} flags;
>>
>> @@ -697,6 +698,90 @@ static int mceusb_set_tx_carrier(void *priv, u32 carrier)
>> 	return carrier;
>> }
>>
>> +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.

Ops!

>> +		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).

Ok.
 
>> +			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;
>> +			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.

>> +		} else if (cmd == 0x80) {
>> +			/*
>> +			 * Special case: timeout event on cx231xx
>> +			 * Is it needed to check if is_polaris?
>> +			 */
>> +			rawir.pulse = 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. 

Cheers,
Mauro

Re: [PATCH 4/4] [media] mceusb: Fix parser for Polaris

[Jarod Wilson]

On Oct 21, 2010, at 4:38 PM, Mauro Carvalho Chehab wrote:

> Em 21-10-2010 18:06, Jarod Wilson escreveu:
>> On Oct 21, 2010, at 10:07 AM, Mauro Carvalho Chehab wrote:
>> 
>>> 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.
>> 
>>> diff --git a/drivers/media/IR/mceusb.c b/drivers/media/IR/mceusb.c
>>> index 609bf3d..7210760 100644
>>> --- a/drivers/media/IR/mceusb.c
>>> +++ b/drivers/media/IR/mceusb.c
>>> @@ -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?

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...

>>> +			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;
>>> +			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.

Yeah, a few of them are at least reasonably well understood.



>>> +		} else if (cmd == 0x80) {
>>> +			/*
>>> +			 * Special case: timeout event on cx231xx
>>> +			 * Is it needed to check if is_polaris?
>>> +			 */
>>> +			rawir.pulse = 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.

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...

-- 
Jarod Wilson
jarod@wilsonet.com

Re: [PATCH 4/4] [media] mceusb: Fix parser for Polaris

[Mauro Carvalho Chehab]

Em 21-10-2010 19:06, Jarod Wilson escreveu:
> On Oct 21, 2010, at 4:38 PM, Mauro Carvalho Chehab wrote:
> 
>> Em 21-10-2010 18:06, Jarod Wilson escreveu:

>>>> @@ -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?
> 
> 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...

Agreed. If this is not part of any kabi (and it isn't, as far as I see), we can just
drop the reserved, as gcc will pad it accordingly.

> 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...
> 

I'll do some tests and send you a feedback.

Cheers,
Mauro