Message ID | 9018be0b7dc441cd8aad625c6cc44e1c@bfs.de |
---|---|
State | RFC |
Delegated to: | David Miller |
Headers | show |
Series | AW: [PATCH] net: dsa: sja1105: fix speed setting for 10 MBPS | expand |
On Fri, May 01, 2020 at 06:00:52PM +0000, Walter Harms wrote: > IMHO it would be better to use switch case here to improve readability. > > switch (bmcr & mask) { > > case BMCR_SPEED1000: > speed = SPEED_1000; > break; > case BMCR_SPEED100: > speed = SPEED_100; > break; > case BMCR_SPEED10: > speed = SPEED_10; > break; > default: > speed = SPEED_UNKNOWN > } > > jm2c, > wh > > btw: an_enabled ? why not !enabled, mich more easy to read You misinterpret "an_enabled". It's not "negated enabled". It's not even "disabled". It's short for "autonegotiation enabled". It's positive logic too.
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c index 472f4eb20c49..59a9038cdc4e 100644 --- a/drivers/net/dsa/sja1105/sja1105_main.c +++ b/drivers/net/dsa/sja1105/sja1105_main.c @@ -1600,6 +1600,7 @@ static const char * const sja1105_reset_reasons[] = { int sja1105_static_config_reload(struct sja1105_private *priv, enum sja1105_reset_reason reason) { + const int mask = (BMCR_SPEED1000 | BMCR_SPEED100 | BMCR_SPEED10); struct ptp_system_timestamp ptp_sts_before; struct ptp_system_timestamp ptp_sts_after; struct sja1105_mac_config_entry *mac; @@ -1684,14 +1685,16 @@ int sja1105_static_config_reload(struct sja1105_private *priv, sja1105_sgmii_pcs_config(priv, an_enabled, false); if (!an_enabled) { - int speed = SPEED_UNKNOWN; + int speed; - if (bmcr & BMCR_SPEED1000) + if ((bmcr & mask) == BMCR_SPEED1000) speed = SPEED_1000; - else if (bmcr & BMCR_SPEED100) + else if ((bmcr & mask) == BMCR_SPEED100) speed = SPEED_100; - else if (bmcr & BMCR_SPEED10) + else if ((bmcr & mask) == BMCR_SPEED10) speed = SPEED_10; + else + speed = SPEED_UNKNOWN; sja1105_sgmii_pcs_force_speed(priv, speed); }