Re: [RFC PATCH 09/16] dsa: dsa: Split up creating/destroying of DSA and CPU ports

Hi Andrew, Florian,

I suggest to use this RFC to agree on a consistent and robust API for
the DSA layer. Some functions have non-intuitive names or signatures.

What about:

    <namespace>_<action>[_<object>]

where <namespace> matches the first argument. So instead of
dsa_cpu_dsa_setup, we would have:

    dsa_switch_setup_cpu_dsa_port(struct dsa_switch *ds, ...)

[...]

Andrew Lunn [off-list ref] writes:

 /* basic switch operations **************************************************/
-static int dsa_cpu_dsa_setup(struct dsa_switch *ds, struct net_device *master)
+int dsa_cpu_dsa_setup(struct dsa_switch *ds, struct device *dev,
+		      struct device_node *port_dn, int port)
 {

We will need the port number in the struct dsa_port at some point. The
device_node is already a member of this structure.

I suggest to add the port index before this patch and change this
function for a more logical:

    dsa_switch_setup_cpu_dsa_port(struct dsa_switch *ds, struct dsa_port *dp)

[...]

 	/* Perform configuration of the CPU and DSA ports */
-	ret = dsa_cpu_dsa_setup(ds, dst->master_netdev);
+	ret = dsa_cpu_dsa_setups(ds, parent);
 	if (ret < 0) {
 		netdev_err(dst->master_netdev, "[%d] : can't configure CPU and DSA ports\n",
 			   index);

dsa_cpu_dsa_setups is not really meaningful and doesn't add much
value. At least I'd call it dsa_switch_setup_cpu_dsa_ports(), or better,
move the loop directly where it is needed so we know which port sucked:

dsa_switch_setup_one:

        /* Perform configuration of the CPU and DSA ports */
        for (port = 0; port < DSA_MAX_PORTS; port++) {
                if (!(dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port)))
                        continue;

                ret = dsa_switch_setup_cpu_dsa_port(ds, dev, ds->ports[port]);
                if (ret) {
                        netdev_err(dst->master_netdev,
                                   "[%d] : can't configure port %d\n",
                                   index, ds->ports[port]->index);
                        break;
                }
        }
        
Thanks,

        Vivien