| Message ID | 20231030062749.2840-5-jyanchou@realtek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add DesignWare Mobile mmc driver | expand |
On 30/10/2023 07:27, Jyan Chou wrote: > Document the device-tree bindings for Realtek SoCs mmc driver. > > Signed-off-by: Jyan Chou <jyanchou@realtek.com> > > --- > v3 -> v4: > - Rename compatible(add SoC-specific part) to be different from filename. What? I asked to be the same. > - 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. > > v0 -> v2: > - Add dt-bindings. > --- > --- > .../bindings/mmc/realtek-dw-mshc.yaml | 161 ++++++++++++++++++ > 1 file changed, 161 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mmc/realtek-dw-mshc.yaml Compatible should match filename. 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. > > diff --git a/Documentation/devicetree/bindings/mmc/realtek-dw-mshc.yaml b/Documentation/devicetree/bindings/mmc/realtek-dw-mshc.yaml > new file mode 100644 > index 000000000000..d238cf3b8b47 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mmc/realtek-dw-mshc.yaml > @@ -0,0 +1,161 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/mmc/realtek-dw-mshc.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. > + > +allOf: > + - $ref: synopsys-dw-mshc-common.yaml# > + > +maintainers: > + - Jyan Chou <jyanchou@realtek.com> > + > +properties: > + compatible: > + enum: > + - realtek,rtd1325-dw-cqe-emmc > + > + reg: > + maxItems: 2 > + description: > + Two regs are required, first reg specifies emmc base address, second reg > + specifies cqhci base register address. > + > + reg-names: > + maxItems: 2 > + items: > + - const: emmc > + - const: cqhci > + > + interrupts: > + maxItems: 1 > + > + cqe: > + maxItems: 1 > + description: > + Cqe should set to 1 while using command queue feature, and set to 0 while > + in legacy mode. Did you just ignore Rob's comment? Please point me where and how did you address it? > + > + clocks: > + minItems: 2 No, why? > + items: > + - description: bus interface unit clock > + - description: card interface unit clock > + - description: select the phase for vpclk0 in realtek chip specified > + - description: select the phase for vpclk1 in realtek chip specified > + > + clock-names: > + minItems: 2 No, why? > + items: > + - const: biu > + - const: ciu > + - const: vp0 > + - const: vp1 > + > + resets: > + maxItems: 1 > + > + reset-names: > + const: reset You ignored my feedback. NAK. > + > + 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: This wasn't even tested. Do not send untested, broken code. Please run `make dt_binding_check` (see Documentation/devicetree/bindings/writing-schema.rst for instructions). Maybe you need to update your dtschema and yamllint. > + - compatible > + - reg > + - reg-names > + - interrupts > + - clocks > + - clock-names > + - pinctrl-names > + > + unevaluatedProperties: false > + > + examples: > + - | > + emmc: mmc@12000 { > + compatible = "realtek,rtd1325-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"; > + clock-freq-min-max = <300000 400000000>; > + clock-frequency = <400000>; > + vmmc-supply = <®_vcc1v8>; > + resets = <&rst 20>; > + reset-names = "reset"; > + speed-step = <3>; > + cqe = <1>; > + pinctrl-names = "default", "sdr50", "ddr50", "hs200", "hs400", > + "tune0","tune1", "tune2","tune3", "tune4"; > + pinctrl-0 = <&emmc_pins_sdr50>; > + pinctrl-1 = <&emmc_pins_sdr50>; > + pinctrl-2 = <&emmc_pins_ddr50>; > + pinctrl-3 = <&emmc_pins_hs200>; > + pinctrl-4 = <&emmc_pins_hs400>; > + pinctrl-5 = <&emmc_pins_tune0>; > + pinctrl-6 = <&emmc_pins_tune1>; > + pinctrl-7 = <&emmc_pins_tune2>; > + pinctrl-8 = <&emmc_pins_tune3>; > + pinctrl-9 = <&emmc_pins_tune4>; > + }; Broken indentation. Best regards, Krzysztof
On Mon, 30 Oct 2023 14:27:49 +0800, Jyan Chou wrote: > Document the device-tree bindings for Realtek SoCs mmc driver. > > Signed-off-by: Jyan Chou <jyanchou@realtek.com> > > --- > v3 -> v4: > - Rename compatible(add SoC-specific part) to be different from filename. > - 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. > > v0 -> v2: > - Add dt-bindings. > --- > --- > .../bindings/mmc/realtek-dw-mshc.yaml | 161 ++++++++++++++++++ > 1 file changed, 161 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mmc/realtek-dw-mshc.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mmc/realtek-dw-mshc.yaml: properties:required: ['compatible', 'reg', 'reg-names', 'interrupts', 'clocks', 'clock-names', 'pinctrl-names'] is not of type 'object', 'boolean' from schema $id: http://json-schema.org/draft-07/schema# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mmc/realtek-dw-mshc.yaml: properties:examples: ['emmc: mmc@12000 {\n compatible = "realtek,rtd1325-dw-cqe-emmc";\n reg = <0x00012000 0x00600>,\n <0x00012180 0x00060>;\n reg-names = "emmc", "cqhci";\n interrupts = <0 42 4>;\n clocks = <&cc 22>, <&cc 26>, <&cc 121>, <&cc 122>;\n clock-names = "biu", "ciu", "vp0", "vp1";\n clock-freq-min-max = <300000 400000000>;\n clock-frequency = <400000>;\n vmmc-supply = <®_vcc1v8>;\n resets = <&rst 20>;\n reset-names = "reset";\n speed-step = <3>;\n cqe = <1>;\n pinctrl-names = "default", "sdr50", "ddr50", "hs200", "hs400",\n "tune0","tune1", "tune2","tune3", "tune4";\n pinctrl-0 = <&emmc_pins_sdr50>;\n pinctrl-1 = <&emmc_pins_sdr50>;\n pinctrl-2 = <&emmc_pins_ddr50>;\n pinctrl-3 = <&emmc_pins_hs200>;\n pinctrl-4 = <&emmc_pins_hs400>;\n pinctrl-5 = <&emmc_pins_tune0>;\n pinctrl-6 = <&emmc_pins_tune1>;\n pinctrl-7 = <&emmc_pins_tu ne2>;\n pinctrl-8 = <&emmc_pins_tune3>;\n pinctrl-9 = <&emmc_pins_tune4>;\n };\n'] is not of type 'object', 'boolean' from schema $id: http://json-schema.org/draft-07/schema# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mmc/realtek-dw-mshc.yaml: properties: 'required' should not be valid under {'$ref': '#/definitions/json-schema-prop-names'} hint: A json-schema keyword was found instead of a DT property name. from schema $id: http://devicetree.org/meta-schemas/keywords.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mmc/realtek-dw-mshc.yaml: properties: 'unevaluatedProperties' should not be valid under {'$ref': '#/definitions/json-schema-prop-names'} hint: A json-schema keyword was found instead of a DT property name. from schema $id: http://devicetree.org/meta-schemas/keywords.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mmc/realtek-dw-mshc.yaml: properties:required: ['compatible', 'reg', 'reg-names', 'interrupts', 'clocks', 'clock-names', 'pinctrl-names'] is not of type 'object', 'boolean' from schema $id: http://devicetree.org/meta-schemas/keywords.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mmc/realtek-dw-mshc.yaml: properties:reg-names: {'maxItems': 2, 'items': [{'const': 'emmc'}, {'const': 'cqhci'}]} should not be valid under {'required': ['maxItems']} hint: "maxItems" is not needed with an "items" list from schema $id: http://devicetree.org/meta-schemas/items.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mmc/realtek-dw-mshc.yaml: properties:examples: ['emmc: mmc@12000 {\n compatible = "realtek,rtd1325-dw-cqe-emmc";\n reg = <0x00012000 0x00600>,\n <0x00012180 0x00060>;\n reg-names = "emmc", "cqhci";\n interrupts = <0 42 4>;\n clocks = <&cc 22>, <&cc 26>, <&cc 121>, <&cc 122>;\n clock-names = "biu", "ciu", "vp0", "vp1";\n clock-freq-min-max = <300000 400000000>;\n clock-frequency = <400000>;\n vmmc-supply = <®_vcc1v8>;\n resets = <&rst 20>;\n reset-names = "reset";\n speed-step = <3>;\n cqe = <1>;\n pinctrl-names = "default", "sdr50", "ddr50", "hs200", "hs400",\n "tune0","tune1", "tune2","tune3", "tune4";\n pinctrl-0 = <&emmc_pins_sdr50>;\n pinctrl-1 = <&emmc_pins_sdr50>;\n pinctrl-2 = <&emmc_pins_ddr50>;\n pinctrl-3 = <&emmc_pins_hs200>;\n pinctrl-4 = <&emmc_pins_hs400>;\n pinctrl-5 = <&emmc_pins_tune0>;\n pinctrl-6 = <&emmc_pins_tune1>;\n pinctrl-7 = <&emmc_pins_tu ne2>;\n pinctrl-8 = <&emmc_pins_tune3>;\n pinctrl-9 = <&emmc_pins_tune4>;\n };\n'] is not of type 'object', 'boolean' from schema $id: http://devicetree.org/meta-schemas/keywords.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mmc/realtek-dw-mshc.yaml: 'oneOf' conditional failed, one must be fixed: 'unevaluatedProperties' is a required property 'additionalProperties' is a required property hint: Either unevaluatedProperties or additionalProperties must be present from schema $id: http://devicetree.org/meta-schemas/core.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mmc/realtek-dw-mshc.yaml: cqe: missing type definition doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231030062749.2840-5-jyanchou@realtek.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
Hi Krzysztof, Rob, Thanks for your code review and advice. > What? I asked to be the same. We had modified it to be the same. > Compatible should match filename. compatible = "realtek,rtd-dw-cqe-emmc"; be the same as realtek,rtd-dw-cqe-emmc.yaml >> + cqe: >> + maxItems: 1 >> + description: >> + Cqe should set to 1 while using command queue feature, and set to 0 while >> + in legacy mode. > Did you just ignore Rob's comment? > Please point me where and how did you address it? Sorry for not fixing this error. We had removed cqe and used common properties related to CQE. >> + clocks: >> + minItems: 2 > No, why? >> + items: >> + - description: bus interface unit clock >> + - description: card interface unit clock >> + - description: select the phase for vpclk0 in realtek chip specified >> + - description: select the phase for vpclk1 in realtek chip >> + specified >> + >> + clock-names: >> + minItems: 2 > No, why? >> + items: >> + - const: biu >> + - const: ciu >> + - const: vp0 >> + - const: vp1 >> + >> + resets: >> + maxItems: 1 >> + >> + reset-names: >> + const: reset We had modified our wrong format of clocks and clock-names, also we removed useless resets properties. >> + - | >> + emmc: mmc@12000 { >> + compatible = "realtek,rtd1325-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"; >> + clock-freq-min-max = <300000 400000000>; >> + clock-frequency = <400000>; >> + vmmc-supply = <®_vcc1v8>; >> + resets = <&rst 20>; >> + reset-names = "reset"; >> + speed-step = <3>; >> + cqe = <1>; >> + pinctrl-names = "default", "sdr50", "ddr50", "hs200", "hs400", >> + "tune0","tune1", "tune2","tune3", "tune4"; >> + pinctrl-0 = <&emmc_pins_sdr50>; >> + pinctrl-1 = <&emmc_pins_sdr50>; >> + pinctrl-2 = <&emmc_pins_ddr50>; >> + pinctrl-3 = <&emmc_pins_hs200>; >> + pinctrl-4 = <&emmc_pins_hs400>; >> + pinctrl-5 = <&emmc_pins_tune0>; >> + pinctrl-6 = <&emmc_pins_tune1>; >> + pinctrl-7 = <&emmc_pins_tune2>; >> + pinctrl-8 = <&emmc_pins_tune3>; >> + pinctrl-9 = <&emmc_pins_tune4>; >> + }; > Broken indentation. We had corrected the wrong indentation and tested it with make dt_binding_check. Best Regards, Jyan -----Original Message----- From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Sent: Monday, October 30, 2023 3:47 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; p.zabel@pengutronix.de Cc: 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 V4][4/4] dt-bindings: mmc: Add dt-bindings for realtek mmc driver External mail. On 30/10/2023 07:27, Jyan Chou wrote: > Document the device-tree bindings for Realtek SoCs mmc driver. > > Signed-off-by: Jyan Chou <jyanchou@realtek.com> > > --- > v3 -> v4: > - Rename compatible(add SoC-specific part) to be different from filename. What? I asked to be the same. > - 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. > > v0 -> v2: > - Add dt-bindings. > --- > --- > .../bindings/mmc/realtek-dw-mshc.yaml | 161 ++++++++++++++++++ > 1 file changed, 161 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/mmc/realtek-dw-mshc.yaml Compatible should match filename. 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. > > diff --git > a/Documentation/devicetree/bindings/mmc/realtek-dw-mshc.yaml > b/Documentation/devicetree/bindings/mmc/realtek-dw-mshc.yaml > new file mode 100644 > index 000000000000..d238cf3b8b47 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mmc/realtek-dw-mshc.yaml > @@ -0,0 +1,161 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/mmc/realtek-dw-mshc.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. > + > +allOf: > + - $ref: synopsys-dw-mshc-common.yaml# > + > +maintainers: > + - Jyan Chou <jyanchou@realtek.com> > + > +properties: > + compatible: > + enum: > + - realtek,rtd1325-dw-cqe-emmc > + > + reg: > + maxItems: 2 > + description: > + Two regs are required, first reg specifies emmc base address, second reg > + specifies cqhci base register address. > + > + reg-names: > + maxItems: 2 > + items: > + - const: emmc > + - const: cqhci > + > + interrupts: > + maxItems: 1 > + > + cqe: > + maxItems: 1 > + description: > + Cqe should set to 1 while using command queue feature, and set to 0 while > + in legacy mode. Did you just ignore Rob's comment? Please point me where and how did you address it? > + > + clocks: > + minItems: 2 No, why? > + items: > + - description: bus interface unit clock > + - description: card interface unit clock > + - description: select the phase for vpclk0 in realtek chip specified > + - description: select the phase for vpclk1 in realtek chip > + specified > + > + clock-names: > + minItems: 2 No, why? > + items: > + - const: biu > + - const: ciu > + - const: vp0 > + - const: vp1 > + > + resets: > + maxItems: 1 > + > + reset-names: > + const: reset You ignored my feedback. NAK. > + > + 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: This wasn't even tested. Do not send untested, broken code. Please run `make dt_binding_check` (see Documentation/devicetree/bindings/writing-schema.rst for instructions). Maybe you need to update your dtschema and yamllint. > + - compatible > + - reg > + - reg-names > + - interrupts > + - clocks > + - clock-names > + - pinctrl-names > + > + unevaluatedProperties: false > + > + examples: > + - | > + emmc: mmc@12000 { > + compatible = "realtek,rtd1325-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"; > + clock-freq-min-max = <300000 400000000>; > + clock-frequency = <400000>; > + vmmc-supply = <®_vcc1v8>; > + resets = <&rst 20>; > + reset-names = "reset"; > + speed-step = <3>; > + cqe = <1>; > + pinctrl-names = "default", "sdr50", "ddr50", "hs200", "hs400", > + "tune0","tune1", "tune2","tune3", "tune4"; > + pinctrl-0 = <&emmc_pins_sdr50>; > + pinctrl-1 = <&emmc_pins_sdr50>; > + pinctrl-2 = <&emmc_pins_ddr50>; > + pinctrl-3 = <&emmc_pins_hs200>; > + pinctrl-4 = <&emmc_pins_hs400>; > + pinctrl-5 = <&emmc_pins_tune0>; > + pinctrl-6 = <&emmc_pins_tune1>; > + pinctrl-7 = <&emmc_pins_tune2>; > + pinctrl-8 = <&emmc_pins_tune3>; > + pinctrl-9 = <&emmc_pins_tune4>; > + }; Broken indentation. Best regards, Krzysztof
On 02/11/2023 08:09, Jyan Chou [周芷安] wrote: > Hi Krzysztof, Rob, > > Thanks for your code review and advice. > >> What? I asked to be the same. > Respond inline, not by copying some pieces out of context to another part of email. That's not mailing list style response. Best regards, Krzysztof
>> Hi Krzysztof, Rob, >> >> Thanks for your code review and advice. >> >>> What? I asked to be the same. > Respond inline, not by copying some pieces out of context to another part of email. That's not mailing list style response. Thanks for your remind. I will resend changelog. >>> Document the device-tree bindings for Realtek SoCs mmc driver. >>> >>> Signed-off-by: Jyan Chou <jyanchou@realtek.com> >>> >> --- >> v3 -> v4: >> - Rename compatible(add SoC-specific part) to be different from filename. > What? I asked to be the same. We had modified it to be the same. >> - 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. >> >> v0 -> v2: >> - Add dt-bindings. >> --- >> --- >> .../bindings/mmc/realtek-dw-mshc.yaml | 161 ++++++++++++++++++ >> 1 file changed, 161 insertions(+) >> create mode 100644 >> Documentation/devicetree/bindings/mmc/realtek-dw-mshc.yaml > Compatible should match filename. compatible = "realtek,rtd-dw-cqe-emmc"; had changes to be the same as realtek,rtd-dw-cqe-emmc.yaml > 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. Sorry for not to fully addressed previous comments, I will corrected in this version. >> >> diff --git >> a/Documentation/devicetree/bindings/mmc/realtek-dw-mshc.yaml >> b/Documentation/devicetree/bindings/mmc/realtek-dw-mshc.yaml >> new file mode 100644 >> index 000000000000..d238cf3b8b47 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/mmc/realtek-dw-mshc.yaml >> @@ -0,0 +1,161 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/mmc/realtek-dw-mshc.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. >> + >> +allOf: >> + - $ref: synopsys-dw-mshc-common.yaml# >> + >> +maintainers: >> + - Jyan Chou <jyanchou@realtek.com> >> + >> +properties: >> + compatible: >> + enum: >> + - realtek,rtd1325-dw-cqe-emmc >> + >> + reg: >> + maxItems: 2 >> + description: >> + Two regs are required, first reg specifies emmc base address, second reg >> + specifies cqhci base register address. >> + >> + reg-names: >> + maxItems: 2 >> + items: >> + - const: emmc >> + - const: cqhci >> + >> + interrupts: >> + maxItems: 1 >> + >> + cqe: >> + maxItems: 1 >> + description: >> + Cqe should set to 1 while using command queue feature, and set to 0 while >> + in legacy mode. > Did you just ignore Rob's comment? > Please point me where and how did you address it? Sorry for not fixing this error. We had removed cqe and used common properties related to CQE. >> + >> + clocks: >> + minItems: 2 > No, why? We had the wrong use of this and had corrected it. >> + items: >> + - description: bus interface unit clock >> + - description: card interface unit clock >> + - description: select the phase for vpclk0 in realtek chip specified >> + - description: select the phase for vpclk1 in realtek chip >> + specified >> + >> + clock-names: >> + minItems: 2 > No, why? We had the wrong use of this and had corrected it. >> + items: >> + - const: biu >> + - const: ciu >> + - const: vp0 >> + - const: vp1 >> + >> + resets: >> + maxItems: 1 >> + >> + reset-names: >> + const: reset > You ignored my feedback. NAK. Sorry. We had dropped 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: > This wasn't even tested. > Do not send untested, broken code. Please run `make dt_binding_check` (see Documentation/devicetree/bindings/writing-schema.rst for instructions). Maybe you need to update your dtschema and yamllint. We had tested it with make dt_binding_check to ensure it is not a broken code. >> + - compatible >> + - reg >> + - reg-names >> + - interrupts >> + - clocks >> + - clock-names >> + - pinctrl-names >> + >> + unevaluatedProperties: false >> + >> + examples: >> + - | >> + emmc: mmc@12000 { >> + compatible = "realtek,rtd1325-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"; >> + clock-freq-min-max = <300000 400000000>; >> + clock-frequency = <400000>; >> + vmmc-supply = <®_vcc1v8>; >> + resets = <&rst 20>; >> + reset-names = "reset"; >> + speed-step = <3>; >> + cqe = <1>; >> + pinctrl-names = "default", "sdr50", "ddr50", "hs200", "hs400", >> + "tune0","tune1", "tune2","tune3", "tune4"; >> + pinctrl-0 = <&emmc_pins_sdr50>; >> + pinctrl-1 = <&emmc_pins_sdr50>; >> + pinctrl-2 = <&emmc_pins_ddr50>; >> + pinctrl-3 = <&emmc_pins_hs200>; >> + pinctrl-4 = <&emmc_pins_hs400>; >> + pinctrl-5 = <&emmc_pins_tune0>; >> + pinctrl-6 = <&emmc_pins_tune1>; >> + pinctrl-7 = <&emmc_pins_tune2>; >> + pinctrl-8 = <&emmc_pins_tune3>; >> + pinctrl-9 = <&emmc_pins_tune4>; >> + }; > Broken indentation. We had corrected the wrong indentation in our new version. Best regards, Jyan -----Original Message----- From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Sent: Thursday, November 2, 2023 3:14 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; p.zabel@pengutronix.de Cc: 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 V4][4/4] dt-bindings: mmc: Add dt-bindings for realtek mmc driver External mail. On 02/11/2023 08:09, Jyan Chou [周芷安] wrote: > Hi Krzysztof, Rob, > > Thanks for your code review and advice. > >> What? I asked to be the same. > Respond inline, not by copying some pieces out of context to another part of email. That's not mailing list style response. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/mmc/realtek-dw-mshc.yaml b/Documentation/devicetree/bindings/mmc/realtek-dw-mshc.yaml new file mode 100644 index 000000000000..d238cf3b8b47 --- /dev/null +++ b/Documentation/devicetree/bindings/mmc/realtek-dw-mshc.yaml @@ -0,0 +1,161 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/mmc/realtek-dw-mshc.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. + +allOf: + - $ref: synopsys-dw-mshc-common.yaml# + +maintainers: + - Jyan Chou <jyanchou@realtek.com> + +properties: + compatible: + enum: + - realtek,rtd1325-dw-cqe-emmc + + reg: + maxItems: 2 + description: + Two regs are required, first reg specifies emmc base address, second reg + specifies cqhci base register address. + + reg-names: + maxItems: 2 + items: + - const: emmc + - const: cqhci + + interrupts: + maxItems: 1 + + cqe: + maxItems: 1 + description: + Cqe should set to 1 while using command queue feature, and set to 0 while + in legacy mode. + + clocks: + minItems: 2 + items: + - description: bus interface unit clock + - description: card interface unit clock + - description: select the phase for vpclk0 in realtek chip specified + - description: select the phase for vpclk1 in realtek chip specified + + clock-names: + minItems: 2 + items: + - const: biu + - const: ciu + - const: vp0 + - const: vp1 + + resets: + maxItems: 1 + + reset-names: + const: reset + + 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 + - pinctrl-names + + unevaluatedProperties: false + + examples: + - | + emmc: mmc@12000 { + compatible = "realtek,rtd1325-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"; + clock-freq-min-max = <300000 400000000>; + clock-frequency = <400000>; + vmmc-supply = <®_vcc1v8>; + resets = <&rst 20>; + reset-names = "reset"; + speed-step = <3>; + cqe = <1>; + pinctrl-names = "default", "sdr50", "ddr50", "hs200", "hs400", + "tune0","tune1", "tune2","tune3", "tune4"; + pinctrl-0 = <&emmc_pins_sdr50>; + pinctrl-1 = <&emmc_pins_sdr50>; + pinctrl-2 = <&emmc_pins_ddr50>; + pinctrl-3 = <&emmc_pins_hs200>; + pinctrl-4 = <&emmc_pins_hs400>; + pinctrl-5 = <&emmc_pins_tune0>; + pinctrl-6 = <&emmc_pins_tune1>; + pinctrl-7 = <&emmc_pins_tune2>; + pinctrl-8 = <&emmc_pins_tune3>; + pinctrl-9 = <&emmc_pins_tune4>; + };
Document the device-tree bindings for Realtek SoCs mmc driver. Signed-off-by: Jyan Chou <jyanchou@realtek.com> --- v3 -> v4: - Rename compatible(add SoC-specific part) to be different from filename. - 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. v0 -> v2: - Add dt-bindings. --- --- .../bindings/mmc/realtek-dw-mshc.yaml | 161 ++++++++++++++++++ 1 file changed, 161 insertions(+) create mode 100644 Documentation/devicetree/bindings/mmc/realtek-dw-mshc.yaml