Re: [PATCH net-next v3 7/8] net: dsa: motorcomm: Split MDIO bus module

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Commit-message accuracy claim: "net: dsa: motorcomm: Split MDIO
  bus module" / "Split it in order to be used in other modules" while…

Pre-existing issues:
- [Medium] Binding-vs-driver mismatch for the `mdio-external` child bus:
  Documentation/devicetree/bindings/net/dsa/motorcomm,yt921x.yaml…

--

net: dsa: motorcomm: Split MDIO bus module

This moves the internal and external MDIO master bus accessors, along with
the YT921X_*_MBUS_* register definitions, out of chip.c into a new
mdio_bus.c/mdio_bus.h compilation unit that is linked into the same
yt921x module. The two bus init helpers lose their static qualifier so
they can be called from chip.c.

> diff --git a/drivers/net/dsa/motorcomm/mdio_bus.c b/drivers/net/dsa/motorcomm/mdio_bus.c
> new file mode 100644
> index 0000000000000..ba70343f3f330
> --- /dev/null
> +++ b/drivers/net/dsa/motorcomm/mdio_bus.c
> @@ -0,0 +1,278 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2026 David Yang
> + */
> +
> +#include <linux/of_mdio.h>
> +
> +#include "chip.h"
> +#include "mdio_bus.h"
> +#include "smi.h"

[ ... ]

> +int yt921x_mbus_ext_init(struct yt921x_priv *priv, struct device_node *mnp)
> +{
> +	struct device *dev = to_device(priv);
> +	struct mii_bus *mbus;
> +	int res;
> +
> +	mbus = devm_mdiobus_alloc(dev);
> +	if (!mbus)
> +		return -ENOMEM;
> +
> +	mbus->name = "YT921x external MDIO bus";
> +	snprintf(mbus->id, MII_BUS_ID_SIZE, "%s@ext", dev_name(dev));
> +	mbus->priv = priv;
> +	/* TODO: c45? */
> +	mbus->read = yt921x_mbus_ext_read;
> +	mbus->write = yt921x_mbus_ext_write;
> +	mbus->parent = dev;
> +
> +	res = devm_of_mdiobus_register(dev, mbus, mnp);
> +	if (res)
> +		return res;
> +
> +	priv->mbus_ext = mbus;
> +
> +	return 0;
> +}

[Severity: Medium]

This isn't something introduced by this patch, it is pre-existing behaviour
that just becomes more visible now that the external bus registration is
exported as shared driver infrastructure.

Can yt921x_mbus_ext_init() ever be used successfully?  Its only caller,
yt921x_dsa_setup() in chip.c, registers the bus and then aborts the switch
setup unconditionally:

	/* External mdio bus is optional */
	child = of_get_child_by_name(np, "mdio-external");
	if (child) {
		res = yt921x_mbus_ext_init(priv, child);
		of_node_put(child);
		if (res)
			return res;

		dev_err(dev, "Untested external mdio bus\n");
		return -ENODEV;
	}

At the same time,
Documentation/devicetree/bindings/net/dsa/motorcomm,yt921x.yaml lists
mdio-external as a supported optional MDIO bus and its example instantiates
it:

	mdio-external {
		phy@b { reg = <0xb>; };
	};

with ethernet-port@9 referencing that phy via phy-handle.

So does a device tree written exactly as the in-tree binding documents make
the whole switch fail to probe with -ENODEV?  Would it make sense to either
drop mdio-external and the corresponding example from the binding, or drop
the -ENODEV bail-out, in a separate patch?

> diff --git a/drivers/net/dsa/motorcomm/mdio_bus.h b/drivers/net/dsa/motorcomm/mdio_bus.h
> new file mode 100644
> index 0000000000000..ae5792b8da24c
> --- /dev/null
> +++ b/drivers/net/dsa/motorcomm/mdio_bus.h
> @@ -0,0 +1,39 @@

[ ... ]

> +struct yt921x_priv;
> +
> +int yt921x_mbus_int_init(struct yt921x_priv *priv, struct device_node *mnp);
> +int yt921x_mbus_ext_init(struct yt921x_priv *priv, struct device_node *mnp);
> +
> +#endif

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917185010.3189199-1-mmyangfl%40gmail.com