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

From: Jyan Chou [周芷安]
Date: Thu Nov 09 2023 - 02:29:33 EST

Next message: Conor Dooley: "Re: [PATCH v5 3/4] dt-bindings: clock: rk3588: export PCLK_VO1GRF clk id"
Previous message: Mi, Dapeng: "Re: [PATCH v7 09/19] KVM: selftests: Test Intel PMU architectural events on gp counters"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

Next message: Conor Dooley: "Re: [PATCH v5 3/4] dt-bindings: clock: rk3588: export PCLK_VO1GRF clk id"
Previous message: Mi, Dapeng: "Re: [PATCH v7 09/19] KVM: selftests: Test Intel PMU architectural events on gp counters"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]