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:3.523 / U:0.136 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site