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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 5 Feb 2020 00:16:39 +0530
From:   Calvin Johnson <linux.cj@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     Calvin Johnson <calvin.johnson@....com>,
        Jon Nettleton <jon@...id-run.com>, linux@...linux.org.uk,
        Makarand Pawagi <makarand.pawagi@....com>,
        cristian.sovaiala@....com, laurentiu.tudor@....com,
        ioana.ciornei@....com, V.Sethi@....com, pankaj.bansal@....com,
        "Rajesh V . Bikkina" <rajesh.bikkina@....com>,
        Calvin Johnson <calvin.johnson@....nxp.com>,
        "David S. Miller" <davem@...emloft.net>,
        Madalin Bucur <madalin.bucur@....nxp.com>,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH v1 3/7] net/fsl: add ACPI support for mdio bus

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@....nxp.com>
> >
> > Add ACPI support for MDIO bus registration while maintaining
> > the existing DT support.
> >
> > Signed-off-by: Calvin Johnson <calvin.johnson@....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