linux-kernel - Re: [PATCH v7][4/4] dt-bindings: mmc: Add dt-bindings for realtek mmc driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 21 Nov 2023 10:35:02 +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
Cc:     conor+dt@...nel.org, asutoshd@...eaurora.org,
        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 v7][4/4] dt-bindings: mmc: Add dt-bindings for realtek mmc
 driver

On 21/11/2023 10:11, Jyan Chou wrote:
> Document the device-tree bindings for Realtek SoCs mmc driver.

This is a friendly reminder during the review process.

It seems my or other reviewer's previous comments were not fully
addressed. Maybe the feedback got lost between the quotes, maybe you
just forgot to apply it. Please go back to the previous discussion and
either implement all requested changes or keep discussing them.

Thank you.

Additionally:

A nit, subject: drop second/last, redundant "bindings for". The
"dt-bindings" prefix is already stating that these are bindings.

> 
> Signed-off-by: Jyan Chou <jyanchou@...ltek.com>
> 
> ---
> v6 -> v7:
> - Drop reset-names and realtek,m2tmx since it is not needed in our driver.
> 
> v5 -> v6:
> - Drop the incorrect, generic compatible and modify it to specific.
> - Drop useless properties.

I asked which ones. I still do not see that information here.


> - Modify bindings to make DTS and driver match.
> 
> v4 -> v5:
> - Remove unused property, e.g.,cqe, resets, clock-freq-min-max.
> - Fix indentation.
> 

...

> +
> +maintainers:
> +  - Jyan Chou <jyanchou@...ltek.com>
> +
> +allOf:
> +  - $ref: synopsys-dw-mshc-common.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - realtek,rtd1325-dw-cqe-emmc
> +      - realtek,rtd1319-dw-cqe-emmc
> +      - realtek,rtd1315e-dw-cqe-emmc
> +      - realtek,rtd1619b-dw-cqe-emmc

That's not what is present in your driver. Why other compatibles are not
there? What do you want to express here?

> +
> +  reg:
> +    items:
> +      - description: emmc base address
> +      - description: cqhci base address
> +
> +  reg-names:
> +    items:
> +      - const: emmc
> +      - const: cqhci
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 4
> +
> +  clock-names:
> +    items:
> +      - const: biu
> +      - const: ciu
> +      - const: vp0
> +      - const: vp1
> +
> +  resets:
> +    maxItems: 1
> +
> +  pinctrl-0:
> +    description:
> +      should contain default/high speed pin ctrl.
> +    maxItems: 1
> +
> +  pinctrl-1:
> +    description:
> +      should contain sdr50 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
> +  - vmmc-supply
> +  - pinctrl-names
> +  - pinctrl-0
> +  - pinctrl-1
> +  - pinctrl-3
> +  - pinctrl-4
> +  - pinctrl-5
> +  - pinctrl-6
> +  - pinctrl-7
> +  - pinctrl-8
> +  - pinctrl-9
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    emmc: mmc@...00 {
> +      compatible = "realtek,rtd1315e-dw-cqe-emmc";
> +      reg = <0x00012000 0x00600>,
> +            <0x00012180 0x00060>;
> +      reg-names = "emmc", "cqhci";
> +      interrupts = <0 42 4>;

Use proper interrupt defines instead of raw values.

> +      clocks = <&cc 22>, <&cc 26>, <&cc 121>, <&cc 122>;
> +      clock-names = "biu", "ciu", "vp0", "vp1";

Where is the DTS implementing this binding?

I said it once:

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

I still do not see DTS anywhere. Please link it.

Best regards,
Krzysztof