Re: Re: [PATCH v1 2/2] [Draft] dt-bindings: arm: rockchip: Add Firefly ITX-3588J board

From: Shimrra Shai <shimrrashai@gmail.com>
To: krzk@kernel.org
Cc: linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
	linux-rockchip@lists.infradead.org, shimrrashai@gmail.com
Subject: Re: Re: [PATCH v1 2/2] [Draft] dt-bindings: arm: rockchip: Add Firefly ITX-3588J board
Date: Fri, 13 Dec 2024 09:02:25 -0600	[thread overview]
Message-ID: <20241213150225.3538-1-shimrrashai@gmail.com> (raw)
In-Reply-To: <c1b662d8-6470-49f2-a904-139a33061885@kernel.org>

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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip