Re: [PATCH net-next 1/3] net: dsa: mediatek: add VLAN support for MT7530

Hi, Andrew

All sounds reasonable. All will be fixed in the next version.

	Sean

On Thu, 2017-12-07 at 16:24 +0100, Andrew Lunn wrote:

quoted

 static void
+mt7530_port_set_vlan_unware(struct dsa_switch *ds, int port)
+{
+	struct mt7530_priv *priv = ds->priv;
+	int i;
+	bool all_user_ports_removed = true;

Hi Sean

Reverse Christmas tree please.

will be fixed 

quoted

+static int
+mt7530_vlan_cmd(struct mt7530_priv *priv, enum mt7530_vlan_cmd cmd, u16 vid)
+{
+	u32 val;
+	int ret;
+	struct mt7530_dummy_poll p;

Here too.

will be fixed

quoted

+static int
+mt7530_port_vlan_prepare(struct dsa_switch *ds, int port,
+			 const struct switchdev_obj_port_vlan *vlan,
+			 struct switchdev_trans *trans)
+{
+	struct mt7530_priv *priv = ds->priv;
+
+	/* The port is being kept as VLAN-unware port when bridge is set up
+	 * with vlan_filtering not being set, Otherwise, the port and the
+	 * corresponding CPU port is required the setup for becoming a
+	 * VLAN-ware port.
+	 */
+	if (!priv->ports[port].vlan_filtering)
+		return 0;
+
+	mt7530_port_set_vlan_ware(ds, port);
+	mt7530_port_set_vlan_ware(ds, MT7530_CPU_PORT);

A prepare function should just validate that it is possible to carry
out the operation. It should not change any state. These two last
lines probably don't belong here.

okay, it will be moved into the proper place such as
mt7530_port_vlan_filtering

quoted

+
+	return 0;
+}
+
+static void
+mt7530_hw_vlan_add(struct mt7530_priv *priv,
+		   struct mt7530_hw_vlan_entry *entry)
+{
+	u32 val;
+	u8 new_members;

Reverse Christmas tree. Please check the whole patch.

 will be fixed

quoted

+static inline void INIT_MT7530_HW_ENTRY(struct mt7530_hw_vlan_entry *e,
+					int port, bool untagged)
+{
+	e->port = port;
+	e->untagged = untagged;
+}

All CAPITAL letters is for #defines. This is just a normal
function. Please use lower case.

will be fixed

	  Andrew