[PATCH v4 0/5] ov5645: Deprecate usage of the clock-frequency

[PATCH v4 0/5] ov5645: Deprecate usage of the clock-frequency

[Lad Prabhakar]

Hi All,

This patch series does the following:
1: Deprecate usage of the clock-frequency property.
2: Increases tolerance level to 5% for external clock frequency
3: Converts bindings to json-schema

Thanks,
Prabhakar

Changes for v4:
* Addressed to comments from Laurent updated patch description for
  patch 1/5, 4/5.
* Included Reviewed-by tags from Laurent.
* Increased tolerance level to 5% for external clock frequency.
* Patch 5/5 is new patch which converts bindings to json-schema.

Changed for v3:
* Dropped reading assigned-clock-rates
* Increased the maximum clock frequency to 24480000

Changes for v2:
* Instead of completely dropping clock-frequency property marked it as
  deprecated.
* Split up imx6qdl-wandboard.dtsi changes as separate patch.

Lad Prabhakar (5):
  media: dt-bindings: media: i2c: Deprecate usage of the clock-frequency
    property
  media: i2c: ov5645: Switch to assigned-clock-rates
  media: i2c: ov5645: Increase tolerance of external clock frequency
  ARM: dts: imx6qdl-wandboard: Switch to assigned-clock-rates for ov5645
    node
  media: dt-bindings: media: i2c: convert ov5645 bindings to json-schema

 .../devicetree/bindings/media/i2c/ov5645.txt  |  54 -------
 .../devicetree/bindings/media/i2c/ov5645.yaml | 140 ++++++++++++++++++
 arch/arm/boot/dts/imx6qdl-wandboard.dtsi      |   3 +-
 drivers/media/i2c/ov5645.c                    |  30 ++--
 4 files changed, 156 insertions(+), 71 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.yaml

-- 
2.20.1

[PATCH v4 1/5] media: dt-bindings: media: i2c: Deprecate usage of the clock-frequency property

[Lad Prabhakar]

Deprecate usage of the clock-frequency property. The preferred method to
set clock rates is to use assigned-clock-rates.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 Documentation/devicetree/bindings/media/i2c/ov5645.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
index 72ad992f77be..1c85c78ec58c 100644
--- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
+++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
@@ -8,7 +8,6 @@ Required Properties:
 - compatible: Value should be "ovti,ov5645".
 - clocks: Reference to the xclk clock.
 - clock-names: Should be "xclk".
-- clock-frequency: Frequency of the xclk clock.
 - enable-gpios: Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH. This corresponds
   to the hardware pin PWDNB which is physically active low.
 - reset-gpios: Chip reset GPIO. Polarity is GPIO_ACTIVE_LOW. This corresponds to
@@ -37,7 +36,8 @@ Example:
 
 			clocks = <&clks 200>;
 			clock-names = "xclk";
-			clock-frequency = <24000000>;
+			assigned-clocks = <&clks 200>;
+			assigned-clock-rates = <24000000>;
 
 			vdddo-supply = <&camera_dovdd_1v8>;
 			vdda-supply = <&camera_avdd_2v8>;
-- 
2.20.1

Re: [PATCH v4 1/5] media: dt-bindings: media: i2c: Deprecate usage of the clock-frequency property

[Rob Herring]

On Thu, Mar 19, 2020 at 12:19:19PM +0000, Lad Prabhakar wrote:
> Deprecate usage of the clock-frequency property. The preferred method to
> set clock rates is to use assigned-clock-rates.

Somewhere here and in the subject you should have 'ov5645'. Otherwise,

Acked-by: Rob Herring <robh@kernel.org>

> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  Documentation/devicetree/bindings/media/i2c/ov5645.txt | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

[PATCH v4 2/5] media: i2c: ov5645: Switch to assigned-clock-rates

[Lad Prabhakar]

This patch switches to assigned-clock-rates for specifying the clock rate.
The clk-conf.c internally handles setting the clock rate when
assigned-clock-rates is passed.

The driver now sets the clock frequency only if the deprecated property
clock-frequency is defined instead assigned-clock-rates, this is to avoid
breakage with existing DT binaries.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/ov5645.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index a6c17d15d754..e298acdadeef 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -1094,25 +1094,25 @@ static int ov5645_probe(struct i2c_client *client)
 		return PTR_ERR(ov5645->xclk);
 	}
 
-	ret = of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq);
-	if (ret) {
-		dev_err(dev, "could not get xclk frequency\n");
-		return ret;
+	/* check if deprecated property clock-frequency is defined */
+	ret = of_property_read_u32(dev->of_node, "clock-frequency",
+				   &xclk_freq);
+	if (!ret) {
+		ret = clk_set_rate(ov5645->xclk, xclk_freq);
+		if (ret) {
+			dev_err(dev, "could not set xclk frequency\n");
+			return ret;
+		}
 	}
 
 	/* external clock must be 24MHz, allow 1% tolerance */
+	xclk_freq = clk_get_rate(ov5645->xclk);
 	if (xclk_freq < 23760000 || xclk_freq > 24240000) {
 		dev_err(dev, "external clock frequency %u is not supported\n",
 			xclk_freq);
 		return -EINVAL;
 	}
 
-	ret = clk_set_rate(ov5645->xclk, xclk_freq);
-	if (ret) {
-		dev_err(dev, "could not set xclk frequency\n");
-		return ret;
-	}
-
 	for (i = 0; i < OV5645_NUM_SUPPLIES; i++)
 		ov5645->supplies[i].supply = ov5645_supply_name[i];
 
-- 
2.20.1

[PATCH v4 3/5] media: i2c: ov5645: Increase tolerance of external clock frequency

[Lad Prabhakar]

While testing on Renesas RZ/G2E platform, noticed the clock frequency to
be 24242424 as a result the probe failed.

This patch increases the tolerance to 5% so that it avoids patching for
new platforms and it warns the users if the frequency is not within the
range and continue further in the probe instead of returning failure.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/ov5645.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index e298acdadeef..52a185ed4368 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -1105,13 +1105,11 @@ static int ov5645_probe(struct i2c_client *client)
 		}
 	}
 
-	/* external clock must be 24MHz, allow 1% tolerance */
+	/* ideally external clock must be 24MHz, allow 5% tolerance */
 	xclk_freq = clk_get_rate(ov5645->xclk);
-	if (xclk_freq < 23760000 || xclk_freq > 24240000) {
-		dev_err(dev, "external clock frequency %u is not supported\n",
-			xclk_freq);
-		return -EINVAL;
-	}
+	if (xclk_freq < 22800000 || xclk_freq > 25200000)
+		dev_warn(dev, "external clock frequency is set to %u, sensor might misbehave\n",
+			 xclk_freq);
 
 	for (i = 0; i < OV5645_NUM_SUPPLIES; i++)
 		ov5645->supplies[i].supply = ov5645_supply_name[i];
-- 
2.20.1

Re: [PATCH v4 3/5] media: i2c: ov5645: Increase tolerance of external clock frequency

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Thu, Mar 19, 2020 at 12:19:21PM +0000, Lad Prabhakar wrote:
> While testing on Renesas RZ/G2E platform, noticed the clock frequency to
> be 24242424 as a result the probe failed.
> 
> This patch increases the tolerance to 5% so that it avoids patching for
> new platforms and it warns the users if the frequency is not within the
> range and continue further in the probe instead of returning failure.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/media/i2c/ov5645.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index e298acdadeef..52a185ed4368 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -1105,13 +1105,11 @@ static int ov5645_probe(struct i2c_client *client)
>  		}
>  	}
>  
> -	/* external clock must be 24MHz, allow 1% tolerance */
> +	/* ideally external clock must be 24MHz, allow 5% tolerance */
>  	xclk_freq = clk_get_rate(ov5645->xclk);
> -	if (xclk_freq < 23760000 || xclk_freq > 24240000) {
> -		dev_err(dev, "external clock frequency %u is not supported\n",
> -			xclk_freq);
> -		return -EINVAL;
> -	}
> +	if (xclk_freq < 22800000 || xclk_freq > 25200000)
> +		dev_warn(dev, "external clock frequency is set to %u, sensor might misbehave\n",
> +			 xclk_freq);

The code looks good to me. You may want to mention in the commit subject
that the probe error is turned into a warning, but that may be hard to
do on a single line. Splitting this in two patches could be best, but is
not worth a new version if it's the only change required. If more
changes are required, you can consider it.

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

>  	for (i = 0; i < OV5645_NUM_SUPPLIES; i++)
>  		ov5645->supplies[i].supply = ov5645_supply_name[i];

-- 
Regards,

Laurent Pinchart

[PATCH v4 4/5] ARM: dts: imx6qdl-wandboard: Switch to assigned-clock-rates for ov5645 node

[Lad Prabhakar]

clock-frequency property has been deprecated in ov5645 binding, so switch
to assigned-clock-rates for specifying xclk clock frequency.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 arch/arm/boot/dts/imx6qdl-wandboard.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/imx6qdl-wandboard.dtsi b/arch/arm/boot/dts/imx6qdl-wandboard.dtsi
index c070893c509e..71f5f75a0aa2 100644
--- a/arch/arm/boot/dts/imx6qdl-wandboard.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-wandboard.dtsi
@@ -126,7 +126,8 @@
 		reg = <0x3c>;
 		clocks = <&clks IMX6QDL_CLK_CKO2>;
 		clock-names = "xclk";
-		clock-frequency = <24000000>;
+		assigned-clocks = <&clks IMX6QDL_CLK_CKO2>;
+		assigned-clock-rates = <24000000>;
 		vdddo-supply = <&reg_1p8v>;
 		vdda-supply = <&reg_2p8v>;
 		vddd-supply = <&reg_1p5v>;
-- 
2.20.1

[PATCH v4 5/5] media: dt-bindings: media: i2c: convert ov5645 bindings to json-schema

[Lad Prabhakar]

Convert ov5645 bindings to json-schema.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 .../devicetree/bindings/media/i2c/ov5645.txt  |  54 -------
 .../devicetree/bindings/media/i2c/ov5645.yaml | 140 ++++++++++++++++++
 2 files changed, 140 insertions(+), 54 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
deleted file mode 100644
index 1c85c78ec58c..000000000000
--- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
+++ /dev/null
@@ -1,54 +0,0 @@
-* Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor
-
-The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image sensor with
-an active array size of 2592H x 1944V. It is programmable through a serial I2C
-interface.
-
-Required Properties:
-- compatible: Value should be "ovti,ov5645".
-- clocks: Reference to the xclk clock.
-- clock-names: Should be "xclk".
-- enable-gpios: Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH. This corresponds
-  to the hardware pin PWDNB which is physically active low.
-- reset-gpios: Chip reset GPIO. Polarity is GPIO_ACTIVE_LOW. This corresponds to
-  the hardware pin RESETB.
-- vdddo-supply: Chip digital IO regulator.
-- vdda-supply: Chip analog regulator.
-- vddd-supply: Chip digital core regulator.
-
-The device node must contain one 'port' child node for its digital output
-video port, in accordance with the video interface bindings defined in
-Documentation/devicetree/bindings/media/video-interfaces.txt.
-
-Example:
-
-	&i2c1 {
-		...
-
-		ov5645: ov5645@3c {
-			compatible = "ovti,ov5645";
-			reg = <0x3c>;
-
-			enable-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>;
-			reset-gpios = <&gpio5 20 GPIO_ACTIVE_LOW>;
-			pinctrl-names = "default";
-			pinctrl-0 = <&camera_rear_default>;
-
-			clocks = <&clks 200>;
-			clock-names = "xclk";
-			assigned-clocks = <&clks 200>;
-			assigned-clock-rates = <24000000>;
-
-			vdddo-supply = <&camera_dovdd_1v8>;
-			vdda-supply = <&camera_avdd_2v8>;
-			vddd-supply = <&camera_dvdd_1v2>;
-
-			port {
-				ov5645_ep: endpoint {
-					clock-lanes = <1>;
-					data-lanes = <0 2>;
-					remote-endpoint = <&csi0_ep>;
-				};
-			};
-		};
-	};
diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.yaml b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml
new file mode 100644
index 000000000000..4bf58ad210c5
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml
@@ -0,0 +1,140 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/ov5645.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor
+
+maintainers:
+  - Sakari Ailus <sakari.ailus@linux.intel.com>
+  - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
+
+description: |-
+ The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image sensor with
+ an active array size of 2592H x 1944V. It is programmable through a serial I2C
+ interface.
+
+properties:
+  compatible:
+    const: ovti,ov5645
+
+  reg:
+    description: I2C device address
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    items:
+      - const: xclk
+
+  assigned-clocks:
+    maxItems: 1
+
+  assigned-clock-rates:
+     items:
+     - description: Must be 24MHz (24000000).
+
+  enable-gpios:
+    description: |-
+      Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH. This corresponds
+      to the hardware pin PWDNB which is physically active low.
+
+  reset-gpios:
+    description: |-
+      Chip reset GPIO. Polarity is GPIO_ACTIVE_LOW. This corresponds to
+      the hardware pin RESETB.
+
+  vdddo-supply:
+    description:
+      Chip digital IO regulator.
+
+  vdda-supply:
+    description:
+      Chip analog regulator.
+
+  vddd-supply:
+    description:
+      Chip digital core regulator.
+
+  # See ../video-interfaces.txt for more details
+  port:
+    type: object
+    properties:
+      endpoint:
+        type: object
+
+        properties:
+          data-lanes:
+            description: |-
+              The sensor supports two-lane operation.
+              For two-lane operation the property must be set to <1 2>.
+            items:
+              - const: 1
+              - const: 2
+
+          clock-lanes:
+            description:
+              should be set to <0> (clock lane on hardware lane 0).
+            items:
+              - const: 0
+
+          remote-endpoint: true
+
+        required:
+          - data-lanes
+          - clock-lanes
+          - remote-endpoint
+
+        additionalProperties: false
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - assigned-clocks
+  - assigned-clock-rates
+  - enable-gpios
+  - reset-gpios
+  - vdddo-supply
+  - vdda-supply
+  - vddd-supply
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c1 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ov5645: sensor@3c {
+            compatible = "ovti,ov5645";
+            reg = <0x3c>;
+            clocks = <&ov5645_cl>;
+            clock-names = "xclk";
+            assigned-clocks = <&ov5645_cl>;
+            assigned-clock-rates = <24000000>;
+            enable-gpios = <&gpio1 6 /* GPIO_ACTIVE_HIGH */>;
+            reset-gpios = <&gpio5 20 /* GPIO_ACTIVE_LOW */>;
+            vdddo-supply = <&camera_dovdd_1v8>;
+            vdda-supply = <&camera_avdd_2v8>;
+            vddd-supply = <&camera_dvdd_1v2>;
+
+            port {
+                ov5645_0: endpoint {
+                    remote-endpoint = <&csi1_ep>;
+                    clock-lanes = <0>;
+                    data-lanes = <1 2>;
+                };
+            };
+        };
+    };
+
+...
-- 
2.20.1

Re: [PATCH v4 5/5] media: dt-bindings: media: i2c: convert ov5645 bindings to json-schema

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Thu, Mar 19, 2020 at 12:19:23PM +0000, Lad Prabhakar wrote:
> Convert ov5645 bindings to json-schema.

\o/

> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  .../devicetree/bindings/media/i2c/ov5645.txt  |  54 -------
>  .../devicetree/bindings/media/i2c/ov5645.yaml | 140 ++++++++++++++++++
>  2 files changed, 140 insertions(+), 54 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> deleted file mode 100644
> index 1c85c78ec58c..000000000000
> --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> +++ /dev/null
> @@ -1,54 +0,0 @@
> -* Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor
> -
> -The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image sensor with
> -an active array size of 2592H x 1944V. It is programmable through a serial I2C
> -interface.
> -
> -Required Properties:
> -- compatible: Value should be "ovti,ov5645".
> -- clocks: Reference to the xclk clock.
> -- clock-names: Should be "xclk".
> -- enable-gpios: Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH. This corresponds
> -  to the hardware pin PWDNB which is physically active low.
> -- reset-gpios: Chip reset GPIO. Polarity is GPIO_ACTIVE_LOW. This corresponds to
> -  the hardware pin RESETB.
> -- vdddo-supply: Chip digital IO regulator.
> -- vdda-supply: Chip analog regulator.
> -- vddd-supply: Chip digital core regulator.
> -
> -The device node must contain one 'port' child node for its digital output
> -video port, in accordance with the video interface bindings defined in
> -Documentation/devicetree/bindings/media/video-interfaces.txt.
> -
> -Example:
> -
> -	&i2c1 {
> -		...
> -
> -		ov5645: ov5645@3c {
> -			compatible = "ovti,ov5645";
> -			reg = <0x3c>;
> -
> -			enable-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>;
> -			reset-gpios = <&gpio5 20 GPIO_ACTIVE_LOW>;
> -			pinctrl-names = "default";
> -			pinctrl-0 = <&camera_rear_default>;
> -
> -			clocks = <&clks 200>;
> -			clock-names = "xclk";
> -			assigned-clocks = <&clks 200>;
> -			assigned-clock-rates = <24000000>;
> -
> -			vdddo-supply = <&camera_dovdd_1v8>;
> -			vdda-supply = <&camera_avdd_2v8>;
> -			vddd-supply = <&camera_dvdd_1v2>;
> -
> -			port {
> -				ov5645_ep: endpoint {
> -					clock-lanes = <1>;
> -					data-lanes = <0 2>;
> -					remote-endpoint = <&csi0_ep>;
> -				};
> -			};
> -		};
> -	};
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.yaml b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml
> new file mode 100644
> index 000000000000..4bf58ad210c5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml
> @@ -0,0 +1,140 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/ov5645.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor

s/Mp/MP/ ?

> +
> +maintainers:
> +  - Sakari Ailus <sakari.ailus@linux.intel.com>
> +  - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> +
> +description: |-
> + The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image sensor with
> + an active array size of 2592H x 1944V. It is programmable through a serial I2C
> + interface.
> +
> +properties:
> +  compatible:
> +    const: ovti,ov5645
> +
> +  reg:
> +    description: I2C device address
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: xclk
> +
> +  assigned-clocks:
> +    maxItems: 1
> +
> +  assigned-clock-rates:
> +     items:
> +     - description: Must be 24MHz (24000000).

These two properties shouldn't be part of the bindings, they're generic.

> +
> +  enable-gpios:
> +    description: |-
> +      Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH. This corresponds
> +      to the hardware pin PWDNB which is physically active low.

Specifying that the polarity is GPIO_ACTIVE_HIGH is confusing in my
opinion. If there's an inverter on the board, you'll need
GPIO_ACTIVE_LOW. We could possibly drop the sentence, as all GPIOs in DT
are supposed to be active high, but the fact that the GPIO name
corresponds to the opposite of the pin probably has to be documented. I
have no better wording to propose now I'm afraid, but it needs to be
addressed. Maybe Rob or Maxime could help.

> +
> +  reset-gpios:
> +    description: |-
> +      Chip reset GPIO. Polarity is GPIO_ACTIVE_LOW. This corresponds to
> +      the hardware pin RESETB.

Here you could just drop the second sentence, or apply the same fix as
for enable-gpios.

> +
> +  vdddo-supply:
> +    description:
> +      Chip digital IO regulator.

You can move the description on the same line as the "description:" key.
Same below.

> +
> +  vdda-supply:
> +    description:
> +      Chip analog regulator.
> +
> +  vddd-supply:
> +    description:
> +      Chip digital core regulator.
> +
> +  # See ../video-interfaces.txt for more details
> +  port:
> +    type: object
> +    properties:
> +      endpoint:
> +        type: object
> +
> +        properties:
> +          data-lanes:
> +            description: |-
> +              The sensor supports two-lane operation.
> +              For two-lane operation the property must be set to <1 2>.
> +            items:
> +              - const: 1
> +              - const: 2


What if only one lane is wired, does the sensor support that ?

> +
> +          clock-lanes:
> +            description:
> +              should be set to <0> (clock lane on hardware lane 0).
> +            items:
> +              - const: 0
> +
> +          remote-endpoint: true
> +
> +        required:
> +          - data-lanes
> +          - clock-lanes
> +          - remote-endpoint
> +
> +        additionalProperties: false
> +
> +    additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - assigned-clocks
> +  - assigned-clock-rates

Those two properties should be dropped.

> +  - enable-gpios
> +  - reset-gpios

Are the GPIOs mandatory ? What if the signals are hardwired on the board
?

> +  - vdddo-supply
> +  - vdda-supply
> +  - vddd-supply
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c1 {

s/i2c1/i2c/

> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        ov5645: sensor@3c {
> +            compatible = "ovti,ov5645";
> +            reg = <0x3c>;
> +            clocks = <&ov5645_cl>;
> +            clock-names = "xclk";
> +            assigned-clocks = <&ov5645_cl>;
> +            assigned-clock-rates = <24000000>;
> +            enable-gpios = <&gpio1 6 /* GPIO_ACTIVE_HIGH */>;
> +            reset-gpios = <&gpio5 20 /* GPIO_ACTIVE_LOW */>;
> +            vdddo-supply = <&camera_dovdd_1v8>;
> +            vdda-supply = <&camera_avdd_2v8>;
> +            vddd-supply = <&camera_dvdd_1v2>;
> +
> +            port {
> +                ov5645_0: endpoint {
> +                    remote-endpoint = <&csi1_ep>;
> +                    clock-lanes = <0>;
> +                    data-lanes = <1 2>;
> +                };
> +            };
> +        };
> +    };
> +
> +...

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 5/5] media: dt-bindings: media: i2c: convert ov5645 bindings to json-schema

[Lad, Prabhakar]

Hi Laurent,

Thank you for the review.

On Thu, Mar 19, 2020 at 3:10 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Thu, Mar 19, 2020 at 12:19:23PM +0000, Lad Prabhakar wrote:
> > Convert ov5645 bindings to json-schema.
>
> \o/
>
:)

> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  .../devicetree/bindings/media/i2c/ov5645.txt  |  54 -------
> >  .../devicetree/bindings/media/i2c/ov5645.yaml | 140 ++++++++++++++++++
> >  2 files changed, 140 insertions(+), 54 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > deleted file mode 100644
> > index 1c85c78ec58c..000000000000
> > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > +++ /dev/null
> > @@ -1,54 +0,0 @@
> > -* Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor
> > -
> > -The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image sensor with
> > -an active array size of 2592H x 1944V. It is programmable through a serial I2C
> > -interface.
> > -
> > -Required Properties:
> > -- compatible: Value should be "ovti,ov5645".
> > -- clocks: Reference to the xclk clock.
> > -- clock-names: Should be "xclk".
> > -- enable-gpios: Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH. This corresponds
> > -  to the hardware pin PWDNB which is physically active low.
> > -- reset-gpios: Chip reset GPIO. Polarity is GPIO_ACTIVE_LOW. This corresponds to
> > -  the hardware pin RESETB.
> > -- vdddo-supply: Chip digital IO regulator.
> > -- vdda-supply: Chip analog regulator.
> > -- vddd-supply: Chip digital core regulator.
> > -
> > -The device node must contain one 'port' child node for its digital output
> > -video port, in accordance with the video interface bindings defined in
> > -Documentation/devicetree/bindings/media/video-interfaces.txt.
> > -
> > -Example:
> > -
> > -     &i2c1 {
> > -             ...
> > -
> > -             ov5645: ov5645@3c {
> > -                     compatible = "ovti,ov5645";
> > -                     reg = <0x3c>;
> > -
> > -                     enable-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>;
> > -                     reset-gpios = <&gpio5 20 GPIO_ACTIVE_LOW>;
> > -                     pinctrl-names = "default";
> > -                     pinctrl-0 = <&camera_rear_default>;
> > -
> > -                     clocks = <&clks 200>;
> > -                     clock-names = "xclk";
> > -                     assigned-clocks = <&clks 200>;
> > -                     assigned-clock-rates = <24000000>;
> > -
> > -                     vdddo-supply = <&camera_dovdd_1v8>;
> > -                     vdda-supply = <&camera_avdd_2v8>;
> > -                     vddd-supply = <&camera_dvdd_1v2>;
> > -
> > -                     port {
> > -                             ov5645_ep: endpoint {
> > -                                     clock-lanes = <1>;
> > -                                     data-lanes = <0 2>;
> > -                                     remote-endpoint = <&csi0_ep>;
> > -                             };
> > -                     };
> > -             };
> > -     };
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.yaml b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml
> > new file mode 100644
> > index 000000000000..4bf58ad210c5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml
> > @@ -0,0 +1,140 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/i2c/ov5645.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor
>
> s/Mp/MP/ ?
>
OK
> > +
> > +maintainers:
> > +  - Sakari Ailus <sakari.ailus@linux.intel.com>
> > +  - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > +
> > +description: |-
> > + The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image sensor with
> > + an active array size of 2592H x 1944V. It is programmable through a serial I2C
> > + interface.
> > +
> > +properties:
> > +  compatible:
> > +    const: ovti,ov5645
> > +
> > +  reg:
> > +    description: I2C device address
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    items:
> > +      - const: xclk
> > +
> > +  assigned-clocks:
> > +    maxItems: 1
> > +
> > +  assigned-clock-rates:
> > +     items:
> > +     - description: Must be 24MHz (24000000).
>
> These two properties shouldn't be part of the bindings, they're generic.
>
In that case how do we specify whats the expected clock frequency ?

> > +
> > +  enable-gpios:
> > +    description: |-
> > +      Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH. This corresponds
> > +      to the hardware pin PWDNB which is physically active low.
>
> Specifying that the polarity is GPIO_ACTIVE_HIGH is confusing in my
> opinion. If there's an inverter on the board, you'll need
> GPIO_ACTIVE_LOW. We could possibly drop the sentence, as all GPIOs in DT
> are supposed to be active high, but the fact that the GPIO name
> corresponds to the opposite of the pin probably has to be documented. I
> have no better wording to propose now I'm afraid, but it needs to be
> addressed. Maybe Rob or Maxime could help.
>
Agreed, will wait for either Rob/Maxime to comment.

> > +
> > +  reset-gpios:
> > +    description: |-
> > +      Chip reset GPIO. Polarity is GPIO_ACTIVE_LOW. This corresponds to
> > +      the hardware pin RESETB.
>
> Here you could just drop the second sentence, or apply the same fix as
> for enable-gpios.
>
OK

> > +
> > +  vdddo-supply:
> > +    description:
> > +      Chip digital IO regulator.
>
> You can move the description on the same line as the "description:" key.
> Same below.
>
Will fix that.

> > +
> > +  vdda-supply:
> > +    description:
> > +      Chip analog regulator.
> > +
> > +  vddd-supply:
> > +    description:
> > +      Chip digital core regulator.
> > +
> > +  # See ../video-interfaces.txt for more details
> > +  port:
> > +    type: object
> > +    properties:
> > +      endpoint:
> > +        type: object
> > +
> > +        properties:
> > +          data-lanes:
> > +            description: |-
> > +              The sensor supports two-lane operation.
> > +              For two-lane operation the property must be set to <1 2>.
> > +            items:
> > +              - const: 1
> > +              - const: 2
>
>
> What if only one lane is wired, does the sensor support that ?
>
Comparing with ov5640 datasheet (Assuming its similar to it) the
sensor can support
single/dual lane but looking at the driver it only supports dual lane mode atm
{ OV5645_MIPI_CTRL00, 0x24 },

> > +
> > +          clock-lanes:
> > +            description:
> > +              should be set to <0> (clock lane on hardware lane 0).
> > +            items:
> > +              - const: 0
> > +
> > +          remote-endpoint: true
> > +
> > +        required:
> > +          - data-lanes
> > +          - clock-lanes
> > +          - remote-endpoint
> > +
> > +        additionalProperties: false
> > +
> > +    additionalProperties: false
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - assigned-clocks
> > +  - assigned-clock-rates
>
> Those two properties should be dropped.
>
Will do that.

> > +  - enable-gpios
> > +  - reset-gpios
>
> Are the GPIOs mandatory ? What if the signals are hardwired on the board
> ?
>
Yes as per the driver, which needs to be fixed for making these optional :)

> > +  - vdddo-supply
> > +  - vdda-supply
> > +  - vddd-supply
> > +  - port
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c1 {
>
> s/i2c1/i2c/
>
Will fix that.

Cheers,
--Prabhakar

> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        ov5645: sensor@3c {
> > +            compatible = "ovti,ov5645";
> > +            reg = <0x3c>;
> > +            clocks = <&ov5645_cl>;
> > +            clock-names = "xclk";
> > +            assigned-clocks = <&ov5645_cl>;
> > +            assigned-clock-rates = <24000000>;
> > +            enable-gpios = <&gpio1 6 /* GPIO_ACTIVE_HIGH */>;
> > +            reset-gpios = <&gpio5 20 /* GPIO_ACTIVE_LOW */>;
> > +            vdddo-supply = <&camera_dovdd_1v8>;
> > +            vdda-supply = <&camera_avdd_2v8>;
> > +            vddd-supply = <&camera_dvdd_1v2>;
> > +
> > +            port {
> > +                ov5645_0: endpoint {
> > +                    remote-endpoint = <&csi1_ep>;
> > +                    clock-lanes = <0>;
> > +                    data-lanes = <1 2>;
> > +                };
> > +            };
> > +        };
> > +    };
> > +
> > +...
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v4 5/5] media: dt-bindings: media: i2c: convert ov5645 bindings to json-schema

[Laurent Pinchart]

Hi Prabhakar,

On Thu, Mar 19, 2020 at 03:38:46PM +0000, Lad, Prabhakar wrote:
> On Thu, Mar 19, 2020 at 3:10 PM Laurent Pinchart wrote:
> > On Thu, Mar 19, 2020 at 12:19:23PM +0000, Lad Prabhakar wrote:
> > > Convert ov5645 bindings to json-schema.
> >
> > \o/
>
> :)
> 
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > >  .../devicetree/bindings/media/i2c/ov5645.txt  |  54 -------
> > >  .../devicetree/bindings/media/i2c/ov5645.yaml | 140 ++++++++++++++++++
> > >  2 files changed, 140 insertions(+), 54 deletions(-)
> > >  delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > deleted file mode 100644
> > > index 1c85c78ec58c..000000000000
> > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > +++ /dev/null
> > > @@ -1,54 +0,0 @@
> > > -* Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor
> > > -
> > > -The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image sensor with
> > > -an active array size of 2592H x 1944V. It is programmable through a serial I2C
> > > -interface.
> > > -
> > > -Required Properties:
> > > -- compatible: Value should be "ovti,ov5645".
> > > -- clocks: Reference to the xclk clock.
> > > -- clock-names: Should be "xclk".
> > > -- enable-gpios: Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH. This corresponds
> > > -  to the hardware pin PWDNB which is physically active low.
> > > -- reset-gpios: Chip reset GPIO. Polarity is GPIO_ACTIVE_LOW. This corresponds to
> > > -  the hardware pin RESETB.
> > > -- vdddo-supply: Chip digital IO regulator.
> > > -- vdda-supply: Chip analog regulator.
> > > -- vddd-supply: Chip digital core regulator.
> > > -
> > > -The device node must contain one 'port' child node for its digital output
> > > -video port, in accordance with the video interface bindings defined in
> > > -Documentation/devicetree/bindings/media/video-interfaces.txt.
> > > -
> > > -Example:
> > > -
> > > -     &i2c1 {
> > > -             ...
> > > -
> > > -             ov5645: ov5645@3c {
> > > -                     compatible = "ovti,ov5645";
> > > -                     reg = <0x3c>;
> > > -
> > > -                     enable-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>;
> > > -                     reset-gpios = <&gpio5 20 GPIO_ACTIVE_LOW>;
> > > -                     pinctrl-names = "default";
> > > -                     pinctrl-0 = <&camera_rear_default>;
> > > -
> > > -                     clocks = <&clks 200>;
> > > -                     clock-names = "xclk";
> > > -                     assigned-clocks = <&clks 200>;
> > > -                     assigned-clock-rates = <24000000>;
> > > -
> > > -                     vdddo-supply = <&camera_dovdd_1v8>;
> > > -                     vdda-supply = <&camera_avdd_2v8>;
> > > -                     vddd-supply = <&camera_dvdd_1v2>;
> > > -
> > > -                     port {
> > > -                             ov5645_ep: endpoint {
> > > -                                     clock-lanes = <1>;
> > > -                                     data-lanes = <0 2>;
> > > -                                     remote-endpoint = <&csi0_ep>;
> > > -                             };
> > > -                     };
> > > -             };
> > > -     };
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.yaml b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml
> > > new file mode 100644
> > > index 000000000000..4bf58ad210c5
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml
> > > @@ -0,0 +1,140 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/media/i2c/ov5645.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor
> >
> > s/Mp/MP/ ?
>
> OK
> 
> > > +
> > > +maintainers:
> > > +  - Sakari Ailus <sakari.ailus@linux.intel.com>
> > > +  - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > +
> > > +description: |-
> > > + The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image sensor with
> > > + an active array size of 2592H x 1944V. It is programmable through a serial I2C
> > > + interface.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: ovti,ov5645
> > > +
> > > +  reg:
> > > +    description: I2C device address
> > > +    maxItems: 1
> > > +
> > > +  clocks:
> > > +    maxItems: 1
> > > +
> > > +  clock-names:
> > > +    items:
> > > +      - const: xclk
> > > +
> > > +  assigned-clocks:
> > > +    maxItems: 1
> > > +
> > > +  assigned-clock-rates:
> > > +     items:
> > > +     - description: Must be 24MHz (24000000).
> >
> > These two properties shouldn't be part of the bindings, they're generic.
>
> In that case how do we specify whats the expected clock frequency ?

You could specify the frequency range supported by the sensor (6 MHz to
27 MHz) in the description of the clocks property. The fact that only 24
MHz is supported is a driver issue, and should not be reflected in the
DT bindings.

> > > +
> > > +  enable-gpios:
> > > +    description: |-
> > > +      Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH. This corresponds
> > > +      to the hardware pin PWDNB which is physically active low.
> >
> > Specifying that the polarity is GPIO_ACTIVE_HIGH is confusing in my
> > opinion. If there's an inverter on the board, you'll need
> > GPIO_ACTIVE_LOW. We could possibly drop the sentence, as all GPIOs in DT
> > are supposed to be active high, but the fact that the GPIO name
> > corresponds to the opposite of the pin probably has to be documented. I
> > have no better wording to propose now I'm afraid, but it needs to be
> > addressed. Maybe Rob or Maxime could help.
>
> Agreed, will wait for either Rob/Maxime to comment.
> 
> > > +
> > > +  reset-gpios:
> > > +    description: |-
> > > +      Chip reset GPIO. Polarity is GPIO_ACTIVE_LOW. This corresponds to
> > > +      the hardware pin RESETB.
> >
> > Here you could just drop the second sentence, or apply the same fix as
> > for enable-gpios.
>
> OK
> 
> > > +
> > > +  vdddo-supply:
> > > +    description:
> > > +      Chip digital IO regulator.
> >
> > You can move the description on the same line as the "description:" key.
> > Same below.
>
> Will fix that.
> 
> > > +
> > > +  vdda-supply:
> > > +    description:
> > > +      Chip analog regulator.
> > > +
> > > +  vddd-supply:
> > > +    description:
> > > +      Chip digital core regulator.
> > > +
> > > +  # See ../video-interfaces.txt for more details
> > > +  port:
> > > +    type: object
> > > +    properties:
> > > +      endpoint:
> > > +        type: object
> > > +
> > > +        properties:
> > > +          data-lanes:
> > > +            description: |-
> > > +              The sensor supports two-lane operation.
> > > +              For two-lane operation the property must be set to <1 2>.
> > > +            items:
> > > +              - const: 1
> > > +              - const: 2
> >
> > What if only one lane is wired, does the sensor support that ?
>
> Comparing with ov5640 datasheet (Assuming its similar to it) the
> sensor can support
> single/dual lane but looking at the driver it only supports dual lane mode atm
> { OV5645_MIPI_CTRL00, 0x24 },

That's a driver limitation though, the bindings should support both.

> > > +
> > > +          clock-lanes:
> > > +            description:
> > > +              should be set to <0> (clock lane on hardware lane 0).
> > > +            items:
> > > +              - const: 0
> > > +
> > > +          remote-endpoint: true
> > > +
> > > +        required:
> > > +          - data-lanes
> > > +          - clock-lanes
> > > +          - remote-endpoint
> > > +
> > > +        additionalProperties: false
> > > +
> > > +    additionalProperties: false
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - clocks
> > > +  - clock-names
> > > +  - assigned-clocks
> > > +  - assigned-clock-rates
> >
> > Those two properties should be dropped.
>
> Will do that.
> 
> > > +  - enable-gpios
> > > +  - reset-gpios
> >
> > Are the GPIOs mandatory ? What if the signals are hardwired on the board
> > ?
>
> Yes as per the driver, which needs to be fixed for making these optional :)

We can make them optional in the bindings at a later point, or do so
already.

> > > +  - vdddo-supply
> > > +  - vdda-supply
> > > +  - vddd-supply
> > > +  - port
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    i2c1 {
> >
> > s/i2c1/i2c/
>
> Will fix that.
> 
> > > +        #address-cells = <1>;
> > > +        #size-cells = <0>;
> > > +
> > > +        ov5645: sensor@3c {
> > > +            compatible = "ovti,ov5645";
> > > +            reg = <0x3c>;
> > > +            clocks = <&ov5645_cl>;
> > > +            clock-names = "xclk";
> > > +            assigned-clocks = <&ov5645_cl>;
> > > +            assigned-clock-rates = <24000000>;
> > > +            enable-gpios = <&gpio1 6 /* GPIO_ACTIVE_HIGH */>;
> > > +            reset-gpios = <&gpio5 20 /* GPIO_ACTIVE_LOW */>;
> > > +            vdddo-supply = <&camera_dovdd_1v8>;
> > > +            vdda-supply = <&camera_avdd_2v8>;
> > > +            vddd-supply = <&camera_dvdd_1v2>;
> > > +
> > > +            port {
> > > +                ov5645_0: endpoint {
> > > +                    remote-endpoint = <&csi1_ep>;
> > > +                    clock-lanes = <0>;
> > > +                    data-lanes = <1 2>;
> > > +                };
> > > +            };
> > > +        };
> > > +    };
> > > +
> > > +...

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 5/5] media: dt-bindings: media: i2c: convert ov5645 bindings to json-schema

[Rob Herring]

On Thu, Mar 19, 2020 at 05:10:35PM +0200, Laurent Pinchart wrote:
> Hi Prabhakar,
> 
> Thank you for the patch.
> 
> On Thu, Mar 19, 2020 at 12:19:23PM +0000, Lad Prabhakar wrote:
> > Convert ov5645 bindings to json-schema.
> 
> \o/
> 
> > 
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  .../devicetree/bindings/media/i2c/ov5645.txt  |  54 -------
> >  .../devicetree/bindings/media/i2c/ov5645.yaml | 140 ++++++++++++++++++
> >  2 files changed, 140 insertions(+), 54 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > deleted file mode 100644
> > index 1c85c78ec58c..000000000000
> > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > +++ /dev/null
> > @@ -1,54 +0,0 @@
> > -* Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor
> > -
> > -The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image sensor with
> > -an active array size of 2592H x 1944V. It is programmable through a serial I2C
> > -interface.
> > -
> > -Required Properties:
> > -- compatible: Value should be "ovti,ov5645".
> > -- clocks: Reference to the xclk clock.
> > -- clock-names: Should be "xclk".
> > -- enable-gpios: Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH. This corresponds
> > -  to the hardware pin PWDNB which is physically active low.
> > -- reset-gpios: Chip reset GPIO. Polarity is GPIO_ACTIVE_LOW. This corresponds to
> > -  the hardware pin RESETB.
> > -- vdddo-supply: Chip digital IO regulator.
> > -- vdda-supply: Chip analog regulator.
> > -- vddd-supply: Chip digital core regulator.
> > -
> > -The device node must contain one 'port' child node for its digital output
> > -video port, in accordance with the video interface bindings defined in
> > -Documentation/devicetree/bindings/media/video-interfaces.txt.
> > -
> > -Example:
> > -
> > -	&i2c1 {
> > -		...
> > -
> > -		ov5645: ov5645@3c {
> > -			compatible = "ovti,ov5645";
> > -			reg = <0x3c>;
> > -
> > -			enable-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>;
> > -			reset-gpios = <&gpio5 20 GPIO_ACTIVE_LOW>;
> > -			pinctrl-names = "default";
> > -			pinctrl-0 = <&camera_rear_default>;
> > -
> > -			clocks = <&clks 200>;
> > -			clock-names = "xclk";
> > -			assigned-clocks = <&clks 200>;
> > -			assigned-clock-rates = <24000000>;
> > -
> > -			vdddo-supply = <&camera_dovdd_1v8>;
> > -			vdda-supply = <&camera_avdd_2v8>;
> > -			vddd-supply = <&camera_dvdd_1v2>;
> > -
> > -			port {
> > -				ov5645_ep: endpoint {
> > -					clock-lanes = <1>;
> > -					data-lanes = <0 2>;
> > -					remote-endpoint = <&csi0_ep>;
> > -				};
> > -			};
> > -		};
> > -	};
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.yaml b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml
> > new file mode 100644
> > index 000000000000..4bf58ad210c5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml
> > @@ -0,0 +1,140 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/i2c/ov5645.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor
> 
> s/Mp/MP/ ?
> 
> > +
> > +maintainers:
> > +  - Sakari Ailus <sakari.ailus@linux.intel.com>
> > +  - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > +
> > +description: |-
> > + The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image sensor with
> > + an active array size of 2592H x 1944V. It is programmable through a serial I2C
> > + interface.
> > +
> > +properties:
> > +  compatible:
> > +    const: ovti,ov5645
> > +
> > +  reg:
> > +    description: I2C device address
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    items:
> > +      - const: xclk
> > +
> > +  assigned-clocks:
> > +    maxItems: 1
> > +
> > +  assigned-clock-rates:
> > +     items:
> > +     - description: Must be 24MHz (24000000).
> 
> These two properties shouldn't be part of the bindings, they're generic.

The fact that they have 1 entry is part of this binding.


> > +  enable-gpios:
> > +    description: |-
> > +      Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH. This corresponds
> > +      to the hardware pin PWDNB which is physically active low.
> 
> Specifying that the polarity is GPIO_ACTIVE_HIGH is confusing in my
> opinion. If there's an inverter on the board, you'll need
> GPIO_ACTIVE_LOW. We could possibly drop the sentence, as all GPIOs in DT
> are supposed to be active high, but the fact that the GPIO name
> corresponds to the opposite of the pin probably has to be documented. I
> have no better wording to propose now I'm afraid, but it needs to be
> addressed. Maybe Rob or Maxime could help.

All GPIOs in DT are active high? What? 

Using 'powerdown-gpios' would have made more sense here, but we're stuck 
with it now. In any case, the description was good enough before and I 
don't care to re-review it as part of the conversion.

> > +  reset-gpios:
> > +    description: |-
> > +      Chip reset GPIO. Polarity is GPIO_ACTIVE_LOW. This corresponds to
> > +      the hardware pin RESETB.
> 
> Here you could just drop the second sentence, or apply the same fix as
> for enable-gpios.

A description that's specific to this chip seems good to me.
 
> > +
> > +  vdddo-supply:
> > +    description:
> > +      Chip digital IO regulator.
> 
> You can move the description on the same line as the "description:" key.
> Same below.
> 
> > +
> > +  vdda-supply:
> > +    description:
> > +      Chip analog regulator.
> > +
> > +  vddd-supply:
> > +    description:
> > +      Chip digital core regulator.
> > +
> > +  # See ../video-interfaces.txt for more details
> > +  port:
> > +    type: object
> > +    properties:
> > +      endpoint:
> > +        type: object
> > +
> > +        properties:
> > +          data-lanes:
> > +            description: |-
> > +              The sensor supports two-lane operation.
> > +              For two-lane operation the property must be set to <1 2>.
> > +            items:
> > +              - const: 1
> > +              - const: 2
> 
> 
> What if only one lane is wired, does the sensor support that ?
> 
> > +
> > +          clock-lanes:
> > +            description:
> > +              should be set to <0> (clock lane on hardware lane 0).
> > +            items:
> > +              - const: 0
> > +
> > +          remote-endpoint: true
> > +
> > +        required:
> > +          - data-lanes
> > +          - clock-lanes
> > +          - remote-endpoint
> > +
> > +        additionalProperties: false
> > +
> > +    additionalProperties: false
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - assigned-clocks
> > +  - assigned-clock-rates
> 
> Those two properties should be dropped.
> 
> > +  - enable-gpios
> > +  - reset-gpios
> 
> Are the GPIOs mandatory ? What if the signals are hardwired on the board
> ?
> 
> > +  - vdddo-supply
> > +  - vdda-supply
> > +  - vddd-supply
> > +  - port
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c1 {
> 
> s/i2c1/i2c/
> 
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        ov5645: sensor@3c {
> > +            compatible = "ovti,ov5645";
> > +            reg = <0x3c>;
> > +            clocks = <&ov5645_cl>;
> > +            clock-names = "xclk";
> > +            assigned-clocks = <&ov5645_cl>;
> > +            assigned-clock-rates = <24000000>;
> > +            enable-gpios = <&gpio1 6 /* GPIO_ACTIVE_HIGH */>;
> > +            reset-gpios = <&gpio5 20 /* GPIO_ACTIVE_LOW */>;
> > +            vdddo-supply = <&camera_dovdd_1v8>;
> > +            vdda-supply = <&camera_avdd_2v8>;
> > +            vddd-supply = <&camera_dvdd_1v2>;
> > +
> > +            port {
> > +                ov5645_0: endpoint {
> > +                    remote-endpoint = <&csi1_ep>;
> > +                    clock-lanes = <0>;
> > +                    data-lanes = <1 2>;
> > +                };
> > +            };
> > +        };
> > +    };
> > +
> > +...
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Re: [PATCH v4 5/5] media: dt-bindings: media: i2c: convert ov5645 bindings to json-schema

[Laurent Pinchart]

Hi Rob,

On Fri, Mar 20, 2020 at 04:48:36PM -0600, Rob Herring wrote:
> On Thu, Mar 19, 2020 at 05:10:35PM +0200, Laurent Pinchart wrote:
> > On Thu, Mar 19, 2020 at 12:19:23PM +0000, Lad Prabhakar wrote:
> > > Convert ov5645 bindings to json-schema.
> > 
> > \o/
> > 
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > >  .../devicetree/bindings/media/i2c/ov5645.txt  |  54 -------
> > >  .../devicetree/bindings/media/i2c/ov5645.yaml | 140 ++++++++++++++++++
> > >  2 files changed, 140 insertions(+), 54 deletions(-)
> > >  delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > deleted file mode 100644
> > > index 1c85c78ec58c..000000000000
> > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > +++ /dev/null
> > > @@ -1,54 +0,0 @@
> > > -* Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor
> > > -
> > > -The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image sensor with
> > > -an active array size of 2592H x 1944V. It is programmable through a serial I2C
> > > -interface.
> > > -
> > > -Required Properties:
> > > -- compatible: Value should be "ovti,ov5645".
> > > -- clocks: Reference to the xclk clock.
> > > -- clock-names: Should be "xclk".
> > > -- enable-gpios: Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH. This corresponds
> > > -  to the hardware pin PWDNB which is physically active low.
> > > -- reset-gpios: Chip reset GPIO. Polarity is GPIO_ACTIVE_LOW. This corresponds to
> > > -  the hardware pin RESETB.
> > > -- vdddo-supply: Chip digital IO regulator.
> > > -- vdda-supply: Chip analog regulator.
> > > -- vddd-supply: Chip digital core regulator.
> > > -
> > > -The device node must contain one 'port' child node for its digital output
> > > -video port, in accordance with the video interface bindings defined in
> > > -Documentation/devicetree/bindings/media/video-interfaces.txt.
> > > -
> > > -Example:
> > > -
> > > -	&i2c1 {
> > > -		...
> > > -
> > > -		ov5645: ov5645@3c {
> > > -			compatible = "ovti,ov5645";
> > > -			reg = <0x3c>;
> > > -
> > > -			enable-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>;
> > > -			reset-gpios = <&gpio5 20 GPIO_ACTIVE_LOW>;
> > > -			pinctrl-names = "default";
> > > -			pinctrl-0 = <&camera_rear_default>;
> > > -
> > > -			clocks = <&clks 200>;
> > > -			clock-names = "xclk";
> > > -			assigned-clocks = <&clks 200>;
> > > -			assigned-clock-rates = <24000000>;
> > > -
> > > -			vdddo-supply = <&camera_dovdd_1v8>;
> > > -			vdda-supply = <&camera_avdd_2v8>;
> > > -			vddd-supply = <&camera_dvdd_1v2>;
> > > -
> > > -			port {
> > > -				ov5645_ep: endpoint {
> > > -					clock-lanes = <1>;
> > > -					data-lanes = <0 2>;
> > > -					remote-endpoint = <&csi0_ep>;
> > > -				};
> > > -			};
> > > -		};
> > > -	};
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.yaml b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml
> > > new file mode 100644
> > > index 000000000000..4bf58ad210c5
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml
> > > @@ -0,0 +1,140 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/media/i2c/ov5645.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor
> > 
> > s/Mp/MP/ ?
> > 
> > > +
> > > +maintainers:
> > > +  - Sakari Ailus <sakari.ailus@linux.intel.com>
> > > +  - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > +
> > > +description: |-
> > > + The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image sensor with
> > > + an active array size of 2592H x 1944V. It is programmable through a serial I2C
> > > + interface.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: ovti,ov5645
> > > +
> > > +  reg:
> > > +    description: I2C device address
> > > +    maxItems: 1
> > > +
> > > +  clocks:
> > > +    maxItems: 1
> > > +
> > > +  clock-names:
> > > +    items:
> > > +      - const: xclk
> > > +
> > > +  assigned-clocks:
> > > +    maxItems: 1
> > > +
> > > +  assigned-clock-rates:
> > > +     items:
> > > +     - description: Must be 24MHz (24000000).
> > 
> > These two properties shouldn't be part of the bindings, they're generic.
> 
> The fact that they have 1 entry is part of this binding.

I'm not sure to agree. assigned-clocks and assigned-clock-rates are very
losely defined, and could be placed (at least in theory) in any node.
Furthermore, in order to cotnrol the rate of xclk, we may need
assigned-clocks and assigned-clock-rates entries for parents of the xclk
clock too. There's really nothing that's specific to this device.

Anway, I think the driver should just set the clock rate, so we can drop
these two properties and skip arguing over them :-)

> > > +  enable-gpios:
> > > +    description: |-
> > > +      Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH. This corresponds
> > > +      to the hardware pin PWDNB which is physically active low.
> > 
> > Specifying that the polarity is GPIO_ACTIVE_HIGH is confusing in my
> > opinion. If there's an inverter on the board, you'll need
> > GPIO_ACTIVE_LOW. We could possibly drop the sentence, as all GPIOs in DT
> > are supposed to be active high, but the fact that the GPIO name
> > corresponds to the opposite of the pin probably has to be documented. I
> > have no better wording to propose now I'm afraid, but it needs to be
> > addressed. Maybe Rob or Maxime could help.
> 
> All GPIOs in DT are active high? What? 

Re-reading my comment, I've expressed myself very badly here. The point
is that the GPIO "object" (enable-gpios), conceptually, hides the
polarity. What's exposed to the OS is something that can be asserted or
deasserted to have an effect indicated by the GPIO name. The polarity
then describes what electrical level of the physical GPIO pin
corresponds to that action. In this case, asserting the GPIO performs
the "enable" action. As this is connected to the PWDNB pin, we have to
specify a polarity (active high) that is inverted compared to the PWDNB
pin polarity (active low). This should be captured in the bindings, but
syaing that "the polarity is GPIO_ACTIVE_HIGH" is wrong. The polarity
depends on how the signal is routed on the board. I'm not sure how to
express all these neatly in the bindings. We may not want to address it
as part of the conversion to YAML, but I think template sentences for
GPIO descriptions would be useful as guidelines for binding authors.

> Using 'powerdown-gpios' would have made more sense here, but we're stuck 
> with it now. In any case, the description was good enough before and I 
> don't care to re-review it as part of the conversion.
> 
> > > +  reset-gpios:
> > > +    description: |-
> > > +      Chip reset GPIO. Polarity is GPIO_ACTIVE_LOW. This corresponds to
> > > +      the hardware pin RESETB.
> > 
> > Here you could just drop the second sentence, or apply the same fix as
> > for enable-gpios.
> 
> A description that's specific to this chip seems good to me.
>  
> > > +
> > > +  vdddo-supply:
> > > +    description:
> > > +      Chip digital IO regulator.
> > 
> > You can move the description on the same line as the "description:" key.
> > Same below.
> > 
> > > +
> > > +  vdda-supply:
> > > +    description:
> > > +      Chip analog regulator.
> > > +
> > > +  vddd-supply:
> > > +    description:
> > > +      Chip digital core regulator.
> > > +
> > > +  # See ../video-interfaces.txt for more details
> > > +  port:
> > > +    type: object
> > > +    properties:
> > > +      endpoint:
> > > +        type: object
> > > +
> > > +        properties:
> > > +          data-lanes:
> > > +            description: |-
> > > +              The sensor supports two-lane operation.
> > > +              For two-lane operation the property must be set to <1 2>.
> > > +            items:
> > > +              - const: 1
> > > +              - const: 2
> > 
> > 
> > What if only one lane is wired, does the sensor support that ?
> > 
> > > +
> > > +          clock-lanes:
> > > +            description:
> > > +              should be set to <0> (clock lane on hardware lane 0).
> > > +            items:
> > > +              - const: 0
> > > +
> > > +          remote-endpoint: true
> > > +
> > > +        required:
> > > +          - data-lanes
> > > +          - clock-lanes
> > > +          - remote-endpoint
> > > +
> > > +        additionalProperties: false
> > > +
> > > +    additionalProperties: false
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - clocks
> > > +  - clock-names
> > > +  - assigned-clocks
> > > +  - assigned-clock-rates
> > 
> > Those two properties should be dropped.
> > 
> > > +  - enable-gpios
> > > +  - reset-gpios
> > 
> > Are the GPIOs mandatory ? What if the signals are hardwired on the board
> > ?
> > 
> > > +  - vdddo-supply
> > > +  - vdda-supply
> > > +  - vddd-supply
> > > +  - port
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    i2c1 {
> > 
> > s/i2c1/i2c/
> > 
> > > +        #address-cells = <1>;
> > > +        #size-cells = <0>;
> > > +
> > > +        ov5645: sensor@3c {
> > > +            compatible = "ovti,ov5645";
> > > +            reg = <0x3c>;
> > > +            clocks = <&ov5645_cl>;
> > > +            clock-names = "xclk";
> > > +            assigned-clocks = <&ov5645_cl>;
> > > +            assigned-clock-rates = <24000000>;
> > > +            enable-gpios = <&gpio1 6 /* GPIO_ACTIVE_HIGH */>;
> > > +            reset-gpios = <&gpio5 20 /* GPIO_ACTIVE_LOW */>;
> > > +            vdddo-supply = <&camera_dovdd_1v8>;
> > > +            vdda-supply = <&camera_avdd_2v8>;
> > > +            vddd-supply = <&camera_dvdd_1v2>;
> > > +
> > > +            port {
> > > +                ov5645_0: endpoint {
> > > +                    remote-endpoint = <&csi1_ep>;
> > > +                    clock-lanes = <0>;
> > > +                    data-lanes = <1 2>;
> > > +                };
> > > +            };
> > > +        };
> > > +    };
> > > +
> > > +...

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 5/5] media: dt-bindings: media: i2c: convert ov5645 bindings to json-schema

[Rob Herring]

On Fri, Mar 20, 2020 at 5:02 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Rob,
>
> On Fri, Mar 20, 2020 at 04:48:36PM -0600, Rob Herring wrote:
> > On Thu, Mar 19, 2020 at 05:10:35PM +0200, Laurent Pinchart wrote:
> > > On Thu, Mar 19, 2020 at 12:19:23PM +0000, Lad Prabhakar wrote:
> > > > Convert ov5645 bindings to json-schema.
> > >
> > > \o/
> > >
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > ---
> > > >  .../devicetree/bindings/media/i2c/ov5645.txt  |  54 -------
> > > >  .../devicetree/bindings/media/i2c/ov5645.yaml | 140 ++++++++++++++++++
> > > >  2 files changed, 140 insertions(+), 54 deletions(-)
> > > >  delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > deleted file mode 100644
> > > > index 1c85c78ec58c..000000000000
> > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > +++ /dev/null
> > > > @@ -1,54 +0,0 @@
> > > > -* Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor
> > > > -
> > > > -The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image sensor with
> > > > -an active array size of 2592H x 1944V. It is programmable through a serial I2C
> > > > -interface.
> > > > -
> > > > -Required Properties:
> > > > -- compatible: Value should be "ovti,ov5645".
> > > > -- clocks: Reference to the xclk clock.
> > > > -- clock-names: Should be "xclk".
> > > > -- enable-gpios: Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH. This corresponds
> > > > -  to the hardware pin PWDNB which is physically active low.
> > > > -- reset-gpios: Chip reset GPIO. Polarity is GPIO_ACTIVE_LOW. This corresponds to
> > > > -  the hardware pin RESETB.
> > > > -- vdddo-supply: Chip digital IO regulator.
> > > > -- vdda-supply: Chip analog regulator.
> > > > -- vddd-supply: Chip digital core regulator.
> > > > -
> > > > -The device node must contain one 'port' child node for its digital output
> > > > -video port, in accordance with the video interface bindings defined in
> > > > -Documentation/devicetree/bindings/media/video-interfaces.txt.
> > > > -
> > > > -Example:
> > > > -
> > > > - &i2c1 {
> > > > -         ...
> > > > -
> > > > -         ov5645: ov5645@3c {
> > > > -                 compatible = "ovti,ov5645";
> > > > -                 reg = <0x3c>;
> > > > -
> > > > -                 enable-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>;
> > > > -                 reset-gpios = <&gpio5 20 GPIO_ACTIVE_LOW>;
> > > > -                 pinctrl-names = "default";
> > > > -                 pinctrl-0 = <&camera_rear_default>;
> > > > -
> > > > -                 clocks = <&clks 200>;
> > > > -                 clock-names = "xclk";
> > > > -                 assigned-clocks = <&clks 200>;
> > > > -                 assigned-clock-rates = <24000000>;
> > > > -
> > > > -                 vdddo-supply = <&camera_dovdd_1v8>;
> > > > -                 vdda-supply = <&camera_avdd_2v8>;
> > > > -                 vddd-supply = <&camera_dvdd_1v2>;
> > > > -
> > > > -                 port {
> > > > -                         ov5645_ep: endpoint {
> > > > -                                 clock-lanes = <1>;
> > > > -                                 data-lanes = <0 2>;
> > > > -                                 remote-endpoint = <&csi0_ep>;
> > > > -                         };
> > > > -                 };
> > > > -         };
> > > > - };
> > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.yaml b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml
> > > > new file mode 100644
> > > > index 000000000000..4bf58ad210c5
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml
> > > > @@ -0,0 +1,140 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/media/i2c/ov5645.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor
> > >
> > > s/Mp/MP/ ?
> > >
> > > > +
> > > > +maintainers:
> > > > +  - Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > +  - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > +
> > > > +description: |-
> > > > + The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image sensor with
> > > > + an active array size of 2592H x 1944V. It is programmable through a serial I2C
> > > > + interface.
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: ovti,ov5645
> > > > +
> > > > +  reg:
> > > > +    description: I2C device address
> > > > +    maxItems: 1
> > > > +
> > > > +  clocks:
> > > > +    maxItems: 1
> > > > +
> > > > +  clock-names:
> > > > +    items:
> > > > +      - const: xclk
> > > > +
> > > > +  assigned-clocks:
> > > > +    maxItems: 1
> > > > +
> > > > +  assigned-clock-rates:
> > > > +     items:
> > > > +     - description: Must be 24MHz (24000000).
> > >
> > > These two properties shouldn't be part of the bindings, they're generic.
> >
> > The fact that they have 1 entry is part of this binding.
>
> I'm not sure to agree. assigned-clocks and assigned-clock-rates are very
> losely defined, and could be placed (at least in theory) in any node.
> Furthermore, in order to cotnrol the rate of xclk, we may need
> assigned-clocks and assigned-clock-rates entries for parents of the xclk
> clock too. There's really nothing that's specific to this device.
>
> Anway, I think the driver should just set the clock rate, so we can drop
> these two properties and skip arguing over them :-)

Works for me. :)

>
> > > > +  enable-gpios:
> > > > +    description: |-
> > > > +      Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH. This corresponds
> > > > +      to the hardware pin PWDNB which is physically active low.
> > >
> > > Specifying that the polarity is GPIO_ACTIVE_HIGH is confusing in my
> > > opinion. If there's an inverter on the board, you'll need
> > > GPIO_ACTIVE_LOW. We could possibly drop the sentence, as all GPIOs in DT
> > > are supposed to be active high, but the fact that the GPIO name
> > > corresponds to the opposite of the pin probably has to be documented. I
> > > have no better wording to propose now I'm afraid, but it needs to be
> > > addressed. Maybe Rob or Maxime could help.
> >
> > All GPIOs in DT are active high? What?
>
> Re-reading my comment, I've expressed myself very badly here. The point
> is that the GPIO "object" (enable-gpios), conceptually, hides the
> polarity. What's exposed to the OS is something that can be asserted or
> deasserted to have an effect indicated by the GPIO name. The polarity
> then describes what electrical level of the physical GPIO pin
> corresponds to that action. In this case, asserting the GPIO performs
> the "enable" action. As this is connected to the PWDNB pin, we have to
> specify a polarity (active high) that is inverted compared to the PWDNB
> pin polarity (active low). This should be captured in the bindings, but
> syaing that "the polarity is GPIO_ACTIVE_HIGH" is wrong. The polarity
> depends on how the signal is routed on the board. I'm not sure how to
> express all these neatly in the bindings. We may not want to address it
> as part of the conversion to YAML, but I think template sentences for
> GPIO descriptions would be useful as guidelines for binding authors.

Yeah, this has come up a couple of times. I think it's important to
state what the polarity of the signal on the device is, but it is
confusing when there's an inverter in the middle.

Rob