On Thu, Jul 9, 2020 at 8:57 PM Calvin Johnson
<calvin.johnson@xxxxxxxxxxx> wrote:
>
> Introduce device_mdiobus_register() to register mdiobus
> in cases of either DT or ACPI.
...
> +/**
> + * device_mdiobus_register - bring up all the PHYs on a given bus and
> + * attach them to bus. This handles both DT and ACPI methods.
This is usually one line summary and description goes...
> + * @bus: target mii_bus
> + * @dev: given MDIO device
> + *
...somewhere here.
> + * Returns 0 on success or < 0 on error.
This would be nicer to read as '...or negative error code' or alike.
> + */
> +int device_mdiobus_register(struct mii_bus *bus,
> + struct device *dev)
> +{
> + if (dev->of_node) {
> + return of_mdiobus_register(bus, dev->of_node);
> + } else if (dev_fwnode(dev)) {
> + bus->dev.fwnode = dev_fwnode(dev);
> + return mdiobus_register(bus);
All these 'else' are redundant, but the main confusion here is the use
of dev_fwnode() vs. dev->of_node.
I would rather see something like
struct fwnode_handle *fwnode = dev_fwnode(dev);
...
if (is_of_node(fwnode))
return ...(..., to_of_node(fwnode));
if (fwnode) {
...
return ...
}
return -ENODEV;
(Okay, 'else':s may be left if you think it's better to read)
> + } else {
> + return -ENODEV;
> + }
> +}
--
With Best Regards,
Andy Shevchenko
