Date:   Fri, 28 Oct 2022 04:41:06 +0000
From:   <Arun.Ramadoss@...rochip.com>
To:     <andrew@...n.ch>, <camel.guo@...s.com>, <robh+dt@...nel.org>,
        <vivien.didelot@...il.com>, <olteanv@...il.com>,
        <linux@...linux.org.uk>, <f.fainelli@...il.com>, <kuba@...nel.org>,
        <pabeni@...hat.com>, <edumazet@...gle.com>,
        <krzysztof.kozlowski+dt@...aro.org>, <davem@...emloft.net>
CC:     <robh@...nel.org>, <linux-kernel@...r.kernel.org>,
        <netdev@...r.kernel.org>, <kernel@...s.com>,
        <devicetree@...r.kernel.org>
Subject: Re: [RFC net-next 2/2] net: dsa: Add driver for Maxlinear GSW1XX
 switch

Hi Camel,
On Tue, 2022-10-25 at 15:52 +0200, Camel Guo wrote:
> Add initial framework for Maxlinear's GSW1xx switch and
> currently only GSW145 in MDIO managed mode is supported.
> 
> Signed-off-by: Camel Guo <camel.guo@...s.com>
> ---
>  MAINTAINERS                   |   3 +
>  drivers/net/dsa/Kconfig       |  16 +
>  drivers/net/dsa/Makefile      |   2 +
>  drivers/net/dsa/gsw1xx.h      |  27 ++
>  drivers/net/dsa/gsw1xx_core.c | 823
> ++++++++++++++++++++++++++++++++++
>  drivers/net/dsa/gsw1xx_mdio.c | 128 ++++++
>  6 files changed, 999 insertions(+)
>  create mode 100644 drivers/net/dsa/gsw1xx.h
>  create mode 100644 drivers/net/dsa/gsw1xx_core.c
>  create mode 100644 drivers/net/dsa/gsw1xx_mdio.c
> 
> 
> diff --git a/drivers/net/dsa/gsw1xx.h b/drivers/net/dsa/gsw1xx.h
> new file mode 100644
> index 000000000000..08b2975e1267
> --- /dev/null
> +++ b/drivers/net/dsa/gsw1xx.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef GSW1XX_H
> +#define GSW1XX_H
> +
> +#include <linux/regmap.h>
> +#include <linux/device.h>

include headers in sorted manner.

> +#include <net/dsa.h>
> +
> +struct gsw1xx_hw_info {
> +	int max_ports;
> +	int cpu_port;
> +};
> +
> +struct gsw1xx_priv {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct dsa_switch *ds;
> +	const struct gsw1xx_hw_info *hw_info;
> +};
> +
> +extern const struct regmap_access_table gsw1xx_register_set;
> +
> +int gsw1xx_probe(struct gsw1xx_priv *priv, struct device *dev);
> +void gsw1xx_remove(struct gsw1xx_priv *priv);
> +void gsw1xx_shutdown(struct gsw1xx_priv *priv);
> +
> +#endif /* GSW1XX_H */
> diff --git a/drivers/net/dsa/gsw1xx_core.c
> b/drivers/net/dsa/gsw1xx_core.c
> new file mode 100644
> index 000000000000..1b3cbee4addc
> --- /dev/null
> +++ b/drivers/net/dsa/gsw1xx_core.c
> @@ -0,0 +1,823 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/delay.h>
> +#include <linux/etherdevice.h>
> +#include <linux/if_bridge.h>
> +#include <linux/if_vlan.h>
> +#include <linux/module.h>
> +#include <linux/of_mdio.h>
> +#include <linux/of_net.h>
> +#include <linux/of_platform.h>
> +#include <linux/phy.h>
> +#include <linux/phylink.h>
> +#include <linux/regmap.h>
> +#include <net/dsa.h>
> +
> +#include "gsw1xx.h"
> +
> +/* GSW1XX MDIO Registers */
> +#define GSW1XX_MDIO_BASE_ADDR		0xF400
> +#define GSW1XX_MDIO_REG_LEN		0x0422
> +#define GSW1XX_MDIO_GLOB		0x00
> +#define  GSW1XX_MDIO_GLOB_ENABLE	BIT(15)
> +#define GSW1XX_MDIO_CTRL		0x08
> +#define  GSW1XX_MDIO_CTRL_BUSY		BIT(12)
> +#define  GSW1XX_MDIO_CTRL_RD		BIT(11)
> +#define  GSW1XX_MDIO_CTRL_WR		BIT(10)
> +#define  GSW1XX_MDIO_CTRL_PHYAD_MASK	0x1F
> +#define  GSW1XX_MDIO_CTRL_PHYAD_SHIFT	5
> +#define  GSW1XX_MDIO_CTRL_REGAD_MASK	0x1F
> +#define GSW1XX_MDIO_READ		0x09
> +#define GSW1XX_MDIO_WRITE		0x0A
> +#define GSW1XX_MDIO_MDC_CFG0		0x0B
> +#define GSW1XX_MDIO_PHYp(p)		(0x15 - (p))

All Capital letters in Macro will be good.

> +#define  GSW1XX_MDIO_PHY_LINK_MASK	0x6000
> +#define  GSW1XX_MDIO_PHY_LINK_AUTO	0x0000
> +#define  GSW1XX_MDIO_PHY_LINK_DOWN	0x4000
> +#define  GSW1XX_MDIO_PHY_LINK_UP	0x2000
> +#define  GSW1XX_MDIO_PHY_SPEED_MASK	0x1800
> +#define  GSW1XX_MDIO_PHY_SPEED_AUTO	0x1800
> +#define  GSW1XX_MDIO_PHY_SPEED_M10	0x0000
> +#define  GSW1XX_MDIO_PHY_SPEED_M100	0x0800
> +#define  GSW1XX_MDIO_PHY_SPEED_G1	0x1000
> +#define  GSW1XX_MDIO_PHY_FDUP_MASK	0x0600
> +#define  GSW1XX_MDIO_PHY_FDUP_AUTO	0x0000
> +#define  GSW1XX_MDIO_PHY_FDUP_EN	0x0200
> +#define  GSW1XX_MDIO_PHY_FDUP_DIS	0x0600
> +#define  GSW1XX_MDIO_PHY_FCONTX_MASK	0x0180
> +#define  GSW1XX_MDIO_PHY_FCONTX_AUTO	0x0000
> +#define  GSW1XX_MDIO_PHY_FCONTX_EN	0x0100
> +#define  GSW1XX_MDIO_PHY_FCONTX_DIS	0x0180
> +#define  GSW1XX_MDIO_PHY_FCONRX_MASK	0x0060
> +#define  GSW1XX_MDIO_PHY_FCONRX_AUTO	0x0000
> +#define  GSW1XX_MDIO_PHY_FCONRX_EN	0x0020
> +#define  GSW1XX_MDIO_PHY_FCONRX_DIS	0x0060
> +#define  GSW1XX_MDIO_PHY_ADDR_MASK	0x001F
> +#define  GSW1XX_MDIO_PHY_MASK		(GSW1XX_MDIO_PHY_ADDR_M
> ASK | \
> +					 GSW1XX_MDIO_PHY_FCONRX_MASK |
> \
> +					 GSW1XX_MDIO_PHY_FCONTX_MASK |
> \
> +					 GSW1XX_MDIO_PHY_LINK_MASK | \
> +					 GSW1XX_MDIO_PHY_SPEED_MASK | \
> +					 GSW1XX_MDIO_PHY_FDUP_MASK)
> +
> 
> +struct gsw1xx_rmon_cnt_desc {
> +	unsigned int size;
> +	unsigned int offset;
> +	const char *name;
> +};
> +
> +#define MIB_DESC(_size, _offset,
> _name)                                        \
> +	{                                                              
>         \
> +		.size = _size, .offset = _offset, .name =
> _name                \
> +	}
> +
> +static const struct gsw1xx_rmon_cnt_desc gsw1xx_rmon_cnt[] = {
> +	/** Receive Packet Count (only packets that are accepted and
> not discarded). */
> +	MIB_DESC(1, 0x1F, "RxGoodPkts"),
> +	MIB_DESC(1, 0x23, "RxUnicastPkts"),
> +	MIB_DESC(1, 0x22, "RxMulticastPkts"),
> +	MIB_DESC(1, 0x21, "RxFCSErrorPkts"),
> +	MIB_DESC(1, 0x1D, "RxUnderSizeGoodPkts"),
> +	MIB_DESC(1, 0x1E, "RxUnderSizeErrorPkts"),
> +	MIB_DESC(1, 0x1B, "RxOversizeGoodPkts"),
> +	MIB_DESC(1, 0x1C, "RxOversizeErrorPkts"),
> +	MIB_DESC(1, 0x20, "RxGoodPausePkts"),
> +	MIB_DESC(1, 0x1A, "RxAlignErrorPkts"),
> +	MIB_DESC(1, 0x12, "Rx64BytePkts"),
> +	MIB_DESC(1, 0x13, "Rx127BytePkts"),
> +	MIB_DESC(1, 0x14, "Rx255BytePkts"),
> +	MIB_DESC(1, 0x15, "Rx511BytePkts"),
> +	MIB_DESC(1, 0x16, "Rx1023BytePkts"),
> +	/** Receive Size 1024-1522 (or more, if configured) Packet
> Count. */
> +	MIB_DESC(1, 0x17, "RxMaxBytePkts"),
> +	MIB_DESC(1, 0x18, "RxDroppedPkts"),
> +	MIB_DESC(1, 0x19, "RxFilteredPkts"),
> +	MIB_DESC(2, 0x24, "RxGoodBytes"),
> +	MIB_DESC(2, 0x26, "RxBadBytes"),
> +	MIB_DESC(1, 0x11, "TxAcmDroppedPkts"),
> +	MIB_DESC(1, 0x0C, "TxGoodPkts"),
> +	MIB_DESC(1, 0x06, "TxUnicastPkts"),
> +	MIB_DESC(1, 0x07, "TxMulticastPkts"),
> +	MIB_DESC(1, 0x00, "Tx64BytePkts"),
> +	MIB_DESC(1, 0x01, "Tx127BytePkts"),
> +	MIB_DESC(1, 0x02, "Tx255BytePkts"),
> +	MIB_DESC(1, 0x03, "Tx511BytePkts"),
> +	MIB_DESC(1, 0x04, "Tx1023BytePkts"),
> +	/** Transmit Size 1024-1522 (or more, if configured) Packet
> Count. */
> +	MIB_DESC(1, 0x05, "TxMaxBytePkts"),
> +	MIB_DESC(1, 0x08, "TxSingleCollCount"),
> +	MIB_DESC(1, 0x09, "TxMultCollCount"),
> +	MIB_DESC(1, 0x0A, "TxLateCollCount"),
> +	MIB_DESC(1, 0x0B, "TxExcessCollCount"),
> +	MIB_DESC(1, 0x0D, "TxPauseCount"),
> +	MIB_DESC(1, 0x10, "TxDroppedPkts"),
> +	MIB_DESC(2, 0x0E, "TxGoodBytes"),
> +};
> +
> +static u32 gsw1xx_switch_r(struct gsw1xx_priv *priv, u32 offset)
> +{
> +	int ret = 0;

In the below apis you have used *err* variable for storing the return
values. Here *ret* is used. It will be good to have either ret or err
in all the apis.
And Do we need to explicitly assign ret  to zero. since it is assigned
return value of regmap_read in the next statement itself.

> +	u32 val = 0;
> +
> +	ret = regmap_read(priv->regmap, GSW1XX_IP_BASE_ADDR + offset,
> &val);
> +
> +	return ret < 0 ? (u32)ret : val;
> +}
> +
> 
> +
> 
> +static u32 gsw1xx_switch_r_timeout(struct gsw1xx_priv *priv, u32
> offset,
> +				   u32 cleared)
> +{
> +	u32 val;
> +
> +	return read_poll_timeout(gsw1xx_switch_r, val, (val & cleared)
> == 0, 20,
> +				 50000, true, priv, offset);
> +}
> +
> +static int gsw1xx_mdio_poll(struct gsw1xx_priv *priv)
> +{
> +	int cnt = 100;

Is it possible to use u8 instead of int since the value is less than
255.

> +
> +	while (likely(cnt--)) {
> +		u32 ctrl = gsw1xx_mdio_r(priv, GSW1XX_MDIO_CTRL);
> +
> +		if ((ctrl & GSW1XX_MDIO_CTRL_BUSY) == 0)
> +			return 0;
> +		usleep_range(20, 40);
> +	}
> +
> +	return -ETIMEDOUT;
> +}
> +
> 
> 
> +static int gsw1xx_mdio(struct gsw1xx_priv *priv, struct device_node
> *mdio_np)
> +{
> +	struct dsa_switch *ds = priv->ds;
> +	int err;
> +
> +	ds->slave_mii_bus = mdiobus_alloc();
> +	if (!ds->slave_mii_bus)
> +		return -ENOMEM;

Is it possible to use devm_mdiobus_alloc api for memory allocation
which doesn't require mdiobus_free.

> +
> +	ds->slave_mii_bus->priv = priv;
> +	ds->slave_mii_bus->read = gsw1xx_mdio_rd;
> +	ds->slave_mii_bus->write = gsw1xx_mdio_wr;
> +	ds->slave_mii_bus->name = "mxl,gsw1xx-mdio";
> +	snprintf(ds->slave_mii_bus->id, MII_BUS_ID_SIZE, "%s-mii",
> +		 dev_name(priv->dev));

If you have struct mii_bus *bus and assign ds->slave_mii_bus = bus,
then all the above statement can fit in single line.
snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(priv->dev));

> +	ds->slave_mii_bus->parent = priv->dev;
> +	ds->slave_mii_bus->phy_mask = ~ds->phys_mii_mask;
> +
> +	err = of_mdiobus_register(ds->slave_mii_bus, mdio_np);

Simillarly here devm_of_mdiobus_register

> +	if (err)
> +		mdiobus_free(ds->slave_mii_bus);
> +
> +	return err;
> +}
> +
> +static int gsw1xx_setup(struct dsa_switch *ds)
> +{
> +	struct gsw1xx_priv *priv = ds->priv;
> +	unsigned int cpu_port = priv->hw_info->cpu_port;

Is it possible to use u8 instead of unsinged int.

> +	int i;
> +	int err;
> +
> +	gsw1xx_switch_w(priv, GSW1XX_IP_SWRES_R0, GSW1XX_IP_SWRES);
> +	usleep_range(5000, 10000);
> +	gsw1xx_switch_w(priv, 0, GSW1XX_IP_SWRES);
> +
> +	/* disable port fetch/store dma on all ports */
> +	for (i = 0; i < priv->hw_info->max_ports; i++)
> +		gsw1xx_port_disable(ds, i);
> +
> +	/* enable Switch */
> +	gsw1xx_mdio_mask(priv, 0, GSW1XX_MDIO_GLOB_ENABLE,
> GSW1XX_MDIO_GLOB);
> +
> +	gsw1xx_switch_w(priv, 0x7F, GSW1XX_IP_PCE_PMAP2);
> +	gsw1xx_switch_w(priv, 0x7F, GSW1XX_IP_PCE_PMAP3);
> +
> +	/* Deactivate MDIO PHY auto polling since it affects mmd
> read/write.
> +	 */
> +	gsw1xx_mdio_w(priv, 0x0, GSW1XX_MDIO_MDC_CFG0);
> +
> +	gsw1xx_switch_mask(priv, 1, GSW1XX_IP_MAC_CTRL_2_MLEN,
> +			   GSW1XX_IP_MAC_CTRL_2p(cpu_port));
> +	gsw1xx_switch_mask(priv, 0, GSW1XX_IP_BM_QUEUE_GCTRL_GL_MOD,
> +			   GSW1XX_IP_BM_QUEUE_GCTRL);
> +
> +	/* Flush MAC Table */
> +	gsw1xx_switch_mask(priv, 0, GSW1XX_IP_PCE_GCTRL_0_MTFL,
> +			   GSW1XX_IP_PCE_GCTRL_0);
> +	err = gsw1xx_switch_r_timeout(priv, GSW1XX_IP_PCE_GCTRL_0,
> +				      GSW1XX_IP_PCE_GCTRL_0_MTFL);
> +	if (err) {
> +		dev_err(priv->dev, "MAC flushing didn't finish\n");
> +		return err;
> +	}
> +
> +	gsw1xx_port_enable(ds, cpu_port, NULL);
> +
> +	return 0;
> +}
> +
> +static void gsw1xx_port_set_link(struct gsw1xx_priv *priv, int port,
> bool link)
> +{
> +	u32 mdio_phy;
> +
> +	if (link)
> +		mdio_phy = GSW1XX_MDIO_PHY_LINK_UP;
> +	else
> +		mdio_phy = GSW1XX_MDIO_PHY_LINK_DOWN;
> +
> +	gsw1xx_mdio_mask(priv, GSW1XX_MDIO_PHY_LINK_MASK, mdio_phy,
> +			 GSW1XX_MDIO_PHYp(port));
> +}
> +
> +static void gsw1xx_port_set_speed(struct gsw1xx_priv *priv, int
> port, int speed,
> +				  phy_interface_t interface)
> +{
> +	u32 mdio_phy = 0, mac_ctrl_0 = 0;

Initialize variable in different line to uniformity in the file.

> +
> +	switch (speed) {
> +	case SPEED_10:
> +		mdio_phy = GSW1XX_MDIO_PHY_SPEED_M10;
> +		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_GMII_MII;
> +		break;
> +
> +	case SPEED_100:
> +		mdio_phy = GSW1XX_MDIO_PHY_SPEED_M100;
> +		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_GMII_MII;
> +		break;
> +
> +	case SPEED_1000:
> +		mdio_phy = GSW1XX_MDIO_PHY_SPEED_G1;
> +		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_GMII_RGMII;
> +		break;
> +	}
> +
> +	gsw1xx_mdio_mask(priv, GSW1XX_MDIO_PHY_SPEED_MASK, mdio_phy,
> +			 GSW1XX_MDIO_PHYp(port));
> +	gsw1xx_switch_mask(priv, GSW1XX_IP_MAC_CTRL_0_GMII_MASK,
> mac_ctrl_0,
> +			   GSW1XX_IP_MAC_CTRL_0p(port));
> +}
> +
> +static void gsw1xx_port_set_duplex(struct gsw1xx_priv *priv, int
> port,
> +				   int duplex)
> +{
> +	u32 mac_ctrl_0, mdio_phy;

Simillarly here. In the above function, you have initialized the values
to zero. But not here.

> +
> +	if (duplex == DUPLEX_FULL) {
> +		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_FDUP_EN;
> +		mdio_phy = GSW1XX_MDIO_PHY_FDUP_EN;
> +	} else {
> +		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_FDUP_DIS;
> +		mdio_phy = GSW1XX_MDIO_PHY_FDUP_DIS;
> +	}
> +
> +	gsw1xx_switch_mask(priv, GSW1XX_IP_MAC_CTRL_0_FDUP_MASK,
> mac_ctrl_0,
> +			   GSW1XX_IP_MAC_CTRL_0p(port));
> +	gsw1xx_mdio_mask(priv, GSW1XX_MDIO_PHY_FDUP_MASK, mdio_phy,
> +			 GSW1XX_MDIO_PHYp(port));
> +}
> +
> +static void gsw1xx_port_set_pause(struct gsw1xx_priv *priv, int
> port,
> +				  bool tx_pause, bool rx_pause)
> +{
> +	u32 mac_ctrl_0, mdio_phy;

Simillarly here.

> +
> +	if (tx_pause && rx_pause) {
> +		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_FCON_RXTX;
> +		mdio_phy =
> +			GSW1XX_MDIO_PHY_FCONTX_EN |
> GSW1XX_MDIO_PHY_FCONRX_EN;
> +	} else if (tx_pause) {
> +		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_FCON_TX;
> +		mdio_phy =
> +			GSW1XX_MDIO_PHY_FCONTX_EN |
> GSW1XX_MDIO_PHY_FCONRX_DIS;
> +	} else if (rx_pause) {
> +		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_FCON_RX;
> +		mdio_phy =
> +			GSW1XX_MDIO_PHY_FCONTX_DIS |
> GSW1XX_MDIO_PHY_FCONRX_EN;
> +	} else {
> +		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_FCON_NONE;
> +		mdio_phy =
> +			GSW1XX_MDIO_PHY_FCONTX_DIS |
> GSW1XX_MDIO_PHY_FCONRX_DIS;
> +	}
> +
> +	gsw1xx_switch_mask(priv, GSW1XX_IP_MAC_CTRL_0_FCON_MASK,
> mac_ctrl_0,
> +			   GSW1XX_IP_MAC_CTRL_0p(port));
> +	gsw1xx_mdio_mask(priv,
> +			 GSW1XX_MDIO_PHY_FCONTX_MASK |
> GSW1XX_MDIO_PHY_FCONRX_MASK,
> +			 mdio_phy, GSW1XX_MDIO_PHYp(port));
> +}
> +
> 
> 
> 
> +
> +static void gsw1xx_get_ethtool_stats(struct dsa_switch *ds, int
> port,
> +				     uint64_t *data)
> +{
> +	struct gsw1xx_priv *priv = ds->priv;
> +	const struct gsw1xx_rmon_cnt_desc *rmon_cnt;
> +	int i;
> +	u64 high;

Reverse christmas tree

> +
> +	for (i = 0; i < ARRAY_SIZE(gsw1xx_rmon_cnt); i++) {
> +		rmon_cnt = &gsw1xx_rmon_cnt[i];
> +
> +		data[i] =
> +			gsw1xx_bcm_ram_entry_read(priv, port, rmon_cnt-
> >offset);
> +		if (rmon_cnt->size == 2) {
> +			high = gsw1xx_bcm_ram_entry_read(priv, port,
> +							 rmon_cnt-
> >offset + 1);
> +			data[i] |= high << 32;
> +		}
> +	}
> +}
> +
> 
> +static int gsw1xx_get_mac_eee(struct dsa_switch *ds, int port,
> +			      struct ethtool_eee *e)
> +{
> +	struct gsw1xx_priv *priv = (struct gsw1xx_priv *)ds->priv;
> +	u32 val = 0;

Val initialized to zero.

> +
> +	val = gsw1xx_switch_r(priv, GSW1XX_IP_MAC_CTRL_4p(port));
> +	e->tx_lpi_enabled = !!(val & GSW1XX_IP_MAC_CTRL_4_LPIEN);
> +
> +	e->tx_lpi_timer = 20;
> +
> +	return 0;
> +}
> +
> 
> 
> +int gsw1xx_probe(struct gsw1xx_priv *priv, struct device *dev)
> +{
> +	struct device_node *np, *mdio_np;
> +	int err;
> +	u32 version;
> +
> +	if (!priv->regmap || IS_ERR(priv->regmap))
> +		return -EINVAL;
> +
> +	priv->hw_info = of_device_get_match_data(dev);
> +	if (!priv->hw_info)
> +		return -EINVAL;
> +
> +	priv->ds = devm_kzalloc(dev, sizeof(*priv->ds), GFP_KERNEL);
> +	if (!priv->ds)
> +		return -ENOMEM;
> +
> +	priv->ds->dev = dev;
> +	priv->ds->num_ports = priv->hw_info->max_ports;
> +	priv->ds->priv = priv;
> +	priv->ds->ops = &gsw1xx_switch_ops;
> +	priv->dev = dev;
> +	version = gsw1xx_switch_r(priv, GSW1XX_IP_VERSION);
> +
> +	np = dev->of_node;
> +	switch (version) {
> +	case GSW1XX_IP_VERSION_2_3:
> +		if (!of_device_is_compatible(np, "mxl,gsw145-mdio"))
> +			return -EINVAL;
> +		break;
> +	default:
> +		dev_err(dev, "unknown GSW1XX_IP version: 0x%x",
> version);
> +		return -ENOENT;
> +	}
> +
> +	/* bring up the mdio bus */
> +	mdio_np = of_get_child_by_name(np, "mdio");
> +	if (!mdio_np) {
> +		dev_err(dev, "missing child mdio node\n");
> +		return -EINVAL;
> +	}
> +
> +	err = gsw1xx_mdio(priv, mdio_np);
> +	if (err) {
> +		dev_err(dev, "mdio probe failed\n");
> +		goto put_mdio_node;
> +	}
> +
> +	err = dsa_register_switch(priv->ds);
> +	if (err) {
> +		dev_err(dev, "dsa switch register failed: %i\n", err);
> +		goto mdio_bus;

To have readability, can be renamed to mdio_bus_unregister.

> +	}
> +	if (!dsa_is_cpu_port(priv->ds, priv->hw_info->cpu_port)) {
> +		dev_err(dev,
> +			"wrong CPU port defined, HW only supports port:
> %i",
> +			priv->hw_info->cpu_port);
> +		err = -EINVAL;
> +		goto disable_switch;
> +	}
> +
> +	dev_set_drvdata(dev, priv);
> +
> +	return 0;
> +
> +disable_switch:
> +	gsw1xx_mdio_mask(priv, GSW1XX_MDIO_GLOB_ENABLE, 0,
> GSW1XX_MDIO_GLOB);
> +	dsa_unregister_switch(priv->ds);
> +mdio_bus:
> +	if (mdio_np) {
> +		mdiobus_unregister(priv->ds->slave_mii_bus);
> +		mdiobus_free(priv->ds->slave_mii_bus);
> +	}
> +put_mdio_node:
> +	of_node_put(mdio_np);
> +	return err;
> +}
> +EXPORT_SYMBOL(gsw1xx_probe);
> +
> 
> 
> 
> diff --git a/drivers/net/dsa/gsw1xx_mdio.c
> b/drivers/net/dsa/gsw1xx_mdio.c
> new file mode 100644
> index 000000000000..8328001041ed
> --- /dev/null
> +++ b/drivers/net/dsa/gsw1xx_mdio.c
> @@ -0,0 +1,128 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * MaxLinear switch driver for GSW1XX in MDIO managed mode
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mdio.h>
> +#include <linux/phy.h>
> +#include <linux/of.h>
                                                          
Header file in sorted order.

> +
> +#include "gsw1xx.h"
> +
> +#define GSW1XX_SMDIO_TARGET_BASE_ADDR_REG	0x1F
> +
> 

Powered by blists - more mailing lists