Re: [RFC Patch net-next v2 05/15] net: dsa: microchip: move the port mirror to ksz_common

[<Arun.Ramadoss@microchip.com>]

On Mon, 2022-06-13 at 12:28 +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:47PM +0530, Arun Ramadoss wrote:
> > This patch updates the common port mirror add/del dsa_switch_ops in
> > ksz_common.c. The individual switches implementation is executed
> > based
> > on the ksz_dev_ops function pointers.
> > 
> > Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
> > ---
> 
> Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
> 
> >  drivers/net/dsa/microchip/ksz8795.c    | 13 ++++++-------
> >  drivers/net/dsa/microchip/ksz9477.c    | 12 ++++++------
> >  drivers/net/dsa/microchip/ksz_common.c | 25
> > +++++++++++++++++++++++++
> >  drivers/net/dsa/microchip/ksz_common.h | 10 ++++++++++
> >  4 files changed, 47 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/microchip/ksz8795.c
> > b/drivers/net/dsa/microchip/ksz8795.c
> > index 157d69e46793..8657b520b336 100644
> > --- a/drivers/net/dsa/microchip/ksz8795.c
> > +++ b/drivers/net/dsa/microchip/ksz8795.c
> > @@ -1089,12 +1089,10 @@ static int ksz8_port_vlan_del(struct
> > ksz_device *dev, int port,
> >       return 0;
> >  }
> > 
> > diff --git a/drivers/net/dsa/microchip/ksz_common.c
> > b/drivers/net/dsa/microchip/ksz_common.c
> > index a1fef9e4e36c..1ed4cc94795e 100644
> > --- a/drivers/net/dsa/microchip/ksz_common.c
> > +++ b/drivers/net/dsa/microchip/ksz_common.c
> > @@ -994,6 +994,31 @@ int ksz_port_vlan_del(struct dsa_switch *ds,
> > int port,
> >  }
> >  EXPORT_SYMBOL_GPL(ksz_port_vlan_del);
> > 
> > +int ksz_port_mirror_add(struct dsa_switch *ds, int port,
> > +                     struct dsa_mall_mirror_tc_entry *mirror,
> > +                     bool ingress, struct netlink_ext_ack *extack)
> > +{
> > +     struct ksz_device *dev = ds->priv;
> > +     int ret = -EOPNOTSUPP;
> > +
> > +     if (dev->dev_ops->mirror_add)
> > +             ret = dev->dev_ops->mirror_add(dev, port, mirror,
> > ingress,
> > +                                            extack);
> > +
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(ksz_port_mirror_add);
> 
> Just as a minor style comment, take it or leave it.
> 
> If you switch the function pointer presence check, you reduce the
> indentation of the long statement, making it fit a single line, and
> you
> eliminate the need for a "ret" variable:
> 
>         if (!dev->dev_ops->mirror_add)
>                 return -EOPNOTSUPP;
> 
>         return dev->dev_ops->mirror_add(dev, port, mirror, ingress,
> extack);

Yes it reduces the indentation. I will update it for other checks too.

> > +
> > +void ksz_port_mirror_del(struct dsa_switch *ds, int port,
> > +                      struct dsa_mall_mirror_tc_entry *mirror)
> > +{
> > +     struct ksz_device *dev = ds->priv;
> > +
> > +     if (dev->dev_ops->mirror_del)
> > +             dev->dev_ops->mirror_del(dev, port, mirror);
> > +}
> > +EXPORT_SYMBOL_GPL(ksz_port_mirror_del);
> > +
> >  static int ksz_switch_detect(struct ksz_device *dev)
> >  {
> >       u8 id1, id2;
> > 
> > 
> > --
> > 2.36.1
> > 
> 
>