Re: [net-next PATCH 2/2] net: dsa: rtl8366: Refactor VLAN/PVID init

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

On 8/23/2020 1:59 PM, Linus Walleij wrote:
> The VLANs and PVIDs on the RTL8366 utilizes a "member
> configuration" (MC) which is largely unexplained in the
> code.
> 
> This set-up requires a special ordering: rtl8366_set_pvid()
> must be called first, followed by rtl8366_set_vlan(),
> else the MC will not be properly allocated. Relax this
> by factoring out the code obtaining an MC and reuse
> the helper in both rtl8366_set_pvid() and
> rtl8366_set_vlan() so we remove this strict ordering
> requirement.
> 
> In the process, add some better comments and debug prints
> so people who read the code understand what is going on.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---

[snip]
> +/**
> + * rtl8366_obtain_mc() - retrieve or allocate a VLAN member configuration
> + * @smi: the Realtek SMI device instance
> + * @vid: the VLAN ID to look up or allocate
> + * @vlanmc: the pointer will be assigned to a pointer to a valid member config
> + * if successful
> + * @index: the pointer will be assigned to the index of the valid member config
> + * if successful
> + * @return: 0 or error number

You could return the index directly, since only negative error codes are 
of interest to deal with failures?

[snip]

> +
> +	dev_err(smi->dev, "all VLAN member configurations are in use\n");
> +	return -EINVAL;

Maybe -ENOSPC?

[snip]

   				vid);
> +
> +		ret = rtl8366_set_pvid(smi, port, vid);
> +		if (ret)
> +			dev_err(smi->dev,
> +				"failed to set PVID on port %d to VLAN %04x",
> +				port, vid);
> +
> +		if (!ret)
> +			dev_dbg(smi->dev, "VLAN add: added VLAN %04x with PVID on port %d\n",
> +				vid, port);

Since you are changing this message to a debug print, can we bring some 
consistency along the way and print the VID as decimal (not hexadecimal) 
to match the prior instances?
-- 
Florian