Re: [PATCH net-next v5 1/4] net: mdio: common handling of phy reset properties

[Buday Csaba <buday.csaba@prolan.hu>]

On Wed, Oct 29, 2025 at 02:03:30PM +0100, Andrew Lunn wrote:
> On Wed, Oct 29, 2025 at 11:23:41AM +0100, Buday Csaba wrote:
> > Unify the handling of reset properties for an `mdio_device`.
> > Replace mdiobus_register_gpiod() and mdiobus_register_reset() with
> > mdio_device_register_reset() and mdio_device_unregister_reset(),
> > and move them from mdio_bus.c to mdio_device.c, where they belong.
> 
> You should probably mention here that there are two sets of reset
> properties. One set applies to the bus as a whole, and the second is
> per device on the bus. You would expect the whole bus properties to be
> handled in mdio_bus.c, where as the per device properties might make
> more sense in mdio_device.c. So you can comment you are only moving
> the per device reset code.
> 

Okay, I will do that.

> > 
> > The new functions handle both reset-controllers and reset-gpios,
> > and also read the corresponding firmware properties from the
> > device tree, which were previously handled in fwnode_mdio.c.
> > This makes tracking the reset properties easier.
> 
> Please split this patch.
> 
> It is normal when moving a function to just move it, make no
> additional changes. That makes the change smaller, easier to review,
> since it is all about, does the new location make sense?
> 
> Once it is in the new location, you can then have patches which
> refactor it.

Understood, I will change it accordingly.

> 
> > -static int mdiobus_register_gpiod(struct mdio_device *mdiodev)
> > -{
> > -	/* Deassert the optional reset signal */
> > -	mdiodev->reset_gpio = gpiod_get_optional(&mdiodev->dev,
> > -						 "reset", GPIOD_OUT_LOW);
> > -	if (IS_ERR(mdiodev->reset_gpio))
> > -		return PTR_ERR(mdiodev->reset_gpio);
> > -
> > -	if (mdiodev->reset_gpio)
> > -		gpiod_set_consumer_name(mdiodev->reset_gpio, "PHY reset");
> > -
> > -	return 0;
> > -}
> > -
> 
> > +/**
> > + * mdio_device_register_reset - Read and initialize the reset properties of
> > + *				an mdio device
> > + * @mdiodev: mdio_device structure
> > + *
> > + * Return: Zero if successful, negative error code on failure
> > + */
> > +int mdio_device_register_reset(struct mdio_device *mdiodev)
> > +{
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(mdio_device_register_reset);
> 
> Before it did not require an EXPORT_SYMBOL, but now it does?  That
> makes me wounder if it is in the correct place. This should be a core
> function, why would something outside of the core need it?

I am using these in the third patch in fwnode_mdio.c, so I assumed it was
necessary. But you are right, neither can be in a module. I will remove it.

> 
> Also, please use the _GPL variant.
> 
> 	Andrew
> 

Csaba