V4L2 subdev and sensor control changes, SMIA++ driver and N9 camera board code

PATCH Superseded Sakari Ailus 2 participants 50 messages Jan 11, 2012 – Jan 19, 2012

Subsystems (6)

Documentation Arm (32-bit) Media (V4L/DVB) Arm sub-architectures +2 more subsystems

lore

Share & tools

lore

Sakari Ailus

author

2012-01-11 21:26 UTC #0 lore raw

cover V4L2 subdev and sensor control changes, SMIA++ driver and N9 camera board code

Hi everyone,

This the second version of my patchset that contains:

- Integer menu controls [2],
- Selection IOCTL for subdevs [3],
- Sensor control changes [5,7],
- link_validate() media entity and V4L2 subdev pad ops,
- OMAP 3 ISP driver improvements [4],
- SMIA++ sensor driver and
- rm680/rm696 board code (a.k.a Nokia N9 and N950)

More detailed information and discussion can be found in the references. The RFC version of the patchset can be found in [6] for more recent discussion. I want to thank all the reviewers of the previous patchset up to now; especially Sylwester Nawrocki and Laurent Pinchart.

Comments and questions are very, very welcome.

I have made changes based on the comments I've gotten so far, the details can be found below. There are a few minor things to be changed in the selection API which I haven't done yet, and this is why:

I have one question left. Will we go forward with the subdev selection API, or shall a more generic properties API cover that functionality, as well as the functionality of current controls API?

Changes to the RFC v1 include:

- Integer controls:
- Target Linux 3.4 instead of 3.3
- Proper control type check in querymenu
- vivi compile fixes

- Subdev selections
- Pad try fields combined to single struct
- Correctly set sel.which based on crop->which in crop fall-back

- Subdev selection documentation
- Better explanation on image processing in subdevs
- Added a diagram to visualise subdev configuration
- Fixed DocBook syntax issues
- Mark VIDIOC_SUBDEV_S_CROP and VIDIOC_SUBDEV_G_CROP obsolete

- Pixel rate
- Pixel rate is now a 64-bit control, not part of v4l2_mbus_framefmt
- Unit for pixel rate is pixels / second
- Pixel rate is read-only

- Link frequency is now in Hz --- documented as such also

- Link validation instead of pipeline validation
- Each link is validated by calling link_validate op
- Added link validation op to media_entity_ops
- Link validation op in pad ops makes this easy for subdev drivers
- media_entity_pipeline_start() may return an error code now
- This might affect other drivers, but will warn in compilation. No adverse effects are caused if the driver does not use link_validate().

- OMAP 3 ISP
- Make lanecfg as part of the platform data structure, not pointer
- Document lane configuration structures
- Link validation moved to respective subdev drivers from ispvideo.c
- isp_validate_pipeline() removed

- SMIA++ driver
- Update pixel order based on vflip and hflip
- Cleanups in the main driver, register definitions and PLL code
- Depend on V4L2_V4L2_SUBDEV_API and MEDIA_CONTROLLER
- Use pr_* macros instead of printk
- Improved error handling for i2c_transfer()
- Removed useless definitions
- Don't access try crop / compose directly but use helper functions
- Add xshutdown to platform data
- Move driver under smiapp directory

- rm680 board code
- Use REGULATOR_SUPPLY() where possible
- Removed printk()'s
- Don't include private smiapp headers

References:

[1] http://www.spinics.net/lists/linux-omap/msg61295.html

[2] http://www.spinics.net/lists/linux-media/msg40796.html

[3] http://www.spinics.net/lists/linux-media/msg41503.html

[4] http://www.spinics.net/lists/linux-media/msg41542.html

[5] http://www.spinics.net/lists/linux-media/msg40861.html

[6] http://www.spinics.net/lists/linux-media/msg41765.html

[7] http://www.spinics.net/lists/linux-media/msg42848.html

Kind regards,

Signature · 2 lines

Sakari Ailus
[email protected]

Sakari Ailus

author

2012-01-11 21:27 UTC #1 lore raw

PATCH 1/23 v4l: Introduce integer menu controls

Create a new control type called V4L2_CTRL_TYPE_INTEGER_MENU. Integer menu controls are just like menu controls but the menu items are 64-bit integers rather than strings.

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/video/v4l2-ctrls.c |   73 +++++++++++++++++++++++++++++---------
 include/linux/videodev2.h        |    6 +++-
 include/media/v4l2-ctrls.h       |    6 +++-
 3 files changed, 66 insertions(+), 19 deletions(-)

drivers/media/video/v4l2-ctrls.c +56 ~~-17~~

index 0f415da..605d4dd 100644
@@ -804,7 +804,8 @@ static void fill_event(struct v4l2_event *ev, struct v4l2_ctrl *ctrl, u32 change
 		ev->u.ctrl.value64 = ctrl->cur.val64;
 	ev->u.ctrl.minimum = ctrl->minimum;
 	ev->u.ctrl.maximum = ctrl->maximum;
-	if (ctrl->type == V4L2_CTRL_TYPE_MENU)
+	if (ctrl->type == V4L2_CTRL_TYPE_MENU
+	    || ctrl->type == V4L2_CTRL_TYPE_INTEGER_MENU)
 		ev->u.ctrl.step = 1;
 	else
 		ev->u.ctrl.step = ctrl->step;

@@ -1035,10 +1036,13 @@ static int validate_new_int(const struct v4l2_ctrl *ctrl, s32 *pval)
 		return 0;
 
 	case V4L2_CTRL_TYPE_MENU:
+	case V4L2_CTRL_TYPE_INTEGER_MENU:
 		if (val < ctrl->minimum || val > ctrl->maximum)
 			return -ERANGE;
-		if (ctrl->qmenu[val][0] == '\0' ||
-		    (ctrl->menu_skip_mask & (1 << val)))
+		if (ctrl->menu_skip_mask & (1 << val))
+			return -EINVAL;
+		if (ctrl->type == V4L2_CTRL_TYPE_MENU &&
+		    ctrl->qmenu[val][0] == '\0')
 			return -EINVAL;
 		return 0;

@@ -1295,7 +1299,8 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 			const struct v4l2_ctrl_ops *ops,
 			u32 id, const char *name, enum v4l2_ctrl_type type,
 			s32 min, s32 max, u32 step, s32 def,
-			u32 flags, const char * const *qmenu, void *priv)
+			u32 flags, const char * const *qmenu,
+			const s64 *qmenu_int, void *priv)
 {
 	struct v4l2_ctrl *ctrl;
 	unsigned sz_extra = 0;

@@ -1308,6 +1313,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 	    (type == V4L2_CTRL_TYPE_INTEGER && step == 0) ||
 	    (type == V4L2_CTRL_TYPE_BITMASK && max == 0) ||
 	    (type == V4L2_CTRL_TYPE_MENU && qmenu == NULL) ||
+	    (type == V4L2_CTRL_TYPE_INTEGER_MENU && qmenu_int == NULL) ||
 	    (type == V4L2_CTRL_TYPE_STRING && max == 0)) {
 		handler_set_err(hdl, -ERANGE);
 		return NULL;

@@ -1318,6 +1324,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 	}
 	if ((type == V4L2_CTRL_TYPE_INTEGER ||
 	     type == V4L2_CTRL_TYPE_MENU ||
+	     type == V4L2_CTRL_TYPE_INTEGER_MENU ||
 	     type == V4L2_CTRL_TYPE_BOOLEAN) &&
 	    (def < min || def > max)) {
 		handler_set_err(hdl, -ERANGE);

@@ -1352,7 +1359,10 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 	ctrl->minimum = min;
 	ctrl->maximum = max;
 	ctrl->step = step;
-	ctrl->qmenu = qmenu;
+	if (type == V4L2_CTRL_TYPE_MENU)
+		ctrl->qmenu = qmenu;
+	else if (type == V4L2_CTRL_TYPE_INTEGER_MENU)
+		ctrl->qmenu_int = qmenu_int;
 	ctrl->priv = priv;
 	ctrl->cur.val = ctrl->val = ctrl->default_value = def;

@@ -1379,6 +1389,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl,
 	struct v4l2_ctrl *ctrl;
 	const char *name = cfg->name;
 	const char * const *qmenu = cfg->qmenu;
+	const s64 *qmenu_int = cfg->qmenu_int;
 	enum v4l2_ctrl_type type = cfg->type;
 	u32 flags = cfg->flags;
 	s32 min = cfg->min;

@@ -1390,18 +1401,24 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl,
 		v4l2_ctrl_fill(cfg->id, &name, &type, &min, &max, &step,
 								&def, &flags);
 
-	is_menu = (cfg->type == V4L2_CTRL_TYPE_MENU);
+	is_menu = (cfg->type == V4L2_CTRL_TYPE_MENU ||
+		   cfg->type == V4L2_CTRL_TYPE_INTEGER_MENU);
 	if (is_menu)
 		WARN_ON(step);
 	else
 		WARN_ON(cfg->menu_skip_mask);
-	if (is_menu && qmenu == NULL)
+	if (cfg->type == V4L2_CTRL_TYPE_MENU && qmenu == NULL)
 		qmenu = v4l2_ctrl_get_menu(cfg->id);
+	else if (cfg->type == V4L2_CTRL_TYPE_INTEGER_MENU &&
+		 qmenu_int == NULL) {
+		handler_set_err(hdl, -EINVAL);
+		return NULL;
+	}
 
 	ctrl = v4l2_ctrl_new(hdl, cfg->ops, cfg->id, name,
 			type, min, max,
 			is_menu ? cfg->menu_skip_mask : step,
-			def, flags, qmenu, priv);
+			def, flags, qmenu, qmenu_int, priv);
 	if (ctrl)
 		ctrl->is_private = cfg->is_private;
 	return ctrl;

@@ -1418,12 +1435,13 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl,
 	u32 flags;
 
 	v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags);
-	if (type == V4L2_CTRL_TYPE_MENU) {
+	if (type == V4L2_CTRL_TYPE_MENU
+	    || type == V4L2_CTRL_TYPE_INTEGER_MENU) {
 		handler_set_err(hdl, -EINVAL);
 		return NULL;
 	}
 	return v4l2_ctrl_new(hdl, ops, id, name, type,
-				    min, max, step, def, flags, NULL, NULL);
+			     min, max, step, def, flags, NULL, NULL, NULL);
 }
 EXPORT_SYMBOL(v4l2_ctrl_new_std);

@@ -1445,7 +1463,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl,
 		return NULL;
 	}
 	return v4l2_ctrl_new(hdl, ops, id, name, type,
-				    0, max, mask, def, flags, qmenu, NULL);
+			     0, max, mask, def, flags, qmenu, NULL, NULL);
 }
 EXPORT_SYMBOL(v4l2_ctrl_new_std_menu);

@@ -1609,6 +1627,9 @@ static void log_ctrl(const struct v4l2_ctrl *ctrl,
 	case V4L2_CTRL_TYPE_MENU:
 		printk(KERN_CONT "%s", ctrl->qmenu[ctrl->cur.val]);
 		break;
+	case V4L2_CTRL_TYPE_INTEGER_MENU:
+		printk(KERN_CONT "%lld", ctrl->qmenu_int[ctrl->cur.val]);
+		break;
 	case V4L2_CTRL_TYPE_BITMASK:
 		printk(KERN_CONT "0x%08x", ctrl->cur.val);
 		break;

@@ -1745,7 +1766,8 @@ int v4l2_queryctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_queryctrl *qc)
 	qc->minimum = ctrl->minimum;
 	qc->maximum = ctrl->maximum;
 	qc->default_value = ctrl->default_value;
-	if (ctrl->type == V4L2_CTRL_TYPE_MENU)
+	if (ctrl->type == V4L2_CTRL_TYPE_MENU
+	    || ctrl->type == V4L2_CTRL_TYPE_INTEGER_MENU)
 		qc->step = 1;
 	else
 		qc->step = ctrl->step;

@@ -1775,16 +1797,33 @@ int v4l2_querymenu(struct v4l2_ctrl_handler *hdl, struct v4l2_querymenu *qm)
 
 	qm->reserved = 0;
 	/* Sanity checks */
-	if (ctrl->qmenu == NULL ||
-	    i < ctrl->minimum || i > ctrl->maximum)
+	switch (ctrl->type) {
+	case V4L2_CTRL_TYPE_MENU:
+		if (ctrl->qmenu == NULL)
+			return -EINVAL;
+		break;
+	case V4L2_CTRL_TYPE_INTEGER_MENU:
+		if (ctrl->qmenu_int == NULL)
+			return -EINVAL;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (i < ctrl->minimum || i > ctrl->maximum)
 		return -EINVAL;
+
 	/* Use mask to see if this menu item should be skipped */
 	if (ctrl->menu_skip_mask & (1 << i))
 		return -EINVAL;
 	/* Empty menu items should also be skipped */
-	if (ctrl->qmenu[i] == NULL || ctrl->qmenu[i][0] == '\0')
-		return -EINVAL;
-	strlcpy(qm->name, ctrl->qmenu[i], sizeof(qm->name));
+	if (ctrl->type == V4L2_CTRL_TYPE_MENU) {
+		if (ctrl->qmenu[i] == NULL || ctrl->qmenu[i][0] == '\0')
+			return -EINVAL;
+		strlcpy(qm->name, ctrl->qmenu[i], sizeof(qm->name));
+	} else {
+		qm->value = ctrl->qmenu_int[i];
+	}
 	return 0;
 }
 EXPORT_SYMBOL(v4l2_querymenu);

include/linux/videodev2.h +5 -1

index 4b752d5..9633c69 100644
@@ -1094,6 +1094,7 @@ enum v4l2_ctrl_type {
 	V4L2_CTRL_TYPE_CTRL_CLASS    = 6,
 	V4L2_CTRL_TYPE_STRING        = 7,
 	V4L2_CTRL_TYPE_BITMASK       = 8,
+	V4L2_CTRL_TYPE_INTEGER_MENU = 9,
 };
 
 /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */

@@ -1113,7 +1114,10 @@ struct v4l2_queryctrl {
 struct v4l2_querymenu {
 	__u32		id;
 	__u32		index;
-	__u8		name[32];	/* Whatever */
+	union {
+		__u8	name[32];	/* Whatever */
+		__s64	value;
+	};
 	__u32		reserved;
 };

include/media/v4l2-ctrls.h +5 -1

index eeb3df6..f7819e7 100644
@@ -129,7 +129,10 @@ struct v4l2_ctrl {
 		u32 step;
 		u32 menu_skip_mask;
 	};
-	const char * const *qmenu;
+	union {
+		const char * const *qmenu;
+		const s64 *qmenu_int;
+	};
 	unsigned long flags;
 	union {
 		s32 val;

@@ -219,6 +222,7 @@ struct v4l2_ctrl_config {
 	u32 flags;
 	u32 menu_skip_mask;
 	const char * const *qmenu;
+	const s64 *qmenu_int;
 	unsigned int is_private:1;
 };

Signature · 1 line

1.7.2.5

Laurent Pinchart

maintainer Acked-by on v1 1/23

2012-01-16 13:49 UTC #2 lore raw

Re: [PATCH 01/23] v4l: Introduce integer menu controls

Quoted 5 lines · On Wednesday 11 January 2012 22:26:38 Sakari Ailus wrote:

Create a new control type called V4L2_CTRL_TYPE_INTEGER_MENU. Integer menu controls are just like menu controls but the menu items are 64-bit integers rather than strings.
Signed-off-by Sakari Ailus <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

Signature · 3 lines

Regards,

Laurent Pinchart

Sakari Ailus

author

2012-01-11 21:27 UTC #3 lore raw

PATCH 2/23 v4l: Document integer menu controls

In reply to Sakari Ailus

Signed-off-by Sakari Ailus <[email protected]>

---
 Documentation/DocBook/media/v4l/compat.xml         |   10 +++++
 Documentation/DocBook/media/v4l/v4l2.xml           |    7 ++++
 .../DocBook/media/v4l/vidioc-queryctrl.xml         |   39 +++++++++++++++++++-
 3 files changed, 54 insertions(+), 2 deletions(-)

Documentation/DocBook/media/v4l/compat.xml +10 -0

index b68698f..985c536 100644
@@ -2379,6 +2379,16 @@ that used it. It was originally scheduled for removal in 2.6.35.
       </orderedlist>
     </section>
 
+    <section>
+      <title>V4L2 in Linux 3.4</title>
+      <orderedlist>
+        <listitem>
+	  <para>Added integer menus, the new type will be
+	  V4L2_CTRL_TYPE_INTEGER_MENU.</para>
+        </listitem>
+      </orderedlist>
+    </section>
+
     <section id="other">
       <title>Relation of V4L2 to other Linux multimedia APIs</title>

Documentation/DocBook/media/v4l/v4l2.xml +7 -0

index 2ab365c..8646fbc 100644
@@ -128,6 +128,13 @@ structs, ioctls) must be noted in more detail in the history chapter
 applications. -->
 
       <revision>
+	<revnumber>3.4</revnumber>
+	<date>2011-11-24</date>
+	<authorinitials>sa</authorinitials>
+	<revremark>Added V4L2_CTRL_TYPE_INTEGER_MENU.</revremark>
+      </revision>
+
+      <revision>
 	<revnumber>3.2</revnumber>
 	<date>2011-08-26</date>
 	<authorinitials>hv</authorinitials>

Documentation/DocBook/media/v4l/vidioc-queryctrl.xml +37 -2

index 0ac0057..02064b0 100644
@@ -215,11 +215,12 @@ the array to zero.</entry>
 
     <table pgwide="1" frame="none" id="v4l2-querymenu">
       <title>struct <structname>v4l2_querymenu</structname></title>
-      <tgroup cols="3">
+      <tgroup cols="4">
 	&cs-str;
 	<tbody valign="top">
 	  <row>
 	    <entry>__u32</entry>
+	    <entry></entry>
 	    <entry><structfield>id</structfield></entry>
 	    <entry>Identifies the control, set by the application
 from the respective &v4l2-queryctrl;

@@ -227,18 +228,38 @@ from the respective &v4l2-queryctrl;
 	  </row>
 	  <row>
 	    <entry>__u32</entry>
+	    <entry></entry>
 	    <entry><structfield>index</structfield></entry>
 	    <entry>Index of the menu item, starting at zero, set by
 	    the application.</entry>
 	  </row>
 	  <row>
+	    <entry>union</entry>
+	    <entry></entry>
+	    <entry></entry>
+	    <entry></entry>
+	  </row>
+	  <row>
+	    <entry></entry>
 	    <entry>__u8</entry>
 	    <entry><structfield>name</structfield>[32]</entry>
 	    <entry>Name of the menu item, a NUL-terminated ASCII
-string. This information is intended for the user.</entry>
+string. This information is intended for the user. This field is valid
+for <constant>V4L2_CTRL_FLAG_MENU</constant> type controls.</entry>
+	  </row>
+	  <row>
+	    <entry></entry>
+	    <entry>__s64</entry>
+	    <entry><structfield>value</structfield></entry>
+	    <entry>
+              Value of the integer menu item. This field is valid for
+              <constant>V4L2_CTRL_FLAG_INTEGER_MENU</constant> type
+              controls.
+            </entry>
 	  </row>
 	  <row>
 	    <entry>__u32</entry>
+	    <entry></entry>
 	    <entry><structfield>reserved</structfield></entry>
 	    <entry>Reserved for future extensions. Drivers must set
 the array to zero.</entry>

@@ -292,6 +313,20 @@ the menu items can be enumerated with the
 <constant>VIDIOC_QUERYMENU</constant> ioctl.</entry>
 	  </row>
 	  <row>
+	    <entry><constant>V4L2_CTRL_TYPE_INTEGER_MENU</constant></entry>
+	    <entry>&ge; 0</entry>
+	    <entry>1</entry>
+	    <entry>N-1</entry>
+	    <entry>
+              The control has a menu of N choices. The values of the
+              menu items can be enumerated with the
+              <constant>VIDIOC_QUERYMENU</constant> ioctl. This is
+              similar to <constant>V4L2_CTRL_TYPE_MENU</constant>
+              except that instead of strings, the menu items are
+              signed 64-bit integers.
+            </entry>
+	  </row>
+	  <row>
 	    <entry><constant>V4L2_CTRL_TYPE_BITMASK</constant></entry>
 	    <entry>0</entry>
 	    <entry>n/a</entry>

Signature · 1 line

1.7.2.5

Laurent Pinchart

maintainer Acked-by on v1 2/23

2012-01-16 13:50 UTC #4 lore raw

Re: [PATCH 02/23] v4l: Document integer menu controls

Quoted 1 line · On Wednesday 11 January 2012 22:26:39 Sakari Ailus wrote:

Signed-off-by Sakari Ailus <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

Signature · 3 lines

Regards,

Laurent Pinchart

Sakari Ailus

author

2012-01-11 21:27 UTC #5 lore raw

PATCH 3/23 vivi: Add an integer menu test control

In reply to Sakari Ailus

Add an integer menu test control for the vivi driver.

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/video/vivi.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

drivers/media/video/vivi.c +22 -0

index 7d754fb..aab375b 100644
@@ -177,6 +177,7 @@ struct vivi_dev {
 	struct v4l2_ctrl	   *menu;
 	struct v4l2_ctrl	   *string;
 	struct v4l2_ctrl	   *bitmask;
+	struct v4l2_ctrl	   *int_menu;
 
 	spinlock_t                 slock;
 	struct mutex		   mutex;

@@ -503,6 +504,10 @@ static void vivi_fillbuff(struct vivi_dev *dev, struct vivi_buffer *buf)
 			dev->boolean->cur.val,
 			dev->menu->qmenu[dev->menu->cur.val],
 			dev->string->cur.string);
+	snprintf(str, sizeof(str), " integer_menu %lld, value %d ",
+			dev->int_menu->qmenu_int[dev->int_menu->cur.val],
+			dev->int_menu->cur.val);
+	gen_text(dev, vbuf, line++ * 16, 16, str);
 	mutex_unlock(&dev->ctrl_handler.lock);
 	gen_text(dev, vbuf, line++ * 16, 16, str);
 	if (dev->button_pressed) {

@@ -1183,6 +1188,22 @@ static const struct v4l2_ctrl_config vivi_ctrl_bitmask = {
 	.step = 0,
 };
 
+static const s64 vivi_ctrl_int_menu_values[] = {
+	1, 1, 2, 3, 5, 8, 13, 21, 42,
+};
+
+static const struct v4l2_ctrl_config vivi_ctrl_int_menu = {
+	.ops = &vivi_ctrl_ops,
+	.id = VIVI_CID_CUSTOM_BASE + 7,
+	.name = "Integer menu",
+	.type = V4L2_CTRL_TYPE_INTEGER_MENU,
+	.min = 1,
+	.max = 8,
+	.def = 4,
+	.menu_skip_mask = 0x02,
+	.qmenu_int = vivi_ctrl_int_menu_values,
+};
+
 static const struct v4l2_file_operations vivi_fops = {
 	.owner		= THIS_MODULE,
 	.open           = v4l2_fh_open,

@@ -1293,6 +1314,7 @@ static int __init vivi_create_instance(int inst)
 	dev->menu = v4l2_ctrl_new_custom(hdl, &vivi_ctrl_menu, NULL);
 	dev->string = v4l2_ctrl_new_custom(hdl, &vivi_ctrl_string, NULL);
 	dev->bitmask = v4l2_ctrl_new_custom(hdl, &vivi_ctrl_bitmask, NULL);
+	dev->int_menu = v4l2_ctrl_new_custom(hdl, &vivi_ctrl_int_menu, NULL);
 	if (hdl->error) {
 		ret = hdl->error;
 		goto unreg_dev;

Signature · 1 line

1.7.2.5

Laurent Pinchart

maintainer Acked-by on v1 3/23

2012-01-16 13:52 UTC #6 lore raw

Re: [PATCH 03/23] vivi: Add an integer menu test control

Quoted 3 lines · On Wednesday 11 January 2012 22:26:40 Sakari Ailus wrote:

Add an integer menu test control for the vivi driver.
Signed-off-by Sakari Ailus <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

Signature · 3 lines

Regards,

Laurent Pinchart

Sakari Ailus

author

2012-01-11 21:27 UTC #7 lore raw

PATCH 4/23 v4l: VIDIOC_SUBDEV_S_SELECTION and VIDIOC_SUBDEV_G_SELECTION IOCTLs

In reply to Sakari Ailus

Add support for VIDIOC_SUBDEV_S_SELECTION and VIDIOC_SUBDEV_G_SELECTION IOCTLs. They replace functionality provided by VIDIOC_SUBDEV_S_CROP and VIDIOC_SUBDEV_G_CROP IOCTLs and also add new functionality (composing).

VIDIOC_SUBDEV_G_CROP and VIDIOC_SUBDEV_S_CROP continue to be supported.

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/video/v4l2-subdev.c |   34 +++++++++++++++++++--------
 include/linux/v4l2-subdev.h       |   45 +++++++++++++++++++++++++++++++++++++
 include/media/v4l2-subdev.h       |   21 ++++++++++++++---
 3 files changed, 86 insertions(+), 14 deletions(-)

drivers/media/video/v4l2-subdev.c +24 ~~-10~~

index 65ade5f..9de25a6 100644
@@ -35,14 +35,9 @@
 static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
 {
 #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
-	/* Allocate try format and crop in the same memory block */
-	fh->try_fmt = kzalloc((sizeof(*fh->try_fmt) + sizeof(*fh->try_crop))
-			      * sd->entity.num_pads, GFP_KERNEL);
-	if (fh->try_fmt == NULL)
+	fh->pad = kzalloc(sizeof(*fh->pad) * sd->entity.num_pads, GFP_KERNEL);
+	if (fh->pad == NULL)
 		return -ENOMEM;
-
-	fh->try_crop = (struct v4l2_rect *)
-		(fh->try_fmt + sd->entity.num_pads);
 #endif
 	return 0;
 }

@@ -50,9 +45,8 @@ static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
 static void subdev_fh_free(struct v4l2_subdev_fh *fh)
 {
 #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
-	kfree(fh->try_fmt);
-	fh->try_fmt = NULL;
-	fh->try_crop = NULL;
+	kfree(fh->pad);
+	fh->pad = NULL;
 #endif
 }

@@ -281,6 +275,26 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 		return v4l2_subdev_call(sd, pad, enum_frame_interval, subdev_fh,
 					fie);
 	}
+
+	case VIDIOC_SUBDEV_G_SELECTION: {
+		struct v4l2_subdev_selection *sel = arg;
+
+		if (sel->pad >= sd->entity.num_pads)
+			return -EINVAL;
+
+		return v4l2_subdev_call(
+			sd, pad, get_selection, subdev_fh, sel);
+	}
+
+	case VIDIOC_SUBDEV_S_SELECTION: {
+		struct v4l2_subdev_selection *sel = arg;
+
+		if (sel->pad >= sd->entity.num_pads)
+			return -EINVAL;
+
+		return v4l2_subdev_call(
+			sd, pad, set_selection, subdev_fh, sel);
+	}
 #endif
 	default:
 		return v4l2_subdev_call(sd, core, ioctl, cmd, arg);

include/linux/v4l2-subdev.h +45 -0

56 lines not on this page · raw message

include/media/v4l2-subdev.h +17 -4

index f0f3358..feab950 100644
@@ -466,6 +466,10 @@ struct v4l2_subdev_pad_ops {
 		       struct v4l2_subdev_crop *crop);
 	int (*get_crop)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
 		       struct v4l2_subdev_crop *crop);
+	int (*get_selection)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
+			     struct v4l2_subdev_selection *sel);
+	int (*set_selection)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
+			     struct v4l2_subdev_selection *sel);
 };
 
 struct v4l2_subdev_ops {

@@ -549,8 +553,11 @@ struct v4l2_subdev {
 struct v4l2_subdev_fh {
 	struct v4l2_fh vfh;
 #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
-	struct v4l2_mbus_framefmt *try_fmt;
-	struct v4l2_rect *try_crop;
+	struct {
+		struct v4l2_mbus_framefmt try_fmt;
+		struct v4l2_rect try_crop;
+		struct v4l2_rect try_compose;
+	} *pad;
 #endif
 };

@@ -561,13 +568,19 @@ struct v4l2_subdev_fh {
 static inline struct v4l2_mbus_framefmt *
 v4l2_subdev_get_try_format(struct v4l2_subdev_fh *fh, unsigned int pad)
 {
-	return &fh->try_fmt[pad];
+	return &fh->pad[pad].try_fmt;
 }
 
 static inline struct v4l2_rect *
 v4l2_subdev_get_try_crop(struct v4l2_subdev_fh *fh, unsigned int pad)
 {
-	return &fh->try_crop[pad];
+	return &fh->pad[pad].try_crop;
+}
+
+static inline struct v4l2_rect *
+v4l2_subdev_get_try_compose(struct v4l2_subdev_fh *fh, unsigned int pad)
+{
+	return &fh->pad[pad].try_compose;
 }
 #endif

Signature · 1 line

1.7.2.5

Sakari Ailus

author

2012-01-11 21:27 UTC #8 lore raw

PATCH 5/23 v4l: Support s_crop and g_crop through s/g_selection

In reply to Sakari Ailus

Fall back to s_selection if s_crop isn't implemented by a driver. Same for g_selection / g_crop.

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/video/v4l2-subdev.c |   37 +++++++++++++++++++++++++++++++++++--
 1 files changed, 35 insertions(+), 2 deletions(-)

drivers/media/video/v4l2-subdev.c +35 -2

index 9de25a6..836270d 100644
@@ -216,6 +216,8 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 
 	case VIDIOC_SUBDEV_G_CROP: {
 		struct v4l2_subdev_crop *crop = arg;
+		struct v4l2_subdev_selection sel;
+		int rval;
 
 		if (crop->which != V4L2_SUBDEV_FORMAT_TRY &&
 		    crop->which != V4L2_SUBDEV_FORMAT_ACTIVE)

@@ -224,11 +226,27 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 		if (crop->pad >= sd->entity.num_pads)
 			return -EINVAL;
 
-		return v4l2_subdev_call(sd, pad, get_crop, subdev_fh, crop);
+		rval = v4l2_subdev_call(sd, pad, get_crop, subdev_fh, crop);
+		if (rval != -ENOIOCTLCMD)
+			return rval;
+
+		memset(&sel, 0, sizeof(sel));
+		sel.which = crop->which;
+		sel.pad = crop->pad;
+		sel.target = V4L2_SUBDEV_SEL_TGT_CROP_ACTIVE;
+
+		rval = v4l2_subdev_call(
+			sd, pad, get_selection, subdev_fh, &sel);
+
+		crop->rect = sel.r;
+
+		return rval;
 	}
 
 	case VIDIOC_SUBDEV_S_CROP: {
 		struct v4l2_subdev_crop *crop = arg;
+		struct v4l2_subdev_selection sel;
+		int rval;
 
 		if (crop->which != V4L2_SUBDEV_FORMAT_TRY &&
 		    crop->which != V4L2_SUBDEV_FORMAT_ACTIVE)

@@ -237,7 +255,22 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 		if (crop->pad >= sd->entity.num_pads)
 			return -EINVAL;
 
-		return v4l2_subdev_call(sd, pad, set_crop, subdev_fh, crop);
+		rval = v4l2_subdev_call(sd, pad, set_crop, subdev_fh, crop);
+		if (rval != -ENOIOCTLCMD)
+			return rval;
+
+		memset(&sel, 0, sizeof(sel));
+		sel.which = crop->which;
+		sel.pad = crop->pad;
+		sel.target = V4L2_SUBDEV_SEL_TGT_CROP_ACTIVE;
+		sel.r = crop->rect;
+
+		rval = v4l2_subdev_call(
+			sd, pad, set_selection, subdev_fh, &sel);
+
+		crop->rect = sel.r;
+
+		return rval;
 	}
 
 	case VIDIOC_SUBDEV_ENUM_MBUS_CODE: {

Signature · 1 line

1.7.2.5

Laurent Pinchart

maintainer Acked-by on v1 5/23

2012-01-16 13:54 UTC #9 lore raw

Re: [PATCH 05/23] v4l: Support s_crop and g_crop through s/g_selection

Quoted 4 lines · On Wednesday 11 January 2012 22:26:42 Sakari Ailus wrote:

Fall back to s_selection if s_crop isn't implemented by a driver. Same for g_selection / g_crop.
Signed-off-by Sakari Ailus <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

Signature · 3 lines

Regards,

Laurent Pinchart

Sakari Ailus

author

2012-01-11 21:27 UTC #10 lore raw

PATCH 6/23 v4l: Add selections documentation

In reply to Sakari Ailus

Signed-off-by Sakari Ailus <[email protected]>

---
 .../DocBook/media/subdev-image-processing.svg      |  116 ++
 Documentation/DocBook/media/v4l/dev-subdev.xml     |  116 ++-
 .../DocBook/media/v4l/subdev-image-processing.dia  | 1137 ++++++++++++++++++++
 Documentation/DocBook/media/v4l/v4l2.xml           |    1 +
 .../media/v4l/vidioc-subdev-g-selection.xml        |  227 ++++
 5 files changed, 1573 insertions(+), 24 deletions(-)
 create mode 100644 Documentation/DocBook/media/subdev-image-processing.svg
 create mode 100644 Documentation/DocBook/media/v4l/subdev-image-processing.dia
 create mode 100644 Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml

Documentation/DocBook/media/subdev-image-processing.svg new file +116 -0

new file mode 100644
index 0000000..8e24d7e
@@ -0,0 +1,116 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>
+<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.0//EN" "http://www.w3.org/TR/2001/PR-SVG-20010719/DTD/svg10.dtd">
+<svg width="49cm" height="18cm" viewBox="3 137 977 346" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
+  <g>
+    <rect style="fill: #ffffff" x="294" y="400" width="342.5" height="82"/>
+    <rect style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #000000" x="294" y="400" width="342.5" height="82"/>
+  </g>
+  <text style="fill: #000000;text-anchor:start;font-size:12.8;font-family:sanserif;font-style:normal;font-weight:normal" x="442" y="446">
+    <tspan x="442" y="446">Subdev</tspan>
+  </text>
+  <g>
+    <ellipse style="fill: #ffffff" cx="284.5" cy="439.712" rx="8.5" ry="8.5"/>
+    <ellipse style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #000000" cx="284.5" cy="439.712" rx="8.5" ry="8.5"/>
+    <ellipse style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #000000" cx="284.5" cy="439.712" rx="8.5" ry="8.5"/>
+  </g>
+  <g>
+    <ellipse style="fill: #ffffff" cx="646.232" cy="440.184" rx="8.5" ry="8.5"/>
+    <ellipse style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #000000" cx="646.232" cy="440.184" rx="8.5" ry="8.5"/>
+    <ellipse style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #000000" cx="646.232" cy="440.184" rx="8.5" ry="8.5"/>
+  </g>
+  <g>
+    <line style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #000000" x1="112.5" y1="440" x2="266.264" y2="439.729"/>
+    <polygon style="fill: #000000" points="273.764,439.716 263.773,444.734 266.264,439.729 263.755,434.734 "/>
+    <polygon style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #000000" points="273.764,439.716 263.773,444.734 266.264,439.729 263.755,434.734 "/>
+  </g>
+  <g>
+    <line style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #000000" x1="654.732" y1="440.184" x2="811.066" y2="440.212"/>
+    <polygon style="fill: #000000" points="818.566,440.214 808.565,445.212 811.066,440.212 808.567,435.212 "/>
+    <polygon style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #000000" points="818.566,440.214 808.565,445.212 811.066,440.212 808.567,435.212 "/>
+  </g>
+  <text style="fill: #000000;text-anchor:start;font-size:12.8;font-family:sanserif;font-style:normal;font-weight:normal" x="263.5" y="409">
+    <tspan x="263.5" y="409">sink</tspan>
+    <tspan x="263.5" y="425">pad</tspan>
+  </text>
+  <text style="fill: #000000;text-anchor:start;font-size:12.8;font-family:sanserif;font-style:normal;font-weight:normal" x="643.5" y="407">
+    <tspan x="643.5" y="407">source</tspan>
+    <tspan x="643.5" y="423">pad</tspan>
+  </text>
+  <g>
+    <rect style="fill: #ffffff" x="4.5" y="189" width="159" height="104"/>
+    <rect style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #a52a2a" x="4.5" y="189" width="159" height="104"/>
+  </g>
+  <g>
+    <rect style="fill: #ffffff" x="213.5" y="209" width="94" height="77"/>
+    <rect style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #0000ff" x="213.5" y="209" width="94" height="77"/>
+  </g>
+  <g>
+    <rect style="fill: #ffffff" x="58.3" y="195" width="94" height="77"/>
+    <rect style="fill: none; fill-opacity:0; stroke-width: 2; stroke-dasharray: 4; stroke: #0000ff" x="58.3" y="195" width="94" height="77"/>
+  </g>
+  <line style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #e60505" x1="213.5" y1="286" x2="58.3" y2="272"/>
+  <line style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #e60505" x1="213.5" y1="209" x2="58.3" y2="195"/>
+  <line style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #e60505" x1="307.5" y1="286" x2="152.3" y2="272"/>
+  <line style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #e60505" x1="307.5" y1="209" x2="152.3" y2="195"/>
+  <text style="fill: #0000ff;text-anchor:start;font-size:12.8;font-family:sanserif;font-style:normal;font-weight:normal" x="212.5" y="153">
+    <tspan x="212.5" y="153">sink</tspan>
+    <tspan x="212.5" y="169">crop</tspan>
+    <tspan x="212.5" y="185">selection</tspan>
+  </text>
+  <text style="fill: #000000;text-anchor:start;font-size:12.8;font-family:sanserif;font-style:normal;font-weight:normal" x="29.5" y="158">
+    <tspan x="29.5" y="158"></tspan>
+  </text>
+  <text style="fill: #a52a2a;text-anchor:start;font-size:12.8;font-family:sanserif;font-style:normal;font-weight:normal" x="8.53836" y="157.914">
+    <tspan x="8.53836" y="157.914">sink media</tspan>
+    <tspan x="8.53836" y="173.914">bus format</tspan>
+  </text>
+  <g>
+    <rect style="fill: #ffffff" x="333.644" y="185.65" width="165.2" height="172.478"/>
+    <rect style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #00ff00" x="333.644" y="185.65" width="165.2" height="172.478"/>
+  </g>
+  <line style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #e60505" x1="333.644" y1="358.128" x2="213.5" y2="286"/>
+  <line style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #e60505" x1="333.644" y1="185.65" x2="213.5" y2="209"/>
+  <line style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #e60505" x1="498.844" y1="358.128" x2="307.5" y2="286"/>
+  <line style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #e60505" x1="498.844" y1="185.65" x2="307.5" y2="209"/>
+  <text style="fill: #00ff00;text-anchor:start;font-size:12.8;font-family:sanserif;font-style:normal;font-weight:normal" x="334.704" y="149.442">
+    <tspan x="334.704" y="149.442">sink compose</tspan>
+    <tspan x="334.704" y="165.442">selection (scaling)</tspan>
+  </text>
+  <g>
+    <rect style="fill: #ffffff" x="543.322" y="187.565" width="100.186" height="71.4523"/>
+    <rect style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #a020f0" x="543.322" y="187.565" width="100.186" height="71.4523"/>
+  </g>
+  <g>
+    <rect style="fill: #ffffff" x="386.64" y="225.1" width="100.186" height="71.4523"/>
+    <rect style="fill: none; fill-opacity:0; stroke-width: 2; stroke-dasharray: 4; stroke: #a020f0" x="386.64" y="225.1" width="100.186" height="71.4523"/>
+  </g>
+  <line style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #e60505" x1="543.322" y1="259.018" x2="386.64" y2="296.552"/>
+  <line style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #e60505" x1="543.322" y1="187.565" x2="386.64" y2="225.1"/>
+  <line style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #e60505" x1="643.508" y1="259.018" x2="486.826" y2="296.552"/>
+  <line style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #e60505" x1="643.508" y1="187.565" x2="486.826" y2="225.1"/>
+  <text style="fill: #a020f0;text-anchor:start;font-size:12.8;font-family:sanserif;font-style:normal;font-weight:normal" x="543.322" y="149.442">
+    <tspan x="543.322" y="149.442">source</tspan>
+    <tspan x="543.322" y="165.442">crop</tspan>
+    <tspan x="543.322" y="181.442">selection</tspan>
+  </text>
+  <g>
+    <rect style="fill: #ffffff" x="685.6" y="182.628" width="292.9" height="200.5"/>
+    <rect style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #8b6914" x="685.6" y="182.628" width="292.9" height="200.5"/>
+  </g>
+  <text style="fill: #8b6914;text-anchor:start;font-size:12.8;font-family:sanserif;font-style:normal;font-weight:normal" x="691.5" y="157.128">
+    <tspan x="691.5" y="157.128">source media</tspan>
+    <tspan x="691.5" y="173.128">bus format</tspan>
+  </text>
+  <g>
+    <rect style="fill: #ffffff" x="780.488" y="268.834" width="100.186" height="71.4523"/>
+    <rect style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #ffa500" x="780.488" y="268.834" width="100.186" height="71.4523"/>
+  </g>
+  <line style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #e60505" x1="780.488" y1="340.286" x2="543.322" y2="259.018"/>
+  <line style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #e60505" x1="780.488" y1="268.834" x2="543.322" y2="187.565"/>
+  <line style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #e60505" x1="880.674" y1="340.286" x2="643.508" y2="259.018"/>
+  <line style="fill: none; fill-opacity:0; stroke-width: 2; stroke: #e60505" x1="880.674" y1="268.834" x2="643.508" y2="187.565"/>
+  <text style="fill: #ffa500;text-anchor:start;font-size:12.8;font-family:sanserif;font-style:normal;font-weight:normal" x="800" y="220">
+    <tspan x="800" y="220">source compose</tspan>
+    <tspan x="800" y="236">selection (composition)</tspan>
+  </text>
+</svg>

Documentation/DocBook/media/v4l/dev-subdev.xml +92 ~~-24~~

140 lines not on this page · raw message

Documentation/DocBook/media/v4l/subdev-image-processing.dia new file +1137 -0

1,137 lines not on this page · raw message

Documentation/DocBook/media/v4l/v4l2.xml +1 -0

index 8646fbc..79a0731 100644
@@ -529,6 +529,7 @@ and discussions on the V4L mailing list.</revremark>
     &sub-subdev-g-crop;
     &sub-subdev-g-fmt;
     &sub-subdev-g-frame-interval;
+    &sub-subdev-g-selection;
     &sub-subscribe-event;
     <!-- End of ioctls. -->
     &sub-mmap;

Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml new file +227 -0

227 lines not on this page · raw message

Signature · 1 line

1.7.2.5

Sakari Ailus

author

2012-01-11 21:27 UTC #11 lore raw

PATCH 7/23 v4l: Mark VIDIOC_SUBDEV_G_CROP and VIDIOC_SUBDEV_S_CROP obsolete

In reply to Sakari Ailus

These two IOCTLS are obsoleted by VIDIOC_SUBDEV_G_SELECTION and VIDIOC_SUBDEV_S_SELECTION. Mark them obsolete.

Signed-off-by Sakari Ailus <[email protected]>

---
 Documentation/DocBook/media/v4l/compat.xml         |    7 +++++++
 .../DocBook/media/v4l/vidioc-subdev-g-crop.xml     |    9 ++++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

Documentation/DocBook/media/v4l/compat.xml +7 -0

index 985c536..a5be8a0 100644
@@ -2514,6 +2514,13 @@ interfaces and should not be implemented in new drivers.</para>
 <constant>VIDIOC_S_MPEGCOMP</constant> ioctls. Use Extended Controls,
 <xref linkend="extended-controls" />.</para>
         </listitem>
+        <listitem>
+	  <para><constant>VIDIOC_SUBDEV_G_CROP</constant> and
+	  <constant>VIDIOC_SUBDEV_S_CROP</constant> ioctls. Use
+	  <constant>VIDIOC_SUBDEV_G_SELECTION</constant> and
+	  <constant>VIDIOC_SUBDEV_S_SELECTION</constant>, <xref
+	  linkend="vidioc-subdev-g-selection" />.</para>
+        </listitem>
       </itemizedlist>
     </section>
   </section>

Documentation/DocBook/media/v4l/vidioc-subdev-g-crop.xml +6 -3

index 0619732..4cddd78 100644
@@ -58,9 +58,12 @@
     <title>Description</title>
 
     <note>
-      <title>Experimental</title>
-      <para>This is an <link linkend="experimental">experimental</link>
-      interface and may change in the future.</para>
+      <title>Obsolete</title>
+
+      <para>This is an <link linkend="obsolete">obsolete</link>
+      interface and may be removed in the future. It is superseded by
+      <link linkend="vidioc-subdev-g-selection">the selection
+      API</link>.</para>
     </note>
 
     <para>To retrieve the current crop rectangle applications set the

Signature · 1 line

1.7.2.5

Sakari Ailus

author

2012-01-11 21:27 UTC #12 lore raw

PATCH 8/23 v4l: Image source control class

In reply to Sakari Ailus

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 <[email protected]>

---
 Documentation/DocBook/media/v4l/controls.xml       |  112 ++++++++++++++++++++
 .../DocBook/media/v4l/vidioc-g-ext-ctrls.xml       |    6 +
 drivers/media/video/v4l2-ctrls.c                   |   15 +++
 include/linux/videodev2.h                          |   11 ++
 4 files changed, 144 insertions(+), 0 deletions(-)

Documentation/DocBook/media/v4l/controls.xml +112 -0

index 3bc5ee8..467ace3 100644
@@ -3356,6 +3356,118 @@ 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
+	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
+	    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
+	    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.
+	    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. The unit of this control
+	    is Hz.
+	    </entry>
+	  </row>
+	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_ANALOGUE_GAIN</constant></entry>
+	    <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.
+	    </entry>
+	  </row>
+	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_PIXEL_RATE</constant></entry>
+	    <entry>64-bit integer</entry>
+	  </row>
+	  <row>
+	    <entry spanname="descr">Pixel rate in the source pads of
+	    the subdev. This control is read-only and its unit is
+	    pixels / second.
+	    </entry>
+	  </row>
+	  <row><entry></entry></row>
+	</tbody>
+      </tgroup>
+      </table>
+
+    </section>
+
 </section>
 
   <!--

Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml +6 -0

index 5122ce8..250c1cf 100644
@@ -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>

drivers/media/video/v4l2-ctrls.c +15 -0

index 605d4dd..51b4559 100644
@@ -606,6 +606,13 @@ 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";
+	case V4L2_CID_IMAGE_SOURCE_PIXEL_RATE:	return "Pixel rate";
+
 	default:
 		return NULL;
 	}

@@ -694,6 +701,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;
 	case V4L2_CID_RDS_TX_PS_NAME:
 	case V4L2_CID_RDS_TX_RADIO_TEXT:
 		*type = V4L2_CTRL_TYPE_STRING;

@@ -703,6 +713,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;

@@ -723,6 +734,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 		*type = V4L2_CTRL_TYPE_INTEGER;
 		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
 		break;
+	case V4L2_CID_IMAGE_SOURCE_PIXEL_RATE:
+		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
+		*type = V4L2_CTRL_TYPE_INTEGER64;
+		break;
 	default:
 		*type = V4L2_CTRL_TYPE_INTEGER;
 		break;

include/linux/videodev2.h +11 -0

index 9633c69..c9d07c7 100644
@@ -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,16 @@ 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)
+#define V4L2_CID_IMAGE_SOURCE_PIXEL_RATE	(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 5)
+
 /*
  *	T U N I N G
  */

Signature · 1 line

1.7.2.5

Laurent Pinchart

maintainer

2012-01-16 13:57 UTC #13 lore raw

Re: [PATCH 08/23] v4l: Image source control class

Hi Sakari,

Thanks for the patch.

Quoted 98 lines · On Wednesday 11 January 2012 22:26:45 Sakari Ailus wrote:

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 <[email protected]>

---
 Documentation/DocBook/media/v4l/controls.xml       |  112
++++++++++++++++++++ .../DocBook/media/v4l/vidioc-g-ext-ctrls.xml       | 
  6 +
 drivers/media/video/v4l2-ctrls.c                   |   15 +++
 include/linux/videodev2.h                          |   11 ++
 4 files changed, 144 insertions(+), 0 deletions(-)

(quoted diff) +0 -0

b/Documentation/DocBook/media/v4l/controls.xml index 3bc5ee8..467ace3 100644

Documentation/DocBook/media/v4l/controls.xml +34 -0

@@ -3356,6 +3356,118 @@ 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
+	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>

Documentation/DocBook/media/v4l/controls.xml +5 -0

+	</thead>
+	<tbody valign="top">
+	  <row><entry></entry></row>
+	  <row>
+	    <entry

spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_CLASS</constant></entry> +
<entry>class</entry>

Documentation/DocBook/media/v4l/controls.xml +6 -0

+	  </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>

Documentation/DocBook/media/v4l/controls.xml +12 -0

+	  </row>
+	  <row>
+	    <entry spanname="descr">Vertical blanking. The idle
+	    preriod after every frame during which no image data is
+	    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>

Documentation/DocBook/media/v4l/controls.xml +9 -0

+	  </row>
+	  <row>
+	    <entry spanname="descr">Horizontal blanking. The idle
+	    preriod after every line of image data during which no
+	    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>

Documentation/DocBook/media/v4l/controls.xml +2 -0

+	    <entry>integer menu</entry>
+	  </row>

Documentation/DocBook/media/v4l/controls.xml interface and may change in the future.</para> · line 3440

Quoted 10 lines

Documentation/DocBook/media/v4l/controls.xml +10 -0

+	  <row>
+	    <entry spanname="descr">Image source's data bus frequency.
+	    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. The unit of this control
+	    is Hz.

I think we're still missing an explanation of how those controls interact with each other, and how they should be used by userspace.

Quoted 119 lines

Documentation/DocBook/media/v4l/controls.xml +4 -0

+	    </entry>
+	  </row>
+	  <row>
+	    <entry

spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_ANALOGUE_GAIN</constant></en
try> + <entry>integer</entry>

Documentation/DocBook/media/v4l/controls.xml +10 -0

+	  </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.
+	    </entry>
+	  </row>
+	  <row>
+	    <entry

spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_PIXEL_RATE</constant></entry

Quoted 1 line

Documentation/DocBook/media/v4l/controls.xml +1 -0
+	    <entry>64-bit integer</entry>

Documentation/DocBook/media/v4l/controls.xml +14 -0

+	  </row>
+	  <row>
+	    <entry spanname="descr">Pixel rate in the source pads of
+	    the subdev. This control is read-only and its unit is
+	    pixels / second.
+	    </entry>
+	  </row>
+	  <row><entry></entry></row>
+	</tbody>
+      </tgroup>
+      </table>
+
+    </section>
+
 </section>
 
   <!--

(quoted diff) +0 -0

b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml index 5122ce8..250c1cf 100644

Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml +6 -0

@@ -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>

(quoted diff) +0 -0

b/drivers/media/video/v4l2-ctrls.c index 605d4dd..51b4559 100644

drivers/media/video/v4l2-ctrls.c +7 -0

@@ -606,6 +606,13 @@ 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";
+	case V4L2_CID_IMAGE_SOURCE_PIXEL_RATE:	return "Pixel rate";
+
 	default:
 		return NULL;
 	}

@@ -694,6 +701,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum

v4l2_ctrl_type *type, case V4L2_CID_MPEG_VIDEO_MPEG4_PROFILE:

drivers/media/video/v4l2-ctrls.c +3 -0

 		*type = V4L2_CTRL_TYPE_MENU;
 		break;
+	case V4L2_CID_IMAGE_SOURCE_LINK_FREQ:
+		*type = V4L2_CTRL_TYPE_INTEGER_MENU;
+		break;
 	case V4L2_CID_RDS_TX_PS_NAME:
 	case V4L2_CID_RDS_TX_RADIO_TEXT:
 		*type = V4L2_CTRL_TYPE_STRING;

@@ -703,6 +713,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum

v4l2_ctrl_type *type, case V4L2_CID_MPEG_CLASS:

drivers/media/video/v4l2-ctrls.c +5 -0

 	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;

@@ -723,6 +734,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum
 v4l2_ctrl_type *type, *type = V4L2_CTRL_TYPE_INTEGER;
 		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
 		break;
+	case V4L2_CID_IMAGE_SOURCE_PIXEL_RATE:
+		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
+		*type = V4L2_CTRL_TYPE_INTEGER64;
+		break;
 	default:
 		*type = V4L2_CTRL_TYPE_INTEGER;
 		break;

include/linux/videodev2.h +1 -0

index 9633c69..c9d07c7 100644
@@ -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)

include/linux/videodev2.h +2 -0

@@ -1690,6 +1691,16 @@ 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 |

Quoted 1 line

0x900) +#define V4L2_CID_IMAGE_SOURCE_CLASS

(V4L2_CTRL_CLASS_IMAGE_SOURCE

Quoted 2 lines

| 1) +
+#define V4L2_CID_IMAGE_SOURCE_VBLANK (V4L2_CID_IMAGE_SOURCE_CLASS_BASE

Quoted 11 lines

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)
+#define
V4L2_CID_IMAGE_SOURCE_PIXEL_RATE (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 5) +
/*
* T U N I N G
*/

Signature · 3 lines

Regards,

Laurent Pinchart

Sakari Ailus

author

2012-01-11 21:27 UTC #14 lore raw

PATCH 9/23 v4l: Add DPCM compressed formats

In reply to Sakari Ailus

Add three other colour orders for 10-bit to 8-bit DPCM compressed formats.

Signed-off-by Sakari Ailus <[email protected]>

---
 Documentation/DocBook/media/v4l/pixfmt-srggb10.xml |    2 +-
 .../DocBook/media/v4l/pixfmt-srggb10dpcm8.xml      |   29 ++++++++++++++++++++
 Documentation/DocBook/media/v4l/pixfmt.xml         |    1 +
 include/linux/videodev2.h                          |    3 ++
 4 files changed, 34 insertions(+), 1 deletions(-)
 create mode 100644 Documentation/DocBook/media/v4l/pixfmt-srggb10dpcm8.xml

Documentation/DocBook/media/v4l/pixfmt-srggb10.xml +1 -1

index 7b27409..c1c62a9 100644
@@ -1,4 +1,4 @@
-    <refentry>
+    <refentry id="pixfmt-srggb10">
       <refmeta>
 	<refentrytitle>V4L2_PIX_FMT_SRGGB10 ('RG10'),
 	 V4L2_PIX_FMT_SGRBG10 ('BA10'),

Documentation/DocBook/media/v4l/pixfmt-srggb10dpcm8.xml new file +29 -0

new file mode 100644
index 0000000..985440c
@@ -0,0 +1,29 @@
+    <refentry>
+      <refmeta>
+	<refentrytitle>
+	 V4L2_PIX_FMT_SRGGB10DPCM8 ('bBA8'),
+	 V4L2_PIX_FMT_SGBRG10DPCM8 ('bGA8'),
+	 V4L2_PIX_FMT_SGRBG10DPCM8 ('BD10'),
+	 V4L2_PIX_FMT_SBGGR10DPCM8 ('bRA8'),
+	 </refentrytitle>
+	&manvol;
+      </refmeta>
+      <refnamediv>
+	<refname id="V4L2-PIX-FMT-SRGGB10DPCM8"><constant>V4L2_PIX_FMT_SRGGB10DPCM8</constant></refname>
+	<refname id="V4L2-PIX-FMT-SGRBG10DPCM8"><constant>V4L2_PIX_FMT_SGRBG10DPCM8</constant></refname>
+	<refname id="V4L2-PIX-FMT-SGBRG10DPCM8"><constant>V4L2_PIX_FMT_SGBRG10DPCM8</constant></refname>
+	<refname id="V4L2-PIX-FMT-SBGGR10DPCM8"><constant>V4L2_PIX_FMT_SBGGR10DPCM8</constant></refname>
+	<refpurpose>10-bit Bayer formats compressed to 8 bits</refpurpose>
+      </refnamediv>
+      <refsect1>
+	<title>Description</title>
+
+	<para>The following four pixel formats are raw sRGB / Bayer
+	formats with 10 bits per colour compressed to 8 bits each,
+	using the DPCM. DPCM, differential pulse-code modulation, is
+	lossy. Each colour component consumes 8 bits of memory. In
+	other respects this format is similar to
+	<xref linkend="pixfmt-srggb10">.</xref></para>
+
+      </refsect1>
+    </refentry>

Documentation/DocBook/media/v4l/pixfmt.xml +1 -0

index 2ff6b77..9b06c7b 100644
@@ -673,6 +673,7 @@ access the palette, this must be done with ioctls of the Linux framebuffer API.<
     &sub-srggb8;
     &sub-sbggr16;
     &sub-srggb10;
+    &sub-srggb10dpcm8;
     &sub-srggb12;
   </section>

include/linux/videodev2.h +3 -0

index c9d07c7..c5bf1db 100644
@@ -365,7 +365,10 @@ struct v4l2_pix_format {
 #define V4L2_PIX_FMT_SGRBG12 v4l2_fourcc('B', 'A', '1', '2') /* 12  GRGR.. BGBG.. */
 #define V4L2_PIX_FMT_SRGGB12 v4l2_fourcc('R', 'G', '1', '2') /* 12  RGRG.. GBGB.. */
 	/* 10bit raw bayer DPCM compressed to 8 bits */
+#define V4L2_PIX_FMT_SBGGR10DPCM8 v4l2_fourcc('b', 'B', 'A', '8')
+#define V4L2_PIX_FMT_SGBRG10DPCM8 v4l2_fourcc('b', 'G', 'A', '8')
 #define V4L2_PIX_FMT_SGRBG10DPCM8 v4l2_fourcc('B', 'D', '1', '0')
+#define V4L2_PIX_FMT_SRGGB10DPCM8 v4l2_fourcc('b', 'R', 'A', '8')
 	/*
 	 * 10bit raw bayer, expanded to 16 bits
 	 * xxxxrrrrrrrrrrxxxxgggggggggg xxxxggggggggggxxxxbbbbbbbbbb...

Signature · 1 line

1.7.2.5

Laurent Pinchart

maintainer

2012-01-16 14:01 UTC #15 lore raw

Re: [PATCH 09/23] v4l: Add DPCM compressed formats

Hi Sakari,

Thanks for the patch.

Quoted 30 lines · On Wednesday 11 January 2012 22:26:46 Sakari Ailus wrote:

Add three other colour orders for 10-bit to 8-bit DPCM compressed formats.
Signed-off-by Sakari Ailus <[email protected]>
---
 Documentation/DocBook/media/v4l/pixfmt-srggb10.xml |    2 +-
 .../DocBook/media/v4l/pixfmt-srggb10dpcm8.xml      |   29
++++++++++++++++++++ Documentation/DocBook/media/v4l/pixfmt.xml         | 
  1 +
 include/linux/videodev2.h                          |    3 ++
 4 files changed, 34 insertions(+), 1 deletions(-)
 create mode 100644 Documentation/DocBook/media/v4l/pixfmt-srggb10dpcm8.xml
(quoted diff) +0 -0
b/Documentation/DocBook/media/v4l/pixfmt-srggb10.xml index 7b27409..c1c62a9 100644
Documentation/DocBook/media/v4l/pixfmt-srggb10.xml +1 -1
@@ -1,4 +1,4 @@
-    <refentry>
+    <refentry id="pixfmt-srggb10">
       <refmeta>
 	<refentrytitle>V4L2_PIX_FMT_SRGGB10 ('RG10'),
 	 V4L2_PIX_FMT_SGRBG10 ('BA10'),
(quoted diff) +0 -0
b/Documentation/DocBook/media/v4l/pixfmt-srggb10dpcm8.xml new file mode 100644
index 0000000..985440c

Documentation/DocBook/media/v4l/pixfmt-srggb10dpcm8.xml line 7

Quoted 7 lines

Documentation/DocBook/media/v4l/pixfmt-srggb10dpcm8.xml new file +7 -0

@@ -0,0 +1,29 @@
+    <refentry>
+      <refmeta>
+	<refentrytitle>
+	 V4L2_PIX_FMT_SRGGB10DPCM8 ('bBA8'),
+	 V4L2_PIX_FMT_SGBRG10DPCM8 ('bGA8'),
+	 V4L2_PIX_FMT_SGRBG10DPCM8 ('BD10'),
+	 V4L2_PIX_FMT_SBGGR10DPCM8 ('bRA8'),

Could you briefly explain the rationale behind the FOURCCs in the patch commit message ? Manjunath needs similar FOURCCs for A-law compression, what should he use ?

Quoted 14 lines

Documentation/DocBook/media/v4l/pixfmt-srggb10dpcm8.xml new file +5 -0
+	 </refentrytitle>
+	&manvol;
+      </refmeta>
+      <refnamediv>
+	<refname
id="V4L2-PIX-FMT-SRGGB10DPCM8"><constant>V4L2_PIX_FMT_SRGGB10DPCM8</consta
nt></refname> + <refname
id="V4L2-PIX-FMT-SGRBG10DPCM8"><constant>V4L2_PIX_FMT_SGRBG10DPCM8</consta
nt></refname> + <refname
id="V4L2-PIX-FMT-SGBRG10DPCM8"><constant>V4L2_PIX_FMT_SGBRG10DPCM8</consta
nt></refname> + <refname
id="V4L2-PIX-FMT-SBGGR10DPCM8"><constant>V4L2_PIX_FMT_SBGGR10DPCM8</consta
nt></refname> + <refpurpose>10-bit Bayer formats compressed to 8
bits</refpurpose> + </refnamediv>

Documentation/DocBook/media/v4l/pixfmt-srggb10dpcm8.xml line 23

Quoted 6 lines

Documentation/DocBook/media/v4l/pixfmt-srggb10dpcm8.xml new file +6 -0

+      <refsect1>
+	<title>Description</title>
+
+	<para>The following four pixel formats are raw sRGB / Bayer
+	formats with 10 bits per colour compressed to 8 bits each,
+	using the DPCM. DPCM, differential pulse-code modulation, is

s/the DPCM/DPCM/ ?

Quoted 33 lines

Documentation/DocBook/media/v4l/pixfmt-srggb10dpcm8.xml new file +6 -0

+	lossy. Each colour component consumes 8 bits of memory. In
+	other respects this format is similar to
+	<xref linkend="pixfmt-srggb10">.</xref></para>
+
+      </refsect1>
+    </refentry>

(quoted diff) +0 -0

b/Documentation/DocBook/media/v4l/pixfmt.xml index 2ff6b77..9b06c7b 100644

Documentation/DocBook/media/v4l/pixfmt.xml +1 -0

@@ -673,6 +673,7 @@ access the palette, this must be done with ioctls of
 the Linux framebuffer API.< &sub-srggb8;
     &sub-sbggr16;
     &sub-srggb10;
+    &sub-srggb10dpcm8;
     &sub-srggb12;
   </section>

include/linux/videodev2.h +0 -0

index c9d07c7..c5bf1db 100644
@@ -365,7 +365,10 @@ struct v4l2_pix_format {
 #define V4L2_PIX_FMT_SGRBG12 v4l2_fourcc('B', 'A', '1', '2') /* 12  GRGR..

BGBG.. */ #define V4L2_PIX_FMT_SRGGB12 v4l2_fourcc('R', 'G', '1', '2') /* 12 RGRG.. GBGB.. */ /* 10bit raw bayer DPCM compressed to 8 bits */

include/linux/videodev2.h +3 -0

+#define V4L2_PIX_FMT_SBGGR10DPCM8 v4l2_fourcc('b', 'B', 'A', '8')
+#define V4L2_PIX_FMT_SGBRG10DPCM8 v4l2_fourcc('b', 'G', 'A', '8')
 #define V4L2_PIX_FMT_SGRBG10DPCM8 v4l2_fourcc('B', 'D', '1', '0')
+#define V4L2_PIX_FMT_SRGGB10DPCM8 v4l2_fourcc('b', 'R', 'A', '8')
 	/*
 	 * 10bit raw bayer, expanded to 16 bits
 	 * xxxxrrrrrrrrrrxxxxgggggggggg xxxxggggggggggxxxxbbbbbbbbbb...

Signature · 3 lines

Regards,

Laurent Pinchart

Sakari Ailus

author

2012-01-17 19:35 UTC #16 lore raw

Re: [PATCH 09/23] v4l: Add DPCM compressed formats

Hi Laurent,

Thanks for the review!

Quoted 34 lines · Laurent Pinchart

Quoted 33 lines · On Wednesday 11 January 2012 22:26:46 Sakari Ailus wrote:
Add three other colour orders for 10-bit to 8-bit DPCM compressed formats.
Signed-off-by Sakari Ailus <[email protected]>
---
 Documentation/DocBook/media/v4l/pixfmt-srggb10.xml |    2 +-
 .../DocBook/media/v4l/pixfmt-srggb10dpcm8.xml      |   29
++++++++++++++++++++ Documentation/DocBook/media/v4l/pixfmt.xml         | 
  1 +
 include/linux/videodev2.h                          |    3 ++
 4 files changed, 34 insertions(+), 1 deletions(-)
 create mode 100644 Documentation/DocBook/media/v4l/pixfmt-srggb10dpcm8.xml
(quoted diff) +0 -0
b/Documentation/DocBook/media/v4l/pixfmt-srggb10.xml index 7b27409..c1c62a9 100644
Documentation/DocBook/media/v4l/pixfmt-srggb10.xml +1 -1
@@ -1,4 +1,4 @@
-    <refentry>
+    <refentry id="pixfmt-srggb10">
       <refmeta>
 	<refentrytitle>V4L2_PIX_FMT_SRGGB10 ('RG10'),
 	 V4L2_PIX_FMT_SGRBG10 ('BA10'),
(quoted diff) +0 -0
b/Documentation/DocBook/media/v4l/pixfmt-srggb10dpcm8.xml new file mode 100644
index 0000000..985440c
Documentation/DocBook/media/v4l/pixfmt-srggb10dpcm8.xml new file +3 -0
@@ -0,0 +1,29 @@
+    <refentry>
+      <refmeta>
+	<refentrytitle>

Quoted 8 lines

Documentation/DocBook/media/v4l/pixfmt-srggb10dpcm8.xml line 7
Quoted 4 lines
Documentation/DocBook/media/v4l/pixfmt-srggb10dpcm8.xml new file +4 -0
+	 V4L2_PIX_FMT_SRGGB10DPCM8 ('bBA8'),
+	 V4L2_PIX_FMT_SGBRG10DPCM8 ('bGA8'),
+	 V4L2_PIX_FMT_SGRBG10DPCM8 ('BD10'),
+	 V4L2_PIX_FMT_SBGGR10DPCM8 ('bRA8'),
Could you briefly explain the rationale behind the FOURCCs in the patch commit message ? Manjunath needs similar FOURCCs for A-law compression, what should he use ?

Sure; I'll do that.

Quoted 14 lines

Quoted 14 lines
Documentation/DocBook/media/v4l/pixfmt-srggb10dpcm8.xml new file +5 -0
+	 </refentrytitle>
+	&manvol;
+      </refmeta>
+      <refnamediv>
+	<refname
id="V4L2-PIX-FMT-SRGGB10DPCM8"><constant>V4L2_PIX_FMT_SRGGB10DPCM8</consta
nt></refname> + <refname
id="V4L2-PIX-FMT-SGRBG10DPCM8"><constant>V4L2_PIX_FMT_SGRBG10DPCM8</consta
nt></refname> + <refname
id="V4L2-PIX-FMT-SGBRG10DPCM8"><constant>V4L2_PIX_FMT_SGBRG10DPCM8</consta
nt></refname> + <refname
id="V4L2-PIX-FMT-SBGGR10DPCM8"><constant>V4L2_PIX_FMT_SBGGR10DPCM8</consta
nt></refname> + <refpurpose>10-bit Bayer formats compressed to 8
bits</refpurpose> + </refnamediv>

Quoted 8 lines

Documentation/DocBook/media/v4l/pixfmt-srggb10dpcm8.xml line 23
Quoted 6 lines
Documentation/DocBook/media/v4l/pixfmt-srggb10dpcm8.xml new file +6 -0
+      <refsect1>
+	<title>Description</title>
+
+	<para>The following four pixel formats are raw sRGB / Bayer
+	formats with 10 bits per colour compressed to 8 bits each,
+	using the DPCM. DPCM, differential pulse-code modulation, is
s/the DPCM/DPCM/ ?

Fixed. It's now s/the DPCM/DPCM compression/.

Signature · 2 lines

Sakari Ailus
e-mail: [email protected]	jabber/XMPP/Gmail: [email protected]

Sakari Ailus

author

2012-01-11 21:27 UTC #17 lore raw

PATCH 10/23 omap3isp: Support additional in-memory compressed bayer formats

In reply to Sakari Ailus

This also prevents accessing NULL pointer in csi2_try_format().

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/video/omap3isp/ispvideo.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

drivers/media/video/omap3isp/ispvideo.c +13 -0

index f229057..9cc5090 100644
@@ -46,6 +46,10 @@
  * Helper functions
  */
 
+/*
+ * NOTE: When adding new media bus codes, always remember to add
+ * corresponding in-memory formats to the table below!!!
+ */
 static struct isp_format_info formats[] = {
 	{ V4L2_MBUS_FMT_Y8_1X8, V4L2_MBUS_FMT_Y8_1X8,
 	  V4L2_MBUS_FMT_Y8_1X8, V4L2_MBUS_FMT_Y8_1X8,

@@ -68,9 +72,18 @@ static struct isp_format_info formats[] = {
 	{ V4L2_MBUS_FMT_SRGGB8_1X8, V4L2_MBUS_FMT_SRGGB8_1X8,
 	  V4L2_MBUS_FMT_SRGGB8_1X8, V4L2_MBUS_FMT_SRGGB8_1X8,
 	  V4L2_PIX_FMT_SRGGB8, 8, },
+	{ V4L2_MBUS_FMT_SBGGR10_DPCM8_1X8, V4L2_MBUS_FMT_SBGGR10_DPCM8_1X8,
+	  V4L2_MBUS_FMT_SBGGR10_1X10, 0,
+	  V4L2_PIX_FMT_SBGGR10DPCM8, 8, },
+	{ V4L2_MBUS_FMT_SGBRG10_DPCM8_1X8, V4L2_MBUS_FMT_SGBRG10_DPCM8_1X8,
+	  V4L2_MBUS_FMT_SGBRG10_1X10, 0,
+	  V4L2_PIX_FMT_SGBRG10DPCM8, 8, },
 	{ V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8, V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8,
 	  V4L2_MBUS_FMT_SGRBG10_1X10, 0,
 	  V4L2_PIX_FMT_SGRBG10DPCM8, 8, },
+	{ V4L2_MBUS_FMT_SRGGB10_DPCM8_1X8, V4L2_MBUS_FMT_SRGGB10_DPCM8_1X8,
+	  V4L2_MBUS_FMT_SRGGB10_1X10, 0,
+	  V4L2_PIX_FMT_SRGGB10DPCM8, 8, },
 	{ V4L2_MBUS_FMT_SBGGR10_1X10, V4L2_MBUS_FMT_SBGGR10_1X10,
 	  V4L2_MBUS_FMT_SBGGR10_1X10, V4L2_MBUS_FMT_SBGGR8_1X8,
 	  V4L2_PIX_FMT_SBGGR10, 10, },

Signature · 1 line

1.7.2.5

Laurent Pinchart

maintainer Acked-by on v1 10/23

2012-01-16 14:04 UTC #18 lore raw

Re: [PATCH 10/23] omap3isp: Support additional in-memory compressed bayer formats

Quoted 3 lines · On Wednesday 11 January 2012 22:26:47 Sakari Ailus wrote:

This also prevents accessing NULL pointer in csi2_try_format().
Signed-off-by Sakari Ailus <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

Signature · 3 lines

Regards,

Laurent Pinchart

Sakari Ailus

author

2012-01-11 21:27 UTC #19 lore raw

PATCH 11/23 omap3isp: Move definitions required by board code under include/media.

In reply to Sakari Ailus

XCLK definitions are often required by the board code. Move them to public include file.

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/video/omap3isp/isp.h |    4 ----
 include/media/omap3isp.h           |    4 ++++
 2 files changed, 4 insertions(+), 4 deletions(-)

drivers/media/video/omap3isp/isp.h +0 -4

index 705946e..ff1c422 100644
@@ -239,10 +239,6 @@ void omap3isp_configure_bridge(struct isp_device *isp,
 			       const struct isp_parallel_platform_data *pdata,
 			       unsigned int shift);
 
-#define ISP_XCLK_NONE			0
-#define ISP_XCLK_A			1
-#define ISP_XCLK_B			2
-
 struct isp_device *omap3isp_get(struct isp_device *isp);
 void omap3isp_put(struct isp_device *isp);

include/media/omap3isp.h +4 -0

index e917b1d..9c1a001 100644
@@ -29,6 +29,10 @@
 struct i2c_board_info;
 struct isp_device;
 
+#define ISP_XCLK_NONE			0
+#define ISP_XCLK_A			1
+#define ISP_XCLK_B			2
+
 enum isp_interface_type {
 	ISP_INTERFACE_PARALLEL,
 	ISP_INTERFACE_CSI2A_PHY2,

Signature · 1 line

1.7.2.5

Laurent Pinchart

maintainer Acked-by on v1 11/23

2012-01-16 14:05 UTC #20 lore raw

Re: [PATCH 11/23] omap3isp: Move definitions required by board code under include/media.

Quoted 4 lines · On Wednesday 11 January 2012 22:26:48 Sakari Ailus wrote:

XCLK definitions are often required by the board code. Move them to public include file.
Signed-off-by Sakari Ailus <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

Signature · 3 lines

Regards,

Laurent Pinchart

Sakari Ailus

author

2012-01-11 21:27 UTC #21 lore raw

PATCH 12/23 omap3: add definition for CONTROL_CAMERA_PHY_CTRL

In reply to Sakari Ailus

This register is available only in OMAP3630.

The original patch was submitted by Vimarsh Zutshi.

Signed-off-by Sakari Ailus <[email protected]>

---
 arch/arm/mach-omap2/control.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

arch/arm/mach-omap2/control.h +1 -0

index d4ef75d..6a26a0d 100644
@@ -183,6 +183,7 @@
 #define OMAP3630_CONTROL_FUSE_OPP120_VDD1       (OMAP2_CONTROL_GENERAL + 0x0120)
 #define OMAP3630_CONTROL_FUSE_OPP50_VDD2        (OMAP2_CONTROL_GENERAL + 0x0128)
 #define OMAP3630_CONTROL_FUSE_OPP100_VDD2       (OMAP2_CONTROL_GENERAL + 0x012C)
+#define OMAP3630_CONTROL_CAMERA_PHY_CTRL	(OMAP2_CONTROL_GENERAL + 0x02f0)
 
 /* OMAP44xx control efuse offsets */
 #define OMAP44XX_CONTROL_FUSE_IVA_OPP50		0x22C

Signature · 1 line

1.7.2.5

Laurent Pinchart

maintainer Acked-by on v1 12/23

2012-01-16 14:06 UTC #22 lore raw

Re: [PATCH 12/23] omap3: add definition for CONTROL_CAMERA_PHY_CTRL

Quoted 5 lines · On Wednesday 11 January 2012 22:26:49 Sakari Ailus wrote:

This register is available only in OMAP3630.
The original patch was submitted by Vimarsh Zutshi.
Signed-off-by Sakari Ailus <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

Signature · 3 lines

Regards,

Laurent Pinchart

Sakari Ailus

author

2012-01-11 21:27 UTC #23 lore raw

PATCH 13/23 omap3isp: Add lane configuration to platform data

In reply to Sakari Ailus

Add lane configuration (order of clock and data lane) to platform data on both CCP2 and CSI-2.

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/video/omap3isp/ispcsiphy.h |   15 ++-------------
 include/media/omap3isp.h                 |   25 +++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 13 deletions(-)

drivers/media/video/omap3isp/ispcsiphy.h +2 ~~-13~~

index 9596dc6..e93a661 100644
@@ -27,22 +27,11 @@
 #ifndef OMAP3_ISP_CSI_PHY_H
 #define OMAP3_ISP_CSI_PHY_H
 
+#include <media/omap3isp.h>
+
 struct isp_csi2_device;
 struct regulator;
 
-struct csiphy_lane {
-	u8 pos;
-	u8 pol;
-};
-
-#define ISP_CSIPHY2_NUM_DATA_LANES	2
-#define ISP_CSIPHY1_NUM_DATA_LANES	1
-
-struct isp_csiphy_lanes_cfg {
-	struct csiphy_lane data[ISP_CSIPHY2_NUM_DATA_LANES];
-	struct csiphy_lane clk;
-};
-
 struct isp_csiphy_dphy_cfg {
 	u8 ths_term;
 	u8 ths_settle;

include/media/omap3isp.h +25 -0

index 9c1a001..bc14099 100644
@@ -91,6 +91,29 @@ enum {
 };
 
 /**
+ * struct isp_csiphy_lane: CCP2/CSI2 lane position and polarity
+ * @pos: position of the lane
+ * @pol: polarity of the lane
+ */
+struct isp_csiphy_lane {
+	u8 pos;
+	u8 pol;
+};
+
+#define ISP_CSIPHY2_NUM_DATA_LANES	2
+#define ISP_CSIPHY1_NUM_DATA_LANES	1
+
+/**
+ * struct isp_csiphy_lanes_cfg - CCP2/CSI2 lane configuration
+ * @data: Configuration of one or two data lanes
+ * @clk: Clock lane configuration
+ */
+struct isp_csiphy_lanes_cfg {
+	struct isp_csiphy_lane data[ISP_CSIPHY2_NUM_DATA_LANES];
+	struct isp_csiphy_lane clk;
+};
+
+/**
  * struct isp_ccp2_platform_data - CCP2 interface platform data
  * @strobe_clk_pol: Strobe/clock polarity
  *		0 - Non Inverted, 1 - Inverted

@@ -109,6 +132,7 @@ struct isp_ccp2_platform_data {
 	unsigned int ccp2_mode:1;
 	unsigned int phy_layer:1;
 	unsigned int vpclk_div:2;
+	struct isp_csiphy_lanes_cfg lanecfg;
 };
 
 /**

@@ -119,6 +143,7 @@ struct isp_ccp2_platform_data {
 struct isp_csi2_platform_data {
 	unsigned crc:1;
 	unsigned vpclk_div:2;
+	struct isp_csiphy_lanes_cfg lanecfg;
 };
 
 struct isp_subdev_i2c_board_info {

Signature · 1 line

1.7.2.5

Laurent Pinchart

maintainer Acked-by on v1 13/23

2012-01-16 14:08 UTC #24 lore raw

Re: [PATCH 13/23] omap3isp: Add lane configuration to platform data

Hi Sakari,

Thanks for the patch.

Quoted 47 lines · On Wednesday 11 January 2012 22:26:50 Sakari Ailus wrote:

Add lane configuration (order of clock and data lane) to platform data on both CCP2 and CSI-2.

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/video/omap3isp/ispcsiphy.h |   15 ++-------------
 include/media/omap3isp.h                 |   25 +++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 13 deletions(-)

(quoted diff) +0 -0

b/drivers/media/video/omap3isp/ispcsiphy.h index 9596dc6..e93a661 100644

drivers/media/video/omap3isp/ispcsiphy.h +2 ~~-13~~

@@ -27,22 +27,11 @@
 #ifndef OMAP3_ISP_CSI_PHY_H
 #define OMAP3_ISP_CSI_PHY_H
 
+#include <media/omap3isp.h>
+
 struct isp_csi2_device;
 struct regulator;
 
-struct csiphy_lane {
-	u8 pos;
-	u8 pol;
-};
-
-#define ISP_CSIPHY2_NUM_DATA_LANES	2
-#define ISP_CSIPHY1_NUM_DATA_LANES	1
-
-struct isp_csiphy_lanes_cfg {
-	struct csiphy_lane data[ISP_CSIPHY2_NUM_DATA_LANES];
-	struct csiphy_lane clk;
-};
-
 struct isp_csiphy_dphy_cfg {
 	u8 ths_term;
 	u8 ths_settle;

include/media/omap3isp.h +1 -0

index 9c1a001..bc14099 100644
@@ -91,6 +91,29 @@ enum {
 };
 
 /**
+ * struct isp_csiphy_lane: CCP2/CSI2 lane position and polarity

include/media/omap3isp.h enum { · line 104

Quoted 10 lines

include/media/omap3isp.h +10 -0

+ * @pos: position of the lane
+ * @pol: polarity of the lane
+ */
+struct isp_csiphy_lane {
+	u8 pos;
+	u8 pol;
+};
+
+#define ISP_CSIPHY2_NUM_DATA_LANES	2
+#define ISP_CSIPHY1_NUM_DATA_LANES	1

Any reason not to put CSIPHY1 first ? :-)

With that modification,

Acked-by Laurent Pinchart <[email protected]>

Quoted 31 lines

include/media/omap3isp.h +14 -0

+
+/**
+ * struct isp_csiphy_lanes_cfg - CCP2/CSI2 lane configuration
+ * @data: Configuration of one or two data lanes
+ * @clk: Clock lane configuration
+ */
+struct isp_csiphy_lanes_cfg {
+	struct isp_csiphy_lane data[ISP_CSIPHY2_NUM_DATA_LANES];
+	struct isp_csiphy_lane clk;
+};
+
+/**
  * struct isp_ccp2_platform_data - CCP2 interface platform data
  * @strobe_clk_pol: Strobe/clock polarity
  *		0 - Non Inverted, 1 - Inverted

@@ -109,6 +132,7 @@ struct isp_ccp2_platform_data {
 	unsigned int ccp2_mode:1;
 	unsigned int phy_layer:1;
 	unsigned int vpclk_div:2;
+	struct isp_csiphy_lanes_cfg lanecfg;
 };
 
 /**

@@ -119,6 +143,7 @@ struct isp_ccp2_platform_data {
 struct isp_csi2_platform_data {
 	unsigned crc:1;
 	unsigned vpclk_div:2;
+	struct isp_csiphy_lanes_cfg lanecfg;
 };
 
 struct isp_subdev_i2c_board_info {

Signature · 3 lines

Regards,

Laurent Pinchart

Sakari Ailus

author

2012-01-17 19:27 UTC #25 lore raw

Re: [PATCH 13/23] omap3isp: Add lane configuration to platform data

Hi Laurent,

Thanks for the review!

Quoted 46 lines · Laurent Pinchart

Quoted 45 lines · On Wednesday 11 January 2012 22:26:50 Sakari Ailus wrote:
Add lane configuration (order of clock and data lane) to platform data on both CCP2 and CSI-2.
Signed-off-by Sakari Ailus <[email protected]>
---
 drivers/media/video/omap3isp/ispcsiphy.h |   15 ++-------------
 include/media/omap3isp.h                 |   25 +++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 13 deletions(-)
(quoted diff) +0 -0
b/drivers/media/video/omap3isp/ispcsiphy.h index 9596dc6..e93a661 100644
drivers/media/video/omap3isp/ispcsiphy.h +2 ~~-13~~
@@ -27,22 +27,11 @@
 #ifndef OMAP3_ISP_CSI_PHY_H
 #define OMAP3_ISP_CSI_PHY_H
 
+#include <media/omap3isp.h>
+
 struct isp_csi2_device;
 struct regulator;
 
-struct csiphy_lane {
-	u8 pos;
-	u8 pol;
-};
-
-#define ISP_CSIPHY2_NUM_DATA_LANES	2
-#define ISP_CSIPHY1_NUM_DATA_LANES	1
-
-struct isp_csiphy_lanes_cfg {
-	struct csiphy_lane data[ISP_CSIPHY2_NUM_DATA_LANES];
-	struct csiphy_lane clk;
-};
-
 struct isp_csiphy_dphy_cfg {
 	u8 ths_term;
 	u8 ths_settle;
include/media/omap3isp.h +0 -0
index 9c1a001..bc14099 100644
@@ -91,6 +91,29 @@ enum {
 };
 

Quoted 14 lines

include/media/omap3isp.h enum { · line 104

Quoted 12 lines

include/media/omap3isp.h +11 -0

 /**
+ * struct isp_csiphy_lane: CCP2/CSI2 lane position and polarity
+ * @pos: position of the lane
+ * @pol: polarity of the lane
+ */
+struct isp_csiphy_lane {
+	u8 pos;
+	u8 pol;
+};
+
+#define ISP_CSIPHY2_NUM_DATA_LANES	2
+#define ISP_CSIPHY1_NUM_DATA_LANES	1

Any reason not to put CSIPHY1 first ? :-)

Yes. I believe wrote it that way. ;-)

I'll change it.

Quoted 3 lines

With that modification,
Acked-by Laurent Pinchart <[email protected]>

Thanks!

Signature · 2 lines

Sakari Ailus
e-mail: [email protected]	jabber/XMPP/Gmail: [email protected]

Sakari Ailus

author

2012-01-11 21:27 UTC #26 lore raw

PATCH 14/23 omap3isp: Configure CSI-2 phy based on platform data

In reply to Sakari Ailus

Configure CSI-2 phy based on platform data in the ISP driver. For that, the new V4L2_CID_IMAGE_SOURCE_PIXEL_RATE control is used. Previously the same was configured from the board code.

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/video/omap3isp/isp.c       |   29 ++++++++++-
 drivers/media/video/omap3isp/isp.h       |    4 --
 drivers/media/video/omap3isp/ispcsi2.c   |   42 ++++++++++++++-
 drivers/media/video/omap3isp/ispcsiphy.c |   84 ++++++++++++++++++++++++++----
 drivers/media/video/omap3isp/ispcsiphy.h |    5 ++
 5 files changed, 148 insertions(+), 16 deletions(-)

drivers/media/video/omap3isp/isp.c +28 -1

index b818cac..d268d55 100644
@@ -765,6 +765,34 @@ static int isp_pipeline_enable(struct isp_pipeline *pipe,
 		if (ret < 0 && ret != -ENOIOCTLCMD)
 			return ret;
 
+		/*
+		 * Configure CCDC pixel clock. host_priv != NULL so
+		 * this one is a sensor.
+		 */
+		if (subdev->host_priv) {
+			struct v4l2_ext_controls ctrls;
+			struct v4l2_ext_control ctrl;
+
+			memset(&ctrls, 0, sizeof(ctrls));
+			memset(&ctrl, 0, sizeof(ctrl));
+
+			ctrl.id = V4L2_CID_IMAGE_SOURCE_PIXEL_RATE;
+
+			ctrls.ctrl_class = V4L2_CTRL_ID2CLASS(ctrl.id);
+			ctrls.count = 1;
+			ctrls.controls = &ctrl;
+
+			ret = v4l2_g_ext_ctrls(subdev->ctrl_handler, &ctrls);
+			if (ret < 0) {
+				dev_warn(isp->dev,
+					 "no pixel rate control in subdev %s\n",
+					 subdev->name);
+				return -EPIPE;
+			}
+
+			isp_set_pixel_clock(isp, ctrl.value64);
+		}
+
 		if (subdev == &isp->isp_ccdc.subdev) {
 			v4l2_subdev_call(&isp->isp_aewb.subdev, video,
 					s_stream, mode);

@@ -2072,7 +2100,6 @@ static int isp_probe(struct platform_device *pdev)
 
 	isp->autoidle = autoidle;
 	isp->platform_cb.set_xclk = isp_set_xclk;
-	isp->platform_cb.set_pixel_clock = isp_set_pixel_clock;
 
 	mutex_init(&isp->isp_mutex);
 	spin_lock_init(&isp->stat_lock);

drivers/media/video/omap3isp/isp.h +0 -4

index ff1c422..dd1b61e 100644
@@ -126,10 +126,6 @@ struct isp_reg {
 
 struct isp_platform_callback {
 	u32 (*set_xclk)(struct isp_device *isp, u32 xclk, u8 xclksel);
-	int (*csiphy_config)(struct isp_csiphy *phy,
-			     struct isp_csiphy_dphy_cfg *dphy,
-			     struct isp_csiphy_lanes_cfg *lanes);
-	void (*set_pixel_clock)(struct isp_device *isp, unsigned int pixelclk);
 };
 
 /*

drivers/media/video/omap3isp/ispcsi2.c +41 -1

54 lines not on this page · raw message

drivers/media/video/omap3isp/ispcsiphy.c +74 ~~-10~~

113 lines not on this page · raw message

drivers/media/video/omap3isp/ispcsiphy.h +5 -0

index e93a661..fc7e821 100644
@@ -56,6 +56,11 @@ struct isp_csiphy {
 	struct isp_csiphy_dphy_cfg dphy;
 };
 
+int omap3isp_csiphy_config(struct isp_device *isp,
+			   struct v4l2_subdev *csi2_subdev,
+			   struct v4l2_subdev *sensor,
+			   struct v4l2_mbus_framefmt *sensor_fmt,
+			   uint32_t pixel_rate);
 int omap3isp_csiphy_acquire(struct isp_csiphy *phy);
 void omap3isp_csiphy_release(struct isp_csiphy *phy);
 int omap3isp_csiphy_init(struct isp_device *isp);

Signature · 1 line

1.7.2.5

Laurent Pinchart

maintainer

2012-01-16 14:22 UTC #27 lore raw

Re: [PATCH 14/23] omap3isp: Configure CSI-2 phy based on platform data

Hi Sakari,

Thanks for the patch.

Quoted 38 lines · On Wednesday 11 January 2012 22:26:51 Sakari Ailus wrote:

Configure CSI-2 phy based on platform data in the ISP driver. For that, the new V4L2_CID_IMAGE_SOURCE_PIXEL_RATE control is used. Previously the same was configured from the board code.

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/video/omap3isp/isp.c       |   29 ++++++++++-
 drivers/media/video/omap3isp/isp.h       |    4 --
 drivers/media/video/omap3isp/ispcsi2.c   |   42 ++++++++++++++-
 drivers/media/video/omap3isp/ispcsiphy.c |   84
++++++++++++++++++++++++++---- drivers/media/video/omap3isp/ispcsiphy.h | 
  5 ++
 5 files changed, 148 insertions(+), 16 deletions(-)

(quoted diff) +0 -0

b/drivers/media/video/omap3isp/isp.c index b818cac..d268d55 100644

drivers/media/video/omap3isp/isp.c +16 -0

@@ -765,6 +765,34 @@ static int isp_pipeline_enable(struct isp_pipeline
 *pipe, if (ret < 0 && ret != -ENOIOCTLCMD)
 			return ret;
 
+		/*
+		 * Configure CCDC pixel clock. host_priv != NULL so
+		 * this one is a sensor.
+		 */
+		if (subdev->host_priv) {
+			struct v4l2_ext_controls ctrls;
+			struct v4l2_ext_control ctrl;
+
+			memset(&ctrls, 0, sizeof(ctrls));
+			memset(&ctrl, 0, sizeof(ctrl));
+
+			ctrl.id = V4L2_CID_IMAGE_SOURCE_PIXEL_RATE;
+
+			ctrls.ctrl_class = V4L2_CTRL_ID2CLASS(ctrl.id);
+			ctrls.count = 1;
+			ctrls.controls = &ctrl;

drivers/media/video/omap3isp/isp.c static int isp_pipeline_enable(struct isp_pipeline · line 793

Quoted 10 lines

drivers/media/video/omap3isp/isp.c +10 -0

+
+			ret = v4l2_g_ext_ctrls(subdev->ctrl_handler, &ctrls);
+			if (ret < 0) {
+				dev_warn(isp->dev,
+					 "no pixel rate control in subdev %s\n",
+					 subdev->name);
+				return -EPIPE;
+			}
+
+			isp_set_pixel_clock(isp, ctrl.value64);

Isn't this too late ? The CCDC has already been started. What about moving this code to ccdc_config_vp() ?

Quoted 33 lines

drivers/media/video/omap3isp/isp.c +2 -1

+		}
+
 		if (subdev == &isp->isp_ccdc.subdev) {
 			v4l2_subdev_call(&isp->isp_aewb.subdev, video,
 					s_stream, mode);

@@ -2072,7 +2100,6 @@ static int isp_probe(struct platform_device *pdev)
 
 	isp->autoidle = autoidle;
 	isp->platform_cb.set_xclk = isp_set_xclk;
-	isp->platform_cb.set_pixel_clock = isp_set_pixel_clock;
 
 	mutex_init(&isp->isp_mutex);
 	spin_lock_init(&isp->stat_lock);

(quoted diff) +0 -0

b/drivers/media/video/omap3isp/isp.h index ff1c422..dd1b61e 100644

drivers/media/video/omap3isp/isp.h +0 -4

@@ -126,10 +126,6 @@ struct isp_reg {
 
 struct isp_platform_callback {
 	u32 (*set_xclk)(struct isp_device *isp, u32 xclk, u8 xclksel);
-	int (*csiphy_config)(struct isp_csiphy *phy,
-			     struct isp_csiphy_dphy_cfg *dphy,
-			     struct isp_csiphy_lanes_cfg *lanes);
-	void (*set_pixel_clock)(struct isp_device *isp, unsigned int pixelclk);
 };
 
 /*

(quoted diff) +0 -0

b/drivers/media/video/omap3isp/ispcsi2.c index 0c5f1cb..0b3e705 100644

drivers/media/video/omap3isp/ispcsi2.c static int csi2_set_stream(struct v4l2_subdev *sd, · line 1060

Quoted 7 lines

drivers/media/video/omap3isp/ispcsi2.c +3 -1

@@ -1055,7 +1055,45 @@ static int csi2_set_stream(struct v4l2_subdev *sd,
 int enable) struct isp_video *video_out = &csi2->video_out;
 
 	switch (enable) {
-	case ISP_PIPELINE_STREAM_CONTINUOUS:
+	case ISP_PIPELINE_STREAM_CONTINUOUS: {
+		struct media_pad *remote_pad =
+			media_entity_remote_source(&sd->entity.pads[0]);

As you will need to locate the sensor in ccdc_config_vp() as well, you should store a pointer to the sensor in the pipeline structure in isp_video_streamon(). I think I've sent you code that does just that, originally written by Stan if my memory is correct.

Quoted 14 lines

drivers/media/video/omap3isp/ispcsi2.c +14 -0

+		struct v4l2_subdev *remote_subdev =
+			media_entity_to_v4l2_subdev(remote_pad->entity);
+		struct v4l2_subdev_format fmt;
+		struct v4l2_ext_controls ctrls;
+		struct v4l2_ext_control ctrl;
+		int ret;
+
+		fmt.pad = remote_pad->index;
+		fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+		ret = v4l2_subdev_call(
+			remote_subdev, pad, get_fmt, NULL, &fmt);
+		if (ret < 0)
+			return -EPIPE;
+

drivers/media/video/omap3isp/ispcsi2.c static int csi2_set_stream(struct v4l2_subdev *sd, int enable) · line 1084

Quoted 10 lines

drivers/media/video/omap3isp/ispcsi2.c +10 -0

+		memset(&ctrls, 0, sizeof(ctrls));
+		memset(&ctrl, 0, sizeof(ctrl));
+
+		ctrl.id = V4L2_CID_IMAGE_SOURCE_PIXEL_RATE;
+
+		ctrls.ctrl_class = V4L2_CTRL_ID2CLASS(ctrl.id);
+		ctrls.count = 1;
+		ctrls.controls = &ctrl;
+
+		ret = v4l2_g_ext_ctrls(remote_subdev->ctrl_handler, &ctrls);

Wouldn't v4l2_g_ctrl be easier ?

An option to avoid duplicating this code in ccdc_config_vp() would be to move pixel rate retrieval to isp_video_streamon().

Quoted 29 lines

drivers/media/video/omap3isp/ispcsi2.c +14 -0

+		if (ret < 0) {
+			dev_warn(isp->dev,
+				 "no pixel rate control in subdev %s\n",
+				 remote_subdev->name);
+			return -EPIPE;
+		}
+
+		ret = omap3isp_csiphy_config(
+			isp, sd, remote_subdev, &fmt.format, ctrl.value64);
+		if (ret < 0)
+			return -EPIPE;
+
 		if (omap3isp_csiphy_acquire(csi2->phy) < 0)
 			return -ENODEV;
 		csi2->use_fs_irq = pipe->do_propagation;

@@ -1080,6 +1118,8 @@ static int csi2_set_stream(struct v4l2_subdev *sd,
 int enable) isp_video_dmaqueue_flags_clr(video_out);
 		break;
 
+	}
+
 	case ISP_PIPELINE_STREAM_STOPPED:
 		if (csi2->state == ISP_PIPELINE_STREAM_STOPPED)
 			return 0;

(quoted diff) +0 -0

b/drivers/media/video/omap3isp/ispcsiphy.c index 5be37ce..f286a01 100644

drivers/media/video/omap3isp/ispcsiphy.c line 32

Quoted 5 lines

drivers/media/video/omap3isp/ispcsiphy.c +2 -0

@@ -28,6 +28,8 @@
 #include <linux/device.h>
 #include <linux/regulator/consumer.h>
 
+#include "../../../../arch/arm/mach-omap2/control.h"
+

Still no solution for this ?

Quoted 28 lines

drivers/media/video/omap3isp/ispcsiphy.c +17 -3

 #include "isp.h"
 #include "ispreg.h"
 #include "ispcsiphy.h"

@@ -138,15 +140,79 @@ static void csiphy_dphy_config(struct isp_csiphy
 *phy) isp_reg_writel(phy->isp, reg, phy->phy_regs, ISPCSIPHY_REG1);
 }
 
-static int csiphy_config(struct isp_csiphy *phy,
-			 struct isp_csiphy_dphy_cfg *dphy,
-			 struct isp_csiphy_lanes_cfg *lanes)
+/*
+ * TCLK values are OK at their reset values
+ */
+#define TCLK_TERM	0
+#define TCLK_MISS	1
+#define TCLK_SETTLE	14
+
+int omap3isp_csiphy_config(struct isp_device *isp,
+			   struct v4l2_subdev *csi2_subdev,
+			   struct v4l2_subdev *sensor,
+			   struct v4l2_mbus_framefmt *sensor_fmt,
+			   uint32_t pixel_rate)
 {
+	struct isp_v4l2_subdevs_group *subdevs = sensor->host_priv;
+	struct isp_csi2_device *csi2 = v4l2_get_subdevdata(csi2_subdev);
+	struct isp_csiphy_dphy_cfg csi2phy;
+	int csi2_ddrclk_khz;
+	struct isp_csiphy_lanes_cfg *lanes;

drivers/media/video/omap3isp/ispcsiphy.c static void csiphy_dphy_config(struct isp_csiphy · line 170

Quoted 10 lines

drivers/media/video/omap3isp/ispcsiphy.c +7 -0

 	unsigned int used_lanes = 0;
 	unsigned int i;
 
+	if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY1
+	    || subdevs->interface == ISP_INTERFACE_CCP2B_PHY2)
+		lanes = &subdevs->bus.ccp2.lanecfg;
+	else
+		lanes = &subdevs->bus.csi2.lanecfg;
+
+	if (!lanes) {

This can't happen.

Quoted 3 lines

drivers/media/video/omap3isp/ispcsiphy.c +3 -0
+		dev_err(isp->dev, "no lane configuration\n");
+		return -EINVAL;
+	}

[snip]

Signature · 3 lines

Regards,

Laurent Pinchart

Sakari Ailus

author

2012-01-17 19:45 UTC #28 lore raw

Re: [PATCH 14/23] omap3isp: Configure CSI-2 phy based on platform data

Hi Laurent,

Many thanks for reviewing these!

Quoted 44 lines · Laurent Pinchart

Quoted 43 lines · On Wednesday 11 January 2012 22:26:51 Sakari Ailus wrote:

Configure CSI-2 phy based on platform data in the ISP driver. For that, the new V4L2_CID_IMAGE_SOURCE_PIXEL_RATE control is used. Previously the same was configured from the board code.

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/video/omap3isp/isp.c       |   29 ++++++++++-
 drivers/media/video/omap3isp/isp.h       |    4 --
 drivers/media/video/omap3isp/ispcsi2.c   |   42 ++++++++++++++-
 drivers/media/video/omap3isp/ispcsiphy.c |   84
++++++++++++++++++++++++++---- drivers/media/video/omap3isp/ispcsiphy.h | 
  5 ++
 5 files changed, 148 insertions(+), 16 deletions(-)

(quoted diff) +0 -0

b/drivers/media/video/omap3isp/isp.c index b818cac..d268d55 100644

drivers/media/video/omap3isp/isp.c +21 -0

@@ -765,6 +765,34 @@ static int isp_pipeline_enable(struct isp_pipeline
 *pipe, if (ret < 0 && ret != -ENOIOCTLCMD)
 			return ret;
 
+		/*
+		 * Configure CCDC pixel clock. host_priv != NULL so
+		 * this one is a sensor.
+		 */
+		if (subdev->host_priv) {
+			struct v4l2_ext_controls ctrls;
+			struct v4l2_ext_control ctrl;
+
+			memset(&ctrls, 0, sizeof(ctrls));
+			memset(&ctrl, 0, sizeof(ctrl));
+
+			ctrl.id = V4L2_CID_IMAGE_SOURCE_PIXEL_RATE;
+
+			ctrls.ctrl_class = V4L2_CTRL_ID2CLASS(ctrl.id);
+			ctrls.count = 1;
+			ctrls.controls = &ctrl;
+
+			ret = v4l2_g_ext_ctrls(subdev->ctrl_handler, &ctrls);
+			if (ret < 0) {
+				dev_warn(isp->dev,
+					 "no pixel rate control in subdev %s\n",

Quoted 8 lines

drivers/media/video/omap3isp/isp.c static int isp_pipeline_enable(struct isp_pipeline *pipe, · line 793
Quoted 5 lines
drivers/media/video/omap3isp/isp.c +5 -0
+					 subdev->name);
+				return -EPIPE;
+			}
+
+			isp_set_pixel_clock(isp, ctrl.value64);
Isn't this too late ? The CCDC has already been started. What about moving this code to ccdc_config_vp() ?

I think you're right. I've mostly rewritten this part of the patchset and I think it's much cleaner now.

I have validate_link() functions where I'm validating the link and also gathering information alongside that process into a few fields in isp_pipeline struct.

Then those fields can be used in streamon time. Some additional checks for those parameters are also done there and they may result into an error which is then handled.

Quoted 35 lines

drivers/media/video/omap3isp/isp.c +2 -1

+		}
+
 		if (subdev == &isp->isp_ccdc.subdev) {
 			v4l2_subdev_call(&isp->isp_aewb.subdev, video,
 					s_stream, mode);

@@ -2072,7 +2100,6 @@ static int isp_probe(struct platform_device *pdev)
 
 	isp->autoidle = autoidle;
 	isp->platform_cb.set_xclk = isp_set_xclk;
-	isp->platform_cb.set_pixel_clock = isp_set_pixel_clock;
 
 	mutex_init(&isp->isp_mutex);
 	spin_lock_init(&isp->stat_lock);

(quoted diff) +0 -0

b/drivers/media/video/omap3isp/isp.h index ff1c422..dd1b61e 100644

drivers/media/video/omap3isp/isp.h +0 -4

@@ -126,10 +126,6 @@ struct isp_reg {
 
 struct isp_platform_callback {
 	u32 (*set_xclk)(struct isp_device *isp, u32 xclk, u8 xclksel);
-	int (*csiphy_config)(struct isp_csiphy *phy,
-			     struct isp_csiphy_dphy_cfg *dphy,
-			     struct isp_csiphy_lanes_cfg *lanes);
-	void (*set_pixel_clock)(struct isp_device *isp, unsigned int pixelclk);
 };
 
 /*

(quoted diff) +0 -0

b/drivers/media/video/omap3isp/ispcsi2.c index 0c5f1cb..0b3e705 100644

drivers/media/video/omap3isp/ispcsi2.c +0 -0

@@ -1055,7 +1055,45 @@ static int csi2_set_stream(struct v4l2_subdev *sd,
 int enable) struct isp_video *video_out = &csi2->video_out;

Quoted 10 lines

drivers/media/video/omap3isp/ispcsi2.c static int csi2_set_stream(struct v4l2_subdev *sd, int enable) · line 1060
Quoted 5 lines
drivers/media/video/omap3isp/ispcsi2.c +3 -1
 	switch (enable) {
-	case ISP_PIPELINE_STREAM_CONTINUOUS:
+	case ISP_PIPELINE_STREAM_CONTINUOUS: {
+		struct media_pad *remote_pad =
+			media_entity_remote_source(&sd->entity.pads[0]);
As you will need to locate the sensor in ccdc_config_vp() as well, you should store a pointer to the sensor in the pipeline structure in isp_video_streamon(). I think I've sent you code that does just that, originally written by Stan if my memory is correct.

Stan's patch assumed all external subdevs will be sensors. That may not be true in all possible cases. That's not the fault of the patch, things have changed after it has been written.

I'd be suspicious calling the SMIA++ sensor's scaler subdev a sensor. Definitely an external ISP will not be a sensor.

The check I'm doing is that if something's connected to either ccp2 or csi2 (and they're subdevs!) it's the external subdev we're interested in. Same for ccdc if the entity is neither of the csi2 receivers or the ccp2 receiver.

Quoted 18 lines

drivers/media/video/omap3isp/ispcsi2.c +18 -0

+		struct v4l2_subdev *remote_subdev =
+			media_entity_to_v4l2_subdev(remote_pad->entity);
+		struct v4l2_subdev_format fmt;
+		struct v4l2_ext_controls ctrls;
+		struct v4l2_ext_control ctrl;
+		int ret;
+
+		fmt.pad = remote_pad->index;
+		fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+		ret = v4l2_subdev_call(
+			remote_subdev, pad, get_fmt, NULL, &fmt);
+		if (ret < 0)
+			return -EPIPE;
+
+		memset(&ctrls, 0, sizeof(ctrls));
+		memset(&ctrl, 0, sizeof(ctrl));
+
+		ctrl.id = V4L2_CID_IMAGE_SOURCE_PIXEL_RATE;

Quoted 8 lines

drivers/media/video/omap3isp/ispcsi2.c static int csi2_set_stream(struct v4l2_subdev *sd, int enable) · line 1084
Quoted 6 lines
drivers/media/video/omap3isp/ispcsi2.c +6 -0
+
+		ctrls.ctrl_class = V4L2_CTRL_ID2CLASS(ctrl.id);
+		ctrls.count = 1;
+		ctrls.controls = &ctrl;
+
+		ret = v4l2_g_ext_ctrls(remote_subdev->ctrl_handler, &ctrls);
Wouldn't v4l2_g_ctrl be easier ?

Yes, but it's a 64-bit control so I must use extended controls.

Quoted 2 lines

An option to avoid duplicating this code in ccdc_config_vp() would be to move pixel rate retrieval to isp_video_streamon().

It's gathered during link validation now.

Quoted 32 lines

drivers/media/video/omap3isp/ispcsi2.c +14 -0

+		if (ret < 0) {
+			dev_warn(isp->dev,
+				 "no pixel rate control in subdev %s\n",
+				 remote_subdev->name);
+			return -EPIPE;
+		}
+
+		ret = omap3isp_csiphy_config(
+			isp, sd, remote_subdev, &fmt.format, ctrl.value64);
+		if (ret < 0)
+			return -EPIPE;
+
 		if (omap3isp_csiphy_acquire(csi2->phy) < 0)
 			return -ENODEV;
 		csi2->use_fs_irq = pipe->do_propagation;

@@ -1080,6 +1118,8 @@ static int csi2_set_stream(struct v4l2_subdev *sd,
 int enable) isp_video_dmaqueue_flags_clr(video_out);
 		break;
 
+	}
+
 	case ISP_PIPELINE_STREAM_STOPPED:
 		if (csi2->state == ISP_PIPELINE_STREAM_STOPPED)
 			return 0;

(quoted diff) +0 -0

b/drivers/media/video/omap3isp/ispcsiphy.c index 5be37ce..f286a01 100644

drivers/media/video/omap3isp/ispcsiphy.c +0 -0

@@ -28,6 +28,8 @@
 #include <linux/device.h>
 #include <linux/regulator/consumer.h>

Quoted 4 lines

drivers/media/video/omap3isp/ispcsiphy.c line 32
Quoted 2 lines
drivers/media/video/omap3isp/ispcsiphy.c +2 -0
+#include "../../../../arch/arm/mach-omap2/control.h"
+
Still no solution for this ?

Not yet. I'll have to see the discussion again to say more on that. I think we can find _something_ but it might not be more compatible with the device tree than this.

Quoted 31 lines

drivers/media/video/omap3isp/ispcsiphy.c +17 -3

 #include "isp.h"
 #include "ispreg.h"
 #include "ispcsiphy.h"

@@ -138,15 +140,79 @@ static void csiphy_dphy_config(struct isp_csiphy
 *phy) isp_reg_writel(phy->isp, reg, phy->phy_regs, ISPCSIPHY_REG1);
 }
 
-static int csiphy_config(struct isp_csiphy *phy,
-			 struct isp_csiphy_dphy_cfg *dphy,
-			 struct isp_csiphy_lanes_cfg *lanes)
+/*
+ * TCLK values are OK at their reset values
+ */
+#define TCLK_TERM	0
+#define TCLK_MISS	1
+#define TCLK_SETTLE	14
+
+int omap3isp_csiphy_config(struct isp_device *isp,
+			   struct v4l2_subdev *csi2_subdev,
+			   struct v4l2_subdev *sensor,
+			   struct v4l2_mbus_framefmt *sensor_fmt,
+			   uint32_t pixel_rate)
 {
+	struct isp_v4l2_subdevs_group *subdevs = sensor->host_priv;
+	struct isp_csi2_device *csi2 = v4l2_get_subdevdata(csi2_subdev);
+	struct isp_csiphy_dphy_cfg csi2phy;
+	int csi2_ddrclk_khz;
+	struct isp_csiphy_lanes_cfg *lanes;
 	unsigned int used_lanes = 0;
 	unsigned int i;

Quoted 9 lines

drivers/media/video/omap3isp/ispcsiphy.c static void csiphy_dphy_config(struct isp_csiphy *phy) · line 170
Quoted 7 lines
drivers/media/video/omap3isp/ispcsiphy.c +7 -0
+	if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY1
+	    || subdevs->interface == ISP_INTERFACE_CCP2B_PHY2)
+		lanes = &subdevs->bus.ccp2.lanecfg;
+	else
+		lanes = &subdevs->bus.csi2.lanecfg;
+
+	if (!lanes) {
This can't happen.

Fixed.

Cheers,

Signature · 2 lines

Sakari Ailus
e-mail: [email protected]	jabber/XMPP/Gmail: [email protected]

Laurent Pinchart

maintainer

2012-01-19 16:16 UTC #29 lore raw

Re: [PATCH 14/23] omap3isp: Configure CSI-2 phy based on platform data

Hi Sakari,

Quoted 130 lines · On Tuesday 17 January 2012 20:45:18 Sakari Ailus wrote:

Quoted 50 lines · Laurent Pinchart
Quoted 49 lines · On Wednesday 11 January 2012 22:26:51 Sakari Ailus wrote:
Configure CSI-2 phy based on platform data in the ISP driver. For that, the new V4L2_CID_IMAGE_SOURCE_PIXEL_RATE control is used. Previously the same was configured from the board code.
Signed-off-by Sakari Ailus <[email protected]>
---
 drivers/media/video/omap3isp/isp.c       |   29 ++++++++++-
 drivers/media/video/omap3isp/isp.h       |    4 --
 drivers/media/video/omap3isp/ispcsi2.c   |   42 ++++++++++++++-
 drivers/media/video/omap3isp/ispcsiphy.c |   84
++++++++++++++++++++++++++---- drivers/media/video/omap3isp/ispcsiphy.h |
5 ++
 5 files changed, 148 insertions(+), 16 deletions(-)
(quoted diff) +0 -0
b/drivers/media/video/omap3isp/isp.c index b818cac..d268d55 100644
drivers/media/video/omap3isp/isp.c +21 -0
@@ -765,6 +765,34 @@ static int isp_pipeline_enable(struct isp_pipeline
 *pipe, if (ret < 0 && ret != -ENOIOCTLCMD)
 
 			return ret;
 
+		/*
+		 * Configure CCDC pixel clock. host_priv != NULL so
+		 * this one is a sensor.
+		 */
+		if (subdev->host_priv) {
+			struct v4l2_ext_controls ctrls;
+			struct v4l2_ext_control ctrl;
+
+			memset(&ctrls, 0, sizeof(ctrls));
+			memset(&ctrl, 0, sizeof(ctrl));
+
+			ctrl.id = V4L2_CID_IMAGE_SOURCE_PIXEL_RATE;
+
+			ctrls.ctrl_class = V4L2_CTRL_ID2CLASS(ctrl.id);
+			ctrls.count = 1;
+			ctrls.controls = &ctrl;
+
+			ret = v4l2_g_ext_ctrls(subdev->ctrl_handler, &ctrls);
+			if (ret < 0) {
+				dev_warn(isp->dev,
+					 "no pixel rate control in subdev %s\n",
Quoted 8 lines
drivers/media/video/omap3isp/isp.c static int isp_pipeline_enable(struct isp_pipeline *pipe, · line 793
Quoted 5 lines
drivers/media/video/omap3isp/isp.c +5 -0
+					 subdev->name);
+				return -EPIPE;
+			}
+
+			isp_set_pixel_clock(isp, ctrl.value64);
Isn't this too late ? The CCDC has already been started. What about moving this code to ccdc_config_vp() ?
I think you're right. I've mostly rewritten this part of the patchset and I think it's much cleaner now.
I have validate_link() functions where I'm validating the link and also gathering information alongside that process into a few fields in isp_pipeline struct.
Then those fields can be used in streamon time. Some additional checks for those parameters are also done there and they may result into an error which is then handled.
Quoted 49 lines
Quoted 49 lines
drivers/media/video/omap3isp/isp.c +2 -0
+		}
+
 
 		if (subdev == &isp->isp_ccdc.subdev) {
drivers/media/video/omap3isp/isp.c +0 -1
 			v4l2_subdev_call(&isp->isp_aewb.subdev, video,
 
 					s_stream, mode);
@@ -2072,7 +2100,6 @@ static int isp_probe(struct platform_device
 *pdev)
 
 	isp->autoidle = autoidle;
 	isp->platform_cb.set_xclk = isp_set_xclk;
 
-	isp->platform_cb.set_pixel_clock = isp_set_pixel_clock;
 
drivers/media/video/omap3isp/isp.c +0 -0
 	mutex_init(&isp->isp_mutex);
 	spin_lock_init(&isp->stat_lock);
(quoted diff) +0 -0
b/drivers/media/video/omap3isp/isp.h index ff1c422..dd1b61e 100644
drivers/media/video/omap3isp/isp.h +0 -4
@@ -126,10 +126,6 @@ struct isp_reg {
 
 struct isp_platform_callback {
 
 	u32 (*set_xclk)(struct isp_device *isp, u32 xclk, u8 xclksel);
 
-	int (*csiphy_config)(struct isp_csiphy *phy,
-			     struct isp_csiphy_dphy_cfg *dphy,
-			     struct isp_csiphy_lanes_cfg *lanes);
-	void (*set_pixel_clock)(struct isp_device *isp, unsigned int
 pixelclk);
};
/*
(quoted diff) +0 -0
b/drivers/media/video/omap3isp/ispcsi2.c index 0c5f1cb..0b3e705 100644
drivers/media/video/omap3isp/ispcsi2.c +0 -0
@@ -1055,7 +1055,45 @@ static int csi2_set_stream(struct v4l2_subdev
 *sd, int enable) struct isp_video *video_out = &csi2->video_out;
 
 	switch (enable) {
 
Quoted 9 lines
drivers/media/video/omap3isp/ispcsi2.c static int csi2_set_stream(struct v4l2_subdev *sd, int enable) · line 1060
Quoted 4 lines
drivers/media/video/omap3isp/ispcsi2.c +3 -1
-	case ISP_PIPELINE_STREAM_CONTINUOUS:
+	case ISP_PIPELINE_STREAM_CONTINUOUS: {
+		struct media_pad *remote_pad =
+			media_entity_remote_source(&sd->entity.pads[0]);
As you will need to locate the sensor in ccdc_config_vp() as well, you should store a pointer to the sensor in the pipeline structure in isp_video_streamon(). I think I've sent you code that does just that, originally written by Stan if my memory is correct.

Quoted 6 lines

Stan's patch assumed all external subdevs will be sensors. That may not be true in all possible cases. That's not the fault of the patch, things have changed after it has been written.
I'd be suspicious calling the SMIA++ sensor's scaler subdev a sensor. Definitely an external ISP will not be a sensor.

Then just rename the field to something else than sensor :-) We need to access the external subdev in various places in the driver, instead of trying to locate it every time, it would be better to locate it once in isp_video_streamon (or one of the helper functions it calls) and store the pointer in the isp_pipeline structure.

Quoted 32 lines

The check I'm doing is that if something's connected to either ccp2 or csi2 (and they're subdevs!) it's the external subdev we're interested in. Same for ccdc if the entity is neither of the csi2 receivers or the ccp2 receiver.
Quoted 18 lines
Quoted 18 lines
drivers/media/video/omap3isp/ispcsi2.c +18 -0
+		struct v4l2_subdev *remote_subdev =
+			media_entity_to_v4l2_subdev(remote_pad->entity);
+		struct v4l2_subdev_format fmt;
+		struct v4l2_ext_controls ctrls;
+		struct v4l2_ext_control ctrl;
+		int ret;
+
+		fmt.pad = remote_pad->index;
+		fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+		ret = v4l2_subdev_call(
+			remote_subdev, pad, get_fmt, NULL, &fmt);
+		if (ret < 0)
+			return -EPIPE;
+
+		memset(&ctrls, 0, sizeof(ctrls));
+		memset(&ctrl, 0, sizeof(ctrl));
+
+		ctrl.id = V4L2_CID_IMAGE_SOURCE_PIXEL_RATE;
Quoted 8 lines
drivers/media/video/omap3isp/ispcsi2.c static int csi2_set_stream(struct v4l2_subdev *sd, int enable) · line 1084
Quoted 6 lines
drivers/media/video/omap3isp/ispcsi2.c +6 -0
+
+		ctrls.ctrl_class = V4L2_CTRL_ID2CLASS(ctrl.id);
+		ctrls.count = 1;
+		ctrls.controls = &ctrl;
+
+		ret = v4l2_g_ext_ctrls(remote_subdev->ctrl_handler, &ctrls);
Wouldn't v4l2_g_ctrl be easier ?

Quoted 1 line

Yes, but it's a 64-bit control so I must use extended controls.

Right.

Quoted 46 lines

Quoted 2 lines
An option to avoid duplicating this code in ccdc_config_vp() would be to move pixel rate retrieval to isp_video_streamon().
It's gathered during link validation now.
Quoted 41 lines
Quoted 41 lines
drivers/media/video/omap3isp/ispcsi2.c +12 -0
+		if (ret < 0) {
+			dev_warn(isp->dev,
+				 "no pixel rate control in subdev %s\n",
+				 remote_subdev->name);
+			return -EPIPE;
+		}
+
+		ret = omap3isp_csiphy_config(
+			isp, sd, remote_subdev, &fmt.format, ctrl.value64);
+		if (ret < 0)
+			return -EPIPE;
+
 
 		if (omap3isp_csiphy_acquire(csi2->phy) < 0)
drivers/media/video/omap3isp/ispcsi2.c +2 -0
 			return -ENODEV;
 
 		csi2->use_fs_irq = pipe->do_propagation;
@@ -1080,6 +1118,8 @@ static int csi2_set_stream(struct v4l2_subdev
 *sd, int enable) isp_video_dmaqueue_flags_clr(video_out);
 
 		break;
 
+	}
+
 
 	case ISP_PIPELINE_STREAM_STOPPED:
drivers/media/video/omap3isp/ispcsi2.c +0 -0
 		if (csi2->state == ISP_PIPELINE_STREAM_STOPPED)
 
 			return 0;
(quoted diff) +0 -0
b/drivers/media/video/omap3isp/ispcsiphy.c index 5be37ce..f286a01 100644 --- a/drivers/media/video/omap3isp/ispcsiphy.c +++ b/drivers/media/video/omap3isp/ispcsiphy.c
drivers/media/video/omap3isp/ispcsiphy.c +0 -0
@@ -28,6 +28,8 @@
 
 #include <linux/device.h>
 #include <linux/regulator/consumer.h>
 

Quoted 8 lines

Quoted 4 lines
drivers/media/video/omap3isp/ispcsiphy.c line 32
Quoted 2 lines
drivers/media/video/omap3isp/ispcsiphy.c +2 -0
+#include "../../../../arch/arm/mach-omap2/control.h"
+
Still no solution for this ?
Not yet. I'll have to see the discussion again to say more on that. I think we can find _something_ but it might not be more compatible with the device tree than this.

I proposed adding an exported function to arch/arm/mach-omap2/control.c to access the field, but there was no reply.

Quoted 47 lines

Quoted 36 lines

drivers/media/video/omap3isp/ispcsiphy.c +0 -0

 #include "isp.h"
 #include "ispreg.h"

#include "ispcsiphy.h"

drivers/media/video/omap3isp/ispcsiphy.c +17 -3

@@ -138,15 +140,79 @@ static void csiphy_dphy_config(struct isp_csiphy
 *phy) isp_reg_writel(phy->isp, reg, phy->phy_regs, ISPCSIPHY_REG1);
 
 }
 
-static int csiphy_config(struct isp_csiphy *phy,
-			 struct isp_csiphy_dphy_cfg *dphy,
-			 struct isp_csiphy_lanes_cfg *lanes)
+/*
+ * TCLK values are OK at their reset values
+ */
+#define TCLK_TERM	0
+#define TCLK_MISS	1
+#define TCLK_SETTLE	14
+
+int omap3isp_csiphy_config(struct isp_device *isp,
+			   struct v4l2_subdev *csi2_subdev,
+			   struct v4l2_subdev *sensor,
+			   struct v4l2_mbus_framefmt *sensor_fmt,
+			   uint32_t pixel_rate)
 
 {
 
+	struct isp_v4l2_subdevs_group *subdevs = sensor->host_priv;
+	struct isp_csi2_device *csi2 = v4l2_get_subdevdata(csi2_subdev);
+	struct isp_csiphy_dphy_cfg csi2phy;
+	int csi2_ddrclk_khz;
+	struct isp_csiphy_lanes_cfg *lanes;
 
 	unsigned int used_lanes = 0;
 	unsigned int i;

Quoted 9 lines

drivers/media/video/omap3isp/ispcsiphy.c static void csiphy_dphy_config(struct isp_csiphy *phy) · line 170
Quoted 7 lines
drivers/media/video/omap3isp/ispcsiphy.c +7 -0
+	if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY1
+	    || subdevs->interface == ISP_INTERFACE_CCP2B_PHY2)
+		lanes = &subdevs->bus.ccp2.lanecfg;
+	else
+		lanes = &subdevs->bus.csi2.lanecfg;
+
+	if (!lanes) {
This can't happen.

Fixed.

Signature · 3 lines

Regards,

Laurent Pinchart

Sakari Ailus

author

2012-01-19 19:11 UTC #30 lore raw

Re: [PATCH 14/23] omap3isp: Configure CSI-2 phy based on platform data

Hi Laurent,

Quoted 135 lines · Laurent Pinchart

Quoted 134 lines · On Tuesday 17 January 2012 20:45:18 Sakari Ailus wrote:
Quoted 50 lines · Laurent Pinchart
Quoted 49 lines · On Wednesday 11 January 2012 22:26:51 Sakari Ailus wrote:
Configure CSI-2 phy based on platform data in the ISP driver. For that, the new V4L2_CID_IMAGE_SOURCE_PIXEL_RATE control is used. Previously the same was configured from the board code.
Signed-off-by Sakari Ailus <[email protected]>
---
 drivers/media/video/omap3isp/isp.c       |   29 ++++++++++-
 drivers/media/video/omap3isp/isp.h       |    4 --
 drivers/media/video/omap3isp/ispcsi2.c   |   42 ++++++++++++++-
 drivers/media/video/omap3isp/ispcsiphy.c |   84
++++++++++++++++++++++++++---- drivers/media/video/omap3isp/ispcsiphy.h |
5 ++
 5 files changed, 148 insertions(+), 16 deletions(-)
(quoted diff) +0 -0
b/drivers/media/video/omap3isp/isp.c index b818cac..d268d55 100644
drivers/media/video/omap3isp/isp.c +21 -0
@@ -765,6 +765,34 @@ static int isp_pipeline_enable(struct isp_pipeline
 *pipe, if (ret < 0 && ret != -ENOIOCTLCMD)
 
 			return ret;
 
+		/*
+		 * Configure CCDC pixel clock. host_priv != NULL so
+		 * this one is a sensor.
+		 */
+		if (subdev->host_priv) {
+			struct v4l2_ext_controls ctrls;
+			struct v4l2_ext_control ctrl;
+
+			memset(&ctrls, 0, sizeof(ctrls));
+			memset(&ctrl, 0, sizeof(ctrl));
+
+			ctrl.id = V4L2_CID_IMAGE_SOURCE_PIXEL_RATE;
+
+			ctrls.ctrl_class = V4L2_CTRL_ID2CLASS(ctrl.id);
+			ctrls.count = 1;
+			ctrls.controls = &ctrl;
+
+			ret = v4l2_g_ext_ctrls(subdev->ctrl_handler, &ctrls);
+			if (ret < 0) {
+				dev_warn(isp->dev,
+					 "no pixel rate control in subdev %s\n",
Quoted 8 lines
drivers/media/video/omap3isp/isp.c static int isp_pipeline_enable(struct isp_pipeline *pipe, · line 793
Quoted 5 lines
drivers/media/video/omap3isp/isp.c +5 -0
+					 subdev->name);
+				return -EPIPE;
+			}
+
+			isp_set_pixel_clock(isp, ctrl.value64);
Isn't this too late ? The CCDC has already been started. What about moving this code to ccdc_config_vp() ?
I think you're right. I've mostly rewritten this part of the patchset and I think it's much cleaner now.
I have validate_link() functions where I'm validating the link and also gathering information alongside that process into a few fields in isp_pipeline struct.
Then those fields can be used in streamon time. Some additional checks for those parameters are also done there and they may result into an error which is then handled.
Quoted 49 lines
Quoted 49 lines
drivers/media/video/omap3isp/isp.c +2 -0
+		}
+
 
 		if (subdev == &isp->isp_ccdc.subdev) {
drivers/media/video/omap3isp/isp.c +0 -1
 			v4l2_subdev_call(&isp->isp_aewb.subdev, video,
 
 					s_stream, mode);
@@ -2072,7 +2100,6 @@ static int isp_probe(struct platform_device
 *pdev)
 
 	isp->autoidle = autoidle;
 	isp->platform_cb.set_xclk = isp_set_xclk;
 
-	isp->platform_cb.set_pixel_clock = isp_set_pixel_clock;
 
drivers/media/video/omap3isp/isp.c +0 -0
 	mutex_init(&isp->isp_mutex);
 	spin_lock_init(&isp->stat_lock);
(quoted diff) +0 -0
b/drivers/media/video/omap3isp/isp.h index ff1c422..dd1b61e 100644
drivers/media/video/omap3isp/isp.h +0 -4
@@ -126,10 +126,6 @@ struct isp_reg {
 
 struct isp_platform_callback {
 
 	u32 (*set_xclk)(struct isp_device *isp, u32 xclk, u8 xclksel);
 
-	int (*csiphy_config)(struct isp_csiphy *phy,
-			     struct isp_csiphy_dphy_cfg *dphy,
-			     struct isp_csiphy_lanes_cfg *lanes);
-	void (*set_pixel_clock)(struct isp_device *isp, unsigned int
 pixelclk);
};
/*
(quoted diff) +0 -0
b/drivers/media/video/omap3isp/ispcsi2.c index 0c5f1cb..0b3e705 100644
drivers/media/video/omap3isp/ispcsi2.c +0 -0
@@ -1055,7 +1055,45 @@ static int csi2_set_stream(struct v4l2_subdev
 *sd, int enable) struct isp_video *video_out = &csi2->video_out;
 
 	switch (enable) {
 
Quoted 9 lines
drivers/media/video/omap3isp/ispcsi2.c static int csi2_set_stream(struct v4l2_subdev *sd, int enable) · line 1060
Quoted 4 lines
drivers/media/video/omap3isp/ispcsi2.c +3 -1
-	case ISP_PIPELINE_STREAM_CONTINUOUS:
+	case ISP_PIPELINE_STREAM_CONTINUOUS: {
+		struct media_pad *remote_pad =
+			media_entity_remote_source(&sd->entity.pads[0]);
As you will need to locate the sensor in ccdc_config_vp() as well, you should store a pointer to the sensor in the pipeline structure in isp_video_streamon(). I think I've sent you code that does just that, originally written by Stan if my memory is correct.
Stan's patch assumed all external subdevs will be sensors. That may not be true in all possible cases. That's not the fault of the patch, things have changed after it has been written.

Quoted 8 lines

Quoted 2 lines
I'd be suspicious calling the SMIA++ sensor's scaler subdev a sensor. Definitely an external ISP will not be a sensor.
Then just rename the field to something else than sensor :-) We need to access the external subdev in various places in the driver, instead of trying to locate it every time, it would be better to locate it once in isp_video_streamon (or one of the helper functions it calls) and store the pointer in the isp_pipeline structure.

My point was that Stan's patch located the subdev based on its type. We can't rely on that. But yes, I'll store the information to isp_pipeline. I'm not even sure the external subdev has to be there, just some information related to it.

Quoted 88 lines

Quoted 33 lines
The check I'm doing is that if something's connected to either ccp2 or csi2 (and they're subdevs!) it's the external subdev we're interested in. Same for ccdc if the entity is neither of the csi2 receivers or the ccp2 receiver.
Quoted 18 lines
Quoted 18 lines
drivers/media/video/omap3isp/ispcsi2.c +18 -0
+		struct v4l2_subdev *remote_subdev =
+			media_entity_to_v4l2_subdev(remote_pad->entity);
+		struct v4l2_subdev_format fmt;
+		struct v4l2_ext_controls ctrls;
+		struct v4l2_ext_control ctrl;
+		int ret;
+
+		fmt.pad = remote_pad->index;
+		fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+		ret = v4l2_subdev_call(
+			remote_subdev, pad, get_fmt, NULL, &fmt);
+		if (ret < 0)
+			return -EPIPE;
+
+		memset(&ctrls, 0, sizeof(ctrls));
+		memset(&ctrl, 0, sizeof(ctrl));
+
+		ctrl.id = V4L2_CID_IMAGE_SOURCE_PIXEL_RATE;
Quoted 8 lines
drivers/media/video/omap3isp/ispcsi2.c static int csi2_set_stream(struct v4l2_subdev *sd, int enable) · line 1084
Quoted 6 lines
drivers/media/video/omap3isp/ispcsi2.c +6 -0
+
+		ctrls.ctrl_class = V4L2_CTRL_ID2CLASS(ctrl.id);
+		ctrls.count = 1;
+		ctrls.controls = &ctrl;
+
+		ret = v4l2_g_ext_ctrls(remote_subdev->ctrl_handler, &ctrls);
Wouldn't v4l2_g_ctrl be easier ?
Quoted 1 line
Yes, but it's a 64-bit control so I must use extended controls.
Right.
Quoted 51 lines
Quoted 2 lines
An option to avoid duplicating this code in ccdc_config_vp() would be to move pixel rate retrieval to isp_video_streamon().
It's gathered during link validation now.
Quoted 45 lines
Quoted 37 lines
drivers/media/video/omap3isp/ispcsi2.c +12 -0
+		if (ret < 0) {
+			dev_warn(isp->dev,
+				 "no pixel rate control in subdev %s\n",
+				 remote_subdev->name);
+			return -EPIPE;
+		}
+
+		ret = omap3isp_csiphy_config(
+			isp, sd, remote_subdev, &fmt.format, ctrl.value64);
+		if (ret < 0)
+			return -EPIPE;
+
 
 		if (omap3isp_csiphy_acquire(csi2->phy) < 0)
drivers/media/video/omap3isp/ispcsi2.c +2 -0
 			return -ENODEV;
 
 		csi2->use_fs_irq = pipe->do_propagation;
@@ -1080,6 +1118,8 @@ static int csi2_set_stream(struct v4l2_subdev
 *sd, int enable) isp_video_dmaqueue_flags_clr(video_out);
 
 		break;
 
+	}
+
 
 	case ISP_PIPELINE_STREAM_STOPPED:
drivers/media/video/omap3isp/ispcsi2.c +0 -0
 		if (csi2->state == ISP_PIPELINE_STREAM_STOPPED)
 
 			return 0;
(quoted diff) +0 -0
b/drivers/media/video/omap3isp/ispcsiphy.c index 5be37ce..f286a01 100644 --- a/drivers/media/video/omap3isp/ispcsiphy.c +++ b/drivers/media/video/omap3isp/ispcsiphy.c
drivers/media/video/omap3isp/ispcsiphy.c line 33
Quoted 6 lines
drivers/media/video/omap3isp/ispcsiphy.c +2 -0
@@ -28,6 +28,8 @@
 
 #include <linux/device.h>
 #include <linux/regulator/consumer.h>
 
+#include "../../../../arch/arm/mach-omap2/control.h"
+
Still no solution for this ?

Quoted 6 lines

Quoted 3 lines
Not yet. I'll have to see the discussion again to say more on that. I think we can find _something_ but it might not be more compatible with the device tree than this.
I proposed adding an exported function to arch/arm/mach-omap2/control.c to access the field, but there was no reply.

I remember that. I think it's a better solution but not ideal. We also must provide the function on all platforms the OMAP 3 ISP driver is to be used, i.e. OMAP 3s. On 3[45]xx it just wouldn't do anything.

Quoted 46 lines

Quoted 45 lines

Quoted 33 lines

drivers/media/video/omap3isp/ispcsiphy.c +0 -0

 #include "isp.h"
 #include "ispreg.h"

#include "ispcsiphy.h"

drivers/media/video/omap3isp/ispcsiphy.c +17 -3

@@ -138,15 +140,79 @@ static void csiphy_dphy_config(struct isp_csiphy
 *phy) isp_reg_writel(phy->isp, reg, phy->phy_regs, ISPCSIPHY_REG1);
 
 }
 
-static int csiphy_config(struct isp_csiphy *phy,
-			 struct isp_csiphy_dphy_cfg *dphy,
-			 struct isp_csiphy_lanes_cfg *lanes)
+/*
+ * TCLK values are OK at their reset values
+ */
+#define TCLK_TERM	0
+#define TCLK_MISS	1
+#define TCLK_SETTLE	14
+
+int omap3isp_csiphy_config(struct isp_device *isp,
+			   struct v4l2_subdev *csi2_subdev,
+			   struct v4l2_subdev *sensor,
+			   struct v4l2_mbus_framefmt *sensor_fmt,
+			   uint32_t pixel_rate)
 
 {
 
+	struct isp_v4l2_subdevs_group *subdevs = sensor->host_priv;
+	struct isp_csi2_device *csi2 = v4l2_get_subdevdata(csi2_subdev);
+	struct isp_csiphy_dphy_cfg csi2phy;
+	int csi2_ddrclk_khz;
+	struct isp_csiphy_lanes_cfg *lanes;

drivers/media/video/omap3isp/ispcsiphy.c static void csiphy_dphy_config(struct isp_csiphy · line 174

Quoted 10 lines

drivers/media/video/omap3isp/ispcsiphy.c +7 -0

 	unsigned int used_lanes = 0;
 	unsigned int i;
 
+	if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY1
+	    || subdevs->interface == ISP_INTERFACE_CCP2B_PHY2)
+		lanes = &subdevs->bus.ccp2.lanecfg;
+	else
+		lanes = &subdevs->bus.csi2.lanecfg;
+
+	if (!lanes) {

This can't happen.

Quoted 1 line

Quoted 1 line
Fixed.

Cheers,

Signature · 2 lines

Sakari Ailus
e-mail: [email protected]	jabber/XMPP/Gmail: [email protected]

Sakari Ailus

author

2012-01-11 21:27 UTC #31 lore raw

PATCH 15/23 omap3isp: Do not attempt to walk the pipeline outside the ISP

In reply to Sakari Ailus

Do not attempt to walk the pipeline outside of the ISP. The external subdevs will handle this internally.

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/video/omap3isp/isp.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

drivers/media/video/omap3isp/isp.c +3 -1

index d268d55..3338176 100644
@@ -767,7 +767,8 @@ static int isp_pipeline_enable(struct isp_pipeline *pipe,
 
 		/*
 		 * Configure CCDC pixel clock. host_priv != NULL so
-		 * this one is a sensor.
+		 * this one is a sensor. We may also quit now since we
+		 * wouldn't encounter more ISP subdevs anymore.
 		 */
 		if (subdev->host_priv) {
 			struct v4l2_ext_controls ctrls;

@@ -791,6 +792,7 @@ static int isp_pipeline_enable(struct isp_pipeline *pipe,
 			}
 
 			isp_set_pixel_clock(isp, ctrl.value64);
+			break;
 		}
 
 		if (subdev == &isp->isp_ccdc.subdev) {

Signature · 1 line

1.7.2.5

Sakari Ailus

author

2012-01-11 21:27 UTC #32 lore raw

PATCH 16/23 media: Add link_validate op to check links to the sink pad

In reply to Sakari Ailus

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/media-entity.c |   73 ++++++++++++++++++++++++++++++++++++++++-
 include/media/media-entity.h |    5 ++-
 2 files changed, 74 insertions(+), 4 deletions(-)

drivers/media/media-entity.c +71 -2

index 056138f..62ef4b8 100644
@@ -196,6 +196,35 @@ media_entity_graph_walk_next(struct media_entity_graph *graph)
 }
 EXPORT_SYMBOL_GPL(media_entity_graph_walk_next);
 
+struct media_link_enum {
+	int i;
+	struct media_entity *entity;
+	unsigned long flags, mask;
+};
+
+static struct media_link
+*media_link_walk_next(struct media_link_enum *link_enum)
+{
+	do {
+		link_enum->i++;
+		if (link_enum->i >= link_enum->entity->num_links)
+			return NULL;
+	} while ((link_enum->entity->links[link_enum->i].flags
+		  & link_enum->mask) != link_enum->flags);
+
+	return &link_enum->entity->links[link_enum->i];
+}
+
+static void media_link_walk_start(struct media_link_enum *link_enum,
+				  struct media_entity *entity,
+				  unsigned long flags, unsigned long mask)
+{
+	link_enum->i = -1;
+	link_enum->entity = entity;
+	link_enum->flags = flags;
+	link_enum->mask = mask;
+}
+
 /* -----------------------------------------------------------------------------
  * Pipeline management
  */

@@ -214,23 +243,63 @@ EXPORT_SYMBOL_GPL(media_entity_graph_walk_next);
  * pipeline pointer must be identical for all nested calls to
  * media_entity_pipeline_start().
  */
-void media_entity_pipeline_start(struct media_entity *entity,
-				 struct media_pipeline *pipe)
+__must_check int media_entity_pipeline_start(struct media_entity *entity,
+					     struct media_pipeline *pipe)
 {
 	struct media_device *mdev = entity->parent;
 	struct media_entity_graph graph;
+	struct media_entity *tmp = entity;
+	int ret = 0;
 
 	mutex_lock(&mdev->graph_mutex);
 
 	media_entity_graph_walk_start(&graph, entity);
 
 	while ((entity = media_entity_graph_walk_next(&graph))) {
+		struct media_entity_graph tmp_graph;
+		struct media_link_enum link_enum;
+		struct media_link *link;
+
 		entity->stream_count++;
 		WARN_ON(entity->pipe && entity->pipe != pipe);
 		entity->pipe = pipe;
+
+		if (!entity->ops || !entity->ops->link_validate)
+			continue;
+
+		media_link_walk_start(&link_enum, entity,
+				      MEDIA_LNK_FL_ENABLED,
+				      MEDIA_LNK_FL_ENABLED);
+
+		while ((link = media_link_walk_next(&link_enum))) {
+			if (link->sink->entity != entity)
+				continue;
+
+			ret = entity->ops->link_validate(link);
+			if (ret < 0 && ret != -ENOIOCTLCMD)
+				break;
+		}
+		if (!ret || ret == -ENOIOCTLCMD)
+			continue;
+
+		/*
+		 * Link validation on graph failed. We revert what we
+		 * did and return the error.
+		 */
+		media_entity_graph_walk_start(&tmp_graph, tmp);
+		do {
+			tmp = media_entity_graph_walk_next(&tmp_graph);
+			tmp->stream_count--;
+			if (entity->stream_count == 0)
+				entity->pipe = NULL;
+		} while (tmp != entity);
+
+		break;
 	}
 
 	mutex_unlock(&mdev->graph_mutex);
+
+	return ret == 0 || ret == -ENOIOCTLCMD ? 0 : ret;
 }
 EXPORT_SYMBOL_GPL(media_entity_pipeline_start);

include/media/media-entity.h +3 -2

index cd8bca6..f7ba80a 100644
@@ -46,6 +46,7 @@ struct media_entity_operations {
 	int (*link_setup)(struct media_entity *entity,
 			  const struct media_pad *local,
 			  const struct media_pad *remote, u32 flags);
+	int (*link_validate)(struct media_link *link);
 };
 
 struct media_entity {

@@ -140,8 +141,8 @@ void media_entity_graph_walk_start(struct media_entity_graph *graph,
 		struct media_entity *entity);
 struct media_entity *
 media_entity_graph_walk_next(struct media_entity_graph *graph);
-void media_entity_pipeline_start(struct media_entity *entity,
-		struct media_pipeline *pipe);
+__must_check int media_entity_pipeline_start(struct media_entity *entity,
+					     struct media_pipeline *pipe);
 void media_entity_pipeline_stop(struct media_entity *entity);
 
 #define media_entity_call(entity, operation, args...)			\

Signature · 1 line

1.7.2.5

Laurent Pinchart

maintainer

2012-01-16 14:35 UTC #33 lore raw

Re: [PATCH 16/23] media: Add link_validate op to check links to the sink pad

Hi Sakari,

Thanks for the patch.

Quoted 33 lines · On Wednesday 11 January 2012 22:26:53 Sakari Ailus wrote:

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/media-entity.c |   73
++++++++++++++++++++++++++++++++++++++++- include/media/media-entity.h |  
 5 ++-
 2 files changed, 74 insertions(+), 4 deletions(-)

drivers/media/media-entity.c +18 -0

index 056138f..62ef4b8 100644
@@ -196,6 +196,35 @@ media_entity_graph_walk_next(struct media_entity_graph
 *graph) }
 EXPORT_SYMBOL_GPL(media_entity_graph_walk_next);
 
+struct media_link_enum {
+	int i;
+	struct media_entity *entity;
+	unsigned long flags, mask;
+};
+
+static struct media_link
+*media_link_walk_next(struct media_link_enum *link_enum)
+{
+	do {
+		link_enum->i++;
+		if (link_enum->i >= link_enum->entity->num_links)
+			return NULL;
+	} while ((link_enum->entity->links[link_enum->i].flags
+		  & link_enum->mask) != link_enum->flags);
+
+	return &link_enum->entity->links[link_enum->i];
+}

drivers/media/media-entity.c media_entity_graph_walk_next(struct media_entity_graph · line 226

Quoted 10 lines

drivers/media/media-entity.c +10 -0

+
+static void media_link_walk_start(struct media_link_enum *link_enum,
+				  struct media_entity *entity,
+				  unsigned long flags, unsigned long mask)
+{
+	link_enum->i = -1;
+	link_enum->entity = entity;
+	link_enum->flags = flags;
+	link_enum->mask = mask;
+}

Do we really need a generic link walking code for a single user ? Merging this in the function below would result in much simpler code.

Quoted 39 lines

drivers/media/media-entity.c +17 -4

+
 /*
--------------------------------------------------------------------------
--- * Pipeline management
  */

@@ -214,23 +243,63 @@ EXPORT_SYMBOL_GPL(media_entity_graph_walk_next);
  * pipeline pointer must be identical for all nested calls to
  * media_entity_pipeline_start().
  */
-void media_entity_pipeline_start(struct media_entity *entity,
-				 struct media_pipeline *pipe)
+__must_check int media_entity_pipeline_start(struct media_entity *entity,
+					     struct media_pipeline *pipe)
 {
 	struct media_device *mdev = entity->parent;
 	struct media_entity_graph graph;
+	struct media_entity *tmp = entity;
+	int ret = 0;
 
 	mutex_lock(&mdev->graph_mutex);
 
 	media_entity_graph_walk_start(&graph, entity);
 
 	while ((entity = media_entity_graph_walk_next(&graph))) {
+		struct media_entity_graph tmp_graph;
+		struct media_link_enum link_enum;
+		struct media_link *link;
+
 		entity->stream_count++;
 		WARN_ON(entity->pipe && entity->pipe != pipe);
 		entity->pipe = pipe;
+
+		if (!entity->ops || !entity->ops->link_validate)
+			continue;
+
+		media_link_walk_start(&link_enum, entity,
+				      MEDIA_LNK_FL_ENABLED,
+				      MEDIA_LNK_FL_ENABLED);
+

drivers/media/media-entity.c EXPORT_SYMBOL_GPL(media_entity_graph_walk_next); · line 283

Quoted 10 lines

drivers/media/media-entity.c +10 -0

+		while ((link = media_link_walk_next(&link_enum))) {
+			if (link->sink->entity != entity)
+				continue;
+
+			ret = entity->ops->link_validate(link);
+			if (ret < 0 && ret != -ENOIOCTLCMD)
+				break;
+		}
+		if (!ret || ret == -ENOIOCTLCMD)
+			continue;

What about a goto error instead ? That would keep the error code out of the loop.

drivers/media/media-entity.c EXPORT_SYMBOL_GPL(media_entity_graph_walk_next); · line 289

Quoted 6 lines

drivers/media/media-entity.c +6 -0

+
+		/*
+		 * Link validation on graph failed. We revert what we
+		 * did and return the error.
+		 */
+		media_entity_graph_walk_start(&tmp_graph, tmp);

I've never liked tmp as a variable name. As the graph variable isn't used anymore from this point on, you can reuse it. tmp can then be renamed to something more descriptive.

Quoted 21 lines

drivers/media/media-entity.c +10 -0

+		do {
+			tmp = media_entity_graph_walk_next(&tmp_graph);
+			tmp->stream_count--;
+			if (entity->stream_count == 0)
+				entity->pipe = NULL;
+		} while (tmp != entity);
+
+		break;
 	}
 
 	mutex_unlock(&mdev->graph_mutex);
+
+	return ret == 0 || ret == -ENOIOCTLCMD ? 0 : ret;
 }
 EXPORT_SYMBOL_GPL(media_entity_pipeline_start);

include/media/media-entity.h +0 -0

index cd8bca6..f7ba80a 100644

include/media/media-entity.h struct media_entity_operations { · line 49

Quoted 4 lines

include/media/media-entity.h +1 -0

@@ -46,6 +46,7 @@ struct media_entity_operations {
 	int (*link_setup)(struct media_entity *entity,
 			  const struct media_pad *local,
 			  const struct media_pad *remote, u32 flags);
+	int (*link_validate)(struct media_link *link);

What about documenting the operation in Documentation/media-framework.txt ?

include/media/media-entity.h void media_entity_graph_walk_start(struct · line 145

Quoted 11 lines

include/media/media-entity.h +2 -2

 };
 
 struct media_entity {

@@ -140,8 +141,8 @@ void media_entity_graph_walk_start(struct
 media_entity_graph *graph, struct media_entity *entity);
 struct media_entity *
 media_entity_graph_walk_next(struct media_entity_graph *graph);
-void media_entity_pipeline_start(struct media_entity *entity,
-		struct media_pipeline *pipe);
+__must_check int media_entity_pipeline_start(struct media_entity *entity,
+					     struct media_pipeline *pipe);

As well as keeping the media_entity_pipeline_start() documentation up-to-date in the same file :-)

Quoted 3 lines

include/media/media-entity.h +0 -0

 void media_entity_pipeline_stop(struct media_entity *entity);
 
 #define media_entity_call(entity, operation, args...)			\

Signature · 3 lines

Regards,

Laurent Pinchart

Sakari Ailus

author

2012-01-17 20:10 UTC #34 lore raw

Re: [PATCH 16/23] media: Add link_validate op to check links to the sink pad

Hi Laurent,

Thanks for the review.

Quoted 39 lines · Laurent Pinchart

Quoted 38 lines · On Wednesday 11 January 2012 22:26:53 Sakari Ailus wrote:

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/media-entity.c |   73
++++++++++++++++++++++++++++++++++++++++- include/media/media-entity.h |  
 5 ++-
 2 files changed, 74 insertions(+), 4 deletions(-)

drivers/media/media-entity.c +23 -0

index 056138f..62ef4b8 100644
@@ -196,6 +196,35 @@ media_entity_graph_walk_next(struct media_entity_graph
 *graph) }
 EXPORT_SYMBOL_GPL(media_entity_graph_walk_next);
 
+struct media_link_enum {
+	int i;
+	struct media_entity *entity;
+	unsigned long flags, mask;
+};
+
+static struct media_link
+*media_link_walk_next(struct media_link_enum *link_enum)
+{
+	do {
+		link_enum->i++;
+		if (link_enum->i >= link_enum->entity->num_links)
+			return NULL;
+	} while ((link_enum->entity->links[link_enum->i].flags
+		  & link_enum->mask) != link_enum->flags);
+
+	return &link_enum->entity->links[link_enum->i];
+}
+
+static void media_link_walk_start(struct media_link_enum *link_enum,
+				  struct media_entity *entity,
+				  unsigned long flags, unsigned long mask)
+{

Quoted 8 lines

drivers/media/media-entity.c media_entity_graph_walk_next(struct media_entity_graph *graph) · line 226
Quoted 5 lines
drivers/media/media-entity.c +5 -0
+	link_enum->i = -1;
+	link_enum->entity = entity;
+	link_enum->flags = flags;
+	link_enum->mask = mask;
+}
Do we really need a generic link walking code for a single user ? Merging this in the function below would result in much simpler code.

It's a single funcition but it's being used from two locations in it. I would keep it as-is, since performing the same in the function itself would much complicate it.

Quoted 44 lines

drivers/media/media-entity.c +22 -4

+
 /*
--------------------------------------------------------------------------
--- * Pipeline management
  */

@@ -214,23 +243,63 @@ EXPORT_SYMBOL_GPL(media_entity_graph_walk_next);
  * pipeline pointer must be identical for all nested calls to
  * media_entity_pipeline_start().
  */
-void media_entity_pipeline_start(struct media_entity *entity,
-				 struct media_pipeline *pipe)
+__must_check int media_entity_pipeline_start(struct media_entity *entity,
+					     struct media_pipeline *pipe)
 {
 	struct media_device *mdev = entity->parent;
 	struct media_entity_graph graph;
+	struct media_entity *tmp = entity;
+	int ret = 0;
 
 	mutex_lock(&mdev->graph_mutex);
 
 	media_entity_graph_walk_start(&graph, entity);
 
 	while ((entity = media_entity_graph_walk_next(&graph))) {
+		struct media_entity_graph tmp_graph;
+		struct media_link_enum link_enum;
+		struct media_link *link;
+
 		entity->stream_count++;
 		WARN_ON(entity->pipe && entity->pipe != pipe);
 		entity->pipe = pipe;
+
+		if (!entity->ops || !entity->ops->link_validate)
+			continue;
+
+		media_link_walk_start(&link_enum, entity,
+				      MEDIA_LNK_FL_ENABLED,
+				      MEDIA_LNK_FL_ENABLED);
+
+		while ((link = media_link_walk_next(&link_enum))) {
+			if (link->sink->entity != entity)
+				continue;
+
+			ret = entity->ops->link_validate(link);

Quoted 8 lines

drivers/media/media-entity.c EXPORT_SYMBOL_GPL(media_entity_graph_walk_next); · line 283
Quoted 5 lines
drivers/media/media-entity.c +5 -0
+			if (ret < 0 && ret != -ENOIOCTLCMD)
+				break;
+		}
+		if (!ret || ret == -ENOIOCTLCMD)
+			continue;
What about a goto error instead ? That would keep the error code out of the loop.

Fixed.

Quoted 10 lines

drivers/media/media-entity.c EXPORT_SYMBOL_GPL(media_entity_graph_walk_next); · line 289
Quoted 6 lines
drivers/media/media-entity.c +6 -0
+
+		/*
+		 * Link validation on graph failed. We revert what we
+		 * did and return the error.
+		 */
+		media_entity_graph_walk_start(&tmp_graph, tmp);
I've never liked tmp as a variable name. As the graph variable isn't used anymore from this point on, you can reuse it. tmp can then be renamed to something more descriptive.

I re-use graph, and tmp is called entity_err now.

Quoted 16 lines

drivers/media/media-entity.c +10 -0

+		do {
+			tmp = media_entity_graph_walk_next(&tmp_graph);
+			tmp->stream_count--;
+			if (entity->stream_count == 0)
+				entity->pipe = NULL;
+		} while (tmp != entity);
+
+		break;
 	}
 
 	mutex_unlock(&mdev->graph_mutex);
+
+	return ret == 0 || ret == -ENOIOCTLCMD ? 0 : ret;
 }
 EXPORT_SYMBOL_GPL(media_entity_pipeline_start);

Quoted 11 lines

include/media/media-entity.h struct media_entity_operations { · line 49
Quoted 9 lines
include/media/media-entity.h +1 -0
index cd8bca6..f7ba80a 100644
@@ -46,6 +46,7 @@ struct media_entity_operations {
 	int (*link_setup)(struct media_entity *entity,
 			  const struct media_pad *local,
 			  const struct media_pad *remote, u32 flags);
+	int (*link_validate)(struct media_link *link);
What about documenting the operation in Documentation/media-framework.txt ?

Yup.

---
Link validation
---------------

Link validation is performed from media_entity_pipeline_start() for any entity which has sink pads in the pipeline. The media_entity::link_validate() callback is used for that purpose. In link_validate() callback, the entity driver should check that the properties of the source pad of the connected entity and its own sink pad match.
---

Quoted 14 lines

include/media/media-entity.h void media_entity_graph_walk_start(struct · line 145

Quoted 11 lines

include/media/media-entity.h +2 -2

 };
 
 struct media_entity {

@@ -140,8 +141,8 @@ void media_entity_graph_walk_start(struct
 media_entity_graph *graph, struct media_entity *entity);
 struct media_entity *
 media_entity_graph_walk_next(struct media_entity_graph *graph);
-void media_entity_pipeline_start(struct media_entity *entity,
-		struct media_pipeline *pipe);
+__must_check int media_entity_pipeline_start(struct media_entity *entity,
+					     struct media_pipeline *pipe);

As well as keeping the media_entity_pipeline_start() documentation up-to-date in the same file :-)

Added a note it may return an error.

Cheers,

Signature · 2 lines

Sakari Ailus
e-mail: [email protected]	jabber/XMPP/Gmail: [email protected]

Laurent Pinchart

maintainer

2012-01-19 16:20 UTC #35 lore raw

Re: [PATCH 16/23] media: Add link_validate op to check links to the sink pad

Hi Sakari,

Quoted 54 lines · On Tuesday 17 January 2012 21:09:58 Sakari Ailus wrote:

Quoted 44 lines · Laurent Pinchart
Quoted 43 lines · On Wednesday 11 January 2012 22:26:53 Sakari Ailus wrote:
Signed-off-by Sakari Ailus <[email protected]>
---
 drivers/media/media-entity.c |   73
++++++++++++++++++++++++++++++++++++++++- include/media/media-entity.h |
5 ++-
 2 files changed, 74 insertions(+), 4 deletions(-)
(quoted diff) +0 -0
b/drivers/media/media-entity.c index 056138f..62ef4b8 100644
drivers/media/media-entity.c +23 -0
@@ -196,6 +196,35 @@ media_entity_graph_walk_next(struct
 media_entity_graph *graph) }
 
 EXPORT_SYMBOL_GPL(media_entity_graph_walk_next);
 
+struct media_link_enum {
+	int i;
+	struct media_entity *entity;
+	unsigned long flags, mask;
+};
+
+static struct media_link
+*media_link_walk_next(struct media_link_enum *link_enum)
+{
+	do {
+		link_enum->i++;
+		if (link_enum->i >= link_enum->entity->num_links)
+			return NULL;
+	} while ((link_enum->entity->links[link_enum->i].flags
+		  & link_enum->mask) != link_enum->flags);
+
+	return &link_enum->entity->links[link_enum->i];
+}
+
+static void media_link_walk_start(struct media_link_enum *link_enum,
+				  struct media_entity *entity,
+				  unsigned long flags, unsigned long mask)
+{
Quoted 8 lines
drivers/media/media-entity.c media_entity_graph_walk_next(struct media_entity_graph *graph) · line 226
Quoted 5 lines
drivers/media/media-entity.c +5 -0
+	link_enum->i = -1;
+	link_enum->entity = entity;
+	link_enum->flags = flags;
+	link_enum->mask = mask;
+}
Do we really need a generic link walking code for a single user ? Merging this in the function below would result in much simpler code.

Quoted 1 line

It's a single funcition but it's being used from two locations in it.

Is it ? Not in this patch at least.

Quoted 2 lines

I would keep it as-is, since performing the same in the function itself would much complicate it.

Are you sure about that ? :-)

Quoted 147 lines

Quoted 56 lines
Quoted 56 lines
drivers/media/media-entity.c +21 -4
+
 
 /*
 
-----------------------------------------------------------------------
--- --- * Pipeline management
 
  */
@@ -214,23 +243,63 @@ EXPORT_SYMBOL_GPL(media_entity_graph_walk_next);
 
  * pipeline pointer must be identical for all nested calls to
  * media_entity_pipeline_start().
  */
 
-void media_entity_pipeline_start(struct media_entity *entity,
-				 struct media_pipeline *pipe)
+__must_check int media_entity_pipeline_start(struct media_entity
 *entity, +					     struct media_pipeline *pipe)
 
 {
 
 	struct media_device *mdev = entity->parent;
 	struct media_entity_graph graph;
 
+	struct media_entity *tmp = entity;
+	int ret = 0;
 
 	mutex_lock(&mdev->graph_mutex);
 
 	media_entity_graph_walk_start(&graph, entity);
 
 	while ((entity = media_entity_graph_walk_next(&graph))) {
 
+		struct media_entity_graph tmp_graph;
+		struct media_link_enum link_enum;
+		struct media_link *link;
+
 
 		entity->stream_count++;
 		WARN_ON(entity->pipe && entity->pipe != pipe);
 		entity->pipe = pipe;
 
+
+		if (!entity->ops || !entity->ops->link_validate)
+			continue;
+
+		media_link_walk_start(&link_enum, entity,
+				      MEDIA_LNK_FL_ENABLED,
+				      MEDIA_LNK_FL_ENABLED);
+
+		while ((link = media_link_walk_next(&link_enum))) {
+			if (link->sink->entity != entity)
+				continue;
+
+			ret = entity->ops->link_validate(link);
Quoted 8 lines
drivers/media/media-entity.c EXPORT_SYMBOL_GPL(media_entity_graph_walk_next); · line 283
Quoted 5 lines
drivers/media/media-entity.c +5 -0
+			if (ret < 0 && ret != -ENOIOCTLCMD)
+				break;
+		}
+		if (!ret || ret == -ENOIOCTLCMD)
+			continue;
What about a goto error instead ? That would keep the error code out of the loop.
Fixed.
Quoted 10 lines
drivers/media/media-entity.c EXPORT_SYMBOL_GPL(media_entity_graph_walk_next); · line 289
Quoted 6 lines
drivers/media/media-entity.c +6 -0
+
+		/*
+		 * Link validation on graph failed. We revert what we
+		 * did and return the error.
+		 */
+		media_entity_graph_walk_start(&tmp_graph, tmp);
I've never liked tmp as a variable name. As the graph variable isn't used anymore from this point on, you can reuse it. tmp can then be renamed to something more descriptive.
I re-use graph, and tmp is called entity_err now.
Quoted 27 lines
Quoted 27 lines
drivers/media/media-entity.c +10 -0
+		do {
+			tmp = media_entity_graph_walk_next(&tmp_graph);
+			tmp->stream_count--;
+			if (entity->stream_count == 0)
+				entity->pipe = NULL;
+		} while (tmp != entity);
+
+		break;
 
 	}
 
 	mutex_unlock(&mdev->graph_mutex);
 
+
+	return ret == 0 || ret == -ENOIOCTLCMD ? 0 : ret;
 
 }
 EXPORT_SYMBOL_GPL(media_entity_pipeline_start);
(quoted diff) +0 -0
b/include/media/media-entity.h index cd8bca6..f7ba80a 100644
include/media/media-entity.h +0 -0
@@ -46,6 +46,7 @@ struct media_entity_operations {
 
 	int (*link_setup)(struct media_entity *entity,
 
Quoted 7 lines
include/media/media-entity.h struct media_entity_operations {
Quoted 4 lines
include/media/media-entity.h +1 -0
 			  const struct media_pad *local,
 			  const struct media_pad *remote, u32 flags);
 
+	int (*link_validate)(struct media_link *link);
What about documenting the operation in Documentation/media-framework.txt ?
Yup.
---
Link validation
---------------
Link validation is performed from media_entity_pipeline_start() for any entity which has sink pads in the pipeline. The media_entity::link_validate() callback is used for that purpose. In link_validate() callback, the entity driver should check that the properties of the source pad of the connected entity and its own sink pad match. ---
Quoted 10 lines
Quoted 10 lines
};
struct media_entity {
include/media/media-entity.h +0 -0
@@ -140,8 +141,8 @@ void media_entity_graph_walk_start(struct
 media_entity_graph *graph, struct media_entity *entity);
 
 struct media_entity *
 media_entity_graph_walk_next(struct media_entity_graph *graph);
 
Quoted 7 lines
include/media/media-entity.h void media_entity_graph_walk_start(struct media_entity_graph *graph,
Quoted 4 lines
include/media/media-entity.h +1 -2
-void media_entity_pipeline_start(struct media_entity *entity,
-		struct media_pipeline *pipe);
+__must_check int media_entity_pipeline_start(struct media_entity
 *entity, +					     struct media_pipeline *pipe);
As well as keeping the media_entity_pipeline_start() documentation up-to-date in the same file :-)
Added a note it may return an error.

Signature · 3 lines

Regards,

Laurent Pinchart

Sakari Ailus

author

2012-01-19 19:13 UTC #36 lore raw

Re: [PATCH 16/23] media: Add link_validate op to check links to the sink pad

Hi Laurent,

On Thu, Jan 19, 2012 at 05:20:55PM +0100, Laurent Pinchart wrote:
...

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
Do we really need a generic link walking code for a single user ? Merging this in the function below would result in much simpler code.
It's a single funcition but it's being used from two locations in it.
Is it ? Not in this patch at least.

Hmm. Good point. I'll check if I can get rid of it and still keep it looking nice.

Signature · 2 lines

Sakari Ailus
e-mail: [email protected]	jabber/XMPP/Gmail: [email protected]

Sakari Ailus

author

2012-01-11 21:27 UTC #37 lore raw

PATCH 17/23 v4l: Implement v4l2_subdev_link_validate()

In reply to Sakari Ailus

v4l2_subdev_link_validate() is the default op for validating a link. In V4L2 subdev context, it is used to call a pad op which performs the proper link check without much extra work.

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/video/v4l2-subdev.c |   62 +++++++++++++++++++++++++++++++++++++
 include/media/v4l2-subdev.h       |   10 ++++++
 2 files changed, 72 insertions(+), 0 deletions(-)

drivers/media/video/v4l2-subdev.c +62 -0

index 836270d..4b329a0 100644
@@ -367,6 +367,68 @@ const struct v4l2_file_operations v4l2_subdev_fops = {
 	.poll = subdev_poll,
 };
 
+#ifdef CONFIG_MEDIA_CONTROLLER
+int v4l2_subdev_link_validate_default(struct v4l2_subdev *sd,
+				      struct media_link *link,
+				      struct v4l2_subdev_format *source_fmt,
+				      struct v4l2_subdev_format *sink_fmt)
+{
+	if (source_fmt->format.width != sink_fmt->format.width
+	    || source_fmt->format.height != sink_fmt->format.height
+	    || source_fmt->format.code != sink_fmt->format.code)
+		return -EINVAL;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate_default);
+
+static struct v4l2_subdev_format
+*v4l2_subdev_link_validate_get_format(struct media_pad *pad,
+				      struct v4l2_subdev_format *fmt)
+{
+	int rval;
+
+	switch (media_entity_type(pad->entity)) {
+	case MEDIA_ENT_T_V4L2_SUBDEV:
+		fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE;
+		fmt->pad = pad->index;
+		rval = v4l2_subdev_call(media_entity_to_v4l2_subdev(
+						pad->entity),
+					pad, get_fmt, NULL, fmt);
+		if (rval < 0)
+			return NULL;
+		return fmt;
+	case MEDIA_ENT_T_DEVNODE_V4L:
+		return NULL;
+	default:
+		BUG();
+	}
+}
+
+int v4l2_subdev_link_validate(struct media_link *link)
+{
+	struct v4l2_subdev *sink = NULL, *source = NULL;
+	struct v4l2_subdev_format _sink_fmt, _source_fmt;
+	struct v4l2_subdev_format *sink_fmt, *source_fmt;
+
+	source_fmt = v4l2_subdev_link_validate_get_format(
+		link->source, &_source_fmt);
+	sink_fmt = v4l2_subdev_link_validate_get_format(
+		link->sink, &_sink_fmt);
+
+	if (source_fmt)
+		source = media_entity_to_v4l2_subdev(link->source->entity);
+	if (sink_fmt)
+		sink = media_entity_to_v4l2_subdev(link->sink->entity);
+
+	if (source_fmt && sink_fmt)
+		return v4l2_subdev_call(sink, pad, link_validate, link,
+					source_fmt, sink_fmt);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate);
+#endif /* CONFIG_MEDIA_CONTROLLER */
+
 void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
 {
 	INIT_LIST_HEAD(&sd->list);

include/media/v4l2-subdev.h +10 -0

index feab950..436e6f4 100644
@@ -470,6 +470,11 @@ struct v4l2_subdev_pad_ops {
 			     struct v4l2_subdev_selection *sel);
 	int (*set_selection)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
 			     struct v4l2_subdev_selection *sel);
+#ifdef CONFIG_MEDIA_CONTROLLER
+	int (*link_validate)(struct v4l2_subdev *sd, struct media_link *link,
+			     struct v4l2_subdev_format *source_fmt,
+			     struct v4l2_subdev_format *sink_fmt);
+#endif /* CONFIG_MEDIA_CONTROLLER */
 };
 
 struct v4l2_subdev_ops {

@@ -606,6 +611,11 @@ static inline void *v4l2_get_subdev_hostdata(const struct v4l2_subdev *sd)
 	return sd->host_priv;
 }
 
+int v4l2_subdev_link_validate_default(struct v4l2_subdev *sd,
+				      struct media_link *link,
+				      struct v4l2_subdev_format *source_fmt,
+				      struct v4l2_subdev_format *sink_fmt);
+int v4l2_subdev_link_validate(struct media_link *link);
 void v4l2_subdev_init(struct v4l2_subdev *sd,
 		      const struct v4l2_subdev_ops *ops);

Signature · 1 line

1.7.2.5

Laurent Pinchart

maintainer

2012-01-16 14:44 UTC #38 lore raw

Re: [PATCH 17/23] v4l: Implement v4l2_subdev_link_validate()

Hi Sakari,

Thanks for the patch.

Quoted 23 lines · On Wednesday 11 January 2012 22:26:54 Sakari Ailus wrote:

v4l2_subdev_link_validate() is the default op for validating a link. In V4L2 subdev context, it is used to call a pad op which performs the proper link check without much extra work.
Signed-off-by Sakari Ailus <[email protected]>
---
 drivers/media/video/v4l2-subdev.c |   62
+++++++++++++++++++++++++++++++++++++ include/media/v4l2-subdev.h       | 
 10 ++++++
 2 files changed, 72 insertions(+), 0 deletions(-)
(quoted diff) +0 -0
b/drivers/media/video/v4l2-subdev.c index 836270d..4b329a0 100644
drivers/media/video/v4l2-subdev.c +4 -0
@@ -367,6 +367,68 @@ const struct v4l2_file_operations v4l2_subdev_fops = {
 	.poll = subdev_poll,
 };
 
+#ifdef CONFIG_MEDIA_CONTROLLER
+int v4l2_subdev_link_validate_default(struct v4l2_subdev *sd,
+				      struct media_link *link,
+				      struct v4l2_subdev_format *source_fmt,

drivers/media/video/v4l2-subdev.c const struct v4l2_file_operations v4l2_subdev_fops = { · line 383

Quoted 10 lines

drivers/media/video/v4l2-subdev.c +10 -0

+				      struct v4l2_subdev_format *sink_fmt)
+{
+	if (source_fmt->format.width != sink_fmt->format.width
+	    || source_fmt->format.height != sink_fmt->format.height
+	    || source_fmt->format.code != sink_fmt->format.code)
+		return -EINVAL;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate_default);

What about calling this function directly from v4l2_subdev_link_validate() if the pad::link_validate operation is NULL ? That wouldn't require changing all subdev drivers to explicitly use the default implementation.

Quoted 11 lines

drivers/media/video/v4l2-subdev.c +11 -0

+
+static struct v4l2_subdev_format
+*v4l2_subdev_link_validate_get_format(struct media_pad *pad,
+				      struct v4l2_subdev_format *fmt)
+{
+	int rval;
+
+	switch (media_entity_type(pad->entity)) {
+	case MEDIA_ENT_T_V4L2_SUBDEV:
+		fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE;
+		fmt->pad = pad->index;

drivers/media/video/v4l2-subdev.c const struct v4l2_file_operations v4l2_subdev_fops = { · line 404

Quoted 10 lines

drivers/media/video/v4l2-subdev.c +10 -0

+		rval = v4l2_subdev_call(media_entity_to_v4l2_subdev(
+						pad->entity),
+					pad, get_fmt, NULL, fmt);
+		if (rval < 0)
+			return NULL;
+		return fmt;
+	case MEDIA_ENT_T_DEVNODE_V4L:
+		return NULL;
+	default:
+		BUG();

Maybe WARN() and return NULL ?

Quoted 12 lines

drivers/media/video/v4l2-subdev.c +12 -0

+	}
+}
+
+int v4l2_subdev_link_validate(struct media_link *link)
+{
+	struct v4l2_subdev *sink = NULL, *source = NULL;
+	struct v4l2_subdev_format _sink_fmt, _source_fmt;
+	struct v4l2_subdev_format *sink_fmt, *source_fmt;
+
+	source_fmt = v4l2_subdev_link_validate_get_format(
+		link->source, &_source_fmt);
+	sink_fmt = v4l2_subdev_link_validate_get_format(

drivers/media/video/v4l2-subdev.c const struct v4l2_file_operations v4l2_subdev_fops = { · line 426

Quoted 10 lines

drivers/media/video/v4l2-subdev.c +10 -0

+		link->sink, &_sink_fmt);
+
+	if (source_fmt)
+		source = media_entity_to_v4l2_subdev(link->source->entity);
+	if (sink_fmt)
+		sink = media_entity_to_v4l2_subdev(link->sink->entity);
+
+	if (source_fmt && sink_fmt)
+		return v4l2_subdev_call(sink, pad, link_validate, link,
+					source_fmt, sink_fmt);

This looks overly complex. Why don't you return 0 if one of the two entities is of a type different than MEDIA_ENT_T_V4L2_SUBDEV, then retrieve the formats for the two entities and return 0 if one of the two operation fails, and finally call pad::link_validate ?

Quoted 35 lines

drivers/media/video/v4l2-subdev.c +5 -0

+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate);
+#endif /* CONFIG_MEDIA_CONTROLLER */
+
 void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops
 *ops) {
 	INIT_LIST_HEAD(&sd->list);

include/media/v4l2-subdev.h +10 -0

index feab950..436e6f4 100644
@@ -470,6 +470,11 @@ struct v4l2_subdev_pad_ops {
 			     struct v4l2_subdev_selection *sel);
 	int (*set_selection)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
 			     struct v4l2_subdev_selection *sel);
+#ifdef CONFIG_MEDIA_CONTROLLER
+	int (*link_validate)(struct v4l2_subdev *sd, struct media_link *link,
+			     struct v4l2_subdev_format *source_fmt,
+			     struct v4l2_subdev_format *sink_fmt);
+#endif /* CONFIG_MEDIA_CONTROLLER */
 };
 
 struct v4l2_subdev_ops {

@@ -606,6 +611,11 @@ static inline void *v4l2_get_subdev_hostdata(const
 struct v4l2_subdev *sd) return sd->host_priv;
 }
 
+int v4l2_subdev_link_validate_default(struct v4l2_subdev *sd,
+				      struct media_link *link,
+				      struct v4l2_subdev_format *source_fmt,
+				      struct v4l2_subdev_format *sink_fmt);
+int v4l2_subdev_link_validate(struct media_link *link);
 void v4l2_subdev_init(struct v4l2_subdev *sd,
 		      const struct v4l2_subdev_ops *ops);

Signature · 3 lines

Regards,

Laurent Pinchart

Sakari Ailus

author

2012-01-17 20:21 UTC #39 lore raw

Re: [PATCH 17/23] v4l: Implement v4l2_subdev_link_validate()

Hi Laurent,

Thanks for the review!

Quoted 30 lines · Laurent Pinchart

Quoted 29 lines · On Wednesday 11 January 2012 22:26:54 Sakari Ailus wrote:
v4l2_subdev_link_validate() is the default op for validating a link. In V4L2 subdev context, it is used to call a pad op which performs the proper link check without much extra work.
Signed-off-by Sakari Ailus <[email protected]>
---
 drivers/media/video/v4l2-subdev.c |   62
+++++++++++++++++++++++++++++++++++++ include/media/v4l2-subdev.h       | 
 10 ++++++
 2 files changed, 72 insertions(+), 0 deletions(-)
(quoted diff) +0 -0
b/drivers/media/video/v4l2-subdev.c index 836270d..4b329a0 100644
drivers/media/video/v4l2-subdev.c +10 -0
@@ -367,6 +367,68 @@ const struct v4l2_file_operations v4l2_subdev_fops = {
 	.poll = subdev_poll,
 };
 
+#ifdef CONFIG_MEDIA_CONTROLLER
+int v4l2_subdev_link_validate_default(struct v4l2_subdev *sd,
+				      struct media_link *link,
+				      struct v4l2_subdev_format *source_fmt,
+				      struct v4l2_subdev_format *sink_fmt)
+{
+	if (source_fmt->format.width != sink_fmt->format.width
+	    || source_fmt->format.height != sink_fmt->format.height
+	    || source_fmt->format.code != sink_fmt->format.code)
+		return -EINVAL;

Quoted 8 lines

drivers/media/video/v4l2-subdev.c const struct v4l2_file_operations v4l2_subdev_fops = { · line 383
Quoted 4 lines
drivers/media/video/v4l2-subdev.c +4 -0
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate_default);
What about calling this function directly from v4l2_subdev_link_validate() if the pad::link_validate operation is NULL ? That wouldn't require changing all subdev drivers to explicitly use the default implementation.

I can do that. I still want to keep the function available for those that want to call it explicitly to perform the above check.

Quoted 15 lines

drivers/media/video/v4l2-subdev.c +15 -0

+
+static struct v4l2_subdev_format
+*v4l2_subdev_link_validate_get_format(struct media_pad *pad,
+				      struct v4l2_subdev_format *fmt)
+{
+	int rval;
+
+	switch (media_entity_type(pad->entity)) {
+	case MEDIA_ENT_T_V4L2_SUBDEV:
+		fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE;
+		fmt->pad = pad->index;
+		rval = v4l2_subdev_call(media_entity_to_v4l2_subdev(
+						pad->entity),
+					pad, get_fmt, NULL, fmt);
+		if (rval < 0)

Quoted 8 lines

drivers/media/video/v4l2-subdev.c const struct v4l2_file_operations v4l2_subdev_fops = { · line 404
Quoted 6 lines
drivers/media/video/v4l2-subdev.c +6 -0
+			return NULL;
+		return fmt;
+	case MEDIA_ENT_T_DEVNODE_V4L:
+		return NULL;
+	default:
+		BUG();
Maybe WARN() and return NULL ?

It's a clear driver BUG() if this happens. If you think the correct response to that is WARN() and return NULL, I can do that.

Quoted 19 lines

drivers/media/video/v4l2-subdev.c +19 -0

+	}
+}
+
+int v4l2_subdev_link_validate(struct media_link *link)
+{
+	struct v4l2_subdev *sink = NULL, *source = NULL;
+	struct v4l2_subdev_format _sink_fmt, _source_fmt;
+	struct v4l2_subdev_format *sink_fmt, *source_fmt;
+
+	source_fmt = v4l2_subdev_link_validate_get_format(
+		link->source, &_source_fmt);
+	sink_fmt = v4l2_subdev_link_validate_get_format(
+		link->sink, &_sink_fmt);
+
+	if (source_fmt)
+		source = media_entity_to_v4l2_subdev(link->source->entity);
+	if (sink_fmt)
+		sink = media_entity_to_v4l2_subdev(link->sink->entity);
+

Quoted 8 lines

drivers/media/video/v4l2-subdev.c const struct v4l2_file_operations v4l2_subdev_fops = { · line 426
Quoted 3 lines
drivers/media/video/v4l2-subdev.c +3 -0
+	if (source_fmt && sink_fmt)
+		return v4l2_subdev_call(sink, pad, link_validate, link,
+					source_fmt, sink_fmt);
This looks overly complex. Why don't you return 0 if one of the two entities is of a type different than MEDIA_ENT_T_V4L2_SUBDEV, then retrieve the formats for the two entities and return 0 if one of the two operation fails, and finally call pad::link_validate ?

Now that you mention that, I agree. :-) I'll fix it.

Regards,

Signature · 2 lines

Sakari Ailus
e-mail: [email protected]	jabber/XMPP/Gmail: [email protected]

Laurent Pinchart

maintainer

2012-01-19 16:21 UTC #40 lore raw

Re: [PATCH 17/23] v4l: Implement v4l2_subdev_link_validate()

Hi Sakari,

Quoted 75 lines · On Tuesday 17 January 2012 21:21:39 Sakari Ailus wrote:

Quoted 38 lines · Laurent Pinchart
Quoted 37 lines · On Wednesday 11 January 2012 22:26:54 Sakari Ailus wrote:
v4l2_subdev_link_validate() is the default op for validating a link. In V4L2 subdev context, it is used to call a pad op which performs the proper link check without much extra work.
Signed-off-by Sakari Ailus <[email protected]>
---
 drivers/media/video/v4l2-subdev.c |   62
+++++++++++++++++++++++++++++++++++++ include/media/v4l2-subdev.h |
10 ++++++
 2 files changed, 72 insertions(+), 0 deletions(-)
(quoted diff) +0 -0
b/drivers/media/video/v4l2-subdev.c index 836270d..4b329a0 100644
drivers/media/video/v4l2-subdev.c +11 -0
@@ -367,6 +367,68 @@ const struct v4l2_file_operations v4l2_subdev_fops
 = {
 
 	.poll = subdev_poll,
 
 };
 
+#ifdef CONFIG_MEDIA_CONTROLLER
+int v4l2_subdev_link_validate_default(struct v4l2_subdev *sd,
+				      struct media_link *link,
+				      struct v4l2_subdev_format *source_fmt,
+				      struct v4l2_subdev_format *sink_fmt)
+{
+	if (source_fmt->format.width != sink_fmt->format.width
+	    || source_fmt->format.height != sink_fmt->format.height
+	    || source_fmt->format.code != sink_fmt->format.code)
+		return -EINVAL;
+
Quoted 8 lines
drivers/media/video/v4l2-subdev.c const struct v4l2_file_operations v4l2_subdev_fops = { · line 383
Quoted 3 lines
drivers/media/video/v4l2-subdev.c +3 -0
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate_default);
What about calling this function directly from v4l2_subdev_link_validate() if the pad::link_validate operation is NULL ? That wouldn't require changing all subdev drivers to explicitly use the default implementation.
I can do that. I still want to keep the function available for those that want to call it explicitly to perform the above check.
Quoted 15 lines
Quoted 15 lines
drivers/media/video/v4l2-subdev.c +15 -0
+
+static struct v4l2_subdev_format
+*v4l2_subdev_link_validate_get_format(struct media_pad *pad,
+				      struct v4l2_subdev_format *fmt)
+{
+	int rval;
+
+	switch (media_entity_type(pad->entity)) {
+	case MEDIA_ENT_T_V4L2_SUBDEV:
+		fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE;
+		fmt->pad = pad->index;
+		rval = v4l2_subdev_call(media_entity_to_v4l2_subdev(
+						pad->entity),
+					pad, get_fmt, NULL, fmt);
+		if (rval < 0)
Quoted 8 lines
drivers/media/video/v4l2-subdev.c const struct v4l2_file_operations v4l2_subdev_fops = { · line 404
Quoted 6 lines
drivers/media/video/v4l2-subdev.c +6 -0
+			return NULL;
+		return fmt;
+	case MEDIA_ENT_T_DEVNODE_V4L:
+		return NULL;
+	default:
+		BUG();
Maybe WARN() and return NULL ?

Quoted 2 lines

It's a clear driver BUG() if this happens. If you think the correct response to that is WARN() and return NULL, I can do that.

You're right.

Quoted 29 lines

Quoted 19 lines
Quoted 19 lines
drivers/media/video/v4l2-subdev.c +19 -0
+	}
+}
+
+int v4l2_subdev_link_validate(struct media_link *link)
+{
+	struct v4l2_subdev *sink = NULL, *source = NULL;
+	struct v4l2_subdev_format _sink_fmt, _source_fmt;
+	struct v4l2_subdev_format *sink_fmt, *source_fmt;
+
+	source_fmt = v4l2_subdev_link_validate_get_format(
+		link->source, &_source_fmt);
+	sink_fmt = v4l2_subdev_link_validate_get_format(
+		link->sink, &_sink_fmt);
+
+	if (source_fmt)
+		source = media_entity_to_v4l2_subdev(link->source->entity);
+	if (sink_fmt)
+		sink = media_entity_to_v4l2_subdev(link->sink->entity);
+
Quoted 8 lines
drivers/media/video/v4l2-subdev.c const struct v4l2_file_operations v4l2_subdev_fops = { · line 426
Quoted 3 lines
drivers/media/video/v4l2-subdev.c +3 -0
+	if (source_fmt && sink_fmt)
+		return v4l2_subdev_call(sink, pad, link_validate, link,
+					source_fmt, sink_fmt);
This looks overly complex. Why don't you return 0 if one of the two entities is of a type different than MEDIA_ENT_T_V4L2_SUBDEV, then retrieve the formats for the two entities and return 0 if one of the two operation fails, and finally call pad::link_validate ?
Now that you mention that, I agree. :-) I'll fix it.

Signature · 3 lines

Regards,

Laurent Pinchart

Sakari Ailus

author

2012-01-11 21:27 UTC #41 lore raw

PATCH 18/23 omap3isp: Assume media_entity_pipeline_start may fail

In reply to Sakari Ailus

Since media_entity_pipeline_start() now does link validation, it may actually fail. Perform the error handling.

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/video/omap3isp/ispvideo.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

drivers/media/video/omap3isp/ispvideo.c +5 -1

index 9cc5090..2ff7f91 100644
@@ -988,7 +988,11 @@ isp_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 	 */
 	pipe = video->video.entity.pipe
 	     ? to_isp_pipeline(&video->video.entity) : &video->pipe;
-	media_entity_pipeline_start(&video->video.entity, &pipe->pipe);
+	ret = media_entity_pipeline_start(&video->video.entity, &pipe->pipe);
+	if (ret < 0) {
+		mutex_unlock(&video->stream_lock);
+		return ret;
+	}
 
 	/* Verify that the currently configured format matches the output of
 	 * the connected subdev.

Signature · 1 line

1.7.2.5

Laurent Pinchart

maintainer Acked-by on v1 18/23

2012-01-16 14:46 UTC #42 lore raw

Re: [PATCH 18/23] omap3isp: Assume media_entity_pipeline_start may fail

Quoted 4 lines · On Wednesday 11 January 2012 22:26:55 Sakari Ailus wrote:

Since media_entity_pipeline_start() now does link validation, it may actually fail. Perform the error handling.
Signed-off-by Sakari Ailus <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

Signature · 3 lines

Regards,

Laurent Pinchart

Sakari Ailus

author

2012-01-11 21:27 UTC #43 lore raw

PATCH 19/23 omap3isp: Default error handling for ccp2, csi2, preview and resizer

In reply to Sakari Ailus

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/video/omap3isp/ispccp2.c    |    2 ++
 drivers/media/video/omap3isp/ispcsi2.c    |    2 ++
 drivers/media/video/omap3isp/isppreview.c |    2 ++
 drivers/media/video/omap3isp/ispresizer.c |    2 ++
 drivers/media/video/omap3isp/ispvideo.c   |   18 ++++++++----------
 5 files changed, 16 insertions(+), 10 deletions(-)

drivers/media/video/omap3isp/ispccp2.c +2 -0

index 904ca8c..ab0b4db 100644
@@ -933,6 +933,7 @@ static const struct v4l2_subdev_pad_ops ccp2_sd_pad_ops = {
 	.enum_frame_size = ccp2_enum_frame_size,
 	.get_fmt = ccp2_get_format,
 	.set_fmt = ccp2_set_format,
+	.link_validate = v4l2_subdev_link_validate_default,
 };
 
 /* subdev operations */

@@ -1029,6 +1030,7 @@ static int ccp2_link_setup(struct media_entity *entity,
 /* media operations */
 static const struct media_entity_operations ccp2_media_ops = {
 	.link_setup = ccp2_link_setup,
+	.link_validate = v4l2_subdev_link_validate,
 };
 
 /*

drivers/media/video/omap3isp/ispcsi2.c +2 -0

index 0b3e705..ab6ae76 100644
@@ -1151,6 +1151,7 @@ static const struct v4l2_subdev_pad_ops csi2_pad_ops = {
 	.enum_frame_size = csi2_enum_frame_size,
 	.get_fmt = csi2_get_format,
 	.set_fmt = csi2_set_format,
+	.link_validate = v4l2_subdev_link_validate_default,
 };
 
 /* subdev operations */

@@ -1225,6 +1226,7 @@ static int csi2_link_setup(struct media_entity *entity,
 /* media operations */
 static const struct media_entity_operations csi2_media_ops = {
 	.link_setup = csi2_link_setup,
+	.link_validate = v4l2_subdev_link_validate,
 };
 
 void omap3isp_csi2_unregister_entities(struct isp_csi2_device *csi2)

drivers/media/video/omap3isp/isppreview.c +2 -0

index ccb876f..62c49d6 100644
@@ -1986,6 +1986,7 @@ static const struct v4l2_subdev_pad_ops preview_v4l2_pad_ops = {
 	.set_fmt = preview_set_format,
 	.get_crop = preview_get_crop,
 	.set_crop = preview_set_crop,
+	.link_validate = v4l2_subdev_link_validate_default,
 };
 
 /* subdev operations */

@@ -2081,6 +2082,7 @@ static int preview_link_setup(struct media_entity *entity,
 /* media operations */
 static const struct media_entity_operations preview_media_ops = {
 	.link_setup = preview_link_setup,
+	.link_validate = v4l2_subdev_link_validate,
 };
 
 void omap3isp_preview_unregister_entities(struct isp_prev_device *prev)

drivers/media/video/omap3isp/ispresizer.c +2 -0

index 50e593b..ecc377b 100644
@@ -1535,6 +1535,7 @@ static const struct v4l2_subdev_pad_ops resizer_v4l2_pad_ops = {
 	.set_fmt = resizer_set_format,
 	.get_crop = resizer_g_crop,
 	.set_crop = resizer_s_crop,
+	.link_validate = v4l2_subdev_link_validate_default,
 };
 
 /* subdev operations */

@@ -1606,6 +1607,7 @@ static int resizer_link_setup(struct media_entity *entity,
 /* media operations */
 static const struct media_entity_operations resizer_media_ops = {
 	.link_setup = resizer_link_setup,
+	.link_validate = v4l2_subdev_link_validate,
 };
 
 void omap3isp_resizer_unregister_entities(struct isp_res_device *res)

drivers/media/video/omap3isp/ispvideo.c +8 ~~-10~~

index 2ff7f91..12b4d99 100644
@@ -304,8 +304,6 @@ static int isp_video_validate_pipeline(struct isp_pipeline *pipe)
 	struct v4l2_subdev *subdev;
 	int ret;
 
-	pipe->max_rate = pipe->l3_ick;
-
 	subdev = isp_video_remote_subdev(pipe->output, NULL);
 	if (subdev == NULL)
 		return -EPIPE;

@@ -988,11 +986,15 @@ isp_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 	 */
 	pipe = video->video.entity.pipe
 	     ? to_isp_pipeline(&video->video.entity) : &video->pipe;
+
+	if (video->isp->pdata->set_constraints)
+		video->isp->pdata->set_constraints(video->isp, true);
+	pipe->l3_ick = clk_get_rate(video->isp->clock[ISP_CLK_L3_ICK]);
+	pipe->max_rate = pipe->l3_ick;
+
 	ret = media_entity_pipeline_start(&video->video.entity, &pipe->pipe);
-	if (ret < 0) {
-		mutex_unlock(&video->stream_lock);
-		return ret;
-	}
+	if (ret < 0)
+		goto error;
 
 	/* Verify that the currently configured format matches the output of
 	 * the connected subdev.

@@ -1024,10 +1026,6 @@ isp_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 		pipe->output = far_end;
 	}
 
-	if (video->isp->pdata->set_constraints)
-		video->isp->pdata->set_constraints(video->isp, true);
-	pipe->l3_ick = clk_get_rate(video->isp->clock[ISP_CLK_L3_ICK]);
-
 	/* Validate the pipeline and update its state. */
 	ret = isp_video_validate_pipeline(pipe);
 	if (ret < 0)

Signature · 1 line

1.7.2.5

Laurent Pinchart

maintainer

2012-01-16 14:50 UTC #44 lore raw

Re: [PATCH 19/23] omap3isp: Default error handling for ccp2, csi2, preview and resizer

Hi Sakari,

Thanks for the patch.

Quoted 8 lines · On Wednesday 11 January 2012 22:26:56 Sakari Ailus wrote:

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/video/omap3isp/ispccp2.c    |    2 ++
 drivers/media/video/omap3isp/ispcsi2.c    |    2 ++
 drivers/media/video/omap3isp/isppreview.c |    2 ++
 drivers/media/video/omap3isp/ispresizer.c |    2 ++
 drivers/media/video/omap3isp/ispvideo.c   |   18 ++++++++----------
 5 files changed, 16 insertions(+), 10 deletions(-)

[snip]

Does the below code belong to this patch ? The commit message doesn't explain why this is needed.

Quoted 20 lines

(quoted diff) +0 -0

b/drivers/media/video/omap3isp/ispvideo.c index 2ff7f91..12b4d99 100644

drivers/media/video/omap3isp/ispvideo.c +0 -2

@@ -304,8 +304,6 @@ static int isp_video_validate_pipeline(struct
 isp_pipeline *pipe) struct v4l2_subdev *subdev;
 	int ret;
 
-	pipe->max_rate = pipe->l3_ick;
-
 	subdev = isp_video_remote_subdev(pipe->output, NULL);
 	if (subdev == NULL)
 		return -EPIPE;

@@ -988,11 +986,15 @@ isp_video_streamon(struct file *file, void *fh, enum

v4l2_buf_type type) */

drivers/media/video/omap3isp/ispvideo.c +3 -0

 	pipe = video->video.entity.pipe
 	     ? to_isp_pipeline(&video->video.entity) : &video->pipe;
+
+	if (video->isp->pdata->set_constraints)
+		video->isp->pdata->set_constraints(video->isp, true);

drivers/media/video/omap3isp/ispvideo.c isp_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type) · line 997

Quoted 10 lines

drivers/media/video/omap3isp/ispvideo.c +5 -4

+	pipe->l3_ick = clk_get_rate(video->isp->clock[ISP_CLK_L3_ICK]);
+	pipe->max_rate = pipe->l3_ick;
+
 	ret = media_entity_pipeline_start(&video->video.entity, &pipe->pipe);
-	if (ret < 0) {
-		mutex_unlock(&video->stream_lock);
-		return ret;
-	}
+	if (ret < 0)
+		goto error;

Won't this result in media_entity_pipeline_stop() being called without the pipeline having been succesfully started first ?

Quoted 14 lines

drivers/media/video/omap3isp/ispvideo.c +0 -4

 	/* Verify that the currently configured format matches the output of
 	 * the connected subdev.

@@ -1024,10 +1026,6 @@ isp_video_streamon(struct file *file, void *fh, enum
 v4l2_buf_type type) pipe->output = far_end;
 	}
 
-	if (video->isp->pdata->set_constraints)
-		video->isp->pdata->set_constraints(video->isp, true);
-	pipe->l3_ick = clk_get_rate(video->isp->clock[ISP_CLK_L3_ICK]);
-
 	/* Validate the pipeline and update its state. */
 	ret = isp_video_validate_pipeline(pipe);
 	if (ret < 0)

Signature · 3 lines

Regards,

Laurent Pinchart

Sakari Ailus

author

2012-01-17 20:22 UTC #45 lore raw

Re: [PATCH 19/23] omap3isp: Default error handling for ccp2, csi2, preview and resizer

Quoted 14 lines · Laurent Pinchart

Hi Sakari,
Thanks for the patch.
Quoted 8 lines · On Wednesday 11 January 2012 22:26:56 Sakari Ailus wrote:
Signed-off-by Sakari Ailus <[email protected]>
---
 drivers/media/video/omap3isp/ispccp2.c    |    2 ++
 drivers/media/video/omap3isp/ispcsi2.c    |    2 ++
 drivers/media/video/omap3isp/isppreview.c |    2 ++
 drivers/media/video/omap3isp/ispresizer.c |    2 ++
 drivers/media/video/omap3isp/ispvideo.c   |   18 ++++++++----------
 5 files changed, 16 insertions(+), 10 deletions(-)

Quoted 4 lines

[snip]
Does the below code belong to this patch ? The commit message doesn't explain why this is needed.

I separated these changes from the rest. I'll send a new patchset in the near future.

Signature · 2 lines

Sakari Ailus
e-mail: [email protected]	jabber/XMPP/Gmail: [email protected]

Sakari Ailus

author

2012-01-11 21:27 UTC #46 lore raw

PATCH 20/23 omap3isp: Move CCDC link validation to ispccdc.c

In reply to Sakari Ailus

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/video/omap3isp/ispccdc.c  |   75 ++++++++++++++++++++++++++
 drivers/media/video/omap3isp/ispvideo.c |   88 ++----------------------------
 2 files changed, 81 insertions(+), 82 deletions(-)

drivers/media/video/omap3isp/ispccdc.c +75 -0

index b0b0fa5..1874be4 100644
@@ -2000,6 +2000,79 @@ static int ccdc_set_format(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
 }
 
 /*
+ * Decide whether desired output pixel code can be obtained with
+ * the lane shifter by shifting the input pixel code.
+ * @in: input pixelcode to shifter
+ * @out: output pixelcode from shifter
+ * @additional_shift: # of bits the sensor's LSB is offset from CAMEXT[0]
+ *
+ * return true if the combination is possible
+ * return false otherwise
+ */
+static bool ccdc_is_shiftable(enum v4l2_mbus_pixelcode in,
+			      enum v4l2_mbus_pixelcode out,
+			      unsigned int additional_shift)
+{
+	const struct isp_format_info *in_info, *out_info;
+
+	if (in == out)
+		return true;
+
+	in_info = omap3isp_video_format_info(in);
+	out_info = omap3isp_video_format_info(out);
+
+	if ((in_info->flavor == 0) || (out_info->flavor == 0))
+		return false;
+
+	if (in_info->flavor != out_info->flavor)
+		return false;
+
+	return in_info->bpp - out_info->bpp + additional_shift <= 6;
+}
+
+static int ccdc_link_validate(struct v4l2_subdev *sd,
+			      struct media_link *link,
+			      struct v4l2_subdev_format *source_fmt,
+			      struct v4l2_subdev_format *sink_fmt)
+{
+	struct isp_ccdc_device *ccdc = v4l2_get_subdevdata(sd);
+	struct isp_device *isp = to_isp_device(ccdc);
+	struct isp_pipeline *pipe = to_isp_pipeline(&sd->entity);
+	unsigned int parallel_shift = 0;
+
+	/* Check if the two ends match */
+	if (source_fmt->format.width != sink_fmt->format.width ||
+	    source_fmt->format.height != sink_fmt->format.height)
+		return -EPIPE;
+
+	/* Check ccdc maximum data rate when data comes from sensor
+	 * TODO: Include ccdc rate in pipe->max_rate and compare the
+	 *       total pipe rate with the input data rate from sensor.
+	 */
+	if (pipe->input == NULL) {
+		unsigned int rate = UINT_MAX;
+
+		omap3isp_ccdc_max_rate(&isp->isp_ccdc, &rate);
+		if (isp->isp_ccdc.vpcfg.pixelclk > rate)
+			return -ENOSPC;
+	}
+
+	/* Lane shifter may be used to drop bits on CCDC sink pad */
+	if (isp->isp_ccdc.input == CCDC_INPUT_PARALLEL) {
+		struct isp_parallel_platform_data *pdata =
+			&((struct isp_v4l2_subdevs_group *)
+			  sd->host_priv)->bus.parallel;
+		parallel_shift = pdata->data_lane_shift * 2;
+	}
+	if (!ccdc_is_shiftable(source_fmt->format.code,
+			       sink_fmt->format.code, parallel_shift))
+		return -EPIPE;
+
+	return 0;
+}
+
+
+/*
  * ccdc_init_formats - Initialize formats on all pads
  * @sd: ISP CCDC V4L2 subdevice
  * @fh: V4L2 subdev file handle

@@ -2041,6 +2114,7 @@ static const struct v4l2_subdev_pad_ops ccdc_v4l2_pad_ops = {
 	.enum_frame_size = ccdc_enum_frame_size,
 	.get_fmt = ccdc_get_format,
 	.set_fmt = ccdc_set_format,
+	.link_validate = ccdc_link_validate,
 };
 
 /* V4L2 subdev operations */

@@ -2150,6 +2224,7 @@ static int ccdc_link_setup(struct media_entity *entity,
 /* media operations */
 static const struct media_entity_operations ccdc_media_ops = {
 	.link_setup = ccdc_link_setup,
+	.link_validate = v4l2_subdev_link_validate,
 };
 
 void omap3isp_ccdc_unregister_entities(struct isp_ccdc_device *ccdc)

drivers/media/video/omap3isp/ispvideo.c +6 ~~-82~~

124 lines not on this page · raw message

Signature · 1 line

1.7.2.5

Sakari Ailus

author

2012-01-11 21:27 UTC #47 lore raw

PATCH 21/23 omap3isp: Move resizer link validation to ispresizer.c

In reply to Sakari Ailus

Also remove isp_pipeline_validate.

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/video/omap3isp/ispresizer.c |   17 ++++++++-
 drivers/media/video/omap3isp/ispvideo.c   |   61 -----------------------------
 2 files changed, 16 insertions(+), 62 deletions(-)

drivers/media/video/omap3isp/ispresizer.c +16 -1

index ecc377b..81f66bf 100644
@@ -1303,6 +1303,21 @@ static int resizer_s_crop(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
 	return 0;
 }
 
+static int resizer_link_validate(struct v4l2_subdev *sd,
+				 struct media_link *link,
+				 struct v4l2_subdev_format *source_fmt,
+				 struct v4l2_subdev_format *sink_fmt)
+{
+	struct isp_ccdc_device *ccdc = v4l2_get_subdevdata(sd);
+	struct isp_device *isp = to_isp_device(ccdc);
+	struct isp_pipeline *pipe = to_isp_pipeline(&sd->entity);
+
+	omap3isp_resizer_max_rate(&isp->isp_res, &pipe->max_rate);
+
+	return v4l2_subdev_link_validate_default(sd, link, source_fmt,
+						 sink_fmt);
+}
+
 /* resizer pixel formats */
 static const unsigned int resizer_formats[] = {
 	V4L2_MBUS_FMT_UYVY8_1X16,

@@ -1535,7 +1550,7 @@ static const struct v4l2_subdev_pad_ops resizer_v4l2_pad_ops = {
 	.set_fmt = resizer_set_format,
 	.get_crop = resizer_g_crop,
 	.set_crop = resizer_s_crop,
-	.link_validate = v4l2_subdev_link_validate_default,
+	.link_validate = resizer_link_validate,
 };
 
 /* subdev operations */

drivers/media/video/omap3isp/ispvideo.c +0 ~~-61~~

73 lines not on this page · raw message

Signature · 1 line

1.7.2.5

Sakari Ailus

author

2012-01-11 21:27 UTC #48 lore raw

PATCH 22/23 smiapp: Add driver.

In reply to Sakari Ailus

From: Sakari Ailus <[email protected]>

Add driver for SMIA++/SMIA image sensors. The driver exposes the sensor as three subdevs, pixel array, binner and scaler --- in case the device has a scaler.

Currently it relies on the board code for external clock handling. There is no fast way out of this dependency before the ISP drivers (omap3isp) among others will be able to export that clock through the clock framework instead.

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/video/Kconfig                  |    2 +
 drivers/media/video/Makefile                 |    1 +
 drivers/media/video/smiapp/Kconfig           |   12 +
 drivers/media/video/smiapp/Makefile          |    3 +
 drivers/media/video/smiapp/smiapp-core.c     | 2660 ++++++++++++++++++++++++++
 drivers/media/video/smiapp/smiapp-debug.h    |   32 +
 drivers/media/video/smiapp/smiapp-limits.c   |  132 ++
 drivers/media/video/smiapp/smiapp-limits.h   |  128 ++
 drivers/media/video/smiapp/smiapp-pll.c      |  581 ++++++
 drivers/media/video/smiapp/smiapp-quirk.c    |  264 +++
 drivers/media/video/smiapp/smiapp-quirk.h    |   72 +
 drivers/media/video/smiapp/smiapp-reg-defs.h |  503 +++++
 drivers/media/video/smiapp/smiapp-reg.h      |  122 ++
 drivers/media/video/smiapp/smiapp-regs.c     |  230 +++
 drivers/media/video/smiapp/smiapp-regs.h     |   46 +
 drivers/media/video/smiapp/smiapp.h          |  253 +++
 include/media/smiapp.h                       |   83 +
 17 files changed, 5124 insertions(+), 0 deletions(-)
 create mode 100644 drivers/media/video/smiapp/Kconfig
 create mode 100644 drivers/media/video/smiapp/Makefile
 create mode 100644 drivers/media/video/smiapp/smiapp-core.c
 create mode 100644 drivers/media/video/smiapp/smiapp-debug.h
 create mode 100644 drivers/media/video/smiapp/smiapp-limits.c
 create mode 100644 drivers/media/video/smiapp/smiapp-limits.h
 create mode 100644 drivers/media/video/smiapp/smiapp-pll.c
 create mode 100644 drivers/media/video/smiapp/smiapp-quirk.c
 create mode 100644 drivers/media/video/smiapp/smiapp-quirk.h
 create mode 100644 drivers/media/video/smiapp/smiapp-reg-defs.h
 create mode 100644 drivers/media/video/smiapp/smiapp-reg.h
 create mode 100644 drivers/media/video/smiapp/smiapp-regs.c
 create mode 100644 drivers/media/video/smiapp/smiapp-regs.h
 create mode 100644 drivers/media/video/smiapp/smiapp.h
 create mode 100644 include/media/smiapp.h

drivers/media/video/Kconfig +2 -0

index 4e8a0c4..78f2a1b 100644
@@ -524,6 +524,8 @@ config VIDEO_S5K6AA
 	  This is a V4L2 sensor-level driver for Samsung S5K6AA(FX) 1.3M
 	  camera sensor with an embedded SoC image signal processor.
 
+source "drivers/media/video/smiapp/Kconfig"
+
 comment "Flash devices"
 
 config VIDEO_ADP1653

drivers/media/video/Makefile +1 -0

index ddeaa6c..f3fb3ea 100644
@@ -73,6 +73,7 @@ obj-$(CONFIG_VIDEO_SR030PC30)	+= sr030pc30.o
 obj-$(CONFIG_VIDEO_NOON010PC30)	+= noon010pc30.o
 obj-$(CONFIG_VIDEO_M5MOLS)	+= m5mols/
 obj-$(CONFIG_VIDEO_S5K6AA)	+= s5k6aa.o
+obj-$(CONFIG_VIDEO_SMIAPP)	+= smiapp/
 obj-$(CONFIG_VIDEO_ADP1653)	+= adp1653.o
 
 obj-$(CONFIG_SOC_CAMERA_IMX074)		+= imx074.o

drivers/media/video/smiapp/Kconfig new file +12 -0

new file mode 100644
index 0000000..3f98e8e
@@ -0,0 +1,12 @@
+config VIDEO_SMIAPP
+	tristate "SMIA++/SMIA sensor support"
+	depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER
+	---help---
+	  This is a generic driver for SMIA++/SMIA camera modules.
+
+config VIDEO_SMIAPP_DEBUG
+	bool "Enable debugging for the generic SMIA++/SMIA driver"
+	depends on VIDEO_SMIAPP
+	---help---
+	  Enable debugging output in the generic SMIA++/SMIA driver. If you
+	  are developing the driver you might want to enable this.

drivers/media/video/smiapp/Makefile new file +3 -0

new file mode 100644
index 0000000..83dc0d1
@@ -0,0 +1,3 @@
+smiapp-objs			+= smiapp-core.o smiapp-regs.o smiapp-pll.o \
+				   smiapp-quirk.o smiapp-limits.o
+obj-$(CONFIG_VIDEO_SMIAPP)	+= smiapp.o

drivers/media/video/smiapp/smiapp-core.c new file +2660 -0

2,660 lines not on this page · raw message

drivers/media/video/smiapp/smiapp-debug.h new file +32 -0

new file mode 100644
index 0000000..011bf53
@@ -0,0 +1,32 @@
+/*
+ * drivers/media/video/smiapp-debug.h
+ *
+ * Generic driver for SMIA/SMIA++ compliant camera modules
+ *
+ * Copyright (C) 2011--2012 Nokia Corporation
+ * Contact: Sakari Ailus <[email protected]>
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ *
+ */
+
+#ifndef SMIAPP_DEBUG_H
+#define SMIAPP_DEBUG_H
+
+#ifdef CONFIG_VIDEO_SMIAPP_DEBUG
+#define DEBUG
+#endif
+
+#endif

drivers/media/video/smiapp/smiapp-limits.c new file +132 -0

132 lines not on this page · raw message

drivers/media/video/smiapp/smiapp-limits.h new file +128 -0

128 lines not on this page · raw message

drivers/media/video/smiapp/smiapp-pll.c new file +581 -0

18 lines not on this page · raw message

drivers/media/video/smiapp/smiapp-quirk.c new file +264 -0

264 lines not on this page · raw message

drivers/media/video/smiapp/smiapp-quirk.h new file +72 -0

72 lines not on this page · raw message

drivers/media/video/smiapp/smiapp-reg-defs.h new file +503 -0

503 lines not on this page · raw message

drivers/media/video/smiapp/smiapp-reg.h new file +122 -0

122 lines not on this page · raw message

drivers/media/video/smiapp/smiapp-regs.c new file +230 -0

230 lines not on this page · raw message

drivers/media/video/smiapp/smiapp-regs.h new file +46 -0

46 lines not on this page · raw message

drivers/media/video/smiapp/smiapp.h new file +253 -0

253 lines not on this page · raw message

include/media/smiapp.h new file +83 -0

83 lines not on this page · raw message

The rest of this diff is not shown. View the raw message.

Signature · 1 line

1.7.2.5

Part of the diff is not shown here. View the raw message.

Sakari Ailus

author

2012-01-11 21:27 UTC #49 lore raw

PATCH 23/23 rm680: Add camera init

In reply to Sakari Ailus

From: Sakari Ailus <[email protected]>

This currently introduces an extra file to the arch/arm/mach-omap2
directory: board-rm680-camera.c. Keeping the device tree in mind, the context of the file could be represented as static data with one exception:
the external clock to the sensor.

This external clock is provided by the OMAP 3 SoC and required by the sensor. The issue is that the clock originates from the ISP and not from PRCM block as the other clocks and thus is not supported by the clock framework. Otherwise the sensor driver could just clk_get() and clk_enable() it, just like the regulators and gpios.

Signed-off-by Sakari Ailus <[email protected]>

---
 arch/arm/mach-omap2/Makefile             |    3 +-
 arch/arm/mach-omap2/board-rm680-camera.c |  375 ++++++++++++++++++++++++++++++
 arch/arm/mach-omap2/board-rm680.c        |   38 +++
 3 files changed, 415 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/mach-omap2/board-rm680-camera.c

arch/arm/mach-omap2/Makefile +2 -1

index 69ab1c0..1444bc5 100644
@@ -201,7 +201,8 @@ obj-$(CONFIG_MACH_OMAP3_PANDORA)	+= board-omap3pandora.o
 obj-$(CONFIG_MACH_OMAP_3430SDP)		+= board-3430sdp.o
 obj-$(CONFIG_MACH_NOKIA_N8X0)		+= board-n8x0.o
 obj-$(CONFIG_MACH_NOKIA_RM680)		+= board-rm680.o \
-					   sdram-nokia.o
+					   sdram-nokia.o \
+					   board-rm680-camera.o
 obj-$(CONFIG_MACH_NOKIA_RX51)		+= board-rx51.o \
 					   sdram-nokia.o \
 					   board-rx51-peripherals.o \

arch/arm/mach-omap2/board-rm680-camera.c new file +375 -0

375 lines not on this page · raw message

arch/arm/mach-omap2/board-rm680.c +38 -0

index a5bcc75..2c5c4db 100644
@@ -66,6 +66,39 @@ static struct platform_device rm680_vemmc_device = {
 	},
 };
 
+#define REGULATOR_INIT_DATA(_name, _min, _max, _apply, _ops_mask) \
+	static struct regulator_init_data _name##_data = { \
+		.constraints = { \
+			.name                   = #_name, \
+			.min_uV                 = _min, \
+			.max_uV                 = _max, \
+			.apply_uV               = _apply, \
+			.valid_modes_mask       = REGULATOR_MODE_NORMAL | \
+						REGULATOR_MODE_STANDBY, \
+			.valid_ops_mask         = _ops_mask, \
+		}, \
+		.num_consumer_supplies  = ARRAY_SIZE(_name##_consumers), \
+		.consumer_supplies      = _name##_consumers, \
+}
+#define REGULATOR_INIT_DATA_FIXED(_name, _voltage) \
+	REGULATOR_INIT_DATA(_name, _voltage, _voltage, true, \
+				REGULATOR_CHANGE_STATUS | REGULATOR_CHANGE_MODE)
+
+static struct regulator_consumer_supply rm680_vaux2_consumers[] = {
+	REGULATOR_SUPPLY("VDD_CSIPHY1", "omap3isp"),	/* OMAP ISP */
+	REGULATOR_SUPPLY("VDD_CSIPHY2", "omap3isp"),	/* OMAP ISP */
+	REGULATOR_SUPPLY("vaux2", NULL),
+};
+REGULATOR_INIT_DATA_FIXED(rm680_vaux2, 1800000);
+
+static struct regulator_consumer_supply rm680_vaux3_consumers[] = {
+	REGULATOR_SUPPLY("VANA", "2-0037"),	/* Main Camera Sensor */
+	REGULATOR_SUPPLY("VANA", "2-000e"),	/* Main Camera Lens */
+	REGULATOR_SUPPLY("VANA", "2-0010"),	/* Front Camera */
+	REGULATOR_SUPPLY("vaux3", NULL),
+};
+REGULATOR_INIT_DATA_FIXED(rm680_vaux3, 2800000);
+
 static struct platform_device *rm680_peripherals_devices[] __initdata = {
 	&rm680_vemmc_device,
 };

@@ -82,6 +115,8 @@ static struct twl4030_gpio_platform_data rm680_gpio_data = {
 static struct twl4030_platform_data rm680_twl_data = {
 	.gpio			= &rm680_gpio_data,
 	/* add rest of the children here */
+	.vaux2			= &rm680_vaux2_data,
+	.vaux3			= &rm680_vaux3_data,
 };
 
 static void __init rm680_i2c_init(void)

@@ -129,6 +164,8 @@ static struct omap_board_mux board_mux[] __initdata = {
 };
 #endif
 
+void rm680_camera_init(void);
+
 static void __init rm680_init(void)
 {
 	struct omap_sdrc_params *sdrc_params;

@@ -141,6 +178,7 @@ static void __init rm680_init(void)
 
 	usb_musb_init(NULL);
 	rm680_peripherals_init();
+	rm680_camera_init();
 }
 
 MACHINE_START(NOKIA_RM680, "Nokia RM-680 board")

Signature · 1 line

1.7.2.5