RE: [PATCH V5][4/4] dt-bindings: mmc: Add dt-bindings for realtek mmc driver

["Jyan Chou [周芷安]" <jyanchou@realtek.com>]

>> v4 -> v5:
>> - Fix compatible to match filename.

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

Sorry for misunderstand your advice. We name our filename realtek,rtd-dw-cqe-emmc.yaml and correct our compatible to 
  compatible:
    enum:
      - realtek,rtd1325-dw-cqe-emmc
      - realtek,rtd1319-dw-cqe-emmc
      - realtek,rtd1315e-dw-cqe-emmc
      - realtek,rtd1619b-dw-cqe-emmc
in new version.

>> - 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.ya
>> +++ ml
>> @@ -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@realtek.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.

We change our compatible to be like realtek,rtd1325-dw-cqe-emmc.

>> +
>> +  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

Okay. I will add 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.

We will drop it in our new version.

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

> Drop entire property. Not needed.

We will drop it. 

>> +
>> +  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

I will replace additionalProperties: false with unevaluatedProperties: false.

>> +
>> +examples:
>> +  - |
>> +    emmc: mmc@12000 {
>> +      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.

Okay, I will test it and push a correct one again. Thanks.

-----Original Message-----
From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 
Sent: Thursday, November 2, 2023 4:54 PM
To: Jyan Chou [周芷安] <jyanchou@realtek.com>; ulf.hansson@linaro.org; adrian.hunter@intel.com; jh80.chung@samsung.com; riteshh@codeaurora.org; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; asutoshd@codeaurora.org
Cc: p.zabel@pengutronix.de; linux-mmc@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; arnd@arndb.de; briannorris@chromium.org; doug@schmorgal.com; tonyhuang.sunplus@gmail.com; abel.vesa@linaro.org; william.qiu@starfivetech.com
Subject: Re: [PATCH V5][4/4] dt-bindings: mmc: Add dt-bindings for realtek mmc driver


External mail.



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@realtek.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.ya
> +++ ml
> @@ -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@realtek.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@12000 {
> +      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