Hi,
On 04/12/2019 14:37, Pavel Machek wrote:
Hi!
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
JJ had to leave this unfinished, and now I'm trying to pick this work up.
How will this interact with LEDs that can be used from atomic context?
I think the idea here is that if the LED uses a regulator, and the regulator needs to be enabled,
then the whole work is scheduled. If there's no regulator or if the regulator is already enabled,
the brightness is set directly.
+static ssize_t regulator_auto_off_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t size)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ ssize_t ret = size;
+ bool auto_off;
+
+ if (strncmp(buf, "enable\n", size) == 0)
+ auto_off = true;
+ else if (strncmp(buf, "disable\n", size) == 0)
+ auto_off = false;
+ else
+ return -EINVAL;
Sounds like device power management to me. Is it compatible with that?
Can you elaborate what you mean?
@@ -135,6 +203,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);
}
You only modify set_brigthness_delays, so this will not work at all
for non-blocking LEDs, right?
I'm not that familiar with led framework yet, but if setting of the brightness always goes via
led_set_brightness_nopm(), then I think this would work for all LEDs, as led_set_brightness_nopm
will schedule the work if needed (which goes to set_brightness_delayed).
static void led_set_software_blink(struct led_classdev *led_cdev,
@@ -189,6 +259,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);
Could this re-use the workqueue? Many systems will not need
regulators, so this is overhead...
You mean re-use the 'set_brightness_work' work? I'm not sure how that would be done, they're a bit
different things. set_brightness_work is used to change the context from atomic to non-atomic, and
reg_off_work is used to turn the regulator off after a specified delay.
+ /*
+ * the regulator must be turned off. This cannot be
Use "The", and fix double spaces between must and be.
+ } else if (regulator_on && old == REG_R_OFF_U_OFF) {
+ /*
+ * the regulator must be enabled. This cannot be here
"The"
+ /*
+ * small optimization. Cancel the work that had been started
"Small."
+#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
+};
That's quite weird use of enum.
Yes, these could as well be defines, if that's what you mean.
+++ b/include/linux/leds.h
@@ -149,6 +149,15 @@ struct led_classdev {
/* Ensures consistent access to the LED Flash Class device */
struct mutex led_access;
+
+ /* regulator */
"Regulator".
Fixed this and the other cosmetic changes, thanks!
Overall, I wonder if all this is worth it as this is somewhat complex change, compared to just
having the regulator always enabled...
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
