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

[Shan-Chun Hung <shanchun1218@gmail.com>]

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