| Message ID | 20200701061233.31120-4-calvin.johnson@oss.nxp.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next,v2,1/3] net: phy: introduce find_phy_device() | expand |
On Wed, Jul 1, 2020 at 9:13 AM Calvin Johnson <calvin.johnson@oss.nxp.com> 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. > Define and use helper function find_phy_device() to find phy_dev > that is later connected to mac->phylink. ... > #include "dpaa2-eth.h" > #include "dpaa2-mac.h" > +#include <linux/acpi.h> > +#include <linux/platform_device.h> Can we put (more) generic headers atop of (more) private ones? ... > + struct fwnode_handle *fsl_mc_fwnode = dev->parent->parent->fwnode; dev_fwnode() please. > + struct fwnode_handle *dpmacs, *dpmac = NULL; > + struct device *fsl_mc = dev->parent->parent; So. something like struct device *fsl_mc = dev->parent->parent; struct fwnode_handle *fsl_mc_fwnode = dev_fwnode(fsl_mc); ... > + dpmacs = device_get_named_child_node(fsl_mc, "dpmacs"); If you have fwnode, why to use device_* API? dpmacs = fwnode_get_named_child_node(fsl_mc_fwnode, "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; > + } ... > + } else if (is_acpi_node(fsl_mc_fwnode)) { is_acpi_device_node() ? > + adev = acpi_find_child_device(ACPI_COMPANION(dev->parent), > + dpmac_id, false); > + if (adev) > + return (&adev->fwnode); No need to have parentheses. Don't we have some special macro to get fwnode out of ACPI device? ... > + err = fwnode_get_phy_mode(dpmac_node); > + if (err > 0) > + return err; Positive?! Why? What's going on here? ... > + if (is_of_node(dpmac_node)) > + err = phylink_of_phy_connect(mac->phylink, > + to_of_node(dpmac_node), 0); > + else if (is_acpi_node(dpmac_node)) { > + phy_dev = find_phy_device(dpmac_node); > + if (IS_ERR(phy_dev)) > + goto err_phylink_destroy; > + err = phylink_connect_phy(mac->phylink, phy_dev); Can't you rather provide phylink_fwnode_connect_phy API and drop this conditional tree entirely? ... > + if (is_of_node(dpmac_node)) Redundant. > + of_node_put(to_of_node(dpmac_node)); Honestly, looking at this code, I think one needs a bit more time to get into fwnode paradigm and APIs. ... > + if (is_of_node(dpmac_node)) Ditto. > + of_node_put(to_of_node(dpmac_node));
> Subject: [net-next PATCH v2 3/3] net: dpaa2-mac: Add ACPI support for DPAA2 > MAC driver > > 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. > Define and use helper function find_phy_device() to find phy_dev that is later > connected to mac->phylink. > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> > > --- > > Changes in v2: > - clean up dpaa2_mac_get_node() > - introduce find_phy_device() > - use acpi_find_child_device() > > .../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 3ee236c5fc37..78e8160c9b52 100644 > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c > @@ -3,6 +3,8 @@ > > #include "dpaa2-eth.h" > #include "dpaa2-mac.h" > +#include <linux/acpi.h> > +#include <linux/platform_device.h> > > #define phylink_to_dpaa2_mac(config) \ > container_of((config), struct dpaa2_mac, phylink_config) @@ -23,38 > +25,46 @@ 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 fwnode_handle *fsl_mc_fwnode = dev->parent->parent- > >fwnode; > + struct fwnode_handle *dpmacs, *dpmac = NULL; > + struct device *fsl_mc = dev->parent->parent; > + struct acpi_device *adev; > int err; > + u32 id; > > - dpmacs = of_find_node_by_name(NULL, "dpmacs"); > - if (!dpmacs) > - return NULL; > - > - 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; > + if (is_of_node(fsl_mc_fwnode)) { > + dpmacs = device_get_named_child_node(fsl_mc, "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; > + } > + } else if (is_acpi_node(fsl_mc_fwnode)) { > + adev = acpi_find_child_device(ACPI_COMPANION(dev->parent), > + dpmac_id, false); > + if (adev) > + return (&adev->fwnode); > } > - > - of_node_put(dpmacs); > - This of_node_put() on the 'dpmacs' node still needs to happen for the OF case. Ioana > - 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; > > err = phy_mode(attr.eth_if, &if_mode); > if (!err) > @@ -231,7 +241,8 @@ 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 phy_device *phy_dev; > struct phylink *phylink; > struct dpmac_attr attr; > int err; > @@ -251,7 +262,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(&dpmac_dev->dev, attr.id); > if (!dpmac_node) { > netdev_err(net_dev, "No dpmac@%d node found.\n", attr.id); > err = -ENODEV; > @@ -269,7 +280,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)) { @@ - > 282,7 +293,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); > @@ -290,20 +301,30 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac) > } > mac->phylink = phylink; > > - err = phylink_of_phy_connect(mac->phylink, dpmac_node, 0); > + if (is_of_node(dpmac_node)) > + err = phylink_of_phy_connect(mac->phylink, > + to_of_node(dpmac_node), 0); > + else if (is_acpi_node(dpmac_node)) { > + phy_dev = find_phy_device(dpmac_node); > + if (IS_ERR(phy_dev)) > + goto err_phylink_destroy; > + err = phylink_connect_phy(mac->phylink, phy_dev); > + } > 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; > > err_phylink_destroy: > phylink_destroy(mac->phylink); > 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; > -- > 2.17.1
On Thu, Jul 2, 2020 at 11:48 AM Ioana Ciornei <ioana.ciornei@nxp.com> wrote: > > > Subject: [net-next PATCH v2 3/3] net: dpaa2-mac: Add ACPI support for DPAA2 > > MAC driver > > > > 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. > > Define and use helper function find_phy_device() to find phy_dev that is later > > connected to mac->phylink. ... > > - 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; > > + if (is_of_node(fsl_mc_fwnode)) { > > + dpmacs = device_get_named_child_node(fsl_mc, "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; > > + } > > + } else if (is_acpi_node(fsl_mc_fwnode)) { > > + adev = acpi_find_child_device(ACPI_COMPANION(dev->parent), > > + dpmac_id, false); > > + if (adev) > > + return (&adev->fwnode); > > } > > - > > - of_node_put(dpmacs); > > - > > This of_node_put() on the 'dpmacs' node still needs to happen for the OF case. Actually this also raises the question if ACPI case increases refcount or not and it should be fixed accordingly (Note, we have to take reference to fwnode before return in ACPI case and drop reference to adev).
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c index 3ee236c5fc37..78e8160c9b52 100644 --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c @@ -3,6 +3,8 @@ #include "dpaa2-eth.h" #include "dpaa2-mac.h" +#include <linux/acpi.h> +#include <linux/platform_device.h> #define phylink_to_dpaa2_mac(config) \ container_of((config), struct dpaa2_mac, phylink_config) @@ -23,38 +25,46 @@ 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 fwnode_handle *fsl_mc_fwnode = dev->parent->parent->fwnode; + struct fwnode_handle *dpmacs, *dpmac = NULL; + struct device *fsl_mc = dev->parent->parent; + struct acpi_device *adev; int err; + u32 id; - dpmacs = of_find_node_by_name(NULL, "dpmacs"); - if (!dpmacs) - return NULL; - - 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; + if (is_of_node(fsl_mc_fwnode)) { + dpmacs = device_get_named_child_node(fsl_mc, "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; + } + } else if (is_acpi_node(fsl_mc_fwnode)) { + adev = acpi_find_child_device(ACPI_COMPANION(dev->parent), + dpmac_id, false); + if (adev) + return (&adev->fwnode); } - - 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; err = phy_mode(attr.eth_if, &if_mode); if (!err) @@ -231,7 +241,8 @@ 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 phy_device *phy_dev; struct phylink *phylink; struct dpmac_attr attr; int err; @@ -251,7 +262,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(&dpmac_dev->dev, attr.id); if (!dpmac_node) { netdev_err(net_dev, "No dpmac@%d node found.\n", attr.id); err = -ENODEV; @@ -269,7 +280,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)) { @@ -282,7 +293,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); @@ -290,20 +301,30 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac) } mac->phylink = phylink; - err = phylink_of_phy_connect(mac->phylink, dpmac_node, 0); + if (is_of_node(dpmac_node)) + err = phylink_of_phy_connect(mac->phylink, + to_of_node(dpmac_node), 0); + else if (is_acpi_node(dpmac_node)) { + phy_dev = find_phy_device(dpmac_node); + if (IS_ERR(phy_dev)) + goto err_phylink_destroy; + err = phylink_connect_phy(mac->phylink, phy_dev); + } 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; err_phylink_destroy: phylink_destroy(mac->phylink); 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;
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. Define and use helper function find_phy_device() to find phy_dev that is later connected to mac->phylink. Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> --- Changes in v2: - clean up dpaa2_mac_get_node() - introduce find_phy_device() - use acpi_find_child_device() .../net/ethernet/freescale/dpaa2/dpaa2-mac.c | 79 ++++++++++++------- 1 file changed, 50 insertions(+), 29 deletions(-)