Re: [net-next RFC PATCH 0/3] net: phy: detach PHY driver OPs from phy_driver struct

[Christian Marangi]

On Sat, Feb 17, 2024 at 11:21:44PM +0100, Andrew Lunn wrote:
> On Sat, Feb 17, 2024 at 08:41:11PM +0100, Christian Marangi wrote:
> > Posting as RFC due to the massive change to a fundamental struct.
> > 
> > While adding some PHY ID for Aquantia, I notice that there is a
> > big problem with duplicating OPs with each PHY.
> > 
> > The original idea to prevent this was to use mask on the PHY ID
> > and identify PHY Family. Problem is that OEM started to use all
> > kind of PHY ID and this is not doable, hence for PHY that have
> > the same OPs, we have to duplicate all of them.
> > 
> > This is present in Aquantia PHY, but is much more present in
> > other PHY, especially in the BCM7XXX where they use a big macro
> > for common PHYs.
> > 
> > To reduce patch delta, I added the additional variable without
> > adding tabs as this would have resulted in a massive patch.
> > Also to have patch bisectable, this change has to be in one go
> > hence I had to use this trick to reduce patch delta.
> 
> To me, this makes the code look ugly. I also expect many text editors
> which understand indentation will be unhappy, saying the indentation is
> wrong.
>

Yes, it was done to limit the patch delta, if I had to account for the
tab for each new section we would be in the order of 2000+ changes I
think.

> >  64 files changed, 737 insertions(+), 291 deletions(-)
> 
> These statistics are not good. If you had deleted more lines than you
> added, then maybe it might be an O.K. idea.
> 
> Sometimes KISS is best.
>

Well IMHO these stats are a bit flawed, the additional code is really
just extra check if ops is defined and the new .ops variable in each
phy_driver.

If you check patch 2 and 3 you can already see some code is removed.
Also while minimal the final .o produced results in bcm7xxx going from
53kb to 45kb and the aquantia driver from 48kb to 45kb.

I understand that this major rework might be too much, so I think the
only way to handle this is with introducing support for declaring
multiple PHY ID for one phy_driver struct.

-- 
	Ansuel