| Message ID | 20220911200147.375198-2-mike.rudenko@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Omnivision OV4689 image sensor driver | expand |
On 11/09/2022 22:01, Mikhail Rudenko wrote: > Add device-tree binding documentation for OV4689 image sensor driver, > and the relevant MAINTAINERS entries. > > Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com> Too many "media" prefixes in the subject. Also you duplicated dt bindings as prefix and commit msg (skip the latter). > --- > .../bindings/media/i2c/ovti,ov4689.yaml | 141 ++++++++++++++++++ > MAINTAINERS | 7 + > 2 files changed, 148 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml > > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml > new file mode 100644 > index 000000000000..376330b5572a > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml > @@ -0,0 +1,141 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/i2c/ovti,ov4689.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Omnivision OV4689 CMOS > + > +maintainers: > + - Mikhail Rudenko <mike.rudenko@gmail.com> > + > +description: | > + The Omnivision OV4689 is a high performance, 1/3-inch, 4 megapixel > + image sensor. Ihis chip supports high frame rate speeds up to 90 fps > + at 2688x1520 resolution. It is programmable through an I2C > + interface, and sensor output is sent via 1/2/4 lane MIPI CSI-2 > + connection. > + > +allOf: > + - $ref: /schemas/media/video-interface-devices.yaml# > + > +properties: > + compatible: > + const: ovti,ov4689 > + > + reg: > + maxItems: 1 > + > + clocks: > + description: > + External clock (XVCLK) for the sensor, 6-64 MHz > + maxItems: 1 > + > + clock-names: true This has to be strictly defined - which name you expect. > + > + dovdd-supply: > + description: > + Digital I/O voltage supply, 1.7-3.0 V > + > + avdd-supply: > + description: > + Analog voltage supply, 2.6-3.0 V > + > + dvdd-supply: > + description: > + Digital core voltage supply, 1.1-1.3 V > + > + powerdown-gpios: > + maxItems: 1 You can skip here maxItems - it is defined by gpio-consumer-common. > + description: > + GPIO connected to the powerdown pin (active low) > + > + reset-gpios: > + maxItems: 1 > + description: > + GPIO connected to the reset pin (active low) > + Best regards, Krzysztof
Hi Mikhail, On Sun, Sep 11, 2022 at 11:01:34PM +0300, Mikhail Rudenko wrote: > Add device-tree binding documentation for OV4689 image sensor driver, > and the relevant MAINTAINERS entries. > > Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com> > --- > .../bindings/media/i2c/ovti,ov4689.yaml | 141 ++++++++++++++++++ > MAINTAINERS | 7 + > 2 files changed, 148 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml > > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml > new file mode 100644 > index 000000000000..376330b5572a > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml > @@ -0,0 +1,141 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/i2c/ovti,ov4689.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Omnivision OV4689 CMOS > + > +maintainers: > + - Mikhail Rudenko <mike.rudenko@gmail.com> > + > +description: | > + The Omnivision OV4689 is a high performance, 1/3-inch, 4 megapixel > + image sensor. Ihis chip supports high frame rate speeds up to 90 fps > + at 2688x1520 resolution. It is programmable through an I2C > + interface, and sensor output is sent via 1/2/4 lane MIPI CSI-2 > + connection. > + > +allOf: > + - $ref: /schemas/media/video-interface-devices.yaml# > + > +properties: > + compatible: > + const: ovti,ov4689 > + > + reg: > + maxItems: 1 > + > + clocks: > + description: > + External clock (XVCLK) for the sensor, 6-64 MHz > + maxItems: 1 > + > + clock-names: true > + > + dovdd-supply: > + description: > + Digital I/O voltage supply, 1.7-3.0 V > + > + avdd-supply: > + description: > + Analog voltage supply, 2.6-3.0 V > + > + dvdd-supply: > + description: > + Digital core voltage supply, 1.1-1.3 V > + > + powerdown-gpios: > + maxItems: 1 > + description: > + GPIO connected to the powerdown pin (active low) > + > + reset-gpios: > + maxItems: 1 > + description: > + GPIO connected to the reset pin (active low) > + > + orientation: true > + > + rotation: true > + > + port: > + $ref: /schemas/graph.yaml#/$defs/port-base > + additionalProperties: false > + description: > + Output port node, single endpoint describing the CSI-2 transmitter > + > + properties: > + endpoint: > + $ref: /schemas/media/video-interfaces.yaml# > + unevaluatedProperties: false > + > + properties: > + data-lanes: > + oneOf: > + - items: > + - const: 1 > + - const: 2 > + - const: 3 > + - const: 4 > + - items: > + - const: 1 > + - const: 2 > + - items: > + - const: 1 > + link-frequencies: true > + > + required: > + - data-lanes > + - link-frequencies > + > +required: > + - compatible > + - reg > + - clocks > + - clock-names > + - dovdd-supply > + - avdd-supply > + - dvdd-supply > + - powerdown-gpios > + - reset-gpios > + - port I think we don't need all of these entries as required. The only let me say "really" required are: - compatible - reg - clocks - port Regards, Tommaso > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + ov4689: camera@36 { > + compatible = "ovti,ov4689"; > + reg = <0x36>; > + > + clocks = <&ov4689_clk>; > + clock-names = "xvclk"; > + > + avdd-supply = <&ov4689_avdd>; > + dovdd-supply = <&ov4689_dovdd>; > + dvdd-supply = <&ov4689_dvdd>; > + > + powerdown-gpios = <&pio 107 GPIO_ACTIVE_LOW>; > + reset-gpios = <&pio 109 GPIO_ACTIVE_LOW>; > + > + orientation = <2>; > + rotation = <0>; > + > + port { > + wcam_out: endpoint { > + remote-endpoint = <&mipi_in_wcam>; > + data-lanes = <1 2 3 4>; > + link-frequencies = /bits/ 64 <500000000>; > + }; > + }; > + }; > + }; > + > +... > diff --git a/MAINTAINERS b/MAINTAINERS > index f468864fd268..63c4844f26e6 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -14523,6 +14523,13 @@ S: Maintained > T: git git://linuxtv.org/media_tree.git > F: drivers/media/i2c/ov2740.c > > +OMNIVISION OV4689 SENSOR DRIVER > +M: Mikhail Rudenko <mike.rudenko@gmail.com> > +L: linux-media@vger.kernel.org > +S: Maintained > +T: git git://linuxtv.org/media_tree.git > +F: Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml > + > OMNIVISION OV5640 SENSOR DRIVER > M: Steve Longerbeam <slongerbeam@gmail.com> > L: linux-media@vger.kernel.org > -- > 2.37.3 >
Hi Krzysztof, On 2022-09-12 at 12:55 +02, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 11/09/2022 22:01, Mikhail Rudenko wrote: >> Add device-tree binding documentation for OV4689 image sensor driver, >> and the relevant MAINTAINERS entries. >> >> Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com> > > Too many "media" prefixes in the subject. I see, will drop the first "media:" in v3. > Also you duplicated dt > bindings as prefix and commit msg (skip the latter). Just to be clear, do you mean dropping "device-tree binding" phrase from the commit message? >> --- >> .../bindings/media/i2c/ovti,ov4689.yaml | 141 ++++++++++++++++++ >> MAINTAINERS | 7 + >> 2 files changed, 148 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml >> >> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml >> new file mode 100644 >> index 000000000000..376330b5572a >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml >> @@ -0,0 +1,141 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/media/i2c/ovti,ov4689.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Omnivision OV4689 CMOS >> + >> +maintainers: >> + - Mikhail Rudenko <mike.rudenko@gmail.com> >> + >> +description: | >> + The Omnivision OV4689 is a high performance, 1/3-inch, 4 megapixel >> + image sensor. Ihis chip supports high frame rate speeds up to 90 fps >> + at 2688x1520 resolution. It is programmable through an I2C >> + interface, and sensor output is sent via 1/2/4 lane MIPI CSI-2 >> + connection. >> + >> +allOf: >> + - $ref: /schemas/media/video-interface-devices.yaml# >> + >> +properties: >> + compatible: >> + const: ovti,ov4689 >> + >> + reg: >> + maxItems: 1 >> + >> + clocks: >> + description: >> + External clock (XVCLK) for the sensor, 6-64 MHz >> + maxItems: 1 >> + >> + clock-names: true > > This has to be strictly defined - which name you expect. Will fix in v3. Or maybe we should drop clock-names altogether and use devm_clk_get(&client->dev, NULL) in the driver instead (I've seen this approach in some existing drivers)? >> + >> + dovdd-supply: >> + description: >> + Digital I/O voltage supply, 1.7-3.0 V >> + >> + avdd-supply: >> + description: >> + Analog voltage supply, 2.6-3.0 V >> + >> + dvdd-supply: >> + description: >> + Digital core voltage supply, 1.1-1.3 V >> + >> + powerdown-gpios: >> + maxItems: 1 > > You can skip here maxItems - it is defined by gpio-consumer-common. Ack, will fix in v3. Does this also apply to reset-gpios? >> + description: >> + GPIO connected to the powerdown pin (active low) >> + >> + reset-gpios: >> + maxItems: 1 >> + description: >> + GPIO connected to the reset pin (active low) >> + > > Best regards, > Krzysztof Thanks for review, -- Best regards, Mikhail Rudenko
Hi Tommaso, On 2022-09-13 at 16:05 +02, Tommaso Merciai <tommaso.merciai@amarulasolutions.com> wrote: > Hi Mikhail, > > On Sun, Sep 11, 2022 at 11:01:34PM +0300, Mikhail Rudenko wrote: >> Add device-tree binding documentation for OV4689 image sensor driver, >> and the relevant MAINTAINERS entries. >> >> Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com> >> --- >> .../bindings/media/i2c/ovti,ov4689.yaml | 141 ++++++++++++++++++ >> MAINTAINERS | 7 + >> 2 files changed, 148 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml >> >> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml >> new file mode 100644 >> index 000000000000..376330b5572a >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml >> @@ -0,0 +1,141 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/media/i2c/ovti,ov4689.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Omnivision OV4689 CMOS >> + >> +maintainers: >> + - Mikhail Rudenko <mike.rudenko@gmail.com> >> + >> +description: | >> + The Omnivision OV4689 is a high performance, 1/3-inch, 4 megapixel >> + image sensor. Ihis chip supports high frame rate speeds up to 90 fps >> + at 2688x1520 resolution. It is programmable through an I2C >> + interface, and sensor output is sent via 1/2/4 lane MIPI CSI-2 >> + connection. >> + >> +allOf: >> + - $ref: /schemas/media/video-interface-devices.yaml# >> + >> +properties: >> + compatible: >> + const: ovti,ov4689 >> + >> + reg: >> + maxItems: 1 >> + >> + clocks: >> + description: >> + External clock (XVCLK) for the sensor, 6-64 MHz >> + maxItems: 1 >> + >> + clock-names: true >> + >> + dovdd-supply: >> + description: >> + Digital I/O voltage supply, 1.7-3.0 V >> + >> + avdd-supply: >> + description: >> + Analog voltage supply, 2.6-3.0 V >> + >> + dvdd-supply: >> + description: >> + Digital core voltage supply, 1.1-1.3 V >> + >> + powerdown-gpios: >> + maxItems: 1 >> + description: >> + GPIO connected to the powerdown pin (active low) >> + >> + reset-gpios: >> + maxItems: 1 >> + description: >> + GPIO connected to the reset pin (active low) >> + >> + orientation: true >> + >> + rotation: true >> + >> + port: >> + $ref: /schemas/graph.yaml#/$defs/port-base >> + additionalProperties: false >> + description: >> + Output port node, single endpoint describing the CSI-2 transmitter >> + >> + properties: >> + endpoint: >> + $ref: /schemas/media/video-interfaces.yaml# >> + unevaluatedProperties: false >> + >> + properties: >> + data-lanes: >> + oneOf: >> + - items: >> + - const: 1 >> + - const: 2 >> + - const: 3 >> + - const: 4 >> + - items: >> + - const: 1 >> + - const: 2 >> + - items: >> + - const: 1 >> + link-frequencies: true >> + >> + required: >> + - data-lanes >> + - link-frequencies >> + >> +required: >> + - compatible >> + - reg >> + - clocks >> + - clock-names >> + - dovdd-supply >> + - avdd-supply >> + - dvdd-supply >> + - powerdown-gpios >> + - reset-gpios >> + - port > > I think we don't need all of these entries as required. > The only let me say "really" required are: > > - compatible > - reg > - clocks > - port Thanks for the review! I agree that the driver may be modified to work without powerdown and reset gpios and they are not required for sensor operation. On contrary, supplies are obviously required. Of course, linux provides dummy regulators if supplies are missing from device tree, but I though the intention was to document hardware, not implementation details. What do think of this? > Regards, > Tommaso -- Best regards, Mikhail Rudenko
On 15/09/2022 13:16, Mikhail Rudenko wrote: > > I see, will drop the first "media:" in v3. > >> Also you duplicated dt >> bindings as prefix and commit msg (skip the latter). > > Just to be clear, do you mean dropping "device-tree binding" phrase from > the commit message? Ah, sorry, I meant in the subject. You already have dt-bindings as prefix, so no "DT bindings" at the end. Just: media: dt-bindings: i2c: document OV4689 > >>> --- >>> .../bindings/media/i2c/ovti,ov4689.yaml | 141 ++++++++++++++++++ >>> MAINTAINERS | 7 + >>> 2 files changed, 148 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml >>> >>> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml >>> new file mode 100644 >>> index 000000000000..376330b5572a >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml >>> @@ -0,0 +1,141 @@ >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/media/i2c/ovti,ov4689.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Omnivision OV4689 CMOS >>> + >>> +maintainers: >>> + - Mikhail Rudenko <mike.rudenko@gmail.com> >>> + >>> +description: | >>> + The Omnivision OV4689 is a high performance, 1/3-inch, 4 megapixel >>> + image sensor. Ihis chip supports high frame rate speeds up to 90 fps >>> + at 2688x1520 resolution. It is programmable through an I2C >>> + interface, and sensor output is sent via 1/2/4 lane MIPI CSI-2 >>> + connection. >>> + >>> +allOf: >>> + - $ref: /schemas/media/video-interface-devices.yaml# >>> + >>> +properties: >>> + compatible: >>> + const: ovti,ov4689 >>> + >>> + reg: >>> + maxItems: 1 >>> + >>> + clocks: >>> + description: >>> + External clock (XVCLK) for the sensor, 6-64 MHz >>> + maxItems: 1 >>> + >>> + clock-names: true >> >> This has to be strictly defined - which name you expect. > > Will fix in v3. Or maybe we should drop clock-names altogether and use > devm_clk_get(&client->dev, NULL) in the driver instead (I've seen this > approach in some existing drivers)? Yes, usually clock-names for one entry does not make sense. > >>> + >>> + dovdd-supply: >>> + description: >>> + Digital I/O voltage supply, 1.7-3.0 V >>> + >>> + avdd-supply: >>> + description: >>> + Analog voltage supply, 2.6-3.0 V >>> + >>> + dvdd-supply: >>> + description: >>> + Digital core voltage supply, 1.1-1.3 V >>> + >>> + powerdown-gpios: >>> + maxItems: 1 >> >> You can skip here maxItems - it is defined by gpio-consumer-common. > > Ack, will fix in v3. Does this also apply to reset-gpios? No. https://elixir.bootlin.com/linux/v6.0-rc1/source/Documentation/devicetree/bindings/gpio/gpio-consumer-common.yaml Best regards, Krzysztof
Hi Mikhail, On Thu, Sep 15, 2022 at 11:11:57PM +0300, Mikhail Rudenko wrote: > > Hi Tommaso, > > On 2022-09-13 at 16:05 +02, Tommaso Merciai <tommaso.merciai@amarulasolutions.com> wrote: > > Hi Mikhail, > > > > On Sun, Sep 11, 2022 at 11:01:34PM +0300, Mikhail Rudenko wrote: > >> Add device-tree binding documentation for OV4689 image sensor driver, > >> and the relevant MAINTAINERS entries. > >> > >> Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com> > >> --- > >> .../bindings/media/i2c/ovti,ov4689.yaml | 141 ++++++++++++++++++ > >> MAINTAINERS | 7 + > >> 2 files changed, 148 insertions(+) > >> create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml > >> > >> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml > >> new file mode 100644 > >> index 000000000000..376330b5572a > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml > >> @@ -0,0 +1,141 @@ > >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > >> +%YAML 1.2 > >> +--- > >> +$id: http://devicetree.org/schemas/media/i2c/ovti,ov4689.yaml# > >> +$schema: http://devicetree.org/meta-schemas/core.yaml# > >> + > >> +title: Omnivision OV4689 CMOS > >> + > >> +maintainers: > >> + - Mikhail Rudenko <mike.rudenko@gmail.com> > >> + > >> +description: | > >> + The Omnivision OV4689 is a high performance, 1/3-inch, 4 megapixel > >> + image sensor. Ihis chip supports high frame rate speeds up to 90 fps > >> + at 2688x1520 resolution. It is programmable through an I2C > >> + interface, and sensor output is sent via 1/2/4 lane MIPI CSI-2 > >> + connection. > >> + > >> +allOf: > >> + - $ref: /schemas/media/video-interface-devices.yaml# > >> + > >> +properties: > >> + compatible: > >> + const: ovti,ov4689 > >> + > >> + reg: > >> + maxItems: 1 > >> + > >> + clocks: > >> + description: > >> + External clock (XVCLK) for the sensor, 6-64 MHz > >> + maxItems: 1 > >> + > >> + clock-names: true > >> + > >> + dovdd-supply: > >> + description: > >> + Digital I/O voltage supply, 1.7-3.0 V > >> + > >> + avdd-supply: > >> + description: > >> + Analog voltage supply, 2.6-3.0 V > >> + > >> + dvdd-supply: > >> + description: > >> + Digital core voltage supply, 1.1-1.3 V > >> + > >> + powerdown-gpios: > >> + maxItems: 1 > >> + description: > >> + GPIO connected to the powerdown pin (active low) > >> + > >> + reset-gpios: > >> + maxItems: 1 > >> + description: > >> + GPIO connected to the reset pin (active low) > >> + > >> + orientation: true > >> + > >> + rotation: true > >> + > >> + port: > >> + $ref: /schemas/graph.yaml#/$defs/port-base > >> + additionalProperties: false > >> + description: > >> + Output port node, single endpoint describing the CSI-2 transmitter > >> + > >> + properties: > >> + endpoint: > >> + $ref: /schemas/media/video-interfaces.yaml# > >> + unevaluatedProperties: false > >> + > >> + properties: > >> + data-lanes: > >> + oneOf: > >> + - items: > >> + - const: 1 > >> + - const: 2 > >> + - const: 3 > >> + - const: 4 > >> + - items: > >> + - const: 1 > >> + - const: 2 > >> + - items: > >> + - const: 1 > >> + link-frequencies: true > >> + > >> + required: > >> + - data-lanes > >> + - link-frequencies > >> + > >> +required: > >> + - compatible > >> + - reg > >> + - clocks > >> + - clock-names > >> + - dovdd-supply > >> + - avdd-supply > >> + - dvdd-supply > >> + - powerdown-gpios > >> + - reset-gpios > >> + - port > > > > I think we don't need all of these entries as required. > > The only let me say "really" required are: > > > > - compatible > > - reg > > - clocks > > - port > > Thanks for the review! I agree that the driver may be modified to work > without powerdown and reset gpios and they are not required for sensor > operation. On contrary, supplies are obviously required. Of course, linux > provides dummy regulators if supplies are missing from device tree, but > I though the intention was to document hardware, not implementation > details. What do think of this? We have already discuss on this on the following thread sometimes ago :) https://www.patchwork.linux-fancy.com/project/linux-fancy/patch/20220630134835.592521-6-tommaso.merciai@amarulasolutions.com/ Take a look and let me know. Thanks, Tommaso > > > Regards, > > Tommaso > > -- > Best regards, > Mikhail Rudenko
On 2022-09-16 at 15:15 +02, Tommaso Merciai <tommaso.merciai@amarulasolutions.com> wrote: > Hi Mikhail, > > On Thu, Sep 15, 2022 at 11:11:57PM +0300, Mikhail Rudenko wrote: >> >> Hi Tommaso, >> >> On 2022-09-13 at 16:05 +02, Tommaso Merciai <tommaso.merciai@amarulasolutions.com> wrote: >> > Hi Mikhail, >> > >> > On Sun, Sep 11, 2022 at 11:01:34PM +0300, Mikhail Rudenko wrote: >> >> Add device-tree binding documentation for OV4689 image sensor driver, >> >> and the relevant MAINTAINERS entries. >> >> >> >> Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com> >> >> --- >> >> .../bindings/media/i2c/ovti,ov4689.yaml | 141 ++++++++++++++++++ >> >> MAINTAINERS | 7 + >> >> 2 files changed, 148 insertions(+) >> >> create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml >> >> >> >> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml >> >> new file mode 100644 >> >> index 000000000000..376330b5572a >> >> --- /dev/null >> >> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml >> >> @@ -0,0 +1,141 @@ >> >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> >> +%YAML 1.2 >> >> +--- >> >> +$id: http://devicetree.org/schemas/media/i2c/ovti,ov4689.yaml# >> >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> >> + >> >> +title: Omnivision OV4689 CMOS >> >> + >> >> +maintainers: >> >> + - Mikhail Rudenko <mike.rudenko@gmail.com> >> >> + >> >> +description: | >> >> + The Omnivision OV4689 is a high performance, 1/3-inch, 4 megapixel >> >> + image sensor. Ihis chip supports high frame rate speeds up to 90 fps >> >> + at 2688x1520 resolution. It is programmable through an I2C >> >> + interface, and sensor output is sent via 1/2/4 lane MIPI CSI-2 >> >> + connection. >> >> + >> >> +allOf: >> >> + - $ref: /schemas/media/video-interface-devices.yaml# >> >> + >> >> +properties: >> >> + compatible: >> >> + const: ovti,ov4689 >> >> + >> >> + reg: >> >> + maxItems: 1 >> >> + >> >> + clocks: >> >> + description: >> >> + External clock (XVCLK) for the sensor, 6-64 MHz >> >> + maxItems: 1 >> >> + >> >> + clock-names: true >> >> + >> >> + dovdd-supply: >> >> + description: >> >> + Digital I/O voltage supply, 1.7-3.0 V >> >> + >> >> + avdd-supply: >> >> + description: >> >> + Analog voltage supply, 2.6-3.0 V >> >> + >> >> + dvdd-supply: >> >> + description: >> >> + Digital core voltage supply, 1.1-1.3 V >> >> + >> >> + powerdown-gpios: >> >> + maxItems: 1 >> >> + description: >> >> + GPIO connected to the powerdown pin (active low) >> >> + >> >> + reset-gpios: >> >> + maxItems: 1 >> >> + description: >> >> + GPIO connected to the reset pin (active low) >> >> + >> >> + orientation: true >> >> + >> >> + rotation: true >> >> + >> >> + port: >> >> + $ref: /schemas/graph.yaml#/$defs/port-base >> >> + additionalProperties: false >> >> + description: >> >> + Output port node, single endpoint describing the CSI-2 transmitter >> >> + >> >> + properties: >> >> + endpoint: >> >> + $ref: /schemas/media/video-interfaces.yaml# >> >> + unevaluatedProperties: false >> >> + >> >> + properties: >> >> + data-lanes: >> >> + oneOf: >> >> + - items: >> >> + - const: 1 >> >> + - const: 2 >> >> + - const: 3 >> >> + - const: 4 >> >> + - items: >> >> + - const: 1 >> >> + - const: 2 >> >> + - items: >> >> + - const: 1 >> >> + link-frequencies: true >> >> + >> >> + required: >> >> + - data-lanes >> >> + - link-frequencies >> >> + >> >> +required: >> >> + - compatible >> >> + - reg >> >> + - clocks >> >> + - clock-names >> >> + - dovdd-supply >> >> + - avdd-supply >> >> + - dvdd-supply >> >> + - powerdown-gpios >> >> + - reset-gpios >> >> + - port >> > >> > I think we don't need all of these entries as required. >> > The only let me say "really" required are: >> > >> > - compatible >> > - reg >> > - clocks >> > - port >> >> Thanks for the review! I agree that the driver may be modified to work >> without powerdown and reset gpios and they are not required for sensor >> operation. On contrary, supplies are obviously required. Of course, linux >> provides dummy regulators if supplies are missing from device tree, but >> I though the intention was to document hardware, not implementation >> details. What do think of this? > > We have already discuss on this on the following thread sometimes ago :) > > https://www.patchwork.linux-fancy.com/project/linux-fancy/patch/20220630134835.592521-6-tommaso.merciai@amarulasolutions.com/ > > Take a look and let me know. Okay, if there already is a consensus regarding this matter, I'll make the regulators optional in v3. -- Best regards, Mikhail Rudenko
CC'ing Liam and Mark. On Fri, Sep 16, 2022 at 04:42:25PM +0300, Mikhail Rudenko wrote: > On 2022-09-16 at 15:15 +02, Tommaso Merciai <tommaso.merciai@amarulasolutions.com> wrote: > > On Thu, Sep 15, 2022 at 11:11:57PM +0300, Mikhail Rudenko wrote: > >> On 2022-09-13 at 16:05 +02, Tommaso Merciai <tommaso.merciai@amarulasolutions.com> wrote: > >> > On Sun, Sep 11, 2022 at 11:01:34PM +0300, Mikhail Rudenko wrote: > >> >> Add device-tree binding documentation for OV4689 image sensor driver, > >> >> and the relevant MAINTAINERS entries. > >> >> > >> >> Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com> > >> >> --- > >> >> .../bindings/media/i2c/ovti,ov4689.yaml | 141 ++++++++++++++++++ > >> >> MAINTAINERS | 7 + > >> >> 2 files changed, 148 insertions(+) > >> >> create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml > >> >> > >> >> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml > >> >> new file mode 100644 > >> >> index 000000000000..376330b5572a > >> >> --- /dev/null > >> >> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml > >> >> @@ -0,0 +1,141 @@ > >> >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > >> >> +%YAML 1.2 > >> >> +--- > >> >> +$id: http://devicetree.org/schemas/media/i2c/ovti,ov4689.yaml# > >> >> +$schema: http://devicetree.org/meta-schemas/core.yaml# > >> >> + > >> >> +title: Omnivision OV4689 CMOS > >> >> + > >> >> +maintainers: > >> >> + - Mikhail Rudenko <mike.rudenko@gmail.com> > >> >> + > >> >> +description: | > >> >> + The Omnivision OV4689 is a high performance, 1/3-inch, 4 megapixel > >> >> + image sensor. Ihis chip supports high frame rate speeds up to 90 fps > >> >> + at 2688x1520 resolution. It is programmable through an I2C > >> >> + interface, and sensor output is sent via 1/2/4 lane MIPI CSI-2 > >> >> + connection. > >> >> + > >> >> +allOf: > >> >> + - $ref: /schemas/media/video-interface-devices.yaml# > >> >> + > >> >> +properties: > >> >> + compatible: > >> >> + const: ovti,ov4689 > >> >> + > >> >> + reg: > >> >> + maxItems: 1 > >> >> + > >> >> + clocks: > >> >> + description: > >> >> + External clock (XVCLK) for the sensor, 6-64 MHz > >> >> + maxItems: 1 > >> >> + > >> >> + clock-names: true > >> >> + > >> >> + dovdd-supply: > >> >> + description: > >> >> + Digital I/O voltage supply, 1.7-3.0 V > >> >> + > >> >> + avdd-supply: > >> >> + description: > >> >> + Analog voltage supply, 2.6-3.0 V > >> >> + > >> >> + dvdd-supply: > >> >> + description: > >> >> + Digital core voltage supply, 1.1-1.3 V > >> >> + > >> >> + powerdown-gpios: > >> >> + maxItems: 1 > >> >> + description: > >> >> + GPIO connected to the powerdown pin (active low) > >> >> + > >> >> + reset-gpios: > >> >> + maxItems: 1 > >> >> + description: > >> >> + GPIO connected to the reset pin (active low) > >> >> + > >> >> + orientation: true > >> >> + > >> >> + rotation: true > >> >> + > >> >> + port: > >> >> + $ref: /schemas/graph.yaml#/$defs/port-base > >> >> + additionalProperties: false > >> >> + description: > >> >> + Output port node, single endpoint describing the CSI-2 transmitter > >> >> + > >> >> + properties: > >> >> + endpoint: > >> >> + $ref: /schemas/media/video-interfaces.yaml# > >> >> + unevaluatedProperties: false > >> >> + > >> >> + properties: > >> >> + data-lanes: > >> >> + oneOf: > >> >> + - items: > >> >> + - const: 1 > >> >> + - const: 2 > >> >> + - const: 3 > >> >> + - const: 4 > >> >> + - items: > >> >> + - const: 1 > >> >> + - const: 2 > >> >> + - items: > >> >> + - const: 1 > >> >> + link-frequencies: true > >> >> + > >> >> + required: > >> >> + - data-lanes > >> >> + - link-frequencies > >> >> + > >> >> +required: > >> >> + - compatible > >> >> + - reg > >> >> + - clocks > >> >> + - clock-names > >> >> + - dovdd-supply > >> >> + - avdd-supply > >> >> + - dvdd-supply > >> >> + - powerdown-gpios > >> >> + - reset-gpios > >> >> + - port > >> > > >> > I think we don't need all of these entries as required. > >> > The only let me say "really" required are: > >> > > >> > - compatible > >> > - reg > >> > - clocks > >> > - port > >> > >> Thanks for the review! I agree that the driver may be modified to work > >> without powerdown and reset gpios and they are not required for sensor > >> operation. On contrary, supplies are obviously required. Of course, linux > >> provides dummy regulators if supplies are missing from device tree, but > >> I though the intention was to document hardware, not implementation > >> details. What do think of this? > > > > We have already discuss on this on the following thread sometimes ago :) > > > > https://www.patchwork.linux-fancy.com/project/linux-fancy/patch/20220630134835.592521-6-tommaso.merciai@amarulasolutions.com/ > > > > Take a look and let me know. > > Okay, if there already is a consensus regarding this matter, I'll make > the regulators optional in v3. I always request the opposite during reviews :-) Could we get an authoritative answer from the maintainers of the regulator framework on this question, and document it somewhere ?
diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml new file mode 100644 index 000000000000..376330b5572a --- /dev/null +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml @@ -0,0 +1,141 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/ovti,ov4689.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Omnivision OV4689 CMOS + +maintainers: + - Mikhail Rudenko <mike.rudenko@gmail.com> + +description: | + The Omnivision OV4689 is a high performance, 1/3-inch, 4 megapixel + image sensor. Ihis chip supports high frame rate speeds up to 90 fps + at 2688x1520 resolution. It is programmable through an I2C + interface, and sensor output is sent via 1/2/4 lane MIPI CSI-2 + connection. + +allOf: + - $ref: /schemas/media/video-interface-devices.yaml# + +properties: + compatible: + const: ovti,ov4689 + + reg: + maxItems: 1 + + clocks: + description: + External clock (XVCLK) for the sensor, 6-64 MHz + maxItems: 1 + + clock-names: true + + dovdd-supply: + description: + Digital I/O voltage supply, 1.7-3.0 V + + avdd-supply: + description: + Analog voltage supply, 2.6-3.0 V + + dvdd-supply: + description: + Digital core voltage supply, 1.1-1.3 V + + powerdown-gpios: + maxItems: 1 + description: + GPIO connected to the powerdown pin (active low) + + reset-gpios: + maxItems: 1 + description: + GPIO connected to the reset pin (active low) + + orientation: true + + rotation: true + + port: + $ref: /schemas/graph.yaml#/$defs/port-base + additionalProperties: false + description: + Output port node, single endpoint describing the CSI-2 transmitter + + properties: + endpoint: + $ref: /schemas/media/video-interfaces.yaml# + unevaluatedProperties: false + + properties: + data-lanes: + oneOf: + - items: + - const: 1 + - const: 2 + - const: 3 + - const: 4 + - items: + - const: 1 + - const: 2 + - items: + - const: 1 + link-frequencies: true + + required: + - data-lanes + - link-frequencies + +required: + - compatible + - reg + - clocks + - clock-names + - dovdd-supply + - avdd-supply + - dvdd-supply + - powerdown-gpios + - reset-gpios + - port + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + + i2c { + #address-cells = <1>; + #size-cells = <0>; + + ov4689: camera@36 { + compatible = "ovti,ov4689"; + reg = <0x36>; + + clocks = <&ov4689_clk>; + clock-names = "xvclk"; + + avdd-supply = <&ov4689_avdd>; + dovdd-supply = <&ov4689_dovdd>; + dvdd-supply = <&ov4689_dvdd>; + + powerdown-gpios = <&pio 107 GPIO_ACTIVE_LOW>; + reset-gpios = <&pio 109 GPIO_ACTIVE_LOW>; + + orientation = <2>; + rotation = <0>; + + port { + wcam_out: endpoint { + remote-endpoint = <&mipi_in_wcam>; + data-lanes = <1 2 3 4>; + link-frequencies = /bits/ 64 <500000000>; + }; + }; + }; + }; + +... diff --git a/MAINTAINERS b/MAINTAINERS index f468864fd268..63c4844f26e6 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14523,6 +14523,13 @@ S: Maintained T: git git://linuxtv.org/media_tree.git F: drivers/media/i2c/ov2740.c +OMNIVISION OV4689 SENSOR DRIVER +M: Mikhail Rudenko <mike.rudenko@gmail.com> +L: linux-media@vger.kernel.org +S: Maintained +T: git git://linuxtv.org/media_tree.git +F: Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml + OMNIVISION OV5640 SENSOR DRIVER M: Steve Longerbeam <slongerbeam@gmail.com> L: linux-media@vger.kernel.org
Add device-tree binding documentation for OV4689 image sensor driver, and the relevant MAINTAINERS entries. Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com> --- .../bindings/media/i2c/ovti,ov4689.yaml | 141 ++++++++++++++++++ MAINTAINERS | 7 + 2 files changed, 148 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml