Hi Krzysztof On Fri, Mar 11, 2022 at 05:11:47PM +0100, Krzysztof Kozlowski wrote: > 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. > Yes, I will have to ifdef in the driver if no better alternatives > > > >> > >>> > >>>>> + > >>>>> + 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 Thanks, you're right. items: enum: [1, 2] validates correctly. Thanks for the suggestion! > > > Best regards, > Krzysztof
