[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZvRmr3aU1Fz6z0Oc@shell.armlinux.org.uk>
Date: Wed, 25 Sep 2024 20:38:23 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
Alexandre Torgue <alexandre.torgue@...s.st.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Florian Fainelli <f.fainelli@...il.com>,
Jakub Kicinski <kuba@...nel.org>,
Jiawen Wu <jiawenwu@...stnetic.com>,
Jose Abreu <joabreu@...opsys.com>,
Jose Abreu <Jose.Abreu@...opsys.com>,
linux-arm-kernel@...ts.infradead.org,
linux-stm32@...md-mailman.stormreply.com,
Maxime Coquelin <mcoquelin.stm32@...il.com>,
Mengyuan Lou <mengyuanlou@...-swift.com>, netdev@...r.kernel.org,
Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH RFC net-next 06/10] net: dsa: sja1105: simplify static
configuration reload
On Wed, Sep 25, 2024 at 04:15:17PM +0300, Vladimir Oltean wrote:
> On Mon, Sep 23, 2024 at 03:01:25PM +0100, Russell King (Oracle) wrote:
> > +static int sja1105_set_port_speed(struct sja1105_private *priv, int port,
> > + int speed_mbps)
> > {
> > struct sja1105_mac_config_entry *mac;
> > - struct device *dev = priv->ds->dev;
>
> I think if you could keep this line in the new sja1105_set_port_speed()
> function..
>
> > u64 speed;
> > - int rc;
> >
> > /* On P/Q/R/S, one can read from the device via the MAC reconfiguration
> > * tables. On E/T, MAC reconfig tables are not readable, only writable.
> > @@ -1313,7 +1295,7 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port,
> > speed = priv->info->port_speed[SJA1105_SPEED_2500MBPS];
> > break;
> > default:
> > - dev_err(dev, "Invalid speed %iMbps\n", speed_mbps);
> > + dev_err(priv->ds->dev, "Invalid speed %iMbps\n", speed_mbps);
>
> you could also get rid of this unnecessary line change.
Yes, maybe I'll move it to another patch, but the reason I made the
change is that I don't see much point to the local variable existing
for just one user (there were multiple users prior to this patch.)
However...
> > return -EINVAL;
> > }
>
> There are 2 more changes which I believe should be made in sja1105_set_port_speed():
> - since it isn't called from mac_config() anymore but from mac_link_up()
> (change which happened quite a while ago), it mustn't handle SPEED_UNKNOWN
> - we can trust that phylink will not call mac_link_up() with a speed
> outside what we provided in mac_capabilities, so we can remove the
> -EINVAL "default" speed_mbps case, and make this method return void,
> as it can never truly cause an error
>
> But I believe these are incremental changes which should be done after
> this patch. I've made a note of them and will create 2 patches on top
> when I have the spare time.
... if we were to make those changes prior to this patch, then the
dev_err() will no longer be there and thus this becomes a non-issue.
So I'd suggest a patch prior to this one to make the changes you state
here, thus eliminating the need for this hunk in this patch.
> > +/* Set link speed in the MAC configuration for a specific port. */
>
> Could this comment state this instead? "Write the MAC Configuration
> Table entry and, if necessary, the CGU settings, after a link speed
> change for this port."
Done.
> > @@ -2293,7 +2294,7 @@ int sja1105_static_config_reload(struct sja1105_private *priv,
> > {
> > struct ptp_system_timestamp ptp_sts_before;
> > struct ptp_system_timestamp ptp_sts_after;
> > - int speed_mbps[SJA1105_MAX_NUM_PORTS];
> > + u64 mac_speed[SJA1105_MAX_NUM_PORTS];
>
> Could you move this line lower to preserve the ordering by decreasing line length?
>
> > u16 bmcr[SJA1105_MAX_NUM_PORTS] = {0};
Probably didn't notice that due to not having full clear sight for the
screen yet (a few more weeks to go before that happens!) Thanks for
spotting that.
> > - /* Back up the dynamic link speed changed by sja1105_adjust_port_config
> > + /* Back up the dynamic link speed changed by sja1105_set_port_speed
>
> Could you please put () after the function name? I know the original
> code did not have it, but my coding style has changed in the past 5 years.
Done.
Thanks!
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Powered by blists - more mailing lists