| Message ID | 20240401-v6-8-0-net-next-mv88e6xxx-leds-v4-v3-5-221b3fa55f78@lunn.ch (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: Add generic support for netdev LEDs | expand |
On Mon, Apr 01, 2024 at 08:35:50AM -0500, Andrew Lunn wrote: > The LED helpers make use of a struct netdev. Add helpers a DSA driver > can use to convert a netdev to a struct dsa_switch and the port index. > > To do this, dsa_user_to_port() has to be made available out side of > net/dev, to convert the inline function in net/dsa/user.h into a > normal function, and export it. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > --- I think the API we have today is sufficient: we have dsa_port_to_netdev(), introduced at Vivien's request rather than exporting dsa_user_to_port(). Also, I believe that having a single API function which returns a single struct dsa_port *, from which we force the caller to get the dp->ds and dp->index, is better (cheaper) than requiring 2 API functions, one for getting the ds and the other for the index.
On Tue, Apr 02, 2024 at 01:56:52PM +0300, Vladimir Oltean wrote: > On Mon, Apr 01, 2024 at 08:35:50AM -0500, Andrew Lunn wrote: > > The LED helpers make use of a struct netdev. Add helpers a DSA driver > > can use to convert a netdev to a struct dsa_switch and the port index. > > > > To do this, dsa_user_to_port() has to be made available out side of > > net/dev, to convert the inline function in net/dsa/user.h into a > > normal function, and export it. > > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > > --- > > I think the API we have today is sufficient: we have dsa_port_to_netdev(), > introduced at Vivien's request rather than exporting dsa_user_to_port(). > > Also, I believe that having a single API function which returns a single > struct dsa_port *, from which we force the caller to get the dp->ds and > dp->index, is better (cheaper) than requiring 2 API functions, one for > getting the ds and the other for the index. Yes, I would tend to agree having done this for my experimental phylink changes. struct dsa_port seems to make the most sense: static inline struct dsa_port * dsa_phylink_to_port(struct phylink_config *config) { return container_of(config, struct dsa_port, pl_config); } which then means e.g.: static void mv88e6xxx_mac_config(struct phylink_config *config, unsigned int mode, const struct phylink_link_state *state) { struct dsa_port *dp = dsa_phylink_to_port(config); struct mv88e6xxx_chip *chip = dp->ds->priv; int port = dp->index; int err = 0; mv88e6xxx_reg_lock(chip); if (mode != MLO_AN_PHY || !mv88e6xxx_phy_is_internal(chip, port)) { err = mv88e6xxx_port_config_interface(chip, port, state->interface); if (err && err != -EOPNOTSUPP) goto err_unlock; } err_unlock: mv88e6xxx_reg_unlock(chip); if (err && err != -EOPNOTSUPP) dev_err(dp->ds->dev, "p%d: failed to configure MAC/PCS\n", port); } vs the current code: static void mv88e6xxx_mac_config(struct dsa_switch *ds, int port, unsigned int mode, const struct phylink_link_state *state) { struct mv88e6xxx_chip *chip = ds->priv; int err = 0; mv88e6xxx_reg_lock(chip); if (mode != MLO_AN_PHY || !mv88e6xxx_phy_is_internal(chip, port)) { err = mv88e6xxx_port_config_interface(chip, port, state->interface); if (err && err != -EOPNOTSUPP) goto err_unlock; } err_unlock: mv88e6xxx_reg_unlock(chip); if (err && err != -EOPNOTSUPP) dev_err(ds->dev, "p%d: failed to configure MAC/PCS\n", port); }
On Tue, Apr 02, 2024 at 02:31:34PM +0100, Russell King (Oracle) wrote: > Yes, I would tend to agree having done this for my experimental phylink > changes. struct dsa_port seems to make the most sense: > > static inline struct dsa_port * > dsa_phylink_to_port(struct phylink_config *config) > { > return container_of(config, struct dsa_port, pl_config); > } > > which then means e.g.: > > static void mv88e6xxx_mac_config(struct phylink_config *config, > unsigned int mode, > const struct phylink_link_state *state) > { > struct dsa_port *dp = dsa_phylink_to_port(config); > struct mv88e6xxx_chip *chip = dp->ds->priv; > int port = dp->index; > int err = 0; > > mv88e6xxx_reg_lock(chip); > > if (mode != MLO_AN_PHY || !mv88e6xxx_phy_is_internal(chip, port)) { > err = mv88e6xxx_port_config_interface(chip, port, > state->interface); > if (err && err != -EOPNOTSUPP) > goto err_unlock; > } > > err_unlock: > mv88e6xxx_reg_unlock(chip); > > if (err && err != -EOPNOTSUPP) > dev_err(dp->ds->dev, "p%d: failed to configure MAC/PCS\n", > port); > } Looks ok, looking forward to seeing more of it. Maybe a slight preference to keeping "struct dsa_switch *ds" as a local variable of its own, even if this means declaring and initializing "chip" on separate lines.
diff --git a/include/net/dsa.h b/include/net/dsa.h index 7c0da9effe4e..1fbfada6678d 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -1359,6 +1359,23 @@ int dsa_register_switch(struct dsa_switch *ds); void dsa_switch_shutdown(struct dsa_switch *ds); struct dsa_switch *dsa_switch_find(int tree_index, int sw_index); void dsa_flush_workqueue(void); + +struct dsa_port *dsa_user_to_port(const struct net_device *dev); + +static inline struct dsa_switch *dsa_user_to_ds(const struct net_device *ndev) +{ + struct dsa_port *dp = dsa_user_to_port(ndev); + + return dp->ds; +} + +static inline unsigned int dsa_user_to_index(const struct net_device *ndev) +{ + struct dsa_port *dp = dsa_user_to_port(ndev); + + return dp->index; +} + #ifdef CONFIG_PM_SLEEP int dsa_switch_suspend(struct dsa_switch *ds); int dsa_switch_resume(struct dsa_switch *ds); diff --git a/net/dsa/user.c b/net/dsa/user.c index 16d395bb1a1f..bbee3f63e2c7 100644 --- a/net/dsa/user.c +++ b/net/dsa/user.c @@ -3699,3 +3699,11 @@ void dsa_user_unregister_notifier(void) if (err) pr_err("DSA: failed to unregister user notifier (%d)\n", err); } + +struct dsa_port *dsa_user_to_port(const struct net_device *dev) +{ + struct dsa_user_priv *p = netdev_priv(dev); + + return p->dp; +} +EXPORT_SYMBOL_GPL(dsa_user_to_port); diff --git a/net/dsa/user.h b/net/dsa/user.h index 996069130bea..b6bcf027643e 100644 --- a/net/dsa/user.h +++ b/net/dsa/user.h @@ -51,13 +51,6 @@ int dsa_user_change_conduit(struct net_device *dev, struct net_device *conduit, int dsa_user_manage_vlan_filtering(struct net_device *dev, bool vlan_filtering); -static inline struct dsa_port *dsa_user_to_port(const struct net_device *dev) -{ - struct dsa_user_priv *p = netdev_priv(dev); - - return p->dp; -} - static inline struct net_device * dsa_user_to_conduit(const struct net_device *dev) {
The LED helpers make use of a struct netdev. Add helpers a DSA driver can use to convert a netdev to a struct dsa_switch and the port index. To do this, dsa_user_to_port() has to be made available out side of net/dev, to convert the inline function in net/dsa/user.h into a normal function, and export it. Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- include/net/dsa.h | 17 +++++++++++++++++ net/dsa/user.c | 8 ++++++++ net/dsa/user.h | 7 ------- 3 files changed, 25 insertions(+), 7 deletions(-)