[PATCH 0/3] ir-kbd-i2c, cx18: IR devices for CX23418 boards

[PATCH 0/3] ir-kbd-i2c, cx18: IR devices for CX23418 boards

[Andy Walls]

Jean,

The following patch series is my preliminary cut at getting the cx18
bridge driver supported IR devices set up properly by the cx18 driver to
allow use by ir-kbd-i2c, lirc_i2c, lirc_pvr150, and lirc_zilog for both
old and new (>= 2.6.30) kernels.

They are:

1/3: ir-kbd-i2c: Allow use of ir-kdb-i2c internal get_key funcs and set ir_type
2/3: cx18: Add i2c initialization for Z8F0811/Hauppage IR transceivers
3/3: ir-kbd-i2c: Add support for Z8F0811/Hauppage IR transceivers

Please take a look and tell me what's wrong.  I put specific points of
concern I have before each patch.

If this works for both ir-kbd-i2c and lirc_*, then I can add similar
logic to fix up ivtv (at least for Zilog Z8 microcontroller IR devices).

Regards,
Andy

[PATCH 1/3] ir-kbd-i2c: Allow use of ir-kdb-i2c internal get_key funcs and set ir_type

[Andy Walls]

This patch augments the init data passed by bridge drivers to ir-kbd-i2c
so that the ir_type can be set explicitly and so ir-kbd-i2c internal
get_key functions can be reused without requiring symbols from
ir-kbd-i2c in the bridge driver.


Regards,
Andy

diff -r d754a2d5a376 linux/drivers/media/video/ir-kbd-i2c.c
--- a/linux/drivers/media/video/ir-kbd-i2c.c	Wed Jul 15 07:28:02 2009 -0300
+++ b/linux/drivers/media/video/ir-kbd-i2c.c	Fri Jul 17 16:05:28 2009 -0400
@@ -478,7 +480,34 @@
 
 		ir_codes = init_data->ir_codes;
 		name = init_data->name;
+		if (init_data->type)
+			ir_type = init_data->type;
 		ir->get_key = init_data->get_key;
+		switch (init_data->internal_get_key_func) {
+		case IR_KBD_GET_KEY_PIXELVIEW:
+			ir->get_key = get_key_pixelview;
+			break;
+		case IR_KBD_GET_KEY_PV951:
+			ir->get_key = get_key_pv951;
+			break;
+		case IR_KBD_GET_KEY_HAUP:
+			ir->get_key = get_key_haup;
+			break;
+		case IR_KBD_GET_KEY_KNC1:
+			ir->get_key = get_key_knc1;
+			break;
+		case IR_KBD_GET_KEY_FUSIONHDTV:
+			ir->get_key = get_key_fusionhdtv;
+			break;
+		case IR_KBD_GET_KEY_HAUP_XVR:
+			ir->get_key = get_key_haup_xvr;
+			break;
+		case IR_KBD_GET_KEY_AVERMEDIA_CARDBUS:
+			ir->get_key = get_key_avermedia_cardbus;
+			break;
+		default:
+			break;
+		}
 	}
 
 	/* Make sure we are all setup before going on */
diff -r d754a2d5a376 linux/include/media/ir-kbd-i2c.h
--- a/linux/include/media/ir-kbd-i2c.h	Wed Jul 15 07:28:02 2009 -0300
+++ b/linux/include/media/ir-kbd-i2c.h	Fri Jul 17 16:05:28 2009 -0400
@@ -24,10 +24,27 @@
 	int                    (*get_key)(struct IR_i2c*, u32*, u32*);
 };
 
+enum ir_kbd_get_key_fn {
+	IR_KBD_GET_KEY_NONE = 0,
+	IR_KBD_GET_KEY_PIXELVIEW,
+	IR_KBD_GET_KEY_PV951,
+	IR_KBD_GET_KEY_HAUP,
+	IR_KBD_GET_KEY_KNC1,
+	IR_KBD_GET_KEY_FUSIONHDTV,
+	IR_KBD_GET_KEY_HAUP_XVR,
+	IR_KBD_GET_KEY_AVERMEDIA_CARDBUS,
+};
+
 /* Can be passed when instantiating an ir_video i2c device */
 struct IR_i2c_init_data {
 	IR_KEYTAB_TYPE         *ir_codes;
 	const char             *name;
+	int                    type; /* IR_TYPE_RC5, IR_TYPE_PD, etc */
+	/*
+	 * Specify either a function pointer or a value indicating one of
+	 * ir_kbd_i2c's internal get_key functions
+	 */
 	int                    (*get_key)(struct IR_i2c*, u32*, u32*);
+	enum ir_kbd_get_key_fn internal_get_key_func;
 };
 #endif
 

[PATCH 2/3] 2/3: cx18: Add i2c initialization for Z8F0811/Hauppage IR transceivers

[Andy Walls]

This patch add support to the cx18 driver for setting up the
Z8F0811/Hauppauge IR Tx/Rx chip with the i2c binding model in newer
kernels.

My concerns/questions:

1. When using the new i2c binding model, I'm not saving the returned
pointer from i2c_new_probed_device() and am hence taking no action on
trying to clean up IR i2c devices on module unload.  Will the i2c
subsystem clean up this automatically when the adapter is unregistered
on module unload?

2. When using the new i2c binding model, I'm calling
i2c_new_probed_device() twice: once for Rx (addr 0x71 of the Z8F0811)
and another time for Tx (addr 0x70 of the Z8F0811).  Is it a problem to
have two Linux i2c devices for two distinct addresses of the same
underlying hardware device?

3. When using the new i2c binding model, I opted not to use ir_video for
the Z8F0811 loaded with microcode from Zilog/Hauppauge.  Since I needed
one name for Rx binding and one for Tx binding, I used these names:

 	"ir_tx_z8f0811_haup"
	"ir_rx_z8f0811_haup"

[Which is ir_(func)_(part number)_(firmware_oem)].  It made sense to me.
I assume these are the names to which ir-kbd-i2c and lirc_* will have to
bind.  Is that correct?


Regards,
Andy



diff -r d754a2d5a376 linux/drivers/media/video/cx18/cx18-cards.c
--- a/linux/drivers/media/video/cx18/cx18-cards.c	Wed Jul 15 07:28:02 2009 -0300
+++ b/linux/drivers/media/video/cx18/cx18-cards.c	Fri Jul 17 16:05:28 2009 -0400
@@ -56,7 +56,8 @@
 	.hw_audio_ctrl = CX18_HW_418_AV,
 	.hw_muxer = CX18_HW_CS5345,
 	.hw_all = CX18_HW_TVEEPROM | CX18_HW_418_AV | CX18_HW_TUNER |
-		  CX18_HW_CS5345 | CX18_HW_DVB | CX18_HW_GPIO_RESET_CTRL,
+		  CX18_HW_CS5345 | CX18_HW_DVB | CX18_HW_GPIO_RESET_CTRL |
+		  CX18_HW_Z8F0811_IR_HAUP,
 	.video_inputs = {
 		{ CX18_CARD_INPUT_VID_TUNER,  0, CX18_AV_COMPOSITE7 },
 		{ CX18_CARD_INPUT_SVIDEO1,    1, CX18_AV_SVIDEO1    },
@@ -102,7 +103,8 @@
 	.hw_audio_ctrl = CX18_HW_418_AV,
 	.hw_muxer = CX18_HW_CS5345,
 	.hw_all = CX18_HW_TVEEPROM | CX18_HW_418_AV | CX18_HW_TUNER |
-		  CX18_HW_CS5345 | CX18_HW_DVB | CX18_HW_GPIO_RESET_CTRL,
+		  CX18_HW_CS5345 | CX18_HW_DVB | CX18_HW_GPIO_RESET_CTRL |
+		  CX18_HW_Z8F0811_IR_HAUP,
 	.video_inputs = {
 		{ CX18_CARD_INPUT_VID_TUNER,  0, CX18_AV_COMPOSITE7 },
 		{ CX18_CARD_INPUT_SVIDEO1,    1, CX18_AV_SVIDEO1    },
diff -r d754a2d5a376 linux/drivers/media/video/cx18/cx18-cards.h
--- a/linux/drivers/media/video/cx18/cx18-cards.h	Wed Jul 15 07:28:02 2009 -0300
+++ b/linux/drivers/media/video/cx18/cx18-cards.h	Fri Jul 17 16:05:28 2009 -0400
@@ -22,13 +22,17 @@
  */
 
 /* hardware flags */
-#define CX18_HW_TUNER		(1 << 0)
-#define CX18_HW_TVEEPROM	(1 << 1)
-#define CX18_HW_CS5345		(1 << 2)
-#define CX18_HW_DVB		(1 << 3)
-#define CX18_HW_418_AV		(1 << 4)
-#define CX18_HW_GPIO_MUX	(1 << 5)
-#define CX18_HW_GPIO_RESET_CTRL	(1 << 6)
+#define CX18_HW_TUNER			(1 << 0)
+#define CX18_HW_TVEEPROM		(1 << 1)
+#define CX18_HW_CS5345			(1 << 2)
+#define CX18_HW_DVB			(1 << 3)
+#define CX18_HW_418_AV			(1 << 4)
+#define CX18_HW_GPIO_MUX		(1 << 5)
+#define CX18_HW_GPIO_RESET_CTRL		(1 << 6)
+#define CX18_HW_Z8F0811_IR_TX_HAUP	(1 << 7)
+#define CX18_HW_Z8F0811_IR_RX_HAUP	(1 << 8)
+#define CX18_HW_Z8F0811_IR_HAUP	(CX18_HW_Z8F0811_IR_RX_HAUP | \
+				 CX18_HW_Z8F0811_IR_TX_HAUP)
 
 /* video inputs */
 #define	CX18_CARD_INPUT_VID_TUNER	1
diff -r d754a2d5a376 linux/drivers/media/video/cx18/cx18-i2c.c
--- a/linux/drivers/media/video/cx18/cx18-i2c.c	Wed Jul 15 07:28:02 2009 -0300
+++ b/linux/drivers/media/video/cx18/cx18-i2c.c	Fri Jul 17 16:05:28 2009 -0400
@@ -40,16 +40,20 @@
 #define GETSDL_BIT      0x0008
 
 #define CX18_CS5345_I2C_ADDR		0x4c
+#define CX18_Z8F0811_IR_TX_I2C_ADDR	0x70
+#define CX18_Z8F0811_IR_RX_I2C_ADDR	0x71
 
 /* This array should match the CX18_HW_ defines */
 static const u8 hw_addrs[] = {
-	0,			/* CX18_HW_TUNER */
-	0,			/* CX18_HW_TVEEPROM */
-	CX18_CS5345_I2C_ADDR,	/* CX18_HW_CS5345 */
-	0,			/* CX18_HW_DVB */
-	0,			/* CX18_HW_418_AV */
-	0,			/* CX18_HW_GPIO_MUX */
-	0,			/* CX18_HW_GPIO_RESET_CTRL */
+	0,				/* CX18_HW_TUNER */
+	0,				/* CX18_HW_TVEEPROM */
+	CX18_CS5345_I2C_ADDR,		/* CX18_HW_CS5345 */
+	0,				/* CX18_HW_DVB */
+	0,				/* CX18_HW_418_AV */
+	0,				/* CX18_HW_GPIO_MUX */
+	0,				/* CX18_HW_GPIO_RESET_CTRL */
+	CX18_Z8F0811_IR_TX_I2C_ADDR,	/* CX18_HW_Z8F0811_IR_TX_HAUP */
+	CX18_Z8F0811_IR_RX_I2C_ADDR,	/* CX18_HW_Z8F0811_IR_RX_HAUP */
 };
 
 /* This array should match the CX18_HW_ defines */
@@ -62,6 +66,8 @@
 	0,	/* CX18_HW_418_AV */
 	0,	/* CX18_HW_GPIO_MUX */
 	0,	/* CX18_HW_GPIO_RESET_CTRL */
+	0,	/* CX18_HW_Z8F0811_IR_TX_HAUP */
+	0,	/* CX18_HW_Z8F0811_IR_RX_HAUP */
 };
 
 /* This array should match the CX18_HW_ defines */
@@ -73,6 +79,8 @@
 	NULL,		/* CX18_HW_418_AV */
 	NULL,		/* CX18_HW_GPIO_MUX */
 	NULL,		/* CX18_HW_GPIO_RESET_CTRL */
+	NULL,		/* CX18_HW_Z8F0811_IR_TX_HAUP */
+	NULL,		/* CX18_HW_Z8F0811_IR_RX_HAUP */
 };
 
 /* This array should match the CX18_HW_ defines */
@@ -84,8 +92,43 @@
 	"cx23418_AV",
 	"gpio_mux",
 	"gpio_reset_ctrl",
+	"ir_tx_z8f0811_haup",
+	"ir_rx_z8f0811_haup",
 };
 
+static int cx18_i2c_new_ir(struct i2c_adapter *adap, u32 hw, const char *type,
+			   u8 addr)
+{
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 30)
+	struct i2c_board_info info;
+	struct IR_i2c_init_data ir_init_data;
+	unsigned short addr_list[2] = { addr, I2C_CLIENT_END };
+
+	memset(&info, 0, sizeof(struct i2c_board_info));
+	strlcpy(info.type, type, I2C_NAME_SIZE);
+
+	/* Our default information for ir-kbd-i2c.c to use */
+	memset(&ir_init_data, 0, sizeof(struct IR_i2c_init_data));
+	switch (hw) {
+	case CX18_HW_Z8F0811_IR_RX_HAUP:
+		ir_init_data.ir_codes = ir_codes_hauppauge_new;
+		ir_init_data.get_key = NULL;
+		ir_init_data.internal_get_key_func = IR_KBD_GET_KEY_HAUP_XVR;
+		ir_init_data.type = IR_TYPE_RC5;
+		ir_init_data.name = "CX23418 Z8F0811 Hauppauge";
+		break;
+	default:
+		break;
+	}
+	if (ir_init_data.name)
+		info.platform_data = &ir_init_data;
+
+	return i2c_new_probed_device(adap, &info, addr_list) == NULL ? -1 : 0;
+#else
+	return -1;
+#endif
+}
+
 int cx18_i2c_register(struct cx18 *cx, unsigned idx)
 {
 	struct v4l2_subdev *sd;
@@ -119,7 +162,10 @@
 	if (!hw_addrs[idx])
 		return -1;
 
-	/* It's an I2C device other than an analog tuner */
+	if (hw & CX18_HW_Z8F0811_IR_HAUP)
+		return cx18_i2c_new_ir(adap, hw, type, hw_addrs[idx]);
+
+	/* It's an I2C device other than an analog tuner or IR chip */
 	sd = v4l2_i2c_new_subdev(&cx->v4l2_dev, adap, mod, type, hw_addrs[idx]);
 	if (sd != NULL)
 		sd->grp_id = hw;

[PATCH 3/3] ir-kbd-i2c: Add support for Z8F0811/Hauppage IR transceivers

[Andy Walls]

This patch adds support for Zilog Z8F0811 IR transceiver chips on
CX2341[68] based boards to ir-kbd-i2c for both the old i2c binding model
and the new i2c binding model.

Regards,
Andy

diff -r d754a2d5a376 linux/drivers/media/video/ir-kbd-i2c.c
--- a/linux/drivers/media/video/ir-kbd-i2c.c	Wed Jul 15 07:28:02 2009 -0300
+++ b/linux/drivers/media/video/ir-kbd-i2c.c	Fri Jul 17 16:05:28 2009 -0400
@@ -442,9 +442,11 @@
 	case 0x47:
 	case 0x71:
 	case 0x2d:
-		if (adap->id == I2C_HW_B_CX2388x) {
+		if (adap->id == I2C_HW_B_CX2388x ||
+		    adap->id == I2C_HW_B_CX2341X) {
 			/* Handled by cx88-input */
-			name        = "CX2388x remote";
+			name = adap->id == I2C_HW_B_CX2341X ? "CX2341x remote"
+							    : "CX2388x remote";
 			ir_type     = IR_TYPE_RC5;
 			ir->get_key = get_key_haup_xvr;
 			if (hauppauge == 1) {
@@ -697,7 +726,8 @@
 static const struct i2c_device_id ir_kbd_id[] = {
 	/* Generic entry for any IR receiver */
 	{ "ir_video", 0 },
-	/* IR device specific entries could be added here */
+	/* IR device specific entries should be added here */
+	{ "ir_rx_z8f0811_haup", 0 },
 	{ }
 };
 

Re: [PATCH 1/3] ir-kbd-i2c: Allow use of ir-kdb-i2c internal get_key funcs and set ir_type

[Jean Delvare]

Hi Andy,

On Fri, 17 Jul 2009 16:35:37 -0400, Andy Walls wrote:
> This patch augments the init data passed by bridge drivers to ir-kbd-i2c
> so that the ir_type can be set explicitly and so ir-kbd-i2c internal
> get_key functions can be reused without requiring symbols from
> ir-kbd-i2c in the bridge driver.
> 
> 
> Regards,
> Andy

Looks good. Minor suggestion below:

> 
> diff -r d754a2d5a376 linux/drivers/media/video/ir-kbd-i2c.c
> --- a/linux/drivers/media/video/ir-kbd-i2c.c	Wed Jul 15 07:28:02 2009 -0300
> +++ b/linux/drivers/media/video/ir-kbd-i2c.c	Fri Jul 17 16:05:28 2009 -0400
> @@ -478,7 +480,34 @@
>  
>  		ir_codes = init_data->ir_codes;
>  		name = init_data->name;
> +		if (init_data->type)
> +			ir_type = init_data->type;
>  		ir->get_key = init_data->get_key;
> +		switch (init_data->internal_get_key_func) {
> +		case IR_KBD_GET_KEY_PIXELVIEW:
> +			ir->get_key = get_key_pixelview;
> +			break;
> +		case IR_KBD_GET_KEY_PV951:
> +			ir->get_key = get_key_pv951;
> +			break;
> +		case IR_KBD_GET_KEY_HAUP:
> +			ir->get_key = get_key_haup;
> +			break;
> +		case IR_KBD_GET_KEY_KNC1:
> +			ir->get_key = get_key_knc1;
> +			break;
> +		case IR_KBD_GET_KEY_FUSIONHDTV:
> +			ir->get_key = get_key_fusionhdtv;
> +			break;
> +		case IR_KBD_GET_KEY_HAUP_XVR:
> +			ir->get_key = get_key_haup_xvr;
> +			break;
> +		case IR_KBD_GET_KEY_AVERMEDIA_CARDBUS:
> +			ir->get_key = get_key_avermedia_cardbus;
> +			break;
> +		default:
> +			break;
> +		}
>  	}
>  
>  	/* Make sure we are all setup before going on */
> diff -r d754a2d5a376 linux/include/media/ir-kbd-i2c.h
> --- a/linux/include/media/ir-kbd-i2c.h	Wed Jul 15 07:28:02 2009 -0300
> +++ b/linux/include/media/ir-kbd-i2c.h	Fri Jul 17 16:05:28 2009 -0400
> @@ -24,10 +24,27 @@
>  	int                    (*get_key)(struct IR_i2c*, u32*, u32*);
>  };
>  
> +enum ir_kbd_get_key_fn {
> +	IR_KBD_GET_KEY_NONE = 0,

As you never use IR_KBD_GET_KEY_NONE, you might as well not define it
and start with IR_KBD_GET_KEY_PIXELVIEW = 1. This would have the added
advantage that you could get rid of the "default" statement in the
above switch, letting gcc warn you (or any other developer) if you ever
add a new enum value and forget to handle it in ir_probe().

> +	IR_KBD_GET_KEY_PIXELVIEW,
> +	IR_KBD_GET_KEY_PV951,
> +	IR_KBD_GET_KEY_HAUP,
> +	IR_KBD_GET_KEY_KNC1,
> +	IR_KBD_GET_KEY_FUSIONHDTV,
> +	IR_KBD_GET_KEY_HAUP_XVR,
> +	IR_KBD_GET_KEY_AVERMEDIA_CARDBUS,
> +};
> +
>  /* Can be passed when instantiating an ir_video i2c device */
>  struct IR_i2c_init_data {
>  	IR_KEYTAB_TYPE         *ir_codes;
>  	const char             *name;
> +	int                    type; /* IR_TYPE_RC5, IR_TYPE_PD, etc */
> +	/*
> +	 * Specify either a function pointer or a value indicating one of
> +	 * ir_kbd_i2c's internal get_key functions
> +	 */
>  	int                    (*get_key)(struct IR_i2c*, u32*, u32*);
> +	enum ir_kbd_get_key_fn internal_get_key_func;
>  };
>  #endif


-- 
Jean Delvare

Re: [PATCH 1/3] ir-kbd-i2c: Allow use of ir-kdb-i2c internal get_key funcs and set ir_type

[Mark Lord]

While you folks are looking into ir-kbd-i2c,
perhaps one of you will fix the regressions
introduced in 2.6.31-* ?

The drive no longer detects/works with the I/R port on
the Hauppauge PVR-250 cards, which is a user-visible regression.

Cheers

Re: [PATCH 1/3] ir-kbd-i2c: Allow use of ir-kdb-i2c internal get_key funcs and set ir_type

[Jean Delvare]

Hi Mark,

On Sun, 19 Jul 2009 08:52:09 -0400, Mark Lord wrote:
> While you folks are looking into ir-kbd-i2c,
> perhaps one of you will fix the regressions
> introduced in 2.6.31-* ?
> 
> The drive no longer detects/works with the I/R port on
> the Hauppauge PVR-250 cards, which is a user-visible regression.

This is bad. If there a bugzilla entry? If not, where can I read more
details / get in touch with an affected user?

-- 
Jean Delvare

Re: [PATCH 1/3] ir-kbd-i2c: Allow use of ir-kdb-i2c internal get_key funcs and set ir_type

[Mark Lord]

Jean Delvare wrote:
> Hi Mark,
> 
> On Sun, 19 Jul 2009 08:52:09 -0400, Mark Lord wrote:
>> While you folks are looking into ir-kbd-i2c,
>> perhaps one of you will fix the regressions
>> introduced in 2.6.31-* ?
>>
>> The drive no longer detects/works with the I/R port on
>> the Hauppauge PVR-250 cards, which is a user-visible regression.
> 
> This is bad. If there a bugzilla entry? If not, where can I read more
> details / get in touch with an affected user?
..

I imagine there will be thousands of affected users once the kernel
is released, but for now I'll volunteer as a guinea-pig.

It is difficult to test with 2.6.31 on the system at present, though,
because that kernel also breaks other things that the MythTV box relies on,
and the system is in regular use as our only PVR.

Right now, all I know is, that the PVR-250 IR port did not show up
in /dev/input/ with 2.6.31 after loading ir_kbd_i2c.  But it does show
up there with all previous kernels going back to the 2.6.1x days.

So, to keep the pain level reasonable, perhaps you could send some
debugging patches, and I'll apply those, reconfigure the machine for
2.6.31 again, and collect some output for you.  And also perhaps try
a few things locally as well to speed up the process.

Okay?

Thanks

Re: [PATCH 1/3] ir-kbd-i2c: Allow use of ir-kdb-i2c internal get_key funcs and set ir_type

[Mark Lord]

Mark Lord wrote:
> Jean Delvare wrote:
>> Hi Mark,
>>
>> On Sun, 19 Jul 2009 08:52:09 -0400, Mark Lord wrote:
>>> While you folks are looking into ir-kbd-i2c,
>>> perhaps one of you will fix the regressions
>>> introduced in 2.6.31-* ?
>>>
>>> The drive no longer detects/works with the I/R port on
>>> the Hauppauge PVR-250 cards, which is a user-visible regression.
>>
>> This is bad. If there a bugzilla entry? If not, where can I read more
>> details / get in touch with an affected user?
> ..
> 
> I imagine there will be thousands of affected users once the kernel
> is released, but for now I'll volunteer as a guinea-pig.
> 
> It is difficult to test with 2.6.31 on the system at present, though,
> because that kernel also breaks other things that the MythTV box relies on,
> and the system is in regular use as our only PVR.
> 
> Right now, all I know is, that the PVR-250 IR port did not show up
> in /dev/input/ with 2.6.31 after loading ir_kbd_i2c.  But it does show
> up there with all previous kernels going back to the 2.6.1x days.
..

Actually, I meant to say that it does not show up in the output from
the lsinput command, whereas it did show up there in all previous kernels.

> So, to keep the pain level reasonable, perhaps you could send some
> debugging patches, and I'll apply those, reconfigure the machine for
> 2.6.31 again, and collect some output for you.  And also perhaps try
> a few things locally as well to speed up the process.
> 
> Okay?
> 
> Thanks
> 

Re: [PATCH 2/3] 2/3: cx18: Add i2c initialization for Z8F0811/Hauppage IR transceivers

[Jean Delvare]

Hi Andy,

On Fri, 17 Jul 2009 16:46:55 -0400, Andy Walls wrote:
> This patch add support to the cx18 driver for setting up the
> Z8F0811/Hauppauge IR Tx/Rx chip with the i2c binding model in newer
> kernels.
> 
> My concerns/questions:
> 
> 1. When using the new i2c binding model, I'm not saving the returned
> pointer from i2c_new_probed_device() and am hence taking no action on
> trying to clean up IR i2c devices on module unload.  Will the i2c
> subsystem clean up this automatically when the adapter is unregistered
> on module unload?

While this isn't currently documented, yes it will. If this ever
changes, I will first let i2c-core emit warnings and still clean up
orphan clients. But I suspect the current behavior is easier for
everyone and I couldn't see any reason to change it at this point.

> 2. When using the new i2c binding model, I'm calling
> i2c_new_probed_device() twice: once for Rx (addr 0x71 of the Z8F0811)
> and another time for Tx (addr 0x70 of the Z8F0811).  Is it a problem to
> have two Linux i2c devices for two distinct addresses of the same
> underlying hardware device?

No, this is not a problem. The fact that this is the same device behind
both addresses is an implementation detail. As long as both functions
are independent, it should work just fine.

> 3. When using the new i2c binding model, I opted not to use ir_video for
> the Z8F0811 loaded with microcode from Zilog/Hauppauge.  Since I needed
> one name for Rx binding and one for Tx binding, I used these names:
> 
>  	"ir_tx_z8f0811_haup"
> 	"ir_rx_z8f0811_haup"
> 
> [Which is ir_(func)_(part number)_(firmware_oem)].  It made sense to me.
> I assume these are the names to which ir-kbd-i2c and lirc_* will have to
> bind.  Is that correct?

Yes, this is correct, and the approach is good. Ideally the "ir_video"
type would not exist (or would go away over time) and we would have a
separate type name for each IR chip, resulting in much cleaner code.
The reason for the current implementation is solely historical.

Review:

> diff -r d754a2d5a376 linux/drivers/media/video/cx18/cx18-cards.c
> --- a/linux/drivers/media/video/cx18/cx18-cards.c	Wed Jul 15 07:28:02 2009 -0300
> +++ b/linux/drivers/media/video/cx18/cx18-cards.c	Fri Jul 17 16:05:28 2009 -0400
> @@ -56,7 +56,8 @@
>  	.hw_audio_ctrl = CX18_HW_418_AV,
>  	.hw_muxer = CX18_HW_CS5345,
>  	.hw_all = CX18_HW_TVEEPROM | CX18_HW_418_AV | CX18_HW_TUNER |
> -		  CX18_HW_CS5345 | CX18_HW_DVB | CX18_HW_GPIO_RESET_CTRL,
> +		  CX18_HW_CS5345 | CX18_HW_DVB | CX18_HW_GPIO_RESET_CTRL |
> +		  CX18_HW_Z8F0811_IR_HAUP,
>  	.video_inputs = {
>  		{ CX18_CARD_INPUT_VID_TUNER,  0, CX18_AV_COMPOSITE7 },
>  		{ CX18_CARD_INPUT_SVIDEO1,    1, CX18_AV_SVIDEO1    },
> @@ -102,7 +103,8 @@
>  	.hw_audio_ctrl = CX18_HW_418_AV,
>  	.hw_muxer = CX18_HW_CS5345,
>  	.hw_all = CX18_HW_TVEEPROM | CX18_HW_418_AV | CX18_HW_TUNER |
> -		  CX18_HW_CS5345 | CX18_HW_DVB | CX18_HW_GPIO_RESET_CTRL,
> +		  CX18_HW_CS5345 | CX18_HW_DVB | CX18_HW_GPIO_RESET_CTRL |
> +		  CX18_HW_Z8F0811_IR_HAUP,
>  	.video_inputs = {
>  		{ CX18_CARD_INPUT_VID_TUNER,  0, CX18_AV_COMPOSITE7 },
>  		{ CX18_CARD_INPUT_SVIDEO1,    1, CX18_AV_SVIDEO1    },
> diff -r d754a2d5a376 linux/drivers/media/video/cx18/cx18-cards.h
> --- a/linux/drivers/media/video/cx18/cx18-cards.h	Wed Jul 15 07:28:02 2009 -0300
> +++ b/linux/drivers/media/video/cx18/cx18-cards.h	Fri Jul 17 16:05:28 2009 -0400
> @@ -22,13 +22,17 @@
>   */
>  
>  /* hardware flags */
> -#define CX18_HW_TUNER		(1 << 0)
> -#define CX18_HW_TVEEPROM	(1 << 1)
> -#define CX18_HW_CS5345		(1 << 2)
> -#define CX18_HW_DVB		(1 << 3)
> -#define CX18_HW_418_AV		(1 << 4)
> -#define CX18_HW_GPIO_MUX	(1 << 5)
> -#define CX18_HW_GPIO_RESET_CTRL	(1 << 6)
> +#define CX18_HW_TUNER			(1 << 0)
> +#define CX18_HW_TVEEPROM		(1 << 1)
> +#define CX18_HW_CS5345			(1 << 2)
> +#define CX18_HW_DVB			(1 << 3)
> +#define CX18_HW_418_AV			(1 << 4)
> +#define CX18_HW_GPIO_MUX		(1 << 5)
> +#define CX18_HW_GPIO_RESET_CTRL		(1 << 6)
> +#define CX18_HW_Z8F0811_IR_TX_HAUP	(1 << 7)
> +#define CX18_HW_Z8F0811_IR_RX_HAUP	(1 << 8)
> +#define CX18_HW_Z8F0811_IR_HAUP	(CX18_HW_Z8F0811_IR_RX_HAUP | \
> +				 CX18_HW_Z8F0811_IR_TX_HAUP)
>  
>  /* video inputs */
>  #define	CX18_CARD_INPUT_VID_TUNER	1
> diff -r d754a2d5a376 linux/drivers/media/video/cx18/cx18-i2c.c
> --- a/linux/drivers/media/video/cx18/cx18-i2c.c	Wed Jul 15 07:28:02 2009 -0300
> +++ b/linux/drivers/media/video/cx18/cx18-i2c.c	Fri Jul 17 16:05:28 2009 -0400
> @@ -40,16 +40,20 @@
>  #define GETSDL_BIT      0x0008
>  
>  #define CX18_CS5345_I2C_ADDR		0x4c
> +#define CX18_Z8F0811_IR_TX_I2C_ADDR	0x70
> +#define CX18_Z8F0811_IR_RX_I2C_ADDR	0x71
>  
>  /* This array should match the CX18_HW_ defines */
>  static const u8 hw_addrs[] = {
> -	0,			/* CX18_HW_TUNER */
> -	0,			/* CX18_HW_TVEEPROM */
> -	CX18_CS5345_I2C_ADDR,	/* CX18_HW_CS5345 */
> -	0,			/* CX18_HW_DVB */
> -	0,			/* CX18_HW_418_AV */
> -	0,			/* CX18_HW_GPIO_MUX */
> -	0,			/* CX18_HW_GPIO_RESET_CTRL */
> +	0,				/* CX18_HW_TUNER */
> +	0,				/* CX18_HW_TVEEPROM */
> +	CX18_CS5345_I2C_ADDR,		/* CX18_HW_CS5345 */
> +	0,				/* CX18_HW_DVB */
> +	0,				/* CX18_HW_418_AV */
> +	0,				/* CX18_HW_GPIO_MUX */
> +	0,				/* CX18_HW_GPIO_RESET_CTRL */
> +	CX18_Z8F0811_IR_TX_I2C_ADDR,	/* CX18_HW_Z8F0811_IR_TX_HAUP */
> +	CX18_Z8F0811_IR_RX_I2C_ADDR,	/* CX18_HW_Z8F0811_IR_RX_HAUP */
>  };
>  
>  /* This array should match the CX18_HW_ defines */
> @@ -62,6 +66,8 @@
>  	0,	/* CX18_HW_418_AV */
>  	0,	/* CX18_HW_GPIO_MUX */
>  	0,	/* CX18_HW_GPIO_RESET_CTRL */
> +	0,	/* CX18_HW_Z8F0811_IR_TX_HAUP */
> +	0,	/* CX18_HW_Z8F0811_IR_RX_HAUP */
>  };
>  
>  /* This array should match the CX18_HW_ defines */
> @@ -73,6 +79,8 @@
>  	NULL,		/* CX18_HW_418_AV */
>  	NULL,		/* CX18_HW_GPIO_MUX */
>  	NULL,		/* CX18_HW_GPIO_RESET_CTRL */
> +	NULL,		/* CX18_HW_Z8F0811_IR_TX_HAUP */
> +	NULL,		/* CX18_HW_Z8F0811_IR_RX_HAUP */
>  };
>  
>  /* This array should match the CX18_HW_ defines */
> @@ -84,8 +92,43 @@
>  	"cx23418_AV",
>  	"gpio_mux",
>  	"gpio_reset_ctrl",
> +	"ir_tx_z8f0811_haup",
> +	"ir_rx_z8f0811_haup",
>  };
>  
> +static int cx18_i2c_new_ir(struct i2c_adapter *adap, u32 hw, const char *type,
> +			   u8 addr)
> +{
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 30)
> +	struct i2c_board_info info;
> +	struct IR_i2c_init_data ir_init_data;
> +	unsigned short addr_list[2] = { addr, I2C_CLIENT_END };
> +
> +	memset(&info, 0, sizeof(struct i2c_board_info));
> +	strlcpy(info.type, type, I2C_NAME_SIZE);
> +
> +	/* Our default information for ir-kbd-i2c.c to use */
> +	memset(&ir_init_data, 0, sizeof(struct IR_i2c_init_data));
> +	switch (hw) {
> +	case CX18_HW_Z8F0811_IR_RX_HAUP:
> +		ir_init_data.ir_codes = ir_codes_hauppauge_new;
> +		ir_init_data.get_key = NULL;

This is a no-op, as ir_init_data was cleared with memset() right above.

> +		ir_init_data.internal_get_key_func = IR_KBD_GET_KEY_HAUP_XVR;
> +		ir_init_data.type = IR_TYPE_RC5;
> +		ir_init_data.name = "CX23418 Z8F0811 Hauppauge";
> +		break;
> +	default:
> +		break;
> +	}
> +	if (ir_init_data.name)
> +		info.platform_data = &ir_init_data;

Not sure why you don't just put this in the switch to save a test? Even
the memset could go there as far as I can see.

> +
> +	return i2c_new_probed_device(adap, &info, addr_list) == NULL ? -1 : 0;
> +#else
> +	return -1;
> +#endif
> +}
> +
>  int cx18_i2c_register(struct cx18 *cx, unsigned idx)
>  {
>  	struct v4l2_subdev *sd;
> @@ -119,7 +162,10 @@
>  	if (!hw_addrs[idx])
>  		return -1;
>  
> -	/* It's an I2C device other than an analog tuner */
> +	if (hw & CX18_HW_Z8F0811_IR_HAUP)
> +		return cx18_i2c_new_ir(adap, hw, type, hw_addrs[idx]);

For consistency I'd move this up a bit (although I agree it is
functionally equivalent.)

> +
> +	/* It's an I2C device other than an analog tuner or IR chip */
>  	sd = v4l2_i2c_new_subdev(&cx->v4l2_dev, adap, mod, type, hw_addrs[idx]);
>  	if (sd != NULL)
>  		sd->grp_id = hw;
> 
> 

The rest looks OK.

-- 
Jean Delvare

Re: [PATCH 1/3] ir-kbd-i2c: Allow use of ir-kdb-i2c internal get_key funcs and set ir_type

[Mark Lord]

Mark Lord wrote:
> Mark Lord wrote:
>> Jean Delvare wrote:
>>> Hi Mark,
>>>
>>> On Sun, 19 Jul 2009 08:52:09 -0400, Mark Lord wrote:
>>>> While you folks are looking into ir-kbd-i2c,
>>>> perhaps one of you will fix the regressions
>>>> introduced in 2.6.31-* ?
>>>>
>>>> The drive no longer detects/works with the I/R port on
>>>> the Hauppauge PVR-250 cards, which is a user-visible regression.
>>>
>>> This is bad. If there a bugzilla entry? If not, where can I read more
>>> details / get in touch with an affected user?
>> ..
>>
>> I imagine there will be thousands of affected users once the kernel
>> is released, but for now I'll volunteer as a guinea-pig.
>>
>> It is difficult to test with 2.6.31 on the system at present, though,
>> because that kernel also breaks other things that the MythTV box 
>> relies on,
>> and the system is in regular use as our only PVR.
>>
>> Right now, all I know is, that the PVR-250 IR port did not show up
>> in /dev/input/ with 2.6.31 after loading ir_kbd_i2c.  But it does show
>> up there with all previous kernels going back to the 2.6.1x days.
> ..
> 
> Actually, I meant to say that it does not show up in the output from
> the lsinput command, whereas it did show up there in all previous kernels.
> 
>> So, to keep the pain level reasonable, perhaps you could send some
>> debugging patches, and I'll apply those, reconfigure the machine for
>> 2.6.31 again, and collect some output for you.  And also perhaps try
>> a few things locally as well to speed up the process.
..

I'm debugging various other b0rked things in 2.6.31 here right now,
so I had a closer look at the Hauppauge I/R remote issue.

The ir_kbd_i2c driver *does* still find it after all.
But the difference is that the output from 'lsinput' has changed
and no longer says "Hauppauge".  Which prevents the application from
finding the remote control in the same way as before.

I'll hack the application code here now to use the new output,
but I wonder what the the thousands of other users will do when
they first try 2.6.31 after release ?

Cheers

Re: [PATCH 1/3] ir-kbd-i2c: Allow use of ir-kdb-i2c internal get_key funcs and set ir_type

[Jean Delvare]

On Sun, 19 Jul 2009 09:17:30 -0400, Mark Lord wrote:
> Mark Lord wrote:
> > Jean Delvare wrote:
> >> Hi Mark,
> >>
> >> On Sun, 19 Jul 2009 08:52:09 -0400, Mark Lord wrote:
> >>> While you folks are looking into ir-kbd-i2c,
> >>> perhaps one of you will fix the regressions
> >>> introduced in 2.6.31-* ?
> >>>
> >>> The drive no longer detects/works with the I/R port on
> >>> the Hauppauge PVR-250 cards, which is a user-visible regression.
> >>
> >> This is bad. If there a bugzilla entry? If not, where can I read more
> >> details / get in touch with an affected user?
> > ..
> > 
> > I imagine there will be thousands of affected users once the kernel
> > is released, but for now I'll volunteer as a guinea-pig.
> > 
> > It is difficult to test with 2.6.31 on the system at present, though,
> > because that kernel also breaks other things that the MythTV box relies on,
> > and the system is in regular use as our only PVR.
> > 
> > Right now, all I know is, that the PVR-250 IR port did not show up
> > in /dev/input/ with 2.6.31 after loading ir_kbd_i2c.  But it does show
> > up there with all previous kernels going back to the 2.6.1x days.
> ..
> 
> Actually, I meant to say that it does not show up in the output from
> the lsinput command, whereas it did show up there in all previous kernels.

Never heard of lsinput, where does it come from?

> 
> > So, to keep the pain level reasonable, perhaps you could send some
> > debugging patches, and I'll apply those, reconfigure the machine for
> > 2.6.31 again, and collect some output for you.  And also perhaps try
> > a few things locally as well to speed up the process.
> > 
> > Okay?

I'd need additional information first, otherwise I have no clue where
to start debugging. Which you just sent in a further post, as I see, so
I'll follow-up there...

-- 
Jean Delvare

Re: [PATCH 1/3] ir-kbd-i2c: Allow use of ir-kdb-i2c internal get_key funcs and set ir_type

[Jean Delvare]

On Sun, 19 Jul 2009 10:38:37 -0400, Mark Lord wrote:
> I'm debugging various other b0rked things in 2.6.31 here right now,
> so I had a closer look at the Hauppauge I/R remote issue.
> 
> The ir_kbd_i2c driver *does* still find it after all.
> But the difference is that the output from 'lsinput' has changed
> and no longer says "Hauppauge".  Which prevents the application from
> finding the remote control in the same way as before.

OK, thanks for the investigation.

> I'll hack the application code here now to use the new output,
> but I wonder what the the thousands of other users will do when
> they first try 2.6.31 after release ?

Where does lsinput get the string from?

What exactly was it before, and what is it exactly in 2.6.31?

-- 
Jean Delvare

Re: [PATCH 1/3] ir-kbd-i2c: Allow use of ir-kdb-i2c internal get_key funcs and set ir_type

[Mark Lord]

Jean Delvare wrote:
> On Sun, 19 Jul 2009 10:38:37 -0400, Mark Lord wrote:
>> I'm debugging various other b0rked things in 2.6.31 here right now,
>> so I had a closer look at the Hauppauge I/R remote issue.
>>
>> The ir_kbd_i2c driver *does* still find it after all.
>> But the difference is that the output from 'lsinput' has changed
>> and no longer says "Hauppauge".  Which prevents the application from
>> finding the remote control in the same way as before.
> 
> OK, thanks for the investigation.
> 
>> I'll hack the application code here now to use the new output,
>> but I wonder what the the thousands of other users will do when
>> they first try 2.6.31 after release ?
> 
> Where does lsinput get the string from?
..

Here's a test program for you:

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <errno.h>
#include <fcntl.h>
#include <sys/ioctl.h>
#include <linux/input.h>

// Invoke with "/dev/input/event4" as argv[1]
//
// On 2.6.30, this gives the real name, eg. "i2c IR (Hauppauge)".
// On 2.6.31, it simply gives "event4" as the "name".

int main(int argc, char *argv[])
{
	char buf[32];
	int fd, rc;

	fd = open(argv[1], O_RDONLY);
	if (fd == -1) {
		perror(argv[1]);
		exit(1);
	}
	rc = ioctl(fd,EVIOCGNAME(sizeof(buf)),buf);
	if (rc >= 0)
		fprintf(stderr,"   name    : \"%.*s\"\n", rc, buf);
	return 0;
}

Re: [PATCH 1/3] ir-kbd-i2c: Allow use of ir-kdb-i2c internal get_key funcs and set ir_type

[Mark Lord]

(resending.. somebody trimmed linux-kernel from the CC: earlier)

Jean Delvare wrote:
> On Sun, 19 Jul 2009 10:38:37 -0400, Mark Lord wrote:
>> I'm debugging various other b0rked things in 2.6.31 here right now,
>> so I had a closer look at the Hauppauge I/R remote issue.
>>
>> The ir_kbd_i2c driver *does* still find it after all.
>> But the difference is that the output from 'lsinput' has changed
>> and no longer says "Hauppauge".  Which prevents the application from
>> finding the remote control in the same way as before.
>
> OK, thanks for the investigation.
>
>> I'll hack the application code here now to use the new output,
>> but I wonder what the the thousands of other users will do when
>> they first try 2.6.31 after release ?
>
> Where does lsinput get the string from?
..

Here's a test program for you:

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <errno.h>
#include <fcntl.h>
#include <sys/ioctl.h>
#include <linux/input.h>

// Invoke with "/dev/input/event4" as argv[1]
//
// On 2.6.30, this gives the real name, eg. "i2c IR (Hauppauge)".
// On 2.6.31, it simply gives "event4" as the "name".

int main(int argc, char *argv[])
{
	char buf[32];
	int fd, rc;

	fd = open(argv[1], O_RDONLY);
	if (fd == -1) {
		perror(argv[1]);
		exit(1);
	}
	rc = ioctl(fd,EVIOCGNAME(sizeof(buf)),buf);
	if (rc >= 0)
		fprintf(stderr,"   name    : \"%.*s\"\n", rc, buf);
	return 0;
}

Regression 2.6.31: ioctl(EVIOCGNAME) no longer returns device name

[Mark Lord]

Mark Lord wrote:
> (resending.. somebody trimmed linux-kernel from the CC: earlier)
> 
> Jean Delvare wrote:
>> On Sun, 19 Jul 2009 10:38:37 -0400, Mark Lord wrote:
>>> I'm debugging various other b0rked things in 2.6.31 here right now,
>>> so I had a closer look at the Hauppauge I/R remote issue.
>>>
>>> The ir_kbd_i2c driver *does* still find it after all.
>>> But the difference is that the output from 'lsinput' has changed
>>> and no longer says "Hauppauge".  Which prevents the application from
>>> finding the remote control in the same way as before.
>>
>> OK, thanks for the investigation.
>>
>>> I'll hack the application code here now to use the new output,
>>> but I wonder what the the thousands of other users will do when
>>> they first try 2.6.31 after release ?
..

Mmm.. appears to be a systemwide thing, not just for the i2c stuff.
*All* of the input devices now no longer show their real names
when queried with ioctl(EVIOCGNAME).  This is a regression from 2.6.30.
Note that the real names *are* still stored somewhere, because they
do still show up correctly under /sys/


> Here's a test program for you:
> 
> #include <stdio.h>
> #include <stdlib.h>
> #include <unistd.h>
> #include <errno.h>
> #include <fcntl.h>
> #include <sys/ioctl.h>
> #include <linux/input.h>
> 
> // Invoke with "/dev/input/event4" as argv[1]
> //
> // On 2.6.30, this gives the real name, eg. "i2c IR (Hauppauge)".
> // On 2.6.31, it simply gives "event4" as the "name".
> 
> int main(int argc, char *argv[])
> {
>     char buf[32];
>     int fd, rc;
> 
>     fd = open(argv[1], O_RDONLY);
>     if (fd == -1) {
>         perror(argv[1]);
>         exit(1);
>     }
>     rc = ioctl(fd,EVIOCGNAME(sizeof(buf)),buf);
>     if (rc >= 0)
>         fprintf(stderr,"   name    : \"%.*s\"\n", rc, buf);
>     return 0;
> }
..

Since this regression should be visible on *any* system, not just mine,
I think perhaps the input-subsystem developers ought to be the ones to
go and burn some time on a git-bisect, if need be.

Eg.  Here's what's different on my notebook here:


--- lsinput.2.6.30	2009-07-19 15:14:38.278293568 -0400
+++ lsinput.2.6.31	2009-07-19 15:15:43.725375340 -0400
@@ -3,7 +3,7 @@
    vendor  : 0x1
    product : 0x1
    version : 43841
-   name    : "AT Translated Set 2 keyboard"
+   name    : "event0"
    phys    : "isa0060/serio0/input0"
    bits ev : EV_SYN EV_KEY EV_MSC EV_LED EV_REP
 
@@ -12,7 +12,7 @@
    vendor  : 0x2
    product : 0x7
    version : 4017
-   name    : "SynPS/2 Synaptics TouchPad"
+   name    : "event1"
    phys    : "isa0060/serio1/input0"
    bits ev : EV_SYN EV_KEY EV_ABS
 
@@ -21,7 +21,7 @@
    vendor  : 0x0
    product : 0x5
    version : 0
-   name    : "Lid Switch"
+   name    : "event2"
    phys    : "PNP0C0D/button/input0"
    bits ev : EV_SYN EV_SW
 
@@ -30,7 +30,7 @@
    vendor  : 0x0
    product : 0x1
    version : 0
-   name    : "Power Button"
+   name    : "event3"
    phys    : "PNP0C0C/button/input0"
    bits ev : EV_SYN EV_KEY
 
@@ -39,7 +39,7 @@
    vendor  : 0x0
    product : 0x3
    version : 0
-   name    : "Sleep Button"
+   name    : "event4"
    phys    : "PNP0C0E/button/input0"
    bits ev : EV_SYN EV_KEY
 
@@ -48,34 +48,16 @@
    vendor  : 0x1f
    product : 0x1
    version : 256
-   name    : "PC Speaker"
+   name    : "event5"
    phys    : "isa0061/input0"
    bits ev : EV_SYN EV_SND
 
 /dev/input/event6
-   bustype : (null)
-   vendor  : 0x0
-   product : 0x0
-   version : 0
-   name    : "HDA Intel Mic at Ext Right Jack"
-   phys    : "ALSA"
-   bits ev : EV_SYN EV_SW
-
-/dev/input/event7
-   bustype : (null)
-   vendor  : 0x0
-   product : 0x0
-   version : 0
-   name    : "HDA Intel HP Out at Ext Right Ja"
-   phys    : "ALSA"
-   bits ev : EV_SYN EV_SW
-
-/dev/input/event8
    bustype : BUS_USB
    vendor  : 0x46d
    product : 0xc016
    version : 272
-   name    : "Logitech Optical USB Mouse"
+   name    : "event6"
    phys    : "usb-0000:00:1d.7-5.4/input0"
    uniq    : ""
    bits ev : EV_SYN EV_KEY EV_REL EV_MSC

Re: [PATCH 3/3] ir-kbd-i2c: Add support for Z8F0811/Hauppage IR transceivers

[Jean Delvare]

Hi Andy,

On Fri, 17 Jul 2009 16:49:55 -0400, Andy Walls wrote:
> This patch adds support for Zilog Z8F0811 IR transceiver chips on
> CX2341[68] based boards to ir-kbd-i2c for both the old i2c binding model
> and the new i2c binding model.
> 
> Regards,
> Andy
> 
> diff -r d754a2d5a376 linux/drivers/media/video/ir-kbd-i2c.c
> --- a/linux/drivers/media/video/ir-kbd-i2c.c	Wed Jul 15 07:28:02 2009 -0300
> +++ b/linux/drivers/media/video/ir-kbd-i2c.c	Fri Jul 17 16:05:28 2009 -0400
> @@ -442,9 +442,11 @@
>  	case 0x47:
>  	case 0x71:
>  	case 0x2d:
> -		if (adap->id == I2C_HW_B_CX2388x) {
> +		if (adap->id == I2C_HW_B_CX2388x ||
> +		    adap->id == I2C_HW_B_CX2341X) {
>  			/* Handled by cx88-input */
> -			name        = "CX2388x remote";
> +			name = adap->id == I2C_HW_B_CX2341X ? "CX2341x remote"
> +							    : "CX2388x remote";
>  			ir_type     = IR_TYPE_RC5;
>  			ir->get_key = get_key_haup_xvr;
>  			if (hauppauge == 1) {
> @@ -697,7 +726,8 @@
>  static const struct i2c_device_id ir_kbd_id[] = {
>  	/* Generic entry for any IR receiver */
>  	{ "ir_video", 0 },
> -	/* IR device specific entries could be added here */
> +	/* IR device specific entries should be added here */
> +	{ "ir_rx_z8f0811_haup", 0 },
>  	{ }
>  };
>  

Yes, looks good.

-- 
Jean Delvare

Re: [PATCH 1/3] ir-kbd-i2c: Allow use of ir-kdb-i2c internal get_key funcs and set ir_type

[Dmitry Torokhov]

On Sun, Jul 19, 2009 at 02:26:53PM -0400, Mark Lord wrote:
> (resending.. somebody trimmed linux-kernel from the CC: earlier)
>
> Jean Delvare wrote:
>> On Sun, 19 Jul 2009 10:38:37 -0400, Mark Lord wrote:
>>> I'm debugging various other b0rked things in 2.6.31 here right now,
>>> so I had a closer look at the Hauppauge I/R remote issue.
>>>
>>> The ir_kbd_i2c driver *does* still find it after all.
>>> But the difference is that the output from 'lsinput' has changed
>>> and no longer says "Hauppauge".  Which prevents the application from
>>> finding the remote control in the same way as before.
>>
>> OK, thanks for the investigation.
>>
>>> I'll hack the application code here now to use the new output,
>>> but I wonder what the the thousands of other users will do when
>>> they first try 2.6.31 after release ?
>>
>> Where does lsinput get the string from?
> ..
>
> Here's a test program for you:
>

And I  think have a fix for that, commit

f936601471d1454dacbd3b2a961fd4d883090aeb

in the for-linus branch of my tree.

-- 
Dmitry

Re: Regression 2.6.31: ioctl(EVIOCGNAME) no longer returns device name

[Dmitry Torokhov]

On Sun, Jul 19, 2009 at 03:20:50PM -0400, Mark Lord wrote:
> Mark Lord wrote:
>> (resending.. somebody trimmed linux-kernel from the CC: earlier)
>>
>> Jean Delvare wrote:
>>> On Sun, 19 Jul 2009 10:38:37 -0400, Mark Lord wrote:
>>>> I'm debugging various other b0rked things in 2.6.31 here right now,
>>>> so I had a closer look at the Hauppauge I/R remote issue.
>>>>
>>>> The ir_kbd_i2c driver *does* still find it after all.
>>>> But the difference is that the output from 'lsinput' has changed
>>>> and no longer says "Hauppauge".  Which prevents the application from
>>>> finding the remote control in the same way as before.
>>>
>>> OK, thanks for the investigation.
>>>
>>>> I'll hack the application code here now to use the new output,
>>>> but I wonder what the the thousands of other users will do when
>>>> they first try 2.6.31 after release ?
> ..
>
> Mmm.. appears to be a systemwide thing, not just for the i2c stuff.
> *All* of the input devices now no longer show their real names
> when queried with ioctl(EVIOCGNAME).  This is a regression from 2.6.30.
> Note that the real names *are* still stored somewhere, because they
> do still show up correctly under /sys/
>

Should be fixed by f936601471d1454dacbd3b2a961fd4d883090aeb in the
for-linus branch of my tree.

-- 
Dmitry

Re: Regression 2.6.31: ioctl(EVIOCGNAME) no longer returns device name

[Jean Delvare]

On Sun, 19 Jul 2009 15:20:50 -0400, Mark Lord wrote:
> Mark Lord wrote:
> > (resending.. somebody trimmed linux-kernel from the CC: earlier)

FWIW I don't think it was there in the first place.

> > Jean Delvare wrote:
> >> On Sun, 19 Jul 2009 10:38:37 -0400, Mark Lord wrote:
> >>> I'm debugging various other b0rked things in 2.6.31 here right now,
> >>> so I had a closer look at the Hauppauge I/R remote issue.
> >>>
> >>> The ir_kbd_i2c driver *does* still find it after all.
> >>> But the difference is that the output from 'lsinput' has changed
> >>> and no longer says "Hauppauge".  Which prevents the application from
> >>> finding the remote control in the same way as before.
> >>
> >> OK, thanks for the investigation.
> >>
> >>> I'll hack the application code here now to use the new output,
> >>> but I wonder what the the thousands of other users will do when
> >>> they first try 2.6.31 after release ?
> ..
> 
> Mmm.. appears to be a systemwide thing, not just for the i2c stuff.
> *All* of the input devices now no longer show their real names
> when queried with ioctl(EVIOCGNAME).  This is a regression from 2.6.30.
> Note that the real names *are* still stored somewhere, because they
> do still show up correctly under /sys/

I was just coming to the same conclusion. So this doesn't have anything
to do with the ir-kbd-i2c conversion after all... This is something for
the input subsystem maintainers.

I suspect this commit is related to the regression:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=3d5cb60ef3042ac479dab82e5a945966a0d54d53

-- 
Jean Delvare

Re: Regression 2.6.31: ioctl(EVIOCGNAME) no longer returns device name

[Mark Lord]

Dmitry Torokhov wrote:
> On Sun, Jul 19, 2009 at 03:20:50PM -0400, Mark Lord wrote:
>> Mark Lord wrote:
>>> (resending.. somebody trimmed linux-kernel from the CC: earlier)
>>>
>>> Jean Delvare wrote:
>>>> On Sun, 19 Jul 2009 10:38:37 -0400, Mark Lord wrote:
>>>>> I'm debugging various other b0rked things in 2.6.31 here right now,
>>>>> so I had a closer look at the Hauppauge I/R remote issue.
>>>>>
>>>>> The ir_kbd_i2c driver *does* still find it after all.
>>>>> But the difference is that the output from 'lsinput' has changed
>>>>> and no longer says "Hauppauge".  Which prevents the application from
>>>>> finding the remote control in the same way as before.
>>>> OK, thanks for the investigation.
>>>>
>>>>> I'll hack the application code here now to use the new output,
>>>>> but I wonder what the the thousands of other users will do when
>>>>> they first try 2.6.31 after release ?
>> ..
>>
>> Mmm.. appears to be a systemwide thing, not just for the i2c stuff.
>> *All* of the input devices now no longer show their real names
>> when queried with ioctl(EVIOCGNAME).  This is a regression from 2.6.30.
>> Note that the real names *are* still stored somewhere, because they
>> do still show up correctly under /sys/
>>
> 
> Should be fixed by f936601471d1454dacbd3b2a961fd4d883090aeb in the
> for-linus branch of my tree.
..

Peachy.  Push it, or post it here and I can re-test with it.

(does anyone else find it spooky that a google search for the
 above commit id actually finds Dmitry's email quoted above ?
 Mere seconds after he posted it for the very first time ??)

Re: Regression 2.6.31: ioctl(EVIOCGNAME) no longer returns device name

[Andy Walls]

On Sun, 2009-07-19 at 16:14 -0400, Mark Lord wrote:
> Dmitry Torokhov wrote:
> > On Sun, Jul 19, 2009 at 03:20:50PM -0400, Mark Lord wrote:

> > Should be fixed by f936601471d1454dacbd3b2a961fd4d883090aeb in the
> > for-linus branch of my tree.
> ..
> 
> Peachy.  Push it, or post it here and I can re-test with it.
> 
> (does anyone else find it spooky that a google search for the
>  above commit id actually finds Dmitry's email quoted above ?
>  Mere seconds after he posted it for the very first time ??)

Not since he's using a Gmail account, no.  Google probably indexes it on
the way in.  Very effcient.

The google is reading ur e-mail...


-Andy

Re: [PATCH 2/3] 2/3: cx18: Add i2c initialization for Z8F0811/Hauppage IR transceivers

[Jarod Wilson]

On Sunday 19 July 2009 09:38:54 Jean Delvare wrote:
> > 3. When using the new i2c binding model, I opted not to use ir_video for
> > the Z8F0811 loaded with microcode from Zilog/Hauppauge.  Since I needed
> > one name for Rx binding and one for Tx binding, I used these names:
> > 
> >       "ir_tx_z8f0811_haup"
> >       "ir_rx_z8f0811_haup"
> > 
> > [Which is ir_(func)_(part number)_(firmware_oem)].  It made sense to me.
> > I assume these are the names to which ir-kbd-i2c and lirc_* will have to
> > bind.  Is that correct?
> 
> Yes, this is correct, and the approach is good. Ideally the "ir_video"
> type would not exist (or would go away over time) and we would have a
> separate type name for each IR chip, resulting in much cleaner code.
> The reason for the current implementation is solely historical.

Cool. When fixing up lirc_i2c, I actually *did* have a question about
that which I forgot about until reading this. The only name I could
find in use anywhere at a glance was ir_video, so that's what lirc_i2c
is set to hook up to for the moment, but yeah, device-specific names
instead would be great. Hrm. Offhand, I don't have a clue what the
actual IR chip is on the PVR-x50 series, let alone any of the other
cards lirc_i2c claims to support...

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH 2/3] 2/3: cx18: Add i2c initialization for Z8F0811/Hauppage IR transceivers

[Andy Walls]

On Mon, 2009-07-20 at 14:51 -0400, Jarod Wilson wrote:
> On Sunday 19 July 2009 09:38:54 Jean Delvare wrote:
> > > 3. When using the new i2c binding model, I opted not to use ir_video for
> > > the Z8F0811 loaded with microcode from Zilog/Hauppauge.  Since I needed
> > > one name for Rx binding and one for Tx binding, I used these names:
> > > 
> > >       "ir_tx_z8f0811_haup"
> > >       "ir_rx_z8f0811_haup"
> > > 
> > > [Which is ir_(func)_(part number)_(firmware_oem)].  It made sense to me.
> > > I assume these are the names to which ir-kbd-i2c and lirc_* will have to
> > > bind.  Is that correct?
> > 
> > Yes, this is correct, and the approach is good. Ideally the "ir_video"
> > type would not exist (or would go away over time) and we would have a
> > separate type name for each IR chip, resulting in much cleaner code.
> > The reason for the current implementation is solely historical.
> 
> Cool. When fixing up lirc_i2c, I actually *did* have a question about
> that which I forgot about until reading this. The only name I could
> find in use anywhere at a glance was ir_video, so that's what lirc_i2c
> is set to hook up to for the moment, but yeah, device-specific names
> instead would be great. 

Yes, I noted "ir_video" implied an IR receiver, but the IR blaster on
the HVR-1600 and newer PVR-150's can't be called "ir_video" and have
lirc_zilog do the right thing obviously.  So "in for a penny, in for a
pound..."


Based on earlier posts from Jean, note that for microcontrollers the
chip part number is not enough to uniquely identify the IR
implementation since the firmware can be different.  I used the name
"haup" to distinguish a Z8F0811 loaded with firmware from
Hauppauge/Zilog.

Given reports from users using lirc_pvr150, the different versions of
firmware loaded into th Z8F0811 chips on the Hauppaugue boards all seem
to be compatable to some degree.  Plus the IR program firmware can
report its exact version if needed.



> Hrm. Offhand, I don't have a clue what the
> actual IR chip is on the PVR-x50 series,

There are a few different IR chips on the PVR-x50 series AFAIK.  I know
that if you find one sitting at 0x71 on the PVR-x50's, then it's likely
a Zilog Z8 Encore! family microcontroller loaded with firmware program
that probably originates from Zilog.  (A Zilog EULA comes with the
Hauppauge Windows drivers.)

Actually the Zilog Z8 chips respond to 0x70: blaster, 0x71 receiver,
0x72 ???, 0x73 ???

I was kind of hoping that addresses 0x72 and 0x73 might support some
sort of "raw mode" or "learning mode", so I could to avoid the whole
lirc_zilog "firmware image" mess.  But I haven't had time to expriment.

Regards,
Andy

>  let alone any of the other
> cards lirc_i2c claims to support...

Re: [PATCH 1/3] ir-kbd-i2c: Allow use of ir-kdb-i2c internal get_key funcs and set ir_type

[Andy Walls]

On Sun, 2009-07-19 at 14:47 +0200, Jean Delvare wrote:
> Hi Andy,
> 
> On Fri, 17 Jul 2009 16:35:37 -0400, Andy Walls wrote:
> > This patch augments the init data passed by bridge drivers to ir-kbd-i2c
> > so that the ir_type can be set explicitly and so ir-kbd-i2c internal
> > get_key functions can be reused without requiring symbols from
> > ir-kbd-i2c in the bridge driver.
> > 
> > 
> > Regards,
> > Andy
> 
> Looks good. Minor suggestion below:

Jean,

Thanks for the reply.  My responses are inline.

> > 
> > diff -r d754a2d5a376 linux/drivers/media/video/ir-kbd-i2c.c
> > --- a/linux/drivers/media/video/ir-kbd-i2c.c	Wed Jul 15 07:28:02 2009 -0300
> > +++ b/linux/drivers/media/video/ir-kbd-i2c.c	Fri Jul 17 16:05:28 2009 -0400
> > @@ -478,7 +480,34 @@
> >  
> >  		ir_codes = init_data->ir_codes;
> >  		name = init_data->name;
> > +		if (init_data->type)
> > +			ir_type = init_data->type;
> >  		ir->get_key = init_data->get_key;
> > +		switch (init_data->internal_get_key_func) {
> > +		case IR_KBD_GET_KEY_PIXELVIEW:
> > +			ir->get_key = get_key_pixelview;
> > +			break;
> > +		case IR_KBD_GET_KEY_PV951:
> > +			ir->get_key = get_key_pv951;
> > +			break;
> > +		case IR_KBD_GET_KEY_HAUP:
> > +			ir->get_key = get_key_haup;
> > +			break;
> > +		case IR_KBD_GET_KEY_KNC1:
> > +			ir->get_key = get_key_knc1;
> > +			break;
> > +		case IR_KBD_GET_KEY_FUSIONHDTV:
> > +			ir->get_key = get_key_fusionhdtv;
> > +			break;
> > +		case IR_KBD_GET_KEY_HAUP_XVR:
> > +			ir->get_key = get_key_haup_xvr;
> > +			break;
> > +		case IR_KBD_GET_KEY_AVERMEDIA_CARDBUS:
> > +			ir->get_key = get_key_avermedia_cardbus;
> > +			break;
> > +		default:
> > +			break;
> > +		}
> >  	}
> >  
> >  	/* Make sure we are all setup before going on */
> > diff -r d754a2d5a376 linux/include/media/ir-kbd-i2c.h
> > --- a/linux/include/media/ir-kbd-i2c.h	Wed Jul 15 07:28:02 2009 -0300
> > +++ b/linux/include/media/ir-kbd-i2c.h	Fri Jul 17 16:05:28 2009 -0400
> > @@ -24,10 +24,27 @@
> >  	int                    (*get_key)(struct IR_i2c*, u32*, u32*);
> >  };
> >  
> > +enum ir_kbd_get_key_fn {
> > +	IR_KBD_GET_KEY_NONE = 0,
> 
> As you never use IR_KBD_GET_KEY_NONE, you might as well not define it
> and start with IR_KBD_GET_KEY_PIXELVIEW = 1. This would have the added
> advantage that you could get rid of the "default" statement in the
> above switch, letting gcc warn you (or any other developer) if you ever
> add a new enum value and forget to handle it in ir_probe().

>From gcc-4.0.1 docs:

-Wswitch
        Warn whenever a switch statement has an index of enumerated type
        and lacks a case for one or more of the named codes of that
        enumeration. (The presence of a default label prevents this
        warning.) case labels outside the enumeration range also provoke
        warnings when this option is used. This warning is enabled by
        -Wall. 
        
Since a calling driver may provide a value of 0 via a memset, I'd choose
keeping the enum label of IR_KBD_GET_KEY_NONE, add a case for it in the
switch(), and remove the "default:" case.  It just seems wrong to let
drivers pass in 0 value for "internal_get_key_func" and the switch()
neither have an explicit nor a "default:" case for it.  (Maybe it's just
the years of Ada programming that beat things like this into me...)

My idea was that a driver would

a. for a driver provided function, specify a pointer to the driver's
function in "get_key" and set the "internal_get_key_func" field set to 0
(IR_KBD_GET_KEY_NONE) likely via memset().

b. for a ir-kbd-i2c provided function, specify a NULL pointer in
"get_key", and use an enumerated value in "internal_get_key_func".

If both are specified, the switch() will set to use the ir-kbd-i2c
internal function, unless an invalid enum value was used.

Regards,
Andy

> > +	IR_KBD_GET_KEY_PIXELVIEW,
> > +	IR_KBD_GET_KEY_PV951,
> > +	IR_KBD_GET_KEY_HAUP,
> > +	IR_KBD_GET_KEY_KNC1,
> > +	IR_KBD_GET_KEY_FUSIONHDTV,
> > +	IR_KBD_GET_KEY_HAUP_XVR,
> > +	IR_KBD_GET_KEY_AVERMEDIA_CARDBUS,
> > +};
> > +
> >  /* Can be passed when instantiating an ir_video i2c device */
> >  struct IR_i2c_init_data {
> >  	IR_KEYTAB_TYPE         *ir_codes;
> >  	const char             *name;
> > +	int                    type; /* IR_TYPE_RC5, IR_TYPE_PD, etc */
> > +	/*
> > +	 * Specify either a function pointer or a value indicating one of
> > +	 * ir_kbd_i2c's internal get_key functions
> > +	 */
> >  	int                    (*get_key)(struct IR_i2c*, u32*, u32*);
> > +	enum ir_kbd_get_key_fn internal_get_key_func;
> >  };
> >  #endif
> 
> 

Re: [PATCH 2/3] 2/3: cx18: Add i2c initialization for Z8F0811/Hauppage IR transceivers

[Andy Walls]

On Sun, 2009-07-19 at 15:38 +0200, Jean Delvare wrote:
> Hi Andy,

Jean,

Thanks for tking the time to answer my questions and review.  My
responses are inline.

> On Fri, 17 Jul 2009 16:46:55 -0400, Andy Walls wrote:
> > This patch add support to the cx18 driver for setting up the
> > Z8F0811/Hauppauge IR Tx/Rx chip with the i2c binding model in newer
> > kernels.
> > 
> > My concerns/questions:
> > 
> > 1. When using the new i2c binding model, I'm not saving the returned
> > pointer from i2c_new_probed_device() and am hence taking no action on
> > trying to clean up IR i2c devices on module unload.  Will the i2c
> > subsystem clean up this automatically when the adapter is unregistered
> > on module unload?
> 
> While this isn't currently documented, yes it will. If this ever
> changes, I will first let i2c-core emit warnings and still clean up
> orphan clients. But I suspect the current behavior is easier for
> everyone and I couldn't see any reason to change it at this point.

OK.  Sounds good. :)

> > 2. When using the new i2c binding model, I'm calling
> > i2c_new_probed_device() twice: once for Rx (addr 0x71 of the Z8F0811)
> > and another time for Tx (addr 0x70 of the Z8F0811).  Is it a problem to
> > have two Linux i2c devices for two distinct addresses of the same
> > underlying hardware device?
> 
> No, this is not a problem. The fact that this is the same device behind
> both addresses is an implementation detail. As long as both functions
> are independent, it should work just fine.

OK. :)


> > 3. When using the new i2c binding model, I opted not to use ir_video for
> > the Z8F0811 loaded with microcode from Zilog/Hauppauge.  Since I needed
> > one name for Rx binding and one for Tx binding, I used these names:
> > 
> >  	"ir_tx_z8f0811_haup"
> > 	"ir_rx_z8f0811_haup"
> > 
> > [Which is ir_(func)_(part number)_(firmware_oem)].  It made sense to me.
> > I assume these are the names to which ir-kbd-i2c and lirc_* will have to
> > bind.  Is that correct?
> 
> Yes, this is correct, and the approach is good.

OK. :)

>  Ideally the "ir_video"
> type would not exist (or would go away over time) and we would have a
> separate type name for each IR chip, resulting in much cleaner code.
> The reason for the current implementation is solely historical.

OK, I had suspected the reason was this.



> Review:
> 
> > diff -r d754a2d5a376 linux/drivers/media/video/cx18/cx18-cards.c
> > --- a/linux/drivers/media/video/cx18/cx18-cards.c	Wed Jul 15 07:28:02 2009 -0300
> > +++ b/linux/drivers/media/video/cx18/cx18-cards.c	Fri Jul 17 16:05:28 2009 -0400
> > @@ -56,7 +56,8 @@
> >  	.hw_audio_ctrl = CX18_HW_418_AV,
> >  	.hw_muxer = CX18_HW_CS5345,
> >  	.hw_all = CX18_HW_TVEEPROM | CX18_HW_418_AV | CX18_HW_TUNER |
> > -		  CX18_HW_CS5345 | CX18_HW_DVB | CX18_HW_GPIO_RESET_CTRL,
> > +		  CX18_HW_CS5345 | CX18_HW_DVB | CX18_HW_GPIO_RESET_CTRL |
> > +		  CX18_HW_Z8F0811_IR_HAUP,
> >  	.video_inputs = {
> >  		{ CX18_CARD_INPUT_VID_TUNER,  0, CX18_AV_COMPOSITE7 },
> >  		{ CX18_CARD_INPUT_SVIDEO1,    1, CX18_AV_SVIDEO1    },
> > @@ -102,7 +103,8 @@
> >  	.hw_audio_ctrl = CX18_HW_418_AV,
> >  	.hw_muxer = CX18_HW_CS5345,
> >  	.hw_all = CX18_HW_TVEEPROM | CX18_HW_418_AV | CX18_HW_TUNER |
> > -		  CX18_HW_CS5345 | CX18_HW_DVB | CX18_HW_GPIO_RESET_CTRL,
> > +		  CX18_HW_CS5345 | CX18_HW_DVB | CX18_HW_GPIO_RESET_CTRL |
> > +		  CX18_HW_Z8F0811_IR_HAUP,
> >  	.video_inputs = {
> >  		{ CX18_CARD_INPUT_VID_TUNER,  0, CX18_AV_COMPOSITE7 },
> >  		{ CX18_CARD_INPUT_SVIDEO1,    1, CX18_AV_SVIDEO1    },
> > diff -r d754a2d5a376 linux/drivers/media/video/cx18/cx18-cards.h
> > --- a/linux/drivers/media/video/cx18/cx18-cards.h	Wed Jul 15 07:28:02 2009 -0300
> > +++ b/linux/drivers/media/video/cx18/cx18-cards.h	Fri Jul 17 16:05:28 2009 -0400
> > @@ -22,13 +22,17 @@
> >   */
> >  
> >  /* hardware flags */
> > -#define CX18_HW_TUNER		(1 << 0)
> > -#define CX18_HW_TVEEPROM	(1 << 1)
> > -#define CX18_HW_CS5345		(1 << 2)
> > -#define CX18_HW_DVB		(1 << 3)
> > -#define CX18_HW_418_AV		(1 << 4)
> > -#define CX18_HW_GPIO_MUX	(1 << 5)
> > -#define CX18_HW_GPIO_RESET_CTRL	(1 << 6)
> > +#define CX18_HW_TUNER			(1 << 0)
> > +#define CX18_HW_TVEEPROM		(1 << 1)
> > +#define CX18_HW_CS5345			(1 << 2)
> > +#define CX18_HW_DVB			(1 << 3)
> > +#define CX18_HW_418_AV			(1 << 4)
> > +#define CX18_HW_GPIO_MUX		(1 << 5)
> > +#define CX18_HW_GPIO_RESET_CTRL		(1 << 6)
> > +#define CX18_HW_Z8F0811_IR_TX_HAUP	(1 << 7)
> > +#define CX18_HW_Z8F0811_IR_RX_HAUP	(1 << 8)
> > +#define CX18_HW_Z8F0811_IR_HAUP	(CX18_HW_Z8F0811_IR_RX_HAUP | \
> > +				 CX18_HW_Z8F0811_IR_TX_HAUP)
> >  
> >  /* video inputs */
> >  #define	CX18_CARD_INPUT_VID_TUNER	1
> > diff -r d754a2d5a376 linux/drivers/media/video/cx18/cx18-i2c.c
> > --- a/linux/drivers/media/video/cx18/cx18-i2c.c	Wed Jul 15 07:28:02 2009 -0300
> > +++ b/linux/drivers/media/video/cx18/cx18-i2c.c	Fri Jul 17 16:05:28 2009 -0400
> > @@ -40,16 +40,20 @@
> >  #define GETSDL_BIT      0x0008
> >  
> >  #define CX18_CS5345_I2C_ADDR		0x4c
> > +#define CX18_Z8F0811_IR_TX_I2C_ADDR	0x70
> > +#define CX18_Z8F0811_IR_RX_I2C_ADDR	0x71
> >  
> >  /* This array should match the CX18_HW_ defines */
> >  static const u8 hw_addrs[] = {
> > -	0,			/* CX18_HW_TUNER */
> > -	0,			/* CX18_HW_TVEEPROM */
> > -	CX18_CS5345_I2C_ADDR,	/* CX18_HW_CS5345 */
> > -	0,			/* CX18_HW_DVB */
> > -	0,			/* CX18_HW_418_AV */
> > -	0,			/* CX18_HW_GPIO_MUX */
> > -	0,			/* CX18_HW_GPIO_RESET_CTRL */
> > +	0,				/* CX18_HW_TUNER */
> > +	0,				/* CX18_HW_TVEEPROM */
> > +	CX18_CS5345_I2C_ADDR,		/* CX18_HW_CS5345 */
> > +	0,				/* CX18_HW_DVB */
> > +	0,				/* CX18_HW_418_AV */
> > +	0,				/* CX18_HW_GPIO_MUX */
> > +	0,				/* CX18_HW_GPIO_RESET_CTRL */
> > +	CX18_Z8F0811_IR_TX_I2C_ADDR,	/* CX18_HW_Z8F0811_IR_TX_HAUP */
> > +	CX18_Z8F0811_IR_RX_I2C_ADDR,	/* CX18_HW_Z8F0811_IR_RX_HAUP */
> >  };
> >  
> >  /* This array should match the CX18_HW_ defines */
> > @@ -62,6 +66,8 @@
> >  	0,	/* CX18_HW_418_AV */
> >  	0,	/* CX18_HW_GPIO_MUX */
> >  	0,	/* CX18_HW_GPIO_RESET_CTRL */
> > +	0,	/* CX18_HW_Z8F0811_IR_TX_HAUP */
> > +	0,	/* CX18_HW_Z8F0811_IR_RX_HAUP */
> >  };
> >  
> >  /* This array should match the CX18_HW_ defines */
> > @@ -73,6 +79,8 @@
> >  	NULL,		/* CX18_HW_418_AV */
> >  	NULL,		/* CX18_HW_GPIO_MUX */
> >  	NULL,		/* CX18_HW_GPIO_RESET_CTRL */
> > +	NULL,		/* CX18_HW_Z8F0811_IR_TX_HAUP */
> > +	NULL,		/* CX18_HW_Z8F0811_IR_RX_HAUP */
> >  };
> >  
> >  /* This array should match the CX18_HW_ defines */
> > @@ -84,8 +92,43 @@
> >  	"cx23418_AV",
> >  	"gpio_mux",
> >  	"gpio_reset_ctrl",
> > +	"ir_tx_z8f0811_haup",
> > +	"ir_rx_z8f0811_haup",
> >  };
> >  
> > +static int cx18_i2c_new_ir(struct i2c_adapter *adap, u32 hw, const char *type,
> > +			   u8 addr)
> > +{
> > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 30)
> > +	struct i2c_board_info info;
> > +	struct IR_i2c_init_data ir_init_data;
> > +	unsigned short addr_list[2] = { addr, I2C_CLIENT_END };
> > +
> > +	memset(&info, 0, sizeof(struct i2c_board_info));
> > +	strlcpy(info.type, type, I2C_NAME_SIZE);
> > +
> > +	/* Our default information for ir-kbd-i2c.c to use */
> > +	memset(&ir_init_data, 0, sizeof(struct IR_i2c_init_data));
> > +	switch (hw) {
> > +	case CX18_HW_Z8F0811_IR_RX_HAUP:
> > +		ir_init_data.ir_codes = ir_codes_hauppauge_new;
> > +		ir_init_data.get_key = NULL;
> 
> This is a no-op, as ir_init_data was cleared with memset() right above.

Agree.  I'll get rid of it.

> 
> > +		ir_init_data.internal_get_key_func = IR_KBD_GET_KEY_HAUP_XVR;
> > +		ir_init_data.type = IR_TYPE_RC5;
> > +		ir_init_data.name = "CX23418 Z8F0811 Hauppauge";
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +	if (ir_init_data.name)
> > +		info.platform_data = &ir_init_data;
> 
> Not sure why you don't just put this in the switch to save a test? Even
> the memset could go there as far as I can see.


If this were the only I2C IR Rx device I had plans for in the cx18
driver, I would have used an if() statement for the whole thing instead
of a switch().  However, the LeadTek boards have an IR device that I
plan to support someday, so I left the common elements split out.

The biggest penalty here is probably the memset() for the IR Tx portion
of the Z8F0811 chip that doesn't use the ir_init_data, so I'll make the
change as you suggest.



> > +
> > +	return i2c_new_probed_device(adap, &info, addr_list) == NULL ? -1 : 0;
> > +#else
> > +	return -1;
> > +#endif
> > +}
> > +
> >  int cx18_i2c_register(struct cx18 *cx, unsigned idx)
> >  {
> >  	struct v4l2_subdev *sd;
> > @@ -119,7 +162,10 @@
> >  	if (!hw_addrs[idx])
> >  		return -1;
> >  
> > -	/* It's an I2C device other than an analog tuner */
> > +	if (hw & CX18_HW_Z8F0811_IR_HAUP)
> > +		return cx18_i2c_new_ir(adap, hw, type, hw_addrs[idx]);
> 
> For consistency I'd move this up a bit (although I agree it is
> functionally equivalent.)

No problem.


> > +
> > +	/* It's an I2C device other than an analog tuner or IR chip */
> >  	sd = v4l2_i2c_new_subdev(&cx->v4l2_dev, adap, mod, type, hw_addrs[idx]);
> >  	if (sd != NULL)
> >  		sd->grp_id = hw;
> > 
> > 
> 
> The rest looks OK.

OK.  Thanks.

-Andy

Re: [PATCH 1/3] ir-kbd-i2c: Allow use of ir-kdb-i2c internal get_key funcs and set ir_type

[Jean Delvare]

Hi Andy,

On Mon, 20 Jul 2009 20:07:50 -0400, Andy Walls wrote:
> On Sun, 2009-07-19 at 14:47 +0200, Jean Delvare wrote:
> > Hi Andy,
> > 
> > On Fri, 17 Jul 2009 16:35:37 -0400, Andy Walls wrote:
> > > This patch augments the init data passed by bridge drivers to ir-kbd-i2c
> > > so that the ir_type can be set explicitly and so ir-kbd-i2c internal
> > > get_key functions can be reused without requiring symbols from
> > > ir-kbd-i2c in the bridge driver.
> > > 
> > > 
> > > Regards,
> > > Andy
> > 
> > Looks good. Minor suggestion below:
> 
> Jean,
> 
> Thanks for the reply.  My responses are inline.
> 
> > > 
> > > diff -r d754a2d5a376 linux/drivers/media/video/ir-kbd-i2c.c
> > > --- a/linux/drivers/media/video/ir-kbd-i2c.c	Wed Jul 15 07:28:02 2009 -0300
> > > +++ b/linux/drivers/media/video/ir-kbd-i2c.c	Fri Jul 17 16:05:28 2009 -0400
> > > @@ -478,7 +480,34 @@
> > >  
> > >  		ir_codes = init_data->ir_codes;
> > >  		name = init_data->name;
> > > +		if (init_data->type)
> > > +			ir_type = init_data->type;
> > >  		ir->get_key = init_data->get_key;
> > > +		switch (init_data->internal_get_key_func) {
> > > +		case IR_KBD_GET_KEY_PIXELVIEW:
> > > +			ir->get_key = get_key_pixelview;
> > > +			break;
> > > +		case IR_KBD_GET_KEY_PV951:
> > > +			ir->get_key = get_key_pv951;
> > > +			break;
> > > +		case IR_KBD_GET_KEY_HAUP:
> > > +			ir->get_key = get_key_haup;
> > > +			break;
> > > +		case IR_KBD_GET_KEY_KNC1:
> > > +			ir->get_key = get_key_knc1;
> > > +			break;
> > > +		case IR_KBD_GET_KEY_FUSIONHDTV:
> > > +			ir->get_key = get_key_fusionhdtv;
> > > +			break;
> > > +		case IR_KBD_GET_KEY_HAUP_XVR:
> > > +			ir->get_key = get_key_haup_xvr;
> > > +			break;
> > > +		case IR_KBD_GET_KEY_AVERMEDIA_CARDBUS:
> > > +			ir->get_key = get_key_avermedia_cardbus;
> > > +			break;
> > > +		default:
> > > +			break;
> > > +		}
> > >  	}
> > >  
> > >  	/* Make sure we are all setup before going on */
> > > diff -r d754a2d5a376 linux/include/media/ir-kbd-i2c.h
> > > --- a/linux/include/media/ir-kbd-i2c.h	Wed Jul 15 07:28:02 2009 -0300
> > > +++ b/linux/include/media/ir-kbd-i2c.h	Fri Jul 17 16:05:28 2009 -0400
> > > @@ -24,10 +24,27 @@
> > >  	int                    (*get_key)(struct IR_i2c*, u32*, u32*);
> > >  };
> > >  
> > > +enum ir_kbd_get_key_fn {
> > > +	IR_KBD_GET_KEY_NONE = 0,
> > 
> > As you never use IR_KBD_GET_KEY_NONE, you might as well not define it
> > and start with IR_KBD_GET_KEY_PIXELVIEW = 1. This would have the added
> > advantage that you could get rid of the "default" statement in the
> > above switch, letting gcc warn you (or any other developer) if you ever
> > add a new enum value and forget to handle it in ir_probe().
> 
> >From gcc-4.0.1 docs:
> 
> -Wswitch
>         Warn whenever a switch statement has an index of enumerated type
>         and lacks a case for one or more of the named codes of that
>         enumeration. (The presence of a default label prevents this
>         warning.) case labels outside the enumeration range also provoke
>         warnings when this option is used. This warning is enabled by
>         -Wall. 
>         
> Since a calling driver may provide a value of 0 via a memset, I'd choose
> keeping the enum label of IR_KBD_GET_KEY_NONE, add a case for it in the
> switch(), and remove the "default:" case.

Yes, that's fine with me too. You might want to rename
IR_KBD_GET_KEY_NONE to IR_KBD_GET_KEY_CUSTOM then, and move
	ir->get_key = init_data->get_key;
inside the switch.

>  It just seems wrong to let
> drivers pass in 0 value for "internal_get_key_func" and the switch()
> neither have an explicit nor a "default:" case for it.  (Maybe it's just
> the years of Ada programming that beat things like this into me...)
> 
> My idea was that a driver would
> 
> a. for a driver provided function, specify a pointer to the driver's
> function in "get_key" and set the "internal_get_key_func" field set to 0
> (IR_KBD_GET_KEY_NONE) likely via memset().
> 
> b. for a ir-kbd-i2c provided function, specify a NULL pointer in
> "get_key", and use an enumerated value in "internal_get_key_func".
> 
> If both are specified, the switch() will set to use the ir-kbd-i2c
> internal function, unless an invalid enum value was used.

My key point was that the default case would prevent gcc from helping
you. Any solution without the default case is thus fine with me.

-- 
Jean Delvare