[v2,1/2] media: dt-bindings: alvium: add document YAML binding

  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
  

Hey Tommaso,

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

No dual license?

> +%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
> +
> +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
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    description: XCLK Input Clock

Description is a bit moot when you have the clock name and there's only
one. No harm done I suppose.

> +
> +  clock-names:
> +    const: xclk
> +
> +  powerdown-gpios:
> +    maxItems: 1
> +    description: >

You don't have any newlines, so you don't need a >

> +      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.
> +
> +  streamon-delay:
> +    maxItems: 1
> +    description: >
> +      Delay before camera start capturing frames in us.
> +
> +  rotation:
> +    enum:
> +      - 0
> +      - 180

Could style this as enum: [0, 180], but I don't mind which you do.

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

Label does not seem to be used & the generic node name should probably
be "camera", no?

> +              compatible = "alliedvision,alvium";
> +              pinctrl-names = "default";
> +              pinctrl-0 = <&pinctrl_csi0_pwn>, <&pinctrl_csi0_rst>, <&pinctrl_csi_mclk>;
> +              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 {

Ditto here, drop the unused label?

Otherwise, looks grand to me.

Cheers,
Conor.

> +                      remote-endpoint = <&mipi_csi_0_in>;
> +                      data-lanes = <1 2 3 4>;
> +                      link-frequencies = /bits/ 64 <681250000>;
> +                      clock-lanes = <0>;
> +                  };
> +              };
> +          };
> +      };
> +
> +...
> -- 
> 2.34.1
>
  

Hi Tommaso,

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
> +
> +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
> +
> +  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.
> +
> +  streamon-delay:
> +    maxItems: 1
> +    description: >
> +      Delay before camera start capturing frames in us.
> +
> +  rotation:
> +    enum:
> +      - 0
> +      - 180
> +
> +  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";
> +              pinctrl-names = "default";
> +              pinctrl-0 = <&pinctrl_csi0_pwn>, <&pinctrl_csi0_rst>, <&pinctrl_csi_mclk>;
> +              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>;
> +                  };
> +              };
> +          };
> +      };
> +
> +...
  

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.

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

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.

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

Hi Conor,
Thanks for the review.

On Fri, May 26, 2023 at 08:00:05PM +0100, Conor Dooley wrote:
> Hey Tommaso,
> 
> 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
> 
> No dual license?

Yep, agree. Thanks.

> 
> > +%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
> > +
> > +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
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    description: XCLK Input Clock
> 
> Description is a bit moot when you have the clock name and there's only
> one. No harm done I suppose.

Agree, we can drop description.

> 
> > +
> > +  clock-names:
> > +    const: xclk
> > +
> > +  powerdown-gpios:
> > +    maxItems: 1
> > +    description: >
> 
> You don't have any newlines, so you don't need a >

Thanks, I found the same ">" into ov5640 .yaml

> 
> > +      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.
> > +
> > +  streamon-delay:
> > +    maxItems: 1
> > +    description: >
> > +      Delay before camera start capturing frames in us.
> > +
> > +  rotation:
> > +    enum:
> > +      - 0
> > +      - 180
> 
> Could style this as enum: [0, 180], but I don't mind which you do.

For now this property is unused.
I'll drop this.

> 
> > +  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
> > +          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 {
> 
> Label does not seem to be used & the generic node name should probably
> be "camera", no?

What about using: "alvium: camera@3c {" ?
Like in some .yaml of ov sensors?

> 
> > +              compatible = "alliedvision,alvium";
> > +              pinctrl-names = "default";
> > +              pinctrl-0 = <&pinctrl_csi0_pwn>, <&pinctrl_csi0_rst>, <&pinctrl_csi_mclk>;
> > +              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 {
> 
> Ditto here, drop the unused label?

I think we need this.



Thanks!

Regards,
Tommaso

> 
> Otherwise, looks grand to me.
> 
> Cheers,
> Conor.
> 
> > +                      remote-endpoint = <&mipi_csi_0_in>;
> > +                      data-lanes = <1 2 3 4>;
> > +                      link-frequencies = /bits/ 64 <681250000>;
> > +                      clock-lanes = <0>;
> > +                  };
> > +              };
> > +          };
> > +      };
> > +
> > +...
> > -- 
> > 2.34.1
> >
  

Hi Sakari,

On Sun, May 28, 2023 at 09:16:05PM +0000, Sakari Ailus wrote:
> Hi Tommaso,
> 
> 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
> > +
> > +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
> > +
> > +  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.

Then you suggest also to drop devm_clk_get(dev, "xclk");
into the driver code and use devm_clk_get(&pdev->dev, NULL);
right?

> 
> > +
> > +  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.
> > +
> > +  streamon-delay:
> > +    maxItems: 1
> > +    description: >
> > +      Delay before camera start capturing frames in us.
> > +
> > +  rotation:
> > +    enum:
> > +      - 0
> > +      - 180
> > +
> > +  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.

Oks.

> 
> > +          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";
> > +              pinctrl-names = "default";
> > +              pinctrl-0 = <&pinctrl_csi0_pwn>, <&pinctrl_csi0_rst>, <&pinctrl_csi_mclk>;
> > +              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>;
> > +                  };
> > +              };
> > +          };
> > +      };
> > +
> > +...

Thanks,
Tommaso

> 
> -- 
> Kind regards,
> 
> Sakari Ailus
  

Hi Tommaso,

On Mon, May 29, 2023 at 09:41:23AM +0200, Tommaso Merciai 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
> > > +
> > > +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
> > > +
> > > +  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.
> 
> Then you suggest also to drop devm_clk_get(dev, "xclk");
> into the driver code and use devm_clk_get(&pdev->dev, NULL);
> right?

Actually, I don't see any clock input pin in the 22-pins FPC connector.
Are you sure the camera module needs an external clock ?

> > > +
> > > +  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.
> > > +
> > > +  streamon-delay:
> > > +    maxItems: 1
> > > +    description: >
> > > +      Delay before camera start capturing frames in us.
> > > +
> > > +  rotation:
> > > +    enum:
> > > +      - 0
> > > +      - 180
> > > +
> > > +  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.
> 
> Oks.
> 
> > 
> > > +          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";
> > > +              pinctrl-names = "default";
> > > +              pinctrl-0 = <&pinctrl_csi0_pwn>, <&pinctrl_csi0_rst>, <&pinctrl_csi_mclk>;
> > > +              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>;
> > > +                  };
> > > +              };
> > > +          };
> > > +      };
> > > +
> > > +...
  

Hello Laurent,

On Mon, May 29, 2023 at 09:39:07AM +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/

Arg... Thanks :)

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

You are completely right I will drop rst and pwdn pins.
About 2 gpios, we don't use those for now.

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

Thanks for the suggestion.
I will provide a cleared description on v3.

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

I'll drop rotation.

> 
> You need a property for the vcc-ext-in supply.

Can you give me more details about this?
Thanks.

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

oks

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

oks

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

Thanks for your time.

Regards,
Tommaso


> 
> -- 
> Regards,
> 
> Laurent Pinchart
  

On Mon, May 29, 2023 at 09:57:45AM +0200, Tommaso Merciai wrote:
> On Mon, May 29, 2023 at 09:39:07AM +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/
> 
> Arg... Thanks :)
> 
> > > > +
> > > > +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.
> > 
> > > > +
> > > > +  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 ?
> 
> You are completely right I will drop rst and pwdn pins.
> About 2 gpios, we don't use those for now.
> 
> > > > +
> > > > +  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.
> 
> Thanks for the suggestion.
> I will provide a cleared description on v3.
> 
> > > > +
> > > > +  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 ?
> 
> I'll drop rotation.
> 
> > You need a property for the vcc-ext-in supply.
> 
> Can you give me more details about this?
> Thanks.

The 22-pin connector has power supply pins, you need a corresponding
-supply property in DT to reference the regulator that provides the
supply (and you need to handle it in the driver too).

> > > > +
> > > > +  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.
> 
> oks
> 
> > 
> > > > +              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.
> 
> oks
> 
> > 
> > > > +              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>;
> > > > +                  };
> > > > +              };
> > > > +          };
> > > > +      };
> > > > +
> > > > +...
  

On 26/05/2023 19:39, Tommaso Merciai wrote:
> Add documentation of device tree in YAML schema for the ALVIUM
> Camera from Allied Vision Inc.
> 


> +
> +          camera: alvium@3c {
> +              compatible = "alliedvision,alvium";
> +              pinctrl-names = "default";
> +              pinctrl-0 = <&pinctrl_csi0_pwn>, <&pinctrl_csi0_rst>, <&pinctrl_csi_mclk>;
> +              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";

Please apply all my comments from v1. I don't see improvements.

Best regards,
Krzysztof
  

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
  

Hi Tommaso,

On Wed, May 31, 2023 at 12:20:47PM +0200, Tommaso Merciai wrote:
> 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?

As far as I understand, the camera modules allow accessing sensors
registers from the host (through the ISP) for fine-grained
configuration. Even if that's not implemented in the driver at the
moment, I think it's an important feature to eventually support, and it
will require a way for the system to identify the camera module
precisely, to know which sensor the module uses. That's why I would like
that information to be available in DT, in the form of a compatible
string. For instance,

	compatible = "alliedvision,alvium-1500c-1240c",
		     "alliedvision,alvium-1500c";

The driver will only need DT match entries for
"alliedvision,alvium-1500c" and "alliedvision,alvium-1800c".

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

Hi Laurent,

On Wed, May 31, 2023 at 02:06:00PM +0300, Laurent Pinchart wrote:
> Hi Tommaso,
> 
> On Wed, May 31, 2023 at 12:20:47PM +0200, Tommaso Merciai wrote:
> > 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?
> 
> As far as I understand, the camera modules allow accessing sensors
> registers from the host (through the ISP) for fine-grained
> configuration. Even if that's not implemented in the driver at the
> moment, I think it's an important feature to eventually support, and it
> will require a way for the system to identify the camera module
> precisely, to know which sensor the module uses. That's why I would like
> that information to be available in DT, in the form of a compatible
> string. For instance,
> 
> 	compatible = "alliedvision,alvium-1500c-1240c",
> 		     "alliedvision,alvium-1500c";
> 
> The driver will only need DT match entries for
> "alliedvision,alvium-1500c" and "alliedvision,alvium-1800c".

Thanks for your feedback.

Some clarification:

The Alvium doesn't allow direct access on the sensor.
Alvium hides the particular sensor totally.

For this reason I would prefer to keep generic
"alvium-csi2" name.

What do you think?

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
  

Hi Tommaso,

On Wed, May 31, 2023 at 04:01:57PM +0200, Tommaso Merciai wrote:
> On Wed, May 31, 2023 at 02:06:00PM +0300, Laurent Pinchart wrote:
> > On Wed, May 31, 2023 at 12:20:47PM +0200, Tommaso Merciai wrote:
> > > 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?
> > 
> > As far as I understand, the camera modules allow accessing sensors
> > registers from the host (through the ISP) for fine-grained
> > configuration. Even if that's not implemented in the driver at the
> > moment, I think it's an important feature to eventually support, and it
> > will require a way for the system to identify the camera module
> > precisely, to know which sensor the module uses. That's why I would like
> > that information to be available in DT, in the form of a compatible
> > string. For instance,
> > 
> > 	compatible = "alliedvision,alvium-1500c-1240c",
> > 		     "alliedvision,alvium-1500c";
> > 
> > The driver will only need DT match entries for
> > "alliedvision,alvium-1500c" and "alliedvision,alvium-1800c".
> 
> Thanks for your feedback.
> 
> Some clarification:
> 
> The Alvium doesn't allow direct access on the sensor.
> Alvium hides the particular sensor totally.

I think I got misled by the "Direct Register Access (DRA) to control the
cameras via registers for advanced users" mention on the Allied Vision
website, and thought direct register access to the sensor was possible.

> For this reason I would prefer to keep generic
> "alvium-csi2" name.

Given that the camera has device identification registers, that should
be OK.

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