Re: [RFC 2/3] ksz: Add Microchip KSZ8873 SMI-DSA driver

[Andrew Lunn <andrew@lunn.ch>]

On Wed, May 08, 2019 at 11:13:29PM +0200, Michael Grzeschik wrote:
> Cc: Tristram.Ha@microchip.com
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/net/dsa/microchip/Kconfig       |   16 +
>  drivers/net/dsa/microchip/Makefile      |    2 +
>  drivers/net/dsa/microchip/ksz8863.c     | 1026 +++++++++++++++++++++++
>  drivers/net/dsa/microchip/ksz8863_reg.h |  605 +++++++++++++
>  drivers/net/dsa/microchip/ksz8863_smi.c |  105 +++
>  drivers/net/dsa/microchip/ksz_priv.h    |    3 +
>  include/net/dsa.h                       |    2 +
>  net/dsa/Kconfig                         |    7 +
>  net/dsa/tag_ksz.c                       |   45 +
>  9 files changed, 1811 insertions(+)
>  create mode 100644 drivers/net/dsa/microchip/ksz8863.c
>  create mode 100644 drivers/net/dsa/microchip/ksz8863_reg.h
>  create mode 100644 drivers/net/dsa/microchip/ksz8863_smi.c
> 
> diff --git a/drivers/net/dsa/microchip/Kconfig b/drivers/net/dsa/microchip/Kconfig
> index bea29fde9f3d1..a6fa6ae972951 100644
> --- a/drivers/net/dsa/microchip/Kconfig
> +++ b/drivers/net/dsa/microchip/Kconfig
> @@ -14,3 +14,19 @@ config NET_DSA_MICROCHIP_KSZ9477_SPI
>  	depends on NET_DSA_MICROCHIP_KSZ9477 && SPI
>  	help
>  	  Select to enable support for registering switches configured through SPI.
> +
> +menuconfig NET_DSA_MICROCHIP_KSZ8863
> +	tristate "Microchip KSZ8863 series switch support"
> +	depends on NET_DSA
> +	select NET_DSA_TAG_KSZ8863
> +	select NET_DSA_MICROCHIP_KSZ_COMMON
> +	help
> +	  This driver adds support for Microchip KSZ8863 switch chips.
> +
> +config NET_DSA_MICROCHIP_KSZ8863_SMI

> +	tristate "KSZ series SMI connected switch driver"
> +	depends on NET_DSA_MICROCHIP_KSZ8863
> +	default y
> +	help
> +	  Select to enable support for registering switches configured through SMI.

SMI is a synonym for MDIO. So we should make it clear, this is a
proprietary version. "... through Microchip SMI".

You might also want to either depend on or select mdio-bitbang, since
that is the other driver which supports Microchip SMI.

> +static int ksz_spi_read(struct ksz_device *dev, u32 reg, u8 *data,
> +			unsigned int len)
> +{
> +	int i = 0;
> +
> +	for (i = 0; i < len; i++)
> +		data[i] = (u8)mdiobus_read(dev->bus, 0,
> +					    (reg + i) | MII_ADDR_SMI0);

mdiobus_read() and mdiobus_write() can return an error, which is why
is returns an int. Please check for the error and return it.

> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 6aaaadd6a413c..57fbf3e722362 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -44,6 +44,7 @@ struct phylink_link_state;
>  #define DSA_TAG_PROTO_TRAILER_VALUE		11
>  #define DSA_TAG_PROTO_8021Q_VALUE		12
>  #define DSA_TAG_PROTO_SJA1105_VALUE		13
> +#define DSA_TAG_PROTO_KSZ8863_VALUE		14
>  
>  enum dsa_tag_protocol {
>  	DSA_TAG_PROTO_NONE		= DSA_TAG_PROTO_NONE_VALUE,
> @@ -60,6 +61,7 @@ enum dsa_tag_protocol {
>  	DSA_TAG_PROTO_TRAILER		= DSA_TAG_PROTO_TRAILER_VALUE,
>  	DSA_TAG_PROTO_8021Q		= DSA_TAG_PROTO_8021Q_VALUE,
>  	DSA_TAG_PROTO_SJA1105		= DSA_TAG_PROTO_SJA1105_VALUE,
> +	DSA_TAG_PROTO_KSZ8863		= DSA_TAG_PROTO_KSZ8863_VALUE,
>  };

Please put all the tag driver changes into a separate patch.

> +static struct sk_buff *ksz8863_rcv(struct sk_buff *skb, struct net_device *dev,
> +				   struct packet_type *pt)
> +{
> +	/* Tag decoding */
> +	u8 *tag = skb_tail_pointer(skb) - KSZ_EGRESS_TAG_LEN;
> +	unsigned int port = tag[0] & 1;

Does this device only have 2 ports, 0 and 1?

> +	unsigned int len = KSZ_EGRESS_TAG_LEN;
> +
> +	return ksz_common_rcv(skb, dev, port, len);
> +}

  Andrew