| Message ID | 1247863615.10066.33.camel@palomino.walls.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Douglas Landgraf |
| Headers | show |
Hi Andy, On Fri, 17 Jul 2009 16:46:55 -0400, Andy Walls wrote: > This patch add support to the cx18 driver for setting up the > Z8F0811/Hauppauge IR Tx/Rx chip with the i2c binding model in newer > kernels. > > My concerns/questions: > > 1. When using the new i2c binding model, I'm not saving the returned > pointer from i2c_new_probed_device() and am hence taking no action on > trying to clean up IR i2c devices on module unload. Will the i2c > subsystem clean up this automatically when the adapter is unregistered > on module unload? While this isn't currently documented, yes it will. If this ever changes, I will first let i2c-core emit warnings and still clean up orphan clients. But I suspect the current behavior is easier for everyone and I couldn't see any reason to change it at this point. > 2. When using the new i2c binding model, I'm calling > i2c_new_probed_device() twice: once for Rx (addr 0x71 of the Z8F0811) > and another time for Tx (addr 0x70 of the Z8F0811). Is it a problem to > have two Linux i2c devices for two distinct addresses of the same > underlying hardware device? No, this is not a problem. The fact that this is the same device behind both addresses is an implementation detail. As long as both functions are independent, it should work just fine. > 3. When using the new i2c binding model, I opted not to use ir_video for > the Z8F0811 loaded with microcode from Zilog/Hauppauge. Since I needed > one name for Rx binding and one for Tx binding, I used these names: > > "ir_tx_z8f0811_haup" > "ir_rx_z8f0811_haup" > > [Which is ir_(func)_(part number)_(firmware_oem)]. It made sense to me. > I assume these are the names to which ir-kbd-i2c and lirc_* will have to > bind. Is that correct? Yes, this is correct, and the approach is good. Ideally the "ir_video" type would not exist (or would go away over time) and we would have a separate type name for each IR chip, resulting in much cleaner code. The reason for the current implementation is solely historical. Review: > diff -r d754a2d5a376 linux/drivers/media/video/cx18/cx18-cards.c > --- a/linux/drivers/media/video/cx18/cx18-cards.c Wed Jul 15 07:28:02 2009 -0300 > +++ b/linux/drivers/media/video/cx18/cx18-cards.c Fri Jul 17 16:05:28 2009 -0400 > @@ -56,7 +56,8 @@ > .hw_audio_ctrl = CX18_HW_418_AV, > .hw_muxer = CX18_HW_CS5345, > .hw_all = CX18_HW_TVEEPROM | CX18_HW_418_AV | CX18_HW_TUNER | > - CX18_HW_CS5345 | CX18_HW_DVB | CX18_HW_GPIO_RESET_CTRL, > + CX18_HW_CS5345 | CX18_HW_DVB | CX18_HW_GPIO_RESET_CTRL | > + CX18_HW_Z8F0811_IR_HAUP, > .video_inputs = { > { CX18_CARD_INPUT_VID_TUNER, 0, CX18_AV_COMPOSITE7 }, > { CX18_CARD_INPUT_SVIDEO1, 1, CX18_AV_SVIDEO1 }, > @@ -102,7 +103,8 @@ > .hw_audio_ctrl = CX18_HW_418_AV, > .hw_muxer = CX18_HW_CS5345, > .hw_all = CX18_HW_TVEEPROM | CX18_HW_418_AV | CX18_HW_TUNER | > - CX18_HW_CS5345 | CX18_HW_DVB | CX18_HW_GPIO_RESET_CTRL, > + CX18_HW_CS5345 | CX18_HW_DVB | CX18_HW_GPIO_RESET_CTRL | > + CX18_HW_Z8F0811_IR_HAUP, > .video_inputs = { > { CX18_CARD_INPUT_VID_TUNER, 0, CX18_AV_COMPOSITE7 }, > { CX18_CARD_INPUT_SVIDEO1, 1, CX18_AV_SVIDEO1 }, > diff -r d754a2d5a376 linux/drivers/media/video/cx18/cx18-cards.h > --- a/linux/drivers/media/video/cx18/cx18-cards.h Wed Jul 15 07:28:02 2009 -0300 > +++ b/linux/drivers/media/video/cx18/cx18-cards.h Fri Jul 17 16:05:28 2009 -0400 > @@ -22,13 +22,17 @@ > */ > > /* hardware flags */ > -#define CX18_HW_TUNER (1 << 0) > -#define CX18_HW_TVEEPROM (1 << 1) > -#define CX18_HW_CS5345 (1 << 2) > -#define CX18_HW_DVB (1 << 3) > -#define CX18_HW_418_AV (1 << 4) > -#define CX18_HW_GPIO_MUX (1 << 5) > -#define CX18_HW_GPIO_RESET_CTRL (1 << 6) > +#define CX18_HW_TUNER (1 << 0) > +#define CX18_HW_TVEEPROM (1 << 1) > +#define CX18_HW_CS5345 (1 << 2) > +#define CX18_HW_DVB (1 << 3) > +#define CX18_HW_418_AV (1 << 4) > +#define CX18_HW_GPIO_MUX (1 << 5) > +#define CX18_HW_GPIO_RESET_CTRL (1 << 6) > +#define CX18_HW_Z8F0811_IR_TX_HAUP (1 << 7) > +#define CX18_HW_Z8F0811_IR_RX_HAUP (1 << 8) > +#define CX18_HW_Z8F0811_IR_HAUP (CX18_HW_Z8F0811_IR_RX_HAUP | \ > + CX18_HW_Z8F0811_IR_TX_HAUP) > > /* video inputs */ > #define CX18_CARD_INPUT_VID_TUNER 1 > diff -r d754a2d5a376 linux/drivers/media/video/cx18/cx18-i2c.c > --- a/linux/drivers/media/video/cx18/cx18-i2c.c Wed Jul 15 07:28:02 2009 -0300 > +++ b/linux/drivers/media/video/cx18/cx18-i2c.c Fri Jul 17 16:05:28 2009 -0400 > @@ -40,16 +40,20 @@ > #define GETSDL_BIT 0x0008 > > #define CX18_CS5345_I2C_ADDR 0x4c > +#define CX18_Z8F0811_IR_TX_I2C_ADDR 0x70 > +#define CX18_Z8F0811_IR_RX_I2C_ADDR 0x71 > > /* This array should match the CX18_HW_ defines */ > static const u8 hw_addrs[] = { > - 0, /* CX18_HW_TUNER */ > - 0, /* CX18_HW_TVEEPROM */ > - CX18_CS5345_I2C_ADDR, /* CX18_HW_CS5345 */ > - 0, /* CX18_HW_DVB */ > - 0, /* CX18_HW_418_AV */ > - 0, /* CX18_HW_GPIO_MUX */ > - 0, /* CX18_HW_GPIO_RESET_CTRL */ > + 0, /* CX18_HW_TUNER */ > + 0, /* CX18_HW_TVEEPROM */ > + CX18_CS5345_I2C_ADDR, /* CX18_HW_CS5345 */ > + 0, /* CX18_HW_DVB */ > + 0, /* CX18_HW_418_AV */ > + 0, /* CX18_HW_GPIO_MUX */ > + 0, /* CX18_HW_GPIO_RESET_CTRL */ > + CX18_Z8F0811_IR_TX_I2C_ADDR, /* CX18_HW_Z8F0811_IR_TX_HAUP */ > + CX18_Z8F0811_IR_RX_I2C_ADDR, /* CX18_HW_Z8F0811_IR_RX_HAUP */ > }; > > /* This array should match the CX18_HW_ defines */ > @@ -62,6 +66,8 @@ > 0, /* CX18_HW_418_AV */ > 0, /* CX18_HW_GPIO_MUX */ > 0, /* CX18_HW_GPIO_RESET_CTRL */ > + 0, /* CX18_HW_Z8F0811_IR_TX_HAUP */ > + 0, /* CX18_HW_Z8F0811_IR_RX_HAUP */ > }; > > /* This array should match the CX18_HW_ defines */ > @@ -73,6 +79,8 @@ > NULL, /* CX18_HW_418_AV */ > NULL, /* CX18_HW_GPIO_MUX */ > NULL, /* CX18_HW_GPIO_RESET_CTRL */ > + NULL, /* CX18_HW_Z8F0811_IR_TX_HAUP */ > + NULL, /* CX18_HW_Z8F0811_IR_RX_HAUP */ > }; > > /* This array should match the CX18_HW_ defines */ > @@ -84,8 +92,43 @@ > "cx23418_AV", > "gpio_mux", > "gpio_reset_ctrl", > + "ir_tx_z8f0811_haup", > + "ir_rx_z8f0811_haup", > }; > > +static int cx18_i2c_new_ir(struct i2c_adapter *adap, u32 hw, const char *type, > + u8 addr) > +{ > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 30) > + struct i2c_board_info info; > + struct IR_i2c_init_data ir_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 */ > + memset(&ir_init_data, 0, sizeof(struct IR_i2c_init_data)); > + switch (hw) { > + case CX18_HW_Z8F0811_IR_RX_HAUP: > + ir_init_data.ir_codes = ir_codes_hauppauge_new; > + ir_init_data.get_key = NULL; This is a no-op, as ir_init_data was cleared with memset() right above. > + ir_init_data.internal_get_key_func = IR_KBD_GET_KEY_HAUP_XVR; > + ir_init_data.type = IR_TYPE_RC5; > + ir_init_data.name = "CX23418 Z8F0811 Hauppauge"; > + break; > + default: > + break; > + } > + if (ir_init_data.name) > + info.platform_data = &ir_init_data; Not sure why you don't just put this in the switch to save a test? Even the memset could go there as far as I can see. > + > + return i2c_new_probed_device(adap, &info, addr_list) == NULL ? -1 : 0; > +#else > + return -1; > +#endif > +} > + > int cx18_i2c_register(struct cx18 *cx, unsigned idx) > { > struct v4l2_subdev *sd; > @@ -119,7 +162,10 @@ > if (!hw_addrs[idx]) > return -1; > > - /* It's an I2C device other than an analog tuner */ > + if (hw & CX18_HW_Z8F0811_IR_HAUP) > + return cx18_i2c_new_ir(adap, hw, type, hw_addrs[idx]); For consistency I'd move this up a bit (although I agree it is functionally equivalent.) > + > + /* It's an I2C device other than an analog tuner or IR chip */ > sd = v4l2_i2c_new_subdev(&cx->v4l2_dev, adap, mod, type, hw_addrs[idx]); > if (sd != NULL) > sd->grp_id = hw; > > The rest looks OK.
On Sunday 19 July 2009 09:38:54 Jean Delvare wrote: > > 3. When using the new i2c binding model, I opted not to use ir_video for > > the Z8F0811 loaded with microcode from Zilog/Hauppauge. Since I needed > > one name for Rx binding and one for Tx binding, I used these names: > > > > "ir_tx_z8f0811_haup" > > "ir_rx_z8f0811_haup" > > > > [Which is ir_(func)_(part number)_(firmware_oem)]. It made sense to me. > > I assume these are the names to which ir-kbd-i2c and lirc_* will have to > > bind. Is that correct? > > Yes, this is correct, and the approach is good. Ideally the "ir_video" > type would not exist (or would go away over time) and we would have a > separate type name for each IR chip, resulting in much cleaner code. > The reason for the current implementation is solely historical. Cool. When fixing up lirc_i2c, I actually *did* have a question about that which I forgot about until reading this. The only name I could find in use anywhere at a glance was ir_video, so that's what lirc_i2c is set to hook up to for the moment, but yeah, device-specific names instead would be great. Hrm. Offhand, I don't have a clue what the actual IR chip is on the PVR-x50 series, let alone any of the other cards lirc_i2c claims to support...
On Mon, 2009-07-20 at 14:51 -0400, Jarod Wilson wrote: > On Sunday 19 July 2009 09:38:54 Jean Delvare wrote: > > > 3. When using the new i2c binding model, I opted not to use ir_video for > > > the Z8F0811 loaded with microcode from Zilog/Hauppauge. Since I needed > > > one name for Rx binding and one for Tx binding, I used these names: > > > > > > "ir_tx_z8f0811_haup" > > > "ir_rx_z8f0811_haup" > > > > > > [Which is ir_(func)_(part number)_(firmware_oem)]. It made sense to me. > > > I assume these are the names to which ir-kbd-i2c and lirc_* will have to > > > bind. Is that correct? > > > > Yes, this is correct, and the approach is good. Ideally the "ir_video" > > type would not exist (or would go away over time) and we would have a > > separate type name for each IR chip, resulting in much cleaner code. > > The reason for the current implementation is solely historical. > > Cool. When fixing up lirc_i2c, I actually *did* have a question about > that which I forgot about until reading this. The only name I could > find in use anywhere at a glance was ir_video, so that's what lirc_i2c > is set to hook up to for the moment, but yeah, device-specific names > instead would be great. Yes, I noted "ir_video" implied an IR receiver, but the IR blaster on the HVR-1600 and newer PVR-150's can't be called "ir_video" and have lirc_zilog do the right thing obviously. So "in for a penny, in for a pound..." Based on earlier posts from Jean, note that for microcontrollers the chip part number is not enough to uniquely identify the IR implementation since the firmware can be different. I used the name "haup" to distinguish a Z8F0811 loaded with firmware from Hauppauge/Zilog. Given reports from users using lirc_pvr150, the different versions of firmware loaded into th Z8F0811 chips on the Hauppaugue boards all seem to be compatable to some degree. Plus the IR program firmware can report its exact version if needed. > Hrm. Offhand, I don't have a clue what the > actual IR chip is on the PVR-x50 series, There are a few different IR chips on the PVR-x50 series AFAIK. I know that if you find one sitting at 0x71 on the PVR-x50's, then it's likely a Zilog Z8 Encore! family microcontroller loaded with firmware program that probably originates from Zilog. (A Zilog EULA comes with the Hauppauge Windows drivers.) Actually the Zilog Z8 chips respond to 0x70: blaster, 0x71 receiver, 0x72 ???, 0x73 ??? I was kind of hoping that addresses 0x72 and 0x73 might support some sort of "raw mode" or "learning mode", so I could to avoid the whole lirc_zilog "firmware image" mess. But I haven't had time to expriment. Regards, Andy > let alone any of the other > cards lirc_i2c claims to support... -- 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
On Sun, 2009-07-19 at 15:38 +0200, Jean Delvare wrote: > Hi Andy, Jean, Thanks for tking the time to answer my questions and review. My responses are inline. > On Fri, 17 Jul 2009 16:46:55 -0400, Andy Walls wrote: > > This patch add support to the cx18 driver for setting up the > > Z8F0811/Hauppauge IR Tx/Rx chip with the i2c binding model in newer > > kernels. > > > > My concerns/questions: > > > > 1. When using the new i2c binding model, I'm not saving the returned > > pointer from i2c_new_probed_device() and am hence taking no action on > > trying to clean up IR i2c devices on module unload. Will the i2c > > subsystem clean up this automatically when the adapter is unregistered > > on module unload? > > While this isn't currently documented, yes it will. If this ever > changes, I will first let i2c-core emit warnings and still clean up > orphan clients. But I suspect the current behavior is easier for > everyone and I couldn't see any reason to change it at this point. OK. Sounds good. :) > > 2. When using the new i2c binding model, I'm calling > > i2c_new_probed_device() twice: once for Rx (addr 0x71 of the Z8F0811) > > and another time for Tx (addr 0x70 of the Z8F0811). Is it a problem to > > have two Linux i2c devices for two distinct addresses of the same > > underlying hardware device? > > No, this is not a problem. The fact that this is the same device behind > both addresses is an implementation detail. As long as both functions > are independent, it should work just fine. OK. :) > > 3. When using the new i2c binding model, I opted not to use ir_video for > > the Z8F0811 loaded with microcode from Zilog/Hauppauge. Since I needed > > one name for Rx binding and one for Tx binding, I used these names: > > > > "ir_tx_z8f0811_haup" > > "ir_rx_z8f0811_haup" > > > > [Which is ir_(func)_(part number)_(firmware_oem)]. It made sense to me. > > I assume these are the names to which ir-kbd-i2c and lirc_* will have to > > bind. Is that correct? > > Yes, this is correct, and the approach is good. OK. :) > Ideally the "ir_video" > type would not exist (or would go away over time) and we would have a > separate type name for each IR chip, resulting in much cleaner code. > The reason for the current implementation is solely historical. OK, I had suspected the reason was this. > Review: > > > diff -r d754a2d5a376 linux/drivers/media/video/cx18/cx18-cards.c > > --- a/linux/drivers/media/video/cx18/cx18-cards.c Wed Jul 15 07:28:02 2009 -0300 > > +++ b/linux/drivers/media/video/cx18/cx18-cards.c Fri Jul 17 16:05:28 2009 -0400 > > @@ -56,7 +56,8 @@ > > .hw_audio_ctrl = CX18_HW_418_AV, > > .hw_muxer = CX18_HW_CS5345, > > .hw_all = CX18_HW_TVEEPROM | CX18_HW_418_AV | CX18_HW_TUNER | > > - CX18_HW_CS5345 | CX18_HW_DVB | CX18_HW_GPIO_RESET_CTRL, > > + CX18_HW_CS5345 | CX18_HW_DVB | CX18_HW_GPIO_RESET_CTRL | > > + CX18_HW_Z8F0811_IR_HAUP, > > .video_inputs = { > > { CX18_CARD_INPUT_VID_TUNER, 0, CX18_AV_COMPOSITE7 }, > > { CX18_CARD_INPUT_SVIDEO1, 1, CX18_AV_SVIDEO1 }, > > @@ -102,7 +103,8 @@ > > .hw_audio_ctrl = CX18_HW_418_AV, > > .hw_muxer = CX18_HW_CS5345, > > .hw_all = CX18_HW_TVEEPROM | CX18_HW_418_AV | CX18_HW_TUNER | > > - CX18_HW_CS5345 | CX18_HW_DVB | CX18_HW_GPIO_RESET_CTRL, > > + CX18_HW_CS5345 | CX18_HW_DVB | CX18_HW_GPIO_RESET_CTRL | > > + CX18_HW_Z8F0811_IR_HAUP, > > .video_inputs = { > > { CX18_CARD_INPUT_VID_TUNER, 0, CX18_AV_COMPOSITE7 }, > > { CX18_CARD_INPUT_SVIDEO1, 1, CX18_AV_SVIDEO1 }, > > diff -r d754a2d5a376 linux/drivers/media/video/cx18/cx18-cards.h > > --- a/linux/drivers/media/video/cx18/cx18-cards.h Wed Jul 15 07:28:02 2009 -0300 > > +++ b/linux/drivers/media/video/cx18/cx18-cards.h Fri Jul 17 16:05:28 2009 -0400 > > @@ -22,13 +22,17 @@ > > */ > > > > /* hardware flags */ > > -#define CX18_HW_TUNER (1 << 0) > > -#define CX18_HW_TVEEPROM (1 << 1) > > -#define CX18_HW_CS5345 (1 << 2) > > -#define CX18_HW_DVB (1 << 3) > > -#define CX18_HW_418_AV (1 << 4) > > -#define CX18_HW_GPIO_MUX (1 << 5) > > -#define CX18_HW_GPIO_RESET_CTRL (1 << 6) > > +#define CX18_HW_TUNER (1 << 0) > > +#define CX18_HW_TVEEPROM (1 << 1) > > +#define CX18_HW_CS5345 (1 << 2) > > +#define CX18_HW_DVB (1 << 3) > > +#define CX18_HW_418_AV (1 << 4) > > +#define CX18_HW_GPIO_MUX (1 << 5) > > +#define CX18_HW_GPIO_RESET_CTRL (1 << 6) > > +#define CX18_HW_Z8F0811_IR_TX_HAUP (1 << 7) > > +#define CX18_HW_Z8F0811_IR_RX_HAUP (1 << 8) > > +#define CX18_HW_Z8F0811_IR_HAUP (CX18_HW_Z8F0811_IR_RX_HAUP | \ > > + CX18_HW_Z8F0811_IR_TX_HAUP) > > > > /* video inputs */ > > #define CX18_CARD_INPUT_VID_TUNER 1 > > diff -r d754a2d5a376 linux/drivers/media/video/cx18/cx18-i2c.c > > --- a/linux/drivers/media/video/cx18/cx18-i2c.c Wed Jul 15 07:28:02 2009 -0300 > > +++ b/linux/drivers/media/video/cx18/cx18-i2c.c Fri Jul 17 16:05:28 2009 -0400 > > @@ -40,16 +40,20 @@ > > #define GETSDL_BIT 0x0008 > > > > #define CX18_CS5345_I2C_ADDR 0x4c > > +#define CX18_Z8F0811_IR_TX_I2C_ADDR 0x70 > > +#define CX18_Z8F0811_IR_RX_I2C_ADDR 0x71 > > > > /* This array should match the CX18_HW_ defines */ > > static const u8 hw_addrs[] = { > > - 0, /* CX18_HW_TUNER */ > > - 0, /* CX18_HW_TVEEPROM */ > > - CX18_CS5345_I2C_ADDR, /* CX18_HW_CS5345 */ > > - 0, /* CX18_HW_DVB */ > > - 0, /* CX18_HW_418_AV */ > > - 0, /* CX18_HW_GPIO_MUX */ > > - 0, /* CX18_HW_GPIO_RESET_CTRL */ > > + 0, /* CX18_HW_TUNER */ > > + 0, /* CX18_HW_TVEEPROM */ > > + CX18_CS5345_I2C_ADDR, /* CX18_HW_CS5345 */ > > + 0, /* CX18_HW_DVB */ > > + 0, /* CX18_HW_418_AV */ > > + 0, /* CX18_HW_GPIO_MUX */ > > + 0, /* CX18_HW_GPIO_RESET_CTRL */ > > + CX18_Z8F0811_IR_TX_I2C_ADDR, /* CX18_HW_Z8F0811_IR_TX_HAUP */ > > + CX18_Z8F0811_IR_RX_I2C_ADDR, /* CX18_HW_Z8F0811_IR_RX_HAUP */ > > }; > > > > /* This array should match the CX18_HW_ defines */ > > @@ -62,6 +66,8 @@ > > 0, /* CX18_HW_418_AV */ > > 0, /* CX18_HW_GPIO_MUX */ > > 0, /* CX18_HW_GPIO_RESET_CTRL */ > > + 0, /* CX18_HW_Z8F0811_IR_TX_HAUP */ > > + 0, /* CX18_HW_Z8F0811_IR_RX_HAUP */ > > }; > > > > /* This array should match the CX18_HW_ defines */ > > @@ -73,6 +79,8 @@ > > NULL, /* CX18_HW_418_AV */ > > NULL, /* CX18_HW_GPIO_MUX */ > > NULL, /* CX18_HW_GPIO_RESET_CTRL */ > > + NULL, /* CX18_HW_Z8F0811_IR_TX_HAUP */ > > + NULL, /* CX18_HW_Z8F0811_IR_RX_HAUP */ > > }; > > > > /* This array should match the CX18_HW_ defines */ > > @@ -84,8 +92,43 @@ > > "cx23418_AV", > > "gpio_mux", > > "gpio_reset_ctrl", > > + "ir_tx_z8f0811_haup", > > + "ir_rx_z8f0811_haup", > > }; > > > > +static int cx18_i2c_new_ir(struct i2c_adapter *adap, u32 hw, const char *type, > > + u8 addr) > > +{ > > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 30) > > + struct i2c_board_info info; > > + struct IR_i2c_init_data ir_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 */ > > + memset(&ir_init_data, 0, sizeof(struct IR_i2c_init_data)); > > + switch (hw) { > > + case CX18_HW_Z8F0811_IR_RX_HAUP: > > + ir_init_data.ir_codes = ir_codes_hauppauge_new; > > + ir_init_data.get_key = NULL; > > This is a no-op, as ir_init_data was cleared with memset() right above. Agree. I'll get rid of it. > > > + ir_init_data.internal_get_key_func = IR_KBD_GET_KEY_HAUP_XVR; > > + ir_init_data.type = IR_TYPE_RC5; > > + ir_init_data.name = "CX23418 Z8F0811 Hauppauge"; > > + break; > > + default: > > + break; > > + } > > + if (ir_init_data.name) > > + info.platform_data = &ir_init_data; > > Not sure why you don't just put this in the switch to save a test? Even > the memset could go there as far as I can see. If this were the only I2C IR Rx device I had plans for in the cx18 driver, I would have used an if() statement for the whole thing instead of a switch(). However, the LeadTek boards have an IR device that I plan to support someday, so I left the common elements split out. The biggest penalty here is probably the memset() for the IR Tx portion of the Z8F0811 chip that doesn't use the ir_init_data, so I'll make the change as you suggest. > > + > > + return i2c_new_probed_device(adap, &info, addr_list) == NULL ? -1 : 0; > > +#else > > + return -1; > > +#endif > > +} > > + > > int cx18_i2c_register(struct cx18 *cx, unsigned idx) > > { > > struct v4l2_subdev *sd; > > @@ -119,7 +162,10 @@ > > if (!hw_addrs[idx]) > > return -1; > > > > - /* It's an I2C device other than an analog tuner */ > > + if (hw & CX18_HW_Z8F0811_IR_HAUP) > > + return cx18_i2c_new_ir(adap, hw, type, hw_addrs[idx]); > > For consistency I'd move this up a bit (although I agree it is > functionally equivalent.) No problem. > > + > > + /* It's an I2C device other than an analog tuner or IR chip */ > > sd = v4l2_i2c_new_subdev(&cx->v4l2_dev, adap, mod, type, hw_addrs[idx]); > > if (sd != NULL) > > sd->grp_id = hw; > > > > > > The rest looks OK. OK. Thanks. -Andy -- 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
diff -r d754a2d5a376 linux/drivers/media/video/cx18/cx18-cards.c --- a/linux/drivers/media/video/cx18/cx18-cards.c Wed Jul 15 07:28:02 2009 -0300 +++ b/linux/drivers/media/video/cx18/cx18-cards.c Fri Jul 17 16:05:28 2009 -0400 @@ -56,7 +56,8 @@ .hw_audio_ctrl = CX18_HW_418_AV, .hw_muxer = CX18_HW_CS5345, .hw_all = CX18_HW_TVEEPROM | CX18_HW_418_AV | CX18_HW_TUNER | - CX18_HW_CS5345 | CX18_HW_DVB | CX18_HW_GPIO_RESET_CTRL, + CX18_HW_CS5345 | CX18_HW_DVB | CX18_HW_GPIO_RESET_CTRL | + CX18_HW_Z8F0811_IR_HAUP, .video_inputs = { { CX18_CARD_INPUT_VID_TUNER, 0, CX18_AV_COMPOSITE7 }, { CX18_CARD_INPUT_SVIDEO1, 1, CX18_AV_SVIDEO1 }, @@ -102,7 +103,8 @@ .hw_audio_ctrl = CX18_HW_418_AV, .hw_muxer = CX18_HW_CS5345, .hw_all = CX18_HW_TVEEPROM | CX18_HW_418_AV | CX18_HW_TUNER | - CX18_HW_CS5345 | CX18_HW_DVB | CX18_HW_GPIO_RESET_CTRL, + CX18_HW_CS5345 | CX18_HW_DVB | CX18_HW_GPIO_RESET_CTRL | + CX18_HW_Z8F0811_IR_HAUP, .video_inputs = { { CX18_CARD_INPUT_VID_TUNER, 0, CX18_AV_COMPOSITE7 }, { CX18_CARD_INPUT_SVIDEO1, 1, CX18_AV_SVIDEO1 }, diff -r d754a2d5a376 linux/drivers/media/video/cx18/cx18-cards.h --- a/linux/drivers/media/video/cx18/cx18-cards.h Wed Jul 15 07:28:02 2009 -0300 +++ b/linux/drivers/media/video/cx18/cx18-cards.h Fri Jul 17 16:05:28 2009 -0400 @@ -22,13 +22,17 @@ */ /* hardware flags */ -#define CX18_HW_TUNER (1 << 0) -#define CX18_HW_TVEEPROM (1 << 1) -#define CX18_HW_CS5345 (1 << 2) -#define CX18_HW_DVB (1 << 3) -#define CX18_HW_418_AV (1 << 4) -#define CX18_HW_GPIO_MUX (1 << 5) -#define CX18_HW_GPIO_RESET_CTRL (1 << 6) +#define CX18_HW_TUNER (1 << 0) +#define CX18_HW_TVEEPROM (1 << 1) +#define CX18_HW_CS5345 (1 << 2) +#define CX18_HW_DVB (1 << 3) +#define CX18_HW_418_AV (1 << 4) +#define CX18_HW_GPIO_MUX (1 << 5) +#define CX18_HW_GPIO_RESET_CTRL (1 << 6) +#define CX18_HW_Z8F0811_IR_TX_HAUP (1 << 7) +#define CX18_HW_Z8F0811_IR_RX_HAUP (1 << 8) +#define CX18_HW_Z8F0811_IR_HAUP (CX18_HW_Z8F0811_IR_RX_HAUP | \ + CX18_HW_Z8F0811_IR_TX_HAUP) /* video inputs */ #define CX18_CARD_INPUT_VID_TUNER 1 diff -r d754a2d5a376 linux/drivers/media/video/cx18/cx18-i2c.c --- a/linux/drivers/media/video/cx18/cx18-i2c.c Wed Jul 15 07:28:02 2009 -0300 +++ b/linux/drivers/media/video/cx18/cx18-i2c.c Fri Jul 17 16:05:28 2009 -0400 @@ -40,16 +40,20 @@ #define GETSDL_BIT 0x0008 #define CX18_CS5345_I2C_ADDR 0x4c +#define CX18_Z8F0811_IR_TX_I2C_ADDR 0x70 +#define CX18_Z8F0811_IR_RX_I2C_ADDR 0x71 /* This array should match the CX18_HW_ defines */ static const u8 hw_addrs[] = { - 0, /* CX18_HW_TUNER */ - 0, /* CX18_HW_TVEEPROM */ - CX18_CS5345_I2C_ADDR, /* CX18_HW_CS5345 */ - 0, /* CX18_HW_DVB */ - 0, /* CX18_HW_418_AV */ - 0, /* CX18_HW_GPIO_MUX */ - 0, /* CX18_HW_GPIO_RESET_CTRL */ + 0, /* CX18_HW_TUNER */ + 0, /* CX18_HW_TVEEPROM */ + CX18_CS5345_I2C_ADDR, /* CX18_HW_CS5345 */ + 0, /* CX18_HW_DVB */ + 0, /* CX18_HW_418_AV */ + 0, /* CX18_HW_GPIO_MUX */ + 0, /* CX18_HW_GPIO_RESET_CTRL */ + CX18_Z8F0811_IR_TX_I2C_ADDR, /* CX18_HW_Z8F0811_IR_TX_HAUP */ + CX18_Z8F0811_IR_RX_I2C_ADDR, /* CX18_HW_Z8F0811_IR_RX_HAUP */ }; /* This array should match the CX18_HW_ defines */ @@ -62,6 +66,8 @@ 0, /* CX18_HW_418_AV */ 0, /* CX18_HW_GPIO_MUX */ 0, /* CX18_HW_GPIO_RESET_CTRL */ + 0, /* CX18_HW_Z8F0811_IR_TX_HAUP */ + 0, /* CX18_HW_Z8F0811_IR_RX_HAUP */ }; /* This array should match the CX18_HW_ defines */ @@ -73,6 +79,8 @@ NULL, /* CX18_HW_418_AV */ NULL, /* CX18_HW_GPIO_MUX */ NULL, /* CX18_HW_GPIO_RESET_CTRL */ + NULL, /* CX18_HW_Z8F0811_IR_TX_HAUP */ + NULL, /* CX18_HW_Z8F0811_IR_RX_HAUP */ }; /* This array should match the CX18_HW_ defines */ @@ -84,8 +92,43 @@ "cx23418_AV", "gpio_mux", "gpio_reset_ctrl", + "ir_tx_z8f0811_haup", + "ir_rx_z8f0811_haup", }; +static int cx18_i2c_new_ir(struct i2c_adapter *adap, u32 hw, const char *type, + u8 addr) +{ +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 30) + struct i2c_board_info info; + struct IR_i2c_init_data ir_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 */ + memset(&ir_init_data, 0, sizeof(struct IR_i2c_init_data)); + switch (hw) { + case CX18_HW_Z8F0811_IR_RX_HAUP: + ir_init_data.ir_codes = ir_codes_hauppauge_new; + ir_init_data.get_key = NULL; + ir_init_data.internal_get_key_func = IR_KBD_GET_KEY_HAUP_XVR; + ir_init_data.type = IR_TYPE_RC5; + ir_init_data.name = "CX23418 Z8F0811 Hauppauge"; + break; + default: + break; + } + if (ir_init_data.name) + info.platform_data = &ir_init_data; + + return i2c_new_probed_device(adap, &info, addr_list) == NULL ? -1 : 0; +#else + return -1; +#endif +} + int cx18_i2c_register(struct cx18 *cx, unsigned idx) { struct v4l2_subdev *sd; @@ -119,7 +162,10 @@ if (!hw_addrs[idx]) return -1; - /* It's an I2C device other than an analog tuner */ + if (hw & CX18_HW_Z8F0811_IR_HAUP) + return cx18_i2c_new_ir(adap, hw, type, hw_addrs[idx]); + + /* It's an I2C device other than an analog tuner or IR chip */ sd = v4l2_i2c_new_subdev(&cx->v4l2_dev, adap, mod, type, hw_addrs[idx]); if (sd != NULL) sd->grp_id = hw;