[PATCH 0/3] i2c IR fixups

[PATCH 0/3] i2c IR fixups

[Jarod Wilson]

After these patches, both ir-kbd-i2c and lirc_zilog behave considerably
better with the HD-PVR and HVR-1950. I'd call the behavior of the 1950
perfect. The HD-PVR needs a touch more work, but these changes make
both RX and TX usable, its just a bit quirky still.

Jarod Wilson (3):
  hdpvr: fix up i2c device registration
  lirc_zilog: z8 on usb doesn't like back-to-back i2c_master_send
  ir-kbd-i2c: improve remote behavior with z8 behind usb

 drivers/media/video/hdpvr/hdpvr-core.c         |   21 ++++++++++++---
 drivers/media/video/hdpvr/hdpvr-i2c.c          |   28 +++++++++++++++------
 drivers/media/video/hdpvr/hdpvr.h              |    6 +++-
 drivers/media/video/ir-kbd-i2c.c               |   13 +++++++++
 drivers/media/video/pvrusb2/pvrusb2-i2c-core.c |    1 -
 drivers/staging/lirc/lirc_zilog.c              |   32 +++++++++++++++++++----
 6 files changed, 81 insertions(+), 20 deletions(-)

-- 
1.7.3.4

[PATCH 1/3] hdpvr: fix up i2c device registration

[Jarod Wilson]

We have to actually call i2c_new_device() once for each of the rx and tx
addresses. Also improve error-handling and device remove i2c cleanup.

Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/media/video/hdpvr/hdpvr-core.c |   21 +++++++++++++++++----
 drivers/media/video/hdpvr/hdpvr-i2c.c  |   28 ++++++++++++++++++++--------
 drivers/media/video/hdpvr/hdpvr.h      |    6 +++++-
 3 files changed, 42 insertions(+), 13 deletions(-)

diff --git a/drivers/media/video/hdpvr/hdpvr-core.c b/drivers/media/video/hdpvr/hdpvr-core.c
index a6572e5..f617a23 100644
--- a/drivers/media/video/hdpvr/hdpvr-core.c
+++ b/drivers/media/video/hdpvr/hdpvr-core.c
@@ -381,13 +381,21 @@ static int hdpvr_probe(struct usb_interface *interface,
 #if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
 	retval = hdpvr_register_i2c_adapter(dev);
 	if (retval < 0) {
-		v4l2_err(&dev->v4l2_dev, "registering i2c adapter failed\n");
+		v4l2_err(&dev->v4l2_dev, "i2c adapter register failed\n");
 		goto error;
 	}
 
-	retval = hdpvr_register_i2c_ir(dev);
-	if (retval < 0)
-		v4l2_err(&dev->v4l2_dev, "registering i2c IR devices failed\n");
+	hdpvr_register_ir_rx_i2c(dev);
+	if (!dev->i2c_rx) {
+		v4l2_err(&dev->v4l2_dev, "i2c IR RX device register failed\n");
+		goto reg_fail;
+	}
+
+	hdpvr_register_ir_tx_i2c(dev);
+	if (!dev->i2c_tx) {
+		v4l2_err(&dev->v4l2_dev, "i2c IR TX device register failed\n");
+		goto reg_fail;
+	}
 #endif
 
 	/* let the user know what node this device is now attached to */
@@ -395,6 +403,8 @@ static int hdpvr_probe(struct usb_interface *interface,
 		  video_device_node_name(dev->video_dev));
 	return 0;
 
+reg_fail:
+	i2c_del_adapter(&dev->i2c_adapter);
 error:
 	if (dev) {
 		/* Destroy single thread */
@@ -424,6 +434,9 @@ static void hdpvr_disconnect(struct usb_interface *interface)
 	mutex_lock(&dev->io_mutex);
 	hdpvr_cancel_queue(dev);
 	mutex_unlock(&dev->io_mutex);
+#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
+	i2c_del_adapter(&dev->i2c_adapter);
+#endif
 	video_unregister_device(dev->video_dev);
 	atomic_dec(&dev_nr);
 }
diff --git a/drivers/media/video/hdpvr/hdpvr-i2c.c b/drivers/media/video/hdpvr/hdpvr-i2c.c
index 89b71fa..e891bb0 100644
--- a/drivers/media/video/hdpvr/hdpvr-i2c.c
+++ b/drivers/media/video/hdpvr/hdpvr-i2c.c
@@ -31,26 +31,38 @@
 #define Z8F0811_IR_RX_I2C_ADDR	0x71
 
 
-static struct i2c_board_info hdpvr_i2c_board_info = {
+static struct i2c_board_info hdpvr_ir_tx_i2c_board_info = {
 	I2C_BOARD_INFO("ir_tx_z8f0811_hdpvr", Z8F0811_IR_TX_I2C_ADDR),
+};
+
+void hdpvr_register_ir_tx_i2c(struct hdpvr_device *dev)
+{
+	struct IR_i2c_init_data *init_data = &dev->ir_i2c_init_data;
+
+	init_data->name = "HD-PVR";
+	hdpvr_ir_tx_i2c_board_info.platform_data = init_data;
+
+	dev->i2c_tx = i2c_new_device(&dev->i2c_adapter,
+				     &hdpvr_ir_tx_i2c_board_info);
+}
+
+static struct i2c_board_info hdpvr_ir_rx_i2c_board_info = {
 	I2C_BOARD_INFO("ir_rx_z8f0811_hdpvr", Z8F0811_IR_RX_I2C_ADDR),
 };
 
-int hdpvr_register_i2c_ir(struct hdpvr_device *dev)
+void hdpvr_register_ir_rx_i2c(struct hdpvr_device *dev)
 {
-	struct i2c_client *c;
 	struct IR_i2c_init_data *init_data = &dev->ir_i2c_init_data;
 
 	/* Our default information for ir-kbd-i2c.c to use */
 	init_data->ir_codes = RC_MAP_HAUPPAUGE_NEW;
 	init_data->internal_get_key_func = IR_KBD_GET_KEY_HAUP_XVR;
 	init_data->type = RC_TYPE_RC5;
-	init_data->name = "HD PVR";
-	hdpvr_i2c_board_info.platform_data = init_data;
-
-	c = i2c_new_device(&dev->i2c_adapter, &hdpvr_i2c_board_info);
+	init_data->name = "HD-PVR";
+	hdpvr_ir_rx_i2c_board_info.platform_data = init_data;
 
-	return (c == NULL) ? -ENODEV : 0;
+	dev->i2c_rx = i2c_new_device(&dev->i2c_adapter,
+				     &hdpvr_ir_rx_i2c_board_info);
 }
 
 static int hdpvr_i2c_read(struct hdpvr_device *dev, int bus,
diff --git a/drivers/media/video/hdpvr/hdpvr.h b/drivers/media/video/hdpvr/hdpvr.h
index ee74e3b..41a579e 100644
--- a/drivers/media/video/hdpvr/hdpvr.h
+++ b/drivers/media/video/hdpvr/hdpvr.h
@@ -108,6 +108,9 @@ struct hdpvr_device {
 
 	/* I2C adapter */
 	struct i2c_adapter	i2c_adapter;
+	/* I2C clients */
+	struct i2c_client	*i2c_rx;
+	struct i2c_client	*i2c_tx;
 	/* I2C lock */
 	struct mutex		i2c_mutex;
 	/* I2C message buffer space */
@@ -313,7 +316,8 @@ int hdpvr_cancel_queue(struct hdpvr_device *dev);
 /* i2c adapter registration */
 int hdpvr_register_i2c_adapter(struct hdpvr_device *dev);
 
-int hdpvr_register_i2c_ir(struct hdpvr_device *dev);
+void hdpvr_register_ir_rx_i2c(struct hdpvr_device *dev);
+void hdpvr_register_ir_tx_i2c(struct hdpvr_device *dev);
 
 /*========================================================================*/
 /* buffer management */
-- 
1.7.3.4

[PATCH 2/3] lirc_zilog: z8 on usb doesn't like back-to-back i2c_master_send

[Jarod Wilson]

Both the HD-PVR and HVR-1950, driven by the hdpvr and pvrusb2 drivers
respectively, have a zilog z8 chip exposed via i2c. These are both
usb-connected devices, and on both of them, back-to-back i2c_master_send
calls that work fine with a z8 on a pci card fail with a -EIO, as the
chip isn't yet ready from the prior command. To cope with that, add a
delay and retry loop where necessary.

Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/staging/lirc/lirc_zilog.c |   32 ++++++++++++++++++++++++++------
 1 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/lirc/lirc_zilog.c b/drivers/staging/lirc/lirc_zilog.c
index 3fe5f41..0aad0d7 100644
--- a/drivers/staging/lirc/lirc_zilog.c
+++ b/drivers/staging/lirc/lirc_zilog.c
@@ -495,7 +495,7 @@ static int send_data_block(struct IR_tx *tx, unsigned char *data_block)
 /* send boot data to the IR TX device */
 static int send_boot_data(struct IR_tx *tx)
 {
-	int ret;
+	int ret, i;
 	unsigned char buf[4];
 
 	/* send the boot block */
@@ -503,7 +503,7 @@ static int send_boot_data(struct IR_tx *tx)
 	if (ret != 0)
 		return ret;
 
-	/* kick it off? */
+	/* Hit the go button to activate the new boot data */
 	buf[0] = 0x00;
 	buf[1] = 0x20;
 	ret = i2c_master_send(tx->c, buf, 2);
@@ -511,7 +511,19 @@ static int send_boot_data(struct IR_tx *tx)
 		zilog_error("i2c_master_send failed with %d\n", ret);
 		return ret < 0 ? ret : -EFAULT;
 	}
-	ret = i2c_master_send(tx->c, buf, 1);
+
+	/*
+	 * Wait for zilog to settle after hitting go post boot block upload.
+	 * Without this delay, the HD-PVR and HVR-1950 both return an -EIO
+	 * upon attempting to get firmware revision, and tx probe thus fails.
+	 */
+	for (i = 0; i < 10; i++) {
+		ret = i2c_master_send(tx->c, buf, 1);
+		if (ret == 1)
+			break;
+		udelay(100);
+	}
+
 	if (ret != 1) {
 		zilog_error("i2c_master_send failed with %d\n", ret);
 		return ret < 0 ? ret : -EFAULT;
@@ -523,8 +535,8 @@ static int send_boot_data(struct IR_tx *tx)
 		zilog_error("i2c_master_recv failed with %d\n", ret);
 		return 0;
 	}
-	if (buf[0] != 0x80) {
-		zilog_error("unexpected IR TX response: %02x\n", buf[0]);
+	if ((buf[0] != 0x80) && (buf[0] != 0xa0)) {
+		zilog_error("unexpected IR TX init response: %02x\n", buf[0]);
 		return 0;
 	}
 	zilog_notify("Zilog/Hauppauge IR blaster firmware version "
@@ -827,7 +839,15 @@ static int send_code(struct IR_tx *tx, unsigned int code, unsigned int key)
 		zilog_error("i2c_master_send failed with %d\n", ret);
 		return ret < 0 ? ret : -EFAULT;
 	}
-	ret = i2c_master_send(tx->c, buf, 1);
+
+	/* Give the z8 a moment to process data block */
+	for (i = 0; i < 10; i++) {
+		ret = i2c_master_send(tx->c, buf, 1);
+		if (ret == 1)
+			break;
+		udelay(100);
+	}
+
 	if (ret != 1) {
 		zilog_error("i2c_master_send failed with %d\n", ret);
 		return ret < 0 ? ret : -EFAULT;
-- 
1.7.3.4

[PATCH 3/3] ir-kbd-i2c: improve remote behavior with z8 behind usb

[Jarod Wilson]

Add the same "are you ready?" i2c_master_send() poll command to
get_key_haup_xvr found in lirc_zilog, which is apparently seen in
the Windows driver for the PVR-150 w/a z8. This stabilizes what is
received from both the HD-PVR and HVR-1950, even with their polling
intervals at the default of 100, thus the removal of the custom
260ms polling_interval in pvrusb2-i2c-core.c.

CC: Andy Walls <awalls@md.metrocast.net>
CC: Mike Isley <isley@isley.net>
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/media/video/ir-kbd-i2c.c               |   13 +++++++++++++
 drivers/media/video/pvrusb2/pvrusb2-i2c-core.c |    1 -
 2 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/ir-kbd-i2c.c b/drivers/media/video/ir-kbd-i2c.c
index d2b20ad..a221ad6 100644
--- a/drivers/media/video/ir-kbd-i2c.c
+++ b/drivers/media/video/ir-kbd-i2c.c
@@ -128,6 +128,19 @@ static int get_key_haup(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw)
 
 static int get_key_haup_xvr(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw)
 {
+	int ret;
+	unsigned char buf[1] = { 0 };
+
+	/*
+	 * This is the same apparent "are you ready?" poll command observed
+	 * watching Windows driver traffic and implemented in lirc_zilog. With
+	 * this added, we get far saner remote behavior with z8 chips on usb
+	 * connected devices, even with the default polling interval of 100ms.
+	 */
+	ret = i2c_master_send(ir->c, buf, 1);
+	if (ret != 1)
+		return (ret < 0) ? ret : -EINVAL;
+
 	return get_key_haup_common (ir, ir_key, ir_raw, 6, 3);
 }
 
diff --git a/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c b/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c
index ccc8849..451ecd4 100644
--- a/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c
+++ b/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c
@@ -597,7 +597,6 @@ static void pvr2_i2c_register_ir(struct pvr2_hdw *hdw)
 		init_data->internal_get_key_func = IR_KBD_GET_KEY_HAUP_XVR;
 		init_data->type                  = RC_TYPE_RC5;
 		init_data->name                  = hdw->hdw_desc->description;
-		init_data->polling_interval      = 260; /* ms From lirc_zilog */
 		/* IR Receiver */
 		info.addr          = 0x71;
 		info.platform_data = init_data;
-- 
1.7.3.4

Re: [PATCH 3/3] ir-kbd-i2c: improve remote behavior with z8 behind usb

[Jarod Wilson]

D'oh, butchered Mike's last name (and domain name), apologies...

On Thu, Jan 20, 2011 at 11:30:25PM -0500, Jarod Wilson wrote:
> Add the same "are you ready?" i2c_master_send() poll command to
> get_key_haup_xvr found in lirc_zilog, which is apparently seen in
> the Windows driver for the PVR-150 w/a z8. This stabilizes what is
> received from both the HD-PVR and HVR-1950, even with their polling
> intervals at the default of 100, thus the removal of the custom
> 260ms polling_interval in pvrusb2-i2c-core.c.
> 
> CC: Andy Walls <awalls@md.metrocast.net>
> CC: Mike Isely <isely@isely.net>
> Signed-off-by: Jarod Wilson <jarod@redhat.com>
> ---
>  drivers/media/video/ir-kbd-i2c.c               |   13 +++++++++++++
>  drivers/media/video/pvrusb2/pvrusb2-i2c-core.c |    1 -
>  2 files changed, 13 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/media/video/ir-kbd-i2c.c b/drivers/media/video/ir-kbd-i2c.c
> index d2b20ad..a221ad6 100644
> --- a/drivers/media/video/ir-kbd-i2c.c
> +++ b/drivers/media/video/ir-kbd-i2c.c
> @@ -128,6 +128,19 @@ static int get_key_haup(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw)
>  
>  static int get_key_haup_xvr(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw)
>  {
> +	int ret;
> +	unsigned char buf[1] = { 0 };
> +
> +	/*
> +	 * This is the same apparent "are you ready?" poll command observed
> +	 * watching Windows driver traffic and implemented in lirc_zilog. With
> +	 * this added, we get far saner remote behavior with z8 chips on usb
> +	 * connected devices, even with the default polling interval of 100ms.
> +	 */
> +	ret = i2c_master_send(ir->c, buf, 1);
> +	if (ret != 1)
> +		return (ret < 0) ? ret : -EINVAL;
> +
>  	return get_key_haup_common (ir, ir_key, ir_raw, 6, 3);
>  }
>  
> diff --git a/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c b/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c
> index ccc8849..451ecd4 100644
> --- a/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c
> +++ b/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c
> @@ -597,7 +597,6 @@ static void pvr2_i2c_register_ir(struct pvr2_hdw *hdw)
>  		init_data->internal_get_key_func = IR_KBD_GET_KEY_HAUP_XVR;
>  		init_data->type                  = RC_TYPE_RC5;
>  		init_data->name                  = hdw->hdw_desc->description;
> -		init_data->polling_interval      = 260; /* ms From lirc_zilog */
>  		/* IR Receiver */
>  		info.addr          = 0x71;
>  		info.platform_data = init_data;
> -- 
> 1.7.3.4
> 

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH 1/3] hdpvr: fix up i2c device registration

[Andy Walls]

On Thu, 2011-01-20 at 23:30 -0500, Jarod Wilson wrote:
> We have to actually call i2c_new_device() once for each of the rx and tx
> addresses. Also improve error-handling and device remove i2c cleanup.
> 
> Signed-off-by: Jarod Wilson <jarod@redhat.com>

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

I do have some comments, but the real show-stoppers are:

- '#if defined(..' for the i2c_del_adapter() in the error path
- A very unlikely race by using file scope data

See below.

> ---
>  drivers/media/video/hdpvr/hdpvr-core.c |   21 +++++++++++++++++----
>  drivers/media/video/hdpvr/hdpvr-i2c.c  |   28 ++++++++++++++++++++--------
>  drivers/media/video/hdpvr/hdpvr.h      |    6 +++++-
>  3 files changed, 42 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/video/hdpvr/hdpvr-core.c b/drivers/media/video/hdpvr/hdpvr-core.c
> index a6572e5..f617a23 100644
> --- a/drivers/media/video/hdpvr/hdpvr-core.c
> +++ b/drivers/media/video/hdpvr/hdpvr-core.c
> @@ -381,13 +381,21 @@ static int hdpvr_probe(struct usb_interface *interface,
>  #if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
>  	retval = hdpvr_register_i2c_adapter(dev);
>  	if (retval < 0) {
> -		v4l2_err(&dev->v4l2_dev, "registering i2c adapter failed\n");
> +		v4l2_err(&dev->v4l2_dev, "i2c adapter register failed\n");
>  		goto error;
>  	}
>  
> -	retval = hdpvr_register_i2c_ir(dev);
> -	if (retval < 0)
> -		v4l2_err(&dev->v4l2_dev, "registering i2c IR devices failed\n");
> +	hdpvr_register_ir_rx_i2c(dev);
> +	if (!dev->i2c_rx) {
> +		v4l2_err(&dev->v4l2_dev, "i2c IR RX device register failed\n");
> +		goto reg_fail;
> +	}
> +
> +	hdpvr_register_ir_tx_i2c(dev);
> +	if (!dev->i2c_tx) {
> +		v4l2_err(&dev->v4l2_dev, "i2c IR TX device register failed\n");
> +		goto reg_fail;
> +	}

Once your driver is debugged and complete, there is really never a need
to save pointers to the i2c_clients.  You might want to consider having
void hdpvr_register_ir_?x_i2c() instead return a pointer that you can
check against NULL.

>  #endif
>  
>  	/* let the user know what node this device is now attached to */
> @@ -395,6 +403,8 @@ static int hdpvr_probe(struct usb_interface *interface,
>  		  video_device_node_name(dev->video_dev));
>  	return 0;
>  
> +reg_fail:
> +	i2c_del_adapter(&dev->i2c_adapter);

Don't you need an '#if defined(CONFIG_I2C)...' here, in case the symbol
does not exist?

>  error:
>  	if (dev) {
>  		/* Destroy single thread */
> @@ -424,6 +434,9 @@ static void hdpvr_disconnect(struct usb_interface *interface)
>  	mutex_lock(&dev->io_mutex);
>  	hdpvr_cancel_queue(dev);
>  	mutex_unlock(&dev->io_mutex);
> +#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
> +	i2c_del_adapter(&dev->i2c_adapter);
> +#endif
>  	video_unregister_device(dev->video_dev);
>  	atomic_dec(&dev_nr);
>  }
> diff --git a/drivers/media/video/hdpvr/hdpvr-i2c.c b/drivers/media/video/hdpvr/hdpvr-i2c.c
> index 89b71fa..e891bb0 100644
> --- a/drivers/media/video/hdpvr/hdpvr-i2c.c
> +++ b/drivers/media/video/hdpvr/hdpvr-i2c.c
> @@ -31,26 +31,38 @@
>  #define Z8F0811_IR_RX_I2C_ADDR	0x71
>  
> 
> -static struct i2c_board_info hdpvr_i2c_board_info = {
> +static struct i2c_board_info hdpvr_ir_tx_i2c_board_info = {
>  	I2C_BOARD_INFO("ir_tx_z8f0811_hdpvr", Z8F0811_IR_TX_I2C_ADDR),
> +};

This does not need to be file scope.  (In fact this creates a hard to
trigger race in the function below.)

It should be non-static function scope, as the I2C subsystem will make a
copy of the string and address byte and the platform data pointer when
you add the I2C device.    See pvrusb2.



> +void hdpvr_register_ir_tx_i2c(struct hdpvr_device *dev)
> +{
> +	struct IR_i2c_init_data *init_data = &dev->ir_i2c_init_data;
> +
> +	init_data->name = "HD-PVR";
> +	hdpvr_ir_tx_i2c_board_info.platform_data = init_data;

If hdpvr_ir_tx_i2c_board_info is file scope and static, then you have a
race for the platform data pointer when two hdpvr's are instantiated
simultaneously.  It'll be extremely rare, I know, but it is possible.

You should use a function scope copy of the board info.

The per device instance of the init_data is correct.

> +	dev->i2c_tx = i2c_new_device(&dev->i2c_adapter,
> +				     &hdpvr_ir_tx_i2c_board_info);
> +}

Again maybe just have this function 'return  i2c_new_device(...);'.
The caller can save away the i2c_client, if you want to keep a pointer
around.


> +static struct i2c_board_info hdpvr_ir_rx_i2c_board_info = {
>  	I2C_BOARD_INFO("ir_rx_z8f0811_hdpvr", Z8F0811_IR_RX_I2C_ADDR),
>  };
>  
> -int hdpvr_register_i2c_ir(struct hdpvr_device *dev)
> +void hdpvr_register_ir_rx_i2c(struct hdpvr_device *dev)
>  {
> -	struct i2c_client *c;
>  	struct IR_i2c_init_data *init_data = &dev->ir_i2c_init_data;
>  
>  	/* Our default information for ir-kbd-i2c.c to use */
>  	init_data->ir_codes = RC_MAP_HAUPPAUGE_NEW;
>  	init_data->internal_get_key_func = IR_KBD_GET_KEY_HAUP_XVR;
>  	init_data->type = RC_TYPE_RC5;
> -	init_data->name = "HD PVR";
> -	hdpvr_i2c_board_info.platform_data = init_data;
> -
> -	c = i2c_new_device(&dev->i2c_adapter, &hdpvr_i2c_board_info);
> +	init_data->name = "HD-PVR";
> +	hdpvr_ir_rx_i2c_board_info.platform_data = init_data;
>  
> -	return (c == NULL) ? -ENODEV : 0;
> +	dev->i2c_rx = i2c_new_device(&dev->i2c_adapter,
> +				     &hdpvr_ir_rx_i2c_board_info);
>  }

Same comments as for Rx.



>  static int hdpvr_i2c_read(struct hdpvr_device *dev, int bus,
> diff --git a/drivers/media/video/hdpvr/hdpvr.h b/drivers/media/video/hdpvr/hdpvr.h
> index ee74e3b..41a579e 100644
> --- a/drivers/media/video/hdpvr/hdpvr.h
> +++ b/drivers/media/video/hdpvr/hdpvr.h
> @@ -108,6 +108,9 @@ struct hdpvr_device {
>  
>  	/* I2C adapter */
>  	struct i2c_adapter	i2c_adapter;
> +	/* I2C clients */
> +	struct i2c_client	*i2c_rx;
> +	struct i2c_client	*i2c_tx;

I don't see where you are using these.  If you need them, you may want
to rename them to i2c_ir_rx and i2c_ir_tx, if hdpvr will ever talk to
another device on the I2C bus.

>  	/* I2C lock */
>  	struct mutex		i2c_mutex;
>  	/* I2C message buffer space */
> @@ -313,7 +316,8 @@ int hdpvr_cancel_queue(struct hdpvr_device *dev);
>  /* i2c adapter registration */
>  int hdpvr_register_i2c_adapter(struct hdpvr_device *dev);
>  
> -int hdpvr_register_i2c_ir(struct hdpvr_device *dev);
> +void hdpvr_register_ir_rx_i2c(struct hdpvr_device *dev);
> +void hdpvr_register_ir_tx_i2c(struct hdpvr_device *dev);
>  
>  /*========================================================================*/
>  /* buffer management */

Regards,
Andy

Re: [PATCH 2/3] lirc_zilog: z8 on usb doesn't like back-to-back i2c_master_send

[Andy Walls]

On Thu, 2011-01-20 at 23:30 -0500, Jarod Wilson wrote:
> Both the HD-PVR and HVR-1950, driven by the hdpvr and pvrusb2 drivers
> respectively, have a zilog z8 chip exposed via i2c. These are both
> usb-connected devices, and on both of them, back-to-back i2c_master_send
> calls that work fine with a z8 on a pci card fail with a -EIO, as the
> chip isn't yet ready from the prior command. To cope with that, add a
> delay and retry loop where necessary.
> 
> Signed-off-by: Jarod Wilson <jarod@redhat.com>

I haven't tested it, but it looks good.

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


> ---
>  drivers/staging/lirc/lirc_zilog.c |   32 ++++++++++++++++++++++++++------
>  1 files changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/lirc/lirc_zilog.c b/drivers/staging/lirc/lirc_zilog.c
> index 3fe5f41..0aad0d7 100644
> --- a/drivers/staging/lirc/lirc_zilog.c
> +++ b/drivers/staging/lirc/lirc_zilog.c
> @@ -495,7 +495,7 @@ static int send_data_block(struct IR_tx *tx, unsigned char *data_block)
>  /* send boot data to the IR TX device */
>  static int send_boot_data(struct IR_tx *tx)
>  {
> -	int ret;
> +	int ret, i;
>  	unsigned char buf[4];
>  
>  	/* send the boot block */
> @@ -503,7 +503,7 @@ static int send_boot_data(struct IR_tx *tx)
>  	if (ret != 0)
>  		return ret;
>  
> -	/* kick it off? */
> +	/* Hit the go button to activate the new boot data */
>  	buf[0] = 0x00;
>  	buf[1] = 0x20;
>  	ret = i2c_master_send(tx->c, buf, 2);
> @@ -511,7 +511,19 @@ static int send_boot_data(struct IR_tx *tx)
>  		zilog_error("i2c_master_send failed with %d\n", ret);
>  		return ret < 0 ? ret : -EFAULT;
>  	}
> -	ret = i2c_master_send(tx->c, buf, 1);
> +
> +	/*
> +	 * Wait for zilog to settle after hitting go post boot block upload.
> +	 * Without this delay, the HD-PVR and HVR-1950 both return an -EIO
> +	 * upon attempting to get firmware revision, and tx probe thus fails.
> +	 */
> +	for (i = 0; i < 10; i++) {
> +		ret = i2c_master_send(tx->c, buf, 1);
> +		if (ret == 1)
> +			break;
> +		udelay(100);
> +	}
> +
>  	if (ret != 1) {
>  		zilog_error("i2c_master_send failed with %d\n", ret);
>  		return ret < 0 ? ret : -EFAULT;
> @@ -523,8 +535,8 @@ static int send_boot_data(struct IR_tx *tx)
>  		zilog_error("i2c_master_recv failed with %d\n", ret);
>  		return 0;
>  	}
> -	if (buf[0] != 0x80) {
> -		zilog_error("unexpected IR TX response: %02x\n", buf[0]);
> +	if ((buf[0] != 0x80) && (buf[0] != 0xa0)) {
> +		zilog_error("unexpected IR TX init response: %02x\n", buf[0]);
>  		return 0;
>  	}
>  	zilog_notify("Zilog/Hauppauge IR blaster firmware version "
> @@ -827,7 +839,15 @@ static int send_code(struct IR_tx *tx, unsigned int code, unsigned int key)
>  		zilog_error("i2c_master_send failed with %d\n", ret);
>  		return ret < 0 ? ret : -EFAULT;
>  	}
> -	ret = i2c_master_send(tx->c, buf, 1);
> +
> +	/* Give the z8 a moment to process data block */
> +	for (i = 0; i < 10; i++) {
> +		ret = i2c_master_send(tx->c, buf, 1);
> +		if (ret == 1)
> +			break;
> +		udelay(100);
> +	}
> +
>  	if (ret != 1) {
>  		zilog_error("i2c_master_send failed with %d\n", ret);
>  		return ret < 0 ? ret : -EFAULT;

Re: [PATCH 3/3] ir-kbd-i2c: improve remote behavior with z8 behind usb

[Andy Walls]

On Thu, 2011-01-20 at 23:30 -0500, Jarod Wilson wrote:
> Add the same "are you ready?" i2c_master_send() poll command to
> get_key_haup_xvr found in lirc_zilog, which is apparently seen in
> the Windows driver for the PVR-150 w/a z8. This stabilizes what is
> received from both the HD-PVR and HVR-1950, even with their polling
> intervals at the default of 100, thus the removal of the custom
> 260ms polling_interval in pvrusb2-i2c-core.c.
> 
> CC: Andy Walls <awalls@md.metrocast.net>
> CC: Mike Isley <isley@isley.net>
> Signed-off-by: Jarod Wilson <jarod@redhat.com>

I haven't tested it, but it looks good.

The only thing to worry about is accidentally writing a 0 to register 0
at address 0x71 and maybe losing an Rx button press.  (However, for that
to happen, the Z8 would have already screwed up in its role of an I2C
slave anyway, or the I2C master did not honor the Z8's clock stretches.)

Since it makes things better and lirc_zilog already does it anyway...

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

> ---
>  drivers/media/video/ir-kbd-i2c.c               |   13 +++++++++++++
>  drivers/media/video/pvrusb2/pvrusb2-i2c-core.c |    1 -
>  2 files changed, 13 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/media/video/ir-kbd-i2c.c b/drivers/media/video/ir-kbd-i2c.c
> index d2b20ad..a221ad6 100644
> --- a/drivers/media/video/ir-kbd-i2c.c
> +++ b/drivers/media/video/ir-kbd-i2c.c
> @@ -128,6 +128,19 @@ static int get_key_haup(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw)
>  
>  static int get_key_haup_xvr(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw)
>  {
> +	int ret;
> +	unsigned char buf[1] = { 0 };
> +
> +	/*
> +	 * This is the same apparent "are you ready?" poll command observed
> +	 * watching Windows driver traffic and implemented in lirc_zilog. With
> +	 * this added, we get far saner remote behavior with z8 chips on usb
> +	 * connected devices, even with the default polling interval of 100ms.
> +	 */
> +	ret = i2c_master_send(ir->c, buf, 1);
> +	if (ret != 1)
> +		return (ret < 0) ? ret : -EINVAL;
> +
>  	return get_key_haup_common (ir, ir_key, ir_raw, 6, 3);
>  }
>  
> diff --git a/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c b/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c
> index ccc8849..451ecd4 100644
> --- a/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c
> +++ b/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c
> @@ -597,7 +597,6 @@ static void pvr2_i2c_register_ir(struct pvr2_hdw *hdw)
>  		init_data->internal_get_key_func = IR_KBD_GET_KEY_HAUP_XVR;
>  		init_data->type                  = RC_TYPE_RC5;
>  		init_data->name                  = hdw->hdw_desc->description;
> -		init_data->polling_interval      = 260; /* ms From lirc_zilog */
>  		/* IR Receiver */
>  		info.addr          = 0x71;
>  		info.platform_data = init_data;

Re: [PATCH 1/3] hdpvr: fix up i2c device registration

[Jarod Wilson]

On Fri, Jan 21, 2011 at 08:34:58AM -0500, Andy Walls wrote:
> On Thu, 2011-01-20 at 23:30 -0500, Jarod Wilson wrote:
> > We have to actually call i2c_new_device() once for each of the rx and tx
> > addresses. Also improve error-handling and device remove i2c cleanup.
> > 
> > Signed-off-by: Jarod Wilson <jarod@redhat.com>
> 
> Reviewed-by: Andy Walls <awalls@md.metrocast.net>
> 
> I do have some comments, but the real show-stoppers are:
> 
> - '#if defined(..' for the i2c_del_adapter() in the error path
> - A very unlikely race by using file scope data
> 
> See below.
> 
> > ---
> >  drivers/media/video/hdpvr/hdpvr-core.c |   21 +++++++++++++++++----
> >  drivers/media/video/hdpvr/hdpvr-i2c.c  |   28 ++++++++++++++++++++--------
> >  drivers/media/video/hdpvr/hdpvr.h      |    6 +++++-
> >  3 files changed, 42 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/media/video/hdpvr/hdpvr-core.c b/drivers/media/video/hdpvr/hdpvr-core.c
> > index a6572e5..f617a23 100644
> > --- a/drivers/media/video/hdpvr/hdpvr-core.c
> > +++ b/drivers/media/video/hdpvr/hdpvr-core.c
> > @@ -381,13 +381,21 @@ static int hdpvr_probe(struct usb_interface *interface,
> >  #if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
> >  	retval = hdpvr_register_i2c_adapter(dev);
> >  	if (retval < 0) {
> > -		v4l2_err(&dev->v4l2_dev, "registering i2c adapter failed\n");
> > +		v4l2_err(&dev->v4l2_dev, "i2c adapter register failed\n");
> >  		goto error;
> >  	}
> >  
> > -	retval = hdpvr_register_i2c_ir(dev);
> > -	if (retval < 0)
> > -		v4l2_err(&dev->v4l2_dev, "registering i2c IR devices failed\n");
> > +	hdpvr_register_ir_rx_i2c(dev);
> > +	if (!dev->i2c_rx) {
> > +		v4l2_err(&dev->v4l2_dev, "i2c IR RX device register failed\n");
> > +		goto reg_fail;
> > +	}
> > +
> > +	hdpvr_register_ir_tx_i2c(dev);
> > +	if (!dev->i2c_tx) {
> > +		v4l2_err(&dev->v4l2_dev, "i2c IR TX device register failed\n");
> > +		goto reg_fail;
> > +	}
> 
> Once your driver is debugged and complete, there is really never a need
> to save pointers to the i2c_clients.  You might want to consider having
> void hdpvr_register_ir_?x_i2c() instead return a pointer that you can
> check against NULL.

Yeah, I'd saved them when I was thinking I might need to call
i2c_unregister_device on driver removal, then debated dropping them, but
left them in, just in case. If you're certain there's never a particularly
good reason to keep them around, I'll drop them. An earlier iteration of
this patch did return a struct i2c_client for hdpvr_probe to check for
NULL.


> >  #endif
> >  
> >  	/* let the user know what node this device is now attached to */
> > @@ -395,6 +403,8 @@ static int hdpvr_probe(struct usb_interface *interface,
> >  		  video_device_node_name(dev->video_dev));
> >  	return 0;
> >  
> > +reg_fail:
> > +	i2c_del_adapter(&dev->i2c_adapter);
> 
> Don't you need an '#if defined(CONFIG_I2C)...' here, in case the symbol
> does not exist?

Crap, yeah, thought I was clever catching it in the remove path, missed it
here... :)


> > -static struct i2c_board_info hdpvr_i2c_board_info = {
> > +static struct i2c_board_info hdpvr_ir_tx_i2c_board_info = {
> >  	I2C_BOARD_INFO("ir_tx_z8f0811_hdpvr", Z8F0811_IR_TX_I2C_ADDR),
> > +};
> 
> This does not need to be file scope.  (In fact this creates a hard to
> trigger race in the function below.)
> 
> It should be non-static function scope, as the I2C subsystem will make a
> copy of the string and address byte and the platform data pointer when
> you add the I2C device.    See pvrusb2.

Okay, will go with code that looks more like pvrusb2.

Thanks for the review, will get a v2 together shortly, sanity-check and
post it...

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH 3/3] ir-kbd-i2c: improve remote behavior with z8 behind usb

[Mike Isely]

The pvrusb2 change is obviously trivial so I have no issue with it.

Acked-By: Mike Isely <isely@pobox.com>

Note the spelling of my last name "Isely" not "Isley".  A good way to 
remember is to think of the normal word "wisely" and just drop the 
leading "w".  (And yes, isely@isely.net and isely@pobox.com lead to the 
same inbox.)

  -Mike


On Thu, 20 Jan 2011, Jarod Wilson wrote:

> Add the same "are you ready?" i2c_master_send() poll command to
> get_key_haup_xvr found in lirc_zilog, which is apparently seen in
> the Windows driver for the PVR-150 w/a z8. This stabilizes what is
> received from both the HD-PVR and HVR-1950, even with their polling
> intervals at the default of 100, thus the removal of the custom
> 260ms polling_interval in pvrusb2-i2c-core.c.
> 
> CC: Andy Walls <awalls@md.metrocast.net>
> CC: Mike Isley <isley@isley.net>
> Signed-off-by: Jarod Wilson <jarod@redhat.com>
> ---
>  drivers/media/video/ir-kbd-i2c.c               |   13 +++++++++++++
>  drivers/media/video/pvrusb2/pvrusb2-i2c-core.c |    1 -
>  2 files changed, 13 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/media/video/ir-kbd-i2c.c b/drivers/media/video/ir-kbd-i2c.c
> index d2b20ad..a221ad6 100644
> --- a/drivers/media/video/ir-kbd-i2c.c
> +++ b/drivers/media/video/ir-kbd-i2c.c
> @@ -128,6 +128,19 @@ static int get_key_haup(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw)
>  
>  static int get_key_haup_xvr(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw)
>  {
> +	int ret;
> +	unsigned char buf[1] = { 0 };
> +
> +	/*
> +	 * This is the same apparent "are you ready?" poll command observed
> +	 * watching Windows driver traffic and implemented in lirc_zilog. With
> +	 * this added, we get far saner remote behavior with z8 chips on usb
> +	 * connected devices, even with the default polling interval of 100ms.
> +	 */
> +	ret = i2c_master_send(ir->c, buf, 1);
> +	if (ret != 1)
> +		return (ret < 0) ? ret : -EINVAL;
> +
>  	return get_key_haup_common (ir, ir_key, ir_raw, 6, 3);
>  }
>  
> diff --git a/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c b/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c
> index ccc8849..451ecd4 100644
> --- a/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c
> +++ b/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c
> @@ -597,7 +597,6 @@ static void pvr2_i2c_register_ir(struct pvr2_hdw *hdw)
>  		init_data->internal_get_key_func = IR_KBD_GET_KEY_HAUP_XVR;
>  		init_data->type                  = RC_TYPE_RC5;
>  		init_data->name                  = hdw->hdw_desc->description;
> -		init_data->polling_interval      = 260; /* ms From lirc_zilog */
>  		/* IR Receiver */
>  		info.addr          = 0x71;
>  		info.platform_data = init_data;
> 

-- 

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

Re: [PATCH 3/3] ir-kbd-i2c: improve remote behavior with z8 behind usb

[Jarod Wilson]

On Fri, Jan 21, 2011 at 10:31:42AM -0600, Mike Isely wrote:
> 
> The pvrusb2 change is obviously trivial so I have no issue with it.
> 
> Acked-By: Mike Isely <isely@pobox.com>
> 
> Note the spelling of my last name "Isely" not "Isley".  A good way to 
> remember is to think of the normal word "wisely" and just drop the 
> leading "w".  (And yes, isely@isely.net and isely@pobox.com lead to the 
> same inbox.)

Thanks Mike, apologies about the misspelling, I didn't catch it until
after I hit send. I had the Isley Brothers in my head. :)


> On Thu, 20 Jan 2011, Jarod Wilson wrote:
> 
> > Add the same "are you ready?" i2c_master_send() poll command to
> > get_key_haup_xvr found in lirc_zilog, which is apparently seen in
> > the Windows driver for the PVR-150 w/a z8. This stabilizes what is
> > received from both the HD-PVR and HVR-1950, even with their polling
> > intervals at the default of 100, thus the removal of the custom
> > 260ms polling_interval in pvrusb2-i2c-core.c.
> > 
> > CC: Andy Walls <awalls@md.metrocast.net>
> > CC: Mike Isley <isley@isley.net>
> > Signed-off-by: Jarod Wilson <jarod@redhat.com>
> > ---
> >  drivers/media/video/ir-kbd-i2c.c               |   13 +++++++++++++
> >  drivers/media/video/pvrusb2/pvrusb2-i2c-core.c |    1 -
> >  2 files changed, 13 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/media/video/ir-kbd-i2c.c b/drivers/media/video/ir-kbd-i2c.c
> > index d2b20ad..a221ad6 100644
> > --- a/drivers/media/video/ir-kbd-i2c.c
> > +++ b/drivers/media/video/ir-kbd-i2c.c
> > @@ -128,6 +128,19 @@ static int get_key_haup(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw)
> >  
> >  static int get_key_haup_xvr(struct IR_i2c *ir, u32 *ir_key, u32 *ir_raw)
> >  {
> > +	int ret;
> > +	unsigned char buf[1] = { 0 };
> > +
> > +	/*
> > +	 * This is the same apparent "are you ready?" poll command observed
> > +	 * watching Windows driver traffic and implemented in lirc_zilog. With
> > +	 * this added, we get far saner remote behavior with z8 chips on usb
> > +	 * connected devices, even with the default polling interval of 100ms.
> > +	 */
> > +	ret = i2c_master_send(ir->c, buf, 1);
> > +	if (ret != 1)
> > +		return (ret < 0) ? ret : -EINVAL;
> > +
> >  	return get_key_haup_common (ir, ir_key, ir_raw, 6, 3);
> >  }
> >  
> > diff --git a/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c b/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c
> > index ccc8849..451ecd4 100644
> > --- a/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c
> > +++ b/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c
> > @@ -597,7 +597,6 @@ static void pvr2_i2c_register_ir(struct pvr2_hdw *hdw)
> >  		init_data->internal_get_key_func = IR_KBD_GET_KEY_HAUP_XVR;
> >  		init_data->type                  = RC_TYPE_RC5;
> >  		init_data->name                  = hdw->hdw_desc->description;
> > -		init_data->polling_interval      = 260; /* ms From lirc_zilog */
> >  		/* IR Receiver */
> >  		info.addr          = 0x71;
> >  		info.platform_data = init_data;
> > 
> 
> -- 
> 
> Mike Isely
> isely @ isely (dot) net
> PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH 3/3] ir-kbd-i2c: improve remote behavior with z8 behind usb

[Mike Isely]

On Fri, 21 Jan 2011, Mike Isely wrote:

> 
> Note the spelling of my last name "Isely" not "Isley".  A good way to 
> remember is to think of the normal word "wisely" and just drop the 
> leading "w".  (And yes, isely@isely.net and isely@pobox.com lead to the 
> same inbox.)

And of course having said that, I then failed to fix the cc list.  
Sorry about that.  D'Oh!!!

  -Mike

-- 

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

Re: [PATCH 3/3] ir-kbd-i2c: improve remote behavior with z8 behind usb

[Mike Isely]

On Fri, 21 Jan 2011, Jarod Wilson wrote:

> On Fri, Jan 21, 2011 at 10:31:42AM -0600, Mike Isely wrote:
> > 
> > The pvrusb2 change is obviously trivial so I have no issue with it.
> > 
> > Acked-By: Mike Isely <isely@pobox.com>
> > 
> > Note the spelling of my last name "Isely" not "Isley".  A good way to 
> > remember is to think of the normal word "wisely" and just drop the 
> > leading "w".  (And yes, isely@isely.net and isely@pobox.com lead to the 
> > same inbox.)
> 
> Thanks Mike, apologies about the misspelling, I didn't catch it until
> after I hit send. I had the Isley Brothers in my head. :)

No problem.  It's a very common mistake.  And no, I'm not related to 
them.  For the record, I generally don't get concerned about the 
spelling of my name, unless the error causes problems (e.g. lost e-mail) 
or the error gets propagated to a large list where it might multiply...

Anyway, sorry also about taking this thread off topic.  Enough said...

  -Mike


-- 

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

[PATCH 1/3 v2] hdpvr: fix up i2c device registration

[Jarod Wilson]

We have to actually call i2c_new_device() once for each of the rx and tx
addresses. Also improve error-handling and device remove i2c cleanup.

Reviewed-by: Andy Walls <awalls@md.metrocast.net>
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---

v2: address Andy's review comments from v1 -
- wrap i2c_del_adapter with #if defined i2c conditionals in error path
- use function-local non-static i2c_board_info to avoid potential races
- don't bother storing i2c_client pointers in hdpvr device struct

 drivers/media/video/hdpvr/hdpvr-core.c |   24 ++++++++++++++++++++----
 drivers/media/video/hdpvr/hdpvr-i2c.c  |   30 +++++++++++++++++++-----------
 drivers/media/video/hdpvr/hdpvr.h      |    3 ++-
 3 files changed, 41 insertions(+), 16 deletions(-)

diff --git a/drivers/media/video/hdpvr/hdpvr-core.c b/drivers/media/video/hdpvr/hdpvr-core.c
index a6572e5..a27d93b 100644
--- a/drivers/media/video/hdpvr/hdpvr-core.c
+++ b/drivers/media/video/hdpvr/hdpvr-core.c
@@ -283,6 +283,7 @@ static int hdpvr_probe(struct usb_interface *interface,
 	struct hdpvr_device *dev;
 	struct usb_host_interface *iface_desc;
 	struct usb_endpoint_descriptor *endpoint;
+	struct i2c_client *client;
 	size_t buffer_size;
 	int i;
 	int retval = -ENOMEM;
@@ -381,13 +382,21 @@ static int hdpvr_probe(struct usb_interface *interface,
 #if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
 	retval = hdpvr_register_i2c_adapter(dev);
 	if (retval < 0) {
-		v4l2_err(&dev->v4l2_dev, "registering i2c adapter failed\n");
+		v4l2_err(&dev->v4l2_dev, "i2c adapter register failed\n");
 		goto error;
 	}
 
-	retval = hdpvr_register_i2c_ir(dev);
-	if (retval < 0)
-		v4l2_err(&dev->v4l2_dev, "registering i2c IR devices failed\n");
+	client = hdpvr_register_ir_rx_i2c(dev);
+	if (!client) {
+		v4l2_err(&dev->v4l2_dev, "i2c IR RX device register failed\n");
+		goto reg_fail;
+	}
+
+	client = hdpvr_register_ir_tx_i2c(dev);
+	if (!client) {
+		v4l2_err(&dev->v4l2_dev, "i2c IR TX device register failed\n");
+		goto reg_fail;
+	}
 #endif
 
 	/* let the user know what node this device is now attached to */
@@ -395,6 +404,10 @@ static int hdpvr_probe(struct usb_interface *interface,
 		  video_device_node_name(dev->video_dev));
 	return 0;
 
+reg_fail:
+#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
+	i2c_del_adapter(&dev->i2c_adapter);
+#endif
 error:
 	if (dev) {
 		/* Destroy single thread */
@@ -424,6 +437,9 @@ static void hdpvr_disconnect(struct usb_interface *interface)
 	mutex_lock(&dev->io_mutex);
 	hdpvr_cancel_queue(dev);
 	mutex_unlock(&dev->io_mutex);
+#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
+	i2c_del_adapter(&dev->i2c_adapter);
+#endif
 	video_unregister_device(dev->video_dev);
 	atomic_dec(&dev_nr);
 }
diff --git a/drivers/media/video/hdpvr/hdpvr-i2c.c b/drivers/media/video/hdpvr/hdpvr-i2c.c
index 89b71fa..e53fa55 100644
--- a/drivers/media/video/hdpvr/hdpvr-i2c.c
+++ b/drivers/media/video/hdpvr/hdpvr-i2c.c
@@ -31,26 +31,34 @@
 #define Z8F0811_IR_RX_I2C_ADDR	0x71
 
 
-static struct i2c_board_info hdpvr_i2c_board_info = {
-	I2C_BOARD_INFO("ir_tx_z8f0811_hdpvr", Z8F0811_IR_TX_I2C_ADDR),
-	I2C_BOARD_INFO("ir_rx_z8f0811_hdpvr", Z8F0811_IR_RX_I2C_ADDR),
-};
+struct i2c_client *hdpvr_register_ir_tx_i2c(struct hdpvr_device *dev)
+{
+	struct IR_i2c_init_data *init_data = &dev->ir_i2c_init_data;
+	struct i2c_board_info hdpvr_ir_tx_i2c_board_info = {
+		I2C_BOARD_INFO("ir_tx_z8f0811_hdpvr", Z8F0811_IR_TX_I2C_ADDR),
+	};
+
+	init_data->name = "HD-PVR";
+	hdpvr_ir_tx_i2c_board_info.platform_data = init_data;
 
-int hdpvr_register_i2c_ir(struct hdpvr_device *dev)
+	return i2c_new_device(&dev->i2c_adapter, &hdpvr_ir_tx_i2c_board_info);
+}
+
+struct i2c_client *hdpvr_register_ir_rx_i2c(struct hdpvr_device *dev)
 {
-	struct i2c_client *c;
 	struct IR_i2c_init_data *init_data = &dev->ir_i2c_init_data;
+	struct i2c_board_info hdpvr_ir_rx_i2c_board_info = {
+		I2C_BOARD_INFO("ir_rx_z8f0811_hdpvr", Z8F0811_IR_RX_I2C_ADDR),
+	};
 
 	/* Our default information for ir-kbd-i2c.c to use */
 	init_data->ir_codes = RC_MAP_HAUPPAUGE_NEW;
 	init_data->internal_get_key_func = IR_KBD_GET_KEY_HAUP_XVR;
 	init_data->type = RC_TYPE_RC5;
-	init_data->name = "HD PVR";
-	hdpvr_i2c_board_info.platform_data = init_data;
-
-	c = i2c_new_device(&dev->i2c_adapter, &hdpvr_i2c_board_info);
+	init_data->name = "HD-PVR";
+	hdpvr_ir_rx_i2c_board_info.platform_data = init_data;
 
-	return (c == NULL) ? -ENODEV : 0;
+	return i2c_new_device(&dev->i2c_adapter, &hdpvr_ir_rx_i2c_board_info);
 }
 
 static int hdpvr_i2c_read(struct hdpvr_device *dev, int bus,
diff --git a/drivers/media/video/hdpvr/hdpvr.h b/drivers/media/video/hdpvr/hdpvr.h
index ee74e3b..072f23c 100644
--- a/drivers/media/video/hdpvr/hdpvr.h
+++ b/drivers/media/video/hdpvr/hdpvr.h
@@ -313,7 +313,8 @@ int hdpvr_cancel_queue(struct hdpvr_device *dev);
 /* i2c adapter registration */
 int hdpvr_register_i2c_adapter(struct hdpvr_device *dev);
 
-int hdpvr_register_i2c_ir(struct hdpvr_device *dev);
+struct i2c_client *hdpvr_register_ir_rx_i2c(struct hdpvr_device *dev);
+struct i2c_client *hdpvr_register_ir_tx_i2c(struct hdpvr_device *dev);
 
 /*========================================================================*/
 /* buffer management */
-- 
1.7.3.4

-- 
Jarod Wilson
jarod@redhat.com