Re: [PATCH 1/3] ARM: tegra: pcie: Add tegra3 support

[Thierry Reding]

On Fri, Apr 12, 2013 at 09:34:13AM -0600, Stephen Warren wrote:
> On 04/12/2013 08:58 AM, Jay Agarwal wrote:
> >>>  	err = regulator_disable(pcie->pex_clk_supply);
> >>>  	if (err < 0)
> >>> -		dev_err(pcie->dev, "failed to disable pex-clk regulator:
> >> %d\n",
> >>> +		dev_warn(pcie->dev, "failed to disable pex-clk regulator:
> >> %d\n",
> >>>  			err);
> >>>
> >>>  	err = regulator_disable(pcie->vdd_supply);
> >>>  	if (err < 0)
> >>> -		dev_err(pcie->dev, "failed to disable VDD regulator: %d\n",
> >>> +		dev_warn(pcie->dev, "failed to disable VDD regulator:
> >> %d\n",
> >>>  			err);
> >>
> >> Please explain why that change is correct. If the regulators only exist on
> >> Tegra20, please represent that fact in the SoC data. Regulators must always
> >> exist, so enable/disable should never fail due to missing regulators. Actual
> >> run-time failures seem like something that really is an error.
> >>
> > [>] These regulators are needed for both tegra20 & tegra30. Since we are not returning error here, so changed to dev_warn.
> 
> If the regulators are required, then any failure to operate them should
> be an error, hence dev_err() seems correct.
> 
> As to why the code doesn't actually return an error? I'm not sure.
> Perhaps that should be fixed with a separate patch, although I don't
> recall exactly where in the code the above excerpt is; if it's in
> remove(), then continuing on without returning an error would be
> appropriate.

That code is from tegra_pcie_power_off(), which is called only during
error cleanup or from tegra_pcie_put_resources() which in turn is also
only called in cleanup paths or during module/device removal. Disabling
as many regulators as possible is still what we want in that case, so
returning an error prematurely might leave more regulators turned on
than necessary.

Thierry

Attachment: pgp00000.pgp
Description: PGP signature