Skip to contentSkip to conversation

leds: remove redundant SET_BRIGHTNESS_SYNC flag

PATCH In review Stas Sergeev 3 participants 4 messages May 13, 2015 – May 14, 2015 LED

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 leds: remove redundant SET_BRIGHTNESS_SYNC flag

There is a complimentary flag called SET_BRIGHTNESS_ASYNC. Having both is redundant. This patch removes the unneeded flag without any functionality change.

---
 drivers/leds/led-class-flash.c |    1 -
 drivers/leds/led-core.c        |    4 +---
 include/linux/leds.h           |    2 +-
 3 files changed, 2 insertions(+), 5 deletions(-)
drivers/leds/led-class-flash.c +0 -1
index 3b25734..bb67364 100644
@@ -318,7 +318,6 @@ int led_classdev_flash_register(struct device *parent,
 
 	/* Setting a torch brightness needs to have immediate effect */
 	led_cdev->flags &= ~SET_BRIGHTNESS_ASYNC;
-	led_cdev->flags |= SET_BRIGHTNESS_SYNC;
 
 	return 0;
 }
drivers/leds/led-core.c +1 -3
index 9886dac..356e851 100644
@@ -129,10 +129,8 @@ void led_set_brightness(struct led_classdev *led_cdev,
 	if (led_cdev->flags & SET_BRIGHTNESS_ASYNC) {
 		led_set_brightness_async(led_cdev, brightness);
 		return;
-	} else if (led_cdev->flags & SET_BRIGHTNESS_SYNC)
+	} else
 		ret = led_set_brightness_sync(led_cdev, brightness);
-	else
-		ret = -EINVAL;
 
 	if (ret < 0)
 		dev_dbg(led_cdev->dev, "Setting LED brightness failed (%d)\n",
include/linux/leds.h +1 -1
index 9a2b000..c9e6e5d 100644
@@ -45,7 +45,7 @@ struct led_classdev {
 #define LED_BLINK_INVERT	(1 << 19)
 #define LED_SYSFS_DISABLE	(1 << 20)
 #define SET_BRIGHTNESS_ASYNC	(1 << 21)
-#define SET_BRIGHTNESS_SYNC	(1 << 22)
+/* bit 22 unused, take it */
 #define LED_DEV_CAP_FLASH	(1 << 23)
 
 	/* Set LED brightness level */
Signature · 1 line
1.7.9.5
#1 lore raw

I personally can't find any strong argument against this optimization. I added both flags on Bryan's request while implementing LED Flash class extension. I can see a few occurrences in the include/linux directory where both SYNC and ASYNC flags are implemented. The argument in favour could be the fact that the flags influence the core functionality of the subsystem.

Bryan, could you express your opinion, please?

Quoted 59 lines · Stas Sergeev

There is a complimentary flag called SET_BRIGHTNESS_ASYNC. Having both is redundant. This patch removes the unneeded flag without any functionality change.

---
  drivers/leds/led-class-flash.c |    1 -
  drivers/leds/led-core.c        |    4 +---
  include/linux/leds.h           |    2 +-
  3 files changed, 2 insertions(+), 5 deletions(-)
drivers/leds/led-class-flash.c +0 -1
index 3b25734..bb67364 100644
@@ -318,7 +318,6 @@ int led_classdev_flash_register(struct device *parent,
 
  	/* Setting a torch brightness needs to have immediate effect */
  	led_cdev->flags &= ~SET_BRIGHTNESS_ASYNC;
-	led_cdev->flags |= SET_BRIGHTNESS_SYNC;
 
  	return 0;
  }
drivers/leds/led-core.c +1 -3
index 9886dac..356e851 100644
@@ -129,10 +129,8 @@ void led_set_brightness(struct led_classdev *led_cdev,
  	if (led_cdev->flags & SET_BRIGHTNESS_ASYNC) {
  		led_set_brightness_async(led_cdev, brightness);
  		return;
-	} else if (led_cdev->flags & SET_BRIGHTNESS_SYNC)
+	} else
  		ret = led_set_brightness_sync(led_cdev, brightness);
-	else
-		ret = -EINVAL;
 
  	if (ret < 0)
  		dev_dbg(led_cdev->dev, "Setting LED brightness failed (%d)\n",
include/linux/leds.h +1 -1
index 9a2b000..c9e6e5d 100644
@@ -45,7 +45,7 @@ struct led_classdev {
  #define LED_BLINK_INVERT	(1 << 19)
  #define LED_SYSFS_DISABLE	(1 << 20)
  #define SET_BRIGHTNESS_ASYNC	(1 << 21)
-#define SET_BRIGHTNESS_SYNC	(1 << 22)
+/* bit 22 unused, take it */
  #define LED_DEV_CAP_FLASH	(1 << 23)
 
  	/* Set LED brightness level */
Signature · 2 lines
Best Regards,
Jacek Anaszewski
#2 lore raw
Quoted 4 lines · Jacek Anaszewski

I personally can't find any strong argument against this optimization. I added both flags on Bryan's request while implementing LED Flash class extension. I can see a few occurrences in the include/linux directory where both SYNC and ASYNC flags are implemented. The argument in favour

I meant: "in favour of keeping that as is"

Quoted 70 lines

could be the fact that the flags influence the core functionality of the subsystem.

Bryan, could you express your opinion, please?

Quoted 62 lines · Stas Sergeev

There is a complimentary flag called SET_BRIGHTNESS_ASYNC. Having both is redundant. This patch removes the unneeded flag without any functionality change.

---
  drivers/leds/led-class-flash.c |    1 -
  drivers/leds/led-core.c        |    4 +---
  include/linux/leds.h           |    2 +-
  3 files changed, 2 insertions(+), 5 deletions(-)
drivers/leds/led-class-flash.c +0 -1
index 3b25734..bb67364 100644
@@ -318,7 +318,6 @@ int led_classdev_flash_register(struct device
 *parent,
 
      /* Setting a torch brightness needs to have immediate effect */
      led_cdev->flags &= ~SET_BRIGHTNESS_ASYNC;
-    led_cdev->flags |= SET_BRIGHTNESS_SYNC;
 
      return 0;
  }
drivers/leds/led-core.c +1 -3
index 9886dac..356e851 100644
@@ -129,10 +129,8 @@ void led_set_brightness(struct led_classdev
 *led_cdev,
      if (led_cdev->flags & SET_BRIGHTNESS_ASYNC) {
          led_set_brightness_async(led_cdev, brightness);
          return;
-    } else if (led_cdev->flags & SET_BRIGHTNESS_SYNC)
+    } else
          ret = led_set_brightness_sync(led_cdev, brightness);
-    else
-        ret = -EINVAL;
 
      if (ret < 0)
          dev_dbg(led_cdev->dev, "Setting LED brightness failed (%d)\n",
include/linux/leds.h +1 -1
index 9a2b000..c9e6e5d 100644
@@ -45,7 +45,7 @@ struct led_classdev {
  #define LED_BLINK_INVERT    (1 << 19)
  #define LED_SYSFS_DISABLE    (1 << 20)
  #define SET_BRIGHTNESS_ASYNC    (1 << 21)
-#define SET_BRIGHTNESS_SYNC    (1 << 22)
+/* bit 22 unused, take it */
  #define LED_DEV_CAP_FLASH    (1 << 23)
 
      /* Set LED brightness level */
Signature · 2 lines
Best Regards,
Jacek Anaszewski
#3 lore raw
In reply to Jacek Anaszewski
Quoted 9 lines · Jacek Anaszewski

I personally can't find any strong argument against this optimization. I added both flags on Bryan's request while implementing LED Flash class extension. I can see a few occurrences in the include/linux directory where both SYNC and ASYNC flags are implemented. The argument in favour could be the fact that the flags influence the core functionality of the subsystem.

Bryan, could you express your opinion, please?

Please keep this flag and although it's redundant at this point I think it's still make code more clear and easy to understand.

Thanks,
-Bryan

Quoted 70 lines · Stas Sergeev
Quoted 55 lines

There is a complimentary flag called SET_BRIGHTNESS_ASYNC. Having both is redundant. This patch removes the unneeded flag without any functionality change.

---
  drivers/leds/led-class-flash.c |    1 -
  drivers/leds/led-core.c        |    4 +---
  include/linux/leds.h           |    2 +-
  3 files changed, 2 insertions(+), 5 deletions(-)
drivers/leds/led-class-flash.c +0 -1
index 3b25734..bb67364 100644
@@ -318,7 +318,6 @@ int led_classdev_flash_register(struct device *parent,
 
        /* Setting a torch brightness needs to have immediate effect */
        led_cdev->flags &= ~SET_BRIGHTNESS_ASYNC;
-       led_cdev->flags |= SET_BRIGHTNESS_SYNC;
 
        return 0;
  }
drivers/leds/led-core.c +1 -3
index 9886dac..356e851 100644
@@ -129,10 +129,8 @@ void led_set_brightness(struct led_classdev
 *led_cdev,
        if (led_cdev->flags & SET_BRIGHTNESS_ASYNC) {
                led_set_brightness_async(led_cdev, brightness);
                return;
-       } else if (led_cdev->flags & SET_BRIGHTNESS_SYNC)
+       } else
                ret = led_set_brightness_sync(led_cdev, brightness);
-       else
-               ret = -EINVAL;
 
        if (ret < 0)
                dev_dbg(led_cdev->dev, "Setting LED brightness failed

(%d)\n",

include/linux/leds.h +0 -0
index 9a2b000..c9e6e5d 100644

include/linux/leds.h struct led_classdev { · line 51

Quoted 8 lines
include/linux/leds.h +1 -1
@@ -45,7 +45,7 @@ struct led_classdev {
  #define LED_BLINK_INVERT      (1 << 19)
  #define LED_SYSFS_DISABLE     (1 << 20)
  #define SET_BRIGHTNESS_ASYNC  (1 << 21)
-#define SET_BRIGHTNESS_SYNC    (1 << 22)
+/* bit 22 unused, take it */
  #define LED_DEV_CAP_FLASH     (1 << 23)
 
        /* Set LED brightness level */

--
Best Regards,
Jacek Anaszewski