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

[Andrew Lunn]

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

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

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

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

	Andrew