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

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

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

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 e8c52d0192a6..c7a2d9d3642e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14550,6 +14550,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.35.3

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

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

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 | 143 +++++++++++++++++++++++++++++++------
 1 file changed, 122 insertions(+), 21 deletions(-)

diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
index 82ba9f56baec..cee380196774 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,50 @@ static int ov5675_set_stream(struct v4l2_subdev *sd, int enable)
 	return ret;
 }
 
+static int ov5675_power_off(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct ov5675 *ov5675 = to_ov5675(sd);
+
+	gpiod_set_value_cansleep(ov5675->reset_gpio, 1);
+	usleep_range(1000, 1200);
+
+	regulator_bulk_disable(OV5675_NUM_SUPPLIES, ov5675->supplies);
+	clk_disable_unprepare(ov5675->xvclk);
+
+	return 0;
+}
+
+static int ov5675_power_on(struct device *dev)
+{
+	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 */
+	usleep_range(2000, 2200);
+
+	gpiod_set_value_cansleep(ov5675->reset_gpio, 0);
+
+	usleep_range(1000, 1200);
+
+	return 0;
+}
+
 static int __maybe_unused ov5675_suspend(struct device *dev)
 {
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
@@ -1106,32 +1165,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 +1273,9 @@ 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);
+
 	return 0;
 }
 
@@ -1195,25 +1285,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 +1339,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 +1353,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 +1373,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.35.3

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

[Jacopo Mondi]

Hi Quentin,

On Mon, May 09, 2022 at 04:32:24PM +0200, Quentin Schulz wrote:
> 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.
>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>

Thanks for addressing all comments on the previous version.

Looks good to me!
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> ---
>
> 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 | 143 +++++++++++++++++++++++++++++++------
>  1 file changed, 122 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
> index 82ba9f56baec..cee380196774 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,50 @@ static int ov5675_set_stream(struct v4l2_subdev *sd, int enable)
>  	return ret;
>  }
>
> +static int ov5675_power_off(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct ov5675 *ov5675 = to_ov5675(sd);
> +
> +	gpiod_set_value_cansleep(ov5675->reset_gpio, 1);
> +	usleep_range(1000, 1200);
> +
> +	regulator_bulk_disable(OV5675_NUM_SUPPLIES, ov5675->supplies);
> +	clk_disable_unprepare(ov5675->xvclk);
> +
> +	return 0;
> +}
> +
> +static int ov5675_power_on(struct device *dev)
> +{
> +	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 */
> +	usleep_range(2000, 2200);
> +
> +	gpiod_set_value_cansleep(ov5675->reset_gpio, 0);
> +
> +	usleep_range(1000, 1200);
> +
> +	return 0;
> +}
> +
>  static int __maybe_unused ov5675_suspend(struct device *dev)
>  {
>  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> @@ -1106,32 +1165,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 +1273,9 @@ 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);
> +
>  	return 0;
>  }
>
> @@ -1195,25 +1285,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 +1339,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 +1353,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 +1373,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.35.3
>

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

[Quentin Schulz]

Hi all,

On 5/10/22 11:46, Jacopo Mondi wrote:
> Hi Quentin,
> 
> On Mon, May 09, 2022 at 04:32:24PM +0200, Quentin Schulz wrote:
>> 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.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> 
> Thanks for addressing all comments on the previous version.
> 
> Looks good to me!

Unfortunately the sensor disagrees :/

For some reasons, the first three power-on + power-off are successful 
(sometimes only the first two) and then the sensor is not working until 
next cold boot. I got lucky when I tested the patch before sending, much 
less now.

I'm looking into it, don't know how long it will take to get the runtime 
PM fixed. Please hold onto those patches (well.. technically patches 3/4 
and 4/4 don't depend on anything from this patch, so feel free to merge 
those once reviewed).

Cheers,
Quentin

> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> Thanks
>    j
> 
>> ---
>>
>> 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 | 143 +++++++++++++++++++++++++++++++------
>>   1 file changed, 122 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
>> index 82ba9f56baec..cee380196774 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,50 @@ static int ov5675_set_stream(struct v4l2_subdev *sd, int enable)
>>   	return ret;
>>   }
>>
>> +static int ov5675_power_off(struct device *dev)
>> +{
>> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>> +	struct ov5675 *ov5675 = to_ov5675(sd);
>> +
>> +	gpiod_set_value_cansleep(ov5675->reset_gpio, 1);
>> +	usleep_range(1000, 1200);
>> +
>> +	regulator_bulk_disable(OV5675_NUM_SUPPLIES, ov5675->supplies);
>> +	clk_disable_unprepare(ov5675->xvclk);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ov5675_power_on(struct device *dev)
>> +{
>> +	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 */
>> +	usleep_range(2000, 2200);
>> +
>> +	gpiod_set_value_cansleep(ov5675->reset_gpio, 0);
>> +
>> +	usleep_range(1000, 1200);
>> +
>> +	return 0;
>> +}
>> +
>>   static int __maybe_unused ov5675_suspend(struct device *dev)
>>   {
>>   	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>> @@ -1106,32 +1165,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 +1273,9 @@ 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);
>> +
>>   	return 0;
>>   }
>>
>> @@ -1195,25 +1285,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 +1339,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 +1353,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 +1373,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.35.3
>>

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

[Quentin Schulz]

Hi all,

On 5/10/22 15:42, Quentin Schulz wrote:
> Hi all,
> 
> On 5/10/22 11:46, Jacopo Mondi wrote:
>> Hi Quentin,
>>
>> On Mon, May 09, 2022 at 04:32:24PM +0200, Quentin Schulz wrote:
>>> 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.
>>>
>>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>
>> Thanks for addressing all comments on the previous version.
>>
>> Looks good to me!
> 
> Unfortunately the sensor disagrees :/
> 
> For some reasons, the first three power-on + power-off are successful 
> (sometimes only the first two) and then the sensor is not working until 
> next cold boot. I got lucky when I tested the patch before sending, much 
> less now. >

For your information, there were actually two issues:

  - the MCLK of our camera sensor is connected to a pin on a Rockchip 
PX30 SoC whose iodomain is not enabled prior to booting the Linux 
kernel. This means that the whole block was not supplied with power 
(something around 100-150mV instead of the expected 1.8V) and the i2c 
transfers during some device probing were broken because the iodomains 
driver could be probed after the OV5675 driver (and also, since it 
persists on CPU reset, a reboot would fix this too since the iodomain 
would have been configured by the preceding boot). We currently handle 
this the hackish way by configuring the iodomain in U-Boot before 
booting the kernel. I have talked with Heiko, the Rockchip maintainer, 
who mentioned there's no way to explicit a dependency to an iodomain in 
Device Tree right now but suggested we expose regulators from the 
iodomain driver. I'll come up with something to fix this properly in the 
kernel in the next few months. This is not related to the camera driver 
in any way but wanted to mention it.

  - the camera sensor was actually faulty. After many attempts at HW 
reworking, looking at the scope, re-reading the datasheet, trying to 
read between the lines, etc... we swapped the camera sensor for a 
fresh-out-of-the-box one and it worked for 30min (then we stopped it) 
with a runtime/suspend every few seconds. I tried with two different 
camera sensors and it worked fine.

Cheers,
Quentin

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

[Quentin Schulz]

Hi Jacopo,

On 5/10/22 11:46, Jacopo Mondi wrote:
> Hi Quentin,
> 
> On Mon, May 09, 2022 at 04:32:24PM +0200, Quentin Schulz wrote:
>> 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.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> 
> Thanks for addressing all comments on the previous version.
> 
> Looks good to me!
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 

I didn't add your Reviewed-by in the v4 because I modified the 
resume/suspend functions, hope this is ok with you.

Thanks for the reviews.

Cheers,
Quentin

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

Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---
 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 cee380196774..5544e1ae444e 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_ctrl_handler *ctrl_hdlr;
+	struct v4l2_fwnode_device_properties props;
 	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.35.3

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

[Jacopo Mondi]

Hi Quentin,

On Mon, May 09, 2022 at 04:32:25PM +0200, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>
> Parse V4L2 device tree properties and register controls for them.
>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> ---
>  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 cee380196774..5544e1ae444e 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_ctrl_handler *ctrl_hdlr;
> +	struct v4l2_fwnode_device_properties props;

You could move the line up and maintain the (in)famous
reverse-xmas-tree ordering in variable declaration.

Nit apart, looks good
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

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

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

As recommended in the SELECTION API documentation, let's put the first
useful active pixel at the top/left corner (0,0).

This window is the default and maximal crop allowed by the sensor.

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

added in v3

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

diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
index 5544e1ae444e..8e3a5bc6c027 100644
--- a/drivers/media/i2c/ov5675.c
+++ b/drivers/media/i2c/ov5675.c
@@ -78,6 +78,9 @@
 #define OV5675_REG_FORMAT1		0x3820
 #define OV5675_REG_FORMAT2		0x373d
 
+#define OV5675_PIXEL_ARRAY_WIDTH	2592U
+#define OV5675_PIXEL_ARRAY_HEIGHT	1944U
+
 #define to_ov5675(_sd)			container_of(_sd, struct ov5675, sd)
 
 static const char * const ov5675_supply_names[] = {
@@ -1115,6 +1118,27 @@ 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_DEFAULT:
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+		/* In HW, top/left corner is actually at (16,16) */
+		sel->r.top = 0;
+		sel->r.left = 0;
+		sel->r.width = OV5675_PIXEL_ARRAY_WIDTH;
+		sel->r.height = OV5675_PIXEL_ARRAY_HEIGHT;
+		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)
@@ -1164,6 +1188,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.35.3

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

[Jacopo Mondi]

Hi Quentin,

On Mon, May 09, 2022 at 04:32:26PM +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".
>
> As recommended in the SELECTION API documentation, let's put the first
> useful active pixel at the top/left corner (0,0).
>
> This window is the default and maximal crop allowed by the sensor.
>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> ---
>
> added in v3
>
>  drivers/media/i2c/ov5675.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
> index 5544e1ae444e..8e3a5bc6c027 100644
> --- a/drivers/media/i2c/ov5675.c
> +++ b/drivers/media/i2c/ov5675.c
> @@ -78,6 +78,9 @@
>  #define OV5675_REG_FORMAT1		0x3820
>  #define OV5675_REG_FORMAT2		0x373d
>
> +#define OV5675_PIXEL_ARRAY_WIDTH	2592U
> +#define OV5675_PIXEL_ARRAY_HEIGHT	1944U
> +
>  #define to_ov5675(_sd)			container_of(_sd, struct ov5675, sd)
>
>  static const char * const ov5675_supply_names[] = {
> @@ -1115,6 +1118,27 @@ 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_DEFAULT:
> +	case V4L2_SEL_TGT_CROP_BOUNDS:
> +		/* In HW, top/left corner is actually at (16,16) */
> +		sel->r.top = 0;
> +		sel->r.left = 0;
> +		sel->r.width = OV5675_PIXEL_ARRAY_WIDTH;
> +		sel->r.height = OV5675_PIXEL_ARRAY_HEIGHT;
> +		return 0;
> +	}

CROP_NATIVE = the full pixel array size = 2592x1944

CROP_BOUNDS = the rectangle that contains all possible crop
              rectangles, aka the readable portion of your pixel array.
              If in your case the sensor can read out dummy and non
              active lines this is == NATIVE

CROP_DEFAULT = the active/valid pixel area. If there are any
               dummy/invalid lines the DEFAULT rectangle should not
               include them

CROP = the portion of the active pixel area cropped to produce the
       final image. You should look into the modes definition and
       inspect what values are programmed in register 0x380x (I don't
       have a datasheet hence I don't know what corresponds to what)

Does this make any sense to you ?

Thanks
  j

> +	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)
> @@ -1164,6 +1188,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.35.3
>

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

[Quentin Schulz]

Hi Jacopo,

On 5/12/22 11:05, Jacopo Mondi wrote:
> Hi Quentin,
> 
> On Mon, May 09, 2022 at 04:32:26PM +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".
>>
>> As recommended in the SELECTION API documentation, let's put the first
>> useful active pixel at the top/left corner (0,0).
>>
>> This window is the default and maximal crop allowed by the sensor.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>> ---
>>
>> added in v3
>>
>>   drivers/media/i2c/ov5675.c | 25 +++++++++++++++++++++++++
>>   1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
>> index 5544e1ae444e..8e3a5bc6c027 100644
>> --- a/drivers/media/i2c/ov5675.c
>> +++ b/drivers/media/i2c/ov5675.c
>> @@ -78,6 +78,9 @@
>>   #define OV5675_REG_FORMAT1		0x3820
>>   #define OV5675_REG_FORMAT2		0x373d
>>
>> +#define OV5675_PIXEL_ARRAY_WIDTH	2592U
>> +#define OV5675_PIXEL_ARRAY_HEIGHT	1944U
>> +
>>   #define to_ov5675(_sd)			container_of(_sd, struct ov5675, sd)
>>
>>   static const char * const ov5675_supply_names[] = {
>> @@ -1115,6 +1118,27 @@ 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_DEFAULT:
>> +	case V4L2_SEL_TGT_CROP_BOUNDS:
>> +		/* In HW, top/left corner is actually at (16,16) */
>> +		sel->r.top = 0;
>> +		sel->r.left = 0;
>> +		sel->r.width = OV5675_PIXEL_ARRAY_WIDTH;
>> +		sel->r.height = OV5675_PIXEL_ARRAY_HEIGHT;
>> +		return 0;
>> +	}
> 
> CROP_NATIVE = the full pixel array size = 2592x1944
> 
> CROP_BOUNDS = the rectangle that contains all possible crop
>                rectangles, aka the readable portion of your pixel array.
>                If in your case the sensor can read out dummy and non
>                active lines this is == NATIVE
> 
> CROP_DEFAULT = the active/valid pixel area. If there are any
>                 dummy/invalid lines the DEFAULT rectangle should not
>                 include them
> 
> CROP = the portion of the active pixel area cropped to produce the
>         final image. You should look into the modes definition and
>         inspect what values are programmed in register 0x380x (I don't
>         have a datasheet hence I don't know what corresponds to what)
> 
> Does this make any sense to you ?
> 

It's difficult to make sense of the datasheet to be honest.

There's a 3296x2480px "full-size" rectangle, then the sensor array 
output area called CROP and configurable through registers 0x380x, then 
another output area called WIN (for window) also configurable through 
registers 0x380x. The WIN area seems to be just a mask applied on top of 
CROP area ("timing is not affected").

On top of that, the schema shows 24 black lines - I assume - 
incorrectly, one after the start of the full-size rectangle, one after 
the end of the full-size rectangle.

Then the sensor array area region in another section explicitly 
specifies the sensor to be 2624x2000px, active dummy pixels (2 16-pixel 
rows and columns) and black lines (1 24-pixel line) included. Which 
makes the actual useful area 2592x1944px.

In the datasheet, the default size for the CROP area is
2624x1952px, offset (0,12) and for WIN it is 2592x1944px, offset (16,4) 
(assumed relative to CROP given the second coordinate).

In the kernel driver though, the 2592x1944px mode configures the CROP 
area to be 2624x1968px offset (0,4) and the WIN area to be 2592x1944px, 
offset (16,13).

The datasheet only ever mentions 2592x1944px as being the max resolution 
of the sensor though the sensor output area documentation and registers 
do not mention such a limitation.

Since we're not modifying the crop area at the moment, CROP_DEFAULT and 
CROP would be the same, which would both be 2592x1944px.

For the two others, I'm not sure. Any clue or hint with the info I just 
gave?

Cheers,
Quentin

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

[Jacopo Mondi]

Hi Quentin

On Tue, May 17, 2022 at 11:25:17AM +0200, Quentin Schulz wrote:
> Hi Jacopo,
>
> On 5/12/22 11:05, Jacopo Mondi wrote:
> > Hi Quentin,
> >
> > On Mon, May 09, 2022 at 04:32:26PM +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".
> > >
> > > As recommended in the SELECTION API documentation, let's put the first
> > > useful active pixel at the top/left corner (0,0).
> > >
> > > This window is the default and maximal crop allowed by the sensor.
> > >
> > > Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > > ---
> > >
> > > added in v3
> > >
> > >   drivers/media/i2c/ov5675.c | 25 +++++++++++++++++++++++++
> > >   1 file changed, 25 insertions(+)
> > >
> > > diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
> > > index 5544e1ae444e..8e3a5bc6c027 100644
> > > --- a/drivers/media/i2c/ov5675.c
> > > +++ b/drivers/media/i2c/ov5675.c
> > > @@ -78,6 +78,9 @@
> > >   #define OV5675_REG_FORMAT1		0x3820
> > >   #define OV5675_REG_FORMAT2		0x373d
> > >
> > > +#define OV5675_PIXEL_ARRAY_WIDTH	2592U
> > > +#define OV5675_PIXEL_ARRAY_HEIGHT	1944U
> > > +
> > >   #define to_ov5675(_sd)			container_of(_sd, struct ov5675, sd)
> > >
> > >   static const char * const ov5675_supply_names[] = {
> > > @@ -1115,6 +1118,27 @@ 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_DEFAULT:
> > > +	case V4L2_SEL_TGT_CROP_BOUNDS:
> > > +		/* In HW, top/left corner is actually at (16,16) */
> > > +		sel->r.top = 0;
> > > +		sel->r.left = 0;
> > > +		sel->r.width = OV5675_PIXEL_ARRAY_WIDTH;
> > > +		sel->r.height = OV5675_PIXEL_ARRAY_HEIGHT;
> > > +		return 0;
> > > +	}
> >
> > CROP_NATIVE = the full pixel array size = 2592x1944
> >
> > CROP_BOUNDS = the rectangle that contains all possible crop
> >                rectangles, aka the readable portion of your pixel array.
> >                If in your case the sensor can read out dummy and non
> >                active lines this is == NATIVE
> >
> > CROP_DEFAULT = the active/valid pixel area. If there are any
> >                 dummy/invalid lines the DEFAULT rectangle should not
> >                 include them
> >
> > CROP = the portion of the active pixel area cropped to produce the
> >         final image. You should look into the modes definition and
> >         inspect what values are programmed in register 0x380x (I don't
> >         have a datasheet hence I don't know what corresponds to what)
> >
> > Does this make any sense to you ?
> >
>
> It's difficult to make sense of the datasheet to be honest.
>

Seems like it is made on purpose, isn't it :)


> There's a 3296x2480px "full-size" rectangle, then the sensor array output

Are there that many blank/invalid lines/cols ?

> area called CROP and configurable through registers 0x380x, then another
> output area called WIN (for window) also configurable through registers
> 0x380x. The WIN area seems to be just a mask applied on top of CROP area
> ("timing is not affected").
>
> On top of that, the schema shows 24 black lines - I assume - incorrectly,
> one after the start of the full-size rectangle, one after the end of the
> full-size rectangle.
>
> Then the sensor array area region in another section explicitly specifies
> the sensor to be 2624x2000px, active dummy pixels (2 16-pixel rows and
> columns) and black lines (1 24-pixel line) included. Which makes the actual
> useful area 2592x1944px.
>
> In the datasheet, the default size for the CROP area is
> 2624x1952px, offset (0,12) and for WIN it is 2592x1944px, offset (16,4)
> (assumed relative to CROP given the second coordinate).
>
> In the kernel driver though, the 2592x1944px mode configures the CROP area
> to be 2624x1968px offset (0,4) and the WIN area to be 2592x1944px, offset
> (16,13).
>
> The datasheet only ever mentions 2592x1944px as being the max resolution of
> the sensor though the sensor output area documentation and registers do not
> mention such a limitation.
>
> Since we're not modifying the crop area at the moment, CROP_DEFAULT and CROP
> would be the same, which would both be 2592x1944px.
>
> For the two others, I'm not sure. Any clue or hint with the info I just
> gave?

Is my intrpretation of the above correct ?

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


Math looks right at least:

        2000 - 16 - 24 - 16 = 1944
        2624 - 16 - 16 = 2592

As V4L2 selection targets are defined in respect to they larger
sourrounding target, if the documentation about the full size array
(3296,2480) doesn't tell you what offset the readable part is, I think
it's fair to define
        - NATIVE == BOUNDS = (0, 0, 2624, 2000)
        - DEFAULT == CROP = (16, 16, 2592, 1944)

Or maybe better not define NATIVE at all.

The skipped rows/cols almost seems to match what the datasheet suggests by
combining the cropping and windowing rectangles top/left offsets (with
2 cols more skipped compared to my understanding)

        crop = (0, 12, 2624, 1952)
        win = (16, 6, 2592, 1944)

        CROP = (16, 18, 2592, 1944)

The driver does that a little differently with:

        crop = (0, 4, 2624, 1968)
        win = (16, 13, 2592, 1944)

        CROP = (16, 17, 2624, 1944)

Which seems a little off as the driver values for the VTS and HTS
register counts from 0 hence it seems to be skipping 17 rows and 18
cols (maybe the driver adjusts when writing the values to registers ?)

If you feel like it you can try to adjust the driver rectangles, but
in my experience subtle regressions might be introduced by moving
those values, hence I would not be too concerned and just report
whatever the driver does. I assume you're doing this in the context of
pleasing libcamera requirements, and as much as I don't like saying
this, if we're 1 or 2 columns off when reporting the CROP rectangle,
it's not a huge issue.

>
> Cheers,
> Quentin

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

[Quentin Schulz]

Hi Jacopo,

On 5/17/22 13:18, Jacopo Mondi wrote:
> Hi Quentin
> 
> On Tue, May 17, 2022 at 11:25:17AM +0200, Quentin Schulz wrote:
>> Hi Jacopo,
>>
>> On 5/12/22 11:05, Jacopo Mondi wrote:
>>> Hi Quentin,
>>>
>>> On Mon, May 09, 2022 at 04:32:26PM +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".
>>>>
>>>> As recommended in the SELECTION API documentation, let's put the first
>>>> useful active pixel at the top/left corner (0,0).
>>>>
>>>> This window is the default and maximal crop allowed by the sensor.
>>>>
>>>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>>> ---
>>>>
>>>> added in v3
>>>>
>>>>    drivers/media/i2c/ov5675.c | 25 +++++++++++++++++++++++++
>>>>    1 file changed, 25 insertions(+)
>>>>
>>>> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
>>>> index 5544e1ae444e..8e3a5bc6c027 100644
>>>> --- a/drivers/media/i2c/ov5675.c
>>>> +++ b/drivers/media/i2c/ov5675.c
>>>> @@ -78,6 +78,9 @@
>>>>    #define OV5675_REG_FORMAT1		0x3820
>>>>    #define OV5675_REG_FORMAT2		0x373d
>>>>
>>>> +#define OV5675_PIXEL_ARRAY_WIDTH	2592U
>>>> +#define OV5675_PIXEL_ARRAY_HEIGHT	1944U
>>>> +
>>>>    #define to_ov5675(_sd)			container_of(_sd, struct ov5675, sd)
>>>>
>>>>    static const char * const ov5675_supply_names[] = {
>>>> @@ -1115,6 +1118,27 @@ 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_DEFAULT:
>>>> +	case V4L2_SEL_TGT_CROP_BOUNDS:
>>>> +		/* In HW, top/left corner is actually at (16,16) */
>>>> +		sel->r.top = 0;
>>>> +		sel->r.left = 0;
>>>> +		sel->r.width = OV5675_PIXEL_ARRAY_WIDTH;
>>>> +		sel->r.height = OV5675_PIXEL_ARRAY_HEIGHT;
>>>> +		return 0;
>>>> +	}
>>>
>>> CROP_NATIVE = the full pixel array size = 2592x1944
>>>
>>> CROP_BOUNDS = the rectangle that contains all possible crop
>>>                 rectangles, aka the readable portion of your pixel array.
>>>                 If in your case the sensor can read out dummy and non
>>>                 active lines this is == NATIVE
>>>
>>> CROP_DEFAULT = the active/valid pixel area. If there are any
>>>                  dummy/invalid lines the DEFAULT rectangle should not
>>>                  include them
>>>
>>> CROP = the portion of the active pixel area cropped to produce the
>>>          final image. You should look into the modes definition and
>>>          inspect what values are programmed in register 0x380x (I don't
>>>          have a datasheet hence I don't know what corresponds to what)
>>>
>>> Does this make any sense to you ?
>>>
>>
>> It's difficult to make sense of the datasheet to be honest.
>>
> 
> Seems like it is made on purpose, isn't it :)
> 
> 
>> There's a 3296x2480px "full-size" rectangle, then the sensor array output
> 
> Are there that many blank/invalid lines/cols ?
> 

Seems very unlikely, but it's in the schema..

Can't rule out that they just reused the same schema they had for a 
bigger sensor and just didn't update the sizes for the full-size rectangle..

>> area called CROP and configurable through registers 0x380x, then another
>> output area called WIN (for window) also configurable through registers
>> 0x380x. The WIN area seems to be just a mask applied on top of CROP area
>> ("timing is not affected").
>>
>> On top of that, the schema shows 24 black lines - I assume - incorrectly,
>> one after the start of the full-size rectangle, one after the end of the
>> full-size rectangle.
>>
>> Then the sensor array area region in another section explicitly specifies
>> the sensor to be 2624x2000px, active dummy pixels (2 16-pixel rows and
>> columns) and black lines (1 24-pixel line) included. Which makes the actual
>> useful area 2592x1944px.
>>
>> In the datasheet, the default size for the CROP area is
>> 2624x1952px, offset (0,12) and for WIN it is 2592x1944px, offset (16,4)
>> (assumed relative to CROP given the second coordinate).
>>
>> In the kernel driver though, the 2592x1944px mode configures the CROP area
>> to be 2624x1968px offset (0,4) and the WIN area to be 2592x1944px, offset
>> (16,13).
>>
>> The datasheet only ever mentions 2592x1944px as being the max resolution of
>> the sensor though the sensor output area documentation and registers do not
>> mention such a limitation.
>>
>> Since we're not modifying the crop area at the moment, CROP_DEFAULT and CROP
>> would be the same, which would both be 2592x1944px.
>>
>> For the two others, I'm not sure. Any clue or hint with the info I just
>> gave?
> 
> Is my intrpretation of the above correct ?
> 
>                       [2624]
>          +------------------------------+
>          |     |     16 dummy     |     |
>          |------------------------------|
>          |     |                  |     |
>          |     |     [2592]       |     |
>          |     |                  |     |
>          |16   |      valid       | 16  |[2000]
>          |dummy|                  |dummy|
>          |     |            [1944]|     |
>          |     |                  |     |
>          |------------------------------|
>          |     |     24 blacks    |     |
>          |------------------------------|
>          |     |     16 dummy     |     |
>          -------------------------------|
> 

the 24 black lines and 16 dummy pixels rows at the bottom are swapped, 
but otherwise yes.

BTW, did you use a specific tool to make this schema?

> 
> Math looks right at least:
> 
>          2000 - 16 - 24 - 16 = 1944
>          2624 - 16 - 16 = 2592
> 
> As V4L2 selection targets are defined in respect to they larger
> sourrounding target, if the documentation about the full size array
> (3296,2480) doesn't tell you what offset the readable part is, I think
> it's fair to define
>          - NATIVE == BOUNDS = (0, 0, 2624, 2000)
>          - DEFAULT == CROP = (16, 16, 2592, 1944)
> 
> Or maybe better not define NATIVE at all.
> 
> The skipped rows/cols almost seems to match what the datasheet suggests by
> combining the cropping and windowing rectangles top/left offsets (with
> 2 cols more skipped compared to my understanding)
> 
>          crop = (0, 12, 2624, 1952)
>          win = (16, 6, 2592, 1944)
> 
>          CROP = (16, 18, 2592, 1944)
> 
> The driver does that a little differently with:
> 
>          crop = (0, 4, 2624, 1968)
>          win = (16, 13, 2592, 1944)
> 
>          CROP = (16, 17, 2624, 1944)
> 
> Which seems a little off as the driver values for the VTS and HTS
> register counts from 0 hence it seems to be skipping 17 rows and 18
> cols (maybe the driver adjusts when writing the values to registers ?)
> 

Datasheet states that whatever the window, it'll reuse the timings of 
the crop ("Windowing is achieved by masking off the pixels outside of 
the window; thus, the original timing is not affected."). So I assume 
what matters it that the timings are right for the crop.

> If you feel like it you can try to adjust the driver rectangles, but
> in my experience subtle regressions might be introduced by moving
> those values, hence I would not be too concerned and just report
> whatever the driver does. I assume you're doing this in the context of
> pleasing libcamera requirements, and as much as I don't like saying
> this, if we're 1 or 2 columns off when reporting the CROP rectangle,
> it's not a huge issue.
> 

Yes, this patch is for satisfying libcamera requirements.

I also just saw that we actually support two modes for the sensor. So 
I'll need to get the width and height of the currently selected 
supported_modes. Fortunately, the window+crop offsets seem identical so 
there's no need to add some logic for those.

I'll resend patch 3 and 4 separately so they can be merged and I'll 
continue fighting with runtime PM on our camera sensor, looking into HW 
if something's off there.

Cheers,
Quentin

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

[Jacopo Mondi]

Hi Quentin,

On Tue, May 17, 2022 at 02:47:06PM +0200, Quentin Schulz wrote:
> Hi Jacopo,
>
> On 5/17/22 13:18, Jacopo Mondi wrote:
> > Hi Quentin
> >
> > On Tue, May 17, 2022 at 11:25:17AM +0200, Quentin Schulz wrote:
> > > Hi Jacopo,
> > >
> > > On 5/12/22 11:05, Jacopo Mondi wrote:
> > > > Hi Quentin,
> > > >
> > > > On Mon, May 09, 2022 at 04:32:26PM +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".
> > > > >
> > > > > As recommended in the SELECTION API documentation, let's put the first
> > > > > useful active pixel at the top/left corner (0,0).
> > > > >
> > > > > This window is the default and maximal crop allowed by the sensor.
> > > > >
> > > > > Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > > > > ---
> > > > >
> > > > > added in v3
> > > > >
> > > > >    drivers/media/i2c/ov5675.c | 25 +++++++++++++++++++++++++
> > > > >    1 file changed, 25 insertions(+)
> > > > >
> > > > > diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
> > > > > index 5544e1ae444e..8e3a5bc6c027 100644
> > > > > --- a/drivers/media/i2c/ov5675.c
> > > > > +++ b/drivers/media/i2c/ov5675.c
> > > > > @@ -78,6 +78,9 @@
> > > > >    #define OV5675_REG_FORMAT1		0x3820
> > > > >    #define OV5675_REG_FORMAT2		0x373d
> > > > >
> > > > > +#define OV5675_PIXEL_ARRAY_WIDTH	2592U
> > > > > +#define OV5675_PIXEL_ARRAY_HEIGHT	1944U
> > > > > +
> > > > >    #define to_ov5675(_sd)			container_of(_sd, struct ov5675, sd)
> > > > >
> > > > >    static const char * const ov5675_supply_names[] = {
> > > > > @@ -1115,6 +1118,27 @@ 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_DEFAULT:
> > > > > +	case V4L2_SEL_TGT_CROP_BOUNDS:
> > > > > +		/* In HW, top/left corner is actually at (16,16) */
> > > > > +		sel->r.top = 0;
> > > > > +		sel->r.left = 0;
> > > > > +		sel->r.width = OV5675_PIXEL_ARRAY_WIDTH;
> > > > > +		sel->r.height = OV5675_PIXEL_ARRAY_HEIGHT;
> > > > > +		return 0;
> > > > > +	}
> > > >
> > > > CROP_NATIVE = the full pixel array size = 2592x1944
> > > >
> > > > CROP_BOUNDS = the rectangle that contains all possible crop
> > > >                 rectangles, aka the readable portion of your pixel array.
> > > >                 If in your case the sensor can read out dummy and non
> > > >                 active lines this is == NATIVE
> > > >
> > > > CROP_DEFAULT = the active/valid pixel area. If there are any
> > > >                  dummy/invalid lines the DEFAULT rectangle should not
> > > >                  include them
> > > >
> > > > CROP = the portion of the active pixel area cropped to produce the
> > > >          final image. You should look into the modes definition and
> > > >          inspect what values are programmed in register 0x380x (I don't
> > > >          have a datasheet hence I don't know what corresponds to what)
> > > >
> > > > Does this make any sense to you ?
> > > >
> > >
> > > It's difficult to make sense of the datasheet to be honest.
> > >
> >
> > Seems like it is made on purpose, isn't it :)
> >
> >
> > > There's a 3296x2480px "full-size" rectangle, then the sensor array output
> >
> > Are there that many blank/invalid lines/cols ?
> >
>
> Seems very unlikely, but it's in the schema..
>
> Can't rule out that they just reused the same schema they had for a bigger
> sensor and just didn't update the sizes for the full-size rectangle..
>
> > > area called CROP and configurable through registers 0x380x, then another
> > > output area called WIN (for window) also configurable through registers
> > > 0x380x. The WIN area seems to be just a mask applied on top of CROP area
> > > ("timing is not affected").
> > >
> > > On top of that, the schema shows 24 black lines - I assume - incorrectly,
> > > one after the start of the full-size rectangle, one after the end of the
> > > full-size rectangle.
> > >
> > > Then the sensor array area region in another section explicitly specifies
> > > the sensor to be 2624x2000px, active dummy pixels (2 16-pixel rows and
> > > columns) and black lines (1 24-pixel line) included. Which makes the actual
> > > useful area 2592x1944px.
> > >
> > > In the datasheet, the default size for the CROP area is
> > > 2624x1952px, offset (0,12) and for WIN it is 2592x1944px, offset (16,4)
> > > (assumed relative to CROP given the second coordinate).
> > >
> > > In the kernel driver though, the 2592x1944px mode configures the CROP area
> > > to be 2624x1968px offset (0,4) and the WIN area to be 2592x1944px, offset
> > > (16,13).
> > >
> > > The datasheet only ever mentions 2592x1944px as being the max resolution of
> > > the sensor though the sensor output area documentation and registers do not
> > > mention such a limitation.
> > >
> > > Since we're not modifying the crop area at the moment, CROP_DEFAULT and CROP
> > > would be the same, which would both be 2592x1944px.
> > >
> > > For the two others, I'm not sure. Any clue or hint with the info I just
> > > gave?
> >
> > Is my intrpretation of the above correct ?
> >
> >                       [2624]
> >          +------------------------------+
> >          |     |     16 dummy     |     |
> >          |------------------------------|
> >          |     |                  |     |
> >          |     |     [2592]       |     |
> >          |     |                  |     |
> >          |16   |      valid       | 16  |[2000]
> >          |dummy|                  |dummy|
> >          |     |            [1944]|     |
> >          |     |                  |     |
> >          |------------------------------|
> >          |     |     24 blacks    |     |
> >          |------------------------------|
> >          |     |     16 dummy     |     |
> >          -------------------------------|
> >
>
> the 24 black lines and 16 dummy pixels rows at the bottom are swapped, but
> otherwise yes.
>
> BTW, did you use a specific tool to make this schema?
>

Yes, a keyboard

:p

> >
> > Math looks right at least:
> >
> >          2000 - 16 - 24 - 16 = 1944
> >          2624 - 16 - 16 = 2592
> >
> > As V4L2 selection targets are defined in respect to they larger
> > sourrounding target, if the documentation about the full size array
> > (3296,2480) doesn't tell you what offset the readable part is, I think
> > it's fair to define
> >          - NATIVE == BOUNDS = (0, 0, 2624, 2000)
> >          - DEFAULT == CROP = (16, 16, 2592, 1944)
> >
> > Or maybe better not define NATIVE at all.
> >
> > The skipped rows/cols almost seems to match what the datasheet suggests by
> > combining the cropping and windowing rectangles top/left offsets (with
> > 2 cols more skipped compared to my understanding)
> >
> >          crop = (0, 12, 2624, 1952)
> >          win = (16, 6, 2592, 1944)
> >
> >          CROP = (16, 18, 2592, 1944)
> >
> > The driver does that a little differently with:
> >
> >          crop = (0, 4, 2624, 1968)
> >          win = (16, 13, 2592, 1944)
> >
> >          CROP = (16, 17, 2624, 1944)
> >
> > Which seems a little off as the driver values for the VTS and HTS
> > register counts from 0 hence it seems to be skipping 17 rows and 18
> > cols (maybe the driver adjusts when writing the values to registers ?)
> >
>
> Datasheet states that whatever the window, it'll reuse the timings of the
> crop ("Windowing is achieved by masking off the pixels outside of the
> window; thus, the original timing is not affected."). So I assume what
> matters it that the timings are right for the crop.
>

I assume that the "timings" the datasheet refers means the pixel array
sampling rate, which depends on the "crop" rectangle that define
the portion of the pixel array fed to the on-board ISP for processing
(mostly subsampling, as this seems to be a raw sensor ?)

The output frame rate will depend instead on the bus link frequency
and the total image size including blankings (usually configured
through the registers named VTS/HTS on OV sensors?)

> > If you feel like it you can try to adjust the driver rectangles, but
> > in my experience subtle regressions might be introduced by moving
> > those values, hence I would not be too concerned and just report
> > whatever the driver does. I assume you're doing this in the context of
> > pleasing libcamera requirements, and as much as I don't like saying
> > this, if we're 1 or 2 columns off when reporting the CROP rectangle,
> > it's not a huge issue.
> >
>
> Yes, this patch is for satisfying libcamera requirements.
>

The most important requirement is to expose VBLANK and PIXEL_RATE as
they're used calculate the sensor's frame rate. Selection targets are
mostly useful to populate the camera properties (ie the full pixel
array size) and to implement digitial zoom, somewhat and "advanced"
feature, hence they're optional to support.

Thanks
  j

> I also just saw that we actually support two modes for the sensor. So I'll
> need to get the width and height of the currently selected supported_modes.
> Fortunately, the window+crop offsets seem identical so there's no need to
> add some logic for those.
>
> I'll resend patch 3 and 4 separately so they can be merged and I'll continue
> fighting with runtime PM on our camera sensor, looking into HW if
> something's off there.
>
> Cheers,
> Quentin

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

[Krzysztof Kozlowski]

On 09/05/2022 16:32, Quentin Schulz wrote:
> 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.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof