On Sat, 13 Feb 2016 00:54:46 +0100 Hartmut Knaack <knaac...@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@lists.openwrt.org https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
