[v2,1/2] media: dt-bindings: media: i2c: document OV4689 DT bindings

  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
  

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 ?