Hi Sakari, Pardon, have you noticed my rely below? Please let us know of any questions or suggestions you have. On Mon, 2020-07-20 at 10:30 +0800, Dongchun Zhu wrote: > Hi Sakari, Tomasz, > > Thanks for the review. > > On Thu, 2020-07-16 at 16:57 +0200, Tomasz Figa wrote: > > On Wed, Jul 15, 2020 at 4:01 PM Sakari Ailus > > <sakari.ailus@xxxxxxxxxxxxxxx> wrote: > > > > > > hi Dongchun, > > > > > > On Mon, May 11, 2020 at 07:41:05PM +0800, Dongchun Zhu wrote: > > > > Hi Sakari, > > > > > > > > On Mon, 2020-05-11 at 01:35 +0300, Sakari Ailus wrote: > > > > > Hi Dongchun, > > > > > > > > > > On Fri, May 08, 2020 at 02:51:25PM +0800, Dongchun Zhu wrote: > > > > > > Hi Sakari, Tomasz, > > > > > > > > > > > > On Thu, 2020-05-07 at 16:25 +0200, Tomasz Figa wrote: > > > > > > > On Thu, May 7, 2020 at 4:12 PM Sakari Ailus > > > > > > > <sakari.ailus@xxxxxxxxxxxxxxx> wrote: > > > > > > > > > > > > > > > > Hi Tomasz, Dongchun, > > > > > > > > > > > > > > > > On Thu, May 07, 2020 at 03:50:40PM +0200, Tomasz Figa wrote: > > > > > > > > > Hi Sakari and Dongchun, > > > > > > > > > > > > > > > > > > On Thu, May 7, 2020 at 3:00 PM Dongchun Zhu <dongchun.zhu@xxxxxxxxxxxx> wrote: > > > > > > > > > > > > > > > > > > > > Hi Sakari, > > > > > > > > > > > > > > > > > > > > Thanks for the review. > > > > > > > > > > > > > > > > > > > > On Wed, 2020-05-06 at 14:21 +0300, Sakari Ailus wrote: > > > > > > > > > > > Hi Dongchun, > > > > > > > > > > > > > > > > > > > > > > On Tue, May 05, 2020 at 10:17:18PM +0800, Dongchun Zhu wrote: > > > > > > > > > > > > Hi Sakari, > > > > > > > > > > > > > > > > > > > > > > > > Thanks for the review. > > > > > > > > > > > > > > > > > > > > > > > > On Tue, 2020-05-05 at 10:04 +0300, Sakari Ailus wrote: > > > > > > > > > > > > > Hi Dongchun, > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Apr 30, 2020 at 04:09:23PM +0800, Dongchun Zhu wrote: > > > > > > > > > > > > > > Add DT bindings documentation for Omnivision OV02A10 image sensor. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@xxxxxxxxxxxx> > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > .../bindings/media/i2c/ovti,ov02a10.yaml | 148 +++++++++++++++++++++ > > > > > > > > > > > > > > MAINTAINERS | 7 + > > > > > > > > > > > > > > 2 files changed, 155 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..2be4bd2 > > > > > > > > > > > > > > --- /dev/null > > > > > > > > > > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml > > > > > > > > > > > > > > @@ -0,0 +1,148 @@ > > > > > > > > > > > > > > +# 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@xxxxxxxxxxxx> > > > > > > > > > > > > > > + > > > > > > > > > > > > > > +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: devider 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 interface power supply. > > > > > > > > > > > > > > + > > > > > > > > > > > > > > + avdd-supply: > > > > > > > > > > > > > > + description: > > > > > > > > > > > > > > + Definition of the regulator used as analog power supply. > > > > > > > > > > > > > > + > > > > > > > > > > > > > > + dvdd-supply: > > > > > > > > > > > > > > + description: > > > > > > > > > > > > > > + Definition of the regulator used as digital power supply. > > > > > > > > > > > > > > + > > > > > > > > > > > > > > + powerdown-gpios: > > > > > > > > > > > > > > + description: > > > > > > > > > > > > > > + The phandle and specifier for the GPIO that controls sensor powerdown. > > > > > > > > > > > > > > + > > > > > > > > > > > > > > + reset-gpios: > > > > > > > > > > > > > > + description: > > > > > > > > > > > > > > + The phandle and specifier for the GPIO that controls sensor reset. > > > > > > > > > > > > > > + > > > > > > > > > > > > > > + rotation: > > > > > > > > > > > > > > + description: > > > > > > > > > > > > > > + Definition of the sensor's placement, valid values are 0 and 180. > > > > > > > > > > > > > > + allOf: > > > > > > > > > > > > > > + - $ref: "/schemas/types.yaml#/definitions/uint32" > > > > > > > > > > > > > > + - enum: > > > > > > > > > > > > > > + - 0 # Sensor Mounted Upright > > > > > > > > > > > > > > + - 180 # Sensor Mounted Upside Down > > > > > > > > > > > > > > + > > > > > > > > > > > > > > + ovti,mipi-tx-speed: > > > > > > > > > > > > > > + description: > > > > > > > > > > > > > > + Indication of MIPI transmission speed select. > > > > > > > > > > > > > > > > > > > > > > > > > > What exactly does this signify? And how do you come up with the number? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Apologies for not addressing this number clear. > > > > > > > > > > > > > > > > > > > > > > > > From the datasheet, P1:0xA1 register represents TX_SPEED_AREA_SEL with > > > > > > > > > > > > the default val: 0x03. > > > > > > > > > > > > The description of this RW register is as below: > > > > > > > > > > > > Bit[2:0]: MIPI transmission speed select. > > > > > > > > > > > > > > > > > > > > > > > > Thus the enum should be definited as [ 0, 1, 2, 3, 4, 5, 6, 7 ]. > > > > > > > > > > > > This would be fixed in next release. > > > > > > > > > > > > > > > > > > > > > > > > In the meantime, as the default val of P1:0xA1 is 0x03, we hope to keep > > > > > > > > > > > > that value if there is no setting for this private property in DT. > > > > > > > > > > > > The caller in driver would be updated like this in next release. > > > > > > > > > > > > if (ov02a10->mipi_clock_tx_speed) > > > > > > > > > > > > ret = i2c_smbus_write_byte_data(...,...); > > > > > > > > > > > > > > > > > > > > > > How did you pick the value in the example? And why do you believe it is > > > > > > > > > > > specific to a platform, and not e.g. a sensor mode? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > We look into P1:0XA1, one register that defines MIPI transmission speed > > > > > > > > > > select. > > > > > > > > > > From the datasheet, we can get the possible values that could be set to > > > > > > > > > > P1:0xA1. > > > > > > > > > > > > > > > > > > > > Actually this register is an independent of sensor mode, it is just > > > > > > > > > > included in sensor mode's register setting table. > > > > > > > > > > > > > > > > > > > > In addition, this private DT Property is created to fix the MIPI test > > > > > > > > > > failure. The register values are adjusted and verified from vendor to > > > > > > > > > > make sensor signal meet MIPI specification. > > > > > > > > > > > > > > > > > > > > > > > > > > > > In theory the value could depend on the mode, because different link > > > > > > > > > rate could impose different requirements for the physical interface. > > > > > > > > > In practice, we haven't seen any hardware that would require different > > > > > > > > > values for different modes. > > > > > > > > > > > > > > > > The mode (possibly in conjunction with other information available to the > > > > > > > > driver via V4L2 fwnode interface) precisely defines the parameters of the > > > > > > > > CSI-2 bus --- apart from the possible exception of the bus timing related > > > > > > > > parameters but this is not supported by the name of the parameter. > > > > > > > > > > > > > > > > Therefore I don't see how this parameter, which supposedly is used to > > > > > > > > determine the CSI-2 transmissions speed, could be board specific and thus > > > > > > > > belong to DT. > > > > > > > > > > > > > > According to the very imprecise information I have access to, it is > > > > > > > not about the CSI-2 bus itself, but rather some internal parameter of > > > > > > > the sensor's CSI interface. Unfortunately there isn't much information > > > > > > > on what this value exactly controls... > > > > > > > > > > > > > > Best regards, > > > > > > > Tomasz > > > > > > > > > > > > Just got some feedback from OV vendor about this parameter. > > > > > > > > > > > > P1:0xA1 is the register to control D-PHY timing setting based on bclk. > > > > > > It is to adjust the MIPI clock voltage to improve the clock drive > > > > > > capability, and has no affect on the transmission speed of MIPI data. > > > > > > > > > > > > From vendor's perspective, P1:0xA1 depends upon the length of FPC of > > > > > > camera module that used on the board. Considering the physical > > > > > > connections for MIPI signals to user-facing camera are very different > > > > > > between our 2 projects, it can be very difficult to find universal SI > > > > > > parameters for both projects. > > > > > > > > > > Are you using different values for this parameter on these two projects? > > > > > > > > > > > > > Yes. We're actually assigning two different values to this property. > > > > One is 0x03, the other is 0x04. > > > > > > > > > > > > > > > > Thus here we create one new DT property to separate these tuning in > > > > > > driver, to be more like project-specific. > > > > > > > > > > > > More details about the register is as below. > > > > > > P1:0xA1 val: 0x03 default > > > > > > Case: 0 20MHz-30MHz > > > > > > 1 30MHz-50MHz > > > > > > 2 50MHz-75MHz > > > > > > 3 75MHz-100MHz (default, old DB setting use) > > > > > > 4 100MHz-130MHz (suggested, new DB setting use) > > > > > > 5 Manual > > > > > > So the value in the example should be [ 0, 1, 2, 3, 4, 5 ]. > > > > > > > > > > > > Additionally, P1:0xA1 is recommended to be set as 0x04 in the newest DB > > > > > > setting. We would adjust the register in next release. > > > > > > > > > > Thank you for digging into the issue. > > > > > > > > > > Based on the above description, the parameter would depend on both the link > > > > > frequency and possibly also on wire length. I guess there's no harm from > > > > > using too strong drive, apart from perhaps power consumption? As in > > > > > principle this could be different for different sensor modes. Albeit I > > > > > don't remember seeing a sensor where such a parameter would have been > > > > > needed to be modified. > > > > > > > > > > > > > This may be related to something about sensor fine tuning. > > > > As OV vendor pointed out, the sensor chip provides such one property > > > > that user could adjust based on their specific project. > > > > Also, case 4 (0x04) setting is confirmed to have a little more power > > > > consumption than case 3 (0x03). > > > > > > Apologies for bringing back an old topic --- the driver supports just a > > > single mode, using a specific data rate. > > > > > > If another mode is added later on, will it continue to use the same value > > > for this? Based on the documentation, it seems that this is primarily > > > defined by the frequency of the bus, not by board design. Therefore putting > > > this to DT (and thus ignoring the frequency) appears wrong. > > > > I don't think this is exactly implied by the frequency of the bus. The > > values there are recommended for given frequency ranges, but there are > > real cases where depending on the board different values are needed. > > > > Sorry for coming late. > For the reg P1: 0xA1, I re-confirmed the setting of the param with OVT. > The replies are as follow. > 1. P1:0xA1 is one register to control D-PHY timing parameters based on > bclk. Its setting shall match the MIPI CSI-2 timing clock. > 2. For one another scenario, if MIPI pixel rate(link frequency) differs > between scenarios, the setting of this parameter may also be different. > 3. In one special case, we may also need to adjust the value, even for > the same scenario, such as the failure of a certain MIPI test. > > From my perspective, temporally we don't plan to have a different > scenario for OV02A10, as the current resolution(1600x1200) is near to > the lower limit of most smart mobile devices. > In the meantime, considering the difference of the physical connections > for MIPI signals to user-facing camera between our 2 projects, it seems > to be very difficult to find universal SI parameters for both projects. > So for this case, I wonder whether we could reserve this private > property to maintain such flexibility. > > > Best regards, > > Tomasz > >
