Re: [PATCH v3 2/3] PCI: Visconti: Add Toshiba Visconti PCIe host controller driver

[Nobuhiro Iwamatsu]

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