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

[Andrew Lunn]

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

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

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

	Andrew