[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAJq09z6ZYpwYiqo-XvLG1=_JZeCM2APmHqBjhD4rBSdRP3ERYA@mail.gmail.com>
Date: Sun, 24 Mar 2024 23:50:51 -0300
From: Luiz Angelo Daros de Luca <luizluca@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: Linus Walleij <linus.walleij@...aro.org>, Alvin Šipraga <alsi@...g-olufsen.dk>,
Florian Fainelli <f.fainelli@...il.com>, Vladimir Oltean <olteanv@...il.com>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 4/4] net: dsa: realtek: add LED drivers for rtl8366rb
> > 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
Powered by blists - more mailing lists