[RFC,v2,3/6] media: dt-bindings: add DS90UB954-Q1 video deserializer
diff mbox series

Message ID 20190723203723.11730-4-luca@lucaceresoli.net
State New
Headers show
Series
  • TI camera serdes and I2C address translation
Related show

Commit Message

Luca Ceresoli July 23, 2019, 8:37 p.m. UTC
Describe the Texas Instruments DS90UB954-Q1, a 2-input video deserializer
with I2C Address Translator and remote GPIOs.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>

---

Changes RFCv1 -> RFCv2:

 - add explicit aliases for the FPD-link RX ports (optional)
 - add proper remote GPIO description
---
 .../bindings/media/i2c/ti,ds90ub954-q1.txt    | 194 ++++++++++++++++++
 1 file changed, 194 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt

Comments

Rob Herring Aug. 13, 2019, 3:44 p.m. UTC | #1
On Tue, Jul 23, 2019 at 10:37:20PM +0200, Luca Ceresoli wrote:
> Describe the Texas Instruments DS90UB954-Q1, a 2-input video deserializer
> with I2C Address Translator and remote GPIOs.
> 
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> 
> ---
> 
> Changes RFCv1 -> RFCv2:
> 
>  - add explicit aliases for the FPD-link RX ports (optional)
>  - add proper remote GPIO description
> ---
>  .../bindings/media/i2c/ti,ds90ub954-q1.txt    | 194 ++++++++++++++++++
>  1 file changed, 194 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt
> new file mode 100644
> index 000000000000..73ce21ecc3b6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt
> @@ -0,0 +1,194 @@
> +Texas Instruments DS90UB954-Q1 dual video deserializer
> +======================================================
> +
> +The TI DS90UB954-Q1 is a MIPI CSI-2 video deserializer that forwards video
> +streams from up to two FPD-Link 3 connections to a MIPI CSI-2 output. It
> +also allows access to remote I2C and GPIO.
> +
> +Required properties:
> +
> + - compatible: must be "ti,ds90ub954-q1"
> +
> + - reg: main I2C slave address; optionally aliases for RX port registers
> +   and remote serializers. The main address is mandatory and must be the
> +   first, others are optional and fall back to defaults if not
> +   specified. See "reg-names".
> +
> + - reset-gpios: chip reset GPIO, active low (connected to PDB pin of the chip)
> + - i2c-alias-pool: list of I2C addresses that are known to be available on the
> +                   "local" (SoC-to-deser) I2C bus; they will be picked at
> +		   runtime and used as aliases to reach remove I2C chips

s/remove/remote/

Needs a vendor prefix.

> + - gpio-controller
> + - #gpio-cells: must be 3: FPD-Link 3 RX port number, remote gpio number, flags

We're pretty standardized on 2 cells for GPIO. Perhaps combine the port 
and gpio number to 1 cell.

> +
> +Optional properties:
> +
> + - reg-names: names of I2C address used to communicate with the chip, must
> +              match the "reg" values; mandatory if there are 2 or more
> +              addresses
> +    - "main": the main I2C address, used to access shared registers
> +    - "rxport0", "rxport1": I2C alias to access FPD-link RX port specific
> +      registers; must not be used by other slaves on the same bus
> +    - "ser0", "ser1": I2C alias to access the remote serializer connected
> +      on each FPD-link RX port; must not be used by other slaves on the
> +      same bus
> + - interrupts: interrupt pin from the chip
> +
> +Required subnodes:
> +
> + - ports: A ports node with one port child node per device input and output
> +          port, in accordance with the video interface bindings defined in
> +          Documentation/devicetree/bindings/media/video-interfaces.txt. The
> +          port nodes are numbered as follows:
> +
> +          Port Description
> +          ------------------------------------
> +          0    Input from FPD-Link 3 RX port 0
> +          1    Input from FPD-Link 3 RX port 1
> +          2    CSI-2 output
> +
> +          Each port must have a "remote-chip" subnode that defines the remote
> +	  chip (serializer) with at least a "compatible" property

We don't allow other nodes within graph nodes. I'm not really clear what 
you are trying to do here.

> +
> + - i2c-atr: contains one child per RX port, each describes the I2C bus on
> +            the remote side
> +
> +	    Required properties:
> +	    - #address-cells = <1>;
> +	    - #size-cells = <0>;
> +
> +	    Subnodes: one per each FPD-link RX port, each having:
> +
> +	    Required properties for "i2c-atr" child bus nodes:
> +	    - reg: The number of the port where the remove chip is connected

s/remove/remote/

> +	    - #address-cells = <1>;
> +	    - #size-cells = <0>;
> +
> +	    Optional properties for "i2c-atr" child bus nodes:
> +	    - Other properties specific to the remote hardware

Such as?

> +	    - Child nodes conforming to i2c bus binding
> +
> +
> +Device node example
> +-------------------
> +
> +&i2c0 {
> +	deser: deser@3d {
> +		compatible = "ti,ds90ub954-q1";
> +		reg-names = "main", "rxport0", "rxport1", "ser0", "ser1";
> +		reg       = <0x3d>,  <0x40>,    <0x41>,   <0x44>, <0x45>;
> +		clocks = <&clk_25M>;
> +		interrupt-parent = <&gic>;
> +		interrupts = <3 1 IRQ_TYPE_LEVEL_HIGH>;
> +		reset-gpios = <&gpio_ctl 4 GPIO_ACTIVE_LOW>;
> +
> +		i2c-alias-pool = /bits/ 16 <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>;
> +
> +		gpio-controller;
> +		#gpio-cells = <3>; /* rxport, remote gpio num, flags */
> +
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			port@0 {
> +				reg = <0>;
> +				ds90ub954_fpd3_in0: endpoint {
> +					remote-endpoint = <&sensor_0_out>;
> +				};
> +			};
> +
> +			port@1 {
> +				reg = <1>;
> +				ds90ub954_fpd3_in1: endpoint {
> +					remote-endpoint = <&sensor_1_out>;
> +				};
> +			};
> +
> +			port@2 {
> +				reg = <2>;
> +				ds90ub954_mipi_out0: endpoint {
> +					data-lanes = <1 2 3 4>;
> +					/* Actually a REFCLK multiplier */
> +					data-rate = <1600000000>;
> +					remote-endpoint = <&csirx_0_in>;
> +				};
> +			};
> +		};
> +
> +		i2c-atr {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			remote_i2c0: i2c@0 {
> +				reg = <0>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;

Presumably, there are child I2C devices here. Please show that in the 
example.

> +			};
> +
> +			remote_i2c1: i2c@1 {
> +				reg = <1>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +			};
> +		};
> +	};
> +};
> +
> +&ds90ub954_fpd3_in0 {
> +	remote-chip {
> +		compatible = "ti,ds90ub953-q1";
> +		gpio-functions = <DS90_GPIO_FUNC_OUTPUT_REMOTE

Not documented.

> +				  DS90_GPIO_FUNC_UNUSED
> +				  DS90_GPIO_FUNC_UNUSED
> +				  DS90_GPIO_FUNC_UNUSED>;
> +	};
> +};
> +
> +&ds90ub954_fpd3_in1 {
> +	remote-chip {
> +		compatible = "ti,ds90ub953-q1";
> +		gpio-functions = <DS90_GPIO_FUNC_OUTPUT_REMOTE
> +				  DS90_GPIO_FUNC_UNUSED
> +				  DS90_GPIO_FUNC_UNUSED
> +				  DS90_GPIO_FUNC_UNUSED>;
> +	};
> +};
> +
> +&remote_i2c0 {
> +	sensor_0@3c {
> +		compatible = "sony,imx274";
> +		reg = <0x3c>;
> +
> +		reset-gpios = <&deser 0 0 GPIO_ACTIVE_LOW>;
> +
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		port@0 {
> +			reg = <0>;
> +			sensor_0_out: endpoint {
> +				remote-endpoint = <&ds90ub954_fpd3_in0>;
> +			};
> +		};
> +	};
> +};
> +
> +&remote_i2c1 {
> +	sensor_0@3c {
> +		compatible = "sony,imx274";
> +		reg = <0x3c>;
> +
> +		reset-gpios = <&deser 1 0 GPIO_ACTIVE_LOW>;
> +
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		port@0 {
> +			reg = <0>;
> +			sensor_1_out: endpoint {
> +				remote-endpoint = <&ds90ub954_fpd3_in1>;
> +			};
> +		};
> +	};
> +};
> -- 
> 2.17.1
>
Luca Ceresoli Aug. 19, 2019, 10:41 p.m. UTC | #2
Hi Rob,

thanks for your review.

On 13/08/19 17:44, Rob Herring wrote:
> On Tue, Jul 23, 2019 at 10:37:20PM +0200, Luca Ceresoli wrote:
>> Describe the Texas Instruments DS90UB954-Q1, a 2-input video deserializer
>> with I2C Address Translator and remote GPIOs.
>>
>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
>>
>> ---
>>
>> Changes RFCv1 -> RFCv2:
>>
>>  - add explicit aliases for the FPD-link RX ports (optional)
>>  - add proper remote GPIO description
>> ---
>>  .../bindings/media/i2c/ti,ds90ub954-q1.txt    | 194 ++++++++++++++++++
>>  1 file changed, 194 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt
>> new file mode 100644
>> index 000000000000..73ce21ecc3b6
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt
>> @@ -0,0 +1,194 @@
>> +Texas Instruments DS90UB954-Q1 dual video deserializer
>> +======================================================
>> +
>> +The TI DS90UB954-Q1 is a MIPI CSI-2 video deserializer that forwards video
>> +streams from up to two FPD-Link 3 connections to a MIPI CSI-2 output. It
>> +also allows access to remote I2C and GPIO.
>> +
>> +Required properties:
>> +
>> + - compatible: must be "ti,ds90ub954-q1"
>> +
>> + - reg: main I2C slave address; optionally aliases for RX port registers
>> +   and remote serializers. The main address is mandatory and must be the
>> +   first, others are optional and fall back to defaults if not
>> +   specified. See "reg-names".
>> +
>> + - reset-gpios: chip reset GPIO, active low (connected to PDB pin of the chip)
>> + - i2c-alias-pool: list of I2C addresses that are known to be available on the
>> +                   "local" (SoC-to-deser) I2C bus; they will be picked at
>> +		   runtime and used as aliases to reach remove I2C chips
> 
> s/remove/remote/

Will fix.

> Needs a vendor prefix.

The ultimate goal here is to define a standard property that all chips
able to to I2C forwarding (video serdes or, potentially, other chips)
can use. That's why the GMSL (Maxim deser) developers are in Cc: they
are also facing a similar need.

However I'm OK to change this to "ti,i2c-alias-pool" just in case there
are reasons to not use a common name [yet]. However, following this
argument, shouldn't a prefix be needed also for other nonstandard
strings, such as "i2c-atr" below?

>> + - gpio-controller
>> + - #gpio-cells: must be 3: FPD-Link 3 RX port number, remote gpio number, flags
> 
> We're pretty standardized on 2 cells for GPIO. Perhaps combine the port 
> and gpio number to 1 cell.

Oh dear. I dislike implementing software that does not model the
physical reality. I know it will bite me back sooner or later. Here we
_really_ have N physically separate GPIO controllers, and the number of
GPIOs they have depends on the model of the chip that is connected remotely.

This is how things look in the case of 2 ports:

 <-- base board -->         <------- remote camera module 1 ----->
                            .---------------------.
 .-----.    .------.        |         SER 1       |
 | CPU |----|port 1|========|----------.          |
 `-----'    |      |  FPD   | GPIO ctl |          |
            |      | Link 3 `---------------------'
            |      | cable        ||||
	    | DES  |         remote GPIO pins
            |      |
            |      |        <------- remote camera module 2 ----->
            |      |        .---------------------.
            |port 2|        |         SER 2       |
            |      |========|----------.          |
            `------'  FPD   | GPIO ctl |          |
                     Link 3 `---------------------'
                     cable    ||||||||
	                     remote GPIO pins

Perhaps we should have N separate gpiochips, one per port? Under which
node? Not under "ports" ("We don't allow other nodes within graph
nodes"), and "i2c-atr" does not seem like a suitable name. Under a new
"remote-gpiochips" node?

>> +Required subnodes:
>> +
>> + - ports: A ports node with one port child node per device input and output
>> +          port, in accordance with the video interface bindings defined in
>> +          Documentation/devicetree/bindings/media/video-interfaces.txt. The
>> +          port nodes are numbered as follows:
>> +
>> +          Port Description
>> +          ------------------------------------
>> +          0    Input from FPD-Link 3 RX port 0
>> +          1    Input from FPD-Link 3 RX port 1
>> +          2    CSI-2 output
>> +
>> +          Each port must have a "remote-chip" subnode that defines the remote
>> +	  chip (serializer) with at least a "compatible" property
> 
> We don't allow other nodes within graph nodes. I'm not really clear what 
> you are trying to do here.

Each of the deser ports (2 ports in this chip) creates a physical
point-to-point "bus". It's called "FPD-Link 3" in the TI chips, "GMSL"
in the Maxim chips. One "remote chip" serializer can be connected to
each bus. The remote chip has, at least, a model (e.g. DS90UB953) and
some properties. So I need a place where model and properties can be
described. The port node looked like a good place, but as you point out
it is not.

Adding to the above discussion about 3 gpio-cells, I can think of a
different, tentative DT layout:

deser: deser@3d {
	compatible = "ti,ds90ub954-q1";
	reg-names = "main", "rxport0", "rxport1", "ser0", "ser1";
	reg       = <0x3d>,  <0x40>,    <0x41>,   <0x44>, <0x45>;
	clocks = <&clk_25M>;
	interrupt-parent = <&gic>;
	interrupts = <3 1 IRQ_TYPE_LEVEL_HIGH>;
	reset-gpios = <&gpio_ctl 4 GPIO_ACTIVE_LOW>;

	i2c-alias-pool = /bits/ 16 <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>;

	ports {
		#address-cells = <1>;
		#size-cells = <0>;

		port@0 {
			reg = <0>;
			endpoint {
				remote-endpoint = <&sensor_0_out>;
			};
		};

		port@1 {
			reg = <1>;
			endpoint {
				remote-endpoint = <&sensor_1_out>;
			};
		};

		port@2 {
			reg = <2>;
			endpoint {
				data-lanes = <1 2 3 4>;
				/* Actually a REFCLK multiplier */
				data-rate = <1600000000>;
				remote-endpoint = <&csirx_0_in>;
			};
		};
	};

	remote-chips {
		#address-cells = <1>;
		#size-cells = <0>;

		remote-chip@0 {
			reg = <0>;
			chip {
				compatible = "ti,ds90ub953-q1";
				gpio-functions = <...>;
			};

			remote_i2c0: i2c@0 {
				reg = <0>;
				#address-cells = <1>;
				#size-cells = <0>;
			};

			gpio-controller;
			/* 2 cells for _each_ gpiochip */
			#gpio-cells = <2>; /* remote gpio num, flags */
		};

		remote-chip@1 {
			reg = <1>;
			/* similar to remote chip 0 */
		};
	};
};

Here there are two subnodes:

 - "remote-chips" with a subnode per remote port, each describing
   the remote chip model and properties, remote i2c bus, gpio controller
   and others (other chips have I2S, SPI, UART...)

 - "ports" with only the standard v4l2 port properties, no subnodes
   under the endpoint nodes

Does this look better from the device tree point of view?

>> + - i2c-atr: contains one child per RX port, each describes the I2C bus on
>> +            the remote side
>> +
>> +	    Required properties:
>> +	    - #address-cells = <1>;
>> +	    - #size-cells = <0>;
>> +
>> +	    Subnodes: one per each FPD-link RX port, each having:
>> +
>> +	    Required properties for "i2c-atr" child bus nodes:
>> +	    - reg: The number of the port where the remove chip is connected
> 
> s/remove/remote/

Ok.

>> +	    - #address-cells = <1>;
>> +	    - #size-cells = <0>;
>> +
>> +	    Optional properties for "i2c-atr" child bus nodes:
>> +	    - Other properties specific to the remote hardware
> 
> Such as?

"clock-frequency" at least. The remote chip is an I2C master, thus any
property that applies to an I2C master might apply as well.

"clock-frequency" is only half-implemented in these RFC patches:

 * in patch 5, function ds90_rxport_add_serializer() passes the REFCLK
   value of the deser to the remote chip (serializer): this value is
   the physical reference clock the remote chip receives, all of its
   timings are based on it

 * the remote chip, given refclk, computes the register values that best
   approximate "clock-frequency" [not implemented in this version, would
   be in patch 6]

I plan to implement and document the whole clock-frequency feature for
the next patch iteration. But I'd love to receive comments about how
reflck is passed from the deser to the serializer via platform_data.

>> +Device node example
>> +-------------------
>> +
>> +&i2c0 {
>> +	deser: deser@3d {
>> +		compatible = "ti,ds90ub954-q1";
>> +		reg-names = "main", "rxport0", "rxport1", "ser0", "ser1";
>> +		reg       = <0x3d>,  <0x40>,    <0x41>,   <0x44>, <0x45>;
>> +		clocks = <&clk_25M>;
>> +		interrupt-parent = <&gic>;
>> +		interrupts = <3 1 IRQ_TYPE_LEVEL_HIGH>;
>> +		reset-gpios = <&gpio_ctl 4 GPIO_ACTIVE_LOW>;
>> +
>> +		i2c-alias-pool = /bits/ 16 <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>;
>> +
>> +		gpio-controller;
>> +		#gpio-cells = <3>; /* rxport, remote gpio num, flags */
>> +
>> +		ports {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +
>> +			port@0 {
>> +				reg = <0>;
>> +				ds90ub954_fpd3_in0: endpoint {
>> +					remote-endpoint = <&sensor_0_out>;
>> +				};
>> +			};
>> +
>> +			port@1 {
>> +				reg = <1>;
>> +				ds90ub954_fpd3_in1: endpoint {
>> +					remote-endpoint = <&sensor_1_out>;
>> +				};
>> +			};
>> +
>> +			port@2 {
>> +				reg = <2>;
>> +				ds90ub954_mipi_out0: endpoint {
>> +					data-lanes = <1 2 3 4>;
>> +					/* Actually a REFCLK multiplier */
>> +					data-rate = <1600000000>;
>> +					remote-endpoint = <&csirx_0_in>;
>> +				};
>> +			};
>> +		};
>> +
>> +		i2c-atr {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +
>> +			remote_i2c0: i2c@0 {
>> +				reg = <0>;
>> +				#address-cells = <1>;
>> +				#size-cells = <0>;
> 
> Presumably, there are child I2C devices here. Please show that in the 
> example.

They are shown below to avoid excessive nesting, look for "&remote_i2c0"
(1).

>> +&ds90ub954_fpd3_in0 {
>> +	remote-chip {
>> +		compatible = "ti,ds90ub953-q1";
>> +		gpio-functions = <DS90_GPIO_FUNC_OUTPUT_REMOTE
> 
> Not documented.

That's because this node describes the remote chip. The remote chip is a
ti,ds90ub953-q1, its bindings are defined in patch 4. It's similar to
showing some child I2C devices under the I2C master, as you suggested
just above, except it's not an I2C bus but an FPD-Link 3 "bus".

Is it OK if I replace the whole gpio-functions node with the comment
"/* properties specific to the ti,ds90ub953-q1 chip */"?

>> +&remote_i2c0 {
>> +	sensor_0@3c {
>> +		compatible = "sony,imx274";
>> +		reg = <0x3c>;

(1) here are the child I2C device mentioned above
Rob Herring Aug. 20, 2019, 3:44 p.m. UTC | #3
On Mon, Aug 19, 2019 at 5:41 PM Luca Ceresoli <luca@lucaceresoli.net> wrote:
>
> Hi Rob,
>
> thanks for your review.
>
> On 13/08/19 17:44, Rob Herring wrote:
> > On Tue, Jul 23, 2019 at 10:37:20PM +0200, Luca Ceresoli wrote:
> >> Describe the Texas Instruments DS90UB954-Q1, a 2-input video deserializer
> >> with I2C Address Translator and remote GPIOs.
> >>
> >> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> >>
> >> ---
> >>
> >> Changes RFCv1 -> RFCv2:
> >>
> >>  - add explicit aliases for the FPD-link RX ports (optional)
> >>  - add proper remote GPIO description
> >> ---
> >>  .../bindings/media/i2c/ti,ds90ub954-q1.txt    | 194 ++++++++++++++++++
> >>  1 file changed, 194 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt
> >> new file mode 100644
> >> index 000000000000..73ce21ecc3b6
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt
> >> @@ -0,0 +1,194 @@
> >> +Texas Instruments DS90UB954-Q1 dual video deserializer
> >> +======================================================
> >> +
> >> +The TI DS90UB954-Q1 is a MIPI CSI-2 video deserializer that forwards video
> >> +streams from up to two FPD-Link 3 connections to a MIPI CSI-2 output. It
> >> +also allows access to remote I2C and GPIO.
> >> +
> >> +Required properties:
> >> +
> >> + - compatible: must be "ti,ds90ub954-q1"
> >> +
> >> + - reg: main I2C slave address; optionally aliases for RX port registers
> >> +   and remote serializers. The main address is mandatory and must be the
> >> +   first, others are optional and fall back to defaults if not
> >> +   specified. See "reg-names".
> >> +
> >> + - reset-gpios: chip reset GPIO, active low (connected to PDB pin of the chip)
> >> + - i2c-alias-pool: list of I2C addresses that are known to be available on the
> >> +                   "local" (SoC-to-deser) I2C bus; they will be picked at
> >> +               runtime and used as aliases to reach remove I2C chips
> >
> > s/remove/remote/
>
> Will fix.
>
> > Needs a vendor prefix.
>
> The ultimate goal here is to define a standard property that all chips
> able to to I2C forwarding (video serdes or, potentially, other chips)
> can use. That's why the GMSL (Maxim deser) developers are in Cc: they
> are also facing a similar need.
>
> However I'm OK to change this to "ti,i2c-alias-pool" just in case there
> are reasons to not use a common name [yet]. However, following this
> argument, shouldn't a prefix be needed also for other nonstandard
> strings, such as "i2c-atr" below?

Okay, no vendor prefix is fine.

'i2c-atr' is a node name, not a property so no vendor prefix.

>
> >> + - gpio-controller
> >> + - #gpio-cells: must be 3: FPD-Link 3 RX port number, remote gpio number, flags
> >
> > We're pretty standardized on 2 cells for GPIO. Perhaps combine the port
> > and gpio number to 1 cell.
>
> Oh dear. I dislike implementing software that does not model the
> physical reality. I know it will bite me back sooner or later. Here we
> _really_ have N physically separate GPIO controllers, and the number of
> GPIOs they have depends on the model of the chip that is connected remotely.
>
> This is how things look in the case of 2 ports:
>
>  <-- base board -->         <------- remote camera module 1 ----->
>                             .---------------------.
>  .-----.    .------.        |         SER 1       |
>  | CPU |----|port 1|========|----------.          |
>  `-----'    |      |  FPD   | GPIO ctl |          |
>             |      | Link 3 `---------------------'
>             |      | cable        ||||
>             | DES  |         remote GPIO pins
>             |      |
>             |      |        <------- remote camera module 2 ----->
>             |      |        .---------------------.
>             |port 2|        |         SER 2       |
>             |      |========|----------.          |
>             `------'  FPD   | GPIO ctl |          |
>                      Link 3 `---------------------'
>                      cable    ||||||||
>                              remote GPIO pins
>
> Perhaps we should have N separate gpiochips, one per port?

Yes, seems like it.

> Under which
> node? Not under "ports" ("We don't allow other nodes within graph
> nodes"), and "i2c-atr" does not seem like a suitable name. Under a new
> "remote-gpiochips" node?
>
> >> +Required subnodes:
> >> +
> >> + - ports: A ports node with one port child node per device input and output
> >> +          port, in accordance with the video interface bindings defined in
> >> +          Documentation/devicetree/bindings/media/video-interfaces.txt. The
> >> +          port nodes are numbered as follows:
> >> +
> >> +          Port Description
> >> +          ------------------------------------
> >> +          0    Input from FPD-Link 3 RX port 0
> >> +          1    Input from FPD-Link 3 RX port 1
> >> +          2    CSI-2 output
> >> +
> >> +          Each port must have a "remote-chip" subnode that defines the remote
> >> +      chip (serializer) with at least a "compatible" property
> >
> > We don't allow other nodes within graph nodes. I'm not really clear what
> > you are trying to do here.
>
> Each of the deser ports (2 ports in this chip) creates a physical
> point-to-point "bus". It's called "FPD-Link 3" in the TI chips, "GMSL"
> in the Maxim chips. One "remote chip" serializer can be connected to
> each bus. The remote chip has, at least, a model (e.g. DS90UB953) and
> some properties. So I need a place where model and properties can be
> described. The port node looked like a good place, but as you point out
> it is not.
>
> Adding to the above discussion about 3 gpio-cells, I can think of a
> different, tentative DT layout:
>
> deser: deser@3d {
>         compatible = "ti,ds90ub954-q1";
>         reg-names = "main", "rxport0", "rxport1", "ser0", "ser1";
>         reg       = <0x3d>,  <0x40>,    <0x41>,   <0x44>, <0x45>;
>         clocks = <&clk_25M>;
>         interrupt-parent = <&gic>;
>         interrupts = <3 1 IRQ_TYPE_LEVEL_HIGH>;
>         reset-gpios = <&gpio_ctl 4 GPIO_ACTIVE_LOW>;
>
>         i2c-alias-pool = /bits/ 16 <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>;
>
>         ports {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>
>                 port@0 {
>                         reg = <0>;
>                         endpoint {
>                                 remote-endpoint = <&sensor_0_out>;
>                         };
>                 };
>
>                 port@1 {
>                         reg = <1>;
>                         endpoint {
>                                 remote-endpoint = <&sensor_1_out>;
>                         };
>                 };
>
>                 port@2 {
>                         reg = <2>;
>                         endpoint {
>                                 data-lanes = <1 2 3 4>;
>                                 /* Actually a REFCLK multiplier */
>                                 data-rate = <1600000000>;
>                                 remote-endpoint = <&csirx_0_in>;
>                         };
>                 };
>         };
>
>         remote-chips {

I don't have a better suggestion for the location of this.

>                 #address-cells = <1>;
>                 #size-cells = <0>;
>
>                 remote-chip@0 {
>                         reg = <0>;
>                         chip {
>                                 compatible = "ti,ds90ub953-q1";

Seems like this should be in the parent? It's the ds90ub953
implementing the I2C bus, GPIO controller, etc.?

>                                 gpio-functions = <...>;
>                         };
>
>                         remote_i2c0: i2c@0 {

What does '0' mean here? At a given level, you can only have 1 number
space of addresses.

>                                 reg = <0>;
>                                 #address-cells = <1>;
>                                 #size-cells = <0>;
>                         };
>
>                         gpio-controller;
>                         /* 2 cells for _each_ gpiochip */
>                         #gpio-cells = <2>; /* remote gpio num, flags */
>                 };
>
>                 remote-chip@1 {
>                         reg = <1>;
>                         /* similar to remote chip 0 */
>                 };
>         };
> };
>
> Here there are two subnodes:
>
>  - "remote-chips" with a subnode per remote port, each describing
>    the remote chip model and properties, remote i2c bus, gpio controller
>    and others (other chips have I2S, SPI, UART...)
>
>  - "ports" with only the standard v4l2 port properties, no subnodes
>    under the endpoint nodes
>
> Does this look better from the device tree point of view?

Yes.

> >> +        - #address-cells = <1>;
> >> +        - #size-cells = <0>;
> >> +
> >> +        Optional properties for "i2c-atr" child bus nodes:
> >> +        - Other properties specific to the remote hardware
> >
> > Such as?
>
> "clock-frequency" at least. The remote chip is an I2C master, thus any
> property that applies to an I2C master might apply as well.
>
> "clock-frequency" is only half-implemented in these RFC patches:
>
>  * in patch 5, function ds90_rxport_add_serializer() passes the REFCLK
>    value of the deser to the remote chip (serializer): this value is
>    the physical reference clock the remote chip receives, all of its
>    timings are based on it
>
>  * the remote chip, given refclk, computes the register values that best
>    approximate "clock-frequency" [not implemented in this version, would
>    be in patch 6]
>
> I plan to implement and document the whole clock-frequency feature for
> the next patch iteration. But I'd love to receive comments about how
> reflck is passed from the deser to the serializer via platform_data.

Okay. Please try to make bindings as complete as possible even if
there's not yet driver support.

> >> +            i2c-atr {
> >> +                    #address-cells = <1>;
> >> +                    #size-cells = <0>;
> >> +
> >> +                    remote_i2c0: i2c@0 {
> >> +                            reg = <0>;
> >> +                            #address-cells = <1>;
> >> +                            #size-cells = <0>;
> >
> > Presumably, there are child I2C devices here. Please show that in the
> > example.
>
> They are shown below to avoid excessive nesting, look for "&remote_i2c0"
> (1).

I'd rather have longish lines than have things split up.

>
> >> +&ds90ub954_fpd3_in0 {
> >> +    remote-chip {
> >> +            compatible = "ti,ds90ub953-q1";
> >> +            gpio-functions = <DS90_GPIO_FUNC_OUTPUT_REMOTE
> >
> > Not documented.
>
> That's because this node describes the remote chip. The remote chip is a
> ti,ds90ub953-q1, its bindings are defined in patch 4. It's similar to
> showing some child I2C devices under the I2C master, as you suggested
> just above, except it's not an I2C bus but an FPD-Link 3 "bus".

I think I realized this after sending this...

>
> Is it OK if I replace the whole gpio-functions node with the comment
> "/* properties specific to the ti,ds90ub953-q1 chip */"?

Not necessary. gpio-functions does need a vendor prefix though.

Rob
Luca Ceresoli Aug. 21, 2019, 9:50 p.m. UTC | #4
Hi Rob,

On 20/08/19 17:44, Rob Herring wrote:
>>>> + - i2c-alias-pool: list of I2C addresses that are known to be available on the
>>>> +                   "local" (SoC-to-deser) I2C bus; they will be picked at
>>>> +               runtime and used as aliases to reach remove I2C chips
>>>
>>> s/remove/remote/
>>
>> Will fix.
>>
>>> Needs a vendor prefix.
>>
>> The ultimate goal here is to define a standard property that all chips
>> able to to I2C forwarding (video serdes or, potentially, other chips)
>> can use. That's why the GMSL (Maxim deser) developers are in Cc: they
>> are also facing a similar need.
>>
>> However I'm OK to change this to "ti,i2c-alias-pool" just in case there
>> are reasons to not use a common name [yet]. However, following this
>> argument, shouldn't a prefix be needed also for other nonstandard
>> strings, such as "i2c-atr" below?
> 
> Okay, no vendor prefix is fine.
> 
> 'i2c-atr' is a node name, not a property so no vendor prefix.

I see, thanks.

>>>> + - gpio-controller
>>>> + - #gpio-cells: must be 3: FPD-Link 3 RX port number, remote gpio number, flags
>>>
>>> We're pretty standardized on 2 cells for GPIO. Perhaps combine the port
>>> and gpio number to 1 cell.
>>
>> Oh dear. I dislike implementing software that does not model the
>> physical reality. I know it will bite me back sooner or later. Here we
>> _really_ have N physically separate GPIO controllers, and the number of
>> GPIOs they have depends on the model of the chip that is connected remotely.
>>
>> This is how things look in the case of 2 ports:
>>
>>  <-- base board -->         <------- remote camera module 1 ----->
>>                             .---------------------.
>>  .-----.    .------.        |         SER 1       |
>>  | CPU |----|port 1|========|----------.          |
>>  `-----'    |      |  FPD   | GPIO ctl |          |
>>             |      | Link 3 `---------------------'
>>             |      | cable        ||||
>>             | DES  |         remote GPIO pins
>>             |      |
>>             |      |        <------- remote camera module 2 ----->
>>             |      |        .---------------------.
>>             |port 2|        |         SER 2       |
>>             |      |========|----------.          |
>>             `------'  FPD   | GPIO ctl |          |
>>                      Link 3 `---------------------'
>>                      cable    ||||||||
>>                              remote GPIO pins
>>
>> Perhaps we should have N separate gpiochips, one per port?
> 
> Yes, seems like it.

I'm more and more convinced of that.

>>>> +Required subnodes:
>>>> +
>>>> + - ports: A ports node with one port child node per device input and output
>>>> +          port, in accordance with the video interface bindings defined in
>>>> +          Documentation/devicetree/bindings/media/video-interfaces.txt. The
>>>> +          port nodes are numbered as follows:
>>>> +
>>>> +          Port Description
>>>> +          ------------------------------------
>>>> +          0    Input from FPD-Link 3 RX port 0
>>>> +          1    Input from FPD-Link 3 RX port 1
>>>> +          2    CSI-2 output
>>>> +
>>>> +          Each port must have a "remote-chip" subnode that defines the remote
>>>> +      chip (serializer) with at least a "compatible" property
>>>
>>> We don't allow other nodes within graph nodes. I'm not really clear what
>>> you are trying to do here.
>>
>> Each of the deser ports (2 ports in this chip) creates a physical
>> point-to-point "bus". It's called "FPD-Link 3" in the TI chips, "GMSL"
>> in the Maxim chips. One "remote chip" serializer can be connected to
>> each bus. The remote chip has, at least, a model (e.g. DS90UB953) and
>> some properties. So I need a place where model and properties can be
>> described. The port node looked like a good place, but as you point out
>> it is not.
>>
>> Adding to the above discussion about 3 gpio-cells, I can think of a
>> different, tentative DT layout:
>>
>> deser: deser@3d {
>>         compatible = "ti,ds90ub954-q1";
>>         reg-names = "main", "rxport0", "rxport1", "ser0", "ser1";
>>         reg       = <0x3d>,  <0x40>,    <0x41>,   <0x44>, <0x45>;
>>         clocks = <&clk_25M>;
>>         interrupt-parent = <&gic>;
>>         interrupts = <3 1 IRQ_TYPE_LEVEL_HIGH>;
>>         reset-gpios = <&gpio_ctl 4 GPIO_ACTIVE_LOW>;
>>
>>         i2c-alias-pool = /bits/ 16 <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>;
>>
>>         ports {
>>                 #address-cells = <1>;
>>                 #size-cells = <0>;
>>
>>                 port@0 {
>>                         reg = <0>;
>>                         endpoint {
>>                                 remote-endpoint = <&sensor_0_out>;
>>                         };
>>                 };
>>
>>                 port@1 {
>>                         reg = <1>;
>>                         endpoint {
>>                                 remote-endpoint = <&sensor_1_out>;
>>                         };
>>                 };
>>
>>                 port@2 {
>>                         reg = <2>;
>>                         endpoint {
>>                                 data-lanes = <1 2 3 4>;
>>                                 /* Actually a REFCLK multiplier */
>>                                 data-rate = <1600000000>;
>>                                 remote-endpoint = <&csirx_0_in>;
>>                         };
>>                 };
>>         };
>>
>>         remote-chips {
> 
> I don't have a better suggestion for the location of this.
> 
>>                 #address-cells = <1>;
>>                 #size-cells = <0>;
>>
>>                 remote-chip@0 {
>>                         reg = <0>;
>>                         chip {
>>                                 compatible = "ti,ds90ub953-q1";
> 
> Seems like this should be in the parent? It's the ds90ub953
> implementing the I2C bus, GPIO controller, etc.?

You're right: it's the ds90ub953 implementing i2c, gpio etc. The
(undoubtedly weird) DT structure originates from the hotplugging issue
that I explained in the cover letter. The compromise I found to handle
it is to describe the remote I2C adapters and GPIO controllers as if
they were part of the "local" domain (the ds90ub954). This is why they
are represented in DT outside the ds90ub953 node: the "chip" node would
be added by an overlay, the i2c and gpio nodes are always there.

But this corresponds to how I implemented it in the code. Let's see if
at least DT can have a better layout:

deser: deser@3d {
	compatible = "ti,ds90ub954-q1";
	reg-names = "main", "rxport0", "rxport1", "ser0", "ser1";
	reg       = <0x3d>,  <0x40>,    <0x41>,   <0x44>, <0x45>;
	clocks = <&clk_25M>;
	interrupt-parent = <&gic>;
	interrupts = <3 1 IRQ_TYPE_LEVEL_HIGH>;
	reset-gpios = <&gpio_ctl 4 GPIO_ACTIVE_LOW>;

	i2c-alias-pool = /bits/ 16 <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>;

	ports {
		#address-cells = <1>;
		#size-cells = <0>;

		port@0 {
			reg = <0>;
			endpoint {
				remote-endpoint = <&sensor_0_out>;
			};
		};

		port@1 {
			reg = <1>;
			endpoint {
				remote-endpoint = <&sensor_1_out>;
			};
		};

		port@2 {
			reg = <2>;
			endpoint {
				data-lanes = <1 2 3 4>;
				/* Actually a REFCLK multiplier */
				data-rate = <1600000000>;
				remote-endpoint = <&csirx_0_in>;
			};
		};
	};

	remote-chips {
		#address-cells = <1>;
		#size-cells = <0>;

		remote-chip@0 {
			reg = <0>;

			/* dynamic */
			compatible = "ti,ds90ub953-q1";
			gpio-functions = <...>;

			remote_i2c0: i2c {
				reg = <0>;
				#address-cells = <1>;
				#size-cells = <0>;
			};

			gpio-controller;
			/* 2 cells for _each_ gpiochip: */
			/* remote gpio num, flags */
			#gpio-cells = <2>;
		};

		remote-chip@1 {
			reg = <1>;
			/* similar to remote chip 0 */
		};
	};
};

Now things _look_ correct: I just removed the "chip" subnode and hoisted
its properties up, where the i2c and gpio nodes are. However this would
not correspond a currently doable implementation. The gpio- and
i2c-related nodes under remote-chip@<N> would be always instantiated in
the base board DT, while the "compatible" property would be
added/removed by overlays on hotplug. This would require the deser chip
driver to probe the remote serializer chip not when the device node
appears but when the compatible property appears inside it. This would
look weird on the device driver side, even though I understand very well
that a DT needs to be future-proof much more than a driver.

I'll have a more detailed look into this.

Also, in case this were not clear, no hotplugging is actually
implemented currently, mostly due to the lack of a complete
implementation of runtime DT overlay insertion and removal. I think work
is in progress on that side, but it doesn't look like it is done yet.
However I'm trying to be ready for it when it will be available.

>>                         remote_i2c0: i2c@0 {
> 
> What does '0' mean here? At a given level, you can only have 1 number
> space of addresses.

Copy-paste leftover, will fix.

>>>> +        - #address-cells = <1>;
>>>> +        - #size-cells = <0>;
>>>> +
>>>> +        Optional properties for "i2c-atr" child bus nodes:
>>>> +        - Other properties specific to the remote hardware
>>>
>>> Such as?
>>
>> "clock-frequency" at least. The remote chip is an I2C master, thus any
>> property that applies to an I2C master might apply as well.
>>
>> "clock-frequency" is only half-implemented in these RFC patches:
>>
>>  * in patch 5, function ds90_rxport_add_serializer() passes the REFCLK
>>    value of the deser to the remote chip (serializer): this value is
>>    the physical reference clock the remote chip receives, all of its
>>    timings are based on it
>>
>>  * the remote chip, given refclk, computes the register values that best
>>    approximate "clock-frequency" [not implemented in this version, would
>>    be in patch 6]
>>
>> I plan to implement and document the whole clock-frequency feature for
>> the next patch iteration. But I'd love to receive comments about how
>> reflck is passed from the deser to the serializer via platform_data.
> 
> Okay. Please try to make bindings as complete as possible even if
> there's not yet driver support.
> 
>>>> +            i2c-atr {
>>>> +                    #address-cells = <1>;
>>>> +                    #size-cells = <0>;
>>>> +
>>>> +                    remote_i2c0: i2c@0 {
>>>> +                            reg = <0>;
>>>> +                            #address-cells = <1>;
>>>> +                            #size-cells = <0>;
>>>
>>> Presumably, there are child I2C devices here. Please show that in the
>>> example.
>>
>> They are shown below to avoid excessive nesting, look for "&remote_i2c0"
>> (1).
> 
> I'd rather have longish lines than have things split up.

Ok, but do we have 80-chars limit in the bindings too?

>>>> +&ds90ub954_fpd3_in0 {
>>>> +    remote-chip {
>>>> +            compatible = "ti,ds90ub953-q1";
>>>> +            gpio-functions = <DS90_GPIO_FUNC_OUTPUT_REMOTE
>>>
>>> Not documented.
>>
>> That's because this node describes the remote chip. The remote chip is a
>> ti,ds90ub953-q1, its bindings are defined in patch 4. It's similar to
>> showing some child I2C devices under the I2C master, as you suggested
>> just above, except it's not an I2C bus but an FPD-Link 3 "bus".
> 
> I think I realized this after sending this...
> 
>>
>> Is it OK if I replace the whole gpio-functions node with the comment
>> "/* properties specific to the ti,ds90ub953-q1 chip */"?
> 
> Not necessary. gpio-functions does need a vendor prefix though.

Fair enough. I think other models/brands of serdes chips will need
something similar, but they could accept different values, wo I'll add a
"ti," prefix.

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt
new file mode 100644
index 000000000000..73ce21ecc3b6
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub954-q1.txt
@@ -0,0 +1,194 @@ 
+Texas Instruments DS90UB954-Q1 dual video deserializer
+======================================================
+
+The TI DS90UB954-Q1 is a MIPI CSI-2 video deserializer that forwards video
+streams from up to two FPD-Link 3 connections to a MIPI CSI-2 output. It
+also allows access to remote I2C and GPIO.
+
+Required properties:
+
+ - compatible: must be "ti,ds90ub954-q1"
+
+ - reg: main I2C slave address; optionally aliases for RX port registers
+   and remote serializers. The main address is mandatory and must be the
+   first, others are optional and fall back to defaults if not
+   specified. See "reg-names".
+
+ - reset-gpios: chip reset GPIO, active low (connected to PDB pin of the chip)
+ - i2c-alias-pool: list of I2C addresses that are known to be available on the
+                   "local" (SoC-to-deser) I2C bus; they will be picked at
+		   runtime and used as aliases to reach remove I2C chips
+ - gpio-controller
+ - #gpio-cells: must be 3: FPD-Link 3 RX port number, remote gpio number, flags
+
+Optional properties:
+
+ - reg-names: names of I2C address used to communicate with the chip, must
+              match the "reg" values; mandatory if there are 2 or more
+              addresses
+    - "main": the main I2C address, used to access shared registers
+    - "rxport0", "rxport1": I2C alias to access FPD-link RX port specific
+      registers; must not be used by other slaves on the same bus
+    - "ser0", "ser1": I2C alias to access the remote serializer connected
+      on each FPD-link RX port; must not be used by other slaves on the
+      same bus
+ - interrupts: interrupt pin from the chip
+
+Required subnodes:
+
+ - ports: A ports node with one port child node per device input and output
+          port, in accordance with the video interface bindings defined in
+          Documentation/devicetree/bindings/media/video-interfaces.txt. The
+          port nodes are numbered as follows:
+
+          Port Description
+          ------------------------------------
+          0    Input from FPD-Link 3 RX port 0
+          1    Input from FPD-Link 3 RX port 1
+          2    CSI-2 output
+
+          Each port must have a "remote-chip" subnode that defines the remote
+	  chip (serializer) with at least a "compatible" property
+
+ - i2c-atr: contains one child per RX port, each describes the I2C bus on
+            the remote side
+
+	    Required properties:
+	    - #address-cells = <1>;
+	    - #size-cells = <0>;
+
+	    Subnodes: one per each FPD-link RX port, each having:
+
+	    Required properties for "i2c-atr" child bus nodes:
+	    - reg: The number of the port where the remove chip is connected
+	    - #address-cells = <1>;
+	    - #size-cells = <0>;
+
+	    Optional properties for "i2c-atr" child bus nodes:
+	    - Other properties specific to the remote hardware
+	    - Child nodes conforming to i2c bus binding
+
+
+Device node example
+-------------------
+
+&i2c0 {
+	deser: deser@3d {
+		compatible = "ti,ds90ub954-q1";
+		reg-names = "main", "rxport0", "rxport1", "ser0", "ser1";
+		reg       = <0x3d>,  <0x40>,    <0x41>,   <0x44>, <0x45>;
+		clocks = <&clk_25M>;
+		interrupt-parent = <&gic>;
+		interrupts = <3 1 IRQ_TYPE_LEVEL_HIGH>;
+		reset-gpios = <&gpio_ctl 4 GPIO_ACTIVE_LOW>;
+
+		i2c-alias-pool = /bits/ 16 <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>;
+
+		gpio-controller;
+		#gpio-cells = <3>; /* rxport, remote gpio num, flags */
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				ds90ub954_fpd3_in0: endpoint {
+					remote-endpoint = <&sensor_0_out>;
+				};
+			};
+
+			port@1 {
+				reg = <1>;
+				ds90ub954_fpd3_in1: endpoint {
+					remote-endpoint = <&sensor_1_out>;
+				};
+			};
+
+			port@2 {
+				reg = <2>;
+				ds90ub954_mipi_out0: endpoint {
+					data-lanes = <1 2 3 4>;
+					/* Actually a REFCLK multiplier */
+					data-rate = <1600000000>;
+					remote-endpoint = <&csirx_0_in>;
+				};
+			};
+		};
+
+		i2c-atr {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			remote_i2c0: i2c@0 {
+				reg = <0>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
+
+			remote_i2c1: i2c@1 {
+				reg = <1>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
+		};
+	};
+};
+
+&ds90ub954_fpd3_in0 {
+	remote-chip {
+		compatible = "ti,ds90ub953-q1";
+		gpio-functions = <DS90_GPIO_FUNC_OUTPUT_REMOTE
+				  DS90_GPIO_FUNC_UNUSED
+				  DS90_GPIO_FUNC_UNUSED
+				  DS90_GPIO_FUNC_UNUSED>;
+	};
+};
+
+&ds90ub954_fpd3_in1 {
+	remote-chip {
+		compatible = "ti,ds90ub953-q1";
+		gpio-functions = <DS90_GPIO_FUNC_OUTPUT_REMOTE
+				  DS90_GPIO_FUNC_UNUSED
+				  DS90_GPIO_FUNC_UNUSED
+				  DS90_GPIO_FUNC_UNUSED>;
+	};
+};
+
+&remote_i2c0 {
+	sensor_0@3c {
+		compatible = "sony,imx274";
+		reg = <0x3c>;
+
+		reset-gpios = <&deser 0 0 GPIO_ACTIVE_LOW>;
+
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		port@0 {
+			reg = <0>;
+			sensor_0_out: endpoint {
+				remote-endpoint = <&ds90ub954_fpd3_in0>;
+			};
+		};
+	};
+};
+
+&remote_i2c1 {
+	sensor_0@3c {
+		compatible = "sony,imx274";
+		reg = <0x3c>;
+
+		reset-gpios = <&deser 1 0 GPIO_ACTIVE_LOW>;
+
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		port@0 {
+			reg = <0>;
+			sensor_1_out: endpoint {
+				remote-endpoint = <&ds90ub954_fpd3_in1>;
+			};
+		};
+	};
+};