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

On Fri, Mar 20, 2020 at 5:02 PM Laurent Pinchart
[off-list ref] wrote:

Hi Rob,

On Fri, Mar 20, 2020 at 04:48:36PM -0600, Rob Herring wrote:

quoted

On Thu, Mar 19, 2020 at 05:10:35PM +0200, Laurent Pinchart wrote:

quoted

On Thu, Mar 19, 2020 at 12:19:23PM +0000, Lad Prabhakar wrote:

quoted

Convert ov5645 bindings to json-schema.

\o/

quoted

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/ ?

quoted

+
+maintainers:
+  - Sakari Ailus [off-list ref]
+  - Lad Prabhakar [off-list ref]
+
+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. :)

quoted

quoted

quoted

+  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