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

Commit Message

Luiz Angelo Daros de Luca March 10, 2024, 4:52 a.m. UTC

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

Software triggers function as expected with manually controlled LEDs.

Signed-off-by: Luiz Angelo Daros de Luca <[e-mail]>
Reviewed-by: Linus Walleij <[e-mail]>
---
 drivers/net/dsa/realtek/rtl8366rb.c | 270 ++++++++++++++++++++++++++++++++----
 1 file changed, 246 insertions(+), 24 deletions(-)

Comments

Krzysztof Kozlowski March 10, 2024, 8:02 a.m. UTC | #1

On 10/03/2024 05:52, 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.
> Once in this mode, there is no way to revert to hardware-controlled LEDs
> (except by resetting the switch).
> 
> Software triggers function as expected with manually controlled LEDs.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <[e-mail]>
> Reviewed-by: Linus Walleij <[e-mail]>

Again, this is the first version, so how could you have a review?

Best regards,
Krzysztof

Simon Horman March 10, 2024, 11:49 a.m. UTC | #2

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.
> Once in this mode, there is no way to revert to hardware-controlled LEDs
> (except by resetting the switch).
> 
> Software triggers function as expected with manually controlled LEDs.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <[e-mail]>
> Reviewed-by: Linus Walleij <[e-mail]>
> ---
>  drivers/net/dsa/realtek/rtl8366rb.c | 270 ++++++++++++++++++++++++++++++++----

...

> +static int rtl8366rb_setup_led(struct realtek_priv *priv, struct dsa_port *dp,
> +			       struct fwnode_handle *led_fwnode)
> +{
> +	struct rtl8366rb *rb = priv->chip_data;
> +	struct led_init_data init_data = { };
> +	struct rtl8366rb_led *led;
> +	enum led_default_state state;
> +	u32 led_group;
> +	int ret;

nit: Please consider using reverse xmas tree - longest line to shortest -
     for local variables in networking code.

...

> +static int rtl8366rb_setup_leds(struct realtek_priv *priv)
> +{
> +	struct device_node *leds_np, *led_np;
> +	struct dsa_switch *ds = &priv->ds;
> +	struct dsa_port *dp;
> +	int ret;
> +
> +	dsa_switch_for_each_port(dp, ds) {
> +		if (!dp->dn)
> +			continue;
> +
> +		leds_np = of_get_child_by_name(dp->dn, "leds");
> +		if (!leds_np) {
> +			dev_dbg(priv->dev, "No leds defined for port %d",
> +				dp->index);
> +			continue;
> +		}
> +
> +		for_each_child_of_node(leds_np, led_np) {
> +			ret = rtl8366rb_setup_led(priv, dp,
> +						  of_fwnode_handle(led_np));
> +			if (ret) {
> +				of_node_put(led_np);

FWIIW, Coccinelle complans about "probable double put" here.
But it looks correct to me as it's only called when breaking out of
the loop, when it is required AFAIK.

> +				break;
> +			}
> +		}
> +
> +		of_node_put(leds_np);
> +		if (ret)

I'm unsure if this can happen. But if for_each_child_of_node()
iterates zero times then ret may be uninitialised here.

Flagged by Smatch.

> +			return ret;
> +	}
> +	return 0;
> +}

...

Andrew Lunn March 10, 2024, 6:47 p.m. UTC | #3

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

Luiz Angelo Daros de Luca March 24, 2024, 2:31 a.m. UTC | #4

> 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.
> > Once in this mode, there is no way to revert to hardware-controlled LEDs
> > (except by resetting the switch).
> >
> > Software triggers function as expected with manually controlled LEDs.
> >
> > Signed-off-by: Luiz Angelo Daros de Luca <[e-mail]>
> > Reviewed-by: Linus Walleij <[e-mail]>
> > ---
> >  drivers/net/dsa/realtek/rtl8366rb.c | 270 ++++++++++++++++++++++++++++++++----
>
> ...
>
> > +static int rtl8366rb_setup_led(struct realtek_priv *priv, struct dsa_port *dp,
> > +                            struct fwnode_handle *led_fwnode)
> > +{
> > +     struct rtl8366rb *rb = priv->chip_data;
> > +     struct led_init_data init_data = { };
> > +     struct rtl8366rb_led *led;
> > +     enum led_default_state state;
> > +     u32 led_group;
> > +     int ret;
>
> nit: Please consider using reverse xmas tree - longest line to shortest -
>      for local variables in networking code.

Sorry, I might have renamed a variable to a shorter form without
rechecking the order. Thanks for the tip.

> ...
>
> > +static int rtl8366rb_setup_leds(struct realtek_priv *priv)
> > +{
> > +     struct device_node *leds_np, *led_np;
> > +     struct dsa_switch *ds = &priv->ds;
> > +     struct dsa_port *dp;
> > +     int ret;
> > +
> > +     dsa_switch_for_each_port(dp, ds) {
> > +             if (!dp->dn)
> > +                     continue;
> > +
> > +             leds_np = of_get_child_by_name(dp->dn, "leds");
> > +             if (!leds_np) {
> > +                     dev_dbg(priv->dev, "No leds defined for port %d",
> > +                             dp->index);
> > +                     continue;
> > +             }
> > +
> > +             for_each_child_of_node(leds_np, led_np) {
> > +                     ret = rtl8366rb_setup_led(priv, dp,
> > +                                               of_fwnode_handle(led_np));
> > +                     if (ret) {
> > +                             of_node_put(led_np);
>
> FWIIW, Coccinelle complans about "probable double put" here.
> But it looks correct to me as it's only called when breaking out of
> the loop, when it is required AFAIK.

I agree. I do think the put is required when you break the loop as the
put happens in the increment/decrement code, including the last one
when it naturally ends the loop with led_np defined as null.

>
> > +                             break;
> > +                     }
> > +             }
> > +
> > +             of_node_put(leds_np);
> > +             if (ret)
>
> I'm unsure if this can happen. But if for_each_child_of_node()
> iterates zero times then ret may be uninitialised here.
>
> Flagged by Smatch.

Yes, I'll initialize it as 0. It could also use if (led_np), as it
will only be defined if the loop was broken (but checking ret seems to
be simpler).

>
> > +                     return ret;
> > +     }
> > +     return 0;
> > +}
>
> ...

Luiz Angelo Daros de Luca March 24, 2024, 3:46 a.m. UTC | #5

Hi Andrew,

Thanks for the review.

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

The group of a LED is a HW property. There are pins for each LED on
each group. You cannot move LEDs out of a group, not even to simply
disable a single LED.
The trigger mode is the same for all LEDs in a group as it is a LED
group property and not a LED characteristic. There is a special
trigger mode (manual) that disables HW triggers and lets the LEDs be
controlled by software triggers using a register bitmap.

> > 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 tried to implement that but I failed. There was some discussion
about it in the RFC thread. The main issue is that hw offload is only
evaluated when a LED changes its sysfs settings. The driver has
limited control about the hw offload decision, only being notified
when led_cdev->hw_control_is_supported() is called. The driver will
not be notified, for example, if the trigger_data->net_dev was changed
or if hw_control was disabled. However, even if you know a HW trigger
could be enabled, you cannot put those other LEDs in HW offload mode.
It is only enabled from sysfs calls but not from the kernel space and
AFAIK, you should not poke with sysfs from the kernel space.

The incompatibility with LDE API also has some unexpected
side-effects. For example, LEDS_DEFSTATE_KEEP will only really keep
the default vendor state if all LEDs in a group use that setting or
are missing in the DT. If one of them differs, it will switch the
group to manual mode.

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

The trigger_data->hw_control is only set from netdev_led_attr_store()
(or during trigger activation). That code is only exposed to sysfs
calls. We'll need an exported function that could set that. Also, the
driver can only control that decision from
led_cdev->hw_control_is_supported. However, it is not enough to
surelly decide if a LED is still in a state that would support HW
offload (i.e. because trigger_data->net_dev could have changed). So,
we need to:

1) expose internal ledtrig-netdev data required for deciding if
offload is supported for any LED at any time
2) expose a way to reevaluate trigger_data->hw_control (or a way to
forcely enable it)

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

Yes, it is, at least, working. The current code is simply not usable.

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

I don't know if this concept of group is unusual but LED API
definitely does not handle it well.

OK, I'll add _group_/_GROUP_ both to the enum name and macros. Led
blink rate, for example, is global, used by all groups. However, it
will be difficult to respect the 80 columns limit passing
RTL8366RB_LED_GROUP_OFF to a rb8366rb_set_ledgroup_mode function with
only two levels of indentation. Do you have any recommendations?

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

Yes, copy/paste mistake. Thanks.

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

Yes, I was redundant. If memory is intact, led->led_group is
guaranteed to be in range. I'll drop it in both get/set.

>
>        Andrew

Regards,

Luiz

Andrew Lunn March 24, 2024, 3:32 p.m. UTC | #6

> OK, I'll add _group_/_GROUP_ both to the enum name and macros. Led
> blink rate, for example, is global, used by all groups. However, it
> will be difficult to respect the 80 columns limit passing
> RTL8366RB_LED_GROUP_OFF to a rb8366rb_set_ledgroup_mode function with
> only two levels of indentation. Do you have any recommendations?

https://www.kernel.org/doc/html/v4.10/process/coding-style.html

  Now, some people will claim that having 8-character indentations
  makes the code move too far to the right, and makes it hard to read
  on a 80-character terminal screen. The answer to that is that if you
  need more than 3 levels of indentation, you’re screwed anyway, and
  should fix your program.

  Functions should be short and sweet, and do just one thing. They
  should fit on one or two screenfuls of text (the ISO/ANSI screen
  size is 80x24, as we all know), and do one thing and do that well.

Maybe you need to use more helper functions?

      Andrew

Luiz Angelo Daros de Luca March 25, 2024, 2:50 a.m. UTC | #7

> > OK, I'll add _group_/_GROUP_ both to the enum name and macros. Led
> > blink rate, for example, is global, used by all groups. However, it
> > will be difficult to respect the 80 columns limit passing
> > RTL8366RB_LED_GROUP_OFF to a rb8366rb_set_ledgroup_mode function with
> > only two levels of indentation. Do you have any recommendations?

Hi Andrew,

> https://www.kernel.org/doc/html/v4.10/process/coding-style.html
>
>   Now, some people will claim that having 8-character indentations
>   makes the code move too far to the right, and makes it hard to read
>   on a 80-character terminal screen. The answer to that is that if you
>   need more than 3 levels of indentation, you’re screwed anyway, and
>   should fix your program.

I need 3, not more than 3.

>   Functions should be short and sweet, and do just one thing. They
>   should fit on one or two screenfuls of text (the ISO/ANSI screen
>   size is 80x24, as we all know), and do one thing and do that well.
>
> Maybe you need to use more helper functions?

The call that violates (by 1) the limit is to
rb8366rb_set_ledgroup_mode(). With its name (a little long), the now
5-char longer macro/enum and 3 tabs, it has 81 columns when I align
the argument to the opening parenthesis.

static int rtl8366rb_setup(struct dsa_switch *ds)
{
       (...)
       if (priv->leds_disabled) {
               /* Turn everything off */
               regmap_update_bits(priv->map,
                                  RTL8366RB_INTERRUPT_CONTROL_REG,
                                  RTL8366RB_P4_RGMII_LED,
                                  0);

               for (i = 0; i < RTL8366RB_NUM_LEDGROUPS; i++) {
                       ret = rb8366rb_set_ledgroup_mode(priv, i,
                                                        RTL8366RB_LEDGROUP_OFF);
                       if (ret)
                               return ret;
               }
        }
}

Should I rename the rb8366rb_set_ledgroup_mode function,
RTL8366RB_LEDGROUP_OFF or is the violation here acceptable?

Can I use the double tab indentation here like it appears in
https://elixir.bootlin.com/linux/latest/source/net/8021q/vlanproc.c#L120?

Regards,

Luiz

Andrew Lunn March 25, 2024, 12:38 p.m. UTC | #8

> static int rtl8366rb_setup(struct dsa_switch *ds)
> {
>        (...)
>        if (priv->leds_disabled) {

         if (!priv->leds_disable)
	    return;
	    
Saves you one level of indentation.

or

static int rtl8366rb_setup_all_off(foo *priv)
{

>                /* Turn everything off */
>                regmap_update_bits(priv->map,
>                                   RTL8366RB_INTERRUPT_CONTROL_REG,
>                                   RTL8366RB_P4_RGMII_LED,
>                                   0);
> 
>                for (i = 0; i < RTL8366RB_NUM_LEDGROUPS; i++) {
>                        ret = rb8366rb_set_ledgroup_mode(priv, i,
>                                                         RTL8366RB_LEDGROUP_OFF);
>                        if (ret)
>                                return ret;
>                }
>         }
> }

This is what i meant by small helpers. And the function names replaces
the need for the comment.

This is part of the thinking behind the coding style. Function names
can replace comments. And the higher level functions becomes easier to
follow because you don't need to dig into the details to understand:

        if (priv->leds_disabled)
	     rtl8366rb_setup_all_off(priv);

     Andrew

Patch

diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c
index 5ccb1a3a149d..e45773fec17e 100644
--- a/drivers/net/dsa/realtek/rtl8366rb.c
+++ b/drivers/net/dsa/realtek/rtl8366rb.c
@@ -191,31 +191,21 @@ 
 	(4 * (led_group))
 #define RTL8366RB_LED_CTRL_MASK(led_group)	\
 	(0xf << RTL8366RB_LED_CTRL_OFFSET(led_group))
-#define RTL8366RB_LED_OFF			0x0
-#define RTL8366RB_LED_DUP_COL			0x1
-#define RTL8366RB_LED_LINK_ACT			0x2
-#define RTL8366RB_LED_SPD1000			0x3
-#define RTL8366RB_LED_SPD100			0x4
-#define RTL8366RB_LED_SPD10			0x5
-#define RTL8366RB_LED_SPD1000_ACT		0x6
-#define RTL8366RB_LED_SPD100_ACT		0x7
-#define RTL8366RB_LED_SPD10_ACT			0x8
-#define RTL8366RB_LED_SPD100_10_ACT		0x9
-#define RTL8366RB_LED_FIBER			0xa
-#define RTL8366RB_LED_AN_FAULT			0xb
-#define RTL8366RB_LED_LINK_RX			0xc
-#define RTL8366RB_LED_LINK_TX			0xd
-#define RTL8366RB_LED_MASTER			0xe
-#define RTL8366RB_LED_FORCE			0xf
 
 /* The RTL8366RB_LED_X_X registers are used to manually set the LED state only
  * when the corresponding LED group in RTL8366RB_LED_CTRL_REG is
  * RTL8366RB_LED_FORCE. Otherwise, it is ignored.
  */
 #define RTL8366RB_LED_0_1_CTRL_REG		0x0432
-#define RTL8366RB_LED_1_OFFSET			6
 #define RTL8366RB_LED_2_3_CTRL_REG		0x0433
-#define RTL8366RB_LED_3_OFFSET			6
+#define RTL8366RB_LED_X_X_CTRL_REG(led_group)	\
+	((led_group) <= 1 ? \
+		RTL8366RB_LED_0_1_CTRL_REG : \
+		RTL8366RB_LED_2_3_CTRL_REG)
+#define RTL8366RB_LED_0_X_CTRL_MASK		GENMASK(5, 0)
+#define RTL8366RB_LED_X_1_CTRL_MASK		GENMASK(11, 6)
+#define RTL8366RB_LED_2_X_CTRL_MASK		GENMASK(5, 0)
+#define RTL8366RB_LED_X_3_CTRL_MASK		GENMASK(11, 6)
 
 #define RTL8366RB_MIB_COUNT			33
 #define RTL8366RB_GLOBAL_MIB_COUNT		1
@@ -359,14 +349,44 @@ 
 #define RTL8366RB_GREEN_FEATURE_TX	BIT(0)
 #define RTL8366RB_GREEN_FEATURE_RX	BIT(2)
 
+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,
+
+	__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)
+{
+	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;
+	}
+
+	ret = regmap_read(priv->map, RTL8366RB_LED_X_X_CTRL_REG(led_group),
+			  &val);
+	if (ret) {
+		dev_err(priv->dev, "error reading LED on port %d group %d\n",
+			led_group, port_num);
+		return ret;
+	}
+
+	return !!(val & rtl8366rb_led_group_port_mask(led_group, port_num));
+}
+
+static int rb8366rb_set_port_led(struct rtl8366rb_led *led, bool enable)
+{
+	struct realtek_priv *priv = led->priv;
+	u8 led_group = led->led_group;
+	u8 port_num = led->port_num;
+	int ret;
+
+	if (led_group >= RTL8366RB_NUM_LEDGROUPS) {
+		dev_err(priv->dev, "Invalid LED group %d for port %d",
+			led_group, port_num);
+		return -EINVAL;
+	}
+
+	ret = regmap_update_bits(priv->map,
+				 RTL8366RB_LED_X_X_CTRL_REG(led_group),
+				 rtl8366rb_led_group_port_mask(led_group,
+							       port_num),
+				 enable ? 0xffff : 0);
+	if (ret) {
+		dev_err(priv->dev, "error updating LED on port %d group %d\n",
+			led_group, port_num);
+		return ret;
+	}
+
+	/* Change the LED group to manual controlled LEDs if required */
+	ret = rb8366rb_set_ledgroup_mode(priv, led_group, RTL8366RB_LED_FORCE);
+
+	if (ret) {
+		dev_err(priv->dev, "error updating LED GROUP group %d\n",
+			led_group);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int
+rtl8366rb_cled_brightness_set_blocking(struct led_classdev *ldev,
+				       enum led_brightness brightness)
+{
+	struct rtl8366rb_led *led = container_of(ldev, struct rtl8366rb_led,
+						 cdev);
+
+	return rb8366rb_set_port_led(led, brightness == LED_ON);
+}
+
+static int rtl8366rb_setup_led(struct realtek_priv *priv, struct dsa_port *dp,
+			       struct fwnode_handle *led_fwnode)
+{
+	struct rtl8366rb *rb = priv->chip_data;
+	struct led_init_data init_data = { };
+	struct rtl8366rb_led *led;
+	enum led_default_state state;
+	u32 led_group;
+	int ret;
+
+	ret = fwnode_property_read_u32(led_fwnode, "reg", &led_group);
+	if (ret)
+		return ret;
+
+	if (led_group >= RTL8366RB_NUM_LEDGROUPS) {
+		dev_warn(priv->dev, "Invalid LED reg %d defined for port %d",
+			 led_group, dp->index);
+		return -EINVAL;
+	}
+
+	led = &rb->leds[dp->index][led_group];
+	led->port_num = dp->index;
+	led->led_group = led_group;
+	led->priv = priv;
+
+	state = led_init_default_state_get(led_fwnode);
+	switch (state) {
+	case LEDS_DEFSTATE_ON:
+		led->cdev.brightness = 1;
+		rb8366rb_set_port_led(led, 1);
+		break;
+	case LEDS_DEFSTATE_KEEP:
+		led->cdev.brightness =
+			rb8366rb_get_port_led(led, 1);
+		break;
+	case LEDS_DEFSTATE_OFF:
+	default:
+		led->cdev.brightness = 0;
+		rb8366rb_set_port_led(led, 0);
+	}
+
+	led->cdev.max_brightness = 1;
+	led->cdev.brightness_set_blocking =
+		rtl8366rb_cled_brightness_set_blocking;
+	init_data.fwnode = led_fwnode;
+	init_data.devname_mandatory = true;
+
+	init_data.devicename = kasprintf(GFP_KERNEL, "Realtek-%d:0%d:%d",
+					 dp->ds->index, dp->index, led_group);
+	if (!init_data.devicename)
+		return -ENOMEM;
+
+	ret = devm_led_classdev_register_ext(priv->dev, &led->cdev, &init_data);
+	if (ret) {
+		dev_warn(priv->dev, "Failed to init LED %d for port %d",
+			 led_group, dp->index);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int rtl8366rb_setup_leds(struct realtek_priv *priv)
+{
+	struct device_node *leds_np, *led_np;
+	struct dsa_switch *ds = &priv->ds;
+	struct dsa_port *dp;
+	int ret;
+
+	dsa_switch_for_each_port(dp, ds) {
+		if (!dp->dn)
+			continue;
+
+		leds_np = of_get_child_by_name(dp->dn, "leds");
+		if (!leds_np) {
+			dev_dbg(priv->dev, "No leds defined for port %d",
+				dp->index);
+			continue;
+		}
+
+		for_each_child_of_node(leds_np, led_np) {
+			ret = rtl8366rb_setup_led(priv, dp,
+						  of_fwnode_handle(led_np));
+			if (ret) {
+				of_node_put(led_np);
+				break;
+			}
+		}
+
+		of_node_put(leds_np);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+
 static int rtl8366rb_setup(struct dsa_switch *ds)
 {
 	struct realtek_priv *priv = ds->priv;
@@ -817,7 +1039,6 @@  static int rtl8366rb_setup(struct dsa_switch *ds)
 	u32 chip_ver = 0;
 	u32 chip_id = 0;
 	int jam_size;
-	u32 val;
 	int ret;
 	int i;
 
@@ -1017,14 +1238,15 @@  static int rtl8366rb_setup(struct dsa_switch *ds)
 				   0);
 
 		for (i = 0; i < RTL8366RB_NUM_LEDGROUPS; i++) {
-			val = RTL8366RB_LED_OFF << RTL8366RB_LED_CTRL_OFFSET(i);
-			ret = regmap_update_bits(priv->map,
-						 RTL8366RB_LED_CTRL_REG,
-						 RTL8366RB_LED_CTRL_MASK(i),
-						 val);
+			ret = rb8366rb_set_ledgroup_mode(priv, i,
+							 RTL8366RB_LED_OFF);
 			if (ret)
 				return ret;
 		}
+	} else {
+		ret = rtl8366rb_setup_leds(priv);
+		if (ret)
+			return ret;
 	}
 
 	ret = rtl8366_reset_vlan(priv);