>> 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@xxxxxxxxxxx>
>> +
>> +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@xxxxxxxxxx>
Sent: Thursday, November 2, 2023 4:54 PM
To: Jyan Chou [周芷安] <jyanchou@xxxxxxxxxxx>; ulf.hansson@xxxxxxxxxx; adrian.hunter@xxxxxxxxx; jh80.chung@xxxxxxxxxxx; riteshh@xxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx; asutoshd@xxxxxxxxxxxxxx
Cc: p.zabel@xxxxxxxxxxxxxx; linux-mmc@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; arnd@xxxxxxxx; briannorris@xxxxxxxxxxxx; doug@xxxxxxxxxxxxx; tonyhuang.sunplus@xxxxxxxxx; abel.vesa@xxxxxxxxxx; william.qiu@xxxxxxxxxxxxxxxx
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@xxxxxxxxxxx>
>
> ---
> 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@xxxxxxxxxxx>
> +
> +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
