Re: [PATCH v1 1/2] dt-bindings: ASoC: Add PDM controller for the StarFive JH8100 SoC

[Krzysztof Kozlowski]

On 07/03/2024 04:37, Xingyu Wu wrote:
> Add bindings for the PDM controller for the StarFive JH8100 SoC.

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.

> 
> Signed-off-by: Xingyu Wu <xingyu.wu@xxxxxxxxxxxxxxxx>
> ---
>  .../bindings/sound/starfive,jh8100-pdm.yaml   | 84 +++++++++++++++++++
>  1 file changed, 84 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/starfive,jh8100-pdm.yaml
> 
> diff --git a/Documentation/devicetree/bindings/sound/starfive,jh8100-pdm.yaml b/Documentation/devicetree/bindings/sound/starfive,jh8100-pdm.yaml
> new file mode 100644
> index 000000000000..a91b47d39ad3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/starfive,jh8100-pdm.yaml
> @@ -0,0 +1,84 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/starfive,jh8100-pdm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: StarFive JH8100 PDM controller
> +
> +description: |
> +  The Pulse Density Modulation (PDM) controller is a digital PDM out
> +  microphone interface controller and decoder that supports both
> +  mono/stereo PDM format, and an Inter-IC Sound (I2S) transmitter that
> +  outputs standard stereo audio data to another device. The I2S transmitter
> +  can be configured to operate either a master or a slave (default mode).
> +  The PDM controller includes two PDM modules, each PDM module can drive
> +  one bitstream sampling clock and two bitstream coming data with sampling
> +  clock rising and falling edge.
> +
> +maintainers:
> +  - Xingyu Wu <xingyu.wu@xxxxxxxxxxxxxxxx>
> +  - Walker Chen <walker.chen@xxxxxxxxxxxxxxxx>
> +
> +properties:
> +  compatible:
> +    const: starfive,jh8100-pdm
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: DMIC output clock
> +      - description: Main ICG clock
> +
> +  clock-names:
> +    items:
> +      - const: dmic
> +      - const: icg
> +
> +  resets:
> +    maxItems: 1
> +
> +  starfive,syscon:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      - items:
> +          - description: phandle to System Register Controller sys_syscon_ne node.
> +          - description: PDM source enabled control offset of SYS_SYSCON_NE register.
> +          - description: PDM source enabled control mask
> +    description:
> +      The phandle to System Register Controller syscon node and the PDM source
> +      from I2S enabled control offset and mask of SYS_SYSCON_NE register.
> +
> +  starfive,pdm-modulex:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1]
> +    description:
> +      The module x will be using in PDM controller. Default use module 0.

This is an index of the block instance? If so, then it's not allowed.
Otherwise I don't understand the description.

Anyway, don't repeat constraints in free form text. default: 0, if this
is going to stay.

> +
> +  "#sound-dai-cells":
> +    const: 0
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - resets
> +  - starfive,syscon
> +
> +unevaluatedProperties: false

This is wrong without $ref, which points you to missing $ref to dai-common.

> +
> +examples:
> +  - |
> +    pdm@12250000 {
> +      compatible = "starfive,jh8100-pdm";
> +      reg = <0x12250000 0x1000>;
> +      clocks = <&syscrg_ne 142>,
> +               <&syscrg_ne 171>;
> +      clock-names = "dmic", "icg";
> +      resets = <&syscrg_ne 44>;
> +      starfive,syscon = <&sys_syscon_ne 0xC 0xFF>;

Lowercase hex only.

> +      #sound-dai-cells = <0>;
> +    };

Best regards,
Krzysztof