| Message ID | 20241213013051.11095-3-shimrrashai@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: dts: rockchip: Add Firefly ITX-3588J Board | expand |
On 13/12/2024 02:30, Shimrra Shai wrote: > Signed-off-by: Shimrra Shai <shimrrashai@gmail.com> > --- Explain why this is draft, what does it even mean. Do you expect any review or not? Please run scripts/checkpatch.pl and fix reported warnings. Then please run `scripts/checkpatch.pl --strict` and (probably) fix more warnings. Some warnings can be ignored, especially from --strict run, but the code here looks like it needs a fix. Feel free to get in touch if the warning is not clear. <form letter> Please use scripts/get_maintainers.pl to get a list of necessary people and lists to CC. It might happen, that command when run on an older kernel, gives you outdated entries. Therefore please be sure you base your patches on recent Linux kernel. Tools like b4 or scripts/get_maintainer.pl provide you proper list of people, so fix your workflow. Tools might also fail if you work on some ancient tree (don't, instead use mainline) or work on fork of kernel (don't, instead use mainline). Just use b4 and everything should be fine, although remember about `b4 prep --auto-to-cc` if you added new patches to the patchset. You missed at least devicetree list (maybe more), so this won't be tested by automated tooling. Performing review on untested code might be a waste of time. Please kindly resend and include all necessary To/Cc entries. </form letter> Best regards, Krzysztof
On 2024-12-13, Krzysztof Kozlowski wrote: > Explain why this is draft, what does it even mean. Do you expect any > review or not? Correct. As I pointed out, not 100% of things work. > Please run scripts/checkpatch.pl and fix reported warnings. Then please > run `scripts/checkpatch.pl --strict` and (probably) fix more warnings. > Some warnings can be ignored, especially from --strict run, but the code > here looks like it needs a fix. Feel free to get in touch if the warning > is not clear. I did this, but I do not see any warnings beyond "Prefer a maximum 75 chars per line (possible unwrapped commit description?)" for the 0th patch, which does not seem to be from the description and "Missing commit description - Add an appropriate one" for the others, and "added, moved or deleted file(s), does MAINTAINERS need updating?" on the 1st. There don't seem to be any substantial errors indicated with the code itself. What issues did you find, as you said it "looks like it needs a fix"? Nonetheless I wasn't planning on this one being a final submit anyway, since as I said it was a draft because there were things not working yet. But if there are other problems with it, I need to know what they are esp. given as I said those tools have not indicated more problems than those and they seem to do more with not adding further info to the emails than the code itself, yet you say the actual code needs a fix. > Please use scripts/get_maintainers.pl to get a list of necessary people > and lists to CC. It might happen, that command when run on an older > kernel, gives you outdated entries. Therefore please be sure you base > your patches on recent Linux kernel. Thanks for all this part. When you say this though: > You missed at least devicetree list (maybe more), so this won't be > tested by automated tooling. Performing review on untested code might be > a waste of time. what do you mean by "device tree list"? I was not aware of this part of the kernel source code. I modeled this submission off of others I've seen here and I have only seen them submit the .dts, Makefile entry, and .yaml entry in rockchip.yaml. I have not seen a "device tree list" different from those. E.g. for this submission for the Orange Pi 5, https://lore.kernel.org/linux-rockchip/20241111045408.1922-1-honyuenkwun@gmail.com/ those are the only items touched that I can see unless I missed something really subtle. Shimrra Shai
On 13/12/2024 16:02, Shimrra Shai wrote: > On 2024-12-13, Krzysztof Kozlowski wrote: > >> Explain why this is draft, what does it even mean. Do you expect any >> review or not? > > Correct. As I pointed out, not 100% of things work. > >> Please run scripts/checkpatch.pl and fix reported warnings. Then please >> run `scripts/checkpatch.pl --strict` and (probably) fix more warnings. >> Some warnings can be ignored, especially from --strict run, but the code >> here looks like it needs a fix. Feel free to get in touch if the warning >> is not clear. > > I did this, but I do not see any warnings beyond > > "Prefer a maximum 75 chars per line (possible unwrapped commit > description?)" > > for the 0th patch, which does not seem to be from the description and > > "Missing commit description - Add an appropriate one" > > for the others, and > > "added, moved or deleted file(s), does MAINTAINERS need updating?" > > on the 1st. > > There don't seem to be any substantial errors indicated with the code > itself. What issues did you find, as you said it "looks like it needs a ""Missing commit description - Add an appropriate one"" is a substantial one - clearly we cannot take empty commits. > fix"? Nonetheless I wasn't planning on this one being a final submit > anyway, since as I said it was a draft because there were things not > working yet. But if there are other problems with it, I need to know what > they are esp. given as I said those tools have not indicated more problems > than those and they seem to do more with not adding further info to the > emails than the code itself, yet you say the actual code needs a fix. > >> Please use scripts/get_maintainers.pl to get a list of necessary people >> and lists to CC. It might happen, that command when run on an older >> kernel, gives you outdated entries. Therefore please be sure you base >> your patches on recent Linux kernel. > > Thanks for all this part. When you say this though: > >> You missed at least devicetree list (maybe more), so this won't be >> tested by automated tooling. Performing review on untested code might be >> a waste of time. > > what do you mean by "device tree list"? I was not aware of this part of I mean exactly what is written. Use the tools and the tools will do the job. > the kernel source code. I modeled this submission off of others I've seen This does not work like this. Use the tools, not other people's incorrect CC list. > here and I have only seen them submit the .dts, Makefile entry, and .yaml > entry in rockchip.yaml. I have not seen a "device tree list" different from > those. E.g. for this submission for the Orange Pi 5, > > https://lore.kernel.org/linux-rockchip/20241111045408.1922-1-honyuenkwun@gmail.com/ > > those are the only items touched that I can see unless I missed something Cc list is entirely different... Did you really read my message? I state you Cc wrong addresses and you claim that above link has the same as yours, which is obviously not correct. So two things - my earlier message and above link - are kind of proofs. What else do you need? I gave you instruction which tools to use, so I do not understand why do you insist on not using them. Best regards, Krzysztof
On 2024-12-13, Krzysztof Kozlowski wrote: > I mean exactly what is written. Use the tools and the tools will do the > job. Since the remainder of your post is talking about CCs and the only other tools listed are related to CCs does this mean that by "device tree list" you were referring to a place to send messages, not a file in the source code with a list of device trees in it? I was interpreting your statement as referring to the latter, not the former. You also mentioned things being wrong with the code, too, even though they did not show up on the scripts/ checkpatch.pl tool. > This does not work like this. Use the tools, not other people's > incorrect CC list. > I gave you instruction which tools to use, so I do not understand why do > you insist on not using them. I am having trouble understanding is why. Are you talking about messaging or a part of the source code itself when you say "device tree list"? As you said that is necessary "for automated tools to work" and that makes me think it's some part of the source code run by such a tool, but here you are talking about "CC lists" i.e. messaging. So I do not understand what that word is referring to - that is my confusion. I certainly can use those CC tools but if there is also something that needs change in the code it won't be changed unless I know exactly what that is and thus I need to know if there are any outstanding code changes indicated before I make another submission using the CC tools. Shimrra
On 2024-12-13, Shimrra Shai wrote:
> As you said that is necessary "for automated tools to work"
Correction to make an exact quotation: I meant to refer to when you said
"so this won't be tested by automated tooling".
Shimrra
On 13/12/2024 17:03, Shimrra Shai wrote: > On 2024-12-13, Shimrra Shai wrote: >> As you said that is necessary "for automated tools to work" > > Correction to make an exact quotation: I meant to refer to when you said > "so this won't be tested by automated tooling". > This was one form letter referring to one issue - you did not use the tools to create correct CC/To address list. You split it unnecessarily looking for some other meaning while it is still above one same thing. DT list is a discussion or mailing list. Entire development is via discussion lists. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/arm/rockchip.yaml b/Documentation/devicetree/bindings/arm/rockchip.yaml index 753199a12..9ee96acdc 100644 --- a/Documentation/devicetree/bindings/arm/rockchip.yaml +++ b/Documentation/devicetree/bindings/arm/rockchip.yaml @@ -190,6 +190,11 @@ properties: - const: firefly,firefly-rk3399 - const: rockchip,rk3399 + - description: Firefly ITX-3588J + items: + - const: firefly,itx-3588j + - const: rockchip,rk3588 + - description: Firefly ROC-RK3308-CC items: - const: firefly,roc-rk3308-cc
Signed-off-by: Shimrra Shai <shimrrashai@gmail.com> --- Documentation/devicetree/bindings/arm/rockchip.yaml | 5 +++++ 1 file changed, 5 insertions(+)