First patch:
Change lan9303_enable_packet_processing,
lan9303_disable_packet_processing():
Pass port number (0,1,2) as parameter instead of port offset.
Plus replaced a constant 0x400 with LAN9303_SWITCH_PORT_REG()
Second patch: Simplify accordingly.
Comments welcome!
Egil Hjelmeland (2):
net: dsa: lan9303: Refactor lan9303_xxx_packet_processing()
net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage
drivers/net/dsa/lan9303-core.c | 73 ++++++++++++++++++++----------------------
1 file changed, 35 insertions(+), 38 deletions(-)
--
2.11.0
lan9303_enable_packet_processing, lan9303_disable_packet_processing()
Pass port number (0,1,2) as parameter instead of port offset.
Plus replaced a constant 0x400 with LAN9303_SWITCH_PORT_REG().
Signed-off-by: Egil Hjelmeland <[email protected]>
---
drivers/net/dsa/lan9303-core.c | 59 +++++++++++++++++++++---------------------
1 file changed, 30 insertions(+), 29 deletions(-)
diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 8e430d1ee297..4d2bb8144c15 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
@@ -458,24 +456,25 @@ 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_reg(
+ chip,
+ LAN9303_SWITCH_PORT_REG(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_reg(
+ chip,
+ LAN9303_SWITCH_PORT_REG(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 +483,21 @@ 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_reg(
+ chip,
+ LAN9303_SWITCH_PORT_REG(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_reg(
+ chip,
+ LAN9303_SWITCH_PORT_REG(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 +561,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 +637,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);
@@ -704,7 +707,7 @@ static void lan9303_get_ethtool_stats(struct dsa_switch *ds, int port,
unsigned int u, poff;
int ret;
- poff = port * 0x400;
+ poff = LAN9303_SWITCH_PORT_REG(port, 0);
for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
ret = lan9303_read_switch_reg(chip,
@@ -757,11 +760,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 +779,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
Simplify usage of lan9303_enable_packet_processing,
lan9303_disable_packet_processing()
Signed-off-by: Egil Hjelmeland <[email protected]>
---
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 4d2bb8144c15..705267a1d2ba 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -559,15 +559,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 <= 2; p++) {
+ int ret;
+
+ ret = lan9303_disable_packet_processing(chip, p);
+ if (ret)
+ return ret;
+ }
+ return 0;
}
static int lan9303_check_device(struct lan9303 *chip)
@@ -760,7 +761,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:
@@ -779,13 +779,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
On Mon, Jul 31, 2017 at 01:33:53PM +0200, Egil Hjelmeland wrote:
> First patch:
> Change lan9303_enable_packet_processing,
> lan9303_disable_packet_processing():
> Pass port number (0,1,2) as parameter instead of port offset.
> Plus replaced a constant 0x400 with LAN9303_SWITCH_PORT_REG()
Hi Egil
The cover note and the commit message should concentrate on the Why,
not the What. I can see the What by reading the patch.
Why do you want to pass port numbers?
Note that for netdev, the cover note also ends up in git as part of
the history.
Andrew
On 31. juli 2017 15:22, Andrew Lunn wrote:
> On Mon, Jul 31, 2017 at 01:33:53PM +0200, Egil Hjelmeland wrote:
>> First patch:
>> Change lan9303_enable_packet_processing,
>> lan9303_disable_packet_processing():
>> Pass port number (0,1,2) as parameter instead of port offset.
>> Plus replaced a constant 0x400 with LAN9303_SWITCH_PORT_REG()
>
> Hi Egil
>
> The cover note and the commit message should concentrate on the Why,
> not the What. I can see the What by reading the patch.
>
> Why do you want to pass port numbers?
>
Because other functions in the module pass port numbers. And to enable
the simplifications.
> Note that for netdev, the cover note also ends up in git as part of
> the history.
>
OK, I did not know that netdev does that.
> Andrew
>
On Mon, Jul 31, 2017 at 01:33:54PM +0200, Egil Hjelmeland wrote:
> lan9303_enable_packet_processing, lan9303_disable_packet_processing()
> Pass port number (0,1,2) as parameter instead of port offset.
>
> Plus replaced a constant 0x400 with LAN9303_SWITCH_PORT_REG().
>
> Signed-off-by: Egil Hjelmeland <[email protected]>
> ---
> drivers/net/dsa/lan9303-core.c | 59 +++++++++++++++++++++---------------------
> 1 file changed, 30 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> index 8e430d1ee297..4d2bb8144c15 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
> @@ -458,24 +456,25 @@ 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_reg(
> + chip,
> + LAN9303_SWITCH_PORT_REG(port, LAN9303_MAC_RX_CFG_0),
> + LAN9303_MAC_RX_CFG_X_REJECT_MAC_TYPES);
Hi Egil
As you can see from the indentation change, this is rather long. What
we have in mv88e6xxx are functions which act on a port. You could have
a function:
lan9303_write_switch_port(struct lan9303 chip, unsigned int port, u16 reg,
u32 val)
> 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_reg(
> + chip,
> + LAN9303_SWITCH_PORT_REG(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 +483,21 @@ 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_reg(
> + chip,
> + LAN9303_SWITCH_PORT_REG(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_reg(
> + chip,
> + LAN9303_SWITCH_PORT_REG(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 +561,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 +637,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);
>
> @@ -704,7 +707,7 @@ static void lan9303_get_ethtool_stats(struct dsa_switch *ds, int port,
> unsigned int u, poff;
> int ret;
>
> - poff = port * 0x400;
> + poff = LAN9303_SWITCH_PORT_REG(port, 0);
>
> for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
> ret = lan9303_read_switch_reg(chip,
> @@ -757,11 +760,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);
case 1 and 2 are now identical. So you can simplify this.
> default:
> dev_dbg(chip->dev,
> "Error: request to power up invalid port %d\n", port);
> @@ -778,12 +779,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);
And this can also be simplified.
Andrew
On Mon, Jul 31, 2017 at 01:33:55PM +0200, Egil Hjelmeland wrote:
> Simplify usage of lan9303_enable_packet_processing,
> lan9303_disable_packet_processing()
>
> Signed-off-by: Egil Hjelmeland <[email protected]>
> ---
> 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 4d2bb8144c15..705267a1d2ba 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -559,15 +559,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 <= 2; p++) {
> + int ret;
> +
> + ret = lan9303_disable_packet_processing(chip, p);
> + if (ret)
> + return ret;
> + }
> + return 0;
> }
>
> static int lan9303_check_device(struct lan9303 *chip)
> @@ -760,7 +761,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:
> @@ -779,13 +779,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:
:-)
So maybe i would squash this part into the previous patch. You were
touching the functions anyway, and the change is obvious, so easy to
review.
But it is also O.K. this way. The cover note could of helped. You
could of said something like: "Changes made in the first patch allow
some simplifications to be made in the same code in the second patch.
Breaking changes up is hard, and you cannot please everybody, all the
time.
Reviewed-by: Andrew Lunn <[email protected]>
Andrew
Hi Egil,
A few nitpicks below for lan9303_disable_processing.
Egil Hjelmeland <[email protected]> writes:
> 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 <= 2; p++) {
Exclusive limits are often prefer, i.e. 'p < 3'.
> + int ret;
> +
> + ret = lan9303_disable_packet_processing(chip, p);
> + if (ret)
> + return ret;
When any non-zero return code means an error, we usually see 'err'
instead of 'ret'.
> + }
blank line before return statments are appreciated.
> + return 0;
> }
>
> static int lan9303_check_device(struct lan9303 *chip)
> @@ -760,7 +761,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);
Is this deletion intentional? The commit message does not explain this.
When possible, it is appreciated to separate functional from
non-functional changes. For example one commit adding the loop in
lan9303_disable_processing and another one to not enable/disable packet
processing on port 1.
> case 2:
> return lan9303_enable_packet_processing(chip, port);
> default:
> @@ -779,13 +779,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;
Thanks,
Vivien
On 31. juli 2017 15:46, Andrew Lunn wrote:
> On Mon, Jul 31, 2017 at 01:33:55PM +0200, Egil Hjelmeland wrote:
>> Simplify usage of lan9303_enable_packet_processing,
>> lan9303_disable_packet_processing()
>>
>> Signed-off-by: Egil Hjelmeland <[email protected]>
>> ---
>> 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 4d2bb8144c15..705267a1d2ba 100644
>> --- a/drivers/net/dsa/lan9303-core.c
>> +++ b/drivers/net/dsa/lan9303-core.c
>> @@ -559,15 +559,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 <= 2; p++) {
>> + int ret;
>> +
>> + ret = lan9303_disable_packet_processing(chip, p);
>> + if (ret)
>> + return ret;
>> + }
>> + return 0;
>> }
>>
>> static int lan9303_check_device(struct lan9303 *chip)
>> @@ -760,7 +761,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:
>> @@ -779,13 +779,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:
>
> :-)
>
> So maybe i would squash this part into the previous patch. You were
> touching the functions anyway, and the change is obvious, so easy to
> review.
>
> But it is also O.K. this way. The cover note could of helped. You
> could of said something like: "Changes made in the first patch allow
> some simplifications to be made in the same code in the second patch.
>
> Breaking changes up is hard, and you cannot please everybody, all the
> time.
>
> Reviewed-by: Andrew Lunn <[email protected]>
>
> Andrew
>
Hi Andrew
This time I took the extra effort to split my original patch...
Your lan9303_write_switch_port suggestion (in previous reply) is fine.
And I can improve the coverletter.
So I will do a v2 of the patch. But what is your advice:
Should I squash the patch?
Egil
On 31. juli 2017 16:00, Vivien Didelot wrote:
> Hi Egil,
>
> A few nitpicks below for lan9303_disable_processing.
>
> Egil Hjelmeland <[email protected]> writes:
>
>> 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 <= 2; p++) {
>
> Exclusive limits are often prefer, i.e. 'p < 3'.
>
OK, that can be nice when I later introduce LAN9303_NUM_PORTS = 3.
>> + int ret;
>> +
>> + ret = lan9303_disable_packet_processing(chip, p);
>> + if (ret)
>> + return ret;
>
> When any non-zero return code means an error, we usually see 'err'
> instead of 'ret'.
>
But 'ret' is used throughout the rest of the file. Is it not better to
be locally consistent?
>> + }
>
> blank line before return statments are appreciated.
>
OK
>> + return 0;
>> }
>>
>> static int lan9303_check_device(struct lan9303 *chip)
>> @@ -760,7 +761,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);
>
> Is this deletion intentional? The commit message does not explain this.
>
> When possible, it is appreciated to separate functional from
> non-functional changes. For example one commit adding the loop in
> lan9303_disable_processing and another one to not enable/disable packet
> processing on port 1.
>
Case fall through, the change is purely non-functional.
You are perhaps thinking of the patch in my first series where I removed
disable of port 0. I have put that on hold. Juergen says that the
mainline driver works out of the box for him. So I will investigate
that problem bit more.
>> case 2:
>> return lan9303_enable_packet_processing(chip, port);
>> default:
>> @@ -779,13 +779,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;
>
> Thanks,
>
> Vivien
>
Egil
Hi Egil,
Egil Hjelmeland <[email protected]> writes:
>>> + for (p = 0; p <= 2; p++) {
>>
>> Exclusive limits are often prefer, i.e. 'p < 3'.
>>
> OK, that can be nice when I later introduce LAN9303_NUM_PORTS = 3.
This is indeed another reason what exclusive limits are prefered ;-)
>>> + int ret;
>>> +
>>> + ret = lan9303_disable_packet_processing(chip, p);
>>> + if (ret)
>>> + return ret;
>>
>> When any non-zero return code means an error, we usually see 'err'
>> instead of 'ret'.
>>
>
> But 'ret' is used throughout the rest of the file. Is it not better to
> be locally consistent?
You are correct, I was missing a bit of context here.
>>> case 1:
>>> - return lan9303_enable_packet_processing(chip, port);
>>
>> Is this deletion intentional? The commit message does not explain this.
>>
>> When possible, it is appreciated to separate functional from
>> non-functional changes. For example one commit adding the loop in
>> lan9303_disable_processing and another one to not enable/disable packet
>> processing on port 1.
>>
>
> Case fall through, the change is purely non-functional.
>
> You are perhaps thinking of the patch in my first series where I removed
> disable of port 0. I have put that on hold. Juergen says that the
> mainline driver works out of the box for him. So I will investigate
> that problem bit more.
Correct! I misread, my bad. This is indeed cleaner with this patch. With
the LAN9303_NUM_PORTS limit and detailed commit message, the patch LGTM.
Thanks,
Vivien
> >Reviewed-by: Andrew Lunn <[email protected]>
> >
> > Andrew
> >
>
> Hi Andrew
>
> This time I took the extra effort to split my original patch...
>
> Your lan9303_write_switch_port suggestion (in previous reply) is fine.
> And I can improve the coverletter.
>
> So I will do a v2 of the patch. But what is your advice:
> Should I squash the patch?
I already gave my Reviewed-by:, meaning i don't really care. Merged or
squashed is a minor point in this case.
Andrew
On 31. juli 2017 16:43, Vivien Didelot wrote:
> Hi Egil,
>
> Egil Hjelmeland <[email protected]> writes:
>
>>>> + for (p = 0; p <= 2; p++) {
>>>
>>> Exclusive limits are often prefer, i.e. 'p < 3'.
>>>
>> OK, that can be nice when I later introduce LAN9303_NUM_PORTS = 3.
>
> This is indeed another reason what exclusive limits are prefered ;-)
>
>>>> + int ret;
>>>> +
>>>> + ret = lan9303_disable_packet_processing(chip, p);
>>>> + if (ret)
>>>> + return ret;
>>>
>>> When any non-zero return code means an error, we usually see 'err'
>>> instead of 'ret'.
>>>
>>
>> But 'ret' is used throughout the rest of the file. Is it not better to
>> be locally consistent?
>
> You are correct, I was missing a bit of context here.
>
>>>> case 1:
>>>> - return lan9303_enable_packet_processing(chip, port);
>>>
>>> Is this deletion intentional? The commit message does not explain this.
>>>
>>> When possible, it is appreciated to separate functional from
>>> non-functional changes. For example one commit adding the loop in
>>> lan9303_disable_processing and another one to not enable/disable packet
>>> processing on port 1.
>>>
>>
>> Case fall through, the change is purely non-functional.
>>
>> You are perhaps thinking of the patch in my first series where I removed
>> disable of port 0. I have put that on hold. Juergen says that the
>> mainline driver works out of the box for him. So I will investigate
>> that problem bit more.
>
> Correct! I misread, my bad. This is indeed cleaner with this patch. With
> the LAN9303_NUM_PORTS limit and detailed commit message, the patch LGTM.
>
>
> Thanks,
>
> Vivien
>
Would doing
- chip->ds = dsa_switch_alloc(chip->dev, DSA_MAX_PORTS);
+ chip->ds = dsa_switch_alloc(chip->dev, LAN9303_NUM_PORTS);
at the same time be good, or breaking the scope of the patch?
Egil
Hi Egil,
Egil Hjelmeland <[email protected]> writes:
> Would doing
>
> - chip->ds = dsa_switch_alloc(chip->dev, DSA_MAX_PORTS);
> + chip->ds = dsa_switch_alloc(chip->dev, LAN9303_NUM_PORTS);
>
> at the same time be good, or breaking the scope of the patch?
It is indeed out of scope. You may want to add a first commit "net: dsa:
lan9303: introduce LAN9303_NUM_PORTS" for instance.
Thanks,
Vivien
On 31. juli 2017 17:01, Vivien Didelot wrote:
> Hi Egil,
>
> Egil Hjelmeland <[email protected]> writes:
>
>> Would doing
>>
>> - chip->ds = dsa_switch_alloc(chip->dev, DSA_MAX_PORTS);
>> + chip->ds = dsa_switch_alloc(chip->dev, LAN9303_NUM_PORTS);
>>
>> at the same time be good, or breaking the scope of the patch?
>
> It is indeed out of scope. You may want to add a first commit "net: dsa:
> lan9303: introduce LAN9303_NUM_PORTS" for instance.
>
In a later series I assume? Or is allowed to add patches to a series
under review?
>
> Thanks,
>
> Vivien
>
Egil
Hi Egil,
Egil Hjelmeland <[email protected]> writes:
>> It is indeed out of scope. You may want to add a first commit "net: dsa:
>> lan9303: introduce LAN9303_NUM_PORTS" for instance.
>
> In a later series I assume? Or is allowed to add patches to a series
> under review?
If it makes sense to include this first limit patch in this series under
review, you can totally add it. A simple "Changes in v2: - add
LAN9303_NUM_PORTS" list in the cover letter will do the job.
Thanks,
Vivien