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