Re: [PATCH 4/4] net: dsa: soce: Add initial driver support for MRS switches

+static u8 soce_map_stp_state(u8 state)
+{
+	switch (state) {
+	case BR_STATE_DISABLED:
+	case BR_STATE_BLOCKING:
+	case BR_STATE_LISTENING:
+		return 0;
+	case BR_STATE_LEARNING:
+		return 1;
+	case BR_STATE_FORWARDING:
+	default:
+		return 3;

#defines for 0, 1 and 3.

What does 2 mean?

Since you don't implement bridge offload, i'm not sure having STP
states makes any sense.

+static void soce_fast_age(struct dsa_switch *ds, int port,
+			  const struct soce_layout *layout)
+{
+	struct soce_priv *priv = ds->priv;
+	struct soce_dsa_local *local;
+	void __iomem *p_mes_ctrl;
+	u32 val;

Does aging make any sense without bridge offload?

+/* Bridge membership is handled by the switch forwarding model directly. */
+int soce_port_bridge_join(struct dsa_switch *ds, int port,
+			  struct dsa_bridge bridge, bool *tx_fwd_offload,
+			  struct netlink_ext_ack *extack)
+{
+	return 0;
+}
+
+void soce_port_bridge_leave(struct dsa_switch *ds, int port,
+			    struct dsa_bridge bridge)
+{
+}

If you don't support bridge offload, you should not need these.

+static void soce_phylink_get_caps(struct dsa_switch *ds, int port,
+				  struct phylink_config *config)
+{
+	if (dsa_is_cpu_port(ds, port)) {
+		__set_bit(PHY_INTERFACE_MODE_GMII,

Is that really true? What is part of the IP core, and what is to do
with integration of the core into the system as a whole? It seems like
RGMII is also supported, so why cannot i instantiate a RGMII MII blob
for the CPU port as well?

+			  config->supported_interfaces);
+		config->mac_capabilities = MAC_10 | MAC_100 | MAC_1000 |
+					   MAC_SYM_PAUSE | MAC_ASYM_PAUSE;
+		return;
+	}
+
+	phy_interface_set_rgmii(config->supported_interfaces);

And flipping that around, if the core supports GMII, could i
instantiate a GMII user port?

+static const struct dsa_switch_ops soce_switch_ops = {
+	.get_tag_protocol	= soce_get_tag_protocol,
+	.setup			= soce_setup,
+	.teardown		= soce_teardown,
+	.phylink_get_caps	= soce_phylink_get_caps,
+	.phy_read		= soce_phy_read,
+	.phy_write		= soce_phy_write,

phy_read and phy_write are legacy. Instantiate a Linux MDIO bus and
don't list anything here.

+static const struct soce_variant_desc *
+soce_match_variant(const char *compatible)
+{
+	int i;
+
+	for (i = 0; soce_variants[i].compatible; i++) {
+		if (!strcmp(compatible, soce_variants[i].compatible))
+			return &soce_variants[i];
+	}
+
+	return NULL;

This kind of looks like a reinvention of of_device_get_match_data()

+static int soce_sw_parse_port_mdio_config(struct soce_dsa_local *local,
+					  struct device *dev,
+					  struct device_node *port_node,
+					  u32 port)
+{
+	struct device_node *phy_node;
+	u32 val;
+	int ret;
+
+	if (of_find_property(port_node, "ethernet", NULL) ||
+	    of_find_property(port_node, "link", NULL)) {
+		if (of_find_property(port_node, "phy-handle", NULL)) {
+			dev_err(dev,
+				"phy-handle not allowed on CPU/DSA port %u\n",
+				port);
+			return -EINVAL;
+		}
+
+		return 0;
+	}

Why is all of this parsing of DT needed? These should all be standard
properties which the DSA core handles.

This currently has the feel of being a "vendor crap" driver. Please
spend some time to clean this driver up, bring it up to Mainline
standards.

	Andrew