| Message ID | 20230105172320.133810-1-jacopo@jmondi.org |
|---|---|
| Headers | show |
| Series | media: i2c: Add driver for OmniVision OV8858 | expand |
On 05/01/2023 18:23, Jacopo Mondi wrote: > From: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > Subject: drop redundant "schema for". > Add binding schema for the OmniVision OV8858 8 Megapixels camera sensor. > Thank you for your patch. There is something to discuss/improve. > +properties: > + compatible: > + const: ovti,ov8858 > + > + reg: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + description: XVCLK external clock > + > + clock-names: > + const: xvclk > + > + dvdd-supply: > + description: Digital Domain Power Supply > + > + avdd-supply: > + description: Analog Domain Power Supply > + > + dovdd-supply: > + description: I/O Domain Power Supply > + > + powerdown-gpios: > + maxItems: 1 No need for maxItems here - it is coming from gpio-consumer-common. > + description: PWDNB powerdown GPIO (active low) > + > + reset-gpios: > + maxItems: 1 > + description: XSHUTDN reset GPIO (active low) > + > + port: > + description: MIPI CSI-2 transmitter port > + $ref: /schemas/graph.yaml#/$defs/port-base > + additionalProperties: false > + > + properties: > + endpoint: > + $ref: /schemas/media/video-interfaces.yaml# > + unevaluatedProperties: false > + > + properties: > + data-lanes: > + minItems: 1 > + maxItems: 4 > + > + required: > + - data-lanes > + > +required: > + - compatible > + - reg > + - clocks > + - assigned-clocks > + - assigned-clock-rates These should not be required. > + - port > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/pinctrl/rockchip.h> Drop, not needed. > + #include <dt-bindings/clock/rk3399-cru.h> > + #include <dt-bindings/gpio/gpio.h> > + > + i2c2 { i2c > + #address-cells = <1>; > + #size-cells = <0>; > + > + ov8858: camera@36 { > + compatible = "ovti,ov8858"; > + reg = <0x36>; > + > + clocks = <&cru SCLK_CIF_OUT>; > + clock-names = "xvclk"; > + assigned-clocks = <&cru SCLK_CIF_OUT>; > + assigned-clock-rates = <24000000>; > + > + dovdd-supply = <&vcc1v8_dvp>; > + > + reset-gpios = <&gpio1 RK_PA4 GPIO_ACTIVE_LOW>; > + powerdown-gpios = <&gpio2 RK_PB4 GPIO_ACTIVE_LOW>; > + > + port { > + ucam_out: endpoint { > + remote-endpoint = <&mipi_in_ucam>; > + data-lanes = <1 2 3 4>; > + }; > + }; > + }; > + }; > +... > -- > 2.38.1 > Best regards, Krzysztof
Hi Krzysztof thanks for the review On Fri, Jan 06, 2023 at 09:34:22AM +0100, Krzysztof Kozlowski wrote: > On 05/01/2023 18:23, Jacopo Mondi wrote: > > From: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > Subject: drop redundant "schema for". > ack > > Add binding schema for the OmniVision OV8858 8 Megapixels camera sensor. > > > > Thank you for your patch. There is something to discuss/improve. > > > +properties: > > + compatible: > > + const: ovti,ov8858 > > + > > + reg: > > + maxItems: 1 > > + > > + clocks: > > + maxItems: 1 > > + description: XVCLK external clock > > + > > + clock-names: > > + const: xvclk > > + > > + dvdd-supply: > > + description: Digital Domain Power Supply > > + > > + avdd-supply: > > + description: Analog Domain Power Supply > > + > > + dovdd-supply: > > + description: I/O Domain Power Supply > > + > > + powerdown-gpios: > > + maxItems: 1 > > No need for maxItems here - it is coming from gpio-consumer-common. > ack > > + description: PWDNB powerdown GPIO (active low) > > + > > + reset-gpios: > > + maxItems: 1 > > + description: XSHUTDN reset GPIO (active low) > > + > > + port: > > + description: MIPI CSI-2 transmitter port > > + $ref: /schemas/graph.yaml#/$defs/port-base > > + additionalProperties: false > > + > > + properties: > > + endpoint: > > + $ref: /schemas/media/video-interfaces.yaml# > > + unevaluatedProperties: false > > + > > + properties: > > + data-lanes: > > + minItems: 1 > > + maxItems: 4 > > + > > + required: > > + - data-lanes > > + > > +required: > > + - compatible > > + - reg > > + - clocks > > + - assigned-clocks > > + - assigned-clock-rates > > These should not be required. > makes sense > > + - port > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/pinctrl/rockchip.h> > > Drop, not needed. > I need it for the definition of RK_PA4 and RK_PB4. The example fails to compile if I remove it > > + #include <dt-bindings/clock/rk3399-cru.h> > > + #include <dt-bindings/gpio/gpio.h> > > + > > + i2c2 { > > i2c > Ack Will resend soon Thanks j > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + ov8858: camera@36 { > > + compatible = "ovti,ov8858"; > > + reg = <0x36>; > > + > > + clocks = <&cru SCLK_CIF_OUT>; > > + clock-names = "xvclk"; > > + assigned-clocks = <&cru SCLK_CIF_OUT>; > > + assigned-clock-rates = <24000000>; > > + > > + dovdd-supply = <&vcc1v8_dvp>; > > + > > + reset-gpios = <&gpio1 RK_PA4 GPIO_ACTIVE_LOW>; > > + powerdown-gpios = <&gpio2 RK_PB4 GPIO_ACTIVE_LOW>; > > + > > + port { > > + ucam_out: endpoint { > > + remote-endpoint = <&mipi_in_ucam>; > > + data-lanes = <1 2 3 4>; > > + }; > > + }; > > + }; > > + }; > > +... > > -- > > 2.38.1 > > > > Best regards, > Krzysztof >
On 06/01/2023 10:15, Jacopo Mondi wrote: > >>> + - port >>> + >>> +additionalProperties: false >>> + >>> +examples: >>> + - | >>> + #include <dt-bindings/pinctrl/rockchip.h> >> >> Drop, not needed. >> > > I need it for the definition of RK_PA4 and RK_PB4. > > The example fails to compile if I remove it Ah, right. Best regards, Krzysztof
Hi Jacopo, Thank you for the patch. One comment in addition to Krzysztof's. On Thu, Jan 05, 2023 at 06:23:19PM +0100, Jacopo Mondi wrote: > From: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Add binding schema for the OmniVision OV8858 8 Megapixels camera sensor. > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > .../bindings/media/i2c/ovti,ov8858.yaml | 109 ++++++++++++++++++ > 1 file changed, 109 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov8858.yaml > > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov8858.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov8858.yaml > new file mode 100644 > index 000000000000..f6d5cf69234c > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov8858.yaml > @@ -0,0 +1,109 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/i2c/ovti,ov8858.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: OmniVision OV8858 Image Sensor > + > +maintainers: > + - Jacopo Mondi <jacopo.mondi@ideasonboard.com> > + - Nicholas Roth <nicholas@rothemail.net> > + > +description: | > + The OmniVision OV8858 is a color CMOS 8 Megapixles (3264x2448) image sensor s/pixles/pixels/ Conditionally-Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> with this and the issues pointed by Krzysztof first. > + controlled through an I2C-compatible SCCB bus. The sensor transmits images > + on a MIPI CSI-2 output interface with up to 4 data lanes. > + > +properties: > + compatible: > + const: ovti,ov8858 > + > + reg: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + description: XVCLK external clock > + > + clock-names: > + const: xvclk > + > + dvdd-supply: > + description: Digital Domain Power Supply > + > + avdd-supply: > + description: Analog Domain Power Supply > + > + dovdd-supply: > + description: I/O Domain Power Supply > + > + powerdown-gpios: > + maxItems: 1 > + description: PWDNB powerdown GPIO (active low) > + > + reset-gpios: > + maxItems: 1 > + description: XSHUTDN reset GPIO (active low) > + > + port: > + description: MIPI CSI-2 transmitter port > + $ref: /schemas/graph.yaml#/$defs/port-base > + additionalProperties: false > + > + properties: > + endpoint: > + $ref: /schemas/media/video-interfaces.yaml# > + unevaluatedProperties: false > + > + properties: > + data-lanes: > + minItems: 1 > + maxItems: 4 > + > + required: > + - data-lanes > + > +required: > + - compatible > + - reg > + - clocks > + - assigned-clocks > + - assigned-clock-rates > + - port > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/pinctrl/rockchip.h> > + #include <dt-bindings/clock/rk3399-cru.h> > + #include <dt-bindings/gpio/gpio.h> > + > + i2c2 { > + #address-cells = <1>; > + #size-cells = <0>; > + > + ov8858: camera@36 { > + compatible = "ovti,ov8858"; > + reg = <0x36>; > + > + clocks = <&cru SCLK_CIF_OUT>; > + clock-names = "xvclk"; > + assigned-clocks = <&cru SCLK_CIF_OUT>; > + assigned-clock-rates = <24000000>; > + > + dovdd-supply = <&vcc1v8_dvp>; > + > + reset-gpios = <&gpio1 RK_PA4 GPIO_ACTIVE_LOW>; > + powerdown-gpios = <&gpio2 RK_PB4 GPIO_ACTIVE_LOW>; > + > + port { > + ucam_out: endpoint { > + remote-endpoint = <&mipi_in_ucam>; > + data-lanes = <1 2 3 4>; > + }; > + }; > + }; > + }; > +...
From: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Hello, this is a new version of of Nicholas' first submission available at https://lore.kernel.org/all/20221106171129.166892-2-nicholas@rothemail.net/ This is a re-write of the previous version so I've restarted numeration from 0 even if in this version I have not changed the register tables. Functionally the most notable difference is the addition of support for binned mode in 4 data lanes mode. There is more space for optimization on top of this series, in example the register tables can be reworked to share more common settings between modes. I have started the effort, but as it is a tedious and error-prone work I would rather have this first version merged and the iterate on top. Programming of the analog crop rectangle and output size could also be made parametric, but the current modes have a few undocumented registers and I didn't feel like biting the bullet and see if the can be safely removed or not. Also, the BSP driver mentions a "not well supported" R1A version of the chip. I would be in favour of removing it if no one oppose. Images are still rather "dark" when tested with libcamera but preview is working as expected in both modes (full res and half-res binned mode) and with 2 and 4 data lanes. Nicholas could you let me know if things are fine with you here ? Thanks j Jacopo Mondi (1): dt-bindings: media: Add schema for OmniVision OV8858 Nicholas Roth (1): media: i2c: Add driver for OmniVision OV8858 .../bindings/media/i2c/ovti,ov8858.yaml | 109 + MAINTAINERS | 9 + drivers/media/i2c/Kconfig | 13 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/ov8858.c | 1989 +++++++++++++++++ 5 files changed, 2121 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov8858.yaml create mode 100644 drivers/media/i2c/ov8858.c -- 2.38.1