Re: [RFC PATCH vN net-next 2/2] net: mscc: ocelot: add support for VSC75XX SPI control

On 04/05/2021 12:59:43+0000, Vladimir Oltean wrote:

quoted

quoted

+static void vsc7512_phylink_validate(struct ocelot *ocelot, int port,
+				     unsigned long *supported,
+				     struct phylink_link_state *state)
+{
+	struct ocelot_port *ocelot_port = ocelot->ports[port];
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = {
+		0,
+	};

This function seems out of place. Why would SPI access change what the
ports are capable of doing? Please split this up into more
patches. Keep the focus of this patch as being adding SPI support.

What is going on is that this is just the way in which the drivers are
structured. Colin is not really "adding SPI support" to any of the
existing DSA switches that are supported (VSC9953, VSC9959) as much as
"adding support for a new switch which happens to be controlled over
SPI" (VSC7512).

Note that this should not only be about vsc7512 as the whole ocelot
family (vsc7511, vsc7512, vsc7513 and vsc7514) can be connected over
spi. Also, they can all be used in a DSA configuration, over PCIe, just
like Felix.

The layering is as follows:
- drivers/net/dsa/ocelot/felix_vsc7512_spi.c: deals with the most
  hardware specific SoC support. The regmap is defined here, so are the
  port capabilities.
- drivers/net/dsa/ocelot/felix.c: common integration with DSA
- drivers/net/ethernet/mscc/ocelot*.c: the SoC-independent hardware
  support.

I'm not actually sure that splitting the port PHY mode support in a
separate patch is possible while keeping functional intermediate
results. But I do agree about the rest, splitting the device tree
changes, etc.

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com