mbox series

[v2,0/2] media: i2c: Add driver for OmniVision OV8858

Message ID 20230106203909.184073-1-jacopo@jmondi.org
Headers show
Series media: i2c: Add driver for OmniVision OV8858 | expand

Message

Jacopo Mondi Jan. 6, 2023, 8:39 p.m. UTC 
Hello,
  this is a new version 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 ?

v1 -> v2:
- Unified and simplified modes
- Adjust blanking values to match FPS
- Address Laurent's comment:
  - rework reg_write() function
  - move to subdev state
  - cosmetic change


Jacopo Mondi (1):
  dt-bindings: media: Add OmniVision OV8858

Nicholas Roth (1):
  media: i2c: Add driver for OmniVision OV8858

 .../bindings/media/i2c/ovti,ov8858.yaml       |  105 +
 MAINTAINERS                                   |    9 +
 drivers/media/i2c/Kconfig                     |   13 +
 drivers/media/i2c/Makefile                    |    1 +
 drivers/media/i2c/ov8858.c                    | 1952 +++++++++++++++++
 5 files changed, 2080 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov8858.yaml
 create mode 100644 drivers/media/i2c/ov8858.c

--
2.38.1

Comments

Krzysztof Kozlowski Jan. 7, 2023, 1:09 p.m. UTC | #1 
On 06/01/2023 21:39, 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>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  .../bindings/media/i2c/ovti,ov8858.yaml       | 105 ++++++++++++++++++
>  1 file changed, 105 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..002461a974f8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov8858.yaml
> @@ -0,0 +1,105 @@
> +# 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 Megapixels (3264x2448) image sensor
> +  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:
> +    description: PWDNB powerdown GPIO (active low)
> +
> +  reset-gpios:
> +    description: XSHUTDN reset GPIO (active low)

Here you need maxItems. I did not propose to remove it here.


Best regards,
Krzysztof
Jacopo Mondi Jan. 7, 2023, 3:18 p.m. UTC | #2 
Hi Krzysztof

On Sat, Jan 07, 2023 at 02:09:54PM +0100, Krzysztof Kozlowski wrote:
> On 06/01/2023 21:39, 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>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  .../bindings/media/i2c/ovti,ov8858.yaml       | 105 ++++++++++++++++++
> >  1 file changed, 105 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..002461a974f8
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov8858.yaml
> > @@ -0,0 +1,105 @@
> > +# 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 Megapixels (3264x2448) image sensor
> > +  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:
> > +    description: PWDNB powerdown GPIO (active low)
> > +
> > +  reset-gpios:
> > +    description: XSHUTDN reset GPIO (active low)
>
> Here you need maxItems. I did not propose to remove it here.
>

I'm wondering why you can have multiple resets but not multiple
powerdowns.

Anyway, how is one supposed to know when maxItems is required or not,
where should I look ?

Thanks
   j

>
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Jan. 7, 2023, 3:30 p.m. UTC | #3 
On 07/01/2023 16:18, Jacopo Mondi wrote:
> Hi Krzysztof
> 
> On Sat, Jan 07, 2023 at 02:09:54PM +0100, Krzysztof Kozlowski wrote:
>> On 06/01/2023 21:39, 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>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> ---
>>>  .../bindings/media/i2c/ovti,ov8858.yaml       | 105 ++++++++++++++++++
>>>  1 file changed, 105 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..002461a974f8
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov8858.yaml
>>> @@ -0,0 +1,105 @@
>>> +# 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 Megapixels (3264x2448) image sensor
>>> +  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:
>>> +    description: PWDNB powerdown GPIO (active low)
>>> +
>>> +  reset-gpios:
>>> +    description: XSHUTDN reset GPIO (active low)
>>
>> Here you need maxItems. I did not propose to remove it here.
>>
> 
> I'm wondering why you can have multiple resets but not multiple
> powerdowns.
> 
> Anyway, how is one supposed to know when maxItems is required or not,
> where should I look ?

gpio-consumer-common.yaml

Best regards,
Krzysztof