[V6, 0/2] media: i2c: Add support for OV02A10 sensor

[V6, 0/2] media: i2c: Add support for OV02A10 sensor

[Dongchun Zhu]

Hello,

This series adds DT binding and driver for Omnivision's OV02A10 2 megapixel CMOS 1/5" sensor,
which has a single MIPI lane interface and output format of 10-bit RAW.

The driver is implemented wth V4L2 framework.
1. Async registered as a V4L2 I2C sub-device.
2. The first component of camera system including Seninf, ISP.
3. A media entity that can provide one source pad in common and two for dual camera.

Changes compared to v5:
 - Rebase onto 5.5-rc1
 - Remove the handler of "ovti, hs-vod-adjust" property
 - Define new macro HZ_PER_MHZ to describe frequency-related parameters
 - Fixup coding style and improve code quality
 - Fix other reviewed issues in v5

Changes of v5 are addressing comments from Sakari, Tomasz.
 - Set default orientation in dt-bindings
 - Move the content of power on/off directly to the resume/suspend callbacks
 - Move sensor id check to power on to avoid the privacy LED flash on boot
 - Remove unnecessary debug log in driver
 - Fix other reviewed issues in v4

Changes of v4 mainly address the comments from Sakari, Rob, Tomasz.
 - Remove data-lanes property in DT
 - Add link frequencies in DT to match the expect value that driver requires
 - Omit open callback as int_cfg is implemented
 - Use i2c_smbus_write_byte_data/i2c_smbus_read_byte_data instead of customed APIs
 - Use do_div to calculate pixel rate
 - Use usleep_range directly for shoter sleep case
 - Re-adjust sensor power up/off sequence
 - Re-set pd/rst GPIO inverter property according to the datasheet
 - Refine set_exposure/set_gain/set_vblanking/set_test_pattern functions
 - Fix other reviewed issues in v3

Changes of v3 are mainly addressing comments from Rob, Sakari, Bingbu.
 - Fix coding style errors in dt-bindings
 - Use macro flag to describle basic line 1224 when updating v-blanking
 - Remove unnecessary debug log in driver

Mainly changes of v2 are addressing the comments from Nicolas, Bingbu, Sakari, Rob,
including,
 - Put dt binding before driver in series
 - Add MAINTAINERS entries
 - Squash the MAINTAINERS entry and Kconfig to driver patch
 - Add rotation support for driver
 - Fix other reviewed issues in v1

Dongchun Zhu (2):
  media: dt-bindings: media: i2c: Document OV02A10 bindings
  media: i2c: ov02a10: Add OV02A10 image sensor driver

 .../devicetree/bindings/media/i2c/ov02a10.txt      |   54 +
 MAINTAINERS                                        |    8 +
 drivers/media/i2c/Kconfig                          |   11 +
 drivers/media/i2c/Makefile                         |    1 +
 drivers/media/i2c/ov02a10.c                        | 1102 ++++++++++++++++++++
 5 files changed, 1176 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov02a10.txt
 create mode 100644 drivers/media/i2c/ov02a10.c

-- 
2.9.2

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

[Dongchun Zhu]

Add DT bindings documentation for Omnivision OV02A10 image sensor.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
---
 .../devicetree/bindings/media/i2c/ov02a10.txt      | 54 ++++++++++++++++++++++
 MAINTAINERS                                        |  7 +++
 2 files changed, 61 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov02a10.txt

diff --git a/Documentation/devicetree/bindings/media/i2c/ov02a10.txt b/Documentation/devicetree/bindings/media/i2c/ov02a10.txt
new file mode 100644
index 0000000..18acc4f
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov02a10.txt
@@ -0,0 +1,54 @@
+* Omnivision OV02A10 MIPI CSI-2 sensor
+
+Required Properties:
+- compatible: shall be "ovti,ov02a10"
+- clocks: reference to the eclk input clock
+- clock-names: shall be "eclk"
+- dovdd-supply: Digital I/O voltage supply, 1.8 volts
+- avdd-supply: Analog voltage supply, 2.8 volts
+- dvdd-supply: Digital core voltage supply, 1.8 volts
+- powerdown-gpios: reference to the GPIO connected to the powerdown pin,
+		   if any. This is an active low signal to the OV02A10.
+- reset-gpios: reference to the GPIO connected to the reset pin, if any.
+	       This is an active high signal to the OV02A10.
+
+Optional Properties:
+- rotation: as defined in
+	    Documentation/devicetree/bindings/media/video-interfaces.txt,
+	    valid values are 0 (sensor mounted upright) and 180 (sensor
+	    mounted upside down).
+
+The device node shall contain one 'port' child node with an
+'endpoint' subnode for its digital output video port,
+in accordance with the video interface bindings defined in
+Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Example:
+&i2c4 {
+	ov02a10: camera-sensor@3d {
+		compatible = "ovti,ov02a10";
+		reg = <0x3d>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&camera_pins_cam1_mclk_on>;
+
+		clocks = <&topckgen CLK_TOP_MUX_CAMTG2>,
+			<&topckgen CLK_TOP_UNIVP_192M_D8>;
+		clock-names = "eclk", "freq_mux";
+		clock-frequency = <24000000>;
+
+		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>;
+		rotation = <180>;
+
+		port {
+			/* MIPI CSI-2 bus endpoint */
+			ov02a10_core: endpoint {
+				remote-endpoint = <&ov02a10_0>;
+				link-frequencies = /bits/ 64 <390000000>;
+			};
+		};
+	};
+};
diff --git a/MAINTAINERS b/MAINTAINERS
index bd5847e..92a868c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12130,6 +12130,13 @@ T:	git git://linuxtv.org/media_tree.git
 S:	Maintained
 F:	drivers/media/i2c/ov13858.c
 
+OMNIVISION OV02A10 SENSOR DRIVER
+M:	Dongchun Zhu <dongchun.zhu@mediatek.com>
+L:	linux-media@vger.kernel.org
+T:	git git://linuxtv.org/media_tree.git
+S:	Maintained
+F:	Documentation/devicetree/bindings/media/i2c/ov02a10.txt
+
 OMNIVISION OV2680 SENSOR DRIVER
 M:	Rui Miguel Silva <rmfrfs@gmail.com>
 L:	linux-media@vger.kernel.org
-- 
2.9.2

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

[Tomasz Figa]

Hi Rob, Dongchun,

On Wed, Dec 11, 2019 at 8:29 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
>
> Add DT bindings documentation for Omnivision OV02A10 image sensor.
>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> ---
>  .../devicetree/bindings/media/i2c/ov02a10.txt      | 54 ++++++++++++++++++++++
>  MAINTAINERS                                        |  7 +++
>  2 files changed, 61 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov02a10.txt
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov02a10.txt b/Documentation/devicetree/bindings/media/i2c/ov02a10.txt
> new file mode 100644
> index 0000000..18acc4f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov02a10.txt
> @@ -0,0 +1,54 @@
> +* Omnivision OV02A10 MIPI CSI-2 sensor
> +
> +Required Properties:
> +- compatible: shall be "ovti,ov02a10"
> +- clocks: reference to the eclk input clock
> +- clock-names: shall be "eclk"
> +- dovdd-supply: Digital I/O voltage supply, 1.8 volts
> +- avdd-supply: Analog voltage supply, 2.8 volts
> +- dvdd-supply: Digital core voltage supply, 1.8 volts
> +- powerdown-gpios: reference to the GPIO connected to the powerdown pin,
> +                  if any. This is an active low signal to the OV02A10.

On the hardware level this pin is active high, i.e. the device is
powered down when the signal is high.

> +- reset-gpios: reference to the GPIO connected to the reset pin, if any.
> +              This is an active high signal to the OV02A10.

On the hardware level this pin is active low, i.e. the device is held
in reset when the signal is low.

However, there is some confusion around how the polarity flag in the
GPIO specifier is supposed to be used.

As per [1],

"The gpio-specifier's polarity flag should represent the physical
level at the GPIO controller that achieves (or represents, for inputs)
a logically asserted value at the device. The exact definition of
logically asserted should be defined by the binding for the device."

In this case it sounds like "logically asserted" means the device is
powered down or held in reset, respectively, which would suggest that
the specifiers should have GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW
respectively. The latter would cause the GPIO subsystem to invert the
values set by the consumers, which would then be confusing from the
driver implementation point of view.

Should the pin be renamed to "nreset"? It would change the meaning of
"logically asserted" to "device is not held in reset" and so
GPIO_ACTIVE_HIGH (or 0) would be the right value to use.

[1] https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/gpio/gpio.txt#L83

Best regards,
Tomasz

> +
> +Optional Properties:
> +- rotation: as defined in
> +           Documentation/devicetree/bindings/media/video-interfaces.txt,
> +           valid values are 0 (sensor mounted upright) and 180 (sensor
> +           mounted upside down).
> +
> +The device node shall contain one 'port' child node with an
> +'endpoint' subnode for its digital output video port,
> +in accordance with the video interface bindings defined in
> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +Example:
> +&i2c4 {
> +       ov02a10: camera-sensor@3d {
> +               compatible = "ovti,ov02a10";
> +               reg = <0x3d>;
> +               pinctrl-names = "default";
> +               pinctrl-0 = <&camera_pins_cam1_mclk_on>;
> +
> +               clocks = <&topckgen CLK_TOP_MUX_CAMTG2>,
> +                       <&topckgen CLK_TOP_UNIVP_192M_D8>;
> +               clock-names = "eclk", "freq_mux";
> +               clock-frequency = <24000000>;
> +
> +               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>;
> +               rotation = <180>;
> +
> +               port {
> +                       /* MIPI CSI-2 bus endpoint */
> +                       ov02a10_core: endpoint {
> +                               remote-endpoint = <&ov02a10_0>;
> +                               link-frequencies = /bits/ 64 <390000000>;
> +                       };
> +               };
> +       };
> +};
> diff --git a/MAINTAINERS b/MAINTAINERS
> index bd5847e..92a868c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12130,6 +12130,13 @@ T:     git git://linuxtv.org/media_tree.git
>  S:     Maintained
>  F:     drivers/media/i2c/ov13858.c
>
> +OMNIVISION OV02A10 SENSOR DRIVER
> +M:     Dongchun Zhu <dongchun.zhu@mediatek.com>
> +L:     linux-media@vger.kernel.org
> +T:     git git://linuxtv.org/media_tree.git
> +S:     Maintained
> +F:     Documentation/devicetree/bindings/media/i2c/ov02a10.txt
> +
>  OMNIVISION OV2680 SENSOR DRIVER
>  M:     Rui Miguel Silva <rmfrfs@gmail.com>
>  L:     linux-media@vger.kernel.org
> --
> 2.9.2

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

[Tomasz Figa]

On Tue, Dec 17, 2019 at 4:15 AM Tomasz Figa <tfiga@chromium.org> wrote:
>
> Hi Rob, Dongchun,
>
> On Wed, Dec 11, 2019 at 8:29 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> >
> > Add DT bindings documentation for Omnivision OV02A10 image sensor.
> >
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > ---
> >  .../devicetree/bindings/media/i2c/ov02a10.txt      | 54 ++++++++++++++++++++++
> >  MAINTAINERS                                        |  7 +++
> >  2 files changed, 61 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov02a10.txt
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ov02a10.txt b/Documentation/devicetree/bindings/media/i2c/ov02a10.txt
> > new file mode 100644
> > index 0000000..18acc4f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/ov02a10.txt
> > @@ -0,0 +1,54 @@
> > +* Omnivision OV02A10 MIPI CSI-2 sensor
> > +
> > +Required Properties:
> > +- compatible: shall be "ovti,ov02a10"
> > +- clocks: reference to the eclk input clock
> > +- clock-names: shall be "eclk"
> > +- dovdd-supply: Digital I/O voltage supply, 1.8 volts
> > +- avdd-supply: Analog voltage supply, 2.8 volts
> > +- dvdd-supply: Digital core voltage supply, 1.8 volts
> > +- powerdown-gpios: reference to the GPIO connected to the powerdown pin,
> > +                  if any. This is an active low signal to the OV02A10.
>
> On the hardware level this pin is active high, i.e. the device is
> powered down when the signal is high.
>
> > +- reset-gpios: reference to the GPIO connected to the reset pin, if any.
> > +              This is an active high signal to the OV02A10.
>
> On the hardware level this pin is active low, i.e. the device is held
> in reset when the signal is low.
>
> However, there is some confusion around how the polarity flag in the
> GPIO specifier is supposed to be used.
>
> As per [1],
>
> "The gpio-specifier's polarity flag should represent the physical
> level at the GPIO controller that achieves (or represents, for inputs)
> a logically asserted value at the device. The exact definition of
> logically asserted should be defined by the binding for the device."
>
> In this case it sounds like "logically asserted" means the device is
> powered down or held in reset, respectively, which would suggest that
> the specifiers should have GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW
> respectively. The latter would cause the GPIO subsystem to invert the
> values set by the consumers, which would then be confusing from the
> driver implementation point of view.
>
> Should the pin be renamed to "nreset"? It would change the meaning of
> "logically asserted" to "device is not held in reset" and so
> GPIO_ACTIVE_HIGH (or 0) would be the right value to use.
>
> [1] https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/gpio/gpio.txt#L83

+ Bartosz, Linus, Sakari and the linux-gpio ML for a broader audience.

Would appreciate some feedback on what's the proper way of defining
GPIO polarity. Thanks!

Best regards,
Tomasz

>
> Best regards,
> Tomasz
>
> > +
> > +Optional Properties:
> > +- rotation: as defined in
> > +           Documentation/devicetree/bindings/media/video-interfaces.txt,
> > +           valid values are 0 (sensor mounted upright) and 180 (sensor
> > +           mounted upside down).
> > +
> > +The device node shall contain one 'port' child node with an
> > +'endpoint' subnode for its digital output video port,
> > +in accordance with the video interface bindings defined in
> > +Documentation/devicetree/bindings/media/video-interfaces.txt.
> > +
> > +Example:
> > +&i2c4 {
> > +       ov02a10: camera-sensor@3d {
> > +               compatible = "ovti,ov02a10";
> > +               reg = <0x3d>;
> > +               pinctrl-names = "default";
> > +               pinctrl-0 = <&camera_pins_cam1_mclk_on>;
> > +
> > +               clocks = <&topckgen CLK_TOP_MUX_CAMTG2>,
> > +                       <&topckgen CLK_TOP_UNIVP_192M_D8>;
> > +               clock-names = "eclk", "freq_mux";
> > +               clock-frequency = <24000000>;
> > +
> > +               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>;
> > +               rotation = <180>;
> > +
> > +               port {
> > +                       /* MIPI CSI-2 bus endpoint */
> > +                       ov02a10_core: endpoint {
> > +                               remote-endpoint = <&ov02a10_0>;
> > +                               link-frequencies = /bits/ 64 <390000000>;
> > +                       };
> > +               };
> > +       };
> > +};
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index bd5847e..92a868c 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12130,6 +12130,13 @@ T:     git git://linuxtv.org/media_tree.git
> >  S:     Maintained
> >  F:     drivers/media/i2c/ov13858.c
> >
> > +OMNIVISION OV02A10 SENSOR DRIVER
> > +M:     Dongchun Zhu <dongchun.zhu@mediatek.com>
> > +L:     linux-media@vger.kernel.org
> > +T:     git git://linuxtv.org/media_tree.git
> > +S:     Maintained
> > +F:     Documentation/devicetree/bindings/media/i2c/ov02a10.txt
> > +
> >  OMNIVISION OV2680 SENSOR DRIVER
> >  M:     Rui Miguel Silva <rmfrfs@gmail.com>
> >  L:     linux-media@vger.kernel.org
> > --
> > 2.9.2

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

[Dongchun Zhu]

Hi Mauro, Sakari, Rob,

On Wed, 2020-04-08 at 14:49 +0200, Tomasz Figa wrote:
> On Tue, Dec 17, 2019 at 4:15 AM Tomasz Figa <tfiga@chromium.org> wrote:
> >
> > Hi Rob, Dongchun,
> >
> > On Wed, Dec 11, 2019 at 8:29 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > >
> > > Add DT bindings documentation for Omnivision OV02A10 image sensor.
> > >
> > > Reviewed-by: Rob Herring <robh@kernel.org>
> > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > ---
> > >  .../devicetree/bindings/media/i2c/ov02a10.txt      | 54 ++++++++++++++++++++++
> > >  MAINTAINERS                                        |  7 +++
> > >  2 files changed, 61 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov02a10.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov02a10.txt b/Documentation/devicetree/bindings/media/i2c/ov02a10.txt
> > > new file mode 100644
> > > index 0000000..18acc4f
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/i2c/ov02a10.txt
> > > @@ -0,0 +1,54 @@
> > > +* Omnivision OV02A10 MIPI CSI-2 sensor
> > > +
> > > +Required Properties:
> > > +- compatible: shall be "ovti,ov02a10"
> > > +- clocks: reference to the eclk input clock
> > > +- clock-names: shall be "eclk"
> > > +- dovdd-supply: Digital I/O voltage supply, 1.8 volts
> > > +- avdd-supply: Analog voltage supply, 2.8 volts
> > > +- dvdd-supply: Digital core voltage supply, 1.8 volts
> > > +- powerdown-gpios: reference to the GPIO connected to the powerdown pin,
> > > +                  if any. This is an active low signal to the OV02A10.
> >
> > On the hardware level this pin is active high, i.e. the device is
> > powered down when the signal is high.
> >
> > > +- reset-gpios: reference to the GPIO connected to the reset pin, if any.
> > > +              This is an active high signal to the OV02A10.
> >
> > On the hardware level this pin is active low, i.e. the device is held
> > in reset when the signal is low.
> >
> > However, there is some confusion around how the polarity flag in the
> > GPIO specifier is supposed to be used.
> >
> > As per [1],
> >
> > "The gpio-specifier's polarity flag should represent the physical
> > level at the GPIO controller that achieves (or represents, for inputs)
> > a logically asserted value at the device. The exact definition of
> > logically asserted should be defined by the binding for the device."
> >
> > In this case it sounds like "logically asserted" means the device is
> > powered down or held in reset, respectively, which would suggest that
> > the specifiers should have GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW
> > respectively. The latter would cause the GPIO subsystem to invert the
> > values set by the consumers, which would then be confusing from the
> > driver implementation point of view.
> >
> > Should the pin be renamed to "nreset"? It would change the meaning of
> > "logically asserted" to "device is not held in reset" and so
> > GPIO_ACTIVE_HIGH (or 0) would be the right value to use.
> >
> > [1] https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/gpio/gpio.txt#L83
> 
> + Bartosz, Linus, Sakari and the linux-gpio ML for a broader audience.
> 
> Would appreciate some feedback on what's the proper way of defining
> GPIO polarity. Thanks!
> 
> Best regards,
> Tomasz
> 

I have another question about OV02A10 CMOS sensor dt-binding.
As its text documentation was already reviewed by Rob on earlier
version:
https://patchwork.linuxtv.org/patch/59787/
I wonder whether we need to convert it to DT in YAML.
In fact, I just submitted one conversion version.
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2143922

Unluckily make dt_binding_check still report errors temporarily.
It seems there is something wrong with the port property in DT.
Could anyone help provide some tips?
$make dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
  SCHEMA  Documentation/devicetree/bindings/processed-schema.yaml
Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml: ignoring,
error in schema: properties: port: patternProperties: endpoint
warning: no schema found in file:
Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
make[2]: *** [Documentation/devicetree/bindings/processed-schema.yaml]
Error 255
make[1]: *** [dt_binding_check] Error 2
make: *** [sub-make] Error 2

In addition, as OV02A10 use one private property to distinguish
different projects that adopting different register settings,
I would appreciate the feedback on how to add private property to DT in
YAML.

> >
> > Best regards,
> > Tomasz
> >
> > > +
> > > +Optional Properties:
> > > +- rotation: as defined in
> > > +           Documentation/devicetree/bindings/media/video-interfaces.txt,
> > > +           valid values are 0 (sensor mounted upright) and 180 (sensor
> > > +           mounted upside down).
> > > +
> > > +The device node shall contain one 'port' child node with an
> > > +'endpoint' subnode for its digital output video port,
> > > +in accordance with the video interface bindings defined in
> > > +Documentation/devicetree/bindings/media/video-interfaces.txt.
> > > +
> > > +Example:
> > > +&i2c4 {
> > > +       ov02a10: camera-sensor@3d {
> > > +               compatible = "ovti,ov02a10";
> > > +               reg = <0x3d>;
> > > +               pinctrl-names = "default";
> > > +               pinctrl-0 = <&camera_pins_cam1_mclk_on>;
> > > +
> > > +               clocks = <&topckgen CLK_TOP_MUX_CAMTG2>,
> > > +                       <&topckgen CLK_TOP_UNIVP_192M_D8>;
> > > +               clock-names = "eclk", "freq_mux";
> > > +               clock-frequency = <24000000>;
> > > +
> > > +               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>;
> > > +               rotation = <180>;
> > > +
> > > +               port {
> > > +                       /* MIPI CSI-2 bus endpoint */
> > > +                       ov02a10_core: endpoint {
> > > +                               remote-endpoint = <&ov02a10_0>;
> > > +                               link-frequencies = /bits/ 64 <390000000>;
> > > +                       };
> > > +               };
> > > +       };
> > > +};
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index bd5847e..92a868c 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -12130,6 +12130,13 @@ T:     git git://linuxtv.org/media_tree.git
> > >  S:     Maintained
> > >  F:     drivers/media/i2c/ov13858.c
> > >
> > > +OMNIVISION OV02A10 SENSOR DRIVER
> > > +M:     Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > +L:     linux-media@vger.kernel.org
> > > +T:     git git://linuxtv.org/media_tree.git
> > > +S:     Maintained
> > > +F:     Documentation/devicetree/bindings/media/i2c/ov02a10.txt
> > > +
> > >  OMNIVISION OV2680 SENSOR DRIVER
> > >  M:     Rui Miguel Silva <rmfrfs@gmail.com>
> > >  L:     linux-media@vger.kernel.org
> > > --
> > > 2.9.2

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

[Rob Herring]

On Thu, Apr 09, 2020 at 09:03:28PM +0800, Dongchun Zhu wrote:
> Hi Mauro, Sakari, Rob,
> 
> On Wed, 2020-04-08 at 14:49 +0200, Tomasz Figa wrote:
> > On Tue, Dec 17, 2019 at 4:15 AM Tomasz Figa <tfiga@chromium.org> wrote:
> > >
> > > Hi Rob, Dongchun,
> > >
> > > On Wed, Dec 11, 2019 at 8:29 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > > >
> > > > Add DT bindings documentation for Omnivision OV02A10 image sensor.
> > > >
> > > > Reviewed-by: Rob Herring <robh@kernel.org>
> > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > > ---
> > > >  .../devicetree/bindings/media/i2c/ov02a10.txt      | 54 ++++++++++++++++++++++
> > > >  MAINTAINERS                                        |  7 +++
> > > >  2 files changed, 61 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov02a10.txt
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov02a10.txt b/Documentation/devicetree/bindings/media/i2c/ov02a10.txt
> > > > new file mode 100644
> > > > index 0000000..18acc4f
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov02a10.txt
> > > > @@ -0,0 +1,54 @@
> > > > +* Omnivision OV02A10 MIPI CSI-2 sensor
> > > > +
> > > > +Required Properties:
> > > > +- compatible: shall be "ovti,ov02a10"
> > > > +- clocks: reference to the eclk input clock
> > > > +- clock-names: shall be "eclk"
> > > > +- dovdd-supply: Digital I/O voltage supply, 1.8 volts
> > > > +- avdd-supply: Analog voltage supply, 2.8 volts
> > > > +- dvdd-supply: Digital core voltage supply, 1.8 volts
> > > > +- powerdown-gpios: reference to the GPIO connected to the powerdown pin,
> > > > +                  if any. This is an active low signal to the OV02A10.
> > >
> > > On the hardware level this pin is active high, i.e. the device is
> > > powered down when the signal is high.
> > >
> > > > +- reset-gpios: reference to the GPIO connected to the reset pin, if any.
> > > > +              This is an active high signal to the OV02A10.
> > >
> > > On the hardware level this pin is active low, i.e. the device is held
> > > in reset when the signal is low.
> > >
> > > However, there is some confusion around how the polarity flag in the
> > > GPIO specifier is supposed to be used.
> > >
> > > As per [1],
> > >
> > > "The gpio-specifier's polarity flag should represent the physical
> > > level at the GPIO controller that achieves (or represents, for inputs)
> > > a logically asserted value at the device. The exact definition of
> > > logically asserted should be defined by the binding for the device."
> > >
> > > In this case it sounds like "logically asserted" means the device is
> > > powered down or held in reset, respectively, which would suggest that
> > > the specifiers should have GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW
> > > respectively. The latter would cause the GPIO subsystem to invert the
> > > values set by the consumers, which would then be confusing from the
> > > driver implementation point of view.
> > >
> > > Should the pin be renamed to "nreset"? It would change the meaning of
> > > "logically asserted" to "device is not held in reset" and so
> > > GPIO_ACTIVE_HIGH (or 0) would be the right value to use.
> > >
> > > [1] https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/gpio/gpio.txt#L83
> > 
> > + Bartosz, Linus, Sakari and the linux-gpio ML for a broader audience.
> > 
> > Would appreciate some feedback on what's the proper way of defining
> > GPIO polarity. Thanks!
> > 
> > Best regards,
> > Tomasz
> > 
> 
> I have another question about OV02A10 CMOS sensor dt-binding.
> As its text documentation was already reviewed by Rob on earlier
> version:
> https://patchwork.linuxtv.org/patch/59787/
> I wonder whether we need to convert it to DT in YAML.

Yes.

> In fact, I just submitted one conversion version.
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2143922
> 
> Unluckily make dt_binding_check still report errors temporarily.
> It seems there is something wrong with the port property in DT.
> Could anyone help provide some tips?
> $make dt_binding_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
>   SCHEMA  Documentation/devicetree/bindings/processed-schema.yaml
> Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml: ignoring,
> error in schema: properties: port: patternProperties: endpoint
> warning: no schema found in file:
> Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> make[2]: *** [Documentation/devicetree/bindings/processed-schema.yaml]
> Error 255
> make[1]: *** [dt_binding_check] Error 2
> make: *** [sub-make] Error 2

    patternProperties:
      endpoint:
      type: object
      additionalProperties: false

You need more indentation under 'endpoint'. Also, 'endpoint' is a fixed 
string, so it should be under 'properties' rather than 'patternProperties'.


> 
> In addition, as OV02A10 use one private property to distinguish
> different projects that adopting different register settings,
> I would appreciate the feedback on how to add private property to DT in
> YAML.

Like any other property. Submit something for review.

Rob

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

[Dongchun Zhu]

Hello Rob,

Thanks for the review.

On Wed, 2020-04-15 at 11:14 -0500, Rob Herring wrote:
> On Thu, Apr 09, 2020 at 09:03:28PM +0800, Dongchun Zhu wrote:
> > Hi Mauro, Sakari, Rob,
> > 
> > On Wed, 2020-04-08 at 14:49 +0200, Tomasz Figa wrote:
> > > On Tue, Dec 17, 2019 at 4:15 AM Tomasz Figa <tfiga@chromium.org> wrote:
> > > >
> > > > Hi Rob, Dongchun,
> > > >
> > > > On Wed, Dec 11, 2019 at 8:29 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > > > >
> > > > > Add DT bindings documentation for Omnivision OV02A10 image sensor.
> > > > >
> > > > > Reviewed-by: Rob Herring <robh@kernel.org>
> > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > > > ---
> > > > >  .../devicetree/bindings/media/i2c/ov02a10.txt      | 54 ++++++++++++++++++++++
> > > > >  MAINTAINERS                                        |  7 +++
> > > > >  2 files changed, 61 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov02a10.txt
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov02a10.txt b/Documentation/devicetree/bindings/media/i2c/ov02a10.txt
> > > > > new file mode 100644
> > > > > index 0000000..18acc4f
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov02a10.txt
> > > > > @@ -0,0 +1,54 @@
> > > > > +* Omnivision OV02A10 MIPI CSI-2 sensor
> > > > > +
> > > > > +Required Properties:
> > > > > +- compatible: shall be "ovti,ov02a10"
> > > > > +- clocks: reference to the eclk input clock
> > > > > +- clock-names: shall be "eclk"
> > > > > +- dovdd-supply: Digital I/O voltage supply, 1.8 volts
> > > > > +- avdd-supply: Analog voltage supply, 2.8 volts
> > > > > +- dvdd-supply: Digital core voltage supply, 1.8 volts
> > > > > +- powerdown-gpios: reference to the GPIO connected to the powerdown pin,
> > > > > +                  if any. This is an active low signal to the OV02A10.
> > > >
> > > > On the hardware level this pin is active high, i.e. the device is
> > > > powered down when the signal is high.
> > > >
> > > > > +- reset-gpios: reference to the GPIO connected to the reset pin, if any.
> > > > > +              This is an active high signal to the OV02A10.
> > > >
> > > > On the hardware level this pin is active low, i.e. the device is held
> > > > in reset when the signal is low.
> > > >
> > > > However, there is some confusion around how the polarity flag in the
> > > > GPIO specifier is supposed to be used.
> > > >
> > > > As per [1],
> > > >
> > > > "The gpio-specifier's polarity flag should represent the physical
> > > > level at the GPIO controller that achieves (or represents, for inputs)
> > > > a logically asserted value at the device. The exact definition of
> > > > logically asserted should be defined by the binding for the device."
> > > >
> > > > In this case it sounds like "logically asserted" means the device is
> > > > powered down or held in reset, respectively, which would suggest that
> > > > the specifiers should have GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW
> > > > respectively. The latter would cause the GPIO subsystem to invert the
> > > > values set by the consumers, which would then be confusing from the
> > > > driver implementation point of view.
> > > >
> > > > Should the pin be renamed to "nreset"? It would change the meaning of
> > > > "logically asserted" to "device is not held in reset" and so
> > > > GPIO_ACTIVE_HIGH (or 0) would be the right value to use.
> > > >
> > > > [1] https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/gpio/gpio.txt#L83
> > > 
> > > + Bartosz, Linus, Sakari and the linux-gpio ML for a broader audience.
> > > 
> > > Would appreciate some feedback on what's the proper way of defining
> > > GPIO polarity. Thanks!
> > > 
> > > Best regards,
> > > Tomasz
> > > 
> > 
> > I have another question about OV02A10 CMOS sensor dt-binding.
> > As its text documentation was already reviewed by Rob on earlier
> > version:
> > https://patchwork.linuxtv.org/patch/59787/
> > I wonder whether we need to convert it to DT in YAML.
> 
> Yes.
> 

Okay.
Let's focus on the new DT binding in YAML for OV02A10.

> > In fact, I just submitted one conversion version.
> > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2143922
> > 
> > Unluckily make dt_binding_check still report errors temporarily.
> > It seems there is something wrong with the port property in DT.
> > Could anyone help provide some tips?
> > $make dt_binding_check
> > DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> >   SCHEMA  Documentation/devicetree/bindings/processed-schema.yaml
> > Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml: ignoring,
> > error in schema: properties: port: patternProperties: endpoint
> > warning: no schema found in file:
> > Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > make[2]: *** [Documentation/devicetree/bindings/processed-schema.yaml]
> > Error 255
> > make[1]: *** [dt_binding_check] Error 2
> > make: *** [sub-make] Error 2
> 
>     patternProperties:
>       endpoint:
>       type: object
>       additionalProperties: false
> 
> You need more indentation under 'endpoint'. Also, 'endpoint' is a fixed 
> string, so it should be under 'properties' rather than 'patternProperties'.
> 
> 

Thanks for the reminder.
Now we could run dt_binding_check pass for the required
property:endpoint.

> > 
> > In addition, as OV02A10 use one private property to distinguish
> > different projects that adopting different register settings,
> > I would appreciate the feedback on how to add private property to DT in
> > YAML.
> 
> Like any other property. Submit something for review.
> 

Could you have the permission to see the change:
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2143922/3

This change is improved relative to earlier patchset.
But there are still some errors if we running dt_binding_check on the
change.
$make dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
  CHKDT   Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
/proj/mtk15013/new_dt_check/kernel_only_dev/kernel/mediatek/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml: Additional properties are not allowed ('optional' was unexpected)
/proj/mtk15013/new_dt_check/kernel_only_dev/kernel/mediatek/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml: Additional properties are not allowed ('optional' was unexpected)
/proj/mtk15013/new_dt_check/kernel_only_dev/kernel/mediatek/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml: properties:ovti,mipi-tx-speed: {'description': 'Indication of MIPI transmission speed select.'} is not valid under any of the given schemas (Possible causes of the failure):
        /proj/mtk15013/new_dt_check/kernel_only_dev/kernel/mediatek/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml: properties:ovti,mipi-tx-speed: 'not' is a required property

make[2]: ***
[Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.example.dts]
Error 1
make[1]: *** [dt_binding_check] Error 2
make: *** [sub-make] Error 2

It seems that we shall not use "optional" to describe private properties
for DT bindings in YAML.
Rob, could you help provide one example to tell us how to define new
properties?
Thanks a lot.

> Rob

[V6, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver

[Dongchun Zhu]

Add a V4L2 sub-device driver for OV02A10 image sensor. The OV02A10 is a
1/5" CMOS sensor from Omnivision, asupporting output format: 10-bit Raw.

This chip has a single MIPI lane interface and use the I2C bus for
control and the CSI-2 bus for data.

Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
---
 MAINTAINERS                 |    1 +
 drivers/media/i2c/Kconfig   |   11 +
 drivers/media/i2c/Makefile  |    1 +
 drivers/media/i2c/ov02a10.c | 1102 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 1115 insertions(+)
 create mode 100644 drivers/media/i2c/ov02a10.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 92a868c..e294530 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12135,6 +12135,7 @@ M:	Dongchun Zhu <dongchun.zhu@mediatek.com>
 L:	linux-media@vger.kernel.org
 T:	git git://linuxtv.org/media_tree.git
 S:	Maintained
+F:	drivers/media/i2c/ov02a10.c
 F:	Documentation/devicetree/bindings/media/i2c/ov02a10.txt
 
 OMNIVISION OV2680 SENSOR DRIVER
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index c68e002..d3e8c41 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -640,6 +640,17 @@ config VIDEO_IMX355
 	  To compile this driver as a module, choose M here: the
 	  module will be called imx355.
 
+config VIDEO_OV02A10
+	tristate "OmniVision OV02A10 sensor support"
+	depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+	select V4L2_FWNODE
+	help
+	  This is a Video4Linux2 sensor driver for the OmniVision
+	  OV02A10 camera.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ov02a10.
+
 config VIDEO_OV2640
 	tristate "OmniVision OV2640 sensor support"
 	depends on VIDEO_V4L2 && I2C
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index c147bb9..b3769d7 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -63,6 +63,7 @@ obj-$(CONFIG_VIDEO_VP27SMPX) += vp27smpx.o
 obj-$(CONFIG_VIDEO_SONY_BTF_MPX) += sony-btf-mpx.o
 obj-$(CONFIG_VIDEO_UPD64031A) += upd64031a.o
 obj-$(CONFIG_VIDEO_UPD64083) += upd64083.o
+obj-$(CONFIG_VIDEO_OV02A10) += ov02a10.o
 obj-$(CONFIG_VIDEO_OV2640) += ov2640.o
 obj-$(CONFIG_VIDEO_OV2680) += ov2680.o
 obj-$(CONFIG_VIDEO_OV2685) += ov2685.o
diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c
new file mode 100644
index 0000000..0b930f6
--- /dev/null
+++ b/drivers/media/i2c/ov02a10.c
@@ -0,0 +1,1102 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2019 MediaTek Inc.
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
+#include <media/media-entity.h>
+#include <media/v4l2-async.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-subdev.h>
+#include <media/v4l2-fwnode.h>
+
+#define CHIP_ID						0x2509
+#define OV02A10_REG_CHIP_ID_H				0x02
+#define OV02A10_REG_CHIP_ID_L				0x03
+#define OV02A10_ID(_msb, _lsb)				((_msb) << 8 | (_lsb))
+
+/* Bit[1] vertical upside down */
+/* Bit[0] horizontal mirror */
+#define REG_MIRROR_FLIP_CONTROL				0x3f
+
+/* Orientation */
+#define REG_MIRROR_FLIP_ENABLE				0x03
+
+/* Bit[7] clock HS mode enable
+ * 0: Clock continue
+ * 1: Clock HS
+ * Bit[6:2] HS VOD adjust
+ * Bit[1:0] P VHI adjust
+ */
+#define REG_HS_MODE_BLC					0x9d
+
+#define CLOCK_HS_MODE_ENABLE				BIT(7)
+
+/* Bit[2:0] MIPI transmission speed select */
+#define TX_SPEED_AREA_SEL				0xa1
+
+#define REG_PAGE_SWITCH					0xfd
+#define REG_GLOBAL_EFFECTIVE				0x01
+#define REG_ENABLE					BIT(0)
+#define OV02A10_MASK_8_BITS				0xff
+
+#define REG_SC_CTRL_MODE				0xac
+#define SC_CTRL_MODE_STANDBY				0x00
+#define SC_CTRL_MODE_STREAMING				0x01
+
+#define OV02A10_EXP_SHIFT				8
+#define OV02A10_REG_EXPOSURE_H				0x03
+#define OV02A10_REG_EXPOSURE_L				0x04
+#define	OV02A10_EXPOSURE_MIN				4
+#define OV02A10_EXPOSURE_MAX_MARGIN			4
+#define	OV02A10_EXPOSURE_STEP				1
+
+#define OV02A10_VTS_SHIFT				8
+#define OV02A10_REG_VTS_H				0x05
+#define OV02A10_REG_VTS_L				0x06
+#define OV02A10_VTS_MAX					0x209f
+#define OV02A10_VTS_MIN					0x04cf
+#define OV02A10_BASIC_LINE				1224
+
+#define OV02A10_REG_GAIN				0x24
+#define OV02A10_GAIN_MIN				0x10
+#define OV02A10_GAIN_MAX				0xf8
+#define OV02A10_GAIN_STEP				0x01
+#define OV02A10_GAIN_DEFAULT				0x40
+
+/* Test pattern control */
+#define OV02A10_REG_TEST_PATTERN			0xb6
+#define OV02A10_TEST_PATTERN_ENABLE			BIT(0)
+
+#define HZ_PER_MHZ					1000000L
+#define OV02A10_LINK_FREQ_390MHZ			(390 * HZ_PER_MHZ)
+#define OV02A10_ECLK_FREQ				(24 * HZ_PER_MHZ)
+#define OV02A10_DATA_LANES				1
+#define OV02A10_BITS_PER_SAMPLE				10
+
+static const char * const ov02a10_supply_names[] = {
+	"dovdd",	/* Digital I/O power */
+	"avdd",		/* Analog power */
+	"dvdd",		/* Digital core power */
+};
+
+#define OV02A10_NUM_SUPPLIES ARRAY_SIZE(ov02a10_supply_names)
+
+struct ov02a10_reg {
+	u8 addr;
+	u8 val;
+};
+
+struct ov02a10_reg_list {
+	u32 num_of_regs;
+	const struct ov02a10_reg *regs;
+};
+
+struct ov02a10_mode {
+	u32 width;
+	u32 height;
+	u32 exp_def;
+	u32 hts_def;
+	u32 vts_def;
+	const struct ov02a10_reg_list reg_list;
+};
+
+struct ov02a10 {
+	u32			eclk_freq;
+	u32                     mipi_clock_tx_speed;
+
+	struct clk		*eclk;
+	struct gpio_desc	*pd_gpio;
+	struct gpio_desc	*n_rst_gpio;
+	struct regulator_bulk_data supplies[OV02A10_NUM_SUPPLIES];
+
+	bool			streaming;
+	bool			upside_down;
+	bool			mipi_clock_hs_mode_enable;
+
+	/*
+	 * Serialize control access, get/set format, get selection
+	 * and start streaming.
+	 */
+	struct mutex		mutex;
+	struct v4l2_subdev	subdev;
+	struct media_pad	pad;
+	struct v4l2_ctrl	*anal_gain;
+	struct v4l2_ctrl	*exposure;
+	struct v4l2_ctrl	*hblank;
+	struct v4l2_ctrl	*vblank;
+	struct v4l2_ctrl	*test_pattern;
+	struct v4l2_mbus_framefmt	fmt;
+	struct v4l2_ctrl_handler ctrl_handler;
+
+	const struct ov02a10_mode *cur_mode;
+};
+
+static inline struct ov02a10 *to_ov02a10(struct v4l2_subdev *sd)
+{
+	return container_of(sd, struct ov02a10, subdev);
+}
+
+/*
+ * eclk 24Mhz
+ * pclk 39Mhz
+ * linelength 934(0x3a6)
+ * framelength 1390(0x56E)
+ * grabwindow_width 1600
+ * grabwindow_height 1200
+ * max_framerate 30fps
+ * mipi_datarate per lane 780Mbps
+ */
+static const struct ov02a10_reg ov02a10_1600x1200_regs[] = {
+	{0xfd, 0x01},
+	{0xac, 0x00},
+	{0xfd, 0x00},
+	{0x2f, 0x29},
+	{0x34, 0x00},
+	{0x35, 0x21},
+	{0x30, 0x15},
+	{0x33, 0x01},
+	{0xfd, 0x01},
+	{0x44, 0x00},
+	{0x2a, 0x4c},
+	{0x2b, 0x1e},
+	{0x2c, 0x60},
+	{0x25, 0x11},
+	{0x03, 0x01},
+	{0x04, 0xae},
+	{0x09, 0x00},
+	{0x0a, 0x02},
+	{0x06, 0xa6},
+	{0x31, 0x00},
+	{0x24, 0x40},
+	{0x01, 0x01},
+	{0xfb, 0x73},
+	{0xfd, 0x01},
+	{0x16, 0x04},
+	{0x1c, 0x09},
+	{0x21, 0x42},
+	{0x12, 0x04},
+	{0x13, 0x10},
+	{0x11, 0x40},
+	{0x33, 0x81},
+	{0xd0, 0x00},
+	{0xd1, 0x01},
+	{0xd2, 0x00},
+	{0x50, 0x10},
+	{0x51, 0x23},
+	{0x52, 0x20},
+	{0x53, 0x10},
+	{0x54, 0x02},
+	{0x55, 0x20},
+	{0x56, 0x02},
+	{0x58, 0x48},
+	{0x5d, 0x15},
+	{0x5e, 0x05},
+	{0x66, 0x66},
+	{0x68, 0x68},
+	{0x6b, 0x00},
+	{0x6c, 0x00},
+	{0x6f, 0x40},
+	{0x70, 0x40},
+	{0x71, 0x0a},
+	{0x72, 0xf0},
+	{0x73, 0x10},
+	{0x75, 0x80},
+	{0x76, 0x10},
+	{0x84, 0x00},
+	{0x85, 0x10},
+	{0x86, 0x10},
+	{0x87, 0x00},
+	{0x8a, 0x22},
+	{0x8b, 0x22},
+	{0x19, 0xf1},
+	{0x29, 0x01},
+	{0xfd, 0x01},
+	{0x9d, 0x16},
+	{0xa0, 0x29},
+	{0xa1, 0x03},
+	{0xad, 0x62},
+	{0xae, 0x00},
+	{0xaf, 0x85},
+	{0xb1, 0x01},
+	{0x8e, 0x06},
+	{0x8f, 0x40},
+	{0x90, 0x04},
+	{0x91, 0xb0},
+	{0x45, 0x01},
+	{0x46, 0x00},
+	{0x47, 0x6c},
+	{0x48, 0x03},
+	{0x49, 0x8b},
+	{0x4a, 0x00},
+	{0x4b, 0x07},
+	{0x4c, 0x04},
+	{0x4d, 0xb7},
+	{0xf0, 0x40},
+	{0xf1, 0x40},
+	{0xf2, 0x40},
+	{0xf3, 0x40},
+	{0x3f, 0x00},
+	{0xfd, 0x01},
+	{0x05, 0x00},
+	{0x06, 0xa6},
+	{0xfd, 0x01},
+};
+
+static const char * const ov02a10_test_pattern_menu[] = {
+	"Disabled",
+	"Color Bar",
+};
+
+static const s64 link_freq_menu_items[] = {
+	OV02A10_LINK_FREQ_390MHZ,
+};
+
+static u64 to_pixel_rate(u32 f_index)
+{
+	u64 pixel_rate = link_freq_menu_items[f_index] * 2 * OV02A10_DATA_LANES;
+
+	do_div(pixel_rate, OV02A10_BITS_PER_SAMPLE);
+
+	return pixel_rate;
+}
+
+static const struct ov02a10_mode supported_modes[] = {
+	{
+		.width = 1600,
+		.height = 1200,
+		.exp_def = 0x01ae,
+		.hts_def = 0x03a6,
+		.vts_def = 0x056e,
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(ov02a10_1600x1200_regs),
+			.regs = ov02a10_1600x1200_regs,
+		},
+	},
+};
+
+static int ov02a10_write_array(struct ov02a10 *ov02a10,
+			       const struct ov02a10_reg_list *r_list)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
+	unsigned int i;
+	int ret;
+
+	for (i = 0; i < r_list->num_of_regs; i++) {
+		ret = i2c_smbus_write_byte_data(client, r_list->regs[i].addr,
+						r_list->regs[i].val);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int ov02a10_read_smbus(struct ov02a10 *ov02a10, unsigned char reg,
+			      unsigned char *val)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(client, reg);
+
+	if (ret < 0)
+		return ret;
+
+	*val = (unsigned char)ret;
+
+	return 0;
+}
+
+static int ov02a10_mod_reg(struct ov02a10 *ov02a10, u8 reg, u8 mask, u8 val)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
+	u8 readval;
+	int ret;
+
+	ret = ov02a10_read_smbus(ov02a10, reg, &readval);
+	if (ret)
+		return ret;
+
+	val = (readval & ~mask) | (val & mask);
+
+	return i2c_smbus_write_byte_data(client, reg, val);
+}
+
+static void ov02a10_fill_fmt(const struct ov02a10_mode *mode,
+			     struct v4l2_mbus_framefmt *fmt)
+{
+	fmt->width = mode->width;
+	fmt->height = mode->height;
+	fmt->field = V4L2_FIELD_NONE;
+}
+
+static int ov02a10_set_fmt(struct v4l2_subdev *sd,
+			   struct v4l2_subdev_pad_config *cfg,
+			   struct v4l2_subdev_format *fmt)
+{
+	struct ov02a10 *ov02a10 = to_ov02a10(sd);
+	struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
+
+	mutex_lock(&ov02a10->mutex);
+
+	if (ov02a10->streaming) {
+		mutex_unlock(&ov02a10->mutex);
+		return -EBUSY;
+	}
+
+	/* Only one sensor mode supported */
+	mbus_fmt->code = ov02a10->fmt.code;
+	ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt);
+	ov02a10->fmt = fmt->format;
+
+	mutex_unlock(&ov02a10->mutex);
+
+	return 0;
+}
+
+static int ov02a10_get_fmt(struct v4l2_subdev *sd,
+			   struct v4l2_subdev_pad_config *cfg,
+			   struct v4l2_subdev_format *fmt)
+{
+	struct ov02a10 *ov02a10 = to_ov02a10(sd);
+	struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
+
+	mutex_lock(&ov02a10->mutex);
+
+	fmt->format = ov02a10->fmt;
+	mbus_fmt->code = ov02a10->fmt.code;
+	ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt);
+
+	mutex_unlock(&ov02a10->mutex);
+
+	return 0;
+}
+
+static int ov02a10_enum_mbus_code(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_pad_config *cfg,
+				  struct v4l2_subdev_mbus_code_enum *code)
+{
+	struct ov02a10 *ov02a10 = to_ov02a10(sd);
+
+	if (code->index >= ARRAY_SIZE(supported_modes))
+		return -EINVAL;
+
+	code->code = ov02a10->fmt.code;
+
+	return 0;
+}
+
+static int ov02a10_enum_frame_sizes(struct v4l2_subdev *sd,
+				    struct v4l2_subdev_pad_config *cfg,
+				    struct v4l2_subdev_frame_size_enum *fse)
+{
+	if (fse->index >= ARRAY_SIZE(supported_modes))
+		return -EINVAL;
+
+	fse->min_width  = supported_modes[fse->index].width;
+	fse->max_width  = supported_modes[fse->index].width;
+	fse->max_height = supported_modes[fse->index].height;
+	fse->min_height = supported_modes[fse->index].height;
+
+	return 0;
+}
+
+static int ov02a10_check_sensor_id(struct ov02a10 *ov02a10)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
+	u16 id;
+	u8 chip_id_h;
+	u8 chip_id_l;
+	int ret;
+
+	/* Check sensor revision */
+	ret = ov02a10_read_smbus(ov02a10, OV02A10_REG_CHIP_ID_H, &chip_id_h);
+	if (ret)
+		return ret;
+
+	ret = ov02a10_read_smbus(ov02a10, OV02A10_REG_CHIP_ID_L, &chip_id_l);
+	if (ret)
+		return ret;
+
+	id = OV02A10_ID(chip_id_h, chip_id_l);
+	if (id != CHIP_ID) {
+		dev_err(&client->dev, "Unexpected sensor id(%04x)\n", id);
+		return -EINVAL;
+	}
+
+	dev_dbg(&client->dev, "Detected OV%04X sensor\n", id);
+
+	return 0;
+}
+
+static int __maybe_unused ov02a10_power_on(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct ov02a10 *ov02a10 = to_ov02a10(sd);
+	int ret;
+
+	gpiod_set_value_cansleep(ov02a10->n_rst_gpio, 0);
+	gpiod_set_value_cansleep(ov02a10->pd_gpio, 1);
+
+	ret = clk_prepare_enable(ov02a10->eclk);
+	if (ret < 0) {
+		dev_err(dev, "failed to enable eclk\n");
+		return ret;
+	}
+
+	ret = regulator_bulk_enable(OV02A10_NUM_SUPPLIES, ov02a10->supplies);
+	if (ret < 0) {
+		dev_err(dev, "failed to enable regulators\n");
+		goto disable_clk;
+	}
+	usleep_range(5000, 6000);
+
+	gpiod_set_value_cansleep(ov02a10->pd_gpio, 0);
+	usleep_range(5000, 6000);
+
+	gpiod_set_value_cansleep(ov02a10->n_rst_gpio, 1);
+	usleep_range(5000, 6000);
+
+	ret = ov02a10_check_sensor_id(ov02a10);
+	if (ret)
+		goto disable_regulator;
+
+	return 0;
+
+disable_regulator:
+	regulator_bulk_disable(OV02A10_NUM_SUPPLIES, ov02a10->supplies);
+disable_clk:
+	clk_disable_unprepare(ov02a10->eclk);
+
+	return ret;
+}
+
+static int __maybe_unused ov02a10_power_off(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct ov02a10 *ov02a10 = to_ov02a10(sd);
+
+	gpiod_set_value_cansleep(ov02a10->n_rst_gpio, 0);
+	clk_disable_unprepare(ov02a10->eclk);
+	gpiod_set_value_cansleep(ov02a10->pd_gpio, 1);
+	regulator_bulk_disable(OV02A10_NUM_SUPPLIES, ov02a10->supplies);
+
+	return 0;
+}
+
+static int __ov02a10_start_stream(struct ov02a10 *ov02a10)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
+	const struct ov02a10_reg_list *reg_list;
+	int ret;
+
+	/* Apply default values of current mode */
+	reg_list = &ov02a10->cur_mode->reg_list;
+	ret = ov02a10_write_array(ov02a10, reg_list);
+	if (ret)
+		return ret;
+
+	/* Apply customized values from user */
+	ret = __v4l2_ctrl_handler_setup(ov02a10->subdev.ctrl_handler);
+	if (ret)
+		return ret;
+
+	/* Set orientation to 180 degree */
+	if (ov02a10->upside_down) {
+		ret = i2c_smbus_write_byte_data(client, REG_MIRROR_FLIP_CONTROL,
+						REG_MIRROR_FLIP_ENABLE);
+		if (ret) {
+			dev_err(&client->dev, "failed to set orientation\n");
+			return ret;
+		}
+		ret = i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
+						REG_ENABLE);
+		if (ret < 0)
+			return ret;
+	}
+
+	/* Set clock lane transmission mode according to DT property */
+	ret = ov02a10_mod_reg(ov02a10, REG_HS_MODE_BLC, CLOCK_HS_MODE_ENABLE,
+			      ov02a10->mipi_clock_hs_mode_enable ?
+			      CLOCK_HS_MODE_ENABLE : 0);
+	if (ret < 0)
+		return ret;
+
+	/* Set mipi TX speed according to DT property */
+	ret = i2c_smbus_write_byte_data(client, TX_SPEED_AREA_SEL,
+					ov02a10->mipi_clock_tx_speed);
+	if (ret < 0)
+		return ret;
+
+	/* Set stream on register */
+	return i2c_smbus_write_byte_data(client, REG_SC_CTRL_MODE,
+					 SC_CTRL_MODE_STREAMING);
+}
+
+static int __ov02a10_stop_stream(struct ov02a10 *ov02a10)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
+
+	return i2c_smbus_write_byte_data(client, REG_SC_CTRL_MODE,
+					 SC_CTRL_MODE_STANDBY);
+}
+
+static int ov02a10_entity_init_cfg(struct v4l2_subdev *sd,
+				   struct v4l2_subdev_pad_config *cfg)
+{
+	struct v4l2_subdev_format fmt = {
+		.which = cfg ? V4L2_SUBDEV_FORMAT_TRY
+			     : V4L2_SUBDEV_FORMAT_ACTIVE,
+		.format = {
+			.width = 1600,
+			.height = 1200
+		}
+	};
+
+	ov02a10_set_fmt(sd, cfg, &fmt);
+
+	return 0;
+}
+
+static int ov02a10_s_stream(struct v4l2_subdev *sd, int on)
+{
+	struct ov02a10 *ov02a10 = to_ov02a10(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
+	int ret;
+
+	dev_dbg(&client->dev, "ov02a10 s_stream (%d)\n", on);
+	mutex_lock(&ov02a10->mutex);
+
+	if (ov02a10->streaming == on)
+		goto unlock_and_return;
+
+	if (on) {
+		ret = pm_runtime_get_sync(&client->dev);
+		if (ret < 0) {
+			pm_runtime_put_noidle(&client->dev);
+			goto unlock_and_return;
+		}
+
+		ret = __ov02a10_start_stream(ov02a10);
+		if (ret) {
+			__ov02a10_stop_stream(ov02a10);
+			ov02a10->streaming = !on;
+			goto err_rpm_put;
+		}
+	} else {
+		__ov02a10_stop_stream(ov02a10);
+		pm_runtime_put(&client->dev);
+	}
+
+	ov02a10->streaming = on;
+	mutex_unlock(&ov02a10->mutex);
+
+	return 0;
+
+err_rpm_put:
+	pm_runtime_put(&client->dev);
+unlock_and_return:
+	mutex_unlock(&ov02a10->mutex);
+
+	return ret;
+}
+
+static const struct dev_pm_ops ov02a10_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+	SET_RUNTIME_PM_OPS(ov02a10_power_off, ov02a10_power_on, NULL)
+};
+
+/*
+ * ov02a10_set_exposure - Function called when setting exposure time
+ * @priv: Pointer to device structure
+ * @val: Variable for exposure time, in the unit of micro-second
+ *
+ * Set exposure time based on input value.
+ *
+ * Return: 0 on success
+ */
+static int ov02a10_set_exposure(struct ov02a10 *ov02a10, int val)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
+	int ret;
+
+	ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE);
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_smbus_write_byte_data(client, OV02A10_REG_EXPOSURE_H,
+					((val >> OV02A10_EXP_SHIFT) &
+					OV02A10_MASK_8_BITS));
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_smbus_write_byte_data(client, OV02A10_REG_EXPOSURE_L,
+					(val & OV02A10_MASK_8_BITS));
+	if (ret < 0)
+		return ret;
+
+	return i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
+					 REG_ENABLE);
+}
+
+static int ov02a10_set_gain(struct ov02a10 *ov02a10, int val)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
+	int ret;
+
+	ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE);
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_smbus_write_byte_data(client, OV02A10_REG_GAIN,
+					(val & OV02A10_MASK_8_BITS));
+	if (ret < 0)
+		return ret;
+
+	return i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
+					 REG_ENABLE);
+}
+
+static int ov02a10_set_vblank(struct ov02a10 *ov02a10, int val)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
+	int ret;
+
+	ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE);
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_H,
+					(((val + ov02a10->cur_mode->height -
+					OV02A10_BASIC_LINE) >>
+					OV02A10_VTS_SHIFT) &
+					OV02A10_MASK_8_BITS));
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_L,
+					((val + ov02a10->cur_mode->height -
+					OV02A10_BASIC_LINE) &
+					OV02A10_MASK_8_BITS));
+	if (ret < 0)
+		return ret;
+
+	return i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
+					 REG_ENABLE);
+}
+
+static int ov02a10_set_test_pattern(struct ov02a10 *ov02a10, int pattern)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
+	int ret;
+
+	if (pattern)
+		pattern = OV02A10_TEST_PATTERN_ENABLE;
+
+	ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE);
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_smbus_write_byte_data(client, OV02A10_REG_TEST_PATTERN,
+					pattern);
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
+					REG_ENABLE);
+	if (ret < 0)
+		return ret;
+
+	return i2c_smbus_write_byte_data(client, REG_SC_CTRL_MODE,
+					 SC_CTRL_MODE_STREAMING);
+}
+
+static int ov02a10_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct ov02a10 *ov02a10 = container_of(ctrl->handler,
+					       struct ov02a10, ctrl_handler);
+	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
+	s64 max_expo;
+	int ret;
+
+	/* Propagate change of current control to all related controls */
+	if (ctrl->id == V4L2_CID_VBLANK) {
+		/* Update max exposure while meeting expected vblanking */
+		max_expo = ov02a10->cur_mode->height + ctrl->val -
+			   OV02A10_EXPOSURE_MAX_MARGIN;
+		__v4l2_ctrl_modify_range(ov02a10->exposure,
+					 ov02a10->exposure->minimum, max_expo,
+					 ov02a10->exposure->step,
+					 ov02a10->exposure->default_value);
+	}
+
+	/* V4L2 controls values will be applied only when power is already up */
+	if (!pm_runtime_get_if_in_use(&client->dev))
+		return 0;
+
+	switch (ctrl->id) {
+	case V4L2_CID_EXPOSURE:
+		ret = ov02a10_set_exposure(ov02a10, ctrl->val);
+		break;
+	case V4L2_CID_ANALOGUE_GAIN:
+		ret = ov02a10_set_gain(ov02a10, ctrl->val);
+		break;
+	case V4L2_CID_VBLANK:
+		ret = ov02a10_set_vblank(ov02a10, ctrl->val);
+		break;
+	case V4L2_CID_TEST_PATTERN:
+		ret = ov02a10_set_test_pattern(ov02a10, ctrl->val);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	};
+
+	pm_runtime_put(&client->dev);
+
+	return ret;
+}
+
+static const struct v4l2_subdev_video_ops ov02a10_video_ops = {
+	.s_stream = ov02a10_s_stream,
+};
+
+static const struct v4l2_subdev_pad_ops ov02a10_pad_ops = {
+	.init_cfg = ov02a10_entity_init_cfg,
+	.enum_mbus_code = ov02a10_enum_mbus_code,
+	.enum_frame_size = ov02a10_enum_frame_sizes,
+	.get_fmt = ov02a10_get_fmt,
+	.set_fmt = ov02a10_set_fmt,
+};
+
+static const struct v4l2_subdev_ops ov02a10_subdev_ops = {
+	.video	= &ov02a10_video_ops,
+	.pad	= &ov02a10_pad_ops,
+};
+
+static const struct media_entity_operations ov02a10_subdev_entity_ops = {
+	.link_validate = v4l2_subdev_link_validate,
+};
+
+static const struct v4l2_ctrl_ops ov02a10_ctrl_ops = {
+	.s_ctrl = ov02a10_set_ctrl,
+};
+
+static int ov02a10_initialize_controls(struct ov02a10 *ov02a10)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
+	const struct ov02a10_mode *mode;
+	struct v4l2_ctrl_handler *handler;
+	struct v4l2_ctrl *ctrl;
+	u64 exposure_max;
+	u32 pixel_rate, h_blank;
+	int ret;
+
+	handler = &ov02a10->ctrl_handler;
+	mode = ov02a10->cur_mode;
+	ret = v4l2_ctrl_handler_init(handler, 7);
+	if (ret)
+		return ret;
+
+	handler->lock = &ov02a10->mutex;
+
+	ctrl = v4l2_ctrl_new_int_menu(handler, NULL, V4L2_CID_LINK_FREQ, 0, 0,
+				      link_freq_menu_items);
+	if (ctrl)
+		ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+	pixel_rate = to_pixel_rate(0);
+	v4l2_ctrl_new_std(handler, NULL, V4L2_CID_PIXEL_RATE, 0, pixel_rate, 1,
+			  pixel_rate);
+
+	h_blank = mode->hts_def - mode->width;
+	ov02a10->hblank = v4l2_ctrl_new_std(handler, NULL, V4L2_CID_HBLANK,
+					    h_blank, h_blank, 1, h_blank);
+	if (ov02a10->hblank)
+		ov02a10->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+	ov02a10->vblank = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops,
+					    V4L2_CID_VBLANK, mode->vts_def -
+					    mode->height,
+					    OV02A10_VTS_MAX - mode->height, 1,
+					    mode->vts_def - mode->height);
+
+	exposure_max = mode->vts_def - 4;
+	ov02a10->exposure = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops,
+					      V4L2_CID_EXPOSURE,
+					      OV02A10_EXPOSURE_MIN,
+					      exposure_max,
+					      OV02A10_EXPOSURE_STEP,
+					      mode->exp_def);
+
+	ov02a10->anal_gain = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops,
+					       V4L2_CID_ANALOGUE_GAIN,
+					       OV02A10_GAIN_MIN,
+					       OV02A10_GAIN_MAX,
+					       OV02A10_GAIN_STEP,
+					       OV02A10_GAIN_DEFAULT);
+
+	ov02a10->test_pattern =
+		v4l2_ctrl_new_std_menu_items(handler, &ov02a10_ctrl_ops,
+					     V4L2_CID_TEST_PATTERN,
+					     ARRAY_SIZE(ov02a10_test_pattern_menu) -
+					     1, 0, 0,
+					     ov02a10_test_pattern_menu);
+
+	if (handler->error) {
+		ret = handler->error;
+		dev_err(&client->dev, "failed to init controls(%d)\n", ret);
+		goto err_free_handler;
+	}
+
+	ov02a10->subdev.ctrl_handler = handler;
+
+	return 0;
+
+err_free_handler:
+	v4l2_ctrl_handler_free(handler);
+
+	return ret;
+}
+
+static int ov02a10_check_hwcfg(struct device *dev, struct ov02a10 *ov02a10)
+{
+	struct fwnode_handle *ep;
+	struct fwnode_handle *fwnode = dev_fwnode(dev);
+	struct v4l2_fwnode_endpoint bus_cfg = {
+		.bus_type = V4L2_MBUS_CSI2_DPHY
+	};
+	unsigned int i, j;
+	int ret;
+
+	if (!fwnode)
+		return -ENXIO;
+
+	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
+	if (!ep)
+		return -ENXIO;
+
+	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
+	fwnode_handle_put(ep);
+	if (ret)
+		return ret;
+
+	/* Optional indication of mipi clock lane mode */
+	if (bus_cfg.bus.mipi_csi2.flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK)
+		ov02a10->mipi_clock_hs_mode_enable = true;
+
+	if (!bus_cfg.nr_of_link_frequencies) {
+		dev_err(dev, "no link frequencies defined");
+		ret = -EINVAL;
+		goto check_hwcfg_error;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(link_freq_menu_items); i++) {
+		for (j = 0; j < bus_cfg.nr_of_link_frequencies; j++) {
+			if (link_freq_menu_items[i] ==
+				bus_cfg.link_frequencies[j])
+				break;
+		}
+
+		if (j == bus_cfg.nr_of_link_frequencies) {
+			dev_err(dev, "no link frequency %lld supported",
+				link_freq_menu_items[i]);
+			ret = -EINVAL;
+			goto check_hwcfg_error;
+		}
+	}
+
+check_hwcfg_error:
+	v4l2_fwnode_endpoint_free(&bus_cfg);
+
+	return ret;
+}
+
+static int ov02a10_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct ov02a10 *ov02a10;
+	unsigned int rotation;
+	unsigned int clock_lane_tx_speed;
+	unsigned int i;
+	int ret;
+
+	ov02a10 = devm_kzalloc(dev, sizeof(*ov02a10), GFP_KERNEL);
+	if (!ov02a10)
+		return -ENOMEM;
+
+	ret = ov02a10_check_hwcfg(dev, ov02a10);
+	if (ret) {
+		dev_err(dev, "failed to check HW configuration: %d", ret);
+		return ret;
+	}
+
+	v4l2_i2c_subdev_init(&ov02a10->subdev, client, &ov02a10_subdev_ops);
+	ov02a10->upside_down = false;
+	ov02a10->fmt.code = MEDIA_BUS_FMT_SBGGR10_1X10;
+
+	/* Optional indication of physical rotation of sensor */
+	ret = fwnode_property_read_u32(dev_fwnode(dev), "rotation", &rotation);
+	if (!ret) {
+		ov02a10->upside_down = rotation == 180;
+		if (rotation == 180) {
+			ov02a10->upside_down = true;
+			ov02a10->fmt.code = MEDIA_BUS_FMT_SRGGB10_1X10;
+		}
+	} else {
+		dev_warn(dev, "failed to get rotation\n");
+	}
+
+	/* Optional indication of mipi TX speed */
+	ret = fwnode_property_read_u32(dev_fwnode(dev), "ovti,mipi-tx-speed",
+				       &clock_lane_tx_speed);
+
+	if (!ret)
+		ov02a10->mipi_clock_tx_speed = clock_lane_tx_speed;
+	else
+		dev_warn(dev, "failed to get mipi tx speed, using default...\n");
+
+	/* Get system clock (eclk) */
+	ov02a10->eclk = devm_clk_get(dev, "eclk");
+	if (IS_ERR(ov02a10->eclk)) {
+		ret = PTR_ERR(ov02a10->eclk);
+		dev_err(dev, "failed to get eclk %d\n", ret);
+		return ret;
+	}
+
+	ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
+				       &ov02a10->eclk_freq);
+	if (ret) {
+		dev_err(dev, "failed to get eclk frequency\n");
+		return ret;
+	}
+
+	ret = clk_set_rate(ov02a10->eclk, ov02a10->eclk_freq);
+	if (ret) {
+		dev_err(dev, "failed to set eclk frequency (24MHz)\n");
+		return ret;
+	}
+
+	if (clk_get_rate(ov02a10->eclk) != OV02A10_ECLK_FREQ) {
+		dev_warn(dev, "wrong eclk frequency %d Hz, expected: %d Hz\n",
+			 ov02a10->eclk_freq, OV02A10_ECLK_FREQ);
+		return -EINVAL;
+	}
+
+	ov02a10->pd_gpio = devm_gpiod_get(dev, "powerdown", GPIOD_OUT_HIGH);
+	if (IS_ERR(ov02a10->pd_gpio)) {
+		ret = PTR_ERR(ov02a10->pd_gpio);
+		dev_err(dev, "failed to get powerdown-gpios %d\n", ret);
+		return ret;
+	}
+
+	ov02a10->n_rst_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(ov02a10->n_rst_gpio)) {
+		ret = PTR_ERR(ov02a10->n_rst_gpio);
+		dev_err(dev, "failed to get reset-gpios %d\n", ret);
+		return ret;
+	}
+
+	for (i = 0; i < OV02A10_NUM_SUPPLIES; i++)
+		ov02a10->supplies[i].supply = ov02a10_supply_names[i];
+
+	ret = devm_regulator_bulk_get(dev, OV02A10_NUM_SUPPLIES,
+				      ov02a10->supplies);
+	if (ret) {
+		dev_err(dev, "failed to get regulators\n");
+		return ret;
+	}
+
+	mutex_init(&ov02a10->mutex);
+	ov02a10->cur_mode = &supported_modes[0];
+	ret = ov02a10_initialize_controls(ov02a10);
+	if (ret) {
+		dev_err(dev, "failed to initialize controls\n");
+		goto err_destroy_mutex;
+	}
+
+	ov02a10->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	ov02a10->subdev.entity.ops = &ov02a10_subdev_entity_ops;
+	ov02a10->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
+	ov02a10->pad.flags = MEDIA_PAD_FL_SOURCE;
+	ret = media_entity_pads_init(&ov02a10->subdev.entity, 1, &ov02a10->pad);
+	if (ret < 0) {
+		dev_err(dev, "failed to init entity pads: %d", ret);
+		goto err_free_handler;
+	}
+
+	ret = v4l2_async_register_subdev(&ov02a10->subdev);
+	if (ret) {
+		dev_err(dev, "failed to register V4L2 subdev: %d", ret);
+		goto err_clean_entity;
+	}
+
+	pm_runtime_enable(dev);
+	if (!pm_runtime_enabled(dev)) {
+		ret = ov02a10_power_on(dev);
+		if (ret < 0) {
+			dev_err(dev, "failed to power on: %d\n", ret);
+			goto err_clean_entity;
+		}
+	}
+
+	return 0;
+
+err_clean_entity:
+	media_entity_cleanup(&ov02a10->subdev.entity);
+err_free_handler:
+	v4l2_ctrl_handler_free(ov02a10->subdev.ctrl_handler);
+err_destroy_mutex:
+	mutex_destroy(&ov02a10->mutex);
+
+	return ret;
+}
+
+static int ov02a10_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct ov02a10 *ov02a10 = to_ov02a10(sd);
+
+	v4l2_async_unregister_subdev(sd);
+	media_entity_cleanup(&sd->entity);
+	v4l2_ctrl_handler_free(sd->ctrl_handler);
+	pm_runtime_disable(&client->dev);
+	if (!pm_runtime_status_suspended(&client->dev))
+		ov02a10_power_off(&client->dev);
+	pm_runtime_set_suspended(&client->dev);
+	mutex_destroy(&ov02a10->mutex);
+
+	return 0;
+}
+
+static const struct of_device_id ov02a10_of_match[] = {
+	{ .compatible = "ovti,ov02a10" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, ov02a10_of_match);
+
+static struct i2c_driver ov02a10_i2c_driver = {
+	.driver = {
+		.name = "ov02a10",
+		.pm = &ov02a10_pm_ops,
+		.of_match_table = ov02a10_of_match,
+	},
+	.probe_new	= &ov02a10_probe,
+	.remove		= &ov02a10_remove,
+};
+
+module_i2c_driver(ov02a10_i2c_driver);
+
+MODULE_AUTHOR("Dongchun Zhu <dongchun.zhu@mediatek.com>");
+MODULE_DESCRIPTION("OmniVision OV02A10 sensor driver");
+MODULE_LICENSE("GPL v2");
+
-- 
2.9.2

Re: [V6, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver

[Andy Shevchenko]

On Wed, Dec 11, 2019 at 07:28:49PM +0800, Dongchun Zhu wrote:
> Add a V4L2 sub-device driver for OV02A10 image sensor. The OV02A10 is a
> 1/5" CMOS sensor from Omnivision, asupporting output format: 10-bit Raw.
> 
> This chip has a single MIPI lane interface and use the I2C bus for
> control and the CSI-2 bus for data.

...

> +#define OV02A10_MASK_8_BITS                            0xff

Besides GENMASK() why do you need a definition here? What's the point?

...

> +static int ov02a10_entity_init_cfg(struct v4l2_subdev *sd,
> +				   struct v4l2_subdev_pad_config *cfg)
> +{
> +	struct v4l2_subdev_format fmt = {
> +		.which = cfg ? V4L2_SUBDEV_FORMAT_TRY
> +			     : V4L2_SUBDEV_FORMAT_ACTIVE,
> +		.format = {
> +			.width = 1600,

> +			.height = 1200

Leave comma here.

> +		}
> +	};
> +
> +	ov02a10_set_fmt(sd, cfg, &fmt);
> +
> +	return 0;
> +}

...

> +	ret = i2c_smbus_write_byte_data(client, OV02A10_REG_GAIN,
> +					(val & OV02A10_MASK_8_BITS));

Too many parentheses.

> +	if (ret < 0)
> +		return ret;

...

> +static int ov02a10_set_vblank(struct ov02a10 *ov02a10, int val)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);

if you do

	int vts = val + ov02a10->cur_mode->height - OV02A10_BASIC_LINE;

you may increase readability below...

> +	int ret;
> +
> +	ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_H,
> +					(((val + ov02a10->cur_mode->height -
> +					OV02A10_BASIC_LINE) >>
> +					OV02A10_VTS_SHIFT) &
> +					OV02A10_MASK_8_BITS));

	ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_H,
					(vts >> OV02A10_VTS_SHIFT) &
					OV02A10_MASK_8_BITS));

And actually why do you need this mask here? Isn't enough to call

	ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_H,
					vts >> OV02A10_VTS_SHIFT);

here...


> +	if (ret < 0)
> +		return ret;
> +
> +	ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_L,
> +					((val + ov02a10->cur_mode->height -
> +					OV02A10_BASIC_LINE) &
> +					OV02A10_MASK_8_BITS));

...and

	ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_L, vts);

here?

> +	if (ret < 0)
> +		return ret;
> +
> +	return i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
> +					 REG_ENABLE);
> +}

...

> +static int ov02a10_check_hwcfg(struct device *dev, struct ov02a10 *ov02a10)
> +{
> +	struct fwnode_handle *ep;
> +	struct fwnode_handle *fwnode = dev_fwnode(dev);
> +	struct v4l2_fwnode_endpoint bus_cfg = {

> +		.bus_type = V4L2_MBUS_CSI2_DPHY

Leave comma here.

> +	};
> +	unsigned int i, j;
> +	int ret;

> +	if (!fwnode)
> +		return -ENXIO;

A bit strange error code here.

> +
> +	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
> +	if (!ep)
> +		return -ENXIO;
> +
> +	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> +	fwnode_handle_put(ep);
> +	if (ret)
> +		return ret;

> +	if (!bus_cfg.nr_of_link_frequencies) {
> +		dev_err(dev, "no link frequencies defined");
> +		ret = -EINVAL;
> +		goto check_hwcfg_error;
> +	}

I still think it's redundant check, though it's up to maintainers.

> +
> +	for (i = 0; i < ARRAY_SIZE(link_freq_menu_items); i++) {
> +		for (j = 0; j < bus_cfg.nr_of_link_frequencies; j++) {
> +			if (link_freq_menu_items[i] ==
> +				bus_cfg.link_frequencies[j])
> +				break;
> +		}
> +
> +		if (j == bus_cfg.nr_of_link_frequencies) {
> +			dev_err(dev, "no link frequency %lld supported",
> +				link_freq_menu_items[i]);
> +			ret = -EINVAL;
> +			goto check_hwcfg_error;
> +		}
> +	}
> +
> +check_hwcfg_error:
> +	v4l2_fwnode_endpoint_free(&bus_cfg);
> +
> +	return ret;
> +}

...

> +static int ov02a10_probe(struct i2c_client *client)
> +{

> +	/* Optional indication of physical rotation of sensor */
> +	ret = fwnode_property_read_u32(dev_fwnode(dev), "rotation", &rotation);

> +	if (!ret) {

Why not positive conditional?

> +		ov02a10->upside_down = rotation == 180;
> +		if (rotation == 180) {
> +			ov02a10->upside_down = true;
> +			ov02a10->fmt.code = MEDIA_BUS_FMT_SRGGB10_1X10;
> +		}
> +	} else {
> +		dev_warn(dev, "failed to get rotation\n");
> +	}
> +
> +	/* Optional indication of mipi TX speed */
> +	ret = fwnode_property_read_u32(dev_fwnode(dev), "ovti,mipi-tx-speed",
> +				       &clock_lane_tx_speed);
> +

> +	if (!ret)

Ditto.

> +		ov02a10->mipi_clock_tx_speed = clock_lane_tx_speed;
> +	else
> +		dev_warn(dev, "failed to get mipi tx speed, using default...\n");
> +

> +	return ret;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [V6, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver

[Dongchun Zhu]

Hello Andy,

Thanks for the review. Sorry for the late reply.

On Mon, 2019-12-11 at 16:36 +0200, Andy Shevchenko wrote:
> On Wed, Dec 11, 2019 at 07:28:49PM +0800, Dongchun Zhu wrote:
> > Add a V4L2 sub-device driver for OV02A10 image sensor. The OV02A10 is
> > a 1/5" CMOS sensor from Omnivision, asupporting output format: 10-bit Raw.
> >
> > This chip has a single MIPI lane interface and use the I2C bus for
> > control and the CSI-2 bus for data.
> 
> ...
> 
> > +#define OV02A10_MASK_8_BITS                            0xff
> 
> Besides GENMASK() why do you need a definition here? What's the point?
> 

Fixed in next release.

> ...
> 
> > +static int ov02a10_entity_init_cfg(struct v4l2_subdev *sd,
> > +   struct v4l2_subdev_pad_config *cfg) {
> > +struct v4l2_subdev_format fmt = {
> > +.which = cfg ? V4L2_SUBDEV_FORMAT_TRY
> > +     : V4L2_SUBDEV_FORMAT_ACTIVE,
> > +.format = {
> > +.width = 1600,
> 
> > +.height = 1200
> 
> Leave comma here.
> 

Fixed in next release.

> > +}
> > +};
> > +
> > +ov02a10_set_fmt(sd, cfg, &fmt);
> > +
> > +return 0;
> > +}
> 
> ...
> 
> > +ret = i2c_smbus_write_byte_data(client, OV02A10_REG_GAIN,
> > +(val & OV02A10_MASK_8_BITS));
> 
> Too many parentheses.
> 

Fixed in next release.

> > +if (ret < 0)
> > +return ret;
> 
> ...
> 
> > +static int ov02a10_set_vblank(struct ov02a10 *ov02a10, int val) {
> > +struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> 
> if you do
> 
> int vts = val + ov02a10->cur_mode->height - OV02A10_BASIC_LINE;
> 
> you may increase readability below...
> 

Thanks for the suggestion.
It seems better now.

> > +int ret;
> > +
> > +ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE);
> > +if (ret < 0)
> > +return ret;
> > +
> > +ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_H,
> > +(((val + ov02a10->cur_mode->height -
> > +OV02A10_BASIC_LINE) >>
> > +OV02A10_VTS_SHIFT) &
> > +OV02A10_MASK_8_BITS));
> 
> ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_H,
> (vts >> OV02A10_VTS_SHIFT) &
> OV02A10_MASK_8_BITS));
> 
> And actually why do you need this mask here? Isn't enough to call
> 
> ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_H,
> vts >> OV02A10_VTS_SHIFT);
> 
> here...
> 
> 

Yes. Now we code like this.

> > +if (ret < 0)
> > +return ret;
> > +
> > +ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_L,
> > +((val + ov02a10->cur_mode->height -
> > +OV02A10_BASIC_LINE) &
> > +OV02A10_MASK_8_BITS));
> 
> ...and
> 
> ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_L, vts);
> 
> here?
> 

Yes. Fixed in next release.

> > +if (ret < 0)
> > +return ret;
> > +
> > +return i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
> > + REG_ENABLE);
> > +}
> 
> ...
> 
> > +static int ov02a10_check_hwcfg(struct device *dev, struct ov02a10
> > +*ov02a10) {
> > +struct fwnode_handle *ep;
> > +struct fwnode_handle *fwnode = dev_fwnode(dev);
> > +struct v4l2_fwnode_endpoint bus_cfg = {
> 
> > +.bus_type = V4L2_MBUS_CSI2_DPHY
> 
> Leave comma here.
> 

Fixed in next release.

> > +};
> > +unsigned int i, j;
> > +int ret;
> 
> > +if (!fwnode)
> > +return -ENXIO;
> 
> A bit strange error code here.
> 

This should be reported as -EINVAL.
Fixed in next release.

> > +
> > +ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
> > +if (!ep)
> > +return -ENXIO;
> > +
> > +ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> > +fwnode_handle_put(ep);
> > +if (ret)
> > +return ret;
> 
> > +if (!bus_cfg.nr_of_link_frequencies) {
> > +dev_err(dev, "no link frequencies defined");
> > +ret = -EINVAL;
> > +goto check_hwcfg_error;
> > +}
> 
> I still think it's redundant check, though it's up to maintainers.
> 

We still wanna keep this check.
Keep same as ov2659 and ov8856.

> > +
> > +for (i = 0; i < ARRAY_SIZE(link_freq_menu_items); i++) {
> > +for (j = 0; j < bus_cfg.nr_of_link_frequencies; j++) {
> > +if (link_freq_menu_items[i] ==
> > +bus_cfg.link_frequencies[j])
> > +break;
> > +}
> > +
> > +if (j == bus_cfg.nr_of_link_frequencies) {
> > +dev_err(dev, "no link frequency %lld supported",
> > +link_freq_menu_items[i]);
> > +ret = -EINVAL;
> > +goto check_hwcfg_error;
> > +}
> > +}
> > +
> > +check_hwcfg_error:
> > +v4l2_fwnode_endpoint_free(&bus_cfg);
> > +
> > +return ret;
> > +}
> 
> ...
> 
> > +static int ov02a10_probe(struct i2c_client *client) {
> 
> > +/* Optional indication of physical rotation of sensor */
> > +ret = fwnode_property_read_u32(dev_fwnode(dev), "rotation",
> > +&rotation);
> 
> > +if (!ret) {
> 
> Why not positive conditional?
> 

Okay. Fixed in next release.

> > +ov02a10->upside_down = rotation == 180;
> > +if (rotation == 180) {
> > +ov02a10->upside_down = true;
> > +ov02a10->fmt.code = MEDIA_BUS_FMT_SRGGB10_1X10;
> > +}
> > +} else {
> > +dev_warn(dev, "failed to get rotation\n");
> > +}
> > +
> > +/* Optional indication of mipi TX speed */
> > +ret = fwnode_property_read_u32(dev_fwnode(dev), "ovti,mipi-tx-speed",
> > +       &clock_lane_tx_speed);
> > +
> 
> > +if (!ret)
> 
> Ditto.
> 

As Sakari mentioned earlier, the property "ovti,mipi-tx-speed" is
optional that shouldn't warn it's missing when ret is 0.
So we would keep the condition like that, just removing else case.

> > +ov02a10->mipi_clock_tx_speed = clock_lane_tx_speed;
> > +else
> > +dev_warn(dev, "failed to get mipi tx speed, using default...\n");
> > +
> 
> > +return ret;
> > +}
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
> 
> *********************MEDIATEK Confidential/Internal Use*********************

Re: [V6, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver

[Sakari Ailus]

Hi Dongchun,

On Wed, Apr 08, 2020 at 07:53:44PM +0800, Dongchun Zhu wrote:
> Hello Andy,
> 
> Thanks for the review. Sorry for the late reply.
> 
> On Mon, 2019-12-11 at 16:36 +0200, Andy Shevchenko wrote:
> > On Wed, Dec 11, 2019 at 07:28:49PM +0800, Dongchun Zhu wrote:
> > > Add a V4L2 sub-device driver for OV02A10 image sensor. The OV02A10 is
> > > a 1/5" CMOS sensor from Omnivision, asupporting output format: 10-bit Raw.
> > >
> > > This chip has a single MIPI lane interface and use the I2C bus for
> > > control and the CSI-2 bus for data.
> > 
> > ...
> > 
> > > +#define OV02A10_MASK_8_BITS                            0xff
> > 
> > Besides GENMASK() why do you need a definition here? What's the point?
> > 
> 
> Fixed in next release.
> 
> > ...
> > 
> > > +static int ov02a10_entity_init_cfg(struct v4l2_subdev *sd,
> > > +   struct v4l2_subdev_pad_config *cfg) {
> > > +struct v4l2_subdev_format fmt = {
> > > +.which = cfg ? V4L2_SUBDEV_FORMAT_TRY
> > > +     : V4L2_SUBDEV_FORMAT_ACTIVE,
> > > +.format = {
> > > +.width = 1600,
> > 
> > > +.height = 1200
> > 
> > Leave comma here.
> > 
> 
> Fixed in next release.
> 
> > > +}
> > > +};
> > > +
> > > +ov02a10_set_fmt(sd, cfg, &fmt);
> > > +
> > > +return 0;
> > > +}
> > 
> > ...
> > 
> > > +ret = i2c_smbus_write_byte_data(client, OV02A10_REG_GAIN,
> > > +(val & OV02A10_MASK_8_BITS));
> > 
> > Too many parentheses.
> > 
> 
> Fixed in next release.
> 
> > > +if (ret < 0)
> > > +return ret;
> > 
> > ...
> > 
> > > +static int ov02a10_set_vblank(struct ov02a10 *ov02a10, int val) {
> > > +struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > 
> > if you do
> > 
> > int vts = val + ov02a10->cur_mode->height - OV02A10_BASIC_LINE;
> > 
> > you may increase readability below...
> > 
> 
> Thanks for the suggestion.
> It seems better now.
> 
> > > +int ret;
> > > +
> > > +ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE);
> > > +if (ret < 0)
> > > +return ret;
> > > +
> > > +ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_H,
> > > +(((val + ov02a10->cur_mode->height -
> > > +OV02A10_BASIC_LINE) >>
> > > +OV02A10_VTS_SHIFT) &
> > > +OV02A10_MASK_8_BITS));
> > 
> > ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_H,
> > (vts >> OV02A10_VTS_SHIFT) &
> > OV02A10_MASK_8_BITS));
> > 
> > And actually why do you need this mask here? Isn't enough to call
> > 
> > ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_H,
> > vts >> OV02A10_VTS_SHIFT);
> > 
> > here...
> > 
> > 
> 
> Yes. Now we code like this.
> 
> > > +if (ret < 0)
> > > +return ret;
> > > +
> > > +ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_L,
> > > +((val + ov02a10->cur_mode->height -
> > > +OV02A10_BASIC_LINE) &
> > > +OV02A10_MASK_8_BITS));
> > 
> > ...and
> > 
> > ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_L, vts);
> > 
> > here?
> > 
> 
> Yes. Fixed in next release.
> 
> > > +if (ret < 0)
> > > +return ret;
> > > +
> > > +return i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
> > > + REG_ENABLE);
> > > +}
> > 
> > ...
> > 
> > > +static int ov02a10_check_hwcfg(struct device *dev, struct ov02a10
> > > +*ov02a10) {
> > > +struct fwnode_handle *ep;
> > > +struct fwnode_handle *fwnode = dev_fwnode(dev);
> > > +struct v4l2_fwnode_endpoint bus_cfg = {
> > 
> > > +.bus_type = V4L2_MBUS_CSI2_DPHY
> > 
> > Leave comma here.
> > 
> 
> Fixed in next release.
> 
> > > +};
> > > +unsigned int i, j;
> > > +int ret;
> > 
> > > +if (!fwnode)
> > > +return -ENXIO;
> > 
> > A bit strange error code here.
> > 
> 
> This should be reported as -EINVAL.
> Fixed in next release.
> 
> > > +
> > > +ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
> > > +if (!ep)
> > > +return -ENXIO;
> > > +
> > > +ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> > > +fwnode_handle_put(ep);
> > > +if (ret)
> > > +return ret;
> > 
> > > +if (!bus_cfg.nr_of_link_frequencies) {
> > > +dev_err(dev, "no link frequencies defined");
> > > +ret = -EINVAL;
> > > +goto check_hwcfg_error;
> > > +}
> > 
> > I still think it's redundant check, though it's up to maintainers.
> > 
> 
> We still wanna keep this check.
> Keep same as ov2659 and ov8856.
> 
> > > +
> > > +for (i = 0; i < ARRAY_SIZE(link_freq_menu_items); i++) {
> > > +for (j = 0; j < bus_cfg.nr_of_link_frequencies; j++) {
> > > +if (link_freq_menu_items[i] ==
> > > +bus_cfg.link_frequencies[j])
> > > +break;
> > > +}
> > > +
> > > +if (j == bus_cfg.nr_of_link_frequencies) {
> > > +dev_err(dev, "no link frequency %lld supported",
> > > +link_freq_menu_items[i]);
> > > +ret = -EINVAL;
> > > +goto check_hwcfg_error;
> > > +}
> > > +}
> > > +
> > > +check_hwcfg_error:
> > > +v4l2_fwnode_endpoint_free(&bus_cfg);
> > > +
> > > +return ret;
> > > +}
> > 
> > ...
> > 
> > > +static int ov02a10_probe(struct i2c_client *client) {
> > 
> > > +/* Optional indication of physical rotation of sensor */
> > > +ret = fwnode_property_read_u32(dev_fwnode(dev), "rotation",
> > > +&rotation);
> > 
> > > +if (!ret) {
> > 
> > Why not positive conditional?
> > 
> 
> Okay. Fixed in next release.
> 
> > > +ov02a10->upside_down = rotation == 180;
> > > +if (rotation == 180) {
> > > +ov02a10->upside_down = true;
> > > +ov02a10->fmt.code = MEDIA_BUS_FMT_SRGGB10_1X10;
> > > +}
> > > +} else {
> > > +dev_warn(dev, "failed to get rotation\n");
> > > +}
> > > +
> > > +/* Optional indication of mipi TX speed */
> > > +ret = fwnode_property_read_u32(dev_fwnode(dev), "ovti,mipi-tx-speed",
> > > +       &clock_lane_tx_speed);
> > > +
> > 
> > > +if (!ret)
> > 
> > Ditto.
> > 
> 
> As Sakari mentioned earlier, the property "ovti,mipi-tx-speed" is
> optional that shouldn't warn it's missing when ret is 0.
> So we would keep the condition like that, just removing else case.

I don't remember discussing this, but could be because it was quite some
time ago.

It doesn't seem to be documented. What is it for?

> > *********************MEDIATEK Confidential/Internal Use*********************

Is this intentional?

-- 
Regards,

Sakari Ailus

Re: [V6, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver

[Dongchun Zhu]

Hi Sakari,

On Wed, 2020-04-08 at 15:20 +0300, Sakari Ailus wrote:
> Hi Dongchun,
> 
> On Wed, Apr 08, 2020 at 07:53:44PM +0800, Dongchun Zhu wrote:
> > Hello Andy,
> > 
> > Thanks for the review. Sorry for the late reply.
> > 
> > On Mon, 2019-12-11 at 16:36 +0200, Andy Shevchenko wrote:
> > > On Wed, Dec 11, 2019 at 07:28:49PM +0800, Dongchun Zhu wrote:
> > > > Add a V4L2 sub-device driver for OV02A10 image sensor. The OV02A10 is
> > > > a 1/5" CMOS sensor from Omnivision, asupporting output format: 10-bit Raw.
> > > >
> > > > This chip has a single MIPI lane interface and use the I2C bus for
> > > > control and the CSI-2 bus for data.
> > > 
> > > ...
> > > 
> > > > +#define OV02A10_MASK_8_BITS                            0xff
> > > 
> > > Besides GENMASK() why do you need a definition here? What's the point?
> > > 
> > 
> > Fixed in next release.
> > 
> > > ...
> > > 
> > > > +static int ov02a10_entity_init_cfg(struct v4l2_subdev *sd,
> > > > +   struct v4l2_subdev_pad_config *cfg) {
> > > > +struct v4l2_subdev_format fmt = {
> > > > +.which = cfg ? V4L2_SUBDEV_FORMAT_TRY
> > > > +     : V4L2_SUBDEV_FORMAT_ACTIVE,
> > > > +.format = {
> > > > +.width = 1600,
> > > 
> > > > +.height = 1200
> > > 
> > > Leave comma here.
> > > 
> > 
> > Fixed in next release.
> > 
> > > > +}
> > > > +};
> > > > +
> > > > +ov02a10_set_fmt(sd, cfg, &fmt);
> > > > +
> > > > +return 0;
> > > > +}
> > > 
> > > ...
> > > 
> > > > +ret = i2c_smbus_write_byte_data(client, OV02A10_REG_GAIN,
> > > > +(val & OV02A10_MASK_8_BITS));
> > > 
> > > Too many parentheses.
> > > 
> > 
> > Fixed in next release.
> > 
> > > > +if (ret < 0)
> > > > +return ret;
> > > 
> > > ...
> > > 
> > > > +static int ov02a10_set_vblank(struct ov02a10 *ov02a10, int val) {
> > > > +struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > > 
> > > if you do
> > > 
> > > int vts = val + ov02a10->cur_mode->height - OV02A10_BASIC_LINE;
> > > 
> > > you may increase readability below...
> > > 
> > 
> > Thanks for the suggestion.
> > It seems better now.
> > 
> > > > +int ret;
> > > > +
> > > > +ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE);
> > > > +if (ret < 0)
> > > > +return ret;
> > > > +
> > > > +ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_H,
> > > > +(((val + ov02a10->cur_mode->height -
> > > > +OV02A10_BASIC_LINE) >>
> > > > +OV02A10_VTS_SHIFT) &
> > > > +OV02A10_MASK_8_BITS));
> > > 
> > > ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_H,
> > > (vts >> OV02A10_VTS_SHIFT) &
> > > OV02A10_MASK_8_BITS));
> > > 
> > > And actually why do you need this mask here? Isn't enough to call
> > > 
> > > ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_H,
> > > vts >> OV02A10_VTS_SHIFT);
> > > 
> > > here...
> > > 
> > > 
> > 
> > Yes. Now we code like this.
> > 
> > > > +if (ret < 0)
> > > > +return ret;
> > > > +
> > > > +ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_L,
> > > > +((val + ov02a10->cur_mode->height -
> > > > +OV02A10_BASIC_LINE) &
> > > > +OV02A10_MASK_8_BITS));
> > > 
> > > ...and
> > > 
> > > ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_L, vts);
> > > 
> > > here?
> > > 
> > 
> > Yes. Fixed in next release.
> > 
> > > > +if (ret < 0)
> > > > +return ret;
> > > > +
> > > > +return i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
> > > > + REG_ENABLE);
> > > > +}
> > > 
> > > ...
> > > 
> > > > +static int ov02a10_check_hwcfg(struct device *dev, struct ov02a10
> > > > +*ov02a10) {
> > > > +struct fwnode_handle *ep;
> > > > +struct fwnode_handle *fwnode = dev_fwnode(dev);
> > > > +struct v4l2_fwnode_endpoint bus_cfg = {
> > > 
> > > > +.bus_type = V4L2_MBUS_CSI2_DPHY
> > > 
> > > Leave comma here.
> > > 
> > 
> > Fixed in next release.
> > 
> > > > +};
> > > > +unsigned int i, j;
> > > > +int ret;
> > > 
> > > > +if (!fwnode)
> > > > +return -ENXIO;
> > > 
> > > A bit strange error code here.
> > > 
> > 
> > This should be reported as -EINVAL.
> > Fixed in next release.
> > 
> > > > +
> > > > +ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
> > > > +if (!ep)
> > > > +return -ENXIO;
> > > > +
> > > > +ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> > > > +fwnode_handle_put(ep);
> > > > +if (ret)
> > > > +return ret;
> > > 
> > > > +if (!bus_cfg.nr_of_link_frequencies) {
> > > > +dev_err(dev, "no link frequencies defined");
> > > > +ret = -EINVAL;
> > > > +goto check_hwcfg_error;
> > > > +}
> > > 
> > > I still think it's redundant check, though it's up to maintainers.
> > > 
> > 
> > We still wanna keep this check.
> > Keep same as ov2659 and ov8856.
> > 
> > > > +
> > > > +for (i = 0; i < ARRAY_SIZE(link_freq_menu_items); i++) {
> > > > +for (j = 0; j < bus_cfg.nr_of_link_frequencies; j++) {
> > > > +if (link_freq_menu_items[i] ==
> > > > +bus_cfg.link_frequencies[j])
> > > > +break;
> > > > +}
> > > > +
> > > > +if (j == bus_cfg.nr_of_link_frequencies) {
> > > > +dev_err(dev, "no link frequency %lld supported",
> > > > +link_freq_menu_items[i]);
> > > > +ret = -EINVAL;
> > > > +goto check_hwcfg_error;
> > > > +}
> > > > +}
> > > > +
> > > > +check_hwcfg_error:
> > > > +v4l2_fwnode_endpoint_free(&bus_cfg);
> > > > +
> > > > +return ret;
> > > > +}
> > > 
> > > ...
> > > 
> > > > +static int ov02a10_probe(struct i2c_client *client) {
> > > 
> > > > +/* Optional indication of physical rotation of sensor */
> > > > +ret = fwnode_property_read_u32(dev_fwnode(dev), "rotation",
> > > > +&rotation);
> > > 
> > > > +if (!ret) {
> > > 
> > > Why not positive conditional?
> > > 
> > 
> > Okay. Fixed in next release.
> > 
> > > > +ov02a10->upside_down = rotation == 180;
> > > > +if (rotation == 180) {
> > > > +ov02a10->upside_down = true;
> > > > +ov02a10->fmt.code = MEDIA_BUS_FMT_SRGGB10_1X10;
> > > > +}
> > > > +} else {
> > > > +dev_warn(dev, "failed to get rotation\n");
> > > > +}
> > > > +
> > > > +/* Optional indication of mipi TX speed */
> > > > +ret = fwnode_property_read_u32(dev_fwnode(dev), "ovti,mipi-tx-speed",
> > > > +       &clock_lane_tx_speed);
> > > > +
> > > 
> > > > +if (!ret)
> > > 
> > > Ditto.
> > > 
> > 
> > As Sakari mentioned earlier, the property "ovti,mipi-tx-speed" is
> > optional that shouldn't warn it's missing when ret is 0.
> > So we would keep the condition like that, just removing else case.
> 
> I don't remember discussing this, but could be because it was quite some
> time ago.
> 
> It doesn't seem to be documented. What is it for?
> 

Sorry for not addressing this point on v6.
"ovti,mipi-tx-speed" is one private property in DT, which is abstracted
to handle different register settings for different Chrome projects.

More details please see the Google Issue:
https://partnerissuetracker.corp.google.com/issues/143749215

And it would be documented in next release.

> > > *********************MEDIATEK Confidential/Internal Use*********************
> 
> Is this intentional?
> 

Re: [V6, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver

[Sakari Ailus]

Hi Dongchun,

On Wed, Dec 11, 2019 at 07:28:49PM +0800, Dongchun Zhu wrote:
> Add a V4L2 sub-device driver for OV02A10 image sensor. The OV02A10 is a
> 1/5" CMOS sensor from Omnivision, asupporting output format: 10-bit Raw.
> 
> This chip has a single MIPI lane interface and use the I2C bus for
> control and the CSI-2 bus for data.
> 
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> ---
>  MAINTAINERS                 |    1 +
>  drivers/media/i2c/Kconfig   |   11 +
>  drivers/media/i2c/Makefile  |    1 +
>  drivers/media/i2c/ov02a10.c | 1102 +++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 1115 insertions(+)
>  create mode 100644 drivers/media/i2c/ov02a10.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 92a868c..e294530 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12135,6 +12135,7 @@ M:	Dongchun Zhu <dongchun.zhu@mediatek.com>
>  L:	linux-media@vger.kernel.org
>  T:	git git://linuxtv.org/media_tree.git
>  S:	Maintained
> +F:	drivers/media/i2c/ov02a10.c
>  F:	Documentation/devicetree/bindings/media/i2c/ov02a10.txt
>  
>  OMNIVISION OV2680 SENSOR DRIVER
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index c68e002..d3e8c41 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -640,6 +640,17 @@ config VIDEO_IMX355
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called imx355.
>  
> +config VIDEO_OV02A10
> +	tristate "OmniVision OV02A10 sensor support"
> +	depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> +	select V4L2_FWNODE
> +	help
> +	  This is a Video4Linux2 sensor driver for the OmniVision
> +	  OV02A10 camera.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ov02a10.
> +
>  config VIDEO_OV2640
>  	tristate "OmniVision OV2640 sensor support"
>  	depends on VIDEO_V4L2 && I2C
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index c147bb9..b3769d7 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -63,6 +63,7 @@ obj-$(CONFIG_VIDEO_VP27SMPX) += vp27smpx.o
>  obj-$(CONFIG_VIDEO_SONY_BTF_MPX) += sony-btf-mpx.o
>  obj-$(CONFIG_VIDEO_UPD64031A) += upd64031a.o
>  obj-$(CONFIG_VIDEO_UPD64083) += upd64083.o
> +obj-$(CONFIG_VIDEO_OV02A10) += ov02a10.o
>  obj-$(CONFIG_VIDEO_OV2640) += ov2640.o
>  obj-$(CONFIG_VIDEO_OV2680) += ov2680.o
>  obj-$(CONFIG_VIDEO_OV2685) += ov2685.o
> diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c
> new file mode 100644
> index 0000000..0b930f6
> --- /dev/null
> +++ b/drivers/media/i2c/ov02a10.c
> @@ -0,0 +1,1102 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2019 MediaTek Inc.
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> +#include <media/media-entity.h>
> +#include <media/v4l2-async.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-subdev.h>
> +#include <media/v4l2-fwnode.h>
> +
> +#define CHIP_ID						0x2509
> +#define OV02A10_REG_CHIP_ID_H				0x02
> +#define OV02A10_REG_CHIP_ID_L				0x03
> +#define OV02A10_ID(_msb, _lsb)				((_msb) << 8 | (_lsb))
> +
> +/* Bit[1] vertical upside down */
> +/* Bit[0] horizontal mirror */
> +#define REG_MIRROR_FLIP_CONTROL				0x3f
> +
> +/* Orientation */
> +#define REG_MIRROR_FLIP_ENABLE				0x03
> +
> +/* Bit[7] clock HS mode enable
> + * 0: Clock continue
> + * 1: Clock HS
> + * Bit[6:2] HS VOD adjust
> + * Bit[1:0] P VHI adjust
> + */
> +#define REG_HS_MODE_BLC					0x9d
> +
> +#define CLOCK_HS_MODE_ENABLE				BIT(7)
> +
> +/* Bit[2:0] MIPI transmission speed select */
> +#define TX_SPEED_AREA_SEL				0xa1
> +
> +#define REG_PAGE_SWITCH					0xfd
> +#define REG_GLOBAL_EFFECTIVE				0x01
> +#define REG_ENABLE					BIT(0)
> +#define OV02A10_MASK_8_BITS				0xff
> +
> +#define REG_SC_CTRL_MODE				0xac
> +#define SC_CTRL_MODE_STANDBY				0x00
> +#define SC_CTRL_MODE_STREAMING				0x01
> +
> +#define OV02A10_EXP_SHIFT				8
> +#define OV02A10_REG_EXPOSURE_H				0x03
> +#define OV02A10_REG_EXPOSURE_L				0x04
> +#define	OV02A10_EXPOSURE_MIN				4
> +#define OV02A10_EXPOSURE_MAX_MARGIN			4
> +#define	OV02A10_EXPOSURE_STEP				1
> +
> +#define OV02A10_VTS_SHIFT				8
> +#define OV02A10_REG_VTS_H				0x05
> +#define OV02A10_REG_VTS_L				0x06
> +#define OV02A10_VTS_MAX					0x209f
> +#define OV02A10_VTS_MIN					0x04cf
> +#define OV02A10_BASIC_LINE				1224
> +
> +#define OV02A10_REG_GAIN				0x24
> +#define OV02A10_GAIN_MIN				0x10
> +#define OV02A10_GAIN_MAX				0xf8
> +#define OV02A10_GAIN_STEP				0x01
> +#define OV02A10_GAIN_DEFAULT				0x40
> +
> +/* Test pattern control */
> +#define OV02A10_REG_TEST_PATTERN			0xb6
> +#define OV02A10_TEST_PATTERN_ENABLE			BIT(0)
> +
> +#define HZ_PER_MHZ					1000000L
> +#define OV02A10_LINK_FREQ_390MHZ			(390 * HZ_PER_MHZ)
> +#define OV02A10_ECLK_FREQ				(24 * HZ_PER_MHZ)
> +#define OV02A10_DATA_LANES				1
> +#define OV02A10_BITS_PER_SAMPLE				10
> +
> +static const char * const ov02a10_supply_names[] = {
> +	"dovdd",	/* Digital I/O power */
> +	"avdd",		/* Analog power */
> +	"dvdd",		/* Digital core power */
> +};
> +
> +#define OV02A10_NUM_SUPPLIES ARRAY_SIZE(ov02a10_supply_names)
> +
> +struct ov02a10_reg {
> +	u8 addr;
> +	u8 val;
> +};
> +
> +struct ov02a10_reg_list {
> +	u32 num_of_regs;
> +	const struct ov02a10_reg *regs;
> +};
> +
> +struct ov02a10_mode {
> +	u32 width;
> +	u32 height;
> +	u32 exp_def;
> +	u32 hts_def;
> +	u32 vts_def;
> +	const struct ov02a10_reg_list reg_list;
> +};
> +
> +struct ov02a10 {
> +	u32			eclk_freq;
> +	u32                     mipi_clock_tx_speed;
> +
> +	struct clk		*eclk;
> +	struct gpio_desc	*pd_gpio;
> +	struct gpio_desc	*n_rst_gpio;
> +	struct regulator_bulk_data supplies[OV02A10_NUM_SUPPLIES];
> +
> +	bool			streaming;
> +	bool			upside_down;
> +	bool			mipi_clock_hs_mode_enable;
> +
> +	/*
> +	 * Serialize control access, get/set format, get selection
> +	 * and start streaming.
> +	 */
> +	struct mutex		mutex;
> +	struct v4l2_subdev	subdev;
> +	struct media_pad	pad;
> +	struct v4l2_ctrl	*anal_gain;
> +	struct v4l2_ctrl	*exposure;
> +	struct v4l2_ctrl	*hblank;
> +	struct v4l2_ctrl	*vblank;
> +	struct v4l2_ctrl	*test_pattern;
> +	struct v4l2_mbus_framefmt	fmt;
> +	struct v4l2_ctrl_handler ctrl_handler;
> +
> +	const struct ov02a10_mode *cur_mode;
> +};
> +
> +static inline struct ov02a10 *to_ov02a10(struct v4l2_subdev *sd)
> +{
> +	return container_of(sd, struct ov02a10, subdev);
> +}
> +
> +/*
> + * eclk 24Mhz
> + * pclk 39Mhz
> + * linelength 934(0x3a6)
> + * framelength 1390(0x56E)
> + * grabwindow_width 1600
> + * grabwindow_height 1200
> + * max_framerate 30fps
> + * mipi_datarate per lane 780Mbps
> + */
> +static const struct ov02a10_reg ov02a10_1600x1200_regs[] = {
> +	{0xfd, 0x01},
> +	{0xac, 0x00},
> +	{0xfd, 0x00},
> +	{0x2f, 0x29},
> +	{0x34, 0x00},
> +	{0x35, 0x21},
> +	{0x30, 0x15},
> +	{0x33, 0x01},
> +	{0xfd, 0x01},
> +	{0x44, 0x00},
> +	{0x2a, 0x4c},
> +	{0x2b, 0x1e},
> +	{0x2c, 0x60},
> +	{0x25, 0x11},
> +	{0x03, 0x01},
> +	{0x04, 0xae},
> +	{0x09, 0x00},
> +	{0x0a, 0x02},
> +	{0x06, 0xa6},
> +	{0x31, 0x00},
> +	{0x24, 0x40},
> +	{0x01, 0x01},
> +	{0xfb, 0x73},
> +	{0xfd, 0x01},
> +	{0x16, 0x04},
> +	{0x1c, 0x09},
> +	{0x21, 0x42},
> +	{0x12, 0x04},
> +	{0x13, 0x10},
> +	{0x11, 0x40},
> +	{0x33, 0x81},
> +	{0xd0, 0x00},
> +	{0xd1, 0x01},
> +	{0xd2, 0x00},
> +	{0x50, 0x10},
> +	{0x51, 0x23},
> +	{0x52, 0x20},
> +	{0x53, 0x10},
> +	{0x54, 0x02},
> +	{0x55, 0x20},
> +	{0x56, 0x02},
> +	{0x58, 0x48},
> +	{0x5d, 0x15},
> +	{0x5e, 0x05},
> +	{0x66, 0x66},
> +	{0x68, 0x68},
> +	{0x6b, 0x00},
> +	{0x6c, 0x00},
> +	{0x6f, 0x40},
> +	{0x70, 0x40},
> +	{0x71, 0x0a},
> +	{0x72, 0xf0},
> +	{0x73, 0x10},
> +	{0x75, 0x80},
> +	{0x76, 0x10},
> +	{0x84, 0x00},
> +	{0x85, 0x10},
> +	{0x86, 0x10},
> +	{0x87, 0x00},
> +	{0x8a, 0x22},
> +	{0x8b, 0x22},
> +	{0x19, 0xf1},
> +	{0x29, 0x01},
> +	{0xfd, 0x01},
> +	{0x9d, 0x16},
> +	{0xa0, 0x29},
> +	{0xa1, 0x03},
> +	{0xad, 0x62},
> +	{0xae, 0x00},
> +	{0xaf, 0x85},
> +	{0xb1, 0x01},
> +	{0x8e, 0x06},
> +	{0x8f, 0x40},
> +	{0x90, 0x04},
> +	{0x91, 0xb0},
> +	{0x45, 0x01},
> +	{0x46, 0x00},
> +	{0x47, 0x6c},
> +	{0x48, 0x03},
> +	{0x49, 0x8b},
> +	{0x4a, 0x00},
> +	{0x4b, 0x07},
> +	{0x4c, 0x04},
> +	{0x4d, 0xb7},
> +	{0xf0, 0x40},
> +	{0xf1, 0x40},
> +	{0xf2, 0x40},
> +	{0xf3, 0x40},
> +	{0x3f, 0x00},
> +	{0xfd, 0x01},
> +	{0x05, 0x00},
> +	{0x06, 0xa6},
> +	{0xfd, 0x01},
> +};
> +
> +static const char * const ov02a10_test_pattern_menu[] = {
> +	"Disabled",
> +	"Color Bar",
> +};
> +
> +static const s64 link_freq_menu_items[] = {
> +	OV02A10_LINK_FREQ_390MHZ,
> +};
> +
> +static u64 to_pixel_rate(u32 f_index)
> +{
> +	u64 pixel_rate = link_freq_menu_items[f_index] * 2 * OV02A10_DATA_LANES;
> +
> +	do_div(pixel_rate, OV02A10_BITS_PER_SAMPLE);
> +
> +	return pixel_rate;
> +}
> +
> +static const struct ov02a10_mode supported_modes[] = {
> +	{
> +		.width = 1600,
> +		.height = 1200,
> +		.exp_def = 0x01ae,
> +		.hts_def = 0x03a6,
> +		.vts_def = 0x056e,
> +		.reg_list = {
> +			.num_of_regs = ARRAY_SIZE(ov02a10_1600x1200_regs),
> +			.regs = ov02a10_1600x1200_regs,
> +		},
> +	},
> +};
> +
> +static int ov02a10_write_array(struct ov02a10 *ov02a10,
> +			       const struct ov02a10_reg_list *r_list)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> +	unsigned int i;
> +	int ret;
> +
> +	for (i = 0; i < r_list->num_of_regs; i++) {
> +		ret = i2c_smbus_write_byte_data(client, r_list->regs[i].addr,
> +						r_list->regs[i].val);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ov02a10_read_smbus(struct ov02a10 *ov02a10, unsigned char reg,
> +			      unsigned char *val)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(client, reg);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = (unsigned char)ret;
> +
> +	return 0;
> +}
> +
> +static int ov02a10_mod_reg(struct ov02a10 *ov02a10, u8 reg, u8 mask, u8 val)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> +	u8 readval;
> +	int ret;
> +
> +	ret = ov02a10_read_smbus(ov02a10, reg, &readval);
> +	if (ret)
> +		return ret;
> +
> +	val = (readval & ~mask) | (val & mask);
> +
> +	return i2c_smbus_write_byte_data(client, reg, val);
> +}
> +
> +static void ov02a10_fill_fmt(const struct ov02a10_mode *mode,
> +			     struct v4l2_mbus_framefmt *fmt)
> +{
> +	fmt->width = mode->width;
> +	fmt->height = mode->height;
> +	fmt->field = V4L2_FIELD_NONE;
> +}
> +
> +static int ov02a10_set_fmt(struct v4l2_subdev *sd,
> +			   struct v4l2_subdev_pad_config *cfg,
> +			   struct v4l2_subdev_format *fmt)
> +{
> +	struct ov02a10 *ov02a10 = to_ov02a10(sd);
> +	struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
> +
> +	mutex_lock(&ov02a10->mutex);
> +
> +	if (ov02a10->streaming) {
> +		mutex_unlock(&ov02a10->mutex);
> +		return -EBUSY;
> +	}
> +
> +	/* Only one sensor mode supported */
> +	mbus_fmt->code = ov02a10->fmt.code;
> +	ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt);
> +	ov02a10->fmt = fmt->format;
> +
> +	mutex_unlock(&ov02a10->mutex);
> +
> +	return 0;
> +}
> +
> +static int ov02a10_get_fmt(struct v4l2_subdev *sd,
> +			   struct v4l2_subdev_pad_config *cfg,
> +			   struct v4l2_subdev_format *fmt)
> +{
> +	struct ov02a10 *ov02a10 = to_ov02a10(sd);
> +	struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
> +
> +	mutex_lock(&ov02a10->mutex);
> +
> +	fmt->format = ov02a10->fmt;
> +	mbus_fmt->code = ov02a10->fmt.code;
> +	ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt);
> +
> +	mutex_unlock(&ov02a10->mutex);
> +
> +	return 0;
> +}
> +
> +static int ov02a10_enum_mbus_code(struct v4l2_subdev *sd,
> +				  struct v4l2_subdev_pad_config *cfg,
> +				  struct v4l2_subdev_mbus_code_enum *code)
> +{
> +	struct ov02a10 *ov02a10 = to_ov02a10(sd);
> +
> +	if (code->index >= ARRAY_SIZE(supported_modes))
> +		return -EINVAL;
> +
> +	code->code = ov02a10->fmt.code;

Do you plan to have a different format as well as a different resolution
for each mode?

> +
> +	return 0;
> +}
> +
> +static int ov02a10_enum_frame_sizes(struct v4l2_subdev *sd,
> +				    struct v4l2_subdev_pad_config *cfg,
> +				    struct v4l2_subdev_frame_size_enum *fse)
> +{
> +	if (fse->index >= ARRAY_SIZE(supported_modes))
> +		return -EINVAL;
> +
> +	fse->min_width  = supported_modes[fse->index].width;
> +	fse->max_width  = supported_modes[fse->index].width;
> +	fse->max_height = supported_modes[fse->index].height;
> +	fse->min_height = supported_modes[fse->index].height;
> +
> +	return 0;
> +}
> +
> +static int ov02a10_check_sensor_id(struct ov02a10 *ov02a10)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> +	u16 id;
> +	u8 chip_id_h;
> +	u8 chip_id_l;
> +	int ret;
> +
> +	/* Check sensor revision */
> +	ret = ov02a10_read_smbus(ov02a10, OV02A10_REG_CHIP_ID_H, &chip_id_h);
> +	if (ret)
> +		return ret;
> +
> +	ret = ov02a10_read_smbus(ov02a10, OV02A10_REG_CHIP_ID_L, &chip_id_l);
> +	if (ret)
> +		return ret;
> +
> +	id = OV02A10_ID(chip_id_h, chip_id_l);
> +	if (id != CHIP_ID) {
> +		dev_err(&client->dev, "Unexpected sensor id(%04x)\n", id);
> +		return -EINVAL;
> +	}
> +
> +	dev_dbg(&client->dev, "Detected OV%04X sensor\n", id);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused ov02a10_power_on(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct ov02a10 *ov02a10 = to_ov02a10(sd);
> +	int ret;
> +
> +	gpiod_set_value_cansleep(ov02a10->n_rst_gpio, 0);
> +	gpiod_set_value_cansleep(ov02a10->pd_gpio, 1);
> +
> +	ret = clk_prepare_enable(ov02a10->eclk);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to enable eclk\n");
> +		return ret;
> +	}
> +
> +	ret = regulator_bulk_enable(OV02A10_NUM_SUPPLIES, ov02a10->supplies);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to enable regulators\n");
> +		goto disable_clk;
> +	}
> +	usleep_range(5000, 6000);
> +
> +	gpiod_set_value_cansleep(ov02a10->pd_gpio, 0);
> +	usleep_range(5000, 6000);
> +
> +	gpiod_set_value_cansleep(ov02a10->n_rst_gpio, 1);
> +	usleep_range(5000, 6000);
> +
> +	ret = ov02a10_check_sensor_id(ov02a10);
> +	if (ret)
> +		goto disable_regulator;
> +
> +	return 0;
> +
> +disable_regulator:
> +	regulator_bulk_disable(OV02A10_NUM_SUPPLIES, ov02a10->supplies);
> +disable_clk:
> +	clk_disable_unprepare(ov02a10->eclk);
> +
> +	return ret;
> +}
> +
> +static int __maybe_unused ov02a10_power_off(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct ov02a10 *ov02a10 = to_ov02a10(sd);
> +
> +	gpiod_set_value_cansleep(ov02a10->n_rst_gpio, 0);
> +	clk_disable_unprepare(ov02a10->eclk);
> +	gpiod_set_value_cansleep(ov02a10->pd_gpio, 1);
> +	regulator_bulk_disable(OV02A10_NUM_SUPPLIES, ov02a10->supplies);
> +
> +	return 0;
> +}
> +
> +static int __ov02a10_start_stream(struct ov02a10 *ov02a10)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> +	const struct ov02a10_reg_list *reg_list;
> +	int ret;
> +
> +	/* Apply default values of current mode */
> +	reg_list = &ov02a10->cur_mode->reg_list;
> +	ret = ov02a10_write_array(ov02a10, reg_list);
> +	if (ret)
> +		return ret;
> +
> +	/* Apply customized values from user */
> +	ret = __v4l2_ctrl_handler_setup(ov02a10->subdev.ctrl_handler);
> +	if (ret)
> +		return ret;
> +
> +	/* Set orientation to 180 degree */
> +	if (ov02a10->upside_down) {
> +		ret = i2c_smbus_write_byte_data(client, REG_MIRROR_FLIP_CONTROL,
> +						REG_MIRROR_FLIP_ENABLE);
> +		if (ret) {
> +			dev_err(&client->dev, "failed to set orientation\n");
> +			return ret;
> +		}
> +		ret = i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
> +						REG_ENABLE);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	/* Set clock lane transmission mode according to DT property */
> +	ret = ov02a10_mod_reg(ov02a10, REG_HS_MODE_BLC, CLOCK_HS_MODE_ENABLE,
> +			      ov02a10->mipi_clock_hs_mode_enable ?
> +			      CLOCK_HS_MODE_ENABLE : 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Set mipi TX speed according to DT property */
> +	ret = i2c_smbus_write_byte_data(client, TX_SPEED_AREA_SEL,
> +					ov02a10->mipi_clock_tx_speed);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Set stream on register */
> +	return i2c_smbus_write_byte_data(client, REG_SC_CTRL_MODE,
> +					 SC_CTRL_MODE_STREAMING);
> +}
> +
> +static int __ov02a10_stop_stream(struct ov02a10 *ov02a10)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> +
> +	return i2c_smbus_write_byte_data(client, REG_SC_CTRL_MODE,
> +					 SC_CTRL_MODE_STANDBY);
> +}
> +
> +static int ov02a10_entity_init_cfg(struct v4l2_subdev *sd,
> +				   struct v4l2_subdev_pad_config *cfg)
> +{
> +	struct v4l2_subdev_format fmt = {
> +		.which = cfg ? V4L2_SUBDEV_FORMAT_TRY
> +			     : V4L2_SUBDEV_FORMAT_ACTIVE,
> +		.format = {
> +			.width = 1600,
> +			.height = 1200
> +		}
> +	};
> +
> +	ov02a10_set_fmt(sd, cfg, &fmt);
> +
> +	return 0;
> +}
> +
> +static int ov02a10_s_stream(struct v4l2_subdev *sd, int on)
> +{
> +	struct ov02a10 *ov02a10 = to_ov02a10(sd);
> +	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> +	int ret;
> +
> +	dev_dbg(&client->dev, "ov02a10 s_stream (%d)\n", on);
> +	mutex_lock(&ov02a10->mutex);
> +
> +	if (ov02a10->streaming == on)
> +		goto unlock_and_return;
> +
> +	if (on) {
> +		ret = pm_runtime_get_sync(&client->dev);
> +		if (ret < 0) {
> +			pm_runtime_put_noidle(&client->dev);
> +			goto unlock_and_return;
> +		}
> +
> +		ret = __ov02a10_start_stream(ov02a10);
> +		if (ret) {
> +			__ov02a10_stop_stream(ov02a10);
> +			ov02a10->streaming = !on;
> +			goto err_rpm_put;
> +		}
> +	} else {
> +		__ov02a10_stop_stream(ov02a10);
> +		pm_runtime_put(&client->dev);
> +	}
> +
> +	ov02a10->streaming = on;
> +	mutex_unlock(&ov02a10->mutex);
> +
> +	return 0;
> +
> +err_rpm_put:
> +	pm_runtime_put(&client->dev);
> +unlock_and_return:
> +	mutex_unlock(&ov02a10->mutex);
> +
> +	return ret;
> +}
> +
> +static const struct dev_pm_ops ov02a10_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +				pm_runtime_force_resume)
> +	SET_RUNTIME_PM_OPS(ov02a10_power_off, ov02a10_power_on, NULL)
> +};
> +
> +/*
> + * ov02a10_set_exposure - Function called when setting exposure time
> + * @priv: Pointer to device structure
> + * @val: Variable for exposure time, in the unit of micro-second
> + *
> + * Set exposure time based on input value.
> + *
> + * Return: 0 on success
> + */
> +static int ov02a10_set_exposure(struct ov02a10 *ov02a10, int val)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = i2c_smbus_write_byte_data(client, OV02A10_REG_EXPOSURE_H,
> +					((val >> OV02A10_EXP_SHIFT) &
> +					OV02A10_MASK_8_BITS));
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = i2c_smbus_write_byte_data(client, OV02A10_REG_EXPOSURE_L,
> +					(val & OV02A10_MASK_8_BITS));
> +	if (ret < 0)
> +		return ret;
> +
> +	return i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
> +					 REG_ENABLE);
> +}
> +
> +static int ov02a10_set_gain(struct ov02a10 *ov02a10, int val)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = i2c_smbus_write_byte_data(client, OV02A10_REG_GAIN,
> +					(val & OV02A10_MASK_8_BITS));
> +	if (ret < 0)
> +		return ret;
> +
> +	return i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
> +					 REG_ENABLE);
> +}
> +
> +static int ov02a10_set_vblank(struct ov02a10 *ov02a10, int val)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_H,
> +					(((val + ov02a10->cur_mode->height -
> +					OV02A10_BASIC_LINE) >>
> +					OV02A10_VTS_SHIFT) &
> +					OV02A10_MASK_8_BITS));
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_L,
> +					((val + ov02a10->cur_mode->height -
> +					OV02A10_BASIC_LINE) &
> +					OV02A10_MASK_8_BITS));
> +	if (ret < 0)
> +		return ret;
> +
> +	return i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
> +					 REG_ENABLE);
> +}
> +
> +static int ov02a10_set_test_pattern(struct ov02a10 *ov02a10, int pattern)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> +	int ret;
> +
> +	if (pattern)
> +		pattern = OV02A10_TEST_PATTERN_ENABLE;
> +
> +	ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = i2c_smbus_write_byte_data(client, OV02A10_REG_TEST_PATTERN,
> +					pattern);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
> +					REG_ENABLE);
> +	if (ret < 0)
> +		return ret;
> +
> +	return i2c_smbus_write_byte_data(client, REG_SC_CTRL_MODE,
> +					 SC_CTRL_MODE_STREAMING);
> +}
> +
> +static int ov02a10_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct ov02a10 *ov02a10 = container_of(ctrl->handler,
> +					       struct ov02a10, ctrl_handler);
> +	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> +	s64 max_expo;
> +	int ret;
> +
> +	/* Propagate change of current control to all related controls */
> +	if (ctrl->id == V4L2_CID_VBLANK) {
> +		/* Update max exposure while meeting expected vblanking */
> +		max_expo = ov02a10->cur_mode->height + ctrl->val -
> +			   OV02A10_EXPOSURE_MAX_MARGIN;
> +		__v4l2_ctrl_modify_range(ov02a10->exposure,
> +					 ov02a10->exposure->minimum, max_expo,
> +					 ov02a10->exposure->step,
> +					 ov02a10->exposure->default_value);
> +	}
> +
> +	/* V4L2 controls values will be applied only when power is already up */
> +	if (!pm_runtime_get_if_in_use(&client->dev))
> +		return 0;
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_EXPOSURE:
> +		ret = ov02a10_set_exposure(ov02a10, ctrl->val);
> +		break;
> +	case V4L2_CID_ANALOGUE_GAIN:
> +		ret = ov02a10_set_gain(ov02a10, ctrl->val);
> +		break;
> +	case V4L2_CID_VBLANK:
> +		ret = ov02a10_set_vblank(ov02a10, ctrl->val);
> +		break;
> +	case V4L2_CID_TEST_PATTERN:
> +		ret = ov02a10_set_test_pattern(ov02a10, ctrl->val);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	};
> +
> +	pm_runtime_put(&client->dev);
> +
> +	return ret;
> +}
> +
> +static const struct v4l2_subdev_video_ops ov02a10_video_ops = {
> +	.s_stream = ov02a10_s_stream,
> +};
> +
> +static const struct v4l2_subdev_pad_ops ov02a10_pad_ops = {
> +	.init_cfg = ov02a10_entity_init_cfg,
> +	.enum_mbus_code = ov02a10_enum_mbus_code,
> +	.enum_frame_size = ov02a10_enum_frame_sizes,
> +	.get_fmt = ov02a10_get_fmt,
> +	.set_fmt = ov02a10_set_fmt,
> +};
> +
> +static const struct v4l2_subdev_ops ov02a10_subdev_ops = {
> +	.video	= &ov02a10_video_ops,
> +	.pad	= &ov02a10_pad_ops,
> +};
> +
> +static const struct media_entity_operations ov02a10_subdev_entity_ops = {
> +	.link_validate = v4l2_subdev_link_validate,
> +};
> +
> +static const struct v4l2_ctrl_ops ov02a10_ctrl_ops = {
> +	.s_ctrl = ov02a10_set_ctrl,
> +};
> +
> +static int ov02a10_initialize_controls(struct ov02a10 *ov02a10)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> +	const struct ov02a10_mode *mode;
> +	struct v4l2_ctrl_handler *handler;
> +	struct v4l2_ctrl *ctrl;
> +	u64 exposure_max;
> +	u32 pixel_rate, h_blank;
> +	int ret;
> +
> +	handler = &ov02a10->ctrl_handler;
> +	mode = ov02a10->cur_mode;
> +	ret = v4l2_ctrl_handler_init(handler, 7);
> +	if (ret)
> +		return ret;
> +
> +	handler->lock = &ov02a10->mutex;
> +
> +	ctrl = v4l2_ctrl_new_int_menu(handler, NULL, V4L2_CID_LINK_FREQ, 0, 0,
> +				      link_freq_menu_items);
> +	if (ctrl)
> +		ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> +	pixel_rate = to_pixel_rate(0);
> +	v4l2_ctrl_new_std(handler, NULL, V4L2_CID_PIXEL_RATE, 0, pixel_rate, 1,
> +			  pixel_rate);
> +
> +	h_blank = mode->hts_def - mode->width;
> +	ov02a10->hblank = v4l2_ctrl_new_std(handler, NULL, V4L2_CID_HBLANK,
> +					    h_blank, h_blank, 1, h_blank);
> +	if (ov02a10->hblank)
> +		ov02a10->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> +	ov02a10->vblank = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops,
> +					    V4L2_CID_VBLANK, mode->vts_def -
> +					    mode->height,
> +					    OV02A10_VTS_MAX - mode->height, 1,
> +					    mode->vts_def - mode->height);
> +
> +	exposure_max = mode->vts_def - 4;
> +	ov02a10->exposure = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops,
> +					      V4L2_CID_EXPOSURE,
> +					      OV02A10_EXPOSURE_MIN,
> +					      exposure_max,
> +					      OV02A10_EXPOSURE_STEP,
> +					      mode->exp_def);
> +
> +	ov02a10->anal_gain = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops,
> +					       V4L2_CID_ANALOGUE_GAIN,
> +					       OV02A10_GAIN_MIN,
> +					       OV02A10_GAIN_MAX,
> +					       OV02A10_GAIN_STEP,
> +					       OV02A10_GAIN_DEFAULT);
> +
> +	ov02a10->test_pattern =
> +		v4l2_ctrl_new_std_menu_items(handler, &ov02a10_ctrl_ops,
> +					     V4L2_CID_TEST_PATTERN,
> +					     ARRAY_SIZE(ov02a10_test_pattern_menu) -
> +					     1, 0, 0,
> +					     ov02a10_test_pattern_menu);
> +
> +	if (handler->error) {
> +		ret = handler->error;
> +		dev_err(&client->dev, "failed to init controls(%d)\n", ret);
> +		goto err_free_handler;
> +	}
> +
> +	ov02a10->subdev.ctrl_handler = handler;
> +
> +	return 0;
> +
> +err_free_handler:
> +	v4l2_ctrl_handler_free(handler);
> +
> +	return ret;
> +}
> +
> +static int ov02a10_check_hwcfg(struct device *dev, struct ov02a10 *ov02a10)
> +{
> +	struct fwnode_handle *ep;
> +	struct fwnode_handle *fwnode = dev_fwnode(dev);
> +	struct v4l2_fwnode_endpoint bus_cfg = {
> +		.bus_type = V4L2_MBUS_CSI2_DPHY
> +	};
> +	unsigned int i, j;
> +	int ret;
> +
> +	if (!fwnode)
> +		return -ENXIO;
> +
> +	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
> +	if (!ep)
> +		return -ENXIO;
> +
> +	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> +	fwnode_handle_put(ep);
> +	if (ret)
> +		return ret;
> +
> +	/* Optional indication of mipi clock lane mode */
> +	if (bus_cfg.bus.mipi_csi2.flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK)
> +		ov02a10->mipi_clock_hs_mode_enable = true;
> +
> +	if (!bus_cfg.nr_of_link_frequencies) {
> +		dev_err(dev, "no link frequencies defined");
> +		ret = -EINVAL;
> +		goto check_hwcfg_error;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(link_freq_menu_items); i++) {
> +		for (j = 0; j < bus_cfg.nr_of_link_frequencies; j++) {
> +			if (link_freq_menu_items[i] ==
> +				bus_cfg.link_frequencies[j])
> +				break;
> +		}
> +
> +		if (j == bus_cfg.nr_of_link_frequencies) {
> +			dev_err(dev, "no link frequency %lld supported",
> +				link_freq_menu_items[i]);
> +			ret = -EINVAL;
> +			goto check_hwcfg_error;
> +		}
> +	}
> +
> +check_hwcfg_error:
> +	v4l2_fwnode_endpoint_free(&bus_cfg);
> +
> +	return ret;
> +}
> +
> +static int ov02a10_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct ov02a10 *ov02a10;
> +	unsigned int rotation;
> +	unsigned int clock_lane_tx_speed;
> +	unsigned int i;
> +	int ret;
> +
> +	ov02a10 = devm_kzalloc(dev, sizeof(*ov02a10), GFP_KERNEL);
> +	if (!ov02a10)
> +		return -ENOMEM;
> +
> +	ret = ov02a10_check_hwcfg(dev, ov02a10);
> +	if (ret) {
> +		dev_err(dev, "failed to check HW configuration: %d", ret);
> +		return ret;
> +	}
> +
> +	v4l2_i2c_subdev_init(&ov02a10->subdev, client, &ov02a10_subdev_ops);
> +	ov02a10->upside_down = false;
> +	ov02a10->fmt.code = MEDIA_BUS_FMT_SBGGR10_1X10;
> +
> +	/* Optional indication of physical rotation of sensor */
> +	ret = fwnode_property_read_u32(dev_fwnode(dev), "rotation", &rotation);
> +	if (!ret) {
> +		ov02a10->upside_down = rotation == 180;
> +		if (rotation == 180) {
> +			ov02a10->upside_down = true;
> +			ov02a10->fmt.code = MEDIA_BUS_FMT_SRGGB10_1X10;
> +		}
> +	} else {
> +		dev_warn(dev, "failed to get rotation\n");

The property is optional; you shouldn't warn it's missing in that case.

> +	}
> +
> +	/* Optional indication of mipi TX speed */
> +	ret = fwnode_property_read_u32(dev_fwnode(dev), "ovti,mipi-tx-speed",
> +				       &clock_lane_tx_speed);
> +

Please see my comments on v5.

> +	if (!ret)
> +		ov02a10->mipi_clock_tx_speed = clock_lane_tx_speed;
> +	else
> +		dev_warn(dev, "failed to get mipi tx speed, using default...\n");
> +
> +	/* Get system clock (eclk) */
> +	ov02a10->eclk = devm_clk_get(dev, "eclk");
> +	if (IS_ERR(ov02a10->eclk)) {
> +		ret = PTR_ERR(ov02a10->eclk);
> +		dev_err(dev, "failed to get eclk %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
> +				       &ov02a10->eclk_freq);
> +	if (ret) {
> +		dev_err(dev, "failed to get eclk frequency\n");
> +		return ret;
> +	}
> +
> +	ret = clk_set_rate(ov02a10->eclk, ov02a10->eclk_freq);
> +	if (ret) {
> +		dev_err(dev, "failed to set eclk frequency (24MHz)\n");
> +		return ret;
> +	}
> +
> +	if (clk_get_rate(ov02a10->eclk) != OV02A10_ECLK_FREQ) {
> +		dev_warn(dev, "wrong eclk frequency %d Hz, expected: %d Hz\n",
> +			 ov02a10->eclk_freq, OV02A10_ECLK_FREQ);
> +		return -EINVAL;
> +	}
> +
> +	ov02a10->pd_gpio = devm_gpiod_get(dev, "powerdown", GPIOD_OUT_HIGH);
> +	if (IS_ERR(ov02a10->pd_gpio)) {
> +		ret = PTR_ERR(ov02a10->pd_gpio);
> +		dev_err(dev, "failed to get powerdown-gpios %d\n", ret);
> +		return ret;
> +	}
> +
> +	ov02a10->n_rst_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(ov02a10->n_rst_gpio)) {
> +		ret = PTR_ERR(ov02a10->n_rst_gpio);
> +		dev_err(dev, "failed to get reset-gpios %d\n", ret);
> +		return ret;
> +	}
> +
> +	for (i = 0; i < OV02A10_NUM_SUPPLIES; i++)
> +		ov02a10->supplies[i].supply = ov02a10_supply_names[i];
> +
> +	ret = devm_regulator_bulk_get(dev, OV02A10_NUM_SUPPLIES,
> +				      ov02a10->supplies);
> +	if (ret) {
> +		dev_err(dev, "failed to get regulators\n");
> +		return ret;
> +	}
> +
> +	mutex_init(&ov02a10->mutex);
> +	ov02a10->cur_mode = &supported_modes[0];
> +	ret = ov02a10_initialize_controls(ov02a10);
> +	if (ret) {
> +		dev_err(dev, "failed to initialize controls\n");
> +		goto err_destroy_mutex;
> +	}
> +
> +	ov02a10->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	ov02a10->subdev.entity.ops = &ov02a10_subdev_entity_ops;
> +	ov02a10->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +	ov02a10->pad.flags = MEDIA_PAD_FL_SOURCE;
> +	ret = media_entity_pads_init(&ov02a10->subdev.entity, 1, &ov02a10->pad);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to init entity pads: %d", ret);
> +		goto err_free_handler;
> +	}
> +
> +	ret = v4l2_async_register_subdev(&ov02a10->subdev);
> +	if (ret) {
> +		dev_err(dev, "failed to register V4L2 subdev: %d", ret);
> +		goto err_clean_entity;
> +	}
> +
> +	pm_runtime_enable(dev);
> +	if (!pm_runtime_enabled(dev)) {
> +		ret = ov02a10_power_on(dev);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to power on: %d\n", ret);
> +			goto err_clean_entity;
> +		}
> +	}
> +
> +	return 0;
> +
> +err_clean_entity:
> +	media_entity_cleanup(&ov02a10->subdev.entity);
> +err_free_handler:
> +	v4l2_ctrl_handler_free(ov02a10->subdev.ctrl_handler);
> +err_destroy_mutex:
> +	mutex_destroy(&ov02a10->mutex);
> +
> +	return ret;
> +}
> +
> +static int ov02a10_remove(struct i2c_client *client)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct ov02a10 *ov02a10 = to_ov02a10(sd);
> +
> +	v4l2_async_unregister_subdev(sd);
> +	media_entity_cleanup(&sd->entity);
> +	v4l2_ctrl_handler_free(sd->ctrl_handler);
> +	pm_runtime_disable(&client->dev);
> +	if (!pm_runtime_status_suspended(&client->dev))
> +		ov02a10_power_off(&client->dev);
> +	pm_runtime_set_suspended(&client->dev);
> +	mutex_destroy(&ov02a10->mutex);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ov02a10_of_match[] = {
> +	{ .compatible = "ovti,ov02a10" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, ov02a10_of_match);
> +
> +static struct i2c_driver ov02a10_i2c_driver = {
> +	.driver = {
> +		.name = "ov02a10",
> +		.pm = &ov02a10_pm_ops,
> +		.of_match_table = ov02a10_of_match,
> +	},
> +	.probe_new	= &ov02a10_probe,
> +	.remove		= &ov02a10_remove,
> +};
> +
> +module_i2c_driver(ov02a10_i2c_driver);
> +
> +MODULE_AUTHOR("Dongchun Zhu <dongchun.zhu@mediatek.com>");
> +MODULE_DESCRIPTION("OmniVision OV02A10 sensor driver");
> +MODULE_LICENSE("GPL v2");
> +

-- 
Regards,

Sakari Ailus

Re: [V6, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver

[Dongchun Zhu]

Hello Sakari,

Thanks for the review. Sorry for the late reply.

On Fri, 2019-12-13 at 12:44 +0300, Sakari Ailus wrote:
> Hi Dongchun,
> 
> On Wed, Dec 11, 2019 at 07:28:49PM +0800, Dongchun Zhu wrote:
> > Add a V4L2 sub-device driver for OV02A10 image sensor. The OV02A10 is
> > a 1/5" CMOS sensor from Omnivision, asupporting output format: 10-bit Raw.
> >
> > This chip has a single MIPI lane interface and use the I2C bus for
> > control and the CSI-2 bus for data.
> >
> > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > ---
> >  MAINTAINERS                 |    1 +
> >  drivers/media/i2c/Kconfig   |   11 +
> >  drivers/media/i2c/Makefile  |    1 +
> >  drivers/media/i2c/ov02a10.c | 1102
> > +++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 1115 insertions(+)
> >  create mode 100644 drivers/media/i2c/ov02a10.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index 92a868c..e294530 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12135,6 +12135,7 @@ M:Dongchun Zhu <dongchun.zhu@mediatek.com>
> >  L:linux-media@vger.kernel.org
> >  T:git git://linuxtv.org/media_tree.git
> >  S:Maintained
> > +F:drivers/media/i2c/ov02a10.c
> >  F:Documentation/devicetree/bindings/media/i2c/ov02a10.txt
> >
> >  OMNIVISION OV2680 SENSOR DRIVER
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index c68e002..d3e8c41 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -640,6 +640,17 @@ config VIDEO_IMX355
> >    To compile this driver as a module, choose M here: the
> >    module will be called imx355.
> >
> > +config VIDEO_OV02A10
> > +tristate "OmniVision OV02A10 sensor support"
> > +depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> > +select V4L2_FWNODE
> > +help
> > +  This is a Video4Linux2 sensor driver for the OmniVision
> > +  OV02A10 camera.
> > +
> > +  To compile this driver as a module, choose M here: the
> > +  module will be called ov02a10.
> > +
> >  config VIDEO_OV2640
> >  tristate "OmniVision OV2640 sensor support"
> >  depends on VIDEO_V4L2 && I2C
> > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> > index c147bb9..b3769d7 100644
> > --- a/drivers/media/i2c/Makefile
> > +++ b/drivers/media/i2c/Makefile
> > @@ -63,6 +63,7 @@ obj-$(CONFIG_VIDEO_VP27SMPX) += vp27smpx.o
> >  obj-$(CONFIG_VIDEO_SONY_BTF_MPX) += sony-btf-mpx.o
> >  obj-$(CONFIG_VIDEO_UPD64031A) += upd64031a.o
> >  obj-$(CONFIG_VIDEO_UPD64083) += upd64083.o
> > +obj-$(CONFIG_VIDEO_OV02A10) += ov02a10.o
> >  obj-$(CONFIG_VIDEO_OV2640) += ov2640.o
> >  obj-$(CONFIG_VIDEO_OV2680) += ov2680.o
> >  obj-$(CONFIG_VIDEO_OV2685) += ov2685.o diff --git
> > a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c new file
> > mode 100644 index 0000000..0b930f6
> > --- /dev/null
> > +++ b/drivers/media/i2c/ov02a10.c
> > @@ -0,0 +1,1102 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (c) 2019 MediaTek Inc.
> > +
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regulator/consumer.h> #include <media/media-entity.h>
> > +#include <media/v4l2-async.h> #include <media/v4l2-ctrls.h> #include
> > +<media/v4l2-subdev.h> #include <media/v4l2-fwnode.h>
> > +
> > +#define CHIP_ID0x2509
> > +#define OV02A10_REG_CHIP_ID_H0x02
> > +#define OV02A10_REG_CHIP_ID_L0x03
> > +#define OV02A10_ID(_msb, _lsb)((_msb) << 8 | (_lsb))
> > +
> > +/* Bit[1] vertical upside down */
> > +/* Bit[0] horizontal mirror */
> > +#define REG_MIRROR_FLIP_CONTROL0x3f
> > +
> > +/* Orientation */
> > +#define REG_MIRROR_FLIP_ENABLE0x03
> > +
> > +/* Bit[7] clock HS mode enable
> > + * 0: Clock continue
> > + * 1: Clock HS
> > + * Bit[6:2] HS VOD adjust
> > + * Bit[1:0] P VHI adjust
> > + */
> > +#define REG_HS_MODE_BLC0x9d
> > +
> > +#define CLOCK_HS_MODE_ENABLEBIT(7)
> > +
> > +/* Bit[2:0] MIPI transmission speed select */
> > +#define TX_SPEED_AREA_SEL0xa1
> > +
> > +#define REG_PAGE_SWITCH0xfd
> > +#define REG_GLOBAL_EFFECTIVE0x01
> > +#define REG_ENABLEBIT(0)
> > +#define OV02A10_MASK_8_BITS0xff
> > +
> > +#define REG_SC_CTRL_MODE0xac
> > +#define SC_CTRL_MODE_STANDBY0x00
> > +#define SC_CTRL_MODE_STREAMING0x01
> > +
> > +#define OV02A10_EXP_SHIFT8
> > +#define OV02A10_REG_EXPOSURE_H0x03
> > +#define OV02A10_REG_EXPOSURE_L0x04
> > +#defineOV02A10_EXPOSURE_MIN4
> > +#define OV02A10_EXPOSURE_MAX_MARGIN4
> > +#defineOV02A10_EXPOSURE_STEP1
> > +
> > +#define OV02A10_VTS_SHIFT8
> > +#define OV02A10_REG_VTS_H0x05
> > +#define OV02A10_REG_VTS_L0x06
> > +#define OV02A10_VTS_MAX0x209f
> > +#define OV02A10_VTS_MIN0x04cf
> > +#define OV02A10_BASIC_LINE1224
> > +
> > +#define OV02A10_REG_GAIN0x24
> > +#define OV02A10_GAIN_MIN0x10
> > +#define OV02A10_GAIN_MAX0xf8
> > +#define OV02A10_GAIN_STEP0x01
> > +#define OV02A10_GAIN_DEFAULT0x40
> > +
> > +/* Test pattern control */
> > +#define OV02A10_REG_TEST_PATTERN0xb6
> > +#define OV02A10_TEST_PATTERN_ENABLEBIT(0)
> > +
> > +#define HZ_PER_MHZ1000000L
> > +#define OV02A10_LINK_FREQ_390MHZ(390 * HZ_PER_MHZ)
> > +#define OV02A10_ECLK_FREQ(24 * HZ_PER_MHZ)
> > +#define OV02A10_DATA_LANES1
> > +#define OV02A10_BITS_PER_SAMPLE10
> > +
> > +static const char * const ov02a10_supply_names[] = {
> > +"dovdd",/* Digital I/O power */
> > +"avdd",/* Analog power */
> > +"dvdd",/* Digital core power */
> > +};
> > +
> > +#define OV02A10_NUM_SUPPLIES ARRAY_SIZE(ov02a10_supply_names)
> > +
> > +struct ov02a10_reg {
> > +u8 addr;
> > +u8 val;
> > +};
> > +
> > +struct ov02a10_reg_list {
> > +u32 num_of_regs;
> > +const struct ov02a10_reg *regs;
> > +};
> > +
> > +struct ov02a10_mode {
> > +u32 width;
> > +u32 height;
> > +u32 exp_def;
> > +u32 hts_def;
> > +u32 vts_def;
> > +const struct ov02a10_reg_list reg_list; };
> > +
> > +struct ov02a10 {
> > +u32eclk_freq;
> > +u32                     mipi_clock_tx_speed;
> > +
> > +struct clk*eclk;
> > +struct gpio_desc*pd_gpio;
> > +struct gpio_desc*n_rst_gpio;
> > +struct regulator_bulk_data supplies[OV02A10_NUM_SUPPLIES];
> > +
> > +boolstreaming;
> > +boolupside_down;
> > +boolmipi_clock_hs_mode_enable;
> > +
> > +/*
> > + * Serialize control access, get/set format, get selection
> > + * and start streaming.
> > + */
> > +struct mutexmutex;
> > +struct v4l2_subdevsubdev;
> > +struct media_padpad;
> > +struct v4l2_ctrl*anal_gain;
> > +struct v4l2_ctrl*exposure;
> > +struct v4l2_ctrl*hblank;
> > +struct v4l2_ctrl*vblank;
> > +struct v4l2_ctrl*test_pattern;
> > +struct v4l2_mbus_framefmtfmt;
> > +struct v4l2_ctrl_handler ctrl_handler;
> > +
> > +const struct ov02a10_mode *cur_mode; };
> > +
> > +static inline struct ov02a10 *to_ov02a10(struct v4l2_subdev *sd) {
> > +return container_of(sd, struct ov02a10, subdev); }
> > +
> > +/*
> > + * eclk 24Mhz
> > + * pclk 39Mhz
> > + * linelength 934(0x3a6)
> > + * framelength 1390(0x56E)
> > + * grabwindow_width 1600
> > + * grabwindow_height 1200
> > + * max_framerate 30fps
> > + * mipi_datarate per lane 780Mbps
> > + */
> > +static const struct ov02a10_reg ov02a10_1600x1200_regs[] = {
> > +{0xfd, 0x01},
> > +{0xac, 0x00},
> > +{0xfd, 0x00},
> > +{0x2f, 0x29},
> > +{0x34, 0x00},
> > +{0x35, 0x21},
> > +{0x30, 0x15},
> > +{0x33, 0x01},
> > +{0xfd, 0x01},
> > +{0x44, 0x00},
> > +{0x2a, 0x4c},
> > +{0x2b, 0x1e},
> > +{0x2c, 0x60},
> > +{0x25, 0x11},
> > +{0x03, 0x01},
> > +{0x04, 0xae},
> > +{0x09, 0x00},
> > +{0x0a, 0x02},
> > +{0x06, 0xa6},
> > +{0x31, 0x00},
> > +{0x24, 0x40},
> > +{0x01, 0x01},
> > +{0xfb, 0x73},
> > +{0xfd, 0x01},
> > +{0x16, 0x04},
> > +{0x1c, 0x09},
> > +{0x21, 0x42},
> > +{0x12, 0x04},
> > +{0x13, 0x10},
> > +{0x11, 0x40},
> > +{0x33, 0x81},
> > +{0xd0, 0x00},
> > +{0xd1, 0x01},
> > +{0xd2, 0x00},
> > +{0x50, 0x10},
> > +{0x51, 0x23},
> > +{0x52, 0x20},
> > +{0x53, 0x10},
> > +{0x54, 0x02},
> > +{0x55, 0x20},
> > +{0x56, 0x02},
> > +{0x58, 0x48},
> > +{0x5d, 0x15},
> > +{0x5e, 0x05},
> > +{0x66, 0x66},
> > +{0x68, 0x68},
> > +{0x6b, 0x00},
> > +{0x6c, 0x00},
> > +{0x6f, 0x40},
> > +{0x70, 0x40},
> > +{0x71, 0x0a},
> > +{0x72, 0xf0},
> > +{0x73, 0x10},
> > +{0x75, 0x80},
> > +{0x76, 0x10},
> > +{0x84, 0x00},
> > +{0x85, 0x10},
> > +{0x86, 0x10},
> > +{0x87, 0x00},
> > +{0x8a, 0x22},
> > +{0x8b, 0x22},
> > +{0x19, 0xf1},
> > +{0x29, 0x01},
> > +{0xfd, 0x01},
> > +{0x9d, 0x16},
> > +{0xa0, 0x29},
> > +{0xa1, 0x03},
> > +{0xad, 0x62},
> > +{0xae, 0x00},
> > +{0xaf, 0x85},
> > +{0xb1, 0x01},
> > +{0x8e, 0x06},
> > +{0x8f, 0x40},
> > +{0x90, 0x04},
> > +{0x91, 0xb0},
> > +{0x45, 0x01},
> > +{0x46, 0x00},
> > +{0x47, 0x6c},
> > +{0x48, 0x03},
> > +{0x49, 0x8b},
> > +{0x4a, 0x00},
> > +{0x4b, 0x07},
> > +{0x4c, 0x04},
> > +{0x4d, 0xb7},
> > +{0xf0, 0x40},
> > +{0xf1, 0x40},
> > +{0xf2, 0x40},
> > +{0xf3, 0x40},
> > +{0x3f, 0x00},
> > +{0xfd, 0x01},
> > +{0x05, 0x00},
> > +{0x06, 0xa6},
> > +{0xfd, 0x01},
> > +};
> > +
> > +static const char * const ov02a10_test_pattern_menu[] = {
> > +"Disabled",
> > +"Color Bar",
> > +};
> > +
> > +static const s64 link_freq_menu_items[] = {
> > +OV02A10_LINK_FREQ_390MHZ,
> > +};
> > +
> > +static u64 to_pixel_rate(u32 f_index)
> > +{
> > +u64 pixel_rate = link_freq_menu_items[f_index] * 2 * OV02A10_DATA_LANES;
> > +
> > +do_div(pixel_rate, OV02A10_BITS_PER_SAMPLE);
> > +
> > +return pixel_rate;
> > +}
> > +
> > +static const struct ov02a10_mode supported_modes[] = {
> > +{
> > +.width = 1600,
> > +.height = 1200,
> > +.exp_def = 0x01ae,
> > +.hts_def = 0x03a6,
> > +.vts_def = 0x056e,
> > +.reg_list = {
> > +.num_of_regs = ARRAY_SIZE(ov02a10_1600x1200_regs),
> > +.regs = ov02a10_1600x1200_regs,
> > +},
> > +},
> > +};
> > +
> > +static int ov02a10_write_array(struct ov02a10 *ov02a10,
> > +       const struct ov02a10_reg_list *r_list)
> > +{
> > +struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > +unsigned int i;
> > +int ret;
> > +
> > +for (i = 0; i < r_list->num_of_regs; i++) {
> > +ret = i2c_smbus_write_byte_data(client, r_list->regs[i].addr,
> > +r_list->regs[i].val);
> > +if (ret < 0)
> > +return ret;
> > +}
> > +
> > +return 0;
> > +}
> > +
> > +static int ov02a10_read_smbus(struct ov02a10 *ov02a10, unsigned char reg,
> > +      unsigned char *val)
> > +{
> > +struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > +int ret;
> > +
> > +ret = i2c_smbus_read_byte_data(client, reg);
> > +
> > +if (ret < 0)
> > +return ret;
> > +
> > +*val = (unsigned char)ret;
> > +
> > +return 0;
> > +}
> > +
> > +static int ov02a10_mod_reg(struct ov02a10 *ov02a10, u8 reg, u8 mask, u8 val)
> > +{
> > +struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > +u8 readval;
> > +int ret;
> > +
> > +ret = ov02a10_read_smbus(ov02a10, reg, &readval);
> > +if (ret)
> > +return ret;
> > +
> > +val = (readval & ~mask) | (val & mask);
> > +
> > +return i2c_smbus_write_byte_data(client, reg, val);
> > +}
> > +
> > +static void ov02a10_fill_fmt(const struct ov02a10_mode *mode,
> > +     struct v4l2_mbus_framefmt *fmt)
> > +{
> > +fmt->width = mode->width;
> > +fmt->height = mode->height;
> > +fmt->field = V4L2_FIELD_NONE;
> > +}
> > +
> > +static int ov02a10_set_fmt(struct v4l2_subdev *sd,
> > +   struct v4l2_subdev_pad_config *cfg,
> > +   struct v4l2_subdev_format *fmt)
> > +{
> > +struct ov02a10 *ov02a10 = to_ov02a10(sd);
> > +struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
> > +
> > +mutex_lock(&ov02a10->mutex);
> > +
> > +if (ov02a10->streaming) {
> > +mutex_unlock(&ov02a10->mutex);
> > +return -EBUSY;
> > +}
> > +
> > +/* Only one sensor mode supported */
> > +mbus_fmt->code = ov02a10->fmt.code;
> > +ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt);
> > +ov02a10->fmt = fmt->format;
> > +
> > +mutex_unlock(&ov02a10->mutex);
> > +
> > +return 0;
> > +}
> > +
> > +static int ov02a10_get_fmt(struct v4l2_subdev *sd,
> > +   struct v4l2_subdev_pad_config *cfg,
> > +   struct v4l2_subdev_format *fmt)
> > +{
> > +struct ov02a10 *ov02a10 = to_ov02a10(sd);
> > +struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
> > +
> > +mutex_lock(&ov02a10->mutex);
> > +
> > +fmt->format = ov02a10->fmt;
> > +mbus_fmt->code = ov02a10->fmt.code;
> > +ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt);
> > +
> > +mutex_unlock(&ov02a10->mutex);
> > +
> > +return 0;
> > +}
> > +
> > +static int ov02a10_enum_mbus_code(struct v4l2_subdev *sd,
> > +  struct v4l2_subdev_pad_config *cfg,
> > +  struct v4l2_subdev_mbus_code_enum *code)
> > +{
> > +struct ov02a10 *ov02a10 = to_ov02a10(sd);
> > +
> > +if (code->index >= ARRAY_SIZE(supported_modes))
> > +return -EINVAL;
> > +
> > +code->code = ov02a10->fmt.code;
> 
> Do you plan to have a different format as well as a different resolution
> for each mode?
> 

No plan for the moment, as OV02A10 supports UXGA (1600X1200) resolution
to the maximum,
and this size is near to the lower limit of most smart mobile devices.

> > +
> > +return 0;
> > +}
> > +
> > +static int ov02a10_enum_frame_sizes(struct v4l2_subdev *sd,
> > +    struct v4l2_subdev_pad_config *cfg,
> > +    struct v4l2_subdev_frame_size_enum *fse)
> > +{
> > +if (fse->index >= ARRAY_SIZE(supported_modes))
> > +return -EINVAL;
> > +
> > +fse->min_width  = supported_modes[fse->index].width;
> > +fse->max_width  = supported_modes[fse->index].width;
> > +fse->max_height = supported_modes[fse->index].height;
> > +fse->min_height = supported_modes[fse->index].height;
> > +
> > +return 0;
> > +}
> > +
> > +static int ov02a10_check_sensor_id(struct ov02a10 *ov02a10)
> > +{
> > +struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > +u16 id;
> > +u8 chip_id_h;
> > +u8 chip_id_l;
> > +int ret;
> > +
> > +/* Check sensor revision */
> > +ret = ov02a10_read_smbus(ov02a10, OV02A10_REG_CHIP_ID_H, &chip_id_h);
> > +if (ret)
> > +return ret;
> > +
> > +ret = ov02a10_read_smbus(ov02a10, OV02A10_REG_CHIP_ID_L, &chip_id_l);
> > +if (ret)
> > +return ret;
> > +
> > +id = OV02A10_ID(chip_id_h, chip_id_l);
> > +if (id != CHIP_ID) {
> > +dev_err(&client->dev, "Unexpected sensor id(%04x)\n", id);
> > +return -EINVAL;
> > +}
> > +
> > +dev_dbg(&client->dev, "Detected OV%04X sensor\n", id);
> > +
> > +return 0;
> > +}
> > +
> > +static int __maybe_unused ov02a10_power_on(struct device *dev)
> > +{
> > +struct i2c_client *client = to_i2c_client(dev);
> > +struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > +struct ov02a10 *ov02a10 = to_ov02a10(sd);
> > +int ret;
> > +
> > +gpiod_set_value_cansleep(ov02a10->n_rst_gpio, 0);
> > +gpiod_set_value_cansleep(ov02a10->pd_gpio, 1);
> > +
> > +ret = clk_prepare_enable(ov02a10->eclk);
> > +if (ret < 0) {
> > +dev_err(dev, "failed to enable eclk\n");
> > +return ret;
> > +}
> > +
> > +ret = regulator_bulk_enable(OV02A10_NUM_SUPPLIES, ov02a10->supplies);
> > +if (ret < 0) {
> > +dev_err(dev, "failed to enable regulators\n");
> > +goto disable_clk;
> > +}
> > +usleep_range(5000, 6000);
> > +
> > +gpiod_set_value_cansleep(ov02a10->pd_gpio, 0);
> > +usleep_range(5000, 6000);
> > +
> > +gpiod_set_value_cansleep(ov02a10->n_rst_gpio, 1);
> > +usleep_range(5000, 6000);
> > +
> > +ret = ov02a10_check_sensor_id(ov02a10);
> > +if (ret)
> > +goto disable_regulator;
> > +
> > +return 0;
> > +
> > +disable_regulator:
> > +regulator_bulk_disable(OV02A10_NUM_SUPPLIES, ov02a10->supplies);
> > +disable_clk:
> > +clk_disable_unprepare(ov02a10->eclk);
> > +
> > +return ret;
> > +}
> > +
> > +static int __maybe_unused ov02a10_power_off(struct device *dev)
> > +{
> > +struct i2c_client *client = to_i2c_client(dev);
> > +struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > +struct ov02a10 *ov02a10 = to_ov02a10(sd);
> > +
> > +gpiod_set_value_cansleep(ov02a10->n_rst_gpio, 0);
> > +clk_disable_unprepare(ov02a10->eclk);
> > +gpiod_set_value_cansleep(ov02a10->pd_gpio, 1);
> > +regulator_bulk_disable(OV02A10_NUM_SUPPLIES, ov02a10->supplies);
> > +
> > +return 0;
> > +}
> > +
> > +static int __ov02a10_start_stream(struct ov02a10 *ov02a10)
> > +{
> > +struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > +const struct ov02a10_reg_list *reg_list;
> > +int ret;
> > +
> > +/* Apply default values of current mode */
> > +reg_list = &ov02a10->cur_mode->reg_list;
> > +ret = ov02a10_write_array(ov02a10, reg_list);
> > +if (ret)
> > +return ret;
> > +
> > +/* Apply customized values from user */
> > +ret = __v4l2_ctrl_handler_setup(ov02a10->subdev.ctrl_handler);
> > +if (ret)
> > +return ret;
> > +
> > +/* Set orientation to 180 degree */
> > +if (ov02a10->upside_down) {
> > +ret = i2c_smbus_write_byte_data(client, REG_MIRROR_FLIP_CONTROL,
> > +REG_MIRROR_FLIP_ENABLE);
> > +if (ret) {
> > +dev_err(&client->dev, "failed to set orientation\n");
> > +return ret;
> > +}
> > +ret = i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
> > +REG_ENABLE);
> > +if (ret < 0)
> > +return ret;
> > +}
> > +
> > +/* Set clock lane transmission mode according to DT property */
> > +ret = ov02a10_mod_reg(ov02a10, REG_HS_MODE_BLC, CLOCK_HS_MODE_ENABLE,
> > +      ov02a10->mipi_clock_hs_mode_enable ?
> > +      CLOCK_HS_MODE_ENABLE : 0);
> > +if (ret < 0)
> > +return ret;
> > +
> > +/* Set mipi TX speed according to DT property */
> > +ret = i2c_smbus_write_byte_data(client, TX_SPEED_AREA_SEL,
> > +ov02a10->mipi_clock_tx_speed);
> > +if (ret < 0)
> > +return ret;
> > +
> > +/* Set stream on register */
> > +return i2c_smbus_write_byte_data(client, REG_SC_CTRL_MODE,
> > + SC_CTRL_MODE_STREAMING);
> > +}
> > +
> > +static int __ov02a10_stop_stream(struct ov02a10 *ov02a10)
> > +{
> > +struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > +
> > +return i2c_smbus_write_byte_data(client, REG_SC_CTRL_MODE,
> > + SC_CTRL_MODE_STANDBY);
> > +}
> > +
> > +static int ov02a10_entity_init_cfg(struct v4l2_subdev *sd,
> > +   struct v4l2_subdev_pad_config *cfg)
> > +{
> > +struct v4l2_subdev_format fmt = {
> > +.which = cfg ? V4L2_SUBDEV_FORMAT_TRY
> > +     : V4L2_SUBDEV_FORMAT_ACTIVE,
> > +.format = {
> > +.width = 1600,
> > +.height = 1200
> > +}
> > +};
> > +
> > +ov02a10_set_fmt(sd, cfg, &fmt);
> > +
> > +return 0;
> > +}
> > +
> > +static int ov02a10_s_stream(struct v4l2_subdev *sd, int on)
> > +{
> > +struct ov02a10 *ov02a10 = to_ov02a10(sd);
> > +struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > +int ret;
> > +
> > +dev_dbg(&client->dev, "ov02a10 s_stream (%d)\n", on);
> > +mutex_lock(&ov02a10->mutex);
> > +
> > +if (ov02a10->streaming == on)
> > +goto unlock_and_return;
> > +
> > +if (on) {
> > +ret = pm_runtime_get_sync(&client->dev);
> > +if (ret < 0) {
> > +pm_runtime_put_noidle(&client->dev);
> > +goto unlock_and_return;
> > +}
> > +
> > +ret = __ov02a10_start_stream(ov02a10);
> > +if (ret) {
> > +__ov02a10_stop_stream(ov02a10);
> > +ov02a10->streaming = !on;
> > +goto err_rpm_put;
> > +}
> > +} else {
> > +__ov02a10_stop_stream(ov02a10);
> > +pm_runtime_put(&client->dev);
> > +}
> > +
> > +ov02a10->streaming = on;
> > +mutex_unlock(&ov02a10->mutex);
> > +
> > +return 0;
> > +
> > +err_rpm_put:
> > +pm_runtime_put(&client->dev);
> > +unlock_and_return:
> > +mutex_unlock(&ov02a10->mutex);
> > +
> > +return ret;
> > +}
> > +
> > +static const struct dev_pm_ops ov02a10_pm_ops = {
> > +SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> > +pm_runtime_force_resume)
> > +SET_RUNTIME_PM_OPS(ov02a10_power_off, ov02a10_power_on, NULL)
> > +};
> > +
> > +/*
> > + * ov02a10_set_exposure - Function called when setting exposure time
> > + * @priv: Pointer to device structure
> > + * @val: Variable for exposure time, in the unit of micro-second
> > + *
> > + * Set exposure time based on input value.
> > + *
> > + * Return: 0 on success
> > + */
> > +static int ov02a10_set_exposure(struct ov02a10 *ov02a10, int val)
> > +{
> > +struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > +int ret;
> > +
> > +ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE);
> > +if (ret < 0)
> > +return ret;
> > +
> > +ret = i2c_smbus_write_byte_data(client, OV02A10_REG_EXPOSURE_H,
> > +((val >> OV02A10_EXP_SHIFT) &
> > +OV02A10_MASK_8_BITS));
> > +if (ret < 0)
> > +return ret;
> > +
> > +ret = i2c_smbus_write_byte_data(client, OV02A10_REG_EXPOSURE_L,
> > +(val & OV02A10_MASK_8_BITS));
> > +if (ret < 0)
> > +return ret;
> > +
> > +return i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
> > + REG_ENABLE);
> > +}
> > +
> > +static int ov02a10_set_gain(struct ov02a10 *ov02a10, int val)
> > +{
> > +struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > +int ret;
> > +
> > +ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE);
> > +if (ret < 0)
> > +return ret;
> > +
> > +ret = i2c_smbus_write_byte_data(client, OV02A10_REG_GAIN,
> > +(val & OV02A10_MASK_8_BITS));
> > +if (ret < 0)
> > +return ret;
> > +
> > +return i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
> > + REG_ENABLE);
> > +}
> > +
> > +static int ov02a10_set_vblank(struct ov02a10 *ov02a10, int val)
> > +{
> > +struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > +int ret;
> > +
> > +ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE);
> > +if (ret < 0)
> > +return ret;
> > +
> > +ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_H,
> > +(((val + ov02a10->cur_mode->height -
> > +OV02A10_BASIC_LINE) >>
> > +OV02A10_VTS_SHIFT) &
> > +OV02A10_MASK_8_BITS));
> > +if (ret < 0)
> > +return ret;
> > +
> > +ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_L,
> > +((val + ov02a10->cur_mode->height -
> > +OV02A10_BASIC_LINE) &
> > +OV02A10_MASK_8_BITS));
> > +if (ret < 0)
> > +return ret;
> > +
> > +return i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
> > + REG_ENABLE);
> > +}
> > +
> > +static int ov02a10_set_test_pattern(struct ov02a10 *ov02a10, int pattern)
> > +{
> > +struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > +int ret;
> > +
> > +if (pattern)
> > +pattern = OV02A10_TEST_PATTERN_ENABLE;
> > +
> > +ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE);
> > +if (ret < 0)
> > +return ret;
> > +
> > +ret = i2c_smbus_write_byte_data(client, OV02A10_REG_TEST_PATTERN,
> > +pattern);
> > +if (ret < 0)
> > +return ret;
> > +
> > +ret = i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
> > +REG_ENABLE);
> > +if (ret < 0)
> > +return ret;
> > +
> > +return i2c_smbus_write_byte_data(client, REG_SC_CTRL_MODE,
> > + SC_CTRL_MODE_STREAMING);
> > +}
> > +
> > +static int ov02a10_set_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +struct ov02a10 *ov02a10 = container_of(ctrl->handler,
> > +       struct ov02a10, ctrl_handler);
> > +struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > +s64 max_expo;
> > +int ret;
> > +
> > +/* Propagate change of current control to all related controls */
> > +if (ctrl->id == V4L2_CID_VBLANK) {
> > +/* Update max exposure while meeting expected vblanking */
> > +max_expo = ov02a10->cur_mode->height + ctrl->val -
> > +   OV02A10_EXPOSURE_MAX_MARGIN;
> > +__v4l2_ctrl_modify_range(ov02a10->exposure,
> > + ov02a10->exposure->minimum, max_expo,
> > + ov02a10->exposure->step,
> > + ov02a10->exposure->default_value);
> > +}
> > +
> > +/* V4L2 controls values will be applied only when power is already up */
> > +if (!pm_runtime_get_if_in_use(&client->dev))
> > +return 0;
> > +
> > +switch (ctrl->id) {
> > +case V4L2_CID_EXPOSURE:
> > +ret = ov02a10_set_exposure(ov02a10, ctrl->val);
> > +break;
> > +case V4L2_CID_ANALOGUE_GAIN:
> > +ret = ov02a10_set_gain(ov02a10, ctrl->val);
> > +break;
> > +case V4L2_CID_VBLANK:
> > +ret = ov02a10_set_vblank(ov02a10, ctrl->val);
> > +break;
> > +case V4L2_CID_TEST_PATTERN:
> > +ret = ov02a10_set_test_pattern(ov02a10, ctrl->val);
> > +break;
> > +default:
> > +ret = -EINVAL;
> > +break;
> > +};
> > +
> > +pm_runtime_put(&client->dev);
> > +
> > +return ret;
> > +}
> > +
> > +static const struct v4l2_subdev_video_ops ov02a10_video_ops = {
> > +.s_stream = ov02a10_s_stream,
> > +};
> > +
> > +static const struct v4l2_subdev_pad_ops ov02a10_pad_ops = {
> > +.init_cfg = ov02a10_entity_init_cfg,
> > +.enum_mbus_code = ov02a10_enum_mbus_code,
> > +.enum_frame_size = ov02a10_enum_frame_sizes,
> > +.get_fmt = ov02a10_get_fmt,
> > +.set_fmt = ov02a10_set_fmt,
> > +};
> > +
> > +static const struct v4l2_subdev_ops ov02a10_subdev_ops = {
> > +.video= &ov02a10_video_ops,
> > +.pad= &ov02a10_pad_ops,
> > +};
> > +
> > +static const struct media_entity_operations ov02a10_subdev_entity_ops = {
> > +.link_validate = v4l2_subdev_link_validate,
> > +};
> > +
> > +static const struct v4l2_ctrl_ops ov02a10_ctrl_ops = {
> > +.s_ctrl = ov02a10_set_ctrl,
> > +};
> > +
> > +static int ov02a10_initialize_controls(struct ov02a10 *ov02a10)
> > +{
> > +struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > +const struct ov02a10_mode *mode;
> > +struct v4l2_ctrl_handler *handler;
> > +struct v4l2_ctrl *ctrl;
> > +u64 exposure_max;
> > +u32 pixel_rate, h_blank;
> > +int ret;
> > +
> > +handler = &ov02a10->ctrl_handler;
> > +mode = ov02a10->cur_mode;
> > +ret = v4l2_ctrl_handler_init(handler, 7);
> > +if (ret)
> > +return ret;
> > +
> > +handler->lock = &ov02a10->mutex;
> > +
> > +ctrl = v4l2_ctrl_new_int_menu(handler, NULL, V4L2_CID_LINK_FREQ, 0, 0,
> > +      link_freq_menu_items);
> > +if (ctrl)
> > +ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +
> > +pixel_rate = to_pixel_rate(0);
> > +v4l2_ctrl_new_std(handler, NULL, V4L2_CID_PIXEL_RATE, 0, pixel_rate, 1,
> > +  pixel_rate);
> > +
> > +h_blank = mode->hts_def - mode->width;
> > +ov02a10->hblank = v4l2_ctrl_new_std(handler, NULL, V4L2_CID_HBLANK,
> > +    h_blank, h_blank, 1, h_blank);
> > +if (ov02a10->hblank)
> > +ov02a10->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +
> > +ov02a10->vblank = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops,
> > +    V4L2_CID_VBLANK, mode->vts_def -
> > +    mode->height,
> > +    OV02A10_VTS_MAX - mode->height, 1,
> > +    mode->vts_def - mode->height);
> > +
> > +exposure_max = mode->vts_def - 4;
> > +ov02a10->exposure = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops,
> > +      V4L2_CID_EXPOSURE,
> > +      OV02A10_EXPOSURE_MIN,
> > +      exposure_max,
> > +      OV02A10_EXPOSURE_STEP,
> > +      mode->exp_def);
> > +
> > +ov02a10->anal_gain = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops,
> > +       V4L2_CID_ANALOGUE_GAIN,
> > +       OV02A10_GAIN_MIN,
> > +       OV02A10_GAIN_MAX,
> > +       OV02A10_GAIN_STEP,
> > +       OV02A10_GAIN_DEFAULT);
> > +
> > +ov02a10->test_pattern =
> > +v4l2_ctrl_new_std_menu_items(handler, &ov02a10_ctrl_ops,
> > +     V4L2_CID_TEST_PATTERN,
> > +     ARRAY_SIZE(ov02a10_test_pattern_menu) -
> > +     1, 0, 0,
> > +     ov02a10_test_pattern_menu);
> > +
> > +if (handler->error) {
> > +ret = handler->error;
> > +dev_err(&client->dev, "failed to init controls(%d)\n", ret);
> > +goto err_free_handler;
> > +}
> > +
> > +ov02a10->subdev.ctrl_handler = handler;
> > +
> > +return 0;
> > +
> > +err_free_handler:
> > +v4l2_ctrl_handler_free(handler);
> > +
> > +return ret;
> > +}
> > +
> > +static int ov02a10_check_hwcfg(struct device *dev, struct ov02a10 *ov02a10)
> > +{
> > +struct fwnode_handle *ep;
> > +struct fwnode_handle *fwnode = dev_fwnode(dev);
> > +struct v4l2_fwnode_endpoint bus_cfg = {
> > +.bus_type = V4L2_MBUS_CSI2_DPHY
> > +};
> > +unsigned int i, j;
> > +int ret;
> > +
> > +if (!fwnode)
> > +return -ENXIO;
> > +
> > +ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
> > +if (!ep)
> > +return -ENXIO;
> > +
> > +ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> > +fwnode_handle_put(ep);
> > +if (ret)
> > +return ret;
> > +
> > +/* Optional indication of mipi clock lane mode */
> > +if (bus_cfg.bus.mipi_csi2.flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK)
> > +ov02a10->mipi_clock_hs_mode_enable = true;
> > +
> > +if (!bus_cfg.nr_of_link_frequencies) {
> > +dev_err(dev, "no link frequencies defined");
> > +ret = -EINVAL;
> > +goto check_hwcfg_error;
> > +}
> > +
> > +for (i = 0; i < ARRAY_SIZE(link_freq_menu_items); i++) {
> > +for (j = 0; j < bus_cfg.nr_of_link_frequencies; j++) {
> > +if (link_freq_menu_items[i] ==
> > +bus_cfg.link_frequencies[j])
> > +break;
> > +}
> > +
> > +if (j == bus_cfg.nr_of_link_frequencies) {
> > +dev_err(dev, "no link frequency %lld supported",
> > +link_freq_menu_items[i]);
> > +ret = -EINVAL;
> > +goto check_hwcfg_error;
> > +}
> > +}
> > +
> > +check_hwcfg_error:
> > +v4l2_fwnode_endpoint_free(&bus_cfg);
> > +
> > +return ret;
> > +}
> > +
> > +static int ov02a10_probe(struct i2c_client *client)
> > +{
> > +struct device *dev = &client->dev;
> > +struct ov02a10 *ov02a10;
> > +unsigned int rotation;
> > +unsigned int clock_lane_tx_speed;
> > +unsigned int i;
> > +int ret;
> > +
> > +ov02a10 = devm_kzalloc(dev, sizeof(*ov02a10), GFP_KERNEL);
> > +if (!ov02a10)
> > +return -ENOMEM;
> > +
> > +ret = ov02a10_check_hwcfg(dev, ov02a10);
> > +if (ret) {
> > +dev_err(dev, "failed to check HW configuration: %d", ret);
> > +return ret;
> > +}
> > +
> > +v4l2_i2c_subdev_init(&ov02a10->subdev, client, &ov02a10_subdev_ops);
> > +ov02a10->upside_down = false;
> > +ov02a10->fmt.code = MEDIA_BUS_FMT_SBGGR10_1X10;
> > +
> > +/* Optional indication of physical rotation of sensor */
> > +ret = fwnode_property_read_u32(dev_fwnode(dev), "rotation", &rotation);
> > +if (!ret) {
> > +ov02a10->upside_down = rotation == 180;
> > +if (rotation == 180) {
> > +ov02a10->upside_down = true;
> > +ov02a10->fmt.code = MEDIA_BUS_FMT_SRGGB10_1X10;
> > +}
> > +} else {
> > +dev_warn(dev, "failed to get rotation\n");
> 
> The property is optional; you shouldn't warn it's missing in that case.
> 

Thanks for the reminder.
Fixed in next release.

> > +}
> > +
> > +/* Optional indication of mipi TX speed */
> > +ret = fwnode_property_read_u32(dev_fwnode(dev), "ovti,mipi-tx-speed",
> > +       &clock_lane_tx_speed);
> > +
> 
> Please see my comments on v5.
> 

Sorry for not addressing that.
Fixed in next release.

> > +if (!ret)
> > +ov02a10->mipi_clock_tx_speed = clock_lane_tx_speed;
> > +else
> > +dev_warn(dev, "failed to get mipi tx speed, using default...\n");
> > +
> > +/* Get system clock (eclk) */
> > +ov02a10->eclk = devm_clk_get(dev, "eclk");
> > +if (IS_ERR(ov02a10->eclk)) {
> > +ret = PTR_ERR(ov02a10->eclk);
> > +dev_err(dev, "failed to get eclk %d\n", ret);
> > +return ret;
> > +}
> > +
> > +ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
> > +       &ov02a10->eclk_freq);
> > +if (ret) {
> > +dev_err(dev, "failed to get eclk frequency\n");
> > +return ret;
> > +}
> > +
> > +ret = clk_set_rate(ov02a10->eclk, ov02a10->eclk_freq);
> > +if (ret) {
> > +dev_err(dev, "failed to set eclk frequency (24MHz)\n");
> > +return ret;
> > +}
> > +
> > +if (clk_get_rate(ov02a10->eclk) != OV02A10_ECLK_FREQ) {
> > +dev_warn(dev, "wrong eclk frequency %d Hz, expected: %d Hz\n",
> > + ov02a10->eclk_freq, OV02A10_ECLK_FREQ);
> > +return -EINVAL;
> > +}
> > +
> > +ov02a10->pd_gpio = devm_gpiod_get(dev, "powerdown", GPIOD_OUT_HIGH);
> > +if (IS_ERR(ov02a10->pd_gpio)) {
> > +ret = PTR_ERR(ov02a10->pd_gpio);
> > +dev_err(dev, "failed to get powerdown-gpios %d\n", ret);
> > +return ret;
> > +}
> > +
> > +ov02a10->n_rst_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> > +if (IS_ERR(ov02a10->n_rst_gpio)) {
> > +ret = PTR_ERR(ov02a10->n_rst_gpio);
> > +dev_err(dev, "failed to get reset-gpios %d\n", ret);
> > +return ret;
> > +}
> > +
> > +for (i = 0; i < OV02A10_NUM_SUPPLIES; i++)
> > +ov02a10->supplies[i].supply = ov02a10_supply_names[i];
> > +
> > +ret = devm_regulator_bulk_get(dev, OV02A10_NUM_SUPPLIES,
> > +      ov02a10->supplies);
> > +if (ret) {
> > +dev_err(dev, "failed to get regulators\n");
> > +return ret;
> > +}
> > +
> > +mutex_init(&ov02a10->mutex);
> > +ov02a10->cur_mode = &supported_modes[0];
> > +ret = ov02a10_initialize_controls(ov02a10);
> > +if (ret) {
> > +dev_err(dev, "failed to initialize controls\n");
> > +goto err_destroy_mutex;
> > +}
> > +
> > +ov02a10->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > +ov02a10->subdev.entity.ops = &ov02a10_subdev_entity_ops;
> > +ov02a10->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> > +ov02a10->pad.flags = MEDIA_PAD_FL_SOURCE;
> > +ret = media_entity_pads_init(&ov02a10->subdev.entity, 1, &ov02a10->pad);
> > +if (ret < 0) {
> > +dev_err(dev, "failed to init entity pads: %d", ret);
> > +goto err_free_handler;
> > +}
> > +
> > +ret = v4l2_async_register_subdev(&ov02a10->subdev);
> > +if (ret) {
> > +dev_err(dev, "failed to register V4L2 subdev: %d", ret);
> > +goto err_clean_entity;
> > +}
> > +
> > +pm_runtime_enable(dev);
> > +if (!pm_runtime_enabled(dev)) {
> > +ret = ov02a10_power_on(dev);
> > +if (ret < 0) {
> > +dev_err(dev, "failed to power on: %d\n", ret);
> > +goto err_clean_entity;
> > +}
> > +}
> > +
> > +return 0;
> > +
> > +err_clean_entity:
> > +media_entity_cleanup(&ov02a10->subdev.entity);
> > +err_free_handler:
> > +v4l2_ctrl_handler_free(ov02a10->subdev.ctrl_handler);
> > +err_destroy_mutex:
> > +mutex_destroy(&ov02a10->mutex);
> > +
> > +return ret;
> > +}
> > +
> > +static int ov02a10_remove(struct i2c_client *client)
> > +{
> > +struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > +struct ov02a10 *ov02a10 = to_ov02a10(sd);
> > +
> > +v4l2_async_unregister_subdev(sd);
> > +media_entity_cleanup(&sd->entity);
> > +v4l2_ctrl_handler_free(sd->ctrl_handler);
> > +pm_runtime_disable(&client->dev);
> > +if (!pm_runtime_status_suspended(&client->dev))
> > +ov02a10_power_off(&client->dev);
> > +pm_runtime_set_suspended(&client->dev);
> > +mutex_destroy(&ov02a10->mutex);
> > +
> > +return 0;
> > +}
> > +
> > +static const struct of_device_id ov02a10_of_match[] = {
> > +{ .compatible = "ovti,ov02a10" },
> > +{}
> > +};
> > +MODULE_DEVICE_TABLE(of, ov02a10_of_match);
> > +
> > +static struct i2c_driver ov02a10_i2c_driver = {
> > +.driver = {
> > +.name = "ov02a10",
> > +.pm = &ov02a10_pm_ops,
> > +.of_match_table = ov02a10_of_match,
> > +},
> > +.probe_new= &ov02a10_probe,
> > +.remove= &ov02a10_remove,
> > +};
> > +
> > +module_i2c_driver(ov02a10_i2c_driver);
> > +
> > +MODULE_AUTHOR("Dongchun Zhu <dongchun.zhu@mediatek.com>");
> > +MODULE_DESCRIPTION("OmniVision OV02A10 sensor driver");
> > +MODULE_LICENSE("GPL v2");
> > +
> 
> --
> Regards,
> 
> Sakari Ailus
> *********************MEDIATEK Confidential/Internal Use*********************