Re: [PATCH net-next 15/17] net: dsa: Add new binding implementation

[Florian Fainelli <f.fainelli@gmail.com>]

On 06/03/2016 09:44 AM, Andrew Lunn wrote:
> The existing DSA binding has a number of limitations and problems. The
> main problem is that it cannot represent a switch as a linux device,
> hanging off some bus. It is limited to one CPU port. The DSA platform
> device is artificial, and does not really represent hardware.
> 
> Implement a new binding which can be embedded into any type of node on
> a bus to represent one switch device, and its links to other switches.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Just a few nits that I had not seen before...


> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 6c314f300424..d8cb2acd4f0a 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -294,6 +294,7 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
>  			}
>  			dst->cpu_switch = index;
>  			dst->cpu_port = i;
> +			ds->cpu_port_mask |= 1 << i;
>  		} else if (!strcmp(name, "dsa")) {
>  			ds->dsa_port_mask |= 1 << i;
>  		} else {

We might want to undo setting the cpu_port_mask bit in
dsa_cpu_dsa_destroy()?

[snip]

> +static int dsa_ds_complete(struct dsa_switch_tree *dst, struct dsa_switch *ds)
> +{
> +	struct device_node *port;
> +	u32 index;
> +	int err;
> +
> +	for (index = 0; index < DSA_MAX_PORTS; index++) {
> +		port = ds->ports[index].dn;
> +		if (!port)
> +			continue;
> +
> +		if (!dsa_port_is_dsa(port))
> +			continue;
> +
> +		ds->dsa_port_mask |= 1 << index;
> +
> +		err = dsa_port_complete(dst, ds, port, index);
> +		if (err != 0)

Should we move ds->dsa_port_mask |= 1 << index into dsa_port_complete,
for a) symetry with code undoing this, and b) avoid letting this bit be
set in case dsa_port_complete() returns an error?
-- 
Florian