Re: [PATCH net-next] net: dsa: rzn1-a5psw: Add vlan support

[<Arun.Ramadoss@microchip.com>]

Hi Clement,
On Wed, 2023-01-11 at 12:56 +0100, Clément Léger wrote:
> Add support for vlan operation (add, del, filtering) on the RZN1
> driver. The a5psw switch supports up to 32 VLAN IDs with filtering,
> tagged/untagged VLANs and PVID for each ports.
> 
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> ---
>  drivers/net/dsa/rzn1_a5psw.c | 182
> +++++++++++++++++++++++++++++++++++
>  drivers/net/dsa/rzn1_a5psw.h |  10 +-
>  2 files changed, 189 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/dsa/rzn1_a5psw.c
> b/drivers/net/dsa/rzn1_a5psw.c
> index ed413d555bec..8ecb9214b5e6 100644
> --- a/drivers/net/dsa/rzn1_a5psw.c
> +++ b/drivers/net/dsa/rzn1_a5psw.c
> @@ -540,6 +540,161 @@ static int a5psw_port_fdb_dump(struct
> dsa_switch *ds, int port,
>  	return ret;
>  }
>  
> +static int a5psw_port_vlan_filtering(struct dsa_switch *ds, int
> port,
> +				     bool vlan_filtering,
> +				     struct netlink_ext_ack *extack)
> +{
> +	u32 mask = BIT(port + A5PSW_VLAN_VERI_SHIFT)
> +		   | BIT(port + A5PSW_VLAN_DISC_SHIFT);

Operator | at the end of line

> +	struct a5psw *a5psw = ds->priv;
> +	u32 val = 0;
> +
> +	if (vlan_filtering)
> +		val = BIT(port + A5PSW_VLAN_VERI_SHIFT)
> +		      | BIT(port + A5PSW_VLAN_DISC_SHIFT);

Operator | at the end of line

> +
> +	a5psw_reg_rmw(a5psw, A5PSW_VLAN_VERIFY, mask, val);
> +
> +	return 0;
> +}
> +
> +static int a5psw_port_vlan_add(struct dsa_switch *ds, int port,
> +			       const struct switchdev_obj_port_vlan
> *vlan,
> +			       struct netlink_ext_ack *extack)
> +{
> +	bool tagged = !(vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED);
> +	bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
> +	struct a5psw *a5psw = ds->priv;
> +	u16 vid = vlan->vid;
> +	int ret = -EINVAL;
> +	int vlan_res_id;
> +
> +	dev_dbg(a5psw->dev, "Add VLAN %d on port %d, %s, %s\n",
> +		vid, port, tagged ? "tagged" : "untagged",
> +		pvid ? "PVID" : "no PVID");
> +
> +	mutex_lock(&a5psw->vlan_lock);
> +
> +	vlan_res_id = a5psw_find_vlan_entry(a5psw, vid);
> +	if (vlan_res_id < 0) {
> +		vlan_res_id = a5psw_get_vlan_res_entry(a5psw, vid);
> +		if (vlan_res_id < 0)

nit: We can initialize ret = 0 initially, and assign ret = -EINVAL here
& remove ret = 0 at end of function.

> +			goto out;
> +	}
> +
> +	a5psw_port_vlan_cfg(a5psw, vlan_res_id, port, true);
> +	if (tagged)
> +		a5psw_port_vlan_tagged_cfg(a5psw, vlan_res_id, port,
> true);
> +
> +	if (pvid) {
> +		a5psw_reg_rmw(a5psw, A5PSW_VLAN_IN_MODE_ENA, BIT(port),
> +			      BIT(port));
> +		a5psw_reg_writel(a5psw, A5PSW_SYSTEM_TAGINFO(port),
> vid);
> +	}
> +
> +	ret = 0;
> +out:
> +	mutex_unlock(&a5psw->vlan_lock);
> +
> +	return ret;
> +}
> +
> +static int a5psw_port_vlan_del(struct dsa_switch *ds, int port,
> +			       const struct switchdev_obj_port_vlan
> *vlan)
> +{
> +	struct a5psw *a5psw = ds->priv;
> +	u16 vid = vlan->vid;
> +	int ret = -EINVAL;

Simillarly here.

> +	int vlan_res_id;
> +
> +	dev_dbg(a5psw->dev, "Removing VLAN %d on port %d\n", vid,
> port);
> +
> +	mutex_lock(&a5psw->vlan_lock);
> +
> +	vlan_res_id = a5psw_find_vlan_entry(a5psw, vid);
> +	if (vlan_res_id < 0)
> +		goto out;
> +
> +	a5psw_port_vlan_cfg(a5psw, vlan_res_id, port, false);
> +	a5psw_port_vlan_tagged_cfg(a5psw, vlan_res_id, port, false);
> +
> +	/* Disable PVID if the vid is matching the port one */
> +	if (vid == a5psw_reg_readl(a5psw, A5PSW_SYSTEM_TAGINFO(port)))
> +		a5psw_reg_rmw(a5psw, A5PSW_VLAN_IN_MODE_ENA, BIT(port),
> 0);
> +
> +	ret = 0;
> +out:
> +	mutex_unlock(&a5psw->vlan_lock);
> +
> +	return ret;
> +}
> +
>