Hi,
Thanks for your review.
On Mon, May 24, 2021 at 01:10:51PM +0200, Krzysztof Wilczyński wrote:
> Hi Nobuhiro,
>
> Thank you for working on this!
>
> [...]
> > +static int visconti_get_resources(struct platform_device *pdev,
> > + struct visconti_pcie *pcie)
> > +{
> [...]
> > + pcie->refclk = devm_clk_get(dev, "pcie_refclk");
> > + if (IS_ERR(pcie->refclk)) {
> > + dev_err(dev, "Failed to get refclk clock: %ld\n",
> > + PTR_ERR(pcie->refclk));
> > + return PTR_ERR(pcie->refclk);
> > + }
> > +
> > + pcie->sysclk = devm_clk_get(dev, "sysclk");
> > + if (IS_ERR(pcie->sysclk)) {
> > + dev_err(dev, "Failed to get sysclk clock: %ld\n",
> > + PTR_ERR(pcie->sysclk));
> > + return PTR_ERR(pcie->sysclk);
> > + }
> > +
> > + pcie->auxclk = devm_clk_get(dev, "auxclk");
> > + if (IS_ERR(pcie->auxclk)) {
> > + dev_err(dev, "Failed to get auxclk clock: %ld\n",
> > + PTR_ERR(pcie->auxclk));
> > + return PTR_ERR(pcie->auxclk);
> > + }
>
> Do you think you could use the dev_err_probe() to handle the
> devm_clk_get() failed? Where applicable this is becoming a common
> patter drivers apply, for example:
>
> pcie->refclk = devm_clk_get(dev, "pcie_refclk");
> if (IS_ERR(pcie->refclk))
> return dev_err_probe(dev, PTR_ERR(pcie->refclk),
> "failed to get refclk clock\n");
>
> [...]
Thanks for your suggestion. I will fix using dev_err_probe().
> > + pci->link_gen = of_pci_get_max_link_speed(pdev->dev.of_node);
> > + if (pci->link_gen < 0 || pci->link_gen > 3) {
> > + pci->link_gen = 3;
> > + dev_dbg(dev, "Applied default link speed\n");
> > + }
> > +
> > + dev_dbg(dev, "Link speed Gen %d", pci->link_gen);
>
> Question about the above debug messages.
>
> Given that both are at the same level and the link speed will be printed
> regardless of whether it was set to a default value or not, does it make
> sense to still print the message about applying the default link speed?
> Unless this is something that will be indeed useful during debugging and
> troubleshooting (and in which case just ignore this question).
I guess so, the message about the default value is not important.
I will remove this, thank you.
Best regards,
Nobuhiro
