Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 24 May 2021 13:10:51 +0200
From:   Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>
To:     Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
Cc:     Bjorn Helgaas <bhelgaas@google.com>,
        Rob Herring <robh+dt@kernel.org>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        linux-pci@vger.kernel.org, punit1.agrawal@toshiba.co.jp,
        yuji2.ishikawa@toshiba.co.jp, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 2/3] PCI: Visconti: Add Toshiba Visconti PCIe host
 controller driver
Message-ID: <20210524111051.GB244904@rocinante.localdomain>
References: <20210524063004.132043-1-nobuhiro1.iwamatsu@toshiba.co.jp>
 <20210524063004.132043-3-nobuhiro1.iwamatsu@toshiba.co.jp>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20210524063004.132043-3-nobuhiro1.iwamatsu@toshiba.co.jp>
Precedence: bulk

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");

[...]
> +	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).

	Krzysztof