| Message ID | 1388832951-11195-5-git-send-email-m.chehab@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 04.01.2014 11:55, schrieb Mauro Carvalho Chehab: > Now that all analog-specific code are at em28xx-video, convert > it into an em28xx extension and load it as a separate module. > > Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com> > --- > drivers/media/usb/em28xx/Kconfig | 6 ++- > drivers/media/usb/em28xx/Makefile | 5 ++- > drivers/media/usb/em28xx/em28xx-camera.c | 1 + > drivers/media/usb/em28xx/em28xx-cards.c | 45 ++++--------------- > drivers/media/usb/em28xx/em28xx-core.c | 12 ++++++ > drivers/media/usb/em28xx/em28xx-v4l.h | 20 +++++++++ > drivers/media/usb/em28xx/em28xx-vbi.c | 1 + > drivers/media/usb/em28xx/em28xx-video.c | 74 +++++++++++++++++++++++--------- > drivers/media/usb/em28xx/em28xx.h | 23 ++-------- > 9 files changed, 107 insertions(+), 80 deletions(-) > create mode 100644 drivers/media/usb/em28xx/em28xx-v4l.h > > diff --git a/drivers/media/usb/em28xx/Kconfig b/drivers/media/usb/em28xx/Kconfig > index d6ba514d31eb..838fc9dbb747 100644 > --- a/drivers/media/usb/em28xx/Kconfig > +++ b/drivers/media/usb/em28xx/Kconfig > @@ -1,8 +1,12 @@ > config VIDEO_EM28XX > - tristate "Empia EM28xx USB video capture support" > + tristate "Empia EM28xx USB devices support" > depends on VIDEO_DEV && I2C > select VIDEO_TUNER > select VIDEO_TVEEPROM > + > +config VIDEO_EM28XX_V4L2 > + tristate "Empia EM28xx analog TV, video capture and/or webcam support" > + depends on VIDEO_EM28XX && I2C VIDEO_EM28XX already depends on I2C. > select VIDEOBUF2_VMALLOC > select VIDEO_SAA711X if MEDIA_SUBDRV_AUTOSELECT > select VIDEO_TVP5150 if MEDIA_SUBDRV_AUTOSELECT > diff --git a/drivers/media/usb/em28xx/Makefile b/drivers/media/usb/em28xx/Makefile > index ad6d48557940..3f850d5063d0 100644 > --- a/drivers/media/usb/em28xx/Makefile > +++ b/drivers/media/usb/em28xx/Makefile > @@ -1,10 +1,11 @@ > -em28xx-y += em28xx-video.o em28xx-i2c.o em28xx-cards.o > -em28xx-y += em28xx-core.o em28xx-vbi.o em28xx-camera.o > +em28xx-y += em28xx-core.o em28xx-i2c.o em28xx-cards.o em28xx-camera.o > > +em28xx-v4l-objs := em28xx-video.o em28xx-vbi.o > em28xx-alsa-objs := em28xx-audio.o > em28xx-rc-objs := em28xx-input.o > > obj-$(CONFIG_VIDEO_EM28XX) += em28xx.o > +obj-$(CONFIG_VIDEO_EM28XX_V4L2) += em28xx-v4l.o > obj-$(CONFIG_VIDEO_EM28XX_ALSA) += em28xx-alsa.o > obj-$(CONFIG_VIDEO_EM28XX_DVB) += em28xx-dvb.o > obj-$(CONFIG_VIDEO_EM28XX_RC) += em28xx-rc.o > diff --git a/drivers/media/usb/em28xx/em28xx-camera.c b/drivers/media/usb/em28xx/em28xx-camera.c > index d666741797d4..c29f5c4e7b40 100644 > --- a/drivers/media/usb/em28xx/em28xx-camera.c > +++ b/drivers/media/usb/em28xx/em28xx-camera.c > @@ -454,3 +454,4 @@ int em28xx_init_camera(struct em28xx *dev) > > return ret; > } > +EXPORT_SYMBOL_GPL(em28xx_init_camera); em28xx-camera should also be part of the em28xx-v4l module. > diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c > index 175cd776e0a1..938daaabd8e0 100644 > --- a/drivers/media/usb/em28xx/em28xx-cards.c > +++ b/drivers/media/usb/em28xx/em28xx-cards.c > @@ -2159,6 +2159,8 @@ struct em28xx_board em28xx_boards[] = { > .ir_codes = RC_MAP_PINNACLE_PCTV_HD, > }, > }; > +EXPORT_SYMBOL_GPL(em28xx_boards); > + > const unsigned int em28xx_bcount = ARRAY_SIZE(em28xx_boards); > > /* table of devices that work with this driver */ > @@ -2827,10 +2829,6 @@ static void em28xx_card_setup(struct em28xx *dev) > "tuner", dev->tuner_addr, NULL); > } > } > - > - em28xx_tuner_setup(dev); > - > - em28xx_init_camera(dev); > } Here you are fixing half of the em28xx_card_setup() oopses which you've introduced with patch 3/22. This needs to be done together with patch 5/22 before patch 3/22. > @@ -2848,11 +2846,12 @@ static void request_module_async(struct work_struct *work) > em28xx_init_extension(dev); > > #if defined(CONFIG_MODULES) && defined(MODULE) > + if (dev->has_video) > + request_module("em28xx-v4l"); > if (dev->has_audio_class) > request_module("snd-usb-audio"); > else if (dev->has_alsa_audio) > request_module("em28xx-alsa"); > - > if (dev->board.has_dvb) > request_module("em28xx-dvb"); > if (dev->board.buttons || > @@ -2881,18 +2880,12 @@ void em28xx_release_resources(struct em28xx *dev) > { > /*FIXME: I2C IR should be disconnected */ > > - em28xx_release_analog_resources(dev); > - > if (dev->def_i2c_bus) > em28xx_i2c_unregister(dev, 1); > em28xx_i2c_unregister(dev, 0); > if (dev->clk) > v4l2_clk_unregister_fixed(dev->clk); > > - v4l2_ctrl_handler_free(&dev->ctrl_handler); > - > - v4l2_device_unregister(&dev->v4l2_dev); > - > usb_put_dev(dev->udev); > > /* Mark device as unused */ > @@ -3043,7 +3036,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 */ > @@ -3057,30 +3050,14 @@ 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; > } > } Hmm... if registering of bus 1 fails, we need to unregister bus 0. But that's an old bug which we can fix later... > /* Do board specific init and eeprom reading */ > em28xx_card_setup(dev); > > - retval = em28xx_register_analog_devices(dev); > - if (retval < 0) { > - goto fail; > - } > - > return 0; > - > -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; > } > > /* high bandwidth multiplier, as encoded in highspeed endpoint descriptors */ > @@ -3283,6 +3260,7 @@ static int em28xx_usb_probe(struct usb_interface *interface, > dev->alt = -1; > dev->is_audio_only = has_audio && !(has_video || has_dvb); > dev->has_alsa_audio = has_audio; > + dev->has_video = has_video; > dev->audio_ifnum = ifnum; > > /* Checks if audio is provided by some interface */ > @@ -3322,10 +3300,9 @@ static int em28xx_usb_probe(struct usb_interface *interface, > > /* allocate device struct */ > mutex_init(&dev->lock); > - mutex_lock(&dev->lock); > retval = em28xx_init_dev(dev, udev, interface, nr); > if (retval) { > - goto unlock_and_free; > + goto err_free; > } > > if (usb_xfer_mode < 0) { > @@ -3368,7 +3345,7 @@ static int em28xx_usb_probe(struct usb_interface *interface, > if (retval) { > printk(DRIVER_NAME > ": Failed to pre-allocate USB transfer buffers for DVB.\n"); > - goto unlock_and_free; > + goto err_free; > } > } > > @@ -3377,13 +3354,9 @@ static int em28xx_usb_probe(struct usb_interface *interface, > /* Should be the last thing to do, to avoid newer udev's to > open the device before fully initializing it > */ > - mutex_unlock(&dev->lock); > > return 0; > > -unlock_and_free: > - mutex_unlock(&dev->lock); > - > err_free: > kfree(dev->alt_max_pkt_size_isoc); > kfree(dev); > diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c > index 3012912d2997..1113d4e107d8 100644 > --- a/drivers/media/usb/em28xx/em28xx-core.c > +++ b/drivers/media/usb/em28xx/em28xx-core.c > @@ -33,6 +33,18 @@ > > #include "em28xx.h" > > +#define DRIVER_AUTHOR "Ludovico Cavedon <cavedon@sssup.it>, " \ > + "Markus Rechberger <mrechberger@gmail.com>, " \ > + "Mauro Carvalho Chehab <mchehab@infradead.org>, " \ > + "Sascha Sommer <saschasommer@freenet.de>" > + > +#define DRIVER_DESC "Empia em28xx based USB core driver" > + > +MODULE_AUTHOR(DRIVER_AUTHOR); > +MODULE_DESCRIPTION(DRIVER_DESC); > +MODULE_LICENSE("GPL"); > +MODULE_VERSION(EM28XX_VERSION); > + > /* #define ENABLE_DEBUG_ISOC_FRAMES */ > > static unsigned int core_debug; > diff --git a/drivers/media/usb/em28xx/em28xx-v4l.h b/drivers/media/usb/em28xx/em28xx-v4l.h > new file mode 100644 > index 000000000000..bce438691e0e > --- /dev/null > +++ b/drivers/media/usb/em28xx/em28xx-v4l.h > @@ -0,0 +1,20 @@ > +/* > + em28xx-video.c - driver for Empia EM2800/EM2820/2840 USB > + video capture devices The information about supported chips is outdated everywhere in the driver, but if you introduce a new header file it should really be up to date. Just talk about EM27xx/EM28xx. > + > + Copyright (C) 2013-2014 Mauro Carvalho Chehab <m.chehab@samsung.com> > + > + 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 of the License. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + */ > + > + > +int em28xx_start_analog_streaming(struct vb2_queue *vq, unsigned int count); > +int em28xx_stop_vbi_streaming(struct vb2_queue *vq); > +extern struct vb2_ops em28xx_vbi_qops; > diff --git a/drivers/media/usb/em28xx/em28xx-vbi.c b/drivers/media/usb/em28xx/em28xx-vbi.c > index 39f39c527c13..db3d655600df 100644 > --- a/drivers/media/usb/em28xx/em28xx-vbi.c > +++ b/drivers/media/usb/em28xx/em28xx-vbi.c > @@ -27,6 +27,7 @@ > #include <linux/init.h> > > #include "em28xx.h" > +#include "em28xx-v4l.h" > > static unsigned int vbibufs = 5; > module_param(vbibufs, int, 0644); > diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c > index 85284626dbd6..d615bff8ac09 100644 > --- a/drivers/media/usb/em28xx/em28xx-video.c > +++ b/drivers/media/usb/em28xx/em28xx-video.c > @@ -38,6 +38,7 @@ > #include <linux/slab.h> > > #include "em28xx.h" > +#include "em28xx-v4l.h" > #include <media/v4l2-common.h> > #include <media/v4l2-ioctl.h> > #include <media/v4l2-event.h> > @@ -141,11 +142,13 @@ static struct em28xx_fmt format[] = { > }, > }; > > +static int em28xx_vbi_supported(struct em28xx *dev); > + Or move em28xx_vbi_supported() before em28xx_set_outfmt() and em28xx_resolution_set() again. If you had not changed the functions order in patch 1/22, this wouldn't be necessary. > /* > * em28xx_wake_i2c() > * configure i2c attached devices > */ > -void em28xx_wake_i2c(struct em28xx *dev) > +static void em28xx_wake_i2c(struct em28xx *dev) > { > v4l2_device_call_all(&dev->v4l2_dev, 0, core, reset, 0); > v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_routing, > @@ -153,7 +156,7 @@ void em28xx_wake_i2c(struct em28xx *dev) > v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_stream, 0); > } > > -int em28xx_colorlevels_set_default(struct em28xx *dev) > +static int em28xx_colorlevels_set_default(struct em28xx *dev) > { > em28xx_write_reg(dev, EM28XX_R20_YGAIN, CONTRAST_DEFAULT); > em28xx_write_reg(dev, EM28XX_R21_YOFFSET, BRIGHTNESS_DEFAULT); > @@ -171,7 +174,7 @@ int em28xx_colorlevels_set_default(struct em28xx *dev) > return em28xx_write_reg(dev, EM28XX_R1A_BOFFSET, 0x00); > } > > -int em28xx_set_outfmt(struct em28xx *dev) > +static int em28xx_set_outfmt(struct em28xx *dev) > { > int ret; > u8 fmt, vinctrl; > @@ -278,7 +281,7 @@ static int em28xx_scaler_set(struct em28xx *dev, u16 h, u16 v) > } > > /* FIXME: this only function read values from dev */ > -int em28xx_resolution_set(struct em28xx *dev) > +static int em28xx_resolution_set(struct em28xx *dev) > { > int width, height; > width = norm_maxw(dev); > @@ -310,7 +313,7 @@ int em28xx_resolution_set(struct em28xx *dev) > return em28xx_scaler_set(dev, dev->hscale, dev->vscale); > } > > -int em28xx_vbi_supported(struct em28xx *dev) > +static int em28xx_vbi_supported(struct em28xx *dev) > { > /* Modprobe option to manually disable */ > if (disable_vbi == 1) > @@ -330,7 +333,7 @@ int em28xx_vbi_supported(struct em28xx *dev) > } > > /* Set USB alternate setting for analog video */ > -int em28xx_set_alternate(struct em28xx *dev) > +static int em28xx_set_alternate(struct em28xx *dev) > { > int errCode; > int i; > @@ -1020,7 +1023,7 @@ static struct vb2_ops em28xx_video_qops = { > .wait_finish = vb2_ops_wait_finish, > }; > > -int em28xx_vb2_setup(struct em28xx *dev) > +static int em28xx_vb2_setup(struct em28xx *dev) > { > int rc; > struct vb2_queue *q; > @@ -1088,7 +1091,7 @@ static void video_mux(struct em28xx *dev, int index) > em28xx_audio_analog_set(dev); > } > > -void em28xx_ctrl_notify(struct v4l2_ctrl *ctrl, void *priv) > +static void em28xx_ctrl_notify(struct v4l2_ctrl *ctrl, void *priv) > { > struct em28xx *dev = priv; > > @@ -1625,7 +1628,7 @@ static int vidioc_g_register(struct file *file, void *priv, > reg->val = ret; > } else { > __le16 val = 0; > - ret = em28xx_read_reg_req_len(dev, USB_REQ_GET_STATUS, > + ret = dev->em28xx_read_reg_req_len(dev, USB_REQ_GET_STATUS, > reg->reg, (char *)&val, 2); Urgh... is it really necessary to start using these pointers again ? Ok... keep it as is, I'll send a patch to clean this up later. > if (ret < 0) > return ret; > @@ -1872,11 +1875,11 @@ static int em28xx_v4l2_open(struct file *filp) > } > > /* > - * em28xx_realease_resources() > + * em28xx_v4l2_fini() > * unregisters the v4l2,i2c and usb devices > * called when the device gets disconected or at module unload > */ > -void em28xx_release_analog_resources(struct em28xx *dev) > +static int em28xx_v4l2_fini(struct em28xx *dev) > { > > /*FIXME: I2C IR should be disconnected */ > @@ -1906,6 +1909,8 @@ void em28xx_release_analog_resources(struct em28xx *dev) > video_device_release(dev->vdev); > dev->vdev = NULL; > } > + > + return 0; > } > > /* > @@ -1927,12 +1932,12 @@ static int em28xx_v4l2_close(struct file *filp) > if (dev->users == 1) { > /* the device is already disconnect, > free the remaining resources */ > + > if (dev->disconnected) { > - em28xx_release_resources(dev); Who releases the resources now ? > + v4l2_ctrl_handler_free(&dev->ctrl_handler); > + v4l2_device_unregister(&dev->v4l2_dev); > kfree(dev->alt_max_pkt_size_isoc); > - mutex_unlock(&dev->lock); > - kfree(dev); > - return 0; > + goto exit; > } > > /* Save some power by putting tuner to sleep */ > @@ -1951,6 +1956,7 @@ static int em28xx_v4l2_close(struct file *filp) > } > } > > +exit: > dev->users--; Nice bugfix. > mutex_unlock(&dev->lock); > return 0; > @@ -2047,8 +2053,6 @@ static struct video_device em28xx_radio_template = { > > /******************************** usb interface ******************************/ > > - > - > static struct video_device *em28xx_vdev_init(struct em28xx *dev, > const struct video_device *template, > const char *type_name) > @@ -2122,7 +2126,7 @@ static void em28xx_setup_xc3028(struct em28xx *dev, struct xc2028_ctrl *ctl) > } > } > > -void em28xx_tuner_setup(struct em28xx *dev) > +static void em28xx_tuner_setup(struct em28xx *dev) > { > struct tuner_setup tun_setup; > struct v4l2_frequency f; > @@ -2181,14 +2185,14 @@ void em28xx_tuner_setup(struct em28xx *dev) > v4l2_device_call_all(&dev->v4l2_dev, 0, tuner, s_frequency, &f); > } > > -int em28xx_register_analog_devices(struct em28xx *dev) > +static int em28xx_v4l2_init(struct em28xx *dev) > { > u8 val; > int ret; > unsigned int maxw; > struct v4l2_ctrl_handler *hdl = &dev->ctrl_handler; > > - if (!dev->is_audio_only) { > + if (!dev->has_video) { > /* This device does not support the v4l2 extension */ > return 0; > } > @@ -2196,6 +2200,8 @@ int em28xx_register_analog_devices(struct em28xx *dev) > printk(KERN_INFO "%s: v4l2 driver version %s\n", > dev->name, EM28XX_VERSION); > > + mutex_lock(&dev->lock); > + > ret = v4l2_device_register(&dev->udev->dev, &dev->v4l2_dev); > if (ret < 0) { > em28xx_errdev("Call to v4l2_device_register() failed!\n"); > @@ -2212,6 +2218,10 @@ int em28xx_register_analog_devices(struct em28xx *dev) > dev->vinctl = EM28XX_VINCTRL_INTERLACED | > EM28XX_VINCTRL_CCIR656_ENABLE; > > + /* Initialize tuner and camera */ > + em28xx_tuner_setup(dev); > + em28xx_init_camera(dev); > + > /* Configure audio */ > ret = em28xx_audio_setup(dev); > if (ret < 0) { > @@ -2422,6 +2432,28 @@ int em28xx_register_analog_devices(struct em28xx *dev) > > /* initialize videobuf2 stuff */ > em28xx_vb2_setup(dev); > + > err: > - return 0; > + mutex_unlock(&dev->lock); > + return ret; > +} > + > +static struct em28xx_ops v4l2_ops = { > + .id = EM28XX_V4L2, > + .name = "Em28xx v4l2 Extension", > + .init = em28xx_v4l2_init, > + .fini = em28xx_v4l2_fini, > +}; > + > +static int __init em28xx_video_register(void) > +{ > + return em28xx_register_extension(&v4l2_ops); > } > + > +static void __exit em28xx_video_unregister(void) > +{ > + em28xx_unregister_extension(&v4l2_ops); > +} > + > +module_init(em28xx_video_register); > +module_exit(em28xx_video_unregister); > diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h > index 7ae05ebc13c1..9d6f43e4681f 100644 > --- a/drivers/media/usb/em28xx/em28xx.h > +++ b/drivers/media/usb/em28xx/em28xx.h > @@ -26,7 +26,7 @@ > #ifndef _EM28XX_H > #define _EM28XX_H > > -#define EM28XX_VERSION "0.2.0" > +#define EM28XX_VERSION "0.2.1" > > #include <linux/workqueue.h> > #include <linux/i2c.h> > @@ -474,6 +474,7 @@ struct em28xx_eeprom { > #define EM28XX_AUDIO 0x10 > #define EM28XX_DVB 0x20 > #define EM28XX_RC 0x30 > +#define EM28XX_V4L2 0x40 > > /* em28xx resource types (used for res_get/res_lock etc */ > #define EM28XX_RESOURCE_VIDEO 0x01 > @@ -527,6 +528,7 @@ struct em28xx { > > unsigned int is_em25xx:1; /* em25xx/em276x/7x/8x family bridge */ > unsigned char disconnected:1; /* device has been diconnected */ > + unsigned int has_video:1; > unsigned int has_audio_class:1; > unsigned int has_alsa_audio:1; > unsigned int is_audio_only:1; > @@ -723,14 +725,9 @@ int em28xx_write_ac97(struct em28xx *dev, u8 reg, u16 val); > int em28xx_audio_analog_set(struct em28xx *dev); > int em28xx_audio_setup(struct em28xx *dev); > > -int em28xx_colorlevels_set_default(struct em28xx *dev); > const struct em28xx_led *em28xx_find_led(struct em28xx *dev, > enum em28xx_led_role role); > int em28xx_capture_start(struct em28xx *dev, int start); > -int em28xx_vbi_supported(struct em28xx *dev); > -int em28xx_set_outfmt(struct em28xx *dev); > -int em28xx_resolution_set(struct em28xx *dev); > -int em28xx_set_alternate(struct em28xx *dev); > int em28xx_alloc_urbs(struct em28xx *dev, enum em28xx_mode mode, int xfer_bulk, > int num_bufs, int max_pkt_size, int packet_multiplier); > int em28xx_init_usb_xfer(struct em28xx *dev, enum em28xx_mode mode, > @@ -742,31 +739,17 @@ void em28xx_uninit_usb_xfer(struct em28xx *dev, enum em28xx_mode mode); > void em28xx_stop_urbs(struct em28xx *dev); > int em28xx_set_mode(struct em28xx *dev, enum em28xx_mode set_mode); > int em28xx_gpio_set(struct em28xx *dev, struct em28xx_reg_seq *gpio); > -void em28xx_wake_i2c(struct em28xx *dev); > int em28xx_register_extension(struct em28xx_ops *dev); > void em28xx_unregister_extension(struct em28xx_ops *dev); > void em28xx_init_extension(struct em28xx *dev); > void em28xx_close_extension(struct em28xx *dev); > > -/* Provided by em28xx-video.c */ > -void em28xx_tuner_setup(struct em28xx *dev); > -int em28xx_vb2_setup(struct em28xx *dev); > -int em28xx_register_analog_devices(struct em28xx *dev); > -void em28xx_release_analog_resources(struct em28xx *dev); > -void em28xx_ctrl_notify(struct v4l2_ctrl *ctrl, void *priv); > -int em28xx_start_analog_streaming(struct vb2_queue *vq, unsigned int count); > -int em28xx_stop_vbi_streaming(struct vb2_queue *vq); > -extern const struct v4l2_ctrl_ops em28xx_ctrl_ops; > - > /* Provided by em28xx-cards.c */ > extern struct em28xx_board em28xx_boards[]; > extern struct usb_device_id em28xx_id_table[]; > int em28xx_tuner_callback(void *ptr, int component, int command, int arg); > void em28xx_release_resources(struct em28xx *dev); > > -/* Provided by em28xx-vbi.c */ > -extern struct vb2_ops em28xx_vbi_qops; > - > /* Provided by em28xx-camera.c */ > int em28xx_detect_sensor(struct em28xx *dev); > int em28xx_init_camera(struct em28xx *dev); Nice clean-up ! :) -- 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
Em Sun, 05 Jan 2014 11:47:00 +0100 Frank Schäfer <fschaefer.oss@googlemail.com> escreveu: > Am 04.01.2014 11:55, schrieb Mauro Carvalho Chehab: > > Now that all analog-specific code are at em28xx-video, convert > > it into an em28xx extension and load it as a separate module. > > > > Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com> > > --- > > drivers/media/usb/em28xx/Kconfig | 6 ++- > > drivers/media/usb/em28xx/Makefile | 5 ++- > > drivers/media/usb/em28xx/em28xx-camera.c | 1 + > > drivers/media/usb/em28xx/em28xx-cards.c | 45 ++++--------------- > > drivers/media/usb/em28xx/em28xx-core.c | 12 ++++++ > > drivers/media/usb/em28xx/em28xx-v4l.h | 20 +++++++++ > > drivers/media/usb/em28xx/em28xx-vbi.c | 1 + > > drivers/media/usb/em28xx/em28xx-video.c | 74 +++++++++++++++++++++++--------- > > drivers/media/usb/em28xx/em28xx.h | 23 ++-------- > > 9 files changed, 107 insertions(+), 80 deletions(-) > > create mode 100644 drivers/media/usb/em28xx/em28xx-v4l.h > > > > diff --git a/drivers/media/usb/em28xx/Kconfig b/drivers/media/usb/em28xx/Kconfig > > index d6ba514d31eb..838fc9dbb747 100644 > > --- a/drivers/media/usb/em28xx/Kconfig > > +++ b/drivers/media/usb/em28xx/Kconfig > > @@ -1,8 +1,12 @@ > > config VIDEO_EM28XX > > - tristate "Empia EM28xx USB video capture support" > > + tristate "Empia EM28xx USB devices support" > > depends on VIDEO_DEV && I2C > > select VIDEO_TUNER > > select VIDEO_TVEEPROM > > + > > +config VIDEO_EM28XX_V4L2 > > + tristate "Empia EM28xx analog TV, video capture and/or webcam support" > > + depends on VIDEO_EM28XX && I2C > VIDEO_EM28XX already depends on I2C. > > > select VIDEOBUF2_VMALLOC > > select VIDEO_SAA711X if MEDIA_SUBDRV_AUTOSELECT > > select VIDEO_TVP5150 if MEDIA_SUBDRV_AUTOSELECT > > diff --git a/drivers/media/usb/em28xx/Makefile b/drivers/media/usb/em28xx/Makefile > > index ad6d48557940..3f850d5063d0 100644 > > --- a/drivers/media/usb/em28xx/Makefile > > +++ b/drivers/media/usb/em28xx/Makefile > > @@ -1,10 +1,11 @@ > > -em28xx-y += em28xx-video.o em28xx-i2c.o em28xx-cards.o > > -em28xx-y += em28xx-core.o em28xx-vbi.o em28xx-camera.o > > +em28xx-y += em28xx-core.o em28xx-i2c.o em28xx-cards.o em28xx-camera.o > > > > +em28xx-v4l-objs := em28xx-video.o em28xx-vbi.o > > em28xx-alsa-objs := em28xx-audio.o > > em28xx-rc-objs := em28xx-input.o > > > > obj-$(CONFIG_VIDEO_EM28XX) += em28xx.o > > +obj-$(CONFIG_VIDEO_EM28XX_V4L2) += em28xx-v4l.o > > obj-$(CONFIG_VIDEO_EM28XX_ALSA) += em28xx-alsa.o > > obj-$(CONFIG_VIDEO_EM28XX_DVB) += em28xx-dvb.o > > obj-$(CONFIG_VIDEO_EM28XX_RC) += em28xx-rc.o > > diff --git a/drivers/media/usb/em28xx/em28xx-camera.c b/drivers/media/usb/em28xx/em28xx-camera.c > > index d666741797d4..c29f5c4e7b40 100644 > > --- a/drivers/media/usb/em28xx/em28xx-camera.c > > +++ b/drivers/media/usb/em28xx/em28xx-camera.c > > @@ -454,3 +454,4 @@ int em28xx_init_camera(struct em28xx *dev) > > > > return ret; > > } > > +EXPORT_SYMBOL_GPL(em28xx_init_camera); > em28xx-camera should also be part of the em28xx-v4l module. I tried that. Moving em28xx-camera to em28xx-v4l would cause a recursive dependency, due to the camera probing logic, that should be called by em28xx-cards.c. Moving that probing part to em28xx-v4l is too complex, due to the code that detects the board. One solution would be to break em28xx-camera into two parts: the detection code, to be merged with the core, and the sensor part, to be merged with em28xx-v4l, but that sounds ugly. Other solution would be to use something like the dvb_attach() macro, to avoid the recursive dependency, but that would be a dirty solution. As the code there is not big, the amount of overhead of having everything at em28xx-core is not high. So, I opted to keep the simplest path here, avoiding the risk of breaking something with a complex changeset. > > diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c > > index 175cd776e0a1..938daaabd8e0 100644 > > --- a/drivers/media/usb/em28xx/em28xx-cards.c > > +++ b/drivers/media/usb/em28xx/em28xx-cards.c > > @@ -2159,6 +2159,8 @@ struct em28xx_board em28xx_boards[] = { > > .ir_codes = RC_MAP_PINNACLE_PCTV_HD, > > }, > > }; > > +EXPORT_SYMBOL_GPL(em28xx_boards); > > + > > const unsigned int em28xx_bcount = ARRAY_SIZE(em28xx_boards); > > > > /* table of devices that work with this driver */ > > @@ -2827,10 +2829,6 @@ static void em28xx_card_setup(struct em28xx *dev) > > "tuner", dev->tuner_addr, NULL); > > } > > } > > - > > - em28xx_tuner_setup(dev); > > - > > - em28xx_init_camera(dev); > > } > Here you are fixing half of the em28xx_card_setup() oopses which you've > introduced with patch 3/22. > This needs to be done together with patch 5/22 before patch 3/22. Ok. > > @@ -2848,11 +2846,12 @@ static void request_module_async(struct work_struct *work) > > em28xx_init_extension(dev); > > > > #if defined(CONFIG_MODULES) && defined(MODULE) > > + if (dev->has_video) > > + request_module("em28xx-v4l"); > > if (dev->has_audio_class) > > request_module("snd-usb-audio"); > > else if (dev->has_alsa_audio) > > request_module("em28xx-alsa"); > > - > > if (dev->board.has_dvb) > > request_module("em28xx-dvb"); > > if (dev->board.buttons || > > @@ -2881,18 +2880,12 @@ void em28xx_release_resources(struct em28xx *dev) > > { > > /*FIXME: I2C IR should be disconnected */ > > > > - em28xx_release_analog_resources(dev); > > - > > if (dev->def_i2c_bus) > > em28xx_i2c_unregister(dev, 1); > > em28xx_i2c_unregister(dev, 0); > > if (dev->clk) > > v4l2_clk_unregister_fixed(dev->clk); > > > > - v4l2_ctrl_handler_free(&dev->ctrl_handler); > > - > > - v4l2_device_unregister(&dev->v4l2_dev); > > - > > usb_put_dev(dev->udev); > > > > /* Mark device as unused */ > > @@ -3043,7 +3036,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 */ > > @@ -3057,30 +3050,14 @@ 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; > > } > > } > Hmm... if registering of bus 1 fails, we need to unregister bus 0. > But that's an old bug which we can fix later... Good point. Yes, this should be on a separate patch. > > /* Do board specific init and eeprom reading */ > > em28xx_card_setup(dev); > > > > - retval = em28xx_register_analog_devices(dev); > > - if (retval < 0) { > > - goto fail; > > - } > > - > > return 0; > > - > > -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; > > } > > > > /* high bandwidth multiplier, as encoded in highspeed endpoint descriptors */ > > @@ -3283,6 +3260,7 @@ static int em28xx_usb_probe(struct usb_interface *interface, > > dev->alt = -1; > > dev->is_audio_only = has_audio && !(has_video || has_dvb); > > dev->has_alsa_audio = has_audio; > > + dev->has_video = has_video; > > dev->audio_ifnum = ifnum; > > > > /* Checks if audio is provided by some interface */ > > @@ -3322,10 +3300,9 @@ static int em28xx_usb_probe(struct usb_interface *interface, > > > > /* allocate device struct */ > > mutex_init(&dev->lock); > > - mutex_lock(&dev->lock); > > retval = em28xx_init_dev(dev, udev, interface, nr); > > if (retval) { > > - goto unlock_and_free; > > + goto err_free; > > } > > > > if (usb_xfer_mode < 0) { > > @@ -3368,7 +3345,7 @@ static int em28xx_usb_probe(struct usb_interface *interface, > > if (retval) { > > printk(DRIVER_NAME > > ": Failed to pre-allocate USB transfer buffers for DVB.\n"); > > - goto unlock_and_free; > > + goto err_free; > > } > > } > > > > @@ -3377,13 +3354,9 @@ static int em28xx_usb_probe(struct usb_interface *interface, > > /* Should be the last thing to do, to avoid newer udev's to > > open the device before fully initializing it > > */ > > - mutex_unlock(&dev->lock); > > > > return 0; > > > > -unlock_and_free: > > - mutex_unlock(&dev->lock); > > - > > err_free: > > kfree(dev->alt_max_pkt_size_isoc); > > kfree(dev); > > diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c > > index 3012912d2997..1113d4e107d8 100644 > > --- a/drivers/media/usb/em28xx/em28xx-core.c > > +++ b/drivers/media/usb/em28xx/em28xx-core.c > > @@ -33,6 +33,18 @@ > > > > #include "em28xx.h" > > > > +#define DRIVER_AUTHOR "Ludovico Cavedon <cavedon@sssup.it>, " \ > > + "Markus Rechberger <mrechberger@gmail.com>, " \ > > + "Mauro Carvalho Chehab <mchehab@infradead.org>, " \ > > + "Sascha Sommer <saschasommer@freenet.de>" > > + > > +#define DRIVER_DESC "Empia em28xx based USB core driver" > > + > > +MODULE_AUTHOR(DRIVER_AUTHOR); > > +MODULE_DESCRIPTION(DRIVER_DESC); > > +MODULE_LICENSE("GPL"); > > +MODULE_VERSION(EM28XX_VERSION); > > + > > /* #define ENABLE_DEBUG_ISOC_FRAMES */ > > > > static unsigned int core_debug; > > diff --git a/drivers/media/usb/em28xx/em28xx-v4l.h b/drivers/media/usb/em28xx/em28xx-v4l.h > > new file mode 100644 > > index 000000000000..bce438691e0e > > --- /dev/null > > +++ b/drivers/media/usb/em28xx/em28xx-v4l.h > > @@ -0,0 +1,20 @@ > > +/* > > + em28xx-video.c - driver for Empia EM2800/EM2820/2840 USB > > + video capture devices > The information about supported chips is outdated everywhere in the driver, > but if you introduce a new header file it should really be up to date. > Just talk about EM27xx/EM28xx. We need to do a cleanup on all those headers: they don't follow Kernel CodingStyle and the old headers use my previous email. For the sake of keeping a coherency, I deliberately opted to just use the same way as the other headers. My plan is to write a patch series fixing this and other CodingStyle issues on em28xx after merging this changeset. > > + > > + Copyright (C) 2013-2014 Mauro Carvalho Chehab <m.chehab@samsung.com> > > + > > + 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 of the License. > > + > > + This program is distributed in the hope that it will be useful, > > + but WITHOUT ANY WARRANTY; without even the implied warranty of > > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + GNU General Public License for more details. > > + */ > > + > > + > > +int em28xx_start_analog_streaming(struct vb2_queue *vq, unsigned int count); > > +int em28xx_stop_vbi_streaming(struct vb2_queue *vq); > > +extern struct vb2_ops em28xx_vbi_qops; > > diff --git a/drivers/media/usb/em28xx/em28xx-vbi.c b/drivers/media/usb/em28xx/em28xx-vbi.c > > index 39f39c527c13..db3d655600df 100644 > > --- a/drivers/media/usb/em28xx/em28xx-vbi.c > > +++ b/drivers/media/usb/em28xx/em28xx-vbi.c > > @@ -27,6 +27,7 @@ > > #include <linux/init.h> > > > > #include "em28xx.h" > > +#include "em28xx-v4l.h" > > > > static unsigned int vbibufs = 5; > > module_param(vbibufs, int, 0644); > > diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c > > index 85284626dbd6..d615bff8ac09 100644 > > --- a/drivers/media/usb/em28xx/em28xx-video.c > > +++ b/drivers/media/usb/em28xx/em28xx-video.c > > @@ -38,6 +38,7 @@ > > #include <linux/slab.h> > > > > #include "em28xx.h" > > +#include "em28xx-v4l.h" > > #include <media/v4l2-common.h> > > #include <media/v4l2-ioctl.h> > > #include <media/v4l2-event.h> > > @@ -141,11 +142,13 @@ static struct em28xx_fmt format[] = { > > }, > > }; > > > > +static int em28xx_vbi_supported(struct em28xx *dev); > > + > Or move em28xx_vbi_supported() before em28xx_set_outfmt() and > em28xx_resolution_set() again. > If you had not changed the functions order in patch 1/22, this wouldn't > be necessary. True. I'll put an extra cleanup patch to remove this reference on the CodingStyle cleanup patchset I'm planning to do. > > /* > > * em28xx_wake_i2c() > > * configure i2c attached devices > > */ > > -void em28xx_wake_i2c(struct em28xx *dev) > > +static void em28xx_wake_i2c(struct em28xx *dev) > > { > > v4l2_device_call_all(&dev->v4l2_dev, 0, core, reset, 0); > > v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_routing, > > @@ -153,7 +156,7 @@ void em28xx_wake_i2c(struct em28xx *dev) > > v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_stream, 0); > > } > > > > -int em28xx_colorlevels_set_default(struct em28xx *dev) > > +static int em28xx_colorlevels_set_default(struct em28xx *dev) > > { > > em28xx_write_reg(dev, EM28XX_R20_YGAIN, CONTRAST_DEFAULT); > > em28xx_write_reg(dev, EM28XX_R21_YOFFSET, BRIGHTNESS_DEFAULT); > > @@ -171,7 +174,7 @@ int em28xx_colorlevels_set_default(struct em28xx *dev) > > return em28xx_write_reg(dev, EM28XX_R1A_BOFFSET, 0x00); > > } > > > > -int em28xx_set_outfmt(struct em28xx *dev) > > +static int em28xx_set_outfmt(struct em28xx *dev) > > { > > int ret; > > u8 fmt, vinctrl; > > @@ -278,7 +281,7 @@ static int em28xx_scaler_set(struct em28xx *dev, u16 h, u16 v) > > } > > > > /* FIXME: this only function read values from dev */ > > -int em28xx_resolution_set(struct em28xx *dev) > > +static int em28xx_resolution_set(struct em28xx *dev) > > { > > int width, height; > > width = norm_maxw(dev); > > @@ -310,7 +313,7 @@ int em28xx_resolution_set(struct em28xx *dev) > > return em28xx_scaler_set(dev, dev->hscale, dev->vscale); > > } > > > > -int em28xx_vbi_supported(struct em28xx *dev) > > +static int em28xx_vbi_supported(struct em28xx *dev) > > { > > /* Modprobe option to manually disable */ > > if (disable_vbi == 1) > > @@ -330,7 +333,7 @@ int em28xx_vbi_supported(struct em28xx *dev) > > } > > > > /* Set USB alternate setting for analog video */ > > -int em28xx_set_alternate(struct em28xx *dev) > > +static int em28xx_set_alternate(struct em28xx *dev) > > { > > int errCode; > > int i; > > @@ -1020,7 +1023,7 @@ static struct vb2_ops em28xx_video_qops = { > > .wait_finish = vb2_ops_wait_finish, > > }; > > > > -int em28xx_vb2_setup(struct em28xx *dev) > > +static int em28xx_vb2_setup(struct em28xx *dev) > > { > > int rc; > > struct vb2_queue *q; > > @@ -1088,7 +1091,7 @@ static void video_mux(struct em28xx *dev, int index) > > em28xx_audio_analog_set(dev); > > } > > > > -void em28xx_ctrl_notify(struct v4l2_ctrl *ctrl, void *priv) > > +static void em28xx_ctrl_notify(struct v4l2_ctrl *ctrl, void *priv) > > { > > struct em28xx *dev = priv; > > > > @@ -1625,7 +1628,7 @@ static int vidioc_g_register(struct file *file, void *priv, > > reg->val = ret; > > } else { > > __le16 val = 0; > > - ret = em28xx_read_reg_req_len(dev, USB_REQ_GET_STATUS, > > + ret = dev->em28xx_read_reg_req_len(dev, USB_REQ_GET_STATUS, > > reg->reg, (char *)&val, 2); > Urgh... is it really necessary to start using these pointers again ? > Ok... keep it as is, I'll send a patch to clean this up later. This is one of the few places where we weren't using those pointers: $ git grep em28xx_read_reg_req_len drivers/media/usb/em28xx/ drivers/media/usb/em28xx/em28xx-cards.c: dev->em28xx_read_reg_req_len = em28xx_read_reg_req_len; drivers/media/usb/em28xx/em28xx-core.c:int em28xx_read_reg_req_len(struct em28xx *dev, u8 req, u16 reg, drivers/media/usb/em28xx/em28xx-core.c: ret = em28xx_read_reg_req_len(dev, req, reg, &val, 1); drivers/media/usb/em28xx/em28xx-core.c: ret = dev->em28xx_read_reg_req_len(dev, 0, EM28XX_R40_AC97LSB, drivers/media/usb/em28xx/em28xx-i2c.c: ret = dev->em28xx_read_reg_req_len(dev, 0x00, 4-len, buf2, len); drivers/media/usb/em28xx/em28xx-i2c.c: ret = dev->em28xx_read_reg_req_len(dev, 2, addr, buf, len); drivers/media/usb/em28xx/em28xx-i2c.c: ret = dev->em28xx_read_reg_req_len(dev, 0x06, addr, buf, len); drivers/media/usb/em28xx/em28xx-input.c: rc = dev->em28xx_read_reg_req_len(dev, 0, EM28XX_R45_IR, drivers/media/usb/em28xx/em28xx-input.c: rc = dev->em28xx_read_reg_req_len(dev, 0, EM2874_R51_IR, drivers/media/usb/em28xx/em28xx-video.c: ret = dev->em28xx_read_reg_req_len(dev, USB_REQ_GET_STATUS, drivers/media/usb/em28xx/em28xx.h: int (*em28xx_read_reg_req_len) (struct em28xx *dev, u8 req, u16 reg, drivers/media/usb/em28xx/em28xx.h:int em28xx_read_reg_req_len(struct em28xx *dev, u8 req, u16 reg, For the sake of coherency, we should either use one or the other way. > > > if (ret < 0) > > return ret; > > @@ -1872,11 +1875,11 @@ static int em28xx_v4l2_open(struct file *filp) > > } > > > > /* > > - * em28xx_realease_resources() > > + * em28xx_v4l2_fini() > > * unregisters the v4l2,i2c and usb devices > > * called when the device gets disconected or at module unload > > */ > > -void em28xx_release_analog_resources(struct em28xx *dev) > > +static int em28xx_v4l2_fini(struct em28xx *dev) > > { > > > > /*FIXME: I2C IR should be disconnected */ > > @@ -1906,6 +1909,8 @@ void em28xx_release_analog_resources(struct em28xx *dev) > > video_device_release(dev->vdev); > > dev->vdev = NULL; > > } > > + > > + return 0; > > } > > > > /* > > @@ -1927,12 +1932,12 @@ static int em28xx_v4l2_close(struct file *filp) > > if (dev->users == 1) { > > /* the device is already disconnect, > > free the remaining resources */ > > + > > if (dev->disconnected) { > > - em28xx_release_resources(dev); > Who releases the resources now ? This is tricky. The hole idea is that em28xx-v4l release the resources allocated for V4L only, and not all resources, the same way as the other em28xx modules do. If you take a look at the original em28xx_release_resources(), what it does is to call this function, and then to de-allocate and unregister v4l2: v4l2_ctrl_handler_free(&dev->ctrl_handler); v4l2_device_unregister(&dev->v4l2_dev); after that, it deallocates the rest of allocated data. Now, em28xx_release_resources() is only called when em28xx is removed from memory or when the device is removed. The code there first calls em28xx_close_extension(dev). So, at device removal, em28xx-v4l will release the V4L2 specific resources, and em28xx_release_resources() will drop the common dev struct. I suspect that it might still be a bug there, but, if so, this bug is also present on em28xx-dvb, em28xx-alsa and em28xx-rc: what happens if the device is removed while the file descriptors is still opened? Maybe the driver core already prevent such things, but I'm not sure. If there's a bug out there, the proper fix seems to use kref for struct em28xx, increasing refcount for every em28xx extension and/or file open, decreasing it at either extension fini call, or at file close. This way, em28xx_release_resources() would be called only after refcount reaching zero, e. g. after being sure that nobody is using it. My plan is to take a look on it after having this changeset merged, as such change, if needed, will be complex and would require lots of testing. Also, it is independent on those changes. > > > + v4l2_ctrl_handler_free(&dev->ctrl_handler); > > + v4l2_device_unregister(&dev->v4l2_dev); > > kfree(dev->alt_max_pkt_size_isoc); > > - mutex_unlock(&dev->lock); > > - kfree(dev); > > - return 0; > > + goto exit; > > } > > > > /* Save some power by putting tuner to sleep */ > > @@ -1951,6 +1956,7 @@ static int em28xx_v4l2_close(struct file *filp) > > } > > } > > > > +exit: > > dev->users--; > Nice bugfix. > > > mutex_unlock(&dev->lock); > > return 0; > > @@ -2047,8 +2053,6 @@ static struct video_device em28xx_radio_template = { > > > > /******************************** usb interface ******************************/ > > > > - > > - > > static struct video_device *em28xx_vdev_init(struct em28xx *dev, > > const struct video_device *template, > > const char *type_name) > > @@ -2122,7 +2126,7 @@ static void em28xx_setup_xc3028(struct em28xx *dev, struct xc2028_ctrl *ctl) > > } > > } > > > > -void em28xx_tuner_setup(struct em28xx *dev) > > +static void em28xx_tuner_setup(struct em28xx *dev) > > { > > struct tuner_setup tun_setup; > > struct v4l2_frequency f; > > @@ -2181,14 +2185,14 @@ void em28xx_tuner_setup(struct em28xx *dev) > > v4l2_device_call_all(&dev->v4l2_dev, 0, tuner, s_frequency, &f); > > } > > > > -int em28xx_register_analog_devices(struct em28xx *dev) > > +static int em28xx_v4l2_init(struct em28xx *dev) > > { > > u8 val; > > int ret; > > unsigned int maxw; > > struct v4l2_ctrl_handler *hdl = &dev->ctrl_handler; > > > > - if (!dev->is_audio_only) { > > + if (!dev->has_video) { > > /* This device does not support the v4l2 extension */ > > return 0; > > } > > @@ -2196,6 +2200,8 @@ int em28xx_register_analog_devices(struct em28xx *dev) > > printk(KERN_INFO "%s: v4l2 driver version %s\n", > > dev->name, EM28XX_VERSION); > > > > + mutex_lock(&dev->lock); > > + > > ret = v4l2_device_register(&dev->udev->dev, &dev->v4l2_dev); > > if (ret < 0) { > > em28xx_errdev("Call to v4l2_device_register() failed!\n"); > > @@ -2212,6 +2218,10 @@ int em28xx_register_analog_devices(struct em28xx *dev) > > dev->vinctl = EM28XX_VINCTRL_INTERLACED | > > EM28XX_VINCTRL_CCIR656_ENABLE; > > > > + /* Initialize tuner and camera */ > > + em28xx_tuner_setup(dev); > > + em28xx_init_camera(dev); > > + > > /* Configure audio */ > > ret = em28xx_audio_setup(dev); > > if (ret < 0) { > > @@ -2422,6 +2432,28 @@ int em28xx_register_analog_devices(struct em28xx *dev) > > > > /* initialize videobuf2 stuff */ > > em28xx_vb2_setup(dev); > > + > > err: > > - return 0; > > + mutex_unlock(&dev->lock); > > + return ret; > > +} > > + > > +static struct em28xx_ops v4l2_ops = { > > + .id = EM28XX_V4L2, > > + .name = "Em28xx v4l2 Extension", > > + .init = em28xx_v4l2_init, > > + .fini = em28xx_v4l2_fini, > > +}; > > + > > +static int __init em28xx_video_register(void) > > +{ > > + return em28xx_register_extension(&v4l2_ops); > > } > > + > > +static void __exit em28xx_video_unregister(void) > > +{ > > + em28xx_unregister_extension(&v4l2_ops); > > +} > > + > > +module_init(em28xx_video_register); > > +module_exit(em28xx_video_unregister); > > diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h > > index 7ae05ebc13c1..9d6f43e4681f 100644 > > --- a/drivers/media/usb/em28xx/em28xx.h > > +++ b/drivers/media/usb/em28xx/em28xx.h > > @@ -26,7 +26,7 @@ > > #ifndef _EM28XX_H > > #define _EM28XX_H > > > > -#define EM28XX_VERSION "0.2.0" > > +#define EM28XX_VERSION "0.2.1" > > > > #include <linux/workqueue.h> > > #include <linux/i2c.h> > > @@ -474,6 +474,7 @@ struct em28xx_eeprom { > > #define EM28XX_AUDIO 0x10 > > #define EM28XX_DVB 0x20 > > #define EM28XX_RC 0x30 > > +#define EM28XX_V4L2 0x40 > > > > /* em28xx resource types (used for res_get/res_lock etc */ > > #define EM28XX_RESOURCE_VIDEO 0x01 > > @@ -527,6 +528,7 @@ struct em28xx { > > > > unsigned int is_em25xx:1; /* em25xx/em276x/7x/8x family bridge */ > > unsigned char disconnected:1; /* device has been diconnected */ > > + unsigned int has_video:1; > > unsigned int has_audio_class:1; > > unsigned int has_alsa_audio:1; > > unsigned int is_audio_only:1; > > @@ -723,14 +725,9 @@ int em28xx_write_ac97(struct em28xx *dev, u8 reg, u16 val); > > int em28xx_audio_analog_set(struct em28xx *dev); > > int em28xx_audio_setup(struct em28xx *dev); > > > > -int em28xx_colorlevels_set_default(struct em28xx *dev); > > const struct em28xx_led *em28xx_find_led(struct em28xx *dev, > > enum em28xx_led_role role); > > int em28xx_capture_start(struct em28xx *dev, int start); > > -int em28xx_vbi_supported(struct em28xx *dev); > > -int em28xx_set_outfmt(struct em28xx *dev); > > -int em28xx_resolution_set(struct em28xx *dev); > > -int em28xx_set_alternate(struct em28xx *dev); > > int em28xx_alloc_urbs(struct em28xx *dev, enum em28xx_mode mode, int xfer_bulk, > > int num_bufs, int max_pkt_size, int packet_multiplier); > > int em28xx_init_usb_xfer(struct em28xx *dev, enum em28xx_mode mode, > > @@ -742,31 +739,17 @@ void em28xx_uninit_usb_xfer(struct em28xx *dev, enum em28xx_mode mode); > > void em28xx_stop_urbs(struct em28xx *dev); > > int em28xx_set_mode(struct em28xx *dev, enum em28xx_mode set_mode); > > int em28xx_gpio_set(struct em28xx *dev, struct em28xx_reg_seq *gpio); > > -void em28xx_wake_i2c(struct em28xx *dev); > > int em28xx_register_extension(struct em28xx_ops *dev); > > void em28xx_unregister_extension(struct em28xx_ops *dev); > > void em28xx_init_extension(struct em28xx *dev); > > void em28xx_close_extension(struct em28xx *dev); > > > > -/* Provided by em28xx-video.c */ > > -void em28xx_tuner_setup(struct em28xx *dev); > > -int em28xx_vb2_setup(struct em28xx *dev); > > -int em28xx_register_analog_devices(struct em28xx *dev); > > -void em28xx_release_analog_resources(struct em28xx *dev); > > -void em28xx_ctrl_notify(struct v4l2_ctrl *ctrl, void *priv); > > -int em28xx_start_analog_streaming(struct vb2_queue *vq, unsigned int count); > > -int em28xx_stop_vbi_streaming(struct vb2_queue *vq); > > -extern const struct v4l2_ctrl_ops em28xx_ctrl_ops; > > - > > /* Provided by em28xx-cards.c */ > > extern struct em28xx_board em28xx_boards[]; > > extern struct usb_device_id em28xx_id_table[]; > > int em28xx_tuner_callback(void *ptr, int component, int command, int arg); > > void em28xx_release_resources(struct em28xx *dev); > > > > -/* Provided by em28xx-vbi.c */ > > -extern struct vb2_ops em28xx_vbi_qops; > > - > > /* Provided by em28xx-camera.c */ > > int em28xx_detect_sensor(struct em28xx *dev); > > int em28xx_init_camera(struct em28xx *dev); > Nice clean-up ! :) Thanks!
Am 05.01.2014 13:56, schrieb Mauro Carvalho Chehab: > Em Sun, 05 Jan 2014 11:47:00 +0100 > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu: > >> Am 04.01.2014 11:55, schrieb Mauro Carvalho Chehab: >>> Now that all analog-specific code are at em28xx-video, convert >>> it into an em28xx extension and load it as a separate module. >>> >>> Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com> >>> --- >>> drivers/media/usb/em28xx/Kconfig | 6 ++- >>> drivers/media/usb/em28xx/Makefile | 5 ++- >>> drivers/media/usb/em28xx/em28xx-camera.c | 1 + >>> drivers/media/usb/em28xx/em28xx-cards.c | 45 ++++--------------- >>> drivers/media/usb/em28xx/em28xx-core.c | 12 ++++++ >>> drivers/media/usb/em28xx/em28xx-v4l.h | 20 +++++++++ >>> drivers/media/usb/em28xx/em28xx-vbi.c | 1 + >>> drivers/media/usb/em28xx/em28xx-video.c | 74 +++++++++++++++++++++++--------- >>> drivers/media/usb/em28xx/em28xx.h | 23 ++-------- >>> 9 files changed, 107 insertions(+), 80 deletions(-) >>> create mode 100644 drivers/media/usb/em28xx/em28xx-v4l.h >>> >>> diff --git a/drivers/media/usb/em28xx/Kconfig b/drivers/media/usb/em28xx/Kconfig >>> index d6ba514d31eb..838fc9dbb747 100644 >>> --- a/drivers/media/usb/em28xx/Kconfig >>> +++ b/drivers/media/usb/em28xx/Kconfig >>> @@ -1,8 +1,12 @@ >>> config VIDEO_EM28XX >>> - tristate "Empia EM28xx USB video capture support" >>> + tristate "Empia EM28xx USB devices support" >>> depends on VIDEO_DEV && I2C >>> select VIDEO_TUNER >>> select VIDEO_TVEEPROM >>> + >>> +config VIDEO_EM28XX_V4L2 >>> + tristate "Empia EM28xx analog TV, video capture and/or webcam support" >>> + depends on VIDEO_EM28XX && I2C >> VIDEO_EM28XX already depends on I2C. >> >>> select VIDEOBUF2_VMALLOC >>> select VIDEO_SAA711X if MEDIA_SUBDRV_AUTOSELECT >>> select VIDEO_TVP5150 if MEDIA_SUBDRV_AUTOSELECT >>> diff --git a/drivers/media/usb/em28xx/Makefile b/drivers/media/usb/em28xx/Makefile >>> index ad6d48557940..3f850d5063d0 100644 >>> --- a/drivers/media/usb/em28xx/Makefile >>> +++ b/drivers/media/usb/em28xx/Makefile >>> @@ -1,10 +1,11 @@ >>> -em28xx-y += em28xx-video.o em28xx-i2c.o em28xx-cards.o >>> -em28xx-y += em28xx-core.o em28xx-vbi.o em28xx-camera.o >>> +em28xx-y += em28xx-core.o em28xx-i2c.o em28xx-cards.o em28xx-camera.o >>> >>> +em28xx-v4l-objs := em28xx-video.o em28xx-vbi.o >>> em28xx-alsa-objs := em28xx-audio.o >>> em28xx-rc-objs := em28xx-input.o >>> >>> obj-$(CONFIG_VIDEO_EM28XX) += em28xx.o >>> +obj-$(CONFIG_VIDEO_EM28XX_V4L2) += em28xx-v4l.o >>> obj-$(CONFIG_VIDEO_EM28XX_ALSA) += em28xx-alsa.o >>> obj-$(CONFIG_VIDEO_EM28XX_DVB) += em28xx-dvb.o >>> obj-$(CONFIG_VIDEO_EM28XX_RC) += em28xx-rc.o >>> diff --git a/drivers/media/usb/em28xx/em28xx-camera.c b/drivers/media/usb/em28xx/em28xx-camera.c >>> index d666741797d4..c29f5c4e7b40 100644 >>> --- a/drivers/media/usb/em28xx/em28xx-camera.c >>> +++ b/drivers/media/usb/em28xx/em28xx-camera.c >>> @@ -454,3 +454,4 @@ int em28xx_init_camera(struct em28xx *dev) >>> >>> return ret; >>> } >>> +EXPORT_SYMBOL_GPL(em28xx_init_camera); >> em28xx-camera should also be part of the em28xx-v4l module. > I tried that. Moving em28xx-camera to em28xx-v4l would cause a recursive > dependency, due to the camera probing logic, that should be called by > em28xx-cards.c. I see... > Moving that probing part to em28xx-v4l is too complex, due to the code > that detects the board. > > One solution would be to break em28xx-camera into two parts: the detection > code, to be merged with the core, and the sensor part, to be merged with > em28xx-v4l, but that sounds ugly. > > Other solution would be to use something like the dvb_attach() macro, > to avoid the recursive dependency, but that would be a dirty solution. Another solution would be to move the sensor probing to em28xx-v4l, too. Then we need to rely on dev->board.is_webcam to load the v4l module. This would of course also require to split the board hint stuff. Even more ugly I think... > As the code there is not big, the amount of overhead of having everything > at em28xx-core is not high. So, I opted to keep the simplest path here, > avoiding the risk of breaking something with a complex changeset. I agree. > >>> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c >>> index 175cd776e0a1..938daaabd8e0 100644 >>> --- a/drivers/media/usb/em28xx/em28xx-cards.c >>> +++ b/drivers/media/usb/em28xx/em28xx-cards.c >>> @@ -2159,6 +2159,8 @@ struct em28xx_board em28xx_boards[] = { >>> .ir_codes = RC_MAP_PINNACLE_PCTV_HD, >>> }, >>> }; >>> +EXPORT_SYMBOL_GPL(em28xx_boards); >>> + >>> const unsigned int em28xx_bcount = ARRAY_SIZE(em28xx_boards); >>> >>> /* table of devices that work with this driver */ >>> @@ -2827,10 +2829,6 @@ static void em28xx_card_setup(struct em28xx *dev) >>> "tuner", dev->tuner_addr, NULL); >>> } >>> } >>> - >>> - em28xx_tuner_setup(dev); >>> - >>> - em28xx_init_camera(dev); >>> } >> Here you are fixing half of the em28xx_card_setup() oopses which you've >> introduced with patch 3/22. >> This needs to be done together with patch 5/22 before patch 3/22. > Ok. > >>> @@ -2848,11 +2846,12 @@ static void request_module_async(struct work_struct *work) >>> em28xx_init_extension(dev); >>> >>> #if defined(CONFIG_MODULES) && defined(MODULE) >>> + if (dev->has_video) >>> + request_module("em28xx-v4l"); >>> if (dev->has_audio_class) >>> request_module("snd-usb-audio"); >>> else if (dev->has_alsa_audio) >>> request_module("em28xx-alsa"); >>> - >>> if (dev->board.has_dvb) >>> request_module("em28xx-dvb"); >>> if (dev->board.buttons || >>> @@ -2881,18 +2880,12 @@ void em28xx_release_resources(struct em28xx *dev) >>> { >>> /*FIXME: I2C IR should be disconnected */ >>> >>> - em28xx_release_analog_resources(dev); >>> - >>> if (dev->def_i2c_bus) >>> em28xx_i2c_unregister(dev, 1); >>> em28xx_i2c_unregister(dev, 0); >>> if (dev->clk) >>> v4l2_clk_unregister_fixed(dev->clk); >>> >>> - v4l2_ctrl_handler_free(&dev->ctrl_handler); >>> - >>> - v4l2_device_unregister(&dev->v4l2_dev); >>> - >>> usb_put_dev(dev->udev); >>> >>> /* Mark device as unused */ >>> @@ -3043,7 +3036,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 */ >>> @@ -3057,30 +3050,14 @@ 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; >>> } >>> } >> Hmm... if registering of bus 1 fails, we need to unregister bus 0. >> But that's an old bug which we can fix later... > Good point. Yes, this should be on a separate patch. > >>> /* Do board specific init and eeprom reading */ >>> em28xx_card_setup(dev); >>> >>> - retval = em28xx_register_analog_devices(dev); >>> - if (retval < 0) { >>> - goto fail; >>> - } >>> - >>> return 0; >>> - >>> -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; >>> } >>> >>> /* high bandwidth multiplier, as encoded in highspeed endpoint descriptors */ >>> @@ -3283,6 +3260,7 @@ static int em28xx_usb_probe(struct usb_interface *interface, >>> dev->alt = -1; >>> dev->is_audio_only = has_audio && !(has_video || has_dvb); >>> dev->has_alsa_audio = has_audio; >>> + dev->has_video = has_video; >>> dev->audio_ifnum = ifnum; >>> >>> /* Checks if audio is provided by some interface */ >>> @@ -3322,10 +3300,9 @@ static int em28xx_usb_probe(struct usb_interface *interface, >>> >>> /* allocate device struct */ >>> mutex_init(&dev->lock); >>> - mutex_lock(&dev->lock); >>> retval = em28xx_init_dev(dev, udev, interface, nr); >>> if (retval) { >>> - goto unlock_and_free; >>> + goto err_free; >>> } >>> >>> if (usb_xfer_mode < 0) { >>> @@ -3368,7 +3345,7 @@ static int em28xx_usb_probe(struct usb_interface *interface, >>> if (retval) { >>> printk(DRIVER_NAME >>> ": Failed to pre-allocate USB transfer buffers for DVB.\n"); >>> - goto unlock_and_free; >>> + goto err_free; >>> } >>> } >>> >>> @@ -3377,13 +3354,9 @@ static int em28xx_usb_probe(struct usb_interface *interface, >>> /* Should be the last thing to do, to avoid newer udev's to >>> open the device before fully initializing it >>> */ >>> - mutex_unlock(&dev->lock); >>> >>> return 0; >>> >>> -unlock_and_free: >>> - mutex_unlock(&dev->lock); >>> - >>> err_free: >>> kfree(dev->alt_max_pkt_size_isoc); >>> kfree(dev); >>> diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c >>> index 3012912d2997..1113d4e107d8 100644 >>> --- a/drivers/media/usb/em28xx/em28xx-core.c >>> +++ b/drivers/media/usb/em28xx/em28xx-core.c >>> @@ -33,6 +33,18 @@ >>> >>> #include "em28xx.h" >>> >>> +#define DRIVER_AUTHOR "Ludovico Cavedon <cavedon@sssup.it>, " \ >>> + "Markus Rechberger <mrechberger@gmail.com>, " \ >>> + "Mauro Carvalho Chehab <mchehab@infradead.org>, " \ >>> + "Sascha Sommer <saschasommer@freenet.de>" >>> + >>> +#define DRIVER_DESC "Empia em28xx based USB core driver" >>> + >>> +MODULE_AUTHOR(DRIVER_AUTHOR); >>> +MODULE_DESCRIPTION(DRIVER_DESC); >>> +MODULE_LICENSE("GPL"); >>> +MODULE_VERSION(EM28XX_VERSION); >>> + >>> /* #define ENABLE_DEBUG_ISOC_FRAMES */ >>> >>> static unsigned int core_debug; >>> diff --git a/drivers/media/usb/em28xx/em28xx-v4l.h b/drivers/media/usb/em28xx/em28xx-v4l.h >>> new file mode 100644 >>> index 000000000000..bce438691e0e >>> --- /dev/null >>> +++ b/drivers/media/usb/em28xx/em28xx-v4l.h >>> @@ -0,0 +1,20 @@ >>> +/* >>> + em28xx-video.c - driver for Empia EM2800/EM2820/2840 USB >>> + video capture devices >> The information about supported chips is outdated everywhere in the driver, >> but if you introduce a new header file it should really be up to date. >> Just talk about EM27xx/EM28xx. > We need to do a cleanup on all those headers: they don't follow Kernel > CodingStyle and the old headers use my previous email. > > For the sake of keeping a coherency, I deliberately opted to just use the > same way as the other headers. > > My plan is to write a patch series fixing this and other CodingStyle > issues on em28xx after merging this changeset. Ok, fine. > >>> + >>> + Copyright (C) 2013-2014 Mauro Carvalho Chehab <m.chehab@samsung.com> >>> + >>> + 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 of the License. >>> + >>> + This program is distributed in the hope that it will be useful, >>> + but WITHOUT ANY WARRANTY; without even the implied warranty of >>> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>> + GNU General Public License for more details. >>> + */ >>> + >>> + >>> +int em28xx_start_analog_streaming(struct vb2_queue *vq, unsigned int count); >>> +int em28xx_stop_vbi_streaming(struct vb2_queue *vq); >>> +extern struct vb2_ops em28xx_vbi_qops; >>> diff --git a/drivers/media/usb/em28xx/em28xx-vbi.c b/drivers/media/usb/em28xx/em28xx-vbi.c >>> index 39f39c527c13..db3d655600df 100644 >>> --- a/drivers/media/usb/em28xx/em28xx-vbi.c >>> +++ b/drivers/media/usb/em28xx/em28xx-vbi.c >>> @@ -27,6 +27,7 @@ >>> #include <linux/init.h> >>> >>> #include "em28xx.h" >>> +#include "em28xx-v4l.h" >>> >>> static unsigned int vbibufs = 5; >>> module_param(vbibufs, int, 0644); >>> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c >>> index 85284626dbd6..d615bff8ac09 100644 >>> --- a/drivers/media/usb/em28xx/em28xx-video.c >>> +++ b/drivers/media/usb/em28xx/em28xx-video.c >>> @@ -38,6 +38,7 @@ >>> #include <linux/slab.h> >>> >>> #include "em28xx.h" >>> +#include "em28xx-v4l.h" >>> #include <media/v4l2-common.h> >>> #include <media/v4l2-ioctl.h> >>> #include <media/v4l2-event.h> >>> @@ -141,11 +142,13 @@ static struct em28xx_fmt format[] = { >>> }, >>> }; >>> >>> +static int em28xx_vbi_supported(struct em28xx *dev); >>> + >> Or move em28xx_vbi_supported() before em28xx_set_outfmt() and >> em28xx_resolution_set() again. >> If you had not changed the functions order in patch 1/22, this wouldn't >> be necessary. > True. I'll put an extra cleanup patch to remove this reference on the > CodingStyle cleanup patchset I'm planning to do. > >>> /* >>> * em28xx_wake_i2c() >>> * configure i2c attached devices >>> */ >>> -void em28xx_wake_i2c(struct em28xx *dev) >>> +static void em28xx_wake_i2c(struct em28xx *dev) >>> { >>> v4l2_device_call_all(&dev->v4l2_dev, 0, core, reset, 0); >>> v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_routing, >>> @@ -153,7 +156,7 @@ void em28xx_wake_i2c(struct em28xx *dev) >>> v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_stream, 0); >>> } >>> >>> -int em28xx_colorlevels_set_default(struct em28xx *dev) >>> +static int em28xx_colorlevels_set_default(struct em28xx *dev) >>> { >>> em28xx_write_reg(dev, EM28XX_R20_YGAIN, CONTRAST_DEFAULT); >>> em28xx_write_reg(dev, EM28XX_R21_YOFFSET, BRIGHTNESS_DEFAULT); >>> @@ -171,7 +174,7 @@ int em28xx_colorlevels_set_default(struct em28xx *dev) >>> return em28xx_write_reg(dev, EM28XX_R1A_BOFFSET, 0x00); >>> } >>> >>> -int em28xx_set_outfmt(struct em28xx *dev) >>> +static int em28xx_set_outfmt(struct em28xx *dev) >>> { >>> int ret; >>> u8 fmt, vinctrl; >>> @@ -278,7 +281,7 @@ static int em28xx_scaler_set(struct em28xx *dev, u16 h, u16 v) >>> } >>> >>> /* FIXME: this only function read values from dev */ >>> -int em28xx_resolution_set(struct em28xx *dev) >>> +static int em28xx_resolution_set(struct em28xx *dev) >>> { >>> int width, height; >>> width = norm_maxw(dev); >>> @@ -310,7 +313,7 @@ int em28xx_resolution_set(struct em28xx *dev) >>> return em28xx_scaler_set(dev, dev->hscale, dev->vscale); >>> } >>> >>> -int em28xx_vbi_supported(struct em28xx *dev) >>> +static int em28xx_vbi_supported(struct em28xx *dev) >>> { >>> /* Modprobe option to manually disable */ >>> if (disable_vbi == 1) >>> @@ -330,7 +333,7 @@ int em28xx_vbi_supported(struct em28xx *dev) >>> } >>> >>> /* Set USB alternate setting for analog video */ >>> -int em28xx_set_alternate(struct em28xx *dev) >>> +static int em28xx_set_alternate(struct em28xx *dev) >>> { >>> int errCode; >>> int i; >>> @@ -1020,7 +1023,7 @@ static struct vb2_ops em28xx_video_qops = { >>> .wait_finish = vb2_ops_wait_finish, >>> }; >>> >>> -int em28xx_vb2_setup(struct em28xx *dev) >>> +static int em28xx_vb2_setup(struct em28xx *dev) >>> { >>> int rc; >>> struct vb2_queue *q; >>> @@ -1088,7 +1091,7 @@ static void video_mux(struct em28xx *dev, int index) >>> em28xx_audio_analog_set(dev); >>> } >>> >>> -void em28xx_ctrl_notify(struct v4l2_ctrl *ctrl, void *priv) >>> +static void em28xx_ctrl_notify(struct v4l2_ctrl *ctrl, void *priv) >>> { >>> struct em28xx *dev = priv; >>> >>> @@ -1625,7 +1628,7 @@ static int vidioc_g_register(struct file *file, void *priv, >>> reg->val = ret; >>> } else { >>> __le16 val = 0; >>> - ret = em28xx_read_reg_req_len(dev, USB_REQ_GET_STATUS, >>> + ret = dev->em28xx_read_reg_req_len(dev, USB_REQ_GET_STATUS, >>> reg->reg, (char *)&val, 2); >> Urgh... is it really necessary to start using these pointers again ? >> Ok... keep it as is, I'll send a patch to clean this up later. > This is one of the few places where we weren't using those pointers: > > $ git grep em28xx_read_reg_req_len drivers/media/usb/em28xx/ > drivers/media/usb/em28xx/em28xx-cards.c: dev->em28xx_read_reg_req_len = em28xx_read_reg_req_len; > drivers/media/usb/em28xx/em28xx-core.c:int em28xx_read_reg_req_len(struct em28xx *dev, u8 req, u16 reg, > drivers/media/usb/em28xx/em28xx-core.c: ret = em28xx_read_reg_req_len(dev, req, reg, &val, 1); > drivers/media/usb/em28xx/em28xx-core.c: ret = dev->em28xx_read_reg_req_len(dev, 0, EM28XX_R40_AC97LSB, > drivers/media/usb/em28xx/em28xx-i2c.c: ret = dev->em28xx_read_reg_req_len(dev, 0x00, 4-len, buf2, len); > drivers/media/usb/em28xx/em28xx-i2c.c: ret = dev->em28xx_read_reg_req_len(dev, 2, addr, buf, len); > drivers/media/usb/em28xx/em28xx-i2c.c: ret = dev->em28xx_read_reg_req_len(dev, 0x06, addr, buf, len); > drivers/media/usb/em28xx/em28xx-input.c: rc = dev->em28xx_read_reg_req_len(dev, 0, EM28XX_R45_IR, > drivers/media/usb/em28xx/em28xx-input.c: rc = dev->em28xx_read_reg_req_len(dev, 0, EM2874_R51_IR, > drivers/media/usb/em28xx/em28xx-video.c: ret = dev->em28xx_read_reg_req_len(dev, USB_REQ_GET_STATUS, > drivers/media/usb/em28xx/em28xx.h: int (*em28xx_read_reg_req_len) (struct em28xx *dev, u8 req, u16 reg, > drivers/media/usb/em28xx/em28xx.h:int em28xx_read_reg_req_len(struct em28xx *dev, u8 req, u16 reg, It seems like your "grep" works better than mine... ;) > For the sake of coherency, we should either use one or the other way. Agreed. > >>> if (ret < 0) >>> return ret; >>> @@ -1872,11 +1875,11 @@ static int em28xx_v4l2_open(struct file *filp) >>> } >>> >>> /* >>> - * em28xx_realease_resources() >>> + * em28xx_v4l2_fini() >>> * unregisters the v4l2,i2c and usb devices >>> * called when the device gets disconected or at module unload >>> */ >>> -void em28xx_release_analog_resources(struct em28xx *dev) >>> +static int em28xx_v4l2_fini(struct em28xx *dev) >>> { >>> >>> /*FIXME: I2C IR should be disconnected */ >>> @@ -1906,6 +1909,8 @@ void em28xx_release_analog_resources(struct em28xx *dev) >>> video_device_release(dev->vdev); >>> dev->vdev = NULL; >>> } >>> + >>> + return 0; >>> } >>> >>> /* >>> @@ -1927,12 +1932,12 @@ static int em28xx_v4l2_close(struct file *filp) >>> if (dev->users == 1) { >>> /* the device is already disconnect, >>> free the remaining resources */ >>> + >>> if (dev->disconnected) { >>> - em28xx_release_resources(dev); >> Who releases the resources now ? > This is tricky. It is. And this part of the code really looks fishy. > The hole idea is that em28xx-v4l release the resources allocated for V4L > only, and not all resources, the same way as the other em28xx modules > do. > > If you take a look at the original em28xx_release_resources(), what it does > is to call this function, and then to de-allocate and unregister v4l2: > > v4l2_ctrl_handler_free(&dev->ctrl_handler); > v4l2_device_unregister(&dev->v4l2_dev); > > after that, it deallocates the rest of allocated data. > > Now, em28xx_release_resources() is only called when em28xx is removed > from memory or when the device is removed. > > The code there first calls em28xx_close_extension(dev). So, at device > removal, em28xx-v4l will release the V4L2 specific resources, and > em28xx_release_resources() will drop the common dev struct. Up to now, em28xx_release_resources() was called after the last close. If you remove this call, the resources are never released. Or am I missing another place where this function is called ? > I suspect that it might still be a bug there, but, if so, this bug is > also present on em28xx-dvb, em28xx-alsa and em28xx-rc: what happens if > the device is removed while the file descriptors is still opened? At least it doesn't produce any oopses with the old code. I'm not sure if I checked after the core/v4l split... Did you observe an problems when you keep the em28xx_release_resources() call in em28xx_v4l_fini() ? I agree that the whole thing smells like bugs and close() probably isn't the correct place for this. But never calling this function is definitely wrong. > Maybe the driver core already prevent such things, but I'm not sure. Hmm... we currently close the extensions unconditionally in em28xx_usb_disconnect()... That seems to avoid trouble. > If there's a bug out there, the proper fix seems to use kref for > struct em28xx, increasing refcount for every em28xx extension and/or > file open, decreasing it at either extension fini call, or at file close. > > This way, em28xx_release_resources() would be called only after refcount > reaching zero, e. g. after being sure that nobody is using it. > > My plan is to take a look on it after having this changeset merged, > as such change, if needed, will be complex and would require lots of > testing. Also, it is independent on those changes. Yes, let's fix things step by step and not all at the same time. > >>> + v4l2_ctrl_handler_free(&dev->ctrl_handler); >>> + v4l2_device_unregister(&dev->v4l2_dev); >>> kfree(dev->alt_max_pkt_size_isoc); >>> - mutex_unlock(&dev->lock); >>> - kfree(dev); >>> - return 0; >>> + goto exit; >>> } >>> >>> /* Save some power by putting tuner to sleep */ >>> @@ -1951,6 +1956,7 @@ static int em28xx_v4l2_close(struct file *filp) >>> } >>> } >>> >>> +exit: >>> dev->users--; >> Nice bugfix. >> >>> mutex_unlock(&dev->lock); >>> return 0; >>> @@ -2047,8 +2053,6 @@ static struct video_device em28xx_radio_template = { >>> >>> /******************************** usb interface ******************************/ >>> >>> - >>> - >>> static struct video_device *em28xx_vdev_init(struct em28xx *dev, >>> const struct video_device *template, >>> const char *type_name) >>> @@ -2122,7 +2126,7 @@ static void em28xx_setup_xc3028(struct em28xx *dev, struct xc2028_ctrl *ctl) >>> } >>> } >>> >>> -void em28xx_tuner_setup(struct em28xx *dev) >>> +static void em28xx_tuner_setup(struct em28xx *dev) >>> { >>> struct tuner_setup tun_setup; >>> struct v4l2_frequency f; >>> @@ -2181,14 +2185,14 @@ void em28xx_tuner_setup(struct em28xx *dev) >>> v4l2_device_call_all(&dev->v4l2_dev, 0, tuner, s_frequency, &f); >>> } >>> >>> -int em28xx_register_analog_devices(struct em28xx *dev) >>> +static int em28xx_v4l2_init(struct em28xx *dev) >>> { >>> u8 val; >>> int ret; >>> unsigned int maxw; >>> struct v4l2_ctrl_handler *hdl = &dev->ctrl_handler; >>> >>> - if (!dev->is_audio_only) { >>> + if (!dev->has_video) { >>> /* This device does not support the v4l2 extension */ >>> return 0; >>> } >>> @@ -2196,6 +2200,8 @@ int em28xx_register_analog_devices(struct em28xx *dev) >>> printk(KERN_INFO "%s: v4l2 driver version %s\n", >>> dev->name, EM28XX_VERSION); >>> >>> + mutex_lock(&dev->lock); >>> + >>> ret = v4l2_device_register(&dev->udev->dev, &dev->v4l2_dev); >>> if (ret < 0) { >>> em28xx_errdev("Call to v4l2_device_register() failed!\n"); >>> @@ -2212,6 +2218,10 @@ int em28xx_register_analog_devices(struct em28xx *dev) >>> dev->vinctl = EM28XX_VINCTRL_INTERLACED | >>> EM28XX_VINCTRL_CCIR656_ENABLE; >>> >>> + /* Initialize tuner and camera */ >>> + em28xx_tuner_setup(dev); >>> + em28xx_init_camera(dev); >>> + >>> /* Configure audio */ >>> ret = em28xx_audio_setup(dev); >>> if (ret < 0) { >>> @@ -2422,6 +2432,28 @@ int em28xx_register_analog_devices(struct em28xx *dev) >>> >>> /* initialize videobuf2 stuff */ >>> em28xx_vb2_setup(dev); >>> + >>> err: >>> - return 0; >>> + mutex_unlock(&dev->lock); >>> + return ret; >>> +} >>> + >>> +static struct em28xx_ops v4l2_ops = { >>> + .id = EM28XX_V4L2, >>> + .name = "Em28xx v4l2 Extension", >>> + .init = em28xx_v4l2_init, >>> + .fini = em28xx_v4l2_fini, >>> +}; >>> + >>> +static int __init em28xx_video_register(void) >>> +{ >>> + return em28xx_register_extension(&v4l2_ops); >>> } >>> + >>> +static void __exit em28xx_video_unregister(void) >>> +{ >>> + em28xx_unregister_extension(&v4l2_ops); >>> +} >>> + >>> +module_init(em28xx_video_register); >>> +module_exit(em28xx_video_unregister); >>> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h >>> index 7ae05ebc13c1..9d6f43e4681f 100644 >>> --- a/drivers/media/usb/em28xx/em28xx.h >>> +++ b/drivers/media/usb/em28xx/em28xx.h >>> @@ -26,7 +26,7 @@ >>> #ifndef _EM28XX_H >>> #define _EM28XX_H >>> >>> -#define EM28XX_VERSION "0.2.0" >>> +#define EM28XX_VERSION "0.2.1" >>> >>> #include <linux/workqueue.h> >>> #include <linux/i2c.h> >>> @@ -474,6 +474,7 @@ struct em28xx_eeprom { >>> #define EM28XX_AUDIO 0x10 >>> #define EM28XX_DVB 0x20 >>> #define EM28XX_RC 0x30 >>> +#define EM28XX_V4L2 0x40 >>> >>> /* em28xx resource types (used for res_get/res_lock etc */ >>> #define EM28XX_RESOURCE_VIDEO 0x01 >>> @@ -527,6 +528,7 @@ struct em28xx { >>> >>> unsigned int is_em25xx:1; /* em25xx/em276x/7x/8x family bridge */ >>> unsigned char disconnected:1; /* device has been diconnected */ >>> + unsigned int has_video:1; >>> unsigned int has_audio_class:1; >>> unsigned int has_alsa_audio:1; >>> unsigned int is_audio_only:1; >>> @@ -723,14 +725,9 @@ int em28xx_write_ac97(struct em28xx *dev, u8 reg, u16 val); >>> int em28xx_audio_analog_set(struct em28xx *dev); >>> int em28xx_audio_setup(struct em28xx *dev); >>> >>> -int em28xx_colorlevels_set_default(struct em28xx *dev); >>> const struct em28xx_led *em28xx_find_led(struct em28xx *dev, >>> enum em28xx_led_role role); >>> int em28xx_capture_start(struct em28xx *dev, int start); >>> -int em28xx_vbi_supported(struct em28xx *dev); >>> -int em28xx_set_outfmt(struct em28xx *dev); >>> -int em28xx_resolution_set(struct em28xx *dev); >>> -int em28xx_set_alternate(struct em28xx *dev); >>> int em28xx_alloc_urbs(struct em28xx *dev, enum em28xx_mode mode, int xfer_bulk, >>> int num_bufs, int max_pkt_size, int packet_multiplier); >>> int em28xx_init_usb_xfer(struct em28xx *dev, enum em28xx_mode mode, >>> @@ -742,31 +739,17 @@ void em28xx_uninit_usb_xfer(struct em28xx *dev, enum em28xx_mode mode); >>> void em28xx_stop_urbs(struct em28xx *dev); >>> int em28xx_set_mode(struct em28xx *dev, enum em28xx_mode set_mode); >>> int em28xx_gpio_set(struct em28xx *dev, struct em28xx_reg_seq *gpio); >>> -void em28xx_wake_i2c(struct em28xx *dev); >>> int em28xx_register_extension(struct em28xx_ops *dev); >>> void em28xx_unregister_extension(struct em28xx_ops *dev); >>> void em28xx_init_extension(struct em28xx *dev); >>> void em28xx_close_extension(struct em28xx *dev); >>> >>> -/* Provided by em28xx-video.c */ >>> -void em28xx_tuner_setup(struct em28xx *dev); >>> -int em28xx_vb2_setup(struct em28xx *dev); >>> -int em28xx_register_analog_devices(struct em28xx *dev); >>> -void em28xx_release_analog_resources(struct em28xx *dev); >>> -void em28xx_ctrl_notify(struct v4l2_ctrl *ctrl, void *priv); >>> -int em28xx_start_analog_streaming(struct vb2_queue *vq, unsigned int count); >>> -int em28xx_stop_vbi_streaming(struct vb2_queue *vq); >>> -extern const struct v4l2_ctrl_ops em28xx_ctrl_ops; >>> - >>> /* Provided by em28xx-cards.c */ >>> extern struct em28xx_board em28xx_boards[]; >>> extern struct usb_device_id em28xx_id_table[]; >>> int em28xx_tuner_callback(void *ptr, int component, int command, int arg); >>> void em28xx_release_resources(struct em28xx *dev); >>> >>> -/* Provided by em28xx-vbi.c */ >>> -extern struct vb2_ops em28xx_vbi_qops; >>> - >>> /* Provided by em28xx-camera.c */ >>> int em28xx_detect_sensor(struct em28xx *dev); >>> int em28xx_init_camera(struct em28xx *dev); >> Nice clean-up ! :) > Thanks! > -- 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 --git a/drivers/media/usb/em28xx/Kconfig b/drivers/media/usb/em28xx/Kconfig index d6ba514d31eb..838fc9dbb747 100644 --- a/drivers/media/usb/em28xx/Kconfig +++ b/drivers/media/usb/em28xx/Kconfig @@ -1,8 +1,12 @@ config VIDEO_EM28XX - tristate "Empia EM28xx USB video capture support" + tristate "Empia EM28xx USB devices support" depends on VIDEO_DEV && I2C select VIDEO_TUNER select VIDEO_TVEEPROM + +config VIDEO_EM28XX_V4L2 + tristate "Empia EM28xx analog TV, video capture and/or webcam support" + depends on VIDEO_EM28XX && I2C select VIDEOBUF2_VMALLOC select VIDEO_SAA711X if MEDIA_SUBDRV_AUTOSELECT select VIDEO_TVP5150 if MEDIA_SUBDRV_AUTOSELECT diff --git a/drivers/media/usb/em28xx/Makefile b/drivers/media/usb/em28xx/Makefile index ad6d48557940..3f850d5063d0 100644 --- a/drivers/media/usb/em28xx/Makefile +++ b/drivers/media/usb/em28xx/Makefile @@ -1,10 +1,11 @@ -em28xx-y += em28xx-video.o em28xx-i2c.o em28xx-cards.o -em28xx-y += em28xx-core.o em28xx-vbi.o em28xx-camera.o +em28xx-y += em28xx-core.o em28xx-i2c.o em28xx-cards.o em28xx-camera.o +em28xx-v4l-objs := em28xx-video.o em28xx-vbi.o em28xx-alsa-objs := em28xx-audio.o em28xx-rc-objs := em28xx-input.o obj-$(CONFIG_VIDEO_EM28XX) += em28xx.o +obj-$(CONFIG_VIDEO_EM28XX_V4L2) += em28xx-v4l.o obj-$(CONFIG_VIDEO_EM28XX_ALSA) += em28xx-alsa.o obj-$(CONFIG_VIDEO_EM28XX_DVB) += em28xx-dvb.o obj-$(CONFIG_VIDEO_EM28XX_RC) += em28xx-rc.o diff --git a/drivers/media/usb/em28xx/em28xx-camera.c b/drivers/media/usb/em28xx/em28xx-camera.c index d666741797d4..c29f5c4e7b40 100644 --- a/drivers/media/usb/em28xx/em28xx-camera.c +++ b/drivers/media/usb/em28xx/em28xx-camera.c @@ -454,3 +454,4 @@ int em28xx_init_camera(struct em28xx *dev) return ret; } +EXPORT_SYMBOL_GPL(em28xx_init_camera); diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c index 175cd776e0a1..938daaabd8e0 100644 --- a/drivers/media/usb/em28xx/em28xx-cards.c +++ b/drivers/media/usb/em28xx/em28xx-cards.c @@ -2159,6 +2159,8 @@ struct em28xx_board em28xx_boards[] = { .ir_codes = RC_MAP_PINNACLE_PCTV_HD, }, }; +EXPORT_SYMBOL_GPL(em28xx_boards); + const unsigned int em28xx_bcount = ARRAY_SIZE(em28xx_boards); /* table of devices that work with this driver */ @@ -2827,10 +2829,6 @@ static void em28xx_card_setup(struct em28xx *dev) "tuner", dev->tuner_addr, NULL); } } - - em28xx_tuner_setup(dev); - - em28xx_init_camera(dev); } @@ -2848,11 +2846,12 @@ static void request_module_async(struct work_struct *work) em28xx_init_extension(dev); #if defined(CONFIG_MODULES) && defined(MODULE) + if (dev->has_video) + request_module("em28xx-v4l"); if (dev->has_audio_class) request_module("snd-usb-audio"); else if (dev->has_alsa_audio) request_module("em28xx-alsa"); - if (dev->board.has_dvb) request_module("em28xx-dvb"); if (dev->board.buttons || @@ -2881,18 +2880,12 @@ void em28xx_release_resources(struct em28xx *dev) { /*FIXME: I2C IR should be disconnected */ - em28xx_release_analog_resources(dev); - if (dev->def_i2c_bus) em28xx_i2c_unregister(dev, 1); em28xx_i2c_unregister(dev, 0); if (dev->clk) v4l2_clk_unregister_fixed(dev->clk); - v4l2_ctrl_handler_free(&dev->ctrl_handler); - - v4l2_device_unregister(&dev->v4l2_dev); - usb_put_dev(dev->udev); /* Mark device as unused */ @@ -3043,7 +3036,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 */ @@ -3057,30 +3050,14 @@ 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; } } /* Do board specific init and eeprom reading */ em28xx_card_setup(dev); - retval = em28xx_register_analog_devices(dev); - if (retval < 0) { - goto fail; - } - return 0; - -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; } /* high bandwidth multiplier, as encoded in highspeed endpoint descriptors */ @@ -3283,6 +3260,7 @@ static int em28xx_usb_probe(struct usb_interface *interface, dev->alt = -1; dev->is_audio_only = has_audio && !(has_video || has_dvb); dev->has_alsa_audio = has_audio; + dev->has_video = has_video; dev->audio_ifnum = ifnum; /* Checks if audio is provided by some interface */ @@ -3322,10 +3300,9 @@ static int em28xx_usb_probe(struct usb_interface *interface, /* allocate device struct */ mutex_init(&dev->lock); - mutex_lock(&dev->lock); retval = em28xx_init_dev(dev, udev, interface, nr); if (retval) { - goto unlock_and_free; + goto err_free; } if (usb_xfer_mode < 0) { @@ -3368,7 +3345,7 @@ static int em28xx_usb_probe(struct usb_interface *interface, if (retval) { printk(DRIVER_NAME ": Failed to pre-allocate USB transfer buffers for DVB.\n"); - goto unlock_and_free; + goto err_free; } } @@ -3377,13 +3354,9 @@ static int em28xx_usb_probe(struct usb_interface *interface, /* Should be the last thing to do, to avoid newer udev's to open the device before fully initializing it */ - mutex_unlock(&dev->lock); return 0; -unlock_and_free: - mutex_unlock(&dev->lock); - err_free: kfree(dev->alt_max_pkt_size_isoc); kfree(dev); diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c index 3012912d2997..1113d4e107d8 100644 --- a/drivers/media/usb/em28xx/em28xx-core.c +++ b/drivers/media/usb/em28xx/em28xx-core.c @@ -33,6 +33,18 @@ #include "em28xx.h" +#define DRIVER_AUTHOR "Ludovico Cavedon <cavedon@sssup.it>, " \ + "Markus Rechberger <mrechberger@gmail.com>, " \ + "Mauro Carvalho Chehab <mchehab@infradead.org>, " \ + "Sascha Sommer <saschasommer@freenet.de>" + +#define DRIVER_DESC "Empia em28xx based USB core driver" + +MODULE_AUTHOR(DRIVER_AUTHOR); +MODULE_DESCRIPTION(DRIVER_DESC); +MODULE_LICENSE("GPL"); +MODULE_VERSION(EM28XX_VERSION); + /* #define ENABLE_DEBUG_ISOC_FRAMES */ static unsigned int core_debug; diff --git a/drivers/media/usb/em28xx/em28xx-v4l.h b/drivers/media/usb/em28xx/em28xx-v4l.h new file mode 100644 index 000000000000..bce438691e0e --- /dev/null +++ b/drivers/media/usb/em28xx/em28xx-v4l.h @@ -0,0 +1,20 @@ +/* + em28xx-video.c - driver for Empia EM2800/EM2820/2840 USB + video capture devices + + Copyright (C) 2013-2014 Mauro Carvalho Chehab <m.chehab@samsung.com> + + 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 of the License. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + */ + + +int em28xx_start_analog_streaming(struct vb2_queue *vq, unsigned int count); +int em28xx_stop_vbi_streaming(struct vb2_queue *vq); +extern struct vb2_ops em28xx_vbi_qops; diff --git a/drivers/media/usb/em28xx/em28xx-vbi.c b/drivers/media/usb/em28xx/em28xx-vbi.c index 39f39c527c13..db3d655600df 100644 --- a/drivers/media/usb/em28xx/em28xx-vbi.c +++ b/drivers/media/usb/em28xx/em28xx-vbi.c @@ -27,6 +27,7 @@ #include <linux/init.h> #include "em28xx.h" +#include "em28xx-v4l.h" static unsigned int vbibufs = 5; module_param(vbibufs, int, 0644); diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c index 85284626dbd6..d615bff8ac09 100644 --- a/drivers/media/usb/em28xx/em28xx-video.c +++ b/drivers/media/usb/em28xx/em28xx-video.c @@ -38,6 +38,7 @@ #include <linux/slab.h> #include "em28xx.h" +#include "em28xx-v4l.h" #include <media/v4l2-common.h> #include <media/v4l2-ioctl.h> #include <media/v4l2-event.h> @@ -141,11 +142,13 @@ static struct em28xx_fmt format[] = { }, }; +static int em28xx_vbi_supported(struct em28xx *dev); + /* * em28xx_wake_i2c() * configure i2c attached devices */ -void em28xx_wake_i2c(struct em28xx *dev) +static void em28xx_wake_i2c(struct em28xx *dev) { v4l2_device_call_all(&dev->v4l2_dev, 0, core, reset, 0); v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_routing, @@ -153,7 +156,7 @@ void em28xx_wake_i2c(struct em28xx *dev) v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_stream, 0); } -int em28xx_colorlevels_set_default(struct em28xx *dev) +static int em28xx_colorlevels_set_default(struct em28xx *dev) { em28xx_write_reg(dev, EM28XX_R20_YGAIN, CONTRAST_DEFAULT); em28xx_write_reg(dev, EM28XX_R21_YOFFSET, BRIGHTNESS_DEFAULT); @@ -171,7 +174,7 @@ int em28xx_colorlevels_set_default(struct em28xx *dev) return em28xx_write_reg(dev, EM28XX_R1A_BOFFSET, 0x00); } -int em28xx_set_outfmt(struct em28xx *dev) +static int em28xx_set_outfmt(struct em28xx *dev) { int ret; u8 fmt, vinctrl; @@ -278,7 +281,7 @@ static int em28xx_scaler_set(struct em28xx *dev, u16 h, u16 v) } /* FIXME: this only function read values from dev */ -int em28xx_resolution_set(struct em28xx *dev) +static int em28xx_resolution_set(struct em28xx *dev) { int width, height; width = norm_maxw(dev); @@ -310,7 +313,7 @@ int em28xx_resolution_set(struct em28xx *dev) return em28xx_scaler_set(dev, dev->hscale, dev->vscale); } -int em28xx_vbi_supported(struct em28xx *dev) +static int em28xx_vbi_supported(struct em28xx *dev) { /* Modprobe option to manually disable */ if (disable_vbi == 1) @@ -330,7 +333,7 @@ int em28xx_vbi_supported(struct em28xx *dev) } /* Set USB alternate setting for analog video */ -int em28xx_set_alternate(struct em28xx *dev) +static int em28xx_set_alternate(struct em28xx *dev) { int errCode; int i; @@ -1020,7 +1023,7 @@ static struct vb2_ops em28xx_video_qops = { .wait_finish = vb2_ops_wait_finish, }; -int em28xx_vb2_setup(struct em28xx *dev) +static int em28xx_vb2_setup(struct em28xx *dev) { int rc; struct vb2_queue *q; @@ -1088,7 +1091,7 @@ static void video_mux(struct em28xx *dev, int index) em28xx_audio_analog_set(dev); } -void em28xx_ctrl_notify(struct v4l2_ctrl *ctrl, void *priv) +static void em28xx_ctrl_notify(struct v4l2_ctrl *ctrl, void *priv) { struct em28xx *dev = priv; @@ -1625,7 +1628,7 @@ static int vidioc_g_register(struct file *file, void *priv, reg->val = ret; } else { __le16 val = 0; - ret = em28xx_read_reg_req_len(dev, USB_REQ_GET_STATUS, + ret = dev->em28xx_read_reg_req_len(dev, USB_REQ_GET_STATUS, reg->reg, (char *)&val, 2); if (ret < 0) return ret; @@ -1872,11 +1875,11 @@ static int em28xx_v4l2_open(struct file *filp) } /* - * em28xx_realease_resources() + * em28xx_v4l2_fini() * unregisters the v4l2,i2c and usb devices * called when the device gets disconected or at module unload */ -void em28xx_release_analog_resources(struct em28xx *dev) +static int em28xx_v4l2_fini(struct em28xx *dev) { /*FIXME: I2C IR should be disconnected */ @@ -1906,6 +1909,8 @@ void em28xx_release_analog_resources(struct em28xx *dev) video_device_release(dev->vdev); dev->vdev = NULL; } + + return 0; } /* @@ -1927,12 +1932,12 @@ static int em28xx_v4l2_close(struct file *filp) if (dev->users == 1) { /* the device is already disconnect, free the remaining resources */ + if (dev->disconnected) { - em28xx_release_resources(dev); + v4l2_ctrl_handler_free(&dev->ctrl_handler); + v4l2_device_unregister(&dev->v4l2_dev); kfree(dev->alt_max_pkt_size_isoc); - mutex_unlock(&dev->lock); - kfree(dev); - return 0; + goto exit; } /* Save some power by putting tuner to sleep */ @@ -1951,6 +1956,7 @@ static int em28xx_v4l2_close(struct file *filp) } } +exit: dev->users--; mutex_unlock(&dev->lock); return 0; @@ -2047,8 +2053,6 @@ static struct video_device em28xx_radio_template = { /******************************** usb interface ******************************/ - - static struct video_device *em28xx_vdev_init(struct em28xx *dev, const struct video_device *template, const char *type_name) @@ -2122,7 +2126,7 @@ static void em28xx_setup_xc3028(struct em28xx *dev, struct xc2028_ctrl *ctl) } } -void em28xx_tuner_setup(struct em28xx *dev) +static void em28xx_tuner_setup(struct em28xx *dev) { struct tuner_setup tun_setup; struct v4l2_frequency f; @@ -2181,14 +2185,14 @@ void em28xx_tuner_setup(struct em28xx *dev) v4l2_device_call_all(&dev->v4l2_dev, 0, tuner, s_frequency, &f); } -int em28xx_register_analog_devices(struct em28xx *dev) +static int em28xx_v4l2_init(struct em28xx *dev) { u8 val; int ret; unsigned int maxw; struct v4l2_ctrl_handler *hdl = &dev->ctrl_handler; - if (!dev->is_audio_only) { + if (!dev->has_video) { /* This device does not support the v4l2 extension */ return 0; } @@ -2196,6 +2200,8 @@ int em28xx_register_analog_devices(struct em28xx *dev) printk(KERN_INFO "%s: v4l2 driver version %s\n", dev->name, EM28XX_VERSION); + mutex_lock(&dev->lock); + ret = v4l2_device_register(&dev->udev->dev, &dev->v4l2_dev); if (ret < 0) { em28xx_errdev("Call to v4l2_device_register() failed!\n"); @@ -2212,6 +2218,10 @@ int em28xx_register_analog_devices(struct em28xx *dev) dev->vinctl = EM28XX_VINCTRL_INTERLACED | EM28XX_VINCTRL_CCIR656_ENABLE; + /* Initialize tuner and camera */ + em28xx_tuner_setup(dev); + em28xx_init_camera(dev); + /* Configure audio */ ret = em28xx_audio_setup(dev); if (ret < 0) { @@ -2422,6 +2432,28 @@ int em28xx_register_analog_devices(struct em28xx *dev) /* initialize videobuf2 stuff */ em28xx_vb2_setup(dev); + err: - return 0; + mutex_unlock(&dev->lock); + return ret; +} + +static struct em28xx_ops v4l2_ops = { + .id = EM28XX_V4L2, + .name = "Em28xx v4l2 Extension", + .init = em28xx_v4l2_init, + .fini = em28xx_v4l2_fini, +}; + +static int __init em28xx_video_register(void) +{ + return em28xx_register_extension(&v4l2_ops); } + +static void __exit em28xx_video_unregister(void) +{ + em28xx_unregister_extension(&v4l2_ops); +} + +module_init(em28xx_video_register); +module_exit(em28xx_video_unregister); diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h index 7ae05ebc13c1..9d6f43e4681f 100644 --- a/drivers/media/usb/em28xx/em28xx.h +++ b/drivers/media/usb/em28xx/em28xx.h @@ -26,7 +26,7 @@ #ifndef _EM28XX_H #define _EM28XX_H -#define EM28XX_VERSION "0.2.0" +#define EM28XX_VERSION "0.2.1" #include <linux/workqueue.h> #include <linux/i2c.h> @@ -474,6 +474,7 @@ struct em28xx_eeprom { #define EM28XX_AUDIO 0x10 #define EM28XX_DVB 0x20 #define EM28XX_RC 0x30 +#define EM28XX_V4L2 0x40 /* em28xx resource types (used for res_get/res_lock etc */ #define EM28XX_RESOURCE_VIDEO 0x01 @@ -527,6 +528,7 @@ struct em28xx { unsigned int is_em25xx:1; /* em25xx/em276x/7x/8x family bridge */ unsigned char disconnected:1; /* device has been diconnected */ + unsigned int has_video:1; unsigned int has_audio_class:1; unsigned int has_alsa_audio:1; unsigned int is_audio_only:1; @@ -723,14 +725,9 @@ int em28xx_write_ac97(struct em28xx *dev, u8 reg, u16 val); int em28xx_audio_analog_set(struct em28xx *dev); int em28xx_audio_setup(struct em28xx *dev); -int em28xx_colorlevels_set_default(struct em28xx *dev); const struct em28xx_led *em28xx_find_led(struct em28xx *dev, enum em28xx_led_role role); int em28xx_capture_start(struct em28xx *dev, int start); -int em28xx_vbi_supported(struct em28xx *dev); -int em28xx_set_outfmt(struct em28xx *dev); -int em28xx_resolution_set(struct em28xx *dev); -int em28xx_set_alternate(struct em28xx *dev); int em28xx_alloc_urbs(struct em28xx *dev, enum em28xx_mode mode, int xfer_bulk, int num_bufs, int max_pkt_size, int packet_multiplier); int em28xx_init_usb_xfer(struct em28xx *dev, enum em28xx_mode mode, @@ -742,31 +739,17 @@ void em28xx_uninit_usb_xfer(struct em28xx *dev, enum em28xx_mode mode); void em28xx_stop_urbs(struct em28xx *dev); int em28xx_set_mode(struct em28xx *dev, enum em28xx_mode set_mode); int em28xx_gpio_set(struct em28xx *dev, struct em28xx_reg_seq *gpio); -void em28xx_wake_i2c(struct em28xx *dev); int em28xx_register_extension(struct em28xx_ops *dev); void em28xx_unregister_extension(struct em28xx_ops *dev); void em28xx_init_extension(struct em28xx *dev); void em28xx_close_extension(struct em28xx *dev); -/* Provided by em28xx-video.c */ -void em28xx_tuner_setup(struct em28xx *dev); -int em28xx_vb2_setup(struct em28xx *dev); -int em28xx_register_analog_devices(struct em28xx *dev); -void em28xx_release_analog_resources(struct em28xx *dev); -void em28xx_ctrl_notify(struct v4l2_ctrl *ctrl, void *priv); -int em28xx_start_analog_streaming(struct vb2_queue *vq, unsigned int count); -int em28xx_stop_vbi_streaming(struct vb2_queue *vq); -extern const struct v4l2_ctrl_ops em28xx_ctrl_ops; - /* Provided by em28xx-cards.c */ extern struct em28xx_board em28xx_boards[]; extern struct usb_device_id em28xx_id_table[]; int em28xx_tuner_callback(void *ptr, int component, int command, int arg); void em28xx_release_resources(struct em28xx *dev); -/* Provided by em28xx-vbi.c */ -extern struct vb2_ops em28xx_vbi_qops; - /* Provided by em28xx-camera.c */ int em28xx_detect_sensor(struct em28xx *dev); int em28xx_init_camera(struct em28xx *dev);
Now that all analog-specific code are at em28xx-video, convert it into an em28xx extension and load it as a separate module. Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com> --- drivers/media/usb/em28xx/Kconfig | 6 ++- drivers/media/usb/em28xx/Makefile | 5 ++- drivers/media/usb/em28xx/em28xx-camera.c | 1 + drivers/media/usb/em28xx/em28xx-cards.c | 45 ++++--------------- drivers/media/usb/em28xx/em28xx-core.c | 12 ++++++ drivers/media/usb/em28xx/em28xx-v4l.h | 20 +++++++++ drivers/media/usb/em28xx/em28xx-vbi.c | 1 + drivers/media/usb/em28xx/em28xx-video.c | 74 +++++++++++++++++++++++--------- drivers/media/usb/em28xx/em28xx.h | 23 ++-------- 9 files changed, 107 insertions(+), 80 deletions(-) create mode 100644 drivers/media/usb/em28xx/em28xx-v4l.h