Re: [net-next PATCH v1 6/7] net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver

[Calvin Johnson]

Hi Grant,

On Fri, Oct 02, 2020 at 12:22:37PM +0100, Grant Likely wrote:
> 
> 
> 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@xxxxxxxxxxx>
> > ---
> > 
> >   .../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?

Yes, the behaviour for ACPI had to be changed as I couldn't find an ACPI method
to find named nodes. I did this change some time back and it didn't work for
ACPI. I'll revisit this once again and keep original code if needed.
Behaviour for DT hasn't changed although the APIs changed.

> 
> I also see that the change to the code has dropped the of_node_put() on the
> exit path.

Sure, I'll fix it.
> 
> > -	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?

Will correct this.

Thanks
Calvin