Re: [PATCH net-next v4 2/2] net: phy: add DAPU Telecom DAP8211R(I) Gigabit Ethernet PHY driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The Kconfig help text claims support for device tree flags enabling a 180-degree TX clock phase shift and an internal packet generator, but the driver implements neither.
- [Medium] Inconsistent handling of RGMII delay device tree properties relative to the selected phy-mode.
--

--- Patch [2]: [PATCH net-next v4 2/2] net: phy: add DAPU Telecom DAP8211R(I) Gigabit Ethernet PHY driver ---
commit 8189e886a8887b076e59fb9f9e71196f6089538d
Author: Artem Shimko <a.shimko.dev@gmail.com>

net: phy: add DAPU Telecom DAP8211R(I) Gigabit Ethernet PHY driver

This commit introduces a new PHY driver for the DAPU Telecom DAP8211R(I)
Gigabit Ethernet PHY. It implements extended register access via indirect
addressing and configures RGMII TX/RX clock delays through device tree
properties.

> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index a29d3fed8a053..aab97e01899c5 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
[ ... ]
> @@ -243,6 +243,16 @@ config DAVICOM_PHY
>  	help
>  	  Currently supports dm9161e and dm9131
>  
> +config DAP8211R_PHY
> +	tristate "DAPU Telecom DAP8211R(I) Gigabit Ethernet PHY"
> +	depends on OF
> +	help
> +	  Support for the DAPU Telecom DAP8211R(I) Gigabit Ethernet PHY.
> +	  This PHY is designed for enterprise and industrial networking
> +	  applications, supporting 10/100/1000 Mbps operation.
> +	  RGMII with: configurable TX/RX clock delays, optional flag to enable
> +	  180-degree TX clock phase shift and internal packet generator.

[Severity: Medium]
Are the 180-degree TX clock phase shift and internal packet generator features
actually implemented in this driver?

The help text advertises these features for users configuring the kernel, but
looking at the driver code, there doesn't seem to be any implementation for
them. Could this mislead users who expect these features to be available based
on the documentation?

> +
>  config ICPLUS_PHY
>  	tristate "ICPlus PHYs"
>  	help
> diff --git a/drivers/net/phy/dap8211r.c b/drivers/net/phy/dap8211r.c
> new file mode 100644
> index 0000000000000..65d6fef86f26b
> --- /dev/null
> +++ b/drivers/net/phy/dap8211r.c
[ ... ]
> +static int dap8211r_config_init(struct phy_device *phydev)
> +{
[ ... ]
> +	switch (phydev->interface) {
> +	case PHY_INTERFACE_MODE_RGMII:
> +		if (rx_internal_delay < 0)
> +			rx_internal_delay = 0;
> +
> +		if (tx_internal_delay < 0)
> +			tx_internal_delay = 0;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +		if (rx_internal_delay < 0)
> +			rx_internal_delay = DAP8211R_DEFAULT_DEL_SEL;
> +
> +		tx_internal_delay = 0;

[Severity: Medium]
Is it intentional to silently drop an explicitly provided tx-internal-delay-ps
property when using PHY_INTERFACE_MODE_RGMII_RXID?

For PHY_INTERFACE_MODE_RGMII, explicit delay properties are kept and applied.
However, in dap8211r_config_init(), tx_internal_delay is hardcoded to 0 for
the rgmii-rxid mode. 

This might lead to unexpected configuration drops if a device tree specifies
phy-mode = "rgmii-rxid" along with a tx-internal-delay-ps property, as the
requested delay will be ignored without warning.

> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_ID:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727075324.3555446-1-a.shimko.dev@gmail.com?part=2