[PATCH v2 0/6] media: i2c: og01a1b: Add OF support to OmniVision OG01A1B

[PATCH v2 0/6] media: i2c: og01a1b: Add OF support to OmniVision OG01A1B

[Vladimir Zapolskiy]

The change adds basic support of OmniVision OG01A1B image sensor on OF
platforms, and a few follow-up changes from the series extend runtime power
management support.

Previous version of the change is found as a shorter patchset:

  https://lore.kernel.org/all/20240620124745.1265011-1-vladimir.zapolskiy@linaro.org/

Changes from v1 to v2:
* updated device tree documentation according to review comments received
  from Krzysztof and Sakari,
* extended runtime power management support, added functional support of
  optional XSHUTDOWN GPIO, XVCLK supply clock and 3 supply regulators.

Vladimir Zapolskiy (6):
  media: dt-bindings: Add description of OmniVision OG01A1B image sensor
  media: i2c: og01a1b: Add OF support to the image sensor driver
  media: i2c: og01a1b: Add stubs of runtime power management functions
  media: i2c: og01a1b: Add support of xvclk supply clock in power management
  media: i2c: og01a1b: Add management of optional reset GPIO
  media: i2c: og01a1b: Add management of optional sensor supply lines

 .../bindings/media/i2c/ovti,og01a1b.yaml      | 107 +++++++++++
 MAINTAINERS                                   |   1 +
 drivers/media/i2c/og01a1b.c                   | 178 ++++++++++++++++--
 3 files changed, 272 insertions(+), 14 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,og01a1b.yaml

-- 
2.45.2

[PATCH v2 1/6] media: dt-bindings: Add description of OmniVision OG01A1B image sensor

[Vladimir Zapolskiy]

Add device tree bindings documentation for OmniVision OG01A1B image
sensor.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 .../bindings/media/i2c/ovti,og01a1b.yaml      | 107 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 108 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,og01a1b.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,og01a1b.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,og01a1b.yaml
new file mode 100644
index 000000000000..ca57c01739d2
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,og01a1b.yaml
@@ -0,0 +1,107 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (c) 2023-2024 Linaro Ltd.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/ovti,og01a1b.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: OmniVision OG01A1B Image Sensor
+
+maintainers:
+  - Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
+
+description:
+  The OmniVision OG01A1B is black and white CMOS 1.3 Megapixel (1280x1024)
+  image sensor controlled over an I2C-compatible SCCB bus.
+  The sensor transmits images on a MIPI CSI-2 output interface with one or
+  two data lanes.
+
+allOf:
+  - $ref: /schemas/media/video-interface-devices.yaml#
+
+properties:
+  compatible:
+    const: ovti,og01a1b
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  reset-gpios:
+    description: Active low GPIO connected to XSHUTDOWN pad of the sensor.
+    maxItems: 1
+
+  strobe-gpios:
+    description: Input GPIO connected to strobe pad of the sensor.
+    maxItems: 1
+
+  avdd-supply:
+    description: Analogue circuit voltage supply.
+
+  dovdd-supply:
+    description: I/O circuit voltage supply.
+
+  dvdd-supply:
+    description: Digital circuit voltage supply.
+
+  port:
+    $ref: /schemas/graph.yaml#/$defs/port-base
+    additionalProperties: false
+    description:
+      Output port node, single endpoint describing the CSI-2 transmitter.
+
+    properties:
+      endpoint:
+        $ref: /schemas/media/video-interfaces.yaml#
+        unevaluatedProperties: false
+
+        properties:
+          data-lanes:
+            minItems: 1
+            maxItems: 2
+            items:
+              enum: [1, 2]
+
+          link-frequencies: true
+
+        required:
+          - data-lanes
+          - link-frequencies
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - port
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        sensor@60 {
+            compatible = "ovti,og01a1b";
+            reg = <0x60>;
+            clocks = <&clk 0>;
+            reset-gpios = <&gpio 117 GPIO_ACTIVE_LOW>;
+            avdd-supply = <&vreg_3v3>;
+            dovdd-supply = <&vreg_1p8>;
+            dvdd-supply = <&vreg_1p2>;
+
+            port {
+                og01a1b_ep: endpoint {
+                    remote-endpoint = <&csiphy_ep>;
+                    data-lanes = <1 2>;
+                    link-frequencies = /bits/ 64 <500000000>;
+                };
+            };
+        };
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index cf9c9221c388..9b0d1db35b7d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16507,6 +16507,7 @@ OMNIVISION OG01A1B SENSOR DRIVER
 M:	Sakari Ailus <sakari.ailus@linux.intel.com>
 L:	linux-media@vger.kernel.org
 S:	Maintained
+F:	Documentation/devicetree/bindings/media/i2c/ovti,og01a1b.yaml
 F:	drivers/media/i2c/og01a1b.c
 
 OMNIVISION OV01A10 SENSOR DRIVER
-- 
2.45.2

[PATCH v2 2/6] media: i2c: og01a1b: Add OF support to the image sensor driver

[Vladimir Zapolskiy]

The OmniVision  OG01A1B image sensor driver currently supports probing
only on ACPI platforms, the changes adds support of OF platforms to
the driver.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 drivers/media/i2c/og01a1b.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/media/i2c/og01a1b.c b/drivers/media/i2c/og01a1b.c
index bac9597faf68..9e756c1c47df 100644
--- a/drivers/media/i2c/og01a1b.c
+++ b/drivers/media/i2c/og01a1b.c
@@ -1057,10 +1057,17 @@ static const struct acpi_device_id og01a1b_acpi_ids[] = {
 MODULE_DEVICE_TABLE(acpi, og01a1b_acpi_ids);
 #endif
 
+static const struct of_device_id og01a1b_of_match[] = {
+	{ .compatible = "ovti,og01a1b" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, og01a1b_of_match);
+
 static struct i2c_driver og01a1b_i2c_driver = {
 	.driver = {
 		.name = "og01a1b",
 		.acpi_match_table = ACPI_PTR(og01a1b_acpi_ids),
+		.of_match_table = og01a1b_of_match,
 	},
 	.probe = og01a1b_probe,
 	.remove = og01a1b_remove,
-- 
2.45.2

[PATCH v2 3/6] media: i2c: og01a1b: Add stubs of runtime power management functions

[Vladimir Zapolskiy]

Rearrange initializations and checks in probe before population of
the power management functions.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 drivers/media/i2c/og01a1b.c | 42 +++++++++++++++++++++++++++++--------
 1 file changed, 33 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/og01a1b.c b/drivers/media/i2c/og01a1b.c
index 9e756c1c47df..d993ef4bad46 100644
--- a/drivers/media/i2c/og01a1b.c
+++ b/drivers/media/i2c/og01a1b.c
@@ -967,6 +967,19 @@ static int og01a1b_check_hwcfg(struct device *dev)
 	return ret;
 }
 
+/* Power/clock management functions */
+static int og01a1b_power_on(struct device *dev)
+{
+	/* Device is already turned on by i2c-core with ACPI domain PM. */
+
+	return 0;
+}
+
+static int og01a1b_power_off(struct device *dev)
+{
+	return 0;
+}
+
 static void og01a1b_remove(struct i2c_client *client)
 {
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
@@ -984,6 +997,12 @@ static int og01a1b_probe(struct i2c_client *client)
 	struct og01a1b *og01a1b;
 	int ret;
 
+	og01a1b = devm_kzalloc(&client->dev, sizeof(*og01a1b), GFP_KERNEL);
+	if (!og01a1b)
+		return -ENOMEM;
+
+	v4l2_i2c_subdev_init(&og01a1b->sd, client, &og01a1b_subdev_ops);
+
 	ret = og01a1b_check_hwcfg(&client->dev);
 	if (ret) {
 		dev_err(&client->dev, "failed to check HW configuration: %d",
@@ -991,15 +1010,15 @@ static int og01a1b_probe(struct i2c_client *client)
 		return ret;
 	}
 
-	og01a1b = devm_kzalloc(&client->dev, sizeof(*og01a1b), GFP_KERNEL);
-	if (!og01a1b)
-		return -ENOMEM;
+	/* The sensor must be powered on to read the CHIP_ID register */
+	ret = og01a1b_power_on(&client->dev);
+	if (ret)
+		return ret;
 
-	v4l2_i2c_subdev_init(&og01a1b->sd, client, &og01a1b_subdev_ops);
 	ret = og01a1b_identify_module(og01a1b);
 	if (ret) {
 		dev_err(&client->dev, "failed to find sensor: %d", ret);
-		return ret;
+		goto power_off;
 	}
 
 	mutex_init(&og01a1b->mutex);
@@ -1028,10 +1047,7 @@ static int og01a1b_probe(struct i2c_client *client)
 		goto probe_error_media_entity_cleanup;
 	}
 
-	/*
-	 * Device is already turned on by i2c-core with ACPI domain PM.
-	 * Enable runtime PM and turn off the device.
-	 */
+	/* Enable runtime PM and turn off the device */
 	pm_runtime_set_active(&client->dev);
 	pm_runtime_enable(&client->dev);
 	pm_runtime_idle(&client->dev);
@@ -1045,9 +1061,16 @@ static int og01a1b_probe(struct i2c_client *client)
 	v4l2_ctrl_handler_free(og01a1b->sd.ctrl_handler);
 	mutex_destroy(&og01a1b->mutex);
 
+power_off:
+	og01a1b_power_off(&client->dev);
+
 	return ret;
 }
 
+static const struct dev_pm_ops og01a1b_pm_ops = {
+	SET_RUNTIME_PM_OPS(og01a1b_power_off, og01a1b_power_on, NULL)
+};
+
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id og01a1b_acpi_ids[] = {
 	{"OVTI01AC"},
@@ -1066,6 +1089,7 @@ MODULE_DEVICE_TABLE(of, og01a1b_of_match);
 static struct i2c_driver og01a1b_i2c_driver = {
 	.driver = {
 		.name = "og01a1b",
+		.pm = &og01a1b_pm_ops,
 		.acpi_match_table = ACPI_PTR(og01a1b_acpi_ids),
 		.of_match_table = og01a1b_of_match,
 	},
-- 
2.45.2

[PATCH v2 4/6] media: i2c: og01a1b: Add support of xvclk supply clock in power management

[Vladimir Zapolskiy]

The OmniVision OG01A1B camera sensor has an xvclk supply clock, which
could be described and then explicitly controlled on OF platforms.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 drivers/media/i2c/og01a1b.c | 35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/og01a1b.c b/drivers/media/i2c/og01a1b.c
index d993ef4bad46..766740bd04c1 100644
--- a/drivers/media/i2c/og01a1b.c
+++ b/drivers/media/i2c/og01a1b.c
@@ -3,6 +3,7 @@
 
 #include <asm/unaligned.h>
 #include <linux/acpi.h>
+#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
@@ -418,6 +419,8 @@ static const struct og01a1b_mode supported_modes[] = {
 };
 
 struct og01a1b {
+	struct clk *xvclk;
+
 	struct v4l2_subdev sd;
 	struct media_pad pad;
 	struct v4l2_ctrl_handler ctrl_handler;
@@ -898,8 +901,10 @@ static int og01a1b_identify_module(struct og01a1b *og01a1b)
 	return 0;
 }
 
-static int og01a1b_check_hwcfg(struct device *dev)
+static int og01a1b_check_hwcfg(struct og01a1b *og01a1b)
 {
+	struct i2c_client *client = v4l2_get_subdevdata(&og01a1b->sd);
+	struct device *dev = &client->dev;
 	struct fwnode_handle *ep;
 	struct fwnode_handle *fwnode = dev_fwnode(dev);
 	struct v4l2_fwnode_endpoint bus_cfg = {
@@ -913,10 +918,13 @@ static int og01a1b_check_hwcfg(struct device *dev)
 		return -ENXIO;
 
 	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
-
 	if (ret) {
-		dev_err(dev, "can't get clock frequency");
-		return ret;
+		if (!og01a1b->xvclk) {
+			dev_err(dev, "can't get clock frequency");
+			return ret;
+		}
+
+		mclk = clk_get_rate(og01a1b->xvclk);
 	}
 
 	if (mclk != OG01A1B_MCLK) {
@@ -970,13 +978,19 @@ static int og01a1b_check_hwcfg(struct device *dev)
 /* Power/clock management functions */
 static int og01a1b_power_on(struct device *dev)
 {
-	/* Device is already turned on by i2c-core with ACPI domain PM. */
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct og01a1b *og01a1b = to_og01a1b(sd);
 
-	return 0;
+	return clk_prepare_enable(og01a1b->xvclk);
 }
 
 static int og01a1b_power_off(struct device *dev)
 {
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct og01a1b *og01a1b = to_og01a1b(sd);
+
+	clk_disable_unprepare(og01a1b->xvclk);
+
 	return 0;
 }
 
@@ -1003,7 +1017,14 @@ static int og01a1b_probe(struct i2c_client *client)
 
 	v4l2_i2c_subdev_init(&og01a1b->sd, client, &og01a1b_subdev_ops);
 
-	ret = og01a1b_check_hwcfg(&client->dev);
+	og01a1b->xvclk = devm_clk_get_optional(&client->dev, NULL);
+	if (IS_ERR(og01a1b->xvclk)) {
+		ret = PTR_ERR(og01a1b->xvclk);
+		dev_err(&client->dev, "failed to get xvclk clock: %d\n", ret);
+		return ret;
+	}
+
+	ret = og01a1b_check_hwcfg(og01a1b);
 	if (ret) {
 		dev_err(&client->dev, "failed to check HW configuration: %d",
 			ret);
-- 
2.45.2

[PATCH v2 5/6] media: i2c: og01a1b: Add management of optional reset GPIO

[Vladimir Zapolskiy]

Omnivision OG01A1B camera sensor may have a connected active low GPIO
to XSHUTDOWN pad, and if so, include it into sensor power up sequence.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 drivers/media/i2c/og01a1b.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/media/i2c/og01a1b.c b/drivers/media/i2c/og01a1b.c
index 766740bd04c1..90a68201f43f 100644
--- a/drivers/media/i2c/og01a1b.c
+++ b/drivers/media/i2c/og01a1b.c
@@ -5,6 +5,7 @@
 #include <linux/acpi.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
@@ -420,6 +421,7 @@ static const struct og01a1b_mode supported_modes[] = {
 
 struct og01a1b {
 	struct clk *xvclk;
+	struct gpio_desc *reset_gpio;
 
 	struct v4l2_subdev sd;
 	struct media_pad pad;
@@ -981,6 +983,9 @@ static int og01a1b_power_on(struct device *dev)
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct og01a1b *og01a1b = to_og01a1b(sd);
 
+	gpiod_set_value_cansleep(og01a1b->reset_gpio, 0);
+	usleep_range(USEC_PER_MSEC, 2 * USEC_PER_MSEC);
+
 	return clk_prepare_enable(og01a1b->xvclk);
 }
 
@@ -991,6 +996,8 @@ static int og01a1b_power_off(struct device *dev)
 
 	clk_disable_unprepare(og01a1b->xvclk);
 
+	gpiod_set_value_cansleep(og01a1b->reset_gpio, 1);
+
 	return 0;
 }
 
@@ -1031,6 +1038,13 @@ static int og01a1b_probe(struct i2c_client *client)
 		return ret;
 	}
 
+	og01a1b->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
+						      GPIOD_OUT_LOW);
+	if (IS_ERR(og01a1b->reset_gpio)) {
+		dev_err(&client->dev, "cannot get reset GPIO\n");
+		return PTR_ERR(og01a1b->reset_gpio);
+	}
+
 	/* The sensor must be powered on to read the CHIP_ID register */
 	ret = og01a1b_power_on(&client->dev);
 	if (ret)
-- 
2.45.2

[PATCH v2 6/6] media: i2c: og01a1b: Add management of optional sensor supply lines

[Vladimir Zapolskiy]

Omnivision OG01A1B camera sensor is supplied by tree power rails,
if supplies are present as device properties, include them into
sensor power up sequence.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 drivers/media/i2c/og01a1b.c | 86 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 85 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/og01a1b.c b/drivers/media/i2c/og01a1b.c
index 90a68201f43f..0150fdd2f424 100644
--- a/drivers/media/i2c/og01a1b.c
+++ b/drivers/media/i2c/og01a1b.c
@@ -9,6 +9,7 @@
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-fwnode.h>
@@ -422,6 +423,9 @@ static const struct og01a1b_mode supported_modes[] = {
 struct og01a1b {
 	struct clk *xvclk;
 	struct gpio_desc *reset_gpio;
+	struct regulator *avdd;
+	struct regulator *dovdd;
+	struct regulator *dvdd;
 
 	struct v4l2_subdev sd;
 	struct media_pad pad;
@@ -982,11 +986,46 @@ static int og01a1b_power_on(struct device *dev)
 {
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct og01a1b *og01a1b = to_og01a1b(sd);
+	int ret;
+
+	if (og01a1b->avdd) {
+		ret = regulator_enable(og01a1b->avdd);
+		if (ret)
+			return ret;
+	}
+
+	if (og01a1b->dovdd) {
+		ret = regulator_enable(og01a1b->dovdd);
+		if (ret)
+			goto avdd_disable;
+	}
+
+	if (og01a1b->dvdd) {
+		ret = regulator_enable(og01a1b->dvdd);
+		if (ret)
+			goto dovdd_disable;
+	}
 
 	gpiod_set_value_cansleep(og01a1b->reset_gpio, 0);
 	usleep_range(USEC_PER_MSEC, 2 * USEC_PER_MSEC);
 
-	return clk_prepare_enable(og01a1b->xvclk);
+	ret = clk_prepare_enable(og01a1b->xvclk);
+	if (ret)
+		goto dvdd_disable;
+
+	return 0;
+
+dvdd_disable:
+	if (og01a1b->dvdd)
+		regulator_disable(og01a1b->dvdd);
+dovdd_disable:
+	if (og01a1b->dovdd)
+		regulator_disable(og01a1b->dovdd);
+avdd_disable:
+	if (og01a1b->avdd)
+		regulator_disable(og01a1b->avdd);
+
+	return ret;
 }
 
 static int og01a1b_power_off(struct device *dev)
@@ -998,6 +1037,15 @@ static int og01a1b_power_off(struct device *dev)
 
 	gpiod_set_value_cansleep(og01a1b->reset_gpio, 1);
 
+	if (og01a1b->dvdd)
+		regulator_disable(og01a1b->dvdd);
+
+	if (og01a1b->dovdd)
+		regulator_disable(og01a1b->dovdd);
+
+	if (og01a1b->avdd)
+		regulator_disable(og01a1b->avdd);
+
 	return 0;
 }
 
@@ -1045,6 +1093,42 @@ static int og01a1b_probe(struct i2c_client *client)
 		return PTR_ERR(og01a1b->reset_gpio);
 	}
 
+	og01a1b->avdd = devm_regulator_get_optional(&client->dev, "avdd");
+	if (IS_ERR(og01a1b->avdd)) {
+		ret = PTR_ERR(og01a1b->avdd);
+		if (ret != -ENODEV) {
+			dev_err_probe(&client->dev, ret,
+				      "Failed to get 'avdd' regulator\n");
+			return ret;
+		}
+
+		og01a1b->avdd = NULL;
+	}
+
+	og01a1b->dovdd = devm_regulator_get_optional(&client->dev, "dovdd");
+	if (IS_ERR(og01a1b->dovdd)) {
+		ret = PTR_ERR(og01a1b->dovdd);
+		if (ret != -ENODEV) {
+			dev_err_probe(&client->dev, ret,
+				      "Failed to get 'dovdd' regulator\n");
+			return ret;
+		}
+
+		og01a1b->dovdd = NULL;
+	}
+
+	og01a1b->dvdd = devm_regulator_get_optional(&client->dev, "dvdd");
+	if (IS_ERR(og01a1b->dvdd)) {
+		ret = PTR_ERR(og01a1b->dvdd);
+		if (ret != -ENODEV) {
+			dev_err_probe(&client->dev, ret,
+				      "Failed to get 'dvdd' regulator\n");
+			return ret;
+		}
+
+		og01a1b->dvdd = NULL;
+	}
+
 	/* The sensor must be powered on to read the CHIP_ID register */
 	ret = og01a1b_power_on(&client->dev);
 	if (ret)
-- 
2.45.2

Re: [PATCH v2 6/6] media: i2c: og01a1b: Add management of optional sensor supply lines

[Kieran Bingham]

Quoting Vladimir Zapolskiy (2024-08-13 11:20:35)
> Omnivision OG01A1B camera sensor is supplied by tree power rails,

three?

> if supplies are present as device properties, include them into
> sensor power up sequence.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>  drivers/media/i2c/og01a1b.c | 86 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 85 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/og01a1b.c b/drivers/media/i2c/og01a1b.c
> index 90a68201f43f..0150fdd2f424 100644
> --- a/drivers/media/i2c/og01a1b.c
> +++ b/drivers/media/i2c/og01a1b.c
> @@ -9,6 +9,7 @@
>  #include <linux/i2c.h>
>  #include <linux/module.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-fwnode.h>
> @@ -422,6 +423,9 @@ static const struct og01a1b_mode supported_modes[] = {
>  struct og01a1b {
>         struct clk *xvclk;
>         struct gpio_desc *reset_gpio;
> +       struct regulator *avdd;
> +       struct regulator *dovdd;
> +       struct regulator *dvdd;
>  
>         struct v4l2_subdev sd;
>         struct media_pad pad;
> @@ -982,11 +986,46 @@ static int og01a1b_power_on(struct device *dev)
>  {
>         struct v4l2_subdev *sd = dev_get_drvdata(dev);
>         struct og01a1b *og01a1b = to_og01a1b(sd);
> +       int ret;
> +
> +       if (og01a1b->avdd) {
> +               ret = regulator_enable(og01a1b->avdd);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       if (og01a1b->dovdd) {
> +               ret = regulator_enable(og01a1b->dovdd);
> +               if (ret)
> +                       goto avdd_disable;
> +       }
> +
> +       if (og01a1b->dvdd) {
> +               ret = regulator_enable(og01a1b->dvdd);
> +               if (ret)
> +                       goto dovdd_disable;
> +       }

Perhaps regulator_bulk_enable()/regulator_bulk_disable() would be
suitable here to reduce lots of repetitive code and error handling?

>  
>         gpiod_set_value_cansleep(og01a1b->reset_gpio, 0);
>         usleep_range(USEC_PER_MSEC, 2 * USEC_PER_MSEC);
>  
> -       return clk_prepare_enable(og01a1b->xvclk);
> +       ret = clk_prepare_enable(og01a1b->xvclk);
> +       if (ret)
> +               goto dvdd_disable;
> +
> +       return 0;
> +
> +dvdd_disable:
> +       if (og01a1b->dvdd)
> +               regulator_disable(og01a1b->dvdd);
> +dovdd_disable:
> +       if (og01a1b->dovdd)
> +               regulator_disable(og01a1b->dovdd);
> +avdd_disable:
> +       if (og01a1b->avdd)
> +               regulator_disable(og01a1b->avdd);
> +
> +       return ret;
>  }
>  
>  static int og01a1b_power_off(struct device *dev)
> @@ -998,6 +1037,15 @@ static int og01a1b_power_off(struct device *dev)
>  
>         gpiod_set_value_cansleep(og01a1b->reset_gpio, 1);
>  
> +       if (og01a1b->dvdd)
> +               regulator_disable(og01a1b->dvdd);
> +
> +       if (og01a1b->dovdd)
> +               regulator_disable(og01a1b->dovdd);
> +
> +       if (og01a1b->avdd)
> +               regulator_disable(og01a1b->avdd);
> +
>         return 0;
>  }
>  
> @@ -1045,6 +1093,42 @@ static int og01a1b_probe(struct i2c_client *client)
>                 return PTR_ERR(og01a1b->reset_gpio);
>         }
>  
> +       og01a1b->avdd = devm_regulator_get_optional(&client->dev, "avdd");
> +       if (IS_ERR(og01a1b->avdd)) {
> +               ret = PTR_ERR(og01a1b->avdd);
> +               if (ret != -ENODEV) {
> +                       dev_err_probe(&client->dev, ret,
> +                                     "Failed to get 'avdd' regulator\n");
> +                       return ret;
> +               }
> +
> +               og01a1b->avdd = NULL;
> +       }
> +
> +       og01a1b->dovdd = devm_regulator_get_optional(&client->dev, "dovdd");
> +       if (IS_ERR(og01a1b->dovdd)) {
> +               ret = PTR_ERR(og01a1b->dovdd);
> +               if (ret != -ENODEV) {
> +                       dev_err_probe(&client->dev, ret,
> +                                     "Failed to get 'dovdd' regulator\n");
> +                       return ret;
> +               }
> +
> +               og01a1b->dovdd = NULL;
> +       }
> +
> +       og01a1b->dvdd = devm_regulator_get_optional(&client->dev, "dvdd");
> +       if (IS_ERR(og01a1b->dvdd)) {
> +               ret = PTR_ERR(og01a1b->dvdd);
> +               if (ret != -ENODEV) {
> +                       dev_err_probe(&client->dev, ret,
> +                                     "Failed to get 'dvdd' regulator\n");
> +                       return ret;
> +               }
> +
> +               og01a1b->dvdd = NULL;
> +       }
> +
>         /* The sensor must be powered on to read the CHIP_ID register */
>         ret = og01a1b_power_on(&client->dev);
>         if (ret)
> -- 
> 2.45.2
>

Re: [PATCH v2 6/6] media: i2c: og01a1b: Add management of optional sensor supply lines

[Vladimir Zapolskiy]

Hi Kieran,

On 8/13/24 15:53, Kieran Bingham wrote:
> Quoting Vladimir Zapolskiy (2024-08-13 11:20:35)
>> Omnivision OG01A1B camera sensor is supplied by tree power rails,
> 
> three?

that's it, thanks for catching the typo.

>> if supplies are present as device properties, include them into
>> sensor power up sequence.
>>
>> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>> ---
>>   drivers/media/i2c/og01a1b.c | 86 ++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 85 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/i2c/og01a1b.c b/drivers/media/i2c/og01a1b.c
>> index 90a68201f43f..0150fdd2f424 100644
>> --- a/drivers/media/i2c/og01a1b.c
>> +++ b/drivers/media/i2c/og01a1b.c
>> @@ -9,6 +9,7 @@
>>   #include <linux/i2c.h>
>>   #include <linux/module.h>
>>   #include <linux/pm_runtime.h>
>> +#include <linux/regulator/consumer.h>
>>   #include <media/v4l2-ctrls.h>
>>   #include <media/v4l2-device.h>
>>   #include <media/v4l2-fwnode.h>
>> @@ -422,6 +423,9 @@ static const struct og01a1b_mode supported_modes[] = {
>>   struct og01a1b {
>>          struct clk *xvclk;
>>          struct gpio_desc *reset_gpio;
>> +       struct regulator *avdd;
>> +       struct regulator *dovdd;
>> +       struct regulator *dvdd;
>>   
>>          struct v4l2_subdev sd;
>>          struct media_pad pad;
>> @@ -982,11 +986,46 @@ static int og01a1b_power_on(struct device *dev)
>>   {
>>          struct v4l2_subdev *sd = dev_get_drvdata(dev);
>>          struct og01a1b *og01a1b = to_og01a1b(sd);
>> +       int ret;
>> +
>> +       if (og01a1b->avdd) {
>> +               ret = regulator_enable(og01a1b->avdd);
>> +               if (ret)
>> +                       return ret;
>> +       }
>> +
>> +       if (og01a1b->dovdd) {
>> +               ret = regulator_enable(og01a1b->dovdd);
>> +               if (ret)
>> +                       goto avdd_disable;
>> +       }
>> +
>> +       if (og01a1b->dvdd) {
>> +               ret = regulator_enable(og01a1b->dvdd);
>> +               if (ret)
>> +                       goto dovdd_disable;
>> +       }
> 
> Perhaps regulator_bulk_enable()/regulator_bulk_disable() would be
> suitable here to reduce lots of repetitive code and error handling?

It won't be possible to support optional regulators with bulk operations,
thus likely it's not an option.

Note, that in ACPI case there are no regulators at all, at least this
should be functionally preserved in the driver.

It might be an option to define all supply regulators as strictly required
for the OF case, but since there is actually no need for it, I'm reluctant
to push the limits into the device tree node schema.

So, from my opinion the left option is the published one.

--
Best wishes,
Vladimir

Re: [PATCH v2 1/6] media: dt-bindings: Add description of OmniVision OG01A1B image sensor

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 282 bytes --]

On Tue, Aug 13, 2024 at 01:20:30PM +0300, Vladimir Zapolskiy wrote:
> Add device tree bindings documentation for OmniVision OG01A1B image
> sensor.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/6] media: dt-bindings: Add description of OmniVision OG01A1B image sensor

[Rob Herring]

On Tue, Aug 13, 2024 at 01:20:30PM +0300, Vladimir Zapolskiy wrote:
> Add device tree bindings documentation for OmniVision OG01A1B image
> sensor.

Drop 'description of ' in subject.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>  .../bindings/media/i2c/ovti,og01a1b.yaml      | 107 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 108 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,og01a1b.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,og01a1b.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,og01a1b.yaml
> new file mode 100644
> index 000000000000..ca57c01739d2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,og01a1b.yaml
> @@ -0,0 +1,107 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (c) 2023-2024 Linaro Ltd.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/ovti,og01a1b.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: OmniVision OG01A1B Image Sensor
> +
> +maintainers:
> +  - Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> +
> +description:
> +  The OmniVision OG01A1B is black and white CMOS 1.3 Megapixel (1280x1024)
> +  image sensor controlled over an I2C-compatible SCCB bus.
> +  The sensor transmits images on a MIPI CSI-2 output interface with one or
> +  two data lanes.
> +
> +allOf:
> +  - $ref: /schemas/media/video-interface-devices.yaml#
> +
> +properties:
> +  compatible:
> +    const: ovti,og01a1b
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    description: Active low GPIO connected to XSHUTDOWN pad of the sensor.
> +    maxItems: 1
> +
> +  strobe-gpios:
> +    description: Input GPIO connected to strobe pad of the sensor.
> +    maxItems: 1
> +
> +  avdd-supply:
> +    description: Analogue circuit voltage supply.
> +
> +  dovdd-supply:
> +    description: I/O circuit voltage supply.
> +
> +  dvdd-supply:
> +    description: Digital circuit voltage supply.
> +
> +  port:
> +    $ref: /schemas/graph.yaml#/$defs/port-base
> +    additionalProperties: false
> +    description:
> +      Output port node, single endpoint describing the CSI-2 transmitter.
> +
> +    properties:
> +      endpoint:
> +        $ref: /schemas/media/video-interfaces.yaml#
> +        unevaluatedProperties: false
> +
> +        properties:
> +          data-lanes:
> +            minItems: 1
> +            maxItems: 2
> +            items:
> +              enum: [1, 2]
> +
> +          link-frequencies: true
> +
> +        required:
> +          - data-lanes
> +          - link-frequencies
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - port
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        sensor@60 {
> +            compatible = "ovti,og01a1b";
> +            reg = <0x60>;
> +            clocks = <&clk 0>;
> +            reset-gpios = <&gpio 117 GPIO_ACTIVE_LOW>;
> +            avdd-supply = <&vreg_3v3>;
> +            dovdd-supply = <&vreg_1p8>;
> +            dvdd-supply = <&vreg_1p2>;
> +
> +            port {
> +                og01a1b_ep: endpoint {
> +                    remote-endpoint = <&csiphy_ep>;
> +                    data-lanes = <1 2>;
> +                    link-frequencies = /bits/ 64 <500000000>;
> +                };
> +            };
> +        };
> +    };
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index cf9c9221c388..9b0d1db35b7d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -16507,6 +16507,7 @@ OMNIVISION OG01A1B SENSOR DRIVER
>  M:	Sakari Ailus <sakari.ailus@linux.intel.com>
>  L:	linux-media@vger.kernel.org
>  S:	Maintained
> +F:	Documentation/devicetree/bindings/media/i2c/ovti,og01a1b.yaml
>  F:	drivers/media/i2c/og01a1b.c
>  
>  OMNIVISION OV01A10 SENSOR DRIVER
> -- 
> 2.45.2
>