[PATCH v4 00/11] media: Report camera sensor properties

[PATCH v4 00/11] media: Report camera sensor properties

[Jacopo Mondi]

Hello, fourth iteration following:

"media: v4l2-ctrls: Add camera sensor location"
https://patchwork.kernel.org/project/linux-media/list/?series=160901
"[v2,00/10] media: Report camera sensor properties
https://patchwork.kernel.org/cover/11116443/
"[PATCH v3 00/11] media: Report camera sensor properties"
https://patchwork.kernel.org/project/linux-media/list/?series=173571

I here included Hans' comments. Most notable changes

v3 -> v4:
- Minor reword in documentation of location and rotation properties
- Fix V4L2_CID_CAMERA_SENSOR_ROTATION control documentation
- Renamed helper in v4l2_ctrl_new_fwnode_properties()
v2->v3:
- Expand 'rotation' property description
- s/device/system in properties description to make them applicable to
  cameras and flash LEDs
- Expand the rotation control description
- Split helper to parse properties and helper to register properties
- Drop the example coreboot patch that add properties to the Soraka device
  ACPI tables

I know there are still doubts the two properties might well apply to
modern devices with movable cameras, but I still think they cover 99% of devices
out there at the moment.

Thanks
   j

Jacopo Mondi (11):
  dt-bindings: video-interfaces: Document 'location' property
  media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_LOCATION
  dt-bindings: video-interface: Expand rotation description
  media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_ROTATION
  media: v4l2-ctrls: Add camera location and rotation
  media: v4l2-fwnode: Add helper to parse device properties
  include: v4l2-ctrl: Sort forward declarations
  media: v4l2-ctrls: Sort includes alphabetically
  media: v4l2-ctrls: Add helper to register properties
  media: i2c: ov5670: Parse and register properties
  media: i2c: ov13858: Parse and register properties

 .../bindings/media/video-interfaces.txt       |  21 ++-
 .../media/uapi/v4l/ext-ctrls-camera.rst       | 148 ++++++++++++++++++
 drivers/media/i2c/ov13858.c                   |  11 ++
 drivers/media/i2c/ov5670.c                    |  12 ++
 drivers/media/v4l2-core/v4l2-ctrls.c          |  52 +++++-
 drivers/media/v4l2-core/v4l2-fwnode.c         |  42 +++++
 include/media/v4l2-ctrls.h                    |  34 +++-
 include/media/v4l2-fwnode.h                   |  48 ++++++
 include/uapi/linux/v4l2-controls.h            |   7 +
 9 files changed, 363 insertions(+), 12 deletions(-)

--
2.23.0

[PATCH v4 01/11] dt-bindings: video-interfaces: Document 'location' property

[Jacopo Mondi]

Add the 'location' device property, used to specify a 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    | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
index f884ada0bffc..1211bdf80722 100644
--- a/Documentation/devicetree/bindings/media/video-interfaces.txt
+++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
@@ -89,6 +89,17 @@ Optional properties
   but a number of degrees counter clockwise. Typical values are 0 and 180
   (upside down).
 
+- location: The mount location of a device (typically an image sensor or a flash
+  LED) expressed as a position relative to the usage orientation of the system
+  where the device is installed on.
+  Possible values are:
+  0 - Front. The device is mounted on the front facing side of the system.
+  For mobile devices such as smartphones, tablets and laptops the front side is
+  the user facing side.
+  1 - Back. The device is mounted on the back side of the system, which is
+  defined as the opposite side of the front facing one.
+  2 - External. The device is not attached directly to the system but is
+  attached in a way that allows it to move freely.
 
 Optional endpoint properties
 ----------------------------
-- 
2.23.0

[PATCH v4 02/11] media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_LOCATION

[Jacopo Mondi]

Add documentation for the V4L2_CID_CAMERA_SENSOR_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       | 32 +++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
index 51c1d5c9eb00..b151c016256c 100644
--- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
+++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
@@ -510,6 +510,38 @@ 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. This control is
+    particularly meaningful for devices which have a well defined orientation,
+    such as phones, laptops and portable devices since the camera location is
+    expressed as a position relative to the device's intended usage orientation.
+    For 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
+    opposite the front one are said to be installed in the
+    ``V4L2_LOCATION_BACK`` location. Camera sensors not directly attached to
+    the device or attached in a way that allows them to move freely, such as
+    webcams and digital cameras, are said to have the ``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 not directly attached to the device and is
+        freely movable.
+
+
+
 .. [#f1]
    This control may be changed to a menu control in the future, if more
    options are required.
-- 
2.23.0

[PATCH v4 03/11] dt-bindings: video-interface: Expand rotation description

[Jacopo Mondi]

Expand the 'rotation' property description to define the direction and
orientation of the axis around which the device mounting rotation is
expressed.

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

diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
index 1211bdf80722..58b87a3f1fa4 100644
--- a/Documentation/devicetree/bindings/media/video-interfaces.txt
+++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
@@ -85,9 +85,11 @@ Optional properties
 
 - lens-focus: A phandle to the node of the focus lens controller.
 
-- rotation: The device, typically an image sensor, is not mounted upright,
-  but a number of degrees counter clockwise. Typical values are 0 and 180
-  (upside down).
+- rotation: The mount rotation of the device (typically an image sensor)
+  expressed as counterclockwise rotation degrees along the axis perpendicular to
+  the device mounting surface directed away from it. Typical values are 0
+  degrees for upright mounted devices and 180 degrees for devices mounted upside
+  down.
 
 - location: The mount location of a device (typically an image sensor or a flash
   LED) expressed as a position relative to the usage orientation of the system
-- 
2.23.0

[PATCH v4 04/11] media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_ROTATION

[Jacopo Mondi]

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

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

diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
index b151c016256c..e1fee4814e5b 100644
--- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
+++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
@@ -542,6 +542,122 @@ enum v4l2_scene_mode -
 
 
 
+``V4L2_CID_CAMERA_SENSOR_ROTATION (integer)``
+    This read-only control describes the sensor orientation expressed as
+    rotation in counterclockwise degrees along the axis perpendicular to the
+    device mounting plane, and directed away from the sensor lens. Possible
+    values for the control are 90, 180 and 270 degrees. To compensate for the
+    device mounting rotation on the captured images, a rotation of the same
+    amount of degrees, in the same counterclockwise rotation direction should be
+    applied along the axis directed from the observer to the captured image when
+    displayed on a screen.
+
+    To better understand the effect of the sensor rotation on the acquired
+    images when displayed on a screen, it is helpful to consider a fictional
+    scan-out sequence of the sensor's pixels, assuming the pixel array having
+    its top-left pixel at position (0, 0) with values on the 'x' axis increasing
+    towards the right direction, and values on the 'y' axis increasing towards
+    the bottom. The effect of sensor rotation could be easily visualized
+    considering the sequence of captured pixels.
+
+    Assuming the following scene has to be captured::
+
+                o
+               -|-
+               / \
+
+    An upright mounted sensor has its pixel array displaced as follow::
+
+                                   x
+            (0,0)------------------->
+              ! 0,0 0,1 0,2 ... (0,num-col)
+              ! 1,0 1,1 1,2 ...
+              ! ...
+              ! ...
+              ! (num-lines,0)...(num-col,num-lines)
+            y V
+
+
+    Assuming pixels are scanned out from (0,0) to (num-lines,num-col)
+    progressively::
+
+             (0,0) ---->------------->   (0,num-col)
+             (1,0) ---->------------->   (1,num-col)
+             ( .... )-->------------->   (   ....   )
+             (num-lines,0)----------->(num-lines,num-col)
+
+
+    If a rotation of 90 degrees counterclockwise along the axis perpendicular to
+    the sensor's lens and directed towards the scene to be captured is applied
+    to the sensor, the pixel array would then be rotated as follows::
+
+            x ^ (0,num-col) ...   ...   (num-lines,num-col)
+              !  ....
+              !  0,2        1,2   ...          ...
+              !  0,1        1,1   ...          ...
+              !  0,0        1,0   ...      (num-lines,0)
+             (0,0)------------------------------------>
+                                                    y
+
+    And the pixel scan-out sequence would then proceed as follows starting
+    from pixel (0,0)::
+
+           (0,num-col)         (num-lines,num-col)
+                ^    ^   ^   ^     ^
+                !    !   !   !     !
+                !    !   !   !     !
+                !    !   !   !     !
+                !    !   !   !     !
+              (0,0) (1,0)....  (num-lines,0)
+
+    Which when applied to the capture scene gives::
+
+           (0,num-col)         (num-lines,num-col)
+                ^    ^   ^   ^     ^
+                !    !   0   !     !
+                !    !  -|- !     !
+                !    !  /!\  !     !
+                !    !   !   !     !
+              (0,0) (1,0)....  (num-lines,0)
+
+    Producing the following image once captured to memory and
+    displayed to the user::
+
+             \ !
+               --0
+             / !
+
+    Which has a rotation of the same amount of degrees applied on the opposite
+    rotation direction along the axis that goes from the observer to the
+    image when displayed on the screen.
+
+    In order to compensate the sensor mounting rotation, when expressed
+    as counterclockwise rotation along the axis directed from the sensor to
+    the captured scene, a rotation of the same amount of degrees in the
+    same counterclockwise rotation direction but applied along the axis
+    directed from the observer to the captured image, has to be applied::
+
+                +------+  90 degree counterclockwise
+                |   o  |  mounting rotation applied
+                |  -|- |  along the axis directed
+                |  / \ |  away from the sensor lens
+                +------+
+                    |
+                    V
+                +------+
+                | \ !  |  Resulting captured
+                |  --0 |  image when displayed
+                | / !  |  on screen
+                +------+
+                    |
+                    V
+                +------+
+                |   o  |  Rotation compensation
+                |  -|- |  is 90 degrees counterclockwise
+                |  / \ |  along the axis directed to the
+                +------+  displayed image
+
+
 .. [#f1]
    This control may be changed to a menu control in the future, if more
    options are required.
-- 
2.23.0

[PATCH v4 05/11] media: v4l2-ctrls: Add camera location and rotation

[Jacopo Mondi]

Add support for the newly defined V4L2_CID_CAMERA_SENSOR_LOCATION
and V4L2_CID_CAMERA_SENSOR_ROTATION read-only controls used to report
the camera device mounting position and orientation respectively.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 4 ++++
 include/uapi/linux/v4l2-controls.h   | 7 +++++++
 2 files changed, 11 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 1d8f38824631..b7af47a25125 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -994,6 +994,8 @@ 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";
+	case V4L2_CID_CAMERA_SENSOR_ROTATION:	return "Camera Sensor Rotation";
 
 	/* FM Radio Modulator controls */
 	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1318,6 +1320,8 @@ 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:
+	case V4L2_CID_CAMERA_SENSOR_ROTATION:
 		*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..f2be7a99818e 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -912,6 +912,13 @@ 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
+
+#define V4L2_CID_CAMERA_SENSOR_ROTATION		(V4L2_CID_CAMERA_CLASS_BASE+35)
+
 /* FM Modulator class control IDs */
 
 #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
-- 
2.23.0

[PATCH v4 06/11] media: v4l2-fwnode: Add helper to parse device properties

[Jacopo Mondi]

Add an helper function to parse common device properties in the same
way as v4l2_fwnode_endpoint_parse() parses common endpoint properties.

Parse the 'rotation' and 'location' properties from the firmware
interface.

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

diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index 192cac076761..8af4174a2bbf 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -596,6 +596,48 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
 }
 EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
 
+int v4l2_fwnode_device_parse(struct device *dev,
+			     struct v4l2_fwnode_device_properties *props)
+{
+	struct fwnode_handle *fwnode = dev_fwnode(dev);
+	u32 val;
+	int ret;
+
+	memset(props, 0, sizeof(*props));
+
+	props->location = V4L2_FWNODE_PROPERTY_UNSET;
+	ret = fwnode_property_read_u32(fwnode, "location", &val);
+	if (!ret) {
+		switch (val) {
+		case V4L2_FWNODE_LOCATION_FRONT:
+		case V4L2_FWNODE_LOCATION_BACK:
+		case V4L2_FWNODE_LOCATION_EXTERNAL:
+			break;
+		default:
+			dev_warn(dev, "Unsupported device location: %u\n", val);
+			return -EINVAL;
+		}
+
+		props->location = val;
+		dev_dbg(dev, "device location: %u\n", val);
+	}
+
+	props->rotation = V4L2_FWNODE_PROPERTY_UNSET;
+	ret = fwnode_property_read_u32(fwnode, "rotation", &val);
+	if (!ret) {
+		if (val >= 360 || val % 90) {
+			dev_warn(dev, "Unsupported device rotation: %u\n", val);
+			return -EINVAL;
+		}
+
+		props->rotation = val;
+		dev_dbg(dev, "device rotation: %u\n", val);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_fwnode_device_parse);
+
 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..6d46d6fc3007 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -109,6 +109,36 @@ struct v4l2_fwnode_endpoint {
 	unsigned int nr_of_link_frequencies;
 };
 
+/**
+ * v4l2_fwnode_location - identify a non initialized property.
+ *
+ * All properties in &struct v4l2_fwnode_device_properties are initialized
+ * to this value.
+ */
+#define V4L2_FWNODE_PROPERTY_UNSET   (-1U)
+
+/**
+ * enum v4l2_fwnode_location - possible device locations
+ * @V4L2_FWNODE_LOCATION_FRONT: device installed on the front side
+ * @V4L2_FWNODE_LOCATION_BACK: device installed on the back side
+ * @V4L2_FWNODE_LOCATION_EXTERNAL: device externally located
+ */
+enum v4l2_fwnode_location {
+	V4L2_FWNODE_LOCATION_FRONT,
+	V4L2_FWNODE_LOCATION_BACK,
+	V4L2_FWNODE_LOCATION_EXTERNAL
+};
+
+/**
+ * struct v4l2_fwnode_device_properties - fwnode device properties
+ * @location: device location. See &enum v4l2_fwnode_location
+ * @rotation: device rotation
+ */
+struct v4l2_fwnode_device_properties {
+	enum v4l2_fwnode_location location;
+	unsigned int rotation;
+};
+
 /**
  * struct v4l2_fwnode_link - a link between two endpoints
  * @local_node: pointer to device_node of this endpoint
@@ -233,6 +263,24 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
  */
 void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
 
+/**
+ * v4l2_fwnode_device_parse() - parse fwnode device properties
+ * @dev: pointer to &struct device
+ * @props: pointer to &struct v4l2_fwnode_device_properties where to store the
+ *	   parsed properties values
+ *
+ * This function parses and validates the V4L2 fwnode device properties from
+ * the firmware interface. It is responsibility of the caller to allocate a
+ * valid @struct v4l2_fwnode_device_properties structure and provide a pointer
+ * to it in the @props parameter.
+ *
+ * Return:
+ *	% 0 on success
+ *	%-EINVAL if a parsed property value is not valid
+ */
+int v4l2_fwnode_device_parse(struct device *dev,
+			     struct v4l2_fwnode_device_properties *props);
+
 /**
  * typedef parse_endpoint_func - Driver's callback function to be called on
  *	each V4L2 fwnode endpoint.
-- 
2.23.0

[PATCH v4 07/11] include: v4l2-ctrl: Sort forward declarations

[Jacopo Mondi]

Before adding a new forward declaration to the v4l2-ctrls.h header file,
sort the existing ones alphabetically.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/media/v4l2-ctrls.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 570ff4b0205a..95b4fa6243d1 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -24,14 +24,14 @@
 
 /* forward references */
 struct file;
+struct poll_table_struct;
+struct v4l2_ctrl;
 struct v4l2_ctrl_handler;
 struct v4l2_ctrl_helper;
-struct v4l2_ctrl;
-struct video_device;
+struct v4l2_fh;
 struct v4l2_subdev;
 struct v4l2_subscribed_event;
-struct v4l2_fh;
-struct poll_table_struct;
+struct video_device;
 
 /**
  * union v4l2_ctrl_ptr - A pointer to a control value.
-- 
2.23.0

[PATCH v4 08/11] media: v4l2-ctrls: Sort includes alphabetically

[Jacopo Mondi]

Before adding a new include directive, sort the existing ones in
alphabetical order.

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

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index b7af47a25125..6fb94968a98d 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -9,14 +9,14 @@
 #define pr_fmt(fmt) "v4l2-ctrls: " fmt
 
 #include <linux/ctype.h>
+#include <linux/export.h>
 #include <linux/mm.h>
 #include <linux/slab.h>
-#include <linux/export.h>
-#include <media/v4l2-ioctl.h>
-#include <media/v4l2-device.h>
 #include <media/v4l2-ctrls.h>
-#include <media/v4l2-event.h>
 #include <media/v4l2-dev.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-event.h>
+#include <media/v4l2-ioctl.h>
 
 #define dprintk(vdev, fmt, arg...) do {					\
 	if (!WARN_ON(!(vdev)) && ((vdev)->dev_debug & V4L2_DEV_DEBUG_CTRL)) \
-- 
2.23.0

[PATCH v4 09/11] media: v4l2-ctrls: Add helper to register properties

[Jacopo Mondi]

Add an helper function to v4l2-ctrls to register controls associated
with a device property.

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

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 6fb94968a98d..50e9daa7a75a 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -16,6 +16,7 @@
 #include <media/v4l2-dev.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-event.h>
+#include <media/v4l2-fwnode.h>
 #include <media/v4l2-ioctl.h>
 
 #define dprintk(vdev, fmt, arg...) do {					\
@@ -4417,3 +4418,42 @@ __poll_t v4l2_ctrl_poll(struct file *file, struct poll_table_struct *wait)
 	return 0;
 }
 EXPORT_SYMBOL(v4l2_ctrl_poll);
+
+int v4l2_ctrl_new_fwnode_properties(struct v4l2_ctrl_handler *hdl,
+				    const struct v4l2_ctrl_ops *ctrl_ops,
+				    const struct v4l2_fwnode_device_properties *p)
+{
+	if (p->location != V4L2_FWNODE_PROPERTY_UNSET) {
+		u32 location_ctrl;
+
+		switch (p->location) {
+		case V4L2_FWNODE_LOCATION_FRONT:
+			location_ctrl = V4L2_LOCATION_FRONT;
+			break;
+		case V4L2_FWNODE_LOCATION_BACK:
+			location_ctrl = V4L2_LOCATION_BACK;
+			break;
+		case V4L2_FWNODE_LOCATION_EXTERNAL:
+			location_ctrl = V4L2_LOCATION_EXTERNAL;
+			break;
+		default:
+			return -EINVAL;
+		}
+		if (!v4l2_ctrl_new_std(hdl, ctrl_ops,
+				       V4L2_CID_CAMERA_SENSOR_LOCATION,
+				       location_ctrl, location_ctrl, 1,
+				       location_ctrl))
+			return hdl->error;
+	}
+
+	if (p->rotation != V4L2_FWNODE_PROPERTY_UNSET) {
+		if (!v4l2_ctrl_new_std(hdl, ctrl_ops,
+				       V4L2_CID_CAMERA_SENSOR_ROTATION,
+				       p->rotation, p->rotation, 1,
+				       p->rotation))
+			return hdl->error;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(v4l2_ctrl_new_fwnode_properties);
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 95b4fa6243d1..6bf8c304c431 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -29,6 +29,7 @@ struct v4l2_ctrl;
 struct v4l2_ctrl_handler;
 struct v4l2_ctrl_helper;
 struct v4l2_fh;
+struct v4l2_fwnode_device_properties;
 struct v4l2_subdev;
 struct v4l2_subscribed_event;
 struct video_device;
@@ -1330,4 +1331,29 @@ int v4l2_ctrl_subdev_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
  */
 int v4l2_ctrl_subdev_log_status(struct v4l2_subdev *sd);
 
+/**
+ * v4l2_ctrl_new_fwnode_properties() - Register controls for the device
+ *				       properties
+ *
+ * @hdl: pointer to &struct v4l2_ctrl_handler to register controls on
+ * @ctrl_ops: pointer to &struct v4l2_ctrl_ops to register controls with
+ * @p: pointer to &struct v4l2_fwnode_device_properties
+ *
+ * This function registers controls associated to device properties, using the
+ * property values contained in @p parameter, if the property has been set to
+ * a value.
+ *
+ * Currently the following v4l2 controls are parsed and registered:
+ * - V4L2_CID_CAMERA_SENSOR_LOCATION;
+ * - V4L2_CID_CAMERA_SENSOR_ROTATION;
+ *
+ * Controls already registered by the caller with the @hdl control handler are
+ * not overwritten. Callers should register the controls they want to handle
+ * themselves before calling this function.
+ *
+ * Return: 0 on success, a negative error code on failure.
+ */
+int v4l2_ctrl_new_fwnode_properties(struct v4l2_ctrl_handler *hdl,
+				    const struct v4l2_ctrl_ops *ctrl_ops,
+				    const struct v4l2_fwnode_device_properties *p);
 #endif
-- 
2.23.0

[PATCH v4 10/11] media: i2c: ov5670: Parse and register properties

[Jacopo Mondi]

Parse device properties and register controls for them using the newly
introduced helpers.

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

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 041fcbb4eebd..f118d44b0889 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,8 @@ 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_fwnode_device_properties props;
 	struct v4l2_ctrl_handler *ctrl_hdlr;
 	s64 vblank_max;
 	s64 vblank_def;
@@ -2129,6 +2132,15 @@ static int ov5670_init_controls(struct ov5670 *ov5670)
 		goto error;
 	}
 
+	ret = v4l2_fwnode_device_parse(&client->dev, &props);
+	if (ret)
+		return ret;
+
+	ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &ov5670_ctrl_ops,
+					      &props);
+	if (ret)
+		return ret;
+
 	ov5670->sd.ctrl_handler = ctrl_hdlr;
 
 	return 0;
-- 
2.23.0

[PATCH v4 11/11] media: i2c: ov13858: Parse and register properties

[Jacopo Mondi]

Parse device properties and register controls for them using the newly
introduced helpers.

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

diff --git a/drivers/media/i2c/ov13858.c b/drivers/media/i2c/ov13858.c
index aac6f77afa0f..2ef5fb5cf519 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
@@ -1589,6 +1590,7 @@ static const struct v4l2_subdev_internal_ops ov13858_internal_ops = {
 static int ov13858_init_controls(struct ov13858 *ov13858)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&ov13858->sd);
+	struct v4l2_fwnode_device_properties props;
 	struct v4l2_ctrl_handler *ctrl_hdlr;
 	s64 exposure_max;
 	s64 vblank_def;
@@ -1666,6 +1668,15 @@ static int ov13858_init_controls(struct ov13858 *ov13858)
 		goto error;
 	}
 
+	ret = v4l2_fwnode_device_parse(&client->dev, &props);
+	if (ret)
+		return ret;
+
+	ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &ov13858_ctrl_ops,
+					      &props);
+	if (ret)
+		return ret;
+
 	ov13858->sd.ctrl_handler = ctrl_hdlr;
 
 	return 0;
-- 
2.23.0

Re: [PATCH v4 01/11] dt-bindings: video-interfaces: Document 'location' property

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1816 bytes --]

On Mon 2019-10-07 18:29:03, Jacopo Mondi wrote:
> Add the 'location' device property, used to specify a 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    | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index f884ada0bffc..1211bdf80722 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -89,6 +89,17 @@ Optional properties
>    but a number of degrees counter clockwise. Typical values are 0 and 180
>    (upside down).
>  
> +- location: The mount location of a device (typically an image sensor or a flash
> +  LED) expressed as a position relative to the usage orientation of the system
> +  where the device is installed on.
> +  Possible values are:
> +  0 - Front. The device is mounted on the front facing side of the system.
> +  For mobile devices such as smartphones, tablets and laptops the front side is
> +  the user facing side.
> +  1 - Back. The device is mounted on the back side of the system, which is
> +  defined as the opposite side of the front facing one.
> +  2 - External. The device is not attached directly to the system but is
> +  attached in a way that allows it to move freely.

I explained why this is not enough, and it did not change.

NAK.
									Pavel
									
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v4 01/11] dt-bindings: video-interfaces: Document 'location' property

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 4251 bytes --]

Pavel,

On Tue, Oct 08, 2019 at 09:40:52AM +0200, Pavel Machek wrote:
> On Mon 2019-10-07 18:29:03, Jacopo Mondi wrote:
> > Add the 'location' device property, used to specify a 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    | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > index f884ada0bffc..1211bdf80722 100644
> > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > @@ -89,6 +89,17 @@ Optional properties
> >    but a number of degrees counter clockwise. Typical values are 0 and 180
> >    (upside down).
> >
> > +- location: The mount location of a device (typically an image sensor or a flash
> > +  LED) expressed as a position relative to the usage orientation of the system
> > +  where the device is installed on.
> > +  Possible values are:
> > +  0 - Front. The device is mounted on the front facing side of the system.
> > +  For mobile devices such as smartphones, tablets and laptops the front side is
> > +  the user facing side.
> > +  1 - Back. The device is mounted on the back side of the system, which is
> > +  defined as the opposite side of the front facing one.
> > +  2 - External. The device is not attached directly to the system but is
> > +  attached in a way that allows it to move freely.
>
> I explained why this is not enough, and it did not change.

I replied to your email and you did not answered back.

I appreciate constructive inputs but just NAK, or throwing a proposal
without following up as you did, doesn't help much in improving the end
result.

I'll paste here my previous reply, so you get a chance to continue the
discussion. Please follow up if you're interested in contributing.

------------------------------------------------------------------------
> I don't think this is nearly enough of description. We have phones
> with displays and cameras at both sides, where both sides can be used
> to operate the system.
>
> We have phone with display spanning both sides -- Mi Max.
>
> https://www.idnes.cz/mobil/telefony/xiaomi-mi-mix-alpha-predstaveni.A190924_105858_telefony_oma
>
> We have Galaxy Fold.
>
> https://www.samsung.com/global/galaxy/galaxy-fold/
>
> What is front side when device can be used in different
> configurations?
>
> Could we instead say that it is "main" vs "selfie" camera?

I'm not sure the intended usage is something that belongs to DT. And
'selfie' implies you have a device side facing you, most like the
'front' one I have defined here.

Not to mention again this devices are all but supported by mainline,
which is just a partial justification as they might be an indication
of a trend.

There is no usable reference place, reference side, reference usage
mode that applies to -all- devices in the world, not one I can think
of.

I still think defining a location property is not blocking any new
extension that accommodate more advanced use cases. It's not like we're
adding a "front-camera" property, it's a "location" and you can expand
its accepted values with "front-when-device-folded" or whatever you
need for future devices.

In the description I mentioned the "usage orientation" to leave room
for possible device-specific details in the definition of the values
accepted by the property.

> > +  location expressed as a position relative to the usage orientation of the
> > +  system where the device is installed on.

99% of devices in the world have a front and a back, as well as they
have a top and a bottom. I still don't see why if a device does not
simply has a front it cannot use something different. The property
definition allows you to do so.

------------------------------------------------------------------------

>
> NAK.
> 									Pavel
>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 01/11] dt-bindings: video-interfaces: Document 'location' property

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 5155 bytes --]

On Tue 2019-10-08 09:58:28, Jacopo Mondi wrote:
> Pavel,
> 
> On Tue, Oct 08, 2019 at 09:40:52AM +0200, Pavel Machek wrote:
> > On Mon 2019-10-07 18:29:03, Jacopo Mondi wrote:
> > > Add the 'location' device property, used to specify a 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    | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > index f884ada0bffc..1211bdf80722 100644
> > > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > @@ -89,6 +89,17 @@ Optional properties
> > >    but a number of degrees counter clockwise. Typical values are 0 and 180
> > >    (upside down).
> > >
> > > +- location: The mount location of a device (typically an image sensor or a flash
> > > +  LED) expressed as a position relative to the usage orientation of the system
> > > +  where the device is installed on.
> > > +  Possible values are:
> > > +  0 - Front. The device is mounted on the front facing side of the system.
> > > +  For mobile devices such as smartphones, tablets and laptops the front side is
> > > +  the user facing side.
> > > +  1 - Back. The device is mounted on the back side of the system, which is
> > > +  defined as the opposite side of the front facing one.
> > > +  2 - External. The device is not attached directly to the system but is
> > > +  attached in a way that allows it to move freely.
> >
> > I explained why this is not enough, and it did not change.
> 
> I replied to your email and you did not answered back.
> 
> I appreciate constructive inputs but just NAK, or throwing a proposal
> without following up as you did, doesn't help much in improving the end
> result.
> 
> I'll paste here my previous reply, so you get a chance to continue the
> discussion. Please follow up if you're interested in contributing.

Yes, you are basically saying someone can solve those problems in
future :-(.

I'd add note that for camera-style devices, main sensor would be the
"back" one, and that for phones, selfie sensor should be marked as a
"front" one.

Plus, I believe you need to add a value for moving sensors, as there
are already devices that use same sensor for "front" and "back".

								Pavel


> ------------------------------------------------------------------------
> > I don't think this is nearly enough of description. We have phones
> > with displays and cameras at both sides, where both sides can be used
> > to operate the system.
> >
> > We have phone with display spanning both sides -- Mi Max.
> >
> > https://www.idnes.cz/mobil/telefony/xiaomi-mi-mix-alpha-predstaveni.A190924_105858_telefony_oma
> >
> > We have Galaxy Fold.
> >
> > https://www.samsung.com/global/galaxy/galaxy-fold/
> >
> > What is front side when device can be used in different
> > configurations?
> >
> > Could we instead say that it is "main" vs "selfie" camera?
> 
> I'm not sure the intended usage is something that belongs to DT. And
> 'selfie' implies you have a device side facing you, most like the
> 'front' one I have defined here.
> 
> Not to mention again this devices are all but supported by mainline,
> which is just a partial justification as they might be an indication
> of a trend.
> 
> There is no usable reference place, reference side, reference usage
> mode that applies to -all- devices in the world, not one I can think
> of.
> 
> I still think defining a location property is not blocking any new
> extension that accommodate more advanced use cases. It's not like we're
> adding a "front-camera" property, it's a "location" and you can expand
> its accepted values with "front-when-device-folded" or whatever you
> need for future devices.
> 
> In the description I mentioned the "usage orientation" to leave room
> for possible device-specific details in the definition of the values
> accepted by the property.
> 
> > > +  location expressed as a position relative to the usage orientation of the
> > > +  system where the device is installed on.
> 
> 99% of devices in the world have a front and a back, as well as they
> have a top and a bottom. I still don't see why if a device does not
> simply has a front it cannot use something different. The property
> definition allows you to do so.
> 
> ------------------------------------------------------------------------
> 
> >
> > NAK.
> > 									Pavel
> >
> > --
> > (english) http://www.livejournal.com/~pavelmachek
> > (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
> 
> 



-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v4 01/11] dt-bindings: video-interfaces: Document 'location' property

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 6782 bytes --]

On Tue, Oct 08, 2019 at 10:06:34AM +0200, Pavel Machek wrote:
> On Tue 2019-10-08 09:58:28, Jacopo Mondi wrote:
> > Pavel,
> >
> > On Tue, Oct 08, 2019 at 09:40:52AM +0200, Pavel Machek wrote:
> > > On Mon 2019-10-07 18:29:03, Jacopo Mondi wrote:
> > > > Add the 'location' device property, used to specify a 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    | 11 +++++++++++
> > > >  1 file changed, 11 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > > index f884ada0bffc..1211bdf80722 100644
> > > > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > > @@ -89,6 +89,17 @@ Optional properties
> > > >    but a number of degrees counter clockwise. Typical values are 0 and 180
> > > >    (upside down).
> > > >
> > > > +- location: The mount location of a device (typically an image sensor or a flash
> > > > +  LED) expressed as a position relative to the usage orientation of the system
> > > > +  where the device is installed on.
> > > > +  Possible values are:
> > > > +  0 - Front. The device is mounted on the front facing side of the system.
> > > > +  For mobile devices such as smartphones, tablets and laptops the front side is
> > > > +  the user facing side.
> > > > +  1 - Back. The device is mounted on the back side of the system, which is
> > > > +  defined as the opposite side of the front facing one.
> > > > +  2 - External. The device is not attached directly to the system but is
> > > > +  attached in a way that allows it to move freely.
> > >
> > > I explained why this is not enough, and it did not change.
> >
> > I replied to your email and you did not answered back.
> >
> > I appreciate constructive inputs but just NAK, or throwing a proposal
> > without following up as you did, doesn't help much in improving the end
> > result.
> >
> > I'll paste here my previous reply, so you get a chance to continue the
> > discussion. Please follow up if you're interested in contributing.
>
> Yes, you are basically saying someone can solve those problems in
> future :-(.

Not really, what I'm saying is that the property definition itself
leaves space for future extensions. As of now the accepted property
values cover the most trivial use case, but they can be expanded to
accommodate more complex setups, possibly with an extended parameters
list (in example something like "movable" with an associated rotation
matrix). In any case, I don't think we're tying anyone's hands by
adding this new property with the most basic locations that covers
99% of devices.

>
> I'd add note that for camera-style devices, main sensor would be the
> "back" one, and that for phones, selfie sensor should be marked as a
> "front" one.

I'm not sure if 'main' or 'selfie' as concepts belongs here at all. I
just want to report where the camera is installed, not the intended
usage.

The most common use case is to make simple for application to pick one
of the cameras based on the position. The front camera in a phone will
likely be for selfies, but in a laptop will mostly be for
'videoconference' or whatever. This is a definition that totally
belongs to userspace, and kernel should just report the mounting
location and that's it.

>
> Plus, I believe you need to add a value for moving sensors, as there
> are already devices that use same sensor for "front" and "back".

I'm skeptical about adding now a property for a device that we don't
support, because we -now- think it's a good idea. I might be wrong,
but my assumption is that when someone will want to support an
'advanced' device, it's easy to add "movable" or whatever else to the
list of accepted properties values. Am I wrong in assuming this? As
long as "front" "back" and "external" will stay supported for backward
DTB compatibility it should be fine, right ?

Thanks
   j

>
> 								Pavel
>
>
> > ------------------------------------------------------------------------
> > > I don't think this is nearly enough of description. We have phones
> > > with displays and cameras at both sides, where both sides can be used
> > > to operate the system.
> > >
> > > We have phone with display spanning both sides -- Mi Max.
> > >
> > > https://www.idnes.cz/mobil/telefony/xiaomi-mi-mix-alpha-predstaveni.A190924_105858_telefony_oma
> > >
> > > We have Galaxy Fold.
> > >
> > > https://www.samsung.com/global/galaxy/galaxy-fold/
> > >
> > > What is front side when device can be used in different
> > > configurations?
> > >
> > > Could we instead say that it is "main" vs "selfie" camera?
> >
> > I'm not sure the intended usage is something that belongs to DT. And
> > 'selfie' implies you have a device side facing you, most like the
> > 'front' one I have defined here.
> >
> > Not to mention again this devices are all but supported by mainline,
> > which is just a partial justification as they might be an indication
> > of a trend.
> >
> > There is no usable reference place, reference side, reference usage
> > mode that applies to -all- devices in the world, not one I can think
> > of.
> >
> > I still think defining a location property is not blocking any new
> > extension that accommodate more advanced use cases. It's not like we're
> > adding a "front-camera" property, it's a "location" and you can expand
> > its accepted values with "front-when-device-folded" or whatever you
> > need for future devices.
> >
> > In the description I mentioned the "usage orientation" to leave room
> > for possible device-specific details in the definition of the values
> > accepted by the property.
> >
> > > > +  location expressed as a position relative to the usage orientation of the
> > > > +  system where the device is installed on.
> >
> > 99% of devices in the world have a front and a back, as well as they
> > have a top and a bottom. I still don't see why if a device does not
> > simply has a front it cannot use something different. The property
> > definition allows you to do so.
> >
> > ------------------------------------------------------------------------
> >
> > >
> > > NAK.
> > > 									Pavel
> > >
> > > --
> > > (english) http://www.livejournal.com/~pavelmachek
> > > (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
> >
> >
>
>
>
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 00/11] media: Report camera sensor properties

[Sakari Ailus]

Hi Jacopo,

On Mon, Oct 07, 2019 at 06:29:02PM +0200, Jacopo Mondi wrote:
> Hello, fourth iteration following:
> 
> "media: v4l2-ctrls: Add camera sensor location"
> https://patchwork.kernel.org/project/linux-media/list/?series=160901
> "[v2,00/10] media: Report camera sensor properties
> https://patchwork.kernel.org/cover/11116443/
> "[PATCH v3 00/11] media: Report camera sensor properties"
> https://patchwork.kernel.org/project/linux-media/list/?series=173571
> 
> I here included Hans' comments. Most notable changes
> 
> v3 -> v4:
> - Minor reword in documentation of location and rotation properties
> - Fix V4L2_CID_CAMERA_SENSOR_ROTATION control documentation
> - Renamed helper in v4l2_ctrl_new_fwnode_properties()
> v2->v3:
> - Expand 'rotation' property description
> - s/device/system in properties description to make them applicable to
>   cameras and flash LEDs
> - Expand the rotation control description
> - Split helper to parse properties and helper to register properties
> - Drop the example coreboot patch that add properties to the Soraka device
>   ACPI tables
> 
> I know there are still doubts the two properties might well apply to
> modern devices with movable cameras, but I still think they cover 99% of devices
> out there at the moment.

Could you cc this on the next time (or bounce now) this to the devicetree
list, please?

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v4 06/11] media: v4l2-fwnode: Add helper to parse device properties

[Sakari Ailus]

Hi Jacopo,

On Mon, Oct 07, 2019 at 06:29:08PM +0200, Jacopo Mondi wrote:
> Add an helper function to parse common device properties in the same
> way as v4l2_fwnode_endpoint_parse() parses common endpoint properties.
> 
> Parse the 'rotation' and 'location' properties from the firmware
> interface.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/v4l2-core/v4l2-fwnode.c | 42 +++++++++++++++++++++++
>  include/media/v4l2-fwnode.h           | 48 +++++++++++++++++++++++++++
>  2 files changed, 90 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 192cac076761..8af4174a2bbf 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -596,6 +596,48 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
>  
> +int v4l2_fwnode_device_parse(struct device *dev,
> +			     struct v4l2_fwnode_device_properties *props)
> +{
> +	struct fwnode_handle *fwnode = dev_fwnode(dev);
> +	u32 val;
> +	int ret;
> +
> +	memset(props, 0, sizeof(*props));
> +
> +	props->location = V4L2_FWNODE_PROPERTY_UNSET;

How about adding V4L2_FWNODE_LOCATION_UNDEFINED instead? Same for the
rotation. I'd use 0 for that value, so memset above already sets it.

> +	ret = fwnode_property_read_u32(fwnode, "location", &val);
> +	if (!ret) {
> +		switch (val) {
> +		case V4L2_FWNODE_LOCATION_FRONT:
> +		case V4L2_FWNODE_LOCATION_BACK:
> +		case V4L2_FWNODE_LOCATION_EXTERNAL:
> +			break;
> +		default:
> +			dev_warn(dev, "Unsupported device location: %u\n", val);
> +			return -EINVAL;
> +		}
> +
> +		props->location = val;
> +		dev_dbg(dev, "device location: %u\n", val);
> +	}
> +
> +	props->rotation = V4L2_FWNODE_PROPERTY_UNSET;
> +	ret = fwnode_property_read_u32(fwnode, "rotation", &val);
> +	if (!ret) {
> +		if (val >= 360 || val % 90) {
> +			dev_warn(dev, "Unsupported device rotation: %u\n", val);

I think it should be up to the caller to determine what's supported.

> +			return -EINVAL;
> +		}
> +
> +		props->rotation = val;
> +		dev_dbg(dev, "device rotation: %u\n", val);
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_fwnode_device_parse);
> +
>  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..6d46d6fc3007 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -109,6 +109,36 @@ struct v4l2_fwnode_endpoint {
>  	unsigned int nr_of_link_frequencies;
>  };
>  
> +/**
> + * v4l2_fwnode_location - identify a non initialized property.
> + *
> + * All properties in &struct v4l2_fwnode_device_properties are initialized
> + * to this value.
> + */
> +#define V4L2_FWNODE_PROPERTY_UNSET   (-1U)
> +
> +/**
> + * enum v4l2_fwnode_location - possible device locations
> + * @V4L2_FWNODE_LOCATION_FRONT: device installed on the front side
> + * @V4L2_FWNODE_LOCATION_BACK: device installed on the back side
> + * @V4L2_FWNODE_LOCATION_EXTERNAL: device externally located
> + */
> +enum v4l2_fwnode_location {
> +	V4L2_FWNODE_LOCATION_FRONT,
> +	V4L2_FWNODE_LOCATION_BACK,
> +	V4L2_FWNODE_LOCATION_EXTERNAL
> +};
> +
> +/**
> + * struct v4l2_fwnode_device_properties - fwnode device properties
> + * @location: device location. See &enum v4l2_fwnode_location
> + * @rotation: device rotation
> + */
> +struct v4l2_fwnode_device_properties {
> +	enum v4l2_fwnode_location location;
> +	unsigned int rotation;
> +};
> +
>  /**
>   * struct v4l2_fwnode_link - a link between two endpoints
>   * @local_node: pointer to device_node of this endpoint
> @@ -233,6 +263,24 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
>   */
>  void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
>  
> +/**
> + * v4l2_fwnode_device_parse() - parse fwnode device properties
> + * @dev: pointer to &struct device
> + * @props: pointer to &struct v4l2_fwnode_device_properties where to store the
> + *	   parsed properties values
> + *
> + * This function parses and validates the V4L2 fwnode device properties from
> + * the firmware interface. It is responsibility of the caller to allocate a
> + * valid @struct v4l2_fwnode_device_properties structure and provide a pointer
> + * to it in the @props parameter.
> + *
> + * Return:
> + *	% 0 on success
> + *	%-EINVAL if a parsed property value is not valid
> + */
> +int v4l2_fwnode_device_parse(struct device *dev,
> +			     struct v4l2_fwnode_device_properties *props);
> +
>  /**
>   * typedef parse_endpoint_func - Driver's callback function to be called on
>   *	each V4L2 fwnode endpoint.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v4 01/11] dt-bindings: video-interfaces: Document 'location' property

[Sakari Ailus]

Hi Jacopo,

(Cc'ing the devicetree list.)

On Mon, Oct 07, 2019 at 06:29:03PM +0200, Jacopo Mondi wrote:
> Add the 'location' device property, used to specify a 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    | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index f884ada0bffc..1211bdf80722 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -89,6 +89,17 @@ Optional properties
>    but a number of degrees counter clockwise. Typical values are 0 and 180
>    (upside down).
>  
> +- location: The mount location of a device (typically an image sensor or a flash
> +  LED) expressed as a position relative to the usage orientation of the system
> +  where the device is installed on.
> +  Possible values are:
> +  0 - Front. The device is mounted on the front facing side of the system.

How about starting from 1? Then 0 can remain "undefined" value, i.e. the
caller can initialise the value to zero without the need to figure out
whether reading a given property was successful or not.

> +  For mobile devices such as smartphones, tablets and laptops the front side is
> +  the user facing side.
> +  1 - Back. The device is mounted on the back side of the system, which is
> +  defined as the opposite side of the front facing one.
> +  2 - External. The device is not attached directly to the system but is
> +  attached in a way that allows it to move freely.
>  
>  Optional endpoint properties
>  ----------------------------

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v4 04/11] media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_ROTATION

[Hans Verkuil]

On 10/7/19 6:29 PM, Jacopo Mondi wrote:
> Add documentation for the V4L2_CID_CAMERA_SENSOR_ROTATION camera
> control. The newly added read-only control reports the camera device
> mounting rotation.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  .../media/uapi/v4l/ext-ctrls-camera.rst       | 116 ++++++++++++++++++
>  1 file changed, 116 insertions(+)
> 
> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> index b151c016256c..e1fee4814e5b 100644
> --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> @@ -542,6 +542,122 @@ enum v4l2_scene_mode -
>  
>  
>  
> +``V4L2_CID_CAMERA_SENSOR_ROTATION (integer)``
> +    This read-only control describes the sensor orientation expressed as
> +    rotation in counterclockwise degrees along the axis perpendicular to the
> +    device mounting plane, and directed away from the sensor lens. Possible
> +    values for the control are 90, 180 and 270 degrees. To compensate for the
> +    device mounting rotation on the captured images, a rotation of the same
> +    amount of degrees, in the same counterclockwise rotation direction should be
> +    applied along the axis directed from the observer to the captured image when
> +    displayed on a screen.
> +
> +    To better understand the effect of the sensor rotation on the acquired
> +    images when displayed on a screen, it is helpful to consider a fictional
> +    scan-out sequence of the sensor's pixels, assuming the pixel array having
> +    its top-left pixel at position (0, 0) with values on the 'x' axis increasing
> +    towards the right direction, and values on the 'y' axis increasing towards
> +    the bottom. The effect of sensor rotation could be easily visualized
> +    considering the sequence of captured pixels.
> +
> +    Assuming the following scene has to be captured::
> +
> +                o
> +               -|-
> +               / \
> +
> +    An upright mounted sensor has its pixel array displaced as follow::
> +
> +                                   x
> +            (0,0)------------------->
> +              ! 0,0 0,1 0,2 ... (0,num-col)
> +              ! 1,0 1,1 1,2 ...
> +              ! ...
> +              ! ...
> +              ! (num-lines,0)...(num-col,num-lines)

Hmm, how about 'max-col' and 'max-row'?

num-col is wrong anyway since it would have to be num-cols - 1 because you
start at 0. It's easier to go with max-col/row.

> +            y V
> +
> +
> +    Assuming pixels are scanned out from (0,0) to (num-lines,num-col)
> +    progressively::
> +
> +             (0,0) ---->------------->   (0,num-col)
> +             (1,0) ---->------------->   (1,num-col)
> +             ( .... )-->------------->   (   ....   )
> +             (num-lines,0)----------->(num-lines,num-col)
> +
> +
> +    If a rotation of 90 degrees counterclockwise along the axis perpendicular to
> +    the sensor's lens and directed towards the scene to be captured is applied
> +    to the sensor, the pixel array would then be rotated as follows::
> +
> +            x ^ (0,num-col) ...   ...   (num-lines,num-col)
> +              !  ....
> +              !  0,2        1,2   ...          ...
> +              !  0,1        1,1   ...          ...
> +              !  0,0        1,0   ...      (num-lines,0)
> +             (0,0)------------------------------------>
> +                                                    y
> +
> +    And the pixel scan-out sequence would then proceed as follows starting
> +    from pixel (0,0)::
> +
> +           (0,num-col)         (num-lines,num-col)
> +                ^    ^   ^   ^     ^
> +                !    !   !   !     !
> +                !    !   !   !     !
> +                !    !   !   !     !
> +                !    !   !   !     !
> +              (0,0) (1,0)....  (num-lines,0)
> +
> +    Which when applied to the capture scene gives::
> +
> +           (0,num-col)         (num-lines,num-col)
> +                ^    ^   ^   ^     ^
> +                !    !   0   !     !
> +                !    !  -|- !     !
> +                !    !  /!\  !     !
> +                !    !   !   !     !
> +              (0,0) (1,0)....  (num-lines,0)
> +
> +    Producing the following image once captured to memory and
> +    displayed to the user::
> +
> +             \ !
> +               --0
> +             / !
> +
> +    Which has a rotation of the same amount of degrees applied on the opposite
> +    rotation direction along the axis that goes from the observer to the
> +    image when displayed on the screen.
> +
> +    In order to compensate the sensor mounting rotation, when expressed
> +    as counterclockwise rotation along the axis directed from the sensor to
> +    the captured scene, a rotation of the same amount of degrees in the
> +    same counterclockwise rotation direction but applied along the axis
> +    directed from the observer to the captured image, has to be applied::
> +
> +                +------+  90 degree counterclockwise
> +                |   o  |  mounting rotation applied
> +                |  -|- |  along the axis directed
> +                |  / \ |  away from the sensor lens
> +                +------+
> +                    |
> +                    V
> +                +------+
> +                | \ !  |  Resulting captured
> +                |  --0 |  image when displayed
> +                | / !  |  on screen
> +                +------+
> +                    |
> +                    V
> +                +------+
> +                |   o  |  Rotation compensation
> +                |  -|- |  is 90 degrees counterclockwise
> +                |  / \ |  along the axis directed to the
> +                +------+  displayed image
> +
> +
>  .. [#f1]
>     This control may be changed to a menu control in the future, if more
>     options are required.
> 

This gave me a headache, but you are correct w.r.t. image and mounting rotation.

Regards,

	Hans

Re: [PATCH v4 01/11] dt-bindings: video-interfaces: Document 'location' property

[Hans Verkuil]

On 10/8/19 10:20 AM, Jacopo Mondi wrote:
> On Tue, Oct 08, 2019 at 10:06:34AM +0200, Pavel Machek wrote:
>> On Tue 2019-10-08 09:58:28, Jacopo Mondi wrote:
>>> Pavel,
>>>
>>> On Tue, Oct 08, 2019 at 09:40:52AM +0200, Pavel Machek wrote:
>>>> On Mon 2019-10-07 18:29:03, Jacopo Mondi wrote:
>>>>> Add the 'location' device property, used to specify a 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    | 11 +++++++++++
>>>>>  1 file changed, 11 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
>>>>> index f884ada0bffc..1211bdf80722 100644
>>>>> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
>>>>> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
>>>>> @@ -89,6 +89,17 @@ Optional properties
>>>>>    but a number of degrees counter clockwise. Typical values are 0 and 180
>>>>>    (upside down).
>>>>>
>>>>> +- location: The mount location of a device (typically an image sensor or a flash
>>>>> +  LED) expressed as a position relative to the usage orientation of the system
>>>>> +  where the device is installed on.
>>>>> +  Possible values are:
>>>>> +  0 - Front. The device is mounted on the front facing side of the system.
>>>>> +  For mobile devices such as smartphones, tablets and laptops the front side is
>>>>> +  the user facing side.
>>>>> +  1 - Back. The device is mounted on the back side of the system, which is
>>>>> +  defined as the opposite side of the front facing one.
>>>>> +  2 - External. The device is not attached directly to the system but is
>>>>> +  attached in a way that allows it to move freely.
>>>>
>>>> I explained why this is not enough, and it did not change.
>>>
>>> I replied to your email and you did not answered back.
>>>
>>> I appreciate constructive inputs but just NAK, or throwing a proposal
>>> without following up as you did, doesn't help much in improving the end
>>> result.
>>>
>>> I'll paste here my previous reply, so you get a chance to continue the
>>> discussion. Please follow up if you're interested in contributing.
>>
>> Yes, you are basically saying someone can solve those problems in
>> future :-(.
> 
> Not really, what I'm saying is that the property definition itself
> leaves space for future extensions. As of now the accepted property
> values cover the most trivial use case, but they can be expanded to
> accommodate more complex setups, possibly with an extended parameters
> list (in example something like "movable" with an associated rotation
> matrix). In any case, I don't think we're tying anyone's hands by
> adding this new property with the most basic locations that covers
> 99% of devices.
> 
>>
>> I'd add note that for camera-style devices, main sensor would be the
>> "back" one, and that for phones, selfie sensor should be marked as a
>> "front" one.
> 
> I'm not sure if 'main' or 'selfie' as concepts belongs here at all. I
> just want to report where the camera is installed, not the intended
> usage.
> 
> The most common use case is to make simple for application to pick one
> of the cameras based on the position. The front camera in a phone will
> likely be for selfies, but in a laptop will mostly be for
> 'videoconference' or whatever. This is a definition that totally
> belongs to userspace, and kernel should just report the mounting
> location and that's it.
> 
>>
>> Plus, I believe you need to add a value for moving sensors, as there
>> are already devices that use same sensor for "front" and "back".
> 
> I'm skeptical about adding now a property for a device that we don't
> support, because we -now- think it's a good idea. I might be wrong,
> but my assumption is that when someone will want to support an
> 'advanced' device, it's easy to add "movable" or whatever else to the
> list of accepted properties values. Am I wrong in assuming this? As
> long as "front" "back" and "external" will stay supported for backward
> DTB compatibility it should be fine, right ?

The basic rule is that you should not define things unless you KNOW that
they will be needed. So when we actually see new devices for which
"front", "back" or "external" does not fit, then new names can be created.

It's impossible to cover all situations since we can't predict the future.
The best we can do is to allow for future extensions.

I think this looks good.

Regards,

	Hans

> 
> Thanks
>    j
> 
>>
>> 								Pavel
>>
>>
>>> ------------------------------------------------------------------------
>>>> I don't think this is nearly enough of description. We have phones
>>>> with displays and cameras at both sides, where both sides can be used
>>>> to operate the system.
>>>>
>>>> We have phone with display spanning both sides -- Mi Max.
>>>>
>>>> https://www.idnes.cz/mobil/telefony/xiaomi-mi-mix-alpha-predstaveni.A190924_105858_telefony_oma
>>>>
>>>> We have Galaxy Fold.
>>>>
>>>> https://www.samsung.com/global/galaxy/galaxy-fold/
>>>>
>>>> What is front side when device can be used in different
>>>> configurations?
>>>>
>>>> Could we instead say that it is "main" vs "selfie" camera?
>>>
>>> I'm not sure the intended usage is something that belongs to DT. And
>>> 'selfie' implies you have a device side facing you, most like the
>>> 'front' one I have defined here.
>>>
>>> Not to mention again this devices are all but supported by mainline,
>>> which is just a partial justification as they might be an indication
>>> of a trend.
>>>
>>> There is no usable reference place, reference side, reference usage
>>> mode that applies to -all- devices in the world, not one I can think
>>> of.
>>>
>>> I still think defining a location property is not blocking any new
>>> extension that accommodate more advanced use cases. It's not like we're
>>> adding a "front-camera" property, it's a "location" and you can expand
>>> its accepted values with "front-when-device-folded" or whatever you
>>> need for future devices.
>>>
>>> In the description I mentioned the "usage orientation" to leave room
>>> for possible device-specific details in the definition of the values
>>> accepted by the property.
>>>
>>>>> +  location expressed as a position relative to the usage orientation of the
>>>>> +  system where the device is installed on.
>>>
>>> 99% of devices in the world have a front and a back, as well as they
>>> have a top and a bottom. I still don't see why if a device does not
>>> simply has a front it cannot use something different. The property
>>> definition allows you to do so.
>>>
>>> ------------------------------------------------------------------------
>>>
>>>>
>>>> NAK.
>>>> 									Pavel
>>>>
>>>> --
>>>> (english) http://www.livejournal.com/~pavelmachek
>>>> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
>>>
>>>
>>
>>
>>
>> --
>> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> 
> 

Re: [PATCH v4 01/11] dt-bindings: video-interfaces: Document 'location' property

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1312 bytes --]

Hi!

> > I'm skeptical about adding now a property for a device that we don't
> > support, because we -now- think it's a good idea. I might be wrong,
> > but my assumption is that when someone will want to support an
> > 'advanced' device, it's easy to add "movable" or whatever else to the
> > list of accepted properties values. Am I wrong in assuming this? As
> > long as "front" "back" and "external" will stay supported for backward
> > DTB compatibility it should be fine, right ?
> 
> The basic rule is that you should not define things unless you KNOW that
> they will be needed. So when we actually see new devices for which
> "front", "back" or "external" does not fit, then new names can be
> created.

> It's impossible to cover all situations since we can't predict the future.
> The best we can do is to allow for future extensions.

Those devices are already being sold, and yes, they are running linux
(with some patches probably).

I believe it would be better to specify "this camera is selfie --
takes pictures of the user" vs. "this is main camera -- takes pictures
of what user is looking at".

Best regards,

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v4 01/11] dt-bindings: video-interfaces: Document 'location' property

[Tomasz Figa]

On Wed, Oct 23, 2019 at 11:27 PM Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
> > > I'm skeptical about adding now a property for a device that we don't
> > > support, because we -now- think it's a good idea. I might be wrong,
> > > but my assumption is that when someone will want to support an
> > > 'advanced' device, it's easy to add "movable" or whatever else to the
> > > list of accepted properties values. Am I wrong in assuming this? As
> > > long as "front" "back" and "external" will stay supported for backward
> > > DTB compatibility it should be fine, right ?
> >
> > The basic rule is that you should not define things unless you KNOW that
> > they will be needed. So when we actually see new devices for which
> > "front", "back" or "external" does not fit, then new names can be
> > created.
>
> > It's impossible to cover all situations since we can't predict the future.
> > The best we can do is to allow for future extensions.
>
> Those devices are already being sold, and yes, they are running linux
> (with some patches probably).
>
> I believe it would be better to specify "this camera is selfie --
> takes pictures of the user" vs. "this is main camera -- takes pictures
> of what user is looking at".

FWIW, Android and Chrome OS call those "user-facing" and
"world-facing" respectively.

Best regards,
Tomasz

Re: [PATCH v4 01/11] dt-bindings: video-interfaces: Document 'location' property

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 1679 bytes --]

Hi Pavel,

On Wed, Oct 23, 2019 at 04:27:17PM +0200, Pavel Machek wrote:
> Hi!
>
> > > I'm skeptical about adding now a property for a device that we don't
> > > support, because we -now- think it's a good idea. I might be wrong,
> > > but my assumption is that when someone will want to support an
> > > 'advanced' device, it's easy to add "movable" or whatever else to the
> > > list of accepted properties values. Am I wrong in assuming this? As
> > > long as "front" "back" and "external" will stay supported for backward
> > > DTB compatibility it should be fine, right ?
> >
> > The basic rule is that you should not define things unless you KNOW that
> > they will be needed. So when we actually see new devices for which
> > "front", "back" or "external" does not fit, then new names can be
> > created.
>
> > It's impossible to cover all situations since we can't predict the future.
> > The best we can do is to allow for future extensions.
>
> Those devices are already being sold, and yes, they are running linux
> (with some patches probably).
>
> I believe it would be better to specify "this camera is selfie --
> takes pictures of the user" vs. "this is main camera -- takes pictures
> of what user is looking at".

The intended usage of a component is something that really does not
belong to DT. The install location is a physical property of the
device, what the user is supposed to do with it is not. I would be
very uncomfortable to have anything like "selfie" in DT bindings.


>
> Best regards,
>
> 								Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 01/11] dt-bindings: video-interfaces: Document 'location' property

[Laurent Pinchart]

On Fri, Oct 25, 2019 at 03:29:49PM +0900, Tomasz Figa wrote:
> On Wed, Oct 23, 2019 at 11:27 PM Pavel Machek <pavel@ucw.cz> wrote:
> >
> > Hi!
> >
> > > > I'm skeptical about adding now a property for a device that we don't
> > > > support, because we -now- think it's a good idea. I might be wrong,
> > > > but my assumption is that when someone will want to support an
> > > > 'advanced' device, it's easy to add "movable" or whatever else to the
> > > > list of accepted properties values. Am I wrong in assuming this? As
> > > > long as "front" "back" and "external" will stay supported for backward
> > > > DTB compatibility it should be fine, right ?
> > >
> > > The basic rule is that you should not define things unless you KNOW that
> > > they will be needed. So when we actually see new devices for which
> > > "front", "back" or "external" does not fit, then new names can be
> > > created.
> >
> > > It's impossible to cover all situations since we can't predict the future.
> > > The best we can do is to allow for future extensions.
> >
> > Those devices are already being sold, and yes, they are running linux
> > (with some patches probably).
> >
> > I believe it would be better to specify "this camera is selfie --
> > takes pictures of the user" vs. "this is main camera -- takes pictures
> > of what user is looking at".
> 
> FWIW, Android and Chrome OS call those "user-facing" and
> "world-facing" respectively.

Isn't that equivalent to what Jacopo is proposing though ? If we define
the orientation of the device relatively to its user (e.g. for all
cellphone devices the front is defined as the side facing the user), and
the location of the camera relative to the device, we get the same
information.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 06/11] media: v4l2-fwnode: Add helper to parse device properties

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 5686 bytes --]

Hi Sakari,

On Fri, Oct 18, 2019 at 10:36:40PM +0300, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Mon, Oct 07, 2019 at 06:29:08PM +0200, Jacopo Mondi wrote:
> > Add an helper function to parse common device properties in the same
> > way as v4l2_fwnode_endpoint_parse() parses common endpoint properties.
> >
> > Parse the 'rotation' and 'location' properties from the firmware
> > interface.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  drivers/media/v4l2-core/v4l2-fwnode.c | 42 +++++++++++++++++++++++
> >  include/media/v4l2-fwnode.h           | 48 +++++++++++++++++++++++++++
> >  2 files changed, 90 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > index 192cac076761..8af4174a2bbf 100644
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -596,6 +596,48 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
> >  }
> >  EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
> >
> > +int v4l2_fwnode_device_parse(struct device *dev,
> > +			     struct v4l2_fwnode_device_properties *props)
> > +{
> > +	struct fwnode_handle *fwnode = dev_fwnode(dev);
> > +	u32 val;
> > +	int ret;
> > +
> > +	memset(props, 0, sizeof(*props));
> > +
> > +	props->location = V4L2_FWNODE_PROPERTY_UNSET;
>
> How about adding V4L2_FWNODE_LOCATION_UNDEFINED instead? Same for the
> rotation. I'd use 0 for that value, so memset above already sets it.
>

I'm not sure... 0 is actually a valid rotation value, and to use 0
here the location properties available to users
(V4L2_FWNODE_LOCATION_FRONT, V4L2_FWNODE_LOCATION_BACK etc) should be
numbered starting from 1... I would keep PROPERTY_UNSET (or
_UNDEFINED) as (-1U) to be honest...

> > +	ret = fwnode_property_read_u32(fwnode, "location", &val);
> > +	if (!ret) {
> > +		switch (val) {
> > +		case V4L2_FWNODE_LOCATION_FRONT:
> > +		case V4L2_FWNODE_LOCATION_BACK:
> > +		case V4L2_FWNODE_LOCATION_EXTERNAL:
> > +			break;
> > +		default:
> > +			dev_warn(dev, "Unsupported device location: %u\n", val);
> > +			return -EINVAL;
> > +		}
> > +
> > +		props->location = val;
> > +		dev_dbg(dev, "device location: %u\n", val);
> > +	}
> > +
> > +	props->rotation = V4L2_FWNODE_PROPERTY_UNSET;
> > +	ret = fwnode_property_read_u32(fwnode, "rotation", &val);
> > +	if (!ret) {
> > +		if (val >= 360 || val % 90) {
> > +			dev_warn(dev, "Unsupported device rotation: %u\n", val);
>
> I think it should be up to the caller to determine what's supported.
>

well, I tend to agree.. Hans and Laurent suggested to have fixed
values, but I would actually just check for (val >= 360) here and let
the caller figure out if it supports the specified location or not...

Thanks for feedback, I'll send a v5 shortly

> > +			return -EINVAL;
> > +		}
> > +
> > +		props->rotation = val;
> > +		dev_dbg(dev, "device rotation: %u\n", val);
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_fwnode_device_parse);
> > +
> >  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..6d46d6fc3007 100644
> > --- a/include/media/v4l2-fwnode.h
> > +++ b/include/media/v4l2-fwnode.h
> > @@ -109,6 +109,36 @@ struct v4l2_fwnode_endpoint {
> >  	unsigned int nr_of_link_frequencies;
> >  };
> >
> > +/**
> > + * v4l2_fwnode_location - identify a non initialized property.
> > + *
> > + * All properties in &struct v4l2_fwnode_device_properties are initialized
> > + * to this value.
> > + */
> > +#define V4L2_FWNODE_PROPERTY_UNSET   (-1U)
> > +
> > +/**
> > + * enum v4l2_fwnode_location - possible device locations
> > + * @V4L2_FWNODE_LOCATION_FRONT: device installed on the front side
> > + * @V4L2_FWNODE_LOCATION_BACK: device installed on the back side
> > + * @V4L2_FWNODE_LOCATION_EXTERNAL: device externally located
> > + */
> > +enum v4l2_fwnode_location {
> > +	V4L2_FWNODE_LOCATION_FRONT,
> > +	V4L2_FWNODE_LOCATION_BACK,
> > +	V4L2_FWNODE_LOCATION_EXTERNAL
> > +};
> > +
> > +/**
> > + * struct v4l2_fwnode_device_properties - fwnode device properties
> > + * @location: device location. See &enum v4l2_fwnode_location
> > + * @rotation: device rotation
> > + */
> > +struct v4l2_fwnode_device_properties {
> > +	enum v4l2_fwnode_location location;
> > +	unsigned int rotation;
> > +};
> > +
> >  /**
> >   * struct v4l2_fwnode_link - a link between two endpoints
> >   * @local_node: pointer to device_node of this endpoint
> > @@ -233,6 +263,24 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
> >   */
> >  void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
> >
> > +/**
> > + * v4l2_fwnode_device_parse() - parse fwnode device properties
> > + * @dev: pointer to &struct device
> > + * @props: pointer to &struct v4l2_fwnode_device_properties where to store the
> > + *	   parsed properties values
> > + *
> > + * This function parses and validates the V4L2 fwnode device properties from
> > + * the firmware interface. It is responsibility of the caller to allocate a
> > + * valid @struct v4l2_fwnode_device_properties structure and provide a pointer
> > + * to it in the @props parameter.
> > + *
> > + * Return:
> > + *	% 0 on success
> > + *	%-EINVAL if a parsed property value is not valid
> > + */
> > +int v4l2_fwnode_device_parse(struct device *dev,
> > +			     struct v4l2_fwnode_device_properties *props);
> > +
> >  /**
> >   * typedef parse_endpoint_func - Driver's callback function to be called on
> >   *	each V4L2 fwnode endpoint.
>
> --
> Kind regards,
>
> Sakari Ailus
> sakari.ailus@linux.intel.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 01/11] dt-bindings: video-interfaces: Document 'location' property

[Tomasz Figa]

On Sun, Oct 27, 2019 at 5:48 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Fri, Oct 25, 2019 at 03:29:49PM +0900, Tomasz Figa wrote:
> > On Wed, Oct 23, 2019 at 11:27 PM Pavel Machek <pavel@ucw.cz> wrote:
> > >
> > > Hi!
> > >
> > > > > I'm skeptical about adding now a property for a device that we don't
> > > > > support, because we -now- think it's a good idea. I might be wrong,
> > > > > but my assumption is that when someone will want to support an
> > > > > 'advanced' device, it's easy to add "movable" or whatever else to the
> > > > > list of accepted properties values. Am I wrong in assuming this? As
> > > > > long as "front" "back" and "external" will stay supported for backward
> > > > > DTB compatibility it should be fine, right ?
> > > >
> > > > The basic rule is that you should not define things unless you KNOW that
> > > > they will be needed. So when we actually see new devices for which
> > > > "front", "back" or "external" does not fit, then new names can be
> > > > created.
> > >
> > > > It's impossible to cover all situations since we can't predict the future.
> > > > The best we can do is to allow for future extensions.
> > >
> > > Those devices are already being sold, and yes, they are running linux
> > > (with some patches probably).
> > >
> > > I believe it would be better to specify "this camera is selfie --
> > > takes pictures of the user" vs. "this is main camera -- takes pictures
> > > of what user is looking at".
> >
> > FWIW, Android and Chrome OS call those "user-facing" and
> > "world-facing" respectively.
>
> Isn't that equivalent to what Jacopo is proposing though ? If we define
> the orientation of the device relatively to its user (e.g. for all
> cellphone devices the front is defined as the side facing the user), and
> the location of the camera relative to the device, we get the same
> information.

Yes, it is the same. Although having some consistency in the naming
isn't necessarily a bad idea, is it? :)

That said, looks like the naming in the Android world isn't consistent
either. The high level Java/Kotlin API uses "front" and "back":
https://developer.android.com/reference/android/hardware/Camera.CameraInfo.html#summary

How about making this property a string instead? It would make it more
readable in the dts and more expressive for some weird cases in the
future, e.g. "front+30deg", "vector" (and a vector could be given in
another property), etc.

Best regards,
Tomasz

Re: [PATCH v4 01/11] dt-bindings: video-interfaces: Document 'location' property

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 3005 bytes --]

Hi Tomasz,

On Mon, Nov 18, 2019 at 08:32:35PM +0900, Tomasz Figa wrote:
> On Sun, Oct 27, 2019 at 5:48 AM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > On Fri, Oct 25, 2019 at 03:29:49PM +0900, Tomasz Figa wrote:
> > > On Wed, Oct 23, 2019 at 11:27 PM Pavel Machek <pavel@ucw.cz> wrote:
> > > >
> > > > Hi!
> > > >
> > > > > > I'm skeptical about adding now a property for a device that we don't
> > > > > > support, because we -now- think it's a good idea. I might be wrong,
> > > > > > but my assumption is that when someone will want to support an
> > > > > > 'advanced' device, it's easy to add "movable" or whatever else to the
> > > > > > list of accepted properties values. Am I wrong in assuming this? As
> > > > > > long as "front" "back" and "external" will stay supported for backward
> > > > > > DTB compatibility it should be fine, right ?
> > > > >
> > > > > The basic rule is that you should not define things unless you KNOW that
> > > > > they will be needed. So when we actually see new devices for which
> > > > > "front", "back" or "external" does not fit, then new names can be
> > > > > created.
> > > >
> > > > > It's impossible to cover all situations since we can't predict the future.
> > > > > The best we can do is to allow for future extensions.
> > > >
> > > > Those devices are already being sold, and yes, they are running linux
> > > > (with some patches probably).
> > > >
> > > > I believe it would be better to specify "this camera is selfie --
> > > > takes pictures of the user" vs. "this is main camera -- takes pictures
> > > > of what user is looking at".
> > >
> > > FWIW, Android and Chrome OS call those "user-facing" and
> > > "world-facing" respectively.
> >
> > Isn't that equivalent to what Jacopo is proposing though ? If we define
> > the orientation of the device relatively to its user (e.g. for all
> > cellphone devices the front is defined as the side facing the user), and
> > the location of the camera relative to the device, we get the same
> > information.
>
> Yes, it is the same. Although having some consistency in the naming
> isn't necessarily a bad idea, is it? :)
>
> That said, looks like the naming in the Android world isn't consistent
> either. The high level Java/Kotlin API uses "front" and "back":
> https://developer.android.com/reference/android/hardware/Camera.CameraInfo.html#summary
>
> How about making this property a string instead? It would make it more
> readable in the dts and more expressive for some weird cases in the
> future, e.g. "front+30deg", "vector" (and a vector could be given in
> another property), etc.

Why do you think this would be better ? We could provide macros if we
want to have users expressing location in a more 'expressive' form
than using a numerical id. I would avoid parsing strings to be honest.

Would you be ok with this ?

I don't see any core v4l2 header in include/dt-bindings/media/ but
that could be a good occasion to add one ?
>
> Best regards,
> Tomasz

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 01/11] dt-bindings: video-interfaces: Document 'location' property

[Tomasz Figa]

On Fri, Nov 22, 2019 at 1:00 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Tomasz,
>
> On Mon, Nov 18, 2019 at 08:32:35PM +0900, Tomasz Figa wrote:
> > On Sun, Oct 27, 2019 at 5:48 AM Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> > >
> > > On Fri, Oct 25, 2019 at 03:29:49PM +0900, Tomasz Figa wrote:
> > > > On Wed, Oct 23, 2019 at 11:27 PM Pavel Machek <pavel@ucw.cz> wrote:
> > > > >
> > > > > Hi!
> > > > >
> > > > > > > I'm skeptical about adding now a property for a device that we don't
> > > > > > > support, because we -now- think it's a good idea. I might be wrong,
> > > > > > > but my assumption is that when someone will want to support an
> > > > > > > 'advanced' device, it's easy to add "movable" or whatever else to the
> > > > > > > list of accepted properties values. Am I wrong in assuming this? As
> > > > > > > long as "front" "back" and "external" will stay supported for backward
> > > > > > > DTB compatibility it should be fine, right ?
> > > > > >
> > > > > > The basic rule is that you should not define things unless you KNOW that
> > > > > > they will be needed. So when we actually see new devices for which
> > > > > > "front", "back" or "external" does not fit, then new names can be
> > > > > > created.
> > > > >
> > > > > > It's impossible to cover all situations since we can't predict the future.
> > > > > > The best we can do is to allow for future extensions.
> > > > >
> > > > > Those devices are already being sold, and yes, they are running linux
> > > > > (with some patches probably).
> > > > >
> > > > > I believe it would be better to specify "this camera is selfie --
> > > > > takes pictures of the user" vs. "this is main camera -- takes pictures
> > > > > of what user is looking at".
> > > >
> > > > FWIW, Android and Chrome OS call those "user-facing" and
> > > > "world-facing" respectively.
> > >
> > > Isn't that equivalent to what Jacopo is proposing though ? If we define
> > > the orientation of the device relatively to its user (e.g. for all
> > > cellphone devices the front is defined as the side facing the user), and
> > > the location of the camera relative to the device, we get the same
> > > information.
> >
> > Yes, it is the same. Although having some consistency in the naming
> > isn't necessarily a bad idea, is it? :)
> >
> > That said, looks like the naming in the Android world isn't consistent
> > either. The high level Java/Kotlin API uses "front" and "back":
> > https://developer.android.com/reference/android/hardware/Camera.CameraInfo.html#summary
> >
> > How about making this property a string instead? It would make it more
> > readable in the dts and more expressive for some weird cases in the
> > future, e.g. "front+30deg", "vector" (and a vector could be given in
> > another property), etc.
>
> Why do you think this would be better ? We could provide macros if we
> want to have users expressing location in a more 'expressive' form
> than using a numerical id. I would avoid parsing strings to be honest.
>
> Would you be ok with this ?
>
> I don't see any core v4l2 header in include/dt-bindings/media/ but
> that could be a good occasion to add one ?

As an afterthought, there is no flexibility issue even with an enum.
We could still have the "location" property set to the enum
corresponding to front and then if we need to rotate it relative to
that position we could define another property called "rotation".

Feel free to add my Acked-by.

Best regards,
Tomasz