Message-ID: <ZPnj2OyHEUuTrH53@pyrite.rasen.tech>
Date:   Thu, 7 Sep 2023 23:53:12 +0900
From:   Paul Elder <paul.elder@...asonboard.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc:     linux-media@...r.kernel.org,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] dt-bindings: media: Add THine THP7312 ISP

On Wed, Sep 06, 2023 at 09:18:30AM +0200, Krzysztof Kozlowski wrote:
> On 06/09/2023 01:31, Paul Elder wrote:
> > Add bindings for the THine THP7312 ISP.
> > 
> > Signed-off-by: Paul Elder <paul.elder@...asonboard.com>
> > ---
> > 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@xilinx.com/
> > 
> >  .../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.

ack

> 
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +      - description: CLKI clock input
> 
> This was absolutely never tested.

I'll admit, yes, I forgot to run the checks. But I did test it on
hardware; it's just that this camera module is always powered and the
clock is always connected so it wouldn't have been caught :/

> 
> > +
> > +  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.

As mentioned elsewhere, it is not duplicated; it's for the input to the
ISP. The data-lanes below is for the output of the ISP. And since the
input to the ISP is completely isolated from the rest of the system
(besides power, I suppose), it's kind of overkill to make an entire
endpoint for it. I suppose the description that I wrote for this
property was slightly too concise to convey that.

I quite like the sensors node introduced in the AP1302; I hope that's a
more acceptable solution?


Paul

> 
> > +    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
> 

Powered by blists - more mailing lists