Re: [PATCH net-next 2/2] net: phy: Introduce Airoha AN8801/R Gigabit Ethernet PHY driver

[Andrew Lunn]

> +static int an8801r_did_interrupt(struct phy_device *phydev)
> +{
> +	u32 irq_en, irq_status;
> +	int ret;
> +
> +	ret = air_buckpbus_reg_read(phydev, AN8801_BPBUS_REG_WAKE_IRQ_EN,
> +				    &irq_en);
> +	if (ret)
> +		return ret;
> +
> +	ret = air_buckpbus_reg_read(phydev, AN8801_BPBUS_REG_WAKE_IRQ_STS,
> +				    &irq_status);
> +	if (ret)
> +		return ret;
> +
> +	if (irq_status & AN8801_IRQ_WAKE_MAGICPKT)
> +		return 0;

With a name like an8801r_did_interrupt() you would expect the return
value to be some value of True, if there was an interrupt. I would
suggest either a different name, or return 1. Maybe also add a
kerneldoc header indicating the return values, since it is probably
not going to be standard.

> +static void an8801r_get_wol(struct phy_device *phydev,
> +			    struct ethtool_wolinfo *wol)
> +{
> +	u32 reg_val;
> +
> +	air_buckpbus_reg_read(phydev, AN8801_BPBUS_REG_WAKEUP_CTL1, &reg_val);
> +
> +	wol->supported = WAKE_MAGIC;

How does WoL work on this device. Only via interrupts? If so, maybe
you should only return WAKE_MAGIC as supported if there is a valid
interrupt?

> +static int an8801r_rgmii_delay_config(struct phy_device *phydev)
> +{
> +	switch (phydev->interface) {
> +	case PHY_INTERFACE_MODE_RGMII_TXID:
> +		return an8801r_rgmii_txdelay(phydev, 4);
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +		return an8801r_rgmii_rxdelay(phydev, 0);
> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +		return an8801r_rgmii_txdelay(phydev, 4);
> +		return an8801r_rgmii_rxdelay(phydev, 0);

The parameters look very odd here. 4 means 2ns, but 0 also means 0ns?
Can this API be improved?

Also, PHY_INTERFACE_MODE_RGMII_TXID means 2ns delay for TX, but it
also means 0ns delay for RX. The code appears to be missing this
second part.


> +	case PHY_INTERFACE_MODE_RGMII:

And here you should be disabling all delays. We have seen boards where
the strapping is wrong, the PHY boots in RGMII_ID, but RGMII is
required, and so the driver must fully implement
PHY_INTERFACE_MODE_RGMII disabling the delays.

> +static int an8801r_config_init(struct phy_device *phydev)
> +{
> +	u8 led_default_function[AN8801R_NUM_LEDS] = { 0 };
> +	int prev_page, ret;
> +
> +	ret = an8801r_of_init_leds(phydev, led_default_function);
> +	if (ret)
> +		return ret;
> +
> +	/* Disable Low Power Mode (LPM) */
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, AN8801_REG_PHY_INTERNAL0,
> +			    FIELD_PREP(AN8801_PHY_INTFUNC_MASK, 0x1e));
> +	if (ret)
> +		return ret;
> +
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, AN8801_REG_PHY_INTERNAL1,
> +			    FIELD_PREP(AN8801_PHY_INTFUNC_MASK, 0x2));
> +	if (ret)
> +		return ret;
> +
> +	/* Disable EEE by default */
> +	ret = phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV, 0);
> +	if (ret)
> +		return ret;

Why? If EEE is broken, this is not sufficient to stop a user
re-enabling it.

> +static int an8801r_read_status(struct phy_device *phydev)
> +{
> +	int prev_speed, ret;
> +	u32 val;
> +
> +	prev_speed = phydev->speed;
> +
> +	ret = genphy_read_status(phydev);
> +	if (ret)
> +		return ret;

You configure the PHY to support downshift. If it has performed a
downshift, does it report the actual speed in the usual registers read
by genphy_read_status(), or is it necessary to read a vendor register?

> +static struct phy_driver airoha_driver[] = {
> +{
> +	PHY_ID_MATCH_MODEL(AN8801R_PHY_ID),
> +	.name			= "Airoha AN8801R",
> +	.features		= PHY_GBIT_FEATURES,

Should not be needed, if the PHY enumerates its capabilities
correctly.

    Andrew

---
pw-bot: cr