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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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