| Message ID | 20220310130829.96001-2-jacopo@jmondi.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [1/6] media: dt-bindings: i2c: Document ov5670 | expand |
| Context | Check | Description |
|---|---|---|
| robh/patch-applied | success | |
| robh/checkpatch | success | |
| robh/dtbs-check | success | |
| robh/dt-meta-schema | success | |
| robh/patch-applied | success | |
| robh/checkpatch | success | |
| robh/dtbs-check | success | |
| robh/dt-meta-schema | success |
On 10/03/2022 14:08, Jacopo Mondi wrote: > Provide the bindings documentation for Omnivision OV5670 image sensor. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > .../devicetree/bindings/media/i2c/ov5670.yaml | 93 +++++++++++++++++++ Add the file to maintainers entry. > 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. > @@ -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@jmondi.org> Please add also driver 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. > + > + pwdn-gpios: > + description: Reference to the GPIO connected to the PWDNB pin. Active low. maxItems > + > + 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 clock-lanes? > + > + clock-noncontinuous: true > + > +required: > + - compatible > + - reg > + - clock-frequency > + - port > + > +additionalProperties: false > + > +examples: > + - | > + i2c0 { > + #address-cells = <1>; > + #size-cells = <0>; > + > + ov5670: sensor@36 { > + compatible = "ovti,ov5670"; > + reg = <0x36>; > + > + clock-frequency=<19200000>; Missing spaces around '='. Best regards, Krzysztof
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@jmondi.org> > > --- > > .../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@jmondi.org> > > 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 > > + > > +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. > > + > > + 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. > > + > > + 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? 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 > > no clock-lanes? > clock lane is fixed on lane #0 afaict ` > > + > > + clock-noncontinuous: true > > + > > +required: > > + - compatible > > + - reg > > + - clock-frequency > > + - port > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + i2c0 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + ov5670: sensor@36 { > > + compatible = "ovti,ov5670"; > > + reg = <0x36>; > > + > > + clock-frequency=<19200000>; > > Missing spaces around '='. ouch, thanks for spotting Thanks j > > > > Best regards, > Krzysztof
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@jmondi.org> >>> --- >>> .../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@jmondi.org> >> >> 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
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@jmondi.org> > >>> --- > >>> .../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@jmondi.org> > >> > >> 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. No need for a MAINTAINERS entry as get_maintainers.pl will pick it up from here. Rob
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@jmondi.org> > >>> --- > >>> .../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@jmondi.org> > >> > >> 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 ? > 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. > > > > >>> + > >>> + 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. 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: --- a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml +++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml @@ -52,10 +52,7 @@ properties: properties: data-lanes: description: The sensor supports 1 or 2 data lanes operations. - minItems: 1 - maxItems: 2 - items: - maximum: 2 + enum: [1, 2] Results in: Documentation/devicetree/bindings/media/i2c/ov5670.example.dt.yaml: sensor@36: port:endpoint:data-lanes:0: [1, 2] is too long If instead --- a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml +++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml @@ -54,8 +54,7 @@ properties: description: The sensor supports 1 or 2 data lanes operations. minItems: 1 maxItems: 2 - items: - maximum: 2 + enum: [1, 2] I get Documentation/devicetree/bindings/media/i2c/ov5670.yaml: properties:port:properties:endpoint:properties:data-lanes: 'enum' should not be valid under {'enum': ['const', 'enum', 'exclusiveMaximum', 'exclusiveMinimum', 'minimum', 'maximum', 'multipleOf', 'pattern']} hint: Scalar and array keywords cannot be mixed from schema $id: http://devicetree.org/meta-schemas/keywords.yaml# Thanks j > > > > 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
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@jmondi.org> >>>>> --- >>>>> .../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@jmondi.org> >>>> >>>> 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
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@jmondi.org> > >>>>> --- > >>>>> .../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@jmondi.org> > >>>> > >>>> 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
On 11/03/2022 19:00, Jacopo Mondi wrote: > 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@jmondi.org> >>>>>>> --- >>>>>>> .../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@jmondi.org> >>>>>> >>>>>> 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 I do not see the need of ifdefs... BTW, imx258 has exactly that case - clock-frequency coming from ACPI world but not added to DT bindings. Best regards, Krzysztof
Hello, On Sat, Mar 12, 2022 at 11:30:55AM +0100, Krzysztof Kozlowski wrote: > On 11/03/2022 19:00, Jacopo Mondi wrote: > > On Fri, Mar 11, 2022 at 05:11:47PM +0100, Krzysztof Kozlowski wrote: > >> On 11/03/2022 17:05, Jacopo Mondi wrote: > >>> On Thu, Mar 10, 2022 at 06:26:02PM +0100, Krzysztof Kozlowski wrote: > >>>> On 10/03/2022 18:16, Jacopo Mondi wrote: > >>>>> 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@jmondi.org> > >>>>>>> --- > >>>>>>> .../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@jmondi.org> > >>>>>> > >>>>>> 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 > > I do not see the need of ifdefs... BTW, imx258 has exactly that case - > clock-frequency coming from ACPI world but not added to DT bindings. The driver can call clk_get_rate() when a clock is provided, and use the clock-frequency property otherwise. I also don't think conditional compilation is needed.
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 @@ -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@jmondi.org> + +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. + + pwdn-gpios: + description: Reference to the GPIO connected to the PWDNB pin. Active low. + + reset-gpios: + description: + Reference to the GPIO connected to the XSHUTDOWN pin. Active low. + + 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 + + clock-noncontinuous: true + +required: + - compatible + - reg + - clock-frequency + - port + +additionalProperties: false + +examples: + - | + i2c0 { + #address-cells = <1>; + #size-cells = <0>; + + ov5670: sensor@36 { + compatible = "ovti,ov5670"; + reg = <0x36>; + + clock-frequency=<19200000>; + + port { + endpoint { + remote-endpoint = <&csi_ep>; + data-lanes = <1 2>; + clock-noncontinuous; + }; + }; + }; + }; + +... +
Provide the bindings documentation for Omnivision OV5670 image sensor. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- .../devicetree/bindings/media/i2c/ov5670.yaml | 93 +++++++++++++++++++ 1 file changed, 93 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5670.yaml -- 2.35.1