Re: [ANNOUNCE] experimental alsa stream support at xawtv3
On Monday, May 23, 2011 22:17:06 Mauro Carvalho Chehab wrote: > Due to the alsa detection code that I've added at libv4l2util (at v4l2-utils) > during the weekend, I decided to add alsa support also on xawtv3, basically > to provide a real usecase example. Of course, for it to work, it needs the > very latest v4l2-utils version from the git tree. Please, please add at the very least some very big disclaimer in libv4l2util that the API/ABI is likely to change. As mentioned earlier, this library is undocumented, has not gone through any peer-review, and I am very unhappy with it and with the decision (without discussion it seems) to install it. Once you install it on systems it becomes much harder to change. Regards, Hans > I've basically added there the code that Devin wrote for tvtime, with a few > small fixes and with the audio device auto-detection. > > With this patch, xawtv will now get the alsa device associated with a video > device node (if any), and start streaming from it, on a separate thread. > > As the code is the same as the one at tvtime, it should work at the > same devices that are supported there. I tested it only on two em28xx devices: > - HVR-950; > - WinTV USB-2. > > It worked with HVR-950, but it didn't work with WinTV USB-2. It seems that > snd-usb-audio do something different to set the framerate, that the > alsa-stream > code doesn't recognize. While I didn't test, I think it probably won't work > with saa7134, as the code seems to hardcode the frame rate to 48 kHz, but > saa7134 supports only 32 kHz. > > It would be good to add an option to disable this behavior and to allow > manually > select the alsa out device, so please send us patches ;) > > Anyway, patches fixing it and more tests are welcome. > > The git repositories for xawtv3 and v4l-utils is at: > > http://git.linuxtv.org/xawtv3.git > http://git.linuxtv.org/v4l-utils.git > > Thanks, > Mauro. > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PATCHES FOR 2.6.40] Fixes
On Tuesday, May 24, 2011 03:42:32 Mauro Carvalho Chehab wrote: > Em 23-05-2011 08:06, Hans Verkuil escreveu: > > Hi Mauro, > > > > Here are a few fixes: the first fixes a bug in the wl12xx drivers (I hope > > Matti's > > email is still correct). The second fixes a few DocBook validation errors, > > and > > the last fixes READ_ONLY and GRABBED handling in the control framework. > > > > Regards, > > > > Hans > > > > The following changes since commit 87cf028f3aa1ed51fe29c36df548aa714dc7438f: > > > > [media] dm1105: GPIO handling added, I2C on GPIO added, LNB control > > through GPIO reworked (2011-05-21 11:10:28 -0300) > > > > are available in the git repository at: > > ssh://linuxtv.org/git/hverkuil/media_tree.git fixes > > > > Hans Verkuil (3): > > wl12xx: g_volatile_ctrl fix: wrong field set. > > Media DocBook: fix validation errors. > > The two above are fixes... > > > v4l2-ctrls: drivers should be able to ignore READ_ONLY and GRABBED > > flags > > But this one is a change at the behaviour. I need to analyse it better. The > idea > of a "read only" control that it is not read only seems too weird on my tired > eyes. It's read-only for *applications*. But if you have a read-only control that reflects a driver state, then it should be possible for a *driver* to change it. It's something that is needed for the upcoming Flash and HDMI APIs. The userspace behavior does not change. BTW, if you prefer to move this last patch to 2.6.41, then that's OK by me. It's not really necessary for 2.6.40. Regards, Hans > > I'll take a more careful look on it tomorrow. > > Thanks, > Mauro. > > > > > Documentation/DocBook/dvb/dvbproperty.xml|5 ++- > > Documentation/DocBook/v4l/subdev-formats.xml | 10 ++-- > > drivers/media/radio/radio-wl1273.c |2 +- > > drivers/media/radio/wl128x/fmdrv_v4l2.c |2 +- > > drivers/media/video/v4l2-ctrls.c | 59 > > +- > > 5 files changed, 50 insertions(+), 28 deletions(-) > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-media" in > > the body of a message to majord...@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] MT9P031: Add support for Aptina mt9p031 sensor.
On 23/05/11 18:54, javier Martin wrote: > On 23 May 2011 05:01, Chris Rodley wrote: >> Error when using media-ctl as below with v2 mt9p031 driver from Javier and >> latest media-ctl version. >> Is there a patch I missed to add different formats - or maybe my command is >> wrong? > Please, try the following: > > ./media-ctl -r -l '"mt9p031 2-0048":0->"OMAP3 ISP CCDC":0[1], "OMAP3 > ISP CCDC":1->"OMAP3 ISP CCDC output":0[1]' > ./media-ctl -f '"mt9p031 2-0048":0[SGRBG12 320x240], "OMAP3 ISP > CCDC":0[SGRBG8 320x240], "OMAP3 ISP CCDC":1[SGRBG8 320x240]' > > Thanks. Thanks Javier! That worked. The command below used to work but no output to stdout any more: ./yavta --stdout -f SGRBG8 -s 320x240 -n 4 --capture=100 -F `./media-ctl -e "OMAP3 ISP CCDC output"` | nc 10.1.1.99 3000 I have played around with this but have been unable to get a result. Something to do with the media-ctl part of the command? Thanks again! Chris -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PATCHES FOR 2.6.40] Fixes
Em 23-05-2011 08:06, Hans Verkuil escreveu: > Hi Mauro, > > Here are a few fixes: the first fixes a bug in the wl12xx drivers (I hope > Matti's > email is still correct). The second fixes a few DocBook validation errors, and > the last fixes READ_ONLY and GRABBED handling in the control framework. > > Regards, > > Hans > > The following changes since commit 87cf028f3aa1ed51fe29c36df548aa714dc7438f: > > [media] dm1105: GPIO handling added, I2C on GPIO added, LNB control through > GPIO reworked (2011-05-21 11:10:28 -0300) > > are available in the git repository at: > ssh://linuxtv.org/git/hverkuil/media_tree.git fixes > > Hans Verkuil (3): > wl12xx: g_volatile_ctrl fix: wrong field set. > Media DocBook: fix validation errors. The two above are fixes... > v4l2-ctrls: drivers should be able to ignore READ_ONLY and GRABBED flags But this one is a change at the behaviour. I need to analyse it better. The idea of a "read only" control that it is not read only seems too weird on my tired eyes. I'll take a more careful look on it tomorrow. Thanks, Mauro. > > Documentation/DocBook/dvb/dvbproperty.xml|5 ++- > Documentation/DocBook/v4l/subdev-formats.xml | 10 ++-- > drivers/media/radio/radio-wl1273.c |2 +- > drivers/media/radio/wl128x/fmdrv_v4l2.c |2 +- > drivers/media/video/v4l2-ctrls.c | 59 > +- > 5 files changed, 50 insertions(+), 28 deletions(-) > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[GIT PULL for v3.0-rc1] media updates
Hi Linus, Please pull from: ssh://master.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-2.6.git v4l_for_linus For the patches for the next version, hopefully Linux 3.0 :) of the kernel. This series: - Extends the DVB API to support the new European terrestrial standard (DVB-T2); - Adds support for Kinect color camera; - Adds new drivers for cxd2820r and drx-d frontends; - Adds support for a Remote controller from a vendor called Redrat (with r); - Adds a driver for a new NXP tuner (tda18212); - The rest is usual driver enhancements, new boards, fixes, etc. Thanks! Mauro - The following changes since commit 61c4f2c81c61f73549928dfd9f3e8f26aa36a8cf: Linux 2.6.39 (2011-05-18 21:06:34 -0700) are available in the git repository at: ssh://master.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-2.6.git v4l_for_linus Alexey Khoroshilov (1): [media] lmedm04: Do not unlock mutex if mutex_lock_interruptible failed Anatolij Gustschin (3): [media] fsl-viu: replace .ioctl by .unlocked_ioctl [media] fsl_viu: add VIDIOC_OVERLAY ioctl [media] media: fsl_viu: fix bug in streamon routine Andreas Oberritter (11): [media] DVB: return meaningful error codes in dvb_frontend [media] DVB: Add basic API support for DVB-T2 and bump minor version [media] DVB: drxd_hard: handle new bandwidths by returning -EINVAL [media] DVB: mxl5005s: handle new bandwidths by returning -EINVAL [media] DVB: dtv_property_cache_submit shouldn't modifiy the cache [media] DVB: call get_property at the end of dtv_property_process_get [media] DVB: dvb_frontend: rename parameters to parameters_in [media] DVB: dvb_frontend: remove unused assignments [media] DVB: dvb_frontend: use shortcut to access fe->dtv_property_cache [media] DVB: dvb_frontend: add parameters_out [media] DVB: allow to read back of detected parameters through S2API Andy Walls (2): [media] cx18: Make RF analog TV work for newer HVR-1600 models with silicon tuners [media] cx18: Bump driver version, since a new class of HVR-1600 is properly supported Antonio Ospite (3): [media] Add Y10B, a 10 bpp bit-packed greyscale format [media] gspca - kinect: New subdriver for Microsoft Kinect [media] gspca - kinect: fix comments referring to color camera Antti Palosaari (23): [media] NXP TDA18212HN silicon tuner driver [media] anysee: I2C address fix [media] anysee: fix multibyte I2C read [media] anysee: change some messages [media] anysee: reimplement demod and tuner attach [media] anysee: add support for TDA18212 based E30 Combo Plus [media] anysee: add support for Anysee E7 TC [media] anysee: fix E30 Combo Plus TDA18212 GPIO [media] anysee: fix E30 Combo Plus TDA18212 DVB-T [media] anysee: enhance demod and tuner attach [media] anysee: add support for two byte I2C address [media] anysee: add more info about known board configs [media] cx24116: add config option to split firmware download [media] anysee: add support for Anysee E30 S2 Plus [media] anysee: add support for Anysee E7 S2 [media] cx24116: make FW DL split more readable [media] tda18271: add DVB-C support [media] em28xx: Multi Frontend (MFE) support [media] em28xx: add support for EM28174 chip [media] Sony CXD2820R DVB-T/T2/C demodulator driver [media] Add support for PCTV nanoStick T2 290e [2013:024f] [media] cxd2820r: whitespace fix [media] cxd2820r: switch automatically between DVB-T and DVB-T2 Bjørn Mork (2): [media] use pci_dev->revision [media] mantis: trivial module parameter documentation fix Bob Liu (2): [media] Revert "V4L/DVB: v4l2-dev: remove get_unmapped_area" [media] uvcvideo: Add support for NOMMU arch Dan Carpenter (2): [media] pvrusb2: check for allocation failures [media] pvrusb2: delete generic_standards_cnt Daniel Drake (1): [media] via-camera: add MODULE_ALIAS David Härdeman (4): [media] rc-core: int to bool conversion for winbond-cir [media] rc-core: add TX support to the winbond-cir driver [media] rc-core: add trailing silence in rc-loopback tx [media] rc-core: use ir_raw_event_store_with_filter in winbond-cir Detlev Casanova (1): [media] v4l: Add mt9v032 sensor driver Devin Heitmueller (12): [media] drxd: add driver to Makefile and Kconfig [media] drxd: provide ability to control rs byte [media] em28xx: enable support for the drx-d on the HVR-900 R2 [media] drxd: provide ability to disable the i2c gate control function [media] em28xx: fix GPIO problem with HVR-900R2 getting out of sync with drx-d [media] em28xx: include model number for PCTV 330e [media] em28xx: add digital support for PCTV 330e [media] drxd: move firmware to binary blob [media] em
em28xx new device
Hi, I have a board with empia chipset. The em28xx driver not load, because the device ID is not listed on source(cards.c, i guess). Following bellow have some infos from board: lsusb: Bus 001 Device 004: ID 1b80:e755 Afatech Opening the device, shows this ic: empia em2888 d351c-195 727-00ag (em28xx) nxp saa7136e/1/g SI5296.1 22 ZSD08411 NXP TDA 18271??C2 HDC2? (tda18271) F JAPAN mb86a20s 0937 M01 E1 (mb86a20s) but... dmesg shows: [18373.454136] usb 6-1: USB disconnect, address 2 [18376.744074] usb 2-1: new high speed USB device using ehci_hcd and address 9 [18376.860283] usb 2-1: New USB device found, idVendor=1b80, idProduct=e755 [18376.860293] usb 2-1: New USB device strings: Mfr=0, Product=1, SerialNumber=2 [18376.860300] usb 2-1: Product: USB 2885 Device [18376.860306] usb 2-1: SerialNumber: 1 The em28xx module can handle this device? Thanks, Moa -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/4] v4l2-ctrls: add support for per-buffer control
This patch adds is_bufferable member to v4l2_ctrl structure and v4l2_ctrl_p_ctrl() to support per-buffer control. Signed-off-by: Jeongtae Park Cc: Hans Verkuil Cc: Kamil Debski --- drivers/media/video/v4l2-ctrls.c | 17 + include/media/v4l2-ctrls.h | 18 ++ 2 files changed, 35 insertions(+), 0 deletions(-) diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c index e6fa9be..4664f6f 100644 --- a/drivers/media/video/v4l2-ctrls.c +++ b/drivers/media/video/v4l2-ctrls.c @@ -1100,6 +1100,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl, if (ctrl) { ctrl->is_private = cfg->is_private; ctrl->is_volatile = cfg->is_volatile; + ctrl->is_bufferable = cfg->is_bufferable; } return ctrl; } @@ -1641,6 +1642,11 @@ static int get_ctrl(struct v4l2_ctrl *ctrl, s32 *val) if (ctrl->is_volatile && master->ops->g_volatile_ctrl) ret = master->ops->g_volatile_ctrl(master); *val = ctrl->cur.val; + if (ctrl->is_bufferable) { + if (!ctrl->is_new) + ret = -EINVAL; + ctrl->is_new = 0; + } v4l2_ctrl_unlock(master); return ret; } @@ -1672,6 +1678,14 @@ s32 v4l2_ctrl_g_ctrl(struct v4l2_ctrl *ctrl) } EXPORT_SYMBOL(v4l2_ctrl_g_ctrl); +int v4l2_ctrl_p_ctrl(struct v4l2_ctrl *ctrl, s32 *val) +{ + /* It's a driver bug if this happens. */ + WARN_ON(!type_is_int(ctrl)); + return get_ctrl(ctrl, val); +} +EXPORT_SYMBOL(v4l2_ctrl_p_ctrl); + /* Core function that calls try/s_ctrl and ensures that the new value is copied to the current value on a set. @@ -1872,6 +1886,9 @@ static int set_ctrl(struct v4l2_ctrl *ctrl, s32 *val) v4l2_ctrl_lock(ctrl); + if (ctrl->is_bufferable && ctrl->is_new) + printk(KERN_WARNING "overwrite unused control value"); + /* Reset the 'is_new' flags of the cluster */ for (i = 0; i < master->ncontrols; i++) if (master->cluster[i]) diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h index 27714c9..34e69d1 100644 --- a/include/media/v4l2-ctrls.h +++ b/include/media/v4l2-ctrls.h @@ -67,6 +67,7 @@ struct v4l2_ctrl_ops { *control's current value cannot be cached and needs to be *retrieved through the g_volatile_ctrl op. Drivers can set *this flag. + * @is_bufferable: If set, then this control use to handle per-buffer control. * @ops: The control ops. * @id: The control ID. * @name: The control name. @@ -108,6 +109,7 @@ struct v4l2_ctrl { unsigned int is_new:1; unsigned int is_private:1; unsigned int is_volatile:1; + unsigned int is_bufferable:1; const struct v4l2_ctrl_ops *ops; u32 id; @@ -201,6 +203,7 @@ struct v4l2_ctrl_fh { * @is_volatile: If set, then this control is volatile. This means that the *control's current value cannot be cached and needs to be *retrieved through the g_volatile_ctrl op. + * @is_bufferable: If set, then this control use to handle per-buffer control. */ struct v4l2_ctrl_config { const struct v4l2_ctrl_ops *ops; @@ -216,6 +219,7 @@ struct v4l2_ctrl_config { const char * const *qmenu; unsigned int is_private:1; unsigned int is_volatile:1; + unsigned int is_bufferable:1; }; /** v4l2_ctrl_fill() - Fill in the control fields based on the control ID. @@ -434,6 +438,20 @@ static inline void v4l2_ctrl_unlock(struct v4l2_ctrl *ctrl) */ s32 v4l2_ctrl_g_ctrl(struct v4l2_ctrl *ctrl); +/** v4l2_ctrl_p_ctrl() - Helper function to pick the control's value from within a driver. + * @ctrl: The control. + * @val: The new value. + * + * This picks the control's new value safely by going through the control + * framework. This function will lock the control's handler, so it cannot be + * used from within the &v4l2_ctrl_ops functions. If control has bufferable flag, + * this returns valid value when control has new value. If not, It will return error value. + * After the execution of this operation control's is_new flag reset to 0. + * + * This function is for integer type controls only. + */ +int v4l2_ctrl_p_ctrl(struct v4l2_ctrl *ctrl, s32 *val); + /** v4l2_ctrl_s_ctrl() - Helper function to set the control's value from within a driver. * @ctrl: The control. * @val: The new value. -- 1.7.1 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/4] v4l2: Apply control events patch series
This patch apply control event patch series by Hans Verkuil. This is only merge-down version of contorl event patches for MFC working branch. http://git.infradead.org/users/kmpark/linux-2.6-samsung/shortlog/refs/heads/mfc_v8 You can be found the original patches below message http://www.spinics.net/lists/linux-media/msg31010.html or http://git.linuxtv.org/hverkuil/v4l-utils.git?a=shortlog;h=refs/heads/core Signed-off-by: Hans Verkuil Signed-off-by: Jeongtae Park Cc: Kamil Debski --- drivers/media/video/cx2341x.c| 58 +-- drivers/media/video/saa7115.c|3 +- drivers/media/video/v4l2-common.c|3 + drivers/media/video/v4l2-ctrls.c | 137 +++--- drivers/media/video/v4l2-event.c | 128 +++- drivers/media/video/v4l2-fh.c|6 +- drivers/media/video/v4l2-ioctl.c | 36 +++-- drivers/media/video/videobuf2-core.c | 16 +--- drivers/media/video/vivi.c | 63 +++- include/linux/videodev2.h| 29 +++- include/media/v4l2-ctrls.h | 47 +++- include/media/v4l2-event.h |2 + include/media/v4l2-fh.h |2 + 13 files changed, 396 insertions(+), 134 deletions(-) diff --git a/drivers/media/video/cx2341x.c b/drivers/media/video/cx2341x.c index 103ef6b..2781889 100644 --- a/drivers/media/video/cx2341x.c +++ b/drivers/media/video/cx2341x.c @@ -1307,6 +1307,12 @@ static int cx2341x_try_ctrl(struct v4l2_ctrl *ctrl) return 0; } +static void cx2341x_activate(struct v4l2_ctrl *ctrl, bool activate) +{ + v4l2_ctrl_flags(ctrl, V4L2_CTRL_FLAG_INACTIVE, + activate ? 0 : V4L2_CTRL_FLAG_INACTIVE); +} + static int cx2341x_s_ctrl(struct v4l2_ctrl *ctrl) { static const int mpeg_stream_type[] = { @@ -1380,10 +1386,10 @@ static int cx2341x_s_ctrl(struct v4l2_ctrl *ctrl) int is_ac3 = hdl->audio_encoding->val == V4L2_MPEG_AUDIO_ENCODING_AC3; - v4l2_ctrl_activate(hdl->audio_ac3_bitrate, is_ac3); - v4l2_ctrl_activate(hdl->audio_l2_bitrate, !is_ac3); + cx2341x_activate(hdl->audio_ac3_bitrate, is_ac3); + cx2341x_activate(hdl->audio_l2_bitrate, !is_ac3); } - v4l2_ctrl_activate(hdl->audio_mode_extension, + cx2341x_activate(hdl->audio_mode_extension, hdl->audio_mode->val == V4L2_MPEG_AUDIO_MODE_JOINT_STEREO); if (cx2341x_neq(hdl->audio_sampling_freq) && hdl->ops && hdl->ops->s_audio_sampling_freq) @@ -1413,9 +1419,9 @@ static int cx2341x_s_ctrl(struct v4l2_ctrl *ctrl) if (err) return err; - v4l2_ctrl_activate(hdl->video_bitrate_mode, + cx2341x_activate(hdl->video_bitrate_mode, hdl->video_encoding->val != V4L2_MPEG_VIDEO_ENCODING_MPEG_1); - v4l2_ctrl_activate(hdl->video_bitrate_peak, + cx2341x_activate(hdl->video_bitrate_peak, hdl->video_bitrate_mode->val != V4L2_MPEG_VIDEO_BITRATE_MODE_CBR); if (cx2341x_neq(hdl->video_encoding) && hdl->ops && hdl->ops->s_video_encoding) @@ -1441,18 +1447,18 @@ static int cx2341x_s_ctrl(struct v4l2_ctrl *ctrl) active_filter = hdl->video_spatial_filter_mode->val != V4L2_MPEG_CX2341X_VIDEO_SPATIAL_FILTER_MODE_AUTO; - v4l2_ctrl_activate(hdl->video_spatial_filter, active_filter); - v4l2_ctrl_activate(hdl->video_luma_spatial_filter_type, active_filter); - v4l2_ctrl_activate(hdl->video_chroma_spatial_filter_type, active_filter); + cx2341x_activate(hdl->video_spatial_filter, active_filter); + cx2341x_activate(hdl->video_luma_spatial_filter_type, active_filter); + cx2341x_activate(hdl->video_chroma_spatial_filter_type, active_filter); active_filter = hdl->video_temporal_filter_mode->val != V4L2_MPEG_CX2341X_VIDEO_TEMPORAL_FILTER_MODE_AUTO; - v4l2_ctrl_activate(hdl->video_temporal_filter, active_filter); + cx2341x_activate(hdl->video_temporal_filter, active_filter); active_filter = hdl->video_median_filter_type->val != V4L2_MPEG_CX2341X_VIDEO_MEDIAN_FILTER_TYPE_OFF; - v4l2_ctrl_activate(hdl->video_luma_median_filter_bottom, active_filter); - v4l2_ctrl_activate(hdl->video_luma_median_filter_top, active_filter); - v4l2_ctrl_activate(hdl->video_chroma_median_filter_bottom, active_filter); - v4l2_ctrl_activate(hdl->video_chroma_median_filter_top, active_filter); + cx2341x_activate(hdl->video_l
[PATCH 4/4] media: MFC: Add support control framework in decoder
This patch migrate to v4l2 control framework for MFC decoder. It utilize per-filehandle & per-buffer control handling facilities. Signed-off-by: Jeongtae Park Cc: Hans Verkuil Cc: Kamil Debski --- drivers/media/video/s5p-mfc/regs-mfc.h |1 + drivers/media/video/s5p-mfc/s5p_mfc.c| 184 - drivers/media/video/s5p-mfc/s5p_mfc_common.h | 31 +- drivers/media/video/s5p-mfc/s5p_mfc_dec.c| 597 +++--- drivers/media/video/s5p-mfc/s5p_mfc_dec.h|4 +- drivers/media/video/s5p-mfc/s5p_mfc_enc.c| 84 +++-- drivers/media/video/s5p-mfc/s5p_mfc_enc.h|4 +- drivers/media/video/s5p-mfc/s5p_mfc_opr.c| 33 ++- 8 files changed, 429 insertions(+), 509 deletions(-) diff --git a/drivers/media/video/s5p-mfc/regs-mfc.h b/drivers/media/video/s5p-mfc/regs-mfc.h index 8c67fe1..396381e 100644 --- a/drivers/media/video/s5p-mfc/regs-mfc.h +++ b/drivers/media/video/s5p-mfc/regs-mfc.h @@ -285,6 +285,7 @@ #define S5P_FIMV_SI_CH0_DPB_CONF_CTRL 0x2068 /* DPB Config Control Register */ #define S5P_FIMV_SLICE_INT_MASK1 #define S5P_FIMV_SLICE_INT_SHIFT 31 +#define S5P_FIMV_DDELAY_ENA_MASK 1 #define S5P_FIMV_DDELAY_ENA_SHIFT 30 #define S5P_FIMV_DDELAY_VAL_MASK 0xff #define S5P_FIMV_DDELAY_VAL_SHIFT 16 diff --git a/drivers/media/video/s5p-mfc/s5p_mfc.c b/drivers/media/video/s5p-mfc/s5p_mfc.c index c9c5d1e..0f86928 100644 --- a/drivers/media/video/s5p-mfc/s5p_mfc.c +++ b/drivers/media/video/s5p-mfc/s5p_mfc.c @@ -673,27 +673,38 @@ static int s5p_mfc_open(struct file *file) { struct s5p_mfc_ctx *ctx = NULL; struct s5p_mfc_dev *dev = video_drvdata(file); - struct vb2_queue *q; unsigned long flags; int ret = 0; + enum s5p_mfc_node_type node; mfc_debug_enter(); + node = s5p_mfc_get_node_type(file); + if (node == MFCNODE_INVALID) { + mfc_err("cannot specify node type\n"); + ret = -ENOENT; + goto err_node_type; + } + dev->num_inst++;/* It is guarded by mfc_mutex in vfd */ /* Allocate memory for context */ - ctx = kzalloc(sizeof *ctx, GFP_KERNEL); + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL); if (!ctx) { mfc_err("Not enough memory.\n"); ret = -ENOMEM; - goto out_open; + goto err_ctx_alloc; } - file->private_data = ctx; + + ret = v4l2_fh_init(&ctx->fh, (node == MFCNODE_DECODER) ? + dev->vfd_dec : dev->vfd_enc); + if (ret) + goto err_v4l2_fh; + file->private_data = &ctx->fh; + v4l2_fh_add(&ctx->fh); + ctx->dev = dev; - INIT_LIST_HEAD(&ctx->src_queue); - INIT_LIST_HEAD(&ctx->dst_queue); - ctx->src_queue_cnt = 0; - ctx->dst_queue_cnt = 0; + /* Get context number */ ctx->num = 0; while (dev->ctx[ctx->num]) { @@ -701,35 +712,31 @@ static int s5p_mfc_open(struct file *file) if (ctx->num >= MFC_NUM_CONTEXTS) { mfc_err("Too many open contexts.\n"); ret = -EBUSY; - goto out_open; + goto err_ctx_num; } } + /* Mark context as idle */ spin_lock_irqsave(&dev->condlock, flags); clear_bit(ctx->num, &dev->ctx_work_bits); spin_unlock_irqrestore(&dev->condlock, flags); dev->ctx[ctx->num] = ctx; - if (s5p_mfc_get_node_type(file) == MFCNODE_DECODER) { - ctx->type = MFCINST_DECODER; - ctx->c_ops = get_dec_codec_ops(); - /* Default format */ - ctx->src_fmt = get_dec_def_fmt(1); - ctx->dst_fmt = get_dec_def_fmt(0); - } else if (s5p_mfc_get_node_type(file) == MFCNODE_ENCODER) { - ctx->type = MFCINST_ENCODER; - ctx->c_ops = get_enc_codec_ops(); - /* Default format */ - ctx->src_fmt = get_enc_def_fmt(1); - ctx->dst_fmt = get_enc_def_fmt(0); - - /* only for encoder */ - INIT_LIST_HEAD(&ctx->ref_queue); - ctx->ref_queue_cnt = 0; - } else { - ret = -ENOENT; - goto out_open; + + init_waitqueue_head(&ctx->queue); + + if (node == MFCNODE_DECODER) + ret = s5p_mfc_init_dec_ctx(ctx); + else + ret = s5p_mfc_init_enc_ctx(ctx); + if (ret) + goto err_ctx_init; + + ret = call_cop(ctx, init_ctx_ctrls, ctx); + if (ret) { + mfc_err("failed in init_buf_ctrls\n"); + goto err_ctx_ctrls; } - ctx->inst_no = -1; + /* Load firmware if this is the first instance */ if (dev->num_inst == 1) { dev->watchdog_timer.expires = jiffies + @@ -738,106 +745,70 @@ static int s5p_mfc_open(stru
Add support control framework in MFC decoder
Hi, This patch series implements MFC contorl framework support in decoder. I started with below branch (MFC v8) http://git.infradead.org/users/kmpark/linux-2.6-samsung/shortlog/refs/heads/mfc_v8 and apply control event patch series by Hans Verkuil to the branch. After that add some minor changes to support per-buffer control in the framework. Last, migrate MFC decoder to the control framework. Any comments are welcome! This patch series contains: [PATCH 1/4] media: MFC: Remove usused variables & compile warnings [PATCH 2/4] v4l2: Apply control events patch series [PATCH 3/4] v4l2-ctrls: add support for per-buffer control [PATCH 4/4] media: MFC: Add support control framework in decoder Best regards, Jeongtae Park -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/4] media: MFC: Remove usused variables & compile warnings
This patch removes unused variables & compile warnings Signed-off-by: Jeongtae Park Cc: Kamil Debski --- drivers/media/video/s5p-mfc/s5p_mfc_enc.c |1 - drivers/media/video/s5p-mfc/s5p_mfc_mem.c |1 + drivers/media/video/s5p-mfc/s5p_mfc_opr.c |1 - 3 files changed, 1 insertions(+), 2 deletions(-) diff --git a/drivers/media/video/s5p-mfc/s5p_mfc_enc.c b/drivers/media/video/s5p-mfc/s5p_mfc_enc.c index 981fdfe..530ff0b 100644 --- a/drivers/media/video/s5p-mfc/s5p_mfc_enc.c +++ b/drivers/media/video/s5p-mfc/s5p_mfc_enc.c @@ -1935,7 +1935,6 @@ static int s5p_mfc_start_streaming(struct vb2_queue *q) struct s5p_mfc_ctx *ctx = q->drv_priv; struct s5p_mfc_dev *dev = ctx->dev; unsigned long flags; - unsigned ret; /* If context is ready then dev = work->data;schedule it to run */ if (s5p_mfc_ctx_ready(ctx)) { diff --git a/drivers/media/video/s5p-mfc/s5p_mfc_mem.c b/drivers/media/video/s5p-mfc/s5p_mfc_mem.c index d5e235f..aeb3306 100644 --- a/drivers/media/video/s5p-mfc/s5p_mfc_mem.c +++ b/drivers/media/video/s5p-mfc/s5p_mfc_mem.c @@ -20,6 +20,7 @@ #ifdef CONFIG_VIDEO_SAMSUNG_S5P_MFC_DMA_IOMMU +#include #include struct vb2_mem_ops *s5p_mfc_mem_ops(void) diff --git a/drivers/media/video/s5p-mfc/s5p_mfc_opr.c b/drivers/media/video/s5p-mfc/s5p_mfc_opr.c index a22ea43..24b2e11 100644 --- a/drivers/media/video/s5p-mfc/s5p_mfc_opr.c +++ b/drivers/media/video/s5p-mfc/s5p_mfc_opr.c @@ -1061,7 +1061,6 @@ static int s5p_mfc_set_enc_params_h263(struct s5p_mfc_ctx *ctx) { struct s5p_mfc_dev *dev = ctx->dev; struct s5p_mfc_enc_params *p = &ctx->enc_params; - struct s5p_mfc_mpeg4_enc_params *p_mpeg4 = &p->codec.mpeg4; unsigned int reg; unsigned int shm; -- 1.7.1 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/5] V4L: omap1-camera: fix huge lookup array
Dnia środa 18 maj 2011 o 16:11:30 Guennadi Liakhovetski napisał(a): > Since V4L2_MBUS_FMT_* codes have become large and sparse, they cannot > be used as array indices anymore. Hi Guennadi, Thanks for taking care of this. Regards, Janusz > Signed-off-by: Guennadi Liakhovetski > --- > drivers/media/video/omap1_camera.c | 41 > ++- 1 files changed, 30 > insertions(+), 11 deletions(-) > > diff --git a/drivers/media/video/omap1_camera.c > b/drivers/media/video/omap1_camera.c > index 5954b93..fe577a9 100644 > --- a/drivers/media/video/omap1_camera.c > +++ b/drivers/media/video/omap1_camera.c > @@ -990,63 +990,80 @@ static void omap1_cam_remove_device(struct > soc_camera_device *icd) } > > /* Duplicate standard formats based on host capability of byte swapping */ > -static const struct soc_mbus_pixelfmt omap1_cam_formats[] = { > - [V4L2_MBUS_FMT_UYVY8_2X8] = { > +static const struct soc_mbus_lookup omap1_cam_formats[] = { > +{ > + .code = V4L2_MBUS_FMT_UYVY8_2X8, > + .fmt = { > .fourcc = V4L2_PIX_FMT_YUYV, > .name = "YUYV", > .bits_per_sample= 8, > .packing= SOC_MBUS_PACKING_2X8_PADHI, > .order = SOC_MBUS_ORDER_BE, > }, > - [V4L2_MBUS_FMT_VYUY8_2X8] = { > +}, { > + .code = V4L2_MBUS_FMT_VYUY8_2X8, > + .fmt = { > .fourcc = V4L2_PIX_FMT_YVYU, > .name = "YVYU", > .bits_per_sample= 8, > .packing= SOC_MBUS_PACKING_2X8_PADHI, > .order = SOC_MBUS_ORDER_BE, > }, > - [V4L2_MBUS_FMT_YUYV8_2X8] = { > +}, { > + .code = V4L2_MBUS_FMT_YUYV8_2X8, > + .fmt = { > .fourcc = V4L2_PIX_FMT_UYVY, > .name = "UYVY", > .bits_per_sample= 8, > .packing= SOC_MBUS_PACKING_2X8_PADHI, > .order = SOC_MBUS_ORDER_BE, > }, > - [V4L2_MBUS_FMT_YVYU8_2X8] = { > +}, { > + .code = V4L2_MBUS_FMT_YVYU8_2X8, > + .fmt = { > .fourcc = V4L2_PIX_FMT_VYUY, > .name = "VYUY", > .bits_per_sample= 8, > .packing= SOC_MBUS_PACKING_2X8_PADHI, > .order = SOC_MBUS_ORDER_BE, > }, > - [V4L2_MBUS_FMT_RGB555_2X8_PADHI_BE] = { > +}, { > + .code = V4L2_MBUS_FMT_RGB555_2X8_PADHI_BE, > + .fmt = { > .fourcc = V4L2_PIX_FMT_RGB555, > .name = "RGB555", > .bits_per_sample= 8, > .packing= SOC_MBUS_PACKING_2X8_PADHI, > .order = SOC_MBUS_ORDER_BE, > }, > - [V4L2_MBUS_FMT_RGB555_2X8_PADHI_LE] = { > +}, { > + .code = V4L2_MBUS_FMT_RGB555_2X8_PADHI_LE, > + .fmt = { > .fourcc = V4L2_PIX_FMT_RGB555X, > .name = "RGB555X", > .bits_per_sample= 8, > .packing= SOC_MBUS_PACKING_2X8_PADHI, > .order = SOC_MBUS_ORDER_BE, > }, > - [V4L2_MBUS_FMT_RGB565_2X8_BE] = { > +}, { > + .code = V4L2_MBUS_FMT_RGB565_2X8_BE, > + .fmt = { > .fourcc = V4L2_PIX_FMT_RGB565, > .name = "RGB565", > .bits_per_sample= 8, > .packing= SOC_MBUS_PACKING_2X8_PADHI, > .order = SOC_MBUS_ORDER_BE, > }, > - [V4L2_MBUS_FMT_RGB565_2X8_LE] = { > +}, { > + .code = V4L2_MBUS_FMT_RGB565_2X8_LE, > + .fmt = { > .fourcc = V4L2_PIX_FMT_RGB565X, > .name = "RGB565X", > .bits_per_sample= 8, > .packing= SOC_MBUS_PACKING_2X8_PADHI, > .order = SOC_MBUS_ORDER_BE, > }, > +}, > }; > > static int omap1_cam_get_formats(struct soc_camera_device *icd, > @@ -1085,12 +1102,14 @@ static int omap1_cam_get_formats(struct > soc_camera_device *icd, case V4L2_MBUS_FMT_RGB565_2X8_LE: > formats++; > if (xlate) { > - xlate->host_fmt = &omap1_cam_formats[code]; > + xlate->host_fmt = soc_mbus_find_fmtdesc(code, > + omap1_cam_formats, > + ARRAY_SIZE(omap1_cam_formats)); > xlate->code = code; > xlate++; > dev_dbg(dev, > "%s: providing format %s as byte swapped code > #%d\n", > -
Re: [RFC] Standardize YUV support in the fbdev API
Hi Laurent, On 05/23/2011 09:00 PM, Laurent Pinchart wrote: On Saturday 21 May 2011 00:33:02 Florian Tobias Schandinat wrote: On 05/17/2011 10:07 PM, Laurent Pinchart wrote: - Other solutions are possible, such as adding new ioctls. Those solutions are more intrusive, and require larger changes to both userspace and kernelspace code. I'm against (ab)using the nonstd field (probably the only sane thing we can do with it is declare it non-standard which interpretation is completely dependent on the specific driver) or requiring previously unused fields to have a special value so I'd like to suggest a different method: I remembered an earlier discussion: [ http://marc.info/?l=linux-fbdev&m=129896686208130&w=2 ] On 03/01/2011 08:07 AM, Geert Uytterhoeven wrote: > On Tue, Mar 1, 2011 at 04:13, Damian wrote: >> On 2011/02/24 15:05, Geert Uytterhoeven wrote: >>> For YUV (do you mean YCbCr?), I'm inclined to suggest adding a new >>> FB_VISUAL_* >>> type instead, which indicates the fb_var_screeninfo.{red,green,blue} >>> fields are >>> YCbCr instead of RGB. >>> Depending on the frame buffer organization, you also need new >>> FB_TYPE_*/FB_AUX_* >>> types. >> >> I just wanted to clarify here. Is your comment about these new flags >> in specific reference to this patch or to Magnus' "going forward" >> comment? It > > About new flags. > >> seems like the beginnings of a method to standardize YCbCr support in >> fbdev across all platforms. >> Also, do I understand correctly that FB_VISUAL_ would specify the >> colorspace > > FB_VISUAL_* specifies how pixel values (which may be tuples) are mapped > to colors on the screen, so to me it looks like the sensible way to set > up YCbCr. > >> (RGB, YCbCr), FB_TYPE_* would be a format specifier (i.e. planar, >> semiplanar, interleaved, etc)? I'm not really sure what you are >> referring to with the FB_AUX_* however. > > Yep, FB_TYPE_* specifies how pixel values/tuples are laid out in frame > buffer memory. > > FB_AUX_* is only used if a specific value of FB_TYPE_* needs an > additional parameter (e.g. the interleave value for interleaved > bitplanes). Adding new standard values for these fb_fix_screeninfo fields would solve the issue for framebuffers which only support a single format. I've never liked changing fixed screen information :-) It would be consistent with the API though. Fixed does only mean that it can't be directly manipulated by applications. The driver has to modify it anyway on about every mode change (line_length). Yes perhaps some of these fields would be in var today and certainly others shouldn't exist at all but I do not blame anyone for not being capable to look into the future. If you have the need to switch Yes I need that. This requires an API to set the mode through fb_var_screeninfo, which is why I skipped modifying fb_fix_screeinfo. A new FB_TYPE_* could be used to report that we use a 4CC-based mode, with the exact mode reported in one of the fb_fix_screeninfo reserved fields (or the type_aux field). This would duplicate the information passed through fb_var_screeninfo though. Do you think it's worth it ? I think it's more like a FB_VISUAL_FOURCC as you want to express how the color <-> pixel mapping is. The FB_TYPE_* is more about whether pixel are packed or represented as planes (the 2 format groups mentioned on fourcc.org). That's certainly something I'd introduce as it would (hopefully) work to prevent old applications which don't know your extension manipulating a FOURCC format thinking that it is RGB. So I think we should fix.visual = FB_VISUAL_FOURCC; fix.type = FB_TYPE_PACKED_PIXELS | FB_TYPE_PLANES; /* (?) */ or maybe add a FB_TYPE_FOURCC and rely only on the information in FOURCC (as things like UYVY could become confusing as macropixel!=pixel) I guess it would be a good idea to add a new flag to the vmode bitfield in fb_var_screeninfo which looks like a general purpose modifier for the videomode. You could than reuse any RGB-specific field you like to pass more information. That looks good to me. The grayscale field could be reused to pass the 4CC. var.grayscale = ; var.vmode = FB_VMODE_FOURCC; and if this vmode flag is not set it means traditional mode (based on RGBA). Maybe we should also use this chance to declare one of the fix_screeninfo reserved fields to be used for capability flags or an API version as we can assume that those are 0 (at least in sane drivers). That's always good, although it's not a hard requirement for the purpose of YUV support. Sure. But it's good to let the application know whether you support the new extension or whether you just ignore the flag. So I'm voting for a fix.caps = FB_CAP_FOURCC; Best regards, Florian Tobias Schandinat -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.or
Re: [GIT PATCH FOR 2.6.40] uvcvideo patches
Hi Mauro, On Friday 20 May 2011 23:01:18 Mauro Carvalho Chehab wrote: > Em 20-05-2011 16:47, Laurent Pinchart escreveu: > > On Friday 20 May 2011 21:16:49 Mauro Carvalho Chehab wrote: > >> Em 20-05-2011 12:49, Laurent Pinchart escreveu: > >>> On Friday 20 May 2011 17:32:45 Mauro Carvalho Chehab wrote: > Em 15-05-2011 04:48, Laurent Pinchart escreveu: > > Hi Mauro, > > > > The following changes since commit > >>> > >>> f9b51477fe540fb4c65a05027fdd6f2ecce4db3b: > > [media] DVB: return meaningful error codes in dvb_frontend > > (2011-05-09 05:47:20 +0200) > > > > are available in the git repository at: > > git://linuxtv.org/pinchartl/uvcvideo.git uvcvideo-next > > > > They replace the git pull request I've sent on Thursday with the same > > subject. [snip] > > Laurent Pinchart (4): > > v4l: Release module if subdev registration fails > > uvcvideo: Register a v4l2_device > > uvcvideo: Register subdevices for each entity > > uvcvideo: Connect video devices to media entities > > We've discussed already about using the media controller for uvcvideo, > but I can't remember anymore what where your aguments in favor of > merging it (and, even if I've remembered it right now, the #irc > channel log is not the proper way to document the rationale to apply > a patch). > > The thing is: it is clear that SoC embedded devices need the media > controller, as they have IP blocks that do weird things, and userspace > may need to access those, as it is not possible to control such IP > blocks using the V4L2 API. > > However, I have serious concerns about media_controller API usage on > generic drivers, as it is required that all drivers should be fully > configurable via V4L2 API alone, otherwise we'll have regressions, as > no generic applications use the media_controller. > > In other words, if you have enough arguments about why we should add > media_controller support at the uvcvideo, please clearly provide them > at the patch descriptions, as this is not obvious. It would equally > important do document, at the uvcvideo doc, what kind of information > is provided via the media_controller and why an userspace application > should care to use it. > >>> > >>> First of all, the uvcvideo driver doesn't require application to use > >>> the media controller API to operate. All configuration is handled > >>> through a V4L2 video device node, and these patches do not modify > >>> that. No change is required to applications to use the uvcvideo > >>> driver. > >>> > >>> There's however a reason why I want to add support for the media > >>> controller API to the uvcvideo driver (I wouldn't have submitted the > >>> patches otherwise > >>> > >>> :-)). UVC-compliant devices are modeled as a connected graph of > >>> :entities > >>> > >>> (called terminals and units in the UVC world). The device topology can > >>> be arbitrarily complex (or simple, which is fortunately often the > >>> case) and is exported by the device to the host through USB > >>> descriptors. The uvcvideo driver parses the descriptor, creates an > >>> internal > >>> representation of the device internal topology, and maps V4L2 > >>> operations to the various entities that the device contains. > >>> The UVC specification standardizes a couple of entities (camera > >>> terminal, processing unit, ...) and allows device vendors to create > >>> vendor-specific entities called extension units (XUs in short). Those > >>> XUs are usually used to expose controls that are not standardized by > >>> UVC to the host. These controls can be anything from an activity LED > >>> to a firmware update system. The uvcvideo tries to map those XU > >>> controls to V4L2 controls when it makes sense (and when the controls > >>> are documented by the device manufacturer, which is unfortunately > >>> often not the case). If an XU control can't be mapped to a V4L2 > >>> control, it can be accessed through uvcvideo-specific (documented) > >>> ioctls. > >>> > >>> In order to access those XU controls, device-specific applications > >>> (such as a firmware update application for instance) need to know what > >>> XUs are present in the device and possibly how they are connected. > >>> That information can't be exported through V4L2. That's why I'm adding > >>> media controller support to the uvcvideo driver. > >> > >> By allowing access to those undocumented XU controls an Evil > >> Manufacturer(tm) could try to sell its own proprietary software that > >> will work on Linux where all other software will deadly fail or will > >> produce very bad results. We've seen that history before. > > As you're putting some names, let me be clearer. In the past we had bad > stuff like this one: > http://kerneltrap.org/node/3729 > > that ended by the need of somebody to rewrite t
Re: IR remote control autorepeat / evdev
On 15.05.2011 06:41, Mauro Carvalho Chehab wrote: > Em 14-05-2011 01:07, Anssi Hannula escreveu: >> On 14.05.2011 01:39, Mauro Carvalho Chehab wrote: >>> Em 13-05-2011 01:48, Anssi Hannula escreveu: On 12.05.2011 04:36, Mauro Carvalho Chehab wrote: > Em 12-05-2011 03:10, Mauro Carvalho Chehab escreveu: >> Em 12-05-2011 02:37, Anssi Hannula escreveu: > >>> I don't see any other places: >>> $ git grep 'REP_PERIOD' . >>> dvb/dvb-usb/dvb-usb-remote.c: input_dev->rep[REP_PERIOD] = >>> d->props.rc.legacy.rc_interval; >> >> Indeed, the REP_PERIOD is not adjusted on other drivers. I agree that we >> should change it to something like 125ms, for example, as 33ms is too >> short, as it takes up to 114ms for a repeat event to arrive. >> > IMO, the enclosed patch should do a better job with repeat events, without > needing to change rc-core/input/event logic. It will indeed reduce the amount of ghost events so it brings us in the right direction. I'd still like to get rid of the ghost repeats entirely, or at least some way for users to do it if we don't do it by default. >>> Maybe we could replace the kernel softrepeat with native repeats (for those protocols/drivers that have them), while making sure that repeat events before REP_DELAY are ignored and repeat events less than REP_PERIOD since the previous event are ignored, so the users can still configure them as they like? >>> >>> This doesn't seem to be the right thing to do. If the kernel will >>> accept 33 ms as the value or REP_PERIOD, but it will internally >>> set the maximum repeat rate is 115 ms (no matter what logic it would >>> use for that), Kernel (or X) shouldn't allow the user to set a smaller >>> value. >>> >>> The thing is that writing a logic to block a small value is not easy, since >>> the max value is protocol-dependent (worse than that, on some cases, it is >>> device-specific). It seems better to add a warning at the userspace tools >>> that delays lower than 115 ms can produce ghost events on IR's. >> >> From what I see, even periods longer than 115 ms can produce ghost events. >> >> For example with your patch softrepeat period is 125ms, release timeout >> 250ms, and a native rate of 110ms: >> >> There are 4 native events transmitted at >> 000 ms >> 110 ms >> 220 ms >> 330 ms >> (user stops between 330ms and 440ms) >> >> This causes these events in the evdev interface: >> 000: 1 >> 125: 2 >> 250: 2 >> 375: 2 >> 500: 2 >> 550: 0 >> >> So we got 1-2 ghost repeat events. >> Or maybe just a module option that causes rc-core to use native repeat events, for those of us that want accurate repeat events without ghosting? >>> >>> If the user already knows about the possibility to generate ghost effects, >>> with low delays, he can simply not pass a bad value to the kernel, instead >>> of forcing a modprobe parameter that will limit the minimal value. >> >> There is no "good value" for REP_PERIOD (as in ghost repeats guaranteed >> gone like with native repeats). Sufficiently large values will make >> ghost repeats increasingly rare, but the period becomes so long the >> autorepeat becomes frustratingly slow to use. >> > The 250 ms delay used internally to wait for a repeat code is there because > shorter periods weren't working on one of the first boards we've added to > rc core (it was a saa7134 - can't remember much details... too much time ago). > > I remember that I added it as a per-board timer (or per protocol?), as it > seemed > to high for me, but later, David sent a series of patches rewriting the > entire > stuff and proposing to have just one timer, arguing that later this could be > changed. As his series were improving rc-core, I ended by acking with the > changes. > > The fact is that REP_PERIODS shorter than that timer makes non-sense, as that > timer is used to actually wait for a repeat message. > > I suspect we should re-work the code, perhaps replacing the 250 ms fixed value > by REP_PERIOD. Well, that still has a 50% chance of a ghost repeat with length 1-125ms (e.g. native rate 110ms, user releases button at 900ms, last native event at 880ms, evdev repeat events at 500,625,750,875,1000ms). It would be significantly better than it was before, though, and I'll have to test it myself to see if it is good enough (though I fear it is not). > I can't work on it this weekend, as I'm about to leave Hungary to return back > home. I suspect that I'll have lots of fun next week, due to a one-week > travel, > and due to the .40 merge window (I suspect it will be opened next week). > > Maybe Jarod can find some time to do such patch and test it. > > Thanks, > Mauro. > -- Anssi Hannula -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] V4L: Extended crop/compose API
Hi Tomasz, On Wednesday 18 May 2011 18:55:51 Tomasz Stanislawski wrote: > Laurent Pinchart wrote: > > On Saturday 14 May 2011 12:50:32 Hans Verkuil wrote: > >> On Friday, May 13, 2011 14:43:08 Laurent Pinchart wrote: > >>> On Saturday 07 May 2011 13:52:25 Hans Verkuil wrote: > On Thursday, May 05, 2011 11:39:54 Tomasz Stanislawski wrote: > Hi Laurent and Hans, > I am very sorry for replying so lately. I was really busy during last week. > Thank you very much for your interest and comments :). No worries. I get more time to rest when you don't reply on the spot, so I don't mind :-) [snip] > > Applications set V4L2_SEL_TRY flag in v4l2_selection::flags to > > prevent a driver from applying selection configuration to hardware. > > I mentioned this before but I am very much opposed to this flag. It is > inconsistent with the TRY_FMT and TRY_EXT_CTRLS ioctls. Note that in > video_ioctl2 it should be just one function with 'try' boolean > argument. It has always been a mistake that the try and set functions > were separated in the driver code IMHO. > > I know that the subdev ioctls do not have a try version, but it is not > quite the same in that those try functions actually store the try > information. > >>> > >>> That's exactly why the subdevs pad-level API has a try flag instead of > >>> a try operation, and that's why g/s_selection on subdevs will be done > >>> with a try flag. > >>> > >>> As for the video device node API, I won't oppose a TRY ioctl, as long > >>> as we can guarantee there will never be dependencies between the > >>> selection rectangles and other parameters (or between the different > >>> selection rectangles). If the crop rectangle depends on the compose > >>> rectangle for instance, how can you implement a TRY ioctl to try a > >>> crop rectangle for a specific compose rectangle, without modifying the > >>> active compose rectangle first ? > >> > >> In that case the TRY would adjust the crop so that it works with the > >> current compose rectangle. > > > > And how do you try both crop and compose settings without modifying the > > active configuration ? That's not possible, and I think it's a bad API > > limitation. > > VIDIOC_TRY_MULTISELECTION ? > > >> But this is just one more example of our lack of proper support for > >> pipeline setup. It doesn't really matter whether this is at the subdev > >> level or at the driver level, both have the same problems. > >> > >> This requires a brainstorm session to work out. > >> > This is something we need to look into more carefully. I am slowly > becoming convinced that we need some sort of transaction-based > configuration for pipelines. > >>> > >>> This RFC is about video device node configuration, not pipelines. For > >>> pipelines I think we'll need a transaction-based API. For video device > >>> nodes, I'm still unsure. As stated above, if we have multiple > >>> parameters that depend on each other, how do we let the user try them > >>> without changing the active configuration ? > >> > >> Cropping/scaling/composing IS a pipeline. Until recently the V4L2 device > >> node API was sufficient to setup the trivial pipelines that most V4L2 > >> consumer devices have. But with the more modern devices it starts to > >> show its limitations. > > > > It's still a simple pipeline, and I think we should aim at making the > > V4L2 device node API useful to configure this kind of pipeline. The > > selection API is a superset of the crop API, applications should be able > > to use it to replace the crop API in the long term. > > > >> The whole 'try' concept we had for a long time needs to be re-examined. > > > > I agree. > > > >> As you remember, I was never satisfied with the subdev 'try' approach > >> either, but I never could come up with a better alternative. > > I've noticed that there are two different meaning of TRY flag > a) checking if a proposed configuration is applicable for a driver > b) storing proposed configuration in some form of temporary buffer > > Ad. a. This is a real TRY command. The state of both hardware and driver > does not change after TRY call. > > Ad. b. It should be called not TRY flag because the internal state of a > driver changes. It should be named as something like SHADOW flag. It > would indicate that the change is applied only to shadow configuration. > > I propose to change name of TRY flag for subdev to SHADOW flag. I think > it would much clear to express the difference of TRY meaning in video > node and subdev contexts. It's not a shadow configuration, it can't get applied to the active configuration (although that might open interesting opportunities). The TRY settings on subdevs are really TRY settings. They're local to the file handle, so you can have any number of unrelated TRY settings (limited by system resources limits of course). They're used to try settings, not to set a shadow configuration. > The
Re: [RFC] Standardize YUV support in the fbdev API
Hi Florian, On Saturday 21 May 2011 00:33:02 Florian Tobias Schandinat wrote: > On 05/17/2011 10:07 PM, Laurent Pinchart wrote: > > Hi everybody, > > > > I need to implement support for a YUV frame buffer in an fbdev driver. As > > the fbdev API doesn't support this out of the box, I've spent a couple > > of days reading fbdev (and KMS) code and thinking about how we could > > cleanly add YUV support to the API. I'd like to share my findings and > > thoughts, and hopefully receive some comments back. > > Thanks. I think you did already a good job, hope we can get it done this > time. Thanks. I'll keep pushing then :-) > > Given the overlap between the KMS, V4L2 and fbdev APIs, the need to share > > data and buffers between those subsystems, and the planned use of V4L2 > > FCCs in the KMS overlay API, I believe using V4L2 FCCs to identify fbdev > > formats would be a wise decision. > > I agree. There seems to be a general agreement on this, so I'll consider this settled. > > To select a frame buffer YUV format, the fb_var_screeninfo structure will > > need to be extended with a format field. The fbdev API and ABI must not > > be broken, which prevents us from changing the current structure layout > > and replacing the existing format selection mechanism (through the red, > > green, blue and alpha bitfields) completely. > > I agree. > > > - Other solutions are possible, such as adding new ioctls. Those > > solutions are more intrusive, and require larger changes to both > > userspace and kernelspace code. > > I'm against (ab)using the nonstd field (probably the only sane thing we can > do with it is declare it non-standard which interpretation is completely > dependent on the specific driver) or requiring previously unused fields to > have a special value so I'd like to suggest a different method: > > I remembered an earlier discussion: > [ http://marc.info/?l=linux-fbdev&m=129896686208130&w=2 ] > > On 03/01/2011 08:07 AM, Geert Uytterhoeven wrote: > > On Tue, Mar 1, 2011 at 04:13, Damian wrote: > >> On 2011/02/24 15:05, Geert Uytterhoeven wrote: > >>> For YUV (do you mean YCbCr?), I'm inclined to suggest adding a new > >>> FB_VISUAL_* > >>> type instead, which indicates the fb_var_screeninfo.{red,green,blue} > >>> fields are > >>> YCbCr instead of RGB. > >>> Depending on the frame buffer organization, you also need new > >>> FB_TYPE_*/FB_AUX_* > >>> types. > >> > >> I just wanted to clarify here. Is your comment about these new flags > >> in specific reference to this patch or to Magnus' "going forward" > >> comment? It > > > > About new flags. > > > >> seems like the beginnings of a method to standardize YCbCr support in > >> fbdev across all platforms. > >> Also, do I understand correctly that FB_VISUAL_ would specify the > >> colorspace > > > > FB_VISUAL_* specifies how pixel values (which may be tuples) are mapped > > to colors on the screen, so to me it looks like the sensible way to set > > up YCbCr. > > > >> (RGB, YCbCr), FB_TYPE_* would be a format specifier (i.e. planar, > >> semiplanar, interleaved, etc)? I'm not really sure what you are > >> referring to with the FB_AUX_* however. > > > > Yep, FB_TYPE_* specifies how pixel values/tuples are laid out in frame > > buffer memory. > > > > FB_AUX_* is only used if a specific value of FB_TYPE_* needs an > > additional parameter (e.g. the interleave value for interleaved > > bitplanes). > > Adding new standard values for these fb_fix_screeninfo fields would solve > the issue for framebuffers which only support a single format. I've never liked changing fixed screen information :-) It would be consistent with the API though. > If you have the need to switch Yes I need that. This requires an API to set the mode through fb_var_screeninfo, which is why I skipped modifying fb_fix_screeinfo. A new FB_TYPE_* could be used to report that we use a 4CC-based mode, with the exact mode reported in one of the fb_fix_screeninfo reserved fields (or the type_aux field). This would duplicate the information passed through fb_var_screeninfo though. Do you think it's worth it ? > I guess it would be a good idea to add a new flag to the vmode bitfield in > fb_var_screeninfo which looks like a general purpose modifier for the > videomode. You could than reuse any RGB-specific field you like to pass more > information. That looks good to me. The grayscale field could be reused to pass the 4CC. > Maybe we should also use this chance to declare one of the fix_screeninfo > reserved fields to be used for capability flags or an API version as we can > assume that those are 0 (at least in sane drivers). That's always good, although it's not a hard requirement for the purpose of YUV support. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-inf
Re: [ANNOUNCE] experimental alsa stream support at xawtv3
Em 23-05-2011 17:19, Devin Heitmueller escreveu: > On Mon, May 23, 2011 at 4:17 PM, Mauro Carvalho Chehab > wrote: >> Due to the alsa detection code that I've added at libv4l2util (at v4l2-utils) >> during the weekend, I decided to add alsa support also on xawtv3, basically >> to provide a real usecase example. Of course, for it to work, it needs the >> very latest v4l2-utils version from the git tree. >> >> I've basically added there the code that Devin wrote for tvtime, with a few >> small fixes and with the audio device auto-detection. > > If any of these fixes you made apply to the code in general, I will be > happy to merge them into our tvtime tree. Let me know. The code bellow will probably be more useful for you. It basically adds alsa device autodetection. The patch is against xawtv source code, but the code there is generic enough to be added on tvtime. Have fun! Mauro diff --git a/x11/xt.c b/x11/xt.c index 81658a0..7cf7281 100644 --- a/x11/xt.c +++ b/x11/xt.c @@ -22,7 +22,9 @@ #include #include #include - +#ifdef HAVE_LIBV42LUTIL +# include +#endif #if defined(__linux__) # include #include @@ -58,6 +60,7 @@ #include "blit.h" #include "parseconfig.h" #include "event.h" +#include "alsa_stream.h" /* jwz */ #include "remote.h" @@ -1377,6 +1380,11 @@ grabber_init() { struct ng_video_fmt screen; void *base = NULL; +#if defined(HAVE_V4L2UTIL) && defined(HAVE_ALSA) +struct media_devices *md; +unsigned int size = 0; +char *alsa_cap, *alsa_out, *p; +#endif memset(&screen,0,sizeof(screen)); #ifdef HAVE_LIBXXF86DGA @@ -1417,6 +1425,22 @@ grabber_init() } f_drv = drv->capabilities(h_drv); add_attrs(drv->list_attrs(h_drv)); + +#if defined(HAVE_V4L2UTIL) && defined(HAVE_ALSA) +/* Start audio capture thread */ +md = discover_media_devices(&size); +p = strrchr(args.device, '/'); +if (!p) + p = args.device; +alsa_cap = get_first_alsa_cap_device(md, size, p + 1); +alsa_out = get_first_no_video_out_device(md, size); + +printf("Alsa devices: cap: %s (%s), out: %s\n", alsa_cap, args.device, alsa_out); + +if (alsa_cap && alsa_out) +alsa_thread_startup(alsa_out, alsa_cap); +free_media_devices(md, size); +#endif } void -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ANNOUNCE] experimental alsa stream support at xawtv3
Em 23-05-2011 17:19, Devin Heitmueller escreveu: > On Mon, May 23, 2011 at 4:17 PM, Mauro Carvalho Chehab > wrote: >> Due to the alsa detection code that I've added at libv4l2util (at v4l2-utils) >> during the weekend, I decided to add alsa support also on xawtv3, basically >> to provide a real usecase example. Of course, for it to work, it needs the >> very latest v4l2-utils version from the git tree. >> >> I've basically added there the code that Devin wrote for tvtime, with a few >> small fixes and with the audio device auto-detection. > > If any of these fixes you made apply to the code in general, I will be > happy to merge them into our tvtime tree. Let me know. They are small. As I changed the non-public stuff to static, some warnings happened. I also added some logic there to avoid re-starting the thread when it is active, to allow stopping a stream and to check if the stream is running. The enclosed patch contains the diff from your version. As I'm using it at xawtv, I've removed tvtime_ prefix from the calls, so you'll probably need to fix it, or to change the function call inside tvtime. --- a/alsa_stream.c +++ b/alsa_stream.c @@ -2,7 +2,7 @@ * tvtime ALSA device support * * Copyright (c) by Devin Heitmueller - * + * * Derived from the alsa-driver test tool latency.c: *Copyright (c) by Jaroslav Kysela * @@ -23,6 +23,10 @@ * */ +#include "config.h" + +#ifdef HAVE_ALSA_ASOUNDLIB_H + #include #include #include @@ -33,6 +37,11 @@ #include #include #include +#include "alsa_stream.h" + +/* Private vars to control alsa thread status */ +static int alsa_is_running = 0; +static int stop_alsa = 0; snd_output_t *output = NULL; @@ -43,12 +52,12 @@ struct final_params { int channels; }; -int setparams_stream(snd_pcm_t *handle, -snd_pcm_hw_params_t *params, -snd_pcm_format_t format, -int channels, -int rate, -const char *id) +static int setparams_stream(snd_pcm_t *handle, + snd_pcm_hw_params_t *params, + snd_pcm_format_t format, + int channels, + int rate, + const char *id) { int err; unsigned int rrate; @@ -66,14 +75,14 @@ int setparams_stream(snd_pcm_t *handle, err = snd_pcm_hw_params_set_access(handle, params, SND_PCM_ACCESS_RW_INTERLEAVED); if (err < 0) { - printf("Access type not available for %s: %s\n", id, + printf("Access type not available for %s: %s\n", id, snd_strerror(err)); return err; } err = snd_pcm_hw_params_set_format(handle, params, format); if (err < 0) { - printf("Sample format not available for %s: %s\n", id, + printf("Sample format not available for %s: %s\n", id, snd_strerror(err)); return err; } @@ -129,10 +138,10 @@ int setparams_bufsize(snd_pcm_t *handle, return 0; } -int setparams_set(snd_pcm_t *handle, - snd_pcm_hw_params_t *params, - snd_pcm_sw_params_t *swparams, - const char *id) +static int setparams_set(snd_pcm_t *handle, +snd_pcm_hw_params_t *params, +snd_pcm_sw_params_t *swparams, +const char *id) { int err; @@ -181,7 +190,7 @@ int setparams(snd_pcm_t *phandle, snd_pcm_t *chandle, snd_pcm_format_t format, snd_pcm_sw_params_t *p_swparams, *c_swparams; snd_pcm_uframes_t p_size, c_size, p_psize, c_psize; unsigned int p_time, c_time; - + snd_pcm_hw_params_alloca(&p_params); snd_pcm_hw_params_alloca(&c_params); snd_pcm_hw_params_alloca(&pt_params); @@ -258,7 +267,7 @@ int setparams(snd_pcm_t *phandle, snd_pcm_t *chandle, snd_pcm_format_t format, printf("final config\n"); snd_pcm_dump_setup(phandle, output); snd_pcm_dump_setup(chandle, output); -printf("Parameters are %iHz, %s, %i channels\n", rate, +printf("Parameters are %iHz, %s, %i channels\n", rate, snd_pcm_format_name(format), channels); fflush(stdout); #endif @@ -270,25 +279,8 @@ int setparams(snd_pcm_t *phandle, snd_pcm_t *chandle, snd_pcm_format_t format, return 0; } -void setscheduler(void) -{ -struct sched_param sched_param; - -if (sched_getparam(0, &sched_param) < 0) { - printf("Scheduler getparam failed...\n"); - return; -} -sched_param.sched_priority = sched_get_priority_max(SCHED_RR); -if (!sched_setscheduler(0, SCHED_RR, &sched_param)) { - printf("Scheduler set to Round Robin with priority %i...\n", sched_param.sched_priority); - fflush(stdout); - return; -} -printf("!!!Scheduler set to Round Robin with priority %i FAILED!!!\n", sched_param.sched_priority); -} - -snd_pcm_sframes_t readbuf(snd_pcm_t *ha
Re: [ANNOUNCE] experimental alsa stream support at xawtv3
On Mon, May 23, 2011 at 4:17 PM, Mauro Carvalho Chehab wrote: > Due to the alsa detection code that I've added at libv4l2util (at v4l2-utils) > during the weekend, I decided to add alsa support also on xawtv3, basically > to provide a real usecase example. Of course, for it to work, it needs the > very latest v4l2-utils version from the git tree. > > I've basically added there the code that Devin wrote for tvtime, with a few > small fixes and with the audio device auto-detection. If any of these fixes you made apply to the code in general, I will be happy to merge them into our tvtime tree. Let me know. Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[ANNOUNCE] experimental alsa stream support at xawtv3
Due to the alsa detection code that I've added at libv4l2util (at v4l2-utils) during the weekend, I decided to add alsa support also on xawtv3, basically to provide a real usecase example. Of course, for it to work, it needs the very latest v4l2-utils version from the git tree. I've basically added there the code that Devin wrote for tvtime, with a few small fixes and with the audio device auto-detection. With this patch, xawtv will now get the alsa device associated with a video device node (if any), and start streaming from it, on a separate thread. As the code is the same as the one at tvtime, it should work at the same devices that are supported there. I tested it only on two em28xx devices: - HVR-950; - WinTV USB-2. It worked with HVR-950, but it didn't work with WinTV USB-2. It seems that snd-usb-audio do something different to set the framerate, that the alsa-stream code doesn't recognize. While I didn't test, I think it probably won't work with saa7134, as the code seems to hardcode the frame rate to 48 kHz, but saa7134 supports only 32 kHz. It would be good to add an option to disable this behavior and to allow manually select the alsa out device, so please send us patches ;) Anyway, patches fixing it and more tests are welcome. The git repositories for xawtv3 and v4l-utils is at: http://git.linuxtv.org/xawtv3.git http://git.linuxtv.org/v4l-utils.git Thanks, Mauro. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Alternate setting 1 must be selected for interface 0 on the model that I received. Else the rest is identical.
On Monday 23 May 2011 22:04:07 Mauro Carvalho Chehab wrote: > Em 23-05-2011 16:52, Hans Petter Selasky escreveu: > > On Monday 23 May 2011 21:47:57 Mauro Carvalho Chehab wrote: > >> Em 23-05-2011 16:17, Hans Petter Selasky escreveu: > >>> On Monday 23 May 2011 21:06:32 Mauro Carvalho Chehab wrote: > Em 23-05-2011 15:48, Hans Petter Selasky escreveu: > > On Monday 23 May 2011 20:14:45 Mauro Carvalho Chehab wrote: > >> Em 23-05-2011 11:37, Hans Petter Selasky escreveu: > >> > >> I don't have any ttusb device here, but I doubt that this would > >> work. > > > > Hi, > > > > It is already tested and works fine. > > This will work for you, but it will likely break for the others. Your > patch is assuming that returning an error if selecting alt 1 is enough > to know that alt 0 should be used. > > > What I see is that interface 1 does not have an alternate setting > > like the driver code expects, while interface 0 does. So it is the > > opposite of what the driver expects. Maybe the manufacturer changed > > something. Endpoints are still the same. > > That sometimes happen. Or maybe you just need a different size. > > > Please find attached an USB descriptor dump from this device. > > Int 0, endpoint 0: > Interface 0 > > bLength = 0x0009 > bDescriptorType = 0x0004 > bInterfaceNumber = 0x > bAlternateSetting = 0x > bNumEndpoints = 0x0003 > bInterfaceClass = 0x > bInterfaceSubClass = 0x > bInterfaceProtocol = 0x > iInterface = 0x > > ... > > Endpoint 2 > > bLength = 0x0007 > bDescriptorType = 0x0005 > bEndpointAddress = 0x0082 > bmAttributes = 0x0001 > wMaxPacketSize = 0x > bInterval = 0x0001 > bRefresh = 0x > bSynchAddress = 0x > > ... > > Interface 0 Alt 1 > > bLength = 0x0009 > bDescriptorType = 0x0004 > bInterfaceNumber = 0x > bAlternateSetting = 0x0001 > bNumEndpoints = 0x0003 > bInterfaceClass = 0x > bInterfaceSubClass = 0x > bInterfaceProtocol = 0x > iInterface = 0x > > ... > > Endpoint 2 > > bLength = 0x0007 > bDescriptorType = 0x0005 > bEndpointAddress = 0x0082 > bmAttributes = 0x0001 > wMaxPacketSize = 0x0390 > bInterval = 0x0001 > bRefresh = 0x > bSynchAddress = 0x > >>> > >>> Hi, > >>> > Hmm... assuming that the driver is using ISOC transfers, the > difference between alt 0 and alt 1 is that, on alt0, the > mwMaxPacketSize is 0 (so, you can't use it for isoc transfers), > while, on alt 1, wMaxPacketSize is 0x390. > > What the driver should be doing is to select an alt mode where the > wMaxPacketSize is big enough to handle the transfer. > >>> > >>> I can write the code to do that. Summed up: > >>> > >>> 1) Search interface 0, for alternate settings that have an ISOC-IN and > >>> wMaxPacket != 0. Select this alternate setting. > >>> > >>> 2) Search interface 1, for alternate settings that have an ISOC-IN and > >>> wMaxPacket != 0. Select this alternate setting. > >>> > >>> 3) Done. > >>> > >>> Do you think this will work better? > >>> > Calculating what "big enough" is device-dependent, but, basically, a > 480 Mbps USB bus is capable of providing 800 isoc slots per interval. > If the packets are bigger, the max bandwidth is bigger. > >>> > >>> This is a FULL speed device, max 10MBit/second. > >> > >> Hmm... USB 1.1 devices are even more limited on the amount of used > >> bandwidth. The above procedure is better than the one you've proposed, > >> but yet you may not be able to receive channels with higher bandwidths. > >> > >> The usb "max" limit is lower than the maximum bandwidth. I think that > >> full speed provides 900 isoc slots per interval, but the interval for > >> usb 1.1 is higher (1s, while the interval for usb 2.0 is 125 us), but > >> you need to double check such constraints at the USB 1.1 and 2.0 specs, > >> as I may be wrong on that, as I read it a long time ago ;) > > > > Hi, > > > > There are 1000 frames per second when using Full Speed USB. > > No, it is 900. See [1]: > > [1] > http://www.google.com.br/url?sa=t&source=web&cd=1&ved=0CBgQFjAA&url=http%3 > A%2F%2Fmprolab.teipir.gr%2Fvivlio80X86%2Fusb11.pdf&ei=DrzaTfD5HqW90AHY-Jn8A > w&usg=AFQjCNHZW7ogFrjqoim1lTduQTHTDJoAUg > > "5.6.4 Isochronous Transfer Bus Access Constraints > Isochronous transfers can be used only by full-speed devices. > The USB r
Re: [PATCH] Alternate setting 1 must be selected for interface 0 on the model that I received. Else the rest is identical.
Em 23-05-2011 16:52, Hans Petter Selasky escreveu: > On Monday 23 May 2011 21:47:57 Mauro Carvalho Chehab wrote: >> Em 23-05-2011 16:17, Hans Petter Selasky escreveu: >>> On Monday 23 May 2011 21:06:32 Mauro Carvalho Chehab wrote: Em 23-05-2011 15:48, Hans Petter Selasky escreveu: > On Monday 23 May 2011 20:14:45 Mauro Carvalho Chehab wrote: >> Em 23-05-2011 11:37, Hans Petter Selasky escreveu: >> >> I don't have any ttusb device here, but I doubt that this would work. > > Hi, > > It is already tested and works fine. This will work for you, but it will likely break for the others. Your patch is assuming that returning an error if selecting alt 1 is enough to know that alt 0 should be used. > What I see is that interface 1 does not have an alternate setting like > the driver code expects, while interface 0 does. So it is the opposite > of what the driver expects. Maybe the manufacturer changed something. > Endpoints are still the same. That sometimes happen. Or maybe you just need a different size. > Please find attached an USB descriptor dump from this device. Int 0, endpoint 0: Interface 0 bLength = 0x0009 bDescriptorType = 0x0004 bInterfaceNumber = 0x bAlternateSetting = 0x bNumEndpoints = 0x0003 bInterfaceClass = 0x bInterfaceSubClass = 0x bInterfaceProtocol = 0x iInterface = 0x ... Endpoint 2 bLength = 0x0007 bDescriptorType = 0x0005 bEndpointAddress = 0x0082 bmAttributes = 0x0001 wMaxPacketSize = 0x bInterval = 0x0001 bRefresh = 0x bSynchAddress = 0x ... Interface 0 Alt 1 bLength = 0x0009 bDescriptorType = 0x0004 bInterfaceNumber = 0x bAlternateSetting = 0x0001 bNumEndpoints = 0x0003 bInterfaceClass = 0x bInterfaceSubClass = 0x bInterfaceProtocol = 0x iInterface = 0x ... Endpoint 2 bLength = 0x0007 bDescriptorType = 0x0005 bEndpointAddress = 0x0082 bmAttributes = 0x0001 wMaxPacketSize = 0x0390 bInterval = 0x0001 bRefresh = 0x bSynchAddress = 0x >>> >>> Hi, >>> Hmm... assuming that the driver is using ISOC transfers, the difference between alt 0 and alt 1 is that, on alt0, the mwMaxPacketSize is 0 (so, you can't use it for isoc transfers), while, on alt 1, wMaxPacketSize is 0x390. What the driver should be doing is to select an alt mode where the wMaxPacketSize is big enough to handle the transfer. >>> >>> I can write the code to do that. Summed up: >>> >>> 1) Search interface 0, for alternate settings that have an ISOC-IN and >>> wMaxPacket != 0. Select this alternate setting. >>> >>> 2) Search interface 1, for alternate settings that have an ISOC-IN and >>> wMaxPacket != 0. Select this alternate setting. >>> >>> 3) Done. >>> >>> Do you think this will work better? >>> Calculating what "big enough" is device-dependent, but, basically, a 480 Mbps USB bus is capable of providing 800 isoc slots per interval. If the packets are bigger, the max bandwidth is bigger. >>> >>> This is a FULL speed device, max 10MBit/second. >> >> Hmm... USB 1.1 devices are even more limited on the amount of used >> bandwidth. The above procedure is better than the one you've proposed, but >> yet you may not be able to receive channels with higher bandwidths. >> >> The usb "max" limit is lower than the maximum bandwidth. I think that full >> speed provides 900 isoc slots per interval, but the interval for usb 1.1 is >> higher (1s, while the interval for usb 2.0 is 125 us), but you need to >> double check such constraints at the USB 1.1 and 2.0 specs, as I may be >> wrong on that, as I read it a long time ago ;) > > Hi, > > There are 1000 frames per second when using Full Speed USB. No, it is 900. See [1]: [1] http://www.google.com.br/url?sa=t&source=web&cd=1&ved=0CBgQFjAA&url=http%3A%2F%2Fmprolab.teipir.gr%2Fvivlio80X86%2Fusb11.pdf&ei=DrzaTfD5HqW90AHY-Jn8Aw&usg=AFQjCNHZW7ogFrjqoim1lTduQTHTDJoAUg "5.6.4 Isochronous Transfer Bus Access Constraints Isochronous transfers can be used only by full-speed devices. The USB requires that no more than 90% of any frame be allocated for periodic (isochronous and interrupt) transfers." 90% of 1000 frames is 900 frames/s. That's the maximum limit for ISOC transfers. > I know the device > cannot receive all streams, but it is very well suited for DVB radio. Yeah, for DVB radio, that's ok, but the driver should be ok also for ot
Re: [PATCH] Alternate setting 1 must be selected for interface 0 on the model that I received. Else the rest is identical.
On Monday 23 May 2011 21:47:57 Mauro Carvalho Chehab wrote: > Em 23-05-2011 16:17, Hans Petter Selasky escreveu: > > On Monday 23 May 2011 21:06:32 Mauro Carvalho Chehab wrote: > >> Em 23-05-2011 15:48, Hans Petter Selasky escreveu: > >>> On Monday 23 May 2011 20:14:45 Mauro Carvalho Chehab wrote: > Em 23-05-2011 11:37, Hans Petter Selasky escreveu: > > I don't have any ttusb device here, but I doubt that this would work. > >>> > >>> Hi, > >>> > >>> It is already tested and works fine. > >> > >> This will work for you, but it will likely break for the others. Your > >> patch is assuming that returning an error if selecting alt 1 is enough > >> to know that alt 0 should be used. > >> > >>> What I see is that interface 1 does not have an alternate setting like > >>> the driver code expects, while interface 0 does. So it is the opposite > >>> of what the driver expects. Maybe the manufacturer changed something. > >>> Endpoints are still the same. > >> > >> That sometimes happen. Or maybe you just need a different size. > >> > >>> Please find attached an USB descriptor dump from this device. > >> > >> Int 0, endpoint 0: > >> Interface 0 > >> > >> bLength = 0x0009 > >> bDescriptorType = 0x0004 > >> bInterfaceNumber = 0x > >> bAlternateSetting = 0x > >> bNumEndpoints = 0x0003 > >> bInterfaceClass = 0x > >> bInterfaceSubClass = 0x > >> bInterfaceProtocol = 0x > >> iInterface = 0x > >> > >> ... > >> > >> Endpoint 2 > >> > >> bLength = 0x0007 > >> bDescriptorType = 0x0005 > >> bEndpointAddress = 0x0082 > >> bmAttributes = 0x0001 > >> wMaxPacketSize = 0x > >> bInterval = 0x0001 > >> bRefresh = 0x > >> bSynchAddress = 0x > >> > >> ... > >> > >> Interface 0 Alt 1 > >> > >> bLength = 0x0009 > >> bDescriptorType = 0x0004 > >> bInterfaceNumber = 0x > >> bAlternateSetting = 0x0001 > >> bNumEndpoints = 0x0003 > >> bInterfaceClass = 0x > >> bInterfaceSubClass = 0x > >> bInterfaceProtocol = 0x > >> iInterface = 0x > >> > >> ... > >> > >> Endpoint 2 > >> > >> bLength = 0x0007 > >> bDescriptorType = 0x0005 > >> bEndpointAddress = 0x0082 > >> bmAttributes = 0x0001 > >> wMaxPacketSize = 0x0390 > >> bInterval = 0x0001 > >> bRefresh = 0x > >> bSynchAddress = 0x > > > > Hi, > > > >> Hmm... assuming that the driver is using ISOC transfers, the difference > >> between alt 0 and alt 1 is that, on alt0, the mwMaxPacketSize is 0 (so, > >> you can't use it for isoc transfers), while, on alt 1, wMaxPacketSize is > >> 0x390. > >> > >> What the driver should be doing is to select an alt mode where the > >> wMaxPacketSize is big enough to handle the transfer. > > > > I can write the code to do that. Summed up: > > > > 1) Search interface 0, for alternate settings that have an ISOC-IN and > > wMaxPacket != 0. Select this alternate setting. > > > > 2) Search interface 1, for alternate settings that have an ISOC-IN and > > wMaxPacket != 0. Select this alternate setting. > > > > 3) Done. > > > > Do you think this will work better? > > > >> Calculating what "big enough" is device-dependent, but, basically, a > >> 480 Mbps USB bus is capable of providing 800 isoc slots per interval. > >> If the packets are bigger, the max bandwidth is bigger. > > > > This is a FULL speed device, max 10MBit/second. > > Hmm... USB 1.1 devices are even more limited on the amount of used > bandwidth. The above procedure is better than the one you've proposed, but > yet you may not be able to receive channels with higher bandwidths. > > The usb "max" limit is lower than the maximum bandwidth. I think that full > speed provides 900 isoc slots per interval, but the interval for usb 1.1 is > higher (1s, while the interval for usb 2.0 is 125 us), but you need to > double check such constraints at the USB 1.1 and 2.0 specs, as I may be > wrong on that, as I read it a long time ago ;) Hi, There are 1000 frames per second when using Full Speed USB. I know the device cannot receive all streams, but it is very well suited for DVB radio. --HPS -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Alternate setting 1 must be selected for interface 0 on the model that I received. Else the rest is identical.
Em 23-05-2011 16:17, Hans Petter Selasky escreveu: > On Monday 23 May 2011 21:06:32 Mauro Carvalho Chehab wrote: >> Em 23-05-2011 15:48, Hans Petter Selasky escreveu: >>> On Monday 23 May 2011 20:14:45 Mauro Carvalho Chehab wrote: Em 23-05-2011 11:37, Hans Petter Selasky escreveu: I don't have any ttusb device here, but I doubt that this would work. >>> >>> Hi, >>> >>> It is already tested and works fine. >> >> This will work for you, but it will likely break for the others. Your patch >> is assuming that returning an error if selecting alt 1 is enough to know >> that alt 0 should be used. >> >>> What I see is that interface 1 does not have an alternate setting like >>> the driver code expects, while interface 0 does. So it is the opposite >>> of what the driver expects. Maybe the manufacturer changed something. >>> Endpoints are still the same. >> >> That sometimes happen. Or maybe you just need a different size. >> >>> Please find attached an USB descriptor dump from this device. >> >> Int 0, endpoint 0: >> >> Interface 0 >> bLength = 0x0009 >> bDescriptorType = 0x0004 >> bInterfaceNumber = 0x >> bAlternateSetting = 0x >> bNumEndpoints = 0x0003 >> bInterfaceClass = 0x >> bInterfaceSubClass = 0x >> bInterfaceProtocol = 0x >> iInterface = 0x >> >> ... >> >> Endpoint 2 >> bLength = 0x0007 >> bDescriptorType = 0x0005 >> bEndpointAddress = 0x0082 >> bmAttributes = 0x0001 >> wMaxPacketSize = 0x >> bInterval = 0x0001 >> bRefresh = 0x >> bSynchAddress = 0x >> >> ... >> >> Interface 0 Alt 1 >> bLength = 0x0009 >> bDescriptorType = 0x0004 >> bInterfaceNumber = 0x >> bAlternateSetting = 0x0001 >> bNumEndpoints = 0x0003 >> bInterfaceClass = 0x >> bInterfaceSubClass = 0x >> bInterfaceProtocol = 0x >> iInterface = 0x >> >> ... >> Endpoint 2 >> bLength = 0x0007 >> bDescriptorType = 0x0005 >> bEndpointAddress = 0x0082 >> bmAttributes = 0x0001 >> wMaxPacketSize = 0x0390 >> bInterval = 0x0001 >> bRefresh = 0x >> bSynchAddress = 0x >> > > Hi, > >> Hmm... assuming that the driver is using ISOC transfers, the difference >> between alt 0 and alt 1 is that, on alt0, the mwMaxPacketSize is 0 (so, >> you can't use it for isoc transfers), while, on alt 1, wMaxPacketSize is >> 0x390. >> >> What the driver should be doing is to select an alt mode where the >> wMaxPacketSize is big enough to handle the transfer. > > I can write the code to do that. Summed up: > > 1) Search interface 0, for alternate settings that have an ISOC-IN and > wMaxPacket != 0. Select this alternate setting. > > 2) Search interface 1, for alternate settings that have an ISOC-IN and > wMaxPacket != 0. Select this alternate setting. > > 3) Done. > > Do you think this will work better? > >> Calculating what "big enough" is device-dependent, but, basically, a 480 >> Mbps USB bus is capable of providing 800 isoc slots per interval. If the >> packets are bigger, the max bandwidth is bigger. > > This is a FULL speed device, max 10MBit/second. Hmm... USB 1.1 devices are even more limited on the amount of used bandwidth. The above procedure is better than the one you've proposed, but yet you may not be able to receive channels with higher bandwidths. The usb "max" limit is lower than the maximum bandwidth. I think that full speed provides 900 isoc slots per interval, but the interval for usb 1.1 is higher (1s, while the interval for usb 2.0 is 125 us), but you need to double check such constraints at the USB 1.1 and 2.0 specs, as I may be wrong on that, as I read it a long time ago ;) The proper way would be to have a function that would dynamically select the alternate depending on the channel bandwidth and on the stream needs. Someone might of course just write a code that selects the highest wMaxPacket, but this would cause some troubles if another device is connected to the USB 1.1 bus. > >> You're able to see the amount of packets per interval by doing a cat >> /proc/bus/usb/devices: >> >> T: Bus=01 Lev=00 Prnt=00 Port=00 Cnt=00 Dev#= 1 Spd=480 MxCh= 8 >> B: Alloc= 0/800 us ( 0%), #Int= 0, #Iso= 0 >> >> The "B:" line above shows the USB bandwidth usage. > > Do you need this information to proceed with the patch? > > --HPS -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Alternate setting 1 must be selected for interface 0 on the model that I received. Else the rest is identical.
On Monday 23 May 2011 21:06:32 Mauro Carvalho Chehab wrote: > Em 23-05-2011 15:48, Hans Petter Selasky escreveu: > > On Monday 23 May 2011 20:14:45 Mauro Carvalho Chehab wrote: > >> Em 23-05-2011 11:37, Hans Petter Selasky escreveu: > >> > >> I don't have any ttusb device here, but I doubt that this would work. > > > > Hi, > > > > It is already tested and works fine. > > This will work for you, but it will likely break for the others. Your patch > is assuming that returning an error if selecting alt 1 is enough to know > that alt 0 should be used. > > > What I see is that interface 1 does not have an alternate setting like > > the driver code expects, while interface 0 does. So it is the opposite > > of what the driver expects. Maybe the manufacturer changed something. > > Endpoints are still the same. > > That sometimes happen. Or maybe you just need a different size. > > > Please find attached an USB descriptor dump from this device. > > Int 0, endpoint 0: > > Interface 0 > bLength = 0x0009 > bDescriptorType = 0x0004 > bInterfaceNumber = 0x > bAlternateSetting = 0x > bNumEndpoints = 0x0003 > bInterfaceClass = 0x > bInterfaceSubClass = 0x > bInterfaceProtocol = 0x > iInterface = 0x > > ... > > Endpoint 2 > bLength = 0x0007 > bDescriptorType = 0x0005 > bEndpointAddress = 0x0082 > bmAttributes = 0x0001 > wMaxPacketSize = 0x > bInterval = 0x0001 > bRefresh = 0x > bSynchAddress = 0x > > ... > > Interface 0 Alt 1 > bLength = 0x0009 > bDescriptorType = 0x0004 > bInterfaceNumber = 0x > bAlternateSetting = 0x0001 > bNumEndpoints = 0x0003 > bInterfaceClass = 0x > bInterfaceSubClass = 0x > bInterfaceProtocol = 0x > iInterface = 0x > > ... > Endpoint 2 > bLength = 0x0007 > bDescriptorType = 0x0005 > bEndpointAddress = 0x0082 > bmAttributes = 0x0001 > wMaxPacketSize = 0x0390 > bInterval = 0x0001 > bRefresh = 0x > bSynchAddress = 0x > Hi, > Hmm... assuming that the driver is using ISOC transfers, the difference > between alt 0 and alt 1 is that, on alt0, the mwMaxPacketSize is 0 (so, > you can't use it for isoc transfers), while, on alt 1, wMaxPacketSize is > 0x390. > > What the driver should be doing is to select an alt mode where the > wMaxPacketSize is big enough to handle the transfer. I can write the code to do that. Summed up: 1) Search interface 0, for alternate settings that have an ISOC-IN and wMaxPacket != 0. Select this alternate setting. 2) Search interface 1, for alternate settings that have an ISOC-IN and wMaxPacket != 0. Select this alternate setting. 3) Done. Do you think this will work better? > Calculating what "big enough" is device-dependent, but, basically, a 480 > Mbps USB bus is capable of providing 800 isoc slots per interval. If the > packets are bigger, the max bandwidth is bigger. This is a FULL speed device, max 10MBit/second. > You're able to see the amount of packets per interval by doing a cat > /proc/bus/usb/devices: > > T: Bus=01 Lev=00 Prnt=00 Port=00 Cnt=00 Dev#= 1 Spd=480 MxCh= 8 > B: Alloc= 0/800 us ( 0%), #Int= 0, #Iso= 0 > > The "B:" line above shows the USB bandwidth usage. Do you need this information to proceed with the patch? --HPS -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Alternate setting 1 must be selected for interface 0 on the model that I received. Else the rest is identical.
Em 23-05-2011 15:48, Hans Petter Selasky escreveu: > On Monday 23 May 2011 20:14:45 Mauro Carvalho Chehab wrote: >> Em 23-05-2011 11:37, Hans Petter Selasky escreveu: >> >> I don't have any ttusb device here, but I doubt that this would work. > > Hi, > > It is already tested and works fine. This will work for you, but it will likely break for the others. Your patch is assuming that returning an error if selecting alt 1 is enough to know that alt 0 should be used. > What I see is that interface 1 does not have an alternate setting like the > driver code expects, while interface 0 does. So it is the opposite of what > the > driver expects. Maybe the manufacturer changed something. Endpoints are still > the same. That sometimes happen. Or maybe you just need a different size. > > Please find attached an USB descriptor dump from this device. Int 0, endpoint 0: Interface 0 bLength = 0x0009 bDescriptorType = 0x0004 bInterfaceNumber = 0x bAlternateSetting = 0x bNumEndpoints = 0x0003 bInterfaceClass = 0x bInterfaceSubClass = 0x bInterfaceProtocol = 0x iInterface = 0x ... Endpoint 2 bLength = 0x0007 bDescriptorType = 0x0005 bEndpointAddress = 0x0082 bmAttributes = 0x0001 wMaxPacketSize = 0x bInterval = 0x0001 bRefresh = 0x bSynchAddress = 0x ... Interface 0 Alt 1 bLength = 0x0009 bDescriptorType = 0x0004 bInterfaceNumber = 0x bAlternateSetting = 0x0001 bNumEndpoints = 0x0003 bInterfaceClass = 0x bInterfaceSubClass = 0x bInterfaceProtocol = 0x iInterface = 0x ... Endpoint 2 bLength = 0x0007 bDescriptorType = 0x0005 bEndpointAddress = 0x0082 bmAttributes = 0x0001 wMaxPacketSize = 0x0390 bInterval = 0x0001 bRefresh = 0x bSynchAddress = 0x Hmm... assuming that the driver is using ISOC transfers, the difference between alt 0 and alt 1 is that, on alt0, the mwMaxPacketSize is 0 (so, you can't use it for isoc transfers), while, on alt 1, wMaxPacketSize is 0x390. What the driver should be doing is to select an alt mode where the wMaxPacketSize is big enough to handle the transfer. Calculating what "big enough" is device-dependent, but, basically, a 480 Mbps USB bus is capable of providing 800 isoc slots per interval. If the packets are bigger, the max bandwidth is bigger. You're able to see the amount of packets per interval by doing a cat /proc/bus/usb/devices: T: Bus=01 Lev=00 Prnt=00 Port=00 Cnt=00 Dev#= 1 Spd=480 MxCh= 8 B: Alloc= 0/800 us ( 0%), #Int= 0, #Iso= 0 The "B:" line above shows the USB bandwidth usage. > >> >> Alternates should be selected depending on the bandwidth needed. The right >> way is to write some logic that will get the maximum packet size for each >> mode, between the alternates that provide the type of transfer (Bulk or >> ISOC) accepted by the driver. > > Right, but this driver doesn't do this. It only selects a working one. > > --HPS -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Make code more readable by not using the return value of the WARN() macro. Set ret variable in an undefined case.
On Monday 23 May 2011 20:22:02 Guennadi Liakhovetski wrote: > Please, inline patches. Otherwise, this is what one gets, when replying. > > On Mon, 23 May 2011, Hans Petter Selasky wrote: > > --HPS > > In any case, just throwing in my 2 cents - no idea how not using the > return value of WARN() makes code more readable. On the contrary, using it > is a standard practice. This patch doesn't seem like an improvement to me. There is no strong reason for the WARN() part, you may ignore that, but the ret = 0, part is still valid. Should I generate a new patch or can you handle this? --HPS -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Inlined functions should be static.
On Monday 23 May 2011 20:29:28 Mauro Carvalho Chehab wrote: > Em 23-05-2011 15:23, Guennadi Liakhovetski escreveu: > > On Mon, 23 May 2011, Hans Petter Selasky wrote: > >> --HPS > > > > (again, inlining would save me copy-pasting) > > Yeah... hard to comment not-inlined patches... > > >> -inline u32 stb0899_do_div(u64 n, u32 d) > >> +static inline u32 stb0899_do_div(u64 n, u32 d) > > > > while at it you could as well remove the unneeded in a C file "inline" > > attribute. > > hmm... foo_do_div()... it seems to be yet-another-implementation > of asm/div64.h. If so, it is better to just remove this thing > and use the existing function. > The reason for this patch is that some version of GCC generated some garbage code on this function under certain conditions. Removing inline completly on this static function in a C file is fine by me. Do I need to create another patch? --HPS -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Alternate setting 1 must be selected for interface 0 on the model that I received. Else the rest is identical.
On Monday 23 May 2011 20:14:45 Mauro Carvalho Chehab wrote: > Em 23-05-2011 11:37, Hans Petter Selasky escreveu: > > I don't have any ttusb device here, but I doubt that this would work. Hi, It is already tested and works fine. What I see is that interface 1 does not have an alternate setting like the driver code expects, while interface 0 does. So it is the opposite of what the driver expects. Maybe the manufacturer changed something. Endpoints are still the same. Please find attached an USB descriptor dump from this device. > > Alternates should be selected depending on the bandwidth needed. The right > way is to write some logic that will get the maximum packet size for each > mode, between the alternates that provide the type of transfer (Bulk or > ISOC) accepted by the driver. Right, but this driver doesn't do this. It only selects a working one. --HPS ugen6.2: at usbus6, cfg=0 md=HOST spd=FULL (12Mbps) pwr=ON bLength = 0x0012 bDescriptorType = 0x0001 bcdUSB = 0x0100 bDeviceClass = 0x bDeviceSubClass = 0x bDeviceProtocol = 0x bMaxPacketSize0 = 0x0008 idVendor = 0x0b48 idProduct = 0x1005 bcdDevice = 0x0100 iManufacturer = 0x0001 iProduct = 0x0002 iSerialNumber = 0x bNumConfigurations = 0x0001 Configuration index 0 bLength = 0x0009 bDescriptorType = 0x0002 wTotalLength = 0x019f bNumInterfaces = 0x0002 bConfigurationValue = 0x0001 iConfiguration = 0x0004 bmAttributes = 0x0040 bMaxPower = 0x Interface 0 bLength = 0x0009 bDescriptorType = 0x0004 bInterfaceNumber = 0x bAlternateSetting = 0x bNumEndpoints = 0x0003 bInterfaceClass = 0x bInterfaceSubClass = 0x bInterfaceProtocol = 0x iInterface = 0x Endpoint 0 bLength = 0x0007 bDescriptorType = 0x0005 bEndpointAddress = 0x0081 bmAttributes = 0x0002 wMaxPacketSize = 0x0020 bInterval = 0x bRefresh = 0x bSynchAddress = 0x Endpoint 1 bLength = 0x0007 bDescriptorType = 0x0005 bEndpointAddress = 0x0001 bmAttributes = 0x0002 wMaxPacketSize = 0x0020 bInterval = 0x bRefresh = 0x bSynchAddress = 0x Endpoint 2 bLength = 0x0007 bDescriptorType = 0x0005 bEndpointAddress = 0x0082 bmAttributes = 0x0001 wMaxPacketSize = 0x bInterval = 0x0001 bRefresh = 0x bSynchAddress = 0x Interface 0 Alt 1 bLength = 0x0009 bDescriptorType = 0x0004 bInterfaceNumber = 0x bAlternateSetting = 0x0001 bNumEndpoints = 0x0003 bInterfaceClass = 0x bInterfaceSubClass = 0x bInterfaceProtocol = 0x iInterface = 0x Endpoint 0 bLength = 0x0007 bDescriptorType = 0x0005 bEndpointAddress = 0x0081 bmAttributes = 0x0002 wMaxPacketSize = 0x0020 bInterval = 0x bRefresh = 0x bSynchAddress = 0x Endpoint 1 bLength = 0x0007 bDescriptorType = 0x0005 bEndpointAddress = 0x0001 bmAttributes = 0x0002 wMaxPacketSize = 0x0020 bInterval = 0x bRefresh = 0x bSynchAddress = 0x Endpoint 2 bLength = 0x0007 bDescriptorType = 0x0005 bEndpointAddress = 0x0082 bmAttributes = 0x0001 wMaxPacketSize = 0x0390 bInterval = 0x0001 bRefresh = 0x bSynchAddress = 0x Interface 0 Alt 2 bLength = 0x0009 bDescriptorType = 0x0004 bInterfaceNumber = 0x bAlternateSetting = 0x0002 bNumEndpoints = 0x0003 bInterfaceClass = 0x bInterfaceSubClass = 0x bInterfaceProtocol = 0x iInterface = 0x Endpoint 0 bLength = 0x0007 bDescriptorType = 0x0005 bEndpointAddress = 0x0081 bmAttributes = 0x0002 wMaxPacketSize = 0x0020 bInterval = 0x bRefresh = 0x bSynchAddress = 0x Endpoint 1 bLength = 0x0007 bDescriptorType = 0x0005 bEndpointAddress = 0x0001 bmAttributes = 0x0002 wMaxPacketSize = 0x0020 bInterval = 0x bRefresh = 0x bSynchAddress = 0x Endpoint 2 bLength = 0x0007 bDescriptorType = 0x0005 bEndpointAddress = 0x0082 bmAttributes = 0x0001 wMaxPacketSize = 0x0360 bInterval = 0x0001 bRefresh = 0x bSynchAddress = 0x Interface 0 Alt 3 bLength = 0x0009 bDescriptorType = 0x0004 bInterfaceNumber = 0x bAlternateSetting = 0x0003 bNumEnd
[cron job] v4l-dvb daily build: ERRORS
This message is generated daily by a cron job that builds v4l-dvb for the kernels and architectures in the list below. Results of the daily build of v4l-dvb: date:Mon May 23 19:00:36 CEST 2011 git hash:f9b51477fe540fb4c65a05027fdd6f2ecce4db3b gcc version: i686-linux-gcc (GCC) 4.5.1 host hardware:x86_64 host os: 2.6.32.5 linux-git-armv5: OK linux-git-armv5-davinci: OK linux-git-armv5-ixp: OK linux-git-armv5-omap2: OK linux-git-i686: OK linux-git-m32r: OK linux-git-mips: WARNINGS linux-git-powerpc64: OK linux-git-x86_64: OK linux-2.6.31.12-i686: ERRORS linux-2.6.32.6-i686: WARNINGS linux-2.6.33-i686: WARNINGS linux-2.6.34-i686: WARNINGS linux-2.6.35.3-i686: WARNINGS linux-2.6.36-i686: WARNINGS linux-2.6.37-i686: WARNINGS linux-2.6.38.2-i686: WARNINGS linux-2.6.31.12-x86_64: ERRORS linux-2.6.32.6-x86_64: WARNINGS linux-2.6.33-x86_64: WARNINGS linux-2.6.34-x86_64: WARNINGS linux-2.6.35.3-x86_64: WARNINGS linux-2.6.36-x86_64: WARNINGS linux-2.6.37-x86_64: WARNINGS linux-2.6.38.2-x86_64: WARNINGS spec-git: OK sparse: ERRORS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Monday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Monday.tar.bz2 The V4L-DVB specification from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/media.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Inlined functions should be static.
Em 23-05-2011 15:23, Guennadi Liakhovetski escreveu: > On Mon, 23 May 2011, Hans Petter Selasky wrote: > >> --HPS >> > > (again, inlining would save me copy-pasting) Yeah... hard to comment not-inlined patches... > >> -inline u32 stb0899_do_div(u64 n, u32 d) >> +static inline u32 stb0899_do_div(u64 n, u32 d) > > while at it you could as well remove the unneeded in a C file "inline" > attribute. hmm... foo_do_div()... it seems to be yet-another-implementation of asm/div64.h. If so, it is better to just remove this thing and use the existing function. > > Thanks > Guennadi > --- > Guennadi Liakhovetski, Ph.D. > Freelance Open-Source Software Developer > http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Inlined functions should be static.
On Mon, 23 May 2011, Hans Petter Selasky wrote: > --HPS > (again, inlining would save me copy-pasting) > -inline u32 stb0899_do_div(u64 n, u32 d) > +static inline u32 stb0899_do_div(u64 n, u32 d) while at it you could as well remove the unneeded in a C file "inline" attribute. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Make code more readable by not using the return value of the WARN() macro. Set ret variable in an undefined case.
Please, inline patches. Otherwise, this is what one gets, when replying. On Mon, 23 May 2011, Hans Petter Selasky wrote: > --HPS > In any case, just throwing in my 2 cents - no idea how not using the return value of WARN() makes code more readable. On the contrary, using it is a standard practice. This patch doesn't seem like an improvement to me. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Alternate setting 1 must be selected for interface 0 on the model that I received. Else the rest is identical.
Em 23-05-2011 11:37, Hans Petter Selasky escreveu: > -HPS > > > dvb-usb-0016.patch > > > From 3cf61d6a77b22f58471188cd0e7e3dc6c3a29b0b Mon Sep 17 00:00:00 2001 > From: Hans Petter Selasky > Date: Mon, 23 May 2011 16:36:55 +0200 > Subject: [PATCH] Alternate setting 1 must be selected for interface 0 on the > model that I received. Else the rest is identical. > > Signed-off-by: Hans Petter Selasky > --- > drivers/media/dvb/ttusb-budget/dvb-ttusb-budget.c |8 > 1 files changed, 8 insertions(+), 0 deletions(-) > > diff --git a/drivers/media/dvb/ttusb-budget/dvb-ttusb-budget.c > b/drivers/media/dvb/ttusb-budget/dvb-ttusb-budget.c > index cbe2f0d..38a7d03 100644 > --- a/drivers/media/dvb/ttusb-budget/dvb-ttusb-budget.c > +++ b/drivers/media/dvb/ttusb-budget/dvb-ttusb-budget.c > @@ -971,6 +971,14 @@ static int ttusb_stop_feed(struct dvb_demux_feed > *dvbdmxfeed) > > static int ttusb_setup_interfaces(struct ttusb *ttusb) > { > + /* > + * Try to select alternate setting 1 for first interface. If > + * that does not work, restore to alternate setting 0. > + */ > + if (usb_set_interface(ttusb->dev, 0, 1) < 0) > + usb_set_interface(ttusb->dev, 0, 0); > + > + /* Select alternate setting 1 for second interface. */ > usb_set_interface(ttusb->dev, 1, 1); > > ttusb->bulk_out_pipe = usb_sndbulkpipe(ttusb->dev, 1); > -- 1.7.1.1 I don't have any ttusb device here, but I doubt that this would work. Alternates should be selected depending on the bandwidth needed. The right way is to write some logic that will get the maximum packet size for each mode, between the alternates that provide the type of transfer (Bulk or ISOC) accepted by the driver. You may take a look at staging/tm6000 or at drivers/media/video/em28xx or at drivers/media/video/gspca to see a few examples on how to do that. IMHO, the alternates selection code at tm6000 is in better shape than the others I know. Cheers, Mauro. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] omap3: isp: fix compiler warning
> -Original Message- > From: Laurent Pinchart [mailto:laurent.pinch...@ideasonboard.com] > Sent: Monday, May 23, 2011 12:56 AM > To: Mauro Carvalho Chehab > Cc: Premi, Sanjeev; linux-media@vger.kernel.org > Subject: Re: [PATCH] omap3: isp: fix compiler warning > > Hi Mauro and Sanjeev, > > On Saturday 21 May 2011 12:55:32 Mauro Carvalho Chehab wrote: > > Em 18-05-2011 13:06, Sanjeev Premi escreveu: > > > This patch fixes this compiler warning: > > > drivers/media/video/omap3isp/isp.c: In function 'isp_isr_dbg': > > > drivers/media/video/omap3isp/isp.c:392:2: warning: zero-length > > > > > >gnu_printf format string > > > > > > Since printk() is used in next few statements, same was used > > > here as well. > > > > > > Signed-off-by: Sanjeev Premi > > > Cc: laurent.pinch...@ideasonboard.com > > > --- > > > > > > Actually full block can be converted to dev_dbg() > > > as well; but i am not sure about original intent > > > of the mix. > > > > > > Based on comments, i can resubmit with all prints > > > converted to dev_dbg. > > > > It is probably better to convert the full block to dev_dbg. > > You can't insert a KERN_CONT with dev_dbg(). [sp] I did realize that hence changed only the call to dev_dbg. > > > > drivers/media/video/omap3isp/isp.c |2 +- > > > 1 files changed, 1 insertions(+), 1 deletions(-) > > > > > > diff --git a/drivers/media/video/omap3isp/isp.c > > > b/drivers/media/video/omap3isp/isp.c index 503bd79..1d38d96 100644 > > > --- a/drivers/media/video/omap3isp/isp.c > > > +++ b/drivers/media/video/omap3isp/isp.c > > > @@ -387,7 +387,7 @@ static inline void isp_isr_dbg(struct > isp_device > > > *isp, u32 irqstatus) > > > > > > }; > > > int i; > > > > > > - dev_dbg(isp->dev, ""); > > > + printk(KERN_DEBUG "%s:\n", dev_driver_string(isp->dev)); > > The original code doesn't include any \n. Is there a > particular reason why you > want to add one ? [sp] Sorry, that's a mistake out of habit. Another way to fix warning would be to make the string meaningful: - dev_dbg(isp->dev, ""); + dev_dbg (isp->dev, "ISP_IRQ:"); Is this better? ~sanjeev > > > > for (i = 0; i < ARRAY_SIZE(name); i++) { > > > > > > if ((1 << i) & irqstatus) > > -- > Regards, > > Laurent Pinchart > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [libdvben50221] [PATCH] Assign same resource_id in open_session_response when "resource non-existent"
On Thu, May 19, 2011 at 5:58 AM, Andreas Oberritter wrote: > On 05/18/2011 09:16 PM, Tomer Barletz wrote: >> On Tue, May 17, 2011 at 8:46 AM, Brice DUBOST wrote: >>> On 18/01/2011 15:42, Tomer Barletz wrote: > ... > > Can you please resend the patch inline with a proper signed-off-by line, > in order to get it tracked by patchwork.kernel.org? > > Regards, > Andreas > Here's the patch with the signed-off-by line: Signed-off-by: Tomer Barletz --- diff -r d3509d6e9499 lib/libdvben50221/en50221_stdcam_llci.c --- a/lib/libdvben50221/en50221_stdcam_llci.c Sat Aug 08 19:17:21 2009 +0200 +++ b/lib/libdvben50221/en50221_stdcam_llci.c Tue Jan 18 14:51:34 2011 +0200 @@ -351,6 +351,10 @@ } } + /* In case the reousrce does not exist, return the same id in the response. + See 7.2.6.2 */ + *connected_resource_id = requested_resource_id; + return -1; } -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/1] davinci: dm646x: move vpif related code to driver core header from platform
On Fri, May 20, 2011 at 19:28:49, Hadli, Manjunath wrote: > move vpif related code for capture and display drivers > from dm646x platform header file to vpif.h as these definitions > are related to driver code more than the platform or board. > > Signed-off-by: Manjunath Hadli > diff --git a/drivers/media/video/davinci/vpif.h > b/drivers/media/video/davinci/vpif.h > index 10550bd..e76dded 100644 > --- a/drivers/media/video/davinci/vpif.h > +++ b/drivers/media/video/davinci/vpif.h > @@ -20,6 +20,7 @@ > #include > #include > #include > +#include mach/hardware.h and mach/dm646x.h can now be dropped. Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] OMAP3BEAGLE: Add support for mt9p031 sensor driver.
On 05/23/11 10:47, Laurent Pinchart wrote: > Hi Javier, > > On Monday 23 May 2011 09:25:01 javier Martin wrote: >> On 22 May 2011 15:49, Igor Grinberg wrote: > [snip] > @@ -309,6 +357,15 @@ static int beagle_twl_gpio_setup(struct device *dev, pr_err("%s: unable to configure EHCI_nOC\n", __func__); } + if (omap3_beagle_get_rev() == OMAP3BEAGLE_BOARD_XM) { + /* + * Power on camera interface - only on pre-production, not + * needed on production boards + */ + gpio_request(gpio + 2, "CAM_EN"); + gpio_direction_output(gpio + 2, 1); >>> Why not gpio_request_one()? >> Just to follow the same approach as in the rest of the code. >> Maybe a further patch could change all "gpio_request()" uses to >> "gpio_request_one()". > Please do it the other way around. Replace calls to gpio_request() + > gpio_direction_output() with a call to gpio_request_one(), and then modify > this patch to use gpio_request_one(). Well, this is done already, you need to follow Tony's linux-next branch... So, just changing this patch would do... Also, good practice is to base patches on maintainer's appropriate branch, so it would be easier to apply. -- Regards, Igor. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v18 07/13] davinci: move DM64XX_VDD3P3V_PWDN to devices.c
On Sat, Apr 02, 2011 at 15:13:00, Hadli, Manjunath wrote: > Move the definition of DM64XX_VDD3P3V_PWDN from hardware.h > to devices.c since it is used only there. > > Signed-off-by: Manjunath Hadli > Acked-by: Sekhar Nori Applying this after updating the patch description to point out that this also helps rid hardware.h of platform private stuff.. > --- > arch/arm/mach-davinci/devices.c |1 + > arch/arm/mach-davinci/include/mach/hardware.h |3 --- > 2 files changed, 1 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/mach-davinci/devices.c b/arch/arm/mach-davinci/devices.c > index 22ebc64..4e1b663 100644 > --- a/arch/arm/mach-davinci/devices.c > +++ b/arch/arm/mach-davinci/devices.c > @@ -182,6 +182,7 @@ static struct platform_device davinci_mmcsd1_device = { > .resource = mmcsd1_resources, > }; > > +#define DM64XX_VDD3P3V_PWDN 0x48 .. and moving this to the top of the file. Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [git:v4l-utils/master] Add an install target to libv4l2util
On Monday, May 23, 2011 16:48:05 Hans de Goede wrote: > Hi, > > On 05/23/2011 12:00 PM, Mauro Carvalho Chehab wrote: > > This is an automatic generated email to let you know that the following > > patch were queued at the > > http://git.linuxtv.org/v4l-utils.git tree: > > > > Subject: Add an install target to libv4l2util > > Author: Mauro Carvalho Chehab > > Date:Mon May 23 07:00:00 2011 -0300 > > > > Signed-off-by: Mauro Carvalho Chehab > > > > Erm, > > This is a static lib, installing static libs globally is considered > bad practice. Either we need to make this a properly versioned .so > and all the API+ABI promises which some with that, or we should just > keep it as a private utility function lib, which gets linked into > a few utils, but not installed system wide. To be honest, I don't think this library should be installed at all. It is undocumented and frankly, I never liked it. As soon as we install this, then we are stuck with respect to the API/ABI, and if we do that, then it has to be reviewed first. > I think this may have something to do with the new get_media_devices That code looks awfully like a poor-mans media controller when it comes to discovering media nodes. No really the way I want to go. > code, but the commit message is rather undescriptive... Just for the record: I think a libv4l2util library is a nice idea. But if we do that, then we need something better than this. Regards, Hans > > Regards, > > Hans > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] FE_GET_PROPERTY should be _IOW, because the associated structure is transferred from userspace to kernelspace. Keep the old ioctl around for compatibility so that existing code is not brok
On 05/23/2011 04:51 PM, Hans Petter Selasky wrote: > On Monday 23 May 2011 16:37:18 Andreas Oberritter wrote: >> On 05/23/2011 03:58 PM, Hans Petter Selasky wrote: >>> From be7d0f72ebf4d945cfb2a5c9cc871707f72e1e3c Mon Sep 17 00:00:00 2001 >>> From: Hans Petter Selasky >>> Date: Mon, 23 May 2011 15:56:31 +0200 >>> Subject: [PATCH] FE_GET_PROPERTY should be _IOW, because the associated >>> structure is transferred from userspace to kernelspace. Keep the old >>> ioctl around for compatibility so that existing code is not broken. >> > > Hi, > >> Good catch, but I think _IOWR would be right, because the result gets >> copied from kernelspace to userspace. > > Those flags are only for the IOCTL associated structure itself. The V4L DVB > kernel only reads the dtv_properties structure in either case and does not > write any data back to it. That's why only _IOW is required. I see. > I checked somewhat and the R/W bits in the IOCTL command does not appear do > be > matched to the R/W permissions you have on the file handle? Or am I mistaken? You're right. There's no direct relationship between them, at least not within dvb-core. > In other words the IOCTL R/W (_IOC_READ, _IOC_WRITE) bits should not reflect > what the IOCTL actually does, like modifying indirect data? I'm not sure. Your patch is certainly doing the right thing for the current implementation of dvb_usercopy, which however wasn't designed with variable length arrays in mind. Taking dvb_usercopy aside, my interpretation of the ioctl bits was: - _IOC_READ is required if copy_to_user/put_user needs to be used during the ioctl. - _IOC_WRITE is required if copy_from_user/get_user needs to be used during the ioctl. Whether that's limited to the structure directly encoded in the ioctl or not is unclear to me. Maybe someone at LKML can shed some light on that. Regards, Andreas -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Add missing include guard to header file.
>From 30f6407c5eb67ac8ebf7da169c1f8e500e221db6 Mon Sep 17 00:00:00 2001 From: Hans Petter Selasky Date: Mon, 23 May 2011 17:10:40 +0200 Subject: [PATCH] Add missing include guard to header file. Signed-off-by: Hans Petter Selasky --- include/media/videobuf-dvb.h |4 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/include/media/videobuf-dvb.h b/include/media/videobuf-dvb.h index 07cf4b9..bf36572 100644 --- a/include/media/videobuf-dvb.h +++ b/include/media/videobuf-dvb.h @@ -4,6 +4,9 @@ #include #include +#ifndef _VIDEOBUF_DVB_H_ +#define_VIDEOBUF_DVB_H_ + struct videobuf_dvb { /* filling that the job of the driver */ char *name; @@ -54,6 +57,7 @@ void videobuf_dvb_dealloc_frontends(struct videobuf_dvb_frontends *f); struct videobuf_dvb_frontend * videobuf_dvb_get_frontend(struct videobuf_dvb_frontends *f, int id); int videobuf_dvb_find_frontend(struct videobuf_dvb_frontends *f, struct dvb_frontend *p); +#endif /* _VIDEOBUF_DVB_H_ */ /* * Local variables: -- 1.7.1.1 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] FE_GET_PROPERTY should be _IOW, because the associated structure is transferred from userspace to kernelspace. Keep the old ioctl around for compatibility so that existing code is not brok
On Monday 23 May 2011 16:37:18 Andreas Oberritter wrote: > On 05/23/2011 03:58 PM, Hans Petter Selasky wrote: > > From be7d0f72ebf4d945cfb2a5c9cc871707f72e1e3c Mon Sep 17 00:00:00 2001 > > From: Hans Petter Selasky > > Date: Mon, 23 May 2011 15:56:31 +0200 > > Subject: [PATCH] FE_GET_PROPERTY should be _IOW, because the associated > > structure is transferred from userspace to kernelspace. Keep the old > > ioctl around for compatibility so that existing code is not broken. > Hi, > Good catch, but I think _IOWR would be right, because the result gets > copied from kernelspace to userspace. Those flags are only for the IOCTL associated structure itself. The V4L DVB kernel only reads the dtv_properties structure in either case and does not write any data back to it. That's why only _IOW is required. I checked somewhat and the R/W bits in the IOCTL command does not appear do be matched to the R/W permissions you have on the file handle? Or am I mistaken? In other words the IOCTL R/W (_IOC_READ, _IOC_WRITE) bits should not reflect what the IOCTL actually does, like modifying indirect data? > > It would be nice if you could send future patches inline rather than > attached. I'd suggest using git format-patch and git send-email. I will try to have a look at that. I'm waiting for the 16 patches I've submitted today to get reviewed and committed, before I start the next batch. Thank you! --HPS -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [git:v4l-utils/master] Add an install target to libv4l2util
Hi, On 05/23/2011 12:00 PM, Mauro Carvalho Chehab wrote: This is an automatic generated email to let you know that the following patch were queued at the http://git.linuxtv.org/v4l-utils.git tree: Subject: Add an install target to libv4l2util Author: Mauro Carvalho Chehab Date:Mon May 23 07:00:00 2011 -0300 Signed-off-by: Mauro Carvalho Chehab Erm, This is a static lib, installing static libs globally is considered bad practice. Either we need to make this a properly versioned .so and all the API+ABI promises which some with that, or we should just keep it as a private utility function lib, which gets linked into a few utils, but not installed system wide. I think this may have something to do with the new get_media_devices code, but the commit message is rather undescriptive... Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Alternate setting 1 must be selected for interface 0 on the model that I received. Else the rest is identical.
--HPS From 3cf61d6a77b22f58471188cd0e7e3dc6c3a29b0b Mon Sep 17 00:00:00 2001 From: Hans Petter Selasky Date: Mon, 23 May 2011 16:36:55 +0200 Subject: [PATCH] Alternate setting 1 must be selected for interface 0 on the model that I received. Else the rest is identical. Signed-off-by: Hans Petter Selasky --- drivers/media/dvb/ttusb-budget/dvb-ttusb-budget.c |8 1 files changed, 8 insertions(+), 0 deletions(-) diff --git a/drivers/media/dvb/ttusb-budget/dvb-ttusb-budget.c b/drivers/media/dvb/ttusb-budget/dvb-ttusb-budget.c index cbe2f0d..38a7d03 100644 --- a/drivers/media/dvb/ttusb-budget/dvb-ttusb-budget.c +++ b/drivers/media/dvb/ttusb-budget/dvb-ttusb-budget.c @@ -971,6 +971,14 @@ static int ttusb_stop_feed(struct dvb_demux_feed *dvbdmxfeed) static int ttusb_setup_interfaces(struct ttusb *ttusb) { + /* + * Try to select alternate setting 1 for first interface. If + * that does not work, restore to alternate setting 0. + */ + if (usb_set_interface(ttusb->dev, 0, 1) < 0) + usb_set_interface(ttusb->dev, 0, 0); + + /* Select alternate setting 1 for second interface. */ usb_set_interface(ttusb->dev, 1, 1); ttusb->bulk_out_pipe = usb_sndbulkpipe(ttusb->dev, 1); -- 1.7.1.1
Re: [PATCH] FE_GET_PROPERTY should be _IOW, because the associated structure is transferred from userspace to kernelspace. Keep the old ioctl around for compatibility so that existing code is not brok
On 05/23/2011 03:58 PM, Hans Petter Selasky wrote: > From be7d0f72ebf4d945cfb2a5c9cc871707f72e1e3c Mon Sep 17 00:00:00 2001 > From: Hans Petter Selasky > Date: Mon, 23 May 2011 15:56:31 +0200 > Subject: [PATCH] FE_GET_PROPERTY should be _IOW, because the associated > structure is transferred from userspace to kernelspace. Keep the old ioctl > around for compatibility so that existing code is not broken. Good catch, but I think _IOWR would be right, because the result gets copied from kernelspace to userspace. It would be nice if you could send future patches inline rather than attached. I'd suggest using git format-patch and git send-email. Regards, Andreas > Signed-off-by: Hans Petter Selasky > --- > drivers/media/dvb/dvb-core/dvb_frontend.c |5 +++-- > include/linux/dvb/frontend.h |3 ++- > 2 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/dvb/dvb-core/dvb_frontend.c > b/drivers/media/dvb/dvb-core/dvb_frontend.c > index 31e2c0d..d93c1ec 100644 > --- a/drivers/media/dvb/dvb-core/dvb_frontend.c > +++ b/drivers/media/dvb/dvb-core/dvb_frontend.c > @@ -1507,7 +1507,8 @@ static int dvb_frontend_ioctl(struct file *file, > if (down_interruptible (&fepriv->sem)) > return -ERESTARTSYS; > > - if ((cmd == FE_SET_PROPERTY) || (cmd == FE_GET_PROPERTY)) > + if ((cmd == FE_SET_PROPERTY) || (cmd == FE_GET_PROPERTY) || > + (cmd == FE_GET_PROPERTY_OLD)) > err = dvb_frontend_ioctl_properties(file, cmd, parg); > else { > fe->dtv_property_cache.state = DTV_UNDEFINED; > @@ -1562,7 +1563,7 @@ static int dvb_frontend_ioctl_properties(struct file > *file, > dprintk("%s() Property cache is full, tuning\n", > __func__); > > } else > - if(cmd == FE_GET_PROPERTY) { > + if(cmd == FE_GET_PROPERTY || cmd == FE_GET_PROPERTY_OLD) { > > tvps = (struct dtv_properties __user *)parg; > > diff --git a/include/linux/dvb/frontend.h b/include/linux/dvb/frontend.h > index 493a2bf..05b38c4 100644 > --- a/include/linux/dvb/frontend.h > +++ b/include/linux/dvb/frontend.h > @@ -374,7 +374,8 @@ struct dtv_properties { > }; > > #define FE_SET_PROPERTY _IOW('o', 82, struct dtv_properties) > -#define FE_GET_PROPERTY _IOR('o', 83, struct dtv_properties) > +#define FE_GET_PROPERTY _IOW('o', 83, struct dtv_properties) > +#define FE_GET_PROPERTY_OLD _IOR('o', 83, struct dtv_properties) > > > /** > -- 1.7.1.1 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[ANN] IRC meeting May 31st, 10:00 CET (UTC+2): Cropping and Composing
Hi all, Just a short announcement that we will have a meeting on the #v4l IRC channel regarding the Samsung proposals for cropping and composing. See: http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/32152 We need to figure out how these proposals interact with the existing VIDIOC_S_FMT and how to handle any ordering dependencies (i.e. do we have to set the crop, s_fmt and composition in a particular order or not?) Since all developers that work on this are all in Europe this meeting is scheduled in the morning for the CET timezone. If people from outside Europe want to join, then please let us know soon. We can probably reschedule it if there are good reasons. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Increase a timeout, so that bad scheduling does not accidentially cause a timeout.
--HPS From 18faaafc9cbbe478bb49023bbeae490149048560 Mon Sep 17 00:00:00 2001 From: Hans Petter Selasky Date: Mon, 23 May 2011 16:21:47 +0200 Subject: [PATCH] Increase a timeout, so that bad scheduling does not accidentially cause a timeout. Signed-off-by: Hans Petter Selasky --- drivers/media/dvb/frontends/stb0899_drv.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/media/dvb/frontends/stb0899_drv.c b/drivers/media/dvb/frontends/stb0899_drv.c index 37a222d..ddb9141 100644 --- a/drivers/media/dvb/frontends/stb0899_drv.c +++ b/drivers/media/dvb/frontends/stb0899_drv.c @@ -706,7 +706,7 @@ static int stb0899_send_diseqc_msg(struct dvb_frontend *fe, struct dvb_diseqc_ma stb0899_write_reg(state, STB0899_DISCNTRL1, reg); for (i = 0; i < cmd->msg_len; i++) { /* wait for FIFO empty */ - if (stb0899_wait_diseqc_fifo_empty(state, 10) < 0) + if (stb0899_wait_diseqc_fifo_empty(state, 100) < 0) return -ETIMEDOUT; stb0899_write_reg(state, STB0899_DISFIFO, cmd->msg[i]); -- 1.7.1.1
[PATCH] Inlined functions should be static.
--HPS From 446037f0f999759b4b801b6512d18bae769465bb Mon Sep 17 00:00:00 2001 From: Hans Petter Selasky Date: Mon, 23 May 2011 16:06:22 +0200 Subject: [PATCH] Inlined functions should be static. Signed-off-by: Hans Petter Selasky --- drivers/media/dvb/frontends/stb0899_algo.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/media/dvb/frontends/stb0899_algo.c b/drivers/media/dvb/frontends/stb0899_algo.c index 2da55ec..d70eee0 100644 --- a/drivers/media/dvb/frontends/stb0899_algo.c +++ b/drivers/media/dvb/frontends/stb0899_algo.c @@ -23,7 +23,7 @@ #include "stb0899_priv.h" #include "stb0899_reg.h" -inline u32 stb0899_do_div(u64 n, u32 d) +static inline u32 stb0899_do_div(u64 n, u32 d) { /* wrap do_div() for ease of use */ -- 1.7.1.1
[PATCH] FE_GET_PROPERTY should be _IOW, because the associated structure is transferred from userspace to kernelspace. Keep the old ioctl around for compatibility so that existing code is not broken.
--HPS From be7d0f72ebf4d945cfb2a5c9cc871707f72e1e3c Mon Sep 17 00:00:00 2001 From: Hans Petter Selasky Date: Mon, 23 May 2011 15:56:31 +0200 Subject: [PATCH] FE_GET_PROPERTY should be _IOW, because the associated structure is transferred from userspace to kernelspace. Keep the old ioctl around for compatibility so that existing code is not broken. Signed-off-by: Hans Petter Selasky --- drivers/media/dvb/dvb-core/dvb_frontend.c |5 +++-- include/linux/dvb/frontend.h |3 ++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/media/dvb/dvb-core/dvb_frontend.c b/drivers/media/dvb/dvb-core/dvb_frontend.c index 31e2c0d..d93c1ec 100644 --- a/drivers/media/dvb/dvb-core/dvb_frontend.c +++ b/drivers/media/dvb/dvb-core/dvb_frontend.c @@ -1507,7 +1507,8 @@ static int dvb_frontend_ioctl(struct file *file, if (down_interruptible (&fepriv->sem)) return -ERESTARTSYS; - if ((cmd == FE_SET_PROPERTY) || (cmd == FE_GET_PROPERTY)) + if ((cmd == FE_SET_PROPERTY) || (cmd == FE_GET_PROPERTY) || + (cmd == FE_GET_PROPERTY_OLD)) err = dvb_frontend_ioctl_properties(file, cmd, parg); else { fe->dtv_property_cache.state = DTV_UNDEFINED; @@ -1562,7 +1563,7 @@ static int dvb_frontend_ioctl_properties(struct file *file, dprintk("%s() Property cache is full, tuning\n", __func__); } else - if(cmd == FE_GET_PROPERTY) { + if(cmd == FE_GET_PROPERTY || cmd == FE_GET_PROPERTY_OLD) { tvps = (struct dtv_properties __user *)parg; diff --git a/include/linux/dvb/frontend.h b/include/linux/dvb/frontend.h index 493a2bf..05b38c4 100644 --- a/include/linux/dvb/frontend.h +++ b/include/linux/dvb/frontend.h @@ -374,7 +374,8 @@ struct dtv_properties { }; #define FE_SET_PROPERTY _IOW('o', 82, struct dtv_properties) -#define FE_GET_PROPERTY _IOR('o', 83, struct dtv_properties) +#define FE_GET_PROPERTY _IOW('o', 83, struct dtv_properties) +#define FE_GET_PROPERTY_OLD _IOR('o', 83, struct dtv_properties) /** -- 1.7.1.1
[PATCH] Make DVB NET configurable in the kernel.
--HPS From 7222450a9d6f96f652237c65019fb25f54586d01 Mon Sep 17 00:00:00 2001 From: Hans Petter Selasky Date: Mon, 23 May 2011 14:43:35 +0200 Subject: [PATCH] Make DVB NET configurable in the kernel. Signed-off-by: Hans Petter Selasky --- drivers/media/Kconfig| 12 +++- drivers/media/dvb/dvb-core/Makefile |4 +++- drivers/media/dvb/dvb-core/dvb_net.h | 26 ++ 3 files changed, 40 insertions(+), 2 deletions(-) diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig index 6995940..dc61895 100644 --- a/drivers/media/Kconfig +++ b/drivers/media/Kconfig @@ -68,7 +68,6 @@ config VIDEO_V4L2_SUBDEV_API config DVB_CORE tristate "DVB for Linux" - depends on NET && INET select CRC32 help DVB core utility functions for device handling, software fallbacks etc. @@ -85,6 +84,17 @@ config DVB_CORE If unsure say N. +config DVB_NET + bool "DVB Network Support" + default (NET && INET) + depends on NET && INET + help + The DVB network support in the DVB core can + optionally be disabled if this + option is set to N. + + If unsure say Y. + config VIDEO_MEDIA tristate default (DVB_CORE && (VIDEO_DEV = n)) || (VIDEO_DEV && (DVB_CORE = n)) || (DVB_CORE && VIDEO_DEV) diff --git a/drivers/media/dvb/dvb-core/Makefile b/drivers/media/dvb/dvb-core/Makefile index 0b51828..8f22bcd 100644 --- a/drivers/media/dvb/dvb-core/Makefile +++ b/drivers/media/dvb/dvb-core/Makefile @@ -2,8 +2,10 @@ # Makefile for the kernel DVB device drivers. # +dvb-net-$(CONFIG_DVB_NET) := dvb_net.o + dvb-core-objs := dvbdev.o dmxdev.o dvb_demux.o dvb_filter.o \ dvb_ca_en50221.o dvb_frontend.o \ - dvb_net.o dvb_ringbuffer.o dvb_math.o + $(dvb-net-y) dvb_ringbuffer.o dvb_math.o obj-$(CONFIG_DVB_CORE) += dvb-core.o diff --git a/drivers/media/dvb/dvb-core/dvb_net.h b/drivers/media/dvb/dvb-core/dvb_net.h index 3a3126c..8a0907f 100644 --- a/drivers/media/dvb/dvb-core/dvb_net.h +++ b/drivers/media/dvb/dvb-core/dvb_net.h @@ -32,6 +32,8 @@ #define DVB_NET_DEVICES_MAX 10 +#ifdef CONFIG_DVB_NET + struct dvb_net { struct dvb_device *dvbdev; struct net_device *device[DVB_NET_DEVICES_MAX]; @@ -45,3 +47,27 @@ void dvb_net_release(struct dvb_net *); int dvb_net_init(struct dvb_adapter *, struct dvb_net *, struct dmx_demux *); #endif + +#ifndef CONFIG_DVB_NET + +struct dvb_dev_stub; + +struct dvb_net { + struct dvb_dev_stub *dvbdev; +}; + +static inline void dvb_net_release(struct dvb_net *dvbnet) +{ + dvbnet->dvbdev = 0; +} + +static inline int dvb_net_init(struct dvb_adapter *adap, +struct dvb_net *dvbnet, struct dmx_demux *dmx) +{ + dvbnet->dvbdev = (void *)1; + return 0; +} + +#endif + +#endif -- 1.7.1.1
[PATCH v5] [resend] radio-sf16fmr2: convert to generic TEA575x interface
Convert radio-sf16fmr2 to use generic TEA575x implementation. Most of the driver code goes away as SF16-FMR2 is basically just a TEA5757 tuner connected to ISA bus. The card can optionally be equipped with PT2254A volume control (equivalent of TC9154AP) - the volume setting is completely reworked (with balance control added) and tested. Signed-off-by: Ondrej Zary --- linux-2.6.39-rc2-/sound/pci/Kconfig 2011-05-15 18:50:18.0 +0200 +++ linux-2.6.39-rc2/sound/pci/Kconfig 2011-05-17 23:35:30.0 +0200 @@ -565,8 +565,8 @@ config SND_FM801_TEA575X_BOOL config SND_TEA575X tristate - depends on SND_FM801_TEA575X_BOOL || SND_ES1968_RADIO - default SND_FM801 || SND_ES1968 + depends on SND_FM801_TEA575X_BOOL || SND_ES1968_RADIO || RADIO_SF16FMR2 + default SND_FM801 || SND_ES1968 || RADIO_SF16FMR2 source "sound/pci/hda/Kconfig" --- linux-2.6.39-rc2-/drivers/media/radio/radio-sf16fmr2.c 2011-04-06 03:30:43.0 +0200 +++ linux-2.6.39-rc2/drivers/media/radio/radio-sf16fmr2.c 2011-05-19 17:56:08.0 +0200 @@ -1,441 +1,209 @@ -/* SF16FMR2 radio driver for Linux radio support - * heavily based on fmi driver... - * (c) 2000-2002 Ziglio Frediano, fredd...@angelfire.com +/* SF16-FMR2 radio driver for Linux + * Copyright (c) 2011 Ondrej Zary * - * Notes on the hardware - * - * Frequency control is done digitally -- ie out(port,encodefreq(95.8)); - * No volume control - only mute/unmute - you have to use line volume - * - * For read stereo/mono you must wait 0.1 sec after set frequency and - * card unmuted so I set frequency on unmute - * Signal handling seem to work only on autoscanning (not implemented) - * - * Converted to V4L2 API by Mauro Carvalho Chehab + * Original driver was (c) 2000-2002 Ziglio Frediano, fredd...@angelfire.com + * but almost nothing remained here after conversion to generic TEA575x + * implementation */ +#include #include /* Modules */ #include /* Initdata */ #include /* request_region */ -#include/* udelay */ -#include/* kernel radio structs */ -#include -#include /* for KERNEL_VERSION MACRO */ #include /* outb, outb_p */ -#include -#include +#include -MODULE_AUTHOR("Ziglio Frediano, fredd...@angelfire.com"); -MODULE_DESCRIPTION("A driver for the SF16FMR2 radio."); +MODULE_AUTHOR("Ondrej Zary"); +MODULE_DESCRIPTION("MediaForte SF16-FMR2 FM radio card driver"); MODULE_LICENSE("GPL"); -static int io = 0x384; -static int radio_nr = -1; - -module_param(io, int, 0); -MODULE_PARM_DESC(io, "I/O address of the SF16FMR2 card (should be 0x384, if do not work try 0x284)"); -module_param(radio_nr, int, 0); - -#define RADIO_VERSION KERNEL_VERSION(0,0,2) - -#define AUD_VOL_INDEX 1 - -#undef DEBUG -//#define DEBUG 1 - -#ifdef DEBUG -# define debug_print(s) printk s -#else -# define debug_print(s) -#endif - -/* this should be static vars for module size */ -struct fmr2 -{ - struct v4l2_device v4l2_dev; - struct video_device vdev; - struct mutex lock; +struct fmr2 { int io; - int curvol; /* 0-15 */ - int mute; - int stereo; /* card is producing stereo audio */ - unsigned long curfreq; /* freq in kHz */ - int card_type; + struct snd_tea575x tea; + struct v4l2_ctrl *volume; + struct v4l2_ctrl *balance; }; +/* the port is hardwired so no need to support multiple cards */ +#define FMR2_PORT 0x384 static struct fmr2 fmr2_card; -/* hw precision is 12.5 kHz - * It is only useful to give freq in interval of 200 (=0.0125Mhz), - * other bits will be truncated - */ -#define RSF16_ENCODE(x)((x) / 200 + 856) -#define RSF16_MINFREQ (87 * 16000) -#define RSF16_MAXFREQ (108 * 16000) - -static inline void wait(int n, int io) -{ - for (; n; --n) - inb(io); -} - -static void outbits(int bits, unsigned int data, int nWait, int io) -{ - int bit; - - for (; --bits >= 0;) { - bit = (data >> bits) & 1; - outb(bit, io); - wait(nWait, io); - outb(bit | 2, io); - wait(nWait, io); - outb(bit, io); - wait(nWait, io); - } -} - -static inline void fmr2_mute(int io) -{ - outb(0x00, io); - wait(4, io); -} - -static inline void fmr2_unmute(int io) -{ - outb(0x04, io); - wait(4, io); -} - -static inline int fmr2_stereo_mode(int io) -{ - int n = inb(io); - - outb(6, io); - inb(io); - n = ((n >> 3) & 1) ^ 1; - debug_print((KERN_DEBUG "stereo: %d\n", n)); - return n; -} - -static int fmr2_product_info(struct fmr2 *dev) -{ - int n = inb(dev->io); - - n &= 0xC1; - if (n == 0) { - /* this should support volume set */ - dev->card_type = 12; - return
[PATCH v5] [resend] tea575x: convert to control framework
Convert tea575x-tuner to use the new V4L2 control framework. Also add ext_init() callback that can be used by a card driver for additional initialization right before registering the video device (for SF16-FMR2). Also embed struct video_device to struct snd_tea575x to simplify the code. Signed-off-by: Ondrej Zary --- linux-2.6.39-rc2-/include/sound/tea575x-tuner.h 2011-05-13 19:39:23.0 +0200 +++ linux-2.6.39-rc2/include/sound/tea575x-tuner.h 2011-05-19 17:27:45.0 +0200 @@ -23,8 +23,8 @@ */ #include +#include #include -#include #define TEA575X_FMIF 10700 @@ -42,7 +42,7 @@ struct snd_tea575x_ops { }; struct snd_tea575x { - struct video_device *vd;/* video device */ + struct video_device vd; /* video device */ bool tea5759; /* 5759 chip is present */ bool mute; /* Device is muted? */ bool stereo;/* receiving stereo */ @@ -54,6 +54,8 @@ struct snd_tea575x { void *private_data; u8 card[32]; u8 bus_info[32]; + struct v4l2_ctrl_handler ctrl_handler; + int (*ext_init)(struct snd_tea575x *tea); }; int snd_tea575x_init(struct snd_tea575x *tea); --- linux-2.6.39-rc2-/sound/i2c/other/tea575x-tuner.c 2011-05-13 19:39:23.0 +0200 +++ linux-2.6.39-rc2/sound/i2c/other/tea575x-tuner.c2011-05-19 17:31:45.0 +0200 @@ -22,11 +22,11 @@ #include #include -#include #include #include #include -#include +#include +#include #include MODULE_AUTHOR("Jaroslav Kysela "); @@ -62,17 +62,6 @@ module_param(radio_nr, int, 0); #define TEA575X_BIT_DUMMY (1<<15) /* buffer */ #define TEA575X_BIT_FREQ_MASK 0x7fff -static struct v4l2_queryctrl radio_qctrl[] = { - { - .id= V4L2_CID_AUDIO_MUTE, - .name = "Mute", - .minimum = 0, - .maximum = 1, - .default_value = 1, - .type = V4L2_CTRL_TYPE_BOOLEAN, - } -}; - /* * lowlevel part */ @@ -266,47 +255,17 @@ static int vidioc_s_audio(struct file *f return 0; } -static int vidioc_queryctrl(struct file *file, void *priv, - struct v4l2_queryctrl *qc) +static int tea575x_s_ctrl(struct v4l2_ctrl *ctrl) { - int i; - - for (i = 0; i < ARRAY_SIZE(radio_qctrl); i++) { - if (qc->id && qc->id == radio_qctrl[i].id) { - memcpy(qc, &(radio_qctrl[i]), - sizeof(*qc)); - return 0; - } - } - return -EINVAL; -} - -static int vidioc_g_ctrl(struct file *file, void *priv, - struct v4l2_control *ctrl) -{ - struct snd_tea575x *tea = video_drvdata(file); + struct snd_tea575x *tea = container_of(ctrl->handler, struct snd_tea575x, ctrl_handler); switch (ctrl->id) { case V4L2_CID_AUDIO_MUTE: - ctrl->value = tea->mute; + tea->mute = ctrl->val; + snd_tea575x_set_freq(tea); return 0; } - return -EINVAL; -} -static int vidioc_s_ctrl(struct file *file, void *priv, - struct v4l2_control *ctrl) -{ - struct snd_tea575x *tea = video_drvdata(file); - - switch (ctrl->id) { - case V4L2_CID_AUDIO_MUTE: - if (tea->mute != ctrl->value) { - tea->mute = ctrl->value; - snd_tea575x_set_freq(tea); - } - return 0; - } return -EINVAL; } @@ -355,16 +314,17 @@ static const struct v4l2_ioctl_ops tea57 .vidioc_s_input = vidioc_s_input, .vidioc_g_frequency = vidioc_g_frequency, .vidioc_s_frequency = vidioc_s_frequency, - .vidioc_queryctrl = vidioc_queryctrl, - .vidioc_g_ctrl = vidioc_g_ctrl, - .vidioc_s_ctrl = vidioc_s_ctrl, }; static struct video_device tea575x_radio = { .name = "tea575x-tuner", .fops = &tea575x_fops, .ioctl_ops = &tea575x_ioctl_ops, - .release= video_device_release, + .release= video_device_release_empty, +}; + +static const struct v4l2_ctrl_ops tea575x_ctrl_ops = { + .s_ctrl = tea575x_s_ctrl, }; /* @@ -373,7 +333,6 @@ static struct video_device tea575x_radio int snd_tea575x_init(struct snd_tea575x *tea) { int retval; - struct video_device *tea575x_radio_inst; tea->mute = 1; @@ -384,40 +343,45 @@ int snd_tea575x_init(struct snd_tea575x tea->in_use = 0; tea->val = TEA575X_BIT_BAND_FM | TEA575X_BIT_SEARCH_10_40; tea->freq = 90500 * 16; /* 90.5Mhz default */ + snd_tea575x_set_freq(tea); - tea575x_radio_inst = video_device_alloc(); - if
Re: [RFC] Standardize YUV support in the fbdev API
On Monday, May 23, 2011 13:55:21 Marek Szyprowski wrote: > Hello, > > On Wednesday, May 18, 2011 8:54 AM Hans Verkuil wrote: > > > On Wednesday, May 18, 2011 00:44:26 Felipe Contreras wrote: > > > On Wed, May 18, 2011 at 1:07 AM, Laurent Pinchart > > > wrote: > > > > I need to implement support for a YUV frame buffer in an fbdev driver. > > As the > > > > fbdev API doesn't support this out of the box, I've spent a couple of > > days > > > > reading fbdev (and KMS) code and thinking about how we could cleanly > > add YUV > > > > support to the API. I'd like to share my findings and thoughts, and > > hopefully > > > > receive some comments back. > > > > > > > > The terms 'format', 'pixel format', 'frame buffer format' and 'data > > format' > > > > will be used interchangeably in this e-mail. They all refer to the way > > pixels > > > > are stored in memory, including both the representation of a pixel as > > integer > > > > values and the layout of those integer values in memory. > > > > > > This is a great proposal. It was about time! > > > > > > > The third solution has my preference. Comments and feedback will be > > > > appreciated. I will then work on a proof of concept and submit patches. > > > > > > I also would prefer the third solution. I don't think there's much > > > difference from the user-space point of view, and a new ioctl would be > > > cleaner. Also the v4l2 fourcc's should do. > > > > I agree with this. > > > > We might want to take the opportunity to fix this section of the V4L2 Spec: > > > > http://www.xs4all.nl/~hverkuil/spec/media.html#pixfmt-rgb > > > > There are two tables, 2.6 and 2.7. But 2.6 is almost certainly wrong and > > should be removed. > > That's definitely true. I was confused at the beginning when I saw 2 > different tables describing the same pixel formats. > > I suspect many if not all V4L2 drivers are badly broken for > > big-endian systems and report the wrong pixel formats. > > > > Officially the pixel formats reflect the contents of the memory. But > > everything is swapped on a big endian system, so you are supposed to > > report a different pix format. > > I always thought that pix_format describes the layout of video data in > memory on byte level, which is exactly the same on both little- and big- > endian systems. Correct. > You can notice swapped data only if you access memory > by units larger than byte, like 16bit or 32bit integers. BTW - I would > really like to avoid little- and big- endian flame, but your statement > about 'everything is swapped on a big endian system' is completely > wrong. It is rather the characteristic of little-endian system not big > endian one if you display the content of the same memory first using > byte access and then using word/long access. You are correct, I wasn't thinking it through. > > I can't remember seeing any driver do that. Some have built-in swapping, > > though, and turn that on if needed. > > The drivers shouldn't do ANY byte swapping at all. Only tools that > extract pixel data with some 'accelerated' methods (like 32bit integer > casting and bit-level shifting) should be aware of endianess. > > > I really need to run some tests, but I've been telling myself this for > > years now :-( > > I've checked the BTTV board in my PowerMac/G4 and the display was > correct with xawtv. It is just a matter of selecting correct pix format > basing on the information returned by xsever. > > Best regards > Just forget my post (except for the part of cleaning up the tables :-) ). Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC] Standardize YUV support in the fbdev API
Hello, On Wednesday, May 18, 2011 8:54 AM Hans Verkuil wrote: > On Wednesday, May 18, 2011 00:44:26 Felipe Contreras wrote: > > On Wed, May 18, 2011 at 1:07 AM, Laurent Pinchart > > wrote: > > > I need to implement support for a YUV frame buffer in an fbdev driver. > As the > > > fbdev API doesn't support this out of the box, I've spent a couple of > days > > > reading fbdev (and KMS) code and thinking about how we could cleanly > add YUV > > > support to the API. I'd like to share my findings and thoughts, and > hopefully > > > receive some comments back. > > > > > > The terms 'format', 'pixel format', 'frame buffer format' and 'data > format' > > > will be used interchangeably in this e-mail. They all refer to the way > pixels > > > are stored in memory, including both the representation of a pixel as > integer > > > values and the layout of those integer values in memory. > > > > This is a great proposal. It was about time! > > > > > The third solution has my preference. Comments and feedback will be > > > appreciated. I will then work on a proof of concept and submit patches. > > > > I also would prefer the third solution. I don't think there's much > > difference from the user-space point of view, and a new ioctl would be > > cleaner. Also the v4l2 fourcc's should do. > > I agree with this. > > We might want to take the opportunity to fix this section of the V4L2 Spec: > > http://www.xs4all.nl/~hverkuil/spec/media.html#pixfmt-rgb > > There are two tables, 2.6 and 2.7. But 2.6 is almost certainly wrong and > should be removed. That's definitely true. I was confused at the beginning when I saw 2 different tables describing the same pixel formats. I suspect many if not all V4L2 drivers are badly broken for > big-endian systems and report the wrong pixel formats. > > Officially the pixel formats reflect the contents of the memory. But > everything is swapped on a big endian system, so you are supposed to > report a different pix format. I always thought that pix_format describes the layout of video data in memory on byte level, which is exactly the same on both little- and big- endian systems. You can notice swapped data only if you access memory by units larger than byte, like 16bit or 32bit integers. BTW - I would really like to avoid little- and big- endian flame, but your statement about 'everything is swapped on a big endian system' is completely wrong. It is rather the characteristic of little-endian system not big endian one if you display the content of the same memory first using byte access and then using word/long access. > I can't remember seeing any driver do that. Some have built-in swapping, > though, and turn that on if needed. The drivers shouldn't do ANY byte swapping at all. Only tools that extract pixel data with some 'accelerated' methods (like 32bit integer casting and bit-level shifting) should be aware of endianess. > I really need to run some tests, but I've been telling myself this for > years now :-( I've checked the BTTV board in my PowerMac/G4 and the display was correct with xawtv. It is just a matter of selecting correct pix format basing on the information returned by xsever. Best regards -- Marek Szyprowski Samsung Poland R&D Center -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] The info and err macros are already defined by the USB stack. Rename these macros to avoid macro redefinition warnings.
--HPS From 49bc5e480f51a9b756f1f7f1503d237e8a1ba939 Mon Sep 17 00:00:00 2001 From: Hans Petter Selasky Date: Mon, 23 May 2011 13:42:40 +0200 Subject: [PATCH] The info and err macros are already defined by the USB stack. Rename these macros to avoid macro redefinition warnings. Signed-off-by: Hans Petter Selasky --- drivers/media/dvb/frontends/cx24113.c | 20 ++-- 1 files changed, 10 insertions(+), 10 deletions(-) diff --git a/drivers/media/dvb/frontends/cx24113.c b/drivers/media/dvb/frontends/cx24113.c index e9ee555..c341d57 100644 --- a/drivers/media/dvb/frontends/cx24113.c +++ b/drivers/media/dvb/frontends/cx24113.c @@ -31,8 +31,8 @@ static int debug; -#define info(args...) do { printk(KERN_INFO "CX24113: " args); } while (0) -#define err(args...) do { printk(KERN_ERR "CX24113: " args); } while (0) +#define cx_info(args...) do { printk(KERN_INFO "CX24113: " args); } while (0) +#define cx_err(args...) do { printk(KERN_ERR "CX24113: " args); } while (0) #define dprintk(args...) \ do { \ @@ -341,7 +341,7 @@ static void cx24113_calc_pll_nf(struct cx24113_state *state, u16 *n, s32 *f) } while (N < 6 && R < 3); if (N < 6) { - err("strange frequency: N < 6\n"); + cx_err("strange frequency: N < 6\n"); return; } F = freq_hz; @@ -563,7 +563,7 @@ struct dvb_frontend *cx24113_attach(struct dvb_frontend *fe, kzalloc(sizeof(struct cx24113_state), GFP_KERNEL); int rc; if (state == NULL) { - err("Unable to kzalloc\n"); + cx_err("Unable to kzalloc\n"); goto error; } @@ -571,7 +571,7 @@ struct dvb_frontend *cx24113_attach(struct dvb_frontend *fe, state->config = config; state->i2c = i2c; - info("trying to detect myself\n"); + cx_info("trying to detect myself\n"); /* making a dummy read, because of some expected troubles * after power on */ @@ -579,24 +579,24 @@ struct dvb_frontend *cx24113_attach(struct dvb_frontend *fe, rc = cx24113_readreg(state, 0x00); if (rc < 0) { - info("CX24113 not found.\n"); + cx_info("CX24113 not found.\n"); goto error; } state->rev = rc; switch (rc) { case 0x43: - info("detected CX24113 variant\n"); + cx_info("detected CX24113 variant\n"); break; case REV_CX24113: - info("successfully detected\n"); + cx_info("successfully detected\n"); break; default: - err("unsupported device id: %x\n", state->rev); + cx_err("unsupported device id: %x\n", state->rev); goto error; } state->ver = cx24113_readreg(state, 0x01); - info("version: %x\n", state->ver); + cx_info("version: %x\n", state->ver); /* create dvb_frontend */ memcpy(&fe->ops.tuner_ops, &cx24113_tuner_ops, -- 1.7.1.1
[PATCH] The dbg, warn and info macros are already defined by the USB stack. Rename these macros to avoid macro redefinition warnings. Refactor lineshift in printouts.
--HPS From c05334a4162912164d9e8d2d551f1bc2a5fdce82 Mon Sep 17 00:00:00 2001 From: Hans Petter Selasky Date: Mon, 23 May 2011 13:40:00 +0200 Subject: [PATCH] The dbg, warn and info macros are already defined by the USB stack. Rename these macros to avoid macro redefinition warnings. Refactor lineshift in printouts. Signed-off-by: Hans Petter Selasky --- drivers/media/dvb/frontends/itd1000.c | 25 +++-- 1 files changed, 11 insertions(+), 14 deletions(-) diff --git a/drivers/media/dvb/frontends/itd1000.c b/drivers/media/dvb/frontends/itd1000.c index f7a40a1..aa9ccb8 100644 --- a/drivers/media/dvb/frontends/itd1000.c +++ b/drivers/media/dvb/frontends/itd1000.c @@ -35,21 +35,18 @@ static int debug; module_param(debug, int, 0644); MODULE_PARM_DESC(debug, "Turn on/off debugging (default:off)."); -#define deb(args...) do { \ +#define itd_dbg(args...) do { \ if (debug) { \ printk(KERN_DEBUG "ITD1000: " args);\ - printk("\n"); \ } \ } while (0) -#define warn(args...) do { \ +#define itd_warn(args...) do { \ printk(KERN_WARNING "ITD1000: " args); \ - printk("\n"); \ } while (0) -#define info(args...) do { \ +#define itd_info(args...) do { \ printk(KERN_INFO"ITD1000: " args); \ - printk("\n"); \ } while (0) /* don't write more than one byte with flexcop behind */ @@ -62,7 +59,7 @@ static int itd1000_write_regs(struct itd1000_state *state, u8 reg, u8 v[], u8 le buf[0] = reg; memcpy(&buf[1], v, len); - /* deb("wr %02x: %02x", reg, v[0]); */ + /* itd_dbg("wr %02x: %02x\n", reg, v[0]); */ if (i2c_transfer(state->i2c, &msg, 1) != 1) { printk(KERN_WARNING "itd1000 I2C write failed\n"); @@ -83,7 +80,7 @@ static int itd1000_read_reg(struct itd1000_state *state, u8 reg) itd1000_write_regs(state, (reg - 1) & 0xff, &state->shadow[(reg - 1) & 0xff], 1); if (i2c_transfer(state->i2c, msg, 2) != 2) { - warn("itd1000 I2C read failed"); + itd_warn("itd1000 I2C read failed\n"); return -EREMOTEIO; } return val; @@ -127,14 +124,14 @@ static void itd1000_set_lpf_bw(struct itd1000_state *state, u32 symbol_rate) u8 bbgvmin = itd1000_read_reg(state, BBGVMIN) & 0xf0; u8 bw = itd1000_read_reg(state, BW) & 0xf0; - deb("symbol_rate = %d", symbol_rate); + itd_dbg("symbol_rate = %d\n", symbol_rate); /* not sure what is that ? - starting to download the table */ itd1000_write_reg(state, CON1, con1 | (1 << 1)); for (i = 0; i < ARRAY_SIZE(itd1000_lpf_pga); i++) if (symbol_rate < itd1000_lpf_pga[i].symbol_rate) { - deb("symrate: index: %d pgaext: %x, bbgvmin: %x", i, itd1000_lpf_pga[i].pgaext, itd1000_lpf_pga[i].bbgvmin); + itd_dbg("symrate: index: %d pgaext: %x, bbgvmin: %x\n", i, itd1000_lpf_pga[i].pgaext, itd1000_lpf_pga[i].bbgvmin); itd1000_write_reg(state, PLLFH, pllfh | (itd1000_lpf_pga[i].pgaext << 4)); itd1000_write_reg(state, BBGVMIN, bbgvmin | (itd1000_lpf_pga[i].bbgvmin)); itd1000_write_reg(state, BW, bw | (i & 0x0f)); @@ -182,7 +179,7 @@ static void itd1000_set_vco(struct itd1000_state *state, u32 freq_khz) adcout = itd1000_read_reg(state, PLLLOCK) & 0x0f; - deb("VCO: %dkHz: %d -> ADCOUT: %d %02x", freq_khz, itd1000_vcorg[i].vcorg, adcout, vco_chp1_i2c); + itd_dbg("VCO: %dkHz: %d -> ADCOUT: %d %02x\n", freq_khz, itd1000_vcorg[i].vcorg, adcout, vco_chp1_i2c); if (adcout > 13) { if (!(itd1000_vcorg[i].vcorg == 7 || itd1000_vcorg[i].vcorg == 15)) @@ -232,7 +229,7 @@ static void itd1000_set_lo(struct itd1000_state *state, u32 freq_khz) pllf = (u32) tmp; state->frequency = ((plln * 1000) + (pllf * 1000)/1048576) * 2*FREF; - deb("frequency: %dkHz (wanted) %dkHz (set), PLLF = %d, PLLN = %d", freq_khz, state->frequency, pllf, plln); + itd_dbg("frequency: %dkHz (wanted) %dkHz (set), PLLF = %d, PLLN = %d\n", freq_khz, state->frequency, pllf, plln); itd1000_write_reg(state, PLLNH, 0x80); /* PLLNH */; itd1000_write_reg(state, PLLNL, plln & 0xff); @@ -242,7 +239,7 @@ static void itd1000_set_lo(struct itd1000_state *state, u32 freq_khz) for (i = 0; i < ARRAY_SIZE(itd1000_fre_values); i++) { if (freq_khz <= itd1000_fre_values[i].freq) { - deb("fre_values: %d", i); + itd_dbg("fre_values: %d\n", i); itd1000_write_reg(state, RFTR, itd1000_fre_values[i].values[0]); for (j = 0; j < 9; j++) itd1000_write_reg(state, RFST1+j, itd1000_fre_values[i].values[j+1]); @@ -382,7 +379,7 @@ struct dvb_frontend *itd1000_attach(struct dvb_frontend *fe, struct i2c_adapter kfree(state); return NULL; } - info("successfully identified (ID: %d)", i); + itd_info("successfully identified (ID: %d)\n", i); memset(state->shadow, 0xff, sizeof(state->shadow)); for (i = 0x65; i < 0x9c; i++) -- 1.7.1.1
[PATCH] Remove invalid parameter description.
--HPS From 843559156c08edaf3ef616df005eae8b8d14b186 Mon Sep 17 00:00:00 2001 From: Hans Petter Selasky Date: Mon, 23 May 2011 13:28:50 +0200 Subject: [PATCH] Remove invalid parameter description. Signed-off-by: Hans Petter Selasky --- drivers/media/dvb/frontends/tda8261.c |1 - 1 files changed, 0 insertions(+), 1 deletions(-) diff --git a/drivers/media/dvb/frontends/tda8261.c b/drivers/media/dvb/frontends/tda8261.c index 1742056..53c7d8f 100644 --- a/drivers/media/dvb/frontends/tda8261.c +++ b/drivers/media/dvb/frontends/tda8261.c @@ -224,7 +224,6 @@ exit: } EXPORT_SYMBOL(tda8261_attach); -MODULE_PARM_DESC(verbose, "Set verbosity level"); MODULE_AUTHOR("Manu Abraham"); MODULE_DESCRIPTION("TDA8261 8PSK/QPSK Tuner"); -- 1.7.1.1
[PATCH] Define parameter description after the parameter itself.
--HPS From 2f5378e5c5cc5528473f77321879fb075005d3dd Mon Sep 17 00:00:00 2001 From: Hans Petter Selasky Date: Mon, 23 May 2011 13:26:04 +0200 Subject: [PATCH] Define parameter description after the parameter itself. Signed-off-by: Hans Petter Selasky --- drivers/media/video/tda7432.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/media/video/tda7432.c b/drivers/media/video/tda7432.c index 3941f95..398b9fb 100644 --- a/drivers/media/video/tda7432.c +++ b/drivers/media/video/tda7432.c @@ -50,8 +50,8 @@ static int loudness; /* disable loudness by default */ static int debug; /* insmod parameter */ module_param(debug, int, S_IRUGO | S_IWUSR); module_param(loudness, int, S_IRUGO); -MODULE_PARM_DESC(maxvol,"Set maximium volume to +20db (0), default is 0db(1)"); module_param(maxvol, int, S_IRUGO | S_IWUSR); +MODULE_PARM_DESC(maxvol,"Set maximium volume to +20db (0), default is 0db(1)"); -- 1.7.1.1
[PATCH] The USB_SPEED... keywords are already defined by the USB stack. Rename currently unused macros to avoid possible future warnings and errors.
--HPS From f7a0f7254da47ff88f69314f14043b01ba0764eb Mon Sep 17 00:00:00 2001 From: Hans Petter Selasky Date: Mon, 23 May 2011 12:43:50 +0200 Subject: [PATCH] The USB_SPEED_XXX keywords are already defined by the USB stack. Rename currently unused macros to avoid possible future warnings and errors. Signed-off-by: Hans Petter Selasky --- drivers/media/dvb/dvb-usb/gp8psk.h |6 +++--- drivers/media/dvb/dvb-usb/vp7045.h |6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/media/dvb/dvb-usb/gp8psk.h b/drivers/media/dvb/dvb-usb/gp8psk.h index 831749a..c271b68 100644 --- a/drivers/media/dvb/dvb-usb/gp8psk.h +++ b/drivers/media/dvb/dvb-usb/gp8psk.h @@ -78,9 +78,9 @@ extern int dvb_usb_gp8psk_debug; #define ADV_MOD_DVB_BPSK 9 /* DVB-S BPSK */ #define GET_USB_SPEED 0x07 - #define USB_SPEED_LOW0 - #define USB_SPEED_FULL 1 - #define USB_SPEED_HIGH 2 + #define TH_USB_SPEED_LOW 0 + #define TH_USB_SPEED_FULL1 + #define TH_USB_SPEED_HIGH2 #define RESET_FX2 0x13 diff --git a/drivers/media/dvb/dvb-usb/vp7045.h b/drivers/media/dvb/dvb-usb/vp7045.h index 969688f..7ce6c00 100644 --- a/drivers/media/dvb/dvb-usb/vp7045.h +++ b/drivers/media/dvb/dvb-usb/vp7045.h @@ -36,9 +36,9 @@ #define Tuner_Power_OFF 0 #define GET_USB_SPEED 0x07 - #define USB_SPEED_LOW0 - #define USB_SPEED_FULL 1 - #define USB_SPEED_HIGH 2 + #define TH_USB_SPEED_LOW 0 + #define TH_USB_SPEED_FULL1 + #define TH_USB_SPEED_HIGH2 #define LOCK_TUNER_COMMAND0x09 -- 1.7.1.1
[GIT PATCHES FOR 2.6.41] Add bitmask controls
Hi Mauro, These patches for 2.6.41 add support for bitmask controls, needed for the upcoming Flash API and HDMI API. Sakari, can you give your ack as well? The patch is the same as the original one posted April 4, except for a small change in the control logging based on feedback from Laurent and the new DocBook documentation. Regards, Hans The following changes since commit 87cf028f3aa1ed51fe29c36df548aa714dc7438f: [media] dm1105: GPIO handling added, I2C on GPIO added, LNB control through GPIO reworked (2011-05-21 11:10:28 -0300) are available in the git repository at: ssh://linuxtv.org/git/hverkuil/media_tree.git core3 Hans Verkuil (3): v4l2-ctrls: add new bitmask control type. vivi: add bitmask test control. DocBook: document V4L2_CTRL_TYPE_BITMASK. Documentation/DocBook/v4l/compat.xml |8 Documentation/DocBook/v4l/v4l2.xml |9 - Documentation/DocBook/v4l/vidioc-queryctrl.xml | 12 +++- drivers/media/video/v4l2-common.c |3 +++ drivers/media/video/v4l2-ctrls.c | 17 - drivers/media/video/vivi.c | 18 -- include/linux/videodev2.h |1 + 7 files changed, 63 insertions(+), 5 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Fix compile warning: Dereferencing type-punned pointer will break strict-aliasing rules.
--HPS From c7a1e3b3f761a8bd75799248dd83499f59eaedae Mon Sep 17 00:00:00 2001 From: Hans Petter Selasky Date: Mon, 23 May 2011 13:13:06 +0200 Subject: [PATCH] Fix compile warning: Dereferencing type-punned pointer will break strict-aliasing rules. Signed-off-by: Hans Petter Selasky --- drivers/media/video/v4l2-ioctl.c |4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c index 506edcc..213ba7d 100644 --- a/drivers/media/video/v4l2-ioctl.c +++ b/drivers/media/video/v4l2-ioctl.c @@ -2264,7 +2264,7 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size, break; } *user_ptr = (void __user *)buf->m.planes; - *kernel_ptr = (void **)&buf->m.planes; + *kernel_ptr = (void *)&buf->m.planes; *array_size = sizeof(struct v4l2_plane) * buf->length; ret = 1; } @@ -2278,7 +2278,7 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size, if (ctrls->count != 0) { *user_ptr = (void __user *)ctrls->controls; - *kernel_ptr = (void **)&ctrls->controls; + *kernel_ptr = (void *)&ctrls->controls; *array_size = sizeof(struct v4l2_ext_control) * ctrls->count; ret = 1; -- 1.7.1.1
[PATCH] Make nchg variable signed because the code compares this variable against negative values.
--HPS From b05d4913df24f11c7b7a2e07201bb87a04a949bc Mon Sep 17 00:00:00 2001 From: Hans Petter Selasky Date: Mon, 23 May 2011 13:09:18 +0200 Subject: [PATCH] Make nchg variable signed because the code compares this variable against negative values. Signed-off-by: Hans Petter Selasky --- drivers/media/video/gspca/sonixj.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/media/video/gspca/sonixj.c b/drivers/media/video/gspca/sonixj.c index 6415aff..81b8a60 100644 --- a/drivers/media/video/gspca/sonixj.c +++ b/drivers/media/video/gspca/sonixj.c @@ -60,7 +60,7 @@ struct sd { u32 pktsz; /* (used by pkt_scan) */ u16 npkt; - u8 nchg; + s8 nchg; s8 short_mark; u8 quality; /* image quality */ -- 1.7.1.1
[PATCH] Make code more readable by not using the return value of the WARN() macro. Set ret variable in an undefined case.
--HPS From 94b88b92763f9309018ba04c200a8842ce1ff0ed Mon Sep 17 00:00:00 2001 From: Hans Petter Selasky Date: Mon, 23 May 2011 13:07:08 +0200 Subject: [PATCH] Make code more readable by not using the return value of the WARN() macro. Set ret variable in an undefined case. Signed-off-by: Hans Petter Selasky --- drivers/media/video/sr030pc30.c |5 - 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/drivers/media/video/sr030pc30.c b/drivers/media/video/sr030pc30.c index c901721..6cc64c9 100644 --- a/drivers/media/video/sr030pc30.c +++ b/drivers/media/video/sr030pc30.c @@ -726,8 +726,10 @@ static int sr030pc30_s_power(struct v4l2_subdev *sd, int on) const struct sr030pc30_platform_data *pdata = info->pdata; int ret; - if (WARN(pdata == NULL, "No platform data!\n")) + if (pdata == NULL) { + WARN(1, "No platform data!\n"); return -ENOMEM; + } /* * Put sensor into power sleep mode before switching off @@ -746,6 +748,7 @@ static int sr030pc30_s_power(struct v4l2_subdev *sd, int on) if (on) { ret = sr030pc30_base_config(sd); } else { + ret = 0; info->curr_win = NULL; info->curr_fmt = NULL; } -- 1.7.1.1
[GIT PATCHES FOR 2.6.40] Fixes
Hi Mauro, Here are a few fixes: the first fixes a bug in the wl12xx drivers (I hope Matti's email is still correct). The second fixes a few DocBook validation errors, and the last fixes READ_ONLY and GRABBED handling in the control framework. Regards, Hans The following changes since commit 87cf028f3aa1ed51fe29c36df548aa714dc7438f: [media] dm1105: GPIO handling added, I2C on GPIO added, LNB control through GPIO reworked (2011-05-21 11:10:28 -0300) are available in the git repository at: ssh://linuxtv.org/git/hverkuil/media_tree.git fixes Hans Verkuil (3): wl12xx: g_volatile_ctrl fix: wrong field set. Media DocBook: fix validation errors. v4l2-ctrls: drivers should be able to ignore READ_ONLY and GRABBED flags Documentation/DocBook/dvb/dvbproperty.xml|5 ++- Documentation/DocBook/v4l/subdev-formats.xml | 10 ++-- drivers/media/radio/radio-wl1273.c |2 +- drivers/media/radio/wl128x/fmdrv_v4l2.c |2 +- drivers/media/video/v4l2-ctrls.c | 59 +- 5 files changed, 50 insertions(+), 28 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Fix warning about invalid trigraph sequence.
--HPS From e3b824b49f3d853ba16d9cdda836bd2fe81c7775 Mon Sep 17 00:00:00 2001 From: Hans Petter Selasky Date: Mon, 23 May 2011 12:59:37 +0200 Subject: [PATCH] Fix warning about invalid trigraph sequence. Signed-off-by: Hans Petter Selasky --- drivers/media/video/cpia2/cpia2_v4l.c |4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/media/video/cpia2/cpia2_v4l.c b/drivers/media/video/cpia2/cpia2_v4l.c index 5111bbc..e909838 100644 --- a/drivers/media/video/cpia2/cpia2_v4l.c +++ b/drivers/media/video/cpia2/cpia2_v4l.c @@ -438,7 +438,7 @@ static int cpia2_querycap(struct file *file, void *fh, struct v4l2_capability *v strcat(vc->card, " (676/"); break; default: - strcat(vc->card, " (???/"); + strcat(vc->card, " (XXX/"); break; } switch (cam->params.version.sensor_flags) { @@ -458,7 +458,7 @@ static int cpia2_querycap(struct file *file, void *fh, struct v4l2_capability *v strcat(vc->card, "500)"); break; default: - strcat(vc->card, "???)"); + strcat(vc->card, "XXX)"); break; } -- 1.7.1.1
[PATCH] The info and err macros are already defined by the USB stack. Rename these macros to avoid macro redefinition warnings.
--HPS From 83b2408914b9c02600c8288459ed869037efd1dd Mon Sep 17 00:00:00 2001 From: Hans Petter Selasky Date: Mon, 23 May 2011 12:54:21 +0200 Subject: [PATCH] The info and err macros are already defined by the USB stack. Rename these macros to avoid macro redefinition warnings. Signed-off-by: Hans Petter Selasky --- drivers/media/dvb/frontends/cx24123.c| 34 +- drivers/media/dvb/frontends/dib3000mb.c | 12 drivers/media/dvb/frontends/dib3000mb_priv.h | 10 +++ 3 files changed, 27 insertions(+), 29 deletions(-) diff --git a/drivers/media/dvb/frontends/cx24123.c b/drivers/media/dvb/frontends/cx24123.c index b1dd8ac..b73fb90 100644 --- a/drivers/media/dvb/frontends/cx24123.c +++ b/drivers/media/dvb/frontends/cx24123.c @@ -41,8 +41,8 @@ static int debug; module_param(debug, int, 0644); MODULE_PARM_DESC(debug, "Activates frontend debugging (default:0)"); -#define info(args...) do { printk(KERN_INFO "CX24123: " args); } while (0) -#define err(args...) do { printk(KERN_ERR "CX24123: " args); } while (0) +#define cx_info(args...) do { printk(KERN_INFO "CX24123: " args); } while (0) +#define cx_err(args...) do { printk(KERN_ERR "CX24123: " args); } while (0) #define dprintk(args...) \ do { \ @@ -274,7 +274,7 @@ static int cx24123_i2c_readreg(struct cx24123_state *state, u8 i2c_addr, u8 reg) ret = i2c_transfer(state->i2c, msg, 2); if (ret != 2) { - err("%s: reg=0x%x (error=%d)\n", __func__, reg, ret); + cx_err("%s: reg=0x%x (error=%d)\n", __func__, reg, ret); return ret; } @@ -620,7 +620,7 @@ static int cx24123_pll_writereg(struct dvb_frontend *fe, cx24123_writereg(state, 0x22, (data >> 16) & 0xff); while ((cx24123_readreg(state, 0x20) & 0x40) == 0) { if (time_after(jiffies, timeout)) { - err("%s: demodulator is not responding, "\ + cx_err("%s: demodulator is not responding, "\ "possibly hung, aborting.\n", __func__); return -EREMOTEIO; } @@ -632,7 +632,7 @@ static int cx24123_pll_writereg(struct dvb_frontend *fe, cx24123_writereg(state, 0x22, (data >> 8) & 0xff); while ((cx24123_readreg(state, 0x20) & 0x40) == 0) { if (time_after(jiffies, timeout)) { - err("%s: demodulator is not responding, "\ + cx_err("%s: demodulator is not responding, "\ "possibly hung, aborting.\n", __func__); return -EREMOTEIO; } @@ -645,7 +645,7 @@ static int cx24123_pll_writereg(struct dvb_frontend *fe, cx24123_writereg(state, 0x22, (data) & 0xff); while ((cx24123_readreg(state, 0x20) & 0x80)) { if (time_after(jiffies, timeout)) { - err("%s: demodulator is not responding," \ + cx_err("%s: demodulator is not responding," \ "possibly hung, aborting.\n", __func__); return -EREMOTEIO; } @@ -668,7 +668,7 @@ static int cx24123_pll_tune(struct dvb_frontend *fe, dprintk("frequency=%i\n", p->frequency); if (cx24123_pll_calculate(fe, p) != 0) { - err("%s: cx24123_pll_calcutate failed\n", __func__); + cx_err("%s: cx24123_pll_calcutate failed\n", __func__); return -EINVAL; } @@ -765,7 +765,7 @@ static void cx24123_wait_for_diseqc(struct cx24123_state *state) unsigned long timeout = jiffies + msecs_to_jiffies(200); while (!(cx24123_readreg(state, 0x29) & 0x40)) { if (time_after(jiffies, timeout)) { - err("%s: diseqc queue not ready, " \ + cx_err("%s: diseqc queue not ready, " \ "command may be lost.\n", __func__); break; } @@ -947,7 +947,7 @@ static int cx24123_set_frontend(struct dvb_frontend *fe, else if (fe->ops.tuner_ops.set_params) fe->ops.tuner_ops.set_params(fe, p); else - err("it seems I don't have a tuner..."); + cx_err("it seems I don't have a tuner..."); /* Enable automatic acquisition and reset cycle */ cx24123_writereg(state, 0x03, (cx24123_readreg(state, 0x03) | 0x07)); @@ -968,11 +968,11 @@ static int cx24123_get_frontend(struct dvb_frontend *fe, dprintk("\n"); if (cx24123_get_inversion(state, &p->inversion) != 0) { - err("%s: Failed to get inversion status\n", __func__); + cx_err("%s: Failed to get inversion status\n", __func__); return -EREMOTEIO; } if (cx24123_get_fec(state, &p->u.qpsk.fec_inner) != 0) { - err("%s: Failed to get fec status\n", __func__); + cx_err("%s: Failed to get fec status\n", __func__); return -EREMOTEIO; } p->frequency = state->currentfreq; @@ -999,7 +999,7 @@ static int cx24123_set_tone(struct dvb_frontend *fe, fe_sec_tone_mode_t tone) dprintk("setting tone off\n"); return cx24123_writereg(state, 0x29, val & 0xef); default: - err("CASE reached default with tone=%d\n", tone); + cx_err("CASE reached default with tone=%d\n", tone); return -EINVAL; } @@ -1075,7 +1075,7 @@ struct dvb_frontend *cx24123_attach(const struct cx24123_config *config, dprintk("\n"); if (state == NULL) { - err("Unable to kzalloc\n"); + cx_err("Unable to kzalloc\n"); goto error; } @@ -1087,13 +1087,13 @@ struct dvb_frontend *cx24123_attach(const struct cx24123_config *config,
[PATCH] Correct a module parameter description to describe the correct parameter.
--HPS From 0b5b16dfa302229060eb4ceae0498f0d60ab86c1 Mon Sep 17 00:00:00 2001 From: Hans Petter Selasky Date: Mon, 23 May 2011 12:50:01 +0200 Subject: [PATCH] Correct a module parameter description to describe the correct parameter. Signed-off-by: Hans Petter Selasky --- drivers/media/dvb/frontends/cx24116.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/media/dvb/frontends/cx24116.c b/drivers/media/dvb/frontends/cx24116.c index 2410d8b..cf1ec6c 100644 --- a/drivers/media/dvb/frontends/cx24116.c +++ b/drivers/media/dvb/frontends/cx24116.c @@ -137,7 +137,7 @@ MODULE_PARM_DESC(toneburst, "DiSEqC toneburst 0=OFF, 1=TONE CACHE, "\ /* SNR measurements */ static int esno_snr; module_param(esno_snr, int, 0644); -MODULE_PARM_DESC(debug, "SNR return units, 0=PERCENTAGE 0-100, "\ +MODULE_PARM_DESC(esno_snr, "SNR return units, 0=PERCENTAGE 0-100, "\ "1=ESNO(db * 10) (default:0)"); enum cmds { -- 1.7.1.1
build.sh fails on kernel 2.6.38
Hello, I tried to build the v4l-dvb on an ubuntu machine with kernel 2.6.38.8 generic and I get make error: = /home/z/media_build/v4l/kinect.c:38:19: error: ‘D_ERR’ undeclared here (not in a function) /home/z/media_build/v4l/kinect.c:38:27: error: ‘D_PROBE’ undeclared here (not in a function) /home/z/media_build/v4l/kinect.c:38:37: error: ‘D_CONF’ undeclared here (not in a function) /home/z/media_build/v4l/kinect.c:38:46: error: ‘D_STREAM’ undeclared here (not in a function) /home/z/media_build/v4l/kinect.c:38:57: error: ‘D_FRAM’ undeclared here (not in a function) /home/z/media_build/v4l/kinect.c:38:66: error: ‘D_PACK’ undeclared here (not in a function) /home/z/media_build/v4l/kinect.c:39:2: error: ‘D_USBI’ undeclared here (not in a function) /home//media_build/v4l/kinect.c:39:11: error: ‘D_USBO’ undeclared here (not in a function) /home//media_build/v4l/kinect.c:39:20: error: ‘D_V4L2′ undeclared here (not in a function) make[3]: *** [/home//media_build/v4l/kinect.o] Error 1 make[2]: *** [_module_/home//media_build/v4l] Error 2 = -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] MT9P031: Add support for Aptina mt9p031 sensor.
On Mon, 23 May 2011, Laurent Pinchart wrote: > > > +{ > > > + struct mt9p031 *mt9p031 = to_mt9p031(client); > > > + int ret; > > > + > > > + /* Disable chip output, synchronous option update */ > > > + ret = reg_write(client, MT9P031_RST, MT9P031_RST_ENABLE); > > > + if (ret < 0) > > > + return -EIO; > > > + ret = reg_write(client, MT9P031_RST, MT9P031_RST_DISABLE); > > > + if (ret < 0) > > > + return -EIO; > > > + ret = mt9p031_set_output_control(mt9p031, MT9P031_OUTPUT_CONTROL_CEN, > > > 0); + if (ret < 0) > > > + return -EIO; > > > + return 0; > > > > I think, a sequence like > > > > ret = fn(); > > if (!ret) > > ret = fn(); > > if (!ret) > > ret = fn(); > > return ret; > > > > is a better way to achieve the same. > > I disagree with you on that :-) I find code sequences that return as soon as > an error occurs, using the main code path for the error-free case, easier to > read. It can be a matter of personal taste though. Whichever way, but it should be consistent, IMHO. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[GIT PATCHES FOR 2.6.40] gspca for_v2.6.40
The following changes since commit 87cf028f3aa1ed51fe29c36df548aa714dc7438f: [media] dm1105: GPIO handling added, I2C on GPIO added, LNB control through GPIO reworked (2011-05-21 11:10:28 -0300) are available in the git repository at: git://linuxtv.org/jfrancois/gspca.git for_v2.6.40 Jean-François Moine (6): gspca - ov519: Fix a regression for ovfx2 webcams gspca - ov519: Change the ovfx2 bulk transfer size gspca: Remove coarse_expo_autogain.h gspca - stv06xx: Set a lower default value of gain for hdcs sensors gspca - ov519: New sensor ov9600 with bridge ovfx2 gspca - ov519: Set the default frame rate to 15 fps drivers/media/video/gspca/coarse_expo_autogain.h | 116 - drivers/media/video/gspca/ov519.c| 117 ++--- drivers/media/video/gspca/stv06xx/stv06xx_hdcs.h |2 +- 3 files changed, 101 insertions(+), 134 deletions(-) delete mode 100644 drivers/media/video/gspca/coarse_expo_autogain.h -- Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [beagleboard] [PATCH v2 2/2] OMAP3BEAGLE: Add support for mt9p031 sensor driver.
Hi Koen, On Monday 23 May 2011 10:55:53 Koen Kooi wrote: > Op 23 mei 2011, om 10:00 heeft Laurent Pinchart het volgende geschreven: > > On Monday 23 May 2011 09:01:07 javier Martin wrote: > >> On 20 May 2011 17:57, Koen Kooi wrote: > >>> In previous patch sets we put that in a seperate file > >>> (omap3beagle-camera.c) so we don't clutter up the board file with all > >>> the different sensor drivers. Would it make sense to do the same with > >>> the current patches? It looks like MCF cuts down a lot on the > >>> boilerplace needed already. > >> > >> I sent my first patch using that approach but I was told to move it to > >> the board code. > >> Please, don't make undo the changes. Or at least, let's discuss this > >> seriously so that we all agree on what is the best way of doing it and > >> I don't have to change it every time. > > > > What we really need here is a modular way to support sensors on pluggable > > expansion boards. Not all Beagleboard users will have an MT9P031 > > connected to the OMAP3 ISP, so that must not be hardcoded in board code. > > As the sensor boards are not runtime detectable > > Well, they are runtime detectable, you just need to read the ID register on > the sensor and they all share the same I2C address. Once you have the > sensor ID you can (re)setup the I2C. I don't think we can guarantee that all sensor boards that will ever be plugged into the Beagleboard will have a sensor ID register readable from a single I2C address at a single register offset. > But doing that in linux seems to be impossible with the current I2C > infrastructure. > > What we (beagleboard.org) are doing now: > > 1) set a bootarg in uboot e.g. camera=llbcm5mp > 2) read bootarg in linux boardfile and setup i2c > > What we are going to do medium term: > > 1) read ID in uboot, set bootarg > 2) read bootarg in linux boardfile > > Long term 1) will probably do some devicetree magic. The goal is to plug in > a sensor and boot, no manual modprobing, it just works. Device tree is definitely the way to go. using the camera parameter in board code to register the correct camera sounds good to me. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] MT9P031: Add support for Aptina mt9p031 sensor.
Hi Guennadi, On Monday 23 May 2011 10:48:36 Guennadi Liakhovetski wrote: > On Mon, 23 May 2011, javier Martin wrote: > > On 21 May 2011 17:29, Guennadi Liakhovetski wrote: > > > On Fri, 20 May 2011, Javier Martin wrote: > > >> This driver adds basic support for Aptina mt9p031 sensor. > > >> > > >> Signed-off-by: Javier Martin > > >> --- > > >> drivers/media/video/Kconfig |8 + > > >> drivers/media/video/Makefile |1 + > > >> drivers/media/video/mt9p031.c | 751 > > >> + include/media/mt9p031.h > > >> | 11 + > > >> 4 files changed, 771 insertions(+), 0 deletions(-) > > >> create mode 100644 drivers/media/video/mt9p031.c > > >> create mode 100644 include/media/mt9p031.h > > >> > > >> diff --git a/drivers/media/video/mt9p031.c > > >> b/drivers/media/video/mt9p031.c new file mode 100644 > > >> index 000..e406b64 > > >> --- /dev/null > > >> +++ b/drivers/media/video/mt9p031.c > > >> @@ -0,0 +1,751 @@ > > >> +/* > > >> + * Driver for MT9P031 CMOS Image Sensor from Aptina > > >> + * > > >> + * Copyright (C) 2011, Javier Martin > > >> + * > > >> + * Copyright (C) 2011, Guennadi Liakhovetski > > >> + * > > >> + * Based on the MT9V032 driver and Bastian Hecht's code. > > >> + * > > >> + * This program is free software; you can redistribute it and/or > > >> modify + * it under the terms of the GNU General Public License > > >> version 2 as + * published by the Free Software Foundation. > > >> + */ > > >> + > > >> +#include > > >> +#include > > >> +#include > > >> +#include > > >> +#include > > >> +#include > > >> +#include > > >> +#include > > >> +#include > > >> + > > >> +#include > > >> +#include > > >> +#include > > >> +#include > > >> + > > >> +/* mt9p031 selected register addresses */ > > >> +#define MT9P031_CHIP_VERSION 0x00 > > >> +#define MT9P031_CHIP_VERSION_VALUE 0x1801 > > >> +#define MT9P031_ROW_START0x01 > > > > > > Don't mix spaces and TABs between "#define" and the macro - just use > > > one space everywhere. > > > > I've done this in order to follow Laurent's directions. He does the > > same in mt9v032 driver. > > So, unless Laurent and you agree I think I won't change it. > > Ah, so, you use a space for registers and TABs for their values, ok then. > > > >> +struct mt9p031 { > > >> + struct v4l2_subdev subdev; > > >> + struct media_pad pad; > > >> + struct v4l2_rect rect; /* Sensor window */ > > >> + struct v4l2_mbus_framefmt format; > > >> + struct mt9p031_platform_data *pdata; > > >> + struct mutex power_lock; > > > > > > Don't locks _always_ have to be documented? And this one: you only > > > protect set_power() with it, Laurent, is this correct? > > > > Just following the model Laurent applies in mt9v032. Let's see what he > > has to say about this. > > Try running scripts/checkpatch.pl on your patch. I think, it will complain > about this. And in general it's a good idea to run it before submission;) > > > >> +static int mt9p031_reset(struct i2c_client *client) > > >> +{ > > >> + struct mt9p031 *mt9p031 = to_mt9p031(client); > > >> + int ret; > > >> + > > >> + /* Disable chip output, synchronous option update */ > > >> + ret = reg_write(client, MT9P031_RST, MT9P031_RST_ENABLE); > > >> + if (ret < 0) > > >> + return -EIO; > > >> + ret = reg_write(client, MT9P031_RST, MT9P031_RST_DISABLE); > > >> + if (ret < 0) > > >> + return -EIO; > > >> + ret = mt9p031_set_output_control(mt9p031, > > >> MT9P031_OUTPUT_CONTROL_CEN, 0); + if (ret < 0) > > >> + return -EIO; > > >> + return 0; > > > > > > I think, a sequence like > > > > > >ret = fn(); > > >if (!ret) > > >ret = fn(); > > >if (!ret) > > >ret = fn(); > > >return ret; > > > > > > is a better way to achieve the same. > > > > Sorry, but I have to disagree. I understand what you want to achieve > > but this seems quite tricky to me. > > I explicitly changed parts of the code that were written using that > > style because I think It was better understandable. > > Well, that was my opinion. Since Laurent will be taking this patch via his > tree, his decision will be final, of course. But I think, he'll agree, > that at least you have to be consistent across the driver. And at least > you'd want to propagate your error code up to the caller instead of > replacing it with "-EIO." To follow up on my previous answer on this, I agree that the return value should be propagated instead of replacing it with -EIO. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] MT9P031: Add support for Aptina mt9p031 sensor.
Hi Guennadi and Javier, On Saturday 21 May 2011 17:29:18 Guennadi Liakhovetski wrote: > On Fri, 20 May 2011, Javier Martin wrote: [snip] > > diff --git a/drivers/media/video/mt9p031.c > > b/drivers/media/video/mt9p031.c new file mode 100644 > > index 000..e406b64 > > --- /dev/null > > +++ b/drivers/media/video/mt9p031.c [snip] > > +#define MT9P031_ROW_START 0x01 > > Don't mix spaces and TABs between "#define" and the macro - just use one > space everywhere. > > > +#defineMT9P031_ROW_START_SKIP 54 That should be MT9P031_ROW_START_DEF. You should define MT9P031_ROW_START_MIN and MT9P031_ROW_START_MAX as well, you will need them. Same for column start, window height and window width. > > +#define MT9P031_COLUMN_START 0x02 > > +#defineMT9P031_COLUMN_START_SKIP 16 > > +#define MT9P031_WINDOW_HEIGHT 0x03 > > +#define MT9P031_WINDOW_WIDTH 0x04 > > +#define MT9P031_H_BLANKING 0x05 > > +#defineMT9P031_H_BLANKING_VALUE0 > > +#define MT9P031_V_BLANKING 0x06 > > +#defineMT9P031_V_BLANKING_VALUE25 > > +#define MT9P031_OUTPUT_CONTROL 0x07 > > +#defineMT9P031_OUTPUT_CONTROL_CEN 2 > > +#defineMT9P031_OUTPUT_CONTROL_SYN 1 > > +#define MT9P031_SHUTTER_WIDTH_UPPER0x08 > > +#define MT9P031_SHUTTER_WIDTH 0x09 > > +#define MT9P031_PIXEL_CLOCK_CONTROL0x0a > > +#define MT9P031_FRAME_RESTART 0x0b > > +#define MT9P031_SHUTTER_DELAY 0x0c > > +#define MT9P031_RST0x0d > > +#defineMT9P031_RST_ENABLE 1 > > +#defineMT9P031_RST_DISABLE 0 > > +#define MT9P031_READ_MODE_10x1e > > +#define MT9P031_READ_MODE_20x20 > > +#defineMT9P031_READ_MODE_2_ROW_MIR 0x8000 > > +#defineMT9P031_READ_MODE_2_COL_MIR 0x4000 > > +#define MT9P031_ROW_ADDRESS_MODE 0x22 > > +#define MT9P031_COLUMN_ADDRESS_MODE0x23 > > +#define MT9P031_GLOBAL_GAIN0x35 > > + > > +#define MT9P031_MAX_HEIGHT 1944 > > +#define MT9P031_MAX_WIDTH 2592 > > +#define MT9P031_MIN_HEIGHT 2 > > +#define MT9P031_MIN_WIDTH 18 You can get rid of those 4 #define's and use MT9P031_WINDOW_(HEIGHT| WIDTH)_(MIN|MAX) instead. > > +struct mt9p031 { > > + struct v4l2_subdev subdev; > > + struct media_pad pad; > > + struct v4l2_rect rect; /* Sensor window */ > > + struct v4l2_mbus_framefmt format; > > + struct mt9p031_platform_data *pdata; > > + struct mutex power_lock; > > Don't locks _always_ have to be documented? And this one: you only protect > set_power() with it, Laurent, is this correct? You're right, locks have to always be documented, either inline or in a comment block above the structure. A small comment such as /* Protects power_count */ is enough. > > + int power_count; > > + u16 xskip; > > + u16 yskip; > > + u16 output_control; > > + struct regulator *reg_1v8; > > + struct regulator *reg_2v8; > > +}; [snip] > > +static int mt9p031_reset(struct i2c_client *client) > > +{ > > + struct mt9p031 *mt9p031 = to_mt9p031(client); > > + int ret; > > + > > + /* Disable chip output, synchronous option update */ > > + ret = reg_write(client, MT9P031_RST, MT9P031_RST_ENABLE); > > + if (ret < 0) > > + return -EIO; > > + ret = reg_write(client, MT9P031_RST, MT9P031_RST_DISABLE); > > + if (ret < 0) > > + return -EIO; > > + ret = mt9p031_set_output_control(mt9p031, MT9P031_OUTPUT_CONTROL_CEN, > > 0); + if (ret < 0) > > + return -EIO; > > + return 0; > > I think, a sequence like > > ret = fn(); > if (!ret) > ret = fn(); > if (!ret) > ret = fn(); > return ret; > > is a better way to achieve the same. I disagree with you on that :-) I find code sequences that return as soon as an error occurs, using the main code path for the error-free case, easier to read. It can be a matter of personal taste though. This being said, the function can end with return mt9p031_set_output_control(mt9p031, MT9P031_OUTPUT_CONTROL_CEN, 0); instead of ret = mt9p031_set_output_control(mt9p031, MT9P031_OUTPUT_CONTROL_CEN, 0); if (ret < 0) return -EIO; return 0; > > +} > > + > > +static int mt9p031_power_on(struct mt9p031 *mt9p031) > > +{ > > + int ret; > > + > > + /* turn on VDD_IO */ > > + ret = regulator_enable(mt9p031->reg_2v8); > > + if (ret) { > > + pr_err("Failed to enable 2.8v regulator: %d\n", ret); > > dev_err() > > > + return ret; > > + } > > + if (mt9p031->pdata->set_xclk
Re: [beagleboard] [PATCH v2 2/2] OMAP3BEAGLE: Add support for mt9p031 sensor driver.
Op 23 mei 2011, om 10:00 heeft Laurent Pinchart het volgende geschreven: > Hi Javier, > > On Monday 23 May 2011 09:01:07 javier Martin wrote: >> On 20 May 2011 17:57, Koen Kooi wrote: >>> In previous patch sets we put that in a seperate file >>> (omap3beagle-camera.c) so we don't clutter up the board file with all >>> the different sensor drivers. Would it make sense to do the same with >>> the current patches? It looks like MCF cuts down a lot on the >>> boilerplace needed already. >> >> I sent my first patch using that approach but I was told to move it to >> the board code. >> Please, don't make undo the changes. Or at least, let's discuss this >> seriously so that we all agree on what is the best way of doing it and >> I don't have to change it every time. > > What we really need here is a modular way to support sensors on pluggable > expansion boards. Not all Beagleboard users will have an MT9P031 connected to > the OMAP3 ISP, so that must not be hardcoded in board code. As the sensor > boards are not runtime detectable Well, they are runtime detectable, you just need to read the ID register on the sensor and they all share the same I2C address. Once you have the sensor ID you can (re)setup the I2C. But doing that in linux seems to be impossible with the current I2C infrastructure. What we (beagleboard.org) are doing now: 1) set a bootarg in uboot e.g. camera=llbcm5mp 2) read bootarg in linux boardfile and setup i2c What we are going to do medium term: 1) read ID in uboot, set bootarg 2) read bootarg in linux boardfile Long term 1) will probably do some devicetree magic. The goal is to plug in a sensor and boot, no manual modprobing, it just works. regards, Koen-- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] MT9P031: Add support for Aptina mt9p031 sensor.
On Mon, 23 May 2011, javier Martin wrote: > On 21 May 2011 17:29, Guennadi Liakhovetski wrote: > > On Fri, 20 May 2011, Javier Martin wrote: > > > >> This driver adds basic support for Aptina mt9p031 sensor. > >> > >> Signed-off-by: Javier Martin > >> --- > >> drivers/media/video/Kconfig | 8 + > >> drivers/media/video/Makefile | 1 + > >> drivers/media/video/mt9p031.c | 751 > >> + > >> include/media/mt9p031.h | 11 + > >> 4 files changed, 771 insertions(+), 0 deletions(-) > >> create mode 100644 drivers/media/video/mt9p031.c > >> create mode 100644 include/media/mt9p031.h > >> > >> diff --git a/drivers/media/video/mt9p031.c b/drivers/media/video/mt9p031.c > >> new file mode 100644 > >> index 000..e406b64 > >> --- /dev/null > >> +++ b/drivers/media/video/mt9p031.c > >> @@ -0,0 +1,751 @@ > >> +/* > >> + * Driver for MT9P031 CMOS Image Sensor from Aptina > >> + * > >> + * Copyright (C) 2011, Javier Martin > >> + * > >> + * Copyright (C) 2011, Guennadi Liakhovetski > >> + * > >> + * Based on the MT9V032 driver and Bastian Hecht's code. > >> + * > >> + * This program is free software; you can redistribute it and/or modify > >> + * it under the terms of the GNU General Public License version 2 as > >> + * published by the Free Software Foundation. > >> + */ > >> + > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> + > >> +#include > >> +#include > >> +#include > >> +#include > >> + > >> +/* mt9p031 selected register addresses */ > >> +#define MT9P031_CHIP_VERSION 0x00 > >> +#define MT9P031_CHIP_VERSION_VALUE 0x1801 > >> +#define MT9P031_ROW_START 0x01 > > > > Don't mix spaces and TABs between "#define" and the macro - just use one > > space everywhere. > > > > I've done this in order to follow Laurent's directions. He does the > same in mt9v032 driver. > So, unless Laurent and you agree I think I won't change it. Ah, so, you use a space for registers and TABs for their values, ok then. > >> +struct mt9p031 { > >> + struct v4l2_subdev subdev; > >> + struct media_pad pad; > >> + struct v4l2_rect rect; /* Sensor window */ > >> + struct v4l2_mbus_framefmt format; > >> + struct mt9p031_platform_data *pdata; > >> + struct mutex power_lock; > > > > Don't locks _always_ have to be documented? And this one: you only protect > > set_power() with it, Laurent, is this correct? > > > > Just following the model Laurent applies in mt9v032. Let's see what he > has to say about this. Try running scripts/checkpatch.pl on your patch. I think, it will complain about this. And in general it's a good idea to run it before submission;) > >> +static int mt9p031_reset(struct i2c_client *client) > >> +{ > >> + struct mt9p031 *mt9p031 = to_mt9p031(client); > >> + int ret; > >> + > >> + /* Disable chip output, synchronous option update */ > >> + ret = reg_write(client, MT9P031_RST, MT9P031_RST_ENABLE); > >> + if (ret < 0) > >> + return -EIO; > >> + ret = reg_write(client, MT9P031_RST, MT9P031_RST_DISABLE); > >> + if (ret < 0) > >> + return -EIO; > >> + ret = mt9p031_set_output_control(mt9p031, > >> MT9P031_OUTPUT_CONTROL_CEN, 0); > >> + if (ret < 0) > >> + return -EIO; > >> + return 0; > > > > > > I think, a sequence like > > > > ret = fn(); > > if (!ret) > > ret = fn(); > > if (!ret) > > ret = fn(); > > return ret; > > > > is a better way to achieve the same. > > > > Sorry, but I have to disagree. I understand what you want to achieve > but this seems quite tricky to me. > I explicitly changed parts of the code that were written using that > style because I think It was better understandable. Well, that was my opinion. Since Laurent will be taking this patch via his tree, his decision will be final, of course. But I think, he'll agree, that at least you have to be consistent across the driver. And at least you'd want to propagate your error code up to the caller instead of replacing it with "-EIO." Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v17 5/6] davinci vpbe: Build infrastructure for VPBE driver
On Fri, May 20, 2011 at 20:02:08, Sergei Shtylyov wrote: > Hello. > > Manjunath Hadli wrote: > > > This patch adds the build infra-structure for Davinci > > VPBE dislay driver. > > > Signed-off-by: Manjunath Hadli > > Acked-by: Muralidharan Karicheri > > Acked-by: Hans Verkuil > [...] > > > diff --git a/drivers/media/video/davinci/Kconfig > > b/drivers/media/video/davinci/Kconfig > > index 6b19540..a7f11e7 100644 > > --- a/drivers/media/video/davinci/Kconfig > > +++ b/drivers/media/video/davinci/Kconfig > > @@ -91,3 +91,25 @@ config VIDEO_ISIF > > > >To compile this driver as a module, choose M here: the > >module will be called vpfe. > > + > > +config VIDEO_DM644X_VPBE > > + tristate "DM644X VPBE HW module" > > BTW, as this seems DM644x specific, shouldn't this depend on > CONFIG_ARCH_DAVINCI_DM644x? Since VENC/OSD etc are also applicable to other DaVinci devices, this KConfig entry should probably be split to refer to them individually and in a generic way. "depends on" can then be used to make sure only the relevant ones show up. Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] MT9P031: Add support for Aptina mt9p031 sensor.
On 21 May 2011 17:29, Guennadi Liakhovetski wrote: > On Fri, 20 May 2011, Javier Martin wrote: > >> This driver adds basic support for Aptina mt9p031 sensor. >> >> Signed-off-by: Javier Martin >> --- >> drivers/media/video/Kconfig | 8 + >> drivers/media/video/Makefile | 1 + >> drivers/media/video/mt9p031.c | 751 >> + >> include/media/mt9p031.h | 11 + >> 4 files changed, 771 insertions(+), 0 deletions(-) >> create mode 100644 drivers/media/video/mt9p031.c >> create mode 100644 include/media/mt9p031.h >> >> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig >> index 00f51dd..5c96b89 100644 >> --- a/drivers/media/video/Kconfig >> +++ b/drivers/media/video/Kconfig >> @@ -329,6 +329,14 @@ config VIDEO_OV7670 >> OV7670 VGA camera. It currently only works with the M88ALP01 >> controller. >> >> +config VIDEO_MT9P031 >> + tristate "Aptina MT9P031 support" >> + depends on I2C && VIDEO_V4L2 >> + ---help--- >> + This driver supports MT9P031 cameras from Micron >> + This is a Video4Linux2 sensor-level driver for the Micron >> + mt0p031 5 Mpixel camera. > > Two sentences seem to repeat the same with other words, and it's better to > stay consistent: just use Aptina everywhere, maybe put Micron in brackets > at one location. > OK, I will fix it. >> + >> config VIDEO_MT9V011 >> tristate "Micron mt9v011 sensor support" >> depends on I2C && VIDEO_V4L2 >> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile >> index ace5d8b..912b29b 100644 >> --- a/drivers/media/video/Makefile >> +++ b/drivers/media/video/Makefile >> @@ -65,6 +65,7 @@ obj-$(CONFIG_VIDEO_UPD64083) += upd64083.o >> obj-$(CONFIG_VIDEO_OV7670) += ov7670.o >> obj-$(CONFIG_VIDEO_TCM825X) += tcm825x.o >> obj-$(CONFIG_VIDEO_TVEEPROM) += tveeprom.o >> +obj-$(CONFIG_VIDEO_MT9P031) += mt9p031.o >> obj-$(CONFIG_VIDEO_MT9V011) += mt9v011.o >> obj-$(CONFIG_VIDEO_SR030PC30) += sr030pc30.o >> obj-$(CONFIG_VIDEO_NOON010PC30) += noon010pc30.o >> diff --git a/drivers/media/video/mt9p031.c b/drivers/media/video/mt9p031.c >> new file mode 100644 >> index 000..e406b64 >> --- /dev/null >> +++ b/drivers/media/video/mt9p031.c >> @@ -0,0 +1,751 @@ >> +/* >> + * Driver for MT9P031 CMOS Image Sensor from Aptina >> + * >> + * Copyright (C) 2011, Javier Martin >> + * >> + * Copyright (C) 2011, Guennadi Liakhovetski >> + * >> + * Based on the MT9V032 driver and Bastian Hecht's code. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> +#include >> +#include >> +#include >> + >> +/* mt9p031 selected register addresses */ >> +#define MT9P031_CHIP_VERSION 0x00 >> +#define MT9P031_CHIP_VERSION_VALUE 0x1801 >> +#define MT9P031_ROW_START 0x01 > > Don't mix spaces and TABs between "#define" and the macro - just use one > space everywhere. > I've done this in order to follow Laurent's directions. He does the same in mt9v032 driver. So, unless Laurent and you agree I think I won't change it. > >> +#define MT9P031_ROW_START_SKIP 54 >> +#define MT9P031_COLUMN_START 0x02 >> +#define MT9P031_COLUMN_START_SKIP 16 >> +#define MT9P031_WINDOW_HEIGHT 0x03 >> +#define MT9P031_WINDOW_WIDTH 0x04 >> +#define MT9P031_H_BLANKING 0x05 >> +#define MT9P031_H_BLANKING_VALUE 0 >> +#define MT9P031_V_BLANKING 0x06 >> +#define MT9P031_V_BLANKING_VALUE 25 >> +#define MT9P031_OUTPUT_CONTROL 0x07 >> +#define MT9P031_OUTPUT_CONTROL_CEN 2 >> +#define MT9P031_OUTPUT_CONTROL_SYN 1 >> +#define MT9P031_SHUTTER_WIDTH_UPPER 0x08 >> +#define MT9P031_SHUTTER_WIDTH 0x09 >> +#define MT9P031_PIXEL_CLOCK_CONTROL 0x0a >> +#define MT9P031_FRAME_RESTART 0x0b >> +#define MT9P031_SHUTTER_DELAY 0x0c >> +#define MT9P031_RST 0x0d >> +#define MT9P031_RST_ENABLE 1 >> +#define MT9P031_RST_DISABLE 0 >> +#define MT9P031_READ_MODE_1 0x1e >> +#define MT9P031_READ_MODE_2 0x20 >> +#define MT9P031_READ_MODE_2_ROW_MIR 0x8000 >> +#define MT9P031_READ_MODE_2_COL_MIR 0x4000 >> +#define MT9P031_ROW_ADDRESS_MODE 0x22 >> +#define MT9P031_COLUMN_ADDRESS_MODE 0x23 >> +#define MT9P031_GLOBAL_GAIN 0x35 >> + >> +#define
Re: [beagleboard] [PATCH v2 2/2] OMAP3BEAGLE: Add support for mt9p031 sensor driver.
Hi Javier, On Monday 23 May 2011 09:01:07 javier Martin wrote: > On 20 May 2011 17:57, Koen Kooi wrote: > > In previous patch sets we put that in a seperate file > > (omap3beagle-camera.c) so we don't clutter up the board file with all > > the different sensor drivers. Would it make sense to do the same with > > the current patches? It looks like MCF cuts down a lot on the > > boilerplace needed already. > > I sent my first patch using that approach but I was told to move it to > the board code. > Please, don't make undo the changes. Or at least, let's discuss this > seriously so that we all agree on what is the best way of doing it and > I don't have to change it every time. What we really need here is a modular way to support sensors on pluggable expansion boards. Not all Beagleboard users will have an MT9P031 connected to the OMAP3 ISP, so that must not be hardcoded in board code. As the sensor boards are not runtime detectable, one solution would be to compile part of the code as a module. Regulator definitions and I2C2 bus registration (and possibly GPIO initialization) can be left in board code. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] OMAP3BEAGLE: Add support for mt9p031 sensor driver.
Hi Javier, On Monday 23 May 2011 09:25:01 javier Martin wrote: > On 22 May 2011 15:49, Igor Grinberg wrote: [snip] > >> @@ -309,6 +357,15 @@ static int beagle_twl_gpio_setup(struct device > >> *dev, pr_err("%s: unable to configure EHCI_nOC\n", __func__); } > >> > >> + if (omap3_beagle_get_rev() == OMAP3BEAGLE_BOARD_XM) { > >> + /* > >> + * Power on camera interface - only on pre-production, not > >> + * needed on production boards > >> + */ > >> + gpio_request(gpio + 2, "CAM_EN"); > >> + gpio_direction_output(gpio + 2, 1); > > > > Why not gpio_request_one()? > > Just to follow the same approach as in the rest of the code. > Maybe a further patch could change all "gpio_request()" uses to > "gpio_request_one()". Please do it the other way around. Replace calls to gpio_request() + gpio_direction_output() with a call to gpio_request_one(), and then modify this patch to use gpio_request_one(). If you want to learn how to use coccinelle (http://coccinelle.lip6.fr/), now would be a good time. You could use it to replace gpio_request() + gpio_direction_output() through the whole kernel. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] OMAP3BEAGLE: Add support for mt9p031 sensor driver.
On 22 May 2011 15:49, Igor Grinberg wrote: > Hi Javier, > > > linux-omap should be CC'ed - added. > > In addition to Koen's comments, some comments below. > > > On 05/20/11 16:47, Javier Martin wrote: > >> isp.h file has to be included as a temporal measure >> since clocks of the isp are not exposed yet. >> >> Signed-off-by: Javier Martin >> --- >> arch/arm/mach-omap2/board-omap3beagle.c | 127 >> ++- >> 1 files changed, 123 insertions(+), 4 deletions(-) >> >> diff --git a/arch/arm/mach-omap2/board-omap3beagle.c >> b/arch/arm/mach-omap2/board-omap3beagle.c >> index 33007fd..f52e6ae 100644 >> --- a/arch/arm/mach-omap2/board-omap3beagle.c >> +++ b/arch/arm/mach-omap2/board-omap3beagle.c >> @@ -24,15 +24,20 @@ >> #include >> #include >> #include >> +#include >> +#include >> +#include >> >> #include >> #include >> #include >> #include >> - >> +#include > > Why include this for second time? Good catch, I'll fix it. [snip] >> @@ -309,6 +357,15 @@ static int beagle_twl_gpio_setup(struct device *dev, >> pr_err("%s: unable to configure EHCI_nOC\n", __func__); >> } >> >> + if (omap3_beagle_get_rev() == OMAP3BEAGLE_BOARD_XM) { >> + /* >> + * Power on camera interface - only on pre-production, not >> + * needed on production boards >> + */ >> + gpio_request(gpio + 2, "CAM_EN"); >> + gpio_direction_output(gpio + 2, 1); > > Why not gpio_request_one()? Just to follow the same approach as in the rest of the code. Maybe a further patch could change all "gpio_request()" uses to "gpio_request_one()". > >> + } >> + >> /* >> * TWL4030_GPIO_MAX + 0 == ledA, EHCI nEN_USB_PWR (out, XM active >> * high / others active low) >> @@ -451,6 +508,8 @@ static struct twl4030_platform_data beagle_twldata = { >> .vsim = &beagle_vsim, >> .vdac = &beagle_vdac, >> .vpll2 = &beagle_vpll2, >> + .vaux3 = &beagle_vaux3, >> + .vaux4 = &beagle_vaux4, >> }; >> >> static struct i2c_board_info __initdata beagle_i2c_boardinfo[] = { >> @@ -463,15 +522,16 @@ static struct i2c_board_info __initdata >> beagle_i2c_boardinfo[] = { >> }; >> >> static struct i2c_board_info __initdata beagle_i2c_eeprom[] = { >> - { >> - I2C_BOARD_INFO("eeprom", 0x50), >> - }, >> + { >> + I2C_BOARD_INFO("eeprom", 0x50), >> + }, >> }; > > This part of the hunk is not related to the patch > and should be in a separate (cleanup) patch. > Sure, I'll fix it. >> >> static int __init omap3_beagle_i2c_init(void) >> { >> omap_register_i2c_bus(1, 2600, beagle_i2c_boardinfo, >> ARRAY_SIZE(beagle_i2c_boardinfo)); >> + omap_register_i2c_bus(2, 100, NULL, 0); /* Enable I2C2 for camera */ >> /* Bus 3 is attached to the DVI port where devices like the pico DLP >> * projector don't work reliably with 400kHz */ >> omap_register_i2c_bus(3, 100, beagle_i2c_eeprom, >> ARRAY_SIZE(beagle_i2c_eeprom)); >> @@ -654,6 +714,60 @@ static void __init beagle_opp_init(void) >> return; >> } >> >> +static int beagle_cam_set_xclk(struct v4l2_subdev *subdev, int hz) >> +{ >> + struct isp_device *isp = v4l2_dev_to_isp_device(subdev->v4l2_dev); >> + int ret; >> + >> + ret = isp->platform_cb.set_xclk(isp, hz, MT9P031_XCLK); >> + return 0; >> +} > > Why do you need ret variable here, if you always return zero? > You are right, it's not needed any longer. >> + >> +static int beagle_cam_reset(struct v4l2_subdev *subdev, int active) >> +{ >> + /* Set RESET_BAR to !active */ >> + gpio_set_value(MT9P031_RESET_GPIO, !active); >> + >> + return 0; >> +} >> + >> +static struct mt9p031_platform_data beagle_mt9p031_platform_data = { >> + .set_xclk = beagle_cam_set_xclk, >> + .reset = beagle_cam_reset, >> +}; >> + >> +static struct i2c_board_info mt9p031_camera_i2c_device = { >> + I2C_BOARD_INFO("mt9p031", 0x48), >> + .platform_data = &beagle_mt9p031_platform_data, >> +}; >> + >> +static struct isp_subdev_i2c_board_info mt9p031_camera_subdevs[] = { >> + { >> + .board_info = &mt9p031_camera_i2c_device, >> + .i2c_adapter_id = 2, >> + }, >> + { NULL, 0, }, >> +}; >> + >> +static struct isp_v4l2_subdevs_group beagle_camera_subdevs[] = { >> + { >> + .subdevs = mt9p031_camera_subdevs, >> + .interface = ISP_INTERFACE_PARALLEL, >> + .bus = { >> + .parallel = { >> + .data_lane_shift = 0, >> + .clk_pol = 1, >> + .bridge = ISPCTRL_PAR_BRIDGE_DISABLE, >> + } >> + }, >> + }, >> + { }, >> +}; >> + >> +static struct isp_platform_data beagle_isp_platform_data = { >> + .subdevs = bea
Re: [beagleboard] [PATCH v2 2/2] OMAP3BEAGLE: Add support for mt9p031 sensor driver.
On 20 May 2011 17:57, Koen Kooi wrote: > In previous patch sets we put that in a seperate file (omap3beagle-camera.c) > so we don't clutter up the board file with all the different sensor drivers. > Would it make sense to do the same with the current patches? It looks like > MCF cuts down a lot on the boilerplace needed already. I sent my first patch using that approach but I was told to move it to the board code. Please, don't make undo the changes. Or at least, let's discuss this seriously so that we all agree on what is the best way of doing it and I don't have to change it every time. -- Javier Martin Vista Silicon S.L. CDTUC - FASE C - Oficina S-345 Avda de los Castros s/n 39005- Santander. Cantabria. Spain +34 942 25 32 60 www.vista-silicon.com -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html