| Message ID | 20240409190833.3485824-6-mr.nuke.me@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2,1/7] dt-bindings: clock: Add PCIe pipe related clocks for IPQ9574 | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dt-meta-schema | fail | build log |
On 09/04/2024 21:08, Alexandru Gagniuc wrote: > The IPQ9574 gen3x2 PHY is very similar to IPQ6018. It requires two > extra clocks named "anoc" and "snoc". Document this, and add a > new compatible string for this PHY. > > Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> > --- > .../phy/qcom,ipq8074-qmp-pcie-phy.yaml | 31 ++++++++++++++++++- > 1 file changed, 30 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/phy/qcom,ipq8074-qmp-pcie-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,ipq8074-qmp-pcie-phy.yaml > index 634cec5d57ea..017ad65a9a3c 100644 > --- a/Documentation/devicetree/bindings/phy/qcom,ipq8074-qmp-pcie-phy.yaml > +++ b/Documentation/devicetree/bindings/phy/qcom,ipq8074-qmp-pcie-phy.yaml > @@ -19,19 +19,22 @@ properties: > - qcom,ipq6018-qmp-pcie-phy > - qcom,ipq8074-qmp-gen3-pcie-phy > - qcom,ipq8074-qmp-pcie-phy > + - qcom,ipq9574-qmp-gen3x2-pcie-phy > > reg: > items: > - description: serdes > > clocks: > - maxItems: 3 > + minItems: 3 Which binding inspired you to such change? No, you need maxItems. See your previous patches here how it is done. > > clock-names: > items: > - const: aux > - const: cfg_ahb > - const: pipe > + - const: anoc > + - const: snoc OK, you did not test it. Neither this, nor DTS. I stop review, please test first. Best regards, Krzysztof
On 4/9/24 15:09, Krzysztof Kozlowski wrote: > On 09/04/2024 21:08, Alexandru Gagniuc wrote: >> The IPQ9574 gen3x2 PHY is very similar to IPQ6018. It requires two >> extra clocks named "anoc" and "snoc". Document this, and add a >> new compatible string for this PHY. >> >> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> >> --- >> .../phy/qcom,ipq8074-qmp-pcie-phy.yaml | 31 ++++++++++++++++++- >> 1 file changed, 30 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/phy/qcom,ipq8074-qmp-pcie-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,ipq8074-qmp-pcie-phy.yaml >> index 634cec5d57ea..017ad65a9a3c 100644 >> --- a/Documentation/devicetree/bindings/phy/qcom,ipq8074-qmp-pcie-phy.yaml >> +++ b/Documentation/devicetree/bindings/phy/qcom,ipq8074-qmp-pcie-phy.yaml >> @@ -19,19 +19,22 @@ properties: >> - qcom,ipq6018-qmp-pcie-phy >> - qcom,ipq8074-qmp-gen3-pcie-phy >> - qcom,ipq8074-qmp-pcie-phy >> + - qcom,ipq9574-qmp-gen3x2-pcie-phy >> >> reg: >> items: >> - description: serdes >> >> clocks: >> - maxItems: 3 >> + minItems: 3 > > Which binding inspired you to such change? No, you need maxItems. See > your previous patches here how it is done. > > >> >> clock-names: >> items: >> - const: aux >> - const: cfg_ahb >> - const: pipe >> + - const: anoc >> + - const: snoc > > OK, you did not test it. Neither this, nor DTS. I stop review, please > test first. I ran both `checkpatch.pl` and `make dt_binding_check`. What in this patch makes you say I "did not test it", and what test or tests did I miss? Alex
On 09/04/2024 22:19, mr.nuke.me@gmail.com wrote: >> Which binding inspired you to such change? No, you need maxItems. See >> your previous patches here how it is done. >> >> >>> >>> clock-names: >>> items: >>> - const: aux >>> - const: cfg_ahb >>> - const: pipe >>> + - const: anoc >>> + - const: snoc >> >> OK, you did not test it. Neither this, nor DTS. I stop review, please >> test first. > > I ran both `checkpatch.pl` and `make dt_binding_check`. What in this > patch makes you say I "did not test it", and what test or tests did I miss? You affect existing bindings, so you must test your and entire existing DTS. You affect, by introducing new errors, in existing DTS. Best regards, Krzysztof
On Tue, 09 Apr 2024 14:08:31 -0500, Alexandru Gagniuc wrote: > The IPQ9574 gen3x2 PHY is very similar to IPQ6018. It requires two > extra clocks named "anoc" and "snoc". Document this, and add a > new compatible string for this PHY. > > Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> > --- > .../phy/qcom,ipq8074-qmp-pcie-phy.yaml | 31 ++++++++++++++++++- > 1 file changed, 30 insertions(+), 1 deletion(-) > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/phy/qcom,ipq8074-qmp-pcie-phy.example.dtb: phy@84000: clock-names: ['aux', 'cfg_ahb', 'pipe'] is too short from schema $id: http://devicetree.org/schemas/phy/qcom,ipq8074-qmp-pcie-phy.yaml# doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240409190833.3485824-6-mr.nuke.me@gmail.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 09/04/2024 22:19, mr.nuke.me@gmail.com wrote: >> >> >>> >>> clock-names: >>> items: >>> - const: aux >>> - const: cfg_ahb >>> - const: pipe >>> + - const: anoc >>> + - const: snoc >> >> OK, you did not test it. Neither this, nor DTS. I stop review, please >> test first. > > I ran both `checkpatch.pl` and `make dt_binding_check`. What in this > patch makes you say I "did not test it", and what test or tests did I miss? > ... and no, you did not. If you tested, you would easily see error: clock-names: ['aux', 'cfg_ahb', 'pipe'] is too short When you receive comment from reviewer, please investigate thoroughly what could get wrong. Don't answer just to get rid of reviewer. It's fine to make mistakes, but if reviewer points to issue and you immediately respond "no issue", that's waste of my time. Look at entire code of qcom,pcie how it is organized. Or: https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L132 Best regards, Krzysztof
On 10/04/2024 08:59, Krzysztof Kozlowski wrote: > On 09/04/2024 22:19, mr.nuke.me@gmail.com wrote: >>> >>> >>>> >>>> clock-names: >>>> items: >>>> - const: aux >>>> - const: cfg_ahb >>>> - const: pipe >>>> + - const: anoc >>>> + - const: snoc >>> >>> OK, you did not test it. Neither this, nor DTS. I stop review, please >>> test first. >> >> I ran both `checkpatch.pl` and `make dt_binding_check`. What in this >> patch makes you say I "did not test it", and what test or tests did I miss? >> > > ... and no, you did not. If you tested, you would easily see error: > clock-names: ['aux', 'cfg_ahb', 'pipe'] is too short > > When you receive comment from reviewer, please investigate thoroughly > what could get wrong. Don't answer just to get rid of reviewer. It's > fine to make mistakes, but if reviewer points to issue and you > immediately respond "no issue", that's waste of my time. To clarify: "no issue" response is waste of my time. If you responded "oh, I see the error, but I don't know how to fix it", it would be ok, I can clarify and help in this. Best regards, Krzysztof
On 4/10/24 02:02, Krzysztof Kozlowski wrote: > On 10/04/2024 08:59, Krzysztof Kozlowski wrote: >> On 09/04/2024 22:19, mr.nuke.me@gmail.com wrote: >>>> >>>> >>>>> >>>>> clock-names: >>>>> items: >>>>> - const: aux >>>>> - const: cfg_ahb >>>>> - const: pipe >>>>> + - const: anoc >>>>> + - const: snoc >>>> >>>> OK, you did not test it. Neither this, nor DTS. I stop review, please >>>> test first. >>> >>> I ran both `checkpatch.pl` and `make dt_binding_check`. What in this >>> patch makes you say I "did not test it", and what test or tests did I miss? >>> >> >> ... and no, you did not. If you tested, you would easily see error: >> clock-names: ['aux', 'cfg_ahb', 'pipe'] is too short >> >> When you receive comment from reviewer, please investigate thoroughly >> what could get wrong. Don't answer just to get rid of reviewer. It's >> fine to make mistakes, but if reviewer points to issue and you >> immediately respond "no issue", that's waste of my time. > > To clarify: "no issue" response is waste of my time. If you responded > "oh, I see the error, but I don't know how to fix it", it would be ok, I > can clarify and help in this. I apologize if I gave you this impression. I tried to follow the testing process, it did not turn out as expected. Obviously, I missed something. I tried to ask what I missed, and in order for that question to make sense, I need to describe what I tried. It turns out what I missed was "make check_dtbs". I only found that out after an automated email from Rob describing some troubleshooting steps. If I may have a few sentences to rant, I see the dt-schema as a hurdle to making an otherwise useful change. I am told I can ask for help when I get stuck, yet I manage to insult the maintainer by aking for help. I find this very intimidating. Alex
On 10/04/2024 18:29, mr.nuke.me@gmail.com wrote: > > > On 4/10/24 02:02, Krzysztof Kozlowski wrote: >> On 10/04/2024 08:59, Krzysztof Kozlowski wrote: >>> On 09/04/2024 22:19, mr.nuke.me@gmail.com wrote: >>>>> >>>>> >>>>>> >>>>>> clock-names: >>>>>> items: >>>>>> - const: aux >>>>>> - const: cfg_ahb >>>>>> - const: pipe >>>>>> + - const: anoc >>>>>> + - const: snoc >>>>> >>>>> OK, you did not test it. Neither this, nor DTS. I stop review, please >>>>> test first. >>>> >>>> I ran both `checkpatch.pl` and `make dt_binding_check`. What in this >>>> patch makes you say I "did not test it", and what test or tests did I miss? >>>> >>> >>> ... and no, you did not. If you tested, you would easily see error: >>> clock-names: ['aux', 'cfg_ahb', 'pipe'] is too short >>> >>> When you receive comment from reviewer, please investigate thoroughly >>> what could get wrong. Don't answer just to get rid of reviewer. It's >>> fine to make mistakes, but if reviewer points to issue and you >>> immediately respond "no issue", that's waste of my time. >> >> To clarify: "no issue" response is waste of my time. If you responded >> "oh, I see the error, but I don't know how to fix it", it would be ok, I >> can clarify and help in this. > > I apologize if I gave you this impression. I tried to follow the testing > process, it did not turn out as expected. Obviously, I missed something. > I tried to ask what I missed, and in order for that question to make > sense, I need to describe what I tried. > > It turns out what I missed was "make check_dtbs". I only found that out > after an automated email from Rob describing some troubleshooting steps. No, the dt_binding_check fails. You don't need to go to dtbs_check even, because the binding already has a failure. > > If I may have a few sentences to rant, I see the dt-schema as a hurdle > to making an otherwise useful change. I am told I can ask for help when > I get stuck, yet I manage to insult the maintainer by aking for help. I > find this very intimidating. I don't feel insulted but I feel my time is wasted if I tell you to test your binding and you immediately within few minutes respond "I tested", but then: 1. Bot confirms it was not tested, 2. I apply your patch and test it and see the failure. Best regards, Krzysztof
On 4/10/24 14:36, Krzysztof Kozlowski wrote: > On 10/04/2024 18:29, mr.nuke.me@gmail.com wrote: >> >> >> On 4/10/24 02:02, Krzysztof Kozlowski wrote: >>> On 10/04/2024 08:59, Krzysztof Kozlowski wrote: >>>> On 09/04/2024 22:19, mr.nuke.me@gmail.com wrote: >>>>>> >>>>>> >>>>>>> >>>>>>> clock-names: >>>>>>> items: >>>>>>> - const: aux >>>>>>> - const: cfg_ahb >>>>>>> - const: pipe >>>>>>> + - const: anoc >>>>>>> + - const: snoc >>>>>> >>>>>> OK, you did not test it. Neither this, nor DTS. I stop review, please >>>>>> test first. >>>>> >>>>> I ran both `checkpatch.pl` and `make dt_binding_check`. What in this >>>>> patch makes you say I "did not test it", and what test or tests did I miss? >>>>> >>>> >>>> ... and no, you did not. If you tested, you would easily see error: >>>> clock-names: ['aux', 'cfg_ahb', 'pipe'] is too short >>>> >>>> When you receive comment from reviewer, please investigate thoroughly >>>> what could get wrong. Don't answer just to get rid of reviewer. It's >>>> fine to make mistakes, but if reviewer points to issue and you >>>> immediately respond "no issue", that's waste of my time. >>> >>> To clarify: "no issue" response is waste of my time. If you responded >>> "oh, I see the error, but I don't know how to fix it", it would be ok, I >>> can clarify and help in this. >> >> I apologize if I gave you this impression. I tried to follow the testing >> process, it did not turn out as expected. Obviously, I missed something. >> I tried to ask what I missed, and in order for that question to make >> sense, I need to describe what I tried. >> >> It turns out what I missed was "make check_dtbs". I only found that out >> after an automated email from Rob describing some troubleshooting steps. > > No, the dt_binding_check fails. You don't need to go to dtbs_check even, > because the binding already has a failure. > >> >> If I may have a few sentences to rant, I see the dt-schema as a hurdle >> to making an otherwise useful change. I am told I can ask for help when >> I get stuck, yet I manage to insult the maintainer by aking for help. I >> find this very intimidating. > > I don't feel insulted but I feel my time is wasted if I tell you to test > your binding and you immediately within few minutes respond "I tested", > but then: > 1. Bot confirms it was not tested, > 2. I apply your patch and test it and see the failure. Thank you for double checking, and I am sorry this escalated in this manner. I believed you the first time that something is wrong, and I had a hard time figuring out what. I am now able to repro the errors, and the below changes appear to work. Is that sufficient? clocks: minItems: 3 maxItems: 5 clock-names: minItems: 3 items: - ... (5 clock names here) For ipq6018/ipq8074: properties: clocks: maxItems: 3 clock-names: maxItems: 3 For ipq9574: properties: clocks: minItems: 5 clock-names: minItems: 5
On 11/04/2024 19:24, mr.nuke.me@gmail.com wrote: > > I am now able to repro the errors, and the below changes appear to work. > Is that sufficient? > > clocks: > minItems: 3 > maxItems: 5 > > clock-names: > minItems: 3 > items: > - ... (5 clock names here) > > For ipq6018/ipq8074: > > properties: > clocks: > maxItems: 3 > clock-names: > maxItems: 3 > > For ipq9574: > > properties: > clocks: > minItems: 5 > clock-names: > minItems: 5 Yes, looks good. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/phy/qcom,ipq8074-qmp-pcie-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,ipq8074-qmp-pcie-phy.yaml index 634cec5d57ea..017ad65a9a3c 100644 --- a/Documentation/devicetree/bindings/phy/qcom,ipq8074-qmp-pcie-phy.yaml +++ b/Documentation/devicetree/bindings/phy/qcom,ipq8074-qmp-pcie-phy.yaml @@ -19,19 +19,22 @@ properties: - qcom,ipq6018-qmp-pcie-phy - qcom,ipq8074-qmp-gen3-pcie-phy - qcom,ipq8074-qmp-pcie-phy + - qcom,ipq9574-qmp-gen3x2-pcie-phy reg: items: - description: serdes clocks: - maxItems: 3 + minItems: 3 clock-names: items: - const: aux - const: cfg_ahb - const: pipe + - const: anoc + - const: snoc resets: maxItems: 2 @@ -61,6 +64,32 @@ required: - clock-output-names - "#phy-cells" +allOf: + - if: + properties: + compatible: + contains: + enum: + - qcom,ipq6018-qmp-pcie-phy + - qcom,ipq8074-qmp-gen3-pcie-phy + - qcom,ipq8074-qmp-pcie-phy + then: + properties: + clocks: + maxItems: 3 + + - if: + properties: + compatible: + contains: + enum: + - qcom,ipq9574-qmp-gen3x2-pcie-phy + then: + properties: + clocks: + minItems: 5 + maxItems: 5 + additionalProperties: false examples:
The IPQ9574 gen3x2 PHY is very similar to IPQ6018. It requires two extra clocks named "anoc" and "snoc". Document this, and add a new compatible string for this PHY. Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> --- .../phy/qcom,ipq8074-qmp-pcie-phy.yaml | 31 ++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-)