* [PATCH v2 1/8] media: dt-bindings: i2c: Document ov5670 [not found] <20220314162714.153970-1-jacopo@jmondi.org> @ 2022-03-14 16:27 ` Jacopo Mondi 2022-03-15 7:32 ` Krzysztof Kozlowski 0 siblings, 1 reply; 10+ messages in thread From: Jacopo Mondi @ 2022-03-14 16:27 UTC (permalink / raw) To: Chiranjeevi Rapolu Cc: Jacopo Mondi, krzysztof.kozlowski, jeanmichel.hautbois, laurent.pinchart, paul.kocialkowski, sakari.ailus, paul.elder, Mauro Carvalho Chehab, open list:OMNIVISION OV5670 SENSOR DRIVER, robh, devicetree Provide the bindings documentation for Omnivision OV5670 image sensor. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- v1->v2 (comments from Krzysztof) - Rename to include manufacturer name - Add entry to MAINTAINERS - Add maxItems: to -gpios properties - Use common clock properties - Use enum: [1, 2] for data lanes - Fix whitespace issue in example --- .../bindings/media/i2c/ovti,ov5670.yaml | 99 +++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 100 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml new file mode 100644 index 000000000000..73cf72203f17 --- /dev/null +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml @@ -0,0 +1,99 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/ovti,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 + + assigned-clocks: true + assigned-clock-parents: true + assigned-clock-rates: true + + clocks: + description: System clock. From 6 to 27 MHz. + maxItems: 1 + + pwdn-gpios: + description: Reference to the GPIO connected to the PWDNB pin. Active low. + maxItems: 1 + + reset-gpios: + description: + Reference to the GPIO connected to the XSHUTDOWN pin. Active low. + maxItems: 1 + + 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: + enum: [1, 2] + + clock-noncontinuous: true + +required: + - compatible + - reg + - clocks + - port + +additionalProperties: false + +examples: + - | + i2c0 { + #address-cells = <1>; + #size-cells = <0>; + + ov5670: sensor@36 { + compatible = "ovti,ov5670"; + reg = <0x36>; + clocks = <&sensor_xclk>; + + port { + endpoint { + remote-endpoint = <&csi_ep>; + data-lanes = <1 2>; + clock-noncontinuous; + }; + }; + }; + }; + +... + diff --git a/MAINTAINERS b/MAINTAINERS index 83d27b57016f..6aa32609a3cf 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14259,6 +14259,7 @@ M: Chiranjeevi Rapolu <chiranjeevi.rapolu@intel.com> L: linux-media@vger.kernel.org S: Maintained T: git git://linuxtv.org/media_tree.git +F: Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml F: drivers/media/i2c/ov5670.c OMNIVISION OV5675 SENSOR DRIVER -- 2.35.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/8] media: dt-bindings: i2c: Document ov5670 2022-03-14 16:27 ` [PATCH v2 1/8] media: dt-bindings: i2c: Document ov5670 Jacopo Mondi @ 2022-03-15 7:32 ` Krzysztof Kozlowski 2022-03-15 7:59 ` Sakari Ailus 2022-03-15 8:41 ` Jacopo Mondi 0 siblings, 2 replies; 10+ messages in thread From: Krzysztof Kozlowski @ 2022-03-15 7:32 UTC (permalink / raw) To: Jacopo Mondi, Chiranjeevi Rapolu Cc: jeanmichel.hautbois, laurent.pinchart, paul.kocialkowski, sakari.ailus, paul.elder, Mauro Carvalho Chehab, open list:OMNIVISION OV5670 SENSOR DRIVER, robh, devicetree On 14/03/2022 17:27, Jacopo Mondi wrote: > Provide the bindings documentation for Omnivision OV5670 image sensor. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > v1->v2 (comments from Krzysztof) > > - Rename to include manufacturer name > - Add entry to MAINTAINERS > - Add maxItems: to -gpios properties > - Use common clock properties > - Use enum: [1, 2] for data lanes > - Fix whitespace issue in example > --- > > .../bindings/media/i2c/ovti,ov5670.yaml | 99 +++++++++++++++++++ > MAINTAINERS | 1 + > 2 files changed, 100 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml > > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml > new file mode 100644 > index 000000000000..73cf72203f17 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml > @@ -0,0 +1,99 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/i2c/ovti,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 > + > + assigned-clocks: true > + assigned-clock-parents: true > + assigned-clock-rates: true You should not need these. These are coming with schema. You can add these to example schema below and double-check. > + > + clocks: > + description: System clock. From 6 to 27 MHz. > + maxItems: 1 > + > + pwdn-gpios: > + description: Reference to the GPIO connected to the PWDNB pin. Active low. This does not look like a standard property, so you need a vendor prefix. > + maxItems: 1 > + Best regards, Krzysztof ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/8] media: dt-bindings: i2c: Document ov5670 2022-03-15 7:32 ` Krzysztof Kozlowski @ 2022-03-15 7:59 ` Sakari Ailus 2022-03-15 8:03 ` Krzysztof Kozlowski 2022-03-15 8:09 ` Laurent Pinchart 2022-03-15 8:41 ` Jacopo Mondi 1 sibling, 2 replies; 10+ messages in thread From: Sakari Ailus @ 2022-03-15 7:59 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Jacopo Mondi, Chiranjeevi Rapolu, jeanmichel.hautbois, laurent.pinchart, paul.kocialkowski, paul.elder, Mauro Carvalho Chehab, open list:OMNIVISION OV5670 SENSOR DRIVER, robh, devicetree Hi Krzysztof, Jacopo, On Tue, Mar 15, 2022 at 08:32:58AM +0100, Krzysztof Kozlowski wrote: > On 14/03/2022 17:27, Jacopo Mondi wrote: > > Provide the bindings documentation for Omnivision OV5670 image sensor. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > v1->v2 (comments from Krzysztof) > > > > - Rename to include manufacturer name > > - Add entry to MAINTAINERS > > - Add maxItems: to -gpios properties > > - Use common clock properties > > - Use enum: [1, 2] for data lanes > > - Fix whitespace issue in example > > --- > > > > .../bindings/media/i2c/ovti,ov5670.yaml | 99 +++++++++++++++++++ > > MAINTAINERS | 1 + > > 2 files changed, 100 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml > > new file mode 100644 > > index 000000000000..73cf72203f17 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml > > @@ -0,0 +1,99 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/media/i2c/ovti,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 > > + > > + assigned-clocks: true > > + assigned-clock-parents: true > > + assigned-clock-rates: true > > You should not need these. These are coming with schema. You can add > these to example schema below and double-check. They should probably be required actually. > > > + > > + clocks: > > + description: System clock. From 6 to 27 MHz. > > + maxItems: 1 > > + > > + pwdn-gpios: > > + description: Reference to the GPIO connected to the PWDNB pin. Active low. > > This does not look like a standard property, so you need a vendor prefix. The similarly named property exists elsewhere. I wouldn't use a vendor prefix, also for the reason that the functionality is quite common. I guess alternative name would be possible, too --- "shutdown" seems to be more common. -- Regards, Sakari Ailus ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/8] media: dt-bindings: i2c: Document ov5670 2022-03-15 7:59 ` Sakari Ailus @ 2022-03-15 8:03 ` Krzysztof Kozlowski 2022-03-15 12:47 ` Sakari Ailus 2022-03-15 8:09 ` Laurent Pinchart 1 sibling, 1 reply; 10+ messages in thread From: Krzysztof Kozlowski @ 2022-03-15 8:03 UTC (permalink / raw) To: Sakari Ailus Cc: Jacopo Mondi, Chiranjeevi Rapolu, jeanmichel.hautbois, laurent.pinchart, paul.kocialkowski, paul.elder, Mauro Carvalho Chehab, open list:OMNIVISION OV5670 SENSOR DRIVER, robh, devicetree On 15/03/2022 08:59, Sakari Ailus wrote: > Hi Krzysztof, Jacopo, > > On Tue, Mar 15, 2022 at 08:32:58AM +0100, Krzysztof Kozlowski wrote: >> On 14/03/2022 17:27, Jacopo Mondi wrote: >>> Provide the bindings documentation for Omnivision OV5670 image sensor. >>> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >>> >>> --- >>> v1->v2 (comments from Krzysztof) >>> >>> - Rename to include manufacturer name >>> - Add entry to MAINTAINERS >>> - Add maxItems: to -gpios properties >>> - Use common clock properties >>> - Use enum: [1, 2] for data lanes >>> - Fix whitespace issue in example >>> --- >>> >>> .../bindings/media/i2c/ovti,ov5670.yaml | 99 +++++++++++++++++++ >>> MAINTAINERS | 1 + >>> 2 files changed, 100 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml >>> >>> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml >>> new file mode 100644 >>> index 000000000000..73cf72203f17 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml >>> @@ -0,0 +1,99 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/media/i2c/ovti,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 >>> + >>> + assigned-clocks: true >>> + assigned-clock-parents: true >>> + assigned-clock-rates: true >> >> You should not need these. These are coming with schema. You can add >> these to example schema below and double-check. > > They should probably be required actually. Why required? The hardware can work with different clocks, get their rate and configure internal PLLs/clocks to new value. Having it required might have sense for current implementation of driver but this is independent of bindings. Bindings do not describe driver, but hardware. >> >>> + >>> + clocks: >>> + description: System clock. From 6 to 27 MHz. >>> + maxItems: 1 >>> + >>> + pwdn-gpios: >>> + description: Reference to the GPIO connected to the PWDNB pin. Active low. >> >> This does not look like a standard property, so you need a vendor prefix. > > The similarly named property exists elsewhere. I wouldn't use a vendor > prefix, also for the reason that the functionality is quite common. I guess > alternative name would be possible, too --- "shutdown" seems to be more > common. It exists in three bindings, so it is not a standard property... although something closer to standard is "powerdown-gpios" so maybe just use that one? Best regards, Krzysztof ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/8] media: dt-bindings: i2c: Document ov5670 2022-03-15 8:03 ` Krzysztof Kozlowski @ 2022-03-15 12:47 ` Sakari Ailus 2022-03-15 12:57 ` Krzysztof Kozlowski 2022-03-15 13:01 ` Laurent Pinchart 0 siblings, 2 replies; 10+ messages in thread From: Sakari Ailus @ 2022-03-15 12:47 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Jacopo Mondi, Chiranjeevi Rapolu, jeanmichel.hautbois, laurent.pinchart, paul.kocialkowski, paul.elder, Mauro Carvalho Chehab, open list:OMNIVISION OV5670 SENSOR DRIVER, robh, devicetree On Tue, Mar 15, 2022 at 09:03:41AM +0100, Krzysztof Kozlowski wrote: > On 15/03/2022 08:59, Sakari Ailus wrote: > > Hi Krzysztof, Jacopo, > > > > On Tue, Mar 15, 2022 at 08:32:58AM +0100, Krzysztof Kozlowski wrote: > >> On 14/03/2022 17:27, Jacopo Mondi wrote: > >>> Provide the bindings documentation for Omnivision OV5670 image sensor. > >>> > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>> > >>> --- > >>> v1->v2 (comments from Krzysztof) > >>> > >>> - Rename to include manufacturer name > >>> - Add entry to MAINTAINERS > >>> - Add maxItems: to -gpios properties > >>> - Use common clock properties > >>> - Use enum: [1, 2] for data lanes > >>> - Fix whitespace issue in example > >>> --- > >>> > >>> .../bindings/media/i2c/ovti,ov5670.yaml | 99 +++++++++++++++++++ > >>> MAINTAINERS | 1 + > >>> 2 files changed, 100 insertions(+) > >>> create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml > >>> > >>> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml > >>> new file mode 100644 > >>> index 000000000000..73cf72203f17 > >>> --- /dev/null > >>> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml > >>> @@ -0,0 +1,99 @@ > >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > >>> +%YAML 1.2 > >>> +--- > >>> +$id: http://devicetree.org/schemas/media/i2c/ovti,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 > >>> + > >>> + assigned-clocks: true > >>> + assigned-clock-parents: true > >>> + assigned-clock-rates: true > >> > >> You should not need these. These are coming with schema. You can add > >> these to example schema below and double-check. > > > > They should probably be required actually. > > Why required? The hardware can work with different clocks, get their > rate and configure internal PLLs/clocks to new value. Having it required > might have sense for current implementation of driver but this is > independent of bindings. Bindings do not describe driver, but hardware. We've had this discussion before and the result of that was this (see "Handling clocks"): Documentation/driver-api/media/camera-sensor.rst > > >> > >>> + > >>> + clocks: > >>> + description: System clock. From 6 to 27 MHz. > >>> + maxItems: 1 > >>> + > >>> + pwdn-gpios: > >>> + description: Reference to the GPIO connected to the PWDNB pin. Active low. > >> > >> This does not look like a standard property, so you need a vendor prefix. > > > > The similarly named property exists elsewhere. I wouldn't use a vendor > > prefix, also for the reason that the functionality is quite common. I guess > > alternative name would be possible, too --- "shutdown" seems to be more > > common. > > It exists in three bindings, so it is not a standard property... > although something closer to standard is "powerdown-gpios" so maybe just > use that one? Seems like a better choice. -- Sakari Ailus ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/8] media: dt-bindings: i2c: Document ov5670 2022-03-15 12:47 ` Sakari Ailus @ 2022-03-15 12:57 ` Krzysztof Kozlowski 2022-03-15 13:01 ` Laurent Pinchart 1 sibling, 0 replies; 10+ messages in thread From: Krzysztof Kozlowski @ 2022-03-15 12:57 UTC (permalink / raw) To: Sakari Ailus Cc: Jacopo Mondi, Chiranjeevi Rapolu, jeanmichel.hautbois, laurent.pinchart, paul.kocialkowski, paul.elder, Mauro Carvalho Chehab, open list:OMNIVISION OV5670 SENSOR DRIVER, robh, devicetree On 15/03/2022 13:47, Sakari Ailus wrote: > On Tue, Mar 15, 2022 at 09:03:41AM +0100, Krzysztof Kozlowski wrote: >> On 15/03/2022 08:59, Sakari Ailus wrote: >>> Hi Krzysztof, Jacopo, >>> >>> On Tue, Mar 15, 2022 at 08:32:58AM +0100, Krzysztof Kozlowski wrote: >>>> On 14/03/2022 17:27, Jacopo Mondi wrote: >>>>> Provide the bindings documentation for Omnivision OV5670 image sensor. >>>>> >>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >>>>> >>>>> --- >>>>> v1->v2 (comments from Krzysztof) >>>>> >>>>> - Rename to include manufacturer name >>>>> - Add entry to MAINTAINERS >>>>> - Add maxItems: to -gpios properties >>>>> - Use common clock properties >>>>> - Use enum: [1, 2] for data lanes >>>>> - Fix whitespace issue in example >>>>> --- >>>>> >>>>> .../bindings/media/i2c/ovti,ov5670.yaml | 99 +++++++++++++++++++ >>>>> MAINTAINERS | 1 + >>>>> 2 files changed, 100 insertions(+) >>>>> create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml >>>>> new file mode 100644 >>>>> index 000000000000..73cf72203f17 >>>>> --- /dev/null >>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml >>>>> @@ -0,0 +1,99 @@ >>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>>>> +%YAML 1.2 >>>>> +--- >>>>> +$id: http://devicetree.org/schemas/media/i2c/ovti,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 >>>>> + >>>>> + assigned-clocks: true >>>>> + assigned-clock-parents: true >>>>> + assigned-clock-rates: true >>>> >>>> You should not need these. These are coming with schema. You can add >>>> these to example schema below and double-check. >>> >>> They should probably be required actually. >> >> Why required? The hardware can work with different clocks, get their >> rate and configure internal PLLs/clocks to new value. Having it required >> might have sense for current implementation of driver but this is >> independent of bindings. Bindings do not describe driver, but hardware. > > We've had this discussion before and the result of that was this (see > "Handling clocks"): > > Documentation/driver-api/media/camera-sensor.rst ... and the "Devicetree" chapter explains usage of assigned-clock-xxx. None of these explain why it should be in the bindings. The chapter you referred explicitly mentions that "clock tree is generally configured by the driver", so it has nothing to do with the bindings. Bindings describe hardware, not Linux driver implementation. The hardware can work with multiple frequencies and can support changing these frequencies, probably combined with some reset sequence. Therefore hardware does not require the clock frequency to be always fixed and defined. Driver requires assigned-clock-xxx, not bindings. Do not put Linux implementation specifics into the bindings. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/8] media: dt-bindings: i2c: Document ov5670 2022-03-15 12:47 ` Sakari Ailus 2022-03-15 12:57 ` Krzysztof Kozlowski @ 2022-03-15 13:01 ` Laurent Pinchart 2022-03-15 20:30 ` Sakari Ailus 1 sibling, 1 reply; 10+ messages in thread From: Laurent Pinchart @ 2022-03-15 13:01 UTC (permalink / raw) To: Sakari Ailus Cc: Krzysztof Kozlowski, Jacopo Mondi, Chiranjeevi Rapolu, jeanmichel.hautbois, paul.kocialkowski, paul.elder, Mauro Carvalho Chehab, open list:OMNIVISION OV5670 SENSOR DRIVER, robh, devicetree Hi Sakari, On Tue, Mar 15, 2022 at 02:47:43PM +0200, Sakari Ailus wrote: > On Tue, Mar 15, 2022 at 09:03:41AM +0100, Krzysztof Kozlowski wrote: > > On 15/03/2022 08:59, Sakari Ailus wrote: > > > On Tue, Mar 15, 2022 at 08:32:58AM +0100, Krzysztof Kozlowski wrote: > > >> On 14/03/2022 17:27, Jacopo Mondi wrote: > > >>> Provide the bindings documentation for Omnivision OV5670 image sensor. > > >>> > > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > >>> > > >>> --- > > >>> v1->v2 (comments from Krzysztof) > > >>> > > >>> - Rename to include manufacturer name > > >>> - Add entry to MAINTAINERS > > >>> - Add maxItems: to -gpios properties > > >>> - Use common clock properties > > >>> - Use enum: [1, 2] for data lanes > > >>> - Fix whitespace issue in example > > >>> --- > > >>> > > >>> .../bindings/media/i2c/ovti,ov5670.yaml | 99 +++++++++++++++++++ > > >>> MAINTAINERS | 1 + > > >>> 2 files changed, 100 insertions(+) > > >>> create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml > > >>> > > >>> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml > > >>> new file mode 100644 > > >>> index 000000000000..73cf72203f17 > > >>> --- /dev/null > > >>> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml > > >>> @@ -0,0 +1,99 @@ > > >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > >>> +%YAML 1.2 > > >>> +--- > > >>> +$id: http://devicetree.org/schemas/media/i2c/ovti,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 > > >>> + > > >>> + assigned-clocks: true > > >>> + assigned-clock-parents: true > > >>> + assigned-clock-rates: true > > >> > > >> You should not need these. These are coming with schema. You can add > > >> these to example schema below and double-check. > > > > > > They should probably be required actually. > > > > Why required? The hardware can work with different clocks, get their > > rate and configure internal PLLs/clocks to new value. Having it required > > might have sense for current implementation of driver but this is > > independent of bindings. Bindings do not describe driver, but hardware. > > We've had this discussion before and the result of that was this (see > "Handling clocks"): > > Documentation/driver-api/media/camera-sensor.rst I don't think those properties should be required in the sensor bindings. There are platforms where the clock provided to the sensor comes from a fixed-frequency oscillator, assigning a rate or parent makes no sense for those (assigning a parent would actually be impossible). Assigning a parent or rate is fine when applicable, but as it can't be required, there's also no point in listing the properties here. > > >>> + > > >>> + clocks: > > >>> + description: System clock. From 6 to 27 MHz. > > >>> + maxItems: 1 > > >>> + > > >>> + pwdn-gpios: > > >>> + description: Reference to the GPIO connected to the PWDNB pin. Active low. > > >> > > >> This does not look like a standard property, so you need a vendor prefix. > > > > > > The similarly named property exists elsewhere. I wouldn't use a vendor > > > prefix, also for the reason that the functionality is quite common. I guess > > > alternative name would be possible, too --- "shutdown" seems to be more > > > common. > > > > It exists in three bindings, so it is not a standard property... > > although something closer to standard is "powerdown-gpios" so maybe just > > use that one? > > Seems like a better choice. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/8] media: dt-bindings: i2c: Document ov5670 2022-03-15 13:01 ` Laurent Pinchart @ 2022-03-15 20:30 ` Sakari Ailus 0 siblings, 0 replies; 10+ messages in thread From: Sakari Ailus @ 2022-03-15 20:30 UTC (permalink / raw) To: Laurent Pinchart Cc: Krzysztof Kozlowski, Jacopo Mondi, Chiranjeevi Rapolu, jeanmichel.hautbois, paul.kocialkowski, paul.elder, Mauro Carvalho Chehab, open list:OMNIVISION OV5670 SENSOR DRIVER, robh, devicetree Hi Laurent, On Tue, Mar 15, 2022 at 03:01:35PM +0200, Laurent Pinchart wrote: > Hi Sakari, > > On Tue, Mar 15, 2022 at 02:47:43PM +0200, Sakari Ailus wrote: > > On Tue, Mar 15, 2022 at 09:03:41AM +0100, Krzysztof Kozlowski wrote: > > > On 15/03/2022 08:59, Sakari Ailus wrote: > > > > On Tue, Mar 15, 2022 at 08:32:58AM +0100, Krzysztof Kozlowski wrote: > > > >> On 14/03/2022 17:27, Jacopo Mondi wrote: > > > >>> Provide the bindings documentation for Omnivision OV5670 image sensor. > > > >>> > > > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > >>> > > > >>> --- > > > >>> v1->v2 (comments from Krzysztof) > > > >>> > > > >>> - Rename to include manufacturer name > > > >>> - Add entry to MAINTAINERS > > > >>> - Add maxItems: to -gpios properties > > > >>> - Use common clock properties > > > >>> - Use enum: [1, 2] for data lanes > > > >>> - Fix whitespace issue in example > > > >>> --- > > > >>> > > > >>> .../bindings/media/i2c/ovti,ov5670.yaml | 99 +++++++++++++++++++ > > > >>> MAINTAINERS | 1 + > > > >>> 2 files changed, 100 insertions(+) > > > >>> create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml > > > >>> > > > >>> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml > > > >>> new file mode 100644 > > > >>> index 000000000000..73cf72203f17 > > > >>> --- /dev/null > > > >>> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml > > > >>> @@ -0,0 +1,99 @@ > > > >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > >>> +%YAML 1.2 > > > >>> +--- > > > >>> +$id: http://devicetree.org/schemas/media/i2c/ovti,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 > > > >>> + > > > >>> + assigned-clocks: true > > > >>> + assigned-clock-parents: true > > > >>> + assigned-clock-rates: true > > > >> > > > >> You should not need these. These are coming with schema. You can add > > > >> these to example schema below and double-check. > > > > > > > > They should probably be required actually. > > > > > > Why required? The hardware can work with different clocks, get their > > > rate and configure internal PLLs/clocks to new value. Having it required > > > might have sense for current implementation of driver but this is > > > independent of bindings. Bindings do not describe driver, but hardware. > > > > We've had this discussion before and the result of that was this (see > > "Handling clocks"): > > > > Documentation/driver-api/media/camera-sensor.rst > > I don't think those properties should be required in the sensor > bindings. There are platforms where the clock provided to the sensor > comes from a fixed-frequency oscillator, assigning a rate or parent > makes no sense for those (assigning a parent would actually be > impossible). > > Assigning a parent or rate is fine when applicable, but as it can't be > required, there's also no point in listing the properties here. The cases where the clock is fixed are quite rare but admittedly they exist. It's just easy to get this wrong. -- Sakari Ailus ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/8] media: dt-bindings: i2c: Document ov5670 2022-03-15 7:59 ` Sakari Ailus 2022-03-15 8:03 ` Krzysztof Kozlowski @ 2022-03-15 8:09 ` Laurent Pinchart 1 sibling, 0 replies; 10+ messages in thread From: Laurent Pinchart @ 2022-03-15 8:09 UTC (permalink / raw) To: Sakari Ailus Cc: Krzysztof Kozlowski, Jacopo Mondi, Chiranjeevi Rapolu, jeanmichel.hautbois, paul.kocialkowski, paul.elder, Mauro Carvalho Chehab, open list:OMNIVISION OV5670 SENSOR DRIVER, robh, devicetree Hi Sakari, On Tue, Mar 15, 2022 at 09:59:17AM +0200, Sakari Ailus wrote: > On Tue, Mar 15, 2022 at 08:32:58AM +0100, Krzysztof Kozlowski wrote: > > On 14/03/2022 17:27, Jacopo Mondi wrote: > > > Provide the bindings documentation for Omnivision OV5670 image sensor. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > --- > > > v1->v2 (comments from Krzysztof) > > > > > > - Rename to include manufacturer name > > > - Add entry to MAINTAINERS > > > - Add maxItems: to -gpios properties > > > - Use common clock properties > > > - Use enum: [1, 2] for data lanes > > > - Fix whitespace issue in example > > > --- > > > > > > .../bindings/media/i2c/ovti,ov5670.yaml | 99 +++++++++++++++++++ > > > MAINTAINERS | 1 + > > > 2 files changed, 100 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml > > > new file mode 100644 > > > index 000000000000..73cf72203f17 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml > > > @@ -0,0 +1,99 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/media/i2c/ovti,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 > > > + > > > + assigned-clocks: true > > > + assigned-clock-parents: true > > > + assigned-clock-rates: true > > > > You should not need these. These are coming with schema. You can add > > these to example schema below and double-check. > > They should probably be required actually. Why so ? > > > + > > > + clocks: > > > + description: System clock. From 6 to 27 MHz. > > > + maxItems: 1 > > > + > > > + pwdn-gpios: > > > + description: Reference to the GPIO connected to the PWDNB pin. Active low. > > > > This does not look like a standard property, so you need a vendor prefix. > > The similarly named property exists elsewhere. I wouldn't use a vendor > prefix, also for the reason that the functionality is quite common. I guess > alternative name would be possible, too --- "shutdown" seems to be more > common. There's a desire to standardize GPIO names ("reset" and "enable" being the two most common candidates), but I'm not aware of an official list of standard names. Have I missed it ? In this case, I'd use powerdown-gpios, as it's more common than pwdn-gpios (used in 21 bindings, compared to 2 for pwdn-gpios). -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/8] media: dt-bindings: i2c: Document ov5670 2022-03-15 7:32 ` Krzysztof Kozlowski 2022-03-15 7:59 ` Sakari Ailus @ 2022-03-15 8:41 ` Jacopo Mondi 1 sibling, 0 replies; 10+ messages in thread From: Jacopo Mondi @ 2022-03-15 8:41 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Chiranjeevi Rapolu, jeanmichel.hautbois, laurent.pinchart, paul.kocialkowski, sakari.ailus, paul.elder, Mauro Carvalho Chehab, open list:OMNIVISION OV5670 SENSOR DRIVER, robh, devicetree Hi Krzysztof, On Tue, Mar 15, 2022 at 08:32:58AM +0100, Krzysztof Kozlowski wrote: > On 14/03/2022 17:27, Jacopo Mondi wrote: > > Provide the bindings documentation for Omnivision OV5670 image sensor. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > v1->v2 (comments from Krzysztof) > > > > - Rename to include manufacturer name > > - Add entry to MAINTAINERS > > - Add maxItems: to -gpios properties > > - Use common clock properties > > - Use enum: [1, 2] for data lanes > > - Fix whitespace issue in example > > --- > > > > .../bindings/media/i2c/ovti,ov5670.yaml | 99 +++++++++++++++++++ > > MAINTAINERS | 1 + > > 2 files changed, 100 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml > > new file mode 100644 > > index 000000000000..73cf72203f17 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml > > @@ -0,0 +1,99 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/media/i2c/ovti,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 > > + > > + assigned-clocks: true > > + assigned-clock-parents: true > > + assigned-clock-rates: true > > You should not need these. These are coming with schema. You can add > these to example schema below and double-check. > Thanks, I'll try removing and see how it works > > + > > + clocks: > > + description: System clock. From 6 to 27 MHz. > > + maxItems: 1 > > + > > + pwdn-gpios: > > + description: Reference to the GPIO connected to the PWDNB pin. Active low. > > This does not look like a standard property, so you need a vendor prefix. > $ git grep powerdown-gpio arch/arm*/boot/dts | wc -l 48 $ git grep pwdn-gpio arch/arm*/boot/dts | wc -l 5 I'll go with the majority. I wonder however if 'standard' proeprties are documented somewhere. Thanks j > > + maxItems: 1 > > + > > Best regards, > Krzysztof ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-03-15 20:30 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20220314162714.153970-1-jacopo@jmondi.org>
2022-03-14 16:27 ` [PATCH v2 1/8] media: dt-bindings: i2c: Document ov5670 Jacopo Mondi
2022-03-15 7:32 ` Krzysztof Kozlowski
2022-03-15 7:59 ` Sakari Ailus
2022-03-15 8:03 ` Krzysztof Kozlowski
2022-03-15 12:47 ` Sakari Ailus
2022-03-15 12:57 ` Krzysztof Kozlowski
2022-03-15 13:01 ` Laurent Pinchart
2022-03-15 20:30 ` Sakari Ailus
2022-03-15 8:09 ` Laurent Pinchart
2022-03-15 8:41 ` Jacopo Mondi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox