[PATCH v2 0/5] media: i2c: ov5645 driver enhancements

[PATCH v2 0/5] media: i2c: ov5645 driver enhancements

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Hi All,

The main aim of this series is to add PM support to the sensor driver.

I had two more patches [0] and [1] which were for ov5645, so instead
sending them separately I have clubbed them as series.

v1-> v2
- patch #1 is infact a v3 [1] no changes
- patch #2 fixed review comments pointed by Sakari
- patch #3 [0] no changes 
- patches #4 and #5 are new

[0] https://patchwork.linuxtv.org/project/linux-media/patch/20220927202005.750621-1-prabhakar.mahadev-lad.rj@bp.renesas.com/
[1] https://patchwork.linuxtv.org/project/linux-media/patch/20220919153540.178732-1-prabhakar.mahadev-lad.rj@bp.renesas.com/

Cheers,
Prabhakar

Lad Prabhakar (5):
  media: dt-bindings: ov5645: Convert OV5645 binding to a schema
  media: i2c: ov5645: Use runtime PM
  media: i2c: ov5645: Drop empty comment
  media: i2c: ov5645: Return zero for s_stream(0)
  media: i2c: ov5645: Call ov5645_entity_init_cfg() before registering
    the subdev

 .../devicetree/bindings/media/i2c/ov5645.txt  |  54 -------
 .../bindings/media/i2c/ovti,ov5645.yaml       | 104 ++++++++++++
 drivers/media/i2c/Kconfig                     |   2 +-
 drivers/media/i2c/ov5645.c                    | 151 +++++++++---------
 4 files changed, 178 insertions(+), 133 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5645.yaml

-- 
2.25.1

[PATCH v2 1/5] media: dt-bindings: ov5645: Convert OV5645 binding to a schema

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Convert the simple OV5645 Device Tree binding to json-schema.

The previous binding marked the below properties as required which was a
driver requirement and not the device requirement so just drop them from
the required list during the conversion.
- clock-frequency
- enable-gpios
- reset-gpios

Also drop the "clock-names" property as we have a single clock source for
the sensor and the driver has been updated to drop the clk referencing by
name.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Resend v3:
* No change

v2 -> v3
* Dropped clock-names property
* Marked power supplies as mandatory
* Dropped the comment for voltage power supplies
* Included RB tag from Laurent
* Driver change to drop clock-names [0]

[0] https://lore.kernel.org/linux-media/Yyh%2F3uzOJOu3drEB@pendragon.ideasonboard.com/T/#t

v1 -> v2
* Dropped ref to video-interface-devices.yaml#
* Dropped driver specific required items from the list
* Updated commit message
* Dropped clock-lanes and bus-type from the port and example node
* Marked data-lanes as required in port node
---
 .../devicetree/bindings/media/i2c/ov5645.txt  |  54 ---------
 .../bindings/media/i2c/ovti,ov5645.yaml       | 104 ++++++++++++++++++
 2 files changed, 104 insertions(+), 54 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5645.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
deleted file mode 100644
index 72ad992f77be..000000000000
--- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
+++ /dev/null
@@ -1,54 +0,0 @@
-* Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor
-
-The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image sensor with
-an active array size of 2592H x 1944V. It is programmable through a serial I2C
-interface.
-
-Required Properties:
-- compatible: Value should be "ovti,ov5645".
-- clocks: Reference to the xclk clock.
-- clock-names: Should be "xclk".
-- clock-frequency: Frequency of the xclk clock.
-- enable-gpios: Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH. This corresponds
-  to the hardware pin PWDNB which is physically active low.
-- reset-gpios: Chip reset GPIO. Polarity is GPIO_ACTIVE_LOW. This corresponds to
-  the hardware pin RESETB.
-- vdddo-supply: Chip digital IO regulator.
-- vdda-supply: Chip analog regulator.
-- vddd-supply: Chip digital core regulator.
-
-The device node must contain one 'port' child node for its digital output
-video port, in accordance with the video interface bindings defined in
-Documentation/devicetree/bindings/media/video-interfaces.txt.
-
-Example:
-
-	&i2c1 {
-		...
-
-		ov5645: ov5645@3c {
-			compatible = "ovti,ov5645";
-			reg = <0x3c>;
-
-			enable-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>;
-			reset-gpios = <&gpio5 20 GPIO_ACTIVE_LOW>;
-			pinctrl-names = "default";
-			pinctrl-0 = <&camera_rear_default>;
-
-			clocks = <&clks 200>;
-			clock-names = "xclk";
-			clock-frequency = <24000000>;
-
-			vdddo-supply = <&camera_dovdd_1v8>;
-			vdda-supply = <&camera_avdd_2v8>;
-			vddd-supply = <&camera_dvdd_1v2>;
-
-			port {
-				ov5645_ep: endpoint {
-					clock-lanes = <1>;
-					data-lanes = <0 2>;
-					remote-endpoint = <&csi0_ep>;
-				};
-			};
-		};
-	};
diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5645.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5645.yaml
new file mode 100644
index 000000000000..0b10483cd267
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5645.yaml
@@ -0,0 +1,104 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/ovti,ov5645.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: OmniVision OV5645 Image Sensor Device Tree Bindings
+
+maintainers:
+  - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
+
+properties:
+  compatible:
+    const: ovti,ov5645
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    description: XCLK Input Clock
+
+  clock-frequency:
+    description: Frequency of the xclk clock in Hz.
+
+  vdda-supply:
+    description: Analog voltage supply, 2.8 volts
+
+  vddd-supply:
+    description: Digital core voltage supply, 1.5 volts
+
+  vdddo-supply:
+    description: Digital I/O voltage supply, 1.8 volts
+
+  enable-gpios:
+    maxItems: 1
+    description:
+      Reference to the GPIO connected to the PWDNB pin, if any.
+
+  reset-gpios:
+    maxItems: 1
+    description:
+      Reference to the GPIO connected to the RESETB pin, if any.
+
+  port:
+    description: Digital Output 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
+            items:
+              enum: [1, 2]
+
+        required:
+          - data-lanes
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - vdddo-supply
+  - vdda-supply
+  - vddd-supply
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+      #include <dt-bindings/gpio/gpio.h>
+
+      i2c {
+          #address-cells = <1>;
+          #size-cells = <0>;
+
+          camera@3c {
+              compatible = "ovti,ov5645";
+              pinctrl-names = "default";
+              pinctrl-0 = <&pinctrl_ov5645>;
+              reg = <0x3c>;
+              clocks = <&clks 1>;
+              clock-frequency = <24000000>;
+              vdddo-supply = <&ov5645_vdddo_1v8>;
+              vdda-supply = <&ov5645_vdda_2v8>;
+              vddd-supply = <&ov5645_vddd_1v5>;
+              enable-gpios = <&gpio1 19 GPIO_ACTIVE_HIGH>;
+              reset-gpios = <&gpio1 20 GPIO_ACTIVE_LOW>;
+
+              port {
+                  ov5645_ep: endpoint {
+                      remote-endpoint = <&csi0_ep>;
+                      data-lanes = <1 2>;
+                  };
+              };
+          };
+      };
+...
-- 
2.25.1

[PATCH v2 2/5] media: i2c: ov5645: Use runtime PM

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Switch to using runtime PM for power management.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v1->v2
* Moved pm_runtime_*_autosuspend() calls after registering the subdev.
---
 drivers/media/i2c/Kconfig  |   2 +-
 drivers/media/i2c/ov5645.c | 137 +++++++++++++++++++------------------
 2 files changed, 70 insertions(+), 69 deletions(-)

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 7806d4b81716..c0edd1017fe8 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -459,7 +459,7 @@ config VIDEO_OV5640
 config VIDEO_OV5645
 	tristate "OmniVision OV5645 sensor support"
 	depends on OF
-	depends on I2C && VIDEO_DEV
+	depends on I2C && PM && VIDEO_DEV
 	select MEDIA_CONTROLLER
 	select VIDEO_V4L2_SUBDEV_API
 	select V4L2_FWNODE
diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index 81e4e87e1821..1551690a94e0 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -27,6 +27,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_graph.h>
+#include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/types.h>
@@ -108,7 +109,6 @@ struct ov5645 {
 	u8 timing_tc_reg21;
 
 	struct mutex power_lock; /* lock to protect power state */
-	int power_count;
 
 	struct gpio_desc *enable_gpio;
 	struct gpio_desc *rst_gpio;
@@ -635,8 +635,24 @@ static int ov5645_set_register_array(struct ov5645 *ov5645,
 	return 0;
 }
 
-static int ov5645_set_power_on(struct ov5645 *ov5645)
+static int ov5645_set_power_off(struct device *dev)
 {
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct ov5645 *ov5645 = to_ov5645(sd);
+
+	ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x58);
+	gpiod_set_value_cansleep(ov5645->rst_gpio, 1);
+	gpiod_set_value_cansleep(ov5645->enable_gpio, 0);
+	clk_disable_unprepare(ov5645->xclk);
+	regulator_bulk_disable(OV5645_NUM_SUPPLIES, ov5645->supplies);
+
+	return 0;
+}
+
+static int ov5645_set_power_on(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct ov5645 *ov5645 = to_ov5645(sd);
 	int ret;
 
 	ret = regulator_bulk_enable(OV5645_NUM_SUPPLIES, ov5645->supplies);
@@ -658,57 +674,19 @@ static int ov5645_set_power_on(struct ov5645 *ov5645)
 
 	msleep(20);
 
-	return 0;
-}
-
-static void ov5645_set_power_off(struct ov5645 *ov5645)
-{
-	gpiod_set_value_cansleep(ov5645->rst_gpio, 1);
-	gpiod_set_value_cansleep(ov5645->enable_gpio, 0);
-	clk_disable_unprepare(ov5645->xclk);
-	regulator_bulk_disable(OV5645_NUM_SUPPLIES, ov5645->supplies);
-}
-
-static int ov5645_s_power(struct v4l2_subdev *sd, int on)
-{
-	struct ov5645 *ov5645 = to_ov5645(sd);
-	int ret = 0;
-
-	mutex_lock(&ov5645->power_lock);
-
-	/* If the power count is modified from 0 to != 0 or from != 0 to 0,
-	 * update the power state.
-	 */
-	if (ov5645->power_count == !on) {
-		if (on) {
-			ret = ov5645_set_power_on(ov5645);
-			if (ret < 0)
-				goto exit;
-
-			ret = ov5645_set_register_array(ov5645,
-					ov5645_global_init_setting,
+	ret = ov5645_set_register_array(ov5645, ov5645_global_init_setting,
 					ARRAY_SIZE(ov5645_global_init_setting));
-			if (ret < 0) {
-				dev_err(ov5645->dev,
-					"could not set init registers\n");
-				ov5645_set_power_off(ov5645);
-				goto exit;
-			}
-
-			usleep_range(500, 1000);
-		} else {
-			ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x58);
-			ov5645_set_power_off(ov5645);
-		}
+	if (ret < 0) {
+		dev_err(ov5645->dev, "could not set init registers\n");
+		goto exit;
 	}
 
-	/* Update the power count. */
-	ov5645->power_count += on ? 1 : -1;
-	WARN_ON(ov5645->power_count < 0);
+	usleep_range(500, 1000);
 
-exit:
-	mutex_unlock(&ov5645->power_lock);
+	return 0;
 
+exit:
+	ov5645_set_power_off(dev);
 	return ret;
 }
 
@@ -795,7 +773,7 @@ static int ov5645_s_ctrl(struct v4l2_ctrl *ctrl)
 	int ret;
 
 	mutex_lock(&ov5645->power_lock);
-	if (!ov5645->power_count) {
+	if (!pm_runtime_get_if_in_use(ov5645->dev)) {
 		mutex_unlock(&ov5645->power_lock);
 		return 0;
 	}
@@ -827,6 +805,7 @@ static int ov5645_s_ctrl(struct v4l2_ctrl *ctrl)
 		break;
 	}
 
+	pm_runtime_put_autosuspend(ov5645->dev);
 	mutex_unlock(&ov5645->power_lock);
 
 	return ret;
@@ -991,6 +970,10 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
 	int ret;
 
 	if (enable) {
+		ret = pm_runtime_resume_and_get(ov5645->dev);
+		if (ret < 0)
+			return ret;
+
 		ret = ov5645_set_register_array(ov5645,
 					ov5645->current_mode->data,
 					ov5645->current_mode->data_size);
@@ -998,22 +981,22 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
 			dev_err(ov5645->dev, "could not set mode %dx%d\n",
 				ov5645->current_mode->width,
 				ov5645->current_mode->height);
-			return ret;
+			goto err_rpm_put;
 		}
 		ret = v4l2_ctrl_handler_setup(&ov5645->ctrls);
 		if (ret < 0) {
 			dev_err(ov5645->dev, "could not sync v4l2 controls\n");
-			return ret;
+			goto err_rpm_put;
 		}
 
 		ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x45);
 		if (ret < 0)
-			return ret;
+			goto err_rpm_put;
 
 		ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
 				       OV5645_SYSTEM_CTRL0_START);
 		if (ret < 0)
-			return ret;
+			goto err_rpm_put;
 	} else {
 		ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
 		if (ret < 0)
@@ -1023,14 +1006,15 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
 				       OV5645_SYSTEM_CTRL0_STOP);
 		if (ret < 0)
 			return ret;
+		pm_runtime_put(ov5645->dev);
 	}
 
 	return 0;
-}
 
-static const struct v4l2_subdev_core_ops ov5645_core_ops = {
-	.s_power = ov5645_s_power,
-};
+err_rpm_put:
+	pm_runtime_put(ov5645->dev);
+	return ret;
+}
 
 static const struct v4l2_subdev_video_ops ov5645_video_ops = {
 	.s_stream = ov5645_s_stream,
@@ -1046,7 +1030,6 @@ static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
 };
 
 static const struct v4l2_subdev_ops ov5645_subdev_ops = {
-	.core = &ov5645_core_ops,
 	.video = &ov5645_video_ops,
 	.pad = &ov5645_subdev_pad_ops,
 };
@@ -1188,11 +1171,9 @@ static int ov5645_probe(struct i2c_client *client)
 		goto free_ctrl;
 	}
 
-	ret = ov5645_s_power(&ov5645->sd, true);
-	if (ret < 0) {
-		dev_err(dev, "could not power up OV5645\n");
+	ret = ov5645_set_power_on(dev);
+	if (ret)
 		goto free_entity;
-	}
 
 	ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_HIGH, &chip_id_high);
 	if (ret < 0 || chip_id_high != OV5645_CHIP_ID_HIGH_BYTE) {
@@ -1209,12 +1190,16 @@ static int ov5645_probe(struct i2c_client *client)
 
 	dev_info(dev, "OV5645 detected at address 0x%02x\n", client->addr);
 
+	pm_runtime_set_active(dev);
+	pm_runtime_get_noresume(dev);
+	pm_runtime_enable(dev);
+
 	ret = ov5645_read_reg(ov5645, OV5645_AEC_PK_MANUAL,
 			      &ov5645->aec_pk_manual);
 	if (ret < 0) {
 		dev_err(dev, "could not read AEC/AGC mode\n");
 		ret = -ENODEV;
-		goto power_down;
+		goto err_pm_runtime;
 	}
 
 	ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG20,
@@ -1222,7 +1207,7 @@ static int ov5645_probe(struct i2c_client *client)
 	if (ret < 0) {
 		dev_err(dev, "could not read vflip value\n");
 		ret = -ENODEV;
-		goto power_down;
+		goto err_pm_runtime;
 	}
 
 	ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG21,
@@ -1230,23 +1215,30 @@ static int ov5645_probe(struct i2c_client *client)
 	if (ret < 0) {
 		dev_err(dev, "could not read hflip value\n");
 		ret = -ENODEV;
-		goto power_down;
+		goto err_pm_runtime;
 	}
 
-	ov5645_s_power(&ov5645->sd, false);
-
 	ret = v4l2_async_register_subdev(&ov5645->sd);
 	if (ret < 0) {
 		dev_err(dev, "could not register v4l2 device\n");
+		pm_runtime_disable(dev);
+		pm_runtime_set_suspended(dev);
 		goto free_entity;
 	}
 
+	pm_runtime_set_autosuspend_delay(dev, 1000);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_put_autosuspend(dev);
+
 	ov5645_entity_init_cfg(&ov5645->sd, NULL);
 
 	return 0;
 
+err_pm_runtime:
+	pm_runtime_disable(dev);
+	pm_runtime_put_noidle(dev);
 power_down:
-	ov5645_s_power(&ov5645->sd, false);
+	ov5645_set_power_off(dev);
 free_entity:
 	media_entity_cleanup(&ov5645->sd.entity);
 free_ctrl:
@@ -1264,6 +1256,10 @@ static void ov5645_remove(struct i2c_client *client)
 	v4l2_async_unregister_subdev(&ov5645->sd);
 	media_entity_cleanup(&ov5645->sd.entity);
 	v4l2_ctrl_handler_free(&ov5645->ctrls);
+	pm_runtime_disable(ov5645->dev);
+	if (!pm_runtime_status_suspended(ov5645->dev))
+		ov5645_set_power_off(ov5645->dev);
+	pm_runtime_set_suspended(ov5645->dev);
 	mutex_destroy(&ov5645->power_lock);
 }
 
@@ -1279,10 +1275,15 @@ static const struct of_device_id ov5645_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, ov5645_of_match);
 
+static const struct dev_pm_ops ov5645_pm_ops = {
+	SET_RUNTIME_PM_OPS(ov5645_set_power_off, ov5645_set_power_on, NULL)
+};
+
 static struct i2c_driver ov5645_i2c_driver = {
 	.driver = {
 		.of_match_table = ov5645_of_match,
 		.name  = "ov5645",
+		.pm = &ov5645_pm_ops,
 	},
 	.probe_new = ov5645_probe,
 	.remove = ov5645_remove,
-- 
2.25.1

[PATCH v2 3/5] media: i2c: ov5645: Drop empty comment

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Drop empty multiline comment.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v1->v2
* No change
---
 drivers/media/i2c/ov5645.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index 1551690a94e0..a0b9d0c43b78 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -14,9 +14,6 @@
  *   https://www.mail-archive.com/linux-media%40vger.kernel.org/msg92671.html
  */
 
-/*
- */
-
 #include <linux/bitops.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
-- 
2.25.1

[PATCH v2 4/5] media: i2c: ov5645: Return zero for s_stream(0)

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Always return zero while stopping the stream as the caller will ignore the
return value.

This patch drops checking the return value of ov5645_write_reg() and
continues further in the code path while stopping stream. The user anyway
gets an error message in case ov5645_write_reg() fails.

Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v1->v2
* New patch
---
 drivers/media/i2c/ov5645.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index a0b9d0c43b78..b3825294aaf1 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -995,14 +995,11 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
 		if (ret < 0)
 			goto err_rpm_put;
 	} else {
-		ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
-		if (ret < 0)
-			return ret;
+		ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
+
+		ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
+				 OV5645_SYSTEM_CTRL0_STOP);
 
-		ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
-				       OV5645_SYSTEM_CTRL0_STOP);
-		if (ret < 0)
-			return ret;
 		pm_runtime_put(ov5645->dev);
 	}
 
-- 
2.25.1

[PATCH v2 5/5] media: i2c: ov5645: Call ov5645_entity_init_cfg() before registering the subdev

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Make sure we call ov5645_entity_init_cfg() before registering the subdev
to make sure default formats are set up.

Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v1->v2
* New patch
---
 drivers/media/i2c/ov5645.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index b3825294aaf1..14bcc6e42dd2 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -1212,6 +1212,8 @@ static int ov5645_probe(struct i2c_client *client)
 		goto err_pm_runtime;
 	}
 
+	ov5645_entity_init_cfg(&ov5645->sd, NULL);
+
 	ret = v4l2_async_register_subdev(&ov5645->sd);
 	if (ret < 0) {
 		dev_err(dev, "could not register v4l2 device\n");
@@ -1224,8 +1226,6 @@ static int ov5645_probe(struct i2c_client *client)
 	pm_runtime_use_autosuspend(dev);
 	pm_runtime_put_autosuspend(dev);
 
-	ov5645_entity_init_cfg(&ov5645->sd, NULL);
-
 	return 0;
 
 err_pm_runtime:
-- 
2.25.1

Re: [PATCH v2 0/5] media: i2c: ov5645 driver enhancements

[Lad, Prabhakar]

Hi Sakari,

On Fri, Oct 14, 2022 at 7:35 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
>
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Hi All,
>
> The main aim of this series is to add PM support to the sensor driver.
>
> I had two more patches [0] and [1] which were for ov5645, so instead
> sending them separately I have clubbed them as series.
>
> v1-> v2
> - patch #1 is infact a v3 [1] no changes
> - patch #2 fixed review comments pointed by Sakari
> - patch #3 [0] no changes
> - patches #4 and #5 are new
>
> [0] https://patchwork.linuxtv.org/project/linux-media/patch/20220927202005.750621-1-prabhakar.mahadev-lad.rj@bp.renesas.com/
> [1] https://patchwork.linuxtv.org/project/linux-media/patch/20220919153540.178732-1-prabhakar.mahadev-lad.rj@bp.renesas.com/
>
> Cheers,
> Prabhakar
>
> Lad Prabhakar (5):
>   media: dt-bindings: ov5645: Convert OV5645 binding to a schema
>   media: i2c: ov5645: Use runtime PM
>   media: i2c: ov5645: Drop empty comment
>   media: i2c: ov5645: Return zero for s_stream(0)
>   media: i2c: ov5645: Call ov5645_entity_init_cfg() before registering
>     the subdev
>
After sending this series I realized I had an additional patch [0] for
ov5645 which I should have tagged along with the series. Can you
please pick [0] while reviewing this series.

[0] https://patchwork.kernel.org/project/linux-media/patch/20220919143350.176746-1-prabhakar.mahadev-lad.rj@bp.renesas.com/

Cheers,
Prabhakar

Re: [PATCH v2 2/5] media: i2c: ov5645: Use runtime PM

[Sakari Ailus]

Hi Prabhakar,

On Fri, Oct 14, 2022 at 07:34:56PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Switch to using runtime PM for power management.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> v1->v2
> * Moved pm_runtime_*_autosuspend() calls after registering the subdev.
> ---
>  drivers/media/i2c/Kconfig  |   2 +-
>  drivers/media/i2c/ov5645.c | 137 +++++++++++++++++++------------------
>  2 files changed, 70 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 7806d4b81716..c0edd1017fe8 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -459,7 +459,7 @@ config VIDEO_OV5640
>  config VIDEO_OV5645
>  	tristate "OmniVision OV5645 sensor support"
>  	depends on OF
> -	depends on I2C && VIDEO_DEV
> +	depends on I2C && PM && VIDEO_DEV
>  	select MEDIA_CONTROLLER
>  	select VIDEO_V4L2_SUBDEV_API
>  	select V4L2_FWNODE
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index 81e4e87e1821..1551690a94e0 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -27,6 +27,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_graph.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
> @@ -108,7 +109,6 @@ struct ov5645 {
>  	u8 timing_tc_reg21;
>  
>  	struct mutex power_lock; /* lock to protect power state */
> -	int power_count;
>  
>  	struct gpio_desc *enable_gpio;
>  	struct gpio_desc *rst_gpio;
> @@ -635,8 +635,24 @@ static int ov5645_set_register_array(struct ov5645 *ov5645,
>  	return 0;
>  }
>  
> -static int ov5645_set_power_on(struct ov5645 *ov5645)
> +static int ov5645_set_power_off(struct device *dev)
>  {
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct ov5645 *ov5645 = to_ov5645(sd);
> +
> +	ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x58);
> +	gpiod_set_value_cansleep(ov5645->rst_gpio, 1);
> +	gpiod_set_value_cansleep(ov5645->enable_gpio, 0);
> +	clk_disable_unprepare(ov5645->xclk);
> +	regulator_bulk_disable(OV5645_NUM_SUPPLIES, ov5645->supplies);
> +
> +	return 0;
> +}
> +
> +static int ov5645_set_power_on(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct ov5645 *ov5645 = to_ov5645(sd);
>  	int ret;
>  
>  	ret = regulator_bulk_enable(OV5645_NUM_SUPPLIES, ov5645->supplies);
> @@ -658,57 +674,19 @@ static int ov5645_set_power_on(struct ov5645 *ov5645)
>  
>  	msleep(20);
>  
> -	return 0;
> -}
> -
> -static void ov5645_set_power_off(struct ov5645 *ov5645)
> -{
> -	gpiod_set_value_cansleep(ov5645->rst_gpio, 1);
> -	gpiod_set_value_cansleep(ov5645->enable_gpio, 0);
> -	clk_disable_unprepare(ov5645->xclk);
> -	regulator_bulk_disable(OV5645_NUM_SUPPLIES, ov5645->supplies);
> -}
> -
> -static int ov5645_s_power(struct v4l2_subdev *sd, int on)
> -{
> -	struct ov5645 *ov5645 = to_ov5645(sd);
> -	int ret = 0;
> -
> -	mutex_lock(&ov5645->power_lock);
> -
> -	/* If the power count is modified from 0 to != 0 or from != 0 to 0,
> -	 * update the power state.
> -	 */
> -	if (ov5645->power_count == !on) {
> -		if (on) {
> -			ret = ov5645_set_power_on(ov5645);
> -			if (ret < 0)
> -				goto exit;
> -
> -			ret = ov5645_set_register_array(ov5645,
> -					ov5645_global_init_setting,
> +	ret = ov5645_set_register_array(ov5645, ov5645_global_init_setting,
>  					ARRAY_SIZE(ov5645_global_init_setting));
> -			if (ret < 0) {
> -				dev_err(ov5645->dev,
> -					"could not set init registers\n");
> -				ov5645_set_power_off(ov5645);
> -				goto exit;
> -			}
> -
> -			usleep_range(500, 1000);
> -		} else {
> -			ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x58);
> -			ov5645_set_power_off(ov5645);
> -		}
> +	if (ret < 0) {
> +		dev_err(ov5645->dev, "could not set init registers\n");
> +		goto exit;
>  	}
>  
> -	/* Update the power count. */
> -	ov5645->power_count += on ? 1 : -1;
> -	WARN_ON(ov5645->power_count < 0);
> +	usleep_range(500, 1000);
>  
> -exit:
> -	mutex_unlock(&ov5645->power_lock);
> +	return 0;
>  
> +exit:
> +	ov5645_set_power_off(dev);
>  	return ret;
>  }
>  
> @@ -795,7 +773,7 @@ static int ov5645_s_ctrl(struct v4l2_ctrl *ctrl)
>  	int ret;
>  
>  	mutex_lock(&ov5645->power_lock);
> -	if (!ov5645->power_count) {
> +	if (!pm_runtime_get_if_in_use(ov5645->dev)) {
>  		mutex_unlock(&ov5645->power_lock);
>  		return 0;
>  	}
> @@ -827,6 +805,7 @@ static int ov5645_s_ctrl(struct v4l2_ctrl *ctrl)
>  		break;
>  	}
>  
> +	pm_runtime_put_autosuspend(ov5645->dev);

I think you'll need pm_runtime_mark_last_busy() before this. I missed this
on the last round. Maybe in probe() too. Feel free to resend just this
patch.

>  	mutex_unlock(&ov5645->power_lock);
>  
>  	return ret;
> @@ -991,6 +970,10 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
>  	int ret;
>  
>  	if (enable) {
> +		ret = pm_runtime_resume_and_get(ov5645->dev);
> +		if (ret < 0)
> +			return ret;
> +
>  		ret = ov5645_set_register_array(ov5645,
>  					ov5645->current_mode->data,
>  					ov5645->current_mode->data_size);
> @@ -998,22 +981,22 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
>  			dev_err(ov5645->dev, "could not set mode %dx%d\n",
>  				ov5645->current_mode->width,
>  				ov5645->current_mode->height);
> -			return ret;
> +			goto err_rpm_put;
>  		}
>  		ret = v4l2_ctrl_handler_setup(&ov5645->ctrls);
>  		if (ret < 0) {
>  			dev_err(ov5645->dev, "could not sync v4l2 controls\n");
> -			return ret;
> +			goto err_rpm_put;
>  		}
>  
>  		ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x45);
>  		if (ret < 0)
> -			return ret;
> +			goto err_rpm_put;
>  
>  		ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
>  				       OV5645_SYSTEM_CTRL0_START);
>  		if (ret < 0)
> -			return ret;
> +			goto err_rpm_put;
>  	} else {
>  		ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
>  		if (ret < 0)
> @@ -1023,14 +1006,15 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
>  				       OV5645_SYSTEM_CTRL0_STOP);
>  		if (ret < 0)
>  			return ret;
> +		pm_runtime_put(ov5645->dev);
>  	}
>  
>  	return 0;
> -}
>  
> -static const struct v4l2_subdev_core_ops ov5645_core_ops = {
> -	.s_power = ov5645_s_power,
> -};
> +err_rpm_put:
> +	pm_runtime_put(ov5645->dev);
> +	return ret;
> +}
>  
>  static const struct v4l2_subdev_video_ops ov5645_video_ops = {
>  	.s_stream = ov5645_s_stream,
> @@ -1046,7 +1030,6 @@ static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
>  };
>  
>  static const struct v4l2_subdev_ops ov5645_subdev_ops = {
> -	.core = &ov5645_core_ops,
>  	.video = &ov5645_video_ops,
>  	.pad = &ov5645_subdev_pad_ops,
>  };
> @@ -1188,11 +1171,9 @@ static int ov5645_probe(struct i2c_client *client)
>  		goto free_ctrl;
>  	}
>  
> -	ret = ov5645_s_power(&ov5645->sd, true);
> -	if (ret < 0) {
> -		dev_err(dev, "could not power up OV5645\n");
> +	ret = ov5645_set_power_on(dev);
> +	if (ret)
>  		goto free_entity;
> -	}
>  
>  	ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_HIGH, &chip_id_high);
>  	if (ret < 0 || chip_id_high != OV5645_CHIP_ID_HIGH_BYTE) {
> @@ -1209,12 +1190,16 @@ static int ov5645_probe(struct i2c_client *client)
>  
>  	dev_info(dev, "OV5645 detected at address 0x%02x\n", client->addr);
>  
> +	pm_runtime_set_active(dev);
> +	pm_runtime_get_noresume(dev);
> +	pm_runtime_enable(dev);
> +
>  	ret = ov5645_read_reg(ov5645, OV5645_AEC_PK_MANUAL,
>  			      &ov5645->aec_pk_manual);
>  	if (ret < 0) {
>  		dev_err(dev, "could not read AEC/AGC mode\n");
>  		ret = -ENODEV;
> -		goto power_down;
> +		goto err_pm_runtime;
>  	}
>  
>  	ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG20,
> @@ -1222,7 +1207,7 @@ static int ov5645_probe(struct i2c_client *client)
>  	if (ret < 0) {
>  		dev_err(dev, "could not read vflip value\n");
>  		ret = -ENODEV;
> -		goto power_down;
> +		goto err_pm_runtime;
>  	}
>  
>  	ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG21,
> @@ -1230,23 +1215,30 @@ static int ov5645_probe(struct i2c_client *client)
>  	if (ret < 0) {
>  		dev_err(dev, "could not read hflip value\n");
>  		ret = -ENODEV;
> -		goto power_down;
> +		goto err_pm_runtime;
>  	}
>  
> -	ov5645_s_power(&ov5645->sd, false);
> -
>  	ret = v4l2_async_register_subdev(&ov5645->sd);
>  	if (ret < 0) {
>  		dev_err(dev, "could not register v4l2 device\n");
> +		pm_runtime_disable(dev);
> +		pm_runtime_set_suspended(dev);
>  		goto free_entity;
>  	}
>  
> +	pm_runtime_set_autosuspend_delay(dev, 1000);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_put_autosuspend(dev);
> +
>  	ov5645_entity_init_cfg(&ov5645->sd, NULL);
>  
>  	return 0;
>  
> +err_pm_runtime:
> +	pm_runtime_disable(dev);
> +	pm_runtime_put_noidle(dev);
>  power_down:
> -	ov5645_s_power(&ov5645->sd, false);
> +	ov5645_set_power_off(dev);
>  free_entity:
>  	media_entity_cleanup(&ov5645->sd.entity);
>  free_ctrl:
> @@ -1264,6 +1256,10 @@ static void ov5645_remove(struct i2c_client *client)
>  	v4l2_async_unregister_subdev(&ov5645->sd);
>  	media_entity_cleanup(&ov5645->sd.entity);
>  	v4l2_ctrl_handler_free(&ov5645->ctrls);
> +	pm_runtime_disable(ov5645->dev);
> +	if (!pm_runtime_status_suspended(ov5645->dev))
> +		ov5645_set_power_off(ov5645->dev);
> +	pm_runtime_set_suspended(ov5645->dev);
>  	mutex_destroy(&ov5645->power_lock);
>  }
>  
> @@ -1279,10 +1275,15 @@ static const struct of_device_id ov5645_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, ov5645_of_match);
>  
> +static const struct dev_pm_ops ov5645_pm_ops = {
> +	SET_RUNTIME_PM_OPS(ov5645_set_power_off, ov5645_set_power_on, NULL)
> +};
> +
>  static struct i2c_driver ov5645_i2c_driver = {
>  	.driver = {
>  		.of_match_table = ov5645_of_match,
>  		.name  = "ov5645",
> +		.pm = &ov5645_pm_ops,
>  	},
>  	.probe_new = ov5645_probe,
>  	.remove = ov5645_remove,

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 1/5] media: dt-bindings: ov5645: Convert OV5645 binding to a schema

[Rob Herring]

On Fri, 14 Oct 2022 19:34:55 +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Convert the simple OV5645 Device Tree binding to json-schema.
> 
> The previous binding marked the below properties as required which was a
> driver requirement and not the device requirement so just drop them from
> the required list during the conversion.
> - clock-frequency
> - enable-gpios
> - reset-gpios
> 
> Also drop the "clock-names" property as we have a single clock source for
> the sensor and the driver has been updated to drop the clk referencing by
> name.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Resend v3:
> * No change
> 
> v2 -> v3
> * Dropped clock-names property
> * Marked power supplies as mandatory
> * Dropped the comment for voltage power supplies
> * Included RB tag from Laurent
> * Driver change to drop clock-names [0]
> 
> [0] https://lore.kernel.org/linux-media/Yyh%2F3uzOJOu3drEB@pendragon.ideasonboard.com/T/#t
> 
> v1 -> v2
> * Dropped ref to video-interface-devices.yaml#
> * Dropped driver specific required items from the list
> * Updated commit message
> * Dropped clock-lanes and bus-type from the port and example node
> * Marked data-lanes as required in port node
> ---
>  .../devicetree/bindings/media/i2c/ov5645.txt  |  54 ---------
>  .../bindings/media/i2c/ovti,ov5645.yaml       | 104 ++++++++++++++++++
>  2 files changed, 104 insertions(+), 54 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5645.yaml
> 

Running 'make dtbs_check' with the schema in this patch gives the
following warnings. Consider if they are expected or the schema is
incorrect. These may not be new warnings.

Note that it is not yet a requirement to have 0 warnings for dtbs_check.
This will change in the future.

Full log is available here: https://patchwork.ozlabs.org/patch/


camera@3c: 'clock-names' does not match any of the regexes: 'pinctrl-[0-9]+'
	arch/arm/boot/dts/imx6dl-pico-dwarf.dtb
	arch/arm/boot/dts/imx6dl-pico-hobbit.dtb
	arch/arm/boot/dts/imx6dl-pico-nymph.dtb
	arch/arm/boot/dts/imx6dl-pico-pi.dtb
	arch/arm/boot/dts/imx6dl-wandboard.dtb
	arch/arm/boot/dts/imx6dl-wandboard-revb1.dtb
	arch/arm/boot/dts/imx6dl-wandboard-revd1.dtb
	arch/arm/boot/dts/imx6q-pico-dwarf.dtb
	arch/arm/boot/dts/imx6q-pico-hobbit.dtb
	arch/arm/boot/dts/imx6q-pico-nymph.dtb
	arch/arm/boot/dts/imx6q-pico-pi.dtb
	arch/arm/boot/dts/imx6qp-wandboard-revd1.dtb
	arch/arm/boot/dts/imx6q-wandboard.dtb
	arch/arm/boot/dts/imx6q-wandboard-revb1.dtb
	arch/arm/boot/dts/imx6q-wandboard-revd1.dtb

ov5645@3c: 'clock-names' does not match any of the regexes: 'pinctrl-[0-9]+'
	arch/arm64/boot/dts/renesas/r8a774a1-hihope-rzg2m-ex-mipi-2.1.dtb
	arch/arm64/boot/dts/renesas/r8a774b1-hihope-rzg2n-ex-mipi-2.1.dtb
	arch/arm64/boot/dts/renesas/r8a774c0-ek874-mipi-2.1.dtb
	arch/arm64/boot/dts/renesas/r8a774e1-hihope-rzg2h-ex-mipi-2.1.dtb

Re: [PATCH v2 1/5] media: dt-bindings: ov5645: Convert OV5645 binding to a schema

[Rob Herring]

On Fri, Oct 14, 2022 at 1:35 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
>
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Convert the simple OV5645 Device Tree binding to json-schema.
>
> The previous binding marked the below properties as required which was a
> driver requirement and not the device requirement so just drop them from
> the required list during the conversion.
> - clock-frequency
> - enable-gpios
> - reset-gpios
>
> Also drop the "clock-names" property as we have a single clock source for
> the sensor and the driver has been updated to drop the clk referencing by
> name.

Driver requirements are the ABI!

This breaks a kernel without the driver change and a DTB that has
dropped the properties.

Also, with 'clock-names' dropped, you've just introduced a bunch of
warnings on other people's platforms. Are you going to 'fix' all of
them?

Rob

Re: [PATCH v2 1/5] media: dt-bindings: ov5645: Convert OV5645 binding to a schema

[Lad, Prabhakar]

Hi Rob,

Thank you for the review.

On Fri, Oct 14, 2022 at 10:05 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Fri, Oct 14, 2022 at 1:35 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> >
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Convert the simple OV5645 Device Tree binding to json-schema.
> >
> > The previous binding marked the below properties as required which was a
> > driver requirement and not the device requirement so just drop them from
> > the required list during the conversion.
> > - clock-frequency
> > - enable-gpios
> > - reset-gpios
> >
> > Also drop the "clock-names" property as we have a single clock source for
> > the sensor and the driver has been updated to drop the clk referencing by
> > name.
>
> Driver requirements are the ABI!
>
> This breaks a kernel without the driver change and a DTB that has
> dropped the properties.
>
I already have a patch for the driver [0] which I missed to include
along with the series.

> Also, with 'clock-names' dropped, you've just introduced a bunch of
> warnings on other people's platforms. Are you going to 'fix' all of
> them?
>
Yes I will fix them, once the patch driver patch [0] is merged in.

[0] https://patchwork.kernel.org/project/linux-media/patch/20220919143350.176746-1-prabhakar.mahadev-lad.rj@bp.renesas.com/

Cheers,
Prabhakar

Re: [PATCH v2 2/5] media: i2c: ov5645: Use runtime PM

[Lad, Prabhakar]

Hi Sakari,

Thank you for the review.

On Fri, Oct 14, 2022 at 8:18 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Prabhakar,
>
> On Fri, Oct 14, 2022 at 07:34:56PM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Switch to using runtime PM for power management.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > v1->v2
> > * Moved pm_runtime_*_autosuspend() calls after registering the subdev.
> > ---
<snip>
> > @@ -795,7 +773,7 @@ static int ov5645_s_ctrl(struct v4l2_ctrl *ctrl)
> >       int ret;
> >
> >       mutex_lock(&ov5645->power_lock);
> > -     if (!ov5645->power_count) {
> > +     if (!pm_runtime_get_if_in_use(ov5645->dev)) {
> >               mutex_unlock(&ov5645->power_lock);
> >               return 0;
> >       }
> > @@ -827,6 +805,7 @@ static int ov5645_s_ctrl(struct v4l2_ctrl *ctrl)
> >               break;
> >       }
> >
> > +     pm_runtime_put_autosuspend(ov5645->dev);
>
> I think you'll need pm_runtime_mark_last_busy() before this. I missed this
> on the last round. Maybe in probe() too. Feel free to resend just this
> patch.
>
Agreed, I'll respin this patch fixing the above.

Cheers,
Prabhakar

Re: [PATCH v2 1/5] media: dt-bindings: ov5645: Convert OV5645 binding to a schema

[Rob Herring]

On Fri, Oct 14, 2022 at 10:27:53PM +0100, Lad, Prabhakar wrote:
> Hi Rob,
> 
> Thank you for the review.
> 
> On Fri, Oct 14, 2022 at 10:05 PM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Fri, Oct 14, 2022 at 1:35 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > >
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >
> > > Convert the simple OV5645 Device Tree binding to json-schema.
> > >
> > > The previous binding marked the below properties as required which was a
> > > driver requirement and not the device requirement so just drop them from
> > > the required list during the conversion.
> > > - clock-frequency
> > > - enable-gpios
> > > - reset-gpios
> > >
> > > Also drop the "clock-names" property as we have a single clock source for
> > > the sensor and the driver has been updated to drop the clk referencing by
> > > name.
> >
> > Driver requirements are the ABI!
> >
> > This breaks a kernel without the driver change and a DTB that has
> > dropped the properties.
> >
> I already have a patch for the driver [0] which I missed to include
> along with the series.

You completely miss the point. Read the first sentence again. Changing 
driver requirements changes the ABI.

This breaks the ABI. The driver patch does not help that.


> > Also, with 'clock-names' dropped, you've just introduced a bunch of
> > warnings on other people's platforms. Are you going to 'fix' all of
> > them?
> >
> Yes I will fix them, once the patch driver patch [0] is merged in.

Why? You are just making extra work. We have enough warnings as-is to 
fix.

Rob

Re: [PATCH v2 1/5] media: dt-bindings: ov5645: Convert OV5645 binding to a schema

[Laurent Pinchart]

Hi Rob,

On Fri, Oct 14, 2022 at 04:40:29PM -0500, Rob Herring wrote:
> On Fri, Oct 14, 2022 at 10:27:53PM +0100, Lad, Prabhakar wrote:
> > On Fri, Oct 14, 2022 at 10:05 PM Rob Herring <robh+dt@kernel.org> wrote:
> > > On Fri, Oct 14, 2022 at 1:35 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > >
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > >
> > > > Convert the simple OV5645 Device Tree binding to json-schema.
> > > >
> > > > The previous binding marked the below properties as required which was a
> > > > driver requirement and not the device requirement so just drop them from
> > > > the required list during the conversion.
> > > > - clock-frequency
> > > > - enable-gpios
> > > > - reset-gpios
> > > >
> > > > Also drop the "clock-names" property as we have a single clock source for
> > > > the sensor and the driver has been updated to drop the clk referencing by
> > > > name.
> > >
> > > Driver requirements are the ABI!
> > >
> > > This breaks a kernel without the driver change and a DTB that has
> > > dropped the properties.
> > >
> > I already have a patch for the driver [0] which I missed to include
> > along with the series.
> 
> You completely miss the point. Read the first sentence again. Changing 
> driver requirements changes the ABI.
> 
> This breaks the ABI. The driver patch does not help that.

I'm not following you here. If the DT binding makes a mandatory property
optional, it doesn't break any existing platform. The only thing that
would not work is a new DT that doesn't contain the now optional
property combined with an older driver that makes it required. That's
not a regression, as it would be a *new* DT.

> > > Also, with 'clock-names' dropped, you've just introduced a bunch of
> > > warnings on other people's platforms. Are you going to 'fix' all of
> > > them?
> > >
> > Yes I will fix them, once the patch driver patch [0] is merged in.
> 
> Why? You are just making extra work. We have enough warnings as-is to 
> fix.

I agree that a DT binding change should patch all in-tree DTS to avoid
introducing new warnings.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/5] media: i2c: ov5645: Use runtime PM

[Laurent Pinchart]

Hello,

On Fri, Oct 14, 2022 at 07:18:11PM +0000, Sakari Ailus wrote:
> On Fri, Oct 14, 2022 at 07:34:56PM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > 
> > Switch to using runtime PM for power management.
> > 
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > v1->v2
> > * Moved pm_runtime_*_autosuspend() calls after registering the subdev.
> > ---
> >  drivers/media/i2c/Kconfig  |   2 +-
> >  drivers/media/i2c/ov5645.c | 137 +++++++++++++++++++------------------
> >  2 files changed, 70 insertions(+), 69 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index 7806d4b81716..c0edd1017fe8 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -459,7 +459,7 @@ config VIDEO_OV5640
> >  config VIDEO_OV5645
> >  	tristate "OmniVision OV5645 sensor support"
> >  	depends on OF
> > -	depends on I2C && VIDEO_DEV
> > +	depends on I2C && PM && VIDEO_DEV
> >  	select MEDIA_CONTROLLER
> >  	select VIDEO_V4L2_SUBDEV_API
> >  	select V4L2_FWNODE
> > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > index 81e4e87e1821..1551690a94e0 100644
> > --- a/drivers/media/i2c/ov5645.c
> > +++ b/drivers/media/i2c/ov5645.c
> > @@ -27,6 +27,7 @@
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/of_graph.h>
> > +#include <linux/pm_runtime.h>
> >  #include <linux/regulator/consumer.h>
> >  #include <linux/slab.h>
> >  #include <linux/types.h>
> > @@ -108,7 +109,6 @@ struct ov5645 {
> >  	u8 timing_tc_reg21;
> >  
> >  	struct mutex power_lock; /* lock to protect power state */
> > -	int power_count;
> >  
> >  	struct gpio_desc *enable_gpio;
> >  	struct gpio_desc *rst_gpio;
> > @@ -635,8 +635,24 @@ static int ov5645_set_register_array(struct ov5645 *ov5645,
> >  	return 0;
> >  }
> >  
> > -static int ov5645_set_power_on(struct ov5645 *ov5645)
> > +static int ov5645_set_power_off(struct device *dev)
> >  {
> > +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > +	struct ov5645 *ov5645 = to_ov5645(sd);
> > +
> > +	ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x58);

I'm not sure this belongs here, but it can be addressed later.

> > +	gpiod_set_value_cansleep(ov5645->rst_gpio, 1);
> > +	gpiod_set_value_cansleep(ov5645->enable_gpio, 0);
> > +	clk_disable_unprepare(ov5645->xclk);
> > +	regulator_bulk_disable(OV5645_NUM_SUPPLIES, ov5645->supplies);
> > +
> > +	return 0;
> > +}
> > +
> > +static int ov5645_set_power_on(struct device *dev)
> > +{
> > +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > +	struct ov5645 *ov5645 = to_ov5645(sd);
> >  	int ret;
> >  
> >  	ret = regulator_bulk_enable(OV5645_NUM_SUPPLIES, ov5645->supplies);
> > @@ -658,57 +674,19 @@ static int ov5645_set_power_on(struct ov5645 *ov5645)
> >  
> >  	msleep(20);
> >  
> > -	return 0;
> > -}
> > -
> > -static void ov5645_set_power_off(struct ov5645 *ov5645)
> > -{
> > -	gpiod_set_value_cansleep(ov5645->rst_gpio, 1);
> > -	gpiod_set_value_cansleep(ov5645->enable_gpio, 0);
> > -	clk_disable_unprepare(ov5645->xclk);
> > -	regulator_bulk_disable(OV5645_NUM_SUPPLIES, ov5645->supplies);
> > -}
> > -
> > -static int ov5645_s_power(struct v4l2_subdev *sd, int on)
> > -{
> > -	struct ov5645 *ov5645 = to_ov5645(sd);
> > -	int ret = 0;
> > -
> > -	mutex_lock(&ov5645->power_lock);
> > -
> > -	/* If the power count is modified from 0 to != 0 or from != 0 to 0,
> > -	 * update the power state.
> > -	 */
> > -	if (ov5645->power_count == !on) {
> > -		if (on) {
> > -			ret = ov5645_set_power_on(ov5645);
> > -			if (ret < 0)
> > -				goto exit;
> > -
> > -			ret = ov5645_set_register_array(ov5645,
> > -					ov5645_global_init_setting,
> > +	ret = ov5645_set_register_array(ov5645, ov5645_global_init_setting,
> >  					ARRAY_SIZE(ov5645_global_init_setting));
> > -			if (ret < 0) {
> > -				dev_err(ov5645->dev,
> > -					"could not set init registers\n");
> > -				ov5645_set_power_off(ov5645);
> > -				goto exit;
> > -			}
> > -
> > -			usleep_range(500, 1000);
> > -		} else {
> > -			ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x58);
> > -			ov5645_set_power_off(ov5645);
> > -		}
> > +	if (ret < 0) {
> > +		dev_err(ov5645->dev, "could not set init registers\n");
> > +		goto exit;
> >  	}
> >  
> > -	/* Update the power count. */
> > -	ov5645->power_count += on ? 1 : -1;
> > -	WARN_ON(ov5645->power_count < 0);
> > +	usleep_range(500, 1000);
> >  
> > -exit:
> > -	mutex_unlock(&ov5645->power_lock);
> > +	return 0;
> >  
> > +exit:
> > +	ov5645_set_power_off(dev);
> >  	return ret;
> >  }
> >  
> > @@ -795,7 +773,7 @@ static int ov5645_s_ctrl(struct v4l2_ctrl *ctrl)
> >  	int ret;
> >  
> >  	mutex_lock(&ov5645->power_lock);
> > -	if (!ov5645->power_count) {
> > +	if (!pm_runtime_get_if_in_use(ov5645->dev)) {
> >  		mutex_unlock(&ov5645->power_lock);
> >  		return 0;
> >  	}
> > @@ -827,6 +805,7 @@ static int ov5645_s_ctrl(struct v4l2_ctrl *ctrl)
> >  		break;
> >  	}
> >  
> > +	pm_runtime_put_autosuspend(ov5645->dev);
> 
> I think you'll need pm_runtime_mark_last_busy() before this. I missed this
> on the last round. Maybe in probe() too. Feel free to resend just this
> patch.
> 
> >  	mutex_unlock(&ov5645->power_lock);
> >  
> >  	return ret;
> > @@ -991,6 +970,10 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
> >  	int ret;
> >  
> >  	if (enable) {
> > +		ret = pm_runtime_resume_and_get(ov5645->dev);
> > +		if (ret < 0)
> > +			return ret;
> > +
> >  		ret = ov5645_set_register_array(ov5645,
> >  					ov5645->current_mode->data,
> >  					ov5645->current_mode->data_size);
> > @@ -998,22 +981,22 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
> >  			dev_err(ov5645->dev, "could not set mode %dx%d\n",
> >  				ov5645->current_mode->width,
> >  				ov5645->current_mode->height);
> > -			return ret;
> > +			goto err_rpm_put;
> >  		}
> >  		ret = v4l2_ctrl_handler_setup(&ov5645->ctrls);
> >  		if (ret < 0) {
> >  			dev_err(ov5645->dev, "could not sync v4l2 controls\n");
> > -			return ret;
> > +			goto err_rpm_put;
> >  		}
> >  
> >  		ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x45);
> >  		if (ret < 0)
> > -			return ret;
> > +			goto err_rpm_put;
> >  
> >  		ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
> >  				       OV5645_SYSTEM_CTRL0_START);
> >  		if (ret < 0)
> > -			return ret;
> > +			goto err_rpm_put;
> >  	} else {
> >  		ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
> >  		if (ret < 0)
> > @@ -1023,14 +1006,15 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
> >  				       OV5645_SYSTEM_CTRL0_STOP);
> >  		if (ret < 0)
> >  			return ret;
> > +		pm_runtime_put(ov5645->dev);
> >  	}
> >  
> >  	return 0;
> > -}
> >  
> > -static const struct v4l2_subdev_core_ops ov5645_core_ops = {
> > -	.s_power = ov5645_s_power,
> > -};
> > +err_rpm_put:
> > +	pm_runtime_put(ov5645->dev);
> > +	return ret;
> > +}
> >  
> >  static const struct v4l2_subdev_video_ops ov5645_video_ops = {
> >  	.s_stream = ov5645_s_stream,
> > @@ -1046,7 +1030,6 @@ static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
> >  };
> >  
> >  static const struct v4l2_subdev_ops ov5645_subdev_ops = {
> > -	.core = &ov5645_core_ops,
> >  	.video = &ov5645_video_ops,
> >  	.pad = &ov5645_subdev_pad_ops,
> >  };
> > @@ -1188,11 +1171,9 @@ static int ov5645_probe(struct i2c_client *client)
> >  		goto free_ctrl;
> >  	}
> >  
> > -	ret = ov5645_s_power(&ov5645->sd, true);
> > -	if (ret < 0) {
> > -		dev_err(dev, "could not power up OV5645\n");
> > +	ret = ov5645_set_power_on(dev);
> > +	if (ret)
> >  		goto free_entity;
> > -	}
> >  
> >  	ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_HIGH, &chip_id_high);
> >  	if (ret < 0 || chip_id_high != OV5645_CHIP_ID_HIGH_BYTE) {
> > @@ -1209,12 +1190,16 @@ static int ov5645_probe(struct i2c_client *client)
> >  
> >  	dev_info(dev, "OV5645 detected at address 0x%02x\n", client->addr);
> >  
> > +	pm_runtime_set_active(dev);
> > +	pm_runtime_get_noresume(dev);
> > +	pm_runtime_enable(dev);
> > +
> >  	ret = ov5645_read_reg(ov5645, OV5645_AEC_PK_MANUAL,
> >  			      &ov5645->aec_pk_manual);

Totally unrelated to this patch, can we drop all these register reads ?
The registers are written through he ov5645_global_init_setting array,
we know what the values are.

> >  	if (ret < 0) {
> >  		dev_err(dev, "could not read AEC/AGC mode\n");
> >  		ret = -ENODEV;
> > -		goto power_down;
> > +		goto err_pm_runtime;
> >  	}
> >  
> >  	ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG20,
> > @@ -1222,7 +1207,7 @@ static int ov5645_probe(struct i2c_client *client)
> >  	if (ret < 0) {
> >  		dev_err(dev, "could not read vflip value\n");
> >  		ret = -ENODEV;
> > -		goto power_down;
> > +		goto err_pm_runtime;
> >  	}
> >  
> >  	ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG21,
> > @@ -1230,23 +1215,30 @@ static int ov5645_probe(struct i2c_client *client)
> >  	if (ret < 0) {
> >  		dev_err(dev, "could not read hflip value\n");
> >  		ret = -ENODEV;
> > -		goto power_down;
> > +		goto err_pm_runtime;
> >  	}
> >  
> > -	ov5645_s_power(&ov5645->sd, false);
> > -
> >  	ret = v4l2_async_register_subdev(&ov5645->sd);
> >  	if (ret < 0) {
> >  		dev_err(dev, "could not register v4l2 device\n");
> > +		pm_runtime_disable(dev);
> > +		pm_runtime_set_suspended(dev);
> >  		goto free_entity;

This looks weird, why do we need special handling of runtime PM here,
instead of just jumping to err_pm_runtime ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> >  	}
> >  
> > +	pm_runtime_set_autosuspend_delay(dev, 1000);
> > +	pm_runtime_use_autosuspend(dev);
> > +	pm_runtime_put_autosuspend(dev);
> > +
> >  	ov5645_entity_init_cfg(&ov5645->sd, NULL);
> >  
> >  	return 0;
> >  
> > +err_pm_runtime:
> > +	pm_runtime_disable(dev);
> > +	pm_runtime_put_noidle(dev);
> >  power_down:
> > -	ov5645_s_power(&ov5645->sd, false);
> > +	ov5645_set_power_off(dev);
> >  free_entity:
> >  	media_entity_cleanup(&ov5645->sd.entity);
> >  free_ctrl:
> > @@ -1264,6 +1256,10 @@ static void ov5645_remove(struct i2c_client *client)
> >  	v4l2_async_unregister_subdev(&ov5645->sd);
> >  	media_entity_cleanup(&ov5645->sd.entity);
> >  	v4l2_ctrl_handler_free(&ov5645->ctrls);
> > +	pm_runtime_disable(ov5645->dev);
> > +	if (!pm_runtime_status_suspended(ov5645->dev))
> > +		ov5645_set_power_off(ov5645->dev);
> > +	pm_runtime_set_suspended(ov5645->dev);
> >  	mutex_destroy(&ov5645->power_lock);
> >  }
> >  
> > @@ -1279,10 +1275,15 @@ static const struct of_device_id ov5645_of_match[] = {
> >  };
> >  MODULE_DEVICE_TABLE(of, ov5645_of_match);
> >  
> > +static const struct dev_pm_ops ov5645_pm_ops = {
> > +	SET_RUNTIME_PM_OPS(ov5645_set_power_off, ov5645_set_power_on, NULL)
> > +};
> > +
> >  static struct i2c_driver ov5645_i2c_driver = {
> >  	.driver = {
> >  		.of_match_table = ov5645_of_match,
> >  		.name  = "ov5645",
> > +		.pm = &ov5645_pm_ops,
> >  	},
> >  	.probe_new = ov5645_probe,
> >  	.remove = ov5645_remove,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/5] media: i2c: ov5645: Drop empty comment

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Fri, Oct 14, 2022 at 07:34:57PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Drop empty multiline comment.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> v1->v2
> * No change
> ---
>  drivers/media/i2c/ov5645.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index 1551690a94e0..a0b9d0c43b78 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -14,9 +14,6 @@
>   *   https://www.mail-archive.com/linux-media%40vger.kernel.org/msg92671.html
>   */
>  
> -/*
> - */
> -
>  #include <linux/bitops.h>
>  #include <linux/clk.h>
>  #include <linux/delay.h>

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/5] media: i2c: ov5645: Use runtime PM

[Laurent Pinchart]

One more comment.

On Sat, Oct 15, 2022 at 09:05:08AM +0300, Laurent Pinchart wrote:
> On Fri, Oct 14, 2022 at 07:18:11PM +0000, Sakari Ailus wrote:
> > On Fri, Oct 14, 2022 at 07:34:56PM +0100, Prabhakar wrote:
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > 
> > > Switch to using runtime PM for power management.
> > > 
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > > v1->v2
> > > * Moved pm_runtime_*_autosuspend() calls after registering the subdev.
> > > ---
> > >  drivers/media/i2c/Kconfig  |   2 +-
> > >  drivers/media/i2c/ov5645.c | 137 +++++++++++++++++++------------------
> > >  2 files changed, 70 insertions(+), 69 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > index 7806d4b81716..c0edd1017fe8 100644
> > > --- a/drivers/media/i2c/Kconfig
> > > +++ b/drivers/media/i2c/Kconfig
> > > @@ -459,7 +459,7 @@ config VIDEO_OV5640
> > >  config VIDEO_OV5645
> > >  	tristate "OmniVision OV5645 sensor support"
> > >  	depends on OF
> > > -	depends on I2C && VIDEO_DEV
> > > +	depends on I2C && PM && VIDEO_DEV
> > >  	select MEDIA_CONTROLLER
> > >  	select VIDEO_V4L2_SUBDEV_API
> > >  	select V4L2_FWNODE
> > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > > index 81e4e87e1821..1551690a94e0 100644
> > > --- a/drivers/media/i2c/ov5645.c
> > > +++ b/drivers/media/i2c/ov5645.c
> > > @@ -27,6 +27,7 @@
> > >  #include <linux/module.h>
> > >  #include <linux/of.h>
> > >  #include <linux/of_graph.h>
> > > +#include <linux/pm_runtime.h>
> > >  #include <linux/regulator/consumer.h>
> > >  #include <linux/slab.h>
> > >  #include <linux/types.h>
> > > @@ -108,7 +109,6 @@ struct ov5645 {
> > >  	u8 timing_tc_reg21;
> > >  
> > >  	struct mutex power_lock; /* lock to protect power state */
> > > -	int power_count;
> > >  
> > >  	struct gpio_desc *enable_gpio;
> > >  	struct gpio_desc *rst_gpio;
> > > @@ -635,8 +635,24 @@ static int ov5645_set_register_array(struct ov5645 *ov5645,
> > >  	return 0;
> > >  }
> > >  
> > > -static int ov5645_set_power_on(struct ov5645 *ov5645)
> > > +static int ov5645_set_power_off(struct device *dev)
> > >  {
> > > +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > > +	struct ov5645 *ov5645 = to_ov5645(sd);
> > > +
> > > +	ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x58);
> 
> I'm not sure this belongs here, but it can be addressed later.
> 
> > > +	gpiod_set_value_cansleep(ov5645->rst_gpio, 1);
> > > +	gpiod_set_value_cansleep(ov5645->enable_gpio, 0);
> > > +	clk_disable_unprepare(ov5645->xclk);
> > > +	regulator_bulk_disable(OV5645_NUM_SUPPLIES, ov5645->supplies);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int ov5645_set_power_on(struct device *dev)
> > > +{
> > > +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > > +	struct ov5645 *ov5645 = to_ov5645(sd);
> > >  	int ret;
> > >  
> > >  	ret = regulator_bulk_enable(OV5645_NUM_SUPPLIES, ov5645->supplies);
> > > @@ -658,57 +674,19 @@ static int ov5645_set_power_on(struct ov5645 *ov5645)
> > >  
> > >  	msleep(20);
> > >  
> > > -	return 0;
> > > -}
> > > -
> > > -static void ov5645_set_power_off(struct ov5645 *ov5645)
> > > -{
> > > -	gpiod_set_value_cansleep(ov5645->rst_gpio, 1);
> > > -	gpiod_set_value_cansleep(ov5645->enable_gpio, 0);
> > > -	clk_disable_unprepare(ov5645->xclk);
> > > -	regulator_bulk_disable(OV5645_NUM_SUPPLIES, ov5645->supplies);
> > > -}
> > > -
> > > -static int ov5645_s_power(struct v4l2_subdev *sd, int on)
> > > -{
> > > -	struct ov5645 *ov5645 = to_ov5645(sd);
> > > -	int ret = 0;
> > > -
> > > -	mutex_lock(&ov5645->power_lock);
> > > -
> > > -	/* If the power count is modified from 0 to != 0 or from != 0 to 0,
> > > -	 * update the power state.
> > > -	 */
> > > -	if (ov5645->power_count == !on) {
> > > -		if (on) {
> > > -			ret = ov5645_set_power_on(ov5645);
> > > -			if (ret < 0)
> > > -				goto exit;
> > > -
> > > -			ret = ov5645_set_register_array(ov5645,
> > > -					ov5645_global_init_setting,
> > > +	ret = ov5645_set_register_array(ov5645, ov5645_global_init_setting,
> > >  					ARRAY_SIZE(ov5645_global_init_setting));
> > > -			if (ret < 0) {
> > > -				dev_err(ov5645->dev,
> > > -					"could not set init registers\n");
> > > -				ov5645_set_power_off(ov5645);
> > > -				goto exit;
> > > -			}
> > > -
> > > -			usleep_range(500, 1000);
> > > -		} else {
> > > -			ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x58);
> > > -			ov5645_set_power_off(ov5645);
> > > -		}
> > > +	if (ret < 0) {
> > > +		dev_err(ov5645->dev, "could not set init registers\n");
> > > +		goto exit;
> > >  	}
> > >  
> > > -	/* Update the power count. */
> > > -	ov5645->power_count += on ? 1 : -1;
> > > -	WARN_ON(ov5645->power_count < 0);
> > > +	usleep_range(500, 1000);
> > >  
> > > -exit:
> > > -	mutex_unlock(&ov5645->power_lock);
> > > +	return 0;
> > >  
> > > +exit:
> > > +	ov5645_set_power_off(dev);
> > >  	return ret;
> > >  }
> > >  
> > > @@ -795,7 +773,7 @@ static int ov5645_s_ctrl(struct v4l2_ctrl *ctrl)
> > >  	int ret;
> > >  
> > >  	mutex_lock(&ov5645->power_lock);
> > > -	if (!ov5645->power_count) {
> > > +	if (!pm_runtime_get_if_in_use(ov5645->dev)) {
> > >  		mutex_unlock(&ov5645->power_lock);
> > >  		return 0;
> > >  	}
> > > @@ -827,6 +805,7 @@ static int ov5645_s_ctrl(struct v4l2_ctrl *ctrl)
> > >  		break;
> > >  	}
> > >  
> > > +	pm_runtime_put_autosuspend(ov5645->dev);
> > 
> > I think you'll need pm_runtime_mark_last_busy() before this. I missed this
> > on the last round. Maybe in probe() too. Feel free to resend just this
> > patch.
> > 
> > >  	mutex_unlock(&ov5645->power_lock);
> > >  
> > >  	return ret;
> > > @@ -991,6 +970,10 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
> > >  	int ret;
> > >  
> > >  	if (enable) {
> > > +		ret = pm_runtime_resume_and_get(ov5645->dev);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +
> > >  		ret = ov5645_set_register_array(ov5645,
> > >  					ov5645->current_mode->data,
> > >  					ov5645->current_mode->data_size);
> > > @@ -998,22 +981,22 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
> > >  			dev_err(ov5645->dev, "could not set mode %dx%d\n",
> > >  				ov5645->current_mode->width,
> > >  				ov5645->current_mode->height);
> > > -			return ret;
> > > +			goto err_rpm_put;
> > >  		}
> > >  		ret = v4l2_ctrl_handler_setup(&ov5645->ctrls);
> > >  		if (ret < 0) {
> > >  			dev_err(ov5645->dev, "could not sync v4l2 controls\n");
> > > -			return ret;
> > > +			goto err_rpm_put;
> > >  		}
> > >  
> > >  		ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x45);
> > >  		if (ret < 0)
> > > -			return ret;
> > > +			goto err_rpm_put;
> > >  
> > >  		ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
> > >  				       OV5645_SYSTEM_CTRL0_START);
> > >  		if (ret < 0)
> > > -			return ret;
> > > +			goto err_rpm_put;
> > >  	} else {
> > >  		ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
> > >  		if (ret < 0)
> > > @@ -1023,14 +1006,15 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
> > >  				       OV5645_SYSTEM_CTRL0_STOP);
> > >  		if (ret < 0)
> > >  			return ret;
> > > +		pm_runtime_put(ov5645->dev);

This should be pm_runtime_put_autosuspend(), with a
pm_runtime_mark_last_busy() call too.

> > >  	}
> > >  
> > >  	return 0;
> > > -}
> > >  
> > > -static const struct v4l2_subdev_core_ops ov5645_core_ops = {
> > > -	.s_power = ov5645_s_power,
> > > -};
> > > +err_rpm_put:
> > > +	pm_runtime_put(ov5645->dev);

Here I would go for pm_runtime_put_sync(), as a failure to start the
stream would benefit from forcing power being cut off before the user
tries again.

> > > +	return ret;
> > > +}
> > >  
> > >  static const struct v4l2_subdev_video_ops ov5645_video_ops = {
> > >  	.s_stream = ov5645_s_stream,
> > > @@ -1046,7 +1030,6 @@ static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
> > >  };
> > >  
> > >  static const struct v4l2_subdev_ops ov5645_subdev_ops = {
> > > -	.core = &ov5645_core_ops,
> > >  	.video = &ov5645_video_ops,
> > >  	.pad = &ov5645_subdev_pad_ops,
> > >  };
> > > @@ -1188,11 +1171,9 @@ static int ov5645_probe(struct i2c_client *client)
> > >  		goto free_ctrl;
> > >  	}
> > >  
> > > -	ret = ov5645_s_power(&ov5645->sd, true);
> > > -	if (ret < 0) {
> > > -		dev_err(dev, "could not power up OV5645\n");
> > > +	ret = ov5645_set_power_on(dev);
> > > +	if (ret)
> > >  		goto free_entity;
> > > -	}
> > >  
> > >  	ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_HIGH, &chip_id_high);
> > >  	if (ret < 0 || chip_id_high != OV5645_CHIP_ID_HIGH_BYTE) {
> > > @@ -1209,12 +1190,16 @@ static int ov5645_probe(struct i2c_client *client)
> > >  
> > >  	dev_info(dev, "OV5645 detected at address 0x%02x\n", client->addr);
> > >  
> > > +	pm_runtime_set_active(dev);
> > > +	pm_runtime_get_noresume(dev);
> > > +	pm_runtime_enable(dev);
> > > +
> > >  	ret = ov5645_read_reg(ov5645, OV5645_AEC_PK_MANUAL,
> > >  			      &ov5645->aec_pk_manual);
> 
> Totally unrelated to this patch, can we drop all these register reads ?
> The registers are written through he ov5645_global_init_setting array,
> we know what the values are.
> 
> > >  	if (ret < 0) {
> > >  		dev_err(dev, "could not read AEC/AGC mode\n");
> > >  		ret = -ENODEV;
> > > -		goto power_down;
> > > +		goto err_pm_runtime;
> > >  	}
> > >  
> > >  	ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG20,
> > > @@ -1222,7 +1207,7 @@ static int ov5645_probe(struct i2c_client *client)
> > >  	if (ret < 0) {
> > >  		dev_err(dev, "could not read vflip value\n");
> > >  		ret = -ENODEV;
> > > -		goto power_down;
> > > +		goto err_pm_runtime;
> > >  	}
> > >  
> > >  	ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG21,
> > > @@ -1230,23 +1215,30 @@ static int ov5645_probe(struct i2c_client *client)
> > >  	if (ret < 0) {
> > >  		dev_err(dev, "could not read hflip value\n");
> > >  		ret = -ENODEV;
> > > -		goto power_down;
> > > +		goto err_pm_runtime;
> > >  	}
> > >  
> > > -	ov5645_s_power(&ov5645->sd, false);
> > > -
> > >  	ret = v4l2_async_register_subdev(&ov5645->sd);
> > >  	if (ret < 0) {
> > >  		dev_err(dev, "could not register v4l2 device\n");
> > > +		pm_runtime_disable(dev);
> > > +		pm_runtime_set_suspended(dev);
> > >  		goto free_entity;
> 
> This looks weird, why do we need special handling of runtime PM here,
> instead of just jumping to err_pm_runtime ?
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > >  	}
> > >  
> > > +	pm_runtime_set_autosuspend_delay(dev, 1000);
> > > +	pm_runtime_use_autosuspend(dev);
> > > +	pm_runtime_put_autosuspend(dev);
> > > +
> > >  	ov5645_entity_init_cfg(&ov5645->sd, NULL);
> > >  
> > >  	return 0;
> > >  
> > > +err_pm_runtime:
> > > +	pm_runtime_disable(dev);
> > > +	pm_runtime_put_noidle(dev);
> > >  power_down:
> > > -	ov5645_s_power(&ov5645->sd, false);
> > > +	ov5645_set_power_off(dev);
> > >  free_entity:
> > >  	media_entity_cleanup(&ov5645->sd.entity);
> > >  free_ctrl:
> > > @@ -1264,6 +1256,10 @@ static void ov5645_remove(struct i2c_client *client)
> > >  	v4l2_async_unregister_subdev(&ov5645->sd);
> > >  	media_entity_cleanup(&ov5645->sd.entity);
> > >  	v4l2_ctrl_handler_free(&ov5645->ctrls);
> > > +	pm_runtime_disable(ov5645->dev);
> > > +	if (!pm_runtime_status_suspended(ov5645->dev))
> > > +		ov5645_set_power_off(ov5645->dev);
> > > +	pm_runtime_set_suspended(ov5645->dev);
> > >  	mutex_destroy(&ov5645->power_lock);
> > >  }
> > >  
> > > @@ -1279,10 +1275,15 @@ static const struct of_device_id ov5645_of_match[] = {
> > >  };
> > >  MODULE_DEVICE_TABLE(of, ov5645_of_match);
> > >  
> > > +static const struct dev_pm_ops ov5645_pm_ops = {
> > > +	SET_RUNTIME_PM_OPS(ov5645_set_power_off, ov5645_set_power_on, NULL)
> > > +};
> > > +
> > >  static struct i2c_driver ov5645_i2c_driver = {
> > >  	.driver = {
> > >  		.of_match_table = ov5645_of_match,
> > >  		.name  = "ov5645",
> > > +		.pm = &ov5645_pm_ops,
> > >  	},
> > >  	.probe_new = ov5645_probe,
> > >  	.remove = ov5645_remove,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 4/5] media: i2c: ov5645: Return zero for s_stream(0)

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Fri, Oct 14, 2022 at 07:34:58PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Always return zero while stopping the stream as the caller will ignore the
> return value.
> 
> This patch drops checking the return value of ov5645_write_reg() and
> continues further in the code path while stopping stream. The user anyway
> gets an error message in case ov5645_write_reg() fails.

Continuing all the way to pm_runtime_put() is fine, but I don't think
the function should return 0. It's not up to the driver to decide if a
failure would be useful to signal to the caller or not.

> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> v1->v2
> * New patch
> ---
>  drivers/media/i2c/ov5645.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index a0b9d0c43b78..b3825294aaf1 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -995,14 +995,11 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
>  		if (ret < 0)
>  			goto err_rpm_put;
>  	} else {
> -		ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
> -		if (ret < 0)
> -			return ret;
> +		ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
> +
> +		ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
> +				 OV5645_SYSTEM_CTRL0_STOP);
>  
> -		ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
> -				       OV5645_SYSTEM_CTRL0_STOP);
> -		if (ret < 0)
> -			return ret;
>  		pm_runtime_put(ov5645->dev);
>  	}
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 5/5] media: i2c: ov5645: Call ov5645_entity_init_cfg() before registering the subdev

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Fri, Oct 14, 2022 at 07:34:59PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Make sure we call ov5645_entity_init_cfg() before registering the subdev
> to make sure default formats are set up.
> 
> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

If you have a few spare cycles, it would be even better to convert the
driver to the subdev active state API :-) You could then drop this call
entirely.

> ---
> v1->v2
> * New patch
> ---
>  drivers/media/i2c/ov5645.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index b3825294aaf1..14bcc6e42dd2 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -1212,6 +1212,8 @@ static int ov5645_probe(struct i2c_client *client)
>  		goto err_pm_runtime;
>  	}
>  
> +	ov5645_entity_init_cfg(&ov5645->sd, NULL);
> +
>  	ret = v4l2_async_register_subdev(&ov5645->sd);
>  	if (ret < 0) {
>  		dev_err(dev, "could not register v4l2 device\n");
> @@ -1224,8 +1226,6 @@ static int ov5645_probe(struct i2c_client *client)
>  	pm_runtime_use_autosuspend(dev);
>  	pm_runtime_put_autosuspend(dev);
>  
> -	ov5645_entity_init_cfg(&ov5645->sd, NULL);
> -
>  	return 0;
>  
>  err_pm_runtime:

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/5] media: dt-bindings: ov5645: Convert OV5645 binding to a schema

[Krzysztof Kozlowski]

On 15/10/2022 01:54, Laurent Pinchart wrote:
> Hi Rob,
> 
> On Fri, Oct 14, 2022 at 04:40:29PM -0500, Rob Herring wrote:
>> On Fri, Oct 14, 2022 at 10:27:53PM +0100, Lad, Prabhakar wrote:
>>> On Fri, Oct 14, 2022 at 10:05 PM Rob Herring <robh+dt@kernel.org> wrote:
>>>> On Fri, Oct 14, 2022 at 1:35 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
>>>>>
>>>>> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>>>>>
>>>>> Convert the simple OV5645 Device Tree binding to json-schema.
>>>>>
>>>>> The previous binding marked the below properties as required which was a
>>>>> driver requirement and not the device requirement so just drop them from
>>>>> the required list during the conversion.
>>>>> - clock-frequency
>>>>> - enable-gpios
>>>>> - reset-gpios
>>>>>
>>>>> Also drop the "clock-names" property as we have a single clock source for
>>>>> the sensor and the driver has been updated to drop the clk referencing by
>>>>> name.
>>>>
>>>> Driver requirements are the ABI!
>>>>
>>>> This breaks a kernel without the driver change and a DTB that has
>>>> dropped the properties.
>>>>
>>> I already have a patch for the driver [0] which I missed to include
>>> along with the series.
>>
>> You completely miss the point. Read the first sentence again. Changing 
>> driver requirements changes the ABI.
>>
>> This breaks the ABI. The driver patch does not help that.
> 
> I'm not following you here. If the DT binding makes a mandatory property
> optional, it doesn't break any existing platform. The only thing that
> would not work is a new DT that doesn't contain the now optional
> property combined with an older driver that makes it required. That's
> not a regression, as it would be a *new* DT.

You're right although in-tree DTS are now not compatible with older
kernels. So it is not only about new DTS, it is about our kernel DTS
which requires new kernel to work.

DTS are exported and used by other systems, thus if someone blindly
takes this new DTS without clock-names, his kernel/OS/bootloader might
stop working.

That is however a more relaxed requirement than kernel ABI against old DTS.

> 
>>>> Also, with 'clock-names' dropped, you've just introduced a bunch of
>>>> warnings on other people's platforms. Are you going to 'fix' all of
>>>> them?
>>>>
>>> Yes I will fix them, once the patch driver patch [0] is merged in.
>>
>> Why? You are just making extra work. We have enough warnings as-is to 
>> fix.
> 
> I agree that a DT binding change should patch all in-tree DTS to avoid
> introducing new warnings.

Yes.

Best regards,
Krzysztof

Re: [PATCH v2 4/5] media: i2c: ov5645: Return zero for s_stream(0)

[Sakari Ailus]

Hi Laurent,

On Sat, Oct 15, 2022 at 09:25:37AM +0300, Laurent Pinchart wrote:
> Hi Prabhakar,
> 
> Thank you for the patch.
> 
> On Fri, Oct 14, 2022 at 07:34:58PM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > 
> > Always return zero while stopping the stream as the caller will ignore the
> > return value.
> > 
> > This patch drops checking the return value of ov5645_write_reg() and
> > continues further in the code path while stopping stream. The user anyway
> > gets an error message in case ov5645_write_reg() fails.
> 
> Continuing all the way to pm_runtime_put() is fine, but I don't think
> the function should return 0. It's not up to the driver to decide if a
> failure would be useful to signal to the caller or not.

If the function returns an error when disabling streaming, what is the
expected power state of the device after this?

The contract between the caller and the callee is that the state is not
changed if there is an error. This is a special case as very few callers
check the return value for streamoff operation and those that do generally
just print something. I've never seen a caller trying to prevent streaming
off in this case, for instance.

Of course we could document that streaming off always counts as succeeded
(e.g. decreasing device's runtime PM usage_count) while it could return an
informational error code. But I wonder if anyone would ever benefit from
that somehow. :-)

> 
> > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > v1->v2
> > * New patch
> > ---
> >  drivers/media/i2c/ov5645.c | 11 ++++-------
> >  1 file changed, 4 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > index a0b9d0c43b78..b3825294aaf1 100644
> > --- a/drivers/media/i2c/ov5645.c
> > +++ b/drivers/media/i2c/ov5645.c
> > @@ -995,14 +995,11 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
> >  		if (ret < 0)
> >  			goto err_rpm_put;
> >  	} else {
> > -		ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
> > -		if (ret < 0)
> > -			return ret;
> > +		ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
> > +
> > +		ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
> > +				 OV5645_SYSTEM_CTRL0_STOP);
> >  
> > -		ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
> > -				       OV5645_SYSTEM_CTRL0_STOP);
> > -		if (ret < 0)
> > -			return ret;
> >  		pm_runtime_put(ov5645->dev);
> >  	}
> >  
> 

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 4/5] media: i2c: ov5645: Return zero for s_stream(0)

[Laurent Pinchart]

Hi Sakari,

On Sat, Oct 15, 2022 at 09:35:12PM +0000, Sakari Ailus wrote:
> On Sat, Oct 15, 2022 at 09:25:37AM +0300, Laurent Pinchart wrote:
> > On Fri, Oct 14, 2022 at 07:34:58PM +0100, Prabhakar wrote:
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > 
> > > Always return zero while stopping the stream as the caller will ignore the
> > > return value.
> > > 
> > > This patch drops checking the return value of ov5645_write_reg() and
> > > continues further in the code path while stopping stream. The user anyway
> > > gets an error message in case ov5645_write_reg() fails.
> > 
> > Continuing all the way to pm_runtime_put() is fine, but I don't think
> > the function should return 0. It's not up to the driver to decide if a
> > failure would be useful to signal to the caller or not.
> 
> If the function returns an error when disabling streaming, what is the
> expected power state of the device after this?

That's up to us to decide :-)

> The contract between the caller and the callee is that the state is not
> changed if there is an error.

For most APIs, but that's not universal.

> This is a special case as very few callers
> check the return value for streamoff operation and those that do generally
> just print something. I've never seen a caller trying to prevent streaming
> off in this case, for instance.

I think the stream off call should proceed and try to power off the
device even if an error occurs along the way, i.e. it shouldn't return
upon the first detected error.

> Of course we could document that streaming off always counts as succeeded
> (e.g. decreasing device's runtime PM usage_count) while it could return an
> informational error code. But I wonder if anyone would ever benefit from
> that somehow. :-)

I think it could be useful to propagate errors up to inform the user
that something wrong happened. That would involve fixing lots of drivers
along the call chain though, so there's no urgency for the ov5645 to do
so, but isn't it better to propagate the error code instead of hiding
the issue ?

> > > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > > v1->v2
> > > * New patch
> > > ---
> > >  drivers/media/i2c/ov5645.c | 11 ++++-------
> > >  1 file changed, 4 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > > index a0b9d0c43b78..b3825294aaf1 100644
> > > --- a/drivers/media/i2c/ov5645.c
> > > +++ b/drivers/media/i2c/ov5645.c
> > > @@ -995,14 +995,11 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
> > >  		if (ret < 0)
> > >  			goto err_rpm_put;
> > >  	} else {
> > > -		ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
> > > -		if (ret < 0)
> > > -			return ret;
> > > +		ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
> > > +
> > > +		ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
> > > +				 OV5645_SYSTEM_CTRL0_STOP);
> > >  
> > > -		ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
> > > -				       OV5645_SYSTEM_CTRL0_STOP);
> > > -		if (ret < 0)
> > > -			return ret;
> > >  		pm_runtime_put(ov5645->dev);
> > >  	}
> > >  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 4/5] media: i2c: ov5645: Return zero for s_stream(0)

[Sakari Ailus]

Hi Laurent,

On Sun, Oct 16, 2022 at 02:23:13AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Sat, Oct 15, 2022 at 09:35:12PM +0000, Sakari Ailus wrote:
> > On Sat, Oct 15, 2022 at 09:25:37AM +0300, Laurent Pinchart wrote:
> > > On Fri, Oct 14, 2022 at 07:34:58PM +0100, Prabhakar wrote:
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > 
> > > > Always return zero while stopping the stream as the caller will ignore the
> > > > return value.
> > > > 
> > > > This patch drops checking the return value of ov5645_write_reg() and
> > > > continues further in the code path while stopping stream. The user anyway
> > > > gets an error message in case ov5645_write_reg() fails.
> > > 
> > > Continuing all the way to pm_runtime_put() is fine, but I don't think
> > > the function should return 0. It's not up to the driver to decide if a
> > > failure would be useful to signal to the caller or not.
> > 
> > If the function returns an error when disabling streaming, what is the
> > expected power state of the device after this?
> 
> That's up to us to decide :-)
> 
> > The contract between the caller and the callee is that the state is not
> > changed if there is an error.
> 
> For most APIs, but that's not universal.
> 
> > This is a special case as very few callers
> > check the return value for streamoff operation and those that do generally
> > just print something. I've never seen a caller trying to prevent streaming
> > off in this case, for instance.
> 
> I think the stream off call should proceed and try to power off the
> device even if an error occurs along the way, i.e. it shouldn't return
> upon the first detected error.
> 
> > Of course we could document that streaming off always counts as succeeded
> > (e.g. decreasing device's runtime PM usage_count) while it could return an
> > informational error code. But I wonder if anyone would ever benefit from
> > that somehow. :-)
> 
> I think it could be useful to propagate errors up to inform the user
> that something wrong happened. That would involve fixing lots of drivers
> along the call chain though, so there's no urgency for the ov5645 to do
> so, but isn't it better to propagate the error code instead of hiding
> the issue ?

I also don't think hiding the issue would be the best thing to do, but that
wouldn't likely be a big problem either.

How about printing a warning in the wrapper while returning zero to the
original caller? This would keep the API intact while still leaving a trace
on something failing. Of course the driver is also free to print whatever
messages it likes.

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 4/5] media: i2c: ov5645: Return zero for s_stream(0)

[Laurent Pinchart]

Hi Sakari,

On Sun, Oct 16, 2022 at 08:19:40PM +0000, Sakari Ailus wrote:
> On Sun, Oct 16, 2022 at 02:23:13AM +0300, Laurent Pinchart wrote:
> > On Sat, Oct 15, 2022 at 09:35:12PM +0000, Sakari Ailus wrote:
> > > On Sat, Oct 15, 2022 at 09:25:37AM +0300, Laurent Pinchart wrote:
> > > > On Fri, Oct 14, 2022 at 07:34:58PM +0100, Prabhakar wrote:
> > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > 
> > > > > Always return zero while stopping the stream as the caller will ignore the
> > > > > return value.
> > > > > 
> > > > > This patch drops checking the return value of ov5645_write_reg() and
> > > > > continues further in the code path while stopping stream. The user anyway
> > > > > gets an error message in case ov5645_write_reg() fails.
> > > > 
> > > > Continuing all the way to pm_runtime_put() is fine, but I don't think
> > > > the function should return 0. It's not up to the driver to decide if a
> > > > failure would be useful to signal to the caller or not.
> > > 
> > > If the function returns an error when disabling streaming, what is the
> > > expected power state of the device after this?
> > 
> > That's up to us to decide :-)
> > 
> > > The contract between the caller and the callee is that the state is not
> > > changed if there is an error.
> > 
> > For most APIs, but that's not universal.
> > 
> > > This is a special case as very few callers
> > > check the return value for streamoff operation and those that do generally
> > > just print something. I've never seen a caller trying to prevent streaming
> > > off in this case, for instance.
> > 
> > I think the stream off call should proceed and try to power off the
> > device even if an error occurs along the way, i.e. it shouldn't return
> > upon the first detected error.
> > 
> > > Of course we could document that streaming off always counts as succeeded
> > > (e.g. decreasing device's runtime PM usage_count) while it could return an
> > > informational error code. But I wonder if anyone would ever benefit from
> > > that somehow. :-)
> > 
> > I think it could be useful to propagate errors up to inform the user
> > that something wrong happened. That would involve fixing lots of drivers
> > along the call chain though, so there's no urgency for the ov5645 to do
> > so, but isn't it better to propagate the error code instead of hiding
> > the issue ?
> 
> I also don't think hiding the issue would be the best thing to do, but that
> wouldn't likely be a big problem either.
> 
> How about printing a warning in the wrapper while returning zero to the
> original caller? This would keep the API intact while still leaving a trace
> on something failing. Of course the driver is also free to print whatever
> messages it likes.

While I think error propagation could be more useful in the long run,
printing a message in the wrapper is a good idea. I like centralized
error handling, it has a tendency to go wrong when left to individual
drivers.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 4/5] media: i2c: ov5645: Return zero for s_stream(0)

[Sakari Ailus]

On Mon, Oct 17, 2022 at 12:03:17AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Sun, Oct 16, 2022 at 08:19:40PM +0000, Sakari Ailus wrote:
> > On Sun, Oct 16, 2022 at 02:23:13AM +0300, Laurent Pinchart wrote:
> > > On Sat, Oct 15, 2022 at 09:35:12PM +0000, Sakari Ailus wrote:
> > > > On Sat, Oct 15, 2022 at 09:25:37AM +0300, Laurent Pinchart wrote:
> > > > > On Fri, Oct 14, 2022 at 07:34:58PM +0100, Prabhakar wrote:
> > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > > 
> > > > > > Always return zero while stopping the stream as the caller will ignore the
> > > > > > return value.
> > > > > > 
> > > > > > This patch drops checking the return value of ov5645_write_reg() and
> > > > > > continues further in the code path while stopping stream. The user anyway
> > > > > > gets an error message in case ov5645_write_reg() fails.
> > > > > 
> > > > > Continuing all the way to pm_runtime_put() is fine, but I don't think
> > > > > the function should return 0. It's not up to the driver to decide if a
> > > > > failure would be useful to signal to the caller or not.
> > > > 
> > > > If the function returns an error when disabling streaming, what is the
> > > > expected power state of the device after this?
> > > 
> > > That's up to us to decide :-)
> > > 
> > > > The contract between the caller and the callee is that the state is not
> > > > changed if there is an error.
> > > 
> > > For most APIs, but that's not universal.
> > > 
> > > > This is a special case as very few callers
> > > > check the return value for streamoff operation and those that do generally
> > > > just print something. I've never seen a caller trying to prevent streaming
> > > > off in this case, for instance.
> > > 
> > > I think the stream off call should proceed and try to power off the
> > > device even if an error occurs along the way, i.e. it shouldn't return
> > > upon the first detected error.
> > > 
> > > > Of course we could document that streaming off always counts as succeeded
> > > > (e.g. decreasing device's runtime PM usage_count) while it could return an
> > > > informational error code. But I wonder if anyone would ever benefit from
> > > > that somehow. :-)
> > > 
> > > I think it could be useful to propagate errors up to inform the user
> > > that something wrong happened. That would involve fixing lots of drivers
> > > along the call chain though, so there's no urgency for the ov5645 to do
> > > so, but isn't it better to propagate the error code instead of hiding
> > > the issue ?
> > 
> > I also don't think hiding the issue would be the best thing to do, but that
> > wouldn't likely be a big problem either.
> > 
> > How about printing a warning in the wrapper while returning zero to the
> > original caller? This would keep the API intact while still leaving a trace
> > on something failing. Of course the driver is also free to print whatever
> > messages it likes.
> 
> While I think error propagation could be more useful in the long run,
> printing a message in the wrapper is a good idea. I like centralized
> error handling, it has a tendency to go wrong when left to individual
> drivers.

I can send a patch...

-- 
Sakari Ailus

Re: [PATCH v2 4/5] media: i2c: ov5645: Return zero for s_stream(0)

[Lad, Prabhakar]

Hi Sakari,

On Mon, Oct 17, 2022 at 8:12 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> On Mon, Oct 17, 2022 at 12:03:17AM +0300, Laurent Pinchart wrote:
> > Hi Sakari,
> >
> > On Sun, Oct 16, 2022 at 08:19:40PM +0000, Sakari Ailus wrote:
> > > On Sun, Oct 16, 2022 at 02:23:13AM +0300, Laurent Pinchart wrote:
> > > > On Sat, Oct 15, 2022 at 09:35:12PM +0000, Sakari Ailus wrote:
> > > > > On Sat, Oct 15, 2022 at 09:25:37AM +0300, Laurent Pinchart wrote:
> > > > > > On Fri, Oct 14, 2022 at 07:34:58PM +0100, Prabhakar wrote:
> > > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > > >
> > > > > > > Always return zero while stopping the stream as the caller will ignore the
> > > > > > > return value.
> > > > > > >
> > > > > > > This patch drops checking the return value of ov5645_write_reg() and
> > > > > > > continues further in the code path while stopping stream. The user anyway
> > > > > > > gets an error message in case ov5645_write_reg() fails.
> > > > > >
> > > > > > Continuing all the way to pm_runtime_put() is fine, but I don't think
> > > > > > the function should return 0. It's not up to the driver to decide if a
> > > > > > failure would be useful to signal to the caller or not.
> > > > >
> > > > > If the function returns an error when disabling streaming, what is the
> > > > > expected power state of the device after this?
> > > >
> > > > That's up to us to decide :-)
> > > >
> > > > > The contract between the caller and the callee is that the state is not
> > > > > changed if there is an error.
> > > >
> > > > For most APIs, but that's not universal.
> > > >
> > > > > This is a special case as very few callers
> > > > > check the return value for streamoff operation and those that do generally
> > > > > just print something. I've never seen a caller trying to prevent streaming
> > > > > off in this case, for instance.
> > > >
> > > > I think the stream off call should proceed and try to power off the
> > > > device even if an error occurs along the way, i.e. it shouldn't return
> > > > upon the first detected error.
> > > >
> > > > > Of course we could document that streaming off always counts as succeeded
> > > > > (e.g. decreasing device's runtime PM usage_count) while it could return an
> > > > > informational error code. But I wonder if anyone would ever benefit from
> > > > > that somehow. :-)
> > > >
> > > > I think it could be useful to propagate errors up to inform the user
> > > > that something wrong happened. That would involve fixing lots of drivers
> > > > along the call chain though, so there's no urgency for the ov5645 to do
> > > > so, but isn't it better to propagate the error code instead of hiding
> > > > the issue ?
> > >
> > > I also don't think hiding the issue would be the best thing to do, but that
> > > wouldn't likely be a big problem either.
> > >
> > > How about printing a warning in the wrapper while returning zero to the
> > > original caller? This would keep the API intact while still leaving a trace
> > > on something failing. Of course the driver is also free to print whatever
> > > messages it likes.
> >
> > While I think error propagation could be more useful in the long run,
> > printing a message in the wrapper is a good idea. I like centralized
> > error handling, it has a tendency to go wrong when left to individual
> > drivers.
>
> I can send a patch...
>
To conclude, for v3 I'll continue down the code path upon error and
then report back the error?

Cheers,
Prabhakar

Re: [PATCH v2 1/5] media: dt-bindings: ov5645: Convert OV5645 binding to a schema

[Lad, Prabhakar]

On Sat, Oct 15, 2022 at 2:17 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 15/10/2022 01:54, Laurent Pinchart wrote:
> > Hi Rob,
> >
> > On Fri, Oct 14, 2022 at 04:40:29PM -0500, Rob Herring wrote:
> >> On Fri, Oct 14, 2022 at 10:27:53PM +0100, Lad, Prabhakar wrote:
> >>> On Fri, Oct 14, 2022 at 10:05 PM Rob Herring <robh+dt@kernel.org> wrote:
> >>>> On Fri, Oct 14, 2022 at 1:35 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> >>>>>
> >>>>> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >>>>>
> >>>>> Convert the simple OV5645 Device Tree binding to json-schema.
> >>>>>
> >>>>> The previous binding marked the below properties as required which was a
> >>>>> driver requirement and not the device requirement so just drop them from
> >>>>> the required list during the conversion.
> >>>>> - clock-frequency
> >>>>> - enable-gpios
> >>>>> - reset-gpios
> >>>>>
> >>>>> Also drop the "clock-names" property as we have a single clock source for
> >>>>> the sensor and the driver has been updated to drop the clk referencing by
> >>>>> name.
> >>>>
> >>>> Driver requirements are the ABI!
> >>>>
> >>>> This breaks a kernel without the driver change and a DTB that has
> >>>> dropped the properties.
> >>>>
> >>> I already have a patch for the driver [0] which I missed to include
> >>> along with the series.
> >>
> >> You completely miss the point. Read the first sentence again. Changing
> >> driver requirements changes the ABI.
> >>
> >> This breaks the ABI. The driver patch does not help that.
> >
> > I'm not following you here. If the DT binding makes a mandatory property
> > optional, it doesn't break any existing platform. The only thing that
> > would not work is a new DT that doesn't contain the now optional
> > property combined with an older driver that makes it required. That's
> > not a regression, as it would be a *new* DT.
>
> You're right although in-tree DTS are now not compatible with older
> kernels. So it is not only about new DTS, it is about our kernel DTS
> which requires new kernel to work.
>
To confirm, we are ok dropping the clock-names property here right?

> DTS are exported and used by other systems, thus if someone blindly
> takes this new DTS without clock-names, his kernel/OS/bootloader might
> stop working.
>
> That is however a more relaxed requirement than kernel ABI against old DTS.
>
> >
> >>>> Also, with 'clock-names' dropped, you've just introduced a bunch of
> >>>> warnings on other people's platforms. Are you going to 'fix' all of
> >>>> them?
> >>>>
> >>> Yes I will fix them, once the patch driver patch [0] is merged in.
> >>
> >> Why? You are just making extra work. We have enough warnings as-is to
> >> fix.
> >
> > I agree that a DT binding change should patch all in-tree DTS to avoid
> > introducing new warnings.
>
> Yes.
>
OK, I'll send dts changes along with this patch series.

Cheers,
Prabhakar

Re: [PATCH v2 5/5] media: i2c: ov5645: Call ov5645_entity_init_cfg() before registering the subdev

[Lad, Prabhakar]

Hi Laurent,

Thank you for the review.

On Sat, Oct 15, 2022 at 7:26 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Fri, Oct 14, 2022 at 07:34:59PM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Make sure we call ov5645_entity_init_cfg() before registering the subdev
> > to make sure default formats are set up.
> >
> > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> If you have a few spare cycles, it would be even better to convert the
> driver to the subdev active state API :-) You could then drop this call
> entirely.
>
For v3 I did think of it, but it looks like I'll need to spend more
time on the subdev state for this driver (as this driver does cropping
which makes use of TRY/ACTIVE). So for v3 I'll keep this patch as and
will work on the subdev state switch in parallel and post when
complete. (Its just I dont want to miss the v6.2 window for RZ/G2L CRU
driver ;-))

Cheers,
Prabhakar

Re: [PATCH v2 2/5] media: i2c: ov5645: Use runtime PM

[Lad, Prabhakar]

Hi Laurent,

Thank you for the review.

On Sat, Oct 15, 2022 at 7:25 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> One more comment.
>
> On Sat, Oct 15, 2022 at 09:05:08AM +0300, Laurent Pinchart wrote:
> > On Fri, Oct 14, 2022 at 07:18:11PM +0000, Sakari Ailus wrote:
> > > On Fri, Oct 14, 2022 at 07:34:56PM +0100, Prabhakar wrote:
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > >
> > > > Switch to using runtime PM for power management.
> > > >
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > ---
> > > > v1->v2
> > > > * Moved pm_runtime_*_autosuspend() calls after registering the subdev.
> > > > ---
> > > >  drivers/media/i2c/Kconfig  |   2 +-
> > > >  drivers/media/i2c/ov5645.c | 137 +++++++++++++++++++------------------
> > > >  2 files changed, 70 insertions(+), 69 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > > index 7806d4b81716..c0edd1017fe8 100644
> > > > --- a/drivers/media/i2c/Kconfig
> > > > +++ b/drivers/media/i2c/Kconfig
> > > > @@ -459,7 +459,7 @@ config VIDEO_OV5640
> > > >  config VIDEO_OV5645
> > > >   tristate "OmniVision OV5645 sensor support"
> > > >   depends on OF
> > > > - depends on I2C && VIDEO_DEV
> > > > + depends on I2C && PM && VIDEO_DEV
> > > >   select MEDIA_CONTROLLER
> > > >   select VIDEO_V4L2_SUBDEV_API
> > > >   select V4L2_FWNODE
> > > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > > > index 81e4e87e1821..1551690a94e0 100644
> > > > --- a/drivers/media/i2c/ov5645.c
> > > > +++ b/drivers/media/i2c/ov5645.c
> > > > @@ -27,6 +27,7 @@
> > > >  #include <linux/module.h>
> > > >  #include <linux/of.h>
> > > >  #include <linux/of_graph.h>
> > > > +#include <linux/pm_runtime.h>
> > > >  #include <linux/regulator/consumer.h>
> > > >  #include <linux/slab.h>
> > > >  #include <linux/types.h>
> > > > @@ -108,7 +109,6 @@ struct ov5645 {
> > > >   u8 timing_tc_reg21;
> > > >
> > > >   struct mutex power_lock; /* lock to protect power state */
> > > > - int power_count;
> > > >
> > > >   struct gpio_desc *enable_gpio;
> > > >   struct gpio_desc *rst_gpio;
> > > > @@ -635,8 +635,24 @@ static int ov5645_set_register_array(struct ov5645 *ov5645,
> > > >   return 0;
> > > >  }
> > > >
> > > > -static int ov5645_set_power_on(struct ov5645 *ov5645)
> > > > +static int ov5645_set_power_off(struct device *dev)
> > > >  {
> > > > + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > > > + struct ov5645 *ov5645 = to_ov5645(sd);
> > > > +
> > > > + ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x58);
> >
> > I'm not sure this belongs here, but it can be addressed later.
> >
> > > > + gpiod_set_value_cansleep(ov5645->rst_gpio, 1);
> > > > + gpiod_set_value_cansleep(ov5645->enable_gpio, 0);
> > > > + clk_disable_unprepare(ov5645->xclk);
> > > > + regulator_bulk_disable(OV5645_NUM_SUPPLIES, ov5645->supplies);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int ov5645_set_power_on(struct device *dev)
> > > > +{
> > > > + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > > > + struct ov5645 *ov5645 = to_ov5645(sd);
> > > >   int ret;
> > > >
> > > >   ret = regulator_bulk_enable(OV5645_NUM_SUPPLIES, ov5645->supplies);
> > > > @@ -658,57 +674,19 @@ static int ov5645_set_power_on(struct ov5645 *ov5645)
> > > >
> > > >   msleep(20);
> > > >
> > > > - return 0;
> > > > -}
> > > > -
> > > > -static void ov5645_set_power_off(struct ov5645 *ov5645)
> > > > -{
> > > > - gpiod_set_value_cansleep(ov5645->rst_gpio, 1);
> > > > - gpiod_set_value_cansleep(ov5645->enable_gpio, 0);
> > > > - clk_disable_unprepare(ov5645->xclk);
> > > > - regulator_bulk_disable(OV5645_NUM_SUPPLIES, ov5645->supplies);
> > > > -}
> > > > -
> > > > -static int ov5645_s_power(struct v4l2_subdev *sd, int on)
> > > > -{
> > > > - struct ov5645 *ov5645 = to_ov5645(sd);
> > > > - int ret = 0;
> > > > -
> > > > - mutex_lock(&ov5645->power_lock);
> > > > -
> > > > - /* If the power count is modified from 0 to != 0 or from != 0 to 0,
> > > > -  * update the power state.
> > > > -  */
> > > > - if (ov5645->power_count == !on) {
> > > > -         if (on) {
> > > > -                 ret = ov5645_set_power_on(ov5645);
> > > > -                 if (ret < 0)
> > > > -                         goto exit;
> > > > -
> > > > -                 ret = ov5645_set_register_array(ov5645,
> > > > -                                 ov5645_global_init_setting,
> > > > + ret = ov5645_set_register_array(ov5645, ov5645_global_init_setting,
> > > >                                   ARRAY_SIZE(ov5645_global_init_setting));
> > > > -                 if (ret < 0) {
> > > > -                         dev_err(ov5645->dev,
> > > > -                                 "could not set init registers\n");
> > > > -                         ov5645_set_power_off(ov5645);
> > > > -                         goto exit;
> > > > -                 }
> > > > -
> > > > -                 usleep_range(500, 1000);
> > > > -         } else {
> > > > -                 ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x58);
> > > > -                 ov5645_set_power_off(ov5645);
> > > > -         }
> > > > + if (ret < 0) {
> > > > +         dev_err(ov5645->dev, "could not set init registers\n");
> > > > +         goto exit;
> > > >   }
> > > >
> > > > - /* Update the power count. */
> > > > - ov5645->power_count += on ? 1 : -1;
> > > > - WARN_ON(ov5645->power_count < 0);
> > > > + usleep_range(500, 1000);
> > > >
> > > > -exit:
> > > > - mutex_unlock(&ov5645->power_lock);
> > > > + return 0;
> > > >
> > > > +exit:
> > > > + ov5645_set_power_off(dev);
> > > >   return ret;
> > > >  }
> > > >
> > > > @@ -795,7 +773,7 @@ static int ov5645_s_ctrl(struct v4l2_ctrl *ctrl)
> > > >   int ret;
> > > >
> > > >   mutex_lock(&ov5645->power_lock);
> > > > - if (!ov5645->power_count) {
> > > > + if (!pm_runtime_get_if_in_use(ov5645->dev)) {
> > > >           mutex_unlock(&ov5645->power_lock);
> > > >           return 0;
> > > >   }
> > > > @@ -827,6 +805,7 @@ static int ov5645_s_ctrl(struct v4l2_ctrl *ctrl)
> > > >           break;
> > > >   }
> > > >
> > > > + pm_runtime_put_autosuspend(ov5645->dev);
> > >
> > > I think you'll need pm_runtime_mark_last_busy() before this. I missed this
> > > on the last round. Maybe in probe() too. Feel free to resend just this
> > > patch.
> > >
> > > >   mutex_unlock(&ov5645->power_lock);
> > > >
> > > >   return ret;
> > > > @@ -991,6 +970,10 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
> > > >   int ret;
> > > >
> > > >   if (enable) {
> > > > +         ret = pm_runtime_resume_and_get(ov5645->dev);
> > > > +         if (ret < 0)
> > > > +                 return ret;
> > > > +
> > > >           ret = ov5645_set_register_array(ov5645,
> > > >                                   ov5645->current_mode->data,
> > > >                                   ov5645->current_mode->data_size);
> > > > @@ -998,22 +981,22 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
> > > >                   dev_err(ov5645->dev, "could not set mode %dx%d\n",
> > > >                           ov5645->current_mode->width,
> > > >                           ov5645->current_mode->height);
> > > > -                 return ret;
> > > > +                 goto err_rpm_put;
> > > >           }
> > > >           ret = v4l2_ctrl_handler_setup(&ov5645->ctrls);
> > > >           if (ret < 0) {
> > > >                   dev_err(ov5645->dev, "could not sync v4l2 controls\n");
> > > > -                 return ret;
> > > > +                 goto err_rpm_put;
> > > >           }
> > > >
> > > >           ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x45);
> > > >           if (ret < 0)
> > > > -                 return ret;
> > > > +                 goto err_rpm_put;
> > > >
> > > >           ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
> > > >                                  OV5645_SYSTEM_CTRL0_START);
> > > >           if (ret < 0)
> > > > -                 return ret;
> > > > +                 goto err_rpm_put;
> > > >   } else {
> > > >           ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
> > > >           if (ret < 0)
> > > > @@ -1023,14 +1006,15 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
> > > >                                  OV5645_SYSTEM_CTRL0_STOP);
> > > >           if (ret < 0)
> > > >                   return ret;
> > > > +         pm_runtime_put(ov5645->dev);
>
> This should be pm_runtime_put_autosuspend(), with a
> pm_runtime_mark_last_busy() call too.
>
OK.

> > > >   }
> > > >
> > > >   return 0;
> > > > -}
> > > >
> > > > -static const struct v4l2_subdev_core_ops ov5645_core_ops = {
> > > > - .s_power = ov5645_s_power,
> > > > -};
> > > > +err_rpm_put:
> > > > + pm_runtime_put(ov5645->dev);
>
> Here I would go for pm_runtime_put_sync(), as a failure to start the
> stream would benefit from forcing power being cut off before the user
> tries again.
>
Agreed.

> > > > + return ret;
> > > > +}
> > > >
> > > >  static const struct v4l2_subdev_video_ops ov5645_video_ops = {
> > > >   .s_stream = ov5645_s_stream,
> > > > @@ -1046,7 +1030,6 @@ static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
> > > >  };
> > > >
> > > >  static const struct v4l2_subdev_ops ov5645_subdev_ops = {
> > > > - .core = &ov5645_core_ops,
> > > >   .video = &ov5645_video_ops,
> > > >   .pad = &ov5645_subdev_pad_ops,
> > > >  };
> > > > @@ -1188,11 +1171,9 @@ static int ov5645_probe(struct i2c_client *client)
> > > >           goto free_ctrl;
> > > >   }
> > > >
> > > > - ret = ov5645_s_power(&ov5645->sd, true);
> > > > - if (ret < 0) {
> > > > -         dev_err(dev, "could not power up OV5645\n");
> > > > + ret = ov5645_set_power_on(dev);
> > > > + if (ret)
> > > >           goto free_entity;
> > > > - }
> > > >
> > > >   ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_HIGH, &chip_id_high);
> > > >   if (ret < 0 || chip_id_high != OV5645_CHIP_ID_HIGH_BYTE) {
> > > > @@ -1209,12 +1190,16 @@ static int ov5645_probe(struct i2c_client *client)
> > > >
> > > >   dev_info(dev, "OV5645 detected at address 0x%02x\n", client->addr);
> > > >
> > > > + pm_runtime_set_active(dev);
> > > > + pm_runtime_get_noresume(dev);
> > > > + pm_runtime_enable(dev);
> > > > +
> > > >   ret = ov5645_read_reg(ov5645, OV5645_AEC_PK_MANUAL,
> > > >                         &ov5645->aec_pk_manual);
> >
> > Totally unrelated to this patch, can we drop all these register reads ?
> > The registers are written through he ov5645_global_init_setting array,
> > we know what the values are.
> >
Indeed, I'll have a closer look while working on the subdev state for
this driver.

> > > >   if (ret < 0) {
> > > >           dev_err(dev, "could not read AEC/AGC mode\n");
> > > >           ret = -ENODEV;
> > > > -         goto power_down;
> > > > +         goto err_pm_runtime;
> > > >   }
> > > >
> > > >   ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG20,
> > > > @@ -1222,7 +1207,7 @@ static int ov5645_probe(struct i2c_client *client)
> > > >   if (ret < 0) {
> > > >           dev_err(dev, "could not read vflip value\n");
> > > >           ret = -ENODEV;
> > > > -         goto power_down;
> > > > +         goto err_pm_runtime;
> > > >   }
> > > >
> > > >   ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG21,
> > > > @@ -1230,23 +1215,30 @@ static int ov5645_probe(struct i2c_client *client)
> > > >   if (ret < 0) {
> > > >           dev_err(dev, "could not read hflip value\n");
> > > >           ret = -ENODEV;
> > > > -         goto power_down;
> > > > +         goto err_pm_runtime;
> > > >   }
> > > >
> > > > - ov5645_s_power(&ov5645->sd, false);
> > > > -
> > > >   ret = v4l2_async_register_subdev(&ov5645->sd);
> > > >   if (ret < 0) {
> > > >           dev_err(dev, "could not register v4l2 device\n");
> > > > +         pm_runtime_disable(dev);
> > > > +         pm_runtime_set_suspended(dev);
> > > >           goto free_entity;
> >
> > This looks weird, why do we need special handling of runtime PM here,
> > instead of just jumping to err_pm_runtime ?
> >
Agreed, I have fixed that now.

> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
Cheers,
Prabhakar

Re: [PATCH v2 1/5] media: dt-bindings: ov5645: Convert OV5645 binding to a schema

[Rob Herring]

On Sat, Oct 15, 2022 at 12:54 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Rob,
>
> On Fri, Oct 14, 2022 at 04:40:29PM -0500, Rob Herring wrote:
> > On Fri, Oct 14, 2022 at 10:27:53PM +0100, Lad, Prabhakar wrote:
> > > On Fri, Oct 14, 2022 at 10:05 PM Rob Herring <robh+dt@kernel.org> wrote:
> > > > On Fri, Oct 14, 2022 at 1:35 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > > >
> > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > >
> > > > > Convert the simple OV5645 Device Tree binding to json-schema.
> > > > >
> > > > > The previous binding marked the below properties as required which was a
> > > > > driver requirement and not the device requirement so just drop them from
> > > > > the required list during the conversion.
> > > > > - clock-frequency
> > > > > - enable-gpios
> > > > > - reset-gpios
> > > > >
> > > > > Also drop the "clock-names" property as we have a single clock source for
> > > > > the sensor and the driver has been updated to drop the clk referencing by
> > > > > name.
> > > >
> > > > Driver requirements are the ABI!
> > > >
> > > > This breaks a kernel without the driver change and a DTB that has
> > > > dropped the properties.
> > > >
> > > I already have a patch for the driver [0] which I missed to include
> > > along with the series.
> >
> > You completely miss the point. Read the first sentence again. Changing
> > driver requirements changes the ABI.
> >
> > This breaks the ABI. The driver patch does not help that.
>
> I'm not following you here. If the DT binding makes a mandatory property
> optional, it doesn't break any existing platform. The only thing that
> would not work is a new DT that doesn't contain the now optional
> property combined with an older driver that makes it required. That's
> not a regression, as it would be a *new* DT.
>
> > > > Also, with 'clock-names' dropped, you've just introduced a bunch of
> > > > warnings on other people's platforms. Are you going to 'fix' all of
> > > > them?
> > > >
> > > Yes I will fix them, once the patch driver patch [0] is merged in.
> >
> > Why? You are just making extra work. We have enough warnings as-is to
> > fix.
>
> I agree that a DT binding change should patch all in-tree DTS to avoid
> introducing new warnings.

That is not what I was saying. Why not just keep 'clock-names' and go
spend the DTS fixing time fixing some other warnings that we already
have. Also, there is no requirement that converting bindings also fix
DTS files. The only wish is that any warnings we do see are ones
deemed needing to be fixed in the DTS file.

Anyways, there's patches now for the new warnings, so nevermind on this one.

Rob

Re: [PATCH v2 2/5] media: i2c: ov5645: Use runtime PM

[Sakari Ailus]

Hi Prabhakar,

One more comment.

On Fri, Oct 14, 2022 at 07:34:56PM +0100, Prabhakar wrote:
> @@ -1209,12 +1190,16 @@ static int ov5645_probe(struct i2c_client *client)
>  
>  	dev_info(dev, "OV5645 detected at address 0x%02x\n", client->addr);
>  
> +	pm_runtime_set_active(dev);
> +	pm_runtime_get_noresume(dev);
> +	pm_runtime_enable(dev);

You won't gain anything by eanbling runtime PM here. Just move it to the
end of the function before the rest of the calls. Error handling becomes
more simple.

> +
>  	ret = ov5645_read_reg(ov5645, OV5645_AEC_PK_MANUAL,
>  			      &ov5645->aec_pk_manual);
>  	if (ret < 0) {
>  		dev_err(dev, "could not read AEC/AGC mode\n");
>  		ret = -ENODEV;
> -		goto power_down;
> +		goto err_pm_runtime;
>  	}
>  
>  	ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG20,
> @@ -1222,7 +1207,7 @@ static int ov5645_probe(struct i2c_client *client)
>  	if (ret < 0) {
>  		dev_err(dev, "could not read vflip value\n");
>  		ret = -ENODEV;
> -		goto power_down;
> +		goto err_pm_runtime;
>  	}
>  
>  	ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG21,
> @@ -1230,23 +1215,30 @@ static int ov5645_probe(struct i2c_client *client)
>  	if (ret < 0) {
>  		dev_err(dev, "could not read hflip value\n");
>  		ret = -ENODEV;
> -		goto power_down;
> +		goto err_pm_runtime;
>  	}
>  
> -	ov5645_s_power(&ov5645->sd, false);
> -
>  	ret = v4l2_async_register_subdev(&ov5645->sd);
>  	if (ret < 0) {
>  		dev_err(dev, "could not register v4l2 device\n");
> +		pm_runtime_disable(dev);
> +		pm_runtime_set_suspended(dev);
>  		goto free_entity;
>  	}
>  
> +	pm_runtime_set_autosuspend_delay(dev, 1000);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_put_autosuspend(dev);
> +
>  	ov5645_entity_init_cfg(&ov5645->sd, NULL);
>  
>  	return 0;
>  
> +err_pm_runtime:
> +	pm_runtime_disable(dev);
> +	pm_runtime_put_noidle(dev);
>  power_down:
> -	ov5645_s_power(&ov5645->sd, false);
> +	ov5645_set_power_off(dev);
>  free_entity:
>  	media_entity_cleanup(&ov5645->sd.entity);
>  free_ctrl:

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 2/5] media: i2c: ov5645: Use runtime PM

[Lad, Prabhakar]

Hi Sakari,

On Thu, Oct 27, 2022 at 12:20 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Prabhakar,
>
> One more comment.
>
> On Fri, Oct 14, 2022 at 07:34:56PM +0100, Prabhakar wrote:
> > @@ -1209,12 +1190,16 @@ static int ov5645_probe(struct i2c_client *client)
> >
> >       dev_info(dev, "OV5645 detected at address 0x%02x\n", client->addr);
> >
> > +     pm_runtime_set_active(dev);
> > +     pm_runtime_get_noresume(dev);
> > +     pm_runtime_enable(dev);
>
> You won't gain anything by eanbling runtime PM here. Just move it to the
> end of the function before the rest of the calls. Error handling becomes
> more simple.
>
If I move the above calls below I get the below warning:

[    2.633386] ov5645 0-003c: Runtime PM usage count underflow!

This is because of the last patch which moves ov5645_entity_init_cfg()
before registering the subdev. ov5645_entity_init_cfg() calls s_ctrl
due to which we are seeing the above message. Please let me know how
to proceed on this.

Cheers,
Prabhakar

Re: [PATCH v2 2/5] media: i2c: ov5645: Use runtime PM

[Sakari Ailus]

Hi Prabhakar,

On Thu, Oct 27, 2022 at 01:01:52PM +0100, Lad, Prabhakar wrote:
> Hi Sakari,
> 
> On Thu, Oct 27, 2022 at 12:20 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Prabhakar,
> >
> > One more comment.
> >
> > On Fri, Oct 14, 2022 at 07:34:56PM +0100, Prabhakar wrote:
> > > @@ -1209,12 +1190,16 @@ static int ov5645_probe(struct i2c_client *client)
> > >
> > >       dev_info(dev, "OV5645 detected at address 0x%02x\n", client->addr);
> > >
> > > +     pm_runtime_set_active(dev);
> > > +     pm_runtime_get_noresume(dev);
> > > +     pm_runtime_enable(dev);
> >
> > You won't gain anything by eanbling runtime PM here. Just move it to the
> > end of the function before the rest of the calls. Error handling becomes
> > more simple.
> >
> If I move the above calls below I get the below warning:
> 
> [    2.633386] ov5645 0-003c: Runtime PM usage count underflow!
> 
> This is because of the last patch which moves ov5645_entity_init_cfg()
> before registering the subdev. ov5645_entity_init_cfg() calls s_ctrl
> due to which we are seeing the above message. Please let me know how
> to proceed on this.

Ah. Yes, this is a problem with the usage pattern of
pm_runtime_get_if_in_use(). But please don't change that.

You can still move enabling runtime PM later in the function.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 2/5] media: i2c: ov5645: Use runtime PM

[Lad, Prabhakar]

Hi Sakari,

On Thu, Oct 27, 2022 at 1:47 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Prabhakar,
>
> On Thu, Oct 27, 2022 at 01:01:52PM +0100, Lad, Prabhakar wrote:
> > Hi Sakari,
> >
> > On Thu, Oct 27, 2022 at 12:20 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Prabhakar,
> > >
> > > One more comment.
> > >
> > > On Fri, Oct 14, 2022 at 07:34:56PM +0100, Prabhakar wrote:
> > > > @@ -1209,12 +1190,16 @@ static int ov5645_probe(struct i2c_client *client)
> > > >
> > > >       dev_info(dev, "OV5645 detected at address 0x%02x\n", client->addr);
> > > >
> > > > +     pm_runtime_set_active(dev);
> > > > +     pm_runtime_get_noresume(dev);
> > > > +     pm_runtime_enable(dev);
> > >
> > > You won't gain anything by eanbling runtime PM here. Just move it to the
> > > end of the function before the rest of the calls. Error handling becomes
> > > more simple.
> > >
> > If I move the above calls below I get the below warning:
> >
> > [    2.633386] ov5645 0-003c: Runtime PM usage count underflow!
> >
> > This is because of the last patch which moves ov5645_entity_init_cfg()
> > before registering the subdev. ov5645_entity_init_cfg() calls s_ctrl
> > due to which we are seeing the above message. Please let me know how
> > to proceed on this.
>
> Ah. Yes, this is a problem with the usage pattern of
> pm_runtime_get_if_in_use(). But please don't change that.
>
> You can still move enabling runtime PM later in the function.
>
Agreed, the final version looks like below:

    pm_runtime_set_active(dev);
    pm_runtime_get_noresume(dev);

    ov5645_entity_init_cfg(&ov5645->sd, NULL);

    ret = v4l2_async_register_subdev(&ov5645->sd);
    if (ret < 0) {
        dev_err(dev, "could not register v4l2 device\n");
        goto err_pm_runtime;
    }

    pm_runtime_set_autosuspend_delay(dev, 1000);
    pm_runtime_use_autosuspend(dev);
    pm_runtime_enable(dev);


Cheers,
Prabhakar

Re: [PATCH v2 2/5] media: i2c: ov5645: Use runtime PM

[Sakari Ailus]

Hi Prabhakar,

On Thu, Oct 27, 2022 at 05:32:07PM +0100, Lad, Prabhakar wrote:
> Hi Sakari,
> 
> On Thu, Oct 27, 2022 at 1:47 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Prabhakar,
> >
> > On Thu, Oct 27, 2022 at 01:01:52PM +0100, Lad, Prabhakar wrote:
> > > Hi Sakari,
> > >
> > > On Thu, Oct 27, 2022 at 12:20 PM Sakari Ailus
> > > <sakari.ailus@linux.intel.com> wrote:
> > > >
> > > > Hi Prabhakar,
> > > >
> > > > One more comment.
> > > >
> > > > On Fri, Oct 14, 2022 at 07:34:56PM +0100, Prabhakar wrote:
> > > > > @@ -1209,12 +1190,16 @@ static int ov5645_probe(struct i2c_client *client)
> > > > >
> > > > >       dev_info(dev, "OV5645 detected at address 0x%02x\n", client->addr);
> > > > >
> > > > > +     pm_runtime_set_active(dev);
> > > > > +     pm_runtime_get_noresume(dev);
> > > > > +     pm_runtime_enable(dev);
> > > >
> > > > You won't gain anything by eanbling runtime PM here. Just move it to the
> > > > end of the function before the rest of the calls. Error handling becomes
> > > > more simple.
> > > >
> > > If I move the above calls below I get the below warning:
> > >
> > > [    2.633386] ov5645 0-003c: Runtime PM usage count underflow!
> > >
> > > This is because of the last patch which moves ov5645_entity_init_cfg()
> > > before registering the subdev. ov5645_entity_init_cfg() calls s_ctrl
> > > due to which we are seeing the above message. Please let me know how
> > > to proceed on this.
> >
> > Ah. Yes, this is a problem with the usage pattern of
> > pm_runtime_get_if_in_use(). But please don't change that.
> >
> > You can still move enabling runtime PM later in the function.
> >
> Agreed, the final version looks like below:
> 
>     pm_runtime_set_active(dev);
>     pm_runtime_get_noresume(dev);
> 

You'll have to enable runtime PM here, before pm_runtime_get_if_in_use()
gets called.

I'll see if it could be made to work in a sensible way when runtime PM
isn't enabled yet.

>     ov5645_entity_init_cfg(&ov5645->sd, NULL);
> 
>     ret = v4l2_async_register_subdev(&ov5645->sd);
>     if (ret < 0) {
>         dev_err(dev, "could not register v4l2 device\n");
>         goto err_pm_runtime;
>     }
> 
>     pm_runtime_set_autosuspend_delay(dev, 1000);
>     pm_runtime_use_autosuspend(dev);
>     pm_runtime_enable(dev);

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 2/5] media: i2c: ov5645: Use runtime PM

[Lad, Prabhakar]

Hi Sakari,

On Thu, Oct 27, 2022 at 7:45 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Prabhakar,
>
> On Thu, Oct 27, 2022 at 05:32:07PM +0100, Lad, Prabhakar wrote:
> > Hi Sakari,
> >
> > On Thu, Oct 27, 2022 at 1:47 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Prabhakar,
> > >
> > > On Thu, Oct 27, 2022 at 01:01:52PM +0100, Lad, Prabhakar wrote:
> > > > Hi Sakari,
> > > >
> > > > On Thu, Oct 27, 2022 at 12:20 PM Sakari Ailus
> > > > <sakari.ailus@linux.intel.com> wrote:
> > > > >
> > > > > Hi Prabhakar,
> > > > >
> > > > > One more comment.
> > > > >
> > > > > On Fri, Oct 14, 2022 at 07:34:56PM +0100, Prabhakar wrote:
> > > > > > @@ -1209,12 +1190,16 @@ static int ov5645_probe(struct i2c_client *client)
> > > > > >
> > > > > >       dev_info(dev, "OV5645 detected at address 0x%02x\n", client->addr);
> > > > > >
> > > > > > +     pm_runtime_set_active(dev);
> > > > > > +     pm_runtime_get_noresume(dev);
> > > > > > +     pm_runtime_enable(dev);
> > > > >
> > > > > You won't gain anything by eanbling runtime PM here. Just move it to the
> > > > > end of the function before the rest of the calls. Error handling becomes
> > > > > more simple.
> > > > >
> > > > If I move the above calls below I get the below warning:
> > > >
> > > > [    2.633386] ov5645 0-003c: Runtime PM usage count underflow!
> > > >
> > > > This is because of the last patch which moves ov5645_entity_init_cfg()
> > > > before registering the subdev. ov5645_entity_init_cfg() calls s_ctrl
> > > > due to which we are seeing the above message. Please let me know how
> > > > to proceed on this.
> > >
> > > Ah. Yes, this is a problem with the usage pattern of
> > > pm_runtime_get_if_in_use(). But please don't change that.
> > >
> > > You can still move enabling runtime PM later in the function.
> > >
> > Agreed, the final version looks like below:
> >
> >     pm_runtime_set_active(dev);
> >     pm_runtime_get_noresume(dev);
> >
>
> You'll have to enable runtime PM here, before pm_runtime_get_if_in_use()
> gets called.
>
> I'll see if it could be made to work in a sensible way when runtime PM
> isn't enabled yet.
>
Agreed, I'll send out v3 after fixing the comments.

Cheers,
Prabhakar

Re: [PATCH v2 2/5] media: i2c: ov5645: Use runtime PM

[Sakari Ailus]

Hi Prabhakar,

On Thu, Oct 27, 2022 at 06:37:58PM +0000, Sakari Ailus wrote:
> Hi Prabhakar,
> 
> On Thu, Oct 27, 2022 at 05:32:07PM +0100, Lad, Prabhakar wrote:
> > Hi Sakari,
> > 
> > On Thu, Oct 27, 2022 at 1:47 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Prabhakar,
> > >
> > > On Thu, Oct 27, 2022 at 01:01:52PM +0100, Lad, Prabhakar wrote:
> > > > Hi Sakari,
> > > >
> > > > On Thu, Oct 27, 2022 at 12:20 PM Sakari Ailus
> > > > <sakari.ailus@linux.intel.com> wrote:
> > > > >
> > > > > Hi Prabhakar,
> > > > >
> > > > > One more comment.
> > > > >
> > > > > On Fri, Oct 14, 2022 at 07:34:56PM +0100, Prabhakar wrote:
> > > > > > @@ -1209,12 +1190,16 @@ static int ov5645_probe(struct i2c_client *client)
> > > > > >
> > > > > >       dev_info(dev, "OV5645 detected at address 0x%02x\n", client->addr);
> > > > > >
> > > > > > +     pm_runtime_set_active(dev);
> > > > > > +     pm_runtime_get_noresume(dev);
> > > > > > +     pm_runtime_enable(dev);
> > > > >
> > > > > You won't gain anything by eanbling runtime PM here. Just move it to the
> > > > > end of the function before the rest of the calls. Error handling becomes
> > > > > more simple.
> > > > >
> > > > If I move the above calls below I get the below warning:
> > > >
> > > > [    2.633386] ov5645 0-003c: Runtime PM usage count underflow!
> > > >
> > > > This is because of the last patch which moves ov5645_entity_init_cfg()
> > > > before registering the subdev. ov5645_entity_init_cfg() calls s_ctrl
> > > > due to which we are seeing the above message. Please let me know how
> > > > to proceed on this.
> > >
> > > Ah. Yes, this is a problem with the usage pattern of
> > > pm_runtime_get_if_in_use(). But please don't change that.
> > >
> > > You can still move enabling runtime PM later in the function.
> > >
> > Agreed, the final version looks like below:
> > 
> >     pm_runtime_set_active(dev);
> >     pm_runtime_get_noresume(dev);
> > 
> 
> You'll have to enable runtime PM here, before pm_runtime_get_if_in_use()
> gets called.
> 
> I'll see if it could be made to work in a sensible way when runtime PM
> isn't enabled yet.

There are various ways how runtime PM interface functions generally work,
and generally return an error when runtime PM is disabled. Incrementing the
usage_count when runtime PM is disabled would make
pm_runtime_get_if_in_use() very special and not match what the rest would
do. Therefore I think it's best to keep this in the driver. After all, mo
other driver needs this in the media tree, which is the major user of the
function.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 2/5] media: i2c: ov5645: Use runtime PM

[Lad, Prabhakar]

Hi Sakari,

On Mon, Oct 31, 2022 at 2:25 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Prabhakar,
>
> On Thu, Oct 27, 2022 at 06:37:58PM +0000, Sakari Ailus wrote:
> > Hi Prabhakar,
> >
> > On Thu, Oct 27, 2022 at 05:32:07PM +0100, Lad, Prabhakar wrote:
> > > Hi Sakari,
> > >
> > > On Thu, Oct 27, 2022 at 1:47 PM Sakari Ailus
> > > <sakari.ailus@linux.intel.com> wrote:
> > > >
> > > > Hi Prabhakar,
> > > >
> > > > On Thu, Oct 27, 2022 at 01:01:52PM +0100, Lad, Prabhakar wrote:
> > > > > Hi Sakari,
> > > > >
> > > > > On Thu, Oct 27, 2022 at 12:20 PM Sakari Ailus
> > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > >
> > > > > > Hi Prabhakar,
> > > > > >
> > > > > > One more comment.
> > > > > >
> > > > > > On Fri, Oct 14, 2022 at 07:34:56PM +0100, Prabhakar wrote:
> > > > > > > @@ -1209,12 +1190,16 @@ static int ov5645_probe(struct i2c_client *client)
> > > > > > >
> > > > > > >       dev_info(dev, "OV5645 detected at address 0x%02x\n", client->addr);
> > > > > > >
> > > > > > > +     pm_runtime_set_active(dev);
> > > > > > > +     pm_runtime_get_noresume(dev);
> > > > > > > +     pm_runtime_enable(dev);
> > > > > >
> > > > > > You won't gain anything by eanbling runtime PM here. Just move it to the
> > > > > > end of the function before the rest of the calls. Error handling becomes
> > > > > > more simple.
> > > > > >
> > > > > If I move the above calls below I get the below warning:
> > > > >
> > > > > [    2.633386] ov5645 0-003c: Runtime PM usage count underflow!
> > > > >
> > > > > This is because of the last patch which moves ov5645_entity_init_cfg()
> > > > > before registering the subdev. ov5645_entity_init_cfg() calls s_ctrl
> > > > > due to which we are seeing the above message. Please let me know how
> > > > > to proceed on this.
> > > >
> > > > Ah. Yes, this is a problem with the usage pattern of
> > > > pm_runtime_get_if_in_use(). But please don't change that.
> > > >
> > > > You can still move enabling runtime PM later in the function.
> > > >
> > > Agreed, the final version looks like below:
> > >
> > >     pm_runtime_set_active(dev);
> > >     pm_runtime_get_noresume(dev);
> > >
> >
> > You'll have to enable runtime PM here, before pm_runtime_get_if_in_use()
> > gets called.
> >
> > I'll see if it could be made to work in a sensible way when runtime PM
> > isn't enabled yet.
>
> There are various ways how runtime PM interface functions generally work,
> and generally return an error when runtime PM is disabled. Incrementing the
> usage_count when runtime PM is disabled would make
> pm_runtime_get_if_in_use() very special and not match what the rest would
> do. Therefore I think it's best to keep this in the driver. After all, mo
> other driver needs this in the media tree, which is the major user of the
> function.
>
Thank you for digging deep into this. I'll keep it as is and send a v3.

Cheers,
Prabhakar