Re: [PATCH] dt-bindings: media: st,stmipid02: Convert the text bindings to YAML

On 9/27/22 13:06, Benjamin MUGNIER wrote:

Hi Marek,

Hi,

Thank you for your patch.

Thank you for the review.

[...]

quoted

+++ b/Documentation/devicetree/bindings/media/i2c/st,st-mipid02.yaml

@@ -0,0 +1,175 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/st,st-mipid02.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: STMicroelectronics MIPID02 CSI-2 to PARALLEL bridge
+
+maintainers:
+  - Mickael Guene <mickael.guene@foss.st.com>

As Mickael is not the maintainer of the driver anymore, there should be both current maintainers instead, as listed in the MAINTAINER file:
  - Benjamin Mugnier [off-list ref]
  - Sylvain Petinot [off-list ref]
Can you change it while at it ? Thank you.

Done and done, I also dropped myself from the list, which I hope is OK.

By the way you will also need to update the MAINTAINERS file to change the st,st-mipid02.txt file to st,st-mipid02.yaml here:
   F:      Documentation/devicetree/bindings/media/i2c/st,st-mipid02.txt

[...]

quoted

+      port@2:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
+        description: Output port
+        properties:
+          endpoint:
+            $ref: /schemas/media/video-interfaces.yaml#
+            unevaluatedProperties: false
+
+            properties:
+              bus-width:
+                enum: [8, 10, 12, 14]

Shouldn't this be [6, 8, 10, 12] as in the original file ?

They are both wrong, per:

https://www.st.com/resource/en/datasheet/stmipid02.pdf
page 5
Table 1. Image format vs. number of bits on output interface
row 2
RAW7 RAW7 7 bits

So this should be
[6, 7, 8, 10, 12]

quoted

+                default: 8

As far as I understand there is no 'default' as this property is required anyway.

Dropped.

quoted

+              hsync-active: true
+              vsync-active: true
+
+            required:
+              - bus-width
+
+    required:
+      - port@0

Actually it could either be port0 or port1, but not both at the same time, see the 'description' part.
I'm not sure how to describe it in device tree binding though.

I think the requirement here is to have at least one of port@0 or port@1 
described in DT. Both port@0 and port@1 can also be described, i.e. you 
can have FRONT and BACK sensor like on a contemporary smartphone, you 
just cannot start streaming from both sensors at the same time, which is 
a software policy and should be handled by the driver.

See:
https://www.st.com/resource/en/datasheet/stmipid02.pdf
page 3
2 Application diagrams
Figure 2. MIPI CSI-2 application diagram

Per suggestion from Rob, I think this should be

allOf:
   - require:
       - port@0
   - require:
       - port@1
require:
   - port@2

quoted

+      - port@2
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg

I noticed this was missing in the original txt file. Thanks for taking care of it.

Sure. I'm glad to see dtbs_check passing on the stm32mp15xx boards, 
largely thanks to ST doing good job keeping the upstream support in good 
shape.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel