Re: [PATCH 07/14] dt-bindings: pinctrl: stm32: support IO synchronization parameters

[Krzysztof Kozlowski <krzk@xxxxxxxxxx>]

On Tue, Oct 22, 2024 at 05:56:51PM +0200, Antonio Borneo wrote:
> From: Fabien Dessenne <fabien.dessenne@xxxxxxxxxxx>
> 
> Support the following IO synchronization parameters:
> - Delay (in ns)
> - Delay path (input / output)
> - Clock edge (single / double edge)
> - Clock inversion
> - Retiming

Why? What is missing for existing hardware support?

> 
> Signed-off-by: Fabien Dessenne <fabien.dessenne@xxxxxxxxxxx>
> Signed-off-by: Antonio Borneo <antonio.borneo@xxxxxxxxxxx>
> ---
>  .../bindings/pinctrl/st,stm32-pinctrl.yaml    | 48 +++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.yaml
> index 5d17d6487ae9c..9a7ecfea6eb5b 100644
> --- a/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.yaml
> +++ b/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.yaml
> @@ -207,6 +207,54 @@ patternProperties:
>                3: High speed
>              $ref: /schemas/types.yaml#/definitions/uint32
>              enum: [0, 1, 2, 3]
> +          st,io-delay-path:
> +            description: |
> +              IO synchronization delay path location
> +              0: Delay switched into the output path
> +              1: Delay switched into the input path
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            enum: [0, 1]

Why enum? Why not bool? What is the "synchronization delay"? Why this is
needed per board?

> +          st,io-clk-edge:
> +            description: |
> +              IO synchronization clock edge
> +              0: Data single-edge (changing on rising or falling clock edge)
> +              1: Data double-edge (changing on both clock edges)
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            enum: [0, 1]

All the same questions.

> +          st,io-clk-type:
> +            description: |
> +              IO synchronization clock inversion
> +              0: IO clocks not inverted. Data retimed to rising clock edge
> +              1: IO clocks inverted. Data retimed to falling clock edge
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            enum: [0, 1]

OK, so if not bool why this cannot be a readable string?

> +          st,io-retime:
> +            description: |
> +              IO synchronization data retime
> +              0: Data not synchronized or retimed on clock edges
> +              1: Data retimed to either rising or falling clock edge
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            enum: [0, 1]

Missing blank lines everywhere between properties.

> +          st,io-delay:

Use proper unit suffix. Or is there no such?

Best regards,
Krzysztof