Re: [PATCH] net: phy: add driver for MaxLinear MxL86110 PHY

[Andrew Lunn <andrew@lunn.ch>]

> +/* only 1 page for MXL86110 */
> +#define MXL86110_DEFAULT_PAGE	0

> +/**
> + * mxl86110_read_page - Read current page number
> + * @phydev: Pointer to the PHY device
> + *
> + * Return: The currently selected page number, or negative errno on failure.
> + */
> +static int mxl86110_read_page(struct phy_device *phydev)
> +{
> +	return __phy_read(phydev, MXL86110_EXTD_REG_ADDR_OFFSET);
> +};

If there is only one page, does these make any sense?

> +static int mxl86110_write_page(struct phy_device *phydev, int page)
> +{
> +	return __phy_write(phydev, MXL86110_EXTD_REG_ADDR_OFFSET, page);
> +};
> +
> +/**
> + * mxl86110_write_extended_reg() - write to a PHY's extended register
> + * @phydev: pointer to a &struct phy_device
> + * @regnum: register number to write
> + * @val: value to write to @regnum
> + *
> + * NOTE: This function assumes the caller already holds the MDIO bus lock
> + * or otherwise has exclusive access to the PHY. If exclusive access
> + * cannot be guaranteed, please use mxl86110_locked_write_extended_reg()
> + * which handles locking internally.
> + *
> + * returns 0 or negative error code
> + */
> +static int mxl86110_write_extended_reg(struct phy_device *phydev, u16 regnum, u16 val)
> +{
> +	int ret;
> +
> +	ret = __phy_write(phydev, MXL86110_EXTD_REG_ADDR_OFFSET, regnum);

This is confusing. It looks identical to mxl86110_write_page(). So
regnum is actually page?

> +	if (ret < 0)
> +		return ret;
> +
> +	return __phy_write(phydev, MXL86110_EXTD_REG_ADDR_DATA, val);

And within that page, there is a single register at address
MXL86110_EXTD_REG_ADDR_DATA?

If you keep the write_page() and read_page(), it looks like you can
replace this with

	return phy_write_paged(phydev, regnum,
	                       MXL86110_EXTD_REG_ADDR_DATA, 
                               val);
			       
> +static int mxl86110_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
> +{
> +	struct net_device *netdev;
> +	int page_to_restore;
> +	const u8 *mac;
> +	int ret = 0;
> +
> +	if (wol->wolopts & WAKE_MAGIC) {
> +		netdev = phydev->attached_dev;
> +		if (!netdev)
> +			return -ENODEV;
> +
> +		page_to_restore = phy_select_page(phydev, MXL86110_DEFAULT_PAGE);
> +		if (page_to_restore < 0)
> +			goto error;

If there is only one page, i think this can be removed. And everywhere
else in the driver.

> +	/*
> +	 * RX_CLK delay (RXDLY) enabled via CHIP_CFG register causes a fixed
> +	 * delay of approximately 2 ns at 125 MHz or 8 ns at 25/2.5 MHz.
> +	 * Digital delays in RGMII_CFG1 register are additive
> +	 */
> +	switch (phydev->interface) {
> +	case PHY_INTERFACE_MODE_RGMII:
> +		val = 0;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +		val = MXL86110_EXT_RGMII_CFG1_RX_DELAY_150PS;

This should be 2000ps, or the nearest you can get to it.

> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_TXID:
> +		val = MXL86110_EXT_RGMII_CFG1_TX_1G_DELAY_2250PS |
> +			MXL86110_EXT_RGMII_CFG1_TX_10MB_100MB_DELAY_2250PS;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +		val = MXL86110_EXT_RGMII_CFG1_TX_1G_DELAY_2250PS |
> +			MXL86110_EXT_RGMII_CFG1_TX_10MB_100MB_DELAY_2250PS;
> +		val |= MXL86110_EXT_RGMII_CFG1_RX_DELAY_150PS;

Same here.

> +		break;
> +	default:
> +		ret = -EINVAL;
> +		goto err_restore_page;
> +	}
> +	ret = mxl86110_modify_extended_reg(phydev, MXL86110_EXT_RGMII_CFG1_REG,
> +					   MXL86110_EXT_RGMII_CFG1_FULL_MASK, val);
> +	if (ret < 0)
> +		goto err_restore_page;
> +
> +	/* Configure RXDLY (RGMII Rx Clock Delay) to keep the default
> +	 * delay value on RX_CLK (2 ns for 125 MHz, 8 ns for 25 MHz/2.5 MHz)
> +	 */
> +	ret = mxl86110_modify_extended_reg(phydev, MXL86110_EXT_CHIP_CFG_REG,
> +					   MXL86110_EXT_CHIP_CFG_RXDLY_ENABLE, 1);

So does the value 1 here mean 8ns? 0 would be 2ns?

    Andrew

---
pw-bot: cr