* [V9, 0/2] media: i2c: Add support for OV02A10 sensor @ 2020-05-23 8:41 Dongchun Zhu 2020-05-23 8:41 ` [V9, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings Dongchun Zhu 2020-05-23 8:41 ` [V9, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver Dongchun Zhu 0 siblings, 2 replies; 31+ messages in thread From: Dongchun Zhu @ 2020-05-23 8:41 UTC (permalink / raw) To: linus.walleij, bgolaszewski, mchehab, andriy.shevchenko, robh+dt, mark.rutland, sakari.ailus, drinkcat, tfiga, matthias.bgg, bingbu.cao Cc: srv_heupstream, linux-mediatek, linux-arm-kernel, sj.huang, linux-media, devicetree, louis.kuo, shengnan.wang, dongchun.zhu Hello, This series adds DT bindings in YAML and V4L2 sub-device driver for Omnivision's OV02A10 2 megapixel CMOS 1/5" sensor, which has a single MIPI lane interface(I/F) and output format of 10-bit RAW. The driver is implemented with V4L2 Framework. - Async registered as one V4L2 sub-device. - As the first component of camera system including Seninf/ISP processing pipeline. - A media entity that provides one source pad in common and two for dual camera. Previous versions of this patch-set can be found here: v8: https://lore.kernel.org/linux-media/20200509080627.23222-1-dongchun.zhu@mediatek.com/ v7: https://lore.kernel.org/linux-media/20200430080924.1140-1-dongchun.zhu@mediatek.com/ v6: https://lore.kernel.org/linux-media/20191211112849.16705-1-dongchun.zhu@mediatek.com/ v5: https://lore.kernel.org/linux-media/20191104105713.24311-1-dongchun.zhu@mediatek.com/ v4: https://lore.kernel.org/linux-media/20190907092728.23897-1-dongchun.zhu@mediatek.com/ v3: https://lore.kernel.org/linux-media/20190819034331.13098-1-dongchun.zhu@mediatek.com/ v2: https://lore.kernel.org/linux-media/20190704084651.3105-1-dongchun.zhu@mediatek.com/ v1: https://lore.kernel.org/linux-media/20190523102204.24112-1-dongchun.zhu@mediatek.com/ Changes of v9 mainly address comments from Rob, Sakari, Tomasz, Andy. Including: - Add more detailed descriptions for powerdown-gpios and reset-gpios in DT - Set default to properties: "rotation" and "ovti,mipi-tx-speed" - Remove reserved values of "ovti,mipi-tx-speed" - Use ARRAY_SIZE() directly to replace of defining macro function - Remove __maybe_unused specifier for ov02a10_power_on and ov02a10_power_off - Refine driver by removing unnecessary logs and unused macros or fields. - Power off sensor when async register subdev failed and !pm_runtime_enabled() - Fix other review comments in v8 Please review. Thanks. Dongchun Zhu (2): media: dt-bindings: media: i2c: Document OV02A10 bindings media: i2c: ov02a10: Add OV02A10 image sensor driver .../bindings/media/i2c/ovti,ov02a10.yaml | 172 ++++ MAINTAINERS | 8 + drivers/media/i2c/Kconfig | 13 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/ov02a10.c | 1025 ++++++++++++++++++++ 5 files changed, 1219 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml create mode 100644 drivers/media/i2c/ov02a10.c -- 2.9.2 ^ permalink raw reply [flat|nested] 31+ messages in thread
* [V9, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings 2020-05-23 8:41 [V9, 0/2] media: i2c: Add support for OV02A10 sensor Dongchun Zhu @ 2020-05-23 8:41 ` Dongchun Zhu 2020-05-26 18:28 ` Rob Herring 2020-05-23 8:41 ` [V9, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver Dongchun Zhu 1 sibling, 1 reply; 31+ messages in thread From: Dongchun Zhu @ 2020-05-23 8:41 UTC (permalink / raw) To: linus.walleij, bgolaszewski, mchehab, andriy.shevchenko, robh+dt, mark.rutland, sakari.ailus, drinkcat, tfiga, matthias.bgg, bingbu.cao Cc: srv_heupstream, linux-mediatek, linux-arm-kernel, sj.huang, linux-media, devicetree, louis.kuo, shengnan.wang, dongchun.zhu Add DT bindings documentation for Omnivision OV02A10 image sensor. Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> --- .../bindings/media/i2c/ovti,ov02a10.yaml | 172 +++++++++++++++++++++ MAINTAINERS | 7 + 2 files changed, 179 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml new file mode 100644 index 0000000..56f31b5 --- /dev/null +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml @@ -0,0 +1,172 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +# Copyright (c) 2020 MediaTek Inc. +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/ovti,ov02a10.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Omnivision OV02A10 CMOS Sensor Device Tree Bindings + +maintainers: + - Dongchun Zhu <dongchun.zhu@mediatek.com> + +description: |- + The Omnivision OV02A10 is a low-cost, high performance, 1/5-inch, 2 megapixel + image sensor, which is the latest production derived from Omnivision's CMOS + image sensor technology. Ihis chip supports high frame rate speeds up to 30fps + @ 1600x1200 (UXGA) resolution transferred over a 1-lane MIPI interface. The + sensor output is available via CSI-2 serial data output. + +properties: + compatible: + const: ovti,ov02a10 + + reg: + maxItems: 1 + + clocks: + items: + - description: top mux camtg clock + - description: divider clock + + clock-names: + items: + - const: eclk + - const: freq_mux + + clock-frequency: + description: + Frequency of the eclk clock in Hertz. + + dovdd-supply: + description: + Definition of the regulator used as Digital I/O voltage supply. + + avdd-supply: + description: + Definition of the regulator used as Analog voltage supply. + + dvdd-supply: + description: + Definition of the regulator used as Digital core voltage supply. + + powerdown-gpios: + description: + Must be the device tree identifier of the GPIO connected to the + PD_PAD pin. This pin is used to place the OV02A10 into Standby mode + or Shutdown mode. As the line is active low, it should be + marked GPIO_ACTIVE_LOW. + + reset-gpios: + description: + Must be the device tree identifier of the GPIO connected to the + RST_PD pin. If specified, it will be asserted during driver probe. + As the line is active high, it should be marked GPIO_ACTIVE_HIGH. + + rotation: + description: + Definition of the sensor's placement. + allOf: + - $ref: "/schemas/types.yaml#/definitions/uint32" + - enum: + - 0 # Sensor Mounted Upright + - 180 # Sensor Mounted Upside Down + default: 0 + + ovti,mipi-tx-speed: + description: + Indication of MIPI transmission speed select, which is to control D-PHY + timing setting by adjusting MIPI clock voltage to improve the clock + driver capability. + allOf: + - $ref: "/schemas/types.yaml#/definitions/uint32" + - enum: + - 0 # 20MHz - 30MHz + - 1 # 30MHz - 50MHz + - 2 # 50MHz - 75MHz + - 3 # 75MHz - 100MHz + - 4 # 100MHz - 130MHz + default: 3 + + # See ../video-interfaces.txt for details + port: + type: object + additionalProperties: false + + properties: + endpoint: + type: object + additionalProperties: false + + properties: + data-lanes: + maxItems: 1 + + link-frequencies: true + remote-endpoint: true + + required: + - data-lanes + - link-frequencies + - remote-endpoint + + required: + - endpoint + +required: + - compatible + - reg + - clocks + - clock-names + - clock-frequency + - dovdd-supply + - avdd-supply + - dvdd-supply + - powerdown-gpios + - reset-gpios + - port + +additionalProperties: false + +examples: + - | + + #include <dt-bindings/clock/mt8183-clk.h> + #include <dt-bindings/gpio/gpio.h> + + i2c { + #address-cells = <1>; + #size-cells = <0>; + + ov02a10: camera-sensor@3d { + compatible = "ovti,ov02a10"; + reg = <0x3d>; + pinctrl-names = "default"; + pinctrl-0 = <&clk_24m_cam>; + + clocks = <&topckgen CLK_TOP_MUX_CAMTG>, + <&topckgen CLK_TOP_UNIVP_192M_D8>; + clock-names = "eclk", "freq_mux"; + clock-frequency = <24000000>; + + rotation = <180>; + ovti,mipi-tx-speed = <4>; + + dovdd-supply = <&mt6358_vcamio_reg>; + avdd-supply = <&mt6358_vcama1_reg>; + dvdd-supply = <&mt6358_vcn18_reg>; + + powerdown-gpios = <&pio 107 GPIO_ACTIVE_LOW>; + reset-gpios = <&pio 109 GPIO_ACTIVE_HIGH>; + + port { + wcam_out: endpoint { + data-lanes = <1>; + link-frequencies = /bits/ 64 <390000000>; + remote-endpoint = <&mipi_in_wcam>; + }; + }; + }; + }; + +... diff --git a/MAINTAINERS b/MAINTAINERS index e64e5db..63a2335 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12389,6 +12389,13 @@ M: Harald Welte <laforge@gnumonks.org> S: Maintained F: drivers/char/pcmcia/cm4040_cs.* +OMNIVISION OV02A10 SENSOR DRIVER +M: Dongchun Zhu <dongchun.zhu@mediatek.com> +L: linux-media@vger.kernel.org +S: Maintained +T: git git://linuxtv.org/media_tree.git +F: Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml + OMNIVISION OV13858 SENSOR DRIVER M: Sakari Ailus <sakari.ailus@linux.intel.com> L: linux-media@vger.kernel.org -- 2.9.2 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [V9, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings 2020-05-23 8:41 ` [V9, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings Dongchun Zhu @ 2020-05-26 18:28 ` Rob Herring 2020-05-27 8:49 ` Dongchun Zhu 0 siblings, 1 reply; 31+ messages in thread From: Rob Herring @ 2020-05-26 18:28 UTC (permalink / raw) To: Dongchun Zhu Cc: linus.walleij, bgolaszewski, mchehab, andriy.shevchenko, mark.rutland, sakari.ailus, drinkcat, tfiga, matthias.bgg, bingbu.cao, srv_heupstream, linux-mediatek, linux-arm-kernel, sj.huang, linux-media, devicetree, louis.kuo, shengnan.wang On Sat, May 23, 2020 at 04:41:02PM +0800, Dongchun Zhu wrote: > Add DT bindings documentation for Omnivision OV02A10 image sensor. > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > --- > .../bindings/media/i2c/ovti,ov02a10.yaml | 172 +++++++++++++++++++++ > MAINTAINERS | 7 + > 2 files changed, 179 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml > > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml > new file mode 100644 > index 0000000..56f31b5 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml > @@ -0,0 +1,172 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +# Copyright (c) 2020 MediaTek Inc. > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/i2c/ovti,ov02a10.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Omnivision OV02A10 CMOS Sensor Device Tree Bindings > + > +maintainers: > + - Dongchun Zhu <dongchun.zhu@mediatek.com> > + > +description: |- > + The Omnivision OV02A10 is a low-cost, high performance, 1/5-inch, 2 megapixel > + image sensor, which is the latest production derived from Omnivision's CMOS > + image sensor technology. Ihis chip supports high frame rate speeds up to 30fps > + @ 1600x1200 (UXGA) resolution transferred over a 1-lane MIPI interface. The > + sensor output is available via CSI-2 serial data output. > + > +properties: > + compatible: > + const: ovti,ov02a10 > + > + reg: > + maxItems: 1 > + > + clocks: > + items: > + - description: top mux camtg clock > + - description: divider clock > + > + clock-names: > + items: > + - const: eclk > + - const: freq_mux > + > + clock-frequency: > + description: > + Frequency of the eclk clock in Hertz. > + > + dovdd-supply: > + description: > + Definition of the regulator used as Digital I/O voltage supply. > + > + avdd-supply: > + description: > + Definition of the regulator used as Analog voltage supply. > + > + dvdd-supply: > + description: > + Definition of the regulator used as Digital core voltage supply. > + > + powerdown-gpios: > + description: > + Must be the device tree identifier of the GPIO connected to the > + PD_PAD pin. This pin is used to place the OV02A10 into Standby mode > + or Shutdown mode. As the line is active low, it should be > + marked GPIO_ACTIVE_LOW. Need to define how many GPIOs ('maxItems: 1') > + > + reset-gpios: > + description: > + Must be the device tree identifier of the GPIO connected to the > + RST_PD pin. If specified, it will be asserted during driver probe. > + As the line is active high, it should be marked GPIO_ACTIVE_HIGH. Here too. > + > + rotation: > + description: > + Definition of the sensor's placement. > + allOf: > + - $ref: "/schemas/types.yaml#/definitions/uint32" > + - enum: > + - 0 # Sensor Mounted Upright > + - 180 # Sensor Mounted Upside Down > + default: 0 > + > + ovti,mipi-tx-speed: > + description: > + Indication of MIPI transmission speed select, which is to control D-PHY > + timing setting by adjusting MIPI clock voltage to improve the clock > + driver capability. > + allOf: > + - $ref: "/schemas/types.yaml#/definitions/uint32" > + - enum: > + - 0 # 20MHz - 30MHz > + - 1 # 30MHz - 50MHz > + - 2 # 50MHz - 75MHz > + - 3 # 75MHz - 100MHz > + - 4 # 100MHz - 130MHz > + default: 3 > + > + # See ../video-interfaces.txt for details > + port: > + type: object > + additionalProperties: false Should have a description of what data the port has. > + > + properties: > + endpoint: > + type: object > + additionalProperties: false > + > + properties: > + data-lanes: > + maxItems: 1 > + > + link-frequencies: true > + remote-endpoint: true > + > + required: > + - data-lanes > + - link-frequencies > + - remote-endpoint > + > + required: > + - endpoint > + > +required: > + - compatible > + - reg > + - clocks > + - clock-names > + - clock-frequency > + - dovdd-supply > + - avdd-supply > + - dvdd-supply > + - powerdown-gpios > + - reset-gpios > + - port > + > +additionalProperties: false > + > +examples: > + - | > + > + #include <dt-bindings/clock/mt8183-clk.h> > + #include <dt-bindings/gpio/gpio.h> > + > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + ov02a10: camera-sensor@3d { > + compatible = "ovti,ov02a10"; > + reg = <0x3d>; > + pinctrl-names = "default"; > + pinctrl-0 = <&clk_24m_cam>; > + > + clocks = <&topckgen CLK_TOP_MUX_CAMTG>, > + <&topckgen CLK_TOP_UNIVP_192M_D8>; > + clock-names = "eclk", "freq_mux"; > + clock-frequency = <24000000>; > + > + rotation = <180>; > + ovti,mipi-tx-speed = <4>; > + > + dovdd-supply = <&mt6358_vcamio_reg>; > + avdd-supply = <&mt6358_vcama1_reg>; > + dvdd-supply = <&mt6358_vcn18_reg>; > + > + powerdown-gpios = <&pio 107 GPIO_ACTIVE_LOW>; > + reset-gpios = <&pio 109 GPIO_ACTIVE_HIGH>; > + > + port { > + wcam_out: endpoint { > + data-lanes = <1>; > + link-frequencies = /bits/ 64 <390000000>; > + remote-endpoint = <&mipi_in_wcam>; > + }; > + }; > + }; > + }; > + > +... > diff --git a/MAINTAINERS b/MAINTAINERS > index e64e5db..63a2335 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -12389,6 +12389,13 @@ M: Harald Welte <laforge@gnumonks.org> > S: Maintained > F: drivers/char/pcmcia/cm4040_cs.* > > +OMNIVISION OV02A10 SENSOR DRIVER > +M: Dongchun Zhu <dongchun.zhu@mediatek.com> > +L: linux-media@vger.kernel.org > +S: Maintained > +T: git git://linuxtv.org/media_tree.git > +F: Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml > + > OMNIVISION OV13858 SENSOR DRIVER > M: Sakari Ailus <sakari.ailus@linux.intel.com> > L: linux-media@vger.kernel.org > -- > 2.9.2 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [V9, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings 2020-05-26 18:28 ` Rob Herring @ 2020-05-27 8:49 ` Dongchun Zhu 2020-05-27 15:27 ` Rob Herring 0 siblings, 1 reply; 31+ messages in thread From: Dongchun Zhu @ 2020-05-27 8:49 UTC (permalink / raw) To: Rob Herring Cc: linus.walleij, bgolaszewski, mchehab, andriy.shevchenko, mark.rutland, sakari.ailus, drinkcat, tfiga, matthias.bgg, bingbu.cao, srv_heupstream, linux-mediatek, linux-arm-kernel, sj.huang, linux-media, devicetree, louis.kuo, shengnan.wang, dongchun.zhu Hi Rob, Thanks for the review. Please see my replies below. On Tue, 2020-05-26 at 12:28 -0600, Rob Herring wrote: > On Sat, May 23, 2020 at 04:41:02PM +0800, Dongchun Zhu wrote: > > Add DT bindings documentation for Omnivision OV02A10 image sensor. > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > --- > > .../bindings/media/i2c/ovti,ov02a10.yaml | 172 +++++++++++++++++++++ > > MAINTAINERS | 7 + > > 2 files changed, 179 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml > > new file mode 100644 > > index 0000000..56f31b5 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml > > @@ -0,0 +1,172 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +# Copyright (c) 2020 MediaTek Inc. > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/media/i2c/ovti,ov02a10.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Omnivision OV02A10 CMOS Sensor Device Tree Bindings > > + > > +maintainers: > > + - Dongchun Zhu <dongchun.zhu@mediatek.com> > > + > > +description: |- > > + The Omnivision OV02A10 is a low-cost, high performance, 1/5-inch, 2 megapixel > > + image sensor, which is the latest production derived from Omnivision's CMOS > > + image sensor technology. Ihis chip supports high frame rate speeds up to 30fps > > + @ 1600x1200 (UXGA) resolution transferred over a 1-lane MIPI interface. The > > + sensor output is available via CSI-2 serial data output. > > + > > +properties: > > + compatible: > > + const: ovti,ov02a10 > > + > > + reg: > > + maxItems: 1 > > + > > + clocks: > > + items: > > + - description: top mux camtg clock > > + - description: divider clock > > + > > + clock-names: > > + items: > > + - const: eclk > > + - const: freq_mux > > + > > + clock-frequency: > > + description: > > + Frequency of the eclk clock in Hertz. > > + Rob, shall we use 'maxItems: 1' to constrain property: clock-frequency? Or could we adopt 'clock-frequency: true' directly here? > > + dovdd-supply: > > + description: > > + Definition of the regulator used as Digital I/O voltage supply. > > + Shall we add 'maxItems: 1' here? > > + avdd-supply: > > + description: > > + Definition of the regulator used as Analog voltage supply. > > + Ditto. > > + dvdd-supply: > > + description: > > + Definition of the regulator used as Digital core voltage supply. > > + Ditto. > > + powerdown-gpios: > > + description: > > + Must be the device tree identifier of the GPIO connected to the > > + PD_PAD pin. This pin is used to place the OV02A10 into Standby mode > > + or Shutdown mode. As the line is active low, it should be > > + marked GPIO_ACTIVE_LOW. > > Need to define how many GPIOs ('maxItems: 1') > It would be fixed like this in next release. powerdown-gpios: maxItems: 1 description: Must be the device tree identifier of the GPIO connected to the PD_PAD pin. This pin is used to place the OV02A10 into Standby mode or Shutdown mode. As the line is active low, it should be marked GPIO_ACTIVE_LOW. > > + > > + reset-gpios: > > + description: > > + Must be the device tree identifier of the GPIO connected to the > > + RST_PD pin. If specified, it will be asserted during driver probe. > > + As the line is active high, it should be marked GPIO_ACTIVE_HIGH. > > Here too. > Similar as 'powerdown-gpios'. Fixed in next release. > > + > > + rotation: > > + description: > > + Definition of the sensor's placement. > > + allOf: > > + - $ref: "/schemas/types.yaml#/definitions/uint32" > > + - enum: > > + - 0 # Sensor Mounted Upright > > + - 180 # Sensor Mounted Upside Down > > + default: 0 > > + > > + ovti,mipi-tx-speed: > > + description: > > + Indication of MIPI transmission speed select, which is to control D-PHY > > + timing setting by adjusting MIPI clock voltage to improve the clock > > + driver capability. > > + allOf: > > + - $ref: "/schemas/types.yaml#/definitions/uint32" > > + - enum: > > + - 0 # 20MHz - 30MHz > > + - 1 # 30MHz - 50MHz > > + - 2 # 50MHz - 75MHz > > + - 3 # 75MHz - 100MHz > > + - 4 # 100MHz - 130MHz > > + default: 3 > > + > > + # See ../video-interfaces.txt for details > > + port: > > + type: object > > + additionalProperties: false > > Should have a description of what data the port has. > It would be updated as below in next release. port: type: object additionalProperties: false description: Input port node, single endpoint describing the CSI-2 transmitter. > > + > > + properties: > > + endpoint: > > + type: object > > + additionalProperties: false > > + > > + properties: Actually I wonder whether we need to declare 'clock-lanes' here? > > + data-lanes: > > + maxItems: 1 > > + > > + link-frequencies: true > > + remote-endpoint: true > > + > > + required: Ditto. > > + - data-lanes > > + - link-frequencies > > + - remote-endpoint > > + > > + required: > > + - endpoint > > + > > +required: > > + - compatible > > + - reg > > + - clocks > > + - clock-names > > + - clock-frequency > > + - dovdd-supply > > + - avdd-supply > > + - dvdd-supply > > + - powerdown-gpios > > + - reset-gpios > > + - port > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + > > + #include <dt-bindings/clock/mt8183-clk.h> > > + #include <dt-bindings/gpio/gpio.h> > > + > > + i2c { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + ov02a10: camera-sensor@3d { > > + compatible = "ovti,ov02a10"; > > + reg = <0x3d>; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&clk_24m_cam>; > > + > > + clocks = <&topckgen CLK_TOP_MUX_CAMTG>, > > + <&topckgen CLK_TOP_UNIVP_192M_D8>; > > + clock-names = "eclk", "freq_mux"; > > + clock-frequency = <24000000>; > > + > > + rotation = <180>; > > + ovti,mipi-tx-speed = <4>; > > + > > + dovdd-supply = <&mt6358_vcamio_reg>; > > + avdd-supply = <&mt6358_vcama1_reg>; > > + dvdd-supply = <&mt6358_vcn18_reg>; > > + > > + powerdown-gpios = <&pio 107 GPIO_ACTIVE_LOW>; > > + reset-gpios = <&pio 109 GPIO_ACTIVE_HIGH>; > > + > > + port { > > + wcam_out: endpoint { > > + data-lanes = <1>; > > + link-frequencies = /bits/ 64 <390000000>; > > + remote-endpoint = <&mipi_in_wcam>; > > + }; > > + }; > > + }; > > + }; > > + > > +... > > diff --git a/MAINTAINERS b/MAINTAINERS > > index e64e5db..63a2335 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -12389,6 +12389,13 @@ M: Harald Welte <laforge@gnumonks.org> > > S: Maintained > > F: drivers/char/pcmcia/cm4040_cs.* > > > > +OMNIVISION OV02A10 SENSOR DRIVER > > +M: Dongchun Zhu <dongchun.zhu@mediatek.com> > > +L: linux-media@vger.kernel.org > > +S: Maintained > > +T: git git://linuxtv.org/media_tree.git > > +F: Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml > > + > > OMNIVISION OV13858 SENSOR DRIVER > > M: Sakari Ailus <sakari.ailus@linux.intel.com> > > L: linux-media@vger.kernel.org > > -- > > 2.9.2 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [V9, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings 2020-05-27 8:49 ` Dongchun Zhu @ 2020-05-27 15:27 ` Rob Herring 2020-05-27 21:16 ` Sakari Ailus 2020-05-28 5:53 ` Dongchun Zhu 0 siblings, 2 replies; 31+ messages in thread From: Rob Herring @ 2020-05-27 15:27 UTC (permalink / raw) To: Dongchun Zhu Cc: Linus Walleij, Bartosz Golaszewski, Mauro Carvalho Chehab, Andy Shevchenko, Mark Rutland, Sakari Ailus, Nicolas Boichat, Tomasz Figa, Matthias Brugger, Cao Bing Bu, srv_heupstream, moderated list:ARM/Mediatek SoC support, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, Sj Huang, Linux Media Mailing List, devicetree, Louis Kuo, Shengnan Wang (王圣男) On Wed, May 27, 2020 at 2:51 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote: > > Hi Rob, > > Thanks for the review. Please see my replies below. > > On Tue, 2020-05-26 at 12:28 -0600, Rob Herring wrote: > > On Sat, May 23, 2020 at 04:41:02PM +0800, Dongchun Zhu wrote: > > > Add DT bindings documentation for Omnivision OV02A10 image sensor. > > > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > --- > > > .../bindings/media/i2c/ovti,ov02a10.yaml | 172 +++++++++++++++++++++ > > > MAINTAINERS | 7 + > > > 2 files changed, 179 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml > > > new file mode 100644 > > > index 0000000..56f31b5 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml > > > @@ -0,0 +1,172 @@ > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > > +# Copyright (c) 2020 MediaTek Inc. > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/media/i2c/ovti,ov02a10.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Omnivision OV02A10 CMOS Sensor Device Tree Bindings > > > + > > > +maintainers: > > > + - Dongchun Zhu <dongchun.zhu@mediatek.com> > > > + > > > +description: |- > > > + The Omnivision OV02A10 is a low-cost, high performance, 1/5-inch, 2 megapixel > > > + image sensor, which is the latest production derived from Omnivision's CMOS > > > + image sensor technology. Ihis chip supports high frame rate speeds up to 30fps > > > + @ 1600x1200 (UXGA) resolution transferred over a 1-lane MIPI interface. The > > > + sensor output is available via CSI-2 serial data output. > > > + > > > +properties: > > > + compatible: > > > + const: ovti,ov02a10 > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + clocks: > > > + items: > > > + - description: top mux camtg clock > > > + - description: divider clock > > > + > > > + clock-names: > > > + items: > > > + - const: eclk > > > + - const: freq_mux > > > + > > > + clock-frequency: > > > + description: > > > + Frequency of the eclk clock in Hertz. > > > + > > Rob, shall we use 'maxItems: 1' to constrain property: clock-frequency? No, because it is a scalar, not an array. > Or could we adopt 'clock-frequency: true' directly here? As-is is fine. > > > + dovdd-supply: > > > + description: > > > + Definition of the regulator used as Digital I/O voltage supply. > > > + > > Shall we add 'maxItems: 1' here? No, supplies are always singular. > > > > + avdd-supply: > > > + description: > > > + Definition of the regulator used as Analog voltage supply. > > > + > > Ditto. > > > > + dvdd-supply: > > > + description: > > > + Definition of the regulator used as Digital core voltage supply. > > > + > > Ditto. > > > > + powerdown-gpios: > > > + description: > > > + Must be the device tree identifier of the GPIO connected to the > > > + PD_PAD pin. This pin is used to place the OV02A10 into Standby mode > > > + or Shutdown mode. As the line is active low, it should be > > > + marked GPIO_ACTIVE_LOW. > > > > Need to define how many GPIOs ('maxItems: 1') > > > > It would be fixed like this in next release. > powerdown-gpios: > maxItems: 1 > description: > Must be the device tree identifier of the GPIO connected to the > PD_PAD pin. This pin is used to place the OV02A10 into Standby mode > or Shutdown mode. As the line is active low, it should be > marked GPIO_ACTIVE_LOW. > > > > + > > > + reset-gpios: > > > + description: > > > + Must be the device tree identifier of the GPIO connected to the > > > + RST_PD pin. If specified, it will be asserted during driver probe. > > > + As the line is active high, it should be marked GPIO_ACTIVE_HIGH. > > > > Here too. > > > > Similar as 'powerdown-gpios'. > Fixed in next release. > > > > + > > > + rotation: > > > + description: > > > + Definition of the sensor's placement. > > > + allOf: > > > + - $ref: "/schemas/types.yaml#/definitions/uint32" > > > + - enum: > > > + - 0 # Sensor Mounted Upright > > > + - 180 # Sensor Mounted Upside Down > > > + default: 0 > > > + > > > + ovti,mipi-tx-speed: > > > + description: > > > + Indication of MIPI transmission speed select, which is to control D-PHY > > > + timing setting by adjusting MIPI clock voltage to improve the clock > > > + driver capability. > > > + allOf: > > > + - $ref: "/schemas/types.yaml#/definitions/uint32" > > > + - enum: > > > + - 0 # 20MHz - 30MHz > > > + - 1 # 30MHz - 50MHz > > > + - 2 # 50MHz - 75MHz > > > + - 3 # 75MHz - 100MHz > > > + - 4 # 100MHz - 130MHz > > > + default: 3 > > > + > > > + # See ../video-interfaces.txt for details > > > + port: > > > + type: object > > > + additionalProperties: false > > > > Should have a description of what data the port has. > > > > It would be updated as below in next release. > port: > type: object > additionalProperties: false > description: > Input port node, single endpoint describing the CSI-2 transmitter. Output? > > > > + > > > + properties: > > > + endpoint: > > > + type: object > > > + additionalProperties: false > > > + > > > + properties: > > Actually I wonder whether we need to declare 'clock-lanes' here? Yes, if you are using it. Rob ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [V9, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings 2020-05-27 15:27 ` Rob Herring @ 2020-05-27 21:16 ` Sakari Ailus 2020-05-28 3:34 ` Dongchun Zhu 2020-05-28 5:53 ` Dongchun Zhu 1 sibling, 1 reply; 31+ messages in thread From: Sakari Ailus @ 2020-05-27 21:16 UTC (permalink / raw) To: Rob Herring, Dongchun Zhu Cc: Linus Walleij, Bartosz Golaszewski, Mauro Carvalho Chehab, Andy Shevchenko, Mark Rutland, Nicolas Boichat, Tomasz Figa, Matthias Brugger, Cao Bing Bu, srv_heupstream, moderated list:ARM/Mediatek SoC support, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, Sj Huang, Linux Media Mailing List, devicetree, Louis Kuo, Shengnan Wang (王圣男) Hi Rob, Dongchun, On Wed, May 27, 2020 at 09:27:22AM -0600, Rob Herring wrote: > > > > + properties: > > > > + endpoint: > > > > + type: object > > > > + additionalProperties: false > > > > + > > > > + properties: > > > > Actually I wonder whether we need to declare 'clock-lanes' here? > > Yes, if you are using it. Dongchun, can you confirm the chip has a single data and a single clock lane and that it does not support lane reordering? So if there's nothing to convey to the driver, also the data-lanes should be removed IMO. -- Kind regards, Sakari Ailus ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [V9, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings 2020-05-27 21:16 ` Sakari Ailus @ 2020-05-28 3:34 ` Dongchun Zhu 2020-05-28 7:23 ` Sakari Ailus 0 siblings, 1 reply; 31+ messages in thread From: Dongchun Zhu @ 2020-05-28 3:34 UTC (permalink / raw) To: Sakari Ailus Cc: Rob Herring, Linus Walleij, Bartosz Golaszewski, Mauro Carvalho Chehab, Andy Shevchenko, Mark Rutland, Nicolas Boichat, Tomasz Figa, Matthias Brugger, Cao Bing Bu, srv_heupstream, moderated list:ARM/Mediatek SoC support, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, Sj Huang, Linux Media Mailing List, devicetree, Louis Kuo, Shengnan Wang (王圣男), dongchun.zhu Hi Sakari, Rob, On Thu, 2020-05-28 at 00:16 +0300, Sakari Ailus wrote: > Hi Rob, Dongchun, > > On Wed, May 27, 2020 at 09:27:22AM -0600, Rob Herring wrote: > > > > > + properties: > > > > > + endpoint: > > > > > + type: object > > > > > + additionalProperties: false > > > > > + > > > > > + properties: > > > > > > Actually I wonder whether we need to declare 'clock-lanes' here? > > > > Yes, if you are using it. > > Dongchun, can you confirm the chip has a single data and a single clock > lane and that it does not support lane reordering? > From the datasheet, 'MIPI inside the OV02A10 provides one single uni-directional clock lane and one bi-directional data lane solution for communication links between components inside a mobile device. The data lane has full support for HS(uni-directional) and LP(bi-directional) data transfer mode.' The sensor doesn't support lane reordering, so 'clock-lanes' property would not be added in next release. > So if there's nothing to convey to the driver, also the data-lanes should > be removed IMO. > However, 'data-lanes' property may still be required. It is known that either data-lanes or clock-lanes is an array of physical data lane indexes. Position of an entry determines the logical lane number, while the value of an entry indicates physical lane, e.g., for 1-lane MIPI CSI-2 bus we could have "data-lanes = <1>;", assuming the clock lane is on hardware lane 0. As mentioned earlier, the OV02A10 sensor supports only 1C1D and does not support lane reordering, so here we shall use 'data-lanes = <1>' as there is only a clock lane for OV02A10. Reminder: If 'data-lanes' property is not present, the driver would assume four-lane operation. This means for one-lane or two-lane operation, this property must be present and set to the right physical lane indexes. If the hardware does not support lane reordering, monotonically incremented values shall be used from 0 or 1 onwards, depending on whether or not there is also a clock lane. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [V9, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings 2020-05-28 3:34 ` Dongchun Zhu @ 2020-05-28 7:23 ` Sakari Ailus 2020-05-28 8:04 ` Dongchun Zhu 0 siblings, 1 reply; 31+ messages in thread From: Sakari Ailus @ 2020-05-28 7:23 UTC (permalink / raw) To: Dongchun Zhu Cc: Rob Herring, Linus Walleij, Bartosz Golaszewski, Mauro Carvalho Chehab, Andy Shevchenko, Mark Rutland, Nicolas Boichat, Tomasz Figa, Matthias Brugger, Cao Bing Bu, srv_heupstream, moderated list:ARM/Mediatek SoC support, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, Sj Huang, Linux Media Mailing List, devicetree, Louis Kuo, Shengnan Wang (王圣男) Hi Dongchun, On Thu, May 28, 2020 at 11:34:42AM +0800, Dongchun Zhu wrote: > Hi Sakari, Rob, > > On Thu, 2020-05-28 at 00:16 +0300, Sakari Ailus wrote: > > Hi Rob, Dongchun, > > > > On Wed, May 27, 2020 at 09:27:22AM -0600, Rob Herring wrote: > > > > > > + properties: > > > > > > + endpoint: > > > > > > + type: object > > > > > > + additionalProperties: false > > > > > > + > > > > > > + properties: > > > > > > > > Actually I wonder whether we need to declare 'clock-lanes' here? > > > > > > Yes, if you are using it. > > > > Dongchun, can you confirm the chip has a single data and a single clock > > lane and that it does not support lane reordering? > > > > From the datasheet, 'MIPI inside the OV02A10 provides one single > uni-directional clock lane and one bi-directional data lane solution for > communication links between components inside a mobile device. > The data lane has full support for HS(uni-directional) and > LP(bi-directional) data transfer mode.' > > The sensor doesn't support lane reordering, so 'clock-lanes' property > would not be added in next release. > > > So if there's nothing to convey to the driver, also the data-lanes should > > be removed IMO. > > > > However, 'data-lanes' property may still be required. > It is known that either data-lanes or clock-lanes is an array of > physical data lane indexes. Position of an entry determines the logical > lane number, while the value of an entry indicates physical lane, e.g., > for 1-lane MIPI CSI-2 bus we could have "data-lanes = <1>;", assuming > the clock lane is on hardware lane 0. > > As mentioned earlier, the OV02A10 sensor supports only 1C1D and does not > support lane reordering, so here we shall use 'data-lanes = <1>' as > there is only a clock lane for OV02A10. > > Reminder: > If 'data-lanes' property is not present, the driver would assume > four-lane operation. This means for one-lane or two-lane operation, this > property must be present and set to the right physical lane indexes. > If the hardware does not support lane reordering, monotonically > incremented values shall be used from 0 or 1 onwards, depending on > whether or not there is also a clock lane. How can the driver use four lanes, considering the device only supports a single lane?? -- Sakari Ailus ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [V9, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings 2020-05-28 7:23 ` Sakari Ailus @ 2020-05-28 8:04 ` Dongchun Zhu 2020-05-29 13:43 ` Tomasz Figa 0 siblings, 1 reply; 31+ messages in thread From: Dongchun Zhu @ 2020-05-28 8:04 UTC (permalink / raw) To: Sakari Ailus Cc: Rob Herring, Linus Walleij, Bartosz Golaszewski, Mauro Carvalho Chehab, Andy Shevchenko, Mark Rutland, Nicolas Boichat, Tomasz Figa, Matthias Brugger, Cao Bing Bu, srv_heupstream, moderated list:ARM/Mediatek SoC support, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, Sj Huang, Linux Media Mailing List, devicetree, Louis Kuo, Shengnan Wang (王圣男) Hi Sakari, On Thu, 2020-05-28 at 10:23 +0300, Sakari Ailus wrote: > Hi Dongchun, > > On Thu, May 28, 2020 at 11:34:42AM +0800, Dongchun Zhu wrote: > > Hi Sakari, Rob, > > > > On Thu, 2020-05-28 at 00:16 +0300, Sakari Ailus wrote: > > > Hi Rob, Dongchun, > > > > > > On Wed, May 27, 2020 at 09:27:22AM -0600, Rob Herring wrote: > > > > > > > + properties: > > > > > > > + endpoint: > > > > > > > + type: object > > > > > > > + additionalProperties: false > > > > > > > + > > > > > > > + properties: > > > > > > > > > > Actually I wonder whether we need to declare 'clock-lanes' here? > > > > > > > > Yes, if you are using it. > > > > > > Dongchun, can you confirm the chip has a single data and a single clock > > > lane and that it does not support lane reordering? > > > > > > > From the datasheet, 'MIPI inside the OV02A10 provides one single > > uni-directional clock lane and one bi-directional data lane solution for > > communication links between components inside a mobile device. > > The data lane has full support for HS(uni-directional) and > > LP(bi-directional) data transfer mode.' > > > > The sensor doesn't support lane reordering, so 'clock-lanes' property > > would not be added in next release. > > > > > So if there's nothing to convey to the driver, also the data-lanes should > > > be removed IMO. > > > > > > > However, 'data-lanes' property may still be required. > > It is known that either data-lanes or clock-lanes is an array of > > physical data lane indexes. Position of an entry determines the logical > > lane number, while the value of an entry indicates physical lane, e.g., > > for 1-lane MIPI CSI-2 bus we could have "data-lanes = <1>;", assuming > > the clock lane is on hardware lane 0. > > > > As mentioned earlier, the OV02A10 sensor supports only 1C1D and does not > > support lane reordering, so here we shall use 'data-lanes = <1>' as > > there is only a clock lane for OV02A10. > > > > Reminder: > > If 'data-lanes' property is not present, the driver would assume > > four-lane operation. This means for one-lane or two-lane operation, this > > property must be present and set to the right physical lane indexes. > > If the hardware does not support lane reordering, monotonically > > incremented values shall be used from 0 or 1 onwards, depending on > > whether or not there is also a clock lane. > > How can the driver use four lanes, considering the device only supports a > single lane?? > I understood your meaning. If we omit the property 'data-lanes', the sensor should work still. But then what's the meaning of the existence of 'data-lanes'? If this property 'data-lanes' is always optional, then why dt-bindings provide the interface? In the meantime, if omitting 'data-lanes' for one sensor(transmitter) that has only one physical data lane, MIPI receiver(e.g., MIPI CSI-2) shall enable four-lane configuration, which may increase consumption of both power and resource in the process of IIC communication. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [V9, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings 2020-05-28 8:04 ` Dongchun Zhu @ 2020-05-29 13:43 ` Tomasz Figa 2020-06-01 2:33 ` Dongchun Zhu 2020-06-02 9:53 ` Sakari Ailus 0 siblings, 2 replies; 31+ messages in thread From: Tomasz Figa @ 2020-05-29 13:43 UTC (permalink / raw) To: Dongchun Zhu Cc: Sakari Ailus, Rob Herring, Linus Walleij, Bartosz Golaszewski, Mauro Carvalho Chehab, Andy Shevchenko, Mark Rutland, Nicolas Boichat, Matthias Brugger, Cao Bing Bu, srv_heupstream, moderated list:ARM/Mediatek SoC support, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo, Shengnan Wang (王圣男) On Thu, May 28, 2020 at 10:06 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote: > > Hi Sakari, > > On Thu, 2020-05-28 at 10:23 +0300, Sakari Ailus wrote: > > Hi Dongchun, > > > > On Thu, May 28, 2020 at 11:34:42AM +0800, Dongchun Zhu wrote: > > > Hi Sakari, Rob, > > > > > > On Thu, 2020-05-28 at 00:16 +0300, Sakari Ailus wrote: > > > > Hi Rob, Dongchun, > > > > > > > > On Wed, May 27, 2020 at 09:27:22AM -0600, Rob Herring wrote: > > > > > > > > + properties: > > > > > > > > + endpoint: > > > > > > > > + type: object > > > > > > > > + additionalProperties: false > > > > > > > > + > > > > > > > > + properties: > > > > > > > > > > > > Actually I wonder whether we need to declare 'clock-lanes' here? > > > > > > > > > > Yes, if you are using it. > > > > > > > > Dongchun, can you confirm the chip has a single data and a single clock > > > > lane and that it does not support lane reordering? > > > > > > > > > > From the datasheet, 'MIPI inside the OV02A10 provides one single > > > uni-directional clock lane and one bi-directional data lane solution for > > > communication links between components inside a mobile device. > > > The data lane has full support for HS(uni-directional) and > > > LP(bi-directional) data transfer mode.' > > > > > > The sensor doesn't support lane reordering, so 'clock-lanes' property > > > would not be added in next release. > > > > > > > So if there's nothing to convey to the driver, also the data-lanes should > > > > be removed IMO. > > > > > > > > > > However, 'data-lanes' property may still be required. > > > It is known that either data-lanes or clock-lanes is an array of > > > physical data lane indexes. Position of an entry determines the logical > > > lane number, while the value of an entry indicates physical lane, e.g., > > > for 1-lane MIPI CSI-2 bus we could have "data-lanes = <1>;", assuming > > > the clock lane is on hardware lane 0. > > > > > > As mentioned earlier, the OV02A10 sensor supports only 1C1D and does not > > > support lane reordering, so here we shall use 'data-lanes = <1>' as > > > there is only a clock lane for OV02A10. > > > > > > Reminder: > > > If 'data-lanes' property is not present, the driver would assume > > > four-lane operation. This means for one-lane or two-lane operation, this > > > property must be present and set to the right physical lane indexes. > > > If the hardware does not support lane reordering, monotonically > > > incremented values shall be used from 0 or 1 onwards, depending on > > > whether or not there is also a clock lane. > > > > How can the driver use four lanes, considering the device only supports a > > single lane?? > > > > I understood your meaning. > If we omit the property 'data-lanes', the sensor should work still. > But then what's the meaning of the existence of 'data-lanes'? > If this property 'data-lanes' is always optional, then why dt-bindings > provide the interface? > > In the meantime, if omitting 'data-lanes' for one sensor(transmitter) > that has only one physical data lane, MIPI receiver(e.g., MIPI CSI-2) > shall enable four-lane configuration, which may increase consumption of > both power and resource in the process of IIC communication. Wouldn't the receiver still have the data-lanes property under its endpoint node, telling it how many lanes and in which order should be used? Best regards, Tomasz ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [V9, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings 2020-05-29 13:43 ` Tomasz Figa @ 2020-06-01 2:33 ` Dongchun Zhu 2020-06-01 18:18 ` Tomasz Figa 2020-06-02 9:53 ` Sakari Ailus 1 sibling, 1 reply; 31+ messages in thread From: Dongchun Zhu @ 2020-06-01 2:33 UTC (permalink / raw) To: Tomasz Figa Cc: Sakari Ailus, Rob Herring, Linus Walleij, Bartosz Golaszewski, Mauro Carvalho Chehab, Andy Shevchenko, Mark Rutland, Nicolas Boichat, Matthias Brugger, Cao Bing Bu, srv_heupstream, moderated list:ARM/Mediatek SoC support, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo, Shengnan Wang (王圣男), dongchun.zhu Hi Tomasz, On Fri, 2020-05-29 at 15:43 +0200, Tomasz Figa wrote: > On Thu, May 28, 2020 at 10:06 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote: > > > > Hi Sakari, > > > > On Thu, 2020-05-28 at 10:23 +0300, Sakari Ailus wrote: > > > Hi Dongchun, > > > > > > On Thu, May 28, 2020 at 11:34:42AM +0800, Dongchun Zhu wrote: > > > > Hi Sakari, Rob, > > > > > > > > On Thu, 2020-05-28 at 00:16 +0300, Sakari Ailus wrote: > > > > > Hi Rob, Dongchun, > > > > > > > > > > On Wed, May 27, 2020 at 09:27:22AM -0600, Rob Herring wrote: > > > > > > > > > + properties: > > > > > > > > > + endpoint: > > > > > > > > > + type: object > > > > > > > > > + additionalProperties: false > > > > > > > > > + > > > > > > > > > + properties: > > > > > > > > > > > > > > Actually I wonder whether we need to declare 'clock-lanes' here? > > > > > > > > > > > > Yes, if you are using it. > > > > > > > > > > Dongchun, can you confirm the chip has a single data and a single clock > > > > > lane and that it does not support lane reordering? > > > > > > > > > > > > > From the datasheet, 'MIPI inside the OV02A10 provides one single > > > > uni-directional clock lane and one bi-directional data lane solution for > > > > communication links between components inside a mobile device. > > > > The data lane has full support for HS(uni-directional) and > > > > LP(bi-directional) data transfer mode.' > > > > > > > > The sensor doesn't support lane reordering, so 'clock-lanes' property > > > > would not be added in next release. > > > > > > > > > So if there's nothing to convey to the driver, also the data-lanes should > > > > > be removed IMO. > > > > > > > > > > > > > However, 'data-lanes' property may still be required. > > > > It is known that either data-lanes or clock-lanes is an array of > > > > physical data lane indexes. Position of an entry determines the logical > > > > lane number, while the value of an entry indicates physical lane, e.g., > > > > for 1-lane MIPI CSI-2 bus we could have "data-lanes = <1>;", assuming > > > > the clock lane is on hardware lane 0. > > > > > > > > As mentioned earlier, the OV02A10 sensor supports only 1C1D and does not > > > > support lane reordering, so here we shall use 'data-lanes = <1>' as > > > > there is only a clock lane for OV02A10. > > > > > > > > Reminder: > > > > If 'data-lanes' property is not present, the driver would assume > > > > four-lane operation. This means for one-lane or two-lane operation, this > > > > property must be present and set to the right physical lane indexes. > > > > If the hardware does not support lane reordering, monotonically > > > > incremented values shall be used from 0 or 1 onwards, depending on > > > > whether or not there is also a clock lane. > > > > > > How can the driver use four lanes, considering the device only supports a > > > single lane?? > > > > > > > I understood your meaning. > > If we omit the property 'data-lanes', the sensor should work still. > > But then what's the meaning of the existence of 'data-lanes'? > > If this property 'data-lanes' is always optional, then why dt-bindings > > provide the interface? > > > > In the meantime, if omitting 'data-lanes' for one sensor(transmitter) > > that has only one physical data lane, MIPI receiver(e.g., MIPI CSI-2) > > shall enable four-lane configuration, which may increase consumption of > > both power and resource in the process of IIC communication. > > Wouldn't the receiver still have the data-lanes property under its > endpoint node, telling it how many lanes and in which order should be > used? > The MIPI receiver(RX) shall use v4l2_async_notifier_add_fwnode_remote_subdev() API to parse the property "data-lanes" under sensor output port. > Best regards, > Tomasz ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [V9, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings 2020-06-01 2:33 ` Dongchun Zhu @ 2020-06-01 18:18 ` Tomasz Figa 2020-06-02 6:15 ` Dongchun Zhu 0 siblings, 1 reply; 31+ messages in thread From: Tomasz Figa @ 2020-06-01 18:18 UTC (permalink / raw) To: Dongchun Zhu Cc: Sakari Ailus, Rob Herring, Linus Walleij, Bartosz Golaszewski, Mauro Carvalho Chehab, Andy Shevchenko, Mark Rutland, Nicolas Boichat, Matthias Brugger, Cao Bing Bu, srv_heupstream, moderated list:ARM/Mediatek SoC support, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo, Shengnan Wang (王圣男) On Mon, Jun 1, 2020 at 4:35 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote: > > Hi Tomasz, > > On Fri, 2020-05-29 at 15:43 +0200, Tomasz Figa wrote: > > On Thu, May 28, 2020 at 10:06 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote: > > > > > > Hi Sakari, > > > > > > On Thu, 2020-05-28 at 10:23 +0300, Sakari Ailus wrote: > > > > Hi Dongchun, > > > > > > > > On Thu, May 28, 2020 at 11:34:42AM +0800, Dongchun Zhu wrote: > > > > > Hi Sakari, Rob, > > > > > > > > > > On Thu, 2020-05-28 at 00:16 +0300, Sakari Ailus wrote: > > > > > > Hi Rob, Dongchun, > > > > > > > > > > > > On Wed, May 27, 2020 at 09:27:22AM -0600, Rob Herring wrote: > > > > > > > > > > + properties: > > > > > > > > > > + endpoint: > > > > > > > > > > + type: object > > > > > > > > > > + additionalProperties: false > > > > > > > > > > + > > > > > > > > > > + properties: > > > > > > > > > > > > > > > > Actually I wonder whether we need to declare 'clock-lanes' here? > > > > > > > > > > > > > > Yes, if you are using it. > > > > > > > > > > > > Dongchun, can you confirm the chip has a single data and a single clock > > > > > > lane and that it does not support lane reordering? > > > > > > > > > > > > > > > > From the datasheet, 'MIPI inside the OV02A10 provides one single > > > > > uni-directional clock lane and one bi-directional data lane solution for > > > > > communication links between components inside a mobile device. > > > > > The data lane has full support for HS(uni-directional) and > > > > > LP(bi-directional) data transfer mode.' > > > > > > > > > > The sensor doesn't support lane reordering, so 'clock-lanes' property > > > > > would not be added in next release. > > > > > > > > > > > So if there's nothing to convey to the driver, also the data-lanes should > > > > > > be removed IMO. > > > > > > > > > > > > > > > > However, 'data-lanes' property may still be required. > > > > > It is known that either data-lanes or clock-lanes is an array of > > > > > physical data lane indexes. Position of an entry determines the logical > > > > > lane number, while the value of an entry indicates physical lane, e.g., > > > > > for 1-lane MIPI CSI-2 bus we could have "data-lanes = <1>;", assuming > > > > > the clock lane is on hardware lane 0. > > > > > > > > > > As mentioned earlier, the OV02A10 sensor supports only 1C1D and does not > > > > > support lane reordering, so here we shall use 'data-lanes = <1>' as > > > > > there is only a clock lane for OV02A10. > > > > > > > > > > Reminder: > > > > > If 'data-lanes' property is not present, the driver would assume > > > > > four-lane operation. This means for one-lane or two-lane operation, this > > > > > property must be present and set to the right physical lane indexes. > > > > > If the hardware does not support lane reordering, monotonically > > > > > incremented values shall be used from 0 or 1 onwards, depending on > > > > > whether or not there is also a clock lane. > > > > > > > > How can the driver use four lanes, considering the device only supports a > > > > single lane?? > > > > > > > > > > I understood your meaning. > > > If we omit the property 'data-lanes', the sensor should work still. > > > But then what's the meaning of the existence of 'data-lanes'? > > > If this property 'data-lanes' is always optional, then why dt-bindings > > > provide the interface? > > > > > > In the meantime, if omitting 'data-lanes' for one sensor(transmitter) > > > that has only one physical data lane, MIPI receiver(e.g., MIPI CSI-2) > > > shall enable four-lane configuration, which may increase consumption of > > > both power and resource in the process of IIC communication. > > > > Wouldn't the receiver still have the data-lanes property under its > > endpoint node, telling it how many lanes and in which order should be > > used? > > > > The MIPI receiver(RX) shall use > v4l2_async_notifier_add_fwnode_remote_subdev() API to parse the property > "data-lanes" under sensor output port. That's not true. The MIPI receiver driver parses its own port node corresponding to the sensor. Also quoting the documentation [1]: "An endpoint subnode of a device contains all properties needed for _configuration of this device_ for data exchange with other device. In most cases properties at the peer 'endpoint' nodes will be identical, however they might need to be different when there is any signal modifications on the bus between two devices, e.g. there are logic signal inverters on the lines." In this case, there is such a signal modification if the sensor has a 1-lane bus and the receiver more lines, so the data-lanes properties would be different on both sides. [1] https://elixir.bootlin.com/linux/v5.7/source/Documentation/devicetree/bindings/media/video-interfaces.txt Best regards, Tomasz ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [V9, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings 2020-06-01 18:18 ` Tomasz Figa @ 2020-06-02 6:15 ` Dongchun Zhu 2020-06-02 9:56 ` Sakari Ailus 0 siblings, 1 reply; 31+ messages in thread From: Dongchun Zhu @ 2020-06-02 6:15 UTC (permalink / raw) To: Tomasz Figa Cc: Sakari Ailus, Rob Herring, Linus Walleij, Bartosz Golaszewski, Mauro Carvalho Chehab, Andy Shevchenko, Mark Rutland, Nicolas Boichat, Matthias Brugger, Cao Bing Bu, srv_heupstream, moderated list:ARM/Mediatek SoC support, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo, Shengnan Wang (王圣男) Hi Tomasz, Sakari, On Mon, 2020-06-01 at 20:18 +0200, Tomasz Figa wrote: > On Mon, Jun 1, 2020 at 4:35 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote: > > > > Hi Tomasz, > > > > On Fri, 2020-05-29 at 15:43 +0200, Tomasz Figa wrote: > > > On Thu, May 28, 2020 at 10:06 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote: > > > > > > > > Hi Sakari, > > > > > > > > On Thu, 2020-05-28 at 10:23 +0300, Sakari Ailus wrote: > > > > > Hi Dongchun, > > > > > > > > > > On Thu, May 28, 2020 at 11:34:42AM +0800, Dongchun Zhu wrote: > > > > > > Hi Sakari, Rob, > > > > > > > > > > > > On Thu, 2020-05-28 at 00:16 +0300, Sakari Ailus wrote: > > > > > > > Hi Rob, Dongchun, > > > > > > > > > > > > > > On Wed, May 27, 2020 at 09:27:22AM -0600, Rob Herring wrote: > > > > > > > > > > > + properties: > > > > > > > > > > > + endpoint: > > > > > > > > > > > + type: object > > > > > > > > > > > + additionalProperties: false > > > > > > > > > > > + > > > > > > > > > > > + properties: > > > > > > > > > > > > > > > > > > Actually I wonder whether we need to declare 'clock-lanes' here? > > > > > > > > > > > > > > > > Yes, if you are using it. > > > > > > > > > > > > > > Dongchun, can you confirm the chip has a single data and a single clock > > > > > > > lane and that it does not support lane reordering? > > > > > > > > > > > > > > > > > > > From the datasheet, 'MIPI inside the OV02A10 provides one single > > > > > > uni-directional clock lane and one bi-directional data lane solution for > > > > > > communication links between components inside a mobile device. > > > > > > The data lane has full support for HS(uni-directional) and > > > > > > LP(bi-directional) data transfer mode.' > > > > > > > > > > > > The sensor doesn't support lane reordering, so 'clock-lanes' property > > > > > > would not be added in next release. > > > > > > > > > > > > > So if there's nothing to convey to the driver, also the data-lanes should > > > > > > > be removed IMO. > > > > > > > > > > > > > > > > > > > However, 'data-lanes' property may still be required. > > > > > > It is known that either data-lanes or clock-lanes is an array of > > > > > > physical data lane indexes. Position of an entry determines the logical > > > > > > lane number, while the value of an entry indicates physical lane, e.g., > > > > > > for 1-lane MIPI CSI-2 bus we could have "data-lanes = <1>;", assuming > > > > > > the clock lane is on hardware lane 0. > > > > > > > > > > > > As mentioned earlier, the OV02A10 sensor supports only 1C1D and does not > > > > > > support lane reordering, so here we shall use 'data-lanes = <1>' as > > > > > > there is only a clock lane for OV02A10. > > > > > > > > > > > > Reminder: > > > > > > If 'data-lanes' property is not present, the driver would assume > > > > > > four-lane operation. This means for one-lane or two-lane operation, this > > > > > > property must be present and set to the right physical lane indexes. > > > > > > If the hardware does not support lane reordering, monotonically > > > > > > incremented values shall be used from 0 or 1 onwards, depending on > > > > > > whether or not there is also a clock lane. > > > > > > > > > > How can the driver use four lanes, considering the device only supports a > > > > > single lane?? > > > > > > > > > > > > > I understood your meaning. > > > > If we omit the property 'data-lanes', the sensor should work still. > > > > But then what's the meaning of the existence of 'data-lanes'? > > > > If this property 'data-lanes' is always optional, then why dt-bindings > > > > provide the interface? > > > > > > > > In the meantime, if omitting 'data-lanes' for one sensor(transmitter) > > > > that has only one physical data lane, MIPI receiver(e.g., MIPI CSI-2) > > > > shall enable four-lane configuration, which may increase consumption of > > > > both power and resource in the process of IIC communication. > > > > > > Wouldn't the receiver still have the data-lanes property under its > > > endpoint node, telling it how many lanes and in which order should be > > > used? > > > > > > > The MIPI receiver(RX) shall use > > v4l2_async_notifier_add_fwnode_remote_subdev() API to parse the property > > "data-lanes" under sensor output port. > > That's not true. The MIPI receiver driver parses its own port node > corresponding to the sensor. Also quoting the documentation [1]: > > "An endpoint subnode of a device contains all properties needed for > _configuration of this device_ for data exchange with other device. In most > cases properties at the peer 'endpoint' nodes will be identical, however they > might need to be different when there is any signal modifications on the bus > between two devices, e.g. there are logic signal inverters on the lines." > > In this case, there is such a signal modification if the sensor has a > 1-lane bus and the receiver more lines, so the data-lanes properties > would be different on both sides. > > [1] https://elixir.bootlin.com/linux/v5.7/source/Documentation/devicetree/bindings/media/video-interfaces.txt > Sorry for the misunderstanding. After doing some experiments about the data-lanes property under sensor i2c node, we found the API v4l2_async_notifier_add_fwnode_remote_subdev() that MIPI receiver driver used indeed parses the data-lanes under its own port node. Sorry make a mistake for the use case of sensor data-lanes previously. Now We may encounter one new question for this patch. In practice we haven't used the data-lanes under sensor i2c node anywhere, if sensor driver itself doesn't parse that. But there is still one reason to keep the exactly right data-lanes in DT. That is, the data-lanes under sensor i2c node could be used as a reference for MIPI receiver driver. Just as Tomasz said, 'The MIPI receiver driver parses its own port node corresponding to the sensor'. Sakari, Tomasz, what's your opinions about the present of data-lanes under sensor node or not? > Best regards, > Tomasz ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [V9, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings 2020-06-02 6:15 ` Dongchun Zhu @ 2020-06-02 9:56 ` Sakari Ailus 2020-06-04 2:20 ` Dongchun Zhu 0 siblings, 1 reply; 31+ messages in thread From: Sakari Ailus @ 2020-06-02 9:56 UTC (permalink / raw) To: Dongchun Zhu Cc: Tomasz Figa, Rob Herring, Linus Walleij, Bartosz Golaszewski, Mauro Carvalho Chehab, Andy Shevchenko, Mark Rutland, Nicolas Boichat, Matthias Brugger, Cao Bing Bu, srv_heupstream, moderated list:ARM/Mediatek SoC support, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo, Shengnan Wang (王圣男) Hi Dongchun, On Tue, Jun 02, 2020 at 02:15:01PM +0800, Dongchun Zhu wrote: > Hi Tomasz, Sakari, > > On Mon, 2020-06-01 at 20:18 +0200, Tomasz Figa wrote: > > On Mon, Jun 1, 2020 at 4:35 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote: > > > > > > Hi Tomasz, > > > > > > On Fri, 2020-05-29 at 15:43 +0200, Tomasz Figa wrote: > > > > On Thu, May 28, 2020 at 10:06 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote: > > > > > > > > > > Hi Sakari, > > > > > > > > > > On Thu, 2020-05-28 at 10:23 +0300, Sakari Ailus wrote: > > > > > > Hi Dongchun, > > > > > > > > > > > > On Thu, May 28, 2020 at 11:34:42AM +0800, Dongchun Zhu wrote: > > > > > > > Hi Sakari, Rob, > > > > > > > > > > > > > > On Thu, 2020-05-28 at 00:16 +0300, Sakari Ailus wrote: > > > > > > > > Hi Rob, Dongchun, > > > > > > > > > > > > > > > > On Wed, May 27, 2020 at 09:27:22AM -0600, Rob Herring wrote: > > > > > > > > > > > > + properties: > > > > > > > > > > > > + endpoint: > > > > > > > > > > > > + type: object > > > > > > > > > > > > + additionalProperties: false > > > > > > > > > > > > + > > > > > > > > > > > > + properties: > > > > > > > > > > > > > > > > > > > > Actually I wonder whether we need to declare 'clock-lanes' here? > > > > > > > > > > > > > > > > > > Yes, if you are using it. > > > > > > > > > > > > > > > > Dongchun, can you confirm the chip has a single data and a single clock > > > > > > > > lane and that it does not support lane reordering? > > > > > > > > > > > > > > > > > > > > > > From the datasheet, 'MIPI inside the OV02A10 provides one single > > > > > > > uni-directional clock lane and one bi-directional data lane solution for > > > > > > > communication links between components inside a mobile device. > > > > > > > The data lane has full support for HS(uni-directional) and > > > > > > > LP(bi-directional) data transfer mode.' > > > > > > > > > > > > > > The sensor doesn't support lane reordering, so 'clock-lanes' property > > > > > > > would not be added in next release. > > > > > > > > > > > > > > > So if there's nothing to convey to the driver, also the data-lanes should > > > > > > > > be removed IMO. > > > > > > > > > > > > > > > > > > > > > > However, 'data-lanes' property may still be required. > > > > > > > It is known that either data-lanes or clock-lanes is an array of > > > > > > > physical data lane indexes. Position of an entry determines the logical > > > > > > > lane number, while the value of an entry indicates physical lane, e.g., > > > > > > > for 1-lane MIPI CSI-2 bus we could have "data-lanes = <1>;", assuming > > > > > > > the clock lane is on hardware lane 0. > > > > > > > > > > > > > > As mentioned earlier, the OV02A10 sensor supports only 1C1D and does not > > > > > > > support lane reordering, so here we shall use 'data-lanes = <1>' as > > > > > > > there is only a clock lane for OV02A10. > > > > > > > > > > > > > > Reminder: > > > > > > > If 'data-lanes' property is not present, the driver would assume > > > > > > > four-lane operation. This means for one-lane or two-lane operation, this > > > > > > > property must be present and set to the right physical lane indexes. > > > > > > > If the hardware does not support lane reordering, monotonically > > > > > > > incremented values shall be used from 0 or 1 onwards, depending on > > > > > > > whether or not there is also a clock lane. > > > > > > > > > > > > How can the driver use four lanes, considering the device only supports a > > > > > > single lane?? > > > > > > > > > > > > > > > > I understood your meaning. > > > > > If we omit the property 'data-lanes', the sensor should work still. > > > > > But then what's the meaning of the existence of 'data-lanes'? > > > > > If this property 'data-lanes' is always optional, then why dt-bindings > > > > > provide the interface? > > > > > > > > > > In the meantime, if omitting 'data-lanes' for one sensor(transmitter) > > > > > that has only one physical data lane, MIPI receiver(e.g., MIPI CSI-2) > > > > > shall enable four-lane configuration, which may increase consumption of > > > > > both power and resource in the process of IIC communication. > > > > > > > > Wouldn't the receiver still have the data-lanes property under its > > > > endpoint node, telling it how many lanes and in which order should be > > > > used? > > > > > > > > > > The MIPI receiver(RX) shall use > > > v4l2_async_notifier_add_fwnode_remote_subdev() API to parse the property > > > "data-lanes" under sensor output port. > > > > That's not true. The MIPI receiver driver parses its own port node > > corresponding to the sensor. Also quoting the documentation [1]: > > > > "An endpoint subnode of a device contains all properties needed for > > _configuration of this device_ for data exchange with other device. In most > > cases properties at the peer 'endpoint' nodes will be identical, however they > > might need to be different when there is any signal modifications on the bus > > between two devices, e.g. there are logic signal inverters on the lines." > > > > In this case, there is such a signal modification if the sensor has a > > 1-lane bus and the receiver more lines, so the data-lanes properties > > would be different on both sides. > > > > [1] https://elixir.bootlin.com/linux/v5.7/source/Documentation/devicetree/bindings/media/video-interfaces.txt > > > > Sorry for the misunderstanding. > After doing some experiments about the data-lanes property under sensor > i2c node, we found the API > v4l2_async_notifier_add_fwnode_remote_subdev() that MIPI receiver driver > used indeed parses the data-lanes under its own port node. > > Sorry make a mistake for the use case of sensor data-lanes previously. > Now We may encounter one new question for this patch. > In practice we haven't used the data-lanes under sensor i2c node > anywhere, if sensor driver itself doesn't parse that. > > But there is still one reason to keep the exactly right data-lanes in > DT. That is, the data-lanes under sensor i2c node could be used as a > reference for MIPI receiver driver. > Just as Tomasz said, 'The MIPI receiver driver parses its own port node > corresponding to the sensor'. > > Sakari, Tomasz, what's your opinions about the present of data-lanes > under sensor node or not? The receiver driver doesn't parse the properties in the sensor (transmitter) device's endpoint. If that property provides no information to the receiver, as is the case here, it should be omitted. -- Regards, Sakari Ailus ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [V9, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings 2020-06-02 9:56 ` Sakari Ailus @ 2020-06-04 2:20 ` Dongchun Zhu 0 siblings, 0 replies; 31+ messages in thread From: Dongchun Zhu @ 2020-06-04 2:20 UTC (permalink / raw) To: Sakari Ailus Cc: Tomasz Figa, Rob Herring, Linus Walleij, Bartosz Golaszewski, Mauro Carvalho Chehab, Andy Shevchenko, Mark Rutland, Nicolas Boichat, Matthias Brugger, Cao Bing Bu, srv_heupstream, moderated list:ARM/Mediatek SoC support, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo, Shengnan Wang (王圣男), dongchun.zhu Hi Sakari, On Tue, 2020-06-02 at 12:56 +0300, Sakari Ailus wrote: > Hi Dongchun, > > On Tue, Jun 02, 2020 at 02:15:01PM +0800, Dongchun Zhu wrote: > > Hi Tomasz, Sakari, > > > > On Mon, 2020-06-01 at 20:18 +0200, Tomasz Figa wrote: > > > On Mon, Jun 1, 2020 at 4:35 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote: > > > > > > > > Hi Tomasz, > > > > > > > > On Fri, 2020-05-29 at 15:43 +0200, Tomasz Figa wrote: > > > > > On Thu, May 28, 2020 at 10:06 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote: > > > > > > > > > > > > Hi Sakari, > > > > > > > > > > > > On Thu, 2020-05-28 at 10:23 +0300, Sakari Ailus wrote: > > > > > > > Hi Dongchun, > > > > > > > > > > > > > > On Thu, May 28, 2020 at 11:34:42AM +0800, Dongchun Zhu wrote: > > > > > > > > Hi Sakari, Rob, > > > > > > > > > > > > > > > > On Thu, 2020-05-28 at 00:16 +0300, Sakari Ailus wrote: > > > > > > > > > Hi Rob, Dongchun, > > > > > > > > > > > > > > > > > > On Wed, May 27, 2020 at 09:27:22AM -0600, Rob Herring wrote: > > > > > > > > > > > > > + properties: > > > > > > > > > > > > > + endpoint: > > > > > > > > > > > > > + type: object > > > > > > > > > > > > > + additionalProperties: false > > > > > > > > > > > > > + > > > > > > > > > > > > > + properties: > > > > > > > > > > > > > > > > > > > > > > Actually I wonder whether we need to declare 'clock-lanes' here? > > > > > > > > > > > > > > > > > > > > Yes, if you are using it. > > > > > > > > > > > > > > > > > > Dongchun, can you confirm the chip has a single data and a single clock > > > > > > > > > lane and that it does not support lane reordering? > > > > > > > > > > > > > > > > > > > > > > > > > From the datasheet, 'MIPI inside the OV02A10 provides one single > > > > > > > > uni-directional clock lane and one bi-directional data lane solution for > > > > > > > > communication links between components inside a mobile device. > > > > > > > > The data lane has full support for HS(uni-directional) and > > > > > > > > LP(bi-directional) data transfer mode.' > > > > > > > > > > > > > > > > The sensor doesn't support lane reordering, so 'clock-lanes' property > > > > > > > > would not be added in next release. > > > > > > > > > > > > > > > > > So if there's nothing to convey to the driver, also the data-lanes should > > > > > > > > > be removed IMO. > > > > > > > > > > > > > > > > > > > > > > > > > However, 'data-lanes' property may still be required. > > > > > > > > It is known that either data-lanes or clock-lanes is an array of > > > > > > > > physical data lane indexes. Position of an entry determines the logical > > > > > > > > lane number, while the value of an entry indicates physical lane, e.g., > > > > > > > > for 1-lane MIPI CSI-2 bus we could have "data-lanes = <1>;", assuming > > > > > > > > the clock lane is on hardware lane 0. > > > > > > > > > > > > > > > > As mentioned earlier, the OV02A10 sensor supports only 1C1D and does not > > > > > > > > support lane reordering, so here we shall use 'data-lanes = <1>' as > > > > > > > > there is only a clock lane for OV02A10. > > > > > > > > > > > > > > > > Reminder: > > > > > > > > If 'data-lanes' property is not present, the driver would assume > > > > > > > > four-lane operation. This means for one-lane or two-lane operation, this > > > > > > > > property must be present and set to the right physical lane indexes. > > > > > > > > If the hardware does not support lane reordering, monotonically > > > > > > > > incremented values shall be used from 0 or 1 onwards, depending on > > > > > > > > whether or not there is also a clock lane. > > > > > > > > > > > > > > How can the driver use four lanes, considering the device only supports a > > > > > > > single lane?? > > > > > > > > > > > > > > > > > > > I understood your meaning. > > > > > > If we omit the property 'data-lanes', the sensor should work still. > > > > > > But then what's the meaning of the existence of 'data-lanes'? > > > > > > If this property 'data-lanes' is always optional, then why dt-bindings > > > > > > provide the interface? > > > > > > > > > > > > In the meantime, if omitting 'data-lanes' for one sensor(transmitter) > > > > > > that has only one physical data lane, MIPI receiver(e.g., MIPI CSI-2) > > > > > > shall enable four-lane configuration, which may increase consumption of > > > > > > both power and resource in the process of IIC communication. > > > > > > > > > > Wouldn't the receiver still have the data-lanes property under its > > > > > endpoint node, telling it how many lanes and in which order should be > > > > > used? > > > > > > > > > > > > > The MIPI receiver(RX) shall use > > > > v4l2_async_notifier_add_fwnode_remote_subdev() API to parse the property > > > > "data-lanes" under sensor output port. > > > > > > That's not true. The MIPI receiver driver parses its own port node > > > corresponding to the sensor. Also quoting the documentation [1]: > > > > > > "An endpoint subnode of a device contains all properties needed for > > > _configuration of this device_ for data exchange with other device. In most > > > cases properties at the peer 'endpoint' nodes will be identical, however they > > > might need to be different when there is any signal modifications on the bus > > > between two devices, e.g. there are logic signal inverters on the lines." > > > > > > In this case, there is such a signal modification if the sensor has a > > > 1-lane bus and the receiver more lines, so the data-lanes properties > > > would be different on both sides. > > > > > > [1] https://elixir.bootlin.com/linux/v5.7/source/Documentation/devicetree/bindings/media/video-interfaces.txt > > > > > > > Sorry for the misunderstanding. > > After doing some experiments about the data-lanes property under sensor > > i2c node, we found the API > > v4l2_async_notifier_add_fwnode_remote_subdev() that MIPI receiver driver > > used indeed parses the data-lanes under its own port node. > > > > Sorry make a mistake for the use case of sensor data-lanes previously. > > Now We may encounter one new question for this patch. > > In practice we haven't used the data-lanes under sensor i2c node > > anywhere, if sensor driver itself doesn't parse that. > > > > But there is still one reason to keep the exactly right data-lanes in > > DT. That is, the data-lanes under sensor i2c node could be used as a > > reference for MIPI receiver driver. > > Just as Tomasz said, 'The MIPI receiver driver parses its own port node > > corresponding to the sensor'. > > > > Sakari, Tomasz, what's your opinions about the present of data-lanes > > under sensor node or not? > > The receiver driver doesn't parse the properties in the sensor > (transmitter) device's endpoint. If that property provides no information > to the receiver, as is the case here, it should be omitted. > Understood. Fixed in next release. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [V9, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings 2020-05-29 13:43 ` Tomasz Figa 2020-06-01 2:33 ` Dongchun Zhu @ 2020-06-02 9:53 ` Sakari Ailus 1 sibling, 0 replies; 31+ messages in thread From: Sakari Ailus @ 2020-06-02 9:53 UTC (permalink / raw) To: Tomasz Figa Cc: Dongchun Zhu, Rob Herring, Linus Walleij, Bartosz Golaszewski, Mauro Carvalho Chehab, Andy Shevchenko, Mark Rutland, Nicolas Boichat, Matthias Brugger, Cao Bing Bu, srv_heupstream, moderated list:ARM/Mediatek SoC support, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo, Shengnan Wang (王圣男) On Fri, May 29, 2020 at 03:43:30PM +0200, Tomasz Figa wrote: > On Thu, May 28, 2020 at 10:06 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote: > > > > Hi Sakari, > > > > On Thu, 2020-05-28 at 10:23 +0300, Sakari Ailus wrote: > > > Hi Dongchun, > > > > > > On Thu, May 28, 2020 at 11:34:42AM +0800, Dongchun Zhu wrote: > > > > Hi Sakari, Rob, > > > > > > > > On Thu, 2020-05-28 at 00:16 +0300, Sakari Ailus wrote: > > > > > Hi Rob, Dongchun, > > > > > > > > > > On Wed, May 27, 2020 at 09:27:22AM -0600, Rob Herring wrote: > > > > > > > > > + properties: > > > > > > > > > + endpoint: > > > > > > > > > + type: object > > > > > > > > > + additionalProperties: false > > > > > > > > > + > > > > > > > > > + properties: > > > > > > > > > > > > > > Actually I wonder whether we need to declare 'clock-lanes' here? > > > > > > > > > > > > Yes, if you are using it. > > > > > > > > > > Dongchun, can you confirm the chip has a single data and a single clock > > > > > lane and that it does not support lane reordering? > > > > > > > > > > > > > From the datasheet, 'MIPI inside the OV02A10 provides one single > > > > uni-directional clock lane and one bi-directional data lane solution for > > > > communication links between components inside a mobile device. > > > > The data lane has full support for HS(uni-directional) and > > > > LP(bi-directional) data transfer mode.' > > > > > > > > The sensor doesn't support lane reordering, so 'clock-lanes' property > > > > would not be added in next release. > > > > > > > > > So if there's nothing to convey to the driver, also the data-lanes should > > > > > be removed IMO. > > > > > > > > > > > > > However, 'data-lanes' property may still be required. > > > > It is known that either data-lanes or clock-lanes is an array of > > > > physical data lane indexes. Position of an entry determines the logical > > > > lane number, while the value of an entry indicates physical lane, e.g., > > > > for 1-lane MIPI CSI-2 bus we could have "data-lanes = <1>;", assuming > > > > the clock lane is on hardware lane 0. > > > > > > > > As mentioned earlier, the OV02A10 sensor supports only 1C1D and does not > > > > support lane reordering, so here we shall use 'data-lanes = <1>' as > > > > there is only a clock lane for OV02A10. > > > > > > > > Reminder: > > > > If 'data-lanes' property is not present, the driver would assume > > > > four-lane operation. This means for one-lane or two-lane operation, this > > > > property must be present and set to the right physical lane indexes. > > > > If the hardware does not support lane reordering, monotonically > > > > incremented values shall be used from 0 or 1 onwards, depending on > > > > whether or not there is also a clock lane. > > > > > > How can the driver use four lanes, considering the device only supports a > > > single lane?? > > > > > > > I understood your meaning. > > If we omit the property 'data-lanes', the sensor should work still. > > But then what's the meaning of the existence of 'data-lanes'? > > If this property 'data-lanes' is always optional, then why dt-bindings > > provide the interface? > > > > In the meantime, if omitting 'data-lanes' for one sensor(transmitter) > > that has only one physical data lane, MIPI receiver(e.g., MIPI CSI-2) > > shall enable four-lane configuration, which may increase consumption of > > both power and resource in the process of IIC communication. > > Wouldn't the receiver still have the data-lanes property under its > endpoint node, telling it how many lanes and in which order should be > used? Yes. -- Sakari Ailus ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [V9, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings 2020-05-27 15:27 ` Rob Herring 2020-05-27 21:16 ` Sakari Ailus @ 2020-05-28 5:53 ` Dongchun Zhu 1 sibling, 0 replies; 31+ messages in thread From: Dongchun Zhu @ 2020-05-28 5:53 UTC (permalink / raw) To: Rob Herring Cc: Linus Walleij, Bartosz Golaszewski, Mauro Carvalho Chehab, Andy Shevchenko, Mark Rutland, Sakari Ailus, Nicolas Boichat, Tomasz Figa, Matthias Brugger, Cao Bing Bu, srv_heupstream, moderated list:ARM/Mediatek SoC support, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, Sj Huang, Linux Media Mailing List, devicetree, Louis Kuo, Shengnan Wang (王圣男), dongchun.zhu Hi Rob, On Wed, 2020-05-27 at 09:27 -0600, Rob Herring wrote: > On Wed, May 27, 2020 at 2:51 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote: > > > > Hi Rob, > > > > Thanks for the review. Please see my replies below. > > > > On Tue, 2020-05-26 at 12:28 -0600, Rob Herring wrote: > > > On Sat, May 23, 2020 at 04:41:02PM +0800, Dongchun Zhu wrote: > > > > Add DT bindings documentation for Omnivision OV02A10 image sensor. > > > > > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > > --- > > > > .../bindings/media/i2c/ovti,ov02a10.yaml | 172 +++++++++++++++++++++ > > > > MAINTAINERS | 7 + > > > > 2 files changed, 179 insertions(+) > > > > create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml > > > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml > > > > new file mode 100644 > > > > index 0000000..56f31b5 > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml > > > > @@ -0,0 +1,172 @@ > > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > > > +# Copyright (c) 2020 MediaTek Inc. > > > > +%YAML 1.2 > > > > +--- > > > > +$id: http://devicetree.org/schemas/media/i2c/ovti,ov02a10.yaml# > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > + > > > > +title: Omnivision OV02A10 CMOS Sensor Device Tree Bindings > > > > + > > > > +maintainers: > > > > + - Dongchun Zhu <dongchun.zhu@mediatek.com> > > > > + > > > > +description: |- > > > > + The Omnivision OV02A10 is a low-cost, high performance, 1/5-inch, 2 megapixel > > > > + image sensor, which is the latest production derived from Omnivision's CMOS > > > > + image sensor technology. Ihis chip supports high frame rate speeds up to 30fps > > > > + @ 1600x1200 (UXGA) resolution transferred over a 1-lane MIPI interface. The > > > > + sensor output is available via CSI-2 serial data output. > > > > + > > > > +properties: > > > > + compatible: > > > > + const: ovti,ov02a10 > > > > + > > > > + reg: > > > > + maxItems: 1 > > > > + > > > > + clocks: > > > > + items: > > > > + - description: top mux camtg clock > > > > + - description: divider clock > > > > + > > > > + clock-names: > > > > + items: > > > > + - const: eclk > > > > + - const: freq_mux > > > > + > > > > + clock-frequency: > > > > + description: > > > > + Frequency of the eclk clock in Hertz. > > > > + > > > > Rob, shall we use 'maxItems: 1' to constrain property: clock-frequency? > > No, because it is a scalar, not an array. > Got it. > > Or could we adopt 'clock-frequency: true' directly here? > > As-is is fine. > Understood. > > > > + dovdd-supply: > > > > + description: > > > > + Definition of the regulator used as Digital I/O voltage supply. > > > > + > > > > Shall we add 'maxItems: 1' here? > > No, supplies are always singular. > Fine. > > > > > > + avdd-supply: > > > > + description: > > > > + Definition of the regulator used as Analog voltage supply. > > > > + > > > > Ditto. > > > > > > + dvdd-supply: > > > > + description: > > > > + Definition of the regulator used as Digital core voltage supply. > > > > + > > > > Ditto. > > > > > > + powerdown-gpios: > > > > + description: > > > > + Must be the device tree identifier of the GPIO connected to the > > > > + PD_PAD pin. This pin is used to place the OV02A10 into Standby mode > > > > + or Shutdown mode. As the line is active low, it should be > > > > + marked GPIO_ACTIVE_LOW. > > > > > > Need to define how many GPIOs ('maxItems: 1') > > > > > > > It would be fixed like this in next release. > > powerdown-gpios: > > maxItems: 1 > > description: > > Must be the device tree identifier of the GPIO connected to the > > PD_PAD pin. This pin is used to place the OV02A10 into Standby mode > > or Shutdown mode. As the line is active low, it should be > > marked GPIO_ACTIVE_LOW. > > Tomasz, I don't know whether you noticed this description. Here I simply defined one necessary GPIO polarity in DT which driver settings need to follow. > > > > + > > > > + reset-gpios: > > > > + description: > > > > + Must be the device tree identifier of the GPIO connected to the > > > > + RST_PD pin. If specified, it will be asserted during driver probe. > > > > + As the line is active high, it should be marked GPIO_ACTIVE_HIGH. > > > > > > Here too. > > > > > > > Similar as 'powerdown-gpios'. > > Fixed in next release. > > > > > > + > > > > + rotation: > > > > + description: > > > > + Definition of the sensor's placement. > > > > + allOf: > > > > + - $ref: "/schemas/types.yaml#/definitions/uint32" > > > > + - enum: > > > > + - 0 # Sensor Mounted Upright > > > > + - 180 # Sensor Mounted Upside Down > > > > + default: 0 > > > > + > > > > + ovti,mipi-tx-speed: > > > > + description: > > > > + Indication of MIPI transmission speed select, which is to control D-PHY > > > > + timing setting by adjusting MIPI clock voltage to improve the clock > > > > + driver capability. > > > > + allOf: > > > > + - $ref: "/schemas/types.yaml#/definitions/uint32" > > > > + - enum: > > > > + - 0 # 20MHz - 30MHz > > > > + - 1 # 30MHz - 50MHz > > > > + - 2 # 50MHz - 75MHz > > > > + - 3 # 75MHz - 100MHz > > > > + - 4 # 100MHz - 130MHz > > > > + default: 3 > > > > + > > > > + # See ../video-interfaces.txt for details > > > > + port: > > > > + type: object > > > > + additionalProperties: false > > > > > > Should have a description of what data the port has. > > > > > > > It would be updated as below in next release. > > port: > > type: object > > additionalProperties: false > > description: > > Input port node, single endpoint describing the CSI-2 transmitter. > > Output? > Sorry for the typo. Yes, this should be output port node. > > > > > > + > > > > + properties: > > > > + endpoint: > > > > + type: object > > > > + additionalProperties: false > > > > + > > > > + properties: > > > > Actually I wonder whether we need to declare 'clock-lanes' here? > > Yes, if you are using it. > Looking back to the upstreamed patches, it seems that there is a deep divide in the setting of 'clock-lanes' property. As the last comment I just posed, for OV02A10, 'clock-lanes' should be set to <0> (clock lane on hardware lane 0). But here we may follow IMX219 patch, which removed 'clock-lanes' property due to the fact that sensor hardware does not support lane reordering. Rob, Sakari, what's your opinions? > Rob ^ permalink raw reply [flat|nested] 31+ messages in thread
* [V9, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver 2020-05-23 8:41 [V9, 0/2] media: i2c: Add support for OV02A10 sensor Dongchun Zhu 2020-05-23 8:41 ` [V9, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings Dongchun Zhu @ 2020-05-23 8:41 ` Dongchun Zhu 2020-06-04 2:14 ` Dongchun Zhu 2020-06-10 19:44 ` Tomasz Figa 1 sibling, 2 replies; 31+ messages in thread From: Dongchun Zhu @ 2020-05-23 8:41 UTC (permalink / raw) To: linus.walleij, bgolaszewski, mchehab, andriy.shevchenko, robh+dt, mark.rutland, sakari.ailus, drinkcat, tfiga, matthias.bgg, bingbu.cao Cc: srv_heupstream, linux-mediatek, linux-arm-kernel, sj.huang, linux-media, devicetree, louis.kuo, shengnan.wang, dongchun.zhu Add a V4L2 sub-device driver for OV02A10 image sensor. Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> --- MAINTAINERS | 1 + drivers/media/i2c/Kconfig | 13 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 1040 insertions(+) create mode 100644 drivers/media/i2c/ov02a10.c diff --git a/MAINTAINERS b/MAINTAINERS index 63a2335..e7677c5 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12395,6 +12395,7 @@ L: linux-media@vger.kernel.org S: Maintained T: git git://linuxtv.org/media_tree.git F: Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml +F: drivers/media/i2c/ov02a10.c OMNIVISION OV13858 SENSOR DRIVER M: Sakari Ailus <sakari.ailus@linux.intel.com> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index 125d596..d8572cd 100644 --- a/drivers/media/i2c/Kconfig +++ b/drivers/media/i2c/Kconfig @@ -655,6 +655,19 @@ 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 + select MEDIA_CONTROLLER + select 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 77bf7d0..6566dd9 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..160a0b5 --- /dev/null +++ b/drivers/media/i2c/ov02a10.c @@ -0,0 +1,1025 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (c) 2020 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 + +/* Bit[1] vertical upside down */ +/* Bit[0] horizontal mirror */ +#define REG_MIRROR_FLIP_CONTROL 0x3f + +/* Orientation */ +#define REG_MIRROR_FLIP_ENABLE 0x03 + +/* Bit[2:0] MIPI transmission speed select */ +#define TX_SPEED_AREA_SEL 0xa1 +#define OV02A10_MIPI_TX_SPEED_DEFAULT 0x03 + +#define REG_PAGE_SWITCH 0xfd +#define REG_GLOBAL_EFFECTIVE 0x01 +#define REG_ENABLE BIT(0) + +#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_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 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 */ +}; + +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[ARRAY_SIZE(ov02a10_supply_names)]; + + bool streaming; + bool upside_down; + + /* + * Serialize control access, get/set format, get selection + * and start streaming. + */ + struct mutex mutex; + struct v4l2_subdev subdev; + struct media_pad pad; + struct v4l2_mbus_framefmt fmt; + struct v4l2_ctrl_handler ctrl_handler; + struct v4l2_ctrl *exposure; + + 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 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 = (chip_id_h << 8) | chip_id_l; + if (id != CHIP_ID) { + dev_err(&client->dev, "Unexpected sensor id(%04x)\n", id); + return -EINVAL; + } + + return 0; +} + +static int 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, 0); + + ret = clk_prepare_enable(ov02a10->eclk); + if (ret < 0) { + dev_err(dev, "failed to enable eclk\n"); + return ret; + } + + ret = regulator_bulk_enable(ARRAY_SIZE(ov02a10_supply_names), + 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, 1); + 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(ARRAY_SIZE(ov02a10_supply_names), + ov02a10->supplies); +disable_clk: + clk_disable_unprepare(ov02a10->eclk); + + return ret; +} + +static int 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, 0); + regulator_bulk_disable(ARRAY_SIZE(ov02a10_supply_names), + 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 mipi TX speed according to DT property */ + if (ov02a10->mipi_clock_tx_speed != OV02A10_MIPI_TX_SPEED_DEFAULT) { + 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; + + 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); + if (ret < 0) + return ret; + + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_EXPOSURE_L, val); + 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); + 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); + u32 vts = val + ov02a10->cur_mode->height - OV02A10_BASIC_LINE; + 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, + vts >> OV02A10_VTS_SHIFT); + if (ret < 0) + return ret; + + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_L, vts); + 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; + + 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; + 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); + + 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); + + v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops, + V4L2_CID_ANALOGUE_GAIN, + OV02A10_GAIN_MIN, + OV02A10_GAIN_MAX, + OV02A10_GAIN_STEP, + OV02A10_GAIN_DEFAULT); + + 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 -EINVAL; + + 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; + } + + 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->mipi_clock_tx_speed = OV02A10_MIPI_TX_SPEED_DEFAULT; + 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 && rotation == 180) { + ov02a10->upside_down = true; + ov02a10->fmt.code = MEDIA_BUS_FMT_SRGGB10_1X10; + } + + /* 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; + + /* 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 < ARRAY_SIZE(ov02a10_supply_names); i++) + ov02a10->supplies[i].supply = ov02a10_supply_names[i]; + + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ov02a10_supply_names), + 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; + } + + 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_free_handler; + } + } + + ret = v4l2_async_register_subdev(&ov02a10->subdev); + if (ret) { + dev_err(dev, "failed to register V4L2 subdev: %d", ret); + if (!pm_runtime_enabled(dev)) + ov02a10_power_off(dev); + 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 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [V9, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver 2020-05-23 8:41 ` [V9, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver Dongchun Zhu @ 2020-06-04 2:14 ` Dongchun Zhu 2020-06-04 9:26 ` Sakari Ailus 2020-06-10 19:44 ` Tomasz Figa 1 sibling, 1 reply; 31+ messages in thread From: Dongchun Zhu @ 2020-06-04 2:14 UTC (permalink / raw) To: linus.walleij Cc: bgolaszewski, mchehab, andriy.shevchenko, robh+dt, mark.rutland, sakari.ailus, drinkcat, tfiga, matthias.bgg, bingbu.cao, srv_heupstream, linux-mediatek, linux-arm-kernel, sj.huang, linux-media, devicetree, louis.kuo, shengnan.wang, dongchun.zhu Hi Tomasz, Sakari, and sirs, Could anyone help to review this patch? On Sat, 2020-05-23 at 16:41 +0800, Dongchun Zhu wrote: > Add a V4L2 sub-device driver for OV02A10 image sensor. > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > --- > MAINTAINERS | 1 + > drivers/media/i2c/Kconfig | 13 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 1040 insertions(+) > create mode 100644 drivers/media/i2c/ov02a10.c > [snip] > +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->mipi_clock_tx_speed = OV02A10_MIPI_TX_SPEED_DEFAULT; > + 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 && rotation == 180) { > + ov02a10->upside_down = true; > + ov02a10->fmt.code = MEDIA_BUS_FMT_SRGGB10_1X10; > + } > + > + /* 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; > + > + /* 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 < ARRAY_SIZE(ov02a10_supply_names); i++) > + ov02a10->supplies[i].supply = ov02a10_supply_names[i]; > + > + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ov02a10_supply_names), > + 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; > + } > + > + 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_free_handler; > + } > + } > + > + ret = v4l2_async_register_subdev(&ov02a10->subdev); > + if (ret) { > + dev_err(dev, "failed to register V4L2 subdev: %d", ret); > + if (!pm_runtime_enabled(dev)) > + ov02a10_power_off(dev); > + goto err_clean_entity; > + } Tomasz, Sakari, is this ok? or coding like this: ret = v4l2_async_register_subdev(&ov02a10->subdev); if (!pm_runtime_enabled(dev)) ov02a10_power_off(dev); if (ret) { dev_err(dev, "failed to register V4L2 subdev: %d", ret); goto err_clean_entity; } What's your opinions about the change? > + > + 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"); > + ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [V9, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver 2020-06-04 2:14 ` Dongchun Zhu @ 2020-06-04 9:26 ` Sakari Ailus 2020-06-04 18:05 ` Tomasz Figa 2020-06-05 3:19 ` Dongchun Zhu 0 siblings, 2 replies; 31+ messages in thread From: Sakari Ailus @ 2020-06-04 9:26 UTC (permalink / raw) To: Dongchun Zhu Cc: linus.walleij, bgolaszewski, mchehab, andriy.shevchenko, robh+dt, mark.rutland, drinkcat, tfiga, matthias.bgg, bingbu.cao, srv_heupstream, linux-mediatek, linux-arm-kernel, sj.huang, linux-media, devicetree, louis.kuo, shengnan.wang Hi Dongchun, On Thu, Jun 04, 2020 at 10:14:05AM +0800, Dongchun Zhu wrote: > Hi Tomasz, Sakari, and sirs, > > Could anyone help to review this patch? > > On Sat, 2020-05-23 at 16:41 +0800, Dongchun Zhu wrote: > > Add a V4L2 sub-device driver for OV02A10 image sensor. > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > --- > > MAINTAINERS | 1 + > > drivers/media/i2c/Kconfig | 13 + > > drivers/media/i2c/Makefile | 1 + > > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 1040 insertions(+) > > create mode 100644 drivers/media/i2c/ov02a10.c > > > > [snip] > > > +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->mipi_clock_tx_speed = OV02A10_MIPI_TX_SPEED_DEFAULT; > > + 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 && rotation == 180) { > > + ov02a10->upside_down = true; > > + ov02a10->fmt.code = MEDIA_BUS_FMT_SRGGB10_1X10; > > + } > > + > > + /* 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; > > + > > + /* 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 < ARRAY_SIZE(ov02a10_supply_names); i++) > > + ov02a10->supplies[i].supply = ov02a10_supply_names[i]; > > + > > + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ov02a10_supply_names), > > + 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; > > + } > > + > > + 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_free_handler; > > + } > > + } > > + > > + ret = v4l2_async_register_subdev(&ov02a10->subdev); > > + if (ret) { > > + dev_err(dev, "failed to register V4L2 subdev: %d", ret); > > + if (!pm_runtime_enabled(dev)) > > + ov02a10_power_off(dev); This should be moved to error handling section below. > > + goto err_clean_entity; > > + } > > Tomasz, Sakari, is this ok? > or coding like this: > > ret = v4l2_async_register_subdev(&ov02a10->subdev); > if (!pm_runtime_enabled(dev)) > ov02a10_power_off(dev); > if (ret) { > dev_err(dev, "failed to register V4L2 subdev: %d", ret); > goto err_clean_entity; > } > > What's your opinions about the change? This turns power off if runtime PM is disabled. I'd keep it as-is, as it'd require re-implementing what runtime PM is used for now --- and that's not a sensor driver's job. > > > + > > + 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"); > > + > -- Sakari Ailus ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [V9, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver 2020-06-04 9:26 ` Sakari Ailus @ 2020-06-04 18:05 ` Tomasz Figa 2020-06-05 3:19 ` Dongchun Zhu 1 sibling, 0 replies; 31+ messages in thread From: Tomasz Figa @ 2020-06-04 18:05 UTC (permalink / raw) To: Sakari Ailus Cc: Dongchun Zhu, Linus Walleij, Bartosz Golaszewski, Mauro Carvalho Chehab, Andy Shevchenko, Rob Herring, Mark Rutland, Nicolas Boichat, Matthias Brugger, Cao Bing Bu, srv_heupstream, moderated list:ARM/Mediatek SoC support, list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>,, Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo, Shengnan Wang (王圣男) On Thu, Jun 4, 2020 at 11:26 AM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Dongchun, > > On Thu, Jun 04, 2020 at 10:14:05AM +0800, Dongchun Zhu wrote: > > Hi Tomasz, Sakari, and sirs, > > > > Could anyone help to review this patch? > > > > On Sat, 2020-05-23 at 16:41 +0800, Dongchun Zhu wrote: > > > Add a V4L2 sub-device driver for OV02A10 image sensor. > > > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > --- > > > MAINTAINERS | 1 + > > > drivers/media/i2c/Kconfig | 13 + > > > drivers/media/i2c/Makefile | 1 + > > > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > > > 4 files changed, 1040 insertions(+) > > > create mode 100644 drivers/media/i2c/ov02a10.c > > > > > > > [snip] > > > > > +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->mipi_clock_tx_speed = OV02A10_MIPI_TX_SPEED_DEFAULT; > > > + 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 && rotation == 180) { > > > + ov02a10->upside_down = true; > > > + ov02a10->fmt.code = MEDIA_BUS_FMT_SRGGB10_1X10; > > > + } > > > + > > > + /* 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; > > > + > > > + /* 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 < ARRAY_SIZE(ov02a10_supply_names); i++) > > > + ov02a10->supplies[i].supply = ov02a10_supply_names[i]; > > > + > > > + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ov02a10_supply_names), > > > + 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; > > > + } > > > + > > > + 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_free_handler; > > > + } > > > + } > > > + > > > + ret = v4l2_async_register_subdev(&ov02a10->subdev); > > > + if (ret) { > > > + dev_err(dev, "failed to register V4L2 subdev: %d", ret); > > > + if (!pm_runtime_enabled(dev)) > > > + ov02a10_power_off(dev); > > This should be moved to error handling section below. > > > > + goto err_clean_entity; > > > + } > > > > Tomasz, Sakari, is this ok? > > or coding like this: > > > > ret = v4l2_async_register_subdev(&ov02a10->subdev); > > if (!pm_runtime_enabled(dev)) > > ov02a10_power_off(dev); > > if (ret) { > > dev_err(dev, "failed to register V4L2 subdev: %d", ret); > > goto err_clean_entity; > > } > > > > What's your opinions about the change? > > This turns power off if runtime PM is disabled. I'd keep it as-is, as it'd > require re-implementing what runtime PM is used for now --- and that's not > a sensor driver's job. That and in general I believe the expectations are: - runtime PM enabled - powered on only when it has something to do - runtime PM disabled - powered on when the driver is bound (probe succeeded), powered off when the driver unbinds (remove or probe error) Best regards, Tomasz > > > > > > + > > > + 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"); > > > + > > > > -- > Sakari Ailus ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [V9, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver 2020-06-04 9:26 ` Sakari Ailus 2020-06-04 18:05 ` Tomasz Figa @ 2020-06-05 3:19 ` Dongchun Zhu 1 sibling, 0 replies; 31+ messages in thread From: Dongchun Zhu @ 2020-06-05 3:19 UTC (permalink / raw) To: Sakari Ailus Cc: linus.walleij, bgolaszewski, mchehab, andriy.shevchenko, robh+dt, mark.rutland, drinkcat, tfiga, matthias.bgg, bingbu.cao, srv_heupstream, linux-mediatek, linux-arm-kernel, sj.huang, linux-media, devicetree, louis.kuo, shengnan.wang Hi Sakari, On Thu, 2020-06-04 at 12:26 +0300, Sakari Ailus wrote: > Hi Dongchun, > > On Thu, Jun 04, 2020 at 10:14:05AM +0800, Dongchun Zhu wrote: > > Hi Tomasz, Sakari, and sirs, > > > > Could anyone help to review this patch? > > > > On Sat, 2020-05-23 at 16:41 +0800, Dongchun Zhu wrote: > > > Add a V4L2 sub-device driver for OV02A10 image sensor. > > > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > --- > > > MAINTAINERS | 1 + > > > drivers/media/i2c/Kconfig | 13 + > > > drivers/media/i2c/Makefile | 1 + > > > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > > > 4 files changed, 1040 insertions(+) > > > create mode 100644 drivers/media/i2c/ov02a10.c > > > > > > > [snip] > > > > > +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->mipi_clock_tx_speed = OV02A10_MIPI_TX_SPEED_DEFAULT; > > > + 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 && rotation == 180) { > > > + ov02a10->upside_down = true; > > > + ov02a10->fmt.code = MEDIA_BUS_FMT_SRGGB10_1X10; > > > + } > > > + > > > + /* 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; > > > + > > > + /* 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 < ARRAY_SIZE(ov02a10_supply_names); i++) > > > + ov02a10->supplies[i].supply = ov02a10_supply_names[i]; > > > + > > > + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ov02a10_supply_names), > > > + 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; > > > + } > > > + > > > + 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_free_handler; This is actually wrong, which should be replaced of "err_clean_entity". > > > + } > > > + } > > > + > > > + ret = v4l2_async_register_subdev(&ov02a10->subdev); > > > + if (ret) { > > > + dev_err(dev, "failed to register V4L2 subdev: %d", ret); > > > + if (!pm_runtime_enabled(dev)) > > > + ov02a10_power_off(dev); > > This should be moved to error handling section below. > Fine. It would be abstracted as "err_async_register" in next release. Something like: err_async_register: if (!pm_runtime_enabled(dev)) ov02a10_power_off(dev); err_clean_entity: media_entity_cleanup(&ov02a10->subdev.entity); ... > > > + goto err_clean_entity; > > > + } > > > > Tomasz, Sakari, is this ok? > > or coding like this: > > > > ret = v4l2_async_register_subdev(&ov02a10->subdev); > > if (!pm_runtime_enabled(dev)) > > ov02a10_power_off(dev); > > if (ret) { > > dev_err(dev, "failed to register V4L2 subdev: %d", ret); > > goto err_clean_entity; > > } > > > > What's your opinions about the change? > > This turns power off if runtime PM is disabled. I'd keep it as-is, as it'd > require re-implementing what runtime PM is used for now --- and that's not > a sensor driver's job. > > > > > > + > > > + 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"); > > > + > > > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [V9, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver 2020-05-23 8:41 ` [V9, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver Dongchun Zhu 2020-06-04 2:14 ` Dongchun Zhu @ 2020-06-10 19:44 ` Tomasz Figa 2020-06-11 9:53 ` Sakari Ailus 2020-06-12 10:46 ` Dongchun Zhu 1 sibling, 2 replies; 31+ messages in thread From: Tomasz Figa @ 2020-06-10 19:44 UTC (permalink / raw) To: Dongchun Zhu Cc: linus.walleij, bgolaszewski, mchehab, andriy.shevchenko, robh+dt, mark.rutland, sakari.ailus, drinkcat, matthias.bgg, bingbu.cao, srv_heupstream, linux-mediatek, linux-arm-kernel, sj.huang, linux-media, devicetree, louis.kuo, shengnan.wang Hi Dongchun, On Sat, May 23, 2020 at 04:41:03PM +0800, Dongchun Zhu wrote: > Add a V4L2 sub-device driver for OV02A10 image sensor. > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > --- > MAINTAINERS | 1 + > drivers/media/i2c/Kconfig | 13 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 1040 insertions(+) > create mode 100644 drivers/media/i2c/ov02a10.c > Thank you for the patch. Please see my comments inline. [snip] > diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c > new file mode 100644 > index 0000000..160a0b5 > --- /dev/null > +++ b/drivers/media/i2c/ov02a10.c [snip] > +static const char * const ov02a10_test_pattern_menu[] = { > + "Disabled", > + "Color Bar", nit: We should normalize this to one of the standard names. What is the pattern on this sensor? Is it perhaps "Eight Vertical Colour Bars"? > +}; [snip] > +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); > + Don't we need to handle the case when fmt->which is V4L2_SUBDEV_FORMAT_TRY, which is used for trying the format, but not applying it to the hardware? > + 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; Ditto. > + 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; Hmm, supported_modes[] doesn't seem to hold the information about mbus codes. Should this just perhaps be "!= 0"? > + > + code->code = ov02a10->fmt.code; > + > + return 0; > +} [snip] > +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; > +} > + I'm not familiar with this init_cfg operation and the documentation is very sparse about it. Sakari, is this a correct implementation? [snip] > +static int ov02a10_set_test_pattern(struct ov02a10 *ov02a10, int pattern) > +{ > + 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_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); Why is this needed? Does writing the test pattern register stop streaming? [snip] > +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; > + 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); > + > + 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); > + > + v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops, > + V4L2_CID_ANALOGUE_GAIN, > + OV02A10_GAIN_MIN, > + OV02A10_GAIN_MAX, > + OV02A10_GAIN_STEP, > + OV02A10_GAIN_DEFAULT); > + > + 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); > + I can see that we're missing some controls here now, VBLANK and HBLANK if I remember correctly. Even though read-only, some userspace need those to get information about how the sensor operates. > + 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; > +} [snip] > + 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_free_handler; > + } > + } > + > + ret = v4l2_async_register_subdev(&ov02a10->subdev); > + if (ret) { > + dev_err(dev, "failed to register V4L2 subdev: %d", ret); > + if (!pm_runtime_enabled(dev)) > + ov02a10_power_off(dev); Please don't mix inline and error-path error handling, as it makes it difficult to tell if it's correct. Please move this below the appropriate err label instead. > + goto err_clean_entity; > + } > + > + return 0; > + > +err_clean_entity: If one calls pm_runtime_enable() in the probe path, one needs to call pm_runtime_disable() on the error and remove paths. > + 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; > +} > + Best regards, Tomasz ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [V9, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver 2020-06-10 19:44 ` Tomasz Figa @ 2020-06-11 9:53 ` Sakari Ailus 2020-06-11 9:57 ` Tomasz Figa 2020-06-12 10:46 ` Dongchun Zhu 1 sibling, 1 reply; 31+ messages in thread From: Sakari Ailus @ 2020-06-11 9:53 UTC (permalink / raw) To: Tomasz Figa Cc: Dongchun Zhu, linus.walleij, bgolaszewski, mchehab, andriy.shevchenko, robh+dt, mark.rutland, drinkcat, matthias.bgg, bingbu.cao, srv_heupstream, linux-mediatek, linux-arm-kernel, sj.huang, linux-media, devicetree, louis.kuo, shengnan.wang Hi Tomasz, On Wed, Jun 10, 2020 at 07:44:55PM +0000, Tomasz Figa wrote: > Hi Dongchun, > > On Sat, May 23, 2020 at 04:41:03PM +0800, Dongchun Zhu wrote: > > Add a V4L2 sub-device driver for OV02A10 image sensor. > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > --- > > MAINTAINERS | 1 + > > drivers/media/i2c/Kconfig | 13 + > > drivers/media/i2c/Makefile | 1 + > > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 1040 insertions(+) > > create mode 100644 drivers/media/i2c/ov02a10.c > > > > Thank you for the patch. Please see my comments inline. > > [snip] > > diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c > > new file mode 100644 > > index 0000000..160a0b5 > > --- /dev/null > > +++ b/drivers/media/i2c/ov02a10.c > [snip] > > +static const char * const ov02a10_test_pattern_menu[] = { > > + "Disabled", > > + "Color Bar", > > nit: We should normalize this to one of the standard names. What is the > pattern on this sensor? Is it perhaps "Eight Vertical Colour Bars"? > > > +}; > [snip] > > +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); > > + > > > Don't we need to handle the case when fmt->which is V4L2_SUBDEV_FORMAT_TRY, > which is used for trying the format, but not applying it to the hardware? Yes. > > > + 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; > > Ditto. > > > + 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; > > Hmm, supported_modes[] doesn't seem to hold the information about mbus > codes. Should this just perhaps be "!= 0"? > > > + > > + code->code = ov02a10->fmt.code; > > + > > + return 0; > > +} > [snip] > > +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; > > +} > > + > > I'm not familiar with this init_cfg operation and the documentation is very > sparse about it. Sakari, is this a correct implementation? The purpose is to initialise a pad configuration (format and selection rectangles) to the device defaults. As there seem to be no selection rectangles, this seems fine to me. -- Regards, Sakari Ailus ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [V9, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver 2020-06-11 9:53 ` Sakari Ailus @ 2020-06-11 9:57 ` Tomasz Figa 2020-06-11 10:03 ` Sakari Ailus 0 siblings, 1 reply; 31+ messages in thread From: Tomasz Figa @ 2020-06-11 9:57 UTC (permalink / raw) To: Sakari Ailus Cc: Dongchun Zhu, Linus Walleij, Bartosz Golaszewski, Mauro Carvalho Chehab, Andy Shevchenko, Rob Herring, Mark Rutland, Nicolas Boichat, Matthias Brugger, Cao Bing Bu, srv_heupstream, moderated list:ARM/Mediatek SoC support, list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>,, Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo, Shengnan Wang (王圣男) On Thu, Jun 11, 2020 at 11:53 AM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Tomasz, > > On Wed, Jun 10, 2020 at 07:44:55PM +0000, Tomasz Figa wrote: > > Hi Dongchun, > > > > On Sat, May 23, 2020 at 04:41:03PM +0800, Dongchun Zhu wrote: > > > Add a V4L2 sub-device driver for OV02A10 image sensor. > > > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > --- > > > MAINTAINERS | 1 + > > > drivers/media/i2c/Kconfig | 13 + > > > drivers/media/i2c/Makefile | 1 + > > > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > > > 4 files changed, 1040 insertions(+) > > > create mode 100644 drivers/media/i2c/ov02a10.c > > > > > > > Thank you for the patch. Please see my comments inline. > > > > [snip] > > > diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c > > > new file mode 100644 > > > index 0000000..160a0b5 > > > --- /dev/null > > > +++ b/drivers/media/i2c/ov02a10.c > > [snip] > > > +static const char * const ov02a10_test_pattern_menu[] = { > > > + "Disabled", > > > + "Color Bar", > > > > nit: We should normalize this to one of the standard names. What is the > > pattern on this sensor? Is it perhaps "Eight Vertical Colour Bars"? > > > > > +}; > > [snip] > > > +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); > > > + > > > > > > Don't we need to handle the case when fmt->which is V4L2_SUBDEV_FORMAT_TRY, > > which is used for trying the format, but not applying it to the hardware? > > Yes. > > > > > > + 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; > > > > Ditto. > > > > > + 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; > > > > Hmm, supported_modes[] doesn't seem to hold the information about mbus > > codes. Should this just perhaps be "!= 0"? > > > > > + > > > + code->code = ov02a10->fmt.code; > > > + > > > + return 0; > > > +} > > [snip] > > > +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; > > > +} > > > + > > > > I'm not familiar with this init_cfg operation and the documentation is very > > sparse about it. Sakari, is this a correct implementation? > > The purpose is to initialise a pad configuration (format and selection > rectangles) to the device defaults. As there seem to be no selection > rectangles, this seems fine to me. Thanks. I traced the code and could only see one place where the callback is being called and that was with cfg != NULL. Still, the code above uses "cfg ?" as a check to determine whether TRY or ACTIVE should be passed to which. Is that also correct? Best regards, Tomasz ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [V9, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver 2020-06-11 9:57 ` Tomasz Figa @ 2020-06-11 10:03 ` Sakari Ailus 2020-06-12 11:01 ` Dongchun Zhu 0 siblings, 1 reply; 31+ messages in thread From: Sakari Ailus @ 2020-06-11 10:03 UTC (permalink / raw) To: Tomasz Figa Cc: Dongchun Zhu, Linus Walleij, Bartosz Golaszewski, Mauro Carvalho Chehab, Andy Shevchenko, Rob Herring, Mark Rutland, Nicolas Boichat, Matthias Brugger, Cao Bing Bu, srv_heupstream, moderated list:ARM/Mediatek SoC support, list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>,, Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo, Shengnan Wang (王圣男) On Thu, Jun 11, 2020 at 11:57:43AM +0200, Tomasz Figa wrote: > On Thu, Jun 11, 2020 at 11:53 AM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > > > Hi Tomasz, > > > > On Wed, Jun 10, 2020 at 07:44:55PM +0000, Tomasz Figa wrote: > > > Hi Dongchun, > > > > > > On Sat, May 23, 2020 at 04:41:03PM +0800, Dongchun Zhu wrote: > > > > Add a V4L2 sub-device driver for OV02A10 image sensor. > > > > > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > > --- > > > > MAINTAINERS | 1 + > > > > drivers/media/i2c/Kconfig | 13 + > > > > drivers/media/i2c/Makefile | 1 + > > > > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > > > > 4 files changed, 1040 insertions(+) > > > > create mode 100644 drivers/media/i2c/ov02a10.c > > > > > > > > > > Thank you for the patch. Please see my comments inline. > > > > > > [snip] > > > > diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c > > > > new file mode 100644 > > > > index 0000000..160a0b5 > > > > --- /dev/null > > > > +++ b/drivers/media/i2c/ov02a10.c > > > [snip] > > > > +static const char * const ov02a10_test_pattern_menu[] = { > > > > + "Disabled", > > > > + "Color Bar", > > > > > > nit: We should normalize this to one of the standard names. What is the > > > pattern on this sensor? Is it perhaps "Eight Vertical Colour Bars"? > > > > > > > +}; > > > [snip] > > > > +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); > > > > + > > > > > > > > > Don't we need to handle the case when fmt->which is V4L2_SUBDEV_FORMAT_TRY, > > > which is used for trying the format, but not applying it to the hardware? > > > > Yes. > > > > > > > > > + 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; > > > > > > Ditto. > > > > > > > + 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; > > > > > > Hmm, supported_modes[] doesn't seem to hold the information about mbus > > > codes. Should this just perhaps be "!= 0"? > > > > > > > + > > > > + code->code = ov02a10->fmt.code; > > > > + > > > > + return 0; > > > > +} > > > [snip] > > > > +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; > > > > +} > > > > + > > > > > > I'm not familiar with this init_cfg operation and the documentation is very > > > sparse about it. Sakari, is this a correct implementation? > > > > The purpose is to initialise a pad configuration (format and selection > > rectangles) to the device defaults. As there seem to be no selection > > rectangles, this seems fine to me. > > Thanks. I traced the code and could only see one place where the > callback is being called and that was with cfg != NULL. Still, the > code above uses "cfg ?" as a check to determine whether TRY or ACTIVE > should be passed to which. Is that also correct? It could be used in setting the active format in probe. That would probably be cleaner than what it currently does. But apart from that, the framework always calls init_cfg with cfg non-NULL. -- Sakari Ailus ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [V9, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver 2020-06-11 10:03 ` Sakari Ailus @ 2020-06-12 11:01 ` Dongchun Zhu 0 siblings, 0 replies; 31+ messages in thread From: Dongchun Zhu @ 2020-06-12 11:01 UTC (permalink / raw) To: Sakari Ailus Cc: Tomasz Figa, Linus Walleij, Bartosz Golaszewski, Mauro Carvalho Chehab, Andy Shevchenko, Rob Herring, Mark Rutland, Nicolas Boichat, Matthias Brugger, Cao Bing Bu, srv_heupstream, moderated list:ARM/Mediatek SoC support, list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>,, Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo, Shengnan Wang (王圣男), dongchun.zhu Hi Sakari, On Thu, 2020-06-11 at 13:03 +0300, Sakari Ailus wrote: > On Thu, Jun 11, 2020 at 11:57:43AM +0200, Tomasz Figa wrote: > > On Thu, Jun 11, 2020 at 11:53 AM Sakari Ailus > > <sakari.ailus@linux.intel.com> wrote: > > > > > > Hi Tomasz, > > > > > > On Wed, Jun 10, 2020 at 07:44:55PM +0000, Tomasz Figa wrote: > > > > Hi Dongchun, > > > > > > > > On Sat, May 23, 2020 at 04:41:03PM +0800, Dongchun Zhu wrote: > > > > > Add a V4L2 sub-device driver for OV02A10 image sensor. > > > > > > > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > > > --- > > > > > MAINTAINERS | 1 + > > > > > drivers/media/i2c/Kconfig | 13 + > > > > > drivers/media/i2c/Makefile | 1 + > > > > > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > > > > > 4 files changed, 1040 insertions(+) > > > > > create mode 100644 drivers/media/i2c/ov02a10.c > > > > > > > > > > > > > Thank you for the patch. Please see my comments inline. > > > > > > > > [snip] > > > > > diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c > > > > > new file mode 100644 > > > > > index 0000000..160a0b5 > > > > > --- /dev/null > > > > > +++ b/drivers/media/i2c/ov02a10.c > > > > [snip] > > > > > +static const char * const ov02a10_test_pattern_menu[] = { > > > > > + "Disabled", > > > > > + "Color Bar", > > > > > > > > nit: We should normalize this to one of the standard names. What is the > > > > pattern on this sensor? Is it perhaps "Eight Vertical Colour Bars"? > > > > > > > > > +}; > > > > [snip] > > > > > +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); > > > > > + > > > > > > > > > > > > Don't we need to handle the case when fmt->which is V4L2_SUBDEV_FORMAT_TRY, > > > > which is used for trying the format, but not applying it to the hardware? > > > > > > Yes. > > > > > > > > > > > > + 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; > > > > > > > > Ditto. > > > > > > > > > + 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; > > > > > > > > Hmm, supported_modes[] doesn't seem to hold the information about mbus > > > > codes. Should this just perhaps be "!= 0"? > > > > > > > > > + > > > > > + code->code = ov02a10->fmt.code; > > > > > + > > > > > + return 0; > > > > > +} > > > > [snip] > > > > > +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; > > > > > +} > > > > > + > > > > > > > > I'm not familiar with this init_cfg operation and the documentation is very > > > > sparse about it. Sakari, is this a correct implementation? > > > > > > The purpose is to initialise a pad configuration (format and selection > > > rectangles) to the device defaults. As there seem to be no selection > > > rectangles, this seems fine to me. > > > > Thanks. I traced the code and could only see one place where the > > callback is being called and that was with cfg != NULL. Still, the > > code above uses "cfg ?" as a check to determine whether TRY or ACTIVE > > should be passed to which. Is that also correct? > > It could be used in setting the active format in probe. That would probably > be cleaner than what it currently does. > Sakari, did you mean that we need to update _probe API? Like this: struct v4l2_subdev_format fmt = { .which = V4L2_SUBDEV_FORMAT_ACTIVE, }; > But apart from that, the framework always calls init_cfg with cfg non-NULL. > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [V9, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver 2020-06-10 19:44 ` Tomasz Figa 2020-06-11 9:53 ` Sakari Ailus @ 2020-06-12 10:46 ` Dongchun Zhu 2020-06-12 18:39 ` Tomasz Figa 1 sibling, 1 reply; 31+ messages in thread From: Dongchun Zhu @ 2020-06-12 10:46 UTC (permalink / raw) To: Tomasz Figa Cc: linus.walleij, bgolaszewski, mchehab, andriy.shevchenko, robh+dt, mark.rutland, sakari.ailus, drinkcat, matthias.bgg, bingbu.cao, srv_heupstream, linux-mediatek, linux-arm-kernel, sj.huang, linux-media, devicetree, louis.kuo, shengnan.wang, dongchun.zhu Hi Tomasz, On Wed, 2020-06-10 at 19:44 +0000, Tomasz Figa wrote: > Hi Dongchun, > > On Sat, May 23, 2020 at 04:41:03PM +0800, Dongchun Zhu wrote: > > Add a V4L2 sub-device driver for OV02A10 image sensor. > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > --- > > MAINTAINERS | 1 + > > drivers/media/i2c/Kconfig | 13 + > > drivers/media/i2c/Makefile | 1 + > > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 1040 insertions(+) > > create mode 100644 drivers/media/i2c/ov02a10.c > > > > Thank you for the patch. Please see my comments inline. > > [snip] > > diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c > > new file mode 100644 > > index 0000000..160a0b5 > > --- /dev/null > > +++ b/drivers/media/i2c/ov02a10.c > [snip] > > +static const char * const ov02a10_test_pattern_menu[] = { > > + "Disabled", > > + "Color Bar", > > nit: We should normalize this to one of the standard names. What is the > pattern on this sensor? Is it perhaps "Eight Vertical Colour Bars"? > Yes. It is one kind of 'Eight Vertical Colour Bars'. This pattern is called as 'MIPI color bar' per the datasheet. Can we here use 'Vertical Color Bar' or 'MIPI Color Bar'? > > +}; > [snip] > > +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); > > + > > > Don't we need to handle the case when fmt->which is V4L2_SUBDEV_FORMAT_TRY, > which is used for trying the format, but not applying it to the hardware? > Got it :-) > > + 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; > > Ditto. > > > + 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; > > Hmm, supported_modes[] doesn't seem to hold the information about mbus > codes. Should this just perhaps be "!= 0"? > Understood. > > + > > + code->code = ov02a10->fmt.code; > > + > > + return 0; > > +} > [snip] > > +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; > > +} > > + > > I'm not familiar with this init_cfg operation and the documentation is very > sparse about it. Sakari, is this a correct implementation? > > [snip] > > +static int ov02a10_set_test_pattern(struct ov02a10 *ov02a10, int pattern) > > +{ > > + 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_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); > > Why is this needed? Does writing the test pattern register stop streaming? > Looking back to the setting history, I found it was suggested by OV. I would leave your question to OV, and update their feedback. > [snip] > > +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; > > + 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); > > + > > + 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); > > + > > + v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops, > > + V4L2_CID_ANALOGUE_GAIN, > > + OV02A10_GAIN_MIN, > > + OV02A10_GAIN_MAX, > > + OV02A10_GAIN_STEP, > > + OV02A10_GAIN_DEFAULT); > > + > > + 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); > > + > > I can see that we're missing some controls here now, VBLANK and HBLANK if I > remember correctly. Even though read-only, some userspace need those to > get information about how the sensor operates. > Yes. I made a mistake. > > + 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; > > +} > [snip] > > + 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_free_handler; > > + } > > + } > > + > > + ret = v4l2_async_register_subdev(&ov02a10->subdev); > > + if (ret) { > > + dev_err(dev, "failed to register V4L2 subdev: %d", ret); > > + if (!pm_runtime_enabled(dev)) > > + ov02a10_power_off(dev); > > Please don't mix inline and error-path error handling, as it makes it > difficult to tell if it's correct. Please move this below the appropriate > err label instead. > Fixed in next release. > > + goto err_clean_entity; > > + } > > + > > + return 0; > > + > > +err_clean_entity: > > If one calls pm_runtime_enable() in the probe path, one needs to call > pm_runtime_disable() on the error and remove paths. > Yes, fixed in next release. > > + 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; > > +} > > + > > Best regards, > Tomasz ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [V9, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver 2020-06-12 10:46 ` Dongchun Zhu @ 2020-06-12 18:39 ` Tomasz Figa 2020-06-15 5:54 ` Dongchun Zhu 0 siblings, 1 reply; 31+ messages in thread From: Tomasz Figa @ 2020-06-12 18:39 UTC (permalink / raw) To: Dongchun Zhu Cc: Linus Walleij, Bartosz Golaszewski, Mauro Carvalho Chehab, Andy Shevchenko, Rob Herring, Mark Rutland, Sakari Ailus, Nicolas Boichat, Matthias Brugger, Cao Bing Bu, srv_heupstream, moderated list:ARM/Mediatek SoC support, list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>,, Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo, Shengnan Wang (王圣男) On Fri, Jun 12, 2020 at 12:49 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote: > > Hi Tomasz, > > On Wed, 2020-06-10 at 19:44 +0000, Tomasz Figa wrote: > > Hi Dongchun, > > > > On Sat, May 23, 2020 at 04:41:03PM +0800, Dongchun Zhu wrote: > > > Add a V4L2 sub-device driver for OV02A10 image sensor. > > > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > --- > > > MAINTAINERS | 1 + > > > drivers/media/i2c/Kconfig | 13 + > > > drivers/media/i2c/Makefile | 1 + > > > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > > > 4 files changed, 1040 insertions(+) > > > create mode 100644 drivers/media/i2c/ov02a10.c > > > > > > > Thank you for the patch. Please see my comments inline. > > > > [snip] > > > diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c > > > new file mode 100644 > > > index 0000000..160a0b5 > > > --- /dev/null > > > +++ b/drivers/media/i2c/ov02a10.c > > [snip] > > > +static const char * const ov02a10_test_pattern_menu[] = { > > > + "Disabled", > > > + "Color Bar", > > > > nit: We should normalize this to one of the standard names. What is the > > pattern on this sensor? Is it perhaps "Eight Vertical Colour Bars"? > > > > Yes. It is one kind of 'Eight Vertical Colour Bars'. > This pattern is called as 'MIPI color bar' per the datasheet. > Can we here use 'Vertical Color Bar' or 'MIPI Color Bar'? > We should try to stick to the names as exposed by existing drivers. There was an attempt to unify the naming of some Sony sensors some time ago [1]. Perhaps one of the names there matches the pattern of this sensor? [1] https://patchwork.kernel.org/patch/10711777/ Best regards, Tomasz ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [V9, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver 2020-06-12 18:39 ` Tomasz Figa @ 2020-06-15 5:54 ` Dongchun Zhu 2020-06-15 12:44 ` Tomasz Figa 0 siblings, 1 reply; 31+ messages in thread From: Dongchun Zhu @ 2020-06-15 5:54 UTC (permalink / raw) To: Tomasz Figa Cc: Linus Walleij, Bartosz Golaszewski, Mauro Carvalho Chehab, Andy Shevchenko, Rob Herring, Mark Rutland, Sakari Ailus, Nicolas Boichat, Matthias Brugger, Cao Bing Bu, srv_heupstream, moderated list:ARM/Mediatek SoC support, list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>,, Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo, Shengnan Wang (王圣男), dongchun.zhu Hi Tomasz, On Fri, 2020-06-12 at 20:39 +0200, Tomasz Figa wrote: > On Fri, Jun 12, 2020 at 12:49 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote: > > > > Hi Tomasz, > > > > On Wed, 2020-06-10 at 19:44 +0000, Tomasz Figa wrote: > > > Hi Dongchun, > > > > > > On Sat, May 23, 2020 at 04:41:03PM +0800, Dongchun Zhu wrote: > > > > Add a V4L2 sub-device driver for OV02A10 image sensor. > > > > > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > > --- > > > > MAINTAINERS | 1 + > > > > drivers/media/i2c/Kconfig | 13 + > > > > drivers/media/i2c/Makefile | 1 + > > > > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > > > > 4 files changed, 1040 insertions(+) > > > > create mode 100644 drivers/media/i2c/ov02a10.c > > > > > > > > > > Thank you for the patch. Please see my comments inline. > > > > > > [snip] > > > > diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c > > > > new file mode 100644 > > > > index 0000000..160a0b5 > > > > --- /dev/null > > > > +++ b/drivers/media/i2c/ov02a10.c > > > [snip] > > > > +static const char * const ov02a10_test_pattern_menu[] = { > > > > + "Disabled", > > > > + "Color Bar", > > > > > > nit: We should normalize this to one of the standard names. What is the > > > pattern on this sensor? Is it perhaps "Eight Vertical Colour Bars"? > > > > > > > Yes. It is one kind of 'Eight Vertical Colour Bars'. > > This pattern is called as 'MIPI color bar' per the datasheet. > > Can we here use 'Vertical Color Bar' or 'MIPI Color Bar'? > > > > We should try to stick to the names as exposed by existing drivers. > There was an attempt to unify the naming of some Sony sensors some > time ago [1]. Perhaps one of the names there matches the pattern of > this sensor? > > [1] https://patchwork.kernel.org/patch/10711777/ > Sounds great. It is one good idea to summarize test patterns from various sensors. But one question plaguing me is that it seems even for the same "Eight Vertical Colour Bars", different sensors may have different RGB color map. Moreover, definition standards of color bar style may differ among different sensor chip vendors. For instance, Sony often uses "Solid Color", "Color Bars With Fade to Grey", "PN9" to abstract test pattern output type; while OmniVision adopts color bar type 1, 2, 3, 4 or "MIPI Color Bar", "ISP Color Bar" instead. > Best regards, > Tomasz ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [V9, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver 2020-06-15 5:54 ` Dongchun Zhu @ 2020-06-15 12:44 ` Tomasz Figa 0 siblings, 0 replies; 31+ messages in thread From: Tomasz Figa @ 2020-06-15 12:44 UTC (permalink / raw) To: Dongchun Zhu, Sakari Ailus Cc: Linus Walleij, Bartosz Golaszewski, Mauro Carvalho Chehab, Andy Shevchenko, Rob Herring, Mark Rutland, Nicolas Boichat, Matthias Brugger, Cao Bing Bu, srv_heupstream, moderated list:ARM/Mediatek SoC support, list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>,, Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo, Shengnan Wang (王圣男) Hi Dongchun, On Mon, Jun 15, 2020 at 7:57 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote: > > Hi Tomasz, > > On Fri, 2020-06-12 at 20:39 +0200, Tomasz Figa wrote: > > On Fri, Jun 12, 2020 at 12:49 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote: > > > > > > Hi Tomasz, > > > > > > On Wed, 2020-06-10 at 19:44 +0000, Tomasz Figa wrote: > > > > Hi Dongchun, > > > > > > > > On Sat, May 23, 2020 at 04:41:03PM +0800, Dongchun Zhu wrote: > > > > > Add a V4L2 sub-device driver for OV02A10 image sensor. > > > > > > > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > > > --- > > > > > MAINTAINERS | 1 + > > > > > drivers/media/i2c/Kconfig | 13 + > > > > > drivers/media/i2c/Makefile | 1 + > > > > > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > > > > > 4 files changed, 1040 insertions(+) > > > > > create mode 100644 drivers/media/i2c/ov02a10.c > > > > > > > > > > > > > Thank you for the patch. Please see my comments inline. > > > > > > > > [snip] > > > > > diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c > > > > > new file mode 100644 > > > > > index 0000000..160a0b5 > > > > > --- /dev/null > > > > > +++ b/drivers/media/i2c/ov02a10.c > > > > [snip] > > > > > +static const char * const ov02a10_test_pattern_menu[] = { > > > > > + "Disabled", > > > > > + "Color Bar", > > > > > > > > nit: We should normalize this to one of the standard names. What is the > > > > pattern on this sensor? Is it perhaps "Eight Vertical Colour Bars"? > > > > > > > > > > Yes. It is one kind of 'Eight Vertical Colour Bars'. > > > This pattern is called as 'MIPI color bar' per the datasheet. > > > Can we here use 'Vertical Color Bar' or 'MIPI Color Bar'? > > > > > > > We should try to stick to the names as exposed by existing drivers. > > There was an attempt to unify the naming of some Sony sensors some > > time ago [1]. Perhaps one of the names there matches the pattern of > > this sensor? > > > > [1] https://patchwork.kernel.org/patch/10711777/ > > > > Sounds great. > It is one good idea to summarize test patterns from various sensors. > But one question plaguing me is that it seems even for the same "Eight > Vertical Colour Bars", different sensors may have different RGB color > map. > > Moreover, definition standards of color bar style may differ among > different sensor chip vendors. > For instance, Sony often uses "Solid Color", "Color Bars With Fade to > Grey", "PN9" to abstract test pattern output type; while OmniVision > adopts color bar type 1, 2, 3, 4 or "MIPI Color Bar", "ISP Color Bar" > instead. The test patterns are commonly very similar, just different vendors call them with different names. There is actually a specification called MIPI CCS [1], which standardizes many aspects of sensor programming, including test patterns. Most of the sensors don't really follow the entire specification yet, but often some aspects happen to match. In the copy of the specification that I have, the test patterns are described in section 10 (Test modes), page 203, with pictures on following pages. Perhaps we could standardize the names based on that, when the pattern matches? Sakari, what do you think? [1] http://resources.mipi.org/camera-command-set-v1-1-download Best regards, Tomasz ^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2020-06-15 12:45 UTC | newest] Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-23 8:41 [V9, 0/2] media: i2c: Add support for OV02A10 sensor Dongchun Zhu 2020-05-23 8:41 ` [V9, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings Dongchun Zhu 2020-05-26 18:28 ` Rob Herring 2020-05-27 8:49 ` Dongchun Zhu 2020-05-27 15:27 ` Rob Herring 2020-05-27 21:16 ` Sakari Ailus 2020-05-28 3:34 ` Dongchun Zhu 2020-05-28 7:23 ` Sakari Ailus 2020-05-28 8:04 ` Dongchun Zhu 2020-05-29 13:43 ` Tomasz Figa 2020-06-01 2:33 ` Dongchun Zhu 2020-06-01 18:18 ` Tomasz Figa 2020-06-02 6:15 ` Dongchun Zhu 2020-06-02 9:56 ` Sakari Ailus 2020-06-04 2:20 ` Dongchun Zhu 2020-06-02 9:53 ` Sakari Ailus 2020-05-28 5:53 ` Dongchun Zhu 2020-05-23 8:41 ` [V9, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver Dongchun Zhu 2020-06-04 2:14 ` Dongchun Zhu 2020-06-04 9:26 ` Sakari Ailus 2020-06-04 18:05 ` Tomasz Figa 2020-06-05 3:19 ` Dongchun Zhu 2020-06-10 19:44 ` Tomasz Figa 2020-06-11 9:53 ` Sakari Ailus 2020-06-11 9:57 ` Tomasz Figa 2020-06-11 10:03 ` Sakari Ailus 2020-06-12 11:01 ` Dongchun Zhu 2020-06-12 10:46 ` Dongchun Zhu 2020-06-12 18:39 ` Tomasz Figa 2020-06-15 5:54 ` Dongchun Zhu 2020-06-15 12:44 ` Tomasz Figa
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).