[PATCH 1/6] media: dt-bindings: i2c: Document ov5670

[PATCH 1/6] media: dt-bindings: i2c: Document ov5670

[Jacopo Mondi]

Provide the bindings documentation for Omnivision OV5670 image sensor.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 .../devicetree/bindings/media/i2c/ov5670.yaml | 93 +++++++++++++++++++
 1 file changed, 93 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5670.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
new file mode 100644
index 000000000000..dc4a3297bf6f
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
@@ -0,0 +1,93 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/ov5670.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Omnivision OV5670 5 Megapixels raw image sensor
+
+maintainers:
+  - Jacopo Mondi <jacopo@jmondi.org>
+
+description: |-
+  The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits
+  RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is
+  controlled through an I2C compatible control bus.
+
+properties:
+  compatible:
+    const: ovti,ov5670
+
+  reg:
+    maxItems: 1
+
+  clock-frequency:
+    description: Frequency of the xclk clock.
+
+  pwdn-gpios:
+    description: Reference to the GPIO connected to the PWDNB pin. Active low.
+
+  reset-gpios:
+    description:
+      Reference to the GPIO connected to the XSHUTDOWN pin. Active low.
+
+  avdd-supply:
+    description: Analog circuit power. Typically 2.8V.
+
+  dvdd-supply:
+    description: Digital circuit power. Typically 1.2V.
+
+  dovdd-supply:
+    description: Digital I/O circuit power. Typically 2.8V or 1.8V.
+
+  port:
+    $ref: /schemas/graph.yaml#/$defs/port-base
+    additionalProperties: false
+
+    properties:
+      endpoint:
+        $ref: /schemas/media/video-interfaces.yaml#
+        unevaluatedProperties: false
+
+        properties:
+          data-lanes:
+            description: The sensor supports 1 or 2 data lanes operations.
+            minItems: 1
+            maxItems: 2
+            items:
+              maximum: 2
+
+          clock-noncontinuous: true
+
+required:
+  - compatible
+  - reg
+  - clock-frequency
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ov5670: sensor@36 {
+            compatible = "ovti,ov5670";
+            reg = <0x36>;
+
+            clock-frequency=<19200000>;
+
+            port {
+                endpoint {
+                    remote-endpoint = <&csi_ep>;
+                    data-lanes = <1 2>;
+                    clock-noncontinuous;
+                };
+            };
+        };
+    };
+
+...
+
--
2.35.1

Re: [PATCH 1/6] media: dt-bindings: i2c: Document ov5670

[Krzysztof Kozlowski]

On 10/03/2022 14:08, Jacopo Mondi wrote:
> Provide the bindings documentation for Omnivision OV5670 image sensor.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  .../devicetree/bindings/media/i2c/ov5670.yaml | 93 +++++++++++++++++++

Add the file to maintainers entry.

>  1 file changed, 93 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> new file mode 100644
> index 000000000000..dc4a3297bf6f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml

Missing vendor prefix in file name.

> @@ -0,0 +1,93 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/ov5670.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Omnivision OV5670 5 Megapixels raw image sensor
> +
> +maintainers:
> +  - Jacopo Mondi <jacopo@jmondi.org>

Please add also driver maintainer.

> +
> +description: |-
> +  The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits
> +  RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is
> +  controlled through an I2C compatible control bus.
> +
> +properties:
> +  compatible:
> +    const: ovti,ov5670
> +
> +  reg:
> +    maxItems: 1
> +
> +  clock-frequency:
> +    description: Frequency of the xclk clock.

Is the xclk external clock coming to the sensor? If yes, there should be
a "clocks" property.

> +
> +  pwdn-gpios:
> +    description: Reference to the GPIO connected to the PWDNB pin. Active low.

maxItems

> +
> +  reset-gpios:
> +    description:
> +      Reference to the GPIO connected to the XSHUTDOWN pin. Active low.

maxItems

> +
> +  avdd-supply:
> +    description: Analog circuit power. Typically 2.8V.
> +
> +  dvdd-supply:
> +    description: Digital circuit power. Typically 1.2V.
> +
> +  dovdd-supply:
> +    description: Digital I/O circuit power. Typically 2.8V or 1.8V.
> +
> +  port:
> +    $ref: /schemas/graph.yaml#/$defs/port-base
> +    additionalProperties: false
> +
> +    properties:
> +      endpoint:
> +        $ref: /schemas/media/video-interfaces.yaml#
> +        unevaluatedProperties: false
> +
> +        properties:
> +          data-lanes:
> +            description: The sensor supports 1 or 2 data lanes operations.
> +            minItems: 1
> +            maxItems: 2
> +            items:
> +              maximum: 2

Is '0' also allowed? If not then maybe 'enum: [ 1, 2 ]'

no clock-lanes?

> +
> +          clock-noncontinuous: true
> +
> +required:
> +  - compatible
> +  - reg
> +  - clock-frequency
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c0 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        ov5670: sensor@36 {
> +            compatible = "ovti,ov5670";
> +            reg = <0x36>;
> +
> +            clock-frequency=<19200000>;

Missing spaces around '='.



Best regards,
Krzysztof

Re: [PATCH 1/6] media: dt-bindings: i2c: Document ov5670

[Jacopo Mondi]

Hi Krzysztof

On Thu, Mar 10, 2022 at 03:29:24PM +0100, Krzysztof Kozlowski wrote:
> On 10/03/2022 14:08, Jacopo Mondi wrote:
> > Provide the bindings documentation for Omnivision OV5670 image sensor.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  .../devicetree/bindings/media/i2c/ov5670.yaml | 93 +++++++++++++++++++
>
> Add the file to maintainers entry.
>

Right

> >  1 file changed, 93 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> > new file mode 100644
> > index 000000000000..dc4a3297bf6f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
>
> Missing vendor prefix in file name.
>

Right x2

> > @@ -0,0 +1,93 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/i2c/ov5670.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Omnivision OV5670 5 Megapixels raw image sensor
> > +
> > +maintainers:
> > +  - Jacopo Mondi <jacopo@jmondi.org>
>
> Please add also driver maintainer.
>

I never got what the policy was, if the maintainer entries here only
refer to the binding file or to the driver too

> > +
> > +description: |-
> > +  The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits
> > +  RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is
> > +  controlled through an I2C compatible control bus.
> > +
> > +properties:
> > +  compatible:
> > +    const: ovti,ov5670
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clock-frequency:
> > +    description: Frequency of the xclk clock.
>
> Is the xclk external clock coming to the sensor? If yes, there should be
> a "clocks" property.
>

To be honest I was not sure about this, as clock-frequency is already
used by the driver for the ACPI part, but it seems to in DT bindings
it is a property meant to be specified in the clock providers, even if
Documentation/devicetree/bindings/clock/clock-bindings.txt doesn't
really clarify this

Clock consumer should rather use 'clocks' and point to the provider's
phandle if my understanding is right.

> > +
> > +  pwdn-gpios:
> > +    description: Reference to the GPIO connected to the PWDNB pin. Active low.
>
> maxItems
>

I thought it was not necessary with a single description: entry. But
looking at the dt-schema source I fail to find any commit mentioning
that.

> > +
> > +  reset-gpios:
> > +    description:
> > +      Reference to the GPIO connected to the XSHUTDOWN pin. Active low.
>
> maxItems
>
> > +
> > +  avdd-supply:
> > +    description: Analog circuit power. Typically 2.8V.
> > +
> > +  dvdd-supply:
> > +    description: Digital circuit power. Typically 1.2V.
> > +
> > +  dovdd-supply:
> > +    description: Digital I/O circuit power. Typically 2.8V or 1.8V.
> > +
> > +  port:
> > +    $ref: /schemas/graph.yaml#/$defs/port-base
> > +    additionalProperties: false
> > +
> > +    properties:
> > +      endpoint:
> > +        $ref: /schemas/media/video-interfaces.yaml#
> > +        unevaluatedProperties: false
> > +
> > +        properties:
> > +          data-lanes:
> > +            description: The sensor supports 1 or 2 data lanes operations.
> > +            minItems: 1
> > +            maxItems: 2
> > +            items:
> > +              maximum: 2
>
> Is '0' also allowed? If not then maybe 'enum: [ 1, 2 ]'

No 0 is not allowed, but the data-lanes properties should accept any
of the following combinations
        <1>
        <1 2>
        <2 1>

As the chip seems to support lane re-ordering.

using enum would allow to between <1> or <2> if I got it right?

as the data-lane property is defined in video-interfaces.yaml

  data-lanes:
    $ref: /schemas/types.yaml#/definitions/uint32-array
    minItems: 1
    maxItems: 8
    items:
      # Assume up to 9 physical lane indices
      maximum: 8
    description:
      An array of physical data lane indexes. Position of an entry determines
      the logical lane number, while the value of an entry indicates physical
      lane, e.g. for 2-lane MIPI CSI-2 bus we could have "data-lanes = <1 2>;",
      assuming the clock lane is on hardware lane 0. If the hardware does not
      support lane reordering, monotonically incremented values shall be used
      from 0 or 1 onwards, depending on whether or not there is also a clock
      lane. This property is valid for serial busses only (e.g. MIPI CSI-2).

I did the same but restricted the max number of items to 2, and the
maximum value to 2 as well

>
> no clock-lanes?
>

clock lane is fixed on lane #0 afaict
`
> > +
> > +          clock-noncontinuous: true
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clock-frequency
> > +  - port
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c0 {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        ov5670: sensor@36 {
> > +            compatible = "ovti,ov5670";
> > +            reg = <0x36>;
> > +
> > +            clock-frequency=<19200000>;
>
> Missing spaces around '='.

ouch, thanks for spotting

Thanks
  j

>
>
>
> Best regards,
> Krzysztof

Re: [PATCH 1/6] media: dt-bindings: i2c: Document ov5670

[Krzysztof Kozlowski]

On 10/03/2022 18:16, Jacopo Mondi wrote:
> Hi Krzysztof
> 
> On Thu, Mar 10, 2022 at 03:29:24PM +0100, Krzysztof Kozlowski wrote:
>> On 10/03/2022 14:08, Jacopo Mondi wrote:
>>> Provide the bindings documentation for Omnivision OV5670 image sensor.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>> ---
>>>  .../devicetree/bindings/media/i2c/ov5670.yaml | 93 +++++++++++++++++++
>>
>> Add the file to maintainers entry.
>>
> 
> Right
> 
>>>  1 file changed, 93 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5670.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
>>> new file mode 100644
>>> index 000000000000..dc4a3297bf6f
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
>>
>> Missing vendor prefix in file name.
>>
> 
> Right x2
> 
>>> @@ -0,0 +1,93 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/media/i2c/ov5670.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Omnivision OV5670 5 Megapixels raw image sensor
>>> +
>>> +maintainers:
>>> +  - Jacopo Mondi <jacopo@jmondi.org>
>>
>> Please add also driver maintainer.
>>
> 
> I never got what the policy was, if the maintainer entries here only
> refer to the binding file or to the driver too

It is a person responsible for the bindings, so indeed it might not feed
existing maintainer.

> 
>>> +
>>> +description: |-
>>> +  The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits
>>> +  RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is
>>> +  controlled through an I2C compatible control bus.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: ovti,ov5670
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  clock-frequency:
>>> +    description: Frequency of the xclk clock.
>>
>> Is the xclk external clock coming to the sensor? If yes, there should be
>> a "clocks" property.
>>
> 
> To be honest I was not sure about this, as clock-frequency is already
> used by the driver for the ACPI part, but it seems to in DT bindings
> it is a property meant to be specified in the clock providers, even if
> Documentation/devicetree/bindings/clock/clock-bindings.txt doesn't
> really clarify this
> 
> Clock consumer should rather use 'clocks' and point to the provider's
> phandle if my understanding is right.

This is a clock-frequency, not clock reference. For external clocks, a
clock phandles + assigned-clock-rates should be rather used. However for
internal clocks, this is a perfectly valid property.

Therefore the question is - what is the "xclk"?

> 
>>> +
>>> +  pwdn-gpios:
>>> +    description: Reference to the GPIO connected to the PWDNB pin. Active low.
>>
>> maxItems
>>
> 
> I thought it was not necessary with a single description: entry. But
> looking at the dt-schema source I fail to find any commit mentioning
> that.

The purpose of maxItems is to constrain the number of GPIOs, so two
would be incorrect.

> 
>>> +
>>> +  reset-gpios:
>>> +    description:
>>> +      Reference to the GPIO connected to the XSHUTDOWN pin. Active low.
>>
>> maxItems
>>
>>> +
>>> +  avdd-supply:
>>> +    description: Analog circuit power. Typically 2.8V.
>>> +
>>> +  dvdd-supply:
>>> +    description: Digital circuit power. Typically 1.2V.
>>> +
>>> +  dovdd-supply:
>>> +    description: Digital I/O circuit power. Typically 2.8V or 1.8V.
>>> +
>>> +  port:
>>> +    $ref: /schemas/graph.yaml#/$defs/port-base
>>> +    additionalProperties: false
>>> +
>>> +    properties:
>>> +      endpoint:
>>> +        $ref: /schemas/media/video-interfaces.yaml#
>>> +        unevaluatedProperties: false
>>> +
>>> +        properties:
>>> +          data-lanes:
>>> +            description: The sensor supports 1 or 2 data lanes operations.
>>> +            minItems: 1
>>> +            maxItems: 2
>>> +            items:
>>> +              maximum: 2
>>
>> Is '0' also allowed? If not then maybe 'enum: [ 1, 2 ]'
> 
> No 0 is not allowed, but the data-lanes properties should accept any
> of the following combinations
>         <1>
>         <1 2>
>         <2 1>
> 
> As the chip seems to support lane re-ordering.
> 
> using enum would allow to between <1> or <2> if I got it right?

Yeah, enum would be equivalent. I find it more readable, than min+max,
but it's not a strong preference.

> 
> as the data-lane property is defined in video-interfaces.yaml
> 
>   data-lanes:
>     $ref: /schemas/types.yaml#/definitions/uint32-array
>     minItems: 1
>     maxItems: 8
>     items:
>       # Assume up to 9 physical lane indices
>       maximum: 8
>     description:
>       An array of physical data lane indexes. Position of an entry determines
>       the logical lane number, while the value of an entry indicates physical
>       lane, e.g. for 2-lane MIPI CSI-2 bus we could have "data-lanes = <1 2>;",
>       assuming the clock lane is on hardware lane 0. If the hardware does not
>       support lane reordering, monotonically incremented values shall be used
>       from 0 or 1 onwards, depending on whether or not there is also a clock
>       lane. This property is valid for serial busses only (e.g. MIPI CSI-2).
> 
> I did the same but restricted the max number of items to 2, and the
> maximum value to 2 as well

Makes sense, but you should also restrict the minimum (so not 0). enum
solves this.

> 
>>
>> no clock-lanes?
>>
> 
> clock lane is fixed on lane #0 afaict

ok


Best regards,
Krzysztof

Re: [PATCH 1/6] media: dt-bindings: i2c: Document ov5670

[Rob Herring]

On Thu, Mar 10, 2022 at 06:26:02PM +0100, Krzysztof Kozlowski wrote:
> On 10/03/2022 18:16, Jacopo Mondi wrote:
> > Hi Krzysztof
> > 
> > On Thu, Mar 10, 2022 at 03:29:24PM +0100, Krzysztof Kozlowski wrote:
> >> On 10/03/2022 14:08, Jacopo Mondi wrote:
> >>> Provide the bindings documentation for Omnivision OV5670 image sensor.
> >>>
> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>> ---
> >>>  .../devicetree/bindings/media/i2c/ov5670.yaml | 93 +++++++++++++++++++
> >>
> >> Add the file to maintainers entry.
> >>
> > 
> > Right
> > 
> >>>  1 file changed, 93 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> >>> new file mode 100644
> >>> index 000000000000..dc4a3297bf6f
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> >>
> >> Missing vendor prefix in file name.
> >>
> > 
> > Right x2
> > 
> >>> @@ -0,0 +1,93 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/media/i2c/ov5670.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Omnivision OV5670 5 Megapixels raw image sensor
> >>> +
> >>> +maintainers:
> >>> +  - Jacopo Mondi <jacopo@jmondi.org>
> >>
> >> Please add also driver maintainer.
> >>
> > 
> > I never got what the policy was, if the maintainer entries here only
> > refer to the binding file or to the driver too
> 
> It is a person responsible for the bindings, so indeed it might not feed
> existing maintainer.

No need for a MAINTAINERS entry as get_maintainers.pl will pick it up 
from here.

Rob

Re: [PATCH 1/6] media: dt-bindings: i2c: Document ov5670

[Jacopo Mondi]

Hi Krzysztof,

On Thu, Mar 10, 2022 at 06:26:02PM +0100, Krzysztof Kozlowski wrote:
> On 10/03/2022 18:16, Jacopo Mondi wrote:
> > Hi Krzysztof
> >
> > On Thu, Mar 10, 2022 at 03:29:24PM +0100, Krzysztof Kozlowski wrote:
> >> On 10/03/2022 14:08, Jacopo Mondi wrote:
> >>> Provide the bindings documentation for Omnivision OV5670 image sensor.
> >>>
> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>> ---
> >>>  .../devicetree/bindings/media/i2c/ov5670.yaml | 93 +++++++++++++++++++
> >>
> >> Add the file to maintainers entry.
> >>
> >
> > Right
> >
> >>>  1 file changed, 93 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> >>> new file mode 100644
> >>> index 000000000000..dc4a3297bf6f
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> >>
> >> Missing vendor prefix in file name.
> >>
> >
> > Right x2
> >
> >>> @@ -0,0 +1,93 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/media/i2c/ov5670.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Omnivision OV5670 5 Megapixels raw image sensor
> >>> +
> >>> +maintainers:
> >>> +  - Jacopo Mondi <jacopo@jmondi.org>
> >>
> >> Please add also driver maintainer.
> >>
> >
> > I never got what the policy was, if the maintainer entries here only
> > refer to the binding file or to the driver too
>
> It is a person responsible for the bindings, so indeed it might not feed
> existing maintainer.
>
> >
> >>> +
> >>> +description: |-
> >>> +  The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits
> >>> +  RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is
> >>> +  controlled through an I2C compatible control bus.
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    const: ovti,ov5670
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  clock-frequency:
> >>> +    description: Frequency of the xclk clock.
> >>
> >> Is the xclk external clock coming to the sensor? If yes, there should be
> >> a "clocks" property.
> >>
> >
> > To be honest I was not sure about this, as clock-frequency is already
> > used by the driver for the ACPI part, but it seems to in DT bindings
> > it is a property meant to be specified in the clock providers, even if
> > Documentation/devicetree/bindings/clock/clock-bindings.txt doesn't
> > really clarify this
> >
> > Clock consumer should rather use 'clocks' and point to the provider's
> > phandle if my understanding is right.
>
> This is a clock-frequency, not clock reference. For external clocks, a

Yes, I was suggesting to replace clock-frequency with clocks, that
accepts a phandle.

The thing is, the driver parses 'clock-frequency'
	device_property_read_u32(&client->dev, "clock-frequency", &input_clk);

which I assume comes from ACPI (as the driver was developed for an
ACPI platform).

If in DTS we don't use it, I then need to

#ifdef CONFIG_ACPI

#elif defined CONFIG_OF

#endif

Which I would really like to avoid.

Anyone with ACPI experience that knows where clock-frequency comes
from ?

> clock phandles + assigned-clock-rates should be rather used. However for
> internal clocks, this is a perfectly valid property.
>
> Therefore the question is - what is the "xclk"?

xclk is the clock fed to the sensor, which which all its internal
clocks are generated, so it's indeed an 'external' clock. As I've
said, clock-frequency seems to be meant for clock providers, and
the image sensor is a clock consumer.

>
> >
> >>> +
> >>> +  pwdn-gpios:
> >>> +    description: Reference to the GPIO connected to the PWDNB pin. Active low.
> >>
> >> maxItems
> >>
> >
> > I thought it was not necessary with a single description: entry. But
> > looking at the dt-schema source I fail to find any commit mentioning
> > that.
>
> The purpose of maxItems is to constrain the number of GPIOs, so two
> would be incorrect.
>

I recall that with a single description entry then maxItems: 1 was
assumed by the dt-schema validator, but I cannot find references to
any commit, so I'll add it.

> >
> >>> +
> >>> +  reset-gpios:
> >>> +    description:
> >>> +      Reference to the GPIO connected to the XSHUTDOWN pin. Active low.
> >>
> >> maxItems
> >>
> >>> +
> >>> +  avdd-supply:
> >>> +    description: Analog circuit power. Typically 2.8V.
> >>> +
> >>> +  dvdd-supply:
> >>> +    description: Digital circuit power. Typically 1.2V.
> >>> +
> >>> +  dovdd-supply:
> >>> +    description: Digital I/O circuit power. Typically 2.8V or 1.8V.
> >>> +
> >>> +  port:
> >>> +    $ref: /schemas/graph.yaml#/$defs/port-base
> >>> +    additionalProperties: false
> >>> +
> >>> +    properties:
> >>> +      endpoint:
> >>> +        $ref: /schemas/media/video-interfaces.yaml#
> >>> +        unevaluatedProperties: false
> >>> +
> >>> +        properties:
> >>> +          data-lanes:
> >>> +            description: The sensor supports 1 or 2 data lanes operations.
> >>> +            minItems: 1
> >>> +            maxItems: 2
> >>> +            items:
> >>> +              maximum: 2
> >>
> >> Is '0' also allowed? If not then maybe 'enum: [ 1, 2 ]'
> >
> > No 0 is not allowed, but the data-lanes properties should accept any
> > of the following combinations
> >         <1>
> >         <1 2>
> >         <2 1>
> >
> > As the chip seems to support lane re-ordering.
> >
> > using enum would allow to between <1> or <2> if I got it right?
>
> Yeah, enum would be equivalent. I find it more readable, than min+max,
> but it's not a strong preference.
>

I don't think enum is equivalent, as it specifies a set of valid values
a property can assume, but it does not support arrays.

https://json-schema.org/draft/2020-12/json-schema-validation.html#rfc.section.6.16.1.2.

enum
  The value of this keyword MUST be an array. This array SHOULD have
  at least one element. Elements in the array SHOULD be unique.

  An instance validates successfully against this keyword if its value
  is equal to one of the elements in this keyword's array value.

In facts:

--- a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
@@ -52,10 +52,7 @@ properties:
         properties:
           data-lanes:
             description: The sensor supports 1 or 2 data lanes operations.
-            minItems: 1
-            maxItems: 2
-            items:
-              maximum: 2
+            enum: [1, 2]


Results in:

Documentation/devicetree/bindings/media/i2c/ov5670.example.dt.yaml: sensor@36: port:endpoint:data-lanes:0: [1, 2] is too long

If instead

--- a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
@@ -54,8 +54,7 @@ properties:
             description: The sensor supports 1 or 2 data lanes operations.
             minItems: 1
             maxItems: 2
-            items:
-              maximum: 2
+            enum: [1, 2]

I get
Documentation/devicetree/bindings/media/i2c/ov5670.yaml:
properties:port:properties:endpoint:properties:data-lanes:
'enum' should not be valid under {'enum': ['const', 'enum', 'exclusiveMaximum', 'exclusiveMinimum', 'minimum', 'maximum', 'multipleOf', 'pattern']}
	hint: Scalar and array keywords cannot be mixed
	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#

Thanks
   j

> >
> > as the data-lane property is defined in video-interfaces.yaml
> >
> >   data-lanes:
> >     $ref: /schemas/types.yaml#/definitions/uint32-array
> >     minItems: 1
> >     maxItems: 8
> >     items:
> >       # Assume up to 9 physical lane indices
> >       maximum: 8
> >     description:
> >       An array of physical data lane indexes. Position of an entry determines
> >       the logical lane number, while the value of an entry indicates physical
> >       lane, e.g. for 2-lane MIPI CSI-2 bus we could have "data-lanes = <1 2>;",
> >       assuming the clock lane is on hardware lane 0. If the hardware does not
> >       support lane reordering, monotonically incremented values shall be used
> >       from 0 or 1 onwards, depending on whether or not there is also a clock
> >       lane. This property is valid for serial busses only (e.g. MIPI CSI-2).
> >
> > I did the same but restricted the max number of items to 2, and the
> > maximum value to 2 as well
>
> Makes sense, but you should also restrict the minimum (so not 0). enum
> solves this.
>
> >
> >>
> >> no clock-lanes?
> >>
> >
> > clock lane is fixed on lane #0 afaict
>
> ok
>
>
> Best regards,
> Krzysztof

Re: [PATCH 1/6] media: dt-bindings: i2c: Document ov5670

[Krzysztof Kozlowski]

On 11/03/2022 17:05, Jacopo Mondi wrote:
> Hi Krzysztof,
> 
> On Thu, Mar 10, 2022 at 06:26:02PM +0100, Krzysztof Kozlowski wrote:
>> On 10/03/2022 18:16, Jacopo Mondi wrote:
>>> Hi Krzysztof
>>>
>>> On Thu, Mar 10, 2022 at 03:29:24PM +0100, Krzysztof Kozlowski wrote:
>>>> On 10/03/2022 14:08, Jacopo Mondi wrote:
>>>>> Provide the bindings documentation for Omnivision OV5670 image sensor.
>>>>>
>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>>>> ---
>>>>>  .../devicetree/bindings/media/i2c/ov5670.yaml | 93 +++++++++++++++++++
>>>>
>>>> Add the file to maintainers entry.
>>>>
>>>
>>> Right
>>>
>>>>>  1 file changed, 93 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5670.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..dc4a3297bf6f
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
>>>>
>>>> Missing vendor prefix in file name.
>>>>
>>>
>>> Right x2
>>>
>>>>> @@ -0,0 +1,93 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/media/i2c/ov5670.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Omnivision OV5670 5 Megapixels raw image sensor
>>>>> +
>>>>> +maintainers:
>>>>> +  - Jacopo Mondi <jacopo@jmondi.org>
>>>>
>>>> Please add also driver maintainer.
>>>>
>>>
>>> I never got what the policy was, if the maintainer entries here only
>>> refer to the binding file or to the driver too
>>
>> It is a person responsible for the bindings, so indeed it might not feed
>> existing maintainer.
>>
>>>
>>>>> +
>>>>> +description: |-
>>>>> +  The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits
>>>>> +  RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is
>>>>> +  controlled through an I2C compatible control bus.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: ovti,ov5670
>>>>> +
>>>>> +  reg:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  clock-frequency:
>>>>> +    description: Frequency of the xclk clock.
>>>>
>>>> Is the xclk external clock coming to the sensor? If yes, there should be
>>>> a "clocks" property.
>>>>
>>>
>>> To be honest I was not sure about this, as clock-frequency is already
>>> used by the driver for the ACPI part, but it seems to in DT bindings
>>> it is a property meant to be specified in the clock providers, even if
>>> Documentation/devicetree/bindings/clock/clock-bindings.txt doesn't
>>> really clarify this
>>>
>>> Clock consumer should rather use 'clocks' and point to the provider's
>>> phandle if my understanding is right.
>>
>> This is a clock-frequency, not clock reference. For external clocks, a
> 
> Yes, I was suggesting to replace clock-frequency with clocks, that
> accepts a phandle.
> 
> The thing is, the driver parses 'clock-frequency'
> 	device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
> 
> which I assume comes from ACPI (as the driver was developed for an
> ACPI platform).
> 
> If in DTS we don't use it, I then need to
> 
> #ifdef CONFIG_ACPI
> 
> #elif defined CONFIG_OF
> 
> #endif
> 
> Which I would really like to avoid.
> 
> Anyone with ACPI experience that knows where clock-frequency comes
> from ?

I would assume that ACPI simply does not support common clock framework,
so it had to use clock-frequency. Several of such drivers were added by
folks from Intel which use ACPI, not Devicetree.

> 
>> clock phandles + assigned-clock-rates should be rather used. However for
>> internal clocks, this is a perfectly valid property.
>>
>> Therefore the question is - what is the "xclk"?
> 
> xclk is the clock fed to the sensor, which which all its internal
> clocks are generated, so it's indeed an 'external' clock. As I've
> said, clock-frequency seems to be meant for clock providers, and
> the image sensor is a clock consumer.

Regardless whether clock-frequency stays or not, you need the clocks
property in such case.

> 
>>
>>>
>>>>> +
>>>>> +  pwdn-gpios:
>>>>> +    description: Reference to the GPIO connected to the PWDNB pin. Active low.
>>>>
>>>> maxItems
>>>>
>>>
>>> I thought it was not necessary with a single description: entry. But
>>> looking at the dt-schema source I fail to find any commit mentioning
>>> that.
>>
>> The purpose of maxItems is to constrain the number of GPIOs, so two
>> would be incorrect.
>>
> 
> I recall that with a single description entry then maxItems: 1 was
> assumed by the dt-schema validator, but I cannot find references to
> any commit, so I'll add it.
> 
>>>
>>>>> +
>>>>> +  reset-gpios:
>>>>> +    description:
>>>>> +      Reference to the GPIO connected to the XSHUTDOWN pin. Active low.
>>>>
>>>> maxItems
>>>>
>>>>> +
>>>>> +  avdd-supply:
>>>>> +    description: Analog circuit power. Typically 2.8V.
>>>>> +
>>>>> +  dvdd-supply:
>>>>> +    description: Digital circuit power. Typically 1.2V.
>>>>> +
>>>>> +  dovdd-supply:
>>>>> +    description: Digital I/O circuit power. Typically 2.8V or 1.8V.
>>>>> +
>>>>> +  port:
>>>>> +    $ref: /schemas/graph.yaml#/$defs/port-base
>>>>> +    additionalProperties: false
>>>>> +
>>>>> +    properties:
>>>>> +      endpoint:
>>>>> +        $ref: /schemas/media/video-interfaces.yaml#
>>>>> +        unevaluatedProperties: false
>>>>> +
>>>>> +        properties:
>>>>> +          data-lanes:
>>>>> +            description: The sensor supports 1 or 2 data lanes operations.
>>>>> +            minItems: 1
>>>>> +            maxItems: 2
>>>>> +            items:
>>>>> +              maximum: 2
>>>>
>>>> Is '0' also allowed? If not then maybe 'enum: [ 1, 2 ]'
>>>
>>> No 0 is not allowed, but the data-lanes properties should accept any
>>> of the following combinations
>>>         <1>
>>>         <1 2>
>>>         <2 1>
>>>
>>> As the chip seems to support lane re-ordering.
>>>
>>> using enum would allow to between <1> or <2> if I got it right?
>>
>> Yeah, enum would be equivalent. I find it more readable, than min+max,
>> but it's not a strong preference.
>>
> 
> I don't think enum is equivalent, as it specifies a set of valid values
> a property can assume, but it does not support arrays.

It is equivalent, just has to be used in equivalent way.

> 
> https://json-schema.org/draft/2020-12/json-schema-validation.html#rfc.section.6.16.1.2.
> 
> enum
>   The value of this keyword MUST be an array. This array SHOULD have
>   at least one element. Elements in the array SHOULD be unique.
> 
>   An instance validates successfully against this keyword if its value
>   is equal to one of the elements in this keyword's array value.
> > In facts:

That's not good usage. See for example:
Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml


Best regards,
Krzysztof

Re: [PATCH 1/6] media: dt-bindings: i2c: Document ov5670

[Jacopo Mondi]

Hi Krzysztof

On Fri, Mar 11, 2022 at 05:11:47PM +0100, Krzysztof Kozlowski wrote:
> On 11/03/2022 17:05, Jacopo Mondi wrote:
> > Hi Krzysztof,
> >
> > On Thu, Mar 10, 2022 at 06:26:02PM +0100, Krzysztof Kozlowski wrote:
> >> On 10/03/2022 18:16, Jacopo Mondi wrote:
> >>> Hi Krzysztof
> >>>
> >>> On Thu, Mar 10, 2022 at 03:29:24PM +0100, Krzysztof Kozlowski wrote:
> >>>> On 10/03/2022 14:08, Jacopo Mondi wrote:
> >>>>> Provide the bindings documentation for Omnivision OV5670 image sensor.
> >>>>>
> >>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>>> ---
> >>>>>  .../devicetree/bindings/media/i2c/ov5670.yaml | 93 +++++++++++++++++++
> >>>>
> >>>> Add the file to maintainers entry.
> >>>>
> >>>
> >>> Right
> >>>
> >>>>>  1 file changed, 93 insertions(+)
> >>>>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> >>>>> new file mode 100644
> >>>>> index 000000000000..dc4a3297bf6f
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> >>>>
> >>>> Missing vendor prefix in file name.
> >>>>
> >>>
> >>> Right x2
> >>>
> >>>>> @@ -0,0 +1,93 @@
> >>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>>>> +%YAML 1.2
> >>>>> +---
> >>>>> +$id: http://devicetree.org/schemas/media/i2c/ov5670.yaml#
> >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>> +
> >>>>> +title: Omnivision OV5670 5 Megapixels raw image sensor
> >>>>> +
> >>>>> +maintainers:
> >>>>> +  - Jacopo Mondi <jacopo@jmondi.org>
> >>>>
> >>>> Please add also driver maintainer.
> >>>>
> >>>
> >>> I never got what the policy was, if the maintainer entries here only
> >>> refer to the binding file or to the driver too
> >>
> >> It is a person responsible for the bindings, so indeed it might not feed
> >> existing maintainer.
> >>
> >>>
> >>>>> +
> >>>>> +description: |-
> >>>>> +  The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits
> >>>>> +  RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is
> >>>>> +  controlled through an I2C compatible control bus.
> >>>>> +
> >>>>> +properties:
> >>>>> +  compatible:
> >>>>> +    const: ovti,ov5670
> >>>>> +
> >>>>> +  reg:
> >>>>> +    maxItems: 1
> >>>>> +
> >>>>> +  clock-frequency:
> >>>>> +    description: Frequency of the xclk clock.
> >>>>
> >>>> Is the xclk external clock coming to the sensor? If yes, there should be
> >>>> a "clocks" property.
> >>>>
> >>>
> >>> To be honest I was not sure about this, as clock-frequency is already
> >>> used by the driver for the ACPI part, but it seems to in DT bindings
> >>> it is a property meant to be specified in the clock providers, even if
> >>> Documentation/devicetree/bindings/clock/clock-bindings.txt doesn't
> >>> really clarify this
> >>>
> >>> Clock consumer should rather use 'clocks' and point to the provider's
> >>> phandle if my understanding is right.
> >>
> >> This is a clock-frequency, not clock reference. For external clocks, a
> >
> > Yes, I was suggesting to replace clock-frequency with clocks, that
> > accepts a phandle.
> >
> > The thing is, the driver parses 'clock-frequency'
> > 	device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
> >
> > which I assume comes from ACPI (as the driver was developed for an
> > ACPI platform).
> >
> > If in DTS we don't use it, I then need to
> >
> > #ifdef CONFIG_ACPI
> >
> > #elif defined CONFIG_OF
> >
> > #endif
> >
> > Which I would really like to avoid.
> >
> > Anyone with ACPI experience that knows where clock-frequency comes
> > from ?
>
> I would assume that ACPI simply does not support common clock framework,
> so it had to use clock-frequency. Several of such drivers were added by
> folks from Intel which use ACPI, not Devicetree.
>
> >
> >> clock phandles + assigned-clock-rates should be rather used. However for
> >> internal clocks, this is a perfectly valid property.
> >>
> >> Therefore the question is - what is the "xclk"?
> >
> > xclk is the clock fed to the sensor, which which all its internal
> > clocks are generated, so it's indeed an 'external' clock. As I've
> > said, clock-frequency seems to be meant for clock providers, and
> > the image sensor is a clock consumer.
>
> Regardless whether clock-frequency stays or not, you need the clocks
> property in such case.
>

Yes, I will have to ifdef in the driver if no better alternatives

> >
> >>
> >>>
> >>>>> +
> >>>>> +  pwdn-gpios:
> >>>>> +    description: Reference to the GPIO connected to the PWDNB pin. Active low.
> >>>>
> >>>> maxItems
> >>>>
> >>>
> >>> I thought it was not necessary with a single description: entry. But
> >>> looking at the dt-schema source I fail to find any commit mentioning
> >>> that.
> >>
> >> The purpose of maxItems is to constrain the number of GPIOs, so two
> >> would be incorrect.
> >>
> >
> > I recall that with a single description entry then maxItems: 1 was
> > assumed by the dt-schema validator, but I cannot find references to
> > any commit, so I'll add it.
> >
> >>>
> >>>>> +
> >>>>> +  reset-gpios:
> >>>>> +    description:
> >>>>> +      Reference to the GPIO connected to the XSHUTDOWN pin. Active low.
> >>>>
> >>>> maxItems
> >>>>
> >>>>> +
> >>>>> +  avdd-supply:
> >>>>> +    description: Analog circuit power. Typically 2.8V.
> >>>>> +
> >>>>> +  dvdd-supply:
> >>>>> +    description: Digital circuit power. Typically 1.2V.
> >>>>> +
> >>>>> +  dovdd-supply:
> >>>>> +    description: Digital I/O circuit power. Typically 2.8V or 1.8V.
> >>>>> +
> >>>>> +  port:
> >>>>> +    $ref: /schemas/graph.yaml#/$defs/port-base
> >>>>> +    additionalProperties: false
> >>>>> +
> >>>>> +    properties:
> >>>>> +      endpoint:
> >>>>> +        $ref: /schemas/media/video-interfaces.yaml#
> >>>>> +        unevaluatedProperties: false
> >>>>> +
> >>>>> +        properties:
> >>>>> +          data-lanes:
> >>>>> +            description: The sensor supports 1 or 2 data lanes operations.
> >>>>> +            minItems: 1
> >>>>> +            maxItems: 2
> >>>>> +            items:
> >>>>> +              maximum: 2
> >>>>
> >>>> Is '0' also allowed? If not then maybe 'enum: [ 1, 2 ]'
> >>>
> >>> No 0 is not allowed, but the data-lanes properties should accept any
> >>> of the following combinations
> >>>         <1>
> >>>         <1 2>
> >>>         <2 1>
> >>>
> >>> As the chip seems to support lane re-ordering.
> >>>
> >>> using enum would allow to between <1> or <2> if I got it right?
> >>
> >> Yeah, enum would be equivalent. I find it more readable, than min+max,
> >> but it's not a strong preference.
> >>
> >
> > I don't think enum is equivalent, as it specifies a set of valid values
> > a property can assume, but it does not support arrays.
>
> It is equivalent, just has to be used in equivalent way.
>
> >
> > https://json-schema.org/draft/2020-12/json-schema-validation.html#rfc.section.6.16.1.2.
> >
> > enum
> >   The value of this keyword MUST be an array. This array SHOULD have
> >   at least one element. Elements in the array SHOULD be unique.
> >
> >   An instance validates successfully against this keyword if its value
> >   is equal to one of the elements in this keyword's array value.
> > > In facts:
>
> That's not good usage. See for example:
> Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml

Thanks, you're right.

        items:
          enum: [1, 2]

validates correctly.

Thanks for the suggestion!

>
>
> Best regards,
> Krzysztof

Re: [PATCH 1/6] media: dt-bindings: i2c: Document ov5670

[Krzysztof Kozlowski]

On 11/03/2022 19:00, Jacopo Mondi wrote:
> Hi Krzysztof
> 
> On Fri, Mar 11, 2022 at 05:11:47PM +0100, Krzysztof Kozlowski wrote:
>> On 11/03/2022 17:05, Jacopo Mondi wrote:
>>> Hi Krzysztof,
>>>
>>> On Thu, Mar 10, 2022 at 06:26:02PM +0100, Krzysztof Kozlowski wrote:
>>>> On 10/03/2022 18:16, Jacopo Mondi wrote:
>>>>> Hi Krzysztof
>>>>>
>>>>> On Thu, Mar 10, 2022 at 03:29:24PM +0100, Krzysztof Kozlowski wrote:
>>>>>> On 10/03/2022 14:08, Jacopo Mondi wrote:
>>>>>>> Provide the bindings documentation for Omnivision OV5670 image sensor.
>>>>>>>
>>>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>>>>>> ---
>>>>>>>  .../devicetree/bindings/media/i2c/ov5670.yaml | 93 +++++++++++++++++++
>>>>>>
>>>>>> Add the file to maintainers entry.
>>>>>>
>>>>>
>>>>> Right
>>>>>
>>>>>>>  1 file changed, 93 insertions(+)
>>>>>>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5670.yaml
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..dc4a3297bf6f
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
>>>>>>
>>>>>> Missing vendor prefix in file name.
>>>>>>
>>>>>
>>>>> Right x2
>>>>>
>>>>>>> @@ -0,0 +1,93 @@
>>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>>> +%YAML 1.2
>>>>>>> +---
>>>>>>> +$id: http://devicetree.org/schemas/media/i2c/ov5670.yaml#
>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>> +
>>>>>>> +title: Omnivision OV5670 5 Megapixels raw image sensor
>>>>>>> +
>>>>>>> +maintainers:
>>>>>>> +  - Jacopo Mondi <jacopo@jmondi.org>
>>>>>>
>>>>>> Please add also driver maintainer.
>>>>>>
>>>>>
>>>>> I never got what the policy was, if the maintainer entries here only
>>>>> refer to the binding file or to the driver too
>>>>
>>>> It is a person responsible for the bindings, so indeed it might not feed
>>>> existing maintainer.
>>>>
>>>>>
>>>>>>> +
>>>>>>> +description: |-
>>>>>>> +  The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits
>>>>>>> +  RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is
>>>>>>> +  controlled through an I2C compatible control bus.
>>>>>>> +
>>>>>>> +properties:
>>>>>>> +  compatible:
>>>>>>> +    const: ovti,ov5670
>>>>>>> +
>>>>>>> +  reg:
>>>>>>> +    maxItems: 1
>>>>>>> +
>>>>>>> +  clock-frequency:
>>>>>>> +    description: Frequency of the xclk clock.
>>>>>>
>>>>>> Is the xclk external clock coming to the sensor? If yes, there should be
>>>>>> a "clocks" property.
>>>>>>
>>>>>
>>>>> To be honest I was not sure about this, as clock-frequency is already
>>>>> used by the driver for the ACPI part, but it seems to in DT bindings
>>>>> it is a property meant to be specified in the clock providers, even if
>>>>> Documentation/devicetree/bindings/clock/clock-bindings.txt doesn't
>>>>> really clarify this
>>>>>
>>>>> Clock consumer should rather use 'clocks' and point to the provider's
>>>>> phandle if my understanding is right.
>>>>
>>>> This is a clock-frequency, not clock reference. For external clocks, a
>>>
>>> Yes, I was suggesting to replace clock-frequency with clocks, that
>>> accepts a phandle.
>>>
>>> The thing is, the driver parses 'clock-frequency'
>>> 	device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
>>>
>>> which I assume comes from ACPI (as the driver was developed for an
>>> ACPI platform).
>>>
>>> If in DTS we don't use it, I then need to
>>>
>>> #ifdef CONFIG_ACPI
>>>
>>> #elif defined CONFIG_OF
>>>
>>> #endif
>>>
>>> Which I would really like to avoid.
>>>
>>> Anyone with ACPI experience that knows where clock-frequency comes
>>> from ?
>>
>> I would assume that ACPI simply does not support common clock framework,
>> so it had to use clock-frequency. Several of such drivers were added by
>> folks from Intel which use ACPI, not Devicetree.
>>
>>>
>>>> clock phandles + assigned-clock-rates should be rather used. However for
>>>> internal clocks, this is a perfectly valid property.
>>>>
>>>> Therefore the question is - what is the "xclk"?
>>>
>>> xclk is the clock fed to the sensor, which which all its internal
>>> clocks are generated, so it's indeed an 'external' clock. As I've
>>> said, clock-frequency seems to be meant for clock providers, and
>>> the image sensor is a clock consumer.
>>
>> Regardless whether clock-frequency stays or not, you need the clocks
>> property in such case.
>>
> 
> Yes, I will have to ifdef in the driver if no better alternatives

I do not see the need of ifdefs... BTW, imx258 has exactly that case -
clock-frequency coming from ACPI world but not added to DT bindings.

Best regards,
Krzysztof

Re: [PATCH 1/6] media: dt-bindings: i2c: Document ov5670

[Laurent Pinchart]

Hello,

On Sat, Mar 12, 2022 at 11:30:55AM +0100, Krzysztof Kozlowski wrote:
> On 11/03/2022 19:00, Jacopo Mondi wrote:
> > On Fri, Mar 11, 2022 at 05:11:47PM +0100, Krzysztof Kozlowski wrote:
> >> On 11/03/2022 17:05, Jacopo Mondi wrote:
> >>> On Thu, Mar 10, 2022 at 06:26:02PM +0100, Krzysztof Kozlowski wrote:
> >>>> On 10/03/2022 18:16, Jacopo Mondi wrote:
> >>>>> On Thu, Mar 10, 2022 at 03:29:24PM +0100, Krzysztof Kozlowski wrote:
> >>>>>> On 10/03/2022 14:08, Jacopo Mondi wrote:
> >>>>>>> Provide the bindings documentation for Omnivision OV5670 image sensor.
> >>>>>>>
> >>>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>>>>> ---
> >>>>>>>  .../devicetree/bindings/media/i2c/ov5670.yaml | 93 +++++++++++++++++++
> >>>>>>
> >>>>>> Add the file to maintainers entry.
> >>>>>
> >>>>> Right
> >>>>>
> >>>>>>>  1 file changed, 93 insertions(+)
> >>>>>>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> >>>>>>>
> >>>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> >>>>>>> new file mode 100644
> >>>>>>> index 000000000000..dc4a3297bf6f
> >>>>>>> --- /dev/null
> >>>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> >>>>>>
> >>>>>> Missing vendor prefix in file name.
> >>>>>
> >>>>> Right x2
> >>>>>
> >>>>>>> @@ -0,0 +1,93 @@
> >>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>>>>>> +%YAML 1.2
> >>>>>>> +---
> >>>>>>> +$id: http://devicetree.org/schemas/media/i2c/ov5670.yaml#
> >>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>>>> +
> >>>>>>> +title: Omnivision OV5670 5 Megapixels raw image sensor
> >>>>>>> +
> >>>>>>> +maintainers:
> >>>>>>> +  - Jacopo Mondi <jacopo@jmondi.org>
> >>>>>>
> >>>>>> Please add also driver maintainer.
> >>>>>
> >>>>> I never got what the policy was, if the maintainer entries here only
> >>>>> refer to the binding file or to the driver too
> >>>>
> >>>> It is a person responsible for the bindings, so indeed it might not feed
> >>>> existing maintainer.
> >>>>
> >>>>>>> +
> >>>>>>> +description: |-
> >>>>>>> +  The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits
> >>>>>>> +  RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is
> >>>>>>> +  controlled through an I2C compatible control bus.
> >>>>>>> +
> >>>>>>> +properties:
> >>>>>>> +  compatible:
> >>>>>>> +    const: ovti,ov5670
> >>>>>>> +
> >>>>>>> +  reg:
> >>>>>>> +    maxItems: 1
> >>>>>>> +
> >>>>>>> +  clock-frequency:
> >>>>>>> +    description: Frequency of the xclk clock.
> >>>>>>
> >>>>>> Is the xclk external clock coming to the sensor? If yes, there should be
> >>>>>> a "clocks" property.
> >>>>>
> >>>>> To be honest I was not sure about this, as clock-frequency is already
> >>>>> used by the driver for the ACPI part, but it seems to in DT bindings
> >>>>> it is a property meant to be specified in the clock providers, even if
> >>>>> Documentation/devicetree/bindings/clock/clock-bindings.txt doesn't
> >>>>> really clarify this
> >>>>>
> >>>>> Clock consumer should rather use 'clocks' and point to the provider's
> >>>>> phandle if my understanding is right.
> >>>>
> >>>> This is a clock-frequency, not clock reference. For external clocks, a
> >>>
> >>> Yes, I was suggesting to replace clock-frequency with clocks, that
> >>> accepts a phandle.
> >>>
> >>> The thing is, the driver parses 'clock-frequency'
> >>> 	device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
> >>>
> >>> which I assume comes from ACPI (as the driver was developed for an
> >>> ACPI platform).
> >>>
> >>> If in DTS we don't use it, I then need to
> >>>
> >>> #ifdef CONFIG_ACPI
> >>>
> >>> #elif defined CONFIG_OF
> >>>
> >>> #endif
> >>>
> >>> Which I would really like to avoid.
> >>>
> >>> Anyone with ACPI experience that knows where clock-frequency comes
> >>> from ?
> >>
> >> I would assume that ACPI simply does not support common clock framework,
> >> so it had to use clock-frequency. Several of such drivers were added by
> >> folks from Intel which use ACPI, not Devicetree.
> >>
> >>>> clock phandles + assigned-clock-rates should be rather used. However for
> >>>> internal clocks, this is a perfectly valid property.
> >>>>
> >>>> Therefore the question is - what is the "xclk"?
> >>>
> >>> xclk is the clock fed to the sensor, which which all its internal
> >>> clocks are generated, so it's indeed an 'external' clock. As I've
> >>> said, clock-frequency seems to be meant for clock providers, and
> >>> the image sensor is a clock consumer.
> >>
> >> Regardless whether clock-frequency stays or not, you need the clocks
> >> property in such case.
> > 
> > Yes, I will have to ifdef in the driver if no better alternatives
> 
> I do not see the need of ifdefs... BTW, imx258 has exactly that case -
> clock-frequency coming from ACPI world but not added to DT bindings.

The driver can call clk_get_rate() when a clock is provided, and use the
clock-frequency property otherwise. I also don't think conditional
compilation is needed.

-- 
Regards,

Laurent Pinchart