Re: [RFC Patch net-next v2 10/15] net: dsa: microchip: move the setup, get_phy_flags & mtu to ksz_common

[<Arun.Ramadoss@microchip.com>]

On Tue, 2022-06-14 at 11:15 +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:52PM +0530, Arun Ramadoss wrote:
> > This patch assigns the .setup, get_phy_flags & mtu  hook of ksz8795
> > and
> > ksz9477 in dsa_switch_ops to ksz_common. And the individual
> > switches
> > setup implementations are called based on the ksz_dev_ops.  For
> > get_phy_flags hooks,checks whether the chip is ksz8863/kss8793 then
> > it
> > returns error for port1.
> > 
> > Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
> > ---
> >  drivers/net/dsa/microchip/ksz8795.c    | 17 ++-------
> >  drivers/net/dsa/microchip/ksz9477.c    | 14 ++++----
> >  drivers/net/dsa/microchip/ksz_common.c | 50
> > ++++++++++++++++++++++++++
> >  drivers/net/dsa/microchip/ksz_common.h |  7 ++++
> >  4 files changed, 68 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/microchip/ksz8795.c
> > b/drivers/net/dsa/microchip/ksz8795.c
> > index 528de481b319..1058b6883caa 100644
> > --- a/drivers/net/dsa/microchip/ksz8795.c
> > +++ b/drivers/net/dsa/microchip/ksz8795.c
> > @@ -898,18 +898,6 @@ static void ksz8_w_phy(struct ksz_device *dev,
> > u16 phy, u16 reg, u16 val)
> >       }
> >  }
> > 
> > diff --git a/drivers/net/dsa/microchip/ksz9477.c
> > b/drivers/net/dsa/microchip/ksz9477.c
> > index d70e0c32b309..d7474d9d4384 100644
> > --- a/drivers/net/dsa/microchip/ksz9477.c
> > +++ b/drivers/net/dsa/microchip/ksz9477.c
> > @@ -47,9 +47,8 @@ static void ksz9477_port_cfg32(struct ksz_device
> > *dev, int port, int offset,
> >                          bits, set ? bits : 0);
> >  }
> > 
> > diff --git a/drivers/net/dsa/microchip/ksz_common.c
> > b/drivers/net/dsa/microchip/ksz_common.c
> > index 8f79ff1ac648..19f8e492d3aa 100644
> > --- a/drivers/net/dsa/microchip/ksz_common.c
> > +++ b/drivers/net/dsa/microchip/ksz_common.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/if_bridge.h>
> >  #include <linux/of_device.h>
> >  #include <linux/of_net.h>
> > +#include <linux/micrel_phy.h>
> >  #include <net/dsa.h>
> >  #include <net/switchdev.h>
> > 
> > @@ -593,6 +594,14 @@ static void ksz_update_port_member(struct
> > ksz_device *dev, int port)
> >       dev->dev_ops->cfg_port_member(dev, port, port_member |
> > cpu_port);
> >  }
> > 
> > +int ksz_setup(struct dsa_switch *ds)
> > +{
> > +     struct ksz_device *dev = ds->priv;
> > +
> > +     return dev->dev_ops->setup(ds);
> > +}
> > +EXPORT_SYMBOL_GPL(ksz_setup);
> 
> I see these changes as being of questionable value if you do not plan
> to
> actually share some code between ->setup implementations. What would
> be
> desirable is if ksz_common.c decides what to do, and ksz8795.c /
> ksz9477.c only offer the implementations for fine-grained dev_ops.
> Currently there is code that can be reused between the setup
> functions
> of the 2 drivers, but also there is some divergence in configuration
> which doesn't have its place (10% rate limit for broadcast storm
> protection in ksz8795?). The goal should be for individual switch
> drivers to not apply configuration behind the curtains as much as
> possible.

Ok. I will try to move common configuration between the switches to
ksz->setup and have less configuration in the individual switch setup.

> 
> > +
> >  static void port_r_cnt(struct ksz_device *dev, int port)
> >  {
> >       struct ksz_port_mib *mib = &dev->ports[port].mib;
> > @@ -692,6 +701,23 @@ int ksz_phy_write16(struct dsa_switch *ds, int
> > addr, int reg, u16 val)
> >  }
> >  EXPORT_SYMBOL_GPL(ksz_phy_write16);
> > 
> > +u32 ksz_get_phy_flags(struct dsa_switch *ds, int port)
> > +{
> > +     struct ksz_device *dev = ds->priv;
> > +
> > +     if (dev->chip_id == KSZ8830_CHIP_ID) {
> > +             /* Silicon Errata Sheet (DS80000830A):
> > +              * Port 1 does not work with LinkMD Cable-Testing.
> > +              * Port 1 does not respond to received PAUSE control
> > frames.
> > +              */
> > +             if (!port)
> > +                     return MICREL_KSZ8_P1_ERRATA;
> > +     }
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(ksz_get_phy_flags);
> > 
> > --
> > 2.36.1
> > 
> 
>