[PATCH 0/3] mceusb cleanups and new device support

[PATCH 0/3] mceusb cleanups and new device support

[Jarod Wilson]

Another round of mceusb patches...

Jarod Wilson (3):
      mceusb: add support for Conexant Hybrid TV RDU253S
      mceusb: fix up reporting of trailing space
      mceusb: buffer parsing fixups for 1st-gen device

 drivers/media/IR/mceusb.c               |   82  +++++++++++++++++++++----------
 1 files changed, 55 insertions(+), 27 deletions(-)

-- 
Jarod Wilson
jarod@redhat.com

[PATCH 1/3] mceusb: add support for Conexant Hybrid TV RDU253S

[Jarod Wilson]

Another multi-function Conexant device. Interface 0 is IR, though on
this model, TX isn't wired up at all, so I've mixed in support for
models without TX (and verified that lircd says TX isn't supported when
trying to send w/this device).

Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/media/IR/mceusb.c |   37 +++++++++++++++++++++++++++----------
 1 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/media/IR/mceusb.c b/drivers/media/IR/mceusb.c
index 9dce684..e453c6b 100644
--- a/drivers/media/IR/mceusb.c
+++ b/drivers/media/IR/mceusb.c
@@ -146,6 +146,7 @@ enum mceusb_model_type {
 	MCE_GEN3,
 	MCE_GEN2_TX_INV,
 	POLARIS_EVK,
+	CX_HYBRID_TV,
 };
 
 struct mceusb_model {
@@ -154,6 +155,7 @@ struct mceusb_model {
 	u32 mce_gen3:1;
 	u32 tx_mask_inverted:1;
 	u32 is_polaris:1;
+	u32 no_tx:1;
 
 	const char *rc_map;	/* Allow specify a per-board map */
 	const char *name;	/* per-board name */
@@ -183,7 +185,12 @@ static const struct mceusb_model mceusb_model[] = {
 		 * to allow testing it
 		 */
 		.rc_map = RC_MAP_RC5_HAUPPAUGE_NEW,
-		.name = "cx231xx MCE IR",
+		.name = "Conexant Hybrid TV (cx231xx) MCE IR",
+	},
+	[CX_HYBRID_TV] = {
+		.is_polaris = 1,
+		.no_tx = 1, /* tx isn't wired up at all */
+		.name = "Conexant Hybrid TV (cx231xx) MCE IR",
 	},
 };
 
@@ -292,9 +299,12 @@ static struct usb_device_id mceusb_dev_table[] = {
 	{ USB_DEVICE(VENDOR_NORTHSTAR, 0xe004) },
 	/* TiVo PC IR Receiver */
 	{ USB_DEVICE(VENDOR_TIVO, 0x2000) },
-	/* Conexant SDK */
+	/* Conexant Hybrid TV "Shelby" Polaris SDK */
 	{ USB_DEVICE(VENDOR_CONEXANT, 0x58a1),
 	  .driver_info = POLARIS_EVK },
+	/* Conexant Hybrid TV RDU253S Polaris */
+	{ USB_DEVICE(VENDOR_CONEXANT, 0x58a5),
+	  .driver_info = CX_HYBRID_TV },
 	/* Terminating entry */
 	{ }
 };
@@ -334,6 +344,7 @@ struct mceusb_dev {
 		u32 connected:1;
 		u32 tx_mask_inverted:1;
 		u32 microsoft_gen1:1;
+		u32 no_tx:1;
 	} flags;
 
 	/* transmit support */
@@ -724,7 +735,7 @@ out:
 	return ret ? ret : n;
 }
 
-/* Sets active IR outputs -- mce devices typically (all?) have two */
+/* Sets active IR outputs -- mce devices typically have two */
 static int mceusb_set_tx_mask(void *priv, u32 mask)
 {
 	struct mceusb_dev *ir = priv;
@@ -984,9 +995,11 @@ static void mceusb_get_parameters(struct mceusb_dev *ir)
 	mce_async_out(ir, GET_CARRIER_FREQ, sizeof(GET_CARRIER_FREQ));
 	mce_sync_in(ir, NULL, maxp);
 
-	/* get the transmitter bitmask */
-	mce_async_out(ir, GET_TX_BITMASK, sizeof(GET_TX_BITMASK));
-	mce_sync_in(ir, NULL, maxp);
+	if (!ir->flags.no_tx) {
+		/* get the transmitter bitmask */
+		mce_async_out(ir, GET_TX_BITMASK, sizeof(GET_TX_BITMASK));
+		mce_sync_in(ir, NULL, maxp);
+	}
 
 	/* get receiver timeout value */
 	mce_async_out(ir, GET_RX_TIMEOUT, sizeof(GET_RX_TIMEOUT));
@@ -1035,9 +1048,11 @@ static struct input_dev *mceusb_init_input_dev(struct mceusb_dev *ir)
 	props->priv = ir;
 	props->driver_type = RC_DRIVER_IR_RAW;
 	props->allowed_protos = IR_TYPE_ALL;
-	props->s_tx_mask = mceusb_set_tx_mask;
-	props->s_tx_carrier = mceusb_set_tx_carrier;
-	props->tx_ir = mceusb_tx_ir;
+	if (!ir->flags.no_tx) {
+		props->s_tx_mask = mceusb_set_tx_mask;
+		props->s_tx_carrier = mceusb_set_tx_carrier;
+		props->tx_ir = mceusb_tx_ir;
+	}
 
 	ir->props = props;
 
@@ -1151,6 +1166,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.no_tx = mceusb_model[model].no_tx;
 	ir->model = model;
 
 	init_ir_raw_event(&ir->rawir);
@@ -1191,7 +1207,8 @@ static int __devinit mceusb_dev_probe(struct usb_interface *intf,
 
 	mceusb_get_parameters(ir);
 
-	mceusb_set_tx_mask(ir, MCE_DEFAULT_TX_MASK);
+	if (!ir->flags.no_tx)
+		mceusb_set_tx_mask(ir, MCE_DEFAULT_TX_MASK);
 
 	usb_set_intfdata(intf, ir);
 
-- 
1.7.1


-- 
Jarod Wilson
jarod@redhat.com

[PATCH 2/3] mceusb: fix up reporting of trailing space

[Jarod Wilson]

We were storing a bunch of spaces at the end of each signal, rather than
a single long space. The in-kernel decoders were actually okay with
this, but lirc isn't. Both are happy again with this change, which
starts accumulating data upon seeing an 0x7f space, and then stores it
when we see the next non-space, non-0x7f space, or an 0x80 end of signal
command. To get to that final 0x80 properly, we also need to support
proper parsing of 0x9f 0x01 commands, support for which is also added.

Also remove an obsolete include, some stray colon-space pairs from
prior to conversion to dev_foo printk macros and a magic number.

Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/media/IR/mceusb.c |   39 +++++++++++++++++++++++----------------
 1 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/drivers/media/IR/mceusb.c b/drivers/media/IR/mceusb.c
index e453c6b..a05dec7 100644
--- a/drivers/media/IR/mceusb.c
+++ b/drivers/media/IR/mceusb.c
@@ -38,7 +38,6 @@
 #include <linux/usb.h>
 #include <linux/input.h>
 #include <media/ir-core.h>
-#include <media/ir-common.h>
 
 #define DRIVER_VERSION	"1.91"
 #define DRIVER_AUTHOR	"Jarod Wilson <jarod@wilsonet.com>"
@@ -74,6 +73,7 @@
 #define MCE_PACKET_LENGTH_MASK	0x1f /* Packet length mask */
 
 /* Sub-commands, which follow MCE_COMMAND_HEADER or MCE_HW_CMD_HEADER */
+#define MCE_CMD_SIG_END		0x01	/* End of signal */
 #define MCE_CMD_PING		0x03	/* Ping device */
 #define MCE_CMD_UNKNOWN		0x04	/* Unknown */
 #define MCE_CMD_UNKNOWN2	0x05	/* Unknown */
@@ -422,6 +422,7 @@ static int mceusb_cmdsize(u8 cmd, u8 subcmd)
 		case MCE_CMD_G_RXSENSOR:
 			datasize = 2;
 			break;
+		case MCE_CMD_SIG_END:
 		case MCE_CMD_S_TXMASK:
 		case MCE_CMD_S_RXSENSOR:
 			datasize = 1;
@@ -502,6 +503,9 @@ static void mceusb_dev_printdata(struct mceusb_dev *ir, char *buf,
 		break;
 	case MCE_COMMAND_HEADER:
 		switch (subcmd) {
+		case MCE_CMD_SIG_END:
+			dev_info(dev, "End of signal\n");
+			break;
 		case MCE_CMD_PING:
 			dev_info(dev, "Ping\n");
 			break;
@@ -539,7 +543,7 @@ static void mceusb_dev_printdata(struct mceusb_dev *ir, char *buf,
 			if (len == 2)
 				dev_info(dev, "Get receive sensor\n");
 			else
-				dev_info(dev, "Received pulse count is %d\n",
+				dev_info(dev, "Remaining pulse count is %d\n",
 					 ((data1 << 8) | data2));
 			break;
 		case MCE_RSP_CMD_INVALID:
@@ -763,7 +767,7 @@ static int mceusb_set_tx_carrier(void *priv, u32 carrier)
 
 		if (carrier == 0) {
 			ir->carrier = carrier;
-			cmdbuf[2] = 0x01;
+			cmdbuf[2] = MCE_CMD_SIG_END;
 			cmdbuf[3] = MCE_IRDATA_TRAILER;
 			dev_dbg(ir->dev, "%s: disabling carrier "
 				"modulation\n", __func__);
@@ -823,8 +827,11 @@ static void mceusb_process_ir_data(struct mceusb_dev *ir, int buf_len)
 					ir->rawir.duration = rawir.duration;
 					ir->rawir.pulse = rawir.pulse;
 				}
-				if (ir->rem)
-					break;
+				if (!ir->rem)
+					ir->parser_state = CMD_HEADER;
+				dev_dbg(ir->dev, "Accumulating %d worth of "
+					"space\n", rawir.duration);
+				break;
 			}
 			rawir.duration += ir->rawir.duration;
 			ir->rawir.duration = 0;
@@ -853,14 +860,14 @@ static void mceusb_process_ir_data(struct mceusb_dev *ir, int buf_len)
 			mceusb_dev_printdata(ir, ir->buf_in, i, ir->rem + 1, false);
 			if (ir->rem) {
 				ir->parser_state = PARSE_IRDATA;
-				break;
+			} else if (ir->rawir.duration) {
+				/* this means we've encounter an 0x80 pkt,
+				 * which means "end of signal" */
+				dev_dbg(ir->dev, "Storing final space with "
+					"duration %d\n", ir->rawir.duration);
+				ir_raw_event_store(ir->idev, &ir->rawir);
+				ir->rawir.duration = 0;
 			}
-			/*
-			 * a package with len=0 (e. g. 0x80) means end of
-			 * data. We could use it to do the call to
-			 * ir_raw_event_handle(). For now, we don't need to
-			 * use it.
-			 */
 			break;
 		}
 
@@ -1092,7 +1099,7 @@ static int __devinit mceusb_dev_probe(struct usb_interface *intf,
 	bool tx_mask_inverted;
 	bool is_polaris;
 
-	dev_dbg(&intf->dev, ": %s called\n", __func__);
+	dev_dbg(&intf->dev, "%s called\n", __func__);
 
 	idesc  = intf->cur_altsetting;
 
@@ -1122,7 +1129,7 @@ static int __devinit mceusb_dev_probe(struct usb_interface *intf,
 			ep_in = ep;
 			ep_in->bmAttributes = USB_ENDPOINT_XFER_INT;
 			ep_in->bInterval = 1;
-			dev_dbg(&intf->dev, ": acceptable inbound endpoint "
+			dev_dbg(&intf->dev, "acceptable inbound endpoint "
 				"found\n");
 		}
 
@@ -1137,12 +1144,12 @@ static int __devinit mceusb_dev_probe(struct usb_interface *intf,
 			ep_out = ep;
 			ep_out->bmAttributes = USB_ENDPOINT_XFER_INT;
 			ep_out->bInterval = 1;
-			dev_dbg(&intf->dev, ": acceptable outbound endpoint "
+			dev_dbg(&intf->dev, "acceptable outbound endpoint "
 				"found\n");
 		}
 	}
 	if (ep_in == NULL) {
-		dev_dbg(&intf->dev, ": inbound and/or endpoint not found\n");
+		dev_dbg(&intf->dev, "inbound and/or endpoint not found\n");
 		return -ENODEV;
 	}
 
-- 
1.7.1


-- 
Jarod Wilson
jarod@redhat.com

[PATCH 3/3] mceusb: buffer parsing fixups for 1st-gen device

[Jarod Wilson]

If we pass in an offset, we shouldn't skip 2 bytes. And the first-gen
hardware generates a constant stream of interrupts, always with two
header bytes, and if there's been no IR, with nothing else. Bail from
ir processing without calling ir_handle_raw_event when we get such a
buffer delivered to us.

Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/media/IR/mceusb.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/media/IR/mceusb.c b/drivers/media/IR/mceusb.c
index a05dec7..09a62f1 100644
--- a/drivers/media/IR/mceusb.c
+++ b/drivers/media/IR/mceusb.c
@@ -445,7 +445,7 @@ static void mceusb_dev_printdata(struct mceusb_dev *ir, char *buf,
 		return;
 
 	/* skip meaningless 0xb1 0x60 header bytes on orig receiver */
-	if (ir->flags.microsoft_gen1 && !out)
+	if (ir->flags.microsoft_gen1 && !out && !offset)
 		skip = 2;
 
 	if (len <= skip)
@@ -806,6 +806,10 @@ static void mceusb_process_ir_data(struct mceusb_dev *ir, int buf_len)
 	if (ir->flags.microsoft_gen1)
 		i = 2;
 
+	/* if there's no data, just return now */
+	if (buf_len <= i)
+		return;
+
 	for (; i < buf_len; i++) {
 		switch (ir->parser_state) {
 		case SUBCMD:
-- 
1.7.1


-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH 2/3] mceusb: fix up reporting of trailing space

[David Härdeman]

On Thu, Oct 28, 2010 at 11:08:10PM -0400, Jarod Wilson wrote:
> We were storing a bunch of spaces at the end of each signal, rather than
> a single long space. The in-kernel decoders were actually okay with
> this, but lirc isn't. Both are happy again with this change, which
> starts accumulating data upon seeing an 0x7f space, and then stores it
> when we see the next non-space, non-0x7f space, or an 0x80 end of signal
> command. To get to that final 0x80 properly, we also need to support
> proper parsing of 0x9f 0x01 commands, support for which is also added.

I think the driver could be further simplified by using 
ir_raw_event_store_with_filter(), right?

-- 
David Härdeman

Re: [PATCH 2/3] mceusb: fix up reporting of trailing space

[Jarod Wilson]

On Fri, Oct 29, 2010 at 09:21:21PM +0200, David Härdeman wrote:
> On Thu, Oct 28, 2010 at 11:08:10PM -0400, Jarod Wilson wrote:
> > We were storing a bunch of spaces at the end of each signal, rather than
> > a single long space. The in-kernel decoders were actually okay with
> > this, but lirc isn't. Both are happy again with this change, which
> > starts accumulating data upon seeing an 0x7f space, and then stores it
> > when we see the next non-space, non-0x7f space, or an 0x80 end of signal
> > command. To get to that final 0x80 properly, we also need to support
> > proper parsing of 0x9f 0x01 commands, support for which is also added.
> 
> I think the driver could be further simplified by using 
> ir_raw_event_store_with_filter(), right?

Hm, yeah, it probably would. Hadn't even thought to look at that. I'll
give that a closer look...

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH 2/3] mceusb: fix up reporting of trailing space

[Jarod Wilson]

On Fri, Oct 29, 2010 at 09:21:21PM +0200, David Härdeman wrote:
> On Thu, Oct 28, 2010 at 11:08:10PM -0400, Jarod Wilson wrote:
> > We were storing a bunch of spaces at the end of each signal, rather than
> > a single long space. The in-kernel decoders were actually okay with
> > this, but lirc isn't. Both are happy again with this change, which
> > starts accumulating data upon seeing an 0x7f space, and then stores it
> > when we see the next non-space, non-0x7f space, or an 0x80 end of signal
> > command. To get to that final 0x80 properly, we also need to support
> > proper parsing of 0x9f 0x01 commands, support for which is also added.
> 
> I think the driver could be further simplified by using 
> ir_raw_event_store_with_filter(), right?

And in fact, it is. I've got a new series of patches redone atop your
rc-core patch series that includes usage of _with_filter in mceusb.

http://git.kernel.org/?p=linux/kernel/git/jarod/linux-2.6-ir.git;a=shortlog;h=refs/heads/staging

And more specifically, the update of this patch:

http://git.kernel.org/?p=linux/kernel/git/jarod/linux-2.6-ir.git;a=commitdiff;h=bc3d1300cd2d51dc8d877be343382d8932320dfc

Still a bit of testing to do before I send v2 off to the list, plus I'd
like to know where we're at w/your patches first wrt getting them
committed, just in case I need to rework them slightly.

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH 2/3] mceusb: fix up reporting of trailing space

[David Härdeman]

On Tue, 2 Nov 2010 17:12:14 -0400, Jarod Wilson <jarod@redhat.com> wrote:
> On Fri, Oct 29, 2010 at 09:21:21PM +0200, David Härdeman wrote:
>> I think the driver could be further simplified by using 
>> ir_raw_event_store_with_filter(), right?
> 
> And in fact, it is. I've got a new series of patches redone atop your
> rc-core patch series that includes usage of _with_filter in mceusb.

>From a quick check, I think the entire PARSE_IRDATA block in
mceusb_process_ir_data() could be simplified to:

case PARSE_IRDATA:
    ir->rem--;
    rawir.pulse = !!(ir->buf_in[i] & MCE_PULSE_BIT);
    rawir.duration = (ir->buf_in[i] & MCE_PULSE_MASK)
                      * MCE_TIME_UNIT * 1000;
    ir_raw_event_store_with_filter(ir->rc, &rawir);
    break;

And you can then remove "struct ir_raw_event rawir" from struct
mceusb_dev.

-- 
David Härdeman

Re: [PATCH 2/3] mceusb: fix up reporting of trailing space

[Jarod Wilson]

On Wed, Nov 03, 2010 at 01:15:30PM +0100, David Härdeman wrote:
> On Tue, 2 Nov 2010 17:12:14 -0400, Jarod Wilson <jarod@redhat.com> wrote:
> > On Fri, Oct 29, 2010 at 09:21:21PM +0200, David Härdeman wrote:
> >> I think the driver could be further simplified by using 
> >> ir_raw_event_store_with_filter(), right?
> > 
> > And in fact, it is. I've got a new series of patches redone atop your
> > rc-core patch series that includes usage of _with_filter in mceusb.
> 
> From a quick check, I think the entire PARSE_IRDATA block in
> mceusb_process_ir_data() could be simplified to:
> 
> case PARSE_IRDATA:
>     ir->rem--;
>     rawir.pulse = !!(ir->buf_in[i] & MCE_PULSE_BIT);
>     rawir.duration = (ir->buf_in[i] & MCE_PULSE_MASK)
>                       * MCE_TIME_UNIT * 1000;
>     ir_raw_event_store_with_filter(ir->rc, &rawir);
>     break;
> 
> And you can then remove "struct ir_raw_event rawir" from struct
> mceusb_dev.

Once again, I think you're correct. :) I'll give this a spin with a few
mceusb devices, but it does certainly look like that'll work, now that
we're using _with_filter.

-- 
Jarod Wilson
jarod@redhat.com