Re: [PATCH v1 1/3] dt-bindings: mmc: Add bindings for StarFive

[Krzysztof Kozlowski]

On 07/12/2022 14:17, William Qiu wrote:
> Add documentation to describe StarFive
> designware mobile storage host controller driver.
> 
> Signed-off-by: William Qiu <william.qiu@xxxxxxxxxxxxxxxx>
> ---
>  .../bindings/mmc/starfive,jh7110-sdio.yaml    | 71 +++++++++++++++++++
>  1 file changed, 71 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.yaml b/Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.yaml
> new file mode 100644
> index 000000000000..4f27ef3cf4f3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/starfive,jh7110-sdio.yaml
> @@ -0,0 +1,71 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mmc/starfive,jh7110-sdio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: StarFive Designware Mobile Storage Host Controller
> +
> +description:
> +  StarFive uses the Synopsys designware mobile storage host controller
> +  to interface a SoC with storage medium such as eMMC or SD/MMC cards.
> +
> +allOf:
> +  - $ref: "synopsys-dw-mshc-common.yaml#"

Drop quotes

> +
> +maintainers:
> +  - William Qiu <william.qiu@xxxxxxxxxxxxxxxx>
> +
> +properties:
> +  compatible:
> +    const: starfive,jh7110-sdio

Why do you call it sdio if the interface is for mmc as well?

> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 1
> +    items:
> +      - description: biu clock
> +      - description: ciu clock

I don't think the card interface clock is optional... are you sure you
have designs working without it? No clock line at all getting to the memory?

> +
> +  clock-names:
> +    minItems: 1
> +    items:
> +      - const: biu
> +      - const: ciu
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  starfive,sys-syscon:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    description:
> +      The desired number of times that the host execute tuning when needed.

That's not matching the property name. Missing number of items... this
is anyway confusing. Why number of tuning tries is a property of DT?

> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - interrupts
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/starfive-jh7110.h>
> +    #include <dt-bindings/reset/starfive-jh7110.h>
> +    mmc@16010000 {
> +            compatible = "starfive,jh7110-sdio";

Use 4 spaces for example indentation.

> +            reg = <0x16010000 0x10000>;
> +            clocks = <&syscrg JH7110_SYSCLK_SDIO0_AHB>,
> +                 <&syscrg JH7110_SYSCLK_SDIO0_SDCARD>;

Align with previous <

Best regards,
Krzysztof