mbox series

[net-next,v2,0/2] net: phy: Use is_phy_driver() and is_phy_device()

Message ID 20240103025334.541682-1-yajun.deng@linux.dev
Headers
Series net: phy: Use is_phy_driver() and is_phy_device() |

Message

Yajun Deng Jan. 3, 2024, 2:53 a.m. UTC 
  There is only one flag that can be set in struct mdio_driver_common and
mdio_device. We can compare the probe of the driver or the type of the
device to implement it. Hence, these flags in struct mdio_driver_common
and mdio_device can be removed.

Introduce is_phy_driver() and is_phy_device(). Use them test the driver
or device.

v1 -> v2:
remove redundant parens and the exported.

Yajun Deng (2):
  net: phy: Cleanup struct mdio_driver_common and introduce
    is_phy_driver()
  net: phy: Introduce is_phy_device()

 drivers/net/dsa/b53/b53_mdio.c          |  2 +-
 drivers/net/dsa/dsa_loop.c              |  2 +-
 drivers/net/dsa/lan9303_mdio.c          |  2 +-
 drivers/net/dsa/microchip/ksz8863_smi.c |  2 +-
 drivers/net/dsa/mt7530-mdio.c           |  2 +-
 drivers/net/dsa/mv88e6060.c             |  2 +-
 drivers/net/dsa/mv88e6xxx/chip.c        |  2 +-
 drivers/net/dsa/qca/ar9331.c            |  2 +-
 drivers/net/dsa/qca/qca8k-8xxx.c        |  2 +-
 drivers/net/dsa/realtek/realtek-mdio.c  |  2 +-
 drivers/net/dsa/xrs700x/xrs700x_mdio.c  |  2 +-
 drivers/net/phy/mdio_bus.c              |  7 ++--
 drivers/net/phy/mdio_device.c           | 21 +++++-------
 drivers/net/phy/phy_device.c            | 44 ++++++++++++++-----------
 drivers/net/phy/xilinx_gmii2rgmii.c     |  2 +-
 drivers/phy/broadcom/phy-bcm-ns-usb3.c  |  8 ++---
 drivers/phy/broadcom/phy-bcm-ns2-pcie.c |  8 ++---
 include/linux/mdio.h                    | 19 ++---------
 include/linux/phy.h                     | 10 +++---
 19 files changed, 62 insertions(+), 79 deletions(-)

Comments

Andrew Lunn Jan. 3, 2024, 1:54 p.m. UTC | #1 
On Wed, Jan 03, 2024 at 10:53:32AM +0800, Yajun Deng wrote:
> There is only one flag that can be set in struct mdio_driver_common and
> mdio_device. We can compare the probe of the driver or the type of the
> device to implement it. Hence, these flags in struct mdio_driver_common
> and mdio_device can be removed.
> 
> Introduce is_phy_driver() and is_phy_device(). Use them test the driver
> or device.

I'm still not convinced this is useful. Please expand your commit
message.  One things which might convince me this is useful is if the
PHY drivers can make there struct phy_driver structures const.

Also, please break this patch series up. You should be able to add the
helper is_phy_driver() and make use of it, without changing
common. You should be able to add is_phy_device() without changing
common. So do these little steps first. The current code is hard to
review because these changes are all mixed in with everything
else. Once you have done the preparation steps, you can then do the
mass change.

    Andrew

---
pw-bot: cr
Russell King (Oracle) Jan. 3, 2024, 2 p.m. UTC | #2 
On Wed, Jan 03, 2024 at 10:53:32AM +0800, Yajun Deng wrote:
> There is only one flag that can be set in struct mdio_driver_common and
> mdio_device. We can compare the probe of the driver or the type of the
> device to implement it. Hence, these flags in struct mdio_driver_common
> and mdio_device can be removed.
> 
> Introduce is_phy_driver() and is_phy_device(). Use them test the driver
> or device.

It is not a good idea to post a new series while discussion of the first
is still on-going, even if it has been 24 hours since you last posted a
patch. If discussion is still going on, then we don't need the
distraction of yet another series to duplicate the comments to.

I remain completely unconvinced of the merit of these changes. IMHO,
it is pure churn for churn's sake - there is no _real_ benefit. It
doesn't fix a bug. It doesn't make the code easier to read. It only
satisfies some ideological idea that all drivers should look the same.

Unless a very good justification can be found, I am not in favour of
changing these drivers.

There _may_ be good merit in is_phy_driver() and is_phy_device(), and
as Andrew says, that should be done _first_.
Yajun Deng Jan. 4, 2024, 2:02 a.m. UTC | #3 
On 2024/1/3 22:00, Russell King (Oracle) wrote:
> On Wed, Jan 03, 2024 at 10:53:32AM +0800, Yajun Deng wrote:
>> There is only one flag that can be set in struct mdio_driver_common and
>> mdio_device. We can compare the probe of the driver or the type of the
>> device to implement it. Hence, these flags in struct mdio_driver_common
>> and mdio_device can be removed.
>>
>> Introduce is_phy_driver() and is_phy_device(). Use them test the driver
>> or device.
> It is not a good idea to post a new series while discussion of the first
> is still on-going, even if it has been 24 hours since you last posted a
> patch. If discussion is still going on, then we don't need the
> distraction of yet another series to duplicate the comments to.
>
> I remain completely unconvinced of the merit of these changes. IMHO,
> it is pure churn for churn's sake - there is no _real_ benefit. It
> doesn't fix a bug. It doesn't make the code easier to read. It only
> satisfies some ideological idea that all drivers should look the same.
>
> Unless a very good justification can be found, I am not in favour of
> changing these drivers.
>
> There _may_ be good merit in is_phy_driver() and is_phy_device(), and
> as Andrew says, that should be done _first_.
>
Ok, I got it, thanks!