Re: [PATCH net-next v2 3/4] net: phy: air_phy_lib: Factorize BuckPBus register accessors

Re: [PATCH net-next v2 3/4] net: phy: air_phy_lib: Factorize BuckPBus register accessors

[Louis-Alexis Eyraud]

Hi Andrew,

On Thu, 2026-03-26 at 13:30 +0100, Andrew Lunn wrote:
> > @@ -480,8 +287,8 @@ static int en8811h_wait_mcu_ready(struct
> > phy_device *phydev)
> >  {
> >  	int ret, reg_value;
> >  
> > -	ret = air_buckpbus_reg_write(phydev, EN8811H_FW_CTRL_1,
> > -				     EN8811H_FW_CTRL_1_FINISH);
> > +	ret = air_phy_buckpbus_reg_write(phydev,
> > EN8811H_FW_CTRL_1,
> > +					
> > EN8811H_FW_CTRL_1_FINISH);
> 
> Is a rename required? Is the namespace air_buckpbus_ used somewhere
> else?
> 
> 	Andrew
Sorry for the delay.

The air_buckpbus_ namespace is only used in the air_en8811h driver.
It seemed better to me that in the new air_phy_lib, all functions (the
buckpbus accessors and air_phy_read/write_page functions) started with
the same prefix. That is the reason I renamed them, even if not
required.

As an alternative, to avoid renaming those buckpbus function calls on
air_en8811h driver and reduce this patch changes, I can add macros at
the beginning of the file such as:
```
#define air_buckpbus_reg_write(_phydev, _pbus_address, _pbus_data) \
	air_phy_buckpbus_reg_write(_phydev, _pbus_address, _pbus_data)
```

Would it be okay?

Best regards,
Louis-Alexis

Re: [PATCH net-next v2 3/4] net: phy: air_phy_lib: Factorize BuckPBus register accessors

[Andrew Lunn]

On Thu, May 07, 2026 at 02:11:54PM +0200, Louis-Alexis Eyraud wrote:
> Hi Andrew,
> 
> On Thu, 2026-03-26 at 13:30 +0100, Andrew Lunn wrote:
> > > @@ -480,8 +287,8 @@ static int en8811h_wait_mcu_ready(struct
> > > phy_device *phydev)
> > >  {
> > >  	int ret, reg_value;
> > >  
> > > -	ret = air_buckpbus_reg_write(phydev, EN8811H_FW_CTRL_1,
> > > -				     EN8811H_FW_CTRL_1_FINISH);
> > > +	ret = air_phy_buckpbus_reg_write(phydev,
> > > EN8811H_FW_CTRL_1,
> > > +					
> > > EN8811H_FW_CTRL_1_FINISH);
> > 
> > Is a rename required? Is the namespace air_buckpbus_ used somewhere
> > else?
> > 
> > 	Andrew
> Sorry for the delay.
> 
> The air_buckpbus_ namespace is only used in the air_en8811h driver.
> It seemed better to me that in the new air_phy_lib, all functions (the
> buckpbus accessors and air_phy_read/write_page functions) started with
> the same prefix. That is the reason I renamed them, even if not
> required.
> 
> As an alternative, to avoid renaming those buckpbus function calls on
> air_en8811h driver and reduce this patch changes, I can add macros at
> the beginning of the file such as:
> ```
> #define air_buckpbus_reg_write(_phydev, _pbus_address, _pbus_data) \
> 	air_phy_buckpbus_reg_write(_phydev, _pbus_address, _pbus_data)

No don't do this.

If you want to rename them, rename them. But do it in a patch which
only contains a rename. That is easier to review, and more obviously
correct.

	Andrew

Re: [PATCH net-next v2 3/4] net: phy: air_phy_lib: Factorize BuckPBus register accessors

[Louis-Alexis Eyraud]

Hi Andrew,

On Thu, 2026-05-07 at 23:36 +0200, Andrew Lunn wrote:
> On Thu, May 07, 2026 at 02:11:54PM +0200, Louis-Alexis Eyraud wrote:
> > Hi Andrew,
> > 
> > On Thu, 2026-03-26 at 13:30 +0100, Andrew Lunn wrote:
> > > > @@ -480,8 +287,8 @@ static int en8811h_wait_mcu_ready(struct
> > > > phy_device *phydev)
> > > >  {
> > > >  	int ret, reg_value;
> > > >  
> > > > -	ret = air_buckpbus_reg_write(phydev,
> > > > EN8811H_FW_CTRL_1,
> > > > -				    
> > > > EN8811H_FW_CTRL_1_FINISH);
> > > > +	ret = air_phy_buckpbus_reg_write(phydev,
> > > > EN8811H_FW_CTRL_1,
> > > > +					
> > > > EN8811H_FW_CTRL_1_FINISH);
> > > 
> > > Is a rename required? Is the namespace air_buckpbus_ used
> > > somewhere
> > > else?
> > > 
> > > 	Andrew
> > Sorry for the delay.
> > 
> > The air_buckpbus_ namespace is only used in the air_en8811h driver.
> > It seemed better to me that in the new air_phy_lib, all functions
> > (the
> > buckpbus accessors and air_phy_read/write_page functions) started
> > with
> > the same prefix. That is the reason I renamed them, even if not
> > required.
> > 
> > As an alternative, to avoid renaming those buckpbus function calls
> > on
> > air_en8811h driver and reduce this patch changes, I can add macros
> > at
> > the beginning of the file such as:
> > ```
> > #define air_buckpbus_reg_write(_phydev, _pbus_address, _pbus_data)
> > \
> > 	air_phy_buckpbus_reg_write(_phydev, _pbus_address,
> > _pbus_data)
> 
> No don't do this.
> 
> If you want to rename them, rename them. But do it in a patch which
> only contains a rename. That is easier to review, and more obviously
> correct.
I'll add in v3 a new patch to rename the functions and the calls in
air_en8811h.c then.

Best regards,
Louis-Alexis

> 
> 	Andrew

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

[Louis-Alexis Eyraud]

Hi Andrew,

On Thu, 2026-03-26 at 13:47 +0100, Andrew Lunn wrote:
> > +static int an8801r_led_blink_set(struct phy_device *phydev, u8
> > index,
> > +				 unsigned long *delay_on,
> > +				 unsigned long *delay_off)
> > +{
> 
> ...
> 
> > +	ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2,
> > LED_ON_CTRL(index),
> > +			     LED_ON_EN, blink ? LED_ON_EN : 0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> 
> Just
> 
> 
> 	return phy_modify_mmd(phydev, MDIO_MMD_VEND2,
> LED_ON_CTRL(index),
> 			     LED_ON_EN, blink ? LED_ON_EN : 0);
> 
> > +		if (!led_trigger)
> > +			continue;
> > +
> > +		ret = an8801r_led_hw_control_set(phydev, led_id,
> > led_trigger);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +	return 0;
> > +}
> 
> 
> Please take a look at all your functions. Can the last error check be
> removed and just use return ret, etc.
I'll fix this in the next version.

> 
> > +static int an8801r_of_init_leds(struct phy_device *phydev, u8
> > *led_cfg)
> > +{
> > +	struct device *dev = &phydev->mdio.dev;
> > +	struct device_node *np = dev->of_node;
> > +	struct device_node *leds;
> > +	u32 function_enum_idx;
> > +	int ret;
> > +
> > +	if (!np)
> > +		return 0;
> > +
> > +	/* If devicetree is present, leds configuration is
> > required */
> > +	leds = of_get_child_by_name(np, "leds");
> > +	if (!leds)
> > +		return 0;
> > +
> > +	for_each_available_child_of_node_scoped(leds, led) {
> > +		u32 led_idx;
> > +
> > +		ret = of_property_read_u32(led, "reg", &led_idx);
> > +		if (ret)
> > +			goto out;
> > +
> > +		if (led_idx >= AN8801R_NUM_LEDS) {
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> > +
> > +		ret = of_property_read_u32(led, "function-
> > enumerator",
> > +					   &function_enum_idx);
> > +		if (ret)
> > +			function_enum_idx = AN8801R_LED_FN_NONE;
> > +
> 
> What is this doing? Is this documented in the binding?
The `function-enumerator` property is only documented in the led common
dt-binding file. The an8801 dt-bindings inherits this property from the
ethernet-phy dt-bindings.

We aimed to have this PHY have its led behaviour (how many to enable
and what their role shall be) configurable using devicetree and not to
rely on a default configuration, hard-coded in the driver (like the
air_en8811h driver did) and also make use of the led hardware
offloading (for functions like 100/1000, activity blinking, and others)
that this PHY is capable of.

From the available property list for the led node, this one seems to be
appropriate to distinguish between the possible LAN functions, that 
would mean that a specific LED has either a link or RX/Tx activity 
role. That is why we used it but we could be wrong.

The an8801 dt-bindings (in patch 1) misses the possible values and
should improved in that regard and I'll fix them in next version if
this implementation seems acceptable to you.
> 
> > +		if (function_enum_idx >= AN8801R_LED_FN_MAX) {
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> > +
> > +		led_cfg[led_idx] = function_enum_idx;
> > +	}
> > +out:
> > +	of_node_put(leds);
> > +	return ret;
> > +}
> 
> > +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;
> > +
> > +	if (phydev->link && prev_speed != phydev->speed) {
> > +		val = phydev->speed == SPEED_1000 ?
> > +		      AN8801_BPBUS_LINK_MODE_1000 : 0;
> > +
> > +		return an8801_buckpbus_reg_rmw(phydev,
> > +					      
> > AN8801_BPBUS_REG_LINK_MODE,
> > +					      
> > AN8801_BPBUS_LINK_MODE_1000,
> > +					       val);
> > +	};
> 
> This is unusual. What is it doing? Please add a comment.
This call is to ensure that the PHY switches to the expected 1Gbps 
speed when available. 
I'll confirm it and add a comment in v3.

Best regards,
Louis-Alexis
> 
> 	Andrew

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

[Andrew Lunn]

> > > +static int an8801r_of_init_leds(struct phy_device *phydev, u8
> > > *led_cfg)
> > > +{
> > > +	struct device *dev = &phydev->mdio.dev;
> > > +	struct device_node *np = dev->of_node;
> > > +	struct device_node *leds;
> > > +	u32 function_enum_idx;
> > > +	int ret;
> > > +
> > > +	if (!np)
> > > +		return 0;
> > > +
> > > +	/* If devicetree is present, leds configuration is
> > > required */
> > > +	leds = of_get_child_by_name(np, "leds");
> > > +	if (!leds)
> > > +		return 0;
> > > +
> > > +	for_each_available_child_of_node_scoped(leds, led) {
> > > +		u32 led_idx;
> > > +
> > > +		ret = of_property_read_u32(led, "reg", &led_idx);
> > > +		if (ret)
> > > +			goto out;
> > > +
> > > +		if (led_idx >= AN8801R_NUM_LEDS) {
> > > +			ret = -EINVAL;
> > > +			goto out;
> > > +		}
> > > +
> > > +		ret = of_property_read_u32(led, "function-
> > > enumerator",
> > > +					   &function_enum_idx);
> > > +		if (ret)
> > > +			function_enum_idx = AN8801R_LED_FN_NONE;
> > > +
> > 
> > What is this doing? Is this documented in the binding?
> The `function-enumerator` property is only documented in the led common
> dt-binding file. The an8801 dt-bindings inherits this property from the
> ethernet-phy dt-bindings.
> 
> We aimed to have this PHY have its led behaviour (how many to enable
> and what their role shall be) configurable using devicetree and not to
> rely on a default configuration, hard-coded in the driver (like the
> air_en8811h driver did) and also make use of the led hardware
> offloading (for functions like 100/1000, activity blinking, and others)
> that this PHY is capable of.

What other drivers do is leave the configuration with its reset
default. They are often sensible. When the netdev trigger loads, it
should ask the LED how it is configured, and the values in sysfs will
reflect it. After that you can change it, via udev rules, etc.

You have to be careful about what you put in DT. DT describes
hardware, not configuration or policy. How the LED blinks is probably
configuration, so it does not belong in DT.

> > > +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;
> > > +
> > > +	if (phydev->link && prev_speed != phydev->speed) {
> > > +		val = phydev->speed == SPEED_1000 ?
> > > +		      AN8801_BPBUS_LINK_MODE_1000 : 0;
> > > +
> > > +		return an8801_buckpbus_reg_rmw(phydev,
> > > +					      
> > > AN8801_BPBUS_REG_LINK_MODE,
> > > +					      
> > > AN8801_BPBUS_LINK_MODE_1000,
> > > +					       val);
> > > +	};
> > 
> > This is unusual. What is it doing? Please add a comment.
> This call is to ensure that the PHY switches to the expected 1Gbps 
> speed when available. 

So this is an errata workaround? Please add this in a patch of its
own, described the problem in the commit message, list the errata etc.

     Andrew

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

[Louis-Alexis Eyraud]

Hi Andrew,

On Thu, 2026-05-07 at 23:43 +0200, Andrew Lunn wrote:
> > > > +static int an8801r_of_init_leds(struct phy_device *phydev, u8
> > > > *led_cfg)
> > > > +{
> > > > +	struct device *dev = &phydev->mdio.dev;
> > > > +	struct device_node *np = dev->of_node;
> > > > +	struct device_node *leds;
> > > > +	u32 function_enum_idx;
> > > > +	int ret;
> > > > +
> > > > +	if (!np)
> > > > +		return 0;
> > > > +
> > > > +	/* If devicetree is present, leds configuration is
> > > > required */
> > > > +	leds = of_get_child_by_name(np, "leds");
> > > > +	if (!leds)
> > > > +		return 0;
> > > > +
> > > > +	for_each_available_child_of_node_scoped(leds, led) {
> > > > +		u32 led_idx;
> > > > +
> > > > +		ret = of_property_read_u32(led, "reg",
> > > > &led_idx);
> > > > +		if (ret)
> > > > +			goto out;
> > > > +
> > > > +		if (led_idx >= AN8801R_NUM_LEDS) {
> > > > +			ret = -EINVAL;
> > > > +			goto out;
> > > > +		}
> > > > +
> > > > +		ret = of_property_read_u32(led, "function-
> > > > enumerator",
> > > > +					  
> > > > &function_enum_idx);
> > > > +		if (ret)
> > > > +			function_enum_idx =
> > > > AN8801R_LED_FN_NONE;
> > > > +
> > > 
> > > What is this doing? Is this documented in the binding?
> > The `function-enumerator` property is only documented in the led
> > common
> > dt-binding file. The an8801 dt-bindings inherits this property from
> > the
> > ethernet-phy dt-bindings.
> > 
> > We aimed to have this PHY have its led behaviour (how many to
> > enable
> > and what their role shall be) configurable using devicetree and not
> > to
> > rely on a default configuration, hard-coded in the driver (like the
> > air_en8811h driver did) and also make use of the led hardware
> > offloading (for functions like 100/1000, activity blinking, and
> > others)
> > that this PHY is capable of.
> 
> What other drivers do is leave the configuration with its reset
> default. They are often sensible. When the netdev trigger loads, it
> should ask the LED how it is configured, and the values in sysfs will
> reflect it. After that you can change it, via udev rules, etc.
When you say "reset default", do you mean the default PHY register
values, the ones that may have been set by the bootloader or by the
driver with a default hardcoded functional config?
> 
> You have to be careful about what you put in DT. DT describes
> hardware, not configuration or policy. How the LED blinks is probably
> configuration, so it does not belong in DT.
I agree.
What I meant as configuration was only the leds node presence and the
led function properties, as the devicetree should describe what/how
many LED are connected to the PHY and they represent (or mean) for a
given board.
Parameters like off/on delay or trigger events are indeed not really
appropriate for devicetree and the AN8801 dt-bindings patch do not add
any such property.

And even, if technically the LEDs are reconfigurable with netdev or any
other LED trigger, the configuration is somehow hardware bound, because
the different colors of LEDs do kind-of bind a specific function to a
specific LED (amber vs green).

This is why the an8801r_of_init_leds function read those LED-related
properties (leds/led/function-enumerator) to set a default led config
behaviour (that can of course be overridden by an user with a led
trigger) rather than relying on bootloader or use a hardcoded led
register config in the driver.

What implementation would be preferred for this driver?

> 
> > > > +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;
> > > > +
> > > > +	if (phydev->link && prev_speed != phydev->speed) {
> > > > +		val = phydev->speed == SPEED_1000 ?
> > > > +		      AN8801_BPBUS_LINK_MODE_1000 : 0;
> > > > +
> > > > +		return an8801_buckpbus_reg_rmw(phydev,
> > > > +					      
> > > > AN8801_BPBUS_REG_LINK_MODE,
> > > > +					      
> > > > AN8801_BPBUS_LINK_MODE_1000,
> > > > +					       val);
> > > > +	};
> > > 
> > > This is unusual. What is it doing? Please add a comment.
> > This call is to ensure that the PHY switches to the expected 1Gbps 
> > speed when available. 
> 
> So this is an errata workaround? Please add this in a patch of its
> own, described the problem in the commit message, list the errata
> etc.
> 
OK, I'll add this in a separate patch in v3.

Best regards,
Louis-Alexis

>      Andrew