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
[unhandled content-type:application/pgp-signature]