Re: [PATCH 15/28] dt-bindings: dpll: Add device tree bindings for DPLL device and pin

[Krzysztof Kozlowski <krzk@kernel.org>]

On 07/04/2025 19:31, Ivan Vecera wrote:
> This adds DT bindings schema for DPLL (device phase-locked loop)

Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

A nit, subject: drop second/last, redundant "device tree bindings for".
The "dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

> device and associated pin. The schema follows existing DPLL core API

What is core API in terms of Devicetree?

> and should be used to expose information that should be provided
> by platform firmware.
> 
> The schema for DPLL device describe a DPLL chip that can contain
> one or more DPLLs (channels) and platform can specify their types.
> For now 'pps' and 'eec' types supported and these values are mapped
> to DPLL core's enums.

Describe entire hardware, not what is supported.

> 
> The DPLL device can have optionally 'input-pins' and 'output-pins'
> sub-nodes that contain pin sub-nodes.
> 
> These pin sub-nodes follows schema for dpll-pin and can contain
> information about the particular pin.

Describe the hardware, not the schema. We can read the contents of
patch. What we cannot read is the hardware and why you are making all
these choices.

> 
> The pin contains the following properties:
> * reg - pin HW index (physical pin number of given type)
> * label - string that is used as board label by DPLL core
> * type - string that indicates pin type (mapped to DPLL core pin type)
> * esync-control - boolean that indicates whether embeddded sync control
>                   is allowed for this pin
> * supported-frequencies - list of 64bit values that represents frequencies
>                           that are allowed to be configured for the pin

Drop. Describe the hardware.


> 
> Reviewed-by: Michal Schmidt <mschmidt@redhat.com>

Did this really happen?

> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
>  .../devicetree/bindings/dpll/dpll-device.yaml | 84 +++++++++++++++++++
>  .../devicetree/bindings/dpll/dpll-pin.yaml    | 43 ++++++++++
>  MAINTAINERS                                   |  2 +
>  3 files changed, 129 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/dpll/dpll-device.yaml
>  create mode 100644 Documentation/devicetree/bindings/dpll/dpll-pin.yaml

Filenames matching compatibles... unless this is common schema, but
commit description did not mention it.

> 
> diff --git a/Documentation/devicetree/bindings/dpll/dpll-device.yaml b/Documentation/devicetree/bindings/dpll/dpll-device.yaml
> new file mode 100644
> index 0000000000000..e6c309abb857f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dpll/dpll-device.yaml
> @@ -0,0 +1,84 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/dpll/dpll-device.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Digital Phase-Locked Loop (DPLL) Device
> +
> +maintainers:
> +  - Ivan Vecera <ivecera@redhat.com>
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> +  Digital Phase-Locked Loop (DPLL) device are used for precise clock
> +  synchronization in networking and telecom hardware. The device can
> +  have one or more channels (DPLLs) and one or more input and output
> +  pins. Each DPLL channel can either produce pulse-per-clock signal
> +  or drive ethernet equipment clock. The type of each channel is
> +  indicated by dpll-types property.
> +
> +properties:
> +  $nodename:
> +    pattern: "^dpll(@.*)?$"
> +
> +  "#address-cells":
> +    const: 0
> +
> +  "#size-cells":
> +    const: 0

Why do you need these cells?

> +
> +  num-dplls:
> +    description: Number of DPLL channels in this device.

Why this is not deducible from compatible?

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 1
> +
> +  dpll-types:
> +    description: List of DPLL types, one per DPLL instance.
> +    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
> +    items:
> +      enum: [pps, eec]

Why this is not deducible from compatible?

> +
> +  input-pins:
> +    type: object
> +    description: DPLL input pins
> +    unevaluatedProperties: false

So this is all for pinctrl? Or something else? Could not figure out from
commit msg. This does not help me either.

> +
> +    properties:
> +      "#address-cells":
> +        const: 1

Why?

> +      "#size-cells":
> +        const: 0

Why? I don't see these being used.

> +
> +    patternProperties:
> +      "^pin@[0-9]+$":
> +        $ref: /schemas/dpll/dpll-pin.yaml
> +        unevaluatedProperties: false
> +
> +    required:
> +      - "#address-cells"
> +      - "#size-cells"
> +
> +  output-pins:
> +    type: object
> +    description: DPLL output pins
> +    unevaluatedProperties: false
> +
> +    properties:
> +      "#address-cells":
> +        const: 1
> +      "#size-cells":
> +        const: 0
> +
> +    patternProperties:
> +      "^pin@[0-9]+$":
> +        $ref: /schemas/dpll/dpll-pin.yaml
> +        unevaluatedProperties: false
> +
> +    required:
> +      - "#address-cells"
> +      - "#size-cells"
> +
> +dependentRequired:
> +  dpll-types: [ num-dplls ]
> +
> +additionalProperties: true
> diff --git a/Documentation/devicetree/bindings/dpll/dpll-pin.yaml b/Documentation/devicetree/bindings/dpll/dpll-pin.yaml
> new file mode 100644
> index 0000000000000..9aea8ceabb5af
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dpll/dpll-pin.yaml
> @@ -0,0 +1,43 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/dpll/dpll-pin.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: DPLL Pin
> +
> +maintainers:
> +  - Ivan Vecera <ivecera@redhat.com>
> +
> +description: |
> +  Schema for defining input and output pins of a Digital Phase-Locked Loop (DPLL).
> +  Each pin can have a set of supported frequencies, label, type and may support
> +  embedded sync.
> +
> +properties:
> +  reg:
> +    description: Hardware index of the pin.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  esync-control:
> +    description: Indicates whether the pin supports embedded sync functionality.
> +    type: boolean
> +
> +  label:
> +    description: String exposed as the pin board label
> +    $ref: /schemas/types.yaml#/definitions/string
> +
> +  supported-frequencies:
> +    description: List of supported frequencies for this pin, expressed in Hz.
> +    $ref: /schemas/types.yaml#/definitions/uint64-array

Use common property suffixes and drop ref.

> +
> +  type:
> +    description: Type of the pin
> +    $ref: /schemas/types.yaml#/definitions/string
> +    enum: [ext, gnss, int, mux, synce]
> +
> +

Just one blank line


I bet that half of my questions could be answered with proper hardware
description which is missing in commit msg and binding description.
Instead your commit msg explains schema which makes no sense - I
mentioned, we can read the schema.
> +required:
> +  - reg
Best regards,
Krzysztof