Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 31 May 2023 12:20:47 +0200
From:   Tommaso Merciai <tomm.merciai@gmail.com>
To:     Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc:     Sakari Ailus <sakari.ailus@linux.intel.com>,
        jacopo.mondi@ideasonboard.com, martin.hecht@avnet.eu,
        linuxfancy@googlegroups.com,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Hans Verkuil <hverkuil@xs4all.nl>,
        Marco Felsch <m.felsch@pengutronix.de>,
        Gerald Loacker <gerald.loacker@wolfvision.net>,
        Krzysztof =?utf-8?Q?Ha=C5=82asa?= <khalasa@piap.pl>,
        Shawn Tu <shawnx.tu@intel.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        Benjamin Mugnier <benjamin.mugnier@foss.st.com>,
        Mikhail Rudenko <mike.rudenko@gmail.com>,
        Nicholas Roth <nicholas@rothemail.net>,
        linux-media@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/2] media: dt-bindings: alvium: add document YAML
 binding
Message-ID: <ZHcff4Gpr9QSgpOs@tom-HP-ZBook-Fury-15-G7-Mobile-Workstation>
References: <20230526173955.797226-1-tomm.merciai@gmail.com>
 <20230526173955.797226-2-tomm.merciai@gmail.com>
 <ZHPElYOeD2C1qo4R@kekkonen.localdomain>
 <20230529063907.GB25984@pendragon.ideasonboard.com>
 <20230529064326.GC25984@pendragon.ideasonboard.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230529064326.GC25984@pendragon.ideasonboard.com>
Precedence: bulk

Hi Laurent,

On Mon, May 29, 2023 at 09:43:26AM +0300, Laurent Pinchart wrote:
> On Mon, May 29, 2023 at 09:39:13AM +0300, Laurent Pinchart wrote:
> > On Sun, May 28, 2023 at 09:16:05PM +0000, Sakari Ailus wrote:
> > > On Fri, May 26, 2023 at 07:39:43PM +0200, Tommaso Merciai wrote:
> > > > Add documentation of device tree in YAML schema for the ALVIUM
> > > > Camera from Allied Vision Inc.
> > > > 
> > > > References:
> > > >  - https://www.alliedvision.com/en/products/embedded-vision-solutions
> > > > 
> > > > Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>
> > > > ---
> > > > Changes since v1:
> > > >  - Fixed build error as suggested by RHerring bot
> > > > 
> > > >  .../media/i2c/alliedvision,alvium.yaml        | 115 ++++++++++++++++++
> > > >  1 file changed, 115 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/alliedvision,alvium.yaml
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/media/i2c/alliedvision,alvium.yaml b/Documentation/devicetree/bindings/media/i2c/alliedvision,alvium.yaml
> > > > new file mode 100644
> > > > index 000000000000..81e9e560c99d
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/media/i2c/alliedvision,alvium.yaml
> > > > @@ -0,0 +1,115 @@
> > > > +# SPDX-License-Identifier: GPL-2.0
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/media/i2c/alliedvision,alvium.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Alliedvision Alvium Camera
> > 
> > s/Alliedvision/Allied Vision/
> > 
> > > > +
> > > > +maintainers:
> > > > +  - Tommaso Merciai <tomm.merciai@gmail.com>
> > > > +  - Martin Hecht <martin.hecht@avnet.eu>
> > > > +
> > > > +allOf:
> > > > +  - $ref: /schemas/media/video-interface-devices.yaml#
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: alliedvision,alvium
> > 
> > The name is very generic. There are Alvium camera modules that have a
> > GMSL or FPD-Link interface, and I'm pretty sure those will require a
> > different driver. I would add module-specific compatible strings (e.g.
> > "alliedvision,alvium-1500c", ...) here, with a generic fallback.
> > "alliedvision,alvium" isn't good as it won't cover GMSL or FPD-Link,
> > maybe "alliedvision,alvium-csi2" would be an option.
> 
> Actually, "alvium-1500c" as a specific compatible string won't do. You
> need the exact model in the compatible string, otherwise it won't be
> possible for the driver to handle device-specific configuration (for
> instance accessing registers of the camera sensor for fine-grained
> configuration). I would thus recommend using "alliedvision,alvium-1500c"
> and "alliedvision,alvium-1800c" as generic fallbacks, along compatible
> strings that include the exact device model.

Agree with alliedvision,alvium-csi2 and thanks for your suggestion.
In my opinion we don’t need names for 1500c and 
others because the same driver can drive all the alvium models.
Alvium is taking care of different sensor abstractions.

I test with this driver with the following models:

 - 1800 C-1240c
 - 1800 C-040c
 - 1500 C-500

What do you think about?

Thanks,
Tommaso

> 
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  clocks:
> > > > +    description: XCLK Input Clock
> > > > +
> > > > +  clock-names:
> > > > +    const: xclk
> > > 
> > > I'd also drop this as you have a single clock only: it's redundant.
> > > 
> > > > +
> > > > +  powerdown-gpios:
> > > > +    maxItems: 1
> > > > +    description: >
> > > > +      Reference to the GPIO connected to the powerdown pin, if any.
> > > > +
> > > > +  reset-gpios:
> > > > +    maxItems: 1
> > > > +    description: >
> > > > +      Reference to the GPIO connected to the reset pin, if any.
> > 
> > Reading the Alvium CSI-2 Cameras User Guide, I don't see any powerdown
> > or reset pin on the 22-pin connector. Am I missing something ? There are
> > however two GPIOs (in addition to the I2C signals that are also
> > documented as GPIOs), do you plan to support those ?
> > 
> > > > +
> > > > +  streamon-delay:
> > > > +    maxItems: 1
> > > > +    description: >
> > > > +      Delay before camera start capturing frames in us.
> > 
> > Add "-us" to the property name to indicate the unit.
> > 
> > This is a vendor-specific property, and should thus have a vendor
> > prefix.
> > 
> > A longer description is needed, from that single line I have no idea
> > what the property does exactly.
> > 
> > > > +
> > > > +  rotation:
> > > > +    enum:
> > > > +      - 0
> > > > +      - 180
> > 
> > Why is the rotation restricted to 0 or 180 ? Someone could mount the
> > module with  90 degrees rotation, shouldn't the DT bindings allow
> > describing that ?
> > 
> > You need a property for the vcc-ext-in supply.
> > 
> > > > +
> > > > +  port:
> > > > +    description: Digital Output Port
> > > > +    $ref: /schemas/graph.yaml#/$defs/port-base
> > > > +    additionalProperties: false
> > > > +
> > > > +    properties:
> > > > +      endpoint:
> > > > +        $ref: /schemas/media/video-interfaces.yaml#
> > > > +        unevaluatedProperties: false
> > > > +
> > > > +        properties:
> > > > +          clock-lanes:
> > > > +            const: 0
> > > 
> > > The driver can know this, no need to have it in DT, i.e. please drop it.
> > > 
> > > > +          data-lanes:
> > > > +            minItems: 1
> > > > +            maxItems: 4
> > > > +          link-frequencies: true
> > > > +
> > > > +        required:
> > > > +          - data-lanes
> > > > +          - link-frequencies
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +  - clocks
> > > > +  - clock-names
> > > > +  - port
> > > > +
> > > > +additionalProperties: false
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +      #include <dt-bindings/gpio/gpio.h>
> > > > +      #include <dt-bindings/clock/imx8mp-clock.h>
> > > > +
> > > > +      i2c {
> > > > +          #address-cells = <1>;
> > > > +          #size-cells = <0>;
> > > > +
> > > > +          camera: alvium@3c {
> > > > +              compatible = "alliedvision,alvium";
> > 
> > The "alliedvision" prefix is missing from
> > Documentation/devicetree/bindings/vendor-prefixes.yaml.
> > 
> > > > +              pinctrl-names = "default";
> > > > +              pinctrl-0 = <&pinctrl_csi0_pwn>, <&pinctrl_csi0_rst>, <&pinctrl_csi_mclk>;
> > 
> > I'd drop pinctrl, it makes the example longer without adding much value.
> > 
> > > > +              reg = <0x3c>;
> > > > +              clocks = <&clk IMX8MP_CLK_IPP_DO_CLKO2>;
> > > > +              clock-names = "xclk";
> > > > +              assigned-clocks = <&clk IMX8MP_CLK_IPP_DO_CLKO2>;
> > > > +              assigned-clock-parents = <&clk IMX8MP_CLK_24M>;
> > > > +              assigned-clock-rates = <24000000>;
> > > > +              streamon-delay = <20>;
> > > > +              powerdown-gpios = <&gpio2 11 GPIO_ACTIVE_HIGH>;
> > > > +              reset-gpios = <&gpio1 6 GPIO_ACTIVE_LOW>;
> > > > +              status = "okay";
> > > > +
> > > > +              port {
> > > > +                  alvium_out: endpoint {
> > > > +                      remote-endpoint = <&mipi_csi_0_in>;
> > > > +                      data-lanes = <1 2 3 4>;
> > > > +                      link-frequencies = /bits/ 64 <681250000>;
> > > > +                      clock-lanes = <0>;
> > > > +                  };
> > > > +              };
> > > > +          };
> > > > +      };
> > > > +
> > > > +...
> 
> -- 
> Regards,
> 
> Laurent Pinchart