Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751996AbdHCSI7 (ORCPT ); Thu, 3 Aug 2017 14:08:59 -0400 Received: from mail-wm0-f66.google.com ([74.125.82.66]:33409 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751870AbdHCSI6 (ORCPT ); Thu, 3 Aug 2017 14:08:58 -0400 Subject: Re: [PATCH v3 net-next 1/5] net: dsa: lan9303: Change lan9303_xxx_packet_processing() port param. To: Egil Hjelmeland , andrew@lunn.ch, vivien.didelot@savoirfairelinux.com, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, kernel@pengutronix.de References: <20170803094507.3439-1-privat@egil-hjelmeland.no> <20170803094507.3439-2-privat@egil-hjelmeland.no> From: Florian Fainelli Message-ID: Date: Thu, 3 Aug 2017 11:08:52 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <20170803094507.3439-2-privat@egil-hjelmeland.no> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6514 Lines: 179 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 > --- > 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 > + > 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