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

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

[Jacopo Mondi]

Hello, third 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/

Compared to v2 I have dropped the two patches reporting the sensor pixel array
size and active areas size

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'
  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       | 149 ++++++++++++++++++
 drivers/media/i2c/ov13858.c                   |  11 ++
 drivers/media/i2c/ov5670.c                    |  12 ++
 drivers/media/v4l2-core/v4l2-ctrls.c          |  54 ++++++-
 drivers/media/v4l2-core/v4l2-fwnode.c         |  44 ++++++
 include/media/v4l2-ctrls.h                    |  36 ++++-
 include/media/v4l2-fwnode.h                   |  42 +++++
 include/uapi/linux/v4l2-controls.h            |   7 +
 9 files changed, 364 insertions(+), 12 deletions(-)

--
2.23.0

[PATCH v3 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..e71b90a29d7a 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 device, typically an image sensor or a flash LED, mounting
+  location 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, or is
+  attached in a way that allows it to move freely.

 Optional endpoint properties
 ----------------------------
--
2.23.0

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

[Hans Verkuil]

On 9/12/19 10:10 PM, 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..e71b90a29d7a 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 device, typically an image sensor or a flash LED, mounting
> +  location expressed as a position relative to the usage orientation of the
> +  system where the device is installed on.

This sentence is a bit awkward. How about this:

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

system For -> system. For

Actually, I'd move the For to the next line.

> +  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, or is

I think you mean 'but is' instead of 'or is'.

> +  attached in a way that allows it to move freely.
> 
>  Optional endpoint properties
>  ----------------------------
> --
> 2.23.0
> 

Regards,

	Hans

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

[Pavel Machek]

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

Hi!

> 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..e71b90a29d7a 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 device, typically an image sensor or a flash LED, mounting
> +  location 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.

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?

Notebooks usually have just "selfie" camera, tablets often have
both... DSLRs have just "main" camera.

Best regards,

									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 v3 01/11] dt-bindings: video-interfaces: Document 'location' property

[Jacopo Mondi]

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

Hi Pavel,
   thanks for input

On Fri, Sep 27, 2019 at 05:27:45PM +0200, Pavel Machek wrote:
> Hi!
>
> > 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..e71b90a29d7a 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 device, typically an image sensor or a flash LED, mounting
> > +  location 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.
>
> 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.

>
> Notebooks usually have just "selfie" camera, tablets often have
> both... DSLRs have just "main" camera.
>
> 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 --]

[PATCH v3 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..f879dcc9409c 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 as the camera location is
+    expressed as a position relative to the device 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 it to move freely, such as
+    webcams and digital cameras, are said to be have ``V4L2_LOCATION_EXTERNAL``
+    location.
+
+
+
+.. flat-table::
+    :header-rows:  0
+    :stub-columns: 0
+
+    * - ``V4L2_LOCATION_FRONT``
+      - The camera sensor is located on the front side of the device.
+    * - ``V4L2_LOCATION_BACK``
+      - The camera sensor is located on the back side of the device.
+    * - ``V4L2_LOCATION_EXTERNAL``
+      - The camera sensor is 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

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

[Hans Verkuil]

On 9/12/19 10:10 PM, Jacopo Mondi wrote:
> 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..f879dcc9409c 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 as the camera location is

as -> since

> +    expressed as a position relative to the device intended usage orientation.

device -> device's

> +    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 it to move freely, such as

it -> them (plural)

> +    webcams and digital cameras, are said to be have ``V4L2_LOCATION_EXTERNAL``

to be have -> to have the

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

Regards,

	Hans

[PATCH v3 03/11] dt-bindings: video-interfaces: 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 e71b90a29d7a..8ab51e0bb63e 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 device, typically an image sensor, mounting rotation 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 device, typically an image sensor or a flash LED, mounting
   location expressed as a position relative to the usage orientation of the
--
2.23.0

Re: [PATCH v3 03/11] dt-bindings: video-interfaces: Expand rotation description

[Hans Verkuil]

On 9/12/19 10:10 PM, 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(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index e71b90a29d7a..8ab51e0bb63e 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 device, typically an image sensor, mounting rotation 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.

Same as with patch 1/11:

"The mount rotation of the device (typically an image sensor) expressed..."

Regards,

	Hans

> 
>  - location: The device, typically an image sensor or a flash LED, mounting
>    location expressed as a position relative to the usage orientation of the
> --
> 2.23.0
> 

[PATCH v3 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       | 117 ++++++++++++++++++
 1 file changed, 117 insertions(+)

diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
index f879dcc9409c..74991522ca3a 100644
--- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
+++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
@@ -542,6 +542,123 @@ 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 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,line-len
+              ! 1,0 1,1 1,2 ...
+              ! ...
+              ! ...
+              ! (num-col,0)...  (num-col,line-len)
+            y V
+
+
+    Assuming pixels are scanned out from (0,0) to (num-col,line-len)
+    progressively::
+
+             (0,0) ---->-------------> (0,line-len)---!
+             !------------------------------------<a--!
+             V
+             (1,0) ---->-------------> (1,line-len)---!
+             !------------------------------------<---!
+             V
+             (...) .-->--------------> ( ,,,, )    ---!
+             !------------------------------------<---!
+             V
+             (num-col,0)------------->(num-col,line-len)
+
+
+    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,line-len,,,(num-col,line-len
+              !  ....
+              !  0,2 1,2      ...
+              !  0,1 1,1      ...
+              !  0,0 1,0 ... num-col,0
+             (0,0)------------------------>
+                                   y
+
+    And the pixel scan-out sequence would then proceed as follows::
+
+            (0,line-len)            (num-cols,line-len)
+                 ^\    ^\    ^\    ^\    ^
+                 ! \   ! \   ! \   ! \   !
+                 !  \  !  \  !  \  !  \  !
+                 !   \ !   \ !   \ !   \ !
+                 !    \!    \!    \!    \!
+               (0,0)  (1,0) ....      (num-cols,0)
+
+    Which when applied to the capture scene gives::
+
+            (0,line-len)            (num-cols,line-len)
+                ^\    ^\    ^\    ^\    ^
+                ! \   ! \   0 \   ! \   !
+                !  \  !  \ -|- \  !  \  !
+                !   \ !    / \  \ !   \ !
+                !    \!    \!    \!    \!
+              (0,0)  (1,0) ....      (num-cols,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
+    displayed image.
+
+    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
+                -------
+                -------
+                | \ !  |  Resulting captured
+                |  --0 |  image when displayed
+                | / !  |  on screen
+                -------
+                -------
+                |   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

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

[Hans Verkuil]

On 9/12/19 10:10 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       | 117 ++++++++++++++++++
>  1 file changed, 117 insertions(+)
> 
> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> index f879dcc9409c..74991522ca3a 100644
> --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> @@ -542,6 +542,123 @@ 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 the device

compensate -> compensate for

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

Is this right? Shouldn't that be "in the clockwise direction"? If the sensor is
mounted 90 degrees counterclockwise, then I need to rotate by 90 degrees clockwise
to compensate for that, right?

> +
> +    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,line-len

Isn't that 0,0 ... 0,num-col?
line-len is a weird name, shouldn't that be num-lines?

line-len sounds like it is the same as num-col.

I'm totally confused.

> +              ! 1,0 1,1 1,2 ...
> +              ! ...
> +              ! ...
> +              ! (num-col,0)...  (num-col,line-len)
> +            y V
> +
> +
> +    Assuming pixels are scanned out from (0,0) to (num-col,line-len)
> +    progressively::
> +
> +             (0,0) ---->-------------> (0,line-len)---!
> +             !------------------------------------<a--!
> +             V
> +             (1,0) ---->-------------> (1,line-len)---!
> +             !------------------------------------<---!
> +             V
> +             (...) .-->--------------> ( ,,,, )    ---!
> +             !------------------------------------<---!
> +             V
> +             (num-col,0)------------->(num-col,line-len)
> +
> +
> +    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,line-len,,,(num-col,line-len
> +              !  ....
> +              !  0,2 1,2      ...
> +              !  0,1 1,1      ...
> +              !  0,0 1,0 ... num-col,0
> +             (0,0)------------------------>
> +                                   y
> +
> +    And the pixel scan-out sequence would then proceed as follows::
> +
> +            (0,line-len)            (num-cols,line-len)
> +                 ^\    ^\    ^\    ^\    ^
> +                 ! \   ! \   ! \   ! \   !
> +                 !  \  !  \  !  \  !  \  !
> +                 !   \ !   \ !   \ !   \ !
> +                 !    \!    \!    \!    \!
> +               (0,0)  (1,0) ....      (num-cols,0)
> +
> +    Which when applied to the capture scene gives::
> +
> +            (0,line-len)            (num-cols,line-len)
> +                ^\    ^\    ^\    ^\    ^
> +                ! \   ! \   0 \   ! \   !
> +                !  \  !  \ -|- \  !  \  !
> +                !   \ !    / \  \ !   \ !
> +                !    \!    \!    \!    \!
> +              (0,0)  (1,0) ....      (num-cols,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
> +    displayed image.
> +
> +    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
> +                -------
> +                -------
> +                | \ !  |  Resulting captured
> +                |  --0 |  image when displayed
> +                | / !  |  on screen
> +                -------

Trying this with my webcam turning it 90 degrees counterclockwise, I
and up with my head to the left, not to the right.

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

Regards,

	Hans

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

[Jacopo Mondi]

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

Hi Hans,

On Fri, Sep 13, 2019 at 04:02:45PM +0200, Hans Verkuil wrote:
> On 9/12/19 10:10 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       | 117 ++++++++++++++++++
> >  1 file changed, 117 insertions(+)
> >
> > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > index f879dcc9409c..74991522ca3a 100644
> > --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > @@ -542,6 +542,123 @@ 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 the device
>
> compensate -> compensate for
>
> > +    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.
>
> Is this right? Shouldn't that be "in the clockwise direction"? If the sensor is
> mounted 90 degrees counterclockwise, then I need to rotate by 90 degrees clockwise
> to compensate for that, right?
>

It really depend along which axis direction you are applying the mounting
rotation and the compensation rotation... See below...

> > +
> > +    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,line-len
>
> Isn't that 0,0 ... 0,num-col?

Yes indeed sorry

> line-len is a weird name, shouldn't that be num-lines?
>
> line-len sounds like it is the same as num-col.
>
> I'm totally confused.
>

num-col is totally wrong, that should have been num-lines

In general
s/line-len/num-col
s/num-col/num-lines

> > +              ! 1,0 1,1 1,2 ...
> > +              ! ...
> > +              ! ...
> > +              ! (num-col,0)...  (num-col,line-len)
> > +            y V
> > +
> > +
> > +    Assuming pixels are scanned out from (0,0) to (num-col,line-len)
> > +    progressively::
> > +
> > +             (0,0) ---->-------------> (0,line-len)---!
> > +             !------------------------------------<a--!
> > +             V
> > +             (1,0) ---->-------------> (1,line-len)---!
> > +             !------------------------------------<---!
> > +             V
> > +             (...) .-->--------------> ( ,,,, )    ---!
> > +             !------------------------------------<---!
> > +             V
> > +             (num-col,0)------------->(num-col,line-len)
> > +
> > +
> > +    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,line-len,,,(num-col,line-len
> > +              !  ....
> > +              !  0,2 1,2      ...
> > +              !  0,1 1,1      ...
> > +              !  0,0 1,0 ... num-col,0
> > +             (0,0)------------------------>
> > +                                   y
> > +
> > +    And the pixel scan-out sequence would then proceed as follows::
> > +
> > +            (0,line-len)            (num-cols,line-len)
> > +                 ^\    ^\    ^\    ^\    ^
> > +                 ! \   ! \   ! \   ! \   !
> > +                 !  \  !  \  !  \  !  \  !
> > +                 !   \ !   \ !   \ !   \ !
> > +                 !    \!    \!    \!    \!
> > +               (0,0)  (1,0) ....      (num-cols,0)
> > +
> > +    Which when applied to the capture scene gives::
> > +
> > +            (0,line-len)            (num-cols,line-len)
> > +                ^\    ^\    ^\    ^\    ^
> > +                ! \   ! \   0 \   ! \   !
> > +                !  \  !  \ -|- \  !  \  !
> > +                !   \ !    / \  \ !   \ !
> > +                !    \!    \!    \!    \!
> > +              (0,0)  (1,0) ....      (num-cols,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
> > +    displayed image.
> > +
> > +    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.::
>
> .:: -> :
>

Don't I need the :: to mark the following block of text as verbatim ?

> > +
> > +                -------   90 degree counterclockwise
> > +                |   o  |  mounting rotation applied
> > +                |  -|- |  along the axis directed
> > +                |  / \ |  away from the sensor lens
> > +                -------
> > +                -------
> > +                | \ !  |  Resulting captured
> > +                |  --0 |  image when displayed
> > +                | / !  |  on screen
> > +                -------
>
> Trying this with my webcam turning it 90 degrees counterclockwise, I
> and up with my head to the left, not to the right.
>

Along which axis direction are you rotating the camera counterclockwise ?

If you see your face, and you rotate the camera counterclockwise while
looking at it, you're actually rotating along the axis directed -towards-
the sensor.

The rotation here in the example and in the 'rotation' property
description has to be applied along the axis pointing aways from the
sensor, so what you're actually doing is rotating clockwise along that
direction (I guess)... So yes, to compensate that, you need to rotate
clockwise when you look at the image on the screen... Confusing,
right?

> > +                -------
> > +                |   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.
> >
>
> Regards,
>
> 	Hans

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

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

[Jacopo Mondi]

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

Hi Hans,

On Fri, Sep 13, 2019 at 08:49:06PM +0200, Jacopo Mondi wrote:
> Hi Hans,
>
> On Fri, Sep 13, 2019 at 04:02:45PM +0200, Hans Verkuil wrote:
> > On 9/12/19 10:10 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       | 117 ++++++++++++++++++
> > >  1 file changed, 117 insertions(+)
> > >
> > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > > index f879dcc9409c..74991522ca3a 100644
> > > --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > > +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > > @@ -542,6 +542,123 @@ 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 the device
> >
> > compensate -> compensate for
> >
> > > +    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.
> >
> > Is this right? Shouldn't that be "in the clockwise direction"? If the sensor is
> > mounted 90 degrees counterclockwise, then I need to rotate by 90 degrees clockwise
> > to compensate for that, right?
> >
>
> It really depend along which axis direction you are applying the mounting
> rotation and the compensation rotation... See below...
>
> > > +
> > > +    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,line-len
> >
> > Isn't that 0,0 ... 0,num-col?
>
> Yes indeed sorry
>
> > line-len is a weird name, shouldn't that be num-lines?
> >
> > line-len sounds like it is the same as num-col.
> >
> > I'm totally confused.
> >
>
> num-col is totally wrong, that should have been num-lines
>
> In general
> s/line-len/num-col
> s/num-col/num-lines
>
> > > +              ! 1,0 1,1 1,2 ...
> > > +              ! ...
> > > +              ! ...
> > > +              ! (num-col,0)...  (num-col,line-len)
> > > +            y V
> > > +
> > > +
> > > +    Assuming pixels are scanned out from (0,0) to (num-col,line-len)
> > > +    progressively::
> > > +
> > > +             (0,0) ---->-------------> (0,line-len)---!
> > > +             !------------------------------------<a--!
> > > +             V
> > > +             (1,0) ---->-------------> (1,line-len)---!
> > > +             !------------------------------------<---!
> > > +             V
> > > +             (...) .-->--------------> ( ,,,, )    ---!
> > > +             !------------------------------------<---!
> > > +             V
> > > +             (num-col,0)------------->(num-col,line-len)
> > > +
> > > +
> > > +    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,line-len,,,(num-col,line-len
> > > +              !  ....
> > > +              !  0,2 1,2      ...
> > > +              !  0,1 1,1      ...
> > > +              !  0,0 1,0 ... num-col,0
> > > +             (0,0)------------------------>
> > > +                                   y
> > > +
> > > +    And the pixel scan-out sequence would then proceed as follows::
> > > +
> > > +            (0,line-len)            (num-cols,line-len)
> > > +                 ^\    ^\    ^\    ^\    ^
> > > +                 ! \   ! \   ! \   ! \   !
> > > +                 !  \  !  \  !  \  !  \  !
> > > +                 !   \ !   \ !   \ !   \ !
> > > +                 !    \!    \!    \!    \!
> > > +               (0,0)  (1,0) ....      (num-cols,0)
> > > +
> > > +    Which when applied to the capture scene gives::
> > > +
> > > +            (0,line-len)            (num-cols,line-len)
> > > +                ^\    ^\    ^\    ^\    ^
> > > +                ! \   ! \   0 \   ! \   !
> > > +                !  \  !  \ -|- \  !  \  !
> > > +                !   \ !    / \  \ !   \ !
> > > +                !    \!    \!    \!    \!
> > > +              (0,0)  (1,0) ....      (num-cols,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
> > > +    displayed image.
> > > +
> > > +    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.::
> >
> > .:: -> :
> >
>
> Don't I need the :: to mark the following block of text as verbatim ?
>
> > > +
> > > +                -------   90 degree counterclockwise
> > > +                |   o  |  mounting rotation applied
> > > +                |  -|- |  along the axis directed
> > > +                |  / \ |  away from the sensor lens
> > > +                -------
> > > +                -------
> > > +                | \ !  |  Resulting captured
> > > +                |  --0 |  image when displayed
> > > +                | / !  |  on screen
> > > +                -------
> >
> > Trying this with my webcam turning it 90 degrees counterclockwise, I
> > and up with my head to the left, not to the right.
> >
>
> Along which axis direction are you rotating the camera counterclockwise ?
>
> If you see your face, and you rotate the camera counterclockwise while
> looking at it, you're actually rotating along the axis directed -towards-
> the sensor.
>
> The rotation here in the example and in the 'rotation' property
> description has to be applied along the axis pointing aways from the
> sensor, so what you're actually doing is rotating clockwise along that
> direction (I guess)... So yes, to compensate that, you need to rotate
> clockwise when you look at the image on the screen... Confusing,
> right?
>

Does it work for you? Can I send a new iteration with your comments on
the previous patches taken in ?

Thanks
  j

> > > +                -------
> > > +                |   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.
> > >
> >
> > Regards,
> >
> > 	Hans



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

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

[Hans Verkuil]

Hi Jacopo,

Sorry for the late reply, it fell through the cracks...

On 9/13/19 8:49 PM, Jacopo Mondi wrote:
> Hi Hans,
> 
> On Fri, Sep 13, 2019 at 04:02:45PM +0200, Hans Verkuil wrote:
>> On 9/12/19 10:10 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       | 117 ++++++++++++++++++
>>>  1 file changed, 117 insertions(+)
>>>
>>> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
>>> index f879dcc9409c..74991522ca3a 100644
>>> --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
>>> +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
>>> @@ -542,6 +542,123 @@ 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 the device
>>
>> compensate -> compensate for
>>
>>> +    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.
>>
>> Is this right? Shouldn't that be "in the clockwise direction"? If the sensor is
>> mounted 90 degrees counterclockwise, then I need to rotate by 90 degrees clockwise
>> to compensate for that, right?
>>
> 
> It really depend along which axis direction you are applying the mounting
> rotation and the compensation rotation... See below...
> 
>>> +
>>> +    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,line-len
>>
>> Isn't that 0,0 ... 0,num-col?
> 
> Yes indeed sorry
> 
>> line-len is a weird name, shouldn't that be num-lines?
>>
>> line-len sounds like it is the same as num-col.
>>
>> I'm totally confused.
>>
> 
> num-col is totally wrong, that should have been num-lines
> 
> In general
> s/line-len/num-col
> s/num-col/num-lines
> 
>>> +              ! 1,0 1,1 1,2 ...
>>> +              ! ...
>>> +              ! ...
>>> +              ! (num-col,0)...  (num-col,line-len)
>>> +            y V
>>> +
>>> +
>>> +    Assuming pixels are scanned out from (0,0) to (num-col,line-len)
>>> +    progressively::
>>> +
>>> +             (0,0) ---->-------------> (0,line-len)---!
>>> +             !------------------------------------<a--!
>>> +             V
>>> +             (1,0) ---->-------------> (1,line-len)---!
>>> +             !------------------------------------<---!
>>> +             V
>>> +             (...) .-->--------------> ( ,,,, )    ---!
>>> +             !------------------------------------<---!
>>> +             V
>>> +             (num-col,0)------------->(num-col,line-len)
>>> +
>>> +
>>> +    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,line-len,,,(num-col,line-len
>>> +              !  ....
>>> +              !  0,2 1,2      ...
>>> +              !  0,1 1,1      ...
>>> +              !  0,0 1,0 ... num-col,0
>>> +             (0,0)------------------------>
>>> +                                   y
>>> +
>>> +    And the pixel scan-out sequence would then proceed as follows::
>>> +
>>> +            (0,line-len)            (num-cols,line-len)
>>> +                 ^\    ^\    ^\    ^\    ^
>>> +                 ! \   ! \   ! \   ! \   !
>>> +                 !  \  !  \  !  \  !  \  !
>>> +                 !   \ !   \ !   \ !   \ !
>>> +                 !    \!    \!    \!    \!
>>> +               (0,0)  (1,0) ....      (num-cols,0)
>>> +
>>> +    Which when applied to the capture scene gives::
>>> +
>>> +            (0,line-len)            (num-cols,line-len)
>>> +                ^\    ^\    ^\    ^\    ^
>>> +                ! \   ! \   0 \   ! \   !
>>> +                !  \  !  \ -|- \  !  \  !
>>> +                !   \ !    / \  \ !   \ !
>>> +                !    \!    \!    \!    \!
>>> +              (0,0)  (1,0) ....      (num-cols,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
>>> +    displayed image.
>>> +
>>> +    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.::
>>
>> .:: -> :
>>
> 
> Don't I need the :: to mark the following block of text as verbatim ?

Ah, sorry, this is for the markup. I missed that.

> 
>>> +
>>> +                -------   90 degree counterclockwise
>>> +                |   o  |  mounting rotation applied
>>> +                |  -|- |  along the axis directed
>>> +                |  / \ |  away from the sensor lens
>>> +                -------
>>> +                -------
>>> +                | \ !  |  Resulting captured
>>> +                |  --0 |  image when displayed
>>> +                | / !  |  on screen
>>> +                -------
>>
>> Trying this with my webcam turning it 90 degrees counterclockwise, I
>> and up with my head to the left, not to the right.
>>
> 
> Along which axis direction are you rotating the camera counterclockwise ?
> 
> If you see your face, and you rotate the camera counterclockwise while
> looking at it, you're actually rotating along the axis directed -towards-
> the sensor.
> 
> The rotation here in the example and in the 'rotation' property
> description has to be applied along the axis pointing aways from the
> sensor, so what you're actually doing is rotating clockwise along that
> direction (I guess)... So yes, to compensate that, you need to rotate
> clockwise when you look at the image on the screen... Confusing,
> right?

I think you are right, but let me take another look at this when v4 is
posted. The line-len/num-col confusion didn't help :-)

Regards,

	Hans

> 
>>> +                -------
>>> +                |   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.
>>>
>>
>> Regards,
>>
>> 	Hans

[PATCH v3 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 v3 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.

Initially 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           | 40 +++++++++++++++++++++++++
 2 files changed, 82 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index 3bd1888787eb..d325a2d5c088 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -595,6 +595,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));
+
+	dev_dbg(dev, "===== begin V4L2 device properties parsing\n");
+	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);
+	}
+
+	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);
+	}
+	dev_dbg(dev, "===== end V4L2 device properties parsing\n");
+
+	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..86af6d9d11fe 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -109,6 +109,28 @@ struct v4l2_fwnode_endpoint {
 	unsigned int nr_of_link_frequencies;
 };
 
+/**
+ * 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 +255,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

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

[Hans Verkuil]

On 9/12/19 10:10 PM, 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.
> 
> Initially parse the 'rotation' and 'location' properties from the

Just drop 'Initially'. It's redundant.

> firmware interface.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/v4l2-core/v4l2-fwnode.c | 42 +++++++++++++++++++++++++++
>  include/media/v4l2-fwnode.h           | 40 +++++++++++++++++++++++++
>  2 files changed, 82 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 3bd1888787eb..d325a2d5c088 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -595,6 +595,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));
> +
> +	dev_dbg(dev, "===== begin V4L2 device properties parsing\n");
> +	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);
> +	}
> +
> +	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);
> +	}
> +	dev_dbg(dev, "===== end V4L2 device properties parsing\n");

Are these dev_dbg lines really needed? It seems a bit overkill to me.

What if rotation is set, but not location. Then location defaults to Front.
Is that what we want, or should we add an UNKNOWN location?

> +
> +	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..86af6d9d11fe 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -109,6 +109,28 @@ struct v4l2_fwnode_endpoint {
>  	unsigned int nr_of_link_frequencies;
>  };
>  
> +/**
> + * 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 +255,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.
> 

Regards,

	Hans

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

[Jacopo Mondi]

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

Hi Hans,

On Fri, Sep 13, 2019 at 04:08:10PM +0200, Hans Verkuil wrote:
> On 9/12/19 10:10 PM, 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.
> >
> > Initially parse the 'rotation' and 'location' properties from the
>
> Just drop 'Initially'. It's redundant.
>
> > firmware interface.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  drivers/media/v4l2-core/v4l2-fwnode.c | 42 +++++++++++++++++++++++++++
> >  include/media/v4l2-fwnode.h           | 40 +++++++++++++++++++++++++
> >  2 files changed, 82 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > index 3bd1888787eb..d325a2d5c088 100644
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -595,6 +595,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));
> > +
> > +	dev_dbg(dev, "===== begin V4L2 device properties parsing\n");
> > +	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);
> > +	}
> > +
> > +	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);
> > +	}
> > +	dev_dbg(dev, "===== end V4L2 device properties parsing\n");
>
> Are these dev_dbg lines really needed? It seems a bit overkill to me.
>

It's the same debug output we have in v4l2_fwnode_endpoint_parse() so
I tried to be consistent. Should I drop it?

> What if rotation is set, but not location. Then location defaults to Front.
> Is that what we want, or should we add an UNKNOWN location?
>

As you have noticed I have added a value to encode UNKNOW in the later
patch, I should probably add them in this one as you suggested ?

> > +
> > +	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..86af6d9d11fe 100644
> > --- a/include/media/v4l2-fwnode.h
> > +++ b/include/media/v4l2-fwnode.h
> > @@ -109,6 +109,28 @@ struct v4l2_fwnode_endpoint {
> >  	unsigned int nr_of_link_frequencies;
> >  };
> >
> > +/**
> > + * 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 +255,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.
> >
>
> Regards,
>
> 	Hans

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

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

[Hans Verkuil]

Hi Jacopo,

On 9/13/19 9:04 PM, Jacopo Mondi wrote:
> Hi Hans,
> 
> On Fri, Sep 13, 2019 at 04:08:10PM +0200, Hans Verkuil wrote:
>> On 9/12/19 10:10 PM, 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.
>>>
>>> Initially parse the 'rotation' and 'location' properties from the
>>
>> Just drop 'Initially'. It's redundant.
>>
>>> firmware interface.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>> ---
>>>  drivers/media/v4l2-core/v4l2-fwnode.c | 42 +++++++++++++++++++++++++++
>>>  include/media/v4l2-fwnode.h           | 40 +++++++++++++++++++++++++
>>>  2 files changed, 82 insertions(+)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
>>> index 3bd1888787eb..d325a2d5c088 100644
>>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
>>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
>>> @@ -595,6 +595,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));
>>> +
>>> +	dev_dbg(dev, "===== begin V4L2 device properties parsing\n");
>>> +	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);
>>> +	}
>>> +
>>> +	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);
>>> +	}
>>> +	dev_dbg(dev, "===== end V4L2 device properties parsing\n");
>>
>> Are these dev_dbg lines really needed? It seems a bit overkill to me.
>>
> 
> It's the same debug output we have in v4l2_fwnode_endpoint_parse() so
> I tried to be consistent. Should I drop it?

I think so, but check with Sakari. If he wants to keep it, then that's fine
with me.

> 
>> What if rotation is set, but not location. Then location defaults to Front.
>> Is that what we want, or should we add an UNKNOWN location?
>>
> 
> As you have noticed I have added a value to encode UNKNOW in the later
> patch, I should probably add them in this one as you suggested ?

Yes, please.

> 
>>> +
>>> +	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..86af6d9d11fe 100644
>>> --- a/include/media/v4l2-fwnode.h
>>> +++ b/include/media/v4l2-fwnode.h
>>> @@ -109,6 +109,28 @@ struct v4l2_fwnode_endpoint {
>>>  	unsigned int nr_of_link_frequencies;
>>>  };
>>>
>>> +/**
>>> + * 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 +255,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.
>>>
>>
>> Regards,
>>
>> 	Hans

Regards,

	Hans

[PATCH v3 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 v3 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 v3 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. Add an UNSET flag to the device properties to
distinguish uninitialized properties from properties with an actual
value at control registration time.

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

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 6fb94968a98d..46e170f04aed 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,44 @@ __poll_t v4l2_ctrl_poll(struct file *file, struct poll_table_struct *wait)
 	return 0;
 }
 EXPORT_SYMBOL(v4l2_ctrl_poll);
+
+int v4l2_ctrl_register_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 &&
+	    !v4l2_ctrl_find(hdl, V4L2_CID_CAMERA_SENSOR_LOCATION)) {
+		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 &&
+	    !v4l2_ctrl_find(hdl, V4L2_CID_CAMERA_SENSOR_ROTATION)) {
+		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_register_properties);
diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index d325a2d5c088..e4fed288e498 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -605,6 +605,7 @@ int v4l2_fwnode_device_parse(struct device *dev,
 	memset(props, 0, sizeof(*props));
 
 	dev_dbg(dev, "===== begin V4L2 device properties parsing\n");
+	props->location = V4L2_FWNODE_PROPERTY_UNSET;
 	ret = fwnode_property_read_u32(fwnode, "location", &val);
 	if (!ret) {
 		switch (val) {
@@ -621,6 +622,7 @@ int v4l2_fwnode_device_parse(struct device *dev,
 		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) {
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 95b4fa6243d1..ce73911f180b 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,31 @@ 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_register_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.
+ *
+ * NOTE: This function locks the @hdl control handler mutex, the caller shall
+ * not hold the lock when calling this function.
+ *
+ * Return: 0 on success, a negative error code on failure.
+ */
+int v4l2_ctrl_register_properties(struct v4l2_ctrl_handler *hdl,
+				  const struct v4l2_ctrl_ops *ctrl_ops,
+				  const struct v4l2_fwnode_device_properties *p);
 #endif
diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
index 86af6d9d11fe..ae02db22c70e 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -109,6 +109,8 @@ struct v4l2_fwnode_endpoint {
 	unsigned int nr_of_link_frequencies;
 };
 
+#define V4L2_FWNODE_PROPERTY_UNSET	(-1U)
+
 /**
  * enum v4l2_fwnode_location - possible device locations
  * @V4L2_FWNODE_LOCATION_FRONT: device installed on the front side
-- 
2.23.0

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

[Hans Verkuil]

On 9/12/19 10:10 PM, Jacopo Mondi wrote:
> Add an helper function to v4l2-ctrls to register controls associated
> with a device property. Add an UNSET flag to the device properties to
> distinguish uninitialized properties from properties with an actual
> value at control registration time.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/v4l2-core/v4l2-ctrls.c  | 42 +++++++++++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-fwnode.c |  2 ++
>  include/media/v4l2-ctrls.h            | 28 ++++++++++++++++++
>  include/media/v4l2-fwnode.h           |  2 ++
>  4 files changed, 74 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 6fb94968a98d..46e170f04aed 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,44 @@ __poll_t v4l2_ctrl_poll(struct file *file, struct poll_table_struct *wait)
>  	return 0;
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_poll);
> +
> +int v4l2_ctrl_register_properties(struct v4l2_ctrl_handler *hdl,

Hmm, let's call this:

v4l2_ctrl_new_fwnode_properties().

You don't register properties, you add new controls representing properties.

And v4l2_ctrl_new_ is in line with existing functions that create controls.

> +				  const struct v4l2_ctrl_ops *ctrl_ops,
> +				  const struct v4l2_fwnode_device_properties *p)
> +{
> +	if (p->location != V4L2_FWNODE_PROPERTY_UNSET &&
> +	    !v4l2_ctrl_find(hdl, V4L2_CID_CAMERA_SENSOR_LOCATION)) {

No need for v4l2_ctrl_find: if the control already exists, then the
control framework will automatically ignore the duplicate control.

> +		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 &&
> +	    !v4l2_ctrl_find(hdl, V4L2_CID_CAMERA_SENSOR_ROTATION)) {

Ditto.

> +		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_register_properties);

I like this split into one fwnode parse function and one ctrl function
much better.

> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index d325a2d5c088..e4fed288e498 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -605,6 +605,7 @@ int v4l2_fwnode_device_parse(struct device *dev,
>  	memset(props, 0, sizeof(*props));
>  
>  	dev_dbg(dev, "===== begin V4L2 device properties parsing\n");
> +	props->location = V4L2_FWNODE_PROPERTY_UNSET;
>  	ret = fwnode_property_read_u32(fwnode, "location", &val);
>  	if (!ret) {
>  		switch (val) {
> @@ -621,6 +622,7 @@ int v4l2_fwnode_device_parse(struct device *dev,
>  		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) {
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 95b4fa6243d1..ce73911f180b 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,31 @@ 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_register_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.
> + *
> + * NOTE: This function locks the @hdl control handler mutex, the caller shall
> + * not hold the lock when calling this function.
> + *
> + * Return: 0 on success, a negative error code on failure.
> + */
> +int v4l2_ctrl_register_properties(struct v4l2_ctrl_handler *hdl,
> +				  const struct v4l2_ctrl_ops *ctrl_ops,
> +				  const struct v4l2_fwnode_device_properties *p);
>  #endif
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index 86af6d9d11fe..ae02db22c70e 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -109,6 +109,8 @@ struct v4l2_fwnode_endpoint {
>  	unsigned int nr_of_link_frequencies;
>  };
>  
> +#define V4L2_FWNODE_PROPERTY_UNSET	(-1U)
> +

Ah, that answers my previous question about unset properties.

I think this fwnode addition should be done in a separate earlier patch.

And it needs a comment.

Regards,

	Hans

>  /**
>   * enum v4l2_fwnode_location - possible device locations
>   * @V4L2_FWNODE_LOCATION_FRONT: device installed on the front side
> 

[PATCH v3 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..3d14f16644ff 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_register_properties(ctrl_hdlr, &ov5670_ctrl_ops,
+					    &props);
+	if (ret)
+		return ret;
+
 	ov5670->sd.ctrl_handler = ctrl_hdlr;
 
 	return 0;
-- 
2.23.0

[PATCH v3 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..aaad8c1522a3 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_register_properties(ctrl_hdlr, &ov13858_ctrl_ops,
+					    &props);
+	if (ret)
+		return ret;
+
 	ov13858->sd.ctrl_handler = ctrl_hdlr;
 
 	return 0;
-- 
2.23.0