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

[Mauro Carvalho Chehab <mchehab@redhat.com>]

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