Messages in this thread | | | From | <> | Subject | Re: [RFC Patch net-next v2 12/15] net: dsa: microchip: ksz9477: separate phylink mode from switch register | Date | Wed, 15 Jun 2022 08:49:46 +0000 |
| |
On Tue, 2022-06-14 at 11:24 +0300, Vladimir Oltean wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Mon, May 30, 2022 at 04:12:54PM +0530, Arun Ramadoss wrote: > > As per 'commit 3506b2f42dff ("net: dsa: microchip: call > > phy_remove_link_mode during probe")' phy_remove_link_mode is added > > in > > the switch_register function after dsa_switch_register. In order to > > have > > the common switch register function, moving this phy init after > > dsa_register_switch using the new ksz_dev_ops.dsa_init hook. > > > > Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com> > > --- > > drivers/net/dsa/microchip/ksz9477.c | 49 ++++++++++++++------ > > ------ > > drivers/net/dsa/microchip/ksz_common.c | 5 ++- > > drivers/net/dsa/microchip/ksz_common.h | 1 + > > 3 files changed, 31 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/net/dsa/microchip/ksz9477.c > > b/drivers/net/dsa/microchip/ksz9477.c > > index ecce99b77ef6..c87ce0e2afd8 100644 > > --- a/drivers/net/dsa/microchip/ksz9477.c > > +++ b/drivers/net/dsa/microchip/ksz9477.c > > @@ -1349,6 +1349,30 @@ static void ksz9477_switch_exit(struct > > ksz_device *dev) > > ksz9477_reset_switch(dev); > > } > > > > +static int ksz9477_dsa_init(struct ksz_device *dev) > > +{ > > + struct phy_device *phydev; > > + int i; > > + > > + for (i = 0; i < dev->phy_port_cnt; ++i) { > > + if (!dsa_is_user_port(dev->ds, i)) > > + continue; > > I understand this is just code movement, but this is more efficient: > > struct dsa_switch *ds = dev->ds; > struct dsa_port *dp; > > dsa_switch_for_each_user_port(dp, ds) { > ... > }
Yes. I will use the macro where ever possible.
> > > + > > + phydev = dsa_to_port(dev->ds, i)->slave->phydev; > > + > > + /* The MAC actually cannot run in 1000 half-duplex > > mode. */ > > + phy_remove_link_mode(phydev, > > + ETHTOOL_LINK_MODE_1000baseT_Half > > _BIT); > > + > > + /* PHY does not support gigabit. */ > > + if (!(dev->features & GBIT_SUPPORT)) > > + phy_remove_link_mode(phydev, > > + ETHTOOL_LINK_MODE_1000ba > > seT_Full_BIT); > > + } > > I wonder why the driver did not just remove these from the supported > mask in the phylink validation procedure in the first place? > Adding these link mode fixups to a dev_ops callback named "dsa_init" > does not sound quite right.
So, it means if the link modes are updated correctly in the phylink_get_caps then we don't need these link mode removal. Is my understanding correct?
> > > + > > + return 0; > > +} > > + > > static const struct ksz_dev_ops ksz9477_dev_ops = { > > .setup = ksz9477_setup, > > .get_port_addr = ksz9477_get_port_addr, > > @@ -1377,35 +1401,14 @@ static const struct ksz_dev_ops > > ksz9477_dev_ops = { > > .change_mtu = ksz9477_change_mtu, > > .max_mtu = ksz9477_max_mtu, > > .shutdown = ksz9477_reset_switch, > > + .dsa_init = ksz9477_dsa_init, > > .init = ksz9477_switch_init, > > .exit = ksz9477_switch_exit, > > }; > > > > diff --git a/drivers/net/dsa/microchip/ksz_common.h > > b/drivers/net/dsa/microchip/ksz_common.h > > index 872d378ac45c..23962f47df46 100644 > > --- a/drivers/net/dsa/microchip/ksz_common.h > > +++ b/drivers/net/dsa/microchip/ksz_common.h > > @@ -213,6 +213,7 @@ struct ksz_dev_ops { > > void (*freeze_mib)(struct ksz_device *dev, int port, bool > > freeze); > > void (*port_init_cnt)(struct ksz_device *dev, int port); > > int (*shutdown)(struct ksz_device *dev); > > + int (*dsa_init)(struct ksz_device *dev); > > int (*init)(struct ksz_device *dev); > > void (*exit)(struct ksz_device *dev); > > }; > > -- > > 2.36.1 > > > >
| |