On 10/03/2022 18:16, Jacopo Mondi wrote: > Hi Krzysztof > > On Thu, Mar 10, 2022 at 03:29:24PM +0100, Krzysztof Kozlowski wrote: >> On 10/03/2022 14:08, Jacopo Mondi wrote: >>> Provide the bindings documentation for Omnivision OV5670 image sensor. >>> >>> Signed-off-by: Jacopo Mondi <jacopo@xxxxxxxxxx> >>> --- >>> .../devicetree/bindings/media/i2c/ov5670.yaml | 93 +++++++++++++++++++ >> >> Add the file to maintainers entry. >> > > Right > >>> 1 file changed, 93 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5670.yaml >>> >>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml >>> new file mode 100644 >>> index 000000000000..dc4a3297bf6f >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml >> >> Missing vendor prefix in file name. >> > > Right x2 > >>> @@ -0,0 +1,93 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/media/i2c/ov5670.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Omnivision OV5670 5 Megapixels raw image sensor >>> + >>> +maintainers: >>> + - Jacopo Mondi <jacopo@xxxxxxxxxx> >> >> Please add also driver maintainer. >> > > I never got what the policy was, if the maintainer entries here only > refer to the binding file or to the driver too It is a person responsible for the bindings, so indeed it might not feed existing maintainer. > >>> + >>> +description: |- >>> + The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits >>> + RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is >>> + controlled through an I2C compatible control bus. >>> + >>> +properties: >>> + compatible: >>> + const: ovti,ov5670 >>> + >>> + reg: >>> + maxItems: 1 >>> + >>> + clock-frequency: >>> + description: Frequency of the xclk clock. >> >> Is the xclk external clock coming to the sensor? If yes, there should be >> a "clocks" property. >> > > To be honest I was not sure about this, as clock-frequency is already > used by the driver for the ACPI part, but it seems to in DT bindings > it is a property meant to be specified in the clock providers, even if > Documentation/devicetree/bindings/clock/clock-bindings.txt doesn't > really clarify this > > Clock consumer should rather use 'clocks' and point to the provider's > phandle if my understanding is right. This is a clock-frequency, not clock reference. For external clocks, a clock phandles + assigned-clock-rates should be rather used. However for internal clocks, this is a perfectly valid property. Therefore the question is - what is the "xclk"? > >>> + >>> + pwdn-gpios: >>> + description: Reference to the GPIO connected to the PWDNB pin. Active low. >> >> maxItems >> > > I thought it was not necessary with a single description: entry. But > looking at the dt-schema source I fail to find any commit mentioning > that. The purpose of maxItems is to constrain the number of GPIOs, so two would be incorrect. > >>> + >>> + reset-gpios: >>> + description: >>> + Reference to the GPIO connected to the XSHUTDOWN pin. Active low. >> >> maxItems >> >>> + >>> + avdd-supply: >>> + description: Analog circuit power. Typically 2.8V. >>> + >>> + dvdd-supply: >>> + description: Digital circuit power. Typically 1.2V. >>> + >>> + dovdd-supply: >>> + description: Digital I/O circuit power. Typically 2.8V or 1.8V. >>> + >>> + port: >>> + $ref: /schemas/graph.yaml#/$defs/port-base >>> + additionalProperties: false >>> + >>> + properties: >>> + endpoint: >>> + $ref: /schemas/media/video-interfaces.yaml# >>> + unevaluatedProperties: false >>> + >>> + properties: >>> + data-lanes: >>> + description: The sensor supports 1 or 2 data lanes operations. >>> + minItems: 1 >>> + maxItems: 2 >>> + items: >>> + maximum: 2 >> >> Is '0' also allowed? If not then maybe 'enum: [ 1, 2 ]' > > No 0 is not allowed, but the data-lanes properties should accept any > of the following combinations > <1> > <1 2> > <2 1> > > As the chip seems to support lane re-ordering. > > using enum would allow to between <1> or <2> if I got it right? Yeah, enum would be equivalent. I find it more readable, than min+max, but it's not a strong preference. > > as the data-lane property is defined in video-interfaces.yaml > > data-lanes: > $ref: /schemas/types.yaml#/definitions/uint32-array > minItems: 1 > maxItems: 8 > items: > # Assume up to 9 physical lane indices > maximum: 8 > description: > 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 2-lane MIPI CSI-2 bus we could have "data-lanes = <1 2>;", > assuming the clock lane is on hardware lane 0. 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. This property is valid for serial busses only (e.g. MIPI CSI-2). > > I did the same but restricted the max number of items to 2, and the > maximum value to 2 as well Makes sense, but you should also restrict the minimum (so not 0). enum solves this. > >> >> no clock-lanes? >> > > clock lane is fixed on lane #0 afaict ok Best regards, Krzysztof
