Re: [PATCH net-next 4/4] net: dsa: realtek: add LED drivers for rtl8366rb

[Andrew Lunn]

On Sun, Mar 10, 2024 at 01:52:01AM -0300, Luiz Angelo Daros de Luca wrote:
> This commit introduces LED drivers for rtl8366rb, enabling LEDs to be
> described in the device tree using the same format as qca8k. Each port
> can configure up to 4 LEDs.
> 
> If all LEDs in a group use the default state "keep", they will use the
> default behavior after a reset. Changing the brightness of one LED,
> either manually or by a trigger, will disable the default hardware
> trigger and switch the entire LED group to manually controlled LEDs.


The previous patch said:

  This switch family supports four LEDs for each of its six
  ports. Each LED group is composed of one of these four LEDs from all
  six ports. LED groups can be configured to display hardware
  information, such as link activity, or manually controlled through a
  bitmap in registers RTL8366RB_LED_0_1_CTRL_REG and
  RTL8366RB_LED_2_3_CTRL_REG.

I could be understanding this wrongly, but to me, it sounds like an
LED is either controlled via the group, or you can take an LED out of
the group and software on/off control it? Ah, after looking at the
code. The group can be put into forced mode, and then each LED in the
group controlled in software.

> Once in this mode, there is no way to revert to hardware-controlled LEDs
> (except by resetting the switch).

Just for my understanding.... This is a software limitation. You could
check if all LEDs in a group are using the same trigger, and then set
the group to that trigger?

I do understand how the current offload concept causes problems here.
You need a call into the trigger to ask it to re-evaluate if offload
can be performed for an LED.

What you have here seems like a good first step, offloaded could be
added later if somebody wants to.

> +enum rtl8366_led_mode {
> +	RTL8366RB_LED_OFF		= 0x0,
> +	RTL8366RB_LED_DUP_COL		= 0x1,
> +	RTL8366RB_LED_LINK_ACT		= 0x2,
> +	RTL8366RB_LED_SPD1000		= 0x3,
> +	RTL8366RB_LED_SPD100		= 0x4,
> +	RTL8366RB_LED_SPD10		= 0x5,
> +	RTL8366RB_LED_SPD1000_ACT	= 0x6,
> +	RTL8366RB_LED_SPD100_ACT	= 0x7,
> +	RTL8366RB_LED_SPD10_ACT		= 0x8,
> +	RTL8366RB_LED_SPD100_10_ACT	= 0x9,
> +	RTL8366RB_LED_FIBER		= 0xa,
> +	RTL8366RB_LED_AN_FAULT		= 0xb,
> +	RTL8366RB_LED_LINK_RX		= 0xc,
> +	RTL8366RB_LED_LINK_TX		= 0xd,
> +	RTL8366RB_LED_MASTER		= 0xe,
> +	RTL8366RB_LED_FORCE		= 0xf,

This is what the group shows? Maybe put _GROUP_ into the name? This
concept of a group is pretty unusual, so we should be careful with
naming to make it clear when we are referring to one LED or a group of
LEDs. I would also put _group_ into the enum.

> +
> +	__RTL8366RB_LED_MAX
> +};
> +
> +struct rtl8366rb_led {
> +	u8 port_num;
> +	u8 led_group;
> +	struct realtek_priv *priv;
> +	struct led_classdev cdev;
> +};
> +
>  /**
>   * struct rtl8366rb - RTL8366RB-specific data
>   * @max_mtu: per-port max MTU setting
>   * @pvid_enabled: if PVID is set for respective port
> + * @leds: per-port and per-ledgroup led info
>   */
>  struct rtl8366rb {
>  	unsigned int max_mtu[RTL8366RB_NUM_PORTS];
>  	bool pvid_enabled[RTL8366RB_NUM_PORTS];
> +	struct rtl8366rb_led leds[RTL8366RB_NUM_PORTS][RTL8366RB_NUM_LEDGROUPS];
>  };
>  
>  static struct rtl8366_mib_counter rtl8366rb_mib_counters[] = {
> @@ -809,6 +829,208 @@ static int rtl8366rb_jam_table(const struct rtl8366rb_jam_tbl_entry *jam_table,
>  	return 0;
>  }
>  
> +static int rb8366rb_set_ledgroup_mode(struct realtek_priv *priv,
> +				      u8 led_group,
> +				      enum rtl8366_led_mode mode)
> +{
> +	int ret;
> +	u32 val;
> +
> +	val = mode << RTL8366RB_LED_CTRL_OFFSET(led_group);
> +
> +	ret = regmap_update_bits(priv->map,
> +				 RTL8366RB_LED_CTRL_REG,
> +				 RTL8366RB_LED_CTRL_MASK(led_group),
> +				 val);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static inline u32 rtl8366rb_led_group_port_mask(u8 led_group, u8 port)
> +{
> +	switch (led_group) {
> +	case 0:
> +		return FIELD_PREP(RTL8366RB_LED_0_X_CTRL_MASK, BIT(port));
> +	case 1:
> +		return FIELD_PREP(RTL8366RB_LED_0_X_CTRL_MASK, BIT(port));
> +	case 2:
> +		return FIELD_PREP(RTL8366RB_LED_0_X_CTRL_MASK, BIT(port));
> +	case 3:
> +		return FIELD_PREP(RTL8366RB_LED_0_X_CTRL_MASK, BIT(port));
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static int rb8366rb_get_port_led(struct rtl8366rb_led *led, bool enable)

enable seems unused here. It also seems an odd parameter to pass to a
_get_ function.

> +{
> +	struct realtek_priv *priv = led->priv;
> +	u8 led_group = led->led_group;
> +	u8 port_num = led->port_num;
> +	int ret;
> +	u32 val;
> +
> +	if (led_group >= RTL8366RB_NUM_LEDGROUPS) {
> +		dev_err(priv->dev, "Invalid LED group %d for port %d",
> +			led_group, port_num);
> +		return -EINVAL;
> +	}

This check seems odd. You can validate it once when you create the
struct rtl8366rb_led. After that, just trust it?

       Andrew