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

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

[Jacopo Mondi]

Hello, fifth 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
"[PATCH v4 00/11] media: Report camera sensor properties
https://patchwork.kernel.org/project/linux-media/list/?series=183859

Minimal changes indeed as I've taken in Hans' suggestion to use max-col,
max-rows in place of num-cols, nul-lines in [04/11].

I didn't address Sakari suggestion to use 0 a 'UNDEFINED' property, mostly
because 0 is a valid rotation, and for location, the user visible property
values should have been enumerated starting from 1, which seem very un-natural.
I can change if you feel strong about it though.

v4 -> v5:
- cc device tree mailing list on relevant patches
- s/num-cols/max-col and s/num-lines/max-row in 4/11

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

Patches for the coreboot provided ACPI tables for two example sensors:
https://jmondi.org/cgit/coreboot/commit/?id=53a5fc6450bd45992f14a41848b72350f257c151
https://jmondi.org/cgit/coreboot/commit/?id=2a6b9b51f0e2e7b4ca5f4eadf21df8468ebc1b3f

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 v5 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 v5 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 v5 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 v5 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..3528d6a3e1c7 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,max-col)
+              ! 1,0 1,1 1,2 ...
+              ! ...
+              ! ...
+              ! (max-row,0) ... (max-col,max-row)
+            y V
+
+
+    Assuming pixels are scanned out from (0,0) to (max-row,max-col)
+    progressively::
+
+             (0,0) ---->------------->   (0,max-col)
+             (1,0) ---->------------->   (1,max-col)
+             ( .... )-->------------->   (   ....   )
+             (max-row,0)----------->   (max-row,max-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,max-col) ...   ...   (max-row,max-col)
+              !  ....
+              !  0,2        1,2   ...          ...
+              !  0,1        1,1   ...          ...
+              !  0,0        1,0   ...      (max-row,0)
+             (0,0)------------------------------------>
+                                                    y
+
+    And the pixel scan-out sequence would then proceed as follows starting
+    from pixel (0,0)::
+
+           (0,max-col)         (max-row,max-col)
+                ^    ^   ^   ^     ^
+                !    !   !   !     !
+                !    !   !   !     !
+                !    !   !   !     !
+                !    !   !   !     !
+              (0,0) (1,0)....  (max-row,0)
+
+    Which when applied to the capture scene gives::
+
+           (0,max-col)         (max-row,max-col)
+                ^    ^   ^   ^     ^
+                !    !   0   !     !
+                !    !  -|-  !     !
+                !    !  /!\  !     !
+                !    !   !   !     !
+              (0,0) (1,0)....  (max-row,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 v5 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 a565ae3ba7e4..0293135833d8 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1015,6 +1015,8 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_PAN_SPEED:		return "Pan, Speed";
 	case V4L2_CID_TILT_SPEED:		return "Tilt, Speed";
 	case V4L2_CID_UNIT_CELL_SIZE:		return "Unit Cell Size";
+	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! */
@@ -1341,6 +1343,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 5a7bedee2b0e..2517d2f63d2f 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 v5 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 v5 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 7db9e719a583..cf59abafb0d9 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -25,14 +25,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 v5 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 0293135833d8..97e97c8069c9 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 v5 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 97e97c8069c9..ac1934558969 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 {					\
@@ -4587,3 +4588,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 cf59abafb0d9..409c800ab1f5 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -30,6 +30,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;
@@ -1417,4 +1418,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 v5 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 v5 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 v5 01/11] dt-bindings: video-interfaces: Document 'location' property

[Rob Herring]

On Fri,  8 Nov 2019 16:59:34 +0100, 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(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

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

[Rob Herring]

On Fri,  8 Nov 2019 16:59:36 +0100, Jacopo Mondi wrote:
> 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(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

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

[Sakari Ailus]

Hi Jacopo,

On Fri, Nov 08, 2019 at 04:59:39PM +0100, 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;
> +	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;

In both cases, you return an error back to the user if there's a problem
with the value of an optinoal property.

It could prevent probing the driver if someone screwed up the firmware
somehow, but also makes the matter more visible. I think it's good to start
with this way, and hope we don't need to change it.

> +		}
> +
> +		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)

On the naming --- this is relevant for only one property at the moment, so
I'd make it specific to that property, too. This also makes it fit for the
enum below.

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

I think the latter sentence (inserted before the leading comma of the
previous sentence) could be phrased as:

"and fills the @struct v4l2_fwnode_device_properties provided by the
caller".

> + *
> + * 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.

-- 
Regards,

Sakari Ailus

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

[Sakari Ailus]

Hi Jacopo,

On Fri, Nov 08, 2019 at 04:59:42PM +0100, Jacopo Mondi wrote:
> 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 97e97c8069c9..ac1934558969 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 {					\
> @@ -4587,3 +4588,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;

I think you should return hdl->error also here: calling this function on a
failed control handler should result in an error.

> +}
> +EXPORT_SYMBOL(v4l2_ctrl_new_fwnode_properties);
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index cf59abafb0d9..409c800ab1f5 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -30,6 +30,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;
> @@ -1417,4 +1418,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

-- 
Regards,

Sakari Ailus

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

[Jacopo Mondi]

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

Hi Sakari,

On Mon, Nov 18, 2019 at 02:52:40PM +0200, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Fri, Nov 08, 2019 at 04:59:39PM +0100, 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;
> > +	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;
>
> In both cases, you return an error back to the user if there's a problem
> with the value of an optinoal property.
>
> It could prevent probing the driver if someone screwed up the firmware
> somehow, but also makes the matter more visible. I think it's good to start
> with this way, and hope we don't need to change it.
>
> > +		}
> > +
> > +		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)
>
> On the naming --- this is relevant for only one property at the moment, so
> I'd make it specific to that property, too. This also makes it fit for the
> enum below.

It is used also for rotation, not only for location. I need an
"UNSET" value as a 0 degree rotation is valid. To be hones I will keep
it generic, I do expect more properties to need it, as it will have to
be renamed later...

>
> > +
> > +/**
> > + * 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.
>
> I think the latter sentence (inserted before the leading comma of the
> previous sentence) could be phrased as:
>
> "and fills the @struct v4l2_fwnode_device_properties provided by the
> caller".

Yeah, thanks, better! I'll take it in!

Thanks
   j
>
> > + *
> > + * 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.
>
> --
> Regards,
>
> Sakari Ailus

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

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

[Jacopo Mondi]

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

Hi Sakari,

On Mon, Nov 18, 2019 at 02:54:35PM +0200, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Fri, Nov 08, 2019 at 04:59:42PM +0100, Jacopo Mondi wrote:
> > 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 97e97c8069c9..ac1934558969 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 {					\
> > @@ -4587,3 +4588,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;
>
> I think you should return hdl->error also here: calling this function on a
> failed control handler should result in an error.
>

I assume it would have failed at v4l2_ctrl_new_std() time, but indeed
it does not hurt to return hdl->error here. Thanks, I'll take this in.

Thanks
  j

> > +}
> > +EXPORT_SYMBOL(v4l2_ctrl_new_fwnode_properties);
> > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> > index cf59abafb0d9..409c800ab1f5 100644
> > --- a/include/media/v4l2-ctrls.h
> > +++ b/include/media/v4l2-ctrls.h
> > @@ -30,6 +30,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;
> > @@ -1417,4 +1418,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
>
> --
> Regards,
>
> Sakari Ailus

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