Re: [PATCH net-next 2/4] net: phy: Add Allwinner AC300 EPHY driver

[Andrew Lunn <andrew@lunn.ch>]

> +static int ac300_phy_config_init(struct phy_device *phydev)
> +{
> +	struct ac300_phy_priv *priv = phydev->priv;
> +	int ret;
> +
> +	/* Write page-based EPHY transceiver and signal path optimizations */
> +	ret = phy_write(phydev, 0x1f, 0x0100); /* Switch to Page 1 */
> +	if (ret)
> +		return ret;
> +	ret = phy_write(phydev, 0x12, 0x4824); /* Disable APS */
> +	if (ret)
> +		return ret;
> +
> +	ret = phy_write(phydev, 0x1f, 0x0200); /* Switch to Page 2 */
> +	if (ret)

Please implement .read_page and .write.page in struct phy_driver.

> +	/* Disable 802.3az EEE */
> +	ret = phy_write(phydev, 0x1f, 0x0200); /* Switch to page 2 */
> +	if (ret)
> +		return ret;

Is it broken? phy_disable_eee()?

> +	return phy_clear_bits_mmd(phydev, 0x7, 0x3c, BIT(1));

MDIO_MMD_AN, MDIO_AN_EEE_ADV.

With those magic numbers replaced with macros, i then have to ask what
is going on with EEE.

I also hope you see why magic numbers are bad and should where ever
possible be replaced to meaningful macros.

> +	/* Read calibration data from NVMEM/SID */
> +	ret = nvmem_cell_read_u16(dev, "calibration", &priv->caldata);
> +	if (ret) {
> +		if (ret == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +		dev_warn(dev, "Failed to read EPHY calibration (%pe)\n", ERR_PTR(ret));

Take a look at dev_err_probe().

> +static int ac300_phy_match_phy_device(struct phy_device *phydev,
> +				      const struct phy_driver *phydrv)
> +{
> +	return of_device_is_compatible(phydev->mdio.dev.of_node,
> +				       "allwinner,sun50i-h618-ac300-ephy");
> +}

Why?

	Andrew