[PATCH 0/2] Add Omnivision OV4689 image sensor driver

[PATCH 0/2] Add Omnivision OV4689 image sensor driver

[Mikhail Rudenko]

Hello,

this series implements support for Omnivision OV4689 image
sensor. The Omnivision OV4689 is a high performance, 1/3-inch, 4
megapixel image sensor. Ihis chip supports high frame rate speeds up
to 90 fps at 2688x1520 resolution. It is programmable through an I2C
interface, and sensor output is sent via 1/2/4 lane MIPI CSI-2
connection.

The driver is based on Rockchip BSP kernel [1]. It implements 4-lane CSI-2
and single 2688x1520 @ 30 fps mode. The driver was tested on Rockchip
3399-based FriendlyElec NanoPi M4 board with MCAM400 camera module.

While porting the driver, I stumbled upon two issues:

(1) In the original driver, horizontal total size (HTS) was set to a
value (2584) lower then the frame width (2688), resulting in negative
hblank. In this driver, I increased HTS to 2688, but fps dropped from
29.88 to 28.73. What is the preferred way to handle this?

(2) The original driver exposes analog gain range 0x0 - 0x7ff, but the
gain is not linear across that range. Instead, it is piecewise linear
(and discontinuous). 0x0-0xff register values result in 0x-2x gain,
0x100-0x1ff to 0x-4x, 0x300-0x3ff to 0x-8x, and 0x700-0x7ff to 0x-16x,
with more linear segments in between. Rockchip's camera engine code
chooses one of the above segments depenging on the desired gain
value. The question is, how should we proceed keeping in mind
libcamera use case? Should the whole 0x0-0x7ff be exposed as-is and
libcamera will do the mapping, or the driver will do the mapping
itself and expose some logical gain units not tied to the actual gain
register value? Meanwhile, this driver conservatively exposes only
0x0-0xf8 gain register range.

[1] https://github.com/rockchip-linux/kernel/blob/develop-4.19/drivers/media/i2c/ov4689.c

Mikhail Rudenko (2):
  media: dt-bindings: media: i2c: document OV4689 DT bindings
  media: i2c: add support for ov4689

 .../bindings/media/i2c/ovti,ov4689.yaml       | 122 +++
 MAINTAINERS                                   |   8 +
 drivers/media/i2c/Kconfig                     |  14 +
 drivers/media/i2c/Makefile                    |   1 +
 drivers/media/i2c/ov4689.c                    | 899 ++++++++++++++++++
 5 files changed, 1044 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml
 create mode 100644 drivers/media/i2c/ov4689.c

--
2.37.0

[PATCH 1/2] media: dt-bindings: media: i2c: document OV4689 DT bindings

[Mikhail Rudenko]

Add device-tree binding documentation for OV4689 image sensor driver,
and the relevant MAINTAINERS entries.

Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com>
---
 .../bindings/media/i2c/ovti,ov4689.yaml       | 122 ++++++++++++++++++
 MAINTAINERS                                   |   7 +
 2 files changed, 129 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml
new file mode 100644
index 000000000000..6bdebe5862b4
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml
@@ -0,0 +1,122 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/ovti,ov4689.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Omnivision OV4689 CMOS Sensor Device Tree Bindings
+
+maintainers:
+  - Mikhail Rudenko <mike.rudenko@gmail.com>
+
+description: |-
+  The Omnivision OV4689 is a high performance, 1/3-inch, 4 megapixel
+  image sensor. Ihis chip supports high frame rate speeds up to 90 fps
+  at 2688x1520 resolution. It is programmable through an I2C
+  interface, and sensor output is sent via 1/2/4 lane MIPI CSI-2
+  connection.
+
+allOf:
+  - $ref: /schemas/media/video-interface-devices.yaml#
+
+properties:
+  compatible:
+    const: ovti,ov4689
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    description:
+      External clock for the sensor.
+    items:
+      - const: xclk
+
+  dovdd-supply:
+    description:
+      Definition of the regulator used as Digital I/O voltage supply.
+
+  avdd-supply:
+    description:
+      Definition of the regulator used as Analog voltage supply.
+
+  dvdd-supply:
+    description:
+      Definition of the regulator used as Digital core voltage supply.
+
+  powerdown-gpios:
+    maxItems: 1
+    description:
+      Reference to the GPIO connected to the powerdown pin (active low).
+
+  reset-gpios:
+    maxItems: 1
+    description:
+      Reference to the GPIO connected to the reset pin (active low).
+
+  orientation: true
+
+  rotation: true
+
+  port:
+    $ref: /schemas/graph.yaml#/$defs/port-base
+    additionalProperties: false
+    description:
+      Output port node, single endpoint describing the CSI-2 transmitter.
+
+    properties:
+      endpoint:
+        $ref: /schemas/media/video-interfaces.yaml#
+        unevaluatedProperties: false
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - dovdd-supply
+  - avdd-supply
+  - dvdd-supply
+  - powerdown-gpios
+  - reset-gpios
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ov4689: camera@36 {
+            compatible = "ovti,ov4689";
+            reg = <0x36>;
+
+            clocks = <&ov4689_clk>;
+            clock-names = "xclk";
+
+            avdd-supply = <&ov4689_avdd>;
+            dovdd-supply = <&ov4689_dovdd>;
+            dvdd-supply = <&ov4689_dvdd>;
+
+            powerdown-gpios = <&pio 107 GPIO_ACTIVE_LOW>;
+            reset-gpios = <&pio 109 GPIO_ACTIVE_LOW>;
+
+            orientation = <2>;
+            rotation = <0>;
+
+            port {
+                wcam_out: endpoint {
+                    remote-endpoint = <&mipi_in_wcam>;
+                };
+            };
+        };
+    };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index f468864fd268..63c4844f26e6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14523,6 +14523,13 @@ S:	Maintained
 T:	git git://linuxtv.org/media_tree.git
 F:	drivers/media/i2c/ov2740.c
 
+OMNIVISION OV4689 SENSOR DRIVER
+M:	Mikhail Rudenko <mike.rudenko@gmail.com>
+L:	linux-media@vger.kernel.org
+S:	Maintained
+T:	git git://linuxtv.org/media_tree.git
+F:	Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml
+
 OMNIVISION OV5640 SENSOR DRIVER
 M:	Steve Longerbeam <slongerbeam@gmail.com>
 L:	linux-media@vger.kernel.org
-- 
2.37.0

Re: [PATCH 1/2] media: dt-bindings: media: i2c: document OV4689 DT bindings

[Krzysztof Kozlowski]

On 12/07/2022 16:19, Mikhail Rudenko wrote:
> Add device-tree binding documentation for OV4689 image sensor driver,
> and the relevant MAINTAINERS entries.
> 
> Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com>
> ---
>  .../bindings/media/i2c/ovti,ov4689.yaml       | 122 ++++++++++++++++++
>  MAINTAINERS                                   |   7 +
>  2 files changed, 129 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml
> new file mode 100644
> index 000000000000..6bdebe5862b4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml
> @@ -0,0 +1,122 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/ovti,ov4689.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Omnivision OV4689 CMOS Sensor Device Tree Bindings

s/Device Tree Bindings//

> +
> +maintainers:
> +  - Mikhail Rudenko <mike.rudenko@gmail.com>
> +
> +description: |-

No need for -

> +  The Omnivision OV4689 is a high performance, 1/3-inch, 4 megapixel
> +  image sensor. Ihis chip supports high frame rate speeds up to 90 fps
> +  at 2688x1520 resolution. It is programmable through an I2C
> +  interface, and sensor output is sent via 1/2/4 lane MIPI CSI-2
> +  connection.
> +
> +allOf:
> +  - $ref: /schemas/media/video-interface-devices.yaml#
> +
> +properties:
> +  compatible:
> +    const: ovti,ov4689
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    description:
> +      External clock for the sensor.

This goes to clocks instead.

> +    items:
> +      - const: xclk

No need for clock-names for such case.

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

s/Definition of the regulator used as //

It's redundant...

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

Ditto

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

Ditto

> +
> +  powerdown-gpios:
> +    maxItems: 1
> +    description:
> +      Reference to the GPIO connected to the powerdown pin (active low).

s/Reference to the//

> +
> +  reset-gpios:
> +    maxItems: 1
> +    description:
> +      Reference to the GPIO connected to the reset pin (active low).

The same.

> +
> +  orientation: true
> +
> +  rotation: true
> +
> +  port:
> +    $ref: /schemas/graph.yaml#/$defs/port-base
> +    additionalProperties: false
> +    description:
> +      Output port node, single endpoint describing the CSI-2 transmitter.
> +
> +    properties:
> +      endpoint:
> +        $ref: /schemas/media/video-interfaces.yaml#
> +        unevaluatedProperties: false
> +

Best regards,
Krzysztof

Re: [PATCH 1/2] media: dt-bindings: media: i2c: document OV4689 DT bindings

[Mikhail Rudenko]

Hi Krzysztof,

On 2022-07-12 at 17:12 +02, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> On 12/07/2022 16:19, Mikhail Rudenko wrote:
>> Add device-tree binding documentation for OV4689 image sensor driver,
>> and the relevant MAINTAINERS entries.
>>
>> Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com>
>> ---
>>  .../bindings/media/i2c/ovti,ov4689.yaml       | 122 ++++++++++++++++++
>>  MAINTAINERS                                   |   7 +
>>  2 files changed, 129 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml
>> new file mode 100644
>> index 000000000000..6bdebe5862b4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml
>> @@ -0,0 +1,122 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/media/i2c/ovti,ov4689.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Omnivision OV4689 CMOS Sensor Device Tree Bindings
>
> s/Device Tree Bindings//

Ack, will fix in v2.

>> +
>> +maintainers:
>> +  - Mikhail Rudenko <mike.rudenko@gmail.com>
>> +
>> +description: |-
>
> No need for -
>

Ack

>> +  The Omnivision OV4689 is a high performance, 1/3-inch, 4 megapixel
>> +  image sensor. Ihis chip supports high frame rate speeds up to 90 fps
>> +  at 2688x1520 resolution. It is programmable through an I2C
>> +  interface, and sensor output is sent via 1/2/4 lane MIPI CSI-2
>> +  connection.
>> +
>> +allOf:
>> +  - $ref: /schemas/media/video-interface-devices.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    const: ovti,ov4689
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  clock-names:
>> +    description:
>> +      External clock for the sensor.
>
> This goes to clocks instead.
>

Ack

>> +    items:
>> +      - const: xclk
>
> No need for clock-names for such case.
>

Ack

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

Ack

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

Ack

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

Ack

>> +
>> +  powerdown-gpios:
>> +    maxItems: 1
>> +    description:
>> +      Reference to the GPIO connected to the powerdown pin (active low).
>
> s/Reference to the//
>

Ack

>> +
>> +  reset-gpios:
>> +    maxItems: 1
>> +    description:
>> +      Reference to the GPIO connected to the reset pin (active low).
>
> The same.
>

Ack

>> +
>> +  orientation: true
>> +
>> +  rotation: true
>> +
>> +  port:
>> +    $ref: /schemas/graph.yaml#/$defs/port-base
>> +    additionalProperties: false
>> +    description:
>> +      Output port node, single endpoint describing the CSI-2 transmitter.
>> +
>> +    properties:
>> +      endpoint:
>> +        $ref: /schemas/media/video-interfaces.yaml#
>> +        unevaluatedProperties: false
>> +
>
> Best regards,
> Krzysztof

Thanks for review! By the way, I used an existing schema for a similar
sensor ("ovti,ov02a10.yaml", specifically) as an example, and your notes
apply to it too. Looks like it should also be fixed...


--
Best regards,
Mikhail Rudenko

Re: [PATCH 1/2] media: dt-bindings: media: i2c: document OV4689 DT bindings

[Sakari Ailus]

Hi Mikhail,

Thanks for the patch.

On Tue, Jul 12, 2022 at 05:19:09PM +0300, Mikhail Rudenko wrote:
> Add device-tree binding documentation for OV4689 image sensor driver,
> and the relevant MAINTAINERS entries.
> 
> Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com>
> ---
>  .../bindings/media/i2c/ovti,ov4689.yaml       | 122 ++++++++++++++++++
>  MAINTAINERS                                   |   7 +
>  2 files changed, 129 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml
> new file mode 100644
> index 000000000000..6bdebe5862b4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml
> @@ -0,0 +1,122 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/ovti,ov4689.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Omnivision OV4689 CMOS Sensor Device Tree Bindings
> +
> +maintainers:
> +  - Mikhail Rudenko <mike.rudenko@gmail.com>
> +
> +description: |-
> +  The Omnivision OV4689 is a high performance, 1/3-inch, 4 megapixel
> +  image sensor. Ihis chip supports high frame rate speeds up to 90 fps
> +  at 2688x1520 resolution. It is programmable through an I2C
> +  interface, and sensor output is sent via 1/2/4 lane MIPI CSI-2
> +  connection.
> +
> +allOf:
> +  - $ref: /schemas/media/video-interface-devices.yaml#
> +
> +properties:
> +  compatible:
> +    const: ovti,ov4689
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    description:
> +      External clock for the sensor.
> +    items:
> +      - const: xclk
> +
> +  dovdd-supply:
> +    description:
> +      Definition of the regulator used as Digital I/O voltage supply.
> +
> +  avdd-supply:
> +    description:
> +      Definition of the regulator used as Analog voltage supply.
> +
> +  dvdd-supply:
> +    description:
> +      Definition of the regulator used as Digital core voltage supply.
> +
> +  powerdown-gpios:
> +    maxItems: 1
> +    description:
> +      Reference to the GPIO connected to the powerdown pin (active low).
> +
> +  reset-gpios:
> +    maxItems: 1
> +    description:
> +      Reference to the GPIO connected to the reset pin (active low).
> +
> +  orientation: true
> +
> +  rotation: true
> +
> +  port:
> +    $ref: /schemas/graph.yaml#/$defs/port-base
> +    additionalProperties: false
> +    description:
> +      Output port node, single endpoint describing the CSI-2 transmitter.
> +
> +    properties:
> +      endpoint:
> +        $ref: /schemas/media/video-interfaces.yaml#
> +        unevaluatedProperties: false

The number of lanes is needed, please require data-lanes property here (as
well as add it to the example).

> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - dovdd-supply
> +  - avdd-supply
> +  - dvdd-supply
> +  - powerdown-gpios
> +  - reset-gpios
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        ov4689: camera@36 {
> +            compatible = "ovti,ov4689";
> +            reg = <0x36>;
> +
> +            clocks = <&ov4689_clk>;
> +            clock-names = "xclk";
> +
> +            avdd-supply = <&ov4689_avdd>;
> +            dovdd-supply = <&ov4689_dovdd>;
> +            dvdd-supply = <&ov4689_dvdd>;
> +
> +            powerdown-gpios = <&pio 107 GPIO_ACTIVE_LOW>;
> +            reset-gpios = <&pio 109 GPIO_ACTIVE_LOW>;
> +
> +            orientation = <2>;
> +            rotation = <0>;
> +
> +            port {
> +                wcam_out: endpoint {
> +                    remote-endpoint = <&mipi_in_wcam>;
> +                };
> +            };
> +        };
> +    };
> +
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f468864fd268..63c4844f26e6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14523,6 +14523,13 @@ S:	Maintained
>  T:	git git://linuxtv.org/media_tree.git
>  F:	drivers/media/i2c/ov2740.c
>  
> +OMNIVISION OV4689 SENSOR DRIVER
> +M:	Mikhail Rudenko <mike.rudenko@gmail.com>
> +L:	linux-media@vger.kernel.org
> +S:	Maintained
> +T:	git git://linuxtv.org/media_tree.git
> +F:	Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml
> +
>  OMNIVISION OV5640 SENSOR DRIVER
>  M:	Steve Longerbeam <slongerbeam@gmail.com>
>  L:	linux-media@vger.kernel.org

-- 
Regards,

Sakari Ailus

Re: [PATCH 1/2] media: dt-bindings: media: i2c: document OV4689 DT bindings

[Mikhail Rudenko]

On 2022-07-15 at 00:11 +03, Sakari Ailus <sakari.ailus@iki.fi> wrote:

> Hi Mikhail,
>
> Thanks for the patch.
>
> On Tue, Jul 12, 2022 at 05:19:09PM +0300, Mikhail Rudenko wrote:
>> Add device-tree binding documentation for OV4689 image sensor driver,
>> and the relevant MAINTAINERS entries.
>>
>> Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com>
>> ---
>>  .../bindings/media/i2c/ovti,ov4689.yaml       | 122 ++++++++++++++++++
>>  MAINTAINERS                                   |   7 +
>>  2 files changed, 129 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml
>> new file mode 100644
>> index 000000000000..6bdebe5862b4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml
>> @@ -0,0 +1,122 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/media/i2c/ovti,ov4689.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Omnivision OV4689 CMOS Sensor Device Tree Bindings
>> +
>> +maintainers:
>> +  - Mikhail Rudenko <mike.rudenko@gmail.com>
>> +
>> +description: |-
>> +  The Omnivision OV4689 is a high performance, 1/3-inch, 4 megapixel
>> +  image sensor. Ihis chip supports high frame rate speeds up to 90 fps
>> +  at 2688x1520 resolution. It is programmable through an I2C
>> +  interface, and sensor output is sent via 1/2/4 lane MIPI CSI-2
>> +  connection.
>> +
>> +allOf:
>> +  - $ref: /schemas/media/video-interface-devices.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    const: ovti,ov4689
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  clock-names:
>> +    description:
>> +      External clock for the sensor.
>> +    items:
>> +      - const: xclk
>> +
>> +  dovdd-supply:
>> +    description:
>> +      Definition of the regulator used as Digital I/O voltage supply.
>> +
>> +  avdd-supply:
>> +    description:
>> +      Definition of the regulator used as Analog voltage supply.
>> +
>> +  dvdd-supply:
>> +    description:
>> +      Definition of the regulator used as Digital core voltage supply.
>> +
>> +  powerdown-gpios:
>> +    maxItems: 1
>> +    description:
>> +      Reference to the GPIO connected to the powerdown pin (active low).
>> +
>> +  reset-gpios:
>> +    maxItems: 1
>> +    description:
>> +      Reference to the GPIO connected to the reset pin (active low).
>> +
>> +  orientation: true
>> +
>> +  rotation: true
>> +
>> +  port:
>> +    $ref: /schemas/graph.yaml#/$defs/port-base
>> +    additionalProperties: false
>> +    description:
>> +      Output port node, single endpoint describing the CSI-2 transmitter.
>> +
>> +    properties:
>> +      endpoint:
>> +        $ref: /schemas/media/video-interfaces.yaml#
>> +        unevaluatedProperties: false
>
> The number of lanes is needed, please require data-lanes property here (as
> well as add it to the example).

Thanks for review, will fix in v2

>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - clocks
>> +  - clock-names
>> +  - dovdd-supply
>> +  - avdd-supply
>> +  - dvdd-supply
>> +  - powerdown-gpios
>> +  - reset-gpios
>> +  - port
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/gpio/gpio.h>
>> +
>> +    i2c {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        ov4689: camera@36 {
>> +            compatible = "ovti,ov4689";
>> +            reg = <0x36>;
>> +
>> +            clocks = <&ov4689_clk>;
>> +            clock-names = "xclk";
>> +
>> +            avdd-supply = <&ov4689_avdd>;
>> +            dovdd-supply = <&ov4689_dovdd>;
>> +            dvdd-supply = <&ov4689_dvdd>;
>> +
>> +            powerdown-gpios = <&pio 107 GPIO_ACTIVE_LOW>;
>> +            reset-gpios = <&pio 109 GPIO_ACTIVE_LOW>;
>> +
>> +            orientation = <2>;
>> +            rotation = <0>;
>> +
>> +            port {
>> +                wcam_out: endpoint {
>> +                    remote-endpoint = <&mipi_in_wcam>;
>> +                };
>> +            };
>> +        };
>> +    };
>> +
>> +...
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index f468864fd268..63c4844f26e6 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -14523,6 +14523,13 @@ S:	Maintained
>>  T:	git git://linuxtv.org/media_tree.git
>>  F:	drivers/media/i2c/ov2740.c
>>
>> +OMNIVISION OV4689 SENSOR DRIVER
>> +M:	Mikhail Rudenko <mike.rudenko@gmail.com>
>> +L:	linux-media@vger.kernel.org
>> +S:	Maintained
>> +T:	git git://linuxtv.org/media_tree.git
>> +F:	Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml
>> +
>>  OMNIVISION OV5640 SENSOR DRIVER
>>  M:	Steve Longerbeam <slongerbeam@gmail.com>
>>  L:	linux-media@vger.kernel.org


--
Best regards,
Mikhail Rudenko

Re: [PATCH 0/2] Add Omnivision OV4689 image sensor driver

[Mikhail Rudenko]

Gentle ping. Still waiting for any [2/2] review. Should I resend?


On 2022-07-12 at 17:19 +03, Mikhail Rudenko <mike.rudenko@gmail.com> wrote:
> Hello,
>
> this series implements support for Omnivision OV4689 image
> sensor. The Omnivision OV4689 is a high performance, 1/3-inch, 4
> megapixel image sensor. Ihis chip supports high frame rate speeds up
> to 90 fps at 2688x1520 resolution. It is programmable through an I2C
> interface, and sensor output is sent via 1/2/4 lane MIPI CSI-2
> connection.
>
> The driver is based on Rockchip BSP kernel [1]. It implements 4-lane CSI-2
> and single 2688x1520 @ 30 fps mode. The driver was tested on Rockchip
> 3399-based FriendlyElec NanoPi M4 board with MCAM400 camera module.
>
> While porting the driver, I stumbled upon two issues:
>
> (1) In the original driver, horizontal total size (HTS) was set to a
> value (2584) lower then the frame width (2688), resulting in negative
> hblank. In this driver, I increased HTS to 2688, but fps dropped from
> 29.88 to 28.73. What is the preferred way to handle this?
>
> (2) The original driver exposes analog gain range 0x0 - 0x7ff, but the
> gain is not linear across that range. Instead, it is piecewise linear
> (and discontinuous). 0x0-0xff register values result in 0x-2x gain,
> 0x100-0x1ff to 0x-4x, 0x300-0x3ff to 0x-8x, and 0x700-0x7ff to 0x-16x,
> with more linear segments in between. Rockchip's camera engine code
> chooses one of the above segments depenging on the desired gain
> value. The question is, how should we proceed keeping in mind
> libcamera use case? Should the whole 0x0-0x7ff be exposed as-is and
> libcamera will do the mapping, or the driver will do the mapping
> itself and expose some logical gain units not tied to the actual gain
> register value? Meanwhile, this driver conservatively exposes only
> 0x0-0xf8 gain register range.
>
> [1] https://github.com/rockchip-linux/kernel/blob/develop-4.19/drivers/media/i2c/ov4689.c
>
> Mikhail Rudenko (2):
>   media: dt-bindings: media: i2c: document OV4689 DT bindings
>   media: i2c: add support for ov4689
>
>  .../bindings/media/i2c/ovti,ov4689.yaml       | 122 +++
>  MAINTAINERS                                   |   8 +
>  drivers/media/i2c/Kconfig                     |  14 +
>  drivers/media/i2c/Makefile                    |   1 +
>  drivers/media/i2c/ov4689.c                    | 899 ++++++++++++++++++
>  5 files changed, 1044 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml
>  create mode 100644 drivers/media/i2c/ov4689.c


--
Best regards,
Mikhail Rudenko