On 1/12/2024 12:06 AM, Dmitry Baryshkov wrote:
On Thu, 11 Jan 2024 at 17:31, Jie Luo <quic_luoj@xxxxxxxxxxx> wrote:
Ok, will update to use a generic name in the link, Thanks for the
guidance and the link.
+ compatible = "qcom,ipq9574-ppe";
I don't see this documented. I don't see reference to posted bindings.
The DT bindings patch was part of the driver series as below. This
property was documented in the DT bindings patch. Attaching it to DTSI
series should make it more clear. If this is fine, I will update the
DTSI series with the DT bindings patch.
https://lore.kernel.org/netdev/20240110142428.52026d9e@xxxxxxxxxx/
Please run scripts/checkpatch.pl and fix reported warnings. Some
warnings can be ignored, but the code here looks like it needs a fix.
Feel free to get in touch if the warning is not clear.
Ignoring this warning is a sign you don't really check your patches
before sending.
We have run the checkpatch.pl on the whole patch series including this
device tree patch set together with PPE driver patch set.
As mentioned above, I will add the DT bindings patch into the DTS
series. This should help with the checkpatch issue.
This will cause even more confusion, as there will be two instances of
the dt-bindings patch. One in the driver patchset, another one in the
DT changes. You just have to specify the dependencies in the cover
letter. Another option is to wait for the bindings + driver to be
accepted, then send the DTSI changes (and again, specify the
dependency).
Thanks Dmitry for the suggestions.
As per the ongoing discussion on this series, we will hold off this DTS
patch series for some time. We will update the cover letter of the DTSI
series to point to the below driver series as a dependency, when we
resume the series.
https://lore.kernel.org/netdev/20240110154414.GH9296@xxxxxxxxxx/
+ reg = <0x3a000000 0xb00000>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
Put after reg.
Ok.
+ status = "okay";
Drop
Ok.
All of above comments apply to your entire patchset and all places.
Looking at code further, it does not look like suitable for mainline,
but copy of downstream code. That's not what we expect upstream. Please
go back to your bindings first. Also, I really insist you reaching out
to other folks to help you in this process.
Best regards,
Krzysztof
We will do internal review of the gaps and update the patches as per
your comments.
Thanks for the review comments.
From the first glance, the bindings do not follow upstream principles.
You have all the settings (tdm, port config, etc) in the DT, while
they should instead go to the driver. Well, unless you expect that the
board might need to override them.
Hi Dmitry,
The TDM configuration varies per SoC type, since the ethernet port
capabilities of the SoCs vary. So we will have two different TDM
configurations for IPQ5332 and IPQ9574 SoC. The driver also will
need to support future SoC, so we choose to configure this from the
DTSI. The same reason applies to the port scheduler config as well.
Thanks for review comments.
