[RFC 0/5] media: v4l2-ctrls: Add camera 'location' support

[RFC 0/5] media: v4l2-ctrls: Add camera 'location' support

[Jacopo Mondi]

Hello,
  as anticipared on #v4l, this is a proposal to add a way for camera devices
to report their mounting location to user-space. The information on the camera
location is particularly meaningful for mobile devices, and in the process of
integrating libcamera with the Android camera stack, we found the need to report
this information to applications and there's currently no way to retrieve it
from the V4L2 API

The idea is to provide a camera class read-only control, named 'Location',
that is used to report the camera mounting position, which is retrieved from
the firmware interface. Hans mentioned v4l2-properties as a better mechanism to
report such static informations, but since they're not here I think a control
would do for now...

The RFC adds a DT standard property to video-interfaces.txt to express the
location in DTS (I actually tested it on ACPI, but..), and then adds a new
control in the camera class to expose it. Finally, two example sensor drivers I
used for testing have been modified to expose the read-only control.

Why an RFC: mostly because of the nature and definition of the property. It's
tricky to define the "location" semantic clearly as it really depends
on the device the camera it is installed on. To begin with, I only defined
FRONT and BACK locations, and documented them the best I could.

Also the control itself it's probably more appropriate expressed as an integer
menu control. However, given the few supported values, and the fact they are
standard, I went for a standard integer control and defined a few macros for the
possible values as setting up a menu control requires a bit more effort in
drivers which I'm not sure it is worth it.

I tried out a few different names for the property as well, such as
"orientation", "position", "mounting-position" but none of them really satisfied
me, so I picked the most simple one. Feedbacks are welcome here too.

Thanks
  j


Jacopo Mondi (5):
  media: dt-bindings: Document 'location' property
  media: v4l2-ctrl: Document V4L2_CID_LOCATION
  media: v4l2-ctrls: Add support for V4L2_CID_LOCATION
  media: i2c: ov5670: Report the camera location
  media: i2c: ov13858: Report the camera location

 .../bindings/media/video-interfaces.txt       |  4 ++++
 .../media/uapi/v4l/ext-ctrls-camera.rst       | 23 +++++++++++++++++++
 drivers/media/i2c/ov13858.c                   | 11 +++++++++
 drivers/media/i2c/ov5670.c                    | 11 +++++++++
 drivers/media/v4l2-core/v4l2-ctrls.c          |  7 ++++++
 include/uapi/linux/v4l2-controls.h            |  4 ++++
 6 files changed, 60 insertions(+)

--
2.22.0

[RFC 1/5] media: dt-bindings: Document 'location' property

[Jacopo Mondi]

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

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

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

+- location: The camera device mounting position, relative to the device
+  usage orientation. Possible values are:
+  0 - Front camera. The image sensor is mounted on the front side of the device.
+  1 - Back camera. The image sensor is mounted on the back side of the device.

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

[RFC 2/5] media: v4l2-ctrl: Document V4L2_CID_LOCATION

[Jacopo Mondi]

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

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

diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
index 51c1d5c9eb00..fc0a02eee6d4 100644
--- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
+++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
@@ -510,6 +510,29 @@ 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_LOCATION (integer)``
+    This read-only control describes the camera location by reporting its
+    mounting position on the device where the camera is installed. 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 position. In example, a camera installed on the user-facing side
+    of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT``
+    position.
+
+
+
+.. flat-table::
+    :header-rows:  0
+    :stub-columns: 0
+
+    * - ``V4L2_LOCATION_FRONT``
+      - The camera device is located on the front side of the device.
+    * - ``V4L2_LOCATION_BACK``
+      - The camera device is located on the back side of the device.
+
+
+
 .. [#f1]
    This control may be changed to a menu control in the future, if more
    options are required.
--
2.22.0

[RFC 3/5] media: v4l2-ctrls: Add support for V4L2_CID_LOCATION

[Jacopo Mondi]

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

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

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 7d3a33258748..8ab0857df59a 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -943,6 +943,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_AUTO_FOCUS_RANGE:		return "Auto Focus, Range";
 	case V4L2_CID_PAN_SPEED:		return "Pan, Speed";
 	case V4L2_CID_TILT_SPEED:		return "Tilt, Speed";
+	case V4L2_CID_LOCATION:			return "Location";
 
 	/* FM Radio Modulator controls */
 	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1300,6 +1301,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 		break;
 	case V4L2_CID_MPEG_VIDEO_FWHT_PARAMS:
 		*type = V4L2_CTRL_TYPE_FWHT_PARAMS;
+	case V4L2_CID_LOCATION:
+		*type = V4L2_CTRL_TYPE_INTEGER;
+		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
+		*min = V4L2_LOCATION_FRONT;
+		*max = V4L2_LOCATION_BACK;
+		*step = 1;
 		break;
 	default:
 		*type = V4L2_CTRL_TYPE_INTEGER;
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 37807f23231e..5c4c7b245921 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -889,6 +889,10 @@ 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_LOCATION			(V4L2_CID_CAMERA_CLASS_BASE+34)
+#define V4L2_LOCATION_FRONT			(0 << 0)
+#define V4L2_LOCATION_BACK			(1 << 0)
+
 /* FM Modulator class control IDs */
 
 #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
-- 
2.22.0

[RFC 4/5] media: i2c: ov5670: Report the camera location

[Jacopo Mondi]

The camera location is retrieved from the firmware interface parsing
the "location" device property and reported through the read-only
V4L2_CID_LOCATION control.

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

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 041fcbb4eebd..1446aef1fc1e 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -2059,10 +2059,12 @@ static const struct v4l2_ctrl_ops ov5670_ctrl_ops = {
 /* Initialize control handlers */
 static int ov5670_init_controls(struct ov5670 *ov5670)
 {
+	struct i2c_client *client = v4l2_get_subdevdata(&ov5670->sd);
 	struct v4l2_ctrl_handler *ctrl_hdlr;
 	s64 vblank_max;
 	s64 vblank_def;
 	s64 vblank_min;
+	u32 location;
 	s64 exposure_max;
 	int ret;
 
@@ -2124,6 +2126,15 @@ static int ov5670_init_controls(struct ov5670 *ov5670)
 				     ARRAY_SIZE(ov5670_test_pattern_menu) - 1,
 				     0, 0, ov5670_test_pattern_menu);
 
+	ret = device_property_read_u32(&client->dev, "location", &location);
+	if (!ret) {
+		v4l2_ctrl_new_std(ctrl_hdlr, &ov5670_ctrl_ops,
+				  V4L2_CID_LOCATION, V4L2_LOCATION_FRONT,
+				  V4L2_LOCATION_BACK, 1,
+				  location == V4L2_LOCATION_FRONT ?
+				  V4L2_LOCATION_FRONT : V4L2_LOCATION_BACK);
+	}
+
 	if (ctrl_hdlr->error) {
 		ret = ctrl_hdlr->error;
 		goto error;
-- 
2.22.0

[RFC 5/5] media: i2c: ov13858: Report the camera location

[Jacopo Mondi]

The camera location is retrieved from the firmware interface parsing
the "location" device property and reported through the read-only
V4L2_CID_LOCATION control.

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 45bb872db3c5..6baefc3083e1 100644
--- a/drivers/media/i2c/ov13858.c
+++ b/drivers/media/i2c/ov13858.c
@@ -1591,6 +1591,7 @@ static int ov13858_init_controls(struct ov13858 *ov13858)
 	struct i2c_client *client = v4l2_get_subdevdata(&ov13858->sd);
 	struct v4l2_ctrl_handler *ctrl_hdlr;
 	s64 exposure_max;
+	u32 location;
 	s64 vblank_def;
 	s64 vblank_min;
 	s64 hblank;
@@ -1659,6 +1660,16 @@ static int ov13858_init_controls(struct ov13858 *ov13858)
 				     V4L2_CID_TEST_PATTERN,
 				     ARRAY_SIZE(ov13858_test_pattern_menu) - 1,
 				     0, 0, ov13858_test_pattern_menu);
+
+	ret = device_property_read_u32(&client->dev, "location", &location);
+	if (!ret) {
+		v4l2_ctrl_new_std(ctrl_hdlr, &ov13858_ctrl_ops,
+				  V4L2_CID_LOCATION, V4L2_LOCATION_FRONT,
+				  V4L2_LOCATION_BACK, 1,
+				  location == V4L2_LOCATION_FRONT ?
+				  V4L2_LOCATION_FRONT : V4L2_LOCATION_BACK);
+	}
+
 	if (ctrl_hdlr->error) {
 		ret = ctrl_hdlr->error;
 		dev_err(&client->dev, "%s control init failed (%d)\n",
-- 
2.22.0

Re: [RFC 1/5] media: dt-bindings: Document 'location' property

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Aug 14, 2019 at 10:28:11PM +0200, Jacopo Mondi wrote:
> Add the 'location' device property, used to specify the camera device
> mounting position. The property is particularly meaningful for mobile
> devices with a well defined usage orientation.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  Documentation/devicetree/bindings/media/video-interfaces.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index f884ada0bffc..819077b2649c 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -89,6 +89,10 @@ Optional properties
>    but a number of degrees counter clockwise. Typical values are 0 and 180
>    (upside down).
> 
> +- location: The camera device mounting position, relative to the device
> +  usage orientation. Possible values are:

I would mention "camera sensor" explicitly here, as well as clearly
stating that the property applies to camera sensors only.

> +  0 - Front camera. The image sensor is mounted on the front side of the device.
> +  1 - Back camera. The image sensor is mounted on the back side of the device.

An additional paragraph explained what "device usage orientation" means
would be useful. In particular I would give examples for phones, tablets
and laptops.

> 
>  Optional endpoint properties
>  ----------------------------

-- 
Regards,

Laurent Pinchart

Re: [RFC 2/5] media: v4l2-ctrl: Document V4L2_CID_LOCATION

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Aug 14, 2019 at 10:28:12PM +0200, Jacopo Mondi wrote:
> Add documentation for the V4L2_CID_LOCATION camera control. The newly
> added read-only control reports the camera device mounting position.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  .../media/uapi/v4l/ext-ctrls-camera.rst       | 23 +++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> index 51c1d5c9eb00..fc0a02eee6d4 100644
> --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> @@ -510,6 +510,29 @@ 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_LOCATION (integer)``

Maybe V4L2_CID_CAMERA_SENSOR_LOCATION ? Same for the values below.

> +    This read-only control describes the camera location by reporting its

Here too I would mention camera sensor instead of just camera (or
possibly imaging sensor).

> +    mounting position on the device where the camera is installed. 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 position. In example, a camera installed on the user-facing side
> +    of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT``
> +    position.

The DT bindings could use such an example :-) I would extend this to
tablets and laptops.

> +
> +
> +

Do we need three blank lines ?

> +.. flat-table::
> +    :header-rows:  0
> +    :stub-columns: 0
> +
> +    * - ``V4L2_LOCATION_FRONT``
> +      - The camera device is located on the front side of the device.
> +    * - ``V4L2_LOCATION_BACK``
> +      - The camera device is located on the back side of the device.
> +
> +
> +
>  .. [#f1]
>     This control may be changed to a menu control in the future, if more
>     options are required.

-- 
Regards,

Laurent Pinchart

Re: [RFC 3/5] media: v4l2-ctrls: Add support for V4L2_CID_LOCATION

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Aug 14, 2019 at 10:28:13PM +0200, Jacopo Mondi wrote:
> Add support for the newly defined V4L2_CID_LOCATION read-only control
> used to report the camera device mounting position.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/v4l2-core/v4l2-ctrls.c | 7 +++++++
>  include/uapi/linux/v4l2-controls.h   | 4 ++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 7d3a33258748..8ab0857df59a 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -943,6 +943,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_AUTO_FOCUS_RANGE:		return "Auto Focus, Range";
>  	case V4L2_CID_PAN_SPEED:		return "Pan, Speed";
>  	case V4L2_CID_TILT_SPEED:		return "Tilt, Speed";
> +	case V4L2_CID_LOCATION:			return "Location";

Depending on what we decide to name the control (see review of 2/5), you
should adjust the description accordingly.

>  
>  	/* FM Radio Modulator controls */
>  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> @@ -1300,6 +1301,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  		break;
>  	case V4L2_CID_MPEG_VIDEO_FWHT_PARAMS:
>  		*type = V4L2_CTRL_TYPE_FWHT_PARAMS;
> +	case V4L2_CID_LOCATION:
> +		*type = V4L2_CTRL_TYPE_INTEGER;
> +		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +		*min = V4L2_LOCATION_FRONT;
> +		*max = V4L2_LOCATION_BACK;

I don't think the control should have a min and a max different than the
current value, as it's a fully static control. I'd drop those two lines
here, and drivers will have to set value = min = max = V4L2_LOCATION_xxx
when creating the control. That why you should be able to collapse this
with V4L2_CID_MIN_BUFFERS_FOR_OUTPUT.

> +		*step = 1;
>  		break;
>  	default:
>  		*type = V4L2_CTRL_TYPE_INTEGER;
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 37807f23231e..5c4c7b245921 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -889,6 +889,10 @@ 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_LOCATION			(V4L2_CID_CAMERA_CLASS_BASE+34)
> +#define V4L2_LOCATION_FRONT			(0 << 0)
> +#define V4L2_LOCATION_BACK			(1 << 0)

Why not just 0 and 1 ?

> +
>  /* FM Modulator class control IDs */
>  
>  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)

-- 
Regards,

Laurent Pinchart

Re: [RFC 4/5] media: i2c: ov5670: Report the camera location

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Aug 14, 2019 at 10:28:14PM +0200, Jacopo Mondi wrote:
> The camera location is retrieved from the firmware interface parsing
> the "location" device property and reported through the read-only
> V4L2_CID_LOCATION control.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/i2c/ov5670.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> index 041fcbb4eebd..1446aef1fc1e 100644
> --- a/drivers/media/i2c/ov5670.c
> +++ b/drivers/media/i2c/ov5670.c
> @@ -2059,10 +2059,12 @@ static const struct v4l2_ctrl_ops ov5670_ctrl_ops = {
>  /* Initialize control handlers */
>  static int ov5670_init_controls(struct ov5670 *ov5670)
>  {
> +	struct i2c_client *client = v4l2_get_subdevdata(&ov5670->sd);
>  	struct v4l2_ctrl_handler *ctrl_hdlr;
>  	s64 vblank_max;
>  	s64 vblank_def;
>  	s64 vblank_min;
> +	u32 location;
>  	s64 exposure_max;
>  	int ret;
>  
> @@ -2124,6 +2126,15 @@ static int ov5670_init_controls(struct ov5670 *ov5670)
>  				     ARRAY_SIZE(ov5670_test_pattern_menu) - 1,
>  				     0, 0, ov5670_test_pattern_menu);
>  
> +	ret = device_property_read_u32(&client->dev, "location", &location);
> +	if (!ret) {
> +		v4l2_ctrl_new_std(ctrl_hdlr, &ov5670_ctrl_ops,
> +				  V4L2_CID_LOCATION, V4L2_LOCATION_FRONT,
> +				  V4L2_LOCATION_BACK, 1,
> +				  location == V4L2_LOCATION_FRONT ?
> +				  V4L2_LOCATION_FRONT : V4L2_LOCATION_BACK);
> +	}
> +

As most camera sensors should do this, I think it would make sense to
create a helper function. We could add parsing of other standard
sensor-related controls there (such as the link frequencies), and also
create the pixel rate control.

>  	if (ctrl_hdlr->error) {
>  		ret = ctrl_hdlr->error;
>  		goto error;

-- 
Regards,

Laurent Pinchart

Re: [RFC 1/5] media: dt-bindings: Document 'location' property

[Sakari Ailus]

Hi Jacopo,

On Wed, Aug 14, 2019 at 10:28:11PM +0200, Jacopo Mondi wrote:
> Add the 'location' device property, used to specify the camera device
> mounting position. The property is particularly meaningful for mobile
> devices with a well defined usage orientation.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  Documentation/devicetree/bindings/media/video-interfaces.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index f884ada0bffc..819077b2649c 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -89,6 +89,10 @@ Optional properties
>    but a number of degrees counter clockwise. Typical values are 0 and 180
>    (upside down).
> 
> +- location: The camera device mounting position, relative to the device
> +  usage orientation. Possible values are:
> +  0 - Front camera. The image sensor is mounted on the front side of the device.
> +  1 - Back camera. The image sensor is mounted on the back side of the device.

Would it make sense to make this a little more generic? Such as s/image
sensor/ device/, for instance?

Is this also relevant for flash or lens devices?

Flash (torch) devices could be present, at least principle, without a
camera. There once was even such a Nokia phone, 1100 unless I'm mistaken.
:-)

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [RFC 2/5] media: v4l2-ctrl: Document V4L2_CID_LOCATION

[Sakari Ailus]

Hi Jacopo,

On Wed, Aug 14, 2019 at 10:28:12PM +0200, Jacopo Mondi wrote:
> Add documentation for the V4L2_CID_LOCATION camera control. The newly
> added read-only control reports the camera device mounting position.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  .../media/uapi/v4l/ext-ctrls-camera.rst       | 23 +++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> index 51c1d5c9eb00..fc0a02eee6d4 100644
> --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> @@ -510,6 +510,29 @@ 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_LOCATION (integer)``
> +    This read-only control describes the camera location by reporting its
> +    mounting position on the device where the camera is installed. 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 position. In example, a camera installed on the user-facing side

s/In/For/

> +    of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT``
> +    position.
> +
> +
> +
> +.. flat-table::
> +    :header-rows:  0
> +    :stub-columns: 0
> +
> +    * - ``V4L2_LOCATION_FRONT``
> +      - The camera device is located on the front side of the device.
> +    * - ``V4L2_LOCATION_BACK``
> +      - The camera device is located on the back side of the device.
> +
> +
> +
>  .. [#f1]
>     This control may be changed to a menu control in the future, if more
>     options are required.

There's an effective limit of 64 for menus. ACPI has less than ten
different locations for a device, I think 64 will be enough here.

So I'd be actually in favour of switching to a menu.

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

Re: [RFC 4/5] media: i2c: ov5670: Report the camera location

[Sakari Ailus]

Hi Jacopo,

On Wed, Aug 14, 2019 at 10:28:14PM +0200, Jacopo Mondi wrote:
> The camera location is retrieved from the firmware interface parsing
> the "location" device property and reported through the read-only
> V4L2_CID_LOCATION control.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/i2c/ov5670.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> index 041fcbb4eebd..1446aef1fc1e 100644
> --- a/drivers/media/i2c/ov5670.c
> +++ b/drivers/media/i2c/ov5670.c
> @@ -2059,10 +2059,12 @@ static const struct v4l2_ctrl_ops ov5670_ctrl_ops = {
>  /* Initialize control handlers */
>  static int ov5670_init_controls(struct ov5670 *ov5670)
>  {
> +	struct i2c_client *client = v4l2_get_subdevdata(&ov5670->sd);
>  	struct v4l2_ctrl_handler *ctrl_hdlr;
>  	s64 vblank_max;
>  	s64 vblank_def;
>  	s64 vblank_min;
> +	u32 location;
>  	s64 exposure_max;
>  	int ret;
>  
> @@ -2124,6 +2126,15 @@ static int ov5670_init_controls(struct ov5670 *ov5670)
>  				     ARRAY_SIZE(ov5670_test_pattern_menu) - 1,
>  				     0, 0, ov5670_test_pattern_menu);
>  
> +	ret = device_property_read_u32(&client->dev, "location", &location);
> +	if (!ret) {
> +		v4l2_ctrl_new_std(ctrl_hdlr, &ov5670_ctrl_ops,
> +				  V4L2_CID_LOCATION, V4L2_LOCATION_FRONT,
> +				  V4L2_LOCATION_BACK, 1,
> +				  location == V4L2_LOCATION_FRONT ?

To do this, you'll need to document that the control values match DT
binding values. I don't think that's there currently.

A switch would be nicer, to check that the value that is not front is
something else meaningful. Otherwise I wouldn't create the control at all.

A helper function for drivers to use would be nice.

> +				  V4L2_LOCATION_FRONT : V4L2_LOCATION_BACK);
> +	}
> +
>  	if (ctrl_hdlr->error) {
>  		ret = ctrl_hdlr->error;
>  		goto error;

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [RFC 1/5] media: dt-bindings: Document 'location' property

[Laurent Pinchart]

Hi Sakari,

On Thu, Aug 15, 2019 at 09:56:35AM +0300, Sakari Ailus wrote:
> On Wed, Aug 14, 2019 at 10:28:11PM +0200, Jacopo Mondi wrote:
> > Add the 'location' device property, used to specify the camera device
> > mounting position. The property is particularly meaningful for mobile
> > devices with a well defined usage orientation.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  Documentation/devicetree/bindings/media/video-interfaces.txt | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > index f884ada0bffc..819077b2649c 100644
> > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > @@ -89,6 +89,10 @@ Optional properties
> >    but a number of degrees counter clockwise. Typical values are 0 and 180
> >    (upside down).
> > 
> > +- location: The camera device mounting position, relative to the device
> > +  usage orientation. Possible values are:
> > +  0 - Front camera. The image sensor is mounted on the front side of the device.
> > +  1 - Back camera. The image sensor is mounted on the back side of the device.
> 
> Would it make sense to make this a little more generic? Such as s/image
> sensor/ device/, for instance?
> 
> Is this also relevant for flash or lens devices?

I certainly hope that the flash or lens will be located on the same side
as the sensor... :-) It could however make sense to extend usage of this
property yet. I'm not sure I would do so already though, unless you
think all flash and lens controllers should really use it.

> Flash (torch) devices could be present, at least principle, without a
> camera. There once was even such a Nokia phone, 1100 unless I'm mistaken.
> :-)

-- 
Regards,

Laurent Pinchart

Re: [RFC 1/5] media: dt-bindings: Document 'location' property

[Jacopo Mondi]

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

Hi Sakari,

On Thu, Aug 15, 2019 at 09:56:35AM +0300, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Wed, Aug 14, 2019 at 10:28:11PM +0200, Jacopo Mondi wrote:
> > Add the 'location' device property, used to specify the camera device
> > mounting position. The property is particularly meaningful for mobile
> > devices with a well defined usage orientation.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  Documentation/devicetree/bindings/media/video-interfaces.txt | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > index f884ada0bffc..819077b2649c 100644
> > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > @@ -89,6 +89,10 @@ Optional properties
> >    but a number of degrees counter clockwise. Typical values are 0 and 180
> >    (upside down).
> >
> > +- location: The camera device mounting position, relative to the device
> > +  usage orientation. Possible values are:
> > +  0 - Front camera. The image sensor is mounted on the front side of the device.
> > +  1 - Back camera. The image sensor is mounted on the back side of the device.
>
> Would it make sense to make this a little more generic? Such as s/image
> sensor/ device/, for instance?

Laurent seems to be of the opposite opinion, but i think staying as
generic as possible might be a good idea. Now I have a linguistic
problem though.

 +- location: The device mounting position, relative to the device
 +  usage orientation. Possible values are:
 +  0 - Front. The device is mounted on the front side of the device.
 +  1 - Back. The device is mounted on the back side of the device.

So I need one "device" to indicate the lens/flash/image sensor and one
to indicate the device they're installed on :) Any idea?

>
> Is this also relevant for flash or lens devices?
>
> Flash (torch) devices could be present, at least principle, without a
> camera. There once was even such a Nokia phone, 1100 unless I'm mistaken.
> :-)

Not to mention that lenses are indeed installed in the same package as
the camera sensor, but they're described as separate device nodes as
flash leds are, so the mounting location might need to be specified in their
device node properties too, even if it would be the same as the image
sensor one.

>
> --
> Regards,
>
> Sakari Ailus
> sakari.ailus@linux.intel.com

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

Re: [RFC 2/5] media: v4l2-ctrl: Document V4L2_CID_LOCATION

[Jacopo Mondi]

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

Hi Laurent,

On Thu, Aug 15, 2019 at 01:43:40AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Aug 14, 2019 at 10:28:12PM +0200, Jacopo Mondi wrote:
> > Add documentation for the V4L2_CID_LOCATION camera control. The newly
> > added read-only control reports the camera device mounting position.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  .../media/uapi/v4l/ext-ctrls-camera.rst       | 23 +++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >
> > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > index 51c1d5c9eb00..fc0a02eee6d4 100644
> > --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > @@ -510,6 +510,29 @@ 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_LOCATION (integer)``
>
> Maybe V4L2_CID_CAMERA_SENSOR_LOCATION ? Same for the values below.
>
> > +    This read-only control describes the camera location by reporting its
>
> Here too I would mention camera sensor instead of just camera (or
> possibly imaging sensor).
>

Let's sort this out in the discussion on the dt property.

> > +    mounting position on the device where the camera is installed. 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 position. In example, a camera installed on the user-facing side
> > +    of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT``
> > +    position.
>
> The DT bindings could use such an example :-) I would extend this to
> tablets and laptops.

I could copy part of the text there and expand the example device
list.

>
> > +
> > +
> > +
>
> Do we need three blank lines ?
>

I don't know :) I copied this style from the other tables in the file.
There doesn't seem to a requirement about this, I just tried to keep
the style consistent :)

> > +.. flat-table::
> > +    :header-rows:  0
> > +    :stub-columns: 0
> > +
> > +    * - ``V4L2_LOCATION_FRONT``
> > +      - The camera device is located on the front side of the device.
> > +    * - ``V4L2_LOCATION_BACK``
> > +      - The camera device is located on the back side of the device.
> > +
> > +
> > +
> >  .. [#f1]
> >     This control may be changed to a menu control in the future, if more
> >     options are required.
>
> --
> Regards,
>
> Laurent Pinchart

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

Re: [RFC 1/5] media: dt-bindings: Document 'location' property

[Laurent Pinchart]

On Thu, Aug 15, 2019 at 02:55:48PM +0200, Jacopo Mondi wrote:
> On Thu, Aug 15, 2019 at 09:56:35AM +0300, Sakari Ailus wrote:
> > On Wed, Aug 14, 2019 at 10:28:11PM +0200, Jacopo Mondi wrote:
> > > Add the 'location' device property, used to specify the camera device
> > > mounting position. The property is particularly meaningful for mobile
> > > devices with a well defined usage orientation.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  Documentation/devicetree/bindings/media/video-interfaces.txt | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > index f884ada0bffc..819077b2649c 100644
> > > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > @@ -89,6 +89,10 @@ Optional properties
> > >    but a number of degrees counter clockwise. Typical values are 0 and 180
> > >    (upside down).
> > >
> > > +- location: The camera device mounting position, relative to the device
> > > +  usage orientation. Possible values are:
> > > +  0 - Front camera. The image sensor is mounted on the front side of the device.
> > > +  1 - Back camera. The image sensor is mounted on the back side of the device.
> >
> > Would it make sense to make this a little more generic? Such as s/image
> > sensor/ device/, for instance?
> 
> Laurent seems to be of the opposite opinion, but i think staying as
> generic as possible might be a good idea. Now I have a linguistic
> problem though.
> 
>  +- location: The device mounting position, relative to the device
>  +  usage orientation. Possible values are:
>  +  0 - Front. The device is mounted on the front side of the device.
>  +  1 - Back. The device is mounted on the back side of the device.
> 
> So I need one "device" to indicate the lens/flash/image sensor and one
> to indicate the device they're installed on :) Any idea?
> 
> >
> > Is this also relevant for flash or lens devices?
> >
> > Flash (torch) devices could be present, at least principle, without a
> > camera. There once was even such a Nokia phone, 1100 unless I'm mistaken.
> > :-)
> 
> Not to mention that lenses are indeed installed in the same package as
> the camera sensor, but they're described as separate device nodes as
> flash leds are, so the mounting location might need to be specified in their
> device node properties too, even if it would be the same as the image
> sensor one.

For the lens I really don't think we need it. For the flash, I envision
it will be useful to know more about its physical location relative to
the camera sensor, but that will be a displacement, not a front/back
location as the flash should really be on the same side as the camera
sensor :-) Note that, technically speaking, it will not be the location
of the flash controller itself, but of its LED (or other light source).
A flash controller could possibly control multiple LEDs, for different
sensors, and possibly on different sides of the devices, so we may need
to create subnodes for light sources in the flash controller DT node.

-- 
Regards,

Laurent Pinchart

Re: [RFC 2/5] media: v4l2-ctrl: Document V4L2_CID_LOCATION

[Laurent Pinchart]

Hi Sakari,

On Thu, Aug 15, 2019 at 10:00:25AM +0300, Sakari Ailus wrote:
> On Wed, Aug 14, 2019 at 10:28:12PM +0200, Jacopo Mondi wrote:
> > Add documentation for the V4L2_CID_LOCATION camera control. The newly
> > added read-only control reports the camera device mounting position.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  .../media/uapi/v4l/ext-ctrls-camera.rst       | 23 +++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > index 51c1d5c9eb00..fc0a02eee6d4 100644
> > --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > @@ -510,6 +510,29 @@ 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_LOCATION (integer)``
> > +    This read-only control describes the camera location by reporting its
> > +    mounting position on the device where the camera is installed. 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 position. In example, a camera installed on the user-facing side
> 
> s/In/For/
> 
> > +    of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT``
> > +    position.
> > +
> > +
> > +
> > +.. flat-table::
> > +    :header-rows:  0
> > +    :stub-columns: 0
> > +
> > +    * - ``V4L2_LOCATION_FRONT``
> > +      - The camera device is located on the front side of the device.
> > +    * - ``V4L2_LOCATION_BACK``
> > +      - The camera device is located on the back side of the device.
> > +
> > +
> > +
> >  .. [#f1]
> >     This control may be changed to a menu control in the future, if more
> >     options are required.
> 
> There's an effective limit of 64 for menus. ACPI has less than ten
> different locations for a device, I think 64 will be enough here.
> 
> So I'd be actually in favour of switching to a menu.

Why ? As you explained yourself, it's a static read-only control, all it
needs to report is a single value.

-- 
Regards,

Laurent Pinchart

Re: [RFC 3/5] media: v4l2-ctrls: Add support for V4L2_CID_LOCATION

[Jacopo Mondi]

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

Hi Laurent,

On Thu, Aug 15, 2019 at 01:53:53AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Aug 14, 2019 at 10:28:13PM +0200, Jacopo Mondi wrote:
> > Add support for the newly defined V4L2_CID_LOCATION read-only control
> > used to report the camera device mounting position.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  drivers/media/v4l2-core/v4l2-ctrls.c | 7 +++++++
> >  include/uapi/linux/v4l2-controls.h   | 4 ++++
> >  2 files changed, 11 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > index 7d3a33258748..8ab0857df59a 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > @@ -943,6 +943,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> >  	case V4L2_CID_AUTO_FOCUS_RANGE:		return "Auto Focus, Range";
> >  	case V4L2_CID_PAN_SPEED:		return "Pan, Speed";
> >  	case V4L2_CID_TILT_SPEED:		return "Tilt, Speed";
> > +	case V4L2_CID_LOCATION:			return "Location";
>
> Depending on what we decide to name the control (see review of 2/5), you
> should adjust the description accordingly.
>
> >
> >  	/* FM Radio Modulator controls */
> >  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> > @@ -1300,6 +1301,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> >  		break;
> >  	case V4L2_CID_MPEG_VIDEO_FWHT_PARAMS:
> >  		*type = V4L2_CTRL_TYPE_FWHT_PARAMS;
> > +	case V4L2_CID_LOCATION:
> > +		*type = V4L2_CTRL_TYPE_INTEGER;
> > +		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +		*min = V4L2_LOCATION_FRONT;
> > +		*max = V4L2_LOCATION_BACK;
>
> I don't think the control should have a min and a max different than the
> current value, as it's a fully static control. I'd drop those two lines
> here, and drivers will have to set value = min = max = V4L2_LOCATION_xxx
> when creating the control. That why you should be able to collapse this
> with V4L2_CID_MIN_BUFFERS_FOR_OUTPUT.
>

Ah, I thought min/max should report the actual control values limits.
Anyway, if we move this to be an integer menu control with an helper
to parse the DT property and register the control on behalf of
drivers, this will change.

> > +		*step = 1;
> >  		break;
> >  	default:
> >  		*type = V4L2_CTRL_TYPE_INTEGER;
> > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > index 37807f23231e..5c4c7b245921 100644
> > --- a/include/uapi/linux/v4l2-controls.h
> > +++ b/include/uapi/linux/v4l2-controls.h
> > @@ -889,6 +889,10 @@ 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_LOCATION			(V4L2_CID_CAMERA_CLASS_BASE+34)
> > +#define V4L2_LOCATION_FRONT			(0 << 0)
> > +#define V4L2_LOCATION_BACK			(1 << 0)
>
> Why not just 0 and 1 ?

Or why not BIT(). I saw that the (1 << x) style is the mostly used one in
this header file when defining macros like this one so I went for
consistency with the existing code.

>
> > +
> >  /* FM Modulator class control IDs */
> >
> >  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
>
> --
> Regards,
>
> Laurent Pinchart

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

Re: [RFC 3/5] media: v4l2-ctrls: Add support for V4L2_CID_LOCATION

[Laurent Pinchart]

On Thu, Aug 15, 2019 at 03:02:45PM +0200, Jacopo Mondi wrote:
> Hi Laurent,
> 
> On Thu, Aug 15, 2019 at 01:53:53AM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > On Wed, Aug 14, 2019 at 10:28:13PM +0200, Jacopo Mondi wrote:
> > > Add support for the newly defined V4L2_CID_LOCATION read-only control
> > > used to report the camera device mounting position.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  drivers/media/v4l2-core/v4l2-ctrls.c | 7 +++++++
> > >  include/uapi/linux/v4l2-controls.h   | 4 ++++
> > >  2 files changed, 11 insertions(+)
> > >
> > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > > index 7d3a33258748..8ab0857df59a 100644
> > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > > @@ -943,6 +943,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> > >  	case V4L2_CID_AUTO_FOCUS_RANGE:		return "Auto Focus, Range";
> > >  	case V4L2_CID_PAN_SPEED:		return "Pan, Speed";
> > >  	case V4L2_CID_TILT_SPEED:		return "Tilt, Speed";
> > > +	case V4L2_CID_LOCATION:			return "Location";
> >
> > Depending on what we decide to name the control (see review of 2/5), you
> > should adjust the description accordingly.
> >
> > >
> > >  	/* FM Radio Modulator controls */
> > >  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> > > @@ -1300,6 +1301,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> > >  		break;
> > >  	case V4L2_CID_MPEG_VIDEO_FWHT_PARAMS:
> > >  		*type = V4L2_CTRL_TYPE_FWHT_PARAMS;
> > > +	case V4L2_CID_LOCATION:
> > > +		*type = V4L2_CTRL_TYPE_INTEGER;
> > > +		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > +		*min = V4L2_LOCATION_FRONT;
> > > +		*max = V4L2_LOCATION_BACK;
> >
> > I don't think the control should have a min and a max different than the
> > current value, as it's a fully static control. I'd drop those two lines
> > here, and drivers will have to set value = min = max = V4L2_LOCATION_xxx
> > when creating the control. That why you should be able to collapse this
> > with V4L2_CID_MIN_BUFFERS_FOR_OUTPUT.
> >
> 
> Ah, I thought min/max should report the actual control values limits.
> Anyway, if we move this to be an integer menu control with an helper
> to parse the DT property and register the control on behalf of
> drivers, this will change.
> 
> > > +		*step = 1;
> > >  		break;
> > >  	default:
> > >  		*type = V4L2_CTRL_TYPE_INTEGER;
> > > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > > index 37807f23231e..5c4c7b245921 100644
> > > --- a/include/uapi/linux/v4l2-controls.h
> > > +++ b/include/uapi/linux/v4l2-controls.h
> > > @@ -889,6 +889,10 @@ 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_LOCATION			(V4L2_CID_CAMERA_CLASS_BASE+34)
> > > +#define V4L2_LOCATION_FRONT			(0 << 0)
> > > +#define V4L2_LOCATION_BACK			(1 << 0)
> >
> > Why not just 0 and 1 ?
> 
> Or why not BIT().

No, BIT(0) == (0 << 0) but BIT(1) != (1 << 0).

> I saw that the (1 << x) style is the mostly used one in this header
> file when defining macros like this one so I went for consistency with
> the existing code.

That's when you need a bitmask, which isn't the case here.

> > > +
> > >  /* FM Modulator class control IDs */
> > >
> > >  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)

-- 
Regards,

Laurent Pinchart

Re: [RFC 2/5] media: v4l2-ctrl: Document V4L2_CID_LOCATION

[Sakari Ailus]

On Thu, Aug 15, 2019 at 03:59:38PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Thu, Aug 15, 2019 at 10:00:25AM +0300, Sakari Ailus wrote:
> > On Wed, Aug 14, 2019 at 10:28:12PM +0200, Jacopo Mondi wrote:
> > > Add documentation for the V4L2_CID_LOCATION camera control. The newly
> > > added read-only control reports the camera device mounting position.
> > > 
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  .../media/uapi/v4l/ext-ctrls-camera.rst       | 23 +++++++++++++++++++
> > >  1 file changed, 23 insertions(+)
> > > 
> > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > > index 51c1d5c9eb00..fc0a02eee6d4 100644
> > > --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > > +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > > @@ -510,6 +510,29 @@ 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_LOCATION (integer)``
> > > +    This read-only control describes the camera location by reporting its
> > > +    mounting position on the device where the camera is installed. 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 position. In example, a camera installed on the user-facing side
> > 
> > s/In/For/
> > 
> > > +    of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT``
> > > +    position.
> > > +
> > > +
> > > +
> > > +.. flat-table::
> > > +    :header-rows:  0
> > > +    :stub-columns: 0
> > > +
> > > +    * - ``V4L2_LOCATION_FRONT``
> > > +      - The camera device is located on the front side of the device.
> > > +    * - ``V4L2_LOCATION_BACK``
> > > +      - The camera device is located on the back side of the device.
> > > +
> > > +
> > > +
> > >  .. [#f1]
> > >     This control may be changed to a menu control in the future, if more
> > >     options are required.
> > 
> > There's an effective limit of 64 for menus. ACPI has less than ten
> > different locations for a device, I think 64 will be enough here.
> > 
> > So I'd be actually in favour of switching to a menu.
> 
> Why ? As you explained yourself, it's a static read-only control, all it
> needs to report is a single value.

Yes. That's true. It wasn't meant for this but it's nevertheless a
convenient API to get that information, both as integer and string.

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

Re: [RFC 2/5] media: v4l2-ctrl: Document V4L2_CID_LOCATION

[Laurent Pinchart]

Hi Sakari,

On Thu, Aug 15, 2019 at 04:08:49PM +0300, Sakari Ailus wrote:
> On Thu, Aug 15, 2019 at 03:59:38PM +0300, Laurent Pinchart wrote:
> > On Thu, Aug 15, 2019 at 10:00:25AM +0300, Sakari Ailus wrote:
> > > On Wed, Aug 14, 2019 at 10:28:12PM +0200, Jacopo Mondi wrote:
> > > > Add documentation for the V4L2_CID_LOCATION camera control. The newly
> > > > added read-only control reports the camera device mounting position.
> > > > 
> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > ---
> > > >  .../media/uapi/v4l/ext-ctrls-camera.rst       | 23 +++++++++++++++++++
> > > >  1 file changed, 23 insertions(+)
> > > > 
> > > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > > > index 51c1d5c9eb00..fc0a02eee6d4 100644
> > > > --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > > > +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > > > @@ -510,6 +510,29 @@ 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_LOCATION (integer)``
> > > > +    This read-only control describes the camera location by reporting its
> > > > +    mounting position on the device where the camera is installed. 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 position. In example, a camera installed on the user-facing side
> > > 
> > > s/In/For/
> > > 
> > > > +    of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT``
> > > > +    position.
> > > > +
> > > > +
> > > > +
> > > > +.. flat-table::
> > > > +    :header-rows:  0
> > > > +    :stub-columns: 0
> > > > +
> > > > +    * - ``V4L2_LOCATION_FRONT``
> > > > +      - The camera device is located on the front side of the device.
> > > > +    * - ``V4L2_LOCATION_BACK``
> > > > +      - The camera device is located on the back side of the device.
> > > > +
> > > > +
> > > > +
> > > >  .. [#f1]
> > > >     This control may be changed to a menu control in the future, if more
> > > >     options are required.
> > > 
> > > There's an effective limit of 64 for menus. ACPI has less than ten
> > > different locations for a device, I think 64 will be enough here.
> > > 
> > > So I'd be actually in favour of switching to a menu.
> > 
> > Why ? As you explained yourself, it's a static read-only control, all it
> > needs to report is a single value.
> 
> Yes. That's true. It wasn't meant for this but it's nevertheless a
> convenient API to get that information, both as integer and string.

But why is that needed ? The integer seems enough to me.

-- 
Regards,

Laurent Pinchart

Re: [RFC 2/5] media: v4l2-ctrl: Document V4L2_CID_LOCATION

[Sakari Ailus]

On Thu, Aug 15, 2019 at 04:10:53PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Thu, Aug 15, 2019 at 04:08:49PM +0300, Sakari Ailus wrote:
> > On Thu, Aug 15, 2019 at 03:59:38PM +0300, Laurent Pinchart wrote:
> > > On Thu, Aug 15, 2019 at 10:00:25AM +0300, Sakari Ailus wrote:
> > > > On Wed, Aug 14, 2019 at 10:28:12PM +0200, Jacopo Mondi wrote:
> > > > > Add documentation for the V4L2_CID_LOCATION camera control. The newly
> > > > > added read-only control reports the camera device mounting position.
> > > > > 
> > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > > ---
> > > > >  .../media/uapi/v4l/ext-ctrls-camera.rst       | 23 +++++++++++++++++++
> > > > >  1 file changed, 23 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > > > > index 51c1d5c9eb00..fc0a02eee6d4 100644
> > > > > --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > > > > +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > > > > @@ -510,6 +510,29 @@ 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_LOCATION (integer)``
> > > > > +    This read-only control describes the camera location by reporting its
> > > > > +    mounting position on the device where the camera is installed. 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 position. In example, a camera installed on the user-facing side
> > > > 
> > > > s/In/For/
> > > > 
> > > > > +    of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT``
> > > > > +    position.
> > > > > +
> > > > > +
> > > > > +
> > > > > +.. flat-table::
> > > > > +    :header-rows:  0
> > > > > +    :stub-columns: 0
> > > > > +
> > > > > +    * - ``V4L2_LOCATION_FRONT``
> > > > > +      - The camera device is located on the front side of the device.
> > > > > +    * - ``V4L2_LOCATION_BACK``
> > > > > +      - The camera device is located on the back side of the device.
> > > > > +
> > > > > +
> > > > > +
> > > > >  .. [#f1]
> > > > >     This control may be changed to a menu control in the future, if more
> > > > >     options are required.
> > > > 
> > > > There's an effective limit of 64 for menus. ACPI has less than ten
> > > > different locations for a device, I think 64 will be enough here.
> > > > 
> > > > So I'd be actually in favour of switching to a menu.
> > > 
> > > Why ? As you explained yourself, it's a static read-only control, all it
> > > needs to report is a single value.
> > 
> > Yes. That's true. It wasn't meant for this but it's nevertheless a
> > convenient API to get that information, both as integer and string.
> 
> But why is that needed ? The integer seems enough to me.

Because it's a qualitative control, not a quantitative one.

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

Re: [RFC 2/5] media: v4l2-ctrl: Document V4L2_CID_LOCATION

[Laurent Pinchart]

On Thu, Aug 15, 2019 at 04:15:10PM +0300, Sakari Ailus wrote:
> On Thu, Aug 15, 2019 at 04:10:53PM +0300, Laurent Pinchart wrote:
> > On Thu, Aug 15, 2019 at 04:08:49PM +0300, Sakari Ailus wrote:
> >> On Thu, Aug 15, 2019 at 03:59:38PM +0300, Laurent Pinchart wrote:
> >>> On Thu, Aug 15, 2019 at 10:00:25AM +0300, Sakari Ailus wrote:
> >>>> On Wed, Aug 14, 2019 at 10:28:12PM +0200, Jacopo Mondi wrote:
> >>>>> Add documentation for the V4L2_CID_LOCATION camera control. The newly
> >>>>> added read-only control reports the camera device mounting position.
> >>>>> 
> >>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>>> ---
> >>>>>  .../media/uapi/v4l/ext-ctrls-camera.rst       | 23 +++++++++++++++++++
> >>>>>  1 file changed, 23 insertions(+)
> >>>>> 
> >>>>> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> >>>>> index 51c1d5c9eb00..fc0a02eee6d4 100644
> >>>>> --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> >>>>> +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> >>>>> @@ -510,6 +510,29 @@ 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_LOCATION (integer)``
> >>>>> +    This read-only control describes the camera location by reporting its
> >>>>> +    mounting position on the device where the camera is installed. 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 position. In example, a camera installed on the user-facing side
> >>>> 
> >>>> s/In/For/
> >>>> 
> >>>>> +    of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT``
> >>>>> +    position.
> >>>>> +
> >>>>> +
> >>>>> +
> >>>>> +.. flat-table::
> >>>>> +    :header-rows:  0
> >>>>> +    :stub-columns: 0
> >>>>> +
> >>>>> +    * - ``V4L2_LOCATION_FRONT``
> >>>>> +      - The camera device is located on the front side of the device.
> >>>>> +    * - ``V4L2_LOCATION_BACK``
> >>>>> +      - The camera device is located on the back side of the device.
> >>>>> +
> >>>>> +
> >>>>> +
> >>>>>  .. [#f1]
> >>>>>     This control may be changed to a menu control in the future, if more
> >>>>>     options are required.
> >>>> 
> >>>> There's an effective limit of 64 for menus. ACPI has less than ten
> >>>> different locations for a device, I think 64 will be enough here.
> >>>> 
> >>>> So I'd be actually in favour of switching to a menu.
> >>> 
> >>> Why ? As you explained yourself, it's a static read-only control, all it
> >>> needs to report is a single value.
> >> 
> >> Yes. That's true. It wasn't meant for this but it's nevertheless a
> >> convenient API to get that information, both as integer and string.
> > 
> > But why is that needed ? The integer seems enough to me.
> 
> Because it's a qualitative control, not a quantitative one.

And ? :-) The integer values are defined in the V4L2 spec, they map to a
usage, and a name can easily be derived from that in userspace if
desired.

-- 
Regards,

Laurent Pinchart

Re: [RFC 3/5] media: v4l2-ctrls: Add support for V4L2_CID_LOCATION

[Hans Verkuil]

On 8/14/19 10:28 PM, Jacopo Mondi wrote:
> Add support for the newly defined V4L2_CID_LOCATION read-only control
> used to report the camera device mounting position.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/v4l2-core/v4l2-ctrls.c | 7 +++++++
>  include/uapi/linux/v4l2-controls.h   | 4 ++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 7d3a33258748..8ab0857df59a 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -943,6 +943,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_AUTO_FOCUS_RANGE:		return "Auto Focus, Range";
>  	case V4L2_CID_PAN_SPEED:		return "Pan, Speed";
>  	case V4L2_CID_TILT_SPEED:		return "Tilt, Speed";
> +	case V4L2_CID_LOCATION:			return "Location";
>  
>  	/* FM Radio Modulator controls */
>  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> @@ -1300,6 +1301,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  		break;
>  	case V4L2_CID_MPEG_VIDEO_FWHT_PARAMS:
>  		*type = V4L2_CTRL_TYPE_FWHT_PARAMS;

Missing break!

Regards,

	Hans

> +	case V4L2_CID_LOCATION:
> +		*type = V4L2_CTRL_TYPE_INTEGER;
> +		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +		*min = V4L2_LOCATION_FRONT;
> +		*max = V4L2_LOCATION_BACK;
> +		*step = 1;
>  		break;
>  	default:
>  		*type = V4L2_CTRL_TYPE_INTEGER;
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 37807f23231e..5c4c7b245921 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -889,6 +889,10 @@ 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_LOCATION			(V4L2_CID_CAMERA_CLASS_BASE+34)
> +#define V4L2_LOCATION_FRONT			(0 << 0)
> +#define V4L2_LOCATION_BACK			(1 << 0)
> +
>  /* FM Modulator class control IDs */
>  
>  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
> 

Re: [RFC 2/5] media: v4l2-ctrl: Document V4L2_CID_LOCATION

[Hans Verkuil]

On 8/14/19 10:28 PM, Jacopo Mondi wrote:
> Add documentation for the V4L2_CID_LOCATION camera control. The newly
> added read-only control reports the camera device mounting position.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  .../media/uapi/v4l/ext-ctrls-camera.rst       | 23 +++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> index 51c1d5c9eb00..fc0a02eee6d4 100644
> --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> @@ -510,6 +510,29 @@ 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_LOCATION (integer)``
> +    This read-only control describes the camera location by reporting its
> +    mounting position on the device where the camera is installed. 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 position. In example, a camera installed on the user-facing side
> +    of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT``
> +    position.

When should this control be created? If there is only one location (e.g.
all sensors are front-facing) would you still expose this? Or does it depend
on the type of device?

And is the sensor in a digital camera front or back facing? (Just curious
about what you think about that situation!)

Regards,

	Hans

> +
> +
> +
> +.. flat-table::
> +    :header-rows:  0
> +    :stub-columns: 0
> +
> +    * - ``V4L2_LOCATION_FRONT``
> +      - The camera device is located on the front side of the device.
> +    * - ``V4L2_LOCATION_BACK``
> +      - The camera device is located on the back side of the device.
> +
> +
> +
>  .. [#f1]
>     This control may be changed to a menu control in the future, if more
>     options are required.
> --
> 2.22.0
> 

Re: [RFC 3/5] media: v4l2-ctrls: Add support for V4L2_CID_LOCATION

[Hans Verkuil]

On 8/15/19 3:02 PM, Jacopo Mondi wrote:
> Hi Laurent,
> 
> On Thu, Aug 15, 2019 at 01:53:53AM +0300, Laurent Pinchart wrote:
>> Hi Jacopo,
>>
>> Thank you for the patch.
>>
>> On Wed, Aug 14, 2019 at 10:28:13PM +0200, Jacopo Mondi wrote:
>>> Add support for the newly defined V4L2_CID_LOCATION read-only control
>>> used to report the camera device mounting position.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>> ---
>>>  drivers/media/v4l2-core/v4l2-ctrls.c | 7 +++++++
>>>  include/uapi/linux/v4l2-controls.h   | 4 ++++
>>>  2 files changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>>> index 7d3a33258748..8ab0857df59a 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>>> @@ -943,6 +943,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>>  	case V4L2_CID_AUTO_FOCUS_RANGE:		return "Auto Focus, Range";
>>>  	case V4L2_CID_PAN_SPEED:		return "Pan, Speed";
>>>  	case V4L2_CID_TILT_SPEED:		return "Tilt, Speed";
>>> +	case V4L2_CID_LOCATION:			return "Location";
>>
>> Depending on what we decide to name the control (see review of 2/5), you
>> should adjust the description accordingly.
>>
>>>
>>>  	/* FM Radio Modulator controls */
>>>  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
>>> @@ -1300,6 +1301,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>>>  		break;
>>>  	case V4L2_CID_MPEG_VIDEO_FWHT_PARAMS:
>>>  		*type = V4L2_CTRL_TYPE_FWHT_PARAMS;
>>> +	case V4L2_CID_LOCATION:
>>> +		*type = V4L2_CTRL_TYPE_INTEGER;
>>> +		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
>>> +		*min = V4L2_LOCATION_FRONT;
>>> +		*max = V4L2_LOCATION_BACK;
>>
>> I don't think the control should have a min and a max different than the
>> current value, as it's a fully static control. I'd drop those two lines
>> here, and drivers will have to set value = min = max = V4L2_LOCATION_xxx
>> when creating the control. That why you should be able to collapse this
>> with V4L2_CID_MIN_BUFFERS_FOR_OUTPUT.
>>
> 
> Ah, I thought min/max should report the actual control values limits.
> Anyway, if we move this to be an integer menu control with an helper
> to parse the DT property and register the control on behalf of
> drivers, this will change.
> 
>>> +		*step = 1;
>>>  		break;
>>>  	default:
>>>  		*type = V4L2_CTRL_TYPE_INTEGER;
>>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>>> index 37807f23231e..5c4c7b245921 100644
>>> --- a/include/uapi/linux/v4l2-controls.h
>>> +++ b/include/uapi/linux/v4l2-controls.h
>>> @@ -889,6 +889,10 @@ 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_LOCATION			(V4L2_CID_CAMERA_CLASS_BASE+34)
>>> +#define V4L2_LOCATION_FRONT			(0 << 0)
>>> +#define V4L2_LOCATION_BACK			(1 << 0)
>>
>> Why not just 0 and 1 ?
> 
> Or why not BIT(). I saw that the (1 << x) style is the mostly used one in
> this header file when defining macros like this one so I went for
> consistency with the existing code.

Definitely not right. This is an enumeration, so just number from 0, 1, 2, ...

Nothing to do with bits/bitmasks.

Regards,

	Hans

> 
>>
>>> +
>>>  /* FM Modulator class control IDs */
>>>
>>>  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
>>
>> --
>> Regards,
>>
>> Laurent Pinchart

Re: [RFC 2/5] media: v4l2-ctrl: Document V4L2_CID_LOCATION

[Laurent Pinchart]

Hi Hans,

On Thu, Aug 15, 2019 at 03:30:59PM +0200, Hans Verkuil wrote:
> On 8/14/19 10:28 PM, Jacopo Mondi wrote:
> > Add documentation for the V4L2_CID_LOCATION camera control. The newly
> > added read-only control reports the camera device mounting position.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  .../media/uapi/v4l/ext-ctrls-camera.rst       | 23 +++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > index 51c1d5c9eb00..fc0a02eee6d4 100644
> > --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > @@ -510,6 +510,29 @@ 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_LOCATION (integer)``
> > +    This read-only control describes the camera location by reporting its
> > +    mounting position on the device where the camera is installed. 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 position. In example, a camera installed on the user-facing side
> > +    of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT``
> > +    position.
> 
> When should this control be created? If there is only one location (e.g.
> all sensors are front-facing) would you still expose this? Or does it depend
> on the type of device?

Those are important questions that need to be answered :-) Going forward
I think all camera sensors should expose this, and I'd like a helper
function to create the control and set its value based on firmware
properties that all (or most) camera sensor drivers should use. That
helper function should also create the other mandatory or optional
standard controls for camera sensors, such as the pixel rate or link
frequency controls.

> And is the sensor in a digital camera front or back facing? (Just curious
> about what you think about that situation!)

I think we should include here a list of supported device types, and for
each device type, define what the front location is. All other locations
are then derived from that. For a digital camera I would define the
front side as facing the scene being photographed.

> > +
> > +
> > +
> > +.. flat-table::
> > +    :header-rows:  0
> > +    :stub-columns: 0
> > +
> > +    * - ``V4L2_LOCATION_FRONT``
> > +      - The camera device is located on the front side of the device.
> > +    * - ``V4L2_LOCATION_BACK``
> > +      - The camera device is located on the back side of the device.
> > +
> > +
> > +
> >  .. [#f1]
> >     This control may be changed to a menu control in the future, if more
> >     options are required.

-- 
Regards,

Laurent Pinchart

Re: [RFC 3/5] media: v4l2-ctrls: Add support for V4L2_CID_LOCATION

[Jacopo Mondi]

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

Hi Hans,

On Thu, Aug 15, 2019 at 03:41:53PM +0200, Hans Verkuil wrote:
> On 8/15/19 3:02 PM, Jacopo Mondi wrote:
> > Hi Laurent,
> >
> > On Thu, Aug 15, 2019 at 01:53:53AM +0300, Laurent Pinchart wrote:
> >> Hi Jacopo,
> >>
> >> Thank you for the patch.
> >>
> >> On Wed, Aug 14, 2019 at 10:28:13PM +0200, Jacopo Mondi wrote:
> >>> Add support for the newly defined V4L2_CID_LOCATION read-only control
> >>> used to report the camera device mounting position.
> >>>
> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>> ---
> >>>  drivers/media/v4l2-core/v4l2-ctrls.c | 7 +++++++
> >>>  include/uapi/linux/v4l2-controls.h   | 4 ++++
> >>>  2 files changed, 11 insertions(+)
> >>>
> >>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> >>> index 7d3a33258748..8ab0857df59a 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> >>> @@ -943,6 +943,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> >>>  	case V4L2_CID_AUTO_FOCUS_RANGE:		return "Auto Focus, Range";
> >>>  	case V4L2_CID_PAN_SPEED:		return "Pan, Speed";
> >>>  	case V4L2_CID_TILT_SPEED:		return "Tilt, Speed";
> >>> +	case V4L2_CID_LOCATION:			return "Location";
> >>
> >> Depending on what we decide to name the control (see review of 2/5), you
> >> should adjust the description accordingly.
> >>
> >>>
> >>>  	/* FM Radio Modulator controls */
> >>>  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> >>> @@ -1300,6 +1301,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> >>>  		break;
> >>>  	case V4L2_CID_MPEG_VIDEO_FWHT_PARAMS:
> >>>  		*type = V4L2_CTRL_TYPE_FWHT_PARAMS;
> >>> +	case V4L2_CID_LOCATION:
> >>> +		*type = V4L2_CTRL_TYPE_INTEGER;
> >>> +		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
> >>> +		*min = V4L2_LOCATION_FRONT;
> >>> +		*max = V4L2_LOCATION_BACK;
> >>
> >> I don't think the control should have a min and a max different than the
> >> current value, as it's a fully static control. I'd drop those two lines
> >> here, and drivers will have to set value = min = max = V4L2_LOCATION_xxx
> >> when creating the control. That why you should be able to collapse this
> >> with V4L2_CID_MIN_BUFFERS_FOR_OUTPUT.
> >>
> >
> > Ah, I thought min/max should report the actual control values limits.
> > Anyway, if we move this to be an integer menu control with an helper
> > to parse the DT property and register the control on behalf of
> > drivers, this will change.
> >
> >>> +		*step = 1;
> >>>  		break;
> >>>  	default:
> >>>  		*type = V4L2_CTRL_TYPE_INTEGER;
> >>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> >>> index 37807f23231e..5c4c7b245921 100644
> >>> --- a/include/uapi/linux/v4l2-controls.h
> >>> +++ b/include/uapi/linux/v4l2-controls.h
> >>> @@ -889,6 +889,10 @@ 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_LOCATION			(V4L2_CID_CAMERA_CLASS_BASE+34)
> >>> +#define V4L2_LOCATION_FRONT			(0 << 0)
> >>> +#define V4L2_LOCATION_BACK			(1 << 0)
> >>
> >> Why not just 0 and 1 ?
> >
> > Or why not BIT(). I saw that the (1 << x) style is the mostly used one in
> > this header file when defining macros like this one so I went for
> > consistency with the existing code.
>
> Definitely not right. This is an enumeration, so just number from 0, 1, 2, ...
>
> Nothing to do with bits/bitmasks.

Aren't these enumerations too?

#define V4L2_CID_3A_LOCK			(V4L2_CID_CAMERA_CLASS_BASE+27)
#define V4L2_LOCK_EXPOSURE			(1 << 0)
#define V4L2_LOCK_WHITE_BALANCE			(1 << 1)
#define V4L2_LOCK_FOCUS				(1 << 2)

#define V4L2_CID_AUTO_FOCUS_START		(V4L2_CID_CAMERA_CLASS_BASE+28)
#define V4L2_CID_AUTO_FOCUS_STOP		(V4L2_CID_CAMERA_CLASS_BASE+29)
#define V4L2_CID_AUTO_FOCUS_STATUS		(V4L2_CID_CAMERA_CLASS_BASE+30)
#define V4L2_AUTO_FOCUS_STATUS_IDLE		(0 << 0)
#define V4L2_AUTO_FOCUS_STATUS_BUSY		(1 << 0)
#define V4L2_AUTO_FOCUS_STATUS_REACHED		(1 << 1)
#define V4L2_AUTO_FOCUS_STATUS_FAILED		(1 << 2)

Anyway, I'm happy to change them to plain numbers.

>
> Regards,
>
> 	Hans
>
> >
> >>
> >>> +
> >>>  /* FM Modulator class control IDs */
> >>>
> >>>  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
> >>
> >> --
> >> Regards,
> >>
> >> Laurent Pinchart
>

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

Re: [RFC 3/5] media: v4l2-ctrls: Add support for V4L2_CID_LOCATION

[Jacopo Mondi]

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

Hi Hans,

On Thu, Aug 15, 2019 at 03:23:47PM +0200, Hans Verkuil wrote:
> On 8/14/19 10:28 PM, Jacopo Mondi wrote:
> > Add support for the newly defined V4L2_CID_LOCATION read-only control
> > used to report the camera device mounting position.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  drivers/media/v4l2-core/v4l2-ctrls.c | 7 +++++++
> >  include/uapi/linux/v4l2-controls.h   | 4 ++++
> >  2 files changed, 11 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > index 7d3a33258748..8ab0857df59a 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > @@ -943,6 +943,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> >  	case V4L2_CID_AUTO_FOCUS_RANGE:		return "Auto Focus, Range";
> >  	case V4L2_CID_PAN_SPEED:		return "Pan, Speed";
> >  	case V4L2_CID_TILT_SPEED:		return "Tilt, Speed";
> > +	case V4L2_CID_LOCATION:			return "Location";
> >
> >  	/* FM Radio Modulator controls */
> >  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> > @@ -1300,6 +1301,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> >  		break;
> >  	case V4L2_CID_MPEG_VIDEO_FWHT_PARAMS:
> >  		*type = V4L2_CTRL_TYPE_FWHT_PARAMS;
>
> Missing break!
>

Sorry, this was a trivial mistake. Thanks for spotting!

> Regards,
>
> 	Hans
>
> > +	case V4L2_CID_LOCATION:
> > +		*type = V4L2_CTRL_TYPE_INTEGER;
> > +		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +		*min = V4L2_LOCATION_FRONT;
> > +		*max = V4L2_LOCATION_BACK;
> > +		*step = 1;
> >  		break;
> >  	default:
> >  		*type = V4L2_CTRL_TYPE_INTEGER;
> > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > index 37807f23231e..5c4c7b245921 100644
> > --- a/include/uapi/linux/v4l2-controls.h
> > +++ b/include/uapi/linux/v4l2-controls.h
> > @@ -889,6 +889,10 @@ 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_LOCATION			(V4L2_CID_CAMERA_CLASS_BASE+34)
> > +#define V4L2_LOCATION_FRONT			(0 << 0)
> > +#define V4L2_LOCATION_BACK			(1 << 0)
> > +
> >  /* FM Modulator class control IDs */
> >
> >  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
> >
>

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

Re: [RFC 2/5] media: v4l2-ctrl: Document V4L2_CID_LOCATION

[Jacopo Mondi]

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

Hi Hans,

On Thu, Aug 15, 2019 at 03:30:59PM +0200, Hans Verkuil wrote:
> On 8/14/19 10:28 PM, Jacopo Mondi wrote:
> > Add documentation for the V4L2_CID_LOCATION camera control. The newly
> > added read-only control reports the camera device mounting position.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  .../media/uapi/v4l/ext-ctrls-camera.rst       | 23 +++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >
> > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > index 51c1d5c9eb00..fc0a02eee6d4 100644
> > --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > @@ -510,6 +510,29 @@ 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_LOCATION (integer)``
> > +    This read-only control describes the camera location by reporting its
> > +    mounting position on the device where the camera is installed. 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 position. In example, a camera installed on the user-facing side
> > +    of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT``
> > +    position.
>
> When should this control be created? If there is only one location (e.g.
> all sensors are front-facing) would you still expose this? Or does it depend
> on the type of device?

If it's meaningful for the device, the location might be reported even
if there's only a single camera in the system.

>
> And is the sensor in a digital camera front or back facing? (Just curious
> about what you think about that situation!)

I would say it really depends on the device type. For a digital camera
like a webcam, defining what's front or back doesn't add much value.
Wherever the camera sensor is oriented to, that's the front :)

The same way, image sensor connected through long cables to the
remotely located base board (I'm thinking about cameras installed in
cars and connected by coax cables) will hardly have a position
defined in the mainline board DTS file, but if someone would like to add
"rearview-mirror" to the list of position and use them in their DTS
for whatever reason, this control gives a way to retrieve the
information easily.

I tried to convey this mentioning the "intended usage orientation" of
the device, to give the idea that the position is totally dependent
on the nature of the device the sensor is installed on. As said, it's
easy to define what "front" is for a smartphone, not so easy for a
camera in a car. But I would not tie themselves to device specific
detail, but instead focus on providing a meachanism to make easy to
expose them. In mainline, we could start with very simple "back" and
"front" position, and then grow them when the need arises.

>
> Regards,
>
> 	Hans
>
> > +
> > +
> > +
> > +.. flat-table::
> > +    :header-rows:  0
> > +    :stub-columns: 0
> > +
> > +    * - ``V4L2_LOCATION_FRONT``
> > +      - The camera device is located on the front side of the device.
> > +    * - ``V4L2_LOCATION_BACK``
> > +      - The camera device is located on the back side of the device.
> > +
> > +
> > +
> >  .. [#f1]
> >     This control may be changed to a menu control in the future, if more
> >     options are required.
> > --
> > 2.22.0
> >
>

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

Re: [RFC 2/5] media: v4l2-ctrl: Document V4L2_CID_LOCATION

[Hans Verkuil]

On 8/15/19 12:43 AM, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Wed, Aug 14, 2019 at 10:28:12PM +0200, Jacopo Mondi wrote:
>> Add documentation for the V4L2_CID_LOCATION camera control. The newly
>> added read-only control reports the camera device mounting position.
>>
>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>> ---
>>  .../media/uapi/v4l/ext-ctrls-camera.rst       | 23 +++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>>
>> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
>> index 51c1d5c9eb00..fc0a02eee6d4 100644
>> --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
>> +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
>> @@ -510,6 +510,29 @@ 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_LOCATION (integer)``
> 
> Maybe V4L2_CID_CAMERA_SENSOR_LOCATION ? Same for the values below.

Probably a better name, if a bit long. But we might need other location
controls in the future (e.g. flash location), so CID_LOCATION is just too
generic.

Regards,

	Hans

> 
>> +    This read-only control describes the camera location by reporting its
> 
> Here too I would mention camera sensor instead of just camera (or
> possibly imaging sensor).
> 
>> +    mounting position on the device where the camera is installed. 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 position. In example, a camera installed on the user-facing side
>> +    of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT``
>> +    position.
> 
> The DT bindings could use such an example :-) I would extend this to
> tablets and laptops.
> 
>> +
>> +
>> +
> 
> Do we need three blank lines ?
> 
>> +.. flat-table::
>> +    :header-rows:  0
>> +    :stub-columns: 0
>> +
>> +    * - ``V4L2_LOCATION_FRONT``
>> +      - The camera device is located on the front side of the device.
>> +    * - ``V4L2_LOCATION_BACK``
>> +      - The camera device is located on the back side of the device.
>> +
>> +
>> +
>>  .. [#f1]
>>     This control may be changed to a menu control in the future, if more
>>     options are required.
> 

Re: [RFC 3/5] media: v4l2-ctrls: Add support for V4L2_CID_LOCATION

[Hans Verkuil]

On 8/15/19 3:50 PM, Jacopo Mondi wrote:
> Hi Hans,
> 
> On Thu, Aug 15, 2019 at 03:41:53PM +0200, Hans Verkuil wrote:
>> On 8/15/19 3:02 PM, Jacopo Mondi wrote:
>>> Hi Laurent,
>>>
>>> On Thu, Aug 15, 2019 at 01:53:53AM +0300, Laurent Pinchart wrote:
>>>> Hi Jacopo,
>>>>
>>>> Thank you for the patch.
>>>>
>>>> On Wed, Aug 14, 2019 at 10:28:13PM +0200, Jacopo Mondi wrote:
>>>>> Add support for the newly defined V4L2_CID_LOCATION read-only control
>>>>> used to report the camera device mounting position.
>>>>>
>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>>>> ---
>>>>>  drivers/media/v4l2-core/v4l2-ctrls.c | 7 +++++++
>>>>>  include/uapi/linux/v4l2-controls.h   | 4 ++++
>>>>>  2 files changed, 11 insertions(+)
>>>>>
>>>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>>>>> index 7d3a33258748..8ab0857df59a 100644
>>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>>>>> @@ -943,6 +943,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>>>>  	case V4L2_CID_AUTO_FOCUS_RANGE:		return "Auto Focus, Range";
>>>>>  	case V4L2_CID_PAN_SPEED:		return "Pan, Speed";
>>>>>  	case V4L2_CID_TILT_SPEED:		return "Tilt, Speed";
>>>>> +	case V4L2_CID_LOCATION:			return "Location";
>>>>
>>>> Depending on what we decide to name the control (see review of 2/5), you
>>>> should adjust the description accordingly.
>>>>
>>>>>
>>>>>  	/* FM Radio Modulator controls */
>>>>>  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
>>>>> @@ -1300,6 +1301,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>>>>>  		break;
>>>>>  	case V4L2_CID_MPEG_VIDEO_FWHT_PARAMS:
>>>>>  		*type = V4L2_CTRL_TYPE_FWHT_PARAMS;
>>>>> +	case V4L2_CID_LOCATION:
>>>>> +		*type = V4L2_CTRL_TYPE_INTEGER;
>>>>> +		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
>>>>> +		*min = V4L2_LOCATION_FRONT;
>>>>> +		*max = V4L2_LOCATION_BACK;
>>>>
>>>> I don't think the control should have a min and a max different than the
>>>> current value, as it's a fully static control. I'd drop those two lines
>>>> here, and drivers will have to set value = min = max = V4L2_LOCATION_xxx
>>>> when creating the control. That why you should be able to collapse this
>>>> with V4L2_CID_MIN_BUFFERS_FOR_OUTPUT.
>>>>
>>>
>>> Ah, I thought min/max should report the actual control values limits.
>>> Anyway, if we move this to be an integer menu control with an helper
>>> to parse the DT property and register the control on behalf of
>>> drivers, this will change.
>>>
>>>>> +		*step = 1;
>>>>>  		break;
>>>>>  	default:
>>>>>  		*type = V4L2_CTRL_TYPE_INTEGER;
>>>>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>>>>> index 37807f23231e..5c4c7b245921 100644
>>>>> --- a/include/uapi/linux/v4l2-controls.h
>>>>> +++ b/include/uapi/linux/v4l2-controls.h
>>>>> @@ -889,6 +889,10 @@ 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_LOCATION			(V4L2_CID_CAMERA_CLASS_BASE+34)
>>>>> +#define V4L2_LOCATION_FRONT			(0 << 0)
>>>>> +#define V4L2_LOCATION_BACK			(1 << 0)
>>>>
>>>> Why not just 0 and 1 ?
>>>
>>> Or why not BIT(). I saw that the (1 << x) style is the mostly used one in
>>> this header file when defining macros like this one so I went for
>>> consistency with the existing code.
>>
>> Definitely not right. This is an enumeration, so just number from 0, 1, 2, ...
>>
>> Nothing to do with bits/bitmasks.
> 
> Aren't these enumerations too?
> 
> #define V4L2_CID_3A_LOCK			(V4L2_CID_CAMERA_CLASS_BASE+27)
> #define V4L2_LOCK_EXPOSURE			(1 << 0)
> #define V4L2_LOCK_WHITE_BALANCE			(1 << 1)
> #define V4L2_LOCK_FOCUS				(1 << 2)
> 
> #define V4L2_CID_AUTO_FOCUS_START		(V4L2_CID_CAMERA_CLASS_BASE+28)
> #define V4L2_CID_AUTO_FOCUS_STOP		(V4L2_CID_CAMERA_CLASS_BASE+29)
> #define V4L2_CID_AUTO_FOCUS_STATUS		(V4L2_CID_CAMERA_CLASS_BASE+30)
> #define V4L2_AUTO_FOCUS_STATUS_IDLE		(0 << 0)
> #define V4L2_AUTO_FOCUS_STATUS_BUSY		(1 << 0)
> #define V4L2_AUTO_FOCUS_STATUS_REACHED		(1 << 1)
> #define V4L2_AUTO_FOCUS_STATUS_FAILED		(1 << 2)

No, these are bitmasks for bitmask controls. So one or more
status/lock bits can be 1.

Regards,

	Hans

> 
> Anyway, I'm happy to change them to plain numbers.
> 
>>
>> Regards,
>>
>> 	Hans
>>
>>>
>>>>
>>>>> +
>>>>>  /* FM Modulator class control IDs */
>>>>>
>>>>>  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
>>>>
>>>> --
>>>> Regards,
>>>>
>>>> Laurent Pinchart
>>

Re: [RFC 2/5] media: v4l2-ctrl: Document V4L2_CID_LOCATION

[Hans Verkuil]

On 8/15/19 4:10 PM, Hans Verkuil wrote:
> On 8/15/19 12:43 AM, Laurent Pinchart wrote:
>> Hi Jacopo,
>>
>> Thank you for the patch.
>>
>> On Wed, Aug 14, 2019 at 10:28:12PM +0200, Jacopo Mondi wrote:
>>> Add documentation for the V4L2_CID_LOCATION camera control. The newly
>>> added read-only control reports the camera device mounting position.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>> ---
>>>  .../media/uapi/v4l/ext-ctrls-camera.rst       | 23 +++++++++++++++++++
>>>  1 file changed, 23 insertions(+)
>>>
>>> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
>>> index 51c1d5c9eb00..fc0a02eee6d4 100644
>>> --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
>>> +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
>>> @@ -510,6 +510,29 @@ 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_LOCATION (integer)``
>>
>> Maybe V4L2_CID_CAMERA_SENSOR_LOCATION ? Same for the values below.
> 
> Probably a better name, if a bit long. But we might need other location
> controls in the future (e.g. flash location), so CID_LOCATION is just too
> generic.

Note that the location defines themselves can most likely be used with any
LOCATION control, so V4L2_LOCATION_FRONT would be fine with any control.

Regards,

	Hans

> 
> Regards,
> 
> 	Hans
> 
>>
>>> +    This read-only control describes the camera location by reporting its
>>
>> Here too I would mention camera sensor instead of just camera (or
>> possibly imaging sensor).
>>
>>> +    mounting position on the device where the camera is installed. 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 position. In example, a camera installed on the user-facing side
>>> +    of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT``
>>> +    position.
>>
>> The DT bindings could use such an example :-) I would extend this to
>> tablets and laptops.
>>
>>> +
>>> +
>>> +
>>
>> Do we need three blank lines ?
>>
>>> +.. flat-table::
>>> +    :header-rows:  0
>>> +    :stub-columns: 0
>>> +
>>> +    * - ``V4L2_LOCATION_FRONT``
>>> +      - The camera device is located on the front side of the device.
>>> +    * - ``V4L2_LOCATION_BACK``
>>> +      - The camera device is located on the back side of the device.
>>> +
>>> +
>>> +
>>>  .. [#f1]
>>>     This control may be changed to a menu control in the future, if more
>>>     options are required.
>>
> 

Re: [RFC 2/5] media: v4l2-ctrl: Document V4L2_CID_LOCATION

[Jacopo Mondi]

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

Hi Hans,

On Thu, Aug 15, 2019 at 04:14:38PM +0200, Hans Verkuil wrote:
> On 8/15/19 4:10 PM, Hans Verkuil wrote:
> > On 8/15/19 12:43 AM, Laurent Pinchart wrote:
> >> Hi Jacopo,
> >>
> >> Thank you for the patch.
> >>
> >> On Wed, Aug 14, 2019 at 10:28:12PM +0200, Jacopo Mondi wrote:
> >>> Add documentation for the V4L2_CID_LOCATION camera control. The newly
> >>> added read-only control reports the camera device mounting position.
> >>>
> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>> ---
> >>>  .../media/uapi/v4l/ext-ctrls-camera.rst       | 23 +++++++++++++++++++
> >>>  1 file changed, 23 insertions(+)
> >>>
> >>> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> >>> index 51c1d5c9eb00..fc0a02eee6d4 100644
> >>> --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> >>> +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> >>> @@ -510,6 +510,29 @@ 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_LOCATION (integer)``
> >>
> >> Maybe V4L2_CID_CAMERA_SENSOR_LOCATION ? Same for the values below.
> >
> > Probably a better name, if a bit long. But we might need other location
> > controls in the future (e.g. flash location), so CID_LOCATION is just too
> > generic.
>

Thanks for the feedback.

> Note that the location defines themselves can most likely be used with any
> LOCATION control, so V4L2_LOCATION_FRONT would be fine with any control.
>

What do you think instead of the control type? Would a single integer
control do or an integer menu one would be better? I see merit in both
proposals actually...

Once this is clarified, I can send a proper v1.

Thanks
  j

> Regards,
>
> 	Hans
>
> >
> > Regards,
> >
> > 	Hans
> >
> >>
> >>> +    This read-only control describes the camera location by reporting its
> >>
> >> Here too I would mention camera sensor instead of just camera (or
> >> possibly imaging sensor).
> >>
> >>> +    mounting position on the device where the camera is installed. 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 position. In example, a camera installed on the user-facing side
> >>> +    of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT``
> >>> +    position.
> >>
> >> The DT bindings could use such an example :-) I would extend this to
> >> tablets and laptops.
> >>
> >>> +
> >>> +
> >>> +
> >>
> >> Do we need three blank lines ?
> >>
> >>> +.. flat-table::
> >>> +    :header-rows:  0
> >>> +    :stub-columns: 0
> >>> +
> >>> +    * - ``V4L2_LOCATION_FRONT``
> >>> +      - The camera device is located on the front side of the device.
> >>> +    * - ``V4L2_LOCATION_BACK``
> >>> +      - The camera device is located on the back side of the device.
> >>> +
> >>> +
> >>> +
> >>>  .. [#f1]
> >>>     This control may be changed to a menu control in the future, if more
> >>>     options are required.
> >>
> >
>

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

Re: [RFC 2/5] media: v4l2-ctrl: Document V4L2_CID_LOCATION

[Hans Verkuil]

On 8/15/19 4:34 PM, Jacopo Mondi wrote:
> Hi Hans,
> 
> On Thu, Aug 15, 2019 at 04:14:38PM +0200, Hans Verkuil wrote:
>> On 8/15/19 4:10 PM, Hans Verkuil wrote:
>>> On 8/15/19 12:43 AM, Laurent Pinchart wrote:
>>>> Hi Jacopo,
>>>>
>>>> Thank you for the patch.
>>>>
>>>> On Wed, Aug 14, 2019 at 10:28:12PM +0200, Jacopo Mondi wrote:
>>>>> Add documentation for the V4L2_CID_LOCATION camera control. The newly
>>>>> added read-only control reports the camera device mounting position.
>>>>>
>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>>>> ---
>>>>>  .../media/uapi/v4l/ext-ctrls-camera.rst       | 23 +++++++++++++++++++
>>>>>  1 file changed, 23 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
>>>>> index 51c1d5c9eb00..fc0a02eee6d4 100644
>>>>> --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
>>>>> +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
>>>>> @@ -510,6 +510,29 @@ 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_LOCATION (integer)``
>>>>
>>>> Maybe V4L2_CID_CAMERA_SENSOR_LOCATION ? Same for the values below.
>>>
>>> Probably a better name, if a bit long. But we might need other location
>>> controls in the future (e.g. flash location), so CID_LOCATION is just too
>>> generic.
>>
> 
> Thanks for the feedback.
> 
>> Note that the location defines themselves can most likely be used with any
>> LOCATION control, so V4L2_LOCATION_FRONT would be fine with any control.
>>
> 
> What do you think instead of the control type? Would a single integer
> control do or an integer menu one would be better? I see merit in both
> proposals actually...

Single integer. It's read-only, so it just reports the location.

It would be different if this was a writable control: then you need to
know which locations are possible to set, and that requires a menu type.

But it doesn't make sense to set the location from software. However, the
location might change as a result of other changes: e.g. if the camera
has motor control of the tilt and the tilt changes from forward facing to
downward facing, then the driver might change the location from FRONT
to DOWN. A convoluted example perhaps, but this is just brainstorming.

Regards,

	Hans

> 
> Once this is clarified, I can send a proper v1.
> 
> Thanks
>   j
> 
>> Regards,
>>
>> 	Hans
>>
>>>
>>> Regards,
>>>
>>> 	Hans
>>>
>>>>
>>>>> +    This read-only control describes the camera location by reporting its
>>>>
>>>> Here too I would mention camera sensor instead of just camera (or
>>>> possibly imaging sensor).
>>>>
>>>>> +    mounting position on the device where the camera is installed. 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 position. In example, a camera installed on the user-facing side
>>>>> +    of a phone device is said to be installed in the ``V4L2_LOCATION_FRONT``
>>>>> +    position.
>>>>
>>>> The DT bindings could use such an example :-) I would extend this to
>>>> tablets and laptops.
>>>>
>>>>> +
>>>>> +
>>>>> +
>>>>
>>>> Do we need three blank lines ?
>>>>
>>>>> +.. flat-table::
>>>>> +    :header-rows:  0
>>>>> +    :stub-columns: 0
>>>>> +
>>>>> +    * - ``V4L2_LOCATION_FRONT``
>>>>> +      - The camera device is located on the front side of the device.
>>>>> +    * - ``V4L2_LOCATION_BACK``
>>>>> +      - The camera device is located on the back side of the device.
>>>>> +
>>>>> +
>>>>> +
>>>>>  .. [#f1]
>>>>>     This control may be changed to a menu control in the future, if more
>>>>>     options are required.
>>>>
>>>
>>

Re: [RFC 2/5] media: v4l2-ctrl: Document V4L2_CID_LOCATION

[Sakari Ailus]

Hi Hans,

On Thu, Aug 15, 2019 at 04:40:03PM +0200, Hans Verkuil wrote:
> On 8/15/19 4:34 PM, Jacopo Mondi wrote:
> > Hi Hans,
> > 
> > On Thu, Aug 15, 2019 at 04:14:38PM +0200, Hans Verkuil wrote:
> >> On 8/15/19 4:10 PM, Hans Verkuil wrote:
> >>> On 8/15/19 12:43 AM, Laurent Pinchart wrote:
> >>>> Hi Jacopo,
> >>>>
> >>>> Thank you for the patch.
> >>>>
> >>>> On Wed, Aug 14, 2019 at 10:28:12PM +0200, Jacopo Mondi wrote:
> >>>>> Add documentation for the V4L2_CID_LOCATION camera control. The newly
> >>>>> added read-only control reports the camera device mounting position.
> >>>>>
> >>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>>> ---
> >>>>>  .../media/uapi/v4l/ext-ctrls-camera.rst       | 23 +++++++++++++++++++
> >>>>>  1 file changed, 23 insertions(+)
> >>>>>
> >>>>> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> >>>>> index 51c1d5c9eb00..fc0a02eee6d4 100644
> >>>>> --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> >>>>> +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> >>>>> @@ -510,6 +510,29 @@ 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_LOCATION (integer)``
> >>>>
> >>>> Maybe V4L2_CID_CAMERA_SENSOR_LOCATION ? Same for the values below.
> >>>
> >>> Probably a better name, if a bit long. But we might need other location
> >>> controls in the future (e.g. flash location), so CID_LOCATION is just too
> >>> generic.
> >>
> > 
> > Thanks for the feedback.
> > 
> >> Note that the location defines themselves can most likely be used with any
> >> LOCATION control, so V4L2_LOCATION_FRONT would be fine with any control.
> >>
> > 
> > What do you think instead of the control type? Would a single integer
> > control do or an integer menu one would be better? I see merit in both
> > proposals actually...
> 
> Single integer. It's read-only, so it just reports the location.
> 
> It would be different if this was a writable control: then you need to
> know which locations are possible to set, and that requires a menu type.
> 
> But it doesn't make sense to set the location from software. However, the
> location might change as a result of other changes: e.g. if the camera
> has motor control of the tilt and the tilt changes from forward facing to
> downward facing, then the driver might change the location from FRONT
> to DOWN. A convoluted example perhaps, but this is just brainstorming.

When the camera points to another direction than directly away from the
surface, then we need another property to describe that. Location tells
where the camera is... well, located. :-)

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

Re: [RFC 1/5] media: dt-bindings: Document 'location' property

[Pavel Machek]

Hi!

> > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > @@ -89,6 +89,10 @@ Optional properties
> >    but a number of degrees counter clockwise. Typical values are 0 and 180
> >    (upside down).
> > 
> > +- location: The camera device mounting position, relative to the device
> > +  usage orientation. Possible values are:
> > +  0 - Front camera. The image sensor is mounted on the front side of the device.
> > +  1 - Back camera. The image sensor is mounted on the back side of the device.
> 
> Would it make sense to make this a little more generic? Such as s/image
> sensor/ device/, for instance?
> 
> Is this also relevant for flash or lens devices?
> 
> Flash (torch) devices could be present, at least principle, without a
> camera. There once was even such a Nokia phone, 1100 unless I'm mistaken.
> :-)

Well, I'd call them LEDs, not camera flashes ... if there's no camera. And IIRC 
these devices had LEDs on top of the phone... so neither front nor back side.

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

Re: [RFC 2/5] media: v4l2-ctrl: Document V4L2_CID_LOCATION

[Pavel Machek]

Hi!

> >>>>> @@ -510,6 +510,29 @@ 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_LOCATION (integer)``
> >>>>
> >>>> Maybe V4L2_CID_CAMERA_SENSOR_LOCATION ? Same for the values below.
> >>>
> >>> Probably a better name, if a bit long. But we might need other location
> >>> controls in the future (e.g. flash location), so CID_LOCATION is just too
> >>> generic.
> >>
> > 
> > Thanks for the feedback.
> > 
> >> Note that the location defines themselves can most likely be used with any
> >> LOCATION control, so V4L2_LOCATION_FRONT would be fine with any control.
> >>
> > 
> > What do you think instead of the control type? Would a single integer
> > control do or an integer menu one would be better? I see merit in both
> > proposals actually...
> 
> Single integer. It's read-only, so it just reports the location.
> 
> It would be different if this was a writable control: then you need to
> know which locations are possible to set, and that requires a menu type.
> 
> But it doesn't make sense to set the location from software. However, the
> location might change as a result of other changes: e.g. if the camera
> has motor control of the tilt and the tilt changes from forward facing to
> downward facing, then the driver might change the location from FRONT
> to DOWN. A convoluted example perhaps, but this is just brainstorming.

There are phones with exactly such camera setup. And yes, it makes sense to be writable
in that case, as software can move the camera in such case.

										Pavel

Re: [RFC 2/5] media: v4l2-ctrl: Document V4L2_CID_LOCATION

[Laurent Pinchart]

Hi Pavel,

On Sun, Sep 01, 2019 at 07:24:57PM +0200, Pavel Machek wrote:
> >>>>>> @@ -510,6 +510,29 @@ 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_LOCATION (integer)``
> >>>>>
> >>>>> Maybe V4L2_CID_CAMERA_SENSOR_LOCATION ? Same for the values below.
> >>>>
> >>>> Probably a better name, if a bit long. But we might need other location
> >>>> controls in the future (e.g. flash location), so CID_LOCATION is just too
> >>>> generic.
> >>>
> >> 
> >> Thanks for the feedback.
> >> 
> >>> Note that the location defines themselves can most likely be used with any
> >>> LOCATION control, so V4L2_LOCATION_FRONT would be fine with any control.
> >>>
> >> 
> >> What do you think instead of the control type? Would a single integer
> >> control do or an integer menu one would be better? I see merit in both
> >> proposals actually...
> > 
> > Single integer. It's read-only, so it just reports the location.
> > 
> > It would be different if this was a writable control: then you need to
> > know which locations are possible to set, and that requires a menu type.
> > 
> > But it doesn't make sense to set the location from software. However, the
> > location might change as a result of other changes: e.g. if the camera
> > has motor control of the tilt and the tilt changes from forward facing to
> > downward facing, then the driver might change the location from FRONT
> > to DOWN. A convoluted example perhaps, but this is just brainstorming.
> 
> There are phones with exactly such camera setup. And yes, it makes
> sense to be writable in that case, as software can move the camera in
> such case.

Out of curiosity, what phones are those ?

-- 
Regards,

Laurent Pinchart

Re: [RFC 1/5] media: dt-bindings: Document 'location' property

[Laurent Pinchart]

Hi Pavel,

On Sun, Sep 01, 2019 at 07:24:15PM +0200, Pavel Machek wrote:
> > > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > @@ -89,6 +89,10 @@ Optional properties
> > >    but a number of degrees counter clockwise. Typical values are 0 and 180
> > >    (upside down).
> > > 
> > > +- location: The camera device mounting position, relative to the device
> > > +  usage orientation. Possible values are:
> > > +  0 - Front camera. The image sensor is mounted on the front side of the device.
> > > +  1 - Back camera. The image sensor is mounted on the back side of the device.
> > 
> > Would it make sense to make this a little more generic? Such as s/image
> > sensor/ device/, for instance?
> > 
> > Is this also relevant for flash or lens devices?
> > 
> > Flash (torch) devices could be present, at least principle, without a
> > camera. There once was even such a Nokia phone, 1100 unless I'm mistaken.
> > :-)
> 
> Well, I'd call them LEDs, not camera flashes ... if there's no camera. And IIRC 
> these devices had LEDs on top of the phone... so neither front nor back side.

I would go for the name "torch" in that case. It really depends on the
device, but in any case, the torch LEDs would have a location (and we
would possibly need to expand this property to
include the top, bottom, left and right sides).

-- 
Regards,

Laurent Pinchart

Re: [RFC 2/5] media: v4l2-ctrl: Document V4L2_CID_LOCATION

[Pavel Machek]

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

Hi!

> > > Single integer. It's read-only, so it just reports the location.
> > > 
> > > It would be different if this was a writable control: then you need to
> > > know which locations are possible to set, and that requires a menu type.
> > > 
> > > But it doesn't make sense to set the location from software. However, the
> > > location might change as a result of other changes: e.g. if the camera
> > > has motor control of the tilt and the tilt changes from forward facing to
> > > downward facing, then the driver might change the location from FRONT
> > > to DOWN. A convoluted example perhaps, but this is just brainstorming.
> > 
> > There are phones with exactly such camera setup. And yes, it makes
> > sense to be writable in that case, as software can move the camera in
> > such case.
> 
> Out of curiosity, what phones are those ?

This one:

https://www.samsung.com/global/galaxy/galaxy-a80/

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: [RFC 1/5] media: dt-bindings: Document 'location' property

[Pavel Machek]

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

On Mon 2019-09-02 11:02:11, Laurent Pinchart wrote:
> Hi Pavel,
> 
> On Sun, Sep 01, 2019 at 07:24:15PM +0200, Pavel Machek wrote:
> > > > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > > @@ -89,6 +89,10 @@ Optional properties
> > > >    but a number of degrees counter clockwise. Typical values are 0 and 180
> > > >    (upside down).
> > > > 
> > > > +- location: The camera device mounting position, relative to the device
> > > > +  usage orientation. Possible values are:
> > > > +  0 - Front camera. The image sensor is mounted on the front side of the device.
> > > > +  1 - Back camera. The image sensor is mounted on the back side of the device.
> > > 
> > > Would it make sense to make this a little more generic? Such as s/image
> > > sensor/ device/, for instance?
> > > 
> > > Is this also relevant for flash or lens devices?
> > > 
> > > Flash (torch) devices could be present, at least principle, without a
> > > camera. There once was even such a Nokia phone, 1100 unless I'm mistaken.
> > > :-)
> > 
> > Well, I'd call them LEDs, not camera flashes ... if there's no camera. And IIRC 
> > these devices had LEDs on top of the phone... so neither front nor back side.
> 
> I would go for the name "torch" in that case. It really depends on the
> device, but in any case, the torch LEDs would have a location (and we
> would possibly need to expand this property to
> include the top, bottom, left and right sides).

Yes, but please let the torch devices be handled by LED subsystem
(/sys/class/leds). media/ subsystem is a bit too big and complex for
toggling LEDs on and off...

Thanks,
									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: [RFC 2/5] media: v4l2-ctrl: Document V4L2_CID_LOCATION

[Laurent Pinchart]

On Mon, Sep 02, 2019 at 10:06:57AM +0200, Pavel Machek wrote:
> >>> Single integer. It's read-only, so it just reports the location.
> >>> 
> >>> It would be different if this was a writable control: then you need to
> >>> know which locations are possible to set, and that requires a menu type.
> >>> 
> >>> But it doesn't make sense to set the location from software. However, the
> >>> location might change as a result of other changes: e.g. if the camera
> >>> has motor control of the tilt and the tilt changes from forward facing to
> >>> downward facing, then the driver might change the location from FRONT
> >>> to DOWN. A convoluted example perhaps, but this is just brainstorming.
> >> 
> >> There are phones with exactly such camera setup. And yes, it makes
> >> sense to be writable in that case, as software can move the camera in
> >> such case.
> > 
> > Out of curiosity, what phones are those ?
> 
> This one:
> 
> https://www.samsung.com/global/galaxy/galaxy-a80/

Interesting device. I'm not sure we should control that through a
location control though, as it seems there's more than the rotation of
the camera involved. In any case I wouldn't care about it for now, and
turn the location control from read-only to read-write later if needed.
We need more information and more thought to support that use case.

-- 
Regards,

Laurent Pinchart

Re: [RFC 2/5] media: v4l2-ctrl: Document V4L2_CID_LOCATION

[Pavel Machek]

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

On Mon 2019-09-02 11:19:42, Laurent Pinchart wrote:
> On Mon, Sep 02, 2019 at 10:06:57AM +0200, Pavel Machek wrote:
> > >>> Single integer. It's read-only, so it just reports the location.
> > >>> 
> > >>> It would be different if this was a writable control: then you need to
> > >>> know which locations are possible to set, and that requires a menu type.
> > >>> 
> > >>> But it doesn't make sense to set the location from software. However, the
> > >>> location might change as a result of other changes: e.g. if the camera
> > >>> has motor control of the tilt and the tilt changes from forward facing to
> > >>> downward facing, then the driver might change the location from FRONT
> > >>> to DOWN. A convoluted example perhaps, but this is just brainstorming.
> > >> 
> > >> There are phones with exactly such camera setup. And yes, it makes
> > >> sense to be writable in that case, as software can move the camera in
> > >> such case.
> > > 
> > > Out of curiosity, what phones are those ?
> > 
> > This one:
> > 
> > https://www.samsung.com/global/galaxy/galaxy-a80/
> 
> Interesting device. I'm not sure we should control that through a
> location control though, as it seems there's more than the rotation of
> the camera involved. In any case I wouldn't care about it for now, and
> turn the location control from read-only to read-write later if needed.
> We need more information and more thought to support that use case.

Well, the mechanism is there just to rotate the camera.

Anyway, that phone is probably nowhere close to having mainline
support, so...

									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: [RFC 2/5] media: v4l2-ctrl: Document V4L2_CID_LOCATION

[Laurent Pinchart]

Hi Pawel,

On Mon, Sep 02, 2019 at 10:27:39AM +0200, Pavel Machek wrote:
> On Mon 2019-09-02 11:19:42, Laurent Pinchart wrote:
> > On Mon, Sep 02, 2019 at 10:06:57AM +0200, Pavel Machek wrote:
> >>>>> Single integer. It's read-only, so it just reports the location.
> >>>>> 
> >>>>> It would be different if this was a writable control: then you need to
> >>>>> know which locations are possible to set, and that requires a menu type.
> >>>>> 
> >>>>> But it doesn't make sense to set the location from software. However, the
> >>>>> location might change as a result of other changes: e.g. if the camera
> >>>>> has motor control of the tilt and the tilt changes from forward facing to
> >>>>> downward facing, then the driver might change the location from FRONT
> >>>>> to DOWN. A convoluted example perhaps, but this is just brainstorming.
> >>>> 
> >>>> There are phones with exactly such camera setup. And yes, it makes
> >>>> sense to be writable in that case, as software can move the camera in
> >>>> such case.
> >>> 
> >>> Out of curiosity, what phones are those ?
> >> 
> >> This one:
> >> 
> >> https://www.samsung.com/global/galaxy/galaxy-a80/
> > 
> > Interesting device. I'm not sure we should control that through a
> > location control though, as it seems there's more than the rotation of
> > the camera involved. In any case I wouldn't care about it for now, and
> > turn the location control from read-only to read-write later if needed.
> > We need more information and more thought to support that use case.
> 
> Well, the mechanism is there just to rotate the camera.

But we don't know how it's implemented, it could be heavily
firmware-based for instance.

> Anyway, that phone is probably nowhere close to having mainline
> support, so...

If we need to support such a device in the future (and I hope we will
:-)) then I'm totally fine expanding the features of the location
control. My only concern is that I don't want to over-design it right
now without having enough information about the hardware that would make
use of it.

-- 
Regards,

Laurent Pinchart

Re: [RFC 2/5] media: v4l2-ctrl: Document V4L2_CID_LOCATION

[Jacopo Mondi]

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

Hi Pavel,

On Sun, Sep 01, 2019 at 07:24:57PM +0200, Pavel Machek wrote:
> Hi!
>
> > >>>>> @@ -510,6 +510,29 @@ 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_LOCATION (integer)``
> > >>>>
> > >>>> Maybe V4L2_CID_CAMERA_SENSOR_LOCATION ? Same for the values below.
> > >>>
> > >>> Probably a better name, if a bit long. But we might need other location
> > >>> controls in the future (e.g. flash location), so CID_LOCATION is just too
> > >>> generic.
> > >>
> > >
> > > Thanks for the feedback.
> > >
> > >> Note that the location defines themselves can most likely be used with any
> > >> LOCATION control, so V4L2_LOCATION_FRONT would be fine with any control.
> > >>
> > >
> > > What do you think instead of the control type? Would a single integer
> > > control do or an integer menu one would be better? I see merit in both
> > > proposals actually...
> >
> > Single integer. It's read-only, so it just reports the location.
> >
> > It would be different if this was a writable control: then you need to
> > know which locations are possible to set, and that requires a menu type.
> >
> > But it doesn't make sense to set the location from software. However, the
> > location might change as a result of other changes: e.g. if the camera
> > has motor control of the tilt and the tilt changes from forward facing to
> > downward facing, then the driver might change the location from FRONT
> > to DOWN. A convoluted example perhaps, but this is just brainstorming.
>
> There are phones with exactly such camera setup. And yes, it makes sense to be writable
> in that case, as software can move the camera in such case.

Nice, I had no idea!

Support for those kind of devices seems a bit far-fetched at the
moment, and as Laurent suggested, I would make the control writable
once we have a use case for that. But let's keep in mind that could
happen sooner or later!

Thanks
   j

>
> 										Pavel

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