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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 08/11/2022 04:38, Nicholas Roth wrote:
> Add a device tree binding for the Omnivision OV8858 image sensor.

Subject: drop redundant, second "binding".

Also - wrong CC list.

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.


> 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>
> ---
>  .../bindings/i2c/ovti,ov8858-i2c.yaml         | 78 +++++++++++++++++++

Where is the driver? If it was sent separately, you must resent entire
patchset.

>  1 file changed, 78 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/ovti,ov8858-i2c.yaml
> 
> diff --git a/Documentation/devicetree/bindings/i2c/ovti,ov8858-i2c.yaml b/Documentation/devicetree/bindings/i2c/ovti,ov8858-i2c.yaml
> new file mode 100644
> index 000000000000..1300c1b6fa17
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/ovti,ov8858-i2c.yaml

Filename matching the compatible.

> @@ -0,0 +1,78 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i2c/i2c-imx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Freescale Inter IC (I2C) and High Speed Inter IC (HS-I2C) for OV8858
> +
> +allOf:
> +  - $ref: /schemas/i2c/i2c-controller.yaml#
> +
> +properties:
> +  compatible: ovti,ov8858
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    const: xvclk
> +
> +  clock-frequency:
> +    minimum: 24000000
> +    default: 24000000
> +    maximum: 24000000

1. This should be then const instead of these tree... but:
2. Why it can work with only one frequency? Most of camera sensors take few.

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    i2c@ff110000 {
> +        status = "okay";

Drop

> +
> +        clock-frequency = <400000>;
> +        i2c-scl-rising-time-ns = <300>;
> +        i2c-scl-falling-time-ns = <15>;
> +        
> +        pinctrl-0 = <&i2c1_xfer &cif_clkouta>;
> +        
> +        assigned-clocks = <&cru SCLK_CIF_OUT &cru SCLK_CIF_OUT_SRC>;
> +        assigned-clock-parents = <&cru SCLK_CIF_OUT_SRC &cru PLL_GPLL>;
> +        assigned-clock-rates = <19200000 0>;

Drop all above properties, not related.

> +        ucam: camera@36 {
> +            compatible = "ovti,ov8858";
> +            reg = <0x36>;
> +            pinctrl-names = "default";
> +            pinctrl-0 = <&ucam_pdn &ucam_rst>;
> +
> +            clocks = <&cru SCLK_CIF_OUT>;
> +            clock-names = "xvclk";
> +
> +            dovdd-supply = <&vcc1v8_dvp>;
> +            /*XXX: also depends on vcca1v8_codec for I2C bus power */

So your bindings are incomplete?
> +
> +            reset-gpios = <&gpio1 RK_PA4 GPIO_ACTIVE_LOW>;
> +            powerdown-gpios = <&gpio2 RK_PB4 GPIO_ACTIVE_LOW>;

Does not look like you tested the bindings. Please run `make
dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).

Best regards,
Krzysztof