[PATCH v2 1/2] dt-bindings: trivial-devices: Remove the OV5642 entry

[PATCH v2 1/2] dt-bindings: trivial-devices: Remove the OV5642 entry

[Fabio Estevam]

As explained in the description text:

"This is a list of trivial I2C and SPI devices that have simple device tree
bindings, consisting only of a compatible field, an address and possibly an
interrupt line."

A camera device does not fall into this category as it needs other
properties such as regulators, reset and powerdown GPIOs, clocks,
media endpoint.

Remove the OV5642 entry.

Signed-off-by: Fabio Estevam <festevam@denx.de>
---
Changes since v1:
- None.

 Documentation/devicetree/bindings/trivial-devices.yaml | 2 --
 1 file changed, 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index 40bc475ee7e1..ab1423a4aa7f 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -313,8 +313,6 @@ properties:
           - nuvoton,w83773g
             # OKI ML86V7667 video decoder
           - oki,ml86v7667
-            # OV5642: Color CMOS QSXGA (5-megapixel) Image Sensor with OmniBSI and Embedded TrueFocus
-          - ovti,ov5642
             # 48-Lane, 12-Port PCI Express Gen 2 (5.0 GT/s) Switch
           - plx,pex8648
             # Pulsedlight LIDAR range-finding sensor
-- 
2.34.1

[PATCH v2 2/2] dt-bindings: media: Add OV5642

[Fabio Estevam]

Add DT bindings for OmniVision OV5642 Image Sensor.

Signed-off-by: Fabio Estevam <festevam@denx.de>
---
Changes since v1:
- Introduce ov5642.yaml (Conor).

 .../bindings/media/i2c/ovti,ov5642.yaml       | 118 ++++++++++++++++++
 1 file changed, 118 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5642.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5642.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5642.yaml
new file mode 100644
index 000000000000..585b4fcf01b3
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5642.yaml
@@ -0,0 +1,118 @@
+# SPDX-License-Identifier: GPL-2.0-only
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/ovti,ov5642.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: OmniVision OV5642 Image Sensor
+
+maintainers:
+  - Fabio Estevam <festevam@gmail.com>
+
+allOf:
+  - $ref: /schemas/media/video-interface-devices.yaml#
+
+properties:
+  compatible:
+    const: ovti,ov5642
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    description: XCLK Input Clock
+
+  clock-names:
+    const: xclk
+
+  AVDD-supply:
+    description: Analog voltage supply, 2.8V.
+
+  DVDD-supply:
+    description: Digital core voltage supply, 1.5V.
+
+  DOVDD-supply:
+    description: Digital I/O voltage supply, 1.8V.
+
+  powerdown-gpios:
+    maxItems: 1
+    description: Reference to the GPIO connected to the powerdown pin, if any.
+
+  reset-gpios:
+    maxItems: 1
+    description: Reference to the GPIO connected to the reset pin, if any.
+
+  rotation:
+    enum:
+      - 0
+      - 180
+
+  port:
+    description: Digital Output Port
+    $ref: /schemas/graph.yaml#/$defs/port-base
+    additionalProperties: false
+
+    properties:
+      endpoint:
+        $ref: /schemas/media/video-interfaces.yaml#
+        unevaluatedProperties: false
+
+        properties:
+          clock-lanes:
+            const: 0
+
+          data-lanes:
+            minItems: 1
+            maxItems: 2
+            items:
+              enum: [1, 2]
+
+          bus-width:
+            enum: [8, 10]
+
+          data-shift:
+            enum: [0, 2]
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+      #include <dt-bindings/gpio/gpio.h>
+
+      i2c {
+          #address-cells = <1>;
+          #size-cells = <0>;
+
+          camera@3c {
+              compatible = "ovti,ov5642";
+              pinctrl-names = "default";
+              pinctrl-0 = <&pinctrl_ov5642>;
+              reg = <0x3c>;
+              clocks = <&clk_ext_camera>;
+              clock-names = "xclk";
+              DOVDD-supply = <&vgen4_reg>;
+              AVDD-supply = <&vgen3_reg>;
+              DVDD-supply = <&vgen2_reg>;
+              powerdown-gpios = <&gpio1 19 GPIO_ACTIVE_HIGH>;
+              reset-gpios = <&gpio1 20 GPIO_ACTIVE_LOW>;
+  
+              port {
+                  /* Parallel bus endpoint */
+                  ov5642_to_parallel: endpoint {
+                      remote-endpoint = <&parallel_from_ov5642>;
+                      bus-width = <8>;
+                      data-shift = <2>; /* lines 9:2 are used */
+                      hsync-active = <0>;
+                      vsync-active = <0>;
+                      pclk-sample = <1>;
+                  };
+              };
+          };
+      };
-- 
2.34.1

Re: [PATCH v2 2/2] dt-bindings: media: Add OV5642

[Conor Dooley]

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

Hey, 

On Tue, Aug 01, 2023 at 08:40:47PM -0300, Fabio Estevam wrote:
> Add DT bindings for OmniVision OV5642 Image Sensor.
> 
> Signed-off-by: Fabio Estevam <festevam@denx.de>

Thanks for doing this. It looks good to me, although I got a complaint
from git while applying it locally:
Applying: dt-bindings: trivial-devices: Remove the OV5642 entry
Applying: dt-bindings: media: Add OV5642
/stuff/linux/.git/worktrees/linux-dt/rebase-apply/patch:119: trailing whitespace.
  
warning: 1 line adds whitespace errors.

I think you can probably squash both patches and add a
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
I have one minor comment below.

I also think it'd be good to CC the media folks though on this.

> ---
> Changes since v1:
> - Introduce ov5642.yaml (Conor).
> 
>  .../bindings/media/i2c/ovti,ov5642.yaml       | 118 ++++++++++++++++++
>  1 file changed, 118 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5642.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5642.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5642.yaml
> new file mode 100644
> index 000000000000..585b4fcf01b3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5642.yaml
> @@ -0,0 +1,118 @@
> +# SPDX-License-Identifier: GPL-2.0-only

Bindings are usually dual licensed. Is there a reason not to do so here?

Thanks,
Conor.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/ovti,ov5642.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: OmniVision OV5642 Image Sensor
> +
> +maintainers:
> +  - Fabio Estevam <festevam@gmail.com>
> +
> +allOf:
> +  - $ref: /schemas/media/video-interface-devices.yaml#
> +
> +properties:
> +  compatible:
> +    const: ovti,ov5642
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    description: XCLK Input Clock
> +
> +  clock-names:
> +    const: xclk
> +
> +  AVDD-supply:
> +    description: Analog voltage supply, 2.8V.
> +
> +  DVDD-supply:
> +    description: Digital core voltage supply, 1.5V.
> +
> +  DOVDD-supply:
> +    description: Digital I/O voltage supply, 1.8V.
> +
> +  powerdown-gpios:
> +    maxItems: 1
> +    description: Reference to the GPIO connected to the powerdown pin, if any.
> +
> +  reset-gpios:
> +    maxItems: 1
> +    description: Reference to the GPIO connected to the reset pin, if any.
> +
> +  rotation:
> +    enum:
> +      - 0
> +      - 180
> +
> +  port:
> +    description: Digital Output Port
> +    $ref: /schemas/graph.yaml#/$defs/port-base
> +    additionalProperties: false
> +
> +    properties:
> +      endpoint:
> +        $ref: /schemas/media/video-interfaces.yaml#
> +        unevaluatedProperties: false
> +
> +        properties:
> +          clock-lanes:
> +            const: 0
> +
> +          data-lanes:
> +            minItems: 1
> +            maxItems: 2
> +            items:
> +              enum: [1, 2]
> +
> +          bus-width:
> +            enum: [8, 10]
> +
> +          data-shift:
> +            enum: [0, 2]
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +      #include <dt-bindings/gpio/gpio.h>
> +
> +      i2c {
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +
> +          camera@3c {
> +              compatible = "ovti,ov5642";
> +              pinctrl-names = "default";
> +              pinctrl-0 = <&pinctrl_ov5642>;
> +              reg = <0x3c>;
> +              clocks = <&clk_ext_camera>;
> +              clock-names = "xclk";
> +              DOVDD-supply = <&vgen4_reg>;
> +              AVDD-supply = <&vgen3_reg>;
> +              DVDD-supply = <&vgen2_reg>;
> +              powerdown-gpios = <&gpio1 19 GPIO_ACTIVE_HIGH>;
> +              reset-gpios = <&gpio1 20 GPIO_ACTIVE_LOW>;
> +  
> +              port {
> +                  /* Parallel bus endpoint */
> +                  ov5642_to_parallel: endpoint {
> +                      remote-endpoint = <&parallel_from_ov5642>;
> +                      bus-width = <8>;
> +                      data-shift = <2>; /* lines 9:2 are used */
> +                      hsync-active = <0>;
> +                      vsync-active = <0>;
> +                      pclk-sample = <1>;
> +                  };
> +              };
> +          };
> +      };
> -- 
> 2.34.1
> 

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

Re: [PATCH v2 2/2] dt-bindings: media: Add OV5642

[Krzysztof Kozlowski]

On 02/08/2023 17:25, Conor Dooley wrote:
> Hey, 
> 
> On Tue, Aug 01, 2023 at 08:40:47PM -0300, Fabio Estevam wrote:
>> Add DT bindings for OmniVision OV5642 Image Sensor.
>>
>> Signed-off-by: Fabio Estevam <festevam@denx.de>
> 
> Thanks for doing this. It looks good to me, although I got a complaint
> from git while applying it locally:
> Applying: dt-bindings: trivial-devices: Remove the OV5642 entry
> Applying: dt-bindings: media: Add OV5642
> /stuff/linux/.git/worktrees/linux-dt/rebase-apply/patch:119: trailing whitespace.
>   
> warning: 1 line adds whitespace errors.
> 
> I think you can probably squash both patches and add a

Yes, they should be squashed, otherwise it is not really bisectable.
Compatible disappears after first patch... and the first patch does not
have any sense on its own. Even if this is not trivial device, removing
it without documentation is not the way.



Best regards,
Krzysztof