ov8858 device tree addition

ov8858 device tree addition

[Nicholas Roth]

Device tree entry for ov8858 image sensor.

Changes since v1:
* Fixed CC list
* Changed filename and path
* Added documentation for power GPIO
* Fixed linter errors

[PATCH v2] dt-bindings: media: Add Omnivision ov8858 binding

[Nicholas Roth]

Add a device tree binding for the Omnivision OV8858 image sensor.
The OV8858 is an 8 megapixel image sensor which provides images in RAW
format over MIPI CSI-2 data bus and is controlled through an
I2C-compatibile SCCB bus.

Tested on PinePhone Pro with libcamera cam and qcam.

Signed-off-by: Nicholas Roth <nicholas@rothemail.net>
---
 .../devicetree/bindings/media/i2c/ov8858.yaml | 139 ++++++++++++++++++
 1 file changed, 139 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8858.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/ov8858.yaml b/Documentation/devicetree/bindings/media/i2c/ov8858.yaml
new file mode 100644
index 000000000000..f004e57b05ed
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov8858.yaml
@@ -0,0 +1,139 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/ov8858.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Omnivision OV8856 CMOS Sensor Device Tree Bindings
+
+maintainers:
+  - Nicholas Roth <nicholas@rothemail.net>
+
+description: |-
+  The Omnivision OV8858 is an 8 megapixel image sensor which provides
+  images in RAW format over MIPI CSI-2 data bus with up to 4 lanes
+  and is controlled through an I2C-compatibile SCCB bus.
+
+properties:
+  compatible:
+    const: ovti,ov8858
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    description:
+      Input clock for the sensor.
+    items:
+      - const: xvclk
+
+  clock-frequency:
+    description:
+      Frequency of the xvclk clock in Hertz.
+    minimum: 6000000
+    default: 24000000
+    maximum: 27000000
+
+  dovdd-supply:
+    description:
+      Definition of the regulator used as interface power supply.
+
+  avdd-supply:
+    description:
+      Definition of the regulator used as analog power supply.
+
+  dvdd-supply:
+    description:
+      Definition of the regulator used as digital power supply.
+
+  reset-gpios:
+    description:
+      The phandle and specifier for the GPIO that controls sensor reset.
+      This corresponds to the hardware pin XSHUTDN which is physically
+      active low.
+
+  powerdown-gpios:
+    description:
+      The phandle and specifier for the GPIO that controls power down.
+      This corresponds to the hardware pin PWDNB which is physically
+      active low.
+
+  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 driver supports both two- and four-lane operation.
+            items:
+              - const: 1
+              - const: 2
+              - const: 3
+              - const: 4
+
+          link-frequencies:
+            description: Frequencies listed are driver, not h/w limitations.
+            maxItems: 1
+            items:
+              enum: [ 360000000 ]
+
+        required:
+          - link-frequencies
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - clock-frequency
+  - dovdd-supply
+  - avdd-supply
+  - dvdd-supply
+  - reset-gpios
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ucam: camera@36 {
+            compatible = "ovti,ov8858";
+            reg = <0x36>;
+
+            reset-gpios = <&gpio1 1 GPIO_ACTIVE_LOW>;
+            powerdown-gpios = <&gpio2 2 GPIO_ACTIVE_LOW>;
+            pinctrl-names = "default";
+            pinctrl-0 = <&ucam_pdn &ucam_rst>;
+
+            clocks = <&cam_osc>;
+            clock-names = "xvclk";
+            clock-frequency = <24000000>;
+
+            dovdd-supply = <&vcc1v8_dvp>;
+            dvdd-supply = <&vcc1v2_dvp>;
+            avdd-supply = <&vcc2v8_dvp>;
+
+            port {
+                ucam_out: endpoint {
+                    remote-endpoint = <&mipi_in_ucam>;
+                    data-lanes = <1 2 3 4>;
+                    link-frequencies = /bits/ 64 <360000000>;
+                };
+            };
+        };
+    };
-- 
2.34.1

Re: [PATCH v2] dt-bindings: media: Add Omnivision ov8858 binding

[Krzysztof Kozlowski]

On 09/11/2022 03:31, Nicholas Roth wrote:
> Add a device tree binding for the Omnivision OV8858 image sensor.
> The OV8858 is an 8 megapixel image sensor which provides images in RAW
> format over MIPI CSI-2 data bus and is controlled through an
> I2C-compatibile SCCB bus.
> 
> Tested on PinePhone Pro with libcamera cam and qcam.

How can you test bindings with libcamera?
> 
> Signed-off-by: Nicholas Roth <nicholas@rothemail.net>

This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.

Thank you.

1. There is no driver, no DTS. You received the feedback about it.
2. Wrong cc list. You were asked to use get_maintainers.pl and still
decide not to.

> ---
>  .../devicetree/bindings/media/i2c/ov8858.yaml | 139 ++++++++++++++++++
>  1 file changed, 139 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8858.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov8858.yaml b/Documentation/devicetree/bindings/media/i2c/ov8858.yaml
> new file mode 100644
> index 000000000000..f004e57b05ed
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov8858.yaml
> @@ -0,0 +1,139 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/ov8858.yaml#

Filename still does not match compatible. ovti,ov8858.yaml

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Omnivision OV8856 CMOS Sensor Device Tree Bindings
> +
> +maintainers:
> +  - Nicholas Roth <nicholas@rothemail.net>
> +
> +description: |-
> +  The Omnivision OV8858 is an 8 megapixel image sensor which provides
> +  images in RAW format over MIPI CSI-2 data bus with up to 4 lanes
> +  and is controlled through an I2C-compatibile SCCB bus.
> +
> +properties:
> +  compatible:
> +    const: ovti,ov8858
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    description:
> +      Input clock for the sensor.
> +    items:
> +      - const: xvclk
> +
> +  clock-frequency:
> +    description:
> +      Frequency of the xvclk clock in Hertz.

The frequency of clock should go via common clock framework - you have
get_rate and set_rate. Drop entire property.

> +    minimum: 6000000
> +    default: 24000000
> +    maximum: 27000000
> +
> +  dovdd-supply:
> +    description:
> +      Definition of the regulator used as interface power supply.

Drop "Definition of the regulator used as "

> +
> +  avdd-supply:
> +    description:
> +      Definition of the regulator used as analog power supply.
> +
> +  dvdd-supply:
> +    description:
> +      Definition of the regulator used as digital power supply.
> +
> +  reset-gpios:
> +    description:
> +      The phandle and specifier for the GPIO that controls sensor reset.

Drop this sentence.

> +      This corresponds to the hardware pin XSHUTDN which is physically
> +      active low.
> +
> +  powerdown-gpios:
> +    description:
> +      The phandle and specifier for the GPIO that controls power down.

Drop this sentence.

> +      This corresponds to the hardware pin PWDNB which is physically
> +      active low.
> +
> +  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 driver supports both two- and four-lane operation.

Which driver? In OpenBSD? Which version of OpenBSD? Drop the sentence.

> +            items:
> +              - const: 1
> +              - const: 2
> +              - const: 3
> +              - const: 4
> +
> +          link-frequencies:
> +            description: Frequencies listed are driver, not h/w limitations.

If these are driver limitations, then drop it. Link frequencies are
hardware related and you should here describe the minimum and maximum.
Or leave it empty if any is allowed by hardware.


> +            maxItems: 1
> +            items:
> +              enum: [ 360000000 ]
> +
> +        required:
> +          - link-frequencies
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - clock-frequency
> +  - dovdd-supply
> +  - avdd-supply
> +  - dvdd-supply
> +  - reset-gpios
> +  - port
> +
> +additionalProperties: false

Best regards,
Krzysztof

Re: [PATCH v2] dt-bindings: media: Add Omnivision ov8858 binding

[Nicholas Roth]

Hello,

I’m doing my best to follow along here. Your feedback didn’t get lost
and I tried my best to follow it— I just must not have understood it
correctly.

> Where is the driver? If it was sent separately, you must resent entire
> patchset.
...
> 1. There is no driver, no DTS. You received the feedback about it.

Driver: I submitted the .c files to linux-media, and as part of the
review they asked for me to separately submit device tree bindings
(https://patchwork.kernel.org/project/linux-media/patch/20221106171129.166892-2-nicholas@rothemail.net/).
Are you saying that the driver and the bindings should be the same
commit after all? Are you saying that I need to change this into a
series and include both linux-media@ and devicetree@ mailing lists in
a v3 with both patches? Are you saying something else? I’m afraid I
still don’t understand what you mean by this, but I want to, and I’m
trying to.

> 2. Wrong cc list. You were asked to use get_maintainers.pl and still
> decide not to.

I included the people from get_maintainers.pl, but it seems like you
would like for me to include all entries, including the multiple
mailing lists. Do I understand correctly?

> Subject: drop redundant, second "binding".
Is this new subject good, or would you prefer just "dt-bindings:
media: Add Omnivision ov8858"? Just want to make sure I follow up.

> How can you test bindings with libcamera?
I validated the device tree + driver on this setup, though I did not
know about the binding linter (and this one should pass). I am happy
to drop the comment about validation though and just say this is
tested with the binding checker, or say nothing at all if you'd like.

> Filename matching the compatible.
...
> Filename still does not match compatible. ovti,ov8858.yaml
I didn't understand this the first time, but now I think I do-- seems
like you're asking me to change the binding filename to
"ovti,ov8858.yaml". I was trying to be consistent with ov8856.yaml,
but happy to change the file name if that’s the convention. Is there a
doc I can read with this information or is it institutional knowledge?

> The frequency of clock should go via common clock framework - you have
> get_rate and set_rate. Drop entire property.
I am trying to be consistent with the ov8856 driver and bindings but
would be happy to change if there's a different standard I should be
following. I’m not familiar with that framework though. Is there
somewhere I could read about this common clock framework, including
the driver and device-tree changes I need in order to use it as
compared to this or the ov8856 driver?

> Which driver? In OpenBSD? Which version of OpenBSD? Drop the sentence.
Seems like your point here and in the subsequent comments is to avoid
implementation specifics. That makes a ton of sense, and I’ll make
those changes for v3.

All of your other comments make sense explicitly and I’ll make these
changes. Please help me understand what you mean by (1) and (2),
correct me where I’m wrong or misunderstanding you here, clarify where
needed, and I’ll submit the v3.

Thanks,
-Nicholas

On Wed, Nov 9, 2022 at 2:26 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 09/11/2022 03:31, Nicholas Roth wrote:
> > Add a device tree binding for the Omnivision OV8858 image sensor.
> > The OV8858 is an 8 megapixel image sensor which provides images in RAW
> > format over MIPI CSI-2 data bus and is controlled through an
> > I2C-compatibile SCCB bus.
> >
> > Tested on PinePhone Pro with libcamera cam and qcam.
>
> How can you test bindings with libcamera?
> >
> > Signed-off-by: Nicholas Roth <nicholas@rothemail.net>
>
> This is a friendly reminder during the review process.
>
> It seems my previous comments were not fully addressed. Maybe my
> feedback got lost between the quotes, maybe you just forgot to apply it.
> Please go back to the previous discussion and either implement all
> requested changes or keep discussing them.
>
> Thank you.
>
> 1. There is no driver, no DTS. You received the feedback about it.
> 2. Wrong cc list. You were asked to use get_maintainers.pl and still
> decide not to.
>
> > ---
> >  .../devicetree/bindings/media/i2c/ov8858.yaml | 139 ++++++++++++++++++
> >  1 file changed, 139 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8858.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ov8858.yaml b/Documentation/devicetree/bindings/media/i2c/ov8858.yaml
> > new file mode 100644
> > index 000000000000..f004e57b05ed
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/ov8858.yaml
> > @@ -0,0 +1,139 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/i2c/ov8858.yaml#
>
> Filename still does not match compatible. ovti,ov8858.yaml
>
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Omnivision OV8856 CMOS Sensor Device Tree Bindings
> > +
> > +maintainers:
> > +  - Nicholas Roth <nicholas@rothemail.net>
> > +
> > +description: |-
> > +  The Omnivision OV8858 is an 8 megapixel image sensor which provides
> > +  images in RAW format over MIPI CSI-2 data bus with up to 4 lanes
> > +  and is controlled through an I2C-compatibile SCCB bus.
> > +
> > +properties:
> > +  compatible:
> > +    const: ovti,ov8858
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    description:
> > +      Input clock for the sensor.
> > +    items:
> > +      - const: xvclk
> > +
> > +  clock-frequency:
> > +    description:
> > +      Frequency of the xvclk clock in Hertz.
>
> The frequency of clock should go via common clock framework - you have
> get_rate and set_rate. Drop entire property.
>
> > +    minimum: 6000000
> > +    default: 24000000
> > +    maximum: 27000000
> > +
> > +  dovdd-supply:
> > +    description:
> > +      Definition of the regulator used as interface power supply.
>
> Drop "Definition of the regulator used as "
>
> > +
> > +  avdd-supply:
> > +    description:
> > +      Definition of the regulator used as analog power supply.
> > +
> > +  dvdd-supply:
> > +    description:
> > +      Definition of the regulator used as digital power supply.
> > +
> > +  reset-gpios:
> > +    description:
> > +      The phandle and specifier for the GPIO that controls sensor reset.
>
> Drop this sentence.
>
> > +      This corresponds to the hardware pin XSHUTDN which is physically
> > +      active low.
> > +
> > +  powerdown-gpios:
> > +    description:
> > +      The phandle and specifier for the GPIO that controls power down.
>
> Drop this sentence.
>
> > +      This corresponds to the hardware pin PWDNB which is physically
> > +      active low.
> > +
> > +  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 driver supports both two- and four-lane operation.
>
> Which driver? In OpenBSD? Which version of OpenBSD? Drop the sentence.
>
> > +            items:
> > +              - const: 1
> > +              - const: 2
> > +              - const: 3
> > +              - const: 4
> > +
> > +          link-frequencies:
> > +            description: Frequencies listed are driver, not h/w limitations.
>
> If these are driver limitations, then drop it. Link frequencies are
> hardware related and you should here describe the minimum and maximum.
> Or leave it empty if any is allowed by hardware.
>
>
> > +            maxItems: 1
> > +            items:
> > +              enum: [ 360000000 ]
> > +
> > +        required:
> > +          - link-frequencies
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - clock-frequency
> > +  - dovdd-supply
> > +  - avdd-supply
> > +  - dvdd-supply
> > +  - reset-gpios
> > +  - port
> > +
> > +additionalProperties: false
>
> Best regards,
> Krzysztof
>

Re: [PATCH v2] dt-bindings: media: Add Omnivision ov8858 binding

[Nicholas Roth]

I just got your earlier email reply regarding the CC list, and I think
that clarifies (2) for me. Indeed, it seems like you were asking me to
also include the mailing lists. I didn't want to bother that many
people, but it seems like this is the procedure.

On Wed, Nov 9, 2022 at 9:48 AM Nicholas Roth <nicholas@rothemail.net> wrote:
>
> Hello,
>
> I’m doing my best to follow along here. Your feedback didn’t get lost
> and I tried my best to follow it— I just must not have understood it
> correctly.
>
> > Where is the driver? If it was sent separately, you must resent entire
> > patchset.
> ...
> > 1. There is no driver, no DTS. You received the feedback about it.
>
> Driver: I submitted the .c files to linux-media, and as part of the
> review they asked for me to separately submit device tree bindings
> (https://patchwork.kernel.org/project/linux-media/patch/20221106171129.166892-2-nicholas@rothemail.net/).
> Are you saying that the driver and the bindings should be the same
> commit after all? Are you saying that I need to change this into a
> series and include both linux-media@ and devicetree@ mailing lists in
> a v3 with both patches? Are you saying something else? I’m afraid I
> still don’t understand what you mean by this, but I want to, and I’m
> trying to.
>
> > 2. Wrong cc list. You were asked to use get_maintainers.pl and still
> > decide not to.
>
> I included the people from get_maintainers.pl, but it seems like you
> would like for me to include all entries, including the multiple
> mailing lists. Do I understand correctly?
>
> > Subject: drop redundant, second "binding".
> Is this new subject good, or would you prefer just "dt-bindings:
> media: Add Omnivision ov8858"? Just want to make sure I follow up.
>
> > How can you test bindings with libcamera?
> I validated the device tree + driver on this setup, though I did not
> know about the binding linter (and this one should pass). I am happy
> to drop the comment about validation though and just say this is
> tested with the binding checker, or say nothing at all if you'd like.
>
> > Filename matching the compatible.
> ...
> > Filename still does not match compatible. ovti,ov8858.yaml
> I didn't understand this the first time, but now I think I do-- seems
> like you're asking me to change the binding filename to
> "ovti,ov8858.yaml". I was trying to be consistent with ov8856.yaml,
> but happy to change the file name if that’s the convention. Is there a
> doc I can read with this information or is it institutional knowledge?
>
> > The frequency of clock should go via common clock framework - you have
> > get_rate and set_rate. Drop entire property.
> I am trying to be consistent with the ov8856 driver and bindings but
> would be happy to change if there's a different standard I should be
> following. I’m not familiar with that framework though. Is there
> somewhere I could read about this common clock framework, including
> the driver and device-tree changes I need in order to use it as
> compared to this or the ov8856 driver?
>
> > Which driver? In OpenBSD? Which version of OpenBSD? Drop the sentence.
> Seems like your point here and in the subsequent comments is to avoid
> implementation specifics. That makes a ton of sense, and I’ll make
> those changes for v3.
>
> All of your other comments make sense explicitly and I’ll make these
> changes. Please help me understand what you mean by (1) and (2),
> correct me where I’m wrong or misunderstanding you here, clarify where
> needed, and I’ll submit the v3.
>
> Thanks,
> -Nicholas
>
> On Wed, Nov 9, 2022 at 2:26 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
> >
> > On 09/11/2022 03:31, Nicholas Roth wrote:
> > > Add a device tree binding for the Omnivision OV8858 image sensor.
> > > The OV8858 is an 8 megapixel image sensor which provides images in RAW
> > > format over MIPI CSI-2 data bus and is controlled through an
> > > I2C-compatibile SCCB bus.
> > >
> > > Tested on PinePhone Pro with libcamera cam and qcam.
> >
> > How can you test bindings with libcamera?
> > >
> > > Signed-off-by: Nicholas Roth <nicholas@rothemail.net>
> >
> > This is a friendly reminder during the review process.
> >
> > It seems my previous comments were not fully addressed. Maybe my
> > feedback got lost between the quotes, maybe you just forgot to apply it.
> > Please go back to the previous discussion and either implement all
> > requested changes or keep discussing them.
> >
> > Thank you.
> >
> > 1. There is no driver, no DTS. You received the feedback about it.
> > 2. Wrong cc list. You were asked to use get_maintainers.pl and still
> > decide not to.
> >
> > > ---
> > >  .../devicetree/bindings/media/i2c/ov8858.yaml | 139 ++++++++++++++++++
> > >  1 file changed, 139 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8858.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov8858.yaml b/Documentation/devicetree/bindings/media/i2c/ov8858.yaml
> > > new file mode 100644
> > > index 000000000000..f004e57b05ed
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/i2c/ov8858.yaml
> > > @@ -0,0 +1,139 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/media/i2c/ov8858.yaml#
> >
> > Filename still does not match compatible. ovti,ov8858.yaml
> >
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Omnivision OV8856 CMOS Sensor Device Tree Bindings
> > > +
> > > +maintainers:
> > > +  - Nicholas Roth <nicholas@rothemail.net>
> > > +
> > > +description: |-
> > > +  The Omnivision OV8858 is an 8 megapixel image sensor which provides
> > > +  images in RAW format over MIPI CSI-2 data bus with up to 4 lanes
> > > +  and is controlled through an I2C-compatibile SCCB bus.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: ovti,ov8858
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  clocks:
> > > +    maxItems: 1
> > > +
> > > +  clock-names:
> > > +    description:
> > > +      Input clock for the sensor.
> > > +    items:
> > > +      - const: xvclk
> > > +
> > > +  clock-frequency:
> > > +    description:
> > > +      Frequency of the xvclk clock in Hertz.
> >
> > The frequency of clock should go via common clock framework - you have
> > get_rate and set_rate. Drop entire property.
> >
> > > +    minimum: 6000000
> > > +    default: 24000000
> > > +    maximum: 27000000
> > > +
> > > +  dovdd-supply:
> > > +    description:
> > > +      Definition of the regulator used as interface power supply.
> >
> > Drop "Definition of the regulator used as "
> >
> > > +
> > > +  avdd-supply:
> > > +    description:
> > > +      Definition of the regulator used as analog power supply.
> > > +
> > > +  dvdd-supply:
> > > +    description:
> > > +      Definition of the regulator used as digital power supply.
> > > +
> > > +  reset-gpios:
> > > +    description:
> > > +      The phandle and specifier for the GPIO that controls sensor reset.
> >
> > Drop this sentence.
> >
> > > +      This corresponds to the hardware pin XSHUTDN which is physically
> > > +      active low.
> > > +
> > > +  powerdown-gpios:
> > > +    description:
> > > +      The phandle and specifier for the GPIO that controls power down.
> >
> > Drop this sentence.
> >
> > > +      This corresponds to the hardware pin PWDNB which is physically
> > > +      active low.
> > > +
> > > +  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 driver supports both two- and four-lane operation.
> >
> > Which driver? In OpenBSD? Which version of OpenBSD? Drop the sentence.
> >
> > > +            items:
> > > +              - const: 1
> > > +              - const: 2
> > > +              - const: 3
> > > +              - const: 4
> > > +
> > > +          link-frequencies:
> > > +            description: Frequencies listed are driver, not h/w limitations.
> >
> > If these are driver limitations, then drop it. Link frequencies are
> > hardware related and you should here describe the minimum and maximum.
> > Or leave it empty if any is allowed by hardware.
> >
> >
> > > +            maxItems: 1
> > > +            items:
> > > +              enum: [ 360000000 ]
> > > +
> > > +        required:
> > > +          - link-frequencies
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - clocks
> > > +  - clock-names
> > > +  - clock-frequency
> > > +  - dovdd-supply
> > > +  - avdd-supply
> > > +  - dvdd-supply
> > > +  - reset-gpios
> > > +  - port
> > > +
> > > +additionalProperties: false
> >
> > Best regards,
> > Krzysztof
> >

Re: [PATCH v2] dt-bindings: media: Add Omnivision ov8858 binding

[Krzysztof Kozlowski]

On 09/11/2022 16:12, Nicholas Roth wrote:
> Hey,
> 
> I’m doing my best to follow along here. Your feedback didn’t get lost and I tried my best to follow it— I just must not have understood it correctly.
> 
>> 1. There is no driver, no DTS. You received the feedback about it. 
> 
> Driver: I submitted the .c files to linux-media, and as part of the review they asked for me to separately submit device tree bindings (https://patchwork.kernel.org/project/linux-media/patch/20221106171129.166892-2-nicholas@rothemail.net/). Are you saying that the driver and the bindings should be the same commit after all? Are you saying something else? I’m afraid I still don’t understand what you mean by this, but I want to, and I’m trying to.

They come as one patchset. Separate patches, one patchset. Otherwise you
get checkpatch errors, right?

> 
>> 2. Wrong cc list. You were asked to use get_maintainers.pl and still
>> decide not to.
> I included the people from get_maintainers.pl, but it seems like you would like for me to include all entries, including the multiple mailing lists. Do I understand correctly?

Yes. Do not strip some lists based on your preference. Why only some
people should receive this, not everyone involved in the subsystem?


>>
> 
>> How can you test bindings with libcamera?
> I validate the device tree + driver on this setup, but I am happy to drop the comment about validation.

It's not about bindings then.

> 
>>
>> Filename still does not match compatible. ovti,ov8858.yaml
> I was trying to be consistent with ov8856.yaml, but happy to change the file name if that’s the convention. Is there a doc I can read with this information or is it institutional knowledge?

All recent submissions follow this, so the best is to take last commits.

>>
>> The frequency of clock should go via common clock framework - you have
>> get_rate and set_rate. Drop entire property.
> I am trying to be consistent with the ov8856 driver and bindings but would be happy to change. I’m not familiar with that framework though. Is there somewhere I could read about this, including the driver and device-tree changes I need to use this?

This is very difficult to respond. Please use inline comments, I have no
clue which part you are now commenting. This is not mailing list style
response.

>>
>> Which driver? In OpenBSD? Which version of OpenBSD? Drop the sentence.
> Seems like your point here and in the subsequent comments is to avoid implementation specifics. That makes a ton of sense, and I’ll make those changes for v3.
> 
> All of your other comments make sense explicitly and I’ll make these changes. Please help me understand what you mean by (1) and (2), correct me where I’m wrong or misunderstanding you here, and I’ll submit the v3.

Best regards,
Krzysztof

Re: [PATCH v2] dt-bindings: media: Add Omnivision ov8858 binding

[Nicholas Roth]

Happy to reply inline next time. I'm still getting used to this format
:-). Here's the context around my clock frequency question-- I'd
really like to understand this better:

> > +  clock-names:
> > +    description:
> > +      Input clock for the sensor.
> > +    items:
> > +      - const: xvclk
> > +
> > +  clock-frequency:
> > +    description:
> > +      Frequency of the xvclk clock in Hertz.
>
> The frequency of clock should go via common clock framework - you have
> get_rate and set_rate. Drop entire property.

I am trying to be consistent with the ov8856 driver and bindings but
would be happy to change. I’m not familiar with that framework though.
Is there somewhere I could read about this, including the driver and
device-tree changes I need to use this?

>
> > +    minimum: 6000000
> > +    default: 24000000
> > +    maximum: 27000000
> > +
> > +  dovdd-supply:
> > +    description:
> > +      Definition of the regulator used as interface power supply.

On Wed, Nov 9, 2022 at 10:19 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 09/11/2022 16:12, Nicholas Roth wrote:
> > Hey,
> >
> > I’m doing my best to follow along here. Your feedback didn’t get lost and I tried my best to follow it— I just must not have understood it correctly.
> >
> >> 1. There is no driver, no DTS. You received the feedback about it.
> >
> > Driver: I submitted the .c files to linux-media, and as part of the review they asked for me to separately submit device tree bindings (https://patchwork.kernel.org/project/linux-media/patch/20221106171129.166892-2-nicholas@rothemail.net/). Are you saying that the driver and the bindings should be the same commit after all? Are you saying something else? I’m afraid I still don’t understand what you mean by this, but I want to, and I’m trying to.
>
> They come as one patchset. Separate patches, one patchset. Otherwise you
> get checkpatch errors, right?
>
> >
> >> 2. Wrong cc list. You were asked to use get_maintainers.pl and still
> >> decide not to.
> > I included the people from get_maintainers.pl, but it seems like you would like for me to include all entries, including the multiple mailing lists. Do I understand correctly?
>
> Yes. Do not strip some lists based on your preference. Why only some
> people should receive this, not everyone involved in the subsystem?
>
>
> >>
> >
> >> How can you test bindings with libcamera?
> > I validate the device tree + driver on this setup, but I am happy to drop the comment about validation.
>
> It's not about bindings then.
>
> >
> >>
> >> Filename still does not match compatible. ovti,ov8858.yaml
> > I was trying to be consistent with ov8856.yaml, but happy to change the file name if that’s the convention. Is there a doc I can read with this information or is it institutional knowledge?
>
> All recent submissions follow this, so the best is to take last commits.
>
> >>
> >> The frequency of clock should go via common clock framework - you have
> >> get_rate and set_rate. Drop entire property.
> > I am trying to be consistent with the ov8856 driver and bindings but would be happy to change. I’m not familiar with that framework though. Is there somewhere I could read about this, including the driver and device-tree changes I need to use this?
>
> This is very difficult to respond. Please use inline comments, I have no
> clue which part you are now commenting. This is not mailing list style
> response.
>
> >>
> >> Which driver? In OpenBSD? Which version of OpenBSD? Drop the sentence.
> > Seems like your point here and in the subsequent comments is to avoid implementation specifics. That makes a ton of sense, and I’ll make those changes for v3.
> >
> > All of your other comments make sense explicitly and I’ll make these changes. Please help me understand what you mean by (1) and (2), correct me where I’m wrong or misunderstanding you here, and I’ll submit the v3.
>
> Best regards,
> Krzysztof
>

Re: [PATCH v2] dt-bindings: media: Add Omnivision ov8858 binding

[Krzysztof Kozlowski]

On 09/11/2022 17:26, Nicholas Roth wrote:
> Happy to reply inline next time. I'm still getting used to this format
> :-). Here's the context around my clock frequency question-- I'd
> really like to understand this better:
> 
>>> +  clock-names:
>>> +    description:
>>> +      Input clock for the sensor.
>>> +    items:
>>> +      - const: xvclk
>>> +
>>> +  clock-frequency:
>>> +    description:
>>> +      Frequency of the xvclk clock in Hertz.
>>
>> The frequency of clock should go via common clock framework - you have
>> get_rate and set_rate. Drop entire property.
> 
> I am trying to be consistent with the ov8856 driver and bindings but
> would be happy to change. I’m not familiar with that framework though.
> Is there somewhere I could read about this, including the driver and
> device-tree changes I need to use this?

git grep clk_get_rate -- drivers/media/i2c/

For example imx219 or imx334.

Best regards,
Krzysztof