Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200131153440.20870-1-calvin.johnson@nxp.com> <20200131153440.20870-4-calvin.johnson@nxp.com>
In-Reply-To: <20200131153440.20870-4-calvin.johnson@nxp.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Fri, 31 Jan 2020 18:08:58 +0200
Message-ID: <CAHp75VeRq8XT67LJOM+9R9xVpsfv7MxZpaCHYkfnCqAzgjXo9A@mail.gmail.com>
Subject: Re: [PATCH v1 3/7] net/fsl: add ACPI support for mdio bus
To:     Calvin Johnson <calvin.johnson@nxp.com>
Cc:     linux.cj@gmail.com, Jon Nettleton <jon@solid-run.com>,
        Russell King - ARM Linux <linux@armlinux.org.uk>,
        Makarand Pawagi <makarand.pawagi@nxp.com>,
        cristian.sovaiala@nxp.com, laurentiu.tudor@nxp.com,
        ioana.ciornei@nxp.com, V.Sethi@nxp.com, pankaj.bansal@nxp.com,
        "Rajesh V . Bikkina" <rajesh.bikkina@nxp.com>,
        Calvin Johnson <calvin.johnson@oss.nxp.com>,
        "David S. Miller" <davem@davemloft.net>,
        Madalin Bucur <madalin.bucur@oss.nxp.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        netdev <netdev@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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?

...

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

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

> +       } 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?

(Hint: missed terminator)

> +};
> +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.

-- 
With Best Regards,
Andy Shevchenko