Re: [PATCH RFC net-next 06/10] net: dsa: sja1105: simplify static configuration reload

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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!