Re: [PATCH net-next v3 05/13] ax88179_178a: Use phylink in get/set_link_ksettings

On 24/07/2026 20:22, Andrew Lunn wrote:

On Fri, Jul 24, 2026 at 07:02:48PM +0200, Birger Koblitz wrote:

quoted

On 7/24/26 18:25, Andrew Lunn wrote:

quoted

quoted

Well, only the new devices use phylink, so it is not converting the existing
chips.

O.K. I suggested you split the existing driver into two, a library of
code moving frames around, and then the glue to probe and use mii. You
can then add a new glue driver using phylink for the new devices.

I replied to that earlier on. My argument was that the frame moving bits are
rather different since the older devices use 32 bit header structures, while
the newer ones use 64 and the structures also are somewhat different. The
library would contain rather a lot of duplicate functions that are specific to
the AX179 and AX179A.

Are you aware that the asix_devices.c and asix_common.c drivers do already
have this structure (including phylink), but target even older devices? And then,
there is a 3rd implementation of the ASIX frame moving, again a bit different,
in aqc111.c (no phylink). If one changes there aqc111_ to asix_ one gets basically
the same code as in asix_common at least for the USB stuff and similarities to the
code of ax88179_178a for the frame moving. I did not want to further contribute
to the mess, but it now looks like I do just that.

Yes ASIX is a mess. It is lacking a Maintainer to look after it as a
whole, and say No to patches etc.

quoted

To summarize, you suggest to split ax88179_178a plus the new HW code into 3 parts:
- ax179_lib (frame moving, USB read/write support)
- ax179_devices (HW support for AX179-based chips using mii)
- ax179a_devices (HW support for AX179A based chips using phylink)

This seems like the best way to not make ax179 a worse of a mess.

When you look at what is actually needed for ax179a_devices, starting
from ax88179_178a.c and deleting stuff, there is not too much.

static void ax88179_set_pm_mode(struct usbnet *dev, bool pm_mode)
static int ax88179_in_pm(struct usbnet *dev)
static int __ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
			      u16 size, void *data)
static int __ax88179_write_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
			       u16 size, const void *data)
static void ax88179_write_cmd_async(struct usbnet *dev, u8 cmd, u16 value,
				    u16 index, u16 size, void *data)
static int ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
			    u16 size, void *data)
static int ax88179_write_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
			     u16 size, const void *data)

All goes into the library.

I don't think

static void ax88179_status(struct usbnet *dev, struct urb *urb)

does anything useful with phylink.

The _mdio_ and _mmd_ functions all need ax179a_devices specific
versions.

phylink does WoL differently, so you need to look at
static int ax88179_suspend(struct usb_interface *intf, pm_message_t message)

and maybe split it into two and have a ax179a_devices wrapper. Same
for resume.

_set_wol and _get_wol just calls into phylink.

All the eeprom stuff goes into the library.

The ethtool ksetting and eee calls just become calls into phylink.

All the rest of the EEE code is ax179_devices specific.

You need a new ethtool_ops structure.

You have some changes to set_multicast. Maybe it can be shared, maybe
a specific version?

set_features, change_mtu, set_mac_addr all looks like library code.

Probably a new net_device_ops. The usbnet_mii_ioctl call maybe does
the wrong thing with phylink?

More eeprom code for the library.

efuses code for the library

I personally would make the LED code ax179_devices specific. That
would allow you to do a clean implementation of /sys/class LEDs.

_get_mac_addr() in the library.

_bind() and _unbind() is clearly ax179a_devices specific

_rx_checksum(), _rx_fixup(), _tx_fixup looks to be library.

_link_reset() needs closely looking at, there seems to be some PHY
bits in there.

_reset() needs splitting so there is a ax179a_devices specific part.
_net_reset() as well.

That sounds like a plan. Thanks a lot for providing your insights!
I'll work on with that and send out a v4 when done.

Birger