diff mbox series

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

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

Commit Message

Dongchun Zhu April 30, 2020, 8:09 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           | 148 +++++++++++++++++++++
 MAINTAINERS                                        |   7 +
 2 files changed, 155 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml

Comments

Sakari Ailus May 5, 2020, 7:04 a.m. UTC | #1 
Hi Dongchun,

On Thu, Apr 30, 2020 at 04:09:23PM +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           | 148 +++++++++++++++++++++
>  MAINTAINERS                                        |   7 +
>  2 files changed, 155 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..2be4bd2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> @@ -0,0 +1,148 @@
> +# 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: devider 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 interface power supply.
> +
> +  avdd-supply:
> +    description:
> +      Definition of the regulator used as analog power supply.
> +
> +  dvdd-supply:
> +    description:
> +      Definition of the regulator used as digital power supply.
> +
> +  powerdown-gpios:
> +    description:
> +      The phandle and specifier for the GPIO that controls sensor powerdown.
> +
> +  reset-gpios:
> +    description:
> +      The phandle and specifier for the GPIO that controls sensor reset.
> +
> +  rotation:
> +    description:
> +      Definition of the sensor's placement, valid values are 0 and 180.
> +    allOf:
> +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> +      - enum:
> +          - 0    # Sensor Mounted Upright
> +          - 180  # Sensor Mounted Upside Down
> +
> +  ovti,mipi-tx-speed:
> +    description:
> +      Indication of MIPI transmission speed select.

What exactly does this signify? And how do you come up with the number?

> +    allOf:
> +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> +      - enum: [ 3, 4 ]
> +
> +  # See ../video-interfaces.txt for details
> +  port:
> +    type: object
> +    additionalProperties: false
> +
> +    properties:
> +      endpoint:
> +        type: object
> +        additionalProperties: false
> +
> +        properties:
> +          remote-endpoint: true
> +          link-frequencies: true
> +
> +    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 {
> +        clock-frequency = <400000>;
> +        #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 = <3>;
> +
> +            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 {
> +                    remote-endpoint = <&mipi_in_wcam>;
> +                    link-frequencies = /bits/ 64 <390000000>;
> +                };
> +            };
> +        };
> +    };
> +
> +...
> 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
Dongchun Zhu May 5, 2020, 2:17 p.m. UTC | #2 
Hi Sakari,

Thanks for the review.

On Tue, 2020-05-05 at 10:04 +0300, Sakari Ailus wrote:
> Hi Dongchun,
> 
> On Thu, Apr 30, 2020 at 04:09:23PM +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           | 148 +++++++++++++++++++++
> >  MAINTAINERS                                        |   7 +
> >  2 files changed, 155 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..2be4bd2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > @@ -0,0 +1,148 @@
> > +# 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: devider 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 interface power supply.
> > +
> > +  avdd-supply:
> > +    description:
> > +      Definition of the regulator used as analog power supply.
> > +
> > +  dvdd-supply:
> > +    description:
> > +      Definition of the regulator used as digital power supply.
> > +
> > +  powerdown-gpios:
> > +    description:
> > +      The phandle and specifier for the GPIO that controls sensor powerdown.
> > +
> > +  reset-gpios:
> > +    description:
> > +      The phandle and specifier for the GPIO that controls sensor reset.
> > +
> > +  rotation:
> > +    description:
> > +      Definition of the sensor's placement, valid values are 0 and 180.
> > +    allOf:
> > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > +      - enum:
> > +          - 0    # Sensor Mounted Upright
> > +          - 180  # Sensor Mounted Upside Down
> > +
> > +  ovti,mipi-tx-speed:
> > +    description:
> > +      Indication of MIPI transmission speed select.
> 
> What exactly does this signify? And how do you come up with the number?
> 

Apologies for not addressing this number clear.

From the datasheet, P1:0xA1 register represents TX_SPEED_AREA_SEL with
the default val: 0x03.
The description of this RW register is as below:
Bit[2:0]: MIPI transmission speed select.

Thus the enum should be definited as [ 0, 1, 2, 3, 4, 5, 6, 7 ].
This would be fixed in next release.

In the meantime, as the default val of P1:0xA1 is 0x03, we hope to keep
that value if there is no setting for this private property in DT.
The caller in driver would be updated like this in next release.
if (ov02a10->mipi_clock_tx_speed)
	ret = i2c_smbus_write_byte_data(...,...);

> > +    allOf:
> > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > +      - enum: [ 3, 4 ]
> > +
> > +  # See ../video-interfaces.txt for details
> > +  port:
> > +    type: object
> > +    additionalProperties: false
> > +
> > +    properties:
> > +      endpoint:
> > +        type: object
> > +        additionalProperties: false
> > +
> > +        properties:
> > +          remote-endpoint: true
> > +          link-frequencies: true
> > +
> > +    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 {
> > +        clock-frequency = <400000>;
> > +        #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 = <3>;
> > +
> > +            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 {
> > +                    remote-endpoint = <&mipi_in_wcam>;
> > +                    link-frequencies = /bits/ 64 <390000000>;
> > +                };
> > +            };
> > +        };
> > +    };
> > +
> > +...
> > 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
>
Rob Herring (Arm) May 5, 2020, 4:15 p.m. UTC | #3 
On Thu, Apr 30, 2020 at 04:09:23PM +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           | 148 +++++++++++++++++++++
>  MAINTAINERS                                        |   7 +
>  2 files changed, 155 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..2be4bd2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> @@ -0,0 +1,148 @@
> +# 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: devider 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 interface power supply.
> +
> +  avdd-supply:
> +    description:
> +      Definition of the regulator used as analog power supply.
> +
> +  dvdd-supply:
> +    description:
> +      Definition of the regulator used as digital power supply.
> +
> +  powerdown-gpios:

maxItems: 1

> +    description:
> +      The phandle and specifier for the GPIO that controls sensor powerdown.

Can be dropped. Doesn't tell me anything specific to this device.

> +
> +  reset-gpios:

maxItems: 1

> +    description:
> +      The phandle and specifier for the GPIO that controls sensor reset.
> +
> +  rotation:
> +    description:
> +      Definition of the sensor's placement, valid values are 0 and 180.
> +    allOf:
> +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> +      - enum:
> +          - 0    # Sensor Mounted Upright
> +          - 180  # Sensor Mounted Upside Down
> +
> +  ovti,mipi-tx-speed:
> +    description:
> +      Indication of MIPI transmission speed select.
> +    allOf:
> +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> +      - enum: [ 3, 4 ]
> +
> +  # See ../video-interfaces.txt for details
> +  port:
> +    type: object
> +    additionalProperties: false
> +
> +    properties:
> +      endpoint:
> +        type: object
> +        additionalProperties: false
> +
> +        properties:
> +          remote-endpoint: true
> +          link-frequencies: true
> +
> +    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 {
> +        clock-frequency = <400000>;
> +        #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 = <3>;
> +
> +            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 {
> +                    remote-endpoint = <&mipi_in_wcam>;
> +                    link-frequencies = /bits/ 64 <390000000>;
> +                };
> +            };
> +        };
> +    };
> +
> +...
> 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
Sakari Ailus May 6, 2020, 11:21 a.m. UTC | #4 
Hi Dongchun,

On Tue, May 05, 2020 at 10:17:18PM +0800, Dongchun Zhu wrote:
> Hi Sakari,
> 
> Thanks for the review.
> 
> On Tue, 2020-05-05 at 10:04 +0300, Sakari Ailus wrote:
> > Hi Dongchun,
> > 
> > On Thu, Apr 30, 2020 at 04:09:23PM +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           | 148 +++++++++++++++++++++
> > >  MAINTAINERS                                        |   7 +
> > >  2 files changed, 155 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..2be4bd2
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > > @@ -0,0 +1,148 @@
> > > +# 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: devider 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 interface power supply.
> > > +
> > > +  avdd-supply:
> > > +    description:
> > > +      Definition of the regulator used as analog power supply.
> > > +
> > > +  dvdd-supply:
> > > +    description:
> > > +      Definition of the regulator used as digital power supply.
> > > +
> > > +  powerdown-gpios:
> > > +    description:
> > > +      The phandle and specifier for the GPIO that controls sensor powerdown.
> > > +
> > > +  reset-gpios:
> > > +    description:
> > > +      The phandle and specifier for the GPIO that controls sensor reset.
> > > +
> > > +  rotation:
> > > +    description:
> > > +      Definition of the sensor's placement, valid values are 0 and 180.
> > > +    allOf:
> > > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > > +      - enum:
> > > +          - 0    # Sensor Mounted Upright
> > > +          - 180  # Sensor Mounted Upside Down
> > > +
> > > +  ovti,mipi-tx-speed:
> > > +    description:
> > > +      Indication of MIPI transmission speed select.
> > 
> > What exactly does this signify? And how do you come up with the number?
> > 
> 
> Apologies for not addressing this number clear.
> 
> From the datasheet, P1:0xA1 register represents TX_SPEED_AREA_SEL with
> the default val: 0x03.
> The description of this RW register is as below:
> Bit[2:0]: MIPI transmission speed select.
> 
> Thus the enum should be definited as [ 0, 1, 2, 3, 4, 5, 6, 7 ].
> This would be fixed in next release.
> 
> In the meantime, as the default val of P1:0xA1 is 0x03, we hope to keep
> that value if there is no setting for this private property in DT.
> The caller in driver would be updated like this in next release.
> if (ov02a10->mipi_clock_tx_speed)
> 	ret = i2c_smbus_write_byte_data(...,...);

How did you pick the value in the example? And why do you believe it is
specific to a platform, and not e.g. a sensor mode?

> 
> > > +    allOf:
> > > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > > +      - enum: [ 3, 4 ]
> > > +
> > > +  # See ../video-interfaces.txt for details
> > > +  port:
> > > +    type: object
> > > +    additionalProperties: false
> > > +
> > > +    properties:
> > > +      endpoint:
> > > +        type: object
> > > +        additionalProperties: false
> > > +
> > > +        properties:
> > > +          remote-endpoint: true
> > > +          link-frequencies: true
> > > +
> > > +    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 {
> > > +        clock-frequency = <400000>;
> > > +        #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 = <3>;
> > > +
> > > +            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 {
> > > +                    remote-endpoint = <&mipi_in_wcam>;
> > > +                    link-frequencies = /bits/ 64 <390000000>;
> > > +                };
> > > +            };
> > > +        };
> > > +    };
> > > +
> > > +...
> > > 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
> > 
>
Dongchun Zhu May 7, 2020, 12:58 p.m. UTC | #5 
Hi Sakari,

Thanks for the review.

On Wed, 2020-05-06 at 14:21 +0300, Sakari Ailus wrote:
> Hi Dongchun,
> 
> On Tue, May 05, 2020 at 10:17:18PM +0800, Dongchun Zhu wrote:
> > Hi Sakari,
> > 
> > Thanks for the review.
> > 
> > On Tue, 2020-05-05 at 10:04 +0300, Sakari Ailus wrote:
> > > Hi Dongchun,
> > > 
> > > On Thu, Apr 30, 2020 at 04:09:23PM +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           | 148 +++++++++++++++++++++
> > > >  MAINTAINERS                                        |   7 +
> > > >  2 files changed, 155 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..2be4bd2
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > > > @@ -0,0 +1,148 @@
> > > > +# 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: devider 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 interface power supply.
> > > > +
> > > > +  avdd-supply:
> > > > +    description:
> > > > +      Definition of the regulator used as analog power supply.
> > > > +
> > > > +  dvdd-supply:
> > > > +    description:
> > > > +      Definition of the regulator used as digital power supply.
> > > > +
> > > > +  powerdown-gpios:
> > > > +    description:
> > > > +      The phandle and specifier for the GPIO that controls sensor powerdown.
> > > > +
> > > > +  reset-gpios:
> > > > +    description:
> > > > +      The phandle and specifier for the GPIO that controls sensor reset.
> > > > +
> > > > +  rotation:
> > > > +    description:
> > > > +      Definition of the sensor's placement, valid values are 0 and 180.
> > > > +    allOf:
> > > > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > > > +      - enum:
> > > > +          - 0    # Sensor Mounted Upright
> > > > +          - 180  # Sensor Mounted Upside Down
> > > > +
> > > > +  ovti,mipi-tx-speed:
> > > > +    description:
> > > > +      Indication of MIPI transmission speed select.
> > > 
> > > What exactly does this signify? And how do you come up with the number?
> > > 
> > 
> > Apologies for not addressing this number clear.
> > 
> > From the datasheet, P1:0xA1 register represents TX_SPEED_AREA_SEL with
> > the default val: 0x03.
> > The description of this RW register is as below:
> > Bit[2:0]: MIPI transmission speed select.
> > 
> > Thus the enum should be definited as [ 0, 1, 2, 3, 4, 5, 6, 7 ].
> > This would be fixed in next release.
> > 
> > In the meantime, as the default val of P1:0xA1 is 0x03, we hope to keep
> > that value if there is no setting for this private property in DT.
> > The caller in driver would be updated like this in next release.
> > if (ov02a10->mipi_clock_tx_speed)
> > 	ret = i2c_smbus_write_byte_data(...,...);
> 
> How did you pick the value in the example? And why do you believe it is
> specific to a platform, and not e.g. a sensor mode?
> 

We look into P1:0XA1, one register that defines MIPI transmission speed
select.
From the datasheet, we can get the possible values that could be set to
P1:0xA1.

Actually this register is an independent of sensor mode, it is just
included in sensor mode's register setting table.

In addition, this private DT Property is created to fix the MIPI test
failure. The register values are adjusted and verified from vendor to
make sensor signal meet MIPI specification.

> > 
> > > > +    allOf:
> > > > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > > > +      - enum: [ 3, 4 ]
> > > > +
> > > > +  # See ../video-interfaces.txt for details
> > > > +  port:
> > > > +    type: object
> > > > +    additionalProperties: false
> > > > +
> > > > +    properties:
> > > > +      endpoint:
> > > > +        type: object
> > > > +        additionalProperties: false
> > > > +
> > > > +        properties:
> > > > +          remote-endpoint: true
> > > > +          link-frequencies: true
> > > > +
> > > > +    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 {
> > > > +        clock-frequency = <400000>;
> > > > +        #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 = <3>;
> > > > +
> > > > +            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 {
> > > > +                    remote-endpoint = <&mipi_in_wcam>;
> > > > +                    link-frequencies = /bits/ 64 <390000000>;
> > > > +                };
> > > > +            };
> > > > +        };
> > > > +    };
> > > > +
> > > > +...
> > > > 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
> > > 
> > 
>
Dongchun Zhu May 7, 2020, 1:02 p.m. UTC | #6 
Hi Rob,

Thanks for the review.

On Tue, 2020-05-05 at 11:15 -0500, Rob Herring wrote:
> On Thu, Apr 30, 2020 at 04:09:23PM +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           | 148 +++++++++++++++++++++
> >  MAINTAINERS                                        |   7 +
> >  2 files changed, 155 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..2be4bd2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > @@ -0,0 +1,148 @@
> > +# 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: devider 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 interface power supply.
> > +
> > +  avdd-supply:
> > +    description:
> > +      Definition of the regulator used as analog power supply.
> > +
> > +  dvdd-supply:
> > +    description:
> > +      Definition of the regulator used as digital power supply.
> > +
> > +  powerdown-gpios:
> 
> maxItems: 1
> 

Thanks for the reminder, it would be fixed in next release.

> > +    description:
> > +      The phandle and specifier for the GPIO that controls sensor powerdown.
> 
> Can be dropped. Doesn't tell me anything specific to this device.
> 

Fixed in next release.

> > +
> > +  reset-gpios:
> 
> maxItems: 1
> 

Same as powerdown-gpios. We would just use maxItems only.

> > +    description:
> > +      The phandle and specifier for the GPIO that controls sensor reset.
> > +
> > +  rotation:
> > +    description:
> > +      Definition of the sensor's placement, valid values are 0 and 180.
> > +    allOf:
> > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > +      - enum:
> > +          - 0    # Sensor Mounted Upright
> > +          - 180  # Sensor Mounted Upside Down
> > +
> > +  ovti,mipi-tx-speed:
> > +    description:
> > +      Indication of MIPI transmission speed select.
> > +    allOf:
> > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > +      - enum: [ 3, 4 ]
> > +
> > +  # See ../video-interfaces.txt for details
> > +  port:
> > +    type: object
> > +    additionalProperties: false
> > +
> > +    properties:
> > +      endpoint:
> > +        type: object
> > +        additionalProperties: false
> > +
> > +        properties:
> > +          remote-endpoint: true
> > +          link-frequencies: true
> > +
> > +    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 {
> > +        clock-frequency = <400000>;
> > +        #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 = <3>;
> > +
> > +            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 {
> > +                    remote-endpoint = <&mipi_in_wcam>;
> > +                    link-frequencies = /bits/ 64 <390000000>;
> > +                };
> > +            };
> > +        };
> > +    };
> > +
> > +...
> > 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
Tomasz Figa May 7, 2020, 1:50 p.m. UTC | #7 
Hi Sakari and Dongchun,

On Thu, May 7, 2020 at 3:00 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
>
> Hi Sakari,
>
> Thanks for the review.
>
> On Wed, 2020-05-06 at 14:21 +0300, Sakari Ailus wrote:
> > Hi Dongchun,
> >
> > On Tue, May 05, 2020 at 10:17:18PM +0800, Dongchun Zhu wrote:
> > > Hi Sakari,
> > >
> > > Thanks for the review.
> > >
> > > On Tue, 2020-05-05 at 10:04 +0300, Sakari Ailus wrote:
> > > > Hi Dongchun,
> > > >
> > > > On Thu, Apr 30, 2020 at 04:09:23PM +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           | 148 +++++++++++++++++++++
> > > > >  MAINTAINERS                                        |   7 +
> > > > >  2 files changed, 155 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..2be4bd2
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > > > > @@ -0,0 +1,148 @@
> > > > > +# 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: devider 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 interface power supply.
> > > > > +
> > > > > +  avdd-supply:
> > > > > +    description:
> > > > > +      Definition of the regulator used as analog power supply.
> > > > > +
> > > > > +  dvdd-supply:
> > > > > +    description:
> > > > > +      Definition of the regulator used as digital power supply.
> > > > > +
> > > > > +  powerdown-gpios:
> > > > > +    description:
> > > > > +      The phandle and specifier for the GPIO that controls sensor powerdown.
> > > > > +
> > > > > +  reset-gpios:
> > > > > +    description:
> > > > > +      The phandle and specifier for the GPIO that controls sensor reset.
> > > > > +
> > > > > +  rotation:
> > > > > +    description:
> > > > > +      Definition of the sensor's placement, valid values are 0 and 180.
> > > > > +    allOf:
> > > > > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > > > > +      - enum:
> > > > > +          - 0    # Sensor Mounted Upright
> > > > > +          - 180  # Sensor Mounted Upside Down
> > > > > +
> > > > > +  ovti,mipi-tx-speed:
> > > > > +    description:
> > > > > +      Indication of MIPI transmission speed select.
> > > >
> > > > What exactly does this signify? And how do you come up with the number?
> > > >
> > >
> > > Apologies for not addressing this number clear.
> > >
> > > From the datasheet, P1:0xA1 register represents TX_SPEED_AREA_SEL with
> > > the default val: 0x03.
> > > The description of this RW register is as below:
> > > Bit[2:0]: MIPI transmission speed select.
> > >
> > > Thus the enum should be definited as [ 0, 1, 2, 3, 4, 5, 6, 7 ].
> > > This would be fixed in next release.
> > >
> > > In the meantime, as the default val of P1:0xA1 is 0x03, we hope to keep
> > > that value if there is no setting for this private property in DT.
> > > The caller in driver would be updated like this in next release.
> > > if (ov02a10->mipi_clock_tx_speed)
> > >     ret = i2c_smbus_write_byte_data(...,...);
> >
> > How did you pick the value in the example? And why do you believe it is
> > specific to a platform, and not e.g. a sensor mode?
> >
>
> We look into P1:0XA1, one register that defines MIPI transmission speed
> select.
> From the datasheet, we can get the possible values that could be set to
> P1:0xA1.
>
> Actually this register is an independent of sensor mode, it is just
> included in sensor mode's register setting table.
>
> In addition, this private DT Property is created to fix the MIPI test
> failure. The register values are adjusted and verified from vendor to
> make sensor signal meet MIPI specification.
>

In theory the value could depend on the mode, because different link
rate could impose different requirements for the physical interface.
In practice, we haven't seen any hardware that would require different
values for different modes.

Best regards,
Tomasz

> > >
> > > > > +    allOf:
> > > > > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > > > > +      - enum: [ 3, 4 ]
> > > > > +
> > > > > +  # See ../video-interfaces.txt for details
> > > > > +  port:
> > > > > +    type: object
> > > > > +    additionalProperties: false
> > > > > +
> > > > > +    properties:
> > > > > +      endpoint:
> > > > > +        type: object
> > > > > +        additionalProperties: false
> > > > > +
> > > > > +        properties:
> > > > > +          remote-endpoint: true
> > > > > +          link-frequencies: true
> > > > > +
> > > > > +    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 {
> > > > > +        clock-frequency = <400000>;
> > > > > +        #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 = <3>;
> > > > > +
> > > > > +            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 {
> > > > > +                    remote-endpoint = <&mipi_in_wcam>;
> > > > > +                    link-frequencies = /bits/ 64 <390000000>;
> > > > > +                };
> > > > > +            };
> > > > > +        };
> > > > > +    };
> > > > > +
> > > > > +...
> > > > > 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
> > > >
> > >
> >
>
Sakari Ailus May 7, 2020, 2:11 p.m. UTC | #8 
Hi Tomasz, Dongchun,

On Thu, May 07, 2020 at 03:50:40PM +0200, Tomasz Figa wrote:
> Hi Sakari and Dongchun,
> 
> On Thu, May 7, 2020 at 3:00 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> >
> > Hi Sakari,
> >
> > Thanks for the review.
> >
> > On Wed, 2020-05-06 at 14:21 +0300, Sakari Ailus wrote:
> > > Hi Dongchun,
> > >
> > > On Tue, May 05, 2020 at 10:17:18PM +0800, Dongchun Zhu wrote:
> > > > Hi Sakari,
> > > >
> > > > Thanks for the review.
> > > >
> > > > On Tue, 2020-05-05 at 10:04 +0300, Sakari Ailus wrote:
> > > > > Hi Dongchun,
> > > > >
> > > > > On Thu, Apr 30, 2020 at 04:09:23PM +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           | 148 +++++++++++++++++++++
> > > > > >  MAINTAINERS                                        |   7 +
> > > > > >  2 files changed, 155 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..2be4bd2
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > > > > > @@ -0,0 +1,148 @@
> > > > > > +# 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: devider 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 interface power supply.
> > > > > > +
> > > > > > +  avdd-supply:
> > > > > > +    description:
> > > > > > +      Definition of the regulator used as analog power supply.
> > > > > > +
> > > > > > +  dvdd-supply:
> > > > > > +    description:
> > > > > > +      Definition of the regulator used as digital power supply.
> > > > > > +
> > > > > > +  powerdown-gpios:
> > > > > > +    description:
> > > > > > +      The phandle and specifier for the GPIO that controls sensor powerdown.
> > > > > > +
> > > > > > +  reset-gpios:
> > > > > > +    description:
> > > > > > +      The phandle and specifier for the GPIO that controls sensor reset.
> > > > > > +
> > > > > > +  rotation:
> > > > > > +    description:
> > > > > > +      Definition of the sensor's placement, valid values are 0 and 180.
> > > > > > +    allOf:
> > > > > > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > > > > > +      - enum:
> > > > > > +          - 0    # Sensor Mounted Upright
> > > > > > +          - 180  # Sensor Mounted Upside Down
> > > > > > +
> > > > > > +  ovti,mipi-tx-speed:
> > > > > > +    description:
> > > > > > +      Indication of MIPI transmission speed select.
> > > > >
> > > > > What exactly does this signify? And how do you come up with the number?
> > > > >
> > > >
> > > > Apologies for not addressing this number clear.
> > > >
> > > > From the datasheet, P1:0xA1 register represents TX_SPEED_AREA_SEL with
> > > > the default val: 0x03.
> > > > The description of this RW register is as below:
> > > > Bit[2:0]: MIPI transmission speed select.
> > > >
> > > > Thus the enum should be definited as [ 0, 1, 2, 3, 4, 5, 6, 7 ].
> > > > This would be fixed in next release.
> > > >
> > > > In the meantime, as the default val of P1:0xA1 is 0x03, we hope to keep
> > > > that value if there is no setting for this private property in DT.
> > > > The caller in driver would be updated like this in next release.
> > > > if (ov02a10->mipi_clock_tx_speed)
> > > >     ret = i2c_smbus_write_byte_data(...,...);
> > >
> > > How did you pick the value in the example? And why do you believe it is
> > > specific to a platform, and not e.g. a sensor mode?
> > >
> >
> > We look into P1:0XA1, one register that defines MIPI transmission speed
> > select.
> > From the datasheet, we can get the possible values that could be set to
> > P1:0xA1.
> >
> > Actually this register is an independent of sensor mode, it is just
> > included in sensor mode's register setting table.
> >
> > In addition, this private DT Property is created to fix the MIPI test
> > failure. The register values are adjusted and verified from vendor to
> > make sensor signal meet MIPI specification.
> >
> 
> In theory the value could depend on the mode, because different link
> rate could impose different requirements for the physical interface.
> In practice, we haven't seen any hardware that would require different
> values for different modes.

The mode (possibly in conjunction with other information available to the
driver via V4L2 fwnode interface) precisely defines the parameters of the
CSI-2 bus --- apart from the possible exception of the bus timing related
parameters but this is not supported by the name of the parameter.

Therefore I don't see how this parameter, which supposedly is used to
determine the CSI-2 transmissions speed, could be board specific and thus
belong to DT.
Tomasz Figa May 7, 2020, 2:25 p.m. UTC | #9 
On Thu, May 7, 2020 at 4:12 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Tomasz, Dongchun,
>
> On Thu, May 07, 2020 at 03:50:40PM +0200, Tomasz Figa wrote:
> > Hi Sakari and Dongchun,
> >
> > On Thu, May 7, 2020 at 3:00 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > >
> > > Hi Sakari,
> > >
> > > Thanks for the review.
> > >
> > > On Wed, 2020-05-06 at 14:21 +0300, Sakari Ailus wrote:
> > > > Hi Dongchun,
> > > >
> > > > On Tue, May 05, 2020 at 10:17:18PM +0800, Dongchun Zhu wrote:
> > > > > Hi Sakari,
> > > > >
> > > > > Thanks for the review.
> > > > >
> > > > > On Tue, 2020-05-05 at 10:04 +0300, Sakari Ailus wrote:
> > > > > > Hi Dongchun,
> > > > > >
> > > > > > On Thu, Apr 30, 2020 at 04:09:23PM +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           | 148 +++++++++++++++++++++
> > > > > > >  MAINTAINERS                                        |   7 +
> > > > > > >  2 files changed, 155 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..2be4bd2
> > > > > > > --- /dev/null
> > > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > > > > > > @@ -0,0 +1,148 @@
> > > > > > > +# 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: devider 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 interface power supply.
> > > > > > > +
> > > > > > > +  avdd-supply:
> > > > > > > +    description:
> > > > > > > +      Definition of the regulator used as analog power supply.
> > > > > > > +
> > > > > > > +  dvdd-supply:
> > > > > > > +    description:
> > > > > > > +      Definition of the regulator used as digital power supply.
> > > > > > > +
> > > > > > > +  powerdown-gpios:
> > > > > > > +    description:
> > > > > > > +      The phandle and specifier for the GPIO that controls sensor powerdown.
> > > > > > > +
> > > > > > > +  reset-gpios:
> > > > > > > +    description:
> > > > > > > +      The phandle and specifier for the GPIO that controls sensor reset.
> > > > > > > +
> > > > > > > +  rotation:
> > > > > > > +    description:
> > > > > > > +      Definition of the sensor's placement, valid values are 0 and 180.
> > > > > > > +    allOf:
> > > > > > > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > > > > > > +      - enum:
> > > > > > > +          - 0    # Sensor Mounted Upright
> > > > > > > +          - 180  # Sensor Mounted Upside Down
> > > > > > > +
> > > > > > > +  ovti,mipi-tx-speed:
> > > > > > > +    description:
> > > > > > > +      Indication of MIPI transmission speed select.
> > > > > >
> > > > > > What exactly does this signify? And how do you come up with the number?
> > > > > >
> > > > >
> > > > > Apologies for not addressing this number clear.
> > > > >
> > > > > From the datasheet, P1:0xA1 register represents TX_SPEED_AREA_SEL with
> > > > > the default val: 0x03.
> > > > > The description of this RW register is as below:
> > > > > Bit[2:0]: MIPI transmission speed select.
> > > > >
> > > > > Thus the enum should be definited as [ 0, 1, 2, 3, 4, 5, 6, 7 ].
> > > > > This would be fixed in next release.
> > > > >
> > > > > In the meantime, as the default val of P1:0xA1 is 0x03, we hope to keep
> > > > > that value if there is no setting for this private property in DT.
> > > > > The caller in driver would be updated like this in next release.
> > > > > if (ov02a10->mipi_clock_tx_speed)
> > > > >     ret = i2c_smbus_write_byte_data(...,...);
> > > >
> > > > How did you pick the value in the example? And why do you believe it is
> > > > specific to a platform, and not e.g. a sensor mode?
> > > >
> > >
> > > We look into P1:0XA1, one register that defines MIPI transmission speed
> > > select.
> > > From the datasheet, we can get the possible values that could be set to
> > > P1:0xA1.
> > >
> > > Actually this register is an independent of sensor mode, it is just
> > > included in sensor mode's register setting table.
> > >
> > > In addition, this private DT Property is created to fix the MIPI test
> > > failure. The register values are adjusted and verified from vendor to
> > > make sensor signal meet MIPI specification.
> > >
> >
> > In theory the value could depend on the mode, because different link
> > rate could impose different requirements for the physical interface.
> > In practice, we haven't seen any hardware that would require different
> > values for different modes.
>
> The mode (possibly in conjunction with other information available to the
> driver via V4L2 fwnode interface) precisely defines the parameters of the
> CSI-2 bus --- apart from the possible exception of the bus timing related
> parameters but this is not supported by the name of the parameter.
>
> Therefore I don't see how this parameter, which supposedly is used to
> determine the CSI-2 transmissions speed, could be board specific and thus
> belong to DT.

According to the very imprecise information I have access to, it is
not about the CSI-2 bus itself, but rather some internal parameter of
the sensor's CSI interface. Unfortunately there isn't much information
on what this value exactly controls...

Best regards,
Tomasz
Dongchun Zhu May 8, 2020, 6:51 a.m. UTC | #10 
Hi Sakari, Tomasz,

On Thu, 2020-05-07 at 16:25 +0200, Tomasz Figa wrote:
> On Thu, May 7, 2020 at 4:12 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Tomasz, Dongchun,
> >
> > On Thu, May 07, 2020 at 03:50:40PM +0200, Tomasz Figa wrote:
> > > Hi Sakari and Dongchun,
> > >
> > > On Thu, May 7, 2020 at 3:00 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > > >
> > > > Hi Sakari,
> > > >
> > > > Thanks for the review.
> > > >
> > > > On Wed, 2020-05-06 at 14:21 +0300, Sakari Ailus wrote:
> > > > > Hi Dongchun,
> > > > >
> > > > > On Tue, May 05, 2020 at 10:17:18PM +0800, Dongchun Zhu wrote:
> > > > > > Hi Sakari,
> > > > > >
> > > > > > Thanks for the review.
> > > > > >
> > > > > > On Tue, 2020-05-05 at 10:04 +0300, Sakari Ailus wrote:
> > > > > > > Hi Dongchun,
> > > > > > >
> > > > > > > On Thu, Apr 30, 2020 at 04:09:23PM +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           | 148 +++++++++++++++++++++
> > > > > > > >  MAINTAINERS                                        |   7 +
> > > > > > > >  2 files changed, 155 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..2be4bd2
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > > > > > > > @@ -0,0 +1,148 @@
> > > > > > > > +# 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: devider 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 interface power supply.
> > > > > > > > +
> > > > > > > > +  avdd-supply:
> > > > > > > > +    description:
> > > > > > > > +      Definition of the regulator used as analog power supply.
> > > > > > > > +
> > > > > > > > +  dvdd-supply:
> > > > > > > > +    description:
> > > > > > > > +      Definition of the regulator used as digital power supply.
> > > > > > > > +
> > > > > > > > +  powerdown-gpios:
> > > > > > > > +    description:
> > > > > > > > +      The phandle and specifier for the GPIO that controls sensor powerdown.
> > > > > > > > +
> > > > > > > > +  reset-gpios:
> > > > > > > > +    description:
> > > > > > > > +      The phandle and specifier for the GPIO that controls sensor reset.
> > > > > > > > +
> > > > > > > > +  rotation:
> > > > > > > > +    description:
> > > > > > > > +      Definition of the sensor's placement, valid values are 0 and 180.
> > > > > > > > +    allOf:
> > > > > > > > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > > > > > > > +      - enum:
> > > > > > > > +          - 0    # Sensor Mounted Upright
> > > > > > > > +          - 180  # Sensor Mounted Upside Down
> > > > > > > > +
> > > > > > > > +  ovti,mipi-tx-speed:
> > > > > > > > +    description:
> > > > > > > > +      Indication of MIPI transmission speed select.
> > > > > > >
> > > > > > > What exactly does this signify? And how do you come up with the number?
> > > > > > >
> > > > > >
> > > > > > Apologies for not addressing this number clear.
> > > > > >
> > > > > > From the datasheet, P1:0xA1 register represents TX_SPEED_AREA_SEL with
> > > > > > the default val: 0x03.
> > > > > > The description of this RW register is as below:
> > > > > > Bit[2:0]: MIPI transmission speed select.
> > > > > >
> > > > > > Thus the enum should be definited as [ 0, 1, 2, 3, 4, 5, 6, 7 ].
> > > > > > This would be fixed in next release.
> > > > > >
> > > > > > In the meantime, as the default val of P1:0xA1 is 0x03, we hope to keep
> > > > > > that value if there is no setting for this private property in DT.
> > > > > > The caller in driver would be updated like this in next release.
> > > > > > if (ov02a10->mipi_clock_tx_speed)
> > > > > >     ret = i2c_smbus_write_byte_data(...,...);
> > > > >
> > > > > How did you pick the value in the example? And why do you believe it is
> > > > > specific to a platform, and not e.g. a sensor mode?
> > > > >
> > > >
> > > > We look into P1:0XA1, one register that defines MIPI transmission speed
> > > > select.
> > > > From the datasheet, we can get the possible values that could be set to
> > > > P1:0xA1.
> > > >
> > > > Actually this register is an independent of sensor mode, it is just
> > > > included in sensor mode's register setting table.
> > > >
> > > > In addition, this private DT Property is created to fix the MIPI test
> > > > failure. The register values are adjusted and verified from vendor to
> > > > make sensor signal meet MIPI specification.
> > > >
> > >
> > > In theory the value could depend on the mode, because different link
> > > rate could impose different requirements for the physical interface.
> > > In practice, we haven't seen any hardware that would require different
> > > values for different modes.
> >
> > The mode (possibly in conjunction with other information available to the
> > driver via V4L2 fwnode interface) precisely defines the parameters of the
> > CSI-2 bus --- apart from the possible exception of the bus timing related
> > parameters but this is not supported by the name of the parameter.
> >
> > Therefore I don't see how this parameter, which supposedly is used to
> > determine the CSI-2 transmissions speed, could be board specific and thus
> > belong to DT.
> 
> According to the very imprecise information I have access to, it is
> not about the CSI-2 bus itself, but rather some internal parameter of
> the sensor's CSI interface. Unfortunately there isn't much information
> on what this value exactly controls...
> 
> Best regards,
> Tomasz

Just got some feedback from OV vendor about this parameter.

P1:0xA1 is the register to control D-PHY timing setting based on bclk.
It is to adjust the MIPI clock voltage to improve the clock drive
capability, and has no affect on the transmission speed of MIPI data.

From vendor's perspective, P1:0xA1 depends upon the length of FPC of
camera module that used on the board. Considering the physical
connections for MIPI signals to user-facing camera are very different
between our 2 projects, it can be very difficult to find universal SI
parameters for both projects.

Thus here we create one new DT property to separate these tuning in
driver, to be more like project-specific.

More details about the register is as below.
P1:0xA1 val: 0x03 default
Case: 0  20MHz-30MHz
      1  30MHz-50MHz
      2  50MHz-75MHz
      3  75MHz-100MHz   (default, old DB setting use)
      4  100MHz-130MHz  (suggested, new DB setting use)
      5  Manual
So the value in the example should be [ 0, 1, 2, 3, 4, 5 ].

Additionally, P1:0xA1 is recommended to be set as 0x04 in the newest DB
setting. We would adjust the register in next release.
Sakari Ailus May 10, 2020, 10:35 p.m. UTC | #11 
Hi Dongchun,

On Fri, May 08, 2020 at 02:51:25PM +0800, Dongchun Zhu wrote:
> Hi Sakari, Tomasz,
> 
> On Thu, 2020-05-07 at 16:25 +0200, Tomasz Figa wrote:
> > On Thu, May 7, 2020 at 4:12 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Tomasz, Dongchun,
> > >
> > > On Thu, May 07, 2020 at 03:50:40PM +0200, Tomasz Figa wrote:
> > > > Hi Sakari and Dongchun,
> > > >
> > > > On Thu, May 7, 2020 at 3:00 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > > > >
> > > > > Hi Sakari,
> > > > >
> > > > > Thanks for the review.
> > > > >
> > > > > On Wed, 2020-05-06 at 14:21 +0300, Sakari Ailus wrote:
> > > > > > Hi Dongchun,
> > > > > >
> > > > > > On Tue, May 05, 2020 at 10:17:18PM +0800, Dongchun Zhu wrote:
> > > > > > > Hi Sakari,
> > > > > > >
> > > > > > > Thanks for the review.
> > > > > > >
> > > > > > > On Tue, 2020-05-05 at 10:04 +0300, Sakari Ailus wrote:
> > > > > > > > Hi Dongchun,
> > > > > > > >
> > > > > > > > On Thu, Apr 30, 2020 at 04:09:23PM +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           | 148 +++++++++++++++++++++
> > > > > > > > >  MAINTAINERS                                        |   7 +
> > > > > > > > >  2 files changed, 155 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..2be4bd2
> > > > > > > > > --- /dev/null
> > > > > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > > > > > > > > @@ -0,0 +1,148 @@
> > > > > > > > > +# 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: devider 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 interface power supply.
> > > > > > > > > +
> > > > > > > > > +  avdd-supply:
> > > > > > > > > +    description:
> > > > > > > > > +      Definition of the regulator used as analog power supply.
> > > > > > > > > +
> > > > > > > > > +  dvdd-supply:
> > > > > > > > > +    description:
> > > > > > > > > +      Definition of the regulator used as digital power supply.
> > > > > > > > > +
> > > > > > > > > +  powerdown-gpios:
> > > > > > > > > +    description:
> > > > > > > > > +      The phandle and specifier for the GPIO that controls sensor powerdown.
> > > > > > > > > +
> > > > > > > > > +  reset-gpios:
> > > > > > > > > +    description:
> > > > > > > > > +      The phandle and specifier for the GPIO that controls sensor reset.
> > > > > > > > > +
> > > > > > > > > +  rotation:
> > > > > > > > > +    description:
> > > > > > > > > +      Definition of the sensor's placement, valid values are 0 and 180.
> > > > > > > > > +    allOf:
> > > > > > > > > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > > > > > > > > +      - enum:
> > > > > > > > > +          - 0    # Sensor Mounted Upright
> > > > > > > > > +          - 180  # Sensor Mounted Upside Down
> > > > > > > > > +
> > > > > > > > > +  ovti,mipi-tx-speed:
> > > > > > > > > +    description:
> > > > > > > > > +      Indication of MIPI transmission speed select.
> > > > > > > >
> > > > > > > > What exactly does this signify? And how do you come up with the number?
> > > > > > > >
> > > > > > >
> > > > > > > Apologies for not addressing this number clear.
> > > > > > >
> > > > > > > From the datasheet, P1:0xA1 register represents TX_SPEED_AREA_SEL with
> > > > > > > the default val: 0x03.
> > > > > > > The description of this RW register is as below:
> > > > > > > Bit[2:0]: MIPI transmission speed select.
> > > > > > >
> > > > > > > Thus the enum should be definited as [ 0, 1, 2, 3, 4, 5, 6, 7 ].
> > > > > > > This would be fixed in next release.
> > > > > > >
> > > > > > > In the meantime, as the default val of P1:0xA1 is 0x03, we hope to keep
> > > > > > > that value if there is no setting for this private property in DT.
> > > > > > > The caller in driver would be updated like this in next release.
> > > > > > > if (ov02a10->mipi_clock_tx_speed)
> > > > > > >     ret = i2c_smbus_write_byte_data(...,...);
> > > > > >
> > > > > > How did you pick the value in the example? And why do you believe it is
> > > > > > specific to a platform, and not e.g. a sensor mode?
> > > > > >
> > > > >
> > > > > We look into P1:0XA1, one register that defines MIPI transmission speed
> > > > > select.
> > > > > From the datasheet, we can get the possible values that could be set to
> > > > > P1:0xA1.
> > > > >
> > > > > Actually this register is an independent of sensor mode, it is just
> > > > > included in sensor mode's register setting table.
> > > > >
> > > > > In addition, this private DT Property is created to fix the MIPI test
> > > > > failure. The register values are adjusted and verified from vendor to
> > > > > make sensor signal meet MIPI specification.
> > > > >
> > > >
> > > > In theory the value could depend on the mode, because different link
> > > > rate could impose different requirements for the physical interface.
> > > > In practice, we haven't seen any hardware that would require different
> > > > values for different modes.
> > >
> > > The mode (possibly in conjunction with other information available to the
> > > driver via V4L2 fwnode interface) precisely defines the parameters of the
> > > CSI-2 bus --- apart from the possible exception of the bus timing related
> > > parameters but this is not supported by the name of the parameter.
> > >
> > > Therefore I don't see how this parameter, which supposedly is used to
> > > determine the CSI-2 transmissions speed, could be board specific and thus
> > > belong to DT.
> > 
> > According to the very imprecise information I have access to, it is
> > not about the CSI-2 bus itself, but rather some internal parameter of
> > the sensor's CSI interface. Unfortunately there isn't much information
> > on what this value exactly controls...
> > 
> > Best regards,
> > Tomasz
> 
> Just got some feedback from OV vendor about this parameter.
> 
> P1:0xA1 is the register to control D-PHY timing setting based on bclk.
> It is to adjust the MIPI clock voltage to improve the clock drive
> capability, and has no affect on the transmission speed of MIPI data.
> 
> From vendor's perspective, P1:0xA1 depends upon the length of FPC of
> camera module that used on the board. Considering the physical
> connections for MIPI signals to user-facing camera are very different
> between our 2 projects, it can be very difficult to find universal SI
> parameters for both projects.

Are you using different values for this parameter on these two projects?

> 
> Thus here we create one new DT property to separate these tuning in
> driver, to be more like project-specific.
> 
> More details about the register is as below.
> P1:0xA1 val: 0x03 default
> Case: 0  20MHz-30MHz
>       1  30MHz-50MHz
>       2  50MHz-75MHz
>       3  75MHz-100MHz   (default, old DB setting use)
>       4  100MHz-130MHz  (suggested, new DB setting use)
>       5  Manual
> So the value in the example should be [ 0, 1, 2, 3, 4, 5 ].
> 
> Additionally, P1:0xA1 is recommended to be set as 0x04 in the newest DB
> setting. We would adjust the register in next release.

Thank you for digging into the issue.

Based on the above description, the parameter would depend on both the link
frequency and possibly also on wire length. I guess there's no harm from
using too strong drive, apart from perhaps power consumption? As in
principle this could be different for different sensor modes. Albeit I
don't remember seeing a sensor where such a parameter would have been
needed to be modified.
Dongchun Zhu May 11, 2020, 11:41 a.m. UTC | #12 
Hi Sakari,

On Mon, 2020-05-11 at 01:35 +0300, Sakari Ailus wrote:
> Hi Dongchun,
> 
> On Fri, May 08, 2020 at 02:51:25PM +0800, Dongchun Zhu wrote:
> > Hi Sakari, Tomasz,
> > 
> > On Thu, 2020-05-07 at 16:25 +0200, Tomasz Figa wrote:
> > > On Thu, May 7, 2020 at 4:12 PM Sakari Ailus
> > > <sakari.ailus@linux.intel.com> wrote:
> > > >
> > > > Hi Tomasz, Dongchun,
> > > >
> > > > On Thu, May 07, 2020 at 03:50:40PM +0200, Tomasz Figa wrote:
> > > > > Hi Sakari and Dongchun,
> > > > >
> > > > > On Thu, May 7, 2020 at 3:00 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > > > > >
> > > > > > Hi Sakari,
> > > > > >
> > > > > > Thanks for the review.
> > > > > >
> > > > > > On Wed, 2020-05-06 at 14:21 +0300, Sakari Ailus wrote:
> > > > > > > Hi Dongchun,
> > > > > > >
> > > > > > > On Tue, May 05, 2020 at 10:17:18PM +0800, Dongchun Zhu wrote:
> > > > > > > > Hi Sakari,
> > > > > > > >
> > > > > > > > Thanks for the review.
> > > > > > > >
> > > > > > > > On Tue, 2020-05-05 at 10:04 +0300, Sakari Ailus wrote:
> > > > > > > > > Hi Dongchun,
> > > > > > > > >
> > > > > > > > > On Thu, Apr 30, 2020 at 04:09:23PM +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           | 148 +++++++++++++++++++++
> > > > > > > > > >  MAINTAINERS                                        |   7 +
> > > > > > > > > >  2 files changed, 155 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..2be4bd2
> > > > > > > > > > --- /dev/null
> > > > > > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > > > > > > > > > @@ -0,0 +1,148 @@
> > > > > > > > > > +# 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: devider 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 interface power supply.
> > > > > > > > > > +
> > > > > > > > > > +  avdd-supply:
> > > > > > > > > > +    description:
> > > > > > > > > > +      Definition of the regulator used as analog power supply.
> > > > > > > > > > +
> > > > > > > > > > +  dvdd-supply:
> > > > > > > > > > +    description:
> > > > > > > > > > +      Definition of the regulator used as digital power supply.
> > > > > > > > > > +
> > > > > > > > > > +  powerdown-gpios:
> > > > > > > > > > +    description:
> > > > > > > > > > +      The phandle and specifier for the GPIO that controls sensor powerdown.
> > > > > > > > > > +
> > > > > > > > > > +  reset-gpios:
> > > > > > > > > > +    description:
> > > > > > > > > > +      The phandle and specifier for the GPIO that controls sensor reset.
> > > > > > > > > > +
> > > > > > > > > > +  rotation:
> > > > > > > > > > +    description:
> > > > > > > > > > +      Definition of the sensor's placement, valid values are 0 and 180.
> > > > > > > > > > +    allOf:
> > > > > > > > > > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > > > > > > > > > +      - enum:
> > > > > > > > > > +          - 0    # Sensor Mounted Upright
> > > > > > > > > > +          - 180  # Sensor Mounted Upside Down
> > > > > > > > > > +
> > > > > > > > > > +  ovti,mipi-tx-speed:
> > > > > > > > > > +    description:
> > > > > > > > > > +      Indication of MIPI transmission speed select.
> > > > > > > > >
> > > > > > > > > What exactly does this signify? And how do you come up with the number?
> > > > > > > > >
> > > > > > > >
> > > > > > > > Apologies for not addressing this number clear.
> > > > > > > >
> > > > > > > > From the datasheet, P1:0xA1 register represents TX_SPEED_AREA_SEL with
> > > > > > > > the default val: 0x03.
> > > > > > > > The description of this RW register is as below:
> > > > > > > > Bit[2:0]: MIPI transmission speed select.
> > > > > > > >
> > > > > > > > Thus the enum should be definited as [ 0, 1, 2, 3, 4, 5, 6, 7 ].
> > > > > > > > This would be fixed in next release.
> > > > > > > >
> > > > > > > > In the meantime, as the default val of P1:0xA1 is 0x03, we hope to keep
> > > > > > > > that value if there is no setting for this private property in DT.
> > > > > > > > The caller in driver would be updated like this in next release.
> > > > > > > > if (ov02a10->mipi_clock_tx_speed)
> > > > > > > >     ret = i2c_smbus_write_byte_data(...,...);
> > > > > > >
> > > > > > > How did you pick the value in the example? And why do you believe it is
> > > > > > > specific to a platform, and not e.g. a sensor mode?
> > > > > > >
> > > > > >
> > > > > > We look into P1:0XA1, one register that defines MIPI transmission speed
> > > > > > select.
> > > > > > From the datasheet, we can get the possible values that could be set to
> > > > > > P1:0xA1.
> > > > > >
> > > > > > Actually this register is an independent of sensor mode, it is just
> > > > > > included in sensor mode's register setting table.
> > > > > >
> > > > > > In addition, this private DT Property is created to fix the MIPI test
> > > > > > failure. The register values are adjusted and verified from vendor to
> > > > > > make sensor signal meet MIPI specification.
> > > > > >
> > > > >
> > > > > In theory the value could depend on the mode, because different link
> > > > > rate could impose different requirements for the physical interface.
> > > > > In practice, we haven't seen any hardware that would require different
> > > > > values for different modes.
> > > >
> > > > The mode (possibly in conjunction with other information available to the
> > > > driver via V4L2 fwnode interface) precisely defines the parameters of the
> > > > CSI-2 bus --- apart from the possible exception of the bus timing related
> > > > parameters but this is not supported by the name of the parameter.
> > > >
> > > > Therefore I don't see how this parameter, which supposedly is used to
> > > > determine the CSI-2 transmissions speed, could be board specific and thus
> > > > belong to DT.
> > > 
> > > According to the very imprecise information I have access to, it is
> > > not about the CSI-2 bus itself, but rather some internal parameter of
> > > the sensor's CSI interface. Unfortunately there isn't much information
> > > on what this value exactly controls...
> > > 
> > > Best regards,
> > > Tomasz
> > 
> > Just got some feedback from OV vendor about this parameter.
> > 
> > P1:0xA1 is the register to control D-PHY timing setting based on bclk.
> > It is to adjust the MIPI clock voltage to improve the clock drive
> > capability, and has no affect on the transmission speed of MIPI data.
> > 
> > From vendor's perspective, P1:0xA1 depends upon the length of FPC of
> > camera module that used on the board. Considering the physical
> > connections for MIPI signals to user-facing camera are very different
> > between our 2 projects, it can be very difficult to find universal SI
> > parameters for both projects.
> 
> Are you using different values for this parameter on these two projects?
> 

Yes. We're actually assigning two different values to this property.
One is 0x03, the other is 0x04.

> > 
> > Thus here we create one new DT property to separate these tuning in
> > driver, to be more like project-specific.
> > 
> > More details about the register is as below.
> > P1:0xA1 val: 0x03 default
> > Case: 0  20MHz-30MHz
> >       1  30MHz-50MHz
> >       2  50MHz-75MHz
> >       3  75MHz-100MHz   (default, old DB setting use)
> >       4  100MHz-130MHz  (suggested, new DB setting use)
> >       5  Manual
> > So the value in the example should be [ 0, 1, 2, 3, 4, 5 ].
> > 
> > Additionally, P1:0xA1 is recommended to be set as 0x04 in the newest DB
> > setting. We would adjust the register in next release.
> 
> Thank you for digging into the issue.
> 
> Based on the above description, the parameter would depend on both the link
> frequency and possibly also on wire length. I guess there's no harm from
> using too strong drive, apart from perhaps power consumption? As in
> principle this could be different for different sensor modes. Albeit I
> don't remember seeing a sensor where such a parameter would have been
> needed to be modified.
> 

This may be related to something about sensor fine tuning.
As OV vendor pointed out, the sensor chip provides such one property
that user could adjust based on their specific project.
Also, case 4 (0x04) setting is confirmed to have a little more power
consumption than case 3 (0x03).
Sakari Ailus July 15, 2020, 2:01 p.m. UTC | #13 
hi Dongchun,

On Mon, May 11, 2020 at 07:41:05PM +0800, Dongchun Zhu wrote:
> Hi Sakari,
> 
> On Mon, 2020-05-11 at 01:35 +0300, Sakari Ailus wrote:
> > Hi Dongchun,
> > 
> > On Fri, May 08, 2020 at 02:51:25PM +0800, Dongchun Zhu wrote:
> > > Hi Sakari, Tomasz,
> > > 
> > > On Thu, 2020-05-07 at 16:25 +0200, Tomasz Figa wrote:
> > > > On Thu, May 7, 2020 at 4:12 PM Sakari Ailus
> > > > <sakari.ailus@linux.intel.com> wrote:
> > > > >
> > > > > Hi Tomasz, Dongchun,
> > > > >
> > > > > On Thu, May 07, 2020 at 03:50:40PM +0200, Tomasz Figa wrote:
> > > > > > Hi Sakari and Dongchun,
> > > > > >
> > > > > > On Thu, May 7, 2020 at 3:00 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > > > > > >
> > > > > > > Hi Sakari,
> > > > > > >
> > > > > > > Thanks for the review.
> > > > > > >
> > > > > > > On Wed, 2020-05-06 at 14:21 +0300, Sakari Ailus wrote:
> > > > > > > > Hi Dongchun,
> > > > > > > >
> > > > > > > > On Tue, May 05, 2020 at 10:17:18PM +0800, Dongchun Zhu wrote:
> > > > > > > > > Hi Sakari,
> > > > > > > > >
> > > > > > > > > Thanks for the review.
> > > > > > > > >
> > > > > > > > > On Tue, 2020-05-05 at 10:04 +0300, Sakari Ailus wrote:
> > > > > > > > > > Hi Dongchun,
> > > > > > > > > >
> > > > > > > > > > On Thu, Apr 30, 2020 at 04:09:23PM +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           | 148 +++++++++++++++++++++
> > > > > > > > > > >  MAINTAINERS                                        |   7 +
> > > > > > > > > > >  2 files changed, 155 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..2be4bd2
> > > > > > > > > > > --- /dev/null
> > > > > > > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > > > > > > > > > > @@ -0,0 +1,148 @@
> > > > > > > > > > > +# 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: devider 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 interface power supply.
> > > > > > > > > > > +
> > > > > > > > > > > +  avdd-supply:
> > > > > > > > > > > +    description:
> > > > > > > > > > > +      Definition of the regulator used as analog power supply.
> > > > > > > > > > > +
> > > > > > > > > > > +  dvdd-supply:
> > > > > > > > > > > +    description:
> > > > > > > > > > > +      Definition of the regulator used as digital power supply.
> > > > > > > > > > > +
> > > > > > > > > > > +  powerdown-gpios:
> > > > > > > > > > > +    description:
> > > > > > > > > > > +      The phandle and specifier for the GPIO that controls sensor powerdown.
> > > > > > > > > > > +
> > > > > > > > > > > +  reset-gpios:
> > > > > > > > > > > +    description:
> > > > > > > > > > > +      The phandle and specifier for the GPIO that controls sensor reset.
> > > > > > > > > > > +
> > > > > > > > > > > +  rotation:
> > > > > > > > > > > +    description:
> > > > > > > > > > > +      Definition of the sensor's placement, valid values are 0 and 180.
> > > > > > > > > > > +    allOf:
> > > > > > > > > > > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > > > > > > > > > > +      - enum:
> > > > > > > > > > > +          - 0    # Sensor Mounted Upright
> > > > > > > > > > > +          - 180  # Sensor Mounted Upside Down
> > > > > > > > > > > +
> > > > > > > > > > > +  ovti,mipi-tx-speed:
> > > > > > > > > > > +    description:
> > > > > > > > > > > +      Indication of MIPI transmission speed select.
> > > > > > > > > >
> > > > > > > > > > What exactly does this signify? And how do you come up with the number?
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Apologies for not addressing this number clear.
> > > > > > > > >
> > > > > > > > > From the datasheet, P1:0xA1 register represents TX_SPEED_AREA_SEL with
> > > > > > > > > the default val: 0x03.
> > > > > > > > > The description of this RW register is as below:
> > > > > > > > > Bit[2:0]: MIPI transmission speed select.
> > > > > > > > >
> > > > > > > > > Thus the enum should be definited as [ 0, 1, 2, 3, 4, 5, 6, 7 ].
> > > > > > > > > This would be fixed in next release.
> > > > > > > > >
> > > > > > > > > In the meantime, as the default val of P1:0xA1 is 0x03, we hope to keep
> > > > > > > > > that value if there is no setting for this private property in DT.
> > > > > > > > > The caller in driver would be updated like this in next release.
> > > > > > > > > if (ov02a10->mipi_clock_tx_speed)
> > > > > > > > >     ret = i2c_smbus_write_byte_data(...,...);
> > > > > > > >
> > > > > > > > How did you pick the value in the example? And why do you believe it is
> > > > > > > > specific to a platform, and not e.g. a sensor mode?
> > > > > > > >
> > > > > > >
> > > > > > > We look into P1:0XA1, one register that defines MIPI transmission speed
> > > > > > > select.
> > > > > > > From the datasheet, we can get the possible values that could be set to
> > > > > > > P1:0xA1.
> > > > > > >
> > > > > > > Actually this register is an independent of sensor mode, it is just
> > > > > > > included in sensor mode's register setting table.
> > > > > > >
> > > > > > > In addition, this private DT Property is created to fix the MIPI test
> > > > > > > failure. The register values are adjusted and verified from vendor to
> > > > > > > make sensor signal meet MIPI specification.
> > > > > > >
> > > > > >
> > > > > > In theory the value could depend on the mode, because different link
> > > > > > rate could impose different requirements for the physical interface.
> > > > > > In practice, we haven't seen any hardware that would require different
> > > > > > values for different modes.
> > > > >
> > > > > The mode (possibly in conjunction with other information available to the
> > > > > driver via V4L2 fwnode interface) precisely defines the parameters of the
> > > > > CSI-2 bus --- apart from the possible exception of the bus timing related
> > > > > parameters but this is not supported by the name of the parameter.
> > > > >
> > > > > Therefore I don't see how this parameter, which supposedly is used to
> > > > > determine the CSI-2 transmissions speed, could be board specific and thus
> > > > > belong to DT.
> > > > 
> > > > According to the very imprecise information I have access to, it is
> > > > not about the CSI-2 bus itself, but rather some internal parameter of
> > > > the sensor's CSI interface. Unfortunately there isn't much information
> > > > on what this value exactly controls...
> > > > 
> > > > Best regards,
> > > > Tomasz
> > > 
> > > Just got some feedback from OV vendor about this parameter.
> > > 
> > > P1:0xA1 is the register to control D-PHY timing setting based on bclk.
> > > It is to adjust the MIPI clock voltage to improve the clock drive
> > > capability, and has no affect on the transmission speed of MIPI data.
> > > 
> > > From vendor's perspective, P1:0xA1 depends upon the length of FPC of
> > > camera module that used on the board. Considering the physical
> > > connections for MIPI signals to user-facing camera are very different
> > > between our 2 projects, it can be very difficult to find universal SI
> > > parameters for both projects.
> > 
> > Are you using different values for this parameter on these two projects?
> > 
> 
> Yes. We're actually assigning two different values to this property.
> One is 0x03, the other is 0x04.
> 
> > > 
> > > Thus here we create one new DT property to separate these tuning in
> > > driver, to be more like project-specific.
> > > 
> > > More details about the register is as below.
> > > P1:0xA1 val: 0x03 default
> > > Case: 0  20MHz-30MHz
> > >       1  30MHz-50MHz
> > >       2  50MHz-75MHz
> > >       3  75MHz-100MHz   (default, old DB setting use)
> > >       4  100MHz-130MHz  (suggested, new DB setting use)
> > >       5  Manual
> > > So the value in the example should be [ 0, 1, 2, 3, 4, 5 ].
> > > 
> > > Additionally, P1:0xA1 is recommended to be set as 0x04 in the newest DB
> > > setting. We would adjust the register in next release.
> > 
> > Thank you for digging into the issue.
> > 
> > Based on the above description, the parameter would depend on both the link
> > frequency and possibly also on wire length. I guess there's no harm from
> > using too strong drive, apart from perhaps power consumption? As in
> > principle this could be different for different sensor modes. Albeit I
> > don't remember seeing a sensor where such a parameter would have been
> > needed to be modified.
> > 
> 
> This may be related to something about sensor fine tuning.
> As OV vendor pointed out, the sensor chip provides such one property
> that user could adjust based on their specific project.
> Also, case 4 (0x04) setting is confirmed to have a little more power
> consumption than case 3 (0x03).

Apologies for bringing back an old topic --- the driver supports just a
single mode, using a specific data rate.

If another mode is added later on, will it continue to use the same value
for this? Based on the documentation, it seems that this is primarily
defined by the frequency of the bus, not by board design. Therefore putting
this to DT (and thus ignoring the frequency) appears wrong.
Tomasz Figa July 16, 2020, 2:57 p.m. UTC | #14 
On Wed, Jul 15, 2020 at 4:01 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> hi Dongchun,
>
> On Mon, May 11, 2020 at 07:41:05PM +0800, Dongchun Zhu wrote:
> > Hi Sakari,
> >
> > On Mon, 2020-05-11 at 01:35 +0300, Sakari Ailus wrote:
> > > Hi Dongchun,
> > >
> > > On Fri, May 08, 2020 at 02:51:25PM +0800, Dongchun Zhu wrote:
> > > > Hi Sakari, Tomasz,
> > > >
> > > > On Thu, 2020-05-07 at 16:25 +0200, Tomasz Figa wrote:
> > > > > On Thu, May 7, 2020 at 4:12 PM Sakari Ailus
> > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > >
> > > > > > Hi Tomasz, Dongchun,
> > > > > >
> > > > > > On Thu, May 07, 2020 at 03:50:40PM +0200, Tomasz Figa wrote:
> > > > > > > Hi Sakari and Dongchun,
> > > > > > >
> > > > > > > On Thu, May 7, 2020 at 3:00 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > > > > > > >
> > > > > > > > Hi Sakari,
> > > > > > > >
> > > > > > > > Thanks for the review.
> > > > > > > >
> > > > > > > > On Wed, 2020-05-06 at 14:21 +0300, Sakari Ailus wrote:
> > > > > > > > > Hi Dongchun,
> > > > > > > > >
> > > > > > > > > On Tue, May 05, 2020 at 10:17:18PM +0800, Dongchun Zhu wrote:
> > > > > > > > > > Hi Sakari,
> > > > > > > > > >
> > > > > > > > > > Thanks for the review.
> > > > > > > > > >
> > > > > > > > > > On Tue, 2020-05-05 at 10:04 +0300, Sakari Ailus wrote:
> > > > > > > > > > > Hi Dongchun,
> > > > > > > > > > >
> > > > > > > > > > > On Thu, Apr 30, 2020 at 04:09:23PM +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           | 148 +++++++++++++++++++++
> > > > > > > > > > > >  MAINTAINERS                                        |   7 +
> > > > > > > > > > > >  2 files changed, 155 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..2be4bd2
> > > > > > > > > > > > --- /dev/null
> > > > > > > > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > > > > > > > > > > > @@ -0,0 +1,148 @@
> > > > > > > > > > > > +# 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: devider 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 interface power supply.
> > > > > > > > > > > > +
> > > > > > > > > > > > +  avdd-supply:
> > > > > > > > > > > > +    description:
> > > > > > > > > > > > +      Definition of the regulator used as analog power supply.
> > > > > > > > > > > > +
> > > > > > > > > > > > +  dvdd-supply:
> > > > > > > > > > > > +    description:
> > > > > > > > > > > > +      Definition of the regulator used as digital power supply.
> > > > > > > > > > > > +
> > > > > > > > > > > > +  powerdown-gpios:
> > > > > > > > > > > > +    description:
> > > > > > > > > > > > +      The phandle and specifier for the GPIO that controls sensor powerdown.
> > > > > > > > > > > > +
> > > > > > > > > > > > +  reset-gpios:
> > > > > > > > > > > > +    description:
> > > > > > > > > > > > +      The phandle and specifier for the GPIO that controls sensor reset.
> > > > > > > > > > > > +
> > > > > > > > > > > > +  rotation:
> > > > > > > > > > > > +    description:
> > > > > > > > > > > > +      Definition of the sensor's placement, valid values are 0 and 180.
> > > > > > > > > > > > +    allOf:
> > > > > > > > > > > > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > > > > > > > > > > > +      - enum:
> > > > > > > > > > > > +          - 0    # Sensor Mounted Upright
> > > > > > > > > > > > +          - 180  # Sensor Mounted Upside Down
> > > > > > > > > > > > +
> > > > > > > > > > > > +  ovti,mipi-tx-speed:
> > > > > > > > > > > > +    description:
> > > > > > > > > > > > +      Indication of MIPI transmission speed select.
> > > > > > > > > > >
> > > > > > > > > > > What exactly does this signify? And how do you come up with the number?
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Apologies for not addressing this number clear.
> > > > > > > > > >
> > > > > > > > > > From the datasheet, P1:0xA1 register represents TX_SPEED_AREA_SEL with
> > > > > > > > > > the default val: 0x03.
> > > > > > > > > > The description of this RW register is as below:
> > > > > > > > > > Bit[2:0]: MIPI transmission speed select.
> > > > > > > > > >
> > > > > > > > > > Thus the enum should be definited as [ 0, 1, 2, 3, 4, 5, 6, 7 ].
> > > > > > > > > > This would be fixed in next release.
> > > > > > > > > >
> > > > > > > > > > In the meantime, as the default val of P1:0xA1 is 0x03, we hope to keep
> > > > > > > > > > that value if there is no setting for this private property in DT.
> > > > > > > > > > The caller in driver would be updated like this in next release.
> > > > > > > > > > if (ov02a10->mipi_clock_tx_speed)
> > > > > > > > > >     ret = i2c_smbus_write_byte_data(...,...);
> > > > > > > > >
> > > > > > > > > How did you pick the value in the example? And why do you believe it is
> > > > > > > > > specific to a platform, and not e.g. a sensor mode?
> > > > > > > > >
> > > > > > > >
> > > > > > > > We look into P1:0XA1, one register that defines MIPI transmission speed
> > > > > > > > select.
> > > > > > > > From the datasheet, we can get the possible values that could be set to
> > > > > > > > P1:0xA1.
> > > > > > > >
> > > > > > > > Actually this register is an independent of sensor mode, it is just
> > > > > > > > included in sensor mode's register setting table.
> > > > > > > >
> > > > > > > > In addition, this private DT Property is created to fix the MIPI test
> > > > > > > > failure. The register values are adjusted and verified from vendor to
> > > > > > > > make sensor signal meet MIPI specification.
> > > > > > > >
> > > > > > >
> > > > > > > In theory the value could depend on the mode, because different link
> > > > > > > rate could impose different requirements for the physical interface.
> > > > > > > In practice, we haven't seen any hardware that would require different
> > > > > > > values for different modes.
> > > > > >
> > > > > > The mode (possibly in conjunction with other information available to the
> > > > > > driver via V4L2 fwnode interface) precisely defines the parameters of the
> > > > > > CSI-2 bus --- apart from the possible exception of the bus timing related
> > > > > > parameters but this is not supported by the name of the parameter.
> > > > > >
> > > > > > Therefore I don't see how this parameter, which supposedly is used to
> > > > > > determine the CSI-2 transmissions speed, could be board specific and thus
> > > > > > belong to DT.
> > > > >
> > > > > According to the very imprecise information I have access to, it is
> > > > > not about the CSI-2 bus itself, but rather some internal parameter of
> > > > > the sensor's CSI interface. Unfortunately there isn't much information
> > > > > on what this value exactly controls...
> > > > >
> > > > > Best regards,
> > > > > Tomasz
> > > >
> > > > Just got some feedback from OV vendor about this parameter.
> > > >
> > > > P1:0xA1 is the register to control D-PHY timing setting based on bclk.
> > > > It is to adjust the MIPI clock voltage to improve the clock drive
> > > > capability, and has no affect on the transmission speed of MIPI data.
> > > >
> > > > From vendor's perspective, P1:0xA1 depends upon the length of FPC of
> > > > camera module that used on the board. Considering the physical
> > > > connections for MIPI signals to user-facing camera are very different
> > > > between our 2 projects, it can be very difficult to find universal SI
> > > > parameters for both projects.
> > >
> > > Are you using different values for this parameter on these two projects?
> > >
> >
> > Yes. We're actually assigning two different values to this property.
> > One is 0x03, the other is 0x04.
> >
> > > >
> > > > Thus here we create one new DT property to separate these tuning in
> > > > driver, to be more like project-specific.
> > > >
> > > > More details about the register is as below.
> > > > P1:0xA1 val: 0x03 default
> > > > Case: 0  20MHz-30MHz
> > > >       1  30MHz-50MHz
> > > >       2  50MHz-75MHz
> > > >       3  75MHz-100MHz   (default, old DB setting use)
> > > >       4  100MHz-130MHz  (suggested, new DB setting use)
> > > >       5  Manual
> > > > So the value in the example should be [ 0, 1, 2, 3, 4, 5 ].
> > > >
> > > > Additionally, P1:0xA1 is recommended to be set as 0x04 in the newest DB
> > > > setting. We would adjust the register in next release.
> > >
> > > Thank you for digging into the issue.
> > >
> > > Based on the above description, the parameter would depend on both the link
> > > frequency and possibly also on wire length. I guess there's no harm from
> > > using too strong drive, apart from perhaps power consumption? As in
> > > principle this could be different for different sensor modes. Albeit I
> > > don't remember seeing a sensor where such a parameter would have been
> > > needed to be modified.
> > >
> >
> > This may be related to something about sensor fine tuning.
> > As OV vendor pointed out, the sensor chip provides such one property
> > that user could adjust based on their specific project.
> > Also, case 4 (0x04) setting is confirmed to have a little more power
> > consumption than case 3 (0x03).
>
> Apologies for bringing back an old topic --- the driver supports just a
> single mode, using a specific data rate.
>
> If another mode is added later on, will it continue to use the same value
> for this? Based on the documentation, it seems that this is primarily
> defined by the frequency of the bus, not by board design. Therefore putting
> this to DT (and thus ignoring the frequency) appears wrong.

I don't think this is exactly implied by the frequency of the bus. The
values there are recommended for given frequency ranges, but there are
real cases where depending on the board different values are needed.

Best regards,
Tomasz
Dongchun Zhu July 20, 2020, 2:30 a.m. UTC | #15 
Hi Sakari, Tomasz,

Thanks for the review.

On Thu, 2020-07-16 at 16:57 +0200, Tomasz Figa wrote:
> On Wed, Jul 15, 2020 at 4:01 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > hi Dongchun,
> >
> > On Mon, May 11, 2020 at 07:41:05PM +0800, Dongchun Zhu wrote:
> > > Hi Sakari,
> > >
> > > On Mon, 2020-05-11 at 01:35 +0300, Sakari Ailus wrote:
> > > > Hi Dongchun,
> > > >
> > > > On Fri, May 08, 2020 at 02:51:25PM +0800, Dongchun Zhu wrote:
> > > > > Hi Sakari, Tomasz,
> > > > >
> > > > > On Thu, 2020-05-07 at 16:25 +0200, Tomasz Figa wrote:
> > > > > > On Thu, May 7, 2020 at 4:12 PM Sakari Ailus
> > > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > > >
> > > > > > > Hi Tomasz, Dongchun,
> > > > > > >
> > > > > > > On Thu, May 07, 2020 at 03:50:40PM +0200, Tomasz Figa wrote:
> > > > > > > > Hi Sakari and Dongchun,
> > > > > > > >
> > > > > > > > On Thu, May 7, 2020 at 3:00 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > > > > > > > >
> > > > > > > > > Hi Sakari,
> > > > > > > > >
> > > > > > > > > Thanks for the review.
> > > > > > > > >
> > > > > > > > > On Wed, 2020-05-06 at 14:21 +0300, Sakari Ailus wrote:
> > > > > > > > > > Hi Dongchun,
> > > > > > > > > >
> > > > > > > > > > On Tue, May 05, 2020 at 10:17:18PM +0800, Dongchun Zhu wrote:
> > > > > > > > > > > Hi Sakari,
> > > > > > > > > > >
> > > > > > > > > > > Thanks for the review.
> > > > > > > > > > >
> > > > > > > > > > > On Tue, 2020-05-05 at 10:04 +0300, Sakari Ailus wrote:
> > > > > > > > > > > > Hi Dongchun,
> > > > > > > > > > > >
> > > > > > > > > > > > On Thu, Apr 30, 2020 at 04:09:23PM +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           | 148 +++++++++++++++++++++
> > > > > > > > > > > > >  MAINTAINERS                                        |   7 +
> > > > > > > > > > > > >  2 files changed, 155 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..2be4bd2
> > > > > > > > > > > > > --- /dev/null
> > > > > > > > > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > > > > > > > > > > > > @@ -0,0 +1,148 @@
> > > > > > > > > > > > > +# 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: devider 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 interface power supply.
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +  avdd-supply:
> > > > > > > > > > > > > +    description:
> > > > > > > > > > > > > +      Definition of the regulator used as analog power supply.
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +  dvdd-supply:
> > > > > > > > > > > > > +    description:
> > > > > > > > > > > > > +      Definition of the regulator used as digital power supply.
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +  powerdown-gpios:
> > > > > > > > > > > > > +    description:
> > > > > > > > > > > > > +      The phandle and specifier for the GPIO that controls sensor powerdown.
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +  reset-gpios:
> > > > > > > > > > > > > +    description:
> > > > > > > > > > > > > +      The phandle and specifier for the GPIO that controls sensor reset.
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +  rotation:
> > > > > > > > > > > > > +    description:
> > > > > > > > > > > > > +      Definition of the sensor's placement, valid values are 0 and 180.
> > > > > > > > > > > > > +    allOf:
> > > > > > > > > > > > > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > > > > > > > > > > > > +      - enum:
> > > > > > > > > > > > > +          - 0    # Sensor Mounted Upright
> > > > > > > > > > > > > +          - 180  # Sensor Mounted Upside Down
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +  ovti,mipi-tx-speed:
> > > > > > > > > > > > > +    description:
> > > > > > > > > > > > > +      Indication of MIPI transmission speed select.
> > > > > > > > > > > >
> > > > > > > > > > > > What exactly does this signify? And how do you come up with the number?
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Apologies for not addressing this number clear.
> > > > > > > > > > >
> > > > > > > > > > > From the datasheet, P1:0xA1 register represents TX_SPEED_AREA_SEL with
> > > > > > > > > > > the default val: 0x03.
> > > > > > > > > > > The description of this RW register is as below:
> > > > > > > > > > > Bit[2:0]: MIPI transmission speed select.
> > > > > > > > > > >
> > > > > > > > > > > Thus the enum should be definited as [ 0, 1, 2, 3, 4, 5, 6, 7 ].
> > > > > > > > > > > This would be fixed in next release.
> > > > > > > > > > >
> > > > > > > > > > > In the meantime, as the default val of P1:0xA1 is 0x03, we hope to keep
> > > > > > > > > > > that value if there is no setting for this private property in DT.
> > > > > > > > > > > The caller in driver would be updated like this in next release.
> > > > > > > > > > > if (ov02a10->mipi_clock_tx_speed)
> > > > > > > > > > >     ret = i2c_smbus_write_byte_data(...,...);
> > > > > > > > > >
> > > > > > > > > > How did you pick the value in the example? And why do you believe it is
> > > > > > > > > > specific to a platform, and not e.g. a sensor mode?
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > We look into P1:0XA1, one register that defines MIPI transmission speed
> > > > > > > > > select.
> > > > > > > > > From the datasheet, we can get the possible values that could be set to
> > > > > > > > > P1:0xA1.
> > > > > > > > >
> > > > > > > > > Actually this register is an independent of sensor mode, it is just
> > > > > > > > > included in sensor mode's register setting table.
> > > > > > > > >
> > > > > > > > > In addition, this private DT Property is created to fix the MIPI test
> > > > > > > > > failure. The register values are adjusted and verified from vendor to
> > > > > > > > > make sensor signal meet MIPI specification.
> > > > > > > > >
> > > > > > > >
> > > > > > > > In theory the value could depend on the mode, because different link
> > > > > > > > rate could impose different requirements for the physical interface.
> > > > > > > > In practice, we haven't seen any hardware that would require different
> > > > > > > > values for different modes.
> > > > > > >
> > > > > > > The mode (possibly in conjunction with other information available to the
> > > > > > > driver via V4L2 fwnode interface) precisely defines the parameters of the
> > > > > > > CSI-2 bus --- apart from the possible exception of the bus timing related
> > > > > > > parameters but this is not supported by the name of the parameter.
> > > > > > >
> > > > > > > Therefore I don't see how this parameter, which supposedly is used to
> > > > > > > determine the CSI-2 transmissions speed, could be board specific and thus
> > > > > > > belong to DT.
> > > > > >
> > > > > > According to the very imprecise information I have access to, it is
> > > > > > not about the CSI-2 bus itself, but rather some internal parameter of
> > > > > > the sensor's CSI interface. Unfortunately there isn't much information
> > > > > > on what this value exactly controls...
> > > > > >
> > > > > > Best regards,
> > > > > > Tomasz
> > > > >
> > > > > Just got some feedback from OV vendor about this parameter.
> > > > >
> > > > > P1:0xA1 is the register to control D-PHY timing setting based on bclk.
> > > > > It is to adjust the MIPI clock voltage to improve the clock drive
> > > > > capability, and has no affect on the transmission speed of MIPI data.
> > > > >
> > > > > From vendor's perspective, P1:0xA1 depends upon the length of FPC of
> > > > > camera module that used on the board. Considering the physical
> > > > > connections for MIPI signals to user-facing camera are very different
> > > > > between our 2 projects, it can be very difficult to find universal SI
> > > > > parameters for both projects.
> > > >
> > > > Are you using different values for this parameter on these two projects?
> > > >
> > >
> > > Yes. We're actually assigning two different values to this property.
> > > One is 0x03, the other is 0x04.
> > >
> > > > >
> > > > > Thus here we create one new DT property to separate these tuning in
> > > > > driver, to be more like project-specific.
> > > > >
> > > > > More details about the register is as below.
> > > > > P1:0xA1 val: 0x03 default
> > > > > Case: 0  20MHz-30MHz
> > > > >       1  30MHz-50MHz
> > > > >       2  50MHz-75MHz
> > > > >       3  75MHz-100MHz   (default, old DB setting use)
> > > > >       4  100MHz-130MHz  (suggested, new DB setting use)
> > > > >       5  Manual
> > > > > So the value in the example should be [ 0, 1, 2, 3, 4, 5 ].
> > > > >
> > > > > Additionally, P1:0xA1 is recommended to be set as 0x04 in the newest DB
> > > > > setting. We would adjust the register in next release.
> > > >
> > > > Thank you for digging into the issue.
> > > >
> > > > Based on the above description, the parameter would depend on both the link
> > > > frequency and possibly also on wire length. I guess there's no harm from
> > > > using too strong drive, apart from perhaps power consumption? As in
> > > > principle this could be different for different sensor modes. Albeit I
> > > > don't remember seeing a sensor where such a parameter would have been
> > > > needed to be modified.
> > > >
> > >
> > > This may be related to something about sensor fine tuning.
> > > As OV vendor pointed out, the sensor chip provides such one property
> > > that user could adjust based on their specific project.
> > > Also, case 4 (0x04) setting is confirmed to have a little more power
> > > consumption than case 3 (0x03).
> >
> > Apologies for bringing back an old topic --- the driver supports just a
> > single mode, using a specific data rate.
> >
> > If another mode is added later on, will it continue to use the same value
> > for this? Based on the documentation, it seems that this is primarily
> > defined by the frequency of the bus, not by board design. Therefore putting
> > this to DT (and thus ignoring the frequency) appears wrong.
> 
> I don't think this is exactly implied by the frequency of the bus. The
> values there are recommended for given frequency ranges, but there are
> real cases where depending on the board different values are needed.
> 

Sorry for coming late.
For the reg P1: 0xA1, I re-confirmed the setting of the param with OVT.
The replies are as follow.
1. P1:0xA1 is one register to control D-PHY timing parameters based on
bclk. Its setting shall match the MIPI CSI-2 timing clock.
2. For one another scenario, if MIPI pixel rate(link frequency) differs
between scenarios, the setting of this parameter may also be different.
3. In one special case, we may also need to adjust the value, even for
the same scenario, such as the failure of a certain MIPI test.

From my perspective, temporally we don't plan to have a different
scenario for OV02A10, as the current resolution(1600x1200) is near to
the lower limit of most smart mobile devices.
In the meantime, considering the difference of the physical connections
for MIPI signals to user-facing camera between our 2 projects, it seems
to be very difficult to find universal SI parameters for both projects.
So for this case, I wonder whether we could reserve this private
property to maintain such flexibility.

> Best regards,
> Tomasz
Dongchun Zhu Aug. 25, 2020, 1:53 a.m. UTC | #16 
Hi Sakari,

Pardon, have you noticed my rely below?
Please let us know of any questions or suggestions you have.

On Mon, 2020-07-20 at 10:30 +0800, Dongchun Zhu wrote:
> Hi Sakari, Tomasz,
> 
> Thanks for the review.
> 
> On Thu, 2020-07-16 at 16:57 +0200, Tomasz Figa wrote:
> > On Wed, Jul 15, 2020 at 4:01 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > hi Dongchun,
> > >
> > > On Mon, May 11, 2020 at 07:41:05PM +0800, Dongchun Zhu wrote:
> > > > Hi Sakari,
> > > >
> > > > On Mon, 2020-05-11 at 01:35 +0300, Sakari Ailus wrote:
> > > > > Hi Dongchun,
> > > > >
> > > > > On Fri, May 08, 2020 at 02:51:25PM +0800, Dongchun Zhu wrote:
> > > > > > Hi Sakari, Tomasz,
> > > > > >
> > > > > > On Thu, 2020-05-07 at 16:25 +0200, Tomasz Figa wrote:
> > > > > > > On Thu, May 7, 2020 at 4:12 PM Sakari Ailus
> > > > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > > > >
> > > > > > > > Hi Tomasz, Dongchun,
> > > > > > > >
> > > > > > > > On Thu, May 07, 2020 at 03:50:40PM +0200, Tomasz Figa wrote:
> > > > > > > > > Hi Sakari and Dongchun,
> > > > > > > > >
> > > > > > > > > On Thu, May 7, 2020 at 3:00 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Sakari,
> > > > > > > > > >
> > > > > > > > > > Thanks for the review.
> > > > > > > > > >
> > > > > > > > > > On Wed, 2020-05-06 at 14:21 +0300, Sakari Ailus wrote:
> > > > > > > > > > > Hi Dongchun,
> > > > > > > > > > >
> > > > > > > > > > > On Tue, May 05, 2020 at 10:17:18PM +0800, Dongchun Zhu wrote:
> > > > > > > > > > > > Hi Sakari,
> > > > > > > > > > > >
> > > > > > > > > > > > Thanks for the review.
> > > > > > > > > > > >
> > > > > > > > > > > > On Tue, 2020-05-05 at 10:04 +0300, Sakari Ailus wrote:
> > > > > > > > > > > > > Hi Dongchun,
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Thu, Apr 30, 2020 at 04:09:23PM +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           | 148 +++++++++++++++++++++
> > > > > > > > > > > > > >  MAINTAINERS                                        |   7 +
> > > > > > > > > > > > > >  2 files changed, 155 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..2be4bd2
> > > > > > > > > > > > > > --- /dev/null
> > > > > > > > > > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > > > > > > > > > > > > > @@ -0,0 +1,148 @@
> > > > > > > > > > > > > > +# 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: devider 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 interface power supply.
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +  avdd-supply:
> > > > > > > > > > > > > > +    description:
> > > > > > > > > > > > > > +      Definition of the regulator used as analog power supply.
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +  dvdd-supply:
> > > > > > > > > > > > > > +    description:
> > > > > > > > > > > > > > +      Definition of the regulator used as digital power supply.
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +  powerdown-gpios:
> > > > > > > > > > > > > > +    description:
> > > > > > > > > > > > > > +      The phandle and specifier for the GPIO that controls sensor powerdown.
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +  reset-gpios:
> > > > > > > > > > > > > > +    description:
> > > > > > > > > > > > > > +      The phandle and specifier for the GPIO that controls sensor reset.
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +  rotation:
> > > > > > > > > > > > > > +    description:
> > > > > > > > > > > > > > +      Definition of the sensor's placement, valid values are 0 and 180.
> > > > > > > > > > > > > > +    allOf:
> > > > > > > > > > > > > > +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> > > > > > > > > > > > > > +      - enum:
> > > > > > > > > > > > > > +          - 0    # Sensor Mounted Upright
> > > > > > > > > > > > > > +          - 180  # Sensor Mounted Upside Down
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +  ovti,mipi-tx-speed:
> > > > > > > > > > > > > > +    description:
> > > > > > > > > > > > > > +      Indication of MIPI transmission speed select.
> > > > > > > > > > > > >
> > > > > > > > > > > > > What exactly does this signify? And how do you come up with the number?
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > Apologies for not addressing this number clear.
> > > > > > > > > > > >
> > > > > > > > > > > > From the datasheet, P1:0xA1 register represents TX_SPEED_AREA_SEL with
> > > > > > > > > > > > the default val: 0x03.
> > > > > > > > > > > > The description of this RW register is as below:
> > > > > > > > > > > > Bit[2:0]: MIPI transmission speed select.
> > > > > > > > > > > >
> > > > > > > > > > > > Thus the enum should be definited as [ 0, 1, 2, 3, 4, 5, 6, 7 ].
> > > > > > > > > > > > This would be fixed in next release.
> > > > > > > > > > > >
> > > > > > > > > > > > In the meantime, as the default val of P1:0xA1 is 0x03, we hope to keep
> > > > > > > > > > > > that value if there is no setting for this private property in DT.
> > > > > > > > > > > > The caller in driver would be updated like this in next release.
> > > > > > > > > > > > if (ov02a10->mipi_clock_tx_speed)
> > > > > > > > > > > >     ret = i2c_smbus_write_byte_data(...,...);
> > > > > > > > > > >
> > > > > > > > > > > How did you pick the value in the example? And why do you believe it is
> > > > > > > > > > > specific to a platform, and not e.g. a sensor mode?
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > We look into P1:0XA1, one register that defines MIPI transmission speed
> > > > > > > > > > select.
> > > > > > > > > > From the datasheet, we can get the possible values that could be set to
> > > > > > > > > > P1:0xA1.
> > > > > > > > > >
> > > > > > > > > > Actually this register is an independent of sensor mode, it is just
> > > > > > > > > > included in sensor mode's register setting table.
> > > > > > > > > >
> > > > > > > > > > In addition, this private DT Property is created to fix the MIPI test
> > > > > > > > > > failure. The register values are adjusted and verified from vendor to
> > > > > > > > > > make sensor signal meet MIPI specification.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > In theory the value could depend on the mode, because different link
> > > > > > > > > rate could impose different requirements for the physical interface.
> > > > > > > > > In practice, we haven't seen any hardware that would require different
> > > > > > > > > values for different modes.
> > > > > > > >
> > > > > > > > The mode (possibly in conjunction with other information available to the
> > > > > > > > driver via V4L2 fwnode interface) precisely defines the parameters of the
> > > > > > > > CSI-2 bus --- apart from the possible exception of the bus timing related
> > > > > > > > parameters but this is not supported by the name of the parameter.
> > > > > > > >
> > > > > > > > Therefore I don't see how this parameter, which supposedly is used to
> > > > > > > > determine the CSI-2 transmissions speed, could be board specific and thus
> > > > > > > > belong to DT.
> > > > > > >
> > > > > > > According to the very imprecise information I have access to, it is
> > > > > > > not about the CSI-2 bus itself, but rather some internal parameter of
> > > > > > > the sensor's CSI interface. Unfortunately there isn't much information
> > > > > > > on what this value exactly controls...
> > > > > > >
> > > > > > > Best regards,
> > > > > > > Tomasz
> > > > > >
> > > > > > Just got some feedback from OV vendor about this parameter.
> > > > > >
> > > > > > P1:0xA1 is the register to control D-PHY timing setting based on bclk.
> > > > > > It is to adjust the MIPI clock voltage to improve the clock drive
> > > > > > capability, and has no affect on the transmission speed of MIPI data.
> > > > > >
> > > > > > From vendor's perspective, P1:0xA1 depends upon the length of FPC of
> > > > > > camera module that used on the board. Considering the physical
> > > > > > connections for MIPI signals to user-facing camera are very different
> > > > > > between our 2 projects, it can be very difficult to find universal SI
> > > > > > parameters for both projects.
> > > > >
> > > > > Are you using different values for this parameter on these two projects?
> > > > >
> > > >
> > > > Yes. We're actually assigning two different values to this property.
> > > > One is 0x03, the other is 0x04.
> > > >
> > > > > >
> > > > > > Thus here we create one new DT property to separate these tuning in
> > > > > > driver, to be more like project-specific.
> > > > > >
> > > > > > More details about the register is as below.
> > > > > > P1:0xA1 val: 0x03 default
> > > > > > Case: 0  20MHz-30MHz
> > > > > >       1  30MHz-50MHz
> > > > > >       2  50MHz-75MHz
> > > > > >       3  75MHz-100MHz   (default, old DB setting use)
> > > > > >       4  100MHz-130MHz  (suggested, new DB setting use)
> > > > > >       5  Manual
> > > > > > So the value in the example should be [ 0, 1, 2, 3, 4, 5 ].
> > > > > >
> > > > > > Additionally, P1:0xA1 is recommended to be set as 0x04 in the newest DB
> > > > > > setting. We would adjust the register in next release.
> > > > >
> > > > > Thank you for digging into the issue.
> > > > >
> > > > > Based on the above description, the parameter would depend on both the link
> > > > > frequency and possibly also on wire length. I guess there's no harm from
> > > > > using too strong drive, apart from perhaps power consumption? As in
> > > > > principle this could be different for different sensor modes. Albeit I
> > > > > don't remember seeing a sensor where such a parameter would have been
> > > > > needed to be modified.
> > > > >
> > > >
> > > > This may be related to something about sensor fine tuning.
> > > > As OV vendor pointed out, the sensor chip provides such one property
> > > > that user could adjust based on their specific project.
> > > > Also, case 4 (0x04) setting is confirmed to have a little more power
> > > > consumption than case 3 (0x03).
> > >
> > > Apologies for bringing back an old topic --- the driver supports just a
> > > single mode, using a specific data rate.
> > >
> > > If another mode is added later on, will it continue to use the same value
> > > for this? Based on the documentation, it seems that this is primarily
> > > defined by the frequency of the bus, not by board design. Therefore putting
> > > this to DT (and thus ignoring the frequency) appears wrong.
> > 
> > I don't think this is exactly implied by the frequency of the bus. The
> > values there are recommended for given frequency ranges, but there are
> > real cases where depending on the board different values are needed.
> > 
> 
> Sorry for coming late.
> For the reg P1: 0xA1, I re-confirmed the setting of the param with OVT.
> The replies are as follow.
> 1. P1:0xA1 is one register to control D-PHY timing parameters based on
> bclk. Its setting shall match the MIPI CSI-2 timing clock.
> 2. For one another scenario, if MIPI pixel rate(link frequency) differs
> between scenarios, the setting of this parameter may also be different.
> 3. In one special case, we may also need to adjust the value, even for
> the same scenario, such as the failure of a certain MIPI test.
> 
> From my perspective, temporally we don't plan to have a different
> scenario for OV02A10, as the current resolution(1600x1200) is near to
> the lower limit of most smart mobile devices.
> In the meantime, considering the difference of the physical connections
> for MIPI signals to user-facing camera between our 2 projects, it seems
> to be very difficult to find universal SI parameters for both projects.
> So for this case, I wonder whether we could reserve this private
> property to maintain such flexibility.
> 
> > Best regards,
> > Tomasz
> 
>
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..2be4bd2
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
@@ -0,0 +1,148 @@ 
+# 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: devider 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 interface power supply.
+
+  avdd-supply:
+    description:
+      Definition of the regulator used as analog power supply.
+
+  dvdd-supply:
+    description:
+      Definition of the regulator used as digital power supply.
+
+  powerdown-gpios:
+    description:
+      The phandle and specifier for the GPIO that controls sensor powerdown.
+
+  reset-gpios:
+    description:
+      The phandle and specifier for the GPIO that controls sensor reset.
+
+  rotation:
+    description:
+      Definition of the sensor's placement, valid values are 0 and 180.
+    allOf:
+      - $ref: "/schemas/types.yaml#/definitions/uint32"
+      - enum:
+          - 0    # Sensor Mounted Upright
+          - 180  # Sensor Mounted Upside Down
+
+  ovti,mipi-tx-speed:
+    description:
+      Indication of MIPI transmission speed select.
+    allOf:
+      - $ref: "/schemas/types.yaml#/definitions/uint32"
+      - enum: [ 3, 4 ]
+
+  # See ../video-interfaces.txt for details
+  port:
+    type: object
+    additionalProperties: false
+
+    properties:
+      endpoint:
+        type: object
+        additionalProperties: false
+
+        properties:
+          remote-endpoint: true
+          link-frequencies: true
+
+    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 {
+        clock-frequency = <400000>;
+        #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 = <3>;
+
+            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 {
+                    remote-endpoint = <&mipi_in_wcam>;
+                    link-frequencies = /bits/ 64 <390000000>;
+                };
+            };
+        };
+    };
+
+...
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