Re: [net-next PATCH v6 2/6] net: phy: introduce device_mdiobus_register()

[Calvin Johnson <calvin.johnson@oss.nxp.com>]

On Sat, Jul 11, 2020 at 02:36:20PM -0700, Florian Fainelli wrote:
> 
> 
> On 7/10/2020 11:55 PM, Calvin Johnson wrote:
> > Introduce device_mdiobus_register() to register mdiobus
> > in cases of either DT or ACPI.
> > 
> > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> > 
> > ---
> > 
> > Changes in v6:
> > - change device_mdiobus_register() parameter position
> > - improve documentation
> > 
> > Changes in v5:
> > - add description
> > - clean up if else
> > 
> > Changes in v4: None
> > Changes in v3: None
> > Changes in v2: None
> > 
> >  drivers/net/phy/mdio_bus.c | 26 ++++++++++++++++++++++++++
> >  include/linux/mdio.h       |  1 +
> >  2 files changed, 27 insertions(+)
> > 
> > diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> > index 46b33701ad4b..8610f938f81f 100644
> > --- a/drivers/net/phy/mdio_bus.c
> > +++ b/drivers/net/phy/mdio_bus.c
> > @@ -501,6 +501,32 @@ static int mdiobus_create_device(struct mii_bus *bus,
> >  	return ret;
> >  }
> >  
> > +/**
> > + * device_mdiobus_register - register mdiobus for either DT or ACPI
> > + * @bus: target mii_bus
> > + * @dev: given MDIO device
> > + *
> > + * Description: Given an MDIO device and target mii bus, this function
> > + * calls of_mdiobus_register() for DT node and mdiobus_register() in
> > + * case of ACPI.
> > + *
> > + * Returns 0 on success or negative error code on failure.
> > + */
> > +int device_mdiobus_register(struct device *dev,
> > +			    struct mii_bus *bus)
> > +{
> > +	struct fwnode_handle *fwnode = dev_fwnode(dev);
> > +
> > +	if (is_of_node(fwnode))
> > +		return of_mdiobus_register(bus, to_of_node(fwnode));
> > +	if (fwnode) {
> > +		bus->dev.fwnode = fwnode;
> > +		return mdiobus_register(bus);
> > +	}
> > +	return -ENODEV;
> > +}
> > +EXPORT_SYMBOL(device_mdiobus_register);
> > +
> >  /**
> >   * __mdiobus_register - bring up all the PHYs on a given bus and attach them to bus
> >   * @bus: target mii_bus
> > diff --git a/include/linux/mdio.h b/include/linux/mdio.h
> > index 898cbf00332a..f454c5435101 100644
> > --- a/include/linux/mdio.h
> > +++ b/include/linux/mdio.h
> > @@ -358,6 +358,7 @@ static inline int mdiobus_c45_read(struct mii_bus *bus, int prtad, int devad,
> >  	return mdiobus_read(bus, prtad, mdiobus_c45_addr(devad, regnum));
> >  }
> >  
> > +int device_mdiobus_register(struct device *dev, struct mii_bus *bus);
> 
> Humm, this header file does not have any of the mii_bus registration
> functions declared, and it typically pertains to mdio_device instances
> which are devices *on* the mii_bus. phy.h may be more appropriate here
> until we break it up into phy_device proper, mii_bus, etc.
> 
> >  int mdiobus_register_device(struct mdio_device *mdiodev);
> >  int mdiobus_unregister_device(struct mdio_device *mdiodev);
> >  bool mdiobus_is_registered_device(struct mii_bus *bus, int addr);

Although some mii_bus registration functions are declared in phy.h, IMO, it
doesn't seem to be the right place. If we look plainly, phy related things
would be expected in phy.h and mdio related in mdio.h. Maybe as you said
we should consider breaking into phy_device proper, mii_bus, etc. We can take it
up later.

Please let me know if you still want device_mdiobus_register() in phy.h.

Thanks
Calvin