Re: [PATCH 1/2] dt-bindings: mmc: nuvoton,ma35d1-sdhci: Document MA35D1 SDHCI controller

[Krzysztof Kozlowski <krzk@kernel.org>]

On 19/06/2024 07:46, Shan-Chun Hung wrote:
> Add binding for Nuvoton MA35D1 SDHCI controller.
> 
> Signed-off-by: schung <schung@nuvoton.com>

Since this was not tested, only limited review follows. Please test your
future patches.

> ---
>  .../bindings/mmc/nuvoton,ma35d1-sdhci.yaml    | 106 ++++++++++++++++++
>  1 file changed, 106 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mmc/nuvoton,ma35d1-sdhci.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mmc/nuvoton,ma35d1-sdhci.yaml b/Documentation/devicetree/bindings/mmc/nuvoton,ma35d1-sdhci.yaml
> new file mode 100644
> index 000000000000..173449360dea
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/nuvoton,ma35d1-sdhci.yaml
> @@ -0,0 +1,106 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mmc/nuvoton,ma35d1-sdhci.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Nuvoton MA35D1 SD/SDIO/MMC Controller
> +
> +maintainers:
> +  - Shan-Chun Hung <shanchun1218@gmail.com>
> +
> +description: |

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

> +  This controller on Nuvoton MA35 family SoCs provides an interface for MMC, SD, and
> +  SDIO types of memory cards.
> +
> +properties:
> +  compatible:
> +    oneOf:

Drop

> +      - enum:
> +          - nuvoton,ma35d1-sdhci

Blank line

> +  reg:
> +    maxItems: 1
> +    description: The SDHCI registers

Drop

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  pinctrl-names:
> +    description:
> +      Should at least contain default and state_uhs.

? Contradicts constraints.

> +    minItems: 1
> +    items:
> +      - const: default
> +      - const: state_uhs
> +
> +  pinctrl-0:
> +    description:
> +      Should contain default/high speed pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-1:
> +    description:
> +      Should contain uhs mode pin ctrl.
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 1

No, maxItems instead.

> +    description: The SDHCI bus clock

Drop

> +
> +  resets:
> +    maxItems: 1
> +    description:
> +      Phandle and reset specifier pair to softreset line of sdhci.

Drop

> +
> +  nuvoton,sys:

1. too generic, what is sys?

> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Phandle to the syscon that configure sdhci votage stable

2. typo: voltage

3. Which syscon?

4. Why you are not implementing regulators?


> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - pinctrl-names
> +  - pinctrl-0
> +
> +unevaluatedProperties: false

Hm? And where is ref to MMC schema?

> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/nuvoton,ma35d1-clk.h>
> +    #include <dt-bindings/reset/nuvoton,ma35d1-reset.h>
> +
> +    soc {
> +	#address-cells = <2>;
> +	#size-cells = <2>;

Fix your indentation.

Use 4 spaces for example indentation.

> +        sdhci0: sdhci@40180000 {

Drop label

> +            compatible = "nuvoton,ma35d1-sdhci";
> +            reg = <0x0 0x40180000 0x0 0x2000>;
> +            interrupts = <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>;
> +            clocks = <&clk SDH0_GATE>;
> +            pinctrl-names = "default";
> +            pinctrl-0 = <&pinctrl_sdhci0>;
> +            bus-width = <4>;
> +            max-frequency = <100000000>;
> +            no-1-8-v;
> +            status = "disabled";

Drop

> +        };
> +
> +        sdhci1: sdhci@40190000 {

Drop this example. One is enough.



Best regards,
Krzysztof