diff mbox series

[V9,1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings

Message ID 20200523084103.31276-2-dongchun.zhu@mediatek.com (mailing list archive)
State New, archived
Headers show
Series media: i2c: Add support for OV02A10 sensor | expand

Commit Message

Dongchun Zhu May 23, 2020, 8:41 a.m. UTC
Add DT bindings documentation for Omnivision OV02A10 image sensor.

Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
---
 .../bindings/media/i2c/ovti,ov02a10.yaml           | 172 +++++++++++++++++++++
 MAINTAINERS                                        |   7 +
 2 files changed, 179 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml

Comments

Rob Herring (Arm) May 26, 2020, 6:28 p.m. UTC | #1
On Sat, May 23, 2020 at 04:41:02PM +0800, Dongchun Zhu wrote:
> Add DT bindings documentation for Omnivision OV02A10 image sensor.
> 
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> ---
>  .../bindings/media/i2c/ovti,ov02a10.yaml           | 172 +++++++++++++++++++++
>  MAINTAINERS                                        |   7 +
>  2 files changed, 179 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> new file mode 100644
> index 0000000..56f31b5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> @@ -0,0 +1,172 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright (c) 2020 MediaTek Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/ovti,ov02a10.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Omnivision OV02A10 CMOS Sensor Device Tree Bindings
> +
> +maintainers:
> +  - Dongchun Zhu <dongchun.zhu@mediatek.com>
> +
> +description: |-
> +  The Omnivision OV02A10 is a low-cost, high performance, 1/5-inch, 2 megapixel
> +  image sensor, which is the latest production derived from Omnivision's CMOS
> +  image sensor technology. Ihis chip supports high frame rate speeds up to 30fps
> +  @ 1600x1200 (UXGA) resolution transferred over a 1-lane MIPI interface. The
> +  sensor output is available via CSI-2 serial data output.
> +
> +properties:
> +  compatible:
> +    const: ovti,ov02a10
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: top mux camtg clock
> +      - description: divider clock
> +
> +  clock-names:
> +    items:
> +      - const: eclk
> +      - const: freq_mux
> +
> +  clock-frequency:
> +    description:
> +      Frequency of the eclk clock in Hertz.
> +
> +  dovdd-supply:
> +    description:
> +      Definition of the regulator used as Digital I/O voltage supply.
> +
> +  avdd-supply:
> +    description:
> +      Definition of the regulator used as Analog voltage supply.
> +
> +  dvdd-supply:
> +    description:
> +      Definition of the regulator used as Digital core voltage supply.
> +
> +  powerdown-gpios:
> +    description:
> +      Must be the device tree identifier of the GPIO connected to the
> +      PD_PAD pin. This pin is used to place the OV02A10 into Standby mode
> +      or Shutdown mode. As the line is active low, it should be
> +      marked GPIO_ACTIVE_LOW.

Need to define how many GPIOs ('maxItems: 1')

> +
> +  reset-gpios:
> +    description:
> +      Must be the device tree identifier of the GPIO connected to the
> +      RST_PD pin. If specified, it will be asserted during driver probe.
> +      As the line is active high, it should be marked GPIO_ACTIVE_HIGH.

Here too.

> +
> +  rotation:
> +    description:
> +      Definition of the sensor's placement.
> +    allOf:
> +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> +      - enum:
> +          - 0    # Sensor Mounted Upright
> +          - 180  # Sensor Mounted Upside Down
> +        default: 0
> +
> +  ovti,mipi-tx-speed:
> +    description:
> +      Indication of MIPI transmission speed select, which is to control D-PHY
> +      timing setting by adjusting MIPI clock voltage to improve the clock
> +      driver capability.
> +    allOf:
> +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> +      - enum:
> +          - 0    #  20MHz -  30MHz
> +          - 1    #  30MHz -  50MHz
> +          - 2    #  50MHz -  75MHz
> +          - 3    #  75MHz - 100MHz
> +          - 4    # 100MHz - 130MHz
> +        default: 3
> +
> +  # See ../video-interfaces.txt for details
> +  port:
> +    type: object
> +    additionalProperties: false

Should have a description of what data the port has.

> +
> +    properties:
> +      endpoint:
> +        type: object
> +        additionalProperties: false
> +
> +        properties:
> +          data-lanes:
> +            maxItems: 1
> +
> +          link-frequencies: true
> +          remote-endpoint: true
> +
> +        required:
> +          - data-lanes
> +          - link-frequencies
> +          - remote-endpoint
> +
> +    required:
> +      - endpoint
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - clock-frequency
> +  - dovdd-supply
> +  - avdd-supply
> +  - dvdd-supply
> +  - powerdown-gpios
> +  - reset-gpios
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +
> +    #include <dt-bindings/clock/mt8183-clk.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        ov02a10: camera-sensor@3d {
> +            compatible = "ovti,ov02a10";
> +            reg = <0x3d>;
> +            pinctrl-names = "default";
> +            pinctrl-0 = <&clk_24m_cam>;
> +
> +            clocks = <&topckgen CLK_TOP_MUX_CAMTG>,
> +                     <&topckgen CLK_TOP_UNIVP_192M_D8>;
> +            clock-names = "eclk", "freq_mux";
> +            clock-frequency = <24000000>;
> +
> +            rotation = <180>;
> +            ovti,mipi-tx-speed = <4>;
> +
> +            dovdd-supply = <&mt6358_vcamio_reg>;
> +            avdd-supply = <&mt6358_vcama1_reg>;
> +            dvdd-supply = <&mt6358_vcn18_reg>;
> +
> +            powerdown-gpios = <&pio 107 GPIO_ACTIVE_LOW>;
> +            reset-gpios = <&pio 109 GPIO_ACTIVE_HIGH>;
> +
> +            port {
> +                wcam_out: endpoint {
> +                    data-lanes = <1>;
> +                    link-frequencies = /bits/ 64 <390000000>;
> +                    remote-endpoint = <&mipi_in_wcam>;
> +                };
> +            };
> +        };
> +    };
> +
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e64e5db..63a2335 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12389,6 +12389,13 @@ M:	Harald Welte <laforge@gnumonks.org>
>  S:	Maintained
>  F:	drivers/char/pcmcia/cm4040_cs.*
>  
> +OMNIVISION OV02A10 SENSOR DRIVER
> +M:	Dongchun Zhu <dongchun.zhu@mediatek.com>
> +L:	linux-media@vger.kernel.org
> +S:	Maintained
> +T:	git git://linuxtv.org/media_tree.git
> +F:	Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> +
>  OMNIVISION OV13858 SENSOR DRIVER
>  M:	Sakari Ailus <sakari.ailus@linux.intel.com>
>  L:	linux-media@vger.kernel.org
> -- 
> 2.9.2
Dongchun Zhu May 27, 2020, 8:49 a.m. UTC | #2
Hi Rob,

Thanks for the review. Please see my replies below.

On Tue, 2020-05-26 at 12:28 -0600, Rob Herring wrote:
> On Sat, May 23, 2020 at 04:41:02PM +0800, Dongchun Zhu wrote:
> > Add DT bindings documentation for Omnivision OV02A10 image sensor.
> > 
> > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > ---
> >  .../bindings/media/i2c/ovti,ov02a10.yaml           | 172 +++++++++++++++++++++
> >  MAINTAINERS                                        |   7 +
> >  2 files changed, 179 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > new file mode 100644
> > index 0000000..56f31b5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > @@ -0,0 +1,172 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +# Copyright (c) 2020 MediaTek Inc.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/i2c/ovti,ov02a10.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Omnivision OV02A10 CMOS Sensor Device Tree Bindings
> > +
> > +maintainers:
> > +  - Dongchun Zhu <dongchun.zhu@mediatek.com>
> > +
> > +description: |-
> > +  The Omnivision OV02A10 is a low-cost, high performance, 1/5-inch, 2 megapixel
> > +  image sensor, which is the latest production derived from Omnivision's CMOS
> > +  image sensor technology. Ihis chip supports high frame rate speeds up to 30fps
> > +  @ 1600x1200 (UXGA) resolution transferred over a 1-lane MIPI interface. The
> > +  sensor output is available via CSI-2 serial data output.
> > +
> > +properties:
> > +  compatible:
> > +    const: ovti,ov02a10
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    items:
> > +      - description: top mux camtg clock
> > +      - description: divider clock
> > +
> > +  clock-names:
> > +    items:
> > +      - const: eclk
> > +      - const: freq_mux
> > +
> > +  clock-frequency:
> > +    description:
> > +      Frequency of the eclk clock in Hertz.
> > +

Rob, shall we use 'maxItems: 1' to constrain property: clock-frequency?
Or could we adopt 'clock-frequency: true' directly here?

> > +  dovdd-supply:
> > +    description:
> > +      Definition of the regulator used as Digital I/O voltage supply.
> > +

Shall we add 'maxItems: 1' here?

> > +  avdd-supply:
> > +    description:
> > +      Definition of the regulator used as Analog voltage supply.
> > +

Ditto.

> > +  dvdd-supply:
> > +    description:
> > +      Definition of the regulator used as Digital core voltage supply.
> > +

Ditto.

> > +  powerdown-gpios:
> > +    description:
> > +      Must be the device tree identifier of the GPIO connected to the
> > +      PD_PAD pin. This pin is used to place the OV02A10 into Standby mode
> > +      or Shutdown mode. As the line is active low, it should be
> > +      marked GPIO_ACTIVE_LOW.
> 
> Need to define how many GPIOs ('maxItems: 1')
> 

It would be fixed like this in next release.
powerdown-gpios:
  maxItems: 1
  description:
    Must be the device tree identifier of the GPIO connected to the
    PD_PAD pin. This pin is used to place the OV02A10 into Standby mode
    or Shutdown mode. As the line is active low, it should be
    marked GPIO_ACTIVE_LOW.

> > +
> > +  reset-gpios:
> > +    description:
> > +      Must be the device tree identifier of the GPIO connected to the
> > +      RST_PD pin. If specified, it will be asserted during driver probe.
> > +      As the line is active high, it should be marked GPIO_ACTIVE_HIGH.
> 
> Here too.
> 

Similar as 'powerdown-gpios'.
Fixed in next release.

> > +
> > +  rotation:
> > +    description:
> > +      Definition of the sensor's placement.
> > +    allOf:
> > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > +      - enum:
> > +          - 0    # Sensor Mounted Upright
> > +          - 180  # Sensor Mounted Upside Down
> > +        default: 0
> > +
> > +  ovti,mipi-tx-speed:
> > +    description:
> > +      Indication of MIPI transmission speed select, which is to control D-PHY
> > +      timing setting by adjusting MIPI clock voltage to improve the clock
> > +      driver capability.
> > +    allOf:
> > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > +      - enum:
> > +          - 0    #  20MHz -  30MHz
> > +          - 1    #  30MHz -  50MHz
> > +          - 2    #  50MHz -  75MHz
> > +          - 3    #  75MHz - 100MHz
> > +          - 4    # 100MHz - 130MHz
> > +        default: 3
> > +
> > +  # See ../video-interfaces.txt for details
> > +  port:
> > +    type: object
> > +    additionalProperties: false
> 
> Should have a description of what data the port has.
> 

It would be updated as below in next release.
port:
  type: object
  additionalProperties: false
  description:
    Input port node, single endpoint describing the CSI-2 transmitter.

> > +
> > +    properties:
> > +      endpoint:
> > +        type: object
> > +        additionalProperties: false
> > +
> > +        properties:

Actually I wonder whether we need to declare 'clock-lanes' here?

> > +          data-lanes:
> > +            maxItems: 1
> > +
> > +          link-frequencies: true
> > +          remote-endpoint: true
> > +
> > +        required:

Ditto.

> > +          - data-lanes
> > +          - link-frequencies
> > +          - remote-endpoint
> > +
> > +    required:
> > +      - endpoint
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - clock-frequency
> > +  - dovdd-supply
> > +  - avdd-supply
> > +  - dvdd-supply
> > +  - powerdown-gpios
> > +  - reset-gpios
> > +  - port
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +
> > +    #include <dt-bindings/clock/mt8183-clk.h>
> > +    #include <dt-bindings/gpio/gpio.h>
> > +
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        ov02a10: camera-sensor@3d {
> > +            compatible = "ovti,ov02a10";
> > +            reg = <0x3d>;
> > +            pinctrl-names = "default";
> > +            pinctrl-0 = <&clk_24m_cam>;
> > +
> > +            clocks = <&topckgen CLK_TOP_MUX_CAMTG>,
> > +                     <&topckgen CLK_TOP_UNIVP_192M_D8>;
> > +            clock-names = "eclk", "freq_mux";
> > +            clock-frequency = <24000000>;
> > +
> > +            rotation = <180>;
> > +            ovti,mipi-tx-speed = <4>;
> > +
> > +            dovdd-supply = <&mt6358_vcamio_reg>;
> > +            avdd-supply = <&mt6358_vcama1_reg>;
> > +            dvdd-supply = <&mt6358_vcn18_reg>;
> > +
> > +            powerdown-gpios = <&pio 107 GPIO_ACTIVE_LOW>;
> > +            reset-gpios = <&pio 109 GPIO_ACTIVE_HIGH>;
> > +
> > +            port {
> > +                wcam_out: endpoint {
> > +                    data-lanes = <1>;
> > +                    link-frequencies = /bits/ 64 <390000000>;
> > +                    remote-endpoint = <&mipi_in_wcam>;
> > +                };
> > +            };
> > +        };
> > +    };
> > +
> > +...
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index e64e5db..63a2335 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12389,6 +12389,13 @@ M:	Harald Welte <laforge@gnumonks.org>
> >  S:	Maintained
> >  F:	drivers/char/pcmcia/cm4040_cs.*
> >  
> > +OMNIVISION OV02A10 SENSOR DRIVER
> > +M:	Dongchun Zhu <dongchun.zhu@mediatek.com>
> > +L:	linux-media@vger.kernel.org
> > +S:	Maintained
> > +T:	git git://linuxtv.org/media_tree.git
> > +F:	Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > +
> >  OMNIVISION OV13858 SENSOR DRIVER
> >  M:	Sakari Ailus <sakari.ailus@linux.intel.com>
> >  L:	linux-media@vger.kernel.org
> > -- 
> > 2.9.2
Rob Herring (Arm) May 27, 2020, 3:27 p.m. UTC | #3
On Wed, May 27, 2020 at 2:51 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
>
> Hi Rob,
>
> Thanks for the review. Please see my replies below.
>
> On Tue, 2020-05-26 at 12:28 -0600, Rob Herring wrote:
> > On Sat, May 23, 2020 at 04:41:02PM +0800, Dongchun Zhu wrote:
> > > Add DT bindings documentation for Omnivision OV02A10 image sensor.
> > >
> > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > ---
> > >  .../bindings/media/i2c/ovti,ov02a10.yaml           | 172 +++++++++++++++++++++
> > >  MAINTAINERS                                        |   7 +
> > >  2 files changed, 179 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > > new file mode 100644
> > > index 0000000..56f31b5
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > > @@ -0,0 +1,172 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +# Copyright (c) 2020 MediaTek Inc.
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/media/i2c/ovti,ov02a10.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Omnivision OV02A10 CMOS Sensor Device Tree Bindings
> > > +
> > > +maintainers:
> > > +  - Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > +
> > > +description: |-
> > > +  The Omnivision OV02A10 is a low-cost, high performance, 1/5-inch, 2 megapixel
> > > +  image sensor, which is the latest production derived from Omnivision's CMOS
> > > +  image sensor technology. Ihis chip supports high frame rate speeds up to 30fps
> > > +  @ 1600x1200 (UXGA) resolution transferred over a 1-lane MIPI interface. The
> > > +  sensor output is available via CSI-2 serial data output.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: ovti,ov02a10
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  clocks:
> > > +    items:
> > > +      - description: top mux camtg clock
> > > +      - description: divider clock
> > > +
> > > +  clock-names:
> > > +    items:
> > > +      - const: eclk
> > > +      - const: freq_mux
> > > +
> > > +  clock-frequency:
> > > +    description:
> > > +      Frequency of the eclk clock in Hertz.
> > > +
>
> Rob, shall we use 'maxItems: 1' to constrain property: clock-frequency?

No, because it is a scalar, not an array.

> Or could we adopt 'clock-frequency: true' directly here?

As-is is fine.

> > > +  dovdd-supply:
> > > +    description:
> > > +      Definition of the regulator used as Digital I/O voltage supply.
> > > +
>
> Shall we add 'maxItems: 1' here?

No, supplies are always singular.


>
> > > +  avdd-supply:
> > > +    description:
> > > +      Definition of the regulator used as Analog voltage supply.
> > > +
>
> Ditto.
>
> > > +  dvdd-supply:
> > > +    description:
> > > +      Definition of the regulator used as Digital core voltage supply.
> > > +
>
> Ditto.
>
> > > +  powerdown-gpios:
> > > +    description:
> > > +      Must be the device tree identifier of the GPIO connected to the
> > > +      PD_PAD pin. This pin is used to place the OV02A10 into Standby mode
> > > +      or Shutdown mode. As the line is active low, it should be
> > > +      marked GPIO_ACTIVE_LOW.
> >
> > Need to define how many GPIOs ('maxItems: 1')
> >
>
> It would be fixed like this in next release.
> powerdown-gpios:
>   maxItems: 1
>   description:
>     Must be the device tree identifier of the GPIO connected to the
>     PD_PAD pin. This pin is used to place the OV02A10 into Standby mode
>     or Shutdown mode. As the line is active low, it should be
>     marked GPIO_ACTIVE_LOW.
>
> > > +
> > > +  reset-gpios:
> > > +    description:
> > > +      Must be the device tree identifier of the GPIO connected to the
> > > +      RST_PD pin. If specified, it will be asserted during driver probe.
> > > +      As the line is active high, it should be marked GPIO_ACTIVE_HIGH.
> >
> > Here too.
> >
>
> Similar as 'powerdown-gpios'.
> Fixed in next release.
>
> > > +
> > > +  rotation:
> > > +    description:
> > > +      Definition of the sensor's placement.
> > > +    allOf:
> > > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > > +      - enum:
> > > +          - 0    # Sensor Mounted Upright
> > > +          - 180  # Sensor Mounted Upside Down
> > > +        default: 0
> > > +
> > > +  ovti,mipi-tx-speed:
> > > +    description:
> > > +      Indication of MIPI transmission speed select, which is to control D-PHY
> > > +      timing setting by adjusting MIPI clock voltage to improve the clock
> > > +      driver capability.
> > > +    allOf:
> > > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > > +      - enum:
> > > +          - 0    #  20MHz -  30MHz
> > > +          - 1    #  30MHz -  50MHz
> > > +          - 2    #  50MHz -  75MHz
> > > +          - 3    #  75MHz - 100MHz
> > > +          - 4    # 100MHz - 130MHz
> > > +        default: 3
> > > +
> > > +  # See ../video-interfaces.txt for details
> > > +  port:
> > > +    type: object
> > > +    additionalProperties: false
> >
> > Should have a description of what data the port has.
> >
>
> It would be updated as below in next release.
> port:
>   type: object
>   additionalProperties: false
>   description:
>     Input port node, single endpoint describing the CSI-2 transmitter.

Output?

>
> > > +
> > > +    properties:
> > > +      endpoint:
> > > +        type: object
> > > +        additionalProperties: false
> > > +
> > > +        properties:
>
> Actually I wonder whether we need to declare 'clock-lanes' here?

Yes, if you are using it.

Rob
Sakari Ailus May 27, 2020, 9:16 p.m. UTC | #4
Hi Rob, Dongchun,

On Wed, May 27, 2020 at 09:27:22AM -0600, Rob Herring wrote:
> > > > +    properties:
> > > > +      endpoint:
> > > > +        type: object
> > > > +        additionalProperties: false
> > > > +
> > > > +        properties:
> >
> > Actually I wonder whether we need to declare 'clock-lanes' here?
> 
> Yes, if you are using it.

Dongchun, can you confirm the chip has a single data and a single clock
lane and that it does not support lane reordering?

So if there's nothing to convey to the driver, also the data-lanes should
be removed IMO.
Dongchun Zhu May 28, 2020, 3:34 a.m. UTC | #5
Hi Sakari, Rob,

On Thu, 2020-05-28 at 00:16 +0300, Sakari Ailus wrote:
> Hi Rob, Dongchun,
> 
> On Wed, May 27, 2020 at 09:27:22AM -0600, Rob Herring wrote:
> > > > > +    properties:
> > > > > +      endpoint:
> > > > > +        type: object
> > > > > +        additionalProperties: false
> > > > > +
> > > > > +        properties:
> > >
> > > Actually I wonder whether we need to declare 'clock-lanes' here?
> > 
> > Yes, if you are using it.
> 
> Dongchun, can you confirm the chip has a single data and a single clock
> lane and that it does not support lane reordering?
> 

From the datasheet, 'MIPI inside the OV02A10 provides one single
uni-directional clock lane and one bi-directional data lane solution for
communication links between components inside a mobile device.
The data lane has full support for HS(uni-directional) and
LP(bi-directional) data transfer mode.'

The sensor doesn't support lane reordering, so 'clock-lanes' property
would not be added in next release.

> So if there's nothing to convey to the driver, also the data-lanes should
> be removed IMO.
> 

However, 'data-lanes' property may still be required.
It is known that either data-lanes or clock-lanes is an array of
physical data lane indexes. Position of an entry determines the logical
lane number, while the value of an entry indicates physical lane, e.g.,
for 1-lane MIPI CSI-2 bus we could have "data-lanes = <1>;", assuming
the clock lane is on hardware lane 0.

As mentioned earlier, the OV02A10 sensor supports only 1C1D and does not
support lane reordering, so here we shall use 'data-lanes = <1>' as
there is only a clock lane for OV02A10.

Reminder:
If 'data-lanes' property is not present, the driver would assume
four-lane operation. This means for one-lane or two-lane operation, this
property must be present and set to the right physical lane indexes.
If the hardware does not support lane reordering, monotonically
incremented values shall be used from 0 or 1 onwards, depending on
whether or not there is also a clock lane.
Dongchun Zhu May 28, 2020, 5:53 a.m. UTC | #6
Hi Rob,

On Wed, 2020-05-27 at 09:27 -0600, Rob Herring wrote:
> On Wed, May 27, 2020 at 2:51 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> >
> > Hi Rob,
> >
> > Thanks for the review. Please see my replies below.
> >
> > On Tue, 2020-05-26 at 12:28 -0600, Rob Herring wrote:
> > > On Sat, May 23, 2020 at 04:41:02PM +0800, Dongchun Zhu wrote:
> > > > Add DT bindings documentation for Omnivision OV02A10 image sensor.
> > > >
> > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > > ---
> > > >  .../bindings/media/i2c/ovti,ov02a10.yaml           | 172 +++++++++++++++++++++
> > > >  MAINTAINERS                                        |   7 +
> > > >  2 files changed, 179 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > > > new file mode 100644
> > > > index 0000000..56f31b5
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > > > @@ -0,0 +1,172 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > +# Copyright (c) 2020 MediaTek Inc.
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/media/i2c/ovti,ov02a10.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Omnivision OV02A10 CMOS Sensor Device Tree Bindings
> > > > +
> > > > +maintainers:
> > > > +  - Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > > +
> > > > +description: |-
> > > > +  The Omnivision OV02A10 is a low-cost, high performance, 1/5-inch, 2 megapixel
> > > > +  image sensor, which is the latest production derived from Omnivision's CMOS
> > > > +  image sensor technology. Ihis chip supports high frame rate speeds up to 30fps
> > > > +  @ 1600x1200 (UXGA) resolution transferred over a 1-lane MIPI interface. The
> > > > +  sensor output is available via CSI-2 serial data output.
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: ovti,ov02a10
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  clocks:
> > > > +    items:
> > > > +      - description: top mux camtg clock
> > > > +      - description: divider clock
> > > > +
> > > > +  clock-names:
> > > > +    items:
> > > > +      - const: eclk
> > > > +      - const: freq_mux
> > > > +
> > > > +  clock-frequency:
> > > > +    description:
> > > > +      Frequency of the eclk clock in Hertz.
> > > > +
> >
> > Rob, shall we use 'maxItems: 1' to constrain property: clock-frequency?
> 
> No, because it is a scalar, not an array.
> 

Got it.

> > Or could we adopt 'clock-frequency: true' directly here?
> 
> As-is is fine.
> 

Understood.

> > > > +  dovdd-supply:
> > > > +    description:
> > > > +      Definition of the regulator used as Digital I/O voltage supply.
> > > > +
> >
> > Shall we add 'maxItems: 1' here?
> 
> No, supplies are always singular.
> 

Fine.

> >
> > > > +  avdd-supply:
> > > > +    description:
> > > > +      Definition of the regulator used as Analog voltage supply.
> > > > +
> >
> > Ditto.
> >
> > > > +  dvdd-supply:
> > > > +    description:
> > > > +      Definition of the regulator used as Digital core voltage supply.
> > > > +
> >
> > Ditto.
> >
> > > > +  powerdown-gpios:
> > > > +    description:
> > > > +      Must be the device tree identifier of the GPIO connected to the
> > > > +      PD_PAD pin. This pin is used to place the OV02A10 into Standby mode
> > > > +      or Shutdown mode. As the line is active low, it should be
> > > > +      marked GPIO_ACTIVE_LOW.
> > >
> > > Need to define how many GPIOs ('maxItems: 1')
> > >
> >
> > It would be fixed like this in next release.
> > powerdown-gpios:
> >   maxItems: 1
> >   description:
> >     Must be the device tree identifier of the GPIO connected to the
> >     PD_PAD pin. This pin is used to place the OV02A10 into Standby mode
> >     or Shutdown mode. As the line is active low, it should be
> >     marked GPIO_ACTIVE_LOW.
> >

Tomasz, I don't know whether you noticed this description.
Here I simply defined one necessary GPIO polarity in DT which driver
settings need to follow.

> > > > +
> > > > +  reset-gpios:
> > > > +    description:
> > > > +      Must be the device tree identifier of the GPIO connected to the
> > > > +      RST_PD pin. If specified, it will be asserted during driver probe.
> > > > +      As the line is active high, it should be marked GPIO_ACTIVE_HIGH.
> > >
> > > Here too.
> > >
> >
> > Similar as 'powerdown-gpios'.
> > Fixed in next release.
> >
> > > > +
> > > > +  rotation:
> > > > +    description:
> > > > +      Definition of the sensor's placement.
> > > > +    allOf:
> > > > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > > > +      - enum:
> > > > +          - 0    # Sensor Mounted Upright
> > > > +          - 180  # Sensor Mounted Upside Down
> > > > +        default: 0
> > > > +
> > > > +  ovti,mipi-tx-speed:
> > > > +    description:
> > > > +      Indication of MIPI transmission speed select, which is to control D-PHY
> > > > +      timing setting by adjusting MIPI clock voltage to improve the clock
> > > > +      driver capability.
> > > > +    allOf:
> > > > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > > > +      - enum:
> > > > +          - 0    #  20MHz -  30MHz
> > > > +          - 1    #  30MHz -  50MHz
> > > > +          - 2    #  50MHz -  75MHz
> > > > +          - 3    #  75MHz - 100MHz
> > > > +          - 4    # 100MHz - 130MHz
> > > > +        default: 3
> > > > +
> > > > +  # See ../video-interfaces.txt for details
> > > > +  port:
> > > > +    type: object
> > > > +    additionalProperties: false
> > >
> > > Should have a description of what data the port has.
> > >
> >
> > It would be updated as below in next release.
> > port:
> >   type: object
> >   additionalProperties: false
> >   description:
> >     Input port node, single endpoint describing the CSI-2 transmitter.
> 
> Output?
> 

Sorry for the typo.
Yes, this should be output port node.

> >
> > > > +
> > > > +    properties:
> > > > +      endpoint:
> > > > +        type: object
> > > > +        additionalProperties: false
> > > > +
> > > > +        properties:
> >
> > Actually I wonder whether we need to declare 'clock-lanes' here?
> 
> Yes, if you are using it.
> 

Looking back to the upstreamed patches, it seems that there is a deep
divide in the setting of 'clock-lanes' property.

As the last comment I just posed, for OV02A10, 'clock-lanes' should be
set to <0> (clock lane on hardware lane 0).
But here we may follow IMX219 patch, which removed 'clock-lanes'
property due to the fact that sensor hardware does not support lane
reordering.

Rob, Sakari, what's your opinions?

> Rob
Sakari Ailus May 28, 2020, 7:23 a.m. UTC | #7
Hi Dongchun,

On Thu, May 28, 2020 at 11:34:42AM +0800, Dongchun Zhu wrote:
> Hi Sakari, Rob,
> 
> On Thu, 2020-05-28 at 00:16 +0300, Sakari Ailus wrote:
> > Hi Rob, Dongchun,
> > 
> > On Wed, May 27, 2020 at 09:27:22AM -0600, Rob Herring wrote:
> > > > > > +    properties:
> > > > > > +      endpoint:
> > > > > > +        type: object
> > > > > > +        additionalProperties: false
> > > > > > +
> > > > > > +        properties:
> > > >
> > > > Actually I wonder whether we need to declare 'clock-lanes' here?
> > > 
> > > Yes, if you are using it.
> > 
> > Dongchun, can you confirm the chip has a single data and a single clock
> > lane and that it does not support lane reordering?
> > 
> 
> From the datasheet, 'MIPI inside the OV02A10 provides one single
> uni-directional clock lane and one bi-directional data lane solution for
> communication links between components inside a mobile device.
> The data lane has full support for HS(uni-directional) and
> LP(bi-directional) data transfer mode.'
> 
> The sensor doesn't support lane reordering, so 'clock-lanes' property
> would not be added in next release.
> 
> > So if there's nothing to convey to the driver, also the data-lanes should
> > be removed IMO.
> > 
> 
> However, 'data-lanes' property may still be required.
> It is known that either data-lanes or clock-lanes is an array of
> physical data lane indexes. Position of an entry determines the logical
> lane number, while the value of an entry indicates physical lane, e.g.,
> for 1-lane MIPI CSI-2 bus we could have "data-lanes = <1>;", assuming
> the clock lane is on hardware lane 0.
> 
> As mentioned earlier, the OV02A10 sensor supports only 1C1D and does not
> support lane reordering, so here we shall use 'data-lanes = <1>' as
> there is only a clock lane for OV02A10.
> 
> Reminder:
> If 'data-lanes' property is not present, the driver would assume
> four-lane operation. This means for one-lane or two-lane operation, this
> property must be present and set to the right physical lane indexes.
> If the hardware does not support lane reordering, monotonically
> incremented values shall be used from 0 or 1 onwards, depending on
> whether or not there is also a clock lane.

How can the driver use four lanes, considering the device only supports a
single lane??
Dongchun Zhu May 28, 2020, 8:04 a.m. UTC | #8
Hi Sakari,

On Thu, 2020-05-28 at 10:23 +0300, Sakari Ailus wrote:
> Hi Dongchun,
> 
> On Thu, May 28, 2020 at 11:34:42AM +0800, Dongchun Zhu wrote:
> > Hi Sakari, Rob,
> > 
> > On Thu, 2020-05-28 at 00:16 +0300, Sakari Ailus wrote:
> > > Hi Rob, Dongchun,
> > > 
> > > On Wed, May 27, 2020 at 09:27:22AM -0600, Rob Herring wrote:
> > > > > > > +    properties:
> > > > > > > +      endpoint:
> > > > > > > +        type: object
> > > > > > > +        additionalProperties: false
> > > > > > > +
> > > > > > > +        properties:
> > > > >
> > > > > Actually I wonder whether we need to declare 'clock-lanes' here?
> > > > 
> > > > Yes, if you are using it.
> > > 
> > > Dongchun, can you confirm the chip has a single data and a single clock
> > > lane and that it does not support lane reordering?
> > > 
> > 
> > From the datasheet, 'MIPI inside the OV02A10 provides one single
> > uni-directional clock lane and one bi-directional data lane solution for
> > communication links between components inside a mobile device.
> > The data lane has full support for HS(uni-directional) and
> > LP(bi-directional) data transfer mode.'
> > 
> > The sensor doesn't support lane reordering, so 'clock-lanes' property
> > would not be added in next release.
> > 
> > > So if there's nothing to convey to the driver, also the data-lanes should
> > > be removed IMO.
> > > 
> > 
> > However, 'data-lanes' property may still be required.
> > It is known that either data-lanes or clock-lanes is an array of
> > physical data lane indexes. Position of an entry determines the logical
> > lane number, while the value of an entry indicates physical lane, e.g.,
> > for 1-lane MIPI CSI-2 bus we could have "data-lanes = <1>;", assuming
> > the clock lane is on hardware lane 0.
> > 
> > As mentioned earlier, the OV02A10 sensor supports only 1C1D and does not
> > support lane reordering, so here we shall use 'data-lanes = <1>' as
> > there is only a clock lane for OV02A10.
> > 
> > Reminder:
> > If 'data-lanes' property is not present, the driver would assume
> > four-lane operation. This means for one-lane or two-lane operation, this
> > property must be present and set to the right physical lane indexes.
> > If the hardware does not support lane reordering, monotonically
> > incremented values shall be used from 0 or 1 onwards, depending on
> > whether or not there is also a clock lane.
> 
> How can the driver use four lanes, considering the device only supports a
> single lane??
> 

I understood your meaning.
If we omit the property 'data-lanes', the sensor should work still.
But then what's the meaning of the existence of 'data-lanes'?
If this property 'data-lanes' is always optional, then why dt-bindings
provide the interface?

In the meantime, if omitting 'data-lanes' for one sensor(transmitter)
that has only one physical data lane, MIPI receiver(e.g., MIPI CSI-2)
shall enable four-lane configuration, which may increase consumption of
both power and resource in the process of IIC communication.
Tomasz Figa May 29, 2020, 1:43 p.m. UTC | #9
On Thu, May 28, 2020 at 10:06 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
>
> Hi Sakari,
>
> On Thu, 2020-05-28 at 10:23 +0300, Sakari Ailus wrote:
> > Hi Dongchun,
> >
> > On Thu, May 28, 2020 at 11:34:42AM +0800, Dongchun Zhu wrote:
> > > Hi Sakari, Rob,
> > >
> > > On Thu, 2020-05-28 at 00:16 +0300, Sakari Ailus wrote:
> > > > Hi Rob, Dongchun,
> > > >
> > > > On Wed, May 27, 2020 at 09:27:22AM -0600, Rob Herring wrote:
> > > > > > > > +    properties:
> > > > > > > > +      endpoint:
> > > > > > > > +        type: object
> > > > > > > > +        additionalProperties: false
> > > > > > > > +
> > > > > > > > +        properties:
> > > > > >
> > > > > > Actually I wonder whether we need to declare 'clock-lanes' here?
> > > > >
> > > > > Yes, if you are using it.
> > > >
> > > > Dongchun, can you confirm the chip has a single data and a single clock
> > > > lane and that it does not support lane reordering?
> > > >
> > >
> > > From the datasheet, 'MIPI inside the OV02A10 provides one single
> > > uni-directional clock lane and one bi-directional data lane solution for
> > > communication links between components inside a mobile device.
> > > The data lane has full support for HS(uni-directional) and
> > > LP(bi-directional) data transfer mode.'
> > >
> > > The sensor doesn't support lane reordering, so 'clock-lanes' property
> > > would not be added in next release.
> > >
> > > > So if there's nothing to convey to the driver, also the data-lanes should
> > > > be removed IMO.
> > > >
> > >
> > > However, 'data-lanes' property may still be required.
> > > It is known that either data-lanes or clock-lanes is an array of
> > > physical data lane indexes. Position of an entry determines the logical
> > > lane number, while the value of an entry indicates physical lane, e.g.,
> > > for 1-lane MIPI CSI-2 bus we could have "data-lanes = <1>;", assuming
> > > the clock lane is on hardware lane 0.
> > >
> > > As mentioned earlier, the OV02A10 sensor supports only 1C1D and does not
> > > support lane reordering, so here we shall use 'data-lanes = <1>' as
> > > there is only a clock lane for OV02A10.
> > >
> > > Reminder:
> > > If 'data-lanes' property is not present, the driver would assume
> > > four-lane operation. This means for one-lane or two-lane operation, this
> > > property must be present and set to the right physical lane indexes.
> > > If the hardware does not support lane reordering, monotonically
> > > incremented values shall be used from 0 or 1 onwards, depending on
> > > whether or not there is also a clock lane.
> >
> > How can the driver use four lanes, considering the device only supports a
> > single lane??
> >
>
> I understood your meaning.
> If we omit the property 'data-lanes', the sensor should work still.
> But then what's the meaning of the existence of 'data-lanes'?
> If this property 'data-lanes' is always optional, then why dt-bindings
> provide the interface?
>
> In the meantime, if omitting 'data-lanes' for one sensor(transmitter)
> that has only one physical data lane, MIPI receiver(e.g., MIPI CSI-2)
> shall enable four-lane configuration, which may increase consumption of
> both power and resource in the process of IIC communication.

Wouldn't the receiver still have the data-lanes property under its
endpoint node, telling it how many lanes and in which order should be
used?

Best regards,
Tomasz
Dongchun Zhu June 1, 2020, 2:33 a.m. UTC | #10
Hi Tomasz,

On Fri, 2020-05-29 at 15:43 +0200, Tomasz Figa wrote:
> On Thu, May 28, 2020 at 10:06 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> >
> > Hi Sakari,
> >
> > On Thu, 2020-05-28 at 10:23 +0300, Sakari Ailus wrote:
> > > Hi Dongchun,
> > >
> > > On Thu, May 28, 2020 at 11:34:42AM +0800, Dongchun Zhu wrote:
> > > > Hi Sakari, Rob,
> > > >
> > > > On Thu, 2020-05-28 at 00:16 +0300, Sakari Ailus wrote:
> > > > > Hi Rob, Dongchun,
> > > > >
> > > > > On Wed, May 27, 2020 at 09:27:22AM -0600, Rob Herring wrote:
> > > > > > > > > +    properties:
> > > > > > > > > +      endpoint:
> > > > > > > > > +        type: object
> > > > > > > > > +        additionalProperties: false
> > > > > > > > > +
> > > > > > > > > +        properties:
> > > > > > >
> > > > > > > Actually I wonder whether we need to declare 'clock-lanes' here?
> > > > > >
> > > > > > Yes, if you are using it.
> > > > >
> > > > > Dongchun, can you confirm the chip has a single data and a single clock
> > > > > lane and that it does not support lane reordering?
> > > > >
> > > >
> > > > From the datasheet, 'MIPI inside the OV02A10 provides one single
> > > > uni-directional clock lane and one bi-directional data lane solution for
> > > > communication links between components inside a mobile device.
> > > > The data lane has full support for HS(uni-directional) and
> > > > LP(bi-directional) data transfer mode.'
> > > >
> > > > The sensor doesn't support lane reordering, so 'clock-lanes' property
> > > > would not be added in next release.
> > > >
> > > > > So if there's nothing to convey to the driver, also the data-lanes should
> > > > > be removed IMO.
> > > > >
> > > >
> > > > However, 'data-lanes' property may still be required.
> > > > It is known that either data-lanes or clock-lanes is an array of
> > > > physical data lane indexes. Position of an entry determines the logical
> > > > lane number, while the value of an entry indicates physical lane, e.g.,
> > > > for 1-lane MIPI CSI-2 bus we could have "data-lanes = <1>;", assuming
> > > > the clock lane is on hardware lane 0.
> > > >
> > > > As mentioned earlier, the OV02A10 sensor supports only 1C1D and does not
> > > > support lane reordering, so here we shall use 'data-lanes = <1>' as
> > > > there is only a clock lane for OV02A10.
> > > >
> > > > Reminder:
> > > > If 'data-lanes' property is not present, the driver would assume
> > > > four-lane operation. This means for one-lane or two-lane operation, this
> > > > property must be present and set to the right physical lane indexes.
> > > > If the hardware does not support lane reordering, monotonically
> > > > incremented values shall be used from 0 or 1 onwards, depending on
> > > > whether or not there is also a clock lane.
> > >
> > > How can the driver use four lanes, considering the device only supports a
> > > single lane??
> > >
> >
> > I understood your meaning.
> > If we omit the property 'data-lanes', the sensor should work still.
> > But then what's the meaning of the existence of 'data-lanes'?
> > If this property 'data-lanes' is always optional, then why dt-bindings
> > provide the interface?
> >
> > In the meantime, if omitting 'data-lanes' for one sensor(transmitter)
> > that has only one physical data lane, MIPI receiver(e.g., MIPI CSI-2)
> > shall enable four-lane configuration, which may increase consumption of
> > both power and resource in the process of IIC communication.
> 
> Wouldn't the receiver still have the data-lanes property under its
> endpoint node, telling it how many lanes and in which order should be
> used?
> 

The MIPI receiver(RX) shall use
v4l2_async_notifier_add_fwnode_remote_subdev() API to parse the property
"data-lanes" under sensor output port.

> Best regards,
> Tomasz
Tomasz Figa June 1, 2020, 6:18 p.m. UTC | #11
On Mon, Jun 1, 2020 at 4:35 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
>
> Hi Tomasz,
>
> On Fri, 2020-05-29 at 15:43 +0200, Tomasz Figa wrote:
> > On Thu, May 28, 2020 at 10:06 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > >
> > > Hi Sakari,
> > >
> > > On Thu, 2020-05-28 at 10:23 +0300, Sakari Ailus wrote:
> > > > Hi Dongchun,
> > > >
> > > > On Thu, May 28, 2020 at 11:34:42AM +0800, Dongchun Zhu wrote:
> > > > > Hi Sakari, Rob,
> > > > >
> > > > > On Thu, 2020-05-28 at 00:16 +0300, Sakari Ailus wrote:
> > > > > > Hi Rob, Dongchun,
> > > > > >
> > > > > > On Wed, May 27, 2020 at 09:27:22AM -0600, Rob Herring wrote:
> > > > > > > > > > +    properties:
> > > > > > > > > > +      endpoint:
> > > > > > > > > > +        type: object
> > > > > > > > > > +        additionalProperties: false
> > > > > > > > > > +
> > > > > > > > > > +        properties:
> > > > > > > >
> > > > > > > > Actually I wonder whether we need to declare 'clock-lanes' here?
> > > > > > >
> > > > > > > Yes, if you are using it.
> > > > > >
> > > > > > Dongchun, can you confirm the chip has a single data and a single clock
> > > > > > lane and that it does not support lane reordering?
> > > > > >
> > > > >
> > > > > From the datasheet, 'MIPI inside the OV02A10 provides one single
> > > > > uni-directional clock lane and one bi-directional data lane solution for
> > > > > communication links between components inside a mobile device.
> > > > > The data lane has full support for HS(uni-directional) and
> > > > > LP(bi-directional) data transfer mode.'
> > > > >
> > > > > The sensor doesn't support lane reordering, so 'clock-lanes' property
> > > > > would not be added in next release.
> > > > >
> > > > > > So if there's nothing to convey to the driver, also the data-lanes should
> > > > > > be removed IMO.
> > > > > >
> > > > >
> > > > > However, 'data-lanes' property may still be required.
> > > > > It is known that either data-lanes or clock-lanes is an array of
> > > > > physical data lane indexes. Position of an entry determines the logical
> > > > > lane number, while the value of an entry indicates physical lane, e.g.,
> > > > > for 1-lane MIPI CSI-2 bus we could have "data-lanes = <1>;", assuming
> > > > > the clock lane is on hardware lane 0.
> > > > >
> > > > > As mentioned earlier, the OV02A10 sensor supports only 1C1D and does not
> > > > > support lane reordering, so here we shall use 'data-lanes = <1>' as
> > > > > there is only a clock lane for OV02A10.
> > > > >
> > > > > Reminder:
> > > > > If 'data-lanes' property is not present, the driver would assume
> > > > > four-lane operation. This means for one-lane or two-lane operation, this
> > > > > property must be present and set to the right physical lane indexes.
> > > > > If the hardware does not support lane reordering, monotonically
> > > > > incremented values shall be used from 0 or 1 onwards, depending on
> > > > > whether or not there is also a clock lane.
> > > >
> > > > How can the driver use four lanes, considering the device only supports a
> > > > single lane??
> > > >
> > >
> > > I understood your meaning.
> > > If we omit the property 'data-lanes', the sensor should work still.
> > > But then what's the meaning of the existence of 'data-lanes'?
> > > If this property 'data-lanes' is always optional, then why dt-bindings
> > > provide the interface?
> > >
> > > In the meantime, if omitting 'data-lanes' for one sensor(transmitter)
> > > that has only one physical data lane, MIPI receiver(e.g., MIPI CSI-2)
> > > shall enable four-lane configuration, which may increase consumption of
> > > both power and resource in the process of IIC communication.
> >
> > Wouldn't the receiver still have the data-lanes property under its
> > endpoint node, telling it how many lanes and in which order should be
> > used?
> >
>
> The MIPI receiver(RX) shall use
> v4l2_async_notifier_add_fwnode_remote_subdev() API to parse the property
> "data-lanes" under sensor output port.

That's not true. The MIPI receiver driver parses its own port node
corresponding to the sensor. Also quoting the documentation [1]:

"An endpoint subnode of a device contains all properties needed for
_configuration of this device_ for data exchange with other device. In most
cases properties at the peer 'endpoint' nodes will be identical, however they
might need to be different when there is any signal modifications on the bus
between two devices, e.g. there are logic signal inverters on the lines."

In this case, there is such a signal modification if the sensor has a
1-lane bus and the receiver more lines, so the data-lanes properties
would be different on both sides.

[1] https://elixir.bootlin.com/linux/v5.7/source/Documentation/devicetree/bindings/media/video-interfaces.txt

Best regards,
Tomasz
Dongchun Zhu June 2, 2020, 6:15 a.m. UTC | #12
Hi Tomasz, Sakari,

On Mon, 2020-06-01 at 20:18 +0200, Tomasz Figa wrote:
> On Mon, Jun 1, 2020 at 4:35 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> >
> > Hi Tomasz,
> >
> > On Fri, 2020-05-29 at 15:43 +0200, Tomasz Figa wrote:
> > > On Thu, May 28, 2020 at 10:06 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > > >
> > > > Hi Sakari,
> > > >
> > > > On Thu, 2020-05-28 at 10:23 +0300, Sakari Ailus wrote:
> > > > > Hi Dongchun,
> > > > >
> > > > > On Thu, May 28, 2020 at 11:34:42AM +0800, Dongchun Zhu wrote:
> > > > > > Hi Sakari, Rob,
> > > > > >
> > > > > > On Thu, 2020-05-28 at 00:16 +0300, Sakari Ailus wrote:
> > > > > > > Hi Rob, Dongchun,
> > > > > > >
> > > > > > > On Wed, May 27, 2020 at 09:27:22AM -0600, Rob Herring wrote:
> > > > > > > > > > > +    properties:
> > > > > > > > > > > +      endpoint:
> > > > > > > > > > > +        type: object
> > > > > > > > > > > +        additionalProperties: false
> > > > > > > > > > > +
> > > > > > > > > > > +        properties:
> > > > > > > > >
> > > > > > > > > Actually I wonder whether we need to declare 'clock-lanes' here?
> > > > > > > >
> > > > > > > > Yes, if you are using it.
> > > > > > >
> > > > > > > Dongchun, can you confirm the chip has a single data and a single clock
> > > > > > > lane and that it does not support lane reordering?
> > > > > > >
> > > > > >
> > > > > > From the datasheet, 'MIPI inside the OV02A10 provides one single
> > > > > > uni-directional clock lane and one bi-directional data lane solution for
> > > > > > communication links between components inside a mobile device.
> > > > > > The data lane has full support for HS(uni-directional) and
> > > > > > LP(bi-directional) data transfer mode.'
> > > > > >
> > > > > > The sensor doesn't support lane reordering, so 'clock-lanes' property
> > > > > > would not be added in next release.
> > > > > >
> > > > > > > So if there's nothing to convey to the driver, also the data-lanes should
> > > > > > > be removed IMO.
> > > > > > >
> > > > > >
> > > > > > However, 'data-lanes' property may still be required.
> > > > > > It is known that either data-lanes or clock-lanes is an array of
> > > > > > physical data lane indexes. Position of an entry determines the logical
> > > > > > lane number, while the value of an entry indicates physical lane, e.g.,
> > > > > > for 1-lane MIPI CSI-2 bus we could have "data-lanes = <1>;", assuming
> > > > > > the clock lane is on hardware lane 0.
> > > > > >
> > > > > > As mentioned earlier, the OV02A10 sensor supports only 1C1D and does not
> > > > > > support lane reordering, so here we shall use 'data-lanes = <1>' as
> > > > > > there is only a clock lane for OV02A10.
> > > > > >
> > > > > > Reminder:
> > > > > > If 'data-lanes' property is not present, the driver would assume
> > > > > > four-lane operation. This means for one-lane or two-lane operation, this
> > > > > > property must be present and set to the right physical lane indexes.
> > > > > > If the hardware does not support lane reordering, monotonically
> > > > > > incremented values shall be used from 0 or 1 onwards, depending on
> > > > > > whether or not there is also a clock lane.
> > > > >
> > > > > How can the driver use four lanes, considering the device only supports a
> > > > > single lane??
> > > > >
> > > >
> > > > I understood your meaning.
> > > > If we omit the property 'data-lanes', the sensor should work still.
> > > > But then what's the meaning of the existence of 'data-lanes'?
> > > > If this property 'data-lanes' is always optional, then why dt-bindings
> > > > provide the interface?
> > > >
> > > > In the meantime, if omitting 'data-lanes' for one sensor(transmitter)
> > > > that has only one physical data lane, MIPI receiver(e.g., MIPI CSI-2)
> > > > shall enable four-lane configuration, which may increase consumption of
> > > > both power and resource in the process of IIC communication.
> > >
> > > Wouldn't the receiver still have the data-lanes property under its
> > > endpoint node, telling it how many lanes and in which order should be
> > > used?
> > >
> >
> > The MIPI receiver(RX) shall use
> > v4l2_async_notifier_add_fwnode_remote_subdev() API to parse the property
> > "data-lanes" under sensor output port.
> 
> That's not true. The MIPI receiver driver parses its own port node
> corresponding to the sensor. Also quoting the documentation [1]:
> 
> "An endpoint subnode of a device contains all properties needed for
> _configuration of this device_ for data exchange with other device. In most
> cases properties at the peer 'endpoint' nodes will be identical, however they
> might need to be different when there is any signal modifications on the bus
> between two devices, e.g. there are logic signal inverters on the lines."
> 
> In this case, there is such a signal modification if the sensor has a
> 1-lane bus and the receiver more lines, so the data-lanes properties
> would be different on both sides.
> 
> [1] https://elixir.bootlin.com/linux/v5.7/source/Documentation/devicetree/bindings/media/video-interfaces.txt
> 

Sorry for the misunderstanding.
After doing some experiments about the data-lanes property under sensor
i2c node, we found the API
v4l2_async_notifier_add_fwnode_remote_subdev() that MIPI receiver driver
used indeed parses the data-lanes under its own port node.

Sorry make a mistake for the use case of sensor data-lanes previously.
Now We may encounter one new question for this patch.
In practice we haven't used the data-lanes under sensor i2c node
anywhere, if sensor driver itself doesn't parse that.

But there is still one reason to keep the exactly right data-lanes in
DT. That is, the data-lanes under sensor i2c node could be used as a
reference for MIPI receiver driver.
Just as Tomasz said, 'The MIPI receiver driver parses its own port node
corresponding to the sensor'.

Sakari, Tomasz, what's your opinions about the present of data-lanes
under sensor node or not?

> Best regards,
> Tomasz
Sakari Ailus June 2, 2020, 9:53 a.m. UTC | #13
On Fri, May 29, 2020 at 03:43:30PM +0200, Tomasz Figa wrote:
> On Thu, May 28, 2020 at 10:06 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> >
> > Hi Sakari,
> >
> > On Thu, 2020-05-28 at 10:23 +0300, Sakari Ailus wrote:
> > > Hi Dongchun,
> > >
> > > On Thu, May 28, 2020 at 11:34:42AM +0800, Dongchun Zhu wrote:
> > > > Hi Sakari, Rob,
> > > >
> > > > On Thu, 2020-05-28 at 00:16 +0300, Sakari Ailus wrote:
> > > > > Hi Rob, Dongchun,
> > > > >
> > > > > On Wed, May 27, 2020 at 09:27:22AM -0600, Rob Herring wrote:
> > > > > > > > > +    properties:
> > > > > > > > > +      endpoint:
> > > > > > > > > +        type: object
> > > > > > > > > +        additionalProperties: false
> > > > > > > > > +
> > > > > > > > > +        properties:
> > > > > > >
> > > > > > > Actually I wonder whether we need to declare 'clock-lanes' here?
> > > > > >
> > > > > > Yes, if you are using it.
> > > > >
> > > > > Dongchun, can you confirm the chip has a single data and a single clock
> > > > > lane and that it does not support lane reordering?
> > > > >
> > > >
> > > > From the datasheet, 'MIPI inside the OV02A10 provides one single
> > > > uni-directional clock lane and one bi-directional data lane solution for
> > > > communication links between components inside a mobile device.
> > > > The data lane has full support for HS(uni-directional) and
> > > > LP(bi-directional) data transfer mode.'
> > > >
> > > > The sensor doesn't support lane reordering, so 'clock-lanes' property
> > > > would not be added in next release.
> > > >
> > > > > So if there's nothing to convey to the driver, also the data-lanes should
> > > > > be removed IMO.
> > > > >
> > > >
> > > > However, 'data-lanes' property may still be required.
> > > > It is known that either data-lanes or clock-lanes is an array of
> > > > physical data lane indexes. Position of an entry determines the logical
> > > > lane number, while the value of an entry indicates physical lane, e.g.,
> > > > for 1-lane MIPI CSI-2 bus we could have "data-lanes = <1>;", assuming
> > > > the clock lane is on hardware lane 0.
> > > >
> > > > As mentioned earlier, the OV02A10 sensor supports only 1C1D and does not
> > > > support lane reordering, so here we shall use 'data-lanes = <1>' as
> > > > there is only a clock lane for OV02A10.
> > > >
> > > > Reminder:
> > > > If 'data-lanes' property is not present, the driver would assume
> > > > four-lane operation. This means for one-lane or two-lane operation, this
> > > > property must be present and set to the right physical lane indexes.
> > > > If the hardware does not support lane reordering, monotonically
> > > > incremented values shall be used from 0 or 1 onwards, depending on
> > > > whether or not there is also a clock lane.
> > >
> > > How can the driver use four lanes, considering the device only supports a
> > > single lane??
> > >
> >
> > I understood your meaning.
> > If we omit the property 'data-lanes', the sensor should work still.
> > But then what's the meaning of the existence of 'data-lanes'?
> > If this property 'data-lanes' is always optional, then why dt-bindings
> > provide the interface?
> >
> > In the meantime, if omitting 'data-lanes' for one sensor(transmitter)
> > that has only one physical data lane, MIPI receiver(e.g., MIPI CSI-2)
> > shall enable four-lane configuration, which may increase consumption of
> > both power and resource in the process of IIC communication.
> 
> Wouldn't the receiver still have the data-lanes property under its
> endpoint node, telling it how many lanes and in which order should be
> used?

Yes.
Sakari Ailus June 2, 2020, 9:56 a.m. UTC | #14
Hi Dongchun,

On Tue, Jun 02, 2020 at 02:15:01PM +0800, Dongchun Zhu wrote:
> Hi Tomasz, Sakari,
> 
> On Mon, 2020-06-01 at 20:18 +0200, Tomasz Figa wrote:
> > On Mon, Jun 1, 2020 at 4:35 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > >
> > > Hi Tomasz,
> > >
> > > On Fri, 2020-05-29 at 15:43 +0200, Tomasz Figa wrote:
> > > > On Thu, May 28, 2020 at 10:06 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > > > >
> > > > > Hi Sakari,
> > > > >
> > > > > On Thu, 2020-05-28 at 10:23 +0300, Sakari Ailus wrote:
> > > > > > Hi Dongchun,
> > > > > >
> > > > > > On Thu, May 28, 2020 at 11:34:42AM +0800, Dongchun Zhu wrote:
> > > > > > > Hi Sakari, Rob,
> > > > > > >
> > > > > > > On Thu, 2020-05-28 at 00:16 +0300, Sakari Ailus wrote:
> > > > > > > > Hi Rob, Dongchun,
> > > > > > > >
> > > > > > > > On Wed, May 27, 2020 at 09:27:22AM -0600, Rob Herring wrote:
> > > > > > > > > > > > +    properties:
> > > > > > > > > > > > +      endpoint:
> > > > > > > > > > > > +        type: object
> > > > > > > > > > > > +        additionalProperties: false
> > > > > > > > > > > > +
> > > > > > > > > > > > +        properties:
> > > > > > > > > >
> > > > > > > > > > Actually I wonder whether we need to declare 'clock-lanes' here?
> > > > > > > > >
> > > > > > > > > Yes, if you are using it.
> > > > > > > >
> > > > > > > > Dongchun, can you confirm the chip has a single data and a single clock
> > > > > > > > lane and that it does not support lane reordering?
> > > > > > > >
> > > > > > >
> > > > > > > From the datasheet, 'MIPI inside the OV02A10 provides one single
> > > > > > > uni-directional clock lane and one bi-directional data lane solution for
> > > > > > > communication links between components inside a mobile device.
> > > > > > > The data lane has full support for HS(uni-directional) and
> > > > > > > LP(bi-directional) data transfer mode.'
> > > > > > >
> > > > > > > The sensor doesn't support lane reordering, so 'clock-lanes' property
> > > > > > > would not be added in next release.
> > > > > > >
> > > > > > > > So if there's nothing to convey to the driver, also the data-lanes should
> > > > > > > > be removed IMO.
> > > > > > > >
> > > > > > >
> > > > > > > However, 'data-lanes' property may still be required.
> > > > > > > It is known that either data-lanes or clock-lanes is an array of
> > > > > > > physical data lane indexes. Position of an entry determines the logical
> > > > > > > lane number, while the value of an entry indicates physical lane, e.g.,
> > > > > > > for 1-lane MIPI CSI-2 bus we could have "data-lanes = <1>;", assuming
> > > > > > > the clock lane is on hardware lane 0.
> > > > > > >
> > > > > > > As mentioned earlier, the OV02A10 sensor supports only 1C1D and does not
> > > > > > > support lane reordering, so here we shall use 'data-lanes = <1>' as
> > > > > > > there is only a clock lane for OV02A10.
> > > > > > >
> > > > > > > Reminder:
> > > > > > > If 'data-lanes' property is not present, the driver would assume
> > > > > > > four-lane operation. This means for one-lane or two-lane operation, this
> > > > > > > property must be present and set to the right physical lane indexes.
> > > > > > > If the hardware does not support lane reordering, monotonically
> > > > > > > incremented values shall be used from 0 or 1 onwards, depending on
> > > > > > > whether or not there is also a clock lane.
> > > > > >
> > > > > > How can the driver use four lanes, considering the device only supports a
> > > > > > single lane??
> > > > > >
> > > > >
> > > > > I understood your meaning.
> > > > > If we omit the property 'data-lanes', the sensor should work still.
> > > > > But then what's the meaning of the existence of 'data-lanes'?
> > > > > If this property 'data-lanes' is always optional, then why dt-bindings
> > > > > provide the interface?
> > > > >
> > > > > In the meantime, if omitting 'data-lanes' for one sensor(transmitter)
> > > > > that has only one physical data lane, MIPI receiver(e.g., MIPI CSI-2)
> > > > > shall enable four-lane configuration, which may increase consumption of
> > > > > both power and resource in the process of IIC communication.
> > > >
> > > > Wouldn't the receiver still have the data-lanes property under its
> > > > endpoint node, telling it how many lanes and in which order should be
> > > > used?
> > > >
> > >
> > > The MIPI receiver(RX) shall use
> > > v4l2_async_notifier_add_fwnode_remote_subdev() API to parse the property
> > > "data-lanes" under sensor output port.
> > 
> > That's not true. The MIPI receiver driver parses its own port node
> > corresponding to the sensor. Also quoting the documentation [1]:
> > 
> > "An endpoint subnode of a device contains all properties needed for
> > _configuration of this device_ for data exchange with other device. In most
> > cases properties at the peer 'endpoint' nodes will be identical, however they
> > might need to be different when there is any signal modifications on the bus
> > between two devices, e.g. there are logic signal inverters on the lines."
> > 
> > In this case, there is such a signal modification if the sensor has a
> > 1-lane bus and the receiver more lines, so the data-lanes properties
> > would be different on both sides.
> > 
> > [1] https://elixir.bootlin.com/linux/v5.7/source/Documentation/devicetree/bindings/media/video-interfaces.txt
> > 
> 
> Sorry for the misunderstanding.
> After doing some experiments about the data-lanes property under sensor
> i2c node, we found the API
> v4l2_async_notifier_add_fwnode_remote_subdev() that MIPI receiver driver
> used indeed parses the data-lanes under its own port node.
> 
> Sorry make a mistake for the use case of sensor data-lanes previously.
> Now We may encounter one new question for this patch.
> In practice we haven't used the data-lanes under sensor i2c node
> anywhere, if sensor driver itself doesn't parse that.
> 
> But there is still one reason to keep the exactly right data-lanes in
> DT. That is, the data-lanes under sensor i2c node could be used as a
> reference for MIPI receiver driver.
> Just as Tomasz said, 'The MIPI receiver driver parses its own port node
> corresponding to the sensor'.
> 
> Sakari, Tomasz, what's your opinions about the present of data-lanes
> under sensor node or not?

The receiver driver doesn't parse the properties in the sensor
(transmitter) device's endpoint. If that property provides no information
to the receiver, as is the case here, it should be omitted.
Dongchun Zhu June 4, 2020, 2:20 a.m. UTC | #15
Hi Sakari,

On Tue, 2020-06-02 at 12:56 +0300, Sakari Ailus wrote:
> Hi Dongchun,
> 
> On Tue, Jun 02, 2020 at 02:15:01PM +0800, Dongchun Zhu wrote:
> > Hi Tomasz, Sakari,
> > 
> > On Mon, 2020-06-01 at 20:18 +0200, Tomasz Figa wrote:
> > > On Mon, Jun 1, 2020 at 4:35 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > > >
> > > > Hi Tomasz,
> > > >
> > > > On Fri, 2020-05-29 at 15:43 +0200, Tomasz Figa wrote:
> > > > > On Thu, May 28, 2020 at 10:06 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > > > > >
> > > > > > Hi Sakari,
> > > > > >
> > > > > > On Thu, 2020-05-28 at 10:23 +0300, Sakari Ailus wrote:
> > > > > > > Hi Dongchun,
> > > > > > >
> > > > > > > On Thu, May 28, 2020 at 11:34:42AM +0800, Dongchun Zhu wrote:
> > > > > > > > Hi Sakari, Rob,
> > > > > > > >
> > > > > > > > On Thu, 2020-05-28 at 00:16 +0300, Sakari Ailus wrote:
> > > > > > > > > Hi Rob, Dongchun,
> > > > > > > > >
> > > > > > > > > On Wed, May 27, 2020 at 09:27:22AM -0600, Rob Herring wrote:
> > > > > > > > > > > > > +    properties:
> > > > > > > > > > > > > +      endpoint:
> > > > > > > > > > > > > +        type: object
> > > > > > > > > > > > > +        additionalProperties: false
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +        properties:
> > > > > > > > > > >
> > > > > > > > > > > Actually I wonder whether we need to declare 'clock-lanes' here?
> > > > > > > > > >
> > > > > > > > > > Yes, if you are using it.
> > > > > > > > >
> > > > > > > > > Dongchun, can you confirm the chip has a single data and a single clock
> > > > > > > > > lane and that it does not support lane reordering?
> > > > > > > > >
> > > > > > > >
> > > > > > > > From the datasheet, 'MIPI inside the OV02A10 provides one single
> > > > > > > > uni-directional clock lane and one bi-directional data lane solution for
> > > > > > > > communication links between components inside a mobile device.
> > > > > > > > The data lane has full support for HS(uni-directional) and
> > > > > > > > LP(bi-directional) data transfer mode.'
> > > > > > > >
> > > > > > > > The sensor doesn't support lane reordering, so 'clock-lanes' property
> > > > > > > > would not be added in next release.
> > > > > > > >
> > > > > > > > > So if there's nothing to convey to the driver, also the data-lanes should
> > > > > > > > > be removed IMO.
> > > > > > > > >
> > > > > > > >
> > > > > > > > However, 'data-lanes' property may still be required.
> > > > > > > > It is known that either data-lanes or clock-lanes is an array of
> > > > > > > > physical data lane indexes. Position of an entry determines the logical
> > > > > > > > lane number, while the value of an entry indicates physical lane, e.g.,
> > > > > > > > for 1-lane MIPI CSI-2 bus we could have "data-lanes = <1>;", assuming
> > > > > > > > the clock lane is on hardware lane 0.
> > > > > > > >
> > > > > > > > As mentioned earlier, the OV02A10 sensor supports only 1C1D and does not
> > > > > > > > support lane reordering, so here we shall use 'data-lanes = <1>' as
> > > > > > > > there is only a clock lane for OV02A10.
> > > > > > > >
> > > > > > > > Reminder:
> > > > > > > > If 'data-lanes' property is not present, the driver would assume
> > > > > > > > four-lane operation. This means for one-lane or two-lane operation, this
> > > > > > > > property must be present and set to the right physical lane indexes.
> > > > > > > > If the hardware does not support lane reordering, monotonically
> > > > > > > > incremented values shall be used from 0 or 1 onwards, depending on
> > > > > > > > whether or not there is also a clock lane.
> > > > > > >
> > > > > > > How can the driver use four lanes, considering the device only supports a
> > > > > > > single lane??
> > > > > > >
> > > > > >
> > > > > > I understood your meaning.
> > > > > > If we omit the property 'data-lanes', the sensor should work still.
> > > > > > But then what's the meaning of the existence of 'data-lanes'?
> > > > > > If this property 'data-lanes' is always optional, then why dt-bindings
> > > > > > provide the interface?
> > > > > >
> > > > > > In the meantime, if omitting 'data-lanes' for one sensor(transmitter)
> > > > > > that has only one physical data lane, MIPI receiver(e.g., MIPI CSI-2)
> > > > > > shall enable four-lane configuration, which may increase consumption of
> > > > > > both power and resource in the process of IIC communication.
> > > > >
> > > > > Wouldn't the receiver still have the data-lanes property under its
> > > > > endpoint node, telling it how many lanes and in which order should be
> > > > > used?
> > > > >
> > > >
> > > > The MIPI receiver(RX) shall use
> > > > v4l2_async_notifier_add_fwnode_remote_subdev() API to parse the property
> > > > "data-lanes" under sensor output port.
> > > 
> > > That's not true. The MIPI receiver driver parses its own port node
> > > corresponding to the sensor. Also quoting the documentation [1]:
> > > 
> > > "An endpoint subnode of a device contains all properties needed for
> > > _configuration of this device_ for data exchange with other device. In most
> > > cases properties at the peer 'endpoint' nodes will be identical, however they
> > > might need to be different when there is any signal modifications on the bus
> > > between two devices, e.g. there are logic signal inverters on the lines."
> > > 
> > > In this case, there is such a signal modification if the sensor has a
> > > 1-lane bus and the receiver more lines, so the data-lanes properties
> > > would be different on both sides.
> > > 
> > > [1] https://elixir.bootlin.com/linux/v5.7/source/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > 
> > 
> > Sorry for the misunderstanding.
> > After doing some experiments about the data-lanes property under sensor
> > i2c node, we found the API
> > v4l2_async_notifier_add_fwnode_remote_subdev() that MIPI receiver driver
> > used indeed parses the data-lanes under its own port node.
> > 
> > Sorry make a mistake for the use case of sensor data-lanes previously.
> > Now We may encounter one new question for this patch.
> > In practice we haven't used the data-lanes under sensor i2c node
> > anywhere, if sensor driver itself doesn't parse that.
> > 
> > But there is still one reason to keep the exactly right data-lanes in
> > DT. That is, the data-lanes under sensor i2c node could be used as a
> > reference for MIPI receiver driver.
> > Just as Tomasz said, 'The MIPI receiver driver parses its own port node
> > corresponding to the sensor'.
> > 
> > Sakari, Tomasz, what's your opinions about the present of data-lanes
> > under sensor node or not?
> 
> The receiver driver doesn't parse the properties in the sensor
> (transmitter) device's endpoint. If that property provides no information
> to the receiver, as is the case here, it should be omitted.
> 

Understood.
Fixed in next release.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
new file mode 100644
index 0000000..56f31b5
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
@@ -0,0 +1,172 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright (c) 2020 MediaTek Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/ovti,ov02a10.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Omnivision OV02A10 CMOS Sensor Device Tree Bindings
+
+maintainers:
+  - Dongchun Zhu <dongchun.zhu@mediatek.com>
+
+description: |-
+  The Omnivision OV02A10 is a low-cost, high performance, 1/5-inch, 2 megapixel
+  image sensor, which is the latest production derived from Omnivision's CMOS
+  image sensor technology. Ihis chip supports high frame rate speeds up to 30fps
+  @ 1600x1200 (UXGA) resolution transferred over a 1-lane MIPI interface. The
+  sensor output is available via CSI-2 serial data output.
+
+properties:
+  compatible:
+    const: ovti,ov02a10
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: top mux camtg clock
+      - description: divider clock
+
+  clock-names:
+    items:
+      - const: eclk
+      - const: freq_mux
+
+  clock-frequency:
+    description:
+      Frequency of the eclk clock in Hertz.
+
+  dovdd-supply:
+    description:
+      Definition of the regulator used as Digital I/O voltage supply.
+
+  avdd-supply:
+    description:
+      Definition of the regulator used as Analog voltage supply.
+
+  dvdd-supply:
+    description:
+      Definition of the regulator used as Digital core voltage supply.
+
+  powerdown-gpios:
+    description:
+      Must be the device tree identifier of the GPIO connected to the
+      PD_PAD pin. This pin is used to place the OV02A10 into Standby mode
+      or Shutdown mode. As the line is active low, it should be
+      marked GPIO_ACTIVE_LOW.
+
+  reset-gpios:
+    description:
+      Must be the device tree identifier of the GPIO connected to the
+      RST_PD pin. If specified, it will be asserted during driver probe.
+      As the line is active high, it should be marked GPIO_ACTIVE_HIGH.
+
+  rotation:
+    description:
+      Definition of the sensor's placement.
+    allOf:
+      - $ref: "/schemas/types.yaml#/definitions/uint32"
+      - enum:
+          - 0    # Sensor Mounted Upright
+          - 180  # Sensor Mounted Upside Down
+        default: 0
+
+  ovti,mipi-tx-speed:
+    description:
+      Indication of MIPI transmission speed select, which is to control D-PHY
+      timing setting by adjusting MIPI clock voltage to improve the clock
+      driver capability.
+    allOf:
+      - $ref: "/schemas/types.yaml#/definitions/uint32"
+      - enum:
+          - 0    #  20MHz -  30MHz
+          - 1    #  30MHz -  50MHz
+          - 2    #  50MHz -  75MHz
+          - 3    #  75MHz - 100MHz
+          - 4    # 100MHz - 130MHz
+        default: 3
+
+  # See ../video-interfaces.txt for details
+  port:
+    type: object
+    additionalProperties: false
+
+    properties:
+      endpoint:
+        type: object
+        additionalProperties: false
+
+        properties:
+          data-lanes:
+            maxItems: 1
+
+          link-frequencies: true
+          remote-endpoint: true
+
+        required:
+          - data-lanes
+          - link-frequencies
+          - remote-endpoint
+
+    required:
+      - endpoint
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - clock-frequency
+  - dovdd-supply
+  - avdd-supply
+  - dvdd-supply
+  - powerdown-gpios
+  - reset-gpios
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+
+    #include <dt-bindings/clock/mt8183-clk.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ov02a10: camera-sensor@3d {
+            compatible = "ovti,ov02a10";
+            reg = <0x3d>;
+            pinctrl-names = "default";
+            pinctrl-0 = <&clk_24m_cam>;
+
+            clocks = <&topckgen CLK_TOP_MUX_CAMTG>,
+                     <&topckgen CLK_TOP_UNIVP_192M_D8>;
+            clock-names = "eclk", "freq_mux";
+            clock-frequency = <24000000>;
+
+            rotation = <180>;
+            ovti,mipi-tx-speed = <4>;
+
+            dovdd-supply = <&mt6358_vcamio_reg>;
+            avdd-supply = <&mt6358_vcama1_reg>;
+            dvdd-supply = <&mt6358_vcn18_reg>;
+
+            powerdown-gpios = <&pio 107 GPIO_ACTIVE_LOW>;
+            reset-gpios = <&pio 109 GPIO_ACTIVE_HIGH>;
+
+            port {
+                wcam_out: endpoint {
+                    data-lanes = <1>;
+                    link-frequencies = /bits/ 64 <390000000>;
+                    remote-endpoint = <&mipi_in_wcam>;
+                };
+            };
+        };
+    };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index e64e5db..63a2335 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12389,6 +12389,13 @@  M:	Harald Welte <laforge@gnumonks.org>
 S:	Maintained
 F:	drivers/char/pcmcia/cm4040_cs.*
 
+OMNIVISION OV02A10 SENSOR DRIVER
+M:	Dongchun Zhu <dongchun.zhu@mediatek.com>
+L:	linux-media@vger.kernel.org
+S:	Maintained
+T:	git git://linuxtv.org/media_tree.git
+F:	Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
+
 OMNIVISION OV13858 SENSOR DRIVER
 M:	Sakari Ailus <sakari.ailus@linux.intel.com>
 L:	linux-media@vger.kernel.org