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> <6501a53b-40aa-5374-3c4a-6b21824f82fd@gmail.com>
In-Reply-To: <6501a53b-40aa-5374-3c4a-6b21824f82fd@gmail.com>
From:   Calvin Johnson <linux.cj@gmail.com>
Date:   Wed, 5 Feb 2020 00:16:39 +0530
Message-ID: <CAEhpT-VfWhha3B6qS6wFu-PQP9+zb4gxt7=nsrA58cecX21Dog@mail.gmail.com>
Subject: Re: [PATCH v1 3/7] net/fsl: add ACPI support for mdio bus
To:     Florian Fainelli <f.fainelli@gmail.com>
Cc:     Calvin Johnson <calvin.johnson@nxp.com>,
        Jon Nettleton <jon@solid-run.com>, 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@vger.kernel.org, netdev@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, Feb 02, 2020 at 07:44:40PM -0800, Florian Fainelli wrote:
>
>
> On 1/31/2020 7:34 AM, Calvin Johnson wrote:
> > From: Calvin Johnson <calvin.johnson@oss.nxp.com>
> >
> > Add ACPI support for MDIO bus registration while maintaining
> > the existing DT support.
> >
> > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> > ---
>
> [snip]
>
> >     bus = mdiobus_alloc_size(sizeof(struct mdio_fsl_priv));
> > @@ -263,25 +265,41 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
> >     bus->read = xgmac_mdio_read;
> >     bus->write = xgmac_mdio_write;
> >     bus->parent = &pdev->dev;
> > -   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);
>
> You could omit this clean up change.
Sure, will avoid split to newline.
> >
> >     /* Set the PHY base address */
> >     priv = bus->priv;
> > -   priv->mdio_base = of_iomap(np, 0);
> > +   priv->mdio_base = devm_ioremap_resource(&pdev->dev, res);
> >     if (!priv->mdio_base) {
>
> This probably needs to become IS_ERR() instead of a plain NULL check
Ok. Will take care in 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)) {
> > +           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");
> > +                   goto err_registration;
> > +           }
> > +   } else if (is_acpi_node(pdev->dev.fwnode)) {
> > +           priv->is_little_endian =
> > +                   fwnode_property_read_bool(pdev->dev.fwnode,
> > +                                             "little-endian");
> > +           ret = fwnode_mdiobus_register(bus, pdev->dev.fwnode);
> > +           if (ret) {
> > +                   dev_err(&pdev->dev, "cannot register MDIO bus\n");
> > +                   goto err_registration;
> > +           }
>
> The little-endian property read can be moved out of the DT/ACPI paths
> and you can just use device_property_read_bool() for that purpose.
> Having both fwnode_mdiobus_register() and of_mdiobus_register() looks
> fairly redundant, you could quite easily introduce a wrapper:
> device_mdiobus_register() which internally takes the appropriate DT/ACPI
> paths as needed.
Had some difficulty with DT while using fwnode APIs. Will resolve them
and provide better integrated code.

Thanks

Calvin