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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Incorrect initialization order: RGMII delay settings are written after the PHY software reset.
- [Low] Brittle error handling in read_poll_timeout relies on mathematical coincidence of negative error codes.
--

--- Patch [3]: [PATCH net-next v5 3/3] net: phy: add DAPU Telecom DAP8211R(I) Gigabit Ethernet PHY driver ---
commit a53a9ce41047cdc6842142dab15fe0303b0f860a
Author: Artem Shimko <[email protected]>

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

Adds a new PHY driver for the DAPU Telecom DAP8211R(I) Gigabit Ethernet PHY.
The driver implements extended register access for RGMII delay configuration.

> diff --git a/drivers/net/phy/dap8211r.c b/drivers/net/phy/dap8211r.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/net/phy/dap8211r.c

[ ... ]

> +static int dap8211r_config_init(struct phy_device *phydev)
> +{

[ ... ]

> +	set |= FIELD_PREP(DAP8211R_RGMII_RX_DEL_MASK, rx_internal_delay);
> +	set |= FIELD_PREP(DAP8211R_RGMII_TX_DEL_MASK, tx_internal_delay);
> +
> +	ret = dap8211r_modify_ext(phydev, DAP8211R_PHY_CON, DAP8211R_PHY_SW_RST, 0);
> +	if (ret)
> +		return ret;
> +
> +	/* Wait for reset self-clear (from low active to high) */
> +	ret = read_poll_timeout(dap8211r_read_ext, val,
> +				(val & DAP8211R_PHY_SW_RST),

[Severity: Low]
Should this exit condition explicitly check for val < 0?

If dap8211r_read_ext() returns a negative error code, this loop currently
terminates only because negative values in two's complement happen to have
BIT(15) set. Relying on this coincidence might mask errors if a different
register or bit is polled in the future.

> +				20, 200, false, phydev, DAP8211R_PHY_CON);
> +	if (ret)
> +		return ret;
> +	if (val < 0)
> +		return val;
> +
> +	ret = dap8211r_modify_ext(phydev, DAP8211R_RGMII_CON, DAP8211R_RGMII_CONFIG_MASK, set);

[Severity: Medium]
The function's docstring mentions "Software reset to apply settings", but here
the RGMII configuration is written after the reset completes. 

Does this PHY require the settings to be written before the software reset in
order to latch them properly?

> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3