diff mbox

[4/4] asix: Add a new driver for the AX88172A

Message ID 1341574388-7464-5-git-send-email-christian.riesch@omicron.at
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Christian Riesch July 6, 2012, 11:33 a.m. UTC
The Asix AX88172A is a USB 2.0 Ethernet interface that supports both an
internal PHY as well as an external PHY (connected via MII).

This patch adds a driver for the AX88172A and provides support for
both modes and supports phylib.

Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
---
 drivers/net/usb/Makefile       |    2 +-
 drivers/net/usb/asix_devices.c |    6 +
 drivers/net/usb/ax88172a.c     |  407 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 414 insertions(+), 1 deletions(-)
 create mode 100644 drivers/net/usb/ax88172a.c

Comments

Ben Hutchings July 6, 2012, 5:37 p.m. UTC | #1
On Fri, 2012-07-06 at 13:33 +0200, Christian Riesch wrote:
> The Asix AX88172A is a USB 2.0 Ethernet interface that supports both an
> internal PHY as well as an external PHY (connected via MII).
> 
> This patch adds a driver for the AX88172A and provides support for
> both modes and supports phylib.
[...]
> +static int ax88172a_init_mdio(struct usbnet *dev)
> +{
> +	struct ax88172a_private *priv =
> +		(struct ax88172a_private *)dev->driver_priv;
> +	int ret, i;
> +
> +	priv->mdio = mdiobus_alloc();
> +	if (!priv->mdio) {
> +		dbg("Could not allocate MDIO bus");
> +		return -1;
> +	}
> +
> +	priv->mdio->priv = (void *)dev;
> +	priv->mdio->read = &asix_mdio_bus_read;
> +	priv->mdio->write = &asix_mdio_bus_write;
> +	priv->mdio->name = "Asix MDIO Bus";
> +	snprintf(priv->mdio->id, MII_BUS_ID_SIZE, "asix-%s",
> +		 dev_name(dev->net->dev.parent));
[...]

I think you need to ensure that the bus identifier is unique throughout
its lifetime, but net devices can be renamed and that could lead to a
collision.  Perhaps you could use the ifindex or the USB device path
(though that might be too long).

Ben.
Grant Grundler July 6, 2012, 9:20 p.m. UTC | #2
On Fri, Jul 6, 2012 at 4:33 AM, Christian Riesch
<christian.riesch@omicron.at> wrote:
> The Asix AX88172A is a USB 2.0 Ethernet interface that supports both an
> internal PHY as well as an external PHY (connected via MII).
>
> This patch adds a driver for the AX88172A and provides support for
> both modes and supports phylib.

Christian,
In general this looks fine to me...but I wouldn't know about "bus
identifier life times" (Ben Hutchings comment).

My nit pick is the declaration and of use_embdphy. An alternative
coding _suggestion_ below.  I'm not substantially altering the
functionality.

thanks,
grant

>
> Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
> ---
>  drivers/net/usb/Makefile       |    2 +-
>  drivers/net/usb/asix_devices.c |    6 +
>  drivers/net/usb/ax88172a.c     |  407 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 414 insertions(+), 1 deletions(-)
>  create mode 100644 drivers/net/usb/ax88172a.c
>
> diff --git a/drivers/net/usb/Makefile b/drivers/net/usb/Makefile
> index a9490d9..bf06300 100644
> --- a/drivers/net/usb/Makefile
> +++ b/drivers/net/usb/Makefile
> @@ -8,7 +8,7 @@ obj-$(CONFIG_USB_PEGASUS)       += pegasus.o
>  obj-$(CONFIG_USB_RTL8150)      += rtl8150.o
>  obj-$(CONFIG_USB_HSO)          += hso.o
>  obj-$(CONFIG_USB_NET_AX8817X)  += asix.o
> -asix-y := asix_devices.o asix_common.o
> +asix-y := asix_devices.o asix_common.o ax88172a.o
>  obj-$(CONFIG_USB_NET_CDCETHER) += cdc_ether.o
>  obj-$(CONFIG_USB_NET_CDC_EEM)  += cdc_eem.o
>  obj-$(CONFIG_USB_NET_DM9601)   += dm9601.o
> diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
> index c8682a5..02b8c21 100644
> --- a/drivers/net/usb/asix_devices.c
> +++ b/drivers/net/usb/asix_devices.c
> @@ -877,6 +877,8 @@ static const struct driver_info ax88178_info = {
>         .tx_fixup = asix_tx_fixup,
>  };
>
> +extern const struct driver_info ax88172a_info;
> +
>  static const struct usb_device_id      products[] = {
>  {
>         /* Linksys USB200M */
> @@ -1002,6 +1004,10 @@ static const struct usb_device_id        products[] = {
>         /* Asus USB Ethernet Adapter */
>         USB_DEVICE(0x0b95, 0x7e2b),
>         .driver_info = (unsigned long) &ax88772_info,
> +}, {
> +       /* ASIX 88172a demo board */
> +       USB_DEVICE(0x0b95, 0x172a),
> +       .driver_info = (unsigned long) &ax88172a_info,
>  },
>         { },            /* END */
>  };
> diff --git a/drivers/net/usb/ax88172a.c b/drivers/net/usb/ax88172a.c
> new file mode 100644
> index 0000000..9f2d1fd
> --- /dev/null
> +++ b/drivers/net/usb/ax88172a.c
> @@ -0,0 +1,407 @@
> +/*
> + * ASIX AX88172A based USB 2.0 Ethernet Devices
> + * Copyright (C) 2012 OMICRON electronics GmbH
> + *
> + * Supports external PHYs via phylib. Based on the driver for the
> + * AX88772. Original copyrights follow:
> + *
> + * Copyright (C) 2003-2006 David Hollis <dhollis@davehollis.com>
> + * Copyright (C) 2005 Phil Chang <pchang23@sbcglobal.net>
> + * Copyright (C) 2006 James Painter <jamie.painter@iname.com>
> + * Copyright (c) 2002-2003 TiVo Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + */
> +
> +#include "asix.h"
> +#include <linux/phy.h>
> +
> +struct ax88172a_private {
> +       int use_embdphy;

Can you move the "int" to the end of the struct?
It's cleaner to have fields "natively align". ie pointers should start
at 8 byte alignments when compiled for 64-bit.

> +       struct mii_bus *mdio;
> +       struct phy_device *phydev;
> +       char phy_name[20];
> +       u16 phy_addr;
> +       u16 oldmode;
> +};
> +
> +static inline int asix_read_phy_addr(struct usbnet *dev, int internal)
> +{
> +       int offset = (internal ? 1 : 0);

One could use "internal" parameter directly for indexing if
use_embdphy were renamed to use_extphy and the logic inverted..

> +       u8 buf[2];
> +       int ret = asix_read_cmd(dev, AX_CMD_READ_PHY_ID, 0, 0, 2, buf);
> +
> +       netdev_dbg(dev->net, "asix_get_phy_addr()\n");
> +
> +       if (ret < 0) {
> +               netdev_err(dev->net, "Error reading PHYID register: %02x\n",
> +                          ret);
> +               goto out;
> +       }
> +       netdev_dbg(dev->net, "asix_get_phy_addr() returning 0x%04x\n",
> +                  *((__le16 *)buf));
> +       ret = buf[offset];
> +
> +out:
> +       return ret;
> +}
> +
> +static int ax88172a_ioctl(struct net_device *net, struct ifreq *rq, int cmd)
> +{
> +       return phy_mii_ioctl(net->phydev, rq, cmd);
> +}
> +
> +/* MDIO read and write wrappers for phylib */
> +static int asix_mdio_bus_read(struct mii_bus *bus, int phy_id, int regnum)
> +{
> +       return asix_mdio_read(((struct usbnet *)bus->priv)->net, phy_id,
> +                             regnum);
> +}
> +
> +static int asix_mdio_bus_write(struct mii_bus *bus, int phy_id, int regnum,
> +                              u16 val)
> +{
> +       asix_mdio_write(((struct usbnet *)bus->priv)->net, phy_id, regnum,
> +                       val);
> +       return 0;
> +}
> +
> +/* set MAC link settings according to information from phylib */
> +static void asix_adjust_link(struct net_device *netdev)
> +{
> +       struct phy_device *phydev = netdev->phydev;
> +       struct usbnet *dev = netdev_priv(netdev);
> +       struct ax88172a_private *priv =
> +               (struct ax88172a_private *)dev->driver_priv;
> +       u16 mode = 0;
> +
> +       dbg("asix_adjust_link called\n");
> +
> +       if (phydev->link) {
> +               mode = AX88772_MEDIUM_DEFAULT;
> +
> +               if (phydev->duplex == DUPLEX_HALF)
> +                       mode &= ~AX_MEDIUM_FD;
> +
> +               if (phydev->speed != SPEED_100)
> +                       mode &= ~AX_MEDIUM_PS;
> +       }
> +
> +       if (mode != priv->oldmode) {
> +               asix_write_medium_mode(dev, mode);
> +               priv->oldmode = mode;
> +               dbg("asix_adjust_link  speed: %u duplex: %d setting mode to 0x%04x\n",
> +                   phydev->speed, phydev->duplex, mode);
> +               phy_print_status(phydev);
> +       }
> +}
> +
> +static void ax88172a_status(struct usbnet *dev, struct urb *urb)
> +{
> +}
> +
> +/* use phylib infrastructure */
> +static int ax88172a_init_mdio(struct usbnet *dev)
> +{
> +       struct ax88172a_private *priv =
> +               (struct ax88172a_private *)dev->driver_priv;
> +       int ret, i;
> +
> +       priv->mdio = mdiobus_alloc();
> +       if (!priv->mdio) {
> +               dbg("Could not allocate MDIO bus");
> +               return -1;
> +       }
> +
> +       priv->mdio->priv = (void *)dev;
> +       priv->mdio->read = &asix_mdio_bus_read;
> +       priv->mdio->write = &asix_mdio_bus_write;
> +       priv->mdio->name = "Asix MDIO Bus";
> +       snprintf(priv->mdio->id, MII_BUS_ID_SIZE, "asix-%s",
> +                dev_name(dev->net->dev.parent));
> +
> +       priv->mdio->irq = kzalloc(sizeof(int) * PHY_MAX_ADDR, GFP_KERNEL);
> +       if (!priv->mdio->irq) {
> +               dbg("Could not allocate MDIO->IRQ");
> +               ret = -ENOMEM;
> +               goto mfree;
> +       }
> +       for (i = 0; i < PHY_MAX_ADDR; i++)
> +               priv->mdio->irq[i] = PHY_POLL;
> +
> +       ret = mdiobus_register(priv->mdio);
> +       if (ret) {
> +               dbg("Could not register MDIO bus");
> +               goto ifree;
> +       }
> +       snprintf(priv->phy_name, 20, PHY_ID_FMT,
> +                priv->mdio->id, priv->phy_addr);
> +
> +       priv->phydev = phy_connect(dev->net, priv->phy_name, &asix_adjust_link,
> +                                  0, PHY_INTERFACE_MODE_MII);
> +       if (IS_ERR(priv->phydev)) {
> +               dbg("Could not connect to PHY device");
> +               ret = PTR_ERR(priv->phydev);
> +               goto munreg;
> +       }
> +       dbg("dev->net->phydev (%s) is now 0x%p", priv->phy_name,
> +           dev->net->phydev);
> +
> +       /* During power-up, the AX88172A set the power down (BMCR_PDOWN)
> +        *   bit of the PHY. Bring the PHY up again.
> +        */
> +       genphy_resume(priv->phydev);
> +
> +       phy_start(priv->phydev);
> +
> +       return 0;
> +munreg:
> +       mdiobus_unregister(priv->mdio);
> +ifree:
> +       kfree(priv->mdio->irq);
> +mfree:
> +       mdiobus_free(priv->mdio);
> +       return ret;
> +}
> +
> +static void ax88172a_remove_mdio(struct usbnet *dev)
> +{
> +       struct ax88172a_private *priv =
> +               (struct ax88172a_private *)dev->driver_priv;
> +
> +       phy_stop(priv->phydev);
> +       phy_disconnect(priv->phydev);
> +       mdiobus_unregister(priv->mdio);
> +       kfree(priv->mdio->irq);
> +       mdiobus_free(priv->mdio);
> +}
> +
> +static const struct net_device_ops ax88172a_netdev_ops = {
> +       .ndo_open               = usbnet_open,
> +       .ndo_stop               = usbnet_stop,
> +       .ndo_start_xmit         = usbnet_start_xmit,
> +       .ndo_tx_timeout         = usbnet_tx_timeout,
> +       .ndo_change_mtu         = usbnet_change_mtu,
> +       .ndo_set_mac_address    = asix_set_mac_address,
> +       .ndo_validate_addr      = eth_validate_addr,
> +       .ndo_do_ioctl           = ax88172a_ioctl,
> +       .ndo_set_rx_mode        = asix_set_multicast,
> +};
> +
> +int ax88172a_get_settings(struct net_device *net, struct ethtool_cmd *cmd)
> +{
> +       return phy_ethtool_gset(net->phydev, cmd);
> +}
> +
> +int ax88172a_set_settings(struct net_device *net, struct ethtool_cmd *cmd)
> +{
> +       return phy_ethtool_sset(net->phydev, cmd);
> +}
> +
> +int ax88172a_nway_reset(struct net_device *net)
> +{
> +       return phy_start_aneg(net->phydev);
> +}
> +
> +static const struct ethtool_ops ax88172a_ethtool_ops = {
> +       .get_drvinfo            = asix_get_drvinfo,
> +       .get_link               = usbnet_get_link,
> +       .get_msglevel           = usbnet_get_msglevel,
> +       .set_msglevel           = usbnet_set_msglevel,
> +       .get_wol                = asix_get_wol,
> +       .set_wol                = asix_set_wol,
> +       .get_eeprom_len         = asix_get_eeprom_len,
> +       .get_eeprom             = asix_get_eeprom,
> +       .get_settings           = ax88172a_get_settings,
> +       .set_settings           = ax88172a_set_settings,
> +       .nway_reset             = ax88172a_nway_reset,
> +};
> +
> +static int ax88172a_reset_phy(struct usbnet *dev, int embd_phy)
> +{
> +       int ret;
> +
> +       ret = asix_sw_reset(dev, AX_SWRESET_IPPD);
> +       if (ret < 0)
> +               goto err;
> +
> +       msleep(150);
> +       ret = asix_sw_reset(dev, AX_SWRESET_CLEAR);
> +       if (ret < 0)
> +               goto err;
> +
> +       msleep(150);
> +
> +       ret = asix_sw_reset(dev, embd_phy ? AX_SWRESET_IPRL : AX_SWRESET_IPPD);

(would have to swap things here if adopting my suggestions.)

> +       if (ret < 0)
> +               goto err;
> +
> +       return 0;
> +
> +err:
> +       return ret;
> +}
> +
> +
> +static int ax88172a_bind(struct usbnet *dev, struct usb_interface *intf)
> +{
> +       int ret;
> +       struct asix_data *data = (struct asix_data *)&dev->data;
> +       u8 buf[ETH_ALEN];
> +       struct ax88172a_private *priv;
> +
> +       data->eeprom_len = AX88772_EEPROM_LEN;
> +
> +       usbnet_get_endpoints(dev, intf);
> +
> +       priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +       if (!priv) {
> +               dbg("Could not allocate memory for private data");
> +               return -ENOMEM;
> +       }
> +       dev->driver_priv = priv;
> +
> +       /* Get the MAC address */
> +       ret = asix_read_cmd(dev, AX_CMD_READ_NODE_ID, 0, 0, ETH_ALEN, buf);
> +       if (ret < 0) {
> +               dbg("Failed to read MAC address: %d", ret);
> +               goto free;
> +       }
> +       memcpy(dev->net->dev_addr, buf, ETH_ALEN);
> +
> +       dev->net->netdev_ops = &ax88172a_netdev_ops;
> +       dev->net->ethtool_ops = &ax88172a_ethtool_ops;
> +
> +       /* are we using the internal or the external phy? */
> +       ret = asix_read_cmd(dev, AX_CMD_SW_PHY_STATUS, 0, 0, 1, buf);
> +       if (ret < 0) {
> +               dbg("Failed to read software interface selection register: %d",
> +                   ret);
> +               goto free;
> +       }
> +       dbg("AX_CMD_SW_PHY_STATUS = 0x%02x\n", buf[0]);
> +       switch ((buf[0] & 0x0c) >> 2) {
> +       case 0:
> +               dbg("use internal phy\n");
> +               priv->use_embdphy = 1;
> +               break;
> +       case 1:
> +               dbg("use external phy\n");
> +               priv->use_embdphy = 0;
> +               break;
> +       default:
> +               dbg("Interface mode not supported by driver\n");
> +               goto free;
> +       }

This switch statement inverts the existing logic. Much simpler code would be:
    /* buf[0] & 0xc describes phy interface mode */
    if (buf[0] &  8) {
         dbg("Interface mode not supported by driver\n");
         goto free;
    }
    priv->use_extphy = (buf[0] & 4) >> 2;

> +
> +       priv->phy_addr = asix_read_phy_addr(dev, priv->use_embdphy);
> +       ax88172a_reset_phy(dev, priv->use_embdphy);
> +
> +       /* Asix framing packs multiple eth frames into a 2K usb bulk transfer */
> +       if (dev->driver_info->flags & FLAG_FRAMING_AX) {
> +               /* hard_mtu  is still the default - the device does not support
> +                  jumbo eth frames */
> +               dev->rx_urb_size = 2048;
> +       }
> +
> +       /* init MDIO bus */
> +       ret = ax88172a_init_mdio(dev);
> +       if (ret)
> +               goto free;
> +
> +       return 0;
> +
> +free:
> +       kfree(priv);
> +       return ret;
> +}
> +
> +static void ax88172a_unbind(struct usbnet *dev, struct usb_interface *intf)
> +{
> +       struct ax88172a_private *priv =
> +               (struct ax88172a_private *)dev->driver_priv;
> +
> +       ax88172a_remove_mdio(dev);
> +       kfree(priv);
> +}
> +
> +static int ax88172a_reset(struct usbnet *dev)
> +{
> +       struct asix_data *data = (struct asix_data *)&dev->data;
> +       struct ax88172a_private *priv =
> +               (struct ax88172a_private *)dev->driver_priv;
> +       int ret;
> +       u16 rx_ctl;
> +
> +       ax88172a_reset_phy(dev, priv->use_embdphy);
> +
> +       msleep(150);
> +       rx_ctl = asix_read_rx_ctl(dev);
> +       dbg("RX_CTL is 0x%04x after software reset", rx_ctl);
> +       ret = asix_write_rx_ctl(dev, 0x0000);
> +       if (ret < 0)
> +               goto out;
> +
> +       rx_ctl = asix_read_rx_ctl(dev);
> +       dbg("RX_CTL is 0x%04x setting to 0x0000", rx_ctl);
> +
> +       msleep(150);
> +
> +       ax88172a_nway_reset(dev->net);
> +
> +       ret = asix_write_cmd(dev, AX_CMD_WRITE_IPG0,
> +                               AX88772_IPG0_DEFAULT | AX88772_IPG1_DEFAULT,
> +                               AX88772_IPG2_DEFAULT, 0, NULL);
> +       if (ret < 0) {
> +               dbg("Write IPG,IPG1,IPG2 failed: %d", ret);
> +               goto out;
> +       }
> +
> +       /* Rewrite MAC address */
> +       memcpy(data->mac_addr, dev->net->dev_addr, ETH_ALEN);
> +       ret = asix_write_cmd(dev, AX_CMD_WRITE_NODE_ID, 0, 0, ETH_ALEN,
> +                                                       data->mac_addr);
> +       if (ret < 0)
> +               goto out;
> +
> +       /* Set RX_CTL to default values with 2k buffer, and enable cactus */
> +       ret = asix_write_rx_ctl(dev, AX_DEFAULT_RX_CTL);
> +       if (ret < 0)
> +               goto out;
> +
> +       rx_ctl = asix_read_rx_ctl(dev);
> +       dbg("RX_CTL is 0x%04x after all initializations", rx_ctl);
> +
> +       rx_ctl = asix_read_medium_status(dev);
> +       dbg("Medium Status is 0x%04x after all initializations", rx_ctl);
> +
> +       return 0;
> +
> +out:
> +       return ret;
> +
> +}
> +
> +const struct driver_info ax88172a_info = {
> +       .description = "ASIX AX88172A USB 2.0 Ethernet",
> +       .bind = ax88172a_bind,
> +       .unbind = ax88172a_unbind,
> +       .status = ax88172a_status,
> +       .reset = ax88172a_reset,
> +       .flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_LINK_INTR |
> +                FLAG_MULTI_PACKET,
> +       .rx_fixup = asix_rx_fixup,
> +       .tx_fixup = asix_tx_fixup,
> +};
> --
> 1.7.0.4
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael Riesch July 8, 2012, 3:39 p.m. UTC | #3
On Fri, 2012-07-06 at 18:37 +0100, Ben Hutchings wrote:
> > +	priv->mdio->priv = (void *)dev;
> > +	priv->mdio->read = &asix_mdio_bus_read;
> > +	priv->mdio->write = &asix_mdio_bus_write;
> > +	priv->mdio->name = "Asix MDIO Bus";
> > +	snprintf(priv->mdio->id, MII_BUS_ID_SIZE, "asix-%s",
> > +		 dev_name(dev->net->dev.parent));
> [...]
> 
> I think you need to ensure that the bus identifier is unique throughout
> its lifetime, but net devices can be renamed and that could lead to a
> collision.  Perhaps you could use the ifindex or the USB device path

Ben,

the dev_name function in the code above returns the sysfs filename of
the USB device (e.g. 1-0:1.0).

> (though that might be too long).

This may be a problem. The bus identifier may be 17 characters long, so
if we leave the endpoint/configuration part (:1.0) and the prefix away
it should be fine in any "normal" system. However, on a system with a
more-than-9-root-hubs 5-tier 127-devices-each USB infrastructure it
results in collisions. So is this approach acceptable?

Using the ifindex sounds good to me,

snprintf(priv->mdio->id, MII_BUS_ID_SIZE, "asix-%d",
	dev->net->ifindex);

works on any system with less than 10^12 network interfaces.

Regards,
Michael



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings July 8, 2012, 3:50 p.m. UTC | #4
On Sun, 2012-07-08 at 17:39 +0200, Michael Riesch wrote:
> On Fri, 2012-07-06 at 18:37 +0100, Ben Hutchings wrote:
> > > +	priv->mdio->priv = (void *)dev;
> > > +	priv->mdio->read = &asix_mdio_bus_read;
> > > +	priv->mdio->write = &asix_mdio_bus_write;
> > > +	priv->mdio->name = "Asix MDIO Bus";
> > > +	snprintf(priv->mdio->id, MII_BUS_ID_SIZE, "asix-%s",
> > > +		 dev_name(dev->net->dev.parent));
> > [...]
> > 
> > I think you need to ensure that the bus identifier is unique throughout
> > its lifetime, but net devices can be renamed and that could lead to a
> > collision.  Perhaps you could use the ifindex or the USB device path
> 
> Ben,
> 
> the dev_name function in the code above returns the sysfs filename of
> the USB device (e.g. 1-0:1.0).
[...]

Sorry, I didn't read that carefully enough.

Ben.
Christian Riesch July 9, 2012, 10:22 a.m. UTC | #5
Grant,

On Fri, Jul 6, 2012 at 11:20 PM, Grant Grundler <grundler@chromium.org> wrote:
> On Fri, Jul 6, 2012 at 4:33 AM, Christian Riesch
> <christian.riesch@omicron.at> wrote:
>> The Asix AX88172A is a USB 2.0 Ethernet interface that supports both an
>> internal PHY as well as an external PHY (connected via MII).
>>
>> This patch adds a driver for the AX88172A and provides support for
>> both modes and supports phylib.
>
> Christian,
> In general this looks fine to me...but I wouldn't know about "bus
> identifier life times" (Ben Hutchings comment).
>
> My nit pick is the declaration and of use_embdphy. An alternative
> coding _suggestion_ below.  I'm not substantially altering the
> functionality.
>
> thanks,
> grant

[...]

>> +
>> +struct ax88172a_private {
>> +       int use_embdphy;
>
> Can you move the "int" to the end of the struct?
> It's cleaner to have fields "natively align". ie pointers should start
> at 8 byte alignments when compiled for 64-bit.
>
>> +       struct mii_bus *mdio;
>> +       struct phy_device *phydev;
>> +       char phy_name[20];
>> +       u16 phy_addr;
>> +       u16 oldmode;
>> +};
>> +

[...]

>> +       /* are we using the internal or the external phy? */
>> +       ret = asix_read_cmd(dev, AX_CMD_SW_PHY_STATUS, 0, 0, 1, buf);
>> +       if (ret < 0) {
>> +               dbg("Failed to read software interface selection register: %d",
>> +                   ret);
>> +               goto free;
>> +       }
>> +       dbg("AX_CMD_SW_PHY_STATUS = 0x%02x\n", buf[0]);
>> +       switch ((buf[0] & 0x0c) >> 2) {
>> +       case 0:
>> +               dbg("use internal phy\n");
>> +               priv->use_embdphy = 1;
>> +               break;
>> +       case 1:
>> +               dbg("use external phy\n");
>> +               priv->use_embdphy = 0;
>> +               break;
>> +       default:
>> +               dbg("Interface mode not supported by driver\n");
>> +               goto free;
>> +       }
>
> This switch statement inverts the existing logic. Much simpler code would be:
>     /* buf[0] & 0xc describes phy interface mode */
>     if (buf[0] &  8) {
>          dbg("Interface mode not supported by driver\n");
>          goto free;
>     }
>     priv->use_extphy = (buf[0] & 4) >> 2;
>

Thank your for your comments! I'll change that in the next version!
Regards, Christian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christian Riesch July 9, 2012, 10:30 a.m. UTC | #6
Hi Ben and Michael,

On Sun, Jul 8, 2012 at 5:39 PM, Michael Riesch <michael@riesch.at> wrote:
> On Fri, 2012-07-06 at 18:37 +0100, Ben Hutchings wrote:
>> > +   priv->mdio->priv = (void *)dev;
>> > +   priv->mdio->read = &asix_mdio_bus_read;
>> > +   priv->mdio->write = &asix_mdio_bus_write;
>> > +   priv->mdio->name = "Asix MDIO Bus";
>> > +   snprintf(priv->mdio->id, MII_BUS_ID_SIZE, "asix-%s",
>> > +            dev_name(dev->net->dev.parent));
>> [...]
>>
>> I think you need to ensure that the bus identifier is unique throughout
>> its lifetime, but net devices can be renamed and that could lead to a
>> collision.  Perhaps you could use the ifindex or the USB device path
>
> Ben,
>
> the dev_name function in the code above returns the sysfs filename of
> the USB device (e.g. 1-0:1.0).
>
>> (though that might be too long).
>
> This may be a problem. The bus identifier may be 17 characters long, so
> if we leave the endpoint/configuration part (:1.0) and the prefix away
> it should be fine in any "normal" system. However, on a system with a
> more-than-9-root-hubs 5-tier 127-devices-each USB infrastructure it
> results in collisions. So is this approach acceptable?
>
> Using the ifindex sounds good to me,
>
> snprintf(priv->mdio->id, MII_BUS_ID_SIZE, "asix-%d",
>         dev->net->ifindex);
>
> works on any system with less than 10^12 network interfaces.

Ok, I'll change that to use ifindex. I didn't like the mdio bus name
with lots of colons, dashes, and periods anyway...
Thank you!

Regards, Christian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christian Riesch July 11, 2012, 8:27 a.m. UTC | #7
Hi Ben and Michael,

On Mon, Jul 9, 2012 at 12:30 PM, Christian Riesch
<christian.riesch@omicron.at> wrote:
> Hi Ben and Michael,
>
> On Sun, Jul 8, 2012 at 5:39 PM, Michael Riesch <michael@riesch.at> wrote:
>> On Fri, 2012-07-06 at 18:37 +0100, Ben Hutchings wrote:
>>> > +   priv->mdio->priv = (void *)dev;
>>> > +   priv->mdio->read = &asix_mdio_bus_read;
>>> > +   priv->mdio->write = &asix_mdio_bus_write;
>>> > +   priv->mdio->name = "Asix MDIO Bus";
>>> > +   snprintf(priv->mdio->id, MII_BUS_ID_SIZE, "asix-%s",
>>> > +            dev_name(dev->net->dev.parent));
>>> [...]
>>>
>>> I think you need to ensure that the bus identifier is unique throughout
>>> its lifetime, but net devices can be renamed and that could lead to a
>>> collision.  Perhaps you could use the ifindex or the USB device path
>>
>> Ben,
>>
>> the dev_name function in the code above returns the sysfs filename of
>> the USB device (e.g. 1-0:1.0).
>>
>>> (though that might be too long).
>>
>> This may be a problem. The bus identifier may be 17 characters long, so
>> if we leave the endpoint/configuration part (:1.0) and the prefix away
>> it should be fine in any "normal" system. However, on a system with a
>> more-than-9-root-hubs 5-tier 127-devices-each USB infrastructure it
>> results in collisions. So is this approach acceptable?
>>
>> Using the ifindex sounds good to me,
>>
>> snprintf(priv->mdio->id, MII_BUS_ID_SIZE, "asix-%d",
>>         dev->net->ifindex);
>>
>> works on any system with less than 10^12 network interfaces.
>
> Ok, I'll change that to use ifindex.

No, I won't.
At the time the mdio bus is registered, ifindex is not yet set, so the
snprintf would always result in "asix-0".

Any ideas?
Should I keep the dev_name(dev->net->dev.parent) and hope that nobody
uses that many USB devices/hubs so that it is short enough? Or use
some other solution? A global counter in ax88172a.c that numbers the
mdio busses (asix-0, asix-1...)?

Regards, Christian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christian Riesch July 11, 2012, 3:10 p.m. UTC | #8
Hi again,

On Wed, Jul 11, 2012 at 10:27 AM, Christian Riesch
<christian.riesch@omicron.at> wrote:
> Hi Ben and Michael,
>
> On Mon, Jul 9, 2012 at 12:30 PM, Christian Riesch
> <christian.riesch@omicron.at> wrote:
>> Hi Ben and Michael,
>>
>> On Sun, Jul 8, 2012 at 5:39 PM, Michael Riesch <michael@riesch.at> wrote:
>>> On Fri, 2012-07-06 at 18:37 +0100, Ben Hutchings wrote:
>>>> > +   priv->mdio->priv = (void *)dev;
>>>> > +   priv->mdio->read = &asix_mdio_bus_read;
>>>> > +   priv->mdio->write = &asix_mdio_bus_write;
>>>> > +   priv->mdio->name = "Asix MDIO Bus";
>>>> > +   snprintf(priv->mdio->id, MII_BUS_ID_SIZE, "asix-%s",
>>>> > +            dev_name(dev->net->dev.parent));
>>>> [...]
>>>>
>>>> I think you need to ensure that the bus identifier is unique throughout
>>>> its lifetime, but net devices can be renamed and that could lead to a
>>>> collision.  Perhaps you could use the ifindex or the USB device path
>>>
>>> Ben,
>>>
>>> the dev_name function in the code above returns the sysfs filename of
>>> the USB device (e.g. 1-0:1.0).
>>>
>>>> (though that might be too long).
>>>
>>> This may be a problem. The bus identifier may be 17 characters long, so
>>> if we leave the endpoint/configuration part (:1.0) and the prefix away
>>> it should be fine in any "normal" system. However, on a system with a
>>> more-than-9-root-hubs 5-tier 127-devices-each USB infrastructure it
>>> results in collisions. So is this approach acceptable?
>>>
>>> Using the ifindex sounds good to me,
>>>
>>> snprintf(priv->mdio->id, MII_BUS_ID_SIZE, "asix-%d",
>>>         dev->net->ifindex);
>>>
>>> works on any system with less than 10^12 network interfaces.
>>
>> Ok, I'll change that to use ifindex.
>
> No, I won't.
> At the time the mdio bus is registered, ifindex is not yet set, so the
> snprintf would always result in "asix-0".

What do you think about
snprintf(priv->mdio->id, MII_BUS_ID_SIZE, "usb-%03d:%03d",
dev->udev->bus->busnum, dev->udev->devnum);
??

This would use the busnum/devnum identifier as reported by lsusb and
would be short enough for an mdio bus name.

Thanks, Christian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael Riesch July 11, 2012, 7:23 p.m. UTC | #9
On Wed, 2012-07-11 at 17:10 +0200, Christian Riesch wrote:
> Hi again,
[...]
> >>> Using the ifindex sounds good to me,
> >>>
> >>> snprintf(priv->mdio->id, MII_BUS_ID_SIZE, "asix-%d",
> >>>         dev->net->ifindex);
> >>>
> >>> works on any system with less than 10^12 network interfaces.
> >>
> >> Ok, I'll change that to use ifindex.
> >
> > No, I won't.
> > At the time the mdio bus is registered, ifindex is not yet set, so the
> > snprintf would always result in "asix-0".
> 
> What do you think about
> snprintf(priv->mdio->id, MII_BUS_ID_SIZE, "usb-%03d:%03d",
> dev->udev->bus->busnum, dev->udev->devnum);
> ??
> 
> This would use the busnum/devnum identifier as reported by lsusb and
> would be short enough for an mdio bus name.

IMHO this would be a good solution - it is unique and there you can tell
which MDIO bus belongs to which USB device by the name (not sure if
someone will ever need to do that, but it is neat).

Regards, Michael

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christian Riesch July 12, 2012, 7:22 a.m. UTC | #10
Hi Grant,

On Mon, Jul 9, 2012 at 12:22 PM, Christian Riesch
<christian.riesch@omicron.at> wrote:
> Grant,
>
> On Fri, Jul 6, 2012 at 11:20 PM, Grant Grundler <grundler@chromium.org> wrote:
>> On Fri, Jul 6, 2012 at 4:33 AM, Christian Riesch
>> <christian.riesch@omicron.at> wrote:
>>> The Asix AX88172A is a USB 2.0 Ethernet interface that supports both an
>>> internal PHY as well as an external PHY (connected via MII).
>>>
>>> This patch adds a driver for the AX88172A and provides support for
>>> both modes and supports phylib.
>>
>> Christian,
>> In general this looks fine to me...but I wouldn't know about "bus
>> identifier life times" (Ben Hutchings comment).
>>
>> My nit pick is the declaration and of use_embdphy. An alternative
>> coding _suggestion_ below.  I'm not substantially altering the
>> functionality.
>>
>> thanks,
>> grant
>
> [...]
>
>>> +
>>> +struct ax88172a_private {
>>> +       int use_embdphy;
>>
>> Can you move the "int" to the end of the struct?
>> It's cleaner to have fields "natively align". ie pointers should start
>> at 8 byte alignments when compiled for 64-bit.
>>
>>> +       struct mii_bus *mdio;
>>> +       struct phy_device *phydev;
>>> +       char phy_name[20];
>>> +       u16 phy_addr;
>>> +       u16 oldmode;
>>> +};
>>> +
>
> [...]
>
>>> +       /* are we using the internal or the external phy? */
>>> +       ret = asix_read_cmd(dev, AX_CMD_SW_PHY_STATUS, 0, 0, 1, buf);
>>> +       if (ret < 0) {
>>> +               dbg("Failed to read software interface selection register: %d",
>>> +                   ret);
>>> +               goto free;
>>> +       }
>>> +       dbg("AX_CMD_SW_PHY_STATUS = 0x%02x\n", buf[0]);
>>> +       switch ((buf[0] & 0x0c) >> 2) {
>>> +       case 0:
>>> +               dbg("use internal phy\n");
>>> +               priv->use_embdphy = 1;
>>> +               break;
>>> +       case 1:
>>> +               dbg("use external phy\n");
>>> +               priv->use_embdphy = 0;
>>> +               break;
>>> +       default:
>>> +               dbg("Interface mode not supported by driver\n");
>>> +               goto free;
>>> +       }
>>
>> This switch statement inverts the existing logic. Much simpler code would be:
>>     /* buf[0] & 0xc describes phy interface mode */
>>     if (buf[0] &  8) {
>>          dbg("Interface mode not supported by driver\n");
>>          goto free;
>>     }
>>     priv->use_extphy = (buf[0] & 4) >> 2;
>>
>
> Thank your for your comments! I'll change that in the next version!
> Regards, Christian

After rethinking it I decided to keep the switch structure, but use
defines for the different modes and the bitmask. I think this will be
easier to understand. I will submit a new version of the patchset
later today, please have a look at it.
Thanks!
Christian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Grant Grundler July 12, 2012, 11:10 p.m. UTC | #11
On Thu, Jul 12, 2012 at 12:22 AM, Christian Riesch
<christian.riesch@omicron.at> wrote:
> Hi Grant,
...
> After rethinking it I decided to keep the switch structure, but use
> defines for the different modes and the bitmask. I think this will be
> easier to understand. I will submit a new version of the patchset
> later today, please have a look at it.

That's fine too. I agree the new version is easier to read.

thanks!
grant

> Thanks!
> Christian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/usb/Makefile b/drivers/net/usb/Makefile
index a9490d9..bf06300 100644
--- a/drivers/net/usb/Makefile
+++ b/drivers/net/usb/Makefile
@@ -8,7 +8,7 @@  obj-$(CONFIG_USB_PEGASUS)	+= pegasus.o
 obj-$(CONFIG_USB_RTL8150)	+= rtl8150.o
 obj-$(CONFIG_USB_HSO)		+= hso.o
 obj-$(CONFIG_USB_NET_AX8817X)	+= asix.o
-asix-y := asix_devices.o asix_common.o
+asix-y := asix_devices.o asix_common.o ax88172a.o
 obj-$(CONFIG_USB_NET_CDCETHER)	+= cdc_ether.o
 obj-$(CONFIG_USB_NET_CDC_EEM)	+= cdc_eem.o
 obj-$(CONFIG_USB_NET_DM9601)	+= dm9601.o
diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
index c8682a5..02b8c21 100644
--- a/drivers/net/usb/asix_devices.c
+++ b/drivers/net/usb/asix_devices.c
@@ -877,6 +877,8 @@  static const struct driver_info ax88178_info = {
 	.tx_fixup = asix_tx_fixup,
 };
 
+extern const struct driver_info ax88172a_info;
+
 static const struct usb_device_id	products[] = {
 {
 	/* Linksys USB200M */
@@ -1002,6 +1004,10 @@  static const struct usb_device_id	products[] = {
 	/* Asus USB Ethernet Adapter */
 	USB_DEVICE(0x0b95, 0x7e2b),
 	.driver_info = (unsigned long) &ax88772_info,
+}, {
+	/* ASIX 88172a demo board */
+	USB_DEVICE(0x0b95, 0x172a),
+	.driver_info = (unsigned long) &ax88172a_info,
 },
 	{ },		/* END */
 };
diff --git a/drivers/net/usb/ax88172a.c b/drivers/net/usb/ax88172a.c
new file mode 100644
index 0000000..9f2d1fd
--- /dev/null
+++ b/drivers/net/usb/ax88172a.c
@@ -0,0 +1,407 @@ 
+/*
+ * ASIX AX88172A based USB 2.0 Ethernet Devices
+ * Copyright (C) 2012 OMICRON electronics GmbH
+ *
+ * Supports external PHYs via phylib. Based on the driver for the
+ * AX88772. Original copyrights follow:
+ *
+ * Copyright (C) 2003-2006 David Hollis <dhollis@davehollis.com>
+ * Copyright (C) 2005 Phil Chang <pchang23@sbcglobal.net>
+ * Copyright (C) 2006 James Painter <jamie.painter@iname.com>
+ * Copyright (c) 2002-2003 TiVo Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include "asix.h"
+#include <linux/phy.h>
+
+struct ax88172a_private {
+	int use_embdphy;
+	struct mii_bus *mdio;
+	struct phy_device *phydev;
+	char phy_name[20];
+	u16 phy_addr;
+	u16 oldmode;
+};
+
+static inline int asix_read_phy_addr(struct usbnet *dev, int internal)
+{
+	int offset = (internal ? 1 : 0);
+	u8 buf[2];
+	int ret = asix_read_cmd(dev, AX_CMD_READ_PHY_ID, 0, 0, 2, buf);
+
+	netdev_dbg(dev->net, "asix_get_phy_addr()\n");
+
+	if (ret < 0) {
+		netdev_err(dev->net, "Error reading PHYID register: %02x\n",
+			   ret);
+		goto out;
+	}
+	netdev_dbg(dev->net, "asix_get_phy_addr() returning 0x%04x\n",
+		   *((__le16 *)buf));
+	ret = buf[offset];
+
+out:
+	return ret;
+}
+
+static int ax88172a_ioctl(struct net_device *net, struct ifreq *rq, int cmd)
+{
+	return phy_mii_ioctl(net->phydev, rq, cmd);
+}
+
+/* MDIO read and write wrappers for phylib */
+static int asix_mdio_bus_read(struct mii_bus *bus, int phy_id, int regnum)
+{
+	return asix_mdio_read(((struct usbnet *)bus->priv)->net, phy_id,
+			      regnum);
+}
+
+static int asix_mdio_bus_write(struct mii_bus *bus, int phy_id, int regnum,
+			       u16 val)
+{
+	asix_mdio_write(((struct usbnet *)bus->priv)->net, phy_id, regnum,
+			val);
+	return 0;
+}
+
+/* set MAC link settings according to information from phylib */
+static void asix_adjust_link(struct net_device *netdev)
+{
+	struct phy_device *phydev = netdev->phydev;
+	struct usbnet *dev = netdev_priv(netdev);
+	struct ax88172a_private *priv =
+		(struct ax88172a_private *)dev->driver_priv;
+	u16 mode = 0;
+
+	dbg("asix_adjust_link called\n");
+
+	if (phydev->link) {
+		mode = AX88772_MEDIUM_DEFAULT;
+
+		if (phydev->duplex == DUPLEX_HALF)
+			mode &= ~AX_MEDIUM_FD;
+
+		if (phydev->speed != SPEED_100)
+			mode &= ~AX_MEDIUM_PS;
+	}
+
+	if (mode != priv->oldmode) {
+		asix_write_medium_mode(dev, mode);
+		priv->oldmode = mode;
+		dbg("asix_adjust_link  speed: %u duplex: %d setting mode to 0x%04x\n",
+		    phydev->speed, phydev->duplex, mode);
+		phy_print_status(phydev);
+	}
+}
+
+static void ax88172a_status(struct usbnet *dev, struct urb *urb)
+{
+}
+
+/* use phylib infrastructure */
+static int ax88172a_init_mdio(struct usbnet *dev)
+{
+	struct ax88172a_private *priv =
+		(struct ax88172a_private *)dev->driver_priv;
+	int ret, i;
+
+	priv->mdio = mdiobus_alloc();
+	if (!priv->mdio) {
+		dbg("Could not allocate MDIO bus");
+		return -1;
+	}
+
+	priv->mdio->priv = (void *)dev;
+	priv->mdio->read = &asix_mdio_bus_read;
+	priv->mdio->write = &asix_mdio_bus_write;
+	priv->mdio->name = "Asix MDIO Bus";
+	snprintf(priv->mdio->id, MII_BUS_ID_SIZE, "asix-%s",
+		 dev_name(dev->net->dev.parent));
+
+	priv->mdio->irq = kzalloc(sizeof(int) * PHY_MAX_ADDR, GFP_KERNEL);
+	if (!priv->mdio->irq) {
+		dbg("Could not allocate MDIO->IRQ");
+		ret = -ENOMEM;
+		goto mfree;
+	}
+	for (i = 0; i < PHY_MAX_ADDR; i++)
+		priv->mdio->irq[i] = PHY_POLL;
+
+	ret = mdiobus_register(priv->mdio);
+	if (ret) {
+		dbg("Could not register MDIO bus");
+		goto ifree;
+	}
+	snprintf(priv->phy_name, 20, PHY_ID_FMT,
+		 priv->mdio->id, priv->phy_addr);
+
+	priv->phydev = phy_connect(dev->net, priv->phy_name, &asix_adjust_link,
+				   0, PHY_INTERFACE_MODE_MII);
+	if (IS_ERR(priv->phydev)) {
+		dbg("Could not connect to PHY device");
+		ret = PTR_ERR(priv->phydev);
+		goto munreg;
+	}
+	dbg("dev->net->phydev (%s) is now 0x%p", priv->phy_name,
+	    dev->net->phydev);
+
+	/* During power-up, the AX88172A set the power down (BMCR_PDOWN)
+	 *   bit of the PHY. Bring the PHY up again.
+	 */
+	genphy_resume(priv->phydev);
+
+	phy_start(priv->phydev);
+
+	return 0;
+munreg:
+	mdiobus_unregister(priv->mdio);
+ifree:
+	kfree(priv->mdio->irq);
+mfree:
+	mdiobus_free(priv->mdio);
+	return ret;
+}
+
+static void ax88172a_remove_mdio(struct usbnet *dev)
+{
+	struct ax88172a_private *priv =
+		(struct ax88172a_private *)dev->driver_priv;
+
+	phy_stop(priv->phydev);
+	phy_disconnect(priv->phydev);
+	mdiobus_unregister(priv->mdio);
+	kfree(priv->mdio->irq);
+	mdiobus_free(priv->mdio);
+}
+
+static const struct net_device_ops ax88172a_netdev_ops = {
+	.ndo_open		= usbnet_open,
+	.ndo_stop		= usbnet_stop,
+	.ndo_start_xmit		= usbnet_start_xmit,
+	.ndo_tx_timeout		= usbnet_tx_timeout,
+	.ndo_change_mtu		= usbnet_change_mtu,
+	.ndo_set_mac_address	= asix_set_mac_address,
+	.ndo_validate_addr	= eth_validate_addr,
+	.ndo_do_ioctl		= ax88172a_ioctl,
+	.ndo_set_rx_mode        = asix_set_multicast,
+};
+
+int ax88172a_get_settings(struct net_device *net, struct ethtool_cmd *cmd)
+{
+	return phy_ethtool_gset(net->phydev, cmd);
+}
+
+int ax88172a_set_settings(struct net_device *net, struct ethtool_cmd *cmd)
+{
+	return phy_ethtool_sset(net->phydev, cmd);
+}
+
+int ax88172a_nway_reset(struct net_device *net)
+{
+	return phy_start_aneg(net->phydev);
+}
+
+static const struct ethtool_ops ax88172a_ethtool_ops = {
+	.get_drvinfo		= asix_get_drvinfo,
+	.get_link		= usbnet_get_link,
+	.get_msglevel		= usbnet_get_msglevel,
+	.set_msglevel		= usbnet_set_msglevel,
+	.get_wol		= asix_get_wol,
+	.set_wol		= asix_set_wol,
+	.get_eeprom_len		= asix_get_eeprom_len,
+	.get_eeprom		= asix_get_eeprom,
+	.get_settings		= ax88172a_get_settings,
+	.set_settings		= ax88172a_set_settings,
+	.nway_reset		= ax88172a_nway_reset,
+};
+
+static int ax88172a_reset_phy(struct usbnet *dev, int embd_phy)
+{
+	int ret;
+
+	ret = asix_sw_reset(dev, AX_SWRESET_IPPD);
+	if (ret < 0)
+		goto err;
+
+	msleep(150);
+	ret = asix_sw_reset(dev, AX_SWRESET_CLEAR);
+	if (ret < 0)
+		goto err;
+
+	msleep(150);
+
+	ret = asix_sw_reset(dev, embd_phy ? AX_SWRESET_IPRL : AX_SWRESET_IPPD);
+	if (ret < 0)
+		goto err;
+
+	return 0;
+
+err:
+	return ret;
+}
+
+
+static int ax88172a_bind(struct usbnet *dev, struct usb_interface *intf)
+{
+	int ret;
+	struct asix_data *data = (struct asix_data *)&dev->data;
+	u8 buf[ETH_ALEN];
+	struct ax88172a_private *priv;
+
+	data->eeprom_len = AX88772_EEPROM_LEN;
+
+	usbnet_get_endpoints(dev, intf);
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv) {
+		dbg("Could not allocate memory for private data");
+		return -ENOMEM;
+	}
+	dev->driver_priv = priv;
+
+	/* Get the MAC address */
+	ret = asix_read_cmd(dev, AX_CMD_READ_NODE_ID, 0, 0, ETH_ALEN, buf);
+	if (ret < 0) {
+		dbg("Failed to read MAC address: %d", ret);
+		goto free;
+	}
+	memcpy(dev->net->dev_addr, buf, ETH_ALEN);
+
+	dev->net->netdev_ops = &ax88172a_netdev_ops;
+	dev->net->ethtool_ops = &ax88172a_ethtool_ops;
+
+	/* are we using the internal or the external phy? */
+	ret = asix_read_cmd(dev, AX_CMD_SW_PHY_STATUS, 0, 0, 1, buf);
+	if (ret < 0) {
+		dbg("Failed to read software interface selection register: %d",
+		    ret);
+		goto free;
+	}
+	dbg("AX_CMD_SW_PHY_STATUS = 0x%02x\n", buf[0]);
+	switch ((buf[0] & 0x0c) >> 2) {
+	case 0:
+		dbg("use internal phy\n");
+		priv->use_embdphy = 1;
+		break;
+	case 1:
+		dbg("use external phy\n");
+		priv->use_embdphy = 0;
+		break;
+	default:
+		dbg("Interface mode not supported by driver\n");
+		goto free;
+	}
+
+	priv->phy_addr = asix_read_phy_addr(dev, priv->use_embdphy);
+	ax88172a_reset_phy(dev, priv->use_embdphy);
+
+	/* Asix framing packs multiple eth frames into a 2K usb bulk transfer */
+	if (dev->driver_info->flags & FLAG_FRAMING_AX) {
+		/* hard_mtu  is still the default - the device does not support
+		   jumbo eth frames */
+		dev->rx_urb_size = 2048;
+	}
+
+	/* init MDIO bus */
+	ret = ax88172a_init_mdio(dev);
+	if (ret)
+		goto free;
+
+	return 0;
+
+free:
+	kfree(priv);
+	return ret;
+}
+
+static void ax88172a_unbind(struct usbnet *dev, struct usb_interface *intf)
+{
+	struct ax88172a_private *priv =
+		(struct ax88172a_private *)dev->driver_priv;
+
+	ax88172a_remove_mdio(dev);
+	kfree(priv);
+}
+
+static int ax88172a_reset(struct usbnet *dev)
+{
+	struct asix_data *data = (struct asix_data *)&dev->data;
+	struct ax88172a_private *priv =
+		(struct ax88172a_private *)dev->driver_priv;
+	int ret;
+	u16 rx_ctl;
+
+	ax88172a_reset_phy(dev, priv->use_embdphy);
+
+	msleep(150);
+	rx_ctl = asix_read_rx_ctl(dev);
+	dbg("RX_CTL is 0x%04x after software reset", rx_ctl);
+	ret = asix_write_rx_ctl(dev, 0x0000);
+	if (ret < 0)
+		goto out;
+
+	rx_ctl = asix_read_rx_ctl(dev);
+	dbg("RX_CTL is 0x%04x setting to 0x0000", rx_ctl);
+
+	msleep(150);
+
+	ax88172a_nway_reset(dev->net);
+
+	ret = asix_write_cmd(dev, AX_CMD_WRITE_IPG0,
+				AX88772_IPG0_DEFAULT | AX88772_IPG1_DEFAULT,
+				AX88772_IPG2_DEFAULT, 0, NULL);
+	if (ret < 0) {
+		dbg("Write IPG,IPG1,IPG2 failed: %d", ret);
+		goto out;
+	}
+
+	/* Rewrite MAC address */
+	memcpy(data->mac_addr, dev->net->dev_addr, ETH_ALEN);
+	ret = asix_write_cmd(dev, AX_CMD_WRITE_NODE_ID, 0, 0, ETH_ALEN,
+							data->mac_addr);
+	if (ret < 0)
+		goto out;
+
+	/* Set RX_CTL to default values with 2k buffer, and enable cactus */
+	ret = asix_write_rx_ctl(dev, AX_DEFAULT_RX_CTL);
+	if (ret < 0)
+		goto out;
+
+	rx_ctl = asix_read_rx_ctl(dev);
+	dbg("RX_CTL is 0x%04x after all initializations", rx_ctl);
+
+	rx_ctl = asix_read_medium_status(dev);
+	dbg("Medium Status is 0x%04x after all initializations", rx_ctl);
+
+	return 0;
+
+out:
+	return ret;
+
+}
+
+const struct driver_info ax88172a_info = {
+	.description = "ASIX AX88172A USB 2.0 Ethernet",
+	.bind = ax88172a_bind,
+	.unbind = ax88172a_unbind,
+	.status = ax88172a_status,
+	.reset = ax88172a_reset,
+	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_LINK_INTR |
+		 FLAG_MULTI_PACKET,
+	.rx_fixup = asix_rx_fixup,
+	.tx_fixup = asix_tx_fixup,
+};