Re: [EXT] Re: [PATCH v4 1/4] dt-bindings: media: i2c: Add OX05B1S sensor

[Mirela Rabulea <mirela.rabulea@nxp.com>]

Hi Sakari,

On 6/28/25 21:27, Sakari Ailus wrote:
> Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button
>
>
> Hi Mirela,
>
> On Wed, Mar 05, 2025 at 11:43:56AM +0200, Mirela Rabulea wrote:
>> Add bindings for Omnivision OX05B1S sensor.
>> Also add compatible for Omnivision OS08A20 sensor.
>>
>> Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> ---
>>
>> Changes in v4:
>>        Collect Reviewed-by
>>
>> Changes in v3:
>>        Use unevaluatedProperties: false and drop orientation/rotation
>>        Drop items and keep alphabetical order in compatible property
>>        Shorten the description for reset_gpio
>>        Make the supplies required.
>>        Use generic node name (camera instead of ox05b1s)
>>
>> Changes in v2:
>>        Small updates on description
>>        Update subject, drop "bindings" and "driver"
>>        Just one binding patch (squash os08a20 bindings)
>>        Re-flow to 80 columns.
>>        Drop clock name (not needed in case of single clock)
>>        Make the clock required property, strictly from sensor module point of view, it is mandatory (will use a fixed clock for nxp board)
>>        Add regulators: avdd, dvdd, dovdd
>>        Add $ref: /schemas/media/video-interface-devices.yaml
>>        Drop assigned-clock* properties (defined in clocks.yaml)
>>        Keep "additionalProperties : false" and orientation/rotation (unevaluatedProperties: false was suggested, but only orientation/rotation are needed from video-interface-devices.yaml)
>>        Include assigned-clock* in the example, for completeness sake (although it was also suggested to omit them)
>>
>>   .../bindings/media/i2c/ovti,ox05b1s.yaml      | 119 ++++++++++++++++++
>>   1 file changed, 119 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ox05b1s.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ox05b1s.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ox05b1s.yaml
>> new file mode 100644
>> index 000000000000..9f35b4e67bea
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ox05b1s.yaml
>> @@ -0,0 +1,119 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +# Copyright (C) 2024 NXP
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/media/i2c/ovti,ox05b1s.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Omnivision OX05B1S Image Sensor
>> +
>> +maintainers:
>> +  - Mirela Rabulea <mirela.rabulea@nxp.com>
>> +
>> +description:
>> +  The Omnivision OX05B1S is a 1/2.5-Inch CMOS image sensor with an active
>> +  array size of 2592 x 1944. It is programmable through I2C interface.
>> +  Image data is available via MIPI CSI-2 serial data output.
>> +
>> +allOf:
>> +  - $ref: /schemas/media/video-interface-devices.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - ovti,os08a20
>> +      - ovti,ox05b1s
> The bindings only describe ox05b1s. How are the two different?

Indeed, thanks for pointing out. I will update the description in _v5 
like this, to include also os08a20:

The Omnivision OX05B1S is a 1/2.5-Inch CMOS image sensor with an active
  array size of 2592 x 1944 and raw RGB-Ir output. It is programmable 
through
  I2C interface. Image data is available via MIPI CSI-2 serial data output.
  The Omnivision OS08A20 is a 1/1.8-Inch CMOS image sensor with an active
  array size of 3840 x 2160, raw RGB output and 2-exposure staggered HDR
  support.

>
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    description: Input clock (24 MHz)
>> +    maxItems: 1
> Is this really a hardware limitation, or what the driver is currently
> limited to? In the former case, I'd include the range.
No, hardware is not limited to 24 MHz. I will include a range like this:

description: Input clock (6-64 MHz for OX05B1S, 6-27 MHz for OS08A20)


Thanks for feedback,

Mirela

>
>> +
>> +  reset-gpios:
>> +    description: Active low XSHUTDOWN pin
>> +    maxItems: 1
>> +
>> +  avdd-supply:
>> +    description: Power for analog circuit (2.8V)
>> +
>> +  dovdd-supply:
>> +    description: Power for I/O circuit (1.8V)
>> +
>> +  dvdd-supply:
>> +    description: Power for digital circuit (1.2V)
>> +
>> +  port:
>> +    $ref: /schemas/graph.yaml#/$defs/port-base
>> +    additionalProperties: false
>> +    description: MIPI CSI-2 transmitter port
>> +
>> +    properties:
>> +      endpoint:
>> +        $ref: /schemas/media/video-interfaces.yaml#
>> +        unevaluatedProperties: false
>> +
>> +        properties:
>> +          data-lanes:
>> +            anyOf:
>> +              - items:
>> +                  - const: 1
>> +                  - const: 2
>> +              - items:
>> +                  - const: 1
>> +                  - const: 2
>> +                  - const: 3
>> +                  - const: 4
>> +        required:
>> +          - data-lanes
>> +
>> +    required:
>> +      - endpoint
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - clocks
>> +  - port
>> +  - avdd-supply
>> +  - dovdd-supply
>> +  - dvdd-supply
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/gpio/gpio.h>
>> +
>> +    i2c {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        camera@36 {
>> +            compatible = "ovti,ox05b1s";
>> +            reg = <0x36>;
>> +            clocks = <&ox05b1s_clk>;
>> +
>> +            assigned-clocks = <&ox05b1s_clk>;
>> +            assigned-clock-parents = <&ox05b1s_clk_parent>;
>> +            assigned-clock-rates = <24000000>;
>> +
>> +            reset-gpios = <&gpio1 6 GPIO_ACTIVE_LOW>;
>> +
>> +            avdd-supply = <&camera_avdd_2v8>;
>> +            dovdd-supply = <&camera_dovdd_1v8>;
>> +            dvdd-supply = <&camera_dvdd_1v2>;
>> +
>> +            orientation = <2>;
>> +            rotation = <0>;
>> +
>> +            port {
>> +                ox05b1s_mipi_0_ep: endpoint {
>> +                    remote-endpoint = <&mipi_csi0_ep>;
>> +                    data-lanes = <1 2 3 4>;
>> +                };
>> +            };
>> +        };
>> +    };
>> +...
> --
> Kind regards,
>
> Sakari Ailus