Re: [PATCH net-next v4 2/3] net: dsa: add Arrow SpeedChips XRS700x driver

On Wed, Jan 13, 2021 at 08:59:21AM -0600, George McCollister wrote:

Add a driver with initial support for the Arrow SpeedChips XRS7000
series of gigabit Ethernet switch chips which are typically used in
critical networking applications.

The switches have up to three RGMII ports and one RMII port.
Management to the switches can be performed over i2c or mdio.

Support for advanced features such as PTP and
HSR/PRP (IEC 62439-3 Clause 5 & 4) is not included in this patch and
may be added at a later date.

Signed-off-by: George McCollister <george.mccollister@gmail.com>
---

Some non-exhaustive feedback below.

+static void xrs700x_teardown(struct dsa_switch *ds)
+{
+	struct xrs700x *priv = ds->priv;
+
+	cancel_delayed_work_sync(&priv->mib_work);
+}
+

+static void xrs700x_mac_link_up(struct dsa_switch *ds, int port,
+				unsigned int mode, phy_interface_t interface,
+				struct phy_device *phydev,
+				int speed, int duplex,
+				bool tx_pause, bool rx_pause)
+{
+	struct xrs700x *priv = ds->priv;
+	unsigned int val;
+
+	switch (speed) {
+	case SPEED_1000:
+		val = XRS_PORT_SPEED_1000;
+		break;
+	case SPEED_100:
+		val = XRS_PORT_SPEED_100;
+		break;
+	case SPEED_10:
+		val = XRS_PORT_SPEED_10;
+		break;
+	default:
+		return;
+	}
+
+	regmap_fields_write(priv->ps_sel_speed, port, val);
+
+	dev_dbg_ratelimited(priv->dev, "%s: port: %d mode: %u speed: %u\n",
+			    __func__, port, mode, speed);
+}

What PHY interface types does the switch support as of this patch?
No RGMII delay configuration needed?

+
+static int xrs700x_bridge_common(struct dsa_switch *ds, int port,
+				 struct net_device *bridge, bool join)
+{
+	unsigned int i, cpu_mask = 0, mask = 0;
+	struct xrs700x *priv = ds->priv;
+	int ret;
+
+	for (i = 0; i < ds->num_ports; i++) {
+		if (dsa_is_cpu_port(ds, i))
+			continue;
+
+		cpu_mask |= BIT(i);
+
+		if (dsa_to_port(ds, i)->bridge_dev == bridge)
+			continue;
+
+		mask |= BIT(i);
+	}
+
+	for (i = 0; i < ds->num_ports; i++) {
+		if (dsa_to_port(ds, i)->bridge_dev != bridge)
+			continue;
+
+		ret = regmap_write(priv->regmap, XRS_PORT_FWD_MASK(i), mask);

Maybe it would be worth mentioning in a comment that PORT_FWD_MASK's
encoding is "1 = Disable forwarding to the port", otherwise this is
confusing.

+		if (ret)
+			return ret;
+	}
+
+	if (!join) {
+		ret = regmap_write(priv->regmap, XRS_PORT_FWD_MASK(port),
+				   cpu_mask);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}

+static int xrs700x_detect(struct xrs700x *dev)
+{
+	const struct xrs700x_info *info;
+	unsigned int id;
+	int ret;
+
+	ret = regmap_read(dev->regmap, XRS_DEV_ID0, &id);
+	if (ret) {
+		dev_err(dev->dev, "error %d while reading switch id.\n",
+			ret);
+		return ret;
+	}
+
+	info = of_device_get_match_data(dev->dev);
+	if (!info)
+		return -EINVAL;
+
+	if (info->id == id) {
+		dev->ds->num_ports = info->num_ports;
+		dev_info(dev->dev, "%s detected.\n", info->name);
+		return 0;
+	}
+
+	dev_err(dev->dev, "expected switch id 0x%x but found 0x%x.\n",
+		info->id, id);

I've been there too, not the smartest of decisions in the long run. See
commit 0b0e299720bb ("net: dsa: sja1105: use detected device id instead
of DT one on mismatch") if you want a sneak preview of how this is going
to feel two years from now. If you can detect the device id you're
probably better off with a single compatible string.

+
+	return -ENODEV;
+}
+
+static int xrs700x_alloc_port_mib(struct xrs700x *dev, int port)
+{
+	struct xrs700x_port *p = &dev->ports[port];
+	size_t mib_size = sizeof(*p->mib_data) * ARRAY_SIZE(xrs700x_mibs);

Reverse Christmas tree ordering... sorry.

+int xrs700x_switch_register(struct xrs700x *dev)
+{
+	int ret;
+	int i;
+
+	ret = xrs700x_detect(dev);
+	if (ret)
+		return ret;
+
+	ret = xrs700x_setup_regmap_range(dev);
+	if (ret)
+		return ret;
+
+	dev->ports = devm_kzalloc(dev->dev,
+				  sizeof(*dev->ports) * dev->ds->num_ports,
+				  GFP_KERNEL);

devm_kcalloc?

+	if (!dev->ports)
+		return -ENOMEM;
+
+	for (i = 0; i < dev->ds->num_ports; i++) {
+		ret = xrs700x_alloc_port_mib(dev, i);
+		if (ret)
+			return ret;
+	}
+
+	ret = dsa_register_switch(dev->ds);
+
+	return ret;

return dsa_register_switch

+}
+EXPORT_SYMBOL(xrs700x_switch_register);
+
+void xrs700x_switch_remove(struct xrs700x *dev)
+{
+	cancel_delayed_work_sync(&dev->mib_work);

Is it not enough that this is called from xrs700x_teardown too, which is
in the call path of dsa_unregister_switch below?

What happened to bit 0 of "reg"?

+
+	ret = mdiobus_write(mdiodev->bus, mdiodev->addr, XRS_MDIO_IBA0, uval);
+	if (ret < 0) {
+		dev_err(dev, "xrs mdiobus_write returned %d\n", ret);
+		return ret;
+	}
+
+	ret = mdiobus_read(mdiodev->bus, mdiodev->addr, XRS_MDIO_IBD);
+	if (ret < 0) {
+		dev_err(dev, "xrs mdiobus_read returned %d\n", ret);
+		return ret;
+	}
+
+	*val = (unsigned int)ret;
+
+	return 0;
+}

+static int xrs700x_mdio_probe(struct mdio_device *mdiodev)
+{
+	struct xrs700x *dev;

May boil down to preference too, but I don't believe "dev" is a happy
name to give to a driver private data structure.