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

Luiz Angelo Daros de Luca
2024-03-25T02:51:05Z

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