lkml.org 
[lkml]   [2021]   [Feb]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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

\
 
 \ /
  Last update: 2021-02-08 19:33    [W:0.443 / U:0.788 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site