ir-kbd-i2c conversion to the new i2c binding model

* Return actual error values as returned by the i2c subsystem, rather than 0 or 1.
* If the registration of the second bus fails, unregister the first one before exiting, otherwise we are leaking resources.

Signed-off-by Jean Delvare <[email protected]>

Cc Hans Verkuil <[email protected]>

Cc Andy Walls <[email protected]>

---
 linux/drivers/media/video/cx18/cx18-i2c.c |   16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

linux/drivers/media/video/cx18/cx18-i2c.c +1 -1

@@ -214,7 +214,7 @@ static struct i2c_algo_bit_data cx18_i2c
 /* init + register i2c algo-bit adapter */
 int init_cx18_i2c(struct cx18 *cx)
 {
-	int i;
+	int i, err;
 	CX18_DEBUG_I2C("i2c init\n");
 
 	for (i = 0; i < 2; i++) {

linux/drivers/media/video/cx18/cx18-i2c.c +4 -0

+	if (err)
+		goto err;
+	err = i2c_bit_add_bus(&cx->i2c_adap[1]);
+	if (err)

Quoted 8 lines · Jean Delvare

* Return actual error values as returned by the i2c subsystem, rather than 0 or 1.
* If the registration of the second bus fails, unregister the first one before exiting, otherwise we are leaking resources.

Signed-off-by Jean Delvare <[email protected]>

Cc Hans Verkuil <[email protected]>

Cc Andy Walls <[email protected]>

Jean,

Thanks for noticing this one and providing a patch. I have one comment below...

Quoted 16 lines

---
 linux/drivers/media/video/cx18/cx18-i2c.c |   16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

linux/drivers/media/video/cx18/cx18-i2c.c +1 -1

@@ -214,7 +214,7 @@ static struct i2c_algo_bit_data cx18_i2c
 /* init + register i2c algo-bit adapter */
 int init_cx18_i2c(struct cx18 *cx)
 {
-	int i;
+	int i, err;
 	CX18_DEBUG_I2C("i2c init\n");
 
 	for (i = 0; i < 2; i++) {

linux/drivers/media/video/cx18/cx18-i2c.c int init_cx18_i2c(struct cx18 *cx) · line 276

Quoted 6 lines

linux/drivers/media/video/cx18/cx18-i2c.c +1 -2

@@ -273,8 +273,18 @@ int init_cx18_i2c(struct cx18 *cx)
 	cx18_call_hw(cx, CX18_HW_GPIO_RESET_CTRL,
 		     core, reset, (u32) CX18_GPIO_RESET_I2C);
 
-	return i2c_bit_add_bus(&cx->i2c_adap[0]) ||
-		i2c_bit_add_bus(&cx->i2c_adap[1]);
+	err = i2c_bit_add_bus(&cx->i2c_adap[0]);

	if (err)
		return err;
	err = i2c_bit_add_bus(&cx->i2c_adap[1]);
	if (err)
		i2c_del_adapter(&cx->i2c_adap[0]);
	return err;

This sequence saves a few lines of code and gets rid of the goto's compared to what you proposed below.

Quoted 13 lines

linux/drivers/media/video/cx18/cx18-i2c.c +11 -0

+	if (err)
+		goto err;
+	err = i2c_bit_add_bus(&cx->i2c_adap[1]);
+	if (err)
+		goto err_del_bus_0;
+	return 0;
+
+ err_del_bus_0:
+ 	i2c_del_adapter(&cx->i2c_adap[0]);
+ err:
+	return err;
 }
 

linux/drivers/media/video/cx18/cx18-i2c.c int init_cx18_i2c(struct cx18 *cx) · line 290

Quoted 1 line

linux/drivers/media/video/cx18/cx18-i2c.c +0 -0

 void exit_cx18_i2c(struct cx18 *cx)

Reviewed-by Andy Walls <[email protected]>

Hi Andy,

Thanks for the fast review.

On Sat, 04 Apr 2009 08:46:00 -0400, Andy Walls wrote:

Correct, actually my initial attempt looked like this. But then patch 3/6 adds code, which makes "your" solution 2 lines bigger, while "my" solution stays as is, so the difference between both becomes very thin.

Some developers (including me) prefer to have a single error path, others hate gotos more than (potential) code duplication. I didn't know what you'd prefer as the driver maintainer. If you want me to use the variant without gotos, I can do that, no problem.

Quoted 43 lines · Jean Delvare

* Return actual error values as returned by the i2c subsystem, rather than 0 or 1.
* If the registration of the second bus fails, unregister the first one before exiting, otherwise we are leaking resources.

Signed-off-by Jean Delvare <[email protected]>

Cc Hans Verkuil <[email protected]>

Cc Andy Walls <[email protected]>

---
 linux/drivers/media/video/cx18/cx18-i2c.c |   16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

linux/drivers/media/video/cx18/cx18-i2c.c +13 -3

@@ -214,7 +214,7 @@ static struct i2c_algo_bit_data cx18_i2c
 /* init + register i2c algo-bit adapter */
 int init_cx18_i2c(struct cx18 *cx)
 {
-	int i;
+	int i, err;
 	CX18_DEBUG_I2C("i2c init\n");
 
 	for (i = 0; i < 2; i++) {

@@ -273,8 +273,18 @@ int init_cx18_i2c(struct cx18 *cx)
 	cx18_call_hw(cx, CX18_HW_GPIO_RESET_CTRL,
 		     core, reset, (u32) CX18_GPIO_RESET_I2C);
 
-	return i2c_bit_add_bus(&cx->i2c_adap[0]) ||
-		i2c_bit_add_bus(&cx->i2c_adap[1]);
+	err = i2c_bit_add_bus(&cx->i2c_adap[0]);
+	if (err)
+		goto err;
+	err = i2c_bit_add_bus(&cx->i2c_adap[1]);
+	if (err)
+		goto err_del_bus_0;
+	return 0;
+
+ err_del_bus_0:
+ 	i2c_del_adapter(&cx->i2c_adap[0]);
+ err:
+	return err;
 }
 

linux/drivers/media/video/cx18/cx18-i2c.c int init_cx18_i2c(struct cx18 *cx) · line 290

Quoted 1 line

linux/drivers/media/video/cx18/cx18-i2c.c +0 -0

 void exit_cx18_i2c(struct cx18 *cx)

Reviewed-by Andy Walls <[email protected]>

Quoted 44 lines · Andy Walls

Quoted 43 lines · Jean Delvare

* Return actual error values as returned by the i2c subsystem, rather than 0 or 1.
* If the registration of the second bus fails, unregister the first one before exiting, otherwise we are leaking resources.

Signed-off-by Jean Delvare <[email protected]>

Cc Hans Verkuil <[email protected]>

Cc Andy Walls <[email protected]>

---
 linux/drivers/media/video/cx18/cx18-i2c.c |   16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

linux/drivers/media/video/cx18/cx18-i2c.c +13 -3

@@ -214,7 +214,7 @@ static struct i2c_algo_bit_data cx18_i2c
 /* init + register i2c algo-bit adapter */
 int init_cx18_i2c(struct cx18 *cx)
 {
-	int i;
+	int i, err;
 	CX18_DEBUG_I2C("i2c init\n");
 
 	for (i = 0; i < 2; i++) {

@@ -273,8 +273,18 @@ int init_cx18_i2c(struct cx18 *cx)
 	cx18_call_hw(cx, CX18_HW_GPIO_RESET_CTRL,
 		     core, reset, (u32) CX18_GPIO_RESET_I2C);
 
-	return i2c_bit_add_bus(&cx->i2c_adap[0]) ||
-		i2c_bit_add_bus(&cx->i2c_adap[1]);
+	err = i2c_bit_add_bus(&cx->i2c_adap[0]);
+	if (err)
+		goto err;
+	err = i2c_bit_add_bus(&cx->i2c_adap[1]);
+	if (err)
+		goto err_del_bus_0;
+	return 0;
+
+ err_del_bus_0:
+ 	i2c_del_adapter(&cx->i2c_adap[0]);
+ err:
+	return err;
 }
 

Quoted 3 lines

linux/drivers/media/video/cx18/cx18-i2c.c int init_cx18_i2c(struct cx18 *cx) · line 290

Quoted 1 line

linux/drivers/media/video/cx18/cx18-i2c.c +0 -0

 void exit_cx18_i2c(struct cx18 *cx)

Reviewed-by Andy Walls <[email protected]>

[Context edited for clarity.]

Mauro, can you please apply this patch now? It is a bug fix not directly related to my ir-kbd-i2c work, it would be nice to have it applied quickly so that I don't have to carry the patch around.

Thanks,

Let card drivers probe for IR receiver devices and instantiate them if found. Ultimately it would be better if we could stop probing completely, but I suspect this won't be possible for all card types.

There's certainly room for cleanups. For example, some drivers are sharing I2C adapter IDs, so they also had to share the list of I2C addresses being probed for an IR receiver. Now that each driver explicitly says which addresses should be probed, maybe some addresses can be dropped from some drivers.

Also, the special cases in saa7134-i2c should probably be handled on a per-board basis. This would be more efficient and less risky than always probing extra addresses on all boards. I'll give it a try later.

Signed-off-by Jean Delvare <[email protected]>

Cc Mauro Carvalho Chehab <[email protected]>

Cc Hans Verkuil <[email protected]>

Cc Andy Walls <[email protected]>

Cc Mike Isely <[email protected]>

---
 linux/drivers/media/video/cx18/cx18-i2c.c            |   30 ++
 linux/drivers/media/video/ivtv/ivtv-i2c.c            |   31 ++
 linux/include/media/ir-kbd-i2c.h                     |    2 
 17 files changed, 284 insertions(+), 196 deletions(-)

linux/drivers/media/video/cx18/cx18-i2c.c +5 -1

@@ -211,7 +211,32 @@ static struct i2c_algo_bit_data cx18_i2c
 	.timeout	= CX18_ALGO_BIT_TIMEOUT*HZ /* jiffies */
 };
 
-/* init + register i2c algo-bit adapter */
+static void init_cx18_i2c_ir(struct cx18 *cx)
+{
+	struct i2c_board_info info;
+	/* The external IR receiver is at i2c address 0x34 (0x35 for
+	   reads).  Future Hauppauge cards will have an internal

linux/drivers/media/video/cx18/cx18-i2c.c +10 -0

+	memset(&info, 0, sizeof(struct i2c_board_info));
+	strlcpy(info.type, "ir-kbd", I2C_NAME_SIZE);
+
+	/* The IR receiver device can be on either I2C bus */
+	if (i2c_new_probed_device(&cx->i2c_adap[0], &info, addr_list))
+		return;
+	i2c_new_probed_device(&cx->i2c_adap[1], &info, addr_list);
+}
+
+/* init + register i2c adapters + instantiate IR receiver */
 int init_cx18_i2c(struct cx18 *cx)
 {
 	int i, err;

I have an I2C related question. If the cx18 or ivtv driver autoloads "ir-kbd-i2c" and registers an I2C client on the bus, does that preclude lirc_i2c, lirc_pvr150 or lirc_zilog from using the device? LIRC users may notice, if it does.

If that is the case, then we probably shouldn't autoload the ir-kbd module after the CX23418 i2c adapters are initialized.

I'm not sure what's the best solution:

1. A module option to the cx18 driver to tell it to call init_cx18_i2c_ir() from cx18_probe() or not? (Easiest solution)

2. Some involved programmatic way for IR device modules to query bridge drivers about what IR devices they may have, and on which I2C bus, and at what addresses to probe, and whether a driver/module has already claimed that device? (Gold plated solution)

Regards,
Andy

On Sat, 4 Apr 2009, Andy Walls wrote:

   [...]

Quoted 18 lines

I have an I2C related question. If the cx18 or ivtv driver autoloads "ir-kbd-i2c" and registers an I2C client on the bus, does that preclude lirc_i2c, lirc_pvr150 or lirc_zilog from using the device? LIRC users may notice, if it does.

If that is the case, then we probably shouldn't autoload the ir-kbd module after the CX23418 i2c adapters are initialized.

I'm not sure what's the best solution:

1. A module option to the cx18 driver to tell it to call init_cx18_i2c_ir() from cx18_probe() or not? (Easiest solution)

2. Some involved programmatic way for IR device modules to query bridge drivers about what IR devices they may have, and on which I2C bus, and at what addresses to probe, and whether a driver/module has already claimed that device? (Gold plated solution)

Quoted 2 lines

Regards,
Andy

Ah, glad to see I'm not the only one concerned about this.

I suppose I could instead add a module option to the pvrusb2 driver to control autoloading of ir-kbd (option 1). It also should probably be a per-device attribute, since AFAIK, ir-kbd only even works when using older Hauppauge IR receivers (i.e. lirc_i2c - cases that would otherwise use lirc_pvr150 or lirc_zilog I believe do not work with ir-kbd). Some devices handled by the pvrusb2 driver are not from Hauppauge. Too bad if this is the case, it was easier to let the user decide just by choosing which actual module to load.

-Mike

On Sat, 2009-04-04 at 11:05 -0500, Mike Isely wrote:

So my rough outline of an idea (which probably runs slightly afoul of Hans' media_controller device, but we don't have it yet):

1. Add a function to the v4l2 framework to iterate over all the v4l2_device's that are registered (if there isn't one already).

2. Add a method to the v4l2_device class to return the IR subdevice for a given v4l2_device:

	v4l2_subdev *v4l2_device_get_ir_subdev(v4l2_device *dev);

and if it returns NULL, that device has no IR chip.

3. To the v4l2_subdev framework add:

	struct v4l2_subdev_ir_ops {
		(*enumerate) (v4l2_subdev *sd, /* bus_type, bus #, addr for Rx, addr for Tx */);
		(*claim) (v4l2_subdev *sd, /* claiming driver name string, going-away callback function pointer */);
		(*release) (v4l2_subdev *sd, /* handle */);
		bool (*is_claimed) (v4l2_subdev *sd, /* output string of the "owner" */);
		/* Or maybe just */
		(*send) (v4l2_subdev *sd, /* data buffer */);
		(*receive) (v4l2_subdev *sd, /* data buffer */);
	}

and have the bridge driver support these. (I also had some in mind for the IR micro-controller debug/programming port, but the above will fit the task at hand I think.)

OK so that's all a bit rough around the edges. The idea is a uniform call in for ir-kdb-i2c or lirc_foo or ir_foo to get at an IR chip behind a bridge device, that the bridge device driver itself cares about very little. *Except* ir driver modules would be coordinated by the bridge driver in what they can and cannot do to get at the IR device. This coordination prevents bad things on the bridge chip's I2C bus(es) or from having modules racing to get the IR device. That way whatever module the user loads will get first shot at claiming the IR chip. This also provides a discovery mechanism four use by ir driver modules that is informed by the bridge chip driver. I think lirc_foo can also still use it's current way of doing business too.

It really just looks like a small subset of what Hans intended for the media controller, so maybe this would be a good chance to get some "lessons learned."

Quoted 46 lines · Jean Delvare

Let card drivers probe for IR receiver devices and instantiate them if found. Ultimately it would be better if we could stop probing completely, but I suspect this won't be possible for all card types.

There's certainly room for cleanups. For example, some drivers are sharing I2C adapter IDs, so they also had to share the list of I2C addresses being probed for an IR receiver. Now that each driver explicitly says which addresses should be probed, maybe some addresses can be dropped from some drivers.

Also, the special cases in saa7134-i2c should probably be handled on a per-board basis. This would be more efficient and less risky than always probing extra addresses on all boards. I'll give it a try later.

Signed-off-by Jean Delvare <[email protected]>

Cc Mauro Carvalho Chehab <[email protected]>

Cc Hans Verkuil <[email protected]>

Cc Andy Walls <[email protected]>

Cc Mike Isely <[email protected]>

---
 linux/drivers/media/video/cx18/cx18-i2c.c            |   30 ++
 linux/drivers/media/video/ivtv/ivtv-i2c.c            |   31 ++
 linux/include/media/ir-kbd-i2c.h                     |    2 
 17 files changed, 284 insertions(+), 196 deletions(-)

linux/drivers/media/video/cx18/cx18-i2c.c +13 -1

@@ -211,7 +211,32 @@ static struct i2c_algo_bit_data cx18_i2c
 	.timeout	= CX18_ALGO_BIT_TIMEOUT*HZ /* jiffies */
 };
 
-/* init + register i2c algo-bit adapter */
+static void init_cx18_i2c_ir(struct cx18 *cx)
+{
+	struct i2c_board_info info;
+	/* The external IR receiver is at i2c address 0x34 (0x35 for
+	   reads).  Future Hauppauge cards will have an internal
+	   receiver at 0x30 (0x31 for reads).  In theory, both can be
+	   fitted, and Hauppauge suggest an external overrides an
+	   internal.
+
+	   That's why we probe 0x1a (~0x34) first. CB
+	*/
+	const unsigned short addr_list[] = {
+		0x1a, 0x18, 0x64, 0x30,

linux/drivers/media/video/cx18/cx18-i2c.c static struct i2c_algo_bit_data cx18_i2c · line 228

Quoted 2 lines

linux/drivers/media/video/cx18/cx18-i2c.c +2 -0

+		I2C_CLIENT_END
+	};

I think this is way out of date for cx18 based boards. The only IR chip I know of so far is the Zilog Z8F0811 sitting at 7 bit addresses 0x70-0x74. I guess 0x71 is the proper address for Rx. I'll let you know when I test.

Quoted 18 lines

linux/drivers/media/video/cx18/cx18-i2c.c +11 -0

+	memset(&info, 0, sizeof(struct i2c_board_info));
+	strlcpy(info.type, "ir-kbd", I2C_NAME_SIZE);
+
+	/* The IR receiver device can be on either I2C bus */
+	if (i2c_new_probed_device(&cx->i2c_adap[0], &info, addr_list))
+		return;
+	i2c_new_probed_device(&cx->i2c_adap[1], &info, addr_list);
+}
+
+/* init + register i2c adapters + instantiate IR receiver */
 int init_cx18_i2c(struct cx18 *cx)
 {
 	int i, err;

@@ -279,6 +304,9 @@ int init_cx18_i2c(struct cx18 *cx)
 	err = i2c_bit_add_bus(&cx->i2c_adap[1]);
 	if (err)
 		goto err_del_bus_0;
+

linux/drivers/media/video/cx18/cx18-i2c.c int init_cx18_i2c(struct cx18 *cx) · line 310

Quoted 3 lines

linux/drivers/media/video/cx18/cx18-i2c.c +2 -0

+	/* Instantiate the IR receiver device, if present */
+	init_cx18_i2c_ir(cx);
 	return 0;

I have an I2C related question. If the cx18 or ivtv driver autoloads "ir-kbd-i2c" and registers an I2C client on the bus, does that preclude lirc_i2c, lirc_pvr150 or lirc_zilog from using the device? LIRC users may notice, if it does.

If that is the case, then we probably shouldn't autoload the ir-kbd module after the CX23418 i2c adapters are initialized.

I'm not sure what's the best solution:

1. A module option to the cx18 driver to tell it to call init_cx18_i2c_ir() from cx18_probe() or not? (Easiest solution)

2. Some involved programmatic way for IR device modules to query bridge drivers about what IR devices they may have, and on which I2C bus, and at what addresses to probe, and whether a driver/module has already claimed that device? (Gold plated solution)

Hi Andy,

Quoted 47 lines · Andy Walls

Quoted 46 lines · Jean Delvare

Let card drivers probe for IR receiver devices and instantiate them if found. Ultimately it would be better if we could stop probing completely, but I suspect this won't be possible for all card types.

There's certainly room for cleanups. For example, some drivers are sharing I2C adapter IDs, so they also had to share the list of I2C addresses being probed for an IR receiver. Now that each driver explicitly says which addresses should be probed, maybe some addresses can be dropped from some drivers.

Also, the special cases in saa7134-i2c should probably be handled on a per-board basis. This would be more efficient and less risky than always probing extra addresses on all boards. I'll give it a try later.

Signed-off-by Jean Delvare <[email protected]>

Cc Mauro Carvalho Chehab <[email protected]>

Cc Hans Verkuil <[email protected]>

Cc Andy Walls <[email protected]>

Cc Mike Isely <[email protected]>

---
 linux/drivers/media/video/cx18/cx18-i2c.c            |   30 ++
 linux/drivers/media/video/ivtv/ivtv-i2c.c            |   31 ++
 linux/include/media/ir-kbd-i2c.h                     |    2 
 17 files changed, 284 insertions(+), 196 deletions(-)

linux/drivers/media/video/cx18/cx18-i2c.c +13 -1

@@ -211,7 +211,32 @@ static struct i2c_algo_bit_data cx18_i2c
 	.timeout	= CX18_ALGO_BIT_TIMEOUT*HZ /* jiffies */
 };
 
-/* init + register i2c algo-bit adapter */
+static void init_cx18_i2c_ir(struct cx18 *cx)
+{
+	struct i2c_board_info info;
+	/* The external IR receiver is at i2c address 0x34 (0x35 for
+	   reads).  Future Hauppauge cards will have an internal
+	   receiver at 0x30 (0x31 for reads).  In theory, both can be
+	   fitted, and Hauppauge suggest an external overrides an
+	   internal.
+
+	   That's why we probe 0x1a (~0x34) first. CB
+	*/
+	const unsigned short addr_list[] = {
+		0x1a, 0x18, 0x64, 0x30,

Quoted 8 lines

linux/drivers/media/video/cx18/cx18-i2c.c static struct i2c_algo_bit_data cx18_i2c · line 228

Quoted 2 lines

linux/drivers/media/video/cx18/cx18-i2c.c +2 -0

+		I2C_CLIENT_END
+	};

I think this is way out of date for cx18 based boards. The only IR chip I know of so far is the Zilog Z8F0811 sitting at 7 bit addresses 0x70-0x74. I guess 0x71 is the proper address for Rx. I'll let you know when I test.

This address list comes from the ir-kbd-i2c driver. The cx18 driver happens to use the same I2C adapter ID as the ivtv driver (I2C_HW_B_CX2341X) and this is what the ir-kbd-i2c driver used to decide which addresses to probe. As I don't know anything about the hardware, I had to keep the new code compatible with the old one and keep probing the same addresses.

Now, if you tell me that this list doesn't make sense for cx18 boards, we can change it.

As addresses 0x70-0x74 were not probed so far on cx18
boards, I guess that IR support never worked for cx18 (at least not with ir-kbd-i2c)?

Does ir-kbd-i2c support the Zilog Z8F0811 at all?

If IR on the cx18 is not supported (by the ir-kbd-i2c driver) then I can simplify my patch set and omit the cx18 entirely.

Quoted 18 lines

Quoted 18 lines

linux/drivers/media/video/cx18/cx18-i2c.c +11 -0

+	memset(&info, 0, sizeof(struct i2c_board_info));
+	strlcpy(info.type, "ir-kbd", I2C_NAME_SIZE);
+
+	/* The IR receiver device can be on either I2C bus */
+	if (i2c_new_probed_device(&cx->i2c_adap[0], &info, addr_list))
+		return;
+	i2c_new_probed_device(&cx->i2c_adap[1], &info, addr_list);
+}
+
+/* init + register i2c adapters + instantiate IR receiver */
 int init_cx18_i2c(struct cx18 *cx)
 {
 	int i, err;

@@ -279,6 +304,9 @@ int init_cx18_i2c(struct cx18 *cx)
 	err = i2c_bit_add_bus(&cx->i2c_adap[1]);
 	if (err)
 		goto err_del_bus_0;
+

Quoted 8 lines

linux/drivers/media/video/cx18/cx18-i2c.c int init_cx18_i2c(struct cx18 *cx) · line 310

Quoted 3 lines

linux/drivers/media/video/cx18/cx18-i2c.c +2 -0

+	/* Instantiate the IR receiver device, if present */
+	init_cx18_i2c_ir(cx);
 	return 0;

I have an I2C related question. If the cx18 or ivtv driver autoloads "ir-kbd-i2c" and registers an I2C client on the bus, does that preclude lirc_i2c, lirc_pvr150 or lirc_zilog from using the device? LIRC users may notice, if it does.

I don't know anything about lirc_i2c, lirc_pvr150 or lirc_zilog. I tend to ignore all the code that is neither in the Linux kernel tree nor in the v4l-dvb tree.

If you want me to answer this question, you'll have
to tell me what exactly these drivers are doing as far as I2C is concerned. Do they instantiate I2C clients? Or do they do raw I2C transfers? Do they check for address business before they do? On what basis do they attach to I2C devices?

Please note that my conversion doesn't actually change anything to the autoloading of the ir-kbd-i2c driver. The bridge drivers which were loading ir-kbd-i2c (saa7134, cx23885, em28xx and cx88) still are. Those which were not, still aren't. The ir-kbd-i2c driver doesn't include a MODULE_ALIAS call as most I2C drivers do, to prevent udev from loading this driver automatically.

What my conversion changes is that an "ir-kbd" I2C device may be instantiated if a probe is successful. This will make the address in question show as busy (except to i2c-dev which only considers an address as busy when a driver is bound to the device.) But that's about it.

Quoted 5 lines

If that is the case, then we probably shouldn't autoload the ir-kbd module after the CX23418 i2c adapters are initialized.

I'm not sure what's the best solution:

Quoted 2 lines

1. A module option to the cx18 driver to tell it to call init_cx18_i2c_ir() from cx18_probe() or not? (Easiest solution)

Sounds perfectly sensible. I seem to remember that Hans Verkuil told me he wanted something like this for ivtv. As a matter of fact, the saa7134, em28xx and cx231xx already have such a module parameter (disable_ir). Implementing the same for bttv, cx88, cx18, ivtv or any other driver should be fairly trivial.

Quoted 4 lines

2. Some involved programmatic way for IR device modules to query bridge drivers about what IR devices they may have, and on which I2C bus, and at what addresses to probe, and whether a driver/module has already claimed that device? (Gold plated solution)

I'd rather name this the over-engineered solution. It's really looking at the situation by the wrong end (that is, with the legacy i2c binding model still in mind.) Bridge drivers know which IR receivers can be present and at which address, it is up to them to instantiate the appropriate I2C devices on the bus, possibly with platform data to help the I2C drivers (be they ir-kbd-i2c, lirc or whatever.) This is exactly what my code does.

The fact that the same IR chip can be handled by 2 or more I2C drivers is a bad idea to start with. Why the hell did we do that in the first place?

If you want a clean solution to the problem, it clearly starts with getting rid of this mess and having each IR receiver chip on I2C supported by exactly one I2C driver and make sure the driver in question is in the Linux kernel tree. Spending time on any other "clean solution" is wasting time IMHO.

Still, note that it is totally possible to have several I2C drivers support the same device. The new model supports this, just like the old model did. I2C devices are instantiated by bridge drivers, which give them a _name_. Several I2C drivers are allowed to support that chip name, and the first one loaded will get to bind to the device. The "ir-i2c" devices created by cx18, ivtv etc. can be requested by other drivers than ir-kbd-i2c if you want to do that.

This will require some
changes to lirc_i2c and friends, but at this point changes to these are very needed anyway.

I hope I managed to clarify the situation a bit.

Quoted 6 lines · Jean Delvare

Quoted 4 lines · Andy Walls

I think this is way out of date for cx18 based boards. The only IR chip I know of so far is the Zilog Z8F0811 sitting at 7 bit addresses 0x70-0x74. I guess 0x71 is the proper address for Rx. I'll let you know when I test.

Quoted 6 lines

This address list comes from the ir-kbd-i2c driver. The cx18 driver happens to use the same I2C adapter ID as the ivtv driver (I2C_HW_B_CX2341X) and this is what the ir-kbd-i2c driver used to decide which addresses to probe. As I don't know anything about the hardware, I had to keep the new code compatible with the old one and keep probing the same addresses.

This is the i2cdetect output from my HVR-1600 - the only cx18 based card known or reported to have an IR chip:

[root@morgan ~]# i2cdetect -l

i2c-0	smbus     	SMBus PIIX4 adapter at 0b00     	SMBus adapter
i2c-1	i2c       	ivtv i2c driver #0              	I2C adapter
i2c-2	i2c       	cx18 i2c driver #0-0            	I2C adapter
i2c-3	i2c       	cx18 i2c driver #0-1            	I2C adapter

[root@morgan ~]# i2cdetect 2
WARNING! This program can confuse your I2C bus, cause data loss and worse! I will probe file /dev/i2c-2.
I will probe address range 0x03-0x77.
Continue? [Y/n] y
0 1 2 3 4 5 6 7 8 9 a b c d e f
00: -- -- -- -- -- -- -- -- -- -- -- -- --
10: -- -- -- -- -- -- -- -- -- 19 -- -- -- -- -- --
20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
40: -- -- -- -- -- -- -- -- -- -- -- -- UU -- -- --
50: 50 -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
60: -- -- -- 63 -- -- -- -- -- -- -- -- -- -- -- --
70: 70 71 72 73 -- -- -- --
[root@morgan ~]# i2cdetect 3
WARNING! This program can confuse your I2C bus, cause data loss and worse! I will probe file /dev/i2c-3.
I will probe address range 0x03-0x77.
Continue? [Y/n] y
0 1 2 3 4 5 6 7 8 9 a b c d e f
00: -- -- -- -- -- -- -- -- -- -- -- -- --
10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
60: -- UU -- -- -- -- -- -- -- -- -- -- -- -- -- --
70: -- -- -- -- -- -- -- --

The Zilog is at 0x70-0x73. The standard IR Tx and RX addresses are 0x70 and 0x71

Quoted 2 lines

Now, if you tell me that this list doesn't make sense for cx18 boards, we can change it.

I owe you the right address to probe. I think it is 0x71, but I want to double check.

Quoted 3 lines

As addresses 0x70-0x74 were not probed so far on cx18
boards, I guess that IR support never worked for cx18 (at least not with ir-kbd-i2c)?

No, the lirc_i2c, lirc_pvr150, and lirc_zilog come in via the i2c subsystem.

Quoted 4 lines

Does ir-kbd-i2c support the Zilog Z8F0811 at all?

If IR on the cx18 is not supported (by the ir-kbd-i2c driver) then I can simplify my patch set and omit the cx18 entirely.

The HVR-1600 could have been supported by ir-kbd-i2c.

It's submission was redirected slightly here:

http://lkml.org/lkml/2009/2/3/118

And deferred here:

http://www.spinics.net/lists/linux-media/msg03883.html

until your changes were done.

Quoted 8 lines

Quoted 4 lines

I have an I2C related question. If the cx18 or ivtv driver autoloads "ir-kbd-i2c" and registers an I2C client on the bus, does that preclude lirc_i2c, lirc_pvr150 or lirc_zilog from using the device? LIRC users may notice, if it does.

I don't know anything about lirc_i2c, lirc_pvr150 or lirc_zilog. I tend to ignore all the code that is neither in the Linux kernel tree nor in the v4l-dvb tree.

lirc_pvr150 has always been out of kernel and likely always will be.

lirc_i2c and lirc_zilog, the stripped down version of lirc_pvr150, was submitted by Janne and Jarrod:

http://lkml.org/lkml/2008/9/9/19

I do not know if it any of the lirc drivers made it in. There were lots of comments.

Quoted 5 lines

If you want me to answer this question, you'll have
to tell me what exactly these drivers are doing as far as I2C is concerned. Do they instantiate I2C clients? Or do they do raw I2C transfers? Do they check for address business before they do? On what basis do they attach to I2C devices?

Let me point you to the lirc_i2c.c file and I think you'll understand it faster than I could explain it. Here it is in Jarrod's patch submission:

http://lkml.org/lkml/2008/9/9/3

Essentially, it

1. loads a bunch of bridge chip modules

2. creates and adds I2C driver

3. for every adapter that tries to "attach" the driver a. checks the i2c hw id

b. probes a list of possible addresses based on the id

Quoted 7 lines

Please note that my conversion doesn't actually change anything to the autoloading of the ir-kbd-i2c driver. The bridge drivers which were loading ir-kbd-i2c (saa7134, cx23885, em28xx and cx88) still are. Those which were not, still aren't. The ir-kbd-i2c driver doesn't include a MODULE_ALIAS call as most I2C drivers do, to prevent udev from loading this driver automatically.

Quoted 5 lines

What my conversion changes is that an "ir-kbd" I2C device may be instantiated if a probe is successful. This will make the address in question show as busy (except to i2c-dev which only considers an address as busy when a driver is bound to the device.) But that's about it.

OK. I didn't quite grok if the "ir-kbd" in i2c_new_probed_device() call would load the driver module or not. Tying up the address and making it unavailable for lirc modules is my concern.

Quoted 8 lines

Quoted 7 lines

If that is the case, then we probably shouldn't autoload the ir-kbd module after the CX23418 i2c adapters are initialized.

I'm not sure what's the best solution:

1. A module option to the cx18 driver to tell it to call init_cx18_i2c_ir() from cx18_probe() or not? (Easiest solution)

Quoted 5 lines

Sounds perfectly sensible. I seem to remember that Hans Verkuil told me he wanted something like this for ivtv. As a matter of fact, the saa7134, em28xx and cx231xx already have such a module parameter (disable_ir). Implementing the same for bttv, cx88, cx18, ivtv or any other driver should be fairly trivial.

Yes it's the most expedient thing to do.

Quoted 13 lines

Quoted 4 lines

2. Some involved programmatic way for IR device modules to query bridge drivers about what IR devices they may have, and on which I2C bus, and at what addresses to probe, and whether a driver/module has already claimed that device? (Gold plated solution)

I'd rather name this the over-engineered solution. It's really looking at the situation by the wrong end (that is, with the legacy i2c binding model still in mind.) Bridge drivers know which IR receivers can be present and at which address, it is up to them to instantiate the appropriate I2C devices on the bus, possibly with platform data to help the I2C drivers (be they ir-kbd-i2c, lirc or whatever.) This is exactly what my code does.

Quoted 3 lines

The fact that the same IR chip can be handled by 2 or more I2C drivers is a bad idea to start with. Why the hell did we do that in the first place?

Accident of history? IR receive vs. IR blast/transmit? Why do we have ir-kbd-i2c.c trying to handle a laundry list of devices (somewhat like tvaudio)?

User space apps such as MythTV and mplayer have specific support for LIRC. I guess LIRC's user space components abstract away a lot of the differences of various IR transmitters, receivers and remote controls to make things easier for application writers. Someone with an infradead.org email address can probably speak to LIRC's strengths and weaknesses better than I.

I was wondering why we had ir-kbd-i2c. :) Mark Lord did say, in one of his posts to get the HVR-1600 support in ir-kbd-i2c, he didn't want the LIRC bloat for what he needed.

Quoted 5 lines

If you want a clean solution to the problem, it clearly starts with getting rid of this mess and having each IR receiver chip on I2C supported by exactly one I2C driver and make sure the driver in question is in the Linux kernel tree. Spending time on any other "clean solution" is wasting time IMHO.

Makes sense to me.

Quoted 7 lines

Still, note that it is totally possible to have several I2C drivers support the same device. The new model supports this, just like the old model did. I2C devices are instantiated by bridge drivers, which give them a _name_. Several I2C drivers are allowed to support that chip name, and the first one loaded will get to bind to the device. The "ir-i2c" devices created by cx18, ivtv etc. can be requested by other drivers than ir-kbd-i2c if you want to do that.

Yes. OK. That's the part I didn't understand.

So a hypothetical kernel ir-haup-zilog-i2c.ko module would look for devices with a name of "haup-zilog-ir", right?

And the i2c adapter #
and address can be used to differentiate different instances of the chip with the same name, so names don't have to be unique? Am I correct in my understanding?

Quoted 3 lines

This will require some
changes to lirc_i2c and friends, but at this point changes to these are very needed anyway.

Yes, it looks like LIRC's kernel space components that use I2C may get broken with upcoming I2C subsystem changes.

Hi Andy,

Quoted 2 lines

Quoted 2 lines

If IR on the cx18 is not supported (by the ir-kbd-i2c driver) then I can simplify my patch set and omit the cx18 entirely.

Which I just did...

Quoted 6 lines

The HVR-1600 could have been supported by ir-kbd-i2c.

It's submission was redirected slightly here:

http://lkml.org/lkml/2009/2/3/118

Quoted 5 lines

And deferred here:

http://www.spinics.net/lists/linux-media/msg03883.html

until your changes were done.

OK. Then let's indeed get my changes merged first, and then we can see the best way to add support for the HVR-1600 IR.

Quoted 1 line

lirc_pvr150 has always been out of kernel and likely always will be.

Any valid reason? Out-of-free drivers are a pain for users :(

linux/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c +24 -0

@@ -649,6 +649,27 @@ static void do_i2c_scan(struct pvr2_hdw
 	printk(KERN_INFO "%s: i2c scan done.\n", hdw->name);
 }
 
+static void pvr2_i2c_register_ir(struct i2c_adapter *i2c_adap)
+{
+	struct i2c_board_info info;
+	/* The external IR receiver is at i2c address 0x34 (0x35 for
+	   reads).  Future Hauppauge cards will have an internal
+	   receiver at 0x30 (0x31 for reads).  In theory, both can be
+	   fitted, and Hauppauge suggest an external overrides an
+	   internal.
+
+	   That's why we probe 0x1a (~0x34) first. CB
+	*/
+	const unsigned short addr_list[] = {
+		0x1a, 0x18, 0x4b, 0x64, 0x30,
+		I2C_CLIENT_END
+	};
+
+	memset(&info, 0, sizeof(struct i2c_board_info));
+	strlcpy(info.type, "ir-kbd", I2C_NAME_SIZE);
+	i2c_new_probed_device(i2c_adap, &info, addr_list);
+}
+
 void pvr2_i2c_core_init(struct pvr2_hdw *hdw)
 {
 	unsigned int idx;

@@ -696,6 +717,9 @@ void pvr2_i2c_core_init(struct pvr2_hdw
 		}
 	}
 	if (i2c_scan) do_i2c_scan(hdw);
+
+	/* Instantiate the IR receiver device, if present */
+	pvr2_i2c_register_ir(&hdw->i2c_adap);
 }
 
 void pvr2_i2c_core_done(struct pvr2_hdw *hdw)

Nacked-by Mike Isely <[email protected]>

This will interfere with the alternative use of LIRC drivers (which work in more cases that ir-kbd).

It will thus break some peoples' use of the driver.

Also we have better information on what i2c addresses needed to be probed based on the model of the device

- and some devices supported
by this device are not from Hauppauge so you are making a too-strong assumption that IR should be probed this way in all cases.

Also, unless
ir-kbd has suddenly improved, this will not work at all for HVR-1950 class devices nor MCE type PVR-24xxx devices (different incompatible IR receiver).

This is why the pvrusb2 driver has never directly attempted to load ir-kbd.

Hi Mike,

Quoted 5 lines · Mike Isely

Nacked-by Mike Isely <[email protected]>

This will interfere with the alternative use of LIRC drivers (which work in more cases that ir-kbd).

Why then is ir-kbd in the kernel tree and not LIRC drivers?

Quoted 1 line

It will thus break some peoples' use of the driver.

Do you think it will, or did you test and it actually does? If it indeed breaks, please explain why, so that a solution can be found.

Quoted 3 lines

Also we have better information on what i2c addresses needed to be probed based on the model of the device

This is excellent news. As I said in the header comment of the patch, avoiding probing when we know what the IR receiver is and at which address it sits is the way to go. Please send me all the information you have and I'll be happy to add a patch to the series, that skips probing whenever possible. Or write that patch yourself if you prefer.

Quoted 4 lines

- and some devices supported
by this device are not from Hauppauge so you are making a too-strong assumption that IR should be probed this way in all cases.

I didn't make any assumption, sorry. I simply copied the code from ir-kbd-i2c. If my code does the wrong thing for some devices, that was already the case before. And this will certainly be easier to fix after my changes than before.

On top of that, the "Hauppauge trick" is really only the order in which the addresses are probed. Just because a specific order is better for Hauppauge boards, doesn't mean it won't work for non-Hauppauge boards.

Quoted 5 lines

Also, unless
ir-kbd has suddenly improved, this will not work at all for HVR-1950 class devices nor MCE type PVR-24xxx devices (different incompatible IR receiver).

I'm sorry but you can't blame me for ir-kbd-i2c not supporting some devices. I updated the driver to make use of the new binding model, but that's about all I did.

Quoted 3 lines

This is why the pvrusb2 driver has never directly attempted to load ir-kbd.

The pvrusb2 driver however abuses the bttv driver's I2C adapter ID (I2C_HW_B_BT848) and was thus affected when ir-kbd-i2c is loaded. This is the only reason why my patch touches the pvrusb2 driver. If you tell me you want the ir-kbd-i2c driver to leave pvrusb2 alone, I can drop all the related changes from my patch, that's very easy.

Quoted 5 lines · Jean Delvare

This is excellent news. As I said in the header comment of the patch, avoiding probing when we know what the IR receiver is and at which address it sits is the way to go. Please send me all the information you have and I'll be happy to add a patch to the series, that skips probing whenever possible. Or write that patch yourself if you prefer.

I have samples of most of the devices in question and can code proper I2C addresses for each of those for each resident I2C client. The pvrusb2 driver's device attribute structure already has allowance for specification of the correct I2C addresses to use for chips specific to each device (part of the v4l2-subdev rework I recently did). Right now the driver has built in defaults, and if a particular model needs something else, it can be overridden as part of the device's profile in pvrusb2-devattr.c.

Quoted 4 lines

I didn't make any assumption, sorry. I simply copied the code from ir-kbd-i2c. If my code does the wrong thing for some devices, that was already the case before. And this will certainly be easier to fix after my changes than before.

No, I think the point you are missing is that by moving that logic from ir-kbd-i2c into each driver (e.g. pvrusb2) you are moving logic that *might* be executed into a spot where it *will* be executed.

Remember
that the pvrusb2 driver did not autoload ir-kbd-i2c before this patch. Before this change, a user could elect not to use ir-i2c-kbd simply by not loading it.

The pvrusb2 driver didn't request it, because the
intent all along there is that the user makes the choice by loading the desired driver.

Now with this change, the pvrusb2 driver is going to
attempt to load ir-kbd-i2c where asked for or not.

And if not, the
resulting binding will prevent lirc_i2c from later on loading. If the user had been loading lirc_i2c previously, this will cause his/her usage of IR to break. No, it's not perfect, but it worked. I'm all for improving things, but not by removing an ability that people are using.

(...)
I really don't want to throw rocks here; it's always better to work out the solution than simply block any changes at all. I realize that something has to be done here in order to keep ir-kbd-i2c viable as a choice for users of the pvrusb2 driver. To that end, how about this strategy:

1. Just drop the part of the patch for the pvrusb2 driver and get the rest merged. Yes, I realize that this will break use of ir-kbd-i2c in cooperation with the pvrusb2 driver.

2. Once ir-kbd-i2c has been updated, I will push another set of changes into the pvrusb2 driver that will make it usable there. Basically what I have in mind is similar to what you tried but I'm going to integrate it with the device profiles so that it can be appropriately loaded based on device model, with the correct I2C address in each case. And most importantly, I will add a module option to enable/disable loading or ir-kbd-i2c. This will fix my main objection, since then it will still allow lirc to be usable, for now...

3. I'd like to fix the "abuse" you mention regarding I2C_HW_B_BT848. I'm unclear on what the cleanest solution is there, but if you like to
(a) point at some documentation, (b) describe what I should do, or (c) suggest a patch, I will work to deal with the problem.

Quoted 5 lines

Remember
that the pvrusb2 driver did not autoload ir-kbd-i2c before this patch. Before this change, a user could elect not to use ir-i2c-kbd simply by not loading it.

Which was pretty fragile because another bridge driver could still have loaded it.

Quoted 4 lines

The pvrusb2 driver didn't request it, because the
intent all along there is that the user makes the choice by loading the desired driver.

Which simply underlines how weird the current situation is... Forcing the choice on the user is pretty bad from a usability perspective.

Quoted 3 lines

Now with this change, the pvrusb2 driver is going to
attempt to load ir-kbd-i2c where asked for or not.

Not exactly, only the device will be the instantiated, the drivers won't be loaded, but the result is indeed the same: it's getting in lirc_i2c's way if that's what the user wants to use.

Quoted 6 lines

And if not, the
resulting binding will prevent lirc_i2c from later on loading. If the user had been loading lirc_i2c previously, this will cause his/her usage of IR to break. No, it's not perfect, but it worked. I'm all for improving things, but not by removing an ability that people are using.

I have just added a disable_ir module parameter to work around this issue. Set it to 1 and no "ir-kbd" device will be instantiated, letting lirc_i2c do whatever it wants with the IR receiver device.

You might argue that this is still a regression because the user now has to pass an extra parameter to get the same result as before, but OTOH lirc_i2c will need changes pretty soon anyway, its behavior will have to be changed and the users will notice. There's no way we can go from the current weird situation to a sane situation without changing the (unfortunate) user habits.

Quoted 8 lines

(...)
I really don't want to throw rocks here; it's always better to work out the solution than simply block any changes at all. I realize that something has to be done here in order to keep ir-kbd-i2c viable as a choice for users of the pvrusb2 driver. To that end, how about this strategy:

Quoted 3 lines

1. Just drop the part of the patch for the pvrusb2 driver and get the rest merged. Yes, I realize that this will break use of ir-kbd-i2c in cooperation with the pvrusb2 driver.

As Mauro already said: not acceptable. Breaking an in-tree driver (ir-kbd-i2c) is worse than breaking an out-of-tree driver (lirc_i2c) regardless of the respective number of users or usefulness of these drivers.

Quoted 9 lines

2. Once ir-kbd-i2c has been updated, I will push another set of changes into the pvrusb2 driver that will make it usable there. Basically what I have in mind is similar to what you tried but I'm going to integrate it with the device profiles so that it can be appropriately loaded based on device model, with the correct I2C address in each case. And most importantly, I will add a module option to enable/disable loading or ir-kbd-i2c. This will fix my main objection, since then it will still allow lirc to be usable, for now...

This sounds like a good idea.

Quoted 4 lines

3. I'd like to fix the "abuse" you mention regarding I2C_HW_B_BT848. I'm unclear on what the cleanest solution is there, but if you like to
(a) point at some documentation, (b) describe what I should do, or (c) suggest a patch, I will work to deal with the problem.

Ideally these adapter IDs will no longer be needed within a couple weeks, so it's probably better to leave this alone and just let it die in silence.

Also, lirc makes it possible to userspace map the underlying
IR codes to keybindings and associate multiple different remotes - all of that is apparently hardcoded in ir-kbd-i2c.

Wierd or not, your
change makes it hard(er) on those who legitimately wish to use lirc. Here's an interesting summary:

If fact, the only pvrusb2-driven hardware from Hauppauge that is known to work with ir-kbd-i2c are the old 29xxx and 24xxx model series (not the "MCE" series). Those devices are out of production, AFAIK. The current devices being sold by Hauppauge don't work at all with ir-kbd-i2c and probably never will.

Perhaps one can conclude that there hasn't been a lot of pressure (that I know about) to deal with updating / enhancing / replacing ir-kbd-i2c because lirc happens to be filling the niche better in many cases.

Here is where LIRC may be its own worst enemy. LIRC has filled some shortcomings in the kernel for support of IR device functions for so long (LWN says LIRC is 10 years old), that large numbers of users have come to depend on its operation, while at the same time apparently removing impetus for doing much to update the in kernel IR device support.

Hi Mike,

Quoted 4 lines · Mike Isely

Nacked-by Mike Isely <[email protected]>

This will interfere with the alternative use of LIRC drivers (which work in more cases that ir-kbd).

Why then is ir-kbd in the kernel tree and not LIRC drivers?

Quoted 1 line

It will thus break some peoples' use of the driver.

Do you think it will, or did you test and it actually does? If it indeed breaks, please explain why, so that a solution can be found.

Quoted 2 lines

Also we have better information on what i2c addresses needed to be probed based on the model of the device

This is excellent news. As I said in the header comment of the patch, avoiding probing when we know what the IR receiver is and at which address it sits is the way to go. Please send me all the information you have and I'll be happy to add a patch to the series, that skips probing whenever possible. Or write that patch yourself if you prefer.

Quoted 3 lines

- and some devices supported
by this device are not from Hauppauge so you are making a too-strong assumption that IR should be probed this way in all cases.

I didn't make any assumption, sorry. I simply copied the code from ir-kbd-i2c. If my code does the wrong thing for some devices, that was already the case before. And this will certainly be easier to fix after my changes than before.

On top of that, the "Hauppauge trick" is really only the order in which the addresses are probed. Just because a specific order is better for Hauppauge boards, doesn't mean it won't work for non-Hauppauge boards.

Quoted 4 lines

Also, unless
ir-kbd has suddenly improved, this will not work at all for HVR-1950 class devices nor MCE type PVR-24xxx devices (different incompatible IR receiver).

I'm sorry but you can't blame me for ir-kbd-i2c not supporting some devices. I updated the driver to make use of the new binding model, but that's about all I did.

Quoted 2 lines

This is why the pvrusb2 driver has never directly attempted to load ir-kbd.

The pvrusb2 driver however abuses the bttv driver's I2C adapter ID (I2C_HW_B_BT848) and was thus affected when ir-kbd-i2c is loaded. This is the only reason why my patch touches the pvrusb2 driver. If you tell me you want the ir-kbd-i2c driver to leave pvrusb2 alone, I can drop all the related changes from my patch, that's very easy.

Quoted 46 lines · On Sunday 05 April 2009 01:05:39 Jean Delvare wrote:

Hi Mike,

Quoted 4 lines · Mike Isely

Nacked-by Mike Isely <[email protected]>

This will interfere with the alternative use of LIRC drivers (which work in more cases that ir-kbd).

Why then is ir-kbd in the kernel tree and not LIRC drivers?

Quoted 1 line

It will thus break some peoples' use of the driver.

Do you think it will, or did you test and it actually does? If it indeed breaks, please explain why, so that a solution can be found.

Quoted 2 lines

Also we have better information on what i2c addresses needed to be probed based on the model of the device

This is excellent news. As I said in the header comment of the patch, avoiding probing when we know what the IR receiver is and at which address it sits is the way to go. Please send me all the information you have and I'll be happy to add a patch to the series, that skips probing whenever possible. Or write that patch yourself if you prefer.

Quoted 3 lines

- and some devices supported
by this device are not from Hauppauge so you are making a too-strong assumption that IR should be probed this way in all cases.

I didn't make any assumption, sorry. I simply copied the code from ir-kbd-i2c. If my code does the wrong thing for some devices, that was already the case before. And this will certainly be easier to fix after my changes than before.

On top of that, the "Hauppauge trick" is really only the order in which the addresses are probed. Just because a specific order is better for Hauppauge boards, doesn't mean it won't work for non-Hauppauge boards.

Quoted 4 lines

Also, unless
ir-kbd has suddenly improved, this will not work at all for HVR-1950 class devices nor MCE type PVR-24xxx devices (different incompatible IR receiver).

I'm sorry but you can't blame me for ir-kbd-i2c not supporting some devices. I updated the driver to make use of the new binding model, but that's about all I did.

Quoted 8 lines

Quoted 2 lines

This is why the pvrusb2 driver has never directly attempted to load ir-kbd.

The pvrusb2 driver however abuses the bttv driver's I2C adapter ID (I2C_HW_B_BT848) and was thus affected when ir-kbd-i2c is loaded. This is the only reason why my patch touches the pvrusb2 driver. If you tell me you want the ir-kbd-i2c driver to leave pvrusb2 alone, I can drop all the related changes from my patch, that's very easy.

Let's keep it simple: add a 'load_ir_kbd_i2c' module option for those drivers that did not autoload this module. The driver author can refine things later (I'll definitely will do that for ivtv).

It will be interesting if someone can find out whether lirc will work at all once autoprobing is removed from i2c. If it isn't, then perhaps that will wake them up to the realization that they really need to move to the kernel.

The new mechanism is the right way to do it: the adapter driver has all the information if, where and what IR is used and so should be the one to tell the kernel what to do. Attempting to autodetect and magically figure out what IR might be there is awkward and probably impossible to get right 100%.

Hell, it's wrong already: if you have another board that already loads ir-kbd-i2c then if you load ivtv or pvrusb2 afterwards you get ir-kbd-i2c whether you like it or not, because ir-kbd-i2c will connect to your i2c adapter like a leech. So with the addition of a module option you at least give back control of this to the user.

When this initial conversion is done I'm pretty sure we can improve ir-kbd-i2c to make it easier to let the adapter driver tell it what to do. So we don't need those horrible adapter ID tests and other magic that's going on in that driver. But that's phase two.

From what I got from Andy and Mike's comments is that the real issue is that

IMO, doing all those tricks to support an out-of-tree driver is the wrong approach. This is just postponing a more serious discussion about what should be done in kernel, in order to better support IR's.

In the case of lirc, the userspace part has already an event interface. If the drivers are doing the right thing with their IR part, lirc can just use the event interface for all drivers. This seems to be the proper approach.

Quoted 1 line

From what I got from Andy and Mike's comments is that the real issue is that

the IR kernel code is incomplete, broken or bad designed. So, several users and userspace apps don't rely on the kernel code but, instead, use lirc as an alternative.

That's said, I propose a different approach:

1) Add some entry at feature-removal-schedule.txt posting a date to end support for out-of-tree I2C IR modules;

2) Start discussing with lirc people (and input/event maintainers if needed) about what is needed to properly support the required functionalities for a better lirc usage;

3) Propose a few API additions in order to support those functionalities;

4) apply IR patches on kernel to support the missing functionalities;

5) remove the support for out-of-tree i2c IR modules.

Cheers,
Mauro

The scope of a complete kernel IR infrastructure goes a bit beyond I2C bus devices that are only input devices.

What's the scope of what you want to tackle here?

I certainly don't want to reinvent something that's going to look just like the LIRC driver model:

http://www.lirc.org/html/technical.html

Which already has an infrastructure for IR driver module writers:
http://www.lirc.org/html/technical.html#lirc_dev

Do we just convert lirc_dev, lirc_i2c, and lirc_zilog to a cleaned up set of in kernel modules?

lirc_i2c can certainly be broken up into
several modules: 1 per supported device.

Should these create an input
device as well to maintain compatability with apps expecting an ir-kbd-i2c like function?

Or do we split up ir-kbd-i2c into per device modules and in addition to the input event interface, have it register with the lirc_dev module?

Do we leverage LIRC's lirc_dev infrastructure module at all? (I think it would be a waste of time not to do so.)

Let's keep it simple: add a 'load_ir_kbd_i2c' module option for those drivers that did not autoload this module. The driver author can refine things later (I'll definitely will do that for ivtv).

It will be interesting if someone can find out whether lirc will work at all once autoprobing is removed from i2c. If it isn't, then perhaps that will wake them up to the realization that they really need to move to the kernel.

The new mechanism is the right way to do it: the adapter driver has all the information if, where and what IR is used and so should be the one to tell the kernel what to do. Attempting to autodetect and magically figure out what IR might be there is awkward and probably impossible to get right 100%.

Hell, it's wrong already: if you have another board that already loads ir-kbd-i2c then if you load ivtv or pvrusb2 afterwards you get ir-kbd-i2c whether you like it or not, because ir-kbd-i2c will connect to your i2c adapter like a leech. So with the addition of a module option you at least give back control of this to the user.

When this initial conversion is done I'm pretty sure we can improve ir-kbd-i2c to make it easier to let the adapter driver tell it what to do. So we don't need those horrible adapter ID tests and other magic that's going on in that driver. But that's phase two.

Hi Hans,

Updated patch set is available at:
http://jdelvare.pck.nerim.net/linux/ir-kbd-i2c/

linux/drivers/media/video/ivtv/ivtv-i2c.c +4 -1

[snip]
-
+		const unsigned short addr_list[] = {
+			0x1a, 0x18, 0x64, 0x30,
+			I2C_CLIENT_END
+		};
[snip]

I just noticed you're missing address 0x71 for ivtv. At least some PVR-150 boards have a Zilog chip at that address.

Hi Andy,

I'm all for adding
support for more boards, however I'd rather do this _after_ the i2c model conversion is done, so that we have a proper changelog entry saying that we added support for the PVR-150, and that it gets proper testing. Hiding support addition in a larger patch would probably do as much harm as good.

Let's keep it simple: add a 'load_ir_kbd_i2c' module option for those drivers that did not autoload this module. The driver author can refine things later (I'll definitely will do that for ivtv).

It will be interesting if someone can find out whether lirc will work at all once autoprobing is removed from i2c. If it isn't, then perhaps that will wake them up to the realization that they really need to move to the kernel.

Quoted 9 lines · Hans Verkuil

Let's keep it simple: add a 'load_ir_kbd_i2c' module option for those drivers that did not autoload this module. The driver author can refine things later (I'll definitely will do that for ivtv).

It will be interesting if someone can find out whether lirc will work at all once autoprobing is removed from i2c. If it isn't, then perhaps that will wake them up to the realization that they really need to move to the kernel.

I would guess that it won't work. There is an effort to merge lirc. It's currently stalled though. A git tree is available at

git://git.wilsonet.com/linux-2.6-lirc.git

Jared Wilson and I were working on it (mainly last september). Since the IR on the HD PVR is also driven by the same zilog chip as on other hauppauge devices I'll take of lirc_zilog. Help converting the i2c drivers to the new i2c model is welcome. General cleanup of lirc to make it ready for mainline is of course wellcome too.

Hi Janne,

Quoted 14 lines · Janne Grunau

Quoted 9 lines · Hans Verkuil

Let's keep it simple: add a 'load_ir_kbd_i2c' module option for those drivers that did not autoload this module. The driver author can refine things later (I'll definitely will do that for ivtv).

It will be interesting if someone can find out whether lirc will work at all once autoprobing is removed from i2c. If it isn't, then perhaps that will wake them up to the realization that they really need to move to the kernel.

I would guess that it won't work. There is an effort to merge lirc. It's currently stalled though. A git tree is available at

Quoted 1 line

git://git.wilsonet.com/linux-2.6-lirc.git

I tried to clone this but it failed:

git.wilsonet.com[0: 72.93.233.4]: errno=Connection timed out
fatal: unable to connect a socket (Connection timed out)

I will happily help you with the i2c side of things. Without an access to your git tree however, I don't know the latest state of your code. And I can't see any lirc_zilog module in the CVS repository of lirc either. But if you show me the i2c code you're worried about, I'll let you know what I think about it.

Quoted 5 lines · Hans Verkuil

Let's keep it simple: add a 'load_ir_kbd_i2c' module option for those drivers that did not autoload this module. The driver author can refine things later (I'll definitely will do that for ivtv).

Quoted 4 lines

It will be interesting if someone can find out whether lirc will work at all once autoprobing is removed from i2c. If it isn't, then perhaps that will wake them up to the realization that they really need to move to the kernel.

I would guess that it won't work. There is an effort to merge lirc. It's currently stalled though.

A git tree is available at

git://git.wilsonet.com/linux-2.6-lirc.git

Jared Wilson and I were working on it (mainly last september). Since the IR on the HD PVR is also driven by the same zilog chip as on other hauppauge devices I'll take of lirc_zilog. Help converting the i2c drivers to the new i2c model is welcome. General cleanup of lirc to make it ready for mainline is of course wellcome too.

Janne

Quoted 3 lines · Janne Grunau

I would guess that it won't work. There is an effort to merge lirc. It's currently stalled though.

Perhaps you and Jarrod and Christopher have already discussed this, but...

Instead of trying to push all of the LIRC kernel components through in one big patch set, perhaps it would be easier to just get the lirc_dev and any other needed infrastructure components in first.

If one focuses on satisfying the LKML comments to lirc_dev and the Makefile to get that kernel module in the kernel, then, at least for video card hosted IR devices, there is an infrastructure to which to hook new or rewritten i2c IR driver modules.

Quoted 9 lines

A git tree is available at

git://git.wilsonet.com/linux-2.6-lirc.git

Jared Wilson and I were working on it (mainly last september). Since the IR on the HD PVR is also driven by the same zilog chip as on other hauppauge devices I'll take of lirc_zilog. Help converting the i2c drivers to the new i2c model is welcome. General cleanup of lirc to make it ready for mainline is of course wellcome too.

I can help with this. I'm mainly concerned with lirc_dev, lirc_i2c (for Rx only use of the zilog at 0x71), lirc_zilog, and lirc_mceusb2. That's because, of course, I have devices that use those modules. :)

lirc_dev and the API header would be my first priority, if you need help. Did anyone consolidate all the comments from the LKML on Jarrod's patch submission?

Quoted 4 lines · Andy Walls

If one focuses on satisfying the LKML comments to lirc_dev and the Makefile to get that kernel module in the kernel, then, at least for video card hosted IR devices, there is an infrastructure to which to hook new or rewritten i2c IR driver modules.

I guess lkml would NAK patches adding infrastructure only bits but we will probably for the next patchset concentrate on a few lirc drivers. Christopher doesn't participate in the merge attempt.

Quoted 10 lines

Quoted 9 lines

A git tree is available at

git://git.wilsonet.com/linux-2.6-lirc.git

Jared Wilson and I were working on it (mainly last september). Since the IR on the HD PVR is also driven by the same zilog chip as on other hauppauge devices I'll take of lirc_zilog. Help converting the i2c drivers to the new i2c model is welcome. General cleanup of lirc to make it ready for mainline is of course wellcome too.

Quoted 3 lines

I can help with this. I'm mainly concerned with lirc_dev, lirc_i2c (for Rx only use of the zilog at 0x71), lirc_zilog, and lirc_mceusb2. That's because, of course, I have devices that use those modules. :)

I have devices for lirc_zilog (which should probably be merged with lirc_i2c)

and lirc serial. Jarod has at least mce usb and imon devices. That are probably the devices we'll concentrate on the next submission.

Quoted 3 lines

lirc_dev and the API header would be my first priority, if you need help. Did anyone consolidate all the comments from the LKML on Jarrod's patch submission?

no and I lost track which comments were already handled.

Janne

Quoted 2 lines · Janne Grunau

I have devices for lirc_zilog (which should probably be merged with lirc_i2c)

Hmmm. Following Jean's reasoning, that may be the wrong way to go, if you want to avoid probing. A module to handle each specific type of I2C IR chip, splitting up lirc_i2c and leaving lirc_zilog as is, may be better in the long run.

Quoted 8 lines · Janne Grunau

Quoted 2 lines

I have devices for lirc_zilog (which should probably be merged with lirc_i2c)

Hmmm. Following Jean's reasoning, that may be the wrong way to go, if you want to avoid probing. A module to handle each specific type of I2C IR chip, splitting up lirc_i2c and leaving lirc_zilog as is, may be better in the long run.

This really doesn't matter. With the new binding model, probing is under control. You can do probing on some cards and not others, and you can probe some addresses and not others. And one i2c drivers can cleanly support more than one device type.

What should be considered to decide whether two devices should be supported by the same driver or not, is how much their supporting code has in common.

Am Sonntag, den 05.04.2009, 22:22 +0200 schrieb Jean Delvare:

Quoted 1 line · Andy Walls

What can not be translated to the input system I would like to know. Andy seems to have closer looked into that.

Quoted 6 lines

Am Sonntag, den 05.04.2009, 22:22 +0200 schrieb Jean Delvare:

Quoted 1 line · Andy Walls

What can not be translated to the input system I would like to know. Andy seems to have closer looked into that.

1. IR blasting: sending IR codes to transmit out to a cable convertor box, DTV to analog convertor box, or similar devices to change channels before recording starts. An input interface doesn't work well for output.

2. Sending raw IR samples to user space: user space applications can then decode or match an unknown or non-standard IR remote protocol in user space software. Timing information to go along with the sample
data probably needs to be preserved. I'm assuming the input interface
currently doesn't support that.

That's all the Gerd mentioned.

One more nice feature to have, that I'm not sure how easily the input system could support:

3. specifying remote control code to key/button translations with a configuration file instead of recompiling a module.

In effect there are actually two devices the ir-kbd-i2c input driver is supporting in various combinations: an IR receiver and an IR remote.

Regards,
Andy

Quoted 2 lines · hermann pitton

Am Sonntag, den 05.04.2009, 22:22 +0200 schrieb Jean Delvare:

Quoted 1 line · Andy Walls

Quoted 2 lines

What can not be translated to the input system I would like to know. Andy seems to have closer looked into that.

1. IR blasting: sending IR codes to transmit out to a cable convertor box, DTV to analog convertor box, or similar devices to change channels before recording starts. An input interface doesn't work well for output.

2. Sending raw IR samples to user space: user space applications can then decode or match an unknown or non-standard IR remote protocol in user space software. Timing information to go along with the sample
data probably needs to be preserved. I'm assuming the input interface
currently doesn't support that.

That's all the Gerd mentioned.

One more nice feature to have, that I'm not sure how easily the input system could support:

3. specifying remote control code to key/button translations with a configuration file instead of recompiling a module.

Quoted 8 lines · On Sun, 05 Apr 2009 18:00:04 -0400 Andy Walls <[email protected]> wrote:

Quoted 6 lines · hermann pitton

Am Sonntag, den 05.04.2009, 22:22 +0200 schrieb Jean Delvare:

Quoted 1 line · Andy Walls

What can not be translated to the input system I would like to know. Andy seems to have closer looked into that.

Quoted 4 lines

1. IR blasting: sending IR codes to transmit out to a cable convertor box, DTV to analog convertor box, or similar devices to change channels before recording starts. An input interface doesn't work well for output.

On my understanding, IR output is a separate issue. AFAIK, only a very few ivtv devices support IR output. I'm not sure how this is currently implemented.

Quoted 7 lines

2. Sending raw IR samples to user space: user space applications can then decode or match an unknown or non-standard IR remote protocol in user space software. Timing information to go along with the sample
data probably needs to be preserved. I'm assuming the input interface
currently doesn't support that.

If the driver processes correctly the IR samples, I don't see why you would need to pass the raw protocols to userspace. Maybe we need to add some ioctls at the API to allow certain controls, like, for example, ask kernel to decode IR using RC4 instead or RC5, on devices that supports more than one IR protocol.

Quoted 9 lines

That's all the Gerd mentioned.

One more nice feature to have, that I'm not sure how easily the input system could support:

3. specifying remote control code to key/button translations with a configuration file instead of recompiling a module.

The input and the current drivers that use input already supports this feature. You just need to load a new code table to replace the existing one.

See v4l2-apps/util/keytable.c to see how easy is to change a key code. It contains a complete code to fully replace a key code table. Also, the Makefile there will extract the current keytables for the in-kernel drivers.

Btw, with only 12 lines, you can create a keycode replace "hello world!":

#include <fcntl.h> /* due to O_RDONLY */
#include <stdio.h> /* open() */
#include <linux/input.h> /* input ioctls and keycode macros */
#include <sys/ioctl.h> /* ioctl() */
void main(void)

{
	int codes[2];
	int fd = open("/dev/video0", O_RDONLY);	/* Hmm.. in real apps, we should check for errors */
	codes[0] = 10;				/* Scan code */
	codes[1] = KEY_UP;			/* Key code */
	ioctl(fd, EVIOCSKEYCODE, codes);	/* hello world! */
}

Quoted 12 lines

Quoted 8 lines · On Sun, 05 Apr 2009 18:00:04 -0400 Andy Walls <[email protected]> wrote:

Quoted 6 lines · hermann pitton

Am Sonntag, den 05.04.2009, 22:22 +0200 schrieb Jean Delvare:

Quoted 1 line · Andy Walls

What can not be translated to the input system I would like to know. Andy seems to have closer looked into that.

Quoted 7 lines

Quoted 4 lines

1. IR blasting: sending IR codes to transmit out to a cable convertor box, DTV to analog convertor box, or similar devices to change channels before recording starts. An input interface doesn't work well for output.

On my understanding, IR output is a separate issue. AFAIK, only a very few ivtv devices support IR output. I'm not sure how this is currently implemented.

For the pvrusb2 driver, MCE style 24xxx devices (2nd generation 24xxx) and HVR-1950 devices have IR blasting capabilities. At the moment, people have gotten this to work on the 24xxx model with the appropriate lirc driver. In theory it should be doable for HVR-1950 as well (and the pvrusb2 does what is needed to make it possible) but I don't think anyone has succeeded there yet.

Sure IR output as a concept and interface is a separate issue. But it can be implemented in the same chip (which is the case in the two examples I list above). So the issue is not separate; it must be dealt with as a whole. Two drivers implementing different features but trying to share one chip is just not fun.

Quoted 8 lines

Quoted 7 lines

2. Sending raw IR samples to user space: user space applications can then decode or match an unknown or non-standard IR remote protocol in user space software. Timing information to go along with the sample
data probably needs to be preserved. I'm assuming the input interface
currently doesn't support that.

Quoted 4 lines

If the driver processes correctly the IR samples, I don't see why you would need to pass the raw protocols to userspace. Maybe we need to add some ioctls at the API to allow certain controls, like, for example, ask kernel to decode IR using RC4 instead or RC5, on devices that supports more than one IR protocol.

Ugh. Why should v4l-dvb get into this business when it's already solved somewhere else? In userspace even.

I see in so many other places people arguing for V4L functionality that needs to be kicked out of the kernel and put into userspace. For example, there's all that silliness over pixel formats that I'm soon going to have to deal with...

Yet in this case with IR, there already exists a subsystem that does *more* than ir-kbd-i2c.c, AND it does all the crazy configuration / control in userspace - and yet you argue that ir-kbd-i2c.c should be preferred? Purely because lirc is not in-tree? Well heck, lirc should be in-tree. Let's help them get there and forget ever having to deal with IR again ourselves. Let them do it.

Quoted 24 lines

Quoted 9 lines

That's all the Gerd mentioned.

One more nice feature to have, that I'm not sure how easily the input system could support:

3. specifying remote control code to key/button translations with a configuration file instead of recompiling a module.

The input and the current drivers that use input already supports this feature. You just need to load a new code table to replace the existing one.

See v4l2-apps/util/keytable.c to see how easy is to change a key code. It contains a complete code to fully replace a key code table. Also, the Makefile there will extract the current keytables for the in-kernel drivers.

Btw, with only 12 lines, you can create a keycode replace "hello world!":

#include <fcntl.h> /* due to O_RDONLY */
#include <stdio.h> /* open() */
#include <linux/input.h> /* input ioctls and keycode macros */
#include <sys/ioctl.h> /* ioctl() */
void main(void)

Quoted 7 lines

{
	int codes[2];
	int fd = open("/dev/video0", O_RDONLY);	/* Hmm.. in real apps, we should check for errors */
	codes[0] = 10;				/* Scan code */
	codes[1] = KEY_UP;			/* Key code */
	ioctl(fd, EVIOCSKEYCODE, codes);	/* hello world! */
}

I just looked at this. I freely admit I haven't noticed this before, but having looked at it now, and having examined ir-kbd-i2c.c, I still don't see the whole picture here:

1. The switch statement in ir-kbd-i2c.c:ir_attach() is apparently implicitly trying to assume a particular type of remote based on the I2C address of the IR receiver it's talking to. Yuck. That's really not right at all. The IR receiver used does not automatically mean which remote is used. What if the vendor switches remotes? That's happened with the PVR-USB2 hardware in the past (based on photos I've seen). Who's to say the next remote to be supplied is compatible?

2. Your example above is opening the video device endpoint and issuing ioctl()s that are not part of V4L. That is supposed to work?!?

3. A given IR remote may be described by much more than what 'scan codes' it produces. I don't know a lot about IR, but looking at the typical lirc definition for a remote, there's obvious timing and protocol parameters as well. Just being able to swap scan codes around is not always going to be enough.

4. I imagine that the input event framework in the kernel has a means for programmatic mapping of scan codes to key codes, but looking at ir-kbd-i2c.c, it appears to only be selecting from among a very small set of kernel-compiled translation tables. I must be missing something here.

In an earlier post (from Andy?) some history was dug up about ir-kbd-i2c.c. From what I understand, the only reason ir-kbd-i2c.c came into existence was because lirc was late in supporting 2.6.x series kernels and Gerd needed *something* to allow IR to work. So he created this module, knowing full well that it didn't cover all possible cases. Rather it covered the common cases he cared about. That was a while ago. And we need to do all the cases - or at least not mess up what already exists elsewhere that does handle the "uncommon" cases. The lirc drivers do work in 2.6. And apparently they were on the scene before ir-kbd-i2c.c, just unfortunately not in-tree. The lirc drivers really need to get into the kernel. From where I'm sitting the long term goal should be to get lirc into the kernel.

-Mike

1. The switch statement in ir-kbd-i2c.c:ir_attach() is apparently implicitly trying to assume a particular type of remote based on the I2C address of the IR receiver it's talking to. Yuck. That's really not right at all. The IR receiver used does not automatically mean which remote is used. What if the vendor switches remotes? That's happened with the PVR-USB2 hardware in the past (based on photos I've seen). Who's to say the next remote to be supplied is compatible?

3. A given IR remote may be described by much more than what 'scan codes' it produces. I don't know a lot about IR, but looking at the typical lirc definition for a remote, there's obvious timing and protocol parameters as well. Just being able to swap scan codes around is not always going to be enough.

Quoted 11 lines · Mauro Carvalho Chehab

Quoted 8 lines · On Sun, 05 Apr 2009 18:00:04 -0400 Andy Walls <[email protected]> wrote:

Quoted 6 lines · hermann pitton

Am Sonntag, den 05.04.2009, 22:22 +0200 schrieb Jean Delvare:

Quoted 1 line · Andy Walls

What can not be translated to the input system I would like to know. Andy seems to have closer looked into that.

Quoted 7 lines

Quoted 4 lines

1. IR blasting: sending IR codes to transmit out to a cable convertor box, DTV to analog convertor box, or similar devices to change channels before recording starts. An input interface doesn't work well for output.

On my understanding, IR output is a separate issue. AFAIK, only a very few ivtv devices support IR output. I'm not sure how this is currently implemented.

For the pvrusb2 driver, MCE style 24xxx devices (2nd generation 24xxx) and HVR-1950 devices have IR blasting capabilities. At the moment, people have gotten this to work on the 24xxx model with the appropriate lirc driver. In theory it should be doable for HVR-1950 as well (and the pvrusb2 does what is needed to make it possible) but I don't think anyone has succeeded there yet.

Sure IR output as a concept and interface is a separate issue. But it can be implemented in the same chip (which is the case in the two examples I list above). So the issue is not separate; it must be dealt with as a whole. Two drivers implementing different features but trying to share one chip is just not fun.

Quoted 4 lines

If the driver processes correctly the IR samples, I don't see why you would need to pass the raw protocols to userspace. Maybe we need to add some ioctls at the API to allow certain controls, like, for example, ask kernel to decode IR using RC4 instead or RC5, on devices that supports more than one IR protocol.

Ugh. Why should v4l-dvb get into this business when it's already solved somewhere else? In userspace even.

I see in so many other places people arguing for V4L functionality that needs to be kicked out of the kernel and put into userspace. For example, there's all that silliness over pixel formats that I'm soon going to have to deal with...

Yet in this case with IR, there already exists a subsystem that does *more* than ir-kbd-i2c.c, AND it does all the crazy configuration / control in userspace - and yet you argue that ir-kbd-i2c.c should be preferred? Purely because lirc is not in-tree?

Well heck, lirc should be in-tree. Let's help them get there and forget ever having to deal with IR again ourselves. Let them do it.

I just looked at this. I freely admit I haven't noticed this before, but having looked at it now, and having examined ir-kbd-i2c.c, I still don't see the whole picture here:

1. The switch statement in ir-kbd-i2c.c:ir_attach() is apparently implicitly trying to assume a particular type of remote based on the I2C address of the IR receiver it's talking to. Yuck. That's really not right at all. The IR receiver used does not automatically mean which remote is used. What if the vendor switches remotes? That's happened with the PVR-USB2 hardware in the past (based on photos I've seen). Who's to say the next remote to be supplied is compatible?

2. Your example above is opening the video device endpoint and issuing ioctl()s that are not part of V4L. That is supposed to work?!?

3. A given IR remote may be described by much more than what 'scan codes' it produces. I don't know a lot about IR, but looking at the typical lirc definition for a remote, there's obvious timing and protocol parameters as well. Just being able to swap scan codes around is not always going to be enough.

4. I imagine that the input event framework in the kernel has a means for programmatic mapping of scan codes to key codes, but looking at ir-kbd-i2c.c, it appears to only be selecting from among a very small set of kernel-compiled translation tables. I must be missing something here.

In an earlier post (from Andy?) some history was dug up about ir-kbd-i2c.c. From what I understand, the only reason ir-kbd-i2c.c came into existence was because lirc was late in supporting 2.6.x series kernels and Gerd needed *something* to allow IR to work. So he created

this module, knowing full well that it didn't cover all possible cases. Rather it covered the common cases he cared about. That was a while ago. And we need to do all the cases - or at least not mess up what already exists elsewhere that does handle the "uncommon" cases. The lirc drivers do work in 2.6. And apparently they were on the scene before ir-kbd-i2c.c, just unfortunately not in-tree. The lirc drivers really need to get into the kernel. From where I'm sitting the long term goal should be to get lirc into the kernel.

Quoted 6 lines

Am Sonntag, den 05.04.2009, 22:22 +0200 schrieb Jean Delvare:

Quoted 1 line · Andy Walls

What can not be translated to the input system I would like to know. Andy seems to have closer looked into that.

1. IR blasting: sending IR codes to transmit out to a cable convertor box, DTV to analog convertor box, or similar devices to change channels before recording starts. An input interface doesn't work well for output.

2. Sending raw IR samples to user space: user space applications can then decode or match an unknown or non-standard IR remote protocol in user space software. Timing information to go along with the sample
data probably needs to be preserved. I'm assuming the input interface
currently doesn't support that.

That's all the Gerd mentioned.

One more nice feature to have, that I'm not sure how easily the input system could support:

3. specifying remote control code to key/button translations with a configuration file instead of recompiling a module.

In effect there are actually two devices the ir-kbd-i2c input driver is supporting in various combinations: an IR receiver and an IR remote.

Regards,
Andy

Hmm, I'm still "happy" with the broken DVB-T for saa7134 on 2.6.29, tasting some Chianti vine now and need to sleep soon, but I'm also not that confident that your saa7134 MSI TV@nywhere Plus i2c remote does work addressing it directly, since previous reports always said it becomes only visible at all after other devices are probed previously.

Unfortunately I can't test it, but will try to reach some with such hardware and ask for testing, likely not on the list currently.

Quoted 8 lines

Hmm, I'm still "happy" with the broken DVB-T for saa7134 on 2.6.29, tasting some Chianti vine now and need to sleep soon, but I'm also not that confident that your saa7134 MSI TV@nywhere Plus i2c remote does work addressing it directly, since previous reports always said it becomes only visible at all after other devices are probed previously.

Unfortunately I can't test it, but will try to reach some with such hardware and ask for testing, likely not on the list currently.

Thanks for the heads up. I was curious about this as well. The original comment said that the MSI TV@nywhere Plus IR receiver would not respond to _probes_ before another device on the I2C bus was accessed. I didn't know for sure if this only applied to the probe sequence or to any attempt to access the IR receiver. As we no longer need to probe for the device, I thought it may be OK to remove the extra code. But probably the removal of the extra code should be delayed until we find one tester to confirm the exact behavior. Here, done.

Anyone out there with a MSI TV@nywhere Plus that could help with testing?

Am Montag, den 06.04.2009, 10:40 +0200 schrieb Jean Delvare:

Quoted 2 lines

Anyone out there with a MSI TV@nywhere Plus that could help with testing?

Here is a link to one of the initial reports by Henry, others are close to it.

http://marc.info/?l=linux-video&m=113324147429459&w=2

There are two different variants of that MSI card, but that undocumented KS003 chip is the same on them.

Quoted 5 lines · hermann pitton

Am Montag, den 06.04.2009, 10:40 +0200 schrieb Jean Delvare:

Quoted 3 lines

Anyone out there with a MSI TV@nywhere Plus that could help with testing?

Quoted 8 lines

Here is a link to one of the initial reports by Henry, others are close to it.

http://marc.info/?l=linux-video&m=113324147429459&w=2

There are two different variants of that MSI card, but that undocumented KS003 chip is the same on them.

Great, thanks for the pointer. If I understand correctly, the KS003 has a state machine flow which causes the chip to stop answering when an invalid address is used on the bus and start answering again when a valid address other than his own is used. As the old i2c model relied a lot on probing, I am not surprised that this was a problem in the past. But with the new model, probes should become infrequent, so I suspect that the workaround may no longer be needed... except when i2c_scan=1 is used.

I'd rather keep the workaround in place for the time being, and only once the ir-kbd-i2c changes have settled, try to remove it if someone really cares.

Regarding the KS003 (& KS007; the other "mystery" chip):

Upon further investigation of some info from a post from last year
(http://www.linuxtv.org/pipermail/linux-dvb/2008-January/022634.html),
it appears that these (assuming that they are the same IC across the various MSI, Leadtek & KWorld cards; and I believe that to be true) are the "AT8PS54/S56" chip from "Feeling Technology" ... the datasheet for that part is available through a google search .... probing further (as I had never heard of FT before and so I looked them up), it looks like FT renamed and/or upgraded the chip to the "FM8PS54/S56" ... the near identical datasheet for that second version is also available:
http://www.feeling-tech.com.tw/km-master/front/bin/ptdetail.phtml?Part=M1-05&Category=100018

From what I've investigated, several of those IR chips are micro-controllers like

Quoted 13 lines · On Tue, 07 Apr 2009 23:02:43 -0400 CityK <[email protected]> wrote:

Regarding the KS003 (& KS007; the other "mystery" chip):

Upon further investigation of some info from a post from last year
(http://www.linuxtv.org/pipermail/linux-dvb/2008-January/022634.html),
it appears that these (assuming that they are the same IC across the various MSI, Leadtek & KWorld cards; and I believe that to be true) are the "AT8PS54/S56" chip from "Feeling Technology" ... the datasheet for that part is available through a google search .... probing further (as I had never heard of FT before and so I looked them up), it looks like FT renamed and/or upgraded the chip to the "FM8PS54/S56" ... the near identical datasheet for that second version is also available:
http://www.feeling-tech.com.tw/km-master/front/bin/ptdetail.phtml?Part=M1-05&Category=100018

From what I've investigated, several of those IR chips are micro-controllers like the one you pointed. I've seen a few boards whose IR chip is not masked. On those, I always went into some micro-controller datasheet.

Those IR's with a micro-controller have some software inside it to decode one IR protocol and generate scan-code sequences that can be received via GPIO or via I2C, depending on the firmware content.

The datasheet of those chips are useless, since the behaviour of the device is programmed inside their ROM/EEPROM [1]. So, even being the same chip, you could have two "K007" devices with different firmwares, listening on different i2c addresses and eventually generating different scan-codes for the same IR.

On the other hand, for USB devices and for bttv, saa7134 and cx88, there are some easy ways to monitor what i2c messages or GPIO pins are involved with IR. In general, the IR received messages generated by the firmware are some header, a scan code, a repeat key bit and a trailer. So, it is not hard to generate a get-key routine to get the scan code and the repeat bit from the protocol.

That's why the modern ir-kbd-i2c approach is to select the proper IR parameters after binding the module, at the bridge driver. The bridge driver is the one who knows what's the IR scan code of the original IR (to set it as the default), and the proper get-key function. With the new i2c behaviour, the bridge driver can also specify the proper i2c address for each device.

Cheers,
Mauro

[1] It doesn't seem to be practical for me to get their internal software.In general, such micro-controllers block EEPROM/ROM read of the software inside. If this is the case of this chip, the only remaining option to get the internal software would be to cut the plastic and try to see the state of each eeprom bit with the help of a good microscope.
Anyway, assuming that there are some way to read the ROM content, in order to see the device behavior, one should remove the chip from the board, get the ROM/EEPROM content, write a disassembler for this processor, disassemble the code and analyse the results. This would be a real hard work, would take a lot of time, and I doubt that this would help to improve the driver, since we already know how to read scan codes from those devices.

Quoted 47 lines · Mauro Carvalho Chehab

Quoted 13 lines · On Tue, 07 Apr 2009 23:02:43 -0400 CityK <[email protected]> wrote:

Regarding the KS003 (& KS007; the other "mystery" chip):

Upon further investigation of some info from a post from last year
(http://www.linuxtv.org/pipermail/linux-dvb/2008-January/022634.html),
it appears that these (assuming that they are the same IC across the various MSI, Leadtek & KWorld cards; and I believe that to be true) are the "AT8PS54/S56" chip from "Feeling Technology" ... the datasheet for that part is available through a google search .... probing further (as I had never heard of FT before and so I looked them up), it looks like FT renamed and/or upgraded the chip to the "FM8PS54/S56" ... the near identical datasheet for that second version is also available:
http://www.feeling-tech.com.tw/km-master/front/bin/ptdetail.phtml?Part=M1-05&Category=100018

From what I've investigated, several of those IR chips are micro-controllers like the one you pointed. I've seen a few boards whose IR chip is not masked. On those, I always went into some micro-controller datasheet.

Those IR's with a micro-controller have some software inside it to decode one IR protocol and generate scan-code sequences that can be received via GPIO or via I2C, depending on the firmware content.

The datasheet of those chips are useless, since the behaviour of the device is programmed inside their ROM/EEPROM [1]. So, even being the same chip, you could have two "K007" devices with different firmwares, listening on different i2c addresses and eventually generating different scan-codes for the same IR.

On the other hand, for USB devices and for bttv, saa7134 and cx88, there are some easy ways to monitor what i2c messages or GPIO pins are involved with IR. In general, the IR received messages generated by the firmware are some header, a scan code, a repeat key bit and a trailer. So, it is not hard to generate a get-key routine to get the scan code and the repeat bit from the protocol.

That's why the modern ir-kbd-i2c approach is to select the proper IR parameters after binding the module, at the bridge driver. The bridge driver is the one who knows what's the IR scan code of the original IR (to set it as the default), and the proper get-key function. With the new i2c behaviour, the bridge driver can also specify the proper i2c address for each device.

Cheers,
Mauro

Quoted 11 lines

[1] It doesn't seem to be practical for me to get their internal software.In general, such micro-controllers block EEPROM/ROM read of the software inside. If this is the case of this chip, the only remaining option to get the internal software would be to cut the plastic and try to see the state of each eeprom bit with the help of a good microscope.
Anyway, assuming that there are some way to read the ROM content, in order to see the device behavior, one should remove the chip from the board, get the ROM/EEPROM content, write a disassembler for this processor, disassemble the code and analyse the results. This would be a real hard work, would take a lot of time, and I doubt that this would help to improve the driver, since we already know how to read scan codes from those devices.

Thanks for the detailed response Mauro. I've actually been wondering about whether the specific "KS00x" designation/label might refer to the embedded firmware or to a dataline, so that thought is certainly consistent with your description.

Hi Jean,

Am Montag, den 06.04.2009, 10:40 +0200 schrieb Jean Delvare:

Quoted 1 line · drunk and tired hermann pitton

don't tell me the French vine is always better. You likely know who introduced that currency once :)

Quoted 20 lines

Quoted 8 lines

Hmm, I'm still "happy" with the broken DVB-T for saa7134 on 2.6.29, tasting some Chianti vine now and need to sleep soon, but I'm also not that confident that your saa7134 MSI TV@nywhere Plus i2c remote does work addressing it directly, since previous reports always said it becomes only visible at all after other devices are probed previously.

Unfortunately I can't test it, but will try to reach some with such hardware and ask for testing, likely not on the list currently.

Thanks for the heads up. I was curious about this as well. The original comment said that the MSI TV@nywhere Plus IR receiver would not respond to _probes_ before another device on the I2C bus was accessed. I didn't know for sure if this only applied to the probe sequence or to any attempt to access the IR receiver. As we no longer need to probe for the device, I thought it may be OK to remove the extra code. But probably the removal of the extra code should be delayed until we find one tester to confirm the exact behavior. Here, done.

Anyone out there with a MSI TV@nywhere Plus that could help with testing?

Here is a link to one of the initial reports by Henry, others are close to it.

http://marc.info/?l=linux-video&m=113324147429459&w=2

There are two different variants of that MSI card, but that undocumented KS003 chip is the same on them.

We still have lots of for the remote unsupported cards with KS chips, many from Kworld. Some of these chips are also seen on cx88xx cards already and other drivers may follow.

Henry doesn't have this card anymore, but maybe Mark and Brian can test and Oldrich might give feedback for the Avermedia.

Cheers,
Hermann

I've tried your patches with AverMedia Cardbus Hybrid (E506R) and they works fine.

My current experience with AverMedia's IR chip (I don't know which one is on the card) is that I2C probing didn't find anything, but it got the chip into some strange state - next operation failed (so that the autodetection on address 0x40 and "subaddress" 0x0b/0x0d failed).

The chip at address 0x40 needs the write first (one byte: 0x0b or 0x0d) and immediate read, otherwise it would not respond. The saa7134's I2C 0xfd quirk (actually I would call it a hack :-)) caused failures in communication with the IR chip.

The way I'm doing the IR reading is the same as the Windows driver does - I got the information through the Qemu with pci-proxy patch applied.

Quoted 12 lines · Andy Walls

Quoted 3 lines · Janne Grunau

I would guess that it won't work. There is an effort to merge lirc. It's currently stalled though.

Perhaps you and Jarrod and Christopher have already discussed this, but...

Instead of trying to push all of the LIRC kernel components through in one big patch set, perhaps it would be easier to just get the lirc_dev and any other needed infrastructure components in first.

Quoted 4 lines

If one focuses on satisfying the LKML comments to lirc_dev and the Makefile to get that kernel module in the kernel, then, at least for video card hosted IR devices, there is an infrastructure to which to hook new or rewritten i2c IR driver modules.

I guess lkml would NAK patches adding infrastructure only bits but we will probably for the next patchset concentrate on a few lirc drivers.

Christopher doesn't participate in the merge attempt.

Quoted 10 lines

Quoted 9 lines

A git tree is available at

git://git.wilsonet.com/linux-2.6-lirc.git

Jared Wilson and I were working on it (mainly last september). Since the IR on the HD PVR is also driven by the same zilog chip as on other hauppauge devices I'll take of lirc_zilog. Help converting the i2c drivers to the new i2c model is welcome. General cleanup of lirc to make it ready for mainline is of course wellcome too.

Quoted 3 lines

I can help with this. I'm mainly concerned with lirc_dev, lirc_i2c (for Rx only use of the zilog at 0x71), lirc_zilog, and lirc_mceusb2. That's because, of course, I have devices that use those modules. :)

I have devices for lirc_zilog (which should probably be merged with lirc_i2c) and lirc serial. Jarod has at least mce usb and imon devices. That are probably the devices we'll concentrate on the next submission.

Quoted 3 lines

lirc_dev and the API header would be my first priority, if you need help. Did anyone consolidate all the comments from the LKML on Jarrod's patch submission?

no and I lost track which comments were already handled.

Let's keep it simple: add a 'load_ir_kbd_i2c' module option for those drivers that did not autoload this module. The driver author can refine things later (I'll definitely will do that for ivtv).

It will be interesting if someone can find out whether lirc will work at all once autoprobing is removed from i2c. If it isn't, then perhaps that will wake them up to the realization that they really need to move to the kernel.

The new mechanism is the right way to do it: the adapter driver has all the information if, where and what IR is used and so should be the one to tell the kernel what to do. Attempting to autodetect and magically figure out what IR might be there is awkward and probably impossible to get right 100%.

Hell, it's wrong already: if you have another board that already loads ir-kbd-i2c then if you load ivtv or pvrusb2 afterwards you get ir-kbd-i2c whether you like it or not, because ir-kbd-i2c will connect to your i2c adapter like a leech. So with the addition of a module option you at least give back control of this to the user.

When this initial conversion is done I'm pretty sure we can improve ir-kbd-i2c to make it easier to let the adapter driver tell it what to do. So we don't need those horrible adapter ID tests and other magic that's going on in that driver. But that's phase two.

My impression (at least for pvrusb2-driven devices) is that the later IR receivers require a completely different driver to work properly; one can't just bolt additional features into ir-kbd-i2c for this.

In lirc's case, a different driver is in fact used. But you know this already.

I haven't looked at ir-kbd-i2c in a while, but one other thing I seriously did not like about it was that the mapping of IR codes to key events was effectively hardcoded in the driver. That makes it difficult to make the same driver work with different kinds of remotes. Even if the bridge driver (e.g. pvrusb2) were to supply such a map, that's still wrong because it's within the realm of possibility that the user might actually want to use a different remote transmitter (provided it is compatible enough to work with the receiver hardware).

The lirc architecture solves this easily via a conf file in userspace. In fact, lirc can map multiple mappings to a single receiver, permitting it to work concurrently with more than one remote. But is such a thing even possible with ir-kbd-i2c? I know this is one reason people tend to choose lirc.

Hi all,

Here finally comes my conversion of ir-kbd-i2c to the new i2c binding model. I've split it into 6 pieces for easier review. Firstly there are 2 preliminary patches:

media-video-01-cx18-fix-i2c-error-handling.patch media-video-02-ir-kbd-i2c-dont-abuse-client-name.patch

Then 2 patches doing the actual conversion:

media-video-03-ir-kbd-i2c-convert-to-new-style.patch media-video-04-configure-ir-receiver.patch

And lastly 2 patches cleaning up saa7134-input thanks to the new possibilities offered by the conversion:

media-video-05-saa7134-input-cleanup-msi-ir.patch media-video-06-saa7134-input-cleanup-avermedia-cardbus.patch

This patch set is against the v4l-dvb repository, but I didn't pay attention to the compatibility issues. I simply build-tested it on 2.6.27 and 2.6.29.

This patch set touches many different drivers and I can't test any of them. My only TV card with an IR receiver doesn't make use of ir-kbd-i2c. So I would warmly welcome testers. The more testing my changes can get, the better.

And of course I welcome reviews and comments as well. I had to touch many drivers I don't know anything about so it is possible that I missed something.

I'll post all 6 patches as replies to this post. They can also be temporarily downloaded from:
http://jdelvare.pck.nerim.net/linux/ir-kbd-i2c/
Additionally I've put a combined patch there, to make testing easier:
http://jdelvare.pck.nerim.net/linux/ir-kbd-i2c/ir-kbd-i2c-conversion-ALL-IN-ONE.patch
But for review the individual patches are much better.

Thanks,

Hi all,

Here finally comes my conversion of ir-kbd-i2c to the new i2c binding model. I've split it into 6 pieces for easier review. Firstly there are 2 preliminary patches:

media-video-01-cx18-fix-i2c-error-handling.patch media-video-02-ir-kbd-i2c-dont-abuse-client-name.patch

Then 2 patches doing the actual conversion:

media-video-03-ir-kbd-i2c-convert-to-new-style.patch media-video-04-configure-ir-receiver.patch

And lastly 2 patches cleaning up saa7134-input thanks to the new possibilities offered by the conversion:

media-video-05-saa7134-input-cleanup-msi-ir.patch media-video-06-saa7134-input-cleanup-avermedia-cardbus.patch

This patch set is against the v4l-dvb repository, but I didn't pay attention to the compatibility issues. I simply build-tested it on 2.6.27 and 2.6.29.

This patch set touches many different drivers and I can't test any of them. My only TV card with an IR receiver doesn't make use of ir-kbd-i2c. So I would warmly welcome testers. The more testing my changes can get, the better.

And of course I welcome reviews and comments as well. I had to touch many drivers I don't know anything about so it is possible that I missed something.

I'll post all 6 patches as replies to this post. They can also be temporarily downloaded from:
http://jdelvare.pck.nerim.net/linux/ir-kbd-i2c/
Additionally I've put a combined patch there, to make testing easier:
http://jdelvare.pck.nerim.net/linux/ir-kbd-i2c/ir-kbd-i2c-conversion-ALL-IN-ONE.patch
But for review the individual patches are much better.

Thanks,

From the discussions we already have, I noticed some points to take care of:

From the discussions we already have, I noticed some points to take care of:

1) about the lirc support, I don't think we should change a kernel driver due to an out-of-tree kernel driver. As I've commented on PATCH 3/6 discussion, we need to better address this with lirc developers;

2) the way Mike is proposing to solve the issue with pvrusb2 will break userspace usage for people that have those devices whose IR work with the in-kernel IR i2c driver. This means that we'll cause a kernel regression due to an out-of-tree driver;

3) So far, nobody gave us any positive return that the new IR model is working with any of the touched drivers. So, more tests are needed. I'm expecting to have a positive reply for each of the touched drivers. People, please test!

Since the merge window is almost finished, IMO, we should postpone those changes to 2.6.31, (...)

to better address the lirc issue and to give people more
time for testing, applying the changesets after the end of the merge window at the v4l/dvb development tree. This will help people to test, review and propose changes if needed.

Hi Mauro,

Quoted 2 lines · Mauro Carvalho Chehab

From the discussions we already have, I noticed some points to take care of:

Quoted 3 lines

1) about the lirc support, I don't think we should change a kernel driver due to an out-of-tree kernel driver. As I've commented on PATCH 3/6 discussion, we need to better address this with lirc developers;

Well, the new binding model makes it harder for "rogue" drivers such as lirc_i2c. They will need _some_ form of cooperation from us, which will most likely come when they get merged into the kernel tree.

Quoted 4 lines

2) the way Mike is proposing to solve the issue with pvrusb2 will break userspace usage for people that have those devices whose IR work with the in-kernel IR i2c driver. This means that we'll cause a kernel regression due to an out-of-tree driver;

Quoted 4 lines

3) So far, nobody gave us any positive return that the new IR model is working with any of the touched drivers. So, more tests are needed. I'm expecting to have a positive reply for each of the touched drivers. People, please test!

Yes, please! :)

Quoted 2 lines

Since the merge window is almost finished, IMO, we should postpone those changes to 2.6.31, (...)

The legacy i2c model will be gone in 2.6.30. Really. Hans and myself have put enough energy into this to not let it slip for just a miserable infrared support module which I understand is hardly used by anyone.

So it's really up to you, either you accept my ir-kbd-i2c conversion "now" (that is, when it has received the minimum testing and reviewing it deserves) and ir-kbd-i2c has a chance to work in 2.6.30, or you don't and I'll just have to mark ir-kbd-i2c as BROKEN to prevent build failures.

Quoted 5 lines

to better address the lirc issue and to give people more
time for testing, applying the changesets after the end of the merge window at the v4l/dvb development tree. This will help people to test, review and propose changes if needed.

These changes are on-going for over a year now. If the lirc people didn't hear about it so far, I doubt they will pay more attention just because we delay the deadline by 2 months. The only thing that will get their attention is when lirc_i2c break. So let's just do that ;)

Don't get me wrong. I don't want to be (too) rude to lirc developers. If they need help to port their code to the new i2c binding model, I'll help them. If they need help to merge lirc_i2c into the kernel, I'll help as well. But I don't see any point in delaying important, long awaited kernel changes just for them. As long as they are out-of-tree, they can fix things after the fact easily. They aren't affected by the merge window. They'll have several weeks before kernel 2.6.30 is actually released, which they can use to fix anything that broke.

Hi Mauro,

Quoted 4 lines · Mauro Carvalho Chehab

3) So far, nobody gave us any positive return that the new IR model is working with any of the touched drivers. So, more tests are needed. I'm expecting to have a positive reply for each of the touched drivers. People, please test!

Yes, please! :)

I tested your original patch set so you can get some real feedback. The news is good, but I'll bore you with the details first. :)

1. My setup:

HVR-1600: Hauppauge model 74041, rev C5B2, serial# 891351
has no radio, has IR receiver, has IR transmitter (Zilog Z8F0811)

Hauppauge Remote: Grey top side & black bottom side;
with 4 colored buttons Red, Green, Yellow, Blue; Numbers inside battery cover:

	A415-HPG
	M25909070211590

uname -a:
Linux morgan.walls.org 2.6.27.9-73.fc9.x86_64 #1 SMP Tue Dec 16 14:54:03 EST 2008 x86_64 x86_64 x86_64 GNU/Linux

v4l-dvb: pulled this morning.

2. OK, my first test had no effect, because I'm an idiot. :) In the cx18 driver, I didn't remove the I2C addresses in your original patch, and I didn't add 0x71 as a valid address.

3. My second test had bad results:

# modprobe ir-kbd-i2c debug=3 hauppauge=1

Message from syslogd@morgan at Apr 5 18:47:14 ... kernel:Oops: 0010 [1] SMP

Message from syslogd@morgan at Apr 5 18:47:14 ... kernel:Code: Bad RIP value.

Message from syslogd@morgan at Apr 5 18:47:14 ... kernel:CR2: 0000000000000000

input: i2c IR (SAA713x remote) as /devices/virtual/input/input6
ir-kbd-i2c: i2c IR (SAA713x remote) detected at i2c-1/1-0071/ir0 [cx18 i2c driver #0-0]
ir-kbd-i2c: ir_poll_key

BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
IP: [<0000000000000000>] 0x0
PGD 71c36067 PUD 2cca2067 PMD 0 
Oops: 0010 [1] SMP 
CPU 0 
Modules linked in: ir_kbd_i2c(+) ir_common mxl5005s s5h1409 tuner_simple tuner_types cs5345 tuner cx18 dvb_core cx2341x v4l2_common videodev v4l1_compat v4l2_compat_ioctl32 tveeprom i2c_algo_bit fuse ipt_REJECT nf_conntrack_ipv4 iptable_filter ip_tables ip6t_REJECT xt_tcpudp nf_conntrack_ipv6 xt_state nf_conntrack ip6table_filter ip6_tables x_tables cpufreq_ondemand powernow_k8 freq_table loop dm_multipath scsi_dh ipv6 snd_hda_intel snd_seq_dummy snd_seq_oss snd_seq_midi_event snd_seq snd_seq_device snd_pcm_oss snd_mixer_oss snd_pcm i2c_piix4 snd_timer snd_page_alloc ppdev parport_pc snd_hwdep i2c_core shpchp parport pcspkr snd soundcore sr_mod r8169 mii k8temp hwmon cdrom sg floppy dm_snapshot dm_zero dm_mirror dm_log dm_mod pata_acpi ata_generic pata_atiixp libata sd_mod scsi_mod crc_t10dif ext3 jbd mbcache uhci_hcd ohci_hcd ehci_hcd [last unloaded: tveeprom]
Pid: 9, comm: events/0 Not tainted 2.6.27.9-73.fc9.x86_64 #1
RIP: 0010:[<0000000000000000>]  [<0000000000000000>] 0x0
RSP: 0018:ffff880077bede58  EFLAGS: 00010286
RAX: 000000000000001b RBX: ffff88005e73ee30 RCX: 000000000000ccc6
RDX: ffffffffa031ba00 RSI: ffffffffa031ba04 RDI: ffff88005e73ec00
RBP: ffff880077bede80 R08: ffff880077bec000 R09: 000000000000ccc6
R10: 00000126a7e22b3d R11: ffff880073159bd8 R12: ffff88005e73ec00
R13: 0000000000000064 R14: ffffffffa0318371 R15: ffff880077804908
FS:  00007fa0409126f0(0000) GS:ffffffff814ad100(0000) knlGS:0000000000000000
CS:  0010 DS: 0018 ES: 0018 CR0: 000000008005003b
CR2: 0000000000000000 CR3: 00000000569f6000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process events/0 (pid: 9, threadinfo ffff880077bec000, task ffff880077bbdb80)
Stack:  ffffffffa03183df ffff880077bede80 ffff88005e73ee38 ffff880077804900
 ffff88005e73ee30 ffff880077bedec0 ffffffff8104fe45 ffff880077bedec0
 ffff880077804900 ffff880077804918 ffff880077804908 ffff880077beded0
Call Trace:
 [<ffffffffa03183df>] ? ir_work+0x6e/0xd2 [ir_kbd_i2c]
 [<ffffffff8104fe45>] run_workqueue+0xa3/0x146
 [<ffffffff8104ffdd>] worker_thread+0xf5/0x109
 [<ffffffff81053741>] ? autoremove_wake_function+0x0/0x38
 [<ffffffff8104fee8>] ? worker_thread+0x0/0x109
 [<ffffffff810533d7>] kthread+0x49/0x76
 [<ffffffff810116e9>] child_rip+0xa/0x11
 [<ffffffff81010a07>] ? restore_args+0x0/0x30
 [<ffffffff8105338e>] ? kthread+0x0/0x76
 [<ffffffff810116df>] ? child_rip+0x0/0x11


Code:  Bad RIP value.
RIP  [<0000000000000000>] 0x0
 RSP <ffff880077bede58>
CR2: 0000000000000000
---[ end trace 6076b08e85151d70 ]---

That's because in ir-kbd-i2c.c:ir_poll_key(), ir->get_key() was a NULL function pointer.

I realized that ir-kbd-i2c.c:ir_probe() would need a fix-up for address 0x71 for cx18 (and ivtv) or I would need to set the parameters via struct IR_i2c_init_data.

There's a usable get_key function for Hauppauge remotes in ir-kbd-i2c, but no way via struct IR_i2c_init_data to specify that one wants to use it from the bridge driver. Nor is there a way to set the RC5 ir_type from the bridge driver. So mucking with ir-kbd-i2c.c for address 0x71 is what I did next.

4. For my third test I modified a few lines in ir-kbd-i2c:ir_probe.c:

	...
        case 0x7a:
        case 0x47:
        case 0x71:
        case 0x2d:
                if (adap->id == I2C_HW_B_CX2388x ||
                    adap->id == I2C_HW_B_CX2341X   ) {
                        /* Handled by cx88-input */
                        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) {
	...

# modprobe ir-kbd-i2c debug=1 hauppauge=1
(pressing number buttons on the remote)
# 1345678920
# dmesg
input: i2c IR (CX2341X remote) as /devices/virtual/input/input15
ir-kbd-i2c: i2c IR (CX2341X remote) detected at i2c-1/1-0071/ir0 [cx18 i2c driver #0-0]
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t1 dev=30 code=1
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t1 dev=30 code=1
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t1 dev=30 code=3
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t1 dev=30 code=3
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t0 dev=30 code=4
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t0 dev=30 code=4
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t1 dev=30 code=5
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t0 dev=30 code=6
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t1 dev=30 code=7
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t1 dev=30 code=7
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t0 dev=30 code=8
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t0 dev=30 code=8
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t1 dev=30 code=9
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t1 dev=30 code=2
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t1 dev=30 code=2
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t0 dev=30 code=0

Success.

Hi Andy,

Note that struct IR_i2c_init_data only contains the fields I needed at the moment, but it would be trivial to extend it to allow bridge drivers to pass more setup information if needed, for example ir_type.

Quoted 1 line

Success.

OK, good to know that adding support for the cx18 will be possible and easy. I propose that we postpone this addition until after my code is merged though, to avoid making the situation more complex than it already is.

Thanks a lot for the testing!

Quoted 1 line · Jean Delvare

Thanks a lot for the testing!

You're welcome.

Sorry for being such a pain to what I suspect you hoped was to be a "simple" change.