Re: [PATCH 1/3] dt-bindings: media: Add THine THP7312 ISP

[Krzysztof Kozlowski]

On 06/09/2023 01:31, Paul Elder wrote:
> Add bindings for the THine THP7312 ISP.
> 
> Signed-off-by: Paul Elder <paul.elder@xxxxxxxxxxxxxxxx>
> ---
> Since the THP7312 supports multiple sensors, thine,rx-data-lanes alone
> might not be enough. I was consdering using sensor nodes like what the
> AP1302 does [1]. This way we can also move the power supplies that only
> concern the sensor in there as well. I was wondering what to do about
> the model name, though, as the thp7312 completely isolates that from the 
> rest of the system.
> 
> I'm planning to add sensor nodes in somehow in a v2.
> 
> [1] https://lore.kernel.org/linux-media/20211006113254.3470-2-anil.mamidala@xxxxxxxxxx/
> 
>  .../bindings/media/thine,thp7312.yaml         | 170 ++++++++++++++++++
>  1 file changed, 170 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/thine,thp7312.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/thine,thp7312.yaml b/Documentation/devicetree/bindings/media/thine,thp7312.yaml
> new file mode 100644
> index 000000000000..e8d203dcda81
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/thine,thp7312.yaml
> @@ -0,0 +1,170 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (c) 2023 Ideas on Board
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/thine,thp7312.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: THine THP7312
> +
> +maintainers:
> +  - Paul Elder <paul.elder@@ideasonboard.com>
> +
> +description:
> +  The THP7312 is a standalone ISP controlled over i2c, and is capable of
> +  various image processing and correction functions, including 3A control. It
> +  can be connected to CMOS image sensors from various vendors, supporting both
> +  MIPI CSI-2 and parallel interfaces. It can also output on either MIPI CSI-2
> +  or parallel. The hardware is capable of transmitting and receiving MIPI
> +  interlaved data strams with data types or multiple virtual channel
> +  identifiers.
> +
> +allOf:
> +  - $ref: ../video-interface-devices.yaml#
> +
> +properties:
> +  compatible:
> +    const: thine,thp7312
> +
> +  reg:
> +    description: I2C device address

You can skip description. It is obvious.

> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +      - description: CLKI clock input

This was absolutely never tested.

> +
> +  reset-gpios:
> +    maxItems: 1
> +    description: |-
> +      Reference to the GPIO connected to the RESET_N pin, if any.
> +      Must be released (set high) after all supplies are applied.
> +
> +  vddcore-supply:
> +    description:
> +      1.2V supply for core, PLL, MIPI rx and MIPI tx.
> +
> +  vhtermnx-supply:
> +    description:
> +      Supply for input (rx). 1.8V for MIPI, or 1.8/2.8/3.3V for parallel.
> +
> +  vddtx-supply:
> +    description:
> +      Supply for output (tx). 1.8V for MIPI, or 1.8/2.8/3.3V for parallel.
> +
> +  vddhost-supply:
> +    description:
> +      Supply for host interface. 1.8V, 2.8V, or 3.3V.
> +
> +  vddcmos-supply:
> +    description:
> +      Supply for sensor interface. 1.8V, 2.8V, or 3.3V.
> +
> +  vddgpio_0-supply:

No, underscores are not allowed in names.

> +    description:
> +      Supply for GPIO_0. 1.8V, 2.8V, or 3.3V.
> +
> +  vddgpio_1-supply:
> +    description:
> +      Supply for GPIO_1. 1.8V, 2.8V, or 3.3V.
> +
> +  DOVDD-supply:

lowercase. Look at your other supplies. VDD is spelled there "vdd", so
do not introduce random style.


> +    description:
> +      Digital I/O (1.8V) supply for image sensor.
> +
> +  AVDD-supply:

lowercase

> +    description:
> +      Analog (2.8V) supply for image sensor.
> +
> +  DVDD-supply:

lowercase

> +    description:
> +      Digital Core (1.2V) supply for image sensor.
> +
> +  orientation: true
> +  rotation: true
> +
> +  thine,rx,data-lanes:

Why are you duplicating properties? With wrong name? No, that's not a
property of a device node, but endpoint.

> +    minItems: 4
> +    maxItems: 4
> +    $ref: /schemas/media/video-interfaces.yaml#data-lanes
> +    description: |-

Drop |- where not needed.

> +      This property is for lane reordering between the THP7312 and the imaging
> +      sensor that it is connected to.
> +
> +  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 either two-lane, or four-lane operation.
> +              This property is for lane reordering between the THP7312 and
> +              the SoC. If this property is omitted four-lane operation is
> +              assumed. For two-lane operation the property must be set to <1 2>.
> +            minItems: 2
> +            maxItems: 4
> +            items:
> +              maximum: 4
> +
> +required:
> +  - compatible
> +  - reg
> +  - reset-gpios
> +  - clocks
> +  - vddcore-supply
> +  - vhtermrx-supply
> +  - vddtx-supply
> +  - vddhost-supply
> +  - vddcmos-supply
> +  - vddgpio_0-supply
> +  - vddgpio_1-supply
> +  - DOVDD-supply
> +  - AVDD-supply
> +  - DVDD-supply
> +  - thine,rx,data-lanes
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        camera@61 {
> +            compatible = "thine,thp7312";
> +            reg = <0x61>;
> +
> +            pinctrl-names = "default";
> +            pinctrl-0 = <&cam1_pins_default>;
> +
> +            reset-gpios = <&pio 119 GPIO_ACTIVE_LOW>;
> +            clocks = <&camera61_clk>;
> +
> +            vddcore-supply = <&vsys_v4p2>;
> +            AVDD-supply = <&vsys_v4p2>;
> +            DVDD-supply = <&vsys_v4p2>;

Srlsy, test it before sending. Look how many supplies you require and
what is provided here. How any of this could possibly work?



Best regards,
Krzysztof