[v3,2/3] media: dt-bindings: alvium: add document YAML binding

Commit Message

Tommaso Merciai June 6, 2023, 3:54 p.m. UTC

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 <[e-mail]>
---
Changes since v1:
 - Fixed build error as suggested by RHerring bot

Changes since v2:
 - Fixed License as suggested by KKozlowski/CDooley
 - Removed rotation property as suggested by CDooley/LPinchart
 - Fixed example node name as suggested by CDooley
 - Fixed title as suggested by LPinchart
 - Fixed compatible name as suggested by LPinchart
 - Removed clock as suggested by LPinchart
 - Removed gpios not as suggested by LPinchart
 - Renamed property name streamon-delay into alliedvision,lp2hs-delay-us
 - Fixed vendor prefix, unit append as suggested by KKozlowski
 - Fixed data-lanes
 - Fixed blank space + example indentation (from 6 -> 4 space) as suggested by KKozlowski
 - Dropped status into example  as suggested by KKozlowski
 - Added vcc-ext-in supply as suggested by LPinchart
 - Dropped pinctrl into example as suggested by LPinchart

 .../media/i2c/alliedvision,alvium-csi2.yaml   | 93 +++++++++++++++++++
 1 file changed, 93 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/alliedvision,alvium-csi2.yaml

Comments

Rob Herring (Arm) June 6, 2023, 4:31 p.m. UTC | #1

On Tue, 06 Jun 2023 17:54:03 +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 <[e-mail]>
> ---
> Changes since v1:
>  - Fixed build error as suggested by RHerring bot
> 
> Changes since v2:
>  - Fixed License as suggested by KKozlowski/CDooley
>  - Removed rotation property as suggested by CDooley/LPinchart
>  - Fixed example node name as suggested by CDooley
>  - Fixed title as suggested by LPinchart
>  - Fixed compatible name as suggested by LPinchart
>  - Removed clock as suggested by LPinchart
>  - Removed gpios not as suggested by LPinchart
>  - Renamed property name streamon-delay into alliedvision,lp2hs-delay-us
>  - Fixed vendor prefix, unit append as suggested by KKozlowski
>  - Fixed data-lanes
>  - Fixed blank space + example indentation (from 6 -> 4 space) as suggested by KKozlowski
>  - Dropped status into example  as suggested by KKozlowski
>  - Added vcc-ext-in supply as suggested by LPinchart
>  - Dropped pinctrl into example as suggested by LPinchart
> 
>  .../media/i2c/alliedvision,alvium-csi2.yaml   | 93 +++++++++++++++++++
>  1 file changed, 93 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/alliedvision,alvium-csi2.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
./Documentation/devicetree/bindings/media/i2c/alliedvision,alvium-csi2.yaml: $id: relative path/filename doesn't match actual path or filename
	expected: http://devicetree.org/schemas/media/i2c/alliedvision,alvium-csi2.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[e-mail]

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Laurent Pinchart June 6, 2023, 4:36 p.m. UTC | #2

Hi Tommaso,

Thank you for the patch.

On Tue, Jun 06, 2023 at 05:54:03PM +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 <[e-mail]>
> ---
> Changes since v1:
>  - Fixed build error as suggested by RHerring bot
> 
> Changes since v2:
>  - Fixed License as suggested by KKozlowski/CDooley
>  - Removed rotation property as suggested by CDooley/LPinchart
>  - Fixed example node name as suggested by CDooley
>  - Fixed title as suggested by LPinchart
>  - Fixed compatible name as suggested by LPinchart
>  - Removed clock as suggested by LPinchart
>  - Removed gpios not as suggested by LPinchart
>  - Renamed property name streamon-delay into alliedvision,lp2hs-delay-us
>  - Fixed vendor prefix, unit append as suggested by KKozlowski
>  - Fixed data-lanes
>  - Fixed blank space + example indentation (from 6 -> 4 space) as suggested by KKozlowski
>  - Dropped status into example  as suggested by KKozlowski
>  - Added vcc-ext-in supply as suggested by LPinchart
>  - Dropped pinctrl into example as suggested by LPinchart
> 
>  .../media/i2c/alliedvision,alvium-csi2.yaml   | 93 +++++++++++++++++++
>  1 file changed, 93 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/alliedvision,alvium-csi2.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/alliedvision,alvium-csi2.yaml b/Documentation/devicetree/bindings/media/i2c/alliedvision,alvium-csi2.yaml
> new file mode 100644
> index 000000000000..191534e2f7bd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/alliedvision,alvium-csi2.yaml
> @@ -0,0 +1,93 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/alliedvision,alvium.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Allied Vision Alvium Camera
> +
> +maintainers:
> +  - Tommaso Merciai <[e-mail]>
> +  - Martin Hecht <[e-mail]>
> +
> +allOf:
> +  - $ref: /schemas/media/video-interface-devices.yaml#
> +
> +properties:
> +  compatible:
> +    const: alliedvision,alvium-csi2
> +
> +  reg:
> +    maxItems: 1
> +
> +  vcc-ext-in-supply:
> +    description:
> +      Definition of the regulator used as interface power supply.

      The regulator that supplies power to the VCC_EXT_IN pins.

> +
> +  alliedvision,lp2hs-delay-us:
> +    maxItems: 1
> +    description:
> +      Low power to high speed delay time in microseconds.

You can drop "in microseconds", that's implied by the suffix.

> +      The purpose of this property is force a DPhy reset for the period
> +      described by the microseconds on the property, before it starts
> +      streaming. To be clear, with that value bigger than 0 the Alvium
> +      forces a dphy-reset on all lanes for that period. That means all
> +      lanes go up into low power state. This may help a csi2 rx ip to
> +      reset if that IP can't deal with a continous clock.

I'd like to propose what I think is a clearer version:

    description: |
      Low power to high speed delay time.

      If the value is larger than 0, the camera forces a reset of all
      D-PHY lanes for the duration specified by this property. All lanes
      will transition to the low-power state and back to the high-speed
      state after the delay. Otherwise the lanes will transition to and
      remain in the high-speed state immediately after power on.

      This is meant to help CSI-2 receivers synchronizing their D-PHY
      RX.

Reviewed-by: Laurent Pinchart <[e-mail]>

> +
> +  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:
> +          link-frequencies: true
> +
> +          data-lanes:
> +            minItems: 1
> +            items:
> +              - const: 1
> +              - const: 2
> +              - const: 3
> +              - const: 4
> +
> +        required:
> +          - data-lanes
> +          - link-frequencies
> +
> +required:
> +  - compatible
> +  - reg
> +  - vcc-ext-in-supply
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        alvium: camera@3c {
> +            compatible = "alliedvision,alvium-csi2";
> +            reg = <0x3c>;
> +            vcc-ext-in-supply = <&reg_vcc_ext_in>;
> +            alliedvision,lp2hs-delay-us = <20>;
> +
> +            port {
> +                alvium_out: endpoint {
> +                    remote-endpoint = <&mipi_csi_0_in>;
> +                    data-lanes = <1 2 3 4>;
> +                    link-frequencies = /bits/ 64 <681250000>;
> +                };
> +            };
> +        };
> +    };
> +
> +...

Conor Dooley June 6, 2023, 6:07 p.m. UTC | #3

Hey Laurent, Tommaso,

On Tue, Jun 06, 2023 at 07:36:56PM +0300, Laurent Pinchart wrote:
> On Tue, Jun 06, 2023 at 05:54:03PM +0200, Tommaso Merciai wrote:

> > +  alliedvision,lp2hs-delay-us:
> > +    maxItems: 1
> > +    description:
> > +      Low power to high speed delay time in microseconds.
> 
> You can drop "in microseconds", that's implied by the suffix.
> 
> > +      The purpose of this property is force a DPhy reset for the period
> > +      described by the microseconds on the property, before it starts
> > +      streaming. To be clear, with that value bigger than 0 the Alvium
> > +      forces a dphy-reset on all lanes for that period. That means all
> > +      lanes go up into low power state. This may help a csi2 rx ip to
> > +      reset if that IP can't deal with a continous clock.
> 
> I'd like to propose what I think is a clearer version:
> 
>     description: |
>       Low power to high speed delay time.
> 
>       If the value is larger than 0, the camera forces a reset of all
>       D-PHY lanes for the duration specified by this property. All lanes
>       will transition to the low-power state and back to the high-speed
>       state after the delay. Otherwise the lanes will transition to and
>       remain in the high-speed state immediately after power on.
> 
>       This is meant to help CSI-2 receivers synchronizing their D-PHY
>       RX.

Question about the property.
Why not make it have a minimum value of 1 and drop the special-case
behaviour for zero?

Cheers,
Conor.

Laurent Pinchart June 6, 2023, 6:17 p.m. UTC | #4

On Tue, Jun 06, 2023 at 07:07:42PM +0100, Conor Dooley wrote:
> Hey Laurent, Tommaso,
> 
> On Tue, Jun 06, 2023 at 07:36:56PM +0300, Laurent Pinchart wrote:
> > On Tue, Jun 06, 2023 at 05:54:03PM +0200, Tommaso Merciai wrote:
> 
> > > +  alliedvision,lp2hs-delay-us:
> > > +    maxItems: 1
> > > +    description:
> > > +      Low power to high speed delay time in microseconds.
> > 
> > You can drop "in microseconds", that's implied by the suffix.
> > 
> > > +      The purpose of this property is force a DPhy reset for the period
> > > +      described by the microseconds on the property, before it starts
> > > +      streaming. To be clear, with that value bigger than 0 the Alvium
> > > +      forces a dphy-reset on all lanes for that period. That means all
> > > +      lanes go up into low power state. This may help a csi2 rx ip to
> > > +      reset if that IP can't deal with a continous clock.
> > 
> > I'd like to propose what I think is a clearer version:
> > 
> >     description: |
> >       Low power to high speed delay time.
> > 
> >       If the value is larger than 0, the camera forces a reset of all
> >       D-PHY lanes for the duration specified by this property. All lanes
> >       will transition to the low-power state and back to the high-speed
> >       state after the delay. Otherwise the lanes will transition to and
> >       remain in the high-speed state immediately after power on.
> > 
> >       This is meant to help CSI-2 receivers synchronizing their D-PHY
> >       RX.
> 
> Question about the property.
> Why not make it have a minimum value of 1 and drop the special-case
> behaviour for zero?

The property is optional, so it can indeed be omitted if no delay is
desired. I have no strong preference on whether or not to allow 0 as a
valid value.

Conor Dooley June 6, 2023, 6:23 p.m. UTC | #5

On Tue, Jun 06, 2023 at 09:17:52PM +0300, Laurent Pinchart wrote:
> On Tue, Jun 06, 2023 at 07:07:42PM +0100, Conor Dooley wrote:
> > Hey Laurent, Tommaso,
> > 
> > On Tue, Jun 06, 2023 at 07:36:56PM +0300, Laurent Pinchart wrote:
> > > On Tue, Jun 06, 2023 at 05:54:03PM +0200, Tommaso Merciai wrote:
> > 
> > > > +  alliedvision,lp2hs-delay-us:
> > > > +    maxItems: 1
> > > > +    description:
> > > > +      Low power to high speed delay time in microseconds.
> > > 
> > > You can drop "in microseconds", that's implied by the suffix.
> > > 
> > > > +      The purpose of this property is force a DPhy reset for the period
> > > > +      described by the microseconds on the property, before it starts
> > > > +      streaming. To be clear, with that value bigger than 0 the Alvium
> > > > +      forces a dphy-reset on all lanes for that period. That means all
> > > > +      lanes go up into low power state. This may help a csi2 rx ip to
> > > > +      reset if that IP can't deal with a continous clock.
> > > 
> > > I'd like to propose what I think is a clearer version:
> > > 
> > >     description: |
> > >       Low power to high speed delay time.
> > > 
> > >       If the value is larger than 0, the camera forces a reset of all
> > >       D-PHY lanes for the duration specified by this property. All lanes
> > >       will transition to the low-power state and back to the high-speed
> > >       state after the delay. Otherwise the lanes will transition to and
> > >       remain in the high-speed state immediately after power on.
> > > 
> > >       This is meant to help CSI-2 receivers synchronizing their D-PHY
> > >       RX.
> > 
> > Question about the property.
> > Why not make it have a minimum value of 1 and drop the special-case
> > behaviour for zero?
> 
> The property is optional, so it can indeed be omitted if no delay is
> desired. I have no strong preference on whether or not to allow 0 as a
> valid value.

FWIW, I prefer the semantics of the property if it doesn't have the
limbo state of being present but doing nothing.

Cheers,
Conor.

BTW, I seem to get bounces from [e-mail], who is listed in
MAINTAINERS for several drivers. Do you know if they have a non-intel
address to replace those entries with, or should they be dropped?

Tommaso Merciai June 7, 2023, 7:11 a.m. UTC | #6

Hi Laurent,

On Tue, Jun 06, 2023 at 07:36:56PM +0300, Laurent Pinchart wrote:
> Hi Tommaso,
> 
> Thank you for the patch.
> 
> On Tue, Jun 06, 2023 at 05:54:03PM +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 <[e-mail]>
> > ---
> > Changes since v1:
> >  - Fixed build error as suggested by RHerring bot
> > 
> > Changes since v2:
> >  - Fixed License as suggested by KKozlowski/CDooley
> >  - Removed rotation property as suggested by CDooley/LPinchart
> >  - Fixed example node name as suggested by CDooley
> >  - Fixed title as suggested by LPinchart
> >  - Fixed compatible name as suggested by LPinchart
> >  - Removed clock as suggested by LPinchart
> >  - Removed gpios not as suggested by LPinchart
> >  - Renamed property name streamon-delay into alliedvision,lp2hs-delay-us
> >  - Fixed vendor prefix, unit append as suggested by KKozlowski
> >  - Fixed data-lanes
> >  - Fixed blank space + example indentation (from 6 -> 4 space) as suggested by KKozlowski
> >  - Dropped status into example  as suggested by KKozlowski
> >  - Added vcc-ext-in supply as suggested by LPinchart
> >  - Dropped pinctrl into example as suggested by LPinchart
> > 
> >  .../media/i2c/alliedvision,alvium-csi2.yaml   | 93 +++++++++++++++++++
> >  1 file changed, 93 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/alliedvision,alvium-csi2.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/alliedvision,alvium-csi2.yaml b/Documentation/devicetree/bindings/media/i2c/alliedvision,alvium-csi2.yaml
> > new file mode 100644
> > index 000000000000..191534e2f7bd
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/alliedvision,alvium-csi2.yaml
> > @@ -0,0 +1,93 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/i2c/alliedvision,alvium.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Allied Vision Alvium Camera
> > +
> > +maintainers:
> > +  - Tommaso Merciai <[e-mail]>
> > +  - Martin Hecht <[e-mail]>
> > +
> > +allOf:
> > +  - $ref: /schemas/media/video-interface-devices.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    const: alliedvision,alvium-csi2
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  vcc-ext-in-supply:
> > +    description:
> > +      Definition of the regulator used as interface power supply.
> 
>       The regulator that supplies power to the VCC_EXT_IN pins.

Oks. Ty.

> 
> > +
> > +  alliedvision,lp2hs-delay-us:
> > +    maxItems: 1
> > +    description:
> > +      Low power to high speed delay time in microseconds.
> 
> You can drop "in microseconds", that's implied by the suffix.

Oks.

> 
> > +      The purpose of this property is force a DPhy reset for the period
> > +      described by the microseconds on the property, before it starts
> > +      streaming. To be clear, with that value bigger than 0 the Alvium
> > +      forces a dphy-reset on all lanes for that period. That means all
> > +      lanes go up into low power state. This may help a csi2 rx ip to
> > +      reset if that IP can't deal with a continous clock.
> 
> I'd like to propose what I think is a clearer version:
> 
>     description: |
>       Low power to high speed delay time.
> 
>       If the value is larger than 0, the camera forces a reset of all
>       D-PHY lanes for the duration specified by this property. All lanes
>       will transition to the low-power state and back to the high-speed
>       state after the delay. Otherwise the lanes will transition to and
>       remain in the high-speed state immediately after power on.
> 
>       This is meant to help CSI-2 receivers synchronizing their D-PHY
>       RX.

Thanks!
I'll fix this in v4 :)

Regards,
Tommaso

> 
> Reviewed-by: Laurent Pinchart <[e-mail]>
> 
> > +
> > +  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:
> > +          link-frequencies: true
> > +
> > +          data-lanes:
> > +            minItems: 1
> > +            items:
> > +              - const: 1
> > +              - const: 2
> > +              - const: 3
> > +              - const: 4
> > +
> > +        required:
> > +          - data-lanes
> > +          - link-frequencies
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - vcc-ext-in-supply
> > +  - port
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        alvium: camera@3c {
> > +            compatible = "alliedvision,alvium-csi2";
> > +            reg = <0x3c>;
> > +            vcc-ext-in-supply = <&reg_vcc_ext_in>;
> > +            alliedvision,lp2hs-delay-us = <20>;
> > +
> > +            port {
> > +                alvium_out: endpoint {
> > +                    remote-endpoint = <&mipi_csi_0_in>;
> > +                    data-lanes = <1 2 3 4>;
> > +                    link-frequencies = /bits/ 64 <681250000>;
> > +                };
> > +            };
> > +        };
> > +    };
> > +
> > +...
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Tommaso Merciai June 7, 2023, 7:14 a.m. UTC | #7

Hi Conor,

Thanks for your feedback.

On Tue, Jun 06, 2023 at 07:07:42PM +0100, Conor Dooley wrote:
> Hey Laurent, Tommaso,
> 
> On Tue, Jun 06, 2023 at 07:36:56PM +0300, Laurent Pinchart wrote:
> > On Tue, Jun 06, 2023 at 05:54:03PM +0200, Tommaso Merciai wrote:
> 
> > > +  alliedvision,lp2hs-delay-us:
> > > +    maxItems: 1
> > > +    description:
> > > +      Low power to high speed delay time in microseconds.
> > 
> > You can drop "in microseconds", that's implied by the suffix.
> > 
> > > +      The purpose of this property is force a DPhy reset for the period
> > > +      described by the microseconds on the property, before it starts
> > > +      streaming. To be clear, with that value bigger than 0 the Alvium
> > > +      forces a dphy-reset on all lanes for that period. That means all
> > > +      lanes go up into low power state. This may help a csi2 rx ip to
> > > +      reset if that IP can't deal with a continous clock.
> > 
> > I'd like to propose what I think is a clearer version:
> > 
> >     description: |
> >       Low power to high speed delay time.
> > 
> >       If the value is larger than 0, the camera forces a reset of all
> >       D-PHY lanes for the duration specified by this property. All lanes
> >       will transition to the low-power state and back to the high-speed
> >       state after the delay. Otherwise the lanes will transition to and
> >       remain in the high-speed state immediately after power on.
> > 
> >       This is meant to help CSI-2 receivers synchronizing their D-PHY
> >       RX.
> 
> Question about the property.
> Why not make it have a minimum value of 1 and drop the special-case
> behaviour for zero?

Personally I prefer to stay with zero case.
This reflect better the real camera register behaviour.

(also is optional)

Thanks! :)
Tommaso

> 
> Cheers,
> Conor.

Tommaso Merciai June 7, 2023, 7:16 a.m. UTC | #8

Hi Conor,

On Tue, Jun 06, 2023 at 07:23:32PM +0100, Conor Dooley wrote:
> On Tue, Jun 06, 2023 at 09:17:52PM +0300, Laurent Pinchart wrote:
> > On Tue, Jun 06, 2023 at 07:07:42PM +0100, Conor Dooley wrote:
> > > Hey Laurent, Tommaso,
> > > 
> > > On Tue, Jun 06, 2023 at 07:36:56PM +0300, Laurent Pinchart wrote:
> > > > On Tue, Jun 06, 2023 at 05:54:03PM +0200, Tommaso Merciai wrote:
> > > 
> > > > > +  alliedvision,lp2hs-delay-us:
> > > > > +    maxItems: 1
> > > > > +    description:
> > > > > +      Low power to high speed delay time in microseconds.
> > > > 
> > > > You can drop "in microseconds", that's implied by the suffix.
> > > > 
> > > > > +      The purpose of this property is force a DPhy reset for the period
> > > > > +      described by the microseconds on the property, before it starts
> > > > > +      streaming. To be clear, with that value bigger than 0 the Alvium
> > > > > +      forces a dphy-reset on all lanes for that period. That means all
> > > > > +      lanes go up into low power state. This may help a csi2 rx ip to
> > > > > +      reset if that IP can't deal with a continous clock.
> > > > 
> > > > I'd like to propose what I think is a clearer version:
> > > > 
> > > >     description: |
> > > >       Low power to high speed delay time.
> > > > 
> > > >       If the value is larger than 0, the camera forces a reset of all
> > > >       D-PHY lanes for the duration specified by this property. All lanes
> > > >       will transition to the low-power state and back to the high-speed
> > > >       state after the delay. Otherwise the lanes will transition to and
> > > >       remain in the high-speed state immediately after power on.
> > > > 
> > > >       This is meant to help CSI-2 receivers synchronizing their D-PHY
> > > >       RX.
> > > 
> > > Question about the property.
> > > Why not make it have a minimum value of 1 and drop the special-case
> > > behaviour for zero?
> > 
> > The property is optional, so it can indeed be omitted if no delay is
> > desired. I have no strong preference on whether or not to allow 0 as a
> > valid value.
> 
> FWIW, I prefer the semantics of the property if it doesn't have the
> limbo state of being present but doing nothing.
> 
> Cheers,
> Conor.
> 
> BTW, I seem to get bounces from [e-mail], who is listed in
> MAINTAINERS for several drivers. Do you know if they have a non-intel
> address to replace those entries with, or should they be dropped?

Same here! :)

Thanks,
Tommaso

Laurent Pinchart June 7, 2023, 7:21 a.m. UTC | #9

On Wed, Jun 07, 2023 at 09:14:48AM +0200, Tommaso Merciai wrote:
> On Tue, Jun 06, 2023 at 07:07:42PM +0100, Conor Dooley wrote:
> > On Tue, Jun 06, 2023 at 07:36:56PM +0300, Laurent Pinchart wrote:
> > > On Tue, Jun 06, 2023 at 05:54:03PM +0200, Tommaso Merciai wrote:
> > 
> > > > +  alliedvision,lp2hs-delay-us:
> > > > +    maxItems: 1
> > > > +    description:
> > > > +      Low power to high speed delay time in microseconds.
> > > 
> > > You can drop "in microseconds", that's implied by the suffix.
> > > 
> > > > +      The purpose of this property is force a DPhy reset for the period
> > > > +      described by the microseconds on the property, before it starts
> > > > +      streaming. To be clear, with that value bigger than 0 the Alvium
> > > > +      forces a dphy-reset on all lanes for that period. That means all
> > > > +      lanes go up into low power state. This may help a csi2 rx ip to
> > > > +      reset if that IP can't deal with a continous clock.
> > > 
> > > I'd like to propose what I think is a clearer version:
> > > 
> > >     description: |
> > >       Low power to high speed delay time.
> > > 
> > >       If the value is larger than 0, the camera forces a reset of all
> > >       D-PHY lanes for the duration specified by this property. All lanes
> > >       will transition to the low-power state and back to the high-speed
> > >       state after the delay. Otherwise the lanes will transition to and
> > >       remain in the high-speed state immediately after power on.
> > > 
> > >       This is meant to help CSI-2 receivers synchronizing their D-PHY
> > >       RX.
> > 
> > Question about the property.
> > Why not make it have a minimum value of 1 and drop the special-case
> > behaviour for zero?
> 
> Personally I prefer to stay with zero case.
> This reflect better the real camera register behaviour.

Speaking of which, could you document the maximum value in the bindings
?

> (also is optional)

Tommaso Merciai June 7, 2023, 11:27 a.m. UTC | #10

Hi Laurent,

On Wed, Jun 07, 2023 at 10:21:34AM +0300, Laurent Pinchart wrote:
> On Wed, Jun 07, 2023 at 09:14:48AM +0200, Tommaso Merciai wrote:
> > On Tue, Jun 06, 2023 at 07:07:42PM +0100, Conor Dooley wrote:
> > > On Tue, Jun 06, 2023 at 07:36:56PM +0300, Laurent Pinchart wrote:
> > > > On Tue, Jun 06, 2023 at 05:54:03PM +0200, Tommaso Merciai wrote:
> > > 
> > > > > +  alliedvision,lp2hs-delay-us:
> > > > > +    maxItems: 1
> > > > > +    description:
> > > > > +      Low power to high speed delay time in microseconds.
> > > > 
> > > > You can drop "in microseconds", that's implied by the suffix.
> > > > 
> > > > > +      The purpose of this property is force a DPhy reset for the period
> > > > > +      described by the microseconds on the property, before it starts
> > > > > +      streaming. To be clear, with that value bigger than 0 the Alvium
> > > > > +      forces a dphy-reset on all lanes for that period. That means all
> > > > > +      lanes go up into low power state. This may help a csi2 rx ip to
> > > > > +      reset if that IP can't deal with a continous clock.
> > > > 
> > > > I'd like to propose what I think is a clearer version:
> > > > 
> > > >     description: |
> > > >       Low power to high speed delay time.
> > > > 
> > > >       If the value is larger than 0, the camera forces a reset of all
> > > >       D-PHY lanes for the duration specified by this property. All lanes
> > > >       will transition to the low-power state and back to the high-speed
> > > >       state after the delay. Otherwise the lanes will transition to and
> > > >       remain in the high-speed state immediately after power on.
> > > > 
> > > >       This is meant to help CSI-2 receivers synchronizing their D-PHY
> > > >       RX.
> > > 
> > > Question about the property.
> > > Why not make it have a minimum value of 1 and drop the special-case
> > > behaviour for zero?
> > 
> > Personally I prefer to stay with zero case.
> > This reflect better the real camera register behaviour.
> 
> Speaking of which, could you document the maximum value in the bindings
> ?

150ms is the max value I'll document this in v4.

Regards,
Tommaso

> 
> > (also is optional)
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch

diff --git a/Documentation/devicetree/bindings/media/i2c/alliedvision,alvium-csi2.yaml b/Documentation/devicetree/bindings/media/i2c/alliedvision,alvium-csi2.yaml
new file mode 100644
index 000000000000..191534e2f7bd
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/alliedvision,alvium-csi2.yaml
@@ -0,0 +1,93 @@ 
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/alliedvision,alvium.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Allied Vision Alvium Camera
+
+maintainers:
+  - Tommaso Merciai <[e-mail]>
+  - Martin Hecht <[e-mail]>
+
+allOf:
+  - $ref: /schemas/media/video-interface-devices.yaml#
+
+properties:
+  compatible:
+    const: alliedvision,alvium-csi2
+
+  reg:
+    maxItems: 1
+
+  vcc-ext-in-supply:
+    description:
+      Definition of the regulator used as interface power supply.
+
+  alliedvision,lp2hs-delay-us:
+    maxItems: 1
+    description:
+      Low power to high speed delay time in microseconds.
+      The purpose of this property is force a DPhy reset for the period
+      described by the microseconds on the property, before it starts
+      streaming. To be clear, with that value bigger than 0 the Alvium
+      forces a dphy-reset on all lanes for that period. That means all
+      lanes go up into low power state. This may help a csi2 rx ip to
+      reset if that IP can't deal with a continous clock.
+
+  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:
+          link-frequencies: true
+
+          data-lanes:
+            minItems: 1
+            items:
+              - const: 1
+              - const: 2
+              - const: 3
+              - const: 4
+
+        required:
+          - data-lanes
+          - link-frequencies
+
+required:
+  - compatible
+  - reg
+  - vcc-ext-in-supply
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        alvium: camera@3c {
+            compatible = "alliedvision,alvium-csi2";
+            reg = <0x3c>;
+            vcc-ext-in-supply = <&reg_vcc_ext_in>;
+            alliedvision,lp2hs-delay-us = <20>;
+
+            port {
+                alvium_out: endpoint {
+                    remote-endpoint = <&mipi_csi_0_in>;
+                    data-lanes = <1 2 3 4>;
+                    link-frequencies = /bits/ 64 <681250000>;
+                };
+            };
+        };
+    };
+
+...