Re: [PATCH v2 net-next 1/3] net: dsa: lan9303: Refactor lan9303_xxx_packet_processing()

From: Egil Hjelmeland
Date: Tue Aug 01 2017 - 10:43:20 EST

On 01. aug. 2017 16:02, Andrew Lunn wrote:
On Tue, Aug 01, 2017 at 03:50:14PM +0200, Egil Hjelmeland wrote:
On 01. aug. 2017 15:39, Andrew Lunn wrote:
@@ -704,7 +710,7 @@ static void lan9303_get_ethtool_stats(struct dsa_switch *ds, int port,
unsigned int u, poff;
int ret;
- poff = port * 0x400;
+ poff = LAN9303_SWITCH_PORT_REG(port, 0);
for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
ret = lan9303_read_switch_reg(chip,

So the actual code is:

for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
ret = lan9303_read_switch_reg(chip,
lan9303_mib[u].offset + poff,

Could this be written as

for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
ret = lan9303_read_switch_port(chip, port, lan9303_mib[u].offset, &reg);

It is then clear you are reading the statistics from a port register.


Yes it can. Since it is (insignificantly) less efficient, I
chose not to touch it. But I can do it if you like.

I doubt it is less efficient. The compiler has seen
lan9303_read_switch_port() and will probably inline it. So what the
optimiser gets to see is probably the same in both cases.

Try generating the assembler listing in both cases, and compare them

make drivers/net/dsa/lan9303-core.lst


Thanks for the tips about generating assembler listing, can be useful
another time. But in this case I trust you :-)
And in this case it does not really matter, because its not in the
data path.

I did try to look at the listing. But I did not quite understand it.
Looks like it is doing both inlining and unrolling.

Anyway, you just decide how you like to have it in this series.