Re: [PATCH net-next v4 6/9] net: dsa: microchip: dcb: add special handling for KSZ88X3 family

[Arun.Ramadoss]

Hi Oleksij,

> 
> diff --git a/drivers/net/dsa/microchip/ksz_dcb.c
> b/drivers/net/dsa/microchip/ksz_dcb.c
> index 46491c6dd6f60..2c28c4ed288a9 100644
> --- a/drivers/net/dsa/microchip/ksz_dcb.c
> +++ b/drivers/net/dsa/microchip/ksz_dcb.c
> @@ -83,6 +83,10 @@ static const u8 ksz_supported_apptrust[] = {
>         IEEE_8021QAZ_APP_SEL_DSCP,
>  };
> 
> +static const u8 ksz8_port2_supported_apptrust[] = {
> +       DCB_APP_SEL_PCP,
> +};
> +
>  static const char * const ksz_supported_apptrust_variants[] = {
>         "empty", "dscp", "pcp", "dscp pcp"
>  };
> @@ -128,6 +132,48 @@ int ksz_port_get_default_prio(struct dsa_switch
> *ds, int port)
>         return (data & mask) >> shift;
>  }
> 
> 
> 
> +/**
> + * ksz88x3_port0_apptrust_quirk - Quirk for apptrust configuration
> on Port 1
> + *                               (port == 0) of KSZ88x3 devices
> + * @dev: Pointer to the KSZ switch device structure
> + * @port: Port number for which to set the apptrust selectors
> + * @reg: Register address for the apptrust configuration
> + * @data: Data to set for the apptrust configuration
> + *
> + * This function implements a quirk for apptrust configuration on
> Port 1 of
> + * KSZ88x3 devices. It ensures that apptrust configuration on Port 1
> is not
> + * possible if PCP apptrust on Port 2 is disabled. This is because
> the Port 2
> + * seems to be permanently hardwired to PCP classification, so we
> need to
> + * do Port 1 configuration always in agreement with Port 2
> configuration.
> + *
> + * Return: 0 on success, or a negative error code on failure
> + */
> +static int ksz88x3_port0_apptrust_quirk(struct ksz_device *dev, int
> port,
> +                                       int reg, u8 data)
> +{
> +       u8 port1_data;

why can't we have some common reference, because it is somewhat
confusing. function name is port0, but apptrust config is for port1 and
u8 port1_data. atleast instead of port1_data, port0_data, we can have
variable name as data, since they are handled in two different
functions. 

> +       int ret;
> +
> +       if (!(data & (KSZ8_PORT_802_1P_ENABLE |
> KSZ8_PORT_DIFFSERV_ENABLE)))
> +               return 0;
> +
> +       ret = ksz_pread8(dev, 1, reg, &port1_data);

Having macros for magic number for port 1 and 0 will be good. 
> --
> 2.39.2
>