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

[Birger Koblitz <mail@birger-koblitz.de>]

On 31/07/2026 21:37, Andrew Lunn wrote:
>> +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.
Will fix in v5, this was code from the original driver.

> 
>> +
>> +	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.
The local copy is not needed. This was also C&P from the original driver.
Fixed in v5.

> 
>> +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.
Will introduce ntdev2data() in v5.

>> +
>> +	netdev_info(dev->net, "ax88179a - Link status is: 1, Link speed: %d, Duplex: %d\n",
>> +		    speed, duplex);
>> +}
> 
> Maybe not needed? Does phylink print something?
Removed in v5, phylink prints the same information.

> 
>> +static int ax88179a_init_mdio(struct usbnet *dev)
>> +	netdev_err(dev->net, "Registering MDIO bus\n");
> 
> Left over debug.
Removed in v5.

> 
>> +	/* 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?
Ooops, forgot to remove this. Removed in v5.

> 
>> +static int ax88179a_reset(struct usbnet *dev)
>> +	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.
Fixed in v5.

> 
>> +	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.
Removed in v5.