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

PATCH Superseded Michał Kępień 3 participants 6 messages Jan 5, 2017 – Jan 7, 2017

Subsystems (2)

x86 platform drivers Fujitsu laptop extras

lore

Share & tools

lore

Michał Kępień

author

2017-01-05 13:12 UTC #0 lore raw

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

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ń <[email protected]>

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

drivers/platform/x86/fujitsu-laptop.c +12 -8

index b725a907a91f..12f7a8346dd0 100644
@@ -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,

Signature · 1 line

2.11.0

Jonathan Woithe

maintainer Acked-by on v1 1/1

2017-01-05 14:08 UTC #1 lore raw

Quoted 8 lines · Micha?? K??pie??

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?? <[email protected]>

Acked-by Jonathan Woithe <[email protected]>

Quoted 47 lines

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

drivers/platform/x86/fujitsu-laptop.c +12 -8

index b725a907a91f..12f7a8346dd0 100644
@@ -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,

Signature · 1 line

2.11.0

Andy Shevchenko

2017-01-06 15:25 UTC #2 lore raw

In reply to Michał Kępień

Quoted 35 lines · Michał Kępień

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ń <[email protected]>
---
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(-)
drivers/platform/x86/fujitsu-laptop.c +0 -6
index b725a907a91f..12f7a8346dd0 100644
@@ -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);

drivers/platform/x86/fujitsu-laptop.c static int call_fext_func(int cmd, int arg0, int arg1, int arg2) · line 281

Quoted 10 lines

drivers/platform/x86/fujitsu-laptop.c +6 -2

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

Quoted 12 lines

drivers/platform/x86/fujitsu-laptop.c +6 -0

+
+       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

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Michał Kępień

author

2017-01-06 19:24 UTC #3 lore raw

Quoted 47 lines · Michał Kępień

Quoted 35 lines
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ń <[email protected]>
---
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(-)
drivers/platform/x86/fujitsu-laptop.c +0 -6
index b725a907a91f..12f7a8346dd0 100644
@@ -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);
drivers/platform/x86/fujitsu-laptop.c static int call_fext_func(int cmd, int arg0, int arg1, int arg2) · line 281
Quoted 10 lines
drivers/platform/x86/fujitsu-laptop.c +6 -2
-       } 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;
        }

Quoted 7 lines

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.

Signature · 2 lines

Best regards,
Michał Kępień

Andy Shevchenko

2017-01-06 20:48 UTC #4 lore raw

Quoted 4 lines · Michał Kępień

Quoted 4 lines · Michał Kępień
Review
Quoted 3 lines
(quoted diff) +2 -0
+       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;

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Jonathan Woithe

maintainer

2017-01-07 01:02 UTC #5 lore raw

In reply to Michał Kępień

Quoted 49 lines · Micha?? K??pie??

Quoted 49 lines · Micha?? K??pie??
Quoted 35 lines
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?? <[email protected]>
---
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(-)
drivers/platform/x86/fujitsu-laptop.c +0 -6
index b725a907a91f..12f7a8346dd0 100644
@@ -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);
drivers/platform/x86/fujitsu-laptop.c static int call_fext_func(int cmd, int arg0, int arg1, int arg2) · line 281
Quoted 10 lines
drivers/platform/x86/fujitsu-laptop.c +6 -2
-       } 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

Quoted 8 lines

Quoted 5 lines
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