[PATCH v3 net-next 0/5] Refactor lan9303_xxx_packet_processing

[PATCH v3 net-next 0/5] Refactor lan9303_xxx_packet_processing

[Egil Hjelmeland]

This series is purely non functional. 

It changes the lan9303_enable_packet_processing,
lan9303_disable_packet_processing() to pass port number (0,1,2) as
parameter instead of port offset. This aligns them with
other functions in the module, and makes it possible to simplify the code.

The lan9303_enable_packet_processing, lan9303_disable_packet_processing
functions operate on port. Therefore rename the functions to reflect that
as well.

Reviewer pointed out lan9303_get_ethtool_stats would be better off with
the use of a lan9303_read_switch_port(). So that was added to the series.

Review welcome!


Changes v2 -> v3:
 - Patch 1: Removed the change in lan9303_get_ethtool_stats
 - Added patch 4: rename lan9303_xxx_packet_processing 
 - Added patch 5: refactor lan9303_get_ethtool_stats
 
Changes v1 -> v2:
 - introduced lan9303_write_switch_port() in first patch
 - inserted LAN9303_NUM_PORTS patch
 - Use LAN9303_NUM_PORTS in last patch. Plus whitespace change.  

Egil Hjelmeland (5):
  net: dsa: lan9303: Change lan9303_xxx_packet_processing() port param.
  net: dsa: lan9303: define LAN9303_NUM_PORTS 3
  net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage
  net: dsa: lan9303: Rename lan9303_xxx_packet_processing()
  net: dsa: lan9303: refactor lan9303_get_ethtool_stats

 drivers/net/dsa/lan9303-core.c | 110 ++++++++++++++++++++++-------------------
 1 file changed, 59 insertions(+), 51 deletions(-)

-- 
2.11.0

[PATCH v3 net-next 1/5] net: dsa: lan9303: Change lan9303_xxx_packet_processing() port param.

[Egil Hjelmeland]

lan9303_enable_packet_processing, lan9303_disable_packet_processing()
Pass port number (0,1,2) as parameter instead of port offset.
Because other functions in the module pass port numbers.
And to enable simplifications in following patch.

Introduce lan9303_write_switch_port().

Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 60 ++++++++++++++++++++++--------------------
 1 file changed, 32 insertions(+), 28 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 8e430d1ee297..fa19e320c5a8 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -159,9 +159,7 @@
 # define LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT1 (BIT(9) | BIT(8))
 # define LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0 (BIT(1) | BIT(0))
 
-#define LAN9303_PORT_0_OFFSET 0x400
-#define LAN9303_PORT_1_OFFSET 0x800
-#define LAN9303_PORT_2_OFFSET 0xc00
+#define LAN9303_SWITCH_PORT_REG(port, reg0) (0x400 * (port) + (reg0))
 
 /* the built-in PHYs are of type LAN911X */
 #define MII_LAN911X_SPECIAL_MODES 0x12
@@ -428,6 +426,13 @@ static int lan9303_read_switch_reg(struct lan9303 *chip, u16 regnum, u32 *val)
 	return ret;
 }
 
+static int lan9303_write_switch_port(
+	struct lan9303 *chip, int port, u16 regnum, u32 val)
+{
+	return lan9303_write_switch_reg(
+		chip, LAN9303_SWITCH_PORT_REG(port, regnum), val);
+}
+
 static int lan9303_detect_phy_setup(struct lan9303 *chip)
 {
 	int reg;
@@ -458,24 +463,23 @@ static int lan9303_detect_phy_setup(struct lan9303 *chip)
 	return 0;
 }
 
-#define LAN9303_MAC_RX_CFG_OFFS (LAN9303_MAC_RX_CFG_0 - LAN9303_PORT_0_OFFSET)
-#define LAN9303_MAC_TX_CFG_OFFS (LAN9303_MAC_TX_CFG_0 - LAN9303_PORT_0_OFFSET)
-
 static int lan9303_disable_packet_processing(struct lan9303 *chip,
 					     unsigned int port)
 {
 	int ret;
 
 	/* disable RX, but keep register reset default values else */
-	ret = lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_OFFS + port,
-				       LAN9303_MAC_RX_CFG_X_REJECT_MAC_TYPES);
+	ret = lan9303_write_switch_port(
+			chip, port, LAN9303_MAC_RX_CFG_0,
+			LAN9303_MAC_RX_CFG_X_REJECT_MAC_TYPES);
 	if (ret)
 		return ret;
 
 	/* disable TX, but keep register reset default values else */
-	return lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_OFFS + port,
-				LAN9303_MAC_TX_CFG_X_TX_IFG_CONFIG_DEFAULT |
-				LAN9303_MAC_TX_CFG_X_TX_PAD_ENABLE);
+	return lan9303_write_switch_port(
+			chip, port, LAN9303_MAC_TX_CFG_0,
+			LAN9303_MAC_TX_CFG_X_TX_IFG_CONFIG_DEFAULT |
+			LAN9303_MAC_TX_CFG_X_TX_PAD_ENABLE);
 }
 
 static int lan9303_enable_packet_processing(struct lan9303 *chip,
@@ -484,17 +488,19 @@ static int lan9303_enable_packet_processing(struct lan9303 *chip,
 	int ret;
 
 	/* enable RX and keep register reset default values else */
-	ret = lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_OFFS + port,
-				       LAN9303_MAC_RX_CFG_X_REJECT_MAC_TYPES |
-				       LAN9303_MAC_RX_CFG_X_RX_ENABLE);
+	ret = lan9303_write_switch_port(
+			chip, port, LAN9303_MAC_RX_CFG_0,
+			LAN9303_MAC_RX_CFG_X_REJECT_MAC_TYPES |
+			LAN9303_MAC_RX_CFG_X_RX_ENABLE);
 	if (ret)
 		return ret;
 
 	/* enable TX and keep register reset default values else */
-	return lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_OFFS + port,
-				LAN9303_MAC_TX_CFG_X_TX_IFG_CONFIG_DEFAULT |
-				LAN9303_MAC_TX_CFG_X_TX_PAD_ENABLE |
-				LAN9303_MAC_TX_CFG_X_TX_ENABLE);
+	return lan9303_write_switch_port(
+			chip, port, LAN9303_MAC_TX_CFG_0,
+			LAN9303_MAC_TX_CFG_X_TX_IFG_CONFIG_DEFAULT |
+			LAN9303_MAC_TX_CFG_X_TX_PAD_ENABLE |
+			LAN9303_MAC_TX_CFG_X_TX_ENABLE);
 }
 
 /* We want a special working switch:
@@ -558,13 +564,13 @@ static int lan9303_disable_processing(struct lan9303 *chip)
 {
 	int ret;
 
-	ret = lan9303_disable_packet_processing(chip, LAN9303_PORT_0_OFFSET);
+	ret = lan9303_disable_packet_processing(chip, 0);
 	if (ret)
 		return ret;
-	ret = lan9303_disable_packet_processing(chip, LAN9303_PORT_1_OFFSET);
+	ret = lan9303_disable_packet_processing(chip, 1);
 	if (ret)
 		return ret;
-	return lan9303_disable_packet_processing(chip, LAN9303_PORT_2_OFFSET);
+	return lan9303_disable_packet_processing(chip, 2);
 }
 
 static int lan9303_check_device(struct lan9303 *chip)
@@ -634,7 +640,7 @@ static int lan9303_setup(struct dsa_switch *ds)
 	if (ret)
 		dev_err(chip->dev, "failed to separate ports %d\n", ret);
 
-	ret = lan9303_enable_packet_processing(chip, LAN9303_PORT_0_OFFSET);
+	ret = lan9303_enable_packet_processing(chip, 0);
 	if (ret)
 		dev_err(chip->dev, "failed to re-enable switching %d\n", ret);
 
@@ -757,11 +763,9 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port,
 	/* enable internal packet processing */
 	switch (port) {
 	case 1:
-		return lan9303_enable_packet_processing(chip,
-							LAN9303_PORT_1_OFFSET);
+		return lan9303_enable_packet_processing(chip, port);
 	case 2:
-		return lan9303_enable_packet_processing(chip,
-							LAN9303_PORT_2_OFFSET);
+		return lan9303_enable_packet_processing(chip, port);
 	default:
 		dev_dbg(chip->dev,
 			"Error: request to power up invalid port %d\n", port);
@@ -778,12 +782,12 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port,
 	/* disable internal packet processing */
 	switch (port) {
 	case 1:
-		lan9303_disable_packet_processing(chip, LAN9303_PORT_1_OFFSET);
+		lan9303_disable_packet_processing(chip, port);
 		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 1,
 				  MII_BMCR, BMCR_PDOWN);
 		break;
 	case 2:
-		lan9303_disable_packet_processing(chip, LAN9303_PORT_2_OFFSET);
+		lan9303_disable_packet_processing(chip, port);
 		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 2,
 				  MII_BMCR, BMCR_PDOWN);
 		break;
-- 
2.11.0

[PATCH v3 net-next 2/5] net: dsa: lan9303: define LAN9303_NUM_PORTS 3

[Egil Hjelmeland]

Will be used instead of '3' in upcomming patches.

Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index fa19e320c5a8..126e8b84bdf0 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -20,6 +20,8 @@
 
 #include "lan9303.h"
 
+#define LAN9303_NUM_PORTS 3
+
 /* 13.2 System Control and Status Registers
  * Multiply register number by 4 to get address offset.
  */
-- 
2.11.0

[PATCH v3 net-next 3/5] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage

[Egil Hjelmeland]

Simplify usage of lan9303_enable_packet_processing,
lan9303_disable_packet_processing()

Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 126e8b84bdf0..31fe66fbe39a 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -564,15 +564,16 @@ static int lan9303_handle_reset(struct lan9303 *chip)
 /* stop processing packets for all ports */
 static int lan9303_disable_processing(struct lan9303 *chip)
 {
-	int ret;
+	int p;
 
-	ret = lan9303_disable_packet_processing(chip, 0);
-	if (ret)
-		return ret;
-	ret = lan9303_disable_packet_processing(chip, 1);
-	if (ret)
-		return ret;
-	return lan9303_disable_packet_processing(chip, 2);
+	for (p = 0; p < LAN9303_NUM_PORTS; p++) {
+		int ret = lan9303_disable_packet_processing(chip, p);
+
+		if (ret)
+			return ret;
+	}
+
+	return 0;
 }
 
 static int lan9303_check_device(struct lan9303 *chip)
@@ -765,7 +766,6 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port,
 	/* enable internal packet processing */
 	switch (port) {
 	case 1:
-		return lan9303_enable_packet_processing(chip, port);
 	case 2:
 		return lan9303_enable_packet_processing(chip, port);
 	default:
@@ -784,13 +784,9 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port,
 	/* disable internal packet processing */
 	switch (port) {
 	case 1:
-		lan9303_disable_packet_processing(chip, port);
-		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 1,
-				  MII_BMCR, BMCR_PDOWN);
-		break;
 	case 2:
 		lan9303_disable_packet_processing(chip, port);
-		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 2,
+		lan9303_phy_write(ds, chip->phy_addr_sel_strap + port,
 				  MII_BMCR, BMCR_PDOWN);
 		break;
 	default:
-- 
2.11.0

[PATCH v3 net-next 4/5] net: dsa: lan9303: Rename lan9303_xxx_packet_processing()

[Egil Hjelmeland]

The lan9303_enable_packet_processing, lan9303_disable_packet_processing
functions operate on port, so the names should reflect that.
And to align with lan9303_disable_processing(), rename:

lan9303_enable_packet_processing -> lan9303_enable_processing_port
lan9303_disable_packet_processing -> lan9303_disable_processing_port

Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 31fe66fbe39a..6f409755ba1a 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -465,8 +465,8 @@ static int lan9303_detect_phy_setup(struct lan9303 *chip)
 	return 0;
 }
 
-static int lan9303_disable_packet_processing(struct lan9303 *chip,
-					     unsigned int port)
+static int lan9303_disable_processing_port(struct lan9303 *chip,
+					   unsigned int port)
 {
 	int ret;
 
@@ -484,8 +484,8 @@ static int lan9303_disable_packet_processing(struct lan9303 *chip,
 			LAN9303_MAC_TX_CFG_X_TX_PAD_ENABLE);
 }
 
-static int lan9303_enable_packet_processing(struct lan9303 *chip,
-					    unsigned int port)
+static int lan9303_enable_processing_port(struct lan9303 *chip,
+					  unsigned int port)
 {
 	int ret;
 
@@ -567,7 +567,7 @@ static int lan9303_disable_processing(struct lan9303 *chip)
 	int p;
 
 	for (p = 0; p < LAN9303_NUM_PORTS; p++) {
-		int ret = lan9303_disable_packet_processing(chip, p);
+		int ret = lan9303_disable_processing_port(chip, p);
 
 		if (ret)
 			return ret;
@@ -643,7 +643,7 @@ static int lan9303_setup(struct dsa_switch *ds)
 	if (ret)
 		dev_err(chip->dev, "failed to separate ports %d\n", ret);
 
-	ret = lan9303_enable_packet_processing(chip, 0);
+	ret = lan9303_enable_processing_port(chip, 0);
 	if (ret)
 		dev_err(chip->dev, "failed to re-enable switching %d\n", ret);
 
@@ -767,7 +767,7 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port,
 	switch (port) {
 	case 1:
 	case 2:
-		return lan9303_enable_packet_processing(chip, port);
+		return lan9303_enable_processing_port(chip, port);
 	default:
 		dev_dbg(chip->dev,
 			"Error: request to power up invalid port %d\n", port);
@@ -785,7 +785,7 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port,
 	switch (port) {
 	case 1:
 	case 2:
-		lan9303_disable_packet_processing(chip, port);
+		lan9303_disable_processing_port(chip, port);
 		lan9303_phy_write(ds, chip->phy_addr_sel_strap + port,
 				  MII_BMCR, BMCR_PDOWN);
 		break;
-- 
2.11.0

[PATCH v3 net-next 5/5] net: dsa: lan9303: refactor lan9303_get_ethtool_stats

[Egil Hjelmeland]

In lan9303_get_ethtool_stats: Get rid of 0x400 constant magic
by using new lan9303_read_switch_reg() inside loop.
Reduced scope of two variables.

Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 6f409755ba1a..5aaa46146c27 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -435,6 +435,13 @@ static int lan9303_write_switch_port(
 		chip, LAN9303_SWITCH_PORT_REG(port, regnum), val);
 }
 
+static int lan9303_read_switch_port(
+	struct lan9303 *chip, int port, u16 regnum, u32 *val)
+{
+	return lan9303_read_switch_reg(
+		chip, LAN9303_SWITCH_PORT_REG(port, regnum), val);
+}
+
 static int lan9303_detect_phy_setup(struct lan9303 *chip)
 {
 	int reg;
@@ -709,19 +716,18 @@ static void lan9303_get_ethtool_stats(struct dsa_switch *ds, int port,
 				      uint64_t *data)
 {
 	struct lan9303 *chip = ds->priv;
-	u32 reg;
-	unsigned int u, poff;
-	int ret;
-
-	poff = port * 0x400;
+	unsigned int u;
 
 	for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
-		ret = lan9303_read_switch_reg(chip,
-					      lan9303_mib[u].offset + poff,
-					      &reg);
+		u32 reg;
+		int ret;
+
+		ret = lan9303_read_switch_port(
+			chip, port, lan9303_mib[u].offset, &reg);
+
 		if (ret)
-			dev_warn(chip->dev, "Reading status reg %u failed\n",
-				 lan9303_mib[u].offset + poff);
+			dev_warn(chip->dev, "Reading status port %d reg %u failed\n",
+				 port, lan9303_mib[u].offset);
 		data[u] = reg;
 	}
 }
-- 
2.11.0

Re: [PATCH v3 net-next 1/5] net: dsa: lan9303: Change lan9303_xxx_packet_processing() port param.

[Andrew Lunn]

On Thu, Aug 03, 2017 at 11:45:03AM +0200, Egil Hjelmeland wrote:
> lan9303_enable_packet_processing, lan9303_disable_packet_processing()
> Pass port number (0,1,2) as parameter instead of port offset.
> Because other functions in the module pass port numbers.
> And to enable simplifications in following patch.
> 
> Introduce lan9303_write_switch_port().
> 
> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v3 net-next 2/5] net: dsa: lan9303: define LAN9303_NUM_PORTS 3

[Andrew Lunn]

On Thu, Aug 03, 2017 at 11:45:04AM +0200, Egil Hjelmeland wrote:
> Will be used instead of '3' in upcomming patches.
> 
> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>

Signed-off-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v3 net-next 3/5] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage

[Andrew Lunn]

On Thu, Aug 03, 2017 at 11:45:05AM +0200, Egil Hjelmeland wrote:
> Simplify usage of lan9303_enable_packet_processing,
> lan9303_disable_packet_processing()
> 
> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v3 net-next 4/5] net: dsa: lan9303: Rename lan9303_xxx_packet_processing()

[Andrew Lunn]

On Thu, Aug 03, 2017 at 11:45:06AM +0200, Egil Hjelmeland wrote:
> The lan9303_enable_packet_processing, lan9303_disable_packet_processing
> functions operate on port, so the names should reflect that.
> And to align with lan9303_disable_processing(), rename:
> 
> lan9303_enable_packet_processing -> lan9303_enable_processing_port
> lan9303_disable_packet_processing -> lan9303_disable_processing_port
> 
> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>


Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v3 net-next 5/5] net: dsa: lan9303: refactor lan9303_get_ethtool_stats

[Andrew Lunn]

On Thu, Aug 03, 2017 at 11:45:07AM +0200, Egil Hjelmeland wrote:
> In lan9303_get_ethtool_stats: Get rid of 0x400 constant magic
> by using new lan9303_read_switch_reg() inside loop.
> Reduced scope of two variables.
> 
> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v3 net-next 2/5] net: dsa: lan9303: define LAN9303_NUM_PORTS 3

[Florian Fainelli]

On 08/03/2017 02:45 AM, Egil Hjelmeland wrote:
> Will be used instead of '3' in upcomming patches.
> 
> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH v3 net-next 5/5] net: dsa: lan9303: refactor lan9303_get_ethtool_stats

[Florian Fainelli]

On 08/03/2017 02:45 AM, Egil Hjelmeland wrote:
> In lan9303_get_ethtool_stats: Get rid of 0x400 constant magic
> by using new lan9303_read_switch_reg() inside loop.
> Reduced scope of two variables.
> 
> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
> ---
>  drivers/net/dsa/lan9303-core.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> index 6f409755ba1a..5aaa46146c27 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -435,6 +435,13 @@ static int lan9303_write_switch_port(
>  		chip, LAN9303_SWITCH_PORT_REG(port, regnum), val);
>  }
>  
> +static int lan9303_read_switch_port(
> +	struct lan9303 *chip, int port, u16 regnum, u32 *val)
> +{

This indentation is really funny, why not just break it up that way:

static int lan9303_read_switch_port(struct lan9303 *chip, int port
				    u16 regnum, u32 *val)
{
}

This applies to patch 5 as well, other than that:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

> +	return lan9303_read_switch_reg(
> +		chip, LAN9303_SWITCH_PORT_REG(port, regnum), val);
> +}
> +
>  static int lan9303_detect_phy_setup(struct lan9303 *chip)
>  {
>  	int reg;
> @@ -709,19 +716,18 @@ static void lan9303_get_ethtool_stats(struct dsa_switch *ds, int port,
>  				      uint64_t *data)
>  {
>  	struct lan9303 *chip = ds->priv;
> -	u32 reg;
> -	unsigned int u, poff;
> -	int ret;
> -
> -	poff = port * 0x400;
> +	unsigned int u;
>  
>  	for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
> -		ret = lan9303_read_switch_reg(chip,
> -					      lan9303_mib[u].offset + poff,
> -					      &reg);
> +		u32 reg;
> +		int ret;
> +
> +		ret = lan9303_read_switch_port(
> +			chip, port, lan9303_mib[u].offset, &reg);
> +
>  		if (ret)
> -			dev_warn(chip->dev, "Reading status reg %u failed\n",
> -				 lan9303_mib[u].offset + poff);
> +			dev_warn(chip->dev, "Reading status port %d reg %u failed\n",
> +				 port, lan9303_mib[u].offset);
>  		data[u] = reg;
>  	}
>  }
> 


-- 
Florian

Re: [PATCH v3 net-next 3/5] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage

[Florian Fainelli]

On 08/03/2017 02:45 AM, Egil Hjelmeland wrote:
> Simplify usage of lan9303_enable_packet_processing,
> lan9303_disable_packet_processing()
> 
> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

took a little while to figure out that we are utilizing fall through of
the switch/case statement and that's why it's okay.

> ---
>  drivers/net/dsa/lan9303-core.c | 24 ++++++++++--------------
>  1 file changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> index 126e8b84bdf0..31fe66fbe39a 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -564,15 +564,16 @@ static int lan9303_handle_reset(struct lan9303 *chip)
>  /* stop processing packets for all ports */
>  static int lan9303_disable_processing(struct lan9303 *chip)
>  {
> -	int ret;
> +	int p;
>  
> -	ret = lan9303_disable_packet_processing(chip, 0);
> -	if (ret)
> -		return ret;
> -	ret = lan9303_disable_packet_processing(chip, 1);
> -	if (ret)
> -		return ret;
> -	return lan9303_disable_packet_processing(chip, 2);
> +	for (p = 0; p < LAN9303_NUM_PORTS; p++) {
> +		int ret = lan9303_disable_packet_processing(chip, p);
> +
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
>  }
>  
>  static int lan9303_check_device(struct lan9303 *chip)
> @@ -765,7 +766,6 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port,
>  	/* enable internal packet processing */
>  	switch (port) {
>  	case 1:
> -		return lan9303_enable_packet_processing(chip, port);
>  	case 2:
>  		return lan9303_enable_packet_processing(chip, port);
>  	default:
> @@ -784,13 +784,9 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port,
>  	/* disable internal packet processing */
>  	switch (port) {
>  	case 1:
> -		lan9303_disable_packet_processing(chip, port);
> -		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 1,
> -				  MII_BMCR, BMCR_PDOWN);
> -		break;
>  	case 2:
>  		lan9303_disable_packet_processing(chip, port);
> -		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 2,
> +		lan9303_phy_write(ds, chip->phy_addr_sel_strap + port,
>  				  MII_BMCR, BMCR_PDOWN);
>  		break;
>  	default:
> 


-- 
Florian

Re: [PATCH v3 net-next 4/5] net: dsa: lan9303: Rename lan9303_xxx_packet_processing()

[Florian Fainelli]

On 08/03/2017 02:45 AM, Egil Hjelmeland wrote:
> The lan9303_enable_packet_processing, lan9303_disable_packet_processing
> functions operate on port, so the names should reflect that.
> And to align with lan9303_disable_processing(), rename:
> 
> lan9303_enable_packet_processing -> lan9303_enable_processing_port
> lan9303_disable_packet_processing -> lan9303_disable_processing_port
> 
> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH v3 net-next 1/5] net: dsa: lan9303: Change lan9303_xxx_packet_processing() port param.

[Florian Fainelli]

On 08/03/2017 02:45 AM, Egil Hjelmeland wrote:
> lan9303_enable_packet_processing, lan9303_disable_packet_processing()
> Pass port number (0,1,2) as parameter instead of port offset.
> Because other functions in the module pass port numbers.
> And to enable simplifications in following patch.
> 
> Introduce lan9303_write_switch_port().
> 
> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
> ---
>  drivers/net/dsa/lan9303-core.c | 60 ++++++++++++++++++++++--------------------
>  1 file changed, 32 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> index 8e430d1ee297..fa19e320c5a8 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -159,9 +159,7 @@
>  # define LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT1 (BIT(9) | BIT(8))
>  # define LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0 (BIT(1) | BIT(0))
>  
> -#define LAN9303_PORT_0_OFFSET 0x400
> -#define LAN9303_PORT_1_OFFSET 0x800
> -#define LAN9303_PORT_2_OFFSET 0xc00
> +#define LAN9303_SWITCH_PORT_REG(port, reg0) (0x400 * (port) + (reg0))
>  
>  /* the built-in PHYs are of type LAN911X */
>  #define MII_LAN911X_SPECIAL_MODES 0x12
> @@ -428,6 +426,13 @@ static int lan9303_read_switch_reg(struct lan9303 *chip, u16 regnum, u32 *val)
>  	return ret;
>  }
>  
> +static int lan9303_write_switch_port(
> +	struct lan9303 *chip, int port, u16 regnum, u32 val)
> +{
> +	return lan9303_write_switch_reg(
> +		chip, LAN9303_SWITCH_PORT_REG(port, regnum), val);
> +}

This argument alignment is not looking too good, can you do this instead:

static int lan9303_write_switch_port(struct lan9303 *chip, int port
				     u16 regnum, u32 *val)
{
}

This applied to patch 5 as well (which should have included it applies
to patch 1 as well).

With that:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

> +
>  static int lan9303_detect_phy_setup(struct lan9303 *chip)
>  {
>  	int reg;
> @@ -458,24 +463,23 @@ static int lan9303_detect_phy_setup(struct lan9303 *chip)
>  	return 0;
>  }
>  
> -#define LAN9303_MAC_RX_CFG_OFFS (LAN9303_MAC_RX_CFG_0 - LAN9303_PORT_0_OFFSET)
> -#define LAN9303_MAC_TX_CFG_OFFS (LAN9303_MAC_TX_CFG_0 - LAN9303_PORT_0_OFFSET)
> -
>  static int lan9303_disable_packet_processing(struct lan9303 *chip,
>  					     unsigned int port)
>  {
>  	int ret;
>  
>  	/* disable RX, but keep register reset default values else */
> -	ret = lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_OFFS + port,
> -				       LAN9303_MAC_RX_CFG_X_REJECT_MAC_TYPES);
> +	ret = lan9303_write_switch_port(
> +			chip, port, LAN9303_MAC_RX_CFG_0,
> +			LAN9303_MAC_RX_CFG_X_REJECT_MAC_TYPES);
>  	if (ret)
>  		return ret;
>  
>  	/* disable TX, but keep register reset default values else */
> -	return lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_OFFS + port,
> -				LAN9303_MAC_TX_CFG_X_TX_IFG_CONFIG_DEFAULT |
> -				LAN9303_MAC_TX_CFG_X_TX_PAD_ENABLE);
> +	return lan9303_write_switch_port(
> +			chip, port, LAN9303_MAC_TX_CFG_0,
> +			LAN9303_MAC_TX_CFG_X_TX_IFG_CONFIG_DEFAULT |
> +			LAN9303_MAC_TX_CFG_X_TX_PAD_ENABLE);

Same here, please don't re-align the arguments, they were fine already.

>  }
>  
>  static int lan9303_enable_packet_processing(struct lan9303 *chip,
> @@ -484,17 +488,19 @@ static int lan9303_enable_packet_processing(struct lan9303 *chip,
>  	int ret;
>  
>  	/* enable RX and keep register reset default values else */
> -	ret = lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_OFFS + port,
> -				       LAN9303_MAC_RX_CFG_X_REJECT_MAC_TYPES |
> -				       LAN9303_MAC_RX_CFG_X_RX_ENABLE);
> +	ret = lan9303_write_switch_port(
> +			chip, port, LAN9303_MAC_RX_CFG_0,
> +			LAN9303_MAC_RX_CFG_X_REJECT_MAC_TYPES |
> +			LAN9303_MAC_RX_CFG_X_RX_ENABLE);
>  	if (ret)
>  		return ret;
>  
>  	/* enable TX and keep register reset default values else */
> -	return lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_OFFS + port,
> -				LAN9303_MAC_TX_CFG_X_TX_IFG_CONFIG_DEFAULT |
> -				LAN9303_MAC_TX_CFG_X_TX_PAD_ENABLE |
> -				LAN9303_MAC_TX_CFG_X_TX_ENABLE);
> +	return lan9303_write_switch_port(
> +			chip, port, LAN9303_MAC_TX_CFG_0,
> +			LAN9303_MAC_TX_CFG_X_TX_IFG_CONFIG_DEFAULT |
> +			LAN9303_MAC_TX_CFG_X_TX_PAD_ENABLE |
> +			LAN9303_MAC_TX_CFG_X_TX_ENABLE);
>  }
>  
>  /* We want a special working switch:
> @@ -558,13 +564,13 @@ static int lan9303_disable_processing(struct lan9303 *chip)
>  {
>  	int ret;
>  
> -	ret = lan9303_disable_packet_processing(chip, LAN9303_PORT_0_OFFSET);
> +	ret = lan9303_disable_packet_processing(chip, 0);
>  	if (ret)
>  		return ret;
> -	ret = lan9303_disable_packet_processing(chip, LAN9303_PORT_1_OFFSET);
> +	ret = lan9303_disable_packet_processing(chip, 1);
>  	if (ret)
>  		return ret;
> -	return lan9303_disable_packet_processing(chip, LAN9303_PORT_2_OFFSET);
> +	return lan9303_disable_packet_processing(chip, 2);
>  }
>  
>  static int lan9303_check_device(struct lan9303 *chip)
> @@ -634,7 +640,7 @@ static int lan9303_setup(struct dsa_switch *ds)
>  	if (ret)
>  		dev_err(chip->dev, "failed to separate ports %d\n", ret);
>  
> -	ret = lan9303_enable_packet_processing(chip, LAN9303_PORT_0_OFFSET);
> +	ret = lan9303_enable_packet_processing(chip, 0);
>  	if (ret)
>  		dev_err(chip->dev, "failed to re-enable switching %d\n", ret);
>  
> @@ -757,11 +763,9 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port,
>  	/* enable internal packet processing */
>  	switch (port) {
>  	case 1:
> -		return lan9303_enable_packet_processing(chip,
> -							LAN9303_PORT_1_OFFSET);
> +		return lan9303_enable_packet_processing(chip, port);
>  	case 2:
> -		return lan9303_enable_packet_processing(chip,
> -							LAN9303_PORT_2_OFFSET);
> +		return lan9303_enable_packet_processing(chip, port);
>  	default:
>  		dev_dbg(chip->dev,
>  			"Error: request to power up invalid port %d\n", port);
> @@ -778,12 +782,12 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port,
>  	/* disable internal packet processing */
>  	switch (port) {
>  	case 1:
> -		lan9303_disable_packet_processing(chip, LAN9303_PORT_1_OFFSET);
> +		lan9303_disable_packet_processing(chip, port);
>  		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 1,
>  				  MII_BMCR, BMCR_PDOWN);
>  		break;
>  	case 2:
> -		lan9303_disable_packet_processing(chip, LAN9303_PORT_2_OFFSET);
> +		lan9303_disable_packet_processing(chip, port);
>  		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 2,
>  				  MII_BMCR, BMCR_PDOWN);
>  		break;
> 


-- 
Florian

Re: [PATCH v3 net-next 3/5] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage

[Egil Hjelmeland]

Den 03. aug. 2017 20:06, skrev Florian Fainelli:
> On 08/03/2017 02:45 AM, Egil Hjelmeland wrote:
>> Simplify usage of lan9303_enable_packet_processing,
>> lan9303_disable_packet_processing()
>>
>> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
> 
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> took a little while to figure out that we are utilizing fall through of
> the switch/case statement and that's why it's okay.
> 
>>
>>   static int lan9303_check_device(struct lan9303 *chip)
>> @@ -765,7 +766,6 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port,
>>   	/* enable internal packet processing */
>>   	switch (port) {
>>   	case 1:
>> -		return lan9303_enable_packet_processing(chip, port);
>>   	case 2:
>>   		return lan9303_enable_packet_processing(chip, port);
>>   	default:

I suppose if we later change to dsa_switch_alloc(...,3), then it could
be further simplified to

if (port != 0)
	return lan9303_enable_packet_processing(chip, port);

Or perhaps no test is needed at all. The driver assumes port 0 is cpu
port, which is the sensible way to use the chip. (Because port 0 has
no phy, the others have phy). Declaring a different port as cpu port in
DTS will not work, but it will not crash the kernel.

Egil

Re: [PATCH v3 net-next 5/5] net: dsa: lan9303: refactor lan9303_get_ethtool_stats

[Egil Hjelmeland]

Den 03. aug. 2017 20:04, skrev Florian Fainelli:
> On 08/03/2017 02:45 AM, Egil Hjelmeland wrote:
>> In lan9303_get_ethtool_stats: Get rid of 0x400 constant magic
>> by using new lan9303_read_switch_reg() inside loop.
>> Reduced scope of two variables.
>>
>> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
>> ---
>>   drivers/net/dsa/lan9303-core.c | 26 ++++++++++++++++----------
>>   1 file changed, 16 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
>> index 6f409755ba1a..5aaa46146c27 100644
>> --- a/drivers/net/dsa/lan9303-core.c
>> +++ b/drivers/net/dsa/lan9303-core.c
>> @@ -435,6 +435,13 @@ static int lan9303_write_switch_port(
>>   		chip, LAN9303_SWITCH_PORT_REG(port, regnum), val);
>>   }
>>   
>> +static int lan9303_read_switch_port(
>> +	struct lan9303 *chip, int port, u16 regnum, u32 *val)
>> +{
> 
> This indentation is really funny, why not just break it up that way:
> 
> static int lan9303_read_switch_port(struct lan9303 *chip, int port
> 				    u16 regnum, u32 *val)
> {
> }
> 
> This applies to patch 5 as well, other than that:
> 
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> 

Because it is the form that passes scripts/checkpatch.pl which I find
easiest to type and maintain.

- No need to fine tune spaces.
- No need to change indentation if later renaming function to name of
   different length.

Do you have any references backing up your claim this is wrong?

Cheers
Egil