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

[Andrew Lunn <andrew@lunn.ch>]

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