[v5,5/8] dt-bindings: media: add bindings for TI DS90UB960

  Add DT bindings for TI DS90UB960 FPDLink-3 Deserializer.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 .../bindings/media/i2c/ti,ds90ub960.yaml      | 358 ++++++++++++++++++
 1 file changed, 358 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
  

On Thu, 08 Dec 2022 12:40:03 +0200, Tomi Valkeinen wrote:
> Add DT bindings for TI DS90UB960 FPDLink-3 Deserializer.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  .../bindings/media/i2c/ti,ds90ub960.yaml      | 358 ++++++++++++++++++
>  1 file changed, 358 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>
  

Hi Tomi,

Thank you for the patch.

On Thu, Dec 08, 2022 at 12:40:03PM +0200, Tomi Valkeinen wrote:
> Add DT bindings for TI DS90UB960 FPDLink-3 Deserializer.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  .../bindings/media/i2c/ti,ds90ub960.yaml      | 358 ++++++++++++++++++
>  1 file changed, 358 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
> new file mode 100644
> index 000000000000..d8b5e219d420
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
> @@ -0,0 +1,358 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/ti,ds90ub960.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments DS90UB9XX Family FPD-Link Deserializer Hubs
> +
> +maintainers:
> +  - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> +
> +description:
> +  The TI DS90UB9XX devices are FPD-Link video deserializers with I2C and GPIO
> +  forwarding.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ti,ds90ub960-q1
> +      - ti,ds90ub9702-q1
> +
> +  reg:
> +    maxItems: 1
> +    description:
> +      i2c addresses for the deserializer and the serializers

s/i2c/I2C/

Same below.

A bit more details would be nice, for instance the order in which
addresses should be specified should be documented. The example below
has one address only, so it's quite unclear. Or is this a left-over,
from before the i2c-alias-pool ?

> +
> +  clocks:
> +    maxItems: 1
> +    description:
> +      Reference clock connected to the REFCLK pin.
> +
> +  clock-names:
> +    items:
> +      - const: refclk
> +
> +  powerdown-gpios:
> +    maxItems: 1
> +    description:
> +      Specifier for the GPIO connected to the PDB pin.
> +
> +  i2c-alias-pool:
> +    $ref: /schemas/types.yaml#/definitions/uint16-array
> +    description:
> +      i2c alias pool is a pool of i2c addresses on the main i2c bus that can be
> +      used to access the remote peripherals. The addresses must be available,
> +      not used by any other peripheral. Each remote peripheral is assigned an
> +      alias from the pool, and transactions to that address will be forwarded
> +      to the remote peripheral, with the address translated to the remote
> +      peripheral's real address.

As this property is optional, should you describe what happens when it's
not specified ?

I would also indicate that the pool doesn't cover the serializers, only
the devices behind them.

> +
> +  links:
> +    type: object
> +    additionalProperties: false
> +
> +    properties:
> +      '#address-cells':
> +        const: 1
> +
> +      '#size-cells':
> +        const: 0
> +
> +      ti,manual-strobe:
> +        type: boolean
> +        description:
> +          Enable manual strobe position and EQ level
> +
> +    patternProperties:
> +      '^link@[0-9a-f]+$':

There can be up to 4 links only, right ? I would then use

      '^link@[0-3]$':

> +        type: object
> +        additionalProperties: false
> +        properties:
> +          reg:
> +            description: The link number
> +            maxItems: 1
> +
> +          i2c-alias:
> +            description:
> +              The i2c address used for the serializer. Transactions to this
> +              address on the i2c bus where the deserializer resides are
> +              forwarded to the serializer.
> +
> +          ti,rx-mode:
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            enum:
> +              - 0 # RAW10
> +              - 1 # RAW12 HF
> +              - 2 # RAW12 LF
> +              - 3 # CSI2 SYNC
> +              - 4 # CSI2 NON-SYNC
> +            description: FPD-Link Input Mode

Are there use cases for controlling this dynamically (in particular the
sync/non-sync modes) ? Is there anything that could be queried at
runtime from the serializers instead of being specified in DT ?

Same question for the parameters below. Additionally, are there any
parameters that need to be identical for all links ?

> +
> +          ti,cdr-mode:
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            enum:
> +              - 0 # FPD3
> +              - 1 # FPD4
> +            description: FPD-Link CDR Mode
> +
> +          ti,strobe-pos:
> +            $ref: /schemas/types.yaml#/definitions/int32
> +            minimum: -13
> +            maximum: 13
> +            description: Manual strobe position
> +
> +          ti,eq-level:
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            maximum: 14
> +            description: Manual EQ level
> +
> +          serializer:
> +            type: object
> +            description: FPD-Link Serializer node
> +
> +        required:
> +          - reg
> +          - i2c-alias
> +          - ti,rx-mode
> +          - serializer
> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        unevaluatedProperties: false
> +        description: FPD-Link input 0
> +
> +      port@1:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        unevaluatedProperties: false
> +        description: FPD-Link input 1
> +
> +      port@2:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        unevaluatedProperties: false
> +        description: FPD-Link input 2
> +
> +      port@3:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        unevaluatedProperties: false
> +        description: FPD-Link input 3
> +
> +      port@4:
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        unevaluatedProperties: false
> +        description: CSI-2 Output 0
> +
> +        properties:
> +          endpoint:
> +            $ref: /schemas/media/video-interfaces.yaml#
> +            unevaluatedProperties: false
> +
> +            properties:
> +              data-lanes:
> +                minItems: 1
> +                maxItems: 4
> +
> +      port@5:
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        unevaluatedProperties: false
> +        description: CSI-2 Output 1
> +
> +        properties:
> +          endpoint:
> +            $ref: /schemas/media/video-interfaces.yaml#
> +            unevaluatedProperties: false
> +
> +            properties:
> +              data-lanes:
> +                minItems: 1
> +                maxItems: 4

The ports should be mandatory, shouldn't they ?

> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - ports
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    i2c {
> +      clock-frequency = <400000>;
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      deser@3d {
> +        compatible = "ti,ds90ub960-q1";
> +        reg = <0x3d>;
> +
> +        clock-names = "refclk";
> +        clocks = <&fixed_clock>;
> +
> +        powerdown-gpios = <&pca9555 7 GPIO_ACTIVE_LOW>;
> +
> +        i2c-alias-pool = /bits/ 16 <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>;
> +
> +        ports {
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +
> +          /* Port 0, Camera 0 */
> +          port@0 {
> +            reg = <0>;
> +
> +            ub960_fpd3_1_in: endpoint {
> +              remote-endpoint = <&ub953_1_out>;
> +            };
> +          };
> +
> +          /* Port 1, Camera 1 */
> +          port@1 {
> +            reg = <1>;
> +
> +            ub960_fpd3_2_in: endpoint {
> +              remote-endpoint = <&ub913_2_out>;
> +            };
> +          };
> +
> +          /* Port 4, CSI-2 TX */
> +          port@4 {
> +            reg = <4>;
> +            ds90ub960_0_csi_out: endpoint {
> +              data-lanes = <1 2 3 4>;
> +              link-frequencies = /bits/ 64 <800000000>;
> +              remote-endpoint = <&csi2_phy0>;
> +            };
> +          };
> +        };
> +
> +        links {
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +
> +          /* Link 0 has DS90UB953 serializer and IMX274 sensor */
> +
> +          link@0 {
> +            reg = <0>;
> +            i2c-alias = <0x44>;
> +
> +            ti,rx-mode = <3>;
> +
> +            serializer1: serializer {
> +              compatible = "ti,ds90ub953-q1";
> +
> +              gpio-controller;
> +              #gpio-cells = <2>;
> +
> +              #clock-cells = <0>;
> +
> +              ports {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                port@0 {
> +                  reg = <0>;
> +                  ub953_1_in: endpoint {
> +                    data-lanes = <1 2 3 4>;
> +                    remote-endpoint = <&sensor_1_out>;
> +                  };
> +                };
> +
> +                port@1 {
> +                  reg = <1>;
> +
> +                  ub953_1_out: endpoint {
> +                    remote-endpoint = <&ub960_fpd3_1_in>;
> +                  };
> +                };
> +              };
> +
> +              i2c {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                sensor@1a {
> +                  compatible = "sony,imx274";
> +                  reg = <0x1a>;
> +
> +                  reset-gpios = <&serializer1 0 GPIO_ACTIVE_LOW>;
> +
> +                  port {
> +                    sensor_1_out: endpoint {
> +                      remote-endpoint = <&ub953_1_in>;
> +                    };
> +                  };
> +                };
> +              };
> +            };
> +          };  /* End of link@0 */
> +
> +          /* Link 1 has DS90UB913 serializer and MT9V111 sensor */
> +
> +          link@1 {
> +            reg = <1>;
> +            i2c-alias = <0x45>;
> +
> +            ti,rx-mode = <0>;
> +
> +            serializer2: serializer {
> +              compatible = "ti,ds90ub913a-q1";
> +
> +              gpio-controller;
> +              #gpio-cells = <2>;
> +
> +              clocks = <&clk_cam_48M>;
> +              clock-names = "clkin";
> +
> +              #clock-cells = <0>;
> +
> +              ports {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                port@0 {
> +                  reg = <0>;
> +                  ub913_2_in: endpoint {
> +                    remote-endpoint = <&sensor_2_out>;
> +                  };
> +                };
> +
> +                port@1 {
> +                  reg = <1>;
> +
> +                  ub913_2_out: endpoint {
> +                    remote-endpoint = <&ub960_fpd3_2_in>;
> +                  };
> +                };
> +              };
> +
> +              i2c {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                sensor@48 {
> +                  compatible = "aptina,mt9v111";
> +                  reg = <0x48>;
> +
> +                  clocks = <&serializer2>;
> +
> +                  port {
> +                    sensor_2_out: endpoint {
> +                      remote-endpoint = <&ub913_2_in>;
> +                    };
> +                  };
> +                };
> +              };
> +            };
> +          }; /* End of link@1 */
> +        };
> +      };
> +    };
> +...
  

On 11/12/2022 19:58, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Thu, Dec 08, 2022 at 12:40:03PM +0200, Tomi Valkeinen wrote:
>> Add DT bindings for TI DS90UB960 FPDLink-3 Deserializer.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   .../bindings/media/i2c/ti,ds90ub960.yaml      | 358 ++++++++++++++++++
>>   1 file changed, 358 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
>> new file mode 100644
>> index 000000000000..d8b5e219d420
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
>> @@ -0,0 +1,358 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/media/i2c/ti,ds90ub960.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Texas Instruments DS90UB9XX Family FPD-Link Deserializer Hubs
>> +
>> +maintainers:
>> +  - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> +
>> +description:
>> +  The TI DS90UB9XX devices are FPD-Link video deserializers with I2C and GPIO
>> +  forwarding.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - ti,ds90ub960-q1
>> +      - ti,ds90ub9702-q1
>> +
>> +  reg:
>> +    maxItems: 1
>> +    description:
>> +      i2c addresses for the deserializer and the serializers
> 
> s/i2c/I2C/
> 
> Same below.
> 
> A bit more details would be nice, for instance the order in which
> addresses should be specified should be documented. The example below
> has one address only, so it's quite unclear. Or is this a left-over,
> from before the i2c-alias-pool ?

That's a left over, but not related to i2c-alias-pool but the i2c-alias 
for the serializers. It already says above 'maxItems: 1', so now it only 
contains the deserializer address. I'll drop the desc.

>> +
>> +  clocks:
>> +    maxItems: 1
>> +    description:
>> +      Reference clock connected to the REFCLK pin.
>> +
>> +  clock-names:
>> +    items:
>> +      - const: refclk
>> +
>> +  powerdown-gpios:
>> +    maxItems: 1
>> +    description:
>> +      Specifier for the GPIO connected to the PDB pin.
>> +
>> +  i2c-alias-pool:
>> +    $ref: /schemas/types.yaml#/definitions/uint16-array
>> +    description:
>> +      i2c alias pool is a pool of i2c addresses on the main i2c bus that can be
>> +      used to access the remote peripherals. The addresses must be available,
>> +      not used by any other peripheral. Each remote peripheral is assigned an
>> +      alias from the pool, and transactions to that address will be forwarded
>> +      to the remote peripheral, with the address translated to the remote
>> +      peripheral's real address.
> 
> As this property is optional, should you describe what happens when it's
> not specified ?
> 
> I would also indicate that the pool doesn't cover the serializers, only
> the devices behind them.

Yep, I'll clarify these.

>> +
>> +  links:
>> +    type: object
>> +    additionalProperties: false
>> +
>> +    properties:
>> +      '#address-cells':
>> +        const: 1
>> +
>> +      '#size-cells':
>> +        const: 0
>> +
>> +      ti,manual-strobe:
>> +        type: boolean
>> +        description:
>> +          Enable manual strobe position and EQ level
>> +
>> +    patternProperties:
>> +      '^link@[0-9a-f]+$':
> 
> There can be up to 4 links only, right ? I would then use
> 
>        '^link@[0-3]$':

Yes, I'll change that.

>> +        type: object
>> +        additionalProperties: false
>> +        properties:
>> +          reg:
>> +            description: The link number
>> +            maxItems: 1
>> +
>> +          i2c-alias:
>> +            description:
>> +              The i2c address used for the serializer. Transactions to this
>> +              address on the i2c bus where the deserializer resides are
>> +              forwarded to the serializer.
>> +
>> +          ti,rx-mode:
>> +            $ref: /schemas/types.yaml#/definitions/uint32
>> +            enum:
>> +              - 0 # RAW10
>> +              - 1 # RAW12 HF
>> +              - 2 # RAW12 LF
>> +              - 3 # CSI2 SYNC
>> +              - 4 # CSI2 NON-SYNC
>> +            description: FPD-Link Input Mode
> 
> Are there use cases for controlling this dynamically (in particular the
> sync/non-sync modes) ? Is there anything that could be queried at
> runtime from the serializers instead of being specified in DT ?

We need a link to the serializer before we can query anything from the 
serializer. To have a link, we need the mode... So, as I mentioned in 
the other reply, we could define these in some way in the serializer's 
properties instead of here, but I'm not sure if that's a good change.

The driver can change the mode at runtime (say, from sync to non-sync 
mode, if the HW supports that). But I think this property should reflect 
the HW strapped configuration of the serializer.

> Same question for the parameters below. Additionally, are there any
> parameters that need to be identical for all links ?

The same answer to the cdr-mode. No need to be identical.

The strobe-pos and eq-level are unrelated to this topic.

>> +
>> +          ti,cdr-mode:
>> +            $ref: /schemas/types.yaml#/definitions/uint32
>> +            enum:
>> +              - 0 # FPD3
>> +              - 1 # FPD4
>> +            description: FPD-Link CDR Mode
>> +
>> +          ti,strobe-pos:
>> +            $ref: /schemas/types.yaml#/definitions/int32
>> +            minimum: -13
>> +            maximum: 13
>> +            description: Manual strobe position
>> +
>> +          ti,eq-level:
>> +            $ref: /schemas/types.yaml#/definitions/uint32
>> +            maximum: 14
>> +            description: Manual EQ level
>> +
>> +          serializer:
>> +            type: object
>> +            description: FPD-Link Serializer node
>> +
>> +        required:
>> +          - reg
>> +          - i2c-alias
>> +          - ti,rx-mode
>> +          - serializer
>> +
>> +  ports:
>> +    $ref: /schemas/graph.yaml#/properties/ports
>> +
>> +    properties:
>> +      port@0:
>> +        $ref: /schemas/graph.yaml#/properties/port
>> +        unevaluatedProperties: false
>> +        description: FPD-Link input 0
>> +
>> +      port@1:
>> +        $ref: /schemas/graph.yaml#/properties/port
>> +        unevaluatedProperties: false
>> +        description: FPD-Link input 1
>> +
>> +      port@2:
>> +        $ref: /schemas/graph.yaml#/properties/port
>> +        unevaluatedProperties: false
>> +        description: FPD-Link input 2
>> +
>> +      port@3:
>> +        $ref: /schemas/graph.yaml#/properties/port
>> +        unevaluatedProperties: false
>> +        description: FPD-Link input 3
>> +
>> +      port@4:
>> +        $ref: /schemas/graph.yaml#/$defs/port-base
>> +        unevaluatedProperties: false
>> +        description: CSI-2 Output 0
>> +
>> +        properties:
>> +          endpoint:
>> +            $ref: /schemas/media/video-interfaces.yaml#
>> +            unevaluatedProperties: false
>> +
>> +            properties:
>> +              data-lanes:
>> +                minItems: 1
>> +                maxItems: 4
>> +
>> +      port@5:
>> +        $ref: /schemas/graph.yaml#/$defs/port-base
>> +        unevaluatedProperties: false
>> +        description: CSI-2 Output 1
>> +
>> +        properties:
>> +          endpoint:
>> +            $ref: /schemas/media/video-interfaces.yaml#
>> +            unevaluatedProperties: false
>> +
>> +            properties:
>> +              data-lanes:
>> +                minItems: 1
>> +                maxItems: 4
> 
> The ports should be mandatory, shouldn't they ?

Did you mean data-lanes? Yes, data-lanes should be mandatory.

  Tomi
  

Hi Tomi,

On Tue, Dec 13, 2022 at 04:25:46PM +0200, Tomi Valkeinen wrote:
> On 11/12/2022 19:58, Laurent Pinchart wrote:
> > On Thu, Dec 08, 2022 at 12:40:03PM +0200, Tomi Valkeinen wrote:
> >> Add DT bindings for TI DS90UB960 FPDLink-3 Deserializer.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >> ---
> >>   .../bindings/media/i2c/ti,ds90ub960.yaml      | 358 ++++++++++++++++++
> >>   1 file changed, 358 insertions(+)
> >>   create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
> >> new file mode 100644
> >> index 000000000000..d8b5e219d420
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
> >> @@ -0,0 +1,358 @@
> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/media/i2c/ti,ds90ub960.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Texas Instruments DS90UB9XX Family FPD-Link Deserializer Hubs
> >> +
> >> +maintainers:
> >> +  - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >> +
> >> +description:
> >> +  The TI DS90UB9XX devices are FPD-Link video deserializers with I2C and GPIO
> >> +  forwarding.
> >> +
> >> +properties:
> >> +  compatible:
> >> +    enum:
> >> +      - ti,ds90ub960-q1
> >> +      - ti,ds90ub9702-q1
> >> +
> >> +  reg:
> >> +    maxItems: 1
> >> +    description:
> >> +      i2c addresses for the deserializer and the serializers
> > 
> > s/i2c/I2C/
> > 
> > Same below.
> > 
> > A bit more details would be nice, for instance the order in which
> > addresses should be specified should be documented. The example below
> > has one address only, so it's quite unclear. Or is this a left-over,
> > from before the i2c-alias-pool ?
> 
> That's a left over, but not related to i2c-alias-pool but the i2c-alias 
> for the serializers. It already says above 'maxItems: 1', so now it only 
> contains the deserializer address. I'll drop the desc.

Looks good to me.

> >> +
> >> +  clocks:
> >> +    maxItems: 1
> >> +    description:
> >> +      Reference clock connected to the REFCLK pin.
> >> +
> >> +  clock-names:
> >> +    items:
> >> +      - const: refclk
> >> +
> >> +  powerdown-gpios:
> >> +    maxItems: 1
> >> +    description:
> >> +      Specifier for the GPIO connected to the PDB pin.
> >> +
> >> +  i2c-alias-pool:
> >> +    $ref: /schemas/types.yaml#/definitions/uint16-array
> >> +    description:
> >> +      i2c alias pool is a pool of i2c addresses on the main i2c bus that can be
> >> +      used to access the remote peripherals. The addresses must be available,
> >> +      not used by any other peripheral. Each remote peripheral is assigned an
> >> +      alias from the pool, and transactions to that address will be forwarded
> >> +      to the remote peripheral, with the address translated to the remote
> >> +      peripheral's real address.
> > 
> > As this property is optional, should you describe what happens when it's
> > not specified ?
> > 
> > I would also indicate that the pool doesn't cover the serializers, only
> > the devices behind them.
> 
> Yep, I'll clarify these.
> 
> >> +
> >> +  links:
> >> +    type: object
> >> +    additionalProperties: false
> >> +
> >> +    properties:
> >> +      '#address-cells':
> >> +        const: 1
> >> +
> >> +      '#size-cells':
> >> +        const: 0
> >> +
> >> +      ti,manual-strobe:
> >> +        type: boolean
> >> +        description:
> >> +          Enable manual strobe position and EQ level
> >> +
> >> +    patternProperties:
> >> +      '^link@[0-9a-f]+$':
> > 
> > There can be up to 4 links only, right ? I would then use
> > 
> >        '^link@[0-3]$':
> 
> Yes, I'll change that.
> 
> >> +        type: object
> >> +        additionalProperties: false
> >> +        properties:
> >> +          reg:
> >> +            description: The link number
> >> +            maxItems: 1
> >> +
> >> +          i2c-alias:
> >> +            description:
> >> +              The i2c address used for the serializer. Transactions to this
> >> +              address on the i2c bus where the deserializer resides are
> >> +              forwarded to the serializer.
> >> +
> >> +          ti,rx-mode:
> >> +            $ref: /schemas/types.yaml#/definitions/uint32
> >> +            enum:
> >> +              - 0 # RAW10
> >> +              - 1 # RAW12 HF
> >> +              - 2 # RAW12 LF
> >> +              - 3 # CSI2 SYNC
> >> +              - 4 # CSI2 NON-SYNC
> >> +            description: FPD-Link Input Mode
> > 
> > Are there use cases for controlling this dynamically (in particular the
> > sync/non-sync modes) ? Is there anything that could be queried at
> > runtime from the serializers instead of being specified in DT ?
> 
> We need a link to the serializer before we can query anything from the 
> serializer.

I meant querying it from the serializer driver, not the serializer
hardware. This being said, it would likely be difficult to do so, as the
serializer driver would need to probe first. I think I'm thus fine
selecting the mode in DT on the deserializer side.

> To have a link, we need the mode... So, as I mentioned in 
> the other reply, we could define these in some way in the serializer's 
> properties instead of here, but I'm not sure if that's a good change.
> 
> The driver can change the mode at runtime (say, from sync to non-sync 
> mode, if the HW supports that). But I think this property should reflect 
> the HW strapped configuration of the serializer.

That would possibly work for the DS90UB953, but the DS90UB913 has no
strapped mode selected at boot time but is instead configured
automatically through the back-channel (see my last reply to patch 3/8).

When connecting a DS90UB913 to a DS90UB914 deserializer, we can probably
start without mode selection in software, as the MODE pin is meant to
bootstrap that to a correct value which is then automatically
transmitted to the serializer (hardware designs where the mode would
need to be overridden should be rate). However, when connecting multiple
DS90UB913 to a DS90UB960, I can imagine connecting different types of
cameras on the four input ports, so the need to specify the mode
per-port in DT would be more common.

For these reasons, I don't think the ti,rx-mode property can be defined
as reflecting the hardware MODE strap with the DS90UB913. I also think
it would be quite confusing to define it as the desired runtime
configuration for the DS90UB913 and as the hardware MODE strap for the
DS90UB953. Could it be (explicitly) defined as the desired runtime
configuration in all cases ?

> > Same question for the parameters below. Additionally, are there any
> > parameters that need to be identical for all links ?
> 
> The same answer to the cdr-mode. No need to be identical.
> 
> The strobe-pos and eq-level are unrelated to this topic.
> 
> >> +
> >> +          ti,cdr-mode:
> >> +            $ref: /schemas/types.yaml#/definitions/uint32
> >> +            enum:
> >> +              - 0 # FPD3
> >> +              - 1 # FPD4
> >> +            description: FPD-Link CDR Mode
> >> +
> >> +          ti,strobe-pos:
> >> +            $ref: /schemas/types.yaml#/definitions/int32
> >> +            minimum: -13
> >> +            maximum: 13
> >> +            description: Manual strobe position
> >> +
> >> +          ti,eq-level:
> >> +            $ref: /schemas/types.yaml#/definitions/uint32
> >> +            maximum: 14
> >> +            description: Manual EQ level
> >> +
> >> +          serializer:
> >> +            type: object
> >> +            description: FPD-Link Serializer node
> >> +
> >> +        required:
> >> +          - reg
> >> +          - i2c-alias
> >> +          - ti,rx-mode
> >> +          - serializer
> >> +
> >> +  ports:
> >> +    $ref: /schemas/graph.yaml#/properties/ports
> >> +
> >> +    properties:
> >> +      port@0:
> >> +        $ref: /schemas/graph.yaml#/properties/port
> >> +        unevaluatedProperties: false
> >> +        description: FPD-Link input 0
> >> +
> >> +      port@1:
> >> +        $ref: /schemas/graph.yaml#/properties/port
> >> +        unevaluatedProperties: false
> >> +        description: FPD-Link input 1
> >> +
> >> +      port@2:
> >> +        $ref: /schemas/graph.yaml#/properties/port
> >> +        unevaluatedProperties: false
> >> +        description: FPD-Link input 2
> >> +
> >> +      port@3:
> >> +        $ref: /schemas/graph.yaml#/properties/port
> >> +        unevaluatedProperties: false
> >> +        description: FPD-Link input 3
> >> +
> >> +      port@4:
> >> +        $ref: /schemas/graph.yaml#/$defs/port-base
> >> +        unevaluatedProperties: false
> >> +        description: CSI-2 Output 0
> >> +
> >> +        properties:
> >> +          endpoint:
> >> +            $ref: /schemas/media/video-interfaces.yaml#
> >> +            unevaluatedProperties: false
> >> +
> >> +            properties:
> >> +              data-lanes:
> >> +                minItems: 1
> >> +                maxItems: 4
> >> +
> >> +      port@5:
> >> +        $ref: /schemas/graph.yaml#/$defs/port-base
> >> +        unevaluatedProperties: false
> >> +        description: CSI-2 Output 1
> >> +
> >> +        properties:
> >> +          endpoint:
> >> +            $ref: /schemas/media/video-interfaces.yaml#
> >> +            unevaluatedProperties: false
> >> +
> >> +            properties:
> >> +              data-lanes:
> >> +                minItems: 1
> >> +                maxItems: 4
> > 
> > The ports should be mandatory, shouldn't they ?
> 
> Did you mean data-lanes? Yes, data-lanes should be mandatory.

Yes that's what I meant, sorry.
  

On 26/12/2022 18:52, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Tue, Dec 13, 2022 at 04:25:46PM +0200, Tomi Valkeinen wrote:
>> On 11/12/2022 19:58, Laurent Pinchart wrote:
>>> On Thu, Dec 08, 2022 at 12:40:03PM +0200, Tomi Valkeinen wrote:
>>>> Add DT bindings for TI DS90UB960 FPDLink-3 Deserializer.
>>>>
>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>> ---
>>>>    .../bindings/media/i2c/ti,ds90ub960.yaml      | 358 ++++++++++++++++++
>>>>    1 file changed, 358 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
>>>> new file mode 100644
>>>> index 000000000000..d8b5e219d420
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
>>>> @@ -0,0 +1,358 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/media/i2c/ti,ds90ub960.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Texas Instruments DS90UB9XX Family FPD-Link Deserializer Hubs
>>>> +
>>>> +maintainers:
>>>> +  - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>> +
>>>> +description:
>>>> +  The TI DS90UB9XX devices are FPD-Link video deserializers with I2C and GPIO
>>>> +  forwarding.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    enum:
>>>> +      - ti,ds90ub960-q1
>>>> +      - ti,ds90ub9702-q1
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +    description:
>>>> +      i2c addresses for the deserializer and the serializers
>>>
>>> s/i2c/I2C/
>>>
>>> Same below.
>>>
>>> A bit more details would be nice, for instance the order in which
>>> addresses should be specified should be documented. The example below
>>> has one address only, so it's quite unclear. Or is this a left-over,
>>> from before the i2c-alias-pool ?
>>
>> That's a left over, but not related to i2c-alias-pool but the i2c-alias
>> for the serializers. It already says above 'maxItems: 1', so now it only
>> contains the deserializer address. I'll drop the desc.
> 
> Looks good to me.
> 
>>>> +
>>>> +  clocks:
>>>> +    maxItems: 1
>>>> +    description:
>>>> +      Reference clock connected to the REFCLK pin.
>>>> +
>>>> +  clock-names:
>>>> +    items:
>>>> +      - const: refclk
>>>> +
>>>> +  powerdown-gpios:
>>>> +    maxItems: 1
>>>> +    description:
>>>> +      Specifier for the GPIO connected to the PDB pin.
>>>> +
>>>> +  i2c-alias-pool:
>>>> +    $ref: /schemas/types.yaml#/definitions/uint16-array
>>>> +    description:
>>>> +      i2c alias pool is a pool of i2c addresses on the main i2c bus that can be
>>>> +      used to access the remote peripherals. The addresses must be available,
>>>> +      not used by any other peripheral. Each remote peripheral is assigned an
>>>> +      alias from the pool, and transactions to that address will be forwarded
>>>> +      to the remote peripheral, with the address translated to the remote
>>>> +      peripheral's real address.
>>>
>>> As this property is optional, should you describe what happens when it's
>>> not specified ?
>>>
>>> I would also indicate that the pool doesn't cover the serializers, only
>>> the devices behind them.
>>
>> Yep, I'll clarify these.
>>
>>>> +
>>>> +  links:
>>>> +    type: object
>>>> +    additionalProperties: false
>>>> +
>>>> +    properties:
>>>> +      '#address-cells':
>>>> +        const: 1
>>>> +
>>>> +      '#size-cells':
>>>> +        const: 0
>>>> +
>>>> +      ti,manual-strobe:
>>>> +        type: boolean
>>>> +        description:
>>>> +          Enable manual strobe position and EQ level
>>>> +
>>>> +    patternProperties:
>>>> +      '^link@[0-9a-f]+$':
>>>
>>> There can be up to 4 links only, right ? I would then use
>>>
>>>         '^link@[0-3]$':
>>
>> Yes, I'll change that.
>>
>>>> +        type: object
>>>> +        additionalProperties: false
>>>> +        properties:
>>>> +          reg:
>>>> +            description: The link number
>>>> +            maxItems: 1
>>>> +
>>>> +          i2c-alias:
>>>> +            description:
>>>> +              The i2c address used for the serializer. Transactions to this
>>>> +              address on the i2c bus where the deserializer resides are
>>>> +              forwarded to the serializer.
>>>> +
>>>> +          ti,rx-mode:
>>>> +            $ref: /schemas/types.yaml#/definitions/uint32
>>>> +            enum:
>>>> +              - 0 # RAW10
>>>> +              - 1 # RAW12 HF
>>>> +              - 2 # RAW12 LF
>>>> +              - 3 # CSI2 SYNC
>>>> +              - 4 # CSI2 NON-SYNC
>>>> +            description: FPD-Link Input Mode
>>>
>>> Are there use cases for controlling this dynamically (in particular the
>>> sync/non-sync modes) ? Is there anything that could be queried at
>>> runtime from the serializers instead of being specified in DT ?
>>
>> We need a link to the serializer before we can query anything from the
>> serializer.
> 
> I meant querying it from the serializer driver, not the serializer
> hardware. This being said, it would likely be difficult to do so, as the
> serializer driver would need to probe first. I think I'm thus fine
> selecting the mode in DT on the deserializer side.
> 
>> To have a link, we need the mode... So, as I mentioned in
>> the other reply, we could define these in some way in the serializer's
>> properties instead of here, but I'm not sure if that's a good change.
>>
>> The driver can change the mode at runtime (say, from sync to non-sync
>> mode, if the HW supports that). But I think this property should reflect
>> the HW strapped configuration of the serializer.
> 
> That would possibly work for the DS90UB953, but the DS90UB913 has no
> strapped mode selected at boot time but is instead configured
> automatically through the back-channel (see my last reply to patch 3/8).

Indeed.

> When connecting a DS90UB913 to a DS90UB914 deserializer, we can probably
> start without mode selection in software, as the MODE pin is meant to
> bootstrap that to a correct value which is then automatically
> transmitted to the serializer (hardware designs where the mode would
> need to be overridden should be rate). However, when connecting multiple

I don't know if that's true. I guess it depends on how you see the deser 
and the camera module. Are they part of the same HW design or not? In my 
setups they are quite separate, and I connect different kinds of camera 
modules to my deserializers. But I can see that if you create a, say, 
car, you'd have both sides known at design time and would never change.

> DS90UB913 to a DS90UB960, I can imagine connecting different types of
> cameras on the four input ports, so the need to specify the mode
> per-port in DT would be more common.

Right, and even with UB914, you might well design the deserializer side 
with, say, RAW10 sensors, but later in the cycle you'd need to change to 
a RAW12 sensor. Depending on the deser mode strap would require you to 
do a HW change on the deser side too.

As I said in the other mail, I don't like the deser's strap, and I think 
we should just basically ignore it as we can provide the necessary data 
in the DT.

> For these reasons, I don't think the ti,rx-mode property can be defined
> as reflecting the hardware MODE strap with the DS90UB913. I also think
> it would be quite confusing to define it as the desired runtime
> configuration for the DS90UB913 and as the hardware MODE strap for the
> DS90UB953. Could it be (explicitly) defined as the desired runtime
> configuration in all cases ?

That sounds bad in a DT context =). You're right that the rx-mode can't 
be defined as reflecting the serializer mode strap, but I think we can 
define it as reflecting the default operation mode of the serializer 
hardware (or maybe rather the camera module).

  Tomi
  

Hi Tomi,

On Wed, Jan 04, 2023 at 10:59:00AM +0200, Tomi Valkeinen wrote:
> On 26/12/2022 18:52, Laurent Pinchart wrote:
> > On Tue, Dec 13, 2022 at 04:25:46PM +0200, Tomi Valkeinen wrote:
> >> On 11/12/2022 19:58, Laurent Pinchart wrote:
> >>> On Thu, Dec 08, 2022 at 12:40:03PM +0200, Tomi Valkeinen wrote:
> >>>> Add DT bindings for TI DS90UB960 FPDLink-3 Deserializer.
> >>>>
> >>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >>>> ---
> >>>>    .../bindings/media/i2c/ti,ds90ub960.yaml      | 358 ++++++++++++++++++
> >>>>    1 file changed, 358 insertions(+)
> >>>>    create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
> >>>> new file mode 100644
> >>>> index 000000000000..d8b5e219d420
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
> >>>> @@ -0,0 +1,358 @@
> >>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>>> +%YAML 1.2
> >>>> +---
> >>>> +$id: http://devicetree.org/schemas/media/i2c/ti,ds90ub960.yaml#
> >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>> +
> >>>> +title: Texas Instruments DS90UB9XX Family FPD-Link Deserializer Hubs
> >>>> +
> >>>> +maintainers:
> >>>> +  - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >>>> +
> >>>> +description:
> >>>> +  The TI DS90UB9XX devices are FPD-Link video deserializers with I2C and GPIO
> >>>> +  forwarding.
> >>>> +
> >>>> +properties:
> >>>> +  compatible:
> >>>> +    enum:
> >>>> +      - ti,ds90ub960-q1
> >>>> +      - ti,ds90ub9702-q1
> >>>> +
> >>>> +  reg:
> >>>> +    maxItems: 1
> >>>> +    description:
> >>>> +      i2c addresses for the deserializer and the serializers
> >>>
> >>> s/i2c/I2C/
> >>>
> >>> Same below.
> >>>
> >>> A bit more details would be nice, for instance the order in which
> >>> addresses should be specified should be documented. The example below
> >>> has one address only, so it's quite unclear. Or is this a left-over,
> >>> from before the i2c-alias-pool ?
> >>
> >> That's a left over, but not related to i2c-alias-pool but the i2c-alias
> >> for the serializers. It already says above 'maxItems: 1', so now it only
> >> contains the deserializer address. I'll drop the desc.
> > 
> > Looks good to me.
> > 
> >>>> +
> >>>> +  clocks:
> >>>> +    maxItems: 1
> >>>> +    description:
> >>>> +      Reference clock connected to the REFCLK pin.
> >>>> +
> >>>> +  clock-names:
> >>>> +    items:
> >>>> +      - const: refclk
> >>>> +
> >>>> +  powerdown-gpios:
> >>>> +    maxItems: 1
> >>>> +    description:
> >>>> +      Specifier for the GPIO connected to the PDB pin.
> >>>> +
> >>>> +  i2c-alias-pool:
> >>>> +    $ref: /schemas/types.yaml#/definitions/uint16-array
> >>>> +    description:
> >>>> +      i2c alias pool is a pool of i2c addresses on the main i2c bus that can be
> >>>> +      used to access the remote peripherals. The addresses must be available,
> >>>> +      not used by any other peripheral. Each remote peripheral is assigned an
> >>>> +      alias from the pool, and transactions to that address will be forwarded
> >>>> +      to the remote peripheral, with the address translated to the remote
> >>>> +      peripheral's real address.
> >>>
> >>> As this property is optional, should you describe what happens when it's
> >>> not specified ?
> >>>
> >>> I would also indicate that the pool doesn't cover the serializers, only
> >>> the devices behind them.
> >>
> >> Yep, I'll clarify these.
> >>
> >>>> +
> >>>> +  links:
> >>>> +    type: object
> >>>> +    additionalProperties: false
> >>>> +
> >>>> +    properties:
> >>>> +      '#address-cells':
> >>>> +        const: 1
> >>>> +
> >>>> +      '#size-cells':
> >>>> +        const: 0
> >>>> +
> >>>> +      ti,manual-strobe:
> >>>> +        type: boolean
> >>>> +        description:
> >>>> +          Enable manual strobe position and EQ level
> >>>> +
> >>>> +    patternProperties:
> >>>> +      '^link@[0-9a-f]+$':
> >>>
> >>> There can be up to 4 links only, right ? I would then use
> >>>
> >>>         '^link@[0-3]$':
> >>
> >> Yes, I'll change that.
> >>
> >>>> +        type: object
> >>>> +        additionalProperties: false
> >>>> +        properties:
> >>>> +          reg:
> >>>> +            description: The link number
> >>>> +            maxItems: 1
> >>>> +
> >>>> +          i2c-alias:
> >>>> +            description:
> >>>> +              The i2c address used for the serializer. Transactions to this
> >>>> +              address on the i2c bus where the deserializer resides are
> >>>> +              forwarded to the serializer.
> >>>> +
> >>>> +          ti,rx-mode:
> >>>> +            $ref: /schemas/types.yaml#/definitions/uint32
> >>>> +            enum:
> >>>> +              - 0 # RAW10
> >>>> +              - 1 # RAW12 HF
> >>>> +              - 2 # RAW12 LF
> >>>> +              - 3 # CSI2 SYNC
> >>>> +              - 4 # CSI2 NON-SYNC
> >>>> +            description: FPD-Link Input Mode
> >>>
> >>> Are there use cases for controlling this dynamically (in particular the
> >>> sync/non-sync modes) ? Is there anything that could be queried at
> >>> runtime from the serializers instead of being specified in DT ?
> >>
> >> We need a link to the serializer before we can query anything from the
> >> serializer.
> > 
> > I meant querying it from the serializer driver, not the serializer
> > hardware. This being said, it would likely be difficult to do so, as the
> > serializer driver would need to probe first. I think I'm thus fine
> > selecting the mode in DT on the deserializer side.
> > 
> >> To have a link, we need the mode... So, as I mentioned in
> >> the other reply, we could define these in some way in the serializer's
> >> properties instead of here, but I'm not sure if that's a good change.
> >>
> >> The driver can change the mode at runtime (say, from sync to non-sync
> >> mode, if the HW supports that). But I think this property should reflect
> >> the HW strapped configuration of the serializer.
> > 
> > That would possibly work for the DS90UB953, but the DS90UB913 has no
> > strapped mode selected at boot time but is instead configured
> > automatically through the back-channel (see my last reply to patch 3/8).
> 
> Indeed.
> 
> > When connecting a DS90UB913 to a DS90UB914 deserializer, we can probably
> > start without mode selection in software, as the MODE pin is meant to
> > bootstrap that to a correct value which is then automatically
> > transmitted to the serializer (hardware designs where the mode would
> > need to be overridden should be rate). However, when connecting multiple
> 
> I don't know if that's true. I guess it depends on how you see the deser 
> and the camera module. Are they part of the same HW design or not? In my 
> setups they are quite separate, and I connect different kinds of camera 
> modules to my deserializers. But I can see that if you create a, say, 
> car, you'd have both sides known at design time and would never change.
> 
> > DS90UB913 to a DS90UB960, I can imagine connecting different types of
> > cameras on the four input ports, so the need to specify the mode
> > per-port in DT would be more common.
> 
> Right, and even with UB914, you might well design the deserializer side 
> with, say, RAW10 sensors, but later in the cycle you'd need to change to 
> a RAW12 sensor. Depending on the deser mode strap would require you to 
> do a HW change on the deser side too.
> 
> As I said in the other mail, I don't like the deser's strap, and I think 
> we should just basically ignore it as we can provide the necessary data 
> in the DT.

What I meant is that, given that the UB914 is meant to be used with a
single camera, using a RAW mode, there's a much higher chance that
hardware strapping will work as intended there. We could thus start
without support for overrides in a UB914 driver (but as far as I
understand we're not planning to work on such a driver in the near
future, so it's hypothetical only), while in the UB960 driver we
probably need override support from the beginning.

> > For these reasons, I don't think the ti,rx-mode property can be defined
> > as reflecting the hardware MODE strap with the DS90UB913. I also think
> > it would be quite confusing to define it as the desired runtime
> > configuration for the DS90UB913 and as the hardware MODE strap for the
> > DS90UB953. Could it be (explicitly) defined as the desired runtime
> > configuration in all cases ?
> 
> That sounds bad in a DT context =). You're right that the rx-mode can't 
> be defined as reflecting the serializer mode strap, but I think we can 
> define it as reflecting the default operation mode of the serializer 
> hardware (or maybe rather the camera module).

What do you mean by "default operation mode" in this case ?
  

On 04/01/2023 14:57, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Wed, Jan 04, 2023 at 10:59:00AM +0200, Tomi Valkeinen wrote:
>> On 26/12/2022 18:52, Laurent Pinchart wrote:
>>> On Tue, Dec 13, 2022 at 04:25:46PM +0200, Tomi Valkeinen wrote:
>>>> On 11/12/2022 19:58, Laurent Pinchart wrote:
>>>>> On Thu, Dec 08, 2022 at 12:40:03PM +0200, Tomi Valkeinen wrote:
>>>>>> Add DT bindings for TI DS90UB960 FPDLink-3 Deserializer.
>>>>>>
>>>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>>>> ---
>>>>>>     .../bindings/media/i2c/ti,ds90ub960.yaml      | 358 ++++++++++++++++++
>>>>>>     1 file changed, 358 insertions(+)
>>>>>>     create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
>>>>>> new file mode 100644
>>>>>> index 000000000000..d8b5e219d420
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
>>>>>> @@ -0,0 +1,358 @@
>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>> +%YAML 1.2
>>>>>> +---
>>>>>> +$id: http://devicetree.org/schemas/media/i2c/ti,ds90ub960.yaml#
>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>> +
>>>>>> +title: Texas Instruments DS90UB9XX Family FPD-Link Deserializer Hubs
>>>>>> +
>>>>>> +maintainers:
>>>>>> +  - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>>>> +
>>>>>> +description:
>>>>>> +  The TI DS90UB9XX devices are FPD-Link video deserializers with I2C and GPIO
>>>>>> +  forwarding.
>>>>>> +
>>>>>> +properties:
>>>>>> +  compatible:
>>>>>> +    enum:
>>>>>> +      - ti,ds90ub960-q1
>>>>>> +      - ti,ds90ub9702-q1
>>>>>> +
>>>>>> +  reg:
>>>>>> +    maxItems: 1
>>>>>> +    description:
>>>>>> +      i2c addresses for the deserializer and the serializers
>>>>>
>>>>> s/i2c/I2C/
>>>>>
>>>>> Same below.
>>>>>
>>>>> A bit more details would be nice, for instance the order in which
>>>>> addresses should be specified should be documented. The example below
>>>>> has one address only, so it's quite unclear. Or is this a left-over,
>>>>> from before the i2c-alias-pool ?
>>>>
>>>> That's a left over, but not related to i2c-alias-pool but the i2c-alias
>>>> for the serializers. It already says above 'maxItems: 1', so now it only
>>>> contains the deserializer address. I'll drop the desc.
>>>
>>> Looks good to me.
>>>
>>>>>> +
>>>>>> +  clocks:
>>>>>> +    maxItems: 1
>>>>>> +    description:
>>>>>> +      Reference clock connected to the REFCLK pin.
>>>>>> +
>>>>>> +  clock-names:
>>>>>> +    items:
>>>>>> +      - const: refclk
>>>>>> +
>>>>>> +  powerdown-gpios:
>>>>>> +    maxItems: 1
>>>>>> +    description:
>>>>>> +      Specifier for the GPIO connected to the PDB pin.
>>>>>> +
>>>>>> +  i2c-alias-pool:
>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint16-array
>>>>>> +    description:
>>>>>> +      i2c alias pool is a pool of i2c addresses on the main i2c bus that can be
>>>>>> +      used to access the remote peripherals. The addresses must be available,
>>>>>> +      not used by any other peripheral. Each remote peripheral is assigned an
>>>>>> +      alias from the pool, and transactions to that address will be forwarded
>>>>>> +      to the remote peripheral, with the address translated to the remote
>>>>>> +      peripheral's real address.
>>>>>
>>>>> As this property is optional, should you describe what happens when it's
>>>>> not specified ?
>>>>>
>>>>> I would also indicate that the pool doesn't cover the serializers, only
>>>>> the devices behind them.
>>>>
>>>> Yep, I'll clarify these.
>>>>
>>>>>> +
>>>>>> +  links:
>>>>>> +    type: object
>>>>>> +    additionalProperties: false
>>>>>> +
>>>>>> +    properties:
>>>>>> +      '#address-cells':
>>>>>> +        const: 1
>>>>>> +
>>>>>> +      '#size-cells':
>>>>>> +        const: 0
>>>>>> +
>>>>>> +      ti,manual-strobe:
>>>>>> +        type: boolean
>>>>>> +        description:
>>>>>> +          Enable manual strobe position and EQ level
>>>>>> +
>>>>>> +    patternProperties:
>>>>>> +      '^link@[0-9a-f]+$':
>>>>>
>>>>> There can be up to 4 links only, right ? I would then use
>>>>>
>>>>>          '^link@[0-3]$':
>>>>
>>>> Yes, I'll change that.
>>>>
>>>>>> +        type: object
>>>>>> +        additionalProperties: false
>>>>>> +        properties:
>>>>>> +          reg:
>>>>>> +            description: The link number
>>>>>> +            maxItems: 1
>>>>>> +
>>>>>> +          i2c-alias:
>>>>>> +            description:
>>>>>> +              The i2c address used for the serializer. Transactions to this
>>>>>> +              address on the i2c bus where the deserializer resides are
>>>>>> +              forwarded to the serializer.
>>>>>> +
>>>>>> +          ti,rx-mode:
>>>>>> +            $ref: /schemas/types.yaml#/definitions/uint32
>>>>>> +            enum:
>>>>>> +              - 0 # RAW10
>>>>>> +              - 1 # RAW12 HF
>>>>>> +              - 2 # RAW12 LF
>>>>>> +              - 3 # CSI2 SYNC
>>>>>> +              - 4 # CSI2 NON-SYNC
>>>>>> +            description: FPD-Link Input Mode
>>>>>
>>>>> Are there use cases for controlling this dynamically (in particular the
>>>>> sync/non-sync modes) ? Is there anything that could be queried at
>>>>> runtime from the serializers instead of being specified in DT ?
>>>>
>>>> We need a link to the serializer before we can query anything from the
>>>> serializer.
>>>
>>> I meant querying it from the serializer driver, not the serializer
>>> hardware. This being said, it would likely be difficult to do so, as the
>>> serializer driver would need to probe first. I think I'm thus fine
>>> selecting the mode in DT on the deserializer side.
>>>
>>>> To have a link, we need the mode... So, as I mentioned in
>>>> the other reply, we could define these in some way in the serializer's
>>>> properties instead of here, but I'm not sure if that's a good change.
>>>>
>>>> The driver can change the mode at runtime (say, from sync to non-sync
>>>> mode, if the HW supports that). But I think this property should reflect
>>>> the HW strapped configuration of the serializer.
>>>
>>> That would possibly work for the DS90UB953, but the DS90UB913 has no
>>> strapped mode selected at boot time but is instead configured
>>> automatically through the back-channel (see my last reply to patch 3/8).
>>
>> Indeed.
>>
>>> When connecting a DS90UB913 to a DS90UB914 deserializer, we can probably
>>> start without mode selection in software, as the MODE pin is meant to
>>> bootstrap that to a correct value which is then automatically
>>> transmitted to the serializer (hardware designs where the mode would
>>> need to be overridden should be rate). However, when connecting multiple
>>
>> I don't know if that's true. I guess it depends on how you see the deser
>> and the camera module. Are they part of the same HW design or not? In my
>> setups they are quite separate, and I connect different kinds of camera
>> modules to my deserializers. But I can see that if you create a, say,
>> car, you'd have both sides known at design time and would never change.
>>
>>> DS90UB913 to a DS90UB960, I can imagine connecting different types of
>>> cameras on the four input ports, so the need to specify the mode
>>> per-port in DT would be more common.
>>
>> Right, and even with UB914, you might well design the deserializer side
>> with, say, RAW10 sensors, but later in the cycle you'd need to change to
>> a RAW12 sensor. Depending on the deser mode strap would require you to
>> do a HW change on the deser side too.
>>
>> As I said in the other mail, I don't like the deser's strap, and I think
>> we should just basically ignore it as we can provide the necessary data
>> in the DT.
> 
> What I meant is that, given that the UB914 is meant to be used with a
> single camera, using a RAW mode, there's a much higher chance that
> hardware strapping will work as intended there. We could thus start
> without support for overrides in a UB914 driver (but as far as I
> understand we're not planning to work on such a driver in the near
> future, so it's hypothetical only), while in the UB960 driver we
> probably need override support from the beginning.

Yes, but depending on the UB914 strap mode would still be only a partial 
solution, and it would still need to also support overriding the strap 
mode (from DT). As you said in the other mail, we anyway have to define 
the serializer and the sensor in the DT, so even with UB914 we couldn't 
just switch the camera module and change the mode via a DIP switch.

So I still don't see why we would even bother supporting the deser strap 
mode, even on UB914.

>>> For these reasons, I don't think the ti,rx-mode property can be defined
>>> as reflecting the hardware MODE strap with the DS90UB913. I also think
>>> it would be quite confusing to define it as the desired runtime
>>> configuration for the DS90UB913 and as the hardware MODE strap for the
>>> DS90UB953. Could it be (explicitly) defined as the desired runtime
>>> configuration in all cases ?
>>
>> That sounds bad in a DT context =). You're right that the rx-mode can't
>> be defined as reflecting the serializer mode strap, but I think we can
>> define it as reflecting the default operation mode of the serializer
>> hardware (or maybe rather the camera module).
> 
> What do you mean by "default operation mode" in this case ?

How the camera module is HW strapped and supposed to be used, how it 
works (more or less) out of the box:

- The UB953 is HW strapped to sync or non-sync CSI mode, or RAW mode.
- Both UB913 and UB953 (in RAW mode) have a sensor connected either with 
10 or 12 lines.
- The sensor has a default (either HW or use-case default) pixel clock.

Based on those, I think we get all the possible modes. So, I think, 
every camera module will have a known "normal" / "default" mode, which 
is what the mode in the DT tells.

Well, for FPD4, there's also the CDR mode. UB971 can work in both FDP3 
and FPD4 modes. But there again it should be clear what's the "normal" 
operation mode, as it comes from a serializer HW strap.

We can do changes to those at runtime (not supported by the drivers), 
e.g. changing from sync to non-sync CSI mode, changing the RAW mode, and 
even changing from FDP4 to FDP3. But those, I believe, are rare cases, 
and the changes have to be done carefully, on both the deser and ser 
sides in certain order.

So, back to the original point, "desired runtime configuration" and 
"default operation mode" are maybe the same thing =).

  Tomi
  

Hi Tomi,

On Wed, Jan 04, 2023 at 04:05:08PM +0200, Tomi Valkeinen wrote:
> On 04/01/2023 14:57, Laurent Pinchart wrote:
> > On Wed, Jan 04, 2023 at 10:59:00AM +0200, Tomi Valkeinen wrote:
> >> On 26/12/2022 18:52, Laurent Pinchart wrote:
> >>> On Tue, Dec 13, 2022 at 04:25:46PM +0200, Tomi Valkeinen wrote:
> >>>> On 11/12/2022 19:58, Laurent Pinchart wrote:
> >>>>> On Thu, Dec 08, 2022 at 12:40:03PM +0200, Tomi Valkeinen wrote:
> >>>>>> Add DT bindings for TI DS90UB960 FPDLink-3 Deserializer.
> >>>>>>
> >>>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >>>>>> ---
> >>>>>>     .../bindings/media/i2c/ti,ds90ub960.yaml      | 358 ++++++++++++++++++
> >>>>>>     1 file changed, 358 insertions(+)
> >>>>>>     create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
> >>>>>>
> >>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
> >>>>>> new file mode 100644
> >>>>>> index 000000000000..d8b5e219d420
> >>>>>> --- /dev/null
> >>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
> >>>>>> @@ -0,0 +1,358 @@
> >>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>>>>> +%YAML 1.2
> >>>>>> +---
> >>>>>> +$id: http://devicetree.org/schemas/media/i2c/ti,ds90ub960.yaml#
> >>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>>> +
> >>>>>> +title: Texas Instruments DS90UB9XX Family FPD-Link Deserializer Hubs
> >>>>>> +
> >>>>>> +maintainers:
> >>>>>> +  - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >>>>>> +
> >>>>>> +description:
> >>>>>> +  The TI DS90UB9XX devices are FPD-Link video deserializers with I2C and GPIO
> >>>>>> +  forwarding.
> >>>>>> +
> >>>>>> +properties:
> >>>>>> +  compatible:
> >>>>>> +    enum:
> >>>>>> +      - ti,ds90ub960-q1
> >>>>>> +      - ti,ds90ub9702-q1
> >>>>>> +
> >>>>>> +  reg:
> >>>>>> +    maxItems: 1
> >>>>>> +    description:
> >>>>>> +      i2c addresses for the deserializer and the serializers
> >>>>>
> >>>>> s/i2c/I2C/
> >>>>>
> >>>>> Same below.
> >>>>>
> >>>>> A bit more details would be nice, for instance the order in which
> >>>>> addresses should be specified should be documented. The example below
> >>>>> has one address only, so it's quite unclear. Or is this a left-over,
> >>>>> from before the i2c-alias-pool ?
> >>>>
> >>>> That's a left over, but not related to i2c-alias-pool but the i2c-alias
> >>>> for the serializers. It already says above 'maxItems: 1', so now it only
> >>>> contains the deserializer address. I'll drop the desc.
> >>>
> >>> Looks good to me.
> >>>
> >>>>>> +
> >>>>>> +  clocks:
> >>>>>> +    maxItems: 1
> >>>>>> +    description:
> >>>>>> +      Reference clock connected to the REFCLK pin.
> >>>>>> +
> >>>>>> +  clock-names:
> >>>>>> +    items:
> >>>>>> +      - const: refclk
> >>>>>> +
> >>>>>> +  powerdown-gpios:
> >>>>>> +    maxItems: 1
> >>>>>> +    description:
> >>>>>> +      Specifier for the GPIO connected to the PDB pin.
> >>>>>> +
> >>>>>> +  i2c-alias-pool:
> >>>>>> +    $ref: /schemas/types.yaml#/definitions/uint16-array
> >>>>>> +    description:
> >>>>>> +      i2c alias pool is a pool of i2c addresses on the main i2c bus that can be
> >>>>>> +      used to access the remote peripherals. The addresses must be available,
> >>>>>> +      not used by any other peripheral. Each remote peripheral is assigned an
> >>>>>> +      alias from the pool, and transactions to that address will be forwarded
> >>>>>> +      to the remote peripheral, with the address translated to the remote
> >>>>>> +      peripheral's real address.
> >>>>>
> >>>>> As this property is optional, should you describe what happens when it's
> >>>>> not specified ?
> >>>>>
> >>>>> I would also indicate that the pool doesn't cover the serializers, only
> >>>>> the devices behind them.
> >>>>
> >>>> Yep, I'll clarify these.
> >>>>
> >>>>>> +
> >>>>>> +  links:
> >>>>>> +    type: object
> >>>>>> +    additionalProperties: false
> >>>>>> +
> >>>>>> +    properties:
> >>>>>> +      '#address-cells':
> >>>>>> +        const: 1
> >>>>>> +
> >>>>>> +      '#size-cells':
> >>>>>> +        const: 0
> >>>>>> +
> >>>>>> +      ti,manual-strobe:
> >>>>>> +        type: boolean
> >>>>>> +        description:
> >>>>>> +          Enable manual strobe position and EQ level
> >>>>>> +
> >>>>>> +    patternProperties:
> >>>>>> +      '^link@[0-9a-f]+$':
> >>>>>
> >>>>> There can be up to 4 links only, right ? I would then use
> >>>>>
> >>>>>          '^link@[0-3]$':
> >>>>
> >>>> Yes, I'll change that.
> >>>>
> >>>>>> +        type: object
> >>>>>> +        additionalProperties: false
> >>>>>> +        properties:
> >>>>>> +          reg:
> >>>>>> +            description: The link number
> >>>>>> +            maxItems: 1
> >>>>>> +
> >>>>>> +          i2c-alias:
> >>>>>> +            description:
> >>>>>> +              The i2c address used for the serializer. Transactions to this
> >>>>>> +              address on the i2c bus where the deserializer resides are
> >>>>>> +              forwarded to the serializer.
> >>>>>> +
> >>>>>> +          ti,rx-mode:
> >>>>>> +            $ref: /schemas/types.yaml#/definitions/uint32
> >>>>>> +            enum:
> >>>>>> +              - 0 # RAW10
> >>>>>> +              - 1 # RAW12 HF
> >>>>>> +              - 2 # RAW12 LF
> >>>>>> +              - 3 # CSI2 SYNC
> >>>>>> +              - 4 # CSI2 NON-SYNC
> >>>>>> +            description: FPD-Link Input Mode
> >>>>>
> >>>>> Are there use cases for controlling this dynamically (in particular the
> >>>>> sync/non-sync modes) ? Is there anything that could be queried at
> >>>>> runtime from the serializers instead of being specified in DT ?
> >>>>
> >>>> We need a link to the serializer before we can query anything from the
> >>>> serializer.
> >>>
> >>> I meant querying it from the serializer driver, not the serializer
> >>> hardware. This being said, it would likely be difficult to do so, as the
> >>> serializer driver would need to probe first. I think I'm thus fine
> >>> selecting the mode in DT on the deserializer side.
> >>>
> >>>> To have a link, we need the mode... So, as I mentioned in
> >>>> the other reply, we could define these in some way in the serializer's
> >>>> properties instead of here, but I'm not sure if that's a good change.
> >>>>
> >>>> The driver can change the mode at runtime (say, from sync to non-sync
> >>>> mode, if the HW supports that). But I think this property should reflect
> >>>> the HW strapped configuration of the serializer.
> >>>
> >>> That would possibly work for the DS90UB953, but the DS90UB913 has no
> >>> strapped mode selected at boot time but is instead configured
> >>> automatically through the back-channel (see my last reply to patch 3/8).
> >>
> >> Indeed.
> >>
> >>> When connecting a DS90UB913 to a DS90UB914 deserializer, we can probably
> >>> start without mode selection in software, as the MODE pin is meant to
> >>> bootstrap that to a correct value which is then automatically
> >>> transmitted to the serializer (hardware designs where the mode would
> >>> need to be overridden should be rate). However, when connecting multiple
> >>
> >> I don't know if that's true. I guess it depends on how you see the deser
> >> and the camera module. Are they part of the same HW design or not? In my
> >> setups they are quite separate, and I connect different kinds of camera
> >> modules to my deserializers. But I can see that if you create a, say,
> >> car, you'd have both sides known at design time and would never change.
> >>
> >>> DS90UB913 to a DS90UB960, I can imagine connecting different types of
> >>> cameras on the four input ports, so the need to specify the mode
> >>> per-port in DT would be more common.
> >>
> >> Right, and even with UB914, you might well design the deserializer side
> >> with, say, RAW10 sensors, but later in the cycle you'd need to change to
> >> a RAW12 sensor. Depending on the deser mode strap would require you to
> >> do a HW change on the deser side too.
> >>
> >> As I said in the other mail, I don't like the deser's strap, and I think
> >> we should just basically ignore it as we can provide the necessary data
> >> in the DT.
> > 
> > What I meant is that, given that the UB914 is meant to be used with a
> > single camera, using a RAW mode, there's a much higher chance that
> > hardware strapping will work as intended there. We could thus start
> > without support for overrides in a UB914 driver (but as far as I
> > understand we're not planning to work on such a driver in the near
> > future, so it's hypothetical only), while in the UB960 driver we
> > probably need override support from the beginning.
> 
> Yes, but depending on the UB914 strap mode would still be only a partial 
> solution, and it would still need to also support overriding the strap 
> mode (from DT). As you said in the other mail, we anyway have to define 
> the serializer and the sensor in the DT, so even with UB914 we couldn't 
> just switch the camera module and change the mode via a DIP switch.
> 
> So I still don't see why we would even bother supporting the deser strap 
> mode, even on UB914.

What I meant is that I wouldn't nack an initial UB914 driver submission
just because it didn't support that feature, while for the UB960, I
think it should be there from the start. That's all theoretical anyway,
no UB914 driver is planned, and the feature should be so easy to add
that it would likely be there from the start.

> >>> For these reasons, I don't think the ti,rx-mode property can be defined
> >>> as reflecting the hardware MODE strap with the DS90UB913. I also think
> >>> it would be quite confusing to define it as the desired runtime
> >>> configuration for the DS90UB913 and as the hardware MODE strap for the
> >>> DS90UB953. Could it be (explicitly) defined as the desired runtime
> >>> configuration in all cases ?
> >>
> >> That sounds bad in a DT context =). You're right that the rx-mode can't
> >> be defined as reflecting the serializer mode strap, but I think we can
> >> define it as reflecting the default operation mode of the serializer
> >> hardware (or maybe rather the camera module).
> > 
> > What do you mean by "default operation mode" in this case ?
> 
> How the camera module is HW strapped and supposed to be used, how it 
> works (more or less) out of the box:
> 
> - The UB953 is HW strapped to sync or non-sync CSI mode, or RAW mode.
> - Both UB913 and UB953 (in RAW mode) have a sensor connected either with 
> 10 or 12 lines.
> - The sensor has a default (either HW or use-case default) pixel clock.
> 
> Based on those, I think we get all the possible modes. So, I think, 
> every camera module will have a known "normal" / "default" mode, which 
> is what the mode in the DT tells.
> 
> Well, for FPD4, there's also the CDR mode. UB971 can work in both FDP3 
> and FPD4 modes. But there again it should be clear what's the "normal" 
> operation mode, as it comes from a serializer HW strap.
> 
> We can do changes to those at runtime (not supported by the drivers), 
> e.g. changing from sync to non-sync CSI mode, changing the RAW mode, and 
> even changing from FDP4 to FDP3. But those, I believe, are rare cases, 
> and the changes have to be done carefully, on both the deser and ser 
> sides in certain order.
> 
> So, back to the original point, "desired runtime configuration" and 
> "default operation mode" are maybe the same thing =).

I'm fine with this, it "just" needs to be expressed in a clear way in
the bindings :-)