linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 1/4] media: dt-bindings: ov5675: document YAML binding
@ 2022-05-25 14:58 Quentin Schulz
  2022-05-25 14:58 ` [PATCH v5 2/4] media: ov5675: add device-tree support and support runtime PM Quentin Schulz
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Quentin Schulz @ 2022-05-25 14:58 UTC (permalink / raw)
  Cc: shawnx.tu, mchehab, robh+dt, krzysztof.kozlowski+dt, linux-media,
	devicetree, linux-kernel, foss+kernel, Quentin Schulz,
	Krzysztof Kozlowski

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 0000000000000..f0a48707bed77
--- /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 f468864fd268c..549cdec4faaf3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14551,6 +14551,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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v5 2/4] media: ov5675: add device-tree support and support runtime PM
  2022-05-25 14:58 [PATCH v5 1/4] media: dt-bindings: ov5675: document YAML binding Quentin Schulz
@ 2022-05-25 14:58 ` Quentin Schulz
  2022-05-31 11:04   ` Jacopo Mondi
  2022-05-31 13:16   ` Jacopo Mondi
  2022-05-25 14:58 ` [PATCH v5 3/4] media: i2c: ov5675: parse and register V4L2 device tree properties Quentin Schulz
  2022-05-25 14:58 ` [PATCH v5 4/4] media: i2c: ov5675: add .get_selection support Quentin Schulz
  2 siblings, 2 replies; 10+ messages in thread
From: Quentin Schulz @ 2022-05-25 14:58 UTC (permalink / raw)
  Cc: shawnx.tu, mchehab, robh+dt, krzysztof.kozlowski+dt, linux-media,
	devicetree, linux-kernel, foss+kernel, 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>
---

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

diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
index 82ba9f56baec8..ea801edb8e408 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)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct ov5675 *ov5675 = to_ov5675(sd);
+	/* 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);
+
+	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)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct ov5675 *ov5675 = to_ov5675(sd);
+	u32 delay_us = DIV_ROUND_UP(8192, OV5675_XVCLK_19_2 / 1000 / 1000);
+	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,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 +1291,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 +1345,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 +1359,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 +1379,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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v5 3/4] media: i2c: ov5675: parse and register V4L2 device tree properties
  2022-05-25 14:58 [PATCH v5 1/4] media: dt-bindings: ov5675: document YAML binding Quentin Schulz
  2022-05-25 14:58 ` [PATCH v5 2/4] media: ov5675: add device-tree support and support runtime PM Quentin Schulz
@ 2022-05-25 14:58 ` Quentin Schulz
  2022-05-25 14:58 ` [PATCH v5 4/4] media: i2c: ov5675: add .get_selection support Quentin Schulz
  2 siblings, 0 replies; 10+ messages in thread
From: Quentin Schulz @ 2022-05-25 14:58 UTC (permalink / raw)
  Cc: shawnx.tu, mchehab, robh+dt, krzysztof.kozlowski+dt, linux-media,
	devicetree, linux-kernel, foss+kernel, Quentin Schulz,
	Jacopo Mondi

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 ea801edb8e408..c1f3c387afde0 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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v5 4/4] media: i2c: ov5675: add .get_selection support
  2022-05-25 14:58 [PATCH v5 1/4] media: dt-bindings: ov5675: document YAML binding Quentin Schulz
  2022-05-25 14:58 ` [PATCH v5 2/4] media: ov5675: add device-tree support and support runtime PM Quentin Schulz
  2022-05-25 14:58 ` [PATCH v5 3/4] media: i2c: ov5675: parse and register V4L2 device tree properties Quentin Schulz
@ 2022-05-25 14:58 ` Quentin Schulz
  2022-05-31 10:50   ` Jacopo Mondi
  2 siblings, 1 reply; 10+ messages in thread
From: Quentin Schulz @ 2022-05-25 14:58 UTC (permalink / raw)
  Cc: shawnx.tu, mchehab, robh+dt, krzysztof.kozlowski+dt, linux-media,
	devicetree, linux-kernel, foss+kernel, 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.

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

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

diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
index c1f3c387afde0..384a9ea2372c3 100644
--- a/drivers/media/i2c/ov5675.c
+++ b/drivers/media/i2c/ov5675.c
@@ -1121,6 +1121,38 @@ 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)
+{
+	struct ov5675 *ov5675 = to_ov5675(sd);
+
+	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:
+		sel->r.top = 16;
+		sel->r.left = 16;
+		sel->r.width = ov5675->cur_mode->width;
+		sel->r.height = ov5675->cur_mode->height;
+		return 0;
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+		sel->r.top = 16;
+		sel->r.left = 16;
+		sel->r.width = supported_modes[0].width;
+		sel->r.height = supported_modes[0].height;
+		return 0;
+	}
+	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 +1202,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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 4/4] media: i2c: ov5675: add .get_selection support
  2022-05-25 14:58 ` [PATCH v5 4/4] media: i2c: ov5675: add .get_selection support Quentin Schulz
@ 2022-05-31 10:50   ` Jacopo Mondi
  2022-05-31 12:19     ` Quentin Schulz
  0 siblings, 1 reply; 10+ messages in thread
From: Jacopo Mondi @ 2022-05-31 10:50 UTC (permalink / raw)
  To: Quentin Schulz
  Cc: shawnx.tu, mchehab, robh+dt, krzysztof.kozlowski+dt, linux-media,
	devicetree, linux-kernel, Quentin Schulz

Hi Quentin

On Wed, May 25, 2022 at 04:58:33PM +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.
>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> ---
>
> 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 | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>
> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
> index c1f3c387afde0..384a9ea2372c3 100644
> --- a/drivers/media/i2c/ov5675.c
> +++ b/drivers/media/i2c/ov5675.c
> @@ -1121,6 +1121,38 @@ 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)
> +{
> +	struct ov5675 *ov5675 = to_ov5675(sd);
> +
> +	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:
> +		sel->r.top = 16;
> +		sel->r.left = 16;
> +		sel->r.width = ov5675->cur_mode->width;
> +		sel->r.height = ov5675->cur_mode->height;
> +		return 0;

I'm afraid this doesn't match exactly my understanding of the
discussion we had.

The driver defines the following modes

/*
 * OV5670 sensor supports following resolutions with full FOV:
 * 4:3  ==> {2592x1944, 1296x972, 648x486}
 * 16:9 ==> {2560x1440, 1280x720, 640x360}
 */
static const struct ov5670_mode supported_modes[] = {
	{
		.width = 2592,
		.height = 1944,
	},
	{
		.width = 1296,
		.height = 972,
	},
	{
		.width = 648,
		.height = 486,
	},
	{
		.width = 2560,
		.height = 1440,
	},
	{
		.width = 1280,
		.height = 720,
	},
	{
		.width = 640,
		.height = 360,
	}
};

The comment says all modes retain the "full FOV", which I assume it
implies they are obtained by sub-sampling and not cropping.

The first three modes (4:3) are indeed obtained by subsampling the
full active pixel array:

        (2592,1944) / 2 = (1296,972) / 2 = (648,486)

The last three are obtained by subsampling a slightly cropped portion
of the pixel array

        (2560,1440) / 2 = (1280,720) / 2 = (640,360)

If you set CROP = cur_mode->[width/height] you will instead report the
visible width/height, which as said it's obtained by subsampling (of a
slightly cropped portion of the pixel array for the last three ones)

The CROP rectangle is then (2592, 1944) for the first three and (2560,
1440) for the last three.

I would add a v4l2_rect to struct ov5670_mode where to record that and
report it here.

> +	case V4L2_SEL_TGT_CROP_DEFAULT:
> +		sel->r.top = 16;
> +		sel->r.left = 16;
> +		sel->r.width = supported_modes[0].width;
> +		sel->r.height = supported_modes[0].height;
> +		return 0;

You could also define these values instead of fishing in the
supported_modes array, to protect against future changes to the array
itself. Up to you.


> +	}
> +	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 +1202,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
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 2/4] media: ov5675: add device-tree support and support runtime PM
  2022-05-25 14:58 ` [PATCH v5 2/4] media: ov5675: add device-tree support and support runtime PM Quentin Schulz
@ 2022-05-31 11:04   ` Jacopo Mondi
  2022-05-31 13:16   ` Jacopo Mondi
  1 sibling, 0 replies; 10+ messages in thread
From: Jacopo Mondi @ 2022-05-31 11:04 UTC (permalink / raw)
  To: Quentin Schulz
  Cc: shawnx.tu, mchehab, robh+dt, krzysztof.kozlowski+dt, linux-media,
	devicetree, linux-kernel, Quentin Schulz

Hi Quentin,

On Wed, May 25, 2022 at 04:58:31PM +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>
> ---
>
> 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 | 149 +++++++++++++++++++++++++++++++------
>  1 file changed, 128 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
> index 82ba9f56baec8..ea801edb8e408 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;

nit: all other variables are declared using a space between the type
and the variable name, not a tab

> +	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)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct ov5675 *ov5675 = to_ov5675(sd);
> +	/* 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);

nit: this can be declared first

> +
> +	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)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct ov5675 *ov5675 = to_ov5675(sd);
> +	u32 delay_us = DIV_ROUND_UP(8192, OV5675_XVCLK_19_2 / 1000 / 1000);

ditto

All minors, the rest looks good to me!

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j


> +	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,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 +1291,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 +1345,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 +1359,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 +1379,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
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 4/4] media: i2c: ov5675: add .get_selection support
  2022-05-31 10:50   ` Jacopo Mondi
@ 2022-05-31 12:19     ` Quentin Schulz
  2022-05-31 12:45       ` Jacopo Mondi
  0 siblings, 1 reply; 10+ messages in thread
From: Quentin Schulz @ 2022-05-31 12:19 UTC (permalink / raw)
  To: Jacopo Mondi, Quentin Schulz
  Cc: shawnx.tu, mchehab, robh+dt, krzysztof.kozlowski+dt, linux-media,
	devicetree, linux-kernel

Hi Jacopo,

On 5/31/22 12:50, Jacopo Mondi wrote:
> Hi Quentin
> 
> On Wed, May 25, 2022 at 04:58:33PM +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.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>> ---
>>
>> 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 | 33 +++++++++++++++++++++++++++++++++
>>   1 file changed, 33 insertions(+)
>>
>> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
>> index c1f3c387afde0..384a9ea2372c3 100644
>> --- a/drivers/media/i2c/ov5675.c
>> +++ b/drivers/media/i2c/ov5675.c
>> @@ -1121,6 +1121,38 @@ 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)
>> +{
>> +	struct ov5675 *ov5675 = to_ov5675(sd);
>> +
>> +	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:
>> +		sel->r.top = 16;
>> +		sel->r.left = 16;
>> +		sel->r.width = ov5675->cur_mode->width;
>> +		sel->r.height = ov5675->cur_mode->height;
>> +		return 0;
> 
> I'm afraid this doesn't match exactly my understanding of the
> discussion we had.
> 
> The driver defines the following modes
> 
> /*
>   * OV5670 sensor supports following resolutions with full FOV:
>   * 4:3  ==> {2592x1944, 1296x972, 648x486}
>   * 16:9 ==> {2560x1440, 1280x720, 640x360}
>   */
> static const struct ov5670_mode supported_modes[] = {
> 	{
> 		.width = 2592,
> 		.height = 1944,
> 	},
> 	{
> 		.width = 1296,
> 		.height = 972,
> 	},
> 	{
> 		.width = 648,
> 		.height = 486,
> 	},
> 	{
> 		.width = 2560,
> 		.height = 1440,
> 	},
> 	{
> 		.width = 1280,
> 		.height = 720,
> 	},
> 	{
> 		.width = 640,
> 		.height = 360,
> 	}
> };
> 
> The comment says all modes retain the "full FOV", which I assume it
> implies they are obtained by sub-sampling and not cropping.
> 
> The first three modes (4:3) are indeed obtained by subsampling the
> full active pixel array:
> 
>          (2592,1944) / 2 = (1296,972) / 2 = (648,486)
> 
> The last three are obtained by subsampling a slightly cropped portion
> of the pixel array
> 
>          (2560,1440) / 2 = (1280,720) / 2 = (640,360)
> 
> If you set CROP = cur_mode->[width/height] you will instead report the
> visible width/height, which as said it's obtained by subsampling (of a
> slightly cropped portion of the pixel array for the last three ones)
> 
> The CROP rectangle is then (2592, 1944) for the first three and (2560,
> 1440) for the last three.
> 
> I would add a v4l2_rect to struct ov5670_mode where to record that and
> report it here.
> 

That makes a lot of sense to me, thanks for your patience and explanations.

FYI, you're looking at the wrong driver (ov5670 vs ov5675; a mistake I 
make every now and then too :) ). However, the datasheet does say that 
"The OV5675 supports a binning mode to provide a lower resolution output 
while maintaining the field of view.[...] The OV5675 supports 2x2 
binning." so I assume we're in the same scenario as you just explained.

Since the OV5675 modes currently supported by the drivers are 4/3 only 
and the smaller size mode a result of subsampling, they both have the 
same CROP rectangle.

>> +	case V4L2_SEL_TGT_CROP_DEFAULT:
>> +		sel->r.top = 16;
>> +		sel->r.left = 16;
>> +		sel->r.width = supported_modes[0].width;
>> +		sel->r.height = supported_modes[0].height;
>> +		return 0;
> 
> You could also define these values instead of fishing in the
> supported_modes array, to protect against future changes to the array
> itself. Up to you.
> 

Since there's no cropping involved in the current modes, I assume we 
could just hardcode the width and height and tackle this limitation 
later, once we add more modes or support for configuring cropping (this 
patch only adds the getter and not the setter).

Cheers,
Quentin

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 4/4] media: i2c: ov5675: add .get_selection support
  2022-05-31 12:19     ` Quentin Schulz
@ 2022-05-31 12:45       ` Jacopo Mondi
  0 siblings, 0 replies; 10+ messages in thread
From: Jacopo Mondi @ 2022-05-31 12:45 UTC (permalink / raw)
  To: Quentin Schulz
  Cc: Quentin Schulz, shawnx.tu, mchehab, robh+dt,
	krzysztof.kozlowski+dt, linux-media, devicetree, linux-kernel

Hi Quentin

On Tue, May 31, 2022 at 02:19:21PM +0200, Quentin Schulz wrote:
> Hi Jacopo,
>
> On 5/31/22 12:50, Jacopo Mondi wrote:
> > Hi Quentin
> >
> > On Wed, May 25, 2022 at 04:58:33PM +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.
> > >
> > > Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > > ---
> > >
> > > 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 | 33 +++++++++++++++++++++++++++++++++
> > >   1 file changed, 33 insertions(+)
> > >
> > > diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
> > > index c1f3c387afde0..384a9ea2372c3 100644
> > > --- a/drivers/media/i2c/ov5675.c
> > > +++ b/drivers/media/i2c/ov5675.c
> > > @@ -1121,6 +1121,38 @@ 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)
> > > +{
> > > +	struct ov5675 *ov5675 = to_ov5675(sd);
> > > +
> > > +	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:
> > > +		sel->r.top = 16;
> > > +		sel->r.left = 16;
> > > +		sel->r.width = ov5675->cur_mode->width;
> > > +		sel->r.height = ov5675->cur_mode->height;
> > > +		return 0;
> >
> > I'm afraid this doesn't match exactly my understanding of the
> > discussion we had.
> >
> > The driver defines the following modes
> >
> > /*
> >   * OV5670 sensor supports following resolutions with full FOV:
> >   * 4:3  ==> {2592x1944, 1296x972, 648x486}
> >   * 16:9 ==> {2560x1440, 1280x720, 640x360}
> >   */
> > static const struct ov5670_mode supported_modes[] = {
> > 	{
> > 		.width = 2592,
> > 		.height = 1944,
> > 	},
> > 	{
> > 		.width = 1296,
> > 		.height = 972,
> > 	},
> > 	{
> > 		.width = 648,
> > 		.height = 486,
> > 	},
> > 	{
> > 		.width = 2560,
> > 		.height = 1440,
> > 	},
> > 	{
> > 		.width = 1280,
> > 		.height = 720,
> > 	},
> > 	{
> > 		.width = 640,
> > 		.height = 360,
> > 	}
> > };
> >
> > The comment says all modes retain the "full FOV", which I assume it
> > implies they are obtained by sub-sampling and not cropping.
> >
> > The first three modes (4:3) are indeed obtained by subsampling the
> > full active pixel array:
> >
> >          (2592,1944) / 2 = (1296,972) / 2 = (648,486)
> >
> > The last three are obtained by subsampling a slightly cropped portion
> > of the pixel array
> >
> >          (2560,1440) / 2 = (1280,720) / 2 = (640,360)
> >
> > If you set CROP = cur_mode->[width/height] you will instead report the
> > visible width/height, which as said it's obtained by subsampling (of a
> > slightly cropped portion of the pixel array for the last three ones)
> >
> > The CROP rectangle is then (2592, 1944) for the first three and (2560,
> > 1440) for the last three.
> >
> > I would add a v4l2_rect to struct ov5670_mode where to record that and
> > report it here.
> >
>
> That makes a lot of sense to me, thanks for your patience and explanations.
>
> FYI, you're looking at the wrong driver (ov5670 vs ov5675; a mistake I make

You know what's depressing ? -I- have a series out for ov5670 :(
I'm so sorry, my brain got short-circuited by that probably...

> every now and then too :) ). However, the datasheet does say that "The
> OV5675 supports a binning mode to provide a lower resolution output while
> maintaining the field of view.[...] The OV5675 supports 2x2 binning." so I
> assume we're in the same scenario as you just explained.
>
> Since the OV5675 modes currently supported by the drivers are 4/3 only and
> the smaller size mode a result of subsampling, they both have the same CROP
> rectangle.
>

Thankfully the comment still applies to ov5675 then, and both modes
have the same (2592, 1944) crop rectangle :)

> > > +	case V4L2_SEL_TGT_CROP_DEFAULT:
> > > +		sel->r.top = 16;
> > > +		sel->r.left = 16;
> > > +		sel->r.width = supported_modes[0].width;
> > > +		sel->r.height = supported_modes[0].height;
> > > +		return 0;
> >
> > You could also define these values instead of fishing in the
> > supported_modes array, to protect against future changes to the array
> > itself. Up to you.
> >
>
> Since there's no cropping involved in the current modes, I assume we could
> just hardcode the width and height and tackle this limitation later, once we
> add more modes or support for configuring cropping (this patch only adds the
> getter and not the setter).

Fine with me!

Sorry again for the slip!

>
> Cheers,
> Quentin

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 2/4] media: ov5675: add device-tree support and support runtime PM
  2022-05-25 14:58 ` [PATCH v5 2/4] media: ov5675: add device-tree support and support runtime PM Quentin Schulz
  2022-05-31 11:04   ` Jacopo Mondi
@ 2022-05-31 13:16   ` Jacopo Mondi
  2022-06-07 14:08     ` Quentin Schulz
  1 sibling, 1 reply; 10+ messages in thread
From: Jacopo Mondi @ 2022-05-31 13:16 UTC (permalink / raw)
  To: Quentin Schulz
  Cc: shawnx.tu, mchehab, robh+dt, krzysztof.kozlowski+dt, linux-media,
	devicetree, linux-kernel, Quentin Schulz

Hi Quentin,
   one more question

On Wed, May 25, 2022 at 04:58:31PM +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>
> ---
>
> 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 | 149 +++++++++++++++++++++++++++++++------
>  1 file changed, 128 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
> index 82ba9f56baec8..ea801edb8e408 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)

Does this (and power_on) require __maybe_unused to avoid a warning
when compiling without CONFIG_PM support ? Have you tried that ?

Thanks
  j

> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct ov5675 *ov5675 = to_ov5675(sd);
> +	/* 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);
> +
> +	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)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct ov5675 *ov5675 = to_ov5675(sd);
> +	u32 delay_us = DIV_ROUND_UP(8192, OV5675_XVCLK_19_2 / 1000 / 1000);
> +	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,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 +1291,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 +1345,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 +1359,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 +1379,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
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 2/4] media: ov5675: add device-tree support and support runtime PM
  2022-05-31 13:16   ` Jacopo Mondi
@ 2022-06-07 14:08     ` Quentin Schulz
  0 siblings, 0 replies; 10+ messages in thread
From: Quentin Schulz @ 2022-06-07 14:08 UTC (permalink / raw)
  To: Jacopo Mondi, Quentin Schulz
  Cc: shawnx.tu, mchehab, robh+dt, krzysztof.kozlowski+dt, linux-media,
	devicetree, linux-kernel

Hi Jacopo,

On 5/31/22 15:16, Jacopo Mondi wrote:
> Hi Quentin,
>     one more question
> 
> On Wed, May 25, 2022 at 04:58:31PM +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>
>> ---
>>
>> 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 | 149 +++++++++++++++++++++++++++++++------
>>   1 file changed, 128 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
>> index 82ba9f56baec8..ea801edb8e408 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)
> 
> Does this (and power_on) require __maybe_unused to avoid a warning
> when compiling without CONFIG_PM support ? Have you tried that ?
> 

It does not, because they are called manually in the probe function and 
its error path (but thanks for the heads up, because I definitely forgot 
about this one).

I received some review outside of this mailing list telling me the error 
path is incorrect for pm_runtime so I'll have a look at it before 
sending a v6.

Cheers,
Quentin

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-06-07 14:08 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-25 14:58 [PATCH v5 1/4] media: dt-bindings: ov5675: document YAML binding Quentin Schulz
2022-05-25 14:58 ` [PATCH v5 2/4] media: ov5675: add device-tree support and support runtime PM Quentin Schulz
2022-05-31 11:04   ` Jacopo Mondi
2022-05-31 13:16   ` Jacopo Mondi
2022-06-07 14:08     ` Quentin Schulz
2022-05-25 14:58 ` [PATCH v5 3/4] media: i2c: ov5675: parse and register V4L2 device tree properties Quentin Schulz
2022-05-25 14:58 ` [PATCH v5 4/4] media: i2c: ov5675: add .get_selection support Quentin Schulz
2022-05-31 10:50   ` Jacopo Mondi
2022-05-31 12:19     ` Quentin Schulz
2022-05-31 12:45       ` Jacopo Mondi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).