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

[Calvin Johnson]

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