Re: [RFC Patch net-next v2 03/15] net: dsa: microchip: move tag_protocol & phy read/write to ksz_common

[<Arun.Ramadoss@microchip.com>]

On Mon, 2022-06-13 at 12:22 +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:45PM +0530, Arun Ramadoss wrote:
> > This patch move the dsa hook get_tag_protocol to ksz_common file.
> > And
> > the tag_protocol is returned based on the dev->chip_id.
> > ksz8795 and ksz9477 implementation on phy read/write hooks are
> > different. This patch modifies the ksz9477 implementation same as
> > ksz8795 by updating the ksz9477_dev_ops structure.
> > 
> > Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
> > ---
> 
> It would be easier to review if you would split the
> phy_read/phy_write
> change from the get_tag_protocol change.
In the RFC v1, these two are in separate patch. But patchwork errored
like maximum patch in the series should be 15. So I merged them into
one. Is it ok if the patch size increased beyond 15 or should I split
this patch series into two.
> 
> >  drivers/net/dsa/microchip/ksz8795.c    | 13 +--------
> >  drivers/net/dsa/microchip/ksz9477.c    | 37 ++++++++------------
> > ------
> >  drivers/net/dsa/microchip/ksz_common.c | 24 +++++++++++++++++
> >  drivers/net/dsa/microchip/ksz_common.h |  2 ++
> >  4 files changed, 38 insertions(+), 38 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/microchip/ksz8795.c
> > b/drivers/net/dsa/microchip/ksz8795.c
> > index 927db57d02db..6e5f665fa1f6 100644
> > --- a/drivers/net/dsa/microchip/ksz8795.c
> > +++ b/drivers/net/dsa/microchip/ksz8795.c
> > @@ -898,17 +898,6 @@ static void ksz8_w_phy(struct ksz_device *dev,
> > u16 phy, u16 reg, u16 val)
> >       }
> >  }
> > 
> > -static enum dsa_tag_protocol ksz8_get_tag_protocol(struct
> > dsa_switch *ds,
> > -                                                int port,
> > -                                                enum
> > dsa_tag_protocol mp)
> > -{
> > -     struct ksz_device *dev = ds->priv;
> > -
> > -     /* ksz88x3 uses the same tag schema as KSZ9893 */
> > -     return ksz_is_ksz88x3(dev) ?
> > -             DSA_TAG_PROTO_KSZ9893 : DSA_TAG_PROTO_KSZ8795;
> > -}
> > -
> >  static u32 ksz8_sw_get_phy_flags(struct dsa_switch *ds, int port)
> >  {
> >       /* Silicon Errata Sheet (DS80000830A):
> > @@ -1394,7 +1383,7 @@ static void ksz8_get_caps(struct dsa_switch
> > *ds, int port,
> >  }
> > 
> >  static const struct dsa_switch_ops ksz8_switch_ops = {
> > -     .get_tag_protocol       = ksz8_get_tag_protocol,
> > +     .get_tag_protocol       = ksz_get_tag_protocol,
> >       .get_phy_flags          = ksz8_sw_get_phy_flags,
> >       .setup                  = ksz8_setup,
> >       .phy_read               = ksz_phy_read16,
> > 
> > diff --git a/drivers/net/dsa/microchip/ksz_common.c
> > b/drivers/net/dsa/microchip/ksz_common.c
> > index 9057cdb5971c..a43b01c2e67f 100644
> > --- a/drivers/net/dsa/microchip/ksz_common.c
> > +++ b/drivers/net/dsa/microchip/ksz_common.c
> > @@ -930,6 +930,30 @@ void ksz_port_stp_state_set(struct dsa_switch
> > *ds, int port,
> >  }
>  EXPORT_SYMBOL_GPL(ksz_port_stp_state_set);
> > 
> > +enum dsa_tag_protocol ksz_get_tag_protocol(struct dsa_switch *ds,
> > +                                        int port, enum
> > dsa_tag_protocol mp)
> > +{
> > +     struct ksz_device *dev = ds->priv;
> > +     enum dsa_tag_protocol proto;
> 
> Please choose a default value in case the dev->chip_id does not take
> any
> of the branches, or at least do something to not return uninitialized
> variables from the stack.

Ok. I will update the default value. 

> > +
> > +     if (dev->chip_id == KSZ8795_CHIP_ID ||
> > +         dev->chip_id == KSZ8794_CHIP_ID ||
> > +         dev->chip_id == KSZ8765_CHIP_ID)
> > +             proto = DSA_TAG_PROTO_KSZ8795;
> > +
> > +     if (dev->chip_id == KSZ8830_CHIP_ID ||
> > +         dev->chip_id == KSZ9893_CHIP_ID)
> > +             proto = DSA_TAG_PROTO_KSZ9893;
> > +
> > +     if (dev->chip_id == KSZ9477_CHIP_ID ||
> > +         dev->chip_id == KSZ9897_CHIP_ID ||
> > +         dev->chip_id == KSZ9567_CHIP_ID)
> > +             proto = DSA_TAG_PROTO_KSZ9477;
> > +
> > +     return proto;
> > +}
> > +EXPORT_SYMBOL_GPL(ksz_get_tag_protocol);
> > +
> >  static int ksz_switch_detect(struct ksz_device *dev)
> >  {
> >       u8 id1, id2;
> > diff --git a/drivers/net/dsa/microchip/ksz_common.h
> > b/drivers/net/dsa/microchip/ksz_common.h
> > index d16c095cdefb..f253f3f22386 100644
> > --- a/drivers/net/dsa/microchip/ksz_common.h
> > +++ b/drivers/net/dsa/microchip/ksz_common.h
> > @@ -231,6 +231,8 @@ int ksz_port_mdb_del(struct dsa_switch *ds, int
> > port,
> >  int ksz_enable_port(struct dsa_switch *ds, int port, struct
> > phy_device *phy);
> >  void ksz_get_strings(struct dsa_switch *ds, int port,
> >                    u32 stringset, uint8_t *buf);
> > +enum dsa_tag_protocol ksz_get_tag_protocol(struct dsa_switch *ds,
> > +                                        int port, enum
> > dsa_tag_protocol mp);
> > 
> >  /* Common register access functions */
> > 
> > --
> > 2.36.1
> > 
> 
>