Re: [PATCH v4 5/5] net: dsa: add support for Atheros AR9331 build-in switch

[Oleksij Rempel <o.rempel@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 5375 bytes --]

Hi,

On Wed, Oct 23, 2019 at 02:58:50AM +0200, Andrew Lunn wrote:
> > --- 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,

done

> > +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?

I have only MAC TX/RX enable bit. This bit is set by phylink_mac_link_up and
removed by phylink_mac_link_down.
The port enable I use only to set predictable state of the port
register: all bits cleared. May be i should just drop port enable
function? What do you think? 

> > +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.

done

> > +}
> > +
> > +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.

ok, thx!

Regards,
Oleksij

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]