Message-ID: <6de77300-fbbf-43ed-b24b-304e27d4c662@linaro.org>
Date:   Thu, 2 Nov 2023 09:53:38 +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
Cc:     p.zabel@...gutronix.de, 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 V5][4/4] dt-bindings: mmc: Add dt-bindings for realtek mmc
 driver

On 02/11/2023 09:15, Jyan Chou wrote:
> Document the device-tree bindings for Realtek SoCs mmc driver.
> 
> Signed-off-by: Jyan Chou <jyanchou@...ltek.com>
> 
> ---
> v4 -> v5:
> - Fix compatible to match filename.

That's not what I said. Filename must match compatible, not the other
way around.

> - Remove unused property, e.g.,cqe, resets, clock-freq-min-max.
> - Fix indentation.
> 
> v3 -> v4:
> - 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.
> 
> v1 -> v2:
> - Add dt-bindings.
> ---
>  .../bindings/mmc/realtek,rtd-dw-cqe-emmc.yaml | 157 ++++++++++++++++++
>  1 file changed, 157 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mmc/realtek,rtd-dw-cqe-emmc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mmc/realtek,rtd-dw-cqe-emmc.yaml b/Documentation/devicetree/bindings/mmc/realtek,rtd-dw-cqe-emmc.yaml
> new file mode 100644
> index 000000000000..f422a216ff93
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/realtek,rtd-dw-cqe-emmc.yaml
> @@ -0,0 +1,157 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mmc/realtek,rtd-dw-cqe-emmc.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.
> +
> +maintainers:
> +  - Jyan Chou <jyanchou@...ltek.com>
> +
> +allOf:
> +  - $ref: mmc-controller.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - realtek,rtd-dw-cqe-emmc

I don't understand what happened here. I asked you to drop the
incorrect, generic compatible. Instead you dropped specific compatibles
and left generic. Nope, this does not work like it.

You *must* use specific compatibles.

> +
> +  reg:
> +    items:
> +      - description: emmc base address
> +      - description: cqhci base address
> +
> +  reg-names:
> +    items:
> +      - const: emmc
> +      - const: cqhci
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    description: Handles to input clocks

Instead: maxItems: 4

> +
> +  clock-names:
> +    items:
> +      - const: biu
> +      - const: ciu
> +      - const: vp0
> +      - const: vp1
> +
> +  clock-frequency:
> +    description:
> +      Operating frequency of realtek emmc controller clock

Drop entire property. There is already max-frequency.

> +    minimum: 300000
> +    maximum: 400000000
> +
> +  vmmc-supply:
> +    description:
> +      Handle to fixed-voltage supply for the card power.

Drop entire property. Not needed.

> +
> +  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:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - interrupts
> +  - clocks
> +  - clock-names
> +  - clock-frequency
> +  - vmmc-supply
> +  - pinctrl-names
> +  - pinctrl-0
> +  - pinctrl-1
> +  - pinctrl-3
> +  - pinctrl-4
> +  - pinctrl-5
> +  - pinctrl-6
> +  - pinctrl-7
> +  - pinctrl-8
> +  - pinctrl-9
> +
> +additionalProperties: false

unevaluatedProperties: false

> +
> +examples:
> +  - |
> +    emmc: mmc@...00 {
> +      compatible = "realtek,rtd-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";

I asked you to test the bindings. This also means that you must test
your DTS against bindings. Your bindings, DTS and driver do not match,
therefore let's be a bit more clear:

NAK, till you upstream your DTS.

Best regards,
Krzysztof

Powered by blists - more mailing lists