Messages in this thread |  | | From | Andy Shevchenko <> | Date | Mon, 8 Feb 2021 18:16:24 +0200 | Subject | Re: [net-next PATCH v5 10/15] net: mdio: Add ACPI support code for mdio |
| |
On Mon, Feb 8, 2021 at 5:14 PM Calvin Johnson <calvin.johnson@oss.nxp.com> wrote: > > Define acpi_mdiobus_register() to Register mii_bus and create PHYs for > each ACPI child node.
...
> +/** > + * acpi_mdiobus_register - Register mii_bus and create PHYs from the ACPI ASL.
> + *
Redundant blank line.
> + * @mdio: pointer to mii_bus structure > + * @fwnode: pointer to fwnode of MDIO bus. > + * > + * This function registers the mii_bus structure and registers a phy_device > + * for each child node of @fwnode. > + */ > +int acpi_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle *fwnode) > +{ > + struct fwnode_handle *child; > + u32 addr; > + int ret; > + > + /* Mask out all PHYs from auto probing. */
> + mdio->phy_mask = ~0;
I would rather see GENMASK(31, 0) here because in case the type of the variable is changed we will need to amend this anyway.
> + ret = mdiobus_register(mdio); > + if (ret) > + return ret;
> + mdio->dev.fwnode = fwnode;
Shouldn't it be rather ACPI_SET_COMPANION() as other bus / drivers do?
> +/* Loop over the child nodes and register a phy_device for each PHY */
Indentation.
> + fwnode_for_each_child_node(fwnode, child) { > + ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
> + if ((ret) || addr >= PHY_MAX_ADDR)
Too many parentheses.
> + continue; > + > + ret = fwnode_mdiobus_register_phy(mdio, child, addr); > + if (ret == -ENODEV) > + dev_err(&mdio->dev, > + "MDIO device at address %d is missing.\n", > + addr); > + } > + return 0; > +}
...
> +/* > + * ACPI helpers for the MDIO (Ethernet PHY) API > + * > + */
It's one line AFAICT!
...
> +#include <linux/device.h> > +#include <linux/phy.h>
This seems a bit inconsistent with the below. I see the user of mdiobus_register(). It's the only header should be included. Everything else would be forward declared like
struct fwnode_handle;
> +#if IS_ENABLED(CONFIG_ACPI_MDIO) > +int acpi_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle *fwnode); > +#else /* CONFIG_ACPI_MDIO */ > +static inline int acpi_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle *fwnode) > +{ > + /* > + * Fall back to mdiobus_register() function to register a bus. > + * This way, we don't have to keep compat bits around in drivers. > + */ > + > + return mdiobus_register(mdio); > +} > +#endif
-- With Best Regards, Andy Shevchenko
|  |