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

Krzysztof Kozlowski
2023-11-02T08:53:54Z

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