Re: [RFC v1 net-next 8/8] net: dsa: ocelot: add external ocelot switch control

From: Vladimir Oltean <vladimir.oltean@nxp.com>
To: Colin Foster <colin.foster@in-advantage.com>
Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Russell King <linux@armlinux.org.uk>,
	Paolo Abeni <pabeni@redhat.com>, Jakub Kicinski <kuba@kernel.org>,
	Eric Dumazet <edumazet@google.com>,
	"David S. Miller" <davem@davemloft.net>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Vivien Didelot <vivien.didelot@gmail.com>,
	Andrew Lunn <andrew@lunn.ch>,
	"UNGLinuxDriver@microchip.com" <UNGLinuxDriver@microchip.com>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	Claudiu Manoil <claudiu.manoil@nxp.com>,
	Lee Jones <lee@kernel.org>
Subject: Re: [RFC v1 net-next 8/8] net: dsa: ocelot: add external ocelot switch control
Date: Fri, 16 Sep 2022 22:31:47 +0000	[thread overview]
Message-ID: <20220916223146.a5djbyvwlh6jekw6@skbuf> (raw)
In-Reply-To: <YySqm8t0pbH4cqR/@euler>

On Fri, Sep 16, 2022 at 09:55:55AM -0700, Colin Foster wrote:
> I'm looking into these changes now. I was using "ocelot_ext_" not
> necessarily as an indication as "this only matters when it is controlled
> externally" but rather "this is a function within ocelot_ext.c"
> 
> So there's a weird line between what constitutes "external control" vs
> "generic"
> 
> There are only two things that really matter for external control:
> 1. The regmaps are set up in a specific way by ocelot-mfd
> 2. The existence of a DSA CPU port
> 
> Going by 1 only - there's basically nothing in
> drivers/net/dsa/ocelot/ocelot_ext.c that is inherently "external only".
> And that's kindof the point. The only thing that can be done externally
> that isn't done internally would be a whole chip reset.
> 
> Going by 2 only - the simple fact that it is in drivers/net/dsa/ means
> that there is a CPU port, and therefore everything in the file requires
> that it is externally controlled.
> 
> 
> 
> Unless you're going another way, and you're not actually talking about
> "function names" but instead "should this actually live in ocelot_lib"
> 
> While I don't think that's what you were directly suggesting - I like
> this! ocelot_ext_reset() shouldn't exist - I should move, update, and
> utilize ocelot_reset() from drivers/net/ethernet/mscc/ocelot_vsc7514.c.
> 
> The ocelot_ext function list will dwindle down to:
> *_probe
> *_remove
> *_shutdown
> *_regmap_init
> *_phylink_validate

Yes, please use as much as possible from the ocelot switch library,
after all you are driving pretty much the same hardware. I'm glad for
your revelation and sorry that I didn't think of expressing it this way
sooner. I think the reset procedure used to be slightly different in the
times when the ocelot_ext DSA driver also took care of setting up what
is now the responsibility of the ocelot-mfd driver. Between then and
now, some time has passed (years if I'm not mistaken) and I forgot what
was and what wasn't said, I even forgot most of my own thoughts.