Re: [PATCH v1 3/7] net/fsl: add ACPI support for mdio bus

[Andy Shevchenko]

On Fri, Jan 31, 2020 at 5:37 PM Calvin Johnson <calvin.johnson@xxxxxxx> wrote:
>
> From: Calvin Johnson <calvin.johnson@xxxxxxxxxxx>
>
> 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