mbox series

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

Message ID 20240217194116.8565-1-ansuelsmth@gmail.com
Headers
Series net: phy: detach PHY driver OPs from phy_driver struct |

Message

Christian Marangi Feb. 17, 2024, 7:41 p.m. UTC 
  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.

Other solution to this problem were to introduce additional
variables to phy_driver struct but that would have resulted
in having 2 different way to do the same thing and that is not O.K.

I took care to compile-test all the PHY, only exception is the unique
RUST driver, where I still have to learn that funny language and
I didn't had time to update it, so that is the only driver that
I think require some fixup.

I posted 2 example that would benefits from this change, but I can
find much more in other PHY driver.

Christian Marangi (3):
  net: phy: detach PHY driver OPs from phy_driver struct
  net: phy: aquantia: use common OPs for PHYs where possible
  net: phy: bcm7xxx: use common OPs for PHYs where possible

 drivers/net/phy/adin.c                   |   4 +
 drivers/net/phy/adin1100.c               |   2 +
 drivers/net/phy/amd.c                    |   4 +
 drivers/net/phy/aquantia/aquantia_main.c | 189 +++++++++--------------
 drivers/net/phy/ax88796b.c               |   6 +
 drivers/net/phy/bcm-cygnus.c             |   4 +
 drivers/net/phy/bcm54140.c               |   2 +
 drivers/net/phy/bcm63xx.c                |   4 +
 drivers/net/phy/bcm7xxx.c                |  72 +++++----
 drivers/net/phy/bcm84881.c               |   2 +
 drivers/net/phy/bcm87xx.c                |   4 +
 drivers/net/phy/broadcom.c               |  42 +++++
 drivers/net/phy/cicada.c                 |   4 +
 drivers/net/phy/cortina.c                |   2 +
 drivers/net/phy/davicom.c                |   8 +
 drivers/net/phy/dp83640.c                |   2 +
 drivers/net/phy/dp83822.c                |   8 +-
 drivers/net/phy/dp83848.c                |   2 +
 drivers/net/phy/dp83867.c                |   2 +
 drivers/net/phy/dp83869.c                |   2 +
 drivers/net/phy/dp83tc811.c              |   2 +
 drivers/net/phy/dp83td510.c              |   2 +
 drivers/net/phy/dp83tg720.c              |   2 +
 drivers/net/phy/et1011c.c                |   2 +
 drivers/net/phy/icplus.c                 |   8 +
 drivers/net/phy/intel-xway.c             |  20 +++
 drivers/net/phy/lxt.c                    |   8 +
 drivers/net/phy/marvell-88q2xxx.c        |   2 +
 drivers/net/phy/marvell-88x2222.c        |   2 +
 drivers/net/phy/marvell.c                |  44 +++++-
 drivers/net/phy/marvell10g.c             |  16 +-
 drivers/net/phy/mediatek-ge-soc.c        |   4 +
 drivers/net/phy/mediatek-ge.c            |   4 +
 drivers/net/phy/meson-gxl.c              |   4 +
 drivers/net/phy/micrel.c                 |  54 ++++++-
 drivers/net/phy/microchip.c              |   2 +
 drivers/net/phy/microchip_t1.c           |   4 +
 drivers/net/phy/microchip_t1s.c          |   4 +
 drivers/net/phy/motorcomm.c              |   8 +
 drivers/net/phy/mscc/mscc_main.c         |  30 ++++
 drivers/net/phy/mxl-gpy.c                |  24 +++
 drivers/net/phy/national.c               |   2 +
 drivers/net/phy/ncn26000.c               |   2 +
 drivers/net/phy/nxp-c45-tja11xx.c        |   8 +-
 drivers/net/phy/nxp-cbtx.c               |   2 +
 drivers/net/phy/nxp-tja11xx.c            |  16 +-
 drivers/net/phy/phy-c45.c                |   2 +
 drivers/net/phy/phy-core.c               |  18 ++-
 drivers/net/phy/phy.c                    |  85 +++++-----
 drivers/net/phy/phy_device.c             |  79 +++++-----
 drivers/net/phy/qcom/at803x.c            |  18 ++-
 drivers/net/phy/qcom/qca807x.c           |   4 +
 drivers/net/phy/qcom/qca808x.c           |   2 +
 drivers/net/phy/qcom/qca83xx.c           |  12 +-
 drivers/net/phy/qsemi.c                  |   2 +
 drivers/net/phy/realtek.c                |  46 +++++-
 drivers/net/phy/rockchip.c               |   2 +
 drivers/net/phy/smsc.c                   |  14 ++
 drivers/net/phy/ste10Xp.c                |   4 +
 drivers/net/phy/teranetics.c             |   2 +
 drivers/net/phy/uPD60620.c               |   2 +
 drivers/net/phy/vitesse.c                |  22 +++
 drivers/net/phy/xilinx_gmii2rgmii.c      |  16 +-
 include/linux/phy.h                      |  57 ++++---
 64 files changed, 737 insertions(+), 291 deletions(-)

Comments

Andrew Lunn Feb. 17, 2024, 11:48 p.m. UTC | #1 
> 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.

Yes, the problem is, it probably needs another 50 patches to remove
all the duplication. I have to question, is that really going to
happen? Are you going to keep working on this until every driver has
its duplicates removed?

It probably needs some tooling to help. Something which can decode the
object file, and tell you which ops structures are identical. That can
then guide you when editing all the PHY drivers.

     Andrew