[PATCH] net: mdiobus: Move all reset registration to `mdiobus_register_reset()`

[PATCH] net: mdiobus: Move all reset registration to `mdiobus_register_reset()`

[Bence Csókás]

Make `mdiobus_register_reset()` function handle both gpiod and
reset-controller-based reset registration.

Signed-off-by: Bence Csókás <csokas.bence@prolan.hu>
---
 drivers/net/phy/mdio_bus.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index cad6ed3aa10b643ad63fac15bfe7551446c8dca1..9117f0f93756f38acb2c367e163ef06616eab6e4 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -33,8 +33,10 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/mdio.h>
 
-static int mdiobus_register_gpiod(struct mdio_device *mdiodev)
+static int mdiobus_register_reset(struct mdio_device *mdiodev)
 {
+	struct reset_control *reset;
+
 	/* Deassert the optional reset signal */
 	mdiodev->reset_gpio = gpiod_get_optional(&mdiodev->dev,
 						 "reset", GPIOD_OUT_LOW);
@@ -44,13 +46,6 @@ static int mdiobus_register_gpiod(struct mdio_device *mdiodev)
 	if (mdiodev->reset_gpio)
 		gpiod_set_consumer_name(mdiodev->reset_gpio, "PHY reset");
 
-	return 0;
-}
-
-static int mdiobus_register_reset(struct mdio_device *mdiodev)
-{
-	struct reset_control *reset;
-
 	reset = reset_control_get_optional_exclusive(&mdiodev->dev, "phy");
 	if (IS_ERR(reset))
 		return PTR_ERR(reset);
@@ -68,10 +63,6 @@ int mdiobus_register_device(struct mdio_device *mdiodev)
 		return -EBUSY;
 
 	if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY) {
-		err = mdiobus_register_gpiod(mdiodev);
-		if (err)
-			return err;
-
 		err = mdiobus_register_reset(mdiodev);
 		if (err)
 			return err;

---
base-commit: 1b237f190eb3d36f52dffe07a40b5eb210280e00
change-id: 20250825-b4-phy-rst-mv-prep-09de61d4790f

Best regards,
-- 
Bence Csókás <csokas.bence@prolan.hu>

Re: [PATCH] net: mdiobus: Move all reset registration to `mdiobus_register_reset()`

[Russell King (Oracle)]

On Mon, Aug 25, 2025 at 04:09:34PM +0200, Bence Csókás wrote:
> Make `mdiobus_register_reset()` function handle both gpiod and
> reset-controller-based reset registration.

The commit description should include not only _what_ is being done but
also _why_.

Here's from Documentation/process/submitting-patches.rst:

 Describe your problem.  Whether your patch is a one-line bug fix or
 5000 lines of a new feature, there must be an underlying problem that
 motivated you to do this work.  Convince the reviewer that there is a
 problem worth fixing and that it makes sense for them to read past the
 first paragraph.

Just describing _what_ is being done doesn't do anything to convince
a reviewer that the patch is worth applying.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] net: mdiobus: Move all reset registration to `mdiobus_register_reset()`

[Csókás Bence]

Hi,

On 2025. 08. 25. 16:16, Russell King (Oracle) wrote:
> On Mon, Aug 25, 2025 at 04:09:34PM +0200, Bence Csókás wrote:
>> Make `mdiobus_register_reset()` function handle both gpiod and
>> reset-controller-based reset registration.
> 
> The commit description should include not only _what_ is being done but
> also _why_.

Well, my question was, when I saw this part of code: why have it 
separate? Users shouldn't care whether a device uses gpiod or 
reset-controller when they call `mdio_device_reset()`, so why should 
they have to care here and call two separate register functions, one 
after another? In fact, the whole thing should be moved to mdio_device.c 
honestly. Along with the setting of mdiodev->reset_{,de}assert_delay.

The end goal is fixing this "Can't read PHY ID because the PHY was never 
reset" bug that's been plaguing users for years.

Bence

Re: [PATCH] net: mdiobus: Move all reset registration to `mdiobus_register_reset()`

[Russell King (Oracle)]

On Mon, Aug 25, 2025 at 04:39:12PM +0200, Csókás Bence wrote:
> Hi,
> 
> On 2025. 08. 25. 16:16, Russell King (Oracle) wrote:
> > On Mon, Aug 25, 2025 at 04:09:34PM +0200, Bence Csókás wrote:
> > > Make `mdiobus_register_reset()` function handle both gpiod and
> > > reset-controller-based reset registration.
> > 
> > The commit description should include not only _what_ is being done but
> > also _why_.
> 
> Well, my question was, when I saw this part of code: why have it separate?
> Users shouldn't care whether a device uses gpiod or reset-controller when
> they call `mdio_device_reset()`, so why should they have to care here and
> call two separate register functions, one after another? In fact, the whole
> thing should be moved to mdio_device.c honestly. Along with the setting of
> mdiodev->reset_{,de}assert_delay.
> 
> The end goal is fixing this "Can't read PHY ID because the PHY was never
> reset" bug that's been plaguing users for years.

I wasn't asking for an explanation in reply to my comment. I was
telling you that you had to do something to modify your commit message
to make your patch acceptable.

Now, I could nitpick your "because the PHY was never reset" - that's
untrue. The common problem is the PHY is _held_ in reset mode making
the PHY unresponsive on the MDIO bus.

If your goal is to fix this, then rather than submitting piecemeal
patches with no explanation, I suggest you work on the problem and
come up with a solution as a series of patches (with commit
descriptions that explain _what_ and _why_ changes are being made)
and submit it with a cover message explaining the overall issue
that is being addressed.

That means we can review the patch series as a whole rather than
being drip-fed individual patches, which is going to take a very
long time to make forward progress.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] net: mdiobus: Move all reset registration to `mdiobus_register_reset()`

[Csókás Bence]

Hi,

On 2025. 08. 25. 16:54, Russell King (Oracle) wrote:
> Now, I could nitpick your "because the PHY was never reset" - that's
> untrue. The common problem is the PHY is _held_ in reset mode making
> the PHY unresponsive on the MDIO bus.

In our case, the problem is that refclk is turned off during init, and 
then before PHY probe, it is turned back on, but reset is not being 
asserted.

> If your goal is to fix this, then rather than submitting piecemeal
> patches with no explanation, I suggest you work on the problem and
> come up with a solution as a series of patches (with commit
> descriptions that explain _what_ and _why_ changes are being made)
> and submit it with a cover message explaining the overall issue
> that is being addressed.
> 
> That means we can review the patch series as a whole rather than
> being drip-fed individual patches, which is going to take a very
> long time to make forward progress.

We are still working on the comprehensive solution. I thought that in 
the meantime, these small and lower-risk pieces could be reviewed and 
picked up.

Bence