lkml.org 
[lkml]   [2020]   [Feb]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH v1 3/7] net/fsl: add ACPI support for mdio bus
Date

> -----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

\
 
 \ /
  Last update: 2020-02-04 08:20    [W:1.754 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site