[PATCH v6 0/2] leds: Add control of the voltage/current regulator to the LED core

[PATCH v6 0/2] leds: Add control of the voltage/current regulator to the LED core

[Jean-Jacques Hiblot]

This series makes it possible for the LED core to manage the power supply
of a LED. It uses the regulator API to disable/enable the power if when the
LED is turned on/off.
This is especially useful in situations where the LED driver/controller is
not supplying the power.
Because updating a regulator state can block, it is always a defered job.

Note: this series relies on led_cdev->dev->of_node being populated [0]

[0] https://lkml.org/lkml/2019/10/3/139

changes in v6:
- Introduce a new property in DT binding to delay turning OFF the regulator
  The idea is to keep the regulator ON for some time after the LED is turned
  off in order to not change the regulator state when the LED is blinking.
- Use an atomic to track the state of the regulator to ensure consistency.
- Remove changes in led_set_brightness_sync().

changes in v5:
- fixed build error in led_set_brightness_sync(). Explain the role of
  flush__work()

changes in v4:
- Add a new patch to make led_set_brightness_sync() use
  led_set_brightness_nosleep() and then wait the work to be done
- Rework how the core knows how the regulator needs to be updated.

changes in v3:
- reword device-tree description
- reword commit log
- remove regulator updates from functions used in atomic context. If the
  regulator must be updated, it is defered to a workqueue.
- Fix led_set_brightness_sync() to work with the non-blocking function
  __led_set_brightness()

changes in v2:
- use devm_regulator_get_optional() to avoid using the dummy regulator and
  do some unnecessary work

Jean-Jacques Hiblot (2):
  dt-bindings: leds: document the "power-supply" property
  leds: Add control of the voltage/current regulator to the LED core

 .../devicetree/bindings/leds/common.txt       |  14 ++
 drivers/leds/led-class.c                      |  21 +++
 drivers/leds/led-core.c                       | 122 +++++++++++++++++-
 drivers/leds/leds.h                           |  18 +++
 include/linux/leds.h                          |   8 ++
 5 files changed, 181 insertions(+), 2 deletions(-)

-- 
2.17.1

[PATCH v6 1/2] dt-bindings: leds: document the "power-supply" property

[Jean-Jacques Hiblot]

Most of the LEDs are powered by a voltage/current regulator. Describing it
in the device-tree makes it possible for the LED core to enable/disable it
when needed.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---
 Documentation/devicetree/bindings/leds/common.txt | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
index 9fa6f9795d50..28c0cbb7c6ab 100644
--- a/Documentation/devicetree/bindings/leds/common.txt
+++ b/Documentation/devicetree/bindings/leds/common.txt
@@ -77,6 +77,19 @@ Optional properties for child nodes:
 - panic-indicator : This property specifies that the LED should be used,
 		    if at all possible, as a panic indicator.
 
+- power-supply : A voltage/current regulator used to to power the LED. When a
+		 LED is turned off, the LED core disable its regulator. The
+		 same regulator can power many LED (or other) devices. It is
+		 turned off only when all of its users disabled it.
+
+- power-off-delays-ms: This property specifies the delay between the time a LED
+                       is turned off and the time the regulator is turned off.
+                       It can be used to limit the overhead of the regulator
+                       handling if the LED is toggling fast.
+                       ex: if power-off-delays-ms is set to 500 ms, the
+                       regulator will not be turned off until the LED is turned
+                       off for more than 500ms.
+
 - trigger-sources : List of devices which should be used as a source triggering
 		    this LED activity. Some LEDs can be related to a specific
 		    device and should somehow indicate its state. E.g. USB 2.0
@@ -124,6 +137,7 @@ led-controller@0 {
 		function = LED_FUNCTION_STATUS;
 		linux,default-trigger = "heartbeat";
 		gpios = <&gpio0 0 GPIO_ACTIVE_HIGH>;
+		power-supply = <&led_regulator>;
 	};
 
 	led1 {
-- 
2.17.1

[PATCH v6 2/2] leds: Add control of the voltage/current regulator to the LED core

[Jean-Jacques Hiblot]

A LED is usually powered by a voltage/current regulator. Let the LED core
know about it. This allows the LED core to turn on or off the power supply
as needed.

Because turning ON/OFF a regulator might block, it is not done
synchronously but done in a workqueue. Turning ON the regulator is always
done as soon as possible, turning it off can be delayed by setting the
"power-off-delays-ms" property. The intent is to keep the regulator
powered on when the blink rate of the LED is high.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---
 drivers/leds/led-class.c |  21 +++++++
 drivers/leds/led-core.c  | 122 ++++++++++++++++++++++++++++++++++++++-
 drivers/leds/leds.h      |  18 ++++++
 include/linux/leds.h     |   8 +++
 4 files changed, 167 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 80c96dd96afc..47d9f920091c 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -344,6 +344,7 @@ int led_classdev_register_ext(struct device *parent,
 	char final_name[LED_MAX_NAME_SIZE];
 	const char *proposed_name = composed_name;
 	int ret;
+	struct regulator *regulator;
 
 	if (init_data) {
 		if (init_data->devname_mandatory && !init_data->devicename) {
@@ -379,6 +380,24 @@ int led_classdev_register_ext(struct device *parent,
 		dev_warn(parent, "Led %s renamed to %s due to name collision",
 				led_cdev->name, dev_name(led_cdev->dev));
 
+	regulator = devm_regulator_get_optional(led_cdev->dev, "power");
+	if (IS_ERR(regulator)) {
+		if (regulator != ERR_PTR(-ENODEV)) {
+			dev_err(led_cdev->dev, "Cannot get the power supply for %s\n",
+				led_cdev->name);
+			device_unregister(led_cdev->dev);
+			mutex_unlock(&led_cdev->led_access);
+			return PTR_ERR(regulator);
+		}
+		led_cdev->regulator = NULL;
+	} else {
+		mutex_init(&led_cdev->regulator_access);
+		led_cdev->regulator = regulator;
+		atomic_set(&led_cdev->regulator_state, REG_R_OFF_U_OFF);
+		fwnode_property_read_u32(init_data->fwnode, "off-delay-ms",
+					 &led_cdev->reg_off_delay);
+	}
+
 	if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) {
 		ret = led_add_brightness_hw_changed(led_cdev);
 		if (ret) {
@@ -458,6 +477,8 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
 	up_write(&leds_list_lock);
 
 	mutex_destroy(&led_cdev->led_access);
+	if (led_cdev->regulator)
+		mutex_destroy(&led_cdev->regulator_access);
 }
 EXPORT_SYMBOL_GPL(led_classdev_unregister);
 
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index f1f718dbe0f8..81105c1ee907 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -37,6 +37,73 @@ const char * const led_colors[LED_COLOR_ID_MAX] = {
 };
 EXPORT_SYMBOL_GPL(led_colors);
 
+static void turn_off_regulator_if_needed(struct led_classdev *led_cdev)
+{
+	int old = atomic_cmpxchg(&led_cdev->regulator_state, REG_R_ON_U_OFF,
+				  REG_R_OFF_U_OFF);
+	if (old == REG_R_ON_U_OFF) {
+		int rc = regulator_disable(led_cdev->regulator);
+
+		if (rc) {
+			dev_err(led_cdev->dev,
+				"Failed to disable the regulator (%d)\n", rc);
+			atomic_or(REG_R_ON, &led_cdev->regulator_state);
+		}
+	}
+}
+
+static void try_turn_on_regulator_if_needed(struct led_classdev *led_cdev)
+{
+	int old = atomic_cmpxchg(&led_cdev->regulator_state, REG_R_OFF_U_ON,
+			      REG_R_ON_U_ON);
+	if (old == REG_R_OFF_U_ON) {
+		int rc = regulator_enable(led_cdev->regulator);
+
+		if (rc) {
+			dev_err(led_cdev->dev,
+				"Failed to enable the regulator (%d)\n", rc);
+			atomic_and(~REG_R_ON, &led_cdev->regulator_state);
+		}
+	}
+}
+
+static void turn_off_regulator_delayed(struct work_struct *ws)
+{
+	struct led_classdev *led_cdev =
+		container_of(ws, struct led_classdev, reg_off_work.work);
+
+	/*
+	 * Take the lock to prevent very unlikely updates due to
+	 * led_handle_regulator() being executed at the same time
+	 */
+	mutex_lock(&led_cdev->regulator_access);
+
+	turn_off_regulator_if_needed(led_cdev);
+
+	mutex_unlock(&led_cdev->regulator_access);
+}
+
+static void led_handle_regulator(struct led_classdev *led_cdev)
+{
+	int delay = (led_cdev->flags & LED_SUSPENDED) ? 0 :
+		    led_cdev->reg_off_delay;
+
+	if (!led_cdev->regulator)
+		return;
+
+	/*
+	 * Take the lock to prevent very unlikely updates due to
+	 * turn_off_regulator_delayed() being executed at the same time
+	 */
+	mutex_lock(&led_cdev->regulator_access);
+
+	try_turn_on_regulator_if_needed(led_cdev);
+	if (!delay)
+		turn_off_regulator_if_needed(led_cdev);
+
+	mutex_unlock(&led_cdev->regulator_access);
+}
+
 static int __led_set_brightness(struct led_classdev *led_cdev,
 				enum led_brightness value)
 {
@@ -135,6 +202,8 @@ static void set_brightness_delayed(struct work_struct *ws)
 	    (led_cdev->flags & LED_HW_PLUGGABLE)))
 		dev_err(led_cdev->dev,
 			"Setting an LED's brightness failed (%d)\n", ret);
+
+	 led_handle_regulator(led_cdev);
 }
 
 static void led_set_software_blink(struct led_classdev *led_cdev,
@@ -189,6 +258,7 @@ static void led_blink_setup(struct led_classdev *led_cdev,
 void led_init_core(struct led_classdev *led_cdev)
 {
 	INIT_WORK(&led_cdev->set_brightness_work, set_brightness_delayed);
+	INIT_DELAYED_WORK(&led_cdev->reg_off_work, turn_off_regulator_delayed);
 
 	timer_setup(&led_cdev->blink_timer, led_timer_function, 0);
 }
@@ -269,8 +339,56 @@ EXPORT_SYMBOL_GPL(led_set_brightness);
 void led_set_brightness_nopm(struct led_classdev *led_cdev,
 			      enum led_brightness value)
 {
-	/* Use brightness_set op if available, it is guaranteed not to sleep */
-	if (!__led_set_brightness(led_cdev, value))
+	bool defer_work = false;
+	bool regulator_on = !!value;
+
+
+	if (led_cdev->regulator) {
+		int old;
+
+		if (regulator_on)
+			old = atomic_fetch_or(REG_U_ON,
+					      &led_cdev->regulator_state);
+		else
+			old = atomic_fetch_and(~REG_U_ON,
+					       &led_cdev->regulator_state);
+
+		if (!regulator_on && old == REG_R_ON_U_ON) {
+			int delay = (led_cdev->flags & LED_SUSPENDED) ? 0 :
+				    led_cdev->reg_off_delay;
+
+			/*
+			 * the regulator must  be turned off. This cannot be
+			 * here because we can't sleep. Defer the job or, if a
+			 * delay is asked for, schedule a delayed work
+			 */
+			if (delay)
+				schedule_delayed_work(
+						 &led_cdev->reg_off_work,
+						 msecs_to_jiffies(delay));
+			else
+				defer_work = true;
+		} else if (regulator_on && old == REG_R_OFF_U_OFF) {
+			/*
+			 * the regulator must be enabled. This cannot be here
+			 * because we can't sleep. Defer the job
+			 */
+			defer_work = true;
+		}
+		/*
+		 * small optimization. Cancel the work that had been started
+		 * to turn OFF the regulator. It wouldn't have been turned off
+		 * anyway because regulator_state has changed to REG_R_x_U_ON
+		 */
+		if (regulator_on && old == REG_R_ON_U_OFF)
+			cancel_delayed_work(&led_cdev->reg_off_work);
+	}
+
+	/*
+	 * If defered work is not asked for, use brightness_set
+	 * op now if available, it is guaranteed not to sleep
+	 */
+	if (!defer_work && !__led_set_brightness(led_cdev, value))
 		return;
 
 	/* If brightness setting can sleep, delegate it to a work queue task */
diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
index 0b577cece8f7..5cb76e547b7d 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -11,6 +11,24 @@
 
 #include <linux/rwsem.h>
 #include <linux/leds.h>
+#include <linux/regulator/consumer.h>
+
+/*
+ * The regulator state tracks 2 boolean variables:
+ * - the state of regulator (or more precisely the state required by
+ *   led core layer, as many users can interact with the same regulator).
+ *   It is tracked by bit 0.
+ * - the state last asked-for by the LED user. It is tracked by bit 1.
+ */
+#define REG_R_ON BIT(0)
+#define REG_U_ON BIT(1)
+
+enum {	REG_R_OFF_U_OFF = 0,
+	REG_R_ON_U_OFF = REG_R_ON,
+	REG_R_OFF_U_ON = REG_U_ON,
+	REG_R_ON_U_ON = REG_R_ON | REG_U_ON
+};
+
 
 static inline int led_get_brightness(struct led_classdev *led_cdev)
 {
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 9b94cf752012..90afbbc67bb0 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -149,6 +149,14 @@ struct led_classdev {
 
 	/* Ensures consistent access to the LED Flash Class device */
 	struct mutex		led_access;
+
+	/* regulator */
+	struct regulator	*regulator;
+	struct mutex		regulator_access;
+	atomic_t		regulator_state;
+	int			reg_off_delay;
+	struct delayed_work	reg_off_work;
+
 };
 
 /**
-- 
2.17.1

Re: [PATCH v6 2/2] leds: Add control of the voltage/current regulator to the LED core

[kbuild test robot]

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

Hi Jean-Jacques,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on j.anaszewski-leds/for-next]
[cannot apply to v5.4-rc1 next-20191004]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Jean-Jacques-Hiblot/leds-Add-control-of-the-voltage-current-regulator-to-the-LED-core/20191005-062631
base:   https://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git for-next
config: sparc-allyesconfig (attached as .config)
compiler: sparc64-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=sparc 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/leds/led-core.c: In function 'try_turn_on_regulator_if_needed':
>> drivers/leds/led-core.c:65:15: warning: overflow in implicit constant conversion [-Woverflow]
       atomic_and(~REG_R_ON, &led_cdev->regulator_state);
                  ^
   drivers/leds/led-core.c: In function 'led_set_brightness_nopm':
   drivers/leds/led-core.c:353:27: warning: overflow in implicit constant conversion [-Woverflow]
       old = atomic_fetch_and(~REG_U_ON,
                              ^

vim +65 drivers/leds/led-core.c

    54	
    55	static void try_turn_on_regulator_if_needed(struct led_classdev *led_cdev)
    56	{
    57		int old = atomic_cmpxchg(&led_cdev->regulator_state, REG_R_OFF_U_ON,
    58				      REG_R_ON_U_ON);
    59		if (old == REG_R_OFF_U_ON) {
    60			int rc = regulator_enable(led_cdev->regulator);
    61	
    62			if (rc) {
    63				dev_err(led_cdev->dev,
    64					"Failed to enable the regulator (%d)\n", rc);
  > 65				atomic_and(~REG_R_ON, &led_cdev->regulator_state);
    66			}
    67		}
    68	}
    69	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 59363 bytes --]