| Message ID | 20200710101850.4604-2-dongchun.zhu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: i2c: Add support for OV02A10 sensor | expand |
Hi Dongchun, On Fri, Jul 10, 2020 at 12:19 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote: > > Add YAML device tree binding for OV02A10 CMOS image sensor, > and the relevant MAINTAINERS entries. > > Reviewed-by: Tomasz Figa <tfiga@chromium.org> > Reviewed-by: Rob Herring <robh@kernel.org> > 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..3a916cc > --- /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 Why do we have two clocks here? Looking at the example suggests that they may be the clocks of the SoC that the integration was done with. However, the binding must only define the aspects of the particular device, i.e. this sensor. I suppose we should only have "eclk" here and it should be described as "external clock for the sensor". > + > + clock-frequency: > + description: > + Frequency of the eclk clock in Hertz. nit: maybe Hz? > + > + 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 needs to be high for the powerdown mode > + to be active, it should be marked GPIO_ACTIVE_HIGH. > + 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 needs to be low for the reset to be active, it should be > + marked GPIO_ACTIVE_LOW. > + maxItems: 1 > + > + 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. The description says that the value adjusts "MIPI clock voltage". Should the property be renamed to "ovti,mipi-clock-voltage"? > + 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 > + I've discussed this on IRC with Sakari. It sounds like this works as is for us because the driver currently only supports 1 mode, always running the link at 390 MHz. This won't scale if one intends to add more modes, because DT can't be expected to be updated when the driver changes. The two are expected to be separate and backwards compatible. I think we could model this in DT as an array of <link speed, clock voltage> pairs. Similarly to the OPP bindings [1]. An example to have all link speeds up to 390 MHz use the value 4: ovti,mipi-clock-voltages = < /* KHz clock voltage unit */ 390000 4 >; if one wants to select different voltage for different link, they could do so as well. With the example below, the driver should configure "3" for link frequencies <= 150 MHz and "4" for > 150 MHz <= 390 MHz. Link frequencies > 390 MHz should be disallowed. ovti,mipi-clock-voltages = < /* KHz clock voltage unit */ 150000 3 390000 4 >; What do you think? [1] https://elixir.bootlin.com/linux/v5.9-rc3/source/Documentation/devicetree/bindings/opp/opp.txt Best regards, Tomasz
Hi Tomasz, Thanks for dressing this into words. On Mon, Aug 31, 2020 at 01:44:27PM +0200, Tomasz Figa wrote: > Hi Dongchun, > > On Fri, Jul 10, 2020 at 12:19 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote: > > > > Add YAML device tree binding for OV02A10 CMOS image sensor, > > and the relevant MAINTAINERS entries. > > > > Reviewed-by: Tomasz Figa <tfiga@chromium.org> > > Reviewed-by: Rob Herring <robh@kernel.org> > > 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..3a916cc > > --- /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 > > Why do we have two clocks here? Looking at the example suggests that > they may be the clocks of the SoC that the integration was done with. > However, the binding must only define the aspects of the particular > device, i.e. this sensor. > > I suppose we should only have "eclk" here and it should be described > as "external clock for the sensor". > > > + > > + clock-frequency: > > + description: > > + Frequency of the eclk clock in Hertz. > > nit: maybe Hz? > > > + > > + 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 needs to be high for the powerdown mode > > + to be active, it should be marked GPIO_ACTIVE_HIGH. > > + 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 needs to be low for the reset to be active, it should be > > + marked GPIO_ACTIVE_LOW. > > + maxItems: 1 > > + > > + 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. > > The description says that the value adjusts "MIPI clock voltage". > Should the property be renamed to "ovti,mipi-clock-voltage"? > > > + 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 > > + > > I've discussed this on IRC with Sakari. It sounds like this works as > is for us because the driver currently only supports 1 mode, always > running the link at 390 MHz. This won't scale if one intends to add > more modes, because DT can't be expected to be updated when the driver > changes. The two are expected to be separate and backwards compatible. > > I think we could model this in DT as an array of <link speed, clock > voltage> pairs. Similarly to the OPP bindings [1]. An example to have > all link speeds up to 390 MHz use the value 4: > > ovti,mipi-clock-voltages = < > /* KHz clock voltage unit */ > 390000 4 > >; > > if one wants to select different voltage for different link, they > could do so as well. With the example below, the driver should > configure "3" for link frequencies <= 150 MHz and "4" for > 150 MHz <= > 390 MHz. Link frequencies > 390 MHz should be disallowed. > > ovti,mipi-clock-voltages = < > /* KHz clock voltage unit */ > 150000 3 > 390000 4 > >; > > What do you think? One more note on my proposal: use the index of the link frequency to access the ovti,mipi-clock-voltages array. That should be more simple for the driver, as the driver is expected to use only link frequencies listed in DT bindings anyway.
Hi Tomasz, Sakari, Thanks for digging into the imperfections. On Mon, 2020-08-31 at 19:06 +0300, Sakari Ailus wrote: > Hi Tomasz, > > Thanks for dressing this into words. > > On Mon, Aug 31, 2020 at 01:44:27PM +0200, Tomasz Figa wrote: > > Hi Dongchun, > > > > On Fri, Jul 10, 2020 at 12:19 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote: > > > > > > Add YAML device tree binding for OV02A10 CMOS image sensor, > > > and the relevant MAINTAINERS entries. > > > > > > Reviewed-by: Tomasz Figa <tfiga@chromium.org> > > > Reviewed-by: Rob Herring <robh@kernel.org> > > > 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..3a916cc > > > --- /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 > > > > Why do we have two clocks here? Looking at the example suggests that > > they may be the clocks of the SoC that the integration was done with. > > However, the binding must only define the aspects of the particular > > device, i.e. this sensor. > > > > I suppose we should only have "eclk" here and it should be described > > as "external clock for the sensor". > > I cannot agree with you more, it would be fixed in next release. > > > + > > > + clock-frequency: > > > + description: > > > + Frequency of the eclk clock in Hertz. > > > > nit: maybe Hz? > > Both shall be okay, let's use Hz instead, which seems to be more simple. > > > + > > > + 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 needs to be high for the powerdown mode > > > + to be active, it should be marked GPIO_ACTIVE_HIGH. > > > + 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 needs to be low for the reset to be active, it should be > > > + marked GPIO_ACTIVE_LOW. > > > + maxItems: 1 > > > + > > > + 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. > > > > The description says that the value adjusts "MIPI clock voltage". > > Should the property be renamed to "ovti,mipi-clock-voltage"? > > Sure, mipi-clock-voltage seems more clear, perfect naming :-) > > > + 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 > > > + > > > > I've discussed this on IRC with Sakari. It sounds like this works as > > is for us because the driver currently only supports 1 mode, always > > running the link at 390 MHz. This won't scale if one intends to add > > more modes, because DT can't be expected to be updated when the driver > > changes. The two are expected to be separate and backwards compatible. > > > > I think we could model this in DT as an array of <link speed, clock > > voltage> pairs. Similarly to the OPP bindings [1]. An example to have > > all link speeds up to 390 MHz use the value 4: > > > > ovti,mipi-clock-voltages = < > > /* KHz clock voltage unit */ > > 390000 4 > > >; > > > > if one wants to select different voltage for different link, they > > could do so as well. With the example below, the driver should > > configure "3" for link frequencies <= 150 MHz and "4" for > 150 MHz <= > > 390 MHz. Link frequencies > 390 MHz should be disallowed. > > > > ovti,mipi-clock-voltages = < > > /* KHz clock voltage unit */ > > 150000 3 > > 390000 4 > > >; > > > > What do you think? > So now we define the MIPI clock voltage (unit) as one function of link frequency? For current sensor mode (1600x1200, 390MHz link frequency), we then shall always use the value 4 to configure the MIPI transmission speed selection (P1:0xA1). Am I correct in thinking that? > One more note on my proposal: use the index of the link frequency to access > the ovti,mipi-clock-voltages array. That should be more simple for the > driver, as the driver is expected to use only link frequencies listed in DT > bindings anyway. > Good idea! That's exactly what I mean. Just submitted one draft change to handle the new property in DT. @Sakari @Tomasz please help to review it. https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2387980/1/drivers/media/i2c/ov02a10.c#883
On Tue, Sep 01, 2020 at 10:35:37PM +0800, Dongchun Zhu wrote: > Hi Tomasz, Sakari, > > Thanks for digging into the imperfections. > > On Mon, 2020-08-31 at 19:06 +0300, Sakari Ailus wrote: > > Hi Tomasz, > > > > Thanks for dressing this into words. > > > > On Mon, Aug 31, 2020 at 01:44:27PM +0200, Tomasz Figa wrote: > > > Hi Dongchun, > > > > > > On Fri, Jul 10, 2020 at 12:19 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote: > > > > > > > > Add YAML device tree binding for OV02A10 CMOS image sensor, > > > > and the relevant MAINTAINERS entries. > > > > > > > > Reviewed-by: Tomasz Figa <tfiga@chromium.org> > > > > Reviewed-by: Rob Herring <robh@kernel.org> > > > > 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..3a916cc > > > > --- /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 > > > > > > Why do we have two clocks here? Looking at the example suggests that > > > they may be the clocks of the SoC that the integration was done with. > > > However, the binding must only define the aspects of the particular > > > device, i.e. this sensor. > > > > > > I suppose we should only have "eclk" here and it should be described > > > as "external clock for the sensor". > > > > > I cannot agree with you more, it would be fixed in next release. > > > > > + > > > > + clock-frequency: > > > > + description: > > > > + Frequency of the eclk clock in Hertz. > > > > > > nit: maybe Hz? > > > > > Both shall be okay, let's use Hz instead, which seems to be more simple. > > > > > + > > > > + 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 needs to be high for the powerdown mode > > > > + to be active, it should be marked GPIO_ACTIVE_HIGH. > > > > + 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 needs to be low for the reset to be active, it should be > > > > + marked GPIO_ACTIVE_LOW. > > > > + maxItems: 1 > > > > + > > > > + 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. > > > > > > The description says that the value adjusts "MIPI clock voltage". > > > Should the property be renamed to "ovti,mipi-clock-voltage"? > > > > > Sure, mipi-clock-voltage seems more clear, perfect naming :-) > > > > > + 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 > > > > + > > > > > > I've discussed this on IRC with Sakari. It sounds like this works as > > > is for us because the driver currently only supports 1 mode, always > > > running the link at 390 MHz. This won't scale if one intends to add > > > more modes, because DT can't be expected to be updated when the driver > > > changes. The two are expected to be separate and backwards compatible. > > > > > > I think we could model this in DT as an array of <link speed, clock > > > voltage> pairs. Similarly to the OPP bindings [1]. An example to have > > > all link speeds up to 390 MHz use the value 4: > > > > > > ovti,mipi-clock-voltages = < > > > /* KHz clock voltage unit */ > > > 390000 4 > > > >; > > > > > > if one wants to select different voltage for different link, they > > > could do so as well. With the example below, the driver should > > > configure "3" for link frequencies <= 150 MHz and "4" for > 150 MHz <= > > > 390 MHz. Link frequencies > 390 MHz should be disallowed. > > > > > > ovti,mipi-clock-voltages = < > > > /* KHz clock voltage unit */ > > > 150000 3 > > > 390000 4 > > > >; > > > > > > What do you think? > > > > So now we define the MIPI clock voltage (unit) as one function of link > frequency? > > For current sensor mode (1600x1200, 390MHz link frequency), we then > shall always use the value 4 to configure the MIPI transmission speed > selection (P1:0xA1). Am I correct in thinking that? > > > One more note on my proposal: use the index of the link frequency to access > > the ovti,mipi-clock-voltages array. That should be more simple for the > > driver, as the driver is expected to use only link frequencies listed in DT > > bindings anyway. > > > > Good idea! That's exactly what I mean. > Just submitted one draft change to handle the new property in DT. > @Sakari @Tomasz please help to review it. > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2387980/1/drivers/media/i2c/ov02a10.c#883 Please send it to the list.
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..3a916cc --- /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 needs to be high for the powerdown mode + to be active, it should be marked GPIO_ACTIVE_HIGH. + 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 needs to be low for the reset to be active, it should be + marked GPIO_ACTIVE_LOW. + maxItems: 1 + + 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 + description: + Output port node, single endpoint describing the CSI-2 transmitter. + + properties: + endpoint: + type: object + additionalProperties: false + + properties: + link-frequencies: true + remote-endpoint: true + + required: + - 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_HIGH>; + reset-gpios = <&pio 109 GPIO_ACTIVE_LOW>; + + port { + wcam_out: endpoint { + link-frequencies = /bits/ 64 <390000000>; + remote-endpoint = <&mipi_in_wcam>; + }; + }; + }; + }; + +... diff --git a/MAINTAINERS b/MAINTAINERS index 68f21d4..378c961 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12560,6 +12560,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