| Message ID | 20240103025334.541682-1-yajun.deng@linux.dev (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: phy: Use is_phy_driver() and is_phy_device() | expand |
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
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_.
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!
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(-)