Re: [PATCH v2 1/1] arm64: dts: Add usbphynop and usbotg pinctrl for S32G platforms

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] The USB PHY node uses a non-generic name 'usbphynop' which violates Devicetree naming conventions.
- [Low] Explicitly setting 'status = "okay";' for the 'usbmisc' node in the board files is redundant.
--

commit 9f52e7dedd0f8d5b7d9ec5b0f3215fe2bb1c921b
Author: Khristine Andreea Barbulescu <khristineandreea.barbulescu@oss.nxp.com>

arm64: dts: Add usbphynop and usbotg pinctrl for S32G platforms

This commit adds the generic USB PHY node and USB OTG pinmux configurations
for the S32G2 and S32G3 SoCs. It enables the USB controller to reference the
generic PHY and use the required pins for USB OTG operations.

> diff --git a/arch/arm64/boot/dts/freescale/s32g2.dtsi b/arch/arm64/boot/dts/freescale/s32g2.dtsi
> index 51d00dac12deb..a35bb284270ed 100644
> --- a/arch/arm64/boot/dts/freescale/s32g2.dtsi
> +++ b/arch/arm64/boot/dts/freescale/s32g2.dtsi
[ ... ]
> @@ -108,6 +108,11 @@ psci {
>  		};
>  	};
>  
> +	usbphynop: usbphynop {

[Severity: Low]
This isn't a bug, but does this node name align with the Devicetree
Specification?

The Devicetree Specification (Section 2.2.2 'Generic Names Recommendation')
states that a node's name should be generic and reflect the function of the
device, rather than its precise programming model or driver name.

Could this be named something more generic like 'phy' or 'usb-phy'
instead of 'usbphynop'?

> +		compatible = "usb-nop-xceiv";
> +		#phy-cells = <0>;
> +	};
> +
>  	soc@0 {
[ ... ]
> diff --git a/arch/arm64/boot/dts/freescale/s32g3.dtsi b/arch/arm64/boot/dts/freescale/s32g3.dtsi
> index e314f3c7d61d0..b980e5f2b059c 100644
> --- a/arch/arm64/boot/dts/freescale/s32g3.dtsi
> +++ b/arch/arm64/boot/dts/freescale/s32g3.dtsi
[ ... ]
> @@ -165,6 +165,11 @@ scmi_shmem: shm@d0000000 {
>  		};
>  	};
>  
> +	usbphynop: usbphynop {

[Severity: Low]
This isn't a bug, but does this node name align with the Devicetree
Specification?

The Devicetree Specification (Section 2.2.2 'Generic Names Recommendation')
states that a node's name should be generic and reflect the function of the
device, rather than its precise programming model or driver name.

Could this be named something more generic like 'phy' or 'usb-phy'
instead of 'usbphynop'?

> +		compatible = "usb-nop-xceiv";
> +		#phy-cells = <0>;
> +	};
> +
>  	soc@0 {
[ ... ]
> diff --git a/arch/arm64/boot/dts/freescale/s32gxxxa-evb.dtsi b/arch/arm64/boot/dts/freescale/s32gxxxa-evb.dtsi
> index 803ff45310771..26009c1e90dcf 100644
> --- a/arch/arm64/boot/dts/freescale/s32gxxxa-evb.dtsi
> +++ b/arch/arm64/boot/dts/freescale/s32gxxxa-evb.dtsi
[ ... ]
> @@ -304,3 +337,14 @@ &spi5 {
>  	pinctrl-names = "default";
>  	status = "okay";
>  };
> +
> +&usbmisc {
> +	status = "okay";
> +};

[Severity: Low]
This isn't a bug, but is it necessary to explicitly set the status for the
usbmisc node here?

In the base SoC devicetrees (s32g2.dtsi and s32g3.dtsi), the usbmisc node
is defined without a 'status = "disabled";' property.

Since nodes default to an 'okay' status when the status property is omitted,
could this assignment be redundant?

> +
> +&usbotg {
[ ... ]
> diff --git a/arch/arm64/boot/dts/freescale/s32gxxxa-rdb.dtsi b/arch/arm64/boot/dts/freescale/s32gxxxa-rdb.dtsi
> index 979868f6d2c5c..a8abb10b0e7ae 100644
> --- a/arch/arm64/boot/dts/freescale/s32gxxxa-rdb.dtsi
> +++ b/arch/arm64/boot/dts/freescale/s32gxxxa-rdb.dtsi
[ ... ]
> @@ -257,3 +290,14 @@ &i2c4 {
>  	pinctrl-1 = <&i2c4_gpio_pins>;
>  	status = "okay";
>  };
> +
> +&usbmisc {
> +	status = "okay";
> +};

[Severity: Low]
This isn't a bug, but is it necessary to explicitly set the status for the
usbmisc node here?

In the base SoC devicetrees (s32g2.dtsi and s32g3.dtsi), the usbmisc node
is defined without a 'status = "disabled";' property.

Since nodes default to an 'okay' status when the status property is omitted,
could this assignment be redundant?

> +
> +&usbotg {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&usbotg_pins>;
> +	phys = <&usbphynop>;
> +	status = "okay";
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520151007.4193688-2-khristineandreea.barbulescu@oss.nxp.com?part=1