Hi Laurent, Thanks for the review! Laurent Pinchart wrote: > On Tuesday 20 December 2011 21:28:00 Sakari Ailus wrote: >> From: Sakari Ailus <sakari.ailus@xxxxxx> >> >> Add image source control class. This control class is intended to contain >> low level controls which deal with control of the image capture process --- >> the A/D converter in image sensors, for example. >> >> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxx> >> --- >> Documentation/DocBook/media/v4l/controls.xml | 101 +++++++++++++++++ >> .../DocBook/media/v4l/vidioc-g-ext-ctrls.xml | 6 + >> drivers/media/video/v4l2-ctrls.c | 10 ++ >> include/linux/videodev2.h | 10 ++ >> 4 files changed, 127 insertions(+), 0 deletions(-) >> >> diff --git a/Documentation/DocBook/media/v4l/controls.xml >> b/Documentation/DocBook/media/v4l/controls.xml index 3bc5ee8..69ede83 >> 100644 >> --- a/Documentation/DocBook/media/v4l/controls.xml >> +++ b/Documentation/DocBook/media/v4l/controls.xml >> @@ -3356,6 +3356,107 @@ interface and may change in the future.</para> >> </table> >> >> </section> >> + >> + <section id="image-source-controls"> >> + <title>Image Source Control Reference</title> >> + >> + <note> >> + <title>Experimental</title> >> + >> + <para>This is an <link >> + linkend="experimental">experimental</link> interface and may >> + change in the future.</para> >> + </note> >> + >> + <para> >> + The Image Source control class is intended for low-level >> + control of image source devices such as image sensors. The >> + devices feature an analogue to digital converter and a bus > > Is the V4L2 documentation written in US or UK English ? US uses analog, UK > uses analogue. Analog would be shorter in control names. Both appear to be used, but the US English appears to be more commonly used. I guess it's mostly chosen by whatever happened to be used by the author of the patch. I prefer UK spelling which you might have noticed already. :-) I remember there was a discussion on this topic years ago within the kernel community but I don't remember how it ended up with... LWN.net appears to remember better than I do, but by a quick check I can't find any definitive conclusion. <URL:http://lwn.net/Articles/44262/> <URL:http://lkml.org/lkml/2003/8/7/245> >> + transmitter to transmit the image data out of the device. >> + </para> >> + >> + <table pgwide="1" frame="none" id="image-source-control-id"> >> + <title>Image Source Control IDs</title> >> + >> + <tgroup cols="4"> >> + <colspec colname="c1" colwidth="1*" /> >> + <colspec colname="c2" colwidth="6*" /> >> + <colspec colname="c3" colwidth="2*" /> >> + <colspec colname="c4" colwidth="6*" /> >> + <spanspec namest="c1" nameend="c2" spanname="id" /> >> + <spanspec namest="c2" nameend="c4" spanname="descr" /> >> + <thead> >> + <row> >> + <entry spanname="id" align="left">ID</entry> >> + <entry align="left">Type</entry> >> + </row><row rowsep="1"><entry spanname="descr" >> align="left">Description</entry> + </row> >> + </thead> >> + <tbody valign="top"> >> + <row><entry></entry></row> >> + <row> >> + <entry >> spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_CLASS</constant></entry> + >> <entry>class</entry> >> + </row> >> + <row> >> + <entry spanname="descr">The IMAGE_SOURCE class descriptor.</entry> >> + </row> >> + <row> >> + <entry >> spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_VBLANK</constant></entry> + >> <entry>integer</entry> >> + </row> >> + <row> >> + <entry spanname="descr">Vertical blanking. The idle >> + preriod after every frame during which no image data is > > s/preriod/period/ > >> + produced. The unit of vertical blanking is a line. Every >> + line has length of the image width plus horizontal >> + blanking at the pixel clock specified by struct >> + v4l2_mbus_framefmt <xref linkend="v4l2-mbus-framefmt" >> + />.</entry> >> + </row> >> + <row> >> + <entry >> spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_HBLANK</constant></entry> + >> <entry>integer</entry> >> + </row> >> + <row> >> + <entry spanname="descr">Horizontal blanking. The idle >> + preriod after every line of image data during which no > > s/preriod/period/ > >> + image data is produced. The unit of horizontal blanking is >> + pixels.</entry> >> + </row> >> + <row> >> + <entry >> spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_LINK_FREQ</constant></entry> >> + <entry>integer menu</entry> >> + </row> >> + <row> >> + <entry spanname="descr">Image source's data bus frequency. > > What's the frequency unit ? Sample/second ? Hz --- that's the unit of frequency. I fixed that in the new version. >> + Together with the media bus pixel code, bus type (clock >> + cycles per sample), the data bus frequency defines the >> + pixel clock. <xref linkend="v4l2-mbus-framefmt" /> The >> + frame rate can be calculated from the pixel clock, image >> + width and height and horizontal and vertical blanking. The >> + frame rate control is performed by selecting the desired >> + horizontal and vertical blanking. >> + </entry> >> + </row> >> + <row> >> + <entry >> spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_ANALOGUE_GAIN</constant></en >> try> + <entry>integer</entry> >> + </row> >> + <row> >> + <entry spanname="descr">Analogue gain is gain affecting >> + all colour components in the pixel matrix. The gain >> + operation is performed in the analogue domain before A/D >> + conversion. > > Should we define one gain per color component ? I think that in the end we may need up to six analogue gains: - Gain for all components - Blue gain - Red gain - Green gain (for both greens) - Gr gain - Gb gain It may be debatable whether Gr / Gb gain will always be the same or not. I'm not fully certain about that. As Hans G. suggested, it might be possible to go with just one for green. >> + </entry> >> + </row> >> + <row><entry></entry></row> >> + </tbody> >> + </tgroup> >> + </table> >> + >> + </section> >> + >> </section> >> >> <!-- >> diff --git a/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml >> b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml index >> 5122ce8..250c1cf 100644 >> --- a/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml >> +++ b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml >> @@ -257,6 +257,12 @@ These controls are described in <xref >> These controls are described in <xref >> linkend="flash-controls" />.</entry> >> </row> >> + <row> >> + <entry><constant>V4L2_CTRL_CLASS_IMAGE_SOURCE</constant></entry> >> + <entry>0x9d0000</entry> <entry>The class containing image >> + source controls. These controls are described in <xref >> + linkend="image-source-controls" />.</entry> >> + </row> >> </tbody> >> </tgroup> >> </table> >> diff --git a/drivers/media/video/v4l2-ctrls.c >> b/drivers/media/video/v4l2-ctrls.c index 083bb79..da1ec52 100644 >> --- a/drivers/media/video/v4l2-ctrls.c >> +++ b/drivers/media/video/v4l2-ctrls.c >> @@ -606,6 +606,12 @@ const char *v4l2_ctrl_get_name(u32 id) >> case V4L2_CID_FLASH_CHARGE: return "Charge"; >> case V4L2_CID_FLASH_READY: return "Ready to strobe"; >> >> + case V4L2_CID_IMAGE_SOURCE_CLASS: return "Image source controls"; >> + case V4L2_CID_IMAGE_SOURCE_VBLANK: return "Vertical blanking"; >> + case V4L2_CID_IMAGE_SOURCE_HBLANK: return "Horizontal blanking"; >> + case V4L2_CID_IMAGE_SOURCE_LINK_FREQ: return "Link frequency"; >> + case V4L2_CID_IMAGE_SOURCE_ANALOGUE_GAIN: return "Analogue gain"; > > Please capitalize each word, as done for the other controls. This isn't done for the flash controls either, have you noticed that? Well, I guess I have to admit that they were added by myself. ;-) I can fix this for the next patchset. >> default: >> return NULL; >> } >> @@ -694,6 +700,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum >> v4l2_ctrl_type *type, case V4L2_CID_MPEG_VIDEO_MPEG4_PROFILE: >> *type = V4L2_CTRL_TYPE_MENU; >> break; >> + case V4L2_CID_IMAGE_SOURCE_LINK_FREQ: >> + *type = V4L2_CTRL_TYPE_INTEGER_MENU; >> + break; > > Will this always be an integer menu control, or can you foresee cases where > the range would be continuous ? Good question. On some hardware this definitely is an integer menu, but hardware may exist where more selections would be available. However, on e.g. the SMIA++ sensor the calculation of the clock tree values depends heavily on the selected link rate. Choosing a wrong link rate may yield to the clock tree calculation to fail. So the driver likely would need to enforce some rules which values are allowed. That might prove unfeasible --- already the PLL code in the SMIA++ driver is relatively complex. I don't see much benefit in being able to specify this freely. AFAIR the conclusion was that controls may only have one type when the control framework was written. > We already have drivers for Aptina parallel sensors that include a > configurable PLL. The pixel clock can be configured more or less freely in > that case, although I'm not sure if that's useful in practice. > >> case V4L2_CID_RDS_TX_PS_NAME: >> case V4L2_CID_RDS_TX_RADIO_TEXT: >> *type = V4L2_CTRL_TYPE_STRING; >> @@ -703,6 +712,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum >> v4l2_ctrl_type *type, case V4L2_CID_MPEG_CLASS: >> case V4L2_CID_FM_TX_CLASS: >> case V4L2_CID_FLASH_CLASS: >> + case V4L2_CID_IMAGE_SOURCE_CLASS: >> *type = V4L2_CTRL_TYPE_CTRL_CLASS; >> /* You can neither read not write these */ >> *flags |= V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY; >> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h >> index 9633c69..0f8f904 100644 >> --- a/include/linux/videodev2.h >> +++ b/include/linux/videodev2.h >> @@ -1080,6 +1080,7 @@ struct v4l2_ext_controls { >> #define V4L2_CTRL_CLASS_CAMERA 0x009a0000 /* Camera class controls */ >> #define V4L2_CTRL_CLASS_FM_TX 0x009b0000 /* FM Modulator control class */ >> #define V4L2_CTRL_CLASS_FLASH 0x009c0000 /* Camera flash controls */ >> +#define V4L2_CTRL_CLASS_IMAGE_SOURCE 0x009d0000 /* Image source flash >> controls */ >> >> #define V4L2_CTRL_ID_MASK (0x0fffffff) >> #define V4L2_CTRL_ID2CLASS(id) ((id) & 0x0fff0000UL) >> @@ -1690,6 +1691,15 @@ enum v4l2_flash_strobe_source { >> #define V4L2_CID_FLASH_CHARGE (V4L2_CID_FLASH_CLASS_BASE + 11) >> #define V4L2_CID_FLASH_READY (V4L2_CID_FLASH_CLASS_BASE + 12) >> >> +/* Image source controls */ >> +#define V4L2_CID_IMAGE_SOURCE_CLASS_BASE (V4L2_CTRL_CLASS_IMAGE_SOURCE | >> 0x900) +#define V4L2_CID_IMAGE_SOURCE_CLASS > (V4L2_CTRL_CLASS_IMAGE_SOURCE >> | 1) + >> +#define V4L2_CID_IMAGE_SOURCE_VBLANK (V4L2_CID_IMAGE_SOURCE_CLASS_BASE > + >> 1) +#define >> V4L2_CID_IMAGE_SOURCE_HBLANK (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 2) >> +#define >> V4L2_CID_IMAGE_SOURCE_LINK_FREQ (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 3) >> +#define >> V4L2_CID_IMAGE_SOURCE_ANALOGUE_GAIN (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 4) >> + >> /* >> * T U N I N G >> */ > -- Sakari Ailus sakari.ailus@xxxxxxxxxxxxxxxxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
