Messages in this thread | | | From | Vivien Didelot <> | Subject | Re: [PATCH v2 net-next 1/2] net: dsa: lan9303: Add port_fast_age and port_fdb_dump methods | Date | Thu, 19 Oct 2017 09:58:54 -0400 |
| |
Hi Egil,
Egil Hjelmeland <privat@egil-hjelmeland.no> writes:
> Add DSA method port_fast_age as a step to STP support. > > Add low level functions for accessing the lan9303 ALR (Address Logic > Resolution). > > Added DSA method port_fdb_dump > > Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
The patch looks good overall, a few nitpicks though.
> --- > drivers/net/dsa/lan9303-core.c | 159 +++++++++++++++++++++++++++++++++++++++++ > drivers/net/dsa/lan9303.h | 2 + > 2 files changed, 161 insertions(+) > > diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c > index 09a748327fc6..ae904242b001 100644 > --- a/drivers/net/dsa/lan9303-core.c > +++ b/drivers/net/dsa/lan9303-core.c > @@ -124,6 +124,21 @@ > #define LAN9303_MAC_RX_CFG_2 0x0c01 > #define LAN9303_MAC_TX_CFG_2 0x0c40 > #define LAN9303_SWE_ALR_CMD 0x1800 > +# define ALR_CMD_MAKE_ENTRY BIT(2) > +# define ALR_CMD_GET_FIRST BIT(1) > +# define ALR_CMD_GET_NEXT BIT(0) > +#define LAN9303_SWE_ALR_WR_DAT_0 0x1801 > +#define LAN9303_SWE_ALR_WR_DAT_1 0x1802 > +# define ALR_DAT1_VALID BIT(26) > +# define ALR_DAT1_END_OF_TABL BIT(25) > +# define ALR_DAT1_AGE_OVERRID BIT(25) > +# define ALR_DAT1_STATIC BIT(24) > +# define ALR_DAT1_PORT_BITOFFS 16 > +# define ALR_DAT1_PORT_MASK (7 << ALR_DAT1_PORT_BITOFFS) > +#define LAN9303_SWE_ALR_RD_DAT_0 0x1805 > +#define LAN9303_SWE_ALR_RD_DAT_1 0x1806 > +#define LAN9303_SWE_ALR_CMD_STS 0x1808 > +# define ALR_STS_MAKE_PEND BIT(0)
Why is there different spacing and prefix with these defines?
> #define LAN9303_SWE_VLAN_CMD 0x180b > # define LAN9303_SWE_VLAN_CMD_RNW BIT(5) > # define LAN9303_SWE_VLAN_CMD_PVIDNVLAN BIT(4) > @@ -478,6 +493,125 @@ static int lan9303_detect_phy_setup(struct lan9303 *chip) > return 0; > } > > +/* ----------------- Address Logic Resolution (ALR)------------------*/ > + > +/* Map ALR-port bits to port bitmap, and back*/
The (leading and trailing) spacing in your comments is often inconsistent. You can use simple inline or block comments, no need for fancy rules. Please refer to Documentation/networking/netdev-FAQ.txt for block comment style in netdev though, they are a bit different.
> +static const int alrport_2_portmap[] = {1, 2, 4, 0, 3, 5, 6, 7 }; > +static const int portmap_2_alrport[] = {3, 0, 1, 4, 2, 5, 6, 7 }; > + > +/* ALR: Actual register access functions */ > + > +/* This function will wait a while until mask & reg == value */ > +/* Otherwise, return timeout */
Same, a single block comment will do the job.
> +static int lan9303_csr_reg_wait(struct lan9303 *chip, int regno, > + int mask, char value) > +{ > + int i; > + > + for (i = 0; i < 0x1000; i++) { > + u32 reg; > + > + lan9303_read_switch_reg(chip, regno, ®); > + if ((reg & mask) == value) > + return 0; > + usleep_range(1000, 2000); > + } > + return -ETIMEDOUT; > +} > + > +static int lan9303_alr_make_entry_raw(struct lan9303 *chip, u32 dat0, u32 dat1) > +{ > + lan9303_write_switch_reg( > + chip, LAN9303_SWE_ALR_WR_DAT_0, dat0); > + lan9303_write_switch_reg( > + chip, LAN9303_SWE_ALR_WR_DAT_1, dat1); > + lan9303_write_switch_reg( > + chip, LAN9303_SWE_ALR_CMD, ALR_CMD_MAKE_ENTRY); > + lan9303_csr_reg_wait( > + chip, LAN9303_SWE_ALR_CMD_STS, ALR_STS_MAKE_PEND, 0);
As I said in a previous series, please don't do this. Function arguments must be vertically aligned with the opening parenthesis.
> + lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, 0); > + return 0;
A newline before a return statement is appreciated.
> +} > + > +typedef void alr_loop_cb_t(struct lan9303 *chip, u32 dat0, u32 dat1, > + int portmap, void *ctx); > + > +static void lan9303_alr_loop(struct lan9303 *chip, alr_loop_cb_t *cb, void *ctx) > +{ > + int i; > + > + lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, ALR_CMD_GET_FIRST); > + lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, 0); > + > + for (i = 1; i < LAN9303_NUM_ALR_RECORDS; i++) { > + u32 dat0, dat1; > + int alrport, portmap; > + > + lan9303_read_switch_reg(chip, LAN9303_SWE_ALR_RD_DAT_0, &dat0); > + lan9303_read_switch_reg(chip, LAN9303_SWE_ALR_RD_DAT_1, &dat1); > + if (dat1 & ALR_DAT1_END_OF_TABL) > + break; > + > + alrport = (dat1 & ALR_DAT1_PORT_MASK) >> ALR_DAT1_PORT_BITOFFS; > + portmap = alrport_2_portmap[alrport]; > + > + cb(chip, dat0, dat1, portmap, ctx); > + > + lan9303_write_switch_reg( > + chip, LAN9303_SWE_ALR_CMD, ALR_CMD_GET_NEXT);
Please align arguments with the opening parenthesis.
> + lan9303_write_switch_reg(chip, LAN9303_SWE_ALR_CMD, 0); > + } > +} > + > +/* ALR: lan9303_alr_loop callback functions */ > +
No need for an extra newline if your comment refers directly to the function below. It will also be consistent with the rest of your patch.
> +static void alr_reg_to_mac(u32 dat0, u32 dat1, u8 mac[6]) > +{ > + mac[0] = (dat0 >> 0) & 0xff; > + mac[1] = (dat0 >> 8) & 0xff; > + mac[2] = (dat0 >> 16) & 0xff; > + mac[3] = (dat0 >> 24) & 0xff; > + mac[4] = (dat1 >> 0) & 0xff; > + mac[5] = (dat1 >> 8) & 0xff; > +} > + > +/* Clear learned (non-static) entry on given port */ > +static void alr_loop_cb_del_port_learned(struct lan9303 *chip, u32 dat0, > + u32 dat1, int portmap, void *ctx) > +{ > + int *port = ctx;
You can get the value directly to make the line below more readable:
int port = *(int *)ctx;
> + > + if (((BIT(*port) & portmap) == 0) || (dat1 & ALR_DAT1_STATIC)) > + return; > + > + /* learned entries has only one port, we can just delete */ > + dat1 &= ~ALR_DAT1_VALID; /* delete entry */ > + lan9303_alr_make_entry_raw(chip, dat0, dat1); > +} > + > +struct port_fdb_dump_ctx { > + int port; > + void *data; > + dsa_fdb_dump_cb_t *cb; > +}; > + > +static void alr_loop_cb_fdb_port_dump(struct lan9303 *chip, u32 dat0, > + u32 dat1, int portmap, void *ctx) > +{ > + struct port_fdb_dump_ctx *dump_ctx = ctx; > + u8 mac[ETH_ALEN]; > + bool is_static; > + > + if ((BIT(dump_ctx->port) & portmap) == 0) > + return; > + > + alr_reg_to_mac(dat0, dat1, mac); > + is_static = !!(dat1 & ALR_DAT1_STATIC); > + dump_ctx->cb(mac, 0, is_static, dump_ctx->data); > +} > + > +/* --------------------- Various chip setup ----------------------*/ > +
This isn't a very useful comment, at least use an inline or block comment if you want to keep it.
> static int lan9303_disable_processing_port(struct lan9303 *chip, > unsigned int port) > { > @@ -923,6 +1057,29 @@ static void lan9303_port_stp_state_set(struct dsa_switch *ds, int port, > /* else: touching SWE_PORT_STATE would break port separation */ > } > > +static void lan9303_port_fast_age(struct dsa_switch *ds, int port) > +{ > + struct lan9303 *chip = ds->priv; > + > + dev_dbg(chip->dev, "%s(%d)\n", __func__, port); > + lan9303_alr_loop(chip, alr_loop_cb_del_port_learned, &port); > +} > + > +static int lan9303_port_fdb_dump(struct dsa_switch *ds, int port, > + dsa_fdb_dump_cb_t *cb, void *data) > +{ > + struct lan9303 *chip = ds->priv; > + struct port_fdb_dump_ctx dump_ctx = { > + .port = port, > + .data = data, > + .cb = cb, > + }; > + > + dev_dbg(chip->dev, "%s(%d)\n", __func__, port); > + lan9303_alr_loop(chip, alr_loop_cb_fdb_port_dump, &dump_ctx); > + return 0;
A newline before the return statement is welcome.
> +} > + > static const struct dsa_switch_ops lan9303_switch_ops = { > .get_tag_protocol = lan9303_get_tag_protocol, > .setup = lan9303_setup, > @@ -937,6 +1094,8 @@ static const struct dsa_switch_ops lan9303_switch_ops = { > .port_bridge_join = lan9303_port_bridge_join, > .port_bridge_leave = lan9303_port_bridge_leave, > .port_stp_state_set = lan9303_port_stp_state_set, > + .port_fast_age = lan9303_port_fast_age, > + .port_fdb_dump = lan9303_port_fdb_dump, > }; > > static int lan9303_register_switch(struct lan9303 *chip) > diff --git a/drivers/net/dsa/lan9303.h b/drivers/net/dsa/lan9303.h > index 68ecd544b658..4db323d65741 100644 > --- a/drivers/net/dsa/lan9303.h > +++ b/drivers/net/dsa/lan9303.h > @@ -11,6 +11,8 @@ struct lan9303_phy_ops { > int regnum, u16 val); > }; > > +#define LAN9303_NUM_ALR_RECORDS 512 > + > struct lan9303 { > struct device *dev; > struct regmap *regmap; > -- > 2.11.0
| |