Am 05.01.2014 15:40, schrieb Mauro Carvalho Chehab:
> Em Sun, 05 Jan 2014 11:26:14 +0100
> Frank Schäfer <fschaefer.oss@xxxxxxxxxxxxxx> escreveu:
>
>> Am 04.01.2014 11:55, schrieb Mauro Carvalho Chehab:
>>> There are several init code inside em28xx-cards that are actually
>>> part of analog initialization. Move the code to em28x-video, in
>>> order to remove part of the mess.
>>>
>>> In thesis, no functional changes so far.
>>>
>>> Signed-off-by: Mauro Carvalho Chehab <m.chehab@xxxxxxxxxxx>
>>> ---
>>> drivers/media/usb/em28xx/em28xx-cards.c | 71 -------------------------
>>> drivers/media/usb/em28xx/em28xx-video.c | 91 ++++++++++++++++++++++++++++++---
>>> 2 files changed, 84 insertions(+), 78 deletions(-)
>>>
>>> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
>>> index 551cbc294190..175cd776e0a1 100644
>>> --- a/drivers/media/usb/em28xx/em28xx-cards.c
>>> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
>>> @@ -2907,7 +2907,6 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
>>> struct usb_interface *interface,
>>> int minor)
>>> {
>>> - struct v4l2_ctrl_handler *hdl = &dev->ctrl_handler;
>>> int retval;
>>> static const char *default_chip_name = "em28xx";
>>> const char *chip_name = default_chip_name;
>>> @@ -3034,15 +3033,6 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
>>> }
>>> }
>>>
>>> - retval = v4l2_device_register(&interface->dev, &dev->v4l2_dev);
>>> - if (retval < 0) {
>>> - em28xx_errdev("Call to v4l2_device_register() failed!\n");
>>> - return retval;
>>> - }
>>> -
>>> - v4l2_ctrl_handler_init(hdl, 8);
>>> - dev->v4l2_dev.ctrl_handler = hdl;
>>> -
>>> rt_mutex_init(&dev->i2c_bus_lock);
>>>
>>> /* register i2c bus 0 */
>>> @@ -3071,72 +3061,14 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
>>> }
>>> }
>>>
>>> - /*
>>> - * Default format, used for tvp5150 or saa711x output formats
>>> - */
>>> - dev->vinmode = 0x10;
>>> - dev->vinctl = EM28XX_VINCTRL_INTERLACED |
>>> - EM28XX_VINCTRL_CCIR656_ENABLE;
>>> -
>>> /* Do board specific init and eeprom reading */
>>> em28xx_card_setup(dev);
>>>
>> em28xx_card_setup() initializes some v4l2 subdevices, but now the v4l2
>> device (dev->v4l2_dev) isn't ready at this point, because
>> v4l2_device_register() isn't called yet.
>> This introduces oopses.
>> You are fixing this with patch 5/22 later, but patches should never
>> introduce any oopses.
>> The simplest soultion is to move patch 5/22 before this patch.
> After thinking for a while, the better to just fold patch 5 into patch 3,
> and do the necessary changes at the error handling logic.
>
> This makes it simpler to review and test.
>
> New patch enclosed.
>
> Btw, I tested here with HVR-950, without any noticeable changes.
>
> Cheers,
> Mauro
>
> -
>
> [PATCH] [media] em28xx: move analog-specific init to em28xx-video
>
> There are several init code inside em28xx-cards that are actually
> part of analog initialization. Move the code to em28x-video, in
> order to remove part of the mess.
>
> In thesis, no functional changes so far.
>
> Signed-off-by: Mauro Carvalho Chehab <m.chehab@xxxxxxxxxxx>
>
> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
> index 154e6f028fd2..541de6df127b 100644
> --- a/drivers/media/usb/em28xx/em28xx-cards.c
> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> @@ -2360,24 +2360,6 @@ static struct em28xx_hash_table em28xx_i2c_hash[] = {
> };
> /* NOTE: introduce a separate hash table for devices with 16 bit eeproms */
>
> -/* I2C possible address to saa7115, tvp5150, msp3400, tvaudio */
> -static unsigned short saa711x_addrs[] = {
> - 0x4a >> 1, 0x48 >> 1, /* SAA7111, SAA7111A and SAA7113 */
> - 0x42 >> 1, 0x40 >> 1, /* SAA7114, SAA7115 and SAA7118 */
> - I2C_CLIENT_END };
> -
> -static unsigned short tvp5150_addrs[] = {
> - 0xb8 >> 1,
> - 0xba >> 1,
> - I2C_CLIENT_END
> -};
> -
> -static unsigned short msp3400_addrs[] = {
> - 0x80 >> 1,
> - 0x88 >> 1,
> - I2C_CLIENT_END
> -};
> -
> int em28xx_tuner_callback(void *ptr, int component, int command, int arg)
> {
> struct em28xx_i2c_bus *i2c_bus = ptr;
> @@ -2782,58 +2764,8 @@ static void em28xx_card_setup(struct em28xx *dev)
> /* Allow override tuner type by a module parameter */
> if (tuner >= 0)
> dev->tuner_type = tuner;
> -
> - /* request some modules */
> - if (dev->board.has_msp34xx)
> - v4l2_i2c_new_subdev(&dev->v4l2_dev, &dev->i2c_adap[dev->def_i2c_bus],
> - "msp3400", 0, msp3400_addrs);
> -
> - if (dev->board.decoder == EM28XX_SAA711X)
> - v4l2_i2c_new_subdev(&dev->v4l2_dev, &dev->i2c_adap[dev->def_i2c_bus],
> - "saa7115_auto", 0, saa711x_addrs);
> -
> - if (dev->board.decoder == EM28XX_TVP5150)
> - v4l2_i2c_new_subdev(&dev->v4l2_dev, &dev->i2c_adap[dev->def_i2c_bus],
> - "tvp5150", 0, tvp5150_addrs);
> -
> - if (dev->board.adecoder == EM28XX_TVAUDIO)
> - v4l2_i2c_new_subdev(&dev->v4l2_dev, &dev->i2c_adap[dev->def_i2c_bus],
> - "tvaudio", dev->board.tvaudio_addr, NULL);
> -
> - if (dev->board.tuner_type != TUNER_ABSENT) {
> - int has_demod = (dev->tda9887_conf & TDA9887_PRESENT);
> -
> - if (dev->board.radio.type)
> - v4l2_i2c_new_subdev(&dev->v4l2_dev, &dev->i2c_adap[dev->def_i2c_bus],
> - "tuner", dev->board.radio_addr, NULL);
> -
> - if (has_demod)
> - v4l2_i2c_new_subdev(&dev->v4l2_dev,
> - &dev->i2c_adap[dev->def_i2c_bus], "tuner",
> - 0, v4l2_i2c_tuner_addrs(ADDRS_DEMOD));
> - if (dev->tuner_addr == 0) {
> - enum v4l2_i2c_tuner_type type =
> - has_demod ? ADDRS_TV_WITH_DEMOD : ADDRS_TV;
> - struct v4l2_subdev *sd;
> -
> - sd = v4l2_i2c_new_subdev(&dev->v4l2_dev,
> - &dev->i2c_adap[dev->def_i2c_bus], "tuner",
> - 0, v4l2_i2c_tuner_addrs(type));
> -
> - if (sd)
> - dev->tuner_addr = v4l2_i2c_subdev_addr(sd);
> - } else {
> - v4l2_i2c_new_subdev(&dev->v4l2_dev, &dev->i2c_adap[dev->def_i2c_bus],
> - "tuner", dev->tuner_addr, NULL);
> - }
> - }
> -
> - em28xx_tuner_setup(dev);
> -
> - em28xx_init_camera(dev);
> }
>
> -
> static void request_module_async(struct work_struct *work)
> {
> struct em28xx *dev = container_of(work,
> @@ -2907,7 +2839,6 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
> struct usb_interface *interface,
> int minor)
> {
> - struct v4l2_ctrl_handler *hdl = &dev->ctrl_handler;
> int retval;
> static const char *default_chip_name = "em28xx";
> const char *chip_name = default_chip_name;
> @@ -3034,15 +2965,6 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
> }
> }
>
> - retval = v4l2_device_register(&interface->dev, &dev->v4l2_dev);
> - if (retval < 0) {
> - em28xx_errdev("Call to v4l2_device_register() failed!\n");
> - return retval;
> - }
> -
> - v4l2_ctrl_handler_init(hdl, 8);
> - dev->v4l2_dev.ctrl_handler = hdl;
> -
> rt_mutex_init(&dev->i2c_bus_lock);
>
> /* register i2c bus 0 */
> @@ -3053,7 +2975,7 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
> if (retval < 0) {
> em28xx_errdev("%s: em28xx_i2c_register bus 0 - error [%d]!\n",
> __func__, retval);
> - goto unregister_dev;
> + return retval;
> }
>
> /* register i2c bus 1 */
> @@ -3067,75 +2989,16 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
> if (retval < 0) {
> em28xx_errdev("%s: em28xx_i2c_register bus 1 - error [%d]!\n",
> __func__, retval);
> - goto unregister_dev;
> + return retval;
> }
> }
>
> - /*
> - * Default format, used for tvp5150 or saa711x output formats
> - */
> - dev->vinmode = 0x10;
> - dev->vinctl = EM28XX_VINCTRL_INTERLACED |
> - EM28XX_VINCTRL_CCIR656_ENABLE;
> -
> /* Do board specific init and eeprom reading */
> em28xx_card_setup(dev);
>
> - /* Configure audio */
> - retval = em28xx_audio_setup(dev);
> - if (retval < 0) {
> - em28xx_errdev("%s: Error while setting audio - error [%d]!\n",
> - __func__, retval);
> - goto fail;
> - }
> - if (dev->audio_mode.ac97 != EM28XX_NO_AC97) {
> - v4l2_ctrl_new_std(hdl, &em28xx_ctrl_ops,
> - V4L2_CID_AUDIO_MUTE, 0, 1, 1, 1);
> - v4l2_ctrl_new_std(hdl, &em28xx_ctrl_ops,
> - V4L2_CID_AUDIO_VOLUME, 0, 0x1f, 1, 0x1f);
> - } else {
> - /* install the em28xx notify callback */
> - v4l2_ctrl_notify(v4l2_ctrl_find(hdl, V4L2_CID_AUDIO_MUTE),
> - em28xx_ctrl_notify, dev);
> - v4l2_ctrl_notify(v4l2_ctrl_find(hdl, V4L2_CID_AUDIO_VOLUME),
> - em28xx_ctrl_notify, dev);
> - }
> -
> - /* wake i2c devices */
> - em28xx_wake_i2c(dev);
> -
> - /* init video dma queues */
> - INIT_LIST_HEAD(&dev->vidq.active);
> - INIT_LIST_HEAD(&dev->vbiq.active);
> -
> - if (dev->board.has_msp34xx) {
> - /* Send a reset to other chips via gpio */
> - retval = em28xx_write_reg(dev, EM2820_R08_GPIO_CTRL, 0xf7);
> - if (retval < 0) {
> - em28xx_errdev("%s: em28xx_write_reg - "
> - "msp34xx(1) failed! error [%d]\n",
> - __func__, retval);
> - goto fail;
> - }
> - msleep(3);
> -
> - retval = em28xx_write_reg(dev, EM2820_R08_GPIO_CTRL, 0xff);
> - if (retval < 0) {
> - em28xx_errdev("%s: em28xx_write_reg - "
> - "msp34xx(2) failed! error [%d]\n",
> - __func__, retval);
> - goto fail;
> - }
> - msleep(3);
> - }
> -
> retval = em28xx_register_analog_devices(dev);
> - if (retval < 0) {
> + if (retval < 0)
> goto fail;
> - }
> -
> - /* Save some power by putting tuner to sleep */
> - v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 0);
>
> return 0;
>
> @@ -3143,10 +3006,6 @@ fail:
> if (dev->def_i2c_bus)
> em28xx_i2c_unregister(dev, 1);
> em28xx_i2c_unregister(dev, 0);
> - v4l2_ctrl_handler_free(&dev->ctrl_handler);
> -
> -unregister_dev:
> - v4l2_device_unregister(&dev->v4l2_dev);
>
> return retval;
> }
> @@ -3388,9 +3247,6 @@ static int em28xx_usb_probe(struct usb_interface *interface,
> /* save our data pointer in this interface device */
> usb_set_intfdata(interface, dev);
>
> - /* initialize videobuf2 stuff */
> - em28xx_vb2_setup(dev);
> -
> /* allocate device struct */
> mutex_init(&dev->lock);
> mutex_lock(&dev->lock);
> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> index b3ede856c32e..3726af134f39 100644
> --- a/drivers/media/usb/em28xx/em28xx-video.c
> +++ b/drivers/media/usb/em28xx/em28xx-video.c
> @@ -2045,6 +2045,24 @@ static struct video_device em28xx_radio_template = {
> .ioctl_ops = &radio_ioctl_ops,
> };
>
> +/* I2C possible address to saa7115, tvp5150, msp3400, tvaudio */
> +static unsigned short saa711x_addrs[] = {
> + 0x4a >> 1, 0x48 >> 1, /* SAA7111, SAA7111A and SAA7113 */
> + 0x42 >> 1, 0x40 >> 1, /* SAA7114, SAA7115 and SAA7118 */
> + I2C_CLIENT_END };
> +
> +static unsigned short tvp5150_addrs[] = {
> + 0xb8 >> 1,
> + 0xba >> 1,
> + I2C_CLIENT_END
> +};
> +
> +static unsigned short msp3400_addrs[] = {
> + 0x80 >> 1,
> + 0x88 >> 1,
> + I2C_CLIENT_END
> +};
> +
> /******************************** usb interface ******************************/
>
>
> @@ -2186,10 +2204,129 @@ int em28xx_register_analog_devices(struct em28xx *dev)
> u8 val;
> int ret;
> unsigned int maxw;
> + struct v4l2_ctrl_handler *hdl = &dev->ctrl_handler;
> +
> + if (dev->is_audio_only) {
> + /* This device does not support the v4l2 extension */
> + return 0;
> + }
>
> printk(KERN_INFO "%s: v4l2 driver version %s\n",
> dev->name, EM28XX_VERSION);
>
> + ret = v4l2_device_register(&dev->udev->dev, &dev->v4l2_dev);
> + if (ret < 0) {
> + em28xx_errdev("Call to v4l2_device_register() failed!\n");
> + goto err;
> + }
> +
> + v4l2_ctrl_handler_init(hdl, 8);
> + dev->v4l2_dev.ctrl_handler = hdl;
> +
> + /*
> + * Default format, used for tvp5150 or saa711x output formats
> + */
> + dev->vinmode = 0x10;
> + dev->vinctl = EM28XX_VINCTRL_INTERLACED |
> + EM28XX_VINCTRL_CCIR656_ENABLE;
> +
> + /* request some modules */
> +
> + if (dev->board.has_msp34xx)
> + v4l2_i2c_new_subdev(&dev->v4l2_dev, &dev->i2c_adap[dev->def_i2c_bus],
> + "msp3400", 0, msp3400_addrs);
> +
> + if (dev->board.decoder == EM28XX_SAA711X)
> + v4l2_i2c_new_subdev(&dev->v4l2_dev, &dev->i2c_adap[dev->def_i2c_bus],
> + "saa7115_auto", 0, saa711x_addrs);
> +
> + if (dev->board.decoder == EM28XX_TVP5150)
> + v4l2_i2c_new_subdev(&dev->v4l2_dev, &dev->i2c_adap[dev->def_i2c_bus],
> + "tvp5150", 0, tvp5150_addrs);
> +
> + if (dev->board.adecoder == EM28XX_TVAUDIO)
> + v4l2_i2c_new_subdev(&dev->v4l2_dev, &dev->i2c_adap[dev->def_i2c_bus],
> + "tvaudio", dev->board.tvaudio_addr, NULL);
> +
> + /* Initialize tuner and camera */
> +
> + if (dev->board.tuner_type != TUNER_ABSENT) {
> + int has_demod = (dev->tda9887_conf & TDA9887_PRESENT);
> +
> + if (dev->board.radio.type)
> + v4l2_i2c_new_subdev(&dev->v4l2_dev, &dev->i2c_adap[dev->def_i2c_bus],
> + "tuner", dev->board.radio_addr, NULL);
> +
> + if (has_demod)
> + v4l2_i2c_new_subdev(&dev->v4l2_dev,
> + &dev->i2c_adap[dev->def_i2c_bus], "tuner",
> + 0, v4l2_i2c_tuner_addrs(ADDRS_DEMOD));
> + if (dev->tuner_addr == 0) {
> + enum v4l2_i2c_tuner_type type =
> + has_demod ? ADDRS_TV_WITH_DEMOD : ADDRS_TV;
> + struct v4l2_subdev *sd;
> +
> + sd = v4l2_i2c_new_subdev(&dev->v4l2_dev,
> + &dev->i2c_adap[dev->def_i2c_bus], "tuner",
> + 0, v4l2_i2c_tuner_addrs(type));
> +
> + if (sd)
> + dev->tuner_addr = v4l2_i2c_subdev_addr(sd);
> + } else {
> + v4l2_i2c_new_subdev(&dev->v4l2_dev, &dev->i2c_adap[dev->def_i2c_bus],
> + "tuner", dev->tuner_addr, NULL);
> + }
> + }
> +
> + em28xx_tuner_setup(dev);
> + em28xx_init_camera(dev);
> +
> + /* Configure audio */
> + ret = em28xx_audio_setup(dev);
> + if (ret < 0) {
> + em28xx_errdev("%s: Error while setting audio - error [%d]!\n",
> + __func__, ret);
> + goto unregister_dev;
> + }
> + if (dev->audio_mode.ac97 != EM28XX_NO_AC97) {
> + v4l2_ctrl_new_std(hdl, &em28xx_ctrl_ops,
> + V4L2_CID_AUDIO_MUTE, 0, 1, 1, 1);
> + v4l2_ctrl_new_std(hdl, &em28xx_ctrl_ops,
> + V4L2_CID_AUDIO_VOLUME, 0, 0x1f, 1, 0x1f);
> + } else {
> + /* install the em28xx notify callback */
> + v4l2_ctrl_notify(v4l2_ctrl_find(hdl, V4L2_CID_AUDIO_MUTE),
> + em28xx_ctrl_notify, dev);
> + v4l2_ctrl_notify(v4l2_ctrl_find(hdl, V4L2_CID_AUDIO_VOLUME),
> + em28xx_ctrl_notify, dev);
> + }
> +
> + /* wake i2c devices */
> + em28xx_wake_i2c(dev);
> +
> + /* init video dma queues */
> + INIT_LIST_HEAD(&dev->vidq.active);
> + INIT_LIST_HEAD(&dev->vbiq.active);
> +
> + if (dev->board.has_msp34xx) {
> + /* Send a reset to other chips via gpio */
> + ret = em28xx_write_reg(dev, EM2820_R08_GPIO_CTRL, 0xf7);
> + if (ret < 0) {
> + em28xx_errdev("%s: em28xx_write_reg - msp34xx(1) failed! error [%d]\n",
> + __func__, ret);
> + goto unregister_dev;
> + }
> + msleep(3);
> +
> + ret = em28xx_write_reg(dev, EM2820_R08_GPIO_CTRL, 0xff);
> + if (ret < 0) {
> + em28xx_errdev("%s: em28xx_write_reg - msp34xx(2) failed! error [%d]\n",
> + __func__, ret);
> + goto unregister_dev;
> + }
> + msleep(3);
> + }
> +
> /* set default norm */
> dev->norm = V4L2_STD_PAL;
> v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_std, dev->norm);
> @@ -2252,14 +2389,16 @@ int em28xx_register_analog_devices(struct em28xx *dev)
> /* Reset image controls */
> em28xx_colorlevels_set_default(dev);
> v4l2_ctrl_handler_setup(&dev->ctrl_handler);
> - if (dev->ctrl_handler.error)
> - return dev->ctrl_handler.error;
> + ret = dev->ctrl_handler.error;
> + if (ret)
> + goto unregister_dev;
>
> /* allocate and fill video video_device struct */
> dev->vdev = em28xx_vdev_init(dev, &em28xx_video_template, "video");
> if (!dev->vdev) {
> em28xx_errdev("cannot allocate video_device.\n");
> - return -ENODEV;
> + ret = -ENODEV;
> + goto unregister_dev;
> }
> dev->vdev->queue = &dev->vb_vidq;
> dev->vdev->queue->lock = &dev->vb_queue_lock;
> @@ -2289,7 +2428,7 @@ int em28xx_register_analog_devices(struct em28xx *dev)
> if (ret) {
> em28xx_errdev("unable to register video device (error=%i).\n",
> ret);
> - return ret;
> + goto unregister_dev;
> }
>
> /* Allocate and fill vbi video_device struct */
> @@ -2318,7 +2457,7 @@ int em28xx_register_analog_devices(struct em28xx *dev)
> vbi_nr[dev->devno]);
> if (ret < 0) {
> em28xx_errdev("unable to register vbi device\n");
> - return ret;
> + goto unregister_dev;
> }
> }
>
> @@ -2327,13 +2466,14 @@ int em28xx_register_analog_devices(struct em28xx *dev)
> "radio");
> if (!dev->radio_dev) {
> em28xx_errdev("cannot allocate video_device.\n");
> - return -ENODEV;
> + ret = -ENODEV;
> + goto unregister_dev;
> }
> ret = video_register_device(dev->radio_dev, VFL_TYPE_RADIO,
> radio_nr[dev->devno]);
> if (ret < 0) {
> em28xx_errdev("can't register radio device\n");
> - return ret;
> + goto unregister_dev;
> }
> em28xx_info("Registered radio device as %s\n",
> video_device_node_name(dev->radio_dev));
> @@ -2346,5 +2486,17 @@ int em28xx_register_analog_devices(struct em28xx *dev)
> em28xx_info("V4L2 VBI device registered as %s\n",
> video_device_node_name(dev->vbi_dev));
>
> + /* Save some power by putting tuner to sleep */
> + v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 0);
> +
> + /* initialize videobuf2 stuff */
> + em28xx_vb2_setup(dev);
> +
> return 0;
> +
> +unregister_dev:
> + v4l2_ctrl_handler_free(&dev->ctrl_handler);
> + v4l2_device_unregister(&dev->v4l2_dev);
> +err:
> + return ret;
> }
>
Ok, looks fine now. :)
Reviewed-by: Frank Schäfer <fschaefer.oss@xxxxxxxxxxxxxx>
--
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
