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

[Andrew Lunn]

> The driver also exports a debugfs interface for diagnostics,
> exposing PHY status registers and providing runtime control over the
> internal packet generator, CRC corruption, and loopback functionality.

Please drop all this. Many PHYs have this sort of facility. We want
one generic API, probably via ethtool, which all PHYs can follow, not
N different incompatible APIs.

> +static u32 dap8211r_get_rgmii_delay(struct phy_device *phydev, const char *prop_name)
> +{
> +	struct device_node *np = phydev->mdio.dev.of_node;
> +	int ret;
> +	u32 ps = 0;
> +
> +	ret = of_property_read_u32(np, prop_name, &ps);
> +	if (ret) {
> +		phydev_dbg(phydev, "Using default delay (%ups)\n", DAP8211R_DEFAULT_DELAY_PS);
> +		ps = DAP8211R_DEFAULT_DELAY_PS;
> +	}

This can be simplified.

First off, there probably too many phydev_dbg() messages, specially
for normal case things, using defaults, etc. 

of_property_read_u32_array() is documented as not touching the results
value, if it does not find the property. So you can do:

	u32 ps = DAP8211R_DEFAULT_DELAY_PS;
	of_property_read_u32(np, prop_name, &ps);
  
	return dap8211r_delay_ps_to_sel(phydev, ps);

> +static int dap8211r_config_init(struct phy_device *phydev)
> +{
> +	struct device_node *phydev_node = phydev->mdio.dev.of_node;
> +	u16 mask = 0, set = 0;
> +	int ret;
> +
> +	switch (phydev->interface) {
> +	case PHY_INTERFACE_MODE_RGMII:
> +		break;

This is one of the less well defined areas. PHY_INTERFACE_MODE_RGMII
would mean either the PCB is adding the delay, or the MAC. There is no
reason however why the PHY cannot perform fine tuning, adding a small
delay, say 150ps. So you should respect the rx-internal-delay-ps and
tx-internal-delay-ps delays here. 

	Andrew