[PATCH 1/3] hdpvr: Add I2C and ir-kdb-i2c registration of the Zilog Z8 IR chip

[PATCH 1/3] hdpvr: Add I2C and ir-kdb-i2c registration of the Zilog Z8 IR chip

[Andy Walls]

Add I2C registration of the Zilog Z8F0811 IR microcontroller for either
lirc_zilog or ir-kbd-i2c to use.  This is a required step in removing
lirc_zilog's use of the deprecated struct i2c_adapter.id field.

Signed-off-by: Andy Walls <awalls@md.metrocast.net>
---
 drivers/media/video/hdpvr/hdpvr-core.c |    5 +++
 drivers/media/video/hdpvr/hdpvr-i2c.c  |   53 ++++++++++++++++++++++++++++++++
 drivers/media/video/hdpvr/hdpvr.h      |    6 +++
 3 files changed, 64 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/hdpvr/hdpvr-core.c b/drivers/media/video/hdpvr/hdpvr-core.c
index b70d6af..f7d1ee5 100644
--- a/drivers/media/video/hdpvr/hdpvr-core.c
+++ b/drivers/media/video/hdpvr/hdpvr-core.c
@@ -385,6 +385,11 @@ static int hdpvr_probe(struct usb_interface *interface,
 		v4l2_err(&dev->v4l2_dev, "registering i2c adapter failed\n");
 		goto error;
 	}
+
+	/* until i2c is working properly */
+	retval = 0; /* hdpvr_register_i2c_ir(dev); */
+	if (retval < 0)
+		v4l2_err(&dev->v4l2_dev, "registering i2c IR devices failed\n");
 #endif /* CONFIG_I2C */
 
 	/* let the user know what node this device is now attached to */
diff --git a/drivers/media/video/hdpvr/hdpvr-i2c.c b/drivers/media/video/hdpvr/hdpvr-i2c.c
index 409de11..24966aa 100644
--- a/drivers/media/video/hdpvr/hdpvr-i2c.c
+++ b/drivers/media/video/hdpvr/hdpvr-i2c.c
@@ -4,6 +4,9 @@
  *
  * Copyright (C) 2008      Janne Grunau (j@jannau.net)
  *
+ * IR device registration code is
+ * Copyright (C) 2010	Andy Walls <awalls@md.metrocast.net>
+ *
  *	This program is free software; you can redistribute it and/or
  *	modify it under the terms of the GNU General Public License as
  *	published by the Free Software Foundation, version 2.
@@ -22,6 +25,56 @@
 #define REQTYPE_I2C_WRITE	0xb0
 #define REQTYPE_I2C_WRITE_STATT	0xd0
 
+#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 int hdpvr_new_i2c_ir(struct hdpvr_device *dev, struct i2c_adapter *adap,
+			    const char *type, u8 addr)
+{
+	struct i2c_board_info info;
+	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;
+}
+
+int hdpvr_register_i2c_ir(struct hdpvr_device *dev)
+{
+	int i;
+	int ret = 0;
+
+	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;
+}
+
 static int hdpvr_i2c_read(struct hdpvr_device *dev, unsigned char addr,
 			  char *data, int len)
 {
diff --git a/drivers/media/video/hdpvr/hdpvr.h b/drivers/media/video/hdpvr/hdpvr.h
index 5efc963..37f1e4c 100644
--- a/drivers/media/video/hdpvr/hdpvr.h
+++ b/drivers/media/video/hdpvr/hdpvr.h
@@ -16,6 +16,7 @@
 #include <linux/videodev2.h>
 
 #include <media/v4l2-device.h>
+#include <media/ir-kbd-i2c.h>
 
 #define HDPVR_MAJOR_VERSION 0
 #define HDPVR_MINOR_VERSION 2
@@ -109,6 +110,9 @@ struct hdpvr_device {
 	/* I2C lock */
 	struct mutex		i2c_mutex;
 
+	/* For passing data to ir-kbd-i2c */
+	struct IR_i2c_init_data	ir_i2c_init_data;
+
 	/* usb control transfer buffer and lock */
 	struct mutex		usbc_mutex;
 	u8			*usbc_buf;
@@ -306,6 +310,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);
+
 /*========================================================================*/
 /* buffer management */
 int hdpvr_free_buffers(struct hdpvr_device *dev);
-- 
1.7.2.1

Re: [PATCH 1/3] hdpvr: Add I2C and ir-kdb-i2c registration of the Zilog Z8 IR chip

[Mauro Carvalho Chehab]

Em 28-12-2010 23:46, Andy Walls escreveu:
> 
> Add I2C registration of the Zilog Z8F0811 IR microcontroller for either
> lirc_zilog or ir-kbd-i2c to use.  This is a required step in removing
> lirc_zilog's use of the deprecated struct i2c_adapter.id field.
> 
> Signed-off-by: Andy Walls <awalls@md.metrocast.net>
> ---
>  drivers/media/video/hdpvr/hdpvr-core.c |    5 +++
>  drivers/media/video/hdpvr/hdpvr-i2c.c  |   53 ++++++++++++++++++++++++++++++++
>  drivers/media/video/hdpvr/hdpvr.h      |    6 +++
>  3 files changed, 64 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/hdpvr/hdpvr-core.c b/drivers/media/video/hdpvr/hdpvr-core.c
> index b70d6af..f7d1ee5 100644
> --- a/drivers/media/video/hdpvr/hdpvr-core.c
> +++ b/drivers/media/video/hdpvr/hdpvr-core.c
> @@ -385,6 +385,11 @@ static int hdpvr_probe(struct usb_interface *interface,
>  		v4l2_err(&dev->v4l2_dev, "registering i2c adapter failed\n");
>  		goto error;
>  	}
> +
> +	/* until i2c is working properly */
> +	retval = 0; /* hdpvr_register_i2c_ir(dev); */

Hmm... It seems that this will just disable the IR logic... Why do you need it?
Your comment is not clear to me.

> +	if (retval < 0)
> +		v4l2_err(&dev->v4l2_dev, "registering i2c IR devices failed\n");
>  #endif /* CONFIG_I2C */
>  
>  	/* let the user know what node this device is now attached to */
> diff --git a/drivers/media/video/hdpvr/hdpvr-i2c.c b/drivers/media/video/hdpvr/hdpvr-i2c.c
> index 409de11..24966aa 100644
> --- a/drivers/media/video/hdpvr/hdpvr-i2c.c
> +++ b/drivers/media/video/hdpvr/hdpvr-i2c.c
> @@ -4,6 +4,9 @@
>   *
>   * Copyright (C) 2008      Janne Grunau (j@jannau.net)
>   *
> + * IR device registration code is
> + * Copyright (C) 2010	Andy Walls <awalls@md.metrocast.net>
> + *
>   *	This program is free software; you can redistribute it and/or
>   *	modify it under the terms of the GNU General Public License as
>   *	published by the Free Software Foundation, version 2.
> @@ -22,6 +25,56 @@
>  #define REQTYPE_I2C_WRITE	0xb0
>  #define REQTYPE_I2C_WRITE_STATT	0xd0
>  
> +#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 int hdpvr_new_i2c_ir(struct hdpvr_device *dev, struct i2c_adapter *adap,
> +			    const char *type, u8 addr)
> +{
> +	struct i2c_board_info info;
> +	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;
> +}
> +
> +int hdpvr_register_i2c_ir(struct hdpvr_device *dev)
> +{
> +	int i;
> +	int ret = 0;
> +
> +	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;
> +}
> +
>  static int hdpvr_i2c_read(struct hdpvr_device *dev, unsigned char addr,
>  			  char *data, int len)
>  {
> diff --git a/drivers/media/video/hdpvr/hdpvr.h b/drivers/media/video/hdpvr/hdpvr.h
> index 5efc963..37f1e4c 100644
> --- a/drivers/media/video/hdpvr/hdpvr.h
> +++ b/drivers/media/video/hdpvr/hdpvr.h
> @@ -16,6 +16,7 @@
>  #include <linux/videodev2.h>
>  
>  #include <media/v4l2-device.h>
> +#include <media/ir-kbd-i2c.h>
>  
>  #define HDPVR_MAJOR_VERSION 0
>  #define HDPVR_MINOR_VERSION 2
> @@ -109,6 +110,9 @@ struct hdpvr_device {
>  	/* I2C lock */
>  	struct mutex		i2c_mutex;
>  
> +	/* For passing data to ir-kbd-i2c */
> +	struct IR_i2c_init_data	ir_i2c_init_data;
> +
>  	/* usb control transfer buffer and lock */
>  	struct mutex		usbc_mutex;
>  	u8			*usbc_buf;
> @@ -306,6 +310,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);
> +
>  /*========================================================================*/
>  /* buffer management */
>  int hdpvr_free_buffers(struct hdpvr_device *dev);

Re: [PATCH 1/3] hdpvr: Add I2C and ir-kdb-i2c registration of the Zilog Z8 IR chip

[Andy Walls]

On Wed, 2010-12-29 at 09:08 -0200, Mauro Carvalho Chehab wrote:
> Em 28-12-2010 23:46, Andy Walls escreveu:
> > 
> > Add I2C registration of the Zilog Z8F0811 IR microcontroller for either
> > lirc_zilog or ir-kbd-i2c to use.  This is a required step in removing
> > lirc_zilog's use of the deprecated struct i2c_adapter.id field.
> > 
> > Signed-off-by: Andy Walls <awalls@md.metrocast.net>
> > ---
> >  drivers/media/video/hdpvr/hdpvr-core.c |    5 +++
> >  drivers/media/video/hdpvr/hdpvr-i2c.c  |   53 ++++++++++++++++++++++++++++++++
> >  drivers/media/video/hdpvr/hdpvr.h      |    6 +++
> >  3 files changed, 64 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/media/video/hdpvr/hdpvr-core.c b/drivers/media/video/hdpvr/hdpvr-core.c
> > index b70d6af..f7d1ee5 100644
> > --- a/drivers/media/video/hdpvr/hdpvr-core.c
> > +++ b/drivers/media/video/hdpvr/hdpvr-core.c
> > @@ -385,6 +385,11 @@ static int hdpvr_probe(struct usb_interface *interface,
> >  		v4l2_err(&dev->v4l2_dev, "registering i2c adapter failed\n");
> >  		goto error;
> >  	}
> > +
> > +	/* until i2c is working properly */
> > +	retval = 0; /* hdpvr_register_i2c_ir(dev); */
> 
> Hmm... It seems that this will just disable the IR logic... Why do you need it?
> Your comment is not clear to me.
>
> > +	if (retval < 0)
> > +		v4l2_err(&dev->v4l2_dev, "registering i2c IR devices failed\n");
> >  #endif /* CONFIG_I2C */

The diff did not provide you with all the context you needed.  See:

http://git.linuxtv.org/awalls/media_tree.git?a=blob;f=drivers/media/video/hdpvr/hdpvr-core.c;h=f7d1ee55185a527c4b0302acdbd30c5b5f523c1c;hb=15a40f5388f382243b6bc41c6874e1549324e789#l380

Note just before my addition, that the I2C adapter of the HD PVR is not
currently being registered in the existing hdpvr code:

 382         /* until i2c is working properly */
 383         retval = 0; /* hdpvr_register_i2c_adapter(dev); */
 384         if (retval < 0) {
 385                 v4l2_err(&dev->v4l2_dev, "registering i2c adapter failed\n");
 386                 goto error;
 387         }
 388 
 389         /* until i2c is working properly */
 390         retval = 0; /* hdpvr_register_i2c_ir(dev); */
 391         if (retval < 0)
 392                 v4l2_err(&dev->v4l2_dev, "registering i2c IR devices failed\n");

A few years ago, Brandon Jenkins was able to trigger a USB/I2C related
Oops when both the hdpvr and cx18 drivers were loaded in his system.  At
the time Janne determined it would be prudent to disable hdpvr I2C
support.

I'd really like Janne to comment on this, but it may be that hdpvr I2C
problems are no longer an issue(?). See this thread:

http://www.mail-archive.com/linux-media@vger.kernel.org/msg09163.html
  
If hdpvr I2C functionality is ever going to be re-enabled, these IR
changes make hdpvr consistent with what ivtv and cx18 do for their Zilog
Z8F0811 IR controller.

IF hdpvr I2C functionality is *never* going to be re-enabled, then this
whole patch can be dropped.

> >  	/* let the user know what node this device is now attached to */
> > diff --git a/drivers/media/video/hdpvr/hdpvr-i2c.c b/drivers/media/video/hdpvr/hdpvr-i2c.c
> > index 409de11..24966aa 100644
> > --- a/drivers/media/video/hdpvr/hdpvr-i2c.c
> > +++ b/drivers/media/video/hdpvr/hdpvr-i2c.c
> > @@ -4,6 +4,9 @@
> >   *
> >   * Copyright (C) 2008      Janne Grunau (j@jannau.net)
> >   *
> > + * IR device registration code is
> > + * Copyright (C) 2010	Andy Walls <awalls@md.metrocast.net>
> > + *
> >   *	This program is free software; you can redistribute it and/or
> >   *	modify it under the terms of the GNU General Public License as
> >   *	published by the Free Software Foundation, version 2.
> > @@ -22,6 +25,56 @@
> >  #define REQTYPE_I2C_WRITE	0xb0
> >  #define REQTYPE_I2C_WRITE_STATT	0xd0
> >  
> > +#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 int hdpvr_new_i2c_ir(struct hdpvr_device *dev, struct i2c_adapter *adap,
> > +			    const char *type, u8 addr)
> > +{
> > +	struct i2c_board_info info;
> > +	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;
> > +}

For comparison of the above with cx18 (very similar) and ivtv (similar
but more complex), see:

http://git.linuxtv.org/awalls/media_tree.git?a=blob;f=drivers/media/video/cx18/cx18-i2c.c;h=c330fb917b50bfd0ea1f3f22fb3d378d9fe3f2b0;hb=79254c3618ed214e7750c54dcd6c8c591314c385#l86
http://git.linuxtv.org/awalls/media_tree.git?a=blob;f=drivers/media/video/ivtv/ivtv-i2c.c;h=68170924448c3894dfa0db524b975eddee0ee8c0;hb=79254c3618ed214e7750c54dcd6c8c591314c385#l148


Regards,
Andy

> > +int hdpvr_register_i2c_ir(struct hdpvr_device *dev)
> > +{
> > +	int i;
> > +	int ret = 0;
> > +
> > +	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;
> > +}
> > +
> >  static int hdpvr_i2c_read(struct hdpvr_device *dev, unsigned char addr,
> >  			  char *data, int len)
> >  {
> > diff --git a/drivers/media/video/hdpvr/hdpvr.h b/drivers/media/video/hdpvr/hdpvr.h
> > index 5efc963..37f1e4c 100644
> > --- a/drivers/media/video/hdpvr/hdpvr.h
> > +++ b/drivers/media/video/hdpvr/hdpvr.h
> > @@ -16,6 +16,7 @@
> >  #include <linux/videodev2.h>
> >  
> >  #include <media/v4l2-device.h>
> > +#include <media/ir-kbd-i2c.h>
> >  
> >  #define HDPVR_MAJOR_VERSION 0
> >  #define HDPVR_MINOR_VERSION 2
> > @@ -109,6 +110,9 @@ struct hdpvr_device {
> >  	/* I2C lock */
> >  	struct mutex		i2c_mutex;
> >  
> > +	/* For passing data to ir-kbd-i2c */
> > +	struct IR_i2c_init_data	ir_i2c_init_data;
> > +
> >  	/* usb control transfer buffer and lock */
> >  	struct mutex		usbc_mutex;
> >  	u8			*usbc_buf;
> > @@ -306,6 +310,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);
> > +
> >  /*========================================================================*/
> >  /* buffer management */
> >  int hdpvr_free_buffers(struct hdpvr_device *dev);
> 

Re: [PATCH 1/3] hdpvr: Add I2C and ir-kdb-i2c registration of the Zilog Z8 IR chip

[Jean Delvare]

On Tue, 28 Dec 2010 20:46:13 -0500, Andy Walls wrote:
> 
> Add I2C registration of the Zilog Z8F0811 IR microcontroller for either
> lirc_zilog or ir-kbd-i2c to use.  This is a required step in removing
> lirc_zilog's use of the deprecated struct i2c_adapter.id field.
> 
> Signed-off-by: Andy Walls <awalls@md.metrocast.net>
> ---
>  drivers/media/video/hdpvr/hdpvr-core.c |    5 +++
>  drivers/media/video/hdpvr/hdpvr-i2c.c  |   53 ++++++++++++++++++++++++++++++++
>  drivers/media/video/hdpvr/hdpvr.h      |    6 +++
>  3 files changed, 64 insertions(+), 0 deletions(-)

Looks good to me (even though it looks strange to update code which is
apparently disabled for quite a while...)

Acked-by: Jean Delvare <khali@linux-fr.org>

-- 
Jean Delvare

Re: [PATCH 1/3] hdpvr: Add I2C and ir-kdb-i2c registration of the Zilog Z8 IR chip

[Andy Walls]

On Wed, 2011-01-05 at 13:50 +0100, Jean Delvare wrote:
> On Tue, 28 Dec 2010 20:46:13 -0500, Andy Walls wrote:
> > 
> > Add I2C registration of the Zilog Z8F0811 IR microcontroller for either
> > lirc_zilog or ir-kbd-i2c to use.  This is a required step in removing
> > lirc_zilog's use of the deprecated struct i2c_adapter.id field.
> > 
> > Signed-off-by: Andy Walls <awalls@md.metrocast.net>
> > ---
> >  drivers/media/video/hdpvr/hdpvr-core.c |    5 +++
> >  drivers/media/video/hdpvr/hdpvr-i2c.c  |   53 ++++++++++++++++++++++++++++++++
> >  drivers/media/video/hdpvr/hdpvr.h      |    6 +++
> >  3 files changed, 64 insertions(+), 0 deletions(-)
> 
> Looks good to me (even though it looks strange to update code which is
> apparently disabled for quite a while...)

Yes it is odd. My intent was ensure that after removal of adap->id, that
the hdpvr driver would, in the future, identify its device in a manner
lirc_zilog was expecting.

I don't have a HD PVR with which to test things myself.  So putting in
all the plumbing lowers the barrier for developers like Jarrod or Janne
to test lirc_zilog with an HD PVR when/if they re-enable I2C in hdpvr.

Regards,
Andy

> Acked-by: Jean Delvare <khali@linux-fr.org>

[PATCH 2/3] ir-kbd-i2c: Add HD PVR IR Rx support to ir-kbd-i2c

[Andy Walls]

Add HD PVR IR Rx support to ir-kbd-i2c

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

---
 drivers/media/video/ir-kbd-i2c.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/ir-kbd-i2c.c b/drivers/media/video/ir-kbd-i2c.c
index dd54c3d..c87b6bc 100644
--- a/drivers/media/video/ir-kbd-i2c.c
+++ b/drivers/media/video/ir-kbd-i2c.c
@@ -449,6 +449,7 @@ static const struct i2c_device_id ir_kbd_id[] = {
 	{ "ir_video", 0 },
 	/* IR device specific entries should be added here */
 	{ "ir_rx_z8f0811_haup", 0 },
+	{ "ir_rx_z8f0811_hdpvr", 0 },
 	{ }
 };
 
-- 
1.7.2.1

Re: [PATCH 2/3] ir-kbd-i2c: Add HD PVR IR Rx support to ir-kbd-i2c

[Mauro Carvalho Chehab]

Em 28-12-2010 23:47, Andy Walls escreveu:
> 
> Add HD PVR IR Rx support to ir-kbd-i2c

Hmm... I know nothing about the hardware designs used on hd-pvr, but
it seems wrong to have both lirc-zilog and ir-kbd-i2c registering for
RX for the same device.

There are variants with hd-pvr that uses ir-kbd-i2c and others
that use another I2C chipset? Or, for some versions of Z8, the RX logic
is identical to the one provided by ir-kbd-i2c?

> 
> Signed-off-by: Andy Walls <awalls@md.metrocast.net>
> 
> ---
>  drivers/media/video/ir-kbd-i2c.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/ir-kbd-i2c.c b/drivers/media/video/ir-kbd-i2c.c
> index dd54c3d..c87b6bc 100644
> --- a/drivers/media/video/ir-kbd-i2c.c
> +++ b/drivers/media/video/ir-kbd-i2c.c
> @@ -449,6 +449,7 @@ static const struct i2c_device_id ir_kbd_id[] = {
>  	{ "ir_video", 0 },
>  	/* IR device specific entries should be added here */
>  	{ "ir_rx_z8f0811_haup", 0 },
> +	{ "ir_rx_z8f0811_hdpvr", 0 },
>  	{ }
>  };
>  

Re: [PATCH 2/3] ir-kbd-i2c: Add HD PVR IR Rx support to ir-kbd-i2c

[Andy Walls]

On Wed, 2010-12-29 at 09:12 -0200, Mauro Carvalho Chehab wrote:
> Em 28-12-2010 23:47, Andy Walls escreveu:
> > 
> > Add HD PVR IR Rx support to ir-kbd-i2c
> 
> Hmm... I know nothing about the hardware designs used on hd-pvr, but
> it seems wrong to have both lirc-zilog and ir-kbd-i2c registering for
> RX for the same device.

There are some historical reasons for why that is the case

1. ir-kbd-i2c:
	a. was in kernel
	b. provides Rx keycodes using the linux input subsystem
	c. the program in the Z8 outputs RX data that is compatible
	    with the Rx data output by IR microcontrollers on older
	    Hauppauge designs.
	d. the keymap was relatively fixed

2. lirc_i2c:
	a. was not in kernel
	b. provided Rx data the "LIRC way" via /dev/lirc
	c. the program in the Z8 outputs RX data that is compatible
	    with the Rx data output by IR microcontrollers on older
	    Hauppauge designs.
	d. the LIRC keymap could be changed by the user

3. lirc_pvr150 (aka lirc_zilog):
	a. was not in kernel and was not in LIRC
	b. provided both Rx and Tx interfaces the "LIRC way"
	    via /dev/lirc
	c. Any user who whats to use the Z8 for IR Tx has to use this
	    module
	d. old linux kernels had problems with the Z8 getting
	    hung up on the I2C bus when doing Tx and RX, and Z8
	    chip resets to recover from than hang affected Rx.



> There are variants with hd-pvr that uses ir-kbd-i2c and others
> that use another I2C chipset? 
> Or, for some versions of Z8, the RX logic
> is identical to the one provided by ir-kbd-i2c?

There are no real variants AFAIK.  All HD PVRs should have a Z8 chip and
are capable of IR Tx and Rx

The program in the Z8 outputs RX data that is compatible with the Rx
data output by IR microcontrollers on older Hauppauge designs.

The lirc_zilog.c code implies some Z8's might not support Tx, but I
suspect that case is really just another chip at the same Z8 Rx I2C
address (0x71) that is not actually a Z8.  


Having multiple modules support the device gives the end user some
choice.

1. ir-kbd-i2c supports the "it just works" use case that was discussed
when implementing the new IR/RC core.


2. A user that only wants Rx with the Z8, uses either ir-kbd-i2c or
lirc_i2c.  They are more reliable.

Since the keymaps for ir-kbd-i2c are now modifiable from userspace,  and
if "cooked" IR Rx data is not passed out via /dev/lirc anymore, then Z8
support can be pulled out of lirc_i2c.  Most or all of lirc_i2c can be
collapsed into ir-kbd-i2c anyway.


3. A user that wants Tx with the Z8 must use lirc_zilog, which needs a
"firmware" and which also handles Rx at the moment.

The reasons for keeping Rx and Tx coupled in lirc_zilog are getting
thin, so it may be possible to convert lirc_zilog to only handle Tx, but
I'm not sure yet.  There may still need to be some sort of locking
between accessing the Tx and Rx I2C addresses of the Z8.

Regards,
Andy

> > 
> > Signed-off-by: Andy Walls <awalls@md.metrocast.net>
> > 
> > ---
> >  drivers/media/video/ir-kbd-i2c.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/media/video/ir-kbd-i2c.c b/drivers/media/video/ir-kbd-i2c.c
> > index dd54c3d..c87b6bc 100644
> > --- a/drivers/media/video/ir-kbd-i2c.c
> > +++ b/drivers/media/video/ir-kbd-i2c.c
> > @@ -449,6 +449,7 @@ static const struct i2c_device_id ir_kbd_id[] = {
> >  	{ "ir_video", 0 },
> >  	/* IR device specific entries should be added here */
> >  	{ "ir_rx_z8f0811_haup", 0 },
> > +	{ "ir_rx_z8f0811_hdpvr", 0 },
> >  	{ }
> >  };
> >  
> 

Re: [PATCH 2/3] ir-kbd-i2c: Add HD PVR IR Rx support to ir-kbd-i2c

[Jean Delvare]

On Tue, 28 Dec 2010 20:47:46 -0500, Andy Walls wrote:
> 
> Add HD PVR IR Rx support to ir-kbd-i2c
> 
> Signed-off-by: Andy Walls <awalls@md.metrocast.net>
> 
> ---
>  drivers/media/video/ir-kbd-i2c.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/ir-kbd-i2c.c b/drivers/media/video/ir-kbd-i2c.c
> index dd54c3d..c87b6bc 100644
> --- a/drivers/media/video/ir-kbd-i2c.c
> +++ b/drivers/media/video/ir-kbd-i2c.c
> @@ -449,6 +449,7 @@ static const struct i2c_device_id ir_kbd_id[] = {
>  	{ "ir_video", 0 },
>  	/* IR device specific entries should be added here */
>  	{ "ir_rx_z8f0811_haup", 0 },
> +	{ "ir_rx_z8f0811_hdpvr", 0 },
>  	{ }
>  };
>  

Acked-by: Jean Delvare <khali@linux-fr.org>

-- 
Jean Delvare

[PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field

[Andy Walls]

Remove use of deprecated struct i2c_adapter.id field.  In the process,
perform different detection of the HD PVR's Z8 IR microcontroller versus
the other Hauppauge cards with the Z8 IR microcontroller.

Also added a comment about probe() function behavior that needs to be
fixed.

Signed-off-by: Andy Walls <awalls@md.metrocast.net>
---
 drivers/staging/lirc/lirc_zilog.c |   47 ++++++++++++++++++++++++------------
 1 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/lirc/lirc_zilog.c b/drivers/staging/lirc/lirc_zilog.c
index 52be6de..ad29bb1 100644
--- a/drivers/staging/lirc/lirc_zilog.c
+++ b/drivers/staging/lirc/lirc_zilog.c
@@ -66,6 +66,7 @@ struct IR {
 	/* Device info */
 	struct mutex ir_lock;
 	int open;
+	bool is_hdpvr;
 
 	/* RX device */
 	struct i2c_client c_rx;
@@ -206,16 +207,12 @@ static int add_to_buf(struct IR *ir)
 		}
 
 		/* key pressed ? */
-#ifdef I2C_HW_B_HDPVR
-		if (ir->c_rx.adapter->id == I2C_HW_B_HDPVR) {
+		if (ir->is_hdpvr) {
 			if (got_data && (keybuf[0] == 0x80))
 				return 0;
 			else if (got_data && (keybuf[0] == 0x00))
 				return -ENODATA;
 		} else if ((ir->b[0] & 0x80) == 0)
-#else
-		if ((ir->b[0] & 0x80) == 0)
-#endif
 			return got_data ? 0 : -ENODATA;
 
 		/* look what we have */
@@ -841,15 +838,15 @@ static int send_code(struct IR *ir, unsigned int code, unsigned int key)
 		return ret < 0 ? ret : -EFAULT;
 	}
 
-#ifdef I2C_HW_B_HDPVR
 	/*
 	 * The sleep bits aren't necessary on the HD PVR, and in fact, the
 	 * last i2c_master_recv always fails with a -5, so for now, we're
 	 * going to skip this whole mess and say we're done on the HD PVR
 	 */
-	if (ir->c_rx.adapter->id == I2C_HW_B_HDPVR)
-		goto done;
-#endif
+	if (ir->is_hdpvr) {
+		dprintk("sent code %u, key %u\n", code, key);
+		return 0;
+	}
 
 	/*
 	 * This bit NAKs until the device is ready, so we retry it
@@ -1111,12 +1108,14 @@ static int ir_remove(struct i2c_client *client);
 static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id);
 static int ir_command(struct i2c_client *client, unsigned int cmd, void *arg);
 
+#define ID_FLAG_TX	0x01
+#define ID_FLAG_HDPVR	0x02
+
 static const struct i2c_device_id ir_transceiver_id[] = {
-	/* Generic entry for any IR transceiver */
-	{ "ir_video", 0 },
-	/* IR device specific entries should be added here */
-	{ "ir_tx_z8f0811_haup", 0 },
-	{ "ir_rx_z8f0811_haup", 0 },
+	{ "ir_tx_z8f0811_haup",  ID_FLAG_TX                 },
+	{ "ir_rx_z8f0811_haup",  0                          },
+	{ "ir_tx_z8f0811_hdpvr", ID_FLAG_HDPVR | ID_FLAG_TX },
+	{ "ir_rx_z8f0811_hdpvr", ID_FLAG_HDPVR              },
 	{ }
 };
 
@@ -1196,10 +1195,25 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	int ret;
 	int have_rx = 0, have_tx = 0;
 
-	dprintk("%s: adapter id=0x%x, client addr=0x%02x\n",
-		__func__, adap->id, client->addr);
+	dprintk("%s: adapter name (%s) nr %d, i2c_device_id name (%s), "
+		"client addr=0x%02x\n",
+		__func__, adap->name, adap->nr, id->name, client->addr);
 
 	/*
+	 * FIXME - This probe function probes both the Tx and Rx
+	 * addresses of the IR microcontroller.
+	 *
+	 * However, the I2C subsystem is passing along one I2C client at a
+	 * time, based on matches to the ir_transceiver_id[] table above.
+	 * The expectation is that each i2c_client address will be probed
+	 * individually by drivers so the I2C subsystem can mark all client
+	 * addresses as claimed or not.
+	 *
+	 * This probe routine causes only one of the client addresses, TX or RX,
+	 * to be claimed.  This will cause a problem if the I2C subsystem is
+	 * subsequently triggered to probe unclaimed clients again.
+	 */
+	/*
 	 * The external IR receiver is at i2c address 0x71.
 	 * The IR transmitter is at 0x70.
 	 */
@@ -1241,6 +1255,7 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	mutex_init(&ir->ir_lock);
 	mutex_init(&ir->buf_lock);
 	ir->need_boot = 1;
+	ir->is_hdpvr = (id->driver_data & ID_FLAG_HDPVR) ? true : false;
 
 	memcpy(&ir->l, &lirc_template, sizeof(struct lirc_driver));
 	ir->l.minor = -1;
-- 
1.7.2.1

Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field

[Jean Delvare]

Hi Andy,

On Tue, 28 Dec 2010 20:49:50 -0500, Andy Walls wrote:
> Remove use of deprecated struct i2c_adapter.id field.  In the process,
> perform different detection of the HD PVR's Z8 IR microcontroller versus
> the other Hauppauge cards with the Z8 IR microcontroller.

Thanks a lot for doing this. I'll be very happy when we can finally get
rid of i2c_adapter.id.

> Also added a comment about probe() function behavior that needs to be
> fixed.

See my suggestion inline below.

> Signed-off-by: Andy Walls <awalls@md.metrocast.net>
> ---
>  drivers/staging/lirc/lirc_zilog.c |   47 ++++++++++++++++++++++++------------
>  1 files changed, 31 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/staging/lirc/lirc_zilog.c b/drivers/staging/lirc/lirc_zilog.c
> index 52be6de..ad29bb1 100644
> --- a/drivers/staging/lirc/lirc_zilog.c
> +++ b/drivers/staging/lirc/lirc_zilog.c
> @@ -66,6 +66,7 @@ struct IR {
>  	/* Device info */
>  	struct mutex ir_lock;
>  	int open;
> +	bool is_hdpvr;
>  
>  	/* RX device */
>  	struct i2c_client c_rx;
> @@ -206,16 +207,12 @@ static int add_to_buf(struct IR *ir)
>  		}
>  
>  		/* key pressed ? */
> -#ifdef I2C_HW_B_HDPVR
> -		if (ir->c_rx.adapter->id == I2C_HW_B_HDPVR) {
> +		if (ir->is_hdpvr) {
>  			if (got_data && (keybuf[0] == 0x80))
>  				return 0;
>  			else if (got_data && (keybuf[0] == 0x00))
>  				return -ENODATA;
>  		} else if ((ir->b[0] & 0x80) == 0)
> -#else
> -		if ((ir->b[0] & 0x80) == 0)
> -#endif
>  			return got_data ? 0 : -ENODATA;
>  
>  		/* look what we have */
> @@ -841,15 +838,15 @@ static int send_code(struct IR *ir, unsigned int code, unsigned int key)
>  		return ret < 0 ? ret : -EFAULT;
>  	}
>  
> -#ifdef I2C_HW_B_HDPVR
>  	/*
>  	 * The sleep bits aren't necessary on the HD PVR, and in fact, the
>  	 * last i2c_master_recv always fails with a -5, so for now, we're
>  	 * going to skip this whole mess and say we're done on the HD PVR
>  	 */
> -	if (ir->c_rx.adapter->id == I2C_HW_B_HDPVR)
> -		goto done;
> -#endif
> +	if (ir->is_hdpvr) {
> +		dprintk("sent code %u, key %u\n", code, key);
> +		return 0;
> +	}

I don't get the change. What was wrong with the "goto done"? Now you
duplicated the dprintk(), and as far as I can see the "done" label is
left unused.

>  
>  	/*
>  	 * This bit NAKs until the device is ready, so we retry it
> @@ -1111,12 +1108,14 @@ static int ir_remove(struct i2c_client *client);
>  static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id);
>  static int ir_command(struct i2c_client *client, unsigned int cmd, void *arg);
>  
> +#define ID_FLAG_TX	0x01
> +#define ID_FLAG_HDPVR	0x02
> +
>  static const struct i2c_device_id ir_transceiver_id[] = {
> -	/* Generic entry for any IR transceiver */
> -	{ "ir_video", 0 },
> -	/* IR device specific entries should be added here */
> -	{ "ir_tx_z8f0811_haup", 0 },
> -	{ "ir_rx_z8f0811_haup", 0 },
> +	{ "ir_tx_z8f0811_haup",  ID_FLAG_TX                 },
> +	{ "ir_rx_z8f0811_haup",  0                          },
> +	{ "ir_tx_z8f0811_hdpvr", ID_FLAG_HDPVR | ID_FLAG_TX },
> +	{ "ir_rx_z8f0811_hdpvr", ID_FLAG_HDPVR              },
>  	{ }
>  };
>  
> @@ -1196,10 +1195,25 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	int ret;
>  	int have_rx = 0, have_tx = 0;
>  
> -	dprintk("%s: adapter id=0x%x, client addr=0x%02x\n",
> -		__func__, adap->id, client->addr);
> +	dprintk("%s: adapter name (%s) nr %d, i2c_device_id name (%s), "
> +		"client addr=0x%02x\n",
> +		__func__, adap->name, adap->nr, id->name, client->addr);

The debug message format is long and confusing. What about:

	dprintk("%s: %s on i2c-%d (%s), client addr=0x%02x\n",
		__func__, id->name, adap->nr, adap->name, client->addr);

>  
>  	/*
> +	 * FIXME - This probe function probes both the Tx and Rx
> +	 * addresses of the IR microcontroller.
> +	 *
> +	 * However, the I2C subsystem is passing along one I2C client at a
> +	 * time, based on matches to the ir_transceiver_id[] table above.
> +	 * The expectation is that each i2c_client address will be probed
> +	 * individually by drivers so the I2C subsystem can mark all client
> +	 * addresses as claimed or not.
> +	 *
> +	 * This probe routine causes only one of the client addresses, TX or RX,
> +	 * to be claimed.  This will cause a problem if the I2C subsystem is
> +	 * subsequently triggered to probe unclaimed clients again.
> +	 */

This can be easily addressed. You can call i2c_new_dummy() from within
the probe function to register secondary addresses. See
drivers/misc/eeprom/at24.c for an example.

That being said, I doubt this is what we want here. i2c_new_dummy() is
only meant for cases where the board code (or bridge driver in your
case) declares a single I2C device by its main address. Here, you have
declared both the TX and RX (sub-)devices in your bridge driver, so
your I2C device driver's probe() function _will_ be called twice. It
makes no sense to ask for this in the bridge driver and then to look
for a way to work around it in the I2C device driver.

Looking at ir_probe(), it seems rather clear to me that it needs to be
redesigned seriously. This function is still doing old-style device
detection which does not belong there in the standard device driver
binding model. The bridge driver did take care of this already so there
is no point in doing it again.

>From a purely technical perspective, changing client->addr in the
probe() function is totally prohibited.

So we need more changes done to the lirc_zilog driver than your patch
currently does. In particular:
* struct IR should _not_ include private i2c_client structures. i2c
  clients are provided by the i2c-core, either as a parameter to the
  probe() function or as the result of i2c_new_dummy(). Private copies
  are never needed if you use the proper binding model.
* struct IR should probably be split into IR_rx and IR_tx. Having a
  single data structure for both TX and RX doesn't make sense when
  probe() is called once for each.
* ir_probe() should be cleaned up to do only what we expect from a
  probe function, i.e. initialize and bind. No device detection, no
  hard-coded client address. Note that it may make sense to move the
  disable_rx and disable_tx module parameters to the bridge driver, as
  in the standard device driver binding model, disabling must be done
  earlier (you want to prevent the I2C device from being instantiated.)

I see comments in the code about RX and TX interfering and IR.ir_lock
being used to prevent that. I presume this is the reason for the
current driver design. However, I can see that the driver uses
i2c_master_send() followed by i2c_master_send() or i2c_master_recv()
when it should use i2c_transfer() to guarantee that nobody uses the
adapter in-between. Assuming that the Z8 understands the repeated-start
I2C condition, this should hopefully let you get rid of IR.ir_lock and
solve the driver design issue.


> +	/*
>  	 * The external IR receiver is at i2c address 0x71.
>  	 * The IR transmitter is at 0x70.
>  	 */
> @@ -1241,6 +1255,7 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	mutex_init(&ir->ir_lock);
>  	mutex_init(&ir->buf_lock);
>  	ir->need_boot = 1;
> +	ir->is_hdpvr = (id->driver_data & ID_FLAG_HDPVR) ? true : false;
>  
>  	memcpy(&ir->l, &lirc_template, sizeof(struct lirc_driver));
>  	ir->l.minor = -1;


-- 
Jean Delvare

Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field

[Mauro Carvalho Chehab]

Hi Jean,

Thanks for your acks for patches 1 and 2. I've already applied the patches 
on my tree and at linux-next. I'll try to add the acks on it before sending
upstream.

Em 05-01-2011 12:45, Jean Delvare escreveu:
> Hi Andy,
> 
> On Tue, 28 Dec 2010 20:49:50 -0500, Andy Walls wrote:
>> Remove use of deprecated struct i2c_adapter.id field.  In the process,
>> perform different detection of the HD PVR's Z8 IR microcontroller versus
>> the other Hauppauge cards with the Z8 IR microcontroller.
> 
> Thanks a lot for doing this. I'll be very happy when we can finally get
> rid of i2c_adapter.id.
> 
>> Also added a comment about probe() function behavior that needs to be
>> fixed.
> 
> See my suggestion inline below.
> 
>> Signed-off-by: Andy Walls <awalls@md.metrocast.net>
>> ---
>>  drivers/staging/lirc/lirc_zilog.c |   47 ++++++++++++++++++++++++------------
>>  1 files changed, 31 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/staging/lirc/lirc_zilog.c b/drivers/staging/lirc/lirc_zilog.c
>> index 52be6de..ad29bb1 100644
>> --- a/drivers/staging/lirc/lirc_zilog.c
>> +++ b/drivers/staging/lirc/lirc_zilog.c
>> @@ -66,6 +66,7 @@ struct IR {
>>  	/* Device info */
>>  	struct mutex ir_lock;
>>  	int open;
>> +	bool is_hdpvr;
>>  
>>  	/* RX device */
>>  	struct i2c_client c_rx;
>> @@ -206,16 +207,12 @@ static int add_to_buf(struct IR *ir)
>>  		}
>>  
>>  		/* key pressed ? */
>> -#ifdef I2C_HW_B_HDPVR
>> -		if (ir->c_rx.adapter->id == I2C_HW_B_HDPVR) {
>> +		if (ir->is_hdpvr) {
>>  			if (got_data && (keybuf[0] == 0x80))
>>  				return 0;
>>  			else if (got_data && (keybuf[0] == 0x00))
>>  				return -ENODATA;
>>  		} else if ((ir->b[0] & 0x80) == 0)
>> -#else
>> -		if ((ir->b[0] & 0x80) == 0)
>> -#endif
>>  			return got_data ? 0 : -ENODATA;
>>  
>>  		/* look what we have */
>> @@ -841,15 +838,15 @@ static int send_code(struct IR *ir, unsigned int code, unsigned int key)
>>  		return ret < 0 ? ret : -EFAULT;
>>  	}
>>  
>> -#ifdef I2C_HW_B_HDPVR
>>  	/*
>>  	 * The sleep bits aren't necessary on the HD PVR, and in fact, the
>>  	 * last i2c_master_recv always fails with a -5, so for now, we're
>>  	 * going to skip this whole mess and say we're done on the HD PVR
>>  	 */
>> -	if (ir->c_rx.adapter->id == I2C_HW_B_HDPVR)
>> -		goto done;
>> -#endif
>> +	if (ir->is_hdpvr) {
>> +		dprintk("sent code %u, key %u\n", code, key);
>> +		return 0;
>> +	}
> 
> I don't get the change. What was wrong with the "goto done"? Now you
> duplicated the dprintk(), and as far as I can see the "done" label is
> left unused.

You probably missed some other changes here. There's a patch fixing the warning
that happens due to the done: label that it was not used.

While this code is, in practice, not used, as IR support is disabled at hdpvr, 
I don't see much sense on trying to optimize its code. I'd rather prefer
to see some patches re-enabling IR support on hdpvr and fixing the remaining
issues at lirc_zilog, converting it to use RC core.

>>  	/*
>>  	 * This bit NAKs until the device is ready, so we retry it
>> @@ -1111,12 +1108,14 @@ static int ir_remove(struct i2c_client *client);
>>  static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id);
>>  static int ir_command(struct i2c_client *client, unsigned int cmd, void *arg);
>>  
>> +#define ID_FLAG_TX	0x01
>> +#define ID_FLAG_HDPVR	0x02
>> +
>>  static const struct i2c_device_id ir_transceiver_id[] = {
>> -	/* Generic entry for any IR transceiver */
>> -	{ "ir_video", 0 },
>> -	/* IR device specific entries should be added here */
>> -	{ "ir_tx_z8f0811_haup", 0 },
>> -	{ "ir_rx_z8f0811_haup", 0 },
>> +	{ "ir_tx_z8f0811_haup",  ID_FLAG_TX                 },
>> +	{ "ir_rx_z8f0811_haup",  0                          },
>> +	{ "ir_tx_z8f0811_hdpvr", ID_FLAG_HDPVR | ID_FLAG_TX },
>> +	{ "ir_rx_z8f0811_hdpvr", ID_FLAG_HDPVR              },
>>  	{ }
>>  };
>>  
>> @@ -1196,10 +1195,25 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
>>  	int ret;
>>  	int have_rx = 0, have_tx = 0;
>>  
>> -	dprintk("%s: adapter id=0x%x, client addr=0x%02x\n",
>> -		__func__, adap->id, client->addr);
>> +	dprintk("%s: adapter name (%s) nr %d, i2c_device_id name (%s), "
>> +		"client addr=0x%02x\n",
>> +		__func__, adap->name, adap->nr, id->name, client->addr);
> 
> The debug message format is long and confusing. What about:
> 
> 	dprintk("%s: %s on i2c-%d (%s), client addr=0x%02x\n",
> 		__func__, id->name, adap->nr, adap->name, client->addr);

Agreed.
> 
>>  
>>  	/*
>> +	 * FIXME - This probe function probes both the Tx and Rx
>> +	 * addresses of the IR microcontroller.
>> +	 *
>> +	 * However, the I2C subsystem is passing along one I2C client at a
>> +	 * time, based on matches to the ir_transceiver_id[] table above.
>> +	 * The expectation is that each i2c_client address will be probed
>> +	 * individually by drivers so the I2C subsystem can mark all client
>> +	 * addresses as claimed or not.
>> +	 *
>> +	 * This probe routine causes only one of the client addresses, TX or RX,
>> +	 * to be claimed.  This will cause a problem if the I2C subsystem is
>> +	 * subsequently triggered to probe unclaimed clients again.
>> +	 */
> 
> This can be easily addressed. You can call i2c_new_dummy() from within
> the probe function to register secondary addresses. See
> drivers/misc/eeprom/at24.c for an example.
> 
> That being said, I doubt this is what we want here. i2c_new_dummy() is
> only meant for cases where the board code (or bridge driver in your
> case) declares a single I2C device by its main address. Here, you have
> declared both the TX and RX (sub-)devices in your bridge driver, so
> your I2C device driver's probe() function _will_ be called twice. It
> makes no sense to ask for this in the bridge driver and then to look
> for a way to work around it in the I2C device driver.
> 
> Looking at ir_probe(), it seems rather clear to me that it needs to be
> redesigned seriously. This function is still doing old-style device
> detection which does not belong there in the standard device driver
> binding model. The bridge driver did take care of this already so there
> is no point in doing it again.
> From a purely technical perspective, changing client->addr in the
> probe() function is totally prohibited.

Agreed. Btw, there are some other hacks with client->addr abuse on some 
other random places at drivers/media, mostly at the device bridge code, 
used to test if certain devices are present and/or to open some I2C gates 
before doing some init code. People use this approach as it provides a
fast way to do some things. On several cases, the amount of code for
doing such hack is very small, when compared to writing a new I2C driver
just to do some static initialization code. Not sure what would be the 
better approach to fix them.

> So we need more changes done to the lirc_zilog driver than your patch
> currently does. In particular:
> * struct IR should _not_ include private i2c_client structures. i2c
>   clients are provided by the i2c-core, either as a parameter to the
>   probe() function or as the result of i2c_new_dummy(). Private copies
>   are never needed if you use the proper binding model.
> * struct IR should probably be split into IR_rx and IR_tx. Having a
>   single data structure for both TX and RX doesn't make sense when
>   probe() is called once for each.
> * ir_probe() should be cleaned up to do only what we expect from a
>   probe function, i.e. initialize and bind. No device detection, no
>   hard-coded client address. Note that it may make sense to move the
>   disable_rx and disable_tx module parameters to the bridge driver, as
>   in the standard device driver binding model, disabling must be done
>   earlier (you want to prevent the I2C device from being instantiated.)
> 
> I see comments in the code about RX and TX interfering and IR.ir_lock
> being used to prevent that. I presume this is the reason for the
> current driver design. However, I can see that the driver uses
> i2c_master_send() followed by i2c_master_send() or i2c_master_recv()
> when it should use i2c_transfer() to guarantee that nobody uses the
> adapter in-between. Assuming that the Z8 understands the repeated-start
> I2C condition, this should hopefully let you get rid of IR.ir_lock and
> solve the driver design issue.

Yeah, using i2c_transfer() will probably do the trick.

Cheers,
Mauro

Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field

[Jean Delvare]

Hi Mauro,

On Wed, 05 Jan 2011 15:34:28 -0200, Mauro Carvalho Chehab wrote:
> Hi Jean,
> 
> Thanks for your acks for patches 1 and 2. I've already applied the patches 
> on my tree and at linux-next. I'll try to add the acks on it before sending
> upstream.

If you can't, it's fine. I merely wanted to show my support to Andy's
work, I don't care if I'm not counted as a reviewer for these small
patches.

> Em 05-01-2011 12:45, Jean Delvare escreveu:
> > From a purely technical perspective, changing client->addr in the
> > probe() function is totally prohibited.
> 
> Agreed. Btw, there are some other hacks with client->addr abuse on some 
> other random places at drivers/media, mostly at the device bridge code, 
> used to test if certain devices are present and/or to open some I2C gates 
> before doing some init code. People use this approach as it provides a
> fast way to do some things. On several cases, the amount of code for
> doing such hack is very small, when compared to writing a new I2C driver
> just to do some static initialization code. Not sure what would be the 
> better approach to fix them.

Hard to tell without seeing the exact code. Ideally,
i2c_new_dummy() would cover these cases: you don't need to write an
actual driver for the device, it's perfectly OK to use the freshly
instantiated i2c_client from the bridge driver directly. Alternatively,
i2c_smbus_xfer() or i2c_transfer() can be used for one-time data
exchange with any slave on the bus as long as you know what you're
doing (i.e. you know that no i2c_client will ever be instantiated for
this slave.)

If you have specific cases you don't know how to solve, please point me
to them and I'll take a look.

-- 
Jean Delvare

Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field

[Mauro Carvalho Chehab]

Em 05-01-2011 19:51, Jean Delvare escreveu:
> Hi Mauro,
> 
> On Wed, 05 Jan 2011 15:34:28 -0200, Mauro Carvalho Chehab wrote:
>> Hi Jean,
>>
>> Thanks for your acks for patches 1 and 2. I've already applied the patches 
>> on my tree and at linux-next. I'll try to add the acks on it before sending
>> upstream.
> 
> If you can't, it's fine. I merely wanted to show my support to Andy's
> work, I don't care if I'm not counted as a reviewer for these small
> patches.

Ok. So, it is probably better to keep it as-is, to avoid rebasing and having
to wait for a couple days at linux-next before sending the git pull request.

> 
>> Em 05-01-2011 12:45, Jean Delvare escreveu:
>>> From a purely technical perspective, changing client->addr in the
>>> probe() function is totally prohibited.
>>
>> Agreed. Btw, there are some other hacks with client->addr abuse on some 
>> other random places at drivers/media, mostly at the device bridge code, 
>> used to test if certain devices are present and/or to open some I2C gates 
>> before doing some init code. People use this approach as it provides a
>> fast way to do some things. On several cases, the amount of code for
>> doing such hack is very small, when compared to writing a new I2C driver
>> just to do some static initialization code. Not sure what would be the 
>> better approach to fix them.
> 
> Hard to tell without seeing the exact code. Ideally,
> i2c_new_dummy() would cover these cases: you don't need to write an
> actual driver for the device, it's perfectly OK to use the freshly
> instantiated i2c_client from the bridge driver directly. Alternatively,
> i2c_smbus_xfer() or i2c_transfer() can be used for one-time data
> exchange with any slave on the bus as long as you know what you're
> doing (i.e. you know that no i2c_client will ever be instantiated for
> this slave.)
> 
> If you have specific cases you don't know how to solve, please point me
> to them and I'll take a look.

You can take a look at saa7134-cards.c, for example. saa7134_tuner_setup()
has several examples. It starts with this one:

	switch (dev->board) {
	case SAA7134_BOARD_BMK_MPEX_NOTUNER:
	case SAA7134_BOARD_BMK_MPEX_TUNER:
		/* Checks if the device has a tuner at 0x60 addr
		   If the device doesn't have a tuner, TUNER_ABSENT
		   will be used at tuner_type, avoiding loading tuner
		   without needing it
		 */
		dev->i2c_client.addr = 0x60;
		board = (i2c_master_recv(&dev->i2c_client, &buf, 0) < 0)
			? SAA7134_BOARD_BMK_MPEX_NOTUNER
			: SAA7134_BOARD_BMK_MPEX_TUNER;

In this specific case, it is simply a probe for a device at address 0x60, but
there are more complex cases there, with eeprom reads and/or some random init
that happens before actually attaching some driver at the i2c address.
It is known to work, but it sounds like a hack.

Cheers,
Mauro

Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field

[Andy Walls]

On Wed, 2011-01-05 at 20:02 -0200, Mauro Carvalho Chehab wrote:
> Em 05-01-2011 19:51, Jean Delvare escreveu:
> > Hi Mauro,
> > 
> > On Wed, 05 Jan 2011 15:34:28 -0200, Mauro Carvalho Chehab wrote:
> >> Hi Jean,
> >>
> >> Thanks for your acks for patches 1 and 2. I've already applied the patches 
> >> on my tree and at linux-next. I'll try to add the acks on it before sending
> >> upstream.
> > 
> > If you can't, it's fine. I merely wanted to show my support to Andy's
> > work, I don't care if I'm not counted as a reviewer for these small
> > patches.
> 
> Ok. So, it is probably better to keep it as-is, to avoid rebasing and having
> to wait for a couple days at linux-next before sending the git pull request.
> 
> > 
> >> Em 05-01-2011 12:45, Jean Delvare escreveu:
> >>> From a purely technical perspective, changing client->addr in the
> >>> probe() function is totally prohibited.
> >>
> >> Agreed. Btw, there are some other hacks with client->addr abuse on some 
> >> other random places at drivers/media, mostly at the device bridge code, 
> >> used to test if certain devices are present and/or to open some I2C gates 
> >> before doing some init code. People use this approach as it provides a
> >> fast way to do some things. On several cases, the amount of code for
> >> doing such hack is very small, when compared to writing a new I2C driver
> >> just to do some static initialization code. Not sure what would be the 
> >> better approach to fix them.
> > 
> > Hard to tell without seeing the exact code. Ideally,
> > i2c_new_dummy() would cover these cases: you don't need to write an
> > actual driver for the device, it's perfectly OK to use the freshly
> > instantiated i2c_client from the bridge driver directly. Alternatively,
> > i2c_smbus_xfer() or i2c_transfer() can be used for one-time data
> > exchange with any slave on the bus as long as you know what you're
> > doing (i.e. you know that no i2c_client will ever be instantiated for
> > this slave.)
> > 
> > If you have specific cases you don't know how to solve, please point me
> > to them and I'll take a look.
> 
> You can take a look at saa7134-cards.c, for example. saa7134_tuner_setup()
> has several examples. It starts with this one:
> 
> 	switch (dev->board) {
> 	case SAA7134_BOARD_BMK_MPEX_NOTUNER:
> 	case SAA7134_BOARD_BMK_MPEX_TUNER:
> 		/* Checks if the device has a tuner at 0x60 addr
> 		   If the device doesn't have a tuner, TUNER_ABSENT
> 		   will be used at tuner_type, avoiding loading tuner
> 		   without needing it
> 		 */
> 		dev->i2c_client.addr = 0x60;
> 		board = (i2c_master_recv(&dev->i2c_client, &buf, 0) < 0)
> 			? SAA7134_BOARD_BMK_MPEX_NOTUNER
> 			: SAA7134_BOARD_BMK_MPEX_TUNER;
> 
> In this specific case, it is simply a probe for a device at address 0x60, but
> there are more complex cases there, with eeprom reads and/or some random init
> that happens before actually attaching some driver at the i2c address.
> It is known to work, but it sounds like a hack.

The cx18 driver has a function scope i2c_client for reading the EEPROM,
and there's a good reason for it.  We don't want to register the EEPROM
with the I2C system and make it visible to the rest of the system,
including i2c-dev and user-space tools.  To avoid EEPROM corruption,
it's better keep communication with EEPROMs to a very limited scope.

Regards,
Andy

> Cheers,
> Mauro
> --
> 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 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field

[Jean Delvare]

Hi Andy,

On Wed, 05 Jan 2011 20:20:35 -0500, Andy Walls wrote:
> The cx18 driver has a function scope i2c_client for reading the EEPROM,
> and there's a good reason for it.  We don't want to register the EEPROM
> with the I2C system and make it visible to the rest of the system,
> including i2c-dev and user-space tools.  To avoid EEPROM corruption,
> it's better keep communication with EEPROMs to a very limited scope.

Note that it is possible to declare a read-only EEPROM, so that
user-space has no chance to write to it. If you really don't want
user-space to touch it, the best approach is i2c_new_dummy(), because
it will mark the slave address as busy, preventing i2c-dev from
accessing it (unless the user forces access - but then he/she is
obviously on his/her own...)

The i2c_client provided by i2c_new_dummy() can be unregistered at the
end of the function which needs it, or kept for the lifetime of the
driver, as you prefer.

-- 
Jean Delvare

Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field

[Jean Delvare]

On Wed, 05 Jan 2011 20:02:41 -0200, Mauro Carvalho Chehab wrote:
> Em 05-01-2011 19:51, Jean Delvare escreveu:
> > If you have specific cases you don't know how to solve, please point me
> > to them and I'll take a look.
> 
> You can take a look at saa7134-cards.c, for example. saa7134_tuner_setup()
> has several examples. It starts with this one:
> 
> 	switch (dev->board) {
> 	case SAA7134_BOARD_BMK_MPEX_NOTUNER:
> 	case SAA7134_BOARD_BMK_MPEX_TUNER:
> 		/* Checks if the device has a tuner at 0x60 addr
> 		   If the device doesn't have a tuner, TUNER_ABSENT
> 		   will be used at tuner_type, avoiding loading tuner
> 		   without needing it
> 		 */
> 		dev->i2c_client.addr = 0x60;
> 		board = (i2c_master_recv(&dev->i2c_client, &buf, 0) < 0)
> 			? SAA7134_BOARD_BMK_MPEX_NOTUNER
> 			: SAA7134_BOARD_BMK_MPEX_TUNER;
> 
> In this specific case, it is simply a probe for a device at address 0x60, but

This call to i2c_master_recv() could be replaced easily with
i2c_transfer(), which doesn't require an i2c_client.

Alternatively, you could delay the probe until you are ready to
instantiate the tuner device, and use i2c_new_device() when you're sure
it's there, and i2c_new_probed_device() when you aren't. This would be
nicer, but this also requires non-trivial changes.

> there are more complex cases there, with eeprom reads and/or some random init
> that happens before actually attaching some driver at the i2c address.
> It is known to work, but it sounds like a hack.

For eeprom reads, I would definitely recommend getting a clean
i2c_client from i2c-core using i2c_new_dummy() or i2c_new_device().

For "random init", well, I guess each case is different, so I can't
make a general statement.

-- 
Jean Delvare

Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field

[Andy Walls]

On Wed, 2011-01-05 at 15:45 +0100, Jean Delvare wrote:
> Hi Andy,
> 
> On Tue, 28 Dec 2010 20:49:50 -0500, Andy Walls wrote:
> > Remove use of deprecated struct i2c_adapter.id field.  In the process,
> > perform different detection of the HD PVR's Z8 IR microcontroller versus
> > the other Hauppauge cards with the Z8 IR microcontroller.
> 
> Thanks a lot for doing this. I'll be very happy when we can finally get
> rid of i2c_adapter.id.

You're welcome.  If I hadn't done it, I was worried that lirc_zilog
would have been deleted.  I want to fix lirc_zilog properly, but haven't
had the time yet.

Keep in mind, that in this patch I had one objective: remove use of
struct i2c_adapter.id . 

I turned a blind-eye to other obvious problems, since fixing
lirc_zilog's problems looked like it was going to be like peeling an
onion.  Once you peel back one layer of problems, you find another one
underneath. ;)

> > Also added a comment about probe() function behavior that needs to be
> > fixed.
> 
> See my suggestion inline below.
> 
> > Signed-off-by: Andy Walls <awalls@md.metrocast.net>
> > ---
> >  drivers/staging/lirc/lirc_zilog.c |   47 ++++++++++++++++++++++++------------
> >  1 files changed, 31 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/staging/lirc/lirc_zilog.c b/drivers/staging/lirc/lirc_zilog.c
> > index 52be6de..ad29bb1 100644
> > --- a/drivers/staging/lirc/lirc_zilog.c
> > +++ b/drivers/staging/lirc/lirc_zilog.c
> > @@ -66,6 +66,7 @@ struct IR {
> >  	/* Device info */
> >  	struct mutex ir_lock;
> >  	int open;
> > +	bool is_hdpvr;
> >  
> >  	/* RX device */
> >  	struct i2c_client c_rx;
> > @@ -206,16 +207,12 @@ static int add_to_buf(struct IR *ir)
> >  		}
> >  
> >  		/* key pressed ? */
> > -#ifdef I2C_HW_B_HDPVR
> > -		if (ir->c_rx.adapter->id == I2C_HW_B_HDPVR) {
> > +		if (ir->is_hdpvr) {
> >  			if (got_data && (keybuf[0] == 0x80))
> >  				return 0;
> >  			else if (got_data && (keybuf[0] == 0x00))
> >  				return -ENODATA;
> >  		} else if ((ir->b[0] & 0x80) == 0)
> > -#else
> > -		if ((ir->b[0] & 0x80) == 0)
> > -#endif
> >  			return got_data ? 0 : -ENODATA;
> >  
> >  		/* look what we have */
> > @@ -841,15 +838,15 @@ static int send_code(struct IR *ir, unsigned int code, unsigned int key)
> >  		return ret < 0 ? ret : -EFAULT;
> >  	}
> >  
> > -#ifdef I2C_HW_B_HDPVR
> >  	/*
> >  	 * The sleep bits aren't necessary on the HD PVR, and in fact, the
> >  	 * last i2c_master_recv always fails with a -5, so for now, we're
> >  	 * going to skip this whole mess and say we're done on the HD PVR
> >  	 */
> > -	if (ir->c_rx.adapter->id == I2C_HW_B_HDPVR)
> > -		goto done;
> > -#endif
> > +	if (ir->is_hdpvr) {
> > +		dprintk("sent code %u, key %u\n", code, key);
> > +		return 0;
> > +	}
> 
> I don't get the change. What was wrong with the "goto done"? Now you
> duplicated the dprintk(), and as far as I can see the "done" label is
> left unused.

Mauro removed the "done:" label in a commit just prior to this one.
Otherwise, yes, I would have used a "goto done:".

So much needs cleaning up in lirc_zilog, that I didn't agonize over this
particular item.


> >  
> >  	/*
> >  	 * This bit NAKs until the device is ready, so we retry it
> > @@ -1111,12 +1108,14 @@ static int ir_remove(struct i2c_client *client);
> >  static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id);
> >  static int ir_command(struct i2c_client *client, unsigned int cmd, void *arg);
> >  
> > +#define ID_FLAG_TX	0x01
> > +#define ID_FLAG_HDPVR	0x02
> > +
> >  static const struct i2c_device_id ir_transceiver_id[] = {
> > -	/* Generic entry for any IR transceiver */
> > -	{ "ir_video", 0 },
> > -	/* IR device specific entries should be added here */
> > -	{ "ir_tx_z8f0811_haup", 0 },
> > -	{ "ir_rx_z8f0811_haup", 0 },
> > +	{ "ir_tx_z8f0811_haup",  ID_FLAG_TX                 },
> > +	{ "ir_rx_z8f0811_haup",  0                          },
> > +	{ "ir_tx_z8f0811_hdpvr", ID_FLAG_HDPVR | ID_FLAG_TX },
> > +	{ "ir_rx_z8f0811_hdpvr", ID_FLAG_HDPVR              },
> >  	{ }
> >  };
> >  
> > @@ -1196,10 +1195,25 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
> >  	int ret;
> >  	int have_rx = 0, have_tx = 0;
> >  
> > -	dprintk("%s: adapter id=0x%x, client addr=0x%02x\n",
> > -		__func__, adap->id, client->addr);
> > +	dprintk("%s: adapter name (%s) nr %d, i2c_device_id name (%s), "
> > +		"client addr=0x%02x\n",
> > +		__func__, adap->name, adap->nr, id->name, client->addr);
> 
> The debug message format is long and confusing. What about:
> 
> 	dprintk("%s: %s on i2c-%d (%s), client addr=0x%02x\n",
> 		__func__, id->name, adap->nr, adap->name, client->addr);

Ack. Your suggestion seems fine to me.  

> >  
> >  	/*
> > +	 * FIXME - This probe function probes both the Tx and Rx
> > +	 * addresses of the IR microcontroller.
> > +	 *
> > +	 * However, the I2C subsystem is passing along one I2C client at a
> > +	 * time, based on matches to the ir_transceiver_id[] table above.
> > +	 * The expectation is that each i2c_client address will be probed
> > +	 * individually by drivers so the I2C subsystem can mark all client
> > +	 * addresses as claimed or not.
> > +	 *
> > +	 * This probe routine causes only one of the client addresses, TX or RX,
> > +	 * to be claimed.  This will cause a problem if the I2C subsystem is
> > +	 * subsequently triggered to probe unclaimed clients again.
> > +	 */
> 
> This can be easily addressed. You can call i2c_new_dummy() from within
> the probe function to register secondary addresses. See
> drivers/misc/eeprom/at24.c for an example.
> 
> That being said, I doubt this is what we want here. i2c_new_dummy() is
> only meant for cases where the board code (or bridge driver in your
> case) declares a single I2C device by its main address. Here, you have
> declared both the TX and RX (sub-)devices in your bridge driver, so
> your I2C device driver's probe() function _will_ be called twice.

My intent is to ultimately have lirc_zilog handle only Tx, and let
ir-kbd-i2c handle Rx.  But that might not be possible without a mutex
shared between the two modules.  See the bit about -nak- s below...


>  It
> makes no sense to ask for this in the bridge driver and then to look
> for a way to work around it in the I2C device driver.
> 
> Looking at ir_probe(), it seems rather clear to me that it needs to be
> redesigned seriously. This function is still doing old-style device
> detection which does not belong there in the standard device driver
> binding model. The bridge driver did take care of this already so there
> is no point in doing it again.
> 
> From a purely technical perspective, changing client->addr in the
> probe() function is totally prohibited.

Ack. You are "preaching to the choir". ;)

This was out of kernel code that has it's origins in reverse engineering
the Z8 chip's behavior, over 5 years ago:

http://www.blushingpenguin.com/mark/blog/?p=17

It has been updated only a few times since then, but that is why this
code still has old style I2C binding in it.  It's current condition is
also why it is still in staging.

> So we need more changes done to the lirc_zilog driver than your patch
> currently does. In particular:
> * struct IR should _not_ include private i2c_client structures. i2c
>   clients are provided by the i2c-core, either as a parameter to the
>   probe() function or as the result of i2c_new_dummy(). Private copies
>   are never needed if you use the proper binding model.
> * struct IR should probably be split into IR_rx and IR_tx. Having a
>   single data structure for both TX and RX doesn't make sense when
>   probe() is called once for each.
> * ir_probe() should be cleaned up to do only what we expect from a
>   probe function, i.e. initialize and bind. No device detection, no
>   hard-coded client address. Note that it may make sense to move the
>   disable_rx and disable_tx module parameters to the bridge driver, as
>   in the standard device driver binding model, disabling must be done
>   earlier (you want to prevent the I2C device from being instantiated.)
> 
> I see comments in the code about RX and TX interfering and IR.ir_lock
> being used to prevent that. I presume this is the reason for the
> current driver design. However, I can see that the driver uses
> i2c_master_send() followed by i2c_master_send() or i2c_master_recv()
> when it should use i2c_transfer() to guarantee that nobody uses the
> adapter in-between. 

Thank you for the insight and guidance on how to clean things up.  


> Assuming that the Z8 understands the repeated-start
> I2C condition, this should hopefully let you get rid of IR.ir_lock and
> solve the driver design issue.

I'm not sure if it does.

Although some understanding I do have about the chip's program comes
from these captures from, I *assume*, the Windows driver talking to the
chip:

http://www.blushingpenguin.com/mark/lmilk/boot3
http://www.blushingpenguin.com/mark/lmilk/wdev3
http://www.blushingpenguin.com/mark/lmilk/thelot4

The Tx I2C address is 0x70 and the Rx I2C address is 0x71.

The host looks to be filling up part of a "page" in the Z8's memory 4
bytes at a time, starting at offsets 01, 05, 09, etc:

Offset in page--+   +--- Total number of bytes in page for this command
for this group  |   |    (only for first write, offset 0x01 in page)
or bytes        |   |
                vv vv
 29.494mS: 70 W 01 60 00 01 CC   ` . . . 
  2.043mS: 70 W 05 02 00 F3 1D   . . . . 
  2.033mS: 70 W 09 78 06 DE 68   x . . h 
  2.090mS: 70 W 0D A0 66 B8 48   . f . H 
  2.043mS: 70 W 11 B2 11 50 EE   . . P . 
  2.072mS: 70 W 15 AF 8D C3 1F   . . . . 
  2.035mS: 70 W 19 C2 A9 B4 BF   . . . . 
  2.033mS: 70 W 1D CB EC 27 56   . . ' V 
  2.028mS: 70 W 21 54 67 54 BC   T g T . 
  2.020mS: 70 W 25 13 D2 68 3F   . . h ? 
  2.070mS: 70 W 29 7B 1D 6E 81   { . n . 
  2.020mS: 70 W 2D 39 0A 44 40   9 . D @ 
  2.030mS: 70 W 31 14 59 72 4E   . Y r N 
  2.033mS: 70 W 35 51 5F 58 4E   Q _ X N 
  2.030mS: 70 W 39 68 4D 56 5E   h M V ^ 
  2.051mS: 70 W 3D 4B AF 25 1A   K . % . 
  2.062mS: 70 W 41 46 F1 02 94   F . . . 
  2.033mS: 70 W 45 2D E4 17 38   - . . 8 
  2.030mS: 70 W 49 31 9D 57 BC   1 . W . 
  2.028mS: 70 W 4D 5B 26 3E 81   [ & > . 
  2.043mS: 70 W 51 0C D7 3A 14   . . : . 
  2.020mS: 70 W 55 35 50 02 B6   5 P . . 
  2.033mS: 70 W 59 43 6A 2D 0B   C j - . 
  2.075mS: 70 W 5D 2B 11 2F 7A   + . / z 
  2.033mS: 70 W 61 00 00 00   . . . 

 10.874mS: 70 W 00 20          <--- Write some sort of "go" command at offset 00
     
  9.988mS: 70 W 00             <--- Read back some sort of Ack/Status from offset 00
    681uS: 70 r 80 01 03 00   . . . 
                ^^ ^^
 Return code----+   + length of following payload


If you look at more of the dumps, it appears that accesses to I2C
addresses 0x70 and 0x71 can be interleaved, so it looks like the
IR.ir_lock might not be needed.  Although looking further I see this:

  2.035mS: 70 W 61 00 00 00   . . . 
 10.887mS: 70 W 00 40   @ 
 10.012mS: 70 W 00   
    681uS: 70 r A0   
    717uS: 70 W 00 80   . 
 18.808mS: 70 W  -nak-  
  1.393mS: 70 W  -nak-  
  1.393mS: 70 W  -nak-  
  1.396mS: 70 W  -nak-  
  1.393mS: 70 W  -nak-  
  1.393mS: 70 W  -nak- 
[...]
  1.393mS: 70 W  -nak-  
  1.477mS: 71 W  -nak-  
  1.391mS: 71 W  -nak-  
  1.393mS: 71 W  -nak-  
  1.393mS: 71 W  -nak-  
  1.391mS: 71 W  -nak-  
  1.438mS: 71 W 00   
    681uS: 71 r 00 00 00 00 00 00   . . . . . 
 51.079mS: 70 W 00   
    681uS: 70 r 80

Which seems to indicate that actions taken on the Transmit side of the
chip can cause it to go unresponsive for both Tx and Rx.  The "goto
done;" statement that was in lirc_zilog skips the code that deals with
those -nak- for the HD PVR.

Regards,
Andy

> 
> > +	/*
> >  	 * The external IR receiver is at i2c address 0x71.
> >  	 * The IR transmitter is at 0x70.
> >  	 */
> > @@ -1241,6 +1255,7 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
> >  	mutex_init(&ir->ir_lock);
> >  	mutex_init(&ir->buf_lock);
> >  	ir->need_boot = 1;
> > +	ir->is_hdpvr = (id->driver_data & ID_FLAG_HDPVR) ? true : false;
> >  
> >  	memcpy(&ir->l, &lirc_template, sizeof(struct lirc_driver));
> >  	ir->l.minor = -1;
> 
> 

Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field

[Jean Delvare]

On Wed, 05 Jan 2011 19:46:20 -0500, Andy Walls wrote:
> If you look at more of the dumps, it appears that accesses to I2C
> addresses 0x70 and 0x71 can be interleaved, so it looks like the
> IR.ir_lock might not be needed.  Although looking further I see this:
> 
>   2.035mS: 70 W 61 00 00 00   . . . 
>  10.887mS: 70 W 00 40   @ 
>  10.012mS: 70 W 00   
>     681uS: 70 r A0   
>     717uS: 70 W 00 80   . 
>  18.808mS: 70 W  -nak-  
>   1.393mS: 70 W  -nak-  
>   1.393mS: 70 W  -nak-  
>   1.396mS: 70 W  -nak-  
>   1.393mS: 70 W  -nak-  
>   1.393mS: 70 W  -nak- 
> [...]
>   1.393mS: 70 W  -nak-  
>   1.477mS: 71 W  -nak-  
>   1.391mS: 71 W  -nak-  
>   1.393mS: 71 W  -nak-  
>   1.393mS: 71 W  -nak-  
>   1.391mS: 71 W  -nak-  
>   1.438mS: 71 W 00   
>     681uS: 71 r 00 00 00 00 00 00   . . . . . 
>  51.079mS: 70 W 00   
>     681uS: 70 r 80
> 
> Which seems to indicate that actions taken on the Transmit side of the
> chip can cause it to go unresponsive for both Tx and Rx.  The "goto
> done;" statement that was in lirc_zilog skips the code that deals with
> those -nak- for the HD PVR.

My bet is that register at 0x00 is a control register, and writing bit
7 (value 0x80) makes the chip busy enough that it can't process I2C
requests at the same time. The following naks would be until the
chip is operational again.

In fact, the "waiting" code in lirc_zilog.c:send_code() makes a lot of
sense, and I wouldn't skip it: if the device is busy, you don't want to
return immediately, otherwise a subsequent request will fail. The
failure documented for the HD PVR simply suggests that the wait loop
isn't long enough. It is 20 * 50 ms currently, i.e. 1 second total,
maybe this isn't sufficient. Have you ever tried a longer delay?

-- 
Jean Delvare

Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field

[Devin Heitmueller]

On Thu, Jan 13, 2011 at 8:21 AM, Jean Delvare <khali@linux-fr.org> wrote:
> My bet is that register at 0x00 is a control register, and writing bit
> 7 (value 0x80) makes the chip busy enough that it can't process I2C
> requests at the same time. The following naks would be until the
> chip is operational again.

Correct.  Poking bit 7 of 0xE0:00 triggers the "send" for all the
bytes that were previously loaded into the device.  It puts the chip
into a busy state, doing an i2c clock stretch until it is available
again.  During that time it cannot see any i2c traffic, which is why
you are getting NAKs.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field

[Andy Walls]

Devin,

You've seen the clock stretch with your I2C analyzer?

Jean,

How should clock stretches by slaves be handled using i2c-algo-bit?

Regards,
Andy

Devin Heitmueller <dheitmueller@kernellabs.com> wrote:

>On Thu, Jan 13, 2011 at 8:21 AM, Jean Delvare <khali@linux-fr.org> wrote:
>> My bet is that register at 0x00 is a control register, and writing bit
>> 7 (value 0x80) makes the chip busy enough that it can't process I2C
>> requests at the same time. The following naks would be until the
>> chip is operational again.
>
>Correct.  Poking bit 7 of 0xE0:00 triggers the "send" for all the
>bytes that were previously loaded into the device.  It puts the chip
>into a busy state, doing an i2c clock stretch until it is available
>again.  During that time it cannot see any i2c traffic, which is why
>you are getting NAKs.
>
>Devin
>
>-- 
>Devin J. Heitmueller - Kernel Labs
>http://www.kernellabs.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 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field

[Jean Delvare]

On Thu, 13 Jan 2011 11:34:42 -0500, Andy Walls wrote:
> How should clock stretches by slaves be handled using i2c-algo-bit?

It is already handled. But hdpvr-i2c doesn't use i2c-algo-bit. I2C
support is done with USB commands instead. Maybe the hardware
implementation doesn't support clock stretching by slaves. Apparently
it doesn't support repeated start conditions either, so it wouldn't
surprise me.

-- 
Jean Delvare

Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field

[Devin Heitmueller]

On Thu, Jan 13, 2011 at 11:48 AM, Jean Delvare <khali@linux-fr.org> wrote:
> On Thu, 13 Jan 2011 11:34:42 -0500, Andy Walls wrote:
>> How should clock stretches by slaves be handled using i2c-algo-bit?
>
> It is already handled. But hdpvr-i2c doesn't use i2c-algo-bit. I2C
> support is done with USB commands instead. Maybe the hardware
> implementation doesn't support clock stretching by slaves. Apparently
> it doesn't support repeated start conditions either, so it wouldn't
> surprise me.

The hardware implementation does support clock stretching, or else it
wouldn't be working under Windows.  That said, it's possible that the
driver for the i2c master isn't checking the proper bits to detect the
clock stretch.  I haven't personally looked at the code for the i2c
master, so I cannot say one way or the other.

The Zilog is a pretty nasty beast, and for various reasons it is
especially problematic on the HD-PVR due to some issues I cannot
really get into on a public forum.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field

[Jean Delvare]

On Thu, 13 Jan 2011 12:07:34 -0500, Devin Heitmueller wrote:
> On Thu, Jan 13, 2011 at 11:48 AM, Jean Delvare <khali@linux-fr.org> wrote:
> > On Thu, 13 Jan 2011 11:34:42 -0500, Andy Walls wrote:
> >> How should clock stretches by slaves be handled using i2c-algo-bit?
> >
> > It is already handled. But hdpvr-i2c doesn't use i2c-algo-bit. I2C
> > support is done with USB commands instead. Maybe the hardware
> > implementation doesn't support clock stretching by slaves. Apparently
> > it doesn't support repeated start conditions either, so it wouldn't
> > surprise me.
> 
> The hardware implementation does support clock stretching, or else it
> wouldn't be working under Windows.

I think your conclusion is too fast and possibly incorrect. The traces
Andy pointed us too earlier suggest that the windows driver is also
"polling" the Zilog after the send operation to figure out when it is
available again. If the Zilog was stretching the clock and the master
was seeing that, it wouldn't return until the clock is released, so no
polling would be necessary.

So, either the Zilog isn't stretching the clock in the standard way, or
the master doesn't notice.

> That said, it's possible that the
> driver for the i2c master isn't checking the proper bits to detect the
> clock stretch.  I haven't personally looked at the code for the i2c
> master, so I cannot say one way or the other.

If you're talking about hdpvr-i2c, it's sending USB commands, so it
doesn't seem like we have much control over what happens behind the
scene. If there is any way to improve its reliability, that will
require external knowledge.

-- 
Jean Delvare

Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field

[Devin Heitmueller]

On Thu, Jan 13, 2011 at 11:34 AM, Andy Walls <awalls@md.metrocast.net> wrote:
> Devin,
>
> You've seen the clock stretch with your I2C analyzer?

The Beagle I was using when I did the captures doesn't show clock
stretch.  My "Logic" logic analyzer does, but I wasn't using that at
the time.

That said, I can say with some authority that the Zilog does put the
bus into a busy state after certain operations (such as setting the
send bit).

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field

[Andy Walls]

Jean,

Yes, however, I asked because ivtv and cx18 use i2c-algo-bit and also provide Zilog z8 IR I2C clients for lirc_zilog to use.  So if those get clock stretch handling "for free", that's great. 

Regards,
Andy
 

Jean Delvare <khali@linux-fr.org> wrote:

>On Thu, 13 Jan 2011 11:34:42 -0500, Andy Walls wrote:
>> How should clock stretches by slaves be handled using i2c-algo-bit?
>
>It is already handled. But hdpvr-i2c doesn't use i2c-algo-bit. I2C
>support is done with USB commands instead. Maybe the hardware
>implementation doesn't support clock stretching by slaves. Apparently
>it doesn't support repeated start conditions either, so it wouldn't
>surprise me.
>
>-- 
>Jean Delvare
>--
>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