On 11/03/2022 17:05, Jacopo Mondi wrote: > Hi Krzysztof, > > On Thu, Mar 10, 2022 at 06:26:02PM +0100, Krzysztof Kozlowski wrote: >> 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 > > Yes, I was suggesting to replace clock-frequency with clocks, that > accepts a phandle. > > The thing is, the driver parses 'clock-frequency' > device_property_read_u32(&client->dev, "clock-frequency", &input_clk); > > which I assume comes from ACPI (as the driver was developed for an > ACPI platform). > > If in DTS we don't use it, I then need to > > #ifdef CONFIG_ACPI > > #elif defined CONFIG_OF > > #endif > > Which I would really like to avoid. > > Anyone with ACPI experience that knows where clock-frequency comes > from ? I would assume that ACPI simply does not support common clock framework, so it had to use clock-frequency. Several of such drivers were added by folks from Intel which use ACPI, not Devicetree. > >> 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"? > > xclk is the clock fed to the sensor, which which all its internal > clocks are generated, so it's indeed an 'external' clock. As I've > said, clock-frequency seems to be meant for clock providers, and > the image sensor is a clock consumer. Regardless whether clock-frequency stays or not, you need the clocks property in such case. > >> >>> >>>>> + >>>>> + 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. >> > > I recall that with a single description entry then maxItems: 1 was > assumed by the dt-schema validator, but I cannot find references to > any commit, so I'll add it. > >>> >>>>> + >>>>> + 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. >> > > I don't think enum is equivalent, as it specifies a set of valid values > a property can assume, but it does not support arrays. It is equivalent, just has to be used in equivalent way. > > https://json-schema.org/draft/2020-12/json-schema-validation.html#rfc.section.6.16.1.2. > > enum > The value of this keyword MUST be an array. This array SHOULD have > at least one element. Elements in the array SHOULD be unique. > > An instance validates successfully against this keyword if its value > is equal to one of the elements in this keyword's array value. > > In facts: That's not good usage. See for example: Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml Best regards, Krzysztof
