[PATCH v2 1/3] media: dt-bindings: ov5675: document YAML binding

[PATCH v2 1/3] media: dt-bindings: ov5675: document YAML binding

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

This patch adds documentation of device tree in YAML schema for the
OV5675 CMOS image sensor from Omnivision.

Cc: Quentin Schulz <foss+kernel@0leil.net>
Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---

v2:
 - fixed incorrect id,
 - fixed device tree example by adding missing dt-bindings headers,
 - fixed device tree example by using vcc_1v2 for dvdd supply, as requested
 in datasheet,

 .../bindings/media/i2c/ovti,ov5675.yaml       | 139 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 140 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml
new file mode 100644
index 000000000000..29df2f82c631
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml
@@ -0,0 +1,139 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (c) 2022 Theobroma Systems Design und Consulting GmbH
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/ovti,ov5675.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Omnivision OV5675 CMOS Sensor Device Tree Bindings
+
+maintainers:
+  - Quentin Schulz <quentin.schulz@theobroma-systems.com>
+
+description: |-
+  The Omnivision OV5675 is a high performance, 1/5-inch, 5 megapixel, CMOS
+  image sensor that delivers 2592x1944 at 30fps. It provides full-frame,
+  sub-sampled, and windowed 10-bit MIPI images in various formats via the
+  Serial Camera Control Bus (SCCB) interface. This chip is programmable
+  through I2C and two-wire SCCB. The sensor output is available via CSI-2
+  serial data output (up to 2-lane).
+
+properties:
+  compatible:
+    const: ovti,ov5675
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    description:
+      Input clock for the sensor.
+    items:
+      - const: xvclk
+
+  clock-frequency:
+    description:
+      Frequency of the xvclk clock in Hertz.
+
+  dovdd-supply:
+    description:
+      Definition of the regulator used as interface power supply.
+
+  avdd-supply:
+    description:
+      Definition of the regulator used as analog power supply.
+
+  dvdd-supply:
+    description:
+      Definition of the regulator used as digital power supply.
+
+  reset-gpios:
+    description:
+      The phandle and specifier for the GPIO that controls sensor reset.
+      This corresponds to the hardware pin XSHUTDOWN which is physically
+      active low.
+
+  port:
+    type: object
+    additionalProperties: false
+    description:
+      A node containing an output port node with an endpoint definition
+      as documented in
+      Documentation/devicetree/bindings/media/video-interfaces.txt
+
+    properties:
+      endpoint:
+        type: object
+
+        properties:
+          data-lanes:
+            description: |-
+              The driver only supports 2-lane operation.
+            items:
+              - const: 1
+              - const: 2
+
+          link-frequencies:
+            $ref: /schemas/types.yaml#/definitions/uint64-array
+            description:
+              Allowed data bus frequencies. 450000000Hz is supported by the driver.
+
+        required:
+          - link-frequencies
+
+    required:
+      - endpoint
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - clock-frequency
+  - dovdd-supply
+  - avdd-supply
+  - dvdd-supply
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/px30-cru.h>
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/pinctrl/rockchip.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ov5675: camera@36 {
+            compatible = "ovti,ov5675";
+            reg = <0x36>;
+
+            reset-gpios = <&gpio2 RK_PB1 GPIO_ACTIVE_LOW>;
+            pinctrl-names = "default";
+            pinctrl-0 = <&cif_clkout_m0>;
+
+            clocks = <&cru SCLK_CIF_OUT>;
+            clock-names = "xvclk";
+            clock-frequency = <19200000>;
+
+            avdd-supply = <&vcc_1v8>;
+            dvdd-supply = <&vcc_1v2>;
+            dovdd-supply = <&vcc_2v8>;
+
+            port {
+                ucam_out: endpoint {
+                    remote-endpoint = <&mipi_in_ucam>;
+                    data-lanes = <1 2>;
+                    link-frequencies = /bits/ 64 <450000000>;
+                };
+            };
+        };
+    };
+...
+
diff --git a/MAINTAINERS b/MAINTAINERS
index edc96cdb85e8..94ff31268c3d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14550,6 +14550,7 @@ M:	Shawn Tu <shawnx.tu@intel.com>
 L:	linux-media@vger.kernel.org
 S:	Maintained
 T:	git git://linuxtv.org/media_tree.git
+F:	Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml
 F:	drivers/media/i2c/ov5675.c
 
 OMNIVISION OV5693 SENSOR DRIVER
-- 
2.35.1

[PATCH v2 2/3] media: ov5675: add device-tree support

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

Until now, this driver only supported ACPI. This adds support for
Device Tree too.

This is heavily inspired by device tree support addition to OV8856
driver. The differentiation between ACPI and DT mode is done through an
is_acpi_node check.

Cc: Quentin Schulz <foss+kernel@0leil.net>
Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---

v2:
 - fixed unused-const-variable warning by removing of_match_ptr in
 of_match_table, reported by kernel test robot,

 drivers/media/i2c/ov5675.c | 134 +++++++++++++++++++++++++++++++++----
 1 file changed, 121 insertions(+), 13 deletions(-)

diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
index 82ba9f56baec..ccbc8dc506ff 100644
--- a/drivers/media/i2c/ov5675.c
+++ b/drivers/media/i2c/ov5675.c
@@ -3,10 +3,13 @@
 
 #include <asm/unaligned.h>
 #include <linux/acpi.h>
+#include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-fwnode.h>
@@ -17,7 +20,7 @@
 
 #define OV5675_LINK_FREQ_450MHZ		450000000ULL
 #define OV5675_SCLK			90000000LL
-#define OV5675_MCLK			19200000
+#define OV5675_XVCLK_19_2		19200000
 #define OV5675_DATA_LANES		2
 #define OV5675_RGB_DEPTH		10
 
@@ -76,6 +79,14 @@
 
 #define to_ov5675(_sd)			container_of(_sd, struct ov5675, sd)
 
+static const char * const ov5675_supply_names[] = {
+	"avdd",		/* Analog power */
+	"dovdd",	/* Digital I/O power */
+	"dvdd",		/* Digital core power */
+};
+
+#define OV5675_NUM_SUPPLIES	ARRAY_SIZE(ov5675_supply_names)
+
 enum {
 	OV5675_LINK_FREQ_900MBPS,
 };
@@ -484,6 +495,9 @@ struct ov5675 {
 	struct v4l2_subdev sd;
 	struct media_pad pad;
 	struct v4l2_ctrl_handler ctrl_handler;
+	struct clk		*xvclk;
+	struct gpio_desc	*reset_gpio;
+	struct regulator_bulk_data supplies[OV5675_NUM_SUPPLIES];
 
 	/* V4L2 Controls */
 	struct v4l2_ctrl *link_freq;
@@ -944,6 +958,52 @@ static int ov5675_set_stream(struct v4l2_subdev *sd, int enable)
 	return ret;
 }
 
+static void __ov5675_power_off(struct ov5675 *ov5675)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&ov5675->sd);
+
+	if (is_acpi_node(dev_fwnode(&client->dev)))
+		return;
+
+	gpiod_set_value_cansleep(ov5675->reset_gpio, 1);
+	usleep_range(1000, 1200);
+
+	regulator_bulk_disable(OV5675_NUM_SUPPLIES, ov5675->supplies);
+	clk_disable_unprepare(ov5675->xvclk);
+}
+
+static int __ov5675_power_on(struct ov5675 *ov5675)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&ov5675->sd);
+	int ret;
+
+	if (is_acpi_node(dev_fwnode(&client->dev)))
+		return 0;
+
+	ret = clk_prepare_enable(ov5675->xvclk);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to enable xvclk: %d\n", ret);
+		return ret;
+	}
+
+	gpiod_set_value_cansleep(ov5675->reset_gpio, 1);
+
+	/* Reset pulse should be at least 2ms */
+	usleep_range(2000, 2200);
+
+	ret = regulator_bulk_enable(OV5675_NUM_SUPPLIES, ov5675->supplies);
+	if (ret) {
+		clk_disable_unprepare(ov5675->xvclk);
+		return ret;
+	}
+
+	gpiod_set_value_cansleep(ov5675->reset_gpio, 0);
+
+	usleep_range(1000, 1200);
+
+	return 0;
+}
+
 static int __maybe_unused ov5675_suspend(struct device *dev)
 {
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
@@ -953,6 +1013,7 @@ static int __maybe_unused ov5675_suspend(struct device *dev)
 	if (ov5675->streaming)
 		ov5675_stop_streaming(ov5675);
 
+	__ov5675_power_off(ov5675);
 	mutex_unlock(&ov5675->mutex);
 
 	return 0;
@@ -965,6 +1026,8 @@ static int __maybe_unused ov5675_resume(struct device *dev)
 	int ret;
 
 	mutex_lock(&ov5675->mutex);
+
+	__ov5675_power_on(ov5675);
 	if (ov5675->streaming) {
 		ret = ov5675_start_streaming(ov5675);
 		if (ret) {
@@ -1106,32 +1169,60 @@ static const struct v4l2_subdev_internal_ops ov5675_internal_ops = {
 	.open = ov5675_open,
 };
 
-static int ov5675_check_hwcfg(struct device *dev)
+static int ov5675_get_hwcfg(struct ov5675 *ov5675, struct device *dev)
 {
 	struct fwnode_handle *ep;
 	struct fwnode_handle *fwnode = dev_fwnode(dev);
 	struct v4l2_fwnode_endpoint bus_cfg = {
 		.bus_type = V4L2_MBUS_CSI2_DPHY
 	};
-	u32 mclk;
+	u32 xvclk_rate;
 	int ret;
 	unsigned int i, j;
 
 	if (!fwnode)
 		return -ENXIO;
 
-	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
+	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &xvclk_rate);
 
 	if (ret) {
 		dev_err(dev, "can't get clock frequency");
 		return ret;
 	}
 
-	if (mclk != OV5675_MCLK) {
-		dev_err(dev, "external clock %d is not supported", mclk);
+	if (!is_acpi_node(fwnode)) {
+		ov5675->xvclk = devm_clk_get(dev, "xvclk");
+		if (IS_ERR(ov5675->xvclk)) {
+			ret = PTR_ERR(ov5675->xvclk);
+			dev_err(dev, "failed to get xvclk: %d\n", ret);
+			return ret;
+		}
+
+		clk_set_rate(ov5675->xvclk, xvclk_rate);
+		xvclk_rate = clk_get_rate(ov5675->xvclk);
+	}
+
+	if (xvclk_rate != OV5675_XVCLK_19_2) {
+		dev_err(dev, "external clock rate %u is unsupported", xvclk_rate);
 		return -EINVAL;
 	}
 
+	ov5675->reset_gpio = devm_gpiod_get_optional(dev, "reset",
+						     GPIOD_OUT_HIGH);
+	if (IS_ERR(ov5675->reset_gpio)) {
+		ret = PTR_ERR(ov5675->reset_gpio);
+		dev_err(dev, "failed to get reset-gpios: %d\n", ret);
+		return ret;
+	}
+
+	for (i = 0; i < OV5675_NUM_SUPPLIES; i++)
+		ov5675->supplies[i].supply = ov5675_supply_names[i];
+
+	ret = devm_regulator_bulk_get(dev, OV5675_NUM_SUPPLIES,
+				      ov5675->supplies);
+	if (ret)
+		return ret;
+
 	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
 	if (!ep)
 		return -ENXIO;
@@ -1186,6 +1277,8 @@ static int ov5675_remove(struct i2c_client *client)
 	pm_runtime_disable(&client->dev);
 	mutex_destroy(&ov5675->mutex);
 
+	__ov5675_power_off(ov5675);
+
 	return 0;
 }
 
@@ -1195,25 +1288,31 @@ static int ov5675_probe(struct i2c_client *client)
 	bool full_power;
 	int ret;
 
-	ret = ov5675_check_hwcfg(&client->dev);
+	ov5675 = devm_kzalloc(&client->dev, sizeof(*ov5675), GFP_KERNEL);
+	if (!ov5675)
+		return -ENOMEM;
+
+	ret = ov5675_get_hwcfg(ov5675, &client->dev);
 	if (ret) {
-		dev_err(&client->dev, "failed to check HW configuration: %d",
+		dev_err(&client->dev, "failed to get HW configuration: %d",
 			ret);
 		return ret;
 	}
 
-	ov5675 = devm_kzalloc(&client->dev, sizeof(*ov5675), GFP_KERNEL);
-	if (!ov5675)
-		return -ENOMEM;
-
 	v4l2_i2c_subdev_init(&ov5675->sd, client, &ov5675_subdev_ops);
 
+	ret = __ov5675_power_on(ov5675);
+	if (ret) {
+		dev_err(&client->dev, "failed to power on: %d\n", ret);
+		return ret;
+	}
+
 	full_power = acpi_dev_state_d0(&client->dev);
 	if (full_power) {
 		ret = ov5675_identify_module(ov5675);
 		if (ret) {
 			dev_err(&client->dev, "failed to find sensor: %d", ret);
-			return ret;
+			goto probe_power_off;
 		}
 	}
 
@@ -1262,6 +1361,8 @@ static int ov5675_probe(struct i2c_client *client)
 probe_error_v4l2_ctrl_handler_free:
 	v4l2_ctrl_handler_free(ov5675->sd.ctrl_handler);
 	mutex_destroy(&ov5675->mutex);
+probe_power_off:
+	__ov5675_power_off(ov5675);
 
 	return ret;
 }
@@ -1279,11 +1380,18 @@ static const struct acpi_device_id ov5675_acpi_ids[] = {
 MODULE_DEVICE_TABLE(acpi, ov5675_acpi_ids);
 #endif
 
+static const struct of_device_id ov5675_of_match[] = {
+	{ .compatible = "ovti,ov5675", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, ov5675_of_match);
+
 static struct i2c_driver ov5675_i2c_driver = {
 	.driver = {
 		.name = "ov5675",
 		.pm = &ov5675_pm_ops,
 		.acpi_match_table = ACPI_PTR(ov5675_acpi_ids),
+		.of_match_table = ov5675_of_match,
 	},
 	.probe_new = ov5675_probe,
 	.remove = ov5675_remove,
-- 
2.35.1

Re: [PATCH v2 2/3] media: ov5675: add device-tree support

[Jacopo Mondi]

Hi Quentin,

On Wed, May 04, 2022 at 03:55:42PM +0200, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>
> Until now, this driver only supported ACPI. This adds support for
> Device Tree too.
>
> This is heavily inspired by device tree support addition to OV8856
> driver. The differentiation between ACPI and DT mode is done through an
> is_acpi_node check.
>
> Cc: Quentin Schulz <foss+kernel@0leil.net>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> ---
>
> v2:
>  - fixed unused-const-variable warning by removing of_match_ptr in
>  of_match_table, reported by kernel test robot,
>
>  drivers/media/i2c/ov5675.c | 134 +++++++++++++++++++++++++++++++++----
>  1 file changed, 121 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
> index 82ba9f56baec..ccbc8dc506ff 100644
> --- a/drivers/media/i2c/ov5675.c
> +++ b/drivers/media/i2c/ov5675.c
> @@ -3,10 +3,13 @@
>
>  #include <asm/unaligned.h>
>  #include <linux/acpi.h>
> +#include <linux/clk.h>
>  #include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>

#include <linux/mod_devicetable.h>

for struct of_device_id

>  #include <linux/module.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-fwnode.h>
> @@ -17,7 +20,7 @@
>
>  #define OV5675_LINK_FREQ_450MHZ		450000000ULL
>  #define OV5675_SCLK			90000000LL
> -#define OV5675_MCLK			19200000
> +#define OV5675_XVCLK_19_2		19200000
>  #define OV5675_DATA_LANES		2
>  #define OV5675_RGB_DEPTH		10
>
> @@ -76,6 +79,14 @@
>
>  #define to_ov5675(_sd)			container_of(_sd, struct ov5675, sd)
>
> +static const char * const ov5675_supply_names[] = {
> +	"avdd",		/* Analog power */
> +	"dovdd",	/* Digital I/O power */
> +	"dvdd",		/* Digital core power */
> +};
> +
> +#define OV5675_NUM_SUPPLIES	ARRAY_SIZE(ov5675_supply_names)
> +
>  enum {
>  	OV5675_LINK_FREQ_900MBPS,
>  };
> @@ -484,6 +495,9 @@ struct ov5675 {
>  	struct v4l2_subdev sd;
>  	struct media_pad pad;
>  	struct v4l2_ctrl_handler ctrl_handler;
> +	struct clk		*xvclk;
> +	struct gpio_desc	*reset_gpio;
> +	struct regulator_bulk_data supplies[OV5675_NUM_SUPPLIES];
>
>  	/* V4L2 Controls */
>  	struct v4l2_ctrl *link_freq;
> @@ -944,6 +958,52 @@ static int ov5675_set_stream(struct v4l2_subdev *sd, int enable)
>  	return ret;
>  }
>
> +static void __ov5675_power_off(struct ov5675 *ov5675)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&ov5675->sd);
> +
> +	if (is_acpi_node(dev_fwnode(&client->dev)))
> +		return;
> +
> +	gpiod_set_value_cansleep(ov5675->reset_gpio, 1);
> +	usleep_range(1000, 1200);
> +
> +	regulator_bulk_disable(OV5675_NUM_SUPPLIES, ov5675->supplies);
> +	clk_disable_unprepare(ov5675->xvclk);
> +}
> +
> +static int __ov5675_power_on(struct ov5675 *ov5675)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&ov5675->sd);
> +	int ret;
> +
> +	if (is_acpi_node(dev_fwnode(&client->dev)))

A question for Sakari here:

I have a similar series for ov5670, where I don't use is_acpi_node()
https://patchwork.linuxtv.org/project/linux-media/patch/20220329090133.338073-7-jacopo@jmondi.org/

should this be done for all drivers supporting acpi && OF ?

> +		return 0;
> +
> +	ret = clk_prepare_enable(ov5675->xvclk);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to enable xvclk: %d\n", ret);
> +		return ret;
> +	}
> +
> +	gpiod_set_value_cansleep(ov5675->reset_gpio, 1);
> +
> +	/* Reset pulse should be at least 2ms */
> +	usleep_range(2000, 2200);
> +
> +	ret = regulator_bulk_enable(OV5675_NUM_SUPPLIES, ov5675->supplies);
> +	if (ret) {
> +		clk_disable_unprepare(ov5675->xvclk);
> +		return ret;
> +	}
> +
> +	gpiod_set_value_cansleep(ov5675->reset_gpio, 0);
> +
> +	usleep_range(1000, 1200);
> +
> +	return 0;
> +}
> +
>  static int __maybe_unused ov5675_suspend(struct device *dev)
>  {
>  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> @@ -953,6 +1013,7 @@ static int __maybe_unused ov5675_suspend(struct device *dev)
>  	if (ov5675->streaming)
>  		ov5675_stop_streaming(ov5675);
>
> +	__ov5675_power_off(ov5675);

So you plumb the device power/up down in the SYSTEM_SLEEP_PM_OPS() callbacks ?

My understanding is that it would be better to create RUNTIME_PM_OPS()
for this, so that the device can be runtime suspended/resumed.

Be aware my understanding of runtime_pm is limited, better check with
Sakari too (I'll ask him to have a look).

>  	mutex_unlock(&ov5675->mutex);
>
>  	return 0;
> @@ -965,6 +1026,8 @@ static int __maybe_unused ov5675_resume(struct device *dev)
>  	int ret;
>
>  	mutex_lock(&ov5675->mutex);
> +
> +	__ov5675_power_on(ov5675);
>  	if (ov5675->streaming) {
>  		ret = ov5675_start_streaming(ov5675);
>  		if (ret) {
> @@ -1106,32 +1169,60 @@ static const struct v4l2_subdev_internal_ops ov5675_internal_ops = {
>  	.open = ov5675_open,
>  };
>
> -static int ov5675_check_hwcfg(struct device *dev)
> +static int ov5675_get_hwcfg(struct ov5675 *ov5675, struct device *dev)
>  {
>  	struct fwnode_handle *ep;
>  	struct fwnode_handle *fwnode = dev_fwnode(dev);
>  	struct v4l2_fwnode_endpoint bus_cfg = {
>  		.bus_type = V4L2_MBUS_CSI2_DPHY
>  	};
> -	u32 mclk;
> +	u32 xvclk_rate;
>  	int ret;
>  	unsigned int i, j;
>
>  	if (!fwnode)
>  		return -ENXIO;
>
> -	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> +	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &xvclk_rate);

Isn't "clock-frequency" a leftover from ACPI ? It shouldn't be in the OF
bindings either (you have it in 1/3).

You can use the common clock framework API as you do below for OF and
parse "clock-frequency" only for ACPI, as far as I can tell.

>  	if (ret) {
>  		dev_err(dev, "can't get clock frequency");
>  		return ret;
>  	}
>
> -	if (mclk != OV5675_MCLK) {
> -		dev_err(dev, "external clock %d is not supported", mclk);
> +	if (!is_acpi_node(fwnode)) {
> +		ov5675->xvclk = devm_clk_get(dev, "xvclk");
> +		if (IS_ERR(ov5675->xvclk)) {
> +			ret = PTR_ERR(ov5675->xvclk);
> +			dev_err(dev, "failed to get xvclk: %d\n", ret);
> +			return ret;
> +		}
> +
> +		clk_set_rate(ov5675->xvclk, xvclk_rate);
> +		xvclk_rate = clk_get_rate(ov5675->xvclk);

> +	}
> +
> +	if (xvclk_rate != OV5675_XVCLK_19_2) {
> +		dev_err(dev, "external clock rate %u is unsupported", xvclk_rate);
>  		return -EINVAL;
>  	}
>
> +	ov5675->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> +						     GPIOD_OUT_HIGH);
> +	if (IS_ERR(ov5675->reset_gpio)) {
> +		ret = PTR_ERR(ov5675->reset_gpio);
> +		dev_err(dev, "failed to get reset-gpios: %d\n", ret);
> +		return ret;
> +	}
> +
> +	for (i = 0; i < OV5675_NUM_SUPPLIES; i++)
> +		ov5675->supplies[i].supply = ov5675_supply_names[i];
> +
> +	ret = devm_regulator_bulk_get(dev, OV5675_NUM_SUPPLIES,
> +				      ov5675->supplies);
> +	if (ret)
> +		return ret;
> +
>  	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
>  	if (!ep)
>  		return -ENXIO;
> @@ -1186,6 +1277,8 @@ static int ov5675_remove(struct i2c_client *client)
>  	pm_runtime_disable(&client->dev);
>  	mutex_destroy(&ov5675->mutex);
>
> +	__ov5675_power_off(ov5675);
> +
>  	return 0;
>  }
>
> @@ -1195,25 +1288,31 @@ static int ov5675_probe(struct i2c_client *client)
>  	bool full_power;
>  	int ret;
>
> -	ret = ov5675_check_hwcfg(&client->dev);
> +	ov5675 = devm_kzalloc(&client->dev, sizeof(*ov5675), GFP_KERNEL);
> +	if (!ov5675)
> +		return -ENOMEM;
> +
> +	ret = ov5675_get_hwcfg(ov5675, &client->dev);
>  	if (ret) {
> -		dev_err(&client->dev, "failed to check HW configuration: %d",
> +		dev_err(&client->dev, "failed to get HW configuration: %d",
>  			ret);
>  		return ret;
>  	}
>
> -	ov5675 = devm_kzalloc(&client->dev, sizeof(*ov5675), GFP_KERNEL);
> -	if (!ov5675)
> -		return -ENOMEM;
> -
>  	v4l2_i2c_subdev_init(&ov5675->sd, client, &ov5675_subdev_ops);
>
> +	ret = __ov5675_power_on(ov5675);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to power on: %d\n", ret);
> +		return ret;
> +	}
> +
>  	full_power = acpi_dev_state_d0(&client->dev);
>  	if (full_power) {
>  		ret = ov5675_identify_module(ov5675);
>  		if (ret) {
>  			dev_err(&client->dev, "failed to find sensor: %d", ret);
> -			return ret;
> +			goto probe_power_off;
>  		}
>  	}

Maybe you can also update the comment at the end of the probe function
to remove references to ACPI. As you wish.

	/*
	 * Device is already turned on by i2c-core with ACPI domain PM.
	 * Enable runtime PM and turn off the device.
	 */

Thanks
   j

>
> @@ -1262,6 +1361,8 @@ static int ov5675_probe(struct i2c_client *client)
>  probe_error_v4l2_ctrl_handler_free:
>  	v4l2_ctrl_handler_free(ov5675->sd.ctrl_handler);
>  	mutex_destroy(&ov5675->mutex);
> +probe_power_off:
> +	__ov5675_power_off(ov5675);
>
>  	return ret;
>  }
> @@ -1279,11 +1380,18 @@ static const struct acpi_device_id ov5675_acpi_ids[] = {
>  MODULE_DEVICE_TABLE(acpi, ov5675_acpi_ids);
>  #endif
>
> +static const struct of_device_id ov5675_of_match[] = {
> +	{ .compatible = "ovti,ov5675", },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, ov5675_of_match);
> +
>  static struct i2c_driver ov5675_i2c_driver = {
>  	.driver = {
>  		.name = "ov5675",
>  		.pm = &ov5675_pm_ops,
>  		.acpi_match_table = ACPI_PTR(ov5675_acpi_ids),
> +		.of_match_table = ov5675_of_match,
>  	},
>  	.probe_new = ov5675_probe,
>  	.remove = ov5675_remove,
> --
> 2.35.1
>

Re: [PATCH v2 2/3] media: ov5675: add device-tree support

[Sakari Ailus]

Hi Jacopo, Quentin,

On Thu, May 05, 2022 at 09:47:25AM +0200, Jacopo Mondi wrote:
> Hi Quentin,
> 
> On Wed, May 04, 2022 at 03:55:42PM +0200, Quentin Schulz wrote:
> > From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> >
> > Until now, this driver only supported ACPI. This adds support for
> > Device Tree too.
> >
> > This is heavily inspired by device tree support addition to OV8856
> > driver. The differentiation between ACPI and DT mode is done through an
> > is_acpi_node check.
> >
> > Cc: Quentin Schulz <foss+kernel@0leil.net>
> > Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > ---
> >
> > v2:
> >  - fixed unused-const-variable warning by removing of_match_ptr in
> >  of_match_table, reported by kernel test robot,
> >
> >  drivers/media/i2c/ov5675.c | 134 +++++++++++++++++++++++++++++++++----
> >  1 file changed, 121 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
> > index 82ba9f56baec..ccbc8dc506ff 100644
> > --- a/drivers/media/i2c/ov5675.c
> > +++ b/drivers/media/i2c/ov5675.c
> > @@ -3,10 +3,13 @@
> >
> >  #include <asm/unaligned.h>
> >  #include <linux/acpi.h>
> > +#include <linux/clk.h>
> >  #include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> >  #include <linux/i2c.h>
> 
> #include <linux/mod_devicetable.h>
> 
> for struct of_device_id
> 
> >  #include <linux/module.h>
> >  #include <linux/pm_runtime.h>
> > +#include <linux/regulator/consumer.h>
> >  #include <media/v4l2-ctrls.h>
> >  #include <media/v4l2-device.h>
> >  #include <media/v4l2-fwnode.h>
> > @@ -17,7 +20,7 @@
> >
> >  #define OV5675_LINK_FREQ_450MHZ		450000000ULL
> >  #define OV5675_SCLK			90000000LL
> > -#define OV5675_MCLK			19200000
> > +#define OV5675_XVCLK_19_2		19200000
> >  #define OV5675_DATA_LANES		2
> >  #define OV5675_RGB_DEPTH		10
> >
> > @@ -76,6 +79,14 @@
> >
> >  #define to_ov5675(_sd)			container_of(_sd, struct ov5675, sd)
> >
> > +static const char * const ov5675_supply_names[] = {
> > +	"avdd",		/* Analog power */
> > +	"dovdd",	/* Digital I/O power */
> > +	"dvdd",		/* Digital core power */
> > +};
> > +
> > +#define OV5675_NUM_SUPPLIES	ARRAY_SIZE(ov5675_supply_names)
> > +
> >  enum {
> >  	OV5675_LINK_FREQ_900MBPS,
> >  };
> > @@ -484,6 +495,9 @@ struct ov5675 {
> >  	struct v4l2_subdev sd;
> >  	struct media_pad pad;
> >  	struct v4l2_ctrl_handler ctrl_handler;
> > +	struct clk		*xvclk;
> > +	struct gpio_desc	*reset_gpio;
> > +	struct regulator_bulk_data supplies[OV5675_NUM_SUPPLIES];
> >
> >  	/* V4L2 Controls */
> >  	struct v4l2_ctrl *link_freq;
> > @@ -944,6 +958,52 @@ static int ov5675_set_stream(struct v4l2_subdev *sd, int enable)
> >  	return ret;
> >  }
> >
> > +static void __ov5675_power_off(struct ov5675 *ov5675)
> > +{
> > +	struct i2c_client *client = v4l2_get_subdevdata(&ov5675->sd);
> > +
> > +	if (is_acpi_node(dev_fwnode(&client->dev)))
> > +		return;
> > +
> > +	gpiod_set_value_cansleep(ov5675->reset_gpio, 1);
> > +	usleep_range(1000, 1200);
> > +
> > +	regulator_bulk_disable(OV5675_NUM_SUPPLIES, ov5675->supplies);
> > +	clk_disable_unprepare(ov5675->xvclk);
> > +}
> > +
> > +static int __ov5675_power_on(struct ov5675 *ov5675)
> > +{
> > +	struct i2c_client *client = v4l2_get_subdevdata(&ov5675->sd);
> > +	int ret;
> > +
> > +	if (is_acpi_node(dev_fwnode(&client->dev)))
> 
> A question for Sakari here:
> 
> I have a similar series for ov5670, where I don't use is_acpi_node()
> https://patchwork.linuxtv.org/project/linux-media/patch/20220329090133.338073-7-jacopo@jmondi.org/
> 
> should this be done for all drivers supporting acpi && OF ?

It's better if you don't.

Regulators and GPIOs can be present in ACPI systems, too, I'm not sure
about clocks (maybe not yet?). If you check for ACPI and then bail out
here, the driver may not work on some systems.

On the other hand, you might be able to skip some of these delays in some
cases if the related resource isn't there. The datasheet probably tells
more of that.

I guess the driver or the example driver name in documentation need
some revising.

> 
> > +		return 0;
> > +
> > +	ret = clk_prepare_enable(ov5675->xvclk);
> > +	if (ret < 0) {
> > +		dev_err(&client->dev, "failed to enable xvclk: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	gpiod_set_value_cansleep(ov5675->reset_gpio, 1);
> > +
> > +	/* Reset pulse should be at least 2ms */
> > +	usleep_range(2000, 2200);
> > +
> > +	ret = regulator_bulk_enable(OV5675_NUM_SUPPLIES, ov5675->supplies);
> > +	if (ret) {
> > +		clk_disable_unprepare(ov5675->xvclk);
> > +		return ret;
> > +	}
> > +
> > +	gpiod_set_value_cansleep(ov5675->reset_gpio, 0);
> > +
> > +	usleep_range(1000, 1200);
> > +
> > +	return 0;
> > +}
> > +
> >  static int __maybe_unused ov5675_suspend(struct device *dev)
> >  {
> >  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > @@ -953,6 +1013,7 @@ static int __maybe_unused ov5675_suspend(struct device *dev)
> >  	if (ov5675->streaming)
> >  		ov5675_stop_streaming(ov5675);
> >
> > +	__ov5675_power_off(ov5675);
> 
> So you plumb the device power/up down in the SYSTEM_SLEEP_PM_OPS() callbacks ?
> 
> My understanding is that it would be better to create RUNTIME_PM_OPS()
> for this, so that the device can be runtime suspended/resumed.

Yes, please. The driver already uses runtime PM.

> 
> Be aware my understanding of runtime_pm is limited, better check with
> Sakari too (I'll ask him to have a look).
> 
> >  	mutex_unlock(&ov5675->mutex);
> >
> >  	return 0;
> > @@ -965,6 +1026,8 @@ static int __maybe_unused ov5675_resume(struct device *dev)
> >  	int ret;
> >
> >  	mutex_lock(&ov5675->mutex);
> > +
> > +	__ov5675_power_on(ov5675);
> >  	if (ov5675->streaming) {
> >  		ret = ov5675_start_streaming(ov5675);
> >  		if (ret) {
> > @@ -1106,32 +1169,60 @@ static const struct v4l2_subdev_internal_ops ov5675_internal_ops = {
> >  	.open = ov5675_open,
> >  };
> >
> > -static int ov5675_check_hwcfg(struct device *dev)
> > +static int ov5675_get_hwcfg(struct ov5675 *ov5675, struct device *dev)
> >  {
> >  	struct fwnode_handle *ep;
> >  	struct fwnode_handle *fwnode = dev_fwnode(dev);
> >  	struct v4l2_fwnode_endpoint bus_cfg = {
> >  		.bus_type = V4L2_MBUS_CSI2_DPHY
> >  	};
> > -	u32 mclk;
> > +	u32 xvclk_rate;
> >  	int ret;
> >  	unsigned int i, j;
> >
> >  	if (!fwnode)
> >  		return -ENXIO;
> >
> > -	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> > +	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &xvclk_rate);
> 
> Isn't "clock-frequency" a leftover from ACPI ? It shouldn't be in the OF
> bindings either (you have it in 1/3).
> 
> You can use the common clock framework API as you do below for OF and
> parse "clock-frequency" only for ACPI, as far as I can tell.

Older bindings had clock-frequency on DT, too, but newer ones rely on the
frequency being set using assigned-clock- stuff.

<URL:https://hverkuil.home.xs4all.nl/spec/driver-api/camera-sensor.html#handling-clocks>

But as discussed earlier, it's not possible to technically add these as
required properties albeit it's almost certainly a bug if they're not
present in dts.

See e.g. 

> 
> >  	if (ret) {
> >  		dev_err(dev, "can't get clock frequency");
> >  		return ret;
> >  	}
> >
> > -	if (mclk != OV5675_MCLK) {
> > -		dev_err(dev, "external clock %d is not supported", mclk);
> > +	if (!is_acpi_node(fwnode)) {
> > +		ov5675->xvclk = devm_clk_get(dev, "xvclk");
> > +		if (IS_ERR(ov5675->xvclk)) {
> > +			ret = PTR_ERR(ov5675->xvclk);
> > +			dev_err(dev, "failed to get xvclk: %d\n", ret);
> > +			return ret;
> > +		}
> > +
> > +		clk_set_rate(ov5675->xvclk, xvclk_rate);
> > +		xvclk_rate = clk_get_rate(ov5675->xvclk);
> 
> > +	}
> > +
> > +	if (xvclk_rate != OV5675_XVCLK_19_2) {
> > +		dev_err(dev, "external clock rate %u is unsupported", xvclk_rate);

This would be nicer wrapped.

> >  		return -EINVAL;
> >  	}
> >
> > +	ov5675->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> > +						     GPIOD_OUT_HIGH);
> > +	if (IS_ERR(ov5675->reset_gpio)) {
> > +		ret = PTR_ERR(ov5675->reset_gpio);
> > +		dev_err(dev, "failed to get reset-gpios: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	for (i = 0; i < OV5675_NUM_SUPPLIES; i++)
> > +		ov5675->supplies[i].supply = ov5675_supply_names[i];
> > +
> > +	ret = devm_regulator_bulk_get(dev, OV5675_NUM_SUPPLIES,
> > +				      ov5675->supplies);
> > +	if (ret)
> > +		return ret;
> > +
> >  	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
> >  	if (!ep)
> >  		return -ENXIO;
> > @@ -1186,6 +1277,8 @@ static int ov5675_remove(struct i2c_client *client)
> >  	pm_runtime_disable(&client->dev);
> >  	mutex_destroy(&ov5675->mutex);
> >
> > +	__ov5675_power_off(ov5675);
> > +
> >  	return 0;
> >  }
> >
> > @@ -1195,25 +1288,31 @@ static int ov5675_probe(struct i2c_client *client)
> >  	bool full_power;
> >  	int ret;
> >
> > -	ret = ov5675_check_hwcfg(&client->dev);
> > +	ov5675 = devm_kzalloc(&client->dev, sizeof(*ov5675), GFP_KERNEL);
> > +	if (!ov5675)
> > +		return -ENOMEM;
> > +
> > +	ret = ov5675_get_hwcfg(ov5675, &client->dev);
> >  	if (ret) {
> > -		dev_err(&client->dev, "failed to check HW configuration: %d",
> > +		dev_err(&client->dev, "failed to get HW configuration: %d",
> >  			ret);
> >  		return ret;
> >  	}
> >
> > -	ov5675 = devm_kzalloc(&client->dev, sizeof(*ov5675), GFP_KERNEL);
> > -	if (!ov5675)
> > -		return -ENOMEM;
> > -
> >  	v4l2_i2c_subdev_init(&ov5675->sd, client, &ov5675_subdev_ops);
> >
> > +	ret = __ov5675_power_on(ov5675);
> > +	if (ret) {
> > +		dev_err(&client->dev, "failed to power on: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> >  	full_power = acpi_dev_state_d0(&client->dev);
> >  	if (full_power) {
> >  		ret = ov5675_identify_module(ov5675);
> >  		if (ret) {
> >  			dev_err(&client->dev, "failed to find sensor: %d", ret);
> > -			return ret;
> > +			goto probe_power_off;
> >  		}
> >  	}
> 
> Maybe you can also update the comment at the end of the probe function
> to remove references to ACPI. As you wish.
> 
> 	/*
> 	 * Device is already turned on by i2c-core with ACPI domain PM.
> 	 * Enable runtime PM and turn off the device.
> 	 */

No need for such a comment --- nothing specific to this driver there.

> 
> Thanks
>    j
> 
> >
> > @@ -1262,6 +1361,8 @@ static int ov5675_probe(struct i2c_client *client)
> >  probe_error_v4l2_ctrl_handler_free:
> >  	v4l2_ctrl_handler_free(ov5675->sd.ctrl_handler);
> >  	mutex_destroy(&ov5675->mutex);
> > +probe_power_off:
> > +	__ov5675_power_off(ov5675);
> >
> >  	return ret;
> >  }
> > @@ -1279,11 +1380,18 @@ static const struct acpi_device_id ov5675_acpi_ids[] = {
> >  MODULE_DEVICE_TABLE(acpi, ov5675_acpi_ids);
> >  #endif
> >
> > +static const struct of_device_id ov5675_of_match[] = {
> > +	{ .compatible = "ovti,ov5675", },
> > +	{ /* sentinel */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, ov5675_of_match);
> > +
> >  static struct i2c_driver ov5675_i2c_driver = {
> >  	.driver = {
> >  		.name = "ov5675",
> >  		.pm = &ov5675_pm_ops,
> >  		.acpi_match_table = ACPI_PTR(ov5675_acpi_ids),
> > +		.of_match_table = ov5675_of_match,
> >  	},
> >  	.probe_new = ov5675_probe,
> >  	.remove = ov5675_remove,

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 2/3] media: ov5675: add device-tree support

[Quentin Schulz]

Sakari, Jacopo,

On 5/5/22 10:28, Sakari Ailus wrote:
> Hi Jacopo, Quentin,
> 
> On Thu, May 05, 2022 at 09:47:25AM +0200, Jacopo Mondi wrote:
>> Hi Quentin,
>>
>> On Wed, May 04, 2022 at 03:55:42PM +0200, Quentin Schulz wrote:
>>> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>>
>>> Until now, this driver only supported ACPI. This adds support for
>>> Device Tree too.
>>>
>>> This is heavily inspired by device tree support addition to OV8856
>>> driver. The differentiation between ACPI and DT mode is done through an
>>> is_acpi_node check.
>>>
>>> Cc: Quentin Schulz <foss+kernel@0leil.net>
>>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>> ---
>>>
>>> v2:
>>>   - fixed unused-const-variable warning by removing of_match_ptr in
>>>   of_match_table, reported by kernel test robot,
>>>
>>>   drivers/media/i2c/ov5675.c | 134 +++++++++++++++++++++++++++++++++----
>>>   1 file changed, 121 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
>>> index 82ba9f56baec..ccbc8dc506ff 100644
>>> --- a/drivers/media/i2c/ov5675.c
>>> +++ b/drivers/media/i2c/ov5675.c
>>> @@ -3,10 +3,13 @@
>>>
>>>   #include <asm/unaligned.h>
>>>   #include <linux/acpi.h>
>>> +#include <linux/clk.h>
>>>   #include <linux/delay.h>
>>> +#include <linux/gpio/consumer.h>
>>>   #include <linux/i2c.h>
>>
>> #include <linux/mod_devicetable.h>
>>
>> for struct of_device_id
>>

Mmm.. Wondering why this is needed if it compiles fine without? What am 
I missing?

>>>   #include <linux/module.h>
>>>   #include <linux/pm_runtime.h>
>>> +#include <linux/regulator/consumer.h>
>>>   #include <media/v4l2-ctrls.h>
>>>   #include <media/v4l2-device.h>
>>>   #include <media/v4l2-fwnode.h>
>>> @@ -17,7 +20,7 @@
>>>
>>>   #define OV5675_LINK_FREQ_450MHZ		450000000ULL
>>>   #define OV5675_SCLK			90000000LL
>>> -#define OV5675_MCLK			19200000
>>> +#define OV5675_XVCLK_19_2		19200000
>>>   #define OV5675_DATA_LANES		2
>>>   #define OV5675_RGB_DEPTH		10
>>>
>>> @@ -76,6 +79,14 @@
>>>
>>>   #define to_ov5675(_sd)			container_of(_sd, struct ov5675, sd)
>>>
>>> +static const char * const ov5675_supply_names[] = {
>>> +	"avdd",		/* Analog power */
>>> +	"dovdd",	/* Digital I/O power */
>>> +	"dvdd",		/* Digital core power */
>>> +};
>>> +
>>> +#define OV5675_NUM_SUPPLIES	ARRAY_SIZE(ov5675_supply_names)
>>> +
>>>   enum {
>>>   	OV5675_LINK_FREQ_900MBPS,
>>>   };
>>> @@ -484,6 +495,9 @@ struct ov5675 {
>>>   	struct v4l2_subdev sd;
>>>   	struct media_pad pad;
>>>   	struct v4l2_ctrl_handler ctrl_handler;
>>> +	struct clk		*xvclk;
>>> +	struct gpio_desc	*reset_gpio;
>>> +	struct regulator_bulk_data supplies[OV5675_NUM_SUPPLIES];
>>>
>>>   	/* V4L2 Controls */
>>>   	struct v4l2_ctrl *link_freq;
>>> @@ -944,6 +958,52 @@ static int ov5675_set_stream(struct v4l2_subdev *sd, int enable)
>>>   	return ret;
>>>   }
>>>
>>> +static void __ov5675_power_off(struct ov5675 *ov5675)
>>> +{
>>> +	struct i2c_client *client = v4l2_get_subdevdata(&ov5675->sd);
>>> +
>>> +	if (is_acpi_node(dev_fwnode(&client->dev)))
>>> +		return;
>>> +
>>> +	gpiod_set_value_cansleep(ov5675->reset_gpio, 1);
>>> +	usleep_range(1000, 1200);
>>> +
>>> +	regulator_bulk_disable(OV5675_NUM_SUPPLIES, ov5675->supplies);
>>> +	clk_disable_unprepare(ov5675->xvclk);
>>> +}
>>> +
>>> +static int __ov5675_power_on(struct ov5675 *ov5675)
>>> +{
>>> +	struct i2c_client *client = v4l2_get_subdevdata(&ov5675->sd);
>>> +	int ret;
>>> +
>>> +	if (is_acpi_node(dev_fwnode(&client->dev)))
>>
>> A question for Sakari here:
>>
>> I have a similar series for ov5670, where I don't use is_acpi_node()
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.linuxtv.org_project_linux-2Dmedia_patch_20220329090133.338073-2D7-2Djacopo-40jmondi.org_&d=DwIBAg&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=d4bhfjobjD_6CMiuzFRtZy27p0ytvQuScDxLXtsaHnzywAfKYeap61kXxLuejvfT&s=pnMz66bcaCWlXZZEIqknW9Atqkxp28rKJFT4lllZhuQ&e=
>>
>> should this be done for all drivers supporting acpi && OF ?
> 
> It's better if you don't.
> 
> Regulators and GPIOs can be present in ACPI systems, too, I'm not sure
> about clocks (maybe not yet?). If you check for ACPI and then bail out
> here, the driver may not work on some systems.
> 

Yet, the driver does not use regulators and GPIOs and is only probed via 
ACPI.

The fact is that today, the ACPI version does not need any of this but 
the Device Tree version does. If the ACPI version would need to support 
regulators and GPIOs too, I'd say that's out of topic for this patch 
series. Especially since I do not have anything to test ACPI version.

I expect the suggested implementation to not break (nor improve!) 
anything on ACPI.

> On the other hand, you might be able to skip some of these delays in some
> cases if the related resource isn't there. The datasheet probably tells
> more of that.
> 

I don't trust the datasheet.. I discovered that I need to wait for 2ms 
AFTER the regulators have been turned on to release the reset gpio and 
not just 2ms of holding reset gpio. Even though the datasheet states 
that there's literally no minimum delay between the rising of the power 
rails and the release of reset gpio. So the way I did in this patchset 
is not always working (actually less often than it does... will fix that 
in v2).

> I guess the driver or the example driver name in documentation need
> some revising.
> 
>>
>>> +		return 0;
>>> +
>>> +	ret = clk_prepare_enable(ov5675->xvclk);
>>> +	if (ret < 0) {
>>> +		dev_err(&client->dev, "failed to enable xvclk: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	gpiod_set_value_cansleep(ov5675->reset_gpio, 1);
>>> +
>>> +	/* Reset pulse should be at least 2ms */
>>> +	usleep_range(2000, 2200);
>>> +
>>> +	ret = regulator_bulk_enable(OV5675_NUM_SUPPLIES, ov5675->supplies);
>>> +	if (ret) {
>>> +		clk_disable_unprepare(ov5675->xvclk);
>>> +		return ret;
>>> +	}
>>> +
>>> +	gpiod_set_value_cansleep(ov5675->reset_gpio, 0);
>>> +
>>> +	usleep_range(1000, 1200);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>   static int __maybe_unused ov5675_suspend(struct device *dev)
>>>   {
>>>   	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>>> @@ -953,6 +1013,7 @@ static int __maybe_unused ov5675_suspend(struct device *dev)
>>>   	if (ov5675->streaming)
>>>   		ov5675_stop_streaming(ov5675);
>>>
>>> +	__ov5675_power_off(ov5675);
>>
>> So you plumb the device power/up down in the SYSTEM_SLEEP_PM_OPS() callbacks ?
>>
>> My understanding is that it would be better to create RUNTIME_PM_OPS()
>> for this, so that the device can be runtime suspended/resumed.

Can't test runtime PM for ACPI version and didn't want to risk breaking 
this support.

The point of this was to be of minimal impact on existing users. Also 
see 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0c2c7a1e0d69221b9d489bfd8cf53262d6f82446

Not saying it is good, just pointing at the inspiration for this patch.

> 
> Yes, please. The driver already uses runtime PM.
> 

Does it? I only see SYSTEM_SLEEP_PM_OPS in the dev_pm_ops data 
structure. My reading of it is that it's not supported unless I add 
RUNTIME_PM_OPS? I'm probably missing some PM subsystem internals 
understanding though :/

c.f. 
https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/ov5675.c#L1244

I have a doubt now we're still talking about this patch and not 
Jacopo's? At least I'm not sure to fully get what's being discussed here.

>>
>> Be aware my understanding of runtime_pm is limited, better check with
>> Sakari too (I'll ask him to have a look).
>>
>>>   	mutex_unlock(&ov5675->mutex);
>>>
>>>   	return 0;
>>> @@ -965,6 +1026,8 @@ static int __maybe_unused ov5675_resume(struct device *dev)
>>>   	int ret;
>>>
>>>   	mutex_lock(&ov5675->mutex);
>>> +
>>> +	__ov5675_power_on(ov5675);
>>>   	if (ov5675->streaming) {
>>>   		ret = ov5675_start_streaming(ov5675);
>>>   		if (ret) {
>>> @@ -1106,32 +1169,60 @@ static const struct v4l2_subdev_internal_ops ov5675_internal_ops = {
>>>   	.open = ov5675_open,
>>>   };
>>>
>>> -static int ov5675_check_hwcfg(struct device *dev)
>>> +static int ov5675_get_hwcfg(struct ov5675 *ov5675, struct device *dev)
>>>   {
>>>   	struct fwnode_handle *ep;
>>>   	struct fwnode_handle *fwnode = dev_fwnode(dev);
>>>   	struct v4l2_fwnode_endpoint bus_cfg = {
>>>   		.bus_type = V4L2_MBUS_CSI2_DPHY
>>>   	};
>>> -	u32 mclk;
>>> +	u32 xvclk_rate;
>>>   	int ret;
>>>   	unsigned int i, j;
>>>
>>>   	if (!fwnode)
>>>   		return -ENXIO;
>>>
>>> -	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
>>> +	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &xvclk_rate);
>>
>> Isn't "clock-frequency" a leftover from ACPI ? It shouldn't be in the OF
>> bindings either (you have it in 1/3).
>>
>> You can use the common clock framework API as you do below for OF and
>> parse "clock-frequency" only for ACPI, as far as I can tell.
> 
> Older bindings had clock-frequency on DT, too, but newer ones rely on the
> frequency being set using assigned-clock- stuff.
> 
> <URL:https://urldefense.proofpoint.com/v2/url?u=https-3A__hverkuil.home.xs4all.nl_spec_driver-2Dapi_camera-2Dsensor.html-23handling-2Dclocks&d=DwIBAg&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=d4bhfjobjD_6CMiuzFRtZy27p0ytvQuScDxLXtsaHnzywAfKYeap61kXxLuejvfT&s=TYhByXuRxctr7fGq94xBetcqW2gfnfZpGK3KyCZnCNA&e= >
> 
> But as discussed earlier, it's not possible to technically add these as
> required properties albeit it's almost certainly a bug if they're not
> present in dts.
> 

I assume this can be "enforced" via dt-bindings checks? So, what's the 
decision here? Do I go for the clock framework version and lose the 
enforcing or do I keep using this one clock-frequency property for both 
ACPI and DT?

> See e.g.
> 

Missing link maybe?

>>
>>>   	if (ret) {
>>>   		dev_err(dev, "can't get clock frequency");
>>>   		return ret;
>>>   	}
>>>
>>> -	if (mclk != OV5675_MCLK) {
>>> -		dev_err(dev, "external clock %d is not supported", mclk);
>>> +	if (!is_acpi_node(fwnode)) {
>>> +		ov5675->xvclk = devm_clk_get(dev, "xvclk");
>>> +		if (IS_ERR(ov5675->xvclk)) {
>>> +			ret = PTR_ERR(ov5675->xvclk);
>>> +			dev_err(dev, "failed to get xvclk: %d\n", ret);
>>> +			return ret;
>>> +		}
>>> +
>>> +		clk_set_rate(ov5675->xvclk, xvclk_rate);
>>> +		xvclk_rate = clk_get_rate(ov5675->xvclk);
>>
>>> +	}
>>> +
>>> +	if (xvclk_rate != OV5675_XVCLK_19_2) {
>>> +		dev_err(dev, "external clock rate %u is unsupported", xvclk_rate);
> 
> This would be nicer wrapped.
> 

I do not get your suggestion, can you rephrase please?

I want the rate to be checked both for ACPI and DT.

>>>   		return -EINVAL;
>>>   	}
>>>
>>> +	ov5675->reset_gpio = devm_gpiod_get_optional(dev, "reset",
>>> +						     GPIOD_OUT_HIGH);
>>> +	if (IS_ERR(ov5675->reset_gpio)) {
>>> +		ret = PTR_ERR(ov5675->reset_gpio);
>>> +		dev_err(dev, "failed to get reset-gpios: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	for (i = 0; i < OV5675_NUM_SUPPLIES; i++)
>>> +		ov5675->supplies[i].supply = ov5675_supply_names[i];
>>> +
>>> +	ret = devm_regulator_bulk_get(dev, OV5675_NUM_SUPPLIES,
>>> +				      ov5675->supplies);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>>   	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
>>>   	if (!ep)
>>>   		return -ENXIO;
>>> @@ -1186,6 +1277,8 @@ static int ov5675_remove(struct i2c_client *client)
>>>   	pm_runtime_disable(&client->dev);
>>>   	mutex_destroy(&ov5675->mutex);
>>>
>>> +	__ov5675_power_off(ov5675);
>>> +
>>>   	return 0;
>>>   }
>>>
>>> @@ -1195,25 +1288,31 @@ static int ov5675_probe(struct i2c_client *client)
>>>   	bool full_power;
>>>   	int ret;
>>>
>>> -	ret = ov5675_check_hwcfg(&client->dev);
>>> +	ov5675 = devm_kzalloc(&client->dev, sizeof(*ov5675), GFP_KERNEL);
>>> +	if (!ov5675)
>>> +		return -ENOMEM;
>>> +
>>> +	ret = ov5675_get_hwcfg(ov5675, &client->dev);
>>>   	if (ret) {
>>> -		dev_err(&client->dev, "failed to check HW configuration: %d",
>>> +		dev_err(&client->dev, "failed to get HW configuration: %d",
>>>   			ret);
>>>   		return ret;
>>>   	}
>>>
>>> -	ov5675 = devm_kzalloc(&client->dev, sizeof(*ov5675), GFP_KERNEL);
>>> -	if (!ov5675)
>>> -		return -ENOMEM;
>>> -
>>>   	v4l2_i2c_subdev_init(&ov5675->sd, client, &ov5675_subdev_ops);
>>>
>>> +	ret = __ov5675_power_on(ov5675);
>>> +	if (ret) {
>>> +		dev_err(&client->dev, "failed to power on: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>>   	full_power = acpi_dev_state_d0(&client->dev);
>>>   	if (full_power) {
>>>   		ret = ov5675_identify_module(ov5675);
>>>   		if (ret) {
>>>   			dev_err(&client->dev, "failed to find sensor: %d", ret);
>>> -			return ret;
>>> +			goto probe_power_off;
>>>   		}
>>>   	}
>>
>> Maybe you can also update the comment at the end of the probe function
>> to remove references to ACPI. As you wish.
>>
>> 	/*
>> 	 * Device is already turned on by i2c-core with ACPI domain PM.
>> 	 * Enable runtime PM and turn off the device.
>> 	 */
> 
> No need for such a comment --- nothing specific to this driver there.
> 

The comment is already there in the driver.

The issue being that if it's probed from ACPI, there's nothing to be 
done power wise (currently at least, in the current state of the 
driver). If it's probed via DT, I need to power the device manually.

Cheers,
Quentin

Re: [PATCH v2 2/3] media: ov5675: add device-tree support

[Jacopo Mondi]

Hi Quentinm

On Fri, May 06, 2022 at 04:13:07PM +0200, Quentin Schulz wrote:
> Sakari, Jacopo,
>
> On 5/5/22 10:28, Sakari Ailus wrote:
> > Hi Jacopo, Quentin,
> >
> > On Thu, May 05, 2022 at 09:47:25AM +0200, Jacopo Mondi wrote:
> > > Hi Quentin,
> > >
> > > On Wed, May 04, 2022 at 03:55:42PM +0200, Quentin Schulz wrote:
> > > > From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > > >
> > > > Until now, this driver only supported ACPI. This adds support for
> > > > Device Tree too.
> > > >
> > > > This is heavily inspired by device tree support addition to OV8856
> > > > driver. The differentiation between ACPI and DT mode is done through an
> > > > is_acpi_node check.
> > > >
> > > > Cc: Quentin Schulz <foss+kernel@0leil.net>
> > > > Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > > > ---
> > > >
> > > > v2:
> > > >   - fixed unused-const-variable warning by removing of_match_ptr in
> > > >   of_match_table, reported by kernel test robot,
> > > >
> > > >   drivers/media/i2c/ov5675.c | 134 +++++++++++++++++++++++++++++++++----
> > > >   1 file changed, 121 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
> > > > index 82ba9f56baec..ccbc8dc506ff 100644
> > > > --- a/drivers/media/i2c/ov5675.c
> > > > +++ b/drivers/media/i2c/ov5675.c
> > > > @@ -3,10 +3,13 @@
> > > >
> > > >   #include <asm/unaligned.h>
> > > >   #include <linux/acpi.h>
> > > > +#include <linux/clk.h>
> > > >   #include <linux/delay.h>
> > > > +#include <linux/gpio/consumer.h>
> > > >   #include <linux/i2c.h>
> > >
> > > #include <linux/mod_devicetable.h>
> > >
> > > for struct of_device_id
> > >
>
> Mmm.. Wondering why this is needed if it compiles fine without? What am I
> missing?
>

Probably included by another header, with the risk that if the
inclusion chain gets changed in future the symbol might be undefined.

It's easy to verify for yourself where struct of_device_id is defined

> > > >   #include <linux/module.h>
> > > >   #include <linux/pm_runtime.h>
> > > > +#include <linux/regulator/consumer.h>
> > > >   #include <media/v4l2-ctrls.h>
> > > >   #include <media/v4l2-device.h>
> > > >   #include <media/v4l2-fwnode.h>
> > > > @@ -17,7 +20,7 @@
> > > >
> > > >   #define OV5675_LINK_FREQ_450MHZ		450000000ULL
> > > >   #define OV5675_SCLK			90000000LL
> > > > -#define OV5675_MCLK			19200000
> > > > +#define OV5675_XVCLK_19_2		19200000
> > > >   #define OV5675_DATA_LANES		2
> > > >   #define OV5675_RGB_DEPTH		10
> > > >
> > > > @@ -76,6 +79,14 @@
> > > >
> > > >   #define to_ov5675(_sd)			container_of(_sd, struct ov5675, sd)
> > > >
> > > > +static const char * const ov5675_supply_names[] = {
> > > > +	"avdd",		/* Analog power */
> > > > +	"dovdd",	/* Digital I/O power */
> > > > +	"dvdd",		/* Digital core power */
> > > > +};
> > > > +
> > > > +#define OV5675_NUM_SUPPLIES	ARRAY_SIZE(ov5675_supply_names)
> > > > +
> > > >   enum {
> > > >   	OV5675_LINK_FREQ_900MBPS,
> > > >   };
> > > > @@ -484,6 +495,9 @@ struct ov5675 {
> > > >   	struct v4l2_subdev sd;
> > > >   	struct media_pad pad;
> > > >   	struct v4l2_ctrl_handler ctrl_handler;
> > > > +	struct clk		*xvclk;
> > > > +	struct gpio_desc	*reset_gpio;
> > > > +	struct regulator_bulk_data supplies[OV5675_NUM_SUPPLIES];
> > > >
> > > >   	/* V4L2 Controls */
> > > >   	struct v4l2_ctrl *link_freq;
> > > > @@ -944,6 +958,52 @@ static int ov5675_set_stream(struct v4l2_subdev *sd, int enable)
> > > >   	return ret;
> > > >   }
> > > >
> > > > +static void __ov5675_power_off(struct ov5675 *ov5675)
> > > > +{
> > > > +	struct i2c_client *client = v4l2_get_subdevdata(&ov5675->sd);
> > > > +
> > > > +	if (is_acpi_node(dev_fwnode(&client->dev)))
> > > > +		return;
> > > > +
> > > > +	gpiod_set_value_cansleep(ov5675->reset_gpio, 1);
> > > > +	usleep_range(1000, 1200);
> > > > +
> > > > +	regulator_bulk_disable(OV5675_NUM_SUPPLIES, ov5675->supplies);
> > > > +	clk_disable_unprepare(ov5675->xvclk);
> > > > +}
> > > > +
> > > > +static int __ov5675_power_on(struct ov5675 *ov5675)
> > > > +{
> > > > +	struct i2c_client *client = v4l2_get_subdevdata(&ov5675->sd);
> > > > +	int ret;
> > > > +
> > > > +	if (is_acpi_node(dev_fwnode(&client->dev)))
> > >
> > > A question for Sakari here:
> > >
> > > I have a similar series for ov5670, where I don't use is_acpi_node()
> > > https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.linuxtv.org_project_linux-2Dmedia_patch_20220329090133.338073-2D7-2Djacopo-40jmondi.org_&d=DwIBAg&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=d4bhfjobjD_6CMiuzFRtZy27p0ytvQuScDxLXtsaHnzywAfKYeap61kXxLuejvfT&s=pnMz66bcaCWlXZZEIqknW9Atqkxp28rKJFT4lllZhuQ&e=
> > >
> > > should this be done for all drivers supporting acpi && OF ?
> >
> > It's better if you don't.
> >
> > Regulators and GPIOs can be present in ACPI systems, too, I'm not sure
> > about clocks (maybe not yet?). If you check for ACPI and then bail out
> > here, the driver may not work on some systems.
> >
>
> Yet, the driver does not use regulators and GPIOs and is only probed via
> ACPI.
>
> The fact is that today, the ACPI version does not need any of this but the
> Device Tree version does. If the ACPI version would need to support
> regulators and GPIOs too, I'd say that's out of topic for this patch series.
> Especially since I do not have anything to test ACPI version.
>
> I expect the suggested implementation to not break (nor improve!) anything
> on ACPI.
>
> > On the other hand, you might be able to skip some of these delays in some
> > cases if the related resource isn't there. The datasheet probably tells
> > more of that.
> >
>
> I don't trust the datasheet.. I discovered that I need to wait for 2ms AFTER
> the regulators have been turned on to release the reset gpio and not just
> 2ms of holding reset gpio. Even though the datasheet states that there's
> literally no minimum delay between the rising of the power rails and the
> release of reset gpio. So the way I did in this patchset is not always
> working (actually less often than it does... will fix that in v2).
>
> > I guess the driver or the example driver name in documentation need
> > some revising.
> >
> > >
> > > > +		return 0;
> > > > +
> > > > +	ret = clk_prepare_enable(ov5675->xvclk);
> > > > +	if (ret < 0) {
> > > > +		dev_err(&client->dev, "failed to enable xvclk: %d\n", ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	gpiod_set_value_cansleep(ov5675->reset_gpio, 1);
> > > > +
> > > > +	/* Reset pulse should be at least 2ms */
> > > > +	usleep_range(2000, 2200);
> > > > +
> > > > +	ret = regulator_bulk_enable(OV5675_NUM_SUPPLIES, ov5675->supplies);
> > > > +	if (ret) {
> > > > +		clk_disable_unprepare(ov5675->xvclk);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	gpiod_set_value_cansleep(ov5675->reset_gpio, 0);
> > > > +
> > > > +	usleep_range(1000, 1200);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >   static int __maybe_unused ov5675_suspend(struct device *dev)
> > > >   {
> > > >   	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > > > @@ -953,6 +1013,7 @@ static int __maybe_unused ov5675_suspend(struct device *dev)
> > > >   	if (ov5675->streaming)
> > > >   		ov5675_stop_streaming(ov5675);
> > > >
> > > > +	__ov5675_power_off(ov5675);
> > >
> > > So you plumb the device power/up down in the SYSTEM_SLEEP_PM_OPS() callbacks ?
> > >
> > > My understanding is that it would be better to create RUNTIME_PM_OPS()
> > > for this, so that the device can be runtime suspended/resumed.
>
> Can't test runtime PM for ACPI version and didn't want to risk breaking this
> support.

Why are you mentioning ACPI ?

My point here is that the driver supports only power management at
system suspend/resume time and does not implement runtime power
management and would be trivial for you to do so.

>
> The point of this was to be of minimal impact on existing users. Also see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0c2c7a1e0d69221b9d489bfd8cf53262d6f82446
>
> Not saying it is good, just pointing at the inspiration for this patch.
>
> >
> > Yes, please. The driver already uses runtime PM.
> >
>
> Does it? I only see SYSTEM_SLEEP_PM_OPS in the dev_pm_ops data structure. My
> reading of it is that it's not supported unless I add RUNTIME_PM_OPS? I'm
> probably missing some PM subsystem internals understanding though :/
>
> c.f. https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/ov5675.c#L1244
>
> I have a doubt now we're still talking about this patch and not Jacopo's? At
> least I'm not sure to fully get what's being discussed here.
>

Maybe Sakari just meant the driver is already instrumented to use the RPM
framework instead of relying on s_power() as some older driver doe,
and you just need to define the corresponding runtime_resume/suspend
operation ?

> > >
> > > Be aware my understanding of runtime_pm is limited, better check with
> > > Sakari too (I'll ask him to have a look).
> > >
> > > >   	mutex_unlock(&ov5675->mutex);
> > > >
> > > >   	return 0;
> > > > @@ -965,6 +1026,8 @@ static int __maybe_unused ov5675_resume(struct device *dev)
> > > >   	int ret;
> > > >
> > > >   	mutex_lock(&ov5675->mutex);
> > > > +
> > > > +	__ov5675_power_on(ov5675);
> > > >   	if (ov5675->streaming) {
> > > >   		ret = ov5675_start_streaming(ov5675);
> > > >   		if (ret) {
> > > > @@ -1106,32 +1169,60 @@ static const struct v4l2_subdev_internal_ops ov5675_internal_ops = {
> > > >   	.open = ov5675_open,
> > > >   };
> > > >
> > > > -static int ov5675_check_hwcfg(struct device *dev)
> > > > +static int ov5675_get_hwcfg(struct ov5675 *ov5675, struct device *dev)
> > > >   {
> > > >   	struct fwnode_handle *ep;
> > > >   	struct fwnode_handle *fwnode = dev_fwnode(dev);
> > > >   	struct v4l2_fwnode_endpoint bus_cfg = {
> > > >   		.bus_type = V4L2_MBUS_CSI2_DPHY
> > > >   	};
> > > > -	u32 mclk;
> > > > +	u32 xvclk_rate;
> > > >   	int ret;
> > > >   	unsigned int i, j;
> > > >
> > > >   	if (!fwnode)
> > > >   		return -ENXIO;
> > > >
> > > > -	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> > > > +	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &xvclk_rate);
> > >
> > > Isn't "clock-frequency" a leftover from ACPI ? It shouldn't be in the OF
> > > bindings either (you have it in 1/3).
> > >
> > > You can use the common clock framework API as you do below for OF and
> > > parse "clock-frequency" only for ACPI, as far as I can tell.
> >
> > Older bindings had clock-frequency on DT, too, but newer ones rely on the
> > frequency being set using assigned-clock- stuff.
> >
> > <URL:https://urldefense.proofpoint.com/v2/url?u=https-3A__hverkuil.home.xs4all.nl_spec_driver-2Dapi_camera-2Dsensor.html-23handling-2Dclocks&d=DwIBAg&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=d4bhfjobjD_6CMiuzFRtZy27p0ytvQuScDxLXtsaHnzywAfKYeap61kXxLuejvfT&s=TYhByXuRxctr7fGq94xBetcqW2gfnfZpGK3KyCZnCNA&e= >
> >
> > But as discussed earlier, it's not possible to technically add these as
> > required properties albeit it's almost certainly a bug if they're not
> > present in dts.
> >
>
> I assume this can be "enforced" via dt-bindings checks? So, what's the
> decision here? Do I go for the clock framework version and lose the
> enforcing or do I keep using this one clock-frequency property for both ACPI
> and DT?
>

My understanding is that "clock-frequency" is
an ACPI leftover that has been brought into some DT bindings as an
historical mistake. OF can work well with the common clock framework,
there's no need to introduce a property for the same purpose.

See
https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/imx258.c#L1262

When it comes to "enforcing" maybe Sakari refers to
https://patchwork.linuxtv.org/project/linux-media/patch/20220314162714.153970-2-jacopo@jmondi.org/#136002

where enforcing of assigned-clock-.. properties presence was
discussed.


> > See e.g.
> >
>
> Missing link maybe?
>
> > >
> > > >   	if (ret) {
> > > >   		dev_err(dev, "can't get clock frequency");
> > > >   		return ret;
> > > >   	}
> > > >
> > > > -	if (mclk != OV5675_MCLK) {
> > > > -		dev_err(dev, "external clock %d is not supported", mclk);
> > > > +	if (!is_acpi_node(fwnode)) {
> > > > +		ov5675->xvclk = devm_clk_get(dev, "xvclk");
> > > > +		if (IS_ERR(ov5675->xvclk)) {
> > > > +			ret = PTR_ERR(ov5675->xvclk);
> > > > +			dev_err(dev, "failed to get xvclk: %d\n", ret);
> > > > +			return ret;
> > > > +		}
> > > > +
> > > > +		clk_set_rate(ov5675->xvclk, xvclk_rate);
> > > > +		xvclk_rate = clk_get_rate(ov5675->xvclk);
> > >
> > > > +	}
> > > > +
> > > > +	if (xvclk_rate != OV5675_XVCLK_19_2) {
> > > > +		dev_err(dev, "external clock rate %u is unsupported", xvclk_rate);
> >
> > This would be nicer wrapped.
> >
>
> I do not get your suggestion, can you rephrase please?
>
> I want the rate to be checked both for ACPI and DT.
>

Is the line over 80-cols maybe ? Is this what 'wrapped' was meant to
indicate ?

> > > >   		return -EINVAL;
> > > >   	}
> > > >
> > > > +	ov5675->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> > > > +						     GPIOD_OUT_HIGH);
> > > > +	if (IS_ERR(ov5675->reset_gpio)) {
> > > > +		ret = PTR_ERR(ov5675->reset_gpio);
> > > > +		dev_err(dev, "failed to get reset-gpios: %d\n", ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	for (i = 0; i < OV5675_NUM_SUPPLIES; i++)
> > > > +		ov5675->supplies[i].supply = ov5675_supply_names[i];
> > > > +
> > > > +	ret = devm_regulator_bulk_get(dev, OV5675_NUM_SUPPLIES,
> > > > +				      ov5675->supplies);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > >   	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
> > > >   	if (!ep)
> > > >   		return -ENXIO;
> > > > @@ -1186,6 +1277,8 @@ static int ov5675_remove(struct i2c_client *client)
> > > >   	pm_runtime_disable(&client->dev);
> > > >   	mutex_destroy(&ov5675->mutex);
> > > >
> > > > +	__ov5675_power_off(ov5675);
> > > > +
> > > >   	return 0;
> > > >   }
> > > >
> > > > @@ -1195,25 +1288,31 @@ static int ov5675_probe(struct i2c_client *client)
> > > >   	bool full_power;
> > > >   	int ret;
> > > >
> > > > -	ret = ov5675_check_hwcfg(&client->dev);
> > > > +	ov5675 = devm_kzalloc(&client->dev, sizeof(*ov5675), GFP_KERNEL);
> > > > +	if (!ov5675)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	ret = ov5675_get_hwcfg(ov5675, &client->dev);
> > > >   	if (ret) {
> > > > -		dev_err(&client->dev, "failed to check HW configuration: %d",
> > > > +		dev_err(&client->dev, "failed to get HW configuration: %d",
> > > >   			ret);
> > > >   		return ret;
> > > >   	}
> > > >
> > > > -	ov5675 = devm_kzalloc(&client->dev, sizeof(*ov5675), GFP_KERNEL);
> > > > -	if (!ov5675)
> > > > -		return -ENOMEM;
> > > > -
> > > >   	v4l2_i2c_subdev_init(&ov5675->sd, client, &ov5675_subdev_ops);
> > > >
> > > > +	ret = __ov5675_power_on(ov5675);
> > > > +	if (ret) {
> > > > +		dev_err(&client->dev, "failed to power on: %d\n", ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > >   	full_power = acpi_dev_state_d0(&client->dev);
> > > >   	if (full_power) {
> > > >   		ret = ov5675_identify_module(ov5675);
> > > >   		if (ret) {
> > > >   			dev_err(&client->dev, "failed to find sensor: %d", ret);
> > > > -			return ret;
> > > > +			goto probe_power_off;
> > > >   		}
> > > >   	}
> > >
> > > Maybe you can also update the comment at the end of the probe function
> > > to remove references to ACPI. As you wish.
> > >
> > > 	/*
> > > 	 * Device is already turned on by i2c-core with ACPI domain PM.
> > > 	 * Enable runtime PM and turn off the device.
> > > 	 */
> >
> > No need for such a comment --- nothing specific to this driver there.
> >
>
> The comment is already there in the driver.
>
> The issue being that if it's probed from ACPI, there's nothing to be done
> power wise (currently at least, in the current state of the driver). If it's
> probed via DT, I need to power the device manually.
>

Exactly, that's why the comment is either incomplete (and I suggested
to change it). Or, as Sakari pointed out, it's rather useless as this
is what happes in all drivers, so no need to point it out here.

Thanks
  j

> Cheers,
> Quentin

Re: [PATCH v2 2/3] media: ov5675: add device-tree support

[Sakari Ailus]

Hi Jacopo,

On Fri, May 06, 2022 at 04:43:00PM +0200, Jacopo Mondi wrote:
> My understanding is that "clock-frequency" is
> an ACPI leftover that has been brought into some DT bindings as an
> historical mistake. OF can work well with the common clock framework,
> there's no need to introduce a property for the same purpose.

The other way around actually. It was first used on DT but it's no longer
the perferred way to set the frequency there. On ACPI it simply indicates
the frequency without an ability to set it.

-- 
Sakari Ailus

Re: [PATCH v2 2/3] media: ov5675: add device-tree support

[Sakari Ailus]

Hi Quentin,

On Fri, May 06, 2022 at 04:13:07PM +0200, Quentin Schulz wrote:
> Sakari, Jacopo,
> 
> On 5/5/22 10:28, Sakari Ailus wrote:
> > Hi Jacopo, Quentin,
> > 
> > On Thu, May 05, 2022 at 09:47:25AM +0200, Jacopo Mondi wrote:
> > > Hi Quentin,
> > > 
> > > On Wed, May 04, 2022 at 03:55:42PM +0200, Quentin Schulz wrote:
> > > > From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > > > 
> > > > Until now, this driver only supported ACPI. This adds support for
> > > > Device Tree too.
> > > > 
> > > > This is heavily inspired by device tree support addition to OV8856
> > > > driver. The differentiation between ACPI and DT mode is done through an
> > > > is_acpi_node check.
> > > > 
> > > > Cc: Quentin Schulz <foss+kernel@0leil.net>
> > > > Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > > > ---
> > > > 
> > > > v2:
> > > >   - fixed unused-const-variable warning by removing of_match_ptr in
> > > >   of_match_table, reported by kernel test robot,
> > > > 
> > > >   drivers/media/i2c/ov5675.c | 134 +++++++++++++++++++++++++++++++++----
> > > >   1 file changed, 121 insertions(+), 13 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
> > > > index 82ba9f56baec..ccbc8dc506ff 100644
> > > > --- a/drivers/media/i2c/ov5675.c
> > > > +++ b/drivers/media/i2c/ov5675.c
> > > > @@ -3,10 +3,13 @@
> > > > 
> > > >   #include <asm/unaligned.h>
> > > >   #include <linux/acpi.h>
> > > > +#include <linux/clk.h>
> > > >   #include <linux/delay.h>
> > > > +#include <linux/gpio/consumer.h>
> > > >   #include <linux/i2c.h>
> > > 
> > > #include <linux/mod_devicetable.h>
> > > 
> > > for struct of_device_id
> > > 
> 
> Mmm.. Wondering why this is needed if it compiles fine without? What am I
> missing?
> 
> > > >   #include <linux/module.h>
> > > >   #include <linux/pm_runtime.h>
> > > > +#include <linux/regulator/consumer.h>
> > > >   #include <media/v4l2-ctrls.h>
> > > >   #include <media/v4l2-device.h>
> > > >   #include <media/v4l2-fwnode.h>
> > > > @@ -17,7 +20,7 @@
> > > > 
> > > >   #define OV5675_LINK_FREQ_450MHZ		450000000ULL
> > > >   #define OV5675_SCLK			90000000LL
> > > > -#define OV5675_MCLK			19200000
> > > > +#define OV5675_XVCLK_19_2		19200000
> > > >   #define OV5675_DATA_LANES		2
> > > >   #define OV5675_RGB_DEPTH		10
> > > > 
> > > > @@ -76,6 +79,14 @@
> > > > 
> > > >   #define to_ov5675(_sd)			container_of(_sd, struct ov5675, sd)
> > > > 
> > > > +static const char * const ov5675_supply_names[] = {
> > > > +	"avdd",		/* Analog power */
> > > > +	"dovdd",	/* Digital I/O power */
> > > > +	"dvdd",		/* Digital core power */
> > > > +};
> > > > +
> > > > +#define OV5675_NUM_SUPPLIES	ARRAY_SIZE(ov5675_supply_names)
> > > > +
> > > >   enum {
> > > >   	OV5675_LINK_FREQ_900MBPS,
> > > >   };
> > > > @@ -484,6 +495,9 @@ struct ov5675 {
> > > >   	struct v4l2_subdev sd;
> > > >   	struct media_pad pad;
> > > >   	struct v4l2_ctrl_handler ctrl_handler;
> > > > +	struct clk		*xvclk;
> > > > +	struct gpio_desc	*reset_gpio;
> > > > +	struct regulator_bulk_data supplies[OV5675_NUM_SUPPLIES];
> > > > 
> > > >   	/* V4L2 Controls */
> > > >   	struct v4l2_ctrl *link_freq;
> > > > @@ -944,6 +958,52 @@ static int ov5675_set_stream(struct v4l2_subdev *sd, int enable)
> > > >   	return ret;
> > > >   }
> > > > 
> > > > +static void __ov5675_power_off(struct ov5675 *ov5675)
> > > > +{
> > > > +	struct i2c_client *client = v4l2_get_subdevdata(&ov5675->sd);
> > > > +
> > > > +	if (is_acpi_node(dev_fwnode(&client->dev)))
> > > > +		return;
> > > > +
> > > > +	gpiod_set_value_cansleep(ov5675->reset_gpio, 1);
> > > > +	usleep_range(1000, 1200);
> > > > +
> > > > +	regulator_bulk_disable(OV5675_NUM_SUPPLIES, ov5675->supplies);
> > > > +	clk_disable_unprepare(ov5675->xvclk);
> > > > +}
> > > > +
> > > > +static int __ov5675_power_on(struct ov5675 *ov5675)
> > > > +{
> > > > +	struct i2c_client *client = v4l2_get_subdevdata(&ov5675->sd);
> > > > +	int ret;
> > > > +
> > > > +	if (is_acpi_node(dev_fwnode(&client->dev)))
> > > 
> > > A question for Sakari here:
> > > 
> > > I have a similar series for ov5670, where I don't use is_acpi_node()
> > > https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.linuxtv.org_project_linux-2Dmedia_patch_20220329090133.338073-2D7-2Djacopo-40jmondi.org_&d=DwIBAg&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=d4bhfjobjD_6CMiuzFRtZy27p0ytvQuScDxLXtsaHnzywAfKYeap61kXxLuejvfT&s=pnMz66bcaCWlXZZEIqknW9Atqkxp28rKJFT4lllZhuQ&e=
> > > 
> > > should this be done for all drivers supporting acpi && OF ?
> > 
> > It's better if you don't.
> > 
> > Regulators and GPIOs can be present in ACPI systems, too, I'm not sure
> > about clocks (maybe not yet?). If you check for ACPI and then bail out
> > here, the driver may not work on some systems.
> > 
> 
> Yet, the driver does not use regulators and GPIOs and is only probed via
> ACPI.
> 
> The fact is that today, the ACPI version does not need any of this but the
> Device Tree version does. If the ACPI version would need to support
> regulators and GPIOs too, I'd say that's out of topic for this patch series.
> Especially since I do not have anything to test ACPI version.
> 
> I expect the suggested implementation to not break (nor improve!) anything
> on ACPI.

It wouldn't break anything but it leaves some technical debt. People tend
to copy code (as you did from the other driver) and it had the same issue.

But fair enough, testing considered feel free to leave it this way now.

> 
> > On the other hand, you might be able to skip some of these delays in some
> > cases if the related resource isn't there. The datasheet probably tells
> > more of that.
> > 
> 
> I don't trust the datasheet.. I discovered that I need to wait for 2ms AFTER
> the regulators have been turned on to release the reset gpio and not just
> 2ms of holding reset gpio. Even though the datasheet states that there's
> literally no minimum delay between the rising of the power rails and the
> release of reset gpio. So the way I did in this patchset is not always
> working (actually less often than it does... will fix that in v2).

I wonder how long the regulator takes to ramp up the voltage. It won't
happen immediately. Does the regulator driver take that into account?

> 
> > I guess the driver or the example driver name in documentation need
> > some revising.
> > 
> > > 
> > > > +		return 0;
> > > > +
> > > > +	ret = clk_prepare_enable(ov5675->xvclk);
> > > > +	if (ret < 0) {
> > > > +		dev_err(&client->dev, "failed to enable xvclk: %d\n", ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	gpiod_set_value_cansleep(ov5675->reset_gpio, 1);
> > > > +
> > > > +	/* Reset pulse should be at least 2ms */
> > > > +	usleep_range(2000, 2200);
> > > > +
> > > > +	ret = regulator_bulk_enable(OV5675_NUM_SUPPLIES, ov5675->supplies);
> > > > +	if (ret) {
> > > > +		clk_disable_unprepare(ov5675->xvclk);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	gpiod_set_value_cansleep(ov5675->reset_gpio, 0);
> > > > +
> > > > +	usleep_range(1000, 1200);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >   static int __maybe_unused ov5675_suspend(struct device *dev)
> > > >   {
> > > >   	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > > > @@ -953,6 +1013,7 @@ static int __maybe_unused ov5675_suspend(struct device *dev)
> > > >   	if (ov5675->streaming)
> > > >   		ov5675_stop_streaming(ov5675);
> > > > 
> > > > +	__ov5675_power_off(ov5675);
> > > 
> > > So you plumb the device power/up down in the SYSTEM_SLEEP_PM_OPS() callbacks ?
> > > 
> > > My understanding is that it would be better to create RUNTIME_PM_OPS()
> > > for this, so that the device can be runtime suspended/resumed.
> 
> Can't test runtime PM for ACPI version and didn't want to risk breaking this
> support.
> 
> The point of this was to be of minimal impact on existing users. Also see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0c2c7a1e0d69221b9d489bfd8cf53262d6f82446
> 
> Not saying it is good, just pointing at the inspiration for this patch.
> 
> > 
> > Yes, please. The driver already uses runtime PM.
> > 
> 
> Does it? I only see SYSTEM_SLEEP_PM_OPS in the dev_pm_ops data structure. My
> reading of it is that it's not supported unless I add RUNTIME_PM_OPS? I'm
> probably missing some PM subsystem internals understanding though :/

It works with ACPI without these ops, too. You're supposed powering it on
and off explicitly only in the driver's probe and remove functions.

> 
> c.f. https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/ov5675.c#L1244
> 
> I have a doubt now we're still talking about this patch and not Jacopo's? At
> least I'm not sure to fully get what's being discussed here.

This patch, yes.

> 
> > > 
> > > Be aware my understanding of runtime_pm is limited, better check with
> > > Sakari too (I'll ask him to have a look).
> > > 
> > > >   	mutex_unlock(&ov5675->mutex);
> > > > 
> > > >   	return 0;
> > > > @@ -965,6 +1026,8 @@ static int __maybe_unused ov5675_resume(struct device *dev)
> > > >   	int ret;
> > > > 
> > > >   	mutex_lock(&ov5675->mutex);
> > > > +
> > > > +	__ov5675_power_on(ov5675);
> > > >   	if (ov5675->streaming) {
> > > >   		ret = ov5675_start_streaming(ov5675);
> > > >   		if (ret) {
> > > > @@ -1106,32 +1169,60 @@ static const struct v4l2_subdev_internal_ops ov5675_internal_ops = {
> > > >   	.open = ov5675_open,
> > > >   };
> > > > 
> > > > -static int ov5675_check_hwcfg(struct device *dev)
> > > > +static int ov5675_get_hwcfg(struct ov5675 *ov5675, struct device *dev)
> > > >   {
> > > >   	struct fwnode_handle *ep;
> > > >   	struct fwnode_handle *fwnode = dev_fwnode(dev);
> > > >   	struct v4l2_fwnode_endpoint bus_cfg = {
> > > >   		.bus_type = V4L2_MBUS_CSI2_DPHY
> > > >   	};
> > > > -	u32 mclk;
> > > > +	u32 xvclk_rate;
> > > >   	int ret;
> > > >   	unsigned int i, j;
> > > > 
> > > >   	if (!fwnode)
> > > >   		return -ENXIO;
> > > > 
> > > > -	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> > > > +	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &xvclk_rate);
> > > 
> > > Isn't "clock-frequency" a leftover from ACPI ? It shouldn't be in the OF
> > > bindings either (you have it in 1/3).
> > > 
> > > You can use the common clock framework API as you do below for OF and
> > > parse "clock-frequency" only for ACPI, as far as I can tell.
> > 
> > Older bindings had clock-frequency on DT, too, but newer ones rely on the
> > frequency being set using assigned-clock- stuff.
> > 
> > <URL:https://urldefense.proofpoint.com/v2/url?u=https-3A__hverkuil.home.xs4all.nl_spec_driver-2Dapi_camera-2Dsensor.html-23handling-2Dclocks&d=DwIBAg&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=d4bhfjobjD_6CMiuzFRtZy27p0ytvQuScDxLXtsaHnzywAfKYeap61kXxLuejvfT&s=TYhByXuRxctr7fGq94xBetcqW2gfnfZpGK3KyCZnCNA&e= >
> > 
> > But as discussed earlier, it's not possible to technically add these as
> > required properties albeit it's almost certainly a bug if they're not
> > present in dts.
> > 
> 
> I assume this can be "enforced" via dt-bindings checks? So, what's the
> decision here? Do I go for the clock framework version and lose the
> enforcing or do I keep using this one clock-frequency property for both ACPI
> and DT?

It needs to be kept for ACPI. But DT should use assigned-clock* properties.

drivers/media/i2c/imx258.c is an example of a driver supporting both the
preferred way.

> 
> > See e.g.
> > 
> 
> Missing link maybe?

See the imx258 driver above.

> 
> > > 
> > > >   	if (ret) {
> > > >   		dev_err(dev, "can't get clock frequency");
> > > >   		return ret;
> > > >   	}
> > > > 
> > > > -	if (mclk != OV5675_MCLK) {
> > > > -		dev_err(dev, "external clock %d is not supported", mclk);
> > > > +	if (!is_acpi_node(fwnode)) {
> > > > +		ov5675->xvclk = devm_clk_get(dev, "xvclk");
> > > > +		if (IS_ERR(ov5675->xvclk)) {
> > > > +			ret = PTR_ERR(ov5675->xvclk);
> > > > +			dev_err(dev, "failed to get xvclk: %d\n", ret);
> > > > +			return ret;
> > > > +		}
> > > > +
> > > > +		clk_set_rate(ov5675->xvclk, xvclk_rate);
> > > > +		xvclk_rate = clk_get_rate(ov5675->xvclk);
> > > 
> > > > +	}
> > > > +
> > > > +	if (xvclk_rate != OV5675_XVCLK_19_2) {
> > > > +		dev_err(dev, "external clock rate %u is unsupported", xvclk_rate);
> > 
> > This would be nicer wrapped.
> > 
> 
> I do not get your suggestion, can you rephrase please?

Please keep it under 80 columns unless there are tangible reasons not to.

> 
> I want the rate to be checked both for ACPI and DT.

Sure.

> 
> > > >   		return -EINVAL;
> > > >   	}
> > > > 
> > > > +	ov5675->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> > > > +						     GPIOD_OUT_HIGH);
> > > > +	if (IS_ERR(ov5675->reset_gpio)) {
> > > > +		ret = PTR_ERR(ov5675->reset_gpio);
> > > > +		dev_err(dev, "failed to get reset-gpios: %d\n", ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	for (i = 0; i < OV5675_NUM_SUPPLIES; i++)
> > > > +		ov5675->supplies[i].supply = ov5675_supply_names[i];
> > > > +
> > > > +	ret = devm_regulator_bulk_get(dev, OV5675_NUM_SUPPLIES,
> > > > +				      ov5675->supplies);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > >   	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
> > > >   	if (!ep)
> > > >   		return -ENXIO;
> > > > @@ -1186,6 +1277,8 @@ static int ov5675_remove(struct i2c_client *client)
> > > >   	pm_runtime_disable(&client->dev);
> > > >   	mutex_destroy(&ov5675->mutex);
> > > > 
> > > > +	__ov5675_power_off(ov5675);
> > > > +
> > > >   	return 0;
> > > >   }
> > > > 
> > > > @@ -1195,25 +1288,31 @@ static int ov5675_probe(struct i2c_client *client)
> > > >   	bool full_power;
> > > >   	int ret;
> > > > 
> > > > -	ret = ov5675_check_hwcfg(&client->dev);
> > > > +	ov5675 = devm_kzalloc(&client->dev, sizeof(*ov5675), GFP_KERNEL);
> > > > +	if (!ov5675)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	ret = ov5675_get_hwcfg(ov5675, &client->dev);
> > > >   	if (ret) {
> > > > -		dev_err(&client->dev, "failed to check HW configuration: %d",
> > > > +		dev_err(&client->dev, "failed to get HW configuration: %d",
> > > >   			ret);
> > > >   		return ret;
> > > >   	}
> > > > 
> > > > -	ov5675 = devm_kzalloc(&client->dev, sizeof(*ov5675), GFP_KERNEL);
> > > > -	if (!ov5675)
> > > > -		return -ENOMEM;
> > > > -
> > > >   	v4l2_i2c_subdev_init(&ov5675->sd, client, &ov5675_subdev_ops);
> > > > 
> > > > +	ret = __ov5675_power_on(ov5675);
> > > > +	if (ret) {
> > > > +		dev_err(&client->dev, "failed to power on: %d\n", ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > >   	full_power = acpi_dev_state_d0(&client->dev);
> > > >   	if (full_power) {
> > > >   		ret = ov5675_identify_module(ov5675);
> > > >   		if (ret) {
> > > >   			dev_err(&client->dev, "failed to find sensor: %d", ret);
> > > > -			return ret;
> > > > +			goto probe_power_off;
> > > >   		}
> > > >   	}
> > > 
> > > Maybe you can also update the comment at the end of the probe function
> > > to remove references to ACPI. As you wish.
> > > 
> > > 	/*
> > > 	 * Device is already turned on by i2c-core with ACPI domain PM.
> > > 	 * Enable runtime PM and turn off the device.
> > > 	 */
> > 
> > No need for such a comment --- nothing specific to this driver there.
> > 
> 
> The comment is already there in the driver.

Ok. Feel free to leave as-is but can be removed, too.

> 
> The issue being that if it's probed from ACPI, there's nothing to be done
> power wise (currently at least, in the current state of the driver). If it's
> probed via DT, I need to power the device manually.

For probe and remove, yes.

-- 
Kind regards,

Sakari Ailus

[PATCH v2 3/3] media: i2c: ov5675: parse and register V4L2 device tree properties

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

Parse V4L2 device tree properties and register controls for them.

Cc: Quentin Schulz <foss+kernel@0leil.net>
Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---
 drivers/media/i2c/ov5675.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
index ccbc8dc506ff..324992fca557 100644
--- a/drivers/media/i2c/ov5675.c
+++ b/drivers/media/i2c/ov5675.c
@@ -778,12 +778,14 @@ static const struct v4l2_ctrl_ops ov5675_ctrl_ops = {
 
 static int ov5675_init_controls(struct ov5675 *ov5675)
 {
+	struct i2c_client *client = v4l2_get_subdevdata(&ov5675->sd);
 	struct v4l2_ctrl_handler *ctrl_hdlr;
+	struct v4l2_fwnode_device_properties props;
 	s64 exposure_max, h_blank;
 	int ret;
 
 	ctrl_hdlr = &ov5675->ctrl_handler;
-	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8);
+	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10);
 	if (ret)
 		return ret;
 
@@ -837,9 +839,23 @@ static int ov5675_init_controls(struct ov5675 *ov5675)
 	if (ctrl_hdlr->error)
 		return ctrl_hdlr->error;
 
+	ret = v4l2_fwnode_device_parse(&client->dev, &props);
+	if (ret)
+		goto error;
+
+	ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &ov5675_ctrl_ops,
+					      &props);
+	if (ret)
+		goto error;
+
 	ov5675->sd.ctrl_handler = ctrl_hdlr;
 
 	return 0;
+
+error:
+	v4l2_ctrl_handler_free(ctrl_hdlr);
+
+	return ret;
 }
 
 static void ov5675_update_pad_format(const struct ov5675_mode *mode,
-- 
2.35.1

Re: [PATCH v2 1/3] media: dt-bindings: ov5675: document YAML binding

[Krzysztof Kozlowski]

On 04/05/2022 15:55, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> 
> This patch adds documentation of device tree in YAML schema for the
> OV5675 CMOS image sensor from Omnivision.
> 
> Cc: Quentin Schulz <foss+kernel@0leil.net>

Don't Cc yourself in commits. This goes to the Git history, so
assumption is that the "other you" knows that you sent it. :)

> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> ---
> 
> v2:
>  - fixed incorrect id,
>  - fixed device tree example by adding missing dt-bindings headers,
>  - fixed device tree example by using vcc_1v2 for dvdd supply, as requested
>  in datasheet,
> 
>  .../bindings/media/i2c/ovti,ov5675.yaml       | 139 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 140 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml
> new file mode 100644
> index 000000000000..29df2f82c631
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml
> @@ -0,0 +1,139 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (c) 2022 Theobroma Systems Design und Consulting GmbH
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/ovti,ov5675.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Omnivision OV5675 CMOS Sensor Device Tree Bindings

s/Device Tree Bindings//

> +
> +maintainers:
> +  - Quentin Schulz <quentin.schulz@theobroma-systems.com>
> +
> +description: |-
> +  The Omnivision OV5675 is a high performance, 1/5-inch, 5 megapixel, CMOS
> +  image sensor that delivers 2592x1944 at 30fps. It provides full-frame,
> +  sub-sampled, and windowed 10-bit MIPI images in various formats via the
> +  Serial Camera Control Bus (SCCB) interface. This chip is programmable
> +  through I2C and two-wire SCCB. The sensor output is available via CSI-2
> +  serial data output (up to 2-lane).
> +
> +properties:
> +  compatible:
> +    const: ovti,ov5675
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    description:
> +      Input clock for the sensor.
> +    items:
> +      - const: xvclk

Just "xv" is preferred.

> +
> +  clock-frequency:
> +    description:
> +      Frequency of the xvclk clock in Hertz.
> +
> +  dovdd-supply:
> +    description:
> +      Definition of the regulator used as interface power supply.
> +
> +  avdd-supply:
> +    description:
> +      Definition of the regulator used as analog power supply.
> +
> +  dvdd-supply:
> +    description:
> +      Definition of the regulator used as digital power supply.
> +
> +  reset-gpios:
> +    description:
> +      The phandle and specifier for the GPIO that controls sensor reset.
> +      This corresponds to the hardware pin XSHUTDOWN which is physically
> +      active low.

Needs maxItems

> +
> +  port:
> +    type: object

Open other bindings and compare how it is done there. This looks like
/schemas/graph.yaml#/$defs/port-base

> +    additionalProperties: false
> +    description:
> +      A node containing an output port node with an endpoint definition
> +      as documented in
> +      Documentation/devicetree/bindings/media/video-interfaces.txt
> +
> +    properties:
> +      endpoint:
> +        type: object

Missing ref

> +
> +        properties:
> +          data-lanes:
> +            description: |-

No need for "|-"

> +              The driver only supports 2-lane operation.

Please remove references to driver. It's not part of hardware.

> +            items:
> +              - const: 1
> +              - const: 2
> +
> +          link-frequencies:
> +            $ref: /schemas/types.yaml#/definitions/uint64-array

The ref should be already provided by video-interfaces.

> +            description:
> +              Allowed data bus frequencies. 450000000Hz is supported by the driver.

Again, skip driver reference. However you need to describe the number of
items.

>
Best regards,
Krzysztof

Re: [PATCH v2 1/3] media: dt-bindings: ov5675: document YAML binding

[Quentin Schulz]

Hi Krzysztof,

On 5/4/22 16:46, Krzysztof Kozlowski wrote:
> On 04/05/2022 15:55, Quentin Schulz wrote:
>> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>
>> This patch adds documentation of device tree in YAML schema for the
>> OV5675 CMOS image sensor from Omnivision.
>>
>> Cc: Quentin Schulz <foss+kernel@0leil.net>
> 
> Don't Cc yourself in commits. This goes to the Git history, so
> assumption is that the "other you" knows that you sent it. :)
> 

It's a habit I've taken so that in the event I switch jobs, there's 
still an address where I can be reached.

But I forgot the kernel as a .mailmap :) so I can avoid doing it.

>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>> ---
>>
>> v2:
>>   - fixed incorrect id,
>>   - fixed device tree example by adding missing dt-bindings headers,
>>   - fixed device tree example by using vcc_1v2 for dvdd supply, as requested
>>   in datasheet,
>>
>>   .../bindings/media/i2c/ovti,ov5675.yaml       | 139 ++++++++++++++++++
>>   MAINTAINERS                                   |   1 +
>>   2 files changed, 140 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml
>> new file mode 100644
>> index 000000000000..29df2f82c631
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml
>> @@ -0,0 +1,139 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +# Copyright (c) 2022 Theobroma Systems Design und Consulting GmbH
>> +%YAML 1.2
>> +---
>> +$id: https://urldefense.proofpoint.com/v2/url?u=http-3A__devicetree.org_schemas_media_i2c_ovti-2Cov5675.yaml-23&d=DwICaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=5gh6rUPawGp8Fw7sPXVsvOViSUSnEVGWWB9RK_CSQCeBjoEfunHaI0w2GiB0zjq_&s=W_iQiFCa2XM4k350eNawjzJR2ZTUuSx-VM4E1iuknz4&e=
>> +$schema: https://urldefense.proofpoint.com/v2/url?u=http-3A__devicetree.org_meta-2Dschemas_core.yaml-23&d=DwICaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=5gh6rUPawGp8Fw7sPXVsvOViSUSnEVGWWB9RK_CSQCeBjoEfunHaI0w2GiB0zjq_&s=nrri3o0sr-6AopqJCaHZ94dUfOwS8r1V7ybfSwD7v2M&e=
>> +
>> +title: Omnivision OV5675 CMOS Sensor Device Tree Bindings
> 
> s/Device Tree Bindings//
> 
>> +
>> +maintainers:
>> +  - Quentin Schulz <quentin.schulz@theobroma-systems.com>
>> +
>> +description: |-
>> +  The Omnivision OV5675 is a high performance, 1/5-inch, 5 megapixel, CMOS
>> +  image sensor that delivers 2592x1944 at 30fps. It provides full-frame,
>> +  sub-sampled, and windowed 10-bit MIPI images in various formats via the
>> +  Serial Camera Control Bus (SCCB) interface. This chip is programmable
>> +  through I2C and two-wire SCCB. The sensor output is available via CSI-2
>> +  serial data output (up to 2-lane).
>> +
>> +properties:
>> +  compatible:
>> +    const: ovti,ov5675
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  clock-names:
>> +    description:
>> +      Input clock for the sensor.
>> +    items:
>> +      - const: xvclk
> 
> Just "xv" is preferred.
> 

The name of the clock in the datasheet is XVCLK though. Wouldn't it be 
confusing to describe HW by using names different from the datasheet?

>> +
>> +  clock-frequency:
>> +    description:
>> +      Frequency of the xvclk clock in Hertz.
>> +
>> +  dovdd-supply:
>> +    description:
>> +      Definition of the regulator used as interface power supply.
>> +
>> +  avdd-supply:
>> +    description:
>> +      Definition of the regulator used as analog power supply.
>> +
>> +  dvdd-supply:
>> +    description:
>> +      Definition of the regulator used as digital power supply.
>> +
>> +  reset-gpios:
>> +    description:
>> +      The phandle and specifier for the GPIO that controls sensor reset.
>> +      This corresponds to the hardware pin XSHUTDOWN which is physically
>> +      active low.
> 
> Needs maxItems
> 
>> +
>> +  port:
>> +    type: object
> 
> Open other bindings and compare how it is done there. This looks like
> /schemas/graph.yaml#/$defs/port-base
> 

Did that but used an old kernel as base :/

>> +    additionalProperties: false
>> +    description:
>> +      A node containing an output port node with an endpoint definition
>> +      as documented in
>> +      Documentation/devicetree/bindings/media/video-interfaces.txt
>> +
>> +    properties:
>> +      endpoint:
>> +        type: object
> 
> Missing ref
> 
>> +
>> +        properties:
>> +          data-lanes:
>> +            description: |-
> 
> No need for "|-"
> 
>> +              The driver only supports 2-lane operation.
> 
> Please remove references to driver. It's not part of hardware.
> 
>> +            items:
>> +              - const: 1
>> +              - const: 2
>> +
>> +          link-frequencies:
>> +            $ref: /schemas/types.yaml#/definitions/uint64-array
> 
> The ref should be already provided by video-interfaces.
> 
>> +            description:
>> +              Allowed data bus frequencies. 450000000Hz is supported by the driver.
> 
> Again, skip driver reference. However you need to describe the number of
> items.
> 

Currently, the driver is limited to 450 MHz link-freq and 2 data lanes, 
while the HW advertises: "The OV5675 supports a MIPI interface of up to 
2-lanes. The MIPI interface can be configured for 1/2-lane and each lane

is capable of a data transfer rate of up to 900 Mbps."

Was wondering what I am supposed to do in this situation as I see 
Documentation/devicetree/bindings/media/i2c/ov8856.yaml mentioning 
driver limitations in the dt-bindings.

Cheers,
Quentin

Re: [PATCH v2 1/3] media: dt-bindings: ov5675: document YAML binding

[Krzysztof Kozlowski]

On 06/05/2022 15:48, Quentin Schulz wrote:
>>> +  clock-names:
>>> +    description:
>>> +      Input clock for the sensor.
>>> +    items:
>>> +      - const: xvclk
>>
>> Just "xv" is preferred.
>>
> 
> The name of the clock in the datasheet is XVCLK though. Wouldn't it be 
> confusing to describe HW by using names different from the datasheet?

No, because datasheet could call it "xvclk_clk_clk_clk" and it is not a
reason to use it in the bindings. All of these are clocks, so don't add
unnecessary suffixes. The same goes to interrupts (wake not wakeirq) or
DMA (tx not txdma).

> 
>>> +
>>> +  clock-frequency:
>>> +    description:
>>> +      Frequency of the xvclk clock in Hertz.
>>> +
>>> +  dovdd-supply:
>>> +    description:
>>> +      Definition of the regulator used as interface power supply.
>>> +
>>> +  avdd-supply:
>>> +    description:
>>> +      Definition of the regulator used as analog power supply.
>>> +
>>> +  dvdd-supply:
>>> +    description:
>>> +      Definition of the regulator used as digital power supply.
>>> +
>>> +  reset-gpios:
>>> +    description:
>>> +      The phandle and specifier for the GPIO that controls sensor reset.
>>> +      This corresponds to the hardware pin XSHUTDOWN which is physically
>>> +      active low.
>>
>> Needs maxItems
>>
>>> +
>>> +  port:
>>> +    type: object
>>
>> Open other bindings and compare how it is done there. This looks like
>> /schemas/graph.yaml#/$defs/port-base
>>
> 
> Did that but used an old kernel as base :/

Then please do not develop on an older kernel.

> 
>>> +    additionalProperties: false
>>> +    description:
>>> +      A node containing an output port node with an endpoint definition
>>> +      as documented in
>>> +      Documentation/devicetree/bindings/media/video-interfaces.txt
>>> +
>>> +    properties:
>>> +      endpoint:
>>> +        type: object
>>
>> Missing ref
>>
>>> +
>>> +        properties:
>>> +          data-lanes:
>>> +            description: |-
>>
>> No need for "|-"
>>
>>> +              The driver only supports 2-lane operation.
>>
>> Please remove references to driver. It's not part of hardware.
>>
>>> +            items:
>>> +              - const: 1
>>> +              - const: 2
>>> +
>>> +          link-frequencies:
>>> +            $ref: /schemas/types.yaml#/definitions/uint64-array
>>
>> The ref should be already provided by video-interfaces.
>>
>>> +            description:
>>> +              Allowed data bus frequencies. 450000000Hz is supported by the driver.
>>
>> Again, skip driver reference. However you need to describe the number of
>> items.
>>
> 
> Currently, the driver is limited to 450 MHz link-freq and 2 data lanes, 
> while the HW advertises: "The OV5675 supports a MIPI interface of up to 
> 2-lanes. The MIPI interface can be configured for 1/2-lane and each lane
> 
> is capable of a data transfer rate of up to 900 Mbps."
> 
> Was wondering what I am supposed to do in this situation as I see 
> Documentation/devicetree/bindings/media/i2c/ov8856.yaml mentioning 
> driver limitations in the dt-bindings.

Bindings describe the hardware and they are used in different projects.
Let's say Linux implementation supports only 450 MHz, but other project
supports 450 and 900, so your bindings would be incorrect in such
case... IOW, bindings should not depend on the implementation.

What is more, the driver might get updated without updating the comments
in the bindings making them incorrect even for Linux.

In the past several bindings contained actual specifics of
implementation, but this is usually not the proper way.

There are clear issues with describing implementation in the bindings,
but what are the benefits?

Best regards,
Krzysztof