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

Shan-Chun Hung posted 2 patches 1 year, 7 months ago
[PATCH 1/2] dt-bindings: mmc: nuvoton,ma35d1-sdhci: Document MA35D1 SDHCI controller
Posted by Shan-Chun Hung 1 year, 7 months ago
Add binding for Nuvoton MA35D1 SDHCI controller.

Signed-off-by: schung <schung@nuvoton.com>
---
 .../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: |
+  This controller on Nuvoton MA35 family SoCs provides an interface for MMC, SD, and
+  SDIO types of memory cards.
+
+properties:
+  compatible:
+    oneOf:
+      - enum:
+          - nuvoton,ma35d1-sdhci
+  reg:
+    maxItems: 1
+    description: The SDHCI registers
+
+  interrupts:
+    maxItems: 1
+
+  pinctrl-names:
+    description:
+      Should at least contain default and state_uhs.
+    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
+    description: The SDHCI bus clock
+
+  resets:
+    maxItems: 1
+    description:
+      Phandle and reset specifier pair to softreset line of sdhci.
+
+  nuvoton,sys:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      Phandle to the syscon that configure sdhci votage stable
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - pinctrl-names
+  - pinctrl-0
+
+unevaluatedProperties: false
+
+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>;
+        sdhci0: sdhci@40180000 {
+            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";
+        };
+
+        sdhci1: sdhci@40190000 {
+            compatible = "nuvoton,ma35d1-sdhci";
+            reg = <0x0 0x40190000 0x0 0x2000>;
+            interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
+            clocks = <&clk SDH1_GATE>;
+            pinctrl-names = "default", "state_uhs";
+            pinctrl-0 = <&pinctrl_sdhci1>;
+            pinctrl-1 = <&pinctrl_sdhci1_uhs>;
+            resets = <&sys MA35D1_RESET_SDH1>;
+            nuvoton,sys = <&sys>;
+            bus-width = <8>;
+            max-frequency = <200000000>;
+            status = "disabled";
+        };
+    };
--
2.25.1
Re: [PATCH 1/2] dt-bindings: mmc: nuvoton,ma35d1-sdhci: Document MA35D1 SDHCI controller
Posted by Krzysztof Kozlowski 1 year, 7 months ago
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
Re: [PATCH 1/2] dt-bindings: mmc: nuvoton,ma35d1-sdhci: Document MA35D1 SDHCI controller
Posted by Shan-Chun Hung 1 year, 7 months ago
Dear Krzysztof,

Thanks for you review

On 2024/6/19 下午 03:29, Krzysztof Kozlowski wrote:
> 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.
I will remove '|'
>> +  This controller on Nuvoton MA35 family SoCs provides an interface for MMC, SD, and
>> +  SDIO types of memory cards.
>> +
>> +properties:
>> +  compatible:
>> +    oneOf:
> Drop

I will remove oneof.

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

I will fix it.

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

I will remove description.

>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  pinctrl-names:
>> +    description:
>> +      Should at least contain default and state_uhs.
> ? Contradicts constraints.
I will modify the description to "Should at least contain default. 
state_uhs is mandatory in this scenario."
>> +    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.

I will fix it.

>> +    description: The SDHCI bus clock
> Drop

I will remove it.

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

I will remove it.

>> +
>> +  nuvoton,sys:
> 1. too generic, what is sys?
>
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description:
>> +      Phandle to the syscon that configure sdhci votage stable

I will modify description as follows:
   nuvoton,sys:
     $ref: /schemas/types.yaml#/definitions/phandle
     description: phandle to access GCR (Global Control Register) registers.

>> 2. typo: voltage
I will fix it.
>> 3. Which syscon?
same as 1.
>> 4. Why you are not implementing regulators?
>>
I will add "vqmmc-supply = <&sdhci1_vqmmc_regulator>;" in the examples.

This requlator is used by regulator-gpio driver.

>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - clocks
>> +  - pinctrl-names
>> +  - pinctrl-0
>> +
>> +unevaluatedProperties: false
> Hm? And where is ref to MMC schema?

I will add as follows:

allOf:
   - $ref: sdhci-common.yaml#

>> +
>> +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.
I will fix it.
>> +        sdhci0: sdhci@40180000 {
> Drop label
I will fix it.
>> +            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
I will fix it.
>> +        };
>> +
>> +        sdhci1: sdhci@40190000 {
> Drop this example. One is enough.
>
>
>
> Best regards,
> Krzysztof
OK , I will remove one.

Best Regards

Shan-Chun

Re: [PATCH 1/2] dt-bindings: mmc: nuvoton,ma35d1-sdhci: Document MA35D1 SDHCI controller
Posted by Krzysztof Kozlowski 1 year, 7 months ago
On 21/06/2024 01:53, Shan-Chun Hung wrote:
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +  pinctrl-names:
>>> +    description:
>>> +      Should at least contain default and state_uhs.
>> ? Contradicts constraints.
> I will modify the description to "Should at least contain default. 
> state_uhs is mandatory in this scenario."

Then just drop it. Don't repeat constraints in free form text.


Best regards,
Krzysztof
Re: [PATCH 1/2] dt-bindings: mmc: nuvoton,ma35d1-sdhci: Document MA35D1 SDHCI controller
Posted by Shan-Chun Hung 1 year, 7 months ago
Dear Krzysztof,

Thanks for your review.

On 2024/6/21 下午 02:04, Krzysztof Kozlowski wrote:
> On 21/06/2024 01:53, Shan-Chun Hung wrote:
>>>> +
>>>> +  interrupts:
>>>> +    maxItems: 1
>>>> +
>>>> +  pinctrl-names:
>>>> +    description:
>>>> +      Should at least contain default and state_uhs.
>>> ? Contradicts constraints.
>> I will modify the description to "Should at least contain default.
>> state_uhs is mandatory in this scenario."
> Then just drop it. Don't repeat constraints in free form text.
>
>
> Best regards,
> Krzysztof
OK , I will drop the description.

Best Regards,

Shan-Chun

Re: [PATCH 1/2] dt-bindings: mmc: nuvoton,ma35d1-sdhci: Document MA35D1 SDHCI controller
Posted by Rob Herring (Arm) 1 year, 7 months ago
On Wed, 19 Jun 2024 13:46:40 +0800, Shan-Chun Hung wrote:
> Add binding for Nuvoton MA35D1 SDHCI controller.
> 
> Signed-off-by: schung <schung@nuvoton.com>
> ---
>  .../bindings/mmc/nuvoton,ma35d1-sdhci.yaml    | 106 ++++++++++++++++++
>  1 file changed, 106 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mmc/nuvoton,ma35d1-sdhci.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/mmc/nuvoton,ma35d1-sdhci.yaml:77:1: [error] syntax error: found character '\t' that cannot start any token (syntax)

dtschema/dtc warnings/errors:
make[2]: *** Deleting file 'Documentation/devicetree/bindings/mmc/nuvoton,ma35d1-sdhci.example.dts'
Documentation/devicetree/bindings/mmc/nuvoton,ma35d1-sdhci.yaml:77:1: found a tab character where an indentation space is expected
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/mmc/nuvoton,ma35d1-sdhci.example.dts] Error 1
make[2]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/mmc/nuvoton,ma35d1-sdhci.yaml:77:1: found a tab character where an indentation space is expected
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mmc/nuvoton,ma35d1-sdhci.yaml: ignoring, error parsing file
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1430: dt_binding_check] Error 2
make: *** [Makefile:240: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240619054641.277062-2-shanchun1218@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.