[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <42ec842e-84c0-40fd-b6fc-e59d25ede89f@gmail.com>
Date: Fri, 21 Jun 2024 07:53:14 +0800
From: Shan-Chun Hung <shanchun1218@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>, ulf.hansson@...aro.org,
robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
adrian.hunter@...el.com, p.zabel@...gutronix.de, pbrobinson@...il.com,
serghox@...il.com, mcgrof@...nel.org,
prabhakar.mahadev-lad.rj@...renesas.com, forbidden405@...look.com,
tmaimon77@...il.com, andy.shevchenko@...il.com,
linux-arm-kernel@...ts.infradead.org, linux-mmc@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Cc: ychuang3@...oton.com, schung@...oton.com
Subject: Re: [PATCH 1/2] dt-bindings: mmc: nuvoton,ma35d1-sdhci: Document
MA35D1 SDHCI controller
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@...oton.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@...il.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@...80000 {
> 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@...90000 {
> Drop this example. One is enough.
>
>
>
> Best regards,
> Krzysztof
OK , I will remove one.
Best Regards
Shan-Chun
Powered by blists - more mailing lists