On Thu, 2011-01-20 at 23:30 -0500, Jarod Wilson wrote: > We have to actually call i2c_new_device() once for each of the rx and tx > addresses. Also improve error-handling and device remove i2c cleanup. > > Signed-off-by: Jarod Wilson <[email protected]>
Reviewed-by: Andy Walls <[email protected]> I do have some comments, but the real show-stoppers are: - '#if defined(..' for the i2c_del_adapter() in the error path - A very unlikely race by using file scope data See below. > --- > drivers/media/video/hdpvr/hdpvr-core.c | 21 +++++++++++++++++---- > drivers/media/video/hdpvr/hdpvr-i2c.c | 28 ++++++++++++++++++++-------- > drivers/media/video/hdpvr/hdpvr.h | 6 +++++- > 3 files changed, 42 insertions(+), 13 deletions(-) > > diff --git a/drivers/media/video/hdpvr/hdpvr-core.c > b/drivers/media/video/hdpvr/hdpvr-core.c > index a6572e5..f617a23 100644 > --- a/drivers/media/video/hdpvr/hdpvr-core.c > +++ b/drivers/media/video/hdpvr/hdpvr-core.c > @@ -381,13 +381,21 @@ static int hdpvr_probe(struct usb_interface *interface, > #if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE) > retval = hdpvr_register_i2c_adapter(dev); > if (retval < 0) { > - v4l2_err(&dev->v4l2_dev, "registering i2c adapter failed\n"); > + v4l2_err(&dev->v4l2_dev, "i2c adapter register failed\n"); > goto error; > } > > - retval = hdpvr_register_i2c_ir(dev); > - if (retval < 0) > - v4l2_err(&dev->v4l2_dev, "registering i2c IR devices failed\n"); > + hdpvr_register_ir_rx_i2c(dev); > + if (!dev->i2c_rx) { > + v4l2_err(&dev->v4l2_dev, "i2c IR RX device register failed\n"); > + goto reg_fail; > + } > + > + hdpvr_register_ir_tx_i2c(dev); > + if (!dev->i2c_tx) { > + v4l2_err(&dev->v4l2_dev, "i2c IR TX device register failed\n"); > + goto reg_fail; > + } Once your driver is debugged and complete, there is really never a need to save pointers to the i2c_clients. You might want to consider having void hdpvr_register_ir_?x_i2c() instead return a pointer that you can check against NULL. > #endif > > /* let the user know what node this device is now attached to */ > @@ -395,6 +403,8 @@ static int hdpvr_probe(struct usb_interface *interface, > video_device_node_name(dev->video_dev)); > return 0; > > +reg_fail: > + i2c_del_adapter(&dev->i2c_adapter); Don't you need an '#if defined(CONFIG_I2C)...' here, in case the symbol does not exist? > error: > if (dev) { > /* Destroy single thread */ > @@ -424,6 +434,9 @@ static void hdpvr_disconnect(struct usb_interface > *interface) > mutex_lock(&dev->io_mutex); > hdpvr_cancel_queue(dev); > mutex_unlock(&dev->io_mutex); > +#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE) > + i2c_del_adapter(&dev->i2c_adapter); > +#endif > video_unregister_device(dev->video_dev); > atomic_dec(&dev_nr); > } > diff --git a/drivers/media/video/hdpvr/hdpvr-i2c.c > b/drivers/media/video/hdpvr/hdpvr-i2c.c > index 89b71fa..e891bb0 100644 > --- a/drivers/media/video/hdpvr/hdpvr-i2c.c > +++ b/drivers/media/video/hdpvr/hdpvr-i2c.c > @@ -31,26 +31,38 @@ > #define Z8F0811_IR_RX_I2C_ADDR 0x71 > > > -static struct i2c_board_info hdpvr_i2c_board_info = { > +static struct i2c_board_info hdpvr_ir_tx_i2c_board_info = { > I2C_BOARD_INFO("ir_tx_z8f0811_hdpvr", Z8F0811_IR_TX_I2C_ADDR), > +}; This does not need to be file scope. (In fact this creates a hard to trigger race in the function below.) It should be non-static function scope, as the I2C subsystem will make a copy of the string and address byte and the platform data pointer when you add the I2C device. See pvrusb2. > +void hdpvr_register_ir_tx_i2c(struct hdpvr_device *dev) > +{ > + struct IR_i2c_init_data *init_data = &dev->ir_i2c_init_data; > + > + init_data->name = "HD-PVR"; > + hdpvr_ir_tx_i2c_board_info.platform_data = init_data; If hdpvr_ir_tx_i2c_board_info is file scope and static, then you have a race for the platform data pointer when two hdpvr's are instantiated simultaneously. It'll be extremely rare, I know, but it is possible. You should use a function scope copy of the board info. The per device instance of the init_data is correct. > + dev->i2c_tx = i2c_new_device(&dev->i2c_adapter, > + &hdpvr_ir_tx_i2c_board_info); > +} Again maybe just have this function 'return i2c_new_device(...);'. The caller can save away the i2c_client, if you want to keep a pointer around. > +static struct i2c_board_info hdpvr_ir_rx_i2c_board_info = { > I2C_BOARD_INFO("ir_rx_z8f0811_hdpvr", Z8F0811_IR_RX_I2C_ADDR), > }; > > -int hdpvr_register_i2c_ir(struct hdpvr_device *dev) > +void hdpvr_register_ir_rx_i2c(struct hdpvr_device *dev) > { > - struct i2c_client *c; > struct IR_i2c_init_data *init_data = &dev->ir_i2c_init_data; > > /* Our default information for ir-kbd-i2c.c to use */ > init_data->ir_codes = RC_MAP_HAUPPAUGE_NEW; > init_data->internal_get_key_func = IR_KBD_GET_KEY_HAUP_XVR; > init_data->type = RC_TYPE_RC5; > - init_data->name = "HD PVR"; > - hdpvr_i2c_board_info.platform_data = init_data; > - > - c = i2c_new_device(&dev->i2c_adapter, &hdpvr_i2c_board_info); > + init_data->name = "HD-PVR"; > + hdpvr_ir_rx_i2c_board_info.platform_data = init_data; > > - return (c == NULL) ? -ENODEV : 0; > + dev->i2c_rx = i2c_new_device(&dev->i2c_adapter, > + &hdpvr_ir_rx_i2c_board_info); > } Same comments as for Rx. > static int hdpvr_i2c_read(struct hdpvr_device *dev, int bus, > diff --git a/drivers/media/video/hdpvr/hdpvr.h > b/drivers/media/video/hdpvr/hdpvr.h > index ee74e3b..41a579e 100644 > --- a/drivers/media/video/hdpvr/hdpvr.h > +++ b/drivers/media/video/hdpvr/hdpvr.h > @@ -108,6 +108,9 @@ struct hdpvr_device { > > /* I2C adapter */ > struct i2c_adapter i2c_adapter; > + /* I2C clients */ > + struct i2c_client *i2c_rx; > + struct i2c_client *i2c_tx; I don't see where you are using these. If you need them, you may want to rename them to i2c_ir_rx and i2c_ir_tx, if hdpvr will ever talk to another device on the I2C bus. > /* I2C lock */ > struct mutex i2c_mutex; > /* I2C message buffer space */ > @@ -313,7 +316,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); > +void hdpvr_register_ir_rx_i2c(struct hdpvr_device *dev); > +void hdpvr_register_ir_tx_i2c(struct hdpvr_device *dev); > > /*========================================================================*/ > /* buffer management */ Regards, Andy -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
