* [PATCH 1/3] hdpvr: Add I2C and ir-kdb-i2c registration of the Zilog Z8 IR chip [not found] <1293587067.3098.10.camel@localhost> @ 2010-12-29 1:46 ` Andy Walls 2010-12-29 11:08 ` Mauro Carvalho Chehab 2011-01-05 12:50 ` Jean Delvare 2010-12-29 1:47 ` [PATCH 2/3] ir-kbd-i2c: Add HD PVR IR Rx support to ir-kbd-i2c Andy Walls 2010-12-29 1:49 ` [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field Andy Walls 2 siblings, 2 replies; 20+ messages in thread From: Andy Walls @ 2010-12-29 1:46 UTC (permalink / raw) To: linux-media Cc: Jean Delvare, Jarod Wilson, Janne Grunau, Mauro Carvalho Chehab 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] hdpvr: Add I2C and ir-kdb-i2c registration of the Zilog Z8 IR chip 2010-12-29 1:46 ` [PATCH 1/3] hdpvr: Add I2C and ir-kdb-i2c registration of the Zilog Z8 IR chip Andy Walls @ 2010-12-29 11:08 ` Mauro Carvalho Chehab 2010-12-29 13:40 ` Andy Walls 2011-01-05 12:50 ` Jean Delvare 1 sibling, 1 reply; 20+ messages in thread From: Mauro Carvalho Chehab @ 2010-12-29 11:08 UTC (permalink / raw) To: Andy Walls; +Cc: linux-media, Jean Delvare, Jarod Wilson, Janne Grunau 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); ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] hdpvr: Add I2C and ir-kdb-i2c registration of the Zilog Z8 IR chip 2010-12-29 11:08 ` Mauro Carvalho Chehab @ 2010-12-29 13:40 ` Andy Walls 0 siblings, 0 replies; 20+ messages in thread From: Andy Walls @ 2010-12-29 13:40 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: linux-media, Jean Delvare, Jarod Wilson, Janne Grunau, bcjenkins 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); > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] hdpvr: Add I2C and ir-kdb-i2c registration of the Zilog Z8 IR chip 2010-12-29 1:46 ` [PATCH 1/3] hdpvr: Add I2C and ir-kdb-i2c registration of the Zilog Z8 IR chip Andy Walls 2010-12-29 11:08 ` Mauro Carvalho Chehab @ 2011-01-05 12:50 ` Jean Delvare 2011-01-05 22:44 ` Andy Walls 1 sibling, 1 reply; 20+ messages in thread From: Jean Delvare @ 2011-01-05 12:50 UTC (permalink / raw) To: Andy Walls; +Cc: linux-media, Jarod Wilson, Janne Grunau, Mauro Carvalho Chehab 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] hdpvr: Add I2C and ir-kdb-i2c registration of the Zilog Z8 IR chip 2011-01-05 12:50 ` Jean Delvare @ 2011-01-05 22:44 ` Andy Walls 0 siblings, 0 replies; 20+ messages in thread From: Andy Walls @ 2011-01-05 22:44 UTC (permalink / raw) To: Jean Delvare Cc: linux-media, Jarod Wilson, Janne Grunau, Mauro Carvalho Chehab 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> ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 2/3] ir-kbd-i2c: Add HD PVR IR Rx support to ir-kbd-i2c [not found] <1293587067.3098.10.camel@localhost> 2010-12-29 1:46 ` [PATCH 1/3] hdpvr: Add I2C and ir-kdb-i2c registration of the Zilog Z8 IR chip Andy Walls @ 2010-12-29 1:47 ` Andy Walls 2010-12-29 11:12 ` Mauro Carvalho Chehab 2011-01-05 12:53 ` Jean Delvare 2010-12-29 1:49 ` [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field Andy Walls 2 siblings, 2 replies; 20+ messages in thread From: Andy Walls @ 2010-12-29 1:47 UTC (permalink / raw) To: linux-media Cc: Jean Delvare, Jarod Wilson, Janne Grunau, Mauro Carvalho Chehab 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] ir-kbd-i2c: Add HD PVR IR Rx support to ir-kbd-i2c 2010-12-29 1:47 ` [PATCH 2/3] ir-kbd-i2c: Add HD PVR IR Rx support to ir-kbd-i2c Andy Walls @ 2010-12-29 11:12 ` Mauro Carvalho Chehab 2010-12-29 14:14 ` Andy Walls 2011-01-05 12:53 ` Jean Delvare 1 sibling, 1 reply; 20+ messages in thread From: Mauro Carvalho Chehab @ 2010-12-29 11:12 UTC (permalink / raw) To: Andy Walls; +Cc: linux-media, Jean Delvare, Jarod Wilson, Janne Grunau 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 }, > { } > }; > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] ir-kbd-i2c: Add HD PVR IR Rx support to ir-kbd-i2c 2010-12-29 11:12 ` Mauro Carvalho Chehab @ 2010-12-29 14:14 ` Andy Walls 0 siblings, 0 replies; 20+ messages in thread From: Andy Walls @ 2010-12-29 14:14 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: linux-media, Jean Delvare, Jarod Wilson, Janne Grunau 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 }, > > { } > > }; > > > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] ir-kbd-i2c: Add HD PVR IR Rx support to ir-kbd-i2c 2010-12-29 1:47 ` [PATCH 2/3] ir-kbd-i2c: Add HD PVR IR Rx support to ir-kbd-i2c Andy Walls 2010-12-29 11:12 ` Mauro Carvalho Chehab @ 2011-01-05 12:53 ` Jean Delvare 1 sibling, 0 replies; 20+ messages in thread From: Jean Delvare @ 2011-01-05 12:53 UTC (permalink / raw) To: Andy Walls; +Cc: linux-media, Jarod Wilson, Janne Grunau, Mauro Carvalho Chehab 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field [not found] <1293587067.3098.10.camel@localhost> 2010-12-29 1:46 ` [PATCH 1/3] hdpvr: Add I2C and ir-kdb-i2c registration of the Zilog Z8 IR chip Andy Walls 2010-12-29 1:47 ` [PATCH 2/3] ir-kbd-i2c: Add HD PVR IR Rx support to ir-kbd-i2c Andy Walls @ 2010-12-29 1:49 ` Andy Walls 2011-01-05 14:45 ` Jean Delvare 2 siblings, 1 reply; 20+ messages in thread From: Andy Walls @ 2010-12-29 1:49 UTC (permalink / raw) To: linux-media Cc: Jean Delvare, Jarod Wilson, Janne Grunau, Mauro Carvalho Chehab 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field 2010-12-29 1:49 ` [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field Andy Walls @ 2011-01-05 14:45 ` Jean Delvare 2011-01-05 17:34 ` Mauro Carvalho Chehab 2011-01-06 0:46 ` Andy Walls 0 siblings, 2 replies; 20+ messages in thread From: Jean Delvare @ 2011-01-05 14:45 UTC (permalink / raw) To: Andy Walls; +Cc: linux-media, Jarod Wilson, Janne Grunau, Mauro Carvalho Chehab 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field 2011-01-05 14:45 ` Jean Delvare @ 2011-01-05 17:34 ` Mauro Carvalho Chehab 2011-01-05 21:51 ` Jean Delvare 2011-01-06 0:46 ` Andy Walls 1 sibling, 1 reply; 20+ messages in thread From: Mauro Carvalho Chehab @ 2011-01-05 17:34 UTC (permalink / raw) To: Jean Delvare; +Cc: Andy Walls, linux-media, Jarod Wilson, Janne Grunau 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field 2011-01-05 17:34 ` Mauro Carvalho Chehab @ 2011-01-05 21:51 ` Jean Delvare 2011-01-05 22:02 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 20+ messages in thread From: Jean Delvare @ 2011-01-05 21:51 UTC (permalink / raw) To: Mauro Carvalho Chehab; +Cc: Andy Walls, linux-media, Jarod Wilson, Janne Grunau 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field 2011-01-05 21:51 ` Jean Delvare @ 2011-01-05 22:02 ` Mauro Carvalho Chehab 2011-01-06 1:20 ` Andy Walls 2011-01-13 13:31 ` Jean Delvare 0 siblings, 2 replies; 20+ messages in thread From: Mauro Carvalho Chehab @ 2011-01-05 22:02 UTC (permalink / raw) To: Jean Delvare; +Cc: Andy Walls, linux-media, Jarod Wilson, Janne Grunau 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field 2011-01-05 22:02 ` Mauro Carvalho Chehab @ 2011-01-06 1:20 ` Andy Walls 2011-01-13 9:46 ` Jean Delvare 2011-01-13 13:31 ` Jean Delvare 1 sibling, 1 reply; 20+ messages in thread From: Andy Walls @ 2011-01-06 1:20 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Jean Delvare, linux-media, Jarod Wilson, Janne Grunau 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field 2011-01-06 1:20 ` Andy Walls @ 2011-01-13 9:46 ` Jean Delvare 0 siblings, 0 replies; 20+ messages in thread From: Jean Delvare @ 2011-01-13 9:46 UTC (permalink / raw) To: Andy Walls; +Cc: Mauro Carvalho Chehab, linux-media, Jarod Wilson, Janne Grunau 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field 2011-01-05 22:02 ` Mauro Carvalho Chehab 2011-01-06 1:20 ` Andy Walls @ 2011-01-13 13:31 ` Jean Delvare 1 sibling, 0 replies; 20+ messages in thread From: Jean Delvare @ 2011-01-13 13:31 UTC (permalink / raw) To: Mauro Carvalho Chehab; +Cc: Andy Walls, linux-media, Jarod Wilson, Janne Grunau 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field 2011-01-05 14:45 ` Jean Delvare 2011-01-05 17:34 ` Mauro Carvalho Chehab @ 2011-01-06 0:46 ` Andy Walls 2011-01-13 13:21 ` Jean Delvare 1 sibling, 1 reply; 20+ messages in thread From: Andy Walls @ 2011-01-06 0:46 UTC (permalink / raw) To: Jean Delvare Cc: linux-media, Jarod Wilson, Janne Grunau, Mauro Carvalho Chehab 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; > > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field 2011-01-06 0:46 ` Andy Walls @ 2011-01-13 13:21 ` Jean Delvare 2011-01-13 14:12 ` Devin Heitmueller 0 siblings, 1 reply; 20+ messages in thread From: Jean Delvare @ 2011-01-13 13:21 UTC (permalink / raw) To: Andy Walls; +Cc: linux-media, Jarod Wilson, Janne Grunau, Mauro Carvalho Chehab 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field 2011-01-13 13:21 ` Jean Delvare @ 2011-01-13 14:12 ` Devin Heitmueller 0 siblings, 0 replies; 20+ messages in thread From: Devin Heitmueller @ 2011-01-13 14:12 UTC (permalink / raw) To: Jean Delvare Cc: Andy Walls, linux-media, Jarod Wilson, Janne Grunau, Mauro Carvalho Chehab 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2011-01-13 14:12 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1293587067.3098.10.camel@localhost>
2010-12-29 1:46 ` [PATCH 1/3] hdpvr: Add I2C and ir-kdb-i2c registration of the Zilog Z8 IR chip Andy Walls
2010-12-29 11:08 ` Mauro Carvalho Chehab
2010-12-29 13:40 ` Andy Walls
2011-01-05 12:50 ` Jean Delvare
2011-01-05 22:44 ` Andy Walls
2010-12-29 1:47 ` [PATCH 2/3] ir-kbd-i2c: Add HD PVR IR Rx support to ir-kbd-i2c Andy Walls
2010-12-29 11:12 ` Mauro Carvalho Chehab
2010-12-29 14:14 ` Andy Walls
2011-01-05 12:53 ` Jean Delvare
2010-12-29 1:49 ` [PATCH 3/3] lirc_zilog: Remove use of deprecated struct i2c_adapter.id field Andy Walls
2011-01-05 14:45 ` Jean Delvare
2011-01-05 17:34 ` Mauro Carvalho Chehab
2011-01-05 21:51 ` Jean Delvare
2011-01-05 22:02 ` Mauro Carvalho Chehab
2011-01-06 1:20 ` Andy Walls
2011-01-13 9:46 ` Jean Delvare
2011-01-13 13:31 ` Jean Delvare
2011-01-06 0:46 ` Andy Walls
2011-01-13 13:21 ` Jean Delvare
2011-01-13 14:12 ` Devin Heitmueller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).