| Message ID | 20230606094535.5388-4-billy_tsai@aspeedtech.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Support pwm/tach driver for aspeed ast26xx | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dt-meta-schema | fail | build log |
On Tue, 06 Jun 2023 17:45:33 +0800, Billy Tsai wrote: > Add device binding for aspeed pwm-tach device which is a multi-function > device include pwm and tach function. > > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com> > > --- > .../bindings/mfd/aspeed,ast2600-pwm-tach.yaml | 76 +++++++++++++++++++ > 1 file changed, 76 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.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: Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.example.dts:37.15-28: Warning (reg_format): /example-0/pwm-tach@1e610000/tach/fan@0:reg: property has invalid length (4 bytes) (#address-cells == 2, #size-cells == 1) Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.example.dtb: Warning (pci_device_reg): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.example.dtb: Warning (pci_device_bus_num): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.example.dtb: Warning (simple_bus_reg): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.example.dtb: Warning (i2c_bus_reg): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.example.dtb: Warning (spi_bus_reg): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.example.dts:36.19-38.15: Warning (avoid_default_addr_size): /example-0/pwm-tach@1e610000/tach/fan@0: Relying on default #address-cells value Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.example.dts:36.19-38.15: Warning (avoid_default_addr_size): /example-0/pwm-tach@1e610000/tach/fan@0: Relying on default #size-cells value Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.example.dtb: Warning (unique_unit_address_if_enabled): Failed prerequisite 'avoid_default_addr_size' doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230606094535.5388-4-billy_tsai@aspeedtech.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.
On 06/06/2023 11:45, Billy Tsai wrote: > Add device binding for aspeed pwm-tach device which is a multi-function > device include pwm and tach function. > > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com> > > --- > .../bindings/mfd/aspeed,ast2600-pwm-tach.yaml | 76 +++++++++++++++++++ > 1 file changed, 76 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml > > diff --git a/Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml b/Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml > new file mode 100644 > index 000000000000..f98c11ff3f8a > --- /dev/null > +++ b/Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml > @@ -0,0 +1,76 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +# Copyright (C) 2021 Aspeed, Inc. > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/mfd/aspeed,ast2600-pwm-tach.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: PWM Tach controller > + > +description: | > + The PWM Tach controller is represented as a multi-function device which > + includes: > + PWM > + Tach > + > +maintainers: > + - Billy Tsai <billy_tsai@aspeedtech.com> > + > +properties: > + compatible: > + items: > + - enum: > + - aspeed,ast2600-pwm-tach > + - const: syscon > + - const: simple-mfd > + > + reg: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + > + resets: > + maxItems: 1 NAK. You got here clear comment. You cannot have simple MFD with resources. It is not simple anymore. Everywhere else you also ignored comments. Best regards, Krzysztof
On Tue, Jun 06, 2023 at 12:49:04PM +0200, Krzysztof Kozlowski wrote: Hi Krzysztof, Thank you for reviewing this from Billy. The Aspeed chip is heavily used by the OpenBMC community and the 2600 has been used in production systems for almost 2 years now. Many companies are having to carry previous versions of these as patches, and some of the APIs changed since the last revision from Billy. So, I had asked him to submit the latest patch set with as many revisions as he understood what to change, since the conversation seemed to have died since last time he submitted. I don't believe Billy is intentionally ignoring your feedback and he is motivated to get this patch set wrapped up into an acceptable state. > On 06/06/2023 11:45, Billy Tsai wrote: > NAK. You got here clear comment. You cannot have simple MFD with > resources. It is not simple anymore. > In fairness, Billy asked for clarification from you on this point and didn't receive it. https://lore.kernel.org/lkml/24DD1FEB-95F3-47BE-BE61-8B0E6FBDE20F@aspeedtech.com/ He felt what he was trying to accomplish met the documented expectations. Are there some changes that need to be done in mfd.txt to further clarify when to use it and when not to? Again, we appreciate the time you've spent reviewing this patch set already. Thank you.
On 06/06/2023 16:06, Patrick Williams wrote: > On Tue, Jun 06, 2023 at 12:49:04PM +0200, Krzysztof Kozlowski wrote: > > Hi Krzysztof, > > Thank you for reviewing this from Billy. > > The Aspeed chip is heavily used by the OpenBMC community and the 2600 > has been used in production systems for almost 2 years now. Many > companies are having to carry previous versions of these as patches, and > some of the APIs changed since the last revision from Billy. So, I had > asked him to submit the latest patch set with as many revisions as he > understood what to change, since the conversation seemed to have died > since last time he submitted. > > I don't believe Billy is intentionally ignoring your feedback and he is > motivated to get this patch set wrapped up into an acceptable state. > >> On 06/06/2023 11:45, Billy Tsai wrote: > >> NAK. You got here clear comment. You cannot have simple MFD with >> resources. It is not simple anymore. >> > > In fairness, Billy asked for clarification from you on this point and didn't > receive it. > > https://lore.kernel.org/lkml/24DD1FEB-95F3-47BE-BE61-8B0E6FBDE20F@aspeedtech.com/ I gave the instruction what Billy should do: https://lore.kernel.org/lkml/41500a04-b004-0e2c-20a1-3a3092b90e6d@linaro.org/ What about other ignored comments? About subject, quotes and more? Even if this one was unclear, then why ignoring all the rest? > > He felt what he was trying to accomplish met the documented > expectations. Are there some changes that need to be done in mfd.txt to > further clarify when to use it and when not to? I think mfd.txt clearly states: "For more complex devices, when the nexus driver has to probe registers to figure out what child devices exist etc, this should not be used. In the latter case the child devices will be determined by the operating system." Also, repeated many times: https://lore.kernel.org/all/YXhINE00HG6hbQI4@robh.at.kernel.org/ https://lore.kernel.org/all/20220701000959.GA3588170-robh@kernel.org/ https://osseu2022.sched.com/event/15z0W Best regards, Krzysztof
On 07/06/2023 08:26, Billy Tsai wrote: > On 06/06/2023 16:06, Patrick Williams wrote: > >> On Tue, Jun 06, 2023 at 12:49:04PM +0200, Krzysztof Kozlowski wrote: > >> > >> Hi Krzysztof, > >> > >> Thank you for reviewing this from Billy. > >> > >> The Aspeed chip is heavily used by the OpenBMC community and the 2600 > >> has been used in production systems for almost 2 years now. Many > >> companies are having to carry previous versions of these as patches, and > >> some of the APIs changed since the last revision from Billy. So, I had > >> asked him to submit the latest patch set with as many revisions as he > >> understood what to change, since the conversation seemed to have died > >> since last time he submitted. > >> > >> I don't believe Billy is intentionally ignoring your feedback and he is > >> motivated to get this patch set wrapped up into an acceptable state. > >> > >>> On 06/06/2023 11:45, Billy Tsai wrote: > >> > >>> NAK. You got here clear comment. You cannot have simple MFD with > >>> resources. It is not simple anymore. > >>> > >> > >> In fairness, Billy asked for clarification from you on this point and didn't > >> receive it. > >> > >> https://lore.kernel.org/lkml/24DD1FEB-95F3-47BE-BE61-8B0E6FBDE20F@aspeedtech.com/ > > > I gave the instruction what Billy should do: > > > https://lore.kernel.org/lkml/41500a04-b004-0e2c-20a1-3a3092b90e6d@linaro.org/ > > > What about other ignored comments? About subject, quotes and more? Even > > if this one was unclear, then why ignoring all the rest? > > It's possible that there was some confusion regarding your message. I apologize for any misunderstanding. > About the subject: I apologize for the misunderstanding. I just drop the redundant "bindings" in the commit message. Read entire message, not some parts of it. "Subject: drop second, redundant "bindings". Also use proper PATCH prefix." Where did you drop the bindings in the subject? I still see it, look: "dt-bindings: mfd: Add aspeed pwm-tach binding" ^^^^^^^^ what is this? > About the quotes: I believe the issue was simply related to the order of the patches, and I have resolved it. Did I misunderstand? I still see them, so how did you solve them? > About the Missing description: > >> +patternProperties: >> + "^fan@[a-z0-9]+$": >> + type: object > >> Missing description. But more important - why do you have such child >> nodes? Your example does not have them. What's the point? Do you expect >> different number of fans per one device (one compatible)? > > In this patch series, I have included examples and descriptions to provide additional information. > The child node is used to enable the channel of this tach controller. You do not need children for this. > I expect that the dts will include information regarding the number of fans connected to the board and their corresponding channels. Your example DTS must be complete and nothing like this was there. There is still no point in the children. > > >> > >> He felt what he was trying to accomplish met the documented > >> expectations. Are there some changes that need to be done in mfd.txt to > >> further clarify when to use it and when not to? > > > I think mfd.txt clearly states: > > "For more complex devices, when the nexus driver has to > > probe registers to figure out what child devices exist etc, this should > > not be used. In the latter case the child devices will be determined by > > the operating system." > > About the mfd: > For our pwm and tach devices, there is no need to check/apply any hardware register from parent to determine child’s existence or functional. > They don’t have any dependency on the parent node. You are joking right? The dependency is clearly visible in the driver. You are getting parent's node to get its resources. That's the dependency which is not allowed. Children should take care of their resources, not parent's! > In fact, it doesn’t require a specific driver to bind with the "aspeed,ast2600-pwm-tach" label. Their purpose is solely to share the same clock, reset phandle and base address. That's what the drivers are for, so you need it... > The main reason for using simple-mfd in this case is because these two independent devices share the same base address. In fact, I can relocate the clock and reset configurations to the child nodes rather than the parent node. How? These are clocks of parent. Don't create fake DTS to represent workarounds. Or you want to say that current DTS is fake and does not match the hardware? > In this case, I still can't use simple-mfd? For the last time: No. You cannot, because you have resources needed for children. Best regards, Krzysztof
On 07/06/2023 10:32, Billy Tsai wrote: > Ok, I got it. I will remove usage of the simple-mfd and parent node in next version of the patch. > Thanks > 1. Whether parent node stays or not, depends on the hardware. Please do not make random changes which do not correspond to the hardware. 2. Implement all, *all* the comments from previous discussions, not only this one. Best regards, Krzysztof
On 07/06/2023 08:26, Billy Tsai wrote: > >> > >> He felt what he was trying to accomplish met the documented > >> expectations. Are there some changes that need to be done in mfd.txt to > >> further clarify when to use it and when not to? > > > I think mfd.txt clearly states: > > "For more complex devices, when the nexus driver has to > > probe registers to figure out what child devices exist etc, this should > > not be used. In the latter case the child devices will be determined by > > the operating system." > > About the mfd: > For our pwm and tach devices, there is no need to check/apply any hardware register from parent to determine child’s existence or functional. > They don’t have any dependency on the parent node. In fact, it doesn’t require a specific driver to bind with the "aspeed,ast2600-pwm-tach" label. Their purpose is solely to share the same clock, reset phandle and base address. The main reason for using simple-mfd in this case is because these two independent devices share the same base address. Actually one more thoughts. I have doubt that you have two independent devices. If you share the clock, reset line and register address space, this means *you do not have two independent devices*. You have most likely only one device. Best regards, Krzysztof
On 07/06/2023 08:26, Billy Tsai wrote: >> Missing description. But more important - why do you have such child >> nodes? Your example does not have them. What's the point? Do you expect >> different number of fans per one device (one compatible)? > > In this patch series, I have included examples and descriptions to provide additional information. > The child node is used to enable the channel of this tach controller. Children are not for this. Look for cells examples (e.g. gpio-cells, pwm-cells). It seems this is the same as Nuvoton NCT7362Y, so no. Don't use reg for that purpose. Best regards, Krzysztof
On Tue, Jun 06, 2023 at 04:23:52PM +0200, Krzysztof Kozlowski wrote: > On 06/06/2023 16:06, Patrick Williams wrote: > > On Tue, Jun 06, 2023 at 12:49:04PM +0200, Krzysztof Kozlowski wrote: > > > > Hi Krzysztof, > > > > Thank you for reviewing this from Billy. > > > > The Aspeed chip is heavily used by the OpenBMC community and the 2600 > > has been used in production systems for almost 2 years now. Many > > companies are having to carry previous versions of these as patches, and > > some of the APIs changed since the last revision from Billy. So, I had > > asked him to submit the latest patch set with as many revisions as he > > understood what to change, since the conversation seemed to have died > > since last time he submitted. > > > > I don't believe Billy is intentionally ignoring your feedback and he is > > motivated to get this patch set wrapped up into an acceptable state. > > > >> On 06/06/2023 11:45, Billy Tsai wrote: > > > >> NAK. You got here clear comment. You cannot have simple MFD with > >> resources. It is not simple anymore. > >> > > > > In fairness, Billy asked for clarification from you on this point and didn't > > receive it. > > > > https://lore.kernel.org/lkml/24DD1FEB-95F3-47BE-BE61-8B0E6FBDE20F@aspeedtech.com/ > > I gave the instruction what Billy should do: > > https://lore.kernel.org/lkml/41500a04-b004-0e2c-20a1-3a3092b90e6d@linaro.org/ > > What about other ignored comments? About subject, quotes and more? Even > if this one was unclear, then why ignoring all the rest? > > > > > He felt what he was trying to accomplish met the documented > > expectations. Are there some changes that need to be done in mfd.txt to > > further clarify when to use it and when not to? > > I think mfd.txt clearly states: > "For more complex devices, when the nexus driver has to > probe registers to figure out what child devices exist etc, this should > not be used. In the latter case the child devices will be determined by > the operating system." > > Also, repeated many times: > https://lore.kernel.org/all/YXhINE00HG6hbQI4@robh.at.kernel.org/ > https://lore.kernel.org/all/20220701000959.GA3588170-robh@kernel.org/ > https://osseu2022.sched.com/event/15z0W I've probably said this already, but any 'fan controller' needs to define a common fan binding that works for multiple scenarios. There's been some attempts in the last year which seems to have stalled out. Rob
diff --git a/Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml b/Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml new file mode 100644 index 000000000000..f98c11ff3f8a --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml @@ -0,0 +1,76 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +# Copyright (C) 2021 Aspeed, Inc. +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/mfd/aspeed,ast2600-pwm-tach.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: PWM Tach controller + +description: | + The PWM Tach controller is represented as a multi-function device which + includes: + PWM + Tach + +maintainers: + - Billy Tsai <billy_tsai@aspeedtech.com> + +properties: + compatible: + items: + - enum: + - aspeed,ast2600-pwm-tach + - const: syscon + - const: simple-mfd + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + + resets: + maxItems: 1 + + pwm: + type: object + $ref: "/schemas/pwm/aspeed,ast2600-pwm.yaml" + + tach: + type: object + $ref: "/schemas/hwmon/aspeed,ast2600-tach.yaml" + +required: + - compatible + - reg + - clocks + - resets + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/ast2600-clock.h> + pwm_tach: pwm-tach@1e610000 { + compatible = "aspeed,ast2600-pwm-tach", "syscon", "simple-mfd"; + reg = <0x1e610000 0x100>; + clocks = <&syscon ASPEED_CLK_AHB>; + resets = <&syscon ASPEED_RESET_PWM>; + + pwm: pwm { + compatible = "aspeed,ast2600-pwm"; + #pwm-cells = <3>; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_pwm0_default>; + }; + + tach: tach { + compatible = "aspeed,ast2600-tach"; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_tach0_default>; + fan@0 { + reg = <0x00>; + }; + }; + };
Add device binding for aspeed pwm-tach device which is a multi-function device include pwm and tach function. Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com> --- .../bindings/mfd/aspeed,ast2600-pwm-tach.yaml | 76 +++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml