[OpenWrt-Devel] [PATCH 2/4] switch: allow Ethernet port LEDs to show specific link speeds only

[Michal]

On Sat, 13 Feb 2016 00:54:46 +0100
Hartmut Knaack <knaack.h at gmx.de> wrote:


> 
> There are a few more style issues, than John pointed out. These can
> be pointed out by using scripts/checkpatch.pl from the linux kernel
> sources (also Documentation/Coding-Style.txt is a good reference).
> Please also find some other recommendations inline.
> 

Thanks Hartmut, I will scan all patches with this tool from now on
before submitting.
> 
> bools are commonly assigned true or false. But there is an easier
> way, which makes the assignment above obsolete and the code slightly
> shorter:
> 
> 				speedok = sw_trig->link_speed[i] & speed_mask; 
> 				if (speedok)
> 					traffic += sw_trig->port_traffic[i];
> 

I changed speedok to int then, it's shorter :-)
This trigger allows to attach more than one port to LED and while usage of that 
multiport setup seems marginal in daily use, I did not want to change that 
behaviour. My approach is that if at least one of ports has link speed
matching speed_mask, LED should be lit. In proposed code above, if last port
in multiport setup has speed we do not want to show, the flag will become 0
even though earlier it was 1. I assume that once speedok is set, it stays 1
to light up LED after all selected ports traffic counters get updated.

> 
> I would factor out this switch block into a separate function and
> call it with the parameters port_link.speed and
> sw_trig->link_speed[i] (by reference). That gives you way more space
> due to lower indentation level. Thanks,
> 

I actually did it at the beginning of my work on this file, then decided to 
move it here when I realized that this conversion code will be used only in 
that particular place and nowhere else. I hope it is OK if we keep it as it
is, especially that after John's intendation fix it looks clearer.

Thanks for you time and help
Regards
Michal
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel