| Message ID | 20210613183520.2247415-1-mw@semihalf.com |
|---|---|
| Headers | show |
| Series | ACPI MDIO support for Marvell controllers | expand |
> @@ -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. > - 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. Andrew
Hi, niedz., 13 cze 2021 o 22:08 Andy Shevchenko <andy.shevchenko@gmail.com> napisał(a): > > > > On Sunday, June 13, 2021, Andrew Lunn <andrew@lunn.ch> wrote: >> >> > @@ -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. >> >> > - 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
Hi, niedz., 13 cze 2021 o 21:20 Andrew Lunn <andrew@lunn.ch> napisał(a): > > > - 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; > > > This seems like something which could be put into fwnode_mdio.c. > Agree - I'll create a simple fwnode_mdiobus_register() helper there. Best regards, Marcin
On Tue, Jun 15, 2021 at 6:09 PM Marcin Wojtas <mw@semihalf.com> wrote: > niedz., 13 cze 2021 o 22:08 Andy Shevchenko > <andy.shevchenko@gmail.com> napisał(a): > > On Sunday, June 13, 2021, Andrew Lunn <andrew@lunn.ch> wrote: > > 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
On Tue, Jun 15, 2021 at 6:14 PM Marcin Wojtas <mw@semihalf.com> wrote: > Hi, > niedz., 13 cze 2021 o 21:20 Andrew Lunn <andrew@lunn.ch> napisał(a): > > > > > - 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; > > > > > > This seems like something which could be put into fwnode_mdio.c. > > > > Agree - I'll create a simple fwnode_mdiobus_register() helper there. Please, also convert the users that we will not have again some open-coded examples here and there https://lore.kernel.org/netdev/162344280835.13501.16334655818490594799.git-patchwork-notify@kernel.org/T/#mff706861dea5d3be037d1546fa9c362b27d5839b (Btw, note the is_of_node() usage there, so should fwnode_mdiobus_register() have) -- With Best Regards, Andy Shevchenko