Re: [PATCH net-next v4 03/12] ax88179_178a: Add HW support for AX179A-based chips

[Andrew Lunn]

> +static int ax88179_mdiobus_write(struct mii_bus *bus, int phy_id, int regnum, u16 val)
> +{
> +	struct usbnet *dev = bus->priv;
> +	u16 res = (u16)val;

val already is a u16, so the cast is not needed.

> +
> +	return ax88179_write_cmd(dev, AX_ACCESS_PHY, phy_id, (__u16)regnum, 2, &res);

Does ax88179_write_cmd actually modify the res value? I'm just
wondering why you need the local copy.

> +static void ax88179a_get_pauseparam(struct net_device *net, struct ethtool_pauseparam *pause)
> +{
> +	struct usbnet *dev = netdev_priv(net);
> +	struct ax88179_data *data;
> +
> +	data = dev->driver_priv;

This pattern happens enough that it is worth adding a helper,
netdev2data() or something like that.

> +static void ax88179a_mac_link_up(struct phylink_config *config,
> +				 struct phy_device *phy,
> +				 unsigned int phy_mode, phy_interface_t interface,
> +				 int speed, int duplex,
> +				 bool tx_pause, bool rx_pause)
> +{

> +
> +	netdev_info(dev->net, "ax88179a - Link status is: 1, Link speed: %d, Duplex: %d\n",
> +		    speed, duplex);
> +}

Maybe not needed? Does phylink print something?

> +static int ax88179a_init_mdio(struct usbnet *dev)
> +{
> +	struct ax88179_data *data = dev->driver_priv;
> +	int ret;
> +
> +	data->mdio = mdiobus_alloc();
> +	if (!data->mdio)
> +		return -ENOMEM;
> +
> +	data->mdio->priv = dev;
> +	data->mdio->read = ax88179_mdiobus_read;
> +	data->mdio->write = ax88179_mdiobus_write;
> +	data->mdio->read_c45 = ax88179_mdiobus_read_c45;
> +	data->mdio->write_c45 = ax88179_mdiobus_write_c45;
> +	data->mdio->name = "AX88179A MDIO Bus";
> +	data->mdio->phy_mask = ~(1 << AX88179_PHY_ID);
> +	/* mii bus name is usb-<usb bus number>-<usb device number> */
> +	snprintf(data->mdio->id, MII_BUS_ID_SIZE, "usb-%03d:%03d",
> +		 dev->udev->bus->busnum, dev->udev->devnum);
> +
> +	netdev_err(dev->net, "Registering MDIO bus\n");

Left over debug.

> +	/* Initialize MII structure */
> +	dev->mii.dev = dev->net;
> +	dev->mii.mdio_read = ax88179_mdio_read;
> +	dev->mii.mdio_write = ax88179_mdio_write;
> +	dev->mii.phy_id_mask = 0xff;
> +	dev->mii.reg_num_mask = 0xff;
> +	dev->mii.phy_id = AX88179_PHY_ID;

So the aim is this is removed. Do the follow up patches remove it?

> +static int ax88179a_reset(struct usbnet *dev)
> +{
> +	struct ax88179_data *ax179_data = dev->driver_priv;
> +	u16 *tmp16;
> +	u8 buf[5];
> +	u8 *tmp;
> +
> +	tmp16 = (u16 *)buf;
> +	tmp = (u8 *)buf;
> +
> +	/* Power up ethernet PHY */
> +	*tmp = AX_PHY_POWER;
> +	ax88179_write_cmd(dev, AX88179A_PHY_POWER, 0, 0, 1, tmp);
> +	msleep(250);
> +
> +	if (ax179_data->chip_version == AX_VERSION_AX88279) {
> +		*tmp16 = ax88179_mdio_read(dev->net, dev->mii.phy_id, MII_ADVERTISE);
> +		*tmp16 &= ~(ADVERTISE_10FULL | ADVERTISE_10HALF);
> +		*tmp16 |= AX_ADVERTISE_2500;
> +		ax88179_mdio_write(dev->net, dev->mii.phy_id, MII_ADVERTISE, *tmp16);
> +	}

If the PHY driver is doing the correct thing, this is not needed.  So
long as you tell phylink that 10Mbps is not supported by the MAC, it
should disable the advertisement of 10Mps link modes.

> +	ax179_data->eee_enabled = 0;
> +	ax179_data->eee_active = 0;

Are these used anywhere? Phylink should be tracking the EEE state, not
the MAC driver.

Overall, this is looking a lot better.

Thanks for continuing to work on this.

       Andrew