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]

Looking at this incomplete list:

• clock-noncontinuous: sony,imx219 - ovti,ov5670
• input-clock-frequency: aptina,mt9p031
• pixel-clock-frequency: aptina,mt9p031
• ovti,mipi-clock-voltage: ovti,ov02a10

Very few! And only one of them has a vendor prefix.

ovti,ov02a10 could also add the ovti,mipi-clock-voltage custom properties like this:

child-bindings:                # port { };
  child-bindings:              # port { endpoint { }; };
    properties:
      ovti,mipi-clock-voltage: # port { endpoint { ovti,mipi-clock-voltage = <>; }; };
        default: 4

For information, here is the list of non-vendor nodes defining child-bindings:

$ grep -rl child-binding dts/bindings/ | grep -v ,

dts/bindings/dsa/microchip_dsa.yaml
dts/bindings/mtd/fixed-partitions.yaml
dts/bindings/led/gpio-leds.yaml
dts/bindings/led/led-controller.yaml
dts/bindings/led/pwm-leds.yaml
dts/bindings/can/can-controller.yaml
dts/bindings/input/analog-axis.yaml
dts/bindings/input/adc-keys.yaml
dts/bindings/input/gpio-keys.yaml
dts/bindings/adc/adc-controller.yaml
dts/bindings/gpio/gpio-controller.yaml

[josuah]

Now that you mention it it... is it impossible to have multiple compatibles? That would completely solve the problem: use the standard one as well as the extension?

zephyr/boards/infineon/cyw920829m2evk_02/cyw920829m2evk_02.dts

Line 104 in f61b8f9

compatible = "zephyr,memory-region", "soc-nv-flash";

zephyr/dts/bindings/base/zephyr,memory-region.yaml

Lines 4 to 12 in f61b8f9

	description: Compatible for devices resulting in linker memory regions
	compatible: "zephyr,memory-region"
	include: [base.yaml, "zephyr,memory-common.yaml"]
	properties:
	zephyr,memory-region:
	required: true

zephyr/dts/bindings/mtd/soc-nv-flash.yaml

Lines 1 to 10 in f61b8f9

	description: Flash node
	compatible: "soc-nv-flash"
	include: base.yaml
	properties:
	erase-block-size:
	type: int
	description: address alignment required by flash erase operations

[ngphibang]

Thanks for the search ! Well, from your above examples:

clock-noncontinuous: sony,imx219 - ovti,ov5670

=> clock-noncontinuous is a not a custom property. It's defined here in video-interfaces.yaml.

input-clock-frequency: aptina,mt9p031
pixel-clock-frequency: aptina,mt9p031

=> Looking at this sensor driver:

• input-clock-frequency is the clock for the sensor. Don't know why they don't use the clocks property instead.
• pixel-clock-frequency is exactly the V4L2_CID_PIXEL_RATE. Normally, this is something provided by the sensor driver itself to its sensor interface (e.g. mipi csi2rx) for frame rate stuffs (see https://elixir.bootlin.com/linux/v6.10-rc1/source/drivers/media/i2c/ov5640.c#L3448, for example). It's not something the sensor needs to read from devicetree.

ovti,mipi-clock-voltage: ovti,ov02a10

=> The only place I see it really needs a new custom property (ovti,mipi-clock-voltage) is in this sensor. But I suppose it can be done differently as well.

So, I still keep my thinking that "the purpose of having a common video-interfaces binding is to standardize the endpoint interface properties to avoid letting sensors, sensor interfaces, ISP, et.c drivers to define their own custom properties." :).

But well, it is safe and does not has any impact if we choose the other way (include the .yaml). I will change to this.

[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]

@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. And technically the DT spec says this should be done also.

[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.