Re: [RESEND PATCH v1] net: dsa: motorcomm: add yt92xx dsa driver

[David Yang]

On Mon, Jun 15, 2026 at 7:12 PM Kyle Switch <kyle.switch@xxxxxxxxxxxxxx> wrote:
>
> Add yt92xx dsa driver supports yt921x and yt922x switch series.
> To support more switch series in the future (e.g., yt923x), the most common configurations are abstracted into switch operation interfaces, due to yt921x and yt922x share similar register layouts and operational logic.

You are blindly plugging existing code into your SDK, without sorting
out register operations (for example in set_mac_eee and
port_change_mtu).

Do not post code you don't understand. Ask if you are not sure.

> +#define CMM_PARAM_CHK(expr, err_code)    \
> +       do {                             \
> +               if ((u32)(expr)) {       \
> +                       return err_code; \
> +               }                        \
> +       } while (0)
> +
> +#define CMM_ERR_CHK(op, ret)           \
> +       do {                           \
> +               ret = (op);            \
> +               if (ret != CMM_ERR_OK) \
> +                       return ret;    \
> +       } while (0)

Do not use macros like this.

> +#define GET_FIELD(value, low_bit, width) \
> +       (((value) >> (low_bit)) & ((1U << (width)) - 1))
> +#define CLR_FIELD(value, low_bit, width) \
> +       ((value) & (~(((1U << (width)) - 1) << (low_bit))))
> +
> +#define HAL_FIELD_SET(low_bit, width, entry, data)                    \
> +       do {                                                          \
> +               *(entry) &= (~(((1UL << (width)) - 1) << (low_bit))); \
> +               *(entry) |= ((data) << (low_bit));                    \
> +       } while (0)
> +
> +#define HAL_FIELD_GET(low_bit, width, entry, pdata) \
> +       (*(pdata) = (((*(entry)) >> (low_bit)) & ((1UL << (width)) - 1)))

FIELD_PREP() and FIELD_GET().

> +/*
> + * Macro Definition
> + */
> +#ifndef NULL
> +#define NULL 0
> +#endif
> +
> +#ifndef FALSE
> +#define FALSE 0
> +#endif
> +
> +#ifndef TRUE
> +#define TRUE 1
> +#endif

Nonsense.

> +       /* Print chipid here since we are interested in lower 16 bits */
> +       dev_info(dev,
> +                "Motorcomm %s ethernet switch.\n",
> +                info->name);

Stop copy-n-paste.

Please, start with a minimal patch that only marks functions which use
different registers/procedures and makes them virtual, and implement
them for yt922x in the future.

> +       {
> +               .compatible = "motorcomm,yt9215,8bytes",
> +               .data = &yt92xx_chip_info[YT9215_8B],
> +       },

Do not change devicetree compatible strings.

> --- a/include/uapi/linux/if_ether.h
> +++ b/include/uapi/linux/if_ether.h
> @@ -118,7 +118,7 @@
>  #define ETH_P_QINQ1    0x9100          /* deprecated QinQ VLAN [ NOT AN OFFICIALLY REGISTERED ID ] */
>  #define ETH_P_QINQ2    0x9200          /* deprecated QinQ VLAN [ NOT AN OFFICIALLY REGISTERED ID ] */
>  #define ETH_P_QINQ3    0x9300          /* deprecated QinQ VLAN [ NOT AN OFFICIALLY REGISTERED ID ] */
> -#define ETH_P_YT921X   0x9988          /* Motorcomm YT921x DSA [ NOT AN OFFICIALLY REGISTERED ID ] */
> +#define ETH_P_YT92XX   0x9988          /* Motorcomm YT92xx DSA [ NOT AN OFFICIALLY REGISTERED ID ] */
>  #define ETH_P_EDSA     0xDADA          /* Ethertype DSA [ NOT AN OFFICIALLY REGISTERED ID ] */
>  #define ETH_P_DSA_8021Q        0xDADB          /* Fake VLAN Header for DSA [ NOT AN OFFICIALLY REGISTERED ID ] */
>  #define ETH_P_DSA_A5PSW        0xE001          /* A5PSW Tag Value [ NOT AN OFFICIALLY REGISTERED ID ] */

UAPI stands for User-space API. Do not change it unless there is a
very very good reason.

> +/* To define the from cpu tag format 8 bytes:
> + *
> + * 0 1 2 3 4 5 6 7 | 0 1 2 3 4 5 6 7
> + *|<----------TPID 0x9988---------->|
> + *|<--RESERVE-->|<-----DST PORT---->|
> + *|-|<---------RESERVE------------->|
> + *|<------------------------------->|
> + */
> +#define YT922X_TAG_FORMAT2_NAME "yt922x-8b"
> +#define YT922X_FORMAT2_TAG_LEN                  8
> +#define YT922X_PKT_TYPE          GENMASK(15, 14)
> +#define YT922X_8B_CPUTAG_PKT_FROM_CPU      0x1
> +#define YT922X_8B_CPUTAG_SRC_PORT          GENMASK(6, 2)
> +#define YT922X_8B_CPUTAG_DST_PORTMASK      GENMASK(8, 0)
> +#define YT922X_8B_CPUTAG_DST_PORTMASK_0      BIT(15)
> +#define YT922X_8B_CPUTAG_DST_PORTMASK_0_EN      0x1
> +#define YT922X_8B_CPUTAG_FORCE_DST         BIT(9)
> +#define YT922X_8B_CPUTAG_FORCE_DST_EN      0x1

If yt922x tag format shares no common with yt921x, make a new tag driver.

> +static struct dsa_tag_driver *dsa_tag_driver_array[] = {
> +       &DSA_TAG_DRIVER_NAME(yt921x_netdev_ops),
> +       &DSA_TAG_DRIVER_NAME(yt922x_4b_netdev_ops),
> +       &DSA_TAG_DRIVER_NAME(yt922x_8b_netdev_ops),
> +};

If both are supported by the chip and 4b does nothing more than 8b
does, do not bother with it.