Re: [RFC Patch net-next v2 12/15] net: dsa: microchip: ksz9477: separate phylink mode from switch register

[<Arun.Ramadoss@microchip.com>]

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
> > 
> 
>