Date:   Wed, 23 Oct 2019 02:58:50 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Oleksij Rempel <o.rempel@...gutronix.de>
Cc:     Chris Snook <chris.snook@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        James Hogan <jhogan@...nel.org>,
        Jay Cliburn <jcliburn@...il.com>,
        Mark Rutland <mark.rutland@....com>,
        Paul Burton <paul.burton@...s.com>,
        Ralf Baechle <ralf@...ux-mips.org>,
        Rob Herring <robh+dt@...nel.org>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        linux-mips@...r.kernel.org, Russell King <linux@...linux.org.uk>
Subject: Re: [PATCH v4 5/5] net: dsa: add support for Atheros AR9331 build-in
 switch

> --- a/drivers/net/dsa/Kconfig
> +++ b/drivers/net/dsa/Kconfig
> @@ -52,6 +52,8 @@ source "drivers/net/dsa/microchip/Kconfig"
>  
>  source "drivers/net/dsa/mv88e6xxx/Kconfig"
>  
> +source "drivers/net/dsa/qca/Kconfig"
> +
>  source "drivers/net/dsa/sja1105/Kconfig"
>  
>  config NET_DSA_QCA8K

> diff --git a/drivers/net/dsa/qca/Kconfig b/drivers/net/dsa/qca/Kconfig
> new file mode 100644
> index 000000000000..7e4978f46642
> --- /dev/null
> +++ b/drivers/net/dsa/qca/Kconfig
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config NET_DSA_AR9331
> +	tristate "Atheros AR9331 Ethernet switch support"

This is where things are a little bit unobvious. If you do
make menu

and go into the DSA menu, you will find the drivers are all sorted
into Alphabetic order, based on the tristate text. But you have
inserted your "Atheros AR9331", after "NXP SJA1105".

It would probably be best if you make the tristate "Qualcomm Atheros
AR9331 ...". The order would be correct then,

> +static int ar9331_sw_port_enable(struct dsa_switch *ds, int port,
> +				 struct phy_device *phy)
> +{
> +	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
> +	struct regmap *regmap = priv->regmap;
> +	int ret;
> +
> +	/* nothing to enable. Just set link to initial state */
> +	ret = regmap_write(regmap, AR9331_SW_REG_PORT_STATUS(port), 0);
> +	if (ret)
> +		dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
> +
> +	return ret;
> +}
> +
> +static void ar9331_sw_port_disable(struct dsa_switch *ds, int port)
> +{
> +	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
> +	struct regmap *regmap = priv->regmap;
> +	int ret;
> +
> +	ret = regmap_write(regmap, AR9331_SW_REG_PORT_STATUS(port), 0);
> +	if (ret)
> +		dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
> +}

I've asked this before, but i don't remember the answer. Why are
port_enable and port_disable the same?

> +static int ar9331_sw_irq_init(struct ar9331_sw_priv *priv)
> +{
> +	struct device_node *np = priv->dev->of_node;
> +	struct device *dev = priv->dev;
> +	int ret, irq;
> +
> +	irq = of_irq_get(np, 0);
> +	if (irq <= 0) {
> +		dev_err(dev, "failed to get parent IRQ\n");
> +		return irq ? irq : -EINVAL;
> +	}
> +
> +	ret = devm_request_threaded_irq(dev, irq, NULL, ar9331_sw_irq,
> +					IRQF_ONESHOT, AR9331_SW_NAME, priv);
> +	if (ret) {
> +		dev_err(dev, "unable to request irq: %d\n", ret);
> +		return ret;
> +	}
> +
> +	priv->irqdomain = irq_domain_add_linear(np, 1, &ar9331_sw_irqdomain_ops,
> +						priv);
> +	if (!priv->irqdomain) {
> +		dev_err(dev, "failed to create IRQ domain\n");
> +		return -EINVAL;
> +	}
> +
> +	irq_set_parent(irq_create_mapping(priv->irqdomain, 0), irq);
> +
> +	return 0;
> +}


> +static int ar9331_sw_probe(struct mdio_device *mdiodev)
> +{
> +	struct ar9331_sw_priv *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(&mdiodev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->regmap = devm_regmap_init(&mdiodev->dev, &ar9331_sw_bus, priv,
> +					&ar9331_mdio_regmap_config);
> +	if (IS_ERR(priv->regmap)) {
> +		ret = PTR_ERR(priv->regmap);
> +		dev_err(&mdiodev->dev, "regmap init failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	priv->sw_reset = devm_reset_control_get(&mdiodev->dev, "switch");
> +	if (IS_ERR(priv->sw_reset)) {
> +		dev_err(&mdiodev->dev, "missing switch reset\n");
> +		return PTR_ERR(priv->sw_reset);
> +	}
> +
> +	priv->sbus = mdiodev->bus;
> +	priv->dev = &mdiodev->dev;
> +
> +	ret = ar9331_sw_irq_init(priv);
> +	if (ret)
> +		return ret;
> +
> +	priv->ds = dsa_switch_alloc(&mdiodev->dev, AR9331_SW_PORTS);
> +	if (!priv->ds)
> +		return -ENOMEM;
> +
> +	priv->ds->priv = priv;
> +	priv->ops = ar9331_sw_ops;
> +	priv->ds->ops = &priv->ops;
> +	dev_set_drvdata(&mdiodev->dev, priv);
> +
> +	return dsa_register_switch(priv->ds);

If there is an error here, you need to undo the IRQ code, etc.

> +}
> +
> +static void ar9331_sw_remove(struct mdio_device *mdiodev)
> +{
> +	struct ar9331_sw_priv *priv = dev_get_drvdata(&mdiodev->dev);
> +
> +	mdiobus_unregister(priv->mbus);
> +	dsa_unregister_switch(priv->ds);
> +
> +	reset_control_assert(priv->sw_reset);

You also need to clean up the IRQ code here.

Thanks
    Andrew

Powered by blists - more mailing lists