Messages in this thread |  | | From | "Calvin Johnson (OSS)" <> | Subject | RE: [PATCH v1 3/7] net/fsl: add ACPI support for mdio bus | Date | Tue, 4 Feb 2020 07:18:24 +0000 |
| |
> -----Original Message----- > From: Andy Shevchenko <andy.shevchenko@gmail.com> > Subject: Re: [PATCH v1 3/7] net/fsl: add ACPI support for mdio bus > > On Fri, Jan 31, 2020 at 5:37 PM Calvin Johnson <calvin.johnson@nxp.com> > wrote: > > > > From: Calvin Johnson <calvin.johnson@oss.nxp.com> > > > > Add ACPI support for MDIO bus registration while maintaining the > > existing DT support. > > ... > > > - ret = of_address_to_resource(np, 0, &res); > > - if (ret) { > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + if (!res) { > > dev_err(&pdev->dev, "could not obtain address\n"); > > - return ret; > > + return -ENODEV; > > } > > ... > > > - snprintf(bus->id, MII_BUS_ID_SIZE, "%llx", (unsigned long > long)res.start); > > + snprintf(bus->id, MII_BUS_ID_SIZE, "%llx", > > + (unsigned long long)res->start); > > Why this has been touched?
Without this change, I get: --------------------------------------------------------- drivers/net/ethernet/freescale/xgmac_mdio.c: In function 'xgmac_mdio_probe': drivers/net/ethernet/freescale/xgmac_mdio.c:269:27: error: request for member 'start' in something not a structure or union (unsigned long long)res.start); ^ scripts/Makefile.build:265: recipe for target 'drivers/net/ethernet/freescale/xgmac_mdio.o' failed make[4]: *** [drivers/net/ethernet/freescale/xgmac_mdio.o] Error 1 ---------------------------------------------------------
On checking other files that calls platform_get_resource, I can see that this is the way they refer 'start'.
> > ... > > > - priv->mdio_base = of_iomap(np, 0); > > + priv->mdio_base = devm_ioremap_resource(&pdev->dev, res); > > if (!priv->mdio_base) { > > Are you sure the check is correct now? devm_ioremap_resource returns non-NULL error values. So, this doesn't look right. I'll work on it for v2.
> > ret = -ENOMEM; > > goto err_ioremap; > > } > > ... > > > > > - priv->is_little_endian = of_property_read_bool(pdev->dev.of_node, > > - "little-endian"); > > - > > - priv->has_a011043 = of_property_read_bool(pdev->dev.of_node, > > - "fsl,erratum-a011043"); > > - > > - ret = of_mdiobus_register(bus, np); > > - if (ret) { > > - dev_err(&pdev->dev, "cannot register MDIO bus\n"); > > > + if (is_of_node(pdev->dev.fwnode)) { > > > + } else if (is_acpi_node(pdev->dev.fwnode)) { > > Oh, no, this is wrong. Pure approach AFAICS is to use fwnode API or device > property API. > > And actually what you need to include is rather <linux/property.h>, and not > acpi.h.
Understood. I had got some issues while using fwnode API to handle DT case due to which DT/ACPI checks were done and both are handled separately. Let me see if I can root cause it.
> > > + } else { > > + dev_err(&pdev->dev, "Cannot get cfg data from DT or ACPI\n"); > > + ret = -ENXIO; > > goto err_registration; > > } > > > +static const struct acpi_device_id xgmac_mdio_acpi_match[] = { > > + {"NXP0006", 0} > > How did you test this on platforms with the same IP and without device of > this ACPI ID present?
I didn't test it on any other platforms other than LX2160ARDB. AFAIU, without device of this ACPI ID present, the driver won't get probed. > (Hint: missed terminator) static const struct acpi_device_id xgmac_mdio_acpi_match[] = { { "NXP0006", 0 }, { } }; Is this what you meant?
> > > +}; > > +MODULE_DEVICE_TABLE(acpi, xgmac_mdio_acpi_match); > > > + .acpi_match_table = ACPI_PTR(xgmac_mdio_acpi_match), > > ACPI_PTR is not needed otherwise you will get a compiler warning.
No compiler warning was observed in both cases. I can see other drivers using this macro. drivers/net/ethernet/apm/xgene-v2/main.c:734: .acpi_match_table = ACPI_PTR(xge_acpi_match), drivers/net/ethernet/apm/xgene/xgene_enet_main.c:2172: .acpi_match_table = ACPI_PTR(xgene_enet_acpi_match), drivers/net/ethernet/hisilicon/hns/hns_enet.c:2445: .acpi_match_table = ACPI_PTR(hns_enet_acpi_match), drivers/net/ethernet/hisilicon/hns_mdio.c:566: .acpi_match_table = ACPI_PTR(hns_mdio_acpi_match), drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:5997: .acpi_match_table = ACPI_PTR(mvpp2_acpi_match), drivers/net/ethernet/qualcomm/emac/emac.c:766: .acpi_match_table = ACPI_PTR(emac_acpi_match), drivers/net/ethernet/smsc/smsc911x.c:2667: .acpi_match_table = ACPI_PTR(smsc911x_acpi_match), drivers/net/ethernet/socionext/netsec.c:2187: .acpi_match_table = ACPI_PTR(netsec_acpi_ids), drivers/net/phy/mdio-xgene.c:456: .acpi_match_table = ACPI_PTR(xgene_mdio_acpi_match),
Thanks Calvin
|  |