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.
--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
