* [PATCH net-next v10 1/4] net: dsa: motorcomm: Move to subdirectory
2026-08-16 19:38 [PATCH net-next v10 0/4] net: dsa: motorcomm: Add LED support David Yang
@ 2026-08-16 19:38 ` David Yang
2026-08-18 17:10 ` Jakub Kicinski
2026-08-16 19:38 ` [PATCH net-next v10 2/4] net: dsa: motorcomm: Split SMI module David Yang
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: David Yang @ 2026-08-16 19:38 UTC (permalink / raw)
To: netdev
Cc: David Yang, Andrew Lunn, Vladimir Oltean, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, devicetree, linux-kernel
yt921x is already the longest single-file DSA driver, so it's time to
split it into parts.
Signed-off-by: David Yang <mmyangfl@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
MAINTAINERS | 2 +-
drivers/net/dsa/Kconfig | 10 ++--------
drivers/net/dsa/Makefile | 2 +-
drivers/net/dsa/motorcomm/Kconfig | 8 ++++++++
drivers/net/dsa/motorcomm/Makefile | 3 +++
drivers/net/dsa/{yt921x.c => motorcomm/chip.c} | 2 +-
drivers/net/dsa/{yt921x.h => motorcomm/chip.h} | 0
7 files changed, 16 insertions(+), 11 deletions(-)
create mode 100644 drivers/net/dsa/motorcomm/Kconfig
create mode 100644 drivers/net/dsa/motorcomm/Makefile
rename drivers/net/dsa/{yt921x.c => motorcomm/chip.c} (99%)
rename drivers/net/dsa/{yt921x.h => motorcomm/chip.h} (100%)
diff --git a/MAINTAINERS b/MAINTAINERS
index 991460050da7..55cc5ac0e86f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18256,7 +18256,7 @@ M: David Yang <mmyangfl@gmail.com>
L: netdev@vger.kernel.org
S: Maintained
F: Documentation/devicetree/bindings/net/dsa/motorcomm,yt921x.yaml
-F: drivers/net/dsa/yt921x.*
+F: drivers/net/dsa/motorcomm/
F: net/dsa/tag_yt921x.c
MOXA SMARTIO/INDUSTIO/INTELLIO SERIAL CARD
diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
index 4ab567c5bbaf..98e9bbe47de7 100644
--- a/drivers/net/dsa/Kconfig
+++ b/drivers/net/dsa/Kconfig
@@ -72,6 +72,8 @@ config NET_DSA_MV88E6060
source "drivers/net/dsa/microchip/Kconfig"
+source "drivers/net/dsa/motorcomm/Kconfig"
+
source "drivers/net/dsa/mv88e6xxx/Kconfig"
source "drivers/net/dsa/mxl862xx/Kconfig"
@@ -158,12 +160,4 @@ config NET_DSA_VITESSE_VSC73XX_PLATFORM
This enables support for the Vitesse VSC7385, VSC7388, VSC7395
and VSC7398 SparX integrated ethernet switches, connected over
a CPU-attached address bus and work in memory-mapped I/O mode.
-
-config NET_DSA_YT921X
- tristate "Motorcomm YT9215 ethernet switch chip support"
- select NET_DSA_TAG_YT921X
- select NET_IEEE8021Q_HELPERS if DCB
- help
- This enables support for the Motorcomm YT9215 ethernet switch
- chip.
endmenu
diff --git a/drivers/net/dsa/Makefile b/drivers/net/dsa/Makefile
index d2975badffc0..138225baa4d5 100644
--- a/drivers/net/dsa/Makefile
+++ b/drivers/net/dsa/Makefile
@@ -14,11 +14,11 @@ obj-$(CONFIG_NET_DSA_SMSC_LAN9303_MDIO) += lan9303_mdio.o
obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX) += vitesse-vsc73xx-core.o
obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX_PLATFORM) += vitesse-vsc73xx-platform.o
obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX_SPI) += vitesse-vsc73xx-spi.o
-obj-$(CONFIG_NET_DSA_YT921X) += yt921x.o
obj-y += b53/
obj-y += hirschmann/
obj-y += lantiq/
obj-y += microchip/
+obj-y += motorcomm/
obj-y += mv88e6xxx/
obj-y += mxl862xx/
obj-y += netc/
diff --git a/drivers/net/dsa/motorcomm/Kconfig b/drivers/net/dsa/motorcomm/Kconfig
new file mode 100644
index 000000000000..1fddd386f866
--- /dev/null
+++ b/drivers/net/dsa/motorcomm/Kconfig
@@ -0,0 +1,8 @@
+# SPDX-License-Identifier: ISC
+config NET_DSA_YT921X
+ tristate "Motorcomm YT9215 ethernet switch chip support"
+ select NET_DSA_TAG_YT921X
+ select NET_IEEE8021Q_HELPERS if DCB
+ help
+ This enables support for the Motorcomm YT9215 ethernet switch
+ chip.
diff --git a/drivers/net/dsa/motorcomm/Makefile b/drivers/net/dsa/motorcomm/Makefile
new file mode 100644
index 000000000000..afd03be9fa35
--- /dev/null
+++ b/drivers/net/dsa/motorcomm/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: ISC
+obj-$(CONFIG_NET_DSA_YT921X) += yt921x.o
+yt921x-objs := chip.o
diff --git a/drivers/net/dsa/yt921x.c b/drivers/net/dsa/motorcomm/chip.c
similarity index 99%
rename from drivers/net/dsa/yt921x.c
rename to drivers/net/dsa/motorcomm/chip.c
index 159b16606f6c..f070732845eb 100644
--- a/drivers/net/dsa/yt921x.c
+++ b/drivers/net/dsa/motorcomm/chip.c
@@ -26,7 +26,7 @@
#include <net/ieee8021q.h>
#include <net/pkt_cls.h>
-#include "yt921x.h"
+#include "chip.h"
struct yt921x_mib_desc {
unsigned int size;
diff --git a/drivers/net/dsa/yt921x.h b/drivers/net/dsa/motorcomm/chip.h
similarity index 100%
rename from drivers/net/dsa/yt921x.h
rename to drivers/net/dsa/motorcomm/chip.h
--
2.53.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH net-next v10 1/4] net: dsa: motorcomm: Move to subdirectory
2026-08-16 19:38 ` [PATCH net-next v10 1/4] net: dsa: motorcomm: Move to subdirectory David Yang
@ 2026-08-18 17:10 ` Jakub Kicinski
0 siblings, 0 replies; 8+ messages in thread
From: Jakub Kicinski @ 2026-08-18 17:10 UTC (permalink / raw)
To: mmyangfl
Cc: Jakub Kicinski, netdev, andrew, olteanv, davem, edumazet, pabeni,
robh, krzk+dt, conor+dt, devicetree, linux-kernel
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: dsa: motorcomm: Move to subdirectory
The yt921x DSA driver is moved from drivers/net/dsa/yt921x.[ch] into a new
drivers/net/dsa/motorcomm/ subdirectory as chip.[ch], with the Kconfig
symbol and the build rule moved into new per-directory Kconfig and Makefile
files. MAINTAINERS is updated to match the new path.
> diff --git a/drivers/net/dsa/motorcomm/Kconfig b/drivers/net/dsa/motorcomm/Kconfig
> new file mode 100644
> index 0000000000000..1fddd386f866c
> --- /dev/null
> +++ b/drivers/net/dsa/motorcomm/Kconfig
> @@ -0,0 +1,8 @@
> +# SPDX-License-Identifier: ISC
> +config NET_DSA_YT921X
> + tristate "Motorcomm YT9215 ethernet switch chip support"
> + select NET_DSA_TAG_YT921X
> + select NET_IEEE8021Q_HELPERS if DCB
> + help
> + This enables support for the Motorcomm YT9215 ethernet switch
> + chip.
> diff --git a/drivers/net/dsa/motorcomm/Makefile b/drivers/net/dsa/motorcomm/Makefile
> new file mode 100644
> index 0000000000000..afd03be9fa35c
> --- /dev/null
> +++ b/drivers/net/dsa/motorcomm/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: ISC
^^^
[Severity: Low]
Is the ISC tag on the two new files intentional? Both
drivers/net/dsa/motorcomm/Kconfig and drivers/net/dsa/motorcomm/Makefile
start with:
# SPDX-License-Identifier: ISC
but the text in them was moved verbatim out of files carrying a different
tag:
drivers/net/dsa/Kconfig: # SPDX-License-Identifier: GPL-2.0-only
drivers/net/dsa/Makefile: # SPDX-License-Identifier: GPL-2.0
so the same config block and the same obj-$(CONFIG_NET_DSA_YT921X) line now
appear under two different licenses.
The sources these files build are also tagged differently:
drivers/net/dsa/motorcomm/chip.c: // SPDX-License-Identifier: GPL-2.0-or-later
drivers/net/dsa/motorcomm/chip.h: /* SPDX-License-Identifier: GPL-2.0-or-later */
ISC only exists in the tree as LICENSES/deprecated/ISC, and
Documentation/process/license-rules.rst says about that set:
2. Deprecated licenses:
These licenses should only be used for existing code or for importing
code from a different project.
These two files are newly created and not imported from another project.
Every other DSA subdirectory (b53, hirschmann, microchip, mv88e6xxx,
mxl862xx, netc, ocelot, qca, realtek, sja1105, xrs700x) uses a GPL-2.0
variant for its Kconfig and Makefile, and these are the only ISC-tagged
files under drivers/net/dsa/.
Would GPL-2.0-only (matching the files the text came from) or
GPL-2.0-or-later (matching chip.c and chip.h) be the right tag here? The
commit message describes the change as a mechanical relocation and does not
mention a licensing change.
> +obj-$(CONFIG_NET_DSA_YT921X) += yt921x.o
> +yt921x-objs := chip.o
[ ... ]
--
pw-bot: cr
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH net-next v10 2/4] net: dsa: motorcomm: Split SMI module
2026-08-16 19:38 [PATCH net-next v10 0/4] net: dsa: motorcomm: Add LED support David Yang
2026-08-16 19:38 ` [PATCH net-next v10 1/4] net: dsa: motorcomm: Move to subdirectory David Yang
@ 2026-08-16 19:38 ` David Yang
2026-08-18 17:10 ` Jakub Kicinski
2026-08-16 19:38 ` [PATCH net-next v10 3/4] net: dsa: motorcomm: Add LED support David Yang
2026-08-16 19:38 ` [PATCH net-next v10 4/4] dt-bindings: net: dsa: yt921x: Add LEDs definition example David Yang
3 siblings, 1 reply; 8+ messages in thread
From: David Yang @ 2026-08-16 19:38 UTC (permalink / raw)
To: netdev
Cc: David Yang, Andrew Lunn, Vladimir Oltean, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, devicetree, linux-kernel
SMI operations are going to be used across different modules. Minor
changes are applied to fix build errors.
Signed-off-by: David Yang <mmyangfl@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
drivers/net/dsa/motorcomm/Makefile | 1 +
drivers/net/dsa/motorcomm/chip.c | 207 +----------------------------
drivers/net/dsa/motorcomm/smi.c | 157 ++++++++++++++++++++++
drivers/net/dsa/motorcomm/smi.h | 92 +++++++++++++
4 files changed, 251 insertions(+), 206 deletions(-)
create mode 100644 drivers/net/dsa/motorcomm/smi.c
create mode 100644 drivers/net/dsa/motorcomm/smi.h
diff --git a/drivers/net/dsa/motorcomm/Makefile b/drivers/net/dsa/motorcomm/Makefile
index afd03be9fa35..6cea5313a444 100644
--- a/drivers/net/dsa/motorcomm/Makefile
+++ b/drivers/net/dsa/motorcomm/Makefile
@@ -1,3 +1,4 @@
# SPDX-License-Identifier: ISC
obj-$(CONFIG_NET_DSA_YT921X) += yt921x.o
yt921x-objs := chip.o
+yt921x-objs += smi.o
diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
index f070732845eb..6dee25b6754a 100644
--- a/drivers/net/dsa/motorcomm/chip.c
+++ b/drivers/net/dsa/motorcomm/chip.c
@@ -13,7 +13,6 @@
#include <linux/if_bridge.h>
#include <linux/if_hsr.h>
#include <linux/if_vlan.h>
-#include <linux/iopoll.h>
#include <linux/mdio.h>
#include <linux/module.h>
#include <linux/of.h>
@@ -27,6 +26,7 @@
#include <net/pkt_cls.h>
#include "chip.h"
+#include "smi.h"
struct yt921x_mib_desc {
unsigned int size;
@@ -155,9 +155,6 @@ static const struct yt921x_info yt921x_infos[] = {
#define YT921X_VID_UNWARE 4095
-#define YT921X_POLL_SLEEP_US 10000
-#define YT921X_POLL_TIMEOUT_US 100000
-
/* The interval should be small enough to avoid overflow of 32bit MIBs.
*
* Until we can read MIBs from stats64 call directly (i.e. sleep
@@ -196,208 +193,6 @@ static u32 ethaddr_lo2_to_u32(const unsigned char *addr)
return (addr[4] << 8) | addr[5];
}
-static int yt921x_reg_read(struct yt921x_priv *priv, u32 reg, u32 *valp)
-{
- WARN_ON(!mutex_is_locked(&priv->reg_lock));
-
- return priv->reg_ops->read(priv->reg_ctx, reg, valp);
-}
-
-static int yt921x_reg_write(struct yt921x_priv *priv, u32 reg, u32 val)
-{
- WARN_ON(!mutex_is_locked(&priv->reg_lock));
-
- return priv->reg_ops->write(priv->reg_ctx, reg, val);
-}
-
-static int
-yt921x_reg_wait(struct yt921x_priv *priv, u32 reg, u32 mask, u32 *valp)
-{
- u32 val;
- int res;
- int ret;
-
- ret = read_poll_timeout(yt921x_reg_read, res,
- res || (val & mask) == *valp,
- YT921X_POLL_SLEEP_US, YT921X_POLL_TIMEOUT_US,
- false, priv, reg, &val);
- if (ret)
- return ret;
- if (res)
- return res;
-
- *valp = val;
- return 0;
-}
-
-static int
-yt921x_reg_update_bits(struct yt921x_priv *priv, u32 reg, u32 mask, u32 val)
-{
- int res;
- u32 v;
- u32 u;
-
- res = yt921x_reg_read(priv, reg, &v);
- if (res)
- return res;
-
- u = v;
- u &= ~mask;
- u |= val;
- if (u == v)
- return 0;
-
- return yt921x_reg_write(priv, reg, u);
-}
-
-static int yt921x_reg_set_bits(struct yt921x_priv *priv, u32 reg, u32 mask)
-{
- return yt921x_reg_update_bits(priv, reg, 0, mask);
-}
-
-static int yt921x_reg_clear_bits(struct yt921x_priv *priv, u32 reg, u32 mask)
-{
- return yt921x_reg_update_bits(priv, reg, mask, 0);
-}
-
-static int
-yt921x_reg_toggle_bits(struct yt921x_priv *priv, u32 reg, u32 mask, bool set)
-{
- return yt921x_reg_update_bits(priv, reg, mask, !set ? 0 : mask);
-}
-
-/* Some multi-word registers, like VLANn_CTRL, should be treated as a single
- * long register. More specifically, writes to parts of its words won't become
- * visible, until the last word is written.
- *
- * Here we require full read and write operations over these registers to
- * eliminate potential issues, although partial reads/writes are also possible.
- */
-
-static void update_ctrls_unaligned(u32 *lo, u32 *hi, u64 mask, u64 val)
-{
- *lo &= ~lower_32_bits(mask);
- *hi &= ~upper_32_bits(mask);
- *lo |= lower_32_bits(val);
- *hi |= upper_32_bits(val);
-}
-
-static int
-yt921x_regs_read(struct yt921x_priv *priv, u32 reg, u32 *vals,
- unsigned int num_regs)
-{
- int res;
-
- for (unsigned int i = 0; i < num_regs; i++) {
- res = yt921x_reg_read(priv, reg + 4 * i, &vals[i]);
- if (res)
- return res;
- }
-
- return 0;
-}
-
-static int
-yt921x_regs_write(struct yt921x_priv *priv, u32 reg, const u32 *vals,
- unsigned int num_regs)
-{
- int res;
-
- for (unsigned int i = 0; i < num_regs; i++) {
- res = yt921x_reg_write(priv, reg + 4 * i, vals[i]);
- if (res)
- return res;
- }
-
- return 0;
-}
-
-static int
-yt921x_regs_update_bits(struct yt921x_priv *priv, u32 reg, const u32 *masks,
- const u32 *vals, unsigned int num_regs)
-{
- bool changed = false;
- u32 vs[4];
- int res;
-
- BUILD_BUG_ON(num_regs > ARRAY_SIZE(vs));
-
- res = yt921x_regs_read(priv, reg, vs, num_regs);
- if (res)
- return res;
-
- for (unsigned int i = 0; i < num_regs; i++) {
- u32 u = vs[i];
-
- u &= ~masks[i];
- u |= vals[i];
- if (u != vs[i])
- changed = true;
-
- vs[i] = u;
- }
-
- if (!changed)
- return 0;
-
- return yt921x_regs_write(priv, reg, vs, num_regs);
-}
-
-static int
-yt921x_regs_clear_bits(struct yt921x_priv *priv, u32 reg, const u32 *masks,
- unsigned int num_regs)
-{
- bool changed = false;
- u32 vs[4];
- int res;
-
- BUILD_BUG_ON(num_regs > ARRAY_SIZE(vs));
-
- res = yt921x_regs_read(priv, reg, vs, num_regs);
- if (res)
- return res;
-
- for (unsigned int i = 0; i < num_regs; i++) {
- u32 u = vs[i];
-
- u &= ~masks[i];
- if (u != vs[i])
- changed = true;
-
- vs[i] = u;
- }
-
- if (!changed)
- return 0;
-
- return yt921x_regs_write(priv, reg, vs, num_regs);
-}
-
-static int
-yt921x_reg64_write(struct yt921x_priv *priv, u32 reg, const u32 *vals)
-{
- return yt921x_regs_write(priv, reg, vals, 2);
-}
-
-static int
-yt921x_reg64_update_bits(struct yt921x_priv *priv, u32 reg, const u32 *masks,
- const u32 *vals)
-{
- return yt921x_regs_update_bits(priv, reg, masks, vals, 2);
-}
-
-static int
-yt921x_reg64_clear_bits(struct yt921x_priv *priv, u32 reg, const u32 *masks)
-{
- return yt921x_regs_clear_bits(priv, reg, masks, 2);
-}
-
-static int
-yt921x_reg96_write(struct yt921x_priv *priv, u32 reg, const u32 *vals)
-{
- return yt921x_regs_write(priv, reg, vals, 3);
-}
-
static int yt921x_reg_mdio_read(void *context, u32 reg, u32 *valp)
{
struct yt921x_reg_mdio *mdio = context;
diff --git a/drivers/net/dsa/motorcomm/smi.c b/drivers/net/dsa/motorcomm/smi.c
new file mode 100644
index 000000000000..164c25ef494a
--- /dev/null
+++ b/drivers/net/dsa/motorcomm/smi.c
@@ -0,0 +1,157 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2026 David Yang
+ */
+
+#include <linux/iopoll.h>
+
+#include "chip.h"
+#include "smi.h"
+
+#define YT921X_POLL_SLEEP_US 10000
+#define YT921X_POLL_TIMEOUT_US 100000
+
+int yt921x_reg_read(struct yt921x_priv *priv, u32 reg, u32 *valp)
+{
+ WARN_ON(!mutex_is_locked(&priv->reg_lock));
+
+ return priv->reg_ops->read(priv->reg_ctx, reg, valp);
+}
+
+int yt921x_reg_write(struct yt921x_priv *priv, u32 reg, u32 val)
+{
+ WARN_ON(!mutex_is_locked(&priv->reg_lock));
+
+ return priv->reg_ops->write(priv->reg_ctx, reg, val);
+}
+
+int yt921x_reg_wait(struct yt921x_priv *priv, u32 reg, u32 mask, u32 *valp)
+{
+ u32 val;
+ int res;
+ int ret;
+
+ ret = read_poll_timeout(yt921x_reg_read, res,
+ res || (val & mask) == *valp,
+ YT921X_POLL_SLEEP_US, YT921X_POLL_TIMEOUT_US,
+ false, priv, reg, &val);
+ if (ret)
+ return ret;
+ if (res)
+ return res;
+
+ *valp = val;
+ return 0;
+}
+
+int yt921x_reg_update_bits(struct yt921x_priv *priv, u32 reg, u32 mask, u32 val)
+{
+ int res;
+ u32 v;
+ u32 u;
+
+ res = yt921x_reg_read(priv, reg, &v);
+ if (res)
+ return res;
+
+ u = v;
+ u &= ~mask;
+ u |= val;
+ if (u == v)
+ return 0;
+
+ return yt921x_reg_write(priv, reg, u);
+}
+
+int
+yt921x_regs_read(struct yt921x_priv *priv, u32 reg, u32 *vals,
+ unsigned int num_regs)
+{
+ int res;
+
+ for (unsigned int i = 0; i < num_regs; i++) {
+ res = yt921x_reg_read(priv, reg + 4 * i, &vals[i]);
+ if (res)
+ return res;
+ }
+
+ return 0;
+}
+
+int
+yt921x_regs_write(struct yt921x_priv *priv, u32 reg, const u32 *vals,
+ unsigned int num_regs)
+{
+ int res;
+
+ for (unsigned int i = 0; i < num_regs; i++) {
+ res = yt921x_reg_write(priv, reg + 4 * i, vals[i]);
+ if (res)
+ return res;
+ }
+
+ return 0;
+}
+
+int
+yt921x_regs_update_bits(struct yt921x_priv *priv, u32 reg, const u32 *masks,
+ const u32 *vals, unsigned int num_regs)
+{
+ bool changed = false;
+ u32 vs[4];
+ int res;
+
+ if (WARN_ON_ONCE(num_regs > ARRAY_SIZE(vs)))
+ return -EINVAL;
+
+ res = yt921x_regs_read(priv, reg, vs, num_regs);
+ if (res)
+ return res;
+
+ for (unsigned int i = 0; i < num_regs; i++) {
+ u32 u = vs[i];
+
+ u &= ~masks[i];
+ u |= vals[i];
+ if (u != vs[i])
+ changed = true;
+
+ vs[i] = u;
+ }
+
+ if (!changed)
+ return 0;
+
+ return yt921x_regs_write(priv, reg, vs, num_regs);
+}
+
+int
+yt921x_regs_clear_bits(struct yt921x_priv *priv, u32 reg, const u32 *masks,
+ unsigned int num_regs)
+{
+ bool changed = false;
+ u32 vs[4];
+ int res;
+
+ if (WARN_ON_ONCE(num_regs > ARRAY_SIZE(vs)))
+ return -EINVAL;
+
+ res = yt921x_regs_read(priv, reg, vs, num_regs);
+ if (res)
+ return res;
+
+ for (unsigned int i = 0; i < num_regs; i++) {
+ u32 u = vs[i];
+
+ u &= ~masks[i];
+ if (u != vs[i])
+ changed = true;
+
+ vs[i] = u;
+ }
+
+ if (!changed)
+ return 0;
+
+ return yt921x_regs_write(priv, reg, vs, num_regs);
+}
diff --git a/drivers/net/dsa/motorcomm/smi.h b/drivers/net/dsa/motorcomm/smi.h
new file mode 100644
index 000000000000..2995e91f3b3d
--- /dev/null
+++ b/drivers/net/dsa/motorcomm/smi.h
@@ -0,0 +1,92 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (c) 2026 David Yang
+ */
+
+#ifndef _YT_SMI_H
+#define _YT_SMI_H
+
+#include <linux/types.h>
+#include <linux/wordpart.h>
+
+struct yt921x_priv;
+
+int yt921x_reg_read(struct yt921x_priv *priv, u32 reg, u32 *valp);
+int yt921x_reg_write(struct yt921x_priv *priv, u32 reg, u32 val);
+int yt921x_reg_wait(struct yt921x_priv *priv, u32 reg, u32 mask, u32 *valp);
+int yt921x_reg_update_bits(struct yt921x_priv *priv, u32 reg, u32 mask,
+ u32 val);
+
+static inline int
+yt921x_reg_set_bits(struct yt921x_priv *priv, u32 reg, u32 mask)
+{
+ return yt921x_reg_update_bits(priv, reg, 0, mask);
+}
+
+static inline int
+yt921x_reg_clear_bits(struct yt921x_priv *priv, u32 reg, u32 mask)
+{
+ return yt921x_reg_update_bits(priv, reg, mask, 0);
+}
+
+static inline int
+yt921x_reg_toggle_bits(struct yt921x_priv *priv, u32 reg, u32 mask, bool set)
+{
+ return yt921x_reg_update_bits(priv, reg, mask, !set ? 0 : mask);
+}
+
+/* Some multi-word registers, like VLANn_CTRL, should be treated as a single
+ * long register. More specifically, writes to parts of its words won't become
+ * visible, until the last word is written.
+ *
+ * Here we require full read and write operations over these registers to
+ * eliminate potential issues, although partial reads/writes are also possible.
+ */
+
+static inline void update_ctrls_unaligned(u32 *lo, u32 *hi, u64 mask, u64 val)
+{
+ *lo &= ~lower_32_bits(mask);
+ *hi &= ~upper_32_bits(mask);
+ *lo |= lower_32_bits(val);
+ *hi |= upper_32_bits(val);
+}
+
+/* yt921x_regs_*() are exported for linkage only. Do not use them directly. Use
+ * yt921x_reg64_*() or yt921x_reg96_*() instead.
+ */
+
+int yt921x_regs_read(struct yt921x_priv *priv, u32 reg, u32 *vals,
+ unsigned int num_regs);
+int yt921x_regs_write(struct yt921x_priv *priv, u32 reg, const u32 *vals,
+ unsigned int num_regs);
+int yt921x_regs_update_bits(struct yt921x_priv *priv, u32 reg, const u32 *masks,
+ const u32 *vals, unsigned int num_regs);
+int yt921x_regs_clear_bits(struct yt921x_priv *priv, u32 reg, const u32 *masks,
+ unsigned int num_regs);
+
+static inline int
+yt921x_reg64_write(struct yt921x_priv *priv, u32 reg, const u32 *vals)
+{
+ return yt921x_regs_write(priv, reg, vals, 2);
+}
+
+static inline int
+yt921x_reg64_update_bits(struct yt921x_priv *priv, u32 reg, const u32 *masks,
+ const u32 *vals)
+{
+ return yt921x_regs_update_bits(priv, reg, masks, vals, 2);
+}
+
+static inline int
+yt921x_reg64_clear_bits(struct yt921x_priv *priv, u32 reg, const u32 *masks)
+{
+ return yt921x_regs_clear_bits(priv, reg, masks, 2);
+}
+
+static inline int
+yt921x_reg96_write(struct yt921x_priv *priv, u32 reg, const u32 *vals)
+{
+ return yt921x_regs_write(priv, reg, vals, 3);
+}
+
+#endif
--
2.53.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH net-next v10 2/4] net: dsa: motorcomm: Split SMI module
2026-08-16 19:38 ` [PATCH net-next v10 2/4] net: dsa: motorcomm: Split SMI module David Yang
@ 2026-08-18 17:10 ` Jakub Kicinski
0 siblings, 0 replies; 8+ messages in thread
From: Jakub Kicinski @ 2026-08-18 17:10 UTC (permalink / raw)
To: mmyangfl
Cc: Jakub Kicinski, netdev, andrew, olteanv, davem, edumazet, pabeni,
robh, krzk+dt, conor+dt, devicetree, linux-kernel
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: dsa: motorcomm: Split SMI module
This moves the yt921x SMI register accessors out of chip.c into a new
smi.c/smi.h pair and adds smi.o to the module objects. The helpers gain
external linkage, and the thin wrappers around them become static inline in
the new header.
A few comments and questions below. The chip.c and Makefile hunks are
snipped since the comments all land on the new files.
> diff --git a/drivers/net/dsa/motorcomm/smi.c b/drivers/net/dsa/motorcomm/smi.c
> new file mode 100644
> index 0000000000000..164c25ef494a1
> --- /dev/null
> +++ b/drivers/net/dsa/motorcomm/smi.c
> @@ -0,0 +1,157 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2026 David Yang
> + */
> +
> +#include <linux/iopoll.h>
> +
> +#include "chip.h"
> +#include "smi.h"
> +
> +#define YT921X_POLL_SLEEP_US 10000
> +#define YT921X_POLL_TIMEOUT_US 100000
> +
> +int yt921x_reg_read(struct yt921x_priv *priv, u32 reg, u32 *valp)
> +{
> + WARN_ON(!mutex_is_locked(&priv->reg_lock));
> +
> + return priv->reg_ops->read(priv->reg_ctx, reg, valp);
> +}
> +
> +int yt921x_reg_write(struct yt921x_priv *priv, u32 reg, u32 val)
> +{
> + WARN_ON(!mutex_is_locked(&priv->reg_lock));
> +
> + return priv->reg_ops->write(priv->reg_ctx, reg, val);
> +}
[Severity: Low]
This is a pre-existing issue and not introduced by this patch, since these
two lines are moved verbatim out of chip.c.
Now that yt921x_reg_read() and yt921x_reg_write() are a cross-file (and per
the commit message cross-module) API, would lockdep_assert_held() be the
better assertion here?
mutex_is_locked(&priv->reg_lock) only reports that some task owns the mutex,
not that the current one does, so a new caller that forgets to take
priv->reg_lock passes the check whenever another thread happens to hold it.
Would WARN_ON_ONCE() also be preferable to WARN_ON() on a path that runs for
every single register access?
[ ... ]
> +int
> +yt921x_regs_update_bits(struct yt921x_priv *priv, u32 reg, const u32 *masks,
> + const u32 *vals, unsigned int num_regs)
> +{
> + bool changed = false;
> + u32 vs[4];
> + int res;
> +
> + if (WARN_ON_ONCE(num_regs > ARRAY_SIZE(vs)))
> + return -EINVAL;
[Severity: Low]
The chip.c versions guarded the vs[4] stack buffer at compile time:
BUILD_BUG_ON(num_regs > ARRAY_SIZE(vs));
and that becomes a runtime warning here and in yt921x_regs_clear_bits().
Is the reason that num_regs is no longer a constant expression inside the
callee once the function has external linkage?
No overflow looks reachable today: the only callers go through the
yt921x_reg64_*()/yt921x_reg96_*() wrappers in smi.h with a literal 2 or 3.
Would it be worth naming this conversion in the commit message instead of
just "Minor changes are applied to fix build errors"? The lengths of the
caller supplied masks[] and vals[] arrays also remain unchecked, which is
worth a word since the helpers are now callable from other files.
[ ... ]
> diff --git a/drivers/net/dsa/motorcomm/smi.h b/drivers/net/dsa/motorcomm/smi.h
> new file mode 100644
> index 0000000000000..2995e91f3b3d1
> --- /dev/null
> +++ b/drivers/net/dsa/motorcomm/smi.h
> @@ -0,0 +1,92 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (c) 2026 David Yang
> + */
> +
> +#ifndef _YT_SMI_H
> +#define _YT_SMI_H
[Severity: Low]
This isn't a bug, but the guard name here is _YT_SMI_H while the sibling
private header in the same directory uses __YT921X_H. Was the difference
intended?
[ ... ]
> +static inline void update_ctrls_unaligned(u32 *lo, u32 *hi, u64 mask, u64 val)
> +{
> + *lo &= ~lower_32_bits(mask);
> + *hi &= ~upper_32_bits(mask);
> + *lo |= lower_32_bits(val);
> + *hi |= upper_32_bits(val);
> +}
[Severity: Low]
Related to the above, and also not a bug: this helper is the only symbol in
the header without the yt921x_ prefix that everything else uses, and it is
now visible to every file that includes smi.h.
Its only users are the two METER control word calls in chip.c:
update_ctrls_unaligned(&ctrls[0], &ctrls[1],
YT921X_METER_CTRLab_EBS_M, ...);
so could it stay a file local static in chip.c, or get a yt921x_ prefix if
it really needs to move?
> +
> +/* yt921x_regs_*() are exported for linkage only. Do not use them directly. Use
> + * yt921x_reg64_*() or yt921x_reg96_*() instead.
> + */
> +
> +int yt921x_regs_read(struct yt921x_priv *priv, u32 reg, u32 *vals,
> + unsigned int num_regs);
[Severity: Low]
Two things about this contract.
The comment above says "Here we require full read and write operations over
these registers", and this one says to use yt921x_reg64_*() or
yt921x_reg96_*() instead of the raw helpers. But the wrapper set is write
side only: yt921x_reg64_write(), yt921x_reg64_update_bits(),
yt921x_reg64_clear_bits() and yt921x_reg96_write(). There is no
yt921x_reg64_read() or yt921x_reg96_read(), so a module that wants the
required full read of something like VLANn_CTRL has no permitted call.
Should a read wrapper be added along with the split?
Second, yt921x_regs_read() is declared here "for linkage only", but its only
callers are yt921x_regs_update_bits() and yt921x_regs_clear_bits() in smi.c
itself, both at the time of this commit and at the end of the series. Can it
stay static in smi.c rather than becoming a global symbol?
[ ... ]
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH net-next v10 3/4] net: dsa: motorcomm: Add LED support
2026-08-16 19:38 [PATCH net-next v10 0/4] net: dsa: motorcomm: Add LED support David Yang
2026-08-16 19:38 ` [PATCH net-next v10 1/4] net: dsa: motorcomm: Move to subdirectory David Yang
2026-08-16 19:38 ` [PATCH net-next v10 2/4] net: dsa: motorcomm: Split SMI module David Yang
@ 2026-08-16 19:38 ` David Yang
2026-08-18 17:10 ` Jakub Kicinski
2026-08-16 19:38 ` [PATCH net-next v10 4/4] dt-bindings: net: dsa: yt921x: Add LEDs definition example David Yang
3 siblings, 1 reply; 8+ messages in thread
From: David Yang @ 2026-08-16 19:38 UTC (permalink / raw)
To: netdev
Cc: David Yang, Andrew Lunn, Vladimir Oltean, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, devicetree, linux-kernel
Each port has at most 3 LEDs connected and can be declared using the
standard LEDs structure.
Currently, only parallel mode and strict 1:1 mapping (i.e. the HW
default) are supported.
Signed-off-by: David Yang <mmyangfl@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
drivers/net/dsa/motorcomm/Kconfig | 9 +
drivers/net/dsa/motorcomm/Makefile | 1 +
drivers/net/dsa/motorcomm/chip.c | 13 +-
drivers/net/dsa/motorcomm/chip.h | 12 +
drivers/net/dsa/motorcomm/leds.c | 654 +++++++++++++++++++++++++++++
drivers/net/dsa/motorcomm/leds.h | 121 ++++++
6 files changed, 808 insertions(+), 2 deletions(-)
create mode 100644 drivers/net/dsa/motorcomm/leds.c
create mode 100644 drivers/net/dsa/motorcomm/leds.h
diff --git a/drivers/net/dsa/motorcomm/Kconfig b/drivers/net/dsa/motorcomm/Kconfig
index 1fddd386f866..3438e0b14361 100644
--- a/drivers/net/dsa/motorcomm/Kconfig
+++ b/drivers/net/dsa/motorcomm/Kconfig
@@ -6,3 +6,12 @@ config NET_DSA_YT921X
help
This enables support for the Motorcomm YT9215 ethernet switch
chip.
+
+config NET_DSA_YT921X_LEDS
+ bool "LED support for Motorcomm YT9215"
+ default y
+ depends on NET_DSA_YT921X
+ depends on LEDS_CLASS=y || LEDS_CLASS=NET_DSA_YT921X
+ help
+ This enables support for controlling the LEDs attached to the
+ Motorcomm YT9215 switch chips.
diff --git a/drivers/net/dsa/motorcomm/Makefile b/drivers/net/dsa/motorcomm/Makefile
index 6cea5313a444..5a63db0029ff 100644
--- a/drivers/net/dsa/motorcomm/Makefile
+++ b/drivers/net/dsa/motorcomm/Makefile
@@ -1,4 +1,5 @@
# SPDX-License-Identifier: ISC
obj-$(CONFIG_NET_DSA_YT921X) += yt921x.o
yt921x-objs := chip.o
+yt921x-$(CONFIG_NET_DSA_YT921X_LEDS) += leds.o
yt921x-objs += smi.o
diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
index 6dee25b6754a..553efacd4f5e 100644
--- a/drivers/net/dsa/motorcomm/chip.c
+++ b/drivers/net/dsa/motorcomm/chip.c
@@ -26,6 +26,7 @@
#include <net/pkt_cls.h>
#include "chip.h"
+#include "leds.h"
#include "smi.h"
struct yt921x_mib_desc {
@@ -151,8 +152,6 @@ static const struct yt921x_info yt921x_infos[] = {
{}
};
-#define YT921X_NAME "yt921x"
-
#define YT921X_VID_UNWARE 4095
/* The interval should be small enough to avoid overflow of 32bit MIBs.
@@ -4581,6 +4580,12 @@ static int yt921x_dsa_setup(struct dsa_switch *ds)
if (res)
return res;
+#if IS_ENABLED(CONFIG_NET_DSA_YT921X_LEDS)
+ res = yt921x_leds_setup(priv);
+ if (res)
+ dev_warn(dev, "Failed to setup LEDs: %d\n", res);
+#endif
+
return 0;
}
@@ -4681,6 +4686,10 @@ static void yt921x_mdio_remove(struct mdio_device *mdiodev)
if (!priv)
return;
+#if IS_ENABLED(CONFIG_NET_DSA_YT921X_LEDS)
+ yt921x_leds_remove(priv);
+#endif
+
for (size_t i = ARRAY_SIZE(priv->ports); i-- > 0; ) {
struct yt921x_port *pp = &priv->ports[i];
diff --git a/drivers/net/dsa/motorcomm/chip.h b/drivers/net/dsa/motorcomm/chip.h
index 5f3b99e189c4..e6815cf9cb3c 100644
--- a/drivers/net/dsa/motorcomm/chip.h
+++ b/drivers/net/dsa/motorcomm/chip.h
@@ -850,9 +850,14 @@ enum yt921x_fdb_entry_status {
#define YT921X_ACL_NUM (YT921X_ACL_BLK_NUM * YT921X_ACL_ENT_PER_BLK)
#define YT921X_UDF_NUM 8
+#define YT921X_LED_GROUP_NUM 3
+#define YT921X_LED_PORT_NUM 10
+
/* 8 internal + 2 external + 1 mcu */
#define YT921X_PORT_NUM 11
+#define YT921X_NAME "yt921x"
+
#define yt921x_port_is_internal(port) ((port) < 8)
#define yt921x_port_is_external(port) ((port) == 8 || (port) == 9)
@@ -938,6 +943,13 @@ struct yt921x_port {
struct yt921x_mib mib;
u64 rx_frames;
u64 tx_frames;
+
+#if IS_ENABLED(CONFIG_NET_DSA_YT921X_LEDS)
+ unsigned char led_duty;
+ unsigned short led_cycle;
+
+ struct yt921x_led *leds[YT921X_LED_GROUP_NUM];
+#endif
};
struct yt921x_reg_ops {
diff --git a/drivers/net/dsa/motorcomm/leds.c b/drivers/net/dsa/motorcomm/leds.c
new file mode 100644
index 000000000000..e7ef0ef80a26
--- /dev/null
+++ b/drivers/net/dsa/motorcomm/leds.c
@@ -0,0 +1,654 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2026 David Yang
+ */
+
+#include <linux/uleds.h>
+
+#include "chip.h"
+#include "leds.h"
+#include "smi.h"
+
+#define to_yt921x_led(led_cdev) \
+ container_of_const((led_cdev), struct yt921x_led, cdev)
+#define to_yt921x_port(led) ((led)->port)
+#define to_yt921x_priv(pp) \
+ container_of_const((pp), struct yt921x_priv, ports[(pp)->index])
+#define to_device(priv) ((priv)->ds.dev)
+
+static u32 yt921x_led_regaddr(struct yt921x_priv *priv, int port, int group)
+{
+ switch (group) {
+ case 0:
+ default:
+ return YT921X_LED0_PORTn(port);
+ case 1:
+ return YT921X_LED1_PORTn(port);
+ case 2:
+ return YT921X_LED2_PORTn(port);
+ }
+}
+
+static int
+yt921x_led_force_get(struct yt921x_priv *priv, int port, int group, bool *onp)
+{
+ u32 val;
+ int res;
+
+ res = yt921x_reg_read(priv, YT921X_LED2_PORTn(port), &val);
+ if (res)
+ return res;
+
+ *onp = (val & YT921X_LED2_PORT_FORCEn_M(group)) ==
+ YT921X_LED2_PORT_FORCEn_ON(group);
+ return 0;
+}
+
+static int
+yt921x_led_force_set(struct yt921x_priv *priv, int port, int group, bool on)
+{
+ struct yt921x_port *pp = &priv->ports[port];
+ struct yt921x_led *led = pp->leds[group];
+ u32 ctrl;
+ u32 mask;
+
+ led->use_cycle = false;
+ led->use_duty = false;
+
+ mask = YT921X_LED2_PORT_FORCEn_M(group);
+ ctrl = on ? YT921X_LED2_PORT_FORCEn_ON(group) :
+ YT921X_LED2_PORT_FORCEn_OFF(group);
+ return yt921x_reg_update_bits(priv, YT921X_LED2_PORTn(port), mask,
+ ctrl);
+}
+
+static int
+yt921x_led_blink_select(const struct yt921x_priv *priv, unsigned long on,
+ unsigned long off, unsigned short *cyclep,
+ unsigned char *dutyp)
+{
+ static const unsigned char dutys[] = {
+ YT921X_LED_DUTY(1, 6),
+ YT921X_LED_DUTY(1, 4),
+ YT921X_LED_DUTY(1, 3),
+ YT921X_LED_DUTY(1, 2),
+ };
+ unsigned int cycle_upper;
+ unsigned int cycle_req;
+ unsigned int duty_req;
+ unsigned int cycle;
+ unsigned int duty;
+
+ cycle = YT921X_LED_BLINK_MAX;
+ cycle_upper = cycle * 11585 / 8192 + 1; /* M_SQRT2 * cycle */
+ if (check_add_overflow(on, off, &cycle_req) || cycle_req >= cycle_upper)
+ return -EOPNOTSUPP;
+
+ for (; cycle > YT921X_LED_BLINK_MIN; cycle_upper >>= 1, cycle >>= 1)
+ if (cycle_req >= cycle_upper >> 1)
+ break;
+ *cyclep = cycle;
+
+ duty_req = DIV_ROUND_CLOSEST(YT921X_LED_DUTY_DENOM *
+ (on > off ? off : on), cycle_req);
+ for (unsigned int i = ARRAY_SIZE(dutys) - 1;; i--)
+ if (i == 0 || duty_req >= (dutys[i - 1] + dutys[i]) / 2) {
+ duty = dutys[i];
+ break;
+ }
+ if (on > off)
+ duty = YT921X_LED_DUTY_DENOM - duty;
+ *dutyp = duty;
+
+ return 0;
+}
+
+static int
+yt921x_led_blink_set(struct yt921x_priv *priv, int port, int group,
+ unsigned long *onp, unsigned long *offp)
+{
+ struct yt921x_port *pp = &priv->ports[port];
+ struct yt921x_led *led = pp->leds[group];
+ unsigned short cycle;
+ unsigned char duty;
+ bool use_cycle;
+ u32 ctrl;
+ u32 mask;
+ u32 val;
+ int res;
+
+ if (!*onp && !*offp) {
+ cycle = YT921X_LED_BLINK_DEF;
+ duty = YT921X_LED_DUTY(1, 2);
+ for (unsigned int i = 0; i < YT921X_LED_GROUP_NUM; i++)
+ if (i != group && pp->leds[i] &&
+ pp->leds[i]->use_duty) {
+ duty = pp->led_duty;
+ break;
+ }
+
+ use_cycle = false;
+ } else {
+ bool change_cycle;
+ bool change_duty;
+
+ res = yt921x_led_blink_select(priv, *onp, *offp, &cycle, &duty);
+ if (res)
+ return res;
+
+ use_cycle = cycle < YT921X_LED_BLINK_DEF;
+ change_cycle = use_cycle && cycle != pp->led_cycle;
+ change_duty = duty != pp->led_duty;
+ if (change_cycle || change_duty)
+ for (unsigned int i = 0; i < YT921X_LED_GROUP_NUM;
+ i++) {
+ if (i == group || !pp->leds[i])
+ continue;
+ if ((change_cycle && pp->leds[i]->use_cycle) ||
+ (change_duty && pp->leds[i]->use_duty))
+ return -EOPNOTSUPP;
+ }
+ }
+
+ /* The chip seems to jam a while if changing duty directly */
+ res = yt921x_reg_read(priv, YT921X_LED2_PORTn(port), &val);
+ if (res)
+ return res;
+
+ ctrl = val & ~YT921X_LED2_PORT_FORCEn_M(group);
+ ctrl |= YT921X_LED2_PORT_FORCEn_DONTCARE(group);
+ if (val != ctrl) {
+ res = yt921x_reg_write(priv, YT921X_LED2_PORTn(port), ctrl);
+ if (res)
+ return res;
+ }
+
+ mask = YT921X_LED1_PORT_BLINK_DUTY_M | YT921X_LED1_PORT_BLINK_DUTY_COMP;
+ switch (duty >= YT921X_LED_DUTY(1, 2) ? duty :
+ YT921X_LED_DUTY_DENOM - duty) {
+ default:
+ duty = YT921X_LED_DUTY(1, 2);
+ fallthrough;
+ case YT921X_LED_DUTY(1, 2):
+ ctrl = YT921X_LED1_PORT_BLINK_DUTY_1_2;
+ break;
+ case YT921X_LED_DUTY(2, 3):
+ ctrl = YT921X_LED1_PORT_BLINK_DUTY_2_3;
+ break;
+ case YT921X_LED_DUTY(3, 4):
+ ctrl = YT921X_LED1_PORT_BLINK_DUTY_3_4;
+ break;
+ case YT921X_LED_DUTY(5, 6):
+ ctrl = YT921X_LED1_PORT_BLINK_DUTY_5_6;
+ break;
+ }
+ if (duty < YT921X_LED_DUTY(1, 2))
+ ctrl |= YT921X_LED1_PORT_BLINK_DUTY_COMP;
+ if (use_cycle) {
+ mask |= YT921X_LED1_PORT_OTHER_BLINK_M;
+ ctrl |= YT921X_LED1_PORT_OTHER_BLINK(9 - __fls(cycle));
+ }
+ res = yt921x_reg_update_bits(priv, YT921X_LED1_PORTn(port), mask, ctrl);
+ if (res)
+ return res;
+
+ ctrl = val & ~(YT921X_LED2_PORT_FORCEn_M(group) |
+ YT921X_LED2_PORT_FORCE_BLINKn_M(group));
+ ctrl |= YT921X_LED2_PORT_FORCEn_BLINK(group);
+ if (use_cycle)
+ ctrl |= YT921X_LED2_PORT_FORCE_BLINKn_OTHER(group);
+ else
+ ctrl |= YT921X_LED2_PORT_FORCE_BLINKn(group, __fls(cycle) - 9);
+ res = yt921x_reg_write(priv, YT921X_LED2_PORTn(port), ctrl);
+ if (res)
+ return res;
+
+ led->use_cycle = use_cycle;
+ if (use_cycle)
+ pp->led_cycle = cycle;
+ led->use_duty = true;
+ pp->led_duty = duty;
+
+ *onp = DIV_ROUND_CLOSEST(duty * cycle, YT921X_LED_DUTY_DENOM);
+ *offp = cycle - *onp;
+ return 0;
+}
+
+struct yt921x_led_trigger_map {
+ unsigned long flags;
+ u32 mask;
+};
+
+static const struct yt921x_led_trigger_map yt921x_led_trigger_maps[] = {
+ {BIT(TRIGGER_NETDEV_LINK),
+ YT921X_LEDx_PORT_ACT_DUPLEX_HALF | YT921X_LEDx_PORT_ACT_DUPLEX_FULL},
+ {BIT(TRIGGER_NETDEV_LINK_10), YT921X_LEDx_PORT_ACT_10M},
+ {BIT(TRIGGER_NETDEV_LINK_100), YT921X_LEDx_PORT_ACT_100M},
+ {BIT(TRIGGER_NETDEV_LINK_1000), YT921X_LEDx_PORT_ACT_1000M},
+ {BIT(TRIGGER_NETDEV_HALF_DUPLEX), YT921X_LEDx_PORT_ACT_DUPLEX_HALF},
+ {BIT(TRIGGER_NETDEV_FULL_DUPLEX), YT921X_LEDx_PORT_ACT_DUPLEX_FULL},
+ {BIT(TRIGGER_NETDEV_TX), YT921X_LEDx_PORT_ACT_TX_BLINK},
+ {BIT(TRIGGER_NETDEV_RX), YT921X_LEDx_PORT_ACT_RX_BLINK},
+ {BIT(TRIGGER_NETDEV_TX_ERR) | BIT(TRIGGER_NETDEV_RX_ERR),
+ YT921X_LEDx_PORT_ACT_COLLISION_BLINK},
+};
+
+static bool
+yt921x_led_trigger_is_supported(const struct yt921x_priv *priv, int port,
+ int group, unsigned long flags)
+{
+ if (!flags)
+ return true;
+
+ for (unsigned int i = 0; i < ARRAY_SIZE(yt921x_led_trigger_maps); i++) {
+ const struct yt921x_led_trigger_map *map = &yt921x_led_trigger_maps[i];
+
+ if ((flags & map->flags) == map->flags) {
+ flags &= ~map->flags;
+ if (!flags)
+ return true;
+ }
+ }
+
+ return false;
+}
+
+static int
+yt921x_led_trigger_get(struct yt921x_priv *priv, int port, int group,
+ unsigned long *flagsp)
+{
+ u32 addr;
+ u32 val;
+ int res;
+
+ res = yt921x_reg_read(priv, YT921X_LED2_PORTn(port), &val);
+ if (res)
+ return res;
+
+ if ((val & YT921X_LED2_PORT_FORCEn_M(group)) !=
+ YT921X_LED2_PORT_FORCEn_DONTCARE(group)) {
+ *flagsp = 0;
+ return 0;
+ }
+
+ if (group != 2) {
+ addr = yt921x_led_regaddr(priv, port, group);
+ res = yt921x_reg_read(priv, addr, &val);
+ if (res)
+ return res;
+ }
+
+ *flagsp = 0;
+ for (unsigned int i = 0; i < ARRAY_SIZE(yt921x_led_trigger_maps); i++) {
+ const struct yt921x_led_trigger_map *map = &yt921x_led_trigger_maps[i];
+
+ if ((val & map->mask) == map->mask)
+ *flagsp |= map->flags;
+ }
+
+ return 0;
+}
+
+static int
+yt921x_led_trigger_set(struct yt921x_priv *priv, int port, int group,
+ unsigned long flags)
+{
+ struct yt921x_port *pp = &priv->ports[port];
+ struct yt921x_led *led = pp->leds[group];
+ u32 addr;
+ u32 ctrl;
+ u32 mask;
+ int res;
+
+ ctrl = 0;
+ for (unsigned int i = 0; i < ARRAY_SIZE(yt921x_led_trigger_maps); i++) {
+ const struct yt921x_led_trigger_map *map = &yt921x_led_trigger_maps[i];
+
+ if ((flags & map->flags) == map->flags) {
+ flags &= ~map->flags;
+ ctrl |= map->mask;
+ if (!flags)
+ break;
+ }
+ }
+ if (flags)
+ return -EOPNOTSUPP;
+
+ led->use_cycle = false;
+ led->use_duty = false;
+
+ mask = !group ? YT921X_LED0_PORT_ACT_M : YT921X_LEDx_PORT_ACT_M;
+ if (group == 2) {
+ mask |= YT921X_LED2_PORT_FORCEn_M(group);
+ ctrl |= YT921X_LED2_PORT_FORCEn_DONTCARE(group);
+ }
+ addr = yt921x_led_regaddr(priv, port, group);
+ res = yt921x_reg_update_bits(priv, addr, mask, ctrl);
+ if (res)
+ return res;
+
+ if (group != 2) {
+ mask = YT921X_LED2_PORT_FORCEn_M(group);
+ ctrl = YT921X_LED2_PORT_FORCEn_DONTCARE(group);
+ res = yt921x_reg_update_bits(priv, YT921X_LED2_PORTn(port),
+ mask, ctrl);
+ if (res)
+ return res;
+ }
+
+ return 0;
+}
+
+static int
+yt921x_cled_brightness_set_blocking(struct led_classdev *led_cdev,
+ enum led_brightness brightness)
+{
+ struct yt921x_led *led = to_yt921x_led(led_cdev);
+ struct yt921x_port *pp = to_yt921x_port(led);
+ struct yt921x_priv *priv = to_yt921x_priv(pp);
+ int res;
+
+ mutex_lock(&priv->reg_lock);
+ res = yt921x_led_force_set(priv, pp->index, led->group, brightness);
+ mutex_unlock(&priv->reg_lock);
+
+ return res;
+}
+
+static int
+yt921x_cled_blink_set(struct led_classdev *led_cdev, unsigned long *delay_on,
+ unsigned long *delay_off)
+{
+ struct yt921x_led *led = to_yt921x_led(led_cdev);
+ struct yt921x_port *pp = to_yt921x_port(led);
+ struct yt921x_priv *priv = to_yt921x_priv(pp);
+ int res;
+
+ mutex_lock(&priv->reg_lock);
+ res = yt921x_led_blink_set(priv, pp->index, led->group, delay_on,
+ delay_off);
+ mutex_unlock(&priv->reg_lock);
+
+ return res;
+}
+
+static struct device * __maybe_unused
+yt921x_cled_hw_control_get_device(struct led_classdev *led_cdev)
+{
+ struct yt921x_led *led = to_yt921x_led(led_cdev);
+ struct yt921x_port *pp = to_yt921x_port(led);
+ struct yt921x_priv *priv = to_yt921x_priv(pp);
+ struct dsa_port *dp;
+
+ dp = dsa_to_port(&priv->ds, pp->index);
+ if (!dp)
+ return NULL;
+
+ if (dsa_port_is_user(dp))
+ return &dp->user->dev;
+ if (dsa_port_is_cpu(dp))
+ return &dp->conduit->dev;
+
+ return NULL;
+}
+
+static int __maybe_unused
+yt921x_cled_hw_control_is_supported(struct led_classdev *led_cdev,
+ unsigned long flags)
+{
+ struct yt921x_led *led = to_yt921x_led(led_cdev);
+ struct yt921x_port *pp = to_yt921x_port(led);
+ struct yt921x_priv *priv = to_yt921x_priv(pp);
+
+ if (yt921x_led_trigger_is_supported(priv, pp->index, led->group, flags))
+ return 0;
+ return -EOPNOTSUPP;
+}
+
+static int __maybe_unused
+yt921x_cled_hw_control_get(struct led_classdev *led_cdev, unsigned long *flagsp)
+{
+ struct yt921x_led *led = to_yt921x_led(led_cdev);
+ struct yt921x_port *pp = to_yt921x_port(led);
+ struct yt921x_priv *priv = to_yt921x_priv(pp);
+ int res;
+
+ mutex_lock(&priv->reg_lock);
+ res = yt921x_led_trigger_get(priv, pp->index, led->group, flagsp);
+ mutex_unlock(&priv->reg_lock);
+
+ return res;
+}
+
+static int __maybe_unused
+yt921x_cled_hw_control_set(struct led_classdev *led_cdev, unsigned long flags)
+{
+ struct yt921x_led *led = to_yt921x_led(led_cdev);
+ struct yt921x_port *pp = to_yt921x_port(led);
+ struct yt921x_priv *priv = to_yt921x_priv(pp);
+ int res;
+
+ mutex_lock(&priv->reg_lock);
+ res = yt921x_led_trigger_set(priv, pp->index, led->group, flags);
+ mutex_unlock(&priv->reg_lock);
+
+ return res;
+}
+
+static int
+yt921x_led_setup(struct yt921x_priv *priv, int port,
+ struct fwnode_handle *fwnode)
+{
+ struct yt921x_port *pp = &priv->ports[port];
+ struct device *dev = to_device(priv);
+ struct led_init_data init_data;
+ struct led_classdev *led_cdev;
+ char name[LED_MAX_NAME_SIZE];
+ enum led_default_state state;
+ struct yt921x_led *led;
+ bool force_high;
+ bool force_low;
+ u32 group;
+ u32 mask;
+ u32 ctrl;
+ u32 val;
+ bool on;
+ int res;
+ int ret;
+
+ res = fwnode_property_read_u32(fwnode, "reg", &group);
+ if (res)
+ return res;
+ if (group >= YT921X_LED_GROUP_NUM) {
+ dev_err(dev, "Invalid LED reg %u for port %d\n", group, port);
+ return -EINVAL;
+ }
+ if (pp->leds[group])
+ return -EEXIST;
+
+ force_high = fwnode_property_read_bool(fwnode, "active-high");
+ force_low = fwnode_property_read_bool(fwnode, "active-low");
+ if (force_high && force_low) {
+ dev_err(dev, "Duplicate polarities for LED %02d:%02u\n",
+ port, group);
+ return -EINVAL;
+ }
+
+ led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
+ if (!led)
+ return -ENOMEM;
+ led->port = pp;
+ led->group = group;
+
+ state = led_init_default_state_get(fwnode);
+
+ mutex_lock(&priv->reg_lock);
+
+ /* Inversion is internal - force on will give low logic.
+ * In the rest of the file, treat LEDs as if active-low.
+ */
+ if (force_high || force_low) {
+ res = yt921x_reg_read(priv, YT921X_LED_PAR_INV, &val);
+ if (res)
+ goto unlock;
+
+ mask = YT921X_LED_PAR_INV_INVnm(group, port);
+ ctrl = force_high ? val | mask : val & ~mask;
+ res = yt921x_reg_write(priv, YT921X_LED_PAR_INV, ctrl);
+ if (res)
+ goto unlock;
+ }
+
+ pp->leds[group] = led;
+
+ switch (state) {
+ case LEDS_DEFSTATE_OFF:
+ case LEDS_DEFSTATE_ON:
+ default:
+ on = state == LEDS_DEFSTATE_ON;
+ res = yt921x_led_force_set(priv, port, group, on);
+ break;
+ case LEDS_DEFSTATE_KEEP:
+ res = yt921x_led_force_get(priv, port, group, &on);
+ break;
+ }
+
+unlock:
+ mutex_unlock(&priv->reg_lock);
+ if (res)
+ goto err;
+
+ led_cdev = &led->cdev;
+ led_cdev->brightness = on;
+ led_cdev->max_brightness = 1;
+ led_cdev->flags = LED_RETAIN_AT_SHUTDOWN;
+ led_cdev->brightness_set_blocking = yt921x_cled_brightness_set_blocking;
+ led_cdev->blink_set = yt921x_cled_blink_set;
+#ifdef CONFIG_LEDS_TRIGGERS
+ led_cdev->hw_control_trigger = "netdev";
+ led_cdev->hw_control_get_device = yt921x_cled_hw_control_get_device;
+ led_cdev->hw_control_is_supported = yt921x_cled_hw_control_is_supported;
+ led_cdev->hw_control_get = yt921x_cled_hw_control_get;
+ led_cdev->hw_control_set = yt921x_cled_hw_control_set;
+#endif
+
+ snprintf(name, sizeof(name), YT921X_NAME "-%u:%02d:%02u",
+ priv->ds.index, port, group);
+ init_data = (typeof(init_data)){
+ .fwnode = fwnode,
+ .default_label = ":port",
+ .devicename = name,
+ .devname_mandatory = true,
+ };
+ res = devm_led_classdev_register_ext(dev, led_cdev, &init_data);
+ if (res)
+ goto err;
+
+ return 0;
+
+err:
+ ret = res;
+
+ mutex_lock(&priv->reg_lock);
+ if (pp->leds[group]) {
+ if (force_high || force_low) {
+ res = yt921x_reg_write(priv, YT921X_LED_PAR_INV, val);
+ if (res)
+ dev_warn(dev,
+ "Failed to revoke initialization for LED %02d:%02u\n",
+ port, group);
+ }
+
+ pp->leds[group] = NULL;
+ }
+ mutex_unlock(&priv->reg_lock);
+
+ devm_kfree(dev, led);
+ return ret;
+}
+
+static void yt921x_leds_remove_port(struct yt921x_priv *priv, int port)
+{
+ struct yt921x_port *pp = &priv->ports[port];
+ struct device *dev = to_device(priv);
+ bool has_leds = false;
+
+ for (int group = 0; group < YT921X_LED_GROUP_NUM; group++) {
+ struct yt921x_led *led = pp->leds[group];
+
+ if (led) {
+ devm_led_classdev_unregister(dev, &led->cdev);
+ has_leds = true;
+ }
+ }
+
+ if (!has_leds)
+ return;
+
+ /* Now no one can access the LED control blocks, free them without
+ * holding the lock.
+ */
+ for (int group = 0; group < YT921X_LED_GROUP_NUM; group++) {
+ struct yt921x_led *led = pp->leds[group];
+
+ if (led) {
+ pp->leds[group] = NULL;
+ devm_kfree(dev, led);
+ }
+ }
+}
+
+static int yt921x_leds_setup_port(struct yt921x_priv *priv, int port)
+{
+ struct device *dev = to_device(priv);
+ struct dsa_switch *ds = &priv->ds;
+ struct device_node *leds_np;
+ struct dsa_port *dp;
+
+ dp = dsa_to_port(ds, port);
+ leds_np = of_get_child_by_name(dp->dn, "leds");
+ if (!leds_np)
+ return 0;
+
+ if (port >= YT921X_LED_PORT_NUM) {
+ dev_err(dev, "Cannot configure LEDs for port %d\n", port);
+ of_node_put(leds_np);
+ return -EINVAL;
+ }
+
+ for_each_child_of_node_scoped(leds_np, led_np) {
+ yt921x_led_setup(priv, port, of_fwnode_handle(led_np));
+ /* Allow partial configuration: LEDs are optional */
+ }
+
+ of_node_put(leds_np);
+ return 0;
+}
+
+void yt921x_leds_remove(struct yt921x_priv *priv)
+{
+ for (int port = 0; port < YT921X_LED_PORT_NUM; port++)
+ yt921x_leds_remove_port(priv, port);
+}
+
+int yt921x_leds_setup(struct yt921x_priv *priv)
+{
+ struct dsa_switch *ds = &priv->ds;
+ struct dsa_port *dp;
+
+ /* LEDs are always enabled. There is no way to disable them altogether
+ * (as far as I know).
+ */
+
+ dsa_switch_for_each_port(dp, ds) {
+ int port = dp->index;
+
+ if (!dp->dn)
+ continue;
+
+ yt921x_leds_setup_port(priv, port);
+ /* Allow partial configuration: LEDs are optional */
+ }
+
+ return 0;
+}
diff --git a/drivers/net/dsa/motorcomm/leds.h b/drivers/net/dsa/motorcomm/leds.h
new file mode 100644
index 000000000000..3c3642988ac7
--- /dev/null
+++ b/drivers/net/dsa/motorcomm/leds.h
@@ -0,0 +1,121 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (c) 2026 David Yang
+ */
+
+#ifndef _YT_LEDS_H
+#define _YT_LEDS_H
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/kconfig.h>
+#include <linux/leds.h>
+
+#define YT921X_LED_CTRL 0xd0000
+#define YT921X_LED_CTRL_UNK BIT(21)
+#define YT921X_LED_CTRL_LOOPDETECT_BLINK_M GENMASK(20, 19) /* cycle = 512 * x ms */
+#define YT921X_LED_CTRL_LOOPDETECT_BLINK(x) FIELD_PREP(YT921X_LED_CTRL_LOOPDETECT_BLINK_M, (x))
+#define YT921X_LED_CTRL_PORT_NUM_M GENMASK(16, 13)
+#define YT921X_LED_CTRL_PORT_NUM(x) FIELD_PREP(YT921X_LED_CTRL_PORT_NUM_M, (x))
+#define YT921X_LED_CTRL_MODE_M GENMASK(1, 0)
+#define YT921X_LED_CTRL_MODE(x) FIELD_PREP(YT921X_LED_CTRL_MODE_M, (x))
+#define YT921X_LED_CTRL_MODE_PARALLEL YT921X_LED_CTRL_MODE(0)
+#define YT921X_LED_CTRL_MODE_SERIAL YT921X_LED_CTRL_MODE(2)
+#define YT921X_LED0_PORTn(port) (0xd0004 + 4 * (port))
+#define YT921X_LED0_PORT_ACT_M GENMASK(17, 0)
+#define YT921X_LED0_PORT_ACT_LINK_TRY_DIS BIT(17)
+#define YT921X_LED0_PORT_ACT_COLLISION_BLINK_INDI BIT(16)
+#define YT921X_LED1_PORTn(port) (0xd0040 + 4 * (port))
+#define YT921X_LED1_PORT_OTHER_BLINK_M GENMASK(31, 30) /* cycle = 512 >> x ms */
+#define YT921X_LED1_PORT_OTHER_BLINK(x) FIELD_PREP(YT921X_LED1_PORT_OTHER_BLINK_M, (x))
+#define YT921X_LED1_PORT_EEE_BLINK_M GENMASK(29, 28) /* cycle = 512 >> x ms */
+#define YT921X_LED1_PORT_EEE_BLINK(x) FIELD_PREP(YT921X_LED1_PORT_EEE_BLINK_M, (x))
+#define YT921X_LED1_PORT_BLINK_DUTY_COMP BIT(27)
+#define YT921X_LED1_PORT_BLINK_DUTY_M GENMASK(26, 25)
+#define YT921X_LED1_PORT_BLINK_DUTY(x) FIELD_PREP(YT921X_LED1_PORT_BLINK_DUTY_M, (x))
+#define YT921X_LED1_PORT_BLINK_DUTY_1_2 YT921X_LED1_PORT_BLINK_DUTY(0)
+#define YT921X_LED1_PORT_BLINK_DUTY_2_3 YT921X_LED1_PORT_BLINK_DUTY(1)
+#define YT921X_LED1_PORT_BLINK_DUTY_3_4 YT921X_LED1_PORT_BLINK_DUTY(2)
+#define YT921X_LED1_PORT_BLINK_DUTY_5_6 YT921X_LED1_PORT_BLINK_DUTY(3)
+#define YT921X_LED2_PORTn(port) (0xd0080 + 4 * (port))
+#define YT921X_LED2_PORT_FORCEn_M(grp) GENMASK(4 * (grp) + 19, 4 * (grp) + 18)
+#define YT921X_LED2_PORT_FORCEn(grp, x) ((x) << (4 * (grp) + 18))
+#define YT921X_LED2_PORT_FORCEn_DONTCARE(grp) YT921X_LED2_PORT_FORCEn(grp, 0)
+#define YT921X_LED2_PORT_FORCEn_BLINK(grp) YT921X_LED2_PORT_FORCEn(grp, 1)
+#define YT921X_LED2_PORT_FORCEn_ON(grp) YT921X_LED2_PORT_FORCEn(grp, 2)
+#define YT921X_LED2_PORT_FORCEn_OFF(grp) YT921X_LED2_PORT_FORCEn(grp, 3)
+#define YT921X_LED2_PORT_FORCE_BLINKn_M(grp) GENMASK(4 * (grp) + 17, 4 * (grp) + 16) /* cycle = 512 << x ms */
+#define YT921X_LED2_PORT_FORCE_BLINKn(grp, x) ((x) << (4 * (grp) + 16))
+#define YT921X_LED2_PORT_FORCE_BLINKn_OTHER(grp) YT921X_LED2_PORT_FORCE_BLINKn(grp, 3)
+#define YT921X_LEDx_PORT_ACT_M GENMASK(15, 0)
+#define YT921X_LEDx_PORT_ACT_EEE_BLINK BIT(15)
+#define YT921X_LEDx_PORT_ACT_LOOPDETECT_BLINK BIT(14)
+#define YT921X_LEDx_PORT_ACT_ACTIVE_BLINK BIT(13)
+#define YT921X_LEDx_PORT_ACT_DUPLEX_FULL BIT(12)
+#define YT921X_LEDx_PORT_ACT_DUPLEX_HALF BIT(11)
+#define YT921X_LEDx_PORT_ACT_TX_BLINK BIT(10)
+#define YT921X_LEDx_PORT_ACT_RX_BLINK BIT(9)
+#define YT921X_LEDx_PORT_ACT_TX BIT(8)
+#define YT921X_LEDx_PORT_ACT_RX BIT(7)
+#define YT921X_LEDx_PORT_ACT_1000M BIT(6)
+#define YT921X_LEDx_PORT_ACT_100M BIT(5)
+#define YT921X_LEDx_PORT_ACT_10M BIT(4)
+#define YT921X_LEDx_PORT_ACT_COLLISION_BLINK BIT(3)
+#define YT921X_LEDx_PORT_ACT_1000M_BLINK BIT(2)
+#define YT921X_LEDx_PORT_ACT_100M_BLINK BIT(1)
+#define YT921X_LEDx_PORT_ACT_10M_BLINK BIT(0)
+#define YT921X_LED_SER_CTRL 0xd0100
+#define YT921X_LED_SER_CTRL_UNK GENMASK(25, 24) /* delay? */
+#define YT921X_LED_SER_CTRL_ACTIVE_LOW BIT(4)
+#define YT921X_LED_SER_CTRL_GRP_NUM_M GENMASK(1, 0) /* #grp - 1 */
+#define YT921X_LED_SER_CTRL_GRP_NUM(x) FIELD_PREP(YT921X_LED_SER_CTRL_GRP_NUM_M, (x))
+#define YT921X_LED_SER_MAPnm(grp, port) (0xd0104 + 8 * (2 - (grp)) + 4 * ((port) / 5))
+#define YT921X_LED_SER_MAP_DSTn_PORT_M(port) GENMASK(6 * ((port) % 5) + 5, 6 * ((port) % 5) + 2)
+#define YT921X_LED_SER_MAP_DSTn_PORT(port, x) ((x) << (6 * ((port) % 5) + 2))
+#define YT921X_LED_SER_MAP_DSTn_LED_M(port) GENMASK(6 * ((port) % 5) + 1, 6 * ((port) % 5))
+#define YT921X_LED_SER_MAP_DSTn_LED(port, x) ((x) << (6 * ((port) % 5)))
+#define YT921X_LED_PAR_PORTS 0xd01c4
+#define YT921X_LED_PAR_INV 0xd01c8
+#define YT921X_LED_PAR_INV_INVnm(grp, port) BIT(10 * (grp) + (port))
+#define YT921X_LED_PAR_MAPn(port) (0xd01d0 + 4 * (port))
+#define YT921X_LED_PAR_MAP_DSTn_PORT_M(grp) GENMASK(6 * (grp) + 5, 6 * (grp) + 2)
+#define YT921X_LED_PAR_MAP_DSTn_PORT(grp, x) ((x) << (6 * (grp) + 2))
+#define YT921X_LED_PAR_MAP_DSTn_LED_M(grp) GENMASK(6 * (grp) + 1, 6 * (grp))
+#define YT921X_LED_PAR_MAP_DSTn_LED(grp, x) ((x) << (6 * (grp)))
+
+#define YT921X_LED_BLINK_MIN 64
+#define YT921X_LED_BLINK_DEF 512
+#define YT921X_LED_BLINK_MAX 2048
+
+/* 2 * lcm(2, 3, 4, 6) */
+#define YT921X_LED_DUTY_DENOM 24
+#define YT921X_LED_DUTY(nom, denom) (YT921X_LED_DUTY_DENOM * (nom) / (denom))
+
+struct yt921x_priv;
+
+struct yt921x_led {
+ struct led_classdev cdev;
+ struct yt921x_port *port;
+ unsigned char group;
+
+ bool use_cycle:1;
+ bool use_duty:1;
+};
+
+#if IS_ENABLED(CONFIG_NET_DSA_YT921X_LEDS)
+
+void yt921x_leds_remove(struct yt921x_priv *priv);
+int yt921x_leds_setup(struct yt921x_priv *priv);
+
+#else
+
+static inline void yt921x_leds_remove(struct yt921x_priv *priv) {}
+
+static inline int yt921x_leds_setup(struct yt921x_priv *priv)
+{
+ return 0;
+}
+
+#endif
+
+#endif
--
2.53.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH net-next v10 3/4] net: dsa: motorcomm: Add LED support
2026-08-16 19:38 ` [PATCH net-next v10 3/4] net: dsa: motorcomm: Add LED support David Yang
@ 2026-08-18 17:10 ` Jakub Kicinski
0 siblings, 0 replies; 8+ messages in thread
From: Jakub Kicinski @ 2026-08-18 17:10 UTC (permalink / raw)
To: mmyangfl
Cc: Jakub Kicinski, netdev, andrew, olteanv, davem, edumazet, pabeni,
robh, krzk+dt, conor+dt, devicetree, linux-kernel
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: dsa: motorcomm: Add LED support
This adds a new leds.c to the motorcomm DSA driver which registers up to
three LED class devices per port from the standard DT "leds" child node,
supporting brightness, hardware blink and the netdev hardware-offload
trigger. Registration happens from the DSA .setup callback and cleanup
from the mdio .remove callback.
A few questions below.
> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 6dee25b6754a8..553efacd4f5ee 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c
[ ... ]
> @@ -4581,6 +4580,12 @@ static int yt921x_dsa_setup(struct dsa_switch *ds)
> if (res)
> return res;
>
> +#if IS_ENABLED(CONFIG_NET_DSA_YT921X_LEDS)
> + res = yt921x_leds_setup(priv);
> + if (res)
> + dev_warn(dev, "Failed to setup LEDs: %d\n", res);
> +#endif
> +
> return 0;
> }
[Severity: Low]
Can this dev_warn() ever be reached? yt921x_leds_setup() discards the
result of every yt921x_leds_setup_port() call and has a single
return 0;
as its only exit, so res is always zero here.
Similarly, yt921x_leds_setup_port() returns -EINVAL for
port >= YT921X_LED_PORT_NUM, but that value is dropped by
yt921x_leds_setup(). And inside the for_each_child_of_node_scoped() loop
the return value of yt921x_led_setup() is not stored at all, so a
duplicated group (-EEXIST), a devm_kzalloc() failure (-ENOMEM) or a
devm_led_classdev_register_ext() failure leaves the LED silently absent
with no message in the log. Was some diagnostic intended for at least the
-EEXIST and -ENOMEM cases?
> @@ -4681,6 +4686,10 @@ static void yt921x_mdio_remove(struct mdio_device *mdiodev)
> if (!priv)
> return;
>
> +#if IS_ENABLED(CONFIG_NET_DSA_YT921X_LEDS)
> + yt921x_leds_remove(priv);
> +#endif
> +
> for (size_t i = ARRAY_SIZE(priv->ports); i-- > 0; ) {
> struct yt921x_port *pp = &priv->ports[i];
>
[Severity: Medium]
The LEDs are created from ds->ops->setup() (yt921x_dsa_setup) but the only
ordered bulk release lives here in the mdio .remove callback, and
yt921x_dsa_switch_ops has no .teardown. Are the two other exit paths
covered?
If dsa_register_switch() fails after ds->ops->setup() already succeeded,
yt921x_mdio_probe() returns an error and .remove is never called, so
cleanup falls back to plain devres unwinding. Each LED has two devres
entries, in the order devm_kzalloc(led) then
devm_led_classdev_register_ext(), so reverse release frees LED N's control
block while LED N-1 is still registered in sysfs and pp->leds[N] still
points at the freed block. A concurrent write to LED N-1's delay_on then
enters yt921x_led_blink_set(), which reads the siblings:
if ((change_cycle && pp->leds[i]->use_cycle) ||
(change_duty && pp->leds[i]->use_duty))
Is that a read of freed memory? yt921x_leds_remove_port() deliberately
unregisters everything first, then clears pp->leds[] before devm_kfree(),
which suggests that ordering is required and devres unwinding does not
provide it.
Also, on a DSA tree teardown that leaves the driver bound,
dsa_switch_teardown() has no driver hook here, so the LED class devices
stay registered and user-space-callable while dsa_port_teardown() sets
dp->user = NULL and the chip is reset. A subsequent tree re-setup
re-initialises nothing, because yt921x_led_setup() bails out early:
if (pp->leds[group])
return -EEXIST;
leaving led_cdev->brightness, pp->led_duty/led_cycle and
led->use_cycle/use_duty describing a hardware state that no longer exists.
Would a .teardown that calls yt921x_leds_remove() address both cases?
> diff --git a/drivers/net/dsa/motorcomm/leds.c b/drivers/net/dsa/motorcomm/leds.c
> new file mode 100644
> index 0000000000000..e7ef0ef80a266
> --- /dev/null
> +++ b/drivers/net/dsa/motorcomm/leds.c
> @@ -0,0 +1,654 @@
[ ... ]
> +static int
> +yt921x_led_force_set(struct yt921x_priv *priv, int port, int group, bool on)
> +{
> + struct yt921x_port *pp = &priv->ports[port];
> + struct yt921x_led *led = pp->leds[group];
> + u32 ctrl;
> + u32 mask;
> +
> + led->use_cycle = false;
> + led->use_duty = false;
> +
> + mask = YT921X_LED2_PORT_FORCEn_M(group);
> + ctrl = on ? YT921X_LED2_PORT_FORCEn_ON(group) :
> + YT921X_LED2_PORT_FORCEn_OFF(group);
> + return yt921x_reg_update_bits(priv, YT921X_LED2_PORTn(port), mask,
> + ctrl);
> +}
[Severity: Low]
Should use_cycle/use_duty only be cleared once the register update has
succeeded? If the SMI/MDIO transfer in yt921x_reg_update_bits() fails,
the LED keeps hardware-blinking while the driver records that this group no
longer uses the shared per-port duty and cycle fields.
The same pattern appears in yt921x_led_trigger_set(), which clears both
flags before two fallible yt921x_reg_update_bits() calls and never
restores them on failure.
[ ... ]
> + /* The chip seems to jam a while if changing duty directly */
> + res = yt921x_reg_read(priv, YT921X_LED2_PORTn(port), &val);
> + if (res)
> + return res;
> +
> + ctrl = val & ~YT921X_LED2_PORT_FORCEn_M(group);
> + ctrl |= YT921X_LED2_PORT_FORCEn_DONTCARE(group);
> + if (val != ctrl) {
> + res = yt921x_reg_write(priv, YT921X_LED2_PORTn(port), ctrl);
> + if (res)
> + return res;
> + }
[ ... ]
> + res = yt921x_reg_update_bits(priv, YT921X_LED1_PORTn(port), mask, ctrl);
> + if (res)
> + return res;
[Severity: Low]
This is a multi-register sequence: FORCE is first switched to DONTCARE, so
the LED immediately follows whatever activity bits happen to be programmed,
then the shared YT921X_LED1_PORTn duty and OTHER_BLINK fields are updated,
then forced blinking is selected. Both intermediate error returns leave
the earlier writes in place with no unwind.
Since the conflict check earlier in this function trusts use_cycle and
use_duty to decide whether another group owns the shared timing fields, can
a later request silently retime a sibling LED that is still blinking, or
leave an LED driven by stale activity bits?
[ ... ]
> +static struct device * __maybe_unused
> +yt921x_cled_hw_control_get_device(struct led_classdev *led_cdev)
> +{
> + struct yt921x_led *led = to_yt921x_led(led_cdev);
> + struct yt921x_port *pp = to_yt921x_port(led);
> + struct yt921x_priv *priv = to_yt921x_priv(pp);
> + struct dsa_port *dp;
> +
> + dp = dsa_to_port(&priv->ds, pp->index);
> + if (!dp)
> + return NULL;
> +
> + if (dsa_port_is_user(dp))
> + return &dp->user->dev;
> + if (dsa_port_is_cpu(dp))
> + return &dp->conduit->dev;
> +
> + return NULL;
> +}
[Severity: High]
Should dp->user and dp->conduit be checked for NULL here rather than only
the port type?
dp->type is set at DT parse time, but dp->user is populated later by
dsa_user_create() from dsa_port_setup(), which runs in
dsa_tree_setup_ports() after dsa_tree_setup_switches() has already invoked
ds->ops->setup():
dsa_tree_setup()
dsa_tree_setup_switches() -> dsa_switch_setup() -> ds->ops->setup()
yt921x_dsa_setup() -> yt921x_leds_setup()
dsa_tree_setup_ports() -> dsa_port_setup() -> dsa_user_create()
So at LED registration time dp->user is still NULL for user ports and this
returns (char *)NULL + offsetof(struct net_device, dev), which is not NULL.
led_classdev_register_ext() honours linux,default-trigger, so a node with
linux,default-trigger = "netdev";
runs netdev_trig_activate() synchronously, and there the only guard is a
NULL test:
drivers/leds/trigger/ledtrig-netdev.c:netdev_trig_activate() {
...
dev = led_cdev->hw_control_get_device(led_cdev);
if (dev) {
const char *name = dev_name(dev);
...
}
Does dev_name() then dereference that bogus pointer during probe?
The window reopens later as well: dsa_port_teardown() does
case DSA_PORT_TYPE_USER:
if (dp->user) {
dsa_user_destroy(dp->user);
dp->user = NULL;
}
while dp->type stays DSA_PORT_TYPE_USER and, with no .teardown in this
driver, the LED class devices remain registered. A later
echo netdev > /sys/class/leds/.../trigger
reaches validate_net_dev(), which also only checks if (!dev).
Both peer DSA LED drivers guard the pointer itself, for example
mv88e6xxx_led_hw_control_get_device():
if (dp->user)
return &dp->user->dev;
return NULL;
and qca8k_cled_hw_control_get_device() does the same.
[ ... ]
> + pp->leds[group] = led;
> +
> + switch (state) {
> + case LEDS_DEFSTATE_OFF:
> + case LEDS_DEFSTATE_ON:
> + default:
> + on = state == LEDS_DEFSTATE_ON;
> + res = yt921x_led_force_set(priv, port, group, on);
> + break;
> + case LEDS_DEFSTATE_KEEP:
> + res = yt921x_led_force_get(priv, port, group, &on);
> + break;
> + }
[ ... ]
> + res = devm_led_classdev_register_ext(dev, led_cdev, &init_data);
> + if (res)
> + goto err;
> +
> + return 0;
> +
> +err:
> + ret = res;
> +
> + mutex_lock(&priv->reg_lock);
> + if (pp->leds[group]) {
> + if (force_high || force_low) {
> + res = yt921x_reg_write(priv, YT921X_LED_PAR_INV, val);
> + if (res)
> + dev_warn(dev,
> + "Failed to revoke initialization for LED %02d:%02u\n",
> + port, group);
> + }
> +
> + pp->leds[group] = NULL;
> + }
> + mutex_unlock(&priv->reg_lock);
> +
> + devm_kfree(dev, led);
> + return ret;
> +}
[Severity: Low]
The FORCE field is programmed for the default on/off state before
devm_led_classdev_register_ext() is called, but the err: path restores only
YT921X_LED_PAR_INV, and only when an explicit active-high/active-low was
given. Should the previous FORCE field be restored too?
If registration fails (the devres_alloc() -ENOMEM, an -EINVAL from
led_compose_name(), or a device_create_with_groups() failure), the LED is
left forced on or off with no class device through which it can be changed,
and yt921x_leds_setup_port() discards the error so nothing higher up
reports or retries.
> +static int yt921x_leds_setup_port(struct yt921x_priv *priv, int port)
> +{
[ ... ]
> + for_each_child_of_node_scoped(leds_np, led_np) {
> + yt921x_led_setup(priv, port, of_fwnode_handle(led_np));
> + /* Allow partial configuration: LEDs are optional */
> + }
> +
> + of_node_put(leds_np);
> + return 0;
> +}
[ ... ]
> +int yt921x_leds_setup(struct yt921x_priv *priv)
> +{
[ ... ]
> + yt921x_leds_setup_port(priv, port);
> + /* Allow partial configuration: LEDs are optional */
> + }
> +
> + return 0;
> +}
[Severity: Low]
These two functions are the ones that make the dev_warn() in
yt921x_dsa_setup() unreachable, as noted above. Is the "LEDs are optional"
policy meant to coexist with that warning, or should one of them go?
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH net-next v10 4/4] dt-bindings: net: dsa: yt921x: Add LEDs definition example
2026-08-16 19:38 [PATCH net-next v10 0/4] net: dsa: motorcomm: Add LED support David Yang
` (2 preceding siblings ...)
2026-08-16 19:38 ` [PATCH net-next v10 3/4] net: dsa: motorcomm: Add LED support David Yang
@ 2026-08-16 19:38 ` David Yang
3 siblings, 0 replies; 8+ messages in thread
From: David Yang @ 2026-08-16 19:38 UTC (permalink / raw)
To: netdev
Cc: David Yang, Andrew Lunn, Vladimir Oltean, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, devicetree, linux-kernel
Add LEDs definition example for yt921x to describe how they should be
defined for a correct usage.
Signed-off-by: David Yang <mmyangfl@gmail.com>
---
.../bindings/net/dsa/motorcomm,yt921x.yaml | 23 +++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/dsa/motorcomm,yt921x.yaml b/Documentation/devicetree/bindings/net/dsa/motorcomm,yt921x.yaml
index 33a6552e46fc..93ba5b181205 100644
--- a/Documentation/devicetree/bindings/net/dsa/motorcomm,yt921x.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/motorcomm,yt921x.yaml
@@ -21,6 +21,9 @@ description: |
Any port can be used as the CPU port.
+ Each port has at most 3 LEDs connected and can be declared using the standard
+ LEDs structure.
+
properties:
compatible:
const: motorcomm,yt9215
@@ -59,6 +62,7 @@ unevaluatedProperties: false
examples:
- |
#include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/leds/common.h>
mdio {
#address-cells = <1>;
@@ -113,6 +117,25 @@ examples:
label = "lan1";
phy-mode = "internal";
phy-handle = <&sw_phy0>;
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_WHITE>;
+ function = LED_FUNCTION_LAN;
+ default-state = "keep";
+ };
+
+ led@1 {
+ reg = <1>;
+ color = <LED_COLOR_ID_AMBER>;
+ function = LED_FUNCTION_LAN;
+ default-state = "keep";
+ };
+ };
};
ethernet-port@1 {
--
2.53.0
^ permalink raw reply related [flat|nested] 8+ messages in thread