[PATCH] regulator: pwm: Don't warn on probe deferral

[PATCH] regulator: pwm: Don't warn on probe deferral

[Jon Hunter]

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(-)

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;
 	}
 
-- 
2.17.1

Re: [PATCH] regulator: pwm: Don't warn on probe deferral

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 521 bytes --]

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/regulator/pwm-regulator.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] regulator: pwm: Don't warn on probe deferral

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 712 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regulator: pwm: Don't warn on probe deferral

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH] regulator: pwm: Don't warn on probe deferral

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 837 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regulator: pwm: Don't warn on probe deferral

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH] regulator: pwm: Don't warn on probe deferral

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 712 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]