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

Hi Marek,

On 9/27/22 20:45, Marek Vasut wrote:

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

quoted

Hi Marek,

Hi,

quoted

Thank you for your patch.

Thank you for the review.

[...]

quoted

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.

No problem for dropping yourself, out of curiosity why so ?

quoted

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

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]

Good catch. Thank you.

quoted

quoted

+                default: 8

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

Dropped.

quoted

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.

Definitely, the device tree should describe connected ports. This is the software's responsibility to not stream from both.
Rob's suggestion is perfect.


Regards,

Benjamin

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

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