On Monday 10 November 2014 19:47:06 al.stone@linaro.org wrote:
- phy_dev = of_phy_connect(ndev, phy_np, &xgene_enet_adjust_link,
0, pdata->phy_mode);- if (!phy_dev) {
- phy_dev = pdata->phy_dev;
- if (phy_dev == NULL ||
phy_connect_direct(ndev, phy_dev, &xgene_enet_adjust_link,pdata->phy_mode)) {
This looks problematic to me: the existing driver handles arbitrary PHYs, which I assume was intentional, while the ACPI code would only work with the built-in PHY.
This will break as soon as anyone uses this chip with an external PHY.
- ret = device_property_read_u32(dev, "phy-channel", &phy_id);
- if (ret)
return -EINVAL;
Undocumented property?
There is also still the open question on whether it's appropriate to use DT properties on ARM64 servers with ACPI or not.
diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_main.c b/drivers/net/ethernet/apm/xgene/xgene_enet_main.c index 3c208cc..6370ff4 100644 --- a/drivers/net/ethernet/apm/xgene/xgene_enet_main.c +++ b/drivers/net/ethernet/apm/xgene/xgene_enet_main.c @@ -746,6 +746,42 @@ static const struct net_device_ops xgene_ndev_ops = { .ndo_set_mac_address = xgene_enet_set_mac_address, }; +#ifdef CONFIG_ACPI +static int acpi_get_mac_address(struct device *dev,
unsigned char *addr)+{
- int ret;
- ret = device_property_read_u8_array(dev, "mac-address", addr, 6);
- if (ret)
return 0;- return 6;
+}
If you need this, please submit a patch to generalize the of_get_mac_address helper the way that the other properties are handled.
+static int acpi_get_phy_mode(struct device *dev) +{
- int i, ret, phy_mode;
- char *modestr;
- ret = device_property_read_string(dev, "phy-mode", &modestr);
- if (ret)
return -1;- phy_mode = -1;
- for (i = 0; i < PHY_INTERFACE_MODE_MAX; i++) {
if (!strcasecmp(modestr, phy_modes(i))) {phy_mode = i;break;}- }
- return phy_mode;
+} +#else +#define acpi_get_mac_address(a, b, c) 0 +#define acpi_get_phy_mode(a) -1 +#endif
Same here with of_get_phy_mode
static int xgene_enet_get_resources(struct xgene_enet_pdata *pdata) { struct platform_device *pdev; @@ -761,6 +797,8 @@ static int xgene_enet_get_resources(struct xgene_enet_pdata *pdata) ndev = pdata->ndev; res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "enet_csr");
- if (!res)
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
These should really be merged into a single call, either by relying on the order on DT, or by naming the register resources on ACPI.
static void xgene_enet_setup_ops(struct xgene_enet_pdata *pdata) @@ -934,7 +979,7 @@ static int xgene_enet_probe(struct platform_device *pdev) goto err; }
- ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
- ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64)); if (ret) { netdev_err(ndev, "No usable DMA configuration\n"); goto err;
No, you have to fix the ACPI code to set the dma mask pointer correctly for DMA capable devices. dma_coerce_mask_and_coherent is not acceptable for new code.
Arnd