Re: [RFC patch net-next 3/3] net: dsa: ksz: moved ksz9477 port mirror to ksz_common.c

[Arun.Ramadoss]

Hi Vladimir,
Thanks for the feedback.

On Wed, 2022-04-27 at 19:57 +0300, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Wed, Apr 27, 2022 at 09:53:43PM +0530, Arun Ramadoss wrote:
> > Moved the port_mirror_add and port_mirror_del function from ksz9477
> > to
> 
> Present tense (move)
> 
> > ksz_common, to make it generic function which can be used by
> > KSZ9477
> > based switch.
> 
> Presumably you mean "which can be used by other switches" (it can
> already be used by ksz9477, so that can't be the argument for moving
> it)
I will update the commit description.
> 
> > 
> > Signed-off-by: Arun Ramadoss <arun.ramadoss@xxxxxxxxxxxxx>
> > ---
> 
> Looks good, except for the spelling mistakes in the code that is
> being
> moved (introduced in patch 1), which I expect you will update in the
> new
> code as well.
Yes, I will update. 
> 
> Reviewed-by: Vladimir Oltean <olteanv@xxxxxxxxx>
> 
> > diff --git a/drivers/net/dsa/microchip/ksz_reg.h
> > b/drivers/net/dsa/microchip/ksz_reg.h
> > new file mode 100644
> > index 000000000000..ccd4a6568e34
> > --- /dev/null
> > +++ b/drivers/net/dsa/microchip/ksz_reg.h
> > @@ -0,0 +1,29 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Microchip KSZ Switch register definitions
> > + *
> > + * Copyright (C) 2017-2022 Microchip Technology Inc.
> > + */
> > +
> > +#ifndef __KSZ_REGS_H
> > +#define __KSZ_REGS_H
> > +
> > +#define REG_SW_MRI_CTRL_0            0x0370
> > +
> > +#define SW_IGMP_SNOOP                        BIT(6)
> > +#define SW_IPV6_MLD_OPTION           BIT(3)
> > +#define SW_IPV6_MLD_SNOOP            BIT(2)
> > +#define SW_MIRROR_RX_TX                      BIT(0)
> > +
> > +/* 8 - Classification and Policing */
> > +#define REG_PORT_MRI_MIRROR_CTRL     0x0800
> > +
> > +#define PORT_MIRROR_RX                       BIT(6)
> > +#define PORT_MIRROR_TX                       BIT(5)
> > +#define PORT_MIRROR_SNIFFER          BIT(1)
> > +
> > +#define
> > P_MIRROR_CTRL                        REG_PORT_MRI_MIRROR_CTRL
> > +
> > +#define S_MIRROR_CTRL                        REG_SW_MRI_CTRL_0
> 
> Small comment: if P_MIRROR_CTRL and S_MIRROR_CTRL are expected to be
> at
> the same register offset for all switch families, why is there a
> macro
> behind a macro for their addresses?

ksz8795 and ksz9477 have different address/register for the
Mirror_ctrl. To make it common for the both, P_MIRROR_CTRL is defined
in ksz8795_reg.h and ksz9477_reg.h file.
I just carried forward to ksz_reg.h.

> 
> > +
> > +#endif
> > --
> > 2.33.0
> >