[PATCH] hdpvr: enable IR part

[PATCH] hdpvr: enable IR part

[Jarod Wilson]

A number of things going on here, but the end result is that the IR part
on the hdpvr gets enabled, and can be used with ir-kbd-i2c and/or
lirc_zilog.

First up, there are some conditional build fixes that come into play
whether i2c is built-in or modular. Second, we're swapping out
i2c_new_probed_device() for i2c_new_device(), as in my testing, probing
always fails, but we *know* that all hdpvr devices have a z8 chip at
0x70 and 0x71. Third, we're poking at an i2c address directly without a
client, and writing some magic bits to actually turn on this IR part
(this could use some improvement in the future). Fourth, some of the
i2c_adapter storage has been reworked, as the existing implementation
used to lead to an oops following i2c changes c. 2.6.31.

Earlier editions of this patch have been floating around the 'net for a
while, including being patched into Fedora kernels, and they *do* work.
This specific version isn't yet tested, beyond loading ir-kbd-i2c and
confirming that it does bind to the RX address of the hdpvr:

Registered IR keymap rc-hauppauge-new
input: i2c IR (HD PVR) as /devices/virtual/rc/rc1/input6
rc1: i2c IR (HD PVR) as /devices/virtual/rc/rc1
ir-kbd-i2c: i2c IR (HD PVR) detected at i2c-1/1-0071/ir0 [Hauppage HD PVR I2C]

(Yes, I'm posting before fully testing, and I do have a reason for that,
and will post a v2 after testing this weekend, if need be)...

Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/media/video/hdpvr/Makefile      |    4 +-
 drivers/media/video/hdpvr/hdpvr-core.c  |   10 +--
 drivers/media/video/hdpvr/hdpvr-i2c.c   |  120 +++++++++++++++----------------
 drivers/media/video/hdpvr/hdpvr-video.c |    7 +--
 drivers/media/video/hdpvr/hdpvr.h       |    2 +-
 5 files changed, 66 insertions(+), 77 deletions(-)

diff --git a/drivers/media/video/hdpvr/Makefile b/drivers/media/video/hdpvr/Makefile
index e0230fc..3baa9f6 100644
--- a/drivers/media/video/hdpvr/Makefile
+++ b/drivers/media/video/hdpvr/Makefile
@@ -1,6 +1,4 @@
-hdpvr-objs	:= hdpvr-control.o hdpvr-core.o hdpvr-video.o
-
-hdpvr-$(CONFIG_I2C) += hdpvr-i2c.o
+hdpvr-objs	:= hdpvr-control.o hdpvr-core.o hdpvr-video.o hdpvr-i2c.o
 
 obj-$(CONFIG_VIDEO_HDPVR) += hdpvr.o
 
diff --git a/drivers/media/video/hdpvr/hdpvr-core.c b/drivers/media/video/hdpvr/hdpvr-core.c
index f7d1ee5..a6572e5 100644
--- a/drivers/media/video/hdpvr/hdpvr-core.c
+++ b/drivers/media/video/hdpvr/hdpvr-core.c
@@ -378,19 +378,17 @@ static int hdpvr_probe(struct usb_interface *interface,
 		goto error;
 	}
 
-#ifdef CONFIG_I2C
-	/* until i2c is working properly */
-	retval = 0; /* hdpvr_register_i2c_adapter(dev); */
+#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");
 		goto error;
 	}
 
-	/* until i2c is working properly */
-	retval = 0; /* hdpvr_register_i2c_ir(dev); */
+	retval = hdpvr_register_i2c_ir(dev);
 	if (retval < 0)
 		v4l2_err(&dev->v4l2_dev, "registering i2c IR devices failed\n");
-#endif /* CONFIG_I2C */
+#endif
 
 	/* let the user know what node this device is now attached to */
 	v4l2_info(&dev->v4l2_dev, "device now attached to %s\n",
diff --git a/drivers/media/video/hdpvr/hdpvr-i2c.c b/drivers/media/video/hdpvr/hdpvr-i2c.c
index 24966aa..c0696c3 100644
--- a/drivers/media/video/hdpvr/hdpvr-i2c.c
+++ b/drivers/media/video/hdpvr/hdpvr-i2c.c
@@ -13,6 +13,8 @@
  *
  */
 
+#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
+
 #include <linux/i2c.h>
 #include <linux/slab.h>
 
@@ -28,55 +30,31 @@
 #define Z8F0811_IR_TX_I2C_ADDR	0x70
 #define Z8F0811_IR_RX_I2C_ADDR	0x71
 
-static const u8 ir_i2c_addrs[] = {
-	Z8F0811_IR_TX_I2C_ADDR,
-	Z8F0811_IR_RX_I2C_ADDR,
-};
 
-static const char * const ir_devicenames[] = {
-	"ir_tx_z8f0811_hdpvr",
-	"ir_rx_z8f0811_hdpvr",
+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),
 };
 
-static int hdpvr_new_i2c_ir(struct hdpvr_device *dev, struct i2c_adapter *adap,
-			    const char *type, u8 addr)
+int hdpvr_register_i2c_ir(struct hdpvr_device *dev)
 {
-	struct i2c_board_info info;
+	struct i2c_client *c;
 	struct IR_i2c_init_data *init_data = &dev->ir_i2c_init_data;
-	unsigned short addr_list[2] = { addr, I2C_CLIENT_END };
-
-	memset(&info, 0, sizeof(struct i2c_board_info));
-	strlcpy(info.type, type, I2C_NAME_SIZE);
 
 	/* Our default information for ir-kbd-i2c.c to use */
-	switch (addr) {
-	case Z8F0811_IR_RX_I2C_ADDR:
-		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";
-		info.platform_data = init_data;
-		break;
-	}
-
-	return i2c_new_probed_device(adap, &info, addr_list, NULL) == NULL ?
-	       -1 : 0;
-}
+	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;
 
-int hdpvr_register_i2c_ir(struct hdpvr_device *dev)
-{
-	int i;
-	int ret = 0;
+	c = i2c_new_device(&dev->i2c_adapter, &hdpvr_i2c_board_info);
 
-	for (i = 0; i < ARRAY_SIZE(ir_i2c_addrs); i++)
-		ret += hdpvr_new_i2c_ir(dev, dev->i2c_adapter,
-					ir_devicenames[i], ir_i2c_addrs[i]);
-
-	return ret;
+	return (c == NULL) ? -ENODEV : 0;
 }
 
-static int hdpvr_i2c_read(struct hdpvr_device *dev, unsigned char addr,
-			  char *data, int len)
+static int hdpvr_i2c_read(struct hdpvr_device *dev, int bus,
+			  unsigned char addr, char *data, int len)
 {
 	int ret;
 	char *buf = kmalloc(len, GFP_KERNEL);
@@ -86,7 +64,7 @@ static int hdpvr_i2c_read(struct hdpvr_device *dev, unsigned char addr,
 	ret = usb_control_msg(dev->udev,
 			      usb_rcvctrlpipe(dev->udev, 0),
 			      REQTYPE_I2C_READ, CTRL_READ_REQUEST,
-			      0x100|addr, 0, buf, len, 1000);
+			      (bus << 8) | addr, 0, buf, len, 1000);
 
 	if (ret == len) {
 		memcpy(data, buf, len);
@@ -99,8 +77,8 @@ static int hdpvr_i2c_read(struct hdpvr_device *dev, unsigned char addr,
 	return ret;
 }
 
-static int hdpvr_i2c_write(struct hdpvr_device *dev, unsigned char addr,
-			   char *data, int len)
+static int hdpvr_i2c_write(struct hdpvr_device *dev, int bus,
+			   unsigned char addr, char *data, int len)
 {
 	int ret;
 	char *buf = kmalloc(len, GFP_KERNEL);
@@ -111,7 +89,7 @@ static int hdpvr_i2c_write(struct hdpvr_device *dev, unsigned char addr,
 	ret = usb_control_msg(dev->udev,
 			      usb_sndctrlpipe(dev->udev, 0),
 			      REQTYPE_I2C_WRITE, CTRL_WRITE_REQUEST,
-			      0x100|addr, 0, buf, len, 1000);
+			      (bus << 8) | addr, 0, buf, len, 1000);
 
 	if (ret < 0)
 		goto error;
@@ -121,7 +99,7 @@ static int hdpvr_i2c_write(struct hdpvr_device *dev, unsigned char addr,
 			      REQTYPE_I2C_WRITE_STATT, CTRL_READ_REQUEST,
 			      0, 0, buf, 2, 1000);
 
-	if (ret == 2)
+	if ((ret == 2) && (buf[1] == (len - 1)))
 		ret = 0;
 	else if (ret >= 0)
 		ret = -EIO;
@@ -146,10 +124,10 @@ static int hdpvr_transfer(struct i2c_adapter *i2c_adapter, struct i2c_msg *msgs,
 		addr = msgs[i].addr << 1;
 
 		if (msgs[i].flags & I2C_M_RD)
-			retval = hdpvr_i2c_read(dev, addr, msgs[i].buf,
+			retval = hdpvr_i2c_read(dev, 1, addr, msgs[i].buf,
 						msgs[i].len);
 		else
-			retval = hdpvr_i2c_write(dev, addr, msgs[i].buf,
+			retval = hdpvr_i2c_write(dev, 1, addr, msgs[i].buf,
 						 msgs[i].len);
 	}
 
@@ -168,30 +146,48 @@ static struct i2c_algorithm hdpvr_algo = {
 	.functionality = hdpvr_functionality,
 };
 
+static struct i2c_adapter hdpvr_i2c_adapter_template = {
+	.name   = "Hauppage HD PVR I2C",
+	.owner  = THIS_MODULE,
+	.algo   = &hdpvr_algo,
+	.class  = I2C_CLASS_TV_ANALOG,
+};
+
+static int hdpvr_activate_ir(struct hdpvr_device *dev)
+{
+	char buffer[8];
+
+	mutex_lock(&dev->i2c_mutex);
+
+	hdpvr_i2c_read(dev, 0, 0x54, buffer, 1);
+
+	buffer[0] = 0;
+	buffer[1] = 0x8;
+	hdpvr_i2c_write(dev, 1, 0x54, buffer, 2);
+
+	buffer[1] = 0x18;
+	hdpvr_i2c_write(dev, 1, 0x54, buffer, 2);
+
+	mutex_unlock(&dev->i2c_mutex);
+
+	return 0;
+}
+
 int hdpvr_register_i2c_adapter(struct hdpvr_device *dev)
 {
-	struct i2c_adapter *i2c_adap;
 	int retval = -ENOMEM;
 
-	i2c_adap = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL);
-	if (i2c_adap == NULL)
-		goto error;
+	hdpvr_activate_ir(dev);
 
-	strlcpy(i2c_adap->name, "Hauppauge HD PVR I2C",
-		sizeof(i2c_adap->name));
-	i2c_adap->algo  = &hdpvr_algo;
-	i2c_adap->owner = THIS_MODULE;
-	i2c_adap->dev.parent = &dev->udev->dev;
+	memcpy(&dev->i2c_adapter, &hdpvr_i2c_adapter_template,
+	       sizeof(struct i2c_adapter));
+	dev->i2c_adapter.dev.parent = &dev->udev->dev;
 
-	i2c_set_adapdata(i2c_adap, dev);
+	i2c_set_adapdata(&dev->i2c_adapter, dev);
 
-	retval = i2c_add_adapter(i2c_adap);
+	retval = i2c_add_adapter(&dev->i2c_adapter);
 
-	if (!retval)
-		dev->i2c_adapter = i2c_adap;
-	else
-		kfree(i2c_adap);
-
-error:
 	return retval;
 }
+
+#endif
diff --git a/drivers/media/video/hdpvr/hdpvr-video.c b/drivers/media/video/hdpvr/hdpvr-video.c
index d38fe10..514aea7 100644
--- a/drivers/media/video/hdpvr/hdpvr-video.c
+++ b/drivers/media/video/hdpvr/hdpvr-video.c
@@ -1220,12 +1220,9 @@ static void hdpvr_device_release(struct video_device *vdev)
 	v4l2_device_unregister(&dev->v4l2_dev);
 
 	/* deregister I2C adapter */
-#ifdef CONFIG_I2C
+#if defined(CONFIG_I2C) || (CONFIG_I2C_MODULE)
 	mutex_lock(&dev->i2c_mutex);
-	if (dev->i2c_adapter)
-		i2c_del_adapter(dev->i2c_adapter);
-	kfree(dev->i2c_adapter);
-	dev->i2c_adapter = NULL;
+	i2c_del_adapter(&dev->i2c_adapter);
 	mutex_unlock(&dev->i2c_mutex);
 #endif /* CONFIG_I2C */
 
diff --git a/drivers/media/video/hdpvr/hdpvr.h b/drivers/media/video/hdpvr/hdpvr.h
index 37f1e4c..29f7426 100644
--- a/drivers/media/video/hdpvr/hdpvr.h
+++ b/drivers/media/video/hdpvr/hdpvr.h
@@ -106,7 +106,7 @@ struct hdpvr_device {
 	struct work_struct	worker;
 
 	/* I2C adapter */
-	struct i2c_adapter	*i2c_adapter;
+	struct i2c_adapter	i2c_adapter;
 	/* I2C lock */
 	struct mutex		i2c_mutex;
 
-- 
1.7.1

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH] hdpvr: enable IR part

[Andy Walls]

On Fri, 2011-01-14 at 14:54 -0500, Jarod Wilson wrote:
> A number of things going on here, but the end result is that the IR part
> on the hdpvr gets enabled, and can be used with ir-kbd-i2c and/or
> lirc_zilog.
> 
> First up, there are some conditional build fixes that come into play
> whether i2c is built-in or modular. Second, we're swapping out
> i2c_new_probed_device() for i2c_new_device(), as in my testing, probing
> always fails, but we *know* that all hdpvr devices have a z8 chip at
> 0x70 and 0x71. Third, we're poking at an i2c address directly without a
> client, and writing some magic bits to actually turn on this IR part
> (this could use some improvement in the future). Fourth, some of the
> i2c_adapter storage has been reworked, as the existing implementation
> used to lead to an oops following i2c changes c. 2.6.31.
> 
> Earlier editions of this patch have been floating around the 'net for a
> while, including being patched into Fedora kernels, and they *do* work.
> This specific version isn't yet tested, beyond loading ir-kbd-i2c and
> confirming that it does bind to the RX address of the hdpvr:
> 
> Registered IR keymap rc-hauppauge-new
> input: i2c IR (HD PVR) as /devices/virtual/rc/rc1/input6
> rc1: i2c IR (HD PVR) as /devices/virtual/rc/rc1
> ir-kbd-i2c: i2c IR (HD PVR) detected at i2c-1/1-0071/ir0 [Hauppage HD PVR I2C]
> 
> (Yes, I'm posting before fully testing, and I do have a reason for that,
> and will post a v2 after testing this weekend, if need be)...

As discussed on IRC
1. no need to probe for the Z8 as HD PVR always has one.  
2. accessing the chip at address 0x54 directly should eventually be
reworked with nicer i2c subsystem methods, but that can wait

So

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

Note, that code in lirc_zilog.c indicates that ir-kbd-i2c.c's function
get_key_haup_xvr() *may* need some additions to account for the Rx data
format.  I'm not sure of this though.  (Or a custom get_key() in the
hdpvr module could be provided as a function pointer to ir-kbd-i2c.c via
platform_data.)

I will provide a debug patch in another email so that it's easier to see
the data ir-kbd-i2c.c sees coming from the Z8.

Regards,
Andy

> Signed-off-by: Jarod Wilson <jarod@redhat.com>
> ---
>  drivers/media/video/hdpvr/Makefile      |    4 +-
>  drivers/media/video/hdpvr/hdpvr-core.c  |   10 +--
>  drivers/media/video/hdpvr/hdpvr-i2c.c   |  120 +++++++++++++++----------------
>  drivers/media/video/hdpvr/hdpvr-video.c |    7 +--
>  drivers/media/video/hdpvr/hdpvr.h       |    2 +-
>  5 files changed, 66 insertions(+), 77 deletions(-)
> 
> diff --git a/drivers/media/video/hdpvr/Makefile b/drivers/media/video/hdpvr/Makefile
> index e0230fc..3baa9f6 100644
> --- a/drivers/media/video/hdpvr/Makefile
> +++ b/drivers/media/video/hdpvr/Makefile
> @@ -1,6 +1,4 @@
> -hdpvr-objs	:= hdpvr-control.o hdpvr-core.o hdpvr-video.o
> -
> -hdpvr-$(CONFIG_I2C) += hdpvr-i2c.o
> +hdpvr-objs	:= hdpvr-control.o hdpvr-core.o hdpvr-video.o hdpvr-i2c.o
>  
>  obj-$(CONFIG_VIDEO_HDPVR) += hdpvr.o
>  
> diff --git a/drivers/media/video/hdpvr/hdpvr-core.c b/drivers/media/video/hdpvr/hdpvr-core.c
> index f7d1ee5..a6572e5 100644
> --- a/drivers/media/video/hdpvr/hdpvr-core.c
> +++ b/drivers/media/video/hdpvr/hdpvr-core.c
> @@ -378,19 +378,17 @@ static int hdpvr_probe(struct usb_interface *interface,
>  		goto error;
>  	}
>  
> -#ifdef CONFIG_I2C
> -	/* until i2c is working properly */
> -	retval = 0; /* hdpvr_register_i2c_adapter(dev); */
> +#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");
>  		goto error;
>  	}
>  
> -	/* until i2c is working properly */
> -	retval = 0; /* hdpvr_register_i2c_ir(dev); */
> +	retval = hdpvr_register_i2c_ir(dev);
>  	if (retval < 0)
>  		v4l2_err(&dev->v4l2_dev, "registering i2c IR devices failed\n");
> -#endif /* CONFIG_I2C */
> +#endif
>  
>  	/* let the user know what node this device is now attached to */
>  	v4l2_info(&dev->v4l2_dev, "device now attached to %s\n",
> diff --git a/drivers/media/video/hdpvr/hdpvr-i2c.c b/drivers/media/video/hdpvr/hdpvr-i2c.c
> index 24966aa..c0696c3 100644
> --- a/drivers/media/video/hdpvr/hdpvr-i2c.c
> +++ b/drivers/media/video/hdpvr/hdpvr-i2c.c
> @@ -13,6 +13,8 @@
>   *
>   */
>  
> +#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
> +
>  #include <linux/i2c.h>
>  #include <linux/slab.h>
>  
> @@ -28,55 +30,31 @@
>  #define Z8F0811_IR_TX_I2C_ADDR	0x70
>  #define Z8F0811_IR_RX_I2C_ADDR	0x71
>  
> -static const u8 ir_i2c_addrs[] = {
> -	Z8F0811_IR_TX_I2C_ADDR,
> -	Z8F0811_IR_RX_I2C_ADDR,
> -};
>  
> -static const char * const ir_devicenames[] = {
> -	"ir_tx_z8f0811_hdpvr",
> -	"ir_rx_z8f0811_hdpvr",
> +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),
>  };
>  
> -static int hdpvr_new_i2c_ir(struct hdpvr_device *dev, struct i2c_adapter *adap,
> -			    const char *type, u8 addr)
> +int hdpvr_register_i2c_ir(struct hdpvr_device *dev)
>  {
> -	struct i2c_board_info info;
> +	struct i2c_client *c;
>  	struct IR_i2c_init_data *init_data = &dev->ir_i2c_init_data;
> -	unsigned short addr_list[2] = { addr, I2C_CLIENT_END };
> -
> -	memset(&info, 0, sizeof(struct i2c_board_info));
> -	strlcpy(info.type, type, I2C_NAME_SIZE);
>  
>  	/* Our default information for ir-kbd-i2c.c to use */
> -	switch (addr) {
> -	case Z8F0811_IR_RX_I2C_ADDR:
> -		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";
> -		info.platform_data = init_data;
> -		break;
> -	}
> -
> -	return i2c_new_probed_device(adap, &info, addr_list, NULL) == NULL ?
> -	       -1 : 0;
> -}
> +	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;
>  
> -int hdpvr_register_i2c_ir(struct hdpvr_device *dev)
> -{
> -	int i;
> -	int ret = 0;
> +	c = i2c_new_device(&dev->i2c_adapter, &hdpvr_i2c_board_info);
>  
> -	for (i = 0; i < ARRAY_SIZE(ir_i2c_addrs); i++)
> -		ret += hdpvr_new_i2c_ir(dev, dev->i2c_adapter,
> -					ir_devicenames[i], ir_i2c_addrs[i]);
> -
> -	return ret;
> +	return (c == NULL) ? -ENODEV : 0;
>  }
>  
> -static int hdpvr_i2c_read(struct hdpvr_device *dev, unsigned char addr,
> -			  char *data, int len)
> +static int hdpvr_i2c_read(struct hdpvr_device *dev, int bus,
> +			  unsigned char addr, char *data, int len)
>  {
>  	int ret;
>  	char *buf = kmalloc(len, GFP_KERNEL);
> @@ -86,7 +64,7 @@ static int hdpvr_i2c_read(struct hdpvr_device *dev, unsigned char addr,
>  	ret = usb_control_msg(dev->udev,
>  			      usb_rcvctrlpipe(dev->udev, 0),
>  			      REQTYPE_I2C_READ, CTRL_READ_REQUEST,
> -			      0x100|addr, 0, buf, len, 1000);
> +			      (bus << 8) | addr, 0, buf, len, 1000);
>  
>  	if (ret == len) {
>  		memcpy(data, buf, len);
> @@ -99,8 +77,8 @@ static int hdpvr_i2c_read(struct hdpvr_device *dev, unsigned char addr,
>  	return ret;
>  }
>  
> -static int hdpvr_i2c_write(struct hdpvr_device *dev, unsigned char addr,
> -			   char *data, int len)
> +static int hdpvr_i2c_write(struct hdpvr_device *dev, int bus,
> +			   unsigned char addr, char *data, int len)
>  {
>  	int ret;
>  	char *buf = kmalloc(len, GFP_KERNEL);
> @@ -111,7 +89,7 @@ static int hdpvr_i2c_write(struct hdpvr_device *dev, unsigned char addr,
>  	ret = usb_control_msg(dev->udev,
>  			      usb_sndctrlpipe(dev->udev, 0),
>  			      REQTYPE_I2C_WRITE, CTRL_WRITE_REQUEST,
> -			      0x100|addr, 0, buf, len, 1000);
> +			      (bus << 8) | addr, 0, buf, len, 1000);
>  
>  	if (ret < 0)
>  		goto error;
> @@ -121,7 +99,7 @@ static int hdpvr_i2c_write(struct hdpvr_device *dev, unsigned char addr,
>  			      REQTYPE_I2C_WRITE_STATT, CTRL_READ_REQUEST,
>  			      0, 0, buf, 2, 1000);
>  
> -	if (ret == 2)
> +	if ((ret == 2) && (buf[1] == (len - 1)))
>  		ret = 0;
>  	else if (ret >= 0)
>  		ret = -EIO;
> @@ -146,10 +124,10 @@ static int hdpvr_transfer(struct i2c_adapter *i2c_adapter, struct i2c_msg *msgs,
>  		addr = msgs[i].addr << 1;
>  
>  		if (msgs[i].flags & I2C_M_RD)
> -			retval = hdpvr_i2c_read(dev, addr, msgs[i].buf,
> +			retval = hdpvr_i2c_read(dev, 1, addr, msgs[i].buf,
>  						msgs[i].len);
>  		else
> -			retval = hdpvr_i2c_write(dev, addr, msgs[i].buf,
> +			retval = hdpvr_i2c_write(dev, 1, addr, msgs[i].buf,
>  						 msgs[i].len);
>  	}
>  
> @@ -168,30 +146,48 @@ static struct i2c_algorithm hdpvr_algo = {
>  	.functionality = hdpvr_functionality,
>  };
>  
> +static struct i2c_adapter hdpvr_i2c_adapter_template = {
> +	.name   = "Hauppage HD PVR I2C",
> +	.owner  = THIS_MODULE,
> +	.algo   = &hdpvr_algo,
> +	.class  = I2C_CLASS_TV_ANALOG,
> +};
> +
> +static int hdpvr_activate_ir(struct hdpvr_device *dev)
> +{
> +	char buffer[8];
> +
> +	mutex_lock(&dev->i2c_mutex);
> +
> +	hdpvr_i2c_read(dev, 0, 0x54, buffer, 1);
> +
> +	buffer[0] = 0;
> +	buffer[1] = 0x8;
> +	hdpvr_i2c_write(dev, 1, 0x54, buffer, 2);
> +
> +	buffer[1] = 0x18;
> +	hdpvr_i2c_write(dev, 1, 0x54, buffer, 2);
> +
> +	mutex_unlock(&dev->i2c_mutex);
> +
> +	return 0;
> +}
> +
>  int hdpvr_register_i2c_adapter(struct hdpvr_device *dev)
>  {
> -	struct i2c_adapter *i2c_adap;
>  	int retval = -ENOMEM;
>  
> -	i2c_adap = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL);
> -	if (i2c_adap == NULL)
> -		goto error;
> +	hdpvr_activate_ir(dev);
>  
> -	strlcpy(i2c_adap->name, "Hauppauge HD PVR I2C",
> -		sizeof(i2c_adap->name));
> -	i2c_adap->algo  = &hdpvr_algo;
> -	i2c_adap->owner = THIS_MODULE;
> -	i2c_adap->dev.parent = &dev->udev->dev;
> +	memcpy(&dev->i2c_adapter, &hdpvr_i2c_adapter_template,
> +	       sizeof(struct i2c_adapter));
> +	dev->i2c_adapter.dev.parent = &dev->udev->dev;
>  
> -	i2c_set_adapdata(i2c_adap, dev);
> +	i2c_set_adapdata(&dev->i2c_adapter, dev);
>  
> -	retval = i2c_add_adapter(i2c_adap);
> +	retval = i2c_add_adapter(&dev->i2c_adapter);
>  
> -	if (!retval)
> -		dev->i2c_adapter = i2c_adap;
> -	else
> -		kfree(i2c_adap);
> -
> -error:
>  	return retval;
>  }
> +
> +#endif
> diff --git a/drivers/media/video/hdpvr/hdpvr-video.c b/drivers/media/video/hdpvr/hdpvr-video.c
> index d38fe10..514aea7 100644
> --- a/drivers/media/video/hdpvr/hdpvr-video.c
> +++ b/drivers/media/video/hdpvr/hdpvr-video.c
> @@ -1220,12 +1220,9 @@ static void hdpvr_device_release(struct video_device *vdev)
>  	v4l2_device_unregister(&dev->v4l2_dev);
>  
>  	/* deregister I2C adapter */
> -#ifdef CONFIG_I2C
> +#if defined(CONFIG_I2C) || (CONFIG_I2C_MODULE)
>  	mutex_lock(&dev->i2c_mutex);
> -	if (dev->i2c_adapter)
> -		i2c_del_adapter(dev->i2c_adapter);
> -	kfree(dev->i2c_adapter);
> -	dev->i2c_adapter = NULL;
> +	i2c_del_adapter(&dev->i2c_adapter);
>  	mutex_unlock(&dev->i2c_mutex);
>  #endif /* CONFIG_I2C */
>  
> diff --git a/drivers/media/video/hdpvr/hdpvr.h b/drivers/media/video/hdpvr/hdpvr.h
> index 37f1e4c..29f7426 100644
> --- a/drivers/media/video/hdpvr/hdpvr.h
> +++ b/drivers/media/video/hdpvr/hdpvr.h
> @@ -106,7 +106,7 @@ struct hdpvr_device {
>  	struct work_struct	worker;
>  
>  	/* I2C adapter */
> -	struct i2c_adapter	*i2c_adapter;
> +	struct i2c_adapter	i2c_adapter;
>  	/* I2C lock */
>  	struct mutex		i2c_mutex;
>  
> -- 
> 1.7.1
> 

Re: [PATCH] hdpvr: enable IR part

[Jarod Wilson]

On Fri, Jan 14, 2011 at 04:44:40PM -0500, Andy Walls wrote:
> On Fri, 2011-01-14 at 14:54 -0500, Jarod Wilson wrote:
> > A number of things going on here, but the end result is that the IR part
> > on the hdpvr gets enabled, and can be used with ir-kbd-i2c and/or
> > lirc_zilog.
> > 
> > First up, there are some conditional build fixes that come into play
> > whether i2c is built-in or modular. Second, we're swapping out
> > i2c_new_probed_device() for i2c_new_device(), as in my testing, probing
> > always fails, but we *know* that all hdpvr devices have a z8 chip at
> > 0x70 and 0x71. Third, we're poking at an i2c address directly without a
> > client, and writing some magic bits to actually turn on this IR part
> > (this could use some improvement in the future). Fourth, some of the
> > i2c_adapter storage has been reworked, as the existing implementation
> > used to lead to an oops following i2c changes c. 2.6.31.
> > 
> > Earlier editions of this patch have been floating around the 'net for a
> > while, including being patched into Fedora kernels, and they *do* work.
> > This specific version isn't yet tested, beyond loading ir-kbd-i2c and
> > confirming that it does bind to the RX address of the hdpvr:
> > 
> > Registered IR keymap rc-hauppauge-new
> > input: i2c IR (HD PVR) as /devices/virtual/rc/rc1/input6
> > rc1: i2c IR (HD PVR) as /devices/virtual/rc/rc1
> > ir-kbd-i2c: i2c IR (HD PVR) detected at i2c-1/1-0071/ir0 [Hauppage HD PVR I2C]
> > 
> > (Yes, I'm posting before fully testing, and I do have a reason for that,
> > and will post a v2 after testing this weekend, if need be)...
> 
> As discussed on IRC
> 1. no need to probe for the Z8 as HD PVR always has one.  

Did a bit further prodding w/i2cdetect under Jean's guidance[1]. The hdpvr
hardware doesn't like the quick writes used by the i2c_new_probed_device()
routine, but does work with short reads for detection. That would require
writing a custom probe routine though, and just isn't worth the effort on
the hdpvr, since we know it always has an IR part, so i2c_new_device()
really is the way to go here.

[1] 'i2cdetect -l' shows my hdpvr at i2c-1, 'i2cdetect 1', which probes
similar to i2_new_probed_device() doesn't return a device where expected,
but 'i2cdetect -r 1', which uses a short read, does.

> 2. accessing the chip at address 0x54 directly should eventually be
> reworked with nicer i2c subsystem methods, but that can wait

Agreed, this should be cleaned up at some point.

> Note, that code in lirc_zilog.c indicates that ir-kbd-i2c.c's function
> get_key_haup_xvr() *may* need some additions to account for the Rx data
> format.  I'm not sure of this though.  (Or a custom get_key() in the
> hdpvr module could be provided as a function pointer to ir-kbd-i2c.c via
> platform_data.)
> 
> I will provide a debug patch in another email so that it's easier to see
> the data ir-kbd-i2c.c sees coming from the Z8.

I'll tack that onto the machine I've got hooked to the hdpvr and see what
I can see this weekend, thanks much for the pointers.

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH] hdpvr: enable IR part

[Jarod Wilson]

On Jan 14, 2011, at 5:08 PM, Jarod Wilson wrote:

> On Fri, Jan 14, 2011 at 04:44:40PM -0500, Andy Walls wrote:
>> On Fri, 2011-01-14 at 14:54 -0500, Jarod Wilson wrote:
>>> A number of things going on here, but the end result is that the IR part
>>> on the hdpvr gets enabled, and can be used with ir-kbd-i2c and/or
>>> lirc_zilog.
>>> 
>>> First up, there are some conditional build fixes that come into play
>>> whether i2c is built-in or modular. Second, we're swapping out
>>> i2c_new_probed_device() for i2c_new_device(), as in my testing, probing
>>> always fails, but we *know* that all hdpvr devices have a z8 chip at
>>> 0x70 and 0x71. Third, we're poking at an i2c address directly without a
>>> client, and writing some magic bits to actually turn on this IR part
>>> (this could use some improvement in the future). Fourth, some of the
>>> i2c_adapter storage has been reworked, as the existing implementation
>>> used to lead to an oops following i2c changes c. 2.6.31.
>>> 
>>> Earlier editions of this patch have been floating around the 'net for a
>>> while, including being patched into Fedora kernels, and they *do* work.
>>> This specific version isn't yet tested, beyond loading ir-kbd-i2c and
>>> confirming that it does bind to the RX address of the hdpvr:
>>> 
>>> Registered IR keymap rc-hauppauge-new
>>> input: i2c IR (HD PVR) as /devices/virtual/rc/rc1/input6
>>> rc1: i2c IR (HD PVR) as /devices/virtual/rc/rc1
>>> ir-kbd-i2c: i2c IR (HD PVR) detected at i2c-1/1-0071/ir0 [Hauppage HD PVR I2C]

And it *almost* works. Every key on the bundled remote is recognized, but
every press gets feedback about like this w/evtest:

Event: time 1295058330.966180, type 4 (Misc), code 4 (ScanCode), value 29
Event: time 1295058330.966212, type 1 (Key), code 401 (Blue), value 1
Event: time 1295058330.966220, -------------- Report Sync ------------
Event: time 1295058331.066175, type 4 (Misc), code 4 (ScanCode), value 29
Event: time 1295058331.166290, type 4 (Misc), code 4 (ScanCode), value 29
Event: time 1295058331.275664, type 4 (Misc), code 4 (ScanCode), value 29
Event: time 1295058331.376160, type 4 (Misc), code 4 (ScanCode), value 29
Event: time 1295058331.465505, type 1 (Key), code 401 (Blue), value 2
Event: time 1295058331.476161, type 4 (Misc), code 4 (ScanCode), value 29
Event: time 1295058331.498502, type 1 (Key), code 401 (Blue), value 2
Event: time 1295058331.531504, type 1 (Key), code 401 (Blue), value 2
Event: time 1295058331.564503, type 1 (Key), code 401 (Blue), value 2
Event: time 1295058331.576153, type 4 (Misc), code 4 (ScanCode), value 29
Event: time 1295058331.597502, type 1 (Key), code 401 (Blue), value 2
Event: time 1295058331.630501, type 1 (Key), code 401 (Blue), value 2
Event: time 1295058331.663502, type 1 (Key), code 401 (Blue), value 2
Event: time 1295058331.696500, type 1 (Key), code 401 (Blue), value 2
Event: time 1295058331.729503, type 1 (Key), code 401 (Blue), value 2
Event: time 1295058331.762502, type 1 (Key), code 401 (Blue), value 2
Event: time 1295058331.795500, type 1 (Key), code 401 (Blue), value 2
Event: time 1295058331.825507, type 1 (Key), code 401 (Blue), value 0
Event: time 1295058331.825526, -------------- Report Sync ------------

That's just a single short press. With arrow keys, its quite funky to
watch in, say, mythtv UI menus. Hit up, and it moves up one, stalls for a
second, then moves up several more.

...
>> Note, that code in lirc_zilog.c indicates that ir-kbd-i2c.c's function
>> get_key_haup_xvr() *may* need some additions to account for the Rx data
>> format.  I'm not sure of this though.  (Or a custom get_key() in the
>> hdpvr module could be provided as a function pointer to ir-kbd-i2c.c via
>> platform_data.)
>> 
>> I will provide a debug patch in another email so that it's easier to see
>> the data ir-kbd-i2c.c sees coming from the Z8.
> 
> I'll tack that onto the machine I've got hooked to the hdpvr and see what
> I can see this weekend, thanks much for the pointers.

I'm inclined to think that it does indeed need some of the changes from
lirc_zilog brought over (or the custom get_key()). Now on to adding that
patch and some testing with lirc_zilog...

-- 
Jarod Wilson
jarod@wilsonet.com

Re: [PATCH] hdpvr: enable IR part

[Andy Walls]

On Fri, 2011-01-14 at 21:30 -0500, Jarod Wilson wrote:
> On Jan 14, 2011, at 5:08 PM, Jarod Wilson wrote:

> >>> Registered IR keymap rc-hauppauge-new
> >>> input: i2c IR (HD PVR) as /devices/virtual/rc/rc1/input6
> >>> rc1: i2c IR (HD PVR) as /devices/virtual/rc/rc1
> >>> ir-kbd-i2c: i2c IR (HD PVR) detected at i2c-1/1-0071/ir0 [Hauppage HD PVR I2C]
> 
> And it *almost* works. Every key on the bundled remote is recognized, but
> every press gets feedback about like this w/evtest:
> 
> Event: time 1295058330.966180, type 4 (Misc), code 4 (ScanCode), value 29
> Event: time 1295058330.966212, type 1 (Key), code 401 (Blue), value 1
> Event: time 1295058330.966220, -------------- Report Sync ------------
> Event: time 1295058331.066175, type 4 (Misc), code 4 (ScanCode), value 29
> Event: time 1295058331.166290, type 4 (Misc), code 4 (ScanCode), value 29
> Event: time 1295058331.275664, type 4 (Misc), code 4 (ScanCode), value 29
> Event: time 1295058331.376160, type 4 (Misc), code 4 (ScanCode), value 29
> Event: time 1295058331.465505, type 1 (Key), code 401 (Blue), value 2
> Event: time 1295058331.476161, type 4 (Misc), code 4 (ScanCode), value 29
> Event: time 1295058331.498502, type 1 (Key), code 401 (Blue), value 2
> Event: time 1295058331.531504, type 1 (Key), code 401 (Blue), value 2
> Event: time 1295058331.564503, type 1 (Key), code 401 (Blue), value 2
> Event: time 1295058331.576153, type 4 (Misc), code 4 (ScanCode), value 29
> Event: time 1295058331.597502, type 1 (Key), code 401 (Blue), value 2
> Event: time 1295058331.630501, type 1 (Key), code 401 (Blue), value 2
> Event: time 1295058331.663502, type 1 (Key), code 401 (Blue), value 2
> Event: time 1295058331.696500, type 1 (Key), code 401 (Blue), value 2
> Event: time 1295058331.729503, type 1 (Key), code 401 (Blue), value 2
> Event: time 1295058331.762502, type 1 (Key), code 401 (Blue), value 2
> Event: time 1295058331.795500, type 1 (Key), code 401 (Blue), value 2
> Event: time 1295058331.825507, type 1 (Key), code 401 (Blue), value 0
> Event: time 1295058331.825526, -------------- Report Sync ------------
> 
> That's just a single short press. With arrow keys, its quite funky to
> watch in, say, mythtv UI menus. Hit up, and it moves up one, stalls for a
> second, then moves up several more.

Hmm bizzarre.

> ...
> >> Note, that code in lirc_zilog.c indicates that ir-kbd-i2c.c's function
> >> get_key_haup_xvr() *may* need some additions to account for the Rx data
> >> format.  I'm not sure of this though.  (Or a custom get_key() in the
> >> hdpvr module could be provided as a function pointer to ir-kbd-i2c.c via
> >> platform_data.)
> >> 
> >> I will provide a debug patch in another email so that it's easier to see
> >> the data ir-kbd-i2c.c sees coming from the Z8.
> > 
> > I'll tack that onto the machine I've got hooked to the hdpvr and see what
> > I can see this weekend, thanks much for the pointers.
> 
> I'm inclined to think that it does indeed need some of the changes from
> lirc_zilog brought over (or the custom get_key()). Now on to adding that
> patch and some testing with lirc_zilog...

Yes, maybe both the Rx data parsing *and* the precise delay between Rx
polls.


BTW, a checkpatch and compiler tested lirc_zilog.c is here:

http://git.linuxtv.org/awalls/media_tree.git?a=shortlog;h=refs/heads/z8

It should fix all the binding and allocation problems related to
ir_probe()/ir_remove().  Except I suspect it may leak the Rx poll
kthread.  That's possibly another bug to add to the list.

Anyway, $DIETY knows if the lirc_zilog module actually still works after
all my hacks.  Give it a test if you are adventurous.  I won't be able
to test until tomorrow evening.

Regards,
Andy

Re: [PATCH] hdpvr: enable IR part

[Jarod Wilson]

On Jan 14, 2011, at 11:35 PM, Andy Walls wrote:

> On Fri, 2011-01-14 at 21:30 -0500, Jarod Wilson wrote:
>> On Jan 14, 2011, at 5:08 PM, Jarod Wilson wrote:
> 
>>>>> Registered IR keymap rc-hauppauge-new
>>>>> input: i2c IR (HD PVR) as /devices/virtual/rc/rc1/input6
>>>>> rc1: i2c IR (HD PVR) as /devices/virtual/rc/rc1
>>>>> ir-kbd-i2c: i2c IR (HD PVR) detected at i2c-1/1-0071/ir0 [Hauppage HD PVR I2C]
>> 
>> And it *almost* works. Every key on the bundled remote is recognized, but
>> every press gets feedback about like this w/evtest:
>> 
>> Event: time 1295058330.966180, type 4 (Misc), code 4 (ScanCode), value 29
>> Event: time 1295058330.966212, type 1 (Key), code 401 (Blue), value 1
>> Event: time 1295058330.966220, -------------- Report Sync ------------
>> Event: time 1295058331.066175, type 4 (Misc), code 4 (ScanCode), value 29
>> Event: time 1295058331.166290, type 4 (Misc), code 4 (ScanCode), value 29
>> Event: time 1295058331.275664, type 4 (Misc), code 4 (ScanCode), value 29
>> Event: time 1295058331.376160, type 4 (Misc), code 4 (ScanCode), value 29
>> Event: time 1295058331.465505, type 1 (Key), code 401 (Blue), value 2
>> Event: time 1295058331.476161, type 4 (Misc), code 4 (ScanCode), value 29
>> Event: time 1295058331.498502, type 1 (Key), code 401 (Blue), value 2
>> Event: time 1295058331.531504, type 1 (Key), code 401 (Blue), value 2
>> Event: time 1295058331.564503, type 1 (Key), code 401 (Blue), value 2
>> Event: time 1295058331.576153, type 4 (Misc), code 4 (ScanCode), value 29
>> Event: time 1295058331.597502, type 1 (Key), code 401 (Blue), value 2
>> Event: time 1295058331.630501, type 1 (Key), code 401 (Blue), value 2
>> Event: time 1295058331.663502, type 1 (Key), code 401 (Blue), value 2
>> Event: time 1295058331.696500, type 1 (Key), code 401 (Blue), value 2
>> Event: time 1295058331.729503, type 1 (Key), code 401 (Blue), value 2
>> Event: time 1295058331.762502, type 1 (Key), code 401 (Blue), value 2
>> Event: time 1295058331.795500, type 1 (Key), code 401 (Blue), value 2
>> Event: time 1295058331.825507, type 1 (Key), code 401 (Blue), value 0
>> Event: time 1295058331.825526, -------------- Report Sync ------------
>> 
>> That's just a single short press. With arrow keys, its quite funky to
>> watch in, say, mythtv UI menus. Hit up, and it moves up one, stalls for a
>> second, then moves up several more.
> 
> Hmm bizzarre.

Shouldn't have said "stalls for a second", its a just brief pause, but
regardless, yeah, its kinda odd.


>> ...
>>>> Note, that code in lirc_zilog.c indicates that ir-kbd-i2c.c's function
>>>> get_key_haup_xvr() *may* need some additions to account for the Rx data
>>>> format.  I'm not sure of this though.  (Or a custom get_key() in the
>>>> hdpvr module could be provided as a function pointer to ir-kbd-i2c.c via
>>>> platform_data.)
>>>> 
>>>> I will provide a debug patch in another email so that it's easier to see
>>>> the data ir-kbd-i2c.c sees coming from the Z8.
>>> 
>>> I'll tack that onto the machine I've got hooked to the hdpvr and see what
>>> I can see this weekend, thanks much for the pointers.
>> 
>> I'm inclined to think that it does indeed need some of the changes from
>> lirc_zilog brought over (or the custom get_key()). Now on to adding that
>> patch and some testing with lirc_zilog...


A single button press w/ir-kbd-i2c debugging and your patch:

ir-kbd-i2c: ir_poll_key
ir-kbd-i2c: get_key_haup_common: received bytes: 80 00 00 fe 54 00        ....T.
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t1 dev=30 code=21
ir-kbd-i2c: ir_poll_key
ir-kbd-i2c: get_key_haup_common: received bytes: 80 00 00 fe 54 00        ....T.
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t1 dev=30 code=21
ir-kbd-i2c: ir_poll_key
ir-kbd-i2c: get_key_haup_common: received bytes: 80 00 00 fe 54 00        ....T.
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t1 dev=30 code=21
ir-kbd-i2c: ir_poll_key
ir-kbd-i2c: get_key_haup_common: received bytes: 80 00 00 fe 54 00        ....T.
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t1 dev=30 code=21
ir-kbd-i2c: ir_poll_key
ir-kbd-i2c: get_key_haup_common: received bytes: 80 00 00 fe 54 00        ....T.
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t1 dev=30 code=21
ir-kbd-i2c: ir_poll_key
ir-kbd-i2c: get_key_haup_common: received bytes: 80 00 00 fe 54 00        ....T.
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t1 dev=30 code=21


> Yes, maybe both the Rx data parsing *and* the precise delay between Rx
> polls.

Receive with lirc_zilog does actually work slightly better, though its still
not perfect. Each key press (using irw to watch) always results in at least
two lines of output, both with sequence number 00 (i.e., two distinct events),
and holding a button down results in a stream of 00 events. So repeat is
obviously busted. But I don't see the wackiness that is happening w/ir-kbd-i2c.

Oh, and transmit works too. So this patch and the buffer alloc patch have now
been formally tested. Unless we go the custom get_key() route inside the hdpvr
driver, I think the rest of the legwork to make the hdpvr's IR part behave is
within lirc_zilog and ir-kbd-i2c (both of which I need to spend some more
time reading over).


> BTW, a checkpatch and compiler tested lirc_zilog.c is here:
> 
> http://git.linuxtv.org/awalls/media_tree.git?a=shortlog;h=refs/heads/z8
> 
> It should fix all the binding and allocation problems related to
> ir_probe()/ir_remove().  Except I suspect it may leak the Rx poll
> kthread.  That's possibly another bug to add to the list.
> 
> Anyway, $DIETY knows if the lirc_zilog module actually still works after
> all my hacks.  Give it a test if you are adventurous.  I won't be able
> to test until tomorrow evening.

I'll try to grab those and give them a test tomorrow, and hey, I've even got
a baseline to test against now.


-- 
Jarod Wilson
jarod@wilsonet.com

Re: [PATCH] hdpvr: enable IR part

[Jarod Wilson]

On Jan 15, 2011, at 12:37 AM, Jarod Wilson wrote:
...
>> BTW, a checkpatch and compiler tested lirc_zilog.c is here:
>> 
>> http://git.linuxtv.org/awalls/media_tree.git?a=shortlog;h=refs/heads/z8
>> 
>> It should fix all the binding and allocation problems related to
>> ir_probe()/ir_remove().  Except I suspect it may leak the Rx poll
>> kthread.  That's possibly another bug to add to the list.
>> 
>> Anyway, $DIETY knows if the lirc_zilog module actually still works after
>> all my hacks.  Give it a test if you are adventurous.  I won't be able
>> to test until tomorrow evening.
> 
> I'll try to grab those and give them a test tomorrow, and hey, I've even got
> a baseline to test against now.

Forgot to mention: I think it was suggested that one could use ir-kbd-i2c
for receive and lirc_zilog for transmit, at the same time. With ir-kbd-i2c
already loaded, lirc_zilog currently won't bind to anything.

-- 
Jarod Wilson
jarod@wilsonet.com

Re: [PATCH] hdpvr: enable IR part

[Jarod Wilson]

On Jan 15, 2011, at 12:37 AM, Jarod Wilson wrote:

> On Jan 14, 2011, at 11:35 PM, Andy Walls wrote:
> 
>> On Fri, 2011-01-14 at 21:30 -0500, Jarod Wilson wrote:
>>> On Jan 14, 2011, at 5:08 PM, Jarod Wilson wrote:
>> 
>>>>>> Registered IR keymap rc-hauppauge-new
>>>>>> input: i2c IR (HD PVR) as /devices/virtual/rc/rc1/input6
>>>>>> rc1: i2c IR (HD PVR) as /devices/virtual/rc/rc1
>>>>>> ir-kbd-i2c: i2c IR (HD PVR) detected at i2c-1/1-0071/ir0 [Hauppage HD PVR I2C]
>>> 
>>> And it *almost* works. Every key on the bundled remote is recognized, but
>>> every press gets feedback about like this w/evtest:
>>> 
>>> Event: time 1295058330.966180, type 4 (Misc), code 4 (ScanCode), value 29
>>> Event: time 1295058330.966212, type 1 (Key), code 401 (Blue), value 1
>>> Event: time 1295058330.966220, -------------- Report Sync ------------
>>> Event: time 1295058331.066175, type 4 (Misc), code 4 (ScanCode), value 29
>>> Event: time 1295058331.166290, type 4 (Misc), code 4 (ScanCode), value 29
>>> Event: time 1295058331.275664, type 4 (Misc), code 4 (ScanCode), value 29
>>> Event: time 1295058331.376160, type 4 (Misc), code 4 (ScanCode), value 29
>>> Event: time 1295058331.465505, type 1 (Key), code 401 (Blue), value 2
>>> Event: time 1295058331.476161, type 4 (Misc), code 4 (ScanCode), value 29
>>> Event: time 1295058331.498502, type 1 (Key), code 401 (Blue), value 2
>>> Event: time 1295058331.531504, type 1 (Key), code 401 (Blue), value 2
>>> Event: time 1295058331.564503, type 1 (Key), code 401 (Blue), value 2
>>> Event: time 1295058331.576153, type 4 (Misc), code 4 (ScanCode), value 29
>>> Event: time 1295058331.597502, type 1 (Key), code 401 (Blue), value 2
>>> Event: time 1295058331.630501, type 1 (Key), code 401 (Blue), value 2
>>> Event: time 1295058331.663502, type 1 (Key), code 401 (Blue), value 2
>>> Event: time 1295058331.696500, type 1 (Key), code 401 (Blue), value 2
>>> Event: time 1295058331.729503, type 1 (Key), code 401 (Blue), value 2
>>> Event: time 1295058331.762502, type 1 (Key), code 401 (Blue), value 2
>>> Event: time 1295058331.795500, type 1 (Key), code 401 (Blue), value 2
>>> Event: time 1295058331.825507, type 1 (Key), code 401 (Blue), value 0
>>> Event: time 1295058331.825526, -------------- Report Sync ------------
>>> 
>>> That's just a single short press. With arrow keys, its quite funky to
>>> watch in, say, mythtv UI menus. Hit up, and it moves up one, stalls for a
>>> second, then moves up several more.
>> 
>> Hmm bizzarre.
> 
> Shouldn't have said "stalls for a second", its a just brief pause, but
> regardless, yeah, its kinda odd.

Okay, last spam before I head off to bed... :)

I can get ir-kbd-i2c behavior to pretty much match lirc_zilog wrt key repeat,
by simply setting init_data->polling_interval = 260; in hdpvr-i2c.c, which
matches up with the delay in lirc_zilog. With the 260 interval:

Event: time 1295072449.490542, -------------- Report Sync ------------
Event: time 1295072453.321206, type 4 (Misc), code 4 (ScanCode), value 15
Event: time 1295072453.321245, type 1 (Key), code 108 (Down), value 1
Event: time 1295072453.321252, -------------- Report Sync ------------
Event: time 1295072453.570512, type 1 (Key), code 108 (Down), value 0
Event: time 1295072453.570544, -------------- Report Sync ------------
Event: time 1295072453.575718, type 4 (Misc), code 4 (ScanCode), value 15
Event: time 1295072453.575744, type 1 (Key), code 108 (Down), value 1
Event: time 1295072453.575752, -------------- Report Sync ------------
Event: time 1295072453.816215, type 4 (Misc), code 4 (ScanCode), value 15
Event: time 1295072454.065515, type 1 (Key), code 108 (Down), value 0
Event: time 1295072454.065544, -------------- Report Sync ------------

Lowering this a bit, I can get split personality, one press will look like
what I was originally seeing, another will look like the 260 output.

Adding filtering (return 0 if buf[0] != 0x80) doesn't help any.

The final thing I've noticed tonight is that ir-kbd-i2c calls rc_keydown
using a value of 0 for its 3rd parameter. From rc-main.c:

 * @toggle:     the toggle value (protocol dependent, if the protocol doesn't
 *              support toggle values, this should be set to zero)

Well, in this case, the protocol *does* use a toggle, so that's probably
something that could use fixing. Not sure it actually has anything to do with
the odd repeats I'm seeing. Okay, wasn't too much work to pass along toggle
values too, but it didn't help any.

I'll sleep on it.

-- 
Jarod Wilson
jarod@wilsonet.com

Re: [PATCH] hdpvr: enable IR part

[Andy Walls]

On Sat, 2011-01-15 at 01:56 -0500, Jarod Wilson wrote:
> On Jan 15, 2011, at 12:37 AM, Jarod Wilson wrote:

> >>>>>> Registered IR keymap rc-hauppauge-new
> >>>>>> input: i2c IR (HD PVR) as /devices/virtual/rc/rc1/input6
> >>>>>> rc1: i2c IR (HD PVR) as /devices/virtual/rc/rc1
> >>>>>> ir-kbd-i2c: i2c IR (HD PVR) detected at i2c-1/1-0071/ir0 [Hauppage HD PVR I2C]

> Okay, last spam before I head off to bed... :)
> 
> I can get ir-kbd-i2c behavior to pretty much match lirc_zilog wrt key repeat,
> by simply setting init_data->polling_interval = 260; in hdpvr-i2c.c, which
> matches up with the delay in lirc_zilog. With the 260 interval:

RC-5 has a repetition interval of about 4096/36kHz = 113.8 ms, IIRC.  

Using 260 ms, you are throwing away one repeat from the remote for sure,
maybe two.  Maybe that will help you understand what may be going on.
(I've lost the bubble on hdpvr with ir-kbd-i2c.)

Regards,
Andy

> Event: time 1295072449.490542, -------------- Report Sync ------------
> Event: time 1295072453.321206, type 4 (Misc), code 4 (ScanCode), value 15
> Event: time 1295072453.321245, type 1 (Key), code 108 (Down), value 1
> Event: time 1295072453.321252, -------------- Report Sync ------------
> Event: time 1295072453.570512, type 1 (Key), code 108 (Down), value 0
> Event: time 1295072453.570544, -------------- Report Sync ------------
> Event: time 1295072453.575718, type 4 (Misc), code 4 (ScanCode), value 15
> Event: time 1295072453.575744, type 1 (Key), code 108 (Down), value 1
> Event: time 1295072453.575752, -------------- Report Sync ------------
> Event: time 1295072453.816215, type 4 (Misc), code 4 (ScanCode), value 15
> Event: time 1295072454.065515, type 1 (Key), code 108 (Down), value 0
> Event: time 1295072454.065544, -------------- Report Sync ------------
> 
> Lowering this a bit, I can get split personality, one press will look like
> what I was originally seeing, another will look like the 260 output.
> 
> Adding filtering (return 0 if buf[0] != 0x80) doesn't help any.
> 
> The final thing I've noticed tonight is that ir-kbd-i2c calls rc_keydown
> using a value of 0 for its 3rd parameter. From rc-main.c:
> 
>  * @toggle:     the toggle value (protocol dependent, if the protocol doesn't
>  *              support toggle values, this should be set to zero)
> 
> Well, in this case, the protocol *does* use a toggle, so that's probably
> something that could use fixing. Not sure it actually has anything to do with
> the odd repeats I'm seeing. Okay, wasn't too much work to pass along toggle
> values too, but it didn't help any.
> 
> I'll sleep on it.

Re: [PATCH] hdpvr: enable IR part

[Jarod Wilson]

On Jan 15, 2011, at 4:56 PM, Andy Walls wrote:

> On Sat, 2011-01-15 at 01:56 -0500, Jarod Wilson wrote:
>> On Jan 15, 2011, at 12:37 AM, Jarod Wilson wrote:
> 
>>>>>>>> Registered IR keymap rc-hauppauge-new
>>>>>>>> input: i2c IR (HD PVR) as /devices/virtual/rc/rc1/input6
>>>>>>>> rc1: i2c IR (HD PVR) as /devices/virtual/rc/rc1
>>>>>>>> ir-kbd-i2c: i2c IR (HD PVR) detected at i2c-1/1-0071/ir0 [Hauppage HD PVR I2C]
> 
>> Okay, last spam before I head off to bed... :)
>> 
>> I can get ir-kbd-i2c behavior to pretty much match lirc_zilog wrt key repeat,
>> by simply setting init_data->polling_interval = 260; in hdpvr-i2c.c, which
>> matches up with the delay in lirc_zilog. With the 260 interval:
> 
> RC-5 has a repetition interval of about 4096/36kHz = 113.8 ms, IIRC.  
> 
> Using 260 ms, you are throwing away one repeat from the remote for sure,
> maybe two.

Yep. From lirc_zilog:

/*
 * This is ~113*2 + 24 + jitter (2*repeat gap +
 * code length).  We use this interval as the chip
 * resets every time you poll it (bad!).  This is
 * therefore just sufficient to catch all of the
 * button presses.  It makes the remote much more
 * responsive.  You can see the difference by
 * running irw and holding down a button.  With
 * 100ms, the old polling interval, you'll notice
 * breaks in the repeat sequence corresponding to
 * lost keypresses.
 */

But as noted previously, even that doesn't result in correct behavior from
lircd/irw's standpoint.


> Maybe that will help you understand what may be going on.
> (I've lost the bubble on hdpvr with ir-kbd-i2c.)

Not yet. After reading that comment in the code, I'd actually though that
something in the 113 to 130 range might actually be the ticket. I still
think that's probably correct, but it didn't make the wacky repeats stop,
so I think I need to instrument lirc_zilog and/or ir-kbd-i2c a bit more to
get a better idea what's going on.

Oh, and while drifting off to sleep last night, I think I came upon an
explanation for why things behave as they do with that 260ms interval. The
rc_keydown() call has an automatic key release after 250ms.


-- 
Jarod Wilson
jarod@wilsonet.com

Re: [PATCH] hdpvr: enable IR part

[Andy Walls]

On Sat, 2011-01-15 at 00:37 -0500, Jarod Wilson wrote:
> On Jan 14, 2011, at 11:35 PM, Andy Walls wrote:
> 

> 
> A single button press w/ir-kbd-i2c debugging and your patch:
> 
> ir-kbd-i2c: ir_poll_key
> ir-kbd-i2c: get_key_haup_common: received bytes: 80 00 00 fe 54 00        ....T.
> ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t1 dev=30 code=21
> ir-kbd-i2c: ir_poll_key
> ir-kbd-i2c: get_key_haup_common: received bytes: 80 00 00 fe 54 00        ....T.
> ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t1 dev=30 code=21
> ir-kbd-i2c: ir_poll_key
> ir-kbd-i2c: get_key_haup_common: received bytes: 80 00 00 fe 54 00        ....T.
> ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t1 dev=30 code=21
> ir-kbd-i2c: ir_poll_key
> ir-kbd-i2c: get_key_haup_common: received bytes: 80 00 00 fe 54 00        ....T.
> ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t1 dev=30 code=21
> ir-kbd-i2c: ir_poll_key
> ir-kbd-i2c: get_key_haup_common: received bytes: 80 00 00 fe 54 00        ....T.
> ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t1 dev=30 code=21
> ir-kbd-i2c: ir_poll_key
> ir-kbd-i2c: get_key_haup_common: received bytes: 80 00 00 fe 54 00        ....T.
> ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t1 dev=30 code=21
> 


FWIW, here's what an HVR-1600 and ir-kbd-i2c dumped out for me:

ir-kbd-i2c: get_key_haup_common: received bytes: 00 00 00 00 00 00   ......
ir-kbd-i2c: get_key_haup_common: received bytes: 00 00 00 00 00 00   ......
ir-kbd-i2c: get_key_haup_common: received bytes: 80 00 00 fe 24 00   ....$.
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t1 dev=30 code=9
ir-kbd-i2c: get_key_haup_common: received bytes: 80 00 00 fe 24 00   ....$.
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t1 dev=30 code=9
ir-kbd-i2c: get_key_haup_common: received bytes: 00 00 00 00 00 00   ......
ir-kbd-i2c: get_key_haup_common: received bytes: 00 00 00 00 00 00   ......
[...]
ir-kbd-i2c: get_key_haup_common: received bytes: 00 00 00 00 00 00   ......
ir-kbd-i2c: get_key_haup_common: received bytes: 00 00 00 00 00 00   ......
ir-kbd-i2c: get_key_haup_common: received bytes: 80 00 00 de 08 00   ......
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t0 dev=30 code=2
ir-kbd-i2c: get_key_haup_common: received bytes: 80 00 00 de 08 00   ......
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t0 dev=30 code=2
ir-kbd-i2c: get_key_haup_common: received bytes: 00 00 00 00 00 00   ......
ir-kbd-i2c: get_key_haup_common: received bytes: 00 00 00 00 00 00   ......
ir-kbd-i2c: get_key_haup_common: received bytes: 00 00 00 00 00 00   ......
ir-kbd-i2c: get_key_haup_common: received bytes: 80 00 00 de 08 00   ......
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t0 dev=30 code=2
ir-kbd-i2c: get_key_haup_common: received bytes: 80 00 00 de 08 00   ......
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t0 dev=30 code=2
ir-kbd-i2c: get_key_haup_common: received bytes: 80 00 00 de 08 00   ......
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t0 dev=30 code=2
ir-kbd-i2c: get_key_haup_common: received bytes: 80 00 00 de 08 00   ......
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t0 dev=30 code=2
ir-kbd-i2c: get_key_haup_common: received bytes: 80 00 00 de 08 00   ......
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t0 dev=30 code=2
ir-kbd-i2c: get_key_haup_common: received bytes: 00 00 00 00 00 00   ......
ir-kbd-i2c: get_key_haup_common: received bytes: 00 00 00 00 00 00   ......
ir-kbd-i2c: get_key_haup_common: received bytes: 00 00 00 00 00 00   ......
ir-kbd-i2c: get_key_haup_common: received bytes: 80 00 00 fe 08 00   ......
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t1 dev=30 code=2
ir-kbd-i2c: get_key_haup_common: received bytes: 80 00 00 fe 08 00   ......
ir-kbd-i2c: ir hauppauge (rc5): s1 r1 t1 dev=30 code=2
ir-kbd-i2c: get_key_haup_common: received bytes: 00 00 00 00 00 00   ......
ir-kbd-i2c: get_key_haup_common: received bytes: 00 00 00 00 00 00   ......

I did a momentary press of button '9' and got a single '9'.

I did a press and hold of button '2' during which I accidentally pointed
the remote away from the sensor and then back.  IIRC I got one '2' and
then many '2''s after an initial lag.  (What I might expect from holding
down a keybard key.)

I did a following momentary press of button '2'. I can't recall if I got
one or many '2''s for that.

Regards,
Andy

Re: [PATCH] hdpvr: enable IR part

[Andy Walls]

On Sat, 2011-01-15 at 00:37 -0500, Jarod Wilson wrote:
> On Jan 14, 2011, at 11:35 PM, Andy Walls wrote:

> Receive with lirc_zilog does actually work slightly better, though its still
> not perfect. Each key press (using irw to watch) always results in at least
> two lines of output, both with sequence number 00 (i.e., two distinct events),
> and holding a button down results in a stream of 00 events. So repeat is
> obviously busted. But I don't see the wackiness that is happening w/ir-kbd-i2c.
> 
> Oh, and transmit works too. So this patch and the buffer alloc patch have now
> been formally tested. Unless we go the custom get_key() route inside the hdpvr
> driver, I think the rest of the legwork to make the hdpvr's IR part behave is
> within lirc_zilog and ir-kbd-i2c (both of which I need to spend some more
> time reading over).
> 
> 
> > BTW, a checkpatch and compiler tested lirc_zilog.c is here:
> > 
> > http://git.linuxtv.org/awalls/media_tree.git?a=shortlog;h=refs/heads/z8
> > 
> > It should fix all the binding and allocation problems related to
> > ir_probe()/ir_remove().  Except I suspect it may leak the Rx poll
> > kthread.  That's possibly another bug to add to the list.
> > 
> > Anyway, $DIETY knows if the lirc_zilog module actually still works after
> > all my hacks.  Give it a test if you are adventurous.  I won't be able
> > to test until tomorrow evening.
> 
> I'll try to grab those and give them a test tomorrow, and hey, I've even got
> a baseline to test against now.

I have now confirmed that with all the above patches to lirc_zilog, both
Tx and Rx using an HVR-1600 work.

Time to start cleaning up the less important things I noticed...

Regards,
Andy

Re: [PATCH] hdpvr: enable IR part

[Andy Walls]

With my newly hacked lirc_zilog, try using the 'tx_only' parameter please.  It's not quite ready yet, but I'd like to know if it can bind.

If you already loaded a lirc_zilog without my latest patches, the i2c subsystem might have bogus crud still registered.  A reboot might be needed for a valid test.

R,
Andy 

Jarod Wilson <jarod@wilsonet.com> wrote:

>On Jan 15, 2011, at 12:37 AM, Jarod Wilson wrote:
>...
>>> BTW, a checkpatch and compiler tested lirc_zilog.c is here:
>>> 
>>> http://git.linuxtv.org/awalls/media_tree.git?a=shortlog;h=refs/heads/z8
>>> 
>>> It should fix all the binding and allocation problems related to
>>> ir_probe()/ir_remove().  Except I suspect it may leak the Rx poll
>>> kthread.  That's possibly another bug to add to the list.
>>> 
>>> Anyway, $DIETY knows if the lirc_zilog module actually still works after
>>> all my hacks.  Give it a test if you are adventurous.  I won't be able
>>> to test until tomorrow evening.
>> 
>> I'll try to grab those and give them a test tomorrow, and hey, I've even got
>> a baseline to test against now.
>
>Forgot to mention: I think it was suggested that one could use ir-kbd-i2c
>for receive and lirc_zilog for transmit, at the same time. With ir-kbd-i2c
>already loaded, lirc_zilog currently won't bind to anything.
>
>-- 
>Jarod Wilson
>jarod@wilsonet.com
>
>
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-media" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] hdpvr: enable IR part

[Andy Walls]

On Sat, 2011-01-15 at 07:50 -0500, Andy Walls wrote:
> Jarod Wilson <jarod@wilsonet.com> wrote:

> >Forgot to mention: I think it was suggested that one could use ir-kbd-i2c
> >for receive and lirc_zilog for transmit, at the same time. With ir-kbd-i2c
> >already loaded, lirc_zilog currently won't bind to anything.


> With my newly hacked lirc_zilog, try using the 'tx_only' parameter
> please.  It's not quite ready yet, but I'd like to know if it can
> bind.

I have now tested this.

Using the 'tx_only' module parameter to lirc_zilog appears to allow
ir-kbd-i2c and lirc_zilog to coexist, for I2C subsystem binding at
least.

It does not appear to matter what order the two modules are loaded. I
tried it both ways.


However, lirc_zilog sharing of Z8 is not fully functional yet.  I need
to change things to have the bridge drivers provide a IR transceiver
mutex to both lirc_zilog and ir-kbd-i2c.  lirc_zilog and ir-kbd-i2c
would use that mutex for exclusive access to the Z8 when needed, if it
was provided by the bridge driver.

I view proper sharing of the Z8 as an important requirement, because of
two use cases:

1. User only wants to use the Z8 for IR Rx.  User doesn't want to fetch
the lirc_zilog required firmware or perform any LIRC setup.

2. User only wants to use the Z8 for IR Tx.  User uses some other
ir-kbd-i2c supported receiver and remote IR Rx.

Maybe use case #2 is too rare to worry about?
However, if one accepts both use cases as valid, then ir-kbd-i2c must
support the Z8, and lirc_zilog must be able to coexist with ir-kbd-i2c.

Proper sharing of the Z8 is, however, lower on my to-do list than fixing
some internal lirc_zilog problems.

Regards,
Andy