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

[PATCH v7 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 v7 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>
---

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

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

[Jacopo Mondi]

Hi Quentin,

On Wed, Jun 08, 2022 at 03:44:18PM +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>
> ---
>

seems like my r-b tag has been dropped from v7.

Any major change or it just slipped ?

Thanks
   j

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

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

[Quentin Schulz]

Hi Jacopo,

On 7/6/22 09:30, Jacopo Mondi wrote:
> Hi Quentin,
> 
> On Wed, Jun 08, 2022 at 03:44:18PM +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>
>> ---
>>
> 
> seems like my r-b tag has been dropped from v7.
> 
> Any major change or it just slipped ?
> 

No change at all, should have been there. Sorry for the oversight.

Cheers,
Quentin

> Thanks
>     j
> 
>> 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://urldefense.proofpoint.com/v2/url?u=https-3A__lists.linuxfoundation.org_pipermail_linux-2Dpm_2012-2DJuly_034705.html&d=DwIBaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=UUADoiN42MXp9pWA9wFQ7vAh8Y0-KlDmQVDA99hi5PBxa7lQPePNq2BuvpIeouvb&s=-NnrgHuWYBKbBeigK_pvYkeHUm1rGSe0l0Gb0HajSUc&e=
>>   - 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 v7 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 v7 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>
---

v7:
 - fixed incorrect V4L2_SEL_TGT_CROP_BOUNDS introduced in v6,

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 | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
index 80840ad7bbb0..5f70ead2dd47 100644
--- a/drivers/media/i2c/ov5675.c
+++ b/drivers/media/i2c/ov5675.c
@@ -1121,6 +1121,31 @@ 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_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;
+	}
+	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 +1195,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 v7 4/4] media: i2c: ov5675: add .get_selection support

[Jacopo Mondi]

Hi Quentin

On Wed, Jun 08, 2022 at 03:44:20PM +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>

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

Thanks
   j

> ---
>
> v7:
>  - fixed incorrect V4L2_SEL_TGT_CROP_BOUNDS introduced in v6,
>
> 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 | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
> index 80840ad7bbb0..5f70ead2dd47 100644
> --- a/drivers/media/i2c/ov5675.c
> +++ b/drivers/media/i2c/ov5675.c
> @@ -1121,6 +1121,31 @@ 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_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;
> +	}
> +	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 +1195,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 v7 1/4] media: dt-bindings: ov5675: document YAML binding

[Quentin Schulz]

Hi all,

Any additional feedback to give on the series?

Thanks,
Quentin

On 6/8/22 15:44, 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.
> 
> 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: https://urldefense.proofpoint.com/v2/url?u=http-3A__devicetree.org_schemas_media_i2c_ovti-2Cov5675.yaml-23&d=DwIDAg&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=hkLsYZWMEhpLI6ShhGo0x3Jiuqy6R6W720Ntjy_xiWib19fmlU43ao0fGq1ALlC0&s=dJP3Z1NZNxGh-udGBCMYQKpHV17u1R0DloWk1smrUPE&e=
> +$schema: https://urldefense.proofpoint.com/v2/url?u=http-3A__devicetree.org_meta-2Dschemas_core.yaml-23&d=DwIDAg&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=hkLsYZWMEhpLI6ShhGo0x3Jiuqy6R6W720Ntjy_xiWib19fmlU43ao0fGq1ALlC0&s=hJpvHfydAEL-abkbxj-ZSKxgR5wrNKozJb-oj-TaIfc&e=
> +
> +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