Message-ID: <b116d46f-f28b-4b7c-afd2-0061b523b00d@linaro.org>
Date:   Mon, 30 Oct 2023 08:47:15 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Jyan Chou <jyanchou@...ltek.com>, ulf.hansson@...aro.org,
        adrian.hunter@...el.com, jh80.chung@...sung.com,
        riteshh@...eaurora.org, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
        asutoshd@...eaurora.org, p.zabel@...gutronix.de
Cc:     linux-mmc@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, arnd@...db.de,
        briannorris@...omium.org, doug@...morgal.com,
        tonyhuang.sunplus@...il.com, abel.vesa@...aro.org,
        william.qiu@...rfivetech.com
Subject: Re: [PATCH V4][4/4] dt-bindings: mmc: Add dt-bindings for realtek mmc
 driver

On 30/10/2023 07:27, Jyan Chou wrote:
> Document the device-tree bindings for Realtek SoCs mmc driver.
> 
> Signed-off-by: Jyan Chou <jyanchou@...ltek.com>
> 
> ---
> v3 -> v4:
> - Rename compatible(add SoC-specific part) to be different from filename.

What? I asked to be the same.

> - Describe the items to make properties and item easy to understand.
> - Fix examples' indentation and compiling error.
> - Drop useless properties.
> 
> v2 -> v3:
> - Modify dt-bindings' content and description.
> - Fix coding style.
> - Update the list of maintainers.
> 
> v0 -> v2:
> - Add dt-bindings.
> ---
> ---
>  .../bindings/mmc/realtek-dw-mshc.yaml         | 161 ++++++++++++++++++
>  1 file changed, 161 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mmc/realtek-dw-mshc.yaml

Compatible should match filename.

This is a friendly reminder during the review process.

It seems my or other reviewer's previous comments were not fully
addressed. Maybe the feedback got lost between the quotes, maybe you
just forgot to apply it. Please go back to the previous discussion and
either implement all requested changes or keep discussing them.

Thank you.


> 
> diff --git a/Documentation/devicetree/bindings/mmc/realtek-dw-mshc.yaml b/Documentation/devicetree/bindings/mmc/realtek-dw-mshc.yaml
> new file mode 100644
> index 000000000000..d238cf3b8b47
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/realtek-dw-mshc.yaml
> @@ -0,0 +1,161 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mmc/realtek-dw-mshc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Realtek DesignWare mobile storage host controller
> +
> +description:
> +  Realtek uses the Synopsys DesignWare mobile storage host controller
> +  to interface a SoC with storage medium. This file documents the Realtek
> +  specific extensions.
> +
> +allOf:
> +  - $ref: synopsys-dw-mshc-common.yaml#
> +
> +maintainers:
> +  - Jyan Chou <jyanchou@...ltek.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - realtek,rtd1325-dw-cqe-emmc
> +
> +  reg:
> +    maxItems: 2
> +    description:
> +      Two regs are required, first reg specifies emmc base address, second reg
> +      specifies cqhci base register address.
> +
> +  reg-names:
> +    maxItems: 2
> +    items:
> +      - const: emmc
> +      - const: cqhci
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  cqe:
> +    maxItems: 1
> +    description:
> +      Cqe should set to 1 while using command queue feature, and set to 0 while
> +      in legacy mode.

Did you just ignore Rob's comment?

Please point me where and how did you address it?

> +
> +  clocks:
> +    minItems: 2

No, why?

> +    items:
> +      - description: bus interface unit clock
> +      - description: card interface unit clock
> +      - description: select the phase for vpclk0 in realtek chip specified
> +      - description: select the phase for vpclk1 in realtek chip specified
> +
> +  clock-names:
> +    minItems: 2

No, why?

> +    items:
> +      - const: biu
> +      - const: ciu
> +      - const: vp0
> +      - const: vp1
> +
> +  resets:
> +    maxItems: 1
> +
> +  reset-names:
> +    const: reset

You ignored my feedback. NAK.

> +
> +  pinctrl-0:
> +    description:
> +      should contain default/high speed pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-1:
> +    description:
> +      should contain sdr50 mode pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-2:
> +    description:
> +      should contain ddr50 mode pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-3:
> +    description:
> +      should contain hs200 speed pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-4:
> +    description:
> +      should contain hs400 speed pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-5:
> +    description:
> +      should contain tune0 pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-6:
> +    description:
> +      should contain tune1 pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-7:
> +    description:
> +      should contain tune2 pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-8:
> +    description:
> +      should contain tune3 pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-9:
> +    description:
> +      should contain tune4 pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-names:
> +    maxItems: 10
> +
> +  required:

This wasn't even tested.

Do not send untested, broken code. Please run `make dt_binding_check`
(see Documentation/devicetree/bindings/writing-schema.rst for
instructions). Maybe you need to update your dtschema and yamllint.

> +    - compatible
> +    - reg
> +    - reg-names
> +    - interrupts
> +    - clocks
> +    - clock-names
> +    - pinctrl-names
> +
> +  unevaluatedProperties: false
> +
> +  examples:


> +    - |
> +      emmc: mmc@...00 {
> +        compatible = "realtek,rtd1325-dw-cqe-emmc";
> +        reg = <0x00012000 0x00600>,
> +              <0x00012180 0x00060>;
> +        reg-names = "emmc", "cqhci";
> +        interrupts = <0 42 4>;
> +        clocks = <&cc 22>, <&cc 26>, <&cc 121>, <&cc 122>;
> +        clock-names = "biu", "ciu", "vp0", "vp1";
> +        clock-freq-min-max = <300000 400000000>;
> +        clock-frequency = <400000>;
> +        vmmc-supply = <&reg_vcc1v8>;
> +        resets = <&rst 20>;
> +        reset-names = "reset";
> +        speed-step = <3>;
> +        cqe = <1>;
> +        pinctrl-names = "default", "sdr50", "ddr50", "hs200", "hs400",
> +                        "tune0","tune1", "tune2","tune3", "tune4";
> +        pinctrl-0 = <&emmc_pins_sdr50>;
> +        pinctrl-1 = <&emmc_pins_sdr50>;
> +        pinctrl-2 = <&emmc_pins_ddr50>;
> +        pinctrl-3 = <&emmc_pins_hs200>;
> +        pinctrl-4 = <&emmc_pins_hs400>;
> +        pinctrl-5 = <&emmc_pins_tune0>;
> +        pinctrl-6 = <&emmc_pins_tune1>;
> +        pinctrl-7 = <&emmc_pins_tune2>;
> +        pinctrl-8 = <&emmc_pins_tune3>;
> +        pinctrl-9 = <&emmc_pins_tune4>;
> +        };

Broken indentation.

Best regards,
Krzysztof

Powered by blists - more mailing lists