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

On Thu, 2026-07-30 at 11:01 +0200, Krzysztof Kozlowski wrote:
> On Wed, Jul 29, 2026 at 06:36:57PM +0200, Vasilij Strassheim wrote:
> > +static const struct soce_variant_desc soce_variants[] = {
> > +	{
> > +		.compatible		= "soce,mrs-21-01",
> > +		.mdio_master_offset	= SOCE_MRS_21_01_MDIO_MASTER_OFFSET,
> > +		.layout			= &soce_mrs_21_01_layout,
> > +		.mdio_ops		= &soce_mdio_ops_20_03,
> > +	},
> > +	{
> > +		.compatible		= "soce,mrs-23-02",
> > +		.mdio_master_offset	= SOCE_MRS_21_01_MDIO_MASTER_OFFSET,
> > +		.layout			= &soce_mrs_24_01_layout,
> > +		.mdio_ops		= &soce_mdio_ops_c22_c45,
> > +	},
> > +	{
> > +		.compatible		= "soce,mrs-24-01",
> > +		.mdio_master_offset	= SOCE_MRS_21_01_MDIO_MASTER_OFFSET,
> > +		.layout			= &soce_mrs_24_01_layout,
> > +		.mdio_ops		= &soce_mdio_ops_c22_c45,
> > +	},
> > +	{ /* sentinel */ },
> > +};
> > +
> > +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];
> 
> Do not re-invent OF matching. This is supposed to be of_device_id table.

I will change that.

> 
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> > +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;
> > +	}
> > +
> > +	phy_node = of_parse_phandle(port_node, "phy-handle", 0);
> > +	if (!phy_node) {
> > +		if (of_phy_is_fixed_link(port_node))
> > +			return 0;
> > +
> > +		dev_err(dev, "user port %u requires phy-handle or fixed-link\n",
> > +			port);
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = of_property_read_u32(phy_node, "soce,mdio-output", &val);
> > +	if (ret) {
> > +		dev_err(dev,
> > +			"missing soce,mdio-output in %pOF referenced by port %u\n",
> > +			phy_node, port);
> > +		goto out_put_phy;
> > +	}
> > +	local->mdio_info[port].mdio_output = val;
> > +
> > +	ret = of_property_read_u32(phy_node, "soce,phy-addr", &val);
> > +	if (ret) {
> > +		dev_err(dev,
> > +			"missing soce,phy-addr in %pOF referenced by port %u\n",
> > +			phy_node, port);
> > +		goto out_put_phy;
> > +	}
> > +	local->mdio_info[port].phy_addr = val;
> > +
> > +out_put_phy:
> > +	of_node_put(phy_node);
> > +	return ret;
> > +}
> > +
> > +static int soce_sw_parse_port_mdio(struct soce_priv *priv, struct device *dev,
> > +				   u32 numports)
> > +{
> > +	struct soce_dsa_local *local = &priv->local;
> > +	struct device_node *ports_node;
> > +	struct device_node *port_node;
> > +	u32 port;
> > +	int ret;
> > +
> > +	ports_node = of_get_child_by_name(dev->of_node, "ports");
> > +	if (!ports_node) {
> > +		dev_err(dev, "missing ports node\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	for_each_available_child_of_node(ports_node, port_node) {
> 
> Why not scoped?

I will change it to scoped.

> 
> > +		if (of_property_read_u32(port_node, "reg", &port))
> > +			continue;
> > +
> > +		if (port >= numports || port >= SOCE_MAX_NUM_PORTS) {
> > +			dev_warn(dev,
> > +				 "ignoring invalid port index %u in %pOF\n",
> > +				 port, port_node);
> > +			continue;
> > +		}
> > +
> > +		ret = soce_sw_parse_port_mdio_config(local, dev, port_node,
> > +						     port);
> > +		if (ret) {
> > +			of_node_put(port_node);
> > +			of_node_put(ports_node);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	of_node_put(ports_node);
> > +	return 0;
> > +}
> > +
> > +static int soce_sw_probe(struct mdio_device *mdiodev)
> > +{
> > +	const struct soce_variant_desc *variant;
> > +	struct device *dev = &mdiodev->dev;
> > +	struct device_node *switch_node;
> > +	struct soce_dsa_local *local;
> > +	const char *soce_compatible;
> > +	struct soce_priv *priv;
> > +	u32 numports;
> > +	int ret;
> > +
> > +	priv = devm_kzalloc(&mdiodev->dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->ds = devm_kzalloc(&mdiodev->dev, sizeof(*priv->ds), GFP_KERNEL);
> > +	if (!priv->ds)
> > +		return -ENOMEM;
> > +
> > +	priv->ds->dev = dev;
> > +	priv->ds->priv = priv;
> > +	local = &priv->local;
> > +
> > +	switch_node = of_parse_phandle(dev->of_node, "soce,switch-ip", 0);
> > +	if (!switch_node) {
> > +		dev_err(dev, "missing or invalid switch IP reference\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = of_property_read_string(switch_node, "compatible",
> > +				      &soce_compatible);
> > +	if (ret) {
> > +		dev_err(dev, "missing compatible property in %pOF\n",
> > +			switch_node);
> > +		of_node_put(switch_node);
> > +		return -EINVAL;
> > +	}
> > +
> > +	variant = soce_match_variant(soce_compatible);
> > +	if (!variant) {
> > +		dev_err(dev, "unsupported compatible '%s' in %pOF\n",
> > +			soce_compatible, switch_node);
> > +		of_node_put(switch_node);
> 
> Use proper error handling exit paths with goto.

I will do it.

> 
> > +		return -ENODEV;
> > +	}
> > +
> > +	ret = of_property_read_u32(switch_node, "soce,num-ports", &numports);
> > +	if (ret) {
> > +		dev_err(dev, "missing soce,num-ports in %pOF\n", switch_node);
> > +		of_node_put(switch_node);
> > +		return -EINVAL;
> > +	}
> > +	if (numports == 0 || numports > SOCE_MAX_NUM_PORTS) {
> > +		dev_err(dev, "invalid soce,num-ports %u (max %d)\n", numports,
> > +			SOCE_MAX_NUM_PORTS);
> > +		of_node_put(switch_node);
> > +		return -EINVAL;
> > +	}
> > +
> > +	local->base_addr = devm_of_iomap(dev, switch_node, 0, NULL);
> > +	if (IS_ERR(local->base_addr)) {
> > +		dev_err(dev, "failed to map switch register space for %pOF\n",
> > +			switch_node);
> > +		of_node_put(switch_node);
> > +		return PTR_ERR(local->base_addr);
> > +	}
> > +
> > +	priv->ds->ops = &soce_switch_ops;
> > +	local->mdio_ops = variant->mdio_ops;
> > +	local->mdio_master_addr =
> > +		local->base_addr + variant->mdio_master_offset;
> > +	local->layout = variant->layout;
> > +	of_node_put(switch_node);
> > +
> > +	priv->ds->num_ports = numports;
> > +	ret = soce_sw_parse_port_mdio(priv, dev, numports);
> > +	if (ret)
> > +		return ret;
> > +
> > +	dev_set_drvdata(&mdiodev->dev, priv);
> > +
> > +	dev_info(dev, "soce %s SDSA driver probed.\n", soce_compatible);
> 
> Drop, driver should be silent on success.

Does this really apply to DSA drivers? I see some corresponding dev_info()
entries in mainlined drivers. See for example mv88e6060_probe(),
gswip_probe_common() and sja1105_probe().
And there are others as well, especially if you consider the _detect()
functions that are called from _probe().
I also find it very helpful for troubleshooting, so I'd like to keep it along
with some additional information about the hardware, just like other drivers.

> 
> > +
> > +	return dsa_register_switch(priv->ds);
> > +}
> > +
> > +static void soce_sw_remove(struct mdio_device *mdiodev)
> > +{
> > +	struct soce_priv *priv = dev_get_drvdata(&mdiodev->dev);
> > +
> > +	dsa_unregister_switch(priv->ds);
> > +}
> > +
> > +static const struct of_device_id soce_of_match[] = {
> > +	{ .compatible = "soce,switch-dsa" },
> > +	{ /* sentinel */ },
> > +};
> > +
> > +static struct mdio_driver soce_mdio_driver = {
> > +	.mdiodrv.driver = {
> > +		.name = "soce-switch-dsa",
> > +		.of_match_table = soce_of_match,
> > +	},
> > +	.probe  = soce_sw_probe,
> > +	.remove = soce_sw_remove,
> > +};
> 
> Best regards,
> Krzysztof
> 

Thanks,
Vasilij