[PATCH v6 1/4] media: dt-bindings: ov5675: document YAML binding

[PATCH v6 1/4] media: dt-bindings: ov5675: document YAML binding

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

This patch adds documentation of device tree in YAML schema for the
OV5675 CMOS image sensor from Omnivision.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---

v4:
 - added Reviewed-by,

v3:
 - removed clock-names,
 - removed clock-frequency,
 - added all-of of video-interface-devices schema,
 - added clock frequency range in description,
 - rephrased definition of supplies,
 - fixed name of reset gpio,
 - used schema ref for port and port->endpoint,
 - removed mentions to driver,
 - added HW data transfer speed limitation in comment for
 link-frequencies,
 - changed root additionalProperties to unevaluatedProperties to not
 have to list all properties from video-interface-devices schema, such as
 orientation or rotation,
 - added maxItems to reset-gpios,
 - updated example to use assigned-clocks and assigned-clock-rates
 instead of clock-frequency and clock-names,

v2:
 - fixed incorrect id,
 - fixed device tree example by adding missing dt-bindings headers,
 - fixed device tree example by using vcc_1v2 for dvdd supply, as requested
 in datasheet,

 .../bindings/media/i2c/ovti,ov5675.yaml       | 123 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 124 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml
new file mode 100644
index 000000000000..f0a48707bed7
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml
@@ -0,0 +1,123 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (c) 2022 Theobroma Systems Design und Consulting GmbH
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/ovti,ov5675.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Omnivision OV5675 CMOS Sensor
+
+maintainers:
+  - Quentin Schulz <quentin.schulz@theobroma-systems.com>
+
+allOf:
+  - $ref: /schemas/media/video-interface-devices.yaml#
+
+description: |
+  The Omnivision OV5675 is a high performance, 1/5-inch, 5 megapixel, CMOS
+  image sensor that delivers 2592x1944 at 30fps. It provides full-frame,
+  sub-sampled, and windowed 10-bit MIPI images in various formats via the
+  Serial Camera Control Bus (SCCB) interface.
+
+  This chip is programmable through I2C and two-wire SCCB. The sensor output
+  is available via CSI-2 serial data output (up to 2-lane).
+
+properties:
+  compatible:
+    const: ovti,ov5675
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    description:
+      System input clock (aka XVCLK). From 6 to 27 MHz.
+    maxItems: 1
+
+  dovdd-supply:
+    description:
+      Digital I/O voltage supply, 1.8 volts.
+
+  avdd-supply:
+    description:
+      Analog voltage supply, 2.8 volts.
+
+  dvdd-supply:
+    description:
+      Digital core voltage supply, 1.2 volts.
+
+  reset-gpios:
+    description:
+      The phandle and specifier for the GPIO that controls sensor reset.
+      This corresponds to the hardware pin XSHUTDN which is physically
+      active low.
+    maxItems: 1
+
+  port:
+    $ref: /schemas/graph.yaml#/$defs/port-base
+    additionalProperties: false
+
+    properties:
+      endpoint:
+        $ref: /schemas/media/video-interfaces.yaml#
+        unevaluatedProperties: false
+
+        properties:
+          data-lanes:
+            minItems: 1
+            maxItems: 2
+
+          # Supports max data transfer of 900 Mbps per lane
+          link-frequencies: true
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - dovdd-supply
+  - avdd-supply
+  - dvdd-supply
+  - port
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/px30-cru.h>
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/pinctrl/rockchip.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ov5675: camera@36 {
+            compatible = "ovti,ov5675";
+            reg = <0x36>;
+
+            reset-gpios = <&gpio2 RK_PB1 GPIO_ACTIVE_LOW>;
+            pinctrl-names = "default";
+            pinctrl-0 = <&cif_clkout_m0>;
+
+            clocks = <&cru SCLK_CIF_OUT>;
+            assigned-clocks = <&cru SCLK_CIF_OUT>;
+            assigned-clock-rates = <19200000>;
+
+            avdd-supply = <&vcc_1v8>;
+            dvdd-supply = <&vcc_1v2>;
+            dovdd-supply = <&vcc_2v8>;
+
+            rotation = <90>;
+            orientation = <0>;
+
+            port {
+                ucam_out: endpoint {
+                    remote-endpoint = <&mipi_in_ucam>;
+                    data-lanes = <1 2>;
+                    link-frequencies = /bits/ 64 <450000000>;
+                };
+            };
+        };
+    };
+...
+
diff --git a/MAINTAINERS b/MAINTAINERS
index a6d3bd9d2a8d..302983893831 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14703,6 +14703,7 @@ M:	Shawn Tu <shawnx.tu@intel.com>
 L:	linux-media@vger.kernel.org
 S:	Maintained
 T:	git git://linuxtv.org/media_tree.git
+F:	Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml
 F:	drivers/media/i2c/ov5675.c
 
 OMNIVISION OV5693 SENSOR DRIVER
-- 
2.36.1

[PATCH v6 2/4] media: ov5675: add device-tree support and support runtime PM

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

Until now, this driver only supported ACPI. This adds support for
Device Tree too while enabling clock and regulators in runtime PM.

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---

v6:
 - inverted-xmas-tree ordering,
 - space alignment instead of tab after data type for clk and
 reset_gpio,
 - added pm_runtime_set_suspended to remove callback as suggested by
 Tommaso (please cc the mailing list next time too so everyone can
 benefit from your reviews :) ),
 c.f. https://lists.linuxfoundation.org/pipermail/linux-pm/2012-July/034705.html
 - added Reviewed-by,

v5:
 - fixed -Wdeclaration-after-statement for delay_us,

v4:
 - added delays based on clock cycles as specified in datasheet for
 pre-power-off and post-power-on,
 - re-arranged clk handling, shutdown toggling and regulator handling to
 better match power up/down sequence defined in datasheet,
 - added comment on need for regulator being stable before releasing
 shutdown pin,

v3:
 - added linux/mod_devicetable.h include,
 - moved delay for reset pulse right after the regulators are enabled,
 - removed check on is_acpi_node in favor of checks on presence of OF
 properties (e.g. devm_clk_get_optional returns NULL),
 - moved power management out of system suspend/resume into runtime PM
 callbacks,
 - removed ACPI specific comment since it's not specific to this driver,
 - changed devm_clk_get to devm_clk_get_optional,
 - remove OF use of clock-frequency (handled by devm_clk_get_optional
 directly),
 - removed name of clock (only one, so no need for anything explicit)
 when requesting a clock from OF,
 - wrapped lines to 80 chars,

v2:
 - fixed unused-const-variable warning by removing of_match_ptr in
 of_match_table, reported by kernel test robot,

 drivers/media/i2c/ov5675.c | 150 +++++++++++++++++++++++++++++++------
 1 file changed, 129 insertions(+), 21 deletions(-)

diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
index 82ba9f56baec..e671fb24ebb4 100644
--- a/drivers/media/i2c/ov5675.c
+++ b/drivers/media/i2c/ov5675.c
@@ -3,10 +3,14 @@
 
 #include <asm/unaligned.h>
 #include <linux/acpi.h>
+#include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-fwnode.h>
@@ -17,7 +21,7 @@
 
 #define OV5675_LINK_FREQ_450MHZ		450000000ULL
 #define OV5675_SCLK			90000000LL
-#define OV5675_MCLK			19200000
+#define OV5675_XVCLK_19_2		19200000
 #define OV5675_DATA_LANES		2
 #define OV5675_RGB_DEPTH		10
 
@@ -76,6 +80,14 @@
 
 #define to_ov5675(_sd)			container_of(_sd, struct ov5675, sd)
 
+static const char * const ov5675_supply_names[] = {
+	"avdd",		/* Analog power */
+	"dovdd",	/* Digital I/O power */
+	"dvdd",		/* Digital core power */
+};
+
+#define OV5675_NUM_SUPPLIES	ARRAY_SIZE(ov5675_supply_names)
+
 enum {
 	OV5675_LINK_FREQ_900MBPS,
 };
@@ -484,6 +496,9 @@ struct ov5675 {
 	struct v4l2_subdev sd;
 	struct media_pad pad;
 	struct v4l2_ctrl_handler ctrl_handler;
+	struct clk *xvclk;
+	struct gpio_desc *reset_gpio;
+	struct regulator_bulk_data supplies[OV5675_NUM_SUPPLIES];
 
 	/* V4L2 Controls */
 	struct v4l2_ctrl *link_freq;
@@ -944,6 +959,56 @@ static int ov5675_set_stream(struct v4l2_subdev *sd, int enable)
 	return ret;
 }
 
+static int ov5675_power_off(struct device *dev)
+{
+	/* 512 xvclk cycles after the last SCCB transation or MIPI frame end */
+	u32 delay_us = DIV_ROUND_UP(512, OV5675_XVCLK_19_2 / 1000 / 1000);
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct ov5675 *ov5675 = to_ov5675(sd);
+
+	usleep_range(delay_us, delay_us * 2);
+
+	clk_disable_unprepare(ov5675->xvclk);
+	gpiod_set_value_cansleep(ov5675->reset_gpio, 1);
+	regulator_bulk_disable(OV5675_NUM_SUPPLIES, ov5675->supplies);
+
+	return 0;
+}
+
+static int ov5675_power_on(struct device *dev)
+{
+	u32 delay_us = DIV_ROUND_UP(8192, OV5675_XVCLK_19_2 / 1000 / 1000);
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct ov5675 *ov5675 = to_ov5675(sd);
+	int ret;
+
+	ret = clk_prepare_enable(ov5675->xvclk);
+	if (ret < 0) {
+		dev_err(dev, "failed to enable xvclk: %d\n", ret);
+		return ret;
+	}
+
+	gpiod_set_value_cansleep(ov5675->reset_gpio, 1);
+
+	ret = regulator_bulk_enable(OV5675_NUM_SUPPLIES, ov5675->supplies);
+	if (ret) {
+		clk_disable_unprepare(ov5675->xvclk);
+		return ret;
+	}
+
+	/* Reset pulse should be at least 2ms and reset gpio released only once
+	 * regulators are stable.
+	 */
+	usleep_range(2000, 2200);
+
+	gpiod_set_value_cansleep(ov5675->reset_gpio, 0);
+
+	/* 8192 xvclk cycles prior to the first SCCB transation */
+	usleep_range(delay_us, delay_us * 2);
+
+	return 0;
+}
+
 static int __maybe_unused ov5675_suspend(struct device *dev)
 {
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
@@ -1106,32 +1171,60 @@ static const struct v4l2_subdev_internal_ops ov5675_internal_ops = {
 	.open = ov5675_open,
 };
 
-static int ov5675_check_hwcfg(struct device *dev)
+static int ov5675_get_hwcfg(struct ov5675 *ov5675, struct device *dev)
 {
 	struct fwnode_handle *ep;
 	struct fwnode_handle *fwnode = dev_fwnode(dev);
 	struct v4l2_fwnode_endpoint bus_cfg = {
 		.bus_type = V4L2_MBUS_CSI2_DPHY
 	};
-	u32 mclk;
+	u32 xvclk_rate;
 	int ret;
 	unsigned int i, j;
 
 	if (!fwnode)
 		return -ENXIO;
 
-	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
+	ov5675->xvclk = devm_clk_get_optional(dev, NULL);
+	if (IS_ERR(ov5675->xvclk))
+		return dev_err_probe(dev, PTR_ERR(ov5675->xvclk),
+				     "failed to get xvclk: %ld\n",
+				     PTR_ERR(ov5675->xvclk));
 
-	if (ret) {
-		dev_err(dev, "can't get clock frequency");
-		return ret;
+	if (ov5675->xvclk) {
+		xvclk_rate = clk_get_rate(ov5675->xvclk);
+	} else {
+		ret = fwnode_property_read_u32(fwnode, "clock-frequency",
+					       &xvclk_rate);
+
+		if (ret) {
+			dev_err(dev, "can't get clock frequency");
+			return ret;
+		}
 	}
 
-	if (mclk != OV5675_MCLK) {
-		dev_err(dev, "external clock %d is not supported", mclk);
+	if (xvclk_rate != OV5675_XVCLK_19_2) {
+		dev_err(dev, "external clock rate %u is unsupported",
+			xvclk_rate);
 		return -EINVAL;
 	}
 
+	ov5675->reset_gpio = devm_gpiod_get_optional(dev, "reset",
+						     GPIOD_OUT_HIGH);
+	if (IS_ERR(ov5675->reset_gpio)) {
+		ret = PTR_ERR(ov5675->reset_gpio);
+		dev_err(dev, "failed to get reset-gpios: %d\n", ret);
+		return ret;
+	}
+
+	for (i = 0; i < OV5675_NUM_SUPPLIES; i++)
+		ov5675->supplies[i].supply = ov5675_supply_names[i];
+
+	ret = devm_regulator_bulk_get(dev, OV5675_NUM_SUPPLIES,
+				      ov5675->supplies);
+	if (ret)
+		return ret;
+
 	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
 	if (!ep)
 		return -ENXIO;
@@ -1186,6 +1279,10 @@ static int ov5675_remove(struct i2c_client *client)
 	pm_runtime_disable(&client->dev);
 	mutex_destroy(&ov5675->mutex);
 
+	if (!pm_runtime_status_suspended(&client->dev))
+		ov5675_power_off(&client->dev);
+	pm_runtime_set_suspended(&client->dev);
+
 	return 0;
 }
 
@@ -1195,25 +1292,31 @@ static int ov5675_probe(struct i2c_client *client)
 	bool full_power;
 	int ret;
 
-	ret = ov5675_check_hwcfg(&client->dev);
+	ov5675 = devm_kzalloc(&client->dev, sizeof(*ov5675), GFP_KERNEL);
+	if (!ov5675)
+		return -ENOMEM;
+
+	ret = ov5675_get_hwcfg(ov5675, &client->dev);
 	if (ret) {
-		dev_err(&client->dev, "failed to check HW configuration: %d",
+		dev_err(&client->dev, "failed to get HW configuration: %d",
 			ret);
 		return ret;
 	}
 
-	ov5675 = devm_kzalloc(&client->dev, sizeof(*ov5675), GFP_KERNEL);
-	if (!ov5675)
-		return -ENOMEM;
-
 	v4l2_i2c_subdev_init(&ov5675->sd, client, &ov5675_subdev_ops);
 
+	ret = ov5675_power_on(&client->dev);
+	if (ret) {
+		dev_err(&client->dev, "failed to power on: %d\n", ret);
+		return ret;
+	}
+
 	full_power = acpi_dev_state_d0(&client->dev);
 	if (full_power) {
 		ret = ov5675_identify_module(ov5675);
 		if (ret) {
 			dev_err(&client->dev, "failed to find sensor: %d", ret);
-			return ret;
+			goto probe_power_off;
 		}
 	}
 
@@ -1243,11 +1346,6 @@ static int ov5675_probe(struct i2c_client *client)
 		goto probe_error_media_entity_cleanup;
 	}
 
-	/*
-	 * Device is already turned on by i2c-core with ACPI domain PM.
-	 * Enable runtime PM and turn off the device.
-	 */
-
 	/* Set the device's state to active if it's in D0 state. */
 	if (full_power)
 		pm_runtime_set_active(&client->dev);
@@ -1262,12 +1360,15 @@ static int ov5675_probe(struct i2c_client *client)
 probe_error_v4l2_ctrl_handler_free:
 	v4l2_ctrl_handler_free(ov5675->sd.ctrl_handler);
 	mutex_destroy(&ov5675->mutex);
+probe_power_off:
+	ov5675_power_off(&client->dev);
 
 	return ret;
 }
 
 static const struct dev_pm_ops ov5675_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(ov5675_suspend, ov5675_resume)
+	SET_RUNTIME_PM_OPS(ov5675_power_off, ov5675_power_on, NULL)
 };
 
 #ifdef CONFIG_ACPI
@@ -1279,11 +1380,18 @@ static const struct acpi_device_id ov5675_acpi_ids[] = {
 MODULE_DEVICE_TABLE(acpi, ov5675_acpi_ids);
 #endif
 
+static const struct of_device_id ov5675_of_match[] = {
+	{ .compatible = "ovti,ov5675", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, ov5675_of_match);
+
 static struct i2c_driver ov5675_i2c_driver = {
 	.driver = {
 		.name = "ov5675",
 		.pm = &ov5675_pm_ops,
 		.acpi_match_table = ACPI_PTR(ov5675_acpi_ids),
+		.of_match_table = ov5675_of_match,
 	},
 	.probe_new = ov5675_probe,
 	.remove = ov5675_remove,
-- 
2.36.1

[PATCH v6 3/4] media: i2c: ov5675: parse and register V4L2 device tree properties

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

Parse V4L2 device tree properties and register controls for them.

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---

v3:
 - reverse-xmas-tree-ordered in ov5675_init_controls,

 drivers/media/i2c/ov5675.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
index e671fb24ebb4..80840ad7bbb0 100644
--- a/drivers/media/i2c/ov5675.c
+++ b/drivers/media/i2c/ov5675.c
@@ -779,12 +779,14 @@ static const struct v4l2_ctrl_ops ov5675_ctrl_ops = {
 
 static int ov5675_init_controls(struct ov5675 *ov5675)
 {
+	struct i2c_client *client = v4l2_get_subdevdata(&ov5675->sd);
+	struct v4l2_fwnode_device_properties props;
 	struct v4l2_ctrl_handler *ctrl_hdlr;
 	s64 exposure_max, h_blank;
 	int ret;
 
 	ctrl_hdlr = &ov5675->ctrl_handler;
-	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8);
+	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10);
 	if (ret)
 		return ret;
 
@@ -838,9 +840,23 @@ static int ov5675_init_controls(struct ov5675 *ov5675)
 	if (ctrl_hdlr->error)
 		return ctrl_hdlr->error;
 
+	ret = v4l2_fwnode_device_parse(&client->dev, &props);
+	if (ret)
+		goto error;
+
+	ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &ov5675_ctrl_ops,
+					      &props);
+	if (ret)
+		goto error;
+
 	ov5675->sd.ctrl_handler = ctrl_hdlr;
 
 	return 0;
+
+error:
+	v4l2_ctrl_handler_free(ctrl_hdlr);
+
+	return ret;
 }
 
 static void ov5675_update_pad_format(const struct ov5675_mode *mode,
-- 
2.36.1

[PATCH v6 4/4] media: i2c: ov5675: add .get_selection support

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

The sensor has 2592*1944 active pixels, surrounded by 16 active dummy
pixels and there are an additional 24 black rows "at the bottom".

                     [2624]
        +-----+------------------+-----+
        |     |     16 dummy     |     |
        +-----+------------------+-----+
        |     |                  |     |
        |     |     [2592]       |     |
        |     |                  |     |
        |16   |      valid       | 16  |[2000]
        |dummy|                  |dummy|
        |     |            [1944]|     |
        |     |                  |     |
        +-----+------------------+-----+
        |     |     16 dummy     |     |
        +-----+------------------+-----+
        |     |  24 black lines  |     |
        +-----+------------------+-----+

The top-left coordinate is gotten from the registers specified in the
modes which are identical for both currently supported modes.

There are currently two modes supported by this driver: 2592*1944 and
1296*972. The second mode is obtained thanks to subsampling while
keeping the same field of view (FoV). No cropping involved, hence the
harcoded values.

Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---

v6:
 - explicit a bit more the commit log around subsampling for lower
 resolution modes,
 - (again) fixed reporting for V4L2_SEL_TGT_CROP_* thanks to Jacopo's help,

v4:
 - explicit a bit more the commit log,
 - added drawing in the commit log,
 - fixed reporting for V4L2_SEL_TGT_CROP_* thanks to Jacopo's help,

added in v3

 drivers/media/i2c/ov5675.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
index 80840ad7bbb0..2230ff47ef49 100644
--- a/drivers/media/i2c/ov5675.c
+++ b/drivers/media/i2c/ov5675.c
@@ -1121,6 +1121,26 @@ static int ov5675_get_format(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int ov5675_get_selection(struct v4l2_subdev *sd,
+				struct v4l2_subdev_state *state,
+				struct v4l2_subdev_selection *sel)
+{
+	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
+		return -EINVAL;
+
+	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP:
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+		sel->r.top = 16;
+		sel->r.left = 16;
+		sel->r.width = 2592;
+		sel->r.height = 1944;
+		return 0;
+	}
+	return -EINVAL;
+}
+
 static int ov5675_enum_mbus_code(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *sd_state,
 				 struct v4l2_subdev_mbus_code_enum *code)
@@ -1170,6 +1190,7 @@ static const struct v4l2_subdev_video_ops ov5675_video_ops = {
 static const struct v4l2_subdev_pad_ops ov5675_pad_ops = {
 	.set_fmt = ov5675_set_format,
 	.get_fmt = ov5675_get_format,
+	.get_selection = ov5675_get_selection,
 	.enum_mbus_code = ov5675_enum_mbus_code,
 	.enum_frame_size = ov5675_enum_frame_size,
 };
-- 
2.36.1

Re: [PATCH v6 4/4] media: i2c: ov5675: add .get_selection support

[Jacopo Mondi]

Hi Quentin,

On Tue, Jun 07, 2022 at 05:33:35PM +0200, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>
> The sensor has 2592*1944 active pixels, surrounded by 16 active dummy
> pixels and there are an additional 24 black rows "at the bottom".
>
>                      [2624]
>         +-----+------------------+-----+
>         |     |     16 dummy     |     |
>         +-----+------------------+-----+
>         |     |                  |     |
>         |     |     [2592]       |     |
>         |     |                  |     |
>         |16   |      valid       | 16  |[2000]
>         |dummy|                  |dummy|
>         |     |            [1944]|     |
>         |     |                  |     |
>         +-----+------------------+-----+
>         |     |     16 dummy     |     |
>         +-----+------------------+-----+
>         |     |  24 black lines  |     |
>         +-----+------------------+-----+
>
> The top-left coordinate is gotten from the registers specified in the
> modes which are identical for both currently supported modes.
>
> There are currently two modes supported by this driver: 2592*1944 and
> 1296*972. The second mode is obtained thanks to subsampling while
> keeping the same field of view (FoV). No cropping involved, hence the
> harcoded values.
>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> ---
>
> v6:
>  - explicit a bit more the commit log around subsampling for lower
>  resolution modes,
>  - (again) fixed reporting for V4L2_SEL_TGT_CROP_* thanks to Jacopo's help,
>
> v4:
>  - explicit a bit more the commit log,
>  - added drawing in the commit log,
>  - fixed reporting for V4L2_SEL_TGT_CROP_* thanks to Jacopo's help,
>
> added in v3
>
>  drivers/media/i2c/ov5675.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
> index 80840ad7bbb0..2230ff47ef49 100644
> --- a/drivers/media/i2c/ov5675.c
> +++ b/drivers/media/i2c/ov5675.c
> @@ -1121,6 +1121,26 @@ static int ov5675_get_format(struct v4l2_subdev *sd,
>  	return 0;
>  }
>
> +static int ov5675_get_selection(struct v4l2_subdev *sd,
> +				struct v4l2_subdev_state *state,
> +				struct v4l2_subdev_selection *sel)
> +{
> +	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> +		return -EINVAL;
> +
> +	switch (sel->target) {
> +	case V4L2_SEL_TGT_CROP:
> +	case V4L2_SEL_TGT_CROP_BOUNDS:

Seem like we have trouble understanding each other, or better, I have
troubles explaining myself most probably :)

If the dummy/black area is readable, this should just be (0, 0, 2624,
2000) like it was in your previous version. What has changed that I
have missed ?

Thanks
  j


> +	case V4L2_SEL_TGT_CROP_DEFAULT:
> +		sel->r.top = 16;
> +		sel->r.left = 16;
> +		sel->r.width = 2592;
> +		sel->r.height = 1944;
> +		return 0;
> +	}
> +	return -EINVAL;
> +}
> +
>  static int ov5675_enum_mbus_code(struct v4l2_subdev *sd,
>  				 struct v4l2_subdev_state *sd_state,
>  				 struct v4l2_subdev_mbus_code_enum *code)
> @@ -1170,6 +1190,7 @@ static const struct v4l2_subdev_video_ops ov5675_video_ops = {
>  static const struct v4l2_subdev_pad_ops ov5675_pad_ops = {
>  	.set_fmt = ov5675_set_format,
>  	.get_fmt = ov5675_get_format,
> +	.get_selection = ov5675_get_selection,
>  	.enum_mbus_code = ov5675_enum_mbus_code,
>  	.enum_frame_size = ov5675_enum_frame_size,
>  };
> --
> 2.36.1
>

Re: [PATCH v6 4/4] media: i2c: ov5675: add .get_selection support

[Tommaso Merciai]

Hi Quentin/Jacopo,

On Tue, Jun 07, 2022 at 06:51:36PM +0200, Jacopo Mondi wrote:
> Hi Quentin,
> 
> On Tue, Jun 07, 2022 at 05:33:35PM +0200, Quentin Schulz wrote:
> > From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> >
> > The sensor has 2592*1944 active pixels, surrounded by 16 active dummy
> > pixels and there are an additional 24 black rows "at the bottom".
> >
> >                      [2624]
> >         +-----+------------------+-----+
> >         |     |     16 dummy     |     |
> >         +-----+------------------+-----+
> >         |     |                  |     |
> >         |     |     [2592]       |     |
> >         |     |                  |     |
> >         |16   |      valid       | 16  |[2000]
> >         |dummy|                  |dummy|
> >         |     |            [1944]|     |
> >         |     |                  |     |
> >         +-----+------------------+-----+
> >         |     |     16 dummy     |     |
> >         +-----+------------------+-----+
> >         |     |  24 black lines  |     |
> >         +-----+------------------+-----+
> >
> > The top-left coordinate is gotten from the registers specified in the
> > modes which are identical for both currently supported modes.
> >
> > There are currently two modes supported by this driver: 2592*1944 and
> > 1296*972. The second mode is obtained thanks to subsampling while
> > keeping the same field of view (FoV). No cropping involved, hence the
> > harcoded values.
> >
> > Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > ---
> >
> > v6:
> >  - explicit a bit more the commit log around subsampling for lower
> >  resolution modes,
> >  - (again) fixed reporting for V4L2_SEL_TGT_CROP_* thanks to Jacopo's help,
> >
> > v4:
> >  - explicit a bit more the commit log,
> >  - added drawing in the commit log,
> >  - fixed reporting for V4L2_SEL_TGT_CROP_* thanks to Jacopo's help,
> >
> > added in v3
> >
> >  drivers/media/i2c/ov5675.c | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
> > index 80840ad7bbb0..2230ff47ef49 100644
> > --- a/drivers/media/i2c/ov5675.c
> > +++ b/drivers/media/i2c/ov5675.c
> > @@ -1121,6 +1121,26 @@ static int ov5675_get_format(struct v4l2_subdev *sd,
> >  	return 0;
> >  }
> >
> > +static int ov5675_get_selection(struct v4l2_subdev *sd,
> > +				struct v4l2_subdev_state *state,
> > +				struct v4l2_subdev_selection *sel)
> > +{
> > +	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> > +		return -EINVAL;
> > +
> > +	switch (sel->target) {
> > +	case V4L2_SEL_TGT_CROP:
> > +	case V4L2_SEL_TGT_CROP_BOUNDS:
> 
> Seem like we have trouble understanding each other, or better, I have
> troubles explaining myself most probably :)
> 
> If the dummy/black area is readable, this should just be (0, 0, 2624,
> 2000) like it was in your previous version. What has changed that I
> have missed ?

Taking as reference drivers/media/i2c/ov5693.c and others,
seems ok what Quentin have done from my side.

Just one thing: maybe is better to avoid magic numbers with more
explicit defines like:

 + case V4L2_SEL_TGT_CROP_DEFAULT:
 +           sel->r.top = OV5675_ACTIVE_START_TOP;
 +           sel->r.left = OV5693_ACTIVE_START_LEFT;
 +           sel->r.width = OV5693_ACTIVE_WIDTH;
 +           sel->r.height = OV5693_ACTIVE_HEIGHT;
 
What do you think about?

Thanks,
Tommaso


> 
> Thanks
>   j
> 
> 
> > +	case V4L2_SEL_TGT_CROP_DEFAULT:
> > +		sel->r.top = 16;
> > +		sel->r.left = 16;
> > +		sel->r.width = 2592;
> > +		sel->r.height = 1944;
> > +		return 0;
> > +	}
> > +	return -EINVAL;
> > +}
> > +
> >  static int ov5675_enum_mbus_code(struct v4l2_subdev *sd,
> >  				 struct v4l2_subdev_state *sd_state,
> >  				 struct v4l2_subdev_mbus_code_enum *code)
> > @@ -1170,6 +1190,7 @@ static const struct v4l2_subdev_video_ops ov5675_video_ops = {
> >  static const struct v4l2_subdev_pad_ops ov5675_pad_ops = {
> >  	.set_fmt = ov5675_set_format,
> >  	.get_fmt = ov5675_get_format,
> > +	.get_selection = ov5675_get_selection,
> >  	.enum_mbus_code = ov5675_enum_mbus_code,
> >  	.enum_frame_size = ov5675_enum_frame_size,
> >  };
> > --
> > 2.36.1
> >

-- 
Tommaso Merciai
Embedded Linux Engineer
tommaso.merciai@amarulasolutions.com
__________________________________

Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
info@amarulasolutions.com
www.amarulasolutions.com

Re: [PATCH v6 4/4] media: i2c: ov5675: add .get_selection support

[Jacopo Mondi]

Hi

On Wed, Jun 08, 2022 at 12:04:05AM +0200, Tommaso Merciai wrote:
> Hi Quentin/Jacopo,
>
> On Tue, Jun 07, 2022 at 06:51:36PM +0200, Jacopo Mondi wrote:
> > Hi Quentin,
> >
> > On Tue, Jun 07, 2022 at 05:33:35PM +0200, Quentin Schulz wrote:
> > > From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > >
> > > The sensor has 2592*1944 active pixels, surrounded by 16 active dummy
> > > pixels and there are an additional 24 black rows "at the bottom".
> > >
> > >                      [2624]
> > >         +-----+------------------+-----+
> > >         |     |     16 dummy     |     |
> > >         +-----+------------------+-----+
> > >         |     |                  |     |
> > >         |     |     [2592]       |     |
> > >         |     |                  |     |
> > >         |16   |      valid       | 16  |[2000]
> > >         |dummy|                  |dummy|
> > >         |     |            [1944]|     |
> > >         |     |                  |     |
> > >         +-----+------------------+-----+
> > >         |     |     16 dummy     |     |
> > >         +-----+------------------+-----+
> > >         |     |  24 black lines  |     |
> > >         +-----+------------------+-----+
> > >
> > > The top-left coordinate is gotten from the registers specified in the
> > > modes which are identical for both currently supported modes.
> > >
> > > There are currently two modes supported by this driver: 2592*1944 and
> > > 1296*972. The second mode is obtained thanks to subsampling while
> > > keeping the same field of view (FoV). No cropping involved, hence the
> > > harcoded values.
> > >
> > > Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > > ---
> > >
> > > v6:
> > >  - explicit a bit more the commit log around subsampling for lower
> > >  resolution modes,
> > >  - (again) fixed reporting for V4L2_SEL_TGT_CROP_* thanks to Jacopo's help,
> > >
> > > v4:
> > >  - explicit a bit more the commit log,
> > >  - added drawing in the commit log,
> > >  - fixed reporting for V4L2_SEL_TGT_CROP_* thanks to Jacopo's help,
> > >
> > > added in v3
> > >
> > >  drivers/media/i2c/ov5675.c | 21 +++++++++++++++++++++
> > >  1 file changed, 21 insertions(+)
> > >
> > > diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
> > > index 80840ad7bbb0..2230ff47ef49 100644
> > > --- a/drivers/media/i2c/ov5675.c
> > > +++ b/drivers/media/i2c/ov5675.c
> > > @@ -1121,6 +1121,26 @@ static int ov5675_get_format(struct v4l2_subdev *sd,
> > >  	return 0;
> > >  }
> > >
> > > +static int ov5675_get_selection(struct v4l2_subdev *sd,
> > > +				struct v4l2_subdev_state *state,
> > > +				struct v4l2_subdev_selection *sel)
> > > +{
> > > +	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> > > +		return -EINVAL;
> > > +
> > > +	switch (sel->target) {
> > > +	case V4L2_SEL_TGT_CROP:
> > > +	case V4L2_SEL_TGT_CROP_BOUNDS:
> >
> > Seem like we have trouble understanding each other, or better, I have
> > troubles explaining myself most probably :)
> >
> > If the dummy/black area is readable, this should just be (0, 0, 2624,
> > 2000) like it was in your previous version. What has changed that I
> > have missed ?
>
> Taking as reference drivers/media/i2c/ov5693.c and others,
> seems ok what Quentin have done from my side.
>
> Just one thing: maybe is better to avoid magic numbers with more
> explicit defines like:
>
>  + case V4L2_SEL_TGT_CROP_DEFAULT:
>  +           sel->r.top = OV5675_ACTIVE_START_TOP;
>  +           sel->r.left = OV5693_ACTIVE_START_LEFT;
>  +           sel->r.width = OV5693_ACTIVE_WIDTH;
>  +           sel->r.height = OV5693_ACTIVE_HEIGHT;
>
> What do you think about?
>
> Thanks,
> Tommaso
>
>

We have extensively discussed this:
https://patchwork.linuxtv.org/project/linux-media/patch/20220509143226.531117-4-foss+kernel@0leil.net/
https://patchwork.linuxtv.org/project/linux-media/patch/20220525145833.1165437-4-foss+kernel@0leil.net/

From the CROP_BOUNDS definition:
Bounds of the crop rectangle. All valid crop rectangles fit inside the
crop bounds rectangle.

From CROP_DEFAULT:
Suggested cropping rectangle that covers the “whole picture”. This
includes only active pixels and excludes other non-active pixels such
as black pixels.

If (and only if) dummy/inactive pixels can be read out, the BOUNDS
rectangle should contain them. In this case, as the analog crop
rectangle is defined with a 16x16 top-left corner (and with a visible
size of 2592x1944) from a larger rectangle, I presume it means
dummy/invalid pixls can be read (IOW you can give to the ISP a rectangle
that includes them).

Anyway, we're discussing details really. I think v5 was almost there

+	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+		sel->r.top = 0;
+		sel->r.left = 0;
+		sel->r.width = 2624;
+		sel->r.height = 2000;
+		return 0;
+	case V4L2_SEL_TGT_CROP:
+		sel->r.top = 16;
+		sel->r.left = 16;
+		sel->r.width = ov5675->cur_mode->width;
+		sel->r.height = ov5675->cur_mode->height;
+		return 0;
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+		sel->r.top = 16;
+		sel->r.left = 16;
+		sel->r.width = supported_modes[0].width;
+		sel->r.height = supported_modes[0].height;
+		return 0;
+	}

Apart from the fact that TGT_CROP should not report the final image
size (after binning/digital crop) but the size of the pixel array
portion processed to obtain the final image.


+	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+		sel->r.top = 0;
+		sel->r.left = 0;
+		sel->r.width = 2624;
+		sel->r.height = 2000;
+		return 0;
+	case V4L2_SEL_TGT_CROP:
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+		sel->r.top = 16;
+		sel->r.left = 16;
+		sel->r.width = 2592;
+		sel->r.height = 1944;
+		return 0;
+	}

Let me remind that (in the context of pleasing libcamera requirements
as Quentin is doing) all targets apart TGT_CROP are only useful to
report static properties of the camera, which are of no real use
unless you start actually looking into reading out black pixels etc
etc.


> >
> > Thanks
> >   j
> >
> >
> > > +	case V4L2_SEL_TGT_CROP_DEFAULT:
> > > +		sel->r.top = 16;
> > > +		sel->r.left = 16;
> > > +		sel->r.width = 2592;
> > > +		sel->r.height = 1944;
> > > +		return 0;
> > > +	}
> > > +	return -EINVAL;
> > > +}
> > > +
> > >  static int ov5675_enum_mbus_code(struct v4l2_subdev *sd,
> > >  				 struct v4l2_subdev_state *sd_state,
> > >  				 struct v4l2_subdev_mbus_code_enum *code)
> > > @@ -1170,6 +1190,7 @@ static const struct v4l2_subdev_video_ops ov5675_video_ops = {
> > >  static const struct v4l2_subdev_pad_ops ov5675_pad_ops = {
> > >  	.set_fmt = ov5675_set_format,
> > >  	.get_fmt = ov5675_get_format,
> > > +	.get_selection = ov5675_get_selection,
> > >  	.enum_mbus_code = ov5675_enum_mbus_code,
> > >  	.enum_frame_size = ov5675_enum_frame_size,
> > >  };
> > > --
> > > 2.36.1
> > >
>
> --
> Tommaso Merciai
> Embedded Linux Engineer
> tommaso.merciai@amarulasolutions.com
> __________________________________
>
> Amarula Solutions SRL
> Via Le Canevare 30, 31100 Treviso, Veneto, IT
> T. +39 042 243 5310
> info@amarulasolutions.com
> www.amarulasolutions.com

Re: [PATCH v6 4/4] media: i2c: ov5675: add .get_selection support

[Tommaso Merciai]

Hi,

On Wed, Jun 08, 2022 at 08:42:09AM +0200, Jacopo Mondi wrote:
> Hi
> 
> On Wed, Jun 08, 2022 at 12:04:05AM +0200, Tommaso Merciai wrote:
> > Hi Quentin/Jacopo,
> >
> > On Tue, Jun 07, 2022 at 06:51:36PM +0200, Jacopo Mondi wrote:
> > > Hi Quentin,
> > >
> > > On Tue, Jun 07, 2022 at 05:33:35PM +0200, Quentin Schulz wrote:
> > > > From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > > >
> > > > The sensor has 2592*1944 active pixels, surrounded by 16 active dummy
> > > > pixels and there are an additional 24 black rows "at the bottom".
> > > >
> > > >                      [2624]
> > > >         +-----+------------------+-----+
> > > >         |     |     16 dummy     |     |
> > > >         +-----+------------------+-----+
> > > >         |     |                  |     |
> > > >         |     |     [2592]       |     |
> > > >         |     |                  |     |
> > > >         |16   |      valid       | 16  |[2000]
> > > >         |dummy|                  |dummy|
> > > >         |     |            [1944]|     |
> > > >         |     |                  |     |
> > > >         +-----+------------------+-----+
> > > >         |     |     16 dummy     |     |
> > > >         +-----+------------------+-----+
> > > >         |     |  24 black lines  |     |
> > > >         +-----+------------------+-----+
> > > >
> > > > The top-left coordinate is gotten from the registers specified in the
> > > > modes which are identical for both currently supported modes.
> > > >
> > > > There are currently two modes supported by this driver: 2592*1944 and
> > > > 1296*972. The second mode is obtained thanks to subsampling while
> > > > keeping the same field of view (FoV). No cropping involved, hence the
> > > > harcoded values.
> > > >
> > > > Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > > > ---
> > > >
> > > > v6:
> > > >  - explicit a bit more the commit log around subsampling for lower
> > > >  resolution modes,
> > > >  - (again) fixed reporting for V4L2_SEL_TGT_CROP_* thanks to Jacopo's help,
> > > >
> > > > v4:
> > > >  - explicit a bit more the commit log,
> > > >  - added drawing in the commit log,
> > > >  - fixed reporting for V4L2_SEL_TGT_CROP_* thanks to Jacopo's help,
> > > >
> > > > added in v3
> > > >
> > > >  drivers/media/i2c/ov5675.c | 21 +++++++++++++++++++++
> > > >  1 file changed, 21 insertions(+)
> > > >
> > > > diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
> > > > index 80840ad7bbb0..2230ff47ef49 100644
> > > > --- a/drivers/media/i2c/ov5675.c
> > > > +++ b/drivers/media/i2c/ov5675.c
> > > > @@ -1121,6 +1121,26 @@ static int ov5675_get_format(struct v4l2_subdev *sd,
> > > >  	return 0;
> > > >  }
> > > >
> > > > +static int ov5675_get_selection(struct v4l2_subdev *sd,
> > > > +				struct v4l2_subdev_state *state,
> > > > +				struct v4l2_subdev_selection *sel)
> > > > +{
> > > > +	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> > > > +		return -EINVAL;
> > > > +
> > > > +	switch (sel->target) {
> > > > +	case V4L2_SEL_TGT_CROP:
> > > > +	case V4L2_SEL_TGT_CROP_BOUNDS:
> > >
> > > Seem like we have trouble understanding each other, or better, I have
> > > troubles explaining myself most probably :)
> > >
> > > If the dummy/black area is readable, this should just be (0, 0, 2624,
> > > 2000) like it was in your previous version. What has changed that I
> > > have missed ?
> >
> > Taking as reference drivers/media/i2c/ov5693.c and others,
> > seems ok what Quentin have done from my side.
> >
> > Just one thing: maybe is better to avoid magic numbers with more
> > explicit defines like:
> >
> >  + case V4L2_SEL_TGT_CROP_DEFAULT:
> >  +           sel->r.top = OV5675_ACTIVE_START_TOP;
> >  +           sel->r.left = OV5693_ACTIVE_START_LEFT;
> >  +           sel->r.width = OV5693_ACTIVE_WIDTH;
> >  +           sel->r.height = OV5693_ACTIVE_HEIGHT;
> >
> > What do you think about?
> >
> > Thanks,
> > Tommaso
> >
> >
> 
> We have extensively discussed this:
> https://patchwork.linuxtv.org/project/linux-media/patch/20220509143226.531117-4-foss+kernel@0leil.net/
> https://patchwork.linuxtv.org/project/linux-media/patch/20220525145833.1165437-4-foss+kernel@0leil.net/
> 
> From the CROP_BOUNDS definition:
> Bounds of the crop rectangle. All valid crop rectangles fit inside the
> crop bounds rectangle.
> 
> From CROP_DEFAULT:
> Suggested cropping rectangle that covers the “whole picture”. This
> includes only active pixels and excludes other non-active pixels such
> as black pixels.
> 
> If (and only if) dummy/inactive pixels can be read out, the BOUNDS
> rectangle should contain them. In this case, as the analog crop
> rectangle is defined with a 16x16 top-left corner (and with a visible
> size of 2592x1944) from a larger rectangle, I presume it means
> dummy/invalid pixls can be read (IOW you can give to the ISP a rectangle
> that includes them).

Thanks for sharing this.

> 
> Anyway, we're discussing details really. I think v5 was almost there
> 
> +	switch (sel->target) {
> +	case V4L2_SEL_TGT_CROP_BOUNDS:
> +		sel->r.top = 0;
> +		sel->r.left = 0;
> +		sel->r.width = 2624;
> +		sel->r.height = 2000;
> +		return 0;
> +	case V4L2_SEL_TGT_CROP:
> +		sel->r.top = 16;
> +		sel->r.left = 16;
> +		sel->r.width = ov5675->cur_mode->width;
> +		sel->r.height = ov5675->cur_mode->height;
> +		return 0;
> +	case V4L2_SEL_TGT_CROP_DEFAULT:
> +		sel->r.top = 16;
> +		sel->r.left = 16;
> +		sel->r.width = supported_modes[0].width;
> +		sel->r.height = supported_modes[0].height;
> +		return 0;
> +	}
> 
> Apart from the fact that TGT_CROP should not report the final image
> size (after binning/digital crop) but the size of the pixel array
> portion processed to obtain the final image.
> 
> 
> +	switch (sel->target) {
> +	case V4L2_SEL_TGT_CROP_BOUNDS:
> +		sel->r.top = 0;
> +		sel->r.left = 0;
> +		sel->r.width = 2624;
> +		sel->r.height = 2000;
> +		return 0;
> +	case V4L2_SEL_TGT_CROP:
> +	case V4L2_SEL_TGT_CROP_DEFAULT:
> +		sel->r.top = 16;
> +		sel->r.left = 16;
> +		sel->r.width = 2592;
> +		sel->r.height = 1944;
> +		return 0;
> +	}
> 
> Let me remind that (in the context of pleasing libcamera requirements
> as Quentin is doing) all targets apart TGT_CROP are only useful to
> report static properties of the camera, which are of no real use
> unless you start actually looking into reading out black pixels etc
> etc.

Got it.
Thanks for clarifications.

Regards,
Tommaso
> 
> 
> > >
> > > Thanks
> > >   j
> > >
> > >
> > > > +	case V4L2_SEL_TGT_CROP_DEFAULT:
> > > > +		sel->r.top = 16;
> > > > +		sel->r.left = 16;
> > > > +		sel->r.width = 2592;
> > > > +		sel->r.height = 1944;
> > > > +		return 0;
> > > > +	}
> > > > +	return -EINVAL;
> > > > +}
> > > > +
> > > >  static int ov5675_enum_mbus_code(struct v4l2_subdev *sd,
> > > >  				 struct v4l2_subdev_state *sd_state,
> > > >  				 struct v4l2_subdev_mbus_code_enum *code)
> > > > @@ -1170,6 +1190,7 @@ static const struct v4l2_subdev_video_ops ov5675_video_ops = {
> > > >  static const struct v4l2_subdev_pad_ops ov5675_pad_ops = {
> > > >  	.set_fmt = ov5675_set_format,
> > > >  	.get_fmt = ov5675_get_format,
> > > > +	.get_selection = ov5675_get_selection,
> > > >  	.enum_mbus_code = ov5675_enum_mbus_code,
> > > >  	.enum_frame_size = ov5675_enum_frame_size,
> > > >  };
> > > > --
> > > > 2.36.1
> > > >
> >
> > --
> > Tommaso Merciai
> > Embedded Linux Engineer
> > tommaso.merciai@amarulasolutions.com
> > __________________________________
> >
> > Amarula Solutions SRL
> > Via Le Canevare 30, 31100 Treviso, Veneto, IT
> > T. +39 042 243 5310
> > info@amarulasolutions.com
> > www.amarulasolutions.com

-- 
Tommaso Merciai
Embedded Linux Engineer
tommaso.merciai@amarulasolutions.com
__________________________________

Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
info@amarulasolutions.com
www.amarulasolutions.com

Re: [PATCH v6 4/4] media: i2c: ov5675: add .get_selection support

[Quentin Schulz]

Jacopo, Tommaso,

On 6/8/22 08:42, Jacopo Mondi wrote:
> Hi
> 
> On Wed, Jun 08, 2022 at 12:04:05AM +0200, Tommaso Merciai wrote:
>> Hi Quentin/Jacopo,
>>
>> On Tue, Jun 07, 2022 at 06:51:36PM +0200, Jacopo Mondi wrote:
>>> Hi Quentin,
>>>
>>> On Tue, Jun 07, 2022 at 05:33:35PM +0200, Quentin Schulz wrote:
>>>> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>>>
>>>> The sensor has 2592*1944 active pixels, surrounded by 16 active dummy
>>>> pixels and there are an additional 24 black rows "at the bottom".
>>>>
>>>>                       [2624]
>>>>          +-----+------------------+-----+
>>>>          |     |     16 dummy     |     |
>>>>          +-----+------------------+-----+
>>>>          |     |                  |     |
>>>>          |     |     [2592]       |     |
>>>>          |     |                  |     |
>>>>          |16   |      valid       | 16  |[2000]
>>>>          |dummy|                  |dummy|
>>>>          |     |            [1944]|     |
>>>>          |     |                  |     |
>>>>          +-----+------------------+-----+
>>>>          |     |     16 dummy     |     |
>>>>          +-----+------------------+-----+
>>>>          |     |  24 black lines  |     |
>>>>          +-----+------------------+-----+
>>>>
>>>> The top-left coordinate is gotten from the registers specified in the
>>>> modes which are identical for both currently supported modes.
>>>>
>>>> There are currently two modes supported by this driver: 2592*1944 and
>>>> 1296*972. The second mode is obtained thanks to subsampling while
>>>> keeping the same field of view (FoV). No cropping involved, hence the
>>>> harcoded values.
>>>>
>>>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>>> ---
>>>>
>>>> v6:
>>>>   - explicit a bit more the commit log around subsampling for lower
>>>>   resolution modes,
>>>>   - (again) fixed reporting for V4L2_SEL_TGT_CROP_* thanks to Jacopo's help,
>>>>
>>>> v4:
>>>>   - explicit a bit more the commit log,
>>>>   - added drawing in the commit log,
>>>>   - fixed reporting for V4L2_SEL_TGT_CROP_* thanks to Jacopo's help,
>>>>
>>>> added in v3
>>>>
>>>>   drivers/media/i2c/ov5675.c | 21 +++++++++++++++++++++
>>>>   1 file changed, 21 insertions(+)
>>>>
>>>> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
>>>> index 80840ad7bbb0..2230ff47ef49 100644
>>>> --- a/drivers/media/i2c/ov5675.c
>>>> +++ b/drivers/media/i2c/ov5675.c
>>>> @@ -1121,6 +1121,26 @@ static int ov5675_get_format(struct v4l2_subdev *sd,
>>>>   	return 0;
>>>>   }
>>>>
>>>> +static int ov5675_get_selection(struct v4l2_subdev *sd,
>>>> +				struct v4l2_subdev_state *state,
>>>> +				struct v4l2_subdev_selection *sel)
>>>> +{
>>>> +	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
>>>> +		return -EINVAL;
>>>> +
>>>> +	switch (sel->target) {
>>>> +	case V4L2_SEL_TGT_CROP:
>>>> +	case V4L2_SEL_TGT_CROP_BOUNDS:
>>>
>>> Seem like we have trouble understanding each other, or better, I have
>>> troubles explaining myself most probably :)
>>>
>>> If the dummy/black area is readable, this should just be (0, 0, 2624,
>>> 2000) like it was in your previous version. What has changed that I
>>> have missed ?
>>

I wouldn't say there's some misunderstanding, it's just super hard to 
figure out how to match what the datasheet says to what the kernel 
wants. Yay to obscure/confusing datasheets \o/

I just did things too quickly, nothing changed. Sorry, will send a v7.

>> Taking as reference drivers/media/i2c/ov5693.c and others,
>> seems ok what Quentin have done from my side.
>>
>> Just one thing: maybe is better to avoid magic numbers with more
>> explicit defines like:
>>
>>   + case V4L2_SEL_TGT_CROP_DEFAULT:
>>   +           sel->r.top = OV5675_ACTIVE_START_TOP;
>>   +           sel->r.left = OV5693_ACTIVE_START_LEFT;
>>   +           sel->r.width = OV5693_ACTIVE_WIDTH;
>>   +           sel->r.height = OV5693_ACTIVE_HEIGHT;
>>

They are hardcoded today but actually depend on what;s set in the 
registers too, which might differ if we add more modes in the future? 
It's anyway auto-magic and it's the only place it's used, so not sure it 
brings much especially since the variable names on the left hand side of 
the operator are pretty self-explanatory (not talking about 
V4L2_SEL_TGT_CROP_* :p)? Not that I'm against it.

Cheers,
Quentin

Re: [PATCH v6 4/4] media: i2c: ov5675: add .get_selection supporty

[Tommaso Merciai]

Hi,

On Wed, Jun 08, 2022 at 03:05:29PM +0200, Quentin Schulz wrote:
> Jacopo, Tommaso,
> 
> On 6/8/22 08:42, Jacopo Mondi wrote:
> > Hi
> > 
> > On Wed, Jun 08, 2022 at 12:04:05AM +0200, Tommaso Merciai wrote:
> > > Hi Quentin/Jacopo,
> > > 
> > > On Tue, Jun 07, 2022 at 06:51:36PM +0200, Jacopo Mondi wrote:
> > > > Hi Quentin,
> > > > 
> > > > On Tue, Jun 07, 2022 at 05:33:35PM +0200, Quentin Schulz wrote:
> > > > > From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > > > > 
> > > > > The sensor has 2592*1944 active pixels, surrounded by 16 active dummy
> > > > > pixels and there are an additional 24 black rows "at the bottom".
> > > > > 
> > > > >                       [2624]
> > > > >          +-----+------------------+-----+
> > > > >          |     |     16 dummy     |     |
> > > > >          +-----+------------------+-----+
> > > > >          |     |                  |     |
> > > > >          |     |     [2592]       |     |
> > > > >          |     |                  |     |
> > > > >          |16   |      valid       | 16  |[2000]
> > > > >          |dummy|                  |dummy|
> > > > >          |     |            [1944]|     |
> > > > >          |     |                  |     |
> > > > >          +-----+------------------+-----+
> > > > >          |     |     16 dummy     |     |
> > > > >          +-----+------------------+-----+
> > > > >          |     |  24 black lines  |     |
> > > > >          +-----+------------------+-----+
> > > > > 
> > > > > The top-left coordinate is gotten from the registers specified in the
> > > > > modes which are identical for both currently supported modes.
> > > > > 
> > > > > There are currently two modes supported by this driver: 2592*1944 and
> > > > > 1296*972. The second mode is obtained thanks to subsampling while
> > > > > keeping the same field of view (FoV). No cropping involved, hence the
> > > > > harcoded values.
> > > > > 
> > > > > Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > > > > ---
> > > > > 
> > > > > v6:
> > > > >   - explicit a bit more the commit log around subsampling for lower
> > > > >   resolution modes,
> > > > >   - (again) fixed reporting for V4L2_SEL_TGT_CROP_* thanks to Jacopo's help,
> > > > > 
> > > > > v4:
> > > > >   - explicit a bit more the commit log,
> > > > >   - added drawing in the commit log,
> > > > >   - fixed reporting for V4L2_SEL_TGT_CROP_* thanks to Jacopo's help,
> > > > > 
> > > > > added in v3
> > > > > 
> > > > >   drivers/media/i2c/ov5675.c | 21 +++++++++++++++++++++
> > > > >   1 file changed, 21 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
> > > > > index 80840ad7bbb0..2230ff47ef49 100644
> > > > > --- a/drivers/media/i2c/ov5675.c
> > > > > +++ b/drivers/media/i2c/ov5675.c
> > > > > @@ -1121,6 +1121,26 @@ static int ov5675_get_format(struct v4l2_subdev *sd,
> > > > >   	return 0;
> > > > >   }
> > > > > 
> > > > > +static int ov5675_get_selection(struct v4l2_subdev *sd,
> > > > > +				struct v4l2_subdev_state *state,
> > > > > +				struct v4l2_subdev_selection *sel)
> > > > > +{
> > > > > +	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	switch (sel->target) {
> > > > > +	case V4L2_SEL_TGT_CROP:
> > > > > +	case V4L2_SEL_TGT_CROP_BOUNDS:
> > > > 
> > > > Seem like we have trouble understanding each other, or better, I have
> > > > troubles explaining myself most probably :)
> > > > 
> > > > If the dummy/black area is readable, this should just be (0, 0, 2624,
> > > > 2000) like it was in your previous version. What has changed that I
> > > > have missed ?
> > > 
> 
> I wouldn't say there's some misunderstanding, it's just super hard to figure
> out how to match what the datasheet says to what the kernel wants. Yay to
> obscure/confusing datasheets \o/

I know your feels :)

> 
> I just did things too quickly, nothing changed. Sorry, will send a v7.
> 
> > > Taking as reference drivers/media/i2c/ov5693.c and others,
> > > seems ok what Quentin have done from my side.
> > > 
> > > Just one thing: maybe is better to avoid magic numbers with more
> > > explicit defines like:
> > > 
> > >   + case V4L2_SEL_TGT_CROP_DEFAULT:
> > >   +           sel->r.top = OV5675_ACTIVE_START_TOP;
> > >   +           sel->r.left = OV5693_ACTIVE_START_LEFT;
> > >   +           sel->r.width = OV5693_ACTIVE_WIDTH;
> > >   +           sel->r.height = OV5693_ACTIVE_HEIGHT;
> > > 
> 
> They are hardcoded today but actually depend on what;s set in the registers
> too, which might differ if we add more modes in the future? It's anyway
> auto-magic and it's the only place it's used, so not sure it brings much
> especially since the variable names on the left hand side of the operator
> are pretty self-explanatory (not talking about V4L2_SEL_TGT_CROP_* :p)? Not
> that I'm against it.

Thanks for the explaination.
You are right.

Regards,
Tommaso

> 
> Cheers,
> Quentin

-- 
Tommaso Merciai
Embedded Linux Engineer
tommaso.merciai@amarulasolutions.com
__________________________________

Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
info@amarulasolutions.com
www.amarulasolutions.com