[PATCH 0/6] ir-kbd-i2c conversion to the new i2c binding model

[PATCH 0/6] ir-kbd-i2c conversion to the new i2c binding model

[Jean Delvare]

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,
-- 
Jean Delvare

[PATCH 1/6] cx18: Fix the handling of i2c bus registration error

[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 <khali@linux-fr.org>
Cc: Hans Verkuil <hverkuil@xs4all.nl>
Cc: Andy Walls <awalls@radix.net>
---
 linux/drivers/media/video/cx18/cx18-i2c.c |   16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

--- v4l-dvb.orig/linux/drivers/media/video/cx18/cx18-i2c.c	2009-03-01 16:09:09.000000000 +0100
+++ v4l-dvb/linux/drivers/media/video/cx18/cx18-i2c.c	2009-04-03 18:45:18.000000000 +0200
@@ -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;
 }
 
 void exit_cx18_i2c(struct cx18 *cx)

-- 
Jean Delvare

Re: [PATCH 1/6] cx18: Fix the handling of i2c bus registration error

[Andy Walls]

On Sat, 2009-04-04 at 14:26 +0200, Jean Delvare wrote:
> * 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 <khali@linux-fr.org>
> Cc: Hans Verkuil <hverkuil@xs4all.nl>
> Cc: Andy Walls <awalls@radix.net>

Jean,

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


> ---
>  linux/drivers/media/video/cx18/cx18-i2c.c |   16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> --- v4l-dvb.orig/linux/drivers/media/video/cx18/cx18-i2c.c	2009-03-01 16:09:09.000000000 +0100
> +++ v4l-dvb/linux/drivers/media/video/cx18/cx18-i2c.c	2009-04-03 18:45:18.000000000 +0200
> @@ -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)
		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.


> +	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;
>  }
>  
>  void exit_cx18_i2c(struct cx18 *cx)

Reviewed-by: Andy Walls <awalls@radix.net>

Regards,
Andy

Re: [PATCH 1/6] cx18: Fix the handling of i2c bus registration error

[Jean Delvare]

Hi Andy,

Thanks for the fast review.

On Sat, 04 Apr 2009 08:46:00 -0400, Andy Walls wrote:
> On Sat, 2009-04-04 at 14:26 +0200, Jean Delvare wrote:
> > * 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 <khali@linux-fr.org>
> > Cc: Hans Verkuil <hverkuil@xs4all.nl>
> > Cc: Andy Walls <awalls@radix.net>
> 
> Jean,
> 
> Thanks for noticing this one and providing a patch.  I have one comment
> below...
> 
> 
> > ---
> >  linux/drivers/media/video/cx18/cx18-i2c.c |   16 +++++++++++++---
> >  1 file changed, 13 insertions(+), 3 deletions(-)
> > 
> > --- v4l-dvb.orig/linux/drivers/media/video/cx18/cx18-i2c.c	2009-03-01 16:09:09.000000000 +0100
> > +++ v4l-dvb/linux/drivers/media/video/cx18/cx18-i2c.c	2009-04-03 18:45:18.000000000 +0200
> > @@ -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)
> 		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.

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.

> > +	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;
> >  }
> >  
> >  void exit_cx18_i2c(struct cx18 *cx)
> 
> Reviewed-by: Andy Walls <awalls@radix.net>

Thanks,
-- 
Jean Delvare

Re: [PATCH 1/6] cx18: Fix the handling of i2c bus registration error

[Andy Walls]

On Sat, 2009-04-04 at 16:23 +0200, Jean Delvare wrote:
> 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.

Meh, whichever way you like is fine for now.  If I really decide to care
about it, I'll muck with it when I get the hardware I2C masters working.
I'll have to touch that section of code at that time anyway.

Acked-by: Andy Walls <awalls@radix.net>

Regards,
Andy

Re: [PATCH 1/6] cx18: Fix the handling of i2c bus registration error

[Jean Delvare]

On Sat, 04 Apr 2009 08:46:00 -0400, Andy Walls wrote:
> On Sat, 2009-04-04 at 14:26 +0200, Jean Delvare wrote:
> > * 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 <khali@linux-fr.org>
> > Cc: Hans Verkuil <hverkuil@xs4all.nl>
> > Cc: Andy Walls <awalls@radix.net>
> > ---
> >  linux/drivers/media/video/cx18/cx18-i2c.c |   16 +++++++++++++---
> >  1 file changed, 13 insertions(+), 3 deletions(-)
> > 
> > --- v4l-dvb.orig/linux/drivers/media/video/cx18/cx18-i2c.c	2009-03-01 16:09:09.000000000 +0100
> > +++ v4l-dvb/linux/drivers/media/video/cx18/cx18-i2c.c	2009-04-03 18:45:18.000000000 +0200
> > @@ -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;
> >  }
> >  
> >  void exit_cx18_i2c(struct cx18 *cx)
> 
> Reviewed-by: Andy Walls <awalls@radix.net>

[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,
-- 
Jean Delvare

Re: [PATCH 1/6] cx18: Fix the handling of i2c bus registration error

[Andy Walls]

On Tue, 2009-04-07 at 11:31 +0200, Jean Delvare wrote:
> On Sat, 04 Apr 2009 08:46:00 -0400, Andy Walls wrote:
> > On Sat, 2009-04-04 at 14:26 +0200, Jean Delvare wrote:
> > > * 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 <khali@linux-fr.org>
> > > Cc: Hans Verkuil <hverkuil@xs4all.nl>
> > > Cc: Andy Walls <awalls@radix.net>
> > > ---
> > >  linux/drivers/media/video/cx18/cx18-i2c.c |   16 +++++++++++++---
> > >  1 file changed, 13 insertions(+), 3 deletions(-)
> > > 
> > > --- v4l-dvb.orig/linux/drivers/media/video/cx18/cx18-i2c.c	2009-03-01 16:09:09.000000000 +0100
> > > +++ v4l-dvb/linux/drivers/media/video/cx18/cx18-i2c.c	2009-04-03 18:45:18.000000000 +0200
> > > @@ -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;
> > >  }
> > >  
> > >  void exit_cx18_i2c(struct cx18 *cx)
> > 
> > Reviewed-by: Andy Walls <awalls@radix.net>

If it matters:

Acked-by: Andy Walls <awalls@radix.net>

Regards,
Andy

> [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,

[PATCH 2/6] ir-kbd-i2c: Don't use i2c_client.name for our own needs

[Jean Delvare]

In the standard device driver binding model, the name field of
struct i2c_client is used to match devices to their drivers, so we
must stop using it for internal purposes. Define a separate field
in struct IR_i2c as a replacement, and use it.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
 linux/drivers/media/video/cx231xx/cx231xx-input.c |    2 +-
 linux/drivers/media/video/em28xx/em28xx-cards.c   |    6 +++---
 linux/drivers/media/video/em28xx/em28xx-input.c   |    2 +-
 linux/drivers/media/video/ir-kbd-i2c.c            |    5 +++--
 linux/drivers/media/video/saa7134/saa7134-input.c |   12 ++++++------
 linux/include/media/ir-kbd-i2c.h                  |    1 +
 6 files changed, 15 insertions(+), 13 deletions(-)

--- v4l-dvb.orig/linux/drivers/media/video/cx231xx/cx231xx-input.c	2009-03-13 09:59:49.000000000 +0100
+++ v4l-dvb/linux/drivers/media/video/cx231xx/cx231xx-input.c	2009-04-03 19:02:28.000000000 +0200
@@ -37,7 +37,7 @@ MODULE_PARM_DESC(ir_debug, "enable debug
 
 #define i2cdprintk(fmt, arg...) \
 	if (ir_debug) { \
-		printk(KERN_DEBUG "%s/ir: " fmt, ir->c.name , ## arg); \
+		printk(KERN_DEBUG "%s/ir: " fmt, ir->name , ## arg); \
 	}
 
 #define dprintk(fmt, arg...) \
--- v4l-dvb.orig/linux/drivers/media/video/em28xx/em28xx-cards.c	2009-04-03 14:18:26.000000000 +0200
+++ v4l-dvb/linux/drivers/media/video/em28xx/em28xx-cards.c	2009-04-03 18:56:40.000000000 +0200
@@ -1921,19 +1921,19 @@ void em28xx_set_ir(struct em28xx *dev, s
 	case (EM2820_BOARD_TERRATEC_CINERGY_250):
 		ir->ir_codes = ir_codes_em_terratec;
 		ir->get_key = em28xx_get_key_terratec;
-		snprintf(ir->c.name, sizeof(ir->c.name),
+		snprintf(ir->name, sizeof(ir->name),
 			 "i2c IR (EM28XX Terratec)");
 		break;
 	case (EM2820_BOARD_PINNACLE_USB_2):
 		ir->ir_codes = ir_codes_pinnacle_grey;
 		ir->get_key = em28xx_get_key_pinnacle_usb_grey;
-		snprintf(ir->c.name, sizeof(ir->c.name),
+		snprintf(ir->name, sizeof(ir->name),
 			 "i2c IR (EM28XX Pinnacle PCTV)");
 		break;
 	case (EM2820_BOARD_HAUPPAUGE_WINTV_USB_2):
 		ir->ir_codes = ir_codes_hauppauge_new;
 		ir->get_key = em28xx_get_key_em_haup;
-		snprintf(ir->c.name, sizeof(ir->c.name),
+		snprintf(ir->name, sizeof(ir->name),
 			 "i2c IR (EM2840 Hauppauge)");
 		break;
 	case (EM2820_BOARD_MSI_VOX_USB_2):
--- v4l-dvb.orig/linux/drivers/media/video/em28xx/em28xx-input.c	2009-03-13 09:59:49.000000000 +0100
+++ v4l-dvb/linux/drivers/media/video/em28xx/em28xx-input.c	2009-04-03 18:56:40.000000000 +0200
@@ -41,7 +41,7 @@ MODULE_PARM_DESC(ir_debug, "enable debug
 
 #define i2cdprintk(fmt, arg...) \
 	if (ir_debug) { \
-		printk(KERN_DEBUG "%s/ir: " fmt, ir->c.name , ## arg); \
+		printk(KERN_DEBUG "%s/ir: " fmt, ir->name , ## arg); \
 	}
 
 #define dprintk(fmt, arg...) \
--- v4l-dvb.orig/linux/drivers/media/video/ir-kbd-i2c.c	2009-03-13 09:59:49.000000000 +0100
+++ v4l-dvb/linux/drivers/media/video/ir-kbd-i2c.c	2009-04-03 18:56:40.000000000 +0200
@@ -346,6 +346,7 @@ static int ir_attach(struct i2c_adapter
 
 	ir->c.adapter = adap;
 	ir->c.addr    = addr;
+	snprintf(ir->c.name, sizeof(ir->c.name), "ir-kbd");
 
 	i2c_set_clientdata(&ir->c, ir);
 
@@ -419,7 +420,7 @@ static int ir_attach(struct i2c_adapter
 	}
 
 	/* Sets name */
-	snprintf(ir->c.name, sizeof(ir->c.name), "i2c IR (%s)", name);
+	snprintf(ir->name, sizeof(ir->name), "i2c IR (%s)", name);
 	ir->ir_codes = ir_codes;
 
 	/* register i2c device
@@ -444,7 +445,7 @@ static int ir_attach(struct i2c_adapter
 	/* init + register input device */
 	ir_input_init(input_dev, &ir->ir, ir_type, ir->ir_codes);
 	input_dev->id.bustype = BUS_I2C;
-	input_dev->name       = ir->c.name;
+	input_dev->name       = ir->name;
 	input_dev->phys       = ir->phys;
 
 	err = input_register_device(ir->input);
--- v4l-dvb.orig/linux/drivers/media/video/saa7134/saa7134-input.c	2009-03-01 16:09:10.000000000 +0100
+++ v4l-dvb/linux/drivers/media/video/saa7134/saa7134-input.c	2009-04-03 18:56:40.000000000 +0200
@@ -60,7 +60,7 @@ MODULE_PARM_DESC(disable_other_ir, "disa
 #define dprintk(fmt, arg...)	if (ir_debug) \
 	printk(KERN_DEBUG "%s/ir: " fmt, dev->name , ## arg)
 #define i2cdprintk(fmt, arg...)    if (ir_debug) \
-	printk(KERN_DEBUG "%s/ir: " fmt, ir->c.name , ## arg)
+	printk(KERN_DEBUG "%s/ir: " fmt, ir->name , ## arg)
 
 /* Helper functions for RC5 and NEC decoding at GPIO16 or GPIO18 */
 static int saa7134_rc5_irq(struct saa7134_dev *dev);
@@ -693,7 +693,7 @@ void saa7134_set_i2c_ir(struct saa7134_d
 	switch (dev->board) {
 	case SAA7134_BOARD_PINNACLE_PCTV_110i:
 	case SAA7134_BOARD_PINNACLE_PCTV_310i:
-		snprintf(ir->c.name, sizeof(ir->c.name), "Pinnacle PCTV");
+		snprintf(ir->name, sizeof(ir->name), "Pinnacle PCTV");
 		if (pinnacle_remote == 0) {
 			ir->get_key   = get_key_pinnacle_color;
 			ir->ir_codes = ir_codes_pinnacle_color;
@@ -703,17 +703,17 @@ void saa7134_set_i2c_ir(struct saa7134_d
 		}
 		break;
 	case SAA7134_BOARD_UPMOST_PURPLE_TV:
-		snprintf(ir->c.name, sizeof(ir->c.name), "Purple TV");
+		snprintf(ir->name, sizeof(ir->name), "Purple TV");
 		ir->get_key   = get_key_purpletv;
 		ir->ir_codes  = ir_codes_purpletv;
 		break;
 	case SAA7134_BOARD_MSI_TVATANYWHERE_PLUS:
-		snprintf(ir->c.name, sizeof(ir->c.name), "MSI TV@nywhere Plus");
+		snprintf(ir->name, sizeof(ir->name), "MSI TV@nywhere Plus");
 		ir->get_key  = get_key_msi_tvanywhere_plus;
 		ir->ir_codes = ir_codes_msi_tvanywhere_plus;
 		break;
 	case SAA7134_BOARD_HAUPPAUGE_HVR1110:
-		snprintf(ir->c.name, sizeof(ir->c.name), "HVR 1110");
+		snprintf(ir->name, sizeof(ir->name), "HVR 1110");
 		ir->get_key   = get_key_hvr1110;
 		ir->ir_codes  = ir_codes_hauppauge_new;
 		break;
@@ -722,7 +722,7 @@ void saa7134_set_i2c_ir(struct saa7134_d
 	case SAA7134_BOARD_BEHOLD_M63:
 	case SAA7134_BOARD_BEHOLD_M6_EXTRA:
 	case SAA7134_BOARD_BEHOLD_H6:
-		snprintf(ir->c.name, sizeof(ir->c.name), "BeholdTV");
+		snprintf(ir->name, sizeof(ir->name), "BeholdTV");
 		ir->get_key   = get_key_beholdm6xx;
 		ir->ir_codes  = ir_codes_behold;
 		break;
--- v4l-dvb.orig/linux/include/media/ir-kbd-i2c.h	2009-03-13 09:59:49.000000000 +0100
+++ v4l-dvb/linux/include/media/ir-kbd-i2c.h	2009-04-03 18:56:40.000000000 +0200
@@ -15,6 +15,7 @@ struct IR_i2c {
 	unsigned char          old;
 
 	struct delayed_work    work;
+	char                   name[32];
 	char                   phys[32];
 	int                    (*get_key)(struct IR_i2c*, u32*, u32*);
 };

-- 
Jean Delvare

[PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[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 <khali@linux-fr.org>
Cc: Mauro Carvalho Chehab <mchehab@infradead.org>
Cc: Hans Verkuil <hverkuil@xs4all.nl>
Cc: Andy Walls <awalls@radix.net>
Cc: Mike Isely <isely@pobox.com>
---
 linux/drivers/media/video/bt8xx/bttv-i2c.c           |   21 +
 linux/drivers/media/video/cx18/cx18-i2c.c            |   30 ++
 linux/drivers/media/video/cx231xx/cx231xx-cards.c    |    5 
 linux/drivers/media/video/cx23885/cx23885-i2c.c      |   12 +
 linux/drivers/media/video/cx88/cx88-i2c.c            |   13 +
 linux/drivers/media/video/em28xx/em28xx-cards.c      |   20 +
 linux/drivers/media/video/em28xx/em28xx-i2c.c        |    3 
 linux/drivers/media/video/em28xx/em28xx-input.c      |    6 
 linux/drivers/media/video/em28xx/em28xx.h            |    1 
 linux/drivers/media/video/ir-kbd-i2c.c               |  198 ++----------------
 linux/drivers/media/video/ivtv/ivtv-i2c.c            |   31 ++
 linux/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c |   24 ++
 linux/drivers/media/video/saa7134/saa7134-i2c.c      |    3 
 linux/drivers/media/video/saa7134/saa7134-input.c    |   86 ++++++-
 linux/drivers/media/video/saa7134/saa7134.h          |    1 
 linux/drivers/media/video/usbvision/usbvision-i2c.c  |   24 ++
 linux/include/media/ir-kbd-i2c.h                     |    2 
 17 files changed, 284 insertions(+), 196 deletions(-)

--- v4l-dvb.orig/linux/drivers/media/video/bt8xx/bttv-i2c.c	2009-04-04 10:53:08.000000000 +0200
+++ v4l-dvb/linux/drivers/media/video/bt8xx/bttv-i2c.c	2009-04-04 10:58:36.000000000 +0200
@@ -405,6 +405,27 @@ int __devinit init_bttv_i2c(struct bttv
 	}
 	if (0 == btv->i2c_rc && i2c_scan)
 		do_i2c_scan(btv->c.v4l2_dev.name, &btv->i2c_client);
+
+	/* Instantiate the IR receiver device, if present */
+	if (0 == btv->i2c_rc) {
+		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(&btv->c.i2c_adap, &info, addr_list);
+	}
 	return btv->i2c_rc;
 }
 
--- v4l-dvb.orig/linux/drivers/media/video/cx18/cx18-i2c.c	2009-04-04 10:53:15.000000000 +0200
+++ v4l-dvb/linux/drivers/media/video/cx18/cx18-i2c.c	2009-04-04 10:58:36.000000000 +0200
@@ -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,
+		I2C_CLIENT_END
+	};
+
+	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;
+
+	/* Instantiate the IR receiver device, if present */
+	init_cx18_i2c_ir(cx);
 	return 0;
 
  err_del_bus_0:
--- v4l-dvb.orig/linux/drivers/media/video/cx231xx/cx231xx-cards.c	2009-04-04 10:53:08.000000000 +0200
+++ v4l-dvb/linux/drivers/media/video/cx231xx/cx231xx-cards.c	2009-04-04 12:56:26.000000000 +0200
@@ -284,11 +284,6 @@ static void cx231xx_config_tuner(struct
 /* ----------------------------------------------------------------------- */
 void cx231xx_set_ir(struct cx231xx *dev, struct IR_i2c *ir)
 {
-	if (disable_ir) {
-		ir->get_key = NULL;
-		return;
-	}
-
 	/* detect & configure */
 	switch (dev->model) {
 
--- v4l-dvb.orig/linux/drivers/media/video/cx23885/cx23885-i2c.c	2009-04-04 10:53:08.000000000 +0200
+++ v4l-dvb/linux/drivers/media/video/cx23885/cx23885-i2c.c	2009-04-04 10:58:36.000000000 +0200
@@ -364,6 +364,18 @@ int cx23885_i2c_register(struct cx23885_
 		printk(KERN_WARNING "%s: i2c bus %d register FAILED\n",
 			dev->name, bus->nr);
 
+	/* Instantiate the IR receiver device, if present */
+	if (0 == bus->i2c_rc) {
+		struct i2c_board_info info;
+		const unsigned short addr_list[] = {
+			0x6b, I2C_CLIENT_END
+		};
+
+		memset(&info, 0, sizeof(struct i2c_board_info));
+		strlcpy(info.type, "ir-kbd", I2C_NAME_SIZE);
+		i2c_new_probed_device(&bus->i2c_adap, &info, addr_list);
+	}
+
 	return bus->i2c_rc;
 }
 
--- v4l-dvb.orig/linux/drivers/media/video/cx88/cx88-i2c.c	2009-04-04 10:53:08.000000000 +0200
+++ v4l-dvb/linux/drivers/media/video/cx88/cx88-i2c.c	2009-04-04 10:58:36.000000000 +0200
@@ -186,6 +186,19 @@ int cx88_i2c_init(struct cx88_core *core
 			do_i2c_scan(core->name,&core->i2c_client);
 	} else
 		printk("%s: i2c register FAILED\n", core->name);
+
+	/* Instantiate the IR receiver device, if present */
+	if (0 == core->i2c_rc) {
+		struct i2c_board_info info;
+		const unsigned short addr_list[] = {
+			0x18, 0x6b, 0x71,
+			I2C_CLIENT_END
+		};
+
+		memset(&info, 0, sizeof(struct i2c_board_info));
+		strlcpy(info.type, "ir-kbd", I2C_NAME_SIZE);
+		i2c_new_probed_device(&core->i2c_adap, &info, addr_list);
+	}
 	return core->i2c_rc;
 }
 
--- v4l-dvb.orig/linux/drivers/media/video/em28xx/em28xx-cards.c	2009-04-04 10:57:57.000000000 +0200
+++ v4l-dvb/linux/drivers/media/video/em28xx/em28xx-cards.c	2009-04-04 12:56:26.000000000 +0200
@@ -1904,13 +1904,23 @@ static int em28xx_hint_board(struct em28
 }
 
 /* ----------------------------------------------------------------------- */
-void em28xx_set_ir(struct em28xx *dev, struct IR_i2c *ir)
+void em28xx_register_i2c_ir(struct em28xx *dev)
 {
-	if (disable_ir) {
-		ir->get_key = NULL;
-		return ;
-	}
+	struct i2c_board_info info;
+	const unsigned short addr_list[] = {
+		 0x30, 0x47, I2C_CLIENT_END
+	};
+
+	if (disable_ir)
+		return;
+
+	memset(&info, 0, sizeof(struct i2c_board_info));
+	strlcpy(info.type, "ir-kbd", I2C_NAME_SIZE);
+	i2c_new_probed_device(&dev->i2c_adap, &info, addr_list);
+}
 
+void em28xx_set_ir(struct em28xx *dev, struct IR_i2c *ir)
+{
 	/* detect & configure */
 	switch (dev->model) {
 	case (EM2800_BOARD_UNKNOWN):
--- v4l-dvb.orig/linux/drivers/media/video/em28xx/em28xx-i2c.c	2009-04-04 10:53:08.000000000 +0200
+++ v4l-dvb/linux/drivers/media/video/em28xx/em28xx-i2c.c	2009-04-04 12:56:26.000000000 +0200
@@ -581,6 +581,9 @@ int em28xx_i2c_register(struct em28xx *d
 	if (i2c_scan)
 		em28xx_do_i2c_scan(dev);
 
+	/* Instantiate the IR receiver device, if present */
+	em28xx_register_i2c_ir(dev);
+
 	return 0;
 }
 
--- v4l-dvb.orig/linux/drivers/media/video/em28xx/em28xx-input.c	2009-04-04 10:57:57.000000000 +0200
+++ v4l-dvb/linux/drivers/media/video/em28xx/em28xx-input.c	2009-04-04 10:58:36.000000000 +0200
@@ -86,7 +86,7 @@ int em28xx_get_key_terratec(struct IR_i2
 	unsigned char b;
 
 	/* poll IR chip */
-	if (1 != i2c_master_recv(&ir->c, &b, 1)) {
+	if (1 != i2c_master_recv(ir->c, &b, 1)) {
 		i2cdprintk("read error\n");
 		return -EIO;
 	}
@@ -115,7 +115,7 @@ int em28xx_get_key_em_haup(struct IR_i2c
 	unsigned char code;
 
 	/* poll IR chip */
-	if (2 != i2c_master_recv(&ir->c, buf, 2))
+	if (2 != i2c_master_recv(ir->c, buf, 2))
 		return -EIO;
 
 	/* Does eliminate repeated parity code */
@@ -153,7 +153,7 @@ int em28xx_get_key_pinnacle_usb_grey(str
 
 	/* poll IR chip */
 
-	if (3 != i2c_master_recv(&ir->c, buf, 3)) {
+	if (3 != i2c_master_recv(ir->c, buf, 3)) {
 		i2cdprintk("read error\n");
 		return -EIO;
 	}
--- v4l-dvb.orig/linux/drivers/media/video/em28xx/em28xx.h	2009-04-04 10:53:08.000000000 +0200
+++ v4l-dvb/linux/drivers/media/video/em28xx/em28xx.h	2009-04-04 12:56:26.000000000 +0200
@@ -648,6 +648,7 @@ extern void em28xx_card_setup(struct em2
 extern struct em28xx_board em28xx_boards[];
 extern struct usb_device_id em28xx_id_table[];
 extern const unsigned int em28xx_bcount;
+void em28xx_register_i2c_ir(struct em28xx *dev);
 void em28xx_set_ir(struct em28xx *dev, struct IR_i2c *ir);
 int em28xx_tuner_callback(void *ptr, int component, int command, int arg);
 void em28xx_release_resources(struct em28xx *dev);
--- v4l-dvb.orig/linux/drivers/media/video/ir-kbd-i2c.c	2009-04-04 10:57:57.000000000 +0200
+++ v4l-dvb/linux/drivers/media/video/ir-kbd-i2c.c	2009-04-04 12:56:26.000000000 +0200
@@ -75,7 +75,7 @@ static int get_key_haup_common(struct IR
 	int start, range, toggle, dev, code, ircode;
 
 	/* poll IR chip */
-	if (size != i2c_master_recv(&ir->c,buf,size))
+	if (size != i2c_master_recv(ir->c, buf, size))
 		return -EIO;
 
 	/* split rc5 data block ... */
@@ -138,7 +138,7 @@ static int get_key_pixelview(struct IR_i
 	unsigned char b;
 
 	/* poll IR chip */
-	if (1 != i2c_master_recv(&ir->c,&b,1)) {
+	if (1 != i2c_master_recv(ir->c, &b, 1)) {
 		dprintk(1,"read error\n");
 		return -EIO;
 	}
@@ -152,7 +152,7 @@ static int get_key_pv951(struct IR_i2c *
 	unsigned char b;
 
 	/* poll IR chip */
-	if (1 != i2c_master_recv(&ir->c,&b,1)) {
+	if (1 != i2c_master_recv(ir->c, &b, 1)) {
 		dprintk(1,"read error\n");
 		return -EIO;
 	}
@@ -172,7 +172,7 @@ static int get_key_fusionhdtv(struct IR_
 	unsigned char buf[4];
 
 	/* poll IR chip */
-	if (4 != i2c_master_recv(&ir->c,buf,4)) {
+	if (4 != i2c_master_recv(ir->c, buf, 4)) {
 		dprintk(1,"read error\n");
 		return -EIO;
 	}
@@ -196,7 +196,7 @@ static int get_key_knc1(struct IR_i2c *i
 	unsigned char b;
 
 	/* poll IR chip */
-	if (1 != i2c_master_recv(&ir->c,&b,1)) {
+	if (1 != i2c_master_recv(ir->c, &b, 1)) {
 		dprintk(1,"read error\n");
 		return -EIO;
 	}
@@ -223,12 +223,12 @@ static int get_key_avermedia_cardbus(str
 				     u32 *ir_key, u32 *ir_raw)
 {
 	unsigned char subaddr, key, keygroup;
-	struct i2c_msg msg[] = { { .addr = ir->c.addr, .flags = 0,
+	struct i2c_msg msg[] = { { .addr = ir->c->addr, .flags = 0,
 				   .buf = &subaddr, .len = 1},
-				 { .addr = ir->c.addr, .flags = I2C_M_RD,
+				 { .addr = ir->c->addr, .flags = I2C_M_RD,
 				  .buf = &key, .len = 1} };
 	subaddr = 0x0d;
-	if (2 != i2c_transfer(ir->c.adapter, msg, 2)) {
+	if (2 != i2c_transfer(ir->c->adapter, msg, 2)) {
 		dprintk(1, "read error\n");
 		return -EIO;
 	}
@@ -238,7 +238,7 @@ static int get_key_avermedia_cardbus(str
 
 	subaddr = 0x0b;
 	msg[1].buf = &keygroup;
-	if (2 != i2c_transfer(ir->c.adapter, msg, 2)) {
+	if (2 != i2c_transfer(ir->c->adapter, msg, 2)) {
 		dprintk(1, "read error\n");
 		return -EIO;
 	}
@@ -295,7 +295,7 @@ static void ir_work(struct work_struct *
 
 	/* MSI TV@nywhere Plus requires more frequent polling
 	   otherwise it will miss some keypresses */
-	if (ir->c.adapter->id == I2C_HW_SAA7134 && ir->c.addr == 0x30)
+	if (ir->c->adapter->id == I2C_HW_SAA7134 && ir->c->addr == 0x30)
 		polling_interval = 50;
 
 	ir_key_poll(ir);
@@ -304,34 +304,15 @@ static void ir_work(struct work_struct *
 
 /* ----------------------------------------------------------------------- */
 
-static int ir_attach(struct i2c_adapter *adap, int addr,
-		      unsigned short flags, int kind);
-static int ir_detach(struct i2c_client *client);
-static int ir_probe(struct i2c_adapter *adap);
-
-static struct i2c_driver driver = {
-	.driver = {
-		.name   = "ir-kbd-i2c",
-	},
-	.id             = I2C_DRIVERID_INFRARED,
-	.attach_adapter = ir_probe,
-	.detach_client  = ir_detach,
-};
-
-static struct i2c_client client_template =
-{
-	.name = "unset",
-	.driver = &driver
-};
-
-static int ir_attach(struct i2c_adapter *adap, int addr,
-		     unsigned short flags, int kind)
+static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	IR_KEYTAB_TYPE *ir_codes = NULL;
 	char *name;
 	int ir_type;
 	struct IR_i2c *ir;
 	struct input_dev *input_dev;
+	struct i2c_adapter *adap = client->adapter;
+	unsigned short addr = client->addr;
 	int err;
 
 	ir = kzalloc(sizeof(struct IR_i2c),GFP_KERNEL);
@@ -341,14 +322,9 @@ static int ir_attach(struct i2c_adapter
 		goto err_out_free;
 	}
 
-	ir->c = client_template;
+	ir->c = client;
 	ir->input = input_dev;
-
-	ir->c.adapter = adap;
-	ir->c.addr    = addr;
-	snprintf(ir->c.name, sizeof(ir->c.name), "ir-kbd");
-
-	i2c_set_clientdata(&ir->c, ir);
+	i2c_set_clientdata(client, ir);
 
 	switch(addr) {
 	case 0x64:
@@ -423,24 +399,9 @@ static int ir_attach(struct i2c_adapter
 	snprintf(ir->name, sizeof(ir->name), "i2c IR (%s)", name);
 	ir->ir_codes = ir_codes;
 
-	/* register i2c device
-	 * At device register, IR codes may be changed to be
-	 * board dependent.
-	 */
-	err = i2c_attach_client(&ir->c);
-	if (err)
-		goto err_out_free;
-
-	/* If IR not supported or disabled, unregisters driver */
-	if (ir->get_key == NULL) {
-		err = -ENODEV;
-		goto err_out_detach;
-	}
-
-	/* Phys addr can only be set after attaching (for ir->c.dev) */
 	snprintf(ir->phys, sizeof(ir->phys), "%s/%s/ir0",
-		 dev_name(&ir->c.adapter->dev),
-		 dev_name(&ir->c.dev));
+		 dev_name(&adap->dev),
+		 dev_name(&client->dev));
 
 	/* init + register input device */
 	ir_input_init(input_dev, &ir->ir, ir_type, ir->ir_codes);
@@ -450,7 +411,7 @@ static int ir_attach(struct i2c_adapter
 
 	err = input_register_device(ir->input);
 	if (err)
-		goto err_out_detach;
+		goto err_out_free;
 
 	printk(DEVNAME ": %s detected at %s [%s]\n",
 	       ir->input->name, ir->input->phys, adap->name);
@@ -465,135 +426,40 @@ static int ir_attach(struct i2c_adapter
 
 	return 0;
 
- err_out_detach:
-	i2c_detach_client(&ir->c);
  err_out_free:
 	input_free_device(input_dev);
 	kfree(ir);
 	return err;
 }
 
-static int ir_detach(struct i2c_client *client)
+static int ir_remove(struct i2c_client *client)
 {
 	struct IR_i2c *ir = i2c_get_clientdata(client);
 
 	/* kill outstanding polls */
 	cancel_delayed_work_sync(&ir->work);
 
-	/* unregister devices */
+	/* unregister device */
 	input_unregister_device(ir->input);
-	i2c_detach_client(&ir->c);
 
 	/* free memory */
 	kfree(ir);
 	return 0;
 }
 
-static int ir_probe(struct i2c_adapter *adap)
-{
-
-	/* 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
-	*/
-
-	static const int probe_bttv[] = { 0x1a, 0x18, 0x4b, 0x64, 0x30, -1};
-	static const int probe_saa7134[] = { 0x7a, 0x47, 0x71, 0x2d, -1 };
-	static const int probe_em28XX[] = { 0x30, 0x47, -1 };
-	static const int probe_cx88[] = { 0x18, 0x6b, 0x71, -1 };
-	static const int probe_cx23885[] = { 0x6b, -1 };
-	const int *probe;
-	struct i2c_msg msg = {
-		.flags = I2C_M_RD,
-		.len = 0,
-		.buf = NULL,
-	};
-	int i, rc;
-
-	switch (adap->id) {
-	case I2C_HW_B_BT848:
-		probe = probe_bttv;
-		break;
-	case I2C_HW_B_CX2341X:
-		probe = probe_bttv;
-		break;
-	case I2C_HW_SAA7134:
-		probe = probe_saa7134;
-		break;
-	case I2C_HW_B_EM28XX:
-		probe = probe_em28XX;
-		break;
-	case I2C_HW_B_CX2388x:
-		probe = probe_cx88;
-		break;
-	case I2C_HW_B_CX23885:
-		probe = probe_cx23885;
-		break;
-	default:
-		return 0;
-	}
-
-	for (i = 0; -1 != probe[i]; i++) {
-		msg.addr = probe[i];
-		rc = i2c_transfer(adap, &msg, 1);
-		dprintk(1,"probe 0x%02x @ %s: %s\n",
-			probe[i], adap->name,
-			(1 == rc) ? "yes" : "no");
-		if (1 == rc) {
-			ir_attach(adap, probe[i], 0, 0);
-			return 0;
-		}
-	}
-
-	/* Special case for MSI TV@nywhere Plus remote */
-	if (adap->id == I2C_HW_SAA7134) {
-		u8 temp;
-
-		/* MSI TV@nywhere Plus controller doesn't seem to
-		   respond to probes unless we read something from
-		   an existing device. Weird... */
-
-		msg.addr = 0x50;
-		rc = i2c_transfer(adap, &msg, 1);
-			dprintk(1, "probe 0x%02x @ %s: %s\n",
-			msg.addr, adap->name,
-			(1 == rc) ? "yes" : "no");
-
-		/* Now do the probe. The controller does not respond
-		   to 0-byte reads, so we use a 1-byte read instead. */
-		msg.addr = 0x30;
-		msg.len = 1;
-		msg.buf = &temp;
-		rc = i2c_transfer(adap, &msg, 1);
-		dprintk(1, "probe 0x%02x @ %s: %s\n",
-			msg.addr, adap->name,
-			(1 == rc) ? "yes" : "no");
-		if (1 == rc)
-			ir_attach(adap, msg.addr, 0, 0);
-	}
-
-	/* Special case for AVerMedia Cardbus remote */
-	if (adap->id == I2C_HW_SAA7134) {
-		unsigned char subaddr, data;
-		struct i2c_msg msg[] = { { .addr = 0x40, .flags = 0,
-					   .buf = &subaddr, .len = 1},
-					 { .addr = 0x40, .flags = I2C_M_RD,
-					   .buf = &data, .len = 1} };
-		subaddr = 0x0d;
-		rc = i2c_transfer(adap, msg, 2);
-		dprintk(1, "probe 0x%02x/0x%02x @ %s: %s\n",
-			msg[0].addr, subaddr, adap->name,
-			(2 == rc) ? "yes" : "no");
-		if (2 == rc)
-			ir_attach(adap, msg[0].addr, 0, 0);
-	}
+static const struct i2c_device_id ir_kbd_id[] = {
+	{ "ir-kbd", 0 },
+	{ }
+};
 
-	return 0;
-}
+static struct i2c_driver driver = {
+	.driver = {
+		.name   = "ir-kbd-i2c",
+	},
+	.probe          = ir_probe,
+	.remove         = ir_remove,
+	.id_table       = ir_kbd_id,
+};
 
 /* ----------------------------------------------------------------------- */
 
--- v4l-dvb.orig/linux/drivers/media/video/ivtv/ivtv-i2c.c	2009-04-04 10:53:08.000000000 +0200
+++ v4l-dvb/linux/drivers/media/video/ivtv/ivtv-i2c.c	2009-04-04 10:58:36.000000000 +0200
@@ -589,9 +589,11 @@ static struct i2c_client ivtv_i2c_client
 	.name = "ivtv internal",
 };
 
-/* init + register i2c algo-bit adapter */
+/* init + register i2c adapter + instantiate IR receiver */
 int init_ivtv_i2c(struct ivtv *itv)
 {
+	int retval;
+
 	IVTV_DEBUG_I2C("i2c init\n");
 
 	/* Sanity checks for the I2C hardware arrays. They must be the
@@ -631,9 +633,32 @@ int init_ivtv_i2c(struct ivtv *itv)
 	ivtv_setsda(itv, 1);
 
 	if (itv->options.newi2c > 0)
-		return i2c_add_adapter(&itv->i2c_adap);
+		retval = i2c_add_adapter(&itv->i2c_adap);
 	else
-		return i2c_bit_add_bus(&itv->i2c_adap);
+		retval = i2c_bit_add_bus(&itv->i2c_adap);
+
+	/* Instantiate the IR receiver device, if present */
+	if (retval == 0) {
+		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,
+			I2C_CLIENT_END
+		};
+
+		memset(&info, 0, sizeof(struct i2c_board_info));
+		strlcpy(info.type, "ir-kbd", I2C_NAME_SIZE);
+		i2c_new_probed_device(&itv->i2c_adap, &info, addr_list);
+	}
+
+	return retval;
 }
 
 void exit_ivtv_i2c(struct ivtv *itv)
--- v4l-dvb.orig/linux/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c	2009-04-04 10:53:08.000000000 +0200
+++ v4l-dvb/linux/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c	2009-04-04 10:58:36.000000000 +0200
@@ -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)
--- v4l-dvb.orig/linux/drivers/media/video/saa7134/saa7134-i2c.c	2009-04-04 10:53:08.000000000 +0200
+++ v4l-dvb/linux/drivers/media/video/saa7134/saa7134-i2c.c	2009-04-04 12:56:26.000000000 +0200
@@ -444,6 +444,9 @@ int saa7134_i2c_register(struct saa7134_
 	saa7134_i2c_eeprom(dev,dev->eedata,sizeof(dev->eedata));
 	if (i2c_scan)
 		do_i2c_scan(dev->name,&dev->i2c_client);
+
+	/* Instantiate the IR receiver device, if present */
+	saa7134_probe_i2c_ir(dev);
 	return 0;
 }
 
--- v4l-dvb.orig/linux/drivers/media/video/saa7134/saa7134-input.c	2009-04-04 10:57:57.000000000 +0200
+++ v4l-dvb/linux/drivers/media/video/saa7134/saa7134-input.c	2009-04-04 12:56:26.000000000 +0200
@@ -134,10 +134,10 @@ static int get_key_msi_tvanywhere_plus(s
 	int gpio;
 
 	/* <dev> is needed to access GPIO. Used by the saa_readl macro. */
-	struct saa7134_dev *dev = ir->c.adapter->algo_data;
+	struct saa7134_dev *dev = ir->c->adapter->algo_data;
 	if (dev == NULL) {
 		dprintk("get_key_msi_tvanywhere_plus: "
-			"gir->c.adapter->algo_data is NULL!\n");
+			"gir->c->adapter->algo_data is NULL!\n");
 		return -EIO;
 	}
 
@@ -156,7 +156,7 @@ static int get_key_msi_tvanywhere_plus(s
 
 	/* GPIO says there is a button press. Get it. */
 
-	if (1 != i2c_master_recv(&ir->c, &b, 1)) {
+	if (1 != i2c_master_recv(ir->c, &b, 1)) {
 		i2cdprintk("read error\n");
 		return -EIO;
 	}
@@ -179,7 +179,7 @@ static int get_key_purpletv(struct IR_i2
 	unsigned char b;
 
 	/* poll IR chip */
-	if (1 != i2c_master_recv(&ir->c,&b,1)) {
+	if (1 != i2c_master_recv(ir->c, &b, 1)) {
 		i2cdprintk("read error\n");
 		return -EIO;
 	}
@@ -202,7 +202,7 @@ static int get_key_hvr1110(struct IR_i2c
 	unsigned char buf[5], cod4, code3, code4;
 
 	/* poll IR chip */
-	if (5 != i2c_master_recv(&ir->c,buf,5))
+	if (5 != i2c_master_recv(ir->c, buf, 5))
 		return -EIO;
 
 	cod4	= buf[4];
@@ -224,7 +224,7 @@ static int get_key_beholdm6xx(struct IR_
 	unsigned char data[12];
 	u32 gpio;
 
-	struct saa7134_dev *dev = ir->c.adapter->algo_data;
+	struct saa7134_dev *dev = ir->c->adapter->algo_data;
 
 	/* rising SAA7134_GPIO_GPRESCAN reads the status */
 	saa_clearb(SAA7134_GPIO_GPMODE3, SAA7134_GPIO_GPRESCAN);
@@ -235,9 +235,9 @@ static int get_key_beholdm6xx(struct IR_
 	if (0x400000 & ~gpio)
 		return 0; /* No button press */
 
-	ir->c.addr = 0x5a >> 1;
+	ir->c->addr = 0x5a >> 1;
 
-	if (12 != i2c_master_recv(&ir->c, data, 12)) {
+	if (12 != i2c_master_recv(ir->c, data, 12)) {
 		i2cdprintk("read error\n");
 		return -EIO;
 	}
@@ -267,7 +267,7 @@ static int get_key_pinnacle(struct IR_i2
 	unsigned int start = 0,parity = 0,code = 0;
 
 	/* poll IR chip */
-	if (4 != i2c_master_recv(&ir->c, b, 4)) {
+	if (4 != i2c_master_recv(ir->c, b, 4)) {
 		i2cdprintk("read error\n");
 		return -EIO;
 	}
@@ -682,14 +682,76 @@ void saa7134_input_fini(struct saa7134_d
 	dev->remote = NULL;
 }
 
-void saa7134_set_i2c_ir(struct saa7134_dev *dev, struct IR_i2c *ir)
+void saa7134_probe_i2c_ir(struct saa7134_dev *dev)
 {
+	struct i2c_board_info info;
+	const unsigned short addr_list[] = {
+		0x7a, 0x47, 0x71, 0x2d,
+		I2C_CLIENT_END
+	};
+
+	const unsigned short addr_list_msi[] = {
+		0x30, I2C_CLIENT_END
+	};
+	struct i2c_msg msg_msi = {
+		.addr = 0x50,
+		.flags = I2C_M_RD,
+		.len = 0,
+		.buf = NULL,
+	};
+
+	unsigned char subaddr, data;
+	struct i2c_msg msg_avermedia[] = { {
+		.addr = 0x40,
+		.flags = 0,
+		.len = 1,
+		.buf = &subaddr,
+	}, {
+		.addr = 0x40,
+		.flags = I2C_M_RD,
+		.len = 1,
+		.buf = &data,
+	} };
+
+	struct i2c_client *client;
+	int rc;
+
 	if (disable_ir) {
-		dprintk("Found supported i2c remote, but IR has been disabled\n");
-		ir->get_key=NULL;
+		dprintk("IR has been disabled, not probing for i2c remote\n");
+		return;
+	}
+
+	memset(&info, 0, sizeof(struct i2c_board_info));
+	strlcpy(info.type, "ir-kbd", I2C_NAME_SIZE);
+	client = i2c_new_probed_device(&dev->i2c_adap, &info, addr_list);
+	if (client)
 		return;
+
+	/* MSI TV@nywhere Plus controller doesn't seem to
+	   respond to probes unless we read something from
+	   an existing device. Weird... */
+	rc = i2c_transfer(&dev->i2c_adap, &msg_msi, 1);
+	dprintk(KERN_DEBUG "probe 0x%02x @ %s: %s\n",
+		msg_msi.addr, dev->i2c_adap.name,
+		(1 == rc) ? "yes" : "no");
+	client = i2c_new_probed_device(&dev->i2c_adap, &info, addr_list_msi);
+	if (client)
+		return;
+
+	/* Special case for AVerMedia Cardbus remote */
+	subaddr = 0x0d;
+	rc = i2c_transfer(&dev->i2c_adap, msg_avermedia, 2);
+	dprintk(KERN_DEBUG "probe 0x%02x/0x%02x @ %s: %s\n",
+		msg_avermedia[0].addr, subaddr, dev->i2c_adap.name,
+		(2 == rc) ? "yes" : "no");
+	if (2 == rc) {
+		info.addr = msg_avermedia[0].addr;
+		i2c_new_device(&dev->i2c_adap, &info);
 	}
+}
 
+void saa7134_set_i2c_ir(struct saa7134_dev *dev, struct IR_i2c *ir)
+{
 	switch (dev->board) {
 	case SAA7134_BOARD_PINNACLE_PCTV_110i:
 	case SAA7134_BOARD_PINNACLE_PCTV_310i:
--- v4l-dvb.orig/linux/drivers/media/video/saa7134/saa7134.h	2009-04-04 10:53:08.000000000 +0200
+++ v4l-dvb/linux/drivers/media/video/saa7134/saa7134.h	2009-04-04 12:56:26.000000000 +0200
@@ -791,6 +791,7 @@ void saa7134_irq_oss_done(struct saa7134
 int  saa7134_input_init1(struct saa7134_dev *dev);
 void saa7134_input_fini(struct saa7134_dev *dev);
 void saa7134_input_irq(struct saa7134_dev *dev);
+void saa7134_probe_i2c_ir(struct saa7134_dev *dev);
 void saa7134_set_i2c_ir(struct saa7134_dev *dev, struct IR_i2c *ir);
 void saa7134_ir_start(struct saa7134_dev *dev, struct card_ir *ir);
 void saa7134_ir_stop(struct saa7134_dev *dev);
--- v4l-dvb.orig/linux/drivers/media/video/usbvision/usbvision-i2c.c	2009-04-04 10:53:08.000000000 +0200
+++ v4l-dvb/linux/drivers/media/video/usbvision/usbvision-i2c.c	2009-04-04 10:58:36.000000000 +0200
@@ -211,6 +211,27 @@ static struct i2c_algorithm usbvision_al
 /* ----------------------------------------------------------------------- */
 static struct i2c_adapter i2c_adap_template;
 
+static void usbvision_i2c_register_ir(struct i2c_adapter *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(adap, &info, addr_list);
+}
+
 int usbvision_i2c_register(struct usb_usbvision *usbvision)
 {
 	static unsigned short saa711x_addrs[] = {
@@ -277,6 +298,9 @@ int usbvision_i2c_register(struct usb_us
 		}
 	}
 
+	/* Instantiate the IR receiver device, if present */
+	usbvision_i2c_register_ir(&usbvision->i2c_adap);
+
 	return 0;
 }
 
--- v4l-dvb.orig/linux/include/media/ir-kbd-i2c.h	2009-04-04 10:57:57.000000000 +0200
+++ v4l-dvb/linux/include/media/ir-kbd-i2c.h	2009-04-04 12:56:26.000000000 +0200
@@ -7,7 +7,7 @@ struct IR_i2c;
 
 struct IR_i2c {
 	IR_KEYTAB_TYPE         *ir_codes;
-	struct i2c_client      c;
+	struct i2c_client      *c;
 	struct input_dev       *input;
 	struct ir_input_state  ir;
 

-- 
Jean Delvare

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Andy Walls]

On Sat, 2009-04-04 at 14:28 +0200, Jean Delvare wrote:
> 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 <khali@linux-fr.org>
> Cc: Mauro Carvalho Chehab <mchehab@infradead.org>
> Cc: Hans Verkuil <hverkuil@xs4all.nl>
> Cc: Andy Walls <awalls@radix.net>
> Cc: Mike Isely <isely@pobox.com>
> ---
>  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(-)
> 

> --- v4l-dvb.orig/linux/drivers/media/video/cx18/cx18-i2c.c	2009-04-04 10:53:15.000000000 +0200
> +++ v4l-dvb/linux/drivers/media/video/cx18/cx18-i2c.c	2009-04-04 10:58:36.000000000 +0200
> @@ -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,
> +		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.


> +	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;
> +
> +	/* 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)

Regards,
Andy

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Mike Isely]

On Sat, 4 Apr 2009, Andy Walls wrote:

   [...]

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

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

-- 

Mike Isely
isely @ pobox (dot) com
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Andy Walls]

On Sat, 2009-04-04 at 11:05 -0500, Mike Isely wrote:
> On Sat, 4 Apr 2009, Andy Walls wrote:
> 
>    [...]
> 
> > 
> > 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)

I was thinking about this while mowing the lawn today....

The objectives seem to be:

1. Avoid auto probing
2. Leverage the bridge driver's knowledge of what should be available
3. Allow the user to dictate which kernel IR driver module be used with
the subordinate device.


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

Regards,
Andy

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

I think we can get there and still have the random probing reduced.

Regards,
Andy


>   -Mike

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Andy Walls]

On Sat, 2009-04-04 at 18:25 -0400, Andy Walls wrote:
> 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."

Oops.  That leaves DTV only devices with IR out in the cold, unless they
start to implement a v4l2_device and IR v4l2_subdev as well, or unless
they were never used with ir-kbd-i2c in the first place.

Regards,
Andy

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Jean Delvare]

Hi Andy,

On Sat, 04 Apr 2009 09:42:09 -0400, Andy Walls wrote:
> On Sat, 2009-04-04 at 14:28 +0200, Jean Delvare wrote:
> > 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 <khali@linux-fr.org>
> > Cc: Mauro Carvalho Chehab <mchehab@infradead.org>
> > Cc: Hans Verkuil <hverkuil@xs4all.nl>
> > Cc: Andy Walls <awalls@radix.net>
> > Cc: Mike Isely <isely@pobox.com>
> > ---
> >  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(-)
> > 
> 
> > --- v4l-dvb.orig/linux/drivers/media/video/cx18/cx18-i2c.c	2009-04-04 10:53:15.000000000 +0200
> > +++ v4l-dvb/linux/drivers/media/video/cx18/cx18-i2c.c	2009-04-04 10:58:36.000000000 +0200
> > @@ -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,
> > +		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.

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

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

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.

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

-- 
Jean Delvare

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Andy Walls]

On Sun, 2009-04-05 at 00:51 +0200, Jean Delvare wrote:
> Hi Andy,
> 
> On Sat, 04 Apr 2009 09:42:09 -0400, Andy Walls wrote:
> > On Sat, 2009-04-04 at 14:28 +0200, Jean Delvare wrote:
> > > 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 <khali@linux-fr.org>
> > > Cc: Mauro Carvalho Chehab <mchehab@infradead.org>
> > > Cc: Hans Verkuil <hverkuil@xs4all.nl>
> > > Cc: Andy Walls <awalls@radix.net>
> > > Cc: Mike Isely <isely@pobox.com>
> > > ---
> > >  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(-)
> > > 
> > 
> > > --- v4l-dvb.orig/linux/drivers/media/video/cx18/cx18-i2c.c	2009-04-04 10:53:15.000000000 +0200
> > > +++ v4l-dvb/linux/drivers/media/video/cx18/cx18-i2c.c	2009-04-04 10:58:36.000000000 +0200
> > > @@ -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,
> > > +		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.

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


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


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



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

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

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.  

>  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



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

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.


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


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

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.
  

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


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


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


> I hope I managed to clarify the situation a bit.

A bit.  I'm not totally clear, but once I play with it a little, I'll
probably get it.

Regards,
Andy

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Jean Delvare]

Hi Andy,

On Sat, 04 Apr 2009 21:50:08 -0400, Andy Walls wrote:
> On Sun, 2009-04-05 at 00:51 +0200, Jean Delvare wrote:
> > On Sat, 04 Apr 2009 09:42:09 -0400, Andy Walls wrote:
> > > 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.
> 
> 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
> 
> 
> > 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.

Well, it doesn't really matter to me at this point. All I care about is
that ir-kbd-i2c didn't support cx18 adapters before, so my patch can
just ignore them. Support can be added later. More interestingly, if
only one board needs to be supported for now and you have all the
information about the IR receiver, then we simply don't need
auto-detection of IR devices on cx18 at all. We can directly take the
clean route of device declaration. As far as I know the cx18 does that
very well for all other chips (tuner, decoder etc.) much like the ivtv
driver does, so adding support for IR should be easy.

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

Which I just did...

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

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.

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

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

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

I can't see any piece of lirc in the v4l-dvb repository so I'd guess
nothing made it in. But at least if they are trying to get their code
merged, that's a good thing.

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

Yeah, I've seen it. How ugly :(

> 2. creates and adds I2C driver

A legacy driver... which will break as soon as the legacy model is
removed from the kernel - really soon now.

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

A deprecated approach...

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

Something the new probing model doesn't easily support. All in all, the
lirc code looks a lot like the old ir-kbd-i2c driver (before my
changes.) So the exact same changes I just made, will have to be done
there as well. It will be more difficult for an out-of-tree driver, but
OTOH they can benefit from the work I just did.

Nevertheless, I don't see ir-kbd-i2c and lirc_i2c (and possibly others)
being developed in parallel as a viable solution for the future. It's
about time to come up with a unified solution which supports all the
hardware out there properly.

I don't know much about input drivers and IR, but my initial feeling is
that handling the remote control as a regular keyboard-like input is
the way to go. After all a remote control is just a set of keys,
nothing really original. Now, if the lirc interface offers something
more that can't be implemented via the input interface, fine with me,
let's keep it. But the user shouldn't have to manually chose between 2
drivers.

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

No, the ir-kbd-i2c driver module doesn't get loaded just because a
device is created. That _would_ be the case if we added a
MODULE_ALIAS() to the driver, and udev is running. But for now there is
no MODULE_ALIAS() exactly because of existing alternatives to
ir-kbd-i2c.

That being said, looking at the lirc code, it _will_ be broken by my
changes, because the i2c core won't let a legacy i2c device be created
at the same address as a new-style i2c device. So users will have to
load the bridge driver with disable_ir=1 if they want to use lirc_i2c.
This sounds like a good reason to add a disable_ir module parameter to
bridge drivers which don't have it yet.

Then again, lirc_i2c will break in the next months with the removal of
the legacy model anyway. It will have to be converted to the new
binding model, at which point we can discuss a new temporary strategy
to keep all ir modules usable. More on this below.

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

Done, I'll post the patch later today.

> > > 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? 
> 
> 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)?

Yeah, that's another good question. But the ir-kbd-i2c driver is pretty
small, so that's not necessarily a problem in practice. A single driver
(cleanly) supporting several IR receiver devices shouldn't be a problem.

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

Well, ir-kbd-i2c exposes the IR as a standard keyboard-like input, this
seems like a reasonable abstraction to me too. But, just like you, I
really don't know enough to compare.

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

Depends. If your bridge driver creates an I2C named "haup-zilog-ir" and
specifically expects ir-haup-zilog-i2c.ko and only ir-haup-zilog-i2c.ko
to bind to it, then yes. But if ir-haup-zilog-i2c.ko if only an
alternative to ir-kbd-i2c for this piece of hardware, then the bridge
driver would create "ir-kbd" (or we might even make the name
driver-neutral such as "v4l-ir") and both ir-kbd-i2c.ko and
ir-haup-zilog-i2c.ko would use that device name (and the one loaded
gets to grab it.)

This is the easiest way to support ir-kbd-i2c and lirc_i2c in parallel
in the new binding model. This might do for short term.

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

Yes, the device name (e.g. "ir-kbd") says what device type this is, it
isn't a unique identifier (you can have more than one IR receiver in a
given system). The unique ID is made of the i2c adapter # and the main
i2c device address.

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

Yes, it will be broken.

-- 
Jean Delvare

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Andy Walls]

On Sun, 2009-04-05 at 15:08 +0200, Jean Delvare wrote:
> Hi Andy,
> 

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

OK.  I'll test your change anyway if I can.



> > 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 :(

Well, like many of the lirc modules, it's a little kludged.  The main
problem is this:

1. lirc_pvr150, in the past, needed to make a direct call into the ivtv
module to reset the IR chip, if it detected that the chip was hung up.
That's why it tries to load the ivtv module, to make sure that symbol is
in the kernel.  This could cause problems, if it was a Z8 chip that was
supported by some other bridge driver.  I wrote a patch for lirc_pvr150
for cx18 devices for users who needed it. 

lirc_zilog is the cut down version of lirc_pvr150 module that was
submitted in the patchset to the LKML, and no longer has the reset
logic.  The reset logic is not needed anymore as far as I can tell, and
thus the cx18 specific patch is probably irrelevant for lirc_zilog.


Other weird things include:

2. In lirc_pvr150 and lirc_zilog, both the IR Rx and IR Tx support are
in one module, which is a break from the normal LIRC driver modules that
keep those functions separate.  This was done for the sake of detecting
if the chip had hung up and to call the reset logic, AFAICT.

3. lirc_pvr150 and lirc_zilog have an IR blaster "firmware" image that
is really an encoding of a bunch of captured sequences between the
Windows driver and the Z8F0811 chip.  It allows the lirc_zilog or
lirc_pvr150 driver to do IR blasting by essentially performing a replay
attack on the Z8F0811.

Since the Zilog EULA that comes with the Hauppauge Windows IR driver for
the Z8F0811 is pretty draconian, replaying captured snoops is probably
the best that can be done legally to stimulate the microcontroller IR Tx
code in the Z8 as delivered.


Regards,
Andy

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Mike Isely]

Nacked-by: Mike Isely <isely@pobox.com>

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.

  -Mike


On Sat, 4 Apr 2009, Jean Delvare wrote:

> --- v4l-dvb.orig/linux/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c	2009-04-04 10:53:08.000000000 +0200
> +++ v4l-dvb/linux/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c	2009-04-04 10:58:36.000000000 +0200
> @@ -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)

-- 

Mike Isely
isely @ pobox (dot) com
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Jean Delvare]

Hi Mike,

On Sat, 4 Apr 2009 10:51:01 -0500 (CDT), Mike Isely wrote:
> 
> Nacked-by: Mike Isely <isely@pobox.com>
> 
> 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?

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

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

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

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

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

-- 
Jean Delvare

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Mike Isely]

On Sun, 5 Apr 2009, Jean Delvare wrote:

> Hi Mike,
> 
> On Sat, 4 Apr 2009 10:51:01 -0500 (CDT), Mike Isely wrote:
> > 
> > Nacked-by: Mike Isely <isely@pobox.com>
> > 
> > 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?
> 

How should I know?  I don't maintain either.  But I know they are both 
used and it's not my place to force usage of one over the other.


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

As I interpret this, your patch will cause the pvrusb2 to probe for the 
IR receiver's I2C address and bind ir-kbd-i2c to what it finds.  That 
will break anyone's usage of the driver where another IR driver (e.g. 
something from LIRC) is used instead.


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

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.


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

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.



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

"Hauppauge trick"?


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

Yes, but I can point out that this change now will cause ir-kbd-i2c to 
be loaded in cases where the user didn't want it and will risk 
interference with the driver that the user *did* want.


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

That "abuse" is a separate issue.  The pvrusb2 driver started a long 
time ago out-of-tree and at that time it was the only way to get IR to 
work at all.  Personally I forgot all about it back in 2005 and was only 
recently reminded that this situation still exists.  It should be fixed 
and I'd welcome the correct patch to fix this.  I haven't fixed it 
myself, because, well I've had much bigger fish to fry here.

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.

  -Mike


-- 

Mike Isely
isely @ pobox (dot) com
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Jean Delvare]

Hi Mike,

Selected answers, as most points have already been discussed elsewhere
meanwhile...

On Sat, 4 Apr 2009 18:29:35 -0500 (CDT), Mike Isely wrote:
> On Sun, 5 Apr 2009, Jean Delvare wrote:
> > 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.

Good. Declaring the right IR receiver device based on board information
is clearly the way to go.

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

Yes, you are right.

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

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

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

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

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

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.

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

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

-- 
Jean Delvare

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Mike Isely]

Jean:

More comments interspersed below...

On Sun, 5 Apr 2009, Jean Delvare wrote:

   [...]

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

Well I didn't say it was perfect.  But this has been a valid use-case 
and I know people have been using this it this way.

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

What's worse is taking away a choice when the alternatives are not 
interchangeable.  In this case we have two different drivers with two 
different interfaces which do not cover the same range of hardware.  
The lirc case happens to work quite well in MythTV.  In addition, lirc 
covers hardware situations involving the pvrusb2 driver that ir-kbd-i2c 
does not.  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.


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

Well yes.  The binding of the address is enough to mess things up.


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

Yes, I would argue that this is still a regression.  I really think this 
should be an *enable* switch in order to match previous behavior.  In 
the long term I agree that this is less than optimal / undesirable, 
however in the long term none of it is going to matter anyway, since it 
looks like lirc without further changes is going to have problems here.  
Once (if) lirc is changed to use the new binding model then this whole 
argument becomes moot.  Given that, then anything done here is a short 
term strategy, designed to avoid a regression, so I would much rather 
this be an enable not a disable switch.


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

*I* didn't break ir-kbd-i2c.  And now because of what has effectively 
"broken" (viewed in just the wrong light) you are asking me to accept 
changes in the pvrusb2 driver to make up the difference?  Normally I 
wouldn't object, except those proposed changes in turn will break 
another driver (lirc).  That is why I nacked the change.  I don't care 
if the driver is in-tree or out of tree, I am not going to accept a 
change that breaks without recourse a known use-case that people are 
using.  I consider Mauro's reasoning flawed here.

What I am suggesting for (1) does not "break" ir-kbd-i2c.  Yes, it 
impacts its use but only within the pvrusb2 driver, and only until I get 
(2) implemented, which unless something comes up I plan on dealing with 
today.  So (1) should be acceptable.


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

OK, no problem.  I'm happy with letting it die :-)

  -Mike


-- 

Mike Isely
isely @ pobox (dot) com
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Andy Walls]

On Sun, 2009-04-05 at 13:33 -0500, Mike Isely wrote:

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

Yes.  My first remote for my PC was an HP OEM'ed and customized unit.
LIRC's text configuration files made support for the remote's
non-standard keys a task for 'vi' and not 'make'.

Key mappings belong in configuration files - not hard coded in the
kernel.



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

Some of the discussion on the LKML about why an input layer device is
sufficient for handling most, but not all cases is interesting:

http://lkml.org/lkml/2008/9/11/63

Gerd also enlightens us on why we have ir-kdb-i2c in kernel in that
post.  It appears that ir-kbd-i2c was written to avoid using LIRC for TV
card I2C IR devices for the most common use cases.  As such, it's a 90%
(80%, 70% ?) solution: no blasting, no raw IR parsing for unknown
protocols, only the most "common" remotes supported, and, of course, no
support for non-I2C devices.


My needs don't fit that unfortunately.  I need IR blasting, an uncommon
remote supported, and both USB and I2C IR device support.


Regards,
Andy

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Trent Piepho]

On Sun, 5 Apr 2009, Andy Walls wrote:
>
> 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.

More than that.  In 1997 I bought a serial port remote off ebay and tried
to get it to work with linux.  I found an abandoned project from the
Metzler brothers called LIRC, though it didn't work.  I wrote a new
protocol for the serial port driver, which was the only one at the time,
rewrote the remote pulse decoding code and came up a new socket based the
client/server protocol and wrote the x-event client.  At that point remotes
were defined in header files so make was still needed to add a new one.

pvrusb2 IR changes coming [was: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model]

[Mike Isely]

Jean:

Here's a description of what I've got on the front burner right now:

1. The pvrusb2 driver now can unambiguously know if it is dealing with a 
device that is known to have ir-kbd-i2c compatible IR capabilities.

2. There is a new module option, "disable_autoload_ir_kbd", which if 
present and set to 1 will indicate that ir-kbd should not be loaded.

2. Based upon (1) and (2), the driver will optionally attempt to load 
ir-kbd using the code from your patch.

3. In the pvrusb2 case, the only i2c address that currently matters is 
0x18 (though I have some suspicions about another case but that can be 
dealt with later), so I trimmed the probe list in the register function 
you had added.

Since calling i2c_new_probed_device() for a non-existent target driver 
doesn't cause any harm, then merging the above now should not result in 
any kind of regression.  So it can go in even before the rest of your 
changes.  That I believe also removes the objection Mauro had - this way 
there's no issues / dependencies.  I've tested this enough to know that 
it at least doesn't do any further harm.

I will put this up in a changeset shortly.

With all that said, we should not ignore lirc and instead do whatever is 
reasonable to help ensure it continues to work.  Though admittedly 
there's been plenty of opportunity to update and this whole transition 
has been going on for a long time.  The stuff I describe above should at 
least keep the pvrusb2 driver out of the fray for now.

  -Mike


-- 

Mike Isely
isely @ pobox (dot) com
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Hans Verkuil]

On Sunday 05 April 2009 01:05:39 Jean Delvare wrote:
> Hi Mike,
>
> On Sat, 4 Apr 2009 10:51:01 -0500 (CDT), Mike Isely wrote:
> > Nacked-by: Mike Isely <isely@pobox.com>
> >
> > 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?
>
> > 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.
>
> > 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.
>
> > - 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.
>
> > 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.
>
> > 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.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Mauro Carvalho Chehab]

On Sun, 5 Apr 2009 07:46:47 +0200
Hans Verkuil <hverkuil@xs4all.nl> wrote:

> On Sunday 05 April 2009 01:05:39 Jean Delvare wrote:
> > Hi Mike,
> >
> > On Sat, 4 Apr 2009 10:51:01 -0500 (CDT), Mike Isely wrote:
> > > Nacked-by: Mike Isely <isely@pobox.com>
> > >
> > > 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?
> >
> > > 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.
> >
> > > 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.
> >
> > > - 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.
> >
> > > 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.
> >
> > > 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.

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.

>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

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Andy Walls]

On Sun, 2009-04-05 at 06:14 -0300, Mauro Carvalho Chehab wrote:

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

The "tricks" were to not break the current user experience by saddling
them with a kernel module that they may not want.  (If the tricks are
needed at all - I'm will test later today.)

I agree that better in kernel infrastructure needs to be in place for
IR.

LIRC's kernel space infrastructure module, lirc_dev, probably isn't a
bad model for support of IR devices.  The individual LIRC modules for
supporting specific sets of devices are the ones that have problems to
varying degrees.


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

Input event interfaces alone neglect IR blasters.


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

There is at least one other motivation:

LIRC also handles a number of other hardware interfaces that are not
I2C: serial ports (/dev/ttySX), parallel port, USB, etc.


I happen to use the lirc_mceusb2 module for my Phillips Home IR
receiver/blaster (I'm not sure if the blaster works under linux.)


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

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

Regards,
Andy

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


> 
> Cheers,
> Mauro
> 

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Mauro Carvalho Chehab]

On Sun, 05 Apr 2009 08:44:15 -0400
Andy Walls <awalls@radix.net> wrote:

> 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

As other out-of-tree drivers that have a long trip, I suspect that lirc did
some assumptions, while event and v4l did different ones. Due to kernel rules
to keep API's forever, we should take some care to avoid breaking the existing
API in favor to another one. So, this probably means some lirc redesign, in
order to get his way to kernel, on a similar path that ivtv driver did.

The part of lirc that I'm concerned with are the ones that work with GPIO and
I2C devices and the API.

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

We should cover also the lirc gpio module(s).

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

I don't think that breaking it into one per device is the better approach. IMO,
we need a common i2c glue (like what ir-kbd-i2c provides, if we remove the
legacy stuff) that it is IR independent. the IR dependent parts can be part of
ir-common module or eventually we can split it into sub-modules.

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

For sure. The event interface is the kernel way for input devices. There are
also other IR devices (like IR mouses and keyboards) already handled via
input/event interface.

On a first glance, I don't see the need to exporting raw data to userspace,
although I understand why lirc needs this currently.

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

IMO, the proper workflow would be to discuss lirc as a hole with Lirc people,
linux-media and input/event people.

Cheers,
Mauro

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Jean Delvare]

Hi Hans,

On Sun, 5 Apr 2009 07:46:47 +0200, Hans Verkuil wrote:
> 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).

I'd rather name the parameter "disable_ir", to make it consistent with
what other bridge drivers already use, and also because what the
parameter really does is preventing I2C devices from being
instantiated, _not_ preventing the ir-kbd-i2c module from loading.

I have a patch doing that already, it's pretty simple, I'll post it in
a minute.

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

lirc_i2c will break, it still uses the legacy binding model. That's
what you get for living outside the kernel tree... Upgrading it
shouldn't be too hard, given that the difficult part is to update the
bridge drivers and I've already taken care of this part (although
lirc_i2c might need to probe more addresses than ir-kbd-i2c does).

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

I wholeheartedly agree.

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

Totally correct. The current mess^Wmodel vaguely works when a single TV
card is present, but mixing different TV cards in the same system would
certainly break. Admittedly this is probably not a very common case, I
guess the vast majority of users have a single TV card.

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

Please note that my conversion _already_ no longer makes any use of
adapter IDs. What it still does is probing, except for a few selected
cards (AVerMedia Cardbus and MSI TV@nywhere Plus). The idea is clearly
to turn probing into a fallback and use per-card data for IR device
instantiation the first choice for as many cards as possible in the
future.

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

Changes since previous version:
* Dropped cx18 changes on request by Andy Walls.
* Added disable_ir module parameter to all bridge drivers which didn't
  have it.

-- 
Jean Delvare

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Andy Walls]

On Sun, 2009-04-05 at 16:05 +0200, Jean Delvare wrote:
> Hi Hans,


Hi Jean,

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


--- v4l-dvb.orig/linux/drivers/media/video/ivtv/ivtv-i2c.c	2009-04-04 10:53:08.000000000 +0200
+++ v4l-dvb/linux/drivers/media/video/ivtv/ivtv-i2c.c	2009-04-04 10:58:36.000000000 +0200
[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.

Regards,
Andy

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Jean Delvare]

Hi Andy,

On Sun, 05 Apr 2009 15:35:52 -0400, Andy Walls wrote:
> --- v4l-dvb.orig/linux/drivers/media/video/ivtv/ivtv-i2c.c	2009-04-04 10:53:08.000000000 +0200
> +++ v4l-dvb/linux/drivers/media/video/ivtv/ivtv-i2c.c	2009-04-04 10:58:36.000000000 +0200
> [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.

Thanks for reporting. The list above is taken directly from the
original ir-kbd-i2c driver (minus address 0x4b which Hans Verkuil told
me was useless for the ivtv and cx18 adapters). 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.

-- 
Jean Delvare

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Andy Walls]

On Mon, 2009-04-06 at 11:04 +0200, Jean Delvare wrote:
> 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.


Makes sens to me.  Especially when I just simply, blindly added 0x71 in
my initial testing, I got a kernel Oops.

Regards,
Andy

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Janne Grunau]

On Sun, Apr 05, 2009 at 07:46:47AM +0200, Hans Verkuil wrote:
>
> 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.

Janne

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Jean Delvare]

Hi Janne,

On Sun, 5 Apr 2009 16:37:49 +0200, Janne Grunau wrote:
> On Sun, Apr 05, 2009 at 07:46:47AM +0200, Hans Verkuil wrote:
> >
> > 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

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)

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

-- 
Jean Delvare

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Janne Grunau]

On Sun, Apr 05, 2009 at 06:37:43PM +0200, Jean Delvare wrote:
> Hi Janne,
> 
> On Sun, 5 Apr 2009 16:37:49 +0200, Janne Grunau wrote:
> > On Sun, Apr 05, 2009 at 07:46:47AM +0200, Hans Verkuil wrote:
> > >
> > > 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
> 
> 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)

hmm, getting that here too. I'll send Jarod a mail. You can use my clone
instead: git://git.jannau.net/linux-2.6-lirc.git

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

lirc_zilog or lirc_pvr is not in lirc's cvs since it requires a
'firmware' for mapping keys to sequences for the ir blaster.

Janne

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Andy Walls]

Hi Janne,

On Sun, 2009-04-05 at 16:37 +0200, Janne Grunau wrote:
> On Sun, Apr 05, 2009 at 07:46:47AM +0200, Hans Verkuil wrote:
> >
> > 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.

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.

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

Regards,
Andy

> Janne

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Janne Grunau]

On Sun, Apr 05, 2009 at 01:39:33PM -0400, Andy Walls wrote:
> On Sun, 2009-04-05 at 16:37 +0200, Janne Grunau wrote:
> > 
> > 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.

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.

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

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.

> 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

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Andy Walls]

On Sun, 2009-04-05 at 20:31 +0200, Janne Grunau wrote:
> On Sun, Apr 05, 2009 at 01:39:33PM -0400, Andy Walls wrote:

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

Oh, OK.

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

I'd personally leave lirc_zilog separate since it handles Tx and RX for
one specific chip, and lirc_i2c is Rx only for a number of chips.
Perhaps dropping Rx support for the Zilog Z8 in lirc_i2c and then
modifying lirc_zilog to still do Rx, even if the "firmware" wasn't
available for Tx, is a better way to go.




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

OK.

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

Hmm.  Well good luck.

Let me know if I can help.  I have 2 cards with the Zilog and a USB unit
that is supported by lirc_mceusb2.

Regards,
Andy

> Janne

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Jean Delvare]

On Sun, 05 Apr 2009 14:58:17 -0400, Andy Walls wrote:
> On Sun, 2009-04-05 at 20:31 +0200, Janne Grunau wrote:
> > 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.

-- 
Jean Delvare

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[hermann pitton]

Am Sonntag, den 05.04.2009, 22:22 +0200 schrieb Jean Delvare:
> On Sun, 05 Apr 2009 14:58:17 -0400, Andy Walls wrote:
> > On Sun, 2009-04-05 at 20:31 +0200, Janne Grunau wrote:
> > > 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.

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.

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

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

To have it was a need after 2.5.x turned into 2.6.x. It was not even in
sight if and when lirc would be ever usable on it again. You have it
from Gerd.

I also think we currently have lots of users with all sorts of "TV"
cards. Triple cards are still quite expensive and can have only one DVB
and one analog stream at once, Quad cards depend on special PCI slots
and PCIe multi capable cards are not yet supported.

Because of that, there are lots of mythtv and similar machines stuffed
with all sort of cards in every free PCI slot I think.

Cheers,
Hermann

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Andy Walls]

On Sun, 2009-04-05 at 23:22 +0200, hermann pitton wrote:
> Am Sonntag, den 05.04.2009, 22:22 +0200 schrieb Jean Delvare:
> > On Sun, 05 Apr 2009 14:58:17 -0400, Andy Walls wrote:


> 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

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[hermann pitton]

Am Sonntag, den 05.04.2009, 18:00 -0400 schrieb Andy Walls:
> On Sun, 2009-04-05 at 23:22 +0200, hermann pitton wrote:
> > Am Sonntag, den 05.04.2009, 22:22 +0200 schrieb Jean Delvare:
> > > On Sun, 05 Apr 2009 14:58:17 -0400, Andy Walls wrote:
> 
> 
> > 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.

Hmmm ....

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

Maybe try that from 5 years back.

Or what Mauro has.

Who ever had to recompile a module for changing the key tables?

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

I doubt you are down to it.

Cheers,
Hermann

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[hermann pitton]

Hi Andy,

Am Sonntag, den 05.04.2009, 18:00 -0400 schrieb Andy Walls:
> On Sun, 2009-04-05 at 23:22 +0200, hermann pitton wrote:
> > Am Sonntag, den 05.04.2009, 22:22 +0200 schrieb Jean Delvare:
> > > On Sun, 05 Apr 2009 14:58:17 -0400, Andy Walls wrote:
> 
> 
> > 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
> 

you always end up with something transmitting series of 0s and 1s.

It does not matter if the medium is infrared, infradead ;), wireless or
whats or ever.

If it spares a chip for the remote and you have to get it from IRQs
triggered, you have to do that.

It could also be some voltage change, a ultrasound beeper or what ever.

The first place for such is the input layer and nothing out of the
kernel.

Cheers,
Hermann

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Mauro Carvalho Chehab]

On Sun, 05 Apr 2009 18:00:04 -0400
Andy Walls <awalls@radix.net> wrote:

> On Sun, 2009-04-05 at 23:22 +0200, hermann pitton wrote:
> > Am Sonntag, den 05.04.2009, 22:22 +0200 schrieb Jean Delvare:
> > > On Sun, 05 Apr 2009 14:58:17 -0400, Andy Walls wrote:
> 
> 
> > 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.

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.


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

> 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! */
}

Cheers,
Mauro

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Mike Isely]

On Sun, 5 Apr 2009, Mauro Carvalho Chehab wrote:

> On Sun, 05 Apr 2009 18:00:04 -0400
> Andy Walls <awalls@radix.net> wrote:
> 
> > On Sun, 2009-04-05 at 23:22 +0200, hermann pitton wrote:
> > > Am Sonntag, den 05.04.2009, 22:22 +0200 schrieb Jean Delvare:
> > > > On Sun, 05 Apr 2009 14:58:17 -0400, Andy Walls wrote:
> > 
> > 
> > > 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.
> 
> 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.


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

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.


> 
> > 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! */
> }

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


-- 

Mike Isely
isely @ pobox (dot) com
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[hermann pitton]

Am Sonntag, den 05.04.2009, 21:52 -0500 schrieb Mike Isely:
> On Sun, 5 Apr 2009, Mauro Carvalho Chehab wrote:
> 
> > On Sun, 05 Apr 2009 18:00:04 -0400
> > Andy Walls <awalls@radix.net> wrote:
> > 
> > > On Sun, 2009-04-05 at 23:22 +0200, hermann pitton wrote:
> > > > Am Sonntag, den 05.04.2009, 22:22 +0200 schrieb Jean Delvare:
> > > > > On Sun, 05 Apr 2009 14:58:17 -0400, Andy Walls wrote:
> > > 
> > > 
> > > > 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.
> > 
> > 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.
> 
> 
> > 
> > 
> > > 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.
> 
> 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.
> 
> 
> > 
> > > 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! */
> > }
> 
> 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
> 

ir_kbd_i2c is much older than Gerd. It comes from bttv.

lirc was unusable on 2.6.x. I do confirm that.

If you like to have some more on the ways, the just merged dvb did state
the whole in kernel i2c is crap, likely with some good reasons.

Mike, I'm too old now to start on it again, but feel free to give us
more insight.

Cheers,
Hermann

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Trent Piepho]

On Sun, 5 Apr 2009, Mike Isely wrote:
> 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?

IMHO, the way the remote supported is compiled into the kernel is absurd.
The system I setup 12 years ago was better than this.  At least the remote
was compiled into a userspace daemon and multiple remotes were supported at
the same time.  The keycode system I used had a remote id/key id split, so
you could have volume up key on any remote control the mixer but make the
power buttons on different remotes turn on different apps.

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

A remote typically sends a header sequence of a long pulse and space before
the code.  The length of the pulse on the space varies greatly by remote
and makes a good way to identify the remote when multiple ones are
supported.

Then a pulse coded remote sends a sequence bits, usually 8 to 32.  The
length of the pulse identifies 1s or 0s.  Different remotes have different
pulse lengths and different spaces between them.  RC5 remotes use
Manchester encoding for this part.

When you hold a key down some remotes just repeat the same sequence over
and over again.  Some repeat the scan code but omit the header part.  Some
send out a special pulse sequence to indicate the last key is being held
down.  With the latter two methods you can tell the difference between a
key being held down and a key being pressed repeatedly.  With the first you
have guess based on how fast the repeats are coming in.  This is very
different than a keyboard, which sends a code when you press a key and
another when you release it.

The rate at which remotes repeat varies greatly.  You might find that one
remote makes your volume change annoying slowly while another is much too
fast to be usable.  Remote keys usually start repeating without delay, so
if you let a toggle like 'mute' repeat it becomes almost impossible to hit
it just once.  Entering numbers becomes impossible as well.

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Mauro Carvalho Chehab]

On Sun, 5 Apr 2009 21:52:31 -0500 (CDT)
Mike Isely <isely@isely.net> wrote:

> On Sun, 5 Apr 2009, Mauro Carvalho Chehab wrote:
> 
> > On Sun, 05 Apr 2009 18:00:04 -0400
> > Andy Walls <awalls@radix.net> wrote:
> > 
> > > On Sun, 2009-04-05 at 23:22 +0200, hermann pitton wrote:
> > > > Am Sonntag, den 05.04.2009, 22:22 +0200 schrieb Jean Delvare:
> > > > > On Sun, 05 Apr 2009 14:58:17 -0400, Andy Walls wrote:
> > > 
> > > 
> > > > 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.
> > 
> > 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.

Yes, this should be considered by the same driver, but perhaps not using the
same userspace API. I'm not sure if event interface allows such usage.

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

Removing kernel functionality breaks compatibility with legacy applications.


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

Setting up lirc to work it is a way more complex that just plugging a
device and having IR working. For all my usages here, I prefer to just load a
different IR table than having to deal with lirc configuration stuff.

It should be users option to use lirc or just rely on the existing IR support.

> 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 agree that we should help with this. IMO, the proper way for lirc drivers for
media devices is that they should include linux-media oh the discussions about
the inclusion of those drivers, in a way that just one driver would be used,
and that the event interface will keep working by default, as currently.

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

This is the legacy model, kept there only due to the fact that we don't really
know with 100% sure what boards were using those functions. For the new model,
you should take a look on a bridge driver, like, for example, on em28xx-cards:

void em28xx_set_ir(struct em28xx *dev, struct IR_i2c *ir)
{
        if (disable_ir) {
                ir->get_key = NULL;
                return ;
        }

        /* detect & configure */
        switch (dev->model) {
        case (EM2800_BOARD_UNKNOWN):
                break;
        case (EM2820_BOARD_UNKNOWN):
                break;
        case (EM2800_BOARD_TERRATEC_CINERGY_200):
        case (EM2820_BOARD_TERRATEC_CINERGY_250):
                ir->ir_codes = ir_codes_em_terratec;
                ir->get_key = em28xx_get_key_terratec;
                snprintf(ir->c.name, sizeof(ir->c.name),
                         "i2c IR (EM28XX Terratec)");
                break;
...

This function is called just after binding the IR, by i2c callback. It properly
fills the IR tables, get_key and IR names based on each different board model.

> 
> 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?!?

I did a typo there. In fact, opens the proper /dev/event interface. The code
is the same kind of code that allows userspace to change from US keymapping to
a local keymapping.

It should be easy to call an userspace app from udev, after a device
connection, if the user wants to have a different IR keycode table for his
device.

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

The IR in-kernel implementation has the proper timing and protocol definitions
for each device. Having something else to take care with will just cause troubles.

For example, I have two saa713x devices with different IR chips. On both, the
IR protocol handling is done inside the chip. On one device, if you press a key
and keep it pressed, it will produce just one scan code. So, that device has no
way to support IR repetition. The other one produces scan codes to indicate a
key press and a key release events.

If you try to handle those cases outside kernel, you'll need to have a complex
setup environment for users to specify or to detect such issues. However, if
this is done on kernel, you can just use the proper event api to handle such
differences.

There is just one issue here that it is not addressed currently: if your device
comes with an IR with, for example pulse distance protocol, but you bought a
RC5 IR and wants to use it with your device, you would need to say to the
driver (and maybe to the hardware) to use a different protocol for decoding.

On some cases where the IR decoding is done via software, if you get the raw
data, you can change the protocol easily. However, on other cases, this can be
done only if you program the device with a different setup.

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

It sets the default key tables. In fact, on most cases, the setting of the key
tables is done at the bridge driver. The ones inside ir-kbd-i2c are the
exceptions. Patches are welcome to remove those by adding such setup at the
bridge drivers, but extra care should be done to be sure that we won't break
any IR support.

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

The ir-kbd-i2c is just the top of the iceberg. It covers the minority of
devices where IR is implemented via an i2c chip. Most devices are
connected via GPIO.

Also, after Gerd stopping work on it, several improvements and changes on the
original driver, including moving the setup process to the bridge drivers.

It should be noticed that there are 4 types of IRs:

1) IR chips connected via GPIO;

2) Simpler ones that just connect the IR sensor directly on some GPIO port
capable of generating interrupts. This kind of setup is very common on saa7134
driver. It should be noticed that a bad implementation here will lead to
machine hangups, due to IRQ troubles;

3) IR chips connected via I2C;

4) bridge chips with IR decoding capabilities. For example, em28xx has RC5
decoding internally.

On several cases, the IR chip is a low-cost generic processor or ASIC, with
some IR decoding software programmed there (read-only, AFAIK).

I haven't look on lirc implementation lately, but I think that lirc can
effectively change the decoding protocol only on the case (2) where the IR raw
data is sent directly via a GPIO port. On the other cases, the IR chip will
handle the protocol. 

So, if you have an IR chip and you want to change the protocol, you'll need to
send commands via the bridge driver to set it differently. It is risky to let
an external driver to do this, since this may cause troubles to the device
driver, as the developers won't generally count that another driver is poking
around with setup commands.

Cheers,
Mauro

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Jean Delvare]

On Sun, 05 Apr 2009 23:22:03 +0200, drunk and tired hermann pitton wrote:
> 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?

-- 
Jean Delvare

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[hermann pitton]

Hi Jean,

Am Montag, den 06.04.2009, 10:40 +0200 schrieb Jean Delvare:
> On Sun, 05 Apr 2009 23:22:03 +0200, drunk and tired hermann pitton wrote:

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

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

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Jean Delvare]

On Mon, 06 Apr 2009 23:10:36 +0200, hermann pitton wrote:
> Am Montag, den 06.04.2009, 10:40 +0200 schrieb Jean Delvare:
> > 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.

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.

-- 
Jean Delvare

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[CityK]

Jean Delvare wrote:
> On Mon, 06 Apr 2009 23:10:36 +0200, hermann pitton wrote:
>   
>> Am Montag, den 06.04.2009, 10:40 +0200 schrieb Jean Delvare:
>>     
>>> 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.
>>     
>
> 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

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Mauro Carvalho Chehab]

On Tue, 07 Apr 2009 23:02:43 -0400
CityK <cityk@rogers.com> 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.

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[CityK]

Mauro Carvalho Chehab wrote:
> On Tue, 07 Apr 2009 23:02:43 -0400
> CityK <cityk@rogers.com> 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.

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.

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[hermann pitton]

Hi!

Am Sonntag, den 12.04.2009, 13:37 -0400 schrieb CityK:
> Mauro Carvalho Chehab wrote:
> > On Tue, 07 Apr 2009 23:02:43 -0400
> > CityK <cityk@rogers.com> 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.
> 
> 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.

Consistent with that, as from some first seen ever, the KS007 chip
remotes seem to have always more keys than the KS003 ones.

Cheers,
Hermann

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Oldrich Jedlicka]

On Monday 06 of April 2009 at 23:10:36, hermann pitton wrote:
> Hi Jean,
>
> Am Montag, den 06.04.2009, 10:40 +0200 schrieb Jean Delvare:
> > On Sun, 05 Apr 2009 23:22:03 +0200, drunk and tired hermann pitton wrote:
>
> don't tell me the French vine is always better.
> You likely know who introduced that currency once :)
>
> > > 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?

Hi Jean,

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.

Cheers,
Oldrich.

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

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Jean Delvare]

Hi Oldrich,

On Thu, 9 Apr 2009 21:15:30 +0200, Oldrich Jedlicka wrote:
> I've tried your patches with AverMedia Cardbus Hybrid (E506R) and they works 
> fine.

Thanks for testing and reporting, and sorry for the late answer.

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

OK, that makes sense. Many I2C devices only support a limited subset of
the I2C protocol, and if you try to address them with a message format
they don't support, their state machine goes into a bad state. That's
probably what was happening there. This is the reason why we should
always instantiate I2C devices explicitly when possible: whatever
probing method you use, you have no guarantee that every device will
like it.

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

I didn't know about this hack. The implementation choice seems wrong to
me. The hack should be triggered only when needed, rather than by
default with an exception for address 0x40. This goes beyond the scope
of my patch though, and I don't want to touch that kind of code without
hardware at hand to test my changes.

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

Thanks,
-- 
Jean Delvare

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Jarod Wilson]

On Sunday 05 April 2009 14:31:54 Janne Grunau wrote:
> On Sun, Apr 05, 2009 at 01:39:33PM -0400, Andy Walls wrote:
> > On Sun, 2009-04-05 at 16:37 +0200, Janne Grunau wrote:
> > > 
> > > 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.
> 
> I guess lkml would NAK patches adding infrastructure only bits but we
> will probably for the next patchset concentrate on a few lirc drivers.

Yep, my thoughts exactly.

> Christopher doesn't participate in the merge attempt.

Christoph has been giving decent feedback and merging the git tree changes
back into lirc cvs of late, but no, he's not directly participating in
the effort to merge lirc into the kernel.

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

Indeed, we should focus on serial, i2c/zilog, mceusb2 and imon. I think
they're by far the most popular and the best maintained, and between
Janne and myself, we can actually test all of them ourselves.

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

I think we've got just about everything handled, but I should go back over
the stack of comments before we resubmit... I was hoping to have something
ready for 2.6.30, but work keeps getting in the way...

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Mike Isely]

On Sun, 5 Apr 2009, Hans Verkuil wrote:

   [...]

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

Yes, and I will do the same for pvrusb2.  Simple is good.


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

It's probably going to break, and that will wake them up.  There's no 
choice otherwise.


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

Yes, I know this is a possibility.  It sucks and long term the new 
mechanism is the solution.  I don't want anyone to think I am against 
the new mechanism.  I'm for it.  But I'd like to minimize the damage 
potential on the way there.


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

  -Mike

-- 

Mike Isely
isely @ pobox (dot) com
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8

Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model

[Mauro Carvalho Chehab]

On Sun, 5 Apr 2009 13:48:02 -0500 (CDT)
Mike Isely <isely@isely.net> wrote:

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

This is the old approach: adding more stuff into ir-kbd-i2c. The new approach
is to let ir-kbd-i2c to prepare for IR, but filling the protocol decoding
routines and IR names after having it bound on i2c bus. So, the IR routines
will be properly filled by the bridge driver (pvrusb2, in this case).

> 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 hardcoded keytables are just the default ones for that keyboard. As I've
shown already in this thread, it can be easily replaced on userspace, and we
have already an userspace tool on v4l2-apps that replaces those tables.

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

Multiple mappings based on what userspace app you're using can't be done
internally. However, you can keep using lirc with event driver for those
cases where you want to have multiple mappings, and this works fine.

The only drawback i saw when I used lirc the last time (a long time ago) is
that, when you remove an usb device, it used to flood the logs with errors
(several errors by second, warning that event interface disappeared). Probably
they already solved this issue. 

Cheers,
Mauro

[PATCH 4/6] ir-kbd-i2c: Use initialization data

[Jean Delvare]

For specific boards, pass initialization data to ir-kbd-i2c instead
of modifying the settings after the device is initialized. This is
more efficient and easier to read.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
 linux/drivers/media/video/cx231xx/cx231xx-cards.c |   14 ---
 linux/drivers/media/video/cx231xx/cx231xx-i2c.c   |   29 ------
 linux/drivers/media/video/cx231xx/cx231xx.h       |    1 
 linux/drivers/media/video/em28xx/em28xx-cards.c   |   31 +++----
 linux/drivers/media/video/em28xx/em28xx-i2c.c     |   22 -----
 linux/drivers/media/video/em28xx/em28xx.h         |    1 
 linux/drivers/media/video/ir-kbd-i2c.c            |   12 ++
 linux/drivers/media/video/saa7134/saa7134-i2c.c   |   28 ------
 linux/drivers/media/video/saa7134/saa7134-input.c |   88 ++++++++++-----------
 linux/drivers/media/video/saa7134/saa7134.h       |    1 
 linux/include/media/ir-kbd-i2c.h                  |    7 +
 11 files changed, 76 insertions(+), 158 deletions(-)

--- v4l-dvb.orig/linux/drivers/media/video/cx231xx/cx231xx-cards.c	2009-04-04 09:20:21.000000000 +0200
+++ v4l-dvb/linux/drivers/media/video/cx231xx/cx231xx-cards.c	2009-04-04 09:20:23.000000000 +0200
@@ -282,20 +282,6 @@ static void cx231xx_config_tuner(struct
 }
 
 /* ----------------------------------------------------------------------- */
-void cx231xx_set_ir(struct cx231xx *dev, struct IR_i2c *ir)
-{
-	/* detect & configure */
-	switch (dev->model) {
-
-	case CX231XX_BOARD_CNXT_RDE_250:
-		break;
-	case CX231XX_BOARD_CNXT_RDU_250:
-		break;
-	default:
-		break;
-	}
-}
-
 void cx231xx_card_setup(struct cx231xx *dev)
 {
 
--- v4l-dvb.orig/linux/drivers/media/video/cx231xx/cx231xx-i2c.c	2009-04-04 09:20:18.000000000 +0200
+++ v4l-dvb/linux/drivers/media/video/cx231xx/cx231xx-i2c.c	2009-04-04 09:20:23.000000000 +0200
@@ -424,34 +424,6 @@ static u32 functionality(struct i2c_adap
 	return I2C_FUNC_SMBUS_EMUL | I2C_FUNC_I2C;
 }
 
-/*
- * attach_inform()
- * gets called when a device attaches to the i2c bus
- * does some basic configuration
- */
-static int attach_inform(struct i2c_client *client)
-{
-	struct cx231xx_i2c *bus = i2c_get_adapdata(client->adapter);
-	struct cx231xx *dev = bus->dev;
-
-	switch (client->addr << 1) {
-	case 0x8e:
-		{
-			struct IR_i2c *ir = i2c_get_clientdata(client);
-			dprintk1(1, "attach_inform: IR detected (%s).\n",
-				 ir->phys);
-			cx231xx_set_ir(dev, ir);
-			break;
-		}
-		break;
-
-	default:
-		break;
-	}
-
-	return 0;
-}
-
 static struct i2c_algorithm cx231xx_algo = {
 	.master_xfer = cx231xx_i2c_xfer,
 	.functionality = functionality,
@@ -465,7 +437,6 @@ static struct i2c_adapter cx231xx_adap_t
 	.name = "cx231xx",
 	.id = I2C_HW_B_CX231XX,
 	.algo = &cx231xx_algo,
-	.client_register = attach_inform,
 };
 
 static struct i2c_client cx231xx_client_template = {
--- v4l-dvb.orig/linux/drivers/media/video/cx231xx/cx231xx.h	2009-04-04 09:20:18.000000000 +0200
+++ v4l-dvb/linux/drivers/media/video/cx231xx/cx231xx.h	2009-04-04 09:20:23.000000000 +0200
@@ -747,7 +747,6 @@ extern void cx231xx_card_setup(struct cx
 extern struct cx231xx_board cx231xx_boards[];
 extern struct usb_device_id cx231xx_id_table[];
 extern const unsigned int cx231xx_bcount;
-void cx231xx_set_ir(struct cx231xx *dev, struct IR_i2c *ir);
 int cx231xx_tuner_callback(void *ptr, int component, int command, int arg);
 
 /* Provided by cx231xx-input.c */
--- v4l-dvb.orig/linux/drivers/media/video/em28xx/em28xx-cards.c	2009-04-04 09:20:21.000000000 +0200
+++ v4l-dvb/linux/drivers/media/video/em28xx/em28xx-cards.c	2009-04-04 09:54:59.000000000 +0200
@@ -1907,6 +1907,7 @@ static int em28xx_hint_board(struct em28
 void em28xx_register_i2c_ir(struct em28xx *dev)
 {
 	struct i2c_board_info info;
+	struct IR_i2c_init_data init_data;
 	const unsigned short addr_list[] = {
 		 0x30, 0x47, I2C_CLIENT_END
 	};
@@ -1915,12 +1916,9 @@ void em28xx_register_i2c_ir(struct em28x
 		return;
 
 	memset(&info, 0, sizeof(struct i2c_board_info));
+	memset(&init_data, 0, sizeof(struct IR_i2c_init_data));
 	strlcpy(info.type, "ir-kbd", I2C_NAME_SIZE);
-	i2c_new_probed_device(&dev->i2c_adap, &info, addr_list);
-}
 
-void em28xx_set_ir(struct em28xx *dev, struct IR_i2c *ir)
-{
 	/* detect & configure */
 	switch (dev->model) {
 	case (EM2800_BOARD_UNKNOWN):
@@ -1929,22 +1927,19 @@ void em28xx_set_ir(struct em28xx *dev, s
 		break;
 	case (EM2800_BOARD_TERRATEC_CINERGY_200):
 	case (EM2820_BOARD_TERRATEC_CINERGY_250):
-		ir->ir_codes = ir_codes_em_terratec;
-		ir->get_key = em28xx_get_key_terratec;
-		snprintf(ir->name, sizeof(ir->name),
-			 "i2c IR (EM28XX Terratec)");
+		init_data.ir_codes = ir_codes_em_terratec;
+		init_data.get_key = em28xx_get_key_terratec;
+		init_data.name = "i2c IR (EM28XX Terratec)";
 		break;
 	case (EM2820_BOARD_PINNACLE_USB_2):
-		ir->ir_codes = ir_codes_pinnacle_grey;
-		ir->get_key = em28xx_get_key_pinnacle_usb_grey;
-		snprintf(ir->name, sizeof(ir->name),
-			 "i2c IR (EM28XX Pinnacle PCTV)");
+		init_data.ir_codes = ir_codes_pinnacle_grey;
+		init_data.get_key = em28xx_get_key_pinnacle_usb_grey;
+		init_data.name = "i2c IR (EM28XX Pinnacle PCTV)";
 		break;
 	case (EM2820_BOARD_HAUPPAUGE_WINTV_USB_2):
-		ir->ir_codes = ir_codes_hauppauge_new;
-		ir->get_key = em28xx_get_key_em_haup;
-		snprintf(ir->name, sizeof(ir->name),
-			 "i2c IR (EM2840 Hauppauge)");
+		init_data.ir_codes = ir_codes_hauppauge_new;
+		init_data.get_key = em28xx_get_key_em_haup;
+		init_data.name = "i2c IR (EM2840 Hauppauge)";
 		break;
 	case (EM2820_BOARD_MSI_VOX_USB_2):
 		break;
@@ -1955,6 +1950,10 @@ void em28xx_set_ir(struct em28xx *dev, s
 	case (EM2800_BOARD_GRABBEEX_USB2800):
 		break;
 	}
+
+	if (init_data.name)
+		info.platform_data = &init_data;
+	i2c_new_probed_device(&dev->i2c_adap, &info, addr_list);
 }
 
 void em28xx_card_setup(struct em28xx *dev)
--- v4l-dvb.orig/linux/drivers/media/video/em28xx/em28xx-i2c.c	2009-04-04 09:20:21.000000000 +0200
+++ v4l-dvb/linux/drivers/media/video/em28xx/em28xx-i2c.c	2009-04-04 09:20:23.000000000 +0200
@@ -451,27 +451,6 @@ static u32 functionality(struct i2c_adap
 	return I2C_FUNC_SMBUS_EMUL;
 }
 
-/*
- * attach_inform()
- * gets called when a device attaches to the i2c bus
- * does some basic configuration
- */
-static int attach_inform(struct i2c_client *client)
-{
-	struct em28xx *dev = client->adapter->algo_data;
-	struct IR_i2c *ir = i2c_get_clientdata(client);
-
-	switch (client->addr << 1) {
-	case 0x60:
-	case 0x8e:
-		dprintk1(1, "attach_inform: IR detected (%s).\n", ir->phys);
-		em28xx_set_ir(dev, ir);
-		break;
-	}
-
-	return 0;
-}
-
 static struct i2c_algorithm em28xx_algo = {
 	.master_xfer   = em28xx_i2c_xfer,
 	.functionality = functionality,
@@ -488,7 +467,6 @@ static struct i2c_adapter em28xx_adap_te
 	.name = "em28xx",
 	.id = I2C_HW_B_EM28XX,
 	.algo = &em28xx_algo,
-	.client_register = attach_inform,
 };
 
 static struct i2c_client em28xx_client_template = {
--- v4l-dvb.orig/linux/drivers/media/video/em28xx/em28xx.h	2009-04-04 09:20:21.000000000 +0200
+++ v4l-dvb/linux/drivers/media/video/em28xx/em28xx.h	2009-04-04 09:20:23.000000000 +0200
@@ -649,7 +649,6 @@ extern struct em28xx_board em28xx_boards
 extern struct usb_device_id em28xx_id_table[];
 extern const unsigned int em28xx_bcount;
 void em28xx_register_i2c_ir(struct em28xx *dev);
-void em28xx_set_ir(struct em28xx *dev, struct IR_i2c *ir);
 int em28xx_tuner_callback(void *ptr, int component, int command, int arg);
 void em28xx_release_resources(struct em28xx *dev);
 
--- v4l-dvb.orig/linux/drivers/media/video/ir-kbd-i2c.c	2009-04-04 09:20:21.000000000 +0200
+++ v4l-dvb/linux/drivers/media/video/ir-kbd-i2c.c	2009-04-04 09:20:23.000000000 +0200
@@ -307,7 +307,7 @@ static void ir_work(struct work_struct *
 static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	IR_KEYTAB_TYPE *ir_codes = NULL;
-	char *name;
+	const char *name;
 	int ir_type;
 	struct IR_i2c *ir;
 	struct input_dev *input_dev;
@@ -395,6 +395,16 @@ static int ir_probe(struct i2c_client *c
 		goto err_out_free;
 	}
 
+	/* Let the caller override settings */
+	if (client->dev.platform_data) {
+		const struct IR_i2c_init_data *init_data =
+						client->dev.platform_data;
+
+		ir_codes = init_data->ir_codes;
+		name = init_data->name;
+		ir->get_key = init_data->get_key;
+	}
+
 	/* Sets name */
 	snprintf(ir->name, sizeof(ir->name), "i2c IR (%s)", name);
 	ir->ir_codes = ir_codes;
--- v4l-dvb.orig/linux/drivers/media/video/saa7134/saa7134-i2c.c	2009-04-04 09:20:21.000000000 +0200
+++ v4l-dvb/linux/drivers/media/video/saa7134/saa7134-i2c.c	2009-04-04 09:53:26.000000000 +0200
@@ -326,33 +326,6 @@ static u32 functionality(struct i2c_adap
 	return I2C_FUNC_SMBUS_EMUL;
 }
 
-static int attach_inform(struct i2c_client *client)
-{
-	struct saa7134_dev *dev = client->adapter->algo_data;
-
-	d1printk( "%s i2c attach [addr=0x%x,client=%s]\n",
-		client->driver->driver.name, client->addr, client->name);
-
-	/* Am I an i2c remote control? */
-
-	switch (client->addr) {
-		case 0x7a:
-		case 0x47:
-		case 0x71:
-		case 0x2d:
-		case 0x30:
-		{
-			struct IR_i2c *ir = i2c_get_clientdata(client);
-			d1printk("%s i2c IR detected (%s).\n",
-				 client->driver->driver.name, ir->phys);
-			saa7134_set_i2c_ir(dev,ir);
-			break;
-		}
-	}
-
-	return 0;
-}
-
 static struct i2c_algorithm saa7134_algo = {
 	.master_xfer   = saa7134_i2c_xfer,
 	.functionality = functionality,
@@ -369,7 +342,6 @@ static struct i2c_adapter saa7134_adap_t
 	.name          = "saa7134",
 	.id            = I2C_HW_SAA7134,
 	.algo          = &saa7134_algo,
-	.client_register = attach_inform,
 };
 
 static struct i2c_client saa7134_client_template = {
--- v4l-dvb.orig/linux/drivers/media/video/saa7134/saa7134-input.c	2009-04-04 09:20:21.000000000 +0200
+++ v4l-dvb/linux/drivers/media/video/saa7134/saa7134-input.c	2009-04-04 10:07:49.000000000 +0200
@@ -685,6 +685,7 @@ void saa7134_input_fini(struct saa7134_d
 void saa7134_probe_i2c_ir(struct saa7134_dev *dev)
 {
 	struct i2c_board_info info;
+	struct IR_i2c_init_data init_data;
 	const unsigned short addr_list[] = {
 		0x7a, 0x47, 0x71, 0x2d,
 		I2C_CLIENT_END
@@ -722,7 +723,49 @@ void saa7134_probe_i2c_ir(struct saa7134
 	}
 
 	memset(&info, 0, sizeof(struct i2c_board_info));
+	memset(&init_data, 0, sizeof(struct IR_i2c_init_data));
 	strlcpy(info.type, "ir-kbd", I2C_NAME_SIZE);
+
+	switch (dev->board) {
+	case SAA7134_BOARD_PINNACLE_PCTV_110i:
+	case SAA7134_BOARD_PINNACLE_PCTV_310i:
+		init_data.name = "Pinnacle PCTV";
+		if (pinnacle_remote == 0) {
+			init_data.get_key = get_key_pinnacle_color;
+			init_data.ir_codes = ir_codes_pinnacle_color;
+		} else {
+			init_data.get_key = get_key_pinnacle_grey;
+			init_data.ir_codes = ir_codes_pinnacle_grey;
+		}
+		break;
+	case SAA7134_BOARD_UPMOST_PURPLE_TV:
+		init_data.name = "Purple TV";
+		init_data.get_key = get_key_purpletv;
+		init_data.ir_codes = ir_codes_purpletv;
+		break;
+	case SAA7134_BOARD_MSI_TVATANYWHERE_PLUS:
+		init_data.name = "MSI TV@nywhere Plus";
+		init_data.get_key = get_key_msi_tvanywhere_plus;
+		init_data.ir_codes = ir_codes_msi_tvanywhere_plus;
+		break;
+	case SAA7134_BOARD_HAUPPAUGE_HVR1110:
+		init_data.name = "HVR 1110";
+		init_data.get_key = get_key_hvr1110;
+		init_data.ir_codes = ir_codes_hauppauge_new;
+		break;
+	case SAA7134_BOARD_BEHOLD_607_9FM:
+	case SAA7134_BOARD_BEHOLD_M6:
+	case SAA7134_BOARD_BEHOLD_M63:
+	case SAA7134_BOARD_BEHOLD_M6_EXTRA:
+	case SAA7134_BOARD_BEHOLD_H6:
+		init_data.name = "BeholdTV";
+		init_data.get_key = get_key_beholdm6xx;
+		init_data.ir_codes = ir_codes_behold;
+		break;
+	}
+
+	if (init_data.name)
+		info.platform_data = &init_data;
 	client = i2c_new_probed_device(&dev->i2c_adap, &info, addr_list);
 	if (client)
 		return;
@@ -750,51 +793,6 @@ void saa7134_probe_i2c_ir(struct saa7134
 	}
 }
 
-void saa7134_set_i2c_ir(struct saa7134_dev *dev, struct IR_i2c *ir)
-{
-	switch (dev->board) {
-	case SAA7134_BOARD_PINNACLE_PCTV_110i:
-	case SAA7134_BOARD_PINNACLE_PCTV_310i:
-		snprintf(ir->name, sizeof(ir->name), "Pinnacle PCTV");
-		if (pinnacle_remote == 0) {
-			ir->get_key   = get_key_pinnacle_color;
-			ir->ir_codes = ir_codes_pinnacle_color;
-		} else {
-			ir->get_key   = get_key_pinnacle_grey;
-			ir->ir_codes = ir_codes_pinnacle_grey;
-		}
-		break;
-	case SAA7134_BOARD_UPMOST_PURPLE_TV:
-		snprintf(ir->name, sizeof(ir->name), "Purple TV");
-		ir->get_key   = get_key_purpletv;
-		ir->ir_codes  = ir_codes_purpletv;
-		break;
-	case SAA7134_BOARD_MSI_TVATANYWHERE_PLUS:
-		snprintf(ir->name, sizeof(ir->name), "MSI TV@nywhere Plus");
-		ir->get_key  = get_key_msi_tvanywhere_plus;
-		ir->ir_codes = ir_codes_msi_tvanywhere_plus;
-		break;
-	case SAA7134_BOARD_HAUPPAUGE_HVR1110:
-		snprintf(ir->name, sizeof(ir->name), "HVR 1110");
-		ir->get_key   = get_key_hvr1110;
-		ir->ir_codes  = ir_codes_hauppauge_new;
-		break;
-	case SAA7134_BOARD_BEHOLD_607_9FM:
-	case SAA7134_BOARD_BEHOLD_M6:
-	case SAA7134_BOARD_BEHOLD_M63:
-	case SAA7134_BOARD_BEHOLD_M6_EXTRA:
-	case SAA7134_BOARD_BEHOLD_H6:
-		snprintf(ir->name, sizeof(ir->name), "BeholdTV");
-		ir->get_key   = get_key_beholdm6xx;
-		ir->ir_codes  = ir_codes_behold;
-		break;
-	default:
-		dprintk("Shouldn't get here: Unknown board %x for I2C IR?\n",dev->board);
-		break;
-	}
-
-}
-
 static int saa7134_rc5_irq(struct saa7134_dev *dev)
 {
 	struct card_ir *ir = dev->remote;
--- v4l-dvb.orig/linux/drivers/media/video/saa7134/saa7134.h	2009-04-04 09:20:21.000000000 +0200
+++ v4l-dvb/linux/drivers/media/video/saa7134/saa7134.h	2009-04-04 09:51:36.000000000 +0200
@@ -792,7 +792,6 @@ int  saa7134_input_init1(struct saa7134_
 void saa7134_input_fini(struct saa7134_dev *dev);
 void saa7134_input_irq(struct saa7134_dev *dev);
 void saa7134_probe_i2c_ir(struct saa7134_dev *dev);
-void saa7134_set_i2c_ir(struct saa7134_dev *dev, struct IR_i2c *ir);
 void saa7134_ir_start(struct saa7134_dev *dev, struct card_ir *ir);
 void saa7134_ir_stop(struct saa7134_dev *dev);
 
--- v4l-dvb.orig/linux/include/media/ir-kbd-i2c.h	2009-04-04 09:20:21.000000000 +0200
+++ v4l-dvb/linux/include/media/ir-kbd-i2c.h	2009-04-04 09:20:23.000000000 +0200
@@ -19,4 +19,11 @@ struct IR_i2c {
 	char                   phys[32];
 	int                    (*get_key)(struct IR_i2c*, u32*, u32*);
 };
+
+/* Passed when instantiating an ir-kbd i2c device */
+struct IR_i2c_init_data {
+	IR_KEYTAB_TYPE         *ir_codes;
+	const char             *name;
+	int                    (*get_key)(struct IR_i2c*, u32*, u32*);
+};
 #endif

-- 
Jean Delvare

[PATCH 5/6] saa7134: Simplify handling of IR on MSI TV@nywhere Plus

[Jean Delvare]

Now that we instantiate I2C IR devices explicitly, we can skip probing
altogether on boards where the I2C IR device address is known. The MSI
TV@nywhere Plus is one of these boards.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
 linux/drivers/media/video/saa7134/saa7134-input.c |   27 +++++----------------
 1 file changed, 7 insertions(+), 20 deletions(-)

--- v4l-dvb.orig/linux/drivers/media/video/saa7134/saa7134-input.c	2009-04-04 10:07:49.000000000 +0200
+++ v4l-dvb/linux/drivers/media/video/saa7134/saa7134-input.c	2009-04-04 10:22:14.000000000 +0200
@@ -691,16 +691,6 @@ void saa7134_probe_i2c_ir(struct saa7134
 		I2C_CLIENT_END
 	};
 
-	const unsigned short addr_list_msi[] = {
-		0x30, I2C_CLIENT_END
-	};
-	struct i2c_msg msg_msi = {
-		.addr = 0x50,
-		.flags = I2C_M_RD,
-		.len = 0,
-		.buf = NULL,
-	};
-
 	unsigned char subaddr, data;
 	struct i2c_msg msg_avermedia[] = { {
 		.addr = 0x40,
@@ -747,6 +737,7 @@ void saa7134_probe_i2c_ir(struct saa7134
 		init_data.name = "MSI TV@nywhere Plus";
 		init_data.get_key = get_key_msi_tvanywhere_plus;
 		init_data.ir_codes = ir_codes_msi_tvanywhere_plus;
+		info.addr = 0x30;
 		break;
 	case SAA7134_BOARD_HAUPPAUGE_HVR1110:
 		init_data.name = "HVR 1110";
@@ -766,18 +757,14 @@ void saa7134_probe_i2c_ir(struct saa7134
 
 	if (init_data.name)
 		info.platform_data = &init_data;
-	client = i2c_new_probed_device(&dev->i2c_adap, &info, addr_list);
-	if (client)
+	/* No need to probe if address is known */
+	if (info.addr) {
+		i2c_new_device(&dev->i2c_adap, &info);
 		return;
+	}
 
-	/* MSI TV@nywhere Plus controller doesn't seem to
-	   respond to probes unless we read something from
-	   an existing device. Weird... */
-	rc = i2c_transfer(&dev->i2c_adap, &msg_msi, 1);
-	dprintk(KERN_DEBUG "probe 0x%02x @ %s: %s\n",
-		msg_msi.addr, dev->i2c_adap.name,
-		(1 == rc) ? "yes" : "no");
-	client = i2c_new_probed_device(&dev->i2c_adap, &info, addr_list_msi);
+	/* Address not known, fallback to probing */
+	client = i2c_new_probed_device(&dev->i2c_adap, &info, addr_list);
 	if (client)
 		return;
 

-- 
Jean Delvare

[PATCH 6/6] saa7134: Simplify handling of IR on AVerMedia Cardbus

[Jean Delvare]

Now that we instantiate I2C IR devices explicitly, we can skip probing
altogether on boards where the I2C IR device address is known. The
AVerMedia Cardbus are two of these boards.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
 linux/drivers/media/video/saa7134/saa7134-input.c |   35 +++------------------
 1 file changed, 5 insertions(+), 30 deletions(-)

--- v4l-dvb.orig/linux/drivers/media/video/saa7134/saa7134-input.c	2009-04-04 10:41:44.000000000 +0200
+++ v4l-dvb/linux/drivers/media/video/saa7134/saa7134-input.c	2009-04-04 10:47:10.000000000 +0200
@@ -691,22 +691,6 @@ void saa7134_probe_i2c_ir(struct saa7134
 		I2C_CLIENT_END
 	};
 
-	unsigned char subaddr, data;
-	struct i2c_msg msg_avermedia[] = { {
-		.addr = 0x40,
-		.flags = 0,
-		.len = 1,
-		.buf = &subaddr,
-	}, {
-		.addr = 0x40,
-		.flags = I2C_M_RD,
-		.len = 1,
-		.buf = &data,
-	} };
-
-	struct i2c_client *client;
-	int rc;
-
 	if (disable_ir) {
 		dprintk("IR has been disabled, not probing for i2c remote\n");
 		return;
@@ -753,6 +737,10 @@ void saa7134_probe_i2c_ir(struct saa7134
 		init_data.get_key = get_key_beholdm6xx;
 		init_data.ir_codes = ir_codes_behold;
 		break;
+	case SAA7134_BOARD_AVERMEDIA_CARDBUS:
+	case SAA7134_BOARD_AVERMEDIA_CARDBUS_506:
+		info.addr = 0x40;
+		break;
 	}
 
 	if (init_data.name)
@@ -764,20 +752,7 @@ void saa7134_probe_i2c_ir(struct saa7134
 	}
 
 	/* Address not known, fallback to probing */
-	client = i2c_new_probed_device(&dev->i2c_adap, &info, addr_list);
-	if (client)
-		return;
-
-	/* Special case for AVerMedia Cardbus remote */
-	subaddr = 0x0d;
-	rc = i2c_transfer(&dev->i2c_adap, msg_avermedia, 2);
-	dprintk(KERN_DEBUG "probe 0x%02x/0x%02x @ %s: %s\n",
-		msg_avermedia[0].addr, subaddr, dev->i2c_adap.name,
-		(2 == rc) ? "yes" : "no");
-	if (2 == rc) {
-		info.addr = msg_avermedia[0].addr;
-		i2c_new_device(&dev->i2c_adap, &info);
-	}
+	i2c_new_probed_device(&dev->i2c_adap, &info, addr_list);
 }
 
 static int saa7134_rc5_irq(struct saa7134_dev *dev)

-- 
Jean Delvare

Re: [PATCH 0/6] ir-kbd-i2c conversion to the new i2c binding model

[Mike Isely]

Jean:

I understand what you're trying to do but how is LIRC expected to still 
work if all drivers now force the user over to ir-kbd?

  -Mike


On Sat, 4 Apr 2009, Jean Delvare wrote:

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

-- 

Mike Isely
isely @ pobox (dot) com
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8

Re: [PATCH 0/6] ir-kbd-i2c conversion to the new i2c binding model

[Mauro Carvalho Chehab]

On Sat, 4 Apr 2009 14:24:27 +0200
Jean Delvare <khali@linux-fr.org> wrote:

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

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.

Cheers,
Mauro

Re: [PATCH 0/6] ir-kbd-i2c conversion to the new i2c binding model

[Jean Delvare]

Hi Mauro,

On Sun, 5 Apr 2009 07:01:16 -0300, Mauro Carvalho Chehab wrote:
> 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;

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.

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

Yes, please! :)

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

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

Thanks,
-- 
Jean Delvare

Re: [PATCH 0/6] ir-kbd-i2c conversion to the new i2c binding model

[Mike Isely]

On Sun, 5 Apr 2009, Jean Delvare wrote:

> Hi Mauro,
> 
> On Sun, 5 Apr 2009 07:01:16 -0300, Mauro Carvalho Chehab wrote:
> > 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;
> 
> 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.
> 
> > 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;

It's an either/or.  If nothing is done, the ir-kbd-i2c become unusable 
for pvrusb2 but lirc (for now) continues to work.  If Jean's change is 
accepted as-is, then ir-kbd-i2c will be ok but now lirc is toast.  If I 
implement what I am suggesting, then it becomes possible at least for 
both cases to still work, but with a module option.  Not perfect, but it 
is the only way I see to allow this situation to retain some sanity.

In the longer term, the lirc folks are going to have to change what they 
are doing.  Fine, that's a problem they have to solve.  It's nothing I 
can do anyting about.  But I am not going to be the instigator that 
breaks lirc as used by the pvrusb2 driver.

In the short term, implementing the module option breaks the deadlock 
here.  Jean can continue getting rid of the old i2c model and I won't be 
a pain about it.


> > 
> > 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! :)
> 
> > 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.

Accept his ir-kbd-i2c conversion now, minus the pvrusb2 changes.  I will 
deal with the pvrusb2 driver appropriately (and immediately).  That 
should resolve the issue for the short term.


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

I agree.

  -Mike


-- 

Mike Isely
isely @ pobox (dot) com
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8

Test results for ir-kbd-i2c.c changes (Re: [PATCH 0/6] ir-kbd-i2c conversion to the new i2c binding model)

[Andy Walls]

On Sun, 2009-04-05 at 16:40 +0200, Jean Delvare wrote: 
> Hi Mauro,
> 
> On Sun, 5 Apr 2009 07:01:16 -0300, Mauro Carvalho Chehab wrote:

> > 
> > 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! :)

Jean,

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.

Regards,
Andy

Re: Test results for ir-kbd-i2c.c changes (Re: [PATCH 0/6] ir-kbd-i2c conversion to the new i2c binding model)

[Jean Delvare]

Hi Andy,

On Sun, 05 Apr 2009 20:22:59 -0400, Andy Walls wrote:
> 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.

Yeah, ir-kbd-i2c is messy, setup for some boards is done in the
ir-kbd-i2c driver itself and for some other boards it is done in the
bridge driver. I presume that the idea was to avoid code duplication on
one hand and bloating ir-kbd-i2c with board-specific functions on the
other hand. But the result is pretty confusing IMHO. There's room for
cleanups there, even though getting rid of the legacy model and merging
lirc into the kernel tree have higher priority at the moment.

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.

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

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!

-- 
Jean Delvare

Re: Test results for ir-kbd-i2c.c changes (Re: [PATCH 0/6] ir-kbd-i2c conversion to the new i2c binding model)

[Andy Walls]

On Mon, 2009-04-06 at 10:54 +0200, Jean Delvare wrote:
> 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.

Yeah, I could have mucked with it myself, but communicating back the
whole merged diff would have been a hassle.  I was just testing anyway.


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

Yeah.  So far one user has asked for it.


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

Regards,
Andy

Re: Test results for ir-kbd-i2c.c changes (Re: [PATCH 0/6] ir-kbd-i2c conversion to the new i2c binding model)

[Jean Delvare]

Hi Andy,

On Mon, 06 Apr 2009 07:56:22 -0400, Andy Walls wrote:
> On Mon, 2009-04-06 at 10:54 +0200, Jean Delvare wrote:
> > 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.

You must be kidding. For one thing, I never expected it to be a simple
change ;) For another, you're helping me a lot with your comments and
testing. Thanks!

-- 
Jean Delvare