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

[David Yang]

On Thu, Jun 18, 2026 at 5:53 PM Kyle Switch <kyle.switch@xxxxxxxxxxxxxx> wrote:
> As you mentioned "One thing i need to point out. Linux has a long tradition of not
> replacing existing code with a new implementation. You take the existing code and step by step improve it. " in another mail before.
> I want to explain the patch in more detail.
>
> Step 1. We do not attempt to remove the existing driver implementation, and don't change the behavior of existing software,
> we will retain the implementation of the existing driver software layer, but encapsulate the use of hardware operations into
> functional interfaces. The advantage of this is that it is easy to maintain and easy to support other motorcomm switch series.
>
> for example: vlan add ops in dsa driver:
>
> Existing code:
>
> yt921x_vlan_add(struct yt921x_priv *priv, int port, u16 vid, bool untagged)
> {
>  u64 mask64;
>  u64 ctrl64;
>
>  mask64 = YT921X_VLAN_CTRL_PORTn(port) |
>    YT921X_VLAN_CTRL_PORTS(priv->cpu_ports_mask);
>  ctrl64 = mask64;
>
>  mask64 |= YT921X_VLAN_CTRL_UNTAG_PORTn(port);
>  if (untagged)
>   ctrl64 |= YT921X_VLAN_CTRL_UNTAG_PORTn(port);
>
>  return yt921x_reg64_update_bits(priv, YT921X_VLANn_CTRL(vid),
>      mask64, ctrl64);
> }
>
> after patch:
>
> yt921x_vlan_add(struct yt921x_priv *priv, int port, u16 vid, bool untagged)
> {
>  struct yt_port_mask member;
>  struct yt_port_mask untag;
>
>  member.portsbits[0] = BIT(port) | priv->cpu_ports_mask;
>  if (untagged)
>   untag.portbits[0] = BIT(port);
>
>   return yt_vlan_port_set(priv->unit, vid, member, untag);  // Here we use encapsulated interfaces to complete the hardware configuration.
>                                                              // We can ignore the differences between different motorcomm series, which will be reflected in driver/net/dsa/motorocmm/switch/yt_vlan. c
> }

No for nuking the existing code. Your encapsulated interfaces do
everything that no other does
  - hardwired to static variables everywhere;
  - invented unnecessary types (yt_enable) and struct (yt_port_mask);
  - contain lots of unused files and meaningless comments, while;
  - incomplete in other parts, for example ACL;
  - still require direct register accesses outside your interfaces.
I see no chance that your current interfaces will ever be accepted,
unless you come up with something totally different.

Looking at other DSA drivers, please use one of the following approaches wisely:

struct yt921x_ops ...;

// for simple register relocations
res = yt921x_reg_read(priv, priv->ops->some_reg, &val);

// for complex operations, but still reuse the context
do_something;
priv->ops->some_op(...);
do_the_rest;

// for something completely different
struct dsa_switch_ops yt921x_dsa_switch_ops = {
    .some_op = yt921x_dsa_some_op,
};
struct dsa_switch_ops yt922x_dsa_switch_ops = {
    .some_op = yt922x_dsa_some_op,
};

> Step 2. if Step 1 is accepted, later, the plan may be to replace the hardware configuration involved in the existing dsa driver
> with the encapsulated interface step by step according to the functional module such as vlan, mirror, lag, etc. Finally, upload the yt922x dsa driver.

You upload the driver for yt922x for your first patch.

Take ar9331 as an example, a DSA driver is only required to implement
.get_tag_protocol(), .setup(), .phylink_get_caps() and
phylink_mac_ops, that's is the "minimal" work for a new chip. So
please make changes to these functions (and possibly more, but keep
the patch size in a reasonable range), and each of your patches will
be an actually usable and testable commit.