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:
signature.asc
Description: PGP signature
