[PATCH v3 0/8] media: i2c: ov5670: OF support, runtime_pm, regulators

[PATCH v3 0/8] media: i2c: ov5670: OF support, runtime_pm, regulators

[Jacopo Mondi]

Hello this small series introduces OF support for the ov5670 sensor and
adds support for regulators and GPIOs.

It also register runtime_pm callbacks and rework the powering sequence (cc
Paul(s) and Sakari for the discussion about the same topic on ov5640)

Tested on an OF system, ACPI should not be affected
(ofc, testing would be nice :)

v2->v3:
- bindings:
  - Drop assigned-clock properties from schema (moved to example)
  - s/pwdn-gpios/powerdown-gpios/

- driver
  - Use is_of_node() to decide how to parse clocks
  - Fix:
    drivers/media/i2c/ov5670.c:1787:18: error: initializer element is not a compile-time constant
                   .analog_crop = ov5670_analog_crop,
                                  ^~~~~~~~~~~~~~~~~~

    reported by kernel test robot and Nathan Chancellor with
    clang15 and gcc < 8

v1->v2:
- Address Krzysztof comments on bindings
- 2/8: new patch to use the common clock framework
- Address Lauren's comment on runtime_pm function names
- 7/8: new patch to implement init_cfg as suggested by Laurent
- Rework 8/8 which was incorrect as reported by Laurent

Thanks
   j

Jacopo Mondi (7):
  media: dt-bindings: i2c: Document ov5670
  media: i2c: ov5670: Allow probing with OF
  media: i2c: ov5670: Probe clocks with OF
  media: i2c: ov5670: Probe regulators
  media: i2c: ov5670: Probe GPIOs
  media: i2c: ov5670: Add runtime_pm operations
  media: i2c: ov5670: Implement init_cfg

Jean-Michel Hautbois (1):
  media: i2c: ov5670: Add .get_selection() support

 .../bindings/media/i2c/ovti,ov5670.yaml       |  99 ++++++
 MAINTAINERS                                   |   1 +
 drivers/media/i2c/ov5670.c                    | 282 +++++++++++++++---
 3 files changed, 341 insertions(+), 41 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml

--
2.35.1

[PATCH v3 1/8] media: dt-bindings: i2c: Document ov5670

[Jacopo Mondi]

Provide the bindings documentation for Omnivision OV5670 image sensor.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 .../bindings/media/i2c/ovti,ov5670.yaml       | 99 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 100 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
new file mode 100644
index 000000000000..9419d19dbfd8
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
@@ -0,0 +1,99 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/ovti,ov5670.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Omnivision OV5670 5 Megapixels raw image sensor
+
+maintainers:
+  - Jacopo Mondi <jacopo@jmondi.org>
+
+description: |-
+  The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits
+  RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is
+  controlled through an I2C compatible control bus.
+
+properties:
+  compatible:
+    const: ovti,ov5670
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    description: System clock. From 6 to 27 MHz.
+    maxItems: 1
+
+  powerdown-gpios:
+    description: Reference to the GPIO connected to the PWDNB pin. Active low.
+    maxItems: 1
+
+  reset-gpios:
+    description:
+      Reference to the GPIO connected to the XSHUTDOWN pin. Active low.
+    maxItems: 1
+
+  avdd-supply:
+    description: Analog circuit power. Typically 2.8V.
+
+  dvdd-supply:
+    description: Digital circuit power. Typically 1.2V.
+
+  dovdd-supply:
+    description: Digital I/O circuit power. Typically 2.8V or 1.8V.
+
+  port:
+    $ref: /schemas/graph.yaml#/$defs/port-base
+    additionalProperties: false
+
+    properties:
+      endpoint:
+        $ref: /schemas/media/video-interfaces.yaml#
+        unevaluatedProperties: false
+
+        properties:
+          data-lanes:
+            description: The sensor supports 1 or 2 data lanes operations.
+            minItems: 1
+            maxItems: 2
+            items:
+              enum: [1, 2]
+
+          clock-noncontinuous: true
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ov5670: sensor@36 {
+            compatible = "ovti,ov5670";
+            reg = <0x36>;
+
+            clocks = <&sensor_xclk>;
+            assigned-clocks = <&sensor_xclk>, <&pll2>;
+            assigned-clock-parents = <&pll2>;
+            assigned-clock-rates = <19200000>;
+
+            port {
+                endpoint {
+                    remote-endpoint = <&csi_ep>;
+                    data-lanes = <1 2>;
+                    clock-noncontinuous;
+                };
+            };
+        };
+    };
+
+...
+
diff --git a/MAINTAINERS b/MAINTAINERS
index 49d1e43f4a9d..700a8ceca564 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14282,6 +14282,7 @@ M:	Chiranjeevi Rapolu <chiranjeevi.rapolu@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,ov5670.yaml
 F:	drivers/media/i2c/ov5670.c
 
 OMNIVISION OV5675 SENSOR DRIVER
-- 
2.35.1

[PATCH v3 2/8] media: i2c: ov5670: Allow probing with OF

[Jacopo Mondi]

The ov5670 driver currently only supports probing using ACPI matching.
Add support for OF and add a missing header inclusion.

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

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 02f75c18e480..721441024598 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -3,7 +3,9 @@
 
 #include <linux/acpi.h>
 #include <linux/i2c.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/pm_runtime.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
@@ -2585,11 +2587,20 @@ static const struct acpi_device_id ov5670_acpi_ids[] = {
 MODULE_DEVICE_TABLE(acpi, ov5670_acpi_ids);
 #endif
 
+#ifdef CONFIG_OF
+static const struct of_device_id ov5670_of_ids[] = {
+	{ .compatible = "ovti,ov5670" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ov5670_of_ids);
+#endif
+
 static struct i2c_driver ov5670_i2c_driver = {
 	.driver = {
 		.name = "ov5670",
 		.pm = &ov5670_pm_ops,
 		.acpi_match_table = ACPI_PTR(ov5670_acpi_ids),
+		.of_match_table = of_match_ptr(ov5670_of_ids),
 	},
 	.probe_new = ov5670_probe,
 	.remove = ov5670_remove,
-- 
2.35.1

[PATCH v3 3/8] media: i2c: ov5670: Probe clocks with OF

[Jacopo Mondi]

Add support for probing the main system clock using the common clock
framework and its OF bindings.

Maintain ACPI compatibility by falling back to parse 'clock-frequency'.

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

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 721441024598..1cc312981c82 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -2,6 +2,7 @@
 // Copyright (c) 2017 Intel Corporation.
 
 #include <linux/acpi.h>
+#include <linux/clk.h>
 #include <linux/i2c.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
@@ -2475,13 +2476,10 @@ static int ov5670_probe(struct i2c_client *client)
 	struct ov5670 *ov5670;
 	const char *err_msg;
 	u32 input_clk = 0;
+	struct clk *clk;
 	bool full_power;
 	int ret;
 
-	device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
-	if (input_clk != 19200000)
-		return -EINVAL;
-
 	ov5670 = devm_kzalloc(&client->dev, sizeof(*ov5670), GFP_KERNEL);
 	if (!ov5670) {
 		ret = -ENOMEM;
@@ -2489,6 +2487,22 @@ static int ov5670_probe(struct i2c_client *client)
 		goto error_print;
 	}
 
+	/* OF uses the common clock framework, ACPI uses "clock-frequency". */
+	if (is_of_node(dev_fwnode(&client->dev))) {
+		clk = devm_clk_get(&client->dev, NULL);
+		if (IS_ERR(clk))
+			return dev_err_probe(&client->dev, PTR_ERR(clk),
+					     "error getting clock\n");
+
+		input_clk = clk_get_rate(clk);
+	} else {
+		device_property_read_u32(&client->dev, "clock-frequency",
+					 &input_clk);
+	}
+
+	if (input_clk != 19200000)
+		return -EINVAL;
+
 	/* Initialize subdev */
 	v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops);
 
-- 
2.35.1

[PATCH v3 4/8] media: i2c: ov5670: Probe regulators

[Jacopo Mondi]

The OV5670 has three power supplies (AVDD, DOVDD and DVDD).

Probe them in the driver to prepare controlling with runtime_pm
operations.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/ov5670.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 1cc312981c82..9ddd259a4015 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -8,6 +8,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-event.h>
@@ -86,6 +87,14 @@ struct ov5670_link_freq_config {
 	const struct ov5670_reg_list reg_list;
 };
 
+static const char * const ov5670_supply_names[] = {
+	"avdd",		/* Analog power */
+	"dvdd",		/* Digital power */
+	"dovdd",	/* Digital output power */
+};
+
+#define OV5670_NUM_SUPPLIES ARRAY_SIZE(ov5670_supply_names)
+
 struct ov5670_mode {
 	/* Frame width in pixels */
 	u32 width;
@@ -1831,6 +1840,9 @@ struct ov5670 {
 	/* Current mode */
 	const struct ov5670_mode *cur_mode;
 
+	/* Regulators */
+	struct regulator_bulk_data supplies[OV5670_NUM_SUPPLIES];
+
 	/* To serialize asynchronus callbacks */
 	struct mutex mutex;
 
@@ -2471,6 +2483,18 @@ static const struct v4l2_subdev_internal_ops ov5670_internal_ops = {
 	.open = ov5670_open,
 };
 
+static int ov5670_regulators_probe(struct ov5670 *ov5670)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&ov5670->sd);
+	unsigned int i;
+
+	for (i = 0; i < OV5670_NUM_SUPPLIES; i++)
+		ov5670->supplies[i].supply = ov5670_supply_names[i];
+
+	return devm_regulator_bulk_get(&client->dev, OV5670_NUM_SUPPLIES,
+				       ov5670->supplies);
+}
+
 static int ov5670_probe(struct i2c_client *client)
 {
 	struct ov5670 *ov5670;
@@ -2506,6 +2530,12 @@ static int ov5670_probe(struct i2c_client *client)
 	/* Initialize subdev */
 	v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops);
 
+	ret = ov5670_regulators_probe(ov5670);
+	if (ret) {
+		err_msg = "Regulators probe failed";
+		goto error_print;
+	}
+
 	full_power = acpi_dev_state_d0(&client->dev);
 	if (full_power) {
 		/* Check module identity */
-- 
2.35.1

[PATCH v3 5/8] media: i2c: ov5670: Probe GPIOs

[Jacopo Mondi]

The OV5670 has a powerdown and reset pin, named respectively "PWDN" and
"XSHUTDOWN".

Optionally probe the gpios connected to the pins during the driver probe
routine.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/ov5670.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 9ddd259a4015..9e69b4008917 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -3,6 +3,7 @@
 
 #include <linux/acpi.h>
 #include <linux/clk.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
@@ -1843,6 +1844,10 @@ struct ov5670 {
 	/* Regulators */
 	struct regulator_bulk_data supplies[OV5670_NUM_SUPPLIES];
 
+	/* Power-down and reset gpios. */
+	struct gpio_desc *pwdn_gpio; /* PWDNB pin. */
+	struct gpio_desc *reset_gpio; /* XSHUTDOWN pin. */
+
 	/* To serialize asynchronus callbacks */
 	struct mutex mutex;
 
@@ -2495,6 +2500,23 @@ static int ov5670_regulators_probe(struct ov5670 *ov5670)
 				       ov5670->supplies);
 }
 
+static int ov5670_gpio_probe(struct ov5670 *ov5670)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&ov5670->sd);
+
+	ov5670->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "powerdown",
+						    GPIOD_OUT_LOW);
+	if (IS_ERR(ov5670->pwdn_gpio))
+		return PTR_ERR(ov5670->pwdn_gpio);
+
+	ov5670->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
+						     GPIOD_OUT_LOW);
+	if (IS_ERR(ov5670->reset_gpio))
+		return PTR_ERR(ov5670->reset_gpio);
+
+	return 0;
+}
+
 static int ov5670_probe(struct i2c_client *client)
 {
 	struct ov5670 *ov5670;
@@ -2536,6 +2558,12 @@ static int ov5670_probe(struct i2c_client *client)
 		goto error_print;
 	}
 
+	ret = ov5670_gpio_probe(ov5670);
+	if (ret) {
+		err_msg = "GPIO probe failed";
+		goto error_print;
+	}
+
 	full_power = acpi_dev_state_d0(&client->dev);
 	if (full_power) {
 		/* Check module identity */
-- 
2.35.1

[PATCH v3 6/8] media: i2c: ov5670: Add runtime_pm operations

[Jacopo Mondi]

Implement the power up and power down routines and install them as
runtime_pm handler for runtime_suspend and runtime_resume operations.

Rework the runtime_pm enablement and the chip power handling during
probe, as calling pm_runtime_idle() in a driver that registers no
idle callback is a nop.

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

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 9e69b4008917..b63b07d8ca2f 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -4,6 +4,7 @@
 #include <linux/acpi.h>
 #include <linux/clk.h>
 #include <linux/gpio/consumer.h>
+#include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
@@ -2424,6 +2425,39 @@ static int ov5670_set_stream(struct v4l2_subdev *sd, int enable)
 	return ret;
 }
 
+static int __maybe_unused ov5670_runtime_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct ov5670 *ov5670 = to_ov5670(sd);
+	int ret;
+
+	ret = regulator_bulk_enable(OV5670_NUM_SUPPLIES, ov5670->supplies);
+	if (ret)
+		return ret;
+
+	gpiod_set_value_cansleep(ov5670->pwdn_gpio, 0);
+	gpiod_set_value_cansleep(ov5670->reset_gpio, 0);
+
+	/* 8192 * 2 clock pulses before the first SCCB transaction. */
+	usleep_range(1000, 1500);
+
+	return 0;
+}
+
+static int __maybe_unused ov5670_runtime_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct ov5670 *ov5670 = to_ov5670(sd);
+
+	gpiod_set_value_cansleep(ov5670->reset_gpio, 1);
+	gpiod_set_value_cansleep(ov5670->pwdn_gpio, 1);
+	regulator_bulk_disable(OV5670_NUM_SUPPLIES, ov5670->supplies);
+
+	return 0;
+}
+
 static int __maybe_unused ov5670_suspend(struct device *dev)
 {
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
@@ -2564,14 +2598,25 @@ static int ov5670_probe(struct i2c_client *client)
 		goto error_print;
 	}
 
+	pm_runtime_enable(&client->dev);
+
 	full_power = acpi_dev_state_d0(&client->dev);
 	if (full_power) {
+		ret = pm_runtime_resume_and_get(&client->dev);
+		if (ret) {
+			err_msg = "Failed to power on";
+			goto error_print;
+		}
+
 		/* Check module identity */
 		ret = ov5670_identify_module(ov5670);
 		if (ret) {
 			err_msg = "ov5670_identify_module() error";
-			goto error_print;
+			goto error_power_off;
 		}
+
+		/* Set the device's state to active if it's in D0 state. */
+		pm_runtime_set_active(&client->dev);
 	}
 
 	mutex_init(&ov5670->mutex);
@@ -2608,11 +2653,7 @@ static int ov5670_probe(struct i2c_client *client)
 
 	ov5670->streaming = false;
 
-	/* Set the device's state to active if it's in D0 state. */
-	if (full_power)
-		pm_runtime_set_active(&client->dev);
-	pm_runtime_enable(&client->dev);
-	pm_runtime_idle(&client->dev);
+	pm_runtime_suspend(&client->dev);
 
 	return 0;
 
@@ -2625,6 +2666,9 @@ static int ov5670_probe(struct i2c_client *client)
 error_mutex_destroy:
 	mutex_destroy(&ov5670->mutex);
 
+error_power_off:
+	pm_runtime_put(&client->dev);
+
 error_print:
 	dev_err(&client->dev, "%s: %s %d\n", __func__, err_msg, ret);
 
@@ -2641,6 +2685,7 @@ static int ov5670_remove(struct i2c_client *client)
 	v4l2_ctrl_handler_free(sd->ctrl_handler);
 	mutex_destroy(&ov5670->mutex);
 
+	pm_runtime_put(&client->dev);
 	pm_runtime_disable(&client->dev);
 
 	return 0;
@@ -2648,6 +2693,7 @@ static int ov5670_remove(struct i2c_client *client)
 
 static const struct dev_pm_ops ov5670_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(ov5670_suspend, ov5670_resume)
+	SET_RUNTIME_PM_OPS(ov5670_runtime_suspend, ov5670_runtime_resume, NULL)
 };
 
 #ifdef CONFIG_ACPI
-- 
2.35.1

[PATCH v3 7/8] media: i2c: ov5670: Implement init_cfg

[Jacopo Mondi]

Implement the .init_cfg() pad operation and initialize the default
format.

With .init_cfg() pad operation implemented the deprecated .open()
internal operation can now be dropped.

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

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index b63b07d8ca2f..7f12b053cb0e 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -1956,27 +1956,6 @@ static int ov5670_write_reg_list(struct ov5670 *ov5670,
 	return ov5670_write_regs(ov5670, r_list->regs, r_list->num_of_regs);
 }
 
-/* Open sub-device */
-static int ov5670_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
-{
-	struct ov5670 *ov5670 = to_ov5670(sd);
-	struct v4l2_mbus_framefmt *try_fmt =
-				v4l2_subdev_get_try_format(sd, fh->state, 0);
-
-	mutex_lock(&ov5670->mutex);
-
-	/* Initialize try_fmt */
-	try_fmt->width = ov5670->cur_mode->width;
-	try_fmt->height = ov5670->cur_mode->height;
-	try_fmt->code = MEDIA_BUS_FMT_SGRBG10_1X10;
-	try_fmt->field = V4L2_FIELD_NONE;
-
-	/* No crop or compose */
-	mutex_unlock(&ov5670->mutex);
-
-	return 0;
-}
-
 static int ov5670_update_digital_gain(struct ov5670 *ov5670, u32 d_gain)
 {
 	int ret;
@@ -2176,6 +2155,25 @@ static int ov5670_init_controls(struct ov5670 *ov5670)
 	return ret;
 }
 
+static int ov5670_init_cfg(struct v4l2_subdev *sd,
+			   struct v4l2_subdev_state *state)
+{
+	struct v4l2_mbus_framefmt *fmt =
+				v4l2_subdev_get_try_format(sd, state, 0);
+	const struct ov5670_mode *default_mode = &supported_modes[0];
+
+	fmt->width = default_mode->width;
+	fmt->height = default_mode->height;
+	fmt->code = MEDIA_BUS_FMT_SGRBG10_1X10;
+	fmt->field = V4L2_FIELD_NONE;
+	fmt->colorspace = V4L2_COLORSPACE_SRGB;
+	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(V4L2_COLORSPACE_SRGB);
+	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
+	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(V4L2_COLORSPACE_SRGB);
+
+	return 0;
+}
+
 static int ov5670_enum_mbus_code(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *sd_state,
 				 struct v4l2_subdev_mbus_code_enum *code)
@@ -2497,6 +2495,7 @@ static const struct v4l2_subdev_video_ops ov5670_video_ops = {
 };
 
 static const struct v4l2_subdev_pad_ops ov5670_pad_ops = {
+	.init_cfg = ov5670_init_cfg,
 	.enum_mbus_code = ov5670_enum_mbus_code,
 	.get_fmt = ov5670_get_pad_format,
 	.set_fmt = ov5670_set_pad_format,
@@ -2518,10 +2517,6 @@ static const struct media_entity_operations ov5670_subdev_entity_ops = {
 	.link_validate = v4l2_subdev_link_validate,
 };
 
-static const struct v4l2_subdev_internal_ops ov5670_internal_ops = {
-	.open = ov5670_open,
-};
-
 static int ov5670_regulators_probe(struct ov5670 *ov5670)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&ov5670->sd);
@@ -2630,7 +2625,6 @@ static int ov5670_probe(struct i2c_client *client)
 		goto error_mutex_destroy;
 	}
 
-	ov5670->sd.internal_ops = &ov5670_internal_ops;
 	ov5670->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
 			    V4L2_SUBDEV_FL_HAS_EVENTS;
 	ov5670->sd.entity.ops = &ov5670_subdev_entity_ops;
-- 
2.35.1

[PATCH v3 8/8] media: i2c: ov5670: Add .get_selection() support

[Jacopo Mondi]

From: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>

Add support for the .get_selection() pad operation to the ov5670 sensor
driver.

Report the native sensor size (pixel array), the crop bounds (readable
pixel array area) and the current and default analog crop rectangles.

Currently all driver's modes use an analog crop rectangle of size
[12, 4, 2600, 1952]. Instead of hardcoding the value in the operation
implementation, ad an .analog_crop field to the sensor's modes
definitions, to make sure that if any mode gets added, its crop
rectangle will be defined as well.

While at it re-sort the modes' field definition order to match the
declaration order and initialize the crop rectangle in init_cfg().

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/ov5670.c | 89 +++++++++++++++++++++++++++++++++++---
 1 file changed, 83 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 7f12b053cb0e..3a601e1600e9 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -71,6 +71,10 @@
 #define OV5670_REG_VALUE_16BIT		2
 #define OV5670_REG_VALUE_24BIT		3
 
+/* Pixel Array */
+#define OV5670_NATIVE_WIDTH		2624
+#define OV5670_NATIVE_HEIGHT		1980
+
 /* Initial number of frames to skip to avoid possible garbage */
 #define OV5670_NUM_OF_SKIP_FRAMES	2
 
@@ -113,10 +117,25 @@ struct ov5670_mode {
 	/* Link frequency needed for this resolution */
 	u32 link_freq_index;
 
+	/* Analog crop rectangle */
+	const struct v4l2_rect *analog_crop;
+
 	/* Sensor register settings for this resolution */
 	const struct ov5670_reg_list reg_list;
 };
 
+/*
+ * All the modes supported by the driver are obtained by subsampling the
+ * full pixel array. The below values are reflected in registers from
+ * 03800-0x3807 in the modes register-value tables.
+ */
+static const struct v4l2_rect ov5670_analog_crop = {
+	.left	= 12,
+	.top	= 4,
+	.width	= 2600,
+	.height	= 1952,
+};
+
 static const struct ov5670_reg mipi_data_rate_840mbps[] = {
 	{0x0300, 0x04},
 	{0x0301, 0x00},
@@ -1764,66 +1783,73 @@ static const struct ov5670_mode supported_modes[] = {
 		.height = 1944,
 		.vts_def = OV5670_VTS_30FPS,
 		.vts_min = OV5670_VTS_30FPS,
+		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
+		.analog_crop = &ov5670_analog_crop,
 		.reg_list = {
 			.num_of_regs = ARRAY_SIZE(mode_2592x1944_regs),
 			.regs = mode_2592x1944_regs,
 		},
-		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
 	},
 	{
 		.width = 1296,
 		.height = 972,
 		.vts_def = OV5670_VTS_30FPS,
 		.vts_min = 996,
+		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
+		.analog_crop = &ov5670_analog_crop,
 		.reg_list = {
 			.num_of_regs = ARRAY_SIZE(mode_1296x972_regs),
 			.regs = mode_1296x972_regs,
 		},
-		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
 	},
 	{
 		.width = 648,
 		.height = 486,
 		.vts_def = OV5670_VTS_30FPS,
 		.vts_min = 516,
+		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
+		.analog_crop = &ov5670_analog_crop,
 		.reg_list = {
 			.num_of_regs = ARRAY_SIZE(mode_648x486_regs),
 			.regs = mode_648x486_regs,
 		},
-		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
 	},
 	{
 		.width = 2560,
 		.height = 1440,
 		.vts_def = OV5670_VTS_30FPS,
 		.vts_min = OV5670_VTS_30FPS,
+		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
+		.analog_crop = &ov5670_analog_crop,
 		.reg_list = {
 			.num_of_regs = ARRAY_SIZE(mode_2560x1440_regs),
 			.regs = mode_2560x1440_regs,
 		},
-		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
 	},
 	{
 		.width = 1280,
 		.height = 720,
 		.vts_def = OV5670_VTS_30FPS,
 		.vts_min = 1020,
+
+		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
+		.analog_crop = &ov5670_analog_crop,
 		.reg_list = {
 			.num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
 			.regs = mode_1280x720_regs,
 		},
-		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
 	},
 	{
 		.width = 640,
 		.height = 360,
 		.vts_def = OV5670_VTS_30FPS,
 		.vts_min = 510,
+		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
+		.analog_crop = &ov5670_analog_crop,
 		.reg_list = {
 			.num_of_regs = ARRAY_SIZE(mode_640x360_regs),
 			.regs = mode_640x360_regs,
 		},
-		.link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX,
 	}
 };
 
@@ -2161,6 +2187,7 @@ static int ov5670_init_cfg(struct v4l2_subdev *sd,
 	struct v4l2_mbus_framefmt *fmt =
 				v4l2_subdev_get_try_format(sd, state, 0);
 	const struct ov5670_mode *default_mode = &supported_modes[0];
+	struct v4l2_rect *crop = v4l2_subdev_get_try_crop(sd, state, 0);
 
 	fmt->width = default_mode->width;
 	fmt->height = default_mode->height;
@@ -2171,6 +2198,8 @@ static int ov5670_init_cfg(struct v4l2_subdev *sd,
 	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
 	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(V4L2_COLORSPACE_SRGB);
 
+	*crop = *default_mode->analog_crop;
+
 	return 0;
 }
 
@@ -2490,6 +2519,52 @@ static const struct v4l2_subdev_core_ops ov5670_core_ops = {
 	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
 };
 
+static const struct v4l2_rect *
+__ov5670_get_pad_crop(struct ov5670 *sensor, struct v4l2_subdev_state *state,
+		      unsigned int pad, enum v4l2_subdev_format_whence which)
+{
+	const struct ov5670_mode *mode = sensor->cur_mode;
+
+	switch (which) {
+	case V4L2_SUBDEV_FORMAT_TRY:
+		return v4l2_subdev_get_try_crop(&sensor->sd, state, pad);
+	case V4L2_SUBDEV_FORMAT_ACTIVE:
+		return mode->analog_crop;
+	}
+
+	return NULL;
+}
+
+static int ov5670_get_selection(struct v4l2_subdev *subdev,
+				struct v4l2_subdev_state *state,
+				struct v4l2_subdev_selection *sel)
+{
+	struct ov5670 *sensor = to_ov5670(subdev);
+
+	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP:
+		mutex_lock(&sensor->mutex);
+		sel->r = *__ov5670_get_pad_crop(sensor, state, sel->pad,
+						sel->which);
+		mutex_unlock(&sensor->mutex);
+		break;
+	case V4L2_SEL_TGT_NATIVE_SIZE:
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+		sel->r.top = 0;
+		sel->r.left = 0;
+		sel->r.width = OV5670_NATIVE_WIDTH;
+		sel->r.height = OV5670_NATIVE_HEIGHT;
+		break;
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+		sel->r = ov5670_analog_crop;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static const struct v4l2_subdev_video_ops ov5670_video_ops = {
 	.s_stream = ov5670_set_stream,
 };
@@ -2500,6 +2575,8 @@ static const struct v4l2_subdev_pad_ops ov5670_pad_ops = {
 	.get_fmt = ov5670_get_pad_format,
 	.set_fmt = ov5670_set_pad_format,
 	.enum_frame_size = ov5670_enum_frame_size,
+	.get_selection = ov5670_get_selection,
+	.set_selection = ov5670_get_selection,
 };
 
 static const struct v4l2_subdev_sensor_ops ov5670_sensor_ops = {
-- 
2.35.1

Re: [PATCH v3 2/8] media: i2c: ov5670: Allow probing with OF

[Sakari Ailus]

Hi Jacopo,

On Tue, Mar 29, 2022 at 11:01:27AM +0200, Jacopo Mondi wrote:
> The ov5670 driver currently only supports probing using ACPI matching.
> Add support for OF and add a missing header inclusion.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/i2c/ov5670.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> index 02f75c18e480..721441024598 100644
> --- a/drivers/media/i2c/ov5670.c
> +++ b/drivers/media/i2c/ov5670.c
> @@ -3,7 +3,9 @@
>  
>  #include <linux/acpi.h>
>  #include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> +#include <linux/of.h>
>  #include <linux/pm_runtime.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
> @@ -2585,11 +2587,20 @@ static const struct acpi_device_id ov5670_acpi_ids[] = {
>  MODULE_DEVICE_TABLE(acpi, ov5670_acpi_ids);
>  #endif
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id ov5670_of_ids[] = {
> +	{ .compatible = "ovti,ov5670" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ov5670_of_ids);
> +#endif

No need for ifdefs here.

The compatible strings are also relevant on ACPI (as alternative for _HID
and _CID).

> +
>  static struct i2c_driver ov5670_i2c_driver = {
>  	.driver = {
>  		.name = "ov5670",
>  		.pm = &ov5670_pm_ops,
>  		.acpi_match_table = ACPI_PTR(ov5670_acpi_ids),
> +		.of_match_table = of_match_ptr(ov5670_of_ids),

Nor of_match_ptr() here.

>  	},
>  	.probe_new = ov5670_probe,
>  	.remove = ov5670_remove,

-- 
Regards,

Sakari Ailus

Re: [PATCH v3 3/8] media: i2c: ov5670: Probe clocks with OF

[Sakari Ailus]

Hi Jacopo,

On Tue, Mar 29, 2022 at 11:01:28AM +0200, Jacopo Mondi wrote:
> Add support for probing the main system clock using the common clock
> framework and its OF bindings.
> 
> Maintain ACPI compatibility by falling back to parse 'clock-frequency'.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/i2c/ov5670.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> index 721441024598..1cc312981c82 100644
> --- a/drivers/media/i2c/ov5670.c
> +++ b/drivers/media/i2c/ov5670.c
> @@ -2,6 +2,7 @@
>  // Copyright (c) 2017 Intel Corporation.
>  
>  #include <linux/acpi.h>
> +#include <linux/clk.h>
>  #include <linux/i2c.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> @@ -2475,13 +2476,10 @@ static int ov5670_probe(struct i2c_client *client)
>  	struct ov5670 *ov5670;
>  	const char *err_msg;
>  	u32 input_clk = 0;
> +	struct clk *clk;
>  	bool full_power;
>  	int ret;
>  
> -	device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
> -	if (input_clk != 19200000)
> -		return -EINVAL;
> -
>  	ov5670 = devm_kzalloc(&client->dev, sizeof(*ov5670), GFP_KERNEL);
>  	if (!ov5670) {
>  		ret = -ENOMEM;
> @@ -2489,6 +2487,22 @@ static int ov5670_probe(struct i2c_client *client)
>  		goto error_print;
>  	}
>  
> +	/* OF uses the common clock framework, ACPI uses "clock-frequency". */
> +	if (is_of_node(dev_fwnode(&client->dev))) {
> +		clk = devm_clk_get(&client->dev, NULL);
> +		if (IS_ERR(clk))
> +			return dev_err_probe(&client->dev, PTR_ERR(clk),
> +					     "error getting clock\n");
> +
> +		input_clk = clk_get_rate(clk);
> +	} else {
> +		device_property_read_u32(&client->dev, "clock-frequency",
> +					 &input_clk);
> +	}
> +
> +	if (input_clk != 19200000)
> +		return -EINVAL;

You could tell what that wrong frequency is. Up to you.

> +
>  	/* Initialize subdev */
>  	v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops);
>  

-- 
Regards,

Sakari Ailus

Re: [PATCH v3 7/8] media: i2c: ov5670: Implement init_cfg

[Sakari Ailus]

Hi Jacopo,

On Tue, Mar 29, 2022 at 11:01:32AM +0200, Jacopo Mondi wrote:
> Implement the .init_cfg() pad operation and initialize the default
> format.
> 
> With .init_cfg() pad operation implemented the deprecated .open()
> internal operation can now be dropped.

Nice patch.

Could you add here the configured size was used as the try size instead
previously?

-- 
Sakari Ailus

Re: [PATCH v3 6/8] media: i2c: ov5670: Add runtime_pm operations

[Sakari Ailus]

Hi Jacopo,

On Tue, Mar 29, 2022 at 11:01:31AM +0200, Jacopo Mondi wrote:
> Implement the power up and power down routines and install them as
> runtime_pm handler for runtime_suspend and runtime_resume operations.
> 
> Rework the runtime_pm enablement and the chip power handling during
> probe, as calling pm_runtime_idle() in a driver that registers no
> idle callback is a nop.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/i2c/ov5670.c | 58 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 52 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> index 9e69b4008917..b63b07d8ca2f 100644
> --- a/drivers/media/i2c/ov5670.c
> +++ b/drivers/media/i2c/ov5670.c
> @@ -4,6 +4,7 @@
>  #include <linux/acpi.h>
>  #include <linux/clk.h>
>  #include <linux/gpio/consumer.h>
> +#include <linux/delay.h>
>  #include <linux/i2c.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> @@ -2424,6 +2425,39 @@ static int ov5670_set_stream(struct v4l2_subdev *sd, int enable)
>  	return ret;
>  }
>  
> +static int __maybe_unused ov5670_runtime_resume(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct ov5670 *ov5670 = to_ov5670(sd);
> +	int ret;
> +
> +	ret = regulator_bulk_enable(OV5670_NUM_SUPPLIES, ov5670->supplies);
> +	if (ret)
> +		return ret;
> +
> +	gpiod_set_value_cansleep(ov5670->pwdn_gpio, 0);
> +	gpiod_set_value_cansleep(ov5670->reset_gpio, 0);
> +
> +	/* 8192 * 2 clock pulses before the first SCCB transaction. */
> +	usleep_range(1000, 1500);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused ov5670_runtime_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct ov5670 *ov5670 = to_ov5670(sd);
> +
> +	gpiod_set_value_cansleep(ov5670->reset_gpio, 1);
> +	gpiod_set_value_cansleep(ov5670->pwdn_gpio, 1);
> +	regulator_bulk_disable(OV5670_NUM_SUPPLIES, ov5670->supplies);
> +
> +	return 0;
> +}
> +
>  static int __maybe_unused ov5670_suspend(struct device *dev)
>  {
>  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> @@ -2564,14 +2598,25 @@ static int ov5670_probe(struct i2c_client *client)
>  		goto error_print;
>  	}
>  
> +	pm_runtime_enable(&client->dev);
> +
>  	full_power = acpi_dev_state_d0(&client->dev);
>  	if (full_power) {
> +		ret = pm_runtime_resume_and_get(&client->dev);

This will power the device on, but only on OF systems.

Could you instead power the device on on OF systems explicitly? That's what
other drivers do, too. The changes would be probably more simple to
implement, too.

> +		if (ret) {
> +			err_msg = "Failed to power on";
> +			goto error_print;
> +		}
> +
>  		/* Check module identity */
>  		ret = ov5670_identify_module(ov5670);
>  		if (ret) {
>  			err_msg = "ov5670_identify_module() error";
> -			goto error_print;
> +			goto error_power_off;
>  		}
> +
> +		/* Set the device's state to active if it's in D0 state. */
> +		pm_runtime_set_active(&client->dev);
>  	}
>  
>  	mutex_init(&ov5670->mutex);
> @@ -2608,11 +2653,7 @@ static int ov5670_probe(struct i2c_client *client)
>  
>  	ov5670->streaming = false;
>  
> -	/* Set the device's state to active if it's in D0 state. */
> -	if (full_power)
> -		pm_runtime_set_active(&client->dev);
> -	pm_runtime_enable(&client->dev);
> -	pm_runtime_idle(&client->dev);
> +	pm_runtime_suspend(&client->dev);
>  
>  	return 0;
>  
> @@ -2625,6 +2666,9 @@ static int ov5670_probe(struct i2c_client *client)
>  error_mutex_destroy:
>  	mutex_destroy(&ov5670->mutex);
>  
> +error_power_off:
> +	pm_runtime_put(&client->dev);
> +
>  error_print:
>  	dev_err(&client->dev, "%s: %s %d\n", __func__, err_msg, ret);
>  
> @@ -2641,6 +2685,7 @@ static int ov5670_remove(struct i2c_client *client)
>  	v4l2_ctrl_handler_free(sd->ctrl_handler);
>  	mutex_destroy(&ov5670->mutex);
>  
> +	pm_runtime_put(&client->dev);
>  	pm_runtime_disable(&client->dev);
>  
>  	return 0;
> @@ -2648,6 +2693,7 @@ static int ov5670_remove(struct i2c_client *client)
>  
>  static const struct dev_pm_ops ov5670_pm_ops = {
>  	SET_SYSTEM_SLEEP_PM_OPS(ov5670_suspend, ov5670_resume)
> +	SET_RUNTIME_PM_OPS(ov5670_runtime_suspend, ov5670_runtime_resume, NULL)
>  };
>  
>  #ifdef CONFIG_ACPI

-- 
Regards,

Sakari Ailus

Re: [PATCH v3 6/8] media: i2c: ov5670: Add runtime_pm operations

[Laurent Pinchart]

Hi Sakari,

On Thu, Mar 31, 2022 at 01:08:17PM +0300, Sakari Ailus wrote:
> On Tue, Mar 29, 2022 at 11:01:31AM +0200, Jacopo Mondi wrote:
> > Implement the power up and power down routines and install them as
> > runtime_pm handler for runtime_suspend and runtime_resume operations.
> > 
> > Rework the runtime_pm enablement and the chip power handling during
> > probe, as calling pm_runtime_idle() in a driver that registers no
> > idle callback is a nop.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  drivers/media/i2c/ov5670.c | 58 ++++++++++++++++++++++++++++++++++----
> >  1 file changed, 52 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> > index 9e69b4008917..b63b07d8ca2f 100644
> > --- a/drivers/media/i2c/ov5670.c
> > +++ b/drivers/media/i2c/ov5670.c
> > @@ -4,6 +4,7 @@
> >  #include <linux/acpi.h>
> >  #include <linux/clk.h>
> >  #include <linux/gpio/consumer.h>
> > +#include <linux/delay.h>
> >  #include <linux/i2c.h>
> >  #include <linux/mod_devicetable.h>
> >  #include <linux/module.h>
> > @@ -2424,6 +2425,39 @@ static int ov5670_set_stream(struct v4l2_subdev *sd, int enable)
> >  	return ret;
> >  }
> >  
> > +static int __maybe_unused ov5670_runtime_resume(struct device *dev)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > +	struct ov5670 *ov5670 = to_ov5670(sd);
> > +	int ret;
> > +
> > +	ret = regulator_bulk_enable(OV5670_NUM_SUPPLIES, ov5670->supplies);
> > +	if (ret)
> > +		return ret;
> > +
> > +	gpiod_set_value_cansleep(ov5670->pwdn_gpio, 0);
> > +	gpiod_set_value_cansleep(ov5670->reset_gpio, 0);
> > +
> > +	/* 8192 * 2 clock pulses before the first SCCB transaction. */
> > +	usleep_range(1000, 1500);
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused ov5670_runtime_suspend(struct device *dev)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > +	struct ov5670 *ov5670 = to_ov5670(sd);
> > +
> > +	gpiod_set_value_cansleep(ov5670->reset_gpio, 1);
> > +	gpiod_set_value_cansleep(ov5670->pwdn_gpio, 1);
> > +	regulator_bulk_disable(OV5670_NUM_SUPPLIES, ov5670->supplies);
> > +
> > +	return 0;
> > +}
> > +
> >  static int __maybe_unused ov5670_suspend(struct device *dev)
> >  {
> >  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > @@ -2564,14 +2598,25 @@ static int ov5670_probe(struct i2c_client *client)
> >  		goto error_print;
> >  	}
> >  
> > +	pm_runtime_enable(&client->dev);
> > +
> >  	full_power = acpi_dev_state_d0(&client->dev);
> >  	if (full_power) {
> > +		ret = pm_runtime_resume_and_get(&client->dev);
> 
> This will power the device on, but only on OF systems.
> 
> Could you instead power the device on on OF systems explicitly? That's what
> other drivers do, too. The changes would be probably more simple to
> implement, too.

Can we fix ACPI to do something more sane instead ? :-) I don't want to
see those complicated patterns replicated in all drivers.

> > +		if (ret) {
> > +			err_msg = "Failed to power on";
> > +			goto error_print;
> > +		}
> > +
> >  		/* Check module identity */
> >  		ret = ov5670_identify_module(ov5670);
> >  		if (ret) {
> >  			err_msg = "ov5670_identify_module() error";
> > -			goto error_print;
> > +			goto error_power_off;
> >  		}
> > +
> > +		/* Set the device's state to active if it's in D0 state. */
> > +		pm_runtime_set_active(&client->dev);
> >  	}
> >  
> >  	mutex_init(&ov5670->mutex);
> > @@ -2608,11 +2653,7 @@ static int ov5670_probe(struct i2c_client *client)
> >  
> >  	ov5670->streaming = false;
> >  
> > -	/* Set the device's state to active if it's in D0 state. */
> > -	if (full_power)
> > -		pm_runtime_set_active(&client->dev);
> > -	pm_runtime_enable(&client->dev);
> > -	pm_runtime_idle(&client->dev);
> > +	pm_runtime_suspend(&client->dev);
> >  
> >  	return 0;
> >  
> > @@ -2625,6 +2666,9 @@ static int ov5670_probe(struct i2c_client *client)
> >  error_mutex_destroy:
> >  	mutex_destroy(&ov5670->mutex);
> >  
> > +error_power_off:
> > +	pm_runtime_put(&client->dev);
> > +
> >  error_print:
> >  	dev_err(&client->dev, "%s: %s %d\n", __func__, err_msg, ret);
> >  
> > @@ -2641,6 +2685,7 @@ static int ov5670_remove(struct i2c_client *client)
> >  	v4l2_ctrl_handler_free(sd->ctrl_handler);
> >  	mutex_destroy(&ov5670->mutex);
> >  
> > +	pm_runtime_put(&client->dev);
> >  	pm_runtime_disable(&client->dev);
> >  
> >  	return 0;
> > @@ -2648,6 +2693,7 @@ static int ov5670_remove(struct i2c_client *client)
> >  
> >  static const struct dev_pm_ops ov5670_pm_ops = {
> >  	SET_SYSTEM_SLEEP_PM_OPS(ov5670_suspend, ov5670_resume)
> > +	SET_RUNTIME_PM_OPS(ov5670_runtime_suspend, ov5670_runtime_resume, NULL)
> >  };
> >  
> >  #ifdef CONFIG_ACPI

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 6/8] media: i2c: ov5670: Add runtime_pm operations

[Sakari Ailus]

Hi Laurent,

On Thu, Mar 31, 2022 at 01:33:27PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Thu, Mar 31, 2022 at 01:08:17PM +0300, Sakari Ailus wrote:
> > On Tue, Mar 29, 2022 at 11:01:31AM +0200, Jacopo Mondi wrote:
> > > Implement the power up and power down routines and install them as
> > > runtime_pm handler for runtime_suspend and runtime_resume operations.
> > > 
> > > Rework the runtime_pm enablement and the chip power handling during
> > > probe, as calling pm_runtime_idle() in a driver that registers no
> > > idle callback is a nop.
> > > 
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  drivers/media/i2c/ov5670.c | 58 ++++++++++++++++++++++++++++++++++----
> > >  1 file changed, 52 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> > > index 9e69b4008917..b63b07d8ca2f 100644
> > > --- a/drivers/media/i2c/ov5670.c
> > > +++ b/drivers/media/i2c/ov5670.c
> > > @@ -4,6 +4,7 @@
> > >  #include <linux/acpi.h>
> > >  #include <linux/clk.h>
> > >  #include <linux/gpio/consumer.h>
> > > +#include <linux/delay.h>
> > >  #include <linux/i2c.h>
> > >  #include <linux/mod_devicetable.h>
> > >  #include <linux/module.h>
> > > @@ -2424,6 +2425,39 @@ static int ov5670_set_stream(struct v4l2_subdev *sd, int enable)
> > >  	return ret;
> > >  }
> > >  
> > > +static int __maybe_unused ov5670_runtime_resume(struct device *dev)
> > > +{
> > > +	struct i2c_client *client = to_i2c_client(dev);
> > > +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > +	struct ov5670 *ov5670 = to_ov5670(sd);
> > > +	int ret;
> > > +
> > > +	ret = regulator_bulk_enable(OV5670_NUM_SUPPLIES, ov5670->supplies);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	gpiod_set_value_cansleep(ov5670->pwdn_gpio, 0);
> > > +	gpiod_set_value_cansleep(ov5670->reset_gpio, 0);
> > > +
> > > +	/* 8192 * 2 clock pulses before the first SCCB transaction. */
> > > +	usleep_range(1000, 1500);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int __maybe_unused ov5670_runtime_suspend(struct device *dev)
> > > +{
> > > +	struct i2c_client *client = to_i2c_client(dev);
> > > +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > +	struct ov5670 *ov5670 = to_ov5670(sd);
> > > +
> > > +	gpiod_set_value_cansleep(ov5670->reset_gpio, 1);
> > > +	gpiod_set_value_cansleep(ov5670->pwdn_gpio, 1);
> > > +	regulator_bulk_disable(OV5670_NUM_SUPPLIES, ov5670->supplies);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int __maybe_unused ov5670_suspend(struct device *dev)
> > >  {
> > >  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > > @@ -2564,14 +2598,25 @@ static int ov5670_probe(struct i2c_client *client)
> > >  		goto error_print;
> > >  	}
> > >  
> > > +	pm_runtime_enable(&client->dev);
> > > +
> > >  	full_power = acpi_dev_state_d0(&client->dev);
> > >  	if (full_power) {
> > > +		ret = pm_runtime_resume_and_get(&client->dev);
> > 
> > This will power the device on, but only on OF systems.
> > 
> > Could you instead power the device on on OF systems explicitly? That's what
> > other drivers do, too. The changes would be probably more simple to
> > implement, too.
> 
> Can we fix ACPI to do something more sane instead ? :-) I don't want to
> see those complicated patterns replicated in all drivers.

I guess it could be changed. But it will take time.

This patch does not quite represent what it takes to implement runtime PM
support without ACPI.

Also, sensor drivers shouldn't be somehow special when it comes to power
management so depending on CONFIG_PM isn't all that nice either. Of course,
removing that Kconfig option would simplify quite a few things.

-- 
Regards,

Sakari Ailus

Re: [PATCH v3 6/8] media: i2c: ov5670: Add runtime_pm operations

[Sakari Ailus]

Hi Jacopo,

On Tue, Mar 29, 2022 at 11:01:31AM +0200, Jacopo Mondi wrote:
> Implement the power up and power down routines and install them as
> runtime_pm handler for runtime_suspend and runtime_resume operations.
> 
> Rework the runtime_pm enablement and the chip power handling during
> probe, as calling pm_runtime_idle() in a driver that registers no
> idle callback is a nop.

The suspend callback is called by rpm_idle() in the absence of the
idle callback.

> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/i2c/ov5670.c | 58 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 52 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> index 9e69b4008917..b63b07d8ca2f 100644
> --- a/drivers/media/i2c/ov5670.c
> +++ b/drivers/media/i2c/ov5670.c
> @@ -4,6 +4,7 @@
>  #include <linux/acpi.h>
>  #include <linux/clk.h>
>  #include <linux/gpio/consumer.h>
> +#include <linux/delay.h>
>  #include <linux/i2c.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> @@ -2424,6 +2425,39 @@ static int ov5670_set_stream(struct v4l2_subdev *sd, int enable)
>  	return ret;
>  }
>  
> +static int __maybe_unused ov5670_runtime_resume(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct ov5670 *ov5670 = to_ov5670(sd);
> +	int ret;
> +
> +	ret = regulator_bulk_enable(OV5670_NUM_SUPPLIES, ov5670->supplies);
> +	if (ret)
> +		return ret;
> +
> +	gpiod_set_value_cansleep(ov5670->pwdn_gpio, 0);
> +	gpiod_set_value_cansleep(ov5670->reset_gpio, 0);
> +
> +	/* 8192 * 2 clock pulses before the first SCCB transaction. */
> +	usleep_range(1000, 1500);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused ov5670_runtime_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct ov5670 *ov5670 = to_ov5670(sd);
> +
> +	gpiod_set_value_cansleep(ov5670->reset_gpio, 1);
> +	gpiod_set_value_cansleep(ov5670->pwdn_gpio, 1);
> +	regulator_bulk_disable(OV5670_NUM_SUPPLIES, ov5670->supplies);
> +
> +	return 0;
> +}
> +
>  static int __maybe_unused ov5670_suspend(struct device *dev)
>  {
>  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> @@ -2564,14 +2598,25 @@ static int ov5670_probe(struct i2c_client *client)
>  		goto error_print;
>  	}
>  
> +	pm_runtime_enable(&client->dev);
> +
>  	full_power = acpi_dev_state_d0(&client->dev);
>  	if (full_power) {
> +		ret = pm_runtime_resume_and_get(&client->dev);

Please see how e.g. the CCS driver does this (no need for autosuspend
though). E.g. don't use runtime PM to power the sensor on in probe, or off
in remove.

> +		if (ret) {
> +			err_msg = "Failed to power on";
> +			goto error_print;
> +		}
> +
>  		/* Check module identity */
>  		ret = ov5670_identify_module(ov5670);
>  		if (ret) {
>  			err_msg = "ov5670_identify_module() error";
> -			goto error_print;
> +			goto error_power_off;
>  		}
> +
> +		/* Set the device's state to active if it's in D0 state. */
> +		pm_runtime_set_active(&client->dev);
>  	}
>  
>  	mutex_init(&ov5670->mutex);
> @@ -2608,11 +2653,7 @@ static int ov5670_probe(struct i2c_client *client)
>  
>  	ov5670->streaming = false;
>  
> -	/* Set the device's state to active if it's in D0 state. */
> -	if (full_power)
> -		pm_runtime_set_active(&client->dev);
> -	pm_runtime_enable(&client->dev);
> -	pm_runtime_idle(&client->dev);
> +	pm_runtime_suspend(&client->dev);
>  
>  	return 0;
>  
> @@ -2625,6 +2666,9 @@ static int ov5670_probe(struct i2c_client *client)
>  error_mutex_destroy:
>  	mutex_destroy(&ov5670->mutex);
>  
> +error_power_off:
> +	pm_runtime_put(&client->dev);
> +
>  error_print:
>  	dev_err(&client->dev, "%s: %s %d\n", __func__, err_msg, ret);
>  
> @@ -2641,6 +2685,7 @@ static int ov5670_remove(struct i2c_client *client)
>  	v4l2_ctrl_handler_free(sd->ctrl_handler);
>  	mutex_destroy(&ov5670->mutex);
>  
> +	pm_runtime_put(&client->dev);
>  	pm_runtime_disable(&client->dev);
>  
>  	return 0;
> @@ -2648,6 +2693,7 @@ static int ov5670_remove(struct i2c_client *client)
>  
>  static const struct dev_pm_ops ov5670_pm_ops = {
>  	SET_SYSTEM_SLEEP_PM_OPS(ov5670_suspend, ov5670_resume)
> +	SET_RUNTIME_PM_OPS(ov5670_runtime_suspend, ov5670_runtime_resume, NULL)
>  };
>  
>  #ifdef CONFIG_ACPI

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v3 6/8] media: i2c: ov5670: Add runtime_pm operations

[Sakari Ailus]

On Wed, Apr 27, 2022 at 12:57:16PM +0300, Sakari Ailus wrote:
> > +		ret = pm_runtime_resume_and_get(&client->dev);
> 
> Please see how e.g. the CCS driver does this (no need for autosuspend
> though). E.g. don't use runtime PM to power the sensor on in probe, or off
> in remove.

What's actually needed in this driver would be explicit use of the resume
and suspend callbacks for !CONFIG_PM as well as setting the callbacks in
ov5670_pm_ops. No other changes should be needed.

-- 
Sakari Ailus