Re: [PATCH net-next 3/3] net: eth: fbnic: Add led support

[Mike Marciniszyn <mike.marciniszyn@gmail.com>]

On Wed, May 20, 2026 at 10:37:01PM +0200, Andrew Lunn wrote:
> > +/**
> > + * fbnic_set_phys_id - Used to strobe the MAC LEDs in a recognizable pattern
> > + * @netdev: Interface/port to strobe the LEDs for
> > + * @phys_id_state: State requested by the call
> > + *
> > + * This function can really be broken down into two parts. There are the
> > + * ACTIVE/INACTIVE states which really are meant to be defining the start
> > + * and stop of the LED strobing. There is also the ON/OFF states which are
> > + * used to provide us with a way of telling us that we should be turning
> > + * the LED on and/or off.
> > + *
> > + * We translate these calls and pass them off to the MAC layer. They will
> > + * be used to initialize a strobe, then on and off will be used to cycle
> > + * between the patterns, and finally we will restore the original LED state.
> > + *
> > + * We will return 2 when we are requested to go active. This will tell the
> > + * call that it will need to call back to turn on/off the LED twice every
> > + * second.
> > + *
> > + * Return: blink in half second intervals on success, negative value on failure
> > + */
>
> Could you drop this for the moment. Ideally, we want the LED core to
> be implementing this somehow, so that all devices using the core get
> this functionality, rather than put it in every driver.
>

The driver does exploit the LED core.   The actually interface to the
LEDs are simple on/off settings.  The strobing follows the OCP spec
to alternate between the "below" speed color (amber for fbnic) and the "at"
speed color (blue for fbnic).

The only complication is the the CSR bit for the activity LED is 1 implies
off and zero implies on.  The activity LED flashes automatically
based on actual network activity provides it is set to 0 (on).

The return to the LED core gives the count of the number of cycles per
second and each cycle alternates between amber and blue.  The driver
is a slave to the ETHTOOL_ID_* phys_id state from the LED core based
on that timing.

I'm not sure what else would need to go into the LED core?

> > +static int fbnic_led_hw_ctl_set(struct led_classdev *led_cdev,
> > +				unsigned long flags)
> > +{
> > +	struct fbnic_led_cdev *ldev = led_classdev_to_fbnic_led(led_cdev);
> > +	struct fbnic_dev *fbd = led_cdev_to_fbd(led_cdev);
> > +	int led_idx = fbnic_led_get_idx(fbd, ldev);
> > +	u32 supported_modes;
> > +
> > +	supported_modes = fbnic_led_get_supported_modes(fbd, led_idx);
> > +
> > +	if (flags & ~supported_modes)
> > +		return -EINVAL;
> > +
> > +	/* Turn on activity LED only when both the TX and RX flags are set. */
> > +	if (led_idx == FBNIC_LED_ACTIVITY && (flags & supported_modes) &&
> > +	    flags != supported_modes) {
> > +		dev_warn(fbd->dev,
> > +			 "Invalid activity LED mode(s): 0x%lx\n", flags);
> > +		return -EINVAL;
>
> How does this happen? It should be that the core will not ask you to
> do something you cannot do, if you have answered -EOPNOTSUPP in
> fbnic_led_hw_ctl_is_supported().
>

Will need to investigate this.

> > +static int fbnic_led_brightness_set_blocking(struct led_classdev *led_cdev,
> > +					     enum led_brightness brightness)
> > +{
> > +	struct fbnic_led_cdev *ldev = led_classdev_to_fbnic_led(led_cdev);
> > +	struct fbnic_dev *fbd = led_cdev_to_fbd(led_cdev);
> > +	int led_idx = fbnic_led_get_idx(fbd, ldev);
> > +
> > +	mutex_lock(&fbd->led_mutex);
> > +	if (!brightness) {
> > +		fbd->leds[led_idx].enabled_modes = 0;
> > +		fbd->leds[led_idx].strobe_mode = 0;
> > +	} else {
> > +		u32 mode;
> > +
> > +		switch (led_idx) {
> > +		case FBNIC_LED_ACTIVITY:
> > +			fbd->leds[led_idx].enabled_modes =
> > +				BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX);
> > +			break;
> > +		default:
> > +			mode = fbnic_led_get_link_speed_mode(fbd);
> > +			fbd->leds[led_idx].enabled_modes = mode;
> > +			break;
> > +		}
> > +	}
>
> This looks odd. led_brightness_set() is simply used to set the LED to
> on or off. The netdev trigger will use it to software blink the LEDs
> when asked to do something which cannot be offloaded to the hardware.
>
> And when the LED is being used to show NUMLOCK, or Morse code crash
> dump, network activity should not intervene.
>

Will need to investigate this.

> > diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_mac.c b/drivers/net/ethernet/meta/fbnic/fbnic_mac.c
> > index 53b7a938b4c2..c6b1130f9159 100644
> > --- a/drivers/net/ethernet/meta/fbnic/fbnic_mac.c
> > +++ b/drivers/net/ethernet/meta/fbnic/fbnic_mac.c
>
>
> > @@ -623,6 +623,53 @@ static bool fbnic_pmd_update_state(struct fbnic_dev *fbd, bool signal_detect)
> >  	return false;
> >  }
> >
> > +u32 fbnic_get_link_speed(u8 link_speed)
> > +{
> > +	switch (link_speed) {
> > +	case FBNIC_FW_LINK_MODE_25CR:
> > +		return SPEED_25000;
> > +	case FBNIC_FW_LINK_MODE_50CR2:
> > +	case FBNIC_FW_LINK_MODE_50CR:
> > +		return SPEED_50000;
> > +	case FBNIC_FW_LINK_MODE_100CR2:
> > +		return SPEED_100000;
> > +	default:
> > +		return 0;
> > +	}
> > +}
> > +
> > +static void fbnic_set_led_state(struct fbnic_dev *fbd, int state)
> > +{
> > +	mutex_lock(&fbd->led_mutex);
> > +
> > +	/* alternating amber,blue IDs device every half second */
> > +	switch (state) {
> > +	case FBNIC_LED_OFF: /* amber on, blue off */
> > +		fbd->leds[FBNIC_LED_LINK_AMBER].strobe_mode = FBNIC_STROBE_ON;
> > +		fbd->leds[FBNIC_LED_LINK_BLUE].strobe_mode = FBNIC_STROBE_OFF;
> > +		break;
> > +	case FBNIC_LED_ON: /* amber off, blue on */
> > +		fbd->leds[FBNIC_LED_LINK_AMBER].strobe_mode = FBNIC_STROBE_OFF;
> > +		fbd->leds[FBNIC_LED_LINK_BLUE].strobe_mode = FBNIC_STROBE_ON;
> > +		break;
> > +	case FBNIC_LED_RESTORE:
> > +		fbd->leds[FBNIC_LED_LINK_AMBER].strobe_mode =
> > +			FBNIC_STROBE_DISABLED;
> > +		fbd->leds[FBNIC_LED_LINK_BLUE].strobe_mode =
> > +			FBNIC_STROBE_DISABLED;
> > +		break;
> > +	case FBNIC_LED_STROBE_INIT: /* a no-op */
> > +		/* Initialization is a no-op; LED toggling happens on ON/OFF */
> > +		goto out_unlock;
> > +	default:
> > +		goto out_unlock;
> > +	}
> > +
> > +	fbnic_led_update_csr(fbd);
> > +out_unlock:
> > +	mutex_unlock(&fbd->led_mutex);
> > +}
> > +
>
> This appears to be something different to trig-netdev and sys class
> LED control of the LED. Please could you move this out into another
> patch. Add clean support for the LED first, and them mess it all up in
> follow up patches adding all your vendor specific stuff. We can then
> think about if some of that can be moved into the LED core/trig-netdev
> somehow.
>

This IS the slave to the LED core ID mechanism.

If I'm understanding your suggestion the ethtool id, this routine and
the fbnic_led_update_csr() need to be a separarate patch?

> 	Andrew