Re: [net-next: PATCH 1/3] net: mvmdio: add ACPI support

Hi,

niedz., 13 cze 2021 o 22:08 Andy Shevchenko
[off-list ref] napisał(a):

On Sunday, June 13, 2021, Andrew Lunn [off-list ref] wrote:

quoted

quoted

@@ -336,7 +338,7 @@ static int orion_mdio_probe(struct platform_device *pdev)
                      dev_warn(&pdev->dev,
                               "unsupported number of clocks, limiting to the first "
                               __stringify(ARRAY_SIZE(dev->clk)) "\n");
-     } else {
+     } else if (!has_acpi_companion(&pdev->dev)) {
              dev->clk[0] = clk_get(&pdev->dev, NULL);
              if (PTR_ERR(dev->clk[0]) == -EPROBE_DEFER) {
                      ret = -EPROBE_DEFER;

Is this needed? As you said, there are no clocks when ACPI is used, So
doesn't clk_get() return -ENODEV? Since this is not EPRODE_DEFER, it
keeps going. The clk_prepare_enable() won't be called.

Indeed, I'll double check if it works and will keep the if {} else {} intact.

The better approach is to switch to devm_get_clk_optional() as I have done in several drivers, IIRC recently in mvpp2


https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=cf3399b731d36bc780803ff63e4d480a1efa33ac

Yes, this would be a nice improvement, however the
devm_get_clk_optional requires clock name (type char *) - mvmdio uses
raw indexes, so this helper unfortunately seems to be not applicable.

quoted

quoted

-     ret = of_mdiobus_register(bus, pdev->dev.of_node);
+     if (pdev->dev.of_node)
+             ret = of_mdiobus_register(bus, pdev->dev.of_node);
+     else if (is_acpi_node(pdev->dev.fwnode))
+             ret = acpi_mdiobus_register(bus, pdev->dev.fwnode);
+     else
+             ret = -EINVAL;
      if (ret < 0) {
              dev_err(&pdev->dev, "Cannot register MDIO bus (%d)\n", ret);
              goto out_mdio;

@@ -383,6 +390,9 @@ static int orion_mdio_probe(struct platform_device *pdev)
      if (dev->err_interrupt > 0)
              writel(0, dev->regs + MVMDIO_ERR_INT_MASK);

+     if (has_acpi_companion(&pdev->dev))
+             return ret;
+

I think this can also be removed for the same reason.

We should try to avoid adding has_acpi_companion() and
!pdev->dev.of_node whenever we can. It makes the driver code too much
of a maze.

Clock routines silently accept NULL pointers, so it will be safe to
drop this addition in v2.

Best regards,
Marcin

On Tue, Jun 15, 2021 at 6:09 PM Marcin Wojtas [off-list ref] wrote:

niedz., 13 cze 2021 o 22:08 Andy Shevchenko
[off-list ref] napisał(a):

quoted

On Sunday, June 13, 2021, Andrew Lunn [off-list ref] wrote:

quoted

The better approach is to switch to devm_get_clk_optional() as I have done in several drivers, IIRC recently in mvpp2

https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=cf3399b731d36bc780803ff63e4d480a1efa33ac

Yes, this would be a nice improvement, however the
devm_get_clk_optional requires clock name (type char *) - mvmdio uses
raw indexes, so this helper unfortunately seems to be not applicable.

As far as I can read the code it smells like devm_clk_bulk_get_optional().
Am I mistaken?

-- 
With Best Regards,
Andy Shevchenko