Re: [PATCH 1/6] arm64: dts: qcom: ipq9574: Add PPE device tree node

[Krzysztof Kozlowski]

On 10/01/2024 12:20, Luo Jie wrote:
> The PPE device tree node includes the PPE initialization configurations
> and UNIPHY instance configuration.
> 
> Ther are 3 UNIPHYs(PCS) on the platform ipq9574, which register the
> clock provider to output the clock for PPE port to work on the different
> link speed.
> 
> Signed-off-by: Luo Jie <quic_luoj@xxxxxxxxxxx>
> ---
>  arch/arm64/boot/dts/qcom/ipq9574.dtsi | 730 +++++++++++++++++++++++++-
>  1 file changed, 724 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> index 810cda4a850f..5fa241e27c8b 100644
> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> @@ -775,16 +775,734 @@ nsscc: nsscc@39b00000 {
>  				 <&bias_pll_nss_noc_clk>,
>  				 <&bias_pll_ubi_nc_clk>,
>  				 <&gcc_gpll0_out_aux>,
> -				 <0>,
> -				 <0>,
> -				 <0>,
> -				 <0>,
> -				 <0>,
> -				 <0>,
> +				 <&uniphys 0>,
> +				 <&uniphys 1>,
> +				 <&uniphys 2>,
> +				 <&uniphys 3>,
> +				 <&uniphys 4>,
> +				 <&uniphys 5>,
>  				 <&xo_board_clk>;
>  			#clock-cells = <1>;
>  			#reset-cells = <1>;
>  		};
> +
> +		qcom_ppe: qcom-ppe@3a000000 {

qcom is definitely not a generic name.

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> +			compatible = "qcom,ipq9574-ppe";

I don't see this documented. I don't see reference to posted bindings.

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.

> +			reg = <0x3a000000 0xb00000>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;

Put after reg.

> +			status = "okay";

Drop

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