Re: [PATCH v1 1/2] dt-bindings: sdhci: eswin: Documentation for eic7700 SoC

[Krzysztof Kozlowski <krzk@kernel.org>]

On 16/05/2025 11:16, dongxuyang@eswincomputing.com wrote:
> From: Xuyang Dong <dongxuyang@eswincomputing.com>
> 
> Add device tree binding documentation for the ESWIN
> eic7700 sdhci controller module.
> 
> Signed-off-by: Xiang Xu <xuxiang@eswincomputing.com>
> Signed-off-by: Xuyang Dong <dongxuyang@eswincomputing.com>
> ---


Folks,you need to slow down because you are sending patchsets with the
same issues, thus we need to repeat the same review comments. Learn from
the review first.

>  .../bindings/mmc/eswin,sdhci-eic7700.yaml     | 131 ++++++++++++++++++
>  1 file changed, 131 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mmc/eswin,sdhci-eic7700.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mmc/eswin,sdhci-eic7700.yaml b/Documentation/devicetree/bindings/mmc/eswin,sdhci-eic7700.yaml
> new file mode 100644
> index 000000000000..d4826f2aa619
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/eswin,sdhci-eic7700.yaml
> @@ -0,0 +1,131 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mmc/eswin,sdhci-eic7700.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ESWIN EIC7700 SoC SDHCI controller
> +
> +maintainers:
> +  - Shuang Liang <liangshuang@eswincomputing.com>
> +  - Xuyang Dong <dongxuyang@eswincomputing.com>
> +
> +allOf:
> +  - $ref: /schemas/mmc/mmc-controller.yaml#
> +
> +properties:
> +  compatible:
> +    const: eswin,eic7700-emmc-sdhci
> +
> +  reg:
> +    maxItems: 1
> +    description: Common configuration registers

Blank line, drop description

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

Blank line

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

This is wrong.

Why do you need these properties?  You are duplicating (and wrongly) MMC
schema.


> +
> +  interrupts:
> +    maxItems: 1
> +
> +  assigned-clocks:
> +    maxItems: 1
> +    description: should contain "core_clk" phandle + clock-specifier pairs.
> +
> +  assigned-clock-rates:
> +    maxItems: 1

Drop both properties. Do you see this anywhere in the bindings?

> +
> +  clocks:
> +    minItems: 2

No list items instead.

> +    description: handles to clock for the sdhci controller.

Drop description

> +
> +  clock-names:
> +    minItems: 2
> +    description: the name of each clock.

No, list items instead. Do you see any bindings written like that?

> +
> +  clock-output-names:
> +    maxItems: 1
> +
> +  '#clock-cells':
> +    enum: [0]
> +    description:
> +      With this property in place we will export one clock
> +      representing the Card Clock. These clocks are expected to be
> +      consumed by our PHY.
> +
> +  resets:
> +    description: resets to be used by the controller.

Missing constraints. Drop description, useless.

> +
> +  reset-names:
> +    description: names of the resets listed in resets property in the same order.

Drop property

> +
> +  bus-width:
> +    enum: [4, 8]
> +    description: for emmc bus-width is 8, for sdio bus-width is 4.

Why do you need it? Look at other bindings.

> +
> +  eswin,hsp_sp_csr:

Look at DTS coding style first. What characters are allowed in properties?

> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description: hsp_sp_csr regs to be used by the controller.

For what purpose?

> +    items:
> +      - description: phandle to HSP_SP_CSR register block
> +      - description: status register offset
> +      - description: control register offset
> +      - description: configuration register offset
> +
> +  eswin,syscrg_csr:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description: syscrg_csr regs to be used by the controller.

For what purpose?


> +    items:
> +      - description: phandle to SYS_CRG_CSR register block
> +      - description: status register offset
> +      - description: control register offset
> +      - description: configuration register offset
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - assigned-clocks
> +  - assigned-clock-rates
> +  - clocks
> +  - clock-names
> +  - clock-output-names
> +  - resets
> +  - reset-names
> +  - bus-width
> +  - eswin,hsp_sp_csr
> +  - eswin,syscrg_csr
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +

Drop blank line

> +    sdhci_emmc: mmc@50450000 {

Drop label

> +      compatible = "eswin,eic7700-emmc-sdhci";
> +      reg = <0x50450000 0x10000>;
> +      interrupt-parent = <&plic>;
> +      interrupts = <79>;
> +      clocks = <&clock 554>, <&clock 546>;
> +      clock-names = "clk_xin", "clk_ahb";
> +      assigned-clocks = <&clock 554>;
> +      assigned-clock-rates = <200000000>;
> +      clock-output-names = "emmc_cardclock";
> +      #clock-cells = <0>;
> +
> +      resets = <&reset 7 (1 << 6)>,
> +        <&reset 7 (1 << 3)>,
> +        <&reset 7 (1 << 19)>,
> +        <&reset 7 (1 << 23)>;
> +      reset-names = "txrx_rst", "phy_rst", "prstn", "arstn";
> +
> +      core-clk-reg = <0x51828160>;
> +      disable-cqe-dcmd;
> +      bus-width = <8>;
> +      non-removable;
> +      mmc-hs400-1_8v;
> +      max-frequency = <200000000>;
> +      eswin,hsp_sp_csr = <&hsp_sp_csr 0x1038 0x508 0x50c>;
> +      eswin,syscrg_csr = <&sys_crg 0x160 0x148 0x14c>;
> +      status = "disabled";

No, drop.


Best regards,
Krzysztof