diff mbox series

backlight: pwm_bl: print errno for probe errors

Message ID 20240801091255.1410027-1-martin.kepplinger-novakovic@ginzinger.com (mailing list archive)
State Handled Elsewhere
Headers show
Series backlight: pwm_bl: print errno for probe errors | expand

Commit Message

Martin Kepplinger-Novaković Aug. 1, 2024, 9:12 a.m. UTC
This makes debugging often easier.

Signed-off-by: Martin Kepplinger-Novaković <martin.kepplinger-novakovic@ginzinger.com>
---
 drivers/video/backlight/pwm_bl.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Uwe Kleine-König Aug. 1, 2024, 9:26 a.m. UTC | #1
Hello Martin,

On Thu, Aug 01, 2024 at 11:12:55AM +0200, Martin Kepplinger-Novaković wrote:
> This makes debugging often easier.
> 
> Signed-off-by: Martin Kepplinger-Novaković <martin.kepplinger-novakovic@ginzinger.com>
> ---
>  drivers/video/backlight/pwm_bl.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index f1005bd0c41e3..cc7e7af71891f 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -502,7 +502,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>  						  GPIOD_ASIS);
>  	if (IS_ERR(pb->enable_gpio)) {
>  		ret = dev_err_probe(&pdev->dev, PTR_ERR(pb->enable_gpio),
> -				    "failed to acquire enable GPIO\n");
> +				    "failed to acquire enable GPIO: %ld\n",
> +				    PTR_ERR(pb->enable_gpio));

AFAIK dev_err_probe already emits the error code passed as 2nd
parameter. So I wonder about this patch's benefit.

Best regards
Uwe
Martin Kepplinger-Novaković Aug. 1, 2024, 9:52 a.m. UTC | #2
Am Donnerstag, dem 01.08.2024 um 11:26 +0200 schrieb Uwe Kleine-König:
> Hello Martin,
> 
> On Thu, Aug 01, 2024 at 11:12:55AM +0200, Martin Kepplinger-Novaković
> wrote:
> > This makes debugging often easier.
> > 
> > Signed-off-by: Martin Kepplinger-Novaković
> > <martin.kepplinger-novakovic@ginzinger.com>
> > ---
> >  drivers/video/backlight/pwm_bl.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/video/backlight/pwm_bl.c
> > b/drivers/video/backlight/pwm_bl.c
> > index f1005bd0c41e3..cc7e7af71891f 100644
> > --- a/drivers/video/backlight/pwm_bl.c
> > +++ b/drivers/video/backlight/pwm_bl.c
> > @@ -502,7 +502,8 @@ static int pwm_backlight_probe(struct
> > platform_device *pdev)
> >                                                   GPIOD_ASIS);
> >         if (IS_ERR(pb->enable_gpio)) {
> >                 ret = dev_err_probe(&pdev->dev, PTR_ERR(pb-
> > >enable_gpio),
> > -                                   "failed to acquire enable
> > GPIO\n");
> > +                                   "failed to acquire enable GPIO:
> > %ld\n",
> > +                                   PTR_ERR(pb->enable_gpio));
> 
> AFAIK dev_err_probe already emits the error code passed as 2nd
> parameter. So I wonder about this patch's benefit.
> 

It does. Other messages only take the deferred_probe_reason without the
error code. It's actually fine if users properly enable debugging after
seeing an error and then this change is not needed :)

thanks,
                               martin
Uwe Kleine-König Aug. 1, 2024, 10:09 a.m. UTC | #3
Hello Martin,

On Thu, Aug 01, 2024 at 09:52:01AM +0000, Kepplinger-Novakovic Martin wrote:
> Am Donnerstag, dem 01.08.2024 um 11:26 +0200 schrieb Uwe Kleine-König:
> > On Thu, Aug 01, 2024 at 11:12:55AM +0200, Martin Kepplinger-Novaković
> > wrote:
> > > diff --git a/drivers/video/backlight/pwm_bl.c
> > > b/drivers/video/backlight/pwm_bl.c
> > > index f1005bd0c41e3..cc7e7af71891f 100644
> > > --- a/drivers/video/backlight/pwm_bl.c
> > > +++ b/drivers/video/backlight/pwm_bl.c
> > > @@ -502,7 +502,8 @@ static int pwm_backlight_probe(struct
> > > platform_device *pdev)
> > >                                                   GPIOD_ASIS);
> > >         if (IS_ERR(pb->enable_gpio)) {
> > >                 ret = dev_err_probe(&pdev->dev, PTR_ERR(pb-
> > > >enable_gpio),
> > > -                                   "failed to acquire enable
> > > GPIO\n");
> > > +                                   "failed to acquire enable GPIO:
> > > %ld\n",
> > > +                                   PTR_ERR(pb->enable_gpio));
> > 
> > AFAIK dev_err_probe already emits the error code passed as 2nd
> > parameter. So I wonder about this patch's benefit.
> > 
> 
> It does. Other messages only take the deferred_probe_reason without the
> error code. It's actually fine if users properly enable debugging after
> seeing an error and then this change is not needed :)

I'm unsure what you intend to say here. Do you agree that this patch
doesn't need to be applied as it doesn't add any information to the
emitted messages? Or do you think there is a value because "users don't
need to enable debugging" then. In the latter case I don't see where
users would see "failed to acquire enable GPIO" before, but not the
value of the error code.

Best regards
Uwe
Martin Kepplinger-Novaković Aug. 1, 2024, 10:12 a.m. UTC | #4
Am Donnerstag, dem 01.08.2024 um 12:09 +0200 schrieb Uwe Kleine-König:
> Hello Martin,
> 
> On Thu, Aug 01, 2024 at 09:52:01AM +0000, Kepplinger-Novakovic Martin
> wrote:
> > Am Donnerstag, dem 01.08.2024 um 11:26 +0200 schrieb Uwe Kleine-
> > König:
> > > On Thu, Aug 01, 2024 at 11:12:55AM +0200, Martin Kepplinger-
> > > Novaković
> > > wrote:
> > > > diff --git a/drivers/video/backlight/pwm_bl.c
> > > > b/drivers/video/backlight/pwm_bl.c
> > > > index f1005bd0c41e3..cc7e7af71891f 100644
> > > > --- a/drivers/video/backlight/pwm_bl.c
> > > > +++ b/drivers/video/backlight/pwm_bl.c
> > > > @@ -502,7 +502,8 @@ static int pwm_backlight_probe(struct
> > > > platform_device *pdev)
> > > >                                                   GPIOD_ASIS);
> > > >         if (IS_ERR(pb->enable_gpio)) {
> > > >                 ret = dev_err_probe(&pdev->dev, PTR_ERR(pb-
> > > > > enable_gpio),
> > > > -                                   "failed to acquire enable
> > > > GPIO\n");
> > > > +                                   "failed to acquire enable
> > > > GPIO:
> > > > %ld\n",
> > > > +                                   PTR_ERR(pb->enable_gpio));
> > > 
> > > AFAIK dev_err_probe already emits the error code passed as 2nd
> > > parameter. So I wonder about this patch's benefit.
> > > 
> > 
> > It does. Other messages only take the deferred_probe_reason without
> > the
> > error code. It's actually fine if users properly enable debugging
> > after
> > seeing an error and then this change is not needed :)
> 
> I'm unsure what you intend to say here. Do you agree that this patch
> doesn't need to be applied as it doesn't add any information to the
> emitted messages? Or do you think there is a value because "users
> don't
> need to enable debugging" then. In the latter case I don't see where
> users would see "failed to acquire enable GPIO" before, but not the
> value of the error code.
> 

hi Uwe, sorry, I agree that this patch doesn't add any information. I
think it can be expected to look at debug when drivers don't probe.

thanks,
                         martin
Lee Jones Aug. 1, 2024, 12:31 p.m. UTC | #5
On Thu, 01 Aug 2024, Martin Kepplinger-Novaković wrote:

> This makes debugging often easier.
> 
> Signed-off-by: Martin Kepplinger-Novaković <martin.kepplinger-novakovic@ginzinger.com>
> ---
>  drivers/video/backlight/pwm_bl.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

Please refrain from signing your mails like this.  It means that some of
us have to physically click a pop-up box as we are parsing our inboxes.

I'm deleting all mails in this thread.
diff mbox series

Patch

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index f1005bd0c41e3..cc7e7af71891f 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -502,7 +502,8 @@  static int pwm_backlight_probe(struct platform_device *pdev)
 						  GPIOD_ASIS);
 	if (IS_ERR(pb->enable_gpio)) {
 		ret = dev_err_probe(&pdev->dev, PTR_ERR(pb->enable_gpio),
-				    "failed to acquire enable GPIO\n");
+				    "failed to acquire enable GPIO: %ld\n",
+				    PTR_ERR(pb->enable_gpio));
 		goto err_alloc;
 	}
 
@@ -513,7 +514,8 @@  static int pwm_backlight_probe(struct platform_device *pdev)
 			pb->power_supply = NULL;
 		} else {
 			dev_err_probe(&pdev->dev, ret,
-				      "failed to acquire power regulator\n");
+				      "failed to acquire power regulator: %d\n",
+				      ret);
 			goto err_alloc;
 		}
 	}
@@ -521,7 +523,8 @@  static int pwm_backlight_probe(struct platform_device *pdev)
 	pb->pwm = devm_pwm_get(&pdev->dev, NULL);
 	if (IS_ERR(pb->pwm)) {
 		ret = dev_err_probe(&pdev->dev, PTR_ERR(pb->pwm),
-				    "unable to request PWM\n");
+				    "unable to request PWM: %ld\n",
+				    PTR_ERR(pb->pwm));
 		goto err_alloc;
 	}