[PATCH net-next v3 5/6] net: phy: Introduce Airoha AN8801/R Gigabit Ethernet PHY driver

[Louis-Alexis Eyraud]

Hi Maxime,

On Tue, 2026-05-12 at 12:06 +0200, Maxime Chevallier wrote:
> Hi :)
> 
> This looks good, I just have very minimal comments
> 
> On 5/12/26 06:33, Louis-Alexis Eyraud wrote:
> > From: AngeloGioacchino Del Regno
> > <angelogioacchino.delregno at collabora.com>
> > 
> > Introduce a driver for the Airoha AN8801R Series Gigabit Ethernet
> > PHY; this currently supports setting up PHY LEDs, 10/100M, 1000M
> > speeds, and Wake on LAN and PHY interrupts.
> > 
> > Signed-off-by: AngeloGioacchino Del Regno
> > <angelogioacchino.delregno at collabora.com>
> > Signed-off-by: Louis-Alexis Eyraud
> > <louisalexis.eyraud at collabora.com>
> 
> [...]
> 
> > +static u32 an8801r_led_blink_ms_to_hw(unsigned long req_ms)
> > +{
> > +	u32 req_ns, regval;
> > +
> > +	if (req_ms > AN8801_MAX_PERIOD_MS)
> > +		req_ms = AN8801_MAX_PERIOD_MS;
> > +
> > +	req_ns = req_ms * 1000000;
> 
> Use NSEC_PER_MSEC :)
I'll fix this in the next version.

> 
> > +
> > +	/* Round to the nearest period unit... */
> > +	regval = req_ns + (AN8801_PERIOD_UNIT / 2);
> > +
> > +	/* ...and now divide by the full period */
> > +	regval >>= AN8801_PERIOD_SHIFT;
> > +
> > +	return regval;
> > +}
> > +
> 
> [...]
> 
> > +static int an8801r_led_hw_control_set(struct phy_device *phydev,
> > u8 index,
> > +				      unsigned long rules)
> > +{
> > +	u16 on = 0, blink = 0;
> > +	int ret;
> > +
> > +	if (index >= AN8801R_NUM_LEDS)
> > +		return -EINVAL;
> > +
> > +	ret = an8801r_led_trig_to_hw(rules, &on, &blink);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2,
> > LED_ON_CTRL(index),
> > +			     LED_ON_EVT_MASK, on);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2,
> > LED_BLINK_CTRL(index),
> > +			     LED_BLINK_EVT_MASK, blink);
> > +
> > +	if (ret)
> > +		return ret;
> 
> Extra newline before the if()
I'll fix this in the next version too.

> 
> > +
> > +	return phy_modify_mmd(phydev, MDIO_MMD_VEND2,
> > LED_ON_CTRL(index),
> > +			      LED_ON_EN, on | blink ? LED_ON_EN :
> > 0);
> > +}
> > +
> 
> [...]
> 
> > +static int an8801r_rgmii_rxdelay(struct phy_device *phydev, bool
> > enable,
> > +				 u16 delay_steps)
> > +{
> > +	u32 reg_val;
> > +
> > +	if (delay_steps > RGMII_DELAY_STEP_MASK)
> > +		return -EINVAL;
> > +
> > +	if (enable) {
> > +		reg_val = delay_steps & RGMII_DELAY_STEP_MASK;
> > +
> > +		 /* Set align bit to add extra offset for RX delay
> > */
> > +		reg_val |= RGMII_RXDELAY_ALIGN;
> > +
> > +		 /* Set force mode bit to enable RX delay
> > insertion */
> > +		reg_val |= RGMII_RXDELAY_FORCE_MODE;
> > +	} else {
> > +		reg_val = 0;
> > +	}
> > +
> > +	return an8801_buckpbus_reg_write(phydev,
> > AN8801_BPBUS_REG_RXDLY_STEP,
> > +					 reg_val);
> > +}
> > +
> > +static int an8801r_rgmii_txdelay(struct phy_device *phydev, bool
> > enable,
> > +				 u16 delay_steps)
> > +{
> > +	u32 reg_val;
> > +
> > +	if (delay_steps > RGMII_DELAY_STEP_MASK)
> > +		return -EINVAL;
> > +
> > +	if (enable) {
> > +		reg_val = delay_steps & RGMII_DELAY_STEP_MASK;
> 
> Is this bitwise and needed, as you have the check above ?
Indeed, it is not needed, so I'll remove this masking operation here
and in an8801r_rgmii_rxdelay too.

> 
> > +
> > +		 /* Set force mode bit to enable TX delay
> > insertion */
> > +		reg_val |= RGMII_TXDELAY_FORCE_MODE;
> > +	} else {
> > +		reg_val = 0;
> > +	}
> > +
> > +	return an8801_buckpbus_reg_write(phydev,
> > AN8801_BPBUS_REG_TXDLY_STEP,
> > +					 reg_val);
> > +}
> > +
> > +static int an8801r_rgmii_delay_config(struct phy_device *phydev)
> > +{
> > +	bool enable_delay;
> > +	u16 delay_step;
> > +	int ret;
> > +
> > +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > +	    phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
> > +		enable_delay = true;
> > +		delay_step = AN8801_RGMII_TXDELAY_DEFAULT;
> > +	} else {
> > +		enable_delay = false;
> > +		delay_step = RGMII_DELAY_NO_STEP;
> > +	}
> > +
> > +	ret = an8801r_rgmii_txdelay(phydev, enable_delay,
> > delay_step);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > +	    phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
> > +		enable_delay = true;
> > +		delay_step = AN8801_RGMII_RXDELAY_DEFAULT;
> 
> Is it correct that AN8801_RGMII_RXDELAY_DEFAULT expands to 
> RGMII_DELAY_NO_STEP ? feels strange, but it may simply be how the HW
> is 
> made :)
As I replied in an earlier comment ([1]), for the inserted RX delay,
when the RGMII_RXDELAY_ALIGN bit is set, the "no step" value is then
the closest setting to the 2ns value.

I'll improve the AN8801_RGMII_RXDELAY_DEFAULT comment to say that it
corresponds to the 1.992ns delay value when the align bit is set and -
0.008ns otherwise.

[1]:https://lore.kernel.org/linux-mediatek/199e674cfdbccad104db761964611b1d6352f9f3.camel@collabora.com/

Best regards,
Louis-Alexis
> 
> Thanks,
> 
> Maxime