Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1551971 > unrolled thread

[PATCH] platform/x86: fujitsu-laptop: rework logolamp_set() to properly handle errors

Started byMichał Kępień <kernel@kempniu.pl>
First post2017-01-05 14:20 +0100
Last post2017-01-07 02:10 +0100
Articles 6 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] platform/x86: fujitsu-laptop: rework logolamp_set() to properly handle errors Michał Kępień <kernel@kempniu.pl> - 2017-01-05 14:20 +0100
    Re: [PATCH] platform/x86: fujitsu-laptop: rework logolamp_set() to  properly handle errors Jonathan Woithe <jwoithe@just42.net> - 2017-01-05 15:10 +0100
    Re: [PATCH] platform/x86: fujitsu-laptop: rework logolamp_set() to  properly handle errors Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-01-06 16:30 +0100
      Re: [PATCH] platform/x86: fujitsu-laptop: rework logolamp_set() to  properly handle errors Michał Kępień <kernel@kempniu.pl> - 2017-01-06 20:30 +0100
        Re: [PATCH] platform/x86: fujitsu-laptop: rework logolamp_set() to  properly handle errors Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-01-06 21:50 +0100
        Re: [PATCH] platform/x86: fujitsu-laptop: rework logolamp_set() to  properly handle errors Jonathan Woithe <jwoithe@just42.net> - 2017-01-07 02:10 +0100

#1551971 — [PATCH] platform/x86: fujitsu-laptop: rework logolamp_set() to properly handle errors

FromMichał Kępień <kernel@kempniu.pl>
Date2017-01-05 14:20 +0100
Subject[PATCH] platform/x86: fujitsu-laptop: rework logolamp_set() to properly handle errors
Message-ID<sWg7w-3sQ-1@gated-at.bofh.it>
Potential errors returned by some call_fext_func() calls inside
logolamp_set() are currently ignored.  Rework logolamp_set() to properly
handle them.  This causes one more call_fext_func() call to be made in
the LED_OFF case, though one could argue that this is logically the
right thing to do (even though the extra call is not needed to shut the
LED off).

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
This is a follow-up to a recent patch, "platform/x86: fujitsu-laptop:
use brightness_set_blocking for LED-setting callbacks" and thus needs
the latter to be applied first (currently it is already applied in
testing).
    
Instead of sticking "if (ret < 0) return ret;" in two branches I decided
to restructure logolamp_set() for improved clarity and also to make it
resemble logolamp_get() a bit more.

 drivers/platform/x86/fujitsu-laptop.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index b725a907a91f..12f7a8346dd0 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -271,15 +271,19 @@ static int call_fext_func(int cmd, int arg0, int arg1, int arg2)
 static int logolamp_set(struct led_classdev *cdev,
 			       enum led_brightness brightness)
 {
-	if (brightness >= LED_FULL) {
-		call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_POWERON, FUNC_LED_ON);
-		return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_ALWAYS, FUNC_LED_ON);
-	} else if (brightness >= LED_HALF) {
-		call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_POWERON, FUNC_LED_ON);
-		return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_ALWAYS, FUNC_LED_OFF);
-	} else {
-		return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_POWERON, FUNC_LED_OFF);
+	int ret, poweron = FUNC_LED_OFF, always = FUNC_LED_OFF;
+
+	if (brightness >= LED_HALF) {
+		poweron = FUNC_LED_ON;
+		if (brightness >= LED_FULL)
+			always = FUNC_LED_ON;
 	}
+
+	ret = call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_POWERON, poweron);
+	if (ret < 0)
+		return ret;
+
+	return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_ALWAYS, always);
 }
 
 static int kblamps_set(struct led_classdev *cdev,
-- 
2.11.0

[toc] | [next] | [standalone]

#1552001 — Re: [PATCH] platform/x86: fujitsu-laptop: rework logolamp_set() to properly handle errors

FromJonathan Woithe <jwoithe@just42.net>
Date2017-01-05 15:10 +0100
SubjectRe: [PATCH] platform/x86: fujitsu-laptop: rework logolamp_set() to properly handle errors
Message-ID<sWgTT-46q-5@gated-at.bofh.it>
In reply to#1551971
On Thu, Jan 05, 2017 at 02:11:29PM +0100, Micha?? K??pie?? wrote:
> Potential errors returned by some call_fext_func() calls inside
> logolamp_set() are currently ignored.  Rework logolamp_set() to properly
> handle them.  This causes one more call_fext_func() call to be made in
> the LED_OFF case, though one could argue that this is logically the
> right thing to do (even though the extra call is not needed to shut the
> LED off).
> 
> Signed-off-by: Micha?? K??pie?? <kernel@kempniu.pl>

Acked-by: Jonathan Woithe <jwoithe@just42.net>

> ---
> This is a follow-up to a recent patch, "platform/x86: fujitsu-laptop:
> use brightness_set_blocking for LED-setting callbacks" and thus needs
> the latter to be applied first (currently it is already applied in
> testing).
>     
> Instead of sticking "if (ret < 0) return ret;" in two branches I decided
> to restructure logolamp_set() for improved clarity and also to make it
> resemble logolamp_get() a bit more.
> 
>  drivers/platform/x86/fujitsu-laptop.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index b725a907a91f..12f7a8346dd0 100644
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -271,15 +271,19 @@ static int call_fext_func(int cmd, int arg0, int arg1, int arg2)
>  static int logolamp_set(struct led_classdev *cdev,
>  			       enum led_brightness brightness)
>  {
> -	if (brightness >= LED_FULL) {
> -		call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_POWERON, FUNC_LED_ON);
> -		return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_ALWAYS, FUNC_LED_ON);
> -	} else if (brightness >= LED_HALF) {
> -		call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_POWERON, FUNC_LED_ON);
> -		return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_ALWAYS, FUNC_LED_OFF);
> -	} else {
> -		return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_POWERON, FUNC_LED_OFF);
> +	int ret, poweron = FUNC_LED_OFF, always = FUNC_LED_OFF;
> +
> +	if (brightness >= LED_HALF) {
> +		poweron = FUNC_LED_ON;
> +		if (brightness >= LED_FULL)
> +			always = FUNC_LED_ON;
>  	}
> +
> +	ret = call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_POWERON, poweron);
> +	if (ret < 0)
> +		return ret;
> +
> +	return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_ALWAYS, always);
>  }
>  
>  static int kblamps_set(struct led_classdev *cdev,
> -- 
> 2.11.0

[toc] | [prev] | [next] | [standalone]

#1552873 — Re: [PATCH] platform/x86: fujitsu-laptop: rework logolamp_set() to properly handle errors

FromAndy Shevchenko <andy.shevchenko@gmail.com>
Date2017-01-06 16:30 +0100
SubjectRe: [PATCH] platform/x86: fujitsu-laptop: rework logolamp_set() to properly handle errors
Message-ID<sWECR-3O1-1@gated-at.bofh.it>
In reply to#1551971
On Thu, Jan 5, 2017 at 3:11 PM, Michał Kępień <kernel@kempniu.pl> wrote:
> Potential errors returned by some call_fext_func() calls inside
> logolamp_set() are currently ignored.  Rework logolamp_set() to properly
> handle them.  This causes one more call_fext_func() call to be made in
> the LED_OFF case, though one could argue that this is logically the
> right thing to do (even though the extra call is not needed to shut the
> LED off).
>
> Signed-off-by: Michał Kępień <kernel@kempniu.pl>
> ---
> This is a follow-up to a recent patch, "platform/x86: fujitsu-laptop:
> use brightness_set_blocking for LED-setting callbacks" and thus needs
> the latter to be applied first (currently it is already applied in
> testing).
>
> Instead of sticking "if (ret < 0) return ret;" in two branches I decided
> to restructure logolamp_set() for improved clarity and also to make it
> resemble logolamp_get() a bit more.
>
>  drivers/platform/x86/fujitsu-laptop.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index b725a907a91f..12f7a8346dd0 100644
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -271,15 +271,19 @@ static int call_fext_func(int cmd, int arg0, int arg1, int arg2)
>  static int logolamp_set(struct led_classdev *cdev,
>                                enum led_brightness brightness)
>  {
> -       if (brightness >= LED_FULL) {
> -               call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_POWERON, FUNC_LED_ON);
> -               return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_ALWAYS, FUNC_LED_ON);
> -       } else if (brightness >= LED_HALF) {
> -               call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_POWERON, FUNC_LED_ON);
> -               return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_ALWAYS, FUNC_LED_OFF);
> -       } else {
> -               return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_POWERON, FUNC_LED_OFF);

> +       int ret, poweron = FUNC_LED_OFF, always = FUNC_LED_OFF;
> +
> +       if (brightness >= LED_HALF) {
> +               poweron = FUNC_LED_ON;
> +               if (brightness >= LED_FULL)
> +                       always = FUNC_LED_ON;
>         }

But if you set ON by default the above could be written like

    if (brightness < LED_FULL)
        always = FUNC_LED_OFF;

    if (brightness < LED_HALF)
        poweron = FUNC_LED_OFF;

> +
> +       ret = call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_POWERON, poweron);
> +       if (ret < 0)
> +               return ret;
> +
> +       return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_ALWAYS, always);
>  }
>
>  static int kblamps_set(struct led_classdev *cdev,
> --
> 2.11.0
>



-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1553052 — Re: [PATCH] platform/x86: fujitsu-laptop: rework logolamp_set() to properly handle errors

FromMichał Kępień <kernel@kempniu.pl>
Date2017-01-06 20:30 +0100
SubjectRe: [PATCH] platform/x86: fujitsu-laptop: rework logolamp_set() to properly handle errors
Message-ID<sWIn7-6oa-1@gated-at.bofh.it>
In reply to#1552873
> On Thu, Jan 5, 2017 at 3:11 PM, Michał Kępień <kernel@kempniu.pl> wrote:
> > Potential errors returned by some call_fext_func() calls inside
> > logolamp_set() are currently ignored.  Rework logolamp_set() to properly
> > handle them.  This causes one more call_fext_func() call to be made in
> > the LED_OFF case, though one could argue that this is logically the
> > right thing to do (even though the extra call is not needed to shut the
> > LED off).
> >
> > Signed-off-by: Michał Kępień <kernel@kempniu.pl>
> > ---
> > This is a follow-up to a recent patch, "platform/x86: fujitsu-laptop:
> > use brightness_set_blocking for LED-setting callbacks" and thus needs
> > the latter to be applied first (currently it is already applied in
> > testing).
> >
> > Instead of sticking "if (ret < 0) return ret;" in two branches I decided
> > to restructure logolamp_set() for improved clarity and also to make it
> > resemble logolamp_get() a bit more.
> >
> >  drivers/platform/x86/fujitsu-laptop.c | 20 ++++++++++++--------
> >  1 file changed, 12 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> > index b725a907a91f..12f7a8346dd0 100644
> > --- a/drivers/platform/x86/fujitsu-laptop.c
> > +++ b/drivers/platform/x86/fujitsu-laptop.c
> > @@ -271,15 +271,19 @@ static int call_fext_func(int cmd, int arg0, int arg1, int arg2)
> >  static int logolamp_set(struct led_classdev *cdev,
> >                                enum led_brightness brightness)
> >  {
> > -       if (brightness >= LED_FULL) {
> > -               call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_POWERON, FUNC_LED_ON);
> > -               return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_ALWAYS, FUNC_LED_ON);
> > -       } else if (brightness >= LED_HALF) {
> > -               call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_POWERON, FUNC_LED_ON);
> > -               return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_ALWAYS, FUNC_LED_OFF);
> > -       } else {
> > -               return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_POWERON, FUNC_LED_OFF);
> 
> > +       int ret, poweron = FUNC_LED_OFF, always = FUNC_LED_OFF;
> > +
> > +       if (brightness >= LED_HALF) {
> > +               poweron = FUNC_LED_ON;
> > +               if (brightness >= LED_FULL)
> > +                       always = FUNC_LED_ON;
> >         }
> 
> But if you set ON by default the above could be written like
> 
>     if (brightness < LED_FULL)
>         always = FUNC_LED_OFF;
> 
>     if (brightness < LED_HALF)
>         poweron = FUNC_LED_OFF;

Ah, neat, thanks.  Jonathan, assuming you also like this, I will later
submit v2 using this approach.

-- 
Best regards,
Michał Kępień

[toc] | [prev] | [next] | [standalone]

#1553101 — Re: [PATCH] platform/x86: fujitsu-laptop: rework logolamp_set() to properly handle errors

FromAndy Shevchenko <andy.shevchenko@gmail.com>
Date2017-01-06 21:50 +0100
SubjectRe: [PATCH] platform/x86: fujitsu-laptop: rework logolamp_set() to properly handle errors
Message-ID<sWJCx-7a6-19@gated-at.bofh.it>
In reply to#1553052
On Fri, Jan 6, 2017 at 9:24 PM, Michał Kępień <kernel@kempniu.pl> wrote:
>> On Thu, Jan 5, 2017 at 3:11 PM, Michał Kępień <kernel@kempniu.pl> wrote:

>> > +       int ret, poweron = FUNC_LED_OFF, always = FUNC_LED_OFF;
>> > +

Don't know if you are already about to send v2, though I would see two
lines of above instead of one.
int ...;
int ret;


-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1553528 — Re: [PATCH] platform/x86: fujitsu-laptop: rework logolamp_set() to properly handle errors

FromJonathan Woithe <jwoithe@just42.net>
Date2017-01-07 02:10 +0100
SubjectRe: [PATCH] platform/x86: fujitsu-laptop: rework logolamp_set() to properly handle errors
Message-ID<sWNGa-1Gc-9@gated-at.bofh.it>
In reply to#1553052
On Fri, Jan 06, 2017 at 08:24:31PM +0100, Micha?? K??pie?? wrote:
> > On Thu, Jan 5, 2017 at 3:11 PM, Micha?? K??pie?? <kernel@kempniu.pl> wrote:
> > > Potential errors returned by some call_fext_func() calls inside
> > > logolamp_set() are currently ignored.  Rework logolamp_set() to properly
> > > handle them.  This causes one more call_fext_func() call to be made in
> > > the LED_OFF case, though one could argue that this is logically the
> > > right thing to do (even though the extra call is not needed to shut the
> > > LED off).
> > >
> > > Signed-off-by: Micha?? K??pie?? <kernel@kempniu.pl>
> > > ---
> > > This is a follow-up to a recent patch, "platform/x86: fujitsu-laptop:
> > > use brightness_set_blocking for LED-setting callbacks" and thus needs
> > > the latter to be applied first (currently it is already applied in
> > > testing).
> > >
> > > Instead of sticking "if (ret < 0) return ret;" in two branches I decided
> > > to restructure logolamp_set() for improved clarity and also to make it
> > > resemble logolamp_get() a bit more.
> > >
> > >  drivers/platform/x86/fujitsu-laptop.c | 20 ++++++++++++--------
> > >  1 file changed, 12 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> > > index b725a907a91f..12f7a8346dd0 100644
> > > --- a/drivers/platform/x86/fujitsu-laptop.c
> > > +++ b/drivers/platform/x86/fujitsu-laptop.c
> > > @@ -271,15 +271,19 @@ static int call_fext_func(int cmd, int arg0, int arg1, int arg2)
> > >  static int logolamp_set(struct led_classdev *cdev,
> > >                                enum led_brightness brightness)
> > >  {
> > > -       if (brightness >= LED_FULL) {
> > > -               call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_POWERON, FUNC_LED_ON);
> > > -               return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_ALWAYS, FUNC_LED_ON);
> > > -       } else if (brightness >= LED_HALF) {
> > > -               call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_POWERON, FUNC_LED_ON);
> > > -               return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_ALWAYS, FUNC_LED_OFF);
> > > -       } else {
> > > -               return call_fext_func(FUNC_LEDS, 0x1, LOGOLAMP_POWERON, FUNC_LED_OFF);
> > 
> > > +       int ret, poweron = FUNC_LED_OFF, always = FUNC_LED_OFF;
> > > +
> > > +       if (brightness >= LED_HALF) {
> > > +               poweron = FUNC_LED_ON;
> > > +               if (brightness >= LED_FULL)
> > > +                       always = FUNC_LED_ON;
> > >         }
> > 
> > But if you set ON by default the above could be written like
> > 
> >     if (brightness < LED_FULL)
> >         always = FUNC_LED_OFF;
> > 
> >     if (brightness < LED_HALF)
> >         poweron = FUNC_LED_OFF;
> 
> Ah, neat, thanks.  Jonathan, assuming you also like this, I will later
> submit v2 using this approach.

Yes, I like this.  It does in fact make the intent clearer.

The subsequent suggestion to split the "int ret, poweron ..." line is also
worthwhile since it makes one less likely to miss the declaration of "ret" at
the start.

Regards
  jonathan

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web