Re: [net-next,PATCH] net: phy: realtek: Add support for PHY LEDs on RTL8211F

[Daniel Golle <daniel@makrotopia.org>]

On Mon, Jun 24, 2024 at 01:40:33AM +0200, Marek Vasut wrote:
> Realtek RTL8211F Ethernet PHY supports 3 LED pins which are used to
> indicate link status and activity. Add minimal LED controller driver
> supporting the most common uses with the 'netdev' trigger.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

> [...]
> +static int rtl8211f_led_hw_is_supported(struct phy_device *phydev, u8 index,
> +					unsigned long rules)
> +{
> +	const unsigned long mask = BIT(TRIGGER_NETDEV_LINK_10) |
> +				   BIT(TRIGGER_NETDEV_LINK_100) |
> +				   BIT(TRIGGER_NETDEV_LINK_1000) |
> +				   BIT(TRIGGER_NETDEV_RX) |
> +				   BIT(TRIGGER_NETDEV_TX);
> +
> +	/* The RTL8211F PHY supports these LED settings on up to three LEDs:
> +	 * - Link: Configurable subset of 10/100/1000 link rates
> +	 * - Active: Blink on activity, RX or TX is not differentiated
> +	 * The Active option has two modes, A and B:
> +	 * - A: Link and Active indication at configurable, but matching,
> +	 *      subset of 10/100/1000 link rates
> +	 * - B: Link indication at configurable subset of 10/100/1000 link
> +	 *      rates and Active indication always at all three 10+100+1000
> +	 *      link rates.
> +	 * This code currently uses mode B only.
> +	 */
> +
> +	if (index >= RTL8211F_LED_COUNT)
> +		return -EINVAL;
> +
> +	/* Filter out any other unsupported triggers. */
> +	if (rules & ~mask)
> +		return -EOPNOTSUPP;

It looks like it is not possible to let the hardware indicate only either
RX or TX, it will always have to go together.

Please express this in this function accordingly, so fallback to
software-driven trigger works as expected.

Example:
if (!(rules & BIT(TRIGGER_NETDEV_RX)) ^ !(rules & BIT(TRIGGER_NETDEV_TX)))
	return -EOPNOTSUPP;

> +
> +	return 0;
> +}