Messages in this thread |  | | Subject | Re: [net-next PATCH v1 6/7] net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver | From | Grant Likely <> | Date | Fri, 2 Oct 2020 12:22:37 +0100 |
| |
On 30/09/2020 17:04, Calvin Johnson wrote: > Modify dpaa2_mac_connect() to support ACPI along with DT. > Modify dpaa2_mac_get_node() to get the dpmac fwnode from either > DT or ACPI. > > Replace of_get_phy_mode with fwnode_get_phy_mode to get > phy-mode for a dpmac_node. > > Use helper function phylink_fwnode_phy_connect() to find phy_dev and > connect to mac->phylink. > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> > --- > > .../net/ethernet/freescale/dpaa2/dpaa2-mac.c | 79 ++++++++++++------- > 1 file changed, 50 insertions(+), 29 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c > index 90cd243070d7..18502ee83e46 100644 > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c > @@ -3,6 +3,7 @@ > > #include "dpaa2-eth.h" > #include "dpaa2-mac.h" > +#include <linux/acpi.h> > > #define phylink_to_dpaa2_mac(config) \ > container_of((config), struct dpaa2_mac, phylink_config) > @@ -35,38 +36,56 @@ static int phy_mode(enum dpmac_eth_if eth_if, phy_interface_t *if_mode) > } > > /* Caller must call of_node_put on the returned value */ > -static struct device_node *dpaa2_mac_get_node(u16 dpmac_id) > +static struct fwnode_handle *dpaa2_mac_get_node(struct device *dev, > + u16 dpmac_id) > { > - struct device_node *dpmacs, *dpmac = NULL; > - u32 id; > + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; > + struct fwnode_handle *dpmacs, *dpmac = NULL; > + unsigned long long adr; > + acpi_status status; > int err; > + u32 id; > > - dpmacs = of_find_node_by_name(NULL, "dpmacs"); > - if (!dpmacs) > - return NULL; > + if (is_of_node(dev->parent->fwnode)) { > + dpmacs = device_get_named_child_node(dev->parent, "dpmacs"); > + if (!dpmacs) > + return NULL; > + > + while ((dpmac = fwnode_get_next_child_node(dpmacs, dpmac))) { > + err = fwnode_property_read_u32(dpmac, "reg", &id); > + if (err) > + continue; > + if (id == dpmac_id) > + return dpmac; > + } There is a change of behaviour here that isn't described in the patch description, so I'm having trouble following the intent. The original code searches the tree for a node named "dpmacs", but the new code constrains the search to just the parent device.
Also, because the new code path is only used in the DT case, I don't see why the behaviour change is required. If it is a bug fix, it should be broken out into a separate patch. If it is something else, can you describe what the reasoning is?
I also see that the change to the code has dropped the of_node_put() on the exit path.
> > - while ((dpmac = of_get_next_child(dpmacs, dpmac)) != NULL) { > - err = of_property_read_u32(dpmac, "reg", &id); > - if (err) > - continue; > - if (id == dpmac_id) > - break; > + } else if (is_acpi_node(dev->parent->fwnode)) { > + device_for_each_child_node(dev->parent, dpmac) { > + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(dpmac), > + "_ADR", NULL, &adr); > + if (ACPI_FAILURE(status)) { > + pr_debug("_ADR returned %d on %s\n", > + status, (char *)buffer.pointer); > + continue; > + } else { > + id = (u32)adr; > + if (id == dpmac_id) > + return dpmac; > + } > + } > } > - > - of_node_put(dpmacs); > - > - return dpmac; > + return NULL; > } > > -static int dpaa2_mac_get_if_mode(struct device_node *node, > +static int dpaa2_mac_get_if_mode(struct fwnode_handle *dpmac_node, > struct dpmac_attr attr) > { > phy_interface_t if_mode; > int err; > > - err = of_get_phy_mode(node, &if_mode); > - if (!err) > - return if_mode; > + err = fwnode_get_phy_mode(dpmac_node); > + if (err > 0) > + return err;
Is this correct? The function prototype from patch 2 is:
struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
It returns struct fwnode_handle *. Does this compile?
> > err = phy_mode(attr.eth_if, &if_mode); > if (!err) > @@ -303,7 +322,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac) > { > struct fsl_mc_device *dpmac_dev = mac->mc_dev; > struct net_device *net_dev = mac->net_dev; > - struct device_node *dpmac_node; > + struct fwnode_handle *dpmac_node = NULL; > struct phylink *phylink; > struct dpmac_attr attr; > int err; > @@ -323,7 +342,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac) > > mac->if_link_type = attr.link_type; > > - dpmac_node = dpaa2_mac_get_node(attr.id); > + dpmac_node = dpaa2_mac_get_node(&mac->mc_dev->dev, attr.id); > if (!dpmac_node) { > netdev_err(net_dev, "No dpmac@%d node found.\n", attr.id); > err = -ENODEV; > @@ -341,7 +360,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac) > * error out if the interface mode requests them and there is no PHY > * to act upon them > */ > - if (of_phy_is_fixed_link(dpmac_node) && > + if (of_phy_is_fixed_link(to_of_node(dpmac_node)) && > (mac->if_mode == PHY_INTERFACE_MODE_RGMII_ID || > mac->if_mode == PHY_INTERFACE_MODE_RGMII_RXID || > mac->if_mode == PHY_INTERFACE_MODE_RGMII_TXID)) { > @@ -352,7 +371,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac) > > if (attr.link_type == DPMAC_LINK_TYPE_PHY && > attr.eth_if != DPMAC_ETH_IF_RGMII) { > - err = dpaa2_pcs_create(mac, dpmac_node, attr.id); > + err = dpaa2_pcs_create(mac, to_of_node(dpmac_node), attr.id); > if (err) > goto err_put_node; > } > @@ -361,7 +380,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac) > mac->phylink_config.type = PHYLINK_NETDEV; > > phylink = phylink_create(&mac->phylink_config, > - of_fwnode_handle(dpmac_node), mac->if_mode, > + dpmac_node, mac->if_mode, > &dpaa2_mac_phylink_ops); > if (IS_ERR(phylink)) { > err = PTR_ERR(phylink); > @@ -372,13 +391,14 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac) > if (mac->pcs) > phylink_set_pcs(mac->phylink, &mac->pcs->pcs); > > - err = phylink_of_phy_connect(mac->phylink, dpmac_node, 0); > + err = phylink_fwnode_phy_connect(mac->phylink, dpmac_node, 0); > if (err) { > - netdev_err(net_dev, "phylink_of_phy_connect() = %d\n", err); > + netdev_err(net_dev, "phylink_fwnode_phy_connect() = %d\n", err); > goto err_phylink_destroy; > } > > - of_node_put(dpmac_node); > + if (is_of_node(dpmac_node)) > + of_node_put(to_of_node(dpmac_node)); > > return 0; > > @@ -387,7 +407,8 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac) > err_pcs_destroy: > dpaa2_pcs_destroy(mac); > err_put_node: > - of_node_put(dpmac_node); > + if (is_of_node(dpmac_node)) > + of_node_put(to_of_node(dpmac_node)); > err_close_dpmac: > dpmac_close(mac->mc_io, 0, dpmac_dev->mc_handle); > return err; >
|  |