Re: [PATCH net-next v2 1/2] net: dsa: mv88e6xxx: Split monitor port configuration

[Andrew Lunn]

On Mon, Oct 21, 2019 at 11:01:42PM +0200, Iwan R Timmer wrote:
> Separate the configuration of the egress and ingress monitor port.
> This allows the port mirror functionality to do ingress and egress
> port mirroring to separate ports.
> 
> Signed-off-by: Iwan R Timmer <irtim...@gmail.com>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c    |  9 ++++++++-
>  drivers/net/dsa/mv88e6xxx/chip.h    |  3 ++-
>  drivers/net/dsa/mv88e6xxx/global1.c | 23 +++++++++++------------
>  drivers/net/dsa/mv88e6xxx/global1.h |  6 ++++--
>  4 files changed, 25 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c 
> b/drivers/net/dsa/mv88e6xxx/chip.c
> index 6787d560e9e3..e9735346838d 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -2378,7 +2378,14 @@ static int mv88e6xxx_setup_upstream_port(struct 
> mv88e6xxx_chip *chip, int port)
>  
>               if (chip->info->ops->set_egress_port) {
>                       err = chip->info->ops->set_egress_port(chip,
> -                                                            upstream_port);
> +                                                            true,
> +                                                            upstream_port);
> +                     if (err)
> +                             return err;
> +
> +                     err = chip->info->ops->set_egress_port(chip,
> +                                                            false,
> +                                                            upstream_port);

Hi Iwam

I never find true/false very simple to understand. Please could you
add an enum. We already have

enum mv88e6xxx_egress_mode {
        MV88E6XXX_EGRESS_MODE_UNMODIFIED,
        MV88E6XXX_EGRESS_MODE_UNTAGGED,
        MV88E6XXX_EGRESS_MODE_TAGGED,
        MV88E6XXX_EGRESS_MODE_ETHERTYPE,
};

so maybe

enum mv88e6xxx_egress_direction {
        MV88E6XXX_EGRESS_DIR_INGRESS,
        MV88E6XXX_EGRESS_DIR_EGRESS,
};

Otherwise the spirit of the patch is O.K.

          Andrew