regulator: pwm: Don't warn on probe deferral
diff mbox series

Message ID 20200224144048.6587-1-jonathanh@nvidia.com
State New
Headers show
Series
  • regulator: pwm: Don't warn on probe deferral
Related show

Commit Message

Jon Hunter Feb. 24, 2020, 2:40 p.m. UTC
Deferred probe is an expected return value for devm_pwm_get(). Given
that the driver deals with it properly, there's no need to output a
warning that may potentially confuse users.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/regulator/pwm-regulator.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Thierry Reding Feb. 24, 2020, 3:12 p.m. UTC | #1
On Mon, Feb 24, 2020 at 02:40:48PM +0000, Jon Hunter wrote:
> Deferred probe is an expected return value for devm_pwm_get(). Given
> that the driver deals with it properly, there's no need to output a
> warning that may potentially confuse users.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/regulator/pwm-regulator.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Thierry Reding <treding@nvidia.com>
Mark Brown Feb. 24, 2020, 4:58 p.m. UTC | #2
On Mon, Feb 24, 2020 at 02:40:48PM +0000, Jon Hunter wrote:
> Deferred probe is an expected return value for devm_pwm_get(). Given
> that the driver deals with it properly, there's no need to output a
> warning that may potentially confuse users.

>  		ret = PTR_ERR(drvdata->pwm);
> -		dev_err(&pdev->dev, "Failed to get PWM: %d\n", ret);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(&pdev->dev, "Failed to get PWM: %d\n", ret);

This then means that there's no way for users to determine why the
driver has failed to instantiate which can be frustrating.  It'd be
better to at least have some dev_dbg() output when deferring so that
there's something for people to go on without having to instrument the
code.
Uwe Kleine-König Feb. 26, 2020, 4:17 p.m. UTC | #3
On Mon, Feb 24, 2020 at 04:58:59PM +0000, Mark Brown wrote:
> On Mon, Feb 24, 2020 at 02:40:48PM +0000, Jon Hunter wrote:
> > Deferred probe is an expected return value for devm_pwm_get(). Given
> > that the driver deals with it properly, there's no need to output a
> > warning that may potentially confuse users.
> 
> >  		ret = PTR_ERR(drvdata->pwm);
> > -		dev_err(&pdev->dev, "Failed to get PWM: %d\n", ret);
> > +		if (ret != -EPROBE_DEFER)
> > +			dev_err(&pdev->dev, "Failed to get PWM: %d\n", ret);
> 
> This then means that there's no way for users to determine why the
> driver has failed to instantiate which can be frustrating.  It'd be
> better to at least have some dev_dbg() output when deferring so that
> there's something for people to go on without having to instrument the
> code.

Not printing an error message is quite usual however. I think a generic
approach that for example makes the list of devices that should be
retried to probe on the next opportunity inspectable would be nice.

Best regards
Uwe
Mark Brown Feb. 26, 2020, 4:39 p.m. UTC | #4
On Wed, Feb 26, 2020 at 05:17:57PM +0100, Uwe Kleine-König wrote:
> On Mon, Feb 24, 2020 at 04:58:59PM +0000, Mark Brown wrote:

> > This then means that there's no way for users to determine why the
> > driver has failed to instantiate which can be frustrating.  It'd be
> > better to at least have some dev_dbg() output when deferring so that
> > there's something for people to go on without having to instrument the
> > code.

> Not printing an error message is quite usual however. I think a generic

Usual yet also frustraing.

> approach that for example makes the list of devices that should be
> retried to probe on the next opportunity inspectable would be nice.

That's not really the issue, the bigger issue is trying to figure out
why things are stuck - what exactly caused things to fail to
instantiate.
Uwe Kleine-König March 6, 2020, 7:51 a.m. UTC | #5
On Wed, Feb 26, 2020 at 04:39:05PM +0000, Mark Brown wrote:
> On Wed, Feb 26, 2020 at 05:17:57PM +0100, Uwe Kleine-König wrote:
> > On Mon, Feb 24, 2020 at 04:58:59PM +0000, Mark Brown wrote:
> 
> > > This then means that there's no way for users to determine why the
> > > driver has failed to instantiate which can be frustrating.  It'd be
> > > better to at least have some dev_dbg() output when deferring so that
> > > there's something for people to go on without having to instrument the
> > > code.
> 
> > Not printing an error message is quite usual however. I think a generic
> 
> Usual yet also frustraing.
> 
> > approach that for example makes the list of devices that should be
> > retried to probe on the next opportunity inspectable would be nice.
> 
> That's not really the issue, the bigger issue is trying to figure out
> why things are stuck - what exactly caused things to fail to
> instantiate.

I wonder if we should do something like:

	ret = some_call(some, args);
	if (ret) {
		if (emit_errmsg_for_err(ret))
			dev_err(dev, "some_call failed: %pE\n", ERR_PTR(ret));
		return ret;
	}

and have emit_errmsg_for_err return true if ret != -EPROBE_DEFER or some
kernel parameter is given.

Best regards
Uwe
Mark Brown March 6, 2020, 12:35 p.m. UTC | #6
On Fri, Mar 06, 2020 at 08:51:29AM +0100, Uwe Kleine-König wrote:

> I wonder if we should do something like:

> 	ret = some_call(some, args);
> 	if (ret) {
> 		if (emit_errmsg_for_err(ret))
> 			dev_err(dev, "some_call failed: %pE\n", ERR_PTR(ret));
> 		return ret;
> 	}

> and have emit_errmsg_for_err return true if ret != -EPROBE_DEFER or some
> kernel parameter is given.

There was some effort in the past to have a dev_probe_err() or something
which could have a similar implementation but that didn't end up going
anywhere I think.  I do prefer the debug level log since it's much
easier to have the information there without having to ask for it, that
design would've supported that.

Patch
diff mbox series

diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index e74e11101fc1..fb25777a7d47 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -354,7 +354,8 @@  static int pwm_regulator_probe(struct platform_device *pdev)
 	drvdata->pwm = devm_pwm_get(&pdev->dev, NULL);
 	if (IS_ERR(drvdata->pwm)) {
 		ret = PTR_ERR(drvdata->pwm);
-		dev_err(&pdev->dev, "Failed to get PWM: %d\n", ret);
+		if (ret != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "Failed to get PWM: %d\n", ret);
 		return ret;
 	}