[PATCH 0/6] media: v4l2-ctrls: Add camera sensor location

[PATCH 0/6] media: v4l2-ctrls: Add camera sensor location

[Jacopo Mondi]

Hello,
  after yesterday's RFC I'm now sending a proper v1 for inclusion.
Thanks Hans, Laurent and Sakari for the comments on the RFC.

I have expanded the documentation, finalized on an integer control to expose
the camera sensor location, and provided and helper in v4l2-fwnode to parse
the firmware property and register the associated control.

On the helper: right now only 'location' is parsed and the assoiated
V4L2_CID_CAMERA_SENSOR_LOCATION control registered. Ideally, I would have liked
to support more properties coming from firmware and exposed through controls,
but:
- V4L2_CID_LINK_FREQ which corresponds to the 'link-frequencies' property is
  never directly parsed for firmware and exposed as a control in any mainline
  driver. The contrary is actually true: all drivers but smiapp register values
  not coming from DT but hardcoded in the driver itself. smiapp parses the link
  frequencies from DT but does not expose them directly. This makes me wonder
  about the actual purpose of the property if nobody uses that.
  Should drivers be moved to retrieve the property from firmware instead?
- V4L2_CID_PIXEL_RATE depends on the link frequencies, number of lanes and
  configured image format. It seems better handled by drivers instead of in an
  helper considering how many parameters are required to calculate it.

So the newly added function just parses and expose the camera location, which
makes me wonder if it's worth an helper at all. What other properties could be
parsed and registered as controls which I have missed?

Thanks
   j

Jacopo Mondi (6):
  media: dt-bindings: Document 'location' property
  media: v4l2-ctrl: Document V4L2_CID_LOCATION
  media: v4l2-ctrls: Add support for V4L2_CID_LOCATION
  media: v4l2-fwnode: Add helper to register controls from fw
  media: i2c: ov5670: Register controls from firmware
  media: i2c: ov13858: Register controls from firmware

 .../bindings/media/video-interfaces.txt       | 10 +++++
 .../media/uapi/v4l/ext-ctrls-camera.rst       | 34 ++++++++++++++++
 drivers/media/i2c/ov13858.c                   |  8 ++++
 drivers/media/i2c/ov5670.c                    |  7 ++++
 drivers/media/v4l2-core/v4l2-ctrls.c          |  2 +
 drivers/media/v4l2-core/v4l2-fwnode.c         | 40 +++++++++++++++++++
 include/media/v4l2-fwnode.h                   | 29 ++++++++++++++
 include/uapi/linux/v4l2-controls.h            |  5 +++
 8 files changed, 135 insertions(+)

--
2.22.0

[PATCH 1/6] media: dt-bindings: Document 'location' property

[Jacopo Mondi]

Add the 'location' device property, used to specify the camera device
mounting position. The property is particularly meaningful for mobile
devices with a well defined usage orientation.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 .../devicetree/bindings/media/video-interfaces.txt     | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
index f884ada0bffc..865f4142f432 100644
--- a/Documentation/devicetree/bindings/media/video-interfaces.txt
+++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
@@ -89,6 +89,16 @@ Optional properties
   but a number of degrees counter clockwise. Typical values are 0 and 180
   (upside down).

+- location: The camera sensor mounting location, expressed as a position
+  relative to the usage orientation of the device the sensor is installed on.
+  Possible values are:
+  0 - Front. The image sensor is mounted on the front facing side of the device.
+  For mobile devices such as smartphones, tablets and laptops the front side is
+  the user facing side of the device.
+  1 - Back. The image sensor is mounted on the back side of the device, which is
+  defined as the opposite side of the front facing one.
+  2 - External. The image sensor is connected to the device by extension cables,
+  and can be freely moved, regardless of the device position.

 Optional endpoint properties
 ----------------------------
--
2.22.0

[PATCH 2/6] media: v4l2-ctrl: Document V4L2_CID_LOCATION

[Jacopo Mondi]

Add documentation for the V4L2_CID_LOCATION camera control. The newly
added read-only control reports the camera device mounting position.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 .../media/uapi/v4l/ext-ctrls-camera.rst       | 34 +++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
index 51c1d5c9eb00..ecf151f3f0f4 100644
--- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
+++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
@@ -510,6 +510,40 @@ enum v4l2_scene_mode -
     value down. A value of zero stops the motion if one is in progress
     and has no effect otherwise.
 
+``V4L2_CID_CAMERA_SENSOR_LOCATION (integer)``
+    This read-only control describes the camera sensor location by
+    reporting its mounting position on the device where the camera is
+    installed. The control value is constant and not modifiable by software
+    and its value is retrieved from the firmware interface by parsing the
+    'location' property. This control is particularly meaningful for
+    devices which have a well defined orientation, such as phones, laptops
+    and portable devices as the camera location is expressed as a position
+    relative to the device intended usage orientation. In example, a camera
+    sensor installed on the user-facing side of a phone, a tablet or a
+    laptop device is said to be installed in the ``V4L2_LOCATION_FRONT``
+    location while camera sensors installed on the side opposed to the
+    front one are said to be installed in the ``V4L2_LOCATION_BACK``
+    location. Camera sensors connected to the device by extension cables
+    which are freely movable regardless of the device orientation, such as
+    webcams and digital cameras, are said to be have
+    ``V4L2_LOCATION_EXTERNAL`` location.
+
+
+
+.. flat-table::
+    :header-rows:  0
+    :stub-columns: 0
+
+    * - ``V4L2_LOCATION_FRONT``
+      - The camera sensor is located on the front side of the device.
+    * - ``V4L2_LOCATION_BACK``
+      - The camera sensor is located on the back side of the device.
+    * - ``V4L2_LOCATION_EXTERNAL``
+      - The camera sensor is connected by extension cables to the device and
+        it's freely movable.
+
+
+
 .. [#f1]
    This control may be changed to a menu control in the future, if more
    options are required.
-- 
2.22.0

[PATCH 3/6] media: v4l2-ctrls: Add support for V4L2_CID_LOCATION

[Jacopo Mondi]

Add support for the newly defined V4L2_CID_LOCATION read-only control
used to report the camera device mounting position.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 2 ++
 include/uapi/linux/v4l2-controls.h   | 5 +++++
 2 files changed, 7 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index cd1ae016706f..02b60fe02409 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -978,6 +978,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_AUTO_FOCUS_RANGE:		return "Auto Focus, Range";
 	case V4L2_CID_PAN_SPEED:		return "Pan, Speed";
 	case V4L2_CID_TILT_SPEED:		return "Tilt, Speed";
+	case V4L2_CID_CAMERA_SENSOR_LOCATION:	return "Camera Sensor Location";
 
 	/* FM Radio Modulator controls */
 	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1300,6 +1301,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 		break;
 	case V4L2_CID_MIN_BUFFERS_FOR_CAPTURE:
 	case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT:
+	case V4L2_CID_CAMERA_SENSOR_LOCATION:
 		*type = V4L2_CTRL_TYPE_INTEGER;
 		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
 		break;
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index a2669b79b294..387c2c8553cb 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -912,6 +912,11 @@ enum v4l2_auto_focus_range {
 #define V4L2_CID_PAN_SPEED			(V4L2_CID_CAMERA_CLASS_BASE+32)
 #define V4L2_CID_TILT_SPEED			(V4L2_CID_CAMERA_CLASS_BASE+33)
 
+#define V4L2_CID_CAMERA_SENSOR_LOCATION		(V4L2_CID_CAMERA_CLASS_BASE+34)
+#define V4L2_LOCATION_FRONT			0
+#define V4L2_LOCATION_BACK			1
+#define V4L2_LOCATION_EXTERNAL			2
+
 /* FM Modulator class control IDs */
 
 #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
-- 
2.22.0

[PATCH 4/6] media: v4l2-fwnode: Add helper to register controls from fw

[Jacopo Mondi]

Add the 'v4l2_fwnode_register_controls()' helper to v4l2-fwnode. The
function parses the device node and endpoint firmware properties to
which a v4l2 control is associated to and register the control with the
provided handler.

Currently, only V4L2_CID_CAMERA_LOCATION is supported.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/v4l2-core/v4l2-fwnode.c | 40 +++++++++++++++++++++++++++
 include/media/v4l2-fwnode.h           | 29 +++++++++++++++++++
 2 files changed, 69 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index 3bd1888787eb..8ad7f70aa2f1 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -25,6 +25,7 @@
 #include <linux/types.h>
 
 #include <media/v4l2-async.h>
+#include <media/v4l2-ctrls.h>
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-subdev.h>
 
@@ -595,6 +596,45 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
 }
 EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
 
+int v4l2_fwnode_register_controls(struct fwnode_handle *fwnode,
+				  struct v4l2_ctrl_handler *hdl,
+				  const struct v4l2_ctrl_ops *ctrl_ops)
+{
+	u32 val;
+	int ret;
+
+	ret = fwnode_property_read_u32(fwnode, "location", &val);
+	if (ret)
+		return 0;
+
+	switch (val) {
+	case V4L2_LOCATION_FRONT:
+	case V4L2_LOCATION_BACK:
+	case V4L2_LOCATION_EXTERNAL:
+		break;
+	default:
+		pr_warn("Unsupported location: %u\n", val);
+		return -EINVAL;
+	}
+
+	if (v4l2_ctrl_find(hdl, V4L2_CID_CAMERA_SENSOR_LOCATION))
+		pr_debug("Skip control '%s': already registered",
+			 v4l2_ctrl_get_name(V4L2_CID_CAMERA_SENSOR_LOCATION));
+	else
+		v4l2_ctrl_new_std(hdl, ctrl_ops,
+				  V4L2_CID_CAMERA_SENSOR_LOCATION,
+				  val, val, 1, val);
+
+	if (hdl->error) {
+		pr_warn("Failed to register controls from firmware: %d\n",
+			hdl->error);
+		return hdl->error;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_fwnode_register_controls);
+
 static int
 v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
 					  struct v4l2_async_notifier *notifier,
diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
index f6a7bcd13197..942c4c21080b 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -25,6 +25,8 @@
 struct fwnode_handle;
 struct v4l2_async_notifier;
 struct v4l2_async_subdev;
+struct v4l2_ctrl_handler;
+struct v4l2_ctrl_ops;
 
 #define V4L2_FWNODE_CSI2_MAX_DATA_LANES	4
 
@@ -233,6 +235,33 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
  */
 void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
 
+/**
+ * v4l2_fwnode_register_controls() - parse device and endpoint fwnode
+ *				     properties and register a v4l2 control
+ *				     for each of them
+ * @fwnode: pointer to the device fwnode handle
+ * @hdl: pointer to the v4l2 control handler to register controls with
+ * @ctrl_ops: pointer to the v4l2 control operations to register with the handler
+ *
+ * Parse the @fwnode device and endpoint properties to which a v4l2 control
+ * is associated and register them with the provided handler @hdl.
+ * Currently the following v4l2 controls are parsed and registered:
+ * - V4L2_CID_CAMERA_SENSOR_LOCATION;
+ *
+ * Controls already registered by the caller with the @hdl control handler
+ * are not overwritten by this function. Callers should register the controls
+ * they want to handle themeselves before calling this function.
+ *
+ * NOTE: This function locks the @hdl control handler mutex, the caller shall
+ * not hold the lock when calling this function.
+ *
+ * Return: 0 on success, -EINVAL if the fwnode properties are not well
+ * specified.
+ */
+int v4l2_fwnode_register_controls(struct fwnode_handle *fwnode,
+				  struct v4l2_ctrl_handler *hdl,
+				  const struct v4l2_ctrl_ops *ctrl_ops);
+
 /**
  * typedef parse_endpoint_func - Driver's callback function to be called on
  *	each V4L2 fwnode endpoint.
-- 
2.22.0

[PATCH 5/6] media: i2c: ov5670: Register controls from firmware

[Jacopo Mondi]

Use the newly introduced helper to parse properties from firmware and
registerer the associated v4l2 control.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov5670.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 041fcbb4eebd..2bc57e85f721 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -7,6 +7,7 @@
 #include <linux/pm_runtime.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
+#include <media/v4l2-fwnode.h>
 
 #define OV5670_REG_CHIP_ID		0x300a
 #define OV5670_CHIP_ID			0x005670
@@ -2059,6 +2060,7 @@ static const struct v4l2_ctrl_ops ov5670_ctrl_ops = {
 /* Initialize control handlers */
 static int ov5670_init_controls(struct ov5670 *ov5670)
 {
+	struct i2c_client *client = v4l2_get_subdevdata(&ov5670->sd);
 	struct v4l2_ctrl_handler *ctrl_hdlr;
 	s64 vblank_max;
 	s64 vblank_def;
@@ -2129,6 +2131,11 @@ static int ov5670_init_controls(struct ov5670 *ov5670)
 		goto error;
 	}
 
+	ret = v4l2_fwnode_register_controls(dev_fwnode(&client->dev),
+					    ctrl_hdlr, &ov5670_ctrl_ops);
+	if (ret)
+		goto error;
+
 	ov5670->sd.ctrl_handler = ctrl_hdlr;
 
 	return 0;
-- 
2.22.0

[PATCH 6/6] media: i2c: ov13858: Register controls from firmware

[Jacopo Mondi]

Use the newly introduced helper to parse properties from firmware and
registerer the associated v4l2 control.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov13858.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/media/i2c/ov13858.c b/drivers/media/i2c/ov13858.c
index aac6f77afa0f..aa239cc5a921 100644
--- a/drivers/media/i2c/ov13858.c
+++ b/drivers/media/i2c/ov13858.c
@@ -7,6 +7,7 @@
 #include <linux/pm_runtime.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
+#include <media/v4l2-fwnode.h>
 
 #define OV13858_REG_VALUE_08BIT		1
 #define OV13858_REG_VALUE_16BIT		2
@@ -1666,6 +1667,13 @@ static int ov13858_init_controls(struct ov13858 *ov13858)
 		goto error;
 	}
 
+	ret = v4l2_fwnode_register_controls(dev_fwnode(&client->dev),
+					    ctrl_hdlr, &ov13858_ctrl_ops);
+	if (ret)
+		goto error;
+
+
+
 	ov13858->sd.ctrl_handler = ctrl_hdlr;
 
 	return 0;
-- 
2.22.0