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

[Xingyu Wu <xingyu.wu@xxxxxxxxxxxxxxxx>]

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

Okay, will fix.

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

No, this is just one instance. The PDM have two internal and independent modules or called channels.
They can be configured and used separately, and the user can choose which channel to use.

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

Oh, I just want to describe that if no this property, the driver default use module 0.
I will make this clear in next version.

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

Will add it. Thanks.

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

Will fix.

> 
> > +      #sound-dai-cells = <0>;
> > +    };
> 
> Best regards,
> Krzysztof

Thanks,
Xingyu Wu