dts: bindings: video: Add common video interface binding

[ngphibang]

Currently, there is no common binding for video endpoint interfaces.

Some video driver defines these properties in their own binding, such as #71462.

This binding provides the most common properties needed to configure an endpoint subnode for data exchange with other device. For example, the bus-type property is required by #74353. The data-lanes prorperty is required by #76475, etc.

The endpoint subnodes which use this binding need to remove the "remote-endpoint" phandle property (which is actually not supported in Zephyr) and use the "remote-endpoint-label" property instead as Zephyr devicetree currently does not support circular dependency and will cause compiling errors

[josuah]

It looks like matching what was already imported for STM32 DMCI here:
https://github.com/zephyrproject-rtos/zephyr/blob/main/dts/bindings/video/st%2Cstm32-dcmi.yaml

[josuah]

Related to #71462

[ngphibang]

@loicpoulain Could you take a look at this ?

Could someone add an assignee to the PR, please as I don't have right to do it ? @dleach02 @decsny

[josuah]

Here are very minor suggestions, this looks ideal as it is.

Thank you for taking the time of detailing these implicit know-how, I did not know the distinction between endpoint@ (for a same bus) and port@ (for different buses)!

[josuah]

@loicpoulain Could you take a look at this ?

Could someone add an assignee to the PR, please as I don't have right to do it ? @dleach02 @decsny

Now I can... Done !

[decsny]

why have a zephyr, compatible? seems like this file should be included by actual hardware bindings, no?

[josuah]

This would be for a nested node rather than the parent node, so there would still be a compatible= node for vendor,device-name:

&video_device {
    compatible = "vendor,device-name";
    port {
        endpoint@0 {
            compatible = "zephyr,video-interfaces";
            remote-endpoint = <&my_source>;
        };
        endpoint@1 {
            compatible = "zephyr,video-interfaces";
            remote-endpoint-label = "my_sink";
        };
    };
};

But endpoint@0 and endpoint@1 also describe properties of vendor,device-name.

Adding vendor,device-name-endpoint.yaml would then look like this?

compatible: "vendor,device-name-endpoint"

descrption: |
   Description of what "endpoint" means in the context
  of this device (input/output? virtual channel?) 

include: "zephyr,video-interfaces.yaml"

properties:

  something-extra:
    type: int
    description: |
      As soon as the driver introduce a new property, all users would
      have to update their devicetree overlay, so having a dedicated
      compatible node helps with forward compatibility too.
    default: 7

I missed this completely, thank you for raising this!

[ngphibang]

@decsny This binding is for the endpoint grandchild or child of grandchild node of the video device, depending on if we have the ports node or not (it's optional). So, if we include this file, in the bindings, we will need 2 or 3 child-binding properties. The problem is we don't know we need exactly 2 or 3 child-binding (it depends on ports node present or not).

That's why I choose the alternate solution using compabtible

[ngphibang]

@josuah Sorry,I can't understand your point. So, what is the problem here ?

But endpoint@0 and endpoint@1 also describe properties of vendor,device-name

Why the child node has to describe the property of its parent ?

Adding vendor,device-name-endpoint.yaml would then look like this?

Why does we need to add this .yaml ?

[josuah]

It might not be familiar enough with devicetrees bindings... Here was my doubt:

Step 1: a driver uses compatible = "zephyr,video-interfaces";

        endpoint@0 {
            compatible = "zephyr,video-interfaces";
            data-lanes = <1 2>;
        };

Step 2: a driver wants extensions for configuring endpoints:

        endpoint@0 {
            compatible = "vendor,device-name-endpoint";
            data-lanes = <1 2>;
            buffer-size = <1024>;
        };

Then, would vendor,device-name-endpoint.yaml be able to include zephyr,video-interfaces.yaml? There would be 2 compatible: "..." then?

I probably missed something.
Thank you for your patience!

[josuah]

[the] purpose of having a common video-interfaces binding is to standardize the endpoint interface properties to avoid letting sensors, sensor interfaces, ISP, etc. drivers to define their own custom properties

I was curious about possible consequences, and I am glad to see such investigation.

@decsny given the last message of @ngphibang, does it look more reasonable keep a per-sensor endpoint bindings, or use a generic zephyr, bindings like used on displays and rely on child-bindings: to inject custom properties from the parent node?

[ngphibang]

@decsny : I tried to include this binding in a video device .yaml, for example, in ovti,ov5640.yaml., instead of using compatible as your request:

child-binding:
  child-binding:
      include: video-interfaces.yaml

but I got error:

devicetree error: 'compatible' is marked as required in 'properties:' in C:/Users/nxf91161/zephyrproject/zephyr/dts/bindings\video\ovti,ov5640.yaml, but does not appear in <Node /soc/i2c@40c38000/ov5640@3c/port/endpoint

It complains because the ovti,ov5640.yaml requires a compatible property for its endpoint child node (coming from the requirement of base.yaml) so it comes back to the "compatible" way (i.e., even we include, the endpoint node still need to declare a compatible).

And as discussed above, we see no advantages or drawbacks between "include" and "compatible" ways.

Could you give the reason why we should "include" and not use "compatible" ? Given that we have an example in the display that do "compatible" way here.

[decsny]

@ngphibang can you show the DT you have for the ov5460 node and child nodes where you got that error, I don't understand why you got that error, the ov5460 binding should not be being applied to the endpoint node?

[ngphibang]

@decsny : Here it is (nxp_btb44_ov5640.overlay):

&nxp_cam_i2c {
	status = "okay";

	ov5640: ov5640@3c {
		compatible = "ovti,ov5640";
		reg = <0x3c>;
		reset-gpios = <&nxp_cam_connector 9 GPIO_ACTIVE_LOW>;
		powerdown-gpios = <&nxp_cam_connector 17 GPIO_ACTIVE_HIGH>;

		port {
			ov5640_ep_out: endpoint {
				remote-endpoint-label = "mipi_csi2rx_ep_in";
				bus-type = "mipi-csi2-dphy";
				data-lanes = <1 2>;
			};
		};
	};
};

&nxp_mipi_csi {
	status = "okay";

	sensor = <&ov5640>;

	ports {
		port@1 {
			reg = <1>;

			mipi_csi2rx_ep_in: endpoint {
				remote-endpoint-label = "ov5640_ep_out";
				data-lanes = <1 2>;
			};
		};
	};
};

And here is the entire ovti,ov5640.yaml

description: OV5640 CMOS video sensor

compatible: "ovti,ov5640"

include: i2c-device.yaml

properties:
  reset-gpios:
    type: phandle-array
    description: |
      The RESETB pin is asserted to cause a hard reset. The sensor
      receives this as an active-low signal.
  powerdown-gpios:
    type: phandle-array
    description: |
      The PWDN pin is asserted to disable the sensor. The sensor
      receives this as an active-high signal.

child-binding:
  child-binding:
    include: video-interfaces.yaml

Though it does not relate to this PR as you said, it's interesting to know why it requires the child node having a "compatible" properties. I think it comes from "include: i2c-device.yaml" which in turn, includes base.yaml which requires compatible. I also tried to move the "child-binding" block before "include: i2c-device.yaml" but it's the same

[decsny]

it's simply just because zephyr's DT binding schemas are not as powerful as linux, in linux for example you can make a property a node, which is what you would want to do in this case, instead we have to use a compatible.

[decsny]

@josuah I completely understand why you would want to use the string enum from the Zephyr bindings or even switch some of these to boolean types, but I think one of the project's goals is to strive to be compatible with linux (and other existing projects) DT bindings. I guess the pipe dream is that people can reuse their linux DT definition to move to Zephyr without any hiccups in DT, I don't know how realistic that is (especially considering there is a ton of other zephyrisms in our DT that totally wreck it anyways) but it's the current aspiration.

[josuah]

one of the project's goals is to strive to be compatible with Linux

Thank you for this beacon! I tried to guess the goals, and I got mistaken in the priorities.

Now I know better what to follow: Whenever there is an existing Linux binding, try to stay as close as possible to these. For new bindings, there is more freedom to use more Zephyr-like DT APIs.

technically the DT spec says this should be done also

That sounds like being this page, good to note.
https://docs.zephyrproject.org/latest/build/dts/design.html#source-compatibility-with-other-operating-systems

Zephyr’s devicetree tooling is based on a generic layer which is interoperable with other devicetree users, such as the Linux kernel.

[josuah]

@ngphibang sorry for inviting you towards the wrong direction in these few PRs.
It looks like I was under-estimating the goal of Linux compatibility.
This also answers "how to design the best Zephyr APIs": also strive to be similar (i.e. function naming, mechanisms) with Linux and other systems when possible. :)

[dleach02]

@decsny @ngphibang are you two converging so we can move this PR along?

[ngphibang]

@dleach02 : I am making change according to request from @decsny. Will push soon.

[josuah]

I am fine with either way w.r.t. inclusion of compatible: in the dts as there seems to be possible to use child-bindings: to add the rare custom link properties from the parent.

[decsny]

I looked into this a bit and I see the limitation of using child-binding here, and maybe why the other zephyr bindings have compatibles like this, the issue appears to be that the zephyr DT binding syntax doesnt allow declaring nodes as properties like in linux, maybe this is something that should be added to zephyr DT binding yaml language, but for now the PR is fine

[decsny]

sorry, I just realized something that might have caused your error