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

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

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@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@collabora.com>
> Signed-off-by: Louis-Alexis Eyraud <louisalexis.eyraud@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 :)

> +
> +	/* 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()

> +
> +	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 ?

> +
> +		 /* 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 :)

Thanks,

Maxime